Re: [PATCH v2] gpg-interface: add function for converting trust level to string

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 10, 2022, 05:44 UTC
Message-ID: <CAPig+cScKabgrh80e5rqWX8cnNEgvxP9JyVJCu+afBOJk_yopg@mail.gmail.com>
In-Reply-To: <xmqqwncmt3el.fsf@gitster.g>

On Sat, Jul 9, 2022 at 4:52 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
> > On Fri, Jul 8, 2022 at 7:28 AM Jaydeep Das via GitGitGadget
> >> + * Returns corresponding string in lowercase for a given member of
> >> + * enum signature_trust_level. For example, `TRUST_ULTIMATE` will
> >> + * return "ultimate".
> >> +char *gpg_trust_level_to_str(enum signature_trust_level level);
> >
> > It would be a good idea to update the function documentation to
> > mention that the caller is responsible for freeing the returned
> > string.
>
> Given that there are small and fixed number of trust level strings,
> I actually think that it would be more reasonable to return a static
> string to the caller, something along the lines of the attached, so
> that callers do not have to worry about freeing it.

I also am not a fan of making the caller free the result, and thought of mentioning it but didn't know if the approach implemented by this patch was suggested by an earlier reviewer.

Show 23 quoted lines

> Perhaps along the lines of ...
>
> +static struct sigcheck_gpg_trust_level {
>         const char *key;
>         enum signature_trust_level value;
> +       const char *downcased;
>  } sigcheck_gpg_trust_level[] = {
>
> +const char *gpg_trust_level_to_string(enum signature_trust_level level)
> +{
> +       struct sigcheck_gpg_trust_level *trust;
> +
> +       if (level < 0 || ARRAY_SIZE(sigcheck_gpg_trust_level) <= level)
> +               BUG("invalid trust_level requested: %d", level);
> +
> +       trust = &sigcheck_gpg_trust_level[level];
> +       if (trust->value != level)
> +               BUG("sigcheck_gpg_trust_level[] unsorted");
> +
> +       if (!trust->downcased)
> +               trust->downcased = xstrdup_tolower(trust->key);
> +       return trust->downcased;
> +}

Given the small, fixed number of trust levels, and if the list is unlikely to change much in the future, I might suggest simply initializing the fields at compile-time rather than on-demand at run-time:

    static struct {
        const char *key;
        const char *display_key;
        enum signature_trust_level value;
    } sigcheck_gpg_trust_level[] = {
        { "UNDEFINED", "undefined", TRUST_UNDEFINED },
        { "NEVER", "never", TRUST_NEVER },
        { "MARGINAL", "marginal", TRUST_MARGINAL },
        { "FULLY", "fully", TRUST_FULLY },
        { "ULTIMATE", "ultimate", TRUST_ULTIMATE },
    };