Re: [PATCH 1/7] reset: behave correctly with sparse-checkout

"Kevin Willford via GitGitGadget" <gitgitgadget@gmail.com> writes:

Not a problem introduced by this patch per-se, but is_missing is a counter-intuitive name for what the boolean wants to represent, I think, which was brought in by b4b313f9 (reset: support "--mixed --intent-to-add" mode, 2014-02-04). Before the commit, we used to say

 	for (i = 0; i < q->nr; i++) {
 		struct diff_filespec *one = q->queue[i]->one;
		if (one->mode && !is_null_sha1(one->sha1)) {
			... create ce out of one and add to the	index ...
		} else
 			remove_file_from_cache(one->path);
		...

i.e. "if one is not missing, create a ce and add it, otherwise remove the path".

It should have been called "one_is_missing" if we wanted to literally express the condition the code checked, but an even better name would have been given after the intent of what the code wants to do with the information. If the resetted-to tree (that is what 'one' side of the comparison in diff_cache() is) has a valid blob, we want it to be in the index, and otherwise, we do not want it in the index.

Now, the patch makes things worse and I had to do the above digging to see why the new code is even more confusing. The 'two' side of the comparison is what is in the to-be-corrected-by-reset index. "was_missing" in contrast to "is_missing" makes it sound as if it was the state before whatever "is_missing" tries to represent, but that is not what is happening. "is_missing" does not mean "the entry is currently not there in the index", but "was_missing" does mean exactly that: "the entry is currently not there in the index".

There isn't any "was" missing about it. It "is" missing in the index. Instead of renaming, I wonder if we can do without this new variable. Let's read on the patch.

Also, now the code uses both sides of the filepair, we must double check that our do_diff_cache() is *not* doing any rename detection. It might be even prudent to ensure that

	if (strcmp(one->path, two->path))
		BUG("reset drove diff-cache with rename detection");

but it might be with too much paranoia.  I dunno.

These two new variables do not need this wide a scope, I would think. Shouldn't it be inside the body of the new "if" statement this patch adds?

In a sparsely checked out working tree, there is nothing in the working tree at the path. It may be because it is sparse and we didn't want to have anything there, or it may be because the user wanted to get rid of it and said "rm path" (not "git rm path") and this part of the tree were of interest even if the sparse checkout feature was used to hide other parts of the tree. With the above two checks alone, we cannot tell which. Let's read on.

We check the index to see if there is an entry for it. I suspect that because we need to do this check anyway, we shouldn't even have to look at 'two' (and add a new 'was_missing' variable), because 'one' and 'two' came from a comparison between the resetted-to tree object and the current index, and if cache_name_pos() for the path (we can use 'one->path') says there is an entry in the index, by definition, 'two' would not be showing a "removed" state (i.e. "the resetted-to tree had it, the index does not" is what "was_missing" wants to say).

So I wonder if it is better to

 - use one->path for !file_exists() above and cache_name_pos() here
   instead of two->path.

 - drop the confusingly named 'was_missing', because (pos < 0) is
   equivalent to it after this point, and we didn't need it up to
   this point.

And we do find an entry for it. So this path is not something sparse cone specifies not to check out (otherwise we would have a tree-like entry that covers this path in the index and not an entry for this specific path)?

Anyway, if it is marked with the skip-worktree bit, does that mean there is no risk that the reason why two->path does not exist in the working tree is because we earlier gave it in the working tree but it was later removed by the user? Just making sure that we are not breaking the end-user's wish that the path should be removed by resurrecting it in the working tree with a new call to checkout_entry().

And in such a case, if the resetted-to tree says we shouldn't have the path in the resulting index, or if the original state in the index had this path (but because (0 <= pos) must be true for us to reach this point, I am not sure if "was_missing" can ever be true here), then do the following, which is ...

... to resurrect the last "git add"ed state from the index and write it out to the working tree. As I suspected, ce_before and state should be scoped inside this block and not visible outside, no?

I am not sure why this behaviour is desirable. The "mixed" reset should not have to touch the working tree in the first place.

The large comment before this block says "... will not have the skip-worktree bit set", but we are dealing with a case where the original index had a cache entry there with skip-worktree bit set, so isn't the more desirable outcome that the cache entry added back to the index has the skip-worktree bit still set and there is no working tree file that the user did not desire to have?

And isn't it the matter of preserving the skip-worktree bit when the code in the post context of this hunk this patch did not touch adds the entry back to the index?

If we look at the code after this point, we do use "is_missing" information to tweak ce->ce_flags with the intent-to-add bit.

Perhaps we can do a similar tweak to the cache entry to mark it with skip-worktree bit if the index had a cache entry at the path with the bit set? The code that needs to do so would only have to remember if the one->path is in the current index and the cache entry for the path has the skip-worktree bit in the body of the new if() statement about (core_apply_sparse_checkout && !file_exists()) added by this patch (I am not sure if !file_exists() even matters, though, as the approach I am suggesting is to preserve the skip bit and not disturb the working tree files at all).

Thanks.