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 10:56:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Junio C Hamano venit, vidit, dixit 09.03.2011 22:49:
>> ...
>>> Conceptually revs->cherry_mark ought to be a subset of revs->cherry_pick
>>> and the code shouldn't have to do something like this:
>>>
>>> 	if (revs->cherry_pick || revs->cherry_mark)
>>>         	cherry_pick_list();
>>>
>>> Instead, the code should arrange that revs->cherry_pick is always set
>>> when revs->cherry_mark is set before the calling application enters the
>>> loop to call get_revision().
>>>
>>> But that would make the command line parsing more cumbersome (you would
>>> either waste one bit so that you can tell if you saw --cherry-pick on the
>>> command line, or keep the version of parser in this series as-is, and add
>>> postprocessing code to flip revs->cherry_pick on when revs->cherry_mark
>>> was given in prepare_revision_walk()), and I understand that is why you
>>> did it that way?
>>
>> Yes, I think so :) Another reason is that even cherry_pick_list() needs
>> to know which object flag it should set.
> 
> I didn't have trouble with "if (revs->cherry_mark)" used for that purpose
> at all.
> 
> That is what I meant by "cherry-mark is a _subset_ of cherry-pick".  In
> other words, if you are marking the cherry-pick result, you must be doing
> a cherry-pick to begin with, so having to say "cherry-pick || cherry-mark"
> is a bad taste.
> 
> But that does not contradict with the need to do something differently
> between the case where you are cherry-marking and where you are not
> cherry-marking.
> 
>> - leave the parser as is
>> - set cherry_pick when cherry_mark is set
>> - if (revs->cherry_pick) cherry_pick_list();
>> - leave cherry_pick_list() as is
> 
> I think that essentially is what I speculated in "either X or Y" part as
> solution Y.
> 
>> Would that be better? I didn't do that because it changes the meaning of
>> cherry_pick (it can mean two things then). It would introduce the
>> assumption "pick is set when mark is" instead of "at most one is set"
> 
> Well, as I said earlier, my suggestion was coming from "if you are
> cherry-marking, you must be cherry-picking in the first place", so yes, I
> am saying that "at most one is set" is a bad taste.  If the assumption has
> to change to match the better taste, that can only be a good thing ;-).

Being half-way through doing "Y" I remember why I really disliked "Y":
because it breaks the association between option names (--cherry-pick,
--cherry-mark) and rev flags (cherry_pick, cherry_mark). With "Y",
cherry_pick would mean --cherry-pick or --cherry-mark. (Also, as
mentioned, we're not "picking" when marking.)

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.

Michael