threads / patch / 40602

patch, 2 partsblame: allow blame --reverse --first-parent when it makes sense

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

## tl;dr

5 messages between Oct 21, 2015 and Oct 22, 2015. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Max Kirillov· Oct 21, 2015, 04:08 UTC · lore

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

Max Kirillov· Oct 21, 2015, 04:08 UTC · re: Max Kirillov · lore

[PATCH 1/2] Add 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

Show changes to t/t8009-blame-reverse.sh +34 −0

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

Max Kirillov· Oct 21, 2015, 04:08 UTC · re: Max Kirillov · lore

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

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

Show changes to 2 files +15 −3

builtin/blame.c, t/t8009-blame-reverse.sh

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

Eric Sunshine· Oct 21, 2015, 04:29 UTC · re: Max Kirillov · lore

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

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/

Show 63 quoted lines

> 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

Max Kirillov· Oct 22, 2015, 03:52 UTC · re: Eric Sunshine · lore

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

On Wed, Oct 21, 2015 at 12:29:12AM -0400, Eric Sunshine wrote:

Show 5 quoted lines

> 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