| Message ID | 38355ec98f04783367d74e38cda3ce5d6632c7ac.1605051739.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix color handling in git add -i | expand |
On Tue, Nov 10, 2020 at 11:42:19PM +0000, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > Now that the Perl version produces the same output as the built-in > version (mostly fixing bugs in the latter), let's add a regression test > to verify that it stays this way. > > Note that we only `grep` for the colored error message instead of > verifying that the entire `stderr` consists of just this one line: when > running the test script via `dash`, using the `-x` option to trace the > commands, the sub-shell in `force_color` causes those commands to be > traced into `err.raw` because we set, but do not export > `BASH_XTRACEFD`). Your reasoning here confuses me. If we are using dash, then surely BASH_XTRACEFD does not matter either way? Perhaps the subshell is important if we are running bash, though. Either way, being forgiving to the use of "-x" is a nice convenience and I approve. > +test_expect_success 'colors can be overridden' ' > + test_config color.interactive.header red && > + test_config color.interactive.help green && > + test_config color.interactive.prompt yellow && > + test_config color.interactive.error blue && > + test_config color.diff.frag magenta && > + test_config color.diff.context cyan && > + test_config color.diff.old bold && > + test_config color.diff.new "bold red" && Since you are being so thorough, and since it is already in the output, maybe color color.diff.meta, too? Like: diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 28549a41a2..cca866c8f4 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -594,6 +594,7 @@ test_expect_success 'colors can be overridden' ' test_config color.interactive.help green && test_config color.interactive.prompt yellow && test_config color.interactive.error blue && + test_config color.diff.meta italic && test_config color.diff.frag magenta && test_config color.diff.context cyan && test_config color.diff.old bold && @@ -625,9 +626,9 @@ test_expect_success 'colors can be overridden' ' 1: +3/-0 +1/-1 <YELLOW>c<RESET>olor-test <YELLOW>Patch update<RESET>>> <RED> staged unstaged path<RESET> * 1: +3/-0 +1/-1 <YELLOW>c<RESET>olor-test - <YELLOW>Patch update<RESET>>> <BOLD>diff --git a/color-test b/color-test<RESET> - <BOLD>--- a/color-test<RESET> - <BOLD>+++ b/color-test<RESET> + <YELLOW>Patch update<RESET>>> <ITALIC>diff --git a/color-test b/color-test<RESET> + <ITALIC>--- a/color-test<RESET> + <ITALIC>+++ b/color-test<RESET> <MAGENTA>@@ -1,3 +1,3 @@<RESET> <CYAN> context<RESET> <BOLD>-old<RESET> -Peff
Hi Peff, On Tue, 10 Nov 2020, Jeff King wrote: > On Tue, Nov 10, 2020 at 11:42:19PM +0000, Johannes Schindelin via GitGitGadget wrote: > > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > Now that the Perl version produces the same output as the built-in > > version (mostly fixing bugs in the latter), let's add a regression test > > to verify that it stays this way. > > > > Note that we only `grep` for the colored error message instead of > > verifying that the entire `stderr` consists of just this one line: when > > running the test script via `dash`, using the `-x` option to trace the > > commands, the sub-shell in `force_color` causes those commands to be > > traced into `err.raw` because we set, but do not export > > `BASH_XTRACEFD`). > > Your reasoning here confuses me. Sorry! > If we are using dash, then surely BASH_XTRACEFD does not matter either > way? It kinda does, though. Dash _does_ respect the `BASH_XTRACEFD` variable, funnily enough, but does not hand the settings through to sub-shells, whereas Bash does. > Perhaps the subshell is important if we are running bash, though. The most important thing, which I of course _failed_ to describe most prominently, is that _in general_ `-x` will mess up the `err` file, i.e. when running with anything but Bash. Therefoer we need the `grep` instead of a `test_cmp`, and that's what I tried to convey in v2. > Either way, being forgiving to the use of "-x" is a nice convenience and > I approve. > > > +test_expect_success 'colors can be overridden' ' > > + test_config color.interactive.header red && > > + test_config color.interactive.help green && > > + test_config color.interactive.prompt yellow && > > + test_config color.interactive.error blue && > > + test_config color.diff.frag magenta && > > + test_config color.diff.context cyan && > > + test_config color.diff.old bold && > > + test_config color.diff.new "bold red" && > > Since you are being so thorough, and since it is already in the output, > maybe color color.diff.meta, too? Like: > > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > index 28549a41a2..cca866c8f4 100755 > --- a/t/t3701-add-interactive.sh > +++ b/t/t3701-add-interactive.sh > @@ -594,6 +594,7 @@ test_expect_success 'colors can be overridden' ' > test_config color.interactive.help green && > test_config color.interactive.prompt yellow && > test_config color.interactive.error blue && > + test_config color.diff.meta italic && > test_config color.diff.frag magenta && > test_config color.diff.context cyan && > test_config color.diff.old bold && > @@ -625,9 +626,9 @@ test_expect_success 'colors can be overridden' ' > 1: +3/-0 +1/-1 <YELLOW>c<RESET>olor-test > <YELLOW>Patch update<RESET>>> <RED> staged unstaged path<RESET> > * 1: +3/-0 +1/-1 <YELLOW>c<RESET>olor-test > - <YELLOW>Patch update<RESET>>> <BOLD>diff --git a/color-test b/color-test<RESET> > - <BOLD>--- a/color-test<RESET> > - <BOLD>+++ b/color-test<RESET> > + <YELLOW>Patch update<RESET>>> <ITALIC>diff --git a/color-test b/color-test<RESET> > + <ITALIC>--- a/color-test<RESET> > + <ITALIC>+++ b/color-test<RESET> > <MAGENTA>@@ -1,3 +1,3 @@<RESET> > <CYAN> context<RESET> > <BOLD>-old<RESET> Oh my. I really had tried to avoid going _this_ deep. The `.meta` setting is not even read by the interactive add command: $ git grep -w meta git-add--interactive.perl add-interactive.c \ add-patch.c comes up empty. The reason is that we actually let the diff machinery do all the coloring. So why, then, you might ask, do we bother to read those values in the first place? Well, you know, in `git add -p`, you can split hunks. That's still fine, we basically just have to color the hunk header that we insert, and can reuse the original coloring of the remaining lines, because they are unchanged. But. But you can also `edit` hunks. And after that `edit` step, those hunks have to be re-colored. And _that_ is why `git-add--interactive` bothers to read those `color.diff.*` values to begin with. Now, how do you see this re-coloring in action? I am almost glad that you asked. By simulating that `edit` in the regression test. But that is not enough because the hunk is staged immediately after editing. But you _can_ go back to that hunk, even if it already has been changed, and make up your mind to _not_ stage it, but that only works if `git add -p` hasn't quit already, which it would do with a single hunk, so we have to have _two_ hunks. This not only complicates the regression test, but _of course_ (because _why would I already be done with this patch series???_) it shows further differences between the Perl version and the built-in version. It's almost as if `git add -i` said "Johannes, you think those 500 hours were all you would spend on converting me to a built-in? Think again!". One of the issues I found (and fixed in v2) was that the built-in version tried to be practical and _not_ special-case a count of one in the hunk header (`@@ -3 +3 @@` is equivalent to `@@ -3,1 +3,1 @@`). Well, now it does, just like libxdiff and the Perl version. The other issue was that the Perl version still used `color.diff.plain` instead of `color.diff.context`. I hope I found a good-enough solution to the problem that we simply cannot call `git config --get-color` or `repo_get_config()` with _two_ keys to look for (and the last one wins). For those reasons, v2 brings more changes than I had hoped for. In the end, it is a better patch series, obviously. So even if I was reluctant to work on all this: thank you for prodding me. Ciao, Dscho
On Wed, Nov 11, 2020 at 04:53:13PM +0100, Johannes Schindelin wrote: > > If we are using dash, then surely BASH_XTRACEFD does not matter either > > way? > > It kinda does, though. Dash _does_ respect the `BASH_XTRACEFD` variable, > funnily enough, but does not hand the settings through to sub-shells, > whereas Bash does. Really? That's news to me, and doesn't seem to trigger: [bash uses it] $ bash -xc 'BASH_XTRACEFD=3; echo foo' 3>trace + BASH_XTRACEFD=3 foo $ cat trace + echo foo [dash does not] $ dash -xc 'BASH_XTRACEFD=3; echo foo' 3>trace + BASH_XTRACEFD=3 + echo foo foo $ cat trace > > Perhaps the subshell is important if we are running bash, though. > > The most important thing, which I of course _failed_ to describe most > prominently, is that _in general_ `-x` will mess up the `err` file, i.e. > when running with anything but Bash. Therefoer we need the `grep` instead > of a `test_cmp`, and that's what I tried to convey in v2. Yeah. I understood that part (because it has bit me so many times ;) ), but I agree it's good to make it clear. > Oh my. I really had tried to avoid going _this_ deep. The `.meta` setting > is not even read by the interactive add command: > > $ git grep -w meta git-add--interactive.perl add-interactive.c \ > add-patch.c > > comes up empty. > [how and why add--interactive.perl reads color config] Hmm. Right, I knew about that weirdness. But I assumed that the builtin add-interactive was doing the diffs in-core. Otherwise, why would we have seen the failure to load diff.color.frag in the first place? Philippe's simple example just did "git add -p". So now I'm doubly confused. The answer seems to be that render_hunk() always _replaces_ the colors we got from running the external diff. Whereas the perl version only applied coloring when reading back in the results of an edit operation (and likewise used the frag color when generating a split hunk header). I'm not sure that what the builtin version is doing is wrong, but it seems like it's putting a lot of extra effort into parsing colors off of the colorized version. Whereas the perl version just assumes the lines match up. I do wonder if there are corner cases we might hit around filters here, though. The lines we get from a filter might bear no resemblance at all to diff lines. The only thing we require in the perl version is that they have correspond 1-to-1 with the unfiltered diff lines in meaning. > For those reasons, v2 brings more changes than I had hoped for. In the > end, it is a better patch series, obviously. So even if I was reluctant to > work on all this: thank you for prodding me. Heh. Sorry and thanks, I guess? :) I'll try to read over v2 carefully. -Peff
Hi Peff, On Wed, 11 Nov 2020, Jeff King wrote: > On Wed, Nov 11, 2020 at 04:53:13PM +0100, Johannes Schindelin wrote: > > > > If we are using dash, then surely BASH_XTRACEFD does not matter either > > > way? > > > > It kinda does, though. Dash _does_ respect the `BASH_XTRACEFD` variable, > > funnily enough, but does not hand the settings through to sub-shells, > > whereas Bash does. > > Really? That's news to me, and doesn't seem to trigger: > > [bash uses it] > $ bash -xc 'BASH_XTRACEFD=3; echo foo' 3>trace > + BASH_XTRACEFD=3 > foo > $ cat trace > + echo foo > > [dash does not] > $ dash -xc 'BASH_XTRACEFD=3; echo foo' 3>trace > + BASH_XTRACEFD=3 > + echo foo > foo > $ cat trace Gaaah! I totally forgot that `BASH_XTRACEFD` is only heeded by BusyBox' ash (and only when built with `ENABLE_ASH_BASH_COMPAT`), not by `dash`. Sorry for the noise. > > Oh my. I really had tried to avoid going _this_ deep. The `.meta` setting > > is not even read by the interactive add command: > > > > $ git grep -w meta git-add--interactive.perl add-interactive.c \ > > add-patch.c > > > > comes up empty. > > [how and why add--interactive.perl reads color config] > > Hmm. Right, I knew about that weirdness. But I assumed that the builtin > add-interactive was doing the diffs in-core. Otherwise, why would we > have seen the failure to load diff.color.frag in the first place? Oh, that's easy to explain: as you can verify reading https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898 the Perl version of `git add -p` insists on (re-)constructing the hunk headers manually, and obviously it needs to color them manually, too. And https://github.com/git/git/blob/e31aba42fb12/add-patch.c#L649-L672 shows that the built-in version of `git add -p` slavishly follows that practice. > Philippe's simple example just did "git add -p". So now I'm doubly > confused. Right. I should have been more precise in what parts are used of the diff that is colored via the internal diff machinery. The hunk headers are not used. The hunks themselves are, unless edited. The file header is, too: https://github.com/git/git/blob/e31aba42fb12/add-patch.c#L683-L714 > The answer seems to be that render_hunk() always _replaces_ the colors > we got from running the external diff. Whereas the perl version only > applied coloring when reading back in the results of an edit operation > (and likewise used the frag color when generating a split hunk header). No, the Perl version also insists on applying `fraginfo_color`, see https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898 > I'm not sure that what the builtin version is doing is wrong, but it > seems like it's putting a lot of extra effort into parsing colors off of > the colorized version. Whereas the perl version just assumes the lines > match up. I do wonder if there are corner cases we might hit around > filters here, though. The lines we get from a filter might bear no > resemblance at all to diff lines. The only thing we require in the perl > version is that they have correspond 1-to-1 with the unfiltered diff > lines in meaning. They do have to correspond 1-to-1 because the assumption is that the individual lines will then correspond one-to-one, too. This does not need to be true, of course, but then the filter is probably less useful than the user wants it to be. > > For those reasons, v2 brings more changes than I had hoped for. In the > > end, it is a better patch series, obviously. So even if I was reluctant to > > work on all this: thank you for prodding me. > > Heh. Sorry and thanks, I guess? :) I'll try to read over v2 carefully. No need to be sorry on your side. _I_ am sorry that I did not add that test case during my re-implementation efforts in the first place. Thanks, DScho
On Thu, Nov 12, 2020 at 03:04:01PM +0100, Johannes Schindelin wrote: > Gaaah! I totally forgot that `BASH_XTRACEFD` is only heeded by BusyBox' > ash (and only when built with `ENABLE_ASH_BASH_COMPAT`), not by `dash`. > > Sorry for the noise. Ah, that makes more sense. And I learned something new about busybox. :) > > Hmm. Right, I knew about that weirdness. But I assumed that the builtin > > add-interactive was doing the diffs in-core. Otherwise, why would we > > have seen the failure to load diff.color.frag in the first place? > > Oh, that's easy to explain: as you can verify reading > https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898 > the Perl version of `git add -p` insists on (re-)constructing the hunk > headers manually, and obviously it needs to color them manually, too. And > https://github.com/git/git/blob/e31aba42fb12/add-patch.c#L649-L672 shows > that the built-in version of `git add -p` slavishly follows that practice. But that is only when we split hunks (your link to the perl script is in split_hunks()). I agree we must color manually there when creating our own hunk header. But outside of that and patch-editing, the perl script does not otherwise recolor or rewrite (nor even really parse beyond line-splitting) what it gets from the colorized version. Whereas add-patch parses the colors off of the version and then re-colors every hunk header. Which seems doubly weird to me. Even if we were going to re-color every hunk (e.g., because we don't want to store the original hunk line, but instead a parsed version of it), why bother parsing the color version at all, and not just the machine-readable version? > > The answer seems to be that render_hunk() always _replaces_ the colors > > we got from running the external diff. Whereas the perl version only > > applied coloring when reading back in the results of an edit operation > > (and likewise used the frag color when generating a split hunk header). > > No, the Perl version also insists on applying `fraginfo_color`, see > https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898 Only when we split. Try this to give different colors between the interactive script and diff: diff --git a/git-add--interactive.perl b/git-add--interactive.perl index e713fe3d02..862a21ff1f 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -28,8 +28,9 @@ my $diff_use_color = $repo->get_colorbool('color.diff'); my ($fraginfo_color) = $diff_use_color ? ( - $repo->get_color('color.diff.frag', 'cyan'), + $repo->get_color('color.diff.nonsense', 'yellow'), ) : (); +# noop to create split hunk my ($diff_plain_color) = $diff_use_color ? ( $repo->get_color('color.diff.plain', ''), Running "git add -p" does not result in yellow hunk headers. But issuing a split command does. The distinction is mostly academic, because diff-tree and the interactive patch code should be using the same colors, so the result should look the same. It could matter if the diff-filter chooses different colors, though then the split headers will not match the originals in style. We _could_ run the newly created hunk header individually through the diff-filter, though I'm not sure how various filters would handle that. That's true of the perl version as well as the builtin one, but I think the builtin one's insistence on parsing the colored output is taking us in the wrong direction to eventually fix that. > > I'm not sure that what the builtin version is doing is wrong, but it > > seems like it's putting a lot of extra effort into parsing colors off of > > the colorized version. Whereas the perl version just assumes the lines > > match up. I do wonder if there are corner cases we might hit around > > filters here, though. The lines we get from a filter might bear no > > resemblance at all to diff lines. The only thing we require in the perl > > version is that they have correspond 1-to-1 with the unfiltered diff > > lines in meaning. > > They do have to correspond 1-to-1 because the assumption is that the > individual lines will then correspond one-to-one, too. This does not need > to be true, of course, but then the filter is probably less useful than > the user wants it to be. Right, I'm not disputing the 1-to-1 thing (I was after all the one who implemented interactive.diffilter, and added the "complain if the counts don't line up" check). But in the perl script they only need to correspond _semantically_, not syntactically. -Peff
Hi Peff, On Thu, 12 Nov 2020, Jeff King wrote: > On Thu, Nov 12, 2020 at 03:04:01PM +0100, Johannes Schindelin wrote: > > > > Hmm. Right, I knew about that weirdness. But I assumed that the builtin > > > add-interactive was doing the diffs in-core. Otherwise, why would we > > > have seen the failure to load diff.color.frag in the first place? > > > > Oh, that's easy to explain: as you can verify reading > > https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898 > > the Perl version of `git add -p` insists on (re-)constructing the hunk > > headers manually, and obviously it needs to color them manually, too. And > > https://github.com/git/git/blob/e31aba42fb12/add-patch.c#L649-L672 shows > > that the built-in version of `git add -p` slavishly follows that practice. > > But that is only when we split hunks (your link to the perl script is in > split_hunks()). I agree we must color manually there when creating our > own hunk header. But outside of that and patch-editing, the perl script > does not otherwise recolor or rewrite (nor even really parse beyond > line-splitting) what it gets from the colorized version. > > Whereas add-patch parses the colors off of the version and then > re-colors every hunk header. Which seems doubly weird to me. Even if we > were going to re-color every hunk (e.g., because we don't want to store > the original hunk line, but instead a parsed version of it), why bother > parsing the color version at all, and not just the machine-readable > version? Let's continue on this distraction for a bit before I go back to fixing the patch series, which actually tries to fix a _different_ concern. The reason why `add-patch.c` "parses the colors off" is that we want to show the rest of the hunk header, in color, even after splitting hunks (this will only be shown for the first hunk, of course). But given that `git add -p` is somewhat of a fringe use, and using `diffFilter` is _even_ more fringe, I do not really want to spend any further time on this tangent. > > > The answer seems to be that render_hunk() always _replaces_ the colors > > > we got from running the external diff. Whereas the perl version only > > > applied coloring when reading back in the results of an edit operation > > > (and likewise used the frag color when generating a split hunk header). > > > > No, the Perl version also insists on applying `fraginfo_color`, see > > https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898 > > Only when we split. Try this to give different colors between the > interactive script and diff: > > diff --git a/git-add--interactive.perl b/git-add--interactive.perl > index e713fe3d02..862a21ff1f 100755 > --- a/git-add--interactive.perl > +++ b/git-add--interactive.perl > @@ -28,8 +28,9 @@ > my $diff_use_color = $repo->get_colorbool('color.diff'); > my ($fraginfo_color) = > $diff_use_color ? ( > - $repo->get_color('color.diff.frag', 'cyan'), > + $repo->get_color('color.diff.nonsense', 'yellow'), > ) : (); > +# noop to create split hunk > my ($diff_plain_color) = > $diff_use_color ? ( > $repo->get_color('color.diff.plain', ''), > > Running "git add -p" does not result in yellow hunk headers. But issuing > a split command does. > > The distinction is mostly academic, because diff-tree and the > interactive patch code should be using the same colors, so the result > should look the same. It could matter if the diff-filter chooses > different colors, though then the split headers will not match the > originals in style. We _could_ run the newly created hunk header > individually through the diff-filter, though I'm not sure how various > filters would handle that. > > That's true of the perl version as well as the builtin one, but I think > the builtin one's insistence on parsing the colored output is taking us > in the wrong direction to eventually fix that. My thinking back then was: what if _I_ want to use a diffFilter? For what would I use it? Probably to emphasize certain hunk headers more, by adding more color to the part after the line range. Anyway. I stand by what I said above: I do not want to spend any further time on this tangent, at least not right now. There are more pressing challenges waiting for me, and I expect those other challenge to have a much bigger "return on investment". Ciao, Dscho > > > I'm not sure that what the builtin version is doing is wrong, but it > > > seems like it's putting a lot of extra effort into parsing colors off of > > > the colorized version. Whereas the perl version just assumes the lines > > > match up. I do wonder if there are corner cases we might hit around > > > filters here, though. The lines we get from a filter might bear no > > > resemblance at all to diff lines. The only thing we require in the perl > > > version is that they have correspond 1-to-1 with the unfiltered diff > > > lines in meaning. > > > > They do have to correspond 1-to-1 because the assumption is that the > > individual lines will then correspond one-to-one, too. This does not need > > to be true, of course, but then the filter is probably less useful than > > the user wants it to be. > > Right, I'm not disputing the 1-to-1 thing (I was after all the one who > implemented interactive.diffilter, and added the "complain if the counts > don't line up" check). But in the perl script they only need to > correspond _semantically_, not syntactically. > > -Peff >
On Mon, Nov 16, 2020 at 12:35:44AM +0100, Johannes Schindelin wrote: > > Whereas add-patch parses the colors off of the version and then > > re-colors every hunk header. Which seems doubly weird to me. Even if we > > were going to re-color every hunk (e.g., because we don't want to store > > the original hunk line, but instead a parsed version of it), why bother > > parsing the color version at all, and not just the machine-readable > > version? > > Let's continue on this distraction for a bit before I go back to fixing > the patch series, which actually tries to fix a _different_ concern. > > The reason why `add-patch.c` "parses the colors off" is that we want to > show the rest of the hunk header, in color, even after splitting hunks > (this will only be shown for the first hunk, of course). OK, this is the part I didn't quite understand. My contention was: if we are regenerating a new hunk header after we split, why do we care what was in the old one? This "rest of the hunk header" is what I didn't get. You are talking here about the funcname header. Which is not colored by default, but people can set color.diff.func. And here the builtin does differ from the perl script quite a bit. The perl script generates the split hunk headers from scratch, and does not bother to include the funcname in the split header: $ git show :foo.c void foo() { call(); some(); functions(); } $ cat foo.c void foo() { call(); some(); more(); functions(); return; } $ git add -p diff --git a/foo.c b/foo.c index 270ccc7..0365419 100644 --- a/foo.c +++ b/foo.c @@ -2,5 +2,7 @@ void foo() { call(); some(); + more(); functions(); + return; } (1/1) Stage this hunk [y,n,q,a,d,s,e,?]? s Split into 2 hunks. @@ -2,4 +2,5 @@ { call(); some(); + more(); functions(); (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? n @@ -5,2 +6,3 @@ functions(); + return; } (2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? n So it does not appear at all after the split, colored or otherwise. I think the behavior of the builtin version is better in this case. It does place more restrictions on what the diffFilter can do, as we've been discussing. The same thing could be accomplished by retaining the uncolored func header, and then coloring it on the fly within add-patch.c (exactly the same way we color the numeric part of the hunk header). It may also be worth covering this with a test (it was a surprise to me that the builtin version was handling this, though as I said I do agree it's an improvement). > But given that `git add -p` is somewhat of a fringe use, and using > `diffFilter` is _even_ more fringe, I do not really want to spend any > further time on this tangent. I agree that diffFilter is quite fringe. And as I said before, I'm fine punting on it for now. Mostly this was just the first I had found out about the difference, and because it is user-visible, it may be something that comes up later. So I wanted to record a description of the problem for later. > My thinking back then was: what if _I_ want to use a diffFilter? For what > would I use it? Probably to emphasize certain hunk headers more, by adding > more color to the part after the line range. Yes, that's what I use it for. I wrote it mostly to pipe through diff-highlight, though also it was to help diff-so-fancy folks (which I don't use myself, but they build on diff-highlight). However, it does seem that they haven't really worked out the problems in the meantime. -Peff
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index ca04fac417..28549a41a2 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -589,6 +589,59 @@ test_expect_success 'diffs can be colorized' ' grep "$(printf "\\033")" output ' +test_expect_success 'colors can be overridden' ' + test_config color.interactive.header red && + test_config color.interactive.help green && + test_config color.interactive.prompt yellow && + test_config color.interactive.error blue && + test_config color.diff.frag magenta && + test_config color.diff.context cyan && + test_config color.diff.old bold && + test_config color.diff.new "bold red" && + + git reset --hard && + test_when_finished "git rm -f color-test" && + test_write_lines context old more-context >color-test && + git add color-test && + test_write_lines context new more-context >color-test && + + echo trigger an error message >input && + force_color git add -i 2>err.raw <input && + test_decode_color <err.raw >err && + grep "<BLUE>Huh (trigger)?<RESET>" err && + + test_write_lines patch color-test "" y quit >input && + force_color git add -i >actual.raw <input && + test_decode_color <actual.raw >actual.decoded && + sed "/index [0-9a-f]*\\.\\.[0-9a-f]* 100644/d" <actual.decoded >actual && + cat >expect <<-\EOF && + <RED> staged unstaged path<RESET> + 1: +3/-0 +1/-1 color-test + + <RED>*** Commands ***<RESET> + 1: <YELLOW>s<RESET>tatus 2: <YELLOW>u<RESET>pdate 3: <YELLOW>r<RESET>evert 4: <YELLOW>a<RESET>dd untracked + 5: <YELLOW>p<RESET>atch 6: <YELLOW>d<RESET>iff 7: <YELLOW>q<RESET>uit 8: <YELLOW>h<RESET>elp + <YELLOW>What now<RESET>> <RED> staged unstaged path<RESET> + 1: +3/-0 +1/-1 <YELLOW>c<RESET>olor-test + <YELLOW>Patch update<RESET>>> <RED> staged unstaged path<RESET> + * 1: +3/-0 +1/-1 <YELLOW>c<RESET>olor-test + <YELLOW>Patch update<RESET>>> <BOLD>diff --git a/color-test b/color-test<RESET> + <BOLD>--- a/color-test<RESET> + <BOLD>+++ b/color-test<RESET> + <MAGENTA>@@ -1,3 +1,3 @@<RESET> + <CYAN> context<RESET> + <BOLD>-old<RESET> + <BOLD;RED>+<RESET><BOLD;RED>new<RESET> + <CYAN> more-context<RESET> + <YELLOW>(1/1) Stage this hunk [y,n,q,a,d,e,?]? <RESET> + <RED>*** Commands ***<RESET> + 1: <YELLOW>s<RESET>tatus 2: <YELLOW>u<RESET>pdate 3: <YELLOW>r<RESET>evert 4: <YELLOW>a<RESET>dd untracked + 5: <YELLOW>p<RESET>atch 6: <YELLOW>d<RESET>iff 7: <YELLOW>q<RESET>uit 8: <YELLOW>h<RESET>elp + <YELLOW>What now<RESET>> Bye. + EOF + test_cmp expect actual +' + test_expect_success 'colorized diffs respect diff.wsErrorHighlight' ' git reset --hard &&