Re: [PATCH v3 07/11] rebase: do not attempt to remove startup_info->original_cwd
- From
Elijah Newren <newren@gmail.com>
- Date
- Nov 29, 2021, 19:22 UTC
- Message-ID
- <CABPp-BFr-U1kaqaj9HR+A=tRt0F_btcbDp89xeoZRdXewM4a2w@mail.gmail.com>
- In-Reply-To
- <7d90bf61-59c6-045e-1987-81d50454c260@gmail.com>
On Mon, Nov 29, 2021 at 9:50 AM Derrick Stolee <stolee@gmail.com> wrote:
Show 35 quoted lines
> > On 11/26/2021 5:40 PM, Elijah Newren via GitGitGadget wrote: > > 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 | 3 +++ > > t/t2501-cwd-empty.sh | 4 ++-- > > 2 files changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/sequencer.c b/sequencer.c > > index ea96837cde3..b71f7b8a0a6 100644 > > --- a/sequencer.c > > +++ b/sequencer.c > > @@ -4228,6 +4228,9 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts, > > > > cmd.git_cmd = 1; > > > > + if (startup_info->original_cwd && > > + !is_absolute_path(startup_info->original_cwd)) > > + cmd.dir = startup_info->original_cwd; > > I was initially confused by the "!is_absolute_path()" because > it seemed to me like it would be natural to store an absolute > path there, but I see this comment in patch 2: > > + * For convience, we would like to have the path relative to the > + * worktree instead of an absolute path. > > So it seems that we won't store it as an absolute path. Is > there any value in this condition, then?
Good catch. This is leftover from an earlier round; I'll remove it. From stash too.
Show 12 quoted lines
> This assignment of cmd.dir to the relative path has a lot > of baked-in knowledge of this variable _and_ the current > state (Git chdir()'d to the root of the worktree). If the > path is always relative, then it should be a BUG() if we > see an absolute path. Also, it seems like we would want > cmd.dir to be a concatenation of the worktree root and the > original_cwd. > > Or perhaps I'm being overly cautious and this could all be > resolved with a comment about the expected state of the > working directory and original_cwd. The tests will catch if > any of those expectations change.
Yeah, with the extra condition in there I should have added a good explanation about why I had it in there. But I think a simple
if (startup_info->original_cwd)
cmd.dir = startup_info->original_cwd;should be good enough? Or do you still want a comment for that?