diff --git a/pkg/commands/git_commands/git_command_builder.go b/pkg/commands/git_commands/git_command_builder.go index cc01784a3..fbc204f7a 100644 --- a/pkg/commands/git_commands/git_command_builder.go +++ b/pkg/commands/git_commands/git_command_builder.go @@ -151,6 +151,9 @@ func (self *GitCommandBuilder) NoLineEndingConversion() *GitCommandBuilder { // // For this we override three configs of the user. diff.noprefix leaves the // prefix out, and diff.srcPrefix and diff.dstPrefix can set it to any string. +// We leave diff.mnemonicPrefix alone. The prefixes it chooses, such as i/ and +// w/, are a single path component like a/ and b/, so we can strip them too, and +// the diffs we show keep the prefixes the user asked for. func (self *GitCommandBuilder) ParseableDiffPrefixes() *GitCommandBuilder { return self. Config("diff.noprefix=false"). diff --git a/pkg/gui/controllers/helpers/diff_line_parser.go b/pkg/gui/controllers/helpers/diff_line_parser.go index 73206a4ee..39d99c9ec 100644 --- a/pkg/gui/controllers/helpers/diff_line_parser.go +++ b/pkg/gui/controllers/helpers/diff_line_parser.go @@ -308,15 +308,17 @@ func diffLineTypeForKind(kind patch.PatchLineKind) types.DiffLineType { // pathFromDiffHeader extracts the new-file path of a single diff section. A // submodule's section states its path in the line it opens with. For a file of the -// repo the path comes from the "+++ b/" line, falling back to "--- a/" -// when the new path is /dev/null (a deleted file), and to the "diff --git" line when -// there are no such lines at all (a pure rename, which has no hunks). +// repo the path comes from the "+++ " line, falling back to the "--- " line when the +// new path is /dev/null (a deleted file). A section without these lines has no hunks: +// it is a rename or copy without a content change, a change of the file mode, or a +// binary file. The path then comes from the "rename to" or "copy to" line, or failing +// that from the "diff --git" line. func pathFromDiffHeader(fileLines []string) string { if path := submodulePath(fileLines[0]); path != "" { return path } - var oldPath, newPath string + var oldPath, newPath, copiedOrRenamedPath string for _, line := range fileLines { if strings.HasPrefix(line, "@@") { break // past the header @@ -326,6 +328,10 @@ func pathFromDiffHeader(fileLines []string) string { newPath = pathFromDiffHeaderField(strings.TrimPrefix(line, "+++ ")) case strings.HasPrefix(line, "--- "): oldPath = pathFromDiffHeaderField(strings.TrimPrefix(line, "--- ")) + case strings.HasPrefix(line, "rename to "): + copiedOrRenamedPath, _ = unquoteDiffPath(strings.TrimPrefix(line, "rename to ")) + case strings.HasPrefix(line, "copy to "): + copiedOrRenamedPath, _ = unquoteDiffPath(strings.TrimPrefix(line, "copy to ")) } } @@ -335,6 +341,9 @@ func pathFromDiffHeader(fileLines []string) string { if oldPath != "" && oldPath != "/dev/null" { return oldPath } + if copiedOrRenamedPath != "" { + return copiedOrRenamedPath + } return pathFromDiffGitLine(fileLines[0]) } @@ -342,13 +351,16 @@ func pathFromDiffHeader(fileLines []string) string { // after "--- " or "+++ ", or one of the two paths on the "diff --git" line — // into the repo-relative path it names. // -// Returns "" for a quoted field we can't decode: better to resolve nothing than -// to point a consumer at a path that doesn't exist. +// Returns "" for a field we can't decode: better to resolve nothing than to point +// a consumer at a path that doesn't exist. func pathFromDiffHeaderField(field string) string { path, ok := unquoteDiffPath(field) if !ok { return "" } + if path == "/dev/null" { + return path + } return stripDiffPathPrefix(path) } @@ -374,14 +386,20 @@ func unquoteDiffPath(field string) (path string, ok bool) { return field, true } -// stripDiffPathPrefix removes the a/ or b/ prefix git puts on the paths in a -// diff header. We ask git for these prefixes explicitly (diff.noprefix=false), -// so they are always there. +// stripDiffPathPrefix removes the prefix git puts in front of the paths in a diff +// header. By default this is a/ or b/, but with diff.mnemonicPrefix git uses a +// letter that names what is compared, such as i/ for the index and w/ for the +// working tree. Either way the prefix is the first component of the path, and we +// ask git to always put one there (ParseableDiffPrefixes). `git apply` strips the +// same component by default. +// +// Returns "" for a path without a slash, which has no prefix to strip. func stripDiffPathPrefix(path string) string { - if strings.HasPrefix(path, "a/") || strings.HasPrefix(path, "b/") { - return path[2:] + _, rest, found := strings.Cut(path, "/") + if !found { + return "" } - return path + return rest } // parseDiffLineMetadata parses the payload of an OSC 1717 record, in which a @@ -445,17 +463,21 @@ func diffLineTypeFromMetadata(typeField string) (types.DiffLineType, bool) { } } -// pathFromDiffGitLine extracts the new-file path from a "diff --git a/X b/X" -// line, where the two paths are separated by a space and either may be quoted. -// A path containing " b/" (or ` "b/`) would defeat this, but the +++/--- lines -// are unambiguous and we only get here when they are absent. +// pathFromDiffGitLine extracts the path from a "diff --git" line of a file whose +// path is the same on both sides. The two paths are separated by a space, but a +// path may contain spaces too. So, like `git apply`, we split the line at the space +// where the two halves name the same path once their prefixes are stripped. Either +// half may be quoted. func pathFromDiffGitLine(line string) string { rest := strings.TrimPrefix(line, diffFilePrefix) - if idx := strings.LastIndex(rest, ` "b/`); idx != -1 { - return pathFromDiffHeaderField(rest[idx+1:]) - } - if idx := strings.LastIndex(rest, " b/"); idx != -1 { - return pathFromDiffHeaderField(rest[idx+1:]) + for i, c := range rest { + if c != ' ' { + continue + } + path := pathFromDiffHeaderField(rest[i+1:]) + if path != "" && path == pathFromDiffHeaderField(rest[:i]) { + return path + } } return "" } diff --git a/pkg/gui/controllers/helpers/diff_line_parser_test.go b/pkg/gui/controllers/helpers/diff_line_parser_test.go index 5c5c41297..f08618b74 100644 --- a/pkg/gui/controllers/helpers/diff_line_parser_test.go +++ b/pkg/gui/controllers/helpers/diff_line_parser_test.go @@ -250,6 +250,11 @@ func TestPathFromDiffHeaderField(t *testing.T) { {"new side", "b/file.go", "file.go"}, {"old side", "a/file.go", "file.go"}, {"a missing file", "/dev/null", "/dev/null"}, + {"file in a directory", "b/dir/file.go", "dir/file.go"}, + // diff.mnemonicPrefix makes git name the two sides by what they are. + {"index side", "i/file.go", "file.go"}, + {"working tree side", "w/file.go", "file.go"}, + {"no prefix", "file.go", ""}, // git terminates the field with a tab when the path has a space in it. {"path with a space", "b/with space.go\t", "with space.go"}, // With core.quotePath enabled (the default) every non-ASCII byte is @@ -260,6 +265,7 @@ func TestPathFromDiffHeaderField(t *testing.T) { {"path with a backslash", `"b/back\\slash.go"`, `back\slash.go`}, {"path with a tab", `"b/tab\there.go"`, "tab\there.go"}, {"undecodable", `"b/unterminated`, ""}, + {"quoted with a mnemonic prefix", `"w/caf\303\251.go"`, "café.go"}, } for _, s := range scenarios { @@ -269,6 +275,66 @@ func TestPathFromDiffHeaderField(t *testing.T) { } } +func TestPathFromDiffGitLine(t *testing.T) { + scenarios := []struct { + name string + line string + expected string + }{ + {"default prefixes", "diff --git a/file.go b/file.go", "file.go"}, + {"mnemonic prefixes", "diff --git i/file.go w/file.go", "file.go"}, + {"path with a space", "diff --git i/my file.go w/my file.go", "my file.go"}, + {"path that looks like it holds the other path", "diff --git i/x w/y w/x w/y", "x w/y"}, + {"quoted paths", `diff --git "i/caf\303\251.go" "w/caf\303\251.go"`, "café.go"}, + // A rename; its path comes from the "rename to" line. + {"different paths", "diff --git c/old.go i/new.go", ""}, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + assert.Equal(t, s.expected, pathFromDiffGitLine(s.line)) + }) + } +} + +func TestParseDiffLineFromBufferMnemonicPrefixes(t *testing.T) { + // With diff.mnemonicPrefix, git names the two sides of the working tree's diff + // i/ and w/, and those of the staged diff c/ and i/. + modified := strings.Split(`diff --git i/file.go w/file.go +index 1111111..2222222 100644 +--- i/file.go ++++ w/file.go +@@ -1,2 +1,2 @@ + apple +-grape ++kiwi`, "\n") + + result, ok := parseDiffLineFromBuffer(modified, 7) + assert.True(t, ok) + assert.Equal(t, parsedDiffLine{Path: "file.go", Type: types.DiffLineAdded, NewLine: 2}, result) + + // A change of the file mode has no +++/--- lines, so the path comes from the + // "diff --git" line. + modeChange := strings.Split(`diff --git i/run.sh w/run.sh +old mode 100644 +new mode 100755`, "\n") + + result, ok = parseDiffLineFromBuffer(modeChange, 2) + assert.True(t, ok) + assert.Equal(t, parsedDiffLine{Path: "run.sh", Type: types.DiffLineFileHeader, NewLine: 1}, result) + + // Neither does a copy without a content change, and the two paths on its + // "diff --git" line differ, so the path comes from the "copy to" line. + copied := strings.Split(`diff --git c/run.sh i/copy of run.sh +similarity index 100% +copy from run.sh +copy to copy of run.sh`, "\n") + + result, ok = parseDiffLineFromBuffer(copied, 2) + assert.True(t, ok) + assert.Equal(t, parsedDiffLine{Path: "copy of run.sh", Type: types.DiffLineFileHeader, NewLine: 1}, result) +} + func TestParseDiffLineFromBufferQuotedPath(t *testing.T) { // A rename of a file whose name needs quoting, with a content change: the // path is quoted on the "diff --git" line and on both of the +++/--- lines. diff --git a/pkg/integration/tests/main_view/stage_diff_lines_with_mnemonic_prefixes.go b/pkg/integration/tests/main_view/stage_diff_lines_with_mnemonic_prefixes.go index c74cbe614..25675b7f8 100644 --- a/pkg/integration/tests/main_view/stage_diff_lines_with_mnemonic_prefixes.go +++ b/pkg/integration/tests/main_view/stage_diff_lines_with_mnemonic_prefixes.go @@ -42,10 +42,7 @@ var StageDiffLinesWithMnemonicPrefixes = NewIntegrationTest(NewIntegrationTestAr PressPrimaryAction() t.Views().Files().Lines( - /* EXPECTED: Contains("MM file1"), - ACTUAL: */ - Contains(" M file1"), ) }, })