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

"Kevin Willford via GitGitGadget" [off-list ref] writes:

When using the sparse checkout feature, 'git reset' will add entries to
the index that will have the skip-worktree bit off but will leave the
working directory empty. File data is lost because the index version of
the files has been changed but there is nothing that is in the working
directory. This will cause the next 'git status' call to show either
deleted for files modified or deleting or nothing for files added. The
added files should be shown as untracked and modified files should be
shown as modified.

I am on vacation today, so let me be brief.

Let me see if I am understanding the situation correctly.

We have the index, with a path that records a blob, but the path is
marked with skip-wortree bit.

    $ rm -fr test && mkdir test && cd test
    $ git init .
    $ date >no-skip
    $ date >skip
    $ git add no-skip skip
    $ git commit -m initial
     2 files changed, 2 insertions(+)
     create mode 100644 no-skip
     create mode 100644 skip
    $ date >no-skip
    $ date >skip
    $ git add no-skip skip
    $ git update-index --skip-wortree skip
    $ rm skip
    $ git commit -m second
    [master e9088ad] second
     2 files changed, 2 insertions(+), 2 deletions(-)
    $ ls *skip
    no-skip
    $ git ls-files -t
    H no-skip
    S skip
    $ git status
    On branch master
    nothing to commit, working tree clean

Note.  There is no 'reset' done yet so far.

The user is happy with the state because

 (1) The user marked the path "skip" with skip-worktree bit, and
     thanks to that, even though "skip" is absent in the working
     tree, the "git status" does not complain.

 (2) The user marked the path "skip" with skip-worktree bit because
     the user did not want to see such a file in the working tree.
     And "git commit -m second", "git ls-files -t", or "git status"
     that were done to get here did not make it materialize in the
     working tree all of sudden.

And then the user says "git reset HEAD^" to switch to a different
commit.

    $ git reset HEAD^
    $ ls *skip
    no-skip
    $ git ls-files -t
    M no-skip
    D skip
    $ git status -suno
     M no-skip
     D skip

The user is unhappy with the state because "skip" is shown as lost.

Do I understand the situation you are trying to deal with correctly?

To fix this when the reset is running if there is not a file in the
working directory and if it will be missing with the new index entry or
was not missing in the previous version, we create the previous index
version of the file in the working directory so that status will report
correctly and the files will be availble for the user to deal with.

Assuming I read the problem description correctly, I am highly
skeptical that the above is a correct approach to keep the user
happy.  Yes, if you created a working tree file with contents that
match the blob recorded for the path in the initial commit when
"reset HEAD^" is done, you may keep "git status" quiet, so (1) above
will be kept, but what about (2)?  The user marked the path with
"skip" but, because the path should not appear on the working tree.
The "fix" is countermanding that wish by the user, isn't it?

Wouldn't a fix to the situation be to 

 * Add the blob for "skip" taken from the initial commit to the
   index, just like the entry for "no-skip" is updated;

 * But remember that "skip" was marked with "skip-worktree" bit
   immediately before "git reset" was asked to do its thing, and
   re-add the bit to the path in the index before "git reset" gives
   the control back to the usre;

 * And keep the working tree untouched, without writing anything out
   to "skip".  If the user had a (possibly unrelated) file there, it
   will not be overwritten, and if the user left the path absent, it
   will still be absent.

so that the last three diagnostic commands in the above sample
sequence would instead read:

    $ ls *skip
    no-skip
    $ git ls-files -t
    M no-skip
    S skip
    $ git status -suno
     M no-skip

i.e. skip gets updated in the index only, nothing changes in the
working tree for "skip" or "no-skip", and status reports that
"no-skip" is different from the index but "skip" hasn't changed in
the working tree since the index (thanks to its skip-worktree bit).

Then the user will be happy in the same way as the user was happy
immediately after the state marked with "There is no 'reset' done
yet so far." above, on both counts, not just for "status does not
report something got changed" part but also "user didn't want to see
'skip' in the working tree, and 'skip' did not materialize" part.

Thanks.

Junio C Hamano wrote:

Wouldn't a fix to the situation be to 

 * Add the blob for "skip" taken from the initial commit to the
   index, just like the entry for "no-skip" is updated;

 * But remember that "skip" was marked with "skip-worktree" bit
   immediately before "git reset" was asked to do its thing, and
   re-add the bit to the path in the index before "git reset" gives
   the control back to the usre;

 * And keep the working tree untouched, without writing anything out
   to "skip".  If the user had a (possibly unrelated) file there, it
   will not be overwritten, and if the user left the path absent, it
   will still be absent.

so that the last three diagnostic commands in the above sample
sequence would instead read:

    $ ls *skip
    no-skip
    $ git ls-files -t
    M no-skip
    S skip
    $ git status -suno
     M no-skip

i.e. skip gets updated in the index only, nothing changes in the
working tree for "skip" or "no-skip", and status reports that
"no-skip" is different from the index but "skip" hasn't changed in
the working tree since the index (thanks to its skip-worktree bit).

Then the user will be happy in the same way as the user was happy
immediately after the state marked with "There is no 'reset' done
yet so far." above, on both counts, not just for "status does not
report something got changed" part but also "user didn't want to see
'skip' in the working tree, and 'skip' did not materialize" part.

Thanks.

Thanks for the thorough explanation, I'm on-board with your approach (and
will re-roll the series with that implemented). A lot of my thought process
(and confusion) came from a comment in e5ca291076 (t1092: document bad
sparse-checkout behavior, 2021-07-14) suggesting that full and sparse
checkouts should have the same result in scenarios like the one you
outlined above. The problem is, as noted earlier, it's impossible to tell
whether (using your example):

1. the user deleted `skip` because they intentionally want to remove it from
   the worktree, and it should continue to be deleted after a reset.
2. `skip` doesn't exist in the worktree because it's excluded from the
   sparse checkout definition and the user does not want its current state
   "deleted" after a reset.

As a result, there's no way `git reset --mixed` could be expected to behave
the same way in full checkouts as it does in sparse, and the most consistent
solution is that the worktree should remain untouched with `skip-worktree`
preserved.

Victoria Dye [off-list ref] writes:

Thanks for the thorough explanation, I'm on-board with your approach (and
will re-roll the series with that implemented). A lot of my thought process
(and confusion) came from a comment in e5ca291076 (t1092: document bad
sparse-checkout behavior, 2021-07-14) suggesting that full and sparse
checkouts should have the same result in scenarios like the one you
outlined above.

Thanks for bringing this up.  I agree that it is crucial to clarify
what use case we are aiming for.  If the objective were to make a
sparse checkout behave just like full checkout, the desired
behaviour would be very different from a system whose objective is
to allow users to pretend as if the hidden parts of sparse checkout
do not even exist, which was the model my example was after.  I
agree with you that the "comment" in an earlier commit may have been
unhelpful in that they stopped at "should behave the same but they
shouldn't" without saying "why they should behave the same".

If the goal were to make sparse behave like full, continuing with
the previous example, after a

    $ git reset --mixed HEAD^

the user should be able to say

    $ git commit -a --amend

to replace the original two-commit history with a single commit
history that records the same resulting tree.  If the path "skip"
were to be reset to the blob from the first commit, just like the
path "no-skip" is, for such a "commit -a --amend" to work, we would
need to have a working tree file for "skip" magically materialized
with the contents from the *second* commit.  After all, the whole
point of mixed (and soft) reset is that they do not (logically)
change the files in the working tree, so if you are resetting from
the second commit to the first, if you were to have a working tree
file, it should come from the second commit, so that both "skip"
and "no-skip" should show "changed in the working tree relative to
the index", i.e.

    $ git reset --mixed HEAD^
    $ git ls-files -t
    M no-skip
    M skip

While such a "make sparse behave the same way as full" can be made
internally consistent, however, as the above example shows, it would
make the resulting "sparse checkout" practically unusable.

By stepping back a bit and realizing that the reason why the user
wanted to mark some path as "skip-worktree" was because the user had
no intention to make any change to them, we can make it usable again,
by not insisting that sparse should behave the same way as full.

When we redesign these patches, I would like to see what we failed
short the last time gets improved.  Instead of saying "skip-worktree
entries should stay so" and stopping there, we should leave a note
for later readers to explain why they should.

Thanks.