{"thread":{"id":"40602","subject":"[PATCH 0/2] blame: allow blame --reverse --first-parent when it makes sense","startedAt":"2015-10-21T04:08:00Z","lastAt":"2015-10-22T03:52:00Z","messageCount":5,"participants":["Max Kirillov","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"272017","messageId":"1445400482-1977-1-git-send-email-max@max630.net","threadId":"40602","inReplyTo":null,"subject":"[PATCH 0/2] blame: allow blame --reverse --first-parent when it makes sense","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-21T04:08:00Z","receivedAt":"2015-10-21T04:08:00Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"This enables --reverse --first-parent back.\n\nMax Kirillov (2):\n Add test to describe expectation of blame --reverse with branched\n history\n blame: allow blame --reverse --first-parent when it makes sense\n\n builtin/blame.c | 11 +++++++++--\n t/t8009-blame-reverse.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 2 deletions(-)\n create mode 100755 t/t8009-blame-reverse.sh\n\n-- \n2.3.4.2801.g3d0809b\n"},{"id":"272018","messageId":"1445400482-1977-2-git-send-email-max@max630.net","threadId":"40602","inReplyTo":"1445400482-1977-1-git-send-email-max@max630.net","subject":"[PATCH 1/2] Add test to describe expectation of blame --reverse with branched history","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-21T04:08:01Z","receivedAt":"2015-10-21T04:08:01Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"If history contains merges from feature branches, `blame --reverse`\nreports not the commit when the line was actually edited, but head of\nthe last merged branch which was created before the edit.\n\nAs a workaround, `blame --reverse --first-parent` could be used to find\nthe merge of branch containing the edit, but it was disabled in\n95a4fb0eac, because incorrectly specified range could produce in\nunexpected and meaningless result.\n\nAdd tests which describe ideal functionality with and without\n`--first-parent`.\n\nSigned-off-by: Max Kirillov \n---\n t/t8009-blame-reverse.sh | 34 ++++++++++++++++++++++++++++++++++\n 1 file changed, 34 insertions(+)\n create mode 100755 t/t8009-blame-reverse.sh\n\ndiff --git a/t/t8009-blame-reverse.sh b/t/t8009-blame-reverse.sh\nnew file mode 100755\nindex 0000000..9f40613\n--- /dev/null\n+++ b/t/t8009-blame-reverse.sh\n@@ -0,0 +1,34 @@\n+#!/bin/sh\n+\n+test_description='git blame reverse'\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\ttest_commit A0 file.t line0 &&\n+\ttest_commit A1 &&\n+\tgit reset --hard A0 &&\n+\ttest_commit B1 &&\n+\ttest_commit B2 file.t line0changed &&\n+\tgit reset --hard A1 &&\n+\ttest_merge A2 B2 &&\n+\tgit reset --hard A1 &&\n+\ttest_commit C1 &&\n+\tgit reset --hard A2 &&\n+\ttest_merge A3 C1\n+\t'\n+\n+test_expect_failure 'blame --reverse finds B1, not C1' '\n+\tgit blame --porcelain --reverse A0..A3 -- file.t >actual_full &&\n+\thead -1 actual &&\n+\tgit rev-parse B1 >expect &&\n+\ttest_cmp expect actual\n+\t'\n+\n+test_expect_failure 'blame --reverse --first-parent finds A1' '\n+\tgit blame --porcelain --reverse --first-parent A0..A3 -- file.t >actual_full &&\n+\thead -1 actual &&\n+\tgit rev-parse A1 >expect &&\n+\ttest_cmp expect actual\n+\t'\n+\n+test_done\n-- \n2.3.4.2801.g3d0809b\n"},{"id":"272019","messageId":"1445400482-1977-3-git-send-email-max@max630.net","threadId":"40602","inReplyTo":"1445400482-1977-1-git-send-email-max@max630.net","subject":"[PATCH 2/2] blame: allow blame --reverse --first-parent when it makes sense","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-21T04:08:02Z","receivedAt":"2015-10-21T04:08:02Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Do not die immediately when the two flags are specified. Instead\ncheck that the specified range is along first-parent chain. Explioit\nhow prepare_revision_walk() handles first_parent_only flag: the commits\noutside of first-parent chain are either unknown (and do not have any\nchildren recorded) or appear as non-first parent of a commit along the\nfirst-parent chain.\n\nSince the check seems fragile, add test which makes sure that blame dies\nin both cases.\n\nSigned-off-by: Max Kirillov \n---\n builtin/blame.c | 11 +++++++++--\n t/t8009-blame-reverse.sh | 7 ++++++-\n 2 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 295ce92..27de544 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2692,8 +2692,6 @@ parse_done:\n \t}\n \telse if (contents_from)\n \t\tdie(\"--contents and --children do not blend well.\");\n-\telse if (revs.first_parent_only)\n-\t\tdie(\"combining --first-parent and --reverse is not supported\");\n \telse {\n \t\tfinal_commit_name = prepare_initial(&sb);\n \t\tsb.commits.compare = compare_commits_by_reverse_commit_date;\n@@ -2721,6 +2719,15 @@ parse_done:\n \tif (prepare_revision_walk(&revs))\n \t\tdie(_(\"revision walk setup failed\"));\n \n+\tif (reverse && revs.first_parent_only) {\n+\t\tstruct commit_list *final_children = lookup_decoration(&revs.children,\n+\t\t\t\t\t\t\t\t &sb.final->object);\n+\t\tif (!final_children ||\n+\t\t hashcmp(final_children->item->parents->item->object.sha1,\n+\t\t\t sb.final->object.sha1))\n+\t\t die(\"--reverse --first-parent together require range along first-parent chain\");\n+\t}\n+\n \tif (is_null_sha1(sb.final->object.sha1)) {\n \t\to = sb.final->util;\n \t\tsb.final_buf = xmemdupz(o->file.ptr, o->file.size);\ndiff --git a/t/t8009-blame-reverse.sh b/t/t8009-blame-reverse.sh\nindex 9f40613..042863b 100755\n--- a/t/t8009-blame-reverse.sh\n+++ b/t/t8009-blame-reverse.sh\n@@ -24,11 +24,16 @@ test_expect_failure 'blame --reverse finds B1, not C1' '\n \ttest_cmp expect actual\n \t'\n \n-test_expect_failure 'blame --reverse --first-parent finds A1' '\n+test_expect_success 'blame --reverse --first-parent finds A1' '\n \tgit blame --porcelain --reverse --first-parent A0..A3 -- file.t >actual_full &&\n \thead -1 actual &&\n \tgit rev-parse A1 >expect &&\n \ttest_cmp expect actual\n \t'\n \n+test_expect_success 'blame --reverse --first-parse dies if no first parent chain' '\n+\ttest_must_fail git blame --porcelain --reverse --first-parent B1..A3 -- file.t &&\n+\ttest_must_fail git blame --porcelain --reverse --first-parent B2..A3 -- file.t\n+\t'\n+\n test_done\n-- \n2.3.4.2801.g3d0809b\n"},{"id":"272020","messageId":"CAPig+cQ_cYQ0i8PtidiLJjBr2UqwkPBW5Kv20VeN7AsmeWBsPg@mail.gmail.com","threadId":"40602","inReplyTo":"1445400482-1977-3-git-send-email-max@max630.net","subject":"Re: [PATCH 2/2] blame: allow blame --reverse --first-parent when it makes sense","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-10-21T04:29:12Z","receivedAt":"2015-10-21T04:29:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Oct 21, 2015 at 12:08 AM, Max Kirillov wrote:\n> Do not die immediately when the two flags are specified. Instead\n> check that the specified range is along first-parent chain. Explioit\n\ns/Explioit/Exploit/\n\n> how prepare_revision_walk() handles first_parent_only flag: the commits\n> outside of first-parent chain are either unknown (and do not have any\n> children recorded) or appear as non-first parent of a commit along the\n> first-parent chain.\n>\n> Since the check seems fragile, add test which makes sure that blame dies\n> in both cases.\n>\n> Signed-off-by: Max Kirillov \n> ---\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 295ce92..27de544 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2692,8 +2692,6 @@ parse_done:\n> }\n> else if (contents_from)\n> die(\"--contents and --children do not blend well.\");\n> - else if (revs.first_parent_only)\n> - die(\"combining --first-parent and --reverse is not supported\");\n> else {\n> final_commit_name = prepare_initial(&sb);\n> sb.commits.compare = compare_commits_by_reverse_commit_date;\n> @@ -2721,6 +2719,15 @@ parse_done:\n> if (prepare_revision_walk(&revs))\n> die(_(\"revision walk setup failed\"));\n>\n> + if (reverse && revs.first_parent_only) {\n> + struct commit_list *final_children = lookup_decoration(&revs.children,\n> + &sb.final->object);\n> + if (!final_children ||\n> + hashcmp(final_children->item->parents->item->object.sha1,\n> + sb.final->object.sha1))\n> + die(\"--reverse --first-parent together require range along first-parent chain\");\n> + }\n> +\n> if (is_null_sha1(sb.final->object.sha1)) {\n> o = sb.final->util;\n> sb.final_buf = xmemdupz(o->file.ptr, o->file.size);\n> diff --git a/t/t8009-blame-reverse.sh b/t/t8009-blame-reverse.sh\n> index 9f40613..042863b 100755\n> --- a/t/t8009-blame-reverse.sh\n> +++ b/t/t8009-blame-reverse.sh\n> @@ -24,11 +24,16 @@ test_expect_failure 'blame --reverse finds B1, not C1' '\n> test_cmp expect actual\n> '\n>\n> -test_expect_failure 'blame --reverse --first-parent finds A1' '\n> +test_expect_success 'blame --reverse --first-parent finds A1' '\n> git blame --porcelain --reverse --first-parent A0..A3 -- file.t >actual_full &&\n> head -1 actual &&\n> git rev-parse A1 >expect &&\n> test_cmp expect actual\n> '\n>\n> +test_expect_success 'blame --reverse --first-parse dies if no first parent chain' '\n> + test_must_fail git blame --porcelain --reverse --first-parent B1..A3 -- file.t &&\n> + test_must_fail git blame --porcelain --reverse --first-parent B2..A3 -- file.t\n> + '\n> +\n> test_done\n> --\n> 2.3.4.2801.g3d0809b\n"},{"id":"272081","messageId":"20151022035200.GB5722@wheezy.local","threadId":"40602","inReplyTo":"CAPig+cQ_cYQ0i8PtidiLJjBr2UqwkPBW5Kv20VeN7AsmeWBsPg@mail.gmail.com","subject":"Re: [PATCH 2/2] blame: allow blame --reverse --first-parent when it makes sense","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-22T03:52:00Z","receivedAt":"2015-10-22T03:52:00Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Wed, Oct 21, 2015 at 12:29:12AM -0400, Eric Sunshine wrote:\n> On Wed, Oct 21, 2015 at 12:08 AM, Max Kirillov wrote:\n>> Do not die immediately when the two flags are specified. Instead\n>> check that the specified range is along first-parent chain. Explioit\n> \n> s/Explioit/Exploit/\n\nFixed. Thanks.\n\n-- \nMax\n"}]}