Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails

From: Jeff King <peff@peff.net>
Date: May 15, 2012, 01:54 UTC
Message-ID: <20120515015437.GA13833@sigill.intra.peff.net>
In-Reply-To: <20120514211324.GA11578@sigill.intra.peff.net>

On Mon, May 14, 2012 at 05:13:24PM -0400, Jeff King wrote:

Show 6 quoted lines

> where we are not careful. The fix is trivial. However, while examining
> fmt_ident, I notice there is another potential spot there that needs
> further investigation (I think it may actually be unreachable code, but
> I need to look closer).
> 
> I'll re-roll the series with the fixes after investigating fmt_ident.

Hmm. This code from fmt_ident is very odd:

Show 23 quoted lines

> const char *fmt_ident(const char *name, const char *email,
> 		      const char *date_str, int flag)
> {
> [...]
> 	setup_ident(&name, &email);
> 
> 	if (!*name) {
> 		struct passwd *pw;
> 
> 		if ((warn_on_no_name || error_on_no_name) &&
> 		    name == git_default_name && env_hint) {
> 			fputs(env_hint, stderr);
> 			env_hint = NULL; /* warn only once */
> 		}
> 		if (error_on_no_name)
> 			die("empty ident %s <%s> not allowed", name, email);
> 		pw = getpwuid(getuid());
> 		if (!pw)
> 			die("You don't exist. Go away!");
> 		strlcpy(git_default_name, pw->pw_name,
> 			sizeof(git_default_name));
> 		name = git_default_name;
> 	}

We call setup_ident with our name pointer, which usually comes from getenv("GIT_*_NAME"), although could also come from something like "git commit -c $commit". We feed that to setup_ident. If name is NULL, then setup_ident will use git_default_name (filling it in from gecos or config). If it's not NULL, then we use it literally. And then we check _that_ result to see if it's empty. If it is, we either die or warn, depending on the flags. In the latter case, we fallback to using the username as the name.

And that's what confuses me. Depending on what was passed in, we may have checked that GIT_COMMITTER_NAME is an empty string, or we may have checked that the config or gecos field yielded an empty string. In the latter case, it makes sense to fall back to the username. But in the former case, it doesn't; we should fall back to the config name or the gecos name. And worse, we've polluted git_default_name for the rest of the program run.

Instead of falling back to getpwuid(), should it fall back to:

   /* If this wasn't our default name already, then fall back to that. */
   if (name != git_default_name) {
           name = NULL;
           setup_ident(&name, &email);
   }

   /* If we _still_ don't have a non-empty name, then fall back to
    * username. */
   if (!*name) {
          pw = getpwuid(getuid());
          if (!pw)
                  die("You don't exist. Go away!");
          strlcpy(git_default_name, pw->pw_name, sizeof(git_default_name));
          nae = git_default_name;
   }

Of course we've still polluted this crappy fake name into git_default_name, so that later calls with error_on_no_name will see it and not error. I think so far it hasn't mattered because the only user of this "warn" code is format-patch, which otherwise does not care about ident (and doesn't even end up using the name at all!). And I doubt this code path gets triggered much anyway; do people really run "GIT_COMMITTER_NAME= git format-patch"?

I can just leave it as it's not really hurting anybody, I think. But I was refactoring in the area and it just seemed flaky and questionable. I wonder if we can simply get rid of the IDENT_WARN_ON_NO_NAME code path entirely. The use here is grabbing the email address to use as part of a message id. Could we just call setup_ident and then read from git_default_email directly? There's no need to respect GIT_COMMITTER_EMAIL here at all.

-Peff