Re: [PATCH 2/2] refs: switch for_each_replace_ref back to use a ref_store
On Tue, Jul 31, 2018 at 2:41 AM Stefan Beller wrote: > > Taking a step back, was there anything that prompted these patches? > > I am flailing around on how to approach the ref store and the repository: > * I dislike having to pass a repository 'r' twice. (current situation after > patch 1. That patch itself is part of Stolees larger series to address > commit graphs and replace refs, so we will have that one way or another) > * So I sent out some RFC patches to have the_repository in the ref store > and pass the repo through to all the call backs to make it easy for > users inside the callback to do basic things like looking up commits. > * both Duy (on list) and Brandon (privately) expressed their dislike for > having the refs API bloated with the repository, as the repository is > not needed per se in the ref store. > * After some reflection I agreed with their concerns, which let me > to re-examine the refs API: all but a few select functions take a > ref_store as the first argument (or imply to work on the ref store > in the_repository, then neither a repo nor a ref store argument is > there) Since I'm the one who added the refs_* variants (which take ref_store as the first argument). There's one thing that I should have done but did not: making each_ref_fn takes the ref store. If a callback is given a refname and wants to do something about it (other that just printing it), chances are you need the same ref-store that triggers the callback and you should not need to pass a separate ref-store around by yourself because you would have the same "passing twice" problem that you disliked. This is more obvious with refs_for_each_reflog() because you will very likely want to parse the ref from the callback. Then, even ref store code needs access to object database and I don't think we want to pass a pair of "struct repository *", "struct ref_store *" in every API. We know the ref store has to be associated with one repository and we do save that information (notice that ref_store_init_fn takes gitdir and the "files" backend does save it). Once refs code is adapted to struct repository, I think it will take a 'struct repository *' instead of the gitdir string and store the pointer to the repository too for internal use. Then if a ref callback needs access to the same repository, we could just provide this repo via refs api. Since callbacks should already have access to the ref store (preferably without having to carrying it via cb_data), it has access to the repository as well and you don't need to explicitly pass the repository. > * I want to bring back the cleanliness of the API, which is to take a > ref store when needed instead of the repository, which is rather > bloated. -- Duy;
Re: [PATCH 2/2] refs: switch for_each_replace_ref back to use a ref_store
On Mon, Jul 30, 2018 at 5:19 PM Jonathan Tan wrote:
>
> > So let's go back to the clean API, just requiring a ref_store as an
> > argument.
>
> Here, you say that we want ref_store as an argument...
I do.
>
> > -int for_each_replace_ref(struct repository *r, each_ref_fn fn, void
> > *cb_data)
> > +int for_each_replace_ref(each_ref_fn fn, void *cb_data)
> > {
> > - return do_for_each_ref(get_main_ref_store(r),
> > + return do_for_each_ref(get_main_ref_store(the_repository),
> > git_replace_ref_base, fn,
> > strlen(git_replace_ref_base),
> > DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
>
> ...but there is no ref_store as an argument here - instead, the
> repository argument is deleted with no replacement. I presume you meant
> to replace it with a ref_store instead? (This will also fix the issue
> that for_each_replace_ref only works on the_repository.)
Yes, I would want to pass in a ref_store and use that as the first argument
in do_for_each_ref for now.
That would reduce the API uncleanliness to have to pass the repository twice.
> Taking a step back, was there anything that prompted these patches?
I am flailing around on how to approach the ref store and the repository:
* I dislike having to pass a repository 'r' twice. (current situation after
patch 1. That patch itself is part of Stolees larger series to address
commit graphs and replace refs, so we will have that one way or another)
* So I sent out some RFC patches to have the_repository in the ref store
and pass the repo through to all the call backs to make it easy for
users inside the callback to do basic things like looking up commits.
* both Duy (on list) and Brandon (privately) expressed their dislike for
having the refs API bloated with the repository, as the repository is
not needed per se in the ref store.
* After some reflection I agreed with their concerns, which let me
to re-examine the refs API: all but a few select functions take a
ref_store as the first argument (or imply to work on the ref store
in the_repository, then neither a repo nor a ref store argument is
there)
* I want to bring back the cleanliness of the API, which is to take a
ref store when needed instead of the repository, which is rather
bloated.
> Maybe at least the 2nd one should wait until we have a situation that
> warrants it (for example, if we want to for_each_replace_ref(), but we
> only have a ref_store, not a repository).
okay, then let's drop this series for now and I'll re-examine what is
needed to have submodule handling in-core.
Thanks,
Stefan
Re: [PATCH 2/2] refs: switch for_each_replace_ref back to use a ref_store
> So let's go back to the clean API, just requiring a ref_store as an
> argument.
Here, you say that we want ref_store as an argument...
> -int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data)
> +int for_each_replace_ref(each_ref_fn fn, void *cb_data)
> {
> - return do_for_each_ref(get_main_ref_store(r),
> + return do_for_each_ref(get_main_ref_store(the_repository),
> git_replace_ref_base, fn,
> strlen(git_replace_ref_base),
> DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
...but there is no ref_store as an argument here - instead, the
repository argument is deleted with no replacement. I presume you meant
to replace it with a ref_store instead? (This will also fix the issue
that for_each_replace_ref only works on the_repository.)
Taking a step back, was there anything that prompted these patches?
Maybe at least the 2nd one should wait until we have a situation that
warrants it (for example, if we want to for_each_replace_ref(), but we
only have a ref_store, not a repository).
[PATCH 2/2] refs: switch for_each_replace_ref back to use a ref_store
This effectively reverts commit 0d296c57ae (refs: allow for_each_replace_ref
to handle arbitrary repositories, 2018-04-11) and 60ce76d3581 (refs: add
repository argument to for_each_replace_ref, 2018-04-11).
The repository argument is not any special from the ref-store's point
of life. If you need a repository (for e.g. lookup_commit or friends),
you'll have to pass it through the callback cookie, whether directly or
as part of a struct tailored to your purpose.
So let's go back to the clean API, just requiring a ref_store as an
argument.
Signed-off-by: Stefan Beller
---
builtin/replace.c | 2 +-
refs.c| 4 ++--
refs.h| 2 +-
replace-object.c | 2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/replace.c b/builtin/replace.c
index deabda21012..52dc371eafc 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -87,7 +87,7 @@ static int list_replace_refs(const char *pattern, const char
*format)
"valid formats are 'short', 'medium' and 'long'\n",
format);
- for_each_replace_ref(the_repository, show_reference, (void *)&data);
+ for_each_replace_ref(show_reference, (void *)&data);
return 0;
}
diff --git a/refs.c b/refs.c
index 08fb5a99148..2d713499125 100644
--- a/refs.c
+++ b/refs.c
@@ -1441,9 +1441,9 @@ 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(each_ref_fn fn, void *cb_data)
{
- return do_for_each_ref(get_main_ref_store(r),
+ return do_for_each_ref(get_main_ref_store(the_repository),
git_replace_ref_base, fn,
strlen(git_replace_ref_base),
DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
diff --git a/refs.h b/refs.h
index cc2fb4c68c0..48d5ffd2082 100644
--- a/refs.h
+++ b/refs.h
@@ -307,7 +307,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(each_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);
diff --git a/replace-object.c b/replace-object.c
index e99fcd1ff6e..ee3374ab59b 100644
--- a/replace-object.c
+++ b/replace-object.c
@@ -41,7 +41,7 @@ static void prepare_replace_object(struct repository *r)
xmalloc(sizeof(*r->objects->replace_map));
oidmap_init(r->objects->replace_map, 0);
- for_each_replace_ref(r, register_replace_ref, r);
+ for_each_replace_ref(register_replace_ref, r);
}
/* We allow "recursive" replacement. Only within reason, though */
--
2.18.0.132.g195c49a2227
