mbox series

[WIP,v1,0/4] mv: fix out-of-cone file/directory move logic

Message ID 20220331091755.385961-1-shaoxuan.yuan02@gmail.com (mailing list archive)
Headers show
Series mv: fix out-of-cone file/directory move logic | expand

Message

Shaoxuan Yuan March 31, 2022, 9:17 a.m. UTC
Before integrating 'mv' with sparse-index, I still find some possibly buggy
UX when 'mv' is interacting with 'sparse-checkout'. 

So I kept sparse-index off in order to sort things out without a sparse index.
We can proceed to integrate with sparse-index once these changes are solid.

Note that this patch is tentative, and still have known glitches, but it 
illustrates a general approach that I intended to harmonize 'mv' 
with 'sparse-checkout'.

Shaoxuan Yuan (4):
  mv: check if out-of-cone file exists in index with SKIP_WORKTREE bit
  mv: add check_dir_in_index() and solve general dir check issue
  mv: add advise_to_reapply hint for moving file into cone
  t7002: add tests for moving out-of-cone file/directory

 builtin/mv.c                  | 76 ++++++++++++++++++++++++++++++++---
 t/t7002-mv-sparse-checkout.sh | 72 +++++++++++++++++++++++++++++++++
 2 files changed, 142 insertions(+), 6 deletions(-)


base-commit: 805e0a68082a217f0112db9ee86a022227a9c81b

Comments

Shaoxuan Yuan March 31, 2022, 9:28 a.m. UTC | #1
On Thu, Mar 31, 2022 at 5:20 PM Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> wrote:
>
> Before integrating 'mv' with sparse-index, I still find some possibly buggy
> UX when 'mv' is interacting with 'sparse-checkout'.
>
> So I kept sparse-index off in order to sort things out without a sparse index.
> We can proceed to integrate with sparse-index once these changes are solid.
>
> Note that this patch is tentative, and still have known glitches, but it
> illustrates a general approach that I intended to harmonize 'mv'
> with 'sparse-checkout'.
>
> Shaoxuan Yuan (4):
>   mv: check if out-of-cone file exists in index with SKIP_WORKTREE bit
>   mv: add check_dir_in_index() and solve general dir check issue
>   mv: add advise_to_reapply hint for moving file into cone
>   t7002: add tests for moving out-of-cone file/directory
>
>  builtin/mv.c                  | 76 ++++++++++++++++++++++++++++++++---
>  t/t7002-mv-sparse-checkout.sh | 72 +++++++++++++++++++++++++++++++++
>  2 files changed, 142 insertions(+), 6 deletions(-)
>
>
> base-commit: 805e0a68082a217f0112db9ee86a022227a9c81b
> --
> 2.35.1
>

The original related RFC patch is [1], and this patch should be
--in-reply-to [2].

[1] https://lore.kernel.org/git/20220315100145.214054-1-shaoxuan.yuan02@gmail.com/
[2] https://lore.kernel.org/git/97a665fe-07c9-c4f6-4ab6-b6c0e1397c31@github.com/
Victoria Dye March 31, 2022, 10:21 p.m. UTC | #2
Shaoxuan Yuan wrote:
> Before integrating 'mv' with sparse-index, I still find some possibly buggy
> UX when 'mv' is interacting with 'sparse-checkout'. 
> 
> So I kept sparse-index off in order to sort things out without a sparse index.
> We can proceed to integrate with sparse-index once these changes are solid.
> 
> Note that this patch is tentative, and still have known glitches, but it 
> illustrates a general approach that I intended to harmonize 'mv' 
> with 'sparse-checkout'.
> 

Thanks for working out some ways to make 'mv' behave more nicely with sparse
checkouts! I did my best to address some of the specific implementation
questions you had in your commit messages. Beyond that, my main points of
feedback (beyond some formatting nits and implementation questions) are:

* Patch 2 deals with sparse directories, which won't show up until you
  enable sparse index; since you can't test that yet, you should save the
  patch for your "sparse index integration" series.
* Patch 4 should either be moved to the beginning of the series (with the
  tests flagged with 'test_expect_failure' until the patch that fixes the
  associated behavior), or split up with the tests associated with a change
  moved into the patch that makes that change.

And, as always, I'm happy to answer any questions and/or clarify weird
behavior you encounter while making changes to this (or subsequent) series!

> Shaoxuan Yuan (4):
>   mv: check if out-of-cone file exists in index with SKIP_WORKTREE bit
>   mv: add check_dir_in_index() and solve general dir check issue
>   mv: add advise_to_reapply hint for moving file into cone
>   t7002: add tests for moving out-of-cone file/directory
> 
>  builtin/mv.c                  | 76 ++++++++++++++++++++++++++++++++---
>  t/t7002-mv-sparse-checkout.sh | 72 +++++++++++++++++++++++++++++++++
>  2 files changed, 142 insertions(+), 6 deletions(-)
> 
> 
> base-commit: 805e0a68082a217f0112db9ee86a022227a9c81b
Shaoxuan Yuan April 1, 2022, 12:18 p.m. UTC | #3
On Fri, Apr 1, 2022 at 6:21 AM Victoria Dye <vdye@github.com> wrote:
> Thanks for working out some ways to make 'mv' behave more nicely with sparse
> checkouts! I did my best to address some of the specific implementation
> questions you had in your commit messages. Beyond that, my main points of
> feedback (beyond some formatting nits and implementation questions) are:
>
> * Patch 2 deals with sparse directories, which won't show up until you
>   enable sparse index; since you can't test that yet, you should save the
>   patch for your "sparse index integration" series.
> * Patch 4 should either be moved to the beginning of the series (with the
>   tests flagged with 'test_expect_failure' until the patch that fixes the
>   associated behavior), or split up with the tests associated with a change
>   moved into the patch that makes that change.
>
> And, as always, I'm happy to answer any questions and/or clarify weird
> behavior you encounter while making changes to this (or subsequent) series!

Hi Victoria,

Thanks a lot for the detailed and informative feedback, they are
incredibly helpful
to guide me through this first attempt! I have read them all and am preparing
corresponding fixes :-)
Shaoxuan Yuan April 8, 2022, 12:22 p.m. UTC | #4
On Thu, Mar 31, 2022 at 5:20 PM Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> wrote:
>
> Before integrating 'mv' with sparse-index, I still find some possibly buggy
> UX when 'mv' is interacting with 'sparse-checkout'.
>
> So I kept sparse-index off in order to sort things out without a sparse index.
> We can proceed to integrate with sparse-index once these changes are solid.
>
> Note that this patch is tentative, and still have known glitches, but it
> illustrates a general approach that I intended to harmonize 'mv'
> with 'sparse-checkout'.
>
> Shaoxuan Yuan (4):
>   mv: check if out-of-cone file exists in index with SKIP_WORKTREE bit
>   mv: add check_dir_in_index() and solve general dir check issue
>   mv: add advise_to_reapply hint for moving file into cone
>   t7002: add tests for moving out-of-cone file/directory
>
>  builtin/mv.c                  | 76 ++++++++++++++++++++++++++++++++---
>  t/t7002-mv-sparse-checkout.sh | 72 +++++++++++++++++++++++++++++++++
>  2 files changed, 142 insertions(+), 6 deletions(-)
>
>
> base-commit: 805e0a68082a217f0112db9ee86a022227a9c81b
> --
> 2.35.1
>

Hi, to whom it may concern,

I'm writing to say that I'm making useful (possibly) progress on this topic.

I've been busy composing a GSoC proposal last week, so the progress
paused for a while. And I'm going to have a trip for around a week, starting
next week, so I may not have enough time next week to push forward on
this topic.

But I will always be available through email ;-)


--
Thanks & Regards,
Shaoxuan