{"thread":{"id":"52721","subject":"Patch text in git-add patch mode lacks whitespace highlighting","startedAt":"2020-01-30T21:54:11Z","lastAt":"2020-02-04T19:57:12Z","messageCount":14,"participants":["Mike McGranahan","Jeff King","Johannes Schindelin","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"390847","messageId":"CAK7jxYgJNvCp=m6rH31HNzN9Mqgaav7_YPvUMZmRb7mdYDZ_1g@mail.gmail.com","threadId":"52721","inReplyTo":null,"subject":"Patch text in git-add patch mode lacks whitespace highlighting","fromName":"Mike McGranahan","fromEmail":"mike@mcgwiz.com","sentAt":"2020-01-30T21:53:52Z","receivedAt":"2020-01-30T21:54:11Z","isPatch":false,"sender":{"key":"mike@mcgwiz.com","avatar":"https://gravatar.com/avatar/4aa9850ebf30bad46f0262c695989fcd8d12f87723ee2b563e71f997f1cfecfd?d=mp&s=160"},"body":"Hello,\n\nI'm using version 2.25.0.windows.1. If I set config \"wsErrorHighlight\"\nto \"old\", and run `git diff -p`, the resulting patch text highlights\nwhitespace differences in the old text.\n\nIf I then run git-add in interactive patch mode, I expect the diff to\ninclude the whitespace highlights. But actually, it does not.\n\nIs this a bug? Thanks for your help.\n\n-Mike\n"},{"id":"390889","messageId":"20200131091917.GC2857810@coredump.intra.peff.net","threadId":"52721","inReplyTo":"CAK7jxYgJNvCp=m6rH31HNzN9Mqgaav7_YPvUMZmRb7mdYDZ_1g@mail.gmail.com","subject":"Re: Patch text in git-add patch mode lacks whitespace highlighting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-31T09:19:17Z","receivedAt":"2020-01-31T09:19:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 30, 2020 at 01:53:52PM -0800, Mike McGranahan wrote:\n\n> I'm using version 2.25.0.windows.1. If I set config \"wsErrorHighlight\"\n> to \"old\", and run `git diff -p`, the resulting patch text highlights\n> whitespace differences in the old text.\n> \n> If I then run git-add in interactive patch mode, I expect the diff to\n> include the whitespace highlights. But actually, it does not.\n> \n> Is this a bug? Thanks for your help.\n\nSort of. It's more of a feature that has not yet been implemented. ;)\n\nLike the rest of the color config, diff.wsErrorHighlight is not enabled\nfor scriptable plumbing commands like git-diff-index, etc; it is only\nused for the user-facing porcelain commands like git-diff.\n\nSo scripts like git-add--interactive, which use the plumbing under the\nhood, are responsible for deciding which config options should be\nrespected and passing the correct command-line options on to the\nplumbing. We do that for color.diff, diff.algorithm, and a few others.\nBut nobody has yet taught it that diff.wsErrorHighlight is safe to pass\n(for the user-facing \"color\" version of the diff).\n\nThe solution for the existing perl script would be something like this:\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex c4b75bcb7f..fac1d1e63e 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -46,6 +46,7 @@\n my $normal_color = $repo->get_color(\"\", \"reset\");\n \n my $diff_algorithm = $repo->config('diff.algorithm');\n+my $diff_error_highlight = $repo->config('diff.wsErrorHighlight');\n my $diff_filter = $repo->config('interactive.difffilter');\n \n my $use_readkey = 0;\n@@ -727,7 +728,11 @@ sub parse_diff {\n \tmy @diff = run_cmd_pipe(\"git\", @diff_cmd, \"--\", $path);\n \tmy @colored = ();\n \tif ($diff_use_color) {\n-\t\tmy @display_cmd = (\"git\", @diff_cmd, qw(--color --), $path);\n+\t\tmy @display_cmd = (\"git\", @diff_cmd, qw(--color),\n+\t\t\t           $diff_error_highlight ?\n+\t\t\t\t     \"--ws-error-highlight=$diff_error_highlight\" :\n+\t\t\t\t     (),\n+\t\t\t\t   qw(--), $path);\n \t\tif (defined $diff_filter) {\n \t\t\t# quotemeta is overkill, but sufficient for shell-quoting\n \t\t\tmy $diff = join(' ', map { quotemeta } @display_cmd);\n\nbut this is all being rewritten in C. I'm not sure how the multiple\ndiffs are being handled there.\n\nAll that said, I kind of wonder if diff.wsErrorHighlight ought to be\nrespected by even plumbing commands. After all, it does nothing unless\ncolor is enabled anyway, so I don't think it runs the risk of confusing\na user of the plumbing. It's no different than color.diff.*, which we\nrespect even in the plumbing commands (if the caller has manually asked\nfor color, of course).\n\n-Peff\n"},{"id":"390892","messageId":"20200131095749.GB2916051@coredump.intra.peff.net","threadId":"52721","inReplyTo":"20200131091917.GC2857810@coredump.intra.peff.net","subject":"[PATCH] diff: move diff.wsErrorHighlight to \"basic\" config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-31T09:57:49Z","receivedAt":"2020-01-31T09:57:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 31, 2020 at 04:19:17AM -0500, Jeff King wrote:\n\n> All that said, I kind of wonder if diff.wsErrorHighlight ought to be\n> respected by even plumbing commands. After all, it does nothing unless\n> color is enabled anyway, so I don't think it runs the risk of confusing\n> a user of the plumbing. It's no different than color.diff.*, which we\n> respect even in the plumbing commands (if the caller has manually asked\n> for color, of course).\n\nThe more I pondered this, the more it seems like the right thing. So\nhere's a patch to do it.\n\nI also suspect that some of the color-moved config would benefit from\nthe same treatment, but I haven't yet convinced myself. Unlike this\noption, which impacts a single line, the move detection covers multiple\nhunks, which you'd be picking out independently. Would that be weird, or\nwould it make sense as it's just annotating the existing diff?\n\n-- >8 --\nSubject: [PATCH] diff: move diff.wsErrorHighlight to \"basic\" config\n\nWe parse diff.wsErrorHighlight in git_diff_ui_config(), meaning that it\ndoesn't take effect for plumbing commands, only for porcelains like\ngit-diff itself. This is mildly annoying as it means scripts like\nadd--interactive, which produce a user-visible diff with color, don't\nrespect the option.\n\nWe could teach that script to parse the config and pass it along as\n--ws-error-highlight to the diff plumbing. But there's a simpler\nsolution.\n\nIt should be reasonably safe for plumbing to respect this option, as it\nonly kicks in when color is otherwise enabled. And anybody parsing\ncolorized output must already deal with the fact that color.diff.* may\nchange the exact output they see; those options have been part of\ngit_diff_basic_config() since its inception in 9a1805a872 (add a \"basic\"\ndiff config callback, 2008-01-04).\n\nSo we can just move it to the \"basic\" config, which fixes\nadd--interactive, along with any other script in the same boat, with a\nvery low risk of hurting any plumbing users.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c                     | 16 ++++++++--------\n t/t3701-add-interactive.sh | 13 +++++++++++++\n 2 files changed, 21 insertions(+), 8 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 8e2914c031..5e4471f15f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -414,14 +414,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\tif (!strcmp(var, \"diff.wserrorhighlight\")) {\n-\t\tint val = parse_ws_error_highlight(value);\n-\t\tif (val < 0)\n-\t\t\treturn -1;\n-\t\tws_error_highlight_default = val;\n-\t\treturn 0;\n-\t}\n-\n \tif (git_color_config(var, value, cb) < 0)\n \t\treturn -1;\n \n@@ -450,6 +442,14 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \t\treturn color_parse(value, diff_colors[slot]);\n \t}\n \n+\tif (!strcmp(var, \"diff.wserrorhighlight\")) {\n+\t\tint val = parse_ws_error_highlight(value);\n+\t\tif (val < 0)\n+\t\t\treturn -1;\n+\t\tws_error_highlight_default = val;\n+\t\treturn 0;\n+\t}\n+\n \t/* like GNU diff's --suppress-blank-empty option  */\n \tif (!strcmp(var, \"diff.suppressblankempty\") ||\n \t\t\t/* for backwards compatibility */\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 2182b1c030..a28182c526 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -553,6 +553,19 @@ test_expect_success 'diffs can be colorized' '\n \tgrep \"$(printf \"\\\\033\")\" output\n '\n \n+test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '\n+\tgit reset --hard &&\n+\n+\techo \"old \" >test &&\n+\tgit add test &&\n+\techo \"new \" >test &&\n+\n+\tprintf y >y &&\n+\tforce_color git -c diff.wsErrorHighlight=all add -p >output.raw 2>&1 <y &&\n+\ttest_decode_color <output.raw >output &&\n+\tgrep \"old<\" output\n+'\n+\n test_expect_success 'diffFilter filters diff' '\n \tgit reset --hard &&\n \n-- \n2.25.0.538.g1d5d7023b0\n\n"},{"id":"390900","messageId":"nycvar.QRO.7.76.6.2001311304140.46@tvgsbejvaqbjf.bet","threadId":"52721","inReplyTo":"CAK7jxYgJNvCp=m6rH31HNzN9Mqgaav7_YPvUMZmRb7mdYDZ_1g@mail.gmail.com","subject":"Re: Patch text in git-add patch mode lacks whitespace highlighting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-31T12:37:44Z","receivedAt":"2020-01-31T12:38:36Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Mike,\n\nOn Thu, 30 Jan 2020, Mike McGranahan wrote:\n\n> I'm using version 2.25.0.windows.1. If I set config \"wsErrorHighlight\"\n> to \"old\", and run `git diff -p`, the resulting patch text highlights\n> whitespace differences in the old text.\n>\n> If I then run git-add in interactive patch mode, I expect the diff to\n> include the whitespace highlights. But actually, it does not.\n>\n> Is this a bug? Thanks for your help.\n\nWell, let's first try to get a preciser report, to make sure that we're on\nthe same page. These are the commands I ran:\n\n\tmkdir add-p-ws-error\n\tcd add-p-ws-error\n\tgit init\n\tgit config diff.wsErrorHighlight old\n\techo \"hello \" >README\n\tgit add README\n\techo hello >README\n\tgit -c add.interactive.usebuiltin=true add -p\n\tgit -c add.interactive.usebuiltin=false add -p\n\nIn both `git add -p` invocations, I see the diff colored, and neither of\nthem shows the red square after the removed \"hello\" that `git diff shows.\n\nSo this is not a change of behavior in the conversion from a Perl script\nto a built-in.\n\nInvestigating further, running with `GIT_TRACE=1` reveals that `add -p`\ninternally calls `git diff-files --color -p --`. If you run that command\nmanually, you will see that it indeed seems to ignore the\n`diff.wsErrorHighlight` setting.\n\nThe actual code running `git diff-files` is here:\nhttps://github.com/git-for-windows/git/blob/v2.25.0.windows.1/add-patch.c#L373-L436\n\nI think it is correct to call the low-level `diff-files` here rather than\nthe high-level `diff` that is intended for human consumption.\n\nLooking at the code in\nhttps://github.com/git/git/blob/v2.25.0/builtin/diff-files.c#L28, I\nbelieve that we found the reason why `git diff-files --color` ignores the\n`wsErrorHighlight` setting.\n\nNow, the documentation at\nhttps://git-scm.com/docs/git-config#Documentation/git-config.txt-diffwsErrorHighlight\nand even at\nhttps://git-scm.com/docs/git-diff-files#Documentation/git-diff-files.txt---ws-error-highlightltkindgt\nmake it appear as if the code is not following the original intention.\n\nIf my reading is correct, and we want `git diff-files --color` to respect\nthe `diff.wsErrorHighlight` setting, then this patch fixes that:\n\n-- snip --\ndiff --git a/diff.c b/diff.c\nindex 8e2914c0312..63afb8638c8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -414,14 +414,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n\n-\tif (!strcmp(var, \"diff.wserrorhighlight\")) {\n-\t\tint val = parse_ws_error_highlight(value);\n-\t\tif (val < 0)\n-\t\t\treturn -1;\n-\t\tws_error_highlight_default = val;\n-\t\treturn 0;\n-\t}\n-\n \tif (git_color_config(var, value, cb) < 0)\n \t\treturn -1;\n\n@@ -469,6 +461,14 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n\n+\tif (!strcmp(var, \"diff.wserrorhighlight\")) {\n+\t\tint val = parse_ws_error_highlight(value);\n+\t\tif (val < 0)\n+\t\t\treturn -1;\n+\t\tws_error_highlight_default = val;\n+\t\treturn 0;\n+\t}\n+\n \tif (git_diff_heuristic_config(var, value, cb) < 0)\n \t\treturn -1;\n\n-- snap --\n\nThe bigger question is whether other core developers agree with this? And\nwhat other `diff.*` settings should be respected by `git diff-files` (and\nof course, `git diff-index`)?\n\nCiao,\nJohannes\n"},{"id":"390979","messageId":"CAK7jxYjLoXZhRzUWERmJg9ADNhvJt5SLLymPVktiWcT4RC6VpA@mail.gmail.com","threadId":"52721","inReplyTo":"20200131091917.GC2857810@coredump.intra.peff.net","subject":"Re: Patch text in git-add patch mode lacks whitespace highlighting","fromName":"Mike McGranahan","fromEmail":"mike@mcgwiz.com","sentAt":"2020-02-01T00:48:10Z","receivedAt":"2020-02-01T00:48:26Z","isPatch":false,"sender":{"key":"mike@mcgwiz.com","avatar":"https://gravatar.com/avatar/4aa9850ebf30bad46f0262c695989fcd8d12f87723ee2b563e71f997f1cfecfd?d=mp&s=160"},"body":"I can confirm this fixes the unexpected behavior I reported. (Thanks!)\nI can't speak directly to the plumbing as I don't regularly use thos\ntools.\n\n-Mike\n\nOn Fri, Jan 31, 2020 at 1:19 AM Jeff King <peff@peff.net> wrote:\n>\n> On Thu, Jan 30, 2020 at 01:53:52PM -0800, Mike McGranahan wrote:\n>\n> > I'm using version 2.25.0.windows.1. If I set config \"wsErrorHighlight\"\n> > to \"old\", and run `git diff -p`, the resulting patch text highlights\n> > whitespace differences in the old text.\n> >\n> > If I then run git-add in interactive patch mode, I expect the diff to\n> > include the whitespace highlights. But actually, it does not.\n> >\n> > Is this a bug? Thanks for your help.\n>\n> Sort of. It's more of a feature that has not yet been implemented. ;)\n>\n> Like the rest of the color config, diff.wsErrorHighlight is not enabled\n> for scriptable plumbing commands like git-diff-index, etc; it is only\n> used for the user-facing porcelain commands like git-diff.\n>\n> So scripts like git-add--interactive, which use the plumbing under the\n> hood, are responsible for deciding which config options should be\n> respected and passing the correct command-line options on to the\n> plumbing. We do that for color.diff, diff.algorithm, and a few others.\n> But nobody has yet taught it that diff.wsErrorHighlight is safe to pass\n> (for the user-facing \"color\" version of the diff).\n>\n> The solution for the existing perl script would be something like this:\n>\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index c4b75bcb7f..fac1d1e63e 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -46,6 +46,7 @@\n>  my $normal_color = $repo->get_color(\"\", \"reset\");\n>\n>  my $diff_algorithm = $repo->config('diff.algorithm');\n> +my $diff_error_highlight = $repo->config('diff.wsErrorHighlight');\n>  my $diff_filter = $repo->config('interactive.difffilter');\n>\n>  my $use_readkey = 0;\n> @@ -727,7 +728,11 @@ sub parse_diff {\n>         my @diff = run_cmd_pipe(\"git\", @diff_cmd, \"--\", $path);\n>         my @colored = ();\n>         if ($diff_use_color) {\n> -               my @display_cmd = (\"git\", @diff_cmd, qw(--color --), $path);\n> +               my @display_cmd = (\"git\", @diff_cmd, qw(--color),\n> +                                  $diff_error_highlight ?\n> +                                    \"--ws-error-highlight=$diff_error_highlight\" :\n> +                                    (),\n> +                                  qw(--), $path);\n>                 if (defined $diff_filter) {\n>                         # quotemeta is overkill, but sufficient for shell-quoting\n>                         my $diff = join(' ', map { quotemeta } @display_cmd);\n>\n> but this is all being rewritten in C. I'm not sure how the multiple\n> diffs are being handled there.\n>\n> All that said, I kind of wonder if diff.wsErrorHighlight ought to be\n> respected by even plumbing commands. After all, it does nothing unless\n> color is enabled anyway, so I don't think it runs the risk of confusing\n> a user of the plumbing. It's no different than color.diff.*, which we\n> respect even in the plumbing commands (if the caller has manually asked\n> for color, of course).\n>\n> -Peff\n"},{"id":"390980","messageId":"CAK7jxYiBPjcLBpNKy5k1jf116EWr4HB9B-2rzUxiCG+m-Kyzeg@mail.gmail.com","threadId":"52721","inReplyTo":"nycvar.QRO.7.76.6.2001311304140.46@tvgsbejvaqbjf.bet","subject":"Re: Patch text in git-add patch mode lacks whitespace highlighting","fromName":"Mike McGranahan","fromEmail":"mike@mcgwiz.com","sentAt":"2020-02-01T00:49:57Z","receivedAt":"2020-02-01T00:50:16Z","isPatch":false,"sender":{"key":"mike@mcgwiz.com","avatar":"https://gravatar.com/avatar/4aa9850ebf30bad46f0262c695989fcd8d12f87723ee2b563e71f997f1cfecfd?d=mp&s=160"},"body":"On Fri, Jan 31, 2020 at 4:37 AM Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> Well, let's first try to get a preciser report, to make sure that we're on\n> the same page. These are the commands I ran:\n>\n>         mkdir add-p-ws-error\n>         cd add-p-ws-error\n>         git init\n>         git config diff.wsErrorHighlight old\n>         echo \"hello \" >README\n>         git add README\n>         echo hello >README\n>         git -c add.interactive.usebuiltin=true add -p\n>         git -c add.interactive.usebuiltin=false add -p\n\nThose commands do indeed reproduce the behavior I described.\n\n-Mike\n"},{"id":"390984","messageId":"20200201110203.GA1864964@coredump.intra.peff.net","threadId":"52721","inReplyTo":"nycvar.QRO.7.76.6.2001311304140.46@tvgsbejvaqbjf.bet","subject":"Re: Patch text in git-add patch mode lacks whitespace highlighting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-01T11:02:03Z","receivedAt":"2020-02-01T11:02:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 31, 2020 at 01:37:44PM +0100, Johannes Schindelin wrote:\n\n> If my reading is correct, and we want `git diff-files --color` to respect\n> the `diff.wsErrorHighlight` setting, then this patch fixes that:\n> [...]\n> The bigger question is whether other core developers agree with this? And\n> what other `diff.*` settings should be respected by `git diff-files` (and\n> of course, `git diff-index`)?\n\nI think you can take my posting of an identical patch elsewhere in the\nthread as a \"yes\". :)\n\n-Peff\n"},{"id":"390997","messageId":"nycvar.QRO.7.76.6.2002012205520.46@tvgsbejvaqbjf.bet","threadId":"52721","inReplyTo":"20200201110203.GA1864964@coredump.intra.peff.net","subject":"Re: Patch text in git-add patch mode lacks whitespace highlighting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-01T21:06:43Z","receivedAt":"2020-02-01T21:06:55Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Sat, 1 Feb 2020, Jeff King wrote:\n\n> On Fri, Jan 31, 2020 at 01:37:44PM +0100, Johannes Schindelin wrote:\n>\n> > If my reading is correct, and we want `git diff-files --color` to respect\n> > the `diff.wsErrorHighlight` setting, then this patch fixes that:\n> > [...]\n> > The bigger question is whether other core developers agree with this? And\n> > what other `diff.*` settings should be respected by `git diff-files` (and\n> > of course, `git diff-index`)?\n>\n> I think you can take my posting of an identical patch elsewhere in the\n> thread as a \"yes\". :)\n\nThank you ;-)\n\nThat answer only applies to the first question, though. The second\nquestion, whether other `diff.*` settings would like to enjoy the same\ntreatment, is still open for debate...\n\nThanks,\nDscho\n"},{"id":"391029","messageId":"20200203085456.GD2164@coredump.intra.peff.net","threadId":"52721","inReplyTo":"nycvar.QRO.7.76.6.2002012205520.46@tvgsbejvaqbjf.bet","subject":"Re: Patch text in git-add patch mode lacks whitespace highlighting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-03T08:54:56Z","receivedAt":"2020-02-03T08:54:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 01, 2020 at 10:06:43PM +0100, Johannes Schindelin wrote:\n\n> > > If my reading is correct, and we want `git diff-files --color` to respect\n> > > the `diff.wsErrorHighlight` setting, then this patch fixes that:\n> > > [...]\n> > > The bigger question is whether other core developers agree with this? And\n> > > what other `diff.*` settings should be respected by `git diff-files` (and\n> > > of course, `git diff-index`)?\n> >\n> > I think you can take my posting of an identical patch elsewhere in the\n> > thread as a \"yes\". :)\n> \n> Thank you ;-)\n> \n> That answer only applies to the first question, though. The second\n> question, whether other `diff.*` settings would like to enjoy the same\n> treatment, is still open for debate...\n\nI hoped you would read the other part of the thread where I mused on\nthat same question. :)\n\nThe short of my answer is that I think the color-moved stuff might be a\ncandidate, but it's sufficiently different that I think it should be\ndecided on as a separate patch.\n\n-Peff\n"},{"id":"391039","messageId":"nycvar.QRO.7.76.6.2002031320180.46@tvgsbejvaqbjf.bet","threadId":"52721","inReplyTo":"20200203085456.GD2164@coredump.intra.peff.net","subject":"Re: Patch text in git-add patch mode lacks whitespace highlighting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-03T12:37:48Z","receivedAt":"2020-02-03T12:37:54Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 3 Feb 2020, Jeff King wrote:\n\n> On Sat, Feb 01, 2020 at 10:06:43PM +0100, Johannes Schindelin wrote:\n>\n> > > > If my reading is correct, and we want `git diff-files --color` to respect\n> > > > the `diff.wsErrorHighlight` setting, then this patch fixes that:\n> > > > [...]\n> > > > The bigger question is whether other core developers agree with this? And\n> > > > what other `diff.*` settings should be respected by `git diff-files` (and\n> > > > of course, `git diff-index`)?\n> > >\n> > > I think you can take my posting of an identical patch elsewhere in the\n> > > thread as a \"yes\". :)\n> >\n> > Thank you ;-)\n> >\n> > That answer only applies to the first question, though. The second\n> > question, whether other `diff.*` settings would like to enjoy the same\n> > treatment, is still open for debate...\n>\n> I hoped you would read the other part of the thread where I mused on\n> that same question. :)\n\nSorry, I totally missed that. Will re-read.\n\n> The short of my answer is that I think the color-moved stuff might be a\n> candidate, but it's sufficiently different that I think it should be\n> decided on as a separate patch.\n\nI actually wonder whether we should do something completely different. The\nproblem with `git diff-files --color`, after all, is that `diff-files` was\nnever intended to produce user-facing output, so the `--color` is somewhat\nof a contradiction here. The fact that `diff-files` is a low-level (or\n\"plumbing\") command means that by nature, it wants to control a lot more\nwhat the user-provided config can change (typically, scripts calling\n`diff-files` expect a certain format, and it would not do at all to heed\n`diff.noPrefix`, for example.\n\nIn that respect, `git add -p` using `diff-files` is kind of wrong: we\n_want_ to show the result to the user, with no processing at all (execpt\nthe user-provided `diffFilter`).\n\nSo why not introduce a new option to `diff-files` and `diff-index` to ask\nit _specifically_ to heed diff UI config settings? I.e. a command-line\noption that makes it call\n\n        git_config(git_diff_ui_config, NULL);\n\ninstead of\n\n        git_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n\n?\n\nCiao,\nDscho\n"},{"id":"391043","messageId":"20200203145155.GA272077@coredump.intra.peff.net","threadId":"52721","inReplyTo":"nycvar.QRO.7.76.6.2002031320180.46@tvgsbejvaqbjf.bet","subject":"Re: Patch text in git-add patch mode lacks whitespace highlighting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-03T14:51:55Z","receivedAt":"2020-02-03T14:51:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 03, 2020 at 01:37:48PM +0100, Johannes Schindelin wrote:\n\n> > The short of my answer is that I think the color-moved stuff might be a\n> > candidate, but it's sufficiently different that I think it should be\n> > decided on as a separate patch.\n> \n> I actually wonder whether we should do something completely different. The\n> problem with `git diff-files --color`, after all, is that `diff-files` was\n> never intended to produce user-facing output, so the `--color` is somewhat\n> of a contradiction here. The fact that `diff-files` is a low-level (or\n> \"plumbing\") command means that by nature, it wants to control a lot more\n> what the user-provided config can change (typically, scripts calling\n> `diff-files` expect a certain format, and it would not do at all to heed\n> `diff.noPrefix`, for example.\n> \n> In that respect, `git add -p` using `diff-files` is kind of wrong: we\n> _want_ to show the result to the user, with no processing at all (execpt\n> the user-provided `diffFilter`).\n\nSort of. The problem is that we need two matching copies of the diff:\none to apply, and one to show the user. They don't need to be\nbyte-for-byte identical, but they should correlate at the level of\nindividual lines. And the \"one to apply\" can take on some user-selected\noptions, as long as the result can still be applied.\n\n> So why not introduce a new option to `diff-files` and `diff-index` to ask\n> it _specifically_ to heed diff UI config settings? I.e. a command-line\n> option that makes it call\n> \n>         git_config(git_diff_ui_config, NULL);\n> \n> instead of\n> \n>         git_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n\nWould you pass that option to both of the diff calls, or just the one\ngenerating the human-readable input?\n\nIf just the human-readable one, then many options that change the line\ncount would be problems: diff.context, diff.interhunkcontext,\ndiff.orderfile, etc.\n\nIf both, then some options would be problematic for applying. Just\nlooking over the list, these jump out at me:\n\n  - color.diff=always would obviously be an issue (though TBH I think\n    anybody doing that is inviting a lot of breakages anyway)\n\n  - diff.external would be a problem if it kicked in, though I think it\n    would require --ext-diff to actually do anything\n\n  - diff.submodule would generate diffs that can't be applied\n\n-Peff\n"},{"id":"391087","messageId":"xmqqlfpi8au6.fsf@gitster-ct.c.googlers.com","threadId":"52721","inReplyTo":"20200203145155.GA272077@coredump.intra.peff.net","subject":"Re: Patch text in git-add patch mode lacks whitespace highlighting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-04T18:29:53Z","receivedAt":"2020-02-04T18:29:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> If just the human-readable one, then many options that change the line\n> count would be problems: diff.context, diff.interhunkcontext,\n> diff.orderfile, etc.\n>\n> If both, then some options would be problematic for applying. Just\n> looking over the list, these jump out at me:\n>\n>   - color.diff=always would obviously be an issue (though TBH I think\n>     anybody doing that is inviting a lot of breakages anyway)\n>\n>   - diff.external would be a problem if it kicked in, though I think it\n>     would require --ext-diff to actually do anything\n>\n>   - diff.submodule would generate diffs that can't be applied\n\nWhat can truly help \"git add -p\" might be a new output mode from\n\"git diff\", perhaps, in which instead of writing a stream of text\n(with color codes intermixed), the output machinery makes a call to\nAPI-user supplied callbacks to report (\"type of output line\", \"the\npayload\", \"list of coloring information\") tuples for each line, so\nthat the API-user can synthesize _both_ versions it currently uses\nwith a single invocation of \"diff\"?  There may be tons of \"output\nline\" types, like the diff header, hunk header, context, removed,\nadded, etc., and depending on the type, the form the payload takes\nmay be different (e.g. a context line may be just a plain text, a\nhunk header line may be a five-tuple (pre- and post-context line\nnumbers and line countsm plus the function-header line text).\n\nAs long as the call the API-user makes to the diff machinery grabs\neverything that is needed for both human and machine consumption in\na single call, there is no risk of getting confused by two sets of\ninconsistent patch data.\n\n\n"},{"id":"391093","messageId":"nycvar.QRO.7.76.6.2002042003320.1058@tvgsbejvaqbjf.bet","threadId":"52721","inReplyTo":"20200203145155.GA272077@coredump.intra.peff.net","subject":"Re: Patch text in git-add patch mode lacks whitespace highlighting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-04T19:04:33Z","receivedAt":"2020-02-04T19:04:45Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 3 Feb 2020, Jeff King wrote:\n\n> On Mon, Feb 03, 2020 at 01:37:48PM +0100, Johannes Schindelin wrote:\n>\n> > > The short of my answer is that I think the color-moved stuff might be a\n> > > candidate, but it's sufficiently different that I think it should be\n> > > decided on as a separate patch.\n> >\n> > I actually wonder whether we should do something completely different. The\n> > problem with `git diff-files --color`, after all, is that `diff-files` was\n> > never intended to produce user-facing output, so the `--color` is somewhat\n> > of a contradiction here. The fact that `diff-files` is a low-level (or\n> > \"plumbing\") command means that by nature, it wants to control a lot more\n> > what the user-provided config can change (typically, scripts calling\n> > `diff-files` expect a certain format, and it would not do at all to heed\n> > `diff.noPrefix`, for example.\n> >\n> > In that respect, `git add -p` using `diff-files` is kind of wrong: we\n> > _want_ to show the result to the user, with no processing at all (execpt\n> > the user-provided `diffFilter`).\n>\n> Sort of. The problem is that we need two matching copies of the diff:\n> one to apply, and one to show the user. They don't need to be\n> byte-for-byte identical, but they should correlate at the level of\n> individual lines. And the \"one to apply\" can take on some user-selected\n> options, as long as the result can still be applied.\n>\n> > So why not introduce a new option to `diff-files` and `diff-index` to ask\n> > it _specifically_ to heed diff UI config settings? I.e. a command-line\n> > option that makes it call\n> >\n> >         git_config(git_diff_ui_config, NULL);\n> >\n> > instead of\n> >\n> >         git_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n>\n> Would you pass that option to both of the diff calls, or just the one\n> generating the human-readable input?\n>\n> If just the human-readable one, then many options that change the line\n> count would be problems: diff.context, diff.interhunkcontext,\n> diff.orderfile, etc.\n\nDarn, you have a very valid point :-(\n\n> If both, then some options would be problematic for applying. Just\n> looking over the list, these jump out at me:\n>\n>   - color.diff=always would obviously be an issue (though TBH I think\n>     anybody doing that is inviting a lot of breakages anyway)\n>\n>   - diff.external would be a problem if it kicked in, though I think it\n>     would require --ext-diff to actually do anything\n>\n>   - diff.submodule would generate diffs that can't be applied\n\nSo I guess it is back to your patch, maybe amended by a move of the\ncolor-moved setting.\n\nCiao,\nDscho\n"},{"id":"391097","messageId":"20200204195709.GC12705@coredump.intra.peff.net","threadId":"52721","inReplyTo":"xmqqlfpi8au6.fsf@gitster-ct.c.googlers.com","subject":"Re: Patch text in git-add patch mode lacks whitespace highlighting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-04T19:57:09Z","receivedAt":"2020-02-04T19:57:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 04, 2020 at 10:29:53AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > If just the human-readable one, then many options that change the line\n> > count would be problems: diff.context, diff.interhunkcontext,\n> > diff.orderfile, etc.\n> >\n> > If both, then some options would be problematic for applying. Just\n> > looking over the list, these jump out at me:\n> >\n> >   - color.diff=always would obviously be an issue (though TBH I think\n> >     anybody doing that is inviting a lot of breakages anyway)\n> >\n> >   - diff.external would be a problem if it kicked in, though I think it\n> >     would require --ext-diff to actually do anything\n> >\n> >   - diff.submodule would generate diffs that can't be applied\n> \n> What can truly help \"git add -p\" might be a new output mode from\n> \"git diff\", perhaps, in which instead of writing a stream of text\n> (with color codes intermixed), the output machinery makes a call to\n> API-user supplied callbacks to report (\"type of output line\", \"the\n> payload\", \"list of coloring information\") tuples for each line, so\n> that the API-user can synthesize _both_ versions it currently uses\n> with a single invocation of \"diff\"?  There may be tons of \"output\n> line\" types, like the diff header, hunk header, context, removed,\n> added, etc., and depending on the type, the form the payload takes\n> may be different (e.g. a context line may be just a plain text, a\n> hunk header line may be a five-tuple (pre- and post-context line\n> numbers and line countsm plus the function-header line text).\n\nI agree that would be a nice thing to have, and would let us avoid\nrunning the same diff twice. But I don't think it solves the problem you\nquoted above, unless all of the features are represented in some\nmachine-readable form. E.g., what would diff.submodule do in such an\noutput, and how would \"add -p\" decide to show it?\n\n> As long as the call the API-user makes to the diff machinery grabs\n> everything that is needed for both human and machine consumption in\n> a single call, there is no risk of getting confused by two sets of\n> inconsistent patch data.\n\nYes, but we still can't say \"use machine-readable output, and turn on\nall the config bells and whistles\" because we'd have to know how to\nhandle every bell and whistle.\n\n-Peff\n"}]}