| Message ID | e6222944a3eb441d44ab6c7d1e9b873f5546220a.1619519903.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Prepare tests for reftable backend | expand |
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...
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 --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;