Jeff King [off-list ref] writes:
quoted hunk
On Sat, Feb 04, 2012 at 05:46:05PM +0200, Felipe Contreras wrote:
quoted
In any case, the one to blame for the header corruption is git:
[...]
f2bb9f88 ([off-list ref]> 2006-11-27 03:41:01 -0500 952)
Notice the mail is wrong.
...
diff --git a/builtin/blame.c b/builtin/blame.c
index 5a67c20..9b886fa 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1406,7 +1406,8 @@ static void get_ac_line(const char *inbuf, const char *what,
/* Add a trailing '>' to email, since map_user returns plain emails
Note: It already has '<', since we replace from mail+1 */
mailpos = memchr(mail, '\0', mail_len);
- if (mailpos && mailpos-mail < mail_len - 1) {
+ if (mailpos && mailpos-mail < mail_len - 1 &&
+ mailpos > mail && *(mailpos-1) != '>') {
*mailpos = '>';
*(mailpos+1) = '\0';
}
but it feels like the fix should go into map_user.
Thanks.
The map_user() API takes an email address that is terminated by either NUL
or '>' to allow the caller to learn the corresponding up-to-date email
address that is NUL terminated, while indicating with its return value
that if the caller even needs to replace what it already has. But the
function does not properly terminate email when it only touched the name
part. And I think that is the real bug.
So I agree that the real fix should go to map_user() so that when it says
"I've updated something, so pick up the updated result from the i/o
arguments you gave me, i.e. email and name", it makes sure what it claims
to be an e-mail address does not have the extra '>' in it.
Working around the current behaviour by forcing all callers that pass '>'
terminated e-mail address to have the code like the above quoted patch
does not feel right.
Junio C Hamano [off-list ref] writes:
Jeff King [off-list ref] writes:
...
quoted
but it feels like the fix should go into map_user.
Thanks.
The map_user() API takes an email address that is terminated by either NUL
or '>' to allow the caller to learn the corresponding up-to-date email
address that is NUL terminated, while indicating with its return value
that if the caller even needs to replace what it already has. But the
function does not properly terminate email when it only touched the name
part. And I think that is the real bug.
And the gist of the patch to fix the bug would look like this two liner.
In the real fix, "p" should be renamed to "end_of_email" or something
descriptive like that.
I only made sure that this fixes the original case of the email address of
Shawn reported by Felipe, but other than that like everything else I send
here with "... would look like this", not tested beyond "it compiles".
But conceptually it looks correct (famous last words ;-).
Felipe, does it pass your test cases?
mailmap.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/mailmap.c b/mailmap.c
index 8c3196c..ce805fa 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -236,6 +236,8 @@ int map_user(struct string_list *map,
}
if (maxlen_email && mi->email)
strlcpy(email, mi->email, maxlen_email);
+ else
+ *p = '\0';
if (maxlen_name && mi->name)
strlcpy(name, mi->name, maxlen_name);
debug_mm("map_user: to '%s' <%s>\n", name, mi->email ? mi->email : "");On Sun, Feb 05, 2012 at 01:38:19PM -0800, Junio C Hamano wrote:
quoted
The map_user() API takes an email address that is terminated by either NUL
or '>' to allow the caller to learn the corresponding up-to-date email
address that is NUL terminated, while indicating with its return value
that if the caller even needs to replace what it already has. But the
function does not properly terminate email when it only touched the name
part. And I think that is the real bug.
And the gist of the patch to fix the bug would look like this two liner.
In the real fix, "p" should be renamed to "end_of_email" or something
descriptive like that.
Exactly; this is much better.
We could also go as far as saying that map_user would _always_ terminate
in this way (i.e., the caller gets a munged result, whether we found
anything or not). Then internally, map_user could be simplified to stop
worrying about making a temporary copy in mailbuf. And callers could
simply call map_user without worrying about branching on whether it
found anything or not.
But maybe it is not worth that level of refactoring. From my reading,
your patch fixes the problem just fine.
-Peff