patch, 3 partsfix 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

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(-)

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(-)

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(-)

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

The second one is off by 270.

On Sun, Aug 28, 2022 at 4:05 PM Junio C Hamano <gitster@pobox.com> wrote:

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

Eric Sunshine <sunshine@sunshineco.com> writes:

I plan to fix it up when I queue.

On Sun, Aug 28, 2022 at 1:05 PM Junio C Hamano <gitster@pobox.com> wrote:

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

On Mon, Aug 29, 2022 at 10:53 PM Elijah Newren <newren@gmail.com> wrote:

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.

On Mon, Aug 29, 2022 at 7:56 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

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

On Sat, Aug 27, 2022 at 10:18 PM Eric Sunshine via GitGitGadget <gitgitgadget@gmail.com> wrote:

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.

Hi Elijah & Eric,

On Mon, 29 Aug 2022, Elijah Newren wrote:

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