On Sun, Mar 27, 2016 at 10:57 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > On Sun, Mar 27, 2016 at 03:27:25PM +0200, SZEDER Gábor wrote: >> > > +! 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. Sorry to create extra noise. This works perfectly fine. I had put an 'exec' locally. But now I have tested this and it works fine.. > > 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. Sorry for this. I might have just started typing and forgot to click reply all. >> 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' ' > -- -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html