Test "t/t7502-commit.sh" failed

Test "t/t7502-commit.sh" failed

[Jiang Xin]

Test "t/t7502-commit.sh" failed. I guess it's commit
v1.7.9.7-1-gf20f387 which breaks it.

    $ git log -1 --oneline --stat v1.7.9.7-1-gf20f387
    f20f commit: check committer identity more strictly
     builtin/commit.c | 2 +-
     1 file changed, 1 insertion(+), 1 deletion(-)

Result of t/t7502-commit.sh:

not ok - 21 committer is automatic
#
#
#               echo >>negative &&
#               (
#                       sane_unset GIT_COMMITTER_EMAIL &&
#                       sane_unset GIT_COMMITTER_NAME &&
#                       # must fail because there is no change
#                       test_must_fail git commit -e -m "sample"
#               ) &&
#               head -n 8 .git/COMMIT_EDITMSG | \
#               sed "s/^# Committer: .*/# Committer:/" >actual
#               test_i18ncmp expect actual
#

Contents of file expect:

    sample

    # Please enter the commit message for your changes. Lines starting
    # with '#' will be ignored, and an empty message aborts the commit.
    #
    # Author:    A U Thor <author@example.com>
    # Committer:
    #

Contents of file actual:

    sample

-- 
Jiang Xin

Re: Test "t/t7502-commit.sh" failed

[Jeff King]

On Thu, Jul 26, 2012 at 02:27:52PM +0800, Jiang Xin wrote:

> Test "t/t7502-commit.sh" failed. I guess it's commit
> v1.7.9.7-1-gf20f387 which breaks it.
> 
>     $ git log -1 --oneline --stat v1.7.9.7-1-gf20f387
>     f20f commit: check committer identity more strictly
>      builtin/commit.c | 2 +-
>      1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Result of t/t7502-commit.sh:
> 
> not ok - 21 committer is automatic
> #
> #
> #               echo >>negative &&
> #               (
> #                       sane_unset GIT_COMMITTER_EMAIL &&
> #                       sane_unset GIT_COMMITTER_NAME &&
> #                       # must fail because there is no change
> #                       test_must_fail git commit -e -m "sample"
> #               ) &&
> #               head -n 8 .git/COMMIT_EDITMSG | \
> #               sed "s/^# Committer: .*/# Committer:/" >actual
> #               test_i18ncmp expect actual
> #

Hmm. It doesn't fail here, but that is probably because I have my name
set properly in /etc/passwd. I think the test is bogus, though. From the
results you report:

> Contents of file expect:
> 
>     sample
> 
>     # Please enter the commit message for your changes. Lines starting
>     # with '#' will be ignored, and an empty message aborts the commit.
>     #
>     # Author:    A U Thor <author@example.com>
>     # Committer:
>     #
> 
> Contents of file actual:
> 
>     sample

The test is expecting us to write out the commit template for the user
to edit. But the whole point of f20f387 is to fail early, before we ask
the user to edit the template. So the test is trying to check that we
wrote _something_ into the Committer field, even though that something
would not necessarily be used to make the commit object (because the
code path for the commit object is going to be much stricter).

I am not sure that the test is really all that useful. The point seems
to be that we fall back to some kind of system-based ident, but that is
not portable. On some systems we can, and on some we can't, depending on
things like how /etc/passwd and the hostname are configured.

I'll see if I can make it more robust, but I think the right solution
may simply be to rip out the test.

-Peff

Re: Test "t/t7502-commit.sh" failed

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Jul 26, 2012 at 02:27:52PM +0800, Jiang Xin wrote:
> ...
>> not ok - 21 committer is automatic
>> #
>> #
>> #               echo >>negative &&
>> #               (
>> #                       sane_unset GIT_COMMITTER_EMAIL &&
>> #                       sane_unset GIT_COMMITTER_NAME &&
>> #                       # must fail because there is no change
>> #                       test_must_fail git commit -e -m "sample"
>> #               ) &&
>> #               head -n 8 .git/COMMIT_EDITMSG | \
>> #               sed "s/^# Committer: .*/# Committer:/" >actual
>> #               test_i18ncmp expect actual
>> #
>
> Hmm. It doesn't fail here, but that is probably because I have my name
> set properly in /etc/passwd. I think the test is bogus, though. From the
> results you report:
>
>> Contents of file expect:
>> 
>>     sample
>> 
>>     # Please enter the commit message for your changes. Lines starting
>>     # with '#' will be ignored, and an empty message aborts the commit.
>>     #
>>     # Author:    A U Thor <author@example.com>
>>     # Committer:
>>     #
>> 
>> Contents of file actual:
>> 
>>     sample
>
> The test is expecting us to write out the commit template for the user
> to edit. But the whole point of f20f387 is to fail early, before we ask
> the user to edit the template. So the test is trying to check that we
> wrote _something_ into the Committer field, even though that something
> would not necessarily be used to make the commit object (because the
> code path for the commit object is going to be much stricter).
>
> I am not sure that the test is really all that useful. The point seems
> to be that we fall back to some kind of system-based ident, but that is
> not portable.

I think the point is to make sure that the "# Committer:" line is
given to the reader to remind that we took the codepath that comes
up with a committer ident by using untrustworthy heuristics.  You
are correct that the usefulness of the value of system-based ident
varies between systems (that is why it is stripped out with sed),
though.

You earlier gave a reason why f20f387 (commit: check committer identity
more strictly, 2012-07-23) does not have a test for it; I think the
same reason applies why this test is unworkable.

A related tangent; all the test vectors in this script seems to be
too wide, and we probably would want to narrow them for what each
test wants to see.  For example, the test in question only wants to
see "# Committer: <some system based ident>" and it does not matter
if the template was rewritten in future versions of Git so that it
does not begin with "# Please enter...".  Similarly, the one
previous only wants to see "# Author: <different from committer>".

Re: Test "t/t7502-commit.sh" failed

[Jeff King]

On Thu, Jul 26, 2012 at 09:34:27AM -0700, Junio C Hamano wrote:

> >> not ok - 21 committer is automatic
> [...]
> > I am not sure that the test is really all that useful. The point seems
> > to be that we fall back to some kind of system-based ident, but that is
> > not portable.
> 
> I think the point is to make sure that the "# Committer:" line is
> given to the reader to remind that we took the codepath that comes
> up with a committer ident by using untrustworthy heuristics.  You
> are correct that the usefulness of the value of system-based ident
> varies between systems (that is why it is stripped out with sed),
> though.

Ah, right. I was led astray by the crappy test title. When viewed with
the test immediately prior (which checks that "Author:" is shown in the
template), it makes more sense.

> You earlier gave a reason why f20f387 (commit: check committer identity
> more strictly, 2012-07-23) does not have a test for it; I think the
> same reason applies why this test is unworkable.

Right. You can check this only when "git var GIT_COMMITTER_IDENT" works,
and you can check the f20f387 behavior only when it does _not_ work. So
we could do something like:

  (sane_unset GIT_COMMITTER_NAME &&
   sane_unset GIT_COMMITTER_EMAIL &&
   git var GIT_COMMITTER_IDENT >/dev/null) &&
  test_set_prereq AUTOIDENT ||
  test_set_prereq NOAUTOIDENT

  test_expect_success AUTOIDENT \
    'mention auto ident in commit template'
    '...'

  test_expect_success NOAUTOIDENT \
    'git rejects bogus ident before starting editor'
    '...'

But it is somewhat unsatisfying to only get random test coverage
depending on how your system happens to be configured. I guess we
somewhat have that already with the case-insensitivity tests.

Do we want to go that route, or just drop this test completely?

> A related tangent; all the test vectors in this script seems to be
> too wide, and we probably would want to narrow them for what each
> test wants to see.  For example, the test in question only wants to
> see "# Committer: <some system based ident>" and it does not matter
> if the template was rewritten in future versions of Git so that it
> does not begin with "# Please enter...".  Similarly, the one
> previous only wants to see "# Author: <different from committer>".

Agreed. They should probably just i18ngrep for "^# Committer: " or
similar.

-Peff

Re: Test "t/t7502-commit.sh" failed

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Right. You can check this only when "git var GIT_COMMITTER_IDENT" works,
> and you can check the f20f387 behavior only when it does _not_ work. So
> we could do something like:
>
>   (sane_unset GIT_COMMITTER_NAME &&
>    sane_unset GIT_COMMITTER_EMAIL &&
>    git var GIT_COMMITTER_IDENT >/dev/null) &&
>   test_set_prereq AUTOIDENT ||
>   test_set_prereq NOAUTOIDENT
>
>   test_expect_success AUTOIDENT \
>     'mention auto ident in commit template'
>     '...'
>
>   test_expect_success NOAUTOIDENT \
>     'git rejects bogus ident before starting editor'
>     '...'
>
> But it is somewhat unsatisfying to only get random test coverage
> depending on how your system happens to be configured. I guess we
> somewhat have that already with the case-insensitivity tests.
>
> Do we want to go that route, or just drop this test completely?

There are three cases with respect to ident:

 - There is a user-configured one;

 - We derive one from the system and that is syntactically correct,
   but we know from the past experience the system is often
   misconfigured.

 - We derive one from the system and that is empty.

Before your tightening commit, the latter two cases were treated the
same way and gave the reminder to the user.  After the tightening,
these were separated into two and give different results.

Perhaps the tightening was not such a good idea in the first place?
The user would have seen a bad committer ident in the log editing
session without the new code anyway, so perhaps we should have added
a messasge e.g. "Abort the editor session and do not edit further;
fix your ident first--this commit will fail anyway" there, or
something?

The second case can lead to commits that the user may have to redo
with filter-branch, as the command does not even error out.  And we
do want to make sure that the user is given a chance to verify that
the commit will carry a name that the user is happy with with this
test.  I think that is far more important than a user on a system
with broken GECOS field has to run the editor twice.

Re: Test "t/t7502-commit.sh" failed

[Jeff King]

On Thu, Jul 26, 2012 at 12:25:16PM -0700, Junio C Hamano wrote:

> There are three cases with respect to ident:
> 
>  - There is a user-configured one;
> 
>  - We derive one from the system and that is syntactically correct,
>    but we know from the past experience the system is often
>    misconfigured.
> 
>  - We derive one from the system and that is empty.

Yes, modulo s/empty/bogus in some way/ in the last one (e.g., we will
complain about both empty names and bogus hostnames).

> Before your tightening commit, the latter two cases were treated the
> same way and gave the reminder to the user.  After the tightening,
> these were separated into two and give different results.

Sort of. They were _not_ treated the same in the long run, as the second
one is OK, but the third case would eventually fail. It is only that
they were treated the same for the first half of git-commit running,
meaning it got as far as running the editor.

The problem is that the test relied on git-commit reaching a certain
point before failing in case 3. But that is a bad assumption of the
test, and one that actively hurts users (who would rather git fail
early).

> Perhaps the tightening was not such a good idea in the first place?
> The user would have seen a bad committer ident in the log editing
> session without the new code anyway, so perhaps we should have added
> a messasge e.g. "Abort the editor session and do not edit further;
> fix your ident first--this commit will fail anyway" there, or
> something?

We could do that, but why? We _know_ it's going to fail, so why not just
fail?

> The second case can lead to commits that the user may have to redo
> with filter-branch, as the command does not even error out.

Sure. And that's why we show the committer at all. In other words, we
have three levels of confidence with three outcomes:

  1. The user definitely told us who they are. Proceed without warning.

  2. We guessed who the user is, and we have reasonable confidence that
     it is right. Proceed, but warn.

  3. We guessed who the user is, but we know that it is syntactically
     bad. Do not proceed.

That has always been the behavior, and was not changed by the recent
tightening. Only _when_ we stopped proceeding changed, and that is not
something I think this test should care about.

> And we do want to make sure that the user is given a chance to verify
> that the commit will carry a name that the user is happy with with
> this test.  I think that is far more important than a user on a system
> with broken GECOS field has to run the editor twice.

Yes, and they are given that chance. This is not about the behavior of
git, but about stupid assumptions by the test.

I'm close to finished with a series that I think will at least make it
better. Stay tuned.

-Peff

Re: Test "t/t7502-commit.sh" failed

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yes, and they are given that chance. This is not about the behavior of
> git, but about stupid assumptions by the test.

OK.

> I'm close to finished with a series that I think will at least make it
> better. Stay tuned.

;-)

[PATCH 0/6] t7502 fixes

[Jeff King]

On Thu, Jul 26, 2012 at 03:33:59PM -0400, Jeff King wrote:

> I'm close to finished with a series that I think will at least make it
> better. Stay tuned.

Here it is. I was able to replicate the original problem by munging my
/etc/passwd, and I've confirmed that this series fixes it by skipping
the related test. I also added in a test for the early-quit behavior
introduced by f20f387. It can't run everywhere, of course, but it seems
we have at least one tester whose system will run it (and I did confirm
that it works with my munged /etc/passwd).

One other option is that we could introduce a
GIT_PRETEND_THIS_IS_MY_GECOS_NAME variable. That would let us wrong both
sides of the test on all systems. I just hate to add an interface that
will be carried around in production code just for the sake of a test.

  [1/6]: t7502: clean up fake_editor tests
  [2/6]: t7502: properly quote GIT_EDITOR
  [3/6]: t7502: narrow checks for author/committer name in template
  [4/6]: t7502: drop confusing test_might_fail call
  [5/6]: t7502: handle systems where auto-identity is broken
  [6/6]: t7502: test early quit from commit with bad ident

-Peff

[PATCH 1/6] t7502: clean up fake_editor tests

[Jeff King]

Using write_script saves us a few lines of code, and means
we consistently use $SHELL_PATH.

We can also drop the setting of the $pwd variable from
$(pwd). In the first instance, there is no reason to use it
(we can just use $(pwd) directly two lines later, since we
are interpolating the here-document). In the second
instance, it is totally pointless and probably just a
cut-and-paste from the first instance.

Finally, we can use a non-interpolating here document for
the final script, which saves some quoting.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7502-commit.sh | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 181456a..ddce53a 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -266,13 +266,10 @@ test_expect_success 'committer is automatic' '
 	test_i18ncmp expect actual
 '
 
-pwd=`pwd`
-cat >> .git/FAKE_EDITOR << EOF
-#! /bin/sh
-echo editor started > "$pwd/.git/result"
+write_script .git/FAKE_EDITOR <<EOF
+echo editor started > "$(pwd)/.git/result"
 exit 0
 EOF
-chmod +x .git/FAKE_EDITOR
 
 test_expect_success 'do not fire editor in the presence of conflicts' '
 
@@ -300,9 +297,7 @@ test_expect_success 'do not fire editor in the presence of conflicts' '
 	test "$(cat .git/result)" = "editor not started"
 '
 
-pwd=`pwd`
-cat >.git/FAKE_EDITOR <<EOF
-#! $SHELL_PATH
+write_script .git/FAKE_EDITOR <<EOF
 # kill -TERM command added below.
 EOF
 
@@ -339,13 +334,12 @@ test_expect_success 'A single-liner subject with a token plus colon is not a foo
 
 '
 
-cat >.git/FAKE_EDITOR <<EOF
-#!$SHELL_PATH
-mv "\$1" "\$1.orig"
+write_script .git/FAKE_EDITOR <<\EOF
+mv "$1" "$1.orig"
 (
 	echo message
-	cat "\$1.orig"
-) >"\$1"
+	cat "$1.orig"
+) >"$1"
 EOF
 
 echo '## Custom template' >template
-- 
1.7.11.3.8.ge78f547

[PATCH 2/6] t7502: properly quote GIT_EDITOR

[Jeff King]

One of the tests tries to ensure that editor is not run due
to an early failure. However, it needs to quote the pathname
of the trash directory used in $GIT_EDITOR, since git will
pass it along to the shell. In other words, the test would
pass whether the code was correct or not, since the unquoted
editor specification would never run.

We never noticed the problem because the code is indeed
correct, so git-commit never even tried to run the editor.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7502-commit.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index ddce53a..3f9fb55 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -290,7 +290,7 @@ test_expect_success 'do not fire editor in the presence of conflicts' '
 	test_must_fail git cherry-pick -n master &&
 	echo "editor not started" >.git/result &&
 	(
-		GIT_EDITOR="$(pwd)/.git/FAKE_EDITOR" &&
+		GIT_EDITOR="\"$(pwd)/.git/FAKE_EDITOR\"" &&
 		export GIT_EDITOR &&
 		test_must_fail git commit
 	) &&
-- 
1.7.11.3.8.ge78f547

[PATCH 3/6] t7502: narrow checks for author/committer name in template

[Jeff King]

t7502.20 and t7502.21 check that the author and committer
name are mentioned in the commit message template under
certain circumstances. However, they end up checking a much
larger and unnecessary portion of the template. Let's narrow
their checks to the specific lines.

While we're at it, let's give these tests more descriptive
names, so their purposes are more obvious.

Signed-off-by: Jeff King <peff@peff.net>
---
The test just above still checks the "Please write your..." part of the
message. But it is purely about using "-F -e --cleanup=strip", and is
testing whether we correctly include the instructions. I guess we could
limit it to just checking for "^# " or something if we didn't want to
depend on the actual text.

I'm inclined to just leave it for now.

 t/t7502-commit.sh | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 3f9fb55..efecb06 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -235,24 +235,15 @@ test_expect_success 'cleanup commit messages (strip,-F,-e): output' '
 	test_i18ncmp expect actual
 '
 
-echo "#
-# Author:    $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>
-#" >> expect
-
-test_expect_success 'author different from committer' '
+test_expect_success 'message shows author when it is not equal to committer' '
 	echo >>negative &&
 	test_might_fail git commit -e -m "sample" &&
-	head -n 7 .git/COMMIT_EDITMSG >actual &&
-	test_i18ncmp expect actual
+	test_i18ngrep \
+	  "^# Author: *A U Thor <author@example.com>\$" \
+	  .git/COMMIT_EDITMSG
 '
 
-mv expect expect.tmp
-sed '$d' < expect.tmp > expect
-rm -f expect.tmp
-echo "# Committer:
-#" >> expect
-
-test_expect_success 'committer is automatic' '
+test_expect_success 'message shows committer when it is automatic' '
 
 	echo >>negative &&
 	(
@@ -261,9 +252,9 @@ test_expect_success 'committer is automatic' '
 		# must fail because there is no change
 		test_must_fail git commit -e -m "sample"
 	) &&
-	head -n 8 .git/COMMIT_EDITMSG |	\
-	sed "s/^# Committer: .*/# Committer:/" >actual
-	test_i18ncmp expect actual
+	# the ident is calculated from the system, so we cannot
+	# check the actual value, only that it is there
+	test_i18ngrep "^# Committer: " .git/COMMIT_EDITMSG
 '
 
 write_script .git/FAKE_EDITOR <<EOF
-- 
1.7.11.3.8.ge78f547

[PATCH 4/6] t7502: drop confusing test_might_fail call

[Jeff King]

In t7502.20, we run "git commit" and check that it warns us
that the author and committer identity are not the same
(this is always the case in the test environment, since we
set up the idents differently).

Instead of actually making a commit, we have a clean index,
so the "git commit" we run will fail. This is marked as
might_fail, which is not really correct; it will always fail
since there is nothing to commit.

However, the only reason not to do a complete commit would
be to see the intermediate state of the COMMIT_EDITMSG file
when the commit is not completed. We don't need to care
about this, though; even a complete commit will leave
COMMIT_EDITMSG for us to view.  By doing a real commit and
dropping the might_fail, we are more robust against other
unforeseen failures of "git commit" that might influence our
test result.

It might seem less robust to depend on the fact that "git
commit" leaves COMMIT_EDITMSG in place after a successful
commit. However, that brings this test in line with others
parts of the script, which make the same assumption.
Furthermore, if that ever does change, the right solution is
not to prevent commit from completing, but to set EDITOR to
a script that will record the contents we see. After all,
the point of these tests is to check what the user sees in
their EDITOR, so that would be the most direct test. For
now, though, we can continue to use the "shortcut" that
COMMIT_EDITMSG is left intact.

Signed-off-by: Jeff King <peff@peff.net>
---
Sorry for the long explanation. It took me a long time to figure out why
this test_might_fail is there at all, so I wanted to present my full
analysis as to why it's OK to remove.

 t/t7502-commit.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index efecb06..d261b82 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -237,7 +237,7 @@ test_expect_success 'cleanup commit messages (strip,-F,-e): output' '
 
 test_expect_success 'message shows author when it is not equal to committer' '
 	echo >>negative &&
-	test_might_fail git commit -e -m "sample" &&
+	git commit -e -m "sample" -a &&
 	test_i18ngrep \
 	  "^# Author: *A U Thor <author@example.com>\$" \
 	  .git/COMMIT_EDITMSG
-- 
1.7.11.3.8.ge78f547

[PATCH 5/6] t7502: handle systems where auto-identity is broken

[Jeff King]

Test t7502.21 checks whether we write the committer name
into COMMIT_EDITMSG when it has been automatically
determined. However, not all systems can produce valid
automatic identities.

Prior to f20f387 (commit: check committer identity more
strictly), this test worked even when we did not have a
valid automatic identity, since it did not run the strict
test until after we had generated the template. That commit
tightened the check to fail early (since we would fail
later, anyway), meaning that systems without a valid GECOS
name or hostname would fail the test.

We cannot just work around this, because it depends on
configuration outside the control of the test script.
Therefore we introduce a new test_prerequisite to run this
test only on systems where automatic ident works at all.

As a result, we can drop the confusing test_must_fail bit
from the test. The intent was that by giving "git commit"
invalid input (namely, nothing to commit), that it would
stop at a predictable point, whether we had a valid identity
or not, from which we could view the contents of
COMMIT_EDITMSG. Since that assumption no longer holds, and
we can only run the test when we have a valid identity,
there is no reason not to let commit run to completion. That
lets us be more robust to other unforeseen failures.

Signed-off-by: Jeff King <peff@peff.net>
---
This should fix the test failure that started the thread. Please let me
know if it doesn't.

 t/t7502-commit.sh | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index d261b82..c444812 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -243,14 +243,21 @@ test_expect_success 'message shows author when it is not equal to committer' '
 	  .git/COMMIT_EDITMSG
 '
 
-test_expect_success 'message shows committer when it is automatic' '
+test_expect_success 'setup auto-ident prerequisite' '
+	if (sane_unset GIT_COMMITTER_EMAIL &&
+	    sane_unset GIT_COMMITTER_NAME &&
+	    git var GIT_COMMITTER_IDENT); then
+		test_set_prereq AUTOIDENT
+	fi
+'
+
+test_expect_success AUTOIDENT 'message shows committer when it is automatic' '
 
 	echo >>negative &&
 	(
 		sane_unset GIT_COMMITTER_EMAIL &&
 		sane_unset GIT_COMMITTER_NAME &&
-		# must fail because there is no change
-		test_must_fail git commit -e -m "sample"
+		git commit -e -m "sample" -a
 	) &&
 	# the ident is calculated from the system, so we cannot
 	# check the actual value, only that it is there
-- 
1.7.11.3.8.ge78f547

[PATCH 6/6] t7502: test early quit from commit with bad ident

[Jeff King]

In commit f20f387, "git commit" notices and dies much
earlier when we have a bogus commit identity. That commit
did not add a test because we cannot do so reliably (namely,
we can only trigger the behavior on a system where the
automatically generated identity is bogus). However, now
that we have a prerequisite check for this feature, we can
add a test that will at least run on systems that produce
such a bogus identity.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7502-commit.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index c444812..deb187e 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -248,6 +248,8 @@ test_expect_success 'setup auto-ident prerequisite' '
 	    sane_unset GIT_COMMITTER_NAME &&
 	    git var GIT_COMMITTER_IDENT); then
 		test_set_prereq AUTOIDENT
+	else
+		test_set_prereq NOAUTOIDENT
 	fi
 '
 
@@ -269,6 +271,21 @@ echo editor started > "$(pwd)/.git/result"
 exit 0
 EOF
 
+test_expect_success NOAUTOIDENT 'do not fire editor when committer is bogus' '
+	>.git/result
+	>expect &&
+
+	echo >>negative &&
+	(
+		sane_unset GIT_COMMITTER_EMAIL &&
+		sane_unset GIT_COMMITTER_NAME &&
+		GIT_EDITOR="\"$(pwd)/.git/FAKE_EDITOR\"" &&
+		export GIT_EDITOR &&
+		test_must_fail git commit -e -m sample -a
+	) &&
+	test_cmp expect .git/result
+'
+
 test_expect_success 'do not fire editor in the presence of conflicts' '
 
 	git clean -f &&
-- 
1.7.11.3.8.ge78f547

Re: [PATCH 0/6] t7502 fixes

[Junio C Hamano]

All patches look very sensible.  Thanks.