Re: [PATCH v4 7/7] mv: add check_dir_in_index() and solve general dir check issue

[Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>]

On 6/23/2022 11:14 PM, Derrick Stolee wrote:
 > On 6/23/2022 7:41 AM, Shaoxuan Yuan wrote:
 >> Originally, moving a <source> directory which is not on-disk due
 >> to its existence outside of sparse-checkout cone, "giv mv" command
 >> errors out with "bad source".
 >>
 >> Add a helper check_dir_in_index() function to see if a directory
 >> name exists in the index. Also add a SKIP_WORKTREE_DIR bit to mark
 >> such directories.
 >>
 >> Change the checking logic, so that such <source> directory makes
 >> "giv mv" command warns with "advise_on_updating_sparse_paths()"
 >> instead of "bad source"; also user now can supply a "--sparse" flag so
 >> this operation can be carried out successfully.
 >>
 >> Helped-by: Victoria Dye <vdye@github.com>
 >> Helped-by: Derrick Stolee <derrickstolee@github.com>
 >> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
 >> ---
 >>  builtin/mv.c                  | 50 ++++++++++++++++++++++++++++++-----
 >>  t/t7002-mv-sparse-checkout.sh |  4 +--
 >>  2 files changed, 46 insertions(+), 8 deletions(-)
 >>
 >> diff --git a/builtin/mv.c b/builtin/mv.c
 >> index aa29da4337..b5d0d8ef4f 100644
 >> --- a/builtin/mv.c
 >> +++ b/builtin/mv.c
 >> @@ -25,6 +25,7 @@ enum update_mode {
 >>      WORKING_DIRECTORY = (1 << 1),
 >>      INDEX = (1 << 2),
 >>      SPARSE = (1 << 3),
 >> +    SKIP_WORKTREE_DIR = (1 << 4),
 >>  };
 >>
 >>  #define DUP_BASENAME 1
 >> @@ -123,6 +124,36 @@ static int index_range_of_same_dir(const char 
*src, int length,
 >>      return last - first;
 >>  }
 >>
 >> +/*
 >> + * Check if an out-of-cone directory should be in the index. 
Imagine this case
 >> + * that all the files under a directory are marked with 
'CE_SKIP_WORKTREE' bit
 >> + * and thus the directory is sparsified.
 >> + *
 >> + * Return 0 if such directory exist (i.e. with any of its contained 
files not
 >> + * marked with CE_SKIP_WORKTREE, the directory would be present in 
working tree).
 >> + * Return 1 otherwise.
 >> + */
 >
 > This description and the implementation seems like it will work
 > even if the path exists as a sparse directory in a sparse index.
 >
 > It would be good to consider testing this kind of move for a
 > directory on the sparse boundary (where it would be a sparse
 > directory in a sparse index) _and_ if it is deeper than the
 > boundary (so the sparse index would expand in the cache_name_pos()
 > method). These tests can be written now for correctness, but later
 > the first case can be updated to use the 'ensure_not_expanded'
 > helper in t1092.

I'm a bit confused here. Shouldn't we turn on the sparse-index
feature for 'mv' before adding sparse-index related tests? Since this
series does not go into sparse-index, I'm not sure how the tests can
pass. Perhaps we can test about this in the future sparse-index
integration series, no?

Thanks & Regards,
Shaoxuan