From: Stefan Beller <sbeller@google.com>
Date: Tue, 07 Mar 2017 17:21:08 GMT
Subject: Re: [PATCH] t*: avoid using pipes
Message-ID: <CAGZ79kaYi1OLuOKvbCmDrMCq0fZnO2Ry7JML=Puwmx6TTtEYog@mail.gmail.com>
In-Reply-To: <CAME+mvUe7itzg7JLu9_131smzHHE0JsN-z7q8_dTY1qEdugYWw@mail.gmail.com>

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

Thanks for working on that microproject!

> I tried sending the
> patch, but it got blocked since the mail contained more than 100 000
> characters.

Yeah, even the github UI seems to have trouble with that commit.
(A bit slow, not showing the full content, but rather I needed to click
on "load diff" for tests files 7000+)

This is a lot of change (in terms of lines) for a micro project. :)
I'd have two competing advices:
* 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.

The outcome will be the same, but in the first you get feedback
quicker, such that hopefully you only need to touch the rest of
files after the first file just once.


> Hence I'll like to attach the link to my branch 'micro-proj', where I did the
> required changes.
>
> 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)

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.

There is a difference in readability, though. For consistency I'd
suggest to drop the "<", as the numbers might support:

$ cd t
$ git grep head |wc -l
# This also counts other occurrences of the string,
# not just the invocation of the head tool
2871
$ git grep head |grep "<" |wc -l
# same here
58

Another aspect might be performance at scale as the "<" will
let the shell open the file and pipe the content via stdin to the
head tool, whereas when giving a filename the head tool needs
to open the file. Both times the file doesn't need to be read completely,
but "head -n 1" can close the file handle early in the game.
I dunno.

Thanks,
Stefan

>
> Thanks.