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

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

[Michael J Gruber]

This is a replacement for mg/rev-list-one-side-only in pu.
The --left/right-only related commits (1-4/9) are unchanged.

5/9 is new and refactors the generation of commit marks (<>-) which we
currently do in 5 places, to ease maintenance and extensibility.

6,7 are new and introduce "--cherry-mark" to the revision walker which marks
those commits which "--cherry-pick" would omit.

8,9 are amended as compared to pu, introducing "--cherry" as a shortcut for
"--cherry-mark --right-only --no-merges" to produce output much like "git
cherry" with the full flexibility of rev-list/log. (We could use the previous
version instead, or with a different synonym; or introduce rev-list-option
aliases...)

The mark for patch-equivalent commits is '=' because git-cherry's '-' is used
for boundary commits already.

The mark for non-equivalent commits is '+' (following git-cherry), or '<'/'>'
with --left-right, or '*' with --graph.

This is up for bike-shed^W^Wdiscussing - we could use ' ' instead or even '*'
also without --graph, or '+' even with --graph.

I would in fact prefer ' ' without --graph and '*' with --graph, which would
make it even more different from git-cherry's output, though.
(Personally, I never liked git-cherry's output anyways.)

This is also the base for refactoring git-format-patch and git-cherry, of
course, where the latter would be helped if it's ok to change the output
format.

Junio C Hamano (1):
  rev-list: --left/right-only are mutually exclusive

Michael J Gruber (8):
  revlist.c: introduce --left/right-only for unsymmetric picking
  t6007: Make sure we test --cherry-pick
  rev-list: documentation and test for --left/right-only
  rev-list/log: factor out revision mark generation
  revision.c: introduce --cherry-mark
  rev-list: documentation and test for --cherry-mark
  log --cherry: a synonym
  t6007: test rev-list --cherry

 Documentation/git-rev-list.txt       |    3 +
 Documentation/rev-list-options.txt   |   26 ++++++++
 builtin/rev-list.c                   |   14 +----
 graph.c                              |   17 +-----
 log-tree.c                           |   28 +-------
 pretty.c                             |    6 +--
 revision.c                           |   74 +++++++++++++++++++++-
 revision.h                           |    7 ++-
 t/t6007-rev-list-cherry-pick-file.sh |  113 +++++++++++++++++++++++++++++++---
 9 files changed, 220 insertions(+), 68 deletions(-)

-- 
1.7.4.1.299.g567d7.dirty

[PATCHv2 1/9] 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} | sed -ne 's/^+ //p'

and potentially more useful than

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

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 revision.c |   24 ++++++++++++++++++++++++
 revision.h |    2 ++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/revision.c b/revision.c
index 7b9eaef..0681c7c 100644
--- a/revision.c
+++ b/revision.c
@@ -733,6 +733,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;
@@ -788,6 +805,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);
@@ -1263,6 +1283,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 05659c6..d2ffde1 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.299.g567d7.dirty

[PATCHv2 2/9] 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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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.299.g567d7.dirty

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

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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..cebba62 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 --no-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.299.g567d7.dirty

[PATCHv2 4/9] rev-list: --left/right-only are mutually exclusive

[Michael J Gruber]

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

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.299.g567d7.dirty

[PATCHv2 5/9] rev-list/log: factor out revision mark generation

[Michael J Gruber]

Currently, we have identical code for generating revision marks ('<',
'>', '-') in 5 places.

Factor out the code to a single function get_revision_mark() for easier
maintenance and extensibility.

Note that the check for !!revs in graph.c (which gets removed
effectively by this patch) is superfluous.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/rev-list.c |   14 ++------------
 graph.c            |   17 ++---------------
 log-tree.c         |   28 ++++------------------------
 pretty.c           |    6 +-----
 revision.c         |   17 +++++++++++++++++
 revision.h         |    1 +
 6 files changed, 27 insertions(+), 56 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ba27d39..f458cb7 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -64,18 +64,8 @@ static void show_commit(struct commit *commit, void *data)
 	if (info->header_prefix)
 		fputs(info->header_prefix, stdout);
 
-	if (!revs->graph) {
-		if (commit->object.flags & BOUNDARY)
-			putchar('-');
-		else if (commit->object.flags & UNINTERESTING)
-			putchar('^');
-		else if (revs->left_right) {
-			if (commit->object.flags & SYMMETRIC_LEFT)
-				putchar('<');
-			else
-				putchar('>');
-		}
-	}
+	if (!revs->graph)
+		fputs(get_revision_mark(revs, commit), stdout);
 	if (revs->abbrev_commit && revs->abbrev)
 		fputs(find_unique_abbrev(commit->object.sha1, revs->abbrev),
 		      stdout);
diff --git a/graph.c b/graph.c
index f1a63c2..ef2e24e 100644
--- a/graph.c
+++ b/graph.c
@@ -798,22 +798,9 @@ static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)
 	}
 
 	/*
-	 * If revs->left_right is set, print '<' for commits that
-	 * come from the left side, and '>' for commits from the right
-	 * side.
+	 * get_revision_mark() handles all other cases without assert()
 	 */
-	if (graph->revs && graph->revs->left_right) {
-		if (graph->commit->object.flags & SYMMETRIC_LEFT)
-			strbuf_addch(sb, '<');
-		else
-			strbuf_addch(sb, '>');
-		return;
-	}
-
-	/*
-	 * Print '*' in all other cases
-	 */
-	strbuf_addch(sb, '*');
+	strbuf_addstr(sb, get_revision_mark(graph->revs, graph->commit));
 }
 
 /*
diff --git a/log-tree.c b/log-tree.c
index b46ed3b..1257040 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -380,18 +380,8 @@ void show_log(struct rev_info *opt)
 	if (!opt->verbose_header) {
 		graph_show_commit(opt->graph);
 
-		if (!opt->graph) {
-			if (commit->object.flags & BOUNDARY)
-				putchar('-');
-			else if (commit->object.flags & UNINTERESTING)
-				putchar('^');
-			else if (opt->left_right) {
-				if (commit->object.flags & SYMMETRIC_LEFT)
-					putchar('<');
-				else
-					putchar('>');
-			}
-		}
+		if (!opt->graph)
+			fputs(get_revision_mark(opt, commit), stdout);
 		fputs(find_unique_abbrev(commit->object.sha1, abbrev_commit), stdout);
 		if (opt->print_parents)
 			show_parents(commit, abbrev_commit);
@@ -448,18 +438,8 @@ void show_log(struct rev_info *opt)
 		if (opt->commit_format != CMIT_FMT_ONELINE)
 			fputs("commit ", stdout);
 
-		if (!opt->graph) {
-			if (commit->object.flags & BOUNDARY)
-				putchar('-');
-			else if (commit->object.flags & UNINTERESTING)
-				putchar('^');
-			else if (opt->left_right) {
-				if (commit->object.flags & SYMMETRIC_LEFT)
-					putchar('<');
-				else
-					putchar('>');
-			}
-		}
+		if (!opt->graph)
+			fputs(get_revision_mark(opt, commit), stdout);
 		fputs(find_unique_abbrev(commit->object.sha1, abbrev_commit),
 		      stdout);
 		if (opt->print_parents)
diff --git a/pretty.c b/pretty.c
index 8549934..f21a30c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -859,11 +859,7 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 		                              c->abbrev_parent_hashes.off;
 		return 1;
 	case 'm':		/* left/right/bottom */
-		strbuf_addch(sb, (commit->object.flags & BOUNDARY)
-		                 ? '-'
-		                 : (commit->object.flags & SYMMETRIC_LEFT)
-		                 ? '<'
-		                 : '>');
+		strbuf_addstr(sb, get_revision_mark(NULL, commit));
 		return 1;
 	case 'd':
 		format_decoration(sb, commit);
diff --git a/revision.c b/revision.c
index 1fcaeb7..49d9ba8 100644
--- a/revision.c
+++ b/revision.c
@@ -2263,3 +2263,20 @@ struct commit *get_revision(struct rev_info *revs)
 		graph_update(revs->graph, c);
 	return c;
 }
+
+char *get_revision_mark(const struct rev_info *revs, const struct commit *commit)
+{
+	if (commit->object.flags & BOUNDARY)
+		return "-";
+	else if (commit->object.flags & UNINTERESTING)
+		return "^";
+	else if (!revs || revs->left_right) {
+		if (commit->object.flags & SYMMETRIC_LEFT)
+			return "<";
+		else
+			return ">";
+	}
+	else if (revs->graph)
+		return "*";
+	return "";
+}
diff --git a/revision.h b/revision.h
index d2ffde1..0e4b35e 100644
--- a/revision.h
+++ b/revision.h
@@ -165,6 +165,7 @@ extern int handle_revision_arg(const char *arg, struct rev_info *revs,int flags,
 
 extern int prepare_revision_walk(struct rev_info *revs);
 extern struct commit *get_revision(struct rev_info *revs);
+extern char *get_revision_mark(const struct rev_info *revs, const struct commit *commit);
 
 extern void mark_parents_uninteresting(struct commit *commit);
 extern void mark_tree_uninteresting(struct tree *tree);
-- 
1.7.4.1.299.g567d7.dirty

[PATCHv2 6/9] revision.c: introduce --cherry-mark

[Michael J Gruber]

for marking those commits which "--cherry-pick" would drop.
The marker for those commits is '=' because '-' denotes a boundary
commit already, even though 'git cherry' uses it.

Nonequivalent commits are denoted '+' unless '--left-right' is used.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 revision.c |   21 ++++++++++++++++++---
 revision.h |    4 +++-
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/revision.c b/revision.c
index 49d9ba8..3da403e 100644
--- a/revision.c
+++ b/revision.c
@@ -535,6 +535,7 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 	int left_count = 0, right_count = 0;
 	int left_first;
 	struct patch_ids ids;
+	unsigned cherryflag;
 
 	/* First count the commits on the left and on the right */
 	for (p = list; p; p = p->next) {
@@ -576,6 +577,9 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 		commit->util = add_commit_patch_id(commit, &ids);
 	}
 
+	/* either cherry_mark or cherry_pick are true */
+	cherryflag = revs->cherry_mark ? PATCHSAME : SHOWN;
+
 	/* Check the other side */
 	for (p = list; p; p = p->next) {
 		struct commit *commit = p->item;
@@ -598,7 +602,7 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 		if (!id)
 			continue;
 		id->seen = 1;
-		commit->object.flags |= SHOWN;
+		commit->object.flags |= cherryflag;
 	}
 
 	/* Now check the original side for seen ones */
@@ -610,7 +614,7 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 		if (!ent)
 			continue;
 		if (ent->seen)
-			commit->object.flags |= SHOWN;
+			commit->object.flags |= cherryflag;
 		commit->util = NULL;
 	}
 
@@ -802,7 +806,7 @@ static int limit_list(struct rev_info *revs)
 		show(revs, newlist);
 		show_early_output = NULL;
 	}
-	if (revs->cherry_pick)
+	if (revs->cherry_pick || revs->cherry_mark)
 		cherry_pick_list(newlist, revs);
 
 	if (revs->left_only || revs->right_only)
@@ -1293,7 +1297,14 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->right_only = 1;
 	} else if (!strcmp(arg, "--count")) {
 		revs->count = 1;
+	} else if (!strcmp(arg, "--cherry-mark")) {
+		if (revs->cherry_pick)
+			die("--cherry-mark is incompatible with --cherry-pick");
+		revs->cherry_mark = 1;
+		revs->limited = 1; /* needs limit_list() */
 	} else if (!strcmp(arg, "--cherry-pick")) {
+		if (revs->cherry_mark)
+			die("--cherry-pick is incompatible with --cherry-mark");
 		revs->cherry_pick = 1;
 		revs->limited = 1;
 	} else if (!strcmp(arg, "--objects")) {
@@ -2270,6 +2281,8 @@ char *get_revision_mark(const struct rev_info *revs, const struct commit *commit
 		return "-";
 	else if (commit->object.flags & UNINTERESTING)
 		return "^";
+	else if (commit->object.flags & PATCHSAME)
+		return "=";
 	else if (!revs || revs->left_right) {
 		if (commit->object.flags & SYMMETRIC_LEFT)
 			return "<";
@@ -2278,5 +2291,7 @@ char *get_revision_mark(const struct rev_info *revs, const struct commit *commit
 	}
 	else if (revs->graph)
 		return "*";
+	else if (revs->cherry_mark)
+		return "+";
 	return "";
 }
diff --git a/revision.h b/revision.h
index 0e4b35e..ee380ad 100644
--- a/revision.h
+++ b/revision.h
@@ -14,7 +14,8 @@
 #define CHILD_SHOWN	(1u<<6)
 #define ADDED		(1u<<7)	/* Parents already parsed and added? */
 #define SYMMETRIC_LEFT	(1u<<8)
-#define ALL_REV_FLAGS	((1u<<9)-1)
+#define PATCHSAME	(1u<<10)
+#define ALL_REV_FLAGS	((1u<<10)-1)
 
 #define DECORATE_SHORT_REFS	1
 #define DECORATE_FULL_REFS	2
@@ -68,6 +69,7 @@ struct rev_info {
 			reverse:1,
 			reverse_output_stage:1,
 			cherry_pick:1,
+			cherry_mark:1,
 			bisect:1,
 			ancestry_path:1,
 			first_parent_only:1;
-- 
1.7.4.1.299.g567d7.dirty

Re: [PATCHv2 6/9] revision.c: introduce --cherry-mark

[Junio C Hamano]

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

> for marking those commits which "--cherry-pick" would drop.

Please do not start your second paragraph with a half-sentence.

> The marker for those commits is '=' because '-' denotes a boundary
> commit already, even though 'git cherry' uses it.
>
> Nonequivalent commits are denoted '+' unless '--left-right' is used.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  revision.c |   21 ++++++++++++++++++---
>  revision.h |    4 +++-
>  2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index 49d9ba8..3da403e 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -535,6 +535,7 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
>  	int left_count = 0, right_count = 0;
>  	int left_first;
>  	struct patch_ids ids;
> +	unsigned cherryflag;

s/cherryflag/cherry_flag/;

> diff --git a/revision.h b/revision.h
> index 0e4b35e..ee380ad 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -14,7 +14,8 @@
>  #define CHILD_SHOWN	(1u<<6)
>  #define ADDED		(1u<<7)	/* Parents already parsed and added? */
>  #define SYMMETRIC_LEFT	(1u<<8)
> -#define ALL_REV_FLAGS	((1u<<9)-1)
> +#define PATCHSAME	(1u<<10)
> +#define ALL_REV_FLAGS	((1u<<10)-1)

I think you meant (1u<<9) so that ALL can cover that bit with
((1u<<10)-1).

[PATCHv2+ 6/9] revision.c: introduce --cherry-mark

[Michael J Gruber]

Introduce --cherry-mark for marking those commits which "--cherry-pick"
would drop.  The marker for those commits is '=' because '-' denotes a
boundary commit already, even though 'git cherry' uses it.

Nonequivalent commits are denoted '+' unless '--left-right' is used.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Junio C Hamano venit, vidit, dixit 09.03.2011 22:29:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> for marking those commits which "--cherry-pick" would drop.
>
> Please do not start your second paragraph with a half-sentence.

I try to get rid of that acquired habit!

>> The marker for those commits is '=' because '-' denotes a boundary
>> commit already, even though 'git cherry' uses it.
>>
>> Nonequivalent commits are denoted '+' unless '--left-right' is used.
>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> ---
>>  revision.c |   21 ++++++++++++++++++---
>>  revision.h |    4 +++-
>>  2 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/revision.c b/revision.c
>> index 49d9ba8..3da403e 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -535,6 +535,7 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
>>  	int left_count = 0, right_count = 0;
>>  	int left_first;
>>  	struct patch_ids ids;
>> +	unsigned cherryflag;
>
> s/cherryflag/cherry_flag/;

OK.

>> diff --git a/revision.h b/revision.h
>> index 0e4b35e..ee380ad 100644
>> --- a/revision.h
>> +++ b/revision.h
>> @@ -14,7 +14,8 @@
>>  #define CHILD_SHOWN	(1u<<6)
>>  #define ADDED		(1u<<7)	/* Parents already parsed and added? */
>>  #define SYMMETRIC_LEFT	(1u<<8)
>> -#define ALL_REV_FLAGS	((1u<<9)-1)
>> +#define PATCHSAME	(1u<<10)
>> +#define ALL_REV_FLAGS	((1u<<10)-1)
>
> I think you meant (1u<<9) so that ALL can cover that bit with
> ((1u<<10)-1).

Thanks for reading so carefully. What a blunder, and who knows when we would
have noticed (not with the current users of ALL_REV_FLAGS).

Oh wait, my v3 is patch-equivalent to what you have in pu already. How did I
find out? "git log1 --cherry origin/pu...", of course :) At least I changed the
message.

Michael

In case someone wonders:
git help log1
`git log1' is aliased to `log --abbrev-commit --pretty=oneline --decorate'

 revision.c |   21 ++++++++++++++++++---
 revision.h |    4 +++-
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/revision.c b/revision.c
index 0de1608..36022a6 100644
--- a/revision.c
+++ b/revision.c
@@ -535,6 +535,7 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 	int left_count = 0, right_count = 0;
 	int left_first;
 	struct patch_ids ids;
+	unsigned cherry_flag;
 
 	/* First count the commits on the left and on the right */
 	for (p = list; p; p = p->next) {
@@ -576,6 +577,9 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 		commit->util = add_commit_patch_id(commit, &ids);
 	}
 
+	/* either cherry_mark or cherry_pick are true */
+	cherry_flag = revs->cherry_mark ? PATCHSAME : SHOWN;
+
 	/* Check the other side */
 	for (p = list; p; p = p->next) {
 		struct commit *commit = p->item;
@@ -598,7 +602,7 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 		if (!id)
 			continue;
 		id->seen = 1;
-		commit->object.flags |= SHOWN;
+		commit->object.flags |= cherry_flag;
 	}
 
 	/* Now check the original side for seen ones */
@@ -610,7 +614,7 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 		if (!ent)
 			continue;
 		if (ent->seen)
-			commit->object.flags |= SHOWN;
+			commit->object.flags |= cherry_flag;
 		commit->util = NULL;
 	}
 
@@ -802,7 +806,7 @@ static int limit_list(struct rev_info *revs)
 		show(revs, newlist);
 		show_early_output = NULL;
 	}
-	if (revs->cherry_pick)
+	if (revs->cherry_pick || revs->cherry_mark)
 		cherry_pick_list(newlist, revs);
 
 	if (revs->left_only || revs->right_only)
@@ -1293,7 +1297,14 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->right_only = 1;
 	} else if (!strcmp(arg, "--count")) {
 		revs->count = 1;
+	} else if (!strcmp(arg, "--cherry-mark")) {
+		if (revs->cherry_pick)
+			die("--cherry-mark is incompatible with --cherry-pick");
+		revs->cherry_mark = 1;
+		revs->limited = 1; /* needs limit_list() */
 	} else if (!strcmp(arg, "--cherry-pick")) {
+		if (revs->cherry_mark)
+			die("--cherry-pick is incompatible with --cherry-mark");
 		revs->cherry_pick = 1;
 		revs->limited = 1;
 	} else if (!strcmp(arg, "--objects")) {
@@ -2270,6 +2281,8 @@ char *get_revision_mark(const struct rev_info *revs, const struct commit *commit
 		return "-";
 	else if (commit->object.flags & UNINTERESTING)
 		return "^";
+	else if (commit->object.flags & PATCHSAME)
+		return "=";
 	else if (!revs || revs->left_right) {
 		if (commit->object.flags & SYMMETRIC_LEFT)
 			return "<";
@@ -2277,5 +2290,7 @@ char *get_revision_mark(const struct rev_info *revs, const struct commit *commit
 			return ">";
 	} else if (revs->graph)
 		return "*";
+	else if (revs->cherry_mark)
+		return "+";
 	return "";
 }
diff --git a/revision.h b/revision.h
index 0e4b35e..d38f135 100644
--- a/revision.h
+++ b/revision.h
@@ -14,7 +14,8 @@
 #define CHILD_SHOWN	(1u<<6)
 #define ADDED		(1u<<7)	/* Parents already parsed and added? */
 #define SYMMETRIC_LEFT	(1u<<8)
-#define ALL_REV_FLAGS	((1u<<9)-1)
+#define PATCHSAME	(1u<<9)
+#define ALL_REV_FLAGS	((1u<<10)-1)
 
 #define DECORATE_SHORT_REFS	1
 #define DECORATE_FULL_REFS	2
@@ -68,6 +69,7 @@ struct rev_info {
 			reverse:1,
 			reverse_output_stage:1,
 			cherry_pick:1,
+			cherry_mark:1,
 			bisect:1,
 			ancestry_path:1,
 			first_parent_only:1;
-- 
1.7.4.1.317.gf445f

[PATCHv2 7/9] rev-list: documentation and test for --cherry-mark

[Michael J Gruber]

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

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index 5f47a13..8a891ca 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -33,6 +33,7 @@ SYNOPSIS
 	     [ \--left-right ]
 	     [ \--left-only ]
 	     [ \--right-only ]
+	     [ \--cherry-mark ]
 	     [ \--cherry-pick ]
 	     [ \--encoding[=<encoding>] ]
 	     [ \--(author|committer|grep)=<pattern> ]
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index cebba62..4755b83 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -305,6 +305,11 @@ ifdef::git-rev-list[]
 	to /dev/null as the output does not have to be formatted.
 endif::git-rev-list[]
 
+--cherry-mark::
+
+	Like `--cherry-pick` (see below) but mark equivalent commits
+	with `=` rather than omitting them, and inequivalent ones with `+`.
+
 --cherry-pick::
 
 	Omit any commit that introduces the same change as
diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index cd089a9..37bd25e 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -100,6 +100,34 @@ test_expect_success '--cherry-pick bar does not come up empty (II)' '
 '
 
 cat >expect <<EOF
++tags/F
+=tags/D
++tags/E
+=tags/C
+EOF
+
+test_expect_success '--cherry-mark' '
+	git rev-list --cherry-mark F...E -- bar > actual &&
+	git name-rev --stdin --name-only --refs="*tags/*" \
+		< actual > actual.named &&
+	test_cmp actual.named expect
+'
+
+cat >expect <<EOF
+<tags/F
+=tags/D
+>tags/E
+=tags/C
+EOF
+
+test_expect_success '--cherry-mark --left-right' '
+	git rev-list --cherry-mark --left-right 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
 
-- 
1.7.4.1.299.g567d7.dirty

[PATCHv2 8/9] log --cherry: a synonym

[Michael J Gruber]

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-mark and --no-merges.  Make it so.

In other words, this provides "git cherry with rev-list interface".

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/rev-list-options.txt |    8 ++++++++
 revision.c                         |   10 +++++++++-
 2 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 4755b83..95d209c 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -337,6 +337,14 @@ 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-mark --no-merges`; useful to
+	limit the output to the commits on our side and mark those that
+	have been applied to the other side of a forked history with
+	`git log --cherry upstream...mybranch`, similar to
+	`git cherry upstream mybranch`.
+
 -g::
 --walk-reflogs::
 
diff --git a/revision.c b/revision.c
index 3da403e..61f571e 100644
--- a/revision.c
+++ b/revision.c
@@ -1289,12 +1289,20 @@ 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_mark = 1;
+		revs->right_only = 1;
+		revs->no_merges = 1;
+		revs->limited = 1;
 	} else if (!strcmp(arg, "--count")) {
 		revs->count = 1;
 	} else if (!strcmp(arg, "--cherry-mark")) {
-- 
1.7.4.1.299.g567d7.dirty

[PATCHv2 9/9] 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 |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index 37bd25e..cacf3de 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -145,6 +145,18 @@ test_expect_success '--cherry-pick --left-only' '
 	test_cmp actual.named expect
 '
 
+cat >expect <<EOF
++tags/E
+=tags/C
+EOF
+
+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 with independent, but identical branches' '
 	git symbolic-ref HEAD refs/heads/independent &&
 	rm .git/index &&
-- 
1.7.4.1.299.g567d7.dirty

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

[Junio C Hamano]

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.

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?

Thanks.

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 09.03.2011 22:49:
> 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.)

> 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

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

[Junio C Hamano]

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

> Junio C Hamano venit, vidit, dixit 09.03.2011 22:49:
> ...
>> 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.

I didn't have trouble with "if (revs->cherry_mark)" used for that purpose
at all.

That is what I meant by "cherry-mark is a _subset_ of cherry-pick".  In
other words, if you are marking the cherry-pick result, you must be doing
a cherry-pick to begin with, so having to say "cherry-pick || cherry-mark"
is a bad taste.

But that does not contradict with the need to do something differently
between the case where you are cherry-marking and where you are not
cherry-marking.

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

I think that essentially is what I speculated in "either X or Y" part as
solution Y.

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

Well, as I said earlier, my suggestion was coming from "if you are
cherry-marking, you must be cherry-picking in the first place", so yes, I
am saying that "at most one is set" is a bad taste.  If the assumption has
to change to match the better taste, that can only be a good thing ;-).

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

I personally don't care very much and also choose not to say much on it
too early even if I had opinion on the choice, because I know there are
many people with better visual tastes than I on the list and I am sure
they will voice their opinion once they see sample output.

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 10.03.2011 10:56:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Junio C Hamano venit, vidit, dixit 09.03.2011 22:49:
>> ...
>>> 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.
> 
> I didn't have trouble with "if (revs->cherry_mark)" used for that purpose
> at all.
> 
> That is what I meant by "cherry-mark is a _subset_ of cherry-pick".  In
> other words, if you are marking the cherry-pick result, you must be doing
> a cherry-pick to begin with, so having to say "cherry-pick || cherry-mark"
> is a bad taste.
> 
> But that does not contradict with the need to do something differently
> between the case where you are cherry-marking and where you are not
> cherry-marking.
> 
>> - 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
> 
> I think that essentially is what I speculated in "either X or Y" part as
> solution Y.
> 
>> 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"
> 
> Well, as I said earlier, my suggestion was coming from "if you are
> cherry-marking, you must be cherry-picking in the first place", so yes, I
> am saying that "at most one is set" is a bad taste.  If the assumption has
> to change to match the better taste, that can only be a good thing ;-).

Being half-way through doing "Y" I remember why I really disliked "Y":
because it breaks the association between option names (--cherry-pick,
--cherry-mark) and rev flags (cherry_pick, cherry_mark). With "Y",
cherry_pick would mean --cherry-pick or --cherry-mark. (Also, as
mentioned, we're not "picking" when marking.)

Additionally, since parse_revision_opt (which calls handle_revision_opt)
is called from other sites for individual args we would need to do the
handling in the Y case (set pick when marking) right in
handle_revision_opt, not just in setup_revisions. It's a matter of a few
more if's and or's, but still.

Taking these together, I wonder whether we shouldn't leave it as in v2.

Michael

[PATCHv3 00/10] --left/right-only and --cherry-mark

[Michael J Gruber]

Anyway, this is what it looks like with "Y" (even though I prefer "X"
because of the option to flag correspondence.

v3 is v2 plus what we have discussed (pu had v2 plus Junio's amends).
In addition, 6 and 9 use "Y" rather than "X".

To make it less boring, I added 10 which changes the output format with
commit marks. See an example there and below. (10 is independent of the
choice of X vs. Y, of course.)

All together, this gives output (from my branch) like:

git-log --oneline --cherry origin/pu...

+ dfe27d4 git-log: put space after commit mark
= ca14e3f t6007: test rev-list --cherry
+ e5a72f0 log --cherry: a synonym
= fd5b652 rev-list: documentation and test for --cherry-mark
+ c17a824 revision.c: introduce --cherry-mark
= 3d5377a rev-list/log: factor out revision mark generation


git-log --oneline --graph --cherry origin/pu...

* dfe27d4 git-log: put space after commit mark
= ca14e3f t6007: test rev-list --cherry
* e5a72f0 log --cherry: a synonym
= fd5b652 rev-list: documentation and test for --cherry-mark
* c17a824 revision.c: introduce --cherry-mark
= 3d5377a rev-list/log: factor out revision mark generation


git-log --oneline --graph --cherry-mark origin/pu...

* dfe27d4 git-log: put space after commit mark
= ca14e3f t6007: test rev-list --cherry
* e5a72f0 log --cherry: a synonym
= fd5b652 rev-list: documentation and test for --cherry-mark
* c17a824 revision.c: introduce --cherry-mark
= 3d5377a rev-list/log: factor out revision mark generation
*   0cb35e5 Merge branch 'mm/maint-log-n-with-diff-filtering' into pu
|\  
| * 251df09 log: fix --max-count when used together with -S or -G
* |   803ba88 Merge branch 'jk/edit-notes-in-commit-log' into pu
...
|  / / / / / / / / / /  
* | | | | | | | | | |   f1b3ae0 Merge branch 'mg/rev-list-one-side-only' into pu
|\ \ \ \ \ \ \ \ \ \ \  
| = | | | | | | | | | | fe3b59e t6007: test rev-list --cherry
| * | | | | | | | | | | 94f605e log --cherry: a synonym
| = | | | | | | | | | | cb56e30 rev-list: documentation and test for --cherry-mark
| * | | | | | | | | | | adbbb31 revision.c: introduce --cherry-mark
| = | | | | | | | | | | 1df2d65 rev-list/log: factor out revision mark generation

And all this in beautiful color (and full log interface), of course! Compare with:

git cherry origin/pu -v
- 3d5377ae86c22a73bdce7896abe2953019039e0e rev-list/log: factor out revision mark generation
+ c17a82478307a5a29455de09232087b17435c52a revision.c: introduce --cherry-mark
- fd5b65227573cb8d5ff7c8087814f9b8f8e0851e rev-list: documentation and test for --cherry-mark
+ e5a72f0ddc0ddc9b6087c08143b188043f3bea1f log --cherry: a synonym
- ca14e3fb22b91653086fbd8c8600f2f93409a0a6 t6007: test rev-list --cherry
+ dfe27d480258ab4af4fa50628a9bbaff33ba6860 git-log: put space after commit mark

Let the bike-shedding begin!

Junio C Hamano (1):
  rev-list: --left/right-only are mutually exclusive

Michael J Gruber (9):
  revlist.c: introduce --left/right-only for unsymmetric picking
  t6007: Make sure we test --cherry-pick
  rev-list: documentation and test for --left/right-only
  rev-list/log: factor out revision mark generation
  revision.c: introduce --cherry-mark
  rev-list: documentation and test for --cherry-mark
  log --cherry: a synonym
  t6007: test rev-list --cherry
  git-log: put space after commit mark

 Documentation/git-rev-list.txt       |    3 +
 Documentation/rev-list-options.txt   |   26 ++++++++
 builtin/rev-list.c                   |   14 +----
 git-svn.perl                         |    2 +-
 graph.c                              |   17 +-----
 log-tree.c                           |   28 +-------
 pretty.c                             |    6 +--
 revision.c                           |   81 +++++++++++++++++++++++-
 revision.h                           |    8 ++-
 t/t6007-rev-list-cherry-pick-file.sh |  113 +++++++++++++++++++++++++++++++---
 10 files changed, 230 insertions(+), 68 deletions(-)

-- 
1.7.4.1.317.gf445f

[PATCHv3 01/10] 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} | sed -ne 's/^+ //p'

and potentially more useful than

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

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 revision.c |   24 ++++++++++++++++++++++++
 revision.h |    2 ++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/revision.c b/revision.c
index 7b9eaef..0681c7c 100644
--- a/revision.c
+++ b/revision.c
@@ -733,6 +733,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;
@@ -788,6 +805,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);
@@ -1263,6 +1283,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 05659c6..d2ffde1 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.317.gf445f

[PATCHv3 02/10] 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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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.317.gf445f

[PATCHv3 03/10] rev-list: documentation and test for --left/right-only

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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..cebba62 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 --no-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.317.gf445f

[PATCHv3 04/10] rev-list: --left/right-only are mutually exclusive

[Michael J Gruber]

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

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

[PATCHv3 05/10] rev-list/log: factor out revision mark generation

[Michael J Gruber]

Currently, we have identical code for generating revision marks ('<',
'>', '-') in 5 places.

Factor out the code to a single function get_revsion_mark() for easier
maintenance and extensibility.

Note that the check for !!revs in graph.c (which gets removed
effectively by this patch) is superfluous.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/rev-list.c |   14 ++------------
 graph.c            |   17 ++---------------
 log-tree.c         |   28 ++++------------------------
 pretty.c           |    6 +-----
 revision.c         |   16 ++++++++++++++++
 revision.h         |    1 +
 6 files changed, 26 insertions(+), 56 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ba27d39..f458cb7 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -64,18 +64,8 @@ static void show_commit(struct commit *commit, void *data)
 	if (info->header_prefix)
 		fputs(info->header_prefix, stdout);
 
-	if (!revs->graph) {
-		if (commit->object.flags & BOUNDARY)
-			putchar('-');
-		else if (commit->object.flags & UNINTERESTING)
-			putchar('^');
-		else if (revs->left_right) {
-			if (commit->object.flags & SYMMETRIC_LEFT)
-				putchar('<');
-			else
-				putchar('>');
-		}
-	}
+	if (!revs->graph)
+		fputs(get_revision_mark(revs, commit), stdout);
 	if (revs->abbrev_commit && revs->abbrev)
 		fputs(find_unique_abbrev(commit->object.sha1, revs->abbrev),
 		      stdout);
diff --git a/graph.c b/graph.c
index f1a63c2..ef2e24e 100644
--- a/graph.c
+++ b/graph.c
@@ -798,22 +798,9 @@ static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)
 	}
 
 	/*
-	 * If revs->left_right is set, print '<' for commits that
-	 * come from the left side, and '>' for commits from the right
-	 * side.
+	 * get_revision_mark() handles all other cases without assert()
 	 */
-	if (graph->revs && graph->revs->left_right) {
-		if (graph->commit->object.flags & SYMMETRIC_LEFT)
-			strbuf_addch(sb, '<');
-		else
-			strbuf_addch(sb, '>');
-		return;
-	}
-
-	/*
-	 * Print '*' in all other cases
-	 */
-	strbuf_addch(sb, '*');
+	strbuf_addstr(sb, get_revision_mark(graph->revs, graph->commit));
 }
 
 /*
diff --git a/log-tree.c b/log-tree.c
index b46ed3b..1257040 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -380,18 +380,8 @@ void show_log(struct rev_info *opt)
 	if (!opt->verbose_header) {
 		graph_show_commit(opt->graph);
 
-		if (!opt->graph) {
-			if (commit->object.flags & BOUNDARY)
-				putchar('-');
-			else if (commit->object.flags & UNINTERESTING)
-				putchar('^');
-			else if (opt->left_right) {
-				if (commit->object.flags & SYMMETRIC_LEFT)
-					putchar('<');
-				else
-					putchar('>');
-			}
-		}
+		if (!opt->graph)
+			fputs(get_revision_mark(opt, commit), stdout);
 		fputs(find_unique_abbrev(commit->object.sha1, abbrev_commit), stdout);
 		if (opt->print_parents)
 			show_parents(commit, abbrev_commit);
@@ -448,18 +438,8 @@ void show_log(struct rev_info *opt)
 		if (opt->commit_format != CMIT_FMT_ONELINE)
 			fputs("commit ", stdout);
 
-		if (!opt->graph) {
-			if (commit->object.flags & BOUNDARY)
-				putchar('-');
-			else if (commit->object.flags & UNINTERESTING)
-				putchar('^');
-			else if (opt->left_right) {
-				if (commit->object.flags & SYMMETRIC_LEFT)
-					putchar('<');
-				else
-					putchar('>');
-			}
-		}
+		if (!opt->graph)
+			fputs(get_revision_mark(opt, commit), stdout);
 		fputs(find_unique_abbrev(commit->object.sha1, abbrev_commit),
 		      stdout);
 		if (opt->print_parents)
diff --git a/pretty.c b/pretty.c
index 8549934..f21a30c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -859,11 +859,7 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 		                              c->abbrev_parent_hashes.off;
 		return 1;
 	case 'm':		/* left/right/bottom */
-		strbuf_addch(sb, (commit->object.flags & BOUNDARY)
-		                 ? '-'
-		                 : (commit->object.flags & SYMMETRIC_LEFT)
-		                 ? '<'
-		                 : '>');
+		strbuf_addstr(sb, get_revision_mark(NULL, commit));
 		return 1;
 	case 'd':
 		format_decoration(sb, commit);
diff --git a/revision.c b/revision.c
index 1fcaeb7..0de1608 100644
--- a/revision.c
+++ b/revision.c
@@ -2263,3 +2263,19 @@ struct commit *get_revision(struct rev_info *revs)
 		graph_update(revs->graph, c);
 	return c;
 }
+
+char *get_revision_mark(const struct rev_info *revs, const struct commit *commit)
+{
+	if (commit->object.flags & BOUNDARY)
+		return "-";
+	else if (commit->object.flags & UNINTERESTING)
+		return "^";
+	else if (!revs || revs->left_right) {
+		if (commit->object.flags & SYMMETRIC_LEFT)
+			return "<";
+		else
+			return ">";
+	} else if (revs->graph)
+		return "*";
+	return "";
+}
diff --git a/revision.h b/revision.h
index d2ffde1..0e4b35e 100644
--- a/revision.h
+++ b/revision.h
@@ -165,6 +165,7 @@ extern int handle_revision_arg(const char *arg, struct rev_info *revs,int flags,
 
 extern int prepare_revision_walk(struct rev_info *revs);
 extern struct commit *get_revision(struct rev_info *revs);
+extern char *get_revision_mark(const struct rev_info *revs, const struct commit *commit);
 
 extern void mark_parents_uninteresting(struct commit *commit);
 extern void mark_tree_uninteresting(struct tree *tree);
-- 
1.7.4.1.317.gf445f

[PATCHv3 06/10] revision.c: introduce --cherry-mark

[Michael J Gruber]

Introduce --cherry-mark for marking those commits which "--cherry-pick"
would drop.  The marker for those commits is '=' because '-' denotes a
boundary commit already, even though 'git cherry' uses it.

Nonequivalent commits are denoted '+' unless '--left-right' is used.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 revision.c |   19 +++++++++++++++++--
 revision.h |    4 +++-
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/revision.c b/revision.c
index 0de1608..864bc9b 100644
--- a/revision.c
+++ b/revision.c
@@ -535,6 +535,7 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 	int left_count = 0, right_count = 0;
 	int left_first;
 	struct patch_ids ids;
+	unsigned cherry_flag;
 
 	/* First count the commits on the left and on the right */
 	for (p = list; p; p = p->next) {
@@ -576,6 +577,8 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 		commit->util = add_commit_patch_id(commit, &ids);
 	}
 
+	cherry_flag = revs->cherry_mark ? PATCHSAME : SHOWN;
+
 	/* Check the other side */
 	for (p = list; p; p = p->next) {
 		struct commit *commit = p->item;
@@ -598,7 +601,7 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 		if (!id)
 			continue;
 		id->seen = 1;
-		commit->object.flags |= SHOWN;
+		commit->object.flags |= cherry_flag;
 	}
 
 	/* Now check the original side for seen ones */
@@ -610,7 +613,7 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 		if (!ent)
 			continue;
 		if (ent->seen)
-			commit->object.flags |= SHOWN;
+			commit->object.flags |= cherry_flag;
 		commit->util = NULL;
 	}
 
@@ -1293,7 +1296,15 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->right_only = 1;
 	} else if (!strcmp(arg, "--count")) {
 		revs->count = 1;
+	} else if (!strcmp(arg, "--cherry-mark")) {
+		if (revs->cherry_pick && !revs->cherry-mark)
+			die("--cherry-mark is incompatible with --cherry-pick");
+		revs->cherry_mark = 1;
+		revs->cherry_pick = 1;
+		revs->limited = 1; /* needs limit_list() */
 	} else if (!strcmp(arg, "--cherry-pick")) {
+		if (revs->cherry_mark)
+			die("--cherry-pick is incompatible with --cherry-mark");
 		revs->cherry_pick = 1;
 		revs->limited = 1;
 	} else if (!strcmp(arg, "--objects")) {
@@ -2270,6 +2281,8 @@ char *get_revision_mark(const struct rev_info *revs, const struct commit *commit
 		return "-";
 	else if (commit->object.flags & UNINTERESTING)
 		return "^";
+	else if (commit->object.flags & PATCHSAME)
+		return "=";
 	else if (!revs || revs->left_right) {
 		if (commit->object.flags & SYMMETRIC_LEFT)
 			return "<";
@@ -2277,5 +2290,7 @@ char *get_revision_mark(const struct rev_info *revs, const struct commit *commit
 			return ">";
 	} else if (revs->graph)
 		return "*";
+	else if (revs->cherry_mark)
+		return "+";
 	return "";
 }
diff --git a/revision.h b/revision.h
index 0e4b35e..d38f135 100644
--- a/revision.h
+++ b/revision.h
@@ -14,7 +14,8 @@
 #define CHILD_SHOWN	(1u<<6)
 #define ADDED		(1u<<7)	/* Parents already parsed and added? */
 #define SYMMETRIC_LEFT	(1u<<8)
-#define ALL_REV_FLAGS	((1u<<9)-1)
+#define PATCHSAME	(1u<<9)
+#define ALL_REV_FLAGS	((1u<<10)-1)
 
 #define DECORATE_SHORT_REFS	1
 #define DECORATE_FULL_REFS	2
@@ -68,6 +69,7 @@ struct rev_info {
 			reverse:1,
 			reverse_output_stage:1,
 			cherry_pick:1,
+			cherry_mark:1,
 			bisect:1,
 			ancestry_path:1,
 			first_parent_only:1;
-- 
1.7.4.1.317.gf445f

[PATCHv3 07/10] rev-list: documentation and test for --cherry-mark

[Michael J Gruber]

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

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index 5f47a13..8a891ca 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -33,6 +33,7 @@ SYNOPSIS
 	     [ \--left-right ]
 	     [ \--left-only ]
 	     [ \--right-only ]
+	     [ \--cherry-mark ]
 	     [ \--cherry-pick ]
 	     [ \--encoding[=<encoding>] ]
 	     [ \--(author|committer|grep)=<pattern> ]
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index cebba62..4755b83 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -305,6 +305,11 @@ ifdef::git-rev-list[]
 	to /dev/null as the output does not have to be formatted.
 endif::git-rev-list[]
 
+--cherry-mark::
+
+	Like `--cherry-pick` (see below) but mark equivalent commits
+	with `=` rather than omitting them, and inequivalent ones with `+`.
+
 --cherry-pick::
 
 	Omit any commit that introduces the same change as
diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index cd089a9..37bd25e 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -100,6 +100,34 @@ test_expect_success '--cherry-pick bar does not come up empty (II)' '
 '
 
 cat >expect <<EOF
++tags/F
+=tags/D
++tags/E
+=tags/C
+EOF
+
+test_expect_success '--cherry-mark' '
+	git rev-list --cherry-mark F...E -- bar > actual &&
+	git name-rev --stdin --name-only --refs="*tags/*" \
+		< actual > actual.named &&
+	test_cmp actual.named expect
+'
+
+cat >expect <<EOF
+<tags/F
+=tags/D
+>tags/E
+=tags/C
+EOF
+
+test_expect_success '--cherry-mark --left-right' '
+	git rev-list --cherry-mark --left-right 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
 
-- 
1.7.4.1.317.gf445f

[PATCHv3 08/10] log --cherry: a synonym

[Michael J Gruber]

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-mark and --no-merges.  Make it so.

In other words, this provides "git cherry with rev-list interface".

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/rev-list-options.txt |    8 ++++++++
 revision.c                         |   13 +++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 4755b83..95d209c 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -337,6 +337,14 @@ 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-mark --no-merges`; useful to
+	limit the output to the commits on our side and mark those that
+	have been applied to the other side of a forked history with
+	`git log --cherry upstream...mybranch`, similar to
+	`git cherry upstream mybranch`.
+
 -g::
 --walk-reflogs::
 
diff --git a/revision.c b/revision.c
index 864bc9b..4881263 100644
--- a/revision.c
+++ b/revision.c
@@ -1288,16 +1288,25 @@ 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_mark = 1;
+		revs->cherry_pick = 1;
+		revs->right_only = 1;
+		revs->no_merges = 1;
+		revs->limited = 1;
 	} else if (!strcmp(arg, "--count")) {
 		revs->count = 1;
 	} else if (!strcmp(arg, "--cherry-mark")) {
-		if (revs->cherry_pick && !revs->cherry-mark)
+		if (revs->cherry_pick && !revs->cherry_mark)
 			die("--cherry-mark is incompatible with --cherry-pick");
 		revs->cherry_mark = 1;
 		revs->cherry_pick = 1;
-- 
1.7.4.1.317.gf445f

[PATCHv3 09/10] 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 |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index 37bd25e..cacf3de 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -145,6 +145,18 @@ test_expect_success '--cherry-pick --left-only' '
 	test_cmp actual.named expect
 '
 
+cat >expect <<EOF
++tags/E
+=tags/C
+EOF
+
+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 with independent, but identical branches' '
 	git symbolic-ref HEAD refs/heads/independent &&
 	rm .git/index &&
-- 
1.7.4.1.317.gf445f

[PATCHv3 10/10] git-log: put space after commit mark

[Michael J Gruber]

Currently, commit marks (left, right, boundary, cherry) are output right
before the commit sha1, which makes it difficult to copy sha1s. Sample
output for "git log --oneline --cherry":

=049c269 t6007: test rev-list --cherry

Change this to

= 049c269 t6007: test rev-list --cherry

which matches exactly the current output of "git log --graph".

Leave "git rev-list" output as is (no space) so that they do not break.

Adjust "git-svn" which uses "git log --pretty=raw --boundary".

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 git-svn.perl |    2 +-
 log-tree.c   |    4 ++--
 revision.c   |    9 +++++++++
 revision.h   |    1 +
 4 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 177dd25..a5857c1 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -5734,7 +5734,7 @@ sub cmd_show_log {
 	my (@k, $c, $d, $stat);
 	my $esc_color = qr/(?:\033\[(?:(?:\d+;)*\d*)?m)*/;
 	while (<$log>) {
-		if (/^${esc_color}commit -?($::sha1_short)/o) {
+		if (/^${esc_color}commit (- )?($::sha1_short)/o) {
 			my $cmt = $1;
 			if ($c && cmt_showable($c) && $c->{r} != $r_last) {
 				$r_last = $c->{r};
diff --git a/log-tree.c b/log-tree.c
index 1257040..2a1e3a9 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -381,7 +381,7 @@ void show_log(struct rev_info *opt)
 		graph_show_commit(opt->graph);
 
 		if (!opt->graph)
-			fputs(get_revision_mark(opt, commit), stdout);
+			put_revision_mark(opt, commit);
 		fputs(find_unique_abbrev(commit->object.sha1, abbrev_commit), stdout);
 		if (opt->print_parents)
 			show_parents(commit, abbrev_commit);
@@ -439,7 +439,7 @@ void show_log(struct rev_info *opt)
 			fputs("commit ", stdout);
 
 		if (!opt->graph)
-			fputs(get_revision_mark(opt, commit), stdout);
+			put_revision_mark(opt, commit);
 		fputs(find_unique_abbrev(commit->object.sha1, abbrev_commit),
 		      stdout);
 		if (opt->print_parents)
diff --git a/revision.c b/revision.c
index 4881263..725f9b7 100644
--- a/revision.c
+++ b/revision.c
@@ -2303,3 +2303,12 @@ char *get_revision_mark(const struct rev_info *revs, const struct commit *commit
 		return "+";
 	return "";
 }
+
+void put_revision_mark(const struct rev_info *revs, const struct commit *commit)
+{
+	char *mark = get_revision_mark(revs, commit);
+	if (!strlen(mark))
+		return;
+	fputs(mark, stdout);
+	putchar(' ');
+}
diff --git a/revision.h b/revision.h
index d38f135..1c0abf0 100644
--- a/revision.h
+++ b/revision.h
@@ -168,6 +168,7 @@ extern int handle_revision_arg(const char *arg, struct rev_info *revs,int flags,
 extern int prepare_revision_walk(struct rev_info *revs);
 extern struct commit *get_revision(struct rev_info *revs);
 extern char *get_revision_mark(const struct rev_info *revs, const struct commit *commit);
+extern void put_revision_mark(const struct rev_info *revs, const struct commit *commit);
 
 extern void mark_parents_uninteresting(struct commit *commit);
 extern void mark_tree_uninteresting(struct tree *tree);
-- 
1.7.4.1.317.gf445f

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

[Junio C Hamano]

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

> ...
> Additionally, since parse_revision_opt (which calls handle_revision_opt)
> is called from other sites for individual args we would need to do the
> handling in the Y case (set pick when marking) right in
> handle_revision_opt, not just in setup_revisions. It's a matter of a few
> more if's and or's, but still.
>
> Taking these together, I wonder whether we shouldn't leave it as in v2.

The primary downside of keeping cherry_pick and cherry_mark as independent
I see is what would happen to the "if (cherry_pick || cherry_mark)" when
somebody comes up with the next bright idea to use the change equivalence
computed in cherry_pick_list(). With the approach in v2, the natural way
of enhancing this would be to add another "|| cherry_xyzzy" there, and the
next bright idea would add another "|| cherry_frotz".

My gut feeling was that it would be a more maintainable implementation to
have an internal bit that does not have to be exposed to the UI and that
tells the revision machinery that we need to compute the equivalence, and
an enum (if the three modes of using the equivalence are incompatible with
each other) or a one-bit-per-feature bitset (if the three features that
use the equivalence can be used at the same time) that tells the machinery
what to do with the equivalence information. From the UI level, the user
only asks for the feature(s) that use(s) the equivalence information, and
asking for any of them would set the internal "need equivalence" bit.

The above is modeled after the way how revs.limited bit is used. That bit
corresponds to the "internal bit" that is flipped by many features that
can be triggered from the UI level that depends on having the history
graph before they do their work.

Counting the number of "if (limited)" and tricky codepaths around it, and
contrasting that with the number of ways we flip the "limited = 1" bit
(which grew over time), I think you can understand where my fear of
potential future complexity against "if (cherry_pick || cherry_mark)"
comes from.

As I said earlier, this was only a minor thing that nagged me. Considering
that currently there is only one place that checks if we are doing any of
the operation that requires us to have change equivalence information to
change the behaviour depending on the result, I wouldn't be too unhappy if
this feature is done in the way you did in your v2 patch.

Thanks.

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 10.03.2011 19:22:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> ...
>> Additionally, since parse_revision_opt (which calls handle_revision_opt)
>> is called from other sites for individual args we would need to do the
>> handling in the Y case (set pick when marking) right in
>> handle_revision_opt, not just in setup_revisions. It's a matter of a few
>> more if's and or's, but still.
>>
>> Taking these together, I wonder whether we shouldn't leave it as in v2.
> 
> The primary downside of keeping cherry_pick and cherry_mark as independent
> I see is what would happen to the "if (cherry_pick || cherry_mark)" when
> somebody comes up with the next bright idea to use the change equivalence
> computed in cherry_pick_list(). With the approach in v2, the natural way
> of enhancing this would be to add another "|| cherry_xyzzy" there, and the
> next bright idea would add another "|| cherry_frotz".
> 
> My gut feeling was that it would be a more maintainable implementation to
> have an internal bit that does not have to be exposed to the UI and that
> tells the revision machinery that we need to compute the equivalence, and
> an enum (if the three modes of using the equivalence are incompatible with
> each other) or a one-bit-per-feature bitset (if the three features that
> use the equivalence can be used at the same time) that tells the machinery
> what to do with the equivalence information. From the UI level, the user
> only asks for the feature(s) that use(s) the equivalence information, and
> asking for any of them would set the internal "need equivalence" bit.
> 
> The above is modeled after the way how revs.limited bit is used. That bit
> corresponds to the "internal bit" that is flipped by many features that
> can be triggered from the UI level that depends on having the history
> graph before they do their work.
> 
> Counting the number of "if (limited)" and tricky codepaths around it, and
> contrasting that with the number of ways we flip the "limited = 1" bit
> (which grew over time), I think you can understand where my fear of
> potential future complexity against "if (cherry_pick || cherry_mark)"
> comes from.

I see, thanks for the background!

Other callers of the revision walker have to remember that they need to
set revs->limited already when they want to use any limitting flags, so
revs->cherry_pick as in v3 would be no different.(*)

So maybe we should have another flag revs->cherry_process which is set
automatically when needed, after processing all options (same with
limited), and leave cherry_pick to keep track of --cherry-pick?

(*) Well, in fact they don't as long as they use the api correctly.
blame and shortlog call parse_revision_opt but use setup_revisions and
prepare_revision_walk afterwards.
pack-objects calls handle_revision_arg after setup_revisions but
prepare_revision_walk after. I was afraid of callers setting
revs->something inconsistently and then jumping into the walker.

So, maybe prepare_revision_walk is the most sensible place to set
revs->limited and revs->cherry_process when needed as indicated by other
flags (or set already), and take out their handling from the option parser?

Michael

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

[Junio C Hamano]

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

> I see, thanks for the background!
> ...
> So maybe we should have another flag revs->cherry_process which is set
> automatically when needed, after processing all options (same with
> limited), and leave cherry_pick to keep track of --cherry-pick?

I actually read the two series again and decided that we can worry about
it when somebody actually wants to add the third one. The "cherry_process"
flag modeled after "limited" would be the right approach when we do so.

So I've kept your v2 as-is (the one you saw in 'pu' last night) and added
the 10th patch from v3.

Thanks.