On Thu, Aug 05, 2021 at 11:44:05AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> writes:
> > 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.
Wouldn't that effectively change semantics though? If the user passes
`git rev-list --no-walk=unsorted --no-walk`, then the result is a sorted
revwalk right now. One may argue that most likely, nobody is doing that,
but you never really know.
An easier approach which keeps existing semantics is to just make
`--no-walk` and `--unsorted-input` mutually exclusive:
- If the `unsorted_input` bit is set and `no_walk` isn't, and we
observe any `--no-walk` option, then we bail.
- Likewise, if the `no_walk` bit is set, then we bail when we see
`--unsorted-input` regardless of the value of `unsorted_input`.
This would keep current semantics of `--no-walk`, but prohobit
using it together with the new option.
Patrick
Attachment:
signature.asc
Description: PGP signature
