From: Jeff King <peff@peff.net>
Date: Tue, 10 May 2011 19:43:34 GMT
Subject: Re: [PATCH] Test atomic git-commit --interactive
Message-ID: <20110510194334.GA14456@sigill.intra.peff.net>
In-Reply-To: <1305054751-12327-1-git-send-email-conrad.irwin@gmail.com>

On Tue, May 10, 2011 at 12:12:31PM -0700, Conrad Irwin wrote:

> +test_expect_success PERL \
> +	"--interactive doesn't change index if editor aborts" \
> +	"EDITOR=false echo zoo >file && \

This EDITOR bit does nothing, since it sets the value only for the
"echo" command. The test happens to do what you want, though, since we
set EDITOR to ":" in test-lib.sh, and that is also a fine value for your
test (it's probably a better test, anyway; it simulates the user
aborting with an empty message instead of the editor crashing).

Even though the implicit EDITOR works, I think it makes sense to be
explicit that it is something we are relying on. So we need to put it in
front of the "git commit" invocation, like:

  EDITOR=: test_must_fail git commit --interactive

Unfortunately, some shells do behave reasonably when setting single-shot
environment variables with functions, so we are stuck with:

  (EDITOR=: && export EDITOR &&
   test_must_fail git commit --interactive)

> +	test_must_fail git diff --exit-code > diff1 && \

You can drop the backslash-continuation on lines that end with "&&" or
"|", which makes things a bit more readable.

You do still need ones for:

  test_expect_success "description" \
                      "actual test"

The usual style for our tests is:

  test_expect_success 'description' '
          actual test
  '

but this particular script does not follow that, so it's probably more
sensible to follow the surrounding style as you did.

> +	(echo u ; echo '*' ; echo q) |\
> +	test_must_fail git commit --interactive && \
> +	git diff > diff2 && \

Minor style nit: we usually write ">file" without the space.

> +	git diff --no-index diff1 diff2"

When comparing results with a simple diff, we generally use "test_cmp",
so that unrelated tests do not rely on "git diff --no-index".

So the result should look like:

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 7f7f7c7..ccb0b95 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -131,6 +131,16 @@ test_expect_success PERL \
 	"interactive add" \
 	"echo 7 | git commit --interactive | grep 'What now'"
 
+test_expect_success PERL \
+	"--interactive doesn't change index if editor aborts" \
+	"echo zoo >file &&
+	test_must_fail git diff --exit-code >diff1 &&
+	(echo u ; echo '*' ; echo q) |
+	(EDITOR=: && export EDITOR &&
+	 test_must_fail git commit --interactive) &&
+	git diff >diff2 &&
+	test_cmp diff1 diff2"
+
 test_expect_success \
 	"showing committed revisions" \
 	"git rev-list HEAD >current"

-Peff