Fix git clone failing when default branch is not master#29004
Fix git clone failing when default branch is not master#29004chrisnojima wants to merge 10 commits intomasterfrom
Conversation
When a user pushes only a non-master branch (e.g. main) to a Keybase git repo, cloning fails with "remote HEAD refers to nonexistent ref, unable to checkout" because gogit.Init() hardcodes HEAD to refs/heads/master. Three fixes: - handleList: validate symref targets exist, rewrite HEAD to best available branch (prefer main > master > alphabetical first) - handlePushBatch: after push, update stored HEAD if it points to a nonexistent ref (skipping delete refspecs) - NewBrowser: resolve HEAD dynamically instead of hardcoding master, with fallback to empty browser for stale HEAD Fixes #28943
There was a problem hiding this comment.
Pull request overview
Fixes Keybase git clone/browse behavior when the repo’s default branch is not master by ensuring HEAD symrefs are valid and by resolving the default branch dynamically instead of assuming refs/heads/master.
Changes:
kbfsgitremote-helperlist: validate/repair symbolic refs (notablyHEAD) to point at an existing branch, preferringmainthenmasterthen alphabetical.kbfsgitremote-helperpush: attempt to repair storedHEADafter a push when it points at a nonexistent ref.libgitbrowser: when no branch is specified, resolveHEADto determine the default branch rather than hardcodingmaster, and tolerate staleHEAD.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
go/kbfs/libgit/browser.go |
Resolves HEAD to pick a default branch when none is provided; treats stale/missing HEAD targets as an empty browser. |
go/kbfs/kbfsgit/runner.go |
Rewrites invalid symref targets during list and repairs HEAD after push if needed. |
go/kbfs/kbfsgit/runner_test.go |
Adds a regression test covering the “only main exists” case for list/HEAD. |
Comments suppressed due to low confidence (1)
go/kbfs/libgit/browser.go:119
NewBrowsernow changes behavior whengitBranchNameis empty by resolvingHEADand (if missing) treating the repo as empty. There are existingbrowser_test.gotests, but none appear to cover this new HEAD-resolution path (e.g., HEAD -> refs/heads/main browsing, or stale HEAD -> nonexistent ref returning an empty browser). Adding a unit test for these cases would help prevent regressions.
// If no branch was specified, try to resolve HEAD to find the
// default branch instead of hardcoding master.
if branchWasEmpty {
headRef, headErr := repo.Reference(plumbing.HEAD, false)
if headErr == nil && headRef.Type() == plumbing.SymbolicReference {
gitBranchName = headRef.Target()
}
}
ref, err := repo.Reference(gitBranchName, true)
if err == plumbing.ErrReferenceNotFound && (gitBranchName == masterBranch || branchWasEmpty) {
// This branch has no commits (or HEAD points to a
// nonexistent ref), so pretend it's empty.
return &Browser{
root: string(gitBranchName),
sharedCache: sharedCache,
}, nil
} else if err != nil {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… test - Use main > master > alphabetical preference for HEAD repair branch selection instead of nondeterministic map iteration order - Add waitForJournal after HEAD repair so the update is flushed - Add TestBrowserHeadResolution covering non-master HEAD and stale HEAD
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…arden test - Extract bestBranchFromCandidates helper to deduplicate HEAD selection logic - Move HEAD fixup before waitForJournal so the update is included in the flush - Iterate args slice instead of refspecs map for deterministic branch selection - Propagate journal flush error instead of swallowing it - Use git checkout -B (idempotent) to avoid flakiness with init.defaultBranch - Add assertion verifying the underlying KBFS repo HEAD symref is updated
…PushBatch The HEAD fixup was only considering refs from the current push batch when selecting the best branch. This could pick a suboptimal branch if better candidates (e.g. main) already existed in the repo. Now enumerates all repo branches, consistent with how handleList selects the best HEAD target.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
go/kbfs/libgit/browser.go:81
NewBrowsernow resolves the default branch fromHEADwhengitBranchNameis empty, but the function-level comment and themasterBranchfallback logic still describe "empty => refs/heads/master" behavior. Please update theNewBrowsercomment to reflect the new resolution order (HEAD target first, then master fallback) so callers understand the default branch semantics.
const masterBranch = "refs/heads/master"
branchWasEmpty := gitBranchName == ""
if gitBranchName == "" {
gitBranchName = masterBranch
} else if !strings.HasPrefix(string(gitBranchName), "refs/") {
gitBranchName = "refs/heads/" + gitBranchName
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Move bestBranchFromCandidates above handleList so its doc comment is not split - Add defer refs.Close() in handleList - Add defer allRefs.Close() in handlePushBatch HEAD fixup
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var bestBranch plumbing.ReferenceName | ||
| allRefs, refsErr := repo.References() | ||
| if refsErr == nil { | ||
| defer allRefs.Close() |
There was a problem hiding this comment.
move to before the error check?
When a user pushes only a non-master branch (e.g. main) to a Keybase git repo, cloning fails with "remote HEAD refers to nonexistent ref, unable to checkout" because gogit.Init() hardcodes HEAD to refs/heads/master.
Three fixes:
Fixes #28943