patchgithooks: discuss Git operations in foreign repositories

>
> From: Eric Sunshine <sunshine@sunshineco.com>
>
> Hook authors are periodically caught off-guard by difficult-to-diagnose
> errors when their hook invokes Git commands in a repository other than
> the local one. In particular, Git environment variables, such as GIT_DIR
> and GIT_WORK_TREE, which reference the local repository cause the Git
> commands to operate on the local repository rather than on the
> repository which the author intended. This is true whether the
> environment variables have been set manually by the user or
> automatically by Git itself. The same problem crops up when a hook
> invokes Git commands in a different worktree of the same repository, as
> well.
>
> Recommended best-practice[1,2,3,4,5,6] for avoiding this problem is for
> the hook to ensure that Git variables are unset before invoking Git
> commands in foreign repositories or other worktrees:
>
>     unset $(git rev-parse --local-env-vars)
>
> However, this advice is not documented anywhere. Rectify this
> shortcoming by mentioning it in githooks.txt documentation.
>
> [1]: https://lore.kernel.org/git/YFuHd1MMlJAvtdzb@coredump.intra.peff.net/
> [2]: https://lore.kernel.org/git/20200228190218.GC1408759@coredump.intra.peff.net/
> [3]: https://lore.kernel.org/git/20190516221702.GA11784@sigill.intra.peff.net/
> [4]: https://lore.kernel.org/git/20190422162127.GC9680@sigill.intra.peff.net/
> [5]: https://lore.kernel.org/git/20180716183942.GB22298@sigill.intra.peff.net/
> [6]: https://lore.kernel.org/git/20150203163235.GA9325@peff.net/
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>     githooks: discuss Git operations in foreign repositories
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1457%2Fsunshineco%2Fhookenv-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1457/sunshineco/hookenv-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1457
>
>  Documentation/githooks.txt | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index a16e62bc8c8..6e9a5420b7c 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -31,6 +31,17 @@ Hooks can get their arguments via the environment, command-line
>  arguments, and stdin. See the documentation for each hook below for
>  details.
>
> +If your hook needs to invoke Git commands in a foreign repository or in a
> +different working tree of the same repository, then it should clear local Git
> +environment variables, such as `GIT_DIR`, `GIT_WORK_TREE`, etc., which could
> +interfere with Git operations in the foreign repository since those variables
> +will be referencing the local repository and working tree. For example:
> +
> +------------
> +local_desc=$(git describe)
> +foreign_desc=$(unset $(git rev-parse --local-env-vars); git -C ../foreign-repo describe)
> +------------
> +
>  `git init` may copy hooks to the new repository, depending on its
>  configuration. See the "TEMPLATE DIRECTORY" section in
>  linkgit:git-init[1] for details. When the rest of this document refers
>
> base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1
> --
> gitgitgadget

> Thank you for this wonderful remedy, Eric! I really appreciate the
> background context and how you framed the problem that I ran into.
>
> I have two questions:
> 1. Documentation is a great first step in addressing this, but I'm
> wondering if this should be automatic? If this is a best practice for
> hook authors, could `git` do this for them automatically when running
> hooks?

> 2. Should we add something in the `git-worktree` documentation? In
> `Documentation/git-worktree.txt`, it mentions:
>
> > BUGS
> > ----
> > Multiple checkout in general is still experimental, and the support
> > for submodules is incomplete. ...
>
> Would it be helpful to plant a flag in the above documentation to
> point to this potential issue?

> "Best-practice" is context
> sensitive. It may be best-practice when a hook needs to invoke Git
> commands in some other repository (or worktree), but clearing those
> variables automatically would, in some situations, break the much more
> common case of the hook invoking Git commands in the local repository
> (or worktree). The fact that those environment variables may have been
> set manually by the user or automatically by Git further complicates
> the situation.

> So, no, I don't think this qualifies for the BUGS section of
> git-wortkree, and mentioning this potential gotcha only in
> git-worktree but not in any other hook-running command doesn't seem
> ideal either. At present, the best place to discuss it seems to be
> Documentation/githooks.txt, as this patch does.

>
> [administrivia: please reply inline rather than top-posting]
>
> On Sun, Jan 8, 2023 at 2:45 PM Preston Tunnell Wilson
> <prestontunnellwilson@gmail.com> wrote:
> > Thank you for this wonderful remedy, Eric! I really appreciate the
> > background context and how you framed the problem that I ran into.
> >
> > I have two questions:
> > 1. Documentation is a great first step in addressing this, but I'm
> > wondering if this should be automatic? If this is a best practice for
> > hook authors, could `git` do this for them automatically when running
> > hooks?
>
> For the general case, probably not. "Best-practice" is context
> sensitive. It may be best-practice when a hook needs to invoke Git
> commands in some other repository (or worktree), but clearing those
> variables automatically would, in some situations, break the much more
> common case of the hook invoking Git commands in the local repository
> (or worktree). The fact that those environment variables may have been
> set manually by the user or automatically by Git further complicates
> the situation.
>
> > 2. Should we add something in the `git-worktree` documentation? In
> > `Documentation/git-worktree.txt`, it mentions:
> >
> > > BUGS
> > > ----
> > > Multiple checkout in general is still experimental, and the support
> > > for submodules is incomplete. ...
> >
> > Would it be helpful to plant a flag in the above documentation to
> > point to this potential issue?
>
> As noted above, we can't really call this a bug. Git is behaving as
> intended. Whether the user set the variables manually or whether some
> parent Git process set them automatically, the child Git respects the
> variables as it should rather than second-guessing about the user's
> intentions, and possibly guessing incorrectly.
>
> So, no, I don't think this qualifies for the BUGS section of
> git-wortkree, and mentioning this potential gotcha only in
> git-worktree but not in any other hook-running command doesn't seem
> ideal either. At present, the best place to discuss it seems to be
> Documentation/githooks.txt, as this patch does. It may be possible to
> argue that gitfaq.txt could talk about it, but considering that this
> issue can manifest in many different ways (various error messages or
> misbehaviors), it's difficult to come up with any text for the "Q"
> which people would be likely to find when Googling. That's not to say
> it shouldn't be mentioned elsewhere in the documentation, but rather
> that I haven't come up with any better places than githooks.txt
> itself.

> > So, no, I don't think this qualifies for the BUGS section of
> > git-wortkree, and mentioning this potential gotcha only in
> > git-worktree but not in any other hook-running command doesn't seem
> > ideal either. At present, the best place to discuss it seems to be
> > Documentation/githooks.txt, as this patch does.
>
> I agree the best place to put it is in Documentation/githooks.txt. I
> also agree the BUGS section doesn't make sense, but I'm still
> wondering if we should call it out in git-worktree.txt in addition to
> githooks.txt. When I ran into this issue, I tried to compare my setup
> to that of my coworkers. The difference was that I was using
> git-worktree, they were not. git-worktree's documentation lists:
>
> Within a linked worktree, $GIT_DIR is set to point to this private
> directory (e.g. /path/main/.git/worktrees/test-next in the example)
> and $GIT_COMMON_DIR is set to point back to the main worktree’s
> $GIT_DIR (e.g. /path/main/.git). These settings are made in a .git
> file located at the top directory of the linked worktree.
>
> To me, this is the "other side of the coin" of your patch. (Or maybe
> one of the many other sides of the coin for commands that can run
> git-hooks.) Mentioning a potential collision between git-hooks and
> these variables being set could maybe go in the above snippet, maybe
> in parentheses. It took a lot of working backwards to narrow the issue
> to the interaction between git-worktree and git-hooks rather than the
> package manager I was using or the tool the hook was calling. Putting
> a note in the git-worktree documentation (in addition to the note in
> git-hooks) might help out someone in the future, but I defer to your
> judgement. If it doesn't make sense, doesn't fit, or adding it here
> would detract and make the documentation more confusing, I am happy to
> leave it out.

> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index a16e62bc8c8..6e9a5420b7c 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -31,6 +31,17 @@ Hooks can get their arguments via the environment, command-line
>  arguments, and stdin. See the documentation for each hook below for
>  details.
>  
> +If your hook needs to invoke Git commands in a foreign repository or in a
> +different working tree of the same repository, then it should clear local Git
> +environment variables, such as `GIT_DIR`, `GIT_WORK_TREE`, etc., which could
> +interfere with Git operations in the foreign repository since those variables
> +will be referencing the local repository and working tree. For example:
> +
> +------------
> +local_desc=$(git describe)
> +foreign_desc=$(unset $(git rev-parse --local-env-vars); git -C ../foreign-repo describe)
> +------------
> +

> "Eric Sunshine via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > +If your hook needs to invoke Git commands in a foreign repository or in a
> > +different working tree of the same repository, then it should clear local Git
> > +environment variables, such as `GIT_DIR`, `GIT_WORK_TREE`, etc., which could
> > +interfere with Git operations in the foreign repository since those variables
> > +will be referencing the local repository and working tree. For example:
> > +
> > +------------
> > +local_desc=$(git describe)
> > +foreign_desc=$(unset $(git rev-parse --local-env-vars); git -C ../foreign-repo describe)
> > +------------
>
> It is an excellent idea to add the above, but
>
>  * I think adding it one paragraph earlier may make it fit better.

>  * The paragraph, after which the above gets inserted, can use a bit
>    of enhancement.
>
> That is, something like this?
>
>  repository. An exception are hooks triggered during a push ('pre-receive',
>  'update', 'post-receive', 'post-update', 'push-to-checkout') which are always
> -executed in $GIT_DIR.
> +executed in $GIT_DIR.  Environment variables like GIT_DIR and GIT_WORK_TREE
> +are exported so that the hook can easily learn which repository it is
> +working with.

>  Documentation/githooks.txt | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index a16e62bc8c8..62908602e7b 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -27,6 +27,18 @@ repository. An exception are hooks triggered during a push ('pre-receive',
>  'update', 'post-receive', 'post-update', 'push-to-checkout') which are always
>  executed in $GIT_DIR.
>
> +Environment variables, such as `GIT_DIR`, `GIT_WORK_TREE`, etc., are exported
> +so that Git commands run by the hook can correctly locate the repository.  If
> +your hook needs to invoke Git commands in a foreign repository or in a
> +different working tree of the same repository, then it should clear these
> +environment variables so they do not interfere with Git operations at the
> +foreign location.  For example:
> +
> +------------
> +local_desc=$(git describe)
> +foreign_desc=$(unset $(git rev-parse --local-env-vars); git -C ../foreign-repo describe)
> +------------

> Recommended best-practice[1,2,3,4,5,6] for avoiding this problem is for
> the hook to ensure that Git variables are unset before invoking Git
> commands in foreign repositories or other worktrees:
> 
>     unset $(git rev-parse --local-env-vars)
> 
> However, this advice is not documented anywhere. Rectify this
> shortcoming by mentioning it in githooks.txt documentation.
> 
> [1]: https://lore.kernel.org/git/YFuHd1MMlJAvtdzb@coredump.intra.peff.net/
> [2]: https://lore.kernel.org/git/20200228190218.GC1408759@coredump.intra.peff.net/
> [3]: https://lore.kernel.org/git/20190516221702.GA11784@sigill.intra.peff.net/
> [4]: https://lore.kernel.org/git/20190422162127.GC9680@sigill.intra.peff.net/
> [5]: https://lore.kernel.org/git/20180716183942.GB22298@sigill.intra.peff.net/
> [6]: https://lore.kernel.org/git/20150203163235.GA9325@peff.net/