Re: [PATCH 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback
Stefan Beller writes: >> - existing users of for_each_replace_ref() who were all happy >>working in the_repository have to pass it explicitly, even >>thought they do not have any need to. > > All callbacks that are passed to for_each_replace_ref now > operate on 'r' instead of the_repository, and that actually fixes > a bug (commit message is lacking), but the cover letter hints at: > ... >> In this case, even if you introduced for_each_replace_ref_in_repo(), >> making for_each_replace_ref() a thin wrapper that always uses >> the_repository is a bit more cumbersome than just a simple macro. > > Yes, but such a callback would do the *wrong* subtle thing in some cases > as you really want to work in the correct repository for e.g. looking up > commits. > >> But it *is* doable (you'd need to use a wrapping structure around >> cb_data), and a developer who case about maintainability during API >> transition would have taken pains to do so. A bit dissapointing. > > My original patches were RFC-ish and a trade off as for the reflog only > there is nothing in flight to care about. > > Given that we would want to upgrade all the ref callbacks, we have to > take this route, I think. Try to rebuild 'pu' on top of 'master' yoruself to realize how many in-flight topics you are hurting and causing unnecessary load on the maintainer with the "let's add the 'r' parameter without changing the function names; compiler would catch and cause breakages" approach. Until that happens, I won't waste any more time trying to educate you on this further.
Re: [PATCH 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback
> > @@ -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); > > With a signature change like this, any change that introduces new > call to for_each_replace_ref() using eac_ref_fn() would get > compilation error, so this is minimally correct. > > Two things that bothersome are that > > - for_each_tag/branch/remote_ref() and for_each_replace_ref() now >work and look quite differently. Yes, this series is not finished; we need to convert/upgrade for_each_tag et al, too. > - existing users of for_each_replace_ref() who were all happy >working in the_repository have to pass it explicitly, even >thought they do not have any need to. All callbacks that are passed to for_each_replace_ref now operate on 'r' instead of the_repository, and that actually fixes a bug (commit message is lacking), but the cover letter hints at: While building this series, I got some test failures in the non-the_repository tests. These issues are related to missing references to an arbitrary repository (instead of the_repository) and some uninitialized values in the tests. Stefan already sent a patch to address this [2], and I've included those commits (along with a small tweak [3]). These are only included for convenience. > In this case, even if you introduced for_each_replace_ref_in_repo(), > making for_each_replace_ref() a thin wrapper that always uses > the_repository is a bit more cumbersome than just a simple macro. Yes, but such a callback would do the *wrong* subtle thing in some cases as you really want to work in the correct repository for e.g. looking up commits. > But it *is* doable (you'd need to use a wrapping structure around > cb_data), and a developer who case about maintainability during API > transition would have taken pains to do so. A bit dissapointing. My original patches were RFC-ish and a trade off as for the reflog only there is nothing in flight to care about. Given that we would want to upgrade all the ref callbacks, we have to take this route, I think. Thanks, Stefan
Re: [PATCH 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback
"Stefan Beller via GitGitGadget" writes:
> From: Stefan Beller
>
> Signed-off-by: Stefan Beller
> Signed-off-by: Derrick Stolee
> ---
> builtin/replace.c | 8
> refs.c| 9 -
> refs.h| 2 +-
> replace-object.c | 5 +++--
> 4 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index ef22d724b..d0b1cdb06 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -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;
> @@ -56,9 +57,8 @@ static int show_reference(const char *refname, const struct
> object_id *oid,
> if (get_oid(refname, ))
> return error("Failed to resolve '%s' as a valid
> ref.", refname);
>
> - obj_type = oid_object_info(the_repository, ,
> -NULL);
> - repl_type = oid_object_info(the_repository, oid, NULL);
> + obj_type = oid_object_info(r, , 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));
> diff --git a/refs.c b/refs.c
> index 2513f77ac..5700cd468 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -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)
> diff --git a/refs.h b/refs.h
> index 80eec8bbc..a0a18223a 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -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);
With a signature change like this, any change that introduces new
call to for_each_replace_ref() using eac_ref_fn() would get
compilation error, so this is minimally correct.
Two things that bothersome are that
- for_each_tag/branch/remote_ref() and for_each_replace_ref() now
work and look quite differently.
- existing users of for_each_replace_ref() who were all happy
working in the_repository have to pass it explicitly, even
thought they do not have any need to.
In this case, even if you introduced for_each_replace_ref_in_repo(),
making for_each_replace_ref() a thin wrapper that always uses
the_repository is a bit more cumbersome than just a simple macro.
But it *is* doable (you'd need to use a wrapping structure around
cb_data), and a developer who case about maintainability during API
transition would have taken pains to do so. A bit dissapointing.
> 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 801b5c167..017f02f8e 100644
> --- a/replace-object.c
> +++ b/replace-object.c
> @@ -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)
> {
> @@ -25,7 +26,7 @@ static int register_replace_ref(const char *refname,
> oidcpy(_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))
>
