# [PATCH 0/2] blame: allow blame --reverse --first-parent when it makes sense 5 messages from 2015-10-21 to 2015-10-22. Participants: Max Kirillov, Eric Sunshine. Thread: https://gitlist.dev/t/40602 ## Max Kirillov, 2015-10-21 04:08 Subject: [PATCH 0/2] blame: allow blame --reverse --first-parent when it makes sense Message-ID: <1445400482-1977-1-git-send-email-max@max630.net> URL: https://gitlist.dev/e/1445400482-1977-1-git-send-email-max%40max630.net ``` 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, 2015-10-21 04:08 Subject: [PATCH 1/2] Add test to describe expectation of blame --reverse with branched history Message-ID: <1445400482-1977-2-git-send-email-max@max630.net> URL: https://gitlist.dev/e/1445400482-1977-2-git-send-email-max%40max630.net In-Reply-To: <1445400482-1977-1-git-send-email-max@max630.net> ``` 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 --- 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 && + 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 && + git rev-parse A1 >expect && + test_cmp expect actual + ' + +test_done -- 2.3.4.2801.g3d0809b ``` ## Max Kirillov, 2015-10-21 04:08 Subject: [PATCH 2/2] blame: allow blame --reverse --first-parent when it makes sense Message-ID: <1445400482-1977-3-git-send-email-max@max630.net> URL: https://gitlist.dev/e/1445400482-1977-3-git-send-email-max%40max630.net In-Reply-To: <1445400482-1977-1-git-send-email-max@max630.net> ``` 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 --- 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 && 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, 2015-10-21 04:29 Subject: Re: [PATCH 2/2] blame: allow blame --reverse --first-parent when it makes sense Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQ_cYQ0i8PtidiLJjBr2UqwkPBW5Kv20VeN7AsmeWBsPg%40mail.gmail.com In-Reply-To: <1445400482-1977-3-git-send-email-max@max630.net> ``` On Wed, Oct 21, 2015 at 12:08 AM, Max Kirillov 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 > --- > 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 && > 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, 2015-10-22 03:52 Subject: Re: [PATCH 2/2] blame: allow blame --reverse --first-parent when it makes sense Message-ID: <20151022035200.GB5722@wheezy.local> URL: https://gitlist.dev/e/20151022035200.GB5722%40wheezy.local In-Reply-To: ``` On Wed, Oct 21, 2015 at 12:29:12AM -0400, Eric Sunshine wrote: > On Wed, Oct 21, 2015 at 12:08 AM, Max Kirillov 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 ```