Hi,
Looking good, just one comment on one small hunk...
On Thu, Nov 2, 2023 at 6:52 AM Christian Couder
<christian.couder@xxxxxxxxx> wrote:
>
[...]
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> -
> strvec_pushl(&rev_walk_args, "", argv[2], "--not", argv[1], NULL);
>
> ++ /*
> ++ * TODO: For now, let's warn when we see an option that we are
> ++ * going to override after setup_revisions() below. In the
> ++ * future we might want to either die() or allow them if we
> ++ * think they could be useful though.
> ++ */
> ++ for (i = 0; i < argc; i++) {
> ++ if (!strcmp(argv[i], "--reverse") || !strcmp(argv[i], "--date-order") ||
> ++ !strcmp(argv[i], "--topo-order") || !strcmp(argv[i], "--author-date-order") ||
> ++ !strcmp(argv[i], "--full-history"))
> ++ warning(_("option '%s' will be overridden"), argv[i]);
> ++ }
> ++
Two things:
1) Not sure it makes sense to throw a warning with --topo-order or
--full-history, since they would result in a value matching what we
would be setting anyway.
2) This seems like an inefficient way to provide this warning; could
we avoid parsing the arguments for an extra time? Perhaps instead
a) set the desired values here, before setup_revisions()
b) after setup_revisions, check whether these values differ from the
desired values, if so throw a warning.
c) set the desired values, again
?
