Re: [PATCH v2 05/20] sparse-index: implement ensure_full_index()

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 12, 2021, 20:08 UTC
Message-ID: <xmqqft106sok.fsf@gitster.g>
In-Reply-To: <c5e42675-5f03-728a-60ec-880da368085b@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

Show 15 quoted lines

>> Ævar, the assumption that led to your e68237bb (tree.h API: remove
>> support for starting at prefix != "", 2021-03-08) closes the door
>> for this code rather badly.  Please work with Derrick to figure out
>> what the best course of action would be.
>
> Thanks for pointing this out, Junio.
>
> My preference would be to drop "tree.h API: remove support for
> starting at prefix != """, but it should be OK to keep "tree.h API:
> remove "stage" parameter from read_tree_recursive()" (currently
> b3a078863f6), even though it introduces a semantic conflict here.
>
> Since I haven't seen my sparse-index topic get picked up by a
> tracking branch, I'd be happy to rebase on top of Ævar's topic if
> I can still set a non-root prefix.

I did try to have both in 'seen' (after all, that is the primary way I find out these conflicts early---no one can keep all the details of all the topics in flight in one's head), and saw that we now have a need for non-empty prefix that we thought we no longer have in the other topic --- I think we should probably keep support of non-empty prefix (as the primary reason why that patch exists is because we saw no in-tree users---now if your 05/20 proves to be a good use of the feature, there is one fewer reasons to remove the support) in some form, so discarding e68237bb certainly is an option.

If we were to base the sparse-index topic on top of ab/read-tree, we may be able to gain further simplification and clean-up of the API.

I think all the clean-up value e68237bb has are on the calling side (they no longer have to pass constant ("", 0) to the function), and we could rewrite e68237bb by

 - renaming "read_tree_recursive()" to "read_tree_at()", with the
   non-empty prefix support.

 - creating a new function "read_tree()", which lacks the support
   for prefix, as a thin-wrapper around "read_tree_at()".

 - modifying the callers of "read_tree_recursive()" changed by
   e68237bb to instead call "read_tree()" (without prefix).

to simplify majority of calling sites without losing functionality.

Then your [05/20] can use the read_tree_at() to read with a prefix.

But that kind of details, I'd want to see you two figure out yourselves.

Thanks.