threads / patch / 32319

patch, 2 partsmailmap from blobs

Subject: [PATCH 0/2] mailmap from blobs

## tl;dr

9 messages between Dec 12, 2012 and Dec 13, 2012. Diffs are folded; open one to read it.

replies: 8people: 2as markdown or json

Jeff King· Dec 12, 2012, 10:58 UTC · lore

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
Jeff King· Dec 12, 2012, 10:59 UTC · re: Jeff King · lore

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

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(-)
Show changes to mailmap.c +40 −34
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
Jeff King· Dec 12, 2012, 11:04 UTC · re: Jeff King · lore

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

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(-)
Show changes to 5 files +129 −2

Documentation/config.txt, cache.h, config.c, mailmap.c, t/t4203-mailmap.sh

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
Jeff King· Dec 12, 2012, 11:18 UTC · re: Jeff King · lore

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

On Wed, Dec 12, 2012 at 06:04:04AM -0500, Jeff King wrote:
Show 13 quoted lines
> 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(-)
Show changes to mailmap.c +21 −11
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
Jeff King· Dec 13, 2012, 13:08 UTC · re: Jeff King · lore

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

On Wed, Dec 12, 2012 at 06:04:04AM -0500, Jeff King wrote:
Show 5 quoted lines
> 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(-)
Show changes to 2 files +3 −2

Documentation/git-log.txt, Documentation/mailmap.txt

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
Junio C Hamano· Dec 12, 2012, 17:54 UTC · re: Jeff King · lore

Re: [PATCH 0/2] mailmap from blobs

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> 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.

Jeff King· Dec 12, 2012, 17:59 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] mailmap from blobs

On Wed, Dec 12, 2012 at 09:54:23AM -0800, Junio C Hamano wrote:
Show 20 quoted lines
> 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
Jeff King· Dec 13, 2012, 13:04 UTC · re: Jeff King · lore

Re: [PATCH 0/2] mailmap from blobs

On Wed, Dec 12, 2012 at 12:59:00PM -0500, Jeff King wrote:
Show 9 quoted lines
> > 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(-)
Show changes to 3 files +35 −3

Documentation/config.txt, mailmap.c, t/t4203-mailmap.sh

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
Junio C Hamano· Dec 13, 2012, 18:23 UTC · re: Jeff King · lore

Re: [PATCH 0/2] mailmap from blobs

Jeff King <peff@peff.net> writes:
Show 26 quoted lines
> 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.
Show 77 quoted lines
>  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
>  '

← back to recent threads