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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 11, 2013, 10:28 UTC
Message-ID: <CAPig+cSNvXQTvhdU-9YTRa3M_BDPBB0jH87y-C16E+_Qqo_Lxg@mail.gmail.com>
In-Reply-To: <CALWbr2zJTBSptGsOr6tqrr4KcVd2GOWCkgy4GgdZ2+0Vz7DU4w@mail.gmail.com>

On Thu, Jul 11, 2013 at 2:45 AM, Antoine Pelisse <apelisse@gmail.com> wrote:

Show 20 quoted lines

> 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.

Show 6 quoted lines

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