Re: [PATCH 1/3] revlist.c: introduce --cherry for unsymmetric picking

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Feb 21, 2011, 12:24 UTC
Message-ID: <4D625972.4090500@drmicha.warpmail.net>
In-Reply-To: <7vzkptnn7x.fsf@alter.siamese.dyndns.org>

Junio C Hamano venit, vidit, dixit 18.02.2011 20:42:

Show 32 quoted lines

> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> The existing "--cherry-pick" does not work with unsymmetric ranges
>> (A..B) for obvious reasons.
>>
>> Introduce "--cherry" which works more like "git-cherry", i.e.: Ignore
>> commits in B which are patch-equivalent to patches in A, i.e. list only
>> those which "git cherry B A" would list with a "-". This is especially
>> useful for things like
>>
>> git log --cherry @{u}..
>>
>> which is a much more descriptive than
>>
>> git cherry @{u}
>>
>> and potentially more useful than
>>
>> git log --cherry-pick @{u}...
>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> 
> A few comments.
> 
> As a Porcelain tool, "log --cherry @{u}.." may very well be useful; what
> do I have that I still need to send.  It is the moral equivalent of what
> format-patch does internally.
> 
> However I don't find it "works more like 'git-cherry'" at all.  The point
> of the command is to show _both_ what are still left to be sent, and what
> are already accepted.  So it is very much inherently a symmetric-range
> operation.

Maybe I should have said "works more like I use git-cherry"... Whenever I use it, I'm interested in one side only.

> At the plumbing level (which 'git-cherry' was designed to be),
> we should be able to ask for either (or both).  Adding a "we are only
> interested in the right hand side" as a rev-list option is somewhat
> distasteful and short-sighted.

I don't understand this remark - isn't that exactly what you suggest below ("--right-only")? Then what is distasteful and short-sighted?

I don't propose changing any current functionality of "git cherry" not "git rev-list --cherry-pick".

Show 6 quoted lines

> I wonder if you can instead extend the revision walking machinery by just
> adding a filter that says "show me only the left/right hand side" [*1*]?
> I.e.
> 
>     git rev-list --right-only --oneline --cherry-pick @{u}...
>

I think that is basically what I have done, just that I only implemented the shortcut option you describe in the next paragraphs:

Show 14 quoted lines

> If that is done, we could ask for "--left-only" when we wanted to.
> 
> As there are by definition more contributors than integrators, naturally
> we can expect that "--right-only" would be a lot more often used mode of
> operation than "--left-only", and I don't mind to have "--cherry" as a
> synonym to trigger "--cherry-pick --right-only" (and perhaps internally
> turn a two-dot automatically into three-dot), so that you can say
> 
>     git log --cherry @{u}..
> 
> from the command line to get the same effect.
> 
> By the way, when this feature is properly implemented internally at the
> revision traversal level, we should be able to lose quite a lot of code

Could you explain in what way the current implementation is not done "properly"? I introduced a flag for the walker and act on the flag. I don't see a way to cut down the walk further since we need the other side for the patch-id comparisons.

Show 9 quoted lines

> from builtin/log.c, as format-patch in it was the original implementation
> of the whole thing, and it was done outside the revision walking machinery
> to implement patch equivalence filtering of the traversal result.  We
> would essentially feed the symmetric upstream...HEAD range with the
> cherry-pick flag, ask it to give only the right hand side (i.e. what are
> left after the patch equivalence filter), and emit the result.
> 
> The get_patch_ids() function itself can go, and its caller and the
> codepaths that use has_commit_patch_id() would be vastly simplified, no?

I have not looked at format-patch at all. Also, I tend to forget --ignore-if-in-upstream. In fact, git-format-patch(1) carefully avoids any mentioning of "cherry" to the extent that it's difficult to recognize as the same concept at all. I would suggest renaming it "--cherry" or "--cherry-pick" in 1.8.0.

So I think the plan is:

- use a "right-only" flag rather than "cherry"
- make "--cherry" do "--cherry-pick --right-only" (with or without
".."->"...")
- simplify cmd_format_patch

Please stop me if I misunderstood you ;)

Michael