Re: [PATCH] git-jump: make `diff` work with filenames containing spaces
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 10, 2025, 00:03 UTC
- Message-ID
- <xmqqjz3c59hd.fsf@gitster.g>
- In-Reply-To
- <pull.1950.git.1754674979929.gitgitgadget@gmail.com>
"Greg Hurrell via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 49 quoted lines
> From: Greg Hurrell <greg.hurrell@datadoghq.com> > > 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 <greg.hurrell@datadoghq.com> > ---
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.
Show 22 quoted lines
> 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