# [PATCH v2 0/5] Add --format to tag verification 17 messages from 2016-09-26 to 2016-09-27. Participants: santiago@nyu.edu, Philip Oakley, Santiago Torres, Junio C Hamano, Lukas Pühringer, Stefan Beller. Thread: https://gitlist.dev/t/44166 ## santiago@nyu.edu, 2016-09-26 22:42 Subject: [PATCH v2 0/5] Add --format to tag verification Message-ID: <20160926224233.32702-1-santiago@nyu.edu> URL: https://gitlist.dev/e/20160926224233.32702-1-santiago%40nyu.edu ``` From: Santiago Torres This is the second iteration of [1], and as a result of the discussion in [2]. In this re-roll we: * Dropped the commit to move the format string parameter to a global variable on builtin/tag. We had to change the signature of for_each_name_fn to do this. * Fixed the signed-off-by lines to match the author/committer [0001] * Moved the GPG_VERIFY_QUIET flag check into tag.c instead of gpg_interface [0002] * Changed the exported function to "format_ref". This way, we can print a single formatted ref element. This also made the patch cleaner/easier to read. [0003] * We fixed style/formatting errors that we had introduced This patch applies to 2.10.0 and master. [1] http://public-inbox.org/git/20160922185317.349-1-santiago@nyu.edu/ [2] http://public-inbox.org/git/20160607195608.16643-1-santiago@nyu.edu/ Lukas P (4): gpg-interface, tag: add GPG_VERIFY_QUIET flag ref-filter: add function to print single ref_array_item tag: add format specifier to gpg_verify_tag builtin/tag: add --format argument for tag -v Santiago Torres (1): builtin/verify-tag: add --format to verify-tag builtin/tag.c | 30 ++++++++++++++++++++---------- builtin/verify-tag.c | 16 ++++++++++++++-- gpg-interface.h | 1 + ref-filter.c | 10 ++++++++++ ref-filter.h | 4 ++++ tag.c | 22 +++++++++++++++------- tag.h | 4 ++-- 7 files changed, 66 insertions(+), 21 deletions(-) -- 2.10.0 ``` ## santiago@nyu.edu, 2016-09-26 22:42 Subject: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag Message-ID: <20160926224233.32702-2-santiago@nyu.edu> URL: https://gitlist.dev/e/20160926224233.32702-2-santiago%40nyu.edu In-Reply-To: <20160926224233.32702-1-santiago@nyu.edu> ``` From: Lukas P Functions that print git object information may require that the gpg-interface functions be silent. Add GPG_VERIFY_QUIET flag and prevent print_signature_buffer from being called if flag is set. Signed-off-by: Lukas P --- gpg-interface.h | 1 + tag.c | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/gpg-interface.h b/gpg-interface.h index ea68885..85dc982 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -3,6 +3,7 @@ #define GPG_VERIFY_VERBOSE 1 #define GPG_VERIFY_RAW 2 +#define GPG_VERIFY_QUIET 4 struct signature_check { char *payload; diff --git a/tag.c b/tag.c index d1dcd18..291073f 100644 --- a/tag.c +++ b/tag.c @@ -3,6 +3,7 @@ #include "commit.h" #include "tree.h" #include "blob.h" +#include "gpg-interface.h" const char *tag_type = "tag"; @@ -24,7 +25,9 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags) ret = check_signature(buf, payload_size, buf + payload_size, size - payload_size, &sigc); - print_signature_buffer(&sigc, flags); + + if (!(flags & GPG_VERIFY_QUIET)) + print_signature_buffer(&sigc, flags); signature_check_clear(&sigc); return ret; -- 2.10.0 ``` ## santiago@nyu.edu, 2016-09-26 22:42 Subject: [PATCH v2 2/5] ref-filter: add function to print single ref_array_item Message-ID: <20160926224233.32702-3-santiago@nyu.edu> URL: https://gitlist.dev/e/20160926224233.32702-3-santiago%40nyu.edu In-Reply-To: <20160926224233.32702-1-santiago@nyu.edu> ``` From: Lukas P ref-filter functions are useful for printing git object information using a format specifier. However, some other modules may not want to use this functionality on a ref-array but only print a single item. Expose a format_ref function to create, pretty print and free individual ref-items. Signed-off-by: Lukas P --- ref-filter.c | 10 ++++++++++ ref-filter.h | 4 ++++ 2 files changed, 14 insertions(+) diff --git a/ref-filter.c b/ref-filter.c index bc551a7..e0aaf5f 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1655,6 +1655,16 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu putchar('\n'); } +void format_ref(const char *name, const unsigned char *sha1, const char *format, + unsigned kind) +{ + struct ref_array_item *ref_item; + ref_item = new_ref_array_item(name, sha1, 0); + ref_item->kind = kind; + show_ref_array_item(ref_item, format, 0); + free_array_item(ref_item); +} + /* If no sorting option is given, use refname to sort as default */ struct ref_sorting *ref_default_sorting(void) { diff --git a/ref-filter.h b/ref-filter.h index 14d435e..1ef7999 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -107,4 +107,8 @@ struct ref_sorting *ref_default_sorting(void); /* Function to parse --merged and --no-merged options */ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset); +/* Pretty-print a single ref */ +void format_ref(const char *name, const unsigned char *sha1, const char *format, + unsigned kind); + #endif /* REF_FILTER_H */ -- 2.10.0 ``` ## santiago@nyu.edu, 2016-09-26 22:42 Subject: [PATCH v2 3/5] tag: add format specifier to gpg_verify_tag Message-ID: <20160926224233.32702-4-santiago@nyu.edu> URL: https://gitlist.dev/e/20160926224233.32702-4-santiago%40nyu.edu In-Reply-To: <20160926224233.32702-1-santiago@nyu.edu> ``` From: Lukas P Calling functions for gpg_verify_tag() may desire to print relevant information about the header for further verification. Add an optional format argument to print any desired information after GPG verification. Signed-off-by: Lukas P --- builtin/tag.c | 2 +- builtin/verify-tag.c | 2 +- tag.c | 17 +++++++++++------ tag.h | 4 ++-- 4 files changed, 15 insertions(+), 10 deletions(-) diff --git a/builtin/tag.c b/builtin/tag.c index 50e4ae5..14f3b48 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -105,7 +105,7 @@ static int delete_tag(const char *name, const char *ref, static int verify_tag(const char *name, const char *ref, const unsigned char *sha1) { - return gpg_verify_tag(sha1, name, GPG_VERIFY_VERBOSE); + return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE); } static int do_sign(struct strbuf *buffer) diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index 99f8148..de10198 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -51,7 +51,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) const char *name = argv[i++]; if (get_sha1(name, sha1)) had_error = !!error("tag '%s' not found.", name); - else if (gpg_verify_tag(sha1, name, flags)) + else if (verify_and_format_tag(sha1, name, NULL, flags)) had_error = 1; } return had_error; diff --git a/tag.c b/tag.c index 291073f..65e1a96 100644 --- a/tag.c +++ b/tag.c @@ -4,6 +4,7 @@ #include "tree.h" #include "blob.h" #include "gpg-interface.h" +#include "ref-filter.h" const char *tag_type = "tag"; @@ -33,8 +34,8 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags) return ret; } -int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report, - unsigned flags) +int verify_and_format_tag(const unsigned char *sha1, const char *name, + const char *fmt_pretty, unsigned flags) { enum object_type type; char *buf; @@ -44,21 +45,25 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report, type = sha1_object_info(sha1, NULL); if (type != OBJ_TAG) return error("%s: cannot verify a non-tag object of type %s.", - name_to_report ? - name_to_report : + name ? + name : find_unique_abbrev(sha1, DEFAULT_ABBREV), typename(type)); buf = read_sha1_file(sha1, &type, &size); if (!buf) return error("%s: unable to read file.", - name_to_report ? - name_to_report : + name ? + name : find_unique_abbrev(sha1, DEFAULT_ABBREV)); ret = run_gpg_verify(buf, size, flags); free(buf); + + if (fmt_pretty) + format_ref(name, sha1, fmt_pretty, FILTER_REFS_TAGS); + return ret; } diff --git a/tag.h b/tag.h index a5721b6..0b6e458 100644 --- a/tag.h +++ b/tag.h @@ -17,7 +17,7 @@ extern int parse_tag_buffer(struct tag *item, const void *data, unsigned long si extern int parse_tag(struct tag *item); extern struct object *deref_tag(struct object *, const char *, int); extern struct object *deref_tag_noverify(struct object *); -extern int gpg_verify_tag(const unsigned char *sha1, - const char *name_to_report, unsigned flags); +extern int verify_and_format_tag(const unsigned char *sha1, const char *name, + const char *fmt_pretty, unsigned flags); #endif /* TAG_H */ -- 2.10.0 ``` ## santiago@nyu.edu, 2016-09-26 22:42 Subject: [PATCH v2 5/5] builtin/tag: add --format argument for tag -v Message-ID: <20160926224233.32702-6-santiago@nyu.edu> URL: https://gitlist.dev/e/20160926224233.32702-6-santiago%40nyu.edu In-Reply-To: <20160926224233.32702-1-santiago@nyu.edu> ``` From: Lukas P Adding --format to git tag -v mutes the default output of the GPG verification and instead prints the formatted tag object. This allows callers to cross-check the tagname from refs/tags with the tagname from the tag object header upon GPG verification. Caveat: The change adds a format specifier argument to the (*each_tag_name_fn) function pointer, i.e. delete_tag now receives this too, although it does not need it. Signed-off-by: Lukas P --- builtin/tag.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/builtin/tag.c b/builtin/tag.c index 14f3b48..f53227e 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -24,7 +24,7 @@ static const char * const git_tag_usage[] = { N_("git tag -d ..."), N_("git tag -l [-n[]] [--contains ] [--points-at ]" "\n\t\t[--format=] [--[no-]merged []] [...]"), - N_("git tag -v ..."), + N_("git tag -v [--format=] ..."), NULL }; @@ -66,9 +66,10 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con } typedef int (*each_tag_name_fn)(const char *name, const char *ref, - const unsigned char *sha1); + const unsigned char *sha1, const char *fmt_pretty); -static int for_each_tag_name(const char **argv, each_tag_name_fn fn) +static int for_each_tag_name(const char **argv, each_tag_name_fn fn, + const char *fmt_pretty) { const char **p; char ref[PATH_MAX]; @@ -87,14 +88,14 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn) had_error = 1; continue; } - if (fn(*p, ref, sha1)) + if (fn(*p, ref, sha1, fmt_pretty)) had_error = 1; } return had_error; } static int delete_tag(const char *name, const char *ref, - const unsigned char *sha1) + const unsigned char *sha1, const char *fmt_pretty) { if (delete_ref(ref, sha1, 0)) return 1; @@ -103,9 +104,15 @@ static int delete_tag(const char *name, const char *ref, } static int verify_tag(const char *name, const char *ref, - const unsigned char *sha1) + const unsigned char *sha1, const char *fmt_pretty) { - return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE); + int flags; + flags = GPG_VERIFY_VERBOSE; + + if (fmt_pretty) + flags = GPG_VERIFY_QUIET; + + return verify_and_format_tag(sha1, name, fmt_pretty, flags); } static int do_sign(struct strbuf *buffer) @@ -424,9 +431,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix) if (filter.merge_commit) die(_("--merged and --no-merged option are only allowed with -l")); if (cmdmode == 'd') - return for_each_tag_name(argv, delete_tag); - if (cmdmode == 'v') - return for_each_tag_name(argv, verify_tag); + return for_each_tag_name(argv, delete_tag, NULL); + if (cmdmode == 'v') { + if (format) + verify_ref_format(format); + return for_each_tag_name(argv, verify_tag, format); + } if (msg.given || msgfile) { if (msg.given && msgfile) -- 2.10.0 ``` ## santiago@nyu.edu, 2016-09-26 22:42 Subject: [PATCH v2 4/5] builtin/verify-tag: add --format to verify-tag Message-ID: <20160926224233.32702-5-santiago@nyu.edu> URL: https://gitlist.dev/e/20160926224233.32702-5-santiago%40nyu.edu In-Reply-To: <20160926224233.32702-1-santiago@nyu.edu> ``` From: Santiago Torres Callers of verify-tag may want to cross-check the tagname from refs/tags with the tagname from the tag object header upon GPG verification. This is to avoid tag refs that point to an incorrect object. Add a --format parameter to git verify-tag to print the formatted tag object header in addition to or instead of the --verbose or --raw GPG verification output. Signed-off-by: Santiago Torres --- builtin/verify-tag.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index de10198..a941053 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -12,12 +12,15 @@ #include #include "parse-options.h" #include "gpg-interface.h" +#include "ref-filter.h" static const char * const verify_tag_usage[] = { - N_("git verify-tag [-v | --verbose] ..."), + N_("git verify-tag [-v | --verbose] [--format=] ..."), NULL }; +static char *fmt_pretty; + static int git_verify_tag_config(const char *var, const char *value, void *cb) { int status = git_gpg_config(var, value, cb); @@ -33,6 +36,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) const struct option verify_tag_options[] = { OPT__VERBOSE(&verbose, N_("print tag contents")), OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW), + OPT_STRING( 0 , "format", &fmt_pretty, N_("format"), N_("format to use for the output")), OPT_END() }; @@ -46,12 +50,17 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) if (verbose) flags |= GPG_VERIFY_VERBOSE; + if (fmt_pretty) { + verify_ref_format(fmt_pretty); + flags |= GPG_VERIFY_QUIET; + } + while (i < argc) { unsigned char sha1[20]; const char *name = argv[i++]; if (get_sha1(name, sha1)) had_error = !!error("tag '%s' not found.", name); - else if (verify_and_format_tag(sha1, name, NULL, flags)) + else if (verify_and_format_tag(sha1, name, fmt_pretty, flags)) had_error = 1; } return had_error; -- 2.10.0 ``` ## Philip Oakley, 2016-09-27 07:44 Subject: Re: [PATCH v2 4/5] builtin/verify-tag: add --format to verify-tag Message-ID: URL: https://gitlist.dev/e/D5DC2D99E1F84C308DDB99096B0E3BCD%40PhilipOakley In-Reply-To: <20160926224233.32702-5-santiago@nyu.edu> ``` From: > From: Santiago Torres > > Callers of verify-tag may want to cross-check the tagname from refs/tags > with the tagname from the tag object header upon GPG verification. This > is to avoid tag refs that point to an incorrect object. > > Add a --format parameter to git verify-tag to print the formatted tag > object header in addition to or instead of the --verbose or --raw GPG > verification output. > > Signed-off-by: Santiago Torres > --- > builtin/verify-tag.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c > index de10198..a941053 100644 > --- a/builtin/verify-tag.c > +++ b/builtin/verify-tag.c > @@ -12,12 +12,15 @@ > #include > #include "parse-options.h" > #include "gpg-interface.h" > +#include "ref-filter.h" > > static const char * const verify_tag_usage[] = { > - N_("git verify-tag [-v | --verbose] ..."), > + N_("git verify-tag [-v | --verbose] [--format=] ..."), Does this require a corresponding documentation change? (also 5/5) > NULL > }; > > +static char *fmt_pretty; > + > static int git_verify_tag_config(const char *var, const char *value, void > *cb) > { > int status = git_gpg_config(var, value, cb); > @@ -33,6 +36,7 @@ int cmd_verify_tag(int argc, const char **argv, const > char *prefix) > const struct option verify_tag_options[] = { > OPT__VERBOSE(&verbose, N_("print tag contents")), > OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), > GPG_VERIFY_RAW), > + OPT_STRING( 0 , "format", &fmt_pretty, N_("format"), N_("format to use > for the output")), > OPT_END() > }; > > @@ -46,12 +50,17 @@ int cmd_verify_tag(int argc, const char **argv, const > char *prefix) > if (verbose) > flags |= GPG_VERIFY_VERBOSE; > > + if (fmt_pretty) { > + verify_ref_format(fmt_pretty); > + flags |= GPG_VERIFY_QUIET; > + } > + > while (i < argc) { > unsigned char sha1[20]; > const char *name = argv[i++]; > if (get_sha1(name, sha1)) > had_error = !!error("tag '%s' not found.", name); > - else if (verify_and_format_tag(sha1, name, NULL, flags)) > + else if (verify_and_format_tag(sha1, name, fmt_pretty, flags)) > had_error = 1; > } > return had_error; > -- > 2.10.0 > -- Philip ``` ## Santiago Torres, 2016-09-27 14:38 Subject: Re: [PATCH v2 4/5] builtin/verify-tag: add --format to verify-tag Message-ID: <20160927143814.z54h3wi55t5xnqlu@LykOS.localdomain> URL: https://gitlist.dev/e/20160927143814.z54h3wi55t5xnqlu%40LykOS.localdomain In-Reply-To: ``` > > static const char * const verify_tag_usage[] = { > > - N_("git verify-tag [-v | --verbose] ..."), > > + N_("git verify-tag [-v | --verbose] [--format=] ..."), > > Does this require a corresponding documentation change? (also 5/5) > Yes, I'll work on this while I wait for more reviews. Thanks! -Santiago. ``` ## Junio C Hamano, 2016-09-27 17:35 Subject: Re: [PATCH v2 2/5] ref-filter: add function to print single ref_array_item Message-ID: URL: https://gitlist.dev/e/xmqqfuolp7nk.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160926224233.32702-3-santiago@nyu.edu> ``` santiago@nyu.edu writes: > From: Lukas P > > ref-filter functions are useful for printing git object information > using a format specifier. However, some other modules may not want to use > this functionality on a ref-array but only print a single item. > > Expose a format_ref function to create, pretty print and free individual > ref-items. > > Signed-off-by: Lukas P > --- > +void format_ref(const char *name, const unsigned char *sha1, const char *format, > + unsigned kind) > +{ > + struct ref_array_item *ref_item; > + ref_item = new_ref_array_item(name, sha1, 0); > + ref_item->kind = kind; > + show_ref_array_item(ref_item, format, 0); > + free_array_item(ref_item); > +} > + > /* If no sorting option is given, use refname to sort as default */ > struct ref_sorting *ref_default_sorting(void) > { > diff --git a/ref-filter.h b/ref-filter.h > index 14d435e..1ef7999 100644 > --- a/ref-filter.h > +++ b/ref-filter.h > @@ -107,4 +107,8 @@ struct ref_sorting *ref_default_sorting(void); > /* Function to parse --merged and --no-merged options */ > int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset); > > +/* Pretty-print a single ref */ > +void format_ref(const char *name, const unsigned char *sha1, const char *format, > + unsigned kind); The fact that you felt a need for comment before its name is a strong sign that the name is not sufficiently descriptive and understandable for readers to tell what the function is for. Would pretty_print_ref() or show_ref_pretty() better names, perhaps? ``` ## Junio C Hamano, 2016-09-27 17:36 Subject: Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag Message-ID: URL: https://gitlist.dev/e/xmqqbmz9p7m0.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160926224233.32702-2-santiago@nyu.edu> ``` santiago@nyu.edu writes: > From: Lukas P > > Functions that print git object information may require that the > gpg-interface functions be silent. Add GPG_VERIFY_QUIET flag and prevent > print_signature_buffer from being called if flag is set. > > Signed-off-by: Lukas P Are you and Lukas sure that "Lukas P" is how luk.puehringer wants to be known by the world? Just checking. ``` ## Junio C Hamano, 2016-09-27 17:41 Subject: Re: [PATCH v2 4/5] builtin/verify-tag: add --format to verify-tag Message-ID: URL: https://gitlist.dev/e/xmqq7f9xp7ev.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160926224233.32702-5-santiago@nyu.edu> ``` santiago@nyu.edu writes: > From: Santiago Torres > > Callers of verify-tag may want to cross-check the tagname from refs/tags > with the tagname from the tag object header upon GPG verification. This > is to avoid tag refs that point to an incorrect object. > > Add a --format parameter to git verify-tag to print the formatted tag > object header in addition to or instead of the --verbose or --raw GPG > verification output. > > Signed-off-by: Santiago Torres > --- > builtin/verify-tag.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c > index de10198..a941053 100644 > --- a/builtin/verify-tag.c > +++ b/builtin/verify-tag.c > @@ -12,12 +12,15 @@ > #include > #include "parse-options.h" > #include "gpg-interface.h" > +#include "ref-filter.h" > > static const char * const verify_tag_usage[] = { > - N_("git verify-tag [-v | --verbose] ..."), > + N_("git verify-tag [-v | --verbose] [--format=] ..."), > NULL > }; > > +static char *fmt_pretty; > + I'd suggest to remove this, and then ... > static int git_verify_tag_config(const char *var, const char *value, void *cb) > { > int status = git_gpg_config(var, value, cb); > @@ -33,6 +36,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) ... instead add char *fmt_pretty = NULL; here, imitating the way "&flags" is handled. You do not need it to be visible outside the function. > const struct option verify_tag_options[] = { > OPT__VERBOSE(&verbose, N_("print tag contents")), > OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW), > + OPT_STRING( 0 , "format", &fmt_pretty, N_("format"), N_("format to use for the output")), > OPT_END() > }; > > @@ -46,12 +50,17 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) > if (verbose) > flags |= GPG_VERIFY_VERBOSE; > > + if (fmt_pretty) { > + verify_ref_format(fmt_pretty); > + flags |= GPG_VERIFY_QUIET; > + } > + > while (i < argc) { > unsigned char sha1[20]; > const char *name = argv[i++]; > if (get_sha1(name, sha1)) > had_error = !!error("tag '%s' not found.", name); > - else if (verify_and_format_tag(sha1, name, NULL, flags)) > + else if (verify_and_format_tag(sha1, name, fmt_pretty, flags)) > had_error = 1; > } > return had_error; ``` ## Junio C Hamano, 2016-09-27 17:50 Subject: Re: [PATCH v2 5/5] builtin/tag: add --format argument for tag -v Message-ID: URL: https://gitlist.dev/e/xmqq37klp6yp.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160926224233.32702-6-santiago@nyu.edu> ``` santiago@nyu.edu writes: > From: Lukas P > > Adding --format to git tag -v mutes the default output of the GPG > verification and instead prints the formatted tag object. > This allows callers to cross-check the tagname from refs/tags with > the tagname from the tag object header upon GPG verification. > > Caveat: The change adds a format specifier argument to the > (*each_tag_name_fn) function pointer, i.e. delete_tag now receives this > too, although it does not need it. That's an interesting "caveat". Generally it is a good idea to give an additional opaque pointer to callback functions of iteration API so that code that uses the iteration can pass custom data to its callback. Looking at the way you enhanced each_tag_name_fn, however, you added a specific argument instead; that is the only reason why you need a "caveat". If it were "void *", it would have been in line with the usual practice, not worth mentioning as a "caveat", but could even be advertised as a feature, replacing the last "Caveat" paragraph with something like this: The callback function for for_each_tag_name() didn't allow callers to pass custom data to their callback functions. Add a new opaque pointer to each_tag_name_fn's parameter to allow this. > Signed-off-by: Lukas P > --- > builtin/tag.c | 30 ++++++++++++++++++++---------- > 1 file changed, 20 insertions(+), 10 deletions(-) > > diff --git a/builtin/tag.c b/builtin/tag.c > index 14f3b48..f53227e 100644 > --- a/builtin/tag.c > +++ b/builtin/tag.c > @@ -24,7 +24,7 @@ static const char * const git_tag_usage[] = { > N_("git tag -d ..."), > N_("git tag -l [-n[]] [--contains ] [--points-at ]" > "\n\t\t[--format=] [--[no-]merged []] [...]"), > - N_("git tag -v ..."), > + N_("git tag -v [--format=] ..."), > NULL > }; > > @@ -66,9 +66,10 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con > } > > typedef int (*each_tag_name_fn)(const char *name, const char *ref, > - const unsigned char *sha1); > + const unsigned char *sha1, const char *fmt_pretty); You'd replace "const char *fmt_pretty" with "void *cb_data" here, and... > > -static int for_each_tag_name(const char **argv, each_tag_name_fn fn) > +static int for_each_tag_name(const char **argv, each_tag_name_fn fn, > + const char *fmt_pretty) ... also here. Then introduce fmt_pretty as an auto variable in the function ... > { > const char **p; ... by adding this line here: const char *fmt_pretty = cb_data; > char ref[PATH_MAX]; > @@ -87,14 +88,14 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn) > had_error = 1; > continue; > } > - if (fn(*p, ref, sha1)) > + if (fn(*p, ref, sha1, fmt_pretty)) > had_error = 1; > } > return had_error; > } > > static int delete_tag(const char *name, const char *ref, > - const unsigned char *sha1) > + const unsigned char *sha1, const char *fmt_pretty) And this "const char *fmt_pretty" also becomes "void *cb_data"... > { > if (delete_ref(ref, sha1, 0)) > return 1; > @@ -103,9 +104,15 @@ static int delete_tag(const char *name, const char *ref, > } > > static int verify_tag(const char *name, const char *ref, > - const unsigned char *sha1) > + const unsigned char *sha1, const char *fmt_pretty) ... and here. Reintroduce fmt_pretty as a name local to the function by doing the same thing as for_each_tag_name() above. > { > - return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE); > + int flags; > + flags = GPG_VERIFY_VERBOSE; > + > + if (fmt_pretty) > + flags = GPG_VERIFY_QUIET; > + > + return verify_and_format_tag(sha1, name, fmt_pretty, flags); > } > > static int do_sign(struct strbuf *buffer) > @@ -424,9 +431,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix) > if (filter.merge_commit) > die(_("--merged and --no-merged option are only allowed with -l")); > if (cmdmode == 'd') > - return for_each_tag_name(argv, delete_tag); > - if (cmdmode == 'v') > - return for_each_tag_name(argv, verify_tag); > + return for_each_tag_name(argv, delete_tag, NULL); > + if (cmdmode == 'v') { > + if (format) > + verify_ref_format(format); > + return for_each_tag_name(argv, verify_tag, format); > + } Thanks. ``` ## Lukas Pühringer, 2016-09-27 18:17 Subject: Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag Message-ID: <15BEFDC3-81B2-47FC-A213-F37B4E6020D2@gmail.com> URL: https://gitlist.dev/e/15BEFDC3-81B2-47FC-A213-F37B4E6020D2%40gmail.com In-Reply-To: ``` Thanks for checking. I am fine with Lukas P, unless git prefers full last names. In that case I am fine with changing too. Best, Lukas P > On Sep 27, 2016, at 1:36 PM, Junio C Hamano wrote: > > santiago@nyu.edu writes: > >> From: Lukas P >> >> Functions that print git object information may require that the >> gpg-interface functions be silent. Add GPG_VERIFY_QUIET flag and prevent >> print_signature_buffer from being called if flag is set. >> >> Signed-off-by: Lukas P > > Are you and Lukas sure that "Lukas P" is how luk.puehringer wants to > be known by the world? Just checking. > ``` ## Junio C Hamano, 2016-09-27 18:22 Subject: Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag Message-ID: URL: https://gitlist.dev/e/xmqqponpnqyf.fsf%40gitster.mtv.corp.google.com In-Reply-To: <15BEFDC3-81B2-47FC-A213-F37B4E6020D2@gmail.com> ``` Lukas Pühringer writes: > Thanks for checking. I am fine with Lukas P, unless git prefers > full last names. In that case I am fine with changing too. We do prefer full names, so that it would be consistent with court document when you are involved in copyright inflingement case ;-) ``` ## Lukas Pühringer, 2016-09-27 18:25 Subject: Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag Message-ID: <3D863101-CA73-4718-A1E7-A1664A5BDB58@gmail.com> URL: https://gitlist.dev/e/3D863101-CA73-4718-A1E7-A1664A5BDB58%40gmail.com In-Reply-To: ``` Good, I will change it to 'Lukas Puehringer' then, when we send you the updated batch of patches, that address your latest comments. Thanks, Lukas > On Sep 27, 2016, at 2:22 PM, Junio C Hamano wrote: > > Lukas Pühringer writes: > >> Thanks for checking. I am fine with Lukas P, unless git prefers >> full last names. In that case I am fine with changing too. > > We do prefer full names, so that it would be consistent with court > document when you are involved in copyright inflingement case ;-) ``` ## Stefan Beller, 2016-09-27 18:31 Subject: Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag Message-ID: URL: https://gitlist.dev/e/CAGZ79kYitOd5-Cxr0VzhRH70B%3Dz2gh1q2aB5xiwe%2BHwHuU7dYg%40mail.gmail.com In-Reply-To: <3D863101-CA73-4718-A1E7-A1664A5BDB58@gmail.com> ``` On Tue, Sep 27, 2016 at 11:25 AM, Lukas Pühringer wrote: > Good, I will change it to 'Lukas Puehringer' then, when we send you the updated batch of patches, that address your latest comments. No need to stay full ASCII. German umlauts are fine. (See `git shortlog -s` for all the contributor names, there are also other alphabets in use) Stefan ``` ## Lukas Pühringer, 2016-09-27 18:37 Subject: Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag Message-ID: <618EBDCB-0BD7-493D-AE08-67133B06CEA9@gmail.com> URL: https://gitlist.dev/e/618EBDCB-0BD7-493D-AE08-67133B06CEA9%40gmail.com In-Reply-To: ``` I made it a habit to use ‘ue’ instead of ‘ü' outside of German speaking countries and in coding. It makes my life easier. But thanks for the hint. Lukas > On Sep 27, 2016, at 2:31 PM, Stefan Beller wrote: > > On Tue, Sep 27, 2016 at 11:25 AM, Lukas Pühringer > wrote: >> Good, I will change it to 'Lukas Puehringer' then, when we send you the updated batch of patches, that address your latest comments. > > No need to stay full ASCII. German umlauts are fine. > (See `git shortlog -s` for all the contributor names, there are also > other alphabets in use) > > Stefan ```