Git Future Proofing

Git Future Proofing

[Martin Atukunda]

This patch series adds git repository future proofing to git.

It adds checks for core.repositoryformatversion at various points in the git
architecture, and this is an overview of the patch series

Patch 1 adds GIT_REPO_VERSION and repository_format_version
Patch 2 fixes init-db's template copy so that it handles copying a config file.
Patch 3 adds support for re-reading gits env variables in certain cases
Patch 4 adds the repo format version check for various major operation
Patch 5 adds support for explictly specifying which config file to use
Patch 6 fixes up init-db config copying so as to never copy anything newer.

comments and suggestions welcome.

- Martin -

[PATCH 2/6] Make init-db check repo format version if copying a config file.

[Martin Atukunda]

This patch enables init-db to check the repo format version for a
repository being re-initialised. If the version of the repo is larger than
GIT_VERSION_REPO, it simply dies with an appropriate message.

Signed-Off-By: Martin Atukunda <matlads@dsmagic.com>

---

 init-db.c |   14 ++++++++++++++
 setup.c   |   19 +++++++++++++++++++
 2 files changed, 33 insertions(+), 0 deletions(-)

applies-to: 3e86d91031df530695933e7d9d5d8d9c2f3a683d
601dd50b595a9aa0a57972364262d98290a5d7b2
diff --git a/init-db.c b/init-db.c
index bd88291..90be428 100644
--- a/init-db.c
+++ b/init-db.c
@@ -110,6 +110,19 @@ static void copy_templates_1(char *path,
 	}
 }
 
+static int init_db_config_check(const char *template_path)
+{
+	DIR *dir;
+	struct dirent *de;
+
+	dir = opendir(template_path);
+	while((de = readdir(dir)) != NULL) {
+		if ((strncmp(de->d_name, "config", 5) == 0))
+			return check_repo_format();
+	}
+	return 0;
+}
+
 static void copy_templates(const char *git_dir, int len, char *template_dir)
 {
 	char path[PATH_MAX];
@@ -131,6 +144,7 @@ static void copy_templates(const char *g
 			template_dir);
 		return;
 	}
+	init_db_config_check(template_path);
 
 	memcpy(path, git_dir, len);
 	path[len] = 0;
diff --git a/setup.c b/setup.c
index c487d7e..8597424 100644
--- a/setup.c
+++ b/setup.c
@@ -127,3 +127,22 @@ const char *setup_git_directory(void)
 	cwd[len] = 0;
 	return cwd + offset;
 }
+
+static int check_repo_config(const char *var, const char *value)
+{
+       if (strcmp(var, "core.repositoryformatversion") == 0) {
+               repository_format_version = git_config_int(var, value);
+               return 0;
+       }
+       return 1;
+}
+
+int check_repo_format(void)
+{
+	if (git_config(check_repo_config) == -1)
+		return -1;
+	if (repository_format_version > GIT_REPO_VERSION)
+		die ("Expected git repo version <= %d, found %d", GIT_REPO_VERSION,
+			repository_format_version);
+	return 0;
+}
---
0.99.9.GIT

[PATCH 3/6] Make get_git_dir take a flag that makes it re-read the env. variables

[Martin Atukunda]

Signed-Off-By: Martin Atukunda <matlads@dsmagic.com>

---

 cache.h       |    2 +-
 environment.c |    4 ++--
 path.c        |    2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

applies-to: 0748fd804fbe155503235e2c36d812fc3ef641a0
e3973791e841440ad92d91340fd954b5a58101c7
diff --git a/cache.h b/cache.h
index 54c283d..a455373 100644
--- a/cache.h
+++ b/cache.h
@@ -138,7 +138,7 @@ extern unsigned int active_nr, active_al
 #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
 #define GRAFT_ENVIRONMENT "GIT_GRAFT_FILE"
 
-extern char *get_git_dir(void);
+extern char *get_git_dir(int recheck_env);
 extern char *get_object_directory(void);
 extern char *get_refs_directory(void);
 extern char *get_index_file(void);
diff --git a/environment.c b/environment.c
index 3f19473..6a961ca 100644
--- a/environment.c
+++ b/environment.c
@@ -39,9 +39,9 @@ static void setup_git_env(void)
 		git_graft_file = strdup(git_path("info/grafts"));
 }
 
-char *get_git_dir(void)
+char *get_git_dir(int recheck_env)
 {
-	if (!git_dir)
+	if (!git_dir || recheck_env)
 		setup_git_env();
 	return git_dir;
 }
diff --git a/path.c b/path.c
index 4d88947..e322dc0 100644
--- a/path.c
+++ b/path.c
@@ -42,7 +42,7 @@ char *mkpath(const char *fmt, ...)
 
 char *git_path(const char *fmt, ...)
 {
-	const char *git_dir = get_git_dir();
+	const char *git_dir = get_git_dir(0);
 	va_list args;
 	unsigned len;
 
---
0.99.9.GIT

[PATCH 1/6] Add GIT_REPO_VERSION, and repository_format_version

[Martin Atukunda]

This variable will enable git to track the repository version. It's
currently set to 0. (in true C style :)

Signed-Off-By: Martin Atukunda <matlads@dsmagic.com>

---

 cache.h       |    4 ++++
 environment.c |    1 +
 2 files changed, 5 insertions(+), 0 deletions(-)

applies-to: 339319e60db7b3f96f8c711407b135a54da7aa2e
976b8d57a80d79853df9c142ba30d39b414e1b8e
diff --git a/cache.h b/cache.h
index c7c6637..54c283d 100644
--- a/cache.h
+++ b/cache.h
@@ -182,6 +182,10 @@ extern int trust_executable_bit;
 extern int only_use_symrefs;
 extern int diff_rename_limit_default;
 
+#define GIT_REPO_VERSION 0
+extern int repository_format_version;
+extern int check_repo_format(void);
+
 #define MTIME_CHANGED	0x0001
 #define CTIME_CHANGED	0x0002
 #define OWNER_CHANGED	0x0004
diff --git a/environment.c b/environment.c
index b5026f1..3f19473 100644
--- a/environment.c
+++ b/environment.c
@@ -13,6 +13,7 @@ char git_default_email[MAX_GITNAME];
 char git_default_name[MAX_GITNAME];
 int trust_executable_bit = 1;
 int only_use_symrefs = 0;
+int repository_format_version = 0;
 
 static char *git_dir, *git_object_dir, *git_index_file, *git_refs_dir,
 	*git_graft_file;
---
0.99.9.GIT

[PATCH 4/6] Add check_repo_format check for all major operations.

[Martin Atukunda]

The git-* command set uses 3 entry points in order to prepare
to work with a git repo: enter_repo, get_git_dir, and obviously
setup_git_directory.

This patch adds a check for the repo format version to each of these
entry points. This will automatically enable repo format version
checking for all the git-* programs.

Signed-Off-By: Martin Atukunda <matlads@dsmagic.com>

---

 environment.c |    3 +++
 path.c        |    1 +
 setup.c       |    3 +++
 3 files changed, 7 insertions(+), 0 deletions(-)

applies-to: 8084b72bfb91efc08a9fa83e0893f21c5f60ad92
5d902692ef7eef2763cadfa646eb9245422579af
diff --git a/environment.c b/environment.c
index 6a961ca..458eff8 100644
--- a/environment.c
+++ b/environment.c
@@ -37,6 +37,9 @@ static void setup_git_env(void)
 	git_graft_file = getenv(GRAFT_ENVIRONMENT);
 	if (!git_graft_file)
 		git_graft_file = strdup(git_path("info/grafts"));
+
+	/* check the repo */
+	check_repo_format();
 }
 
 char *get_git_dir(int recheck_env)
diff --git a/path.c b/path.c
index e322dc0..84cb1c5 100644
--- a/path.c
+++ b/path.c
@@ -199,6 +199,7 @@ char *enter_repo(char *path, int strict)
 	if(access("objects", X_OK) == 0 && access("refs", X_OK) == 0 &&
 	   validate_symref("HEAD") == 0) {
 		putenv("GIT_DIR=.");
+		get_git_dir(1); /* re-read the env variables */
 		return current_dir();
 	}
 
diff --git a/setup.c b/setup.c
index 8597424..934f9a3 100644
--- a/setup.c
+++ b/setup.c
@@ -97,6 +97,9 @@ const char *setup_git_directory(void)
 	static char cwd[PATH_MAX+1];
 	int len, offset;
 
+	get_git_dir(1);
+	check_repo_format();
+
 	/*
 	 * If GIT_DIR is set explicitly, we're not going
 	 * to do any discovery
---
0.99.9.GIT

[PATCH 5/6] Allow Specification of the conf file to read for git_config operations

[Martin Atukunda]

This patch adds a git_config_from_file which allows us to specify the
config file to use for git_config operations.

Signed-Off-By: Martin Atukunda <matlads@dsmagic.com>

---

 cache.h  |    1 +
 config.c |   10 +++++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

applies-to: d6c3faa0566795b1c74e693ecc004439c116e6c6
39e4ab307abb445115f3c9ee0e09ed1812568247
diff --git a/cache.h b/cache.h
index a455373..48018ab 100644
--- a/cache.h
+++ b/cache.h
@@ -388,6 +388,7 @@ extern int gitfakemunmap(void *start, si
 
 typedef int (*config_fn_t)(const char *, const char *);
 extern int git_default_config(const char *, const char *);
+extern int git_config_from_file(const char *, config_fn_t fn);
 extern int git_config(config_fn_t fn);
 extern int git_config_int(const char *, const char *);
 extern int git_config_bool(const char *, const char *);
diff --git a/config.c b/config.c
index 5d237c8..c5a5312 100644
--- a/config.c
+++ b/config.c
@@ -245,11 +245,10 @@ int git_default_config(const char *var, 
 	return 0;
 }
 
-int git_config(config_fn_t fn)
+int git_config_from_file(const char *confpath, config_fn_t fn)
 {
 	int ret;
-	FILE *f = fopen(git_path("config"), "r");
-
+	FILE *f = fopen(confpath, "r");
 	ret = -1;
 	if (f) {
 		config_file = f;
@@ -260,6 +259,11 @@ int git_config(config_fn_t fn)
 	return ret;
 }
 
+int git_config(config_fn_t fn)
+{
+	return git_config_from_file(git_path("config"), fn);
+}
+
 /*
  * Find all the stuff for git_config_set() below.
  */
---
0.99.9.GIT

[PATCH 6/6] Add check for downgrading of repo format version via init-db

[Martin Atukunda]

This corrects an earlier assumption that init-db made. It assumed that the
config file was specifying a correct repo format version.

This patch clarifies the assumption by checking the repo format version
specified in the config to be copied, and dies if the copy will result in
an upgrade.

It however, warns if the copy will result in a downgrade of the repo format
version, as git tools are supposed (or will be able) to handle this case :)

Signed-Off-By: Martin Atukunda <matlads@dsmagic.com>

---

 init-db.c |   41 +++++++++++++++++++++++++++++++++++++++--
 1 files changed, 39 insertions(+), 2 deletions(-)

applies-to: 0095aa60b05c91308a25a00dc939bcd95e63b03f
7858de7a1a57e73d2585271b57acfcd044e27e68
diff --git a/init-db.c b/init-db.c
index 90be428..d1fc142 100644
--- a/init-db.c
+++ b/init-db.c
@@ -110,6 +110,15 @@ static void copy_templates_1(char *path,
 	}
 }
 
+static int check_repo_config(const char *var, const char *value)
+{
+       if (strcmp(var, "core.repositoryformatversion") == 0) {
+               repository_format_version = git_config_int(var, value);
+               return 0;
+       }
+       return 1;
+}
+
 static int init_db_config_check(const char *template_path)
 {
 	DIR *dir;
@@ -117,8 +126,36 @@ static int init_db_config_check(const ch
 
 	dir = opendir(template_path);
 	while((de = readdir(dir)) != NULL) {
-		if ((strncmp(de->d_name, "config", 5) == 0))
-			return check_repo_format();
+		if ((strncmp(de->d_name, "config", 5) == 0)) {
+			int rfv1, rfv2;
+			char cpath[PATH_MAX];
+			check_repo_format();
+
+			/* is the file we are copying friendly? */
+			rfv1 = repository_format_version;
+			snprintf(cpath, sizeof(cpath), "%s%s", template_path,
+				de->d_name);
+			git_config_from_file(cpath, check_repo_config);
+			rfv2 = repository_format_version;
+			if (rfv1 == rfv2) {
+				break;
+			}
+			if (rfv2 < rfv1) {
+				/* the repo format specified in the conf file
+				 * we are copying is older than the repo we
+				 * are re-initialising! Downgrading?
+				 */
+				fprintf(stderr, "Possibly downgrading repo"
+					" format version from %d to %d. Check"
+					" config template file!\n", rfv1, rfv2);
+				break;
+			} else 
+				/* OK we die */
+				die ("Won't copy config file"
+					" for repo format version %d over"
+					" one for version %d",
+					rfv2, rfv1);
+		}
 	}
 	return 0;
 }
---
0.99.9.GIT

Re: Git Future Proofing

[Junio C Hamano]

Martin Atukunda <matlads@dsmagic.com> writes:

> Patch 2 fixes init-db's template copy so that it handles
> copying a config file.

The readdir() loop in init_db_config_check() confuses me.  Why
check the prefix config (and "config" has 6 bytes not 5 ;-) not
just open("$template_path/config")???

> Patch 6 fixes up init-db config copying so as to never copy anything newer.

>> It however, warns if the copy will result in a downgrade of the repo format
>> version, as git tools are supposed (or will be able) to handle this case :)

I suspect that it is not enough to copy an older version of
config file along with older version of templates.

Suppose version 0 had .git/remotes/{origin,linus,...} and
version 1 moved that information to a flat file ".git/remotes",
that has a bunch of sections like [remotes.origin] in the config
file format, because we have a mechanism in your patch 5 that
lets us read from more than one configuration file.

Now suppose you are running a version 1 repository, so all your
remotes trees you subscribe to are described in .git/remotes
file.  You somehow used git-init-db to reiniailize it, using
version 0 template, which has "remotes/origin" and
"remotes/linus".  What happens?

Template-copying is designed not to overwrite what is in the
repository, so your .git/remotes file will hopefully be kept,
and the configuration file now claims the repository is in
version 0 format.  But is it really in version 0 format?  You
cannot create .git/remote/frotz file in such a repository.

I think copying older one into a fresh repository might be safe,
but I'd feel safer if we do not play downgrade games like this.

Re: [PATCH 4/6] Add check_repo_format check for all major operations.

[Junio C Hamano]

Martin Atukunda <matlads@dsmagic.com> writes:

> The git-* command set uses 3 entry points in order to prepare
> to work with a git repo: enter_repo, get_git_dir, and obviously
> setup_git_directory.

Thanks, but I think this one is wrong.

> diff --git a/environment.c b/environment.c
> index 6a961ca..458eff8 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -37,6 +37,9 @@ static void setup_git_env(void)
>  	git_graft_file = getenv(GRAFT_ENVIRONMENT);
>  	if (!git_graft_file)
>  		git_graft_file = strdup(git_path("info/grafts"));
> +
> +	/* check the repo */
> +	check_repo_format();
>  }

> diff --git a/setup.c b/setup.c
> index 8597424..934f9a3 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -97,6 +97,9 @@ const char *setup_git_directory(void)
>  	static char cwd[PATH_MAX+1];
>  	int len, offset;
>  
> +	get_git_dir(1);
> +	check_repo_format();
> +
>  	/*
>  	 * If GIT_DIR is set explicitly, we're not going
>  	 * to do any discovery

In setup_git_env() you have only read GIT_DIR environment but
have not done the toplevel discovery.  Especially, this is
called from get_git_dir(), and you call that as the first thing
as setup_git_directory().  However, that function is supposed to
be callable by processes that are in a subdirectory, without
GIT_DIR explicitly specified, and the place get_git_dir() is
called in that function is way before the discovery of the
toplevel happens.  Until then, you do not know where your .git/
directory or .git/config file is. If you start in Documentation
subdirectory in git project, your setup_git_directory() would
first call get_git_dir(), which says "I assume the config file
is at ./.git/config -- oh there is no such thing".  At that
point you are checking Documentation/.git/config.  

It would happen to work because you intend to allow version 0
repository for any future version of tool, and even if this
codepath mistakenly thinks the repository is version 0, it does
not hurt, as long as your setup_git_directory() calls
check_repo_format again after doing the toplevel discovery and
checks the true .git/config file, but I do not think you have
that call in the current series yet.  Even if you had, this does
not feel quite right to me.

Re: [PATCH 4/6] Add check_repo_format check for all major operations.

[Martin Atukunda]

On Tuesday 22 November 2005 11:29, Junio C Hamano wrote:
> Martin Atukunda <matlads@dsmagic.com> writes:
> > The git-* command set uses 3 entry points in order to prepare
> > to work with a git repo: enter_repo, get_git_dir, and obviously
> > setup_git_directory.
>
> Thanks, but I think this one is wrong.
<snip>
> In setup_git_env() you have only read GIT_DIR environment but
> have not done the toplevel discovery.  Especially, this is
> called from get_git_dir(), and you call that as the first thing
> as setup_git_directory().  However, that function is supposed to
> be callable by processes that are in a subdirectory, without
> GIT_DIR explicitly specified, and the place get_git_dir() is
> called in that function is way before the discovery of the
> toplevel happens.  Until then, you do not know where your .git/
> directory or .git/config file is. If you start in Documentation
> subdirectory in git project, your setup_git_directory() would
> first call get_git_dir(), which says "I assume the config file
> is at ./.git/config -- oh there is no such thing".  At that
> point you are checking Documentation/.git/config.
>
> It would happen to work because you intend to allow version 0
> repository for any future version of tool, and even if this
> codepath mistakenly thinks the repository is version 0, it does
> not hurt, as long as your setup_git_directory() calls
> check_repo_format again after doing the toplevel discovery and
> checks the true .git/config file, but I do not think you have
> that call in the current series yet.  Even if you had, this does
> not feel quite right to me.

would something like the following apply in this case: (totally untested :)

--

Add check_repo_format check for setup_git_directory(). This check needs
to be done in 2 cases in this function. first if GIT_DIR is set, and also
after the top_level directory is found.

Signed-Off-By: Martin Atukunda <matlads@dsmagic.com>

diff --git a/setup.c b/setup.c
index 44b9866..45e716a 100644
--- a/setup.c
+++ b/setup.c
@@ -101,8 +101,11 @@ const char *setup_git_directory(void)
 	 * If GIT_DIR is set explicitly, we're not going
 	 * to do any discovery
 	 */
-	if (getenv(GIT_DIR_ENVIRONMENT))
+	if (getenv(GIT_DIR_ENVIRONMENT)) {
+		get_git_dir(1);
+		check_repo_format();
 		return NULL;
+	}
 
 	if (!getcwd(cwd, sizeof(cwd)) || cwd[0] != '/')
 		die("Unable to read current working directory");
@@ -118,6 +121,8 @@ const char *setup_git_directory(void)
 		} while (cwd[--offset] != '/');
 	}
 
+	check_repo_format();
+
 	if (offset == len)
 		return NULL;
 

Re: [PATCH 4/6] Add check_repo_format check for all major operations.

[Junio C Hamano]

Martin Atukunda <matlads@dsmagic.com> writes:

>> that call in the current series yet.  Even if you had, this does
>> not feel quite right to me.
>
> would something like the following apply in this case: (totally untested :)

Yes, although that is exactly what I said "this does not quite
feel right" ;-).  Is it hard to arrange things so that a process
does exactly one check_repo_format() during its lifetime?

Two more issues I've been thinking about:

1. The core.repositoryformatversion scheme assumes and relies on
   that at least .git/config would stay forward compatible.  But
   if you cover unconditionally the three main entry points, I
   suspect "git-var" or "git-config-set --get" would stop
   working in a wrong repository.  I think the scripts and
   Porcelains need a way to check if we are on a repository from
   the correct vintage before running other low-level commands,
   so one of these commands probably needs to be made to work
   without check_repo_format() dying. Or we could introduce a
   new command 'git-check-repo-format' for this specific
   purpose, and special case only that one.

2. Some commands are read-only and are handy for problem
   diagnosis (e.g. cat-file), so it _might_ make sense to allow
   them to attempt running in a newer repository (which may well
   fail due to repository format difference).  I am not sure
   about the merit of doing that outweighs the complexity,
   though.  What you did covers _everything_ uniformly, and
   certainly is simpler, easier to explain, and nicer.



   

Re: [PATCH 4/6] Add check_repo_format check for all major operations.

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> Yes, although that is exactly what I said "this does not quite
> feel right" ;-).  Is it hard to arrange things so that a process
> does exactly one check_repo_format() during its lifetime?

Let's back up a bit.  I'll deal with only low-level commands
here.

First, the easiest group.  The following commands do not look at
the repository (.git directory) at all.

    check-ref-format get-tar-commit-id git index-pack mailinfo
    mailsplit patch-id shell show-index stripspace verify-pack

We do not need to do anything special about them.

The following three use enter_repo() to the given path (either
from the end user at the command line or over the network):

    daemon receive-pack upload-pack

The repo format check should be done at the same place as we
make sure enter_repo() finds the given directory a satisfactory
path.  That is, just before putenv(GIT_DIR=.) in enter_repo().

The following use setup_git_directory(), which chdir()s to the
toplevel unless GIT_DIR is set.

    cat-file config-set diff-files diff-index diff-stages diff-tree
    ls-files name-rev rev-list rev-parse show-branch symbolic-ref
    update-index update-ref

Maybe we can have a thin wrapper around setup_git_directory()
and after it returns check "${GIT_DIR-.git}" for repository
format mismatch.  What to do when GIT_DIR is set?  Then we can
just use it to read the config from "$GIT_DIR/config" and check
the version.

The following commands implicitly assume that they are either
run from the toplevel or GIT_DIR environment tells them where
the .git/ directory is:

    apply checkout-index clone-pack commit-tree convert-objects
    fetch-pack fsck-objects hash-object http-fetch http-push init-db
    local-fetch ls-tree merge-base merge-index mktag pack-objects
    pack-redundant peek-remote prune-packed read-tree send-pack
    ssh-fetch ssh-upload tar-tree unpack-file unpack-objects
    update-server-info var write-tree

With some exceptions, they are pretty much repository wide
commands, so I think it is OK for them to assume they start at
the toplevel (the Porcelain would chdir to the top for them
otherwise).  The ones that take paths, namely, checkout-index,
hash-object and ls-tree, may want to use setup_git_directory()
and do the path prefixing.

That means we would need to have some way for the rest of the
commands to check if "${GIT_DIR-.git}" is of the right format
version, and call that *once* per process invocation,
perferrably at the beginning of the main().

We need an access to .git/config file to do the repository
format check anyway, which means we need setup_git_directory()
if we ever want to run them from subdirectories.  And running
setup_git_directory() from the toplevel would not hurt, so
perhaps if we add setup_git_directory() at the beginning of
main() for the "implicity toplevel" class, and rewrite
setup_git_directory() like this:

        static const char *setup_git_directory_main(void)
        {
                /* current setup_git_driectory() */
        }

	const char *setup_git_directory(void)
        {
        	const char *retval = setup_git_directory_main();
                check_repository_format_version(); /* dies on mismatch */
		return retval;
	}

it _might_ be good enough.

I said "it _might_" here, because we need to be careful.  Right
now, if you run the "implicitly toplevel" commands from a
subdirectory, they fail.  Some Porcelains and scripts may be
relying on that and there will be consequences if things
suddenly start not to fail but do something unexpected in higher
directories.