On Thu, Nov 12, 2020 at 03:20:38PM +0100, Johannes Schindelin wrote: > > $ 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`): Yes, there are definitely problems with how diff-so-fancy represents things (because they don't maintain a 1-to-1 correspondence). You should generally get a complaint like: $ git -c interactive.difffilter='diff-so-fancy' add -p fatal: mismatched output from interactive.diffFilter hint: Your filter must maintain a one-to-one correspondence hint: between its input and output lines. The diff-so-fancy folks have asked me about this, and I've made clear the problem to them, and that the solution is to have a mode which maintains the line correspondence. AFAIK there's no fix yet. I don't know how many people are actually using it in practice in its current buggy state. There's a big thread here: https://github.com/so-fancy/diff-so-fancy/issues/35 I don't really recommend reading the whole thing. Beyond what I just said, the interesting bits are: - people recommend using the "delta" tool; it has a "--color-only" option which does just diff-highlight-style coloring, but doesn't break line correspondence - somebody suggested instead of using interactive.difffilter, to add an option to show each hunk in an individual pager command. So add-interactive would never see the "fancy" version at all, but it would be generated on the fly when the user sees it (and that filter would have to be able to handle seeing individual hunks without the diff header). All of which is to say that the current state is a bit of a mess, and I don't consider it completely necessary to fix it before the builtin version becomes the default. But: - I think we should expect some possible complaints / bug reports from fringe users of the already-somewhat-broken state - IMHO the parsing of the filtered version done by the builtin is going in the wrong direction (see my other mail for an elaboration) > > 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. Yeah, obviously calling die() in the split case is bad. And the fact that newly-created split hunk headers are not filtered the same way as the original hunk headers isn't ideal. But it's a pretty easy fix in the perl version, and not in the builtin version. -Peff
