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

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

[Greg Hurrell via GitGitGadget]

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

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

[D. Ben Knoble]

On Fri, Aug 8, 2025 at 1:43 PM Greg Hurrell via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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 :/

> 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

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

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

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

[Junio C Hamano]

"Greg Hurrell via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

> 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

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

[Phillip Wood]

On 09/08/2025 15:44, D. Ben Knoble wrote:
> On Fri, Aug 8, 2025 at 1:43 PM Greg Hurrell via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> From: Greg Hurrell <greg.hurrell@datadoghq.com>
>>
>> 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?

An alternative would be to parse the filename from the "diff --git" line 
like "git apply" does. As we're generating the diff with "--no-prefix" 
that should be straight forward as the line is "diff --git <name> 
<name>" where <name> is the name of the post-image file unless it is a 
deletion in which case it is the name of the pre-image file. We'd still 
need to check the "+++ " line or look for a "deleted file mode" line to 
handle deletions.

Thanks

Phillip

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

[Phillip Wood]

On 10/08/2025 11:09, Phillip Wood wrote:
> On 09/08/2025 15:44, D. Ben Knoble wrote:
>> On Fri, Aug 8, 2025 at 1:43 PM Greg Hurrell via GitGitGadget
>> <gitgitgadget@gmail.com> wrote:
>>> From: Greg Hurrell <greg.hurrell@datadoghq.com>
>>>
>>> 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".

When I wrote earlier I forgot that git quotes filenames with control 
characters. If a name contains a tab it it quoted and so cases 2 and 3 
will be quoted and so there is no ambiguity when trimming a literal tab 
character from the end. I haven't checked but I suspect git-jump does 
not handle quoted filenames, if we wanted to add support it should be 
pretty easy as Git.pm has a function to do the unquoting for us.

Thanks

Phillip

>>>
>>> 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?
> 
> An alternative would be to parse the filename from the "diff --git" line 
> like "git apply" does. As we're generating the diff with "--no-prefix" 
> that should be straight forward as the line is "diff --git <name> 
> <name>" where <name> is the name of the post-image file unless it is a 
> deletion in which case it is the name of the pre-image file. We'd still 
> need to check the "+++ " line or look for a "deleted file mode" line to 
> handle deletions.
> 
> Thanks
> 
> Phillip

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

[Greg Hurrell via GitGitGadget]

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", `git diff --no-prefix` will emit:

    +++ foo.txt

but for "foo bar.txt" it will emit:

    +++ foo bar.txt\t

This in turn leads `git-jump` 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", Git will emit "foo".
2. For file "foo bar", Git will emit "foo bar\t".
3. For file "foo\t", Git will emit "\"foo\t\"".
4. For file "foo bar\t", Git will emit "\"foo bar\t\"".

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

After this commit, `git-jump` correctly handles cases "1" and "2". In
reality, these are the only cases people are going to run into with any
regularity, and the other two are rare edge cases, which probably aren't
worth the effort to support unless somebody actually complains about
them.

Signed-off-by: Greg Hurrell <greg.hurrell@datadoghq.com>
---
    git-jump: make diff work with filenames containing spaces
    
    Changed since v1:
    
     * No code changes, but reworded commit message to include examples of
       quoted paths.
    
    Turns out that quoted paths never worked, so this commit isn't "robbing
    Peter to pay Paul", but rather, "giving something to Paul for free
    (Peter, sadly, is still out of luck)".

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1950%2Fwincent%2Fstrip-trailing-tab-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1950/wincent/strip-trailing-tab-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1950

Range-diff vs v1:

 1:  afe01c156e5 ! 1:  03fa9ac1ab2 git-jump: make `diff` work with filenames containing spaces
     @@ Commit message
                              strchr(line, ' ') ? "\t" : "");
                      break;
      
     -    That is, for a file "foo.txt" we'll emit:
     +    That is, for a file "foo.txt", `git diff --no-prefix` will emit:
      
     -        +++ a/foo.txt
     +        +++ foo.txt
      
     -    but for "foo bar.txt" we'll emit:
     +    but for "foo bar.txt" it will emit:
      
     -        +++ a/foo bar.txt\t
     +        +++ foo bar.txt\t
      
     -    This in turn leads us to produce a quickfix format like this:
     +    This in turn leads `git-jump` to produce a quickfix format like this:
      
              foo bar.txt\t:1:1:contents
      
     @@ Commit message
          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".
     +    1. For file "foo", Git will emit "foo".
     +    2. For file "foo bar", Git will emit "foo bar\t".
     +    3. For file "foo\t", Git will emit "\"foo\t\"".
     +    4. For file "foo bar\t", Git will emit "\"foo bar\t\"".
      
     -    Before this commit, `git-jump` correctly handled only case "2".
     +    Before this commit, `git-jump` correctly handled only case "1".
      
     -    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.
     +    After this commit, `git-jump` correctly handles cases "1" and "2". In
     +    reality, these are the only cases people are going to run into with any
     +    regularity, and the other two are rare edge cases, which probably aren't
     +    worth the effort to support unless somebody actually complains about
     +    them.
      
          Signed-off-by: Greg Hurrell <greg.hurrell@datadoghq.com>
      


 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

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

[Phillip Wood]

Hi Greg

On 11/08/2025 12:55, Greg Hurrell via GitGitGadget wrote:
> From: Greg Hurrell <greg.hurrell@datadoghq.com>
> [...]
> 1. For file "foo", Git will emit "foo".
> 2. For file "foo bar", Git will emit "foo bar\t".
> 3. For file "foo\t", Git will emit "\"foo\t\"".
> 4. For file "foo bar\t", Git will emit "\"foo bar\t\"".
> 
> Before this commit, `git-jump` correctly handled only case "1".
> 
> After this commit, `git-jump` correctly handles cases "1" and "2". In
> reality, these are the only cases people are going to run into with any
> regularity, and the other two are rare edge cases, which probably aren't
> worth the effort to support unless somebody actually complains about
> them.

Thanks for updating the commit message, I agree it's probably not worth 
worrying about cases 3 & 4 unless someone complains

Thanks

Phillip

> Signed-off-by: Greg Hurrell <greg.hurrell@datadoghq.com>
> ---
>      git-jump: make diff work with filenames containing spaces
>      
>      Changed since v1:
>      
>       * No code changes, but reworded commit message to include examples of
>         quoted paths.
>      
>      Turns out that quoted paths never worked, so this commit isn't "robbing
>      Peter to pay Paul", but rather, "giving something to Paul for free
>      (Peter, sadly, is still out of luck)".
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1950%2Fwincent%2Fstrip-trailing-tab-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1950/wincent/strip-trailing-tab-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1950
> 
> Range-diff vs v1:
> 
>   1:  afe01c156e5 ! 1:  03fa9ac1ab2 git-jump: make `diff` work with filenames containing spaces
>       @@ Commit message
>                                strchr(line, ' ') ? "\t" : "");
>                        break;
>        
>       -    That is, for a file "foo.txt" we'll emit:
>       +    That is, for a file "foo.txt", `git diff --no-prefix` will emit:
>        
>       -        +++ a/foo.txt
>       +        +++ foo.txt
>        
>       -    but for "foo bar.txt" we'll emit:
>       +    but for "foo bar.txt" it will emit:
>        
>       -        +++ a/foo bar.txt\t
>       +        +++ foo bar.txt\t
>        
>       -    This in turn leads us to produce a quickfix format like this:
>       +    This in turn leads `git-jump` to produce a quickfix format like this:
>        
>                foo bar.txt\t:1:1:contents
>        
>       @@ Commit message
>            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".
>       +    1. For file "foo", Git will emit "foo".
>       +    2. For file "foo bar", Git will emit "foo bar\t".
>       +    3. For file "foo\t", Git will emit "\"foo\t\"".
>       +    4. For file "foo bar\t", Git will emit "\"foo bar\t\"".
>        
>       -    Before this commit, `git-jump` correctly handled only case "2".
>       +    Before this commit, `git-jump` correctly handled only case "1".
>        
>       -    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.
>       +    After this commit, `git-jump` correctly handles cases "1" and "2". In
>       +    reality, these are the only cases people are going to run into with any
>       +    regularity, and the other two are rare edge cases, which probably aren't
>       +    worth the effort to support unless somebody actually complains about
>       +    them.
>        
>            Signed-off-by: Greg Hurrell <greg.hurrell@datadoghq.com>
>        
> 
> 
>   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

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

[D. Ben Knoble]

On Mon, Aug 11, 2025 at 9:15 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Greg
>
> On 11/08/2025 12:55, Greg Hurrell via GitGitGadget wrote:
> > From: Greg Hurrell <greg.hurrell@datadoghq.com>
> > [...]
> > 1. For file "foo", Git will emit "foo".
> > 2. For file "foo bar", Git will emit "foo bar\t".
> > 3. For file "foo\t", Git will emit "\"foo\t\"".
> > 4. For file "foo bar\t", Git will emit "\"foo bar\t\"".
> >
> > Before this commit, `git-jump` correctly handled only case "1".
> >
> > After this commit, `git-jump` correctly handles cases "1" and "2". In
> > reality, these are the only cases people are going to run into with any
> > regularity, and the other two are rare edge cases, which probably aren't
> > worth the effort to support unless somebody actually complains about
> > them.
>
> Thanks for updating the commit message, I agree it's probably not worth
> worrying about cases 3 & 4 unless someone complains
>
> Thanks
>
> Phillip

Agreed, and fine by me (since we have a strict improvement).

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

[Jeff King]

On Sun, Aug 10, 2025 at 02:20:13PM +0100, Phillip Wood wrote:

> On 10/08/2025 11:09, Phillip Wood wrote:
> > On 09/08/2025 15:44, D. Ben Knoble wrote:
> > > On Fri, Aug 8, 2025 at 1:43 PM Greg Hurrell via GitGitGadget
> > > <gitgitgadget@gmail.com> wrote:
> > > > From: Greg Hurrell <greg.hurrell@datadoghq.com>
> > > > 
> > > > 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".
> 
> When I wrote earlier I forgot that git quotes filenames with control
> characters. If a name contains a tab it it quoted and so cases 2 and 3 will
> be quoted and so there is no ambiguity when trimming a literal tab character
> from the end. I haven't checked but I suspect git-jump does not handle
> quoted filenames, if we wanted to add support it should be pretty easy as
> Git.pm has a function to do the unquoting for us.

Yeah, git-jump does not do any unquoting at all. Ironically I used the
"+++" line because I wanted to avoid quoting and whitespace headaches on
the "diff --git" line. But I guess it is unavoidable for truly weird
path names. ;)

I'd prefer to avoid an extra dependency on Git.pm and just leave it
broken for quoted names. Since names with spaces are the likely thing to
see, and those aren't quoted, I think running into this should be pretty
rare (another alternative is to lazy-load Git.pm only when necessary,
since we're already in a perl script).

I wondered if it might almost work without any intelligence on the part
of git-jump, just because vim's quickfix parser already does a bunch of
heuristic regex matches, some of which understand quotes. But it looks
like the answer is no. This one from the default set:

  "%f"%*\D%l: %m

is quite close, and would match:

  echo content >'foo bar'
  vim -q <(printf '"foo bar":1: some error')

but it won't automatically undo backslash escapes inside the quoted
portion. So in something that actually needed quoting would end up
looking for a file with the literal sequence "\t" in it, rather than a
tab. Oh well.

-Peff

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

[Jeff King]

On Mon, Aug 11, 2025 at 11:55:23AM +0000, Greg Hurrell via GitGitGadget wrote:

> This commit takes the simple approach of unconditionally stripping any
> trailing tab. Consider the following three examples:
> 
> 1. For file "foo", Git will emit "foo".
> 2. For file "foo bar", Git will emit "foo bar\t".
> 3. For file "foo\t", Git will emit "\"foo\t\"".
> 4. For file "foo bar\t", Git will emit "\"foo bar\t\"".
> 
> Before this commit, `git-jump` correctly handled only case "1".
> 
> After this commit, `git-jump` correctly handles cases "1" and "2". In
> reality, these are the only cases people are going to run into with any
> regularity, and the other two are rare edge cases, which probably aren't
> worth the effort to support unless somebody actually complains about
> them.

Thanks for laying out these cases. I think this list shows that we are
making things strictly better, but just stopping short of handling
unquoting. So it seems like a no-brainer to take this patch (though I
think even with the description in v1, in which we thought we might
regress "foo\t", I think it would still have been worth it).

And I think this is a good stopping point. Handling unquoting would be
tricky for a case that is unlikely to come up in practice. And I'm not
even sure we could make it foolproof, anyway. We're feeding this to the
editor's quickfix parser, so I'm not sure how we'd represent something
like a newline. We'd have to agree on the quoting scheme with the
editor, and from my (admittedly brief) research, there is not even a
mechanism in vim for that.

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

The patch itself looks good. Nice and simple, and should not incur any
extra cost or regress any other cases.

-Peff

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

[Phillip Wood]

Hi Peff

On 15/08/2025 00:14, Jeff King wrote:
> On Sun, Aug 10, 2025 at 02:20:13PM +0100, Phillip Wood wrote:
>> On 10/08/2025 11:09, Phillip Wood wrote:
>> 
>> When I wrote earlier I forgot that git quotes filenames with control
>> characters. If a name contains a tab it it quoted and so cases 2 and 3 will
>> be quoted and so there is no ambiguity when trimming a literal tab character
>> from the end. I haven't checked but I suspect git-jump does not handle
>> quoted filenames, if we wanted to add support it should be pretty easy as
>> Git.pm has a function to do the unquoting for us.
> 
> Yeah, git-jump does not do any unquoting at all. Ironically I used the
> "+++" line because I wanted to avoid quoting and whitespace headaches on
> the "diff --git" line. But I guess it is unavoidable for truly weird
> path names. ;)
> 
> I'd prefer to avoid an extra dependency on Git.pm and just leave it
> broken for quoted names. Since names with spaces are the likely thing to
> see, and those aren't quoted, I think running into this should be pretty
> rare (another alternative is to lazy-load Git.pm only when necessary,
> since we're already in a perl script).

I agree there's no pressing need to handle quoted names.

Thanks

Phillip