Re: [PATCH v4 2/6] connected: do not sort input revisions
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 5, 2021, 18:44 UTC
- Message-ID
- <xmqqfsvnloju.fsf@gitster.g>
- In-Reply-To
- <9d7f484907e2bd2492e6676238579e9f0c6ed374.1628162156.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> + } else if (!strcmp(arg, "--unsorted-input")) {
> + if (revs->no_walk && !revs->unsorted_input)
> + die(_("--unsorted-input is incompatible with --no-walk and --no-walk=sorted"));
> + revs->unsorted_input = 1;So this can be used with --no-walk=unsorted, even though doing so would be redundant and meaningless. OK.
Show 6 quoted lines
> @@ -2651,6 +2655,8 @@ static int handle_revision_pseudo_opt(const char *submodule,
> } else if (!strcmp(arg, "--not")) {
> *flags ^= UNINTERESTING | BOTTOM;
> } else if (!strcmp(arg, "--no-walk")) {
> + if (revs->unsorted_input)
> + die(_("--no-walk is incompatible with --no-walk=unsorted and --unsorted-input"));And likewise, --no-walk is --no-walk=sorted so we do not allow it to be used with --unsorted-input or --no=walk=unsorted. OK.
Show 12 quoted lines
> revs->no_walk = 1;
> } else if (skip_prefix(arg, "--no-walk=", &optarg)) {
> /*
> @@ -2658,9 +2664,12 @@ static int handle_revision_pseudo_opt(const char *submodule,
> * not allowed, since the argument is optional.
> */
> revs->no_walk = 1;
> - if (!strcmp(optarg, "sorted"))
> + if (!strcmp(optarg, "sorted")) {
> + if (revs->unsorted_input)
> + die(_("--no-walk=sorted is incompatible with --no-walk=unsorted "
> + "and --unsorted-input"));OK.
> revs->unsorted_input = 0; > - else if (!strcmp(optarg, "unsorted")) > + } else if (!strcmp(optarg, "unsorted")) > revs->unsorted_input = 1;
This is --no-walk=unsorted; could it have been given after --no-walk or --no-walk=unsorted?
The application of the incompatibility rules seems a bit uneven. An earlier piece of code will reject "--no-walk=unsorted --no-walk" given in this order (see "And likewise" above). But here, this part of the code will happily take "--no-walk --no-walk=unsorted".
Of course these details can be fixed with more careful code design, but I wonder if it may be result in the code and behaviour that is far simpler to explain (and probably implement) if we declare that
* --no-walk is not a synonym to --no-walk=sorted; it just flips .no_walk member on.
* --no-walk=sorted and --no-walk=unsorted flip .no_walk member on, and then flips .unsorted_input member off or on, respectively.
and define that the usual last-one-wins rule would apply?
Thanks.