Re: [PATCH] cherry-pick: add failing test for out-of-order pick

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

>   $ git cherry-pick master..topic
>   $ git cherry-pick topic ^master
>   $ git cherry-pick ^master topic
>
> So the order of the arguments specified on the command-line is
> irrelevant in these cases.  However, there are cases where it is worth
> paying attention to the order.  For instance:
>

This segue feels a bit unnatural.  I think the relevant point was that
early output from revision traversal (and perhaps some other things
--- I haven't checked) relies on commits having been inserted in a
topologically sorted order.

Anyway, I don't think the background is necessary --- the
one-paragraph description below stands well enough alone.

>   $ git cherry-pick commit3 commit1 commit2
> 
> picks commits after sorting by date order, which is counter-intuitive.
> Add a failing test to t3508 (cherry-pick-many-commits) documenting
> this behavior.
> 
> Reported-by: SZEDER Gábor <szeder@ira.uka.de>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
[...]
> --- a/t/t3508-cherry-pick-many-commits.sh
> +++ b/t/t3508-cherry-pick-many-commits.sh
> @@ -59,6 +59,31 @@ test_expect_success 'cherry-pick first..fourth works' '
>  	check_head_differs_from fourth
>  '
>  
> +test_expect_failure 'cherry-pick picks commits in the right order' '

I would say "in the order requested" instead of the right order, since
it is not completely obvious to me what the right order is.

> +	cat <<-\EOF >expected &&
> +	[master OBJID] fourth
> +	 Author: A U Thor <author@example.com>
> +	 1 files changed, 1 insertions(+), 0 deletions(-)
> +	[master OBJID] second
> +	 Author: A U Thor <author@example.com>
> +	 1 files changed, 1 insertions(+), 0 deletions(-)
> +	[master OBJID] third
> +	 Author: A U Thor <author@example.com>
> +	 1 files changed, 1 insertions(+), 0 deletions(-)
> +	EOF

Why check all these details of formatting, instead of e.g. using "git
rev-list | git diff-tree -s --format=%s"?

[...]
> +	test_cmp expected actual.fuzzy &&
> +	check_head_differs_from second

Why make the same check twice?

Hope that helps,
Jonathan