On 11/06/2020 15:59, Phillip Wood wrote:
> On 10/06/2020 19:05, Han-Wen Nienhuys wrote:
>> On Tue, Jun 9, 2020 at 12:36 PM Phillip Wood
>> <phillip.wood123@xxxxxxxxx> wrote:
>>>
>>> On 05/06/2020 19:03, Han-Wen Nienhuys via GitGitGadget wrote:
>>>> From: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
>>>>
>>>> Signed-off-by: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
>>>> ---
>>>> 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.
In the end I put some patches together that change delete_ref(),
update_ref() and ref_exists() to take a struct repository*. I'll clean
them up and post them next week. Hopefully that will mean that this
series can then use those functions when converting unlink() etc which
will avoid having to expose a separate api for pseudo refs.
Best Wishes
Phillip
