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

[Jeff King <peff@peff.net>]

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