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

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

[Eric Sunshine]

This patch series adds builtin command check-mailmap, similar to
check-attr and check-ignore, which allows direct testing of .mailmap
configuration.  More importantly, as plumbing accessible to scripts
and other porcelain, check-mailmap publishes the stable, well-tested
.mailmap functionality employed by built-in Git commands.

It is RFC because it lacks documentation; because its utility as a
check-foo command is minimal compared to check-attr and check-ignore
(although its utility as plumbing for scripts and porcelain is more
significant); and because I want to make sure that the project is
willing to accept yet another check-foo command.

The idea and motivation for git-check-mailmap arose from the effort to
implement .mailmap support for git-contacts [1] (presently at
es/contacts in 'pu').

Felipe's Ruby implementation of .mailmap support for his git-related
script is introduced in patch 9/15 [2] of v5, and is augmented in patch
10/15 [3] to support invocation from within a subdirectory. His version
supports configuration variable mailmap.file, but not mailmap.blob.

Rather than rewriting the functionality yet again, this time in Perl,
along with all the details and nuances of the C version employed by
Git builtins, I decided that it made more sense to expose the
well-tested and polished C implementation as plumbing, thus allowing
any script or porcelain to take advantage of it.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/229533/
[2]: http://article.gmane.org/gmane.comp.version-control.git/224782/
[3]: http://article.gmane.org/gmane.comp.version-control.git/224783/

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 +
 Makefile                               |   1 +
 builtin.h                              |   1 +
 builtin/check-mailmap.c                |  73 ++++++++++
 command-list.txt                       |   1 +
 contrib/completion/git-completion.bash |   1 +
 git.c                                  |   1 +
 t/t4203-mailmap.sh                     | 246 +++++++++++++++++----------------
 8 files changed, 209 insertions(+), 116 deletions(-)
 create mode 100644 builtin/check-mailmap.c

-- 
1.8.3.2

[RFC/PATCH 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 +
 Makefile                               |  1 +
 builtin.h                              |  1 +
 builtin/check-mailmap.c                | 73 ++++++++++++++++++++++++++++++++++
 command-list.txt                       |  1 +
 contrib/completion/git-completion.bash |  1 +
 git.c                                  |  1 +
 7 files changed, 79 insertions(+)
 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/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..cbcc6db
--- /dev/null
+++ b/builtin/check-mailmap.c
@@ -0,0 +1,73 @@
+#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_BOOLEAN(0, "stdin", &use_stdin,
+		    N_("also read contacts from stdin")),
+	OPT_BOOLEAN('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 <%.*s>%c",
+			(int)namelen, name, (int)maillen, mail, term);
+	else
+		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, "contact to 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, "contact to 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

[RFC/PATCH 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

[RFC/PATCH 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

[RFC/PATCH 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 repository-setup function.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

The code near the top of the file which creates two commits, and the
code later which creates five commits have been consolidated into a
single repository-setup function. The patch is primarily code movement,
but the noisy diff makes it look more busy than it actually is.


 t/t4203-mailmap.sh | 69 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 31 deletions(-)

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 48a000b..7e8b3cc 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -13,19 +13,10 @@ fuzz_blame () {
 }
 
 test_expect_success setup '
-	cat >contacts <<-\EOF &&
+	cat >contacts <<-\EOF
 	A U Thor <author@example.com>
 	nick1 <bugs@company.xx>
 	EOF
-
-	echo one >one &&
-	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
 '
 
 test_expect_success 'check-mailmap no arguments' '
@@ -168,8 +159,10 @@ test_expect_success 'No mailmap files, but configured' '
 '
 
 test_expect_success 'setup mailmap blob tests' '
+	git checkout --orphan empty &&
+	git commit --allow-empty --allow-empty-message &&
 	git checkout -b map &&
-	test_when_finished "git checkout master" &&
+	test_when_finished "git checkout empty" &&
 	cat >just-bugs <<-\EOF &&
 	Blob Guy <bugs@company.xx>
 	EOF
@@ -254,28 +247,19 @@ test_expect_success 'cleanup after mailmap.blob tests' '
 	rm -f .mailmap
 '
 
-# Extended mailmap configurations should give us the following output for shortlog
-cat >expect <<\EOF
-A U Thor <author@example.com> (1):
-      initial
-
-CTO <cto@company.xx> (1):
-      seventh
+test_expect_success 'setup mailmap test repo' '
+	git checkout --orphan master &&
 
-Other Author <other@author.xx> (2):
-      third
-      fourth
-
-Santa Claus <santa.claus@northpole.xx> (2):
-      fifth
-      sixth
-
-Some Dude <some@dude.xx> (1):
-      second
+	echo one >one &&
+	git add one &&
+	test_tick &&
+	git commit -m initial &&
 
-EOF
+	echo two >>one &&
+	git add one &&
+	test_tick &&
+	git commit --author "nick1 <bugs@company.xx>" -m second &&
 
-test_expect_success 'Shortlog output (complex mapping)' '
 	echo three >>one &&
 	git add one &&
 	test_tick &&
@@ -299,8 +283,31 @@ test_expect_success 'Shortlog output (complex mapping)' '
 	echo seven >>one &&
 	git add one &&
 	test_tick &&
-	git commit --author "CTO <cto@coompany.xx>" -m seventh &&
+	git commit --author "CTO <cto@coompany.xx>" -m seventh
+'
+
+# Extended mailmap configurations should give us the following output for shortlog
+cat >expect <<\EOF
+A U Thor <author@example.com> (1):
+      initial
+
+CTO <cto@company.xx> (1):
+      seventh
 
+Other Author <other@author.xx> (2):
+      third
+      fourth
+
+Santa Claus <santa.claus@northpole.xx> (2):
+      fifth
+      sixth
+
+Some Dude <some@dude.xx> (1):
+      second
+
+EOF
+
+test_expect_success 'Shortlog output (complex mapping)' '
 	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: [RFC/PATCH 1/4] builtin: add git-check-mailmap command

[Duy Nguyen]

On Thu, Jul 11, 2013 at 2:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> +static const struct option check_mailmap_options[] = {
> +       OPT_BOOLEAN(0, "stdin", &use_stdin,
> +                   N_("also read contacts from stdin")),
> +       OPT_BOOLEAN('z', NULL, &null_out,
> +                   N_("null-terminate output lines")),

I think OPT_BOOLEAN is deprecated in favor of OPT_BOOL (or OPT_COUNTUP
if you really want -z -z -z to mean differently than -z)

> +       maybe_flush_or_die(stdout, "contact to stdout");

On error this function will print

write failure on 'contact to stdout'

maybe maybe_flush_or_die(stdout, "write contact to stdout") or
something? From i18n point of view, maybe_flush_or_die should not
compose a sentence this way. Let the second argument be a complete
sentence so that translators have more freedom. But that's a different
issue.
--
Duy

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

[Eric Sunshine]

On Wed, Jul 10, 2013 at 10:31 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Thu, Jul 11, 2013 at 2:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> +static const struct option check_mailmap_options[] = {
>> +       OPT_BOOLEAN(0, "stdin", &use_stdin,
>> +                   N_("also read contacts from stdin")),
>> +       OPT_BOOLEAN('z', NULL, &null_out,
>> +                   N_("null-terminate output lines")),
>
> I think OPT_BOOLEAN is deprecated in favor of OPT_BOOL (or OPT_COUNTUP
> if you really want -z -z -z to mean differently than -z)

Thanks, I knew this and intended to change it to OPT_BOOL but forgot.
(The OPT_BOOLEAN was inherited from check-attr.c and check-ignore.c
which I used as templates for check-mailmap.c.)

>> +       maybe_flush_or_die(stdout, "contact to stdout");
>
> On error this function will print
>
> write failure on 'contact to stdout'
>
> maybe maybe_flush_or_die(stdout, "write contact to stdout") or
> something? From i18n point of view, maybe_flush_or_die should not
> compose a sentence this way. Let the second argument be a complete
> sentence so that translators have more freedom. But that's a different
> issue.

Indeed, it's not ideal. I chose "contact to stdout" for consistency
with other callers, not because of a fondness for it.

  % git grep maybe_flush_or_die
  builtin/blame.c: maybe_flush_or_die(stdout, "stdout");
  builtin/check-attr.c: maybe_flush_or_die(stdout, "attribute to stdout");
  builtin/check-attr.c: maybe_flush_or_die(stdout, "attribute to stdout");
  builtin/check-ignore.c: maybe_flush_or_die(stdout, "check-ignore to stdout");
  builtin/check-ignore.c: maybe_flush_or_die(stdout, "ignore to stdout");
  builtin/hash-object.c: maybe_flush_or_die(stdout, "hash to stdout");
  builtin/rev-list.c: maybe_flush_or_die(stdout, "stdout");
  log-tree.c: maybe_flush_or_die(stdout, "stdout");

They seem pretty evenly split between just "stdout" and "foo to
stdout". (I actually prefer plain "stdout" and will happily change it
to that.)

-- ES

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

[Antoine Pelisse]

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 ?

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

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

[Duy Nguyen]

On Thu, Jul 11, 2013 at 12:50 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> +       maybe_flush_or_die(stdout, "contact to stdout");
>>
>> On error this function will print
>>
>> write failure on 'contact to stdout'
>>
>> maybe maybe_flush_or_die(stdout, "write contact to stdout") or
>> something? From i18n point of view, maybe_flush_or_die should not
>> compose a sentence this way. Let the second argument be a complete
>> sentence so that translators have more freedom. But that's a different
>> issue.
>
> Indeed, it's not ideal. I chose "contact to stdout" for consistency
> with other callers, not because of a fondness for it.
>
>   % git grep maybe_flush_or_die
>   builtin/blame.c: maybe_flush_or_die(stdout, "stdout");
>   builtin/check-attr.c: maybe_flush_or_die(stdout, "attribute to stdout");
>   builtin/check-attr.c: maybe_flush_or_die(stdout, "attribute to stdout");
>   builtin/check-ignore.c: maybe_flush_or_die(stdout, "check-ignore to stdout");
>   builtin/check-ignore.c: maybe_flush_or_die(stdout, "ignore to stdout");
>   builtin/hash-object.c: maybe_flush_or_die(stdout, "hash to stdout");
>   builtin/rev-list.c: maybe_flush_or_die(stdout, "stdout");
>   log-tree.c: maybe_flush_or_die(stdout, "stdout");
>
> They seem pretty evenly split between just "stdout" and "foo to
> stdout". (I actually prefer plain "stdout" and will happily change it
> to that.)

Then probably stick to the convention. If this i18n thing is fixed, it
has to be fixed everywhere anyway.
-- 
Duy

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

[Eric Sunshine]

On Thu, Jul 11, 2013 at 2:45 AM, Antoine Pelisse <apelisse@gmail.com> wrote:
> 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.

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