git aliases and GIT_PREFIX

git aliases and GIT_PREFIX

[Federico Kircheis]

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

If it is consistently set, it would make it much easier to notice that 
an alias is actually broken.



Best

Federico


PS: I'm not subscribed to the mailing list (yet), so please keep me in CC.

Re: git aliases and GIT_PREFIX

[Ævar Arnfjörð Bjarmason]

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

Re: git aliases and GIT_PREFIX

[Johannes Schindelin]

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

Re: git aliases and GIT_PREFIX

[Federico Kircheis]

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:
>
>> 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?).

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?

Re: git aliases and GIT_PREFIX

[Johannes Schindelin]

Hi Federico,

On Tue, 2 Nov 2021, Federico Kircheis wrote:

> On Tue, Nov 02, 2021 at 03:26:05PM +0100, Johannes Schindelin wrote:
> >
> >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?).
>
> 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

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