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

From: Elijah Newren <newren@gmail.com>
Date: Nov 7, 2023, 02:43 UTC
Message-ID: <CABPp-BE6G2qaF50bhz-CwxSsvxGDHzwvsWtfQO4zVcX6ERppLw@mail.gmail.com>
In-Reply-To: <20231102135151.843758-1-christian.couder@gmail.com>

Hi,

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:

[...]

Show 17 quoted lines

>     @@ 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
?