From ca92e59e30b503ff8861b6cd4d431d38738a7ee8 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Tue, 28 Aug 2012 23:15:54 -0700 Subject: teach log --no-walk=unsorted, which avoids sorting 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 | 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 Signed-off-by: Junio C Hamano --- Documentation/rev-list-options.txt | 11 ++++++++--- builtin/log.c | 2 +- builtin/revert.c | 2 +- revision.c | 18 +++++++++++++++--- revision.h | 6 +++++- t/t4202-log.sh | 10 ++++++++++ 6 files changed, 40 insertions(+), 9 deletions(-) diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index 1ae3c899ef..9780f4646a 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -619,9 +619,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. +--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 906dca475a..9e21d1b490 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -454,7 +454,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 82d1bf844b..42ce399d88 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 935e7a7ba4..2f9dbddf60 100644 --- a/revision.c +++ b/revision.c @@ -1294,7 +1294,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; @@ -1687,7 +1687,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 { @@ -2091,10 +2102,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 863f4f6454..80e22fea4a 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 71be59d446..bd8335580d 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -178,11 +178,21 @@ test_expect_success 'git log --no-walk sorts by commit time' ' test_cmp expect actual ' +test_expect_success 'git log --no-walk=sorted 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 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 leaves list of commits as given' ' git show --oneline -s 5d31159 804a787 394ef78 > actual && test_cmp expect actual -- cgit v1.2.1 From d023c248a39a7ac3ee226e34eadcee6913913f2f Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Tue, 28 Aug 2012 23:15:55 -0700 Subject: demonstrate broken 'git cherry-pick three one two' 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 Signed-off-by: Junio C Hamano --- 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 75f7ff4f2f..afda2d5825 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 -- cgit v1.2.1 From a73e22e96350c9bcb5c0e31e0298ce67bec0a527 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Tue, 28 Aug 2012 23:15:56 -0700 Subject: 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 Signed-off-by: Junio C Hamano --- builtin/revert.c | 2 +- sequencer.c | 6 +++++- t/t3508-cherry-pick-many-commits.sh | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 42ce399d88..98ad6410ab 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 bf078f274b..bd626806d6 100644 --- 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 afda2d5825..340afc760d 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 && -- cgit v1.2.1