Re: [PATCH 2/2] fast-import: add '--signed-commits=<mode>' option

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> @@ -2785,6 +2787,23 @@ static void store_signature(struct signature_data *stored_sig,
>  	}
>  }
>  
> +/* Process signatures (up to 2: one "sha1" and one "sha256") */
> +static void import_signature(struct signature_data *sig_sha1,
> +			     struct signature_data *sig_sha256,
> +			     const char *v)
> +{
> +	struct signature_data sig = { NULL, NULL, STRBUF_INIT };
> +
> +	parse_one_signature(&sig, v);
> +
> +	if (!strcmp(sig.hash_algo, "sha1"))
> +		store_signature(sig_sha1, &sig, "SHA-1");
> +	else if (!strcmp(sig.hash_algo, "sha256"))
> +		store_signature(sig_sha256, &sig, "SHA-256");
> +	else
> +		BUG("parse_one_signature() returned unknown hash algo");
> +}
> +

THis is a new function; I am not sure if the division of labor
between this one and its caller is done right.  See below.

> @@ -2817,19 +2836,28 @@ static void parse_new_commit(const char *arg)
>  	if (!committer)
>  		die("Expected committer but didn't get one");
>  
> -	/* Process signatures (up to 2: one "sha1" and one "sha256") */
>  	while (skip_prefix(command_buf.buf, "gpgsig ", &v)) {
> -		struct signature_data sig = { NULL, NULL, STRBUF_INIT };
> -
> -		parse_one_signature(&sig, v);
> -
> -		if (!strcmp(sig.hash_algo, "sha1"))
> -			store_signature(&sig_sha1, &sig, "SHA-1");
> -		else if (!strcmp(sig.hash_algo, "sha256"))
> -			store_signature(&sig_sha256, &sig, "SHA-256");
> -		else
> -			BUG("parse_one_signature() returned unknown hash algo");
> -
> +		struct strbuf data = STRBUF_INIT;
> +		switch (signed_commit_mode) {
> +		case SIGN_ABORT:
> +			die("encountered signed commit; use "
> +			    "--signed-commits=<mode> to handle it");
> +		case SIGN_WARN_VERBATIM:
> +			warning("importing a commit signature");
> +			/* fallthru */
> +		case SIGN_VERBATIM:
> +			import_signature(&sig_sha1, &sig_sha256, v);
> +			break;
> +		case SIGN_WARN_STRIP:
> +			warning("stripping a commit signature");
> +			/* fallthru */
> +		case SIGN_STRIP:
> +			/* Read signature data and discard it */
> +			read_next_command();
> +			parse_data(&data, 0, NULL);
> +			strbuf_release(&data);
> +			break;
> +		}
>  		read_next_command();
>  	}

I am not sure if this change had to be this way.  The old code
always called parse_one_signature(), which was responsible for
checking the signature format and then calling read_next_command()
and parse_data(), so no matter what happened afterwards, we know we
are consuming the data stream regardless of the conditional execution
that happens here.

The new code calls import_signature() or an inlined sequence of
read_next_command() plus parse_data(), essentially making each case
arm in the switch() statement responsible individually for consuming
the incoming data.  When somebody adds a new case there to specify a
different way to handle signatures, they have to make sure that they
do not forget calling read_next_command() and parse_data() themselves.

Even though I can see, after some code inspection, that no branches
in the current code forgets to advance the incoming data stream to
leave us out of sync right now, this change feels like a bit of code
ergonomics regression to me.  Was it so important that we pass a
broken signature without inspecting in STRIP mode?  I am guessing
that is the reason why the new code tries hard to avoid calling the
parse_one_signature() function in these case arms.

An aside.  I think the warning message about importing should have a
word "verbatim" in it, e.g.

	warning("importing a commit signature verbatim");

Thanks.