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

[Michael J Gruber <git@drmicha.warpmail.net>]

Junio C Hamano venit, vidit, dixit 10.03.2011 19:22:
> 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.

I see, thanks for the background!

Other callers of the revision walker have to remember that they need to
set revs->limited already when they want to use any limitting flags, so
revs->cherry_pick as in v3 would be no different.(*)

So maybe we should have another flag revs->cherry_process which is set
automatically when needed, after processing all options (same with
limited), and leave cherry_pick to keep track of --cherry-pick?

(*) Well, in fact they don't as long as they use the api correctly.
blame and shortlog call parse_revision_opt but use setup_revisions and
prepare_revision_walk afterwards.
pack-objects calls handle_revision_arg after setup_revisions but
prepare_revision_walk after. I was afraid of callers setting
revs->something inconsistently and then jumping into the walker.

So, maybe prepare_revision_walk is the most sensible place to set
revs->limited and revs->cherry_process when needed as indicated by other
flags (or set already), and take out their handling from the option parser?

Michael