threads / patch / 26547

patch, 3 partsrevlist.c: introduce --cherry for unsymmetric picking

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

## tl;dr

15 messages between Feb 18, 2011 and Feb 22, 2011. Diffs are folded; open one to read it.

replies: 14people: 3as markdown or json

Michael J Gruber· Feb 18, 2011, 12:34 UTC · lore

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(-)
Show changes to 2 files +16 −2

revision.c, revision.h

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
Michael J Gruber· Feb 18, 2011, 12:34 UTC · re: Michael J Gruber · lore

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

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(-)
Show changes to t/t6007-rev-list-cherry-pick-file.sh +41 −8
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
Michael J Gruber· Feb 18, 2011, 12:34 UTC · re: Michael J Gruber · lore

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

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(-)
Show changes to 3 files +32 −4

Documentation/git-rev-list.txt, Documentation/rev-list-options.txt, t/t6007-rev-list-cherry-pick-file.sh

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
Junio C Hamano· Feb 18, 2011, 19:42 UTC · re: Michael J Gruber · lore

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

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 19 quoted lines
> 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.

Jay Soffian· Feb 19, 2011, 14:47 UTC · re: Junio C Hamano · lore

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

On Fri, Feb 18, 2011 at 2:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> 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.
Michael J Gruber· Feb 21, 2011, 12:24 UTC · re: Junio C Hamano · lore

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

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
Michael J Gruber· Feb 21, 2011, 16:09 UTC · re: Michael J Gruber · lore

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

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(-)
Show changes to 2 files +26 −0

revision.c, revision.h

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
Junio C Hamano· Feb 22, 2011, 00:48 UTC · re: Michael J Gruber · lore

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

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.

Show changes to revision.c +3 −0
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.
Junio C Hamano· Feb 22, 2011, 01:17 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to revision.c +4 −0
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
Junio C Hamano· Feb 22, 2011, 01:19 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 2 files +15 −1

Documentation/rev-list-options.txt, revision.c

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
Michael J Gruber· Feb 22, 2011, 13:36 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to t/t6007-rev-list-cherry-pick-file.sh +7 −0
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
Michael J Gruber· Feb 21, 2011, 16:09 UTC · re: Michael J Gruber · lore

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

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(-)
Show changes to 3 files +43 −4

Documentation/git-rev-list.txt, Documentation/rev-list-options.txt, t/t6007-rev-list-cherry-pick-file.sh

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
Jay Soffian· Feb 21, 2011, 17:37 UTC · re: Michael J Gruber · lore

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

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.
Michael J Gruber· Feb 22, 2011, 07:23 UTC · re: Jay Soffian · lore

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

Jay Soffian venit, vidit, dixit 21.02.2011 18:37:
Show 7 quoted lines
> 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
Junio C Hamano· Feb 21, 2011, 19:49 UTC · re: Michael J Gruber · lore

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

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 7 quoted lines
>> 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.

← back to recent threads