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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 25, 2021, 10:44 UTC
Message-ID: <211125.86bl28jyil.gmgdl@evledraar.gmail.com>
In-Reply-To: <f6129a8ac9c3d052fb7fb508a62d4eedb8d9ed57.1637829556.git.gitgitgadget@gmail.com>

On Thu, Nov 25 2021, Elijah Newren via GitGitGadget wrote:

Show 12 quoted lines

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

Show 33 quoted lines

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

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.

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

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

1. https://lore.kernel.org/git/patch-1.1-fc26c46d39-20210722T140648Z-avarab@gmail.com/