On 11/23/21 3:39 PM, Junio C Hamano wrote:
"Lessley Dennington via GitGitGadget" <gitgitgadget@xxxxxxxxx>
writes:
From: Lessley Dennington <lessleydennington@xxxxxxxxx>
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 <lessleydennington@xxxxxxxxx>
---
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
