Re: [PATCH 4/4] unpack-trees: handle missing sparse directories

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 5, 2022, 19:24 UTC
Message-ID: <xmqqiln6ze64.fsf@gitster.g>
In-Reply-To: <3825ef9a-4c71-21ed-6452-bbd322ca839c@github.com>

Victoria Dye <vdye@github.com> writes:

> If I'm reading this correctly, it's not quite what I meant - the situation
> this patch addresses is when _nothing_ in the tree rooted at 'A/' (files,
> subdirectories) exists in the index, but 'unpack_trees()' is merging tree(s)
> where 'A/' *does* exist.

OK, that is very different from my flawed readign.

Show 5 quoted lines

> Maybe it would be clearer to describe it with a different perspective: "If
> 'unpack_callback()' is merging a new tree into a sparse index, merge the
> tree as a sparse directory rather than traversing its contents" or something
> like that? I'll try to come up with a better way of explaining this and
> update in V2.

Yeah, that explains a typical scenario where you want to do this kind of thing better. Right now, do we just merge in the contents of that whole tree whether the root of that new tree is or is not in the sparse cone(s)? Noticing that the new tree is outside the cones of our interest and populating the index minimally, just enough to be able to write the resulting index as a tree, does make sense.

Show 6 quoted lines

> I wrote 'missing_dir_is_sparse()' in an attempt keep some complex logic out
> of the already-complicated 'unpack_single_entry()', but as part of that it
> relies on information already established by its caller.
>
> We know 'p' is a directory because 'missing_dir_is_sparse()' is called
> inside a 'mask == dirmask' condition.

Ahh, thanks---that is exactly what I missed, and led to my questions.

Show 13 quoted lines

> 'mask' is a representation of which
> trees being traversed have an entry with the given name and 'dirmask' is a
> representation of which of those entries are directories, so the only way
> 'mask == dirmask' and 'p' is *not* a directory is if the currently-traversed
> entries in all of the trees do not exist. *That* won't happen because
> 'unpack_callback()' won't be invoked at all by 'traverse_trees()' if 'mask'
> is 0.
>
> Given that it requires jumping through multiple function invocations and
> callbacks to figure that out, I can add some assertions or 'return 0'
> conditions at the beginning of 'missing_dir_is_sparse()' to codify its
> assumptions. Even if the assertions are slightly redundant now, they'll make
> the code clearer and make the function safer for reuse.

Sounds good.

Thanks.