cherry-pick/revert: respect order of revisions to pick

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/revert.c b/builtin/revert.c
index 42ce399d88f7f53cb52255f74e15f6557cdb5f70..98ad6410abb603cb8285f88df58ed0937ea64609 100644 (file)
--- 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 bf078f274bfe23997fcd1bc8c2a1c958f39ac9fe..bd626806d64b9cd9a814f062d092501315546bdf 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -543,7 +543,11 @@ 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 afda2d5825f6c087e22a7830d2b1aebb50923c3a..340afc760de030879fff496dca3db8432a6b992b 100755 (executable)
--- 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 &&