{"thread":{"id":"40614","subject":"[PATCH v3 1/2] Add test to describe expectation of blame --reverse with branched history","startedAt":"2015-10-22T04:07:37Z","lastAt":"2015-10-27T17:57:48Z","messageCount":13,"participants":["Max Kirillov","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"272084","messageId":"1445486859-21906-1-git-send-email-max@max630.net","threadId":"40614","inReplyTo":"1445485872-21453-1-git-send-email-max@max630.net","subject":"[PATCH v3 0/2] blame: allow blame --reverse --first-parent when it makes sense","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-22T04:07:37Z","receivedAt":"2015-10-22T04:07:37Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Fix indentation.\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":"272082","messageId":"1445486859-21906-2-git-send-email-max@max630.net","threadId":"40614","inReplyTo":"1445486859-21906-1-git-send-email-max@max630.net","subject":"[PATCH v3 1/2] Add test to describe expectation of blame --reverse with branched history","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-22T04:07:38Z","receivedAt":"2015-10-22T04:07:38Z","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 <max@max630.net>\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_full | sed -e \"sX .*XX\" >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_full | sed -e \"sX .*XX\" >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":"272083","messageId":"1445486859-21906-3-git-send-email-max@max630.net","threadId":"40614","inReplyTo":"1445486859-21906-1-git-send-email-max@max630.net","subject":"[PATCH v3 2/2] blame: allow blame --reverse --first-parent when it makes sense","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-22T04:07:39Z","receivedAt":"2015-10-22T04:07:39Z","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 <max@max630.net>\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..eb348f0 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\tdie(\"--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_full | sed -e \"sX .*XX\" >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":"272217","messageId":"1445837217-4252-1-git-send-email-max@max630.net","threadId":"40614","inReplyTo":"1445486859-21906-1-git-send-email-max@max630.net","subject":"[PATCH v4 0/4] blame: allow blame --reverse --first-parent when it makes sense","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-26T05:26:53Z","receivedAt":"2015-10-26T05:26:53Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Change aproach - instead of assuming children content build them explicitly,\nalso accurately verifying that the specified range is along the first-parent chain\n\nMax Kirillov (3):\n  Add test to describe expectation of blame --reverse with branched\n    history\n  blame: extract find_single_final\n  blame: allow blame --reverse --first-parent when it makes sense\n\n builtin/blame.c          | 61 +++++++++++++++++++++++++++++++++++++-----------\n t/t8009-blame-reverse.sh | 34 +++++++++++++++++++++++++++\n 2 files changed, 81 insertions(+), 14 deletions(-)\n create mode 100755 t/t8009-blame-reverse.sh\n\n-- \n2.3.4.2801.g3d0809b\n"},{"id":"272221","messageId":"1445837217-4252-2-git-send-email-max@max630.net","threadId":"40614","inReplyTo":"1445837217-4252-1-git-send-email-max@max630.net","subject":"[PATCH v4 0/4] blame: allow blame --reverse --first-parent when it makes sense","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-26T05:26:54Z","receivedAt":"2015-10-26T05:26:54Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"* Change aproach - instead of assuming children content build them explicitly,\n  also accurately verifying that the specified range is along the first-parent chain\n* Fix error message\n\nMax Kirillov (4):\n  Add test to describe expectation of blame --reverse with branched\n    history\n  blame: extract find_single_final\n  blame: allow blame --reverse --first-parent when it makes sense\n  blame: fix option name in error message\n\n builtin/blame.c          | 63 ++++++++++++++++++++++++++++++++++++------------\n t/t8009-blame-reverse.sh | 34 ++++++++++++++++++++++++++\n 2 files changed, 82 insertions(+), 15 deletions(-)\n create mode 100755 t/t8009-blame-reverse.sh\n\n-- \n2.3.4.2801.g3d0809b\n"},{"id":"272220","messageId":"1445837217-4252-3-git-send-email-max@max630.net","threadId":"40614","inReplyTo":"1445837217-4252-1-git-send-email-max@max630.net","subject":"[PATCH v4 1/3] Add test to describe expectation of blame --reverse with branched history","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-26T05:26:55Z","receivedAt":"2015-10-26T05:26:55Z","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 <max@max630.net>\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_full | sed -e \"sX .*XX\" >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_full | sed -e \"sX .*XX\" >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":"272218","messageId":"1445837217-4252-4-git-send-email-max@max630.net","threadId":"40614","inReplyTo":"1445837217-4252-1-git-send-email-max@max630.net","subject":"[PATCH v4 2/3] blame: extract find_single_final","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-26T05:26:56Z","receivedAt":"2015-10-26T05:26:56Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Signed-off-by: Max Kirillov <max@max630.net>\n---\n builtin/blame.c | 29 +++++++++++++++++------------\n 1 file changed, 17 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 295ce92..38f6267 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-- \n2.3.4.2801.g3d0809b\n"},{"id":"272219","messageId":"1445837217-4252-5-git-send-email-max@max630.net","threadId":"40614","inReplyTo":"1445837217-4252-1-git-send-email-max@max630.net","subject":"[PATCH v4 3/3] blame: allow blame --reverse --first-parent when it makes sense","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-26T05:26:57Z","receivedAt":"2015-10-26T05:26:57Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Allow combining --reverse and --first-parent if initial commit of\nspecified range is at the first-parent chain starting from the final\ncommit. Disable the prepare_revision_walk()'s builtin children\ncollection, instead picking only the ones which are along the first\nparent chain.\n\nSigned-off-by: Max Kirillov <max@max630.net>\n---\n builtin/blame.c          | 32 ++++++++++++++++++++++++++++++--\n t/t8009-blame-reverse.sh |  2 +-\n 2 files changed, 31 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 38f6267..98b1810 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2507,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 *final_commit = NULL;\n \n \tstatic struct string_list range_list;\n \tstatic int output_option = 0, opt = 0;\n@@ -2697,11 +2698,11 @@ 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+\t\tif (revs.first_parent_only)\n+\t\t\trevs.children.name = NULL;\n \t}\n \n \tif (!sb.final) {\n@@ -2718,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\tfinal_commit = (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@@ -2726,6 +2735,25 @@ 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 *c = final_commit;\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 \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..4413815 100755\n--- a/t/t8009-blame-reverse.sh\n+++ b/t/t8009-blame-reverse.sh\n@@ -24,7 +24,7 @@ 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_full | sed -e \"sX .*XX\" >actual &&\n \tgit rev-parse A1 >expect &&\n-- \n2.3.4.2801.g3d0809b\n"},{"id":"272223","messageId":"20151026053119.GB27615@wheezy.local","threadId":"40614","inReplyTo":"1445837217-4252-2-git-send-email-max@max630.net","subject":"Re: [PATCH v4 0/4] blame: allow blame --reverse --first-parent when it makes sense","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-26T05:31:19Z","receivedAt":"2015-10-26T05:31:19Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Mon, Oct 26, 2015 at 07:26:54AM +0200, Max Kirillov wrote:\n> * Change aproach - instead of assuming children content build them explicitly,\n>   also accurately verifying that the specified range is along the first-parent chain\n> * Fix error message\n\nSent by mistake, ignore this, please\n"},{"id":"272224","messageId":"20151026054115.GC27615@wheezy.local","threadId":"40614","inReplyTo":"1445837217-4252-3-git-send-email-max@max630.net","subject":"Re: [PATCH v4 1/3] Add test to describe expectation of blame --reverse with branched history","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-26T05:41:15Z","receivedAt":"2015-10-26T05:41:15Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Sorry, forgot to fix the tests. Will send another batch some later.\n"},{"id":"272226","messageId":"xmqqeggijga3.fsf@gitster.mtv.corp.google.com","threadId":"40614","inReplyTo":"1445837217-4252-3-git-send-email-max@max630.net","subject":"Re: [PATCH v4 1/3] Add test to describe expectation of blame --reverse with branched history","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-26T06:27:32Z","receivedAt":"2015-10-26T06:27:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov <max@max630.net> 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 <max@max630.net>\n\nI _think_ I know why it would be useful to allow \"--first-parent\" to\nthe command; it is useful the same way why \"git log --first-parent\n$path\" would be a good way to get an overview.\n\nBut I am puzzled by your complaints (I'd characterise the statement\nas such, given your second paragraph calls the combination a\n\"workaround\") in the first paragraph.  I honestly do not understand\nwhere it comes from at all.\n\nThe reverse blame begins from an old state and shows the most recent\nchild in the history that each line survived to, and it does not\nshow what commit removed the line from the original state.  And that\ndoes not have anything to do with the presence of any merges or\nforks in the history.  The command will always report \"not the\ncommit that edited the line.\"  There is nothing special about \"If\nthe history contains merges\".\n\nIf you have this history, for example:\n\n    D---E---F\n   /         \\*\n  O           X---Y\n   \\         /\n    A---B---C\n\nwhere O had the original file, which was not touched by any commits\non the branch on the upper side, and commit B rewrote all lines of\nthe file, running blame in reverse may show A as the last point\nwhere all lines survived up to, if the \"reversed\" history happened\nto consider A as the earlier \"parent\" (in reality it is a child but\nblame is about assigning blame for each line from child to parents\nso in the reversed history, real children becomes parents).  Or it\nmay show F as the last point where all lines survived up to, if D\nwas picked as the earlier \"parent\".  Because there is no inherent\nordering between A and D, both of which are children of O, your\nresult is not necessarily \"head of the last merged branch\".\n\nBut I do not see how \"first-parent\" would be a workaround for that.\nThe option would be useful to force the assignment of blame (in\nreverse) along the first-parent chain O---D---E---F---X---Y so that\nyou can get a bird's-eye view of the history, i.e. squashing all\nthat happened in A---B---C as if that happened at X.\n\nThe explanation of the first paragraph needs to be rewritten to make\nit understandable, but I am not sure what relevance it has with this\nchange.\n\nThanks.\n"},{"id":"272289","messageId":"20151027044049.GA18120@wheezy.local","threadId":"40614","inReplyTo":"xmqqeggijga3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 1/3] Add test to describe expectation of blame --reverse with branched history","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-27T04:40:50Z","receivedAt":"2015-10-27T04:40:50Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Sun, Oct 25, 2015 at 11:27:32PM -0700, Junio C Hamano wrote:\n> Max Kirillov <max@max630.net> 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 <max@max630.net>\n>\n> I _think_ I know why it would be useful to allow \"--first-parent\" to\n> the command; it is useful the same way why \"git log --first-parent\n> $path\" would be a good way to get an overview.\n> \n> But I am puzzled by your complaints (I'd characterise the statement\n> as such, given your second paragraph calls the combination a\n> \"workaround\") in the first paragraph.  I honestly do not understand\n> where it comes from at all.\n> \n> The reverse blame begins from an old state and shows the most recent\n> child in the history that each line survived to, and it does not\n> show what commit removed the line from the original state.  And that\n> does not have anything to do with the presence of any merges or\n> forks in the history.  The command will always report \"not the\n> commit that edited the line.\"  There is nothing special about \"If\n> the history contains merges\".\n> \n> If you have this history, for example:\n> \n>     D---E---F\n>    /         \\*\n>   O           X---Y\n>    \\         /\n>     A---B---C\n> \n> where O had the original file, which was not touched by any commits\n> on the branch on the upper side, and commit B rewrote all lines of\n> the file, running blame in reverse may show A as the last point\n> where all lines survived up to, if the \"reversed\" history happened\n> to consider A as the earlier \"parent\" (in reality it is a child but\n> blame is about assigning blame for each line from child to parents\n> so in the reversed history, real children becomes parents).  Or it\n> may show F as the last point where all lines survived up to, if D\n> was picked as the earlier \"parent\".  Because there is no inherent\n> ordering between A and D, both of which are children of O, your\n> result is not necessarily \"head of the last merged branch\".\n> \n> But I do not see how \"first-parent\" would be a workaround for that.\n> The option would be useful to force the assignment of blame (in\n> reverse) along the first-parent chain O---D---E---F---X---Y so that\n> you can get a bird's-eye view of the history, i.e. squashing all\n> that happened in A---B---C as if that happened at X.\n> \n> The explanation of the first paragraph needs to be rewritten to make\n> it understandable, but I am not sure what relevance it has with this\n> change.\n\nI understand how the blame works and why does it produce the\nresult which it used to produce. In one of my letter I\ncalled it \"technically correct, but absolutely useless\", and\nlet me explain why I think so.\n\nIn a big project which uses the nowadays conventional topic\nbranches aka pull-requests aka however it's named workflow,\nthe history is a straigh first-parent chain with\nshort-living branches, which are forked from it, exists for\nseveral days, then merged back and closed. When there are\nmany people working on a project, there can be tens of\nmerges during day, and average pull-request exists for\nseveral days. So the history looks rather like (the\ninteresting line is removed in B1, line removal is\npractically more interesting case because edits can be found\nwith normal forward blame):\n\n\n a0--a1-----*a2-*a3-a4...-*a100\n |\\         /   /         /\n | b0-B1..bN   /         /\n |\\           /         /\n | c0..   ..cN         /\n \\                    /\n  z0..            ..zN\n\n\n...where many of the c-z branches started before a1 and\ncontain the older version of line. And, what I usually need\nis the change b0->B1, because I expect to find there the\nperson who did it and explanation why that was done.\n\nNow the git blame --reverse a0..a100 may return me zN, and in\npractice it often does return some quite late commit wN\nwhich was merged to some a90. Then, I continue search with\nrange a0..a89, and so on. So, to find the commit B1 I might\nhave to perform many blames. \n\n(I realize that this behavior is correct, and it's even not\nobvious how to formally specify the b0 commit as something\ndifferent than zM commit, so we could discuss the\nimplementation of its search. But it does not make me want\nbeing able to find it less)\n\nIn contrast, git blame --reverse --first-parent gives me a1,\nand then I need only one more step:\ngit blame --reverse --first-parent a0..bN (--first-parent\nfor case there are synchronizing merges from master). And,\nmoreover, in the commit message of a2 I can often find the\ninformation which I expect to find in B1, because it\nsummarizes what was done in the merged branch and often\ncontains a link to other resources, like number of ticket.\n\nSo \"blame --reverse --first-parent\" it not like \"log\n--first-parent\", which just decreases number of information\nbut still pproduces a list which I should look through. It\nis really a difference between getting answer and not\ngetting it.\n\n-- \nMax\n"},{"id":"272318","messageId":"xmqqk2q8chyb.fsf@gitster.mtv.corp.google.com","threadId":"40614","inReplyTo":"20151027044049.GA18120@wheezy.local","subject":"Re: [PATCH v4 1/3] Add test to describe expectation of blame --reverse with branched history","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-27T17:57:48Z","receivedAt":"2015-10-27T17:57:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov <max@max630.net> writes:\n\n>> The explanation of the first paragraph needs to be rewritten to make\n>> it understandable, but I am not sure what relevance it has with this\n>> change.\n> ...\n> So the history looks rather like (the interesting line is removed\n> in B1, line removal is practically more interesting case because\n> edits can be found with normal forward blame):\n>\n>\n>  a0--a1-----*a2-*a3-a4...-*a100\n>  |\\         /   /         /\n>  | b0-B1..bN   /         /\n>  |\\           /         /\n>  | c0..   ..cN         /\n>  \\                    /\n>   z0..            ..zN\n>\n> ...where many of the c-z branches started before a1 and\n> contain the older version of line. And, what I usually need\n> is the change b0->B1, because I expect to find there the\n> person who did it and explanation why that was done.\n>\n> Now the git blame --reverse a0..a100 may return me zN, and in\n> practice it often does return some quite late commit wN\n> which was merged to some a90.\n\nThat covers a lot of what I meant by \"rewritten to make it\nunderstandable\".  Instead of picking any of the random cN thru zN,\nyou are forcing to find a2, because at the initial phase of the\nsearch, you are interested in granularity at the topic level.  And\nthe --first-parent combined with --reverse is exactly that, and it\nis a good thing to support it.\n"}]}