Hi Peff, On Thu, 12 Nov 2020, Jeff King wrote: > 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) It's a little bit of a surprise that this is the first time I hear about this. The way _I_ would go about fixing this is to look for a tell-tale that we're looking at a `diff-so-fancy` style output, e.g. by looking for that horizontal line, then adding special code to handle that. This is not a minor undertaking, and it would currently require _two_ implementations: the Perl version and the built-in version. They would look _very_ different from one another. Therefore, I would probably either wait until the Perl version is retired, or selectively only adjust the built-in version, if _I_ was to implement this. But given that it does not work right now, and given that it has not been working for a long time, I think it does not hurt so much if it is left in the current state for a bit longer. I would love to focus on the patch series that kicked off this discussion first, getting it done, before I think about other `add -p` aspects. Ciao, Dscho > > > > 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 >
