Re: [PATCH 1/2] log: grep author/committer using mailmap

From: Antoine Pelisse <apelisse@gmail.com>
Date: Dec 26, 2012, 21:12 UTC
Message-ID: <CALWbr2xW6r5ysJ8KQZa1eGYehG8ZbEp6K+s5JkG2goK9ef7rcA@mail.gmail.com>
In-Reply-To: <7vr4mcobpu.fsf@alter.siamese.dyndns.org>

Show 16 quoted lines

>>
>> +static int commit_rewrite_authors(struct strbuf *buf, const char *what, struct string_list *mailmap)
>> +{
>> +     char *author, *endp;
>> +     size_t len;
>> +     struct strbuf name = STRBUF_INIT;
>> +     struct strbuf mail = STRBUF_INIT;
>> +     struct ident_split ident;
>> +
>> +     author = strstr(buf->buf, what);
>> +     if (!author)
>> +             goto error;
>
> This does not stop at the end of the header part and would match a
> random line in the log message that happens to begin with "author ";
> is this something we would worry about, or would we leave it to "fsck"?

The only worrying case would be:
 - commit doesn't have "\nauthor" in the header (can that happen
without corruption?)
 - commit has "\nauthor" in the commit log
 - This line from commit log contains an <email> (split_ident_line works)
Then, I guess it's going to replace the name in the commit log.

Otherwise, it would not replace anything, as there is no author to replace anyway.

It looks like most mechanisms using mailmap would have the same issue.

Show 6 quoted lines

>> +     author += strlen(what);
>> +     endp = strstr(author, "\n");
>
> Using strchr(author, '\n') would feel more natural.  Also rename
> "author" to "person" or something, as you would be using this
> function for the committer information as well?

Both fixed

Show 15 quoted lines

>> +     if (!endp)
>> +             goto error;
>> +
>> +     len = endp - author;
>> +
>> +     if (split_ident_line(&ident, author, len)) {
>> +     error:
>> +             strbuf_release(&name);
>> +             strbuf_release(&mail);
>> +
>> +             return 1;
>
> We usually signal error by returning a negative integer.  It does
> not matter too much in this case as no callers seem to check the
> return value from this function, though.

Fixed, or would you rather see it `void` ?

Show 15 quoted lines

>> +     }
>> +
>> +     strbuf_add(&name, ident.name_begin, ident.name_end - ident.name_begin);
>> +     strbuf_add(&mail, ident.mail_begin, ident.mail_end - ident.mail_begin);
>> +
>> +     map_user(mailmap, &mail, &name);
>> +
>> +     strbuf_addf(&name, " <%s>", mail.buf);
>> +
>> +     strbuf_splice(buf, ident.name_begin - buf->buf,
>> +                   ident.mail_end - ident.name_begin + 1,
>> +                   name.buf, name.len);
>
> Would it give us better performance if we splice only when
> map_user() tells us that we actually rewrote the ident?

My intuition was that the cost of splice belongs to "memoving", when the size is different. Yet, Fixed, as it removes two copies.