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

Try to clone 'main' first. #13

Merged
merged 2 commits into from
Jan 31, 2022
Merged

Conversation

floitsch
Copy link
Member

The git-library doesn't support cloning repositories that use 'main' as
default branch: go-git/go-git#363

We therefore try 'main', 'master' and 'trunk'.

Until now we tried 'master' first, but since the Toit registry already
uses 'main' that leads to an unnecessary clone-attempt. It also allows a
race condition where another toit.pkg command could try to pull the
'master' branch.

The git-library doesn't support cloning repositories that use 'main' as
default branch: go-git/go-git#363

We therefore try 'main', 'master' and 'trunk'.

Until now we tried 'master' first, but since the Toit registry already
uses 'main' that leads to an unnecessary clone-attempt. It also allows a
race condition where another toit.pkg command could try to pull the
'master' branch.
@floitsch floitsch requested a review from kasperl January 28, 2022 20:25
@cla-bot cla-bot bot added the cla-signed label Jan 28, 2022
Copy link
Member

@kasperl kasperl left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@@ -343,7 +343,7 @@ func (gr *gitRegistry) Load(ctx context.Context, sync bool, cache Cache, ui UI)
url := gr.url

var err error
for _, branch := range []string{"master", "main", "trunk"} {
for _, branch := range []string{"main", "master", "trunk"} {
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment that explains that the order matters here.

Copy link
Member Author

Choose a reason for hiding this comment

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

added a comment, but since the race-condition will disappear with the follow-up PRs, didn't explicitly say that the order matters (except for speed).

@floitsch floitsch merged commit 2b63ba5 into main Jan 31, 2022
@floitsch floitsch deleted the improve_registry_download.10.main_first branch January 31, 2022 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants