Hi Elijah and Dscho,
On Tue, Nov 7, 2023 at 10:43 AM Christian Couder
<christian.couder@xxxxxxxxx> wrote:
> On Tue, Nov 7, 2023 at 3:44 AM Elijah Newren <newren@xxxxxxxxx> wrote:
> > 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]);
> > > ++ }
> > > ++
> > 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
>
> Yeah, that would work. The downside is that it would be more difficult
> in the warning to tell the user which command line option was
> overridden as there are some values changed by different options.
> Maybe we can come up with a warning message that is still useful and
> enough for now, as the command is supposed to be used by experienced
> users. Perhaps something like:
>
> warning(_("some rev walking options will be overridden as '%s' bit in
> 'struct rev_info' will be forced"), "sort_order");
I have implemented this in the v7 I just sent.
Thanks!
