Re: [PATCH v4 1/4] sparse index: enable only for git repos
From: Elijah Newren <hidden>
Date: 2021-11-23 07:41:52
On Mon, Nov 22, 2021 at 2:42 PM Lessley Dennington via GitGitGadget [off-list ref] wrote:
From: Lessley Dennington <redacted> 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.
Why only checkout and pack-objects? Why don't the -h flags to all of the following need it as well?: $ git grep -l prepare_repo_settings | grep builtin/ builtin/add.c builtin/blame.c builtin/checkout.c builtin/commit.c builtin/diff.c builtin/fetch.c builtin/gc.c builtin/merge.c builtin/pack-objects.c builtin/rebase.c builtin/reset.c builtin/revert.c builtin/sparse-checkout.c builtin/update-index.c If none of these need it, was it because they put prepare_repo_settings() calls after some other basic checks had been done so more do not have to be added? If so, is there a similar place in checkout and pack-objects where their calls to prepare_repo_settings() can be moved? (Looking ahead, it appears you moved some code in patch 2 to do something like this. Are the similar moves that could be done here?)
Finally, ensure the above BUG is not triggered for commit-graph by returning early if the git directory does not exist.
If commit-graph needs a special case to avoid triggering the BUG, wouldn't several of these need it too?: $ git grep -l prepare_repo_settings | grep -v builtin/ commit-graph.c fetch-negotiator.c merge-recursive.c midx.c read-cache.c repo-settings.c repository.c repository.h sparse-index.c t/helper/test-read-cache.c t/helper/test-read-graph.c unpack-trees.c or are their calls to prepare_repo_settings() only done after gitdir setup? If the latter, perhaps the commit-graph function calls could be moved after gitdir setup too to avoid the need to do extra checks in it?
quoted hunk ↗ jump to hunk
Signed-off-by: Lessley Dennington <redacted> --- 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; + } opts->track = BRANCH_TRACK_UNSPECIFIED;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; 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 (startup_info->have_repository) { + 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);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; if (r->objects->commit_graph_attempted)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"); + if (r->settings.initialized++) return;
I'm not what the BUG() is trying to help us catch, but I'm worried that there are many additional places that now need workarounds to avoid triggering bugs.