[v16,02/14] Make refs_ref_exists public

Commit Message

Johannes Schindelin via GitGitGadget June 5, 2020, 6:03 p.m. UTC

From: Han-Wen Nienhuys <[e-mail]>

Signed-off-by: Han-Wen Nienhuys <[e-mail]>
---
 refs.c | 2 +-
 refs.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

Comments

Phillip Wood June 9, 2020, 10:36 a.m. UTC | #1

On 05/06/2020 19:03, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <[e-mail]>
> 
> Signed-off-by: Han-Wen Nienhuys <[e-mail]>
> ---
>  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
code in the following patches would be pleasanter if we could write
    ref_exists(r, ref)
rather than
    refs_ref_exists(get_main_ref_store(r), ref)
all the time

Maybe we should think about updating the existing callers to
ref_exists() to pass a struct repository.

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.

Best Wishes

Phillip

> diff --git a/refs.h b/refs.h
> index 4dad8f24914..7aaa1226551 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -105,6 +105,8 @@ int refs_verify_refname_available(struct ref_store *refs,
>  				  const struct string_list *skip,
>  				  struct strbuf *err);
>  
> +int refs_ref_exists(struct ref_store *refs, const char *refname);
> +
>  int ref_exists(const char *refname);
>  
>  int should_autocreate_reflog(const char *refname);
>

Han-Wen Nienhuys June 10, 2020, 6:05 p.m. UTC | #2

On Tue, Jun 9, 2020 at 12:36 PM Phillip Wood <[e-mail]> wrote:
>
> On 05/06/2020 19:03, Han-Wen Nienhuys via GitGitGadget wrote:
> > From: Han-Wen Nienhuys <[e-mail]>
> >
> > Signed-off-by: Han-Wen Nienhuys <[e-mail]>
> > ---
> >  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.

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

Phillip Wood June 11, 2020, 2:59 p.m. UTC | #3

On 10/06/2020 19:05, Han-Wen Nienhuys wrote:
> On Tue, Jun 9, 2020 at 12:36 PM Phillip Wood <[e-mail]> wrote:
>>
>> On 05/06/2020 19:03, Han-Wen Nienhuys via GitGitGadget wrote:
>>> From: Han-Wen Nienhuys <[e-mail]>
>>>
>>> Signed-off-by: Han-Wen Nienhuys <[e-mail]>
>>> ---
>>>   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.

Best Wishes

Phillip

Phillip Wood June 12, 2020, 9:51 a.m. UTC | #4

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
>> <[e-mail]> wrote:
>>>
>>> On 05/06/2020 19:03, Han-Wen Nienhuys via GitGitGadget wrote:
>>>> From: Han-Wen Nienhuys <[e-mail]>
>>>>
>>>> Signed-off-by: Han-Wen Nienhuys <[e-mail]>
>>>> ---
>>>>   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

Han-Wen Nienhuys June 15, 2020, 11:32 a.m. UTC | #5

On Fri, Jun 12, 2020 at 11:51 AM Phillip Wood <[e-mail]> wrote:
> > 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.

Sounds good.  Can you CC me on the patches?

Patch

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);
 }
diff --git a/refs.h b/refs.h
index 4dad8f24914..7aaa1226551 100644
--- a/refs.h
+++ b/refs.h
@@ -105,6 +105,8 @@  int refs_verify_refname_available(struct ref_store *refs,
 				  const struct string_list *skip,
 				  struct strbuf *err);
 
+int refs_ref_exists(struct ref_store *refs, const char *refname);
+
 int ref_exists(const char *refname);
 
 int should_autocreate_reflog(const char *refname);