t4014 broken by 43ae9f47ab: format-patch: use default email for generating message ids

On my setup, the above commit causes 12 tests in t4014 to fail.  For 
example, test 25:

Beyond bisecting, I haven't looked into this problem further.  Let me 
know if you need more info.

Michael

-- 
Michael Haggerty
Head of Software Development

JPK Instruments AG
Bouchéstr. 12
12435 Berlin, Germany

tel:  +49 30 5331 12070
fax:  +49 30 5331 22555
mail: haggerty@jpk.com
web:  www.jpk.com, www.nanobioviews.net

Aufsichtsrat: Dr. Franz-Ferdinand von Falkenhausen (Vorsitzender)
Vorstand:     Torsten Jähnke, Frank Pelzer, Jörn Kamps, René Grünberg

JPKinstruments Aktiengesellschaft
Amtsgericht Berlin-Charlottenburg
HRB 75513

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:

quoted

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

On 05/24/2012 07:16 PM, Jeff King wrote:

On Thu, May 24, 2012 at 02:01:57PM +0200, Michael Haggerty wrote:

quoted

On my setup, the above commit causes 12 tests in t4014 to fail.  For
example, test 25:

quoted

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

I'm trying hard not to get sucked into this topic (I just want the test 
suite to work again!) but I infer that the reason for the failure in my 
setup is that I have a global user.name but no global user.email 
configured.  I want git to remind me to configure user.email at the 
repository level so that I can set my work email address for proprietary 
projects and my personal email for open-source projects.

Ignorant idea: since this test is executed in a test repo, would it help 
to set a dummy user.name and user.email at the test repository level 
using "git config", perhaps as part of the standard test repo setup?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

On Thu, May 24, 2012 at 10:49:55PM +0200, Michael Haggerty wrote:

quoted

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.

I'm trying hard not to get sucked into this topic (I just want the
test suite to work again!) but I infer that the reason for the
failure in my setup is that I have a global user.name but no global
user.email configured.

No, not at all. The problem is that the test suite does not look in your
.gitconfig at all (nor should it), but rather than providing its own
sensible gitconfig, it relies on the environment variables. Your
personal setup should not be relevant; only the fact that your machine
happens to not have a fully qualified hostname.

I want git to remind me to configure user.email at the repository
level so that I can set my work email address for proprietary projects
and my personal email for open-source projects.

What you're doing is sane. However, I suspect that format-patch silently
generates bogus message-ids when you do not have your user.email set
(and it always has). I wonder if it is worth having it barf when
"(none)" is in the email.

For that matter, I really wonder if the "(none)" fallback even makes
sense these days. We encourage people to set up user.*. But I guess it
helps people on remote servers which write reflogs during a push; it is
better to write junk into the reflog than to fail the push.

Ignorant idea: since this test is executed in a test repo, would it
help to set a dummy user.name and user.email at the test repository
level using "git config", perhaps as part of the standard test repo
setup?

Yes, that would solve your test failure. On the other hand, not having
it has spurred more discussion of this situation, so maybe there is some
good in having it that way.

At any rate, there is a slight complication, because tests sometimes
make their own sub-repositories. A more sensible solution would be to
put it in $HOME/.gitconfig, but that is also complicated. The tests have
$HOME set to the repository directory, so the extra cruft there would
cause some tests to fail (and splitting them into two directories would
likewise cause other tests to fail). That can be remedied, of course,
but it is definitely not a one-line fix.

I think at this point I favor just fixing your bug, either with the
patch I just posted, or a slightly nicer series refactoring fmt_ident to
handle this situation better (which I am working up right now).

-Peff