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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 13, 2012, 20:23 UTC
Message-ID: <7vpq6uczis.fsf@alter.siamese.dyndns.org>
In-Reply-To: <50289e51.29d0320a.65ff.2c48SMTPIN_ADDED@gmr-mx.google.com>

y@google.com writes:

Show 7 quoted lines

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

Show 21 quoted lines

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