Re: [RFC PATCH 1/1] mv: integrate with sparse-index

From: Derrick Stolee <derrickstolee@github.com>
Date: Mar 28, 2022, 13:32 UTC
Message-ID: <b709435b-d5f7-d7f8-a22a-0fc78e106928@github.com>
In-Reply-To: <CAJyCBOQT1TwkNX_be9B3uKsv4Buf_ojfZoqfTAUqQ22Na7dY=g@mail.gmail.com>

On 3/26/2022 11:48 PM, Shaoxuan Yuan wrote:

Show 19 quoted lines

> On Fri, Mar 18, 2022 at 5:57 AM Victoria Dye <vdye@github.com> wrote:
> 
> Hi all,
> 
> It's been a busy week, I'm sorry that did not have much time to respond.
> 
> =================================
> A brief summary of my latest investigations:
> 
> I think the 'git mv' command still has some questionable aspects, especially
> with the 'git sparse-checkout' command. And I feel we have to sort things out
> between 'git mv' and sparse-checkout first, then proceed to the issues with
> sparse-index.
> ===========
> 
> I'm trying to fix the first 2 of the 3 potential things mentioned earlier.
> 
>> 1. When empty folder2/ is on-disk, 'git mv' (without '--sparse') doesn't
>>    fail with "bad source", even though it should.

>> 2. When you try to move a sparse file with 'git mv --sparse', it still
>>    fails.

I think that these conditions are all related to the perspective as described in the documentation of 'git mv':

  In the first form, it renames <source>, which **must exist** and be
  either a file, symlink or directory, to <destination>. In the second
  form, the last argument has to be **an existing** directory; the given
  sources will be moved into this directory.

  The index is updated after successful completion, but the change must
  still be committed.

(**emphasis mine**)

So, this documents the perspective that files must exist in the worktree (and destination directories must exist in the worktree), and after all of those checks, then the move is staged.

I think part of our issues here is that in the case of a sparse-checkout, we can have index entries that don't exist in the worktree. The expected behavior we are considering is that 'git mv' should stage the movement of the file in the index, ignoring the worktree for paths outside of the sparse-checkout definition.

One way to do this would be to flip the implementation's direction: perform the index operation of moving the cache entry, then update the worktree to reflect that change (if necessary).

The case that I can think about being a bit strange is if the user has an unstaged deletion of the source file, then runs 'git mv'. Since the worktree is missing the file, then we cannot do the equivalent 'mv' operation in the worktree.

One other thing to keep in mind is that 'git mv <source> <destination>' can act like 'mv <source> <destination> && git add <destination>' if <source> is an untracked path. So, 'git mv' can succeed even if the source is not in the index!

So the change here is to ignore a non-existing path when the same path exists as a cache entry with the SKIP_WORKTREE bit. That bit does say "ignore the worktree" so 'git mv' isn't doing the right thing already.

---

In conclusion, there might be multiple ways forward here:

 1. Keep the expectation that <source> is in the worktree as given,
    and let a tracked <source> outside of the sparse-checkout cone
    result in a failure (as it currently does). Consider adding an
    advice message if <source> is a tracked, sparse path.

 2. Change the expectation to be that <source> must either be a
    file in the worktree _or_ a tracked, sparse path (or both).

The nice thing here is that we can do (1) and then (2) later. The key things to investigate for the sparse index, then are what happens when <destination> is outside of the sparse-checkout cone? I imagine it would be fine if the moved file still appears in the worktree and is cleaned up by a later 'git sparse-checkout reapply'.

I'm eager to here alternate opinions and options on this topic.

Thanks, -Stolee