fix failing t4301 test and &&-chain breakage

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

From: Eric Sunshine <[email protected]>

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.

---
 t/t4301-merge-tree-write-tree.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c5fd56df28f..d44c7767f30 100755
@@ -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

From: Eric Sunshine <[email protected]>

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.

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

index d44c7767f30..82a104bcbc9 100755
@@ -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

From: Eric Sunshine <[email protected]>

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.

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

index 82a104bcbc9..28ca5c38bb5 100755
@@ -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

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.

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.

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.

Hi Elijah & Eric,

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