[PATCH 0/4] Fixes from the per-repository object store series

[PATCH 0/4] Fixes from the per-repository object store series

[Jonathan Nieder]

Hi,

This is a continuation of the series at [1].  That was part 1 ---
you can think of this as part 0, since it contains the simplest and
least controversial part of the series --- each patch in this series
is a bugfix in its own right.

Patch 1 should be familiar if you reviewed the series [1].  It is
unchanged from the patch there, except to note Peff's ack.

Patches 2-4 have not yet visited the list but are fixes (or, in the
case of patch 4, cleanups) noticed as part of the same process of
teaching object store code to handle multiple repositories.

We hope that splitting these out should make them easier to review
and for people to benefit from these fixes without having to wait
for the rest of the series to settle.

Thoughts of all kinds welcome.

Jonathan Nieder (1):
  pack: make packed_git_mru global a value instead of a pointer

Stefan Beller (3):
  push, fetch: error out for submodule entries not pointing to commits
  replace-objects: evaluate replacement refs without using the object
    store
  packed refs: pass .git dir instead of packed-refs path to init_fn

 builtin/pack-objects.c         |  4 ++--
 cache.h                        |  4 ++--
 packfile.c                     | 12 +++++-------
 refs.c                         |  2 +-
 refs/files-backend.c           |  4 ++--
 refs/packed-backend.c          |  4 ++--
 submodule.c                    | 33 +++++++++++++++++++++++++--------
 t/t5531-deep-submodule-push.sh | 10 ++++++++++
 8 files changed, 49 insertions(+), 24 deletions(-)

[1] https://public-inbox.org/git/20170830064634.GA153983@aiede.mtv.corp.google.com/

[PATCH 1/4] pack: make packed_git_mru global a value instead of a pointer

[Jonathan Nieder]

The MRU cache that keeps track of recently used packs is represented
using two global variables:

	struct mru packed_git_mru_storage;
	struct mru *packed_git_mru = &packed_git_mru_storage;

Callers never assign to the packed_git_mru pointer, though, so we can
simplify by eliminating it and using &packed_git_mru_storage (renamed
to &packed_git_mru) directly.  This variable is only used by the
packfile subsystem, making this a relatively uninvasive change (and
any new unadapted callers would trigger a compile error).

Noticed while moving these globals to the object_store struct.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Acked-by: Jeff King <peff@peff.net>
---
Unchanged from
https://public-inbox.org/git/20170830064827.GB153983@aiede.mtv.corp.google.com/.

I agree with the comments there that it would be nice to make this use
list.h (#leftoverbits) and would be happy to read a patch doing that.
Once we're done adapting the object store to work with arbitrary
repositories we may come back to this, but I'd be happier if it's just
already taken care of for us.

 builtin/pack-objects.c |  4 ++--
 cache.h                |  4 ++--
 packfile.c             | 12 +++++-------
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index a57b4f058d..f721137eaf 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1012,7 +1012,7 @@ static int want_object_in_pack(const unsigned char *sha1,
 			return want;
 	}
 
-	for (entry = packed_git_mru->head; entry; entry = entry->next) {
+	for (entry = packed_git_mru.head; entry; entry = entry->next) {
 		struct packed_git *p = entry->item;
 		off_t offset;
 
@@ -1030,7 +1030,7 @@ static int want_object_in_pack(const unsigned char *sha1,
 			}
 			want = want_found_object(exclude, p);
 			if (!exclude && want > 0)
-				mru_mark(packed_git_mru, entry);
+				mru_mark(&packed_git_mru, entry);
 			if (want != -1)
 				return want;
 		}
diff --git a/cache.h b/cache.h
index a916bc79e3..49b083ee0a 100644
--- a/cache.h
+++ b/cache.h
@@ -4,6 +4,7 @@
 #include "git-compat-util.h"
 #include "strbuf.h"
 #include "hashmap.h"
+#include "mru.h"
 #include "advice.h"
 #include "gettext.h"
 #include "convert.h"
@@ -1589,8 +1590,7 @@ extern struct packed_git {
  * A most-recently-used ordered version of the packed_git list, which can
  * be iterated instead of packed_git (and marked via mru_mark).
  */
-struct mru;
-extern struct mru *packed_git_mru;
+extern struct mru packed_git_mru;
 
 struct pack_entry {
 	off_t offset;
diff --git a/packfile.c b/packfile.c
index f86fa051c9..f69a5c8d60 100644
--- a/packfile.c
+++ b/packfile.c
@@ -40,9 +40,7 @@ static unsigned int pack_max_fds;
 static size_t peak_pack_mapped;
 static size_t pack_mapped;
 struct packed_git *packed_git;
-
-static struct mru packed_git_mru_storage;
-struct mru *packed_git_mru = &packed_git_mru_storage;
+struct mru packed_git_mru;
 
 #define SZ_FMT PRIuMAX
 static inline uintmax_t sz_fmt(size_t s) { return s; }
@@ -861,9 +859,9 @@ static void prepare_packed_git_mru(void)
 {
 	struct packed_git *p;
 
-	mru_clear(packed_git_mru);
+	mru_clear(&packed_git_mru);
 	for (p = packed_git; p; p = p->next)
-		mru_append(packed_git_mru, p);
+		mru_append(&packed_git_mru, p);
 }
 
 static int prepare_packed_git_run_once = 0;
@@ -1832,9 +1830,9 @@ int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
 	if (!packed_git)
 		return 0;
 
-	for (p = packed_git_mru->head; p; p = p->next) {
+	for (p = packed_git_mru.head; p; p = p->next) {
 		if (fill_pack_entry(sha1, e, p->item)) {
-			mru_mark(packed_git_mru, p);
+			mru_mark(&packed_git_mru, p);
 			return 1;
 		}
 	}
-- 
2.14.1.690.gbb1197296e

[PATCH 2/4] push, fetch: error out for submodule entries not pointing to commits

[Jonathan Nieder]

From: Stefan Beller <sbeller@google.com>

The check_has_commit helper uses resolves a submodule entry to a
commit, when validating its existence. As a side effect this means
tolerates a submodule entry pointing to a tag, which is not a valid
submodule entry that git commands would know how to cope with.

Tighten the check to require an actual commit, not a tag pointing to a
commit.

Also improve the error handling when a submodule entry points to
non-commit (e.g., a blob) to error out instead of warning and
pretending the pointed to object doesn't exist.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 submodule.c                    | 33 +++++++++++++++++++++++++--------
 t/t5531-deep-submodule-push.sh | 10 ++++++++++
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/submodule.c b/submodule.c
index 3cea8221e0..e0da55920d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -767,19 +767,36 @@ static int append_oid_to_argv(const struct object_id *oid, void *data)
 	return 0;
 }
 
+struct has_commit_data {
+	int result;
+	const char *path;
+};
+
 static int check_has_commit(const struct object_id *oid, void *data)
 {
-	int *has_commit = data;
+	struct has_commit_data *cb = data;
 
-	if (!lookup_commit_reference(oid))
-		*has_commit = 0;
+	enum object_type type = sha1_object_info(oid->hash, NULL);
 
-	return 0;
+	switch (type) {
+	case OBJ_COMMIT:
+		return 0;
+	case OBJ_BAD:
+		/*
+		 * Object is missing or invalid. If invalid, an error message
+		 * has already been printed.
+		 */
+		cb->result = 0;
+		return 0;
+	default:
+		die(_("submodule entry '%s' (%s) is a %s, not a commit"),
+		    cb->path, oid_to_hex(oid), typename(type));
+	}
 }
 
 static int submodule_has_commits(const char *path, struct oid_array *commits)
 {
-	int has_commit = 1;
+	struct has_commit_data has_commit = { 1, path };
 
 	/*
 	 * Perform a cheap, but incorrect check for the existence of 'commits'.
@@ -795,7 +812,7 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)
 
 	oid_array_for_each_unique(commits, check_has_commit, &has_commit);
 
-	if (has_commit) {
+	if (has_commit.result) {
 		/*
 		 * Even if the submodule is checked out and the commit is
 		 * present, make sure it exists in the submodule's object store
@@ -814,12 +831,12 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)
 		cp.dir = path;
 
 		if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)
-			has_commit = 0;
+			has_commit.result = 0;
 
 		strbuf_release(&out);
 	}
 
-	return has_commit;
+	return has_commit.result;
 }
 
 static int submodule_needs_pushing(const char *path, struct oid_array *commits)
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 0f84a53146..39cb2c1c34 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -298,6 +298,16 @@ test_expect_success 'push succeeds if submodule commit disabling recursion from
 	)
 '
 
+test_expect_success 'submodule entry pointing at a tag is error' '
+	git -C work/gar/bage tag -a test1 -m "tag" &&
+	tag=$(git -C work/gar/bage rev-parse test1^{tag}) &&
+	git -C work update-index --cacheinfo 160000 "$tag" gar/bage &&
+	git -C work commit -m "bad commit" &&
+	test_when_finished "git -C work reset --hard HEAD^" &&
+	test_must_fail git -C work push --recurse-submodules=on-demand ../pub.git master 2>err &&
+	test_i18ngrep "is a tag, not a commit" err
+'
+
 test_expect_success 'push fails if recurse submodules option passed as yes' '
 	(
 		cd work/gar/bage &&
-- 
2.14.1.690.gbb1197296e

Re: [PATCH 2/4] push, fetch: error out for submodule entries not pointing to commits

[Heiko Voigt]

On Tue, Sep 12, 2017 at 10:30:27AM -0700, Jonathan Nieder wrote:
> From: Stefan Beller <sbeller@google.com>
> 
> The check_has_commit helper uses resolves a submodule entry to a
> commit, when validating its existence. As a side effect this means
> tolerates a submodule entry pointing to a tag, which is not a valid
> submodule entry that git commands would know how to cope with.
> 
> Tighten the check to require an actual commit, not a tag pointing to a
> commit.
> 
> Also improve the error handling when a submodule entry points to
> non-commit (e.g., a blob) to error out instead of warning and
> pretending the pointed to object doesn't exist.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Looks good to me.

Cheers Heiko

Re: [PATCH 2/4] push, fetch: error out for submodule entries not pointing to commits

[Brandon Williams]

On 09/12, Jonathan Nieder wrote:
> From: Stefan Beller <sbeller@google.com>
> 
> The check_has_commit helper uses resolves a submodule entry to a
> commit, when validating its existence. As a side effect this means
> tolerates a submodule entry pointing to a tag, which is not a valid
> submodule entry that git commands would know how to cope with.
> 
> Tighten the check to require an actual commit, not a tag pointing to a
> commit.
> 
> Also improve the error handling when a submodule entry points to
> non-commit (e.g., a blob) to error out instead of warning and
> pretending the pointed to object doesn't exist.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Looks good!

> ---
>  submodule.c                    | 33 +++++++++++++++++++++++++--------
>  t/t5531-deep-submodule-push.sh | 10 ++++++++++
>  2 files changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/submodule.c b/submodule.c
> index 3cea8221e0..e0da55920d 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -767,19 +767,36 @@ static int append_oid_to_argv(const struct object_id *oid, void *data)
>  	return 0;
>  }
>  
> +struct has_commit_data {
> +	int result;
> +	const char *path;
> +};
> +
>  static int check_has_commit(const struct object_id *oid, void *data)
>  {
> -	int *has_commit = data;
> +	struct has_commit_data *cb = data;
>  
> -	if (!lookup_commit_reference(oid))
> -		*has_commit = 0;
> +	enum object_type type = sha1_object_info(oid->hash, NULL);
>  
> -	return 0;
> +	switch (type) {
> +	case OBJ_COMMIT:
> +		return 0;
> +	case OBJ_BAD:
> +		/*
> +		 * Object is missing or invalid. If invalid, an error message
> +		 * has already been printed.
> +		 */
> +		cb->result = 0;
> +		return 0;
> +	default:
> +		die(_("submodule entry '%s' (%s) is a %s, not a commit"),
> +		    cb->path, oid_to_hex(oid), typename(type));
> +	}
>  }
>  
>  static int submodule_has_commits(const char *path, struct oid_array *commits)
>  {
> -	int has_commit = 1;
> +	struct has_commit_data has_commit = { 1, path };
>  
>  	/*
>  	 * Perform a cheap, but incorrect check for the existence of 'commits'.
> @@ -795,7 +812,7 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)
>  
>  	oid_array_for_each_unique(commits, check_has_commit, &has_commit);
>  
> -	if (has_commit) {
> +	if (has_commit.result) {
>  		/*
>  		 * Even if the submodule is checked out and the commit is
>  		 * present, make sure it exists in the submodule's object store
> @@ -814,12 +831,12 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)
>  		cp.dir = path;
>  
>  		if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)
> -			has_commit = 0;
> +			has_commit.result = 0;
>  
>  		strbuf_release(&out);
>  	}
>  
> -	return has_commit;
> +	return has_commit.result;
>  }
>  
>  static int submodule_needs_pushing(const char *path, struct oid_array *commits)
> diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
> index 0f84a53146..39cb2c1c34 100755
> --- a/t/t5531-deep-submodule-push.sh
> +++ b/t/t5531-deep-submodule-push.sh
> @@ -298,6 +298,16 @@ test_expect_success 'push succeeds if submodule commit disabling recursion from
>  	)
>  '
>  
> +test_expect_success 'submodule entry pointing at a tag is error' '
> +	git -C work/gar/bage tag -a test1 -m "tag" &&
> +	tag=$(git -C work/gar/bage rev-parse test1^{tag}) &&
> +	git -C work update-index --cacheinfo 160000 "$tag" gar/bage &&
> +	git -C work commit -m "bad commit" &&
> +	test_when_finished "git -C work reset --hard HEAD^" &&
> +	test_must_fail git -C work push --recurse-submodules=on-demand ../pub.git master 2>err &&
> +	test_i18ngrep "is a tag, not a commit" err
> +'
> +
>  test_expect_success 'push fails if recurse submodules option passed as yes' '
>  	(
>  		cd work/gar/bage &&
> -- 
> 2.14.1.690.gbb1197296e
> 

-- 
Brandon Williams

[PATCH 3/4] replace-objects: evaluate replacement refs without using the object store

[Jonathan Nieder]

From: Stefan Beller <sbeller@google.com>

Pass DO_FOR_EACH_INCLUDE_BROKEN when iterating over replacement refs
so that the iteration does not require opening the named objects from
the object store. This avoids a dependency cycle between object access
and replace ref iteration.

Moreover the ref subsystem has not been migrated yet to access the
object store via passed in repository objects.  As a result, without
this patch, iterating over replace refs in a repository other than
the_repository it produces errors:

   error: refs/replace/3afabef75c627b894cccc3bcae86837abc7c32fe does not point to a valid object!

Noticed while adapting the object store (and in particular its
evaluation of replace refs) to handle arbitrary repositories.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index b0106b8162..cd84ed9710 100644
--- a/refs.c
+++ b/refs.c
@@ -1394,7 +1394,7 @@ int for_each_replace_ref(each_ref_fn fn, void *cb_data)
 	return do_for_each_ref(get_main_ref_store(),
 			       git_replace_ref_base, fn,
 			       strlen(git_replace_ref_base),
-			       0, cb_data);
+			       DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
 }
 
 int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)
-- 
2.14.1.690.gbb1197296e

Re: [PATCH 3/4] replace-objects: evaluate replacement refs without using the object store

[Michael Haggerty]

On 09/12/2017 07:31 PM, Jonathan Nieder wrote:
> From: Stefan Beller <sbeller@google.com>
> 
> Pass DO_FOR_EACH_INCLUDE_BROKEN when iterating over replacement refs
> so that the iteration does not require opening the named objects from
> the object store. This avoids a dependency cycle between object access
> and replace ref iteration.
> 
> Moreover the ref subsystem has not been migrated yet to access the
> object store via passed in repository objects.  As a result, without
> this patch, iterating over replace refs in a repository other than
> the_repository it produces errors:
> 
>    error: refs/replace/3afabef75c627b894cccc3bcae86837abc7c32fe does not point to a valid object!
> 
> Noticed while adapting the object store (and in particular its
> evaluation of replace refs) to handle arbitrary repositories.

Have you checked that consumers of this API can handle broken
references? Aside from missing values, broken references can have
illegal names (though hopefully not unsafe in the sense of causing
filesystem traversal outside of `$GITDIR`).

Michael

Re: [PATCH 3/4] replace-objects: evaluate replacement refs without using the object store

[Jeff King]

On Wed, Sep 13, 2017 at 10:03:58AM +0200, Michael Haggerty wrote:

> On 09/12/2017 07:31 PM, Jonathan Nieder wrote:
> > From: Stefan Beller <sbeller@google.com>
> > 
> > Pass DO_FOR_EACH_INCLUDE_BROKEN when iterating over replacement refs
> > so that the iteration does not require opening the named objects from
> > the object store. This avoids a dependency cycle between object access
> > and replace ref iteration.
> > 
> > Moreover the ref subsystem has not been migrated yet to access the
> > object store via passed in repository objects.  As a result, without
> > this patch, iterating over replace refs in a repository other than
> > the_repository it produces errors:
> > 
> >    error: refs/replace/3afabef75c627b894cccc3bcae86837abc7c32fe does not point to a valid object!
> > 
> > Noticed while adapting the object store (and in particular its
> > evaluation of replace refs) to handle arbitrary repositories.
> 
> Have you checked that consumers of this API can handle broken
> references? Aside from missing values, broken references can have
> illegal names (though hopefully not unsafe in the sense of causing
> filesystem traversal outside of `$GITDIR`).

It looks like there are only two callers. One complains about
names that aren't 40-hex. The other ("git replace -l") parses the 40-hex
in "long" mode, but will print anything in short mode (not that printing
is very dangerous).

I do have to wonder if for_each_replace_ref() should just do the 40-hex
syntactic check itself (and issue a warning for other refs). It seems
like that would be the point of calling for_each_replace_ref() instead
of just for_each_ref_in("refs/replace") yourself, and both of the
callers would benefit.

-Peff

[PATCH 4/4] packed refs: pass .git dir instead of packed-refs path to init_fn

[Jonathan Nieder]

From: Stefan Beller <sbeller@google.com>

The first argument of a ref_store_init_fn is documented to represent
the $GIT_DIR, not the path to the packed-refs file. This brings the
packed-refs store more in line with the usual ref store interface.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
That's the end of the series.  Thanks for reading.

 refs/files-backend.c  | 4 ++--
 refs/packed-backend.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index fccbc24ac4..3b8e13a8b7 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -57,8 +57,8 @@ static struct ref_store *files_ref_store_create(const char *gitdir,
 	refs->gitdir = xstrdup(gitdir);
 	get_common_dir_noenv(&sb, gitdir);
 	refs->gitcommondir = strbuf_detach(&sb, NULL);
-	strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir);
-	refs->packed_ref_store = packed_ref_store_create(sb.buf, flags);
+	refs->packed_ref_store =
+			packed_ref_store_create(refs->gitcommondir, flags);
 	strbuf_release(&sb);
 
 	return ref_store;
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 412c85034f..2c5db15279 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -78,7 +78,7 @@ struct packed_ref_store {
 	struct tempfile tempfile;
 };
 
-struct ref_store *packed_ref_store_create(const char *path,
+struct ref_store *packed_ref_store_create(const char *common_dir,
 					  unsigned int store_flags)
 {
 	struct packed_ref_store *refs = xcalloc(1, sizeof(*refs));
@@ -87,7 +87,7 @@ struct ref_store *packed_ref_store_create(const char *path,
 	base_ref_store_init(ref_store, &refs_be_packed);
 	refs->store_flags = store_flags;
 
-	refs->path = xstrdup(path);
+	refs->path = xstrfmt("%s/packed-refs", common_dir);
 	return ref_store;
 }
 
-- 
2.14.1.690.gbb1197296e

Re: [PATCH 4/4] packed refs: pass .git dir instead of packed-refs path to init_fn

[Michael Haggerty]

On 09/12/2017 07:32 PM, Jonathan Nieder wrote:
> From: Stefan Beller <sbeller@google.com>
> 
> The first argument of a ref_store_init_fn is documented to represent
> the $GIT_DIR, not the path to the packed-refs file. This brings the
> packed-refs store more in line with the usual ref store interface.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> That's the end of the series.  Thanks for reading.
> 
>  refs/files-backend.c  | 4 ++--
>  refs/packed-backend.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index fccbc24ac4..3b8e13a8b7 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -57,8 +57,8 @@ static struct ref_store *files_ref_store_create(const char *gitdir,
>  	refs->gitdir = xstrdup(gitdir);
>  	get_common_dir_noenv(&sb, gitdir);
>  	refs->gitcommondir = strbuf_detach(&sb, NULL);
> -	strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir);
> -	refs->packed_ref_store = packed_ref_store_create(sb.buf, flags);
> +	refs->packed_ref_store =
> +			packed_ref_store_create(refs->gitcommondir, flags);
>  	strbuf_release(&sb);
>  
>  	return ref_store;
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 412c85034f..2c5db15279 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -78,7 +78,7 @@ struct packed_ref_store {
>  	struct tempfile tempfile;
>  };
>  
> -struct ref_store *packed_ref_store_create(const char *path,
> +struct ref_store *packed_ref_store_create(const char *common_dir,
>  					  unsigned int store_flags)
>  {
>  	struct packed_ref_store *refs = xcalloc(1, sizeof(*refs));
> @@ -87,7 +87,7 @@ struct ref_store *packed_ref_store_create(const char *path,
>  	base_ref_store_init(ref_store, &refs_be_packed);
>  	refs->store_flags = store_flags;
>  
> -	refs->path = xstrdup(path);
> +	refs->path = xstrfmt("%s/packed-refs", common_dir);
>  	return ref_store;
>  }
>  
> 

This little patch, perhaps surprisingly, brings up some deeper issues.

First of all there is a superficial naming inconsistency. This method is
called `init` in the vtable, but the functions implementing it are
called `*_ref_store_create()`. It actually initializes the data
structures for dealing with the reference store, as opposed to
initializing the reference store on disk (*that* virtual function is
called `init_db`). It should maybe be called `open` instead.

But more fundamentally, what is a `ref_store`? Originally it always
represented a *logical* place to store all of the references for a
repository. In that sense, it made sense to have an "open" method that
takes a `gitdir` as argument.

But its role is currently getting stretched. Now it sometimes represents
a *physical* place to store references, which might be combined with
other physical `ref_store`s to make a logical `ref_store`. There is not
necessarily a 1:1 correspondence between gitdirs and physical
`ref_store`s; more to the point you might want to initialize a physical
refstore that doesn't correspond to `$GITDIR`. So requiring every
`ref_store` to have a factory function with a gitdir argument is
probably incorrect.

For example, a subtree has a single logical reference store, but it is
composed out of three physical reference stores: the loose references in
the subtree's gitdir plus loose and packed references in the common gitdir.

It seems to me that we need two separate concepts:

1. Create an object representing a gitdir's *logical* `ref_store`. This
"class method" could always have a single gitdir as parameter. This
would be the method by which the repository initialization code
instantiates its logical `ref_store`, for example by reading the type of
the reference store from the repo's config, then looking up the class by
its type, then calling its "open" method to create an instance.

2. Create an object representing a physical `ref_store`. Different
reference store classes might have different "constructor" arguments.
For example, if it represents a namespace within a larger reference tree
contained in a shared repository, its arguments might be
`(shared_gitdir, namespace)`. (CC to Richard Maw for this angle.)

Since a `packed_ref_store` is definitely a physical `ref_store`, the
function that we're talking about here is the second type, so I don't
see a need for it to take a gitdir as argument. OTOH, the function
certainly doesn't belong in the vtable slot for `init`, because a
`packed_ref_store` can't serve as a repository's logical reference store.

Did you have a particular reason for changing this now, aside from
"consistency"? If not, feel free to drop this patch and I'll implement
something more like what I have described above when I have time.

Michael

Re: [PATCH 4/4] packed refs: pass .git dir instead of packed-refs path to init_fn

[Richard Maw]

On Wed, Sep 13, 2017 at 10:45:45AM +0200, Michael Haggerty wrote:
> First of all there is a superficial naming inconsistency. This method is
> called `init` in the vtable, but the functions implementing it are
> called `*_ref_store_create()`. It actually initializes the data
> structures for dealing with the reference store, as opposed to
> initializing the reference store on disk (*that* virtual function is
> called `init_db`). It should maybe be called `open` instead.
> 
> But more fundamentally, what is a `ref_store`? Originally it always
> represented a *logical* place to store all of the references for a
> repository. In that sense, it made sense to have an "open" method that
> takes a `gitdir` as argument.
> 
> But its role is currently getting stretched. Now it sometimes represents
> a *physical* place to store references, which might be combined with
> other physical `ref_store`s to make a logical `ref_store`. There is not
> necessarily a 1:1 correspondence between gitdirs and physical
> `ref_store`s; more to the point you might want to initialize a physical
> refstore that doesn't correspond to `$GITDIR`. So requiring every
> `ref_store` to have a factory function with a gitdir argument is
> probably incorrect.
>
> For example, a subtree has a single logical reference store, but it is
> composed out of three physical reference stores: the loose references in
> the subtree's gitdir plus loose and packed references in the common gitdir.

One way to implement a namespacing backend would include
a loose references backend of the subtree of refs for the namespace
overlayed with a namespace translating backend backed onto packed references,
so there's benefits to not being strict about it being a gitdir.

> It seems to me that we need two separate concepts:
> 
> 1. Create an object representing a gitdir's *logical* `ref_store`. This
> "class method" could always have a single gitdir as parameter. This
> would be the method by which the repository initialization code
> instantiates its logical `ref_store`, for example by reading the type of
> the reference store from the repo's config, then looking up the class by
> its type, then calling its "open" method to create an instance.
> 
> 2. Create an object representing a physical `ref_store`. Different
> reference store classes might have different "constructor" arguments.
> For example, if it represents a namespace within a larger reference tree
> contained in a shared repository, its arguments might be
> `(shared_gitdir, namespace)`. (CC to Richard Maw for this angle.)

The distinction confused me while attempting to add the namespaced ref backend,
since I was unfamiliar with how the ref stores were meant to be defined.
In the end I just omitted the init function and create them separately.

I'm not sure drawing the line at "logical ref stores init" and
"physical ref stores are constructed differently" is the right division,
since the namespaced ref store is more of a local ref store
and its current interface is an existing ref store and a namespace.

Perhaps a better distinction would be frontend vs implementation ref stores
where frontend ones are registered to be found with find_ref_storage_backend,
while implementation ref stores don't use that
and hence don't need the next, name, init or init_db.

Then the physical vs logical distinction is whether they access refs directly
or only via another backend.

It wouldn't be implausible for the namespaced ref store backend
to be a frontend logical ref store that constructs the files ref store itself;
I just didn't have enough of a feel of how things went together to spot that.

Re: [PATCH 0/4] Fixes from the per-repository object store series

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> This is a continuation of the series at [1].  That was part 1 ---
> you can think of this as part 0, since it contains the simplest and
> least controversial part of the series --- each patch in this series
> is a bugfix in its own right.
>
> Patch 1 should be familiar if you reviewed the series [1].  It is
> unchanged from the patch there, except to note Peff's ack.
>
> Patches 2-4 have not yet visited the list but are fixes (or, in the
> case of patch 4, cleanups) noticed as part of the same process of
> teaching object store code to handle multiple repositories.
>
> We hope that splitting these out should make them easier to review
> and for people to benefit from these fixes without having to wait
> for the rest of the series to settle.

One thing that is unclear is if you are tentatively withdrawing the
longer series with this update, but I'd assume that it is the case
because these two obviously will conflict with each other, and the
most of what these four patches do are subset of what the larger one
you sent earlier do.

Thanks.