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

[Elijah Newren <newren@gmail.com>]

On Tue, Jan 25, 2022 at 4:30 PM Glen Choo <chooglen@google.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > On Tue, Jan 25, 2022 at 12:27 PM Glen Choo <chooglen@google.com> wrote:
> >>
> >> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >>
> >> > From: Elijah Newren <newren@gmail.com>
> >> >
> >> > Since rebase spawns a `checkout` subprocess, make sure we run that from
> >> > the startup_info->original_cwd directory, so that the checkout process
> >> > knows to protect that directory.
> >> >
> >> > Signed-off-by: Elijah Newren <newren@gmail.com>
> >> > ---
> >> >  sequencer.c          | 2 ++
> >> >  t/t2501-cwd-empty.sh | 4 ++--
> >> >  2 files changed, 4 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/sequencer.c b/sequencer.c
> >> > index ea96837cde3..83f257e7fa4 100644
> >> > --- a/sequencer.c
> >> > +++ b/sequencer.c
> >> > @@ -4228,6 +4228,8 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts,
> >> >
> >> >       cmd.git_cmd = 1;
> >> >
> >> > +     if (startup_info->original_cwd)
> >> > +             cmd.dir = startup_info->original_cwd;
> >> >       strvec_push(&cmd.args, "checkout");
> >> >       strvec_push(&cmd.args, commit);
> >> >       strvec_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
> >> > diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh
> >> > index b1182390ba3..52335a8afe9 100755
> >> > --- a/t/t2501-cwd-empty.sh
> >> > +++ b/t/t2501-cwd-empty.sh
> >> > @@ -166,11 +166,11 @@ test_expect_success 'cherry-pick fails if cwd needs to be removed' '
> >> >  '
> >> >
> >> >  test_expect_success 'rebase does not clean cwd incidentally' '
> >> > -     test_incidental_dir_removal failure git rebase reverted
> >> > +     test_incidental_dir_removal success git rebase reverted
> >> >  '
> >> >
> >> >  test_expect_success 'rebase fails if cwd needs to be removed' '
> >> > -     test_required_dir_removal failure git rebase fd_conflict
> >> > +     test_required_dir_removal success git rebase fd_conflict
> >> >  '
> >> >
> >> >  test_expect_success 'revert does not clean cwd incidentally' '
> >> > --
> >> > gitgitgadget
> >>
> >> This commit (which is already in master) introduces a bug that breaks
> >> rebase when rebasing inside a subdirectory of a worktree. You can see
> >> that the below test fails with:
> >>
> >>   error: The following untracked working tree files would be overwritten by merge:
> >>           a/b/c
> >>   Please move or remove them before you merge.
> >
> > Thanks for the detailed report -- with a full testcase!
>
> Glad to be of help :)
>
> >> This only affects subdirectories in worktrees, i.e. rebasing anywhere in
> >> the `main-wt` directory is fine, and rebasing from the top of `other-wt`
> >> is fine, but `other-wt/any/other/dir` fails.
> >>
> >> I haven't tracked down the root cause yet, but judging from the commit,
> >> I would suppose that the checkout is being spawned in the wrong
> >> directory, causing the files to not be cleaned up.
> >
> > There's nothing wrong with running checkout from a subdirectory.  It
> > is unfortunate that setup.c auto-discovers both the git directory and
> > the working tree, but sets GIT_DIR without setting GIT_WORK_TREE in
> > the case of a non-main worktree; it's not particularly friendly for
> > subcommands.  Of course, it's also unfortunate that sequencer still
> > forks subprocesses other than those requested by a user with e.g.
> > --exec.
> >
> > But, anyway, I've got a patch that I'll send as soon as it passes CI
> > (https://github.com/git/git/pull/1205).
>
> I wish I had seen this email before diving into debugging it myself
> since your fix is more comprehensive, but it was a good learning
> experience nonetheless.

Yeah, sorry, it took me quite a bit of debugging as well before
figuring all this stuff out, so I don't think I sent the email in time
to save you the work.