githooks: discuss Git operations in foreign repositories
PATCH 1/1 githooks: discuss Git operations in foreign repositories
From: Eric Sunshine <[email protected]>
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/[email protected]/
[2]: https://lore.kernel.org/git/[email protected]/
[3]: https://lore.kernel.org/git/[email protected]/
[4]: https://lore.kernel.org/git/[email protected]/
[5]: https://lore.kernel.org/git/[email protected]/
[6]: https://lore.kernel.org/git/[email protected]/
---
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(+)
Documentation/githooks.txt +11 -0
@@ -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
Signature · 1 line
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:
Quoted 4 lines
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?
Thank you,
Preston
Quoted 67 lines · Eric Sunshine via GitGitGadget
From: Eric Sunshine <[email protected]>
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_DIRandGIT_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/[email protected]/
[2]: https://lore.kernel.org/git/[email protected]/
[3]: https://lore.kernel.org/git/[email protected]/
[4]: https://lore.kernel.org/git/[email protected]/
[5]: https://lore.kernel.org/git/[email protected]/
[6]: https://lore.kernel.org/git/[email protected]/Signed-off-by Eric Sunshine <[email protected]>---
githooks: discuss Git operations in foreign repositoriesPublished-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/1457Documentation/githooks.txt | 11 +++++++++++ 1 file changed, 11 insertions(+)Documentation/githooks.txt +11
-0@@ -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 refersbase-commit:
a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1--
gitgitgadget
[administrivia: please reply inline rather than top-posting]
Quoted 8 lines · Preston Tunnell Wilson
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, couldgitdo 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.
Quoted 10 lines
2. Should we add something in the
git-worktreedocumentation? InDocumentation/git-worktree.txt, it mentions:Quoted 4 lines
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.
Quoted 8 lines
"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!
Quoted 5 lines
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!
Quoted 52 lines · Eric Sunshine
[administrivia: please reply inline rather than top-posting]
Quoted 8 lines · Preston Tunnell Wilson
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, couldgitdo 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.
Quoted 10 lines
2. Should we add something in the
git-worktreedocumentation? InDocumentation/git-worktree.txt, it mentions:Quoted 4 lines
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.
Quoted 19 lines · Preston Tunnell Wilson
Quoted 5 lines
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.
Quoted 12 lines
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.
I understand your concern, and can relate to the amount of effort it took to narrow down the problem. Nevertheless, even though you encountered this problem in relation to git-worktree, it's a more general issue which can manifest in other situations. As such, I can't think of a good way to discuss the issue in the git-worktree documentation that wouldn't feel out of place and make the documentation more confusing.
I'm not necessarily opposed to someone else giving it a shot if it can be done in a way which doesn't feel out of place and doesn't confuse git-worktree documentation further (especially for those new to the documentation). I just don't know how to do so myself.
Quoted 9 lines · Eric Sunshine via GitGitGadget
Documentation/githooks.txt +1
-0@@ -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
Documentation/githooks.txt Hooks can get their arguments via the environment, command-line · line 44
Quoted 10 lines
Documentation/githooks.txt +10
-0+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?
Documentation/githooks.txt | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
Documentation/githooks.txt +14 -1
@@ -25,7 +25,20 @@ Before Git invokes a hook, it changes its working directory to either $GIT_DIR in a bare repository or the root of the working tree in a non-bare 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. + +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) +------------ Hooks can get their arguments via the environment, command-line arguments, and stdin. See the documentation for each hook below for
Quoted 14 lines · Junio C Hamano
Documentation/githooks.txt Before Git invokes a hook, it changes its working directory to either · line 41
Quoted 10 lines · Eric Sunshine via GitGitGadget
Documentation/githooks.txt +10
-0+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
Quoted 1 line
* I think adding it one paragraph earlier may make it fit better.
That was my initial choice, as well, and is where I initially inserted it, but moved it down a paragraph at the last moment. I'm happy to move it back up again.
Documentation/githooks.txt Before Git invokes a hook, it changes its working directory to either · line 30
Quoted 11 lines
* 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
Documentation/githooks.txt +3
-1-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.
Yes, good idea, although I might phrase it something like:
Environment variables such as ... are exported so that Git commands run by the hook can correctly locate the repository.
Quoted 4 lines · Eric Sunshine
Yes, good idea, although I might phrase it something like:
Environment variables such as ... are exported so that Git commands run by the hook can correctly locate the repository.
Excellent. Thanks.
PATCH v2 1/1 githooks: discuss Git operations in foreign repositories
In reply to Eric Sunshine via GitGitGadgetFrom: Eric Sunshine <[email protected]>
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/[email protected]/
[2]: https://lore.kernel.org/git/[email protected]/
[3]: https://lore.kernel.org/git/[email protected]/
[4]: https://lore.kernel.org/git/[email protected]/
[5]: https://lore.kernel.org/git/[email protected]/
[6]: https://lore.kernel.org/git/[email protected]/
---
githooks: discuss Git operations in foreign repositories
This is a re-roll of [1]. It incorporates a refined version of Junio's
suggested improvement[2].
[1]
https://lore.kernel.org/git/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1457%2Fsunshineco%2Fhookenv-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1457/sunshineco/hookenv-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1457
Range-diff vs v1:
1: b9a2e23359a ! 1: d58694a4137 githooks: discuss Git operations in foreign repositories
@@ Commit message
## Documentation/githooks.txt ##
-@@ Documentation/githooks.txt: Hooks can get their arguments via the environment, command-line
- arguments, and stdin. See the documentation for each hook below for
- details.
+@@ Documentation/githooks.txt: 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.
-+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:
++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)
+------------
+
- 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
+ Hooks can get their arguments via the environment, command-line
+ arguments, and stdin. See the documentation for each hook below for
+ details.
Documentation/githooks.txt | 12 ++++++++++++ 1 file changed, 12 insertions(+)
Documentation/githooks.txt +12 -0
@@ -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) +------------ + Hooks can get their arguments via the environment, command-line arguments, and stdin. See the documentation for each hook below for details.
base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1
Signature · 1 line
gitgitgadget
Quoted 12 lines · Eric Sunshine via GitGitGadget
Documentation/githooks.txt | 12 ++++++++++++ 1 file changed, 12 insertions(+)Documentation/githooks.txt +1
-0@@ -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
Documentation/githooks.txt repository. An exception are hooks triggered during a push ('pre-receive', · line 40
Quoted 10 lines
Documentation/githooks.txt +10
-0+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) +------------
This looks good to me! Thank you! (And I'm sorry about top-posting *again*! I think I have the hang of it now.)
Quoted 9 lines · Eric Sunshine via GitGitGadget
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.
Quoted 6 lines
[1]: https://lore.kernel.org/git/[email protected]/
[2]: https://lore.kernel.org/git/[email protected]/
[3]: https://lore.kernel.org/git/[email protected]/
[4]: https://lore.kernel.org/git/[email protected]/
[5]: https://lore.kernel.org/git/[email protected]/
[6]: https://lore.kernel.org/git/[email protected]/
Boy, I'm like a broken record.
The patch here looks good to me. The problem is wider than just hooks, but it seems like that's going to be a common place for people to get caught by it. So certainly this is going in the right direction.
The other place I've run into it is writing a script meant to be run as an external command. E.g., running this:
git --git-dir=/some/path my-external-command
means that "my-external-command" is going to have $GIT_DIR set. If it wants to operate on another repository it needs to take care to clear that from the environment.
-Peff