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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 10, 2011, 09:56 UTC
Message-ID: <7vd3lz5me5.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4D7886FD.60109@drmicha.warpmail.net>

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

Show 21 quoted lines

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

> Any thoughts on the actual marks ('+' vs. ' ' vs. '*')?
> I also feel like we could use a space between the mark and the sha1
> (like git cherry does) to ease copy and paste but that is an orthogonal
> issue applying to all marks (<>-^+=).

I personally don't care very much and also choose not to say much on it too early even if I had opinion on the choice, because I know there are many people with better visual tastes than I on the list and I am sure they will voice their opinion once they see sample output.