Re: [PATCH v2 2/2] blame: allow blame --reverse --first-parent when it makes sense

[Junio C Hamano <gitster@pobox.com>]

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?

> 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