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

[Victoria Dye <vdye@github.com>]

Junio C Hamano wrote:
> "Kevin Willford via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> @@ -127,12 +129,49 @@ static void update_index_from_diff(struct diff_queue_struct *q,
>>  		struct diff_options *opt, void *data)
>>  {
>>  	int i;
>> +	int pos;
>>  	int intent_to_add = *(int *)data;
>>  
>>  	for (i = 0; i < q->nr; i++) {
>>  		struct diff_filespec *one = q->queue[i]->one;
>> +		struct diff_filespec *two = q->queue[i]->two;
>>  		int is_missing = !(one->mode && !is_null_oid(&one->oid));
>> +		int was_missing = !two->mode && is_null_oid(&two->oid);
> 
> 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.

The new variable can most likely be refactored away, but based on this it's
probably worth renaming "is_missing" to "is_missing_in_reset_tree" (or
inverting the boolean and using "is_in_reset_tree").

> 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.

I don't think a rename would break what this change intends to do (although
it does break some of the current assumptions in the patch). I'll make sure
to verify the rename case works before submitting a new version, just in 
case.

>>  		struct cache_entry *ce;
>> +		struct cache_entry *ce_before;
>> +		struct checkout state = CHECKOUT_INIT;
> 
> 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?

I will likely need to make other changes to this patch and re-roll, so I'll
fix the scoping of all of the variables added here when I do.

>> +		/*
>> +		 * When using the sparse-checkout feature the cache entries
>> +		 * that are added here will not have the skip-worktree bit
>> +		 * set. Without this code there is data that is lost because
>> +		 * the files that would normally be in the working directory
>> +		 * are not there and show as deleted for the next status.
>> +		 * In the case of added files, they just disappear.
>> +		 *
>> +		 * We need to create the previous version of the files in
>> +		 * the working directory so that they will have the right
>> +		 * content and the next status call will show modified or
>> +		 * untracked files correctly.
>> +		 */
>> +		if (core_apply_sparse_checkout && !file_exists(two->path)) {
> 
> 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.
> 
>> +			pos = cache_name_pos(two->path, strlen(two->path));
> 
> 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.
> 
>> +			if ((pos >= 0 && ce_skip_worktree(active_cache[pos])) &&
> 
> 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().
> 
>> +			    (is_missing || !was_missing)) {
> 
> 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 ...
> 
>> +				state.force = 1;
>> +				state.refresh_cache = 1;
>> +				state.istate = &the_index;
>> +
>> +				ce_before = make_cache_entry(&the_index, two->mode,
>> +							     &two->oid, two->path,
>> +							     0, 0);
>> +				if (!ce_before)
>> +					die(_("make_cache_entry failed for path '%s'"),
>> +						two->path);
>> +
>> +				checkout_entry(ce_before, &state, NULL, NULL);
> 
> ... 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 (is_missing && !intent_to_add) {
>>  			remove_file_from_cache(one->path);
> 
> 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).

I think it might easier to address these points as a whole rather than
inline.

The problem this patch is attempting to solve is that, while (as you noted)
`git reset --mixed` should not touch the working tree, it is *also* expected
to preserve the files of the pre-reset state (both statements paraphrased
from the `--mixed` option doc). Normally these statements don't conflict,
but if `skip-worktree` is respected and nothing is done to the working tree
before resetting the index, `skip-worktree` files will effectively be `reset
--hard`. So, to force preservation of the pre-reset state, the files are
checked out.

Based on that high-level intent, the implementation here can be simplified
(and clarified). The condition on checking out a file (to avoid the `reset 
--hard`) would be "if the path exists in the current index and the entry 
in the index has `skip-worktree` enabled".

* "if the path exists in the current index" - if it does not exist in the
  index, there's nothing to preserve.
* "if the entry in the index has `skip-worktree` enabled" - if it does not,
  it's already in the working tree so we don't need to checkout.

Then, `checkout_entry()` can then be run on the index entry found (rather
than a "fake" one created with `make_cache_entry`). This eliminates a lot of
unnecessary usage of `one` and `two`, which hopefully addresses some of your
concerns about them. After that, the index reset proceeds as normal (without
manual changes to the `skip-worktree` bit).

As for the issue of ignoring `skip-worktree`: all of this could be
conditioned on a "--ignore-skip-worktree-bits" flag (or something like it)
if you'd prefer the default behavior is "don't touch the working tree".