[PATCH 0/10] cleaning up check_repository_format_gently

[PATCH 0/10] cleaning up check_repository_format_gently

[Jeff King]

After the discussion in:

  http://thread.gmane.org/gmane.comp.version-control.git/287949/focus=288002

I came up with this series to try to address some of the warts in
check_repository_format_gently().

I had hoped to come up with something very small that could go at the
front of the refs-backend series, but as with any time I look at the
setup code, it managed to snowball into a potpourri of hidden
assumptions.

So I hope David isn't _too_ irritated to see this in his inbox. Rebasing
the refs-backend on top shouldn't be too bad, and the end result would
be cleaner and more correct. My bigger worry is just that changing the
setup code is inherently flaky, and I don't want to hold David's series
hostage.

  [01/10]: setup: document check_repository_format()
  [02/10]: wrap shared_repository global in get/set accessors
  [03/10]: lazily load core.sharedrepository
  [04/10]: check_repository_format_gently: stop using git_config_early
  [05/10]: config: drop git_config_early
  [06/10]: setup: refactor repo format reading and verification
  [07/10]: init: use setup.c's repo version verification
  [08/10]: setup: unify repository version callbacks
  [09/10]: setup: drop repository_format_version global
  [10/10]: setup: drop GIT_REPO_VERSION constants

-Peff

[PATCH 01/10] setup: document check_repository_format()

[Jeff King]

This function's interface is rather enigmatic, so let's
document it further.

While we're here, let's also drop the return value. It will
always either be "0" or the function will die (consequently,
neither of its two callers bothered to check the return).

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h | 9 ++++++++-
 setup.c | 4 ++--
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index d7ff46e..a3b6b0f 100644
--- a/cache.h
+++ b/cache.h
@@ -747,7 +747,14 @@ extern int grafts_replace_parents;
 #define GIT_REPO_VERSION_READ 1
 extern int repository_format_version;
 extern int repository_format_precious_objects;
-extern int check_repository_format(void);
+
+/*
+ * 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
+ * set_git_dir() before calling this, and use it only for "are we in a valid
+ * repo?".
+ */
+extern void check_repository_format(void);
 
 #define MTIME_CHANGED	0x0001
 #define CTIME_CHANGED	0x0002
diff --git a/setup.c b/setup.c
index de1a2a7..b2f2e69 100644
--- a/setup.c
+++ b/setup.c
@@ -982,9 +982,9 @@ int check_repository_format_version(const char *var, const char *value, void *cb
 	return 0;
 }
 
-int check_repository_format(void)
+void check_repository_format(void)
 {
-	return check_repository_format_gently(get_git_dir(), NULL);
+	check_repository_format_gently(get_git_dir(), NULL);
 }
 
 /*
-- 
2.8.0.rc0.278.gfeb5644

[PATCH 02/10] wrap shared_repository global in get/set accessors

[Jeff King]

It would be useful to control access to the global
shared_repository, so that we can lazily load its config.
The first step to doing so is to make sure all access
goes through a set of functions.

This step is purely mechanical, and should result in no
change of behavior.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/init-db.c | 24 ++++++++++++------------
 cache.h           |  4 +++-
 environment.c     | 13 ++++++++++++-
 path.c            | 10 +++++-----
 setup.c           |  2 +-
 5 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 6223b7d..e9b2256 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -199,13 +199,13 @@ static int create_default_files(const char *template_path)
 
 	/* reading existing config may have overwrote it */
 	if (init_shared_repository != -1)
-		shared_repository = init_shared_repository;
+		set_shared_repository(init_shared_repository);
 
 	/*
 	 * We would have created the above under user's umask -- under
 	 * shared-repository settings, we would need to fix them up.
 	 */
-	if (shared_repository) {
+	if (get_shared_repository()) {
 		adjust_shared_perm(get_git_dir());
 		adjust_shared_perm(git_path_buf(&buf, "refs"));
 		adjust_shared_perm(git_path_buf(&buf, "refs/heads"));
@@ -369,7 +369,7 @@ int init_db(const char *template_dir, unsigned int flags)
 
 	create_object_directory();
 
-	if (shared_repository) {
+	if (get_shared_repository()) {
 		char buf[10];
 		/* We do not spell "group" and such, so that
 		 * the configuration can be read by older version
@@ -377,12 +377,12 @@ int init_db(const char *template_dir, unsigned int flags)
 		 * and compatibility values for PERM_GROUP and
 		 * PERM_EVERYBODY.
 		 */
-		if (shared_repository < 0)
+		if (get_shared_repository() < 0)
 			/* force to the mode value */
-			xsnprintf(buf, sizeof(buf), "0%o", -shared_repository);
-		else if (shared_repository == PERM_GROUP)
+			xsnprintf(buf, sizeof(buf), "0%o", -get_shared_repository());
+		else if (get_shared_repository() == PERM_GROUP)
 			xsnprintf(buf, sizeof(buf), "%d", OLD_PERM_GROUP);
-		else if (shared_repository == PERM_EVERYBODY)
+		else if (get_shared_repository() == PERM_EVERYBODY)
 			xsnprintf(buf, sizeof(buf), "%d", OLD_PERM_EVERYBODY);
 		else
 			die("BUG: invalid value for shared_repository");
@@ -398,7 +398,7 @@ int init_db(const char *template_dir, unsigned int flags)
 		   "", and the last '%s%s' is the verbatim directory name. */
 		printf(_("%s%s Git repository in %s%s\n"),
 		       reinit ? _("Reinitialized existing") : _("Initialized empty"),
-		       shared_repository ? _(" shared") : "",
+		       get_shared_repository() ? _(" shared") : "",
 		       git_dir, len && git_dir[len-1] != '/' ? "/" : "");
 	}
 
@@ -493,8 +493,8 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 				 * and we know shared_repository should always be 0;
 				 * but just in case we play safe.
 				 */
-				saved = shared_repository;
-				shared_repository = 0;
+				saved = get_shared_repository();
+				set_shared_repository(0);
 				switch (safe_create_leading_directories_const(argv[0])) {
 				case SCLD_OK:
 				case SCLD_PERMS:
@@ -506,7 +506,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 					die_errno(_("cannot mkdir %s"), argv[0]);
 					break;
 				}
-				shared_repository = saved;
+				set_shared_repository(saved);
 				if (mkdir(argv[0], 0777) < 0)
 					die_errno(_("cannot mkdir %s"), argv[0]);
 				mkdir_tried = 1;
@@ -524,7 +524,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	}
 
 	if (init_shared_repository != -1)
-		shared_repository = init_shared_repository;
+		set_shared_repository(init_shared_repository);
 
 	/*
 	 * GIT_WORK_TREE makes sense only in conjunction with GIT_DIR
diff --git a/cache.h b/cache.h
index a3b6b0f..43c6a44 100644
--- a/cache.h
+++ b/cache.h
@@ -651,7 +651,6 @@ extern int prefer_symlink_refs;
 extern int log_all_ref_updates;
 extern int warn_ambiguous_refs;
 extern int warn_on_object_refname_ambiguity;
-extern int shared_repository;
 extern const char *apply_default_whitespace;
 extern const char *apply_default_ignorewhitespace;
 extern const char *git_attributes_file;
@@ -664,6 +663,9 @@ extern size_t delta_base_cache_limit;
 extern unsigned long big_file_threshold;
 extern unsigned long pack_size_limit_cfg;
 
+void set_shared_repository(int value);
+int get_shared_repository(void);
+
 /*
  * Do replace refs need to be checked this run?  This variable is
  * initialized to true unless --no-replace-object is used or
diff --git a/environment.c b/environment.c
index 6dec9d0..b42e238 100644
--- a/environment.c
+++ b/environment.c
@@ -29,7 +29,6 @@ int repository_format_version;
 int repository_format_precious_objects;
 const char *git_commit_encoding;
 const char *git_log_output_encoding;
-int shared_repository = PERM_UMASK;
 const char *apply_default_whitespace;
 const char *apply_default_ignorewhitespace;
 const char *git_attributes_file;
@@ -325,3 +324,15 @@ const char *get_commit_output_encoding(void)
 {
 	return git_commit_encoding ? git_commit_encoding : "UTF-8";
 }
+
+static int the_shared_repository = PERM_UMASK;
+
+void set_shared_repository(int value)
+{
+	the_shared_repository = value;
+}
+
+int get_shared_repository(void)
+{
+	return the_shared_repository;
+}
diff --git a/path.c b/path.c
index 8b7e168..a6f1cd6 100644
--- a/path.c
+++ b/path.c
@@ -699,17 +699,17 @@ static int calc_shared_perm(int mode)
 {
 	int tweak;
 
-	if (shared_repository < 0)
-		tweak = -shared_repository;
+	if (get_shared_repository() < 0)
+		tweak = -get_shared_repository();
 	else
-		tweak = shared_repository;
+		tweak = get_shared_repository();
 
 	if (!(mode & S_IWUSR))
 		tweak &= ~0222;
 	if (mode & S_IXUSR)
 		/* Copy read bits to execute bits */
 		tweak |= (tweak & 0444) >> 2;
-	if (shared_repository < 0)
+	if (get_shared_repository() < 0)
 		mode = (mode & ~0777) | tweak;
 	else
 		mode |= tweak;
@@ -722,7 +722,7 @@ int adjust_shared_perm(const char *path)
 {
 	int old_mode, new_mode;
 
-	if (!shared_repository)
+	if (!get_shared_repository())
 		return 0;
 	if (get_st_mode_bits(path, &old_mode) < 0)
 		return -1;
diff --git a/setup.c b/setup.c
index b2f2e69..ac777c5 100644
--- a/setup.c
+++ b/setup.c
@@ -377,7 +377,7 @@ static int check_repo_format(const char *var, const char *value, void *cb)
 	if (strcmp(var, "core.repositoryformatversion") == 0)
 		repository_format_version = git_config_int(var, value);
 	else if (strcmp(var, "core.sharedrepository") == 0)
-		shared_repository = git_config_perm(var, value);
+		set_shared_repository(git_config_perm(var, value));
 	else if (skip_prefix(var, "extensions.", &ext)) {
 		/*
 		 * record any known extensions here; otherwise,
-- 
2.8.0.rc0.278.gfeb5644

[PATCH 03/10] lazily load core.sharedrepository

[Jeff King]

The "shared_repository" config is loaded as part of
check_repository_format_version, but it's not quite like the
other values we check there. Something like
core.repositoryformatversion only makes sense in per-repo
config, but core.sharedrepository can be set in a per-user
config (e.g., to make all "git init" invocations shared by
default).

So it would make more sense as part of git_default_config.
Commit 457f06d (Introduce core.sharedrepository, 2005-12-22)
says:

  [...]the config variable is set in the function which
  checks the repository format. If this were done in
  git_default_config instead, a lot of programs would need
  to be modified to call git_config(git_default_config)
  first.

This is still the case today, but we have one extra trick up
our sleeve. Now that we have the git_configset
infrastructure, it's not so expensive for us to ask for a
single value. So we can simply lazy-load it on demand.

This should be OK to do in general. There are some problems
with loading config before setup_git_directory() is called,
but we shouldn't be accessing the value before then (if we
were, then it would already be broken, as the variable would
not have been set by check_repository_format_version!). The
trickiest caller is git-init, but it handles the values
manually itself.

Signed-off-by: Jeff King <peff@peff.net>
---
Of the whole series, this is the one I most fear causing problems. It
passes the test suite, but that isn't to say there isn't a weird corner
case lurking.

 environment.c | 9 +++++++++
 setup.c       | 2 --
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/environment.c b/environment.c
index b42e238..8b8c8e8 100644
--- a/environment.c
+++ b/environment.c
@@ -326,13 +326,22 @@ const char *get_commit_output_encoding(void)
 }
 
 static int the_shared_repository = PERM_UMASK;
+static int need_shared_repository_from_config = 1;
 
 void set_shared_repository(int value)
 {
 	the_shared_repository = value;
+	need_shared_repository_from_config = 0;
 }
 
 int get_shared_repository(void)
 {
+	if (need_shared_repository_from_config) {
+		const char *var = "core.sharedrepository";
+		const char *value;
+		if (!git_config_get_value(var, &value))
+			the_shared_repository = git_config_perm(var, value);
+		need_shared_repository_from_config = 0;
+	}
 	return the_shared_repository;
 }
diff --git a/setup.c b/setup.c
index ac777c5..a02932b 100644
--- a/setup.c
+++ b/setup.c
@@ -376,8 +376,6 @@ static int check_repo_format(const char *var, const char *value, void *cb)
 
 	if (strcmp(var, "core.repositoryformatversion") == 0)
 		repository_format_version = git_config_int(var, value);
-	else if (strcmp(var, "core.sharedrepository") == 0)
-		set_shared_repository(git_config_perm(var, value));
 	else if (skip_prefix(var, "extensions.", &ext)) {
 		/*
 		 * record any known extensions here; otherwise,
-- 
2.8.0.rc0.278.gfeb5644

[PATCH 04/10] check_repository_format_gently: stop using git_config_early

[Jeff King]

There's a chicken-and-egg problem with using the regular
git_config during the repository setup process. We get
around it here by using a special interface that lets us
specify the per-repo config, and avoid calling
git_pathdup().

But this interface doesn't actually make sense. It will look
in the system and per-user config, too; we definitely would
not want to accept a core.repositoryformatversion from
there.

The git_config_from_file interface is a better match, as it
lets us look at a single file.

Signed-off-by: Jeff King <peff@peff.net>
---
This has literally been bugging me for 8 years.

 setup.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/setup.c b/setup.c
index a02932b..a6013e6 100644
--- a/setup.c
+++ b/setup.c
@@ -409,15 +409,10 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)
 	repo_config = sb.buf;
 
 	/*
-	 * git_config() can't be used here because it calls git_pathdup()
-	 * to get $GIT_CONFIG/config. That call will make setup_git_env()
-	 * set git_dir to ".git".
-	 *
-	 * We are in gitdir setup, no git dir has been found useable yet.
-	 * Use a gentler version of git_config() to check if this repo
-	 * is a good one.
+	 * Ignore return value; for historical reasons, we must treat a missing
+	 * config file as a noop (git-init relies on this).
 	 */
-	git_config_early(fn, NULL, repo_config);
+	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",
-- 
2.8.0.rc0.278.gfeb5644

[PATCH 05/10] config: drop git_config_early

[Jeff King]

There are no more callers, and it's a rather confusing
interface. This could just be folded into
git_config_with_options(), but for the sake of readability,
we'll leave it as a separate (static) helper function.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/technical/api-config.txt |  7 -------
 cache.h                                |  1 -
 config.c                               | 12 ++++--------
 3 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index 0d8b99b..20741f3 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -63,13 +63,6 @@ parse for configuration, rather than looking in the usual files. Regular
 Specify whether include directives should be followed in parsed files.
 Regular `git_config` defaults to `1`.
 
-There is a special version of `git_config` called `git_config_early`.
-This version takes an additional parameter to specify the repository
-config, instead of having it looked up via `git_path`. This is useful
-early in a Git program before the repository has been found. Unless
-you're working with early setup code, you probably don't want to use
-this.
-
 Reading Specific Files
 ----------------------
 
diff --git a/cache.h b/cache.h
index 43c6a44..1825851 100644
--- a/cache.h
+++ b/cache.h
@@ -1525,7 +1525,6 @@ extern void git_config(config_fn_t fn, void *);
 extern int git_config_with_options(config_fn_t fn, void *,
 				   struct git_config_source *config_source,
 				   int respect_includes);
-extern int git_config_early(config_fn_t fn, void *, const char *repo_config);
 extern int git_parse_ulong(const char *, unsigned long *);
 extern int git_parse_maybe_bool(const char *);
 extern int git_config_int(const char *, const char *);
diff --git a/config.c b/config.c
index 9ba40bc..7ddb287 100644
--- a/config.c
+++ b/config.c
@@ -1188,11 +1188,12 @@ int git_config_system(void)
 	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
 }
 
-int git_config_early(config_fn_t fn, void *data, const char *repo_config)
+static int do_git_config_sequence(config_fn_t fn, void *data)
 {
 	int ret = 0, found = 0;
 	char *xdg_config = xdg_config_home("config");
 	char *user_config = expand_user_path("~/.gitconfig");
+	char *repo_config = git_pathdup("config");
 
 	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {
 		ret += git_config_from_file(fn, git_etc_gitconfig(),
@@ -1228,6 +1229,7 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 
 	free(xdg_config);
 	free(user_config);
+	free(repo_config);
 	return ret == 0 ? found : ret;
 }
 
@@ -1235,8 +1237,6 @@ int git_config_with_options(config_fn_t fn, void *data,
 			    struct git_config_source *config_source,
 			    int respect_includes)
 {
-	char *repo_config = NULL;
-	int ret;
 	struct config_include_data inc = CONFIG_INCLUDE_INIT;
 
 	if (respect_includes) {
@@ -1257,11 +1257,7 @@ int git_config_with_options(config_fn_t fn, void *data,
 	else if (config_source && config_source->blob)
 		return git_config_from_blob_ref(fn, config_source->blob, data);
 
-	repo_config = git_pathdup("config");
-	ret = git_config_early(fn, data, repo_config);
-	if (repo_config)
-		free(repo_config);
-	return ret;
+	return do_git_config_sequence(fn, data);
 }
 
 static void git_config_raw(config_fn_t fn, void *data)
-- 
2.8.0.rc0.278.gfeb5644

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

[Jeff King]

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

[PATCH 07/10] init: use setup.c's repo version verification

[Jeff King]

We check our templates to make sure they are from a
version of git we understand (otherwise we would init a
repository we cannot ourselves run in!). But our simple
integer check has fallen behind the times. Let's use the
helpers that setup.c provides to do it right.

Signed-off-by: Jeff King <peff@peff.net>
---
I actually wonder if this code has been triggered ever, in the history
of git. We do not ship a "config" file in our templates, and I doubt
that anybody would ship one that sets core.repositoryformatversion.

 builtin/init-db.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index e9b2256..d9934f3 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -95,6 +95,8 @@ static void copy_templates(const char *template_dir)
 	struct strbuf path = STRBUF_INIT;
 	struct strbuf template_path = STRBUF_INIT;
 	size_t template_len;
+	struct repository_format template_format;
+	struct strbuf err = STRBUF_INIT;
 	DIR *dir;
 	char *to_free = NULL;
 
@@ -121,17 +123,18 @@ static void copy_templates(const char *template_dir)
 
 	/* Make sure that template is from the correct vintage */
 	strbuf_addstr(&template_path, "config");
-	repository_format_version = 0;
-	git_config_from_file(check_repository_format_version,
-			     template_path.buf, NULL);
+	read_repository_format(&template_format, template_path.buf);
 	strbuf_setlen(&template_path, template_len);
 
-	if (repository_format_version &&
-	    repository_format_version != GIT_REPO_VERSION) {
-		warning(_("not copying templates of "
-			"a wrong format version %d from '%s'"),
-			repository_format_version,
-			template_dir);
+	/*
+	 * No mention of version at all is OK, but anything else should be
+	 * verified.
+	 */
+	if (template_format.version >= 0 &&
+	    verify_repository_format(&template_format, &err) < 0) {
+		warning(_("not copying templates from '%s': %s"),
+			  template_dir, err.buf);
+		strbuf_release(&err);
 		goto close_free_return;
 	}
 
-- 
2.8.0.rc0.278.gfeb5644

[PATCH 08/10] setup: unify repository version callbacks

[Jeff King]

Once upon a time, check_repository_format_gently would parse
the config with a single callback, and that callback would
set up a bunch of global variables. But now that we have
separate workdirs, we have to be more careful. Commit
31e26eb (setup.c: support multi-checkout repo setup,
2014-11-30) introduced a reduced callback which omits some
values like core.worktree. In the "main" callback we call
the reduced one, and then add back in the missing variables.

Now that we have split the config-parsing from the munging
of the global variables, we can handle all the config with a
single callback, and keep all of the "are we in a separate
workdir" logic together in the caller.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h |  1 -
 setup.c | 69 ++++++++++++++++++++++-------------------------------------------
 2 files changed, 23 insertions(+), 47 deletions(-)

diff --git a/cache.h b/cache.h
index d03f5d6..1795807 100644
--- a/cache.h
+++ b/cache.h
@@ -1571,7 +1571,6 @@ extern void git_config_set_multivar_in_file(const char *, const char *, const ch
 extern int git_config_rename_section(const char *, const char *);
 extern int git_config_rename_section_in_file(const char *, const char *, const char *);
 extern const char *git_etc_gitconfig(void);
-extern int check_repository_format_version(const char *var, const char *value, void *cb);
 extern int git_env_bool(const char *, int);
 extern unsigned long git_env_ulong(const char *, unsigned long);
 extern int git_config_system(void);
diff --git a/setup.c b/setup.c
index 3d498af..a04d7dd 100644
--- a/setup.c
+++ b/setup.c
@@ -388,27 +388,26 @@ static int check_repo_format(const char *var, const char *value, void *vdata)
 			data->precious_objects = git_config_bool(var, value);
 		else
 			string_list_append(&data->unknown_extensions, ext);
+	} else if (strcmp(var, "core.bare") == 0) {
+		data->is_bare = git_config_bool(var, value);
+	} else if (strcmp(var, "core.worktree") == 0) {
+		if (!value)
+			return config_error_nonbool(var);
+		data->work_tree = xstrdup(value);
 	}
 	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;
 	struct strbuf err = STRBUF_INIT;
 	struct repository_format candidate;
-	config_fn_t fn;
-
-	if (get_common_dir(&sb, gitdir))
-		fn = check_repo_format;
-	else
-		fn = check_repository_format_version;
+	int has_common;
 
+	has_common = get_common_dir(&sb, gitdir);
 	strbuf_addstr(&sb, "/config");
-	read_repository_format_1(&candidate, fn, sb.buf);
+	read_repository_format(&candidate, sb.buf);
 	strbuf_release(&sb);
 
 	/*
@@ -432,37 +431,31 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)
 	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)
+	if (!has_common) {
+		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;
-	}
-	if (candidate.work_tree) {
-		free(git_work_tree_cfg);
-		git_work_tree_cfg = candidate.work_tree;
-		inside_work_tree = -1;
+		}
+	} else {
+		free(candidate.work_tree);
 	}
 
 	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)
+void read_repository_format(struct repository_format *format, 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);
-}
-
-void read_repository_format(struct repository_format *format, const char *path)
-{
-	read_repository_format_1(format, check_repository_format_version, path);
+	git_config_from_file(check_repo_format, path, format);
 }
 
 int verify_repository_format(const struct repository_format *format,
@@ -1000,22 +993,6 @@ int git_config_perm(const char *var, const char *value)
 	return -(i & 0666);
 }
 
-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) {
-		data->is_bare = git_config_bool(var, value);
-	} else if (strcmp(var, "core.worktree") == 0) {
-		if (!value)
-			return config_error_nonbool(var);
-		data->work_tree = xstrdup(value);
-	}
-	return 0;
-}
-
 void check_repository_format(void)
 {
 	check_repository_format_gently(get_git_dir(), NULL);
-- 
2.8.0.rc0.278.gfeb5644

[PATCH 09/10] setup: drop repository_format_version global

[Jeff King]

Nobody reads this anymore, and they're not likely to; the
interesting thing is whether or not we passed
check_repository_format(), and possibly the individual
"extension" variables.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h       | 1 -
 environment.c | 1 -
 setup.c       | 1 -
 3 files changed, 3 deletions(-)

diff --git a/cache.h b/cache.h
index 1795807..ecefa00 100644
--- a/cache.h
+++ b/cache.h
@@ -747,7 +747,6 @@ extern int grafts_replace_parents;
  */
 #define GIT_REPO_VERSION 0
 #define GIT_REPO_VERSION_READ 1
-extern int repository_format_version;
 extern int repository_format_precious_objects;
 
 struct repository_format {
diff --git a/environment.c b/environment.c
index 8b8c8e8..d9e3861 100644
--- a/environment.c
+++ b/environment.c
@@ -25,7 +25,6 @@ int log_all_ref_updates = -1; /* unspecified */
 int warn_ambiguous_refs = 1;
 int warn_on_object_refname_ambiguity = 1;
 int ref_paranoia = -1;
-int repository_format_version;
 int repository_format_precious_objects;
 const char *git_commit_encoding;
 const char *git_log_output_encoding;
diff --git a/setup.c b/setup.c
index a04d7dd..f52011e 100644
--- a/setup.c
+++ b/setup.c
@@ -428,7 +428,6 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)
 		die("%s", err.buf);
 	}
 
-	repository_format_version = candidate.version;
 	repository_format_precious_objects = candidate.precious_objects;
 	string_list_clear(&candidate.unknown_extensions, 0);
 	if (!has_common) {
-- 
2.8.0.rc0.278.gfeb5644

[PATCH 10/10] setup: drop GIT_REPO_VERSION constants

[Jeff King]

As each constant is used in only one place, they are not
helping us avoid duplication. And they may be actively
misleading, as a version check is now much more complicated
than a simple integer comparison. The logic is in
verify_repository_format, and if you are thinking about
bumping the version, you _should_ have to go look at that
function.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/init-db.c | 5 +----
 cache.h           | 7 -------
 setup.c           | 6 +++---
 3 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index d9934f3..ee2156e 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -176,7 +176,6 @@ static int create_default_files(const char *template_path)
 	struct stat st1;
 	struct strbuf buf = STRBUF_INIT;
 	char *path;
-	char repo_version_string[10];
 	char junk[2];
 	int reinit;
 	int filemode;
@@ -228,9 +227,7 @@ static int create_default_files(const char *template_path)
 	}
 
 	/* This forces creation of new config file */
-	xsnprintf(repo_version_string, sizeof(repo_version_string),
-		  "%d", GIT_REPO_VERSION);
-	git_config_set("core.repositoryformatversion", repo_version_string);
+	git_config_set("core.repositoryformatversion", "0");
 
 	/* Check filemode trustability */
 	path = git_path_buf(&buf, "config");
diff --git a/cache.h b/cache.h
index ecefa00..d9c23d5 100644
--- a/cache.h
+++ b/cache.h
@@ -740,13 +740,6 @@ extern char *notes_ref_name;
 
 extern int grafts_replace_parents;
 
-/*
- * GIT_REPO_VERSION is the version we write by default. The
- * _READ variant is the highest number we know how to
- * handle.
- */
-#define GIT_REPO_VERSION 0
-#define GIT_REPO_VERSION_READ 1
 extern int repository_format_precious_objects;
 
 struct repository_format {
diff --git a/setup.c b/setup.c
index f52011e..75d5939 100644
--- a/setup.c
+++ b/setup.c
@@ -460,9 +460,9 @@ void read_repository_format(struct repository_format *format, const char *path)
 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);
+	if (format->version > 1) {
+		strbuf_addf(err, "Expected git repo version <= 1, found %d",
+			    format->version);
 		return -1;
 	}
 
-- 
2.8.0.rc0.278.gfeb5644

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

[David Turner]

On Tue, 2016-03-01 at 09:42 -0500, Jeff King wrote:
> +/*
> + * 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);

Generally speaking, I don't care for this interface; I would prefer to
return -1 and not change the struct.  But I see why it's maybe simpler
in this one case.

+/*
> + * 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.
> + */

I don't understand this comment.  fn is not being swapped out -- it's
being passed on directly.  

> +static void read_repository_format_1(struct repository_format
> *format,
> +				     config_fn_t fn, const char
> *path)

The argument order here is different from git_config_from_file -- is
that for a reason?

> +	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);

newline here or something?  Otherwise multiple unknown extensions will
get jammed together.

Re: [PATCH 10/10] setup: drop GIT_REPO_VERSION constants

[David Turner]

On Tue, 2016-03-01 at 09:45 -0500, Jeff King wrote:
> -	char repo_version_string[10];
>  
>  	/* This forces creation of new config file */
> -	xsnprintf(repo_version_string, sizeof(repo_version_string),
> -		  "%d", GIT_REPO_VERSION);
> -	git_config_set("core.repositoryformatversion",
> repo_version_string);
> +	git_config_set("core.repositoryformatversion", "0");

I'm about to use that in my series, so let's not get rid of it here.

Re: [PATCH 0/10] cleaning up check_repository_format_gently

[David Turner]

On Tue, 2016-03-01 at 09:35 -0500, Jeff King wrote:
> After the discussion in:
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/287949/focus
> =288002
> 
> I came up with this series to try to address some of the warts in
> check_repository_format_gently().
> 
> I had hoped to come up with something very small that could go at the
> front of the refs-backend series, but as with any time I look at the
> setup code, it managed to snowball into a potpourri of hidden
> assumptions.
> 
> So I hope David isn't _too_ irritated to see this in his inbox.


I commented on two of the patches; the rest look good to me.

I rebased on these (minus the repo_version_string change from 10/10). Wasn't too bad.  And it did clean up the submodule check.

I will wait to resend my series until I hear back about your shortlog/mailmap fix and your suggestion on a better archive --remote fix.  

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

[Jeff King]

On Tue, Mar 01, 2016 at 04:20:06PM -0500, David Turner wrote:

> On Tue, 2016-03-01 at 09:42 -0500, Jeff King wrote:
> > +/*
> > + * 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);
> 
> Generally speaking, I don't care for this interface; I would prefer to
> return -1 and not change the struct.  But I see why it's maybe simpler
> in this one case.

Yeah, I waffled on it. The main reason was simply that the existing code
it's replacing generally wanted to have a stateful return, do some
cleanup, and then check "did we get a version?".

It would be easy to provide both (which matches the redundancy of
nongit_ok).

> > + * 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.
> > + */
> 
> I don't understand this comment.  fn is not being swapped out -- it's
> being passed on directly.

I meant that internally to setup.c, we need to call with different
variants of "fn", but that is an ugly interface we don't need to expose
outside the file. And further on in the series, we clean that up, and
can drop this internal interface. Maybe it's not even worth commenting
on (or commenting on in the commit message only).

> > +static void read_repository_format_1(struct repository_format
> > *format,
> > +				     config_fn_t fn, const char
> > *path)
> 
> The argument order here is different from git_config_from_file -- is
> that for a reason?

Only that I consider this to be a more object-oriented interface (akin
to strbuf, argv_array, etc), with "struct repository_format" as the
"this" object.  We usually put that argument first.

> > +	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);
> 
> newline here or something?  Otherwise multiple unknown extensions will
> get jammed together.

Thanks, I blindly replaced warning(), which handles the newline. I'll
double-check the other conversions to sure we handle newlines there,
too.

-Peff

Re: [PATCH 10/10] setup: drop GIT_REPO_VERSION constants

[Jeff King]

On Tue, Mar 01, 2016 at 07:13:02PM -0500, David Turner wrote:

> On Tue, 2016-03-01 at 09:45 -0500, Jeff King wrote:
> > -	char repo_version_string[10];
> >  
> >  	/* This forces creation of new config file */
> > -	xsnprintf(repo_version_string, sizeof(repo_version_string),
> > -		  "%d", GIT_REPO_VERSION);
> > -	git_config_set("core.repositoryformatversion",
> > repo_version_string);
> > +	git_config_set("core.repositoryformatversion", "0");
> 
> I'm about to use that in my series, so let's not get rid of it here.

Thanks. I floated this one up to the end exactly to make it easy to drop
in case there were objections. :)

-Peff

Re: [PATCH 03/10] lazily load core.sharedrepository

[Duy Nguyen]

On Tue, Mar 1, 2016 at 9:39 PM, Jeff King <peff@peff.net> wrote:
> +       if (need_shared_repository_from_config) {
> +               const char *var = "core.sharedrepository";
> +               const char *value;
> +               if (!git_config_get_value(var, &value))
> +                       the_shared_repository = git_config_perm(var, value);
> +               need_shared_repository_from_config = 0;
> +       }

If config cache is invalidated and reloaded by some crazy code (alias
code, most likely, but I didn't check), we could be in trouble. Is
there any clean way we could hook in git_config_clear() and reset
need_shared..  back  to 1 (or something of similar effect)? Or perhaps
maintain a "clear counter", increased at every clear, and we keep a
copy here, so we know if any more clear() has been called between
get_shared..() calls.

>         return the_shared_repository;
>  }

-- 
Duy

Re: [PATCH 04/10] check_repository_format_gently: stop using git_config_early

[Duy Nguyen]

On Tue, Mar 1, 2016 at 9:40 PM, Jeff King <peff@peff.net> wrote:
> There's a chicken-and-egg problem with using the regular
> git_config during the repository setup process. We get
> around it here by using a special interface that lets us
> specify the per-repo config, and avoid calling
> git_pathdup().
>
> But this interface doesn't actually make sense. It will look
> in the system and per-user config, too; we definitely would
> not want to accept a core.repositoryformatversion from
> there.
>
> The git_config_from_file interface is a better match, as it
> lets us look at a single file.

Let's see. We look at core.worktree, core.bare,
core.repositoryformatversion, and extensions.* in this code. The first
three should definitely per-repo (and should be mentioned in the
commit as well). But what about the last one? Any possible use case
that wants to enable, say extensions.preciousobjects, for all repos?
Yes we would need to switch core.repo..version to 1 to take effect,
but that does not actually eliminate this case.. just thinking out
loud...
-- 
Duy

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

[Duy Nguyen]

On Tue, Mar 1, 2016 at 9:42 PM, Jeff King <peff@peff.net> wrote:
> -               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;

Since you have given lots of thoughts about this code, perhaps just a
little bit more, double check if these strings should be translatable
or not and mark _() if so?

>         }
>
> -       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);

Ditto.
-- 
Duy

Re: [PATCH 03/10] lazily load core.sharedrepository

[Jeff King]

On Thu, Mar 03, 2016 at 08:00:03PM +0700, Duy Nguyen wrote:

> On Tue, Mar 1, 2016 at 9:39 PM, Jeff King <peff@peff.net> wrote:
> > +       if (need_shared_repository_from_config) {
> > +               const char *var = "core.sharedrepository";
> > +               const char *value;
> > +               if (!git_config_get_value(var, &value))
> > +                       the_shared_repository = git_config_perm(var, value);
> > +               need_shared_repository_from_config = 0;
> > +       }
> 
> If config cache is invalidated and reloaded by some crazy code (alias
> code, most likely, but I didn't check), we could be in trouble. Is
> there any clean way we could hook in git_config_clear() and reset
> need_shared..  back  to 1 (or something of similar effect)? Or perhaps
> maintain a "clear counter", increased at every clear, and we keep a
> copy here, so we know if any more clear() has been called between
> get_shared..() calls.

Yeah, this caches forever. But I'm not sure that's really different from
the existing code, which sets it in check_repository_format_gently(),
which is only called during setup_git_directory(), which cannot be
called twice. So it was effectively static for the full program anyway
(modulo people tweaking the integer variable manually, which is
supported here).

I think something like the "clear counter" you describe is also not
quite sufficient. We have two sources for the variable: the config, and
manually setting it. Once it has been manually set, I think we don't
want to auto-read it from the config anymore (and stomp over what
somebody set). I'm not sure even git_config_clear() is enough of a
single to say "...and also reset everything that might have been set
from these config variables in the past". I think we'd want to do
per-variable invalidation, depending on what the caller wants.

So I'd rather punt on it for now, unless this is breaking a specific
case. I think it should behave the same as the status quo.

-Peff

Re: [PATCH 04/10] check_repository_format_gently: stop using git_config_early

[Jeff King]

On Thu, Mar 03, 2016 at 08:08:40PM +0700, Duy Nguyen wrote:

> On Tue, Mar 1, 2016 at 9:40 PM, Jeff King <peff@peff.net> wrote:
> > There's a chicken-and-egg problem with using the regular
> > git_config during the repository setup process. We get
> > around it here by using a special interface that lets us
> > specify the per-repo config, and avoid calling
> > git_pathdup().
> >
> > But this interface doesn't actually make sense. It will look
> > in the system and per-user config, too; we definitely would
> > not want to accept a core.repositoryformatversion from
> > there.
> >
> > The git_config_from_file interface is a better match, as it
> > lets us look at a single file.
> 
> Let's see. We look at core.worktree, core.bare,
> core.repositoryformatversion, and extensions.* in this code. The first
> three should definitely per-repo (and should be mentioned in the
> commit as well). But what about the last one? Any possible use case
> that wants to enable, say extensions.preciousobjects, for all repos?
> Yes we would need to switch core.repo..version to 1 to take effect,
> but that does not actually eliminate this case.. just thinking out
> loud...

I thought about that; a more likely use is to set extensions.refStorage
once it exists, to use your preferred backend everywhere. But it _has_
to match what's in the repository's on-disk format. Setting it in your
~/.gitconfig is going to break all of your existing repositories using
another backend.

So the right thing is to have init.refStorage or something, and have
init/clone default to that when creating the repo, and set up the
correct extensions.refStorage config _and_ set up the on-disk ref
storage to match.

-Peff

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

[Jeff King]

On Thu, Mar 03, 2016 at 08:19:22PM +0700, Duy Nguyen wrote:

> On Tue, Mar 1, 2016 at 9:42 PM, Jeff King <peff@peff.net> wrote:
> > -               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;
> 
> Since you have given lots of thoughts about this code, perhaps just a
> little bit more, double check if these strings should be translatable
> or not and mark _() if so?

I don't see any reason they cannot be. I prefer not to mix it in with
the other changes, though, so I'll do a patch on top.

-Peff