Skip to content

Commit 18f05da

Browse files
authored
[perf] Fix IsSquashMerged object leak: replace commit-tree with patch-id (#300) (#304)
* [perf] Fix IsSquashMerged object leak: replace commit-tree with patch-id (#300) * [fix] address review: stderr capture, drop errEmptyDiff, args-dispatch tests * [test] content-based ComputePatchIDs dispatch in stubs
1 parent f1f99a5 commit 18f05da

7 files changed

Lines changed: 251 additions & 89 deletions

File tree

internal/git/branch.go

Lines changed: 64 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
package git
22

33
import (
4+
"bytes"
45
"context"
56
"fmt"
7+
"os"
8+
"os/exec"
69
"strconv"
710
"strings"
811
"time"
@@ -12,6 +15,40 @@ import (
1215

1316
const internalGitInvariantHint = "report this — git output unexpectedly malformed"
1417

18+
// ComputePatchIDs pipes diff into git patch-id --stable and returns the set of
19+
// patch-id hex strings. Exposed as a variable so tests can stub it without a
20+
// real git subprocess. Not safe for concurrent modification.
21+
var ComputePatchIDs = func(ctx context.Context, diff string) (map[string]bool, error) {
22+
if diff == "" {
23+
return map[string]bool{}, nil
24+
}
25+
cmd := exec.CommandContext(ctx, "git", "patch-id", "--stable")
26+
cmd.Env = stableGitEnv(os.Environ())
27+
cmd.Stdin = strings.NewReader(diff)
28+
var stderr bytes.Buffer
29+
cmd.Stderr = &stderr
30+
out, err := cmd.Output()
31+
if err != nil {
32+
if msg := strings.TrimSpace(stderr.String()); msg != "" {
33+
return nil, fmt.Errorf("git patch-id --stable: %s: %w", msg, err)
34+
}
35+
return nil, fmt.Errorf("git patch-id --stable: %w", err)
36+
}
37+
38+
ids := make(map[string]bool)
39+
for line := range strings.SplitSeq(string(out), "\n") {
40+
line = strings.TrimSpace(line)
41+
if line == "" {
42+
continue
43+
}
44+
pid, _, _ := strings.Cut(line, " ")
45+
if pid != "" {
46+
ids[pid] = true
47+
}
48+
}
49+
return ids, nil
50+
}
51+
1552
// BranchExists checks whether a local branch exists.
1653
func BranchExists(ctx context.Context, r Runner, branch string) bool {
1754
_, err := r.Run(ctx, cmdRevParse, flagVerify, refsHeadsPrefix+branch)
@@ -94,33 +131,50 @@ func AheadBehind(ctx context.Context, r Runner, dir string) (ahead, behind int,
94131
return ahead, behind, nil
95132
}
96133

97-
// IsSquashMerged checks whether a branch's content has been squash-merged into mergeRef.
98-
// It uses the commit-tree + cherry technique: create a synthetic commit with the
99-
// branch's tree on the merge-base, then check if that content is already in mergeRef.
100-
// Note: each call creates an unreferenced commit object in the git store; these are
101-
// cleaned up automatically by git gc.
134+
// IsSquashMerged reports whether branch's diff patch-id matches any commit in
135+
// mergeRef since their common merge-base, indicating a squash merge.
102136
func IsSquashMerged(ctx context.Context, r Runner, mergeRef, branch string) (bool, error) {
103137
mergeBase, err := MergeBase(ctx, r, mergeRef, branch)
104138
if err != nil {
105139
return false, err
106140
}
107141

108-
tree, err := r.Run(ctx, cmdRevParse, branch+treeSuffix)
142+
tip, err := r.Run(ctx, cmdRevParse, "--verify", branch)
109143
if err != nil {
110144
return false, err
111145
}
112146

113-
tempCommit, err := r.Run(ctx, cmdCommitTree, tree, "-p", mergeBase, "-m", "temp")
147+
if strings.TrimSpace(mergeBase) == strings.TrimSpace(tip) { // empty branch — nothing squash-merged
148+
return false, nil
149+
}
150+
151+
branchDiff, err := r.Run(ctx, CmdDiff, mergeBase, branch)
152+
if err != nil {
153+
return false, err
154+
}
155+
branchPIDs, err := ComputePatchIDs(ctx, branchDiff)
114156
if err != nil {
115157
return false, err
116158
}
159+
if len(branchPIDs) == 0 {
160+
return false, nil
161+
}
117162

118-
out, err := r.Run(ctx, cmdCherry, mergeRef, tempCommit)
163+
mergeRefDiffs, err := r.Run(ctx, CmdLog, "-p", "--no-merges", mergeBase+".."+mergeRef)
164+
if err != nil {
165+
return false, err
166+
}
167+
mergeRefPIDs, err := ComputePatchIDs(ctx, mergeRefDiffs)
119168
if err != nil {
120169
return false, err
121170
}
122171

123-
return strings.HasPrefix(out, cherryMerged), nil
172+
for pid := range mergeRefPIDs {
173+
if branchPIDs[pid] {
174+
return true, nil
175+
}
176+
}
177+
return false, nil
124178
}
125179

126180
// MergedBranches returns branches that have been merged into the given branch.
@@ -152,7 +206,7 @@ func LastCommitTime(ctx context.Context, r Runner, branch string) (time.Time, er
152206

153207
// LastCommitInfo returns the time and subject of the last commit on the given branch.
154208
func LastCommitInfo(ctx context.Context, r Runner, branch string) (time.Time, string, error) {
155-
out, err := r.Run(ctx, "log", "-1", "--format=%ct\t%s", branch)
209+
out, err := r.Run(ctx, CmdLog, "-1", "--format=%ct\t%s", branch)
156210
if err != nil {
157211
return time.Time{}, "", errhint.WithFix(
158212
fmt.Errorf("last commit info for %s: %w", branch, err),

internal/git/diff.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import (
88
// DiffNameOnly returns files changed between base and branch using three-dot diff.
99
// The three-dot notation (base...branch) shows changes on branch since it diverged from base.
1010
func DiffNameOnly(ctx context.Context, r Runner, base, branch string) ([]string, error) {
11-
out, err := r.Run(ctx, "diff", "--name-only", base+"..."+branch)
11+
out, err := r.Run(ctx, CmdDiff, "--name-only", base+"..."+branch)
1212
if err != nil {
1313
return nil, err
1414
}

0 commit comments

Comments
 (0)