[PATCH] unpack-trees: do not set SKIP_WORKTREE on submodules

From: Elijah Newren <newren@gmail.com>

As noted in commit e7d7c73249 ("git-sparse-checkout: clarify interactions with submodules", 2020-06-10), sparse-checkout cannot remove submodules even if they don't match the sparsity patterns, because doing so would risk data loss -- unpushed, uncommitted, or untracked changes could all be lost. That commit also updated the documentation to point out that submodule initialization state was a parallel, orthogonal reason that entries in the index might not be present in the working tree.

However, sparsity and submodule initialization weren't actually fully orthogonal yet. The SKIP_WORKTREE handling in unpack_trees would attempt to set the SKIP_WORKTREE bit on submodules when the submodule did not match the sparsity patterns. This resulted in innocuous but potentially alarming warning messages:

    warning: unable to rmdir 'sha1collisiondetection': Directory not empty

It could also make things confusing since the entry would be marked as SKIP_WORKTREE in the index but actually still be present in the working tree:

    $ git ls-files -t | grep sha1collisiondetection
    S sha1collisiondetection
    $ ls -al sha1collisiondetection/ | wc -l
    13

Submodules have always been their own form of "partial checkout" behavior, with their own "present or not" state determined by running "git submodule [init|deinit|update]". Enforce that separation by having the SKIP_WORKTREE logic not touch submodules and allow submodules to continue using their own initialization state for determining if the submodule is present.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
    unpack-trees: do not set SKIP_WORKTREE on submodules
    
    Interactions between submodules and sparsity patterns have come up a few
    times, with a certain edge case coming up multiple times recently:
    should a submodule have the SKIP_WORKTREE bit set if the submodule path
    does not match the sparsity patterns?[1][2][3].
    
    Here I try to resolve the question, with the answer of 'no'.
    
    This patch depends on en/sparse-with-submodule-doc lightly -- the commit
    message in this patch references the commit from that other series. It
    could possibly be considered an addition to that other topic, but
    "sparse-with-submodule-doc" implies the other topic is only a
    documentation change, whereas this patch involves a code change.
    
    [1] 
    https://lore.kernel.org/git/pull.805.git.git.1591831009762.gitgitgadget@gmail.com/
    
    > Since submodules may have unpushed changes or untracked files,
    > removing them could result in data loss. Thus, changing sparse
    > inclusion/exclusion rules will not cause an already checked out
    > submodule to be removed from the working copy. Said another way, just
    > as checkout will not cause submodules to be automatically removed or
    > initialized even when switching between branches that remove or add
    > submodules, using sparse-checkout to reduce or expand the scope of
    > "interesting" files will not cause submodules to be automatically
    > deinitialized or initialized either.
    
    [2] 
    https://lore.kernel.org/git/CABPp-BE+BL3Nq=Co=-kNB_wr=6gqX8zcGwa0ega_pGBpk6xYsg@mail.gmail.com/
    
    > If sparsity patterns would exclude a submodule that is initialized,
    > sparse-checkout clearly can't remove the submodule. However, should it
    > set the SKIP_WORKTREE bit for that submodule if it's not going to
    > remove it?
    
    [3] 
    https://lore.kernel.org/git/CABPp-BFJG7uFAZNidDPK2o7eHv-eYBsmcdnVxkOnKcZo7WzmFQ@mail.gmail.com/
    
    >> Or if you don't deinitialize a submodule that is excluded by the
    >> sparsity patterns (thus remaining in the working copy, anyway).
    >
    > This case requires more thought. If a submodule doesn't match the
    > sparsity patterns, we already said elsewhere that sparse-checkout
    > should not remove the submodule (since doing so would risk data loss).
    > But do we set the SKIP_WORKTREE bit for it? Generally, sparse-checkout
    > avoids removing files with modifications, and if it doesn't remove
    > them it also doesn't set the SKIP_WORKTREE bit. For consistency,
    > should sparse-checkout not set SKIP_WORKTREE for initialized
    > submodules?

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-809%2Fnewren%2Fsparse-submodule-no-skip-worktree-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-809/newren/sparse-submodule-no-skip-worktree-v1
Pull-Request: https://github.com/git/git/pull/809

 unpack-trees.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)