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

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

[Michael J Gruber]

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>
---
I first considered "--cherry-pick A..B" to automatically invoke this mode.
Here are a few reasons why I didn't:

- I haven't found a better way to propagate "we have a fake symmetric
range" from handle_revision_arg() to cherry_pick_list(). Flags like
SHOWN or TMP_MARK are reset somewhere in between! A global wouldn't do a
better (more fine grained) job than the rev flag.

- In the case of multiple revision args, it's probably less confusing to
have one overall mode (think "--cherry-pick A...B C..D") than the added
flexibility.

- I don't like the name "--cherry-pick" for an option which is like "git
cherry" and unlike "git cherry-pick".

- We could still activate this mode as soon as one A..B range apears.

 revision.c |   17 +++++++++++++++--
 revision.h |    1 +
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index 86d2470..91d27ea 100644
--- a/revision.c
+++ b/revision.c
@@ -611,6 +611,16 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 	}
 
 	free_patch_ids(&ids);
+
+	if (!revs->cherry)
+		return;
+	/* Remove auxiliary commits */
+	for (p = list; p; p = p->next) {
+		struct commit *commit = p->item;
+
+		if (commit->object.flags & SYMMETRIC_LEFT)
+			commit->object.flags |= SHOWN;
+	}
 }
 
 /* How many extra uninteresting commits we want to see.. */
@@ -781,7 +791,7 @@ static int limit_list(struct rev_info *revs)
 		show(revs, newlist);
 		show_early_output = NULL;
 	}
-	if (revs->cherry_pick)
+	if (revs->cherry || revs->cherry_pick)
 		cherry_pick_list(newlist, revs);
 
 	if (bottom) {
@@ -1028,7 +1038,7 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
 				verify_non_filename(revs->prefix, arg);
 			}
 
-			if (symmetric) {
+			if (symmetric || revs->cherry) {
 				exclude = get_merge_bases(a, b, 1);
 				add_pending_commit_list(revs, exclude,
 							flags_exclude);
@@ -1265,6 +1275,9 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--cherry-pick")) {
 		revs->cherry_pick = 1;
 		revs->limited = 1;
+	} else if (!strcmp(arg, "--cherry")) {
+		revs->cherry = 1;
+		revs->limited = 1;
 	} else if (!strcmp(arg, "--objects")) {
 		revs->tag_objects = 1;
 		revs->tree_objects = 1;
diff --git a/revision.h b/revision.h
index 82509dd..9a01050 100644
--- a/revision.h
+++ b/revision.h
@@ -65,6 +65,7 @@ struct rev_info {
 			show_decorations:1,
 			reverse:1,
 			reverse_output_stage:1,
+			cherry:1,
 			cherry_pick:1,
 			bisect:1,
 			ancestry_path:1,
-- 
1.7.4.1.74.gf39475.dirty

[PATCH 2/3] t6007: Make sure we test --cherry-pick

[Michael J Gruber]

Test 5 wants to test --cherry-pick but limits by pathspec in such a way
that there are no commits on the left side of the range.

Add a test without "--cherry-pick" which displays this, and add two
more commits and another test which tests what we're after. This also
shortens the last test.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t6007-rev-list-cherry-pick-file.sh |   49 ++++++++++++++++++++++++++++-----
 1 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index b565638..64b72a3 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -4,13 +4,14 @@ test_description='test git rev-list --cherry-pick -- file'
 
 . ./test-lib.sh
 
-# A---B
+# A---B---D
 #  \
 #   \
-#    C
+#    C---E
 #
 # B changes a file foo.c, adding a line of text.  C changes foo.c as
 # well as bar.c, but the change in foo.c was identical to change B.
+# D and C change bar in the same way, E differently.
 
 test_expect_success setup '
 	echo Hallo > foo &&
@@ -25,11 +26,21 @@ test_expect_success setup '
 	test_tick &&
 	git commit -m "C" &&
 	git tag C &&
+	echo Dello > bar &&
+	git add bar &&
+	test_tick &&
+	git commit -m "E" &&
+	git tag E &&
 	git checkout master &&
 	git checkout branch foo &&
 	test_tick &&
 	git commit -m "B" &&
-	git tag B
+	git tag B &&
+	echo Cello > bar &&
+	git add bar &&
+	test_tick &&
+	git commit -m "D" &&
+	git tag D
 '
 
 cat >expect <<EOF
@@ -53,8 +64,33 @@ test_expect_success '--cherry-pick foo comes up empty' '
 	test -z "$(git rev-list --left-right --cherry-pick B...C -- foo)"
 '
 
+cat >expect <<EOF
+>tags/C
+EOF
+
 test_expect_success '--cherry-pick bar does not come up empty' '
-	! test -z "$(git rev-list --left-right --cherry-pick B...C -- bar)"
+	git rev-list --left-right --cherry-pick B...C -- bar > actual &&
+	git name-rev --stdin --name-only --refs="*tags/*" \
+		< actual > actual.named &&
+	test_cmp actual.named expect
+'
+
+test_expect_success 'bar does not come up empty' '
+	git rev-list --left-right B...C -- bar > actual &&
+	git name-rev --stdin --name-only --refs="*tags/*" \
+		< actual > actual.named &&
+	test_cmp actual.named expect
+'
+
+cat >expect <<EOF
+>tags/E
+EOF
+
+test_expect_success '--cherry-pick bar does not come up empty (II)' '
+	git rev-list --left-right --cherry-pick D...E -- bar > actual &&
+	git name-rev --stdin --name-only --refs="*tags/*" \
+		< actual > actual.named &&
+	test_cmp actual.named expect
 '
 
 test_expect_success '--cherry-pick with independent, but identical branches' '
@@ -75,11 +111,8 @@ cat >expect <<EOF
 1	2
 EOF
 
-# Insert an extra commit to break the symmetry
 test_expect_success '--count --left-right' '
-	git checkout branch &&
-	test_commit D &&
-	git rev-list --count --left-right B...D > actual &&
+	git rev-list --count --left-right C...D > actual &&
 	test_cmp expect actual
 '
 
-- 
1.7.4.1.74.gf39475.dirty

[PATCH 3/3] rev-list: documentation and test for --cherry

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-rev-list.txt       |    1 +
 Documentation/rev-list-options.txt   |   10 ++++++++++
 t/t6007-rev-list-cherry-pick-file.sh |   25 +++++++++++++++++++++----
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index 8e1e329..a74fb97 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -32,6 +32,7 @@ SYNOPSIS
 	     [ \--timestamp ]
 	     [ \--left-right ]
 	     [ \--cherry-pick ]
+	     [ \--cherry ]
 	     [ \--encoding[=<encoding>] ]
 	     [ \--(author|committer|grep)=<pattern> ]
 	     [ \--regexp-ignore-case | -i ]
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 44a2ef1..3a68629 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -319,6 +319,16 @@ from the other branch (for example, "3rd on b" may be cherry-picked
 from branch A).  With this option, such pairs of commits are
 excluded from the output.
 
+--cherry::
+
+	Omit any commit that instroduces the same change as
+	another commit on the left side of a symmetric or asymmetric
+	range (they are treated the same).
++
+For example, `git rev-list --cherry A..B` omits those commits from
+`B` which are in `A` or are patch-equivalent to a commit in `A`.
+In other words, this lists the `{plus}` commits from `git cherry A B`.
+
 -g::
 --walk-reflogs::
 
diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index 64b72a3..56efdd0 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -4,14 +4,14 @@ test_description='test git rev-list --cherry-pick -- file'
 
 . ./test-lib.sh
 
-# A---B---D
+# A---B---D---F
 #  \
 #   \
 #    C---E
 #
 # B changes a file foo.c, adding a line of text.  C changes foo.c as
 # well as bar.c, but the change in foo.c was identical to change B.
-# D and C change bar in the same way, E differently.
+# D and C change bar in the same way, E and F differently.
 
 test_expect_success setup '
 	echo Hallo > foo &&
@@ -40,7 +40,12 @@ test_expect_success setup '
 	git add bar &&
 	test_tick &&
 	git commit -m "D" &&
-	git tag D
+	git tag D &&
+	echo Nello > bar &&
+	git add bar &&
+	test_tick &&
+	git commit -m "F" &&
+	git tag F
 '
 
 cat >expect <<EOF
@@ -83,11 +88,23 @@ test_expect_success 'bar does not come up empty' '
 '
 
 cat >expect <<EOF
+<tags/F
 >tags/E
 EOF
 
 test_expect_success '--cherry-pick bar does not come up empty (II)' '
-	git rev-list --left-right --cherry-pick D...E -- bar > actual &&
+	git rev-list --left-right --cherry-pick F...E -- bar > actual &&
+	git name-rev --stdin --name-only --refs="*tags/*" \
+		< actual > actual.named &&
+	test_cmp actual.named expect
+'
+
+cat >expect <<EOF
+tags/E
+EOF
+
+test_expect_success '--cherry bar does not come up empty' '
+	git rev-list --cherry F..E -- bar > actual &&
 	git name-rev --stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp actual.named expect
-- 
1.7.4.1.74.gf39475.dirty

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

[Junio C Hamano]

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

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


[Footnote]

*1* It might be even interesting to add a mode that allows you to ask for
only the matching ones, but that would take somewhat a different logic in
cherry_pick_list() in revision.c.  Instead of filtering out the equivalent
ones, you would need to keep them, painted in yet another color.

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

[Jay Soffian]

On Fri, Feb 18, 2011 at 2:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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
> 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.

Tangent: it occurred to me it would be useful to give the user some
say in how the patch-id is computed. Currently, the patch-id is the
hash of the diff hunks, which is very strict.

I think it would be useful to have a "loose" mode of computing the
patch-id. What I was thinking was to use the hash of certain commit
metadata that is likely to be unique: say, the commit authorship
(name, email, timestamp) + commit subject and/or body.

This would allow rev-list to (optionally) filter out commits that,
modulo conflict resolution, introduce the "same" change. IOW, give the
user of rev-list some say about what a "same" commit is.

(I suppose this is my itch to scratch.) :-)

j.

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

[Michael J Gruber]

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

[PATCHv2 1/2] revlist.c: introduce --left/right-only for unsymmetric picking

[Michael J Gruber]

The existing "--cherry-pick" does not work with unsymmetric ranges
(A..B) for obvious reasons.

Introduce "--left-only" and "--right-only" which limit the output to
commits on the respective sides of a symmetric range (i.e. only "<"
resp. ">" commits as per "--left-right").

This is especially useful for things like

git log --cherry-pick --right-only @{u}...

which is much more flexible (and 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>
---
Notes:
    v2 does it the --left/right-only way, which I hope is the right way :)
    
    Those new limiters work together with --left-right, --cherry-pick etc.
    
    It could be followed up by introducing --cherry as equivalent to
    --cherry-pick --right-only --ignore-merges
    and optionally making this work for A..B, or making "--cherry-pick A..B"
    invoke "--cherry-pick --right-only A...B" which would make a lot of sense
    but also change existing behavior.

 revision.c |   24 ++++++++++++++++++++++++
 revision.h |    2 ++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/revision.c b/revision.c
index 86d2470..6ffa8fb 100644
--- a/revision.c
+++ b/revision.c
@@ -729,6 +729,23 @@ static struct commit_list *collect_bottom_commits(struct commit_list *list)
 	return bottom;
 }
 
+/* Assumes either left_only or right_only is set */
+static void limit_left_right(struct commit_list *list, struct rev_info *revs)
+{
+	struct commit_list *p;
+
+	for (p = list; p; p = p->next) {
+		struct commit *commit = p->item;
+
+		if (revs->right_only) {
+			if (commit->object.flags & SYMMETRIC_LEFT)
+				commit->object.flags |= SHOWN;
+		} else	/* revs->left_only is set */
+			if (!(commit->object.flags & SYMMETRIC_LEFT))
+				commit->object.flags |= SHOWN;
+	}
+}
+
 static int limit_list(struct rev_info *revs)
 {
 	int slop = SLOP;
@@ -784,6 +801,9 @@ static int limit_list(struct rev_info *revs)
 	if (revs->cherry_pick)
 		cherry_pick_list(newlist, revs);
 
+	if (revs->left_only || revs->right_only)
+		limit_left_right(newlist, revs);
+
 	if (bottom) {
 		limit_to_ancestry(bottom, newlist);
 		free_commit_list(bottom);
@@ -1260,6 +1280,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->boundary = 1;
 	} else if (!strcmp(arg, "--left-right")) {
 		revs->left_right = 1;
+	} else if (!strcmp(arg, "--left-only")) {
+		revs->left_only = 1;
+	} else if (!strcmp(arg, "--right-only")) {
+		revs->right_only = 1;
 	} else if (!strcmp(arg, "--count")) {
 		revs->count = 1;
 	} else if (!strcmp(arg, "--cherry-pick")) {
diff --git a/revision.h b/revision.h
index 82509dd..a4096ca 100644
--- a/revision.h
+++ b/revision.h
@@ -59,6 +59,8 @@ struct rev_info {
 			boundary:2,
 			count:1,
 			left_right:1,
+			left_only:1,
+			right_only:1,
 			rewrite_parents:1,
 			print_parents:1,
 			show_source:1,
-- 
1.7.4.1.74.gf39475.dirty

[PATCHv2 2/2] rev-list: documentation and test for --left/right-only

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Notes:
    v2 adjusts to the new naming and tests both left and right.

 Documentation/git-rev-list.txt       |    2 ++
 Documentation/rev-list-options.txt   |   13 +++++++++++++
 t/t6007-rev-list-cherry-pick-file.sh |   32 ++++++++++++++++++++++++++++----
 3 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index 8e1e329..5f47a13 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -31,6 +31,8 @@ SYNOPSIS
 	     [ \--parents ]
 	     [ \--timestamp ]
 	     [ \--left-right ]
+	     [ \--left-only ]
+	     [ \--right-only ]
 	     [ \--cherry-pick ]
 	     [ \--encoding[=<encoding>] ]
 	     [ \--(author|committer|grep)=<pattern> ]
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 44a2ef1..1e67d95 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -319,6 +319,19 @@ from the other branch (for example, "3rd on b" may be cherry-picked
 from branch A).  With this option, such pairs of commits are
 excluded from the output.
 
+--left-only::
+--right-only::
+
+	List only commits on the respective side of a symmetric range,
+	i.e. only those which would be marked `<` resp. `>` by
+	`--left-right`.
++
+For example, `--cherry-pick --right-only A...B` omits those
+commits from `B` which are in `A` or are patch-equivalent to a commit in
+`A`. In other words, this lists the `{plus}` commits from `git cherry A B`.
+More precisely, `--cherry-pick --right-only --ignore-merges` gives the
+exact list. 
+
 -g::
 --walk-reflogs::
 
diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index 64b72a3..cd089a9 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -4,14 +4,14 @@ test_description='test git rev-list --cherry-pick -- file'
 
 . ./test-lib.sh
 
-# A---B---D
+# A---B---D---F
 #  \
 #   \
 #    C---E
 #
 # B changes a file foo.c, adding a line of text.  C changes foo.c as
 # well as bar.c, but the change in foo.c was identical to change B.
-# D and C change bar in the same way, E differently.
+# D and C change bar in the same way, E and F differently.
 
 test_expect_success setup '
 	echo Hallo > foo &&
@@ -40,7 +40,12 @@ test_expect_success setup '
 	git add bar &&
 	test_tick &&
 	git commit -m "D" &&
-	git tag D
+	git tag D &&
+	echo Nello > bar &&
+	git add bar &&
+	test_tick &&
+	git commit -m "F" &&
+	git tag F
 '
 
 cat >expect <<EOF
@@ -83,11 +88,30 @@ test_expect_success 'bar does not come up empty' '
 '
 
 cat >expect <<EOF
+<tags/F
 >tags/E
 EOF
 
 test_expect_success '--cherry-pick bar does not come up empty (II)' '
-	git rev-list --left-right --cherry-pick D...E -- bar > actual &&
+	git rev-list --left-right --cherry-pick F...E -- bar > actual &&
+	git name-rev --stdin --name-only --refs="*tags/*" \
+		< actual > actual.named &&
+	test_cmp actual.named expect
+'
+
+cat >expect <<EOF
+tags/E
+EOF
+
+test_expect_success '--cherry-pick --right-only' '
+	git rev-list --cherry-pick --right-only F...E -- bar > actual &&
+	git name-rev --stdin --name-only --refs="*tags/*" \
+		< actual > actual.named &&
+	test_cmp actual.named expect
+'
+
+test_expect_success '--cherry-pick --left-only' '
+	git rev-list --cherry-pick --left-only E...F -- bar > actual &&
 	git name-rev --stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp actual.named expect
-- 
1.7.4.1.74.gf39475.dirty

Re: [PATCHv2 2/2] rev-list: documentation and test for --left/right-only

[Jay Soffian]

On Mon, Feb 21, 2011 at 11:09 AM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> +More precisely, `--cherry-pick --right-only --ignore-merges` gives the

Nit: s/ignore-merges/no-merges/

j.

Re: [PATCHv2 2/2] rev-list: documentation and test for --left/right-only

[Michael J Gruber]

Jay Soffian venit, vidit, dixit 21.02.2011 18:37:
> On Mon, Feb 21, 2011 at 11:09 AM, Michael J Gruber
> <git@drmicha.warpmail.net> wrote:
>> +More precisely, `--cherry-pick --right-only --ignore-merges` gives the
> 
> Nit: s/ignore-merges/no-merges/
> 
> j.

Thanks. I think I applied our usual rev-flag to option mapping which
failed me here.

Michael

Re: [PATCHv2 1/2] revlist.c: introduce --left/right-only for unsymmetric picking

[Junio C Hamano]

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

> The existing "--cherry-pick" does not work with unsymmetric ranges
> (A..B) for obvious reasons.

The implementation of this round looks a lot simpler and cleaner.  We
might want to make sure that l/r only are mutually exclusive at the
command parser, though.

diff --git a/revision.c b/revision.c
index 0681c7c..02aa788 100644
--- a/revision.c
+++ b/revision.c
@@ -1906,6 +1906,9 @@ int prepare_revision_walk(struct rev_info *revs)
 	int nr = revs->pending.nr;
 	struct object_array_entry *e, *list;
 
+	if (revs->left_only && revs->right_only)
+		die("left-only and right-only are mutually exclusive");
+
 	e = list = revs->pending.objects;
 	revs->pending.nr = 0;
 	revs->pending.alloc = 0;


Also, I wonder if we want to enhance cherry_pick_list() a bit to give it
an option to show only the commits that have equivalent commits on the
other side (i.e. "the ones that I can now discard"); it obviously is a
separate topic.

>     It could be followed up by introducing --cherry as equivalent to
>     --cherry-pick --right-only --no-merges.

Yeah, I think that is a good idea.

Thanks.

Re: [PATCHv2 1/2] revlist.c: introduce --left/right-only for unsymmetric picking

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> ...  We
> might want to make sure that l/r only are mutually exclusive at the
> command parser, though.

-- >8 --
Subject: [PATCH] rev-list: --left/right-only are mutually exclusive

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 revision.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/revision.c b/revision.c
index 0681c7c..1fcaeb7 100644
--- a/revision.c
+++ b/revision.c
@@ -1284,8 +1284,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--left-right")) {
 		revs->left_right = 1;
 	} else if (!strcmp(arg, "--left-only")) {
+		if (revs->right_only)
+			die("--left-only is incompatible with --right-only");
 		revs->left_only = 1;
 	} else if (!strcmp(arg, "--right-only")) {
+		if (revs->left_only)
+			die("--right-only is incompatible with --left-only");
 		revs->right_only = 1;
 	} else if (!strcmp(arg, "--count")) {
 		revs->count = 1;
-- 
1.7.4.1.107.ga7184

Re: [PATCHv2 1/2] revlist.c: introduce --left/right-only for unsymmetric picking

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>>     It could be followed up by introducing --cherry as equivalent to
>>     --cherry-pick --right-only --no-merges.
>
> Yeah, I think that is a good idea.

-- >8 --
Subject: [PATCH] log --cherry: a synonym

At the Porcelain level, because by definition there are many more contributors
than integrators, it makes sense to give a handy short-hand for --right-only
used with --cherry-pick.  Make it so.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * We would want to add some tests, but I thought at least I owe you
   the final patch to bring the handy short-hand after this back and
   forth first, so here it is.

 Documentation/rev-list-options.txt |    7 +++++++
 revision.c                         |    9 ++++++++-
 2 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index cebba62..3068dc3 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -332,6 +332,13 @@ commits from `B` which are in `A` or are patch-equivalent to a commit in
 More precisely, `--cherry-pick --right-only --no-merges` gives the exact
 list.
 
+--cherry::
+
+	A synonym for `--right-only --cherry-pick --no-merges`; useful to
+	limit the output to the commits on our side that have not been
+	applied to the other side of a forked history with `git log --cherry
+	upstream...mybranch`.
+
 -g::
 --walk-reflogs::
 
diff --git a/revision.c b/revision.c
index 1fcaeb7..f747526 100644
--- a/revision.c
+++ b/revision.c
@@ -1285,12 +1285,19 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->left_right = 1;
 	} else if (!strcmp(arg, "--left-only")) {
 		if (revs->right_only)
-			die("--left-only is incompatible with --right-only");
+			die("--left-only is incompatible with --right-only"
+			    " or --cherry");
 		revs->left_only = 1;
 	} else if (!strcmp(arg, "--right-only")) {
 		if (revs->left_only)
 			die("--right-only is incompatible with --left-only");
 		revs->right_only = 1;
+	} else if (!strcmp(arg, "--cherry")) {
+		if (revs->left_only)
+			die("--cherry is incompatible with --left-only");
+		revs->cherry_pick = 1;
+		revs->right_only = 1;
+		revs->limited = 1;
 	} else if (!strcmp(arg, "--count")) {
 		revs->count = 1;
 	} else if (!strcmp(arg, "--cherry-pick")) {
-- 
1.7.4.1.107.ga7184

[PATCH (n+1)/n] t6007: test rev-list --cherry

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t6007-rev-list-cherry-pick-file.sh |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index cd089a9..5373541 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -110,6 +110,13 @@ test_expect_success '--cherry-pick --right-only' '
 	test_cmp actual.named expect
 '
 
+test_expect_success '--cherry' '
+	git rev-list --cherry F...E -- bar > actual &&
+	git name-rev --stdin --name-only --refs="*tags/*" \
+		< actual > actual.named &&
+	test_cmp actual.named expect
+'
+
 test_expect_success '--cherry-pick --left-only' '
 	git rev-list --cherry-pick --left-only E...F -- bar > actual &&
 	git name-rev --stdin --name-only --refs="*tags/*" \
-- 
1.7.4.1.74.gf39475.dirty

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

[Junio C Hamano]

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

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

It does not give people an option to ask for only left side if you give
only "--cherry" and nothing else.  Don't you find that a bad taste?

Compare that with being explicit and supporting both --right/left-only.
The point is to avoid making the internal implementation and plumbing
interface needlessly limited to one use case, and instead keeping them
flexible.

> Could you explain in what way the current implementation is not done
> "properly"?

Your "cherry" implementation limits its utility unnecessarily by assuming
a few things it does not have to assume:

 - It only needs to discard left hand side, but not right hand side.
 - It only needs to work when patch-id equivalence filtering is on.

The former is easy to rectify, I think.

Right now, there is only one thing (cherry-pick) that works on a symmetric
difference set and filter, so it may _appear_ that the latter does not
matter, but by not tying the two unrelated concepts together and keeping
the "right-side-only output" logic and "filter the symmetric difference
set" logic orthogonal, you will leave the door open for others to add
different filtering mode easier.

> So I think the plan is:
>
> - use a "right-only" flag rather than "cherry"

And perhaps left-only for symmetry.

> - make "--cherry" do "--cherry-pick --right-only" (with or without
> ".."->"...")

Yes.

> - simplify cmd_format_patch

As long as the internal machinery to run the right/left-only filtering of
the result of cherry-pick filtering is done (i.e. step 1) and then the
syntactic sugar --cherry synonym is implemented in terms of it (step 2),
cmd_format_patch simplification would come as a natural consequence, I
think.

The simplification does not have to be done as part of this series,
though.  I only mentioned it as a reason why we would want to get the
internal for this topic (step 1) right, not tied too strongly to step 2.

Thanks.