On Thu, Jan 27, 2022 at 3:03 PM Elijah Newren <newren@xxxxxxxxx> wrote:
> On Wed, Jan 26, 2022 at 9:53 AM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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);
> }
