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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 24, 2020, 20:18 UTC
Message-ID: <xmqqimhod4cu.fsf@gitster.c.googlers.com>
In-Reply-To: <20200424043225.GB1648190@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 9 quoted lines

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

Correct. I just followed the code from bloom.c::get_bloom_filter() down, and for a root commit, diff_tree_oid() with NULL in the first parameter (i.e. old_oid) is called. This NULL pointer eventually reaches tree-walk.c::fill_tree_descriptor() and the function just gives an empty tree in that case, which is what we want.

Show 6 quoted lines

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

True. oideq() is probably an improvement; I agree that pointer equality is taking it too far.