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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 5, 2022, 11:42 UTC
Message-ID: <CAPig+cRCN=QQHupg1eOx9YvsLhgn+dDN6eYvQL6JhzknCa_3=w@mail.gmail.com>
In-Reply-To: <CAPig+cSi8_90=-Fvt_fq=VtOW_HzifNhrk1gaa6F1GrEonng+Q@mail.gmail.com>

On Sat, Feb 5, 2022 at 6:23 AM Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 17 quoted lines

> On Thu, Jan 27, 2022 at 3:03 PM Elijah Newren <newren@gmail.com> wrote:
> > 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
>
> 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.

I meant to ask here what was the nature of the various failures you were seeing without GIT_DIR being set, and whether you had considered tackling those failures with --git-dir= instead of GIT_DIR. If so, were the problems too difficult to overcome by --git-dir= alone? Fleshing out the commit message with such information might be worthwhile.

By the way, I also didn't mean to imply that the "difficult-to-reason-about GIT_DIR interaction in relation to subprograms" problem is new to this patch; it isn't. That problem has been around for ages (at least since 2007-08-01) but it wasn't as obvious since the setting of GIT_DIR was so far removed from other code which runs subprograms, thus readers likely wouldn't be thinking about GIT_DIR when reading the code which runs subprograms. This patch only makes the problem more obvious since readers now see the sequence:

    (1) set GIT_DIR
    (2) launch subprogram

So, a reader is more likely to wonder why GIT_DIR is needed for the subprogram and what possible wider side-effects it might have.