Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file

From: SZEDER Gábor <szeder@ira.uka.de>
Date: Mar 27, 2016, 13:27 UTC
Message-ID: <1459085245-20658-1-git-send-email-szeder@ira.uka.de>
In-Reply-To: <CAPig+cRQ5hOLi3539Emq=YexHRXqdesf+Yo2cY+NkiPO_9Vhyg@mail.gmail.com>

Show 17 quoted lines

> > +! test -s out ||
> > +rm out &&
> 
> Why not just "rm -f out"? But, more importantly, why do you need to
> remove the file at all? The '>' redirection operator (used below) will
> overwrite the file, so no need to remove it beforehand.
> 
> > +! grep '^diff --git' "$1" ||
> > +grep '^diff --git' "$1" >out
> 
> Um, what? Why two greps? I would have expected you to simply re-use
> the existing grep (minus the backslash) while adding the redirection:
> 
>     -exec grep '^diff --git' "\$1"
>     +exec grep '^diff --git' "$1" >out
> 
> Am I missing something obvious?

In the non-verbose cases no diff is included in the commit message template, thus the pattern looking for it doesn't match anything, grep exits with error code, which in turn becomes the editor's exit code, ultimately making 'git commit' fail. Not good.

I suppose both the explicit 'rm out' and the '! grep ... || ...' is there to deal with this situation.

Hmph.

I think we could:

  - either revive the idea of two editor scripts: one for the
    non-verbose case checking with '! grep ...' that there are no
    diffs in the commit message template, and one for all verbose
    cases storing those diff lines in a file to be counted later.

  - or use a fake editor that merely copies the whole commit message
    template to a separate file, and we do the greping in the tests
    themselves as well.

  - or simply stick a 'true' at the end of the editor script ensuring
    that it returns success even when grep can't find the pattern, but
    I kind of feel ashamed of myself for even mentioning this
    possibility ;)

I would go for the second possibility, but don't feel strong about it.