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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 25/01/2022 23:59, Elijah Newren wrote:
On Tue, Jan 25, 2022 at 12:27 PM Glen Choo <chooglen@xxxxxxxxxx> wrote:

"Elijah Newren via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:

From: Elijah Newren <newren@xxxxxxxxx>

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@xxxxxxxxx>
---
  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!

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

The patch hasn't come through to me on the mailing list yet, but it looks good. I thought we set both GIT_DIR and GIT_WORK_TREE when we were in a non-main worktree but obviously we don't. Eric do you happen to know if that is intentional?

As an aside I'm going to post an updated version of my series removing the call to 'git checkout' from sequencer.c later today but we definitely want to have this fix before that series gets merged.

Best Wishes

Phillip

---
  t/t3400-rebase.sh | 29 +++++++++++++++++++++++++++++
  1 file changed, 29 insertions(+)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 23dbd3c82e..8b8b66538b 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -416,4 +416,33 @@ test_expect_success MINGW,SYMLINKS_WINDOWS 'rebase when .git/logs is a symlink'
         mv actual_logs .git/logs
  '

+test_expect_success 'rebase when inside worktree subdirectory' '
+       git init main-wt &&
+       (
+               cd main-wt &&
+               git commit --allow-empty -m "initial" &&
+               # create commit with foo/bar/baz
+               mkdir -p foo/bar &&
+               touch foo/bar/baz &&
+               git add foo/bar/baz &&
+               git commit -m "add foo/bar/baz" &&
+               # create commit with a/b/c
+               mkdir -p a/b &&
+               touch a/b/c &&
+               git add a/b/c &&
+               git commit -m "add a/b/c" &&
+               # create another branch for our other worktree
+               git branch other &&
+               git worktree add ../other-wt other &&
+               (
+                       cd ../other-wt &&
+                       mkdir -p random/dir &&
+                       (
+                               cd random/dir &&
+                               git rebase --onto HEAD^^ HEAD^  # drops the HEAD^ commit
+                       )
+               )
+       )
+'
+
  test_done
--
2.35.0.rc0.227.g00780c9af4-goog




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux