On Sat, Apr 02, 2016 at 07:16:15PM -0400, santiago@xxxxxxx wrote:
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 1705c94..3dffdff 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -65,9 +65,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, unsigned flags);
I'm not sure it's a good idea to add a flags field here; most of the
callbacks don't use it, and as you probably noticed, it makes the patch
a lot noisier. It does let you directly use pgp_verify_tag like this:
> if (cmdmode == 'v')
> - return for_each_tag_name(argv, verify_tag);
> + return for_each_tag_name(argv, pgp_verify_tag,
> + GPG_VERIFY_VERBOSE);
but I think that is coupling too closely. What happens later when the
public, multi-file pgp_verify_tag function changes its interface? Or we
want to change our interface here, and it no longer matches
pgp_verify_tag? The results ripple a lot further than they should.
I think you probably want to keep a simple adapter callback in this
file, like:
int verify_tag(const char *name, const char *ref, const unsigned char *sha1)
{
return pgp_verify_tag(name, GPG_VERIFY_VERBOSE));
}
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index f776778..8abc357 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -30,6 +30,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
> {
> int i = 1, verbose = 0, had_error = 0;
> unsigned flags = 0;
> + unsigned char sha1[20];
> + const char *name;
> 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),
> @@ -46,8 +48,16 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
> if (verbose)
> flags |= GPG_VERIFY_VERBOSE;
>
> - while (i < argc)
> - if (pgp_verify_tag(argv[i++], flags))
> + while (i < argc) {
> + name = argv[i++];
> + if (get_sha1(name, sha1)) {
> + error("tag '%s' not found.", name);
> had_error = 1;
> + }
> +
> + if (pgp_verify_tag(name, NULL, sha1, flags))
> + had_error = 1;
> +
> + }
So this is a good example of the rippling I mentioned earlier.
As a side note, it might actually be an improvement for pgp_verify_tag
to take a sha1 (so that git-tag is sure that it is verifying the same
object that it is printing), but that refactoring should probably come
separately, I think.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html