Re: [WIP v2 5/5] mv: use update_sparsity() after touching sparse contents

From: Victoria Dye <vdye@github.com>
Date: Jun 16, 2022, 16:42 UTC
Message-ID: <6375c172-82cb-dffc-875f-e5e742d5e49e@github.com>
In-Reply-To: <CAJyCBOQGAL9aGW+Gxv8sZH9T_tB6_pdeLNwmNgqPhz7cMdZrbA@mail.gmail.com>

Shaoxuan Yuan wrote:

Show 37 quoted lines

> On Sat, May 28, 2022 at 5:24 AM Victoria Dye <vdye@github.com> wrote:
>>
>> Junio C Hamano wrote:
>>> Victoria Dye <vdye@github.com> writes:
>>>
>>>> Note that you'll also probably need to check out the file(s) (if moving into
>>>> the cone) or remove them from disk (if moving out of cone). If you don't,
>>>> files moved into cone will appear "deleted" on-disk, and files moved
>>>> out-of-cone that still appear on disk will have 'SKIP_WORKTREE'
>>>> automatically disabled (see [1]).
>>>
>>> Does it also imply that we should forbid "git mv" of a dirty path
>>> out of the cone?  Or is that too draconian and it suffices to tweak
>>> the rule slightly to "remove from the worktree when moving a clean
>>> path out of cone", perhaps?  When a dirty path is moved out of cone,
>>> we would trigger the "SKIP_WORKTREE automatically disabled" behaviour
>>> and that would be a good thing, I imagine?
>>>
>>
>> I like the idea of the modified rule as an option since it *does* complete
>> the move in accordance with '--force', but doesn't result in silently lost
>> information.
>>
>> An alternative might be 'mv' refusing to move a modified file out-of-cone
>> (despite '--force'), printing something like
>> 'WARNING_SPARSE_NOT_UPTODATE_FILE' ("Path 'x' not uptodate; will not remove
>> from working tree").
>>
>> I'm not sure which would provide a more vs. less frustrating experience, but
>> both are at least safe in terms of preserving unstaged changes.
> 
> For me, the alternative provides a less frustrating experience.
> 
> Since it is more explicit (giving a message and directly saying NO).
>> Also, the `sparse-checkout` users should expect the moved file to be
> missing in the working tree, as opposed to being present.
>

Good point, since the sparseness of the destination file would be different depending on whether it had local modifications or not (with no indication from 'mv' of the different treatment).

If you're interested, maybe there's a middle-ground option? Suppose you want to move a file 'file1' to an out-of-cone location:

1. If 'file1' is clean, regardless of use of '--force', move the file & make
   it sparse.
2. If 'file1' is *not* clean and '--force' is *not* used, refuse to move the
   file (with a "Path 'file1' not uptodate; will not move. Use '--force' to
   override." type of error).
3. If 'file1' is *not* clean and '--force' is used, move the file but do not
   make it sparse.

That way, '--force' really does force the move to happen, but users are generally warned against it. I'm still not sure what the "right" approach is, but to your point I think it should err on the side of not surprising the user.

> And the tweaked rule suggested by Junio [1] might need an extra
>  `git sparse-checkout reapply` to re-sparsify the file that moved out-of-cone
> after staging its change?
>

Just so I understand correctly, do you mean 'git sparse-checkout reapply' *as part of* the 'mv' operation? Or are you thinking that a user might want to manually run 'git sparse-checkout reapply' after running 'mv'?

If it's the former (internally calling 'git sparse-checkout reapply' in 'mv'), then no, you wouldn't want to do that. In Junio's suggestion, he said (emphasis mine):

> When a dirty path is moved out of cone, we would trigger the
> "SKIP_WORKTREE automatically disabled" behaviour" *and that would be a
> good thing, I imagine?*

We don't want the file moved out-of-cone to be sparse again because it has local (on-disk) modifications that would disappear (since a file needs to be removed from disk to be "sparse" in the eyes of 'sparse-checkout'). It's *completely valid* behavior to have an out-of-cone file become non-sparse if a user does something to cause that; it doesn't cause any bugs/corruption with the repo. And, even if you did want to make the file sparse, it should be done by manually setting 'SKIP_WORKTREE' and individually removing the file from disk (for all the reasons I mentioned in my upthread comment [1]).

On the other hand, if you're talking about a user manually running 'git sparse-checkout reapply' after the fact, that wouldn't work either - they'd get an error:

warning: The following paths are not up to date and were left despite sparse patterns:
        <out-of-cone modified file>

[1] https://lore.kernel.org/git/077a0579-903e-32ad-029c-48572d471c84@github.com/

> [1] https://lore.kernel.org/git/xmqq8rqm3fxa.fsf@gitster.g/
>