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

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.

I have several hooks that were broken like this, and from a quick Google search, using $GIT_DIR without setting a default does seem pretty common in hooks.

Greg

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

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).

Hi Peff,

On Sat, 25 Aug 2018, Jeff King wrote:

We did something similar in sequencer.c, and it required setting `GIT_WORK_TREE`, too, to avoid problems in worktrees. Do you need the same here, too?

Ciao, Dscho

On Mon, Aug 27, 2018 at 06:25:26PM +0200, Johannes Schindelin wrote:

Hmm, good point. Maybe. If we're just trying to restore the original behavior (i.e., fix a regression), then this would behave as the original.

I'm not at all opposed to going beyond that and actually fixing more cases. But I am a little nervous of introducing a new regression, given the subtlety around some of our startup code.

My current understanding is:

  - If we're setting GIT_DIR, it's probably always save to set
    GIT_WORK_TREE (or GIT_IMPLICIT_WORK_TREE=0 if there isn't a
    worktree). I.e., there is no case I know that would be broken by
    this.

  - Passing GIT_DIR to any sub-process operating in the context of our
    repo (i.e., excluding cases where we're clearing local_repo_env) can
    break a script that expects to do:

      cd /another/repo
      git ...

    and operate in /another/repo. But such a script is already broken at
    least in some cases, because running the initial command as:

      git --git-dir=/some/repo

    will mean that GIT_DIR is set. So a hook depending on that is
    already broken, though it may be small consolation to somebody whose
    hook happened to work most of the time, because they do not pass in
    GIT_DIR.

  - Any command that uses setup_work_tree() (which includes things with
    NEED_WORK_TREE in git.c) would have always been setting GIT_DIR up
    through v2.17.x. So any hooks they run would have seen it
    consistently, and therefore are exempt from being regressed in the
    case above.

So I _think_ it would be safe to:

  1. Set GIT_DIR again anytime we call setup_work_tree(), because that's
     what has always happened.

  2. Start setting GIT_WORK_TREE at the same time. This didn't happen
     before, but it can't break anybody, and might help.

That makes the rules for when GIT_DIR is set confusing, but at least it shouldn't regress. A more consistent rule of "hooks always see GIT_DIR" (or even "any sub-process always sees...") is easier to explain, but might break people in practice.

I notice also that sequencer.c sets an absolute path, since 09d7b6c6fa (sequencer: pass absolute GIT_DIR to exec commands, 2017-10-31). That does seem friendlier, though I wonder if could break any scripts. I cannot think of a case, unless the intermediate paths were no accessible to the uid running the process (but then, how would Git have generated the absolute path in the first place?).

-Peff

Hi Peff,

On Mon, 27 Aug 2018, Jeff King wrote:

I concur about the subtlety around the startup code. Quite a bit of that mess is now a bit easier to grok, thanks to my work to discover the GIT_DIR gently, but there are still monsters lurking in that code.

Having said that, I don't think that we can get away with setting GIT_DIR without GIT_WORK_TREE here. That would *definitely* introduce a bug, as far as I can tell.

With s/save/safe/, I agree.

Since this mailing list often indulges in tangent fest, I'll allow myself this one: `git --git-dir=...` should probably handle the case where GIT_DIR/GIT_WORK_TREE were set differently when the command was started, and if so, unset them.

I seem to remember two reports about such funny way to do things. So they exist, I agree.

With s/might help/will help in some edge cases/, I agree.

Indeed.

Please note that (IIRC) this commit just reinstated the handling in git-sh-setup, which always set GIT_DIR to an absolute path. Scripts used in conjunction with `git rebase -i` would therefore have had to expect this.

Ciao, Dscho