| Message ID | febcfb0a-c410-fb71-cff9-92acfcb269e2@kdbg.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 500317ae03f635b247627eeb9760d9de2e343875 |
| Headers | show |
| Series | t3920: don't ignore errors of more than one command with `|| true` | expand |
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) &&
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.
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.
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).
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
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.
Æ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".
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) &&
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(-)