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

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Mar 10, 2011, 08:08 UTC
Message-ID: <4D7886FD.60109@drmicha.warpmail.net>
In-Reply-To: <7vtyfc7ymk.fsf@alter.siamese.dyndns.org>

Junio C Hamano venit, vidit, dixit 09.03.2011 22:49:

Show 12 quoted lines

> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> This is a replacement for mg/rev-list-one-side-only in pu.
>> The --left/right-only related commits (1-4/9) are unchanged.
> 
> I like the general idea of marking the equivalent ones instead of outright
> discarding the commits in the cherry_pick_list() function.
> 
> It might be a good idea to record the correspondence between equivalent
> commits in some way; the current topic does not need that information in
> order to produce its output, so that is something other people can build
> on top of this topic in the future.

Yes, I often want to look up the commit on pu which a patch resulted in. But usually looking it up by commit subject works better than by patch-id (because of occasional minor edits).

In general it might be nice to have maybe --patch-ids similar to --parents, or a --patch=<commit> limitting option. (I don't think outputting equivalent commits along with all commits is viable.)

Show 18 quoted lines

> There is only one minor point that nagged me while reading this series.
> 
> 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. So we could

- 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

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" which is as good or bad. (The current series does cherry_mark when both are set by a different caller, and the above would do neither when only cherry_mark is set.)

I think I also didn't do it because "marking" does not do any "picking", so with the above "cherry_pick" would really be "cherry_process".

But I'm open to changing this, of course (maybe along with renaming revs->cherry_pick).

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 (<>-^+=).

Michael