git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Greg Hurrell via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: "D. Ben Knoble" <ben.knoble@gmail.com>,
	Phillip Wood <phillip.wood123@gmail.com>,
	Greg Hurrell <greg.hurrell@datadoghq.com>,
	Greg Hurrell <greg.hurrell@datadoghq.com>
Subject: [PATCH v2] git-jump: make `diff` work with filenames containing spaces
Date: Mon, 11 Aug 2025 11:55:23 +0000	[thread overview]
Message-ID: <pull.1950.v2.git.1754913323810.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1950.git.1754674979929.gitgitgadget@gmail.com>

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

  parent reply	other threads:[~2025-08-11 11:55 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 13:20     ` Phillip Wood
2025-08-14 23:14       ` Jeff King
2025-08-15 15:51         ` Phillip Wood
2025-08-10  0:03 ` Junio C Hamano
2025-08-11 11:55 ` Greg Hurrell via GitGitGadget [this message]
2025-08-11 13:16   ` [PATCH v2] " Phillip Wood
2025-08-11 21:05     ` D. Ben Knoble
2025-08-14 23:18   ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=pull.1950.v2.git.1754913323810.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=ben.knoble@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=greg.hurrell@datadoghq.com \
    --cc=phillip.wood123@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).