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

From: Jeff King <peff@peff.net>
Date: May 24, 2012, 20:15 UTC
Message-ID: <20120524201553.GA19990@sigill.intra.peff.net>
In-Reply-To: <7vtxz5wdg8.fsf@alter.siamese.dyndns.org>

On Thu, May 24, 2012 at 01:07:03PM -0700, Junio C Hamano wrote:

Show 9 quoted lines

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

True. Although the chances that they have set GIT_COMMITTER_EMAIL in their environment seem unlikely in that case. In other words, it was broken before, and it is broken now.

Show 9 quoted lines

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

I think it is even grosser than that. We follow-up that match with a search-and-replace using the message-ids we have found as regular expressions, but we do not bother to quote them. So the () metacharacters get interpreted as regular expressions. I suspect something like this would fix it:

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index b473b6d..3171c06 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -251,7 +251,7 @@ check_threading () {
 		}
 		if ($printing) {
 			$h{$1}=$i++ if (/<([^>]+)>/ and !exists $h{$1});
-			for $k (keys %h) {s/$k/$h{$k}/};
+			for $k (keys %h) {s/\Q$k\E/$h{$k}/};
 			print;
 		}
 		print "---\n" if /^From /i;

> > 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 meant the latter. There is no such call, but I can make one. Let me
see how awkward it is.

-Peff