From: Jeff King <peff@peff.net>
Date: Tue, 22 May 2012 17:12:04 GMT
Subject: Re: [PATCHv2 14/15] ident: trim whitespace from default name/email
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:

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

> 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