Derrick Stolee [off-list ref] writes:
quoted
Æ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.
On 3/12/2021 3:08 PM, Junio C Hamano wrote:
Derrick Stolee [off-list ref] writes:
quoted
quoted
Æ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 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.
You've given us a great proposal. I'll wait for Ævar to chime in
(and probably update his topic) before I submit a new version.
Thanks,
-Stolee
On Fri, Mar 12 2021, Derrick Stolee wrote:
On 3/12/2021 3:08 PM, Junio C Hamano wrote:
quoted
Derrick Stolee [off-list ref] writes:
quoted
quoted
Æ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 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.
You've given us a great proposal. I'll wait for Ævar to chime in
(and probably update his topic) before I submit a new version.
I've re-rolled my series just now at
https://lore.kernel.org/git/20210315234344.28427-1-avarab@gmail.com/
sorry for the delay.
You should be able to rebase easily on top of it, although note that the
new read_tree_at() uses a strbuf, but is otherwise the same as the old
read_tree_recursive().
Note that the pathspec can also be used to get to where
read_tree_recursive() would have brought you. I haven't looked at
whether there's reasons to convert in-tree (or this) code to pathspec
use, or vice-versa convert some things that use pathspecs now
(e.g. ls-tree with a path) to providing a prefix via the strbuf.