[PATCH 0/2] Make cvs tests pass with '-x' tracing and /bin/sh

[PATCH 0/2] Make cvs tests pass with '-x' tracing and /bin/sh

[SZEDER Gábor]

> On Sat, Feb 24, 2018 at 12:39 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > This patch series makes '-x' tracing of tests work reliably even when
> > running them with /bin/sh, and setting TEST_SHELL_PATH=bash will be
> > unnecessary.
> >
> >   make GIT_TEST_OPTS='-x --verbose-log' test
> >
> > passes on my setup and on all Travis CI build jobs (though neither me
> > nor Travis CI run all tests, e.g. CVS).
> 
> I installed 'cvs' and whatnot to run t94* and t96* tests, and sure
> enough, 5 tests in 2 test scripts fail with '-x' tracing and /bin/sh.
> I think I will be able to get around to send v2 during the weekend.

Well, I wasn't able to get around to it, and in the meantime
'sg/test-x' got merged into 'next'.  So here are the updates for those
two test scripts.

The commit message of the first patch is perhaps unnecessary long, but
it's mostly about explaining why the affected test was working
reasonably well despite the rather weird 'test_cmp this that >>out
2>&1' thing.

SZEDER Gábor (2):
  t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'
  t9402-git-cvsserver-refs: don't check the stderr of a subshell

 t/t9400-git-cvsserver-server.sh | 15 +++++++++------
 t/t9402-git-cvsserver-refs.sh   |  8 ++++----
 2 files changed, 13 insertions(+), 10 deletions(-)

-- 
2.16.2.603.g180c1428f0

[PATCH 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'

[SZEDER Gábor]

The test 'cvs update (-p)' redirects and checks 'test_cmp's stdout and
even its stderr.  The commit introducing this test in 6e8937a084
(cvsserver: Add test for update -p, 2008-03-27) doesn't discuss why,
in fact its log message only consists of that subject line.  Anyway,
weird as it is, it kind of made sense due to the way that test was
structured:

After a bit of preparation, this test updates four files via CVS and
checks their contents using 'test_cmp', but it does so in a for loop
iterating over the names of those four files.  Now, the exit status of
a for loop is the exit status of the last command executed in the
loop, meaning that the test can't simply rely on the exit code of
'test_cmp' in the loop's body.  Instead, the test works it around by
relying on the stdout of 'test_cmp' being silent on success and
showing the diff on failure, as it appends the stdout of all four
'test_cmp' invocations to a single file and checks that file's
emptiness after the loop (with 'test -z "$(cat ...)"', no less; there
was no 'test_must_be_empty' back then).  Furthermore, the test
redirects the stderr of those 'test_cmp' invocations to this file,
too: while 'test_cmp' itself doesn't output anything to stderr, the
invoked 'diff' or 'cmp' commands do send their error messages there,
e.g. if they can't open a file because its name was misspelled.

This also makes this test fail when the test script is run with '-x'
tracing (and using a shell other than a Bash version supporting
BASH_XTRACEFD), because 'test_cmp's stderr contains the trace of the
'diff' command executed inside the helper function, throwing off the
subsequent emptiness check.

Unroll that for loop, so we can check the files' contents the usual
way and rely on 'test_cmp's exit code failing the && chain.  Extract
updating a file via CVS and verifying its contents using 'test_cmp'
into a helper function requiring the file's name as parameter to avoid
much of the repetition resulting from unrolling the loop.

After this change t9400 passes with '-x', even when running with
/bin/sh.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t9400-git-cvsserver-server.sh | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
index c30660d606..1f67d2822f 100755
--- a/t/t9400-git-cvsserver-server.sh
+++ b/t/t9400-git-cvsserver-server.sh
@@ -437,6 +437,11 @@ test_expect_success 'cvs update (merge no-op)' \
     GIT_CONFIG="$git_config" cvs -Q update &&
     test_cmp merge ../merge'
 
+check_cvs_update_p () {
+    GIT_CONFIG="$git_config" cvs update -p "$1" >"$1".out &&
+    test_cmp "$1".out ../"$1"
+}
+
 cd "$WORKDIR"
 test_expect_success 'cvs update (-p)' '
     touch really-empty &&
@@ -447,12 +452,10 @@ test_expect_success 'cvs update (-p)' '
     git push gitcvs.git >/dev/null &&
     cd cvswork &&
     GIT_CONFIG="$git_config" cvs update &&
-    rm -f failures &&
-    for i in merge no-lf empty really-empty; do
-        GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out
-	test_cmp $i.out ../$i >>failures 2>&1
-    done &&
-    test -z "$(cat failures)"
+    check_cvs_update_p merge &&
+    check_cvs_update_p no-lf &&
+    check_cvs_update_p empty &&
+    check_cvs_update_p really-empty
 '
 
 cd "$WORKDIR"
-- 
2.16.2.603.g180c1428f0

Re: [PATCH 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'

[Eric Sunshine]

On Thu, Mar 8, 2018 at 7:38 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> The test 'cvs update (-p)' redirects and checks 'test_cmp's stdout and
> even its stderr.  The commit introducing this test in 6e8937a084
> (cvsserver: Add test for update -p, 2008-03-27) doesn't discuss why,
> in fact its log message only consists of that subject line.  Anyway,
> weird as it is, it kind of made sense due to the way that test was
> structured:

[excellent explanation snipped]

> Unroll that for loop, so we can check the files' contents the usual
> way and rely on 'test_cmp's exit code failing the && chain.  Extract
> updating a file via CVS and verifying its contents using 'test_cmp'
> into a helper function requiring the file's name as parameter to avoid
> much of the repetition resulting from unrolling the loop.

An alternative approach used elsewhere in the test suite[1] would be
simply to 'exit' if test_cmp fails:

    for i in merge no-lf empty really-empty; do
        GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out
        test_cmp $i.out ../$i || exit 1
    done &&

(And, like the existing patch, this removes the need for capturing
test_cmp's output into a "failures" file.)

[1]: For example, the "setup" test of t2204-add-ignored.sh.

> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
> @@ -447,12 +452,10 @@ test_expect_success 'cvs update (-p)' '
>      git push gitcvs.git >/dev/null &&
>      cd cvswork &&
>      GIT_CONFIG="$git_config" cvs update &&
> -    rm -f failures &&
> -    for i in merge no-lf empty really-empty; do
> -        GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out
> -       test_cmp $i.out ../$i >>failures 2>&1
> -    done &&
> -    test -z "$(cat failures)"
> +    check_cvs_update_p merge &&
> +    check_cvs_update_p no-lf &&
> +    check_cvs_update_p empty &&
> +    check_cvs_update_p really-empty
>  '

Re: [PATCH 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'

[SZEDER Gábor]

On Thu, Mar 8, 2018 at 7:51 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Mar 8, 2018 at 7:38 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:

>> Unroll that for loop, so we can check the files' contents the usual
>> way and rely on 'test_cmp's exit code failing the && chain.  Extract
>> updating a file via CVS and verifying its contents using 'test_cmp'
>> into a helper function requiring the file's name as parameter to avoid
>> much of the repetition resulting from unrolling the loop.
>
> An alternative approach used elsewhere in the test suite[1] would be
> simply to 'exit' if test_cmp fails:
>
>     for i in merge no-lf empty really-empty; do
>         GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out
>         test_cmp $i.out ../$i || exit 1
>     done &&
>
> (And, like the existing patch, this removes the need for capturing
> test_cmp's output into a "failures" file.)
>
> [1]: For example, the "setup" test of t2204-add-ignored.sh.

But 't/README' sayeth:

    Don't:

     - exit() within a <script> part.

And it's right: 'exit' terminates the shell process it's invoked in,
i.e. the whole test script (well, unless it's invoked in a subshell)
without executing the remaining tests and the usual housekeeping and
reporting.

Consider the following test script:

  -- >8 --

#!/bin/sh

test_description='exit 1?'

. ./test-lib.sh

test_expect_success 'exit 1' '
        exit 1
'

test_expect_success 'second test' '
        true
'

test_done

  -- >8 --

It's output:

  $ ./t9999-exit.sh
  FATAL: Unexpected exit with code 1
  $ sed -e 's/exit 1/false/' -i t9999-exit.sh
  $ ./t9999-exit.sh not ok 1 - false
  #
  #             false
  #
  ok 2 - second test
  # failed 1 among 2 test(s)
  1..2

Re: [PATCH 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'

[Eric Sunshine]

On Thu, Mar 8, 2018 at 4:49 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Thu, Mar 8, 2018 at 7:51 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> An alternative approach used elsewhere in the test suite[1] would be
>> simply to 'exit' if test_cmp fails:
>>
>>     for i in merge no-lf empty really-empty; do
>>         GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out
>>         test_cmp $i.out ../$i || exit 1
>>     done &&
>
> And it's right: 'exit' terminates the shell process it's invoked in,
> i.e. the whole test script (well, unless it's invoked in a subshell)
> without executing the remaining tests and the usual housekeeping and
> reporting.
>
> Consider the following test script:
>
>   $ ./t9999-exit.sh
>   FATAL: Unexpected exit with code 1

Sorry for the confusion. I meant "return 1" as used elsewhere in the
test suite[1].

--- >8 ---
#!/bin/sh
test_description='return 1?'
. ./test-lib.sh

test_expect_success 'return 1' '
        return 1
'

test_expect_success 'second test' '
        true
'

test_done
--- >8 ---

$ ./t9977.sh
not ok 1 - return 1
#
#         return 1
#
ok 2 - second test
# failed 1 among 2 test(s)
1..2
$

[1]: For example, the "setup" test of t4151-am-abort.sh.

Re: [PATCH 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'

[Eric Sunshine]

On Thu, Mar 8, 2018 at 5:01 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> Sorry for the confusion. I meant "return 1" as used elsewhere in the
> test suite[1].
> [1]: For example, the "setup" test of t4151-am-abort.sh.

Additional context: e6821d09e4 (t: fix some trivial cases of ignored
exit codes in loops, 2015-03-25)

Re: [PATCH 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'

[SZEDER Gábor]

On Thu, Mar 8, 2018 at 11:01 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Mar 8, 2018 at 4:49 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>> On Thu, Mar 8, 2018 at 7:51 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> An alternative approach used elsewhere in the test suite[1] would be
>>> simply to 'exit' if test_cmp fails:
>>>
>>>     for i in merge no-lf empty really-empty; do
>>>         GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out
>>>         test_cmp $i.out ../$i || exit 1
>>>     done &&

> Sorry for the confusion. I meant "return 1" as used elsewhere in the
> test suite[1].

Oh, I see, that's indeed better.  Thanks.

[PATCH v1.1 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'

[SZEDER Gábor]

The test 'cvs update (-p)' redirects and checks 'test_cmp's stdout and
even its stderr.  The commit introducing this test in 6e8937a084
(cvsserver: Add test for update -p, 2008-03-27) doesn't discuss why,
in fact its log message only consists of that subject line.  Anyway,
weird as it is, it kind of made sense due to the way that test was
structured:

After a bit of preparation, this test updates four files via CVS and
checks their contents using 'test_cmp', but it does so in a for loop
iterating over the names of those four files.  Now, the exit status of
a for loop is the exit status of the last command executed in the
loop, meaning that the test can't simply rely on the exit code of
'test_cmp' in the loop's body.  Instead, the test works it around by
relying on the stdout of 'test_cmp' being silent on success and
showing the diff on failure, as it appends the stdout of all four
'test_cmp' invocations to a single file and checks that file's
emptiness after the loop (with 'test -z "$(cat ...)"', no less; there
was no 'test_must_be_empty' back then).  Furthermore, the test
redirects the stderr of those 'test_cmp' invocations to this file,
too: while 'test_cmp' itself doesn't output anything to stderr, the
invoked 'diff' or 'cmp' commands do send their error messages there,
e.g. if they can't open a file because its name was misspelled.

This also makes this test fail when the test script is run with '-x'
tracing (and using a shell other than a Bash version supporting
BASH_XTRACEFD), because 'test_cmp's stderr contains the trace of the
'diff' command executed inside the helper function, throwing off the
subsequent emptiness check.

Stop relying on 'test_cmp's output and instead run 'test_cmp a b ||
return 1' in the for loop in order to make 'test_cmp's error code fail
the test.  Furthermore, add the missing && after the cvs command to
create a && chain in the loop's body.

After this change t9400 passes with '-x', even when running with
/bin/sh.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---

Changes:
Use Eric's suggestion and run 'test_cmp a b || return 1' in the for
loop to fail the test.

 t/t9400-git-cvsserver-server.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
index c30660d606..5ff3789199 100755
--- a/t/t9400-git-cvsserver-server.sh
+++ b/t/t9400-git-cvsserver-server.sh
@@ -449,10 +449,9 @@ test_expect_success 'cvs update (-p)' '
     GIT_CONFIG="$git_config" cvs update &&
     rm -f failures &&
     for i in merge no-lf empty really-empty; do
-        GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out
-	test_cmp $i.out ../$i >>failures 2>&1
-    done &&
-    test -z "$(cat failures)"
+	GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out &&
+	test_cmp $i.out ../$i || return 1
+    done
 '
 
 cd "$WORKDIR"
-- 
2.16.2.603.g180c1428f0

Re: [PATCH v1.1 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
> index c30660d606..5ff3789199 100755
> --- a/t/t9400-git-cvsserver-server.sh
> +++ b/t/t9400-git-cvsserver-server.sh
> @@ -449,10 +449,9 @@ test_expect_success 'cvs update (-p)' '
>      GIT_CONFIG="$git_config" cvs update &&
>      rm -f failures &&
>      for i in merge no-lf empty really-empty; do
> -        GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out
> -	test_cmp $i.out ../$i >>failures 2>&1
> -    done &&
> -    test -z "$(cat failures)"
> +	GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out &&
> +	test_cmp $i.out ../$i || return 1
> +    done
>  '

This makes "rm -f failures &&" unnecessary, no?

Re: [PATCH v1.1 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'

[SZEDER Gábor]

On Fri, Mar 9, 2018 at 12:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
>> index c30660d606..5ff3789199 100755
>> --- a/t/t9400-git-cvsserver-server.sh
>> +++ b/t/t9400-git-cvsserver-server.sh
>> @@ -449,10 +449,9 @@ test_expect_success 'cvs update (-p)' '
>>      GIT_CONFIG="$git_config" cvs update &&
>>      rm -f failures &&
>>      for i in merge no-lf empty really-empty; do
>> -        GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out
>> -     test_cmp $i.out ../$i >>failures 2>&1
>> -    done &&
>> -    test -z "$(cat failures)"
>> +     GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out &&
>> +     test_cmp $i.out ../$i || return 1
>> +    done
>>  '
>
> This makes "rm -f failures &&" unnecessary, no?

Indeed, it does.

Re: [PATCH v1.1 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Fri, Mar 9, 2018 at 12:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> SZEDER Gábor <szeder.dev@gmail.com> writes:
>>
>>> diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
>>> index c30660d606..5ff3789199 100755
>>> --- a/t/t9400-git-cvsserver-server.sh
>>> +++ b/t/t9400-git-cvsserver-server.sh
>>> @@ -449,10 +449,9 @@ test_expect_success 'cvs update (-p)' '
>>>      GIT_CONFIG="$git_config" cvs update &&
>>>      rm -f failures &&
>>>      for i in merge no-lf empty really-empty; do
>>> -        GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out
>>> -     test_cmp $i.out ../$i >>failures 2>&1
>>> -    done &&
>>> -    test -z "$(cat failures)"
>>> +     GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out &&
>>> +     test_cmp $i.out ../$i || return 1
>>> +    done
>>>  '
>>
>> This makes "rm -f failures &&" unnecessary, no?
>
> Indeed, it does.

OK, no need to resend, as I'll amend it locally before queuing
(unless there is something else that needs updating, that is).

Thanks.

Re: [PATCH v1.1 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'

[SZEDER Gábor]

On Fri, Mar 9, 2018 at 12:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:

>>> This makes "rm -f failures &&" unnecessary, no?
>>
>> Indeed, it does.
>
> OK, no need to resend, as I'll amend it locally before queuing
> (unless there is something else that needs updating, that is).

Thanks.

That 'rm' was unnecessary to begin with, because none of the previous
tests wrote and left behind a file 'failures', ever.  Though one could
argue that the original author was careful and added that 'rm failures'
as a protection against future changes to previous tests...  Dunno, we
are talking about a test that checked the stderr of 'test_cmp', so
anything is possible :)

[PATCH 2/2] t9402-git-cvsserver-refs: don't check the stderr of a subshell

[SZEDER Gábor]

Four 'cvs diff' related tests in 't9402-git-cvsserver-refs.sh' fail
when the test script is run with '-x' tracing (and using a shell other
than a Bash version supporting BASH_XTRACEFD).  The reason for those
failures is that the tests check the emptiness of a subshell's stderr,
which includes the trace of commands executed in that subshell as
well, throwing off the emptiness check.

Save the stdout and stderr of the invoked 'cvs' command instead of the
whole subshell, so the latter remains free from tracing output.  (Note
that changing how stdout is saved is only done for the sake of
consistency, it's not necessary for correctness.)

After this change t9402 passes with '-x', even when running with
/bin/sh.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t9402-git-cvsserver-refs.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t9402-git-cvsserver-refs.sh b/t/t9402-git-cvsserver-refs.sh
index 6d2d3c8739..cf31ace667 100755
--- a/t/t9402-git-cvsserver-refs.sh
+++ b/t/t9402-git-cvsserver-refs.sh
@@ -455,20 +455,20 @@ test_expect_success 'cvs up -r $(git rev-parse v1)' '
 '
 
 test_expect_success 'cvs diff -r v1 -u' '
-	( cd cvswork && cvs -f diff -r v1 -u ) >cvsDiff.out 2>cvs.log &&
+	( cd cvswork && cvs -f diff -r v1 -u >../cvsDiff.out 2>../cvs.log ) &&
 	test_must_be_empty cvsDiff.out &&
 	test_must_be_empty cvs.log
 '
 
 test_expect_success 'cvs diff -N -r v2 -u' '
-	( cd cvswork && ! cvs -f diff -N -r v2 -u ) >cvsDiff.out 2>cvs.log &&
+	( cd cvswork && ! cvs -f diff -N -r v2 -u >../cvsDiff.out 2>../cvs.log ) &&
 	test_must_be_empty cvs.log &&
 	test -s cvsDiff.out &&
 	check_diff cvsDiff.out v2 v1 >check_diff.out 2>&1
 '
 
 test_expect_success 'cvs diff -N -r v2 -r v1.2' '
-	( cd cvswork && ! cvs -f diff -N -r v2 -r v1.2 -u ) >cvsDiff.out 2>cvs.log &&
+	( cd cvswork && ! cvs -f diff -N -r v2 -r v1.2 -u >../cvsDiff.out 2>../cvs.log ) &&
 	test_must_be_empty cvs.log &&
 	test -s cvsDiff.out &&
 	check_diff cvsDiff.out v2 v1.2 >check_diff.out 2>&1
@@ -487,7 +487,7 @@ test_expect_success 'apply early [cvswork3] diff to b3' '
 '
 
 test_expect_success 'check [cvswork3] diff' '
-	( cd cvswork3 && ! cvs -f diff -N -u ) >"$WORKDIR/cvsDiff.out" 2>cvs.log &&
+	( cd cvswork3 && ! cvs -f diff -N -u >"$WORKDIR/cvsDiff.out" 2>../cvs.log ) &&
 	test_must_be_empty cvs.log &&
 	test -s cvsDiff.out &&
 	test $(grep Index: cvsDiff.out | wc -l) = 3 &&
-- 
2.16.2.603.g180c1428f0

Re: [PATCH 0/2] Make cvs tests pass with '-x' tracing and /bin/sh

[Jeff King]

On Thu, Mar 08, 2018 at 01:38:42PM +0100, SZEDER Gábor wrote:

> > I installed 'cvs' and whatnot to run t94* and t96* tests, and sure
> > enough, 5 tests in 2 test scripts fail with '-x' tracing and /bin/sh.
> > I think I will be able to get around to send v2 during the weekend.
> 
> Well, I wasn't able to get around to it, and in the meantime
> 'sg/test-x' got merged into 'next'.  So here are the updates for those
> two test scripts.
> 
> The commit message of the first patch is perhaps unnecessary long, but
> it's mostly about explaining why the affected test was working
> reasonably well despite the rather weird 'test_cmp this that >>out
> 2>&1' thing.

You know I would never complain about a long commit message. :)

Both patches look OK to me. My only comment on the first one was "you
should just use 'return'", but I see Eric beat me to it.

I do think the postimage of the second one is a little less readable.
That's not a big deal to me, but I'm pretty sure I would intentionally
write it the "original" way if I found myself in a similar situation.
Which makes me wonder whether we'll end up accidentally re-introducing
these kinds of "-x" failures. But as I said before, I'm willing to wait
and see.

-Peff