diff mbox series

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

Message ID e6222944a3eb441d44ab6c7d1e9b873f5546220a.1619519903.git.gitgitgadget@gmail.com (mailing list archive)
State New
Headers show
Series Prepare tests for reftable backend | expand

Commit Message

Han-Wen Nienhuys April 27, 2021, 10:38 a.m. UTC
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(-)

Comments

Ævar Arnfjörð Bjarmason May 20, 2021, 3:06 p.m. UTC | #1
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()

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
particular assumption, i.e. the recent discussion (haven't followed
where it went) of the "oid" having some sort of "hash type" member,
which surely would do the wrong thing here under either SHA-1 or
SHA-256, or maybe I mis(understood|remember) that discussion...
Han-Wen Nienhuys May 31, 2021, 2:48 p.m. UTC | #2
On Thu, May 20, 2021 at 5:09 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> 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).
diff mbox series

Patch

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;