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

[carry] Automatically detect default git branch #2228

Merged
merged 4 commits into from
Jul 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 32 additions & 10 deletions source/git/gitsource.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (
)

var validHex = regexp.MustCompile(`^[a-f0-9]{40}$`)
var defaultBranch = regexp.MustCompile(`refs/heads/(\S+)`)

type Opt struct {
CacheAccessor cache.Accessor
Expand Down Expand Up @@ -307,14 +308,10 @@ func (gs *gitSourceHandler) mountKnownHosts(ctx context.Context) (string, func()

func (gs *gitSourceHandler) CacheKey(ctx context.Context, g session.Group, index int) (string, solver.CacheOpts, bool, error) {
remote := gs.src.Remote
ref := gs.src.Ref
if ref == "" {
ref = "master"
}
gs.locker.Lock(remote)
defer gs.locker.Unlock(remote)

if isCommitSHA(ref) {
if ref := gs.src.Ref; ref != "" && isCommitSHA(ref) {
ref = gs.shaToCacheKey(ref)
gs.cacheKey = ref
return ref, nil, true, nil
Expand Down Expand Up @@ -348,6 +345,14 @@ func (gs *gitSourceHandler) CacheKey(ctx context.Context, g session.Group, index
defer unmountKnownHosts()
}

ref := gs.src.Ref
if ref == "" {
ref, err = getDefaultBranch(ctx, gitDir, "", sock, knownHosts, gs.auth, gs.src.Remote)
if err != nil {
return "", nil, false, err
}
}

// TODO: should we assume that remote tag is immutable? add a timer?

buf, err := gitWithinDir(ctx, gitDir, "", sock, knownHosts, gs.auth, "ls-remote", "origin", ref)
Expand All @@ -370,11 +375,6 @@ func (gs *gitSourceHandler) CacheKey(ctx context.Context, g session.Group, index
}

func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (out cache.ImmutableRef, retErr error) {
ref := gs.src.Ref
if ref == "" {
ref = "master"
}

cacheKey := gs.cacheKey
if cacheKey == "" {
var err error
Expand Down Expand Up @@ -426,6 +426,14 @@ func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (out
defer unmountKnownHosts()
}

ref := gs.src.Ref
if ref == "" {
ref, err = getDefaultBranch(ctx, gitDir, "", sock, knownHosts, gs.auth, gs.src.Remote)
if err != nil {
return nil, err
}
}

doFetch := true
if isCommitSHA(ref) {
// skip fetch if commit already exists
Expand Down Expand Up @@ -686,3 +694,17 @@ func tokenScope(remote string) string {
}
return remote
}

// getDefaultBranch gets the default branch of a repository using ls-remote
func getDefaultBranch(ctx context.Context, gitDir, workDir, sshAuthSock, knownHosts string, auth []string, remoteURL string) (string, error) {
buf, err := gitWithinDir(ctx, gitDir, workDir, sshAuthSock, knownHosts, auth, "ls-remote", "--symref", remoteURL, "HEAD")
if err != nil {
Copy link

Choose a reason for hiding this comment

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

I think it would be helpful to include a hint on how you would set the branch being used (add #branchname to URL). If greenlit, I can make a PR for the hint in error messages.

return "", errors.Wrapf(err, "error fetching default branch for repository %s", redactCredentials(remoteURL))
Copy link
Member

Choose a reason for hiding this comment

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

Can we fallback to "master" on error?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you know a case that wouldn't work? The problem is that this is the first network request via git so there is actually a good chance that this will legitimately fail and I wouldn't want to hide the error.

Copy link
Member

@AkihiroSuda AkihiroSuda Jul 7, 2021

Choose a reason for hiding this comment

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

When was ls-remote introduced? (EDIT: no later than 2007: git/git@7c2c6ee)

Some git server implementation may not support ls-remote?

Copy link
Member Author

Choose a reason for hiding this comment

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

We already had a dependency on ls-remote before to get the sha for the ref.

}

ss := defaultBranch.FindAllStringSubmatch(buf.String(), -1)
if len(ss) == 0 || len(ss[0]) != 2 {
return "", errors.Errorf("could not find default branch for repository: %s", redactCredentials(remoteURL))
}
return ss[0][1], nil
}
1 change: 1 addition & 0 deletions source/git/gitsource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,7 @@ func setupGitRepo(dir string) (string, error) {
"git submodule add "+subPath+" sub",
"git add -A",
"git commit -m withsub",
"git checkout master",
); err != nil {
return "", err
}
Expand Down
2 changes: 1 addition & 1 deletion source/gitidentifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func isGitTransport(str string) bool {

func getRefAndSubdir(fragment string) (ref string, subdir string) {
refAndDir := strings.SplitN(fragment, ":", 2)
ref = "master"
ref = ""
if len(refAndDir[0]) != 0 {
ref = refAndDir[0]
}
Expand Down
8 changes: 4 additions & 4 deletions source/gitidentifier_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ func TestNewGitIdentifier(t *testing.T) {
gi, err := NewGitIdentifier("ssh://[email protected]:2222/root/my/really/weird/path/foo.git")
require.Nil(t, err)
require.Equal(t, "ssh://[email protected]:2222/root/my/really/weird/path/foo.git", gi.Remote)
require.Equal(t, "master", gi.Ref)
require.Equal(t, "", gi.Ref)
require.Equal(t, "", gi.Subdir)

gi, err = NewGitIdentifier("ssh://[email protected]:2222/root/my/really/weird/path/foo.git#main")
Expand All @@ -22,13 +22,13 @@ func TestNewGitIdentifier(t *testing.T) {
gi, err = NewGitIdentifier("[email protected]:moby/buildkit.git")
require.Nil(t, err)
require.Equal(t, "[email protected]:moby/buildkit.git", gi.Remote)
require.Equal(t, "master", gi.Ref)
require.Equal(t, "", gi.Ref)
require.Equal(t, "", gi.Subdir)

gi, err = NewGitIdentifier("github.com/moby/buildkit.git")
gi, err = NewGitIdentifier("github.com/moby/buildkit.git#main")
require.Nil(t, err)
require.Equal(t, "https://github.com/moby/buildkit.git", gi.Remote)
require.Equal(t, "master", gi.Ref)
require.Equal(t, "main", gi.Ref)
require.Equal(t, "", gi.Subdir)

}