Re: [PATCH v4 2/6] connected: do not sort input revisions

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1961 bytes --]

On Thu, Aug 05, 2021 at 11:44:05AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]