[PATCH 0/3] fix failing t4301 test and &&-chain breakage

[PATCH 0/3] fix failing t4301 test and &&-chain breakage

[Eric Sunshine via GitGitGadget]

This series fixes a failing test in t4301 due to 'sed' behavioral
differences between implementations. It also fixes a couple broken &&-chains
and adds missing explicit loop termination.

The third patch is entirely subjective and can be dropped if unwanted. I
spent more than a few minutes puzzling over the script's use of 'printf
"\\n"' rather than the more typical 'printf "\n"' or even a simple 'echo',
wondering if there was some subtlety I was missing or whether Elijah had
encountered an unusual situation in which '\\n' was needed over '\n'. The
third patch chooses to replace 'printf "\\n"' with 'echo' which I find more
idiomatic, but I can see value in using 'printf "\n"' as perhaps being
clearer that it is adding a newline where one is missing.

The series is built atop 'en/t4301-more-merge-tree-tests' which is already
in 'next'.

Eric Sunshine (3):
  t4301: account for behavior differences between sed implementations
  t4031: fix broken &&-chains and add missing loop termination
  t4301: emit blank line in more idiomatic fashion

 t/t4301-merge-tree-write-tree.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)


base-commit: 3c4dbf556f425d83f3fbb729dcbecdc719ee4099
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1339%2Fsunshineco%2Fanonhash-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1339/sunshineco/anonhash-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1339
-- 
gitgitgadget

[PATCH 1/3] t4301: account for behavior differences between sed implementations

[Eric Sunshine via GitGitGadget]

From: Eric Sunshine <sunshine@sunshineco.com>

It is a common pattern in this script to write the result of
`merge-tree -z` (NUL-termination mode) to an "actual" file and then
manually append a newline to that file so that it can be diff'd easily
with a hand-crafted "expect" file which itself ends with a newline since
it has been created by standard Unix tools which terminate lines by
default. For instance:

    git merge-tree --write-tree -z ... >out &&
    printf "\\n" >>out
    anonymize_hash out >actual &&
    q_to_nul <<-EOF >expect &&
    ...
    EOF
    test_cmp expect actual

However, one test gets this backward:

    git merge-tree --write-tree -z ... >out &&
    anonymize_hash out >actual &&
    printf "\\n" >>actual

which means that, unlike all other cases, when anonymize_hash() is
called, the file being anonymized does not end with a newline. As a
result, this test fails on some platforms.

anonymize_hash() is implemented like this:

    anonymize_hash() {
        sed -e "s/[0-9a-f]\{40,\}/HASH/g" "$@"
    }

The problem arises due to differences in behavior of various `sed`
implementations when fed an incomplete line (lacking a newline).
Although most modern `sed` implementations output such a line
unmolested (i.e. without a newline), some older `sed` implementations
forcibly add a newline to the incomplete line (giving the output an
extra unexpected newline), while other very old implementations simply
swallow an incomplete line and don't emit it at all (making the output
shorter than expected).

Fix this test by manually adding the newline before passing it through
`sed`, thus ensuring identical behavior with all `sed` implementation,
and bringing the test in line with other tests in this script.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t4301-merge-tree-write-tree.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index c5fd56df28f..d44c7767f30 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -760,8 +760,8 @@ test_expect_success 'NUL terminated conflicted file "lines"' '
 	git commit -m "Renamed numbers" &&
 
 	test_expect_code 1 git merge-tree --write-tree -z tweak1 side2 >out &&
+	printf "\\n" >>out &&
 	anonymize_hash out >actual &&
-	printf "\\n" >>actual &&
 
 	# Expected results:
 	#   "greeting" should merge with conflicts
-- 
gitgitgadget

[PATCH 2/3] t4031: fix broken &&-chains and add missing loop termination

[Eric Sunshine via GitGitGadget]

From: Eric Sunshine <sunshine@sunshineco.com>

Fix &&-chain breaks in a couple tests which went unnoticed due to blind
spots in the &&-chain linters. In particular, the "magic exit code 117"
&&-chain checker built into test-lib.sh only recognizes broken &&-chains
at the top-level; it does not work within `{...}` groups, `(...)`
subshells, `$(...)` substitutions, or within bodies of compound
statements, such as `if`, `for`, `while`, `case`, etc. Furthermore,
`chainlint.sed`, which detects broken &&-chains only in `(...)`
subshells, missed these cases (which are in subshells) because it
(surprisingly) neglects to check for intact &&-chain on single-line
`for` loops.

While at it, explicitly signal failure of commands within the `for`
loops (which might arise due to the filesystem being full or "inode"
exhaustion). This is important since failures within `for` and `while`
loops can go unnoticed if not detected and signaled manually since the
loop itself does not abort when a contained command fails, nor will a
failure necessarily be detected when the loop finishes since the loop
returns the exit code of the last command it ran on the final iteration,
which may not be the command which failed.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t4301-merge-tree-write-tree.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index d44c7767f30..82a104bcbc9 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -150,7 +150,7 @@ test_expect_success 'directory rename + content conflict' '
 		cd dir-rename-and-content &&
 		test_write_lines 1 2 3 4 5 >foo &&
 		mkdir olddir &&
-		for i in a b c; do echo $i >olddir/$i; done
+		for i in a b c; do echo $i >olddir/$i || exit 1; done &&
 		git add foo olddir &&
 		git commit -m "original" &&
 
@@ -662,7 +662,7 @@ test_expect_success 'directory rename + rename/delete + modify/delete + director
 		cd 4-stacked-conflict &&
 		test_write_lines 1 2 3 4 5 >foo &&
 		mkdir olddir &&
-		for i in a b c; do echo $i >olddir/$i; done
+		for i in a b c; do echo $i >olddir/$i || exit 1; done &&
 		git add foo olddir &&
 		git commit -m "original" &&
 
-- 
gitgitgadget

[PATCH 3/3] t4301: emit blank line in more idiomatic fashion

[Eric Sunshine via GitGitGadget]

From: Eric Sunshine <sunshine@sunshineco.com>

The unusual use of:

    printf "\\n" >>file &&

may give readers pause, making them wonder why this form was chosen over
the more typical:

    printf "\n" >>file &&

However, even that may give pause since it is a somewhat unusual and
long-winded way of saying:

    echo >>file &&

Therefore, replace `printf` with the more idiomatic `echo`, with the
hope of eliminating a possible stumbling block for those reading the
code.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t4301-merge-tree-write-tree.sh | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index 82a104bcbc9..28ca5c38bb5 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -176,7 +176,7 @@ test_expect_success 'directory rename + content conflict' '
 
 		test_expect_code 1 \
 			git merge-tree -z A^0 B^0 >out &&
-		printf "\\n" >>out &&
+		echo >>out &&
 		anonymize_hash out >actual &&
 		q_to_tab <<-\EOF | lf_to_nul >expect &&
 		HASH
@@ -230,7 +230,7 @@ test_expect_success 'rename/delete handling' '
 
 		test_expect_code 1 \
 			git merge-tree -z A^0 B^0 >out &&
-		printf "\\n" >>out &&
+		echo >>out &&
 		anonymize_hash out >actual &&
 		q_to_tab <<-\EOF | lf_to_nul >expect &&
 		HASH
@@ -284,7 +284,7 @@ test_expect_success 'rename/add handling' '
 
 		test_expect_code 1 \
 			git merge-tree -z A^0 B^0 >out &&
-		printf "\\n" >>out &&
+		echo >>out &&
 
 		#
 		# First, check that the bar that appears at stage 3 does not
@@ -351,7 +351,7 @@ test_expect_success SYMLINKS 'rename/add, where add is a mode conflict' '
 
 		test_expect_code 1 \
 			git merge-tree -z A^0 B^0 >out &&
-		printf "\\n" >>out &&
+		echo >>out &&
 
 		#
 		# First, check that the bar that appears at stage 3 does not
@@ -417,7 +417,7 @@ test_expect_success 'rename/rename + content conflict' '
 
 		test_expect_code 1 \
 			git merge-tree -z A^0 B^0 >out &&
-		printf "\\n" >>out &&
+		echo >>out &&
 		anonymize_hash out >actual &&
 		q_to_tab <<-\EOF | lf_to_nul >expect &&
 		HASH
@@ -471,7 +471,7 @@ test_expect_success 'rename/add/delete conflict' '
 
 		test_expect_code 1 \
 			git merge-tree -z B^0 A^0 >out &&
-		printf "\\n" >>out &&
+		echo >>out &&
 		anonymize_hash out >actual &&
 
 		q_to_tab <<-\EOF | lf_to_nul >expect &&
@@ -528,7 +528,7 @@ test_expect_success 'rename/rename(2to1)/delete/delete conflict' '
 
 		test_expect_code 1 \
 			git merge-tree -z A^0 B^0 >out &&
-		printf "\\n" >>out &&
+		echo >>out &&
 		anonymize_hash out >actual &&
 
 		q_to_tab <<-\EOF | lf_to_nul >expect &&
@@ -600,7 +600,7 @@ test_expect_success 'mod6: chains of rename/rename(1to2) and add/add via collidi
 
 		test_expect_code 1 \
 			git merge-tree -z A^0 B^0 >out &&
-		printf "\\n" >>out &&
+		echo >>out &&
 
 		#
 		# First, check that some of the hashes that appear as stage
@@ -690,7 +690,7 @@ test_expect_success 'directory rename + rename/delete + modify/delete + director
 
 		test_expect_code 1 \
 			git merge-tree -z A^0 B^0 >out &&
-		printf "\\n" >>out &&
+		echo >>out &&
 		anonymize_hash out >actual &&
 
 		q_to_tab <<-\EOF | lf_to_nul >expect &&
@@ -760,7 +760,7 @@ test_expect_success 'NUL terminated conflicted file "lines"' '
 	git commit -m "Renamed numbers" &&
 
 	test_expect_code 1 git merge-tree --write-tree -z tweak1 side2 >out &&
-	printf "\\n" >>out &&
+	echo >>out &&
 	anonymize_hash out >actual &&
 
 	# Expected results:
-- 
gitgitgadget

Re: [PATCH 0/3] fix failing t4301 test and &&-chain breakage

[Junio C Hamano]

"Eric Sunshine via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Eric Sunshine (3):
>   t4301: account for behavior differences between sed implementations
>   t4031: fix broken &&-chains and add missing loop termination
>   t4301: emit blank line in more idiomatic fashion
>
>  t/t4301-merge-tree-write-tree.sh | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)

The second one is off by 270.

Re: [PATCH 0/3] fix failing t4301 test and &&-chain breakage

[Eric Sunshine]

On Sun, Aug 28, 2022 at 4:05 PM Junio C Hamano <gitster@pobox.com> wrote:
> "Eric Sunshine via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >   t4301: account for behavior differences between sed implementations
> >   t4031: fix broken &&-chains and add missing loop termination
> >   t4301: emit blank line in more idiomatic fashion
>
> The second one is off by 270.

Shall I re-roll or will you fix it while queuing (assuming you queue it)?

Re: [PATCH 0/3] fix failing t4301 test and &&-chain breakage

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sun, Aug 28, 2022 at 4:05 PM Junio C Hamano <gitster@pobox.com> wrote:
>> "Eric Sunshine via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> >   t4301: account for behavior differences between sed implementations
>> >   t4031: fix broken &&-chains and add missing loop termination
>> >   t4301: emit blank line in more idiomatic fashion
>>
>> The second one is off by 270.
>
> Shall I re-roll or will you fix it while queuing (assuming you queue it)?

I plan to fix it up when I queue.

Re: [PATCH 0/3] fix failing t4301 test and &&-chain breakage

[Elijah Newren]

On Sun, Aug 28, 2022 at 1:05 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Eric Sunshine via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > Eric Sunshine (3):
> >   t4301: account for behavior differences between sed implementations
> >   t4031: fix broken &&-chains and add missing loop termination
> >   t4301: emit blank line in more idiomatic fashion
> >
> >  t/t4301-merge-tree-write-tree.sh | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
>
> The second one is off by 270.

Apparently Eric knew what you meant, but I'm perplexed by this
statement and what it means.  What am I missing?

Re: [PATCH 0/3] fix failing t4301 test and &&-chain breakage

[Eric Sunshine]

On Mon, Aug 29, 2022 at 10:53 PM Elijah Newren <newren@gmail.com> wrote:
> On Sun, Aug 28, 2022 at 1:05 PM Junio C Hamano <gitster@pobox.com> wrote:
> > >   t4301: account for behavior differences between sed implementations
> > >   t4031: fix broken &&-chains and add missing loop termination
> > >   t4301: emit blank line in more idiomatic fashion
> >
> > The second one is off by 270.
>
> Apparently Eric knew what you meant, but I'm perplexed by this
> statement and what it means.  What am I missing?

Junio's comment was opaque to me, as well, and it took several minutes
to figure it out (especially since I authored the patches, thus I read
what I expected to read, not what was really there). Taking a look at
just prefixes on the patch subject lines...

    t4301
    t4031
    t4301

there's a transposition in there which, mathematically speaking, makes
one of the test script numbers 270 less than the others.

Re: [PATCH 0/3] fix failing t4301 test and &&-chain breakage

[Elijah Newren]

On Mon, Aug 29, 2022 at 7:56 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Mon, Aug 29, 2022 at 10:53 PM Elijah Newren <newren@gmail.com> wrote:
> > On Sun, Aug 28, 2022 at 1:05 PM Junio C Hamano <gitster@pobox.com> wrote:
> > > >   t4301: account for behavior differences between sed implementations
> > > >   t4031: fix broken &&-chains and add missing loop termination
> > > >   t4301: emit blank line in more idiomatic fashion
> > >
> > > The second one is off by 270.
> >
> > Apparently Eric knew what you meant, but I'm perplexed by this
> > statement and what it means.  What am I missing?
>
> Junio's comment was opaque to me, as well, and it took several minutes
> to figure it out (especially since I authored the patches, thus I read
> what I expected to read, not what was really there). Taking a look at
> just prefixes on the patch subject lines...
>
>     t4301
>     t4031
>     t4301
>
> there's a transposition in there which, mathematically speaking, makes
> one of the test script numbers 270 less than the others.

Ah, gotcha.  Yeah, I totally missed both the digit transposition and
the attempt to highlight it.  Thanks for the explanation.

Re: [PATCH 0/3] fix failing t4301 test and &&-chain breakage

[Elijah Newren]

On Sat, Aug 27, 2022 at 10:18 PM Eric Sunshine via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> This series fixes a failing test in t4301 due to 'sed' behavioral
> differences between implementations. It also fixes a couple broken &&-chains
> and adds missing explicit loop termination.
>
> The third patch is entirely subjective and can be dropped if unwanted. I
> spent more than a few minutes puzzling over the script's use of 'printf
> "\\n"' rather than the more typical 'printf "\n"' or even a simple 'echo',
> wondering if there was some subtlety I was missing or whether Elijah had
> encountered an unusual situation in which '\\n' was needed over '\n'. The
> third patch chooses to replace 'printf "\\n"' with 'echo' which I find more
> idiomatic, but I can see value in using 'printf "\n"' as perhaps being
> clearer that it is adding a newline where one is missing.

I can't actually provide the reasoning for it; I took Dscho's testcase
from [1] and used it as a basis for adding several other testcases.
When I was copying & pasting and adjusting, I just didn't notice the
'printf "\\n"'.  But using a simple echo makes sense.

[1] https://lore.kernel.org/git/3b4ed8bb1bb615277ee51a7b2af5fc53bae0a6e4.1660892256.git.gitgitgadget@gmail.com/

Anyway, I've read through the patches and your series looks good to me.

Re: [PATCH 0/3] fix failing t4301 test and &&-chain breakage

[Johannes Schindelin]

Hi Elijah & Eric,

On Mon, 29 Aug 2022, Elijah Newren wrote:

> On Sat, Aug 27, 2022 at 10:18 PM Eric Sunshine via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > This series fixes a failing test in t4301 due to 'sed' behavioral
> > differences between implementations. It also fixes a couple broken &&-chains
> > and adds missing explicit loop termination.
> >
> > The third patch is entirely subjective and can be dropped if unwanted. I
> > spent more than a few minutes puzzling over the script's use of 'printf
> > "\\n"' rather than the more typical 'printf "\n"' or even a simple 'echo',
> > wondering if there was some subtlety I was missing or whether Elijah had
> > encountered an unusual situation in which '\\n' was needed over '\n'. The
> > third patch chooses to replace 'printf "\\n"' with 'echo' which I find more
> > idiomatic, but I can see value in using 'printf "\n"' as perhaps being
> > clearer that it is adding a newline where one is missing.
>
> I can't actually provide the reasoning for it; I took Dscho's testcase
> from [1] and used it as a basis for adding several other testcases.
> When I was copying & pasting and adjusting, I just didn't notice the
> 'printf "\\n"'.  But using a simple echo makes sense.
>
> [1] https://lore.kernel.org/git/3b4ed8bb1bb615277ee51a7b2af5fc53bae0a6e4.1660892256.git.gitgitgadget@gmail.com/

No other reason than that I _seem_ to recall having run into some issues
where _some_ POSIX shell (was it BusyBox' ash?) did not like the
single-escape form "\n".

I have no firm recollection, though, and am fine with converting all of
the double backslashes to single backslashes (read: I am very indifferent
to this issue).

Ciao,
Dscho