Re: js/add-p-diff-parsing-fix (was: What's cooking in git.git (Aug 2022, #10; Tue, 30))

[Phillip Wood <phillip.wood123@gmail.com>]

On 31/08/2022 09:29, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Aug 30 2022, Junio C Hamano wrote:
> 
>> * js/add-p-diff-parsing-fix (2022-08-29) 5 commits
>>   - add -p: ignore dirty submodules
>>   - add -p: handle `diff-so-fancy`'s hunk headers better
>>   - add -p: insert space in colored hunk header as needed
>>   - add -p: gracefully ignore unparseable hunk headers in colored diffs
>>   - t3701: redefine what is "bogus" output of a diff filter
>>
>>   Those who use diff-so-fancy as the diff-filter noticed a regression
>>   or two in the reimplemented "add -p", which has been corrected.
>>
>>   Will merge to 'next'?
>>   source: <pull.1336.v3.git.1661785916.gitgitgadget@gmail.com>
> 
> This LGTM.

I'm not sure that the different output from the builtin version compared 
to the perl version when using interactive.diffFilter is an improvement. 
The perl version leaves the filtered output alone but the builtin 
version has started prepending @@ -a,b +d,c @@ to the "hunk header" 
lines produced by the filter.

Best Wishes

Phillip

> For the release notes you might want to tweak this. While this series
> fixes an issue that some users ran into with "diff-so-fancy" it's really
> just a generic set of fixes for "git add -p". The issue just happens to
> have been discovered with that tool.
> 
> E.g. on master now you can try this in git.git:
> 
> 	echo hi >README.mda
> 	./git -c interactive.diffFilter=tac add -p
> 	error: could not parse colored hunk header '?[31m-[Documentation/gitcvs-migration.txt]: Documentation/gitcvs-migration.txt?[m'
> 
> I.e. just reversing the output with tac(1) is enough to trigger the
> failure fixed in this series.
> 
> Maybe?:
> 
> 	'git-add -p' can now parse diffs that don't contain the line
> 	information that "git diff" itself would emit within "@@
> 	... @@". As a result the reimplemented "add -p" can now be used
> 	with the popular "diff-so-fancy" utility.
>