Re: [PATCH v6 00/14] Introduce new `git replay` command

> On Tue, Nov 7, 2023 at 3:44 AM Elijah Newren <newren@gmail.com> wrote:
> > Looking good, just one comment on one small hunk...
> >
> > On Thu, Nov 2, 2023 at 6:52 AM Christian Couder
> > <christian.couder@gmail.com> 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");