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

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 6, 2021, 06:00 UTC
Message-ID: <YQzQGVQdh4t0uZ3N@ncase>
In-Reply-To: <xmqqfsvnloju.fsf@gitster.g>

On Thu, Aug 05, 2021 at 11:44:05AM -0700, Junio C Hamano wrote:

Show 27 quoted lines

> 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