From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org, szeder.dev@gmail.com, me@ttaylorr.com,
derrickstolee@github.com
Subject: Re: [RFC PATCH] Not computing changed path filter for root commits
Date: Fri, 15 Sep 2023 11:25:09 -0700 [thread overview]
Message-ID: <xmqqled7e1kq.fsf@gitster.g> (raw)
In-Reply-To: <20230911223157.446269-1-jonathantanmy@google.com> (Jonathan Tan's message of "Mon, 11 Sep 2023 15:31:56 -0700")
Jonathan Tan <jonathantanmy@google.com> writes:
> 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?
next prev parent reply other threads:[~2023-09-15 18:25 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-11 22:31 [RFC PATCH] Not computing changed path filter for root commits Jonathan Tan
2023-09-15 18:25 ` Junio C Hamano [this message]
2023-09-15 20:29 ` SZEDER Gábor
2023-09-19 18:19 ` Taylor Blau
2023-10-02 22:55 ` Jonathan Tan
2023-10-09 17:20 ` Taylor Blau
2023-10-09 17:26 ` Taylor Blau
2023-10-09 20:59 ` Jonathan Tan
2023-10-10 19:47 ` Taylor Blau
2023-09-19 18:21 ` Taylor Blau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqled7e1kq.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=jonathantanmy@google.com \
--cc=me@ttaylorr.com \
--cc=szeder.dev@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).