git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH v6 0/6] Add --format to tag verification
@ 2017-01-17 23:37 santiago
  2017-01-17 23:37 ` [PATCH v6 1/6] gpg-interface,tag: add GPG_VERIFY_OMIT_STATUS flag santiago
                   ` (6 more replies)
  0 siblings, 7 replies; 20+ messages in thread
From: santiago @ 2017-01-17 23:37 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, sunshine, walters, Santiago Torres

From: Santiago Torres <santiago@nyu.edu>

This is the sixth iteration of [1][2][3][4][5], and as a result of the
discussion in [5]. 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 it.

In this re-woll we:

* Changed the call interface so printing is done outside of verification. 

* Fixed a couple of whitespace issues and whatnot. 

Thanks,
-Santiago

[1] http://public-inbox.org/git/20170115184705.10376-1-santiago@nyu.edu/
[2] http://public-inbox.org/git/20161007210721.20437-1-santiago@nyu.edu/
[3] http://public-inbox.org/git/20160930221806.3398-1-santiago@nyu.edu/
[4] http://public-inbox.org/git/20160922185317.349-1-santiago@nyu.edu/
[5] http://public-inbox.org/git/20160926224233.32702-1-santiago@nyu.edu/
[6] http://public-inbox.org/git/20160607195608.16643-1-santiago@nyu.edu/
[7] http://public-inbox.org/git/20161019203546.dfqmi2czcxopgj6w@sigill.intra.peff.net/
[8] http://public-inbox.org/git/20161019203943.epjxnfci7vcqg4xv@sigill.intra.peff.net/

Lukas Puehringer (3):
  gpg-interface,tag: add GPG_VERIFY_OMIT_STATUS 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                    | 38 ++++++++++++++++++++++++++++----------
 builtin/verify-tag.c             | 23 ++++++++++++++++++++---
 gpg-interface.h                  |  5 +++--
 ref-filter.c                     | 27 +++++++++++++++++++++------
 ref-filter.h                     |  7 +++++++
 t/t7004-tag.sh                   | 16 ++++++++++++++++
 t/t7030-verify-tag.sh            | 16 ++++++++++++++++
 tag.c                            |  5 ++++-
 10 files changed, 117 insertions(+), 24 deletions(-)

-- 
2.11.0


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v6 1/6] gpg-interface,tag: add GPG_VERIFY_OMIT_STATUS flag
  2017-01-17 23:37 [PATCH v6 0/6] Add --format to tag verification santiago
@ 2017-01-17 23:37 ` santiago
  2017-01-17 23:37 ` [PATCH v6 2/6] ref-filter: add function to print single ref_array_item santiago
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 20+ messages in thread
From: santiago @ 2017-01-17 23:37 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, sunshine, walters, Lukas Puehringer

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_OMIT_STATUS 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 | 5 +++--
 tag.c           | 5 ++++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/gpg-interface.h b/gpg-interface.h
index ea68885ad..d2d4fd3a6 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -1,8 +1,9 @@
 #ifndef GPG_INTERFACE_H
 #define GPG_INTERFACE_H
 
-#define GPG_VERIFY_VERBOSE	1
-#define GPG_VERIFY_RAW		2
+#define GPG_VERIFY_VERBOSE		1
+#define GPG_VERIFY_RAW			2
+#define GPG_VERIFY_OMIT_STATUS	4
 
 struct signature_check {
 	char *payload;
diff --git a/tag.c b/tag.c
index d1dcd18cd..243d1fdbb 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_OMIT_STATUS))
+		print_signature_buffer(&sigc, flags);
 
 	signature_check_clear(&sigc);
 	return ret;
-- 
2.11.0


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v6 2/6] ref-filter: add function to print single ref_array_item
  2017-01-17 23:37 [PATCH v6 0/6] Add --format to tag verification santiago
  2017-01-17 23:37 ` [PATCH v6 1/6] gpg-interface,tag: add GPG_VERIFY_OMIT_STATUS flag santiago
@ 2017-01-17 23:37 ` santiago
  2017-01-17 23:37 ` [PATCH v6 3/6] builtin/verify-tag: add --format to verify-tag santiago
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 20+ messages in thread
From: santiago @ 2017-01-17 23:37 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, sunshine, walters, Lukas Puehringer

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 | 27 +++++++++++++++++++++------
 ref-filter.h |  7 +++++++
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 1a978405e..5f4b08792 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1361,7 +1361,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;
 
@@ -1374,11 +1374,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++) {
@@ -1389,6 +1385,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.
@@ -1671,6 +1676,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)
+{
+	struct ref_array_item *ref_item;
+	ref_item = new_ref_array_item(name, sha1, 0);
+	ref_item->kind = ref_kind_from_refname(name);
+	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 fc55fa357..7b05592ba 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -109,4 +109,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);
+
 #endif /*  REF_FILTER_H  */
-- 
2.11.0


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v6 3/6] builtin/verify-tag: add --format to verify-tag
  2017-01-17 23:37 [PATCH v6 0/6] Add --format to tag verification santiago
  2017-01-17 23:37 ` [PATCH v6 1/6] gpg-interface,tag: add GPG_VERIFY_OMIT_STATUS flag santiago
  2017-01-17 23:37 ` [PATCH v6 2/6] ref-filter: add function to print single ref_array_item santiago
@ 2017-01-17 23:37 ` santiago
  2017-01-17 23:37 ` [PATCH v6 4/6] builtin/tag: add --format argument for tag -v santiago
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 20+ messages in thread
From: santiago @ 2017-01-17 23:37 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, sunshine, walters, Santiago Torres

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             | 23 ++++++++++++++++++++---
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-verify-tag.txt b/Documentation/git-verify-tag.txt
index d590edceb..0b8075dad 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 99f8148cf..18443bf9f 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,13 +50,26 @@ 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_OMIT_STATUS;
+	}
+
 	while (i < argc) {
 		unsigned char sha1[20];
 		const char *name = argv[i++];
-		if (get_sha1(name, sha1))
+		if (get_sha1(name, sha1)) {
 			had_error = !!error("tag '%s' not found.", name);
-		else if (gpg_verify_tag(sha1, name, flags))
+			continue;
+		}
+
+		if (gpg_verify_tag(sha1, name, flags)) {
 			had_error = 1;
+			continue;
+		}
+
+		if (fmt_pretty)
+			pretty_print_ref(name, sha1, fmt_pretty);
 	}
 	return had_error;
 }
-- 
2.11.0


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v6 4/6] builtin/tag: add --format argument for tag -v
  2017-01-17 23:37 [PATCH v6 0/6] Add --format to tag verification santiago
                   ` (2 preceding siblings ...)
  2017-01-17 23:37 ` [PATCH v6 3/6] builtin/verify-tag: add --format to verify-tag santiago
@ 2017-01-17 23:37 ` santiago
  2017-01-18  0:02   ` Junio C Hamano
                     ` (2 more replies)
  2017-01-17 23:37 ` [PATCH v6 5/6] t/t7030-verify-tag: Add --format specifier tests santiago
                   ` (2 subsequent siblings)
  6 siblings, 3 replies; 20+ messages in thread
From: santiago @ 2017-01-17 23:37 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, sunshine, walters, Lukas Puehringer

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             | 38 ++++++++++++++++++++++++++++----------
 2 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 76cfe40d9..586aaa315 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 73df72811..b9da72761 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,22 @@ 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 gpg_verify_tag(sha1, name, GPG_VERIFY_VERBOSE);
+	int flags;
+	char *fmt_pretty = cb_data;
+	flags = GPG_VERIFY_VERBOSE;
+
+	if (fmt_pretty)
+		flags = GPG_VERIFY_OMIT_STATUS;
+
+	if (gpg_verify_tag(sha1, name, flags))
+		return -1;
+
+    if (fmt_pretty)
+		pretty_print_ref(name, sha1, fmt_pretty);
+
+	return 0;
 }
 
 static int do_sign(struct strbuf *buffer)
@@ -428,9 +443,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.11.0


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v6 5/6] t/t7030-verify-tag: Add --format specifier tests
  2017-01-17 23:37 [PATCH v6 0/6] Add --format to tag verification santiago
                   ` (3 preceding siblings ...)
  2017-01-17 23:37 ` [PATCH v6 4/6] builtin/tag: add --format argument for tag -v santiago
@ 2017-01-17 23:37 ` santiago
  2017-01-17 23:37 ` [PATCH v6 6/6] t/t7004-tag: " santiago
  2017-01-18  0:07 ` [PATCH v6 0/6] Add --format to tag verification Junio C Hamano
  6 siblings, 0 replies; 20+ messages in thread
From: santiago @ 2017-01-17 23:37 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, sunshine, walters, Santiago Torres

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 07079a41c..d62ccbb98 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.11.0


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v6 6/6] t/t7004-tag: Add --format specifier tests
  2017-01-17 23:37 [PATCH v6 0/6] Add --format to tag verification santiago
                   ` (4 preceding siblings ...)
  2017-01-17 23:37 ` [PATCH v6 5/6] t/t7030-verify-tag: Add --format specifier tests santiago
@ 2017-01-17 23:37 ` santiago
  2017-01-18  0:07 ` [PATCH v6 0/6] Add --format to tag verification Junio C Hamano
  6 siblings, 0 replies; 20+ messages in thread
From: santiago @ 2017-01-17 23:37 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, sunshine, walters, Santiago Torres

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 07869b0c0..ba88b556b 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -874,6 +874,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.11.0


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v
  2017-01-17 23:37 ` [PATCH v6 4/6] builtin/tag: add --format argument for tag -v santiago
@ 2017-01-18  0:02   ` Junio C Hamano
  2017-01-18  0:05   ` Junio C Hamano
  2017-01-18  0:19   ` Junio C Hamano
  2 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2017-01-18  0:02 UTC (permalink / raw)
  To: santiago; +Cc: git, peff, sunshine, walters, Lukas Puehringer

santiago@nyu.edu writes:

> +	if (gpg_verify_tag(sha1, name, flags))
> +		return -1;
> +
> +    if (fmt_pretty)
> +		pretty_print_ref(name, sha1, fmt_pretty);

That's a funny indentation.  I'll fix it up locally while queuing.

> +
> +	return 0;
>  }


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v
  2017-01-17 23:37 ` [PATCH v6 4/6] builtin/tag: add --format argument for tag -v santiago
  2017-01-18  0:02   ` Junio C Hamano
@ 2017-01-18  0:05   ` Junio C Hamano
  2017-01-18  0:19   ` Junio C Hamano
  2 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2017-01-18  0:05 UTC (permalink / raw)
  To: santiago; +Cc: git, peff, sunshine, walters, Lukas Puehringer

santiago@nyu.edu writes:

> -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];
>  
> +

Why?  I'll remove this while queuing.

>  	for (p = argv; *p; p++) {
>  		if (snprintf(ref, sizeof(ref), "refs/tags/%s", *p)
>  					>= sizeof(ref)) {

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 0/6] Add --format to tag verification
  2017-01-17 23:37 [PATCH v6 0/6] Add --format to tag verification santiago
                   ` (5 preceding siblings ...)
  2017-01-17 23:37 ` [PATCH v6 6/6] t/t7004-tag: " santiago
@ 2017-01-18  0:07 ` Junio C Hamano
  6 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2017-01-18  0:07 UTC (permalink / raw)
  To: santiago; +Cc: git, peff, sunshine, walters

santiago@nyu.edu writes:

> From: Santiago Torres <santiago@nyu.edu>
>
> This is the sixth iteration of [1][2][3][4][5], and as a result of the
> discussion in [5]. 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 it.
>
> In this re-woll we:
>
> * Changed the call interface so printing is done outside of verification. 
>
> * Fixed a couple of whitespace issues and whatnot. 

With the small code structure change Peff suggested the result looks
much easier to read.  I didn't spot any more problems.

Will replace what has been sitting in my tree.  Thanks.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v
  2017-01-17 23:37 ` [PATCH v6 4/6] builtin/tag: add --format argument for tag -v santiago
  2017-01-18  0:02   ` Junio C Hamano
  2017-01-18  0:05   ` Junio C Hamano
@ 2017-01-18  0:19   ` Junio C Hamano
  2017-01-18 18:25     ` Junio C Hamano
  2 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2017-01-18  0:19 UTC (permalink / raw)
  To: santiago; +Cc: git, peff, sunshine, walters, Lukas Puehringer

santiago@nyu.edu writes:

> @@ -428,9 +443,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);
> +	}

This triggers:

    builtin/tag.c: In function 'cmd_tag':
    builtin/tag.c:451:3: error: passing argument 3 of
    'for_each_tag_name' discards 'const' qualifier from pointer target type [-Werror]
       return for_each_tag_name(argv, verify_tag, format);

Either for-each-tag-name's new parameter needs to be typed
correctly, or the type of the "format" variable needs to be updated.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v
  2017-01-18  0:19   ` Junio C Hamano
@ 2017-01-18 18:25     ` Junio C Hamano
  2017-01-18 18:28       ` Santiago Torres
  2017-01-18 18:28       ` Jeff King
  0 siblings, 2 replies; 20+ messages in thread
From: Junio C Hamano @ 2017-01-18 18:25 UTC (permalink / raw)
  To: santiago; +Cc: git, peff, sunshine, walters, Lukas Puehringer

Junio C Hamano <gitster@pobox.com> writes:

> santiago@nyu.edu writes:
>
>> @@ -428,9 +443,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);
>> +	}
>
> This triggers:
>
>     builtin/tag.c: In function 'cmd_tag':
>     builtin/tag.c:451:3: error: passing argument 3 of
>     'for_each_tag_name' discards 'const' qualifier from pointer target type [-Werror]
>        return for_each_tag_name(argv, verify_tag, format);
>
> Either for-each-tag-name's new parameter needs to be typed
> correctly, or the type of the "format" variable needs to be updated.

Squashing the following into this commit solves this issue with the
former approach.  The lines it touches are all from 4/6 and I view
all of it as general improvement, including type correctness and
code formatting.

diff --git a/builtin/tag.c b/builtin/tag.c
index f81273a85a..fbb85ba3dc 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -66,10 +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, void *cb_data);
+				const unsigned char *sha1, const void *cb_data);
 
 static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
-		void *cb_data)
+			     const void *cb_data)
 {
 	const char **p;
 	char ref[PATH_MAX];
@@ -95,7 +95,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
 }
 
 static int delete_tag(const char *name, const char *ref,
-				const unsigned char *sha1, void *cb_data)
+		      const unsigned char *sha1, const void *cb_data)
 {
 	if (delete_ref(ref, sha1, 0))
 		return 1;
@@ -104,10 +104,10 @@ static int delete_tag(const char *name, const char *ref,
 }
 
 static int verify_tag(const char *name, const char *ref,
-				const unsigned char *sha1, void *cb_data)
+		      const unsigned char *sha1, const void *cb_data)
 {
 	int flags;
-	char *fmt_pretty = cb_data;
+	const char *fmt_pretty = cb_data;
 	flags = GPG_VERIFY_VERBOSE;
 
 	if (fmt_pretty)

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v
  2017-01-18 18:25     ` Junio C Hamano
@ 2017-01-18 18:28       ` Santiago Torres
  2017-01-18 18:44         ` Junio C Hamano
  2017-01-18 18:52         ` Junio C Hamano
  2017-01-18 18:28       ` Jeff King
  1 sibling, 2 replies; 20+ messages in thread
From: Santiago Torres @ 2017-01-18 18:28 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, sunshine, walters, Lukas Puehringer

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

On Wed, Jan 18, 2017 at 10:25:59AM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > santiago@nyu.edu writes:
> >
> >> @@ -428,9 +443,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);
> >> +	}
> >
> > This triggers:
> >
> >     builtin/tag.c: In function 'cmd_tag':
> >     builtin/tag.c:451:3: error: passing argument 3 of
> >     'for_each_tag_name' discards 'const' qualifier from pointer target type [-Werror]
> >        return for_each_tag_name(argv, verify_tag, format);
> >
> > Either for-each-tag-name's new parameter needs to be typed
> > correctly, or the type of the "format" variable needs to be updated.
> 
> Squashing the following into this commit solves this issue with the
> former approach.  The lines it touches are all from 4/6 and I view
> all of it as general improvement, including type correctness and
> code formatting.

Thanks!

Should I re-roll this really quick? Or would you rather apply this on
your tree directly? 

-Santiago.

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v
  2017-01-18 18:25     ` Junio C Hamano
  2017-01-18 18:28       ` Santiago Torres
@ 2017-01-18 18:28       ` Jeff King
  2017-01-18 18:49         ` Junio C Hamano
  1 sibling, 1 reply; 20+ messages in thread
From: Jeff King @ 2017-01-18 18:28 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: santiago, git, sunshine, walters, Lukas Puehringer

On Wed, Jan 18, 2017 at 10:25:59AM -0800, Junio C Hamano wrote:

> > This triggers:
> >
> >     builtin/tag.c: In function 'cmd_tag':
> >     builtin/tag.c:451:3: error: passing argument 3 of
> >     'for_each_tag_name' discards 'const' qualifier from pointer target type [-Werror]
> >        return for_each_tag_name(argv, verify_tag, format);
> >
> > Either for-each-tag-name's new parameter needs to be typed
> > correctly, or the type of the "format" variable needs to be updated.
> 
> Squashing the following into this commit solves this issue with the
> former approach.  The lines it touches are all from 4/6 and I view
> all of it as general improvement, including type correctness and
> code formatting.
> 
> diff --git a/builtin/tag.c b/builtin/tag.c
> index f81273a85a..fbb85ba3dc 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -66,10 +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, void *cb_data);
> +				const unsigned char *sha1, const void *cb_data);

This would bite us later if one of the iterators really does need to
pass something mutable. But as this iteration interface is confined to
builtin/tag.c, I think it's a nice simple fix.

A more general fix would be to pass a non-const pointer to const pointer
(preferably inside a struct for readability). But I don't see any need
for that complexity here.

-Peff

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v
  2017-01-18 18:28       ` Santiago Torres
@ 2017-01-18 18:44         ` Junio C Hamano
  2017-01-18 18:50           ` Santiago Torres
  2017-01-18 20:16           ` Eric Wong
  2017-01-18 18:52         ` Junio C Hamano
  1 sibling, 2 replies; 20+ messages in thread
From: Junio C Hamano @ 2017-01-18 18:44 UTC (permalink / raw)
  To: Santiago Torres, meta
  Cc: git, peff, sunshine, walters, Lukas Puehringer, Eric Wong

Santiago Torres <santiago@nyu.edu> writes:

<<nothing>>???

Eric, I've noticed that this message

  http://public-inbox.org/git/20170118182831.pkhqu2np3bh2puei@LykOS.localdomain/

and all messages from Santiago appear empty when they come via
public-inbox.org; the reason I suspect we haven't heard much
complaints is because nobody else around here sends multipart/signed
disposition inline other than Santiago.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v
  2017-01-18 18:28       ` Jeff King
@ 2017-01-18 18:49         ` Junio C Hamano
  0 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2017-01-18 18:49 UTC (permalink / raw)
  To: Jeff King; +Cc: santiago, git, sunshine, walters, Lukas Puehringer

Jeff King <peff@peff.net> writes:

>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index f81273a85a..fbb85ba3dc 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -66,10 +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, void *cb_data);
>> +				const unsigned char *sha1, const void *cb_data);
>
> This would bite us later if one of the iterators really does need to
> pass something mutable. But as this iteration interface is confined to
> builtin/tag.c, I think it's a nice simple fix.
>
> A more general fix would be to pass a non-const pointer to const pointer
> (preferably inside a struct for readability). But I don't see any need
> for that complexity here.

My first trial was to loosen the constness of existing variable,
which was OK, but made me feel dirty by turning what does not need
to be mutable into mutable.  The iterator being local made me try
the other way and it turned out that currently there is no need for
mutable callback data ;-)

I agree that this may have to be updated, and if this were more
global thing, we'd better off doing so from the get-go, but for a
calling convention that is limited within a single file, I am more
comfortable saying we'll cross the bridge when we need to.

Thanks.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v
  2017-01-18 18:44         ` Junio C Hamano
@ 2017-01-18 18:50           ` Santiago Torres
  2017-01-18 20:16           ` Eric Wong
  1 sibling, 0 replies; 20+ messages in thread
From: Santiago Torres @ 2017-01-18 18:50 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: meta, git, peff, sunshine, walters, Lukas Puehringer, Eric Wong

On Wed, Jan 18, 2017 at 10:44:03AM -0800, Junio C Hamano wrote:
> Santiago Torres <santiago@nyu.edu> writes:
> 
Was:

Thanks!

Would you want me to re-roll really quick? or would you rather apply
this on your side?

Thanks,
-Santiago.
> 
> Eric, I've noticed that this message
> 
>   http://public-inbox.org/git/20170118182831.pkhqu2np3bh2puei@LykOS.localdomain/
> 
> and all messages from Santiago appear empty when they come via
> public-inbox.org; the reason I suspect we haven't heard much
> complaints is because nobody else around here sends multipart/signed
> disposition inline other than Santiago.
> 

Interesting, I thought I wasn't inlining the .asc. I guess I can disable
signing for this ML for the time being. 

Thanks for letting me know.
-Santiago.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v
  2017-01-18 18:28       ` Santiago Torres
  2017-01-18 18:44         ` Junio C Hamano
@ 2017-01-18 18:52         ` Junio C Hamano
  1 sibling, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2017-01-18 18:52 UTC (permalink / raw)
  To: Santiago Torres; +Cc: git, peff, sunshine, walters, Lukas Puehringer

Santiago Torres <santiago@nyu.edu> writes:

>> Squashing the following into this commit solves this issue with the
>> former approach.  The lines it touches are all from 4/6 and I view
>> all of it as general improvement, including type correctness and
>> code formatting.
>
> Thanks!
>
> Should I re-roll this really quick? Or would you rather apply this on
> your tree directly? 

Nah, local squashing should be sufficient in this case.  The squash
only touches a single patch from the original and it itself is easy
to review (and was reviewed already from what I can tell in this
thread).


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v
  2017-01-18 18:44         ` Junio C Hamano
  2017-01-18 18:50           ` Santiago Torres
@ 2017-01-18 20:16           ` Eric Wong
  2017-01-19  0:37             ` Eric Wong
  1 sibling, 1 reply; 20+ messages in thread
From: Eric Wong @ 2017-01-18 20:16 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Santiago Torres, meta, git, peff, sunshine, walters, Lukas Puehringer

Junio C Hamano <gitster@pobox.com> wrote:
> Santiago Torres <santiago@nyu.edu> writes:
> 
> <<nothing>>???
> 
> Eric, I've noticed that this message
> 
>   http://public-inbox.org/git/20170118182831.pkhqu2np3bh2puei@LykOS.localdomain/
> 
> and all messages from Santiago appear empty when they come via
> public-inbox.org; the reason I suspect we haven't heard much
> complaints is because nobody else around here sends multipart/signed
> disposition inline other than Santiago.

Eeep!  This looks like a regression I introduced when working
around Richard Hansen's S/MIME mails the other week on git@vger:

  https://public-inbox.org/meta/20170110222235.GB27356@dcvr/T/#u

Worse is they now corrupted on the way in into the git repo
because of search indexing.  Will fix ASAP.  Thanks for the
heads up.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v
  2017-01-18 20:16           ` Eric Wong
@ 2017-01-19  0:37             ` Eric Wong
  0 siblings, 0 replies; 20+ messages in thread
From: Eric Wong @ 2017-01-19  0:37 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Santiago Torres, meta, git, peff, sunshine, walters, Lukas Puehringer

Eric Wong <e@80x24.org> wrote:
> Junio C Hamano <gitster@pobox.com> wrote:
> > 
> >   http://public-inbox.org/git/20170118182831.pkhqu2np3bh2puei@LykOS.localdomain/
> 
> Eeep!  This looks like a regression I introduced when working
> around Richard Hansen's S/MIME mails the other week on git@vger:
>
>   https://public-inbox.org/meta/20170110222235.GB27356@dcvr/T/#u

Yep, I copied SUPER and used it improperly in a subclass.  Should be
fixed now, and reimported several messages from my Maildir.  NNTP
readers will see new article numbers for reimported Message-IDs.

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2017-01-19  0:37 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-17 23:37 [PATCH v6 0/6] Add --format to tag verification santiago
2017-01-17 23:37 ` [PATCH v6 1/6] gpg-interface,tag: add GPG_VERIFY_OMIT_STATUS flag santiago
2017-01-17 23:37 ` [PATCH v6 2/6] ref-filter: add function to print single ref_array_item santiago
2017-01-17 23:37 ` [PATCH v6 3/6] builtin/verify-tag: add --format to verify-tag santiago
2017-01-17 23:37 ` [PATCH v6 4/6] builtin/tag: add --format argument for tag -v santiago
2017-01-18  0:02   ` Junio C Hamano
2017-01-18  0:05   ` Junio C Hamano
2017-01-18  0:19   ` Junio C Hamano
2017-01-18 18:25     ` Junio C Hamano
2017-01-18 18:28       ` Santiago Torres
2017-01-18 18:44         ` Junio C Hamano
2017-01-18 18:50           ` Santiago Torres
2017-01-18 20:16           ` Eric Wong
2017-01-19  0:37             ` Eric Wong
2017-01-18 18:52         ` Junio C Hamano
2017-01-18 18:28       ` Jeff King
2017-01-18 18:49         ` Junio C Hamano
2017-01-17 23:37 ` [PATCH v6 5/6] t/t7030-verify-tag: Add --format specifier tests santiago
2017-01-17 23:37 ` [PATCH v6 6/6] t/t7004-tag: " santiago
2017-01-18  0:07 ` [PATCH v6 0/6] Add --format to tag verification Junio C Hamano

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).