Junio C Hamano venit, vidit, dixit 10.03.2011 10:56: > Michael J Gruber <git@xxxxxxxxxxxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html