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

[Michael J Gruber <git@drmicha.warpmail.net>]

Junio C Hamano venit, vidit, dixit 18.02.2011 20:42:
> 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".

> 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:

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

> 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