On Thu, Oct 28 2021, Federico Kircheis wrote:
Hello to everyone,
today I reported what I believed to be a bug on
https://github.com/git-for-windows/git/issues/3496
and learned about GIT_DIR when working with aliases and git worktree.
It's annoying that GIT_DIR it is defined only if (as far as I've
understood) working from a worktrees or submodule, as it does not seem
to be related to those type of repositories.
This is also irritating because apparently working aliases breaks when
being executed from those repositories.
I believe it would be better if GIT_DIR it's either always set or
never (could someone enlighten me why the variable is needed in first
place?).
I don't know the full story, but a good place to start is to apply this
patch:
diff --git a/cache.h b/cache.h
index eba12487b99..84d4c8da167 100644
--- a/cache.h
+++ b/cache.h
@@ -486,7 +486,7 @@ static inline enum object_type object_type(unsigned int mode)
}
/* Double-check local_repo_env below if you add to this list. */
-#define GIT_DIR_ENVIRONMENT "GIT_DIR"
+#define GIT_DIR_ENVIRONMENT "POISON_GIT_DIR"
#define GIT_COMMON_DIR_ENVIRONMENT "GIT_COMMON_DIR"
#define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
#define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
If you then run the full test suite that comes with git you should get a
pretty good picture of why/how not having GIT_DIR breaks.
Surely some of those reasons are fixable, e.g. are we invoking our own
"git" with "GIT_DIR=<path> git [...]", then we can just use "git
--git-dir=<path>", some of the others might be tricky.
When I did that the failed tests were the following:
Test Summary Report
-------------------
t9401-git-cvsserver-crlf.sh (Wstat: 256 Tests: 18 Failed: 16)
Failed tests: 2-8, 10-18
Non-zero exit status: 1
t9400-git-cvsserver-server.sh (Wstat: 256 Tests: 45 Failed: 23)
Failed tests: 2, 20, 22-25, 27-41, 43, 45
Non-zero exit status: 1
t9200-git-cvsexportcommit.sh (Wstat: 256 Tests: 15 Failed: 13)
Failed tests: 1-2, 4-8, 10-15
Non-zero exit status: 1
t9402-git-cvsserver-refs.sh (Wstat: 256 Tests: 37 Failed: 25)
Failed tests: 7-12, 14-24, 27-29, 31-32, 34, 36-37
Non-zero exit status: 1
t5516-fetch-push.sh (Wstat: 256 Tests: 103 Failed: 3)
Failed tests: 101-103
Non-zero exit status: 1
t5500-fetch-pack.sh (Wstat: 256 Tests: 373 Failed: 6)
Failed tests: 7, 38-42
Non-zero exit status: 1
t7003-filter-branch.sh (Wstat: 256 Tests: 48 Failed: 10)
Failed tests: 6-7, 9-14, 16, 19
Non-zero exit status: 1
t7406-submodule-update.sh (Wstat: 256 Tests: 57 Failed: 1)
Failed test: 13
Non-zero exit status: 1
t9902-completion.sh (Wstat: 256 Tests: 212 Failed: 2)
Failed tests: 11-12
Non-zero exit status: 1
t5601-clone.sh (Wstat: 256 Tests: 107 Failed: 3)
Failed tests: 12-13, 35
Non-zero exit status: 1
t1510-repo-setup.sh (Wstat: 256 Tests: 109 Failed: 26)
Failed tests: 3-4, 6, 8-9, 14-17, 21, 23-24, 29-30, 55
57, 59-60, 67, 69-70, 73-74, 78, 80-81
Non-zero exit status: 1
t5310-pack-bitmaps.sh (Wstat: 256 Tests: 73 Failed: 1)
Failed test: 48
Non-zero exit status: 1
t5401-update-hooks.sh (Wstat: 256 Tests: 13 Failed: 9)
Failed tests: 3-10, 12
Non-zero exit status: 1
t5531-deep-submodule-push.sh (Wstat: 256 Tests: 27 Failed: 22)
Failed tests: 2-3, 5-15, 18-20, 22-27
Non-zero exit status: 1
t2400-worktree-add.sh (Wstat: 256 Tests: 71 Failed: 2)
Failed tests: 61-62
Non-zero exit status: 1
t3430-rebase-merges.sh (Wstat: 256 Tests: 25 Failed: 1)
Failed test: 11
Non-zero exit status: 1
t5801-remote-helpers.sh (Wstat: 256 Tests: 31 Failed: 28)
Failed tests: 2, 4-17, 19-31
Non-zero exit status: 1
t7401-submodule-summary.sh (Wstat: 256 Tests: 23 Failed: 1)
Failed test: 14
Non-zero exit status: 1
t0001-init.sh (Wstat: 256 Tests: 61 Failed: 3)
Failed tests: 10, 13, 36
Non-zero exit status: 1
t1500-rev-parse.sh (Wstat: 256 Tests: 75 Failed: 7)
Failed tests: 35-36, 45, 49-51, 59
Non-zero exit status: 1
t1501-work-tree.sh (Wstat: 256 Tests: 39 Failed: 19)
Failed tests: 4-7, 9-11, 13-15, 17-18, 24-26, 31, 33-34
39
Non-zero exit status: 1
t5509-fetch-push-namespaces.sh (Wstat: 256 Tests: 14 Failed: 1)
Failed test: 14
Non-zero exit status: 1
t4201-shortlog.sh (Wstat: 256 Tests: 25 Failed: 1)
Failed test: 9
Non-zero exit status: 1
t5519-push-alternates.sh (Wstat: 256 Tests: 8 Failed: 8)
Failed tests: 1-8
Non-zero exit status: 1
t6060-merge-index.sh (Wstat: 256 Tests: 7 Failed: 2)
Failed tests: 6-7
Non-zero exit status: 1
t7409-submodule-detached-work-tree.sh (Wstat: 256 Tests: 2 Failed: 2)
Failed tests: 1-2
Non-zero exit status: 1
t5403-post-checkout-hook.sh (Wstat: 256 Tests: 8 Failed: 1)
Failed test: 8
Non-zero exit status: 1
t2201-add-update-typechange.sh (Wstat: 256 Tests: 6 Failed: 2)
Failed tests: 5-6
Non-zero exit status: 1
t5402-post-merge-hook.sh (Wstat: 256 Tests: 6 Failed: 4)
Failed tests: 3-6
Non-zero exit status: 1
t1515-rev-parse-outside-repo.sh (Wstat: 256 Tests: 4 Failed: 1)
Failed test: 3
Non-zero exit status: 1
Files=940, Tests=24858, 144 wallclock secs ( 6.34 usr 1.64 sys + 654.49 cusr 307.66 csys = 970.13 CPU)
Result: FAIL
make: *** [Makefile:53: prove] Error 1
Hi Federico,
On Thu, 28 Oct 2021, Federico Kircheis wrote:
today I reported what I believed to be a bug on
https://github.com/git-for-windows/git/issues/3496
and learned about GIT_DIR when working with aliases and git worktree.
It's annoying that GIT_DIR it is defined only if (as far as I've understood)
working from a worktrees or submodule, as it does not seem to be related to
those type of repositories.
To clarify: `GIT_DIR` is set when executing an alias in a worktree other
than the primary one (and probably also in submodules), but not when
executing in a primary worktree.
This is also irritating because apparently working aliases breaks when being
executed from those repositories.
To clarify: an alias that wants to switch to a different repository and
execute Git commands there works well in a primary worktree. But when you
switch to a different repository while executing an alias from a secondary
worktree, it will fail because of `GIT_DIR` having been set.
I believe it would be better if GIT_DIR it's either always set or never
(could someone enlighten me why the variable is needed in first place?).
The fact that `GIT_DIR` is not set when calling an alias in a primary
worktree suggests that the behavior in secondary worktrees is not by
design. We should therefore be able to stop setting it there.
The question is: what code is responsible for setting it only in some
circumstances but not others?
Federico, do you have any experience in debugging C code? If so, it would
be good if you could take a crack at investigating this.
Ciao,
Johannes
Sorry for the late reply.
On Tue, Nov 02, 2021 at 03:26:05PM +0100, Johannes Schindelin wrote:
Hi Federico,
On Thu, 28 Oct 2021, Federico Kircheis wrote:
quoted
today I reported what I believed to be a bug on
https://github.com/git-for-windows/git/issues/3496
and learned about GIT_DIR when working with aliases and git worktree.
It's annoying that GIT_DIR it is defined only if (as far as I've understood)
working from a worktrees or submodule, as it does not seem to be related to
those type of repositories.
To clarify: `GIT_DIR` is set when executing an alias in a worktree other
than the primary one (and probably also in submodules), but not when
executing in a primary worktree.
quoted
This is also irritating because apparently working aliases breaks when being
executed from those repositories.
To clarify: an alias that wants to switch to a different repository and
execute Git commands there works well in a primary worktree. But when you
switch to a different repository while executing an alias from a secondary
worktree, it will fail because of `GIT_DIR` having been set.
quoted
I believe it would be better if GIT_DIR it's either always set or never
(could someone enlighten me why the variable is needed in first place?).
Yes, sorry if I was not clear, your clarification are what I meant to say.
The fact that `GIT_DIR` is not set when calling an alias in a primary
worktree suggests that the behavior in secondary worktrees is not by
design. We should therefore be able to stop setting it there.
The question is: what code is responsible for setting it only in some
circumstances but not others?
Federico, do you have any experience in debugging C code? If so, it would
be good if you could take a crack at investigating this.
Ciao,
Johannes
Yes, I have some experience, but never looked at the git codebase.
On GitHub (https://github.com/git-for-windows/git/issues/3496) there is already an hint where those variables are set:
https://github.com/git/git/blob/v2.33.1/git.c#L354
Or do you mean if I could investigate the test cases Ævar Arnfjörð Bjarmason mentioned?
Hi Federico,
On Tue, 2 Nov 2021, Federico Kircheis wrote:
On Tue, Nov 02, 2021 at 03:26:05PM +0100, Johannes Schindelin wrote:
quoted
On Thu, 28 Oct 2021, Federico Kircheis wrote:
quoted
today I reported what I believed to be a bug on
https://github.com/git-for-windows/git/issues/3496
and learned about GIT_DIR when working with aliases and git worktree.
It's annoying that GIT_DIR it is defined only if (as far as I've
understood)
working from a worktrees or submodule, as it does not seem to be related
to
those type of repositories.
To clarify: `GIT_DIR` is set when executing an alias in a worktree other
than the primary one (and probably also in submodules), but not when
executing in a primary worktree.
quoted
This is also irritating because apparently working aliases breaks when
being
executed from those repositories.
To clarify: an alias that wants to switch to a different repository and
execute Git commands there works well in a primary worktree. But when you
switch to a different repository while executing an alias from a secondary
worktree, it will fail because of `GIT_DIR` having been set.
quoted
I believe it would be better if GIT_DIR it's either always set or never
(could someone enlighten me why the variable is needed in first place?).
Yes, sorry if I was not clear, your clarification are what I meant to say.
quoted
The fact that `GIT_DIR` is not set when calling an alias in a primary
worktree suggests that the behavior in secondary worktrees is not by
design. We should therefore be able to stop setting it there.
The question is: what code is responsible for setting it only in some
circumstances but not others?
Federico, do you have any experience in debugging C code? If so, it would
be good if you could take a crack at investigating this.
Ciao,
Johannes
Yes, I have some experience, but never looked at the git codebase.
On GitHub (https://github.com/git-for-windows/git/issues/3496) there is
already an hint where those variables are set:
https://github.com/git/git/blob/v2.33.1/git.c#L354
Well, that's a call to `setup_git_directory_gently()`, and obviously we
already know a couple of scenarios where the `GIT_DIR` environment
variable is _not_ set in this function.
Unfortunately, this function is a bit long and calls other functions, too:
https://github.com/git/git/blob/v2.33.1/setup.c#L1206-L1339
I actually believe that the `GIT_DIR` is set (_when_ it is set) in
`set_git_dir_1()`:
https://github.com/git/git/blob/v2.33.1/environment.c#L332. This function
is called, unconditionally, in `set_git_dir()`, so my best bet is that the
`set_git_dir()` function calls in `setup_git_directory_gently()` are the
reason why `GIT_DIR` is set sometimes, but not always.
And it is further my suspicion that calls to `set_git_dir()` are used to
potentially turn a relative path into an absolute one, by passing a
non-zero value for `make_realpath`. And this might be the reason for the
calls in the first place, not the `GIT_DIR` variable.
So the first step would probably be to write a test case (see the scripts
in t/) that creates a secondary worktree, then calls an alias that
verifies that `GIT_DIR` is not set when run in that worktree. This test
case would be marked with `test_expect_failure`.
Then, you need to debug that test case (e.g. by using the `debug` function
to run a given Git command through `gdb` and then setting a breakpoint on
`setenv()`). Once you have the call path of the offending `setenv()`, you
might find an elegant way to avoid setting the environment variable.
On the other hand, you could also simply unset `GIT_DIR` and friends
explicitly, by adding something like this after
https://github.com/git/git/blob/v2.33.1/git.c#L364:
strvec_push(&child.env, "GIT_DIR");
But then, you might actually break things. The `GIT_PREFIX` variable is
required to let aliases know in which subdirectory (if any) they were
started. For example, if you run this command in Git's `Documentation/`
directory, it will actually print the path of the top-level directory:
git -c alias.pwd='!pwd' pwd
This is long-established behavior, and the only way to go back to the
directory from where the alias was started is to call `cd "$GIT_PREFIX"`.
That's behavior, therefore, that you cannot change.
And if `GIT_PREFIX` needs to be set, maybe there are scenarios where
`GIT_DIR` actually _does_ need to be set?
Hopefully these explanations make some sense to you.
Ciao,
Johannes