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

[Junio C Hamano <gitster@pobox.com>]

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.

> @@ -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.

>  		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.