| Message ID | 20240915112024.GB2017851@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 083b82544d6e5b27a4b3fa7f105ba330f490227b |
| Headers | show |
| Series | a few git-jump quality-of-life fixes | expand |
On Sun, 15 Sept 2024 at 13:20, Jeff King <peff@peff.net> wrote: > > If you do something like this: > > rm file_a > echo change >file_b > git jump diff > > then we'll generate two quickfix entries for the diff, one for each > file. But the one for the deleted file is rather pointless. There's no > content to show since the file is gone, and in fact we open the editor > with the path /dev/null! Ah, yes, I've seen this. I just went "I can see why this would happen" and moved on to the next hunk. Thanks for actually patching. > Let's skip such entries entirely. There's nothing useful to show, since > the point is that the file has been deleted. Agreed. Anybody who relies on `git jump diff` to produce an empty quickfix list (no entries to go through) if *and only if* the diff is empty is already in for a surprise due to binary files. Skipping /dev/null makes perfect sense. > perl -ne ' > - if (m{^\+\+\+ (.*)}) { $file = $1; next } > + if (m{^\+\+\+ (.*)}) { $file = $1 eq "/dev/null" ? undef : $1; next } > defined($file) or next; This looks very reasonable. From my testing, this patch fixes the issue. Martin
On Sun, Sep 15, 2024 at 07:20:24AM -0400, Jeff King wrote: > diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump > index 78e7394406..3f69675961 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; next } > + if (m{^\+\+\+ (.*)}) { $file = $1 eq "/dev/null" ? undef : $1; next } I was surprised to not see you use `--diff-filter` here, but I think that that makes sense. You only would want to exclude deletions, since that would be the only time the post-image is /dev/null AFAICT. But I guess that would make it impossible to do "git jump diff --diff-filter", which may be useful in some cases. TBH, I have almost never used --diff-filter myself, so I'm not sure how common that is. In any event, this seems to be quite a reasonable implementation to me. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > On Sun, Sep 15, 2024 at 07:20:24AM -0400, Jeff King wrote: >> diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump >> index 78e7394406..3f69675961 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; next } >> + if (m{^\+\+\+ (.*)}) { $file = $1 eq "/dev/null" ? undef : $1; next } > > I was surprised to not see you use `--diff-filter` here, but I think > that that makes sense. You only would want to exclude deletions, since > that would be the only time the post-image is /dev/null AFAICT. So "--diff-filter=d" (lowercase)?
diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump index 78e7394406..3f69675961 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; next } + if (m{^\+\+\+ (.*)}) { $file = $1 eq "/dev/null" ? undef : $1; next } defined($file) or next; if (m/^@@ .*?\+(\d+)/) { $line = $1; next } defined($line) or next;
If you do something like this: rm file_a echo change >file_b git jump diff then we'll generate two quickfix entries for the diff, one for each file. But the one for the deleted file is rather pointless. There's no content to show since the file is gone, and in fact we open the editor with the path /dev/null! In vim, at least, the result is a confusing annoyance: the editor opens with an empty buffer, and you have to skip past it to the useful quickfix entry (after scratching your head and figuring out that no, nothing is broken). Let's skip such entries entirely. There's nothing useful to show, since the point is that the file has been deleted. It is possible that you could be doing a diff whose post-image is not the working tree, and then you'd perhaps be jumping to the deleted content (or at least something that was in the same spot). But I don't think it's worth worrying about that case. For one thing, using git-jump for such diffs is a bad idea in general, as it's going to sometimes move you to the wrong spot. And two, a deletion is always going to have one hunk starting at line 1, which is not that interesting to jump to in the first place. Signed-off-by: Jeff King <peff@peff.net> --- contrib/git-jump/git-jump | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)