Re: [PATCH] fast-(import|export): improve on the signature algorithm name

From: Elijah Newren <newren@gmail.com>
Date: May 28, 2025, 20:06 UTC
Message-ID: <CABPp-BGe6r-X7NOiBFEvLGZH+GQvLkOYWn+qf3ZzzgT4sGAyOg@mail.gmail.com>
In-Reply-To: <xmqq8qmgsky8.fsf@gitster.g>

On Wed, May 28, 2025 at 10:29 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 25 quoted lines

>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Christian Couder <christian.couder@gmail.com> writes:
> >
> >> I agree that we should have at least said in big letters that the
> >> improved support for signed commits in fast-export/import is very
> >> experimental and very likely to change in the future.
> >>
> >> We could still do so. This could give us a bit of time and flexibility
> >> until we agree on and implement something better and backward
> >> compatible. (Hopefully the v2 will help us move forward.)
> >
> > OK, as the next release is approaching, perhaps we do a bit of
> > documentation update to address that "we are experimenting" and
> > nothing else, and leave the v2 updates for the next cycle?
>
> ---- >8 ----
> Subject: [PATCH] fast-export: --signed-commits is experimental
>
> As the design of signature handling is still being discussed, it is
> likely that the data stream produced by the code in Git 2.50 would
> have to be changed in such a way that is not backward compatible.
>
> Mark the feature as experimental and discourge its use for now.

I think this is a very good thing to do.

minor nit: discourge -> discourage

Show 5 quoted lines

> Also flip the default on the generation side to "strip"; users of
> existing versions would not have passed --signed-commits=strip and
> will be broken by this change if the default is made to abort, and
> will be encouraged by the error message to produce data stream with
> future breakage guarantees by passing --signed-commits option.

So...git-filter-repo runs fast-export and has limited flexibility about which options it passes to fast-export under the hood, so this change would save me from the patch I was planning to add to filter-repo. So that's evidence in support of your statement, but the "will be broken" statement appears to me to be incongruent with past deprecations and changes of default that we have gone through. Often when we have deprecated or changed an option our process was to first produce an error and update documentation and wait a while, then go and change the default after a sufficiently long time. Here, we had kind of stopped at just producing the error with no plans to take another step. If that was the route we took in the past, what makes this considered a breakage and not the other changes we made?

(Just curious, I'm not against this change.)

> As we tone down the default behaviour, we no longer need the
> FAST_EXPORT_SIGNED_COMMITS_NOABORT environment variable, which was
> not discoverable enough.

Makes sense.

Show 128 quoted lines

>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/RelNotes/2.50.0.adoc |  4 +++-
>  Documentation/git-fast-export.adoc | 12 +++++-------
>  Documentation/git-fast-import.adoc |  3 +++
>  builtin/fast-export.c              |  7 +------
>  t/t9350-fast-export.sh             | 20 ++++----------------
>  5 files changed, 16 insertions(+), 30 deletions(-)
>
> diff --git a/Documentation/RelNotes/2.50.0.adoc b/Documentation/RelNotes/2.50.0.adoc
> index c6c34d1a1d..9a1cdf0dc0 100644
> --- a/Documentation/RelNotes/2.50.0.adoc
> +++ b/Documentation/RelNotes/2.50.0.adoc
> @@ -100,7 +100,9 @@ Performance, Internal Implementation, Development Support etc.
>   * "git fsck" becomes more careful when checking the refs.
>
>   * "git fast-export | git fast-import" learns to deal with commit and
> -   tag objects with embedded signatures a bit better.
> +   tag objects with embedded signatures a bit better.  This is highly
> +   experimental and the format of the data stream may change in the
> +   future without compatibility guarantees.
>
>   * The code paths to check whether a refname X is available (by seeing
>     if another ref X/Y exists, etc.) have been optimized.
> diff --git a/Documentation/git-fast-export.adoc b/Documentation/git-fast-export.adoc
> index 413a527496..43bbb4f63c 100644
> --- a/Documentation/git-fast-export.adoc
> +++ b/Documentation/git-fast-export.adoc
> @@ -46,14 +46,12 @@ 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.  Default is 'abort'.
> +       '--signed-tags', but for commits.  Default is 'strip', which
> +       is the same as how earlier versions of this command without
> +       this option behaved.
>  +
> -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'.
> +NOTE: This is highly experimental and the format of the data stream may
> +change in the future without compatibility guarantees.
>
>  --tag-of-filtered-object=(abort|drop|rewrite)::
>         Specify how to handle tags whose tagged object is filtered out.
> diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc
> index 7b107f5e8e..250d866652 100644
> --- a/Documentation/git-fast-import.adoc
> +++ b/Documentation/git-fast-import.adoc
> @@ -523,6 +523,9 @@ that signs the commit data.
>  Here <alg> specifies which hashing algorithm is used for this
>  signature, either `sha1` or `sha256`.
>
> +NOTE: This is highly experimental and the format of the data stream may
> +change in the future without compatibility guarantees.
> +
>  `encoding`
>  ^^^^^^^^^^
>  The optional `encoding` command indicates the encoding of the commit
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index 37c01d6c6f..fcf6b00d5f 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -39,7 +39,7 @@ enum sign_mode { SIGN_ABORT, SIGN_VERBATIM, SIGN_STRIP, SIGN_WARN_VERBATIM, SIGN
>
>  static int progress;
>  static enum sign_mode signed_tag_mode = SIGN_ABORT;
> -static enum sign_mode signed_commit_mode = SIGN_ABORT;
> +static enum sign_mode signed_commit_mode = SIGN_STRIP;
>  static enum tag_of_filtered_mode { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
>  static enum reencode_mode { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
>  static int fake_missing_tagger;
> @@ -1269,7 +1269,6 @@ int cmd_fast_export(int argc,
>                     const char *prefix,
>                     struct repository *repo UNUSED)
>  {
> -       const char *env_signed_commits_noabort;
>         struct rev_info revs;
>         struct commit *commit;
>         char *export_filename = NULL,
> @@ -1327,10 +1326,6 @@ int cmd_fast_export(int argc,
>         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/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
> index dda9e7c3e7..76619765fc 100755
> --- a/t/t9350-fast-export.sh
> +++ b/t/t9350-fast-export.sh
> @@ -299,22 +299,10 @@ test_expect_success GPG 'set up signed commit' '
>
>  '
>
> -test_expect_success GPG 'signed-commits default' '
> -
> -       sane_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 &&
> -               STRIPPED=$(git rev-parse --verify refs/heads/commit-strip-signing) &&
> -               test $COMMIT_SIGNING != $STRIPPED
> -       )
> -
> +test_expect_success GPG 'signed-commits default is same as strip' '
> +       git fast-export --reencode=no commit-signing >out1 2>err &&
> +       git fast-export --reencode=no --signed-commits=strip commit-signing >out2 &&
> +       test_cmp out1 out2
>  '
>
>  test_expect_success GPG 'signed-commits=abort' '
> --
> 2.50.0-rc0-134-gb29a910c2a

Looks good to me.