Re: [PATCH] blame: drop unused parameter from maybe_changed_path
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 23, 2020, 21:36 UTC
- Message-ID
- <xmqqk125g9ze.fsf@gitster.c.googlers.com>
- In-Reply-To
- <20200423210303.GA1635761@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> 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?
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.
Thanks.
Show 26 quoted lines
>
> blame.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/blame.c b/blame.c
> index 9fbf79e47c..da7e28800e 100644
> --- a/blame.c
> +++ b/blame.c
> @@ -1263,7 +1263,6 @@ struct blame_bloom_data {
> static int bloom_count_queries = 0;
> static int bloom_count_no = 0;
> static int maybe_changed_path(struct repository *r,
> - struct commit *parent,
> struct blame_origin *origin,
> struct blame_bloom_data *bd)
> {
> @@ -1355,8 +1354,7 @@ static struct blame_origin *find_origin(struct repository *r,
> if (origin->commit->parents &&
> !oidcmp(&parent->object.oid,
> &origin->commit->parents->item->object.oid))
> - compute_diff = maybe_changed_path(r, parent,
> - origin, bd);
> + compute_diff = maybe_changed_path(r, origin, bd);
>
> if (compute_diff)
> diff_tree_oid(get_commit_tree_oid(parent),