Re: [PATCH v2 02/21] t/helper/ref-store: initialize oid in resolve-ref

From: Han-Wen Nienhuys <hanwen@google.com>
Date: May 31, 2021, 14:48 UTC
Message-ID: <CAFQ2z_N2CfX9UYZTBV_kcssz6Pku4GqjtdZ=ZRiMAyCyNn+Chw@mail.gmail.com>
In-Reply-To: <877djtqw5d.fsf@evledraar.gmail.com>

On Thu, May 20, 2021 at 5:09 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 43 quoted lines

>
>
> On Tue, Apr 27 2021, Han-Wen Nienhuys via GitGitGadget wrote:
>
> > From: Han-Wen Nienhuys <hanwen@google.com>
> >
> > This will print $ZERO_OID when asking for a non-existent ref from the
> > test-helper.
> >
> > Since resolve-ref provides direct access to refs_resolve_ref_unsafe(), it
> > provides a reliable mechanism for accessing REFNAME, while avoiding the implicit
> > resolution to refs/heads/REFNAME.
> >
> > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> > ---
> >  t/helper/test-ref-store.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
> > index bba5f841c6ab..01d8f3285dc8 100644
> > --- a/t/helper/test-ref-store.c
> > +++ b/t/helper/test-ref-store.c
> > @@ -118,7 +118,7 @@ static int cmd_for_each_ref(struct ref_store *refs, const char **argv)
> >
> >  static int cmd_resolve_ref(struct ref_store *refs, const char **argv)
> >  {
> > -     struct object_id oid;
> > +     struct object_id oid = { 0 };
> >       const char *refname = notnull(*argv++, "refname");
> >       int resolve_flags = arg_flags(*argv++, "resolve-flags");
> >       int flags;
>
> This feels a bit magical, later we have this:
>
>         printf("%s %s 0x%x\n", oid_to_hex(&oid), ref ? ref : "(null)", flags);
>
> Isn't ref always going to be NULL in that case too? Wouldn't it make
> more sense to not zero this out and instead do:
>
>     if (ref)
>         /* current code, mostly */
>     else
>         use zero_oid()

for programmatic access, the if will actually make it harder to parse out what is happening, so I think this solution is better. Note that the function already handles ref == null, so changing the printf format in this way can only serve to break other tests.

> That seems more straightforward to me than this implicit proxy for
> zero_oid(). Also, isn't the point of zero_oid() to not make this

used null_oid() instead (zero_oid() doesn't exist in C).

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