git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 06/10] setup: refactor repo format reading and verification

From
Jeff King <peff@peff.net>
Date
Mar 1, 2016, 14:42 UTC
Message-ID
<20160301144206.GF12887@sigill.intra.peff.net>
In-Reply-To
<20160301143546.GA30806@sigill.intra.peff.net>

When we want to know if we're in a git repository of reasonable vintage, we can call check_repository_format_gently(), which does three things:

  1. Reads the config from the .git/config file.
  2. Verifies that the version info we read is sane.
  3. Writes some global variables based on this.
There are a few things we could improve here.

One is that steps 1 and 3 happen together. So if the verification in step 2 fails, we still clobber the global variables. This is especially bad if we go on to try another repository directory; we may end up with a state of mixed config variables.

The second is there's no way to ask about the repository version for anything besides the main repository we're in. git-init wants to do this, and it's possible that we would want to start doing so for submodules (e.g., to find out which ref backend they're using).

We can improve both by splitting the first two steps into separate functions. Now check_repository_format_gently() calls out to steps 1 and 2, and does 3 only if step 2 succeeds.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h |  23 ++++++++++++
 setup.c | 121 +++++++++++++++++++++++++++++++++++++++++++---------------------
 2 files changed, 104 insertions(+), 40 deletions(-)
diff --git a/cache.h b/cache.h
index 1825851..d03f5d6 100644
--- a/cache.h
+++ b/cache.h
@@ -750,6 +750,29 @@ extern int grafts_replace_parents;
 extern int repository_format_version;
 extern int repository_format_precious_objects;
 
+struct repository_format {
+	int version;
+	int precious_objects;
+	int is_bare;
+	char *work_tree;
+	struct string_list unknown_extensions;
+};
+
+/*
+ * Read the repository format characteristics from the config file "path". If
+ * the version cannot be extracted from the file for any reason, the version
+ * field will be set to -1, and all other fields are undefined.
+ */
+void read_repository_format(struct repository_format *format, const char *path);
+
+/*
+ * Verify that the repository described by repository_format is something we
+ * can read. If it is, return 0. Otherwise, return -1, and "err" will describe
+ * any errors encountered.
+ */
+int verify_repository_format(const struct repository_format *format,
+			     struct strbuf *err);
+
 /*
  * Check the repository format version in the path found in get_git_dir(),
  * and die if it is a version we don't understand. Generally one would
diff --git a/setup.c b/setup.c
index a6013e6..3d498af 100644
--- a/setup.c
+++ b/setup.c
@@ -5,7 +5,6 @@
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
 static int work_tree_config_is_bogus;
-static struct string_list unknown_extensions = STRING_LIST_INIT_DUP;
 
 /*
  * The input parameter must contain an absolute path, and it must already be
@@ -370,12 +369,13 @@ void setup_work_tree(void)
 	initialized = 1;
 }
 
-static int check_repo_format(const char *var, const char *value, void *cb)
+static int check_repo_format(const char *var, const char *value, void *vdata)
 {
+	struct repository_format *data = vdata;
 	const char *ext;
 
 	if (strcmp(var, "core.repositoryformatversion") == 0)
-		repository_format_version = git_config_int(var, value);
+		data->version = git_config_int(var, value);
 	else if (skip_prefix(var, "extensions.", &ext)) {
 		/*
 		 * record any known extensions here; otherwise,
@@ -385,61 +385,105 @@ static int check_repo_format(const char *var, const char *value, void *cb)
 		if (!strcmp(ext, "noop"))
 			;
 		else if (!strcmp(ext, "preciousobjects"))
-			repository_format_precious_objects = git_config_bool(var, value);
+			data->precious_objects = git_config_bool(var, value);
 		else
-			string_list_append(&unknown_extensions, ext);
+			string_list_append(&data->unknown_extensions, ext);
 	}
 	return 0;
 }
 
+static void read_repository_format_1(struct repository_format *, config_fn_t,
+				     const char *);
+
 static int check_repository_format_gently(const char *gitdir, int *nongit_ok)
 {
 	struct strbuf sb = STRBUF_INIT;
-	const char *repo_config;
+	struct strbuf err = STRBUF_INIT;
+	struct repository_format candidate;
 	config_fn_t fn;
-	int ret = 0;
-
-	string_list_clear(&unknown_extensions, 0);
 
 	if (get_common_dir(&sb, gitdir))
 		fn = check_repo_format;
 	else
 		fn = check_repository_format_version;
+
 	strbuf_addstr(&sb, "/config");
-	repo_config = sb.buf;
+	read_repository_format_1(&candidate, fn, sb.buf);
+	strbuf_release(&sb);
 
 	/*
-	 * Ignore return value; for historical reasons, we must treat a missing
-	 * config file as a noop (git-init relies on this).
+	 * For historical use of check_repository_format() in git-init,
+	 * we treat a missing config as a silent "ok", even when nongit_ok
+	 * is unset.
 	 */
-	git_config_from_file(fn, repo_config, NULL);
-	if (GIT_REPO_VERSION_READ < repository_format_version) {
-		if (!nongit_ok)
-			die ("Expected git repo version <= %d, found %d",
-			     GIT_REPO_VERSION_READ, repository_format_version);
-		warning("Expected git repo version <= %d, found %d",
-			GIT_REPO_VERSION_READ, repository_format_version);
-		warning("Please upgrade Git");
-		*nongit_ok = -1;
-		ret = -1;
+	if (candidate.version < 0)
+		return 0;
+
+	if (verify_repository_format(&candidate, &err) < 0) {
+		if (nongit_ok) {
+			warning("%s", err.buf);
+			strbuf_release(&err);
+			*nongit_ok = -1;
+			return -1;
+		}
+		die("%s", err.buf);
 	}
 
-	if (repository_format_version >= 1 && unknown_extensions.nr) {
-		int i;
+	repository_format_version = candidate.version;
+	repository_format_precious_objects = candidate.precious_objects;
+	string_list_clear(&candidate.unknown_extensions, 0);
+	if (candidate.is_bare != -1) {
+		is_bare_repository_cfg = candidate.is_bare;
+		if (is_bare_repository_cfg == 1)
+			inside_work_tree = -1;
+	}
+	if (candidate.work_tree) {
+		free(git_work_tree_cfg);
+		git_work_tree_cfg = candidate.work_tree;
+		inside_work_tree = -1;
+	}
+
+	return 0;
+}
+
+/*
+ * Internally, we need to swap out "fn" here, but we don't want to expose
+ * that to the world. Hence a wrapper around this internal function.
+ */
+static void read_repository_format_1(struct repository_format *format,
+				     config_fn_t fn, const char *path)
+{
+	memset(format, 0, sizeof(*format));
+	format->version = -1;
+	format->is_bare = -1;
+	string_list_init(&format->unknown_extensions, 1);
+	git_config_from_file(fn, path, format);
+}
 
-		if (!nongit_ok)
-			die("unknown repository extension: %s",
-			    unknown_extensions.items[0].string);
+void read_repository_format(struct repository_format *format, const char *path)
+{
+	read_repository_format_1(format, check_repository_format_version, path);
+}
 
-		for (i = 0; i < unknown_extensions.nr; i++)
-			warning("unknown repository extension: %s",
-				unknown_extensions.items[i].string);
-		*nongit_ok = -1;
-		ret = -1;
+int verify_repository_format(const struct repository_format *format,
+			     struct strbuf *err)
+{
+	if (GIT_REPO_VERSION_READ < format->version) {
+		strbuf_addf(err, "Expected git repo version <= %d, found %d",
+			    GIT_REPO_VERSION_READ, format->version);
+		return -1;
 	}
 
-	strbuf_release(&sb);
-	return ret;
+	if (format->version >= 1 && format->unknown_extensions.nr) {
+		int i;
+
+		for (i = 0; i < format->unknown_extensions.nr; i++)
+			strbuf_addf(err, "unknown repository extension: %s",
+				    format->unknown_extensions.items[i].string);
+		return -1;
+	}
+
+	return 0;
 }
 
 /*
@@ -958,19 +1002,16 @@ int git_config_perm(const char *var, const char *value)
 
 int check_repository_format_version(const char *var, const char *value, void *cb)
 {
+	struct repository_format *data = cb;
 	int ret = check_repo_format(var, value, cb);
 	if (ret)
 		return ret;
 	if (strcmp(var, "core.bare") == 0) {
-		is_bare_repository_cfg = git_config_bool(var, value);
-		if (is_bare_repository_cfg == 1)
-			inside_work_tree = -1;
+		data->is_bare = git_config_bool(var, value);
 	} else if (strcmp(var, "core.worktree") == 0) {
 		if (!value)
 			return config_error_nonbool(var);
-		free(git_work_tree_cfg);
-		git_work_tree_cfg = xstrdup(value);
-		inside_work_tree = -1;
+		data->work_tree = xstrdup(value);
 	}
 	return 0;
 }

-- 
2.8.0.rc0.278.gfeb5644
Previous: Jeff KingNext: David Turner
Message 11 of 22 in “cleaning up check_repository_format_gently”
  1. 0/10 cleaning up check_repository_format_gentlyJeff King, Mar 1, 2016
  2. 01/10 setup: document check_repository_format()Jeff King, Mar 1, 2016
  3. 02/10 wrap shared_repository global in get/set accessorsJeff King, Mar 1, 2016
  4. 03/10 lazily load core.sharedrepositoryJeff King, Mar 1, 2016
  5. Duy NguyenMar 3, 2016
  6. Jeff KingMar 3, 2016
  7. 04/10 check_repository_format_gently: stop using git_config_earlyJeff King, Mar 1, 2016
  8. Duy NguyenMar 3, 2016
  9. Jeff KingMar 3, 2016
  10. 05/10 config: drop git_config_earlyJeff King, Mar 1, 2016
  11. 06/10 setup: refactor repo format reading and verificationJeff King, Mar 1, 2016
  12. David TurnerMar 1, 2016
  13. Jeff KingMar 2, 2016
  14. Duy NguyenMar 3, 2016
  15. Jeff KingMar 3, 2016
  16. 07/10 init: use setup.c's repo version verificationJeff King, Mar 1, 2016
  17. 08/10 setup: unify repository version callbacksJeff King, Mar 1, 2016
  18. 09/10 setup: drop repository_format_version globalJeff King, Mar 1, 2016
  19. 10/10 setup: drop GIT_REPO_VERSION constantsJeff King, Mar 1, 2016
  20. David TurnerMar 2, 2016
  21. Jeff KingMar 2, 2016
  22. David TurnerMar 2, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.