On Tue, May 9, 2023 at 10:54 AM Christian Couder
<christian.couder@xxxxxxxxx> wrote:
>
> A previous commit changed `git replay` to make it accept standard
> revision ranges using the setup_revisions() function. While this is good
s/is good/is a good/
> thing to make this command more standard and more flexible, it has the
> downside of enabling many revision related options accepted and eaten by
> setup_revisions().
[...]
> @@ -135,6 +135,20 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
> argc = parse_options(argc, argv, prefix, replay_options, replay_usage,
> PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN_OPT);
>
> + /*
> + * TODO: For now, we reject any unknown or invalid option,
> + * including revision related ones, like --not,
> + * --first-parent, etc that would be allowed and eaten by
> + * setup_revisions() below. In the future we should definitely
> + * accept those that make sense and add related tests and doc
> + * though.
> + */
> + for (i = 0; i < argc; i++)
> + if (argv[i][0] == '-') {
> + error(_("invalid option: %s"), argv[i]);
> + usage_with_options(replay_usage, replay_options);
> + }
> +
> if (!onto_name) {
> error(_("option --onto is mandatory"));
> usage_with_options(replay_usage, replay_options);
> @@ -150,6 +164,17 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
> goto cleanup;
> }
>
> + /*
> + * TODO: For now, we reject any pathspec. (They are allowed
> + * and eaten by setup_revisions() above.) In the future we
> + * should definitely accept them and add related tests and doc
> + * though.
> + */
I like the previous TODO, but I think this one can just be left out.
While it might be possible to do something sensible with pathspecs at
least for linear history, it's not clear to me how it could generally
work with non-linear history. And since replay has handling
non-linear history as a primary (eventual) goal, the "definitely" here
seems incorrect to me.
[...]
Rest looks good.
