{"thread":{"id":"62111","subject":"[PATCH 0/2] a few git-jump quality-of-life fixes","startedAt":"2024-09-15T11:11:20Z","lastAt":"2024-09-25T17:12:02Z","messageCount":10,"participants":["Jeff King","Martin Ågren","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"502801","messageId":"20240915111119.GA2017770@coredump.intra.peff.net","threadId":"62111","inReplyTo":null,"subject":"[PATCH 0/2] a few git-jump quality-of-life fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-15T11:11:19Z","receivedAt":"2024-09-15T11:11:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here are fixes for some small corner-case annoyances I found using\n\"git-jump diff\".\n\n [1/2]: git-jump: always specify column 1 for diff entries\n [2/2]: git-jump: ignore deleted files in diff mode\n\n contrib/git-jump/git-jump | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\n-Peff\n"},{"id":"502802","messageId":"20240915111846.GA2017851@coredump.intra.peff.net","threadId":"62111","inReplyTo":"20240915111119.GA2017770@coredump.intra.peff.net","subject":"[PATCH 1/2] git-jump: always specify column 1 for diff entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-15T11:18:46Z","receivedAt":"2024-09-15T11:18:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we generate a quickfix entry for a diff hunk, we provide just the\nfilename and line number along with the content, like:\n\n file:1: contents of the line\n\nThis can be a problem if the line itself looks like a quickfix header.\nFor example (and this is adapted from a real-world case that bit me):\n\n echo 'static_lease 10:11:12:13:14:15:16 10.0.0.1' >file\n git add file\n echo change >file\n\nproduces:\n\n file:1: static_lease 10:11:12:13:14:15:16 10.0.0.1\n\nwhich is ambiguous. It could be line 1 of \"file\", or line 11 of the file\n\"file:1: static_lease 10\", and so on. In the case of vim's default\nconfig, it seems to prefer the latter (you can configure \"errorformat\"\nwith a variety of patterns, but out of the box it matches some common\nones).\n\nOne easy way to fix this is to provide a column number, like:\n\n file:1:1: static_lease 10:11:12:13:14:15:16 10.0.0.1\n\nwhich causes vim to prefer line 1 of \"file\" again (due to the preference\norder of the various patterns in the default errorformat).\n\nThere are other options. For example, at least in my version of vim,\nwrapping the file in quotation marks like:\n\n \"file\":1: static_lease 10:11:12:13:14:15:16 10.0.0.1\n\nalso works. That perhaps would the right thing even if you had the silly\nfile name \"file:1:1: foo 10\". But it's not clear what would happen if\nyou had a filename with quotes in it.\n\nThis feature is inherently scraping text, and there's bound to be some\nambiguities. I don't think it's worth worrying too much about unlikely\nfilenames, as its the file content that is more likely to introduce\nunexpected characters.\n\nSo let's just go with the extra \":1\" column specifier. We know this is\nsupported everywhere, as git-jump's \"grep\" mode already uses it (and\nthus doesn't exhibit the same problem).\n\nThe \"merge\" mode is mostly immune to this, as it only matches \"<<<<<<<\"\nconflict marker lines. It's possible of course to have a marker that\nsays \"foo 10:11\" later in the line, but in practice these will only have\nbranches and perhaps file names, so it's probably not worth worrying\nabout (and fixing it would involve passing --column to the system grep,\nwhich may not be portable).\n\nI also gave some thought as to whether we could put something more\nuseful than \"1\" in the column field for diffs. In theory we could find\nthe first changed character of the line, but this is tricky in practice.\nYou'd have to correlate before/after lines of the hunk to decide what\nchanged. So:\n\n -this is a foo line\n +this is a bar line\n\nis easy (column 11). But:\n\n -this is a foo line\n +another line\n +this is a bar line\n\nis harder.\n\nThis commit certainly doesn't preclude trying to do something more\nclever later, but it's a much deeper rabbit hole than just fixing the\nsyntactic ambiguity.\n\nSigned-off-by: Jeff King \n---\n contrib/git-jump/git-jump | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump\nindex 47e0c557e6..78e7394406 100755\n--- a/contrib/git-jump/git-jump\n+++ b/contrib/git-jump/git-jump\n@@ -50,7 +50,7 @@ mode_diff() {\n \tdefined($line) or next;\n \tif (/^ /) { $line++; next }\n \tif (/^[-+]\\s*(.*)/) {\n-\t\tprint \"$file:$line: $1\\n\";\n+\t\tprint \"$file:$line:1: $1\\n\";\n \t\t$line = undef;\n \t}\n \t'\n-- \n2.46.1.893.gc4b01a7614\n\n"},{"id":"502803","messageId":"20240915112024.GB2017851@coredump.intra.peff.net","threadId":"62111","inReplyTo":"20240915111119.GA2017770@coredump.intra.peff.net","subject":"[PATCH 2/2] git-jump: ignore deleted files in diff mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-15T11:20:24Z","receivedAt":"2024-09-15T11:20:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you do something like this:\n\n rm file_a\n echo change >file_b\n git jump diff\n\nthen we'll generate two quickfix entries for the diff, one for each\nfile. But the one for the deleted file is rather pointless. There's no\ncontent to show since the file is gone, and in fact we open the editor\nwith the path /dev/null!\n\nIn vim, at least, the result is a confusing annoyance: the editor opens\nwith an empty buffer, and you have to skip past it to the useful\nquickfix entry (after scratching your head and figuring out that no,\nnothing is broken).\n\nLet's skip such entries entirely. There's nothing useful to show, since\nthe point is that the file has been deleted.\n\nIt is possible that you could be doing a diff whose post-image is not\nthe working tree, and then you'd perhaps be jumping to the deleted\ncontent (or at least something that was in the same spot). But I don't\nthink it's worth worrying about that case. For one thing, using git-jump\nfor such diffs is a bad idea in general, as it's going to sometimes move\nyou to the wrong spot. And two, a deletion is always going to have one\nhunk starting at line 1, which is not that interesting to jump to in the\nfirst place.\n\nSigned-off-by: Jeff King \n---\n contrib/git-jump/git-jump | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump\nindex 78e7394406..3f69675961 100755\n--- a/contrib/git-jump/git-jump\n+++ b/contrib/git-jump/git-jump\n@@ -44,7 +44,7 @@ open_editor() {\n mode_diff() {\n \tgit diff --no-prefix --relative \"$@\" |\n \tperl -ne '\n-\tif (m{^\\+\\+\\+ (.*)}) { $file = $1; next }\n+\tif (m{^\\+\\+\\+ (.*)}) { $file = $1 eq \"/dev/null\" ? undef : $1; next }\n \tdefined($file) or next;\n \tif (m/^@@ .*?\\+(\\d+)/) { $line = $1; next }\n \tdefined($line) or next;\n-- \n2.46.1.893.gc4b01a7614\n"},{"id":"502908","messageId":"CAN0heSrQiV6WOP4C5mFPh1AMgX9yy3CcHx2RPraKMFtmNJ2fPw@mail.gmail.com","threadId":"62111","inReplyTo":"20240915111846.GA2017851@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] git-jump: always specify column 1 for diff entries","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2024-09-16T20:13:39Z","receivedAt":"2024-09-16T20:13:53Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Sun, 15 Sept 2024 at 13:18, Jeff King wrote:\n>\n> When we generate a quickfix entry for a diff hunk, we provide just the\n> filename and line number along with the content, like:\n>\n> file:1: contents of the line\n>\n> This can be a problem if the line itself looks like a quickfix header.\n> For example (and this is adapted from a real-world case that bit me):\n>\n> echo 'static_lease 10:11:12:13:14:15:16 10.0.0.1' >file\n> git add file\n> echo change >file\n>\n> produces:\n>\n> file:1: static_lease 10:11:12:13:14:15:16 10.0.0.1\n>\n> which is ambiguous. It could be line 1 of \"file\", or line 11 of the file\n> \"file:1: static_lease 10\", and so on. In the case of vim's default\n> config, it seems to prefer the latter (you can configure \"errorformat\"\n> with a variety of patterns, but out of the box it matches some common\n> ones).\n\nI've never hit this, but it doesn't look too crazy. A couple of digits\nand a colon and things begin to match. Ok.\n\n> One easy way to fix this is to provide a column number, like:\n>\n> file:1:1: static_lease 10:11:12:13:14:15:16 10.0.0.1\n>\n> which causes vim to prefer line 1 of \"file\" again (due to the preference\n> order of the various patterns in the default errorformat).\n\nMakes sense.\n\n> There are other options. For example, at least in my version of vim,\n> wrapping the file in quotation marks like:\n>\n> \"file\":1: static_lease 10:11:12:13:14:15:16 10.0.0.1\n>\n> also works. That perhaps would the right thing even if you had the silly\n> file name \"file:1:1: foo 10\". But it's not clear what would happen if\n> you had a filename with quotes in it.\n\nRight. Looking around, I can find someone asking the Internet how to\nescape the filename and not getting any response.\n\n> This feature is inherently scraping text, and there's bound to be some\n> ambiguities. I don't think it's worth worrying too much about unlikely\n> filenames, as its the file content that is more likely to introduce\n> unexpected characters.\n\nAgreed. (s/its/it's/)\n\n> So let's just go with the extra \":1\" column specifier. We know this is\n> supported everywhere, as git-jump's \"grep\" mode already uses it (and\n> thus doesn't exhibit the same problem).\n>\n> The \"merge\" mode is mostly immune to this, as it only matches \"<<<<<<<\"\n> conflict marker lines. It's possible of course to have a marker that\n> says \"foo 10:11\" later in the line, but in practice these will only have\n> branches and perhaps file names, so it's probably not worth worrying\n> about (and fixing it would involve passing --column to the system grep,\n> which may not be portable).\n\nI suppose we could use `git grep --no-index` instead of `grep` for `git\njump merge`. Anyway, that's out of scope here.\n\n> I also gave some thought as to whether we could put something more\n> useful than \"1\" in the column field for diffs. In theory we could find\n\nHeh. Yes, in theory everything is possible. Your approach makes sense.\n\n> - print \"$file:$line: $1\\n\";\n> + print \"$file:$line:1: $1\\n\";\n\nLooks good to me and from my testing, this fixes the problem as\ndescribed.\n\nMartin\n"},{"id":"502909","messageId":"CAN0heSpk0f1-8tbguqAyTOQbuWrHYpkE-W6H29CJ-QBzqULCrg@mail.gmail.com","threadId":"62111","inReplyTo":"20240915112024.GB2017851@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] git-jump: ignore deleted files in diff mode","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2024-09-16T20:14:55Z","receivedAt":"2024-09-16T20:15:08Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Sun, 15 Sept 2024 at 13:20, Jeff King wrote:\n>\n> If you do something like this:\n>\n> rm file_a\n> echo change >file_b\n> git jump diff\n>\n> then we'll generate two quickfix entries for the diff, one for each\n> file. But the one for the deleted file is rather pointless. There's no\n> content to show since the file is gone, and in fact we open the editor\n> with the path /dev/null!\n\nAh, yes, I've seen this. I just went \"I can see why this would happen\"\nand moved on to the next hunk. Thanks for actually patching.\n\n> Let's skip such entries entirely. There's nothing useful to show, since\n> the point is that the file has been deleted.\n\nAgreed. Anybody who relies on `git jump diff` to produce an empty\nquickfix list (no entries to go through) if *and only if* the diff is\nempty is already in for a surprise due to binary files. Skipping\n/dev/null makes perfect sense.\n\n> perl -ne '\n> - if (m{^\\+\\+\\+ (.*)}) { $file = $1; next }\n> + if (m{^\\+\\+\\+ (.*)}) { $file = $1 eq \"/dev/null\" ? undef : $1; next }\n> defined($file) or next;\n\nThis looks very reasonable. From my testing, this patch fixes the issue.\n\nMartin\n"},{"id":"502921","messageId":"ZulEwjnNQet6th8w@nand.local","threadId":"62111","inReplyTo":"20240915112024.GB2017851@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] git-jump: ignore deleted files in diff mode","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-09-17T08:58:42Z","receivedAt":"2024-09-17T08:58:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Sep 15, 2024 at 07:20:24AM -0400, Jeff King wrote:\n> diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump\n> index 78e7394406..3f69675961 100755\n> --- a/contrib/git-jump/git-jump\n> +++ b/contrib/git-jump/git-jump\n> @@ -44,7 +44,7 @@ open_editor() {\n> mode_diff() {\n> \tgit diff --no-prefix --relative \"$@\" |\n> \tperl -ne '\n> -\tif (m{^\\+\\+\\+ (.*)}) { $file = $1; next }\n> +\tif (m{^\\+\\+\\+ (.*)}) { $file = $1 eq \"/dev/null\" ? undef : $1; next }\n\nI was surprised to not see you use `--diff-filter` here, but I think\nthat that makes sense. You only would want to exclude deletions, since\nthat would be the only time the post-image is /dev/null AFAICT.\n\nBut I guess that would make it impossible to do \"git jump diff\n--diff-filter\", which may be useful in some cases. TBH, I have almost\nnever used --diff-filter myself, so I'm not sure how common that is.\n\nIn any event, this seems to be quite a reasonable implementation to me.\n\nThanks,\nTaylor\n"},{"id":"502957","messageId":"xmqqed5il3ui.fsf@gitster.g","threadId":"62111","inReplyTo":"ZulEwjnNQet6th8w@nand.local","subject":"Re: [PATCH 2/2] git-jump: ignore deleted files in diff mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-17T15:03:33Z","receivedAt":"2024-09-17T15:03:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Sun, Sep 15, 2024 at 07:20:24AM -0400, Jeff King wrote:\n>> diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump\n>> index 78e7394406..3f69675961 100755\n>> --- a/contrib/git-jump/git-jump\n>> +++ b/contrib/git-jump/git-jump\n>> @@ -44,7 +44,7 @@ open_editor() {\n>> mode_diff() {\n>> \tgit diff --no-prefix --relative \"$@\" |\n>> \tperl -ne '\n>> -\tif (m{^\\+\\+\\+ (.*)}) { $file = $1; next }\n>> +\tif (m{^\\+\\+\\+ (.*)}) { $file = $1 eq \"/dev/null\" ? undef : $1; next }\n>\n> I was surprised to not see you use `--diff-filter` here, but I think\n> that that makes sense. You only would want to exclude deletions, since\n> that would be the only time the post-image is /dev/null AFAICT.\n\nSo \"--diff-filter=d\" (lowercase)?\n"},{"id":"503397","messageId":"20240924205558.GC586150@coredump.intra.peff.net","threadId":"62111","inReplyTo":"xmqqed5il3ui.fsf@gitster.g","subject":"Re: [PATCH 2/2] git-jump: ignore deleted files in diff mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T20:55:58Z","receivedAt":"2024-09-24T20:55:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 17, 2024 at 08:03:33AM -0700, Junio C Hamano wrote:\n\n> Taylor Blau writes:\n> \n> > On Sun, Sep 15, 2024 at 07:20:24AM -0400, Jeff King wrote:\n> >> diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump\n> >> index 78e7394406..3f69675961 100755\n> >> --- a/contrib/git-jump/git-jump\n> >> +++ b/contrib/git-jump/git-jump\n> >> @@ -44,7 +44,7 @@ open_editor() {\n> >> mode_diff() {\n> >> \tgit diff --no-prefix --relative \"$@\" |\n> >> \tperl -ne '\n> >> -\tif (m{^\\+\\+\\+ (.*)}) { $file = $1; next }\n> >> +\tif (m{^\\+\\+\\+ (.*)}) { $file = $1 eq \"/dev/null\" ? undef : $1; next }\n> >\n> > I was surprised to not see you use `--diff-filter` here, but I think\n> > that that makes sense. You only would want to exclude deletions, since\n> > that would be the only time the post-image is /dev/null AFAICT.\n> \n> So \"--diff-filter=d\" (lowercase)?\n\nHmm, yeah, I think that probably would work. In my mind, we are\naccepting arbitrary diffs from the user because of the \"$@\". But anybody\nwho overrides us with\n\n git jump diff --diff-filter=D\n\nmaybe gets what they want/deserve. I think it's mostly academic, and as\nthe original has already graduated, I'm inclined to leave it unless\nsomebody comes back with some use case.\n\n(Sorry for the slow reply, I am quite behind due to travel last week).\n\n-Peff\n"},{"id":"503428","messageId":"ZvM3N3+JJNlkpZSa@nand.local","threadId":"62111","inReplyTo":"20240924205558.GC586150@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] git-jump: ignore deleted files in diff mode","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-09-24T22:03:35Z","receivedAt":"2024-09-24T22:03:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 24, 2024 at 04:55:58PM -0400, Jeff King wrote:\n> On Tue, Sep 17, 2024 at 08:03:33AM -0700, Junio C Hamano wrote:\n>\n> > Taylor Blau writes:\n> >\n> > > On Sun, Sep 15, 2024 at 07:20:24AM -0400, Jeff King wrote:\n> > >> diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump\n> > >> index 78e7394406..3f69675961 100755\n> > >> --- a/contrib/git-jump/git-jump\n> > >> +++ b/contrib/git-jump/git-jump\n> > >> @@ -44,7 +44,7 @@ open_editor() {\n> > >> mode_diff() {\n> > >> \tgit diff --no-prefix --relative \"$@\" |\n> > >> \tperl -ne '\n> > >> -\tif (m{^\\+\\+\\+ (.*)}) { $file = $1; next }\n> > >> +\tif (m{^\\+\\+\\+ (.*)}) { $file = $1 eq \"/dev/null\" ? undef : $1; next }\n> > >\n> > > I was surprised to not see you use `--diff-filter` here, but I think\n> > > that that makes sense. You only would want to exclude deletions, since\n> > > that would be the only time the post-image is /dev/null AFAICT.\n> >\n> > So \"--diff-filter=d\" (lowercase)?\n>\n> Hmm, yeah, I think that probably would work. In my mind, we are\n> accepting arbitrary diffs from the user because of the \"$@\". But anybody\n> who overrides us with\n>\n> git jump diff --diff-filter=D\n>\n> maybe gets what they want/deserve. I think it's mostly academic, and as\n> the original has already graduated, I'm inclined to leave it unless\n> somebody comes back with some use case.\n\nYeah, this is definitely all academic. I do not at all mind the\nimplementation that you went with here.\n\n> (Sorry for the slow reply, I am quite behind due to travel last week).\n\nHopefully your travel back went smoothly. Welcome back :-).\n\nThanks,\nTaylor\n"},{"id":"503482","messageId":"xmqqfrpnody8.fsf@gitster.g","threadId":"62111","inReplyTo":"ZvM3N3+JJNlkpZSa@nand.local","subject":"Re: [PATCH 2/2] git-jump: ignore deleted files in diff mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-25T17:11:59Z","receivedAt":"2024-09-25T17:12:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Yeah, this is definitely all academic. I do not at all mind the\n> implementation that you went with here.\n\nNeither do I. Thanks, both.\n\n"}]}