RFC ref store to repository migration
cover RFC ref store to repository migration
Stolee said (privately):
I also ran into an issue where some of the "arbitrary repository"
patches don't fully connect. Jonathan's test demonstrated this
issue when I connected some things in an in-process patch
Work in progress: https://github.com/gitgitgadget/git/pull/11
Specifically: https://github.com/gitgitgadget/git/pull/11/commits/287ec6c1cd4bf4da76c05698373aee15749d011a
And I dislike the approach taken in
https://github.com/gitgitgadget/git/pull/11/commits/287ec6c1cd4bf4da76c05698373aee15749d011a
and would prefer another approach shown at
https://github.com/stefanbeller/git/tree/object-store-convert-refstore-partial
or in this series.
This approach doesn't have the ugliness of having the repository around twice, e.g.
int register_replace_ref(const char *refname, ...
{
struct repository *r = cb_data;
...
}
...
for_each_replace_ref(r, register_replace_ref, r);which would iterate over the refs of the first "r" and have "r" as a call back data point for register_replace_ref.
This approach also takes the gentle hint of Junio to not replace well used functions throughout the whole code base (using coccinelle), but for now exposes just one example in the second patch.
These patches have been developed on top of jt/commit-graph-per-object-store.
Opinions?
Thanks,
Stefan
Stefan Beller (2):
refs.c: migrate internal ref iteration to pass thru repository
argument
refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback
builtin/replace.c | 3 ++- refs.c | 48 +++++++++++++++++++++++++++++++++++++------- refs.h | 12 ++++++++++- refs/iterator.c | 6 +++--- refs/refs-internal.h | 5 +++-- replace-object.c | 3 ++- 6 files changed, 62 insertions(+), 15 deletions(-)
Signature · 1 line
2.18.0.233.g985f88cf7e-goog
PATCH 1/2 refs.c: migrate internal ref iteration to pass thru repository argument
In 60ce76d3581 (refs: add repository argument to for_each_replace_ref, 2018-04-11) and 0d296c57aec (refs: allow for_each_replace_ref to handle arbitrary repositories, 2018-04-11), for_each_replace_ref learned how to iterate over refs by a given arbitrary repository.
New attempts in the object store conversion have shown that it is useful to have the repository handle available that the refs iteration is currently iterating over.
To achieve this goal we will need to add a repository argument to each_ref_fn in refs.h. However as many callers rely on the signature such a patch would be too large.
So convert the internals of the ref subsystem first to pass through a repository argument without exposing the change to the user. Assume the_repository for the passed through repository, although it is not used anywhere yet.
--- refs.c | 39 +++++++++++++++++++++++++++++++++++++-- refs.h | 10 ++++++++++ refs/iterator.c | 6 +++--- refs/refs-internal.h | 5 +++-- 4 files changed, 53 insertions(+), 7 deletions(-)
refs.c +37 -2
@@ -1390,17 +1390,50 @@ struct ref_iterator *refs_ref_iterator_begin(* non-zero value, stop the iteration and return that value; * otherwise, return 0. */ +static int do_for_each_repo_ref (struct repository *r,const char *prefix, +each_repo_ref_fn fn,int trim,int flags, +void *cb_data) +{ +struct ref_iterator *iter; +struct ref_store *refs =get_main_ref_store (r); + +if (!refs) +return 0 ; + + iter =refs_ref_iterator_begin (refs, prefix, trim, flags); + +return do_for_each_repo_ref_iterator (r, iter, fn, cb_data); +} + +struct do_for_each_ref_help { +each_ref_fn *fn; +void *cb_data; +}; + +static int do_for_each_ref_helper (struct repository *r, +const char *refname, +const struct object_id *oid, +int flags, +void *cb_data) +{ +struct do_for_each_ref_help *hp = cb_data; + +return hp->fn (refname, oid, flags, hp->cb_data); +} +static int do_for_each_ref (struct ref_store *refs,const char *prefix,each_ref_fn fn,int trim,int flags,void *cb_data) {struct ref_iterator *iter; +struct do_for_each_ref_help hp = { fn, cb_data };if (!refs)return 0 ; iter =refs_ref_iterator_begin (refs, prefix, trim, flags); -return do_for_each_ref_iterator (iter, fn, cb_data); +return do_for_each_repo_ref_iterator (the_repository, iter, + do_for_each_ref_helper, &hp); }int refs_for_each_ref (struct ref_store *refs,each_ref_fn fn,void *cb_data)
@@ -2029,10 +2062,12 @@ int refs_verify_refname_available(struct ref_store *refs,int refs_for_each_reflog (struct ref_store *refs,each_ref_fn fn,void *cb_data) {struct ref_iterator *iter; +struct do_for_each_ref_help hp = { fn, cb_data }; iter = refs->be->reflog_iterator_begin (refs); -return do_for_each_ref_iterator (iter, fn, cb_data); +return do_for_each_repo_ref_iterator (the_repository, iter, + do_for_each_ref_helper, &hp); }int for_each_reflog (each_ref_fn fn,void *cb_data)
refs.h +10 -0
@@ -274,6 +274,16 @@ struct ref_transaction;typedef int each_ref_fn (const char *refname,const struct object_id *oid,int flags,void *cb_data); +/* +* The same as each_ref_fn, but also with a repository argument that +* contains the repository associated with the callback. +*/ +typedef int each_repo_ref_fn (struct repository *r, +const char *refname, +const struct object_id *oid, +int flags, +void *cb_data); +/* * The following functions invoke the specified callback function for * each reference indicated. If the function ever returns a nonzero
refs/iterator.c +3 -3
@@ -407,15 +407,15 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,struct ref_iterator *current_ref_iter =NULL ; -int do_for_each_ref_iterator (struct ref_iterator *iter, -each_ref_fn fn,void *cb_data) +int do_for_each_repo_ref_iterator (struct repository *r,struct ref_iterator *iter, +each_repo_ref_fn fn,void *cb_data) {int retval =0 , ok;struct ref_iterator *old_ref_iter = current_ref_iter; current_ref_iter = iter;while ((ok =ref_iterator_advance (iter)) ==ITER_OK ) { - retval =fn (iter->refname, iter->oid, iter->flags, cb_data); + retval =fn (r, iter->refname, iter->oid, iter->flags, cb_data);if (retval) {/* * If ref_iterator_abort() returns ITER_ERROR,
refs/refs-internal.h +3 -2
@@ -473,8 +473,9 @@ extern struct ref_iterator *current_ref_iter;* adapter between the callback style of reference iteration and the * iterator style. */ -int do_for_each_ref_iterator (struct ref_iterator *iter, -each_ref_fn fn,void *cb_data); +int do_for_each_repo_ref_iterator (struct repository *r, +struct ref_iterator *iter, +each_repo_ref_fn fn,void *cb_data);/* * Only include per-worktree refs in a do_for_each_ref*() iteration.
Signature · 1 line
2.18.0.233.g985f88cf7e-goog
PATCH 2/2 refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback
In reply to Stefan Beller--- builtin/replace.c | 3 ++- refs.c | 9 ++++----- refs.h | 2 +- replace-object.c | 3 ++- 4 files changed, 9 insertions(+), 8 deletions(-)
builtin/replace.c +2 -1
@@ -39,7 +39,8 @@ struct show_data {enum replace_format format; }; -static int show_reference (const char *refname,const struct object_id *oid, +static int show_reference (struct repository *r,const char *refname, +const struct object_id *oid,int flag,void *cb_data) {struct show_data *data = cb_data;
refs.c +4 -5
@@ -1478,12 +1478,11 @@ int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,return do_for_each_ref (refs, prefix, fn,0 , flag, cb_data); } -int for_each_replace_ref (struct repository *r,each_ref_fn fn,void *cb_data) +int for_each_replace_ref (struct repository *r,each_repo_ref_fn fn,void *cb_data) { -return do_for_each_ref (get_main_ref_store (r), - git_replace_ref_base, fn, -strlen (git_replace_ref_base), -DO_FOR_EACH_INCLUDE_BROKEN , cb_data); +return do_for_each_repo_ref (r, git_replace_ref_base, fn, +strlen (git_replace_ref_base), +DO_FOR_EACH_INCLUDE_BROKEN , cb_data); }int for_each_namespaced_ref (each_ref_fn fn,void *cb_data)
refs.h +1 -1
@@ -317,7 +317,7 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,int for_each_tag_ref (each_ref_fn fn,void *cb_data);int for_each_branch_ref (each_ref_fn fn,void *cb_data);int for_each_remote_ref (each_ref_fn fn,void *cb_data); -int for_each_replace_ref (struct repository *r,each_ref_fn fn,void *cb_data); +int for_each_replace_ref (struct repository *r,each_repo_ref_fn fn,void *cb_data);int for_each_glob_ref (each_ref_fn fn,const char *pattern,void *cb_data);int for_each_glob_ref_in (each_ref_fn fn,const char *pattern,const char *prefix,void *cb_data);
replace-object.c +2 -1
@@ -6,7 +6,8 @@#include "repository.h" #include "commit.h" -static int register_replace_ref (const char *refname, +static int register_replace_ref (struct repository *r, +const char *refname,const struct object_id *oid,int flag,void *cb_data) {
Signature · 1 line
2.18.0.233.g985f88cf7e-goog
Re: [PATCH 2/2] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback
Quoted 61 lines · Stefan Beller
Signed-off-by Stefan Beller <[email protected]>--- builtin/replace.c | 3 ++- refs.c | 9 ++++----- refs.h | 2 +- replace-object.c | 3 ++- 4 files changed, 9 insertions(+), 8 deletions(-)builtin/replace.c +2
-1@@ -39,7 +39,8 @@ struct show_data {enum replace_format format; }; -static int show_reference (const char *refname,const struct object_id *oid, +static int show_reference (struct repository *r,const char *refname, +const struct object_id *oid,int flag,void *cb_data) {struct show_data *data = cb_data;refs.c +4
-5@@ -1478,12 +1478,11 @@ int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,return do_for_each_ref (refs, prefix, fn,0 , flag, cb_data); } -int for_each_replace_ref (struct repository *r,each_ref_fn fn,void *cb_data) +int for_each_replace_ref (struct repository *r,each_repo_ref_fn fn,void *cb_data) { -return do_for_each_ref (get_main_ref_store (r), - git_replace_ref_base, fn, -strlen (git_replace_ref_base), -DO_FOR_EACH_INCLUDE_BROKEN , cb_data); +return do_for_each_repo_ref (r, git_replace_ref_base, fn, +strlen (git_replace_ref_base), +DO_FOR_EACH_INCLUDE_BROKEN , cb_data); }int for_each_namespaced_ref (each_ref_fn fn,void *cb_data)refs.h +1
-1@@ -317,7 +317,7 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,int for_each_tag_ref (each_ref_fn fn,void *cb_data);int for_each_branch_ref (each_ref_fn fn,void *cb_data);int for_each_remote_ref (each_ref_fn fn,void *cb_data); -int for_each_replace_ref (struct repository *r,each_ref_fn fn,void *cb_data); +int for_each_replace_ref (struct repository *r,each_repo_ref_fn fn,void *cb_data);int for_each_glob_ref (each_ref_fn fn,const char *pattern,void *cb_data);int for_each_glob_ref_in (each_ref_fn fn,const char *pattern,const char *prefix,void *cb_data);replace-object.c +0
-0
replace-object.c line 13
Quoted 9 lines
replace-object.c +2
-1@@ -6,7 +6,8 @@#include "repository.h" #include "commit.h" -static int register_replace_ref (const char *refname, +static int register_replace_ref (struct repository *r, +const char *refname,const struct object_id *oid,int flag,void *cb_data) {
Overall, I think this is the right approach. The only problem is that you're missing a few 'the_repository' to 'r' replacements in the bodies of show_reference and register_replace_ref.
Thanks,
-Stolee
Re: [PATCH 2/2] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback
Hi Derrick,
Quoted 3 lines
Overall, I think this is the right approach. The only problem is that you're missing a few '
the_repository' to 'r' replacements in the bodies ofshow_referenceandregister_replace_ref.
Thanks.
Originally I had another approach, which was to convert all callbacks to take a repository argument, and only if the refs backend learned to propagate the correct repository the repo argument would be the specific repo, NULL otherwise.
With that approach it was not possible to replace all occurrences of the_repository with 'r', and it would also be confusing.
But as I pursued that approach at the time of writing this patch... I missed the conversions needed.
Thanks for catching them (and fixing them in the reroll that you just sent out via gitgitgadget)!
Thanks,
Stefan
PATCH 1/1 TO-SQUASH: replace the_repository with arbitrary r
In reply to Stefan Beller---
This should just be squashed into PATCH 2.
builtin/replace.c | 5 ++--- replace-object.c | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-)
builtin/replace.c +2 -3
@@ -57,9 +57,8 @@ static int show_reference(struct repository *r, const char *refname,if (get_oid (refname, &object))return error ("Failed to resolve '%s' as a valid ref." , refname); - obj_type =oid_object_info (the_repository, &object, -NULL ); - repl_type =oid_object_info (the_repository, oid,NULL ); + obj_type =oid_object_info (r, &object,NULL ); + repl_type =oid_object_info (r, oid,NULL );printf ("%s (%s) -> %s (%s)\n" , refname,type_name (obj_type),oid_to_hex (oid),type_name (repl_type));
replace-object.c +1 -1
@@ -26,7 +26,7 @@ static int register_replace_ref(struct repository *r,oidcpy (&repl_obj->replacement, oid);/* Register new object */ -if (oidmap_put (the_repository->objects->replace_map, repl_obj)) +if (oidmap_put (r->objects->replace_map, repl_obj))die ("duplicate replace ref: %s" , refname);return 0 ;
Signature · 1 line
2.18.0.118.gd4f65b8d14