v2, 2 partsAdd test to describe expectation of blame --reverse with branched history

> If history contains merges from feature branches, `blame --reverse`
> reports not the commit when the line was actually edited, but head of
> the last merged branch which was created before the edit.
>
> As a workaround, `blame --reverse --first-parent` could be used to find
> the merge of branch containing the edit, but it was disabled in
> 95a4fb0eac, because incorrectly specified range could produce in
> unexpected and meaningless result.
>
> Add tests which describe ideal functionality with and without
> `--first-parent`.
>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
>  t/t8009-blame-reverse.sh | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100755 t/t8009-blame-reverse.sh
>
> diff --git a/t/t8009-blame-reverse.sh b/t/t8009-blame-reverse.sh
> new file mode 100755
> index 0000000..9f40613
> --- /dev/null
> +++ b/t/t8009-blame-reverse.sh
> @@ -0,0 +1,34 @@
> +#!/bin/sh
> +
> +test_description='git blame reverse'
> +. ./test-lib.sh
> +

> +test_expect_success setup '
> +	test_commit A0 file.t line0 &&
> +	test_commit A1 &&
> +	git reset --hard A0 &&
> +	test_commit B1 &&
> +	test_commit B2 file.t line0changed &&
> +	git reset --hard A1 &&
> +	test_merge A2 B2 &&
> +	git reset --hard A1 &&
> +	test_commit C1 &&
> +	git reset --hard A2 &&
> +	test_merge A3 C1
> +	'

> +	git rev-parse B1 >expect &&
> +	test_cmp expect actual
> +	'
> +
> +test_expect_failure 'blame --reverse --first-parent finds A1' '
> +	git blame --porcelain --reverse --first-parent A0..A3 -- file.t >actual_full &&
> +	head -1 <actual_full | sed -e "sX .*XX" >actual &&
> +	git rev-parse A1 >expect &&
> +	test_cmp expect actual
> +	'
> +
> +test_done

> Let me try.
> 
>  (1) for merges, an edge with '*' denotes the one to the first
>      parent.
>  (2) a commit that touches file.t are in capital
> 
>            c1---a3
>           /    *
>          /    /
>    A0---a1--*a2
>      \      /
>       b1---B2
> 
> Did I draw it correctly?

>> +test_expect_failure 'blame --reverse finds B1, not C1' '
>> +	git blame --porcelain --reverse A0..A3 -- file.t >actual_full &&
>> +	head -1 <actual_full | sed -e "sX .*XX" >actual &&
> 
> "head -n 1" is more POSIXy way to spell this.
> 
> Don't get cute with sXXX in sed script when the usual s/// suffices;
> the only effect of the cuteness is to waste readers' time, who have
> to wonder if there is something interesting going on.

> Do not die immediately when the two flags are specified. Instead
> check that the specified range is along first-parent chain. Exploit
> how prepare_revision_walk() handles first_parent_only flag: the commits
> outside of first-parent chain are either unknown (and do not have any
> children recorded) or appear as non-first parent of a commit along the
> first-parent chain.
>
> Since the check seems fragile, add test which verifies that blame dies
> in both cases.

> Signed-off-by: Max Kirillov <max@max630.net>
> ---
>  builtin/blame.c          | 11 +++++++++--
>  t/t8009-blame-reverse.sh |  7 ++++++-
>  2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 295ce92..27de544 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2692,8 +2692,6 @@ parse_done:
>  	}
>  	else if (contents_from)
>  		die("--contents and --children do not blend well.");
> -	else if (revs.first_parent_only)
> -		die("combining --first-parent and --reverse is not supported");
>  	else {
>  		final_commit_name = prepare_initial(&sb);
>  		sb.commits.compare = compare_commits_by_reverse_commit_date;
> @@ -2721,6 +2719,15 @@ parse_done:
>  	if (prepare_revision_walk(&revs))
>  		die(_("revision walk setup failed"));
>  
> +	if (reverse && revs.first_parent_only) {
> +		struct commit_list *final_children = lookup_decoration(&revs.children,
> +								       &sb.final->object);
> +		if (!final_children ||
> +		    hashcmp(final_children->item->parents->item->object.sha1,
> +			    sb.final->object.sha1))
> +		    die("--reverse --first-parent together require range along first-parent chain");
> +	}
> +
>  	if (is_null_sha1(sb.final->object.sha1)) {
>  		o = sb.final->util;
>  		sb.final_buf = xmemdupz(o->file.ptr, o->file.size);
> diff --git a/t/t8009-blame-reverse.sh b/t/t8009-blame-reverse.sh
> index 9f40613..042863b 100755
> --- a/t/t8009-blame-reverse.sh
> +++ b/t/t8009-blame-reverse.sh
> @@ -24,11 +24,16 @@ test_expect_failure 'blame --reverse finds B1, not C1' '
>  	test_cmp expect actual
>  	'
>  
> -test_expect_failure 'blame --reverse --first-parent finds A1' '
> +test_expect_success 'blame --reverse --first-parent finds A1' '
>  	git blame --porcelain --reverse --first-parent A0..A3 -- file.t >actual_full &&
>  	head -1 <actual_full | sed -e "sX .*XX" >actual &&
>  	git rev-parse A1 >expect &&
>  	test_cmp expect actual
>  	'
>  
> +test_expect_success 'blame --reverse --first-parse dies if no first parent chain' '
> +	test_must_fail git blame --porcelain --reverse --first-parent B1..A3 -- file.t &&
> +	test_must_fail git blame --porcelain --reverse --first-parent B2..A3 -- file.t
> +	'
> +
>  test_done

> Max Kirillov <max@max630.net> writes:
> 
>> Do not die immediately when the two flags are specified. Instead
>> check that the specified range is along first-parent chain. Exploit
>> how prepare_revision_walk() handles first_parent_only flag: the commits
>> outside of first-parent chain are either unknown (and do not have any
>> children recorded) or appear as non-first parent of a commit along the
>> first-parent chain.
>>
>> Since the check seems fragile, add test which verifies that blame dies
>> in both cases.
> 
> It is not quite clear in what way the "check seems fragile".
> 
> It is either "correct" or "appears to have worked by chance and
> nobody has any confidence that it would tell if 'it makes sense'
> reliably", and the latter cannot be papered over with any number of
> tests.
> 
> The logic you implemented feels solid to me, at least at a first
> glance.  What kind of gotchas are you worried about?

> On Wed, Oct 21, 2015 at 09:25:37PM -0700, Junio C Hamano wrote:
>
>> The logic you implemented feels solid to me, at least at a first
>> glance.  What kind of gotchas are you worried about?
>
> The fact than arbitrary commit's children are unknown does
> not seem reliable to me. It more fits the "works by chance"
> description.

> Max Kirillov <max@max630.net> writes:
>> On Wed, Oct 21, 2015 at 09:25:37PM -0700, Junio C Hamano wrote:
>>
>>> The logic you implemented feels solid to me, at least at a first
>>> glance.  What kind of gotchas are you worried about?
>>
>> The fact than arbitrary commit's children are unknown does
>> not seem reliable to me. It more fits the "works by chance"
>> description.
>
> That's sad.