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
