{"thread":{"id":"47955","subject":"Bug report: \"Use of uninitialized value $_ in print\"","startedAt":"2018-03-02T01:19:42Z","lastAt":"2018-03-05T22:15:56Z","messageCount":23,"participants":["Sam Kuper","Jonathan Nieder","Jeff King","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"340775","messageId":"CAD-Jur+6m1SjqHVWBxW5HnTjutSVrkP+dEXdYmFHzoUf0FGdNA@mail.gmail.com","threadId":"47955","inReplyTo":null,"subject":"Bug report: \"Use of uninitialized value $_ in print\"","fromName":"Sam Kuper","fromEmail":"sam.kuper@uclmail.net","sentAt":"2018-03-02T01:19:35Z","receivedAt":"2018-03-02T01:19:42Z","isPatch":false,"sender":{"key":"sam.kuper@uclmail.net","avatar":"https://gravatar.com/avatar/ae189c4c56e5cd31dc7371016f169e68aef81a3ea3318041a1ca4be46f10435b?d=mp&s=160"},"body":"First, background. I encountered a bug on Debian Stretch, using this\ngit version:\n\n$ git --version\ngit version 2.11.0\n\n\nThe bug is that in the midst of running\n\ngit -c interactive.diffFilter=\"git diff --word-diff --color\" add --patch\n\nand after having answered \"y\" to some patches and \"n\" to others, git\nsuddenly spewed the following output:\n\n\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 74.\nStage this hunk [y,n,q,a,d,/,K,j,J,g,e,?]? n\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\nUse of uninitialized value $_ in print at\n/usr/lib/git-core/git-add--interactive line 1371, line 75.\n\n\nI hope that this bug report can help the git core maintainers to\neither fix the problem upstream, or to co-ordinate a fix with the\nDebian git maintainer(s) if the bug does not exist upstream.\n\nThanks for the great DVCS :)\n\nP.S. I am not subscribed to this mailing list, so please CC me in your\nreply if you want me to see it.\n"},{"id":"340784","messageId":"20180302070434.GG238112@aiede.svl.corp.google.com","threadId":"47955","inReplyTo":"CAD-Jur+6m1SjqHVWBxW5HnTjutSVrkP+dEXdYmFHzoUf0FGdNA@mail.gmail.com","subject":"Re: Bug report: \"Use of uninitialized value $_ in print\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-03-02T07:04:34Z","receivedAt":"2018-03-02T07:04:43Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nSam Kuper wrote:\n\n> First, background. I encountered a bug on Debian Stretch, using this\n> git version:\n>\n> $ git --version\n> git version 2.11.0\n>\n> The bug is that in the midst of running\n>\n> git -c interactive.diffFilter=\"git diff --word-diff --color\" add --patch\n>\n> and after having answered \"y\" to some patches and \"n\" to others, git\n> suddenly spewed the following output:\n>\n> Use of uninitialized value $_ in print at\n> /usr/lib/git-core/git-add--interactive line 1371, line 74.\n> Stage this hunk [y,n,q,a,d,/,K,j,J,g,e,?]? n\n> Use of uninitialized value $_ in print at\n> /usr/lib/git-core/git-add--interactive line 1371, line 75.\n[...]\n\nStrange. The relevant line, for reference:\n\n$ dpkg-deb --fsys-tarfile git_2.11.0-3+deb9u2_amd64.deb |\n tar Oxf - ./usr/lib/git-core/git-add--interactive |\n sed -n '1370,1372 p'\n\n\t\tfor (@{$hunk[$ix]{DISPLAY}}) {\n\t\t\tprint; <---- this one\n\t\t}\n\nThis is a foreach loop, so it's supposed to have set $_ to each value\nin the list @{$hunk[$ix]{DISPLAY}). So why is Perl considering it\nuninitialized?\n\nIs this reproducible for you? Do you have more details about how I\ncan reproduce it?\n\nWhat arch are you on? What perl version do you use? Can you report\nthis using \"reportbug git\"?\n\nThanks,\nJonathan\n"},{"id":"340792","messageId":"20180302104232.GA11074@sigill.intra.peff.net","threadId":"47955","inReplyTo":"CAD-Jur+6m1SjqHVWBxW5HnTjutSVrkP+dEXdYmFHzoUf0FGdNA@mail.gmail.com","subject":"Re: Bug report: \"Use of uninitialized value $_ in print\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-02T10:42:32Z","receivedAt":"2018-03-02T10:43:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 02, 2018 at 01:19:35AM +0000, Sam Kuper wrote:\n\n> The bug is that in the midst of running\n> \n> git -c interactive.diffFilter=\"git diff --word-diff --color\" add --patch\n\nThat's not how interactive.diffFilter is supposed to work.\n\nIt's meant to have the output of an existing diff piped into it.\nGenerating the diff anew will appear to work for some simple cases, but\nfall down for others:\n\n 1. Any of the flavors besides \"add -p\" will be running the wrong diff\n (e.g., \"reset -p\" runs a diff between the index and HEAD).\n\n 2. Any pathspec limiters would be ignored (e.g., \"add -p file\").\n\n 3. Your invocation in particular is a problem because it uses\n --word-diff, which will not have a one-to-one line correspondence\n with the bare diff.\n\n add--interactive handles pretty-printing by running the diff\n command twice: once with no special options, and once with\n \"--color\" and piped through the diffFilter. It assumes that the two\n match each other line for line, so it shows you the \"DISPLAY\"\n variant, but then ultimately applies the \"TEXT\" variant.\n\nAnd that last one is the cause of the errors you see:\n\n> Use of uninitialized value $_ in print at\n> /usr/lib/git-core/git-add--interactive line 1371, line 74.\n\nThe \"DISPLAY\" run for your case generates fewer lines than the \"TEXT\"\nrun, and we complain on trying to show those extra lines.\n\nUnfortunately, I don't think there's an easy way to do what you want\n(show word-diffs but apply the full diff).\n\n-Peff\n"},{"id":"340793","messageId":"20180302104608.GB11074@sigill.intra.peff.net","threadId":"47955","inReplyTo":"20180302070434.GG238112@aiede.svl.corp.google.com","subject":"Re: Bug report: \"Use of uninitialized value $_ in print\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-02T10:46:08Z","receivedAt":"2018-03-02T10:46:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 01, 2018 at 11:04:34PM -0800, Jonathan Nieder wrote:\n\n> > Use of uninitialized value $_ in print at\n> > /usr/lib/git-core/git-add--interactive line 1371, line 75.\n> [...]\n> \n> Strange. The relevant line, for reference:\n> \n> $ dpkg-deb --fsys-tarfile git_2.11.0-3+deb9u2_amd64.deb |\n> tar Oxf - ./usr/lib/git-core/git-add--interactive |\n> sed -n '1370,1372 p'\n> \n> \t\tfor (@{$hunk[$ix]{DISPLAY}}) {\n> \t\t\tprint; <---- this one\n> \t\t}\n> \n> This is a foreach loop, so it's supposed to have set $_ to each value\n> in the list @{$hunk[$ix]{DISPLAY}). So why is Perl considering it\n> uninitialized?\n\nBecause the array is full of \"undef\". See parse_diff(), which does this:\n\n my @diff = run_cmd_pipe(\"git\", @diff_cmd, \"--\", $path);\n ...\n @colored = run_cmd_pipe(@display_cmd);\n ...\n for (my $i = 0; $i < @diff; $i++) {\n ...\n push @{$hunk[-1]{TEXT}}, $diff[$i];\n push @{$hunk[-1]{DISPLAY}},\n (@colored ? $colored[$i] : $diff[$i]);\n }\n\nIf the @colored output is shorter than the normal @diff output, we'll\npush a bunch of \"undef\" onto the DISPLAY array (the ternary there is\nbecause sometimes @colored is completely empty if the user did not ask\nfor color).\n\n-Peff\n"},{"id":"340816","messageId":"xmqq1sh2e7w1.fsf@gitster-ct.c.googlers.com","threadId":"47955","inReplyTo":"20180302104608.GB11074@sigill.intra.peff.net","subject":"Re: Bug report: \"Use of uninitialized value $_ in print\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-02T16:53:34Z","receivedAt":"2018-03-02T16:53:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Because the array is full of \"undef\". See parse_diff(), which does this:\n>\n> my @diff = run_cmd_pipe(\"git\", @diff_cmd, \"--\", $path);\n> ...\n> @colored = run_cmd_pipe(@display_cmd);\n> ...\n> for (my $i = 0; $i < @diff; $i++) {\n> ...\n> push @{$hunk[-1]{TEXT}}, $diff[$i];\n> push @{$hunk[-1]{DISPLAY}},\n> (@colored ? $colored[$i] : $diff[$i]);\n> }\n>\n> If the @colored output is shorter than the normal @diff output, we'll\n> push a bunch of \"undef\" onto the DISPLAY array (the ternary there is\n> because sometimes @colored is completely empty if the user did not ask\n> for color).\n\nAn obvious sanity check would be to ensure @colored == @diff\n\n push @{$hunk[-1]{DISPLAY}},\n - (@colored ? $colored[$i] : $diff[$i]);\n + (@colored && @colored == @diff ? $colored[$i] : $diff[$i]);\n }\n\nor something like that?\n"},{"id":"340817","messageId":"20180302165543.GA4616@sigill.intra.peff.net","threadId":"47955","inReplyTo":"xmqq1sh2e7w1.fsf@gitster-ct.c.googlers.com","subject":"Re: Bug report: \"Use of uninitialized value $_ in print\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-02T16:55:43Z","receivedAt":"2018-03-02T16:55:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 02, 2018 at 08:53:34AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Because the array is full of \"undef\". See parse_diff(), which does this:\n> >\n> > my @diff = run_cmd_pipe(\"git\", @diff_cmd, \"--\", $path);\n> > ...\n> > @colored = run_cmd_pipe(@display_cmd);\n> > ...\n> > for (my $i = 0; $i < @diff; $i++) {\n> > ...\n> > push @{$hunk[-1]{TEXT}}, $diff[$i];\n> > push @{$hunk[-1]{DISPLAY}},\n> > (@colored ? $colored[$i] : $diff[$i]);\n> > }\n> >\n> > If the @colored output is shorter than the normal @diff output, we'll\n> > push a bunch of \"undef\" onto the DISPLAY array (the ternary there is\n> > because sometimes @colored is completely empty if the user did not ask\n> > for color).\n> \n> An obvious sanity check would be to ensure @colored == @diff\n> \n> push @{$hunk[-1]{DISPLAY}},\n> - (@colored ? $colored[$i] : $diff[$i]);\n> + (@colored && @colored == @diff ? $colored[$i] : $diff[$i]);\n> }\n> \n> or something like that?\n\nThat's probably a reasonable sanity check, but I think we need to abort\nand not just have a too-small DISPLAY array. Because later code like the\nhunk-splitting is going to assume that there's a 1:1 line\ncorrespondence. We definitely don't want to end up in a situation where\nwe show one thing but apply another.\n\n-Peff\n"},{"id":"340821","messageId":"xmqqo9k6csan.fsf@gitster-ct.c.googlers.com","threadId":"47955","inReplyTo":"20180302165543.GA4616@sigill.intra.peff.net","subject":"Re: Bug report: \"Use of uninitialized value $_ in print\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-02T17:15:44Z","receivedAt":"2018-03-02T17:15:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> That's probably a reasonable sanity check, but I think we need to abort\n> and not just have a too-small DISPLAY array. Because later code like the\n> hunk-splitting is going to assume that there's a 1:1 line\n> correspondence. We definitely don't want to end up in a situation where\n> we show one thing but apply another.\n\nYes, agreed completely.\n"},{"id":"340823","messageId":"CAD-Jur+juVNte7vJOi-iJYwPhbkoQOne9PTyRLD+skL_1=PgCQ@mail.gmail.com","threadId":"47955","inReplyTo":"20180302070434.GG238112@aiede.svl.corp.google.com","subject":"Re: Bug report: \"Use of uninitialized value $_ in print\"","fromName":"Sam Kuper","fromEmail":"sam.kuper@uclmail.net","sentAt":"2018-03-02T17:28:13Z","receivedAt":"2018-03-02T17:28:25Z","isPatch":false,"sender":{"key":"sam.kuper@uclmail.net","avatar":"https://gravatar.com/avatar/ae189c4c56e5cd31dc7371016f169e68aef81a3ea3318041a1ca4be46f10435b?d=mp&s=160"},"body":"On 02/03/2018, Jonathan Nieder wrote:\n> Is this reproducible for you?\n\nYes. It seems to occur consistently, given the same input.\n\n> Do you have more details about how I can reproduce it?\n\nUnfortunately, the particular git repo I encountered it on is private,\notherwise I would point you to it.\n\nI haven't attempted to create a MWE. Am I correct that doing so is now\nnot needed, in the light of Jeff King's email below?\n\n\n> What arch are you on? What perl version do you use? Can you report\n> this using \"reportbug git\"?\n\nAll perfectly decent questions. For a modicum of security/privacy, I\nwould prefer to avoid answering them unless necessary.\n\nAm I right in thinking that these answers are no longer needed, in the\nlight of Jeff King's email below?\n\n\n\nOn 02/03/2018, Jeff King wrote:\n> 3. Your invocation in particular is a problem because it uses\n> --word-diff, which will not have a one-to-one line correspondence\n> with the bare diff.\n>\n> add--interactive handles pretty-printing by running the diff\n> command twice: once with no special options, and once with\n> \"--color\" and piped through the diffFilter. It assumes that the two\n> match each other line for line, so it shows you the \"DISPLAY\"\n> variant, but then ultimately applies the \"TEXT\" variant.\n>\n> And that last one is the cause of the errors you see:\n>\n>> Use of uninitialized value $_ in print at\n>> /usr/lib/git-core/git-add--interactive line 1371, line 74.\n>\n> The \"DISPLAY\" run for your case generates fewer lines than the \"TEXT\"\n> run, and we complain on trying to show those extra lines.\n"},{"id":"340824","messageId":"CAD-JurJLDSzf7_A0ZGWPYGyRiSnHWSpUrVEd9_+N1J7BamMM=Q@mail.gmail.com","threadId":"47955","inReplyTo":"20180302104232.GA11074@sigill.intra.peff.net","subject":"Re: Bug report: \"Use of uninitialized value $_ in print\"","fromName":"Sam Kuper","fromEmail":"sam.kuper@uclmail.net","sentAt":"2018-03-02T17:30:28Z","receivedAt":"2018-03-02T17:30:37Z","isPatch":false,"sender":{"key":"sam.kuper@uclmail.net","avatar":"https://gravatar.com/avatar/ae189c4c56e5cd31dc7371016f169e68aef81a3ea3318041a1ca4be46f10435b?d=mp&s=160"},"body":"On 02/03/2018, Jeff King wrote:\n> Unfortunately, I don't think there's an easy way to do what you want\n> (show word-diffs but apply the full diff).\n\nOh :(\n\nThat would be a *very* useful feature to have, especially where\nmultiple small (e.g. single character or single word) changes are\nsprinkled throughout a large file.\n\nShould I start a separate thread for this as a feature request?\n"},{"id":"340827","messageId":"xmqqefl2cqsd.fsf@gitster-ct.c.googlers.com","threadId":"47955","inReplyTo":"20180302104232.GA11074@sigill.intra.peff.net","subject":"Re: Bug report: \"Use of uninitialized value $_ in print\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-02T17:48:18Z","receivedAt":"2018-03-02T17:48:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Unfortunately, I don't think there's an easy way to do what you want\n> (show word-diffs but apply the full diff).\n\nThe current \"word-diff\" discards the information on where the lines\nend, and it is pretty much hopeless/useless in the context of \"add\n-p\". It would be a good addition to revamp it so that it keeps the\noriginal lines in pre/post images but only colors/highlights the\nbyte ranges in such a way that (1) on a pre-image line, paint only\nthe byte range that do not appear in the post-image, and (2) on a\npost-image line, paint only the byte range that do not appear in the\npre-image. \n\nE.g. Given these two\n\n diff --git a/1 b/2\n index aa49234b68..1cd9f6b5fd 100644\n --- a/1\n +++ b/2\n @@ -1,2 +1 @@\n -a b c e\n -f g i j\n +a b d f g h\n\nthe current word-diff would give (I cannot do colors here, so I'll\nupcase the colored letters):\n\n @@ -1,2 +1 @@\n a b C ED f g I JH\n\nas the output, but if we produced\n\n @@ -1,2 +1 @@\n -a b C E\n -f g I J\n +a b D f g H\n\ninstead, then colored \"add -p\" would become easier to see.\n\nAnd that would be useful outside the context of \"add -p\", which is a\nhuge plus.\n\n"},{"id":"340835","messageId":"CAD-Jur+nRsDnV2YrUwC9pt2aLCYHpEunXFKb22KdZ0sKF-Ga+A@mail.gmail.com","threadId":"47955","inReplyTo":"xmqqefl2cqsd.fsf@gitster-ct.c.googlers.com","subject":"Re: Bug report: \"Use of uninitialized value $_ in print\"","fromName":"Sam Kuper","fromEmail":"sam.kuper@uclmail.net","sentAt":"2018-03-02T20:34:30Z","receivedAt":"2018-03-02T20:34:39Z","isPatch":false,"sender":{"key":"sam.kuper@uclmail.net","avatar":"https://gravatar.com/avatar/ae189c4c56e5cd31dc7371016f169e68aef81a3ea3318041a1ca4be46f10435b?d=mp&s=160"},"body":"On 02/03/2018, Junio C Hamano wrote:\n> Jeff King writes:\n>> Unfortunately, I don't think there's an easy way to do what you want\n>> (show word-diffs but apply the full diff).\n>\n> The current \"word-diff\" discards the information on where the lines\n> end, and it is pretty much hopeless/useless in the context of \"add\n> -p\".\n\nThis is unfortunate.\n\nI am familiar with the model-view-controller (\"MVC\") paradigm used in\nsome software packages. I had hoped that Git followed this paradigm\nsomewhat, and cleanly separated the underlying model of the diff (i.e.\na representation that would be generated in all cases where a diff is\nneeded), and the view of the diff (i.e. the visual representation,\ne.g. word-diff, line diff, colored, non-colored, etc, as requested by\nthe user).\n\nSuch separation of concerns strikes me as being the best approach\nhere. It could be a lot of work, but I would be grateful if the Git\ndevelopers/maintainers could work towards it as an end goal for this\naspect of Git's architecture.\n\nUnfortunately, I am not very familiar with the Git codebase, nor\nwell-versed in its primary languages, so I can't be of much help here\n:(\n\n> It would be a good addition to revamp it so that it keeps the\n> original lines in pre/post images but only colors/highlights the\n> byte ranges in such a way that (1) on a pre-image line, paint only\n> the byte range that do not appear in the post-image, and (2) on a\n> post-image line, paint only the byte range that do not appear in the\n> pre-image.\n>\n> E.g. Given these two\n>\n> diff --git a/1 b/2\n> index aa49234b68..1cd9f6b5fd 100644\n> --- a/1\n> +++ b/2\n> @@ -1,2 +1 @@\n> -a b c e\n> -f g i j\n> +a b d f g h\n>\n> the current word-diff would give (I cannot do colors here, so I'll\n> upcase the colored letters):\n>\n> @@ -1,2 +1 @@\n> a b C ED f g I JH\n>\n> as the output, but if we produced\n>\n> @@ -1,2 +1 @@\n> -a b C E\n> -f g I J\n> +a b D f g H\n>\n> instead, then colored \"add -p\" would become easier to see.\n>\n> And that would be useful outside the context of \"add -p\", which is a\n> huge plus.\n\nThis would be much better than the current situation, where the visual\nrepresentation gives misleading feedback about the underlying diff,\nand where the error I reported crops up.\n\nHowever, in my opinion your proposed solution would still be not as\ngood as separating the two concerns, as described earlier in this\nemail, on two fronts:\n\n1. It would yield, IIUC, less flexibility to create new kinds of view\nbased on a consistent, standardised underlying model.\n\n2. It is harder to read, for some types of input (e.g. prose) than the\nview generated by the existing word-diff algorithm. Your approach, as\noutlined in your example above, requires the reader to visually jump\n(saccade) between two lines that are separated by an intervening line,\nin order to see what has changed. The existing word-diff is much\neasier to use: it puts the changes right next to each other, avoiding\nline-skipping and allowing horizontal saccades (which are much more\nfamiliar to people used to languages written in left-to-right or\nright-to-left scripts).\n\nI don't want to sound negative. As I say, I think your solution is a\nbig improvement on what currently exists. But I would see it as an\nintermediate solution - a stopgap - rather than an endpoint of\ndevelopment.\n\nIn other words, if your solution would be much quicker to implement\nthan the one I proposed, then please go ahead with yours first, and\nplease add mine to the longer-term to-do list :)\n\nThank you again for helping to make Git such a great tool, and for\nworking tirelessly to improve it further!\n\nP.S. There is a related StackOverflow question here:\nhttps://stackoverflow.com/questions/49058817/git-add-patch-with-word-diff\n"},{"id":"340839","messageId":"xmqqd10mb0v7.fsf@gitster-ct.c.googlers.com","threadId":"47955","inReplyTo":"CAD-Jur+nRsDnV2YrUwC9pt2aLCYHpEunXFKb22KdZ0sKF-Ga+A@mail.gmail.com","subject":"Re: Bug report: \"Use of uninitialized value $_ in print\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-02T21:53:32Z","receivedAt":"2018-03-02T21:53:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sam Kuper writes:\n\n> 1. It would yield, IIUC, less flexibility to create new kinds of view\n> based on a consistent, standardised underlying model.\n>\n> 2. It is harder to read, for some types of input (e.g. prose) than the\n> view generated by the existing word-diff algorithm.\n\nThe loss of line-end by the lossy \"word-diff\" output does not matter\nif you never split hunks, but to be able to split a hunk at an\nin-between context line (which you can already do) and new features\nlike per-line selection that are emerging, keeping 1:1 line\ncorrespondence between what is shown and what is applied is a must.\n\nUnless you are volunteering to design (notice that I am not saying\n\"implement\") both diff generation/coloration side _and_ patch\napplication side, that is. In which case, you may be able to come\nup with a magic ;-)\n\n"},{"id":"340871","messageId":"20180303055715.GH27689@sigill.intra.peff.net","threadId":"47955","inReplyTo":"xmqqo9k6csan.fsf@gitster-ct.c.googlers.com","subject":"Re: Bug report: \"Use of uninitialized value $_ in print\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-03T05:57:15Z","receivedAt":"2018-03-03T05:57:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 02, 2018 at 09:15:44AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > That's probably a reasonable sanity check, but I think we need to abort\n> > and not just have a too-small DISPLAY array. Because later code like the\n> > hunk-splitting is going to assume that there's a 1:1 line\n> > correspondence. We definitely don't want to end up in a situation where\n> > we show one thing but apply another.\n> \n> Yes, agreed completely.\n\nLet's add this sanity check while we're thinking about it. Here's a\nseries.\n\n [1/2]: t3701: add a test for interactive.diffFilter\n [2/2]: add--interactive: detect bogus diffFilter output\n\n git-add--interactive.perl | 8 ++++++++\n t/t3701-add-interactive.sh | 20 ++++++++++++++++++++\n 2 files changed, 28 insertions(+)\n\n-Peff\n"},{"id":"340872","messageId":"20180303055809.GA15952@sigill.intra.peff.net","threadId":"47955","inReplyTo":"20180303055715.GH27689@sigill.intra.peff.net","subject":"[PATCH 1/2] t3701: add a test for interactive.diffFilter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-03T05:58:10Z","receivedAt":"2018-03-03T05:58:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This feature was added in 01143847db (add--interactive:\nallow custom diff highlighting programs, 2016-02-27) but\nnever tested. Let's add a basic test.\n\nNote that we only apply the filter when color is enabled,\nso we have to use test_terminal. This is an open limitation\nexplicitly mentioned in the original commit. So take this\ncommit as testing the status quo, and not making a statement\non whether we'd want to enhance that in the future.\n\nSigned-off-by: Jeff King \n---\n t/t3701-add-interactive.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 058698df6a..64fe56c3d5 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -392,6 +392,18 @@ test_expect_success TTY 'diffs can be colorized' '\n \tgrep \"$(printf \"\\\\033\")\" output\n '\n \n+test_expect_success TTY 'diffFilter filters diff' '\n+\tgit reset --hard &&\n+\n+\techo content >test &&\n+\ttest_config interactive.diffFilter \"sed s/^/foo:/\" &&\n+\tprintf y | test_terminal git add -p >output 2>&1 &&\n+\n+\t# avoid depending on the exact coloring or content of the prompts,\n+\t# and just make sure we saw our diff prefixed\n+\tgrep foo:.*content output\n+'\n+\n test_expect_success 'patch-mode via -i prompts for files' '\n \tgit reset --hard &&\n \n-- \n2.16.2.708.g0b2ed7f536\n\n"},{"id":"340873","messageId":"20180303055849.GB15952@sigill.intra.peff.net","threadId":"47955","inReplyTo":"20180303055715.GH27689@sigill.intra.peff.net","subject":"[PATCH 2/2] add--interactive: detect bogus diffFilter output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-03T05:58:49Z","receivedAt":"2018-03-03T05:58:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's important that the diff-filter only filter the\nindividual lines, and that there remain a one-to-one mapping\nbetween the input and output lines. Otherwise, things like\nhunk-splitting will behave quite unexpectedly (e.g., you\nthink you are splitting at one point, but it has a different\neffect in the text patch we apply).\n\nWe can't detect all problematic cases, but we can at least\ncatch the obvious case where we don't even have the correct\nnumber of lines.\n\nSigned-off-by: Jeff King \n---\n git-add--interactive.perl | 8 ++++++++\n t/t3701-add-interactive.sh | 8 ++++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 964c3a7542..ff02008abe 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -705,6 +705,14 @@ sub parse_diff {\n \t}\n \tmy (@hunk) = { TEXT => [], DISPLAY => [], TYPE => 'header' };\n \n+\tif (@colored && @colored != @diff) {\n+\t\tprint STDERR\n+\t\t \"fatal: mismatched output from interactive.diffFilter\\n\",\n+\t\t \"hint: Your filter must maintain a one-to-one correspondence\\n\",\n+\t\t \"hint: between its input and output lines.\\n\";\n+\t\texit 1;\n+\t}\n+\n \tfor (my $i = 0; $i < @diff; $i++) {\n \t\tif ($diff[$i] =~ /^@@ /) {\n \t\t\tpush @hunk, { TEXT => [], DISPLAY => [],\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 64fe56c3d5..9bb17f91b2 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -404,6 +404,14 @@ test_expect_success TTY 'diffFilter filters diff' '\n \tgrep foo:.*content output\n '\n \n+test_expect_success TTY 'detect bogus diffFilter output' '\n+\tgit reset --hard &&\n+\n+\techo content >test &&\n+\ttest_config interactive.diffFilter \"echo too-short\" &&\n+\tprintf y | test_must_fail test_terminal git add -p\n+'\n+\n test_expect_success 'patch-mode via -i prompts for files' '\n \tgit reset --hard &&\n \n-- \n2.16.2.708.g0b2ed7f536\n"},{"id":"340874","messageId":"20180303060215.GI27689@sigill.intra.peff.net","threadId":"47955","inReplyTo":"CAD-JurJLDSzf7_A0ZGWPYGyRiSnHWSpUrVEd9_+N1J7BamMM=Q@mail.gmail.com","subject":"Re: Bug report: \"Use of uninitialized value $_ in print\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-03T06:02:16Z","receivedAt":"2018-03-03T06:02:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 02, 2018 at 05:30:28PM +0000, Sam Kuper wrote:\n\n> On 02/03/2018, Jeff King wrote:\n> > Unfortunately, I don't think there's an easy way to do what you want\n> > (show word-diffs but apply the full diff).\n> \n> Oh :(\n> \n> That would be a *very* useful feature to have, especially where\n> multiple small (e.g. single character or single word) changes are\n> sprinkled throughout a large file.\n\nYou might want to try diff-highlight from Git's contrib/ directory. It\nwas the reason I added interactive.diffFilter in the first place. E.g.:\n\n cd contrib/diff-highlight\n make\n cp diff-highlight /to/your/$PATH\n git config --global interactive.diffFilter diff-highlight\n\nIt's not quite the same as a word-diff, because it doesn't find matches\nacross word boundaries. So if you re-wrap a paragraph, for example, it\nwon't be much help. But I do find it useful (and also as a general\nfilter for log and diff output).\n\n> Should I start a separate thread for this as a feature request?\n\nI think this thread can serve.\n\n-Peff\n"},{"id":"340875","messageId":"20180303061802.GJ27689@sigill.intra.peff.net","threadId":"47955","inReplyTo":"xmqqd10mb0v7.fsf@gitster-ct.c.googlers.com","subject":"Re: Bug report: \"Use of uninitialized value $_ in print\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-03T06:18:02Z","receivedAt":"2018-03-03T06:19:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 02, 2018 at 01:53:32PM -0800, Junio C Hamano wrote:\n\n> Sam Kuper writes:\n> \n> > 1. It would yield, IIUC, less flexibility to create new kinds of view\n> > based on a consistent, standardised underlying model.\n> >\n> > 2. It is harder to read, for some types of input (e.g. prose) than the\n> > view generated by the existing word-diff algorithm.\n> \n> The loss of line-end by the lossy \"word-diff\" output does not matter\n> if you never split hunks, but to be able to split a hunk at an\n> in-between context line (which you can already do) and new features\n> like per-line selection that are emerging, keeping 1:1 line\n> correspondence between what is shown and what is applied is a must.\n> \n> Unless you are volunteering to design (notice that I am not saying\n> \"implement\") both diff generation/coloration side _and_ patch\n> application side, that is. In which case, you may be able to come\n> up with a magic ;-)\n\nIIRC, we do the word-diff by first finding the individual hunks with a\nnormal line diff, and then doing a word diff inside those hunks. So one\neasy option would be to add a --color-words option that gets passed\nalong to the underlying display diff and just disables hunk splitting. I\nthink we'd also need to start parsing the DISPLAY hunks more carefully\nto make sure we re-sync at hunk boundaries.\n\n-Peff\n"},{"id":"341063","messageId":"xmqqsh9e8css.fsf@gitster-ct.c.googlers.com","threadId":"47955","inReplyTo":"20180303055849.GB15952@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] add--interactive: detect bogus diffFilter output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-05T20:53:07Z","receivedAt":"2018-03-05T20:53:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It's important that the diff-filter only filter the\n> individual lines, and that there remain a one-to-one mapping\n> between the input and output lines. Otherwise, things like\n> hunk-splitting will behave quite unexpectedly (e.g., you\n> think you are splitting at one point, but it has a different\n> effect in the text patch we apply).\n>\n> We can't detect all problematic cases, but we can at least\n> catch the obvious case where we don't even have the correct\n> number of lines.\n\nWill queue. We could probably also make sure that each of the\ncorresponding line pair begins with the same '-/ /+' letter, but we\nneed to draw a line and stop somewhere, and I think the number of\nlines is probably a good enough place.\n\n>\n> Signed-off-by: Jeff King \n> ---\n> git-add--interactive.perl | 8 ++++++++\n> t/t3701-add-interactive.sh | 8 ++++++++\n> 2 files changed, 16 insertions(+)\n>\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index 964c3a7542..ff02008abe 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -705,6 +705,14 @@ sub parse_diff {\n> \t}\n> \tmy (@hunk) = { TEXT => [], DISPLAY => [], TYPE => 'header' };\n> \n> +\tif (@colored && @colored != @diff) {\n> +\t\tprint STDERR\n> +\t\t \"fatal: mismatched output from interactive.diffFilter\\n\",\n> +\t\t \"hint: Your filter must maintain a one-to-one correspondence\\n\",\n> +\t\t \"hint: between its input and output lines.\\n\";\n> +\t\texit 1;\n> +\t}\n> +\n> \tfor (my $i = 0; $i < @diff; $i++) {\n> \t\tif ($diff[$i] =~ /^@@ /) {\n> \t\t\tpush @hunk, { TEXT => [], DISPLAY => [],\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 64fe56c3d5..9bb17f91b2 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -404,6 +404,14 @@ test_expect_success TTY 'diffFilter filters diff' '\n> \tgrep foo:.*content output\n> '\n> \n> +test_expect_success TTY 'detect bogus diffFilter output' '\n> +\tgit reset --hard &&\n> +\n> +\techo content >test &&\n> +\ttest_config interactive.diffFilter \"echo too-short\" &&\n> +\tprintf y | test_must_fail test_terminal git add -p\n> +'\n> +\n> test_expect_success 'patch-mode via -i prompts for files' '\n> \tgit reset --hard &&\n"},{"id":"341064","messageId":"20180305205626.GD5953@sigill.intra.peff.net","threadId":"47955","inReplyTo":"xmqqsh9e8css.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] add--interactive: detect bogus diffFilter output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-05T20:56:27Z","receivedAt":"2018-03-05T20:56:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 05, 2018 at 12:53:07PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > It's important that the diff-filter only filter the\n> > individual lines, and that there remain a one-to-one mapping\n> > between the input and output lines. Otherwise, things like\n> > hunk-splitting will behave quite unexpectedly (e.g., you\n> > think you are splitting at one point, but it has a different\n> > effect in the text patch we apply).\n> >\n> > We can't detect all problematic cases, but we can at least\n> > catch the obvious case where we don't even have the correct\n> > number of lines.\n> \n> Will queue. We could probably also make sure that each of the\n> corresponding line pair begins with the same '-/ /+' letter, but we\n> need to draw a line and stop somewhere, and I think the number of\n> lines is probably a good enough place.\n\nI think that would break things like diff-so-fancy, which actually\nremoves the -/+ in favor of pure coloring (not something I care for\nmyself, but it seems a legitimate use case). So it's probably best not\nto look at the content, not just from a \"we can only go so far\"\nperspective but also because we actively don't know what the filter's\noutput is supposed to look like.\n\n-Peff\n"},{"id":"341067","messageId":"878tb6nr6u.fsf@evledraar.gmail.com","threadId":"47955","inReplyTo":"20180303055849.GB15952@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] add--interactive: detect bogus diffFilter output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-03-05T21:33:13Z","receivedAt":"2018-03-05T21:33:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Mar 03 2018, Jeff King jotted:\n\n> +\tif (@colored && @colored != @diff) {\n\nnit: should just be:\n\n if (@colored != @diff) {\n\nIt's not possible for @arrays in scalar context to be undefined.\n"},{"id":"341071","messageId":"20180305215405.GG28067@aiede.svl.corp.google.com","threadId":"47955","inReplyTo":"20180303055715.GH27689@sigill.intra.peff.net","subject":"Re: Bug report: \"Use of uninitialized value $_ in print\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-03-05T21:54:05Z","receivedAt":"2018-03-05T21:54:13Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Fri, Mar 02, 2018 at 09:15:44AM -0800, Junio C Hamano wrote:\n>> Jeff King writes:\n\n>>> That's probably a reasonable sanity check, but I think we need to abort\n>>> and not just have a too-small DISPLAY array. Because later code like the\n>>> hunk-splitting is going to assume that there's a 1:1 line\n>>> correspondence. We definitely don't want to end up in a situation where\n>>> we show one thing but apply another.\n>>\n>> Yes, agreed completely.\n>\n> Let's add this sanity check while we're thinking about it. Here's a\n> series.\n>\n> [1/2]: t3701: add a test for interactive.diffFilter\n> [2/2]: add--interactive: detect bogus diffFilter output\n>\n> git-add--interactive.perl | 8 ++++++++\n> t/t3701-add-interactive.sh | 20 ++++++++++++++++++++\n> 2 files changed, 28 insertions(+)\n\nWith or without the tweak Ævar Arnfjörð Bjarmason suggested,\nReviewed-by: Jonathan Nieder \n\nThanks. It's probably also worth adding Sam's reported-by to patch 2/2:\nReported-by: Sam Kuper \n"},{"id":"341073","messageId":"xmqqbmg2898w.fsf@gitster-ct.c.googlers.com","threadId":"47955","inReplyTo":"878tb6nr6u.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] add--interactive: detect bogus diffFilter output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-05T22:09:51Z","receivedAt":"2018-03-05T22:09:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Sat, Mar 03 2018, Jeff King jotted:\n>\n>> +\tif (@colored && @colored != @diff) {\n>\n> nit: should just be:\n>\n> if (@colored != @diff) {\n>\n> It's not possible for @arrays in scalar context to be undefined.\n\nIt is true that @array can not be undef, but your rewrite I think is\nwrong.\n\nThe first \"do the comparison only @colored is true\" is not about\ndefinedness. It is \"@colored can be an empty array when the user is\nnot using separate 'show these colored lines to the end user, feed\nthese noncolored lines to git-apply command' feature, so @colored==0\nand @diff > 0 is perfectly fine\".\n"},{"id":"341075","messageId":"CACBZZX5H2aiswHNkWVrva20vaEQqN0GGBTidM1htksY6Q3at2w@mail.gmail.com","threadId":"47955","inReplyTo":"xmqqbmg2898w.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] add--interactive: detect bogus diffFilter output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-03-05T22:15:30Z","receivedAt":"2018-03-05T22:15:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Mar 5, 2018 at 11:09 PM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> On Sat, Mar 03 2018, Jeff King jotted:\n>>\n>>> + if (@colored && @colored != @diff) {\n>>\n>> nit: should just be:\n>>\n>> if (@colored != @diff) {\n>>\n>> It's not possible for @arrays in scalar context to be undefined.\n>\n> It is true that @array can not be undef, but your rewrite I think is\n> wrong.\n>\n> The first \"do the comparison only @colored is true\" is not about\n> definedness. It is \"@colored can be an empty array when the user is\n> not using separate 'show these colored lines to the end user, feed\n> these noncolored lines to git-apply command' feature, so @colored==0\n> and @diff > 0 is perfectly fine\".\n\nYes, sorry for the noise. I misread the intent of the code.\n"}]}