Re: [PATCH v3 07/11] rebase: do not attempt to remove startup_info->original_cwd

[Elijah Newren <newren@gmail.com>]

On Mon, Nov 29, 2021 at 9:50 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> 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.

> 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?