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

Patrick Steinhardt [off-list ref] 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.

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.

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.

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

Patrick Steinhardt [off-list ref] writes:

quoted

 			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