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.
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 suspect that is not especially sane, but at the same time, it is not
hard to support. The patch below (on top of jk/ident-gecos-strbuf)
should fix it.
-- >8 --
Subject: format-patch: use GIT_COMMITTER_EMAIL when making message ids
Before commit 43ae9f4, we generated the tail of a message id
by calling git_committer_info and parsing the email out of
the result. 43ae9f4 changed to use ident_default_email
directly, so we didn't have to bother with parsing. As a
side effect, it meant we no longer used GIT_COMMITTER_EMAIL
at all.
In general, this is probably reasonable behavior. Either the
default email is sane on your system, or you are using
user.email to provide something sane. The exception is if
you rely on GIT_COMMITTER_EMAIL being set all the time to
override the bogus generated email.
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.
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. It also strikes me as a little ugly that this code path
needs to care about $GIT_COMMITTER_EMAIL at all. I can rework the ident
interface to provide a more sanitized broken-down version of the ident
if we care.
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);
}
--
1.7.10.1.25.g7031a0f
--
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