Re: [PATCH] blame: drop unused parameter from maybe_changed_path

From: Jeff King <peff@peff.net>
Date: Apr 24, 2020, 04:32 UTC
Message-ID: <20200424043225.GB1648190@coredump.intra.peff.net>
In-Reply-To: <xmqqk125g9ze.fsf@gitster.c.googlers.com>

On Thu, Apr 23, 2020 at 02:36:53PM -0700, Junio C Hamano wrote:

Show 14 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > We don't use the "parent" parameter at all (probably because the bloom
> > filter for a commit is always defined against a single parent anyway).
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > This is on top of ds/blame-on-bloom, which just made it to next.
> >
> > I _think_ this is the right solution, but perhaps the function should be
> > verifying that we're looking at the right parent?
> 
> Hmph, "solution" to what problem?  Ah, the fact that parent is an
> unused parameter?

Yes, exactly.

Show 14 quoted lines

> find_origin() runs a tree-diff over "parent" and "origin->commit",
> with literal pathspec limited to the single path.
> 
> And the Bloom filter addition changed the code so that we first
> consult the filter when "origin->commit"'s first parent *is*
> "parent".  Presumably, by asking maybe_changed_path about "origin",
> as "origin" knows what the commit is (i.e. "origin->commit") and
> what path we are talking about (i.e. "origin->path"), it can answer
> "does origin->commit change origin->path relative to its first
> parent?" and it can do so only for the first parent?
> 
> The way I read bloom.c::get_bloom_filter(), it only computes a
> diff-tree between the given commit and its first parent (or an empty
> tree), so I think the above is correct.

Yeah, the bloom filters are always against the first parent. I think I just got lost in this rather long oidcmp(), which is really just "is 'parent' the first parent?"

	if (origin->commit->parents &&
	    !oidcmp(&parent->object.oid,
		    &origin->commit->parents->item->object.oid))
		compute_diff = maybe_changed_path(r, origin, bd);

If the bloom filter also computes against an empty tree for root commits (I didn't check, but that would make sense), I think that AND could be an OR:

   if (!origin->commit->parents ||
       !oidcmp(...))

though it probably doesn't matter that much in practice. Root commits are rather rare.

BTW, we could also be using oideq() here. I thought coccicheck would note this, but it doesn't seem to. I suspect we could also get away with a direct pointer comparison of "parent == origin->commit->parents->item" due to the way we allocate "struct commit", but I'd rather err on the safer and less subtle side. :)

-Peff