diff mbox series

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

Message ID 20210423164118.693197-3-lukeshu@lukeshu.com (mailing list archive)
State New, archived
Headers show
Series fast-export, fast-import: implement signed-commits | expand

Commit Message

Luke Shumaker April 23, 2021, 4:41 p.m. UTC 
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(-)

Comments

Junio C Hamano April 28, 2021, 3:29 a.m. UTC | #1 
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.
Luke Shumaker April 29, 2021, 7:02 p.m. UTC | #2 
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.
Junio C Hamano April 30, 2021, 12:03 a.m. UTC | #3 
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.
diff mbox series

Patch

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