Re: [PATCH v2 3/8] sparse-index: silently return when cache tree fails

On 8/19/2021 2:24 PM, Elijah Newren wrote:

On Tue, Aug 10, 2021 at 12:50 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

From: Derrick Stolee <redacted>

If cache_tree_update() returns a non-zero value, then it could not
create the cache tree. This is likely due to a path having a merge
conflict. Since we are already returning early, let's return silently to
avoid making it seem like we failed to write the index at all.

If we remove our dependence on the cache tree within
convert_to_sparse(), then we could still recover from this scenario and
have a sparse index.

When constructing the cache-tree extension in convert_to_sparse(), it is
possible that we construct a tree object that is new to the object
database. Without the WRITE_TREE_MISSING_OK flag, this results in an
error that halts our conversion to a sparse index. Add this flag to
remove this limitation.

Would this only happen when the user has staged but uncommitted
changes outside the sparsity paths, and tries to sparsify while in
that state?  (Notably, this is a much different condition than the
above mentioned merge conflict case that would case
cache_tree_update() to just fail.)

I think it might be nicer to split this commit in two, just to make it
easier to understand for future readers.  This seems like two logical
changes and trying to understand them and why would I think be easier
if the two were split.  I'd be tempted to put the
WRITE_TREE_MISSING_OK first.

Ironically, I _had_ this as two commits because I discovered the
problems independently. It wasn't until I was organizing things that
I realized I was editing the same 'if' twice and thought it better
to merge patches. But I also don't feel strongly about that, so I
can split them.

quoted

+       /*
+        * Silently return if there is a problem with the cache tree update,
+        * which might just be due to a conflict state in some entry.
+        *
+        * This might create new tree objects, so be sure to use
+        * WRITE_TREE_MISSING_OK.
+        */
+       if (cache_tree_update(istate, WRITE_TREE_MISSING_OK))
+               return 0;

...

These feel like cases where it would be nice to have a testcase
demonstrating the change in behavior.  Perhaps just splitting the
commit would be enough, but it took a bit of time to try to understand
what would change and why, despite the simple changes.

I found these were required in the Scalar functional tests, so I bet
that if I remove this change I can create a test case from that. Thanks.

-Stolee