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

From: Jeff King <peff@peff.net>
Date: Jul 26, 2012, 17:12 UTC
Message-ID: <20120726171256.GC13942@sigill.intra.peff.net>
In-Reply-To: <7vtxwu8orw.fsf@alter.siamese.dyndns.org>

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

Show 12 quoted lines

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

Show 7 quoted lines

> 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