[PATCH v4 0/7] Add --format to tag verification

[PATCH v4 0/7] Add --format to tag verification

[santiago]

From: Santiago Torres <santiago@nyu.edu>

This is the fourth iteration of the series in [1][2][3], which comes as a
result of the discussion in [4]. The main goal of this patch series is to bring
--format to git tag verification so that upper-layer tools can inspect the
content of a tag and make decisions based on those contents.

In this re-woll we:

* Fixed the author fields and signed off by's throughout the patch
  series

* Added two more patches with unit tests to ensure the format specifier
  behaves as expected

* Fixed a missing initialization for the format specifier in verify-tag which
  caused a crash.

* Fixed an outdated git commit message that had the previous name of a
  function definition.

Thanks,
-Santiago

[1] http://public-inbox.org/git/20160930221806.3398-1-santiago@nyu.edu/
[2] http://public-inbox.org/git/20160922185317.349-1-santiago@nyu.edu/
[3] http://public-inbox.org/git/20160926224233.32702-1-santiago@nyu.edu/
[4] http://public-inbox.org/git/20160607195608.16643-1-santiago@nyu.edu/


Lukas Puehringer (4):
  tag: add format specifier to gpg_verify_tag
  gpg-interface, tag: add GPG_VERIFY_QUIET flag
  ref-filter: add function to print single ref_array_item
  builtin/tag: add --format argument for tag -v

Santiago Torres (3):
  builtin/verify-tag: add --format to verify-tag
  t/t7030-verify-tag: Add --format specifier tests
  t/t7004-tag: Add --format specifier tests

 Documentation/git-tag.txt        |  2 +-
 Documentation/git-verify-tag.txt |  2 +-
 builtin/tag.c                    | 34 +++++++++++++++++++++++-----------
 builtin/verify-tag.c             | 13 +++++++++++--
 gpg-interface.h                  |  1 +
 ref-filter.c                     | 10 ++++++++++
 ref-filter.h                     |  3 +++
 t/t7004-tag.sh                   | 16 ++++++++++++++++
 t/t7030-verify-tag.sh            | 16 ++++++++++++++++
 tag.c                            | 22 +++++++++++++++-------
 tag.h                            |  4 ++--
 11 files changed, 99 insertions(+), 24 deletions(-)

-- 
2.10.0

[PATCH v4 1/7] gpg-interface, tag: add GPG_VERIFY_QUIET flag

[santiago]

From: Lukas Puehringer <luk.puehringer@gmail.com>

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 Puehringer <luk.puehringer@gmail.com>
---
 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

[PATCH v4 2/7] ref-filter: add function to print single ref_array_item

[santiago]

From: Lukas Puehringer <luk.puehringer@gmail.com>

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 pretty_print_ref function to create, pretty print and free
individual ref-items.

Signed-off-by: Lukas Puehringer <luk.puehringer@gmail.com>
---
 ref-filter.c | 10 ++++++++++
 ref-filter.h |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/ref-filter.c b/ref-filter.c
index 9adbb8a..cfbcd73 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1637,6 +1637,16 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 	putchar('\n');
 }
 
+void pretty_print_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..3d23090 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -107,4 +107,7 @@ 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);
 
+void pretty_print_ref(const char *name, const unsigned char *sha1,
+		const char *format, unsigned kind);
+
 #endif /*  REF_FILTER_H  */
-- 
2.10.0

[PATCH v4 3/7] tag: add format specifier to gpg_verify_tag

[santiago]

From: Lukas Puehringer <luk.puehringer@gmail.com>

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 Puehringer <luk.puehringer@gmail.com>
---
 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..d3512c0 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)
+		pretty_print_ref(name, sha1, fmt_pretty, FILTER_REFS_TAGS);
+
 	return ret;
 }
 
diff --git a/tag.h b/tag.h
index a5721b6..896b9c2 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

[PATCH v4 4/7] builtin/verify-tag: add --format to verify-tag

[santiago]

From: Santiago Torres <santiago@nyu.edu>

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 <santiago@nyu.edu>
---
 Documentation/git-verify-tag.txt |  2 +-
 builtin/verify-tag.c             | 13 +++++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-verify-tag.txt b/Documentation/git-verify-tag.txt
index d590edc..0b8075d 100644
--- a/Documentation/git-verify-tag.txt
+++ b/Documentation/git-verify-tag.txt
@@ -8,7 +8,7 @@ git-verify-tag - Check the GPG signature of tags
 SYNOPSIS
 --------
 [verse]
-'git verify-tag' <tag>...
+'git verify-tag' [--format=<format>] <tag>...
 
 DESCRIPTION
 -----------
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index de10198..745b6a6 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -12,12 +12,14 @@
 #include <signal.h>
 #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] <tag>..."),
+		N_("git verify-tag [-v | --verbose] [--format=<format>] <tag>..."),
 		NULL
 };
 
+
 static int git_verify_tag_config(const char *var, const char *value, void *cb)
 {
 	int status = git_gpg_config(var, value, cb);
@@ -30,9 +32,11 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 {
 	int i = 1, verbose = 0, had_error = 0;
 	unsigned flags = 0;
+	char *fmt_pretty = NULL;
 	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

[PATCH v4 5/7] builtin/tag: add --format argument for tag -v

[santiago]

From: Lukas Puehringer <luk.puehringer@gmail.com>

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.

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 Puehringer <luk.puehringer@gmail.com>
---
 Documentation/git-tag.txt |  2 +-
 builtin/tag.c             | 34 +++++++++++++++++++++++-----------
 builtin/verify-tag.c      |  2 +-
 3 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 7ecca8e..3bb5e3c 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -15,7 +15,7 @@ SYNOPSIS
 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
 	[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
 	[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]
-'git tag' -v <tagname>...
+'git tag' -v [--format=<format>] <tagname>...
 
 DESCRIPTION
 -----------
diff --git a/builtin/tag.c b/builtin/tag.c
index 14f3b48..7730fd0 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -24,7 +24,7 @@ static const char * const git_tag_usage[] = {
 	N_("git tag -d <tagname>..."),
 	N_("git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>]"
 		"\n\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"),
-	N_("git tag -v <tagname>..."),
+	N_("git tag -v [--format=<format>] <tagname>..."),
 	NULL
 };
 
@@ -66,15 +66,17 @@ 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, void *cb_data);
 
-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,
+		void *cb_data)
 {
 	const char **p;
 	char ref[PATH_MAX];
 	int had_error = 0;
 	unsigned char sha1[20];
 
+
 	for (p = argv; *p; p++) {
 		if (snprintf(ref, sizeof(ref), "refs/tags/%s", *p)
 					>= sizeof(ref)) {
@@ -87,14 +89,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, cb_data))
 			had_error = 1;
 	}
 	return had_error;
 }
 
 static int delete_tag(const char *name, const char *ref,
-				const unsigned char *sha1)
+				const unsigned char *sha1, void *cb_data)
 {
 	if (delete_ref(ref, sha1, 0))
 		return 1;
@@ -103,9 +105,16 @@ 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, void *cb_data)
 {
-	return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
+	int flags;
+	char *fmt_pretty = cb_data;
+	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)
@@ -334,7 +343,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct strbuf err = STRBUF_INIT;
 	struct ref_filter filter;
 	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
-	const char *format = NULL;
+	char *format = NULL;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
@@ -424,9 +433,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

[PATCH v4 6/7] t/t7030-verify-tag: Add --format specifier tests

[santiago]

From: Santiago Torres <santiago@nyu.edu>

Verify-tag now provides --format specifiers to inspect and ensure the
contents of the tag are proper. We add two tests to ensure this
functionality works as expected: the return value should indicate if
verification passed, and the format specifiers must be respected.

Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 t/t7030-verify-tag.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index 07079a4..ce37fd9 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -125,4 +125,20 @@ test_expect_success GPG 'verify multiple tags' '
 	test_cmp expect.stderr actual.stderr
 '
 
+test_expect_success 'verifying tag with --format' '
+	cat >expect <<-\EOF &&
+	tagname : fourth-signed
+	EOF
+	git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
+	cat >expect <<-\EOF &&
+	tagname : 7th forged-signed
+	EOF
+	test_must_fail git verify-tag --format="tagname : %(tag)" $(cat forged1.tag) >actual-forged &&
+	test_cmp expect actual-forged
+'
+
 test_done
-- 
2.10.0

[PATCH v4 7/7] t/t7004-tag: Add --format specifier tests

[santiago]

From: Santiago Torres <santiago@nyu.edu>

tag -v now supports --format specifiers to inspect the contents of a tag
upon verification. Add two tests to ensure this behavior is respected in
future changes.

Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 t/t7004-tag.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 8b0f71a..633b089 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -847,6 +847,22 @@ test_expect_success GPG 'verifying a forged tag should fail' '
 	test_must_fail git tag -v forged-tag
 '
 
+test_expect_success 'verifying a proper tag with --format pass and format accordingly' '
+	cat >expect <<-\EOF &&
+	tagname : signed-tag
+	EOF
+	git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
+	cat >expect <<-\EOF &&
+	tagname : forged-tag
+	EOF
+	test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" >actual &&
+	test_cmp expect actual
+'
+
 # blank and empty messages for signed tags:
 
 get_tag_header empty-signed-tag $commit commit $time >expect
-- 
2.10.0

Re: [PATCH v4 0/7] Add --format to tag verification

[Santiago Torres]

[-- Attachment #1: Type: text/plain, Size: 2504 bytes --]

Hi,

I noticed there were no replies for this thread. I was curious if it got
buried because I sent it on the Friday evening before a long weekend.

I don't mean to pressure or anything.

Thanks!
-Santiago.


On Fri, Oct 07, 2016 at 05:07:14PM -0400, santiago@nyu.edu wrote:
> From: Santiago Torres <santiago@nyu.edu>
> 
> This is the fourth iteration of the series in [1][2][3], which comes as a
> result of the discussion in [4]. The main goal of this patch series is to bring
> --format to git tag verification so that upper-layer tools can inspect the
> content of a tag and make decisions based on those contents.
> 
> In this re-woll we:
> 
> * Fixed the author fields and signed off by's throughout the patch
>   series
> 
> * Added two more patches with unit tests to ensure the format specifier
>   behaves as expected
> 
> * Fixed a missing initialization for the format specifier in verify-tag which
>   caused a crash.
> 
> * Fixed an outdated git commit message that had the previous name of a
>   function definition.
> 
> Thanks,
> -Santiago
> 
> [1] http://public-inbox.org/git/20160930221806.3398-1-santiago@nyu.edu/
> [2] http://public-inbox.org/git/20160922185317.349-1-santiago@nyu.edu/
> [3] http://public-inbox.org/git/20160926224233.32702-1-santiago@nyu.edu/
> [4] http://public-inbox.org/git/20160607195608.16643-1-santiago@nyu.edu/
> 
> 
> Lukas Puehringer (4):
>   tag: add format specifier to gpg_verify_tag
>   gpg-interface, tag: add GPG_VERIFY_QUIET flag
>   ref-filter: add function to print single ref_array_item
>   builtin/tag: add --format argument for tag -v
> 
> Santiago Torres (3):
>   builtin/verify-tag: add --format to verify-tag
>   t/t7030-verify-tag: Add --format specifier tests
>   t/t7004-tag: Add --format specifier tests
> 
>  Documentation/git-tag.txt        |  2 +-
>  Documentation/git-verify-tag.txt |  2 +-
>  builtin/tag.c                    | 34 +++++++++++++++++++++++-----------
>  builtin/verify-tag.c             | 13 +++++++++++--
>  gpg-interface.h                  |  1 +
>  ref-filter.c                     | 10 ++++++++++
>  ref-filter.h                     |  3 +++
>  t/t7004-tag.sh                   | 16 ++++++++++++++++
>  t/t7030-verify-tag.sh            | 16 ++++++++++++++++
>  tag.c                            | 22 +++++++++++++++-------
>  tag.h                            |  4 ++--
>  11 files changed, 99 insertions(+), 24 deletions(-)
> 
> -- 
> 2.10.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v4 1/7] gpg-interface, tag: add GPG_VERIFY_QUIET flag

[Jeff King]

On Fri, Oct 07, 2016 at 05:07:15PM -0400, santiago@nyu.edu wrote:

> From: Lukas Puehringer <luk.puehringer@gmail.com>
> 
> 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.

The layering here is a little funny. The gpg-interface code allocates a
new flag, but none of its functions do anything with it.  Instead, it's
acted on only by the run_gpg_verify() command local to tag.c.

I guess this "flags" variable comes to us from other callsites via
gpg_verify_tag. That's still in tag.c, but it's arguably part of the gpg
interface.

So I think it's OK.

-Peff

Re: [PATCH v4 2/7] ref-filter: add function to print single ref_array_item

[Jeff King]

On Fri, Oct 07, 2016 at 05:07:16PM -0400, santiago@nyu.edu wrote:

> From: Lukas Puehringer <luk.puehringer@gmail.com>
> 
> 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 pretty_print_ref function to create, pretty print and free
> individual ref-items.

Makes sense.

> diff --git a/ref-filter.h b/ref-filter.h
> index 14d435e..3d23090 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -107,4 +107,7 @@ 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);
>  
> +void pretty_print_ref(const char *name, const unsigned char *sha1,
> +		const char *format, unsigned kind);
> +

What are the possible values for "kind"? I guess these should come from
FILTER_REFS_TAGS, BRANCHES, etc. It's probably worth documenting that.
Alternatively, is it possible to just determine this from the name? It
looks like filter_ref_kind() is how it happens for a normal ref-filter.

-Peff

Re: [PATCH v4 4/7] builtin/verify-tag: add --format to verify-tag

[Jeff King]

On Fri, Oct 07, 2016 at 05:07:18PM -0400, santiago@nyu.edu wrote:

> @@ -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;
> +	}

I see why you would want to disable the normal output when there is a
custom format. But it seems a shame that the GPG information cannot be
retrieved as part of that format.

I think in the long run we'd want something like pretty.c's "%G"
placeholders for the ref-filter pretty-printer. But I think we are OK to
do this patch without it. It allows at least:

  tag=v2.0.0
  got=$(git verify-tag --format='%(tag)' "$tag") &&
  test "$tag" = "$got" ||
  echo >&2 "verification failed"

which is better than what we have now, and can be extended in the
future.

-Peff

Re: [PATCH v4 5/7] builtin/tag: add --format argument for tag -v

[Jeff King]

On Fri, Oct 07, 2016 at 05:07:19PM -0400, santiago@nyu.edu wrote:

> Adding --format to git tag -v mutes the default output of the GPG
> verification and instead prints the formatted tag object.

The same comments apply to "mutes" here, as to the previous patch (which
is to say I think we may want something more, but this is probably OK
for now).

> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 7ecca8e..3bb5e3c 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -15,7 +15,7 @@ SYNOPSIS
>  'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
>  	[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
>  	[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]
> -'git tag' -v <tagname>...
> +'git tag' -v [--format=<format>] <tagname>...

Just thinking out loud, but if we had ref-filter placeholders that
triggered GPG verification, you could do this all with the listing mode,
like:

  git tag --format='%(gpgstatus) %(tag) %(refname:short)'

and verify multiple tags (or give a single tag to limit it to just one).

I don't think that's any kind of blocker for this series. We already
have "-v", and adding --format to it is reasonable, even if we
eventually move to a world where people can use the listing mode. Like I
said, just thinking out loud.

> +static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
> +		void *cb_data)
>  {
>  	const char **p;
>  	char ref[PATH_MAX];
>  	int had_error = 0;
>  	unsigned char sha1[20];
>  
> +
>  	for (p = argv; *p; p++) {

Extra space?

>  static int verify_tag(const char *name, const char *ref,
> -				const unsigned char *sha1)
> +				const unsigned char *sha1, void *cb_data)
>  {
> -	return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
> +	int flags;

Probably doesn't matter much, but these flags are defined as "unsigned"
elsewhere.

-Peff

Re: [PATCH v4 6/7] t/t7030-verify-tag: Add --format specifier tests

[Jeff King]

On Fri, Oct 07, 2016 at 05:07:20PM -0400, santiago@nyu.edu wrote:

> Verify-tag now provides --format specifiers to inspect and ensure the
> contents of the tag are proper. We add two tests to ensure this
> functionality works as expected: the return value should indicate if
> verification passed, and the format specifiers must be respected.

Sounds good.

> +test_expect_success 'verifying tag with --format' '
> +	cat >expect <<-\EOF &&
> +	tagname : fourth-signed
> +	EOF
> +	git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
> +	test_cmp expect actual
> +'

I suppose it's a matter of style, but for a single-line expectation I
would just do:

  echo "tagname : fourth-signed" >expect &&

which is shorter and saves a process invocation.

Ditto on the next patch (which IMHO could just be squashed into a single
patch).

-Peff

Re: [PATCH v4 2/7] ref-filter: add function to print single ref_array_item

[Jeff King]

On Wed, Oct 19, 2016 at 04:55:43AM -0400, Jeff King wrote:

> > diff --git a/ref-filter.h b/ref-filter.h
> > index 14d435e..3d23090 100644
> > --- a/ref-filter.h
> > +++ b/ref-filter.h
> > @@ -107,4 +107,7 @@ 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);
> >  
> > +void pretty_print_ref(const char *name, const unsigned char *sha1,
> > +		const char *format, unsigned kind);
> > +
> 
> What are the possible values for "kind"? I guess these should come from
> FILTER_REFS_TAGS, BRANCHES, etc. It's probably worth documenting that.
> Alternatively, is it possible to just determine this from the name? It
> looks like filter_ref_kind() is how it happens for a normal ref-filter.

I guess that may complicate things for the caller you add in this
series, which may not have a fully-qualified refname (which is obviously
how filter_ref_kind() figures it out). I'd argue that is a bug, though,
as things like "%(refname)" are generally expected to print out the
fully refname ("git tag --format=%(refname)" does so, and you can use
"%(refname:short)" if you want the shorter part).

-Peff

Re: [PATCH v4 2/7] ref-filter: add function to print single ref_array_item

[Santiago Torres]

[-- Attachment #1: Type: text/plain, Size: 1623 bytes --]

On Wed, Oct 19, 2016 at 05:16:42AM -0400, Jeff King wrote:
> On Wed, Oct 19, 2016 at 04:55:43AM -0400, Jeff King wrote:
> 
> > > diff --git a/ref-filter.h b/ref-filter.h
> > > index 14d435e..3d23090 100644
> > > --- a/ref-filter.h
> > > +++ b/ref-filter.h
> > > @@ -107,4 +107,7 @@ 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);
> > >  
> > > +void pretty_print_ref(const char *name, const unsigned char *sha1,
> > > +		const char *format, unsigned kind);
> > > +
> > 
> > What are the possible values for "kind"? I guess these should come from
> > FILTER_REFS_TAGS, BRANCHES, etc. It's probably worth documenting that.
> > Alternatively, is it possible to just determine this from the name? It
> > looks like filter_ref_kind() is how it happens for a normal ref-filter.
> 
> I guess that may complicate things for the caller you add in this
> series, which may not have a fully-qualified refname (which is obviously
> how filter_ref_kind() figures it out). I'd argue that is a bug, though,
> as things like "%(refname)" are generally expected to print out the
> fully refname ("git tag --format=%(refname)" does so, and you can use
> "%(refname:short)" if you want the shorter part).

Hmm, I hadn't actually noticed that. Do you have any suggestions in how to
address this?

In general this feels like a consequence of disambiguating .git/tags/*
within builtin/tag.c rather than letting ref-filter figure it out.

Thanks,
-Santiago.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v4 2/7] ref-filter: add function to print single ref_array_item

[Jeff King]

On Wed, Oct 19, 2016 at 01:07:34PM -0400, Santiago Torres wrote:

> > I guess that may complicate things for the caller you add in this
> > series, which may not have a fully-qualified refname (which is obviously
> > how filter_ref_kind() figures it out). I'd argue that is a bug, though,
> > as things like "%(refname)" are generally expected to print out the
> > fully refname ("git tag --format=%(refname)" does so, and you can use
> > "%(refname:short)" if you want the shorter part).
> 
> Hmm, I hadn't actually noticed that. Do you have any suggestions in how to
> address this?
> 
> In general this feels like a consequence of disambiguating .git/tags/*
> within builtin/tag.c rather than letting ref-filter figure it out.

The partial solution would look like something below. It's not too bad
because git-tag always knows that it's working a ref in the refs/tags
namespace (and we don't even have to qualify it ourselves,
for_each_tag_name already does it for us).

But verify-tag feeds arbitrary sha1 expressions. See the notes in the
second patch.

  [1/2]: ref-filter: split ref_kind_from_filter
  [2/2]: tag: send fully qualified refnames to verify_tag_and_format

 builtin/tag.c |  2 +-
 ref-filter.c  | 21 +++++++++++++--------
 ref-filter.h  |  6 +++++-
 tag.c         |  2 +-
 4 files changed, 20 insertions(+), 11 deletions(-)

-Peff

[PATCH 1/2] ref-filter: split ref_kind_from_filter

[Jeff King]

This function does two things: if we know we are filtering
only a certain kind of ref, then we can immediately know
that we have that kind. If not, then we compute the kind
from the fully-qualified refname. The latter half is useful
for other callers; let's split it out.

Signed-off-by: Jeff King <peff@peff.net>
---
 ref-filter.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index cfbcd73..77ec9de 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1329,7 +1329,7 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
 	return ref;
 }
 
-static int filter_ref_kind(struct ref_filter *filter, const char *refname)
+static int ref_kind_from_refname(const char *refname)
 {
 	unsigned int i;
 
@@ -1342,11 +1342,7 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname)
 		{ "refs/tags/", FILTER_REFS_TAGS}
 	};
 
-	if (filter->kind == FILTER_REFS_BRANCHES ||
-	    filter->kind == FILTER_REFS_REMOTES ||
-	    filter->kind == FILTER_REFS_TAGS)
-		return filter->kind;
-	else if (!strcmp(refname, "HEAD"))
+	if (!strcmp(refname, "HEAD"))
 		return FILTER_REFS_DETACHED_HEAD;
 
 	for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
@@ -1357,6 +1353,15 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname)
 	return FILTER_REFS_OTHERS;
 }
 
+static int filter_ref_kind(struct ref_filter *filter, const char *refname)
+{
+	if (filter->kind == FILTER_REFS_BRANCHES ||
+	    filter->kind == FILTER_REFS_REMOTES ||
+	    filter->kind == FILTER_REFS_TAGS)
+		return filter->kind;
+	return ref_kind_from_refname(refname);
+}
+
 /*
  * A call-back given to for_each_ref().  Filter refs and keep them for
  * later object processing.
-- 
2.10.1.619.g16351a7

[PATCH 2/2] tag: send fully qualified refnames to verify_tag_and_format

[Jeff King]

The ref-filter code generally expects to see fully qualified
refs, so that things like "%(refname)" and "%(refname:short)"
work as expected. We can do so easily from git-tag, which
always works with refnames in the refs/tags namespace. As a
bonus, we can drop the "kind" parameter from
pretty_print_ref() and just deduce it automatically.

Unfortunately, things are not so simple for verify-tag,
which takes an arbitrary sha1 expression. It has no clue if
a refname as used or not, and whether it was in the
refs/tags namespace.

In an ideal world, get_sha1_with_context() would optionally
tell us about any refs we resolved while it was working, and
we could just feed that refname (and then in cases where we
didn't use a ref at all, like a bare sha1, we could fallback
to just showing the sha1 name the user gave us).

Signed-off-by: Jeff King <peff@peff.net>
---
I think you'd really just squash the various bits of this into your
series at the right spots, though I don't mind it on top, either.

The big question is to what degree we should care about the verify-tag
case. I don't think it's any worse off with this change than it is with
your series (its "kind" becomes "OTHER", but I don't think that is
actually used for display at all; the name remains the same). I'd be OK
with leaving it like this, as a known bug, until get_sha1_with_context()
learns to tell us about the ref. It's an unhandled corner case in a
brand-new feature, not a regression in an existing one.

 builtin/tag.c | 2 +-
 ref-filter.c  | 4 ++--
 ref-filter.h  | 6 +++++-
 tag.c         | 2 +-
 4 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 49aeb50..18eab7e 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -114,7 +114,7 @@ static int verify_tag(const char *name, const char *ref,
 	if (fmt_pretty)
 		flags = GPG_VERIFY_QUIET;
 
-	return verify_and_format_tag(sha1, name, fmt_pretty, flags);
+	return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
 }
 
 static int do_sign(struct strbuf *buffer)
diff --git a/ref-filter.c b/ref-filter.c
index 77ec9de..74da17a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1643,11 +1643,11 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 }
 
 void pretty_print_ref(const char *name, const unsigned char *sha1,
-		const char *format, unsigned kind)
+		      const char *format)
 {
 	struct ref_array_item *ref_item;
 	ref_item = new_ref_array_item(name, sha1, 0);
-	ref_item->kind = kind;
+	ref_item->kind = ref_kind_from_refname(name);
 	show_ref_array_item(ref_item, format, 0);
 	free_array_item(ref_item);
 }
diff --git a/ref-filter.h b/ref-filter.h
index 3d23090..fed2f5e 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -107,7 +107,11 @@ 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);
 
+/*
+ * Print a single ref, outside of any ref-filter. Note that the
+ * name must be a fully qualified refname.
+ */
 void pretty_print_ref(const char *name, const unsigned char *sha1,
-		const char *format, unsigned kind);
+		      const char *format);
 
 #endif /*  REF_FILTER_H  */
diff --git a/tag.c b/tag.c
index d3512c0..d5a7cfb 100644
--- a/tag.c
+++ b/tag.c
@@ -62,7 +62,7 @@ int verify_and_format_tag(const unsigned char *sha1, const char *name,
 	free(buf);
 
 	if (fmt_pretty)
-		pretty_print_ref(name, sha1, fmt_pretty, FILTER_REFS_TAGS);
+		pretty_print_ref(name, sha1, fmt_pretty);
 
 	return ret;
 }
-- 
2.10.1.619.g16351a7

Re: [PATCH 1/2] ref-filter: split ref_kind_from_filter

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Subject: Re: [PATCH 1/2] ref-filter: split ref_kind_from_filter

I think you meant ref_kind_from_refname() ;-)

Looks like a good idea.

> This function does two things: if we know we are filtering
> only a certain kind of ref, then we can immediately know
> that we have that kind. If not, then we compute the kind
> from the fully-qualified refname. The latter half is useful
> for other callers; let's split it out.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  ref-filter.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index cfbcd73..77ec9de 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1329,7 +1329,7 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
>  	return ref;
>  }
>  
> -static int filter_ref_kind(struct ref_filter *filter, const char *refname)
> +static int ref_kind_from_refname(const char *refname)
>  {
>  	unsigned int i;
>  
> @@ -1342,11 +1342,7 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname)
>  		{ "refs/tags/", FILTER_REFS_TAGS}
>  	};
>  
> -	if (filter->kind == FILTER_REFS_BRANCHES ||
> -	    filter->kind == FILTER_REFS_REMOTES ||
> -	    filter->kind == FILTER_REFS_TAGS)
> -		return filter->kind;
> -	else if (!strcmp(refname, "HEAD"))
> +	if (!strcmp(refname, "HEAD"))
>  		return FILTER_REFS_DETACHED_HEAD;
>  
>  	for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
> @@ -1357,6 +1353,15 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname)
>  	return FILTER_REFS_OTHERS;
>  }
>  
> +static int filter_ref_kind(struct ref_filter *filter, const char *refname)
> +{
> +	if (filter->kind == FILTER_REFS_BRANCHES ||
> +	    filter->kind == FILTER_REFS_REMOTES ||
> +	    filter->kind == FILTER_REFS_TAGS)
> +		return filter->kind;
> +	return ref_kind_from_refname(refname);
> +}
> +
>  /*
>   * A call-back given to for_each_ref().  Filter refs and keep them for
>   * later object processing.

Re: [PATCH 1/2] ref-filter: split ref_kind_from_filter

[Jeff King]

On Wed, Oct 19, 2016 at 02:23:41PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Subject: Re: [PATCH 1/2] ref-filter: split ref_kind_from_filter
> 
> I think you meant ref_kind_from_refname() ;-)
> 
> Looks like a good idea.

Heh, I actually meant filter_ref_kind(), which is the original function.
But any name that is actually a real function would do. :)

-Peff

Re: [PATCH 2/2] tag: send fully qualified refnames to verify_tag_and_format

[Santiago Torres]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]

On Wed, Oct 19, 2016 at 04:39:44PM -0400, Jeff King wrote:
> The ref-filter code generally expects to see fully qualified
> refs, so that things like "%(refname)" and "%(refname:short)"
> work as expected. We can do so easily from git-tag, which
> always works with refnames in the refs/tags namespace. As a
> bonus, we can drop the "kind" parameter from
> pretty_print_ref() and just deduce it automatically.
> 
> Unfortunately, things are not so simple for verify-tag,
> which takes an arbitrary sha1 expression. It has no clue if
> a refname as used or not, and whether it was in the
> refs/tags namespace.
> 
> In an ideal world, get_sha1_with_context() would optionally
> tell us about any refs we resolved while it was working, and
> we could just feed that refname (and then in cases where we
> didn't use a ref at all, like a bare sha1, we could fallback
> to just showing the sha1 name the user gave us).
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I think you'd really just squash the various bits of this into your
> series at the right spots, though I don't mind it on top, either.
> 
> The big question is to what degree we should care about the verify-tag
> case. I don't think it's any worse off with this change than it is with
> your series (its "kind" becomes "OTHER", but I don't think that is
> actually used for display at all; the name remains the same). I'd be OK
> with leaving it like this, as a known bug, until get_sha1_with_context()
> learns to tell us about the ref. It's an unhandled corner case in a
> brand-new feature, not a regression in an existing one.

I see now, I think I can sprinkle some of these changes on 2/7 then. The
rest should be doing 4/7 and 5/7. Does this sound ok?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/2] tag: send fully qualified refnames to verify_tag_and_format

[Jeff King]

On Thu, Oct 20, 2016 at 12:57:01PM -0400, Santiago Torres wrote:

> > I think you'd really just squash the various bits of this into your
> > series at the right spots, though I don't mind it on top, either.
> > 
> > The big question is to what degree we should care about the verify-tag
> > case. I don't think it's any worse off with this change than it is with
> > your series (its "kind" becomes "OTHER", but I don't think that is
> > actually used for display at all; the name remains the same). I'd be OK
> > with leaving it like this, as a known bug, until get_sha1_with_context()
> > learns to tell us about the ref. It's an unhandled corner case in a
> > brand-new feature, not a regression in an existing one.
> 
> I see now, I think I can sprinkle some of these changes on 2/7 then. The
> rest should be doing 4/7 and 5/7. Does this sound ok?

Yep, that sounds about right.

-Peff