Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gitutil: find default remote by tracking branch #2146

Merged
merged 1 commit into from Dec 4, 2023

Conversation

tonistiigi
Copy link
Member

@tonistiigi tonistiigi commented Dec 1, 2023

Using this command resolves remote based on remote-tracking branch of the currently selected branch and should be more precise in case we can't predict if the user uses origin to mark upstream or their fork.

This reverses #1548 . The issue there isn't unfortunately very clear about what exact case did not show correct output before. It is possible it was just a misunderstanding about how ls-remote --get-url works.

https://github.com/git/git/blob/v2.43.0/remote.c#L550-L554

The "origin" fallback probably does not do anything anymore. 95% sure it is safe to remove.

Looking at the git source, another difference is that if there is only one remote, then ls-remote --get-url returns that(even if not remote-tracking) while the current implementation would not set URL in this case. If this is not the behavior we want, it could be detected with some additional commands or we could get the remote-tracking branch directly without ls-remote.

@tonistiigi tonistiigi marked this pull request as draft December 1, 2023 06:35
Using this command resolves remote based on remote
tracking branch of the curently selected branch and
should be more precise in case we can't predict if user
uses origin to mark upstream or their fork.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi
Copy link
Member Author

The test failed for the condition of a single remote without any remote-tracking so I switched the implementation to still check for remote-tracking branch but avoid using ls-remote.

@tonistiigi tonistiigi marked this pull request as ready for review December 1, 2023 07:36
Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it makes more sense

@crazy-max crazy-max merged commit 5b5c4c8 into docker:master Dec 4, 2023
61 checks passed
@crazy-max crazy-max added this to the v0.13.0 milestone Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants