Re: [PATCH] t3920: don't ignore errors of more than one command with `|| true`

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 22, 2022, 22:57 UTC
Message-ID: <221123.86ilj6a8ux.gmgdl@evledraar.gmail.com>
In-Reply-To: <271dd9e6-3575-a46f-4c92-fa202aaf95d7@kdbg.org>

On Tue, Nov 22 2022, Johannes Sixt wrote:

Show 13 quoted lines

> Am 22.11.22 um 23:24 schrieb Ævar Arnfjörð Bjarmason:
>> On Mon, Nov 21 2022, Johannes Sixt wrote:
>>> -	grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&
>>> +	{ grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true; } &&
>> 
>> Any reason not to make this:
>> 
>> 	-       grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&
>> 	+       sed -ne '/Body/p' <.crlf-message-$branch.txt >.crlf-body-$branch.txt &&
>> 
>> ?
>
> Yes: I have tested my version, but not this one.

I don't find that too surprising, as unless there's a discussion of "I tried xyz, but it didn't work, so..." a submitted patch is unlikely to have tried various alternatives for such a trivial fix, but just gone with whatever the author tried first.

But in case it wasn't clear, the implied suggestion is that unless there is a good reason to introduce a pattern of:

	 { <cmd> in >out || true; } &&

That we should use another suitable command with the simpler use of:

	 <cmd2> <in >out &&

If the result is equivalent, as subsequent maintainers won't need to puzzle over the seemingly odd pattern.

We have that "sed -n -e" in somewhat wide use:

	$ git grep 'sed (-n -e|-ne).*/p.*&&' | wc -l
	54

The only existing occurance of this "grep but ignore the exit code" I could find was:

	t/t9001-send-email.sh:  { grep '^X-Mailer:' out || :; } >mailer &&

For which we can also:

	-       { grep '^X-Mailer:' out || :; } >mailer &&
	+       sed -ne '/^X-Mailer:/p' <out >mailer &&

And which I'd think would be great to have in a re-roll, i.e. "here's these two cases where a grep-but-dont-care-about-the-exit-code could be replaced by sed -ne".

But if re-testing this is tedious etc. I don't mind if it goes in as-is, but then I'd think we could safely emulate the t9001-send-email.sh pattern of using ":" instead of "true"; we don't need to invoke another process just to ignore the exit code.