Re: [PATCH v16 02/14] Make refs_ref_exists public

> On 10/06/2020 19:05, Han-Wen Nienhuys wrote:
>> On Tue, Jun 9, 2020 at 12:36 PM Phillip Wood
>> <phillip.wood123@gmail.com> wrote:
>>>
>>> On 05/06/2020 19:03, Han-Wen Nienhuys via GitGitGadget wrote:
>>>> From: Han-Wen Nienhuys <hanwen@google.com>
>>>>
>>>> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
>>>> ---
>>>>   refs.c | 2 +-
>>>>   refs.h | 2 ++
>>>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/refs.c b/refs.c
>>>> index 12908066b13..812fee47108 100644
>>>> --- a/refs.c
>>>> +++ b/refs.c
>>>> @@ -311,7 +311,7 @@ int read_ref(const char *refname, struct
>>>> object_id *oid)
>>>>        return read_ref_full(refname, RESOLVE_REF_READING, oid, NULL);
>>>>   }
>>>>
>>>> -static int refs_ref_exists(struct ref_store *refs, const char
>>>> *refname)
>>>> +int refs_ref_exists(struct ref_store *refs, const char *refname)
>>>>   {
>>>>        return !!refs_resolve_ref_unsafe(refs, refname,
>>>> RESOLVE_REF_READING, NULL, NULL);
>>>>   }
>>>
>>> It is a shame that ref_exists() does not take a struct repository. The
>>
>> I'm trying to follow the pattern that the rest of the code
>> establishes; you're right that the code isn't very consistent (the
>> fact that it uses unlink() rather than go through the ref store in the
>> first place is an indication of that). I want to avoid starting a
>> general cleanup tour of the code base, the reftable patch is hard
>> enough to pull off without.
> 
> Sure
> 
>>> The existing code is inconsistent about its repository handling - we
>>> create the refs with update_ref() which operates on the main repository
>>> but when checking their existence and deleting them we use a path which
>>> depends on the repository. I've realized now the answer to my question
>>> about using delete_ref() in my reply to the previous patch - it does not
>>> take a repository - maybe it should along with update_ref() but that
>>> might be more work than you want to do though.
>>
>> Why do they take repository arguments anyway? Is it because
>> rebase/cherry-pick supports recursion into submodules?
> 
> It was a stepping stone towards that, the git_path mechanism that is
> used to create git_path_cherry_pick_head() etc was changed to take a
> struct repository so it could support submodules without forking a
> separate process. However are still plenty of places where the sequencer
> code assumes a single repository (it calls update_ref(), delete_ref(),
> commit_tree_extended(), ...) and the two contributors who did a lot of
> that work have moved on. With that in mind perhaps we'd be better off
> just using ref_exists() and delete_ref() in this conversion. The call
> sites will be easy enough to fixup if those functions are converted to
> take a struct repository in the future and the result of this patch
> series will be nicer. I've cc'd dscho and Junio to see what they think.