[PATCH 0/2] mailmap from blobs

[PATCH 0/2] mailmap from blobs

[Jeff King]

I noticed recently that the GitHub contributions page for git.git did
not seem very accurate. The problem is that while it uses shortlog, it
does not respect .mailmap, because we do not have a working tree from
which to read the .mailmap.

This series adds a config option analogous to mailmap.file, but which
reads from a blob in the repository (so the obvious thing to set it to
is "HEAD:.mailmap" in a bare repo, and probably "master:.mailmap" if you
frequently want to traverse while on unrelated branches). The obvious
alternative is to checkout a temporary file of .mailmap and point
mailmap.file at it, but this is a bit more convenient.

A config option is perhaps not the most flexible way to access this. For
example, one could in theory want to pull the mailmap from the tip of
the history being traversed (e.g., because you have multiple unrelated
DAGs in a single repo). But that could also produce the _wrong_ results,
if you are looking at the shortlog of older history (e.g., when doing
"git shortlog v1.5.0..v1.5.5", you would still want to be using the
modern mailmap from "master").

By making it a config option, the simple, common case does the right
thing, and people with complex cases can use "git -c mailmap.blob=..."
to feed the appropriate map for the history they are traversing. If
somebody wants to do something fancier (like --mailmap-from-tip or
something), it would be easy to build on top later.

  [1/2]: mailmap: refactor mailmap parsing for non-file sources
  [2/2]: mailmap: support reading mailmap from blobs

-Peff

[PATCH 1/2] mailmap: refactor mailmap parsing for non-file sources

[Jeff King]

The read_single_mailmap function opens a mailmap file and
parses each line. In preparation for having non-file
mailmaps, let's pull out the line-parsing logic into its own
function (read_mailmap_line), and rename the file-parsing
function to match (read_mailmap_file).

Signed-off-by: Jeff King <peff@peff.net>
---
Cleanup for the next patch. It's mostly indentation changes, so "diff
-w" is much easier to review.

 mailmap.c | 74 ++++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 40 insertions(+), 34 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index ea4b471..89bc318 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -129,44 +129,50 @@ static int read_single_mailmap(struct string_list *map, const char *filename, ch
 	return (*right == '\0' ? NULL : right);
 }
 
-static int read_single_mailmap(struct string_list *map, const char *filename, char **repo_abbrev)
+static void read_mailmap_line(struct string_list *map, char *buffer,
+			      char **repo_abbrev)
+{
+	char *name1 = NULL, *email1 = NULL, *name2 = NULL, *email2 = NULL;
+	if (buffer[0] == '#') {
+		static const char abbrev[] = "# repo-abbrev:";
+		int abblen = sizeof(abbrev) - 1;
+		int len = strlen(buffer);
+
+		if (!repo_abbrev)
+			return;
+
+		if (len && buffer[len - 1] == '\n')
+			buffer[--len] = 0;
+		if (!strncmp(buffer, abbrev, abblen)) {
+			char *cp;
+
+			if (repo_abbrev)
+				free(*repo_abbrev);
+			*repo_abbrev = xmalloc(len);
+
+			for (cp = buffer + abblen; isspace(*cp); cp++)
+				; /* nothing */
+			strcpy(*repo_abbrev, cp);
+		}
+		return;
+	}
+	if ((name2 = parse_name_and_email(buffer, &name1, &email1, 0)) != NULL)
+		parse_name_and_email(name2, &name2, &email2, 1);
+
+	if (email1)
+		add_mapping(map, name1, email1, name2, email2);
+}
+
+static int read_mailmap_file(struct string_list *map, const char *filename,
+			     char **repo_abbrev)
 {
 	char buffer[1024];
 	FILE *f = (filename == NULL ? NULL : fopen(filename, "r"));
 
 	if (f == NULL)
 		return 1;
-	while (fgets(buffer, sizeof(buffer), f) != NULL) {
-		char *name1 = NULL, *email1 = NULL, *name2 = NULL, *email2 = NULL;
-		if (buffer[0] == '#') {
-			static const char abbrev[] = "# repo-abbrev:";
-			int abblen = sizeof(abbrev) - 1;
-			int len = strlen(buffer);
-
-			if (!repo_abbrev)
-				continue;
-
-			if (len && buffer[len - 1] == '\n')
-				buffer[--len] = 0;
-			if (!strncmp(buffer, abbrev, abblen)) {
-				char *cp;
-
-				if (repo_abbrev)
-					free(*repo_abbrev);
-				*repo_abbrev = xmalloc(len);
-
-				for (cp = buffer + abblen; isspace(*cp); cp++)
-					; /* nothing */
-				strcpy(*repo_abbrev, cp);
-			}
-			continue;
-		}
-		if ((name2 = parse_name_and_email(buffer, &name1, &email1, 0)) != NULL)
-			parse_name_and_email(name2, &name2, &email2, 1);
-
-		if (email1)
-			add_mapping(map, name1, email1, name2, email2);
-	}
+	while (fgets(buffer, sizeof(buffer), f) != NULL)
+		read_mailmap_line(map, buffer, repo_abbrev);
 	fclose(f);
 	return 0;
 }
@@ -175,8 +181,8 @@ int read_mailmap(struct string_list *map, char **repo_abbrev)
 {
 	map->strdup_strings = 1;
 	/* each failure returns 1, so >1 means both calls failed */
-	return read_single_mailmap(map, ".mailmap", repo_abbrev) +
-	       read_single_mailmap(map, git_mailmap_file, repo_abbrev) > 1;
+	return read_mailmap_file(map, ".mailmap", repo_abbrev) +
+	       read_mailmap_file(map, git_mailmap_file, repo_abbrev) > 1;
 }
 
 void clear_mailmap(struct string_list *map)
-- 
1.8.0.2.4.g59402aa

[PATCH 2/2] mailmap: support reading mailmap from blobs

[Jeff King]

In a bare repository, there isn't a simple way to respect an
in-tree mailmap without extracting it to a temporary file.
This patch provides a config variable, similar to
mailmap.file, which reads the mailmap from a blob in the
repository.

Signed-off-by: Jeff King <peff@peff.net>
---
The error-return convention from read_mailmap is really wonky, but I
didn't change it here. It will return "1" for error, and will do so only
if no mailmap sources could be read (including if they simply don't
exist). But it's perfectly OK not to have a mailmap at all.  However,
nobody actually seems to check the return code, so nobody has cared.

A more sane convention would probably be:

  1. If ENOENT (or no such blob), silently return success.

  2. Otherwise, return -1 and print a message to stderr indicating that
     there is a mailmap file, but it is broken or otherwise could not be
     opened.

 Documentation/config.txt |  6 ++++
 cache.h                  |  1 +
 config.c                 |  2 ++
 mailmap.c                | 49 ++++++++++++++++++++++++++++++--
 t/t4203-mailmap.sh       | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 129 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index bf8f911..3760077 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1517,6 +1517,12 @@ mailmap.file::
 	subdirectory, or somewhere outside of the repository itself.
 	See linkgit:git-shortlog[1] and linkgit:git-blame[1].
 
+mailmap.blob::
+	Like `mailmap.file`, but consider the value as a reference to a
+	blob in the repository (e.g., `HEAD:.mailmap`). If both
+	`mailmap.file` and `mailmap.blob` are given, both are parsed,
+	with entries from `mailmap.file` taking precedence.
+
 man.viewer::
 	Specify the programs that may be used to display help in the
 	'man' format. See linkgit:git-help[1].
diff --git a/cache.h b/cache.h
index 18fdd18..a65f6d1 100644
--- a/cache.h
+++ b/cache.h
@@ -1155,6 +1155,7 @@ extern const char *git_mailmap_file;
 extern const char *git_commit_encoding;
 extern const char *git_log_output_encoding;
 extern const char *git_mailmap_file;
+extern const char *git_mailmap_blob;
 
 /* IO helper functions */
 extern void maybe_flush_or_die(FILE *, const char *);
diff --git a/config.c b/config.c
index fb3f868..97364c0 100644
--- a/config.c
+++ b/config.c
@@ -839,6 +839,8 @@ static int git_default_mailmap_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "mailmap.file"))
 		return git_config_string(&git_mailmap_file, var, value);
+	if (!strcmp(var, "mailmap.blob"))
+		return git_config_string(&git_mailmap_blob, var, value);
 
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
diff --git a/mailmap.c b/mailmap.c
index 89bc318..2f9c691 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -10,6 +10,7 @@ const char *git_mailmap_file;
 #endif
 
 const char *git_mailmap_file;
+const char *git_mailmap_blob;
 
 struct mailmap_info {
 	char *name;
@@ -177,12 +178,56 @@ int read_mailmap(struct string_list *map, char **repo_abbrev)
 	return 0;
 }
 
+static void read_mailmap_buf(struct string_list *map,
+			     const char *buf, unsigned long len,
+			     char **repo_abbrev)
+{
+	while (len) {
+		const char *end = strchrnul(buf, '\n');
+		unsigned long linelen = end - buf + 1;
+		char *line = xmemdupz(buf, linelen);
+
+		read_mailmap_line(map, line, repo_abbrev);
+
+		free(line);
+		buf += linelen;
+		len -= linelen;
+	}
+}
+
+static int read_mailmap_blob(struct string_list *map,
+			     const char *name,
+			     char **repo_abbrev)
+{
+	unsigned char sha1[20];
+	char *buf;
+	unsigned long size;
+	enum object_type type;
+
+	if (!name)
+		return 1;
+	if (get_sha1(name, sha1) < 0)
+		return 1;
+
+	buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return 1;
+	if (type != OBJ_BLOB)
+		return 1;
+
+	read_mailmap_buf(map, buf, size, repo_abbrev);
+
+	free(buf);
+	return 0;
+}
+
 int read_mailmap(struct string_list *map, char **repo_abbrev)
 {
 	map->strdup_strings = 1;
-	/* each failure returns 1, so >1 means both calls failed */
+	/* each failure returns 1, so >2 means all calls failed */
 	return read_mailmap_file(map, ".mailmap", repo_abbrev) +
-	       read_mailmap_file(map, git_mailmap_file, repo_abbrev) > 1;
+	       read_mailmap_blob(map, git_mailmap_blob, repo_abbrev) +
+	       read_mailmap_file(map, git_mailmap_file, repo_abbrev) > 2;
 }
 
 void clear_mailmap(struct string_list *map)
diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 1f182f6..e7ea40c 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -149,6 +149,79 @@ test_expect_success 'No mailmap files, but configured' '
 	test_cmp expect actual
 '
 
+test_expect_success 'setup mailmap blob tests' '
+	git checkout -b map &&
+	test_when_finished "git checkout master" &&
+	cat >just-bugs <<-\EOF &&
+	Blob Guy <bugs@company.xx>
+	EOF
+	cat >both <<-\EOF &&
+	Blob Guy <author@example.com>
+	Blob Guy <bugs@company.xx>
+	EOF
+	git add just-bugs both &&
+	git commit -m "my mailmaps" &&
+	echo "Repo Guy <author@example.com>" >.mailmap &&
+	echo "Internal Guy <author@example.com>" >internal.map
+'
+
+test_expect_success 'mailmap.blob set' '
+	cat >expect <<-\EOF &&
+	Blob Guy (1):
+	      second
+
+	Repo Guy (1):
+	      initial
+
+	EOF
+	git -c mailmap.blob=map:just-bugs shortlog HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'mailmap.blob overrides .mailmap' '
+	cat >expect <<-\EOF &&
+	Blob Guy (2):
+	      initial
+	      second
+
+	EOF
+	git -c mailmap.blob=map:both shortlog HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'mailmap.file overrides mailmap.blob' '
+	cat >expect <<-\EOF &&
+	Blob Guy (1):
+	      second
+
+	Internal Guy (1):
+	      initial
+
+	EOF
+	git \
+	  -c mailmap.blob=map:both \
+	  -c mailmap.file=internal.map \
+	  shortlog HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'mailmap.blob can be missing' '
+	cat >expect <<-\EOF &&
+	Repo Guy (1):
+	      initial
+
+	nick1 (1):
+	      second
+
+	EOF
+	git -c mailmap.blob=map:nonexistent shortlog HEAD >actual &&
+	test_cmp expect actual
+'
+
+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):
-- 
1.8.0.2.4.g59402aa

[PATCH 3/2] mailmap: clean up read_mailmap error handling

[Jeff King]

On Wed, Dec 12, 2012 at 06:04:04AM -0500, Jeff King wrote:

> The error-return convention from read_mailmap is really wonky, but I
> didn't change it here. It will return "1" for error, and will do so only
> if no mailmap sources could be read (including if they simply don't
> exist). But it's perfectly OK not to have a mailmap at all.  However,
> nobody actually seems to check the return code, so nobody has cared.
> 
> A more sane convention would probably be:
> 
>   1. If ENOENT (or no such blob), silently return success.
> 
>   2. Otherwise, return -1 and print a message to stderr indicating that
>      there is a mailmap file, but it is broken or otherwise could not be
>      opened.

Maybe like this:

-- >8 --
Subject: [PATCH] mailmap: clean up read_mailmap error handling

The error handling for the read_mailmap function is odd. It
returns 1 on error, rather than -1. And it treats a
non-existent mailmap as an error, even though there is no
reason that one needs to exist. Unless some other mailmap
source loads successfully, in which case the original error
is completely masked.

This does not cause any bugs, however, because no caller
bothers to check the return value, anyway. Let's make this a
little more robust to real errors and less surprising for
future callers that do check the error code:

  1. Return -1 on errors.

  2. Treat a missing entry (e.g., no mailmap.file given),
     ENOENT, or a non-existent blob (for mailmap.blob) as
     "no error".

  3. Complain loudly when a real error (e.g., a transient
     I/O error, no permission to open the mailmap file,
     missing or corrupted blob object, etc) occurs.

Signed-off-by: Jeff King <peff@peff.net>
---
 mailmap.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index 2f9c691..5ffe48a 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -168,10 +168,19 @@ static int read_mailmap_file(struct string_list *map, const char *filename,
 			     char **repo_abbrev)
 {
 	char buffer[1024];
-	FILE *f = (filename == NULL ? NULL : fopen(filename, "r"));
+	FILE *f;
+
+	if (!filename)
+		return 0;
+
+	f = fopen(filename, "r");
+	if (!f) {
+		if (errno == ENOENT)
+			return 0;
+		return error("unable to open mailmap at %s: %s",
+			     filename, strerror(errno));
+	}
 
-	if (f == NULL)
-		return 1;
 	while (fgets(buffer, sizeof(buffer), f) != NULL)
 		read_mailmap_line(map, buffer, repo_abbrev);
 	fclose(f);
@@ -205,15 +214,15 @@ static int read_mailmap_blob(struct string_list *map,
 	enum object_type type;
 
 	if (!name)
-		return 1;
+		return 0;
 	if (get_sha1(name, sha1) < 0)
-		return 1;
+		return 0;
 
 	buf = read_sha1_file(sha1, &type, &size);
 	if (!buf)
-		return 1;
+		return error("unable to read mailmap object at %s", name);
 	if (type != OBJ_BLOB)
-		return 1;
+		return error("mailmap is not a blob: %s", name);
 
 	read_mailmap_buf(map, buf, size, repo_abbrev);
 
@@ -223,11 +232,12 @@ int read_mailmap(struct string_list *map, char **repo_abbrev)
 
 int read_mailmap(struct string_list *map, char **repo_abbrev)
 {
+	int err = 0;
 	map->strdup_strings = 1;
-	/* each failure returns 1, so >2 means all calls failed */
-	return read_mailmap_file(map, ".mailmap", repo_abbrev) +
-	       read_mailmap_blob(map, git_mailmap_blob, repo_abbrev) +
-	       read_mailmap_file(map, git_mailmap_file, repo_abbrev) > 2;
+	err |= read_mailmap_file(map, ".mailmap", repo_abbrev);
+	err |= read_mailmap_blob(map, git_mailmap_blob, repo_abbrev);
+	err |= read_mailmap_file(map, git_mailmap_file, repo_abbrev);
+	return err;
 }
 
 void clear_mailmap(struct string_list *map)
-- 
1.8.0.2.4.g59402aa

Re: [PATCH 0/2] mailmap from blobs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I noticed recently that the GitHub contributions page for git.git did
> not seem very accurate. The problem is that while it uses shortlog, it
> does not respect .mailmap, because we do not have a working tree from
> which to read the .mailmap.
>
> This series adds a config option analogous to mailmap.file, but which
> reads from a blob in the repository (so the obvious thing to set it to
> is "HEAD:.mailmap" in a bare repo, and probably "master:.mailmap" if you
> frequently want to traverse while on unrelated branches). The obvious
> alternative is to checkout a temporary file of .mailmap and point
> mailmap.file at it, but this is a bit more convenient.

Yeah, I think this is sane.

Have you considered defaulting to read from HEAD:.mailmap even when
this new configuration is not there if core.bare is set?  I would
imagine that it would be the most convenient and match people's
expectations.

Re: [PATCH 0/2] mailmap from blobs

[Jeff King]

On Wed, Dec 12, 2012 at 09:54:23AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I noticed recently that the GitHub contributions page for git.git did
> > not seem very accurate. The problem is that while it uses shortlog, it
> > does not respect .mailmap, because we do not have a working tree from
> > which to read the .mailmap.
> >
> > This series adds a config option analogous to mailmap.file, but which
> > reads from a blob in the repository (so the obvious thing to set it to
> > is "HEAD:.mailmap" in a bare repo, and probably "master:.mailmap" if you
> > frequently want to traverse while on unrelated branches). The obvious
> > alternative is to checkout a temporary file of .mailmap and point
> > mailmap.file at it, but this is a bit more convenient.
> 
> Yeah, I think this is sane.
> 
> Have you considered defaulting to read from HEAD:.mailmap even when
> this new configuration is not there if core.bare is set?  I would
> imagine that it would be the most convenient and match people's
> expectations.

Yeah, I almost suggested that, but I figured it could wait for the
feature to prove itself in the real world before turning it on by
default. It _should_ be pretty harmless, though, so I don't mind turning
it on by default.

-Peff

Re: [PATCH 0/2] mailmap from blobs

[Jeff King]

On Wed, Dec 12, 2012 at 12:59:00PM -0500, Jeff King wrote:

> > Have you considered defaulting to read from HEAD:.mailmap even when
> > this new configuration is not there if core.bare is set?  I would
> > imagine that it would be the most convenient and match people's
> > expectations.
> 
> Yeah, I almost suggested that, but I figured it could wait for the
> feature to prove itself in the real world before turning it on by
> default. It _should_ be pretty harmless, though, so I don't mind turning
> it on by default.

That patch would look like this:

-- >8 --
Subject: [PATCH] mailmap: default mailmap.blob in bare repositories

The motivation for mailmap.blob is to let users of bare
repositories use the mailmap feature, as they would not have
a checkout containing the .mailmap file. We can make it even
easier for them by just looking in HEAD:.mailmap by default.

We can't know for sure that this is where they would keep a
mailmap, of course, but it is the best guess (and it matches
the non-bare behavior, which reads from HEAD:.mailmap in the
working tree). If it's missing, git will silently ignore the
setting.

We do not do the same magic in the non-bare case, because:

  1. In the common case, HEAD:.mailmap will be the same as
     the .mailmap in the working tree, which is a no-op.

  2. In the uncommon case, the user has modified .mailmap
     but not yet committed it, and would expect the working
     tree version to take precedence.

Signed-off-by: Jeff King <peff@peff.net>
---
I went with defaulting mailmap.blob, because it provides an easy path
for turning off the feature (you just override mailmap.blob).

Another way of thinking about this would be that it is the bare analog
to "read .mailmap from the working tree", and the logic should be:

  if (is_bare_repository())
          read_mailmap_blob(map, "HEAD:.mailmap");
  else
          read_mailmap_file(map, ".mailmap");
  read_mailmap_blob(map, git_mailmap_blob);
  read_mailmap_file(map, git_mailmap_file);

However, the current is not exactly "read from the root of the working
tree". It is "read from the current directory", and it works because all
of the callers run from the toplevel of the working tree (when one
exists). That means that bare repositories have always read from
$GIT_DIR/.mailmap. I doubt that was intentional, but people with bare
repositories may be depending on it.  So I think that falls into the
"how I might do it from scratch" bin.

We could still do:

  if (is_bare_repository())
          read_mailmap_blob(map, "HEAD:.mailmap");
  read_mailmap_file(map, ".mailmap");
  read_mailmap_blob(map, git_mailmap_blob);
  read_mailmap_file(map, git_mailmap_file);

but IMHO that is just making the rules more complicated to explain for
little benefit (and in fact, you lose the ability to suppress the HEAD
mailmap, which you might care about if you are hosting multiple bits of
unrelated history in the same repo).

 Documentation/config.txt |  8 +++++---
 mailmap.c                |  5 +++++
 t/t4203-mailmap.sh       | 25 +++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3760077..1a3c554 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1519,9 +1519,11 @@ mailmap.blob::
 
 mailmap.blob::
 	Like `mailmap.file`, but consider the value as a reference to a
-	blob in the repository (e.g., `HEAD:.mailmap`). If both
-	`mailmap.file` and `mailmap.blob` are given, both are parsed,
-	with entries from `mailmap.file` taking precedence.
+	blob in the repository. If both `mailmap.file` and
+	`mailmap.blob` are given, both are parsed, with entries from
+	`mailmap.file` taking precedence. In a bare repository, this
+	defaults to `HEAD:.mailmap`. In a non-bare repository, it
+	defaults to empty.
 
 man.viewer::
 	Specify the programs that may be used to display help in the
diff --git a/mailmap.c b/mailmap.c
index 5ffe48a..b16542f 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -233,7 +233,12 @@ int read_mailmap(struct string_list *map, char **repo_abbrev)
 int read_mailmap(struct string_list *map, char **repo_abbrev)
 {
 	int err = 0;
+
 	map->strdup_strings = 1;
+
+	if (!git_mailmap_blob && is_bare_repository())
+		git_mailmap_blob = "HEAD:.mailmap";
+
 	err |= read_mailmap_file(map, ".mailmap", repo_abbrev);
 	err |= read_mailmap_blob(map, git_mailmap_blob, repo_abbrev);
 	err |= read_mailmap_file(map, git_mailmap_file, repo_abbrev);
diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index e7ea40c..aae30d9 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -218,6 +218,31 @@ test_expect_success 'mailmap.blob can be missing' '
 	test_cmp expect actual
 '
 
+test_expect_success 'mailmap.blob defaults to off in non-bare repo' '
+	git init non-bare &&
+	(
+		cd non-bare &&
+		test_commit one .mailmap "Fake Name <author@example.com>" &&
+		echo "     1	Fake Name" >expect &&
+		git shortlog -ns HEAD >actual &&
+		test_cmp expect actual &&
+		rm .mailmap &&
+		echo "     1	A U Thor" >expect &&
+		git shortlog -ns HEAD >actual &&
+		test_cmp expect actual
+	)
+'
+
+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 &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'cleanup after mailmap.blob tests' '
 	rm -f .mailmap
 '
-- 
1.8.0.2.4.g59402aa

Re: [PATCH 2/2] mailmap: support reading mailmap from blobs

[Jeff King]

On Wed, Dec 12, 2012 at 06:04:04AM -0500, Jeff King wrote:

> In a bare repository, there isn't a simple way to respect an
> in-tree mailmap without extracting it to a temporary file.
> This patch provides a config variable, similar to
> mailmap.file, which reads the mailmap from a blob in the
> repository.

While preparing the "default mailmap.blob" patch, I noticed a few loose
ends in the documentation. They can be squashed into this 2/2
(jk/mailmap-from-blob~1), or can go on top of the series:

-- >8 --
Subject: [PATCH] fix some documentation loose-ends for mailmap.blob

Anywhere we mention mailmap.file, it is probably worth
mentioning mailmap.blob, as well.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-log.txt | 2 +-
 Documentation/mailmap.txt | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 585dac4..08a185d 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -167,7 +167,7 @@ log.showroot::
 	`git log -p` output would be shown without a diff attached.
 	The default is `true`.
 
-mailmap.file::
+mailmap.*::
 	See linkgit:git-shortlog[1].
 
 notes.displayRef::
diff --git a/Documentation/mailmap.txt b/Documentation/mailmap.txt
index 288f04e..bb349c2 100644
--- a/Documentation/mailmap.txt
+++ b/Documentation/mailmap.txt
@@ -1,5 +1,6 @@ If the file `.mailmap` exists at the toplevel of the repository, or at
 If the file `.mailmap` exists at the toplevel of the repository, or at
-the location pointed to by the mailmap.file configuration option, it
+the location pointed to by the mailmap.file or mailmap.blob
+configuration options, it
 is used to map author and committer names and email addresses to
 canonical real names and email addresses.
 
-- 
1.8.0.2.4.g59402aa

Re: [PATCH 0/2] mailmap from blobs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Subject: [PATCH] mailmap: default mailmap.blob in bare repositories
>
> The motivation for mailmap.blob is to let users of bare
> repositories use the mailmap feature, as they would not have
> a checkout containing the .mailmap file. We can make it even
> easier for them by just looking in HEAD:.mailmap by default.
>
> We can't know for sure that this is where they would keep a
> mailmap, of course, but it is the best guess (and it matches
> the non-bare behavior, which reads from HEAD:.mailmap in the
> working tree). If it's missing, git will silently ignore the
> setting.
>
> We do not do the same magic in the non-bare case, because:
>
>   1. In the common case, HEAD:.mailmap will be the same as
>      the .mailmap in the working tree, which is a no-op.
>
>   2. In the uncommon case, the user has modified .mailmap
>      but not yet committed it, and would expect the working
>      tree version to take precedence.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I went with defaulting mailmap.blob, because it provides an easy path
> for turning off the feature (you just override mailmap.blob).

Very sensibly explained.  I like it when people clearly explain the
thinking behind the change in the log message.

Thanks, will queue.


>  Documentation/config.txt |  8 +++++---
>  mailmap.c                |  5 +++++
>  t/t4203-mailmap.sh       | 25 +++++++++++++++++++++++++
>  3 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 3760077..1a3c554 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1519,9 +1519,11 @@ mailmap.blob::
>  
>  mailmap.blob::
>  	Like `mailmap.file`, but consider the value as a reference to a
> -	blob in the repository (e.g., `HEAD:.mailmap`). If both
> -	`mailmap.file` and `mailmap.blob` are given, both are parsed,
> -	with entries from `mailmap.file` taking precedence.
> +	blob in the repository. If both `mailmap.file` and
> +	`mailmap.blob` are given, both are parsed, with entries from
> +	`mailmap.file` taking precedence. In a bare repository, this
> +	defaults to `HEAD:.mailmap`. In a non-bare repository, it
> +	defaults to empty.
>  
>  man.viewer::
>  	Specify the programs that may be used to display help in the
> diff --git a/mailmap.c b/mailmap.c
> index 5ffe48a..b16542f 100644
> --- a/mailmap.c
> +++ b/mailmap.c
> @@ -233,7 +233,12 @@ int read_mailmap(struct string_list *map, char **repo_abbrev)
>  int read_mailmap(struct string_list *map, char **repo_abbrev)
>  {
>  	int err = 0;
> +
>  	map->strdup_strings = 1;
> +
> +	if (!git_mailmap_blob && is_bare_repository())
> +		git_mailmap_blob = "HEAD:.mailmap";
> +
>  	err |= read_mailmap_file(map, ".mailmap", repo_abbrev);
>  	err |= read_mailmap_blob(map, git_mailmap_blob, repo_abbrev);
>  	err |= read_mailmap_file(map, git_mailmap_file, repo_abbrev);
> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
> index e7ea40c..aae30d9 100755
> --- a/t/t4203-mailmap.sh
> +++ b/t/t4203-mailmap.sh
> @@ -218,6 +218,31 @@ test_expect_success 'mailmap.blob can be missing' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'mailmap.blob defaults to off in non-bare repo' '
> +	git init non-bare &&
> +	(
> +		cd non-bare &&
> +		test_commit one .mailmap "Fake Name <author@example.com>" &&
> +		echo "     1	Fake Name" >expect &&
> +		git shortlog -ns HEAD >actual &&
> +		test_cmp expect actual &&
> +		rm .mailmap &&
> +		echo "     1	A U Thor" >expect &&
> +		git shortlog -ns HEAD >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +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 &&
> +		test_cmp expect actual
> +	)
> +'
> +
>  test_expect_success 'cleanup after mailmap.blob tests' '
>  	rm -f .mailmap
>  '