Jeff King <peff@xxxxxxxx> writes:
> On Thu, May 24, 2012 at 02:01:57PM +0200, Michael Haggerty wrote:
>
>> On my setup, the above commit causes 12 tests in t4014 to fail. For
>> example, test 25:
>>
>> >-Message-Id: <0>
>> >+Message-Id: <1135adfeed86678c55e1aad7c568046ee8215660.1337860646.git.mhagger@michael.(none)>
>
> Thanks for the report. I know exactly what the issue is, as it came up
> in the discussion of the original series. 43ae9f47ab stopped using
> git_committer_info (which looks at $GIT_COMMITTER_EMAIL) for the end of
> the message-id and started using the default-generated email directly.
>
> Nobody should care, because either:
>
> 1. The defaults set up a reasonable hostname for your machine.
>
> 2. They do not, but you adjust it by setting user.email. Otherwise,
> your author ident would have this bogus email in it.
>
> The only setup that _would_ care is if the generated default is bogus
> and you set $GIT_COMMITTER_EMAIL in the environment and relied on that
> to get a sane value. Which is exactly what the test environment does.
Or they worked to create their series in a good machine, pull it down to
another machine during their lunch break, and run format-patch to send it
out after the final eyeballing. Perhaps they are not supposed to be
working on the project in question during the day at work, so the work
machine does not have user.email set up correctly yet.
> The question is, is what it is doing sane and something we should care
> about? Or is the test broken (it fails to parse the message-id that
> contains ".(none)", but I am not even sure that is intentional and not
> simply lazy regex writing in the test).
I doubt that it was carefully written to try to exclude ".(none)".
It somewhat curious---it seems to want to grab everything after "<" up to
the first occurrence of ">"---why isn't this pattern matching?
> This is unlikely to match anybody's real-life setup, but we do use it in
> the test environment. And furthermore, it's what we have always done,
> and the change in 43ae9f4 was about cleaning up, not fixing any bug; we
> should be conservative and keep the behavior identical.
I agree with the last sentence. I wouldn't be surprised if somebody was
indexing patch e-mails on the list, keyed with their Message-Id: field,
and using the key as something more than just a random "uniqueness" token.
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> Note that we check the environment outside of the usual strbuf_trim that
> happens to the default email. And outside of fmt_ident, which trims
> whitespace, as well. So compared to the state before this series,
> something like:
>
> GIT_COMMITTER_EMAIL="$(printf 'foo@bar\n')" git format-patch ...
>
> is now broken.
True, and setting GIT_COMMITTER_EMAIL to that value will not break other
uses because they trim. But as we discussed before, we do not strongly
stop users from deliberately adding unnecessary whitespaces around names
and e-mails that they themselves configure (i.e. user.email and these
environment variables), so it probably is not a huge deal.
> It also strikes me as a little ugly that this code path
> needs to care about $GIT_COMMITTER_EMAIL at all.
Do you mean "why committer and not author"? It primarily is because we
want to see "who is this person who wants a unique token tied to his
identity" and author and committer ident are both equally reasonable
choices. But we have picked to use committer in these cases long time
ago.
If you mean "why environment and not an API call?", then I would have to
agree. ident_committer_email() call, that returns a sanitized version,
would have been a natural way to write this, if it were available.
> I can rework the ident interface to provide a more sanitized broken-down
> version of the ident if we care.
That is a healthy thing to do in the longer term, I would guess.
> builtin/log.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 8010a40..3f1883c 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -739,8 +739,11 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha
> static void gen_message_id(struct rev_info *info, char *base)
> {
> struct strbuf buf = STRBUF_INIT;
> + const char *email = getenv("GIT_COMMITTER_EMAIL");
> + if (!email)
> + email = ident_default_email();
> strbuf_addf(&buf, "%s.%lu.git.%s", base,
> - (unsigned long) time(NULL), ident_default_email());
> + (unsigned long) time(NULL), email);
> info->message_id = strbuf_detach(&buf, NULL);
> }
--
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