[PATCH v2 0/4] add git-check-mailmap command

[PATCH v2 0/4] add git-check-mailmap command

[Eric Sunshine]

This is a re-roll of [1] which introduces git-check-mailmap. The
primary motivation for this command is to expose git's stable,
well-tested C-implementation of .mailmap functionality to scripts and
porcelains, thus relieving them of the need to reimplement support
themselves. The git-contacts [2] script (currently at es/contacts in
'pu') would be one such client. v2 removes the RFC status and addresses
comments from reviewers [1].

Changes since v1:

* Add Documentation/git-check-mailmap.txt.

* Add --null alias for -z.

* Use OPT_BOOL rather than deprecated OPT_BOOLEAN.

* Simplify code which outputs normalized contact.

* Settle on "stdout" as argument to maybe_flush_or_die().

* Eliminate diff noise from patch 4/4.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/230068/
[2]: http://thread.gmane.org/gmane.comp.version-control.git/229533/

Eric Sunshine (4):
  builtin: add git-check-mailmap command
  t4203: test check-mailmap command invocation
  t4203: test mailmap functionality directly rather than indirectly
  t4203: consolidate test-repository setup

 .gitignore                             |   1 +
 Documentation/git-check-mailmap.txt    |  55 ++++++++
 Makefile                               |   1 +
 builtin.h                              |   1 +
 builtin/check-mailmap.c                |  69 ++++++++++
 command-list.txt                       |   1 +
 contrib/completion/git-completion.bash |   1 +
 git.c                                  |   1 +
 t/t4203-mailmap.sh                     | 234 +++++++++++++++++----------------
 9 files changed, 251 insertions(+), 113 deletions(-)
 create mode 100644 Documentation/git-check-mailmap.txt
 create mode 100644 builtin/check-mailmap.c

-- 
1.8.3.2

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

[Eric Sunshine]

Introduce command check-mailmap, similar to check-attr and check-ignore,
which allows direct testing of .mailmap configuration.

As plumbing accessible to scripts and other porcelain, check-mailmap
publishes the stable, well-tested .mailmap functionality employed by
built-in Git commands.  Consequently, script authors need not
re-implement .mailmap functionality manually, thus avoiding potential
quirks and behavioral differences.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 .gitignore                             |  1 +
 Documentation/git-check-mailmap.txt    | 55 +++++++++++++++++++++++++++
 Makefile                               |  1 +
 builtin.h                              |  1 +
 builtin/check-mailmap.c                | 69 ++++++++++++++++++++++++++++++++++
 command-list.txt                       |  1 +
 contrib/completion/git-completion.bash |  1 +
 git.c                                  |  1 +
 8 files changed, 130 insertions(+)
 create mode 100644 Documentation/git-check-mailmap.txt
 create mode 100644 builtin/check-mailmap.c

diff --git a/.gitignore b/.gitignore
index efa8db0..6b1fd1b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -23,6 +23,7 @@
 /git-cat-file
 /git-check-attr
 /git-check-ignore
+/git-check-mailmap
 /git-check-ref-format
 /git-checkout
 /git-checkout-index
diff --git a/Documentation/git-check-mailmap.txt b/Documentation/git-check-mailmap.txt
new file mode 100644
index 0000000..3040bbc
--- /dev/null
+++ b/Documentation/git-check-mailmap.txt
@@ -0,0 +1,55 @@
+git-check-mailmap(1)
+====================
+
+NAME
+----
+git-check-mailmap - Show canonical mappings of names and email addresses
+
+
+SYNOPSIS
+--------
+[verse]
+'git check-mailmap' [options] <contact>...
+
+
+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.
+
+
+OPTIONS
+-------
+--stdin::
+	Read contacts, one per line, from the standard input after exhausting
+	contacts provided on the command-line.
+
+-z::
+--null::
+	Terminate each printed contact line with a null character `\0` rather
+	than a newline.
+
+
+OUTPUT
+------
+
+For each contact, a single line is printed of the form ``Name
+$$<email@address>$$'' if the name is provided or known to the 'mailmap'; or of
+the form ``$$<email@address>$$'' if no name is provided or known. Each line is
+terminated by a newline, or by a null character `\0` when `-z` or `--null` is
+specified.
+
+
+MAPPING AUTHORS
+---------------
+
+include::mailmap.txt[]
+
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 0600eb4..ef442eb 100644
--- a/Makefile
+++ b/Makefile
@@ -912,6 +912,7 @@ BUILTIN_OBJS += builtin/bundle.o
 BUILTIN_OBJS += builtin/cat-file.o
 BUILTIN_OBJS += builtin/check-attr.o
 BUILTIN_OBJS += builtin/check-ignore.o
+BUILTIN_OBJS += builtin/check-mailmap.o
 BUILTIN_OBJS += builtin/check-ref-format.o
 BUILTIN_OBJS += builtin/checkout-index.o
 BUILTIN_OBJS += builtin/checkout.o
diff --git a/builtin.h b/builtin.h
index 1ed8edb..8afa2de 100644
--- a/builtin.h
+++ b/builtin.h
@@ -40,6 +40,7 @@ extern int cmd_checkout(int argc, const char **argv, const char *prefix);
 extern int cmd_checkout_index(int argc, const char **argv, const char *prefix);
 extern int cmd_check_attr(int argc, const char **argv, const char *prefix);
 extern int cmd_check_ignore(int argc, const char **argv, const char *prefix);
+extern int cmd_check_mailmap(int argc, const char **argv, const char *prefix);
 extern int cmd_check_ref_format(int argc, const char **argv, const char *prefix);
 extern int cmd_cherry(int argc, const char **argv, const char *prefix);
 extern int cmd_cherry_pick(int argc, const char **argv, const char *prefix);
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;
+static const char * const check_mailmap_usage[] = {
+N_("git check-mailmap [options] <contact>..."),
+NULL
+};
+
+static const struct option check_mailmap_options[] = {
+	OPT_BOOL(0, "stdin", &use_stdin, N_("also read contacts from stdin")),
+	OPT_BOOL('z', "null", &null_out, N_("null-terminate output lines")),
+	OPT_END()
+};
+
+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);
+
+	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);
+	if (argc == 0 && !use_stdin)
+		die(_("no contacts specified"));
+
+	read_mailmap(&mailmap, NULL);
+
+	for (i = 0; i < argc; ++i)
+		check_mailmap(&mailmap, argv[i]);
+	maybe_flush_or_die(stdout, "stdout");
+
+	if (use_stdin) {
+		struct strbuf buf = STRBUF_INIT;
+		while (strbuf_getline(&buf, stdin, '\n') != EOF) {
+			check_mailmap(&mailmap, buf.buf);
+			maybe_flush_or_die(stdout, "stdout");
+		}
+		strbuf_release(&buf);
+	}
+
+	clear_mailmap(&mailmap);
+	return 0;
+}
diff --git a/command-list.txt b/command-list.txt
index bf83303..08b04e2 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -13,6 +13,7 @@ git-bundle                              mainporcelain
 git-cat-file                            plumbinginterrogators
 git-check-attr                          purehelpers
 git-check-ignore                        purehelpers
+git-check-mailmap                       purehelpers
 git-checkout                            mainporcelain common
 git-checkout-index                      plumbingmanipulators
 git-check-ref-format                    purehelpers
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ebc40d4..c118550 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -648,6 +648,7 @@ __git_list_porcelain_commands ()
 		cat-file)         : plumbing;;
 		check-attr)       : plumbing;;
 		check-ignore)     : plumbing;;
+		check-mailmap)    : plumbing;;
 		check-ref-format) : plumbing;;
 		checkout-index)   : plumbing;;
 		commit-tree)      : plumbing;;
diff --git a/git.c b/git.c
index 4359086..02c3035 100644
--- a/git.c
+++ b/git.c
@@ -324,6 +324,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "cat-file", cmd_cat_file, RUN_SETUP },
 		{ "check-attr", cmd_check_attr, RUN_SETUP },
 		{ "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
+		{ "check-mailmap", cmd_check_mailmap, RUN_SETUP },
 		{ "check-ref-format", cmd_check_ref_format },
 		{ "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
 		{ "checkout-index", cmd_checkout_index,
-- 
1.8.3.2

[PATCH v2 2/4] t4203: test check-mailmap command invocation

[Eric Sunshine]

Test the command-line interface of check-mailmap. (Actual .mailmap
functionality is already covered by existing tests.)

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t4203-mailmap.sh | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 842b754..8645492 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -13,6 +13,11 @@ fuzz_blame () {
 }
 
 test_expect_success setup '
+	cat >contacts <<-\EOF &&
+	A U Thor <author@example.com>
+	nick1 <bugs@company.xx>
+	EOF
+
 	echo one >one &&
 	git add one &&
 	test_tick &&
@@ -23,6 +28,51 @@ test_expect_success setup '
 	git commit --author "nick1 <bugs@company.xx>" -m second
 '
 
+test_expect_success 'check-mailmap no arguments' '
+	test_must_fail git check-mailmap
+'
+
+test_expect_success 'check-mailmap arguments' '
+	cat >expect <<-\EOF &&
+	A U Thor <author@example.com>
+	nick1 <bugs@company.xx>
+	EOF
+	git check-mailmap \
+		"A U Thor <author@example.com>" \
+		"nick1 <bugs@company.xx>" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check-mailmap --stdin' '
+	cat >expect <<-\EOF &&
+	A U Thor <author@example.com>
+	nick1 <bugs@company.xx>
+	EOF
+	git check-mailmap --stdin <contacts >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check-mailmap --stdin arguments' '
+	cat >expect <<-\EOF &&
+	Internal Guy <bugs@company.xy>
+	EOF
+	cat <contacts >>expect &&
+	git check-mailmap --stdin "Internal Guy <bugs@company.xy>" \
+		<contacts >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check-mailmap -z' '
+	printf "A U Thor <author@example.com>\0" >expect &&
+	printf "nick1 <bugs@company.xx>\0" >>expect &&
+	git check-mailmap -z --stdin <contacts >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check-mailmap bogus contact' '
+	test_must_fail git check-mailmap bogus
+'
+
 cat >expect <<\EOF
 A U Thor (1):
       initial
-- 
1.8.3.2

[PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly

[Eric Sunshine]

With the introduction of check-mailmap, it is now possible to check
.mailmap functionality directly rather than indirectly as a side-effect
of other commands (such as git-shortlog), therefore, do so.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t4203-mailmap.sh | 133 ++++++++++++++++++-----------------------------------
 1 file changed, 45 insertions(+), 88 deletions(-)

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 8645492..48a000b 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -74,128 +74,96 @@ test_expect_success 'check-mailmap bogus contact' '
 '
 
 cat >expect <<\EOF
-A U Thor (1):
-      initial
-
-nick1 (1):
-      second
-
+A U Thor <author@example.com>
+nick1 <bugs@company.xx>
 EOF
 
 test_expect_success 'No mailmap' '
-	git shortlog HEAD >actual &&
+	git check-mailmap --stdin <contacts >actual &&
 	test_cmp expect actual
 '
 
 cat >expect <<\EOF
-Repo Guy (1):
-      initial
-
-nick1 (1):
-      second
-
+Repo Guy <author@example.com>
+nick1 <bugs@company.xx>
 EOF
 
 test_expect_success 'default .mailmap' '
 	echo "Repo Guy <author@example.com>" > .mailmap &&
-	git shortlog HEAD >actual &&
+	git check-mailmap --stdin <contacts >actual &&
 	test_cmp expect actual
 '
 
 # Using a mailmap file in a subdirectory of the repo here, but
 # could just as well have been a file outside of the repository
 cat >expect <<\EOF
-Internal Guy (1):
-      second
-
-Repo Guy (1):
-      initial
-
+Repo Guy <author@example.com>
+Internal Guy <bugs@company.xx>
 EOF
 test_expect_success 'mailmap.file set' '
 	mkdir -p internal_mailmap &&
 	echo "Internal Guy <bugs@company.xx>" > internal_mailmap/.mailmap &&
 	git config mailmap.file internal_mailmap/.mailmap &&
-	git shortlog HEAD >actual &&
+	git check-mailmap --stdin <contacts >actual &&
 	test_cmp expect actual
 '
 
 cat >expect <<\EOF
-External Guy (1):
-      initial
-
-Internal Guy (1):
-      second
-
+External Guy <author@example.com>
+Internal Guy <bugs@company.xx>
 EOF
 test_expect_success 'mailmap.file override' '
 	echo "External Guy <author@example.com>" >> internal_mailmap/.mailmap &&
 	git config mailmap.file internal_mailmap/.mailmap &&
-	git shortlog HEAD >actual &&
+	git check-mailmap --stdin <contacts >actual &&
 	test_cmp expect actual
 '
 
 cat >expect <<\EOF
-Repo Guy (1):
-      initial
-
-nick1 (1):
-      second
-
+Repo Guy <author@example.com>
+nick1 <bugs@company.xx>
 EOF
 
 test_expect_success 'mailmap.file non-existent' '
 	rm internal_mailmap/.mailmap &&
 	rmdir internal_mailmap &&
-	git shortlog HEAD >actual &&
+	git check-mailmap --stdin <contacts >actual &&
 	test_cmp expect actual
 '
 
 cat >expect <<\EOF
-Internal Guy (1):
-      second
-
-Repo Guy (1):
-      initial
-
+Repo Guy <author@example.com>
+Internal Guy <bugs@company.xy>
 EOF
 
 test_expect_success 'name entry after email entry' '
 	mkdir -p internal_mailmap &&
 	echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap &&
 	echo "Internal Guy <bugs@company.xx>" >>internal_mailmap/.mailmap &&
-	git shortlog HEAD >actual &&
+	git check-mailmap --stdin <contacts >actual &&
 	test_cmp expect actual
 '
 
 cat >expect <<\EOF
-Internal Guy (1):
-      second
-
-Repo Guy (1):
-      initial
-
+Repo Guy <author@example.com>
+Internal Guy <bugs@company.xy>
 EOF
 
 test_expect_success 'name entry after email entry, case-insensitive' '
 	mkdir -p internal_mailmap &&
 	echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap &&
 	echo "Internal Guy <BUGS@Company.xx>" >>internal_mailmap/.mailmap &&
-	git shortlog HEAD >actual &&
+	git check-mailmap --stdin <contacts >actual &&
 	test_cmp expect actual
 '
 
 cat >expect <<\EOF
-A U Thor (1):
-      initial
-
-nick1 (1):
-      second
-
+A U Thor <author@example.com>
+nick1 <bugs@company.xx>
 EOF
 test_expect_success 'No mailmap files, but configured' '
 	rm -f .mailmap internal_mailmap/.mailmap &&
-	git shortlog HEAD >actual &&
+	git check-mailmap --stdin <contacts >actual &&
 	test_cmp expect actual
 '
 
@@ -217,54 +185,43 @@ test_expect_success 'setup mailmap blob tests' '
 
 test_expect_success 'mailmap.blob set' '
 	cat >expect <<-\EOF &&
-	Blob Guy (1):
-	      second
-
-	Repo Guy (1):
-	      initial
-
+	Repo Guy <author@example.com>
+	Blob Guy <bugs@company.xx>
 	EOF
-	git -c mailmap.blob=map:just-bugs shortlog HEAD >actual &&
+	git -c mailmap.blob=map:just-bugs check-mailmap --stdin \
+		<contacts >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'mailmap.blob overrides .mailmap' '
 	cat >expect <<-\EOF &&
-	Blob Guy (2):
-	      initial
-	      second
-
+	Blob Guy <author@example.com>
+	Blob Guy <bugs@company.xx>
 	EOF
-	git -c mailmap.blob=map:both shortlog HEAD >actual &&
+	git -c mailmap.blob=map:both check-mailmap --stdin \
+		<contacts  >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'mailmap.file overrides mailmap.blob' '
 	cat >expect <<-\EOF &&
-	Blob Guy (1):
-	      second
-
-	Internal Guy (1):
-	      initial
-
+	Internal Guy <author@example.com>
+	Blob Guy <bugs@company.xx>
 	EOF
 	git \
 	  -c mailmap.blob=map:both \
 	  -c mailmap.file=internal.map \
-	  shortlog HEAD >actual &&
+	  check-mailmap --stdin <contacts >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'mailmap.blob can be missing' '
 	cat >expect <<-\EOF &&
-	Repo Guy (1):
-	      initial
-
-	nick1 (1):
-	      second
-
+	Repo Guy <author@example.com>
+	nick1 <bugs@company.xx>
 	EOF
-	git -c mailmap.blob=map:nonexistent shortlog HEAD >actual &&
+	git -c mailmap.blob=map:nonexistent check-mailmap --stdin \
+		<contacts >actual &&
 	test_cmp expect actual
 '
 
@@ -273,12 +230,12 @@ test_expect_success 'mailmap.blob defaults to off in non-bare repo' '
 	(
 		cd non-bare &&
 		test_commit one .mailmap "Fake Name <author@example.com>" &&
-		echo "     1	Fake Name" >expect &&
-		git shortlog -ns HEAD >actual &&
+		echo "Fake Name <author@example.com>" >expect &&
+		git check-mailmap "A U Thor <author@example.com>" >actual &&
 		test_cmp expect actual &&
 		rm .mailmap &&
-		echo "     1	A U Thor" >expect &&
-		git shortlog -ns HEAD >actual &&
+		echo "A U Thor <author@example.com>" >expect &&
+		git check-mailmap "A U Thor <author@example.com>" >actual &&
 		test_cmp expect actual
 	)
 '
@@ -287,8 +244,8 @@ test_expect_success 'mailmap.blob defaults to HEAD:.mailmap in bare repo' '
 	git clone --bare non-bare bare &&
 	(
 		cd bare &&
-		echo "     1	Fake Name" >expect &&
-		git shortlog -ns HEAD >actual &&
+		echo "Fake Name <author@example.com>" >expect &&
+		git check-mailmap "A U Thor <author@example.com>" >actual &&
 		test_cmp expect actual
 	)
 '
-- 
1.8.3.2

[PATCH v2 4/4] t4203: consolidate test-repository setup

[Eric Sunshine]

The simple two-commit test-repository created by 'setup' is no longer
needed by any of the tests retrofitted to use check-mailmap. Subsequent,
more complex tests of git-shortlog, git-log, and git-blame functionality
expand the repository by adding five commits. Consolidate the creation
of this seven-commit repository into a single setup function.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t4203-mailmap.sh | 53 +++++++++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 26 deletions(-)

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 48a000b..9c3c83a 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -22,10 +22,36 @@ test_expect_success setup '
 	git add one &&
 	test_tick &&
 	git commit -m initial &&
+
 	echo two >>one &&
 	git add one &&
 	test_tick &&
-	git commit --author "nick1 <bugs@company.xx>" -m second
+	git commit --author "nick1 <bugs@company.xx>" -m second &&
+
+	echo three >>one &&
+	git add one &&
+	test_tick &&
+	git commit --author "nick2 <bugs@company.xx>" -m third &&
+
+	echo four >>one &&
+	git add one &&
+	test_tick &&
+	git commit --author "nick2 <nick2@company.xx>" -m fourth &&
+
+	echo five >>one &&
+	git add one &&
+	test_tick &&
+	git commit --author "santa <me@company.xx>" -m fifth &&
+
+	echo six >>one &&
+	git add one &&
+	test_tick &&
+	git commit --author "claus <me@company.xx>" -m sixth &&
+
+	echo seven >>one &&
+	git add one &&
+	test_tick &&
+	git commit --author "CTO <cto@coompany.xx>" -m seventh
 '
 
 test_expect_success 'check-mailmap no arguments' '
@@ -276,31 +302,6 @@ Some Dude <some@dude.xx> (1):
 EOF
 
 test_expect_success 'Shortlog output (complex mapping)' '
-	echo three >>one &&
-	git add one &&
-	test_tick &&
-	git commit --author "nick2 <bugs@company.xx>" -m third &&
-
-	echo four >>one &&
-	git add one &&
-	test_tick &&
-	git commit --author "nick2 <nick2@company.xx>" -m fourth &&
-
-	echo five >>one &&
-	git add one &&
-	test_tick &&
-	git commit --author "santa <me@company.xx>" -m fifth &&
-
-	echo six >>one &&
-	git add one &&
-	test_tick &&
-	git commit --author "claus <me@company.xx>" -m sixth &&
-
-	echo seven >>one &&
-	git add one &&
-	test_tick &&
-	git commit --author "CTO <cto@coompany.xx>" -m seventh &&
-
 	mkdir -p internal_mailmap &&
 	echo "Committed <committer@example.com>" > internal_mailmap/.mailmap &&
 	echo "<cto@company.xx>                       <cto@coompany.xx>" >> internal_mailmap/.mailmap &&
-- 
1.8.3.2

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

[Junio C Hamano]

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

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

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

> +	if (argc == 0 && !use_stdin)
> +		die(_("no contacts specified"));
> +
> +	read_mailmap(&mailmap, NULL);
> +
> +	for (i = 0; i < argc; ++i)
> +		check_mailmap(&mailmap, argv[i]);
> +	maybe_flush_or_die(stdout, "stdout");
> +
> +	if (use_stdin) {
> +		struct strbuf buf = STRBUF_INIT;
> +		while (strbuf_getline(&buf, stdin, '\n') != EOF) {
> +			check_mailmap(&mailmap, buf.buf);
> +			maybe_flush_or_die(stdout, "stdout");
> +		}
> +		strbuf_release(&buf);
> +	}
> +
> +	clear_mailmap(&mailmap);
> +	return 0;
> +}
> diff --git a/command-list.txt b/command-list.txt
> index bf83303..08b04e2 100644
> --- a/command-list.txt
> +++ b/command-list.txt
> @@ -13,6 +13,7 @@ git-bundle                              mainporcelain
>  git-cat-file                            plumbinginterrogators
>  git-check-attr                          purehelpers
>  git-check-ignore                        purehelpers
> +git-check-mailmap                       purehelpers
>  git-checkout                            mainporcelain common
>  git-checkout-index                      plumbingmanipulators
>  git-check-ref-format                    purehelpers
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index ebc40d4..c118550 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -648,6 +648,7 @@ __git_list_porcelain_commands ()
>  		cat-file)         : plumbing;;
>  		check-attr)       : plumbing;;
>  		check-ignore)     : plumbing;;
> +		check-mailmap)    : plumbing;;
>  		check-ref-format) : plumbing;;
>  		checkout-index)   : plumbing;;
>  		commit-tree)      : plumbing;;
> diff --git a/git.c b/git.c
> index 4359086..02c3035 100644
> --- a/git.c
> +++ b/git.c
> @@ -324,6 +324,7 @@ static void handle_internal_command(int argc, const char **argv)
>  		{ "cat-file", cmd_cat_file, RUN_SETUP },
>  		{ "check-attr", cmd_check_attr, RUN_SETUP },
>  		{ "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
> +		{ "check-mailmap", cmd_check_mailmap, RUN_SETUP },

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,

Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> With the introduction of check-mailmap, it is now possible to check
> .mailmap functionality directly rather than indirectly as a side-effect
> of other commands (such as git-shortlog), therefore, do so.

Does this patch mean that we will now ignore future breakages in
shortlog and blame if their mailmap integration becomes buggy?

I am not convinced it is a good idea if that is what is going on.

>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  t/t4203-mailmap.sh | 133 ++++++++++++++++++-----------------------------------
>  1 file changed, 45 insertions(+), 88 deletions(-)
>
> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
> index 8645492..48a000b 100755
> --- a/t/t4203-mailmap.sh
> +++ b/t/t4203-mailmap.sh
> @@ -74,128 +74,96 @@ test_expect_success 'check-mailmap bogus contact' '
>  '
>  
>  cat >expect <<\EOF
> -A U Thor (1):
> -      initial
> -
> -nick1 (1):
> -      second
> -
> +A U Thor <author@example.com>
> +nick1 <bugs@company.xx>
>  EOF
>  
>  test_expect_success 'No mailmap' '
> -	git shortlog HEAD >actual &&
> +	git check-mailmap --stdin <contacts >actual &&
>  	test_cmp expect actual
>  '
>  
>  cat >expect <<\EOF
> -Repo Guy (1):
> -      initial
> -
> -nick1 (1):
> -      second
> -
> +Repo Guy <author@example.com>
> +nick1 <bugs@company.xx>
>  EOF
>  
>  test_expect_success 'default .mailmap' '
>  	echo "Repo Guy <author@example.com>" > .mailmap &&
> -	git shortlog HEAD >actual &&
> +	git check-mailmap --stdin <contacts >actual &&
>  	test_cmp expect actual
>  '
>  
>  # Using a mailmap file in a subdirectory of the repo here, but
>  # could just as well have been a file outside of the repository
>  cat >expect <<\EOF
> -Internal Guy (1):
> -      second
> -
> -Repo Guy (1):
> -      initial
> -
> +Repo Guy <author@example.com>
> +Internal Guy <bugs@company.xx>
>  EOF
>  test_expect_success 'mailmap.file set' '
>  	mkdir -p internal_mailmap &&
>  	echo "Internal Guy <bugs@company.xx>" > internal_mailmap/.mailmap &&
>  	git config mailmap.file internal_mailmap/.mailmap &&
> -	git shortlog HEAD >actual &&
> +	git check-mailmap --stdin <contacts >actual &&
>  	test_cmp expect actual
>  '
>  
>  cat >expect <<\EOF
> -External Guy (1):
> -      initial
> -
> -Internal Guy (1):
> -      second
> -
> +External Guy <author@example.com>
> +Internal Guy <bugs@company.xx>
>  EOF
>  test_expect_success 'mailmap.file override' '
>  	echo "External Guy <author@example.com>" >> internal_mailmap/.mailmap &&
>  	git config mailmap.file internal_mailmap/.mailmap &&
> -	git shortlog HEAD >actual &&
> +	git check-mailmap --stdin <contacts >actual &&
>  	test_cmp expect actual
>  '
>  
>  cat >expect <<\EOF
> -Repo Guy (1):
> -      initial
> -
> -nick1 (1):
> -      second
> -
> +Repo Guy <author@example.com>
> +nick1 <bugs@company.xx>
>  EOF
>  
>  test_expect_success 'mailmap.file non-existent' '
>  	rm internal_mailmap/.mailmap &&
>  	rmdir internal_mailmap &&
> -	git shortlog HEAD >actual &&
> +	git check-mailmap --stdin <contacts >actual &&
>  	test_cmp expect actual
>  '
>  
>  cat >expect <<\EOF
> -Internal Guy (1):
> -      second
> -
> -Repo Guy (1):
> -      initial
> -
> +Repo Guy <author@example.com>
> +Internal Guy <bugs@company.xy>
>  EOF
>  
>  test_expect_success 'name entry after email entry' '
>  	mkdir -p internal_mailmap &&
>  	echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap &&
>  	echo "Internal Guy <bugs@company.xx>" >>internal_mailmap/.mailmap &&
> -	git shortlog HEAD >actual &&
> +	git check-mailmap --stdin <contacts >actual &&
>  	test_cmp expect actual
>  '
>  
>  cat >expect <<\EOF
> -Internal Guy (1):
> -      second
> -
> -Repo Guy (1):
> -      initial
> -
> +Repo Guy <author@example.com>
> +Internal Guy <bugs@company.xy>
>  EOF
>  
>  test_expect_success 'name entry after email entry, case-insensitive' '
>  	mkdir -p internal_mailmap &&
>  	echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap &&
>  	echo "Internal Guy <BUGS@Company.xx>" >>internal_mailmap/.mailmap &&
> -	git shortlog HEAD >actual &&
> +	git check-mailmap --stdin <contacts >actual &&
>  	test_cmp expect actual
>  '
>  
>  cat >expect <<\EOF
> -A U Thor (1):
> -      initial
> -
> -nick1 (1):
> -      second
> -
> +A U Thor <author@example.com>
> +nick1 <bugs@company.xx>
>  EOF
>  test_expect_success 'No mailmap files, but configured' '
>  	rm -f .mailmap internal_mailmap/.mailmap &&
> -	git shortlog HEAD >actual &&
> +	git check-mailmap --stdin <contacts >actual &&
>  	test_cmp expect actual
>  '
>  
> @@ -217,54 +185,43 @@ test_expect_success 'setup mailmap blob tests' '
>  
>  test_expect_success 'mailmap.blob set' '
>  	cat >expect <<-\EOF &&
> -	Blob Guy (1):
> -	      second
> -
> -	Repo Guy (1):
> -	      initial
> -
> +	Repo Guy <author@example.com>
> +	Blob Guy <bugs@company.xx>
>  	EOF
> -	git -c mailmap.blob=map:just-bugs shortlog HEAD >actual &&
> +	git -c mailmap.blob=map:just-bugs check-mailmap --stdin \
> +		<contacts >actual &&
>  	test_cmp expect actual
>  '
>  
>  test_expect_success 'mailmap.blob overrides .mailmap' '
>  	cat >expect <<-\EOF &&
> -	Blob Guy (2):
> -	      initial
> -	      second
> -
> +	Blob Guy <author@example.com>
> +	Blob Guy <bugs@company.xx>
>  	EOF
> -	git -c mailmap.blob=map:both shortlog HEAD >actual &&
> +	git -c mailmap.blob=map:both check-mailmap --stdin \
> +		<contacts  >actual &&
>  	test_cmp expect actual
>  '
>  
>  test_expect_success 'mailmap.file overrides mailmap.blob' '
>  	cat >expect <<-\EOF &&
> -	Blob Guy (1):
> -	      second
> -
> -	Internal Guy (1):
> -	      initial
> -
> +	Internal Guy <author@example.com>
> +	Blob Guy <bugs@company.xx>
>  	EOF
>  	git \
>  	  -c mailmap.blob=map:both \
>  	  -c mailmap.file=internal.map \
> -	  shortlog HEAD >actual &&
> +	  check-mailmap --stdin <contacts >actual &&
>  	test_cmp expect actual
>  '
>  
>  test_expect_success 'mailmap.blob can be missing' '
>  	cat >expect <<-\EOF &&
> -	Repo Guy (1):
> -	      initial
> -
> -	nick1 (1):
> -	      second
> -
> +	Repo Guy <author@example.com>
> +	nick1 <bugs@company.xx>
>  	EOF
> -	git -c mailmap.blob=map:nonexistent shortlog HEAD >actual &&
> +	git -c mailmap.blob=map:nonexistent check-mailmap --stdin \
> +		<contacts >actual &&
>  	test_cmp expect actual
>  '
>  
> @@ -273,12 +230,12 @@ test_expect_success 'mailmap.blob defaults to off in non-bare repo' '
>  	(
>  		cd non-bare &&
>  		test_commit one .mailmap "Fake Name <author@example.com>" &&
> -		echo "     1	Fake Name" >expect &&
> -		git shortlog -ns HEAD >actual &&
> +		echo "Fake Name <author@example.com>" >expect &&
> +		git check-mailmap "A U Thor <author@example.com>" >actual &&
>  		test_cmp expect actual &&
>  		rm .mailmap &&
> -		echo "     1	A U Thor" >expect &&
> -		git shortlog -ns HEAD >actual &&
> +		echo "A U Thor <author@example.com>" >expect &&
> +		git check-mailmap "A U Thor <author@example.com>" >actual &&
>  		test_cmp expect actual
>  	)
>  '
> @@ -287,8 +244,8 @@ test_expect_success 'mailmap.blob defaults to HEAD:.mailmap in bare repo' '
>  	git clone --bare non-bare bare &&
>  	(
>  		cd bare &&
> -		echo "     1	Fake Name" >expect &&
> -		git shortlog -ns HEAD >actual &&
> +		echo "Fake Name <author@example.com>" >expect &&
> +		git check-mailmap "A U Thor <author@example.com>" >actual &&
>  		test_cmp expect actual
>  	)
>  '

Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly

[Eric Sunshine]

On Thu, Jul 11, 2013 at 3:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> With the introduction of check-mailmap, it is now possible to check
>> .mailmap functionality directly rather than indirectly as a side-effect
>> of other commands (such as git-shortlog), therefore, do so.
>
> Does this patch mean that we will now ignore future breakages in
> shortlog and blame if their mailmap integration becomes buggy?
>
> I am not convinced it is a good idea if that is what is going on.

I meant to mention in the cover letter that this patch was open for
debate, however, it does not eliminate all testing of these other
commands.

The tests in which git-check-mailmap is substituted for git-shortlog
all worked against a simplistic two-commit repository. Those tests
were checking the low-level mailmap functionality under various
conditions and configurations; they were not especially checking any
particular behavior of git-shortlog.

There still remain a final eight tests which cover git-shortlog,
git-log, and git-blame. These tests do check mailmap-related behavior
of those commands, and they do so using a more involved seven-commit
repository with "complex" mappings, so we have not necessarily lost
any checks of mailmap integration for those commands.

Would this patch become more palatable if I stated the above in the
commit message?

-- ES

Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly

[Jonathan Nieder]

Eric Sunshine wrote:
> On Thu, Jul 11, 2013 at 3:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:

>>> With the introduction of check-mailmap, it is now possible to check
>>> .mailmap functionality directly rather than indirectly as a side-effect
>>> of other commands (such as git-shortlog), therefore, do so.
>>
>> Does this patch mean that we will now ignore future breakages in
>> shortlog and blame if their mailmap integration becomes buggy?
>>
>> I am not convinced it is a good idea if that is what is going on.
>
> I meant to mention in the cover letter that this patch was open for
> debate, however, it does not eliminate all testing of these other
> commands.
>
> The tests in which git-check-mailmap is substituted for git-shortlog
> all worked against a simplistic two-commit repository. Those tests
> were checking the low-level mailmap functionality under various
> conditions and configurations; they were not especially checking any
> particular behavior of git-shortlog.
>
> There still remain a final eight tests which cover git-shortlog,
> git-log, and git-blame. These tests do check mailmap-related behavior
> of those commands, and they do so using a more involved seven-commit
> repository with "complex" mappings, so we have not necessarily lost
> any checks of mailmap integration for those commands.
>
> Would this patch become more palatable if I stated the above in the
> commit message?

My current thinking is "no" --- the patch has as a justification "Now
we can test these aspects of .mailmap handling directly with a
low-level tool instead of using the tool most people will use, so do
so", which sounds an awful lot like "Reduce test coverage of commonly
used tools, because we can".

But I imagine the actual motivation was something other than "because
we can".  For example, maybe the idea is that after this patch, it
should be easier to make cosmetic improvements to the shortlog, log,
and blame output and only have to change those final 8 tests that are
adequately covering the output?  If you have such plans and this patch
makes them easier, it could sound like a good patch as a stepping
stone toward that.

Thanks and hope that helps,
Jonathan

Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly

[Eric Sunshine]

On Thu, Jul 11, 2013 at 8:55 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Eric Sunshine wrote:
>> On Thu, Jul 11, 2013 at 3:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>>> With the introduction of check-mailmap, it is now possible to check
>>>> .mailmap functionality directly rather than indirectly as a side-effect
>>>> of other commands (such as git-shortlog), therefore, do so.
>>>
>>> Does this patch mean that we will now ignore future breakages in
>>> shortlog and blame if their mailmap integration becomes buggy?
>>>
>>> I am not convinced it is a good idea if that is what is going on.
>>
>> I meant to mention in the cover letter that this patch was open for
>> debate, however, it does not eliminate all testing of these other
>> commands.
>>
>> The tests in which git-check-mailmap is substituted for git-shortlog
>> all worked against a simplistic two-commit repository. Those tests
>> were checking the low-level mailmap functionality under various
>> conditions and configurations; they were not especially checking any
>> particular behavior of git-shortlog.
>>
>> There still remain a final eight tests which cover git-shortlog,
>> git-log, and git-blame. These tests do check mailmap-related behavior
>> of those commands, and they do so using a more involved seven-commit
>> repository with "complex" mappings, so we have not necessarily lost
>> any checks of mailmap integration for those commands.
>>
>> Would this patch become more palatable if I stated the above in the
>> commit message?
>
> My current thinking is "no" --- the patch has as a justification "Now
> we can test these aspects of .mailmap handling directly with a
> low-level tool instead of using the tool most people will use, so do
> so", which sounds an awful lot like "Reduce test coverage of commonly
> used tools, because we can".
>
> But I imagine the actual motivation was something other than "because
> we can".

The motivation is as stated in the subject: Direct rather than
indirect testing of mailmap functionality. The tests in question are
properly crafted to check only low-level mailmap (not git-shortlog)
functionality under various conditions and configurations, yet the
mental load on the reader is high because he has to keep in mind the
authors and commits in the repository underlying git-shortlog's
output. When testing via git-check-mailmap, mental load is low: there
is a direct correlation between the "Name <email@address>" which goes
in and that which comes out. What is being tested is the same, but
it's easier to reason about and understand the the tests when done
directly.

When converting the tests, I also discovered an additional, perhaps
more important motivation. Most of the tests check only that name
remapping functions correctly, however, a couple also attempt to check
address remapping (bugs@company.xx => bugs@company.xy). Even though
the tests succeed, they don't actually prove that address remapping
was correct (or occurred at all) since git-shortlog does not display
the email address. git-check-mailmap always displays the email
address, thus the check actually tests the intended email address
remapping.

> For example, maybe the idea is that after this patch, it
> should be easier to make cosmetic improvements to the shortlog, log,
> and blame output and only have to change those final 8 tests that are
> adequately covering the output?  If you have such plans and this patch
> makes them easier, it could sound like a good patch as a stepping
> stone toward that.

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

[Eric Sunshine]

On Thu, Jul 11, 2013 at 3:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

I find it easier than your original, but I do not know if you would
want to repeat the "Name... or <user@host>" at the end.  It does not
seem to add much useful information and is distracting.

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

That is unfortunate but it is good that you found the breakage.  As
we do not have --nul-terminated-input and --nul-terminated-output
options separtely, -z should apply to both input and output.  What
b4666852 (check-attr: Add --stdin option, 2008-10-07) did is broken.
What check-ignore does 

We should find a way to fix it.  I have a feeling that silently
fixing it and seeing if anybody screams might be the best course of
action ;-).

Also "git check-ignore -h" advertises "-z" as only affecting "--stdin",
which is also wrong.  It does affect both input and output as it should,
so it should be described as such, I think.

Thanks for noticing.

Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> My current thinking is "no" --- the patch has as a justification "Now
> we can test these aspects of .mailmap handling directly with a
> low-level tool instead of using the tool most people will use, so do
> so", which sounds an awful lot like "Reduce test coverage of commonly
> used tools, because we can".

Yes, that was exactly my reaction that prompted my response.

Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly

[Eric Sunshine]

On Fri, Jul 12, 2013 at 1:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> My current thinking is "no" --- the patch has as a justification "Now
>> we can test these aspects of .mailmap handling directly with a
>> low-level tool instead of using the tool most people will use, so do
>> so", which sounds an awful lot like "Reduce test coverage of commonly
>> used tools, because we can".
>
> Yes, that was exactly my reaction that prompted my response.

Does any of my follow-up commentary result in a different reaction? If
not, I'll drop patches 3 &4 in the re-roll.

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

[Eric Sunshine]

On Fri, Jul 12, 2013 at 1:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>>         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.
>
> I find it easier than your original, but I do not know if you would
> want to repeat the "Name... or <user@host>" at the end.  It does not
> seem to add much useful information and is distracting.

Next attempt:

    For each ``Name $$<user@host>$$'' or ``$$<user@host>$$'' from the
    command-line or standard input (when using `--stdin`) look up the
    person's canonical name and email address (see "Mapping Authors"
    below). If found, print them; otherwise print the input as-is.

>> 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.
>
> That is unfortunate but it is good that you found the breakage.  As
> we do not have --nul-terminated-input and --nul-terminated-output
> options separtely, -z should apply to both input and output.  What
> b4666852 (check-attr: Add --stdin option, 2008-10-07) did is broken.

I can make git-check-mailmap behave this way, however, other than
git-check-ignore (which is quite new), there doesn't seem to be any
precedence (that I can find) anywhere else in git which ties input and
output null-termination to a single switch. Is it desirable to do so
or should the user have more fine-grained control? ("xargs -0" comes
to mind when thinking of a null-termination input switch.)

> Also "git check-ignore -h" advertises "-z" as only affecting "--stdin",
> which is also wrong.  It does affect both input and output as it should,
> so it should be described as such, I think.

I also noticed this. (It was copied from check-attr.c).

Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Fri, Jul 12, 2013 at 1:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>>> My current thinking is "no" --- the patch has as a justification "Now
>>> we can test these aspects of .mailmap handling directly with a
>>> low-level tool instead of using the tool most people will use, so do
>>> so", which sounds an awful lot like "Reduce test coverage of commonly
>>> used tools, because we can".
>>
>> Yes, that was exactly my reaction that prompted my response.
>
> Does any of my follow-up commentary result in a different
> reaction?

Not really.  While I _do_ think direct testing has merits, I think
that should be done by adding direct tests, not by removing the
tests that are meant to protect higher level _users_ of the
underlying mechanism from breakages.  After all, their breakages may
not come from new breakages of the lower level mechanism (i.e. the
mailmap.c code) but the way these higher level code makes calls into
the mechanism, and direct test of the lower level mechanism will not
protect them from the latter kind of breakages.

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> I find it easier than your original, but I do not know if you would
>> want to repeat the "Name... or <user@host>" at the end.  It does not
>> seem to add much useful information and is distracting.
>
> Next attempt:
>
>     For each ``Name $$<user@host>$$'' or ``$$<user@host>$$'' from the
>     command-line or standard input (when using `--stdin`) look up the
>     person's canonical name and email address (see "Mapping Authors"
>     below). If found, print them; otherwise print the input as-is.

Nice.

> ... Is it desirable to do so
> or should the user have more fine-grained control? ("xargs -0" comes
> to mind when thinking of a null-termination input switch.)

For the purposes of check-attr and check-ignore, a single "-z"
governing both is sufficient.  I think you already got that from my
4-patch series, but the core reason for that is :

 - when "-z" is used, the user knows the input paths may need
   protection against LF.

 - our output contains these same paths.

 - which means our output cannot be expressed unambiguously using LF
   as record separator.

For the purpose of check-mailmap, I actually do not see much point
in supporting "-z" format.  We do not even handle names or addresses
with LF in it.  The mailmap format would not let you express such
records in the first place, no?

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

[Eric Sunshine]

On Fri, Jul 12, 2013 at 2:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> ... Is it desirable to do so
>> or should the user have more fine-grained control? ("xargs -0" comes
>> to mind when thinking of a null-termination input switch.)
>
> For the purposes of check-attr and check-ignore, a single "-z"
> governing both is sufficient.  I think you already got that from my
> 4-patch series, but the core reason for that is :

I'm reading it right now.

>  - when "-z" is used, the user knows the input paths may need
>    protection against LF.
>
>  - our output contains these same paths.
>
>  - which means our output cannot be expressed unambiguously using LF
>    as record separator.
>
> For the purpose of check-mailmap, I actually do not see much point
> in supporting "-z" format.  We do not even handle names or addresses
> with LF in it.  The mailmap format would not let you express such
> records in the first place, no?

You're right. I had this exact argument in mind for why
null-termination was not needed on the input side of check-mailmap,
but for some reason I had a blind spot concerning the output side.
I'll drop this option in the next re-roll.