On Tue, May 22, 2012 at 09:55:19AM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html