diff mbox series

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

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

Commit Message

Luke Shumaker April 22, 2021, 12:27 a.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
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(-)

Comments

Eric Sunshine April 22, 2021, 3:59 a.m. UTC | #1 
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`.
Luke Shumaker April 22, 2021, 4:43 a.m. UTC | #2 
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.
Luke Shumaker April 22, 2021, 4:50 a.m. UTC | #3 
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.
diff mbox series

Patch

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