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

From: D. Ben Knoble <ben.knoble@gmail.com>
Date: Aug 9, 2025, 14:44 UTC
Message-ID: <CALnO6CDnSXpUVQEUJr=dc1ZY6errSv2M=4EmeaOmfDvcifHvnA@mail.gmail.com>
In-Reply-To: <pull.1950.git.1754674979929.gitgitgadget@gmail.com>

On Fri, Aug 8, 2025 at 1:43 PM Greg Hurrell via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 22 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
>

A little spelunking dates this back to 1a9eb3b9d5 (git-diff/git-apply: make diff output a bit friendlier to GNU patch (part 2), 2006-09-22), so we may be stuck with it :/

Show 6 quoted lines

> 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.

I can reproduce this with

    echo 1 >'a b' && git add --intent-to-add a?b && git jump diff

Show 13 quoted lines

>
> 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.

So we drop support for case 2? Hm. I personally try to avoid this situation anyway, but it would be nice if we could just do the right thing here. Or maybe we should consider trying to parse --patch-with-raw output for the filenames?

Show 36 quoted lines

>
> 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>
> ---
>     git-jump: make diff work with filenames containing spaces
>
> 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
> --
> gitgitgadget
>

This fix works as claimed and drops case (2) above, as discussed, so if we don't keep support for that then this looks right to me.

-- 
D. Ben Knoble