* [PATCHv2 1/6] pretty: free the gpg status buf
2014-06-13 10:42 ` [PATCHv2 0/6] verify-commit: verify commit signatures Michael J Gruber
@ 2014-06-13 10:42 ` Michael J Gruber
2014-06-13 11:39 ` Jeff King
2014-06-13 10:42 ` [PATCHv2 2/6] gpg-interface: provide access to the payload Michael J Gruber
` (5 subsequent siblings)
6 siblings, 1 reply; 75+ messages in thread
From: Michael J Gruber @ 2014-06-13 10:42 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
4a868fd (pretty: parse the gpg status lines rather than the output, 2013-02-14)
made the gpg status lines available to callers and made sure they freed
the used space, but missed one spot.
Free the status line buffer also in the remaining spot.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
pretty.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/pretty.c b/pretty.c
index 4f51287..f1e8a70 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1538,6 +1538,7 @@ void format_commit_message(const struct commit *commit,
free(context.commit_encoding);
logmsg_free(context.message, commit);
free(context.signature_check.gpg_output);
+ free(context.signature_check.gpg_status);
free(context.signature_check.signer);
}
--
2.0.0.426.g37dbf84
^ permalink raw reply related [flat|nested] 75+ messages in thread
* Re: [PATCHv2 1/6] pretty: free the gpg status buf
2014-06-13 10:42 ` [PATCHv2 1/6] pretty: free the gpg status buf Michael J Gruber
@ 2014-06-13 11:39 ` Jeff King
0 siblings, 0 replies; 75+ messages in thread
From: Jeff King @ 2014-06-13 11:39 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
On Fri, Jun 13, 2014 at 12:42:43PM +0200, Michael J Gruber wrote:
> 4a868fd (pretty: parse the gpg status lines rather than the output, 2013-02-14)
> made the gpg status lines available to callers and made sure they freed
> the used space, but missed one spot.
>
> Free the status line buffer also in the remaining spot.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
> pretty.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/pretty.c b/pretty.c
> index 4f51287..f1e8a70 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1538,6 +1538,7 @@ void format_commit_message(const struct commit *commit,
> free(context.commit_encoding);
> logmsg_free(context.message, commit);
> free(context.signature_check.gpg_output);
> + free(context.signature_check.gpg_status);
> free(context.signature_check.signer);
What about .key?
I would have expected your patch 6 to come first, which would fix this,
and then save you from making similar mistakes in patch 3. :)
-Peff
^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCHv2 2/6] gpg-interface: provide access to the payload
2014-06-13 10:42 ` [PATCHv2 0/6] verify-commit: verify commit signatures Michael J Gruber
2014-06-13 10:42 ` [PATCHv2 1/6] pretty: free the gpg status buf Michael J Gruber
@ 2014-06-13 10:42 ` Michael J Gruber
2014-06-13 10:42 ` [PATCHv2 3/6] verify-commit: scriptable commit signature verification Michael J Gruber
` (4 subsequent siblings)
6 siblings, 0 replies; 75+ messages in thread
From: Michael J Gruber @ 2014-06-13 10:42 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In contrast to tag signatures, commit signatures are put into the
header, that is between the other header parts and commit messages.
Provide access to the commit content sans the signature, which is the
payload that is actually signed. Commit signature verification does the
parsing anyways, and callers may wish to act on or display the commit
object sans the signature.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
builtin/merge.c | 1 +
commit.c | 1 +
gpg-interface.h | 1 +
pretty.c | 1 +
4 files changed, 4 insertions(+)
diff --git a/builtin/merge.c b/builtin/merge.c
index 428ca24..6a9812a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1282,6 +1282,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
printf(_("Commit %s has a good GPG signature by %s\n"),
hex, signature_check.signer);
+ free(signature_check.payload);
free(signature_check.gpg_output);
free(signature_check.gpg_status);
free(signature_check.signer);
diff --git a/commit.c b/commit.c
index 881be3b..d0ad7f0 100644
--- a/commit.c
+++ b/commit.c
@@ -1219,6 +1219,7 @@ void check_commit_signature(const struct commit* commit, struct signature_check
&gpg_output, &gpg_status);
if (status && !gpg_output.len)
goto out;
+ sigc->payload = strbuf_detach(&payload, NULL);
sigc->gpg_output = strbuf_detach(&gpg_output, NULL);
sigc->gpg_status = strbuf_detach(&gpg_status, NULL);
parse_gpg_output(sigc);
diff --git a/gpg-interface.h b/gpg-interface.h
index a85cb5b..d727c25 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -2,6 +2,7 @@
#define GPG_INTERFACE_H
struct signature_check {
+ char *payload;
char *gpg_output;
char *gpg_status;
char result; /* 0 (not checked),
diff --git a/pretty.c b/pretty.c
index f1e8a70..24fb877 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1537,6 +1537,7 @@ void format_commit_message(const struct commit *commit,
free(context.commit_encoding);
logmsg_free(context.message, commit);
+ free(context.signature_check.payload);
free(context.signature_check.gpg_output);
free(context.signature_check.gpg_status);
free(context.signature_check.signer);
--
2.0.0.426.g37dbf84
^ permalink raw reply related [flat|nested] 75+ messages in thread
* [PATCHv2 3/6] verify-commit: scriptable commit signature verification
2014-06-13 10:42 ` [PATCHv2 0/6] verify-commit: verify commit signatures Michael J Gruber
2014-06-13 10:42 ` [PATCHv2 1/6] pretty: free the gpg status buf Michael J Gruber
2014-06-13 10:42 ` [PATCHv2 2/6] gpg-interface: provide access to the payload Michael J Gruber
@ 2014-06-13 10:42 ` Michael J Gruber
2014-06-13 11:19 ` Jeff King
2014-06-13 10:42 ` [PATCHv2 4/6] t7510: exit for loop with test result Michael J Gruber
` (3 subsequent siblings)
6 siblings, 1 reply; 75+ messages in thread
From: Michael J Gruber @ 2014-06-13 10:42 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
Commit signatures can be verified using "git show -s --show-signature"
or the "%G?" pretty format and parsing the output, which is well suited
for user inspection, but not for scripting.
Provide a command "verify-commit" which is analogous to "verify-tag": It
returns 0 for good signatures and non-zero otherwise, has the gpg output
on stderr and (optionally) the commit object on stdout, sans the
signature, just like "verify-tag" does.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Documentation/git-verify-commit.txt | 28 +++++++++++
Makefile | 1 +
builtin.h | 1 +
builtin/verify-commit.c | 98 +++++++++++++++++++++++++++++++++++++
command-list.txt | 1 +
git.c | 1 +
6 files changed, 130 insertions(+)
create mode 100644 Documentation/git-verify-commit.txt
create mode 100644 builtin/verify-commit.c
diff --git a/Documentation/git-verify-commit.txt b/Documentation/git-verify-commit.txt
new file mode 100644
index 0000000..9413e28
--- /dev/null
+++ b/Documentation/git-verify-commit.txt
@@ -0,0 +1,28 @@
+git-verify-commit(1)
+====================
+
+NAME
+----
+git-verify-commit - Check the GPG signature of commits
+
+SYNOPSIS
+--------
+[verse]
+'git verify-commit' <commit>...
+
+DESCRIPTION
+-----------
+Validates the gpg signature created by 'git commit -S'.
+
+OPTIONS
+-------
+-v::
+--verbose::
+ Print the contents of the commit object before validating it.
+
+<commit>...::
+ SHA-1 identifiers of Git commit objects.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 07ea105..b92418d 100644
--- a/Makefile
+++ b/Makefile
@@ -999,6 +999,7 @@ BUILTIN_OBJS += builtin/update-ref.o
BUILTIN_OBJS += builtin/update-server-info.o
BUILTIN_OBJS += builtin/upload-archive.o
BUILTIN_OBJS += builtin/var.o
+BUILTIN_OBJS += builtin/verify-commit.o
BUILTIN_OBJS += builtin/verify-pack.o
BUILTIN_OBJS += builtin/verify-tag.o
BUILTIN_OBJS += builtin/write-tree.o
diff --git a/builtin.h b/builtin.h
index c47c110..5d91f31 100644
--- a/builtin.h
+++ b/builtin.h
@@ -128,6 +128,7 @@ extern int cmd_update_server_info(int argc, const char **argv, const char *prefi
extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);
extern int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix);
extern int cmd_var(int argc, const char **argv, const char *prefix);
+extern int cmd_verify_commit(int argc, const char **argv, const char *prefix);
extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);
extern int cmd_version(int argc, const char **argv, const char *prefix);
extern int cmd_whatchanged(int argc, const char **argv, const char *prefix);
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
new file mode 100644
index 0000000..69b7c6d
--- /dev/null
+++ b/builtin/verify-commit.c
@@ -0,0 +1,98 @@
+/*
+ * Builtin "git commit-commit"
+ *
+ * Copyright (c) 2014 Michael J Gruber <git@drmicha.warpmail.net>
+ *
+ * Based on git-verify-tag
+ */
+#include "cache.h"
+#include "builtin.h"
+#include "commit.h"
+#include "run-command.h"
+#include <signal.h>
+#include "parse-options.h"
+#include "gpg-interface.h"
+
+static const char * const verify_commit_usage[] = {
+ N_("git verify-commit [-v|--verbose] <commit>..."),
+ NULL
+};
+
+static int run_gpg_verify(const unsigned char *sha1, const char *buf, unsigned long size, int verbose)
+{
+ struct signature_check signature_check;
+
+ memset(&signature_check, 0, sizeof(signature_check));
+
+ check_commit_signature(lookup_commit(sha1), &signature_check);
+
+ if (verbose && signature_check.payload)
+ fputs(signature_check.payload, stdout);
+
+ if (signature_check.gpg_output)
+ fputs(signature_check.gpg_output, stderr);
+
+ free(signature_check.gpg_output);
+ free(signature_check.gpg_status);
+ free(signature_check.signer);
+ free(signature_check.key);
+ return signature_check.result != 'G';
+}
+
+static int verify_commit(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("commit '%s' not found.", name);
+
+ type = sha1_object_info(sha1, NULL);
+ if (type != OBJ_COMMIT)
+ return error("%s: cannot verify a non-commit 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(sha1, buf, size, verbose);
+
+ free(buf);
+ return ret;
+}
+
+static int git_verify_commit_config(const char *var, const char *value, void *cb)
+{
+ int status = git_gpg_config(var, value, cb);
+ if (status)
+ return status;
+ return git_default_config(var, value, cb);
+}
+
+int cmd_verify_commit(int argc, const char **argv, const char *prefix)
+{
+ int i = 1, verbose = 0, had_error = 0;
+ const struct option verify_commit_options[] = {
+ OPT__VERBOSE(&verbose, N_("print commit contents")),
+ OPT_END()
+ };
+
+ git_config(git_verify_commit_config, NULL);
+
+ argc = parse_options(argc, argv, prefix, verify_commit_options,
+ verify_commit_usage, PARSE_OPT_KEEP_ARGV0);
+ if (argc <= i)
+ usage_with_options(verify_commit_usage, verify_commit_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_commit(argv[i++], verbose))
+ had_error = 1;
+ return had_error;
+}
diff --git a/command-list.txt b/command-list.txt
index cf36c3d..a3ff0c9 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -132,6 +132,7 @@ git-update-server-info synchingrepositories
git-upload-archive synchelpers
git-upload-pack synchelpers
git-var plumbinginterrogators
+git-verify-commit ancillaryinterrogators
git-verify-pack plumbinginterrogators
git-verify-tag ancillaryinterrogators
gitweb ancillaryinterrogators
diff --git a/git.c b/git.c
index 7780572..bbc3293 100644
--- a/git.c
+++ b/git.c
@@ -441,6 +441,7 @@ static struct cmd_struct commands[] = {
{ "upload-archive", cmd_upload_archive },
{ "upload-archive--writer", cmd_upload_archive_writer },
{ "var", cmd_var, RUN_SETUP_GENTLY },
+ { "verify-commit", cmd_verify_commit, RUN_SETUP },
{ "verify-pack", cmd_verify_pack },
{ "verify-tag", cmd_verify_tag, RUN_SETUP },
{ "version", cmd_version },
--
2.0.0.426.g37dbf84
^ permalink raw reply related [flat|nested] 75+ messages in thread
* Re: [PATCHv2 3/6] verify-commit: scriptable commit signature verification
2014-06-13 10:42 ` [PATCHv2 3/6] verify-commit: scriptable commit signature verification Michael J Gruber
@ 2014-06-13 11:19 ` Jeff King
2014-06-13 11:45 ` Michael J Gruber
0 siblings, 1 reply; 75+ messages in thread
From: Jeff King @ 2014-06-13 11:19 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
On Fri, Jun 13, 2014 at 12:42:45PM +0200, Michael J Gruber wrote:
> +
> + free(signature_check.gpg_output);
> + free(signature_check.gpg_status);
> + free(signature_check.signer);
> + free(signature_check.key);
> + return signature_check.result != 'G';
> +}
How about .payload here?
> + type = sha1_object_info(sha1, NULL);
> + if (type != OBJ_COMMIT)
> + return error("%s: cannot verify a non-commit object of type %s.",
> + name, typename(type));
> +
> + buf = read_sha1_file(sha1, &type, &size);
> + if (!buf)
> + return error("%s: unable to read file.", name);
I think you can drop the sha1_object_info call and just check "type"
from read_sha1_file (they _should_ agree, but if they do not, I'd rather
pay attention to the one that came along with the buffer). And this is
the uncommon error path, so expanding the object into memory before we
die is not a big deal.
Should this peel to a commit if given a tag? I'd say probably. I know
you raised the issue elsewhere of keeping things simple, but I think if
you are calling verify-commit, you know you want a commit, and we should
treat the argument as a commit-ish. Anyway, if you go that route, then
lookup_commit_or_die is probably what you want.
Also, minor nit, but we typically do not end the error messages with a
full stop (we've been rather inconsistent in the past, but these days
seem to mostly settle on no punctuation).
-Peff
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv2 3/6] verify-commit: scriptable commit signature verification
2014-06-13 11:19 ` Jeff King
@ 2014-06-13 11:45 ` Michael J Gruber
2014-06-13 11:50 ` Jeff King
0 siblings, 1 reply; 75+ messages in thread
From: Michael J Gruber @ 2014-06-13 11:45 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
Jeff King venit, vidit, dixit 13.06.2014 13:19:
> On Fri, Jun 13, 2014 at 12:42:45PM +0200, Michael J Gruber wrote:
>
>> +
>> + free(signature_check.gpg_output);
>> + free(signature_check.gpg_status);
>> + free(signature_check.signer);
>> + free(signature_check.key);
>> + return signature_check.result != 'G';
>> +}
>
> How about .payload here?
I sneekily fix this in 6/6... I thought 3/6 is on next already, too late
for a real v2. Otherwise I would put 6/6 before everything else.
>> + type = sha1_object_info(sha1, NULL);
>> + if (type != OBJ_COMMIT)
>> + return error("%s: cannot verify a non-commit object of type %s.",
>> + name, typename(type));
>> +
>> + buf = read_sha1_file(sha1, &type, &size);
>> + if (!buf)
>> + return error("%s: unable to read file.", name);
>
> I think you can drop the sha1_object_info call and just check "type"
> from read_sha1_file (they _should_ agree, but if they do not, I'd rather
> pay attention to the one that came along with the buffer). And this is
> the uncommon error path, so expanding the object into memory before we
> die is not a big deal.
>
> Should this peel to a commit if given a tag? I'd say probably. I know
> you raised the issue elsewhere of keeping things simple, but I think if
> you are calling verify-commit, you know you want a commit, and we should
> treat the argument as a commit-ish. Anyway, if you go that route, then
> lookup_commit_or_die is probably what you want.
>
> Also, minor nit, but we typically do not end the error messages with a
> full stop (we've been rather inconsistent in the past, but these days
> seem to mostly settle on no punctuation).
>
> -Peff
Both of these issues actually come for following verify-tag as closely
as possible. If 3 is not applied already, I should do away with
sha1_object_info.
About the peeling I'm not so sure, since there's a difference between a
signed tag pointing to a commit and a signed commit. Since
verify-{tag,commit} are bare metal plumbing, I would expect callers to
use <rev>^{commit} explicitly if they don't care how <rev> peels to a
commit.
Michael
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv2 3/6] verify-commit: scriptable commit signature verification
2014-06-13 11:45 ` Michael J Gruber
@ 2014-06-13 11:50 ` Jeff King
2014-06-13 12:12 ` Michael J Gruber
0 siblings, 1 reply; 75+ messages in thread
From: Jeff King @ 2014-06-13 11:50 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
On Fri, Jun 13, 2014 at 01:45:58PM +0200, Michael J Gruber wrote:
> I sneekily fix this in 6/6... I thought 3/6 is on next already, too late
> for a real v2. Otherwise I would put 6/6 before everything else.
Ah, yeah, I assumed we were still re-rolling (and it looks like you're
just on pu so far).
> About the peeling I'm not so sure, since there's a difference between a
> signed tag pointing to a commit and a signed commit.
There is, but "verify-commit" is always going to verify the commit, no?
Not peeling will always result in an error, and never do anything
useful.
I admit it's probably not going to come up too often, though. And I
don't have any argument beyond "it makes sense to me", so I won't push
for it further.
-Peff
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv2 3/6] verify-commit: scriptable commit signature verification
2014-06-13 11:50 ` Jeff King
@ 2014-06-13 12:12 ` Michael J Gruber
0 siblings, 0 replies; 75+ messages in thread
From: Michael J Gruber @ 2014-06-13 12:12 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
Jeff King venit, vidit, dixit 13.06.2014 13:50:
> On Fri, Jun 13, 2014 at 01:45:58PM +0200, Michael J Gruber wrote:
>
>> I sneekily fix this in 6/6... I thought 3/6 is on next already, too late
>> for a real v2. Otherwise I would put 6/6 before everything else.
>
> Ah, yeah, I assumed we were still re-rolling (and it looks like you're
> just on pu so far).
We are, I had misread a "What's cooking". So a reroll it is.
>> About the peeling I'm not so sure, since there's a difference between a
>> signed tag pointing to a commit and a signed commit.
>
> There is, but "verify-commit" is always going to verify the commit, no?
> Not peeling will always result in an error, and never do anything
> useful.
>
> I admit it's probably not going to come up too often, though. And I
> don't have any argument beyond "it makes sense to me", so I won't push
> for it further.
>
> -Peff
I guess it boils down to the fact how plumberish it's supposed to be.
Since it's about verification, for some definition of "verify", I'd
rather apply as few automatisms as possible. If the caller wants to know
whether commit deadbeef carries a valid commit signature I'd rather
check that very object.
I also picture doing the "git-verify" thing in the future (with
"--commit" and "--tag" options which make the command insist on the
object type), and then we would not want to peel tags under the hood.
Michael
^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCHv2 4/6] t7510: exit for loop with test result
2014-06-13 10:42 ` [PATCHv2 0/6] verify-commit: verify commit signatures Michael J Gruber
` (2 preceding siblings ...)
2014-06-13 10:42 ` [PATCHv2 3/6] verify-commit: scriptable commit signature verification Michael J Gruber
@ 2014-06-13 10:42 ` Michael J Gruber
2014-06-13 11:46 ` Jeff King
2014-06-13 10:42 ` [PATCHv2 5/6] t7510: test verify-commit Michael J Gruber
` (2 subsequent siblings)
6 siblings, 1 reply; 75+ messages in thread
From: Michael J Gruber @ 2014-06-13 10:42 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
When t7510 was introduced, the author made sure that a for loop in
a subshell would return with the appropriate error code.
Make sure this is true also the for the first line in each loop, which
was missed.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
t/t7510-signed-commit.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 5ddac1a..a5ba48e 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' '
(
for commit in initial second merge fourth-signed fifth-signed sixth-signed master
do
- git show --pretty=short --show-signature $commit >actual &&
+ git show --pretty=short --show-signature $commit >actual || exit 1
grep "Good signature from" actual || exit 1
! grep "BAD signature from" actual || exit 1
echo $commit OK
@@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' '
(
for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
do
- git show --pretty=short --show-signature $commit >actual &&
+ git show --pretty=short --show-signature $commit >actual || exit 1
grep "Good signature from" actual && exit 1
! grep "BAD signature from" actual || exit 1
echo $commit OK
--
2.0.0.426.g37dbf84
^ permalink raw reply related [flat|nested] 75+ messages in thread
* Re: [PATCHv2 4/6] t7510: exit for loop with test result
2014-06-13 10:42 ` [PATCHv2 4/6] t7510: exit for loop with test result Michael J Gruber
@ 2014-06-13 11:46 ` Jeff King
2014-06-13 12:04 ` Michael J Gruber
2014-06-13 18:23 ` Junio C Hamano
0 siblings, 2 replies; 75+ messages in thread
From: Jeff King @ 2014-06-13 11:46 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote:
> When t7510 was introduced, the author made sure that a for loop in
> a subshell would return with the appropriate error code.
>
> Make sure this is true also the for the first line in each loop, which
> was missed.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
> t/t7510-signed-commit.sh | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> index 5ddac1a..a5ba48e 100755
> --- a/t/t7510-signed-commit.sh
> +++ b/t/t7510-signed-commit.sh
> @@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' '
> (
> for commit in initial second merge fourth-signed fifth-signed sixth-signed master
> do
> - git show --pretty=short --show-signature $commit >actual &&
> + git show --pretty=short --show-signature $commit >actual || exit 1
> grep "Good signature from" actual || exit 1
Hrm. The original is:
X &&
Y || exit 1
Won't that still exit (i.e., it is already correct)? Doing:
for X in true false; do
for Y in true false; do
($X && $Y || exit 1)
echo "$X/$Y: $?"
done
done
yields:
true/true: 0
true/false: 1
false/true: 1
false/false: 1
(and should still short-circuit Y, because we go from left-to-right).
I do not mind changing it to keep the style of each line consistent,
though. I would have written it as a series of "&&"-chains, with a
single exit at the end, but I think that is just a matter of preference.
-Peff
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv2 4/6] t7510: exit for loop with test result
2014-06-13 11:46 ` Jeff King
@ 2014-06-13 12:04 ` Michael J Gruber
2014-06-13 12:22 ` Michael J Gruber
2014-06-13 18:23 ` Junio C Hamano
1 sibling, 1 reply; 75+ messages in thread
From: Michael J Gruber @ 2014-06-13 12:04 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
Jeff King venit, vidit, dixit 13.06.2014 13:46:
> On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote:
>
>> When t7510 was introduced, the author made sure that a for loop in
>> a subshell would return with the appropriate error code.
>>
>> Make sure this is true also the for the first line in each loop, which
>> was missed.
>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> ---
>> t/t7510-signed-commit.sh | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
>> index 5ddac1a..a5ba48e 100755
>> --- a/t/t7510-signed-commit.sh
>> +++ b/t/t7510-signed-commit.sh
>> @@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' '
>> (
>> for commit in initial second merge fourth-signed fifth-signed sixth-signed master
>> do
>> - git show --pretty=short --show-signature $commit >actual &&
>> + git show --pretty=short --show-signature $commit >actual || exit 1
>> grep "Good signature from" actual || exit 1
>
> Hrm. The original is:
>
> X &&
> Y || exit 1
>
> Won't that still exit (i.e., it is already correct)? Doing:
>
> for X in true false; do
> for Y in true false; do
> ($X && $Y || exit 1)
> echo "$X/$Y: $?"
> done
> done
>
> yields:
>
> true/true: 0
> true/false: 1
> false/true: 1
> false/false: 1
>
> (and should still short-circuit Y, because we go from left-to-right).
>
> I do not mind changing it to keep the style of each line consistent,
> though. I would have written it as a series of "&&"-chains, with a
> single exit at the end, but I think that is just a matter of preference.
If I remember correctly, I put something failing on the first line of
the original version, and the test succeeded. I think the point is that
we have a for loop in a subshell, and we need to make sure that the
false of one iteration is not overwritten by the true of the next one -
"exit 1" makes sure to "break" the for loop and exit the subshell.
(The chain should do that as well, I'll recheck.)
Michael
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv2 4/6] t7510: exit for loop with test result
2014-06-13 12:04 ` Michael J Gruber
@ 2014-06-13 12:22 ` Michael J Gruber
2014-06-13 12:33 ` Michael J Gruber
0 siblings, 1 reply; 75+ messages in thread
From: Michael J Gruber @ 2014-06-13 12:22 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
Michael J Gruber venit, vidit, dixit 13.06.2014 14:04:
> Jeff King venit, vidit, dixit 13.06.2014 13:46:
>> On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote:
>>
>>> When t7510 was introduced, the author made sure that a for loop in
>>> a subshell would return with the appropriate error code.
>>>
>>> Make sure this is true also the for the first line in each loop, which
>>> was missed.
>>>
>>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>>> ---
>>> t/t7510-signed-commit.sh | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
>>> index 5ddac1a..a5ba48e 100755
>>> --- a/t/t7510-signed-commit.sh
>>> +++ b/t/t7510-signed-commit.sh
>>> @@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' '
>>> (
>>> for commit in initial second merge fourth-signed fifth-signed sixth-signed master
>>> do
>>> - git show --pretty=short --show-signature $commit >actual &&
>>> + git show --pretty=short --show-signature $commit >actual || exit 1
>>> grep "Good signature from" actual || exit 1
>>
>> Hrm. The original is:
>>
>> X &&
>> Y || exit 1
>>
>> Won't that still exit (i.e., it is already correct)? Doing:
>>
>> for X in true false; do
>> for Y in true false; do
>> ($X && $Y || exit 1)
>> echo "$X/$Y: $?"
>> done
>> done
>>
>> yields:
>>
>> true/true: 0
>> true/false: 1
>> false/true: 1
>> false/false: 1
>>
>> (and should still short-circuit Y, because we go from left-to-right).
>>
>> I do not mind changing it to keep the style of each line consistent,
>> though. I would have written it as a series of "&&"-chains, with a
>> single exit at the end, but I think that is just a matter of preference.
>
> If I remember correctly, I put something failing on the first line of
> the original version, and the test succeeded. I think the point is that
> we have a for loop in a subshell, and we need to make sure that the
> false of one iteration is not overwritten by the true of the next one -
> "exit 1" makes sure to "break" the for loop and exit the subshell.
> (The chain should do that as well, I'll recheck.)
... the chain does not, which is the point :)
With X && Y || exit 1 inside the loop, the loop statement will return
false, but the loop will continue (if X returns false), which is exactly
the problem that the exit avoids.
Make your example iterate over false true instead in the inner loop and
you'll see ;)
Michael
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv2 4/6] t7510: exit for loop with test result
2014-06-13 12:22 ` Michael J Gruber
@ 2014-06-13 12:33 ` Michael J Gruber
2014-06-13 12:45 ` Jeff King
2014-06-13 12:54 ` Johannes Sixt
0 siblings, 2 replies; 75+ messages in thread
From: Michael J Gruber @ 2014-06-13 12:33 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
Michael J Gruber venit, vidit, dixit 13.06.2014 14:22:
> Michael J Gruber venit, vidit, dixit 13.06.2014 14:04:
>> Jeff King venit, vidit, dixit 13.06.2014 13:46:
>>> On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote:
>>>
>>>> When t7510 was introduced, the author made sure that a for loop in
>>>> a subshell would return with the appropriate error code.
>>>>
>>>> Make sure this is true also the for the first line in each loop, which
>>>> was missed.
>>>>
>>>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>>>> ---
>>>> t/t7510-signed-commit.sh | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
>>>> index 5ddac1a..a5ba48e 100755
>>>> --- a/t/t7510-signed-commit.sh
>>>> +++ b/t/t7510-signed-commit.sh
>>>> @@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' '
>>>> (
>>>> for commit in initial second merge fourth-signed fifth-signed sixth-signed master
>>>> do
>>>> - git show --pretty=short --show-signature $commit >actual &&
>>>> + git show --pretty=short --show-signature $commit >actual || exit 1
>>>> grep "Good signature from" actual || exit 1
>>>
>>> Hrm. The original is:
>>>
>>> X &&
>>> Y || exit 1
>>>
>>> Won't that still exit (i.e., it is already correct)? Doing:
>>>
>>> for X in true false; do
>>> for Y in true false; do
>>> ($X && $Y || exit 1)
>>> echo "$X/$Y: $?"
>>> done
>>> done
>>>
>>> yields:
>>>
>>> true/true: 0
>>> true/false: 1
>>> false/true: 1
>>> false/false: 1
>>>
>>> (and should still short-circuit Y, because we go from left-to-right).
>>>
>>> I do not mind changing it to keep the style of each line consistent,
>>> though. I would have written it as a series of "&&"-chains, with a
>>> single exit at the end, but I think that is just a matter of preference.
>>
>> If I remember correctly, I put something failing on the first line of
>> the original version, and the test succeeded. I think the point is that
>> we have a for loop in a subshell, and we need to make sure that the
>> false of one iteration is not overwritten by the true of the next one -
>> "exit 1" makes sure to "break" the for loop and exit the subshell.
>> (The chain should do that as well, I'll recheck.)
>
> ... the chain does not, which is the point :)
>
> With X && Y || exit 1 inside the loop, the loop statement will return
> false, but the loop will continue (if X returns false), which is exactly
> the problem that the exit avoids.
>
> Make your example iterate over false true instead in the inner loop and
> you'll see ;)
>
> Michael
... with this loop, sorry:
for X in true false; do
for Y in false true; do
($X && $Y || exit 1)
done
echo "$X/last inner $Y: $?"
done
gives
true/last inner true: 0
false/last inner true: 1
even though on both cases we have at least one failure of Y. (failure of
one subtest = failure of the test)
Looping in the other order:
for X in true false; do
for Y in true false; do
($X && $Y || exit 1)
done
echo "$X/last inner $Y: $?"
done
gives
true/last inner false: 1
false/last inner false: 1
as it should be.
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv2 4/6] t7510: exit for loop with test result
2014-06-13 12:33 ` Michael J Gruber
@ 2014-06-13 12:45 ` Jeff King
2014-06-13 12:54 ` Johannes Sixt
1 sibling, 0 replies; 75+ messages in thread
From: Jeff King @ 2014-06-13 12:45 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
On Fri, Jun 13, 2014 at 02:33:02PM +0200, Michael J Gruber wrote:
> > With X && Y || exit 1 inside the loop, the loop statement will return
> > false, but the loop will continue (if X returns false), which is exactly
> > the problem that the exit avoids.
> >
> > Make your example iterate over false true instead in the inner loop and
> > you'll see ;)
> >
> > Michael
>
> ... with this loop, sorry:
>
> for X in true false; do
> for Y in false true; do
> ($X && $Y || exit 1)
> done
> echo "$X/last inner $Y: $?"
> done
I'm somewhat confused, as my loops were meant only to expand the truth
table, not to simulate a real loop in the code. That is why I have a
subshell in the loop around my exit, to make sure we keep looping. In
the real code, the subshell surrounds the whole loop (so that an "exit"
leaves the entire loop without leaving the whole script).
The actual code is more like:
(
for i in a b c; do
echo $i: got to first step &&
test $i != b &&
echo $i: got to second step || exit 1
done
)
echo overall status: $?
which should fail on the second loop iteration. And it does:
a: got to first step
a: got to second step
b: got to first step
overall status: 1
That is, we short-circuit to the "exit 1" as soon as "test $i != b"
fails. You can replace the use of "$?" above with more "&&"-chaining, of
course.
-Peff
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv2 4/6] t7510: exit for loop with test result
2014-06-13 12:33 ` Michael J Gruber
2014-06-13 12:45 ` Jeff King
@ 2014-06-13 12:54 ` Johannes Sixt
2014-06-13 13:06 ` Michael J Gruber
1 sibling, 1 reply; 75+ messages in thread
From: Johannes Sixt @ 2014-06-13 12:54 UTC (permalink / raw)
To: Michael J Gruber, Jeff King; +Cc: git, Junio C Hamano
Am 6/13/2014 14:33, schrieb Michael J Gruber:
> .... with this loop, sorry:
>
> for X in true false; do
> for Y in false true; do
> ($X && $Y || exit 1)
> done
> echo "$X/last inner $Y: $?"
> done
>
> gives
>
> true/last inner true: 0
> false/last inner true: 1
>
> even though on both cases we have at least one failure of Y. (failure of
> one subtest = failure of the test)
Place the loop(s) inside the subshell, and you observe termination on the
first failure, and a failure exit code of the subshell.
The change proposed in this patch should not be necessary. Something else
must be wrong with your tests.
Ah, here it is:
@@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' '
(
for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
do
- git show --pretty=short --show-signature $commit >actual &&
+ git show --pretty=short --show-signature $commit >actual || exit 1
grep "Good signature from" actual && exit 1
! grep "BAD signature from" actual || exit 1
echo $commit OK
Notice the '&& exit 1'! It should be '|| exit 1', right?
-- Hannes
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv2 4/6] t7510: exit for loop with test result
2014-06-13 12:54 ` Johannes Sixt
@ 2014-06-13 13:06 ` Michael J Gruber
2014-06-13 13:21 ` Johannes Sixt
0 siblings, 1 reply; 75+ messages in thread
From: Michael J Gruber @ 2014-06-13 13:06 UTC (permalink / raw)
To: Johannes Sixt, Jeff King; +Cc: git, Junio C Hamano
Johannes Sixt venit, vidit, dixit 13.06.2014 14:54:
> Am 6/13/2014 14:33, schrieb Michael J Gruber:
>> .... with this loop, sorry:
>>
>> for X in true false; do
>> for Y in false true; do
>> ($X && $Y || exit 1)
>> done
>> echo "$X/last inner $Y: $?"
>> done
>>
>> gives
>>
>> true/last inner true: 0
>> false/last inner true: 1
>>
>> even though on both cases we have at least one failure of Y. (failure of
>> one subtest = failure of the test)
>
> Place the loop(s) inside the subshell, and you observe termination on the
> first failure, and a failure exit code of the subshell.
>
> The change proposed in this patch should not be necessary. Something else
> must be wrong with your tests.
I know I started this (or Jeff did, who knows ;) ), but we keep
confusing each other more and more:
> Ah, here it is:
>
> @@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' '
> (
> for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
> do
> - git show --pretty=short --show-signature $commit >actual &&
> + git show --pretty=short --show-signature $commit >actual || exit 1
> grep "Good signature from" actual && exit 1
This is as in the original, it tests invalid signatures, so "Good
signature" should not be in the response.
> ! grep "BAD signature from" actual || exit 1
> echo $commit OK
>
> Notice the '&& exit 1'! It should be '|| exit 1', right?
>
> -- Hannes
In other words, the original tests already had
grep foo && exit 1
! grep bar || exit 1
to test that we have neither foo nor bar. The reason is (supposedly) to
have this portion of the test mostly analogous to the previous one,
where we want foo and do want bar.
So this is completely unrelated.
Otoh, it seems the original test could have had
a &&
b &&
c || exit 1
or
a || exit 1
b || exit 1
c || exit 1
rather than
a &&
b || exit 1
c || exit 1
which I thought was incorrect (but I can't recreate the proof right
now). I'd say both of the former versions are preferable to the last one
unless there is a difference that neither Jeff nor I see.
I need a break before looking at this again ;)
Michael
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv2 4/6] t7510: exit for loop with test result
2014-06-13 13:06 ` Michael J Gruber
@ 2014-06-13 13:21 ` Johannes Sixt
2014-06-13 13:30 ` Jeff King
2014-06-13 13:31 ` Michael J Gruber
0 siblings, 2 replies; 75+ messages in thread
From: Johannes Sixt @ 2014-06-13 13:21 UTC (permalink / raw)
To: Michael J Gruber, Jeff King; +Cc: git, Junio C Hamano
Am 6/13/2014 15:06, schrieb Michael J Gruber:
> Johannes Sixt venit, vidit, dixit 13.06.2014 14:54:
>> Am 6/13/2014 14:33, schrieb Michael J Gruber:
>>> .... with this loop, sorry:
>>>
>>> for X in true false; do
>>> for Y in false true; do
>>> ($X && $Y || exit 1)
>>> done
>>> echo "$X/last inner $Y: $?"
>>> done
>>>
>>> gives
>>>
>>> true/last inner true: 0
>>> false/last inner true: 1
>>>
>>> even though on both cases we have at least one failure of Y. (failure of
>>> one subtest = failure of the test)
>>
>> Place the loop(s) inside the subshell, and you observe termination on the
>> first failure, and a failure exit code of the subshell.
>>
>> The change proposed in this patch should not be necessary. Something else
>> must be wrong with your tests.
>
> I know I started this (or Jeff did, who knows ;) ), but we keep
> confusing each other more and more:
>
>> Ah, here it is:
>>
>> @@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' '
>> (
>> for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
>> do
>> - git show --pretty=short --show-signature $commit >actual &&
>> + git show --pretty=short --show-signature $commit >actual || exit 1
>> grep "Good signature from" actual && exit 1
>
> This is as in the original, it tests invalid signatures, so "Good
> signature" should not be in the response.
>
>> ! grep "BAD signature from" actual || exit 1
>> echo $commit OK
>>
>> Notice the '&& exit 1'! It should be '|| exit 1', right?
>>
>> -- Hannes
>
> In other words, the original tests already had
>
> grep foo && exit 1
> ! grep bar || exit 1
>
> to test that we have neither foo nor bar. The reason is (supposedly) to
> have this portion of the test mostly analogous to the previous one,
> where we want foo and do want bar.
>
> So this is completely unrelated.
I don't think so. What is the outcome of
false && # simulate a regression
grep foo && exit 1
! grep bar || exit 1
assuming that the '! grep bar' happens to be true? Answer: The regression
is not diagnosed because the &&-chain is broken.
*That* is what I think you described earlier in this thread as "I put
something failing on the first line of the original version, and the test
succeeded."
-- Hannes
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv2 4/6] t7510: exit for loop with test result
2014-06-13 13:21 ` Johannes Sixt
@ 2014-06-13 13:30 ` Jeff King
2014-06-13 13:31 ` Michael J Gruber
1 sibling, 0 replies; 75+ messages in thread
From: Jeff King @ 2014-06-13 13:30 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Michael J Gruber, git, Junio C Hamano
On Fri, Jun 13, 2014 at 03:21:55PM +0200, Johannes Sixt wrote:
> I don't think so. What is the outcome of
>
> false && # simulate a regression
> grep foo && exit 1
> ! grep bar || exit 1
>
> assuming that the '! grep bar' happens to be true? Answer: The regression
> is not diagnosed because the &&-chain is broken.
>
> *That* is what I think you described earlier in this thread as "I put
> something failing on the first line of the original version, and the test
> succeeded."
Yeah, I think that is the bit that I was missing from my original
confusion.
false &&
anything || exit 1
_does_ work. But that is not what is written. ;)
Thanks for pointing it out.
-Peff
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv2 4/6] t7510: exit for loop with test result
2014-06-13 13:21 ` Johannes Sixt
2014-06-13 13:30 ` Jeff King
@ 2014-06-13 13:31 ` Michael J Gruber
2014-06-13 13:42 ` Johannes Sixt
1 sibling, 1 reply; 75+ messages in thread
From: Michael J Gruber @ 2014-06-13 13:31 UTC (permalink / raw)
To: Johannes Sixt, Jeff King; +Cc: git, Junio C Hamano
Johannes Sixt venit, vidit, dixit 13.06.2014 15:21:
> Am 6/13/2014 15:06, schrieb Michael J Gruber:
>> Johannes Sixt venit, vidit, dixit 13.06.2014 14:54:
>>> Am 6/13/2014 14:33, schrieb Michael J Gruber:
>>>> .... with this loop, sorry:
>>>>
>>>> for X in true false; do
>>>> for Y in false true; do
>>>> ($X && $Y || exit 1)
>>>> done
>>>> echo "$X/last inner $Y: $?"
>>>> done
>>>>
>>>> gives
>>>>
>>>> true/last inner true: 0
>>>> false/last inner true: 1
>>>>
>>>> even though on both cases we have at least one failure of Y. (failure of
>>>> one subtest = failure of the test)
>>>
>>> Place the loop(s) inside the subshell, and you observe termination on the
>>> first failure, and a failure exit code of the subshell.
>>>
>>> The change proposed in this patch should not be necessary. Something else
>>> must be wrong with your tests.
>>
>> I know I started this (or Jeff did, who knows ;) ), but we keep
>> confusing each other more and more:
>>
>>> Ah, here it is:
>>>
>>> @@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' '
>>> (
>>> for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
>>> do
>>> - git show --pretty=short --show-signature $commit >actual &&
>>> + git show --pretty=short --show-signature $commit >actual || exit 1
>>> grep "Good signature from" actual && exit 1
>>
>> This is as in the original, it tests invalid signatures, so "Good
>> signature" should not be in the response.
>>
>>> ! grep "BAD signature from" actual || exit 1
>>> echo $commit OK
>>>
>>> Notice the '&& exit 1'! It should be '|| exit 1', right?
>>>
>>> -- Hannes
>>
>> In other words, the original tests already had
>>
>> grep foo && exit 1
>> ! grep bar || exit 1
>>
>> to test that we have neither foo nor bar. The reason is (supposedly) to
>> have this portion of the test mostly analogous to the previous one,
>> where we want foo and do want bar.
>>
>> So this is completely unrelated.
>
> I don't think so. What is the outcome of
>
> false && # simulate a regression
> grep foo && exit 1
> ! grep bar || exit 1
>
> assuming that the '! grep bar' happens to be true? Answer: The regression
> is not diagnosed because the &&-chain is broken.
>
> *That* is what I think you described earlier in this thread as "I put
> something failing on the first line of the original version, and the test
> succeeded."
>
> -- Hannes
If you say that something I have said makes sense I'm happy, because I
can't confirm that myself right now. I'll take a break and look into a
rewrite of the form
a &&
b &&
test_must_fail c &&
d || exit 1
hoping that will make things both readable (by avoiding !) and concise
(by avoiding repeated exits).
Michael
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv2 4/6] t7510: exit for loop with test result
2014-06-13 13:31 ` Michael J Gruber
@ 2014-06-13 13:42 ` Johannes Sixt
0 siblings, 0 replies; 75+ messages in thread
From: Johannes Sixt @ 2014-06-13 13:42 UTC (permalink / raw)
To: Michael J Gruber, Jeff King; +Cc: git, Junio C Hamano
Am 6/13/2014 15:31, schrieb Michael J Gruber:
> rewrite of the form
>
> a &&
> b &&
> test_must_fail c &&
> d || exit 1
>
> hoping that will make things both readable (by avoiding !) and concise
> (by avoiding repeated exits).
Thanks!
Please note that we use 'test_must_fail' only for git invocations, but we
do write '!' in front of system commands that we expect to fail, e.g.,
'! grep'.
-- Hannes
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv2 4/6] t7510: exit for loop with test result
2014-06-13 11:46 ` Jeff King
2014-06-13 12:04 ` Michael J Gruber
@ 2014-06-13 18:23 ` Junio C Hamano
1 sibling, 0 replies; 75+ messages in thread
From: Junio C Hamano @ 2014-06-13 18:23 UTC (permalink / raw)
To: Jeff King; +Cc: Michael J Gruber, git
Jeff King <peff@peff.net> writes:
>> (
>> for commit in initial second merge fourth-signed fifth-signed sixth-signed master
>> do
>> - git show --pretty=short --show-signature $commit >actual &&
>> + git show --pretty=short --show-signature $commit >actual || exit 1
>> grep "Good signature from" actual || exit 1
>
> Hrm. The original is:
>
> X &&
> Y || exit 1
>
> Won't that still exit (i.e., it is already correct)? Doing:
>
> for X in true false; do
> for Y in true false; do
> ($X && $Y || exit 1)
> echo "$X/$Y: $?"
> done
> done
>
> yields:
>
> true/true: 0
> true/false: 1
> false/true: 1
> false/false: 1
>
> (and should still short-circuit Y, because we go from left-to-right).
>
> I do not mind changing it to keep the style of each line consistent,
> though. I would have written it as a series of "&&"-chains, with a
> single exit at the end, but I think that is just a matter of preference.
Yeah, series of && chain with a single exit at the end is good, and
the subshell is there only to allow us to do that "exit at the end".
^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCHv2 5/6] t7510: test verify-commit
2014-06-13 10:42 ` [PATCHv2 0/6] verify-commit: verify commit signatures Michael J Gruber
` (3 preceding siblings ...)
2014-06-13 10:42 ` [PATCHv2 4/6] t7510: exit for loop with test result Michael J Gruber
@ 2014-06-13 10:42 ` Michael J Gruber
2014-06-13 11:51 ` Jeff King
2014-06-13 10:42 ` [PATCHv2 6/6] gpg-interface: provide clear helper for struct signature_check Michael J Gruber
2014-06-23 7:05 ` [PATCHv3 0/5] verify-commit: verify commit signatures Michael J Gruber
6 siblings, 1 reply; 75+ messages in thread
From: Michael J Gruber @ 2014-06-13 10:42 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
This mixes the "git verify-commit" tests in with the "git show
--show-signature" tests, to keep the tests more readable.
The tests already mix in the "call show" tests with the "verify" tests.
So in case of a test beakage, a '-v' run would be needed to reveal the
exact point of breakage anyway.
Additionally, test the actual output of "git verify-commit" and "git
show --show-signature" and compare to "git cat-file".
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
t/t7510-signed-commit.sh | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index a5ba48e..89941a7 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -45,10 +45,11 @@ test_expect_success GPG 'create signed commits' '
git tag seventh-signed
'
-test_expect_success GPG 'show signatures' '
+test_expect_success GPG 'verify and show signatures' '
(
for commit in initial second merge fourth-signed fifth-signed sixth-signed master
do
+ git verify-commit $commit || exit 1
git show --pretty=short --show-signature $commit >actual || exit 1
grep "Good signature from" actual || exit 1
! grep "BAD signature from" actual || exit 1
@@ -58,6 +59,7 @@ test_expect_success GPG 'show signatures' '
(
for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
do
+ ! git verify-commit $commit || exit 1
git show --pretty=short --show-signature $commit >actual || exit 1
grep "Good signature from" actual && exit 1
! grep "BAD signature from" actual || exit 1
@@ -66,11 +68,25 @@ test_expect_success GPG 'show signatures' '
)
'
+test_expect_success GPG 'show signed commit with signature' '
+ git show -s initial >commit &&
+ git show -s --show-signature initial >show &&
+ git verify-commit -v initial >verify.1 2>verify.2 &&
+ git cat-file commit initial >cat &&
+ grep -v "gpg: " show >show.commit &&
+ grep "gpg: " show >show.gpg &&
+ grep -v "^ " cat | grep -v "^gpgsig " >cat.commit &&
+ test_cmp show.commit commit &&
+ test_cmp show.gpg verify.2 &&
+ test_cmp cat.commit verify.1
+'
+
test_expect_success GPG 'detect fudged signature' '
git cat-file commit master >raw &&
sed -e "s/seventh/7th forged/" raw >forged1 &&
git hash-object -w -t commit forged1 >forged1.commit &&
+ ! git verify-commit $(cat forged1.commit) &&
git show --pretty=short --show-signature $(cat forged1.commit) >actual1 &&
grep "BAD signature from" actual1 &&
! grep "Good signature from" actual1
@@ -81,6 +97,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
cat raw >forged2 &&
echo Qwik | tr "Q" "\000" >>forged2 &&
git hash-object -w -t commit forged2 >forged2.commit &&
+ ! git verify-commit $(cat forged2.commit) &&
git show --pretty=short --show-signature $(cat forged2.commit) >actual2 &&
grep "BAD signature from" actual2 &&
! grep "Good signature from" actual2
@@ -89,6 +106,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
test_expect_success GPG 'amending already signed commit' '
git checkout fourth-signed^0 &&
git commit --amend -S --no-edit &&
+ git verify-commit HEAD &&
git show -s --show-signature HEAD >actual &&
grep "Good signature from" actual &&
! grep "BAD signature from" actual
--
2.0.0.426.g37dbf84
^ permalink raw reply related [flat|nested] 75+ messages in thread
* Re: [PATCHv2 5/6] t7510: test verify-commit
2014-06-13 10:42 ` [PATCHv2 5/6] t7510: test verify-commit Michael J Gruber
@ 2014-06-13 11:51 ` Jeff King
2014-06-13 12:14 ` Michael J Gruber
0 siblings, 1 reply; 75+ messages in thread
From: Jeff King @ 2014-06-13 11:51 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
On Fri, Jun 13, 2014 at 12:42:47PM +0200, Michael J Gruber wrote:
> test_expect_success GPG 'detect fudged signature' '
> git cat-file commit master >raw &&
>
> sed -e "s/seventh/7th forged/" raw >forged1 &&
> git hash-object -w -t commit forged1 >forged1.commit &&
> + ! git verify-commit $(cat forged1.commit) &&
Please use test_must_fail here (and further down), which will catch
things like signal death.
-Peff
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv2 5/6] t7510: test verify-commit
2014-06-13 11:51 ` Jeff King
@ 2014-06-13 12:14 ` Michael J Gruber
2014-06-13 18:16 ` Junio C Hamano
0 siblings, 1 reply; 75+ messages in thread
From: Michael J Gruber @ 2014-06-13 12:14 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
Jeff King venit, vidit, dixit 13.06.2014 13:51:
> On Fri, Jun 13, 2014 at 12:42:47PM +0200, Michael J Gruber wrote:
>
>> test_expect_success GPG 'detect fudged signature' '
>> git cat-file commit master >raw &&
>>
>> sed -e "s/seventh/7th forged/" raw >forged1 &&
>> git hash-object -w -t commit forged1 >forged1.commit &&
>> + ! git verify-commit $(cat forged1.commit) &&
>
> Please use test_must_fail here (and further down), which will catch
> things like signal death.
Again, that is an issue of keeping the style of the surrounding code
(which is relatively new) vs. doing it differently. I don't mind
changing t7510 to a different style, though.
Michael
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv2 5/6] t7510: test verify-commit
2014-06-13 12:14 ` Michael J Gruber
@ 2014-06-13 18:16 ` Junio C Hamano
0 siblings, 0 replies; 75+ messages in thread
From: Junio C Hamano @ 2014-06-13 18:16 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Jeff King, git
Michael J Gruber <git@drmicha.warpmail.net> writes:
> Jeff King venit, vidit, dixit 13.06.2014 13:51:
>> On Fri, Jun 13, 2014 at 12:42:47PM +0200, Michael J Gruber wrote:
>>
>>> test_expect_success GPG 'detect fudged signature' '
>>> git cat-file commit master >raw &&
>>>
>>> sed -e "s/seventh/7th forged/" raw >forged1 &&
>>> git hash-object -w -t commit forged1 >forged1.commit &&
>>> + ! git verify-commit $(cat forged1.commit) &&
>>
>> Please use test_must_fail here (and further down), which will catch
>> things like signal death.
>
> Again, that is an issue of keeping the style of the surrounding code
> (which is relatively new) vs. doing it differently. I don't mind
> changing t7510 to a different style, though.
We do not have any "! git anything" in that script, don't we? We do
not expect "grep" supplied by the platform to lie, and that is why
we should never use test_must_fail on them, but we do want to notice
when git starts segfaulting, and that is what test_must_fail is for.
We tell novices and cluelesses who do not know better to try to
follow and match the surrounding style. This is because it would at
least not make things worse than if we let them run loose on our
codebase. It is not about "if we have a bad style, it is better to
match that existing bad style to spread the badness than making it
partly better". But you have been here long enough and know what is
preferred and and what is to be avoided---more importantly, Peff has
been here long enough to know and has given such an advice, so...
In any case, I do not offhand see anything wrong style-wise in 7510,
so please do not change anything in it for the sake of styles.
^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCHv2 6/6] gpg-interface: provide clear helper for struct signature_check
2014-06-13 10:42 ` [PATCHv2 0/6] verify-commit: verify commit signatures Michael J Gruber
` (4 preceding siblings ...)
2014-06-13 10:42 ` [PATCHv2 5/6] t7510: test verify-commit Michael J Gruber
@ 2014-06-13 10:42 ` Michael J Gruber
2014-06-23 7:05 ` [PATCHv3 0/5] verify-commit: verify commit signatures Michael J Gruber
6 siblings, 0 replies; 75+ messages in thread
From: Michael J Gruber @ 2014-06-13 10:42 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
The struct has been growing members whose malloced memory needs to be
freed. Do this with one helper function so that no malloced memory shall
be left unfreed.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
builtin/merge.c | 6 +-----
builtin/verify-commit.c | 5 +----
gpg-interface.c | 14 ++++++++++++++
gpg-interface.h | 1 +
pretty.c | 5 +----
5 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 6a9812a..e50323d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1282,11 +1282,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
printf(_("Commit %s has a good GPG signature by %s\n"),
hex, signature_check.signer);
- free(signature_check.payload);
- free(signature_check.gpg_output);
- free(signature_check.gpg_status);
- free(signature_check.signer);
- free(signature_check.key);
+ signature_check_clear(&signature_check);
}
}
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
index 69b7c6d..d254fcf 100644
--- a/builtin/verify-commit.c
+++ b/builtin/verify-commit.c
@@ -32,10 +32,7 @@ static int run_gpg_verify(const unsigned char *sha1, const char *buf, unsigned l
if (signature_check.gpg_output)
fputs(signature_check.gpg_output, stderr);
- free(signature_check.gpg_output);
- free(signature_check.gpg_status);
- free(signature_check.signer);
- free(signature_check.key);
+ signature_check_clear(&signature_check);
return signature_check.result != 'G';
}
diff --git a/gpg-interface.c b/gpg-interface.c
index 8b0e874..ff07012 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -7,6 +7,20 @@
static char *configured_signing_key;
static const char *gpg_program = "gpg";
+void signature_check_clear(struct signature_check *sigc)
+{
+ free(sigc->payload);
+ free(sigc->gpg_output);
+ free(sigc->gpg_status);
+ free(sigc->signer);
+ free(sigc->key);
+ sigc->payload = NULL;
+ sigc->gpg_output = NULL;
+ sigc->gpg_status = NULL;
+ sigc->signer = NULL;
+ sigc->key = NULL;
+}
+
void set_signing_key(const char *key)
{
free(configured_signing_key);
diff --git a/gpg-interface.h b/gpg-interface.h
index d727c25..37c23da 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -14,6 +14,7 @@ struct signature_check {
char *key;
};
+extern void signature_check_clear(struct signature_check *sigc);
extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key);
extern int verify_signed_buffer(const char *payload, size_t payload_size, const char *signature, size_t signature_size, struct strbuf *gpg_output, struct strbuf *gpg_status);
extern int git_gpg_config(const char *, const char *, void *);
diff --git a/pretty.c b/pretty.c
index 24fb877..ac901b8 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1537,10 +1537,7 @@ void format_commit_message(const struct commit *commit,
free(context.commit_encoding);
logmsg_free(context.message, commit);
- free(context.signature_check.payload);
- free(context.signature_check.gpg_output);
- free(context.signature_check.gpg_status);
- free(context.signature_check.signer);
+ signature_check_clear(&context.signature_check);
}
static void pp_header(struct pretty_print_context *pp,
--
2.0.0.426.g37dbf84
^ permalink raw reply related [flat|nested] 75+ messages in thread
* [PATCHv3 0/5] verify-commit: verify commit signatures
2014-06-13 10:42 ` [PATCHv2 0/6] verify-commit: verify commit signatures Michael J Gruber
` (5 preceding siblings ...)
2014-06-13 10:42 ` [PATCHv2 6/6] gpg-interface: provide clear helper for struct signature_check Michael J Gruber
@ 2014-06-23 7:05 ` Michael J Gruber
2014-06-23 7:05 ` [PATCHv3 1/5] gpg-interface: provide clear helper for struct signature_check Michael J Gruber
` (5 more replies)
6 siblings, 6 replies; 75+ messages in thread
From: Michael J Gruber @ 2014-06-23 7:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
This incorporates all remarks about the test coding guidelines and
rearranging the series.
Open questions:
- There was some debate about (optionally) verifying more than what
git-verify-{commit,tag} currently do, or going for a generic git-verify command.
The former would require both to be changed (in order to treat similar cases similarly),
the latter would need a deprecation for git-verify-tag.
- I haven't looked yet at what happened over the weekend.
Michael J Gruber (5):
gpg-interface: provide clear helper for struct signature_check
gpg-interface: provide access to the payload
verify-commit: scriptable commit signature verification
t7510: exit for loop with test result
t7510: test verify-commit
Documentation/git-verify-commit.txt | 28 +++++++++++
Makefile | 1 +
builtin.h | 1 +
builtin/merge.c | 5 +-
builtin/verify-commit.c | 93 +++++++++++++++++++++++++++++++++++++
command-list.txt | 1 +
commit.c | 1 +
git.c | 1 +
gpg-interface.c | 14 ++++++
gpg-interface.h | 2 +
pretty.c | 3 +-
t/t7510-signed-commit.sh | 24 ++++++++--
12 files changed, 165 insertions(+), 9 deletions(-)
create mode 100644 Documentation/git-verify-commit.txt
create mode 100644 builtin/verify-commit.c
--
2.0.0.426.g37dbf84
^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCHv3 1/5] gpg-interface: provide clear helper for struct signature_check
2014-06-23 7:05 ` [PATCHv3 0/5] verify-commit: verify commit signatures Michael J Gruber
@ 2014-06-23 7:05 ` Michael J Gruber
2014-06-23 7:05 ` [PATCHv3 2/5] gpg-interface: provide access to the payload Michael J Gruber
` (4 subsequent siblings)
5 siblings, 0 replies; 75+ messages in thread
From: Michael J Gruber @ 2014-06-23 7:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
The struct has been growing members whose malloced memory needs to be
freed. Do this with one helper function so that no malloced memory shall
be left unfreed.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
builtin/merge.c | 5 +----
gpg-interface.c | 12 ++++++++++++
gpg-interface.h | 1 +
pretty.c | 3 +--
4 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 428ca24..e50323d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1282,10 +1282,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
printf(_("Commit %s has a good GPG signature by %s\n"),
hex, signature_check.signer);
- free(signature_check.gpg_output);
- free(signature_check.gpg_status);
- free(signature_check.signer);
- free(signature_check.key);
+ signature_check_clear(&signature_check);
}
}
diff --git a/gpg-interface.c b/gpg-interface.c
index 8b0e874..e71b59d 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -7,6 +7,18 @@
static char *configured_signing_key;
static const char *gpg_program = "gpg";
+void signature_check_clear(struct signature_check *sigc)
+{
+ free(sigc->gpg_output);
+ free(sigc->gpg_status);
+ free(sigc->signer);
+ free(sigc->key);
+ sigc->gpg_output = NULL;
+ sigc->gpg_status = NULL;
+ sigc->signer = NULL;
+ sigc->key = NULL;
+}
+
void set_signing_key(const char *key)
{
free(configured_signing_key);
diff --git a/gpg-interface.h b/gpg-interface.h
index a85cb5b..9f0784a 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -13,6 +13,7 @@ struct signature_check {
char *key;
};
+extern void signature_check_clear(struct signature_check *sigc);
extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key);
extern int verify_signed_buffer(const char *payload, size_t payload_size, const char *signature, size_t signature_size, struct strbuf *gpg_output, struct strbuf *gpg_status);
extern int git_gpg_config(const char *, const char *, void *);
diff --git a/pretty.c b/pretty.c
index 4f51287..ac901b8 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1537,8 +1537,7 @@ void format_commit_message(const struct commit *commit,
free(context.commit_encoding);
logmsg_free(context.message, commit);
- free(context.signature_check.gpg_output);
- free(context.signature_check.signer);
+ signature_check_clear(&context.signature_check);
}
static void pp_header(struct pretty_print_context *pp,
--
2.0.0.426.g37dbf84
^ permalink raw reply related [flat|nested] 75+ messages in thread
* [PATCHv3 2/5] gpg-interface: provide access to the payload
2014-06-23 7:05 ` [PATCHv3 0/5] verify-commit: verify commit signatures Michael J Gruber
2014-06-23 7:05 ` [PATCHv3 1/5] gpg-interface: provide clear helper for struct signature_check Michael J Gruber
@ 2014-06-23 7:05 ` Michael J Gruber
2014-06-23 7:05 ` [PATCHv3 3/5] verify-commit: scriptable commit signature verification Michael J Gruber
` (3 subsequent siblings)
5 siblings, 0 replies; 75+ messages in thread
From: Michael J Gruber @ 2014-06-23 7:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In contrast to tag signatures, commit signatures are put into the
header, that is between the other header parts and commit messages.
Provide access to the commit content sans the signature, which is the
payload that is actually signed. Commit signature verification does the
parsing anyways, and callers may wish to act on or display the commit
object sans the signature.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
commit.c | 1 +
gpg-interface.c | 2 ++
gpg-interface.h | 1 +
3 files changed, 4 insertions(+)
diff --git a/commit.c b/commit.c
index 881be3b..d0ad7f0 100644
--- a/commit.c
+++ b/commit.c
@@ -1219,6 +1219,7 @@ void check_commit_signature(const struct commit* commit, struct signature_check
&gpg_output, &gpg_status);
if (status && !gpg_output.len)
goto out;
+ sigc->payload = strbuf_detach(&payload, NULL);
sigc->gpg_output = strbuf_detach(&gpg_output, NULL);
sigc->gpg_status = strbuf_detach(&gpg_status, NULL);
parse_gpg_output(sigc);
diff --git a/gpg-interface.c b/gpg-interface.c
index e71b59d..ff07012 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -9,10 +9,12 @@ static const char *gpg_program = "gpg";
void signature_check_clear(struct signature_check *sigc)
{
+ free(sigc->payload);
free(sigc->gpg_output);
free(sigc->gpg_status);
free(sigc->signer);
free(sigc->key);
+ sigc->payload = NULL;
sigc->gpg_output = NULL;
sigc->gpg_status = NULL;
sigc->signer = NULL;
diff --git a/gpg-interface.h b/gpg-interface.h
index 9f0784a..37c23da 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -2,6 +2,7 @@
#define GPG_INTERFACE_H
struct signature_check {
+ char *payload;
char *gpg_output;
char *gpg_status;
char result; /* 0 (not checked),
--
2.0.0.426.g37dbf84
^ permalink raw reply related [flat|nested] 75+ messages in thread
* [PATCHv3 3/5] verify-commit: scriptable commit signature verification
2014-06-23 7:05 ` [PATCHv3 0/5] verify-commit: verify commit signatures Michael J Gruber
2014-06-23 7:05 ` [PATCHv3 1/5] gpg-interface: provide clear helper for struct signature_check Michael J Gruber
2014-06-23 7:05 ` [PATCHv3 2/5] gpg-interface: provide access to the payload Michael J Gruber
@ 2014-06-23 7:05 ` Michael J Gruber
2014-06-23 7:05 ` [PATCHv3 4/5] t7510: exit for loop with test result Michael J Gruber
` (2 subsequent siblings)
5 siblings, 0 replies; 75+ messages in thread
From: Michael J Gruber @ 2014-06-23 7:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
Commit signatures can be verified using "git show -s --show-signature"
or the "%G?" pretty format and parsing the output, which is well suited
for user inspection, but not for scripting.
Provide a command "verify-commit" which is analogous to "verify-tag": It
returns 0 for good signatures and non-zero otherwise, has the gpg output
on stderr and (optionally) the commit object on stdout, sans the
signature, just like "verify-tag" does.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Documentation/git-verify-commit.txt | 28 +++++++++++
Makefile | 1 +
builtin.h | 1 +
builtin/verify-commit.c | 93 +++++++++++++++++++++++++++++++++++++
command-list.txt | 1 +
git.c | 1 +
6 files changed, 125 insertions(+)
create mode 100644 Documentation/git-verify-commit.txt
create mode 100644 builtin/verify-commit.c
diff --git a/Documentation/git-verify-commit.txt b/Documentation/git-verify-commit.txt
new file mode 100644
index 0000000..9413e28
--- /dev/null
+++ b/Documentation/git-verify-commit.txt
@@ -0,0 +1,28 @@
+git-verify-commit(1)
+====================
+
+NAME
+----
+git-verify-commit - Check the GPG signature of commits
+
+SYNOPSIS
+--------
+[verse]
+'git verify-commit' <commit>...
+
+DESCRIPTION
+-----------
+Validates the gpg signature created by 'git commit -S'.
+
+OPTIONS
+-------
+-v::
+--verbose::
+ Print the contents of the commit object before validating it.
+
+<commit>...::
+ SHA-1 identifiers of Git commit objects.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 07ea105..b92418d 100644
--- a/Makefile
+++ b/Makefile
@@ -999,6 +999,7 @@ BUILTIN_OBJS += builtin/update-ref.o
BUILTIN_OBJS += builtin/update-server-info.o
BUILTIN_OBJS += builtin/upload-archive.o
BUILTIN_OBJS += builtin/var.o
+BUILTIN_OBJS += builtin/verify-commit.o
BUILTIN_OBJS += builtin/verify-pack.o
BUILTIN_OBJS += builtin/verify-tag.o
BUILTIN_OBJS += builtin/write-tree.o
diff --git a/builtin.h b/builtin.h
index c47c110..5d91f31 100644
--- a/builtin.h
+++ b/builtin.h
@@ -128,6 +128,7 @@ extern int cmd_update_server_info(int argc, const char **argv, const char *prefi
extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);
extern int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix);
extern int cmd_var(int argc, const char **argv, const char *prefix);
+extern int cmd_verify_commit(int argc, const char **argv, const char *prefix);
extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);
extern int cmd_version(int argc, const char **argv, const char *prefix);
extern int cmd_whatchanged(int argc, const char **argv, const char *prefix);
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
new file mode 100644
index 0000000..b0f8504
--- /dev/null
+++ b/builtin/verify-commit.c
@@ -0,0 +1,93 @@
+/*
+ * Builtin "git commit-commit"
+ *
+ * Copyright (c) 2014 Michael J Gruber <git@drmicha.warpmail.net>
+ *
+ * Based on git-verify-tag
+ */
+#include "cache.h"
+#include "builtin.h"
+#include "commit.h"
+#include "run-command.h"
+#include <signal.h>
+#include "parse-options.h"
+#include "gpg-interface.h"
+
+static const char * const verify_commit_usage[] = {
+ N_("git verify-commit [-v|--verbose] <commit>..."),
+ NULL
+};
+
+static int run_gpg_verify(const unsigned char *sha1, const char *buf, unsigned long size, int verbose)
+{
+ struct signature_check signature_check;
+
+ memset(&signature_check, 0, sizeof(signature_check));
+
+ check_commit_signature(lookup_commit(sha1), &signature_check);
+
+ if (verbose && signature_check.payload)
+ fputs(signature_check.payload, stdout);
+
+ if (signature_check.gpg_output)
+ fputs(signature_check.gpg_output, stderr);
+
+ signature_check_clear(&signature_check);
+ return signature_check.result != 'G';
+}
+
+static int verify_commit(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("commit '%s' not found.", name);
+
+ buf = read_sha1_file(sha1, &type, &size);
+ if (!buf)
+ return error("%s: unable to read file.", name);
+ if (type != OBJ_COMMIT)
+ return error("%s: cannot verify a non-commit object of type %s.",
+ name, typename(type));
+
+ ret = run_gpg_verify(sha1, buf, size, verbose);
+
+ free(buf);
+ return ret;
+}
+
+static int git_verify_commit_config(const char *var, const char *value, void *cb)
+{
+ int status = git_gpg_config(var, value, cb);
+ if (status)
+ return status;
+ return git_default_config(var, value, cb);
+}
+
+int cmd_verify_commit(int argc, const char **argv, const char *prefix)
+{
+ int i = 1, verbose = 0, had_error = 0;
+ const struct option verify_commit_options[] = {
+ OPT__VERBOSE(&verbose, N_("print commit contents")),
+ OPT_END()
+ };
+
+ git_config(git_verify_commit_config, NULL);
+
+ argc = parse_options(argc, argv, prefix, verify_commit_options,
+ verify_commit_usage, PARSE_OPT_KEEP_ARGV0);
+ if (argc <= i)
+ usage_with_options(verify_commit_usage, verify_commit_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_commit(argv[i++], verbose))
+ had_error = 1;
+ return had_error;
+}
diff --git a/command-list.txt b/command-list.txt
index cf36c3d..a3ff0c9 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -132,6 +132,7 @@ git-update-server-info synchingrepositories
git-upload-archive synchelpers
git-upload-pack synchelpers
git-var plumbinginterrogators
+git-verify-commit ancillaryinterrogators
git-verify-pack plumbinginterrogators
git-verify-tag ancillaryinterrogators
gitweb ancillaryinterrogators
diff --git a/git.c b/git.c
index 7780572..bbc3293 100644
--- a/git.c
+++ b/git.c
@@ -441,6 +441,7 @@ static struct cmd_struct commands[] = {
{ "upload-archive", cmd_upload_archive },
{ "upload-archive--writer", cmd_upload_archive_writer },
{ "var", cmd_var, RUN_SETUP_GENTLY },
+ { "verify-commit", cmd_verify_commit, RUN_SETUP },
{ "verify-pack", cmd_verify_pack },
{ "verify-tag", cmd_verify_tag, RUN_SETUP },
{ "version", cmd_version },
--
2.0.0.426.g37dbf84
^ permalink raw reply related [flat|nested] 75+ messages in thread
* [PATCHv3 4/5] t7510: exit for loop with test result
2014-06-23 7:05 ` [PATCHv3 0/5] verify-commit: verify commit signatures Michael J Gruber
` (2 preceding siblings ...)
2014-06-23 7:05 ` [PATCHv3 3/5] verify-commit: scriptable commit signature verification Michael J Gruber
@ 2014-06-23 7:05 ` Michael J Gruber
2014-06-23 7:05 ` [PATCHv3 5/5] t7510: test verify-commit Michael J Gruber
2014-06-23 17:28 ` [PATCHv3 0/5] verify-commit: verify commit signatures Jeff King
5 siblings, 0 replies; 75+ messages in thread
From: Michael J Gruber @ 2014-06-23 7:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
t7510 uses for loops in a subshell, which need to make sure that the test
returns with the appropriate error code from within the loop.
Restructure the loops as the usual && chains with a single point of
"exit 1" at the end of the loop to make this clearer.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
t/t7510-signed-commit.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 5ddac1a..96cfddf 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -50,7 +50,7 @@ test_expect_success GPG 'show signatures' '
for commit in initial second merge fourth-signed fifth-signed sixth-signed master
do
git show --pretty=short --show-signature $commit >actual &&
- grep "Good signature from" actual || exit 1
+ grep "Good signature from" actual &&
! grep "BAD signature from" actual || exit 1
echo $commit OK
done
@@ -59,7 +59,7 @@ test_expect_success GPG 'show signatures' '
for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
do
git show --pretty=short --show-signature $commit >actual &&
- grep "Good signature from" actual && exit 1
+ ! grep "Good signature from" actual &&
! grep "BAD signature from" actual || exit 1
echo $commit OK
done
--
2.0.0.426.g37dbf84
^ permalink raw reply related [flat|nested] 75+ messages in thread
* [PATCHv3 5/5] t7510: test verify-commit
2014-06-23 7:05 ` [PATCHv3 0/5] verify-commit: verify commit signatures Michael J Gruber
` (3 preceding siblings ...)
2014-06-23 7:05 ` [PATCHv3 4/5] t7510: exit for loop with test result Michael J Gruber
@ 2014-06-23 7:05 ` Michael J Gruber
2014-06-23 23:02 ` Junio C Hamano
2014-06-23 17:28 ` [PATCHv3 0/5] verify-commit: verify commit signatures Jeff King
5 siblings, 1 reply; 75+ messages in thread
From: Michael J Gruber @ 2014-06-23 7:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
This mixes the "git verify-commit" tests in with the "git show
--show-signature" tests, to keep the tests more readable.
The tests already mix in the "call show" tests with the "verify" tests.
So in case of a test beakage, a '-v' run would be needed to reveal the
exact point of breakage anyway.
Additionally, test the actual output of "git verify-commit" and "git
show --show-signature" and compare to "git cat-file".
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
t/t7510-signed-commit.sh | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 96cfddf..dd4b948 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -45,10 +45,11 @@ test_expect_success GPG 'create signed commits' '
git tag seventh-signed
'
-test_expect_success GPG 'show signatures' '
+test_expect_success GPG 'verify and show signatures' '
(
for commit in initial second merge fourth-signed fifth-signed sixth-signed master
do
+ git verify-commit $commit &&
git show --pretty=short --show-signature $commit >actual &&
grep "Good signature from" actual &&
! grep "BAD signature from" actual || exit 1
@@ -58,6 +59,7 @@ test_expect_success GPG 'show signatures' '
(
for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
do
+ test_must_fail git verify-commit $commit &&
git show --pretty=short --show-signature $commit >actual &&
! grep "Good signature from" actual &&
! grep "BAD signature from" actual || exit 1
@@ -66,11 +68,25 @@ test_expect_success GPG 'show signatures' '
)
'
+test_expect_success GPG 'show signed commit with signature' '
+ git show -s initial >commit &&
+ git show -s --show-signature initial >show &&
+ git verify-commit -v initial >verify.1 2>verify.2 &&
+ git cat-file commit initial >cat &&
+ grep -v "gpg: " show >show.commit &&
+ grep "gpg: " show >show.gpg &&
+ grep -v "^ " cat | grep -v "^gpgsig " >cat.commit &&
+ test_cmp show.commit commit &&
+ test_cmp show.gpg verify.2 &&
+ test_cmp cat.commit verify.1
+'
+
test_expect_success GPG 'detect fudged signature' '
git cat-file commit master >raw &&
sed -e "s/seventh/7th forged/" raw >forged1 &&
git hash-object -w -t commit forged1 >forged1.commit &&
+ ! git verify-commit $(cat forged1.commit) &&
git show --pretty=short --show-signature $(cat forged1.commit) >actual1 &&
grep "BAD signature from" actual1 &&
! grep "Good signature from" actual1
@@ -81,6 +97,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
cat raw >forged2 &&
echo Qwik | tr "Q" "\000" >>forged2 &&
git hash-object -w -t commit forged2 >forged2.commit &&
+ ! git verify-commit $(cat forged2.commit) &&
git show --pretty=short --show-signature $(cat forged2.commit) >actual2 &&
grep "BAD signature from" actual2 &&
! grep "Good signature from" actual2
@@ -89,6 +106,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
test_expect_success GPG 'amending already signed commit' '
git checkout fourth-signed^0 &&
git commit --amend -S --no-edit &&
+ git verify-commit HEAD &&
git show -s --show-signature HEAD >actual &&
grep "Good signature from" actual &&
! grep "BAD signature from" actual
--
2.0.0.426.g37dbf84
^ permalink raw reply related [flat|nested] 75+ messages in thread
* Re: [PATCHv3 5/5] t7510: test verify-commit
2014-06-23 7:05 ` [PATCHv3 5/5] t7510: test verify-commit Michael J Gruber
@ 2014-06-23 23:02 ` Junio C Hamano
0 siblings, 0 replies; 75+ messages in thread
From: Junio C Hamano @ 2014-06-23 23:02 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Jeff King
Michael J Gruber <git@drmicha.warpmail.net> writes:
> test_expect_success GPG 'detect fudged signature' '
> git cat-file commit master >raw &&
>
> sed -e "s/seventh/7th forged/" raw >forged1 &&
> git hash-object -w -t commit forged1 >forged1.commit &&
> + ! git verify-commit $(cat forged1.commit) &&
This should be "test_must_fail git verify-commit ...", I think.
Otherwise you would end up declaring a segfaulting implementation a
good one.
> + ! git verify-commit $(cat forged2.commit) &&
Likewise.
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv3 0/5] verify-commit: verify commit signatures
2014-06-23 7:05 ` [PATCHv3 0/5] verify-commit: verify commit signatures Michael J Gruber
` (4 preceding siblings ...)
2014-06-23 7:05 ` [PATCHv3 5/5] t7510: test verify-commit Michael J Gruber
@ 2014-06-23 17:28 ` Jeff King
2014-06-23 17:52 ` Junio C Hamano
5 siblings, 1 reply; 75+ messages in thread
From: Jeff King @ 2014-06-23 17:28 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
On Mon, Jun 23, 2014 at 09:05:46AM +0200, Michael J Gruber wrote:
> This incorporates all remarks about the test coding guidelines and
> rearranging the series.
>
> Open questions:
> - There was some debate about (optionally) verifying more than what
> git-verify-{commit,tag} currently do, or going for a generic git-verify command.
> The former would require both to be changed (in order to treat similar cases similarly),
> the latter would need a deprecation for git-verify-tag.
I think that a potential "git verify" doesn't need to block this series,
per the logic I gave elsewhere.
The one thing that does give me pause is that we do not seem to have any
way of accessing mergetag signatures. We should perhaps stop and think
for a second about how we might expose those (and whether it would fit
into the "git-verify-{commit,tag}" paradigm). I am tempted to say that
"git verify-tag" on a commit should verify the mergetag (right now it
would simply be an error). But I haven't though that hard on it.
I don't think implementation of that needs to be a blocker for this
series, but I'd rather see at least a vague plan so that we do not paint
ourselves into a corner.
> Michael J Gruber (5):
> gpg-interface: provide clear helper for struct signature_check
> gpg-interface: provide access to the payload
> verify-commit: scriptable commit signature verification
> t7510: exit for loop with test result
> t7510: test verify-commit
I didn't see anything objectionable here. I think you may want to rebase
on top of jk/pretty-G-format-fixes. That makes your patch 4 redundant,
and your patch 5 will probably need a few minor updates to match
cleanups in the surrounding code.
-Peff
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv3 0/5] verify-commit: verify commit signatures
2014-06-23 17:28 ` [PATCHv3 0/5] verify-commit: verify commit signatures Jeff King
@ 2014-06-23 17:52 ` Junio C Hamano
2014-06-23 21:09 ` Jeff King
0 siblings, 1 reply; 75+ messages in thread
From: Junio C Hamano @ 2014-06-23 17:52 UTC (permalink / raw)
To: Jeff King; +Cc: Michael J Gruber, git
Jeff King <peff@peff.net> writes:
> On Mon, Jun 23, 2014 at 09:05:46AM +0200, Michael J Gruber wrote:
>
>> This incorporates all remarks about the test coding guidelines and
>> rearranging the series.
>>
>> Open questions:
>> - There was some debate about (optionally) verifying more than what
>> git-verify-{commit,tag} currently do, or going for a generic git-verify command.
>> The former would require both to be changed (in order to treat similar cases similarly),
>> the latter would need a deprecation for git-verify-tag.
>
> I think that a potential "git verify" doesn't need to block this series,
> per the logic I gave elsewhere.
>
> The one thing that does give me pause is that we do not seem to have any
> way of accessing mergetag signatures. We should perhaps stop and think
> for a second about how we might expose those (and whether it would fit
> into the "git-verify-{commit,tag}" paradigm). I am tempted to say that
> "git verify-tag" on a commit should verify the mergetag (right now it
> would simply be an error). But I haven't though that hard on it.
I agree that "verify-commit" that lives next to "verify-tag" is fine
and does not have to wait for a unified "verify" that may not even
be a good idea, but if we were to verify the mergetags in one of
these "verify-$OBJECTTYPE" commands, I would think "verify-commit"
should be the one to check them, for the simple reason that they
appear in "commit" objects, not in "tag" objects.
I would imagine that I would not mind too much if "verify-tag"
delegated the verification to "verify-commit" automatically when it
is given a commit object, but for a command fairly low-level to be
useful for scripting, such a DWIMmage may be too unexpected and make
them unnecessarily unreliable. Using scripts that want strictness
(and who in the right mind that wants to verify things do not want
strictness?) would need to "cat-file -t" upfront to switch on the
object type.
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv3 0/5] verify-commit: verify commit signatures
2014-06-23 17:52 ` Junio C Hamano
@ 2014-06-23 21:09 ` Jeff King
2014-06-23 21:23 ` Junio C Hamano
0 siblings, 1 reply; 75+ messages in thread
From: Jeff King @ 2014-06-23 21:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael J Gruber, git
On Mon, Jun 23, 2014 at 10:52:38AM -0700, Junio C Hamano wrote:
> > The one thing that does give me pause is that we do not seem to have any
> > way of accessing mergetag signatures. We should perhaps stop and think
> > for a second about how we might expose those (and whether it would fit
> > into the "git-verify-{commit,tag}" paradigm). I am tempted to say that
> > "git verify-tag" on a commit should verify the mergetag (right now it
> > would simply be an error). But I haven't though that hard on it.
>
> I agree that "verify-commit" that lives next to "verify-tag" is fine
> and does not have to wait for a unified "verify" that may not even
> be a good idea, but if we were to verify the mergetags in one of
> these "verify-$OBJECTTYPE" commands, I would think "verify-commit"
> should be the one to check them, for the simple reason that they
> appear in "commit" objects, not in "tag" objects.
My thinking was the opposite: it is a signature on a tag, but that
signature happens to be stuffed into a commit object. But I do not have
a strong feeling either way.
-Peff
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv3 0/5] verify-commit: verify commit signatures
2014-06-23 21:09 ` Jeff King
@ 2014-06-23 21:23 ` Junio C Hamano
2014-06-27 14:13 ` [PATCHv4 0/4] " Michael J Gruber
0 siblings, 1 reply; 75+ messages in thread
From: Junio C Hamano @ 2014-06-23 21:23 UTC (permalink / raw)
To: Jeff King; +Cc: Michael J Gruber, git
Jeff King <peff@peff.net> writes:
> On Mon, Jun 23, 2014 at 10:52:38AM -0700, Junio C Hamano wrote:
>
>> > The one thing that does give me pause is that we do not seem to have any
>> > way of accessing mergetag signatures. We should perhaps stop and think
>> > for a second about how we might expose those (and whether it would fit
>> > into the "git-verify-{commit,tag}" paradigm). I am tempted to say that
>> > "git verify-tag" on a commit should verify the mergetag (right now it
>> > would simply be an error). But I haven't though that hard on it.
>>
>> I agree that "verify-commit" that lives next to "verify-tag" is fine
>> and does not have to wait for a unified "verify" that may not even
>> be a good idea, but if we were to verify the mergetags in one of
>> these "verify-$OBJECTTYPE" commands, I would think "verify-commit"
>> should be the one to check them, for the simple reason that they
>> appear in "commit" objects, not in "tag" objects.
>
> My thinking was the opposite: it is a signature on a tag, but that
> signature happens to be stuffed into a commit object. But I do not have
> a strong feeling either way.
Well, the whole point of storing mergetag inside commit objects is
so that these transient "please pull, here is a tag to prove you
that it is from me" tags do not have to be kept in the history;
hence people who are following along only see commits and not tags.
The signature being sent via a signed tag from the requestor to the
integrator is merely an implementation detail after the mergetag is
recorded and when people would want to verify them.
So...
^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCHv4 0/4] verify-commit: verify commit signatures
2014-06-23 21:23 ` Junio C Hamano
@ 2014-06-27 14:13 ` Michael J Gruber
2014-06-27 14:13 ` [PATCHv4 1/4] gpg-interface: provide clear helper for struct signature_check Michael J Gruber
` (5 more replies)
0 siblings, 6 replies; 75+ messages in thread
From: Michael J Gruber @ 2014-06-27 14:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
This is v3 rebased on current next (the %G works by Jeff & Junio).
Open questions:
- Should one of git verify-{commit,tag} learn how to verify mergetags?
(Probably no, it differs from both other cases.)
- Should we do this now or go for generic "git verify" right away?
That depends on whether signed commits need to be verified by scripts now,
or whether mergetags are more important.
For a general command which allows different verification policies,
I'm still wondering whether we may need hooks which receive all
the relevant information in the environment. Otherwise we'll have a ton of
options such as --match-committer-uid, --verify--AllParentsHaveMergeTags,
--verify--All-ParentsAreSignedCommits, --peel-to-commit, --merge-commit-only, ...
I imagine that a generic "git verify" would provide "git verify-{commit,tag}"
aliases which call "git verify" with options that reproduce the current (suggested)
behavior.
Michael J Gruber (4):
gpg-interface: provide clear helper for struct signature_check
gpg-interface: provide access to the payload
verify-commit: scriptable commit signature verification
t7510: test verify-commit
Documentation/git-verify-commit.txt | 28 +++++++++++
Makefile | 1 +
builtin.h | 1 +
builtin/merge.c | 5 +-
builtin/verify-commit.c | 93 +++++++++++++++++++++++++++++++++++++
command-list.txt | 1 +
commit.c | 1 +
git.c | 1 +
gpg-interface.c | 14 ++++++
gpg-interface.h | 2 +
pretty.c | 3 +-
t/t7510-signed-commit.sh | 20 +++++++-
12 files changed, 163 insertions(+), 7 deletions(-)
create mode 100644 Documentation/git-verify-commit.txt
create mode 100644 builtin/verify-commit.c
--
2.0.1.563.g162087b.dirty
^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCHv4 1/4] gpg-interface: provide clear helper for struct signature_check
2014-06-27 14:13 ` [PATCHv4 0/4] " Michael J Gruber
@ 2014-06-27 14:13 ` Michael J Gruber
2014-06-27 14:13 ` [PATCHv4 2/4] gpg-interface: provide access to the payload Michael J Gruber
` (4 subsequent siblings)
5 siblings, 0 replies; 75+ messages in thread
From: Michael J Gruber @ 2014-06-27 14:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
The struct has been growing members whose malloced memory needs to be
freed. Do this with one helper function so that no malloced memory shall
be left unfreed.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
builtin/merge.c | 5 +----
gpg-interface.c | 12 ++++++++++++
gpg-interface.h | 1 +
pretty.c | 3 +--
4 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index b49c310..86e9c61 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1282,10 +1282,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
printf(_("Commit %s has a good GPG signature by %s\n"),
hex, signature_check.signer);
- free(signature_check.gpg_output);
- free(signature_check.gpg_status);
- free(signature_check.signer);
- free(signature_check.key);
+ signature_check_clear(&signature_check);
}
}
diff --git a/gpg-interface.c b/gpg-interface.c
index 8b0e874..e71b59d 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -7,6 +7,18 @@
static char *configured_signing_key;
static const char *gpg_program = "gpg";
+void signature_check_clear(struct signature_check *sigc)
+{
+ free(sigc->gpg_output);
+ free(sigc->gpg_status);
+ free(sigc->signer);
+ free(sigc->key);
+ sigc->gpg_output = NULL;
+ sigc->gpg_status = NULL;
+ sigc->signer = NULL;
+ sigc->key = NULL;
+}
+
void set_signing_key(const char *key)
{
free(configured_signing_key);
diff --git a/gpg-interface.h b/gpg-interface.h
index a85cb5b..9f0784a 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -13,6 +13,7 @@ struct signature_check {
char *key;
};
+extern void signature_check_clear(struct signature_check *sigc);
extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key);
extern int verify_signed_buffer(const char *payload, size_t payload_size, const char *signature, size_t signature_size, struct strbuf *gpg_output, struct strbuf *gpg_status);
extern int git_gpg_config(const char *, const char *, void *);
diff --git a/pretty.c b/pretty.c
index ce3bbaa..353af81 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1520,8 +1520,7 @@ void format_commit_message(const struct commit *commit,
free(context.commit_encoding);
unuse_commit_buffer(commit, context.message);
- free(context.signature_check.gpg_output);
- free(context.signature_check.signer);
+ signature_check_clear(&context.signature_check);
}
static void pp_header(struct pretty_print_context *pp,
--
2.0.1.563.g162087b.dirty
^ permalink raw reply related [flat|nested] 75+ messages in thread
* [PATCHv4 2/4] gpg-interface: provide access to the payload
2014-06-27 14:13 ` [PATCHv4 0/4] " Michael J Gruber
2014-06-27 14:13 ` [PATCHv4 1/4] gpg-interface: provide clear helper for struct signature_check Michael J Gruber
@ 2014-06-27 14:13 ` Michael J Gruber
2014-06-27 14:13 ` [PATCHv4 3/4] verify-commit: scriptable commit signature verification Michael J Gruber
` (3 subsequent siblings)
5 siblings, 0 replies; 75+ messages in thread
From: Michael J Gruber @ 2014-06-27 14:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In contrast to tag signatures, commit signatures are put into the
header, that is between the other header parts and commit messages.
Provide access to the commit content sans the signature, which is the
payload that is actually signed. Commit signature verification does the
parsing anyways, and callers may wish to act on or display the commit
object sans the signature.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
commit.c | 1 +
gpg-interface.c | 2 ++
gpg-interface.h | 1 +
3 files changed, 4 insertions(+)
diff --git a/commit.c b/commit.c
index fb7897c..acb74b5 100644
--- a/commit.c
+++ b/commit.c
@@ -1270,6 +1270,7 @@ void check_commit_signature(const struct commit* commit, struct signature_check
&gpg_output, &gpg_status);
if (status && !gpg_output.len)
goto out;
+ sigc->payload = strbuf_detach(&payload, NULL);
sigc->gpg_output = strbuf_detach(&gpg_output, NULL);
sigc->gpg_status = strbuf_detach(&gpg_status, NULL);
parse_gpg_output(sigc);
diff --git a/gpg-interface.c b/gpg-interface.c
index e71b59d..ff07012 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -9,10 +9,12 @@ static const char *gpg_program = "gpg";
void signature_check_clear(struct signature_check *sigc)
{
+ free(sigc->payload);
free(sigc->gpg_output);
free(sigc->gpg_status);
free(sigc->signer);
free(sigc->key);
+ sigc->payload = NULL;
sigc->gpg_output = NULL;
sigc->gpg_status = NULL;
sigc->signer = NULL;
diff --git a/gpg-interface.h b/gpg-interface.h
index 9f0784a..37c23da 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -2,6 +2,7 @@
#define GPG_INTERFACE_H
struct signature_check {
+ char *payload;
char *gpg_output;
char *gpg_status;
char result; /* 0 (not checked),
--
2.0.1.563.g162087b.dirty
^ permalink raw reply related [flat|nested] 75+ messages in thread
* [PATCHv4 3/4] verify-commit: scriptable commit signature verification
2014-06-27 14:13 ` [PATCHv4 0/4] " Michael J Gruber
2014-06-27 14:13 ` [PATCHv4 1/4] gpg-interface: provide clear helper for struct signature_check Michael J Gruber
2014-06-27 14:13 ` [PATCHv4 2/4] gpg-interface: provide access to the payload Michael J Gruber
@ 2014-06-27 14:13 ` Michael J Gruber
2014-06-27 14:13 ` [PATCHv4 4/4] t7510: test verify-commit Michael J Gruber
` (2 subsequent siblings)
5 siblings, 0 replies; 75+ messages in thread
From: Michael J Gruber @ 2014-06-27 14:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
Commit signatures can be verified using "git show -s --show-signature"
or the "%G?" pretty format and parsing the output, which is well suited
for user inspection, but not for scripting.
Provide a command "verify-commit" which is analogous to "verify-tag": It
returns 0 for good signatures and non-zero otherwise, has the gpg output
on stderr and (optionally) the commit object on stdout, sans the
signature, just like "verify-tag" does.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Documentation/git-verify-commit.txt | 28 +++++++++++
Makefile | 1 +
builtin.h | 1 +
builtin/verify-commit.c | 93 +++++++++++++++++++++++++++++++++++++
command-list.txt | 1 +
git.c | 1 +
6 files changed, 125 insertions(+)
create mode 100644 Documentation/git-verify-commit.txt
create mode 100644 builtin/verify-commit.c
diff --git a/Documentation/git-verify-commit.txt b/Documentation/git-verify-commit.txt
new file mode 100644
index 0000000..9413e28
--- /dev/null
+++ b/Documentation/git-verify-commit.txt
@@ -0,0 +1,28 @@
+git-verify-commit(1)
+====================
+
+NAME
+----
+git-verify-commit - Check the GPG signature of commits
+
+SYNOPSIS
+--------
+[verse]
+'git verify-commit' <commit>...
+
+DESCRIPTION
+-----------
+Validates the gpg signature created by 'git commit -S'.
+
+OPTIONS
+-------
+-v::
+--verbose::
+ Print the contents of the commit object before validating it.
+
+<commit>...::
+ SHA-1 identifiers of Git commit objects.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 07ea105..b92418d 100644
--- a/Makefile
+++ b/Makefile
@@ -999,6 +999,7 @@ BUILTIN_OBJS += builtin/update-ref.o
BUILTIN_OBJS += builtin/update-server-info.o
BUILTIN_OBJS += builtin/upload-archive.o
BUILTIN_OBJS += builtin/var.o
+BUILTIN_OBJS += builtin/verify-commit.o
BUILTIN_OBJS += builtin/verify-pack.o
BUILTIN_OBJS += builtin/verify-tag.o
BUILTIN_OBJS += builtin/write-tree.o
diff --git a/builtin.h b/builtin.h
index c47c110..5d91f31 100644
--- a/builtin.h
+++ b/builtin.h
@@ -128,6 +128,7 @@ extern int cmd_update_server_info(int argc, const char **argv, const char *prefi
extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);
extern int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix);
extern int cmd_var(int argc, const char **argv, const char *prefix);
+extern int cmd_verify_commit(int argc, const char **argv, const char *prefix);
extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);
extern int cmd_version(int argc, const char **argv, const char *prefix);
extern int cmd_whatchanged(int argc, const char **argv, const char *prefix);
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
new file mode 100644
index 0000000..b0f8504
--- /dev/null
+++ b/builtin/verify-commit.c
@@ -0,0 +1,93 @@
+/*
+ * Builtin "git commit-commit"
+ *
+ * Copyright (c) 2014 Michael J Gruber <git@drmicha.warpmail.net>
+ *
+ * Based on git-verify-tag
+ */
+#include "cache.h"
+#include "builtin.h"
+#include "commit.h"
+#include "run-command.h"
+#include <signal.h>
+#include "parse-options.h"
+#include "gpg-interface.h"
+
+static const char * const verify_commit_usage[] = {
+ N_("git verify-commit [-v|--verbose] <commit>..."),
+ NULL
+};
+
+static int run_gpg_verify(const unsigned char *sha1, const char *buf, unsigned long size, int verbose)
+{
+ struct signature_check signature_check;
+
+ memset(&signature_check, 0, sizeof(signature_check));
+
+ check_commit_signature(lookup_commit(sha1), &signature_check);
+
+ if (verbose && signature_check.payload)
+ fputs(signature_check.payload, stdout);
+
+ if (signature_check.gpg_output)
+ fputs(signature_check.gpg_output, stderr);
+
+ signature_check_clear(&signature_check);
+ return signature_check.result != 'G';
+}
+
+static int verify_commit(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("commit '%s' not found.", name);
+
+ buf = read_sha1_file(sha1, &type, &size);
+ if (!buf)
+ return error("%s: unable to read file.", name);
+ if (type != OBJ_COMMIT)
+ return error("%s: cannot verify a non-commit object of type %s.",
+ name, typename(type));
+
+ ret = run_gpg_verify(sha1, buf, size, verbose);
+
+ free(buf);
+ return ret;
+}
+
+static int git_verify_commit_config(const char *var, const char *value, void *cb)
+{
+ int status = git_gpg_config(var, value, cb);
+ if (status)
+ return status;
+ return git_default_config(var, value, cb);
+}
+
+int cmd_verify_commit(int argc, const char **argv, const char *prefix)
+{
+ int i = 1, verbose = 0, had_error = 0;
+ const struct option verify_commit_options[] = {
+ OPT__VERBOSE(&verbose, N_("print commit contents")),
+ OPT_END()
+ };
+
+ git_config(git_verify_commit_config, NULL);
+
+ argc = parse_options(argc, argv, prefix, verify_commit_options,
+ verify_commit_usage, PARSE_OPT_KEEP_ARGV0);
+ if (argc <= i)
+ usage_with_options(verify_commit_usage, verify_commit_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_commit(argv[i++], verbose))
+ had_error = 1;
+ return had_error;
+}
diff --git a/command-list.txt b/command-list.txt
index cf36c3d..a3ff0c9 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -132,6 +132,7 @@ git-update-server-info synchingrepositories
git-upload-archive synchelpers
git-upload-pack synchelpers
git-var plumbinginterrogators
+git-verify-commit ancillaryinterrogators
git-verify-pack plumbinginterrogators
git-verify-tag ancillaryinterrogators
gitweb ancillaryinterrogators
diff --git a/git.c b/git.c
index dd54f57..5b6c761 100644
--- a/git.c
+++ b/git.c
@@ -478,6 +478,7 @@ static struct cmd_struct commands[] = {
{ "upload-archive", cmd_upload_archive },
{ "upload-archive--writer", cmd_upload_archive_writer },
{ "var", cmd_var, RUN_SETUP_GENTLY },
+ { "verify-commit", cmd_verify_commit, RUN_SETUP },
{ "verify-pack", cmd_verify_pack },
{ "verify-tag", cmd_verify_tag, RUN_SETUP },
{ "version", cmd_version },
--
2.0.1.563.g162087b.dirty
^ permalink raw reply related [flat|nested] 75+ messages in thread
* [PATCHv4 4/4] t7510: test verify-commit
2014-06-27 14:13 ` [PATCHv4 0/4] " Michael J Gruber
` (2 preceding siblings ...)
2014-06-27 14:13 ` [PATCHv4 3/4] verify-commit: scriptable commit signature verification Michael J Gruber
@ 2014-06-27 14:13 ` Michael J Gruber
2014-06-27 19:32 ` Junio C Hamano
2014-06-27 19:07 ` [PATCHv4 0/4] verify-commit: verify commit signatures Junio C Hamano
2014-06-28 0:49 ` Jeff King
5 siblings, 1 reply; 75+ messages in thread
From: Michael J Gruber @ 2014-06-27 14:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
This mixes the "git verify-commit" tests in with the "git show
--show-signature" tests, to keep the tests more readable.
The tests already mix in the "call show" tests with the "verify" tests.
So in case of a test beakage, a '-v' run would be needed to reveal the
exact point of breakage anyway.
Additionally, test the actual output of "git verify-commit" and "git
show --show-signature" and compare to "git cat-file".
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
t/t7510-signed-commit.sh | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index e97477a..474dab3 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -48,10 +48,11 @@ test_expect_success GPG 'create signed commits' '
git tag eighth-signed-alt
'
-test_expect_success GPG 'show signatures' '
+test_expect_success GPG 'verify and show signatures' '
(
for commit in initial second merge fourth-signed fifth-signed sixth-signed seventh-signed
do
+ git verify-commit $commit &&
git show --pretty=short --show-signature $commit >actual &&
grep "Good signature from" actual &&
! grep "BAD signature from" actual &&
@@ -61,6 +62,7 @@ test_expect_success GPG 'show signatures' '
(
for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
do
+ test_must_fail git verify-commit $commit &&
git show --pretty=short --show-signature $commit >actual &&
! grep "Good signature from" actual &&
! grep "BAD signature from" actual &&
@@ -79,11 +81,25 @@ test_expect_success GPG 'show signatures' '
)
'
+test_expect_success GPG 'show signed commit with signature' '
+ git show -s initial >commit &&
+ git show -s --show-signature initial >show &&
+ git verify-commit -v initial >verify.1 2>verify.2 &&
+ git cat-file commit initial >cat &&
+ grep -v "gpg: " show >show.commit &&
+ grep "gpg: " show >show.gpg &&
+ grep -v "^ " cat | grep -v "^gpgsig " >cat.commit &&
+ test_cmp show.commit commit &&
+ test_cmp show.gpg verify.2 &&
+ test_cmp cat.commit verify.1
+'
+
test_expect_success GPG 'detect fudged signature' '
git cat-file commit seventh-signed >raw &&
sed -e "s/seventh/7th forged/" raw >forged1 &&
git hash-object -w -t commit forged1 >forged1.commit &&
+ ! git verify-commit $(cat forged1.commit) &&
git show --pretty=short --show-signature $(cat forged1.commit) >actual1 &&
grep "BAD signature from" actual1 &&
! grep "Good signature from" actual1
@@ -94,6 +110,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
cat raw >forged2 &&
echo Qwik | tr "Q" "\000" >>forged2 &&
git hash-object -w -t commit forged2 >forged2.commit &&
+ ! git verify-commit $(cat forged2.commit) &&
git show --pretty=short --show-signature $(cat forged2.commit) >actual2 &&
grep "BAD signature from" actual2 &&
! grep "Good signature from" actual2
@@ -102,6 +119,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
test_expect_success GPG 'amending already signed commit' '
git checkout fourth-signed^0 &&
git commit --amend -S --no-edit &&
+ git verify-commit HEAD &&
git show -s --show-signature HEAD >actual &&
grep "Good signature from" actual &&
! grep "BAD signature from" actual
--
2.0.1.563.g162087b.dirty
^ permalink raw reply related [flat|nested] 75+ messages in thread
* Re: [PATCHv4 4/4] t7510: test verify-commit
2014-06-27 14:13 ` [PATCHv4 4/4] t7510: test verify-commit Michael J Gruber
@ 2014-06-27 19:32 ` Junio C Hamano
2014-06-27 20:26 ` Michael J Gruber
0 siblings, 1 reply; 75+ messages in thread
From: Junio C Hamano @ 2014-06-27 19:32 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Jeff King
Michael J Gruber <git@drmicha.warpmail.net> writes:
> sed -e "s/seventh/7th forged/" raw >forged1 &&
> git hash-object -w -t commit forged1 >forged1.commit &&
> + ! git verify-commit $(cat forged1.commit) &&
test_must_fail git verify-commit ... &&
> git show --pretty=short --show-signature $(cat forged1.commit) >actual1 &&
> grep "BAD signature from" actual1 &&
> ! grep "Good signature from" actual1
> @@ -94,6 +110,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
> cat raw >forged2 &&
> echo Qwik | tr "Q" "\000" >>forged2 &&
> git hash-object -w -t commit forged2 >forged2.commit &&
> + ! git verify-commit $(cat forged2.commit) &&
test_must_fail git verify-commit ... &&
> git show --pretty=short --show-signature $(cat forged2.commit) >actual2 &&
> grep "BAD signature from" actual2 &&
> ! grep "Good signature from" actual2
> @@ -102,6 +119,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
> test_expect_success GPG 'amending already signed commit' '
> git checkout fourth-signed^0 &&
> git commit --amend -S --no-edit &&
> + git verify-commit HEAD &&
> git show -s --show-signature HEAD >actual &&
> grep "Good signature from" actual &&
> ! grep "BAD signature from" actual
Most of the tests, unlike "git show --show-signature" tests, do not
seem to check the output from the command. Is it because its
primary interface to scripts is its exit status [*1*]?
[Footnote]
*1* "Yes" is totally an acceptable answer and a justification for
not checking the output in many of these tests.
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv4 4/4] t7510: test verify-commit
2014-06-27 19:32 ` Junio C Hamano
@ 2014-06-27 20:26 ` Michael J Gruber
0 siblings, 0 replies; 75+ messages in thread
From: Michael J Gruber @ 2014-06-27 20:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
On 27. Juni 2014 21:32:30 MESZ, Junio C Hamano <gitster@pobox.com> wrote:
>Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> sed -e "s/seventh/7th forged/" raw >forged1 &&
>> git hash-object -w -t commit forged1 >forged1.commit &&
>> + ! git verify-commit $(cat forged1.commit) &&
>
>test_must_fail git verify-commit ... &&
>
>> git show --pretty=short --show-signature $(cat forged1.commit)
>>actual1 &&
>> grep "BAD signature from" actual1 &&
>> ! grep "Good signature from" actual1
>> @@ -94,6 +110,7 @@ test_expect_success GPG 'detect fudged signature
>with NUL' '
>> cat raw >forged2 &&
>> echo Qwik | tr "Q" "\000" >>forged2 &&
>> git hash-object -w -t commit forged2 >forged2.commit &&
>> + ! git verify-commit $(cat forged2.commit) &&
>
>test_must_fail git verify-commit ... &&
Sorry for missing or mis-rebasing these. I meant to cover them all.
>
>> git show --pretty=short --show-signature $(cat forged2.commit)
>>actual2 &&
>> grep "BAD signature from" actual2 &&
>> ! grep "Good signature from" actual2
>> @@ -102,6 +119,7 @@ test_expect_success GPG 'detect fudged signature
>with NUL' '
>> test_expect_success GPG 'amending already signed commit' '
>> git checkout fourth-signed^0 &&
>> git commit --amend -S --no-edit &&
>> + git verify-commit HEAD &&
>> git show -s --show-signature HEAD >actual &&
>> grep "Good signature from" actual &&
>> ! grep "BAD signature from" actual
>
>Most of the tests, unlike "git show --show-signature" tests, do not
>seem to check the output from the command. Is it because its
>primary interface to scripts is its exit status [*1*]?
>
>[Footnote]
>
>*1* "Yes" is totally an acceptable answer and a justification for
>not checking the output in many of these tests.
Yes, the idea was to check the exit status in all cases (the loopy subtests) and the textual output in a few exemplary ones. I didn't want to bloat the test unnecessarily.
Michael
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv4 0/4] verify-commit: verify commit signatures
2014-06-27 14:13 ` [PATCHv4 0/4] " Michael J Gruber
` (3 preceding siblings ...)
2014-06-27 14:13 ` [PATCHv4 4/4] t7510: test verify-commit Michael J Gruber
@ 2014-06-27 19:07 ` Junio C Hamano
2014-06-28 0:48 ` Jeff King
2014-06-28 0:49 ` Jeff King
5 siblings, 1 reply; 75+ messages in thread
From: Junio C Hamano @ 2014-06-27 19:07 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Jeff King
Michael J Gruber <git@drmicha.warpmail.net> writes:
> This is v3 rebased on current next (the %G works by Jeff & Junio).
>
> Open questions:
>
> - Should one of git verify-{commit,tag} learn how to verify mergetags?
> (Probably no, it differs from both other cases.)
If we were to teach one of them, "verify-commit" as part of
"verifying what is recorded in the commit object", would be the
logical place to do so.
It is OK to implement only verification of signatures on commit
objects themselves, but we would need a plan for handling other
kinds of verifications later, so that we can give a stable output to
scripts.
If we decide to signal successful verification of the signature on
the commit itself one way in this implementation, that should
reliably be the way to do so even if we later add verification of
other aspects on the commit object (e.g. mergetags it carries). If
running "verify-commit $commit" and checking the zero-ness of its
exit status is the way, that should not change if later versions of
Git learns to verify mergetags as well; even if the given $commit
carries a mergetag that does not verify, as long as the signature in
the commit itself is valid, the script should continue to receive
"success" from the command.
> - Should we do this now or go for generic "git verify" right away?
I do not think we are ready to do "git verify" yet. We first need
to design how "verify-commit" should communicate failure/success
combinations of verification of a commit that has a signature on
itself and a mergetag on one of its parents, and of a commit that
has zero or one signature on itself and two or more mergetags on its
parents. Do we fail unless all of them are found to be valid? Do
we use bits in exit status? Output to the standard output, one line
per signature verified?
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCHv4 0/4] verify-commit: verify commit signatures
2014-06-27 14:13 ` [PATCHv4 0/4] " Michael J Gruber
` (4 preceding siblings ...)
2014-06-27 19:07 ` [PATCHv4 0/4] verify-commit: verify commit signatures Junio C Hamano
@ 2014-06-28 0:49 ` Jeff King
5 siblings, 0 replies; 75+ messages in thread
From: Jeff King @ 2014-06-28 0:49 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
On Fri, Jun 27, 2014 at 04:13:22PM +0200, Michael J Gruber wrote:
> This is v3 rebased on current next (the %G works by Jeff & Junio).
Aside from the minor test issues Junio pointed out, I think this version
looks OK.
> For a general command which allows different verification policies,
> I'm still wondering whether we may need hooks which receive all
> the relevant information in the environment. Otherwise we'll have a ton of
> options such as --match-committer-uid, --verify--AllParentsHaveMergeTags,
> --verify--All-ParentsAreSignedCommits, --peel-to-commit, --merge-commit-only, ...
I'd guess these would be project policies that could go in config. But I
do not think your series needs to be concerned with that yet.
-Peff
^ permalink raw reply [flat|nested] 75+ messages in thread