Re: [RFC/PATCH 1/4] builtin: add git-check-mailmap command

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Jul 11, 2013 at 2:45 AM, Antoine Pelisse <apelisse@gmail.com> wrote:
> On Wed, Jul 10, 2013 at 9:03 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> +static void check_mailmap(struct string_list *mailmap, const char *contact)
>> +{
>> +       const char *name, *mail;
>> +       size_t namelen, maillen;
>> +       struct ident_split ident;
>> +       char term = null_out ? '\0' : '\n';
>> +
>> +       if (split_ident_line(&ident, contact, strlen(contact)))
>> +               die(_("unable to parse contact: %s"), contact);
>> +
>> +       name = ident.name_begin;
>> +       namelen = ident.name_end - ident.name_begin;
>> +       mail = ident.mail_begin;
>> +       maillen = ident.mail_end - ident.mail_begin;
>> +
>> +       map_user(mailmap, &mail, &maillen, &name, &namelen);
>
> Would it be useful to check the return value of this function, to
> display a message when the name can't mapped ?

I thought about it but decided against the added complexity (at least
initially) for a few reasons.

There are only two callers in the existing code which even look at the
return value:

  % git grep 'map_user('
  builtin/blame.c: map_user(&mailmap, &mailbuf, &maillen,
  builtin/shortlog.c: map_user(&log->mailmap, &mailbuf, &maillen,
&namebuf, &namelen);
  pretty.c: map_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
  pretty.c: return mail_map->nr && map_user(mail_map, email,
email_len, name, name_len);
  revision.c: if (map_user(mailmap, &mail, &maillen, &name, &namelen)) {

Of those, mailmap_name() in pretty.c merely passes the return value
along to its callers, but its callers don't care about it:

  % git grep 'mailmap_name('
  pretty.c: mailmap_name(&mail, &maillen, &name, &namelen);

commit_rewrite_person() in revision.c uses the return value apparently
only as an optimization to decide if it should go through the effort
of replacing the "old" person with the re-mapped person, and then
passes the return value along to its callers, none of which care:

  % git grep 'commit_rewrite_person('
  revision.c: commit_rewrite_person(&buf, "\nauthor ", opt->mailmap);
  revision.c: commit_rewrite_person(&buf, "\ncommitter ", opt->mailmap);

The sort of optimization taken by commit_rewrite_person() is
effectively lost to script and porcelain clients of check-mailmap
since the cost of executing the command probably swamps any savings
the client might otherwise achieve by knowing whether the person was
remapped. Also, modulo possible whitespace changes, a client can
compare what it passed in to what is received back to determine if
remapping occurred.

As plumbing, the expectation is that most clients will pass a value in
and use the output without further interpretation. If check-mailmap
unconditionally adds some "mapped" or "not mapped" indicator to each
returned value, then that places extra burden on all clients by
forcing them to parse the result. Alternately, if the indicator is
controlled by a command-line option, then (at the least) that
increases documentation costs.

For people using check-mailmap as a .mailmap debugging aid, such an
indicator might indeed be useful, however, it seems prudent to keep
things simple initially by omitting the bells and whistles until
practical experience shows that more features (and complexity) are
warranted.

>> +       if (namelen)
>> +               printf("%.*s <%.*s>%c",
>> +                       (int)namelen, name, (int)maillen, mail, term);
>> +       else
>> +               printf("<%.*s>%c", (int)maillen, mail, term);
>> +}