Re: [PATCH 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback

[Junio C Hamano <gitster@pobox.com>]

"Stefan Beller via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Stefan Beller <sbeller@google.com>
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  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, &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));
> 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(&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;