Status of Tree.cache and TreeModifier is unclear #1771

Thanks for bringing this up.

Since I don't have any knowledge anymore about this implementation except that it is most likely incorrect and should rather not be used for anything serious, what's left to discuss, I presume, is if code should be deprecated so it can eventually be removed.

Maybe it would be worth trying to find it if it is usable for something after all (are there no tests for this?), or one could pre-emptively deprecate it to see if an outcry follows.

In general, I'd always be for removing code, especially if its effectiveness is unclear, but I'd also not want to risk breakage in case we are missing something.

As always, I'd rely on your expertise and analysis, with a strong slant towards following your suggestions.

Maybe it would be worth trying to find it if it is usable for something after all (are there no tests for this?)

Currently there are no tests, as they were removed in b295c13, which possibly was also intended to remove the code under test, I am not sure. Some of that test code could potentially be brought back in some capacity (or just experimented with on a feature branch to try and figure out what, if anything, Tree.cache and TreeModifier might be used for). I don't know that they were able to pass: they appear to have been expanded as part of a change made to verify that Tree.cache and TreeModifier couldn't achieve their once hoped-for goal.

This is not necessarily the most important aspect of the situation, but if people are using this, and using it with a lot of data, then a possible benefit of replacing the hand-rolled mergesort implementation with a call to list.sort (which should be feasible without complicated code or big overhead, ever since the behavior of sorting longer names before their prefixes was abandoned) is that list.sort in CPython is Timsort, which is highly adaptive: sorting data that are already almost sorted takes only linear time. The overhead of re-sorting seems to have been a concern in guiding usage:

Anyway, I don't propose that any such change be rushed into.

In b295c13, this was added to the changelog:

• CRITICAL: Tree.cache was removed without replacement. It is technically impossible to change individual trees and expect their serialization results to be consistent with what git expects. Instead, use the IndexFile facilities to adjust the content of the staging area, and write it out to the respective tree objects using IndexFile.write_tree() instead.

That commit was referenced in closing #369. Recently, work picked back up on this in #1799, which changed the sorting to use a custom key selector function that performs the comparison in a different way. Since then, this issue is out of date, but I am unsure if it is fully fixed.

I think the the point for this issue, and for improving documentation of TreeModifier and what advice should be given about whether and when to use it, is:

It is technically impossible to change individual trees and expect their serialization results to be consistent with what git expects.

What is the reason that was the case, and is it not still the case?

Unfortunately, I don't know and assume the git commit log also doesn't clarify it. I don't believe it's impossible either, but also don't believe that these facilities should be used - I think they can't be trusted and using them may proliferate subtle or (hopefully) less subtle breakage in git data structures when written to the object database.

If they were to be fixed, I'd probably focus on what GitPython was originally meant to be, that is a wrapper around the git executable, and implement all slightly more complex functionality by invoking git instead. That way it's probably slower particularly for many small trees, but at least it will definitely be correct.

With all that said, it's probably still unclear what to do here, apologies :/.

I am sorry, my memory has eroded over the last decade. However, the code in question should have tests which might make matters clearer. If there are no tests, then I think it's best to deprecate and discourage usage entirely, as I also think it's not really worth spending time on.