{"thread":{"id":"48901","subject":"[PATCH 0/2] RFC ref store to repository migration","startedAt":"2018-07-17T22:49:42Z","lastAt":"2018-07-18T17:07:47Z","messageCount":6,"participants":["Stefan Beller","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"352841","messageId":"20180717224935.96397-1-sbeller@google.com","threadId":"48901","inReplyTo":null,"subject":"[PATCH 0/2] RFC ref store to repository migration","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-17T22:49:33Z","receivedAt":"2018-07-17T22:49:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Stolee said (privately):\n\n    I also ran into an issue where some of the \"arbitrary repository\"\n    patches don't fully connect. Jonathan's test demonstrated this\n    issue when I connected some things in an in-process patch \n    Work in progress: https://github.com/gitgitgadget/git/pull/11\n    Specifically: https://github.com/gitgitgadget/git/pull/11/commits/287ec6c1cd4bf4da76c05698373aee15749d011a\n    \nAnd I dislike the approach taken in\nhttps://github.com/gitgitgadget/git/pull/11/commits/287ec6c1cd4bf4da76c05698373aee15749d011a\nand would prefer another approach shown at\nhttps://github.com/stefanbeller/git/tree/object-store-convert-refstore-partial\nor in this series.\n\nThis approach doesn't have the ugliness of having the repository around twice,\ne.g.\n\n    int register_replace_ref(const char *refname, ...\n    {\n      struct repository *r = cb_data;\n      ...\n    }\n    \n    ...  \n\n    for_each_replace_ref(r, register_replace_ref, r);\n    \nwhich would iterate over the refs of the first \"r\" and have \"r\" as a call back\ndata point for register_replace_ref.\n\nThis approach also takes the gentle hint of Junio to not replace well used functions\nthroughout the whole code base (using coccinelle), but for now exposes just\none example in the second patch.\n\nThese patches have been developed on top of jt/commit-graph-per-object-store.\n\nOpinions?\n\nThanks,\nStefan   \n\nStefan Beller (2):\n  refs.c: migrate internal ref iteration to pass thru repository\n    argument\n  refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback\n\n builtin/replace.c    |  3 ++-\n refs.c               | 48 +++++++++++++++++++++++++++++++++++++-------\n refs.h               | 12 ++++++++++-\n refs/iterator.c      |  6 +++---\n refs/refs-internal.h |  5 +++--\n replace-object.c     |  3 ++-\n 6 files changed, 62 insertions(+), 15 deletions(-)\n\n-- \n2.18.0.233.g985f88cf7e-goog\n\n"},{"id":"352842","messageId":"20180717224935.96397-2-sbeller@google.com","threadId":"48901","inReplyTo":"20180717224935.96397-1-sbeller@google.com","subject":"[PATCH 1/2] refs.c: migrate internal ref iteration to pass thru repository argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-17T22:49:34Z","receivedAt":"2018-07-17T22:49:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In 60ce76d3581 (refs: add repository argument to for_each_replace_ref,\n2018-04-11) and 0d296c57aec (refs: allow for_each_replace_ref to handle\narbitrary repositories, 2018-04-11), for_each_replace_ref learned how\nto iterate over refs by a given arbitrary repository.\nNew attempts in the object store conversion have shown that it is useful\nto have the repository handle available that the refs iteration is\ncurrently iterating over.\n\nTo achieve this goal we will need to add a repository argument to\neach_ref_fn in refs.h. However as many callers rely on the signature\nsuch a patch would be too large.\n\nSo convert the internals of the ref subsystem first to pass through a\nrepository argument without exposing the change to the user. Assume\nthe_repository for the passed through repository, although it is not\nused anywhere yet.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n refs.c               | 39 +++++++++++++++++++++++++++++++++++++--\n refs.h               | 10 ++++++++++\n refs/iterator.c      |  6 +++---\n refs/refs-internal.h |  5 +++--\n 4 files changed, 53 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex fcfd3171e83..2513f77acb3 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1390,17 +1390,50 @@ struct ref_iterator *refs_ref_iterator_begin(\n  * non-zero value, stop the iteration and return that value;\n  * otherwise, return 0.\n  */\n+static int do_for_each_repo_ref(struct repository *r, const char *prefix,\n+\t\t\t\teach_repo_ref_fn fn, int trim, int flags,\n+\t\t\t\tvoid *cb_data)\n+{\n+\tstruct ref_iterator *iter;\n+\tstruct ref_store *refs = get_main_ref_store(r);\n+\n+\tif (!refs)\n+\t\treturn 0;\n+\n+\titer = refs_ref_iterator_begin(refs, prefix, trim, flags);\n+\n+\treturn do_for_each_repo_ref_iterator(r, iter, fn, cb_data);\n+}\n+\n+struct do_for_each_ref_help {\n+\teach_ref_fn *fn;\n+\tvoid *cb_data;\n+};\n+\n+static int do_for_each_ref_helper(struct repository *r,\n+\t\t\t\t  const char *refname,\n+\t\t\t\t  const struct object_id *oid,\n+\t\t\t\t  int flags,\n+\t\t\t\t  void *cb_data)\n+{\n+\tstruct do_for_each_ref_help *hp = cb_data;\n+\n+\treturn hp->fn(refname, oid, flags, hp->cb_data);\n+}\n+\n static int do_for_each_ref(struct ref_store *refs, const char *prefix,\n \t\t\t   each_ref_fn fn, int trim, int flags, void *cb_data)\n {\n \tstruct ref_iterator *iter;\n+\tstruct do_for_each_ref_help hp = { fn, cb_data };\n \n \tif (!refs)\n \t\treturn 0;\n \n \titer = refs_ref_iterator_begin(refs, prefix, trim, flags);\n \n-\treturn do_for_each_ref_iterator(iter, fn, cb_data);\n+\treturn do_for_each_repo_ref_iterator(the_repository, iter,\n+\t\t\t\t\tdo_for_each_ref_helper, &hp);\n }\n \n int refs_for_each_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n@@ -2029,10 +2062,12 @@ int refs_verify_refname_available(struct ref_store *refs,\n int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n {\n \tstruct ref_iterator *iter;\n+\tstruct do_for_each_ref_help hp = { fn, cb_data };\n \n \titer = refs->be->reflog_iterator_begin(refs);\n \n-\treturn do_for_each_ref_iterator(iter, fn, cb_data);\n+\treturn do_for_each_repo_ref_iterator(the_repository, iter,\n+\t\t\t\t\t     do_for_each_ref_helper, &hp);\n }\n \n int for_each_reflog(each_ref_fn fn, void *cb_data)\ndiff --git a/refs.h b/refs.h\nindex cc2fb4c68c0..80eec8bbc68 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -274,6 +274,16 @@ struct ref_transaction;\n typedef int each_ref_fn(const char *refname,\n \t\t\tconst struct object_id *oid, int flags, void *cb_data);\n \n+/*\n+ * The same as each_ref_fn, but also with a repository argument that\n+ * contains the repository associated with the callback.\n+ */\n+typedef int each_repo_ref_fn(struct repository *r,\n+\t\t\t     const char *refname,\n+\t\t\t     const struct object_id *oid,\n+\t\t\t     int flags,\n+\t\t\t     void *cb_data);\n+\n /*\n  * The following functions invoke the specified callback function for\n  * each reference indicated.  If the function ever returns a nonzero\ndiff --git a/refs/iterator.c b/refs/iterator.c\nindex 2ac91ac3401..629e00a122a 100644\n--- a/refs/iterator.c\n+++ b/refs/iterator.c\n@@ -407,15 +407,15 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,\n \n struct ref_iterator *current_ref_iter = NULL;\n \n-int do_for_each_ref_iterator(struct ref_iterator *iter,\n-\t\t\t     each_ref_fn fn, void *cb_data)\n+int do_for_each_repo_ref_iterator(struct repository *r, struct ref_iterator *iter,\n+\t\t\t\t  each_repo_ref_fn fn, void *cb_data)\n {\n \tint retval = 0, ok;\n \tstruct ref_iterator *old_ref_iter = current_ref_iter;\n \n \tcurrent_ref_iter = iter;\n \twhile ((ok = ref_iterator_advance(iter)) == ITER_OK) {\n-\t\tretval = fn(iter->refname, iter->oid, iter->flags, cb_data);\n+\t\tretval = fn(r, iter->refname, iter->oid, iter->flags, cb_data);\n \t\tif (retval) {\n \t\t\t/*\n \t\t\t * If ref_iterator_abort() returns ITER_ERROR,\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex dd834314bd8..5c7414bf099 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -473,8 +473,9 @@ extern struct ref_iterator *current_ref_iter;\n  * adapter between the callback style of reference iteration and the\n  * iterator style.\n  */\n-int do_for_each_ref_iterator(struct ref_iterator *iter,\n-\t\t\t     each_ref_fn fn, void *cb_data);\n+int do_for_each_repo_ref_iterator(struct repository *r,\n+\t\t\t\t  struct ref_iterator *iter,\n+\t\t\t\t  each_repo_ref_fn fn, void *cb_data);\n \n /*\n  * Only include per-worktree refs in a do_for_each_ref*() iteration.\n-- \n2.18.0.233.g985f88cf7e-goog\n\n"},{"id":"352843","messageId":"20180717224935.96397-3-sbeller@google.com","threadId":"48901","inReplyTo":"20180717224935.96397-1-sbeller@google.com","subject":"[PATCH 2/2] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-17T22:49:35Z","receivedAt":"2018-07-17T22:49:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/replace.c | 3 ++-\n refs.c            | 9 ++++-----\n refs.h            | 2 +-\n replace-object.c  | 3 ++-\n 4 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex ef22d724bbc..5f34659071f 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -39,7 +39,8 @@ struct show_data {\n \tenum replace_format format;\n };\n \n-static int show_reference(const char *refname, const struct object_id *oid,\n+static int show_reference(struct repository *r, const char *refname,\n+\t\t\t  const struct object_id *oid,\n \t\t\t  int flag, void *cb_data)\n {\n \tstruct show_data *data = cb_data;\ndiff --git a/refs.c b/refs.c\nindex 2513f77acb3..5700cd4683f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1478,12 +1478,11 @@ int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,\n \treturn do_for_each_ref(refs, prefix, fn, 0, flag, cb_data);\n }\n \n-int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data)\n+int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data)\n {\n-\treturn do_for_each_ref(get_main_ref_store(r),\n-\t\t\t       git_replace_ref_base, fn,\n-\t\t\t       strlen(git_replace_ref_base),\n-\t\t\t       DO_FOR_EACH_INCLUDE_BROKEN, cb_data);\n+\treturn do_for_each_repo_ref(r, git_replace_ref_base, fn,\n+\t\t\t\t    strlen(git_replace_ref_base),\n+\t\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)\ndiff --git a/refs.h b/refs.h\nindex 80eec8bbc68..a0a18223a14 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -317,7 +317,7 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,\n int for_each_tag_ref(each_ref_fn fn, void *cb_data);\n int for_each_branch_ref(each_ref_fn fn, void *cb_data);\n int for_each_remote_ref(each_ref_fn fn, void *cb_data);\n-int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data);\n+int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data);\n int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);\n int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,\n \t\t\t const char *prefix, void *cb_data);\ndiff --git a/replace-object.c b/replace-object.c\nindex 801b5c16789..01a5a59a35a 100644\n--- a/replace-object.c\n+++ b/replace-object.c\n@@ -6,7 +6,8 @@\n #include \"repository.h\"\n #include \"commit.h\"\n \n-static int register_replace_ref(const char *refname,\n+static int register_replace_ref(struct repository *r,\n+\t\t\t\tconst char *refname,\n \t\t\t\tconst struct object_id *oid,\n \t\t\t\tint flag, void *cb_data)\n {\n-- \n2.18.0.233.g985f88cf7e-goog\n\n"},{"id":"352855","messageId":"6c74aee8-2e18-99de-f775-dced1310809f@gmail.com","threadId":"48901","inReplyTo":"20180717224935.96397-3-sbeller@google.com","subject":"Re: [PATCH 2/2] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-18T10:58:44Z","receivedAt":"2018-07-18T10:58:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/17/2018 6:49 PM, Stefan Beller wrote:\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>   builtin/replace.c | 3 ++-\n>   refs.c            | 9 ++++-----\n>   refs.h            | 2 +-\n>   replace-object.c  | 3 ++-\n>   4 files changed, 9 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index ef22d724bbc..5f34659071f 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -39,7 +39,8 @@ struct show_data {\n>   \tenum replace_format format;\n>   };\n>   \n> -static int show_reference(const char *refname, const struct object_id *oid,\n> +static int show_reference(struct repository *r, const char *refname,\n> +\t\t\t  const struct object_id *oid,\n>   \t\t\t  int flag, void *cb_data)\n>   {\n>   \tstruct show_data *data = cb_data;\n> diff --git a/refs.c b/refs.c\n> index 2513f77acb3..5700cd4683f 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1478,12 +1478,11 @@ int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,\n>   \treturn do_for_each_ref(refs, prefix, fn, 0, flag, cb_data);\n>   }\n>   \n> -int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data)\n> +int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data)\n>   {\n> -\treturn do_for_each_ref(get_main_ref_store(r),\n> -\t\t\t       git_replace_ref_base, fn,\n> -\t\t\t       strlen(git_replace_ref_base),\n> -\t\t\t       DO_FOR_EACH_INCLUDE_BROKEN, cb_data);\n> +\treturn do_for_each_repo_ref(r, git_replace_ref_base, fn,\n> +\t\t\t\t    strlen(git_replace_ref_base),\n> +\t\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> diff --git a/refs.h b/refs.h\n> index 80eec8bbc68..a0a18223a14 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -317,7 +317,7 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,\n>   int for_each_tag_ref(each_ref_fn fn, void *cb_data);\n>   int for_each_branch_ref(each_ref_fn fn, void *cb_data);\n>   int for_each_remote_ref(each_ref_fn fn, void *cb_data);\n> -int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data);\n> +int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data);\n>   int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);\n>   int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,\n>   \t\t\t const char *prefix, void *cb_data);\n> diff --git a/replace-object.c b/replace-object.c\n> index 801b5c16789..01a5a59a35a 100644\n> --- a/replace-object.c\n> +++ b/replace-object.c\n> @@ -6,7 +6,8 @@\n>   #include \"repository.h\"\n>   #include \"commit.h\"\n>   \n> -static int register_replace_ref(const char *refname,\n> +static int register_replace_ref(struct repository *r,\n> +\t\t\t\tconst char *refname,\n>   \t\t\t\tconst struct object_id *oid,\n>   \t\t\t\tint flag, void *cb_data)\n>   {\n\nOverall, I think this is the right approach. The only problem is that \nyou're missing a few 'the_repository' to 'r' replacements in the bodies \nof show_reference and register_replace_ref.\n\nThanks,\n-Stolee\n"},{"id":"352865","messageId":"20180718134026.243823-1-dstolee@microsoft.com","threadId":"48901","inReplyTo":"20180717224935.96397-3-sbeller@google.com","subject":"[PATCH] TO-SQUASH: replace the_repository with arbitrary r","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-07-18T13:40:38Z","receivedAt":"2018-07-18T13:40:43Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n\nThis should just be squashed into PATCH 2.\n\n builtin/replace.c | 5 ++---\n replace-object.c  | 2 +-\n 2 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 5f34659071..d0b1cdb061 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -57,9 +57,8 @@ static int show_reference(struct repository *r, const char *refname,\n \t\t\tif (get_oid(refname, &object))\n \t\t\t\treturn error(\"Failed to resolve '%s' as a valid ref.\", refname);\n \n-\t\t\tobj_type = oid_object_info(the_repository, &object,\n-\t\t\t\t\t\t   NULL);\n-\t\t\trepl_type = oid_object_info(the_repository, oid, NULL);\n+\t\t\tobj_type = oid_object_info(r, &object, NULL);\n+\t\t\trepl_type = oid_object_info(r, oid, NULL);\n \n \t\t\tprintf(\"%s (%s) -> %s (%s)\\n\", refname, type_name(obj_type),\n \t\t\t       oid_to_hex(oid), type_name(repl_type));\ndiff --git a/replace-object.c b/replace-object.c\nindex 01a5a59a35..017f02f8ef 100644\n--- a/replace-object.c\n+++ b/replace-object.c\n@@ -26,7 +26,7 @@ static int register_replace_ref(struct repository *r,\n \toidcpy(&repl_obj->replacement, oid);\n \n \t/* Register new object */\n-\tif (oidmap_put(the_repository->objects->replace_map, repl_obj))\n+\tif (oidmap_put(r->objects->replace_map, repl_obj))\n \t\tdie(\"duplicate replace ref: %s\", refname);\n \n \treturn 0;\n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"352914","messageId":"CAGZ79kZyWWivaVgdCt-Zf-_3p9L5fis7ZC73pwDEGNQUfc8f+Q@mail.gmail.com","threadId":"48901","inReplyTo":"6c74aee8-2e18-99de-f775-dced1310809f@gmail.com","subject":"Re: [PATCH 2/2] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-18T17:07:33Z","receivedAt":"2018-07-18T17:07:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi Derrick,\n\n> Overall, I think this is the right approach. The only problem is that\n> you're missing a few 'the_repository' to 'r' replacements in the bodies\n> of show_reference and register_replace_ref.\n\nThanks.\n\nOriginally I had another approach, which was to convert all\ncallbacks to take a repository argument, and only if the refs\nbackend learned to propagate the correct repository the repo\nargument would be the specific repo, NULL otherwise.\n\nWith that approach it was not possible to replace all occurrences\nof the_repository with 'r', and it would also be confusing.\n\nBut as I pursued that approach at the time of writing this patch...\nI missed the conversions needed.\n\nThanks for catching them (and fixing them in the reroll that\nyou just sent out via gitgitgadget)!\n\nThanks,\nStefan\n"}]}