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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 10, 2011, 18:22 UTC
Message-ID: <7v4o7a6dj3.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4D78AC8B.7010308@drmicha.warpmail.net>

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

Show 8 quoted lines

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