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

From: Han-Wen Nienhuys <hanwen@google.com>
Date: Jun 10, 2020, 18:05 UTC
Message-ID: <CAFQ2z_MDeiZshhmx=BjqCg7hTF04Fj7oM5dKs15qeESEPjjXEg@mail.gmail.com>
In-Reply-To: <9a5c0243-115e-ce50-dd80-2be4c889f4ba@gmail.com>

On Tue, Jun 9, 2020 at 12:36 PM Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 25 quoted lines

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

Show 7 quoted lines

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

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado