{"thread":{"id":"49196","subject":"$GIT_DIR is no longer set when pre-commit hooks are called","startedAt":"2018-08-22T23:16:40Z","lastAt":"2018-08-28T12:50:20Z","messageCount":5,"participants":["Gregory Oschwald","Jeff King","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"356318","messageId":"CAFKzd1qaMU=K6uc62xL0DUyZOWxY79Qakaog2dzBjiNrmq0ydg@mail.gmail.com","threadId":"49196","inReplyTo":null,"subject":"$GIT_DIR is no longer set when pre-commit hooks are called","fromName":"Gregory Oschwald","fromEmail":"oschwald@gmail.com","sentAt":"2018-08-22T23:16:00Z","receivedAt":"2018-08-22T23:16:40Z","isPatch":false,"sender":{"key":"oschwald@gmail.com","avatar":null},"body":"As of the release of 2.18.0, $GIT_DIR is no longer set before calling\npre-commit hooks. This change was introduced in \"set_work_tree: use\nchdir_notify\" (8500e0de) and is still present in master.\n\nI reviewed the discussion when this change was initially submitted,\nand I don't think this behavior change was intentional.\n\nI have several hooks that were broken like this, and from a quick\nGoogle search, using $GIT_DIR without setting a default does seem\npretty common in hooks.\n\nGreg\n"},{"id":"356528","messageId":"20180826004150.GA31168@sigill.intra.peff.net","threadId":"49196","inReplyTo":"CAFKzd1qaMU=K6uc62xL0DUyZOWxY79Qakaog2dzBjiNrmq0ydg@mail.gmail.com","subject":"Re: $GIT_DIR is no longer set when pre-commit hooks are called","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-26T00:41:50Z","receivedAt":"2018-08-26T00:41:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 22, 2018 at 04:16:00PM -0700, Gregory Oschwald wrote:\n\n> As of the release of 2.18.0, $GIT_DIR is no longer set before calling\n> pre-commit hooks. This change was introduced in \"set_work_tree: use\n> chdir_notify\" (8500e0de) and is still present in master.\n> \n> I reviewed the discussion when this change was initially submitted,\n> and I don't think this behavior change was intentional.\n\nIt was sort-of intentional and sort-of not.\n\nWe never set intentionally GIT_DIR for hooks in the first place, but it\nwould sometimes end up set as a side effect of other operations. So some\nhooks might see it and some might not. In the case of the pre-commit\nhook, I think it was probably set consistently, since git-commit\nrequires a working tree, and setup_work_tree() used to set it as an\naccidental side effect of its absolute/relative adjustments.\n\nThe \"right\" way to find the directory has always been \"git rev-parse\n--git-dir\" (which will use GIT_DIR if set, and otherwise do the normal\ndiscovery process).\n\nHowever, I am sympathetic to the breakage of existing hooks. AFAICT that\nwhile unintentional, we've probably been consistently setting $GIT_DIR\nin hooks for commands with work-trees since 2008, as a side effect of\n044bbbcb63 (Make git_dir a path relative to work_tree in\nsetup_work_tree(), 2008-06-19). Although I am slightly confused by this\nearlier thread where the OP complained that the variable is set only\nsometimes:\n\n  https://public-inbox.org/git/CAEDDsWdXQ1+UukvbfRoTPzY3Y9sOaxQ7nh+qL_Mcuy3=XKKh7w@mail.gmail.com/\n\n(and there the preferred behavior is actually _not_ to have it set,\nbecause it's a gotcha when chdir-ing to another repo).\n\nIf we want to keep supporting this case, then I think we should be doing\nit consistently, by setting $GIT_DIR in the child environment as we run\nthe hook. Something like the patch below, but ideally we'd apply it\nconsistently to all hooks (of course, that would break any hooks that\nchdir to a new repo without resetting GIT_DIR, but such hooks are\nalready iffy, as they may already sometimes see GIT_DIR set in the\nenvironment).\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 3bfeabc463..3670024a25 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1440,6 +1440,7 @@ int run_commit_hook(int editor_is_used, const char *index_file, const char *name\n \tint ret;\n \n \targv_array_pushf(&hook_env, \"GIT_INDEX_FILE=%s\", index_file);\n+\targv_array_pushf(&hook_env, \"GIT_DIR=%s\", get_git_dir());\n \n \t/*\n \t * Let the hook know that no editor will be launched.\n\n-Peff\n"},{"id":"356589","messageId":"nycvar.QRO.7.76.6.1808271824270.73@tvgsbejvaqbjf.bet","threadId":"49196","inReplyTo":"20180826004150.GA31168@sigill.intra.peff.net","subject":"Re: $GIT_DIR is no longer set when pre-commit hooks are called","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-27T16:25:26Z","receivedAt":"2018-08-27T16:25:32Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Sat, 25 Aug 2018, Jeff King wrote:\n\n> On Wed, Aug 22, 2018 at 04:16:00PM -0700, Gregory Oschwald wrote:\n> \n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 3bfeabc463..3670024a25 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1440,6 +1440,7 @@ int run_commit_hook(int editor_is_used, const char *index_file, const char *name\n>  \tint ret;\n>  \n>  \targv_array_pushf(&hook_env, \"GIT_INDEX_FILE=%s\", index_file);\n> +\targv_array_pushf(&hook_env, \"GIT_DIR=%s\", get_git_dir());\n\nWe did something similar in sequencer.c, and it required setting\n`GIT_WORK_TREE`, too, to avoid problems in worktrees. Do you need the same\nhere, too?\n\nCiao,\nDscho\n\n>  \t/*\n>  \t * Let the hook know that no editor will be launched.\n> \n> -Peff\n> \n"},{"id":"356640","messageId":"20180827233706.GA11663@sigill.intra.peff.net","threadId":"49196","inReplyTo":"nycvar.QRO.7.76.6.1808271824270.73@tvgsbejvaqbjf.bet","subject":"Re: $GIT_DIR is no longer set when pre-commit hooks are called","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-27T23:37:06Z","receivedAt":"2018-08-27T23:37:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 27, 2018 at 06:25:26PM +0200, Johannes Schindelin wrote:\n\n> On Sat, 25 Aug 2018, Jeff King wrote:\n> \n> > On Wed, Aug 22, 2018 at 04:16:00PM -0700, Gregory Oschwald wrote:\n> > \n> > diff --git a/builtin/commit.c b/builtin/commit.c\n> > index 3bfeabc463..3670024a25 100644\n> > --- a/builtin/commit.c\n> > +++ b/builtin/commit.c\n> > @@ -1440,6 +1440,7 @@ int run_commit_hook(int editor_is_used, const char *index_file, const char *name\n> >  \tint ret;\n> >  \n> >  \targv_array_pushf(&hook_env, \"GIT_INDEX_FILE=%s\", index_file);\n> > +\targv_array_pushf(&hook_env, \"GIT_DIR=%s\", get_git_dir());\n> \n> We did something similar in sequencer.c, and it required setting\n> `GIT_WORK_TREE`, too, to avoid problems in worktrees. Do you need the same\n> here, too?\n\nHmm, good point. Maybe. If we're just trying to restore the original\nbehavior (i.e., fix a regression), then this would behave as the\noriginal.\n\nI'm not at all opposed to going beyond that and actually fixing more\ncases. But I am a little nervous of introducing a new regression, given\nthe subtlety around some of our startup code.\n\nMy current understanding is:\n\n  - If we're setting GIT_DIR, it's probably always save to set\n    GIT_WORK_TREE (or GIT_IMPLICIT_WORK_TREE=0 if there isn't a\n    worktree). I.e., there is no case I know that would be broken by\n    this.\n\n  - Passing GIT_DIR to any sub-process operating in the context of our\n    repo (i.e., excluding cases where we're clearing local_repo_env) can\n    break a script that expects to do:\n\n      cd /another/repo\n      git ...\n\n    and operate in /another/repo. But such a script is already broken at\n    least in some cases, because running the initial command as:\n\n      git --git-dir=/some/repo\n\n    will mean that GIT_DIR is set. So a hook depending on that is\n    already broken, though it may be small consolation to somebody whose\n    hook happened to work most of the time, because they do not pass in\n    GIT_DIR.\n\n  - Any command that uses setup_work_tree() (which includes things with\n    NEED_WORK_TREE in git.c) would have always been setting GIT_DIR up\n    through v2.17.x. So any hooks they run would have seen it\n    consistently, and therefore are exempt from being regressed in the\n    case above.\n\nSo I _think_ it would be safe to:\n\n  1. Set GIT_DIR again anytime we call setup_work_tree(), because that's\n     what has always happened.\n\n  2. Start setting GIT_WORK_TREE at the same time. This didn't happen\n     before, but it can't break anybody, and might help.\n\nThat makes the rules for when GIT_DIR is set confusing, but at least it\nshouldn't regress. A more consistent rule of \"hooks always see GIT_DIR\"\n(or even \"any sub-process always sees...\") is easier to explain, but\nmight break people in practice.\n\nI notice also that sequencer.c sets an absolute path, since 09d7b6c6fa\n(sequencer: pass absolute GIT_DIR to exec commands, 2017-10-31). That\ndoes seem friendlier, though I wonder if could break any scripts. I\ncannot think of a case, unless the intermediate paths were no accessible\nto the uid running the process (but then, how would Git have generated\nthe absolute path in the first place?).\n\n-Peff\n"},{"id":"356686","messageId":"nycvar.QRO.7.76.6.1808281442190.73@tvgsbejvaqbjf.bet","threadId":"49196","inReplyTo":"20180827233706.GA11663@sigill.intra.peff.net","subject":"Re: $GIT_DIR is no longer set when pre-commit hooks are called","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-28T12:50:09Z","receivedAt":"2018-08-28T12:50:20Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 27 Aug 2018, Jeff King wrote:\n\n> On Mon, Aug 27, 2018 at 06:25:26PM +0200, Johannes Schindelin wrote:\n> \n> > On Sat, 25 Aug 2018, Jeff King wrote:\n> > \n> > > On Wed, Aug 22, 2018 at 04:16:00PM -0700, Gregory Oschwald wrote:\n> > > \n> > > diff --git a/builtin/commit.c b/builtin/commit.c\n> > > index 3bfeabc463..3670024a25 100644\n> > > --- a/builtin/commit.c\n> > > +++ b/builtin/commit.c\n> > > @@ -1440,6 +1440,7 @@ int run_commit_hook(int editor_is_used, const char *index_file, const char *name\n> > >  \tint ret;\n> > >  \n> > >  \targv_array_pushf(&hook_env, \"GIT_INDEX_FILE=%s\", index_file);\n> > > +\targv_array_pushf(&hook_env, \"GIT_DIR=%s\", get_git_dir());\n> > \n> > We did something similar in sequencer.c, and it required setting\n> > `GIT_WORK_TREE`, too, to avoid problems in worktrees. Do you need the same\n> > here, too?\n> \n> Hmm, good point. Maybe. If we're just trying to restore the original\n> behavior (i.e., fix a regression), then this would behave as the\n> original.\n> \n> I'm not at all opposed to going beyond that and actually fixing more\n> cases. But I am a little nervous of introducing a new regression, given\n> the subtlety around some of our startup code.\n\nI concur about the subtlety around the startup code. Quite a bit of that\nmess is now a bit easier to grok, thanks to my work to discover the\nGIT_DIR gently, but there are still monsters lurking in that code.\n\nHaving said that, I don't think that we can get away with setting GIT_DIR\nwithout GIT_WORK_TREE here. That would *definitely* introduce a bug, as\nfar as I can tell.\n\n> My current understanding is:\n> \n>   - If we're setting GIT_DIR, it's probably always save to set\n\nWith s/save/safe/, I agree.\n\n>     GIT_WORK_TREE (or GIT_IMPLICIT_WORK_TREE=0 if there isn't a\n>     worktree). I.e., there is no case I know that would be broken by\n>     this.\n> \n>   - Passing GIT_DIR to any sub-process operating in the context of our\n>     repo (i.e., excluding cases where we're clearing local_repo_env) can\n>     break a script that expects to do:\n> \n>       cd /another/repo\n>       git ...\n> \n>     and operate in /another/repo. But such a script is already broken at\n>     least in some cases, because running the initial command as:\n> \n>       git --git-dir=/some/repo\n> \n>     will mean that GIT_DIR is set.\n\nSince this mailing list often indulges in tangent fest, I'll allow myself\nthis one: `git --git-dir=...` should probably handle the case where\nGIT_DIR/GIT_WORK_TREE were set differently when the command was started,\nand if so, unset them.\n\n>     already broken, though it may be small consolation to somebody whose\n>     hook happened to work most of the time, because they do not pass in\n>     GIT_DIR.\n\nI seem to remember two reports about such funny way to do things. So they\nexist, I agree.\n\n>   - Any command that uses setup_work_tree() (which includes things with\n>     NEED_WORK_TREE in git.c) would have always been setting GIT_DIR up\n>     through v2.17.x. So any hooks they run would have seen it\n>     consistently, and therefore are exempt from being regressed in the\n>     case above.\n> \n> So I _think_ it would be safe to:\n> \n>   1. Set GIT_DIR again anytime we call setup_work_tree(), because that's\n>      what has always happened.\n> \n>   2. Start setting GIT_WORK_TREE at the same time. This didn't happen\n>      before, but it can't break anybody, and might help.\n\nWith s/might help/will help in some edge cases/, I agree.\n\n> That makes the rules for when GIT_DIR is set confusing, but at least it\n> shouldn't regress. A more consistent rule of \"hooks always see GIT_DIR\"\n> (or even \"any sub-process always sees...\") is easier to explain, but\n> might break people in practice.\n\nIndeed.\n\n> I notice also that sequencer.c sets an absolute path, since 09d7b6c6fa\n> (sequencer: pass absolute GIT_DIR to exec commands, 2017-10-31). That\n> does seem friendlier, though I wonder if could break any scripts. I\n> cannot think of a case, unless the intermediate paths were no accessible\n> to the uid running the process (but then, how would Git have generated\n> the absolute path in the first place?).\n\nPlease note that (IIRC) this commit just reinstated the handling in\ngit-sh-setup, which always set GIT_DIR to an absolute path. Scripts used\nin conjunction with `git rebase -i` would therefore have had to expect\nthis.\n\nCiao,\nDscho\n"}]}