Re: [PATCH] merge/pull: verify GPG signatures of commits being merged

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 22, 2013, 23:02 UTC
Message-ID: <7v4ng3597s.fsf@alter.siamese.dyndns.org>
In-Reply-To: <514CD26C.2070702@physik.tu-berlin.de>

Sebastian Götte <jaseg@physik.tu-berlin.de> writes:

Show 22 quoted lines

> 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@physik-pool.tu-berlin.de>
> ---
> 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.

Show 10 quoted lines

> 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.

Show 10 quoted lines

> 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.

Show 15 quoted lines

> @@ -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.