git-jump: make diff work with filenames containing spaces
#1950
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Welcome to GitGitGadgetHi @wincent, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that either:
You can CC potential reviewers by adding a footer to the PR description with the following syntax: NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description, Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join [email protected], where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
|
There are issues in commit 0a927ad: |
wincent
force-pushed
the
strip-trailing-tab
branch
from
0a927ad to
afe01c1
Compare
|
/allow |
|
User wincent is now allowed to use GitGitGadget. WARNING: wincent has no public email address set on GitHub; GitGitGadget needs an email address to Cc: you on your contribution, so that you receive any feedback on the Git mailing list. Go to https://github.com/settings/profile to make your preferred email public to let GitGitGadget know which email address to use. |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@375f520. |
|
On the Git mailing list, "D. Ben Knoble" wrote (reply to this): On Fri, Aug 8, 2025 at 1:43 PM Greg Hurrell via GitGitGadget
<[email protected]> wrote:
>
> From: Greg Hurrell <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Greg Hurrell via GitGitGadget" <[email protected]> writes:
> From: Greg Hurrell <[email protected]>
>
> 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 <[email protected]>
> ---
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 |
|
This branch is now known as |
|
This patch series was integrated into seen via git@83a2ca8. |
|
On the Git mailing list, Phillip Wood wrote (reply to this): On 09/08/2025 15:44, D. Ben Knoble wrote:
> On Fri, Aug 8, 2025 at 1:43 PM Greg Hurrell via GitGitGadget
> <[email protected]> wrote:
>> From: Greg Hurrell <[email protected]>
>>
>> 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 |
|
User |
|
On the Git mailing list, Phillip Wood wrote (reply to this): 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
>> <[email protected]> wrote:
>>> From: Greg Hurrell <[email protected]>
>>>
>>> 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 |
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 <[email protected]>
wincent
force-pushed
the
strip-trailing-tab
branch
from
afe01c1 to
03fa9ac
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Phillip Wood wrote (reply to this): Hi Greg
On 11/08/2025 12:55, Greg Hurrell via GitGitGadget wrote:
> From: Greg Hurrell <[email protected]>
> [...]
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> > > > 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 |
|
This patch series was integrated into seen via git@f448e19. |
|
On the Git mailing list, "D. Ben Knoble" wrote (reply to this): On Mon, Aug 11, 2025 at 9:15 AM Phillip Wood <[email protected]> wrote:
>
> Hi Greg
>
> On 11/08/2025 12:55, Greg Hurrell via GitGitGadget wrote:
> > From: Greg Hurrell <[email protected]>
> > [...]
> > 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). |
|
This patch series was integrated into seen via git@93751a5. |
|
There was a status update in the "Cooking" section about the branch "git jump" (in contrib/) fails to parse the diff header correctly when a file has a space in its name, which has been corrected. Will merge to 'next'? source: <[email protected]> |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changed since v1:
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)".
cc: "D. Ben Knoble" [email protected]
cc: Phillip Wood [email protected]