(Sorry for the late answer to this.)
On Wed, May 28, 2025 at 7:29 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
> > Christian Couder <christian.couder@xxxxxxxxx> 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?
Thanks for this. I agree that it's the best approach.
> ---- >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.
Yeah, right.
> 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.
>
> As we tone down the default behaviour, we no longer need the
> FAST_EXPORT_SIGNED_COMMITS_NOABORT environment variable, which was
> not discoverable enough.
>
> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
> ---
> 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.
I wonder if it should say that the default is likely to change too?
> --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' '
Here also maybe we should say that the default could change in case
advanced users look at test cases to get hints at what is cast in
stone?
> + 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
> '
Otherwise the patch looks good to me.
