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

[Taylor Blau <me@ttaylorr.com>]

On Fri, May 01, 2020 at 03:30:22PM +0000, Derrick Stolee via GitGitGadget wrote:
> 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.

>  	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