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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 11, 2015, 16:35 UTC
Message-ID: <xmqqa8ss29tq.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20150911140133.GA14311@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 20 quoted lines

> 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 ;-).