Hi Junio, On Mon, 7 Feb 2022, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx> > writes: > > > From: Johannes Schindelin <johannes.schindelin@xxxxxx> > > > > 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@xxxxxx> > > --- > > 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); >
