Re: [PATCH v2 1/4] builtin: add git-check-mailmap command
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jul 12, 2013, 03:28 UTC
- Message-ID
- <CAPig+cQvnKSxZ5M6zW_r6Y9xwZ0Ch99JDq4SccYXCRof9svsOg@mail.gmail.com>
- In-Reply-To
- <7vli5crk8l.fsf@alter.siamese.dyndns.org>
On Thu, Jul 11, 2013 at 3:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 23 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes: > >> +DESCRIPTION >> +----------- >> + >> +For each ``Name $$<email@address>$$'' or ``$$<email@address>$$'' provided on >> +the command-line or standard input (when using `--stdin`), prints a line with >> +the canonical contact information for that person according to the 'mailmap' >> +(see "Mapping Authors" below). If no mapping exists for the person, echoes the >> +provided contact information. > > OK. The last part needed a reading and a half for me to realize the > "echoes the provided contact information" is the same thing as "the > "input string is printed as-is", especially because "contact > information" is not defined anywhere in the previous part of the > DESCRIPTION section, though. I might be worth starting the > paragraph with: > > For each contact information (either in the form of ``Name > <user@host>'' or ...) > > in order to clarify that the two forms of input is what you call > "contact information".
Is this easier to read?
For each ``Name $$<user@host>$$'' or ``$$<user@host>$$'' from the
command-line or standard input (when using `--stdin`), print a line
showing either the canonical name and email address (see "Mapping
Authors" below), or the input ``Name $$<user@host>$$'' or
``$$<user@host>$$'' if there is no mapping for that person.Show 19 quoted lines
>> diff --git a/builtin/check-mailmap.c b/builtin/check-mailmap.c >> new file mode 100644 >> index 0000000..c36a61c >> --- /dev/null >> +++ b/builtin/check-mailmap.c >> @@ -0,0 +1,69 @@ >> +#include "builtin.h" >> +#include "mailmap.h" >> +#include "parse-options.h" >> +#include "string-list.h" >> + >> +static int use_stdin; >> +static int null_out; > > Is there a reason why the variable name should not match that of the > corresponding variables in check-ignore and check-attr, which you > copied the bulk of the code from? > > If there isn't, use "null_term_line" like they do.
In check-attr, null_term_line indicates that _input_ lines are null-terminated. In check-ignore, null_term_lines is overloaded (and perhaps abused) to mean that both _input_ and _output_ lines are null-terminated. In check-mailmap, -z affects only _output_ lines. A reader of the code can easily be misled into thinking that the variable controls the same function in all three programs, hence null_out was chosen to make it clear that it applies only to output. A lesser reason is that, in the future, someone might add an option to null terminate input lines (distinct from output), and null_in would be a reasonable name for that variable.
Other than the above reasoning, I don't feel strongly about it and can revert the name if you prefer.
Show 12 quoted lines
>> +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';
>
> Is there a reason why the variable name "term" should not match that
> of the corresponding variables in check-ignore and check-attr, which
> you copied the bulk of the code from?
>
> If there isn't, use "line_termination" like they do.No strong justification. As with 'buf' vs. 'buffer', 'line_termination' required more reading effort without conveying any more information than 'term'.
Show 29 quoted lines
>> + 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);
>> +
>> + if (namelen)
>> + printf("%.*s ", (int)namelen, name);
>> + printf("<%.*s>%c", (int)maillen, mail, term);
>> +}
>> +
>> +int cmd_check_mailmap(int argc, const char **argv, const char *prefix)
>> +{
>> + int i;
>> + struct string_list mailmap = STRING_LIST_INIT_NODUP;
>> +
>> + git_config(git_default_config, NULL);
>> + argc = parse_options(argc, argv, prefix, check_mailmap_options,
>> + check_mailmap_usage, 0);
>
> It is a bit distracting that the second line that is not indented
> enough. Either (1) with enough HT and with minimum number of SP,
> align "argc" and "check_mailmap_usage", or (2) with minimum number
> of HT and no SP, push "check_mailmap_usage" to the right of opening
> parenthesis of "(argc". Our code tend to prefer (1).Okay.