Re: [PATCH] githooks: discuss Git operations in foreign repositories

[Preston Tunnell Wilson <prestontunnellwilson@gmail.com>]

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

That makes sense, thank you for your answer!

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

And thank you for the administrivia!

On Sun, Jan 8, 2023 at 5:25 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> [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.