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

Eric Sunshine [off-list ref] 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 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.

+static const char * const check_mailmap_usage[] = {
+N_("git check-mailmap [options] <contact>..."),
+NULL
+};

This mis-indentation looks somewhat unfortunate, but they are shared
with other check-blah, and they are meant to contain a rather long
string, so let's keep it like so.

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

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

As we can read from HEAD:.mailmap these days, the patch is correct
that it does not require NEED_WORK_TREE here.

Thanks.

 		{ "check-ref-format", cmd_check_ref_format },
 		{ "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
 		{ "checkout-index", cmd_checkout_index,

On Thu, Jul 11, 2013 at 3:04 PM, Junio C Hamano [off-list ref] wrote:

Eric Sunshine [off-list ref] writes:

quoted

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

quoted

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.

quoted

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

quoted

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