On Thu, Jul 11, 2013 at 3:04 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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.
>> 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.
>> +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'.
>> + 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html