Thread (1 message) 1 message, 1 author, 2025-08-10

Re: [PATCH] git-jump: make `diff` work with filenames containing spaces

From: Junio C Hamano <hidden>
Date: 2025-08-10 00:03:13

"Greg Hurrell via GitGitGadget" [off-list ref] writes:
From: Greg Hurrell <redacted>

In diff.c, we output a trailing "\t" at the end of any filename that
contains a space:

    case DIFF_SYMBOL_FILEPAIR_PLUS:
            meta = diff_get_color_opt(o, DIFF_METAINFO);
            reset = diff_get_color_opt(o, DIFF_RESET);
            fprintf(o->file, "%s%s+++ %s%s%s\n", diff_line_prefix(o), meta,
                    line, reset,
                    strchr(line, ' ') ? "\t" : "");
            break;

That is, for a file "foo.txt" we'll emit:

    +++ a/foo.txt

but for "foo bar.txt" we'll emit:

    +++ a/foo bar.txt\t

This in turn leads us to produce a quickfix format like this:

    foo bar.txt\t:1:1:contents

Because no "foo bar.txt\t" file actually exists on disk, opening it in
Vim will just land the user in an empty buffer.

This commit takes the simple approach of unconditionally stripping any
trailing tab. Consider the following three examples:

1. For file "foo bar", Git will emit "foo bar\t".
2. For file "foo\t", Git will emit "foo\t".
3. For file "foo bar\t", Git will emit "foo bar\t\t".

Before this commit, `git-jump` correctly handled only case "2".

After this commit, `git-jump` correctly handles cases "1" and "3". In
reality, "1" is the only case people are going to run into with any
regularity, and the other two are extreme edge cases.

The argument here is that stripping the "\t" unconditionally gives us a
minimal change, and it addresses the common case without bringing in
complexity for the uncommon ones. If anybody ever complains about case
"2" no longer working for them, we can do the more complicated thing and
only strip the "\t" if the filename contains a space.

Signed-off-by: Greg Hurrell <redacted>
---
Because (1) I do not use 'git jump', (2) I do not use 'vim' or
'quickfix format', and (3) I know this is your brainchid but you are
offline this week, I won't do anything to this topic other than
possibly to keep it in 'seen' to avoid losing it.

FWIW, I do not disagree with the decision of this patch makes to
"break" those who has file "foo\t" to help those with file "foo",
even though I usually frown upon a change that robs Peter to pay
Paul.  Among the three cases considerd, #1 is the only one that
would matter in practice.
quoted hunk
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1950%2Fwincent%2Fstrip-trailing-tab-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1950/wincent/strip-trailing-tab-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1950

 contrib/git-jump/git-jump | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
index 3f696759617..8d1d5d79a69 100755
--- a/contrib/git-jump/git-jump
+++ b/contrib/git-jump/git-jump
@@ -44,7 +44,7 @@ open_editor() {
 mode_diff() {
 	git diff --no-prefix --relative "$@" |
 	perl -ne '
-	if (m{^\+\+\+ (.*)}) { $file = $1 eq "/dev/null" ? undef : $1; next }
+	if (m{^\+\+\+ (.*?)\t?$}) { $file = $1 eq "/dev/null" ? undef : $1; next }
 	defined($file) or next;
 	if (m/^@@ .*?\+(\d+)/) { $line = $1; next }
 	defined($line) or next;
base-commit: 2c2ba49d55ff26c1082b8137b1ec5eeccb4337d1
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help