Re: [PATCHv2 0/9] --left/right-only and --cherry-mark

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

Michael J Gruber <git@drmicha.warpmail.net> writes:

> ...
> Additionally, since parse_revision_opt (which calls handle_revision_opt)
> is called from other sites for individual args we would need to do the
> handling in the Y case (set pick when marking) right in
> handle_revision_opt, not just in setup_revisions. It's a matter of a few
> more if's and or's, but still.
>
> Taking these together, I wonder whether we shouldn't leave it as in v2.

The primary downside of keeping cherry_pick and cherry_mark as independent
I see is what would happen to the "if (cherry_pick || cherry_mark)" when
somebody comes up with the next bright idea to use the change equivalence
computed in cherry_pick_list(). With the approach in v2, the natural way
of enhancing this would be to add another "|| cherry_xyzzy" there, and the
next bright idea would add another "|| cherry_frotz".

My gut feeling was that it would be a more maintainable implementation to
have an internal bit that does not have to be exposed to the UI and that
tells the revision machinery that we need to compute the equivalence, and
an enum (if the three modes of using the equivalence are incompatible with
each other) or a one-bit-per-feature bitset (if the three features that
use the equivalence can be used at the same time) that tells the machinery
what to do with the equivalence information. From the UI level, the user
only asks for the feature(s) that use(s) the equivalence information, and
asking for any of them would set the internal "need equivalence" bit.

The above is modeled after the way how revs.limited bit is used. That bit
corresponds to the "internal bit" that is flipped by many features that
can be triggered from the UI level that depends on having the history
graph before they do their work.

Counting the number of "if (limited)" and tricky codepaths around it, and
contrasting that with the number of ways we flip the "limited = 1" bit
(which grew over time), I think you can understand where my fear of
potential future complexity against "if (cherry_pick || cherry_mark)"
comes from.

As I said earlier, this was only a minor thing that nagged me. Considering
that currently there is only one place that checks if we are doing any of
the operation that requires us to have change equivalence information to
change the behaviour depending on the result, I wouldn't be too unhappy if
this feature is done in the way you did in your v2 patch.

Thanks.