{"thread":{"id":"46740","subject":"[PATCH 0/4] Fixes from the per-repository object store series","startedAt":"2017-09-12T17:24:15Z","lastAt":"2017-09-18T16:52:25Z","messageCount":12,"participants":["Jonathan Nieder","Michael Haggerty","Jeff King","Richard Maw","Heiko Voigt","Junio C Hamano","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"327896","messageId":"20170912172330.GA144745@aiede.mtv.corp.google.com","threadId":"46740","inReplyTo":null,"subject":"[PATCH 0/4] Fixes from the per-repository object store series","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-12T17:23:30Z","receivedAt":"2017-09-12T17:24:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nThis is a continuation of the series at [1].  That was part 1 ---\nyou can think of this as part 0, since it contains the simplest and\nleast controversial part of the series --- each patch in this series\nis a bugfix in its own right.\n\nPatch 1 should be familiar if you reviewed the series [1].  It is\nunchanged from the patch there, except to note Peff's ack.\n\nPatches 2-4 have not yet visited the list but are fixes (or, in the\ncase of patch 4, cleanups) noticed as part of the same process of\nteaching object store code to handle multiple repositories.\n\nWe hope that splitting these out should make them easier to review\nand for people to benefit from these fixes without having to wait\nfor the rest of the series to settle.\n\nThoughts of all kinds welcome.\n\nJonathan Nieder (1):\n  pack: make packed_git_mru global a value instead of a pointer\n\nStefan Beller (3):\n  push, fetch: error out for submodule entries not pointing to commits\n  replace-objects: evaluate replacement refs without using the object\n    store\n  packed refs: pass .git dir instead of packed-refs path to init_fn\n\n builtin/pack-objects.c         |  4 ++--\n cache.h                        |  4 ++--\n packfile.c                     | 12 +++++-------\n refs.c                         |  2 +-\n refs/files-backend.c           |  4 ++--\n refs/packed-backend.c          |  4 ++--\n submodule.c                    | 33 +++++++++++++++++++++++++--------\n t/t5531-deep-submodule-push.sh | 10 ++++++++++\n 8 files changed, 49 insertions(+), 24 deletions(-)\n\n[1] https://public-inbox.org/git/20170830064634.GA153983@aiede.mtv.corp.google.com/\n"},{"id":"327897","messageId":"20170912172839.GB144745@aiede.mtv.corp.google.com","threadId":"46740","inReplyTo":"20170912172330.GA144745@aiede.mtv.corp.google.com","subject":"[PATCH 1/4] pack: make packed_git_mru global a value instead of a pointer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-12T17:28:39Z","receivedAt":"2017-09-12T17:28:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The MRU cache that keeps track of recently used packs is represented\nusing two global variables:\n\n\tstruct mru packed_git_mru_storage;\n\tstruct mru *packed_git_mru = &packed_git_mru_storage;\n\nCallers never assign to the packed_git_mru pointer, though, so we can\nsimplify by eliminating it and using &packed_git_mru_storage (renamed\nto &packed_git_mru) directly.  This variable is only used by the\npackfile subsystem, making this a relatively uninvasive change (and\nany new unadapted callers would trigger a compile error).\n\nNoticed while moving these globals to the object_store struct.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\nAcked-by: Jeff King <peff@peff.net>\n---\nUnchanged from\nhttps://public-inbox.org/git/20170830064827.GB153983@aiede.mtv.corp.google.com/.\n\nI agree with the comments there that it would be nice to make this use\nlist.h (#leftoverbits) and would be happy to read a patch doing that.\nOnce we're done adapting the object store to work with arbitrary\nrepositories we may come back to this, but I'd be happier if it's just\nalready taken care of for us.\n\n builtin/pack-objects.c |  4 ++--\n cache.h                |  4 ++--\n packfile.c             | 12 +++++-------\n 3 files changed, 9 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex a57b4f058d..f721137eaf 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1012,7 +1012,7 @@ static int want_object_in_pack(const unsigned char *sha1,\n \t\t\treturn want;\n \t}\n \n-\tfor (entry = packed_git_mru->head; entry; entry = entry->next) {\n+\tfor (entry = packed_git_mru.head; entry; entry = entry->next) {\n \t\tstruct packed_git *p = entry->item;\n \t\toff_t offset;\n \n@@ -1030,7 +1030,7 @@ static int want_object_in_pack(const unsigned char *sha1,\n \t\t\t}\n \t\t\twant = want_found_object(exclude, p);\n \t\t\tif (!exclude && want > 0)\n-\t\t\t\tmru_mark(packed_git_mru, entry);\n+\t\t\t\tmru_mark(&packed_git_mru, entry);\n \t\t\tif (want != -1)\n \t\t\t\treturn want;\n \t\t}\ndiff --git a/cache.h b/cache.h\nindex a916bc79e3..49b083ee0a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -4,6 +4,7 @@\n #include \"git-compat-util.h\"\n #include \"strbuf.h\"\n #include \"hashmap.h\"\n+#include \"mru.h\"\n #include \"advice.h\"\n #include \"gettext.h\"\n #include \"convert.h\"\n@@ -1589,8 +1590,7 @@ extern struct packed_git {\n  * A most-recently-used ordered version of the packed_git list, which can\n  * be iterated instead of packed_git (and marked via mru_mark).\n  */\n-struct mru;\n-extern struct mru *packed_git_mru;\n+extern struct mru packed_git_mru;\n \n struct pack_entry {\n \toff_t offset;\ndiff --git a/packfile.c b/packfile.c\nindex f86fa051c9..f69a5c8d60 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -40,9 +40,7 @@ static unsigned int pack_max_fds;\n static size_t peak_pack_mapped;\n static size_t pack_mapped;\n struct packed_git *packed_git;\n-\n-static struct mru packed_git_mru_storage;\n-struct mru *packed_git_mru = &packed_git_mru_storage;\n+struct mru packed_git_mru;\n \n #define SZ_FMT PRIuMAX\n static inline uintmax_t sz_fmt(size_t s) { return s; }\n@@ -861,9 +859,9 @@ static void prepare_packed_git_mru(void)\n {\n \tstruct packed_git *p;\n \n-\tmru_clear(packed_git_mru);\n+\tmru_clear(&packed_git_mru);\n \tfor (p = packed_git; p; p = p->next)\n-\t\tmru_append(packed_git_mru, p);\n+\t\tmru_append(&packed_git_mru, p);\n }\n \n static int prepare_packed_git_run_once = 0;\n@@ -1832,9 +1830,9 @@ int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n \tif (!packed_git)\n \t\treturn 0;\n \n-\tfor (p = packed_git_mru->head; p; p = p->next) {\n+\tfor (p = packed_git_mru.head; p; p = p->next) {\n \t\tif (fill_pack_entry(sha1, e, p->item)) {\n-\t\t\tmru_mark(packed_git_mru, p);\n+\t\t\tmru_mark(&packed_git_mru, p);\n \t\t\treturn 1;\n \t\t}\n \t}\n-- \n2.14.1.690.gbb1197296e\n\n"},{"id":"327898","messageId":"20170912173027.GC144745@aiede.mtv.corp.google.com","threadId":"46740","inReplyTo":"20170912172330.GA144745@aiede.mtv.corp.google.com","subject":"[PATCH 2/4] push, fetch: error out for submodule entries not pointing to commits","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-12T17:30:27Z","receivedAt":"2017-09-12T17:30:35Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Stefan Beller <sbeller@google.com>\n\nThe check_has_commit helper uses resolves a submodule entry to a\ncommit, when validating its existence. As a side effect this means\ntolerates a submodule entry pointing to a tag, which is not a valid\nsubmodule entry that git commands would know how to cope with.\n\nTighten the check to require an actual commit, not a tag pointing to a\ncommit.\n\nAlso improve the error handling when a submodule entry points to\nnon-commit (e.g., a blob) to error out instead of warning and\npretending the pointed to object doesn't exist.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n submodule.c                    | 33 +++++++++++++++++++++++++--------\n t/t5531-deep-submodule-push.sh | 10 ++++++++++\n 2 files changed, 35 insertions(+), 8 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 3cea8221e0..e0da55920d 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -767,19 +767,36 @@ static int append_oid_to_argv(const struct object_id *oid, void *data)\n \treturn 0;\n }\n \n+struct has_commit_data {\n+\tint result;\n+\tconst char *path;\n+};\n+\n static int check_has_commit(const struct object_id *oid, void *data)\n {\n-\tint *has_commit = data;\n+\tstruct has_commit_data *cb = data;\n \n-\tif (!lookup_commit_reference(oid))\n-\t\t*has_commit = 0;\n+\tenum object_type type = sha1_object_info(oid->hash, NULL);\n \n-\treturn 0;\n+\tswitch (type) {\n+\tcase OBJ_COMMIT:\n+\t\treturn 0;\n+\tcase OBJ_BAD:\n+\t\t/*\n+\t\t * Object is missing or invalid. If invalid, an error message\n+\t\t * has already been printed.\n+\t\t */\n+\t\tcb->result = 0;\n+\t\treturn 0;\n+\tdefault:\n+\t\tdie(_(\"submodule entry '%s' (%s) is a %s, not a commit\"),\n+\t\t    cb->path, oid_to_hex(oid), typename(type));\n+\t}\n }\n \n static int submodule_has_commits(const char *path, struct oid_array *commits)\n {\n-\tint has_commit = 1;\n+\tstruct has_commit_data has_commit = { 1, path };\n \n \t/*\n \t * Perform a cheap, but incorrect check for the existence of 'commits'.\n@@ -795,7 +812,7 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)\n \n \toid_array_for_each_unique(commits, check_has_commit, &has_commit);\n \n-\tif (has_commit) {\n+\tif (has_commit.result) {\n \t\t/*\n \t\t * Even if the submodule is checked out and the commit is\n \t\t * present, make sure it exists in the submodule's object store\n@@ -814,12 +831,12 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)\n \t\tcp.dir = path;\n \n \t\tif (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)\n-\t\t\thas_commit = 0;\n+\t\t\thas_commit.result = 0;\n \n \t\tstrbuf_release(&out);\n \t}\n \n-\treturn has_commit;\n+\treturn has_commit.result;\n }\n \n static int submodule_needs_pushing(const char *path, struct oid_array *commits)\ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex 0f84a53146..39cb2c1c34 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -298,6 +298,16 @@ test_expect_success 'push succeeds if submodule commit disabling recursion from\n \t)\n '\n \n+test_expect_success 'submodule entry pointing at a tag is error' '\n+\tgit -C work/gar/bage tag -a test1 -m \"tag\" &&\n+\ttag=$(git -C work/gar/bage rev-parse test1^{tag}) &&\n+\tgit -C work update-index --cacheinfo 160000 \"$tag\" gar/bage &&\n+\tgit -C work commit -m \"bad commit\" &&\n+\ttest_when_finished \"git -C work reset --hard HEAD^\" &&\n+\ttest_must_fail git -C work push --recurse-submodules=on-demand ../pub.git master 2>err &&\n+\ttest_i18ngrep \"is a tag, not a commit\" err\n+'\n+\n test_expect_success 'push fails if recurse submodules option passed as yes' '\n \t(\n \t\tcd work/gar/bage &&\n-- \n2.14.1.690.gbb1197296e\n\n"},{"id":"327899","messageId":"20170912173140.GD144745@aiede.mtv.corp.google.com","threadId":"46740","inReplyTo":"20170912172330.GA144745@aiede.mtv.corp.google.com","subject":"[PATCH 3/4] replace-objects: evaluate replacement refs without using the object store","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-12T17:31:40Z","receivedAt":"2017-09-12T17:31:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Stefan Beller <sbeller@google.com>\n\nPass DO_FOR_EACH_INCLUDE_BROKEN when iterating over replacement refs\nso that the iteration does not require opening the named objects from\nthe object store. This avoids a dependency cycle between object access\nand replace ref iteration.\n\nMoreover the ref subsystem has not been migrated yet to access the\nobject store via passed in repository objects.  As a result, without\nthis patch, iterating over replace refs in a repository other than\nthe_repository it produces errors:\n\n   error: refs/replace/3afabef75c627b894cccc3bcae86837abc7c32fe does not point to a valid object!\n\nNoticed while adapting the object store (and in particular its\nevaluation of replace refs) to handle arbitrary repositories.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n refs.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex b0106b8162..cd84ed9710 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1394,7 +1394,7 @@ int for_each_replace_ref(each_ref_fn fn, void *cb_data)\n \treturn do_for_each_ref(get_main_ref_store(),\n \t\t\t       git_replace_ref_base, fn,\n \t\t\t       strlen(git_replace_ref_base),\n-\t\t\t       0, cb_data);\n+\t\t\t       DO_FOR_EACH_INCLUDE_BROKEN, cb_data);\n }\n \n int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)\n-- \n2.14.1.690.gbb1197296e\n\n"},{"id":"327900","messageId":"20170912173234.GE144745@aiede.mtv.corp.google.com","threadId":"46740","inReplyTo":"20170912172330.GA144745@aiede.mtv.corp.google.com","subject":"[PATCH 4/4] packed refs: pass .git dir instead of packed-refs path to init_fn","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-12T17:32:34Z","receivedAt":"2017-09-12T17:32:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Stefan Beller <sbeller@google.com>\n\nThe first argument of a ref_store_init_fn is documented to represent\nthe $GIT_DIR, not the path to the packed-refs file. This brings the\npacked-refs store more in line with the usual ref store interface.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\nThat's the end of the series.  Thanks for reading.\n\n refs/files-backend.c  | 4 ++--\n refs/packed-backend.c | 4 ++--\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex fccbc24ac4..3b8e13a8b7 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -57,8 +57,8 @@ static struct ref_store *files_ref_store_create(const char *gitdir,\n \trefs->gitdir = xstrdup(gitdir);\n \tget_common_dir_noenv(&sb, gitdir);\n \trefs->gitcommondir = strbuf_detach(&sb, NULL);\n-\tstrbuf_addf(&sb, \"%s/packed-refs\", refs->gitcommondir);\n-\trefs->packed_ref_store = packed_ref_store_create(sb.buf, flags);\n+\trefs->packed_ref_store =\n+\t\t\tpacked_ref_store_create(refs->gitcommondir, flags);\n \tstrbuf_release(&sb);\n \n \treturn ref_store;\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 412c85034f..2c5db15279 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -78,7 +78,7 @@ struct packed_ref_store {\n \tstruct tempfile tempfile;\n };\n \n-struct ref_store *packed_ref_store_create(const char *path,\n+struct ref_store *packed_ref_store_create(const char *common_dir,\n \t\t\t\t\t  unsigned int store_flags)\n {\n \tstruct packed_ref_store *refs = xcalloc(1, sizeof(*refs));\n@@ -87,7 +87,7 @@ struct ref_store *packed_ref_store_create(const char *path,\n \tbase_ref_store_init(ref_store, &refs_be_packed);\n \trefs->store_flags = store_flags;\n \n-\trefs->path = xstrdup(path);\n+\trefs->path = xstrfmt(\"%s/packed-refs\", common_dir);\n \treturn ref_store;\n }\n \n-- \n2.14.1.690.gbb1197296e\n\n"},{"id":"327915","messageId":"fe956802-41ab-783f-62b3-f7c4b8292e8c@alum.mit.edu","threadId":"46740","inReplyTo":"20170912173140.GD144745@aiede.mtv.corp.google.com","subject":"Re: [PATCH 3/4] replace-objects: evaluate replacement refs without using the object store","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T08:03:58Z","receivedAt":"2017-09-13T08:04:11Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/12/2017 07:31 PM, Jonathan Nieder wrote:\n> From: Stefan Beller <sbeller@google.com>\n> \n> Pass DO_FOR_EACH_INCLUDE_BROKEN when iterating over replacement refs\n> so that the iteration does not require opening the named objects from\n> the object store. This avoids a dependency cycle between object access\n> and replace ref iteration.\n> \n> Moreover the ref subsystem has not been migrated yet to access the\n> object store via passed in repository objects.  As a result, without\n> this patch, iterating over replace refs in a repository other than\n> the_repository it produces errors:\n> \n>    error: refs/replace/3afabef75c627b894cccc3bcae86837abc7c32fe does not point to a valid object!\n> \n> Noticed while adapting the object store (and in particular its\n> evaluation of replace refs) to handle arbitrary repositories.\n\nHave you checked that consumers of this API can handle broken\nreferences? Aside from missing values, broken references can have\nillegal names (though hopefully not unsafe in the sense of causing\nfilesystem traversal outside of `$GITDIR`).\n\nMichael\n"},{"id":"327916","messageId":"827535d1-b549-8fe6-166c-23148d4290d8@alum.mit.edu","threadId":"46740","inReplyTo":"20170912173234.GE144745@aiede.mtv.corp.google.com","subject":"Re: [PATCH 4/4] packed refs: pass .git dir instead of packed-refs path to init_fn","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T08:45:45Z","receivedAt":"2017-09-13T08:45:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/12/2017 07:32 PM, Jonathan Nieder wrote:\n> From: Stefan Beller <sbeller@google.com>\n> \n> The first argument of a ref_store_init_fn is documented to represent\n> the $GIT_DIR, not the path to the packed-refs file. This brings the\n> packed-refs store more in line with the usual ref store interface.\n> \n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> That's the end of the series.  Thanks for reading.\n> \n>  refs/files-backend.c  | 4 ++--\n>  refs/packed-backend.c | 4 ++--\n>  2 files changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index fccbc24ac4..3b8e13a8b7 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -57,8 +57,8 @@ static struct ref_store *files_ref_store_create(const char *gitdir,\n>  \trefs->gitdir = xstrdup(gitdir);\n>  \tget_common_dir_noenv(&sb, gitdir);\n>  \trefs->gitcommondir = strbuf_detach(&sb, NULL);\n> -\tstrbuf_addf(&sb, \"%s/packed-refs\", refs->gitcommondir);\n> -\trefs->packed_ref_store = packed_ref_store_create(sb.buf, flags);\n> +\trefs->packed_ref_store =\n> +\t\t\tpacked_ref_store_create(refs->gitcommondir, flags);\n>  \tstrbuf_release(&sb);\n>  \n>  \treturn ref_store;\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index 412c85034f..2c5db15279 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -78,7 +78,7 @@ struct packed_ref_store {\n>  \tstruct tempfile tempfile;\n>  };\n>  \n> -struct ref_store *packed_ref_store_create(const char *path,\n> +struct ref_store *packed_ref_store_create(const char *common_dir,\n>  \t\t\t\t\t  unsigned int store_flags)\n>  {\n>  \tstruct packed_ref_store *refs = xcalloc(1, sizeof(*refs));\n> @@ -87,7 +87,7 @@ struct ref_store *packed_ref_store_create(const char *path,\n>  \tbase_ref_store_init(ref_store, &refs_be_packed);\n>  \trefs->store_flags = store_flags;\n>  \n> -\trefs->path = xstrdup(path);\n> +\trefs->path = xstrfmt(\"%s/packed-refs\", common_dir);\n>  \treturn ref_store;\n>  }\n>  \n> \n\nThis little patch, perhaps surprisingly, brings up some deeper issues.\n\nFirst of all there is a superficial naming inconsistency. This method is\ncalled `init` in the vtable, but the functions implementing it are\ncalled `*_ref_store_create()`. It actually initializes the data\nstructures for dealing with the reference store, as opposed to\ninitializing the reference store on disk (*that* virtual function is\ncalled `init_db`). It should maybe be called `open` instead.\n\nBut more fundamentally, what is a `ref_store`? Originally it always\nrepresented a *logical* place to store all of the references for a\nrepository. In that sense, it made sense to have an \"open\" method that\ntakes a `gitdir` as argument.\n\nBut its role is currently getting stretched. Now it sometimes represents\na *physical* place to store references, which might be combined with\nother physical `ref_store`s to make a logical `ref_store`. There is not\nnecessarily a 1:1 correspondence between gitdirs and physical\n`ref_store`s; more to the point you might want to initialize a physical\nrefstore that doesn't correspond to `$GITDIR`. So requiring every\n`ref_store` to have a factory function with a gitdir argument is\nprobably incorrect.\n\nFor example, a subtree has a single logical reference store, but it is\ncomposed out of three physical reference stores: the loose references in\nthe subtree's gitdir plus loose and packed references in the common gitdir.\n\nIt seems to me that we need two separate concepts:\n\n1. Create an object representing a gitdir's *logical* `ref_store`. This\n\"class method\" could always have a single gitdir as parameter. This\nwould be the method by which the repository initialization code\ninstantiates its logical `ref_store`, for example by reading the type of\nthe reference store from the repo's config, then looking up the class by\nits type, then calling its \"open\" method to create an instance.\n\n2. Create an object representing a physical `ref_store`. Different\nreference store classes might have different \"constructor\" arguments.\nFor example, if it represents a namespace within a larger reference tree\ncontained in a shared repository, its arguments might be\n`(shared_gitdir, namespace)`. (CC to Richard Maw for this angle.)\n\nSince a `packed_ref_store` is definitely a physical `ref_store`, the\nfunction that we're talking about here is the second type, so I don't\nsee a need for it to take a gitdir as argument. OTOH, the function\ncertainly doesn't belong in the vtable slot for `init`, because a\n`packed_ref_store` can't serve as a repository's logical reference store.\n\nDid you have a particular reason for changing this now, aside from\n\"consistency\"? If not, feel free to drop this patch and I'll implement\nsomething more like what I have described above when I have time.\n\nMichael\n"},{"id":"327921","messageId":"20170913111501.cosukqzf76oy65ow@sigill.intra.peff.net","threadId":"46740","inReplyTo":"fe956802-41ab-783f-62b3-f7c4b8292e8c@alum.mit.edu","subject":"Re: [PATCH 3/4] replace-objects: evaluate replacement refs without using the object store","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T11:15:01Z","receivedAt":"2017-09-13T11:15:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 13, 2017 at 10:03:58AM +0200, Michael Haggerty wrote:\n\n> On 09/12/2017 07:31 PM, Jonathan Nieder wrote:\n> > From: Stefan Beller <sbeller@google.com>\n> > \n> > Pass DO_FOR_EACH_INCLUDE_BROKEN when iterating over replacement refs\n> > so that the iteration does not require opening the named objects from\n> > the object store. This avoids a dependency cycle between object access\n> > and replace ref iteration.\n> > \n> > Moreover the ref subsystem has not been migrated yet to access the\n> > object store via passed in repository objects.  As a result, without\n> > this patch, iterating over replace refs in a repository other than\n> > the_repository it produces errors:\n> > \n> >    error: refs/replace/3afabef75c627b894cccc3bcae86837abc7c32fe does not point to a valid object!\n> > \n> > Noticed while adapting the object store (and in particular its\n> > evaluation of replace refs) to handle arbitrary repositories.\n> \n> Have you checked that consumers of this API can handle broken\n> references? Aside from missing values, broken references can have\n> illegal names (though hopefully not unsafe in the sense of causing\n> filesystem traversal outside of `$GITDIR`).\n\nIt looks like there are only two callers. One complains about\nnames that aren't 40-hex. The other (\"git replace -l\") parses the 40-hex\nin \"long\" mode, but will print anything in short mode (not that printing\nis very dangerous).\n\nI do have to wonder if for_each_replace_ref() should just do the 40-hex\nsyntactic check itself (and issue a warning for other refs). It seems\nlike that would be the point of calling for_each_replace_ref() instead\nof just for_each_ref_in(\"refs/replace\") yourself, and both of the\ncallers would benefit.\n\n-Peff\n"},{"id":"327937","messageId":"20170913140633.GB2281@mimir","threadId":"46740","inReplyTo":"827535d1-b549-8fe6-166c-23148d4290d8@alum.mit.edu","subject":"Re: [PATCH 4/4] packed refs: pass .git dir instead of packed-refs path to init_fn","fromName":"Richard Maw","fromEmail":"richard.maw@gmail.com","sentAt":"2017-09-13T14:06:45Z","receivedAt":"2017-09-13T14:07:02Z","isPatch":true,"sender":{"key":"richard.maw@gmail.com","avatar":"https://gravatar.com/avatar/ee59b32f2b88ec3ea1653e30e237c5952e6eaa8602906afd161831127745ff70?d=mp&s=160"},"body":"On Wed, Sep 13, 2017 at 10:45:45AM +0200, Michael Haggerty wrote:\n> First of all there is a superficial naming inconsistency. This method is\n> called `init` in the vtable, but the functions implementing it are\n> called `*_ref_store_create()`. It actually initializes the data\n> structures for dealing with the reference store, as opposed to\n> initializing the reference store on disk (*that* virtual function is\n> called `init_db`). It should maybe be called `open` instead.\n> \n> But more fundamentally, what is a `ref_store`? Originally it always\n> represented a *logical* place to store all of the references for a\n> repository. In that sense, it made sense to have an \"open\" method that\n> takes a `gitdir` as argument.\n> \n> But its role is currently getting stretched. Now it sometimes represents\n> a *physical* place to store references, which might be combined with\n> other physical `ref_store`s to make a logical `ref_store`. There is not\n> necessarily a 1:1 correspondence between gitdirs and physical\n> `ref_store`s; more to the point you might want to initialize a physical\n> refstore that doesn't correspond to `$GITDIR`. So requiring every\n> `ref_store` to have a factory function with a gitdir argument is\n> probably incorrect.\n>\n> For example, a subtree has a single logical reference store, but it is\n> composed out of three physical reference stores: the loose references in\n> the subtree's gitdir plus loose and packed references in the common gitdir.\n\nOne way to implement a namespacing backend would include\na loose references backend of the subtree of refs for the namespace\noverlayed with a namespace translating backend backed onto packed references,\nso there's benefits to not being strict about it being a gitdir.\n\n> It seems to me that we need two separate concepts:\n> \n> 1. Create an object representing a gitdir's *logical* `ref_store`. This\n> \"class method\" could always have a single gitdir as parameter. This\n> would be the method by which the repository initialization code\n> instantiates its logical `ref_store`, for example by reading the type of\n> the reference store from the repo's config, then looking up the class by\n> its type, then calling its \"open\" method to create an instance.\n> \n> 2. Create an object representing a physical `ref_store`. Different\n> reference store classes might have different \"constructor\" arguments.\n> For example, if it represents a namespace within a larger reference tree\n> contained in a shared repository, its arguments might be\n> `(shared_gitdir, namespace)`. (CC to Richard Maw for this angle.)\n\nThe distinction confused me while attempting to add the namespaced ref backend,\nsince I was unfamiliar with how the ref stores were meant to be defined.\nIn the end I just omitted the init function and create them separately.\n\nI'm not sure drawing the line at \"logical ref stores init\" and\n\"physical ref stores are constructed differently\" is the right division,\nsince the namespaced ref store is more of a local ref store\nand its current interface is an existing ref store and a namespace.\n\nPerhaps a better distinction would be frontend vs implementation ref stores\nwhere frontend ones are registered to be found with find_ref_storage_backend,\nwhile implementation ref stores don't use that\nand hence don't need the next, name, init or init_db.\n\nThen the physical vs logical distinction is whether they access refs directly\nor only via another backend.\n\nIt wouldn't be implausible for the namespaced ref store backend\nto be a frontend logical ref store that constructs the files ref store itself;\nI just didn't have enough of a feel of how things went together to spot that.\n"},{"id":"328046","messageId":"20170914124536.GA66700@book.hvoigt.net","threadId":"46740","inReplyTo":"20170912173027.GC144745@aiede.mtv.corp.google.com","subject":"Re: [PATCH 2/4] push, fetch: error out for submodule entries not pointing to commits","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-09-14T12:45:36Z","receivedAt":"2017-09-14T12:45:46Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Tue, Sep 12, 2017 at 10:30:27AM -0700, Jonathan Nieder wrote:\n> From: Stefan Beller <sbeller@google.com>\n> \n> The check_has_commit helper uses resolves a submodule entry to a\n> commit, when validating its existence. As a side effect this means\n> tolerates a submodule entry pointing to a tag, which is not a valid\n> submodule entry that git commands would know how to cope with.\n> \n> Tighten the check to require an actual commit, not a tag pointing to a\n> commit.\n> \n> Also improve the error handling when a submodule entry points to\n> non-commit (e.g., a blob) to error out instead of warning and\n> pretending the pointed to object doesn't exist.\n> \n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n\nLooks good to me.\n\nCheers Heiko\n"},{"id":"328093","messageId":"xmqqa81wzoqz.fsf@gitster.mtv.corp.google.com","threadId":"46740","inReplyTo":"20170912172330.GA144745@aiede.mtv.corp.google.com","subject":"Re: [PATCH 0/4] Fixes from the per-repository object store series","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-15T02:20:36Z","receivedAt":"2017-09-15T02:21:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> This is a continuation of the series at [1].  That was part 1 ---\n> you can think of this as part 0, since it contains the simplest and\n> least controversial part of the series --- each patch in this series\n> is a bugfix in its own right.\n>\n> Patch 1 should be familiar if you reviewed the series [1].  It is\n> unchanged from the patch there, except to note Peff's ack.\n>\n> Patches 2-4 have not yet visited the list but are fixes (or, in the\n> case of patch 4, cleanups) noticed as part of the same process of\n> teaching object store code to handle multiple repositories.\n>\n> We hope that splitting these out should make them easier to review\n> and for people to benefit from these fixes without having to wait\n> for the rest of the series to settle.\n\nOne thing that is unclear is if you are tentatively withdrawing the\nlonger series with this update, but I'd assume that it is the case\nbecause these two obviously will conflict with each other, and the\nmost of what these four patches do are subset of what the larger one\nyou sent earlier do.\n\nThanks.\n"},{"id":"328306","messageId":"20170918165217.GD144331@google.com","threadId":"46740","inReplyTo":"20170912173027.GC144745@aiede.mtv.corp.google.com","subject":"Re: [PATCH 2/4] push, fetch: error out for submodule entries not pointing to commits","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-09-18T16:52:17Z","receivedAt":"2017-09-18T16:52:25Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 09/12, Jonathan Nieder wrote:\n> From: Stefan Beller <sbeller@google.com>\n> \n> The check_has_commit helper uses resolves a submodule entry to a\n> commit, when validating its existence. As a side effect this means\n> tolerates a submodule entry pointing to a tag, which is not a valid\n> submodule entry that git commands would know how to cope with.\n> \n> Tighten the check to require an actual commit, not a tag pointing to a\n> commit.\n> \n> Also improve the error handling when a submodule entry points to\n> non-commit (e.g., a blob) to error out instead of warning and\n> pretending the pointed to object doesn't exist.\n> \n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n\nLooks good!\n\n> ---\n>  submodule.c                    | 33 +++++++++++++++++++++++++--------\n>  t/t5531-deep-submodule-push.sh | 10 ++++++++++\n>  2 files changed, 35 insertions(+), 8 deletions(-)\n> \n> diff --git a/submodule.c b/submodule.c\n> index 3cea8221e0..e0da55920d 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -767,19 +767,36 @@ static int append_oid_to_argv(const struct object_id *oid, void *data)\n>  \treturn 0;\n>  }\n>  \n> +struct has_commit_data {\n> +\tint result;\n> +\tconst char *path;\n> +};\n> +\n>  static int check_has_commit(const struct object_id *oid, void *data)\n>  {\n> -\tint *has_commit = data;\n> +\tstruct has_commit_data *cb = data;\n>  \n> -\tif (!lookup_commit_reference(oid))\n> -\t\t*has_commit = 0;\n> +\tenum object_type type = sha1_object_info(oid->hash, NULL);\n>  \n> -\treturn 0;\n> +\tswitch (type) {\n> +\tcase OBJ_COMMIT:\n> +\t\treturn 0;\n> +\tcase OBJ_BAD:\n> +\t\t/*\n> +\t\t * Object is missing or invalid. If invalid, an error message\n> +\t\t * has already been printed.\n> +\t\t */\n> +\t\tcb->result = 0;\n> +\t\treturn 0;\n> +\tdefault:\n> +\t\tdie(_(\"submodule entry '%s' (%s) is a %s, not a commit\"),\n> +\t\t    cb->path, oid_to_hex(oid), typename(type));\n> +\t}\n>  }\n>  \n>  static int submodule_has_commits(const char *path, struct oid_array *commits)\n>  {\n> -\tint has_commit = 1;\n> +\tstruct has_commit_data has_commit = { 1, path };\n>  \n>  \t/*\n>  \t * Perform a cheap, but incorrect check for the existence of 'commits'.\n> @@ -795,7 +812,7 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)\n>  \n>  \toid_array_for_each_unique(commits, check_has_commit, &has_commit);\n>  \n> -\tif (has_commit) {\n> +\tif (has_commit.result) {\n>  \t\t/*\n>  \t\t * Even if the submodule is checked out and the commit is\n>  \t\t * present, make sure it exists in the submodule's object store\n> @@ -814,12 +831,12 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)\n>  \t\tcp.dir = path;\n>  \n>  \t\tif (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)\n> -\t\t\thas_commit = 0;\n> +\t\t\thas_commit.result = 0;\n>  \n>  \t\tstrbuf_release(&out);\n>  \t}\n>  \n> -\treturn has_commit;\n> +\treturn has_commit.result;\n>  }\n>  \n>  static int submodule_needs_pushing(const char *path, struct oid_array *commits)\n> diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\n> index 0f84a53146..39cb2c1c34 100755\n> --- a/t/t5531-deep-submodule-push.sh\n> +++ b/t/t5531-deep-submodule-push.sh\n> @@ -298,6 +298,16 @@ test_expect_success 'push succeeds if submodule commit disabling recursion from\n>  \t)\n>  '\n>  \n> +test_expect_success 'submodule entry pointing at a tag is error' '\n> +\tgit -C work/gar/bage tag -a test1 -m \"tag\" &&\n> +\ttag=$(git -C work/gar/bage rev-parse test1^{tag}) &&\n> +\tgit -C work update-index --cacheinfo 160000 \"$tag\" gar/bage &&\n> +\tgit -C work commit -m \"bad commit\" &&\n> +\ttest_when_finished \"git -C work reset --hard HEAD^\" &&\n> +\ttest_must_fail git -C work push --recurse-submodules=on-demand ../pub.git master 2>err &&\n> +\ttest_i18ngrep \"is a tag, not a commit\" err\n> +'\n> +\n>  test_expect_success 'push fails if recurse submodules option passed as yes' '\n>  \t(\n>  \t\tcd work/gar/bage &&\n> -- \n> 2.14.1.690.gbb1197296e\n> \n\n-- \nBrandon Williams\n"}]}