On 8/19/2021 2:24 PM, Elijah Newren wrote: > On Tue, Aug 10, 2021 at 12:50 PM Derrick Stolee via GitGitGadget > <gitgitgadget@xxxxxxxxx> wrote: >> >> From: Derrick Stolee <dstolee@xxxxxxxxxxxxx> >> >> 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. >> + /* >> + * 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
