patch*** Avoid using Pipes ***

From: achluma <achluma@gmail.com>

*** BLURB HERE ***

achluma (1):
  t2400: avoid using pipes

 t/t2400-worktree-add.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

base-commit: d0e8084c65cbf949038ae4cc344ac2c2efd77415

From: achluma <ach.lumap@gmail.com>

The exit code of the preceding command in a pipe is disregarded, so it's advisable to refrain from relying on it. Instead, by saving the output of a Git command to a file, we gain the ability to examine the exit codes of both commands separately.

Signed-off-by: achluma <ach.lumap@gmail.com>
---
 t/t2400-worktree-add.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

On Tue, Oct 3, 2023 at 1:49 PM <ach.lumap@gmail.com> wrote:

Pipes themselves are not necessarily problematic, and there are many places in the test suite where they are legitimately used. Rather...

... as you correctly explain here, we don't want to lose the exit code from the Git command. Thus, if you want to convey more information to readers of `git log --oneline` (or other such commands), a better subject for the patch might be:

    t2400: avoid losing Git exit code

That minor comment aside (which is probably not worth a reroll), the commit message properly explains why this change is desirable and the patch itself looks good.

Thanks for following the style guideline and omitting whitespace between the redirection operator and the destination file.

Eric Sunshine <sunshine@sunshineco.com> writes:

Thanks for writing and reviewing.  Will queue.

*** BLURB HERE ***

Achu Luma (1):
  t2400: avoid using pipes

 t/t2400-worktree-add.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

base-commit: d0e8084c65cbf949038ae4cc344ac2c2efd77415

The exit code of the preceding command in a pipe is disregarded, so it's advisable to refrain from relying on it. Instead, by saving the output of a Git command to a file, we gain the ability to examine the exit codes of both commands separately.

Signed-off-by: achluma <ach.lumap@gmail.com>
---
 t/t2400-worktree-add.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Hi Luma,

On Thu, Nov 30, 2023 at 6:37 PM Achu Luma <ach.lumap@gmail.com> wrote:

I think the issue with merging your patch (in https://lore.kernel.org/git/xmqqedibzgi1.fsf@gitster.g/) was that this "Signed-off-by: ..." line didn't show your full real name and didn't match your name in your email address.

Assuming that "Achu Luma" is your full real name, you should replace "achluma" with "Achu Luma" in the "Signed-off-by: ..." line.

Also it's better not to send a cover letter patch like https://lore.kernel.org/git/20231130165429.2595-1-ach.lumap@gmail.com/ with no content for small patches like this.

When you resend, please also make sure to use [Outreachy] in the patch subject and to increment the version number of the patch, using for example "[PATCH v3]".

It would be nice too if after the line starting with --- below, you could describe in a few lines the changes in the new version of the patch compared to the previous version.

Here (after the line starting with --- above) is the place where you can tell what changed in the patch compared to the previous version.

Note that when there is a cover letter patch, it's better to talk about changes in the new version in the cover letter, but I dont think it's worth sending a cover letter patch.

Thanks, Christian.

The exit code of the preceding command in a pipe is disregarded, so it's advisable to refrain from relying on it. Instead, by saving the output of a Git command to a file, we gain the ability to examine the exit codes of both commands separately.

Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
 Since v2 I don't send a cover  letter anymore, and I changed 
 my "Signed-of-by: ..." line so that it
 contains my full real name and I added "Outreachy" to the subject.

 t/t2400-worktree-add.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Achu Luma <ach.lumap@gmail.com> writes:

"avoid using pipes" is a means to an end. And it is more important to tell readers what that "end" is. With this patch, what are we trying to achieve? Cater to platforms that lack pipes? Help platforms that cannot run two processes at the same time, so let one run and store the result in a file, and then let the other one run, to reduce the CPU load?

If we run a "git" command, especially a command we are testing, on the upstream side of a pipe, we lose information. We cannot tell what exit status the command exited with. That is what we care about.

So, it is better to say that in the title, e.g.,

    Subject: [PATCH] t2400: avoid losing exit status to pipes

It is unclear what "it" refers to here. We cannot rely on the exit code of the command on the upstream side of a pipe, obviously.

Surely. I personally think that the title that says what the purpose of the patch is clearly should be sufficient without any further description in the body, though.

Nicely done.

The exit code of the preceding command in a pipe is disregarded. So if that preceding command is a Git command that fails, the test would not fail. Instead, by saving the output of that Git command to a file, and removing the pipe, we make sure the test will fail if that Git command fails.

Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
 The difference between v3 and v4 is:
 - Changed subject to better reflect what the patch is doing.
 - Updated the commit message.

 t/t2400-worktree-add.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)