Re: [PATCH 1/4] fast-import: add 'abort-if-invalid' mode to '--signed-commits=<mode>'

[Justin Tobler <jltobler@gmail.com>]

On 26/03/24 03:22PM, Junio C Hamano wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> 
> > diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> > index 13621b0d6a..dcbc5bc82d 100644
> > --- a/builtin/fast-export.c
> > +++ b/builtin/fast-export.c
> > @@ -822,6 +822,9 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
> >  			die(_("encountered signed commit %s; use "
> >  			      "--signed-commits=<mode> to handle it"),
> >  			    oid_to_hex(&commit->object.oid));
> > +		case SIGN_ABORT_IF_INVALID:
> > +			die(_("'abort-if-invalid' is not a valid mode for "
> > +			      "git fast-export with --signed-commits=<mode>"));
> >  		case SIGN_STRIP_IF_INVALID:
> >  			die(_("'strip-if-invalid' is not a valid mode for "
> >  			      "git fast-export with --signed-commits=<mode>"));
> 
> There are a few similar hunks in this patch to fast-export, but I am
> not sure what is going on here.

git-fast-export(1) does not support the `{strip,sign,abort}-if-invalid`
mode for the `--signed-{commits,tags}` options. Therefore we die in
cases where we parse this option with an unsupported mode.

> I may be misreading the code, but this error, and the similar one
> for strip-if-invalid that is already there, trigger if the command
> is given "--signed-commits=abort-if-invalid" on its command line,
> and when we see a signed commit in the range we walk to export the
> commits.  Why shouldn't the user get the error immediately while the
> command is parsing the command line options?  You may be sharing the
> underlying parse_sign_mode() with import side that may support more
> variants, but that is not a good excuse to make these two
> 
>     git fast-export --signed-commits=abort-if-invalid
>     git fast-export --signed-commits=i-dont-know-what-i-am-doing
> 
> behave completely differently, no?

Ya, that is a completely fair point. If the mode is unsupported, it
doesn't make sense to wait until we encounter a signed commit/tag to
declare the user provided an invalid signed mode for the command. 

> You can either move these "no, these subset of options are not
> available here" to fast-export.c::parse_opt_sign_mode(), or even
> better yet, teach parse_sign_mode() an option to say "hey, you are
> being called from fast-export, so pretend that you have never heard
> of options that are only available on fast-import" and error out
> right there, can't you?  Or would it be too _early_ to give errors
> to users?

I think it definately makes sense to move signing mode verification to
be around the same time the option is initially parsed. With this patch
series, the `--signed-commits` and `--signed-tags` options for
git-fast-import(1) will support the same modes. It will only be
git-fast-export(1) that supports a subset of the signing modes. It
should be easy enough to update "fast-export.c:parse_opt_sign_mode()" to
explictly handle the unsupported signing modes upfront.

I'll update in the next version accordingly.

Thanks,
-Justin