Hi Peff, On Wed, 11 Nov 2020, Jeff King wrote: > On Wed, Nov 11, 2020 at 12:28:13PM +0000, Johannes Schindelin via GitGitGadget wrote: > > > Changes since v1: > > > > * The regression test now actually exercises the re-coloring (that is the > > primary purpose of git add -p looking at the color.diff.* variables). > > * The way the built-in git add -p renders hunk headers of split hunks was > > aligned with how the Perl version does things. > > * We now consistently prefer color.diff.context over color.diff.plain, no > > matter whether using the Perl or the built-in version of git add -p. > > * The commit message for the regression test no longer confuses readers by > > mentioning dash. > > * The regression test was structured a bit better, first testing error > > message coloring, then the menu in git add -i and then the diff coloring > > in git add -p. > > The changes were less scary than I was led to believe after reading your > earlier response. :) > > Everything here looks sensible. As I said elsewhere, I do worry there > may be a lingering issue with how parse_diff() looks at the filtered > diffs. Let me see if I can get diff-so-fancy working... > > Aha, yes. I think using diff-so-fancy here isn't entirely robust, > because it has some cases where it fails at the 1-to-1 line > correspondence, but they're aware of the issue. But it does work in > simples cases now (there's some coloring which makes the output more > meaningful, but I obviously won't paste it here): > > $ git -c interactive.difffilter='diff-so-fancy' add -p > ────────────────────────────────────────────────────────────────────── > modified: file > ────────────────────────────────────────────────────────────────────── > @ file:1 @ > old > new > (1/1) Stage this hunk [y,n,q,a,d,e,?]? It might _seem_ that it works. But try to split a hunk. I did this with the test case (interrupting it just before running `add -p`): -- snip -- $ git -C ./t/trash\ directory.t3701-add-interactive/ -c interactive.difffilter='diff-so-fancy' -c add.interactive.usebuiltin=false add -p <BOLD>────────────────────────────────────────────────────────────────────────────────</BOLD> <BOLD>modified: color-test</BOLD> <BOLD>────────────────────────────────────────────────────────────────────────────────</BOLD> <MAGENTA>@ color-test:1 @</MAGENTA> context <RED>old</RED> <GREEN>new</GREEN> more-context <GREEN>another-one</GREEN> <BLUE>(1/1) Stage this hunk [y,n,q,a,d,s,e,?]?</BLUE> s <BOLD>Split into 2 hunks.</BOLD> <MAGENTA>@@ -1,3 +1,3 @@</MAGENTA> <RED>old</RED> <GREEN>new</GREEN> more-context <GREEN>another-one</GREEN> <BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?</BLUE> -- snap -- Obviously, I marked it up so you can see what parts were colored. See how it first _looks_ as if it works? But then you split the hunk, but instead of showing only the old/new part, it shows the old/new/another-one part! In other words, it does not work at all, and the fact that it does not even warn you about it is misleading, and that's pretty much all I will say about it. > But with the builtin: > > $ git -c interactive.difffilter='diff-so-fancy' \ > -c add.interactive.usebuiltin=true \ > add -p > error: could not parse colored hunk header '?[1m?[38;5;1m?[31mold?[m' Granted, this is not quite helpful. I _think_ what is happening is that the number of lines is different, and `add -p` goes like: noooope! But it could probably be better at describing what the issue is. Or it could cater to `diff-so-fancy`, if that is what people use these days, and special-case it by falling back to detecting the hunk boundaries in a manner that is compatible with `diff-so-fancy`. Or we might be able to come up with a strategy that is not so limited and that works for more than just `diff-so-fancy`. > I don't use it myself, and as I said, I think they have some fixes to > make for it to be robust as a diff-filter. But I suspect this may be a > problem once the builtin version becomes the default. > > I don't think it should be dealt with in this patch series, though. Oh, _that_ I wholeheartedly agree with. > While it may touch on some of the coloring code, it's otherwise > orthogonal to the fixes here. And while the fix is likely to be to make > render_hunk() stop re-coloring in the non-edit cases, your more-robust > test here will still be checking what you expect (because it really is > triggering the edit case, not relying on the extra coloring to see the > effect of in-process color config). I don't actually think that we _can_ stop re-coloring the hunk header in the general case because we need to keep this working even for split hunks. It would be a very bad idea to make it work for non-split cases and then something like `die()` only when the hunk is split. Better re-color all of them, then, to not even risk that situation. Ciao, Dscho
