[PATCH v2 0/3] fast-export, fast-import: implement signed-commits

[PATCH v2 0/3] fast-export, fast-import: implement signed-commits

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

fast-export has an existing --signed-tags= flag 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).

So implement a --signed-commits= flag in fast-export, and implement
the receiving side of it in fast-import.

I believe that this revision addresses all of the feedback so far,
with the exception that I have not implemented Elijah's suggestion to
implement a flag on fast-import to validate signatures.  While I agree
that this would be a useful feature, I consider it to be beyond the
scope of this work.

This passes all of the GitHub actions CI checks, and passes all but
one of the Travis-CI checks; the failing Travis-CI check seems to be
an unrelated 404 from `apt-get`.
https://github.com/LukeShu/git/runs/2405123468

Luke Shumaker (3):
  git-fast-import.txt: add missing LF in the BNF
    v2: no changes
  fast-export: rename --signed-tags='warn' to 'warn-verbatim'
    v2:
     - Reword commit message, based on feedback from Taylor.
     - Fix copy-pasto in the test, noticed by Taylor.
     - Add a comment to the tests.
     - Fix whitespace in the tests.
  fast-export, fast-import: implement signed-commits
    v2:
     - Remove erroneous remark about ordering from the commit message.
     - Adjust the stream syntax to include the hash algorithm, as
       suggested by brian.
     - Add support for sha256 (based on lots of useful information from
       brian).  It does not support multiply-signed commits.
     - Shorten the documentation, based on feedback from Taylor.
     - Add comments, based on feedback from Taylor.
     - Change the default from `--signed-commits=strip` to
       `--signed-commits=warn-strip`.  This shouldn't break anyone, and
       means that users get useful feedback by default.

 Documentation/git-fast-export.txt |  11 ++-
 Documentation/git-fast-import.txt |  20 ++++-
 builtin/fast-export.c             | 123 ++++++++++++++++++++++++++----
 builtin/fast-import.c             |  23 ++++++
 t/t9350-fast-export.sh            |  88 +++++++++++++++++++++
 5 files changed, 245 insertions(+), 20 deletions(-)

-- 
2.31.1

Happy hacking,
~ Luke Shumaker

[PATCH v2 1/3] git-fast-import.txt: add missing LF in the BNF

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---
 Documentation/git-fast-import.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 39cfa05b28..458af0a2d6 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -437,7 +437,7 @@ change to the project.
 	original-oid?
 	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
 	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
-	('encoding' SP <encoding>)?
+	('encoding' SP <encoding> LF)?
 	data
 	('from' SP <commit-ish> LF)?
 	('merge' SP <commit-ish> LF)*
-- 
2.31.1

[PATCH v2 2/3] fast-export: rename --signed-tags='warn' to 'warn-verbatim'

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

The --signed-tags= option takes one of five arguments specifying how to
handle signed tags during export.  Among these arguments, 'strip' is to
'warn-strip' as 'verbatim' is to 'warn' (the unmentioned argument is
'abort', which stops the fast-export process entirely).  That is,
signatures are either stripped or copied verbatim while exporting, with
or without a warning.

Match the pattern and rename 'warn' to 'warn-verbatim' to make it clear
that it instructs fast-export to copy signatures verbatim.

To maintain backwards compatibility, 'warn' is still recognized as
an undocumented alias.

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---
v2:
 - Reword commit message based on feedback from Taylor.
 - Fix copy-pasto in the test, noticed by Taylor.
 - Add a comment to the tests.
 - Fix whitespace in the tests.

 Documentation/git-fast-export.txt |  6 +++---
 builtin/fast-export.c             |  2 +-
 t/t9350-fast-export.sh            | 18 ++++++++++++++++++
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index 1978dbdc6a..d4a2bfe037 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -27,7 +27,7 @@ OPTIONS
 	Insert 'progress' statements every <n> objects, to be shown by
 	'git fast-import' during import.
 
---signed-tags=(verbatim|warn|warn-strip|strip|abort)::
+--signed-tags=(verbatim|warn-verbatim|warn-strip|strip|abort)::
 	Specify how to handle signed tags.  Since any transformation
 	after the export can change the tag names (which can also happen
 	when excluding revisions) the signatures will not match.
@@ -36,8 +36,8 @@ When asking to 'abort' (which is the default), this program will die
 when encountering a signed tag.  With 'strip', the tags will silently
 be made unsigned, with 'warn-strip' they will be made unsigned but a
 warning will be displayed, with 'verbatim', they will be silently
-exported and with 'warn', they will be exported, but you will see a
-warning.
+exported and with 'warn-verbatim', they will be exported, but you will
+see a warning.
 
 --tag-of-filtered-object=(abort|drop|rewrite)::
 	Specify how to handle tags whose tagged object is filtered out.
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 85a76e0ef8..d121dd2ee6 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -55,7 +55,7 @@ static int parse_opt_signed_tag_mode(const struct option *opt,
 		signed_tag_mode = SIGNED_TAG_ABORT;
 	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
 		signed_tag_mode = VERBATIM;
-	else if (!strcmp(arg, "warn"))
+	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
 		signed_tag_mode = WARN;
 	else if (!strcmp(arg, "warn-strip"))
 		signed_tag_mode = WARN_STRIP;
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 409b48e244..36b84eff36 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -253,6 +253,24 @@ test_expect_success 'signed-tags=verbatim' '
 
 '
 
+test_expect_success 'signed-tags=warn-verbatim' '
+
+	git fast-export --signed-tags=warn-verbatim sign-your-name >output 2>err &&
+	grep PGP output &&
+	test -s err
+
+'
+
+# 'warn' is an undocumented alias for 'warn-verbatim', for backward
+# compatibility; test that it keeps working.
+test_expect_success 'signed-tags=warn' '
+
+	git fast-export --signed-tags=warn sign-your-name >output 2>err &&
+	grep PGP output &&
+	test -s err
+
+'
+
 test_expect_success 'signed-tags=strip' '
 
 	git fast-export --signed-tags=strip sign-your-name > output &&
-- 
2.31.1

[PATCH v2 3/3] fast-export, fast-import: implement signed-commits

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

fast-export has an existing --signed-tags= flag 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 signed-commits.

On the fast-export side, try to be as much like signed-tags as possible,
in both implementation and in user-interface; with the exception that
the default should be `--signed-commits=warn-strip` (compared to the
default `--signed-tags=abort`), in order to avoid breaking the
historical behavior (it will now print a warning while doing that
behavior, though).

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---
v2:
 - Remove erroneous remark about ordering from the commit message.
 - Adjust the stream syntax to include the hash algorithm, as
   suggested by brian.
 - Add support for sha256 (based on lots of useful information from
   brian).  It does not support multiply-signed commits.
 - Shorten the documentation, based on feedback from Taylor.
 - Add comments, based on feedback from Taylor.
 - Change the default from `--signed-commits=strip` to
   `--signed-commits=warn-strip`.  This shouldn't break anyone, and
   means that users get useful feedback by default.

 Documentation/git-fast-export.txt |   5 ++
 Documentation/git-fast-import.txt |  18 +++++
 builtin/fast-export.c             | 121 ++++++++++++++++++++++++++----
 builtin/fast-import.c             |  23 ++++++
 t/t9350-fast-export.sh            |  70 +++++++++++++++++
 5 files changed, 222 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index d4a2bfe037..2ff1719f8b 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -39,6 +39,11 @@ warning will be displayed, with 'verbatim', they will be silently
 exported and with 'warn-verbatim', they will be exported, but you will
 see a warning.
 
+--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::
+	Specify how to handle signed commits.  Behaves exactly as
+	--signed-tags (but for commits), except that the default is
+	'warn-strip' rather than 'abort'.
+
 --tag-of-filtered-object=(abort|drop|rewrite)::
 	Specify how to handle tags whose tagged object is filtered out.
 	Since revisions and files to export can be limited by path,
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 458af0a2d6..4955c94305 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -431,12 +431,21 @@ and control the current import process.  More detailed discussion
 Create or update a branch with a new commit, recording one logical
 change to the project.
 
+////
+Yes, it's intentional that the 'gpgsig' line doesn't have a trailing
+`LF`; the the definition of `data` has a byte-count prefix, so it
+doesn't need an `LF` to act as a terminator (and `data` also already
+includes an optional trailing `LF?` just in case you want to include
+one).
+////
+
 ....
 	'commit' SP <ref> LF
 	mark?
 	original-oid?
 	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
 	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
+	('gpgsig' SP <alg> LF data)?
 	('encoding' SP <encoding> LF)?
 	data
 	('from' SP <commit-ish> LF)?
@@ -505,6 +514,15 @@ that was selected by the --date-format=<fmt> command-line option.
 See ``Date Formats'' above for the set of supported formats, and
 their syntax.
 
+`gpgsig`
+^^^^^^^^
+
+The optional `gpgsig` command is used to include a PGP/GPG signature
+that signs the commit data.
+
+Here <alg> specifies which hashing algorithm is used for this
+signature, either `sha1` or `sha256`.
+
 `encoding`
 ^^^^^^^^^^
 The optional `encoding` command indicates the encoding of the commit
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index d121dd2ee6..2b1101d104 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -30,8 +30,11 @@ static const char *fast_export_usage[] = {
 	NULL
 };
 
+enum sign_mode { SIGN_ABORT, SIGN_VERBATIM, SIGN_STRIP, SIGN_VERBATIM_WARN, SIGN_STRIP_WARN };
+
 static int progress;
-static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
+static enum sign_mode signed_tag_mode = SIGN_ABORT;
+static enum sign_mode signed_commit_mode = SIGN_STRIP_WARN;
 static enum { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
 static enum { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
 static int fake_missing_tagger;
@@ -48,21 +51,24 @@ static int anonymize;
 static struct hashmap anonymized_seeds;
 static struct revision_sources revision_sources;
 
-static int parse_opt_signed_tag_mode(const struct option *opt,
+static int parse_opt_sign_mode(const struct option *opt,
 				     const char *arg, int unset)
 {
-	if (unset || !strcmp(arg, "abort"))
-		signed_tag_mode = SIGNED_TAG_ABORT;
+	enum sign_mode *valptr = opt->value;
+	if (unset)
+		return 0;
+	else if (!strcmp(arg, "abort"))
+		*valptr = SIGN_ABORT;
 	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
-		signed_tag_mode = VERBATIM;
+		*valptr = SIGN_VERBATIM;
 	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
-		signed_tag_mode = WARN;
+		*valptr = SIGN_VERBATIM_WARN;
 	else if (!strcmp(arg, "warn-strip"))
-		signed_tag_mode = WARN_STRIP;
+		*valptr = SIGN_STRIP_WARN;
 	else if (!strcmp(arg, "strip"))
-		signed_tag_mode = STRIP;
+		*valptr = SIGN_STRIP;
 	else
-		return error("Unknown signed-tags mode: %s", arg);
+		return error("Unknown %s mode: %s", opt->long_name, arg);
 	return 0;
 }
 
@@ -499,6 +505,60 @@ static void show_filemodify(struct diff_queue_struct *q,
 	}
 }
 
+static const char *find_signature(const char *begin, const char *end, const char *key)
+{
+	static struct strbuf needle = STRBUF_INIT;
+	char *bod, *eod, *eol;
+
+	strbuf_reset(&needle);
+	strbuf_addch(&needle, '\n');
+	strbuf_addstr(&needle, key);
+	strbuf_addch(&needle, ' ');
+
+	bod = memmem(begin, end ? end - begin : strlen(begin),
+		     needle.buf, needle.len);
+	if (!bod)
+		return NULL;
+	bod += needle.len;
+
+	/*
+	 * In the commit object, multi-line header values are stored
+	 * by prefixing continuation lines begin with a space.  So
+	 * within the commit object, it looks like
+	 *
+	 *     "gpgsig -----BEGIN PGP SIGNATURE-----\n"
+	 *     " Version: GnuPG v1.4.5 (GNU/Linux)\n"
+	 *     " \n"
+	 *     " base64_pem_here\n"
+	 *     " -----END PGP SIGNATURE-----\n"
+	 *
+	 * So we need to look for the first '\n' that *isn't* followed
+	 * by a ' ' (or the first '\0', if no such '\n' exists).
+	 */
+	eod = strchrnul(bod, '\n');
+	while (eod[0] == '\n' && eod[1] == ' ') {
+		eod = strchrnul(eod+1, '\n');
+	}
+	*eod = '\0';
+
+	/*
+	 * We now have the value as it's stored in the commit object.
+	 * However, we want the raw value; we want to return
+	 *
+	 *     "-----BEGIN PGP SIGNATURE-----\n"
+	 *     "Version: GnuPG v1.4.5 (GNU/Linux)\n"
+	 *     "\n"
+	 *     "base64_pem_here\n"
+	 *     "-----END PGP SIGNATURE-----\n"
+	 *
+	 * So now we need to strip out all of those extra spaces.
+	 */
+	while ((eol = strstr(bod, "\n ")))
+		memmove(eol+1, eol+2, strlen(eol+1));
+
+	return bod;
+}
+
 static const char *find_encoding(const char *begin, const char *end)
 {
 	const char *needle = "\nencoding ";
@@ -621,6 +681,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
 	int saved_output_format = rev->diffopt.output_format;
 	const char *commit_buffer;
 	const char *author, *author_end, *committer, *committer_end;
+	const char *signature_alg = NULL, *signature;
 	const char *encoding, *message;
 	char *reencoded = NULL;
 	struct commit_list *p;
@@ -644,6 +705,10 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
 	committer++;
 	committer_end = strchrnul(committer, '\n');
 	message = strstr(committer_end, "\n\n");
+	if ((signature = find_signature(committer_end, message, "gpgsig")))
+		signature_alg = "sha1";
+	else if ((signature = find_signature(committer_end, message, "gpgsig-sha256")))
+		signature_alg = "sha256";
 	encoding = find_encoding(committer_end, message);
 	if (message)
 		message += 2;
@@ -703,6 +768,29 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
 	printf("%.*s\n%.*s\n",
 	       (int)(author_end - author), author,
 	       (int)(committer_end - committer), committer);
+	if (signature)
+		switch(signed_commit_mode) {
+		case SIGN_ABORT:
+			die("encountered signed commit %s",
+			    oid_to_hex(&commit->object.oid));
+		case SIGN_VERBATIM_WARN:
+			warning("exporting signed commit %s",
+				oid_to_hex(&commit->object.oid));
+			/* fallthru */
+		case SIGN_VERBATIM:
+			printf("gpgsig %s\ndata %u\n%s",
+			       signature_alg,
+			       (unsigned)strlen(signature),
+			       signature);
+			break;
+		case SIGN_STRIP_WARN:
+			warning("stripping signature from commit %s; use"
+				"--signed-commits=<mode> to handle it differently",
+				oid_to_hex(&commit->object.oid));
+			/* fallthru */
+		case SIGN_STRIP:
+			break;
+		}
 	if (!reencoded && encoding)
 		printf("encoding %s\n", encoding);
 	printf("data %u\n%s",
@@ -830,21 +918,21 @@ static void handle_tag(const char *name, struct tag *tag)
 					       "\n-----BEGIN PGP SIGNATURE-----\n");
 		if (signature)
 			switch(signed_tag_mode) {
-			case SIGNED_TAG_ABORT:
+			case SIGN_ABORT:
 				die("encountered signed tag %s; use "
 				    "--signed-tags=<mode> to handle it",
 				    oid_to_hex(&tag->object.oid));
-			case WARN:
+			case SIGN_VERBATIM_WARN:
 				warning("exporting signed tag %s",
 					oid_to_hex(&tag->object.oid));
 				/* fallthru */
-			case VERBATIM:
+			case SIGN_VERBATIM:
 				break;
-			case WARN_STRIP:
+			case SIGN_STRIP_WARN:
 				warning("stripping signature from tag %s",
 					oid_to_hex(&tag->object.oid));
 				/* fallthru */
-			case STRIP:
+			case SIGN_STRIP:
 				message_size = signature + 1 - message;
 				break;
 			}
@@ -1197,7 +1285,10 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 			    N_("show progress after <n> objects")),
 		OPT_CALLBACK(0, "signed-tags", &signed_tag_mode, N_("mode"),
 			     N_("select handling of signed tags"),
-			     parse_opt_signed_tag_mode),
+			     parse_opt_sign_mode),
+		OPT_CALLBACK(0, "signed-commits", &signed_commit_mode, N_("mode"),
+			     N_("select handling of signed commits"),
+			     parse_opt_sign_mode),
 		OPT_CALLBACK(0, "tag-of-filtered-object", &tag_of_filtered_mode, N_("mode"),
 			     N_("select handling of tags that tag filtered objects"),
 			     parse_opt_tag_of_filtered_mode),
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 3afa81cf9a..ee7516dd38 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -2669,10 +2669,13 @@ static struct hash_list *parse_merge(unsigned int *count)
 
 static void parse_new_commit(const char *arg)
 {
+	static struct strbuf sig = STRBUF_INIT;
 	static struct strbuf msg = STRBUF_INIT;
+	struct string_list siglines = STRING_LIST_INIT_NODUP;
 	struct branch *b;
 	char *author = NULL;
 	char *committer = NULL;
+	char *sig_alg = NULL;
 	char *encoding = NULL;
 	struct hash_list *merge_list = NULL;
 	unsigned int merge_count;
@@ -2696,6 +2699,13 @@ static void parse_new_commit(const char *arg)
 	}
 	if (!committer)
 		die("Expected committer but didn't get one");
+	if (skip_prefix(command_buf.buf, "gpgsig ", &v)) {
+		sig_alg = xstrdup(v);
+		read_next_command();
+		parse_data(&sig, 0, NULL);
+		read_next_command();
+	} else
+		strbuf_setlen(&sig, 0);
 	if (skip_prefix(command_buf.buf, "encoding ", &v)) {
 		encoding = xstrdup(v);
 		read_next_command();
@@ -2769,10 +2779,23 @@ static void parse_new_commit(const char *arg)
 		strbuf_addf(&new_data,
 			"encoding %s\n",
 			encoding);
+	if (sig_alg) {
+		if (!strcmp(sig_alg, "sha1"))
+			strbuf_addstr(&new_data, "gpgsig ");
+		else if (!strcmp(sig_alg, "sha256"))
+			strbuf_addstr(&new_data, "gpgsig-sha256 ");
+		else
+			die("Expected gpgsig algorithm sha1 or sha256, got %s", sig_alg);
+		string_list_split_in_place(&siglines, sig.buf, '\n', -1);
+		strbuf_add_separated_string_list(&new_data, "\n ", &siglines);
+		strbuf_addch(&new_data, '\n');
+	}
 	strbuf_addch(&new_data, '\n');
 	strbuf_addbuf(&new_data, &msg);
+	string_list_clear(&siglines, 1);
 	free(author);
 	free(committer);
+	free(sig_alg);
 	free(encoding);
 
 	if (!store_object(OBJ_COMMIT, &new_data, NULL, &b->oid, next_mark))
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 36b84eff36..45952358ca 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,78 @@ 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 &&
+	git add file-sign &&
+	git commit -S -m "signed commit" &&
+	COMMIT_SIGNING=$(git rev-parse --verify commit-signing)
+
+'
+
+test_expect_success GPG 'signed-commits=abort' '
+
+	test_must_fail git fast-export --signed-commits=abort commit-signing
+
+'
+
+test_expect_success GPG 'signed-commits=verbatim' '
+
+	git fast-export --signed-commits=verbatim --reencode=no commit-signing >output &&
+	grep "^gpgsig sha" output &&
+	grep "encoding ISO-8859-1" output &&
+	(cd new &&
+	 git fast-import &&
+	 test $COMMIT_SIGNING = $(git rev-parse --verify refs/heads/commit-signing)) <output
+
+'
+
+test_expect_success GPG 'signed-commits=warn-verbatim' '
+
+	git fast-export --signed-commits=warn-verbatim --reencode=no commit-signing >output 2>err &&
+	grep "^gpgsig sha" output &&
+	grep "encoding ISO-8859-1" output &&
+	test -s err &&
+	(cd new &&
+	 git fast-import &&
+	 test $COMMIT_SIGNING = $(git rev-parse --verify refs/heads/commit-signing)) <output
+
+'
+
+test_expect_success GPG 'signed-commits=strip' '
+
+	git fast-export --signed-commits=strip --reencode=no commit-signing >output &&
+	! grep ^gpgsig output &&
+	grep "^encoding ISO-8859-1" output &&
+	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))
+
+'
+
+test_expect_success GPG 'signed-commits=warn-strip' '
+
+	git fast-export --signed-commits=warn-strip --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))
+
+'
+
 test_expect_success 'setup submodule' '
 
 	git checkout -f main &&
+	{ git update-ref -d refs/heads/commit-signing || true; } &&
 	mkdir sub &&
 	(
 		cd sub &&
-- 
2.31.1

Re: [PATCH v2 2/3] fast-export: rename --signed-tags='warn' to 'warn-verbatim'

[Eric Sunshine]

On Wed, Apr 21, 2021 at 8:34 PM Luke Shumaker <lukeshu@lukeshu.com> wrote:
> The --signed-tags= option takes one of five arguments specifying how to
> handle signed tags during export.  Among these arguments, 'strip' is to
> 'warn-strip' as 'verbatim' is to 'warn' (the unmentioned argument is
> 'abort', which stops the fast-export process entirely).  That is,
> signatures are either stripped or copied verbatim while exporting, with
> or without a warning.
>
> Match the pattern and rename 'warn' to 'warn-verbatim' to make it clear
> that it instructs fast-export to copy signatures verbatim.
>
> To maintain backwards compatibility, 'warn' is still recognized as
> an undocumented alias.

Maintaining backward compatibility is good; making it undocumented is
perhaps less than good. I understand the motivation for not wanting to
document it: if it's not documented, people won't discover it, thus
won't use it. However, we also should take into consideration that
there may be scripts and documentation in the wild which use `warn`.
If someone comes across one of those and wants to learn what it means,
they won't be able to if the documentation doesn't mention it at all;
they'll either have to consult the source code to find out its purpose
or post a question somewhere, hoping that someone knows the answer.

So, rather than removing it from the documentation altogether, it's
probably better to mention that it is a deprecated alias of
`warn-verbatim`. As precedent, for instance, see the `dimmed-zebra`
option in Documentation/diff-options.txt which still mentions its
`dimmed_zebra` synonym:

    dimmed-zebra::
        Similar to 'zebra', but additional dimming of uninteresting
        parts of moved code is performed. The bordering lines of two
        adjacent blocks are considered interesting, the rest is
        uninteresting. `dimmed_zebra` is a deprecated synonym.

> Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
> ---
> diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
> @@ -27,7 +27,7 @@ OPTIONS
> ---signed-tags=(verbatim|warn|warn-strip|strip|abort)::
> +--signed-tags=(verbatim|warn-verbatim|warn-strip|strip|abort)::
> @@ -36,8 +36,8 @@ When asking to 'abort' (which is the default), this program will die
> -exported and with 'warn', they will be exported, but you will see a
> -warning.
> +exported and with 'warn-verbatim', they will be exported, but you will
> +see a warning.

So, perhaps this could end with:

    `warn` is a deprecated synonym of `warn-verbatim`.

Re: [PATCH v2 2/3] fast-export: rename --signed-tags='warn' to 'warn-verbatim'

[Luke Shumaker]

On Wed, 21 Apr 2021 21:59:04 -0600,
Eric Sunshine wrote:
> On Wed, Apr 21, 2021 at 8:34 PM Luke Shumaker <lukeshu@lukeshu.com> wrote:
> > The --signed-tags= option takes one of five arguments specifying how to
> > handle signed tags during export.  Among these arguments, 'strip' is to
> > 'warn-strip' as 'verbatim' is to 'warn' (the unmentioned argument is
> > 'abort', which stops the fast-export process entirely).  That is,
> > signatures are either stripped or copied verbatim while exporting, with
> > or without a warning.
> >
> > Match the pattern and rename 'warn' to 'warn-verbatim' to make it clear
> > that it instructs fast-export to copy signatures verbatim.
> >
> > To maintain backwards compatibility, 'warn' is still recognized as
> > an undocumented alias.
> 
> Maintaining backward compatibility is good; making it undocumented is
> perhaps less than good. I understand the motivation for not wanting to
> document it: if it's not documented, people won't discover it, thus
> won't use it. However, we also should take into consideration that
> there may be scripts and documentation in the wild which use `warn`.
> If someone comes across one of those and wants to learn what it means,
> they won't be able to if the documentation doesn't mention it at all;
> they'll either have to consult the source code to find out its purpose
> or post a question somewhere, hoping that someone knows the answer.

Fair enough.  My precedent was ee4bc3715f (fast-export: rename the
signed tag mode 'ignore' to 'verbatim', 2007-12-03).

I guess I'll document --signed-tags=ignore too, while I'm at it.

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH v2 2/3] fast-export: rename --signed-tags='warn' to 'warn-verbatim'

[Luke Shumaker]

On Wed, 21 Apr 2021 22:43:48 -0600,
Luke Shumaker wrote:
> I guess I'll document --signed-tags=ignore too, while I'm at it.

Eh, nevermind, --signed-tags=ignore was only a thing for a month in
2007, compared to 13 years that --signed-tags=warn has been the way of
doing things.

-- 
Happy hacking,
~ Luke Shumaker

[PATCH v3 0/3] fast-export, fast-import: implement signed-commits

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

(First of all, my apologies for neglecting to set the In-Reply-To on
the v2 patcheset.)

fast-export has an existing --signed-tags= flag 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).

So implement a --signed-commits= flag in fast-export, and implement
the receiving side of it in fast-import.

I believe that this revision addresses all of the feedback so far,
with the exception that I have not implemented Elijah's suggestion to
implement a flag on fast-import to validate signatures.  While I agree
that this would be a useful feature, I consider it to be beyond the
scope of this work.

This passes all of the GitHub Actions CI checks, and passes all but
one of the Travis-CI checks; the failing Travis-CI check seems to be
an unrelated 404 from `apt-get`.
https://github.com/LukeShu/git/runs/2405123468

Luke Shumaker (3):
  git-fast-import.txt: add missing LF in the BNF
    v2: no changes
    v3: no changes
  fast-export: rename --signed-tags='warn' to 'warn-verbatim'
    v2:
     - Reword commit message, based on feedback from Taylor.
     - Fix copy-pasto in the test, noticed by Taylor.
     - Add a comment to the tests.
     - Fix whitespace in the tests.
    v3:
     - Document that --signed-tags='warn' is a deprecated synonym for
       --signed-tags='warn-verbatim', rather than leaving it
       undocumented, based on feedback from Eric.
  fast-export, fast-import: implement signed-commits
    v2:
     - Remove erroneous remark about ordering from the commit message.
     - Adjust the stream syntax to include the hash algorithm, as
       suggested by brian.
     - Add support for sha256 (based on lots of useful information from
       brian).  It does not support multiply-signed commits.
     - Shorten the documentation, based on feedback from Taylor.
     - Add comments, based on feedback from Taylor.
     - Change the default from `--signed-commits=strip` to
       `--signed-commits=warn-strip`.  This shouldn't break anyone, and
       means that users get useful feedback by default.
    v3: no changes

 Documentation/git-fast-export.txt |  13 +++-
 Documentation/git-fast-import.txt |  20 ++++-
 builtin/fast-export.c             | 123 ++++++++++++++++++++++++++----
 builtin/fast-import.c             |  23 ++++++
 t/t9350-fast-export.sh            |  88 +++++++++++++++++++++
 5 files changed, 247 insertions(+), 20 deletions(-)

-- 
2.31.1

Happy hacking,
~ Luke Shumaker

[PATCH v3 1/3] git-fast-import.txt: add missing LF in the BNF

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---
v2: no changes
v3: no changes

 Documentation/git-fast-import.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 39cfa05b28..458af0a2d6 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -437,7 +437,7 @@ change to the project.
 	original-oid?
 	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
 	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
-	('encoding' SP <encoding>)?
+	('encoding' SP <encoding> LF)?
 	data
 	('from' SP <commit-ish> LF)?
 	('merge' SP <commit-ish> LF)*
-- 
2.31.1

[PATCH v3 2/3] fast-export: rename --signed-tags='warn' to 'warn-verbatim'

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

The --signed-tags= option takes one of five arguments specifying how to
handle signed tags during export.  Among these arguments, 'strip' is to
'warn-strip' as 'verbatim' is to 'warn' (the unmentioned argument is
'abort', which stops the fast-export process entirely).  That is,
signatures are either stripped or copied verbatim while exporting, with
or without a warning.

Match the pattern and rename 'warn' to 'warn-verbatim' to make it clear
that it instructs fast-export to copy signatures verbatim.

To maintain backwards compatibility, 'warn' is still recognized as
deprecated synonym of 'warn-verbatim'.

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---
v2:
 - Reword commit message based on feedback from Taylor.
 - Fix copy-pasto in the test, noticed by Taylor.
 - Add a comment to the tests.
 - Fix whitespace in the tests.
v3:
 - Document that --signed-tags='warn' is a deprecated synonym for
   --signed-tags='warn-verbatim', rather than leaving it
   undocumented, based on feedback from Eric.

 Documentation/git-fast-export.txt |  8 +++++---
 builtin/fast-export.c             |  2 +-
 t/t9350-fast-export.sh            | 18 ++++++++++++++++++
 3 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index 1978dbdc6a..c307839e81 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -27,7 +27,7 @@ OPTIONS
 	Insert 'progress' statements every <n> objects, to be shown by
 	'git fast-import' during import.
 
---signed-tags=(verbatim|warn|warn-strip|strip|abort)::
+--signed-tags=(verbatim|warn-verbatim|warn-strip|strip|abort)::
 	Specify how to handle signed tags.  Since any transformation
 	after the export can change the tag names (which can also happen
 	when excluding revisions) the signatures will not match.
@@ -36,8 +36,10 @@ When asking to 'abort' (which is the default), this program will die
 when encountering a signed tag.  With 'strip', the tags will silently
 be made unsigned, with 'warn-strip' they will be made unsigned but a
 warning will be displayed, with 'verbatim', they will be silently
-exported and with 'warn', they will be exported, but you will see a
-warning.
+exported and with 'warn-verbatim', they will be exported, but you will
+see a warning.
++
+`warn` is a deprecated synonym of `warn-verbatim`.
 
 --tag-of-filtered-object=(abort|drop|rewrite)::
 	Specify how to handle tags whose tagged object is filtered out.
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 85a76e0ef8..d121dd2ee6 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -55,7 +55,7 @@ static int parse_opt_signed_tag_mode(const struct option *opt,
 		signed_tag_mode = SIGNED_TAG_ABORT;
 	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
 		signed_tag_mode = VERBATIM;
-	else if (!strcmp(arg, "warn"))
+	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
 		signed_tag_mode = WARN;
 	else if (!strcmp(arg, "warn-strip"))
 		signed_tag_mode = WARN_STRIP;
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 409b48e244..892737439b 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -253,6 +253,24 @@ test_expect_success 'signed-tags=verbatim' '
 
 '
 
+test_expect_success 'signed-tags=warn-verbatim' '
+
+	git fast-export --signed-tags=warn-verbatim sign-your-name >output 2>err &&
+	grep PGP output &&
+	test -s err
+
+'
+
+# 'warn' is an backward-compatibility alias for 'warn-verbatim'; test
+# that it keeps working.
+test_expect_success 'signed-tags=warn' '
+
+	git fast-export --signed-tags=warn sign-your-name >output 2>err &&
+	grep PGP output &&
+	test -s err
+
+'
+
 test_expect_success 'signed-tags=strip' '
 
 	git fast-export --signed-tags=strip sign-your-name > output &&
-- 
2.31.1

[PATCH v3 3/3] fast-export, fast-import: implement signed-commits

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

fast-export has an existing --signed-tags= flag 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 signed-commits.

On the fast-export side, try to be as much like signed-tags as possible,
in both implementation and in user-interface; with the exception that
the default should be `--signed-commits=warn-strip` (compared to the
default `--signed-tags=abort`), in order to avoid breaking the
historical behavior (it will now print a warning while doing that
behavior, though).

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---
v2:
 - Remove erroneous remark about ordering from the commit message.
 - Adjust the stream syntax to include the hash algorithm, as
   suggested by brian.
 - Add support for sha256 (based on lots of useful information from
   brian).  It does not support multiply-signed commits.
 - Shorten the documentation, based on feedback from Taylor.
 - Add comments, based on feedback from Taylor.
 - Change the default from `--signed-commits=strip` to
   `--signed-commits=warn-strip`.  This shouldn't break anyone, and
   means that users get useful feedback by default.
v3: no changes

 Documentation/git-fast-export.txt |   5 ++
 Documentation/git-fast-import.txt |  18 +++++
 builtin/fast-export.c             | 121 ++++++++++++++++++++++++++----
 builtin/fast-import.c             |  23 ++++++
 t/t9350-fast-export.sh            |  70 +++++++++++++++++
 5 files changed, 222 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index c307839e81..b651fa993b 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -41,6 +41,11 @@ see a warning.
 +
 `warn` is a deprecated synonym of `warn-verbatim`.
 
+--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::
+	Specify how to handle signed commits.  Behaves exactly as
+	--signed-tags (but for commits), except that the default is
+	'warn-strip' rather than 'abort'.
+
 --tag-of-filtered-object=(abort|drop|rewrite)::
 	Specify how to handle tags whose tagged object is filtered out.
 	Since revisions and files to export can be limited by path,
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 458af0a2d6..4955c94305 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -431,12 +431,21 @@ and control the current import process.  More detailed discussion
 Create or update a branch with a new commit, recording one logical
 change to the project.
 
+////
+Yes, it's intentional that the 'gpgsig' line doesn't have a trailing
+`LF`; the the definition of `data` has a byte-count prefix, so it
+doesn't need an `LF` to act as a terminator (and `data` also already
+includes an optional trailing `LF?` just in case you want to include
+one).
+////
+
 ....
 	'commit' SP <ref> LF
 	mark?
 	original-oid?
 	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
 	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
+	('gpgsig' SP <alg> LF data)?
 	('encoding' SP <encoding> LF)?
 	data
 	('from' SP <commit-ish> LF)?
@@ -505,6 +514,15 @@ that was selected by the --date-format=<fmt> command-line option.
 See ``Date Formats'' above for the set of supported formats, and
 their syntax.
 
+`gpgsig`
+^^^^^^^^
+
+The optional `gpgsig` command is used to include a PGP/GPG signature
+that signs the commit data.
+
+Here <alg> specifies which hashing algorithm is used for this
+signature, either `sha1` or `sha256`.
+
 `encoding`
 ^^^^^^^^^^
 The optional `encoding` command indicates the encoding of the commit
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index d121dd2ee6..2b1101d104 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -30,8 +30,11 @@ static const char *fast_export_usage[] = {
 	NULL
 };
 
+enum sign_mode { SIGN_ABORT, SIGN_VERBATIM, SIGN_STRIP, SIGN_VERBATIM_WARN, SIGN_STRIP_WARN };
+
 static int progress;
-static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
+static enum sign_mode signed_tag_mode = SIGN_ABORT;
+static enum sign_mode signed_commit_mode = SIGN_STRIP_WARN;
 static enum { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
 static enum { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
 static int fake_missing_tagger;
@@ -48,21 +51,24 @@ static int anonymize;
 static struct hashmap anonymized_seeds;
 static struct revision_sources revision_sources;
 
-static int parse_opt_signed_tag_mode(const struct option *opt,
+static int parse_opt_sign_mode(const struct option *opt,
 				     const char *arg, int unset)
 {
-	if (unset || !strcmp(arg, "abort"))
-		signed_tag_mode = SIGNED_TAG_ABORT;
+	enum sign_mode *valptr = opt->value;
+	if (unset)
+		return 0;
+	else if (!strcmp(arg, "abort"))
+		*valptr = SIGN_ABORT;
 	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
-		signed_tag_mode = VERBATIM;
+		*valptr = SIGN_VERBATIM;
 	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
-		signed_tag_mode = WARN;
+		*valptr = SIGN_VERBATIM_WARN;
 	else if (!strcmp(arg, "warn-strip"))
-		signed_tag_mode = WARN_STRIP;
+		*valptr = SIGN_STRIP_WARN;
 	else if (!strcmp(arg, "strip"))
-		signed_tag_mode = STRIP;
+		*valptr = SIGN_STRIP;
 	else
-		return error("Unknown signed-tags mode: %s", arg);
+		return error("Unknown %s mode: %s", opt->long_name, arg);
 	return 0;
 }
 
@@ -499,6 +505,60 @@ static void show_filemodify(struct diff_queue_struct *q,
 	}
 }
 
+static const char *find_signature(const char *begin, const char *end, const char *key)
+{
+	static struct strbuf needle = STRBUF_INIT;
+	char *bod, *eod, *eol;
+
+	strbuf_reset(&needle);
+	strbuf_addch(&needle, '\n');
+	strbuf_addstr(&needle, key);
+	strbuf_addch(&needle, ' ');
+
+	bod = memmem(begin, end ? end - begin : strlen(begin),
+		     needle.buf, needle.len);
+	if (!bod)
+		return NULL;
+	bod += needle.len;
+
+	/*
+	 * In the commit object, multi-line header values are stored
+	 * by prefixing continuation lines begin with a space.  So
+	 * within the commit object, it looks like
+	 *
+	 *     "gpgsig -----BEGIN PGP SIGNATURE-----\n"
+	 *     " Version: GnuPG v1.4.5 (GNU/Linux)\n"
+	 *     " \n"
+	 *     " base64_pem_here\n"
+	 *     " -----END PGP SIGNATURE-----\n"
+	 *
+	 * So we need to look for the first '\n' that *isn't* followed
+	 * by a ' ' (or the first '\0', if no such '\n' exists).
+	 */
+	eod = strchrnul(bod, '\n');
+	while (eod[0] == '\n' && eod[1] == ' ') {
+		eod = strchrnul(eod+1, '\n');
+	}
+	*eod = '\0';
+
+	/*
+	 * We now have the value as it's stored in the commit object.
+	 * However, we want the raw value; we want to return
+	 *
+	 *     "-----BEGIN PGP SIGNATURE-----\n"
+	 *     "Version: GnuPG v1.4.5 (GNU/Linux)\n"
+	 *     "\n"
+	 *     "base64_pem_here\n"
+	 *     "-----END PGP SIGNATURE-----\n"
+	 *
+	 * So now we need to strip out all of those extra spaces.
+	 */
+	while ((eol = strstr(bod, "\n ")))
+		memmove(eol+1, eol+2, strlen(eol+1));
+
+	return bod;
+}
+
 static const char *find_encoding(const char *begin, const char *end)
 {
 	const char *needle = "\nencoding ";
@@ -621,6 +681,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
 	int saved_output_format = rev->diffopt.output_format;
 	const char *commit_buffer;
 	const char *author, *author_end, *committer, *committer_end;
+	const char *signature_alg = NULL, *signature;
 	const char *encoding, *message;
 	char *reencoded = NULL;
 	struct commit_list *p;
@@ -644,6 +705,10 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
 	committer++;
 	committer_end = strchrnul(committer, '\n');
 	message = strstr(committer_end, "\n\n");
+	if ((signature = find_signature(committer_end, message, "gpgsig")))
+		signature_alg = "sha1";
+	else if ((signature = find_signature(committer_end, message, "gpgsig-sha256")))
+		signature_alg = "sha256";
 	encoding = find_encoding(committer_end, message);
 	if (message)
 		message += 2;
@@ -703,6 +768,29 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
 	printf("%.*s\n%.*s\n",
 	       (int)(author_end - author), author,
 	       (int)(committer_end - committer), committer);
+	if (signature)
+		switch(signed_commit_mode) {
+		case SIGN_ABORT:
+			die("encountered signed commit %s",
+			    oid_to_hex(&commit->object.oid));
+		case SIGN_VERBATIM_WARN:
+			warning("exporting signed commit %s",
+				oid_to_hex(&commit->object.oid));
+			/* fallthru */
+		case SIGN_VERBATIM:
+			printf("gpgsig %s\ndata %u\n%s",
+			       signature_alg,
+			       (unsigned)strlen(signature),
+			       signature);
+			break;
+		case SIGN_STRIP_WARN:
+			warning("stripping signature from commit %s; use"
+				"--signed-commits=<mode> to handle it differently",
+				oid_to_hex(&commit->object.oid));
+			/* fallthru */
+		case SIGN_STRIP:
+			break;
+		}
 	if (!reencoded && encoding)
 		printf("encoding %s\n", encoding);
 	printf("data %u\n%s",
@@ -830,21 +918,21 @@ static void handle_tag(const char *name, struct tag *tag)
 					       "\n-----BEGIN PGP SIGNATURE-----\n");
 		if (signature)
 			switch(signed_tag_mode) {
-			case SIGNED_TAG_ABORT:
+			case SIGN_ABORT:
 				die("encountered signed tag %s; use "
 				    "--signed-tags=<mode> to handle it",
 				    oid_to_hex(&tag->object.oid));
-			case WARN:
+			case SIGN_VERBATIM_WARN:
 				warning("exporting signed tag %s",
 					oid_to_hex(&tag->object.oid));
 				/* fallthru */
-			case VERBATIM:
+			case SIGN_VERBATIM:
 				break;
-			case WARN_STRIP:
+			case SIGN_STRIP_WARN:
 				warning("stripping signature from tag %s",
 					oid_to_hex(&tag->object.oid));
 				/* fallthru */
-			case STRIP:
+			case SIGN_STRIP:
 				message_size = signature + 1 - message;
 				break;
 			}
@@ -1197,7 +1285,10 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 			    N_("show progress after <n> objects")),
 		OPT_CALLBACK(0, "signed-tags", &signed_tag_mode, N_("mode"),
 			     N_("select handling of signed tags"),
-			     parse_opt_signed_tag_mode),
+			     parse_opt_sign_mode),
+		OPT_CALLBACK(0, "signed-commits", &signed_commit_mode, N_("mode"),
+			     N_("select handling of signed commits"),
+			     parse_opt_sign_mode),
 		OPT_CALLBACK(0, "tag-of-filtered-object", &tag_of_filtered_mode, N_("mode"),
 			     N_("select handling of tags that tag filtered objects"),
 			     parse_opt_tag_of_filtered_mode),
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 3afa81cf9a..ee7516dd38 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -2669,10 +2669,13 @@ static struct hash_list *parse_merge(unsigned int *count)
 
 static void parse_new_commit(const char *arg)
 {
+	static struct strbuf sig = STRBUF_INIT;
 	static struct strbuf msg = STRBUF_INIT;
+	struct string_list siglines = STRING_LIST_INIT_NODUP;
 	struct branch *b;
 	char *author = NULL;
 	char *committer = NULL;
+	char *sig_alg = NULL;
 	char *encoding = NULL;
 	struct hash_list *merge_list = NULL;
 	unsigned int merge_count;
@@ -2696,6 +2699,13 @@ static void parse_new_commit(const char *arg)
 	}
 	if (!committer)
 		die("Expected committer but didn't get one");
+	if (skip_prefix(command_buf.buf, "gpgsig ", &v)) {
+		sig_alg = xstrdup(v);
+		read_next_command();
+		parse_data(&sig, 0, NULL);
+		read_next_command();
+	} else
+		strbuf_setlen(&sig, 0);
 	if (skip_prefix(command_buf.buf, "encoding ", &v)) {
 		encoding = xstrdup(v);
 		read_next_command();
@@ -2769,10 +2779,23 @@ static void parse_new_commit(const char *arg)
 		strbuf_addf(&new_data,
 			"encoding %s\n",
 			encoding);
+	if (sig_alg) {
+		if (!strcmp(sig_alg, "sha1"))
+			strbuf_addstr(&new_data, "gpgsig ");
+		else if (!strcmp(sig_alg, "sha256"))
+			strbuf_addstr(&new_data, "gpgsig-sha256 ");
+		else
+			die("Expected gpgsig algorithm sha1 or sha256, got %s", sig_alg);
+		string_list_split_in_place(&siglines, sig.buf, '\n', -1);
+		strbuf_add_separated_string_list(&new_data, "\n ", &siglines);
+		strbuf_addch(&new_data, '\n');
+	}
 	strbuf_addch(&new_data, '\n');
 	strbuf_addbuf(&new_data, &msg);
+	string_list_clear(&siglines, 1);
 	free(author);
 	free(committer);
+	free(sig_alg);
 	free(encoding);
 
 	if (!store_object(OBJ_COMMIT, &new_data, NULL, &b->oid, next_mark))
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 892737439b..e686c3b894 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,78 @@ 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 &&
+	git add file-sign &&
+	git commit -S -m "signed commit" &&
+	COMMIT_SIGNING=$(git rev-parse --verify commit-signing)
+
+'
+
+test_expect_success GPG 'signed-commits=abort' '
+
+	test_must_fail git fast-export --signed-commits=abort commit-signing
+
+'
+
+test_expect_success GPG 'signed-commits=verbatim' '
+
+	git fast-export --signed-commits=verbatim --reencode=no commit-signing >output &&
+	grep "^gpgsig sha" output &&
+	grep "encoding ISO-8859-1" output &&
+	(cd new &&
+	 git fast-import &&
+	 test $COMMIT_SIGNING = $(git rev-parse --verify refs/heads/commit-signing)) <output
+
+'
+
+test_expect_success GPG 'signed-commits=warn-verbatim' '
+
+	git fast-export --signed-commits=warn-verbatim --reencode=no commit-signing >output 2>err &&
+	grep "^gpgsig sha" output &&
+	grep "encoding ISO-8859-1" output &&
+	test -s err &&
+	(cd new &&
+	 git fast-import &&
+	 test $COMMIT_SIGNING = $(git rev-parse --verify refs/heads/commit-signing)) <output
+
+'
+
+test_expect_success GPG 'signed-commits=strip' '
+
+	git fast-export --signed-commits=strip --reencode=no commit-signing >output &&
+	! grep ^gpgsig output &&
+	grep "^encoding ISO-8859-1" output &&
+	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))
+
+'
+
+test_expect_success GPG 'signed-commits=warn-strip' '
+
+	git fast-export --signed-commits=warn-strip --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))
+
+'
+
 test_expect_success 'setup submodule' '
 
 	git checkout -f main &&
+	{ git update-ref -d refs/heads/commit-signing || true; } &&
 	mkdir sub &&
 	(
 		cd sub &&
-- 
2.31.1

Re: [PATCH v3 2/3] fast-export: rename --signed-tags='warn' to 'warn-verbatim'

[Junio C Hamano]

Luke Shumaker <lukeshu@lukeshu.com> writes:

> ---signed-tags=(verbatim|warn|warn-strip|strip|abort)::
> +--signed-tags=(verbatim|warn-verbatim|warn-strip|strip|abort)::
>  	Specify how to handle signed tags.  Since any transformation
>  	after the export can change the tag names (which can also happen
>  	when excluding revisions) the signatures will not match.
> @@ -36,8 +36,10 @@ When asking to 'abort' (which is the default), this program will die
>  when encountering a signed tag.  With 'strip', the tags will silently
>  be made unsigned, with 'warn-strip' they will be made unsigned but a
>  warning will be displayed, with 'verbatim', they will be silently
> -exported and with 'warn', they will be exported, but you will see a
> -warning.
> +exported and with 'warn-verbatim', they will be exported, but you will
> +see a warning.
> ++
> +`warn` is a deprecated synonym of `warn-verbatim`.

Two minor points

 - Is it obvious to everybody what is the implication of using
   "verbatim" (which in turn would bring the readers to realize why
   it often deserves a warning)?  If not, would it make sense to
   explain why "verbatim" may (may not) be a good idea in different
   situations?

 - I am not sure a deprecated synonym deserves a separate paragraph.

   ... silently exported, and with 'warn-verbatim' (or `warn`, a
   deprecated synonym), they will be exported with a warning.

   may be less irritating to the eyes, perhaps?

> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index 85a76e0ef8..d121dd2ee6 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -55,7 +55,7 @@ static int parse_opt_signed_tag_mode(const struct option *opt,
>  		signed_tag_mode = SIGNED_TAG_ABORT;
>  	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
>  		signed_tag_mode = VERBATIM;
> -	else if (!strcmp(arg, "warn"))
> +	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
>  		signed_tag_mode = WARN;
>  	else if (!strcmp(arg, "warn-strip"))
>  		signed_tag_mode = WARN_STRIP;

It would be preferrable to do s/WARN/WARN_VERBATIM/ at this step, as
the plan is to deprecate "warn", even if you are going to redo the
enums in later steps.  May want to consider doing so as a clean-up
iff this topic need rerolling for other reasons.

> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
> index 409b48e244..892737439b 100755
> --- a/t/t9350-fast-export.sh
> +++ b/t/t9350-fast-export.sh
> @@ -253,6 +253,24 @@ test_expect_success 'signed-tags=verbatim' '
>  
>  '
>  
> +test_expect_success 'signed-tags=warn-verbatim' '
> +
> +	git fast-export --signed-tags=warn-verbatim sign-your-name >output 2>err &&
> +	grep PGP output &&
> +	test -s err

I didn't look at the surrounding existing tests, but in general
"test -s err" is not a good ingredient in any test.  The feature you
happen to care about today may not stay to be be the only thing that
writes to the standard error stream.

Re: [PATCH v3 3/3] fast-export, fast-import: implement signed-commits

[Junio C Hamano]

Luke Shumaker <lukeshu@lukeshu.com> writes:

> From: Luke Shumaker <lukeshu@datawire.io>
>
> fast-export has an existing --signed-tags= flag that controls how to

Don't call a command line option "a flag", especially when it is not
a boolean.

"has an existing" feels redundantly repeticious.

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

That's misleading.  You are not inventing "git commit --signed"
here.

    So implement `--signed-commits=<disposition>` that mirrors the
    `--signed-tags=<disposition>` option.

> +--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::
> +	Specify how to handle signed commits.  Behaves exactly as
> +	--signed-tags (but for commits), except that the default is
> +	'warn-strip' rather than 'abort'.

Why deliberate inconsistency?  I am not sure "historically we did a
wrong thing" is a good reason (if we view that silently stripping
was a disservice to the users, aborting would be a bugfix).

> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> index 458af0a2d6..4955c94305 100644
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index d121dd2ee6..2b1101d104 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -30,8 +30,11 @@ static const char *fast_export_usage[] = {
>  	NULL
>  };
>  
> +enum sign_mode { SIGN_ABORT, SIGN_VERBATIM, SIGN_STRIP, SIGN_VERBATIM_WARN, SIGN_STRIP_WARN };
> +
>  static int progress;
> -static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;

Giving the enum values consistent prefix "SIGN_" is a great
improvement.  On the other hand, swapping the word order,
e.g. WARN_STRIP to SIGN_STRIP_WARN, is unwarranted.

> +static enum sign_mode signed_tag_mode = SIGN_ABORT;
> +static enum sign_mode signed_commit_mode = SIGN_STRIP_WARN;

I think it is safer to abort for both and sell it as a bugfix
("silently stripping commit signatures was wrong. we should abort
the same way by default when encountering a signed tag").

> -static int parse_opt_signed_tag_mode(const struct option *opt,
> +static int parse_opt_sign_mode(const struct option *opt,
>  				     const char *arg, int unset)
>  {
> -	if (unset || !strcmp(arg, "abort"))
> -		signed_tag_mode = SIGNED_TAG_ABORT;
> +	enum sign_mode *valptr = opt->value;
> +	if (unset)
> +		return 0;
> +	else if (!strcmp(arg, "abort"))
> +		*valptr = SIGN_ABORT;
>  	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
> -		signed_tag_mode = VERBATIM;
> +		*valptr = SIGN_VERBATIM;

Interesting and not a new issue at all, but "ignore" is a confusing
symonym to "verbatim"---I would have expected "ignore", if accepted
as a choice, would strip the signature.  Not documenting it is
probably good, but perhaps we would eventually remove it?

> @@ -499,6 +505,60 @@ static void show_filemodify(struct diff_queue_struct *q,
>  	}
>  }
>  
> +static const char *find_signature(const char *begin, const char *end, const char *key)

This is only for in-header signature used in commit objects, and not
for the traditional "attached to the end" signature used in tag
objects, right?

The name of this function should be designed to answer the above
question, but find_signature() that does not say either commit or
tag implies it can accept both (which would be a horrible interface,
though).  If this is only for in-header signature, rename it to make
sure that the fact is readable out of its name?

> +{
> +	static struct strbuf needle = STRBUF_INIT;
> +	char *bod, *eod, *eol;
> +
> +	strbuf_reset(&needle);
> +	strbuf_addch(&needle, '\n');
> +	strbuf_addstr(&needle, key);
> +	strbuf_addch(&needle, ' ');

strbuf_addf(), perhaps?

> +	bod = memmem(begin, end ? end - begin : strlen(begin),
> +		     needle.buf, needle.len);
> +	if (!bod)
> +		return NULL;
> +	bod += needle.len;
> +
> +	/*
> +	 * In the commit object, multi-line header values are stored
> +	 * by prefixing continuation lines begin with a space.  So

"by prefixig continuation lines with a space"

> +	 * within the commit object, it looks like
> +	 *
> +	 *     "gpgsig -----BEGIN PGP SIGNATURE-----\n"
> +	 *     " Version: GnuPG v1.4.5 (GNU/Linux)\n"
> +	 *     " \n"
> +	 *     " base64_pem_here\n"
> +	 *     " -----END PGP SIGNATURE-----\n"
> +	 *
> +	 * So we need to look for the first '\n' that *isn't* followed
> +	 * by a ' ' (or the first '\0', if no such '\n' exists).
> +	 */

> +	eod = strchrnul(bod, '\n');
> +	while (eod[0] == '\n' && eod[1] == ' ') {
> +		eod = strchrnul(eod+1, '\n');
> +	}

SP on both sides of '+'; no {} around a block that consists of a
single statement.

> +	*eod = '\0';

The begin and end pointers pointed to a piece of memory that is
supposed to be read-only, but this pointer points into that region
of memory and then updates a byte?  The function signature is
misleading---if you intend to muck with the string, accept them as
mutable pointers.

Better yet, don't butcher the region of memory pointed by the
"message" variable the caller uses to keep reading from the
remainder of the commit object buffer with this and memmove()
below.  Perhaps have the caller pass a strbuf to fill in the
signature found by this helper as another parameter, and then return
a bool "Yes, I found a sig" as its return value?

> +
> +	/*
> +	 * We now have the value as it's stored in the commit object.
> +	 * However, we want the raw value; we want to return
> +	 *
> +	 *     "-----BEGIN PGP SIGNATURE-----\n"
> +	 *     "Version: GnuPG v1.4.5 (GNU/Linux)\n"
> +	 *     "\n"
> +	 *     "base64_pem_here\n"
> +	 *     "-----END PGP SIGNATURE-----\n"
> +	 *
> +	 * So now we need to strip out all of those extra spaces.
> +	 */
> +	while ((eol = strstr(bod, "\n ")))
> +		memmove(eol+1, eol+2, strlen(eol+1));

Besides, this is O(n^2), isn't it, as it always starts scanning at
bod while there are lines in the signature block to be processed, it
needs to skip over the lines that the loop already has processed.

I'd stop here for now, as there should be enough to polish.

Thanks.

Re: [PATCH v3 2/3] fast-export: rename --signed-tags='warn' to 'warn-verbatim'

[Luke Shumaker]

On Tue, 27 Apr 2021 21:29:12 -0600,
Junio C Hamano wrote:
> Luke Shumaker <lukeshu@lukeshu.com> writes:
> 
> > ---signed-tags=(verbatim|warn|warn-strip|strip|abort)::
> > +--signed-tags=(verbatim|warn-verbatim|warn-strip|strip|abort)::
> >  	Specify how to handle signed tags.  Since any transformation
> >  	after the export can change the tag names (which can also happen
> >  	when excluding revisions) the signatures will not match.
> > @@ -36,8 +36,10 @@ When asking to 'abort' (which is the default), this program will die
> >  when encountering a signed tag.  With 'strip', the tags will silently
> >  be made unsigned, with 'warn-strip' they will be made unsigned but a
> >  warning will be displayed, with 'verbatim', they will be silently
> > -exported and with 'warn', they will be exported, but you will see a
> > -warning.
> > +exported and with 'warn-verbatim', they will be exported, but you will
> > +see a warning.
> > ++
> > +`warn` is a deprecated synonym of `warn-verbatim`.
> 
> Two minor points
> 
>  - Is it obvious to everybody what is the implication of using
>    "verbatim" (which in turn would bring the readers to realize why
>    it often deserves a warning)?  If not, would it make sense to
>    explain why "verbatim" may (may not) be a good idea in different
>    situations?

I had assumed that the above paragraph

|	Specify how to handle signed tags.  Since any transformation
|	after the export can change the tag names (which can also happen
|	when excluding revisions) the signatures will not match.

was adaquate for that purpose, but we can maybe do better?

>  - I am not sure a deprecated synonym deserves a separate paragraph.

Fair enough.  My thinking was to keep the deprecation separate from
the main "happy path" text.

How about:

| Specify how to handle signed tags.  Since any transformation after the
| export (or during the export, such as excluding revisions) can change
| the hashes being signed, the signatures may not match.
|
| When asking to 'abort' (which is the default), this program will die
| when encountering a signed tag.  With 'strip', the tags will silently
| be made unsigned, with 'warn-strip' they will be made unsigned but a
| warning will be displayed, with 'verbatim', they will be silently
| exported and with 'warn-verbatim' (or 'warn', a deprecated synonym),
| they will be exported, but you will see a warning.  'verbatim' should
| not be used unless you know that no transformations affecting tags
| will be performed, or unless you do not care that the resulting tag
| will have an invalid signature.

?

> > diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> > index 85a76e0ef8..d121dd2ee6 100644
> > --- a/builtin/fast-export.c
> > +++ b/builtin/fast-export.c
> > @@ -55,7 +55,7 @@ static int parse_opt_signed_tag_mode(const struct option *opt,
> >  		signed_tag_mode = SIGNED_TAG_ABORT;
> >  	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
> >  		signed_tag_mode = VERBATIM;
> > -	else if (!strcmp(arg, "warn"))
> > +	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
> >  		signed_tag_mode = WARN;
> >  	else if (!strcmp(arg, "warn-strip"))
> >  		signed_tag_mode = WARN_STRIP;
> 
> It would be preferrable to do s/WARN/WARN_VERBATIM/ at this step, as
> the plan is to deprecate "warn", even if you are going to redo the
> enums in later steps.  May want to consider doing so as a clean-up
> iff this topic need rerolling for other reasons.

Ack.

> > diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
> > index 409b48e244..892737439b 100755
> > --- a/t/t9350-fast-export.sh
> > +++ b/t/t9350-fast-export.sh
> > @@ -253,6 +253,24 @@ test_expect_success 'signed-tags=verbatim' '
> >  
> >  '
> >  
> > +test_expect_success 'signed-tags=warn-verbatim' '
> > +
> > +	git fast-export --signed-tags=warn-verbatim sign-your-name >output 2>err &&
> > +	grep PGP output &&
> > +	test -s err
> 
> I didn't look at the surrounding existing tests, but in general
> "test -s err" is not a good ingredient in any test.  The feature you
> happen to care about today may not stay to be be the only thing that
> writes to the standard error stream.

Yeah, that line made me nervous, but I figured if it was good enough
for the existing 'warn-strip' test, then it was good enough for
'warn-verbatim' too.

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH v3 3/3] fast-export, fast-import: implement signed-commits

[Luke Shumaker]

On Tue, 27 Apr 2021 22:02:47 -0600,
Junio C Hamano wrote:
> 
> Luke Shumaker <lukeshu@lukeshu.com> writes:
> 
> > fast-export has an existing --signed-tags= flag that controls how to
> 
> Don't call a command line option "a flag", especially when it is not
> a boolean.

Good to know, perhaps this should be mentioned in CodingGuidelines or
SubmittingPatches.txt?  I see lots of instances in the docs of "flag"
being used.

> "has an existing" feels redundantly repeticious.

I guess I did this to make it clearer that that paragraph is
describing the state of things before the patch, rather than after the
patch.  This is of course the required way of writing messages for
git.git, but I worded it that way to make it clearer to reviewers that
I'm following that requirement (especially since I haven't gotten a
commit landed in git.git before).

> > So, implement signed-commits.
> 
> That's misleading.  You are not inventing "git commit --signed"
> here.
> 
>     So implement `--signed-commits=<disposition>` that mirrors the
>     `--signed-tags=<disposition>` option.

Ack.

> > +--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::
> > +	Specify how to handle signed commits.  Behaves exactly as
> > +	--signed-tags (but for commits), except that the default is
> > +	'warn-strip' rather than 'abort'.
> 
> Why deliberate inconsistency?  I am not sure "historically we did a
> wrong thing" is a good reason (if we view that silently stripping
> was a disservice to the users, aborting would be a bugfix).

I *almost* agree.  I agree in principle, but disagree in practice
because I know that it would break a bunch of existing tooling,
including git-filter-repo.

> >  
> > +enum sign_mode { SIGN_ABORT, SIGN_VERBATIM, SIGN_STRIP, SIGN_VERBATIM_WARN, SIGN_STRIP_WARN };
> > +
> >  static int progress;
> > -static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
> 
> Giving the enum values consistent prefix "SIGN_" is a great
> improvement.  On the other hand, swapping the word order,
> e.g. WARN_STRIP to SIGN_STRIP_WARN, is unwarranted.

Flipping it around made the switch statements read better, I thought.
But I can change it back.

> > +static enum sign_mode signed_tag_mode = SIGN_ABORT;
> > +static enum sign_mode signed_commit_mode = SIGN_STRIP_WARN;
> 
> I think it is safer to abort for both and sell it as a bugfix
> ("silently stripping commit signatures was wrong. we should abort
> the same way by default when encountering a signed tag").
> 
> > -static int parse_opt_signed_tag_mode(const struct option *opt,
> > +static int parse_opt_sign_mode(const struct option *opt,
> >  				     const char *arg, int unset)
> >  {
> > -	if (unset || !strcmp(arg, "abort"))
> > -		signed_tag_mode = SIGNED_TAG_ABORT;
> > +	enum sign_mode *valptr = opt->value;
> > +	if (unset)
> > +		return 0;
> > +	else if (!strcmp(arg, "abort"))
> > +		*valptr = SIGN_ABORT;
> >  	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
> > -		signed_tag_mode = VERBATIM;
> > +		*valptr = SIGN_VERBATIM;
> 
> Interesting and not a new issue at all, but "ignore" is a confusing
> symonym to "verbatim"---I would have expected "ignore", if accepted
> as a choice, would strip the signature.  Not documenting it is
> probably good, but perhaps we would eventually remove it?

Indeed, it was renamed from "ignore" to "verbatim" because "ignore"
was such a confusing name.  It was renamed (in ee4bc3715f
(fast-export: rename the signed tag mode 'ignore' to 'verbatim',
2007-12-03)) by the original fast-export author, pretty much
immediately after fast-export was originally introduced.  There was
never a released version of Git that had fast-export but didn't have
the 'ignore'->'verbatim' rename (fast-export was first released in
v1.5.4, and the rename was already present then).

> > @@ -499,6 +505,60 @@ static void show_filemodify(struct diff_queue_struct *q,
> >  	}
> >  }
> >  
> > +static const char *find_signature(const char *begin, const char *end, const char *key)
> 
> This is only for in-header signature used in commit objects, and not
> for the traditional "attached to the end" signature used in tag
> objects, right?
> 
> The name of this function should be designed to answer the above
> question, but find_signature() that does not say either commit or
> tag implies it can accept both (which would be a horrible interface,
> though).  If this is only for in-header signature, rename it to make
> sure that the fact is readable out of its name?
> 
> > +{
> > +	static struct strbuf needle = STRBUF_INIT;
> > +	char *bod, *eod, *eol;
> > +
> > +	strbuf_reset(&needle);
> > +	strbuf_addch(&needle, '\n');
> > +	strbuf_addstr(&needle, key);
> > +	strbuf_addch(&needle, ' ');
> 
> strbuf_addf(), perhaps?

Currently, strbuf_addf is only used by fast-export.c in the
"anonymize_" functions.  I took that to mean "avoid strbuf_addf if you
can", figuring that fast-export and fast-import seem to go reasonably
far out of their way to avoid dynamic things (like printf) in the
happy-path.

But I can change if it I'm mis-reading fast-export's paranoia.

> > +	bod = memmem(begin, end ? end - begin : strlen(begin),
> > +		     needle.buf, needle.len);
> > +	if (!bod)
> > +		return NULL;
> > +	bod += needle.len;
> > +
> > +	/*
> > +	 * In the commit object, multi-line header values are stored
> > +	 * by prefixing continuation lines begin with a space.  So
> 
> "by prefixig continuation lines with a space"

Oops.

> > +	 * within the commit object, it looks like
> > +	 *
> > +	 *     "gpgsig -----BEGIN PGP SIGNATURE-----\n"
> > +	 *     " Version: GnuPG v1.4.5 (GNU/Linux)\n"
> > +	 *     " \n"
> > +	 *     " base64_pem_here\n"
> > +	 *     " -----END PGP SIGNATURE-----\n"
> > +	 *
> > +	 * So we need to look for the first '\n' that *isn't* followed
> > +	 * by a ' ' (or the first '\0', if no such '\n' exists).
> > +	 */
> 
> > +	eod = strchrnul(bod, '\n');
> > +	while (eod[0] == '\n' && eod[1] == ' ') {
> > +		eod = strchrnul(eod+1, '\n');
> > +	}
> 
> SP on both sides of '+'; no {} around a block that consists of a
> single statement.

Ack.

> > +	*eod = '\0';
> 
> The begin and end pointers pointed to a piece of memory that is
> supposed to be read-only, but this pointer points into that region
> of memory and then updates a byte?  The function signature is
> misleading---if you intend to muck with the string, accept them as
> mutable pointers.
> 
> Better yet, don't butcher the region of memory pointed by the
> "message" variable the caller uses to keep reading from the
> remainder of the commit object buffer with this and memmove()
> below.  Perhaps have the caller pass a strbuf to fill in the
> signature found by this helper as another parameter, and then return
> a bool "Yes, I found a sig" as its return value?

That all sounds very sane, but I was mimicking the existing
`find_encoding`.

You aren't supposed to modify the memory from get_commit_buffer, but
fast-export does anyway.  I assume that Johannes knew what he was
doing when he wrote it (that it's safe because fast-export never
traverses the same object twice?) and that he did it as an
allocation-avoiding optimization.

Part of me thinks that it would be better to just use the standard
functions for this, like read_commit_extra_headers or
find_commit_header?

> > +
> > +	/*
> > +	 * We now have the value as it's stored in the commit object.
> > +	 * However, we want the raw value; we want to return
> > +	 *
> > +	 *     "-----BEGIN PGP SIGNATURE-----\n"
> > +	 *     "Version: GnuPG v1.4.5 (GNU/Linux)\n"
> > +	 *     "\n"
> > +	 *     "base64_pem_here\n"
> > +	 *     "-----END PGP SIGNATURE-----\n"
> > +	 *
> > +	 * So now we need to strip out all of those extra spaces.
> > +	 */
> > +	while ((eol = strstr(bod, "\n ")))
> > +		memmove(eol+1, eol+2, strlen(eol+1));
> 
> Besides, this is O(n^2), isn't it, as it always starts scanning at
> bod while there are lines in the signature block to be processed, it
> needs to skip over the lines that the loop already has processed.

Indeed, I'm embarrassed that made it in to something I submitted.

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH v3 3/3] fast-export, fast-import: implement signed-commits

[Elijah Newren]

On Thu, Apr 29, 2021 at 1:06 PM Luke Shumaker <lukeshu@lukeshu.com> wrote:
>
> On Tue, 27 Apr 2021 22:02:47 -0600,
> Junio C Hamano wrote:
> >
> > Luke Shumaker <lukeshu@lukeshu.com> writes:
> >

> > > +--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::
> > > +   Specify how to handle signed commits.  Behaves exactly as
> > > +   --signed-tags (but for commits), except that the default is
> > > +   'warn-strip' rather than 'abort'.
> >
> > Why deliberate inconsistency?  I am not sure "historically we did a
> > wrong thing" is a good reason (if we view that silently stripping
> > was a disservice to the users, aborting would be a bugfix).
>
> I *almost* agree.  I agree in principle, but disagree in practice
> because I know that it would break a bunch of existing tooling,
> including git-filter-repo.

I understand that fast-export's behavior in the past matched what
--signed-commits=warn-strip would now do, and thus you wanted to
select it for backward compatibility.  But throwing an error and
making the user choose when they are potentially losing data seems
like a safer choice to me.

I do get that we might have to use warn-strip as the default anyway
just because some existing tools might rely on it, but do you have any
examples outside of git-filter-repo?  Given the filter-repo bug
reports I've gotten with users being surprised at commit signatures
being stripped (despite the fact that this is documented -- users
don't always read the documentation), I'd argue that changing to
--signed-commits=abort as the default is probably a good bugfix for
both fast-export and for filter-repo.

Clearly, it'd probably make sense for filter-repo to also add an
option for the user to select to: (0) abort if commit signatures are
found, (1) strip commit signatures, (2) retain commit signatures even
if they are invalid, or (3) only retain commit signatures if they are
valid.  In the past, we could only reasonably do (1).  Your series
makes (0) and (2) possible.  More work in fast-import would be needed
to make (3) a possibility, so I wouldn't be able to add it to
filter-repo yet, but I could add the other options.

Re: [PATCH v3 3/3] fast-export, fast-import: implement signed-commits

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> I do get that we might have to use warn-strip as the default anyway
> just because some existing tools might rely on it, but do you have any
> examples outside of git-filter-repo?  Given the filter-repo bug
> reports I've gotten with users being surprised at commit signatures
> being stripped (despite the fact that this is documented -- users
> don't always read the documentation), I'd argue that changing to
> --signed-commits=abort as the default is probably a good bugfix for
> both fast-export and for filter-repo.

Thanks.  The "filter-repo already gets bug reports from the users"
is a valuable input when deciding if it is reasonable to sell the
behaviour change as a bugfix to our users.

Perhaps teaching fast-export to pay attention to two environment
variables that say "when no --signed-{tag,commit}=<disposition>"
command line option is given, use this behaviour" would be a good
enough escape hatch for existing tools and their users, while they
are waiting for their tools to get updated with the new option you
are planning to add?

Also, I am glad that you brought up another possible behaviour that
Luke's patch did not add.  Exporting existing signatures that may
become invalid and deciding what to do with them on the receiving
end would be a good option to have.  And that would most likely have
to be done at "fast-import" end, as a commit that "fast-export"
expected to retain its object name if its export stream were applied
as-is may not retain the object name when the export stream gets
preprocessed before being fed to "fast-import".

Re: [PATCH v3 2/3] fast-export: rename --signed-tags='warn' to 'warn-verbatim'

[Junio C Hamano]

Luke Shumaker <lukeshu@lukeshu.com> writes:

> How about:
>
> | Specify how to handle signed tags.  Since any transformation after the
> | export (or during the export, such as excluding revisions) can change
> | the hashes being signed, the signatures may not match.

I find it a bit worrying that it is unclear what the signature may
not match.  Knowing Git, I know the answer is "contents that is
signed", and I want to make sure it is clear for all readers.

Would "may become invalid" be better?  I dunno.

> | When asking to 'abort' (which is the default), this program will die
> | when encountering a signed tag.  With 'strip', the tags will silently
> | be made unsigned, with 'warn-strip' they will be made unsigned but a
> | warning will be displayed, with 'verbatim', they will be silently
> | exported and with 'warn-verbatim' (or 'warn', a deprecated synonym),
> | they will be exported, but you will see a warning.  'verbatim' should
> | not be used unless you know that no transformations affecting tags
> | will be performed, or unless you do not care that the resulting tag
> | will have an invalid signature.

OK.

As the current version of "fast-import" has no way to specify what
is done to incoming signed tags, it may be the best we can do to
discourage 'verbatim'.  But if it learns "--signed-tags=<disposition>",
I think the resulting ecosystem would become much better.

In the ideal world, I would imagine that we want to encourage to
always write out the original signatures to the export stream, let
any intermediary filters process the stream, and at the very end
stage at fast-import, have the --signed-commit/tag option to control
what is done to such signatures.  The set of plausible options are
what you invented for the export side in this series, plus "if the
signature still matches, keep it, otherwise strip with warning".

If we want to get closer to such an ideal world (you can point out I
am wrong and why such a world is not ideal, of course, though), we
probably do not want to add "--signed-commit" to "fast-export", as
it will have to get deprecated when the ideal world happens.
Rather, the future would deprecate the existing "--signed-tags"
option from "fast-export" instead.

Re: [PATCH v3 3/3] fast-export, fast-import: implement signed-commits

[Elijah Newren]

On Thu, Apr 29, 2021 at 4:42 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > I do get that we might have to use warn-strip as the default anyway
> > just because some existing tools might rely on it, but do you have any
> > examples outside of git-filter-repo?  Given the filter-repo bug
> > reports I've gotten with users being surprised at commit signatures
> > being stripped (despite the fact that this is documented -- users
> > don't always read the documentation), I'd argue that changing to
> > --signed-commits=abort as the default is probably a good bugfix for
> > both fast-export and for filter-repo.
>
> Thanks.  The "filter-repo already gets bug reports from the users"
> is a valuable input when deciding if it is reasonable to sell the
> behaviour change as a bugfix to our users.
>
> Perhaps teaching fast-export to pay attention to two environment
> variables that say "when no --signed-{tag,commit}=<disposition>"
> command line option is given, use this behaviour" would be a good
> enough escape hatch for existing tools and their users, while they
> are waiting for their tools to get updated with the new option you
> are planning to add?

As far as git filter-repo is concerned, you can immediately introduce
--signed-commit and give it a default value of abort with no
deprecation period.  filter-repo already has to check git versions for
available command line options, so one more wouldn't hurt.  And a
default of "abort" seems more user friendly, as it gives users a
chance to be aware of and handle their data appropriately.

Of course, there are a few factors that make filter-repo more tolerant
of upstream changes: I don't expect people to user filter-repo often
(it's a once-in-a-blue-moon rewrite), I don't expect them to use it in
automated processes, I tend to make releases that coincide in timing
with git releases (so I'll just release a git-filter-repo 2.32.0 the
day you release git 2.32, and it'll come with an option to handle this
new default), and filter-repo includes the following disclaimer in its
documentation:

"""
I assume that people use filter-repo for one-shot conversions, not
ongoing data transfers. I explicitly reserve the right to change any
API in filter-repo based on this presumption (and a comment to this
effect is found in multiple places in the code and examples). You have
been warned.
"""

So, if it's just for filter-repo, then I'd say just change
fast-export's default now.  If you're concerned with
--signed-commit=abort being a changed default being too drastic for
other users or tools, then the environment variable escape hatch
sounds reasonable to me.

Personally, I'm worried users are seeing "lost" data (though they
don't notice it until weeks or months later) and are being surprised
by it, which feels like a bigger issue to me than "my automated script
isn't running anymore on this one repo, now I have to figure out what
flag to use in order to choose whether I care about that data from
that special repo being tossed or not".  So I would bias towards
throwing an error so users get a chance to handle it.

> Also, I am glad that you brought up another possible behaviour that
> Luke's patch did not add.  Exporting existing signatures that may
> become invalid and deciding what to do with them on the receiving
> end would be a good option to have.  And that would most likely have
> to be done at "fast-import" end, as a commit that "fast-export"
> expected to retain its object name if its export stream were applied
> as-is may not retain the object name when the export stream gets
> preprocessed before being fed to "fast-import".

Right, but I'd go a step further: Even if the fast-export stream is
not pre-processed before feeding to fast-import, you still cannot
always expect to get the same object names when importing the stream.

To see why, note that the fast-export stream has no way to encode tree
information.  So if trees in the original history deviated from
"normal" in some fashion, such as not-quite-sorted entries, or non
standard modes, then sending those objects through fast-export and
fast-import will necessarily result in different object names.
fast-export also may have modified other objects to normalize them,
either because of default re-encoding of commit messages into UTF-8,
because of stripping any unrecognized commit headers, or perhaps even
because it'd truncate commit messages with an embedded NUL character.

Combine all these "normalizations" that fast-export/fast-import do
with the ability for users to process the stream from fast-export to
fast-import and it becomes clear that the only stage in the pipeline
that can check the validity of the gpg signatures for the imported
history is the fast-import step.

Re: [PATCH v3 3/3] fast-export, fast-import: implement signed-commits

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> So, if it's just for filter-repo, then I'd say just change
> fast-export's default now.  If you're concerned with
> --signed-commit=abort being a changed default being too drastic for
> other users or tools, then the environment variable escape hatch
> sounds reasonable to me.

I wasn't specifically worried about any single tool.  It is largely
third-party's business, and my job is to make sure it won't be too
hard for them to adjust to our changes.

Even existing users of filter-repo would probably need such an
escape hatch, as it may not necessarily be possible to update
filter-repo at the same time they update Git.

Unless filter-repo refuses to work with a version of Git that is
newer than what it knows about (which is not quite how I would
prepare a tool for external change, though), that is.

> Combine all these "normalizations" that fast-export/fast-import do
> with the ability for users to process the stream from fast-export to
> fast-import and it becomes clear that the only stage in the pipeline
> that can check the validity of the gpg signatures for the imported
> history is the fast-import step.

Yup.  So I guess we two are in agreement wrt the "ideal world".

Re: [PATCH v3 3/3] fast-export, fast-import: implement signed-commits

[Luke Shumaker]

On Thu, 29 Apr 2021 17:42:24 -0600,
Junio C Hamano wrote:
> 
> Elijah Newren <newren@gmail.com> writes:
> 
> > I do get that we might have to use warn-strip as the default anyway
> > just because some existing tools might rely on it, but do you have any
> > examples outside of git-filter-repo?  Given the filter-repo bug
> > reports I've gotten with users being surprised at commit signatures
> > being stripped (despite the fact that this is documented -- users
> > don't always read the documentation), I'd argue that changing to
> > --signed-commits=abort as the default is probably a good bugfix for
> > both fast-export and for filter-repo.
> 
> Thanks.  The "filter-repo already gets bug reports from the users"
> is a valuable input when deciding if it is reasonable to sell the
> behaviour change as a bugfix to our users.
> 
> Perhaps teaching fast-export to pay attention to two environment
> variables that say "when no --signed-{tag,commit}=<disposition>"
> command line option is given, use this behaviour" would be a good
> enough escape hatch for existing tools and their users, while they
> are waiting for their tools to get updated with the new option you
> are planning to add?

Between Elijah being on-board with changing the default, and the
suggested env-var escape hatch, you've won me over.

I'll change the default to 'abort' and implement an env-var escape
hatch.  Any suggestions on how to name it?
`FAST_EXPORT_SIGNED_COMMITS`?  Should I give it a `GIT_` prefix?
`FILTER_BRANCH_SQUELCH_WARNING` doesn't have a `GIT_` prefix...

> Also, I am glad that you brought up another possible behaviour that
> Luke's patch did not add.  Exporting existing signatures that may
> become invalid and deciding what to do with them on the receiving
> end would be a good option to have.  And that would most likely have
> to be done at "fast-import" end, as a commit that "fast-export"
> expected to retain its object name if its export stream were applied
> as-is may not retain the object name when the export stream gets
> preprocessed before being fed to "fast-import".

Elijah suggested that on an earlier version of the patchset too.  I
agree that it's a splendid idea, but I'm not willing to be the one to
do the work of implementing it... at least not in the next few months.

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH v3 3/3] fast-export, fast-import: implement signed-commits

[Luke Shumaker]

On Tue, 27 Apr 2021 22:02:47 -0600,
Junio C Hamano wrote:
> Better yet, don't butcher the region of memory pointed by the
> "message" variable the caller uses to keep reading from the
> remainder of the commit object buffer with this and memmove()
> below.  Perhaps have the caller pass a strbuf to fill in the
> signature found by this helper as another parameter, and then return
> a bool "Yes, I found a sig" as its return value?

Stupid question: is there a better way to append a region of bytes to
a strbuf than

    strbuf_addf(&buf, "%.*s", (int)(str_end - str_beg), str);

?

It seems weird to me to invoke the printf machinery for something so
simple, but I don't see anything alternatives in strbuf.h.  Am I
missing something?

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH v3 3/3] fast-export, fast-import: implement signed-commits

[Elijah Newren]

On Fri, Apr 30, 2021 at 12:34 PM Luke Shumaker <lukeshu@lukeshu.com> wrote:
>
> On Tue, 27 Apr 2021 22:02:47 -0600,
> Junio C Hamano wrote:
> > Better yet, don't butcher the region of memory pointed by the
> > "message" variable the caller uses to keep reading from the
> > remainder of the commit object buffer with this and memmove()
> > below.  Perhaps have the caller pass a strbuf to fill in the
> > signature found by this helper as another parameter, and then return
> > a bool "Yes, I found a sig" as its return value?
>
> Stupid question: is there a better way to append a region of bytes to
> a strbuf than
>
>     strbuf_addf(&buf, "%.*s", (int)(str_end - str_beg), str);
>
> ?
>
> It seems weird to me to invoke the printf machinery for something so
> simple, but I don't see anything alternatives in strbuf.h.  Am I
> missing something?

I struggled to find it some time ago as well; I wonder if some
reorganization of strbuf.[ch] might make it more clear.

Anyway, strbuf_add() if you have the number of bytes already handy,
strbuf_addstr() if you don't have the number of bytes handy but the
string is NUL-delimited.

Re: [PATCH v3 3/3] fast-export, fast-import: implement signed-commits

[Luke Shumaker]

On Fri, 30 Apr 2021 13:59:43 -0600,
Elijah Newren wrote:
> 
> On Fri, Apr 30, 2021 at 12:34 PM Luke Shumaker <lukeshu@lukeshu.com> wrote:
> >
> > On Tue, 27 Apr 2021 22:02:47 -0600,
> > Junio C Hamano wrote:
> > > Better yet, don't butcher the region of memory pointed by the
> > > "message" variable the caller uses to keep reading from the
> > > remainder of the commit object buffer with this and memmove()
> > > below.  Perhaps have the caller pass a strbuf to fill in the
> > > signature found by this helper as another parameter, and then return
> > > a bool "Yes, I found a sig" as its return value?
> >
> > Stupid question: is there a better way to append a region of bytes to
> > a strbuf than
> >
> >     strbuf_addf(&buf, "%.*s", (int)(str_end - str_beg), str);
> >
> > ?
> >
> > It seems weird to me to invoke the printf machinery for something so
> > simple, but I don't see anything alternatives in strbuf.h.  Am I
> > missing something?
> 
> I struggled to find it some time ago as well; I wonder if some
> reorganization of strbuf.[ch] might make it more clear.
> 
> Anyway, strbuf_add() if you have the number of bytes already handy,
> strbuf_addstr() if you don't have the number of bytes handy but the
> string is NUL-delimited.

Ah!  I was looking for `char *`, but strbuf_add takes a `void *`,
that's why I didn't find it.

Thank you!

-- 
Happy hacking,
~ Luke Shumaker

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

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

fast-export has an existing --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).

So implement a --signed-commits= flag in fast-export, and implement
the receiving side of it in fast-import.

I believe that this revision addresses all of the feedback so far,
with the exceptions that: (1) I have not implemented Elijah's
suggestion to implement a flag on fast-import to validate signatures.
While I agree that this would be a useful feature, I consider it to be
beyond the scope of this work. (2) The added tests still use `test -s
err`, as that's what's used by the other existing tests.

Notable changes in v4 include adjusting fast-export to not butcher
memory from get_commit_buffer (both adding a new commit to fix
existing butchery, and adjusting the code added in the final commit),
and changing the default to --signed-commits=abort, but adding a
`FAST_EXPORT_SIGNED_COMMITS_NOABORT=1` environment variable.

Luke Shumaker (5):
  git-fast-import.txt: add missing LF in the BNF
  fast-export: rename --signed-tags='warn' to 'warn-verbatim'
  git-fast-export.txt: clarify why 'verbatim' may not be a good idea
  fast-export: do not modify memory from get_commit_buffer
  fast-export, fast-import: add support for signed-commits

 Documentation/git-fast-export.txt |  25 ++++-
 Documentation/git-fast-import.txt |  20 +++-
 builtin/fast-export.c             | 181 ++++++++++++++++++++++--------
 builtin/fast-import.c             |  23 ++++
 t/t9350-fast-export.sh            | 104 +++++++++++++++++
 5 files changed, 303 insertions(+), 50 deletions(-)

Range-diff against v3:
1:  ee767f3a8f ! 1:  3116d531ab git-fast-import.txt: add missing LF in the BNF
    @@ Commit message
     
         Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
     
    +
    + ## Notes ##
    +    v2: no changes
    +    v3: no changes
    +    v4: no changes
    +
      ## Documentation/git-fast-import.txt ##
     @@ Documentation/git-fast-import.txt: change to the project.
      	original-oid?
2:  4612dbcdd5 ! 2:  b035fae93c fast-export: rename --signed-tags='warn' to 'warn-verbatim'
    @@ Commit message
     
         Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
     
    +
    + ## Notes ##
    +    v2:
    +     - Reword commit message based on feedback from Taylor.
    +     - Fix copy-pasto in the test, noticed by Taylor.
    +     - Add a comment to the tests.
    +     - Fix whitespace in the tests.
    +    v3:
    +     - Document that --signed-tags='warn' is a deprecated synonym for
    +       --signed-tags='warn-verbatim', rather than leaving it
    +       undocumented, based on feedback from Eric.
    +    v4:
    +     - Don't give the "deprecated synonym" mention in the docs its own
    +       paragraph.
    +     - Don't just rename the user-facing string, also rename the internal
    +       enum item from WARN to WARN_VERBATIM.
    +
      ## Documentation/git-fast-export.txt ##
     @@ Documentation/git-fast-export.txt: OPTIONS
      	Insert 'progress' statements every <n> objects, to be shown by
    @@ Documentation/git-fast-export.txt: When asking to 'abort' (which is the default)
      warning will be displayed, with 'verbatim', they will be silently
     -exported and with 'warn', they will be exported, but you will see a
     -warning.
    -+exported and with 'warn-verbatim', they will be exported, but you will
    -+see a warning.
    -++
    -+`warn` is a deprecated synonym of `warn-verbatim`.
    ++exported and with 'warn-verbatim' (or 'warn', a deprecated synonym),
    ++they will be exported, but you will see a warning.
      
      --tag-of-filtered-object=(abort|drop|rewrite)::
      	Specify how to handle tags whose tagged object is filtered out.
     
      ## builtin/fast-export.c ##
    +@@ builtin/fast-export.c: static const char *fast_export_usage[] = {
    + };
    + 
    + static int progress;
    +-static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
    ++static enum { SIGNED_TAG_ABORT, VERBATIM, WARN_VERBATIM, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
    + static enum { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
    + static enum { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
    + static int fake_missing_tagger;
     @@ builtin/fast-export.c: static int parse_opt_signed_tag_mode(const struct option *opt,
      		signed_tag_mode = SIGNED_TAG_ABORT;
      	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
      		signed_tag_mode = VERBATIM;
     -	else if (!strcmp(arg, "warn"))
    +-		signed_tag_mode = WARN;
     +	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
    - 		signed_tag_mode = WARN;
    ++		signed_tag_mode = WARN_VERBATIM;
      	else if (!strcmp(arg, "warn-strip"))
      		signed_tag_mode = WARN_STRIP;
    + 	else if (!strcmp(arg, "strip"))
    +@@ builtin/fast-export.c: static void handle_tag(const char *name, struct tag *tag)
    + 				die("encountered signed tag %s; use "
    + 				    "--signed-tags=<mode> to handle it",
    + 				    oid_to_hex(&tag->object.oid));
    +-			case WARN:
    ++			case WARN_VERBATIM:
    + 				warning("exporting signed tag %s",
    + 					oid_to_hex(&tag->object.oid));
    + 				/* fallthru */
     
      ## t/t9350-fast-export.sh ##
     @@ t/t9350-fast-export.sh: test_expect_success 'signed-tags=verbatim' '
-:  ---------- > 3:  38b1ea78fd git-fast-export.txt: clarify why 'verbatim' may not be a good idea
-:  ---------- > 4:  1c34b843fb fast-export: do not modify memory from get_commit_buffer
3:  e57f82e443 ! 5:  788542f669 fast-export, fast-import: implement signed-commits
    @@ Metadata
     Author: Luke Shumaker <lukeshu@datawire.io>
     
      ## Commit message ##
    -    fast-export, fast-import: implement signed-commits
    +    fast-export, fast-import: add support for signed-commits
     
    -    fast-export has an existing --signed-tags= flag 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).
    +    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 signed-commits.
    +    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; with the exception that
    -    the default should be `--signed-commits=warn-strip` (compared to the
    -    default `--signed-tags=abort`), in order to avoid breaking the
    -    historical behavior (it will now print a warning while doing that
    -    behavior, though).
    +    in both implementation and in user-interface.  This will changes the
    +    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'.
     
         Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
     
    +
    + ## Notes ##
    +    v2:
    +     - Remove erroneous remark about ordering from the commit message.
    +     - Adjust the stream syntax to include the hash algorithm, as
    +       suggested by brian.
    +     - Add support for sha256 (based on lots of useful information from
    +       brian).  It does not support multiply-signed commits.
    +     - Shorten the documentation, based on feedback from Taylor.
    +     - Add comments, based on feedback from Taylor.
    +     - Change the default from `--signed-commits=strip` to
    +       `--signed-commits=warn-strip`.  This shouldn't break anyone, and
    +       means that users get useful feedback by default.
    +    v3: no changes
    +    v4:
    +     - Reword the commit message based on feedback from Junio.
    +     - v1-v3 renamed enum items to SIGN_VERBATIM_WARN and SIGN_STRIP_WARN,
    +       rename them to SIGN_WARN_VERBATIM and SIGN_WARN_STRIP instead.
    +     - Rewrite find_signature() as find_commit_multiline_header().  Don't
    +       have it butcher the memory that we pass to it; have it return its
    +       own buffer.
    +     - Change the default from `--signed-commits=warn-strip` to
    +       `--signed-commits=abort`, to match `--signed-tags`.
    +     - Add a FAST_EXPORT_SIGNED_COMMITS_NOABORT=1 env-var to change the
    +       default to `--signed-commits=warn-strip`.
    +
      ## Documentation/git-fast-export.txt ##
    -@@ Documentation/git-fast-export.txt: see a warning.
    - +
    - `warn` is a deprecated synonym of `warn-verbatim`.
    +@@ Documentation/git-fast-export.txt: they will be exported, but you will see a warning.  'verbatim' and
    + transformations affecting tags will be performed, or if you do not
    + care that the resulting tag will have an invalid signature.
      
     +--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::
     +	Specify how to handle signed commits.  Behaves exactly as
    -+	--signed-tags (but for commits), except that the default is
    -+	'warn-strip' rather than 'abort'.
    ++	'--signed-tags', but for commits.
    +++
    ++Earlier versions this command that did not have '--signed-commits'
    ++behaved as if '--signed-commits=strip'.  As an escape hatch for users
    ++of tools that call 'git fast-export' but do not yet support
    ++'--signed-commits', you may set the environment variable
    ++'FAST_EXPORT_SIGNED_COMMITS_NOABORT=1' in order to change the default
    ++from 'abort' to 'warn-strip'.
     +
      --tag-of-filtered-object=(abort|drop|rewrite)::
      	Specify how to handle tags whose tagged object is filtered out.
    @@ builtin/fast-export.c: static const char *fast_export_usage[] = {
      	NULL
      };
      
    -+enum sign_mode { SIGN_ABORT, SIGN_VERBATIM, SIGN_STRIP, SIGN_VERBATIM_WARN, SIGN_STRIP_WARN };
    ++enum sign_mode { SIGN_ABORT, SIGN_VERBATIM, SIGN_STRIP, SIGN_WARN_VERBATIM, SIGN_WARN_STRIP };
     +
      static int progress;
    --static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
    +-static enum { SIGNED_TAG_ABORT, VERBATIM, WARN_VERBATIM, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
     +static enum sign_mode signed_tag_mode = SIGN_ABORT;
    -+static enum sign_mode signed_commit_mode = SIGN_STRIP_WARN;
    ++static enum sign_mode signed_commit_mode = SIGN_ABORT;
      static enum { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
      static enum { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
      static int fake_missing_tagger;
    @@ builtin/fast-export.c: static int anonymize;
     -		signed_tag_mode = VERBATIM;
     +		*valptr = SIGN_VERBATIM;
      	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
    --		signed_tag_mode = WARN;
    -+		*valptr = SIGN_VERBATIM_WARN;
    +-		signed_tag_mode = WARN_VERBATIM;
    ++		*valptr = SIGN_WARN_VERBATIM;
      	else if (!strcmp(arg, "warn-strip"))
     -		signed_tag_mode = WARN_STRIP;
    -+		*valptr = SIGN_STRIP_WARN;
    ++		*valptr = SIGN_WARN_STRIP;
      	else if (!strcmp(arg, "strip"))
     -		signed_tag_mode = STRIP;
     +		*valptr = SIGN_STRIP;
    @@ builtin/fast-export.c: static int anonymize;
      	return 0;
      }
      
    -@@ builtin/fast-export.c: static void show_filemodify(struct diff_queue_struct *q,
    - 	}
    +@@ builtin/fast-export.c: static void anonymize_ident_line(const char **beg, const char **end)
    + 	*end = out->buf + out->len;
      }
      
    -+static const char *find_signature(const char *begin, const char *end, const char *key)
    ++/*
    ++ * find_commit_multiline_header is similar to find_commit_header,
    ++ * except that it handles multi-line headers, rathar than simply
    ++ * returning the first line of the header.
    ++ *
    ++ * The returned string has had the ' ' line continuation markers
    ++ * removed, and points to staticly allocated memory (not to memory
    ++ * within 'msg'), so it is only valid until the next call to
    ++ * find_commit_multiline_header.
    ++ *
    ++ * If the header is found, then *end is set to point at the '\n' in
    ++ * msg that immediately follows the header value.
    ++ */
    ++static const char *find_commit_multiline_header(const char *msg,
    ++						const char *key,
    ++						const char **end)
     +{
    -+	static struct strbuf needle = STRBUF_INIT;
    -+	char *bod, *eod, *eol;
    ++	static struct strbuf val = STRBUF_INIT;
    ++	const char *bol, *eol;
    ++	size_t len;
     +
    -+	strbuf_reset(&needle);
    -+	strbuf_addch(&needle, '\n');
    -+	strbuf_addstr(&needle, key);
    -+	strbuf_addch(&needle, ' ');
    ++	strbuf_reset(&val);
     +
    -+	bod = memmem(begin, end ? end - begin : strlen(begin),
    -+		     needle.buf, needle.len);
    -+	if (!bod)
    ++	bol = find_commit_header(msg, key, &len);
    ++	if (!bol)
     +		return NULL;
    -+	bod += needle.len;
    -+
    -+	/*
    -+	 * In the commit object, multi-line header values are stored
    -+	 * by prefixing continuation lines begin with a space.  So
    -+	 * within the commit object, it looks like
    -+	 *
    -+	 *     "gpgsig -----BEGIN PGP SIGNATURE-----\n"
    -+	 *     " Version: GnuPG v1.4.5 (GNU/Linux)\n"
    -+	 *     " \n"
    -+	 *     " base64_pem_here\n"
    -+	 *     " -----END PGP SIGNATURE-----\n"
    -+	 *
    -+	 * So we need to look for the first '\n' that *isn't* followed
    -+	 * by a ' ' (or the first '\0', if no such '\n' exists).
    -+	 */
    -+	eod = strchrnul(bod, '\n');
    -+	while (eod[0] == '\n' && eod[1] == ' ') {
    -+		eod = strchrnul(eod+1, '\n');
    ++	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);
     +	}
    -+	*eod = '\0';
    -+
    -+	/*
    -+	 * We now have the value as it's stored in the commit object.
    -+	 * However, we want the raw value; we want to return
    -+	 *
    -+	 *     "-----BEGIN PGP SIGNATURE-----\n"
    -+	 *     "Version: GnuPG v1.4.5 (GNU/Linux)\n"
    -+	 *     "\n"
    -+	 *     "base64_pem_here\n"
    -+	 *     "-----END PGP SIGNATURE-----\n"
    -+	 *
    -+	 * So now we need to strip out all of those extra spaces.
    -+	 */
    -+	while ((eol = strstr(bod, "\n ")))
    -+		memmove(eol+1, eol+2, strlen(eol+1));
    -+
    -+	return bod;
    ++
    ++	*end = eol;
    ++	return val.buf;
     +}
     +
    - static const char *find_encoding(const char *begin, const char *end)
    + static char *reencode_message(const char *in_msg,
    + 			      const char *in_encoding, size_t in_encoding_len)
      {
    - 	const char *needle = "\nencoding ";
     @@ builtin/fast-export.c: static void handle_commit(struct commit *commit, struct rev_info *rev,
    - 	int saved_output_format = rev->diffopt.output_format;
    - 	const char *commit_buffer;
      	const char *author, *author_end, *committer, *committer_end;
    + 	const char *encoding;
    + 	size_t encoding_len;
     +	const char *signature_alg = NULL, *signature;
    - 	const char *encoding, *message;
    + 	const char *message;
      	char *reencoded = NULL;
      	struct commit_list *p;
     @@ builtin/fast-export.c: static void handle_commit(struct commit *commit, struct rev_info *rev,
      	committer++;
    - 	committer_end = strchrnul(committer, '\n');
    - 	message = strstr(committer_end, "\n\n");
    -+	if ((signature = find_signature(committer_end, message, "gpgsig")))
    + 	commit_buffer_cursor = committer_end = strchrnul(committer, '\n');
    + 
    +-	/* find_commit_header() gets a `+ 1` because
    +-	 * commit_buffer_cursor points at the trailing "\n" at the end
    +-	 * of the previous line, but find_commit_header() wants a
    ++	/* find_commit_header() and find_commit_multiline_header() get
    ++	 * a `+ 1` because commit_buffer_cursor points at the trailing
    ++	 * "\n" at the end of the previous line, but they want a
    + 	 * pointer to the beginning of the next line. */
    ++
    + 	encoding = find_commit_header(commit_buffer_cursor + 1, "encoding", &encoding_len);
    + 	if (encoding)
    + 		commit_buffer_cursor = encoding + encoding_len;
    + 
    ++	if ((signature = find_commit_multiline_header(commit_buffer_cursor + 1, "gpgsig", &commit_buffer_cursor)))
     +		signature_alg = "sha1";
    -+	else if ((signature = find_signature(committer_end, message, "gpgsig-sha256")))
    ++	else if ((signature = find_commit_multiline_header(commit_buffer_cursor + 1, "gpgsig-sha256", &commit_buffer_cursor)))
     +		signature_alg = "sha256";
    - 	encoding = find_encoding(committer_end, message);
    ++
    + 	message = strstr(commit_buffer_cursor, "\n\n");
      	if (message)
      		message += 2;
     @@ builtin/fast-export.c: static void handle_commit(struct commit *commit, struct rev_info *rev,
    @@ builtin/fast-export.c: static void handle_commit(struct commit *commit, struct r
     +	if (signature)
     +		switch(signed_commit_mode) {
     +		case SIGN_ABORT:
    -+			die("encountered signed commit %s",
    ++			die("encountered signed commit %s; use "
    ++			    "--signed-commits=<mode> to handle it",
     +			    oid_to_hex(&commit->object.oid));
    -+		case SIGN_VERBATIM_WARN:
    ++		case SIGN_WARN_VERBATIM:
     +			warning("exporting signed commit %s",
     +				oid_to_hex(&commit->object.oid));
     +			/* fallthru */
    @@ builtin/fast-export.c: static void handle_commit(struct commit *commit, struct r
     +			       (unsigned)strlen(signature),
     +			       signature);
     +			break;
    -+		case SIGN_STRIP_WARN:
    -+			warning("stripping signature from commit %s; use"
    -+				"--signed-commits=<mode> to handle it differently",
    ++		case SIGN_WARN_STRIP:
    ++			warning("stripping signature from commit %s",
     +				oid_to_hex(&commit->object.oid));
     +			/* fallthru */
     +		case SIGN_STRIP:
     +			break;
     +		}
      	if (!reencoded && encoding)
    - 		printf("encoding %s\n", encoding);
    + 		printf("encoding %.*s\n", (int)encoding_len, encoding);
      	printf("data %u\n%s",
     @@ builtin/fast-export.c: static void handle_tag(const char *name, struct tag *tag)
      					       "\n-----BEGIN PGP SIGNATURE-----\n");
    @@ builtin/fast-export.c: static void handle_tag(const char *name, struct tag *tag)
      				die("encountered signed tag %s; use "
      				    "--signed-tags=<mode> to handle it",
      				    oid_to_hex(&tag->object.oid));
    --			case WARN:
    -+			case SIGN_VERBATIM_WARN:
    +-			case WARN_VERBATIM:
    ++			case SIGN_WARN_VERBATIM:
      				warning("exporting signed tag %s",
      					oid_to_hex(&tag->object.oid));
      				/* fallthru */
    @@ builtin/fast-export.c: static void handle_tag(const char *name, struct tag *tag)
     +			case SIGN_VERBATIM:
      				break;
     -			case WARN_STRIP:
    -+			case SIGN_STRIP_WARN:
    ++			case SIGN_WARN_STRIP:
      				warning("stripping signature from tag %s",
      					oid_to_hex(&tag->object.oid));
      				/* fallthru */
    @@ builtin/fast-export.c: static void handle_tag(const char *name, struct tag *tag)
      				message_size = signature + 1 - message;
      				break;
      			}
    +@@ builtin/fast-export.c: static int parse_opt_anonymize_map(const struct option *opt,
    + 
    + int cmd_fast_export(int argc, const char **argv, const char *prefix)
    + {
    ++	const char *env_signed_commits_noabort;
    + 	struct rev_info revs;
    + 	struct object_array commits = OBJECT_ARRAY_INIT;
    + 	struct commit *commit;
     @@ builtin/fast-export.c: int cmd_fast_export(int argc, const char **argv, const char *prefix)
      			    N_("show progress after <n> objects")),
      		OPT_CALLBACK(0, "signed-tags", &signed_tag_mode, N_("mode"),
    @@ builtin/fast-export.c: int cmd_fast_export(int argc, const char **argv, const ch
      		OPT_CALLBACK(0, "tag-of-filtered-object", &tag_of_filtered_mode, N_("mode"),
      			     N_("select handling of tags that tag filtered objects"),
      			     parse_opt_tag_of_filtered_mode),
    +@@ builtin/fast-export.c: int cmd_fast_export(int argc, const char **argv, const char *prefix)
    + 	if (argc == 1)
    + 		usage_with_options (fast_export_usage, options);
    + 
    ++	env_signed_commits_noabort = getenv("FAST_EXPORT_SIGNED_COMMITS_NOABORT");
    ++	if (env_signed_commits_noabort && *env_signed_commits_noabort)
    ++		signed_commit_mode = SIGN_WARN_STRIP;
    ++
    + 	/* we handle encodings */
    + 	git_config(git_default_config, NULL);
    + 
     
      ## builtin/fast-import.c ##
     @@ builtin/fast-import.c: static struct hash_list *parse_merge(unsigned int *count)
    @@ t/t9350-fast-export.sh: test_expect_success 'signed-tags=warn-strip' '
     +
     +'
     +
    ++test_expect_success GPG 'signed-commits default' '
    ++
    ++	unset FAST_EXPORT_SIGNED_COMMITS_NOABORT &&
    ++	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))
    ++
    ++'
    ++
     +test_expect_success GPG 'signed-commits=abort' '
     +
     +	test_must_fail git fast-export --signed-commits=abort commit-signing
-- 
2.31.1

Happy hacking,
~ Luke Shumaker

[PATCH v4 1/5] git-fast-import.txt: add missing LF in the BNF

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---

Notes:
    v2: no changes
    v3: no changes
    v4: no changes

 Documentation/git-fast-import.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 39cfa05b28..458af0a2d6 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -437,7 +437,7 @@ change to the project.
 	original-oid?
 	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
 	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
-	('encoding' SP <encoding>)?
+	('encoding' SP <encoding> LF)?
 	data
 	('from' SP <commit-ish> LF)?
 	('merge' SP <commit-ish> LF)*
-- 
2.31.1

[PATCH v4 2/5] fast-export: rename --signed-tags='warn' to 'warn-verbatim'

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

The --signed-tags= option takes one of five arguments specifying how to
handle signed tags during export.  Among these arguments, 'strip' is to
'warn-strip' as 'verbatim' is to 'warn' (the unmentioned argument is
'abort', which stops the fast-export process entirely).  That is,
signatures are either stripped or copied verbatim while exporting, with
or without a warning.

Match the pattern and rename 'warn' to 'warn-verbatim' to make it clear
that it instructs fast-export to copy signatures verbatim.

To maintain backwards compatibility, 'warn' is still recognized as
deprecated synonym of 'warn-verbatim'.

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---

Notes:
    v2:
     - Reword commit message based on feedback from Taylor.
     - Fix copy-pasto in the test, noticed by Taylor.
     - Add a comment to the tests.
     - Fix whitespace in the tests.
    v3:
     - Document that --signed-tags='warn' is a deprecated synonym for
       --signed-tags='warn-verbatim', rather than leaving it
       undocumented, based on feedback from Eric.
    v4:
     - Don't give the "deprecated synonym" mention in the docs its own
       paragraph.
     - Don't just rename the user-facing string, also rename the internal
       enum item from WARN to WARN_VERBATIM.

 Documentation/git-fast-export.txt |  6 +++---
 builtin/fast-export.c             |  8 ++++----
 t/t9350-fast-export.sh            | 18 ++++++++++++++++++
 3 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index 1978dbdc6a..593be7e9a2 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -27,7 +27,7 @@ OPTIONS
 	Insert 'progress' statements every <n> objects, to be shown by
 	'git fast-import' during import.
 
---signed-tags=(verbatim|warn|warn-strip|strip|abort)::
+--signed-tags=(verbatim|warn-verbatim|warn-strip|strip|abort)::
 	Specify how to handle signed tags.  Since any transformation
 	after the export can change the tag names (which can also happen
 	when excluding revisions) the signatures will not match.
@@ -36,8 +36,8 @@ When asking to 'abort' (which is the default), this program will die
 when encountering a signed tag.  With 'strip', the tags will silently
 be made unsigned, with 'warn-strip' they will be made unsigned but a
 warning will be displayed, with 'verbatim', they will be silently
-exported and with 'warn', they will be exported, but you will see a
-warning.
+exported and with 'warn-verbatim' (or 'warn', a deprecated synonym),
+they will be exported, but you will see a warning.
 
 --tag-of-filtered-object=(abort|drop|rewrite)::
 	Specify how to handle tags whose tagged object is filtered out.
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 85a76e0ef8..d1cb8a3183 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -31,7 +31,7 @@ static const char *fast_export_usage[] = {
 };
 
 static int progress;
-static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
+static enum { SIGNED_TAG_ABORT, VERBATIM, WARN_VERBATIM, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
 static enum { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
 static enum { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
 static int fake_missing_tagger;
@@ -55,8 +55,8 @@ static int parse_opt_signed_tag_mode(const struct option *opt,
 		signed_tag_mode = SIGNED_TAG_ABORT;
 	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
 		signed_tag_mode = VERBATIM;
-	else if (!strcmp(arg, "warn"))
-		signed_tag_mode = WARN;
+	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
+		signed_tag_mode = WARN_VERBATIM;
 	else if (!strcmp(arg, "warn-strip"))
 		signed_tag_mode = WARN_STRIP;
 	else if (!strcmp(arg, "strip"))
@@ -834,7 +834,7 @@ static void handle_tag(const char *name, struct tag *tag)
 				die("encountered signed tag %s; use "
 				    "--signed-tags=<mode> to handle it",
 				    oid_to_hex(&tag->object.oid));
-			case WARN:
+			case WARN_VERBATIM:
 				warning("exporting signed tag %s",
 					oid_to_hex(&tag->object.oid));
 				/* fallthru */
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 409b48e244..892737439b 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -253,6 +253,24 @@ test_expect_success 'signed-tags=verbatim' '
 
 '
 
+test_expect_success 'signed-tags=warn-verbatim' '
+
+	git fast-export --signed-tags=warn-verbatim sign-your-name >output 2>err &&
+	grep PGP output &&
+	test -s err
+
+'
+
+# 'warn' is an backward-compatibility alias for 'warn-verbatim'; test
+# that it keeps working.
+test_expect_success 'signed-tags=warn' '
+
+	git fast-export --signed-tags=warn sign-your-name >output 2>err &&
+	grep PGP output &&
+	test -s err
+
+'
+
 test_expect_success 'signed-tags=strip' '
 
 	git fast-export --signed-tags=strip sign-your-name > output &&
-- 
2.31.1

[PATCH v4 3/5] git-fast-export.txt: clarify why 'verbatim' may not be a good idea

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---

Notes:
    v4: This commit is new in v4.

 Documentation/git-fast-export.txt | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index 593be7e9a2..a364812d9f 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -29,15 +29,19 @@ OPTIONS
 
 --signed-tags=(verbatim|warn-verbatim|warn-strip|strip|abort)::
 	Specify how to handle signed tags.  Since any transformation
-	after the export can change the tag names (which can also happen
-	when excluding revisions) the signatures will not match.
+	after the export (or during the export, such as excluding
+	revisions) can change the hashes being signed, the signatures
+	may become invalid.
 +
 When asking to 'abort' (which is the default), this program will die
 when encountering a signed tag.  With 'strip', the tags will silently
 be made unsigned, with 'warn-strip' they will be made unsigned but a
 warning will be displayed, with 'verbatim', they will be silently
 exported and with 'warn-verbatim' (or 'warn', a deprecated synonym),
-they will be exported, but you will see a warning.
+they will be exported, but you will see a warning.  'verbatim' and
+'warn-verbatim' should only be used if you know that no
+transformations affecting tags will be performed, or if you do not
+care that the resulting tag will have an invalid signature.
 
 --tag-of-filtered-object=(abort|drop|rewrite)::
 	Specify how to handle tags whose tagged object is filtered out.
-- 
2.31.1

[PATCH v4 4/5] fast-export: do not modify memory from get_commit_buffer

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

fast-export's helper function find_encoding() takes a `const char *`, but
modifies that memory despite the `const`.  Ultimately, this memory came
from get_commit_buffer(), and you're not supposed to modify the memory
that you get from get_commit_buffer().

So, get rid of find_encoding() in favor of commit.h:find_commit_header(),
which gives back a string length, rather than mutating the memory to
insert a '\0' terminator.

Because find_commit_header() detects the "\n\n" string that separates the
headers and the commit message, move the call to be above the
`message = strstr(..., "\n\n")` call.  This helps readability, and allows
for the value of `encoding` to be used for a better value of "..." so that
the same memory doesn't need to be checked twice.  Introduce a
`commit_buffer_cursor` variable to avoid writing an awkward
`encoding ? encoding + encoding_len : committer_end` expression.

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---

Notes:
    v4: This commit is new in v4.

 builtin/fast-export.c | 65 ++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 28 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index d1cb8a3183..81f3fb1f05 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -499,21 +499,6 @@ static void show_filemodify(struct diff_queue_struct *q,
 	}
 }
 
-static const char *find_encoding(const char *begin, const char *end)
-{
-	const char *needle = "\nencoding ";
-	char *bol, *eol;
-
-	bol = memmem(begin, end ? end - begin : strlen(begin),
-		     needle, strlen(needle));
-	if (!bol)
-		return NULL;
-	bol += strlen(needle);
-	eol = strchrnul(bol, '\n');
-	*eol = '\0';
-	return bol;
-}
-
 static char *anonymize_ref_component(void *data)
 {
 	static int counter;
@@ -615,13 +600,26 @@ static void anonymize_ident_line(const char **beg, const char **end)
 	*end = out->buf + out->len;
 }
 
+static char *reencode_message(const char *in_msg,
+			      const char *in_encoding, size_t in_encoding_len)
+{
+	static struct strbuf in_encoding_buf = STRBUF_INIT;
+
+	strbuf_reset(&in_encoding_buf);
+	strbuf_add(&in_encoding_buf, in_encoding, in_encoding_len);
+
+	return reencode_string(in_msg, "UTF-8", in_encoding_buf.buf);
+}
+
 static void handle_commit(struct commit *commit, struct rev_info *rev,
 			  struct string_list *paths_of_changed_objects)
 {
 	int saved_output_format = rev->diffopt.output_format;
-	const char *commit_buffer;
+	const char *commit_buffer, *commit_buffer_cursor;
 	const char *author, *author_end, *committer, *committer_end;
-	const char *encoding, *message;
+	const char *encoding;
+	size_t encoding_len;
+	const char *message;
 	char *reencoded = NULL;
 	struct commit_list *p;
 	const char *refname;
@@ -630,21 +628,31 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
 	rev->diffopt.output_format = DIFF_FORMAT_CALLBACK;
 
 	parse_commit_or_die(commit);
-	commit_buffer = get_commit_buffer(commit, NULL);
-	author = strstr(commit_buffer, "\nauthor ");
+	commit_buffer_cursor = commit_buffer = get_commit_buffer(commit, NULL);
+
+	author = strstr(commit_buffer_cursor, "\nauthor ");
 	if (!author)
 		die("could not find author in commit %s",
 		    oid_to_hex(&commit->object.oid));
 	author++;
-	author_end = strchrnul(author, '\n');
-	committer = strstr(author_end, "\ncommitter ");
+	commit_buffer_cursor = author_end = strchrnul(author, '\n');
+
+	committer = strstr(commit_buffer_cursor, "\ncommitter ");
 	if (!committer)
 		die("could not find committer in commit %s",
 		    oid_to_hex(&commit->object.oid));
 	committer++;
-	committer_end = strchrnul(committer, '\n');
-	message = strstr(committer_end, "\n\n");
-	encoding = find_encoding(committer_end, message);
+	commit_buffer_cursor = committer_end = strchrnul(committer, '\n');
+
+	/* find_commit_header() gets a `+ 1` because
+	 * commit_buffer_cursor points at the trailing "\n" at the end
+	 * of the previous line, but find_commit_header() wants a
+	 * pointer to the beginning of the next line. */
+	encoding = find_commit_header(commit_buffer_cursor + 1, "encoding", &encoding_len);
+	if (encoding)
+		commit_buffer_cursor = encoding + encoding_len;
+
+	message = strstr(commit_buffer_cursor, "\n\n");
 	if (message)
 		message += 2;
 
@@ -685,14 +693,15 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
 	} else if (encoding) {
 		switch(reencode_mode) {
 		case REENCODE_YES:
-			reencoded = reencode_string(message, "UTF-8", encoding);
+			reencoded = reencode_message(message, encoding, encoding_len);
 			break;
 		case REENCODE_NO:
 			break;
 		case REENCODE_ABORT:
-			die("Encountered commit-specific encoding %s in commit "
+			die("Encountered commit-specific encoding %.*s in commit "
 			    "%s; use --reencode=[yes|no] to handle it",
-			    encoding, oid_to_hex(&commit->object.oid));
+			    (int)encoding_len, encoding,
+			    oid_to_hex(&commit->object.oid));
 		}
 	}
 	if (!commit->parents)
@@ -704,7 +713,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
 	       (int)(author_end - author), author,
 	       (int)(committer_end - committer), committer);
 	if (!reencoded && encoding)
-		printf("encoding %s\n", encoding);
+		printf("encoding %.*s\n", (int)encoding_len, encoding);
 	printf("data %u\n%s",
 	       (unsigned)(reencoded
 			  ? strlen(reencoded) : message
-- 
2.31.1

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

[Luke Shumaker]

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

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---

Notes:
    v2:
     - Remove erroneous remark about ordering from the commit message.
     - Adjust the stream syntax to include the hash algorithm, as
       suggested by brian.
     - Add support for sha256 (based on lots of useful information from
       brian).  It does not support multiply-signed commits.
     - Shorten the documentation, based on feedback from Taylor.
     - Add comments, based on feedback from Taylor.
     - Change the default from `--signed-commits=strip` to
       `--signed-commits=warn-strip`.  This shouldn't break anyone, and
       means that users get useful feedback by default.
    v3: no changes
    v4:
     - Reword the commit message based on feedback from Junio.
     - v1-v3 renamed enum items to SIGN_VERBATIM_WARN and SIGN_STRIP_WARN,
       rename them to SIGN_WARN_VERBATIM and SIGN_WARN_STRIP instead.
     - Rewrite find_signature() as find_commit_multiline_header().  Don't
       have it butcher the memory that we pass to it; have it return its
       own buffer.
     - Change the default from `--signed-commits=warn-strip` to
       `--signed-commits=abort`, to match `--signed-tags`.
     - Add a FAST_EXPORT_SIGNED_COMMITS_NOABORT=1 env-var to change the
       default to `--signed-commits=warn-strip`.

 Documentation/git-fast-export.txt |  11 +++
 Documentation/git-fast-import.txt |  18 +++++
 builtin/fast-export.c             | 120 +++++++++++++++++++++++++-----
 builtin/fast-import.c             |  23 ++++++
 t/t9350-fast-export.sh            |  86 +++++++++++++++++++++
 5 files changed, 240 insertions(+), 18 deletions(-)

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index a364812d9f..7a946e2ede 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -43,6 +43,17 @@ they will be exported, but you will see a warning.  'verbatim' and
 transformations affecting tags will be performed, or if you do not
 care that the resulting tag will have an invalid signature.
 
+--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::
+	Specify how to handle signed commits.  Behaves exactly as
+	'--signed-tags', but for commits.
++
+Earlier versions this command that did not have '--signed-commits'
+behaved as if '--signed-commits=strip'.  As an escape hatch for users
+of tools that call 'git fast-export' but do not yet support
+'--signed-commits', you may set the environment variable
+'FAST_EXPORT_SIGNED_COMMITS_NOABORT=1' in order to change the default
+from 'abort' to 'warn-strip'.
+
 --tag-of-filtered-object=(abort|drop|rewrite)::
 	Specify how to handle tags whose tagged object is filtered out.
 	Since revisions and files to export can be limited by path,
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 458af0a2d6..4955c94305 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -431,12 +431,21 @@ and control the current import process.  More detailed discussion
 Create or update a branch with a new commit, recording one logical
 change to the project.
 
+////
+Yes, it's intentional that the 'gpgsig' line doesn't have a trailing
+`LF`; the the definition of `data` has a byte-count prefix, so it
+doesn't need an `LF` to act as a terminator (and `data` also already
+includes an optional trailing `LF?` just in case you want to include
+one).
+////
+
 ....
 	'commit' SP <ref> LF
 	mark?
 	original-oid?
 	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
 	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
+	('gpgsig' SP <alg> LF data)?
 	('encoding' SP <encoding> LF)?
 	data
 	('from' SP <commit-ish> LF)?
@@ -505,6 +514,15 @@ that was selected by the --date-format=<fmt> command-line option.
 See ``Date Formats'' above for the set of supported formats, and
 their syntax.
 
+`gpgsig`
+^^^^^^^^
+
+The optional `gpgsig` command is used to include a PGP/GPG signature
+that signs the commit data.
+
+Here <alg> specifies which hashing algorithm is used for this
+signature, either `sha1` or `sha256`.
+
 `encoding`
 ^^^^^^^^^^
 The optional `encoding` command indicates the encoding of the commit
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 81f3fb1f05..075630f185 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -30,8 +30,11 @@ static const char *fast_export_usage[] = {
 	NULL
 };
 
+enum sign_mode { SIGN_ABORT, SIGN_VERBATIM, SIGN_STRIP, SIGN_WARN_VERBATIM, SIGN_WARN_STRIP };
+
 static int progress;
-static enum { SIGNED_TAG_ABORT, VERBATIM, WARN_VERBATIM, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
+static enum sign_mode signed_tag_mode = SIGN_ABORT;
+static enum sign_mode signed_commit_mode = SIGN_ABORT;
 static enum { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
 static enum { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
 static int fake_missing_tagger;
@@ -48,21 +51,24 @@ static int anonymize;
 static struct hashmap anonymized_seeds;
 static struct revision_sources revision_sources;
 
-static int parse_opt_signed_tag_mode(const struct option *opt,
+static int parse_opt_sign_mode(const struct option *opt,
 				     const char *arg, int unset)
 {
-	if (unset || !strcmp(arg, "abort"))
-		signed_tag_mode = SIGNED_TAG_ABORT;
+	enum sign_mode *valptr = opt->value;
+	if (unset)
+		return 0;
+	else if (!strcmp(arg, "abort"))
+		*valptr = SIGN_ABORT;
 	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
-		signed_tag_mode = VERBATIM;
+		*valptr = SIGN_VERBATIM;
 	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
-		signed_tag_mode = WARN_VERBATIM;
+		*valptr = SIGN_WARN_VERBATIM;
 	else if (!strcmp(arg, "warn-strip"))
-		signed_tag_mode = WARN_STRIP;
+		*valptr = SIGN_WARN_STRIP;
 	else if (!strcmp(arg, "strip"))
-		signed_tag_mode = STRIP;
+		*valptr = SIGN_STRIP;
 	else
-		return error("Unknown signed-tags mode: %s", arg);
+		return error("Unknown %s mode: %s", opt->long_name, arg);
 	return 0;
 }
 
@@ -600,6 +606,46 @@ static void anonymize_ident_line(const char **beg, const char **end)
 	*end = out->buf + out->len;
 }
 
+/*
+ * find_commit_multiline_header is similar to find_commit_header,
+ * except that it handles multi-line headers, rathar than simply
+ * returning the first line of the header.
+ *
+ * The returned string has had the ' ' line continuation markers
+ * removed, and points to staticly allocated memory (not to memory
+ * within 'msg'), so it is only valid until the next call to
+ * find_commit_multiline_header.
+ *
+ * If the header is found, then *end is set to point at the '\n' in
+ * msg that immediately follows the header value.
+ */
+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;
+}
+
 static char *reencode_message(const char *in_msg,
 			      const char *in_encoding, size_t in_encoding_len)
 {
@@ -619,6 +665,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
 	const char *author, *author_end, *committer, *committer_end;
 	const char *encoding;
 	size_t encoding_len;
+	const char *signature_alg = NULL, *signature;
 	const char *message;
 	char *reencoded = NULL;
 	struct commit_list *p;
@@ -644,14 +691,20 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
 	committer++;
 	commit_buffer_cursor = committer_end = strchrnul(committer, '\n');
 
-	/* find_commit_header() gets a `+ 1` because
-	 * commit_buffer_cursor points at the trailing "\n" at the end
-	 * of the previous line, but find_commit_header() wants a
+	/* find_commit_header() and find_commit_multiline_header() get
+	 * a `+ 1` because commit_buffer_cursor points at the trailing
+	 * "\n" at the end of the previous line, but they want a
 	 * pointer to the beginning of the next line. */
+
 	encoding = find_commit_header(commit_buffer_cursor + 1, "encoding", &encoding_len);
 	if (encoding)
 		commit_buffer_cursor = encoding + encoding_len;
 
+	if ((signature = find_commit_multiline_header(commit_buffer_cursor + 1, "gpgsig", &commit_buffer_cursor)))
+		signature_alg = "sha1";
+	else if ((signature = find_commit_multiline_header(commit_buffer_cursor + 1, "gpgsig-sha256", &commit_buffer_cursor)))
+		signature_alg = "sha256";
+
 	message = strstr(commit_buffer_cursor, "\n\n");
 	if (message)
 		message += 2;
@@ -712,6 +765,29 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
 	printf("%.*s\n%.*s\n",
 	       (int)(author_end - author), author,
 	       (int)(committer_end - committer), committer);
+	if (signature)
+		switch(signed_commit_mode) {
+		case SIGN_ABORT:
+			die("encountered signed commit %s; use "
+			    "--signed-commits=<mode> to handle it",
+			    oid_to_hex(&commit->object.oid));
+		case SIGN_WARN_VERBATIM:
+			warning("exporting signed commit %s",
+				oid_to_hex(&commit->object.oid));
+			/* fallthru */
+		case SIGN_VERBATIM:
+			printf("gpgsig %s\ndata %u\n%s",
+			       signature_alg,
+			       (unsigned)strlen(signature),
+			       signature);
+			break;
+		case SIGN_WARN_STRIP:
+			warning("stripping signature from commit %s",
+				oid_to_hex(&commit->object.oid));
+			/* fallthru */
+		case SIGN_STRIP:
+			break;
+		}
 	if (!reencoded && encoding)
 		printf("encoding %.*s\n", (int)encoding_len, encoding);
 	printf("data %u\n%s",
@@ -839,21 +915,21 @@ static void handle_tag(const char *name, struct tag *tag)
 					       "\n-----BEGIN PGP SIGNATURE-----\n");
 		if (signature)
 			switch(signed_tag_mode) {
-			case SIGNED_TAG_ABORT:
+			case SIGN_ABORT:
 				die("encountered signed tag %s; use "
 				    "--signed-tags=<mode> to handle it",
 				    oid_to_hex(&tag->object.oid));
-			case WARN_VERBATIM:
+			case SIGN_WARN_VERBATIM:
 				warning("exporting signed tag %s",
 					oid_to_hex(&tag->object.oid));
 				/* fallthru */
-			case VERBATIM:
+			case SIGN_VERBATIM:
 				break;
-			case WARN_STRIP:
+			case SIGN_WARN_STRIP:
 				warning("stripping signature from tag %s",
 					oid_to_hex(&tag->object.oid));
 				/* fallthru */
-			case STRIP:
+			case SIGN_STRIP:
 				message_size = signature + 1 - message;
 				break;
 			}
@@ -1192,6 +1268,7 @@ static int parse_opt_anonymize_map(const struct option *opt,
 
 int cmd_fast_export(int argc, const char **argv, const char *prefix)
 {
+	const char *env_signed_commits_noabort;
 	struct rev_info revs;
 	struct object_array commits = OBJECT_ARRAY_INIT;
 	struct commit *commit;
@@ -1206,7 +1283,10 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 			    N_("show progress after <n> objects")),
 		OPT_CALLBACK(0, "signed-tags", &signed_tag_mode, N_("mode"),
 			     N_("select handling of signed tags"),
-			     parse_opt_signed_tag_mode),
+			     parse_opt_sign_mode),
+		OPT_CALLBACK(0, "signed-commits", &signed_commit_mode, N_("mode"),
+			     N_("select handling of signed commits"),
+			     parse_opt_sign_mode),
 		OPT_CALLBACK(0, "tag-of-filtered-object", &tag_of_filtered_mode, N_("mode"),
 			     N_("select handling of tags that tag filtered objects"),
 			     parse_opt_tag_of_filtered_mode),
@@ -1247,6 +1327,10 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 	if (argc == 1)
 		usage_with_options (fast_export_usage, options);
 
+	env_signed_commits_noabort = getenv("FAST_EXPORT_SIGNED_COMMITS_NOABORT");
+	if (env_signed_commits_noabort && *env_signed_commits_noabort)
+		signed_commit_mode = SIGN_WARN_STRIP;
+
 	/* we handle encodings */
 	git_config(git_default_config, NULL);
 
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 3afa81cf9a..ee7516dd38 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -2669,10 +2669,13 @@ static struct hash_list *parse_merge(unsigned int *count)
 
 static void parse_new_commit(const char *arg)
 {
+	static struct strbuf sig = STRBUF_INIT;
 	static struct strbuf msg = STRBUF_INIT;
+	struct string_list siglines = STRING_LIST_INIT_NODUP;
 	struct branch *b;
 	char *author = NULL;
 	char *committer = NULL;
+	char *sig_alg = NULL;
 	char *encoding = NULL;
 	struct hash_list *merge_list = NULL;
 	unsigned int merge_count;
@@ -2696,6 +2699,13 @@ static void parse_new_commit(const char *arg)
 	}
 	if (!committer)
 		die("Expected committer but didn't get one");
+	if (skip_prefix(command_buf.buf, "gpgsig ", &v)) {
+		sig_alg = xstrdup(v);
+		read_next_command();
+		parse_data(&sig, 0, NULL);
+		read_next_command();
+	} else
+		strbuf_setlen(&sig, 0);
 	if (skip_prefix(command_buf.buf, "encoding ", &v)) {
 		encoding = xstrdup(v);
 		read_next_command();
@@ -2769,10 +2779,23 @@ static void parse_new_commit(const char *arg)
 		strbuf_addf(&new_data,
 			"encoding %s\n",
 			encoding);
+	if (sig_alg) {
+		if (!strcmp(sig_alg, "sha1"))
+			strbuf_addstr(&new_data, "gpgsig ");
+		else if (!strcmp(sig_alg, "sha256"))
+			strbuf_addstr(&new_data, "gpgsig-sha256 ");
+		else
+			die("Expected gpgsig algorithm sha1 or sha256, got %s", sig_alg);
+		string_list_split_in_place(&siglines, sig.buf, '\n', -1);
+		strbuf_add_separated_string_list(&new_data, "\n ", &siglines);
+		strbuf_addch(&new_data, '\n');
+	}
 	strbuf_addch(&new_data, '\n');
 	strbuf_addbuf(&new_data, &msg);
+	string_list_clear(&siglines, 1);
 	free(author);
 	free(committer);
+	free(sig_alg);
 	free(encoding);
 
 	if (!store_object(OBJ_COMMIT, &new_data, NULL, &b->oid, next_mark))
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 &&
+	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 &&
+	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))
+
+'
+
+test_expect_success GPG 'signed-commits=abort' '
+
+	test_must_fail git fast-export --signed-commits=abort commit-signing
+
+'
+
+test_expect_success GPG 'signed-commits=verbatim' '
+
+	git fast-export --signed-commits=verbatim --reencode=no commit-signing >output &&
+	grep "^gpgsig sha" output &&
+	grep "encoding ISO-8859-1" output &&
+	(cd new &&
+	 git fast-import &&
+	 test $COMMIT_SIGNING = $(git rev-parse --verify refs/heads/commit-signing)) <output
+
+'
+
+test_expect_success GPG 'signed-commits=warn-verbatim' '
+
+	git fast-export --signed-commits=warn-verbatim --reencode=no commit-signing >output 2>err &&
+	grep "^gpgsig sha" output &&
+	grep "encoding ISO-8859-1" output &&
+	test -s err &&
+	(cd new &&
+	 git fast-import &&
+	 test $COMMIT_SIGNING = $(git rev-parse --verify refs/heads/commit-signing)) <output
+
+'
+
+test_expect_success GPG 'signed-commits=strip' '
+
+	git fast-export --signed-commits=strip --reencode=no commit-signing >output &&
+	! grep ^gpgsig output &&
+	grep "^encoding ISO-8859-1" output &&
+	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))
+
+'
+
+test_expect_success GPG 'signed-commits=warn-strip' '
+
+	git fast-export --signed-commits=warn-strip --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))
+
+'
+
 test_expect_success 'setup submodule' '
 
 	git checkout -f main &&
+	{ git update-ref -d refs/heads/commit-signing || true; } &&
 	mkdir sub &&
 	(
 		cd sub &&
-- 
2.31.1

Re: [PATCH v4 4/5] fast-export: do not modify memory from get_commit_buffer

[Junio C Hamano]

Luke Shumaker <lukeshu@lukeshu.com> writes:

> +static char *reencode_message(const char *in_msg,
> +			      const char *in_encoding, size_t in_encoding_len)
> +{
> +	static struct strbuf in_encoding_buf = STRBUF_INIT;
> +
> +	strbuf_reset(&in_encoding_buf);
> +	strbuf_add(&in_encoding_buf, in_encoding, in_encoding_len);
> +
> +	return reencode_string(in_msg, "UTF-8", in_encoding_buf.buf);
> +}

There is only a single caller of this, so making it caller's
responsibility to do the strbuf thing would allow us to make this
thread-safe quite easily (and at that point we might not even have
this helper function).

> +	committer = strstr(commit_buffer_cursor, "\ncommitter ");
>  	if (!committer)
>  		die("could not find committer in commit %s",
>  		    oid_to_hex(&commit->object.oid));
>  	committer++;
> -	committer_end = strchrnul(committer, '\n');
> -	message = strstr(committer_end, "\n\n");
> -	encoding = find_encoding(committer_end, message);
> +	commit_buffer_cursor = committer_end = strchrnul(committer, '\n');
> +
> +	/* find_commit_header() gets a `+ 1` because
> +	 * commit_buffer_cursor points at the trailing "\n" at the end
> +	 * of the previous line, but find_commit_header() wants a
> +	 * pointer to the beginning of the next line. */
> +	encoding = find_commit_header(commit_buffer_cursor + 1, "encoding", &encoding_len);

	/*
	 * Our multi-line comments have opening and closing
	 * slash-asterisk and asterisk-slash on their own
	 * lines.
	 */

What if strchrnul() returned a pointer to the terminating NUL
instead of the LF at the end of the line?  +1 will run past the end
of the buffer.

> +	if (encoding)
> +		commit_buffer_cursor = encoding + encoding_len;
> +
> +	message = strstr(commit_buffer_cursor, "\n\n");

Good.

> @@ -685,14 +693,15 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
>  	} else if (encoding) {
>  		switch(reencode_mode) {
>  		case REENCODE_YES:
> -			reencoded = reencode_string(message, "UTF-8", encoding);
> +			reencoded = reencode_message(message, encoding, encoding_len);
>  			break;

Here is where we can do the temporary strbuf to hold encoding[0,
encoding_len] and directly call reencode_string().

Other than that, this step looks good to me.

Thanks.

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

[Junio C Hamano]

Luke Shumaker <lukeshu@lukeshu.com> writes:

> 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/;

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

> +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().

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

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

> +	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 &&