On Thu, Aug 05, 2021 at 11:44:05AM -0700, Junio C Hamano wrote: > Patrick Steinhardt 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