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

From: Jeff King <peff@peff.net>
Date: Sep 12, 2015, 03:30 UTC
Message-ID: <20150912033054.GA30431@sigill.intra.peff.net>
In-Reply-To: <xmqqsi6kzsgc.fsf@gitster.mtv.corp.google.com>

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

Show 6 quoted lines

> 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:

Show 20 quoted lines

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

Show 15 quoted lines

> 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