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

From: Duy Nguyen <pclouds@gmail.com>
Date: Jul 31, 2018, 16:17 UTC
Message-ID: <CACsJy8DwaLCxY-ryV+=OwRytzwwQZCfVmfXo0z91z9YRMMT0VA@mail.gmail.com>
In-Reply-To: <CAGZ79kbwt2RGo2Z2ARSzfHOZdL_VWF1sR+=EE=QWx1ibLL+KwQ@mail.gmail.com>

On Tue, Jul 31, 2018 at 2:41 AM Stefan Beller <sbeller@google.com> wrote:

Show 17 quoted lines

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