Sebastian Götte <jaseg@xxxxxxxxxxxxxxxxxxx> writes:
> git merge/pull:
> When --verify-signatures is specified on the command-line of git-merge
> or git-pull, check whether the commits being merged have good gpg
> signatures and abort the merge in case they do not. This allows e.g.
> auto-deployment from untrusted repo hosts.
>
> pretty printing:
> Tell about an "untrusted good signature" in addition to the previous
> "good signature" and "bad signature". In case of a missing signature,
> expand the pretty format string "%G?" to "N" in since this eases the
> wirting of anything parsing git-log output.
>
> Signed-off-by: Sebastian Götte <jaseg@xxxxxxxxxxxxxxxxxxxxxxxx>
> ---
> I moved the commit signature verification code from pretty.c to commit.c
> because it is used from pretty.c and builtin/merge.c. I include that pretty
> printing change here because I needed to add the good/untrusted check for the
> merge --verify-signatures option to really make sense.
>
> Those new %G? options are implicitly tested by t7612-merge-verify-signatures.sh
> because %G? is just replaced by the passed-through output of the commit
> verification function.
While I think the new --verify-signature option may be a good
addition, I wonder if you can split this patch down a bit for easier
review and validation.
Perhaps this needs to be done in at least three steps:
(1) first move the code without changing anything (most
importantly, do not add 'U' or 'N' at this step); then
(2) teach 'merge' and 'pull' to understand the new option, and
finally;
(3) introduce 'U' and 'N'.
The existing users of '%G?' placeholders are not expecting to see
'N' but turning it into an empty string, so if the third step turns
out to be problematic to these users, we can discard the third step
and still benefit from the first two, for example.
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index 105f18a..7297b1b 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -131,7 +131,7 @@ The placeholders are:
> - '%B': raw body (unwrapped subject and body)
> - '%N': commit notes
> - '%GG': raw verification message from GPG for a signed commit
> -- '%G?': show either "G" for Good or "B" for Bad for a signed commit
> +- '%G?': show "G" for a Good signature, "B" for a Bad signature, "U" for a good, untrusted signature and "N" for no signature
Even though this is a source that is turned into html and manpages,
people do read these in the original text format (that is the whole
point of using AsciiDoc as the source format), so please see if you
can avoid this overly long line.
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 7c8922c..37ece3d 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -49,7 +49,7 @@ static const char * const builtin_merge_usage[] = {
> static int show_diffstat = 1, shortlog_len = -1, squash;
> static int option_commit = 1, allow_fast_forward = 1;
> static int fast_forward_only, option_edit = -1;
> -static int allow_trivial = 1, have_message;
> +static int allow_trivial = 1, have_message, verify_signatures = 0;
Avoid initializing static variables to 0, and instead let BSS take
care of them.
> @@ -199,6 +199,8 @@ static struct option builtin_merge_options[] = {
> OPT_BOOLEAN(0, "ff-only", &fast_forward_only,
> N_("abort if fast-forward is not possible")),
> OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
> + OPT_BOOLEAN(0, "verify-signatures", &verify_signatures,
> + N_("Verify that the named commit has a valid GPG signature")),
> OPT_CALLBACK('s', "strategy", &use_strategies, N_("strategy"),
> N_("merge strategy to use"), option_parse_strategy),
> OPT_CALLBACK('X', "strategy-option", &xopts, N_("option=value"),
> @@ -1233,6 +1235,35 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> usage_with_options(builtin_merge_usage,
> builtin_merge_options);
>
> + if (verify_signatures) {
> + //Verify the commit signatures
No // C99/C++ comments.
The rest of the patch not reviewed; expecting a split reroll.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html