patch, 3 partstests: fix broken &&-chains & abort loops on error

This series fixes some broken &&-chains in tests and adds missing || return 1 (or || exit 1) to loops to ensure they exit early upon error. It is a followup to an earlier series which fixed many more such problems.^1 [https://lore.kernel.org/git/20211209051115.52629-1-sunshine@sunshineco.com/]

Eric Sunshine (3):
  t2407: fix broken &&-chains in compound statement
  t1092: fix buggy sparse "blame" test
  t: detect and signal failure within loop

 t/perf/p7527-builtin-fsmonitor.sh        |  2 +-
 t/t1092-sparse-checkout-compatibility.sh | 10 +++++-----
 t/t2407-worktree-heads.sh                |  4 ++--
 t/t5329-pack-objects-cruft.sh            |  8 ++++----
 t/t6429-merge-sequence-rename-caching.sh |  2 +-
 5 files changed, 13 insertions(+), 13 deletions(-)

base-commit: 795ea8776befc95ea2becd8020c7a284677b4161
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1312%2Fsunshineco%2Fchainmore-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1312/sunshineco/chainmore-v1
Pull-Request: https://github.com/git/git/pull/1312

From: Eric Sunshine <sunshine@sunshineco.com>

The breaks in the &&-chain in this test went unnoticed because 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` detects broken &&-chains only in `(...)` subshells. Thus, the &&-chain breaks in this test fall into the blind spots of the &&-chain linters.

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

From: Eric Sunshine <sunshine@sunshineco.com>

This test wants to verify that `git blame` errors out when asked to blame a file _not_ in the sparse checkout. However, the very first file it asks to blame _is_ present in the checkout, thus `test_must_fail git blame $file` gives an unexpected result (the "blame" succeeds). This problem went unnoticed because the test invokes `test_must_fail git blame $file` in loop but forgets to break out of the loop early upon failure, thus the failure gets swallowed.

Fix the test by having it not ask to blame a file present in the sparse checkout, and instead only blame files not present, as intended. While at it, also add the missing `|| return 1` which allowed this bug to go unnoticed.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

On 8/22/2022 2:26 PM, Eric Sunshine via GitGitGadget wrote:

Thank you for catching this!

-Stolee

From: Eric Sunshine <sunshine@sunshineco.com>

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. Therefore, detect and signal failures manually within loops using the idiom `|| return 1` (or `|| exit 1` within subshells).

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/perf/p7527-builtin-fsmonitor.sh        | 2 +-
 t/t1092-sparse-checkout-compatibility.sh | 6 +++---
 t/t5329-pack-objects-cruft.sh            | 8 ++++----
 t/t6429-merge-sequence-rename-caching.sh | 2 +-
 4 files changed, 9 insertions(+), 9 deletions(-)

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

With the loop being on the upstream of a pipe, does the added "exit 1" have any effect?

Everything else in these three patches looked very sensible, but this one I found questionable.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

And the answer is "no".  Without use of rhetorical question:

    The loop is on the upstream side of a pipe, so "exit 1" will be
    lost.  "sort -k1" will get a shortened output, unless the
    failure happens at the last iteration, so it is likely that the
    test may fail, but relying on the "expect" (what is supposed to
    have the _right_ answer) file not being right to get our
    breakage noticed does not sound right.

As to the questionable one, we could probably do something like the attached patch if we really wanted to. We can guarantee that this "expect" will never match any "actual", which is output from pack-mtimes test tool command. Whatever "tricky/ugly" approach we choose to take, I think this one deserves to be done in a single patch on its own with an explanation.

----- >8 --------- >8 --------- >8 --------- >8 ---- t5329: notice a failure within a loop

We try to write "|| return 1" at the end of a sequence of &&-chained command in a loop of our tests, so that a failure of any step during the earlier iteration of the loop can properly be caught.

There is one loop in this test script that is used to compute the expected result, that will be later compared with an actual output produced by the "test-tool pack-mtimes" command. This particular loop, however, is placed on the upstream side of a pipe, whose non-zero exit code does not get noticed.

Emit a line that will never be produced by the "test-tool pack-mtimes" to cause the later comparison to fail. As we use test_cmp to compare this "expected output" file with the "actual output", the "error message" we are emitting into the expected output stream will stand out and shown to the tester.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5329-pack-objects-cruft.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Am 22.08.22 um 22:59 schrieb Junio C Hamano:

This looks like the right thing to do. But write $oid, not $obj.

-- Hannes

On Mon, Aug 22, 2022 at 11:26 AM Eric Sunshine via GitGitGadget <gitgitgadget@gmail.com> wrote:

[...]

That's not something I'm likely ever going to remember to think of as capable of failing and needing this special care. Is this a preliminary series before you send chainlint improvements that finds this kind of thing for us? Or did you notice this some other way?

Change is fine, of course, I'm just curious how it was found (and how I can avoid adding more of these that you'll need to later fix up).

On Mon, Aug 22, 2022 at 11:05 PM Elijah Newren <newren@gmail.com> wrote:

This could fail due to lack of space on the filesystem or "inode" exhaustion (indeed, I've seen out-of-space failures when I've underallocated the ramdisk on which I run tests). But there are some cases of added `|| return` or `|| exit` in the original series[1] which are just churn because the code inside the loop wouldn't / couldn't / shouldn't fail. I wasn't happy about adding those simply to pacify a not-smart-enough linter, but I eventually convinced myself that the small amount of inconvenience of those pointless cases was greatly outweighed by the vast number of cases in which adding `|| return` or `|| exit` was the correct thing to do since those cases could genuinely have allowed errors in Git or in the tests themselves to go unnoticed.

Yes, this is another preliminary series before sending the new `chainlint` series, and it was the new linter which found these cases.

[1]: https://lore.kernel.org/git/20211209051115.52629-1-sunshine@sunshineco.com/

Although the implementation of the new linter has been complete for over a year, I finally found time to work on polishing the patch series itself. I had hoped to get it submitted this past week, but time constraints prevented it. So, the answer is that the new linter (once I submit it and once Junio accepts it -- if he does) should help you avoid introducing more such cases.