diff --git a/pkg/commands/git_commands/branch_loader.go b/pkg/commands/git_commands/branch_loader.go index f42b66e2c..532167acf 100644 --- a/pkg/commands/git_commands/branch_loader.go +++ b/pkg/commands/git_commands/branch_loader.go @@ -494,8 +494,7 @@ func parseDifference(track string, regexStr string) string { // HEAD). Entries are fed in the order git produces them, i.e. newest first. type reflogEntry struct { hash string // the commit HEAD pointed at when this reflog action occurred - subject string - timestamp int64 // commit timestamp of the `hash` commit + timestamp int64 // commit timestamp of the `hash` commit from string // set on "checkout: moving from X to Y" lines to the source branch X; "" otherwise to string // set to the destination branch Y on checkout lines; "" otherwise } @@ -507,7 +506,7 @@ type reflogEntry struct { // (reconstructed from the reflog) is the commit it pointed at when it was // deleted. func (self *BranchLoader) GetDeletedBranches() ([]*models.DeletedBranch, error) { - currentBranches, err := self.getCurrentBranchNames() + existingRefs, err := self.getExistingRefNames() if err != nil { return nil, err } @@ -524,21 +523,40 @@ func (self *BranchLoader) GetDeletedBranches() ([]*models.DeletedBranch, error) } entries := parseReflogEntries(rawReflog) - return obtainDeletedBranches(entries, currentBranches), nil + return obtainDeletedBranches(entries, existingRefs, self.isValidRefFormat), nil } -// getCurrentBranchNames returns the short names of all local branches. -func (self *BranchLoader) getCurrentBranchNames() ([]string, error) { +// isValidRefFormat reports whether git accepts `name` as a valid ref name, +// deferring the ref-name grammar (e.g. rejecting "HEAD~1", "main@{0}", trailing +// dots) to `git check-ref-format`. We pass --allow-onelevel because reflog +// checkout names are bare branch names (e.g. "master"), which are single-level +// refs. Note that git's rules only cover shape: things like tags, remote-tracking +// branches and abbreviated SHAs are all valid refs to git, so the caller still +// has to filter those out separately. +func (self *BranchLoader) isValidRefFormat(name string) bool { + return self.cmd.New( + NewGitCmd("check-ref-format"). + Arg("--allow-onelevel"). + Arg(name). + ToArgv(), + ).DontLog().Run() == nil +} + +// getExistingRefNames returns the short names of all refs (local branches, +// remote-tracking branches and tags) plus HEAD itself. A name present here is +// known not to be a deleted local branch, so it is excluded from recovery +// candidates. +func (self *BranchLoader) getExistingRefNames() ([]string, error) { output, err := self.cmd.New( NewGitCmd("for-each-ref"). Arg("--format=%(refname:short)"). - Arg("refs/heads"). + Arg("refs/heads", "refs/remotes", "refs/tags"). ToArgv(), ).DontLog().RunWithOutput() if err != nil { return nil, err } - return strings.Split(strings.TrimSpace(output), "\n"), nil + return append(strings.Split(strings.TrimSpace(output), "\n"), "HEAD"), nil } // parseReflogEntries parses the raw output of @@ -585,8 +603,8 @@ func parseReflogCheckoutSubject(subject string) (string, string) { // out but are no longer local branches, together with the commit they pointed // at when last seen. The result is ordered by recency (most recently committed // to first). -func obtainDeletedBranches(entries []*reflogEntry, currentBranchNames []string) []*models.DeletedBranch { - currentBranches := set.NewFromSlice(currentBranchNames) +func obtainDeletedBranches(entries []*reflogEntry, existingRefs []string, isValidRefFormat func(string) bool) []*models.DeletedBranch { + existing := set.NewFromSlice(existingRefs) // currentBranch is the branch HEAD was on leading up to the current entry. currentBranch := "" @@ -597,11 +615,21 @@ func obtainDeletedBranches(entries []*reflogEntry, currentBranchNames []string) entry := entries[i] if entry.from != "" && entry.to != "" { + // The hash of a checkout entry is the tip of the branch being + // moved to. Seeding it here means branches created via + // `git checkout -b` (and never committed to) are still + // recoverable. We never touch the source branch: its tip was + // recorded by the older entries that preceded this checkout, and + // overwriting it with the destination's tip would be wrong. + if isBranchName(entry.to, isValidRefFormat) { + branchTip[entry.to] = entry.hash + branchTimestamp[entry.to] = entry.timestamp + } currentBranch = entry.to continue } - if currentBranch != "" && currentBranch != "HEAD" { + if currentBranch != "" && isBranchName(currentBranch, isValidRefFormat) { branchTip[currentBranch] = entry.hash branchTimestamp[currentBranch] = entry.timestamp } @@ -609,7 +637,7 @@ func obtainDeletedBranches(entries []*reflogEntry, currentBranchNames []string) deleted := make([]*models.DeletedBranch, 0, len(branchTip)) for name, tip := range branchTip { - if name == "HEAD" || currentBranches.Includes(name) { + if existing.Includes(name) { continue } deleted = append(deleted, &models.DeletedBranch{ @@ -637,6 +665,47 @@ func obtainDeletedBranches(entries []*reflogEntry, currentBranchNames []string) return deleted } +// isBranchName returns true if the given string could be a local branch name +// (as opposed to a commit hash, a tag, a remote-tracking ref, or HEAD). This +// filters out reflog noise like "checkout: moving from HEAD to abc1234" or +// tag/remote checkouts, which would otherwise show up as phantom deleted +// branches. The ref-name grammar itself is validated by isValidRefFormat +// (which defers to `git check-ref-format`). +func isBranchName(name string, isValidRefFormat func(string) bool) bool { + if name == "" || name == "HEAD" { + return false + } + if !isValidRefFormat(name) { + return false + } + // A name containing a slash is only treated as a branch if the part before + // the first slash is not a well-known remote marker (git writes + // "origin/feature" or "tags/v1.0" for remote/tag checkouts, and branch + // names can legitimately contain slashes, e.g. "feature/foo"). + if strings.ContainsRune(name, '/') { + remote, _, _ := strings.Cut(name, "/") + return !lo.Contains([]string{"origin", "upstream", "fork", "tags", "remotes"}, remote) + } + return !looksLikeSha(name) +} + +// looksLikeSha returns true if the string looks like a commit hash: all hex +// characters and at least as long as git's minimum abbreviation. This covers +// both abbreviated and full-length hashes that git writes for detached-head +// checkouts. A branch name that happens to be all-hex would be missed, but +// that's an acceptable trade-off since such names are vanishingly rare. +func looksLikeSha(name string) bool { + if len(name) < 7 { + return false + } + for _, c := range name { + if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F')) { + return false + } + } + return true +} + // TODO: only look at the new reflog commits, and otherwise store the recencies in // int form against the branch to recalculate the time ago func (self *BranchLoader) obtainReflogBranches(reflogCommits []*models.Commit) []*models.Branch { diff --git a/pkg/commands/git_commands/deleted_branch_test.go b/pkg/commands/git_commands/deleted_branch_test.go index 2b3b5d207..3c62ea63b 100644 --- a/pkg/commands/git_commands/deleted_branch_test.go +++ b/pkg/commands/git_commands/deleted_branch_test.go @@ -1,6 +1,8 @@ package git_commands import ( + "errors" + "strings" "testing" "github.com/jesseduffield/lazygit/pkg/commands/models" @@ -10,10 +12,10 @@ import ( func TestObtainDeletedBranches(t *testing.T) { type scenario struct { - testName string - entries []*reflogEntry - currentBranchNames []string - expected []*models.DeletedBranch + testName string + entries []*reflogEntry + existingRefs []string + expected []*models.DeletedBranch } scenarios := []scenario{ @@ -26,7 +28,7 @@ func TestObtainDeletedBranches(t *testing.T) { {hash: "c", timestamp: 100, from: "main", to: "feature"}, {hash: "d", timestamp: 50}, }, - currentBranchNames: []string{"main"}, + existingRefs: []string{"main", "HEAD"}, expected: []*models.DeletedBranch{ {Name: "feature", CommitHash: "b", DisplayName: "feature", Recency: "56y", UnixTimestamp: 200}, }, @@ -36,35 +38,35 @@ func TestObtainDeletedBranches(t *testing.T) { // newest-first reflog entries: []*reflogEntry{ {hash: "a", timestamp: 300, from: "feat/a", to: "main"}, // newest: leave feat/a - {hash: "b", timestamp: 250}, // commit on feat/a + {hash: "b", timestamp: 250}, // commit on feat/a {hash: "a", timestamp: 200, from: "feat/b", to: "feat/a"}, - {hash: "c", timestamp: 150}, // commit on feat/b + {hash: "c", timestamp: 150}, // commit on feat/b {hash: "a", timestamp: 100, from: "main", to: "feat/b"}, // oldest: create feat/b }, - currentBranchNames: []string{"main"}, + existingRefs: []string{"main", "HEAD"}, expected: []*models.DeletedBranch{ {Name: "feat/a", CommitHash: "b", DisplayName: "feat/a", Recency: "56y", UnixTimestamp: 250}, {Name: "feat/b", CommitHash: "c", DisplayName: "feat/b", Recency: "56y", UnixTimestamp: 150}, }, }, { - testName: "existing branches are excluded", - entries: []*reflogEntry{ + testName: "existing branches are excluded", + entries: []*reflogEntry{ {hash: "a", timestamp: 300, from: "main", to: "other"}, {hash: "b", timestamp: 200}, {hash: "c", timestamp: 100, from: "other", to: "main"}, }, - currentBranchNames: []string{"main", "other"}, - expected: nil, + existingRefs: []string{"main", "other", "HEAD"}, + expected: nil, }, { - testName: "no checkout entries means nothing recoverable", - entries: []*reflogEntry{ + testName: "no checkout entries means nothing recoverable", + entries: []*reflogEntry{ {hash: "a", timestamp: 300}, {hash: "b", timestamp: 200}, }, - currentBranchNames: []string{"main"}, - expected: nil, + existingRefs: []string{"main", "HEAD"}, + expected: nil, }, { testName: "HEAD is not treated as a deleted branch", @@ -72,24 +74,161 @@ func TestObtainDeletedBranches(t *testing.T) { {hash: "a", timestamp: 300, from: "main", to: "HEAD"}, {hash: "b", timestamp: 200}, }, - currentBranchNames: []string{"main"}, - expected: nil, + existingRefs: []string{"main", "HEAD"}, + expected: nil, + }, + { + testName: "branch created via checkout with no commits is recoverable", + // newest-first reflog: create feature, then immediately leave it + entries: []*reflogEntry{ + {hash: "a", timestamp: 300, from: "feature", to: "main"}, // leave feature + {hash: "b", timestamp: 200, from: "main", to: "feature"}, // create feature + }, + existingRefs: []string{"main", "HEAD"}, + expected: []*models.DeletedBranch{ + {Name: "feature", CommitHash: "b", DisplayName: "feature", Recency: "56y", UnixTimestamp: 200}, + }, + }, + { + testName: "detached head checkout is not treated as a deleted branch", + entries: []*reflogEntry{ + {hash: "a", timestamp: 300, from: "abc1234567890abcdefabcdefabcdefabcdefabcdef", to: "main"}, + {hash: "b", timestamp: 200, from: "main", to: "abc1234567890abcdefabcdefabcdefabcdefabcdef"}, + }, + existingRefs: []string{"main", "HEAD"}, + expected: nil, + }, + { + testName: "existing remote-tracking ref is excluded from candidates", + entries: []*reflogEntry{ + {hash: "a", timestamp: 300, from: "origin/feature", to: "main"}, + {hash: "b", timestamp: 200, from: "main", to: "origin/feature"}, + }, + existingRefs: []string{"main", "origin/feature", "HEAD"}, + expected: nil, + }, + { + testName: "existing tag checkout is not treated as a deleted branch", + entries: []*reflogEntry{ + {hash: "a", timestamp: 300, from: "v1.0.0", to: "main"}, + {hash: "b", timestamp: 200, from: "main", to: "v1.0.0"}, + }, + existingRefs: []string{"main", "v1.0.0", "HEAD"}, + expected: nil, + }, + { + testName: "checkout to a commit expression is not treated as a deleted branch", + entries: []*reflogEntry{ + {hash: "a", timestamp: 300, from: "HEAD~1", to: "main"}, + {hash: "b", timestamp: 200, from: "main", to: "HEAD~1"}, + }, + existingRefs: []string{"main", "HEAD"}, + expected: nil, + }, + { + testName: "checkout to a sha-256 hash is not treated as a deleted branch", + entries: []*reflogEntry{ + {hash: "a", timestamp: 300, from: "8f0f1f2f3f4f5f6f7f8f9fafbfcfdfefff0f1f2f3f4f5f6f7f8f9fafbfcfdfeff", to: "main"}, + {hash: "b", timestamp: 200, from: "main", to: "8f0f1f2f3f4f5f6f7f8f9fafbfcfdfefff0f1f2f3f4f5f6f7f8f9fafbfcfdfeff"}, + }, + existingRefs: []string{"main", "HEAD"}, + expected: nil, }, } for _, s := range scenarios { t.Run(s.testName, func(t *testing.T) { - result := obtainDeletedBranches(s.entries, s.currentBranchNames) + result := obtainDeletedBranches(s.entries, s.existingRefs, isValidRefFormatStub) assert.Equal(t, s.expected, result) }) } } +// isValidRefFormatStub is a stand-in for `git check-ref-format` used to keep +// the pure tests independent of the subprocess. It applies the same ref-name +// grammar rules git enforces. +func isValidRefFormatStub(name string) bool { + if name == "" { + return false + } + if strings.HasPrefix(name, "-") { + return false + } + if strings.HasSuffix(name, ".") || strings.HasSuffix(name, "/") { + return false + } + if strings.Contains(name, "..") || strings.Contains(name, "@{") { + return false + } + for _, c := range name { + if c <= ' ' || strings.ContainsRune("~^:?*[\\", c) { + return false + } + } + return true +} + +func TestIsValidRefFormat(t *testing.T) { + type scenario struct { + testName string + name string + isValid bool + } + + scenarios := []scenario{ + { + testName: "valid branch name", + name: "feature/foo", + isValid: true, + }, + { + // a sha-looking string is still a valid ref to git; filtering it is + // done separately by looksLikeSha + testName: "sha-like is a valid ref to git", + name: "8f0f1f2f3f4f", + isValid: true, + }, + { + testName: "rejects commit expression", + name: "HEAD~1", + }, + { + testName: "rejects trailing slash", + name: "feature/", + }, + { + testName: "rejects double dot", + name: "feature..other", + }, + } + + for _, s := range scenarios { + t.Run(s.testName, func(t *testing.T) { + err := errors.New("invalid ref") + if s.isValid { + err = nil + } + runner := oscommands.NewFakeRunner(t). + ExpectGitArgs([]string{"check-ref-format", "--allow-onelevel", s.name}, "", err) + + gitCommon := buildGitCommon(commonDeps{runner: runner}) + loader := &BranchLoader{ + Common: gitCommon.Common, + GitCommon: gitCommon, + cmd: gitCommon.cmd, + } + + assert.Equal(t, s.isValid, loader.isValidRefFormat(s.name)) + runner.CheckForMissingCalls() + }) + } +} + func TestParseReflogCheckoutSubject(t *testing.T) { type scenario struct { - testName string - subject string - expected []string + testName string + subject string + expected []string } scenarios := []scenario{ @@ -121,9 +260,9 @@ func TestParseReflogCheckoutSubject(t *testing.T) { func TestBranchRestoreBranch(t *testing.T) { type scenario struct { - testName string - runner *oscommands.FakeCmdObjRunner - expectedErr bool + testName string + runner *oscommands.FakeCmdObjRunner + expectedErr bool expectedUpstream string } @@ -133,7 +272,7 @@ func TestBranchRestoreBranch(t *testing.T) { runner: oscommands.NewFakeRunner(t). ExpectGitArgs([]string{"branch", "feature", "abc123"}, "", nil). ExpectGitArgs([]string{"for-each-ref", "--format=%(refname:short)", "refs/remotes"}, "", nil), - expectedErr: false, + expectedErr: false, expectedUpstream: "", }, { @@ -142,7 +281,7 @@ func TestBranchRestoreBranch(t *testing.T) { ExpectGitArgs([]string{"branch", "feature", "abc123"}, "", nil). ExpectGitArgs([]string{"for-each-ref", "--format=%(refname:short)", "refs/remotes"}, "origin/feature\n", nil). ExpectGitArgs([]string{"branch", "--set-upstream-to=origin/feature", "feature"}, "", nil), - expectedErr: false, + expectedErr: false, expectedUpstream: "origin/feature", }, { @@ -150,7 +289,7 @@ func TestBranchRestoreBranch(t *testing.T) { runner: oscommands.NewFakeRunner(t). ExpectGitArgs([]string{"branch", "feature", "abc123"}, "", nil). ExpectGitArgs([]string{"for-each-ref", "--format=%(refname:short)", "refs/remotes"}, "origin/feature\nfork/feature\n", nil), - expectedErr: false, + expectedErr: false, expectedUpstream: "", }, }