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

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 9, 2023, 17:26 UTC
Message-ID: <ZSQ3s3ZiRcvQIKOa@nand.local>
In-Reply-To: <ZSQ2XwbTM4DDLfJq@nand.local>

On Mon, Oct 09, 2023 at 01:20:31PM -0400, Taylor Blau wrote:

Show 8 quoted lines

> On Mon, Oct 02, 2023 at 03:55:46PM -0700, Jonathan Tan wrote:
> > As for the second issue, we can probably solve this by being defensive
> > in rev_same_tree_as_empty() by only using the Bloom filter when the
> > commit has no parents. Not sure if this is being overly defensive,
> > though.
>
> I am also unsure whether we are being overly defensive here or not. But
> I agree that it does feel safer to apply something like:

Never mind, we are being overly defensive there. The goal here is to avoid using a commit's Bloom filter in cases where we are acting as if a commit is at the root of history, but in fact has parents.

This only happens when we return REV_TREE_NEW from a call to `rev_compare_tree(revs, p, commit, nth_parent)`. But we'll only get REV_TREE_NEW back if

    repo_get_commit_tree(the_repository, p);

returns NULL. But when we call rev_same_tree_as_empty(revs, p) in the REV_TREE_NEW case, we return early as follows:

    struct tree *t1 = repo_get_commit_tree(revs, p);
    if (!t1)
      return 0;

So we won't even consult the Bloom filter in that case, since t1 is NULL for the same reason as what caused rev_compare_tree() to return REV_TREE_NEW in the first place.

I am still dumbfounded by how we would ever get REV_TREE_NEW in the first place, but if we did, I think we would be OK here.

Thanks, Taylor