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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 27, 2016, 17:27 UTC
Message-ID: <20160327172741.GA4005@flurp.local>
In-Reply-To: <1459085245-20658-1-git-send-email-szeder@ira.uka.de>

On Sun, Mar 27, 2016 at 03:27:25PM +0200, SZEDER Gábor wrote:

Show 25 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.

Sure, I understand that, but that's not what I meant. What I wanted to know was why Pranit didn't just use the simpler:

    grep '^diff --git' "$1" >out
    exit 0

and then, in tests:

    test_line_count = n out

where 'n' is 0, 1, or 2 as expected by the test.

Unfortunately, you missed the rest of the discussion since Pranit (presumably) accidentally dropped the mailing list when he replied, and I didn't notice the omission when replying to him with the above suggestion, which would have saved you the bother of going through this, as well.

Show 17 quoted lines

> 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.

Your #3 is effectively what I had suggested, as well (which is reproduced above). I had already made this change locally along with some other changes I suggested in other responses, and those changes look like this (atop Pranit's two patches), which isn't too bad:

--- 8< ---

diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index 569fd8b..ea26b57 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -4,12 +4,9 @@ test_description='verbose commit template'
 . ./test-lib.sh
 
 write_script "check-for-diff" <<'EOF' &&
-! test -s out ||
-rm out &&
-! grep '^diff --git' "$1" ||
 grep '^diff --git' "$1" >out
+exit 0
 EOF
-chmod +x check-for-diff
 test_set_editor "$PWD/check-for-diff"
 
 cat >message <<'EOF'
@@ -101,11 +98,12 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '
 	test_i18ngrep "Aborting commit due to empty commit message." err
 '
 
+test_expect_success 'setup -v -v' '
+	echo dirty >file
+'
+
 test_expect_success 'commit.verbose true and --verbose omitted' '
-	echo content >file2 &&
-	echo content >>file &&
-	git add file2 &&
-	git -c commit.verbose=true commit -F message &&
+	git -c commit.verbose=true commit --amend &&
 	test_line_count = 1 out
 '
 
@@ -121,7 +119,7 @@ test_expect_success 'commit.verbose true and -v -v' '
 
 test_expect_success 'commit.verbose true and --no-verbose' '
 	git -c commit.verbose=true commit --amend --no-verbose &&
-	! test -s out
+	test_line_count = 0 out
 '
 
 test_expect_success 'commit.verbose false and --verbose' '
@@ -136,12 +134,12 @@ test_expect_success 'commit.verbose false and -v -v' '
 
 test_expect_success 'commit.verbose false and --verbose omitted' '
 	git -c commit.verbose=false commit --amend &&
-	! test -s out
+	test_line_count = 0 out
 '
 
 test_expect_success 'commit.verbose false and --no-verbose' '
 	git -c commit.verbose=false commit --amend --no-verbose &&
-	! test -s out
+	test_line_count = 0 out
 '
 
 test_expect_success 'status ignores commit.verbose=true' '