Jeff King [off-list ref] writes:
Ugh, yeah. I was thinking about how it would improve this call site, but
I don't want to get into auditing the others. Let's drop it and go with
your patch.
In any case, here is what I queued for tonight.
-- >8 --
Subject: [PATCH] mailmap: do not leave '>' in the output when answering "we did something"
The callers of map_user() give email and name to it, and expect to get an
up-to-date versions of email and/or name to be used in their output. The
function rewrites the given buffers in place. To optimize the majority of
cases, the function returns 0 when it did not do anything, and it returns
1 when the caller should use the updated contents.
The 'email' input to the function is terminated by '>' or a NUL (whichever
comes first) for historical reasons, but when a rewrite happens, the value
is replaced with the mailbox inside the <> pair. However, it failed to
meet this expectation when it only rewrote the name part without rewriting
the email part, and the email in the input was terminated by '>'.
This causes an extra '>' to appear in the output of "blame -e", because the
caller does send in '>'-terminated email, and when the function returned 1
to tell it that rewriting happened, it appends '>' that is necessary when
the email part was rewritten.
The patch looks bigger than it actually is, because this change makes a
variable that points at the end of the email part in the input 'p' live
much longer than it used to, deserving a more descriptive name.
Noticed and diagnosed by Felipe Contreras and Jeff King.
Signed-off-by: Junio C Hamano <redacted>
---
mailmap.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/mailmap.c b/mailmap.c
index 8c3196c..47aa419 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -190,27 +190,27 @@ void clear_mailmap(struct string_list *map)
int map_user(struct string_list *map,
char *email, int maxlen_email, char *name, int maxlen_name)
{
- char *p;
+ char *end_of_email;
struct string_list_item *item;
struct mailmap_entry *me;
char buf[1024], *mailbuf;
int i;
/* figure out space requirement for email */
- p = strchr(email, '>');
- if (!p) {
+ end_of_email = strchr(email, '>');
+ if (!end_of_email) {
/* email passed in might not be wrapped in <>, but end with a \0 */
- p = memchr(email, '\0', maxlen_email);
- if (!p)
+ end_of_email = memchr(email, '\0', maxlen_email);
+ if (!end_of_email)
return 0;
}
- if (p - email + 1 < sizeof(buf))
+ if (end_of_email - email + 1 < sizeof(buf))
mailbuf = buf;
else
- mailbuf = xmalloc(p - email + 1);
+ mailbuf = xmalloc(end_of_email - email + 1);
/* downcase the email address */
- for (i = 0; i < p - email; i++)
+ for (i = 0; i < end_of_email - email; i++)
mailbuf[i] = tolower(email[i]);
mailbuf[i] = 0;
@@ -236,6 +236,8 @@ int map_user(struct string_list *map,
}
if (maxlen_email && mi->email)
strlcpy(email, mi->email, maxlen_email);
+ else
+ *end_of_email = '\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 : "");--
1.7.9.204.gdf845
On Sun, Feb 05, 2012 at 07:16:22PM -0800, Junio C Hamano wrote:
In any case, here is what I queued for tonight.
-- >8 --
Subject: [PATCH] mailmap: do not leave '>' in the output when answering "we did something"
Looks good to me.
The callers of map_user() give email and name to it, and expect to get an
up-to-date versions of email and/or name to be used in their output. The
Minor grammar error.
-Peff
On Mon, Feb 6, 2012 at 5:16 AM, Junio C Hamano [off-list ref] wrote:
Jeff King [off-list ref] writes:
quoted
Ugh, yeah. I was thinking about how it would improve this call site, but
I don't want to get into auditing the others. Let's drop it and go with
your patch.
In any case, here is what I queued for tonight.
-- >8 --
Subject: [PATCH] mailmap: do not leave '>' in the output when answering "we did something"
This subject doesn't explain the *purpose* of the patch: always return
a plain mail address from map_user()
That would be enough for most readers.
I think the immediate problem should be here:
Currently 'git blame -e' would add an extra '>' if map_user() returns
true, which would end up as '[off-list ref]>'. This is because
map_user() sometimes modifies, the mail string, but sometimes not. So
let's always modify it.
At this point a lot of readers can skip the rest of the explanation.
The callers of map_user() give email and name to it, and expect to get an
up-to-date versions of email and/or name to be used in their output. The
function rewrites the given buffers in place. To optimize the majority of
cases, the function returns 0 when it did not do anything, and it returns
1 when the caller should use the updated contents.
The 'email' input to the function is terminated by '>' or a NUL (whichever
comes first) for historical reasons, but when a rewrite happens, the value
is replaced with the mailbox inside the <> pair. However, it failed to
meet this expectation when it only rewrote the name part without rewriting
the email part, and the email in the input was terminated by '>'.
With the above explanation I suggested, I think this can be summarized as:
As of right now most of the callers of map_user() give a plan address,
and expect a plain address, however, 'git blame -e' passes along a
mail address that ends with >, and uses the return value to determine
if a transformation was made, and adds the missing '>'. This is
because when map_user() does the transformation, it returns a plan
address.
This might have worked in previous versions of map_user(), but now not
only does it transforms mail addresses, but also the name (phrase). So
now callers can't use the return value to know if they need to add an
extra '>', or not. So lets always remove the '>', so they can.
This causes an extra '>' to appear in the output of "blame -e", because the
caller does send in '>'-terminated email, and when the function returned 1
to tell it that rewriting happened, it appends '>' that is necessary when
the email part was rewritten.
It took too long to reach the problem. As a reader, that's the first
thing I would expect.
The patch looks bigger than it actually is, because this change makes a
variable that points at the end of the email part in the input 'p' live
much longer than it used to, deserving a more descriptive name.
This is a *separate* logically independent patch. And you yourself
mention, makes the review of the patch more difficult, as of right now
it's difficult to spot the functional change, because it's mixed with
non-functional changes.
I find it peculiar how patches in the Linux kernel are truly logically
independent, but Git has a tendency to squash many things: cleanups,
fixes, documentation, tests, extra tests, remove unused code, etc.
Cheers.
--
Felipe Contreras