Re: [RFC PATCH] Not computing changed path filter for root commits

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 15, 2023, 18:25 UTC
Message-ID: <xmqqled7e1kq.fsf@gitster.g>
In-Reply-To: <20230911223157.446269-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 13 quoted lines

> This is following the discussion about adding a new changed path filter
> version due to the current implementation of murmur3 not matching the
> algorithm. [1]
>
> SZEDER Gábor suggested [2] that we change the revision walk to read
> changed path filters also for root commits, but I don't think that's
> possible - we have to tie reading changed path filters to when we read
> trees, and right now, we don't seem to read trees when evaluating root
> commits (rev_compare_tree() in revision.c is in the only code path that
> uses changed path filters, and it itself is only called per-parent and
> thus not called for root commits). The alternative is to not generate
> changed path filters for root commits (or what I did in this patch,
> which is to generate an all-1 filter), which seems reasonable to me.

I know this is a very silly question, but if the filter is not read for root commits at runtime, does it matter if a filter is created for them beforehand (or not)? They will not be read whether if they exist or not, no? One observation in the thread [2] appears in was:

    In several of the above test cases test_bloom_filters_used is invoked
    in a repository with only a root commit, so they don't check that
    the output is the same with and without Bloom filters.

i.e. the check would be ineffective with the current system that we know does not use the filter for a root commit even if it existed. But would it be an improvement to add a filter to a root commit and test with the filter enabled and disabled to compare the results, if we know the filter is not used anyway?