| Message ID | pull.1281.v2.git.1657279447515.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] gpg-interface: add function for converting trust level to string | expand |
On Fri, Jul 8, 2022 at 7:28 AM Jaydeep Das via GitGitGadget <gitgitgadget@gmail.com> wrote: > Add new helper function `gpg_trust_level_to_str()` which will > convert a given member of `enum signature_trust_level` to its > corresponding string(in lowercase). For example, `TRUST_ULTIMATE` s/g(/g (/ > will yield the string "ultimate". > > This will abstract out some code in `pretty.c` relating to gpg > signature trust levels. > > Signed-off-by: Jaydeep Das <jaydeepjd.8914@gmail.com> > --- > diff --git a/gpg-interface.h b/gpg-interface.h > @@ -71,6 +71,14 @@ size_t parse_signed_buffer(const char *buf, size_t size); > +/* > + * 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.
On 7/9/22 6:28 AM, Eric Sunshine <sunshine@sunshineco.com> wrote: > On Fri, Jul 8, 2022 at 7:28 AM Jaydeep Das via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > Signed-off-by: Jaydeep Das <jaydeepjd.8914@gmail.com> > > --- > > diff --git a/gpg-interface.h b/gpg-interface.h > > @@ -71,6 +71,14 @@ size_t parse_signed_buffer(const char *buf, size_t size); > > +/* > > + * 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. Yeah. Will do in next version. Thanks, Jaydeep.
Eric Sunshine <sunshine@sunshineco.com> writes: > On Fri, Jul 8, 2022 at 7:28 AM Jaydeep Das via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> Add new helper function `gpg_trust_level_to_str()` which will >> convert a given member of `enum signature_trust_level` to its >> corresponding string(in lowercase). For example, `TRUST_ULTIMATE` > > s/g(/g (/ > >> will yield the string "ultimate". >> >> This will abstract out some code in `pretty.c` relating to gpg >> signature trust levels. >> >> Signed-off-by: Jaydeep Das <jaydeepjd.8914@gmail.com> >> --- >> diff --git a/gpg-interface.h b/gpg-interface.h >> @@ -71,6 +71,14 @@ size_t parse_signed_buffer(const char *buf, size_t size); >> +/* >> + * 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. Perhaps along the lines of ... gpg-interface.c | 19 ++++++++++++++++++- gpg-interface.h | 2 ++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git c/gpg-interface.c w/gpg-interface.c index 947b58ad4d..4a5b9d0f3a 100644 --- c/gpg-interface.c +++ w/gpg-interface.c @@ -165,9 +165,10 @@ static struct { { 0, "TRUST_", GPG_STATUS_TRUST_LEVEL }, }; -static struct { +static struct sigcheck_gpg_trust_level { const char *key; enum signature_trust_level value; + const char *downcased; } sigcheck_gpg_trust_level[] = { { "UNDEFINED", TRUST_UNDEFINED }, { "NEVER", TRUST_NEVER }, @@ -176,6 +177,22 @@ static struct { { "ULTIMATE", TRUST_ULTIMATE }, }; +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; +} + static void replace_cstring(char **field, const char *line, const char *next) { free(*field); diff --git c/gpg-interface.h w/gpg-interface.h index b30cbdcd3d..2dffcb836d 100644 --- c/gpg-interface.h +++ w/gpg-interface.h @@ -85,4 +85,6 @@ int check_signature(struct signature_check *sigc, void print_signature_buffer(const struct signature_check *sigc, unsigned flags); +const char *gpg_trust_level_to_string(enum signature_trust_level); + #endif
On Sat, Jul 9, 2022 at 4:52 PM Junio C Hamano <gitster@pobox.com> wrote: > 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. > 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 }, };
Eric Sunshine <sunshine@sunshineco.com> writes: > 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 }, > }; Yup, that is even better. I wonder if we can upcase in C preprocessor macro? It would be wonderful if we can do so, but for just 5 entries, we can type each token three times just fine.
On Sun, Jul 10, 2022 at 1:48 AM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > 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 }, > > }; > > Yup, that is even better. I wonder if we can upcase in C > preprocessor macro? It would be wonderful if we can do so, > but for just 5 entries, we can type each token three times > just fine. No standardized way to upcase via the C preprocessor, as far as I know. At any rate, I suspect such a macro would be uglier and harder to reason about than the code above, which is dead-simple to understand.
On 7/10/22 11:14, Eric Sunshine wrote: > 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. > > > 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 }, > }; Will do in next patch. Thanks, Jaydeep
diff --git a/gpg-interface.c b/gpg-interface.c index 947b58ad4da..4ef660a09fc 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -165,6 +165,7 @@ static struct { { 0, "TRUST_", GPG_STATUS_TRUST_LEVEL }, }; +/* Keep the order same as enum signature_trust_level */ static struct { const char *key; enum signature_trust_level value; @@ -905,6 +906,12 @@ const char *get_signing_key(void) return git_committer_info(IDENT_STRICT | IDENT_NO_DATE); } +char *gpg_trust_level_to_str(enum signature_trust_level level){ + if (level < TRUST_UNDEFINED || level > TRUST_ULTIMATE) + return NULL; + return xstrdup_tolower(sigcheck_gpg_trust_level[level].key); +} + int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key) { return use_format->sign_buffer(buffer, signature, signing_key); diff --git a/gpg-interface.h b/gpg-interface.h index b30cbdcd3da..ce2db6f3780 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -71,6 +71,14 @@ size_t parse_signed_buffer(const char *buf, size_t size); int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key); + +/* + * 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); + int git_gpg_config(const char *, const char *, void *); void set_signing_key(const char *); const char *get_signing_key(void); diff --git a/pretty.c b/pretty.c index ee6114e3f0a..5ee03d6fe09 100644 --- a/pretty.c +++ b/pretty.c @@ -1348,6 +1348,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *msg = c->message; struct commit_list *p; const char *arg, *eol; + char *sig_str; size_t res; char **slot; @@ -1575,23 +1576,10 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ strbuf_addstr(sb, c->signature_check.primary_key_fingerprint); break; case 'T': - switch (c->signature_check.trust_level) { - case TRUST_UNDEFINED: - strbuf_addstr(sb, "undefined"); - break; - case TRUST_NEVER: - strbuf_addstr(sb, "never"); - break; - case TRUST_MARGINAL: - strbuf_addstr(sb, "marginal"); - break; - case TRUST_FULLY: - strbuf_addstr(sb, "fully"); - break; - case TRUST_ULTIMATE: - strbuf_addstr(sb, "ultimate"); - break; - } + sig_str = gpg_trust_level_to_str(c->signature_check.trust_level); + if (sig_str) + strbuf_addstr(sb, sig_str); + free(sig_str); break; default: return 0;