Re: [PATCH 05/12] bloom: parse commit before computing filters

From: Taylor Blau <me@ttaylorr.com>
Date: May 1, 2020, 23:10 UTC
Message-ID: <20200501231042.GB46422@syl.local>
In-Reply-To: <ef4c08e401bd57dddf327910032220632a36885f.1588347029.git.gitgitgadget@gmail.com>

On Fri, May 01, 2020 at 03:30:22PM +0000, Derrick Stolee via GitGitGadget wrote:

Show 24 quoted lines

> From: Derrick Stolee <dstolee@microsoft.com>
>
> When computing changed-path Bloom filters for a commit, we need to
> know if the commit has a parent or not. If the commit is not parsed,
> then its parent pointer will be NULL.
>
> As far as I can tell, the only opportunity to reach this code
> without parsing the commit is inside "test-tool bloom
> get_filter_for_commit" but it is best to be safe.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  bloom.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/bloom.c b/bloom.c
> index eb08571c628..c3b81b1a38a 100644
> --- a/bloom.c
> +++ b/bloom.c
> @@ -206,6 +206,9 @@ struct bloom_filter *get_bloom_filter(struct repository *r,
>  	diffopt.max_changes = max_changes;
>  	diff_setup_done(&diffopt);
>
> +	/* ensure commit is parsed so we have parent information */

I don't think that this comment is critical to the change, since familiar readers will know why this 'parse_commit' call is here, but I don't really mind either way...

> +	parse_commit(c);
> +

OK. I figure that we could force the callers to parse these commits, but I think that this approach makes more sense. This should be a no-op for already-parsed commits, which means we'll get out of this call pretty quickly.

The benefit there, of course, would be that we don't have to worry about some callers forgetting to make sure that they are passing already-parsed commits, and that the trade-off we make to allow such a thing is pretty cheap, relatively speaking.

Show 5 quoted lines

>  	if (c->parents)
>  		diff_tree_oid(&c->parents->item->object.oid, &c->object.oid, "", &diffopt);
>  	else
> --
> gitgitgadget

Makes sense, thanks.

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks, Taylor