Re: [PATCH 0/3] built-in add -p: support diff-so-fancy better

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 24, 2022, 18:24 UTC
Message-ID: <xmqqmtbt7b5p.fsf@gitster.g>
In-Reply-To: <145ddce5-8084-a024-39b5-9dea870d1afe@gmail.com>

Philippe Blain <levraiphilippeblain@gmail.com> writes:

Show 25 quoted lines

>>>> However, I've tried it on a more "real-life" example, and then I get:
>>>>
>>>>     error: mismatched output from interactive.diffFilter
>>>>     hint: Your filter must maintain a one-to-one correspondence
>>>>     hint: between its input and output lines.
>>>>
>>>> This is despite using "diff-so-fancy --patch" as interactive.diffFilter, which should
>>>> keep the number of lines the same.
>>>
>>> Would you mind sharing the example with me?
>>>
>>> Thanks,
>>> Dscho
>> 
>> In trying to write a reproducer, I realize that this is in fact a separate "bug".
>> I happened to have a submodule in the repository I was testing, and it had modified 
>> content. This is what was causing the "mismatched output" error, although I'm not
>> sure why. If I use a pathspec that does not include the submodule when invoking 
>> 'git add -p', it works correctly. 
>> 
>> I'm a bit out of time now but I'll try to write a separate
>> reproducer for this later today.
>
> So in trying to write a new reproducer, I found the cause of the
> bug. ...

I somehow had an impression that that the C reimplementation was designed to be a bug-for-bug compatible faithful rewrite of the original scripted version, but looking at the way how this bug is handled, I am not sure (it looks as if the initial fix was to patch the code to match the symptom, not to make the code to behave more faithfully to the scripted version). As with any big rewrite, a complete re-implementation always has risks to introduce unintended regressions and that is why starting with faithful rewrite with the staged transition plan is valuable.

In this case, the staged transition plan, the step to flip the default without before remove the old one, is working beautifully. And it was only made possible with the actual users reporting issues before they say "the new one is broken, so let's use the knob to retain the old implementation". Please thank those in diff-so-fancy land for reporting the issue.

Thanks, you two, for working on this.