Re: [PATCH 2/2] refs: switch for_each_replace_ref back to use a ref_store

[Stefan Beller <sbeller@google.com>]

On Mon, Jul 30, 2018 at 5:19 PM Jonathan Tan <jonathantanmy@google.com> 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