[PATCH 08/13] ident: don't write fallback username into git_default_name

The fmt_ident function gets a flag that tells us whether to die if the name field is blank. If it is blank and we don't die, then we fall back to the username from the passwd file.

The current code writes the value into git_default_name. However, that's not necessarily correct, as the empty value might have come from git_default_name, or it might have been passed in. This leads to two potential problems:

  1. If we are overriding an empty name in the passed-in
     value, then we may be overwriting a perfectly good name
     (from gitconfig or gecos) in the git_default_name
     buffer. Later calls to fmt_ident will end up using the
     fallback name, even though a better name was available.

  2. If we override an empty gecos name, we end up with the
     fallback name in git_default_name. A later call that
     uses IDENT_ERROR_ON_NO_NAME will see the fallback name
     and think that it is a good name, instead of producing
     an error. In other words, a blank gecos name would
     cause an error with this code:

       git_committer_info(IDENT_ERROR_ON_NO_NAME);

     but not this:

       git_committer_info(0);
       git_committer_info(IDENT_ERROR_ON_NO_NAME);

     because in the latter case, the first call has polluted
     the name buffer.

Instead, let's make the fallback a per-invocation variable. We can just use the pw->pw_name string directly, since it only needs to persist through the rest of the function (and we don't do any other getpwent calls).

Note that while this solves (1) for future invocations of fmt_indent, the current invocation might use the fallback when it could in theory load a better value from git_default_name. However, by not passing IDENT_ERROR_ON_NO_NAME, the caller is indicating that it does not care too much about the name, anyway, so we don't bother; this is primarily about protecting future callers who do care.

Signed-off-by: Jeff King <peff@peff.net>
---
You can trigger bug (2) by having an empty gecos field, no user.name,
and running a merge. The recent credit_person code path will call
git_committer_info(0), then the commit-generating code path will call
git_committer_info(IDENT_ERROR_ON_NO_NAME), and you will end
up with a commit with your username in the "name" field (even though the
second call would want to error out).

I don't think (1) is triggerable in the current code, mainly because we typically are feeding getenv("GIT_*_NAME") to fmt_ident, and a blank there will end up causing an error later on in the program.

By the way, I left it so that:

  GIT_COMMITTER_NAME= git ...

will continue to fallback to the passwd username if a caller doesn't request to die on a blank name. We could pretty easily die in that case, but people with flaky scripts might be affected. I figured that callers who don't give ERROR_ON_NO_NAME don't really care too much what's in the name anyway, so there's not much point in tightening the rules.

 ident.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)