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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 28, 2021, 03:29 UTC
Message-ID: <xmqqpmyfccjb.fsf@gitster.g>
In-Reply-To: <20210423164118.693197-3-lukeshu@lukeshu.com>

Luke Shumaker <lukeshu@lukeshu.com> writes:

Show 15 quoted lines

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

Show 13 quoted lines

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

Show 13 quoted lines

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