git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] t3920: don't ignore errors of more than one command with `|| true`
@ 2022-11-21 17:58 Johannes Sixt
  2022-11-21 22:56 ` René Scharfe
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Johannes Sixt @ 2022-11-21 17:58 UTC (permalink / raw)
  To: Philippe Blain; +Cc: René Scharfe, Git Mailing List

It is customary to write `A || true` to ignore a potential error exit of
command A. But when we have a sequence `A && B && C || true && D`, then
a failure of any of A, B, or C skips to D right away. This is not
intended here. Turn the command whose failure is to be ignored into a
compound command to ensure it is the only one that is allowed to fail.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t3920-crlf-messages.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh
index 4c661d4d54..a58522c163 100755
--- a/t/t3920-crlf-messages.sh
+++ b/t/t3920-crlf-messages.sh
@@ -12,7 +12,7 @@ create_crlf_ref () {
 	cat >.crlf-orig-$branch.txt &&
 	cat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&
 	grep 'Subject' .crlf-orig-$branch.txt | tr '\n' ' ' | sed 's/[ ]*$//' | tr -d '\n' >.crlf-subject-$branch.txt &&
-	grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&
+	{ grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true; } &&
 	LIB_CRLF_BRANCHES="${LIB_CRLF_BRANCHES} ${branch}" &&
 	test_tick &&
 	hash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&
-- 
2.36.0.137.geb37740430

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] t3920: don't ignore errors of more than one command with `|| true`
  2022-11-21 17:58 [PATCH] t3920: don't ignore errors of more than one command with `|| true` Johannes Sixt
@ 2022-11-21 22:56 ` René Scharfe
  2022-11-22  0:53   ` Junio C Hamano
  2022-11-22 18:28 ` Philippe Blain
  2022-11-22 22:24 ` Ævar Arnfjörð Bjarmason
  2 siblings, 1 reply; 8+ messages in thread
From: René Scharfe @ 2022-11-21 22:56 UTC (permalink / raw)
  To: Johannes Sixt, Philippe Blain; +Cc: Git Mailing List

Am 21.11.22 um 18:58 schrieb Johannes Sixt:
> It is customary to write `A || true` to ignore a potential error exit of
> command A. But when we have a sequence `A && B && C || true && D`, then
> a failure of any of A, B, or C skips to D right away. This is not
> intended here. Turn the command whose failure is to be ignored into a
> compound command to ensure it is the only one that is allowed to fail.

Good catch!

> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  t/t3920-crlf-messages.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh
> index 4c661d4d54..a58522c163 100755
> --- a/t/t3920-crlf-messages.sh
> +++ b/t/t3920-crlf-messages.sh
> @@ -12,7 +12,7 @@ create_crlf_ref () {
>  	cat >.crlf-orig-$branch.txt &&
>  	cat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&

Useless use of cat.

>  	grep 'Subject' .crlf-orig-$branch.txt | tr '\n' ' ' | sed 's/[ ]*$//' | tr -d '\n' >.crlf-subject-$branch.txt &&

My knee-jerk reaction to long lines like this is to pull out awk:

	awk '/Subject/ {printf "%s", sep $0; sep = " "}' .crlf-orig-$branch.txt >.crlf-subject-$branch.txt &&

This is not a faithful conversion because the original trims all
spaces from the end of the subject for some reason.  That would be:

	awk '/Subject/ {s = s $0 " "} END {sub(/ *$/, "", s); printf "%s", s}' .crlf-orig-$branch.txt >.crlf-subject-$branch.txt &&

> -	grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&
> +	{ grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true; } &&

OK, back on topic: Adding CRs explicitly instead of relying on grep to
pass them through (which it doesn't on MinGW) also ignores the return
value of grep as a side effect of the pipe:

	grep 'Body' .crlf-orig-$branch.txt | append_cr >.crlf-body-$branch.txt &&

>  	LIB_CRLF_BRANCHES="${LIB_CRLF_BRANCHES} ${branch}" &&
>  	test_tick &&
>  	hash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] t3920: don't ignore errors of more than one command with `|| true`
  2022-11-21 22:56 ` René Scharfe
@ 2022-11-22  0:53   ` Junio C Hamano
  0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2022-11-22  0:53 UTC (permalink / raw)
  To: René Scharfe; +Cc: Johannes Sixt, Philippe Blain, Git Mailing List

René Scharfe <l.s.r@web.de> writes:

>> @@ -12,7 +12,7 @@ create_crlf_ref () {
>>  	cat >.crlf-orig-$branch.txt &&
>>  	cat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&
>
> Useless use of cat.

Very good eyes.

> OK, back on topic: Adding CRs explicitly instead of relying on grep to
> pass them through (which it doesn't on MinGW) also ignores the return
> value of grep as a side effect of the pipe:
>
> 	grep 'Body' .crlf-orig-$branch.txt | append_cr >.crlf-body-$branch.txt &&

Very nice.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] t3920: don't ignore errors of more than one command with `|| true`
  2022-11-21 17:58 [PATCH] t3920: don't ignore errors of more than one command with `|| true` Johannes Sixt
  2022-11-21 22:56 ` René Scharfe
@ 2022-11-22 18:28 ` Philippe Blain
  2022-11-22 22:24 ` Ævar Arnfjörð Bjarmason
  2 siblings, 0 replies; 8+ messages in thread
From: Philippe Blain @ 2022-11-22 18:28 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: René Scharfe, Git Mailing List

Hi Johannes,

Le 2022-11-21 à 12:58, Johannes Sixt a écrit :
> It is customary to write `A || true` to ignore a potential error exit of
> command A. But when we have a sequence `A && B && C || true && D`, then
> a failure of any of A, B, or C skips to D right away. This is not
> intended here. Turn the command whose failure is to be ignored into a
> compound command to ensure it is the only one that is allowed to fail.
> 
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  t/t3920-crlf-messages.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh
> index 4c661d4d54..a58522c163 100755
> --- a/t/t3920-crlf-messages.sh
> +++ b/t/t3920-crlf-messages.sh
> @@ -12,7 +12,7 @@ create_crlf_ref () {
>  	cat >.crlf-orig-$branch.txt &&
>  	cat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&
>  	grep 'Subject' .crlf-orig-$branch.txt | tr '\n' ' ' | sed 's/[ ]*$//' | tr -d '\n' >.crlf-subject-$branch.txt &&
> -	grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&
> +	{ grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true; } &&
>  	LIB_CRLF_BRANCHES="${LIB_CRLF_BRANCHES} ${branch}" &&
>  	test_tick &&
>  	hash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&
> 

Thank you for making that function more robust.

Philippe.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] t3920: don't ignore errors of more than one command with `|| true`
  2022-11-21 17:58 [PATCH] t3920: don't ignore errors of more than one command with `|| true` Johannes Sixt
  2022-11-21 22:56 ` René Scharfe
  2022-11-22 18:28 ` Philippe Blain
@ 2022-11-22 22:24 ` Ævar Arnfjörð Bjarmason
  2022-11-22 22:37   ` Johannes Sixt
  2 siblings, 1 reply; 8+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-22 22:24 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Philippe Blain, René Scharfe, Git Mailing List


On Mon, Nov 21 2022, Johannes Sixt wrote:

> It is customary to write `A || true` to ignore a potential error exit of
> command A. But when we have a sequence `A && B && C || true && D`, then
> a failure of any of A, B, or C skips to D right away. This is not
> intended here. Turn the command whose failure is to be ignored into a
> compound command to ensure it is the only one that is allowed to fail.

> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  t/t3920-crlf-messages.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh
> index 4c661d4d54..a58522c163 100755
> --- a/t/t3920-crlf-messages.sh
> +++ b/t/t3920-crlf-messages.sh
> @@ -12,7 +12,7 @@ create_crlf_ref () {
>  	cat >.crlf-orig-$branch.txt &&
>  	cat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&
>  	grep 'Subject' .crlf-orig-$branch.txt | tr '\n' ' ' | sed 's/[ ]*$//' | tr -d '\n' >.crlf-subject-$branch.txt &&
> -	grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&
> +	{ grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true; } &&
>  	LIB_CRLF_BRANCHES="${LIB_CRLF_BRANCHES} ${branch}" &&
>  	test_tick &&
>  	hash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&

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 &&

?

We usually use that for "grep when we don't care about the exit
code". But maybe some CRLF concerns in this code don't allow it (I only
tested this on *nix).

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] t3920: don't ignore errors of more than one command with `|| true`
  2022-11-22 22:24 ` Ævar Arnfjörð Bjarmason
@ 2022-11-22 22:37   ` Johannes Sixt
  2022-11-22 22:57     ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 8+ messages in thread
From: Johannes Sixt @ 2022-11-22 22:37 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Philippe Blain, René Scharfe, Git Mailing List

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.

-- Hannes


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] t3920: don't ignore errors of more than one command with `|| true`
  2022-11-22 22:37   ` Johannes Sixt
@ 2022-11-22 22:57     ` Ævar Arnfjörð Bjarmason
  2022-11-23  0:55       ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-22 22:57 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Philippe Blain, René Scharfe, Git Mailing List


On Tue, Nov 22 2022, Johannes Sixt wrote:

> 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.




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] t3920: don't ignore errors of more than one command with `|| true`
  2022-11-22 22:57     ` Ævar Arnfjörð Bjarmason
@ 2022-11-23  0:55       ` Junio C Hamano
  0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2022-11-23  0:55 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Johannes Sixt, Philippe Blain, René Scharfe, Git Mailing List

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> 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.

Let's leave all of the above (mostly good ideas) for "after the dust
settles" clean-up.

If sed is much slower than grep (for such a small string use case,
start-up cost of a process would dwarf everybody else), "grep || :"
might be justifiable, but other than that I do not think of a good
reason why we might favor "grep || :" offhand over "sed -ne //p".




^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-11-23  0:55 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-21 17:58 [PATCH] t3920: don't ignore errors of more than one command with `|| true` Johannes Sixt
2022-11-21 22:56 ` René Scharfe
2022-11-22  0:53   ` Junio C Hamano
2022-11-22 18:28 ` Philippe Blain
2022-11-22 22:24 ` Ævar Arnfjörð Bjarmason
2022-11-22 22:37   ` Johannes Sixt
2022-11-22 22:57     ` Ævar Arnfjörð Bjarmason
2022-11-23  0:55       ` Junio C Hamano

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).