[Feature Request] git blame showing only revisions from git rev-list --first-parent

[Feature Request] git blame showing only revisions from git rev-list --first-parent

[Stephen Connolly]

Background
===

My colleagues and I disagree on how exactly to handle feature
development in branches.
There are approximately two camps:

Camp 1: All feature branches should be squashed after code review to a
single commit before merging.

Camp 2: Leave the history of development alone, being able to trace
why the feature evolved that way is useful information, never squash
commits after code review.

Being somebody who leans towards the second camp I have been trying to
understand the first camps objections...

Those objections seem to boil down to tracing when the feature was introduced.

I have been able to get past some of the objections by pointing out
the `--first-parent` option available to `git log` and `gitk`. With
that command line option they are presented with a single history of
the branch and all merge commits are shown *as if squashed* (or at
least that is what it looks like from my testing)

The only remaining objection is around `git blame`...

You see a problem on one line in the file, you run `git blame
file.txt` and it says <rev1> was when it changed...

Now <rev1> is not on the main line of the branch but actually is a
commit that was merged from one branch to another before finally being
merged into the current branch in <rev2>

That is a complex nest of commits to detangle, especially when we have
had some refactoring branches with multiple experiments that drag on
over the course of a couple of weeks with many commits... all they
want to know is <rev2>.

Yes I could (and I do try to) teach them how to find the commit where
that SHA was merged into the current branch, but really they just want
to be able to see the current branches SHAs only.

Doing some reading in the git blame sources I discovered that git
blame takes the rev-list command line options... so I thought "hey
--first-parent should work"

Except it doesn't

I suspect that this is because of how blame uses rev-list

Request
===

A command line option to `git blame HEAD -- path` that instructs that
the revisions of blame be the revisions where the change was applied
to the current branch not the revision where the change first
originated (i.e. limit commits to `git rev-list --first-parent HEAD`)

I can get what I want with the following:

git rev-list --first-parent HEAD | awk '{print p " " $0}{p=$0}' >
tmpfile && git blame -b -S tmpfile HEAD -- path && rm tmpfile

But that is a rather ugly command. Could we have something built in to
git blame to make this much easier for users?

Thanks

Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent

[Jeff King]

On Fri, Sep 11, 2015 at 11:47:30AM +0100, Stephen Connolly wrote:

> A command line option to `git blame HEAD -- path` that instructs that
> the revisions of blame be the revisions where the change was applied
> to the current branch not the revision where the change first
> originated (i.e. limit commits to `git rev-list --first-parent HEAD`)
> 
> I can get what I want with the following:
> 
> git rev-list --first-parent HEAD | awk '{print p " " $0}{p=$0}' >
> tmpfile && git blame -b -S tmpfile HEAD -- path && rm tmpfile
> 
> But that is a rather ugly command. Could we have something built in to
> git blame to make this much easier for users?

I agree this would be a useful feature. Though blame takes rev-list
options, it doesn't use the stock rev-list traversal internally, so it
has to handle first-parent itself.

I'm not too familiar with the code, but this _seems_ to work for me:

diff --git a/builtin/blame.c b/builtin/blame.c
index 21321be..2e03d47 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1375,6 +1375,10 @@ static struct commit_list *first_scapegoat(struct rev_info *revs, struct commit
 static int num_scapegoats(struct rev_info *revs, struct commit *commit)
 {
 	struct commit_list *l = first_scapegoat(revs, commit);
+	if (!l)
+		return 0;
+	if (revs->first_parent_only)
+		return 1;
 	return commit_list_count(l);
 }
 

I suspect it doesn't work at all with `--reverse`. I also have the
nagging feeling that this could be handled inside revision.c with
parent-rewriting, but I don't really know.

But "git blame --first-parent <file>" seems to behave sanely in git.git
with this.

-Peff

Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent

[Stephen Connolly]

On 11 September 2015 at 15:01, Jeff King <peff@peff.net> wrote:
> On Fri, Sep 11, 2015 at 11:47:30AM +0100, Stephen Connolly wrote:
>
>> A command line option to `git blame HEAD -- path` that instructs that
>> the revisions of blame be the revisions where the change was applied
>> to the current branch not the revision where the change first
>> originated (i.e. limit commits to `git rev-list --first-parent HEAD`)
>>
>> I can get what I want with the following:
>>
>> git rev-list --first-parent HEAD | awk '{print p " " $0}{p=$0}' >
>> tmpfile && git blame -b -S tmpfile HEAD -- path && rm tmpfile
>>
>> But that is a rather ugly command. Could we have something built in to
>> git blame to make this much easier for users?
>
> I agree this would be a useful feature. Though blame takes rev-list
> options, it doesn't use the stock rev-list traversal internally, so it
> has to handle first-parent itself.
>
> I'm not too familiar with the code, but this _seems_ to work for me:
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 21321be..2e03d47 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1375,6 +1375,10 @@ static struct commit_list *first_scapegoat(struct rev_info *revs, struct commit
>  static int num_scapegoats(struct rev_info *revs, struct commit *commit)
>  {
>         struct commit_list *l = first_scapegoat(revs, commit);
> +       if (!l)
> +               return 0;
> +       if (revs->first_parent_only)
> +               return 1;
>         return commit_list_count(l);
>  }
>
>
> I suspect it doesn't work at all with `--reverse`. I also have the
> nagging feeling that this could be handled inside revision.c with
> parent-rewriting, but I don't really know.
>
> But "git blame --first-parent <file>" seems to behave sanely in git.git
> with this.
>
> -Peff

It would help if somebody who knew the code could comment, but I am
impressed with the progress ;-)

Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm not too familiar with the code, but this _seems_ to work for me:
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 21321be..2e03d47 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1375,6 +1375,10 @@ static struct commit_list *first_scapegoat(struct rev_info *revs, struct commit
>  static int num_scapegoats(struct rev_info *revs, struct commit *commit)
>  {
>  	struct commit_list *l = first_scapegoat(revs, commit);
> +	if (!l)
> +		return 0;
> +	if (revs->first_parent_only)
> +		return 1;
>  	return commit_list_count(l);
>  }
>
> I suspect it doesn't work at all with `--reverse`. I also have the
> nagging feeling that this could be handled inside revision.c with
> parent-rewriting, but I don't really know.

I am not sure how well --children, which is what we use from
revision.c, works with --first-parent flag passed to the revision
walking machinery.  If it already does the right thing, then the
revs.children decoration that collects all children of any given
commit should automatically give its "sole child" (in the world
limited to the first-parent chain) from first_scapegoat().

And if it does not, perhaps that is what we would want to fix,
i.e. making sure rev-list --first-parent --children does what
people would expect.

> But "git blame --first-parent <file>" seems to behave sanely in git.git
> with this.

It is intresting to see that the above is the only thing necessary,
as a naive way to try all parents would be to do:

	for (sg = first_scapegoat(...); sg; sg = sg->next)
		assign blame to sg;
	take the remainder ourselves;

in which case, a better place to patch would be first_scapegoat(),
not this function, so that we will always see zero or one element
parent list returned from the function.

But in reality, the code instead does this:

	num_sg = num_scapegoats(...);
	for (i = 0, sg = first_scapegoat(...);
             i < num_sg && sg;
             i++, sg = sg->next)
		assign blame to sg;
	take the remainder ourselves;

so you do not have to touch first_scapegoat() at all.

I do not offhand know if this was a sign of foresight in the
original, or just an overly redundant programming ;-).

Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> I'm not too familiar with the code, but this _seems_ to work for me:
>>
>> diff --git a/builtin/blame.c b/builtin/blame.c
>> index 21321be..2e03d47 100644
>> --- a/builtin/blame.c
>> +++ b/builtin/blame.c
>> @@ -1375,6 +1375,10 @@ static struct commit_list *first_scapegoat(struct rev_info *revs, struct commit
>>  static int num_scapegoats(struct rev_info *revs, struct commit *commit)
>>  {
>>  	struct commit_list *l = first_scapegoat(revs, commit);
>> +	if (!l)
>> +		return 0;
>> +	if (revs->first_parent_only)
>> +		return 1;
>>  	return commit_list_count(l);
>>  }
>>
>> I suspect it doesn't work at all with `--reverse`. I also have the
>> nagging feeling that this could be handled inside revision.c with
>> parent-rewriting, but I don't really know.
>
> I am not sure how well --children, which is what we use from
> revision.c, works with --first-parent flag passed to the revision
> walking machinery.  If it already does the right thing, then the
> revs.children decoration that collects all children of any given
> commit should automatically give its "sole child" (in the world
> limited to the first-parent chain) from first_scapegoat().
>
> And if it does not, perhaps that is what we would want to fix,
> i.e. making sure rev-list --first-parent --children does what
> people would expect.
> 
>> But "git blame --first-parent <file>" seems to behave sanely in git.git
>> with this.
>
> It is intresting to see that the above is the only thing necessary,
> as a naive way to try all parents would be to do:
>
> 	for (sg = first_scapegoat(...); sg; sg = sg->next)
> 		assign blame to sg;
> 	take the remainder ourselves;
>
> in which case, a better place to patch would be first_scapegoat(),
> not this function, so that we will always see zero or one element
> parent list returned from the function.
>
> But in reality, the code instead does this:
>
> 	num_sg = num_scapegoats(...);
> 	for (i = 0, sg = first_scapegoat(...);
>              i < num_sg && sg;
>              i++, sg = sg->next)
> 		assign blame to sg;
> 	take the remainder ourselves;
>
> so you do not have to touch first_scapegoat() at all.
>
> I do not offhand know if this was a sign of foresight in the
> original, or just an overly redundant programming ;-).

So here is an outline of what I had in mind when I was writing the
above.  Instead of trusting that num_scapegoats() is always used to
limit the enumeration of commit->parents, we discard the second and
subsequent parents from the commit objects, and also make sure the
later parents do not participate in the revs.children annotation, so
that the world "blame" sees truly becomes a single strand of pearls.

As usual, not even compile tested, but it feels correct ;-)



 builtin/blame.c | 9 ++++++++-
 revision.c      | 4 +++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 4db01c1..bc87d9d 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1366,8 +1366,15 @@ static void pass_whole_blame(struct scoreboard *sb,
  */
 static struct commit_list *first_scapegoat(struct rev_info *revs, struct commit *commit)
 {
-	if (!reverse)
+	if (!reverse) {
+		if (revs->first_parent_only &&
+		    commit->parents &&
+		    commit->parents->next) {
+			free_commit_list(commit->parents->next);
+			commit->parents->next = NULL;
+		}
 		return commit->parents;
+	}
 	return lookup_decoration(&revs->children, &commit->object);
 }
 
diff --git a/revision.c b/revision.c
index af2a18e..a020a42 100644
--- a/revision.c
+++ b/revision.c
@@ -2765,7 +2765,9 @@ static void set_children(struct rev_info *revs)
 		struct commit *commit = l->item;
 		struct commit_list *p;
 
-		for (p = commit->parents; p; p = p->next)
+		for (p = commit->parents;
+		     p && !revs->first_parent_only;
+		     p = p->next)
 			add_child(revs, p->item, commit);
 	}
 }

Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent

[Jeff King]

On Fri, Sep 11, 2015 at 12:06:27PM -0700, Junio C Hamano wrote:

> So here is an outline of what I had in mind when I was writing the
> above.  Instead of trusting that num_scapegoats() is always used to
> limit the enumeration of commit->parents, we discard the second and
> subsequent parents from the commit objects, and also make sure the
> later parents do not participate in the revs.children annotation, so
> that the world "blame" sees truly becomes a single strand of pearls.

Yeah, I think what is happening in this first hunk:

> diff --git a/builtin/blame.c b/builtin/blame.c
> index 4db01c1..bc87d9d 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1366,8 +1366,15 @@ static void pass_whole_blame(struct scoreboard *sb,
>   */
>  static struct commit_list *first_scapegoat(struct rev_info *revs, struct commit *commit)
>  {
> -	if (!reverse)
> +	if (!reverse) {
> +		if (revs->first_parent_only &&
> +		    commit->parents &&
> +		    commit->parents->next) {
> +			free_commit_list(commit->parents->next);
> +			commit->parents->next = NULL;
> +		}
>  		return commit->parents;
> +	}
>  	return lookup_decoration(&revs->children, &commit->object);
>  }

is doing the right thing. It did feel a little weird to me to be munging
the global commit objects themselves, but I guess it is fairly normal
for git code.

I wondered if you could then simplify away the counters used in the
for-loops. I.e., to end up with:

  for (sg = first_scapegoat(revs, commit); sg; sg = sg->next)

in the callers. But no. One of the reasons for the counters is that we
need to know we are on the n'th parent, so we can compare it to parent
n-1, n-2, etc, for sameness.

> diff --git a/revision.c b/revision.c
> index af2a18e..a020a42 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2765,7 +2765,9 @@ static void set_children(struct rev_info *revs)
>  		struct commit *commit = l->item;
>  		struct commit_list *p;
>  
> -		for (p = commit->parents; p; p = p->next)
> +		for (p = commit->parents;
> +		     p && !revs->first_parent_only;
> +		     p = p->next)
>  			add_child(revs, p->item, commit);
>  	}
>  }

Yeah, this makes sense to me. Adding all children to the decoration list
and then later choosing only the first child (as my earlier suggestion
did) is not right. You can tell immediately that it is nonsense because
the child you would get depends on the order we visited the commits when
building up the decoration. And that does not necessarily have any real
meaning.

-Peff

Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, I think what is happening in this first hunk:
> ...
> is doing the right thing. It did feel a little weird to me to be munging
> the global commit objects themselves, but I guess it is fairly normal
> for git code.

Yeah, the above may be brilliant, but ...

>> diff --git a/revision.c b/revision.c
>> index af2a18e..a020a42 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -2765,7 +2765,9 @@ static void set_children(struct rev_info *revs)
>>  		struct commit *commit = l->item;
>>  		struct commit_list *p;
>>  
>> -		for (p = commit->parents; p; p = p->next)
>> +		for (p = commit->parents;
>> +		     p && !revs->first_parent_only;
>> +		     p = p->next)
>>  			add_child(revs, p->item, commit);
>>  	}
>>  }

... this is a total crap and shows that I am doubly an idiot.

The loop is a no-op when first-parent-only (the intent is to call
add-child for just the first parent), so the code is stupid and
wrong in the first place, but more importantly, the logic is utterly
confused.

The thing is, traversing first-parent chain in reverse fundamentally
is undefined.  You can fork multiple topics at the tip of 'master'
and each of the topics may be single strand of pearls, but which one
of the topics is the first-child-chain---there is no answer to that
question.

The add_child() call above is exactly that.  It is saying "record
the fact that commit is child of p->item for these p on that parents
list".  We may limit the call to record that commit C is a child of
its first parent P (and nobody else), but that does not make us
record that that P has only one child (which is C).  This loop will
be entered with another commit C' whose first parent is the same P
and when we return from the set_children() function, we will find
that P has more than one children (C and C', at least); the only
thing we ensured is that all of these recorded children have P as
their first parent.  It does not tell us which one of C's we would
want to pick when digging in reverse from P.

Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent

[Philip Oakley]

From: "Junio C Hamano" <gitster@pobox.com>
Sent: Saturday, September 12, 2015 9:29 AM
> Jeff King <peff@peff.net> writes:
>
>> Yeah, I think what is happening in this first hunk:
>> ...
>> is doing the right thing. It did feel a little weird to me to be 
>> munging
>> the global commit objects themselves, but I guess it is fairly normal
>> for git code.
>
> Yeah, the above may be brilliant, but ...
>
>>> diff --git a/revision.c b/revision.c
>>> index af2a18e..a020a42 100644
>>> --- a/revision.c
>>> +++ b/revision.c
>>> @@ -2765,7 +2765,9 @@ static void set_children(struct rev_info 
>>> *revs)
>>>  struct commit *commit = l->item;
>>>  struct commit_list *p;
>>>
>>> - for (p = commit->parents; p; p = p->next)
>>> + for (p = commit->parents;
>>> +      p && !revs->first_parent_only;
>>> +      p = p->next)
>>>  add_child(revs, p->item, commit);
>>>  }
>>>  }
>
> ... this is a total crap and shows that I am doubly an idiot.
>
> The loop is a no-op when first-parent-only (the intent is to call
> add-child for just the first parent), so the code is stupid and
> wrong in the first place, but more importantly, the logic is utterly
> confused.
>
> The thing is, traversing first-parent chain in reverse fundamentally
> is undefined.  You can fork multiple topics at the tip of 'master'
> and each of the topics may be single strand of pearls, but which one
> of the topics is the first-child-chain---there is no answer to that
> question.

I don't understand why this would be so. If we just have a base commit 
from which to seek a --reversed chain then it would be true.

But if I understood the man page correctly: --reverse "This requires a 
range of revision like START..END where the path to blame exists in 
START." which when combined with --first-parent, would define a single 
string of pearls which could then be reversed. Or is it that the code 
doesn't do it in that order?

>
> The add_child() call above is exactly that.  It is saying "record
> the fact that commit is child of p->item for these p on that parents
> list".  We may limit the call to record that commit C is a child of
> its first parent P (and nobody else), but that does not make us
> record that that P has only one child (which is C).  This loop will
> be entered with another commit C' whose first parent is the same P
> and when we return from the set_children() function, we will find
> that P has more than one children (C and C', at least); the only
> thing we ensured is that all of these recorded children have P as
> their first parent.  It does not tell us which one of C's we would
> want to pick when digging in reverse from P.
--
Philip 

Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent

[Jeff King]

On Sat, Sep 12, 2015 at 01:29:43AM -0700, Junio C Hamano wrote:

> >> -		for (p = commit->parents; p; p = p->next)
> >> +		for (p = commit->parents;
> >> +		     p && !revs->first_parent_only;
> >> +		     p = p->next)
> >>  			add_child(revs, p->item, commit);
> >>  	}
> >>  }
> 
> ... this is a total crap and shows that I am doubly an idiot.
> 
> The loop is a no-op when first-parent-only (the intent is to call
> add-child for just the first parent), so the code is stupid and
> wrong in the first place, but more importantly, the logic is utterly
> confused.

Whoops, yeah. I think you need "if (revs->first_parent_only) break;".

> The thing is, traversing first-parent chain in reverse fundamentally
> is undefined.  You can fork multiple topics at the tip of 'master'
> and each of the topics may be single strand of pearls, but which one
> of the topics is the first-child-chain---there is no answer to that
> question.

In general I think I agree, but in the case of blame, we know that we
are starting from a single tip (and we know because we are using
first-parent that we remain in a single strand of pearls, because we
follow only one parent and there are no cycles).

That is assuming that we create the set of children by traversing the
first-parent history, though, which I am not at all sure about.

-Peff

Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent

[Junio C Hamano]

On Sun, Sep 13, 2015 at 3:07 AM, Jeff King <peff@peff.net> wrote:
>> The thing is, traversing first-parent chain in reverse fundamentally
>> is undefined.  You can fork multiple topics at the tip of 'master'
>> and each of the topics may be single strand of pearls, but which one
>> of the topics is the first-child-chain---there is no answer to that
>> question.
>
> In general I think I agree, but in the case of blame, we know that we
> are starting from a single tip (and we know because we are using
> first-parent that we remain in a single strand of pearls, because we
> follow only one parent and there are no cycles).

The thing is that the patch is inside revision.c machinery for --children,
and "rev-list --first-parent --children ^A ^B C" (i.e. range with multiple
bottoms) may be a perfectly a reasonable request. The requestor does
not necessarily know if A or B is an ancestor of C on the first-parent
chain. I am not sure if we are introducing unintended consequences
on this unrelated request only to change the behaviour of "blame".

For that matter, I am not sure how "blame A..B" with first-parent &
reverse should behave when A is not an ancestor on the first-parent
chain. Wouldn't we try to find a cut-point on the first-parent chain by
traversing first-parent chain from B and painting them as positive,
while traversing _all_ parents from B and painting them as negative,
until the traversal intersect? And wouldn't we discover at least two
children (one positive and one negative) for the cut point we discover
by that traversal? That cut point would be the (fake) latest state the
blame traversal starts at, and then we try to use the first (fake) parent
that in real life is the first child (which we do not have a good definition
for). And at that point a simple panda brain explodes ;-)

We might end up doing the right thing even in that case, but I haven't
convinced myself about that (yet).  If the change were limited to "blame",
the change may be much less problematic.

Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent

[Jeff King]

On Sun, Sep 13, 2015 at 10:19:33PM -0700, Junio C Hamano wrote:

> For that matter, I am not sure how "blame A..B" with first-parent &
> reverse should behave when A is not an ancestor on the first-parent
> chain. Wouldn't we try to find a cut-point on the first-parent chain by
> traversing first-parent chain from B and painting them as positive,
> while traversing _all_ parents from B and painting them as negative,
> until the traversal intersect? And wouldn't we discover at least two
> children (one positive and one negative) for the cut point we discover
> by that traversal? That cut point would be the (fake) latest state the
> blame traversal starts at, and then we try to use the first (fake) parent
> that in real life is the first child (which we do not have a good definition
> for). And at that point a simple panda brain explodes ;-)
> 
> We might end up doing the right thing even in that case, but I haven't
> convinced myself about that (yet).  If the change were limited to "blame",
> the change may be much less problematic.

Good point.

It seems like nobody is actually that interested in what "blame
--first-parent --reverse" does in the first place, though, and there's
no reason for its complexity to hold up vanilla --first-parent. So what
do you think of:

-- >8 --
Subject: [PATCH] blame: handle --first-parent

The revision.c options-parser will parse "--first-parent"
for us, but the blame code does not actually respect it, as
we simply iterate over the whole list returned by
first_scapegoat(). We can fix this by returning a
truncated parent list.

Note that we could technically also do so by limiting the
return value of num_scapegoats(), but that is less robust.
We would rely on nobody ever looking at the "next" pointer
from the returned list.

Combining "--reverse" with "--first-parent" is more
complicated, and will probably involve cooperation from
revision.c. Since the desired semantics are not even clear,
let's punt on this for now, but explicitly disallow it to
avoid confusing users (this is not really a regression,
since it did something nonsensical before).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/blame.c     | 11 ++++++++++-
 t/annotate-tests.sh |  4 ++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 4db01c1..ae4301c 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1366,8 +1366,15 @@ static void pass_whole_blame(struct scoreboard *sb,
  */
 static struct commit_list *first_scapegoat(struct rev_info *revs, struct commit *commit)
 {
-	if (!reverse)
+	if (!reverse) {
+		if (revs->first_parent_only &&
+		    commit->parents &&
+		    commit->parents->next) {
+			free_commit_list(commit->parents->next);
+			commit->parents->next = NULL;
+		}
 		return commit->parents;
+	}
 	return lookup_decoration(&revs->children, &commit->object);
 }
 
@@ -2680,6 +2687,8 @@ 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;
diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index f5c0175..b1673b3 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -111,6 +111,10 @@ test_expect_success 'blame 2 authors + 2 merged-in authors' '
 	check_count A 2 B 1 B1 2 B2 1
 '
 
+test_expect_success 'blame --first-parent blames merge for branch1' '
+	check_count --first-parent A 2 B 1 "A U Thor" 2 B2 1
+'
+
 test_expect_success 'blame ancestor' '
 	check_count -h master A 2 B 2
 '
-- 
2.6.0.rc2.408.ga2926b9

Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It seems like nobody is actually that interested in what "blame
> --first-parent --reverse" does in the first place, though, and there's
> no reason for its complexity to hold up vanilla --first-parent. So what
> do you think of:

I like the part that explicitly disables the combination of the two
;-)

Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent

[Jeff King]

On Tue, Sep 15, 2015 at 06:14:26PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > It seems like nobody is actually that interested in what "blame
> > --first-parent --reverse" does in the first place, though, and there's
> > no reason for its complexity to hold up vanilla --first-parent. So what
> > do you think of:
> 
> I like the part that explicitly disables the combination of the two
> ;-)

Meaning you didn't like the other part, or that you'll pick up the patch
as-is? :)

-Peff

Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Sep 15, 2015 at 06:14:26PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > It seems like nobody is actually that interested in what "blame
>> > --first-parent --reverse" does in the first place, though, and there's
>> > no reason for its complexity to hold up vanilla --first-parent. So what
>> > do you think of:
>> 
>> I like the part that explicitly disables the combination of the two
>> ;-)
>
> Meaning you didn't like the other part, or that you'll pick up the patch
> as-is? :)
>
> -Peff

I _especially_ like the part that explicitly disables the
combination of the two ;-)

Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent

[Max Kirillov]

On Tue, Sep 15, 2015 at 06:05:39AM -0400, Jeff King wrote:
> It seems like nobody is actually that interested in what "blame
> --first-parent --reverse" does in the first place, though, and there's
> no reason for its complexity to hold up vanilla --first-parent. So what
> do you think of:
...
> Combining "--reverse" with "--first-parent" is more
> complicated, and will probably involve cooperation from
> revision.c. Since the desired semantics are not even clear,
> let's punt on this for now, but explicitly disallow it to
> avoid confusing users (this is not really a regression,
> since it did something nonsensical before).

Hi.

I might be late for this discussion, but I seem to have
a case when blame --reverse --first-parent seems to work.

Consider the folowing history (from left ro right):

       +-----------------D1-+
      +--->C1------>C2-+     \
     /                  \     \
A0->A1-------->A2---..-->A3-->A4-->A5
 \            /
  +->B1-->B2-+

, and a line was removed in B2. Then, blame --reverse
returns D1 for this line, which is, while technically
correct, absolutely useless to find real place where the
line was removed. But blame --reverse --first-parent seems
to return A1, which is much more useful and actually what
I would expect to return. I tried it recently with
2.3-something and it seems to work as expected.

Was it the behavior you mentioned as nonsensical or you have
some other examples?

So please may I ask to not kill this completely. As about
the issue mentioned by Junio, it could fail loudly if the
requested range is not a first-parent chain.

-- 
Max

Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

> I might be late for this discussion, but I seem to have
> a case when blame --reverse --first-parent seems to work.

I think during the discussion we already established that there are
cases where the mode happens to do the right thing (the most trivial
is a completely linear history).

I do not strongly object to enabling the mode when it is safe to
enable (i.e. it can be proven to work and produce a sensible and
meaningful result); patches welcome to enable it when it can be
shown that it is safe; "to disable it only when it can be shown that
it is meaningless" is a different way to state the same thing.