Re: [PATCH v2 08/10] sparse-index: complete partial expansion

[Derrick Stolee <derrickstolee@github.com>]

On 5/21/2022 3:45 AM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> +	if (pl && !pl->use_cone_patterns) {
>>  		pl = NULL;
>> +	} else {
>> +		/*
>> +		 * We might contract file entries into sparse-directory
>> +		 * entries, and for that we will need the cache tree to
>> +		 * be recomputed.
>> +		 */
>> +		cache_tree_free(&istate->cache_tree);
>> +
>> +		/*
>> +		 * If there is a problem creating the cache tree, then we
>> +		 * need to expand to a full index since we cannot satisfy
>> +		 * the current request as a sparse index.
>> +		 */
>> +		if (cache_tree_update(istate, WRITE_TREE_MISSING_OK))
>> +			pl = NULL;
>> +	}
> 
> So, if the current index has some irrelevant (i.e. do not match the
> pattern list) subtrees in collapsed form, presense of an unmerged
> entry, presumably inside the cone(s) we are interested in, makes us
> lose the pattern list here, and we end up expanding everything?
> 
> I suspect that is a situation that is not so uncommon.  Working
> inside a narrow cone of a wide tree, performing a merge would
> hopefully allow many subtrees that are outside of the cones of our
> interest merged without getting expanded at all (e.g. only the other
> side touched these subtrees we are not working on, so their version
> will become the merge result), while changes to some paths in the
> cone of our interest may result in true conflicts represented as
> cache entries at higher stages, needing conflict resolution
> concluded with "git add".  Having to expand these subtrees that we
> managed to merge while still collapsed, only because we have
> conflicts in some other parts of the tree, feels somewhat sad.

You are correct that conflicts outside of the sparse-checkout cone will
cause index expansion. That happens during the 'git merge' command, but
the index will continue to fail to collapse as long as those conflicts
still exist in the index.

When there are conflicts like this during the merge, then the index
expansion is not as large of a portion of the command as normal, because
the conflict resolution also takes some time to compute. The commands
afterwards do take longer purely because of the expanded index.

However, this state is also not as common as you might think. If a user
has a sparse-checkout cone specified, then they are unlikely to change
files outside of the sparse-checkout cone. They would not be the reason
that those files have a conflict. The conflicts would exist only if they
are merging branches that had conflicts outside of the cone. Typically,
any merge of external changes like this are of the form of "git pull" or
"git rebase", in which case the conflicts are still "local" to the
developer's changes.

You are right that there is additional work that could be done here,
specifically allowing the cache tree to partially succeed and use the
successfully generated trees to create sparse directory entries where
possible. This was not viable before because we lacked the "partially
expanded" index state. This series establishes the necessary vocabulary to
do such an improvement later.
> By the way, why are we passing the "--missing-ok" option to "git
> write-tree" here?
> 
>> +	cache_tree_update(istate, WRITE_TREE_MISSING_OK);
> 
> The same question here.  We didn't say "missing trees are OK".  What
> made it OK in this change?
 
Both of these additions of WRITE_TREE_MISSING_OK are not needed. I
think I added them in an earlier version, thinking they were needed
due to something in the Scalar functional tests. I confirmed just now
that they are not needed for that. I will remove them.

Thanks,
-Stolee