On Thu, May 24, 2012 at 01:07:03PM -0700, Junio C Hamano wrote:
> > 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.
> > 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
--
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