Re: [PATCH] Make git log work for git CWD outside of work tree

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 9, 2017, 10:54 UTC
Message-ID: <alpine.DEB.2.20.1704091238560.4268@virtualbox>
In-Reply-To: <20170409022128.21337-1-danny@dannysauer.com>

Hi Danny,

On Sat, 8 Apr 2017, Danny Sauer wrote:

> Make git log's `--use-mailmap` argument works if the GIT_DIR &
> GIT_WORK_TREE env vars are set and git is run from outside of work tree.
> Without the NEED_WORK_TREE set on the log subcommand, .mailmap is
> silently not found.

A laudable goal. How about adding a test case, say, to t/t4203-mailmap.sh, to ensure that Git won't regress on your fix?

Show 13 quoted lines

> diff --git a/git.c b/git.c
> index 8ff44f0..e147f01 100644
> --- a/git.c
> +++ b/git.c
> @@ -440,7 +440,7 @@ static struct cmd_struct commands[] = {
>  	{ "init", cmd_init_db },
>  	{ "init-db", cmd_init_db },
>  	{ "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY },
> -	{ "log", cmd_log, RUN_SETUP },
> +	{ "log", cmd_log, RUN_SETUP | NEED_WORK_TREE },
>  	{ "ls-files", cmd_ls_files, RUN_SETUP | SUPPORT_SUPER_PREFIX },
>  	{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
>  	{ "ls-tree", cmd_ls_tree, RUN_SETUP },

This may work for you, but it does not work for me, as I often call `git log` in a bare repository. And that call works, and it should keep working.

Instead, I think, you need to figure out why the .mailmap file is not read correctly when you use the GIT_DIR & GIT_WORK_TREE approach. My vague hunch is that you need to replace the ".mailmap" in read_mailmap() (defined in mailmap.c):

        err |= read_mailmap_file(map, ".mailmap", repo_abbrev);

by something like mkpath("%s/%s", get_git_work_tree(), ".mailmap"), but probably only after testing that we're not in a bare repository (which would also fix a bug, I suspect, as `git log` in a bare repository probably heeds a .mailmap file in the current directory, which is incorrect). I.e. something like:

	if (!is_bare_repository()) {
		const char *path = mkpath("%s/%s",
					  get_git_work_tree(), ".mailmap")
		err |= read_mailmap_file(map, path, repo_abbrev);
	}

But you really want to add the test case first, with `test_expect_failure`, to demonstrate what is currently broken, and then triumphantly setting it to `test_expect_success` with your patch.

Ciao, Johannes