Re: [PATCHv2 14/15] ident: trim whitespace from default name/email

From: Jeff King <peff@peff.net>
Date: May 22, 2012, 17:12 UTC
Message-ID: <20120522171204.GB11600@sigill.intra.peff.net>
In-Reply-To: <7vipfo5f60.fsf@alter.siamese.dyndns.org>

On Tue, May 22, 2012 at 09:55:19AM -0700, Junio C Hamano wrote:

Show 8 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > ... Any other value
> > comes from a config file, where we will have cleaned up
> > accidental whitespace already.
> 
> Are you referring to the behaviour of the config parser that removes
> leading and trailing whitespaces when reading an unquoted string value?

Yes, exactly.

> If the user really wanted to have trailing whitespaces by quoting, we
> would let it pass, in other words; it probably is a reasonable behaviour.

Right, that's why I said "accidental" above; you can still do it, but I think you'd have to be really trying. But I really wonder if we should be blocking that, too. fmt_ident will drop it anyway, so this is really only for code paths that want to use the email directly.

> The same can be said about the environment variable GIT_COMMITTER_NAME and
> friends, but "accidental whitespaces are cleaned up already" does not
> apply to them.

Yes, but those variables don't hit this code path at all. They go straight to fmt_ident, which does the cleanup. If you have code like this:

  email = getenv("GIT_COMMITTER_EMAIL");
  if (!email)
          email = ident_default_email();

you might have trim-able spaces in the getenv result, and nobody is handling that (fmt_ident does, but accessing it directly does not). We could go a step further and replace ident_default_email with a git_committer_email() which does the above, plus any whitespace trimming.

> So, isn't the real rationale behind this choice to allow users who give
> leading or trailing whitespaces in the configuration and environment
> variables on purpose use whatever value they specified?

To be honest, the real rationale was laziness and a desire to keep the interfaces simple. The risky code path (in my opinion) for getting extra whitespace is pulling data from gecos, or from a file like /etc/mailname. IOW, my intent was to block common accidents, but not worry about somebody intentionally trying to push whitespace through.

Show 6 quoted lines

> I agree with the placement of trimming in this patch, but I do not quite
> get (I do not mean "I do not agree with") what the quoted sentence wanted
> to say.
> 
> Other than that single small "hrm...", the entire series was a pleasant
> read.  Thanks.

Thanks. I'm happy to update this patch if we want it to be more paranoid, or I can update the commit message if it just needs explained better.

-Peff