Hi Chizoba
On 10/10/2024 19:51, Chizoba ODINAKA wrote:
On Thu, 10 Oct 2024 at 15:08, Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
On 10/10/2024 07:39, chizobajames21@xxxxxxxxx wrote:
From: Chizoba ODINAKA <chizobajames21@xxxxxxxxx>
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.
It's great that you listed the changes between versions below the "---"
line - that is really helpful for reviewers. However those comments are
not part of the commit message when the patch is applied. It is
important for the commit message to explain the reasons for all the
changes in a patch so that someone reading it later can understand why
the change was made. Therefore the grep->test_grep change should be
explained in the commit message. In general one should avoid making
unrelated changes in the same commit. In this case I think one can argue
that the changes are small enough that combining them is fine.
Thanks Philip for the review, I will make the needed changes in my
next patch.
That's great, I look forward to reviewing it
And look into
the index-pack proposal in a new patch, since it is outside this scope.
Let's finish this patch first. I'm not sure what the best way to improve
that test is at the moment.
Best Wishes
Phillip
