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

Sebastian Götte [off-list ref] writes:

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.

Signed-off-by: Sebastian Götte <redacted>
---
...
+	if (verify_signatures) {
+		/* Verify the commit signatures */
+		for (p = remoteheads; p; p = p->next) {
+			struct commit *commit = p->item;
+			struct signature signature;
+			signature.check_result = 0;

Even though you may happen to know all other fields are output only,
it is a good practice to clear it with

			memset(&signature, 0, sizeof(signature);

By the way, I think this variable and type should be called more
like "signature_check" or something with "check" in its name, not
"signature".  After all it is _not_ a signature itself.

+			check_commit_signature(commit, &signature);
+
+			char hex[41];

builtin/merge.c: In function 'cmd_merge':
builtin/merge.c:1247: error: ISO C90 forbids mixed declarations and code

+
+test_expect_success GPG 'merge unsigned commit with verification' '
+	test_must_fail git merge --ff-only --verify-signatures side-unsigned 2> mergeerror &&

No SP between redirection and the target filename.

+	grep "does not have a GPG signature" mergeerror

Do we plan to make this message localized?  If so I think you would
need to do this with test_i18ngrep.

+'
+
+test_expect_success GPG 'merge commit with bad signature with verification' '
+	test_must_fail git merge --ff-only --verify-signatures $(cat forged.commit) 2> mergeerror &&
+	grep "has a bad GPG signature" mergeerror
+'
+
+test_expect_success GPG 'merge signed commit with verification' '
+	git merge -v --ff-only --verify-signatures side-signed > mergeoutput &&
+	grep "has a good GPG signature" mergeoutput
+'

Hmph.

So the caller needs to check both the standard output and the
standard error to get the whole picture?  Is there a reason why we
are not sending everything to the standard output consistently?

+test_expect_success GPG 'merge commit with bad signature without verification' '
+	git merge $(cat forged.commit)
+'

Good to see that negative case where the new feature should _not_
trigger is properly tested.

+
+test_done