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

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

[Max Kirillov]

This enables --reverse --first-parent back.

Max Kirillov (2):
  Add test to describe expectation of blame --reverse with branched
    history
  blame: allow blame --reverse --first-parent when it makes sense

 builtin/blame.c          | 11 +++++++++--
 t/t8009-blame-reverse.sh | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)
 create mode 100755 t/t8009-blame-reverse.sh

-- 
2.3.4.2801.g3d0809b

[PATCH 1/2] Add test to describe expectation of blame --reverse with branched history

[Max Kirillov]

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
+	'
+
+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 &&
+	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
-- 
2.3.4.2801.g3d0809b

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

[Max Kirillov]

Do not die immediately when the two flags are specified. Instead
check that the specified range is along first-parent chain.  Explioit
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 makes sure 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
-- 
2.3.4.2801.g3d0809b

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

[Eric Sunshine]

On Wed, Oct 21, 2015 at 12:08 AM, Max Kirillov <max@max630.net> wrote:
> Do not die immediately when the two flags are specified. Instead
> check that the specified range is along first-parent chain.  Explioit

s/Explioit/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 makes sure that blame dies
> in both cases.
>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
> 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
> --
> 2.3.4.2801.g3d0809b

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

[Max Kirillov]

On Wed, Oct 21, 2015 at 12:29:12AM -0400, Eric Sunshine wrote:
> On Wed, Oct 21, 2015 at 12:08 AM, Max Kirillov <max@max630.net> wrote:
>> Do not die immediately when the two flags are specified. Instead
>> check that the specified range is along first-parent chain.  Explioit
> 
> s/Explioit/Exploit/

Fixed. Thanks.

-- 
Max

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

[Max Kirillov]

Fixed mistype in commit message.

Max Kirillov (2):
  Add test to describe expectation of blame --reverse with branched
    history
  blame: allow blame --reverse --first-parent when it makes sense

 builtin/blame.c          | 11 +++++++++--
 t/t8009-blame-reverse.sh | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)
 create mode 100755 t/t8009-blame-reverse.sh

-- 
2.3.4.2801.g3d0809b

[PATCH v2 1/2] Add test to describe expectation of blame --reverse with branched history

[Max Kirillov]

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
+	'
+
+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 &&
+	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
-- 
2.3.4.2801.g3d0809b

Re: [PATCH v2 1/2] Add test to describe expectation of blame --reverse with branched history

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

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

If you are going to dedicate the whole test script for this test,
could you draw the topology of the commit DAG here, so that readers
can more easily follow what is going on?

> +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
> +	'

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.

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

Re: [PATCH v2 1/2] Add test to describe expectation of blame --reverse with branched history

[Max Kirillov]

On Wed, Oct 21, 2015 at 09:19:45PM -0700, Junio C Hamano wrote:
> 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?

Yes. Though I would

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

Ok. Will fix it and add the graph.

Re: [PATCH v2 1/2] Add test to describe expectation of blame --reverse with branched history

[Max Kirillov]

On Thu, Oct 22, 2015 at 05:37:21PM +0300, Max Kirillov wrote:
> Yes. Though I would

... make the line of As straight.

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

[Max Kirillov]

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

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

[Junio C Hamano]

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

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

[Max Kirillov]

On Wed, Oct 21, 2015 at 09:25:37PM -0700, Junio C Hamano wrote:
> 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?

The fact than arbitrary commit's children are unknown does
not seem reliable to me. It more fits the "works by chance"
description.

Actually, I realized that this can produce false positive if
the final commit is no-ff merged into first-parent chain,
because it woul be both first and non-first parent of the
first-parent chain.
But afaik such merge is not possible to create by frontent git
commands, so I'm not sure I should consider such case.

-- 
Max

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

[Junio C Hamano]

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.

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

[Max Kirillov]

On Thu, Oct 22, 2015 at 12:11:23PM -0700, Junio C Hamano wrote:
> 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.

How about this then? I could collect only children
along the first-parent chain. This makes sure that
no extra child entry appear in revs->children. Since the revs
structure is exclusive for cmd_blame() it is guaranteed
that no other command will affect or be affected by this
behavior.

This additionally forbids having several end commits
(git blame --reverse --first-parent ^A B C ...), but
this does not seem to have much practical use.

-- 
Max

8< ------------------
diff --git a/builtin/blame.c b/builtin/blame.c
index eb348f0..f66d0ae 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2401,16 +2401,11 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	return commit;
 }
 
-static char *prepare_final(struct scoreboard *sb)
+static struct object_array_entry *find_single_final(struct rev_info *revs)
 {
 	int i;
-	const char *final_commit_name = NULL;
-	struct rev_info *revs = sb->revs;
+	struct object_array_entry *found = NULL;
 
-	/*
-	 * There must be one and only one positive commit in the
-	 * revs->pending array.
-	 */
 	for (i = 0; i < revs->pending.nr; i++) {
 		struct object *obj = revs->pending.objects[i].item;
 		if (obj->flags & UNINTERESTING)
@@ -2419,14 +2414,24 @@ static char *prepare_final(struct scoreboard *sb)
 			obj = deref_tag(obj, NULL, 0);
 		if (obj->type != OBJ_COMMIT)
 			die("Non commit %s?", revs->pending.objects[i].name);
-		if (sb->final)
+		if (found)
 			die("More than one commit to dig from %s and %s?",
 			    revs->pending.objects[i].name,
-			    final_commit_name);
-		sb->final = (struct commit *) obj;
-		final_commit_name = revs->pending.objects[i].name;
+			    found->name);
+		found = &(revs->pending.objects[i]);
+	}
+	return found;
+}
+
+static char *prepare_final(struct scoreboard *sb)
+{
+	struct object_array_entry *found = find_single_final(sb->revs);
+	if (found) {
+		sb->final = (struct commit *) found->item;
+		return xstrdup(found->name);
+	} else {
+		return NULL;
 	}
-	return xstrdup_or_null(final_commit_name);
 }
 
 static char *prepare_initial(struct scoreboard *sb)
@@ -2502,6 +2507,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	long dashdash_pos, lno;
 	char *final_commit_name = NULL;
 	enum object_type type;
+	struct commit *firstparent_chain_initial = NULL;
 
 	static struct string_list range_list;
 	static int output_option = 0, opt = 0;
@@ -2695,6 +2701,8 @@ parse_done:
 	else {
 		final_commit_name = prepare_initial(&sb);
 		sb.commits.compare = compare_commits_by_reverse_commit_date;
+		if (revs.first_parent_only)
+			revs.children.name = NULL;
 	}
 
 	if (!sb.final) {
@@ -2711,6 +2719,14 @@ parse_done:
 	else if (contents_from)
 		die("Cannot use --contents with final commit object name");
 
+	if (reverse && revs.first_parent_only) {
+		struct object_array_entry *entry = find_single_final(sb.revs);
+		if (!entry)
+			die("--reverse and --first-parent together require specified latest commit");
+		else
+			firstparent_chain_initial = (struct commit*) entry->item;
+	}
+
 	/*
 	 * If we have bottom, this will mark the ancestors of the
 	 * bottom commits we would reach while traversing as
@@ -2720,11 +2736,21 @@ parse_done:
 		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))
+		struct commit *c = firstparent_chain_initial;
+
+		sb.revs->children.name = "children";
+		while (c->parents &&
+		       hashcmp(c->object.sha1, sb.final->object.sha1)) {
+			struct commit_list *l = xcalloc(1, sizeof(*l));
+
+			l->item = c;
+			if (add_decoration(&sb.revs->children,
+					   &c->parents->item->object, l))
+				die("BUG: not unique item in first-parent chain");
+			c = c->parents->item;
+		}
+
+		if (hashcmp(c->object.sha1, sb.final->object.sha1))
 			die("--reverse --first-parent together require range along first-parent chain");
 	}
 

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

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

> This additionally forbids having several end commits
> (git blame --reverse --first-parent ^A B C ...), but
> this does not seem to have much practical use.

Even if there were practical uses for a request with multiple end
points, a solution that

 (1) guarantees that "blame --reverse --first-parent" is the only
     thing that is affected; and

 (2) errors out if "blame --reverse --first-parent" cannot give a
     sensible answer.

is a strict improvement over "we don't know how to produce a
sensible answer in all cases, so we just forbid".  Somebody who
needs multiple end points badly can build on top with the same
principle, just widening the definition of what is "a sensible
answer" in the criteria (2) above.  Until then, defining the
sensible answer to be "an answer that consists of commits on the
first parent chain (but in reverse)" is perfectly fine [*1*].

As you added your patch after "-- <EOL>", my MUA refuses to quote it
in this response, but that's OK, as this message is not about the
patch itself ;-)


[Footnote]

*1* I haven't read the patch text to see if that is what the updated
    code is computing, though.  That's for a weekday ;-).

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

[Max Kirillov]

Fix indentation.

Max Kirillov (2):
  Add test to describe expectation of blame --reverse with branched
    history
  blame: allow blame --reverse --first-parent when it makes sense

 builtin/blame.c          | 11 +++++++++--
 t/t8009-blame-reverse.sh | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)
 create mode 100755 t/t8009-blame-reverse.sh

-- 
2.3.4.2801.g3d0809b

[PATCH v3 1/2] Add test to describe expectation of blame --reverse with branched history

[Max Kirillov]

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
+	'
+
+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 &&
+	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
-- 
2.3.4.2801.g3d0809b

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

[Max Kirillov]

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..eb348f0 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
-- 
2.3.4.2801.g3d0809b

[PATCH v4 0/4] blame: allow blame --reverse --first-parent when it makes sense

[Max Kirillov]

Change aproach - instead of assuming children content build them explicitly,
also accurately verifying that the specified range is along the first-parent chain

Max Kirillov (3):
  Add test to describe expectation of blame --reverse with branched
    history
  blame: extract find_single_final
  blame: allow blame --reverse --first-parent when it makes sense

 builtin/blame.c          | 61 +++++++++++++++++++++++++++++++++++++-----------
 t/t8009-blame-reverse.sh | 34 +++++++++++++++++++++++++++
 2 files changed, 81 insertions(+), 14 deletions(-)
 create mode 100755 t/t8009-blame-reverse.sh

-- 
2.3.4.2801.g3d0809b

[PATCH v4 1/3] Add test to describe expectation of blame --reverse with branched history

[Max Kirillov]

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
+	'
+
+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 &&
+	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
-- 
2.3.4.2801.g3d0809b

Re: [PATCH v4 1/3] Add test to describe expectation of blame --reverse with branched history

[Max Kirillov]

Sorry, forgot to fix the tests. Will send another batch some later.

Re: [PATCH v4 1/3] Add test to describe expectation of blame --reverse with branched history

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

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

I _think_ I know why it would be useful to allow "--first-parent" to
the command; it is useful the same way why "git log --first-parent
$path" would be a good way to get an overview.

But I am puzzled by your complaints (I'd characterise the statement
as such, given your second paragraph calls the combination a
"workaround") in the first paragraph.  I honestly do not understand
where it comes from at all.

The reverse blame begins from an old state and shows the most recent
child in the history that each line survived to, and it does not
show what commit removed the line from the original state.  And that
does not have anything to do with the presence of any merges or
forks in the history.  The command will always report "not the
commit that edited the line."  There is nothing special about "If
the history contains merges".

If you have this history, for example:

    D---E---F
   /         \*
  O           X---Y
   \         /
    A---B---C

where O had the original file, which was not touched by any commits
on the branch on the upper side, and commit B rewrote all lines of
the file, running blame in reverse may show A as the last point
where all lines survived up to, if the "reversed" history happened
to consider A as the earlier "parent" (in reality it is a child but
blame is about assigning blame for each line from child to parents
so in the reversed history, real children becomes parents).  Or it
may show F as the last point where all lines survived up to, if D
was picked as the earlier "parent".  Because there is no inherent
ordering between A and D, both of which are children of O, your
result is not necessarily "head of the last merged branch".

But I do not see how "first-parent" would be a workaround for that.
The option would be useful to force the assignment of blame (in
reverse) along the first-parent chain O---D---E---F---X---Y so that
you can get a bird's-eye view of the history, i.e. squashing all
that happened in A---B---C as if that happened at X.

The explanation of the first paragraph needs to be rewritten to make
it understandable, but I am not sure what relevance it has with this
change.

Thanks.

Re: [PATCH v4 1/3] Add test to describe expectation of blame --reverse with branched history

[Max Kirillov]

On Sun, Oct 25, 2015 at 11:27:32PM -0700, Junio C Hamano wrote:
> Max Kirillov <max@max630.net> writes:
> 
>> 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>
>
> I _think_ I know why it would be useful to allow "--first-parent" to
> the command; it is useful the same way why "git log --first-parent
> $path" would be a good way to get an overview.
> 
> But I am puzzled by your complaints (I'd characterise the statement
> as such, given your second paragraph calls the combination a
> "workaround") in the first paragraph.  I honestly do not understand
> where it comes from at all.
> 
> The reverse blame begins from an old state and shows the most recent
> child in the history that each line survived to, and it does not
> show what commit removed the line from the original state.  And that
> does not have anything to do with the presence of any merges or
> forks in the history.  The command will always report "not the
> commit that edited the line."  There is nothing special about "If
> the history contains merges".
> 
> If you have this history, for example:
> 
>     D---E---F
>    /         \*
>   O           X---Y
>    \         /
>     A---B---C
> 
> where O had the original file, which was not touched by any commits
> on the branch on the upper side, and commit B rewrote all lines of
> the file, running blame in reverse may show A as the last point
> where all lines survived up to, if the "reversed" history happened
> to consider A as the earlier "parent" (in reality it is a child but
> blame is about assigning blame for each line from child to parents
> so in the reversed history, real children becomes parents).  Or it
> may show F as the last point where all lines survived up to, if D
> was picked as the earlier "parent".  Because there is no inherent
> ordering between A and D, both of which are children of O, your
> result is not necessarily "head of the last merged branch".
> 
> But I do not see how "first-parent" would be a workaround for that.
> The option would be useful to force the assignment of blame (in
> reverse) along the first-parent chain O---D---E---F---X---Y so that
> you can get a bird's-eye view of the history, i.e. squashing all
> that happened in A---B---C as if that happened at X.
> 
> The explanation of the first paragraph needs to be rewritten to make
> it understandable, but I am not sure what relevance it has with this
> change.

I understand how the blame works and why does it produce the
result which it used to produce. In one of my letter I
called it "technically correct, but absolutely useless", and
let me explain why I think so.

In a big project which uses the nowadays conventional topic
branches aka pull-requests aka however it's named workflow,
the history is a straigh first-parent chain with
short-living branches, which are forked from it, exists for
several days, then merged back and closed. When there are
many people working on a project, there can be tens of
merges during day, and average pull-request exists for
several days. So the history looks rather like (the
interesting line is removed in B1, line removal is
practically more interesting case because edits can be found
with normal forward blame):


 a0--a1-----*a2-*a3-a4...-*a100
 |\         /   /         /
 | b0-B1..bN   /         /
 |\           /         /
 | c0..   ..cN         /
 \                    /
  z0..            ..zN


...where many of the c-z branches started before a1 and
contain the older version of line. And, what I usually need
is the change b0->B1, because I expect to find there the
person who did it and explanation why that was done.

Now the git blame --reverse a0..a100 may return me zN, and in
practice it often does return some quite late commit wN
which was merged to some a90. Then, I continue search with
range a0..a89, and so on. So, to find the commit B1 I might
have to perform many blames. 

(I realize that this behavior is correct, and it's even not
obvious how to formally specify the b0 commit as something
different than zM commit, so we could discuss the
implementation of its search. But it does not make me want
being able to find it less)

In contrast, git blame --reverse --first-parent gives me a1,
and then I need only one more step:
git blame --reverse --first-parent a0..bN (--first-parent
for case there are synchronizing merges from master). And,
moreover, in the commit message of a2 I can often find the
information which I expect to find in B1, because it
summarizes what was done in the merged branch and often
contains a link to other resources, like number of ticket.

So "blame --reverse --first-parent" it not like "log
--first-parent", which just decreases number of information
but still pproduces a list which I should look through. It
is really a difference between getting answer and not
getting it.

-- 
Max

Re: [PATCH v4 1/3] Add test to describe expectation of blame --reverse with branched history

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

>> The explanation of the first paragraph needs to be rewritten to make
>> it understandable, but I am not sure what relevance it has with this
>> change.
> ...
> So the history looks rather like (the interesting line is removed
> in B1, line removal is practically more interesting case because
> edits can be found with normal forward blame):
>
>
>  a0--a1-----*a2-*a3-a4...-*a100
>  |\         /   /         /
>  | b0-B1..bN   /         /
>  |\           /         /
>  | c0..   ..cN         /
>  \                    /
>   z0..            ..zN
>
> ...where many of the c-z branches started before a1 and
> contain the older version of line. And, what I usually need
> is the change b0->B1, because I expect to find there the
> person who did it and explanation why that was done.
>
> Now the git blame --reverse a0..a100 may return me zN, and in
> practice it often does return some quite late commit wN
> which was merged to some a90.

That covers a lot of what I meant by "rewritten to make it
understandable".  Instead of picking any of the random cN thru zN,
you are forcing to find a2, because at the initial phase of the
search, you are interested in granularity at the topic level.  And
the --first-parent combined with --reverse is exactly that, and it
is a good thing to support it.

[PATCH v4 2/3] blame: extract find_single_final

[Max Kirillov]

Signed-off-by: Max Kirillov <max@max630.net>
---
 builtin/blame.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 295ce92..38f6267 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2401,16 +2401,11 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	return commit;
 }
 
-static char *prepare_final(struct scoreboard *sb)
+static struct object_array_entry *find_single_final(struct rev_info *revs)
 {
 	int i;
-	const char *final_commit_name = NULL;
-	struct rev_info *revs = sb->revs;
+	struct object_array_entry *found = NULL;
 
-	/*
-	 * There must be one and only one positive commit in the
-	 * revs->pending array.
-	 */
 	for (i = 0; i < revs->pending.nr; i++) {
 		struct object *obj = revs->pending.objects[i].item;
 		if (obj->flags & UNINTERESTING)
@@ -2419,14 +2414,24 @@ static char *prepare_final(struct scoreboard *sb)
 			obj = deref_tag(obj, NULL, 0);
 		if (obj->type != OBJ_COMMIT)
 			die("Non commit %s?", revs->pending.objects[i].name);
-		if (sb->final)
+		if (found)
 			die("More than one commit to dig from %s and %s?",
 			    revs->pending.objects[i].name,
-			    final_commit_name);
-		sb->final = (struct commit *) obj;
-		final_commit_name = revs->pending.objects[i].name;
+			    found->name);
+		found = &(revs->pending.objects[i]);
+	}
+	return found;
+}
+
+static char *prepare_final(struct scoreboard *sb)
+{
+	struct object_array_entry *found = find_single_final(sb->revs);
+	if (found) {
+		sb->final = (struct commit *) found->item;
+		return xstrdup(found->name);
+	} else {
+		return NULL;
 	}
-	return xstrdup_or_null(final_commit_name);
 }
 
 static char *prepare_initial(struct scoreboard *sb)
-- 
2.3.4.2801.g3d0809b

[PATCH v4 3/3] blame: allow blame --reverse --first-parent when it makes sense

[Max Kirillov]

Allow combining --reverse and --first-parent if initial commit of
specified range is at the first-parent chain starting from the final
commit. Disable the prepare_revision_walk()'s builtin children
collection, instead picking only the ones which are along the first
parent chain.

Signed-off-by: Max Kirillov <max@max630.net>
---
 builtin/blame.c          | 32 ++++++++++++++++++++++++++++++--
 t/t8009-blame-reverse.sh |  2 +-
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 38f6267..98b1810 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2507,6 +2507,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	long dashdash_pos, lno;
 	char *final_commit_name = NULL;
 	enum object_type type;
+	struct commit *final_commit = NULL;
 
 	static struct string_list range_list;
 	static int output_option = 0, opt = 0;
@@ -2697,11 +2698,11 @@ 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;
+		if (revs.first_parent_only)
+			revs.children.name = NULL;
 	}
 
 	if (!sb.final) {
@@ -2718,6 +2719,14 @@ parse_done:
 	else if (contents_from)
 		die("Cannot use --contents with final commit object name");
 
+	if (reverse && revs.first_parent_only) {
+		struct object_array_entry *entry = find_single_final(sb.revs);
+		if (!entry)
+			die("--reverse and --first-parent together require specified latest commit");
+		else
+			final_commit = (struct commit*) entry->item;
+	}
+
 	/*
 	 * If we have bottom, this will mark the ancestors of the
 	 * bottom commits we would reach while traversing as
@@ -2726,6 +2735,25 @@ parse_done:
 	if (prepare_revision_walk(&revs))
 		die(_("revision walk setup failed"));
 
+	if (reverse && revs.first_parent_only) {
+		struct commit *c = final_commit;
+
+		sb.revs->children.name = "children";
+		while (c->parents &&
+		       hashcmp(c->object.sha1, sb.final->object.sha1)) {
+			struct commit_list *l = xcalloc(1, sizeof(*l));
+
+			l->item = c;
+			if (add_decoration(&sb.revs->children,
+					   &c->parents->item->object, l))
+				die("BUG: not unique item in first-parent chain");
+			c = c->parents->item;
+		}
+
+		if (hashcmp(c->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..4413815 100755
--- a/t/t8009-blame-reverse.sh
+++ b/t/t8009-blame-reverse.sh
@@ -24,7 +24,7 @@ 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 &&
-- 
2.3.4.2801.g3d0809b

[PATCH v5 0/3] blame: allow blame --reverse --first-parent when it makes sense

[Max Kirillov]

Update the test:
* Fix style notes in tests
* Remove the non-first-parent case, because it's more like fature request, and is not fixed in this batch
* Rewrite the commit message, hopely now it answers better to "why"

Max Kirillov (3):
  blame: add test to describe use of blame --reverse --first-parent
  blame: extract find_single_final
  blame: allow blame --reverse --first-parent when it makes sense

 builtin/blame.c                   | 61 ++++++++++++++++++++++++++++++---------
 t/t8009-blame-vs-topicbranches.sh | 36 +++++++++++++++++++++++
 2 files changed, 83 insertions(+), 14 deletions(-)
 create mode 100755 t/t8009-blame-vs-topicbranches.sh

-- 
2.3.4.2801.g3d0809b

[PATCH v5 1/3] blame: test to describe use of blame --reverse --first-parent

[Max Kirillov]

Reverse blame can be used to locate removal of lines which does not
change adjacent lines. Such edits do not appear in non-reverse blame,
because the adjacent lines last changed commit is older history, before
the edit.

For a big and active project which uses topic branches, or analogous
feature, for example pull-requests, the history can contain many
concurrent branches, and even after an edit merged into the target
branch, there are still many (sometimes several tens or even hundreds)
topic branch which do not contain it:

 a0--a1-----*a2-*a3-a4...-*a100
 |\         /   /         /
 | b0-B1..bN   /         /
 |\           /         /
 | c0..   ..cN         /
 \                    /
  z0..            ..zN

Here, the '*'s mark the first parent in merge, and uppercase B1 - the
commit where the line being blamed for was removed. Since commits cN-zN
do not contain the B1, the still have the line removed in B1, and
reverce blame can report that the last commit for the line was zN
(meaning that it was removed in a100). In fact it really does return
some very late commit, and this makes it unusable for finding the B1
commit.

The search could be done by blame --reverse --first-parent. For range
a0..a100 it would return a1, and then only one additional blame along
the a0..bN will return the desired commit b0. But combining --reverse
and --first-parent was forbidden in 95a4fb0eac, because incorrectly
specified range could produce unexpected and meaningless result.

Add test which describes the expected behavior of
`blame --reverse --first-parent` in the case described above.

Signed-off-by: Max Kirillov <max@max630.net>
---
 t/t8009-blame-vs-topicbranches.sh | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100755 t/t8009-blame-vs-topicbranches.sh

diff --git a/t/t8009-blame-vs-topicbranches.sh b/t/t8009-blame-vs-topicbranches.sh
new file mode 100755
index 0000000..175ad37
--- /dev/null
+++ b/t/t8009-blame-vs-topicbranches.sh
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+test_description='blaming trough history with topic branches'
+. ./test-lib.sh
+
+# Creates the history shown below. '*'s mark the first parent in the merges.
+# The only line of file.t is changed in commit B2
+#
+#        +---C1
+#       /      \
+# A0--A1--*A2--*A3
+#   \     /
+#    B1-B2
+#
+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
+	'
+
+test_expect_failure 'blame --reverse --first-parent finds A1' '
+	git blame --porcelain --reverse --first-parent A0..A3 -- file.t >actual_full &&
+	head -n 1 <actual_full | sed -e "s/ .*//" >actual &&
+	git rev-parse A1 >expect &&
+	test_cmp expect actual
+	'
+
+test_done
-- 
2.3.4.2801.g3d0809b

Re: [PATCH v5 1/3] blame: test to describe use of blame --reverse --first-parent

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

> Reverse blame can be used to locate removal of lines which does not
> change adjacent lines. Such edits do not appear in non-reverse blame,
> because the adjacent lines last changed commit is older history, before
> the edit.
>
> For a big and active project which uses topic branches, or analogous
> feature, for example pull-requests, the history can contain many
> concurrent branches, and even after an edit merged into the target
> branch, there are still many (sometimes several tens or even hundreds)
> topic branch which do not contain it:
>
>  a0--a1-----*a2-*a3-a4...-*a100
>  |\         /   /         /
>  | b0-B1..bN   /         /
>  |\           /         /
>  | c0..   ..cN         /
>  \                    /
>   z0..            ..zN
>
> Here, the '*'s mark the first parent in merge, and uppercase B1 - the
> commit where the line being blamed for was removed. Since commits cN-zN
> do not contain the B1, the still have the line removed in B1, and
> reverce blame can report that the last commit for the line was zN
> (meaning that it was removed in a100). In fact it really does return
> some very late commit, and this makes it unusable for finding the B1
> commit.
>
> The search could be done by blame --reverse --first-parent. For range
> a0..a100 it would return a1, and then only one additional blame along
> the a0..bN will return the desired commit b0. But combining --reverse
> and --first-parent was forbidden in 95a4fb0eac, because incorrectly
> specified range could produce unexpected and meaningless result.
>
> Add test which describes the expected behavior of
> `blame --reverse --first-parent` in the case described above.
>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---

There were a few obvious typos I spotted but other than that a very
understandable description.  Will queue.

Thanks.

>  t/t8009-blame-vs-topicbranches.sh | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100755 t/t8009-blame-vs-topicbranches.sh
>
> diff --git a/t/t8009-blame-vs-topicbranches.sh b/t/t8009-blame-vs-topicbranches.sh
> new file mode 100755
> index 0000000..175ad37
> --- /dev/null
> +++ b/t/t8009-blame-vs-topicbranches.sh
> @@ -0,0 +1,36 @@
> +#!/bin/sh
> +
> +test_description='blaming trough history with topic branches'
> +. ./test-lib.sh
> +
> +# Creates the history shown below. '*'s mark the first parent in the merges.
> +# The only line of file.t is changed in commit B2
> +#
> +#        +---C1
> +#       /      \
> +# A0--A1--*A2--*A3
> +#   \     /
> +#    B1-B2
> +#
> +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
> +	'
> +
> +test_expect_failure 'blame --reverse --first-parent finds A1' '
> +	git blame --porcelain --reverse --first-parent A0..A3 -- file.t >actual_full &&
> +	head -n 1 <actual_full | sed -e "s/ .*//" >actual &&
> +	git rev-parse A1 >expect &&
> +	test_cmp expect actual
> +	'
> +
> +test_done

[PATCH v5 2/3] blame: extract find_single_final

[Max Kirillov]

Signed-off-by: Max Kirillov <max@max630.net>
---
 builtin/blame.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 295ce92..38f6267 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2401,16 +2401,11 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	return commit;
 }
 
-static char *prepare_final(struct scoreboard *sb)
+static struct object_array_entry *find_single_final(struct rev_info *revs)
 {
 	int i;
-	const char *final_commit_name = NULL;
-	struct rev_info *revs = sb->revs;
+	struct object_array_entry *found = NULL;
 
-	/*
-	 * There must be one and only one positive commit in the
-	 * revs->pending array.
-	 */
 	for (i = 0; i < revs->pending.nr; i++) {
 		struct object *obj = revs->pending.objects[i].item;
 		if (obj->flags & UNINTERESTING)
@@ -2419,14 +2414,24 @@ static char *prepare_final(struct scoreboard *sb)
 			obj = deref_tag(obj, NULL, 0);
 		if (obj->type != OBJ_COMMIT)
 			die("Non commit %s?", revs->pending.objects[i].name);
-		if (sb->final)
+		if (found)
 			die("More than one commit to dig from %s and %s?",
 			    revs->pending.objects[i].name,
-			    final_commit_name);
-		sb->final = (struct commit *) obj;
-		final_commit_name = revs->pending.objects[i].name;
+			    found->name);
+		found = &(revs->pending.objects[i]);
+	}
+	return found;
+}
+
+static char *prepare_final(struct scoreboard *sb)
+{
+	struct object_array_entry *found = find_single_final(sb->revs);
+	if (found) {
+		sb->final = (struct commit *) found->item;
+		return xstrdup(found->name);
+	} else {
+		return NULL;
 	}
-	return xstrdup_or_null(final_commit_name);
 }
 
 static char *prepare_initial(struct scoreboard *sb)
-- 
2.3.4.2801.g3d0809b

[PATCH v5 3/3] blame: allow blame --reverse --first-parent when it makes sense

[Max Kirillov]

Allow combining --reverse and --first-parent if initial commit of
specified range is at the first-parent chain starting from the final
commit. Disable the prepare_revision_walk()'s builtin children
collection, instead picking only the ones which are along the first
parent chain.

Signed-off-by: Max Kirillov <max@max630.net>
---
 builtin/blame.c                   | 32 ++++++++++++++++++++++++++++++--
 t/t8009-blame-vs-topicbranches.sh |  2 +-
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 38f6267..98b1810 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2507,6 +2507,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	long dashdash_pos, lno;
 	char *final_commit_name = NULL;
 	enum object_type type;
+	struct commit *final_commit = NULL;
 
 	static struct string_list range_list;
 	static int output_option = 0, opt = 0;
@@ -2697,11 +2698,11 @@ 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;
+		if (revs.first_parent_only)
+			revs.children.name = NULL;
 	}
 
 	if (!sb.final) {
@@ -2718,6 +2719,14 @@ parse_done:
 	else if (contents_from)
 		die("Cannot use --contents with final commit object name");
 
+	if (reverse && revs.first_parent_only) {
+		struct object_array_entry *entry = find_single_final(sb.revs);
+		if (!entry)
+			die("--reverse and --first-parent together require specified latest commit");
+		else
+			final_commit = (struct commit*) entry->item;
+	}
+
 	/*
 	 * If we have bottom, this will mark the ancestors of the
 	 * bottom commits we would reach while traversing as
@@ -2726,6 +2735,25 @@ parse_done:
 	if (prepare_revision_walk(&revs))
 		die(_("revision walk setup failed"));
 
+	if (reverse && revs.first_parent_only) {
+		struct commit *c = final_commit;
+
+		sb.revs->children.name = "children";
+		while (c->parents &&
+		       hashcmp(c->object.sha1, sb.final->object.sha1)) {
+			struct commit_list *l = xcalloc(1, sizeof(*l));
+
+			l->item = c;
+			if (add_decoration(&sb.revs->children,
+					   &c->parents->item->object, l))
+				die("BUG: not unique item in first-parent chain");
+			c = c->parents->item;
+		}
+
+		if (hashcmp(c->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-vs-topicbranches.sh b/t/t8009-blame-vs-topicbranches.sh
index 175ad37..72596e3 100755
--- a/t/t8009-blame-vs-topicbranches.sh
+++ b/t/t8009-blame-vs-topicbranches.sh
@@ -26,7 +26,7 @@ test_expect_success setup '
 	test_merge A3 C1
 	'
 
-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 -n 1 <actual_full | sed -e "s/ .*//" >actual &&
 	git rev-parse A1 >expect &&
-- 
2.3.4.2801.g3d0809b

[PATCH v4 0/4] blame: allow blame --reverse --first-parent when it makes sense

[Max Kirillov]

* Change aproach - instead of assuming children content build them explicitly,
  also accurately verifying that the specified range is along the first-parent chain
* Fix error message

Max Kirillov (4):
  Add test to describe expectation of blame --reverse with branched
    history
  blame: extract find_single_final
  blame: allow blame --reverse --first-parent when it makes sense
  blame: fix option name in error message

 builtin/blame.c          | 63 ++++++++++++++++++++++++++++++++++++------------
 t/t8009-blame-reverse.sh | 34 ++++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 15 deletions(-)
 create mode 100755 t/t8009-blame-reverse.sh

-- 
2.3.4.2801.g3d0809b

Re: [PATCH v4 0/4] blame: allow blame --reverse --first-parent when it makes sense

[Max Kirillov]

On Mon, Oct 26, 2015 at 07:26:54AM +0200, Max Kirillov wrote:
> * Change aproach - instead of assuming children content build them explicitly,
>   also accurately verifying that the specified range is along the first-parent chain
> * Fix error message

Sent by mistake, ignore this, please