Re: [PATCH v2 2/9] setup: introduce startup_info->original_cwd

[Elijah Newren <newren@gmail.com>]

On Thu, Nov 25, 2021 at 2:55 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Thu, Nov 25 2021, Elijah Newren via GitGitGadget wrote:
>
> > Removing the current working directory causes all subsequent git
> > commands run from that directory to get confused and fail with a message
> > about being unable to read the current working directory:
> >
> >     $ git status
> >     fatal: Unable to read current working directory: No such file or directory
> >
> > Non-git commands likely have similar warnings or even errors, e.g.
> >
> >     $ bash -c 'echo hello'
> >     shell-init: error retrieving current directory: getcwd: cannot access parent directories: No such file or directory
> >     hello
>
> Okey, but...
>
> > diff --git a/git.c b/git.c
> > index 5ff21be21f3..2c98ab48936 100644
> > --- a/git.c
> > +++ b/git.c
> > @@ -866,6 +866,8 @@ int cmd_main(int argc, const char **argv)
> >
> >       trace_command_performance(argv);
> >
> > +     startup_info->original_cwd = xgetcwd();
> > +
> >       /*
> >        * "git-xxxx" is the same as "git xxxx", but we obviously:
> >        *
> > diff --git a/setup.c b/setup.c
> > index 347d7181ae9..f30657723ea 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -432,6 +432,54 @@ void setup_work_tree(void)
> >       initialized = 1;
> >  }
> >
> > +static void setup_original_cwd(void)
> > +{
> > +     struct strbuf tmp = STRBUF_INIT;
> > +     const char *worktree = NULL;
> > +     int offset = -1;
> > +
> > +     /*
> > +      * startup_info->original_cwd wass set early on in cmd_main(), unless
> > +      * we're an auxiliary tool like git-remote-http or test-tool.
> > +      */
> > +     if (!startup_info->original_cwd)
> > +             return;
>
> This really doesn't belong in those places, by calling xgetcwd() so
> early you'll be making commands that don't care about RUN_SETUP at all
> die. E.g. with this change:
>
>     mkdir x &&
>     cd x &&
>     rm -rf ../x &&
>     git version
>
> Will die.

Whoops!  Yeah, I should have used strbuf_getcwd() there.  Thanks for
the careful reading.

> So as a follow-up to my comment on this v2's 01/09 I think what's also
> missing here is something that does that, but instead of "git version"
> does it for all of the "while read builtin" list in t0012-help.sh.

If I had to do some kind of special casing, or if I did potentially
call a path that could die() in the final version, then yes I'd need
to test all of these.  But by using strbuf_getcwd(), I remove the
possibility of die()'ing, so I think testing one of these is good
enough.

> There's other cans of worms to be had here, the discussion downthread of
> the not-integrated[1] points to some of that.
>
> I.e. how should the various commands like "ls-remote" that can work
> with/without a repo behave here? That one happens to die before/after,
> but as noted in that thread that's also a bug.
>
> So anything that adds more really early dying in this area should also
> think about the greater goals of what we're doing with RUN_SETUP &
> related flags. I.e. does the direction make sense?
>
> If this is moved to some soft recording of the getcwd() (and maybe the
> $PWD, as in my WIP change?) shouldn't this go into common-main.c? With
> git.c's cmd_main() we're excluding the test helpers and things like
> git-daemon. Is that intentional?

I didn't think e.g. git-remote-http, git-daemon, etc. mattered.
test-tool is supposed to only be used in the testsuite to my
knowledge.  But fair point, I'll move it to common-main.c.  And, as
per Junio's suggestion in this thread, I'll make it only affect
worktrees.

> I'd also think we'd want to do this much earlier if e.g.  thing like the
> trace2 setup wanted to call the remove_directory() call.

Nah, I'll just put the original_cwd in a temporary, and then if setup
is never called, it won't affect anything else.  And setup will only
make it affect the worktree (and thus won't affect bare repos, nor
affect paths outside the worktree if the user is running from outside
the worktree).

> Per what Glen & mentioned I'm still not sure if I'm on board with that
> idea at all, just running with the ball you put in play :) I.e. if we're
> modifying all callers, let's make sure we catch all callers.
>
> Perhaps a better approach is to intercept chdir() instead? And anything
> that calls chdir() sets some GIT_* variable so we'll pass "here's our
> original cwd" down to sub-processes, like we do with "git -c" for
> config.
>
> That would presumably save you the effort of in-advance whitelisting
> everything like "git version", we can just move to doing this lazily. If
> you're a command that does a RUN_SETUP or otherwise needs chdir() we'll
> record the original getcwd(), otherwise...

There is a chdir_notify(), but of course it isn't called by the
codepath that handles the -C argument.  I believe the above solution
of a temporary handles things just fine, though.

Thanks for the thoughtful and careful review!