Re: [PATCH 01/11] t: add helpers to test for reference existence

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Oct 18, 2023 at 1:35 AM Patrick Steinhardt <ps@pks.im> wrote:
> Introduce a new subcommand for our ref-store test helper that explicitly
> checks only for the presence or absence of a reference. This addresses
> these limitations:
> [...]
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
> @@ -221,6 +221,30 @@ static int cmd_verify_ref(struct ref_store *refs, const char **argv)
> +static int cmd_ref_exists(struct ref_store *refs, const char **argv)
> +{
> +       const char *refname = notnull(*argv++, "refname");
> +       struct strbuf unused_referent = STRBUF_INIT;
> +       struct object_id unused_oid;
> +       unsigned int unused_type;
> +       int failure_errno;
> +
> +       if (refs_read_raw_ref(refs, refname, &unused_oid, &unused_referent,
> +                             &unused_type, &failure_errno)) {
> +               /*
> +                * We handle ENOENT separately here such that it is possible to
> +                * distinguish actually-missing references from any kind of
> +                * generic error.
> +                */
> +               if (failure_errno == ENOENT)
> +                       return 17;
> +               return -1;
> +       }
> +
> +       strbuf_release(&unused_referent);
> +       return 0;
> +}

Unless refs_read_raw_ref() guarantees that `unused_referent` remains
unallocated upon failure[*], then the early returns inside the
conditional leak the strbuf. True, the program is exiting immediately
anyhow, so this (potential) leak isn't significant, but it seems odd
to clean up in one case (return 0) but not in the others (return -1 &
17).

[*] In my (admittedly brief) scan of the code and documentation, I
didn't see any such promise.