Re: [Outreachy][PATCH v2] t6050: avoid pipes with downstream Git commands

[phillip.wood123@gmail.com]

Hi Chizoba

On 10/10/2024 19:51, Chizoba ODINAKA wrote:
> On Thu, 10 Oct 2024 at 15:08, Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 10/10/2024 07:39, chizobajames21@gmail.com wrote:
>>> From: Chizoba ODINAKA <chizobajames21@gmail.com>
>>>
>>> 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