| 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)?
On Tue, Sep 17, 2024 at 08:03:33AM -0700, Junio C Hamano wrote: > 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)? Hmm, yeah, I think that probably would work. In my mind, we are accepting arbitrary diffs from the user because of the "$@". But anybody who overrides us with git jump diff --diff-filter=D maybe gets what they want/deserve. I think it's mostly academic, and as the original has already graduated, I'm inclined to leave it unless somebody comes back with some use case. (Sorry for the slow reply, I am quite behind due to travel last week). -Peff
On Tue, Sep 24, 2024 at 04:55:58PM -0400, Jeff King wrote: > On Tue, Sep 17, 2024 at 08:03:33AM -0700, Junio C Hamano wrote: > > > 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)? > > Hmm, yeah, I think that probably would work. In my mind, we are > accepting arbitrary diffs from the user because of the "$@". But anybody > who overrides us with > > git jump diff --diff-filter=D > > maybe gets what they want/deserve. I think it's mostly academic, and as > the original has already graduated, I'm inclined to leave it unless > somebody comes back with some use case. Yeah, this is definitely all academic. I do not at all mind the implementation that you went with here. > (Sorry for the slow reply, I am quite behind due to travel last week). Hopefully your travel back went smoothly. Welcome back :-). Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > Yeah, this is definitely all academic. I do not at all mind the > implementation that you went with here. Neither do I. Thanks, both.
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(-)