Hi Philip,
On Thu, 10 Oct 2024 at 15:08, Phillip Wood [off-list ref] wrote:

Hi Chizoba

On 10/10/2024 07:39, chizobajames21@gmail.com wrote:

quoted

From: Chizoba ODINAKA <redacted>

In pipes, the exit code of a chain of commands is determined by
the downstream command.

I would perhaps say "final command" rather than "downstream command" as
in a pipeline "cmd1 | cmd2 | cmd3" cmd2 and cmd3 are downstream of cmd1
but it is the exit code of cmd3 that will be used

Yes, final command is a more appropriate explain.

quoted

In order not to loss the entire result code of tests,
write output of upstreams into a file.

We're interested in checking the exit code of git, but not other
commands so it would be helpful to make that clear. Usman's patch [1]
has a good explanation of this.

I just read that sentence again, it obviously needs some clarity. "In order not
to miss the exit code of any Git command, avoid using pipe and write
output into a file"
has more clarity. I will look up on Usman's patch [1], before my next changes.

This patch also changes instances of "grep" to "test_grep" so the commit
message needs to explain the reason for that change which is that it
gives a better debugging experience if the test fails.

I had included that in my "Changes in v2", appended beneath my "Sign-off-by".
"Changes in v2:
- split multiple commands chain on the same line across multiple line,
  for easier readability
- replace "grep" with "test_grep", for more context in case of a "grep"
  failure"
Maybe it was not so obvious that you didn't notice, or it is not the
traditional way of including it.

The patch is looking pretty good, most of the conversions look correct.
I've left a few comments below


[1]
https://lore.kernel.org/git/bfff7937cd20737bb5a8791dc7492700b1d7881f.1728315124.git.gitgitgadget@gmail.com (local)

quoted

  test_expect_success 'replace the author' '
-     git cat-file commit $HASH2 | grep "author A U Thor" &&
-     R=$(git cat-file commit $HASH2 | sed -e "s/A U/O/" | git hash-object -t commit --stdin -w) &&
-     git cat-file commit $R | grep "author O Thor" &&
+     git cat-file commit $HASH2 >actual &&
+     test_grep "author A U Thor" actual &&
+     git cat-file commit $HASH2 >actual &&

You don't need to repeat this command now that we are saving the output
of "git cat-file commit $HASH2"

quoted

+     R=$(sed -e "s/A U/O/" actual | git hash-object -t commit --stdin -w) &&
+     git cat-file commit $R >actual &&
+     test_grep "author O Thor" actual &&

quoted

  test_expect_success 'push branch with replacement' '
-     git cat-file commit $PARA3 | grep "author A U Thor" &&
-     S=$(git cat-file commit $PARA3 | sed -e "s/A U/O/" | git hash-object -t commit --stdin -w) &&
-     git cat-file commit $S | grep "author O Thor" &&
+     git cat-file commit $PARA3 >actual &&
+     test_grep "author A U Thor" actual &&
+     git cat-file commit $PARA3 >actual &&

We can drop this line for the same reason as above

quoted

+     S=$(sed -e "s/A U/O/" actual | git hash-object -t commit --stdin -w) &&
+     git cat-file commit $S >actual &&
+     test_grep "author O Thor" actual &&

quoted

@@ -260,14 +291,14 @@ test_expect_success 'fetch branch with replacement' '
              cd clone_dir &&
              git fetch origin refs/heads/tofetch:refs/heads/parallel3 &&
              git log --pretty=oneline parallel3 >output.txt &&
-             ! grep $PARA3 output.txt &&
+             ! test_grep $PARA3 output.txt &&

test_grep will print an error message the pattern does not match. In
this case we expect it not to match so we want to print an error if it
does match. We can do that with

        test_grep ! $PARA3 output.txt &&

quoted

  test_expect_success 'index-pack and replacements' '
-     git --no-replace-objects rev-list --objects HEAD |
+     git --no-replace-objects rev-list --objects HEAD >actual &&
      git --no-replace-objects pack-objects test- &&

This command has lost its input - you need to use '<actual' to get it to
read output from "git rev-list". This test itself could probably do a
better job of checking that index-pack does what we expect but that is
outside the scope of this patch.

quoted

      git index-pack test-*.pack
  '

Everything that I've not commented on looks correct to me.

Thanks Philip for the review, I will make the needed changes in my
next patch. And look into
the index-pack proposal in a new patch, since it is outside this scope.

Best Wishes

Phillip

Thanks
Chizoba