Re: $GIT_DIR is no longer set when pre-commit hooks are called

[Jeff King <peff@peff.net>]

On Wed, Aug 22, 2018 at 04:16:00PM -0700, Gregory Oschwald wrote:

> As of the release of 2.18.0, $GIT_DIR is no longer set before calling
> pre-commit hooks. This change was introduced in "set_work_tree: use
> chdir_notify" (8500e0de) and is still present in master.
> 
> I reviewed the discussion when this change was initially submitted,
> and I don't think this behavior change was intentional.

It was sort-of intentional and sort-of not.

We never set intentionally GIT_DIR for hooks in the first place, but it
would sometimes end up set as a side effect of other operations. So some
hooks might see it and some might not. In the case of the pre-commit
hook, I think it was probably set consistently, since git-commit
requires a working tree, and setup_work_tree() used to set it as an
accidental side effect of its absolute/relative adjustments.

The "right" way to find the directory has always been "git rev-parse
--git-dir" (which will use GIT_DIR if set, and otherwise do the normal
discovery process).

However, I am sympathetic to the breakage of existing hooks. AFAICT that
while unintentional, we've probably been consistently setting $GIT_DIR
in hooks for commands with work-trees since 2008, as a side effect of
044bbbcb63 (Make git_dir a path relative to work_tree in
setup_work_tree(), 2008-06-19). Although I am slightly confused by this
earlier thread where the OP complained that the variable is set only
sometimes:

  https://public-inbox.org/git/CAEDDsWdXQ1+UukvbfRoTPzY3Y9sOaxQ7nh+qL_Mcuy3=XKKh7w@mail.gmail.com/

(and there the preferred behavior is actually _not_ to have it set,
because it's a gotcha when chdir-ing to another repo).

If we want to keep supporting this case, then I think we should be doing
it consistently, by setting $GIT_DIR in the child environment as we run
the hook. Something like the patch below, but ideally we'd apply it
consistently to all hooks (of course, that would break any hooks that
chdir to a new repo without resetting GIT_DIR, but such hooks are
already iffy, as they may already sometimes see GIT_DIR set in the
environment).

diff --git a/builtin/commit.c b/builtin/commit.c
index 3bfeabc463..3670024a25 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1440,6 +1440,7 @@ int run_commit_hook(int editor_is_used, const char *index_file, const char *name
 	int ret;
 
 	argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
+	argv_array_pushf(&hook_env, "GIT_DIR=%s", get_git_dir());
 
 	/*
 	 * Let the hook know that no editor will be launched.

-Peff