[PATCH] git-commit: search author pattern against mailmap

[PATCH] git-commit: search author pattern against mailmap

[Antoine Pelisse]

When committing for someone else, using the --author option, it can be
nice to use the mailmap file to find the correct name spelling and email
address.

Currently, you would have to find the correct mapping in mailmap file
first, and then use the full ident form when committing.

Let's allow git-commit to find if an entry exists in mailmap file for
that pattern, and use that instead.

Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
Hi,
I would use that feature at work where I happen to commit some work for
other colleagues, while we heavily rely on mailmap file to have decent indents.

On the other hand, I'm kind of embarrassed to add this new option to
git-commit.

 Documentation/git-commit.txt |  6 +++++-
 builtin/commit.c             | 16 +++++++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 1a7616c..9e3fe04 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 	   [--dry-run] [(-c | -C | --fixup | --squash) <commit>]
 	   [-F <file> | -m <msg>] [--reset-author] [--allow-empty]
 	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
-	   [--date=<date>] [--cleanup=<mode>] [--[no-]status]
+	   [--use-mailmap] [--date=<date>] [--cleanup=<mode>] [--[no-]status]
 	   [-i | -o] [-S[<keyid>]] [--] [<file>...]

 DESCRIPTION
@@ -131,6 +131,10 @@ OPTIONS
 	commit by that author (i.e. rev-list --all -i --author=<author>);
 	the commit author is then copied from the first such commit found.

+--use-mailmap::
+	When used with `--author=<author>`, match the <author> pattern
+	against mapped name and email. See linkgit:git-shortlog[1].
+
 --date=<date>::
 	Override the author date used in the commit.

diff --git a/builtin/commit.c b/builtin/commit.c
index 10acc53..fbd0664 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -30,6 +30,7 @@
 #include "column.h"
 #include "sequencer.h"
 #include "notes-utils.h"
+#include "mailmap.h"

 static const char * const builtin_commit_usage[] = {
 	N_("git commit [options] [--] <pathspec>..."),
@@ -87,6 +88,7 @@ static enum {
 } commit_style;

 static const char *logfile, *force_author;
+static int mailmap;
 static const char *template_file;
 /*
  * The _message variables are commit names from which to take
@@ -945,13 +947,24 @@ static const char *find_author_by_nickname(const char *name)
 	av[++ac] = buf.buf;
 	av[++ac] = NULL;
 	setup_revisions(ac, av, &revs, NULL);
+	if (mailmap) {
+		revs.mailmap = xcalloc(1, sizeof(struct string_list));
+		read_mailmap(revs.mailmap, NULL);
+	}
 	prepare_revision_walk(&revs);
 	commit = get_revision(&revs);
 	if (commit) {
 		struct pretty_print_context ctx = {0};
+		const char *format;
+
+		if (mailmap)
+			format = "%aN <%aE>";
+		else
+			format = "%an <%ae>";
+
 		ctx.date_mode = DATE_NORMAL;
 		strbuf_release(&buf);
-		format_commit_message(commit, "%an <%ae>", &buf, &ctx);
+		format_commit_message(commit, format, &buf, &ctx);
 		return strbuf_detach(&buf, NULL);
 	}
 	die(_("No existing author found with '%s'"), name);
@@ -1428,6 +1441,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_GROUP(N_("Commit message options")),
 		OPT_FILENAME('F', "file", &logfile, N_("read message from file")),
 		OPT_STRING(0, "author", &force_author, N_("author"), N_("override author for commit")),
+		OPT_BOOLEAN(0, "use-mailmap", &mailmap, N_("Use mailmap file when searching for author")),
 		OPT_STRING(0, "date", &force_date, N_("date"), N_("override date for commit")),
 		OPT_CALLBACK('m', "message", &message, N_("message"), N_("commit message"), opt_parse_m),
 		OPT_STRING('c', "reedit-message", &edit_message, N_("commit"), N_("reuse and edit message from specified commit")),
--
1.8.4.rc4.1.g0d8beaa.dirty

Re: [PATCH] git-commit: search author pattern against mailmap

[Junio C Hamano]

Antoine Pelisse <apelisse@gmail.com> writes:

> When committing for someone else, using the --author option, it can be
> nice to use the mailmap file to find the correct name spelling and email
> address.
>
> Currently, you would have to find the correct mapping in mailmap file
> first, and then use the full ident form when committing.
>
> Let's allow git-commit to find if an entry exists in mailmap file for
> that pattern, and use that instead.
>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---
> Hi,
> I would use that feature at work where I happen to commit some work for
> other colleagues, while we heavily rely on mailmap file to have decent indents.
>
> On the other hand, I'm kind of embarrassed to add this new option to
> git-commit.

My initial reaction was "Why should something as important as 'git
commit' should be playing a guessing-game?" ;-) and I am kind of
ashamed to have added 146ea068 (git commit --author=$name: look
$name up in existing commits, 2008-08-26) and then am embarrased to
have completely forgotten about it. I never use the feature myself.

But for that old and established "--author parameter that does not
use the standard format guesses" feature to be useful, I agree that
it should honor the mailmap.

I wonder if it would hurt anybody if we made this unconditional, not
even with "--no-mailmap" override? Opinions?

>
>  Documentation/git-commit.txt |  6 +++++-
>  builtin/commit.c             | 16 +++++++++++++++-
>  2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index 1a7616c..9e3fe04 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -12,7 +12,7 @@ SYNOPSIS
>  	   [--dry-run] [(-c | -C | --fixup | --squash) <commit>]
>  	   [-F <file> | -m <msg>] [--reset-author] [--allow-empty]
>  	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
> -	   [--date=<date>] [--cleanup=<mode>] [--[no-]status]
> +	   [--use-mailmap] [--date=<date>] [--cleanup=<mode>] [--[no-]status]
>  	   [-i | -o] [-S[<keyid>]] [--] [<file>...]
>
>  DESCRIPTION
> @@ -131,6 +131,10 @@ OPTIONS
>  	commit by that author (i.e. rev-list --all -i --author=<author>);
>  	the commit author is then copied from the first such commit found.
>
> +--use-mailmap::
> +	When used with `--author=<author>`, match the <author> pattern
> +	against mapped name and email. See linkgit:git-shortlog[1].
> +
>  --date=<date>::
>  	Override the author date used in the commit.
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 10acc53..fbd0664 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -30,6 +30,7 @@
>  #include "column.h"
>  #include "sequencer.h"
>  #include "notes-utils.h"
> +#include "mailmap.h"
>
>  static const char * const builtin_commit_usage[] = {
>  	N_("git commit [options] [--] <pathspec>..."),
> @@ -87,6 +88,7 @@ static enum {
>  } commit_style;
>
>  static const char *logfile, *force_author;
> +static int mailmap;
>  static const char *template_file;
>  /*
>   * The _message variables are commit names from which to take
> @@ -945,13 +947,24 @@ static const char *find_author_by_nickname(const char *name)
>  	av[++ac] = buf.buf;
>  	av[++ac] = NULL;
>  	setup_revisions(ac, av, &revs, NULL);
> +	if (mailmap) {
> +		revs.mailmap = xcalloc(1, sizeof(struct string_list));
> +		read_mailmap(revs.mailmap, NULL);
> +	}
>  	prepare_revision_walk(&revs);
>  	commit = get_revision(&revs);
>  	if (commit) {
>  		struct pretty_print_context ctx = {0};
> +		const char *format;
> +
> +		if (mailmap)
> +			format = "%aN <%aE>";
> +		else
> +			format = "%an <%ae>";
> +
>  		ctx.date_mode = DATE_NORMAL;
>  		strbuf_release(&buf);
> -		format_commit_message(commit, "%an <%ae>", &buf, &ctx);
> +		format_commit_message(commit, format, &buf, &ctx);
>  		return strbuf_detach(&buf, NULL);
>  	}
>  	die(_("No existing author found with '%s'"), name);
> @@ -1428,6 +1441,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		OPT_GROUP(N_("Commit message options")),
>  		OPT_FILENAME('F', "file", &logfile, N_("read message from file")),
>  		OPT_STRING(0, "author", &force_author, N_("author"), N_("override author for commit")),
> +		OPT_BOOLEAN(0, "use-mailmap", &mailmap, N_("Use mailmap file when searching for author")),
>  		OPT_STRING(0, "date", &force_date, N_("date"), N_("override date for commit")),
>  		OPT_CALLBACK('m', "message", &message, N_("message"), N_("commit message"), opt_parse_m),
>  		OPT_STRING('c', "reedit-message", &edit_message, N_("commit"), N_("reuse and edit message from specified commit")),
> --
> 1.8.4.rc4.1.g0d8beaa.dirty

Re: [PATCH] git-commit: search author pattern against mailmap

[Jeff King]

On Fri, Aug 23, 2013 at 10:44:03AM -0700, Junio C Hamano wrote:

> My initial reaction was "Why should something as important as 'git
> commit' should be playing a guessing-game?" ;-) and I am kind of
> ashamed to have added 146ea068 (git commit --author=$name: look
> $name up in existing commits, 2008-08-26) and then am embarrased to
> have completely forgotten about it. I never use the feature myself.
> 
> But for that old and established "--author parameter that does not
> use the standard format guesses" feature to be useful, I agree that
> it should honor the mailmap.
> 
> I wonder if it would hurt anybody if we made this unconditional, not
> even with "--no-mailmap" override? Opinions?

I think it would be OK. You can always override by giving the actual
full address you want instead of a partial one. And if somebody is not
up to date in the .mailmap file, maybe this would be a good hint that
you should take care of that. :)

I paused for a second, thinking that such advice might not be good for
people who do not want to make an official change to upstream's
.mailmap (e.g., because they do not want to pollute a long-running fork
that will need to merge from upstream, or do not want to pollute a topic
branch with an unrelated commit). But I forgot that we have
mailmap.file, if they want something custom.

So I think anyone for whom the mailmap lookup does not provide the right
answer will fall into one of two groups:

  1. A one-off, which can be overridden by specifying the address you
     do want.

  2. Somebody you will be mentioning frequently; bother to set up
     a mailmap.file.

As an aside, it seems silly that we do not respect $GIT_DIR/mailmap by
default, even without a config option. But I doubt that anybody cares
too much, if nobody has raised the issue in all of these years.

-Peff

Re: [PATCH] git-commit: search author pattern against mailmap

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> But for that old and established "--author parameter that does not
>> use the standard format guesses" feature to be useful, I agree that
>> it should honor the mailmap.
>> 
>> I wonder if it would hurt anybody if we made this unconditional, not
>> even with "--no-mailmap" override? Opinions?
>
> I think it would be OK. You can always override by giving the actual
> full address you want instead of a partial one.

OK, so how about labelling it as a bugfix, like this perhaps?  We
obviously need a test or two, though.

-- >8 --
From: Antoine Pelisse <apelisse@gmail.com>
Date: Fri, 23 Aug 2013 15:48:31 +0200
Subject: [PATCH] commit: search author pattern against mailmap

"git commit --author=$name" sets the author to one whose name
matches the given string from existing commits, when $name is not in
the "Name <e-mail>" format. However, it does not honor the mailmap
to use the canonical name for the author found this way.

Fix it by telling the logic to find a matching existing author to
honor the mailmap, and use the name and email after applying the
mailmap.

Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/commit.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 10acc53..5b7d969 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -30,6 +30,7 @@
 #include "column.h"
 #include "sequencer.h"
 #include "notes-utils.h"
+#include "mailmap.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [options] [--] <pathspec>..."),
@@ -945,13 +946,16 @@ static const char *find_author_by_nickname(const char *name)
 	av[++ac] = buf.buf;
 	av[++ac] = NULL;
 	setup_revisions(ac, av, &revs, NULL);
+	revs.mailmap = xcalloc(1, sizeof(struct string_list));
+	read_mailmap(revs.mailmap, NULL);
+
 	prepare_revision_walk(&revs);
 	commit = get_revision(&revs);
 	if (commit) {
 		struct pretty_print_context ctx = {0};
 		ctx.date_mode = DATE_NORMAL;
 		strbuf_release(&buf);
-		format_commit_message(commit, "%an <%ae>", &buf, &ctx);
+		format_commit_message(commit, "%aN <%aE>", &buf, &ctx);
 		return strbuf_detach(&buf, NULL);
 	}
 	die(_("No existing author found with '%s'"), name);
-- 
1.8.4-rc4-299-g7e07a8d

Re: [PATCH] git-commit: search author pattern against mailmap

[Antoine Pelisse]

On Fri, Aug 23, 2013 at 9:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> OK, so how about labelling it as a bugfix, like this perhaps?  We
> obviously need a test or two, though.

OK,
I will resubmit tomorrow with some tests.

Re: [PATCH] git-commit: search author pattern against mailmap

[Junio C Hamano]

Antoine Pelisse <apelisse@gmail.com> writes:

> On Fri, Aug 23, 2013 at 9:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> OK, so how about labelling it as a bugfix, like this perhaps?  We
>> obviously need a test or two, though.
>
> OK,
> I will resubmit tomorrow with some tests.

Thanks.

Also, after I sent out that "like this" patch, I realized that the
mailmap string-list no longer has to be on the heap if we are doing
it unconditionally (it can be an auto variable in the function,
sitting next to "struct strbuf buf").

[PATCH] commit: search author pattern against mailmap

[Antoine Pelisse]

"git commit --author=$name" sets the author to one whose name
matches the given string from existing commits, when $name is not in
the "Name <e-mail>" format. However, it does not honor the mailmap
to use the canonical name for the author found this way.

Fix it by telling the logic to find a matching existing author to
honor the mailmap, and use the name and email after applying the
mailmap.

Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
 builtin/commit.c   |  7 ++++++-
 t/t4203-mailmap.sh | 11 +++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 10acc53..21e0f95 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -30,6 +30,7 @@
 #include "column.h"
 #include "sequencer.h"
 #include "notes-utils.h"
+#include "mailmap.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [options] [--] <pathspec>..."),
@@ -935,6 +936,7 @@ static const char *find_author_by_nickname(const char *name)
 	struct rev_info revs;
 	struct commit *commit;
 	struct strbuf buf = STRBUF_INIT;
+	struct string_list mailmap = STRING_LIST_INIT_NODUP;
 	const char *av[20];
 	int ac = 0;
 
@@ -945,13 +947,16 @@ static const char *find_author_by_nickname(const char *name)
 	av[++ac] = buf.buf;
 	av[++ac] = NULL;
 	setup_revisions(ac, av, &revs, NULL);
+	revs.mailmap = &mailmap;
+	read_mailmap(revs.mailmap, NULL);
+
 	prepare_revision_walk(&revs);
 	commit = get_revision(&revs);
 	if (commit) {
 		struct pretty_print_context ctx = {0};
 		ctx.date_mode = DATE_NORMAL;
 		strbuf_release(&buf);
-		format_commit_message(commit, "%an <%ae>", &buf, &ctx);
+		format_commit_message(commit, "%aN <%aE>", &buf, &ctx);
 		return strbuf_detach(&buf, NULL);
 	}
 	die(_("No existing author found with '%s'"), name);
diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index baa4685..4d715f0 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -470,4 +470,15 @@ test_expect_success 'Blame output (complex mapping)' '
 	test_cmp expect actual.fuzz
 '
 
+cat >expect <<\EOF
+Some Dude <some@dude.xx>
+EOF
+
+test_expect_success 'commit --author honors mailmap' '
+	test_must_fail git commit --author "nick" --allow-empty -meight &&
+	git commit --author "Some Dude" --allow-empty -meight &&
+	git show --pretty=format:"%an <%ae>%n" >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.4.rc4.2.g8483dfa

Re: [PATCH] commit: search author pattern against mailmap

[Jeff King]

On Sat, Aug 24, 2013 at 04:07:47PM +0200, Antoine Pelisse wrote:

> @@ -945,13 +947,16 @@ static const char *find_author_by_nickname(const char *name)
>  	av[++ac] = buf.buf;
>  	av[++ac] = NULL;
>  	setup_revisions(ac, av, &revs, NULL);
> +	revs.mailmap = &mailmap;
> +	read_mailmap(revs.mailmap, NULL);
> +
>  	prepare_revision_walk(&revs);
>  	commit = get_revision(&revs);
>  	if (commit) {
>  		struct pretty_print_context ctx = {0};
>  		ctx.date_mode = DATE_NORMAL;
>  		strbuf_release(&buf);
> -		format_commit_message(commit, "%an <%ae>", &buf, &ctx);
> +		format_commit_message(commit, "%aN <%aE>", &buf, &ctx);
>  		return strbuf_detach(&buf, NULL);
>  	}
>  	die(_("No existing author found with '%s'"), name);

Do we need to clear_mailmap before returning to avoid a leak?

I suspect we may be leaking pending commits from the revision walker,
too, but I'm not sure we have an easy "clear everything" function there.

-Peff

Re: [PATCH] commit: search author pattern against mailmap

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Aug 24, 2013 at 04:07:47PM +0200, Antoine Pelisse wrote:
>
>> @@ -945,13 +947,16 @@ static const char *find_author_by_nickname(const char *name)
>>  	av[++ac] = buf.buf;
>>  	av[++ac] = NULL;
>>  	setup_revisions(ac, av, &revs, NULL);
>> +	revs.mailmap = &mailmap;
>> +	read_mailmap(revs.mailmap, NULL);
>> +
>>  	prepare_revision_walk(&revs);
>>  	commit = get_revision(&revs);
>>  	if (commit) {
>>  		struct pretty_print_context ctx = {0};
>>  		ctx.date_mode = DATE_NORMAL;
>>  		strbuf_release(&buf);
>> -		format_commit_message(commit, "%an <%ae>", &buf, &ctx);
>> +		format_commit_message(commit, "%aN <%aE>", &buf, &ctx);
>>  		return strbuf_detach(&buf, NULL);
>>  	}
>>  	die(_("No existing author found with '%s'"), name);
>
> Do we need to clear_mailmap before returning to avoid a leak?

Good question. What I queued yesterday seems to have a call to
clear_mailmap(&mailmap) before that return.

Re: [PATCH] commit: search author pattern against mailmap

[Antoine Pelisse]

On Sun, Aug 25, 2013 at 7:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>> Do we need to clear_mailmap before returning to avoid a leak?
>
> Good question. What I queued yesterday seems to have a call to
> clear_mailmap(&mailmap) before that return.

Indeed, the version you queued has clear_mailmap(), not the version
you sent by email.

Will resend.

Thanks,

[PATCH] commit: search author pattern against mailmap

[Antoine Pelisse]

"git commit --author=$name" sets the author to one whose name
matches the given string from existing commits, when $name is not in
the "Name <e-mail>" format. However, it does not honor the mailmap
to use the canonical name for the author found this way.

Fix it by telling the logic to find a matching existing author to
honor the mailmap, and use the name and email after applying the
mailmap.

Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
Hey,
So I kept clear_mailmap() where you put it, but I think it could be moved
right after get_revision(). That is because I think format_commit_message()
will run another read_mailmap() with an heap-allocated string_list.
Anyway, I'm not sure it makes a big difference here.

Thanks,
Antoine

 builtin/commit.c   |  8 +++++++-
 t/t4203-mailmap.sh | 11 +++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 10acc53..a48a7fe 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -30,6 +30,7 @@
 #include "column.h"
 #include "sequencer.h"
 #include "notes-utils.h"
+#include "mailmap.h"

 static const char * const builtin_commit_usage[] = {
 	N_("git commit [options] [--] <pathspec>..."),
@@ -935,6 +936,7 @@ static const char *find_author_by_nickname(const char *name)
 	struct rev_info revs;
 	struct commit *commit;
 	struct strbuf buf = STRBUF_INIT;
+	struct string_list mailmap = STRING_LIST_INIT_NODUP;
 	const char *av[20];
 	int ac = 0;

@@ -945,13 +947,17 @@ static const char *find_author_by_nickname(const char *name)
 	av[++ac] = buf.buf;
 	av[++ac] = NULL;
 	setup_revisions(ac, av, &revs, NULL);
+	revs.mailmap = &mailmap;
+	read_mailmap(revs.mailmap, NULL);
+
 	prepare_revision_walk(&revs);
 	commit = get_revision(&revs);
 	if (commit) {
 		struct pretty_print_context ctx = {0};
 		ctx.date_mode = DATE_NORMAL;
 		strbuf_release(&buf);
-		format_commit_message(commit, "%an <%ae>", &buf, &ctx);
+		format_commit_message(commit, "%aN <%aE>", &buf, &ctx);
+		clear_mailmap(&mailmap);
 		return strbuf_detach(&buf, NULL);
 	}
 	die(_("No existing author found with '%s'"), name);
diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index baa4685..4d715f0 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -470,4 +470,15 @@ test_expect_success 'Blame output (complex mapping)' '
 	test_cmp expect actual.fuzz
 '

+cat >expect <<\EOF
+Some Dude <some@dude.xx>
+EOF
+
+test_expect_success 'commit --author honors mailmap' '
+	test_must_fail git commit --author "nick" --allow-empty -meight &&
+	git commit --author "Some Dude" --allow-empty -meight &&
+	git show --pretty=format:"%an <%ae>%n" >actual &&
+	test_cmp expect actual
+'
+
 test_done
--
1.8.4.rc4.2.g8483dfa.dirty

Re: [PATCH] commit: search author pattern against mailmap

[Jeff King]

On Sun, Aug 25, 2013 at 12:01:29PM +0200, Antoine Pelisse wrote:

> So I kept clear_mailmap() where you put it, but I think it could be moved
> right after get_revision(). That is because I think format_commit_message()
> will run another read_mailmap() with an heap-allocated string_list.
> Anyway, I'm not sure it makes a big difference here.

Yeah, format_commit_message does not even pay attention to our
revs.mailmap.

It does make me wonder if there should simply be a static singleton
mailmap that gets loaded once per program invocation and then cleaned up
at exit. That is clearly what format_commit_message is doing. Is there
actually a use case for having a custom one in rev_info? It's not like
you can even control where it reads from when you call read_mailmap.

I guess we need it as a boolean "do we want to mailmap at all" for the
regular pretty formats, but it could just be a flag in rev_info instead
of a pointer.

-Peff

Re: [PATCH] commit: search author pattern against mailmap

[Antoine Pelisse]

On Sun, Aug 25, 2013 at 12:30 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Aug 25, 2013 at 12:01:29PM +0200, Antoine Pelisse wrote:
>
>> So I kept clear_mailmap() where you put it, but I think it could be moved
>> right after get_revision(). That is because I think format_commit_message()
>> will run another read_mailmap() with an heap-allocated string_list.
>> Anyway, I'm not sure it makes a big difference here.
>
> Yeah, format_commit_message does not even pay attention to our
> revs.mailmap.
>
> It does make me wonder if there should simply be a static singleton
> mailmap that gets loaded once per program invocation and then cleaned up
> at exit. That is clearly what format_commit_message is doing. Is there
> actually a use case for having a custom one in rev_info? It's not like
> you can even control where it reads from when you call read_mailmap.
>
> I guess we need it as a boolean "do we want to mailmap at all" for the
> regular pretty formats, but it could just be a flag in rev_info instead
> of a pointer.

So we would stop passing mailmap string_list along down to map_user(),
and the mailmap file (or blob) would be read the first time it's
needed, and stored in a static global variable in mailmap.c. I think
I'm OK with that because I don't think it would make sense to have
multiple instances of a mailmap string_list in the same git-command
instance.

Who would be responsible for deleting the string_list ? It would
either be done in each command, or done through a atexit(3) registered
function (but then, why would we even care about cleaning it up?).

Re: [PATCH] commit: search author pattern against mailmap

[Jeff King]

On Sun, Aug 25, 2013 at 03:37:24PM +0200, Antoine Pelisse wrote:

> So we would stop passing mailmap string_list along down to map_user(),
> and the mailmap file (or blob) would be read the first time it's
> needed, and stored in a static global variable in mailmap.c. I think
> I'm OK with that because I don't think it would make sense to have
> multiple instances of a mailmap string_list in the same git-command
> instance.

Exactly. Sample (largely untested) patch is below if you want to use it
as a starting point. There are probably a few additional cleanups on top
(e.g., "git log" understands "--mailmap", which should probably be
centralized to handle_revision_opt).

I'm on the fence. It doesn't actually save that many lines of code, and
I guess it's possible that somebody would want a custom mailmap in the
future. Even though you can't do it right now, all it would take is
exposing read_mailmap_file and read_mailmap_blob outside of mailmap.c.
Of course, it would be easy to expose map_user_from at the same time.

> Who would be responsible for deleting the string_list ? It would
> either be done in each command, or done through a atexit(3) registered
> function (but then, why would we even care about cleaning it up?).

Exactly. You would clean it up at exit, but the OS does it for you
already.

-Peff

---
 builtin/blame.c         |  7 +------
 builtin/check-mailmap.c | 12 ++++--------
 builtin/log.c           |  5 +----
 builtin/shortlog.c      |  7 ++-----
 commit.h                |  2 +-
 log-tree.c              |  2 +-
 mailmap.c               | 31 ++++++++++++++++++++++++++++---
 mailmap.h               |  7 +++----
 pretty.c                | 17 +++--------------
 revision.c              | 10 +++++-----
 revision.h              |  2 +-
 shortlog.h              |  2 --
 12 files changed, 50 insertions(+), 54 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 079dcd3..680adaf 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -48,8 +48,6 @@ static size_t blame_date_width;
 static enum date_mode blame_date_mode = DATE_ISO8601;
 static size_t blame_date_width;
 
-static struct string_list mailmap;
-
 #ifndef DEBUG
 #define DEBUG 0
 #endif
@@ -1394,8 +1392,7 @@ static void get_ac_line(const char *inbuf, const char *what,
 	/*
 	 * Now, convert both name and e-mail using mailmap
 	 */
-	map_user(&mailmap, &mailbuf, &maillen,
-		 &namebuf, &namelen);
+	map_user(&mailbuf, &maillen, &namebuf, &namelen);
 
 	strbuf_addf(mail, "<%.*s>", (int)maillen, mailbuf);
 	strbuf_add(name, namebuf, namelen);
@@ -2512,8 +2509,6 @@ parse_done:
 	sb.ent = ent;
 	sb.path = path;
 
-	read_mailmap(&mailmap, NULL);
-
 	if (!incremental)
 		setup_pager();
 
diff --git a/builtin/check-mailmap.c b/builtin/check-mailmap.c
index 8f4d809..b3a13f4 100644
--- a/builtin/check-mailmap.c
+++ b/builtin/check-mailmap.c
@@ -14,7 +14,7 @@ static const struct option check_mailmap_options[] = {
 	OPT_END()
 };
 
-static void check_mailmap(struct string_list *mailmap, const char *contact)
+static void check_mailmap(const char *contact)
 {
 	const char *name, *mail;
 	size_t namelen, maillen;
@@ -28,7 +28,7 @@ static void check_mailmap(struct string_list *mailmap, const char *contact)
 	mail = ident.mail_begin;
 	maillen = ident.mail_end - ident.mail_begin;
 
-	map_user(mailmap, &mail, &maillen, &name, &namelen);
+	map_user(&mail, &maillen, &name, &namelen);
 
 	if (namelen)
 		printf("%.*s ", (int)namelen, name);
@@ -38,7 +38,6 @@ int cmd_check_mailmap(int argc, const char **argv, const char *prefix)
 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,
@@ -46,21 +45,18 @@ int cmd_check_mailmap(int argc, const char **argv, const char *prefix)
 	if (argc == 0 && !use_stdin)
 		die(_("no contacts specified"));
 
-	read_mailmap(&mailmap, NULL);
-
 	for (i = 0; i < argc; ++i)
-		check_mailmap(&mailmap, argv[i]);
+		check_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);
+			check_mailmap(buf.buf);
 			maybe_flush_or_die(stdout, "stdout");
 		}
 		strbuf_release(&buf);
 	}
 
-	clear_mailmap(&mailmap);
 	return 0;
 }
diff --git a/builtin/log.c b/builtin/log.c
index 2625f98..88ebd85 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -168,10 +168,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 	if (source)
 		rev->show_source = 1;
 
-	if (mailmap) {
-		rev->mailmap = xcalloc(1, sizeof(struct string_list));
-		read_mailmap(rev->mailmap, NULL);
-	}
+	rev->use_mailmap = mailmap;
 
 	if (rev->pretty_given && rev->commit_format == CMIT_FMT_RAW) {
 		/*
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 1434f8f..22010b3 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -31,7 +31,7 @@ static void insert_one_record(struct shortlog *log,
 			      const char *author,
 			      const char *oneline)
 {
-	const char *dot3 = log->common_repo_prefix;
+	const char *dot3 = mailmap_repo_abbrev();
 	char *buffer, *p;
 	struct string_list_item *item;
 	const char *mailbuf, *namebuf;
@@ -49,7 +49,7 @@ static void insert_one_record(struct shortlog *log,
 	namelen = ident.name_end - ident.name_begin;
 	maillen = ident.mail_end - ident.mail_begin;
 
-	map_user(&log->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
+	map_user(&mailbuf, &maillen, &namebuf, &namelen);
 	strbuf_add(&namemailbuf, namebuf, namelen);
 
 	if (log->email)
@@ -209,8 +209,6 @@ void shortlog_init(struct shortlog *log)
 {
 	memset(log, 0, sizeof(*log));
 
-	read_mailmap(&log->mailmap, &log->common_repo_prefix);
-
 	log->list.strdup_strings = 1;
 	log->wrap = DEFAULT_WRAPLEN;
 	log->in1 = DEFAULT_INDENT1;
@@ -323,5 +321,4 @@ void shortlog_output(struct shortlog *log)
 	strbuf_release(&sb);
 	log->list.strdup_strings = 1;
 	string_list_clear(&log->list, 1);
-	clear_mailmap(&log->mailmap);
 }
diff --git a/commit.h b/commit.h
index d912a9d..d391a53 100644
--- a/commit.h
+++ b/commit.h
@@ -94,7 +94,7 @@ struct pretty_print_context {
 	char *notes_message;
 	struct reflog_walk_info *reflog_info;
 	const char *output_encoding;
-	struct string_list *mailmap;
+	int use_mailmap;
 	int color;
 	struct ident_split *from_ident;
 
diff --git a/log-tree.c b/log-tree.c
index a49d8e8..ed77e61 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -615,7 +615,7 @@ void show_log(struct rev_info *opt)
 	ctx.preserve_subject = opt->preserve_subject;
 	ctx.reflog_info = opt->reflog_info;
 	ctx.fmt = opt->commit_format;
-	ctx.mailmap = opt->mailmap;
+	ctx.use_mailmap = opt->use_mailmap;
 	ctx.color = opt->diffopt.use_color;
 	ctx.output_encoding = get_log_output_encoding();
 	if (opt->from_ident.mail_begin && opt->from_ident.name_begin)
diff --git a/mailmap.c b/mailmap.c
index 44614fc..05540fa 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -13,6 +13,9 @@ const char *git_mailmap_blob;
 
 const char *git_mailmap_file;
 const char *git_mailmap_blob;
+static struct string_list the_mailmap;
+static char *repo_abbrev;
+static int initialized;
 
 struct mailmap_info {
 	char *name;
@@ -28,6 +31,15 @@ struct mailmap_entry {
 	struct string_list namemap;
 };
 
+static void init_mailmap(void)
+{
+	if (initialized)
+		return;
+
+	read_mailmap(&the_mailmap, &repo_abbrev);
+	initialized = 1;
+}
+
 static void free_mailmap_info(void *p, const char *s)
 {
 	struct mailmap_info *mi = (struct mailmap_info *)p;
@@ -311,9 +323,9 @@ static struct string_list_item *lookup_prefix(struct string_list *map,
 	return NULL;
 }
 
-int map_user(struct string_list *map,
-	     const char **email, size_t *emaillen,
-	     const char **name, size_t *namelen)
+static int map_user_from(struct string_list *map,
+			 const char **email, size_t *emaillen,
+			 const char **name, size_t *namelen)
 {
 	struct string_list_item *item;
 	struct mailmap_entry *me;
@@ -359,3 +371,16 @@ int map_user(struct string_list *map,
 	debug_mm("map_user:  --\n");
 	return 0;
 }
+
+int map_user(const char **email, size_t *emaillen,
+	     const char **name, size_t *namelen)
+{
+	init_mailmap();
+	return map_user_from(&the_mailmap, email, emaillen, name, namelen);
+}
+
+const char *mailmap_repo_abbrev(void)
+{
+	init_mailmap();
+	return repo_abbrev;
+}
diff --git a/mailmap.h b/mailmap.h
index ed7c93b..de52e63 100644
--- a/mailmap.h
+++ b/mailmap.h
@@ -1,10 +1,9 @@ void clear_mailmap(struct string_list *map);
 #ifndef MAILMAP_H
 #define MAILMAP_H
 
-int read_mailmap(struct string_list *map, char **repo_abbrev);
-void clear_mailmap(struct string_list *map);
+int map_user(const char **email, size_t *emaillen,
+	     const char **name, size_t *namelen);
 
-int map_user(struct string_list *map,
-			 const char **email, size_t *emaillen, const char **name, size_t *namelen);
+const char *mailmap_repo_abbrev(void);
 
 #endif
diff --git a/pretty.c b/pretty.c
index 74563c9..94a9628 100644
--- a/pretty.c
+++ b/pretty.c
@@ -428,8 +428,8 @@ void pp_user_info(struct pretty_print_context *pp,
 	namebuf = ident.name_begin;
 	namelen = ident.name_end - ident.name_begin;
 
-	if (pp->mailmap)
-		map_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
+	if (pp->use_mailmap)
+		map_user(&mailbuf, &maillen, &namebuf, &namelen);
 
 	if (pp->fmt == CMIT_FMT_EMAIL) {
 		if (pp->from_ident) {
@@ -688,17 +688,6 @@ void logmsg_free(char *msg, const struct commit *commit)
 		free(msg);
 }
 
-static int mailmap_name(const char **email, size_t *email_len,
-			const char **name, size_t *name_len)
-{
-	static struct string_list *mail_map;
-	if (!mail_map) {
-		mail_map = xcalloc(1, sizeof(*mail_map));
-		read_mailmap(mail_map, NULL);
-	}
-	return mail_map->nr && map_user(mail_map, email, email_len, name, name_len);
-}
-
 static size_t format_person_part(struct strbuf *sb, char part,
 				 const char *msg, int len, enum date_mode dmode)
 {
@@ -717,7 +706,7 @@ static size_t format_person_part(struct strbuf *sb, char part,
 	maillen = s.mail_end - s.mail_begin;
 
 	if (part == 'N' || part == 'E') /* mailmap lookup */
-		mailmap_name(&mail, &maillen, &name, &namelen);
+		map_user(&mail, &maillen, &name, &namelen);
 	if (part == 'n' || part == 'N') {	/* name */
 		strbuf_add(sb, name, namelen);
 		return placeholder_len;
diff --git a/revision.c b/revision.c
index 84ccc05..5f96316 100644
--- a/revision.c
+++ b/revision.c
@@ -2661,7 +2661,7 @@ int rewrite_parents(struct rev_info *revs, struct commit *commit,
 	return 0;
 }
 
-static int commit_rewrite_person(struct strbuf *buf, const char *what, struct string_list *mailmap)
+static int commit_rewrite_person(struct strbuf *buf, const char *what)
 {
 	char *person, *endp;
 	size_t len, namelen, maillen;
@@ -2688,7 +2688,7 @@ static int commit_rewrite_person(struct strbuf *buf, const char *what, struct st
 	name = ident.name_begin;
 	namelen = ident.name_end - ident.name_begin;
 
-	if (map_user(mailmap, &mail, &maillen, &name, &namelen)) {
+	if (map_user(&mail, &maillen, &name, &namelen)) {
 		struct strbuf namemail = STRBUF_INIT;
 
 		strbuf_addf(&namemail, "%.*s <%.*s>",
@@ -2737,12 +2737,12 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
 	if (buf.len)
 		strbuf_addstr(&buf, message);
 
-	if (opt->grep_filter.header_list && opt->mailmap) {
+	if (opt->grep_filter.header_list && opt->use_mailmap) {
 		if (!buf.len)
 			strbuf_addstr(&buf, message);
 
-		commit_rewrite_person(&buf, "\nauthor ", opt->mailmap);
-		commit_rewrite_person(&buf, "\ncommitter ", opt->mailmap);
+		commit_rewrite_person(&buf, "\nauthor ");
+		commit_rewrite_person(&buf, "\ncommitter ");
 	}
 
 	/* Append "fake" message parts as needed */
diff --git a/revision.h b/revision.h
index 95859ba..a79817e 100644
--- a/revision.h
+++ b/revision.h
@@ -152,7 +152,7 @@ struct rev_info {
 	const char	*subject_prefix;
 	int		no_inline;
 	int		show_log_size;
-	struct string_list *mailmap;
+	int use_mailmap;
 
 	/* Filter by commit log message */
 	struct grep_opt	grep_filter;
diff --git a/shortlog.h b/shortlog.h
index de4f86f..e6c3055 100644
--- a/shortlog.h
+++ b/shortlog.h
@@ -14,9 +14,7 @@ struct shortlog {
 	int user_format;
 	int abbrev;
 
-	char *common_repo_prefix;
 	int email;
-	struct string_list mailmap;
 };
 
 void shortlog_init(struct shortlog *log);

Re: [PATCH] commit: search author pattern against mailmap

[Antoine Pelisse]

On Sun, Aug 25, 2013 at 6:51 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Aug 25, 2013 at 03:37:24PM +0200, Antoine Pelisse wrote:
>
>> So we would stop passing mailmap string_list along down to map_user(),
>> and the mailmap file (or blob) would be read the first time it's
>> needed, and stored in a static global variable in mailmap.c. I think
>> I'm OK with that because I don't think it would make sense to have
>> multiple instances of a mailmap string_list in the same git-command
>> instance.
>
> Exactly. Sample (largely untested) patch is below if you want to use it
> as a starting point. There are probably a few additional cleanups on top
> (e.g., "git log" understands "--mailmap", which should probably be
> centralized to handle_revision_opt).

I'm not exactly sure how I would improve the patch you sent. I
remember Junio was not willing to move --use-mailmap option to
revision options and wanted to keep it just for "log" (though I don't
have a reference to that email).

I've tested the patch against the test-suite and have given a thorough
read to it, and I think it's fine.

Would you mind sending it as a proper patch ? I have nothing to add,
and I'm terrible at writing commit messages :-/
Or maybe someone else's opinion would be nice. I'm still not convinced
this is even necessary.

Thanks !

Re: [PATCH] commit: search author pattern against mailmap

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Exactly. Sample (largely untested) patch is below if you want to use it
> as a starting point. There are probably a few additional cleanups on top
> (e.g., "git log" understands "--mailmap", which should probably be
> centralized to handle_revision_opt).
>
> I'm on the fence. It doesn't actually save that many lines of code, and
> I guess it's possible that somebody would want a custom mailmap in the
> future. Even though you can't do it right now, all it would take is
> exposing read_mailmap_file and read_mailmap_blob outside of mailmap.c.
> Of course, it would be easy to expose map_user_from at the same time.

I am of two minds on this, but if I were forced to pick one _today_,
I would have to say that I am moderately negative to the approach.

Having to always specify that you want to use mailmap and make sure
you read it is a bit cumbersome from callers' point of view, and
using a singleton global may be one attractive way to do so.

It however regresses the "you can choose which mailmap to apply"
structure we already have, it would make things less libifiable, and
will make it harder to allow a single Git process work on two or
more independent repositories (yes, we would need to restructure the
object API to allow us to manage multiple object stores, the ref
API, etc. in a way similar to how we weaned ourselves away from the
single "active_cache" abstraction in the index API). I am personally
OK to declare that we should _never_ touch more than one repository
in a single process, but submodule support already does this to some
extent, so...

I think it is a reasonable tentative solution to hook a singleton
instance to something that is commonly used, e.g. the rev_info
structure, for large subset of commands that do use the structure
chosen to host that singleton instance, but those that do not work
based on revision traversal (e.g. "grep") need to also honor mailmap
consistently, so we must keep the lower level API that takes an
explicit mailmap instance for them anyway.

So...

Re: [PATCH] commit: search author pattern against mailmap

[Jeff King]

On Sun, Aug 25, 2013 at 10:27:52PM -0700, Junio C Hamano wrote:

> > I'm on the fence. It doesn't actually save that many lines of code, and
> > I guess it's possible that somebody would want a custom mailmap in the
> > future. Even though you can't do it right now, all it would take is
> > exposing read_mailmap_file and read_mailmap_blob outside of mailmap.c.
> > Of course, it would be easy to expose map_user_from at the same time.
> 
> I am of two minds on this, but if I were forced to pick one _today_,
> I would have to say that I am moderately negative to the approach.
> 
> Having to always specify that you want to use mailmap and make sure
> you read it is a bit cumbersome from callers' point of view, and
> using a singleton global may be one attractive way to do so.

It is also slightly wasteful, in that we may parse and store the mailmap
multiple times. But I doubt it's a big deal.

> I think it is a reasonable tentative solution to hook a singleton
> instance to something that is commonly used, e.g. the rev_info
> structure, for large subset of commands that do use the structure
> chosen to host that singleton instance, but those that do not work
> based on revision traversal (e.g. "grep") need to also honor mailmap
> consistently, so we must keep the lower level API that takes an
> explicit mailmap instance for them anyway.

My patch kept the lower-level API (well, it de-publicized it because
nobody was using it, but we do not need to do that part).

But as I said, I am on the fence, and you do not seem enthused, so let's
just drop it.

-Peff