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

From: Stefan Beller <sbeller@google.com>
Date: Jul 31, 2018, 00:41 UTC
Message-ID: <CAGZ79kbwt2RGo2Z2ARSzfHOZdL_VWF1sR+=EE=QWx1ibLL+KwQ@mail.gmail.com>
In-Reply-To: <20180731001858.122968-1-jonathantanmy@google.com>

On Mon, Jul 30, 2018 at 5:19 PM Jonathan Tan <jonathantanmy@google.com> wrote:

Show 5 quoted lines

>
> > 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.

Show 14 quoted lines

>
> > -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