Re: [PATCH] t*: avoid using pipes

From: Johannes Sixt <j6t@kdbg.org>
Date: Mar 7, 2017, 20:39 UTC
Message-ID: <3026648b-a26c-bc67-62dc-170217d6c2ca@kdbg.org>
In-Reply-To: <CAGZ79kaYi1OLuOKvbCmDrMCq0fZnO2Ry7JML=Puwmx6TTtEYog@mail.gmail.com>

Am 07.03.2017 um 18:21 schrieb Stefan Beller:

> On Tue, Mar 7, 2017 at 8:10 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:
>> I'm Prathamesh Chavan. As a part of my micropraoject I have been working on
>> "Avoid pipes for git related commands in test suites".

Welcome to the Git community!

Show 6 quoted lines

> * keep it micro first, e.g. just convert one file,
>   send to list, wait for reviewers feedback and incorporate that
>   (optional step after having done the full development cycle:
>   convert all the other files; each as its own patch)
> * split up this one patch into multiple patches, e.g. one
>   file per patch, then send a patch series.

Actually, being a *micro* project, it should stay so. Not doing all of the changes would leave some tasks for other apprentices to get warm with our review process.

Show 6 quoted lines

>> https://github.com/pratham-pc/git/tree/micro-proj
>
> While I did look at that, not everyone here in the git community
> does so. (Also for getting your change in, Junio seems to strongly prefer
> patches on list instead of e.g. fetching and cherry-picking from your
> github)

Thank you, Stefan, for digging out one particularly interesting case.

Show 10 quoted lines

> When looking at the content, the conversion seems a bit mechanical
> (which is fine for a micro project), such as:
> ...
> - test "$(git show --pretty=format:%s | head -n 1)" = "one"
> + test "$(git show --pretty=format:%s >out && head -n 1 <out)" = "one"
> ...
>
> specifically for the "head" command I don't think it makes a
> difference in correctness whether you pipe the file into the tool
> or give the filename, i.e.  "head -n 1 out" would work just as fine.

True, but!

The intent of removing git invocations from the upstream of a pipe is that a failure exit code is able to stop a && chain.

The example above does not achieve this even after removal of the pipe. The reason is that exit code of process expansions $(...) is usually ignored. To meet the intent, further changes are necessary, for example to:

	git show --pretty=format:%s >out &&
	test "$(head -n 1 out)" = "one"

Side note: There is one exception where the exit code of $(...) is not ignored: when it occurs in the last variable assignment of a command that consists only of variable assignments.

-- Hannes