{"thread":{"id":"40325","subject":"[Feature Request] git blame showing only revisions from git rev-list --first-parent","startedAt":"2015-09-11T10:47:30Z","lastAt":"2015-10-18T17:41:17Z","messageCount":16,"participants":["Stephen Connolly","Jeff King","Junio C Hamano","Philip Oakley","Max Kirillov"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"269758","messageId":"CA+nPnMx1tkwTRckUjhg6LD055n-jzYDTKsH2sz-0PXPfJiy_tA@mail.gmail.com","threadId":"40325","inReplyTo":null,"subject":"[Feature Request] git blame showing only revisions from git rev-list --first-parent","fromName":"Stephen Connolly","fromEmail":"stephen.alan.connolly@gmail.com","sentAt":"2015-09-11T10:47:30Z","receivedAt":"2015-09-11T10:47:30Z","isPatch":false,"sender":{"key":"stephen.alan.connolly@gmail.com","avatar":"https://gravatar.com/avatar/3c6a583dd3ff57a68201c6400605d778b3c7ef80dfa026d6701c2ba9a5365eb1?d=mp&s=160"},"body":"Background\n===\n\nMy colleagues and I disagree on how exactly to handle feature\ndevelopment in branches.\nThere are approximately two camps:\n\nCamp 1: All feature branches should be squashed after code review to a\nsingle commit before merging.\n\nCamp 2: Leave the history of development alone, being able to trace\nwhy the feature evolved that way is useful information, never squash\ncommits after code review.\n\nBeing somebody who leans towards the second camp I have been trying to\nunderstand the first camps objections...\n\nThose objections seem to boil down to tracing when the feature was introduced.\n\nI have been able to get past some of the objections by pointing out\nthe `--first-parent` option available to `git log` and `gitk`. With\nthat command line option they are presented with a single history of\nthe branch and all merge commits are shown *as if squashed* (or at\nleast that is what it looks like from my testing)\n\nThe only remaining objection is around `git blame`...\n\nYou see a problem on one line in the file, you run `git blame\nfile.txt` and it says was when it changed...\n\nNow is not on the main line of the branch but actually is a\ncommit that was merged from one branch to another before finally being\nmerged into the current branch in \n\nThat is a complex nest of commits to detangle, especially when we have\nhad some refactoring branches with multiple experiments that drag on\nover the course of a couple of weeks with many commits... all they\nwant to know is .\n\nYes I could (and I do try to) teach them how to find the commit where\nthat SHA was merged into the current branch, but really they just want\nto be able to see the current branches SHAs only.\n\nDoing some reading in the git blame sources I discovered that git\nblame takes the rev-list command line options... so I thought \"hey\n--first-parent should work\"\n\nExcept it doesn't\n\nI suspect that this is because of how blame uses rev-list\n\nRequest\n===\n\nA command line option to `git blame HEAD -- path` that instructs that\nthe revisions of blame be the revisions where the change was applied\nto the current branch not the revision where the change first\noriginated (i.e. limit commits to `git rev-list --first-parent HEAD`)\n\nI can get what I want with the following:\n\ngit rev-list --first-parent HEAD | awk '{print p \" \" $0}{p=$0}' >\ntmpfile && git blame -b -S tmpfile HEAD -- path && rm tmpfile\n\nBut that is a rather ugly command. Could we have something built in to\ngit blame to make this much easier for users?\n\nThanks\n"},{"id":"269759","messageId":"20150911140133.GA14311@sigill.intra.peff.net","threadId":"40325","inReplyTo":"CA+nPnMx1tkwTRckUjhg6LD055n-jzYDTKsH2sz-0PXPfJiy_tA@mail.gmail.com","subject":"Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-11T14:01:33Z","receivedAt":"2015-09-11T14:01:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 11, 2015 at 11:47:30AM +0100, Stephen Connolly wrote:\n\n> A command line option to `git blame HEAD -- path` that instructs that\n> the revisions of blame be the revisions where the change was applied\n> to the current branch not the revision where the change first\n> originated (i.e. limit commits to `git rev-list --first-parent HEAD`)\n> \n> I can get what I want with the following:\n> \n> git rev-list --first-parent HEAD | awk '{print p \" \" $0}{p=$0}' >\n> tmpfile && git blame -b -S tmpfile HEAD -- path && rm tmpfile\n> \n> But that is a rather ugly command. Could we have something built in to\n> git blame to make this much easier for users?\n\nI agree this would be a useful feature. Though blame takes rev-list\noptions, it doesn't use the stock rev-list traversal internally, so it\nhas to handle first-parent itself.\n\nI'm not too familiar with the code, but this _seems_ to work for me:\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 21321be..2e03d47 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1375,6 +1375,10 @@ static struct commit_list *first_scapegoat(struct rev_info *revs, struct commit\n static int num_scapegoats(struct rev_info *revs, struct commit *commit)\n {\n \tstruct commit_list *l = first_scapegoat(revs, commit);\n+\tif (!l)\n+\t\treturn 0;\n+\tif (revs->first_parent_only)\n+\t\treturn 1;\n \treturn commit_list_count(l);\n }\n \n\nI suspect it doesn't work at all with `--reverse`. I also have the\nnagging feeling that this could be handled inside revision.c with\nparent-rewriting, but I don't really know.\n\nBut \"git blame --first-parent \" seems to behave sanely in git.git\nwith this.\n\n-Peff\n"},{"id":"269772","messageId":"CA+nPnMxbVnMzj8d2K0U56AV3R4m9O82U-LWdqcAsH=B21=ZUDg@mail.gmail.com","threadId":"40325","inReplyTo":"20150911140133.GA14311@sigill.intra.peff.net","subject":"Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent","fromName":"Stephen Connolly","fromEmail":"stephen.alan.connolly@gmail.com","sentAt":"2015-09-11T15:31:18Z","receivedAt":"2015-09-11T15:31:18Z","isPatch":false,"sender":{"key":"stephen.alan.connolly@gmail.com","avatar":"https://gravatar.com/avatar/3c6a583dd3ff57a68201c6400605d778b3c7ef80dfa026d6701c2ba9a5365eb1?d=mp&s=160"},"body":"On 11 September 2015 at 15:01, Jeff King wrote:\n> On Fri, Sep 11, 2015 at 11:47:30AM +0100, Stephen Connolly wrote:\n>\n>> A command line option to `git blame HEAD -- path` that instructs that\n>> the revisions of blame be the revisions where the change was applied\n>> to the current branch not the revision where the change first\n>> originated (i.e. limit commits to `git rev-list --first-parent HEAD`)\n>>\n>> I can get what I want with the following:\n>>\n>> git rev-list --first-parent HEAD | awk '{print p \" \" $0}{p=$0}' >\n>> tmpfile && git blame -b -S tmpfile HEAD -- path && rm tmpfile\n>>\n>> But that is a rather ugly command. Could we have something built in to\n>> git blame to make this much easier for users?\n>\n> I agree this would be a useful feature. Though blame takes rev-list\n> options, it doesn't use the stock rev-list traversal internally, so it\n> has to handle first-parent itself.\n>\n> I'm not too familiar with the code, but this _seems_ to work for me:\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 21321be..2e03d47 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -1375,6 +1375,10 @@ static struct commit_list *first_scapegoat(struct rev_info *revs, struct commit\n> static int num_scapegoats(struct rev_info *revs, struct commit *commit)\n> {\n> struct commit_list *l = first_scapegoat(revs, commit);\n> + if (!l)\n> + return 0;\n> + if (revs->first_parent_only)\n> + return 1;\n> return commit_list_count(l);\n> }\n>\n>\n> I suspect it doesn't work at all with `--reverse`. I also have the\n> nagging feeling that this could be handled inside revision.c with\n> parent-rewriting, but I don't really know.\n>\n> But \"git blame --first-parent \" seems to behave sanely in git.git\n> with this.\n>\n> -Peff\n\nIt would help if somebody who knew the code could comment, but I am\nimpressed with the progress ;-)\n"},{"id":"269780","messageId":"xmqqa8ss29tq.fsf@gitster.mtv.corp.google.com","threadId":"40325","inReplyTo":"20150911140133.GA14311@sigill.intra.peff.net","subject":"Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-11T16:35:13Z","receivedAt":"2015-09-11T16:35:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm not too familiar with the code, but this _seems_ to work for me:\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 21321be..2e03d47 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -1375,6 +1375,10 @@ static struct commit_list *first_scapegoat(struct rev_info *revs, struct commit\n> static int num_scapegoats(struct rev_info *revs, struct commit *commit)\n> {\n> \tstruct commit_list *l = first_scapegoat(revs, commit);\n> +\tif (!l)\n> +\t\treturn 0;\n> +\tif (revs->first_parent_only)\n> +\t\treturn 1;\n> \treturn commit_list_count(l);\n> }\n>\n> I suspect it doesn't work at all with `--reverse`. I also have the\n> nagging feeling that this could be handled inside revision.c with\n> parent-rewriting, but I don't really know.\n\nI am not sure how well --children, which is what we use from\nrevision.c, works with --first-parent flag passed to the revision\nwalking machinery. If it already does the right thing, then the\nrevs.children decoration that collects all children of any given\ncommit should automatically give its \"sole child\" (in the world\nlimited to the first-parent chain) from first_scapegoat().\n\nAnd if it does not, perhaps that is what we would want to fix,\ni.e. making sure rev-list --first-parent --children does what\npeople would expect.\n\n> But \"git blame --first-parent \" seems to behave sanely in git.git\n> with this.\n\nIt is intresting to see that the above is the only thing necessary,\nas a naive way to try all parents would be to do:\n\n\tfor (sg = first_scapegoat(...); sg; sg = sg->next)\n\t\tassign blame to sg;\n\ttake the remainder ourselves;\n\nin which case, a better place to patch would be first_scapegoat(),\nnot this function, so that we will always see zero or one element\nparent list returned from the function.\n\nBut in reality, the code instead does this:\n\n\tnum_sg = num_scapegoats(...);\n\tfor (i = 0, sg = first_scapegoat(...);\n i < num_sg && sg;\n i++, sg = sg->next)\n\t\tassign blame to sg;\n\ttake the remainder ourselves;\n\nso you do not have to touch first_scapegoat() at all.\n\nI do not offhand know if this was a sign of foresight in the\noriginal, or just an overly redundant programming ;-).\n"},{"id":"269791","messageId":"xmqqsi6kzsgc.fsf@gitster.mtv.corp.google.com","threadId":"40325","inReplyTo":"xmqqa8ss29tq.fsf@gitster.mtv.corp.google.com","subject":"Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-11T19:06:27Z","receivedAt":"2015-09-11T19:06:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> I'm not too familiar with the code, but this _seems_ to work for me:\n>>\n>> diff --git a/builtin/blame.c b/builtin/blame.c\n>> index 21321be..2e03d47 100644\n>> --- a/builtin/blame.c\n>> +++ b/builtin/blame.c\n>> @@ -1375,6 +1375,10 @@ static struct commit_list *first_scapegoat(struct rev_info *revs, struct commit\n>> static int num_scapegoats(struct rev_info *revs, struct commit *commit)\n>> {\n>> \tstruct commit_list *l = first_scapegoat(revs, commit);\n>> +\tif (!l)\n>> +\t\treturn 0;\n>> +\tif (revs->first_parent_only)\n>> +\t\treturn 1;\n>> \treturn commit_list_count(l);\n>> }\n>>\n>> I suspect it doesn't work at all with `--reverse`. I also have the\n>> nagging feeling that this could be handled inside revision.c with\n>> parent-rewriting, but I don't really know.\n>\n> I am not sure how well --children, which is what we use from\n> revision.c, works with --first-parent flag passed to the revision\n> walking machinery. If it already does the right thing, then the\n> revs.children decoration that collects all children of any given\n> commit should automatically give its \"sole child\" (in the world\n> limited to the first-parent chain) from first_scapegoat().\n>\n> And if it does not, perhaps that is what we would want to fix,\n> i.e. making sure rev-list --first-parent --children does what\n> people would expect.\n> \n>> But \"git blame --first-parent \" seems to behave sanely in git.git\n>> with this.\n>\n> It is intresting to see that the above is the only thing necessary,\n> as a naive way to try all parents would be to do:\n>\n> \tfor (sg = first_scapegoat(...); sg; sg = sg->next)\n> \t\tassign blame to sg;\n> \ttake the remainder ourselves;\n>\n> in which case, a better place to patch would be first_scapegoat(),\n> not this function, so that we will always see zero or one element\n> parent list returned from the function.\n>\n> But in reality, the code instead does this:\n>\n> \tnum_sg = num_scapegoats(...);\n> \tfor (i = 0, sg = first_scapegoat(...);\n> i < num_sg && sg;\n> i++, sg = sg->next)\n> \t\tassign blame to sg;\n> \ttake the remainder ourselves;\n>\n> so you do not have to touch first_scapegoat() at all.\n>\n> I do not offhand know if this was a sign of foresight in the\n> original, or just an overly redundant programming ;-).\n\nSo here is an outline of what I had in mind when I was writing the\nabove. Instead of trusting that num_scapegoats() is always used to\nlimit the enumeration of commit->parents, we discard the second and\nsubsequent parents from the commit objects, and also make sure the\nlater parents do not participate in the revs.children annotation, so\nthat the world \"blame\" sees truly becomes a single strand of pearls.\n\nAs usual, not even compile tested, but it feels correct ;-)\n\n\n\n builtin/blame.c | 9 ++++++++-\n revision.c | 4 +++-\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 4db01c1..bc87d9d 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1366,8 +1366,15 @@ static void pass_whole_blame(struct scoreboard *sb,\n */\n static struct commit_list *first_scapegoat(struct rev_info *revs, struct commit *commit)\n {\n-\tif (!reverse)\n+\tif (!reverse) {\n+\t\tif (revs->first_parent_only &&\n+\t\t commit->parents &&\n+\t\t commit->parents->next) {\n+\t\t\tfree_commit_list(commit->parents->next);\n+\t\t\tcommit->parents->next = NULL;\n+\t\t}\n \t\treturn commit->parents;\n+\t}\n \treturn lookup_decoration(&revs->children, &commit->object);\n }\n \ndiff --git a/revision.c b/revision.c\nindex af2a18e..a020a42 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2765,7 +2765,9 @@ static void set_children(struct rev_info *revs)\n \t\tstruct commit *commit = l->item;\n \t\tstruct commit_list *p;\n \n-\t\tfor (p = commit->parents; p; p = p->next)\n+\t\tfor (p = commit->parents;\n+\t\t p && !revs->first_parent_only;\n+\t\t p = p->next)\n \t\t\tadd_child(revs, p->item, commit);\n \t}\n }\n"},{"id":"269813","messageId":"20150912033054.GA30431@sigill.intra.peff.net","threadId":"40325","inReplyTo":"xmqqsi6kzsgc.fsf@gitster.mtv.corp.google.com","subject":"Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-12T03:30:55Z","receivedAt":"2015-09-12T03:30:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 11, 2015 at 12:06:27PM -0700, Junio C Hamano wrote:\n\n> So here is an outline of what I had in mind when I was writing the\n> above. Instead of trusting that num_scapegoats() is always used to\n> limit the enumeration of commit->parents, we discard the second and\n> subsequent parents from the commit objects, and also make sure the\n> later parents do not participate in the revs.children annotation, so\n> that the world \"blame\" sees truly becomes a single strand of pearls.\n\nYeah, I think what is happening in this first hunk:\n\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 4db01c1..bc87d9d 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -1366,8 +1366,15 @@ static void pass_whole_blame(struct scoreboard *sb,\n> */\n> static struct commit_list *first_scapegoat(struct rev_info *revs, struct commit *commit)\n> {\n> -\tif (!reverse)\n> +\tif (!reverse) {\n> +\t\tif (revs->first_parent_only &&\n> +\t\t commit->parents &&\n> +\t\t commit->parents->next) {\n> +\t\t\tfree_commit_list(commit->parents->next);\n> +\t\t\tcommit->parents->next = NULL;\n> +\t\t}\n> \t\treturn commit->parents;\n> +\t}\n> \treturn lookup_decoration(&revs->children, &commit->object);\n> }\n\nis doing the right thing. It did feel a little weird to me to be munging\nthe global commit objects themselves, but I guess it is fairly normal\nfor git code.\n\nI wondered if you could then simplify away the counters used in the\nfor-loops. I.e., to end up with:\n\n for (sg = first_scapegoat(revs, commit); sg; sg = sg->next)\n\nin the callers. But no. One of the reasons for the counters is that we\nneed to know we are on the n'th parent, so we can compare it to parent\nn-1, n-2, etc, for sameness.\n\n> diff --git a/revision.c b/revision.c\n> index af2a18e..a020a42 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2765,7 +2765,9 @@ static void set_children(struct rev_info *revs)\n> \t\tstruct commit *commit = l->item;\n> \t\tstruct commit_list *p;\n> \n> -\t\tfor (p = commit->parents; p; p = p->next)\n> +\t\tfor (p = commit->parents;\n> +\t\t p && !revs->first_parent_only;\n> +\t\t p = p->next)\n> \t\t\tadd_child(revs, p->item, commit);\n> \t}\n> }\n\nYeah, this makes sense to me. Adding all children to the decoration list\nand then later choosing only the first child (as my earlier suggestion\ndid) is not right. You can tell immediately that it is nonsense because\nthe child you would get depends on the order we visited the commits when\nbuilding up the decoration. And that does not necessarily have any real\nmeaning.\n\n-Peff\n"},{"id":"269816","messageId":"xmqqy4gcxcp4.fsf@gitster.mtv.corp.google.com","threadId":"40325","inReplyTo":"20150912033054.GA30431@sigill.intra.peff.net","subject":"Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-12T08:29:43Z","receivedAt":"2015-09-12T08:29:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, I think what is happening in this first hunk:\n> ...\n> is doing the right thing. It did feel a little weird to me to be munging\n> the global commit objects themselves, but I guess it is fairly normal\n> for git code.\n\nYeah, the above may be brilliant, but ...\n\n>> diff --git a/revision.c b/revision.c\n>> index af2a18e..a020a42 100644\n>> --- a/revision.c\n>> +++ b/revision.c\n>> @@ -2765,7 +2765,9 @@ static void set_children(struct rev_info *revs)\n>> \t\tstruct commit *commit = l->item;\n>> \t\tstruct commit_list *p;\n>> \n>> -\t\tfor (p = commit->parents; p; p = p->next)\n>> +\t\tfor (p = commit->parents;\n>> +\t\t p && !revs->first_parent_only;\n>> +\t\t p = p->next)\n>> \t\t\tadd_child(revs, p->item, commit);\n>> \t}\n>> }\n\n... this is a total crap and shows that I am doubly an idiot.\n\nThe loop is a no-op when first-parent-only (the intent is to call\nadd-child for just the first parent), so the code is stupid and\nwrong in the first place, but more importantly, the logic is utterly\nconfused.\n\nThe thing is, traversing first-parent chain in reverse fundamentally\nis undefined. You can fork multiple topics at the tip of 'master'\nand each of the topics may be single strand of pearls, but which one\nof the topics is the first-child-chain---there is no answer to that\nquestion.\n\nThe add_child() call above is exactly that. It is saying \"record\nthe fact that commit is child of p->item for these p on that parents\nlist\". We may limit the call to record that commit C is a child of\nits first parent P (and nobody else), but that does not make us\nrecord that that P has only one child (which is C). This loop will\nbe entered with another commit C' whose first parent is the same P\nand when we return from the set_children() function, we will find\nthat P has more than one children (C and C', at least); the only\nthing we ensured is that all of these recorded children have P as\ntheir first parent. It does not tell us which one of C's we would\nwant to pick when digging in reverse from P.\n"},{"id":"269837","messageId":"6EB617480AFE4F7898553BCE6A91D946@PhilipOakley","threadId":"40325","inReplyTo":"xmqqy4gcxcp4.fsf@gitster.mtv.corp.google.com","subject":"Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2015-09-12T22:09:18Z","receivedAt":"2015-09-12T22:09:18Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Junio C Hamano\" \nSent: Saturday, September 12, 2015 9:29 AM\n> Jeff King writes:\n>\n>> Yeah, I think what is happening in this first hunk:\n>> ...\n>> is doing the right thing. It did feel a little weird to me to be \n>> munging\n>> the global commit objects themselves, but I guess it is fairly normal\n>> for git code.\n>\n> Yeah, the above may be brilliant, but ...\n>\n>>> diff --git a/revision.c b/revision.c\n>>> index af2a18e..a020a42 100644\n>>> --- a/revision.c\n>>> +++ b/revision.c\n>>> @@ -2765,7 +2765,9 @@ static void set_children(struct rev_info \n>>> *revs)\n>>> struct commit *commit = l->item;\n>>> struct commit_list *p;\n>>>\n>>> - for (p = commit->parents; p; p = p->next)\n>>> + for (p = commit->parents;\n>>> + p && !revs->first_parent_only;\n>>> + p = p->next)\n>>> add_child(revs, p->item, commit);\n>>> }\n>>> }\n>\n> ... this is a total crap and shows that I am doubly an idiot.\n>\n> The loop is a no-op when first-parent-only (the intent is to call\n> add-child for just the first parent), so the code is stupid and\n> wrong in the first place, but more importantly, the logic is utterly\n> confused.\n>\n> The thing is, traversing first-parent chain in reverse fundamentally\n> is undefined. You can fork multiple topics at the tip of 'master'\n> and each of the topics may be single strand of pearls, but which one\n> of the topics is the first-child-chain---there is no answer to that\n> question.\n\nI don't understand why this would be so. If we just have a base commit \nfrom which to seek a --reversed chain then it would be true.\n\nBut if I understood the man page correctly: --reverse \"This requires a \nrange of revision like START..END where the path to blame exists in \nSTART.\" which when combined with --first-parent, would define a single \nstring of pearls which could then be reversed. Or is it that the code \ndoesn't do it in that order?\n\n>\n> The add_child() call above is exactly that. It is saying \"record\n> the fact that commit is child of p->item for these p on that parents\n> list\". We may limit the call to record that commit C is a child of\n> its first parent P (and nobody else), but that does not make us\n> record that that P has only one child (which is C). This loop will\n> be entered with another commit C' whose first parent is the same P\n> and when we return from the set_children() function, we will find\n> that P has more than one children (C and C', at least); the only\n> thing we ensured is that all of these recorded children have P as\n> their first parent. It does not tell us which one of C's we would\n> want to pick when digging in reverse from P.\n--\nPhilip \n"},{"id":"269870","messageId":"20150913100728.GA26562@sigill.intra.peff.net","threadId":"40325","inReplyTo":"xmqqy4gcxcp4.fsf@gitster.mtv.corp.google.com","subject":"Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-13T10:07:29Z","receivedAt":"2015-09-13T10:07:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 12, 2015 at 01:29:43AM -0700, Junio C Hamano wrote:\n\n> >> -\t\tfor (p = commit->parents; p; p = p->next)\n> >> +\t\tfor (p = commit->parents;\n> >> +\t\t p && !revs->first_parent_only;\n> >> +\t\t p = p->next)\n> >> \t\t\tadd_child(revs, p->item, commit);\n> >> \t}\n> >> }\n> \n> ... this is a total crap and shows that I am doubly an idiot.\n> \n> The loop is a no-op when first-parent-only (the intent is to call\n> add-child for just the first parent), so the code is stupid and\n> wrong in the first place, but more importantly, the logic is utterly\n> confused.\n\nWhoops, yeah. I think you need \"if (revs->first_parent_only) break;\".\n\n> The thing is, traversing first-parent chain in reverse fundamentally\n> is undefined. You can fork multiple topics at the tip of 'master'\n> and each of the topics may be single strand of pearls, but which one\n> of the topics is the first-child-chain---there is no answer to that\n> question.\n\nIn general I think I agree, but in the case of blame, we know that we\nare starting from a single tip (and we know because we are using\nfirst-parent that we remain in a single strand of pearls, because we\nfollow only one parent and there are no cycles).\n\nThat is assuming that we create the set of children by traversing the\nfirst-parent history, though, which I am not at all sure about.\n\n-Peff\n"},{"id":"269903","messageId":"CAPc5daUcwCferagupF+iy3gxxorfMyEMn0oFMRWBNjc0=2r4og@mail.gmail.com","threadId":"40325","inReplyTo":"20150913100728.GA26562@sigill.intra.peff.net","subject":"Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-14T05:19:33Z","receivedAt":"2015-09-14T05:19:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Sun, Sep 13, 2015 at 3:07 AM, Jeff King wrote:\n>> The thing is, traversing first-parent chain in reverse fundamentally\n>> is undefined. You can fork multiple topics at the tip of 'master'\n>> and each of the topics may be single strand of pearls, but which one\n>> of the topics is the first-child-chain---there is no answer to that\n>> question.\n>\n> In general I think I agree, but in the case of blame, we know that we\n> are starting from a single tip (and we know because we are using\n> first-parent that we remain in a single strand of pearls, because we\n> follow only one parent and there are no cycles).\n\nThe thing is that the patch is inside revision.c machinery for --children,\nand \"rev-list --first-parent --children ^A ^B C\" (i.e. range with multiple\nbottoms) may be a perfectly a reasonable request. The requestor does\nnot necessarily know if A or B is an ancestor of C on the first-parent\nchain. I am not sure if we are introducing unintended consequences\non this unrelated request only to change the behaviour of \"blame\".\n\nFor that matter, I am not sure how \"blame A..B\" with first-parent &\nreverse should behave when A is not an ancestor on the first-parent\nchain. Wouldn't we try to find a cut-point on the first-parent chain by\ntraversing first-parent chain from B and painting them as positive,\nwhile traversing _all_ parents from B and painting them as negative,\nuntil the traversal intersect? And wouldn't we discover at least two\nchildren (one positive and one negative) for the cut point we discover\nby that traversal? That cut point would be the (fake) latest state the\nblame traversal starts at, and then we try to use the first (fake) parent\nthat in real life is the first child (which we do not have a good definition\nfor). And at that point a simple panda brain explodes ;-)\n\nWe might end up doing the right thing even in that case, but I haven't\nconvinced myself about that (yet). If the change were limited to \"blame\",\nthe change may be much less problematic.\n"},{"id":"269995","messageId":"20150915100538.GA21831@sigill.intra.peff.net","threadId":"40325","inReplyTo":"CAPc5daUcwCferagupF+iy3gxxorfMyEMn0oFMRWBNjc0=2r4og@mail.gmail.com","subject":"Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-15T10:05:39Z","receivedAt":"2015-09-15T10:05:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 13, 2015 at 10:19:33PM -0700, Junio C Hamano wrote:\n\n> For that matter, I am not sure how \"blame A..B\" with first-parent &\n> reverse should behave when A is not an ancestor on the first-parent\n> chain. Wouldn't we try to find a cut-point on the first-parent chain by\n> traversing first-parent chain from B and painting them as positive,\n> while traversing _all_ parents from B and painting them as negative,\n> until the traversal intersect? And wouldn't we discover at least two\n> children (one positive and one negative) for the cut point we discover\n> by that traversal? That cut point would be the (fake) latest state the\n> blame traversal starts at, and then we try to use the first (fake) parent\n> that in real life is the first child (which we do not have a good definition\n> for). And at that point a simple panda brain explodes ;-)\n> \n> We might end up doing the right thing even in that case, but I haven't\n> convinced myself about that (yet). If the change were limited to \"blame\",\n> the change may be much less problematic.\n\nGood point.\n\nIt seems like nobody is actually that interested in what \"blame\n--first-parent --reverse\" does in the first place, though, and there's\nno reason for its complexity to hold up vanilla --first-parent. So what\ndo you think of:\n\n-- >8 --\nSubject: [PATCH] blame: handle --first-parent\n\nThe revision.c options-parser will parse \"--first-parent\"\nfor us, but the blame code does not actually respect it, as\nwe simply iterate over the whole list returned by\nfirst_scapegoat(). We can fix this by returning a\ntruncated parent list.\n\nNote that we could technically also do so by limiting the\nreturn value of num_scapegoats(), but that is less robust.\nWe would rely on nobody ever looking at the \"next\" pointer\nfrom the returned list.\n\nCombining \"--reverse\" with \"--first-parent\" is more\ncomplicated, and will probably involve cooperation from\nrevision.c. Since the desired semantics are not even clear,\nlet's punt on this for now, but explicitly disallow it to\navoid confusing users (this is not really a regression,\nsince it did something nonsensical before).\n\nSigned-off-by: Jeff King \n---\n builtin/blame.c | 11 ++++++++++-\n t/annotate-tests.sh | 4 ++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 4db01c1..ae4301c 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1366,8 +1366,15 @@ static void pass_whole_blame(struct scoreboard *sb,\n */\n static struct commit_list *first_scapegoat(struct rev_info *revs, struct commit *commit)\n {\n-\tif (!reverse)\n+\tif (!reverse) {\n+\t\tif (revs->first_parent_only &&\n+\t\t commit->parents &&\n+\t\t commit->parents->next) {\n+\t\t\tfree_commit_list(commit->parents->next);\n+\t\t\tcommit->parents->next = NULL;\n+\t\t}\n \t\treturn commit->parents;\n+\t}\n \treturn lookup_decoration(&revs->children, &commit->object);\n }\n \n@@ -2680,6 +2687,8 @@ 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;\ndiff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\nindex f5c0175..b1673b3 100644\n--- a/t/annotate-tests.sh\n+++ b/t/annotate-tests.sh\n@@ -111,6 +111,10 @@ test_expect_success 'blame 2 authors + 2 merged-in authors' '\n \tcheck_count A 2 B 1 B1 2 B2 1\n '\n \n+test_expect_success 'blame --first-parent blames merge for branch1' '\n+\tcheck_count --first-parent A 2 B 1 \"A U Thor\" 2 B2 1\n+'\n+\n test_expect_success 'blame ancestor' '\n \tcheck_count -h master A 2 B 2\n '\n-- \n2.6.0.rc2.408.ga2926b9\n"},{"id":"270086","messageId":"xmqqwpvrtbbh.fsf@gitster.mtv.corp.google.com","threadId":"40325","inReplyTo":"20150915100538.GA21831@sigill.intra.peff.net","subject":"Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-16T01:14:26Z","receivedAt":"2015-09-16T01:14:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It seems like nobody is actually that interested in what \"blame\n> --first-parent --reverse\" does in the first place, though, and there's\n> no reason for its complexity to hold up vanilla --first-parent. So what\n> do you think of:\n\nI like the part that explicitly disables the combination of the two\n;-)\n"},{"id":"270135","messageId":"20150916173704.GA2727@sigill.intra.peff.net","threadId":"40325","inReplyTo":"xmqqwpvrtbbh.fsf@gitster.mtv.corp.google.com","subject":"Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-16T17:37:04Z","receivedAt":"2015-09-16T17:37:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 15, 2015 at 06:14:26PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > It seems like nobody is actually that interested in what \"blame\n> > --first-parent --reverse\" does in the first place, though, and there's\n> > no reason for its complexity to hold up vanilla --first-parent. So what\n> > do you think of:\n> \n> I like the part that explicitly disables the combination of the two\n> ;-)\n\nMeaning you didn't like the other part, or that you'll pick up the patch\nas-is? :)\n\n-Peff\n"},{"id":"270238","messageId":"xmqq1tdxj7v5.fsf@gitster.mtv.corp.google.com","threadId":"40325","inReplyTo":"20150916173704.GA2727@sigill.intra.peff.net","subject":"Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-17T17:03:42Z","receivedAt":"2015-09-17T17:03:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Sep 15, 2015 at 06:14:26PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > It seems like nobody is actually that interested in what \"blame\n>> > --first-parent --reverse\" does in the first place, though, and there's\n>> > no reason for its complexity to hold up vanilla --first-parent. So what\n>> > do you think of:\n>> \n>> I like the part that explicitly disables the combination of the two\n>> ;-)\n>\n> Meaning you didn't like the other part, or that you'll pick up the patch\n> as-is? :)\n>\n> -Peff\n\nI _especially_ like the part that explicitly disables the\ncombination of the two ;-)\n"},{"id":"271871","messageId":"20151018113857.GA5722@wheezy.local","threadId":"40325","inReplyTo":"20150915100538.GA21831@sigill.intra.peff.net","subject":"Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-18T11:38:57Z","receivedAt":"2015-10-18T11:38:57Z","isPatch":false,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Tue, Sep 15, 2015 at 06:05:39AM -0400, Jeff King wrote:\n> It seems like nobody is actually that interested in what \"blame\n> --first-parent --reverse\" does in the first place, though, and there's\n> no reason for its complexity to hold up vanilla --first-parent. So what\n> do you think of:\n...\n> Combining \"--reverse\" with \"--first-parent\" is more\n> complicated, and will probably involve cooperation from\n> revision.c. Since the desired semantics are not even clear,\n> let's punt on this for now, but explicitly disallow it to\n> avoid confusing users (this is not really a regression,\n> since it did something nonsensical before).\n\nHi.\n\nI might be late for this discussion, but I seem to have\na case when blame --reverse --first-parent seems to work.\n\nConsider the folowing history (from left ro right):\n\n +-----------------D1-+\n +--->C1------>C2-+ \\\n / \\ \\\nA0->A1-------->A2---..-->A3-->A4-->A5\n \\ /\n +->B1-->B2-+\n\n, and a line was removed in B2. Then, blame --reverse\nreturns D1 for this line, which is, while technically\ncorrect, absolutely useless to find real place where the\nline was removed. But blame --reverse --first-parent seems\nto return A1, which is much more useful and actually what\nI would expect to return. I tried it recently with\n2.3-something and it seems to work as expected.\n\nWas it the behavior you mentioned as nonsensical or you have\nsome other examples?\n\nSo please may I ask to not kill this completely. As about\nthe issue mentioned by Junio, it could fail loudly if the\nrequested range is not a first-parent chain.\n\n-- \nMax\n"},{"id":"271880","messageId":"xmqqlhb0axc2.fsf@gitster.mtv.corp.google.com","threadId":"40325","inReplyTo":"20151018113857.GA5722@wheezy.local","subject":"Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-18T17:41:17Z","receivedAt":"2015-10-18T17:41:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov writes:\n\n> I might be late for this discussion, but I seem to have\n> a case when blame --reverse --first-parent seems to work.\n\nI think during the discussion we already established that there are\ncases where the mode happens to do the right thing (the most trivial\nis a completely linear history).\n\nI do not strongly object to enabling the mode when it is safe to\nenable (i.e. it can be proven to work and produce a sensible and\nmeaningful result); patches welcome to enable it when it can be\nshown that it is safe; \"to disable it only when it can be shown that\nit is meaningless\" is a different way to state the same thing.\n"}]}