Re: [PATCH v2 00/11] Fix color handling in git add -i
flat view
From: Johannes Schindelin <hidden>
Date: 2020-11-16 12:38:18
Hi Peff, On Thu, 12 Nov 2020, Jeff King wrote:
On Thu, Nov 12, 2020 at 03:20:38PM +0100, Johannes Schindelin wrote:quoted
quoted
$ 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
quoted
quoted
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