[PATCH/RFC 0/5] libify verify_tag()

[PATCH/RFC 0/5] libify verify_tag()

[Jonathan Nieder]

Hi,

I noticed that ‘git tag -v’ runs git-verify-tag (dashed form), and while
fixing that, I thought, why not make cmd_tag() run verify_tag()
directly?  Here is what that would look like.

The performance impact is pretty negligible: t/t7004-tag.sh is about
2% faster after the change here (with hot or cold cache, it doesn’t
matter).

What do you think?
Jonathan Nieder (5):
  tag: Run ‘git verify-tag’ using its undashed name
  verify-tag: use sigchain library to block SIGPIPE
  Expose verify_tag()
  tag: Do not spawn a separate process for verify-tag
  verify_tag_signature(): let caller look up tag object sha1

 Makefile                             |    1 +
 builtin/tag.c                        |    6 +--
 builtin/verify-tag.c                 |   84 ++++------------------------------
 tag.h                                |    2 +
 builtin/verify-tag.c => verify-tag.c |   49 ++++----------------
 5 files changed, 22 insertions(+), 120 deletions(-)
 copy builtin/verify-tag.c => verify-tag.c (61%)

[PATCH 1/5] tag -v: use ‘git verify-tag’ without dash

[Jonathan Nieder]

There is not much reason to avoid dashed command names here except
setting a good example, but setting a good example is reason enough.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/tag.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 4ef1c4f..d56c882 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -147,9 +147,9 @@ 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 char *argv_verify_tag[] = {"git-verify-tag",
+	const char *argv_verify_tag[] = {"git", "verify-tag",
 					"-v", "SHA1_HEX", NULL};
-	argv_verify_tag[2] = sha1_to_hex(sha1);
+	argv_verify_tag[3] = sha1_to_hex(sha1);
 
 	if (run_command_v_opt(argv_verify_tag, 0))
 		return error("could not verify the tag '%s'", name);
-- 
1.7.0.2

Re: [PATCH 1/5] tag -v: use ‘git verify-tag’ without dash

[Johannes Sixt]

On Samstag, 20. März 2010, Jonathan Nieder wrote:
> -	const char *argv_verify_tag[] = {"git-verify-tag",
> +	const char *argv_verify_tag[] = {"git", "verify-tag",
>  					"-v", "SHA1_HEX", NULL};
> -	argv_verify_tag[2] = sha1_to_hex(sha1);
> +	argv_verify_tag[3] = sha1_to_hex(sha1);
>
>  	if (run_command_v_opt(argv_verify_tag, 0))
>  		return error("could not verify the tag '%s'", name);

This should rather be changed to run_command_v_opt(..., RUN_GIT_CMD). See 
other examples in the code.

Oh, you remove this in a later patch. Then why have this patch at all?

-- Hannes

Re: [PATCH 1/5] tag -v: use ‘git verify-tag’ without dash

[Jonathan Nieder]

Johannes Sixt wrote:
> On Samstag, 20. März 2010, Jonathan Nieder wrote:

>> -	const char *argv_verify_tag[] = {"git-verify-tag",
>> +	const char *argv_verify_tag[] = {"git", "verify-tag",
>>  					"-v", "SHA1_HEX", NULL};
>> -	argv_verify_tag[2] = sha1_to_hex(sha1);
>> +	argv_verify_tag[3] = sha1_to_hex(sha1);
>>
>>  	if (run_command_v_opt(argv_verify_tag, 0))
>>  		return error("could not verify the tag '%s'", name);
>
> This should rather be changed to run_command_v_opt(..., RUN_GIT_CMD). See 
> other examples in the code.

Neat.  Thanks for the pointer.

> Oh, you remove this in a later patch. Then why have this patch at all?

If the rest of the patches go in the right direction, this one should
be dropped.  I should presented it as two alternative series instead:
one that follows the appropriate calling convention here, and one that
libifies verify_tag.

In other words, no good reason at all; thanks for pointing it out.
I’ll send a version of patch 4/5 that doesn’t depend on this one in a
moment.

Jonathan

[PATCH 2/5] verify-tag: use sigchain library to block SIGPIPE

[Jonathan Nieder]

Without this change, callers cannot use verify_tag() safely without
blocking SIGPIPE themselves.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/verify-tag.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 9f482c2..91dd1c1 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -10,6 +10,7 @@
 #include "tag.h"
 #include "run-command.h"
 #include <signal.h>
+#include "sigchain.h"
 #include "parse-options.h"
 
 static const char * const verify_tag_usage[] = {
@@ -54,8 +55,15 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
 		return error("could not run gpg.");
 	}
 
+	/*
+	 * gpg will stop as soon as it knows the signature is bad,
+	 * which can result in SIGPIPE.
+	 */
+	sigchain_push(SIGPIPE, SIG_IGN);
 	write_in_full(gpg.in, buf, len);
 	close(gpg.in);
+	sigchain_pop(SIGPIPE);
+
 	ret = finish_command(&gpg);
 
 	unlink_or_warn(path);
@@ -104,9 +112,6 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 	if (argc <= i)
 		usage_with_options(verify_tag_usage, verify_tag_options);
 
-	/* sometimes the program was terminated because this signal
-	 * was received in the process of writing the gpg input: */
-	signal(SIGPIPE, SIG_IGN);
 	while (i < argc)
 		if (verify_tag(argv[i++], verbose))
 			had_error = 1;
-- 
1.7.0.2

[PATCH 3/5] Expose verify_tag()

[Jonathan Nieder]

Expose the verify_tag() function so ‘git tag -v’ can use it directly
in the future.  verify_tag() already frees all the memory it allocates
and does not call any functions that can exit, so this should be safe.

The function is renamed to verify_tag_signature() for clarity and to
avoid conflicting with builtin/tag.c and builtin/mktag.c’s unrelated
verify_tag().

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile                             |    1 +
 builtin/verify-tag.c                 |   85 +---------------------------------
 tag.h                                |    1 +
 builtin/verify-tag.c => verify-tag.c |   34 +-------------
 4 files changed, 4 insertions(+), 117 deletions(-)
 copy builtin/verify-tag.c => verify-tag.c (71%)

diff --git a/Makefile b/Makefile
index 7c616f8..ed58261 100644
--- a/Makefile
+++ b/Makefile
@@ -616,6 +616,7 @@ LIB_OBJS += unpack-trees.o
 LIB_OBJS += usage.o
 LIB_OBJS += userdiff.o
 LIB_OBJS += utf8.o
+LIB_OBJS += verify-tag.o
 LIB_OBJS += walker.o
 LIB_OBJS += wrapper.o
 LIB_OBJS += write_or_die.o
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 91dd1c1..ca075bd 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -5,12 +5,8 @@
  *
  * Based on git-verify-tag.sh
  */
-#include "cache.h"
 #include "builtin.h"
 #include "tag.h"
-#include "run-command.h"
-#include <signal.h>
-#include "sigchain.h"
 #include "parse-options.h"
 
 static const char * const verify_tag_usage[] = {
@@ -18,85 +14,6 @@ static const char * const verify_tag_usage[] = {
 		NULL
 };
 
-#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
-
-static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
-{
-	struct child_process gpg;
-	const char *args_gpg[] = {"gpg", "--verify", "FILE", "-", NULL};
-	char path[PATH_MAX], *eol;
-	size_t len;
-	int fd, ret;
-
-	fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX");
-	if (fd < 0)
-		return error("could not create temporary file '%s': %s",
-						path, strerror(errno));
-	if (write_in_full(fd, buf, size) < 0)
-		return error("failed writing temporary file '%s': %s",
-						path, strerror(errno));
-	close(fd);
-
-	/* find the length without signature */
-	len = 0;
-	while (len < size && prefixcmp(buf + len, PGP_SIGNATURE)) {
-		eol = memchr(buf + len, '\n', size - len);
-		len += eol ? eol - (buf + len) + 1 : size - len;
-	}
-	if (verbose)
-		write_in_full(1, buf, len);
-
-	memset(&gpg, 0, sizeof(gpg));
-	gpg.argv = args_gpg;
-	gpg.in = -1;
-	args_gpg[2] = path;
-	if (start_command(&gpg)) {
-		unlink(path);
-		return error("could not run gpg.");
-	}
-
-	/*
-	 * gpg will stop as soon as it knows the signature is bad,
-	 * which can result in SIGPIPE.
-	 */
-	sigchain_push(SIGPIPE, SIG_IGN);
-	write_in_full(gpg.in, buf, len);
-	close(gpg.in);
-	sigchain_pop(SIGPIPE);
-
-	ret = finish_command(&gpg);
-
-	unlink_or_warn(path);
-
-	return ret;
-}
-
-static int verify_tag(const char *name, int verbose)
-{
-	enum object_type type;
-	unsigned char sha1[20];
-	char *buf;
-	unsigned long size;
-	int ret;
-
-	if (get_sha1(name, sha1))
-		return error("tag '%s' not found.", name);
-
-	type = sha1_object_info(sha1, NULL);
-	if (type != OBJ_TAG)
-		return error("%s: cannot verify a non-tag object of type %s.",
-				name, typename(type));
-
-	buf = read_sha1_file(sha1, &type, &size);
-	if (!buf)
-		return error("%s: unable to read file.", name);
-
-	ret = run_gpg_verify(buf, size, verbose);
-
-	free(buf);
-	return ret;
-}
-
 int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 {
 	int i = 1, verbose = 0, had_error = 0;
@@ -113,7 +30,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 		usage_with_options(verify_tag_usage, verify_tag_options);
 
 	while (i < argc)
-		if (verify_tag(argv[i++], verbose))
+		if (verify_tag_signature(argv[i++], verbose))
 			had_error = 1;
 	return had_error;
 }
diff --git a/tag.h b/tag.h
index 7a0cb00..1034109 100644
--- a/tag.h
+++ b/tag.h
@@ -16,5 +16,6 @@ extern struct tag *lookup_tag(const unsigned char *sha1);
 extern int parse_tag_buffer(struct tag *item, void *data, unsigned long size);
 extern int parse_tag(struct tag *item);
 extern struct object *deref_tag(struct object *, const char *, int);
+extern int verify_tag_signature(const char *name, int verbose);
 
 #endif /* TAG_H */
diff --git a/builtin/verify-tag.c b/verify-tag.c
similarity index 71%
copy from builtin/verify-tag.c
copy to verify-tag.c
index 91dd1c1..7152e99 100644
--- a/builtin/verify-tag.c
+++ b/verify-tag.c
@@ -1,22 +1,11 @@
 /*
- * Builtin "git verify-tag"
- *
  * Copyright (c) 2007 Carlos Rica <jasampler@gmail.com>
- *
- * Based on git-verify-tag.sh
  */
 #include "cache.h"
-#include "builtin.h"
 #include "tag.h"
 #include "run-command.h"
 #include <signal.h>
 #include "sigchain.h"
-#include "parse-options.h"
-
-static const char * const verify_tag_usage[] = {
-		"git verify-tag [-v|--verbose] <tag>...",
-		NULL
-};
 
 #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
 
@@ -71,7 +60,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
 	return ret;
 }
 
-static int verify_tag(const char *name, int verbose)
+int verify_tag_signature(const char *name, int verbose)
 {
 	enum object_type type;
 	unsigned char sha1[20];
@@ -96,24 +85,3 @@ static int verify_tag(const char *name, int verbose)
 	free(buf);
 	return ret;
 }
-
-int cmd_verify_tag(int argc, const char **argv, const char *prefix)
-{
-	int i = 1, verbose = 0, had_error = 0;
-	const struct option verify_tag_options[] = {
-		OPT__VERBOSE(&verbose),
-		OPT_END()
-	};
-
-	git_config(git_default_config, NULL);
-
-	argc = parse_options(argc, argv, prefix, verify_tag_options,
-			     verify_tag_usage, PARSE_OPT_KEEP_ARGV0);
-	if (argc <= i)
-		usage_with_options(verify_tag_usage, verify_tag_options);
-
-	while (i < argc)
-		if (verify_tag(argv[i++], verbose))
-			had_error = 1;
-	return had_error;
-}
-- 
1.7.0.2

[PATCH 4/5] tag -v: Do not spawn a separate process for verify-tag

[Jonathan Nieder]

Aside from handling of SIGPIPE, there was no reason for verify-tag
to run in a separate process.

Not much advantage to this I can see.  It just simplifies things
a little.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/tag.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index d56c882..d138e0c 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -147,11 +147,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)
 {
-	const char *argv_verify_tag[] = {"git", "verify-tag",
-					"-v", "SHA1_HEX", NULL};
-	argv_verify_tag[3] = sha1_to_hex(sha1);
-
-	if (run_command_v_opt(argv_verify_tag, 0))
+	if (verify_tag_signature(sha1_to_hex(sha1), 1))
 		return error("could not verify the tag '%s'", name);
 	return 0;
 }
-- 
1.7.0.2

[PATCH 4/5 v2] tag -v: Do not spawn a separate process for verify-tag

[Jonathan Nieder]

Aside from handling of SIGPIPE, there was no reason for verify-tag
to run in a separate process.

Not much advantage to this I can see.  It just simplifies things
a little.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Rebased to remove the churn from patch 1, as suggested by Johannes.
Thanks.

The new series has only 4 patches:

  [PATCH 2/5] verify-tag: use sigchain library to block SIGPIPE
  [PATCH 3/5] Expose verify_tag()
  [PATCH 4/5 v2] tag -v: Do not spawn a separate process for verify-tag
  [PATCH 5/5] verify_tag_signature(): let caller look up tag object sha1

 builtin/tag.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 4ef1c4f..d138e0c 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -147,11 +147,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)
 {
-	const char *argv_verify_tag[] = {"git-verify-tag",
-					"-v", "SHA1_HEX", NULL};
-	argv_verify_tag[2] = sha1_to_hex(sha1);
-
-	if (run_command_v_opt(argv_verify_tag, 0))
+	if (verify_tag_signature(sha1_to_hex(sha1), 1))
 		return error("could not verify the tag '%s'", name);
 	return 0;
 }
-- 
1.7.0.2

[PATCH 5/5] verify_tag_signature(): let caller look up tag object sha1

[Jonathan Nieder]

Avoid an unnecessary get_sha1() call in cmd_tag().

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/tag.c        |    2 +-
 builtin/verify-tag.c |   16 ++++++++++++++--
 tag.h                |    3 ++-
 verify-tag.c         |    6 +-----
 4 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index d138e0c..df94a8c 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -147,7 +147,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)
 {
-	if (verify_tag_signature(sha1_to_hex(sha1), 1))
+	if (verify_tag_signature(sha1, name, 1))
 		return error("could not verify the tag '%s'", name);
 	return 0;
 }
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index ca075bd..df2e095 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -14,6 +14,13 @@ static const char * const verify_tag_usage[] = {
 		NULL
 };
 
+static int get_sha1_or_whine(const char *name, unsigned char *sha1)
+{
+	if (get_sha1(name, sha1))
+		return error("tag '%s' not found.", name);
+	return 0;
+}
+
 int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 {
 	int i = 1, verbose = 0, had_error = 0;
@@ -29,8 +36,13 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 	if (argc <= i)
 		usage_with_options(verify_tag_usage, verify_tag_options);
 
-	while (i < argc)
-		if (verify_tag_signature(argv[i++], verbose))
+	while (i < argc) {
+		unsigned char sha1[20];
+		const char *name = argv[i++];
+
+		if (get_sha1_or_whine(name, sha1) < 0 ||
+		    verify_tag_signature(sha1, name, verbose) < 0)
 			had_error = 1;
+	}
 	return had_error;
 }
diff --git a/tag.h b/tag.h
index 1034109..f83dce6 100644
--- a/tag.h
+++ b/tag.h
@@ -16,6 +16,7 @@ extern struct tag *lookup_tag(const unsigned char *sha1);
 extern int parse_tag_buffer(struct tag *item, void *data, unsigned long size);
 extern int parse_tag(struct tag *item);
 extern struct object *deref_tag(struct object *, const char *, int);
-extern int verify_tag_signature(const char *name, int verbose);
+extern int verify_tag_signature(const unsigned char *sha1, const char *name,
+				int verbose);
 
 #endif /* TAG_H */
diff --git a/verify-tag.c b/verify-tag.c
index 7152e99..bf88707 100644
--- a/verify-tag.c
+++ b/verify-tag.c
@@ -60,17 +60,13 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
 	return ret;
 }
 
-int verify_tag_signature(const char *name, int verbose)
+int verify_tag_signature(const unsigned char *sha1, const char *name, int verbose)
 {
 	enum object_type type;
-	unsigned char sha1[20];
 	char *buf;
 	unsigned long size;
 	int ret;
 
-	if (get_sha1(name, sha1))
-		return error("tag '%s' not found.", name);
-
 	type = sha1_object_info(sha1, NULL);
 	if (type != OBJ_TAG)
 		return error("%s: cannot verify a non-tag object of type %s.",
-- 
1.7.0.2