* Re: [PATCH] git-jump: make `diff` work with filenames containing spaces
2025-08-08 17:42 [PATCH] git-jump: make `diff` work with filenames containing spaces Greg Hurrell via GitGitGadget
@ 2025-08-09 14:44 ` D. Ben Knoble
2025-08-10 10:09 ` Phillip Wood
2025-08-10 0:03 ` Junio C Hamano
2025-08-11 11:55 ` [PATCH v2] " Greg Hurrell via GitGitGadget
2 siblings, 1 reply; 11+ messages in thread
From: D. Ben Knoble @ 2025-08-09 14:44 UTC (permalink / raw)
To: Greg Hurrell via GitGitGadget; +Cc: git, Greg Hurrell
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] git-jump: make `diff` work with filenames containing spaces
2025-08-09 14:44 ` D. Ben Knoble
@ 2025-08-10 10:09 ` Phillip Wood
2025-08-10 13:20 ` Phillip Wood
0 siblings, 1 reply; 11+ messages in thread
From: Phillip Wood @ 2025-08-10 10:09 UTC (permalink / raw)
To: D. Ben Knoble, Greg Hurrell via GitGitGadget; +Cc: git, Greg Hurrell
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] git-jump: make `diff` work with filenames containing spaces
2025-08-10 10:09 ` Phillip Wood
@ 2025-08-10 13:20 ` Phillip Wood
2025-08-14 23:14 ` Jeff King
0 siblings, 1 reply; 11+ messages in thread
From: Phillip Wood @ 2025-08-10 13:20 UTC (permalink / raw)
To: D. Ben Knoble, Greg Hurrell via GitGitGadget; +Cc: git, Greg Hurrell
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] git-jump: make `diff` work with filenames containing spaces
2025-08-10 13:20 ` Phillip Wood
@ 2025-08-14 23:14 ` Jeff King
2025-08-15 15:51 ` Phillip Wood
0 siblings, 1 reply; 11+ messages in thread
From: Jeff King @ 2025-08-14 23:14 UTC (permalink / raw)
To: Phillip Wood
Cc: D. Ben Knoble, Greg Hurrell via GitGitGadget, git, Greg Hurrell
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] git-jump: make `diff` work with filenames containing spaces
2025-08-14 23:14 ` Jeff King
@ 2025-08-15 15:51 ` Phillip Wood
0 siblings, 0 replies; 11+ messages in thread
From: Phillip Wood @ 2025-08-15 15:51 UTC (permalink / raw)
To: Jeff King; +Cc: D. Ben Knoble, Greg Hurrell via GitGitGadget, git, Greg Hurrell
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] git-jump: make `diff` work with filenames containing spaces
2025-08-08 17:42 [PATCH] git-jump: make `diff` work with filenames containing spaces Greg Hurrell via GitGitGadget
2025-08-09 14:44 ` D. Ben Knoble
@ 2025-08-10 0:03 ` Junio C Hamano
2025-08-11 11:55 ` [PATCH v2] " Greg Hurrell via GitGitGadget
2 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2025-08-10 0:03 UTC (permalink / raw)
To: Jeff King; +Cc: git, Greg Hurrell, Greg Hurrell via GitGitGadget
"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
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v2] git-jump: make `diff` work with filenames containing spaces
2025-08-08 17:42 [PATCH] git-jump: make `diff` work with filenames containing spaces Greg Hurrell via GitGitGadget
2025-08-09 14:44 ` D. Ben Knoble
2025-08-10 0:03 ` Junio C Hamano
@ 2025-08-11 11:55 ` Greg Hurrell via GitGitGadget
2025-08-11 13:16 ` Phillip Wood
2025-08-14 23:18 ` Jeff King
2 siblings, 2 replies; 11+ messages in thread
From: Greg Hurrell via GitGitGadget @ 2025-08-11 11:55 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Phillip Wood, Greg Hurrell, Greg Hurrell
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2] git-jump: make `diff` work with filenames containing spaces
2025-08-11 11:55 ` [PATCH v2] " Greg Hurrell via GitGitGadget
@ 2025-08-11 13:16 ` Phillip Wood
2025-08-11 21:05 ` D. Ben Knoble
2025-08-14 23:18 ` Jeff King
1 sibling, 1 reply; 11+ messages in thread
From: Phillip Wood @ 2025-08-11 13:16 UTC (permalink / raw)
To: Greg Hurrell via GitGitGadget, git
Cc: D. Ben Knoble, Phillip Wood, Greg Hurrell
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] git-jump: make `diff` work with filenames containing spaces
2025-08-11 13:16 ` Phillip Wood
@ 2025-08-11 21:05 ` D. Ben Knoble
0 siblings, 0 replies; 11+ messages in thread
From: D. Ben Knoble @ 2025-08-11 21:05 UTC (permalink / raw)
To: Phillip Wood; +Cc: Greg Hurrell via GitGitGadget, git, Greg Hurrell
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).
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] git-jump: make `diff` work with filenames containing spaces
2025-08-11 11:55 ` [PATCH v2] " Greg Hurrell via GitGitGadget
2025-08-11 13:16 ` Phillip Wood
@ 2025-08-14 23:18 ` Jeff King
1 sibling, 0 replies; 11+ messages in thread
From: Jeff King @ 2025-08-14 23:18 UTC (permalink / raw)
To: Greg Hurrell via GitGitGadget
Cc: git, D. Ben Knoble, Phillip Wood, Greg Hurrell
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
^ permalink raw reply [flat|nested] 11+ messages in thread