cherry-pick and 'log --no-walk' and ordering

cherry-pick and 'log --no-walk' and ordering

[Martin von Zweigbergk]

A while ago when I was looking at revision.c, I was surprised to see
that commits are sorted even when --no-walk is passed, but as 8e64006
(Teach revision machinery about --no-walk, 2007-07-24) points out,
this can be useful for doing

 $ git log --abbrev-commit --pretty=oneline --decorate --all --no-walk

and get the result sorted by date. However, it can also be useful
_not_ to get a result sorted by date, e.g. when doing something like
"<generate an ordered list of revisions> | git rev-list --oneline
--no-walk --stdin". Would a --no-sort option to rev-list be
appreciated or are there better solutions?

There is also cherry-pick/revert, which I _think_ does not really want
the revisions sorted. cherry-pick implicitly reverses the order of the
walk, so 'git cherry-pick branch~2..branch' applies them in the right
order (at least in the absence of clock skew). The documentation for
cherry-pick suggests "git rev-list --reverse master -- README | git
cherry-pick -n --stdin", which I think makes no sense -- this would
reverse the output from rev-list only to have it reversed again in
cherry-pick, if it wasn't for the sorting by date. I think the
--reverse passed to rev-list might even break the cherry-pick if there
are commits in non-increasing date order. This is also supported by
the fact that test still pass after applying the patch below. I think
the test cases make more sense after the patch.

Do others agree with the analysis? I suppose it's too late to change
cherry-pick to start differentiating between "git cherry-pick commit1
commit2" and "git cherry-pick commit2 commit1", but I think we should
at least update the documentation as in the patch below (or maybe even
with a --topo-order passed to cherry-pick?). We could possibly change
cherry-pick's ordering from the default ordering to topological
ordering.



Martin


Sorry about the mangled whitespace below; just for reference, not
intended to be applied.

diff --git a/Documentation/git-cherry-pick.txt
b/Documentation/git-cherry-pick.txt
index 0e170a5..454e205 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -181,7 +181,7 @@ EXAMPLES
        are in next but not HEAD to the current branch, creating a new
        commit for each new change.

-`git rev-list --reverse master -- README | git cherry-pick -n --stdin`::
+`git rev-list master -- README | git cherry-pick -n --stdin`::

        Apply the changes introduced by all commits on the master
        branch that touched README to the working tree and index,
diff --git a/t/t3508-cherry-pick-many-commits.sh
b/t/t3508-cherry-pick-many-commits.sh
index 75f7ff4..020baaf 100755
--- a/t/t3508-cherry-pick-many-commits.sh
+++ b/t/t3508-cherry-pick-many-commits.sh
@@ -164,7 +164,7 @@ test_expect_success 'cherry-pick --stdin works' '
        git checkout -f master &&
        git reset --hard first &&
        test_tick &&
-       git rev-list --reverse first..fourth | git cherry-pick --stdin &&
+       git rev-list first..fourth | git cherry-pick --stdin &&
        git diff --quiet other &&
        git diff --quiet HEAD other &&
        check_head_differs_from fourth
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index f4e6450..9e28910 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -400,7 +400,7 @@ test_expect_success '--continue of single-pick
respects -x' '

 test_expect_success '--continue respects -x in first commit in multi-pick' '
        pristine_detach initial &&
-       test_must_fail git cherry-pick -x picked anotherpick &&
+       test_must_fail git cherry-pick -x anotherpick picked &&
        echo c >foo &&
        git add foo &&
        git cherry-pick --continue &&
@@ -430,7 +430,7 @@ test_expect_success '--signoff is not
automatically propagated to resolved confl

 test_expect_success '--signoff dropped for implicit commit of
resolution, multi-pick case' '
        pristine_detach initial &&
-       test_must_fail git cherry-pick -s picked anotherpick &&
+       test_must_fail git cherry-pick -s anotherpick picked &&
        echo c >foo &&
        git add foo &&
        git cherry-pick --continue &&

Re: cherry-pick and 'log --no-walk' and ordering

[Junio C Hamano]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> There is also cherry-pick/revert, which I _think_ does not really want
> the revisions sorted.

Yes, I think sequencer.c::prepare_revs() is wrong to unconditoinally
call prepare_revision_walk().

It instead should first check the revs->pending.objects list to see
if what was given by the caller is a mere collection of individual
objects or a range expression (i.e. check if any of them is marked
with UNINTERESTING), and refrain from going into the body of the
preparation steps, which has to involve sorting.

I think we had to fix a bug in "git show" coming from a similar root
cause, but the bug manifested in the opposite direction.

Re: cherry-pick and 'log --no-walk' and ordering

[Martin von Zweigbergk]

On Fri, Aug 10, 2012 at 2:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>
>> There is also cherry-pick/revert, which I _think_ does not really want
>> the revisions sorted.
>
> Yes, I think sequencer.c::prepare_revs() is wrong to unconditoinally
> call prepare_revision_walk().
>
> It instead should first check the revs->pending.objects list to see
> if what was given by the caller is a mere collection of individual
> objects or a range expression (i.e. check if any of them is marked
> with UNINTERESTING), and refrain from going into the body of the
> preparation steps, which has to involve sorting.

Do you mean "has to involve sorting" as in "has to involve sorting in
order not to break current users of e.g. 'git log --no-walk
--branches'"  or "revision walking inherently involves sorting"? My
current working assumption is that it is the former.

I will make rev_info.no_walk a tri-state {walk, no-walk-sorted,
no-walk-unsorted}. The third state would be used from
cherry-pick/revert (and maybe git-show, although it should make no
difference). I would also expose the third state to rev-list's command
line, maybe as --no-walk=unsorted.

Actually, all but command-line parsing is done now and test seem fine,
with quite a small patch:
$ git diff --stat
 builtin/log.c    | 2 +-
 builtin/revert.c | 2 +-
 revision.c       | 5 +++--
 revision.h       | 6 +++++-
 4 files changed, 10 insertions(+), 5 deletions(-)

Did you see a problem with this approach, since you said that
sequencer shouldn't unconditionally call prepare_revision_walk()? I
can see that git-show needs to go through revs->pending.objects
because it handles tags and stuff, but cherry-pick/revert only seem to
need the revisions.

Martin

Re: cherry-pick and 'log --no-walk' and ordering

[Junio C Hamano]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> On Fri, Aug 10, 2012 at 2:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>>
>>> There is also cherry-pick/revert, which I _think_ does not really want
>>> the revisions sorted.
>>
>> Yes, I think sequencer.c::prepare_revs() is wrong to unconditoinally
>> call prepare_revision_walk().
>>
>> It instead should first check the revs->pending.objects list to see
>> if what was given by the caller is a mere collection of individual
>> objects or a range expression (i.e. check if any of them is marked
>> with UNINTERESTING), and refrain from going into the body of the
>> preparation steps, which has to involve sorting.
>
> Do you mean "has to involve sorting" as in "has to involve sorting in
> order not to break current users of e.g. 'git log --no-walk
> --branches'"  or "revision walking inherently involves sorting"?

Range limited revision walking, e.g. "git cherry-pick A..B D~4..D",
fundamentally implies sorting and you cannot assume B would appear
before D only because B comes before D on the command line (B may
even be inside D~4..D range in which case it would not even appear
in the final output).

Any caller that wants to retrieve the objects given from the command
line in the order the user gave it, e.g. "git cherry-pick A B C",
using setup_revisions() and without walking the history, must look
at revs->pending.objects without calling prepare_revision_walk().

[PATCH 0/4] Re: cherry-pick and 'log --no-walk' and ordering

[y]

From: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>

This series adds supports for 'git log --no-walk=unsorted', which
should be useful for the re-roll of my mz/rebase-range series. It also
addresses the bug in cherry-pick/revert, which makes it sort revisions
by date.

On Fri, Aug 10, 2012 at 11:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Range limited revision walking, e.g. "git cherry-pick A..B D~4..D",
> fundamentally implies sorting and you cannot assume B would appear
> before D only because B comes before D on the command line (B may
> even be inside D~4..D range in which case it would not even appear
> in the final output).

Sorry, I probably wasn't clear; I mentioned "revision walking", but I
only meant the no-walk case. I hope the patches make sense.


Martin von Zweigbergk (4):
  teach log --no-walk=unsorted, which avoids sorting
  revisions passed to cherry-pick should be in "default" order
  cherry-pick/revert: respect order of revisions to pick
  cherry-pick/revert: default to topological sorting

 Documentation/git-cherry-pick.txt   |  2 +-
 builtin/log.c                       |  2 +-
 builtin/revert.c                    |  3 ++-
 revision.c                          | 18 +++++++++++++++---
 revision.h                          |  6 +++++-
 t/t3508-cherry-pick-many-commits.sh |  2 +-
 t/t3510-cherry-pick-sequence.sh     |  4 ++--
 t/t4202-log.sh                      | 10 ++++++++++
 8 files changed, 37 insertions(+), 10 deletions(-)

-- 
1.7.11.1.104.ge7b44f1

[PATCH 1/4] teach log --no-walk=unsorted, which avoids sorting

[y]

From: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>

When 'git log' is passed the --no-walk option, no revision walk takes
place, naturally. Perhaps somewhat surprisingly, however, the provided
revisions still get sorted by commit date. So e.g 'git log --no-walk
HEAD HEAD~1' and 'git log --no-walk HEAD~1 HEAD' give the same result
(unless the two revisions share the commit date, in which case they
will retain the order given on the command line). As the commit that
introduced --no-walk (8e64006 (Teach revision machinery about
--no-walk, 2007-07-24)) points out, the sorting is intentional, to
allow things like

 git log --abbrev-commit --pretty=oneline --decorate --all --no-walk

to show all refs in order by commit date.

But there are also other cases where the sorting is not wanted, such
as

 <command producing revisions in order> |
       git log --oneline --no-walk --stdin

To accomodate both cases, leave the decision of whether or not to sort
up to the caller, by allowing --no-walk={sorted,unsorted}, defaulting
to 'sorted'.

Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
 builtin/log.c    |  2 +-
 builtin/revert.c |  2 +-
 revision.c       | 18 +++++++++++++++---
 revision.h       |  6 +++++-
 t/t4202-log.sh   | 10 ++++++++++
 5 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index ecc2793..20838b1 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -456,7 +456,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
 	rev.always_show_header = 1;
-	rev.no_walk = 1;
+	rev.no_walk = REVISION_WALK_NO_WALK_SORTED;
 	rev.diffopt.stat_width = -1; 	/* Scale to real terminal size */
 
 	memset(&opt, 0, sizeof(opt));
diff --git a/builtin/revert.c b/builtin/revert.c
index 82d1bf8..42ce399 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -193,7 +193,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		struct setup_revision_opt s_r_opt;
 		opts->revs = xmalloc(sizeof(*opts->revs));
 		init_revisions(opts->revs, NULL);
-		opts->revs->no_walk = 1;
+		opts->revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
 		if (argc < 2)
 			usage_with_options(usage_str, options);
 		memset(&s_r_opt, 0, sizeof(s_r_opt));
diff --git a/revision.c b/revision.c
index 9e8f47a..2faf675 100644
--- a/revision.c
+++ b/revision.c
@@ -1298,7 +1298,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	    !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk") ||
 	    !strcmp(arg, "--bisect") || !prefixcmp(arg, "--glob=") ||
 	    !prefixcmp(arg, "--branches=") || !prefixcmp(arg, "--tags=") ||
-	    !prefixcmp(arg, "--remotes="))
+	    !prefixcmp(arg, "--remotes=") || !prefixcmp(arg, "--no-walk="))
 	{
 		unkv[(*unkc)++] = arg;
 		return 1;
@@ -1693,7 +1693,18 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	} else if (!strcmp(arg, "--not")) {
 		*flags ^= UNINTERESTING;
 	} else if (!strcmp(arg, "--no-walk")) {
-		revs->no_walk = 1;
+		revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
+	} else if (!prefixcmp(arg, "--no-walk=")) {
+		/*
+		 * Detached form ("--no-walk X" as opposed to "--no-walk=X")
+		 * not allowed, since the argument is optional.
+		 */
+		if (!strcmp(arg + 10, "sorted"))
+			revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
+		else if (!strcmp(arg + 10, "unsorted"))
+			revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;
+		else
+			return error("invalid argument to --no-walk");
 	} else if (!strcmp(arg, "--do-walk")) {
 		revs->no_walk = 0;
 	} else {
@@ -2116,10 +2127,11 @@ int prepare_revision_walk(struct rev_info *revs)
 		}
 		e++;
 	}
-	commit_list_sort_by_date(&revs->commits);
 	if (!revs->leak_pending)
 		free(list);
 
+	if (revs->no_walk != REVISION_WALK_NO_WALK_UNSORTED)
+		commit_list_sort_by_date(&revs->commits);
 	if (revs->no_walk)
 		return 0;
 	if (revs->limited)
diff --git a/revision.h b/revision.h
index cb5ab35..a95bd0b 100644
--- a/revision.h
+++ b/revision.h
@@ -41,6 +41,10 @@ struct rev_cmdline_info {
 	} *rev;
 };
 
+#define REVISION_WALK_WALK 0
+#define REVISION_WALK_NO_WALK_SORTED 1
+#define REVISION_WALK_NO_WALK_UNSORTED 2
+
 struct rev_info {
 	/* Starting list */
 	struct commit_list *commits;
@@ -62,7 +66,7 @@ struct rev_info {
 	/* Traversal flags */
 	unsigned int	dense:1,
 			prune:1,
-			no_walk:1,
+			no_walk:2,
 			show_all:1,
 			remove_empty_trees:1,
 			simplify_history:1,
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 71be59d..bd83355 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -178,11 +178,21 @@ test_expect_success 'git log --no-walk <commits> sorts by commit time' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git log --no-walk=sorted <commits> sorts by commit time' '
+	git log --no-walk=sorted --oneline 5d31159 804a787 394ef78 > actual &&
+	test_cmp expect actual
+'
+
 cat > expect << EOF
 5d31159 fourth
 804a787 sixth
 394ef78 fifth
 EOF
+test_expect_success 'git log --no-walk=unsorted <commits> leaves list of commits as given' '
+	git log --no-walk=unsorted --oneline 5d31159 804a787 394ef78 > actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'git show <commits> leaves list of commits as given' '
 	git show --oneline -s 5d31159 804a787 394ef78 > actual &&
 	test_cmp expect actual
-- 
1.7.11.1.104.ge7b44f1

[PATCH 2/4] revisions passed to cherry-pick should be in "default" order

[y]

From: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>

'git cherry-pick' internally sets the --reverse option while walking
revisions, so that 'git cherry-pick branch@{u}..branch' will apply the
revisions starting at the oldest one. If no uninteresing revisions are
given, --no-walk is implied. Still, the documentation for 'git
cherry-pick --stdin' uses the following example:

 git rev-list --reverse master -- README | git cherry-pick -n --stdin

The above would seem to reverse the revisions in the output (which it
does), and then pipe them to 'git cherry-pick', which would reverse
them again and apply them in the wrong order. The same problem occurs
when supplying revisions explicitly on the command line instead of
sending them to stdin.

Because of the sorting-by-date that is done by the revision walker
(even with the implied --no-walk), the ordering in the output from
'git rev-list' in the example above is effectively ignored, and the
above actually works most of the time. However, if revisions share a
commit date (as can easily happen as a result of rebasing), they do
get applied out-of-order.

Update the documentation not to suggest reversing the input to 'git
cherry-pick'. Also update test cases where the inputs are reversed.

Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
 Documentation/git-cherry-pick.txt   | 2 +-
 t/t3508-cherry-pick-many-commits.sh | 2 +-
 t/t3510-cherry-pick-sequence.sh     | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 0e170a5..454e205 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -181,7 +181,7 @@ EXAMPLES
 	are in next but not HEAD to the current branch, creating a new
 	commit for each new change.
 
-`git rev-list --reverse master -- README | git cherry-pick -n --stdin`::
+`git rev-list master -- README | git cherry-pick -n --stdin`::
 
 	Apply the changes introduced by all commits on the master
 	branch that touched README to the working tree and index,
diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh
index 75f7ff4..020baaf 100755
--- a/t/t3508-cherry-pick-many-commits.sh
+++ b/t/t3508-cherry-pick-many-commits.sh
@@ -164,7 +164,7 @@ test_expect_success 'cherry-pick --stdin works' '
 	git checkout -f master &&
 	git reset --hard first &&
 	test_tick &&
-	git rev-list --reverse first..fourth | git cherry-pick --stdin &&
+	git rev-list first..fourth | git cherry-pick --stdin &&
 	git diff --quiet other &&
 	git diff --quiet HEAD other &&
 	check_head_differs_from fourth
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index f4e6450..9e28910 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -400,7 +400,7 @@ test_expect_success '--continue of single-pick respects -x' '
 
 test_expect_success '--continue respects -x in first commit in multi-pick' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick -x picked anotherpick &&
+	test_must_fail git cherry-pick -x anotherpick picked &&
 	echo c >foo &&
 	git add foo &&
 	git cherry-pick --continue &&
@@ -430,7 +430,7 @@ test_expect_success '--signoff is not automatically propagated to resolved confl
 
 test_expect_success '--signoff dropped for implicit commit of resolution, multi-pick case' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick -s picked anotherpick &&
+	test_must_fail git cherry-pick -s anotherpick picked &&
 	echo c >foo &&
 	git add foo &&
 	git cherry-pick --continue &&
-- 
1.7.11.1.104.ge7b44f1

[PATCH 3/4] cherry-pick/revert: respect order of revisions to pick

[y]

From: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>

'git cherry-pick A B' implicitly sends --no-walk=sorted to the
revision walker, which means that the older of A and B will be applied
first, which is most likely surprising to most. Fix this by instead
sending --no-walk=unsorted to the revision walker.

Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---

This has actually been reported before, in
http://thread.gmane.org/gmane.comp.version-control.git/164794/focus=164807,
where I apparently replied myself. Incidentally, it seems like the
unrelated bug in 'git show' I reported in that thread is the one that
Junio mentioned got fixed recently.

 builtin/revert.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 42ce399..98ad641 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -193,7 +193,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		struct setup_revision_opt s_r_opt;
 		opts->revs = xmalloc(sizeof(*opts->revs));
 		init_revisions(opts->revs, NULL);
-		opts->revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
+		opts->revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;
 		if (argc < 2)
 			usage_with_options(usage_str, options);
 		memset(&s_r_opt, 0, sizeof(s_r_opt));
-- 
1.7.11.1.104.ge7b44f1

[PATCH 4/4] cherry-pick/revert: default to topological sorting

[y]

From: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>

When 'git cherry-pick' and 'git revert' are used with ranges such as
'git cherry-pick A..B', the order of the commits to pick are
determined by the default date-based sorting. If a commit has a commit
date before the commit date of its parent, it will therfore be applied
before its parent. In the context of cherry-pick/revert, this is most
likely not what the user expected, so let's enable topological sorting
by default.

Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
 builtin/revert.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/revert.c b/builtin/revert.c
index 98ad641..6880ce5 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -194,6 +194,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		opts->revs = xmalloc(sizeof(*opts->revs));
 		init_revisions(opts->revs, NULL);
 		opts->revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;
+		opts->revs->topo_order = 1;
 		if (argc < 2)
 			usage_with_options(usage_str, options);
 		memset(&s_r_opt, 0, sizeof(s_r_opt));
-- 
1.7.11.1.104.ge7b44f1

Re: [PATCH 0/4] Re: cherry-pick and 'log --no-walk' and ordering

[Junio C Hamano]

y@google.com writes:

[Administrivia: I somehow doubt y@google.com would reach you, and
futzed with the To: line above]

> From: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
>
> This series adds supports for 'git log --no-walk=unsorted', which
> should be useful for the re-roll of my mz/rebase-range series. It also
> addresses the bug in cherry-pick/revert, which makes it sort revisions
> by date.
>
> On Fri, Aug 10, 2012 at 11:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Range limited revision walking, e.g. "git cherry-pick A..B D~4..D",
>> fundamentally implies sorting and you cannot assume B would appear
>> before D only because B comes before D on the command line (B may
>> even be inside D~4..D range in which case it would not even appear
>> in the final output).
>
> Sorry, I probably wasn't clear; I mentioned "revision walking", but I
> only meant the no-walk case. I hope the patches make sense.

I actually think --no-walk, especially when given any negative
revision, that sorts is fundamentally a flawed concept (it led to
the inconsistency that made "git show A..B C" vs "git show C A..B"
behave differently, which we had to fix recently).

Would anything break if we take your patch, but without two
possibilities to revs->no_walk option (i.e. we never sort under
no_walk)?  That is, the core of your change would become something
like this:

 revision.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 9e8f47a..589d17f 100644
--- a/revision.c
+++ b/revision.c
@@ -2116,12 +2116,12 @@ int prepare_revision_walk(struct rev_info *revs)
 		}
 		e++;
 	}
-	commit_list_sort_by_date(&revs->commits);
 	if (!revs->leak_pending)
 		free(list);
 
 	if (revs->no_walk)
 		return 0;
+	commit_list_sort_by_date(&revs->commits);
 	if (revs->limited)
 		if (limit_list(revs) < 0)
 			return -1;

Re: [PATCH 0/4] Re: cherry-pick and 'log --no-walk' and ordering

[Junio C Hamano]

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

> Would anything break if we take your patch, but without two
> possibilities to revs->no_walk option (i.e. we never sort under
> no_walk)?

By the way, by "would anything break", I do not just mean if our
existing tests trigger failures from "test_expect_success"; I
suspect some do assume the sorting behaviour.  I am wondering if the
sorting makes sense in the real users; in other words, if the
failing tests, if any, are expecting sensible and useful behaviour.

After all, the sorting by the commit timestamp is made solely to
optimize the limit_list() which wants to traverse commits ancestry
near the tip of the history, and sorting by the commit timestamp is
done because it is usually a good and quick approximation for
topological sorting.

Re: [PATCH 0/4] Re: cherry-pick and 'log --no-walk' and ordering

[Martin von Zweigbergk]

On Mon, Aug 13, 2012 at 12:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
> y@google.com writes:
>
> [Administrivia: I somehow doubt y@google.com would reach you, and
> futzed with the To: line above]

:-( Sorry, sendemail.from now set. (I apparently answered "y" instead
of just <enter> to accept the default.)

> I actually think --no-walk, especially when given any negative
> revision, that sorts is fundamentally a flawed concept (it led to
> the inconsistency that made "git show A..B C" vs "git show C A..B"
> behave differently, which we had to fix recently).

I completely agree.

> Would anything break if we take your patch, but without two
> possibilities to revs->no_walk option (i.e. we never sort under
> no_walk)?  That is, the core of your change would become something
> like this:

I also thought the sorting was just a bug. From what I understand by
looking how the code has evolved, the sorting in the no-walk case was
not intentional, but more of a consequence of the implementation. That
patch you suggested was my first attempt and led me to find the broken
cherry-pick test cases that I then fixed in patch 2/4. But, it clearly
would break the test case in t4202 called 'git log --no-walk <commits>
sorts by commit time'. So I started digging from there and found e.g.

http://thread.gmane.org/gmane.comp.version-control.git/123205/focus=123216

For convenience, I have pasted the commit message of the commit
mentioned in that thread at the end of this email.  So we would be
breaking at least Johannes's use case if we changed it. I would think
almost everyone who doesn't already know would expect "git rev-list A
B" to list them in that order, so is a migration desired? Or just
change the default for --no-walk from "sorted" to "unsorted" in git
2.0?

By the way, git-log's documentation says "By default, the commits are
shown in reverse chronological order.", which to some degree is in
support of the current behavior.


commit 8e64006eee9c82eba513b98306c179c9e2385e4e
Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date:   Tue Jul 24 00:38:40 2007 +0100

    Teach revision machinery about --no-walk

    The flag "no_walk" is present in struct rev_info since a long time, but
    so far has been in use exclusively by "git show".

    With this flag, you can see all your refs, ordered by date of the last
    commit:

    $ git log --abbrev-commit --pretty=oneline --decorate --all --no-walk

    which is extremely helpful if you have to juggle with a lot topic
    branches, and do not remember in which one you introduced that uber
    debug option, or simply want to get an overview what is cooking.

    (Note that the "git log" invocation above does not output the same as

     $ git show --abbrev-commit --pretty=oneline --decorate --all --quiet

     since "git show" keeps the alphabetic order that "--all" returns the
     refs in, even if the option "--date-order" was passed.)

    For good measure, this also adds the "--do-walk" option which overrides
    "--no-walk".

    Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: [PATCH 0/4] Re: cherry-pick and 'log --no-walk' and ordering

[Junio C Hamano]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> I also thought the sorting was just a bug. From what I understand by
> looking how the code has evolved, the sorting in the no-walk case was
> not intentional, but more of a consequence of the implementation. That
> patch you suggested was my first attempt and led me to find the broken
> cherry-pick test cases that I then fixed in patch 2/4. But, it clearly
> would break the test case in t4202 called 'git log --no-walk <commits>
> sorts by commit time'. So I started digging from there and found e.g.
>
> http://thread.gmane.org/gmane.comp.version-control.git/123205/focus=123216
>
> For convenience, I have pasted the commit message of the commit
> mentioned in that thread at the end of this email.  So we would be
> breaking at least Johannes's use case if we changed it.

Ok.  Having a way to conveniently sort based on committer date is
indeed handy, and losing it would be a regression.

Not that the accident that supports only on committer date is a
nicely designed feature.  The user may want to sort on author date
instead, but there is no way to do so with --no-walk.  So in that
sense, Johannes's use case happens to work by accident.

> ... so is a migration desired? Or just
> change the default for --no-walk from "sorted" to "unsorted" in git
> 2.0?

I think the proper support for Johannes's case should give users
more control on what to sort on, and that switch should not be tied
to "--no-walk".  After all, being able to sort commits in the result
of limit_list() with various criteria would equally useful as being
able to sort commits listed on the command line with --no-walk.
Think about what "git shortlog A..B" does, for example. It is like
first enumerating commits within the given range, and sorting the
result using author as the primary and then timestamp as the
secondary sort column.

So let's not even think about migration, and go in the direction of
giving "--no-walk" two flavours, for now.  Either it keeps the order
commits were given from the command line, or it does the default
sort using the timestamp.  We can later add the --sort-on option that
would work with or without --no-walk for people who want output that
is differently sorted, but that is outside the scope of your series.

> By the way, git-log's documentation says "By default, the commits are
> shown in reverse chronological order.", which to some degree is in
> support of the current behavior.

That is talking about the presentation order of the result of
limit_list(), predates --no-walk, and was not adjusted to the new
world order when --no-walk was introduced, so I would not take it as
a supporting argument.

But not regressing the current "you can see them sorted on the
commit timestamp (this is merely an accident and not a designed
feature, so you cannot choose to sort on other things)" behaviour is
a reason enough not to disable sorting for the plain "--no-walk"
option.

Thanks.

Re: [PATCH 0/4] Re: cherry-pick and 'log --no-walk' and ordering

[Martin von Zweigbergk]

On Mon, Aug 13, 2012 at 10:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>>
>> ... so is a migration desired? Or just
>> change the default for --no-walk from "sorted" to "unsorted" in git
>> 2.0?
>
> I think the proper support for Johannes's case should give users
> more control on what to sort on, and that switch should not be tied
> to "--no-walk".  After all, being able to sort commits in the result
> of limit_list() with various criteria would equally useful as being
> able to sort commits listed on the command line with --no-walk.
> Think about what "git shortlog A..B" does, for example. It is like
> first enumerating commits within the given range, and sorting the
> result using author as the primary and then timestamp as the
> secondary sort column.
>
> So let's not even think about migration, and go in the direction of
> giving "--no-walk" two flavours, for now.  Either it keeps the order
> commits were given from the command line, or it does the default
> sort using the timestamp.  We can later add the --sort-on option that
> would work with or without --no-walk for people who want output that
> is differently sorted, but that is outside the scope of your series.

Makes sense. The shortlog example is a good example of sorting that
completely reorders the commit graph sometimes even making sense for
ranges. Thanks!

Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order

[Junio C Hamano]

y@google.com writes:

> From: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
>
> 'git cherry-pick' internally sets the --reverse option while walking
> revisions, so that 'git cherry-pick branch@{u}..branch' will apply the
> revisions starting at the oldest one. If no uninteresing revisions are
> given, --no-walk is implied. Still, the documentation for 'git
> cherry-pick --stdin' uses the following example:
>
>  git rev-list --reverse master -- README | git cherry-pick -n --stdin
>
> The above would seem to reverse the revisions in the output (which it
> does), and then pipe them to 'git cherry-pick', which would reverse
> them again and apply them in the wrong order.

I think we have cleared this confusion up in the previous
discussion.  It it sequencer's bug that reorders the commits when
the caller ("rev-list --reverse" in this case) gives list of
individual commits to replay.

So I think we are all OK with chucking this patch.  Am I mistaken?

Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order

[Martin von Zweigbergk]

On Sun, Aug 12, 2012 at 11:27 PM,  <y@google.com> wrote:
> From: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
>
> 'git cherry-pick' internally sets the --reverse option while walking
> revisions, so that 'git cherry-pick branch@{u}..branch' will apply the
> revisions starting at the oldest one.

By the way, I can see the usefulness of --reverse when giving a range,
but I think it's a little confusing when not giving a range. So "git
cherry-pick A B" will apply B first, then A. I thought I'd mention
that explicitly in case it wasn't clear.

Re: [PATCH 4/4] cherry-pick/revert: default to topological sorting

[Junio C Hamano]

y@google.com writes:

> From: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
>
> When 'git cherry-pick' and 'git revert' are used with ranges such as
> 'git cherry-pick A..B', the order of the commits to pick are
> determined by the default date-based sorting. If a commit has a commit
> date before the commit date of its parent, it will therfore be applied
> before its parent.

Is that what --topo-order really means?

I just tried this:

	$ git checkout v1.7.12-rc2
	$ GIT_COMMITTER_DATE='@0 +0000' git commit --allow-empty -m old
        $ git log --pretty=fuller -2

and (obviously) the result shows the "old" one and then the v1.7.12-rc2.

The point of --topo-order is to deal with merges more sensibly, I
think, e.g. with a history with this shape with timestamps,

    ---1----2----4----7
        \              \
         3----5----6----8---   

"git log" may show "8 7 6 5 4 3 2 1", while "git log --topo-order"
would give you "8 6 5 3 7 4 2 1".

And indeed in the context of cherry-pick and revert, topo-order is a
more sensible option.

So there is nothing wrong in the patch, but the above explanation of
yours is flawed.

> In the context of cherry-pick/revert, this is most
> likely not what the user expected, so let's enable topological sorting
> by default.
>
> Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
> ---
>  builtin/revert.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 98ad641..6880ce5 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -194,6 +194,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>  		opts->revs = xmalloc(sizeof(*opts->revs));
>  		init_revisions(opts->revs, NULL);
>  		opts->revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;
> +		opts->revs->topo_order = 1;
>  		if (argc < 2)
>  			usage_with_options(usage_str, options);
>  		memset(&s_r_opt, 0, sizeof(s_r_opt));

Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order

[Martin von Zweigbergk]

On Mon, Aug 13, 2012 at 1:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> y@google.com writes:
>
>> From: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
>>
>> 'git cherry-pick' internally sets the --reverse option while walking
>> revisions, so that 'git cherry-pick branch@{u}..branch' will apply the
>> revisions starting at the oldest one. If no uninteresing revisions are
>> given, --no-walk is implied. Still, the documentation for 'git
>> cherry-pick --stdin' uses the following example:
>>
>>  git rev-list --reverse master -- README | git cherry-pick -n --stdin
>>
>> The above would seem to reverse the revisions in the output (which it
>> does), and then pipe them to 'git cherry-pick', which would reverse
>> them again and apply them in the wrong order.
>
> I think we have cleared this confusion up in the previous
> discussion.  It it sequencer's bug that reorders the commits when
> the caller ("rev-list --reverse" in this case) gives list of
> individual commits to replay.
>
> So I think we are all OK with chucking this patch.  Am I mistaken?

I can't really say. I suppose the current patch is smaller (it can't
really get smaller than one line), but iterating over the arguments
the sequencer level might be more correct. Would the result be
different in some cases? I would be happy to add a test case at least,
although I'm not sure when I would have time to implement it in
sequencer.

To connect to the other mail I sent on this thread (in parallel with
yours), do you think "git cherrry-pick HEAD HEAD~1" should apply the
commits in the same order as "git cherry-pick HEAD~2..HEAD" (which
would give the same result if passed to 'rev-list --no-walk' for a
linear history) or in the order specified on the command line? I
couldn't find any conclusive evidence of what was intended in either
log messages or test cases.

Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order

[Junio C Hamano]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> By the way, I can see the usefulness of --reverse when giving a range,
> but I think it's a little confusing when not giving a range.

"git rev-list --reverse --root v1.0.0" is a way to say "give me a
list of commits to be replayed in sequence" without having a bottom,
no?

Ah, you mean when we do _not_ walk.

Yeah, that is why I said that when we do not walk, we should not
even call into prepare_revision_walk() in the first place in my
earlier message.  We should take the commits as given from the
revs->pending.objects list instead.

With your "no_walk = NO_WALK_UNSORTED", calling prepare_revision_walk()
would amont to the same thing, as you would not sort the commits and
use them as given by the user.

> So "git cherry-pick A B" will apply B first, then A.

I am confused a bit.  Are you describing a buggy behaviour in the
current codebase, or are you saying we should fix it to behave that
way?

Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order

[Junio C Hamano]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> To connect to the other mail I sent on this thread (in parallel with
> yours), do you think "git cherrry-pick HEAD HEAD~1" should apply the
> commits in the same order as "git cherry-pick HEAD~2..HEAD" (which
> would give the same result if passed to 'rev-list --no-walk' for a
> linear history) or in the order specified on the command line?

Definitely the latter; I do not think of any semi-reasonable excuse
to do otherwise.

> I couldn't find any conclusive evidence of what was intended in
> either log messages or test cases.

Do not take the "multi-commit handling" that was bolted on to
cherry-pick and revert long after these commands with a single
commit form were polished and have become stable too seriously and
its behaviour cast in stone.  There is no reason to believe the
bolted-on part was designed with sufficient thoughts behind it, nor
was implemented with the same competency as the code before it was
introduced.  I recall myself applying these patches after only
cursory review, saying "Meh, I wouldn't do multiple commits anyway,
and bugs found by people can be fixed later" ;-).

It is OK to consider its doneness as "the developers declared
success based on their limited testing; it internally still sorts,
but sorting a range by timestamp happens to yield the correct result
most of the time, and this bug was not found until much later. There
certainly are other bugs, at both implementation and design level,
yet to be discovered." phase of its lifecycle.

Re: [PATCH 0/4] Re: cherry-pick and 'log --no-walk' and ordering

[Junio C Hamano]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> Makes sense. The shortlog example is a good example of sorting that
> completely reorders the commit graph sometimes even making sense for
> ranges. Thanks!

By the way, does this topic relate to the long stalled "rebase"
topic from you, and if so how?

Re: [PATCH 4/4] cherry-pick/revert: default to topological sorting

[Junio C Hamano]

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

> y@google.com writes:
>
>> From: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
>>
>> When 'git cherry-pick' and 'git revert' are used with ranges such as
>> 'git cherry-pick A..B', the order of the commits to pick are
>> determined by the default date-based sorting. If a commit has a commit
>> date before the commit date of its parent, it will therfore be applied
>> before its parent.
>
> Is that what --topo-order really means?

And it turns out that the documentation is crappy.  Perhaps
something like this, but an illustration may not hurt.

 Documentation/rev-list-options.txt | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index d9b2b5b..c147117 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -579,9 +579,10 @@ Commit Ordering
 By default, the commits are shown in reverse chronological order.
 
 --topo-order::
-
-	This option makes them appear in topological order (i.e.
-	descendant commits are shown before their parents).
+	This option makes them appear in topological order.  Even
+	without this option, descendant commits are shown before
+	their parents, but this tries to avoid showing commits on
+	multiple lines of history intermixed.
 
 --date-order::
 

Re: [PATCH 0/4] Re: cherry-pick and 'log --no-walk' and ordering

[Martin von Zweigbergk]

On Mon, Aug 13, 2012 at 2:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>
>> Makes sense. The shortlog example is a good example of sorting that
>> completely reorders the commit graph sometimes even making sense for
>> ranges. Thanks!
>
> By the way, does this topic relate to the long stalled "rebase"
> topic from you, and if so how?

Yes, but only through the first patch in the series. Unless I'm
mistaken, I would can get a list of revisions to rebase using
git-patch-id, but to convert that into a instruction list with running
git-log on each commit, I planned to use 'git rev-list --format=...
--no-walk=unsorted --stdin', which of course doesn't exist before
patch 1/4.

The rest of the current series is a little fuzzy to me, especially the
confusion about reversing or not. Feel free to split out patch 1 into
a separate topic if you like, or however you would handle that.

Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order

[Martin von Zweigbergk]

On Mon, Aug 13, 2012 at 2:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>
>> To connect to the other mail I sent on this thread (in parallel with
>> yours), do you think "git cherrry-pick HEAD HEAD~1" should apply the
>> commits in the same order as "git cherry-pick HEAD~2..HEAD" (which
>> would give the same result if passed to 'rev-list --no-walk' for a
>> linear history) or in the order specified on the command line?
>
> Definitely the latter; I do not think of any semi-reasonable excuse
> to do otherwise.

Indeed. My patches tried to fix the wrong problem.

Sorry I'm slow, but I think I'm finally starting to understand
what you've been saying all along about the bug being in
sequencer. I'll try to recapitulate a bit for my own and maybe
others' understanding. For simplicity, let's assume a linear
history with unique timestamps, but not necessarily increasing
with each commit.

Currently:

 1) 'git cherry-pick A..C' picks the commits order in
  reverse "default" order

 2) 'git cherry-pick B C' picks the commits in chronological
  order

 3) 'git rev-list --reverse A..C | git cherry-pick --stdin'
  behaves just like 'git cherry-pick B C' and therefore picks
  the commits in chronological order

In cases 2) and 3), even though cherry-pick tells the revision
walker not to walk, it still sorts the commits in reverse
chronological order. But cherry-pick also tells the revision
walker explicitly to reverse the list, so in the end, the order
is chronological.

In case 2), however, the first ordering make no difference in
this "limited" case (IIUC). So the "default" ordering (which
would be C, then B in this case, regardless of timestamps), gets
reversed and B gets applied first, followed by C.

So all of the above case give the right result in the end as long
as the timestamps are chronological, and case 1) gives the right
result regardless. The other two cases only works in most cases
because the unexpcted sorting when no-walk is in effect
counteracts the final reversal.

When I noticed that the order of inputs to cases 2) and 3) above
was ignored, and thinking that 'git rev-list A..C | git
cherry-pick --stdin' should mimic 'git cherry-pick A..C', I
incorrectly thought that the error was the use of --reverse to
'git rev-list' as well as the sorting done in the no-walk case. I
think completely ignored case 2) at this point.

I now think I understand that the sorting done in the no-walk
case is indeed incorrect, but that the --reverse passed to
rev-list is correct. Instead, the final reversal, which is
currently unconditional, should not be done in the no-walk case.

IIUC, this could be implemented by making cherry-pick iterate
over rev_info.pending.objects just like 'git show' does when not
walking.

Junio, I think it makes sense to just drop this whole series for
now. I'll probably include patch 1/4 in my stalled rebase-range
series instead. If I understood you correctly, you didn't have
any objections to that patch.

Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order

[Junio C Hamano]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> So all of the above case give the right result in the end as long
> as the timestamps are chronological, and case 1) gives the right
> result regardless. The other two cases only works in most cases
> because the unexpcted sorting when no-walk is in effect
> counteracts the final reversal.

In short, if you have three commits in a row, A--B--C, with
timestamps that are not skewed, and want to replay changes of B and
then C in that order, all three you listed ends up doing the right
thing.  But if you want to apply the change C and then B:

    - "git cherry-pick A..C" is obviously not a way to do so, so we
      won't discuss it further.

    - "git cherry-pick C B" is the most natural way the user would
      want to express this request, but because of the sorting
      (i.e. commit_list_sort_by_date() in prepare_revision_walk(),
      combined with ->reverse in sequencer.c::prepare_revs()), it
      applies B and then C.  That is the real bug.

      Feeding the revs to "git cherry-pick --stdin" in the order the
      user wishes them to be applied has the same issue.

> IIUC, this could be implemented by making cherry-pick iterate
> over rev_info.pending.objects just like 'git show' does when not
> walking.

Yes, that was exactly why I said sequencer.c::prepare_revs() is
wrong to call prepare_revision_walk() unconditionally, even when
there is no revision walking involved.

I actually think your approach to place the "do not sort when we are
not walking" logic in prepare_revision_walk() makes more sense.
"show" has to look at pending.objects[] because it needs to show
objects other than commits (e.g. "git show :foo"), so there won't be
any change in its implementation with your change.  It will have to
look at pending.objects[] itself.

But "cherry-pick" and sequencer-derived commands only deal with
commits.  It would be far less error prone to let them call
get_revision() repeatedly like all other revision enumerating
commands do, than to have them go over the pending.objects[] list,
dereferencing tags and using only commits.  The resulting callers
would be more readable, too, I would think.

Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order

[Martin von Zweigbergk]

On Wed, Aug 15, 2012 at 10:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>
>> So all of the above case give the right result in the end as long
>> as the timestamps are chronological, and case 1) gives the right
>> result regardless. The other two cases only works in most cases
>> because the unexpcted sorting when no-walk is in effect
>> counteracts the final reversal.
>
> In short, if you have three commits in a row, A--B--C, with
> timestamps that are not skewed, and want to replay changes of B and
> then C in that order, all three you listed ends up doing the right
> thing.  But if you want to apply the change C and then B:
>
>     - "git cherry-pick A..C" is obviously not a way to do so, so we
>       won't discuss it further.
>
>     - "git cherry-pick C B" is the most natural way the user would
>       want to express this request, but because of the sorting
>       (i.e. commit_list_sort_by_date() in prepare_revision_walk(),
>       combined with ->reverse in sequencer.c::prepare_revs()), it
>       applies B and then C.  That is the real bug.
>
>       Feeding the revs to "git cherry-pick --stdin" in the order the
>       user wishes them to be applied has the same issue.

Exactly.

> I actually think your approach to place the "do not sort when we are
> not walking" logic in prepare_revision_walk() makes more sense.
> "show" has to look at pending.objects[] because it needs to show
> objects other than commits (e.g. "git show :foo"), so there won't be
> any change in its implementation with your change.  It will have to
> look at pending.objects[] itself.

Yes, I noticed that's why "show" has to do it that way.

> But "cherry-pick" and sequencer-derived commands only deal with
> commits.  It would be far less error prone to let them call
> get_revision() repeatedly like all other revision enumerating
> commands do, than to have them go over the pending.objects[] list,
> dereferencing tags and using only commits.  The resulting callers
> would be more readable, too, I would think.

Makes sense, I'll try to implement it that way. I was afraid that
we would need to call prepare_revision_walk() once first and then
if we afterwards find out that we should not walk, we would need
to call it again without the reverse option. But after looking at
how rev_info.reverse is used, it seem like it's only used in
get_revision(), so we can leave it either on or off during the
prepare_revision_walk() and the and set appropriately before
calling get_revision(), like so:

  init_revisions(&revs);
  revs.no_walk = REVISION_WALK_NO_WALK_UNSORTED;
  setup_revisions(...);
  prepare_revision_walk(&revs);
  revs.reverse = !revs.no_walk;
  // iterate over revisions

Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order

[Junio C Hamano]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> Makes sense, I'll try to implement it that way. I was afraid that
> we would need to call prepare_revision_walk() once first and then
> if we afterwards find out that we should not walk, we would need
> to call it again without the reverse option.

> But after looking at
> how rev_info.reverse is used, it seem like it's only used in
> get_revision(), so we can leave it either on or off during the
> prepare_revision_walk() and the and set appropriately before
> calling get_revision(), like so:
>
>   init_revisions(&revs);
>   revs.no_walk = REVISION_WALK_NO_WALK_UNSORTED;
>   setup_revisions(...);
>   prepare_revision_walk(&revs);
>   revs.reverse = !revs.no_walk;

Sorry, but I do not understand why you frutz with "reverse" after
prepare, and not before.

I think you can just set no_walk and let setup_revisions() turn it
off upon seeing a range (this happens in add_pending_object()).
After setup_revisions() returns, if no_walk is still set, you only
got individual refs without ranges, so no reversing required.

You also need to be careful about "revert" that shares the code;
when reverting range A..C in your example, you want to undo C and
then B, and you do not want to reverse them.

Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order

[Martin von Zweigbergk]

On Wed, Aug 15, 2012 at 11:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>
>> Makes sense, I'll try to implement it that way. I was afraid that
>> we would need to call prepare_revision_walk() once first and then
>> if we afterwards find out that we should not walk, we would need
>> to call it again without the reverse option.
>
>> But after looking at
>> how rev_info.reverse is used, it seem like it's only used in
>> get_revision(), so we can leave it either on or off during the
>> prepare_revision_walk() and the and set appropriately before
>> calling get_revision(), like so:
>>
>>   init_revisions(&revs);
>>   revs.no_walk = REVISION_WALK_NO_WALK_UNSORTED;
>>   setup_revisions(...);
>>   prepare_revision_walk(&revs);
>>   revs.reverse = !revs.no_walk;
>
> Sorry, but I do not understand why you frutz with "reverse" after
> prepare, and not before.
>
> I think you can just set no_walk and let setup_revisions() turn it
> off upon seeing a range (this happens in add_pending_object()).

Ah, of course. For some reason I thought that was called from
prepare_revision_walk()

> After setup_revisions() returns, if no_walk is still set, you only
> got individual refs without ranges, so no reversing required.

Yes, it's in the other case (e.g. 'git cherry-pick A..C', when
no_walk is not set), that we need to set reverse before walking.

> You also need to be careful about "revert" that shares the code;
> when reverting range A..C in your example, you want to undo C and
> then B, and you do not want to reverse them.

Yep. It looks like this, so should be safe. But thanks for the reminder.

  if (opts->action != REPLAY_REVERT)
        opts->revs->reverse ^= 1;

[PATCH v2 0/3] revision (no-)walking in order

[Martin von Zweigbergk]

I'm still working on a re-roll of my rebase-range series, but I think
these three are quite unrelated and shouldn't be held up by that other
series.

Junio, thanks for all the help with explaining revision walking. It
was a little blurry for a long time, but at least I feel more
comfortable with these few patches now.

Btw, the rebase-range series seems to need (or be greatly simplified),
although I'm not 100% sure yet, by teaching patch-id --keep-empty,
which would be its first command line option. Let me know if you
(plural) sees a problem with that.

Btw2, I'm migrating my email to martinvonz@gmail.com (not y@google.com
;-) which saves a few keystrokes and matches some of my other
accounts, so these patches will be the first ones from the new
address.

Martin von Zweigbergk (3):
  teach log --no-walk=unsorted, which avoids sorting
  demonstrate broken 'git cherry-pick three one two'
  cherry-pick/revert: respect order of revisions to pick

 Documentation/rev-list-options.txt  | 12 ++++++++----
 builtin/log.c                       |  2 +-
 builtin/revert.c                    |  2 +-
 revision.c                          | 18 +++++++++++++++---
 revision.h                          |  6 +++++-
 sequencer.c                         |  4 +++-
 t/t3508-cherry-pick-many-commits.sh | 15 +++++++++++++++
 t/t4202-log.sh                      | 10 ++++++++++
 8 files changed, 58 insertions(+), 11 deletions(-)

-- 
1.7.11.1.104.ge7b44f1

[PATCH v2 1/3] teach log --no-walk=unsorted, which avoids sorting

[Martin von Zweigbergk]

When 'git log' is passed the --no-walk option, no revision walk takes
place, naturally. Perhaps somewhat surprisingly, however, the provided
revisions still get sorted by commit date. So e.g 'git log --no-walk
HEAD HEAD~1' and 'git log --no-walk HEAD~1 HEAD' give the same result
(unless the two revisions share the commit date, in which case they
will retain the order given on the command line). As the commit that
introduced --no-walk (8e64006 (Teach revision machinery about
--no-walk, 2007-07-24)) points out, the sorting is intentional, to
allow things like

 git log --abbrev-commit --pretty=oneline --decorate --all --no-walk

to show all refs in order by commit date.

But there are also other cases where the sorting is not wanted, such
as

 <command producing revisions in order> |
       git log --oneline --no-walk --stdin

To accomodate both cases, leave the decision of whether or not to sort
up to the caller, by allowing --no-walk={sorted,unsorted}, defaulting
to 'sorted' for backward-compatibility reasons.

Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
---
 Documentation/rev-list-options.txt | 12 ++++++++----
 builtin/log.c                      |  2 +-
 builtin/revert.c                   |  2 +-
 revision.c                         | 18 +++++++++++++++---
 revision.h                         |  6 +++++-
 t/t4202-log.sh                     | 10 ++++++++++
 6 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index def1340..5436eba 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -636,10 +636,14 @@ These options are mostly targeted for packing of git repositories.
 	Only useful with '--objects'; print the object IDs that are not
 	in packs.
 
---no-walk::
-
-	Only show the given revs, but do not traverse their ancestors.
-	This has no effect if a range is specified.
+--no-walk[=(sorted|unsorted)]::
+
+	Only show the given commits, but do not traverse their ancestors.
+	This has no effect if a range is specified. If the argument
+	"unsorted" is given, the commits are show in the order they were
+	given on the command line. Otherwise (if "sorted" or no argument
+	was given), the commits are show in reverse chronological order
+	by commit time.
 
 --do-walk::
 
diff --git a/builtin/log.c b/builtin/log.c
index ecc2793..20838b1 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -456,7 +456,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
 	rev.always_show_header = 1;
-	rev.no_walk = 1;
+	rev.no_walk = REVISION_WALK_NO_WALK_SORTED;
 	rev.diffopt.stat_width = -1; 	/* Scale to real terminal size */
 
 	memset(&opt, 0, sizeof(opt));
diff --git a/builtin/revert.c b/builtin/revert.c
index 82d1bf8..42ce399 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -193,7 +193,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		struct setup_revision_opt s_r_opt;
 		opts->revs = xmalloc(sizeof(*opts->revs));
 		init_revisions(opts->revs, NULL);
-		opts->revs->no_walk = 1;
+		opts->revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
 		if (argc < 2)
 			usage_with_options(usage_str, options);
 		memset(&s_r_opt, 0, sizeof(s_r_opt));
diff --git a/revision.c b/revision.c
index 442a945..66ba2e6 100644
--- a/revision.c
+++ b/revision.c
@@ -1300,7 +1300,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	    !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk") ||
 	    !strcmp(arg, "--bisect") || !prefixcmp(arg, "--glob=") ||
 	    !prefixcmp(arg, "--branches=") || !prefixcmp(arg, "--tags=") ||
-	    !prefixcmp(arg, "--remotes="))
+	    !prefixcmp(arg, "--remotes=") || !prefixcmp(arg, "--no-walk="))
 	{
 		unkv[(*unkc)++] = arg;
 		return 1;
@@ -1695,7 +1695,18 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	} else if (!strcmp(arg, "--not")) {
 		*flags ^= UNINTERESTING;
 	} else if (!strcmp(arg, "--no-walk")) {
-		revs->no_walk = 1;
+		revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
+	} else if (!prefixcmp(arg, "--no-walk=")) {
+		/*
+		 * Detached form ("--no-walk X" as opposed to "--no-walk=X")
+		 * not allowed, since the argument is optional.
+		 */
+		if (!strcmp(arg + 10, "sorted"))
+			revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
+		else if (!strcmp(arg + 10, "unsorted"))
+			revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;
+		else
+			return error("invalid argument to --no-walk");
 	} else if (!strcmp(arg, "--do-walk")) {
 		revs->no_walk = 0;
 	} else {
@@ -2117,10 +2128,11 @@ int prepare_revision_walk(struct rev_info *revs)
 		}
 		e++;
 	}
-	commit_list_sort_by_date(&revs->commits);
 	if (!revs->leak_pending)
 		free(list);
 
+	if (revs->no_walk != REVISION_WALK_NO_WALK_UNSORTED)
+		commit_list_sort_by_date(&revs->commits);
 	if (revs->no_walk)
 		return 0;
 	if (revs->limited)
diff --git a/revision.h b/revision.h
index cb5ab35..a95bd0b 100644
--- a/revision.h
+++ b/revision.h
@@ -41,6 +41,10 @@ struct rev_cmdline_info {
 	} *rev;
 };
 
+#define REVISION_WALK_WALK 0
+#define REVISION_WALK_NO_WALK_SORTED 1
+#define REVISION_WALK_NO_WALK_UNSORTED 2
+
 struct rev_info {
 	/* Starting list */
 	struct commit_list *commits;
@@ -62,7 +66,7 @@ struct rev_info {
 	/* Traversal flags */
 	unsigned int	dense:1,
 			prune:1,
-			no_walk:1,
+			no_walk:2,
 			show_all:1,
 			remove_empty_trees:1,
 			simplify_history:1,
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 71be59d..bd83355 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -178,11 +178,21 @@ test_expect_success 'git log --no-walk <commits> sorts by commit time' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git log --no-walk=sorted <commits> sorts by commit time' '
+	git log --no-walk=sorted --oneline 5d31159 804a787 394ef78 > actual &&
+	test_cmp expect actual
+'
+
 cat > expect << EOF
 5d31159 fourth
 804a787 sixth
 394ef78 fifth
 EOF
+test_expect_success 'git log --no-walk=unsorted <commits> leaves list of commits as given' '
+	git log --no-walk=unsorted --oneline 5d31159 804a787 394ef78 > actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'git show <commits> leaves list of commits as given' '
 	git show --oneline -s 5d31159 804a787 394ef78 > actual &&
 	test_cmp expect actual
-- 
1.7.11.1.104.ge7b44f1

[PATCH v2 2/3] demonstrate broken 'git cherry-pick three one two'

[Martin von Zweigbergk]

Cherry-picking commits out of order (w.r.t. commit time stamp) doesn't
currently work. Add a test case to demonstrate it.

Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
---
 t/t3508-cherry-pick-many-commits.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh
index 75f7ff4..fff20c3 100755
--- a/t/t3508-cherry-pick-many-commits.sh
+++ b/t/t3508-cherry-pick-many-commits.sh
@@ -44,6 +44,21 @@ test_expect_success 'cherry-pick first..fourth works' '
 	check_head_differs_from fourth
 '
 
+test_expect_failure 'cherry-pick three one two works' '
+	git checkout -f first &&
+	test_commit one &&
+	test_commit two &&
+	test_commit three &&
+	git checkout -f master &&
+	git reset --hard first &&
+	git cherry-pick three one two &&
+	git diff --quiet three &&
+	git diff --quiet HEAD three &&
+	test "$(git log --reverse --format=%s first..)" == "three
+one
+two"
+'
+
 test_expect_success 'output to keep user entertained during multi-pick' '
 	cat <<-\EOF >expected &&
 	[master OBJID] second
-- 
1.7.11.1.104.ge7b44f1

[PATCH v2 3/3] cherry-pick/revert: respect order of revisions to pick

[Martin von Zweigbergk]

When giving multiple individual revisions to cherry-pick or revert, as
in 'git cherry-pick A B' or 'git revert B A', one would expect them to
be picked/reverted in the order given on the command line. They are
instead ordered by their commit timestamp -- in chronological order
for "cherry-pick" and in reverse chronological order for
"revert". This matches the order in which one would usually give them
on the command line, making this bug somewhat hard to notice. Still,
it has been reported at least once before [1].

It seems like the chronological sorting happened by accident because
the revision walker has traditionally always sorted commits in reverse
chronological order when rev_info.no_walk was enabled. In the case of
'git revert B A' where B is newer than A, this sorting is a no-op. For
'git cherry-pick A B', the sorting would reverse the arguments, but
because the sequencer also flips the rev_info.reverse flag when
picking (as opposed to reverting), the end result is a chronological
order. The rev_info.reverse flag was probably flipped so that the
revision walker emits B before C in 'git cherry-pick A..C'; that it
happened to effectively undo the unexpected sorting done when not
walking, was probably a coincidence that allowed this bug to happen at
all.

Fix the bug by telling the revision walker not to sort the commits
when not walking. The only case we want to reverse the order is now
when cherry-picking and walking revisions (rev_info.no_walk = 0).

 [1] http://thread.gmane.org/gmane.comp.version-control.git/164794

Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
---
 builtin/revert.c                    | 2 +-
 sequencer.c                         | 4 +++-
 t/t3508-cherry-pick-many-commits.sh | 2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 42ce399..98ad641 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -193,7 +193,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		struct setup_revision_opt s_r_opt;
 		opts->revs = xmalloc(sizeof(*opts->revs));
 		init_revisions(opts->revs, NULL);
-		opts->revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
+		opts->revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;
 		if (argc < 2)
 			usage_with_options(usage_str, options);
 		memset(&s_r_opt, 0, sizeof(s_r_opt));
diff --git a/sequencer.c b/sequencer.c
index bf078f2..9f32104 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -543,7 +543,9 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 
 static void prepare_revs(struct replay_opts *opts)
 {
-	if (opts->action != REPLAY_REVERT)
+	// picking (but not reverting) ranges (but not individual revisions)
+	// should be done in reverse
+	if (opts->action == REPLAY_PICK && !opts->revs->no_walk)
 		opts->revs->reverse ^= 1;
 
 	if (prepare_revision_walk(opts->revs))
diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh
index fff20c3..04b5ad4 100755
--- a/t/t3508-cherry-pick-many-commits.sh
+++ b/t/t3508-cherry-pick-many-commits.sh
@@ -44,7 +44,7 @@ test_expect_success 'cherry-pick first..fourth works' '
 	check_head_differs_from fourth
 '
 
-test_expect_failure 'cherry-pick three one two works' '
+test_expect_success 'cherry-pick three one two works' '
 	git checkout -f first &&
 	test_commit one &&
 	test_commit two &&
-- 
1.7.11.1.104.ge7b44f1

Re: [PATCH v2 0/3] revision (no-)walking in order

[Junio C Hamano]

Martin von Zweigbergk <martinvonz@gmail.com> writes:

> Btw2, I'm migrating my email to martinvonz@gmail.com (not y@google.com
> ;-) which saves a few keystrokes and matches some of my other
> accounts, so these patches will be the first ones from the new
> address.

Please send in something like this, then.

 .mailmap | 1 +
 1 file changed, 1 insertion(+)

diff --git i/.mailmap w/.mailmap
index 6303782..2650f9e 100644
--- i/.mailmap
+++ w/.mailmap
@@ -43,6 +43,7 @@ Lars Doelle <lars.doelle@on-line.de>
 Li Hong <leehong@pku.edu.cn>
 Lukas Sandström <lukass@etek.chalmers.se>
 Martin Langhoff <martin@laptop.org>
+Martin von Zweigbergk <martinvonz@gmail.com> <martin.von.zweigbergk@gmail.com>
 Michael Coleman <tutufan@gmail.com>
 Michael J Gruber <git@drmicha.warpmail.net> <michaeljgruber+gmane@fastmail.fm>
 Michael W. Olson <mwolson@gnu.org>

[PATCH] Martin von Zweigbergk has a new e-mail address

[Martin von Zweigbergk]

Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
---
 .mailmap | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.mailmap b/.mailmap
index 6303782..2650f9e 100644
--- a/.mailmap
+++ b/.mailmap
@@ -43,6 +43,7 @@ Lars Doelle <lars.doelle@on-line.de>
 Li Hong <leehong@pku.edu.cn>
 Lukas Sandström <lukass@etek.chalmers.se>
 Martin Langhoff <martin@laptop.org>
+Martin von Zweigbergk <martinvonz@gmail.com> <martin.von.zweigbergk@gmail.com>
 Michael Coleman <tutufan@gmail.com>
 Michael J Gruber <git@drmicha.warpmail.net> <michaeljgruber+gmane@fastmail.fm>
 Michael W. Olson <mwolson@gnu.org>
-- 
1.7.11.1.104.ge7b44f1

Re: [PATCH v2 1/3] teach log --no-walk=unsorted, which avoids sorting

[Dan Johnson]

On Wed, Aug 29, 2012 at 2:15 AM, Martin von Zweigbergk
<martinvonz@gmail.com> wrote:
> When 'git log' is passed the --no-walk option, no revision walk takes
> place, naturally. Perhaps somewhat surprisingly, however, the provided
> revisions still get sorted by commit date. So e.g 'git log --no-walk
> HEAD HEAD~1' and 'git log --no-walk HEAD~1 HEAD' give the same result
> (unless the two revisions share the commit date, in which case they
> will retain the order given on the command line). As the commit that
> introduced --no-walk (8e64006 (Teach revision machinery about
> --no-walk, 2007-07-24)) points out, the sorting is intentional, to
> allow things like
>
>  git log --abbrev-commit --pretty=oneline --decorate --all --no-walk
>
> to show all refs in order by commit date.
>
> But there are also other cases where the sorting is not wanted, such
> as
>
>  <command producing revisions in order> |
>        git log --oneline --no-walk --stdin
>
> To accomodate both cases, leave the decision of whether or not to sort
> up to the caller, by allowing --no-walk={sorted,unsorted}, defaulting
> to 'sorted' for backward-compatibility reasons.
>
> Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
> ---

Perhaps I am missing something from an earlier discussion, but it is
not obvious to me why this is an option to the no-walk behavior and
not something like --sorted/--unsorted as a separate option.

In other words, I don't understand why you always want to sort if you
are doing revision walking.

Thanks for any explanation,
-Dan

Re: [PATCH v2 1/3] teach log --no-walk=unsorted, which avoids sorting

[Junio C Hamano]

Dan Johnson <computerdruid@gmail.com> writes:

> Perhaps I am missing something from an earlier discussion, but it is

  http://thread.gmane.org/gmane.comp.version-control.git/203259/focus=203344

> not obvious to me why this is an option to the no-walk behavior and
> not something like --sorted/--unsorted as a separate option.
>
> In other words, I don't understand why you always want to sort if you
> are doing revision walking.

When you have more than one starting points to dig the history from
(e.g. "git log foo bar baz"), you would want to start digging from
the newer ones, as that would help you find the fork points of the
branches involved more efficiently.  But you need to follow the
previous discussion if you want to understand implications around
sorting.

Re: [PATCH v2 2/3] demonstrate broken 'git cherry-pick three one two'

[Junio C Hamano]

Martin von Zweigbergk <martinvonz@gmail.com> writes:

> Cherry-picking commits out of order (w.r.t. commit time stamp) doesn't
> currently work. Add a test case to demonstrate it.
>
> Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
> ---
>  t/t3508-cherry-pick-many-commits.sh | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh
> index 75f7ff4..fff20c3 100755
> --- a/t/t3508-cherry-pick-many-commits.sh
> +++ b/t/t3508-cherry-pick-many-commits.sh
> @@ -44,6 +44,21 @@ test_expect_success 'cherry-pick first..fourth works' '
>  	check_head_differs_from fourth
>  '
>  
> +test_expect_failure 'cherry-pick three one two works' '
> +	git checkout -f first &&
> +	test_commit one &&
> +	test_commit two &&
> +	test_commit three &&
> +	git checkout -f master &&
> +	git reset --hard first &&
> +	git cherry-pick three one two &&
> +	git diff --quiet three &&
> +	git diff --quiet HEAD three &&
> +	test "$(git log --reverse --format=%s first..)" == "three
> +one
> +two"
> +'

"test $A == $B" is not POSIX.  I'll drop '=' when queuing, so no
need to resend.

Thanks.

> +
>  test_expect_success 'output to keep user entertained during multi-pick' '
>  	cat <<-\EOF >expected &&
>  	[master OBJID] second