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

On Sun, May 20, 2012 at 07:54:24PM -0700, Junio C Hamano wrote:

I found the issue. The test failure is actually introduced by patch 06/13 (format-patch: use default email for generating message ids). But then it later gets fixed in 09/13 (drop length limitations on gecos-derived names and emails).

The problems turns out to be a latent bug hidden in add_mailname_host. It uses fgets to read from /etc/mailname, but never trims the trailing newline. So git_default_email ends up set to something like "peff@peff.net\n". Most of the time, this bug gets covered up by the normalization that happens in fmt_ident (where we remove cruft characters like newline). But in patch 6, we start using it directly instead of re-parsing the output of fmt_ident, introducing a noticeable bug.

I suspect there is also a bug in http-push.c, which directly uses git_default_email as part of the xml in the dav request. I don't know if it actually matters there or not. And of course this triggers only on systems with /etc/mailname, and only when you don't have user.email set. So it may just be that the combination of that and the fact that hardly anybody uses dumb http push has caused it to remain hidden.

The bug later ends up fixed in patch 9, because we replace fgets with strbuf_getline, which trims the newline for us. Placing the patch below anywhere before patch 6 fixes the issue (and then patch 9 would need to later remove the code).

It does raise a subtle issue, though: should we be trimming whitespace and other undesirable characters from git_default_email (or git_default_name)? We don't currently, and it ends up OK because the result typically is fed through fmt_ident, which cleans it up. But:

  1. We do look at the git_default_* variables for things like deciding
     whether the name is blank. So if your gecos field was " ", I think
     that would fool git into thinking it had something useful, and skip
     the IDENT_ERROR_ON_NO_NAME check, even though fmt_ident would
     produce an empty name.

  2. We don't always feed it through fmt_ident (the http-push.c callsite
     I mentioned above, and now patch 6 adds another one).

So I think my preference would be:

  - apply the patch below as 5.5/13

  - tweak patch 9 to remove the extra trimming

  - add a patch 14 to call strbuf_trim on the name and email buffers
    after reading them from system files.

-- >8 --
Subject: [PATCH] ident: trim trailing whitespace from /etc/mailname

Otherwise we will typically end up with an extra newline in our git_default_email. Most of the time this doesn't matter, as fmt_ident will skip it as cruft, but there is one code path that accesses it directly (in http-push.c:lock_remote).

Let's trim any trailing space or line termination when we read from /etc/mailname.

Signed-off-by: Jeff King <peff@peff.net>
---
 ident.c | 3 +++
 1 file changed, 3 insertions(+)