Re: [PATCH 1/2] checkout/fetch/pull/pack-objects: allow `-h` outside a repository

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Mon, 7 Feb 2022, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > When we taught these commands about the sparse index, we did not account
> > for the fact that the `cmd_*()` functions _can_ be called without a
> > gitdir, namely when `-h` is passed to show the usage.
> >
> > A plausible approach to address this is to move the
> > `prepare_repo_settings()` calls right after the `parse_options()` calls:
> > The latter will never return when it handles `-h`, and therefore it is
> > safe to assume that we have a `gitdir` at that point, as long as the
> > built-in is marked with the `RUN_SETUP` flag.
> >
> > However, it is unfortunately not that simple. In `cmd_pack_objects()`,
> > for example, the repo settings need to be fully populated so that the
> > command-line options `--sparse`/`--no-sparse` can override them, not the
> > other way round.
> >
> > Therefore, we choose to imitate the strategy taken in `cmd_diff()`,
> > where we simply do not bother to prepare and initialize the repo
> > settings unless we have a `gitdir`.
> >
> > This fixes https://github.com/git-for-windows/git/issues/3688
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
>
> These "prepare" calls are always tricky.  This somehow reminds me of
> 44c7e62e (repo-settings: prepare_repo_settings only in git repos,
> 2021-12-06) and e5b17bda (git: ensure correct git directory setup
> with -h, 2021-12-06), which were part of the ld/sparse-diff-blame
> topic that was merged at 8d2c3732 during the last cycle.

Yes, and I did indeed imitate the way Lessley changed `cmd_diff()`. It
turned out to be much smarter than the first approach I tried (and that I
described as a "plausible approach" in the commit message).

> > diff --git a/builtin/checkout.c b/builtin/checkout.c
> > index cc804ba8e1e..1c13d7fedb3 100644
> > --- a/builtin/checkout.c
> > +++ b/builtin/checkout.c
> > @@ -1602,9 +1602,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
> >  	opts->show_progress = -1;
> >
> >  	git_config(git_checkout_config, opts);
> > -
> > -	prepare_repo_settings(the_repository);
> > -	the_repository->settings.command_requires_full_index = 0;
> > +	if (the_repository->gitdir) {
> > +		prepare_repo_settings(the_repository);
> > +		the_repository->settings.command_requires_full_index = 0;
> > +	}
>
> Looks quite straight-forward and sensible.  Of course, the checkout
> command will go berserk and do nonsense if it is allowed to proceed
> much further from here without .gitdir, but we know that
>
>  (1) the only case where .gitdir is NULL at this point is when
>      running "-h" from outside a repository (we would have done a
>      full RUN_SETUP without demoting it to RUN_SETUP_GENTLY if we
>      weren't doing "-h"), and that
>
>  (2) this is soon followed by parse_options() where it notices "-h"
>      and stops.
>
> so skipping "prepare" outside a repository is totally safe.

Indeed.

Plus, do not forget how the second patch in this series ensures that there
are no other calls that would get the hiccups when `gitdir` is `NULL`.

> > diff --git a/builtin/fetch.c b/builtin/fetch.c
> > index 5f06b21f8e9..e0a05de10ee 100644
> > --- a/builtin/fetch.c
> > +++ b/builtin/fetch.c
> > @@ -2014,11 +2014,14 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> >  	}
> >
> >  	git_config(git_fetch_config, NULL);
> > -	prepare_repo_settings(the_repository);
> > -	the_repository->settings.command_requires_full_index = 0;
> > +	if (the_repository->gitdir) {
> > +		prepare_repo_settings(the_repository);
> > +		the_repository->settings.command_requires_full_index = 0;
> > +	}
>
> Ditto.
>
> >  	argc = parse_options(argc, argv, prefix,
> >  			     builtin_fetch_options, builtin_fetch_usage, 0);
> > +
> >  	if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
>
> Unrelated blank line?

Whoops, yes. I did mention that I first tried to move the call, right?
That's a left-over from not _completely_ cleaning up after that failed
attempt.

>
> >  		int *sfjc = submodule_fetch_jobs_config == -1
> >  			    ? &submodule_fetch_jobs_config : NULL;
> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > index ba2006f2212..87cb7b45c37 100644
> > --- a/builtin/pack-objects.c
> > +++ b/builtin/pack-objects.c
> > @@ -3976,9 +3976,11 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
> >  	read_replace_refs = 0;
> >
> >  	sparse = git_env_bool("GIT_TEST_PACK_SPARSE", -1);
> > -	prepare_repo_settings(the_repository);
> > -	if (sparse < 0)
> > -		sparse = the_repository->settings.pack_use_sparse;
> > +	if (the_repository->gitdir) {
> > +		prepare_repo_settings(the_repository);
> > +		if (sparse < 0)
> > +			sparse = the_repository->settings.pack_use_sparse;
> > +	}
> >
> >  	reset_pack_idx_option(&pack_idx_opts);
> >  	git_config(git_pack_config, NULL);
>
> This has a bit more sequence of calls until it reaches parse_options(),
> but their side effect, when run outside a repository, are all benign
> in-core effects, so this is safe, too.

And once again, I would like to point out that the regression test is
crucial to keep this benign.

I'll send out v2 shortly.

Ciao,
Dscho

>
> > diff --git a/builtin/pull.c b/builtin/pull.c
> > index 100cbf9fb85..d15007d93f3 100644
> > --- a/builtin/pull.c
> > +++ b/builtin/pull.c
> > @@ -994,8 +994,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> >  		set_reflog_message(argc, argv);
> >
> >  	git_config(git_pull_config, NULL);
> > -	prepare_repo_settings(the_repository);
> > -	the_repository->settings.command_requires_full_index = 0;
> > +	if (the_repository->gitdir) {
> > +		prepare_repo_settings(the_repository);
> > +		the_repository->settings.command_requires_full_index = 0;
> > +	}
> >
> >  	argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
>