On Mon, Oct 09, 2023 at 01:59:25PM -0700, Jonathan Tan wrote: > Taylor Blau <me@xxxxxxxxxxxx> writes: > > 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 > > Ah, good point. Your patch in > https://lore.kernel.org/git/ZQnmTXUO94%2FQy8mq@nand.local/ looks good to > me, then. Oops, I made a mistake in the quoted portion, which is that we could get REV_TREE_NEW if the tree-diff itself only adds files. This is the non-trivial case that we get when t1 is non-NULL, and we end up calling `diff_tree_oid()` which sets the static `tree_difference` variable. So I think adding an nth_parent field (like you originally suggested[^1]) makes sense. Thanks, Taylor [^1]: Thanks for being patient with me ;-).
