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