patcht6423: fix suppression of Git���s exit code in tests

From: Ayush Chandekar <ayu.chandekar@gmail.com>

Some test in t6423 supress Git's exit code, which can cause test failures go unnoticed. Specifically using git <subcommand> | <other-command> masks potential failures of the Git command.

This commit ensures that Git's exit status is correctly propogated by:
- Avoiding pipes that suppress exit codes.

Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
---
 t/t6423-merge-rename-directories.sh | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

On Sun, 2 Feb 2025 at 17:40, ayu-ch <ayu.chandekar@gmail.com> wrote:

s/supress/suppress

s/propogated/propagated The commit message should be in imperative mood (cf. Documentation/SubmittingPatches)

It should’ve been v2 of the patch you sent earlier [1] (cf. Documentation/MyFirstConribution), but otherwise, it looks good. [1]: https://lore.kernel.org/git/20250201004556.930220-1-ayu.chandekar@gmail.com/

Meet

On Sun, Feb 2, 2025 at 7:09 AM ayu-ch <ayu.chandekar@gmail.com> wrote:

I was curious if the project has a preference between `uniq filename` and `uniq <filename`, but apparently we haven't:

    % git grep 'uniq <' -- t | wc -l
    2
    git grep 'uniq [a-z0-9]' -- t | wc -l
    2

Though there does seem to be a global preference in the project to specify the filename directly to the command rather than redirecting from stdin. For instance:

    % git grep 'sort <' -- t | wc -l
    54
    % git grep 'sort [a-z0-9]' -- t | wc -l
    140

In any case, what you have here is probably fine, so no need to reroll just for this.

Eric Sunshine <sunshine@sunshineco.com> writes:

Have you inspected the hits from these grep runs?

    $ git grep -c 'sort [a-z0-9]' -- t/t7004-tag.sh
    t/t7004-tag.sh:17

Among 17 of them, 15 are on test titles.

    $ git grep -c '^test_expect_[sf].*sort [a-z0-9]' -- t/t7004-tag.sh
    t/t7004-tag.sh:15

So the above numbers are totally unreliable as a guide, I am afraid.

It is probably better to use sort/uniq without input redirection because your

    $ sort/uniq input >output

can be easily extended to

    $ sort/uniq input-a input-b input-c >output

but 

    $ sort/uniq <input >output

cannot be extended the same way, and you'd end up doing nonsense pipe like this:

    $ cat input-a input-b input-c | sort >output

which is a no-no.

In reality, however, we are not all that logical.

    $ git grep -e '^[ 	]*sort [a-z0-9][-a-z0-9]* ' -- t | wc -l
    46
    $ git grep -e '^[ 	]*sort <' -- t | wc -l
    51

with "s/sort/uniq/", the numbers are 0 vs 1.

There are a handful of sort invocations that take their input from redirected <<HEREDOC included in the latter number, but the overall picture does not change with them excluded.

Do you see any other changes needed in this patch? Let me know if there's anything you want me to adjust, especially in my commit message. Since my previous attempt wasn't very suitable.

Regards, Ayush

Ayush Chandekar <ayu.chandekar@gmail.com> writes:

If I were to change something, there are two minor things, but they are so minor that I'd be OK without these changes.

If this is supposed to be a part of microproject exchange (sorry, I lost track), then I am also OK to do the second (and hopefully final) iteration to give us a chance to practice.

If I were you and I chose to iterate one more time, I'd rephrase this

    This commit ensures that Git's exit status is correctly propogated by:
    - Avoiding pipes that suppress exit codes.

to more like

    Instead of placing a git command on the upstream side of a pipe,
    redirect its output to a file and process the file contents in
    two separate steps to avoid losing the exit status.

Also I'd not redirect into "uniq", i.e. instead of

	uniq <actual >tracked &&

I'd write

	uniq actual >tracked &&

but as discussed with Eric, this "better style" is not followed by existing code.

Some test in t6423 supress Git's exit code, which can cause test failures go unnoticed. Specifically using git <subcommand> | <other-command> masks potential failures of the Git command.

Instead of executing a Git command as the upstream component of a pipe, which can result in the exit status being lost, redirect its output to a file and then process that file in two steps to ensure the exit status is properly preserved.

Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
---
 t/t6423-merge-rename-directories.sh | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Ayush Chandekar <ayu.chandekar@gmail.com> writes:

Thanks for practicing yet another iteration. I am not going to actually replace the previous one with this one, as the previous one is just OK, but let's pretend I would to complete the "simulated" iteration.

Below, pretend that we will discard the previous one and replace it with this one, and plan to merge the result to 'next', but that is only for practice.

---

This is about 6423 ;-)  I'll amend while applying the patch.

OK. And the change to the test body to lose input redirection into "uniq" look OK, too.

Thanks.  Let's replace it and mark the topic for 'next'.

Thanks, Junio!

Appreciate the clarification about the test script number. I’ll be more careful next time.

Thanks for the review!

Regards, Ayush