Skip to content

Commit cafa137

Browse files
committed
refactor(diff): normalize git argument ordering in workspaceTrackedDiff
Cosmetic consistency per alibaba#374, discovered in alibaba#365 review. Reorders the two runGit calls so all option flags precede positional args, matching the file's other diff call sites (GetDiff ranges/commit, computeMergeBase). The HEAD call now places --end-of-options before the HEAD ref, hardening against ref-looking filenames; --staged stays with the option flags (it is a flag, not a ref, so no --end-of-options). Pure reordering plus one --end-of-options addition; no flag or value changed. All diff tests pass.
1 parent 2bf81bc commit cafa137

2 files changed

Lines changed: 39 additions & 2 deletions

File tree

internal/diff/git.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -261,14 +261,19 @@ func (p *Provider) computeMergeBase(ctx context.Context, from, to string) string
261261
}
262262

263263
func (p *Provider) workspaceTrackedDiff(ctx context.Context) (string, error) {
264-
out, err := p.runGit(ctx, "-c", "core.quotepath=false", "diff", "--no-ext-diff", "--no-textconv", "--find-renames", "--src-prefix=a/", "--dst-prefix=b/", "HEAD", "--no-color", "-U"+fmt.Sprint(DiffContextLines), "--")
264+
out, err := p.runGit(ctx, "-c", "core.quotepath=false", "diff", "--no-ext-diff", "--no-textconv", "--find-renames", "--src-prefix=a/", "--dst-prefix=b/", "--no-color", "-U"+fmt.Sprint(DiffContextLines), "--end-of-options", "HEAD", "--")
265265
if err == nil && out != "" {
266266
return out, nil
267267
}
268268
if ctx.Err() != nil {
269269
return "", ctx.Err()
270270
}
271-
return p.runGit(ctx, "-c", "core.quotepath=false", "diff", "--no-ext-diff", "--no-textconv", "--find-renames", "--src-prefix=a/", "--dst-prefix=b/", "--staged", "--no-color", "-U"+fmt.Sprint(DiffContextLines), "--")
271+
// Fall back to the staged diff when `git diff HEAD` errored or was empty. This is
272+
// not redundant with the call above: in a repository with no commits yet there is no
273+
// HEAD, so `git diff HEAD` fails with "bad revision 'HEAD'", but `git diff --staged`
274+
// still surfaces staged changes by diffing the index against the empty tree — the only
275+
// way to review a workspace before its first commit.
276+
return p.runGit(ctx, "-c", "core.quotepath=false", "diff", "--no-ext-diff", "--no-textconv", "--find-renames", "--src-prefix=a/", "--dst-prefix=b/", "--no-color", "-U"+fmt.Sprint(DiffContextLines), "--staged", "--")
272277
}
273278

274279
func (p *Provider) untrackedFileDiffs(ctx context.Context) ([]string, error) {

internal/diff/git_test.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,38 @@ func TestWorkspaceDiffSurvivesExternalDiffTool(t *testing.T) {
205205
}
206206
}
207207

208+
// TestWorkspaceDiffNoCommitsUsesStagedFallback pins the second runGit call in
209+
// workspaceTrackedDiff: in a repository with no commits there is no HEAD, so
210+
// `git diff HEAD` fails, and the staged diff is the only way to review the
211+
// workspace. Removing the `--staged` fallback (as an over-eager "simplification"
212+
// might) makes this return zero diffs.
213+
func TestWorkspaceDiffNoCommitsUsesStagedFallback(t *testing.T) {
214+
repo := t.TempDir()
215+
runGitTest(t, repo, "init", "-q")
216+
runGitTest(t, repo, "config", "user.email", "test@example.com")
217+
runGitTest(t, repo, "config", "user.name", "Test User")
218+
runGitTest(t, repo, "config", "commit.gpgsign", "false")
219+
220+
// Staged file, no commit yet -> no HEAD.
221+
file := filepath.Join(repo, "staged.txt")
222+
if err := os.WriteFile(file, []byte("alpha\nbeta\n"), 0o644); err != nil {
223+
t.Fatalf("write staged.txt: %v", err)
224+
}
225+
runGitTest(t, repo, "add", "staged.txt")
226+
227+
runner := gitcmd.New(0)
228+
provider := NewWorkspaceProvider(repo, runner)
229+
230+
diffs, err := provider.GetDiff(context.Background())
231+
if err != nil {
232+
t.Fatalf("GetDiff returned error: %v", err)
233+
}
234+
if len(diffs) == 0 {
235+
t.Fatalf("expected staged changes to be surfaced in a repo with no commits " +
236+
"(the --staged fallback in workspaceTrackedDiff), got 0 diffs")
237+
}
238+
}
239+
208240
// TestCommitDiffSurvivesExternalDiffTool covers the ModeCommit call site
209241
// (git show <commit>), which likewise must pass --no-ext-diff so that a
210242
// user's external diff tool does not break single-commit analysis.

0 commit comments

Comments
 (0)