Re: [PATCH v4 5/5] fast-export, fast-import: add support for signed-commits

From: Junio C Hamano <gitster@pobox.com>
Date: May 3, 2021, 05:09 UTC
Message-ID: <xmqq1rao9zev.fsf@gitster.g>
In-Reply-To: <20210430232537.1131641-6-lukeshu@lukeshu.com>

Luke Shumaker <lukeshu@lukeshu.com> writes:

Show 16 quoted lines

> From: Luke Shumaker <lukeshu@datawire.io>
>
> fast-export has a --signed-tags= option that controls how to handle tag
> signatures.  However, there is no equivalent for commit signatures; it
> just silently strips the signature out of the commit (analogously to
> --signed-tags=strip).
>
> While signatures are generally problematic for fast-export/fast-import
> (because hashes are likely to change), if they're going to support tag
> signatures, there's no reason to not also support commit signatures.
>
> So, implement a --signed-commits= option that mirrors the --signed-tags=
> option.
>
> On the fast-export side, try to be as much like signed-tags as possible,
> in both implementation and in user-interface.  This will changes the

s/changes/change/;

Show 6 quoted lines

> default behavior to '--signed-commits=abort' from what is now
> '--signed-commits=strip'.  In order to provide an escape hatch for users
> of third-party tools that call fast-export and do not yet know of the
> --signed-commits= option, add an environment variable
> 'FAST_EXPORT_SIGNED_COMMITS_NOABORT=1' that changes the default to
> '--signed-commits=warn-strip'.

Nicely explained.

Show 25 quoted lines

> +static const char *find_commit_multiline_header(const char *msg,
> +						const char *key,
> +						const char **end)
> +{
> +	static struct strbuf val = STRBUF_INIT;
> +	const char *bol, *eol;
> +	size_t len;
> +
> +	strbuf_reset(&val);
> +
> +	bol = find_commit_header(msg, key, &len);
> +	if (!bol)
> +		return NULL;
> +	eol = bol + len;
> +	strbuf_add(&val, bol, len);
> +
> +	while (eol[0] == '\n' && eol[1] == ' ') {
> +		bol = eol + 2;
> +		eol = strchrnul(bol, '\n');
> +		strbuf_addch(&val, '\n');
> +		strbuf_add(&val, bol, eol - bol);
> +	}
> +
> +	*end = eol;
> +	return val.buf;

It is not exactly wrong per se, but using non-static (on stack) strbuf would make it easier to follow. You can then lose the strbuf_reset() upfront, and then this will call strbuf_detach().

Show 24 quoted lines

> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
> index 892737439b..cd51c78418 100755
> --- a/t/t9350-fast-export.sh
> +++ b/t/t9350-fast-export.sh
> @@ -8,6 +8,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>  
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY/lib-gpg.sh"
>  
>  test_expect_success 'setup' '
>  
> @@ -284,9 +285,94 @@ test_expect_success 'signed-tags=warn-strip' '
>  	test -s err
>  '
>  
> +test_expect_success GPG 'set up signed commit' '
> +
> +	# Generate a commit with both "gpgsig" and "encoding" set, so
> +	# that we can test that fast-import gets the ordering correct
> +	# between the two.
> +	test_config i18n.commitEncoding ISO-8859-1 &&
> +	git checkout -f -b commit-signing main &&
> +	echo Sign your name > file-sign &&

Style. >file-sign (lose SP between the redirection operator and its operand).

Show 9 quoted lines

> +	git add file-sign &&
> +	git commit -S -m "signed commit" &&
> +	COMMIT_SIGNING=$(git rev-parse --verify commit-signing)
> +
> +'
> +
> +test_expect_success GPG 'signed-commits default' '
> +
> +	unset FAST_EXPORT_SIGNED_COMMITS_NOABORT &&

sane_unset would be safer here.

Show 10 quoted lines

> +	test_must_fail git fast-export --reencode=no commit-signing &&
> +
> +	FAST_EXPORT_SIGNED_COMMITS_NOABORT=1 git fast-export --reencode=no commit-signing >output 2>err &&
> +	! grep ^gpgsig output &&
> +	grep "^encoding ISO-8859-1" output &&
> +	test -s err &&
> +	sed "s/commit-signing/commit-strip-signing/" output |
> +		(cd new &&
> +		 git fast-import &&
> +		 test $COMMIT_SIGNING != $(git rev-parse --verify refs/heads/commit-strip-signing))

Let's not force readers to match nested parentheses visually (applies to multiple places in this patch):

	sed "s/commit-signing/commit-strip-signing/" output | (
		cd new &&
		git fast-import &&
		STRIPPED=$(git rev-parse --verify refs/heads/commit-strip-signing) &&
		test $COMMIT_SIGNING != $STRIPPED
	)

>  test_expect_success 'setup submodule' '
>  
>  	git checkout -f main &&
> +	{ git update-ref -d refs/heads/commit-signing || true; } &&

	test_might_fail git update-ref -d refs/heads/commit-signing &&