Remote helpers and signed tags

It appears to be impossible to push signed tags using a remote helper that supports only fast-export. This is reported against gitifyhg[1] but I think it is actually a Git issue.

[1] https://github.com/buchuki/gitifyhg/issues/59

I can reproduce the error using a trivial remote helper (run this in a clone of git.git):

-- >8 --
cat >git-remote-export <<EOF &&
#!/bin/sh

alias=$1
url=${2-$1}

while read -r line
do

	case "$line" in
	capabilities)
		echo 'export'
		echo 'refspec *:*'
		echo
		;;
	list)
		echo
		;;
	export)
		while read -r line
		do
			echo "$line" >&3
			test "$line" = done && break
		done 3>"$url"
		echo
		;;
	'')
		exit
		;;
	*)
		echo >&2 "unsupported command: $line"
		exit 1
		;;
	esac

done
EOF
chmod +x git-remote-export &&
PATH="$(pwd):$PATH" git push "export::$(pwd)/v1.8.2.export" v1.8.2 -- 8< --

This produces:

fatal: Encountered signed tag 572a535454612a046e7dd7404dcca94d6243c788;
use --signed-tag=<mode> to handle it.
fatal: Error while running fast-export

which is not particularly helpful for a user who doesn't know how the remote helper is implemented, particularly because adding --signed-tag=<mode> to the command won't work.

I think there are two problems here:

1) The error message is misleading: "--signed-tag" isn't an option to git-push and as a user I don't know why Git thought I wanted to run fast-export.

2) There is no way (that I have found) to change the signed-tag behaviour of git-fast-export when it is being invoked for a remote helper.

How do remote helpers using the "push" method handle this? In that case it seems to be completely up to the helper program to decide what to do.

I wonder if the way forward is to do some combination of:

a) Add a --signed-tags option to git-push, which is either passed to fast-export or given as a new "option signed-tags" to the remote-helper when using the push interface (and ignored for the connect interface).

b) Add a configuration variable to specify how to handle signed tags when pushing to a remote that uses a remote helper that cannot handle them; something like "remote.<name>.signedTags".

c) Improve the "Error while running fast-export" message to something more like "Error pushing with fast-export (using helper git-remote-foo)".

Hi John,

[...]

Yeah, this is idiotic.

The remote helper infrastructure is certainly being unhelpful here. I wonder if transport-helper should just pass --signed-tag=strip and be done with it (leaving open the possibility of a capability to switch to --signed-tag=verbatim when someone wants to teach the testgit helper to support that). What do you think?

Thanks,
Jonathan

I think that's (at least for now) the right thing to do. Passing anything but signed-tag=strip should be triggered by a capability from the helper, since most helpers won't know how to deal with signed tags.

--
Cheers,

Sverre Rabbelier

I don't like the idea of silently stripping tags, so how about this?

Patch 1 adds a new 'warn-strip' mode to 'fast-export --signed-tags=...' which strips tags but issues a warning when doing so. Then we make transport-helper use that before finally adding a new capability to allow a remote helper to change '--signed-tags=warn-strip' into '--signed-tags=verbatim'.

John Keeping (3):
fast-export: add --signed-tags=warn-strip mode
transport-helper: pass --signed-tags=warn-strip to fast-export
transport-helper: add 'signed-tags' capability

 Documentation/git-fast-export.txt   | 10 ++++++----
 Documentation/gitremote-helpers.txt |  4 ++++
 builtin/fast-export.c               |  8 +++++++-
 git-remote-testgit                  |  1 +
 t/t5801-remote-helpers.sh           | 20 ++++++++++++++++++++
 t/t9350-fast-export.sh              |  6 ++++++
 transport-helper.c                  |  7 ++++++-
 7 files changed, 50 insertions(+), 6 deletions(-)

1.8.2.694.ga76e9c3.dirty

This issues a warning while stripping signatures from signed tags, which allows us to use it as default behaviour for remote helpers which cannot specify how to handle signed tags.

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

index feab7a3..03fc8c3 100644
@@ -27,15 +27,17 @@ OPTIONS
 	Insert 'progress' statements every <n> objects, to be shown by
 	'git fast-import' during import.
 
---signed-tags=(verbatim|warn|strip|abort)::
+--signed-tags=(verbatim|warn|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.
 +
 When asking to 'abort' (which is the default), this program will die
-when encountering a signed tag.  With 'strip', the tags will be made
-unsigned, with 'verbatim', they will be silently exported
-and with 'warn', they will be exported, but you will see a warning.
+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.
 
 --tag-of-filtered-object=(abort|drop|rewrite)::
 	Specify how to handle tags whose tagged object is filtered out.

index 725c0a7..d60d675 100644
@@ -24,7 +24,7 @@ static const char *fast_export_usage[] = {
 };
 
 static int progress;
-static enum { ABORT, VERBATIM, WARN, STRIP } signed_tag_mode = ABORT;
+static enum { ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = ABORT;
 static enum { ERROR, DROP, REWRITE } tag_of_filtered_mode = ERROR;
 static int fake_missing_tagger;
 static int use_done_feature;

@@ -40,6 +40,8 @@ static int parse_opt_signed_tag_mode(const struct option *opt,
 		signed_tag_mode = VERBATIM;
 	else if (!strcmp(arg, "warn"))
 		signed_tag_mode = WARN;
+	else if (!strcmp(arg, "warn-strip"))
+		signed_tag_mode = WARN_STRIP;
 	else if (!strcmp(arg, "strip"))
 		signed_tag_mode = STRIP;
 	else

@@ -428,6 +430,10 @@ static void handle_tag(const char *name, struct tag *tag)
 				/* fallthru */
 			case VERBATIM:
 				break;
+			case WARN_STRIP:
+				warning ("Stripping signature from tag %s",
+					 sha1_to_hex(tag->object.sha1));
+				/* fallthru */
 			case STRIP:
 				message_size = signature + 1 - message;
 				break;

index 9320b4f..2471bc6 100755
@@ -146,6 +146,12 @@ test_expect_success 'signed-tags=strip' '
 
 '
 
+test_expect_success 'signed-tags=warn-strip' '
+	git fast-export --signed-tags=warn-strip sign-your-name >output 2>err &&
+	! grep PGP output &&
+	test -s err
+'
+
 test_expect_success 'setup submodule' '
 
 	git checkout -f master &&

1.8.2.694.ga76e9c3.dirty

Perhaps it makes sense to instead count the number of signed tags and emit "Stripped signature from %d tags"? For example, for git.git it would be on the order of a hundred warning lines.

--
Cheers,

Sverre Rabbelier

When you see 78 in the output and you know you have 92 tags in the repository, is that sufficient to let you go on, or do we also need an easy way to tell which ones are those 78 that were stripped and the remaining 14 were not stripped?

There is no reason to worry about "some signed tags are stripped but not others", so it feels that the number alone should be sufficient, I guess. If those remaining 14 weren't stripped, that is (at least at the moment) by definition because they are unsigned, annotated tags.

I suspect the actually relevant information is

warning: stripping signature before pushing signed tags

The count and the list of signed tags are not too important.

Or because they were not exported? Perhaps "78 tags stripped, 92 exported in total".

--
Cheers,

Sverre Rabbelier

I think I prefer Jonathan's suggestion to this one if we need to change it.

The reason I didn't do this initially was that I assumed that from a remote helper we would, in general, not be pushing any tags which already exist, so the number of tags to push will be small.

Printing one message per tag also matches the current behaviour for --signed-tags=warn. I don't want to make the behaviour for "warn" and "warn-strip" different, so should "warn" also print a summary message instead of a message for each tag?

That is a valid point. Nobody has complained that the current warning is too noisy, so perhaps the patch is good as-is?

What do others think?

Ah, hadn't realized that. Probably fine then.

--
Cheers,

Sverre Rabbelier

Currently, attempting to push a signed tag to a remote helper which uses fast-export results in the remote helper failing because the default fast-export action for signed tags is "abort". This is not helpful for users because there is no way to pass additional arguments to fast-export here, either from the remote helper or from the command line.

In general, the signature will be invalidated by whatever transformation a remote helper performs on a tag to push it to a repository in a different format so the correct behaviour is to strip the tag. Doing this silently may surprise people, so use "warn-strip" to issue a warning when a signed tag is encountered.

---
 t/t5801-remote-helpers.sh | 10 ++++++++++
 transport-helper.c        |  3 ++-
 2 files changed, 12 insertions(+), 1 deletion(-)

index f387027..9b287db 100755
@@ -6,6 +6,7 @@
 test_description='Test remote-helper import and export commands'
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-gpg.sh
 
 if ! type "${BASH-bash}" >/dev/null 2>&1; then
 	skip_all='skipping remote-testgit tests, bash not available'

@@ -166,4 +167,13 @@ test_expect_success 'push ref with existing object' '
 	compare_refs local dup server dup
 '
 
+test_expect_success GPG 'push signed tag' '
+	(cd local &&
+	git checkout master &&
+	git tag -s -m signed-tag signed-tag &&
+	git push origin signed-tag
+	) &&
+	compare_refs local signed-tag^{} server signed-tag^{}
+'
+
 test_done

index dcd8d97..3ce8259 100644
@@ -410,9 +410,10 @@ static int get_exporter(struct transport *transport,
 	/* we need to duplicate helper->in because we want to use it after
 	 * fastexport is done with it. */
 	fastexport->out = dup(helper->in);
-	fastexport->argv = xcalloc(5 + revlist_args->nr, sizeof(*fastexport->argv));
+	fastexport->argv = xcalloc(6 + revlist_args->nr, sizeof(*fastexport->argv));
 	fastexport->argv[argc++] = "fast-export";
 	fastexport->argv[argc++] = "--use-done-feature";
+	fastexport->argv[argc++] = "--signed-tags=warn-strip";
 	if (data->export_marks)
 		fastexport->argv[argc++] = data->export_marks;
 	if (data->import_marks)

1.8.2.694.ga76e9c3.dirty

This allows a remote helper using the 'export' protocol to specify that it supports signed tags, changing the handing from 'warn-strip' to 'verbatim'.

---
 Documentation/gitremote-helpers.txt |  4 ++++
 git-remote-testgit                  |  1 +
 t/t5801-remote-helpers.sh           | 12 +++++++++++-
 transport-helper.c                  |  6 +++++-
 4 files changed, 21 insertions(+), 2 deletions(-)

index f506031..da74641 100644
@@ -202,6 +202,10 @@ capability then it should advertise `refspec *:*`.
 	marks specified in <file> before processing any input. For details,
 	read up on '--import-marks=<file>' in linkgit:git-fast-export[1].
 
+'signed-tags'::
+	This modifies the 'export' capability, instructing Git to pass
+	'--signed-tags=verbatim' to linkgit:git-fast-export[1].  In the
+	absence of this capability, Git will use '--signed-tags=warn-strip'.
 
 
 

index b395c8d..e7ed3a3 100755
@@ -38,6 +38,7 @@ do
 			echo "*import-marks $gitmarks"
 			echo "*export-marks $gitmarks"
 		fi
+		test -n "$GIT_REMOTE_TESTGIT_SIGNED_TAGS" && echo "signed-tags"
 		echo
 		;;
 	list)

index 9b287db..69212cd 100755
@@ -173,7 +173,17 @@ test_expect_success GPG 'push signed tag' '
 	git tag -s -m signed-tag signed-tag &&
 	git push origin signed-tag
 	) &&
-	compare_refs local signed-tag^{} server signed-tag^{}
+	compare_refs local signed-tag^{} server signed-tag^{} &&
+	test_must_fail compare_refs local signed-tag server signed-tag
+'
+
+test_expect_success GPG 'push signed tag with signed-tags capability' '
+	(cd local &&
+	git checkout master &&
+	git tag -s -m signed-tag signed-tag-2 &&
+	GIT_REMOTE_TESTGIT_SIGNED_TAGS=1 git push origin signed-tag-2
+	) &&
+	compare_refs local signed-tag-2 server signed-tag-2
 '
 
 test_done

index 3ce8259..5f8d075 100644
@@ -25,6 +25,7 @@ struct helper_data {
 		option : 1,
 		push : 1,
 		connect : 1,
+		signed_tags : 1,
 		no_disconnect_req : 1;
 	char *export_marks;
 	char *import_marks;

@@ -191,6 +192,8 @@ static struct child_process *get_helper(struct transport *transport)
 			refspecs[refspec_nr++] = xstrdup(capname + strlen("refspec "));
 		} else if (!strcmp(capname, "connect")) {
 			data->connect = 1;
+		} else if (!strcmp(capname, "signed-tags")) {
+			data->signed_tags = 1;
 		} else if (!prefixcmp(capname, "export-marks ")) {
 			struct strbuf arg = STRBUF_INIT;
 			strbuf_addstr(&arg, "--export-marks=");

@@ -413,7 +416,8 @@ static int get_exporter(struct transport *transport,
 	fastexport->argv = xcalloc(6 + revlist_args->nr, sizeof(*fastexport->argv));
 	fastexport->argv[argc++] = "fast-export";
 	fastexport->argv[argc++] = "--use-done-feature";
-	fastexport->argv[argc++] = "--signed-tags=warn-strip";
+	fastexport->argv[argc++] = data->signed_tags ?
+		"--signed-tags=verbatim" : "--signed-tags=warn-strip";
 	if (data->export_marks)
 		fastexport->argv[argc++] = data->export_marks;
 	if (data->import_marks)

1.8.2.694.ga76e9c3.dirty