Re: [Bug] Rebase from worktree subdir is broken (was Re: [PATCH v5 07/11] rebase: do not attempt to remove startup_info->original_cwd)

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Jan 27, 2022 at 3:03 PM Elijah Newren <newren@gmail.com> wrote:
> On Wed, Jan 26, 2022 at 9:53 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > As far as I know, there is no reason to set GIT_DIR and GIT_WORK_TREE,
> > in general, when in a linked worktree since each worktree has its own
> > .git file ("gitfile") which tells Git commands where the repository is
> > and signals that that directory itself (which contains the .git file)
> > is indeed a Git worktree.
>
> Oh, interesting.  Not setting GIT_DIR either does sound a bit better.
>
> ...though after digging for a while, it turns out to be a bit more
> involved than I thought.  Although the below patch passes our current
> testsuite and fixes the reported bug, I'm worried I've missed some cases
> not tested by the testsuite.
>
> Not sure if we want to pursue this, drop it, or something else.  Thoughts?

It's an enticing idea, though I have no deep knowledge about all the
possible interactions which may be impacted by such a change. Duy had
a deep understanding of how all this worked, and probably Peff, as
well, but they aren't around to offer opinions.

set_git_dir() has been setting the GIT_DIR environment variable ever
since it (the function) was introduced by d7ac12b25d (Add
set_git_dir() function, 2007-08-01). Unfortunately, the commit message
doesn't explain why it does so.

More below...

> -- >8 --
> Subject: [RFC/POC PATCH] setup: do not pre-emptively set GIT_DIR based on discovery
>
> Some comments on the various code changes:
>    * clone/push/fetch related:
>      * there are *many* subprocesses involved in fetch/push and friends,
>        and they typically need to know the GIT_DIR they are operating on
>      * this involves: fetch-patch.c, connected.c, bundle.c, clone.c,
>        transport-helper.c, receive-pack.c, upload-pack.c
>      * this accounts for the majority of this patch
>      * much of this work could be avoided by having enter_repo() call
>        xsetenv(GIT_DIR_ENVIRONMENT, ".", 1) just after its set_git_dir()
>        call, but I don't know if that'd be considered a half measure

It does feel a bit like a bandaid to insert new code at these
locations to set GIT_DIR manually. It's not clear to readers why
GIT_DIR is needed for these specific cases, nor what the impact is
when it is not set. Thus, one wonders if such a blanket approach is
indeed required or if a more narrow and directed fix can be applied,
such as calling subprograms with an explicit --git-dir= rather than
setting GIT_DIR with its potentially more broad and
difficult-to-reason-about impact.

> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> diff --git a/builtin/clone.c b/builtin/clone.c
> @@ -836,13 +836,18 @@ static void dissociate_from_references(void)
>  {
> +       struct strvec env = STRVEC_INIT;
> +       strvec_pushf(&env, GIT_DIR_ENVIRONMENT "=%s", the_repository->gitdir);

Minor inconsistency: all the other similar changes in this patch use
"%s=%s" and then pass in GIT_DIR_ENVIRONMENT to be interpolated by
`%s`.

> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> @@ -930,12 +930,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> -                               const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);
> +                               const char *gitdir = the_repository->gitdir;
>                                 if (arg[2] == 'g') {    /* --git-dir */
> -                                       if (gitdir) {
> +                                       if (strcmp(gitdir, ".git")) {
> @@ -945,9 +945,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> -                                       if (!gitdir && !prefix)
> -                                               gitdir = ".git";
> -                                       if (gitdir) {
> +                                       if (strcmp(gitdir, ".git") || !prefix) {

The meaning here becomes more obscure with this change applied. In the
original code, it was obvious enough that non-NULL `gitdir` meant that
the GIT_DIR environment variable had a value, but
`strcmp(gitdir,".git")` probably doesn't convey much to readers of
this code? Assigning the result of the strcmp() to a well-named
variable could go a long way toward making the meaning clearer. Or, an
in-code comment might be warranted.

> diff --git a/environment.c b/environment.c
> @@ -327,7 +327,6 @@ char *get_graft_file(struct repository *r)
>  static void set_git_dir_1(const char *path)
>  {
> -       xsetenv(GIT_DIR_ENVIRONMENT, path, 1);
>         setup_git_env(path);
>  }