On Mon, Nov 29, 2021 at 9:50 AM Derrick Stolee <stolee@xxxxxxxxx> wrote:
>
> On 11/26/2021 5:40 PM, Elijah Newren via GitGitGadget wrote:
> > 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 | 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?
