{"thread":{"id":"40613","subject":"[PATCH v2 1/2] Add test to describe expectation of blame --reverse with branched history","startedAt":"2015-10-22T03:51:10Z","lastAt":"2015-10-25T16:52:53Z","messageCount":11,"participants":["Max Kirillov","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"272079","messageId":"1445485872-21453-1-git-send-email-max@max630.net","threadId":"40613","inReplyTo":"1445400482-1977-1-git-send-email-max@max630.net","subject":"[PATCH v2 0/2] blame: allow blame --reverse --first-parent when it makes sense","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-22T03:51:10Z","receivedAt":"2015-10-22T03:51:10Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Fixed mistype in commit message.\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":"272078","messageId":"1445485872-21453-2-git-send-email-max@max630.net","threadId":"40613","inReplyTo":"1445485872-21453-1-git-send-email-max@max630.net","subject":"[PATCH v2 1/2] Add test to describe expectation of blame --reverse with branched history","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-22T03:51:11Z","receivedAt":"2015-10-22T03:51:11Z","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":"272080","messageId":"1445485872-21453-3-git-send-email-max@max630.net","threadId":"40613","inReplyTo":"1445485872-21453-1-git-send-email-max@max630.net","subject":"[PATCH v2 2/2] blame: allow blame --reverse --first-parent when it makes sense","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-22T03:51:12Z","receivedAt":"2015-10-22T03:51:12Z","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. Exploit\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 verifies 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":"272085","messageId":"xmqqk2qfttzy.fsf@gitster.mtv.corp.google.com","threadId":"40613","inReplyTo":"1445485872-21453-2-git-send-email-max@max630.net","subject":"Re: [PATCH v2 1/2] Add test to describe expectation of blame --reverse with branched history","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-22T04:19:45Z","receivedAt":"2015-10-22T04:19:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov writes:\n\n> If history contains merges from feature branches, `blame --reverse`\n> reports not the commit when the line was actually edited, but head of\n> the last merged branch which was created before the edit.\n>\n> As a workaround, `blame --reverse --first-parent` could be used to find\n> the merge of branch containing the edit, but it was disabled in\n> 95a4fb0eac, because incorrectly specified range could produce in\n> unexpected and meaningless result.\n>\n> Add tests which describe ideal functionality with and without\n> `--first-parent`.\n>\n> Signed-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>\n> diff --git a/t/t8009-blame-reverse.sh b/t/t8009-blame-reverse.sh\n> new file mode 100755\n> index 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\nIf you are going to dedicate the whole test script for this test,\ncould you draw the topology of the commit DAG here, so that readers\ncan more easily follow what is going on?\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\nLet me try.\n\n (1) for merges, an edge with '*' denotes the one to the first\n parent.\n (2) a commit that touches file.t are in capital\n\n c1---a3\n / *\n / /\n A0---a1--*a2\n \\ /\n b1---B2\n\nDid I draw it correctly?\n\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\n\"head -n 1\" is more POSIXy way to spell this.\n\nDon't get cute with sXXX in sed script when the usual s/// suffices;\nthe only effect of the cuteness is to waste readers' time, who have\nto wonder if there is something interesting going on.\n\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"},{"id":"272086","messageId":"xmqqfv13ttq6.fsf@gitster.mtv.corp.google.com","threadId":"40613","inReplyTo":"1445485872-21453-3-git-send-email-max@max630.net","subject":"Re: [PATCH v2 2/2] blame: allow blame --reverse --first-parent when it makes sense","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-22T04:25:37Z","receivedAt":"2015-10-22T04:25:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov writes:\n\n> Do not die immediately when the two flags are specified. Instead\n> check that the specified range is along first-parent chain. Exploit\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 verifies that blame dies\n> in both cases.\n\nIt is not quite clear in what way the \"check seems fragile\".\n\nIt is either \"correct\" or \"appears to have worked by chance and\nnobody has any confidence that it would tell if 'it makes sense'\nreliably\", and the latter cannot be papered over with any number of\ntests.\n\nThe logic you implemented feels solid to me, at least at a first\nglance. What kind of gotchas are you worried about?\n\n> Signed-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>\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> \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);\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> \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"},{"id":"272102","messageId":"20151022143721.GC5722@wheezy.local","threadId":"40613","inReplyTo":"xmqqk2qfttzy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/2] Add test to describe expectation of blame --reverse with branched history","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-22T14:37:21Z","receivedAt":"2015-10-22T14:37:21Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Wed, Oct 21, 2015 at 09:19:45PM -0700, Junio C Hamano wrote:\n> Let me try.\n> \n> (1) for merges, an edge with '*' denotes the one to the first\n> parent.\n> (2) a commit that touches file.t are in capital\n> \n> c1---a3\n> / *\n> / /\n> A0---a1--*a2\n> \\ /\n> b1---B2\n> \n> Did I draw it correctly?\n\nYes. Though I would\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> \n> \"head -n 1\" is more POSIXy way to spell this.\n> \n> Don't get cute with sXXX in sed script when the usual s/// suffices;\n> the only effect of the cuteness is to waste readers' time, who have\n> to wonder if there is something interesting going on.\n\nOk. Will fix it and add the graph.\n"},{"id":"272103","messageId":"20151022145606.GD5722@wheezy.local","threadId":"40613","inReplyTo":"xmqqfv13ttq6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] blame: allow blame --reverse --first-parent when it makes sense","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-22T14:56:06Z","receivedAt":"2015-10-22T14:56:06Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Wed, Oct 21, 2015 at 09:25:37PM -0700, Junio C Hamano wrote:\n> Max Kirillov writes:\n> \n>> Do not die immediately when the two flags are specified. Instead\n>> check that the specified range is along first-parent chain. Exploit\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 verifies that blame dies\n>> in both cases.\n> \n> It is not quite clear in what way the \"check seems fragile\".\n> \n> It is either \"correct\" or \"appears to have worked by chance and\n> nobody has any confidence that it would tell if 'it makes sense'\n> reliably\", and the latter cannot be papered over with any number of\n> tests.\n> \n> The logic you implemented feels solid to me, at least at a first\n> glance. What kind of gotchas are you worried about?\n\nThe fact than arbitrary commit's children are unknown does\nnot seem reliable to me. It more fits the \"works by chance\"\ndescription.\n\nActually, I realized that this can produce false positive if\nthe final commit is no-ff merged into first-parent chain,\nbecause it woul be both first and non-first parent of the\nfirst-parent chain.\nBut afaik such merge is not possible to create by frontent git\ncommands, so I'm not sure I should consider such case.\n\n-- \nMax\n"},{"id":"272104","messageId":"20151022145657.GE5722@wheezy.local","threadId":"40613","inReplyTo":"20151022143721.GC5722@wheezy.local","subject":"Re: [PATCH v2 1/2] Add test to describe expectation of blame --reverse with branched history","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-22T14:56:57Z","receivedAt":"2015-10-22T14:56:57Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Thu, Oct 22, 2015 at 05:37:21PM +0300, Max Kirillov wrote:\n> Yes. Though I would\n\n... make the line of As straight.\n"},{"id":"272117","messageId":"xmqqvb9yra5g.fsf@gitster.mtv.corp.google.com","threadId":"40613","inReplyTo":"20151022145606.GD5722@wheezy.local","subject":"Re: [PATCH v2 2/2] blame: allow blame --reverse --first-parent when it makes sense","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-22T19:11:23Z","receivedAt":"2015-10-22T19:11:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov writes:\n\n> On Wed, Oct 21, 2015 at 09:25:37PM -0700, Junio C Hamano wrote:\n>\n>> The logic you implemented feels solid to me, at least at a first\n>> glance. What kind of gotchas are you worried about?\n>\n> The fact than arbitrary commit's children are unknown does\n> not seem reliable to me. It more fits the \"works by chance\"\n> description.\n\nThat's sad.\n"},{"id":"272186","messageId":"20151025124338.GA27615@wheezy.local","threadId":"40613","inReplyTo":"xmqqvb9yra5g.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] blame: allow blame --reverse --first-parent when it makes sense","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-25T12:43:38Z","receivedAt":"2015-10-25T12:43:38Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Thu, Oct 22, 2015 at 12:11:23PM -0700, Junio C Hamano wrote:\n> Max Kirillov writes:\n>> On Wed, Oct 21, 2015 at 09:25:37PM -0700, Junio C Hamano wrote:\n>>\n>>> The logic you implemented feels solid to me, at least at a first\n>>> glance. What kind of gotchas are you worried about?\n>>\n>> The fact than arbitrary commit's children are unknown does\n>> not seem reliable to me. It more fits the \"works by chance\"\n>> description.\n>\n> That's sad.\n\nHow about this then? I could collect only children\nalong the first-parent chain. This makes sure that\nno extra child entry appear in revs->children. Since the revs\nstructure is exclusive for cmd_blame() it is guaranteed\nthat no other command will affect or be affected by this\nbehavior.\n\nThis additionally forbids having several end commits\n(git blame --reverse --first-parent ^A B C ...), but\nthis does not seem to have much practical use.\n\n-- \nMax\n\n8< ------------------\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex eb348f0..f66d0ae 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2401,16 +2401,11 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \treturn commit;\n }\n \n-static char *prepare_final(struct scoreboard *sb)\n+static struct object_array_entry *find_single_final(struct rev_info *revs)\n {\n \tint i;\n-\tconst char *final_commit_name = NULL;\n-\tstruct rev_info *revs = sb->revs;\n+\tstruct object_array_entry *found = NULL;\n \n-\t/*\n-\t * There must be one and only one positive commit in the\n-\t * revs->pending array.\n-\t */\n \tfor (i = 0; i < revs->pending.nr; i++) {\n \t\tstruct object *obj = revs->pending.objects[i].item;\n \t\tif (obj->flags & UNINTERESTING)\n@@ -2419,14 +2414,24 @@ static char *prepare_final(struct scoreboard *sb)\n \t\t\tobj = deref_tag(obj, NULL, 0);\n \t\tif (obj->type != OBJ_COMMIT)\n \t\t\tdie(\"Non commit %s?\", revs->pending.objects[i].name);\n-\t\tif (sb->final)\n+\t\tif (found)\n \t\t\tdie(\"More than one commit to dig from %s and %s?\",\n \t\t\t revs->pending.objects[i].name,\n-\t\t\t final_commit_name);\n-\t\tsb->final = (struct commit *) obj;\n-\t\tfinal_commit_name = revs->pending.objects[i].name;\n+\t\t\t found->name);\n+\t\tfound = &(revs->pending.objects[i]);\n+\t}\n+\treturn found;\n+}\n+\n+static char *prepare_final(struct scoreboard *sb)\n+{\n+\tstruct object_array_entry *found = find_single_final(sb->revs);\n+\tif (found) {\n+\t\tsb->final = (struct commit *) found->item;\n+\t\treturn xstrdup(found->name);\n+\t} else {\n+\t\treturn NULL;\n \t}\n-\treturn xstrdup_or_null(final_commit_name);\n }\n \n static char *prepare_initial(struct scoreboard *sb)\n@@ -2502,6 +2507,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tlong dashdash_pos, lno;\n \tchar *final_commit_name = NULL;\n \tenum object_type type;\n+\tstruct commit *firstparent_chain_initial = NULL;\n \n \tstatic struct string_list range_list;\n \tstatic int output_option = 0, opt = 0;\n@@ -2695,6 +2701,8 @@ parse_done:\n \telse {\n \t\tfinal_commit_name = prepare_initial(&sb);\n \t\tsb.commits.compare = compare_commits_by_reverse_commit_date;\n+\t\tif (revs.first_parent_only)\n+\t\t\trevs.children.name = NULL;\n \t}\n \n \tif (!sb.final) {\n@@ -2711,6 +2719,14 @@ parse_done:\n \telse if (contents_from)\n \t\tdie(\"Cannot use --contents with final commit object name\");\n \n+\tif (reverse && revs.first_parent_only) {\n+\t\tstruct object_array_entry *entry = find_single_final(sb.revs);\n+\t\tif (!entry)\n+\t\t\tdie(\"--reverse and --first-parent together require specified latest commit\");\n+\t\telse\n+\t\t\tfirstparent_chain_initial = (struct commit*) entry->item;\n+\t}\n+\n \t/*\n \t * If we have bottom, this will mark the ancestors of the\n \t * bottom commits we would reach while traversing as\n@@ -2720,11 +2736,21 @@ parse_done:\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\tstruct commit *c = firstparent_chain_initial;\n+\n+\t\tsb.revs->children.name = \"children\";\n+\t\twhile (c->parents &&\n+\t\t hashcmp(c->object.sha1, sb.final->object.sha1)) {\n+\t\t\tstruct commit_list *l = xcalloc(1, sizeof(*l));\n+\n+\t\t\tl->item = c;\n+\t\t\tif (add_decoration(&sb.revs->children,\n+\t\t\t\t\t &c->parents->item->object, l))\n+\t\t\t\tdie(\"BUG: not unique item in first-parent chain\");\n+\t\t\tc = c->parents->item;\n+\t\t}\n+\n+\t\tif (hashcmp(c->object.sha1, sb.final->object.sha1))\n \t\t\tdie(\"--reverse --first-parent together require range along first-parent chain\");\n \t}\n \n"},{"id":"272204","messageId":"xmqqk2qalwka.fsf@gitster.mtv.corp.google.com","threadId":"40613","inReplyTo":"20151025124338.GA27615@wheezy.local","subject":"Re: [PATCH v2 2/2] blame: allow blame --reverse --first-parent when it makes sense","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-25T16:52:53Z","receivedAt":"2015-10-25T16:52:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov writes:\n\n> This additionally forbids having several end commits\n> (git blame --reverse --first-parent ^A B C ...), but\n> this does not seem to have much practical use.\n\nEven if there were practical uses for a request with multiple end\npoints, a solution that\n\n (1) guarantees that \"blame --reverse --first-parent\" is the only\n thing that is affected; and\n\n (2) errors out if \"blame --reverse --first-parent\" cannot give a\n sensible answer.\n\nis a strict improvement over \"we don't know how to produce a\nsensible answer in all cases, so we just forbid\". Somebody who\nneeds multiple end points badly can build on top with the same\nprinciple, just widening the definition of what is \"a sensible\nanswer\" in the criteria (2) above. Until then, defining the\nsensible answer to be \"an answer that consists of commits on the\nfirst parent chain (but in reverse)\" is perfectly fine [*1*].\n\nAs you added your patch after \"-- \", my MUA refuses to quote it\nin this response, but that's OK, as this message is not about the\npatch itself ;-)\n\n\n[Footnote]\n\n*1* I haven't read the patch text to see if that is what the updated\n code is computing, though. That's for a weekday ;-).\n"}]}