Re: [PATCH 5/8] diff-highlight: use test_decode_color in tests

[Tian Yuchen <a3205153416@gmail.com>]

On 3/23/26 04:47, Jeff King wrote:
> On Mon, Mar 23, 2026 at 01:24:00AM +0800, Tian Yuchen wrote:
> 
>>> @@ -42,9 +39,9 @@ dh_test () {
>>>    	} >/dev/null &&
>>>    	"$DIFF_HIGHLIGHT" <diff.raw >diff.hi &&
>>> -	test_strip_patch_header <diff.hi >diff.act
>>> +	test_strip_patch_header <diff.hi | test_decode_color >diff.act
>>
>> Although this is just simple text filtering and leaving it as is wouldn’t
>> cause any problems IMO, why not go ahead and add the && while you’re at it?
> 
> The bug is in an earlier commit (patch 3), which breaks apart the pipe
> but doesn't add the necessary &&. And it's more than just text
> filtering; it breaks the &&-chain, so we miss the exit code of
> $DIFF_HIGHLIGHT (which was the whole point of patch 3).
> 
> chainlint doesn't find it because we're in a helper function, not
> directinly in a test snippet.
> 
> I'll send a revised series to fix it, but...
> 
>> I've noticed that there are several missing &&.
> 
> Where else do you see?
> 
> Or do you mean that we should not pipe text filtering commands? There
> I'd disagree. We are not likely to see a failure from 'sed', and if we
> do, the fact that the output does not match would catch it. And the cost
> of breaking every command down without pipes means having to manage lots
> of intermediate files.
> 
> -Peff

Oh, I see.

To be honest, I didn't think about it in that much detail. I just 
noticed this tiny issue and wanted to take the opportunity to remind you 
to check other parts as well. I'm not saying we should remove those pipe 
commands :P

Thanks for the explanation about chainlint.

Regards, Yuchen