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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 26, 2012, 19:25 UTC
Message-ID: <7v8ve672ar.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120726171256.GC13942@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 23 quoted lines

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