From: Lessley Dennington Date: Wed, 24 Nov 2021 14:41:26 GMT Subject: Re: [PATCH v4 1/4] sparse index: enable only for git repos Message-ID: <724abbd4-b9ee-3b3d-226c-b7999f138152@gmail.com> In-Reply-To: On 11/23/21 3:39 PM, Junio C Hamano wrote: > "Lessley Dennington via GitGitGadget" > writes: > >> From: Lessley Dennington >> >> Check whether git dir exists before adding any repo settings. If it >> does not exist, BUG with the message that one cannot add settings for an >> uninitialized repository. If it does exist, proceed with adding repo >> settings. >> >> Additionally, ensure the above BUG is not triggered when users pass the -h >> flag by adding a check for the repository to the checkout and pack-objects >> builtins. >> >> Finally, ensure the above BUG is not triggered for commit-graph by >> returning early if the git directory does not exist. >> >> Signed-off-by: Lessley Dennington >> --- >> builtin/checkout.c | 6 ++++-- >> builtin/pack-objects.c | 9 ++++++--- >> commit-graph.c | 5 ++++- >> repo-settings.c | 3 +++ >> 4 files changed, 17 insertions(+), 6 deletions(-) >> >> diff --git a/builtin/checkout.c b/builtin/checkout.c >> index 8c69dcdf72a..31632b07888 100644 >> --- a/builtin/checkout.c >> +++ b/builtin/checkout.c >> @@ -1588,8 +1588,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix, >> >> git_config(git_checkout_config, opts); >> >> - prepare_repo_settings(the_repository); >> - the_repository->settings.command_requires_full_index = 0; >> + if (startup_info->have_repository) { >> + prepare_repo_settings(the_repository); >> + the_repository->settings.command_requires_full_index = 0; >> + } > > I am kind-a surprised if the control reaches this deep if you are > not in a repository. In git.c::commands[] list, all three primary > entry points that call checkout_main(), namely, cmd_checkout(), > cmd_restore(), and cmd_switch(), are marked with RUN_SETUP bit, > which makes us call setup_git_directory() even before we call the > cmd_X() function. And setup_git_directory() dies with "not a git > repository (or any of the parent directories)" outside a repository. > > So, how can startup_info->have_repository bit be false if the > control flow reaches here? Or am I grossly misunderstanding what > you are trying to do? > This was in reaction to the t0012-help.sh tests failing with the new BUG in prepare_repo_settings. However, Elijah pointed out that it's a better idea to move prepare_repo_settings farther down (after parse_options) instead. So I will be issuing that change as part of v5. >> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c >> index 1a3dd445f83..45dc2258dc7 100644 >> --- a/builtin/pack-objects.c >> +++ b/builtin/pack-objects.c >> @@ -3976,9 +3976,12 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) >> read_replace_refs = 0; > > Ditto wrt RUN_SETUP. > Updating with the same change as checkout for v5. >> diff --git a/commit-graph.c b/commit-graph.c >> index 2706683acfe..265c010122e 100644 >> --- a/commit-graph.c >> +++ b/commit-graph.c >> @@ -632,10 +632,13 @@ static int prepare_commit_graph(struct repository *r) >> struct object_directory *odb; >> /* >> + * Early return if there is no git dir or if the commit graph is >> + * disabled. >> + * >> * This must come before the "already attempted?" check below, because >> * we want to disable even an already-loaded graph file. >> */ >> - if (r->commit_graph_disabled) >> + if (!r->gitdir || r->commit_graph_disabled) >> return 0; > > I haven't followed the control flow, but this one probably is a good > addition (in other words, unlike cmd_pack_objects(), I cannot convince > myself that r->gitdir will never be NULL here). > >> diff --git a/repo-settings.c b/repo-settings.c >> index b93e91a212e..00ca5571a1a 100644 >> --- a/repo-settings.c >> +++ b/repo-settings.c >> @@ -17,6 +17,9 @@ void prepare_repo_settings(struct repository *r) >> char *strval; >> int manyfiles; >> >> + if (!r->gitdir) >> + BUG("Cannot add settings for uninitialized repository"); >> + > > This is a very good idea. If I recall correctly, I think I reviewed > a bugfix patch that would have been simplified if we had this check > here. >Lessley