| Message ID | 20220527100804.209890-6-shaoxuan.yuan02@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mv: fix out-of-cone file/directory move logic | expand |
On Fri, May 27 2022, Shaoxuan Yuan wrote: > + if (sparse_moved) { > + struct unpack_trees_options o; > + memset(&o, 0, sizeof(o)); > + o.verbose_update = isatty(2); > + o.update = 1; > + o.head_idx = -1; > + o.src_index = &the_index; > + o.dst_index = &the_index; > + o.skip_sparse_checkout = 0; > + o.pl = the_index.sparse_checkout_patterns; You can drop the memset here and use the designated init syntax instead. I.e. "struct x o = { .verbose_update .... };"
Shaoxuan Yuan wrote: > Originally, "git mv" a sparse file/directory from out/in-cone to > in/out-cone does not update the sparsity following the sparse-checkout > patterns. > I generally agree with the intent here - that, if you move a non-sparse file out-of-cone, it should become sparse (and vice versa). However, that result can be reached by simply flipping the 'SKIP_WORKTREE' bit(s) on the resultant index entry/entries (which you already have, since they're renamed with 'rename_cache_entry_at()' below). 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]). For reference, I'd advise against reapplying the sparsity patterns - as you do below - because involves a much more expensive traversal of the entire repository. It also has the possibly unwanted side effect of resetting the 'SKIP_WORKTREE' bit to match the sparse patterns on *all* files, not just the one(s) you moved. [1] https://lore.kernel.org/git/11d46a399d26c913787b704d2b7169cafc28d639.1642175983.git.gitgitgadget@gmail.com/ > Use update_sparsity() after touching sparse contents, so the sparsity > will be updated after the move. > > Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> > --- > builtin/mv.c | 19 +++++++++++++++++++ > t/t7002-mv-sparse-checkout.sh | 16 ++++++++++++++++ > 2 files changed, 35 insertions(+) > > diff --git a/builtin/mv.c b/builtin/mv.c > index e64f251a69..2c02120941 100644 > --- a/builtin/mv.c > +++ b/builtin/mv.c > @@ -13,6 +13,7 @@ > #include "string-list.h" > #include "parse-options.h" > #include "submodule.h" > +#include "unpack-trees.h" > > static const char * const builtin_mv_usage[] = { > N_("git mv [<options>] <source>... <destination>"), > @@ -158,6 +159,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) > { > int i, flags, gitmodules_modified = 0; > int verbose = 0, show_only = 0, force = 0, ignore_errors = 0, ignore_sparse = 0; > + int sparse_moved = 0; > struct option builtin_mv_options[] = { > OPT__VERBOSE(&verbose, N_("be verbose")), > OPT__DRY_RUN(&show_only, N_("dry run")), > @@ -376,6 +378,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix) > const char *src = source[i], *dst = destination[i]; > enum update_mode mode = modes[i]; > int pos; > + if (!sparse_moved && mode & (SPARSE | SKIP_WORKTREE_DIR)) > + sparse_moved = 1; > if (show_only || verbose) > printf(_("Renaming %s to %s\n"), src, dst); > if (show_only) > @@ -403,6 +407,21 @@ int cmd_mv(int argc, const char **argv, const char *prefix) > rename_cache_entry_at(pos, dst); > } > > + if (sparse_moved) { > + struct unpack_trees_options o; > + memset(&o, 0, sizeof(o)); > + o.verbose_update = isatty(2); > + o.update = 1; > + o.head_idx = -1; > + o.src_index = &the_index; > + o.dst_index = &the_index; > + o.skip_sparse_checkout = 0; > + o.pl = the_index.sparse_checkout_patterns; > + setup_unpack_trees_porcelain(&o, "mv"); > + update_sparsity(&o); > + clear_unpack_trees_porcelain(&o); > + } > + > if (gitmodules_modified) > stage_updated_gitmodules(&the_index); > > diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh > index cf2f5dc46f..1fd3e3c0fc 100755 > --- a/t/t7002-mv-sparse-checkout.sh > +++ b/t/t7002-mv-sparse-checkout.sh > @@ -287,6 +287,22 @@ test_expect_success 'refuse to move sparse file to existing destination' ' > test_cmp expect stderr > ' > > +# Need fix. > +# > +# The *expected* behavior: > +# > +# Using --sparse to accept a sparse file, --force to overwrite the destination. > +# The folder1/file1 should replace the sub/file1 without error. > +# > +# The *actual* behavior: > +# > +# It emits a warning: > +# > +# warning: Path ' sub/file1 > +# ' already present; will not overwrite with sparse update. > +# After fixing the above paths, you may want to run `git sparse-checkout > +# reapply`. > + > test_expect_failure 'move sparse file to existing destination with --force and --sparse' ' > git sparse-checkout disable && > git reset --hard && This error is (I think) part of 'update_sparsity()'. If you change the approach to only modifying the 'SKIP_WORKTREE' bit, hopefully you'll get the behavior you're looking for.
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?
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.
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. 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? [1] https://lore.kernel.org/git/xmqq8rqm3fxa.fsf@gitster.g/
Shaoxuan Yuan wrote: > 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/ >
On Fri, Jun 17, 2022 at 12:42 AM Victoria Dye <vdye@github.com> wrote: *Truncated messages* > > 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. I generally think this middle-ground option is good. Though I think the sort of options that "messing with sparse contents" should be handled by '--sparse', instead of '--force', since the latter is used to "force move/rename even if target exists". Mixing the usage may cause syntax confusion? > > 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> This is what I meant, a user manually running `git sparse-checkout reapply`. Though I did say users should only do this "after staging its change". I propose this solution which sounds good to me: 1. If 'file1' is clean, iff with the use of '--sparse', move the file & make it sparse. 2. If 'file1' is dirty, iff with the use of '--sparse', move the file & *do not* make it sparse, instead advise something like "file1 is not up to date, keep it non-sparse. Stage file1 then run `git sparse-checkout reapply` to re-sparsify it." > [1] https://lore.kernel.org/git/077a0579-903e-32ad-029c-48572d471c84@github.com/ > > > [1] https://lore.kernel.org/git/xmqq8rqm3fxa.fsf@gitster.g/ > >
diff --git a/builtin/mv.c b/builtin/mv.c index e64f251a69..2c02120941 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -13,6 +13,7 @@ #include "string-list.h" #include "parse-options.h" #include "submodule.h" +#include "unpack-trees.h" static const char * const builtin_mv_usage[] = { N_("git mv [<options>] <source>... <destination>"), @@ -158,6 +159,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) { int i, flags, gitmodules_modified = 0; int verbose = 0, show_only = 0, force = 0, ignore_errors = 0, ignore_sparse = 0; + int sparse_moved = 0; struct option builtin_mv_options[] = { OPT__VERBOSE(&verbose, N_("be verbose")), OPT__DRY_RUN(&show_only, N_("dry run")), @@ -376,6 +378,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix) const char *src = source[i], *dst = destination[i]; enum update_mode mode = modes[i]; int pos; + if (!sparse_moved && mode & (SPARSE | SKIP_WORKTREE_DIR)) + sparse_moved = 1; if (show_only || verbose) printf(_("Renaming %s to %s\n"), src, dst); if (show_only) @@ -403,6 +407,21 @@ int cmd_mv(int argc, const char **argv, const char *prefix) rename_cache_entry_at(pos, dst); } + if (sparse_moved) { + struct unpack_trees_options o; + memset(&o, 0, sizeof(o)); + o.verbose_update = isatty(2); + o.update = 1; + o.head_idx = -1; + o.src_index = &the_index; + o.dst_index = &the_index; + o.skip_sparse_checkout = 0; + o.pl = the_index.sparse_checkout_patterns; + setup_unpack_trees_porcelain(&o, "mv"); + update_sparsity(&o); + clear_unpack_trees_porcelain(&o); + } + if (gitmodules_modified) stage_updated_gitmodules(&the_index); diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh index cf2f5dc46f..1fd3e3c0fc 100755 --- a/t/t7002-mv-sparse-checkout.sh +++ b/t/t7002-mv-sparse-checkout.sh @@ -287,6 +287,22 @@ test_expect_success 'refuse to move sparse file to existing destination' ' test_cmp expect stderr ' +# Need fix. +# +# The *expected* behavior: +# +# Using --sparse to accept a sparse file, --force to overwrite the destination. +# The folder1/file1 should replace the sub/file1 without error. +# +# The *actual* behavior: +# +# It emits a warning: +# +# warning: Path ' sub/file1 +# ' already present; will not overwrite with sparse update. +# After fixing the above paths, you may want to run `git sparse-checkout +# reapply`. + test_expect_failure 'move sparse file to existing destination with --force and --sparse' ' git sparse-checkout disable && git reset --hard &&
Originally, "git mv" a sparse file/directory from out/in-cone to in/out-cone does not update the sparsity following the sparse-checkout patterns. Use update_sparsity() after touching sparse contents, so the sparsity will be updated after the move. Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> --- builtin/mv.c | 19 +++++++++++++++++++ t/t7002-mv-sparse-checkout.sh | 16 ++++++++++++++++ 2 files changed, 35 insertions(+)