From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 11 Aug 2025 13:16:11 GMT
Subject: Re: [PATCH v2] git-jump: make `diff` work with filenames containing spaces
Message-ID: <4e2e2bea-c8e5-4343-9e70-a2bd139eb242@gmail.com>
In-Reply-To: <pull.1950.v2.git.1754913323810.gitgitgadget@gmail.com>

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