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

> Originally, moving a <source> file which is not on-disk but exists in
> index as a SKIP_WORKTREE enabled cache entry, "giv mv" command errors
> out with "bad source".
> 
> Change the checking logic, so that such <source>
> file 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.
> 

> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
> I found a new problem introduced by this patch, it is written in the TODO.
> I still haven't found a better way to reconcile this conflict. Please enlighten
> me on this :-)
> 
>  builtin/mv.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 83a465ba83..32ad4d5682 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -185,8 +185,32 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  
>  		length = strlen(src);
>  		if (lstat(src, &st) < 0) {
> +			/*
> +			 * TODO: for now, when you try to overwrite a <destination>
> +			 * with your <source> as a sparse file, if you supply a "--sparse"
> +			 * flag, then the action will be done without providing "--force"
> +			 * and no warning.
> +			 *
> +			 * This is mainly because the sparse <source>
> +			 * is not on-disk, and this if-else chain will be cut off early in
> +			 * this check, thus the "--force" check is ignored. Need fix.
> +			 */
> +

> +			int pos = cache_name_pos(src, length);
> +			if (pos >= 0) {
> +				const struct cache_entry *ce = active_cache[pos];
> +
> +				if (ce_skip_worktree(ce)) {
> +					if (!ignore_sparse)
> +						string_list_append(&only_match_skip_worktree, src);
> +					else
> +						modes[i] = SPARSE;
> +				}
> +				else
> +					bad = _("bad source");

> +			}
>  			/* only error if existence is expected. */
> -			if (modes[i] != SPARSE)
> +			else if (modes[i] != SPARSE)
>  				bad = _("bad source");
>  		} else if (!strncmp(src, dst, length) &&
>  				(dst[length] == 0 || dst[length] == '/')) {

>>  		if (lstat(src, &st) < 0) {
>> +			/*
>> +			 * TODO: for now, when you try to overwrite a <destination>
>> +			 * with your <source> as a sparse file, if you supply a "--sparse"
>> +			 * flag, then the action will be done without providing "--force"
>> +			 * and no warning.
>> +			 *
>> +			 * This is mainly because the sparse <source>
>> +			 * is not on-disk, and this if-else chain will be cut off early in
>> +			 * this check, thus the "--force" check is ignored. Need fix.
>> +			 */
>> +
> 
> I can clarify this a bit. 'mv' is done in two steps: first the file-on-disk
> rename (in the call to 'rename()'), then the index entry (in
> 'rename_cache_entry_at()'). In the case of a sparse file, you're only
> dealing with the latter. However, 'rename_cache_entry_at()' moves the index
> entry with the flag 'ADD_CACHE_OK_TO_REPLACE', since it leaves it up to
> 'cmd_mv()' to enforce the "no overwrite" rule. 
> 
> So, in the case of moving *to* a SKIP_WORKTREE entry (where a file being
> present won't trigger the failure), you'll want to check that the
> destination *index entry* doesn't exist in addition to the 'lstat()' check.
> It might require some rearranging of if-statements in this block, but I
> think it can be done in 'cmd_mv'. 

> +static int check_dir_in_index(const char *dir)
> +{
> +	int ret = 0;
> +	int length = sizeof(dir) + 1;
> +	char *substr = malloc(length);
> +
> +	for (int i = 0; i < the_index.cache_nr; i++) {

>
> On Thu, Mar 31 2022, Shaoxuan Yuan wrote:
>
> > +static int check_dir_in_index(const char *dir)
> > +{
> > +     int ret = 0;
> > +     int length = sizeof(dir) + 1;
> > +     char *substr = malloc(length);
> > +
> > +     for (int i = 0; i < the_index.cache_nr; i++) {
>
> See https://lore.kernel.org/git/xmqqy20r3rv7.fsf@gitster.g/ for how
> we're not quite using this syntax yet.
>
> This should also be "unsigned int" to go with the "cache_nr" member.
>
> > +             memcpy(substr, the_index.cache[i]->name, length);
> > +             memset(substr + length - 1, 0, 1);
> > +
> > +             if (strcmp(dir, substr) == 0) {
>
> Style: don't compare against == 0, or == NULL, use !, see CodingGuidelines.
>
> > +                     else if (check_dir_in_index(src_w_slash) &&
> > +                     !path_in_sparse_checkout(src_w_slash, &the_index)) {
>
> Funny indentation, the ! should be aligned with "(".
>
> > -                             modes[i] = WORKING_DIRECTORY;
> > +                             if (!modes[i])
> > +                                     modes[i] = WORKING_DIRECTORY;
>
> This works, but assuming things about enum values (even if 0) always
> seems a bit nasty, can this be a comparison to BOTH instead of !? May or
> may not be better...
>
> But then again we do xcalloc() to allocate it, so we assume that
> already, nevermind... :)
>
> (there were also indentation issues below)

> 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 SPARSE_DIRECTORY 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.
> 
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
> Since I'm so new to C language (not an acquaintance until this patch), 
> the "check_dir_in_index()" function I added might not be ideal (in terms of 
> safety and correctness?). I have digging into the APIs provided in the codebase 
> but I haven't found anything to do this very job: find out if a directory is 
> in the index (am I missing something?). 
> Probably because contents are stored in the index as blobs and 
> they all represent regular files. So I came up with this dull solution...
> 
>  builtin/mv.c | 41 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 32ad4d5682..9da9205e01 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -115,6 +115,25 @@ static int index_range_of_same_dir(const char *src, int length,
>  	return last - first;
>  }
>  
> +static int check_dir_in_index(const char *dir)
> +{

> +	int ret = 0;
> +	int length = sizeof(dir) + 1;
> +	char *substr = malloc(length);
> +
> +	for (int i = 0; i < the_index.cache_nr; i++) {
> +		memcpy(substr, the_index.cache[i]->name, length);
> +		memset(substr + length - 1, 0, 1);
> +
> +		if (strcmp(dir, substr) == 0) {
> +			ret = 1;
> +			return ret;
> +		}
> +	}
> +	free(substr);
> +	return ret;
> +}
> +
>  int cmd_mv(int argc, const char **argv, const char *prefix)
>  {
>  	int i, flags, gitmodules_modified = 0;
> @@ -129,7 +148,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		OPT_END(),
>  	};
>  	const char **source, **destination, **dest_path, **submodule_gitfile;
> -	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX, SPARSE } *modes;
> +	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX, SPARSE,
> +	SPARSE_DIRECTORY } *modes;
>  	struct stat st;
>  	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
>  	struct lock_file lock_file = LOCK_INIT;
> @@ -197,6 +217,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			 */
>  
>  			int pos = cache_name_pos(src, length);
> +			const char *src_w_slash = add_slash(src);
> +
>  			if (pos >= 0) {
>  				const struct cache_entry *ce = active_cache[pos];
>  
> @@ -209,6 +231,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  				else
>  					bad = _("bad source");
>  			}
> +			else if (check_dir_in_index(src_w_slash) &&
> +			!path_in_sparse_checkout(src_w_slash, &the_index)) {
> +				modes[i] = SPARSE_DIRECTORY;
> +				goto dir_check;
> +			}

>  			/* only error if existence is expected. */
>  			else if (modes[i] != SPARSE)
>  				bad = _("bad source");
> @@ -219,7 +246,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  				&& lstat(dst, &st) == 0)
>  			bad = _("cannot move directory over file");
>  		else if (src_is_dir) {
> -			int first = cache_name_pos(src, length), last;
> +			int first, last;
> +dir_check:
> +			first = cache_name_pos(src, length);
>  
>  			if (first >= 0)
>  				prepare_move_submodule(src, first,
> @@ -230,7 +259,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			else { /* last - first >= 1 */
>  				int j, dst_len, n;
>  
> -				modes[i] = WORKING_DIRECTORY;
> +				if (!modes[i])
> +					modes[i] = WORKING_DIRECTORY;
>  				n = argc + last - first;
>  				REALLOC_ARRAY(source, n);
>  				REALLOC_ARRAY(destination, n);
> @@ -332,7 +362,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			printf(_("Renaming %s to %s\n"), src, dst);
>  		if (show_only)
>  			continue;
> -		if (mode != INDEX && mode != SPARSE && rename(src, dst) < 0) {
> +		if (mode != INDEX && mode != SPARSE && mode != SPARSE_DIRECTORY &&
> +		 rename(src, dst) < 0) {
>  			if (ignore_errors)
>  				continue;
>  			die_errno(_("renaming '%s' failed"), src);
> @@ -346,7 +377,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  							      1);
>  		}
>  
> -		if (mode == WORKING_DIRECTORY)
> +		if (mode == WORKING_DIRECTORY || mode == SPARSE_DIRECTORY)

>
> 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 SPARSE_DIRECTORY bit to mark
> > such directories.
> >
>
> Hmm, I think this patch would fit better in your eventual "sparse index
> integration" series than this "prerequisite fixes to sparse-checkout"
> series. Sparse directories *only* appear when you're using a sparse index
> so, theoretically, this shouldn't ever come up (and thus isn't testable)
> until you're using a sparse index.

> > diff --git a/builtin/mv.c b/builtin/mv.c
> > index 32ad4d5682..9da9205e01 100644
> > --- a/builtin/mv.c
> > +++ b/builtin/mv.c
> > @@ -115,6 +115,25 @@ static int index_range_of_same_dir(const char *src, int length,
> >       return last - first;
> >  }
> >
> > +static int check_dir_in_index(const char *dir)
> > +{
>
> This function can be made a lot simpler - you can use `cache_name_pos()` to
> find the index entry - if it's found, the directory exists as a sparse
> directory. And, because 'add_slash()' enforces the trailing slash later on,
> you don't need to worry about adjusting the name before you look for the
> entry.

> > +     int ret = 0;
> > +     int length = sizeof(dir) + 1;
> > +     char *substr = malloc(length);
> > +
> > +     for (int i = 0; i < the_index.cache_nr; i++) {
> > +             memcpy(substr, the_index.cache[i]->name, length);
> > +             memset(substr + length - 1, 0, 1);
> > +
> > +             if (strcmp(dir, substr) == 0) {
> > +                     ret = 1;
> > +                     return ret;
> > +             }
> > +     }
> > +     free(substr);
> > +     return ret;
> > +}
> > +
> >  int cmd_mv(int argc, const char **argv, const char *prefix)
> >  {
> >       int i, flags, gitmodules_modified = 0;
> > @@ -129,7 +148,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> >               OPT_END(),
> >       };
> >       const char **source, **destination, **dest_path, **submodule_gitfile;
> > -     enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX, SPARSE } *modes;
> > +     enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX, SPARSE,
> > +     SPARSE_DIRECTORY } *modes;
> >       struct stat st;
> >       struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
> >       struct lock_file lock_file = LOCK_INIT;
> > @@ -197,6 +217,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> >                        */
> >
> >                       int pos = cache_name_pos(src, length);
> > +                     const char *src_w_slash = add_slash(src);
> > +
> >                       if (pos >= 0) {
> >                               const struct cache_entry *ce = active_cache[pos];
> >
> > @@ -209,6 +231,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> >                               else
> >                                       bad = _("bad source");
> >                       }
> > +                     else if (check_dir_in_index(src_w_slash) &&
> > +                     !path_in_sparse_checkout(src_w_slash, &the_index)) {
> > +                             modes[i] = SPARSE_DIRECTORY;
> > +                             goto dir_check;
> > +                     }
>
> In if-statements like this, you'll want to line up the statements in
> parentheses on subsequent lines, like:
>
>         else if (check_dir_in_index(src_w_slash) &&
>                  !path_in_sparse_checkout(src_w_slash, &the_index)) {
>
> ...where the second line is indented 1 (8-space-sized) tab + 1 space.
>
> In general, if you're trying to align code (in this repository), align first
> with as many tabs as possible, then the "remainder" with spaces. Note that
> this isn't 100% consistent throughout the repository - older lines might not
> have been aligned properly - but you should go for this styling on any new
> lines that you add.

>
> >                       /* only error if existence is expected. */
> >                       else if (modes[i] != SPARSE)
> >                               bad = _("bad source");
> > @@ -219,7 +246,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> >                               && lstat(dst, &st) == 0)
> >                       bad = _("cannot move directory over file");
> >               else if (src_is_dir) {
> > -                     int first = cache_name_pos(src, length), last;
> > +                     int first, last;
> > +dir_check:
> > +                     first = cache_name_pos(src, length);
> >
> >                       if (first >= 0)
> >                               prepare_move_submodule(src, first,
> > @@ -230,7 +259,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> >                       else { /* last - first >= 1 */
> >                               int j, dst_len, n;
> >
> > -                             modes[i] = WORKING_DIRECTORY;
> > +                             if (!modes[i])
> > +                                     modes[i] = WORKING_DIRECTORY;
> >                               n = argc + last - first;
> >                               REALLOC_ARRAY(source, n);
> >                               REALLOC_ARRAY(destination, n);
> > @@ -332,7 +362,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> >                       printf(_("Renaming %s to %s\n"), src, dst);
> >               if (show_only)
> >                       continue;
> > -             if (mode != INDEX && mode != SPARSE && rename(src, dst) < 0) {
> > +             if (mode != INDEX && mode != SPARSE && mode != SPARSE_DIRECTORY &&
> > +              rename(src, dst) < 0) {
> >                       if (ignore_errors)
> >                               continue;
> >                       die_errno(_("renaming '%s' failed"), src);
> > @@ -346,7 +377,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> >                                                             1);
> >               }
> >
> > -             if (mode == WORKING_DIRECTORY)
> > +             if (mode == WORKING_DIRECTORY || mode == SPARSE_DIRECTORY)
>
> I'm a bit confused - doesn't this mean the sparse dir move will be skipped?
> In your commit description, you mention that this 'mv' succeeds with the
> '--sparse' option, but I don't see any place where the sparse directory
> would be moved.

>>
>> 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 SPARSE_DIRECTORY bit to mark
>>> such directories.
>>>
>>
>> Hmm, I think this patch would fit better in your eventual "sparse index
>> integration" series than this "prerequisite fixes to sparse-checkout"
>> series. Sparse directories *only* appear when you're using a sparse index
>> so, theoretically, this shouldn't ever come up (and thus isn't testable)
>> until you're using a sparse index.
> 
> After reading your feedback, I realized that I totally misused
> the phrase "sparse directory". Clearly, this patch series does not
> deal with sparse-
> index yet, as "sparse directory" is a cache entry that points to a
> tree, if sparse-index
> is enabled. Silly me ;)
> 
> What I was *actually* trying to say is: I want to change the checking
> logic of moving
> a "directory that exists outside of sparse-checkout cone", and I
> apparently misused
> "sparse directory" to reference such a thing.

>>> +static int check_dir_in_index(const char *dir)
>>> +{
>>
>> This function can be made a lot simpler - you can use `cache_name_pos()` to
>> find the index entry - if it's found, the directory exists as a sparse
>> directory. And, because 'add_slash()' enforces the trailing slash later on,
>> you don't need to worry about adjusting the name before you look for the
>> entry.
> 
> Yes, if I correctly used the phrase "sparse directory", but I did not...
> I think I can use 'cache_name_pos()' to
> check a directory *iff* it is a legit sparse directory when using sparse-index?
> 
> In my case, I just want to check a regular directory that is not in
> the worktree,
> since the cone pattern excludes it. And in a non-sparse index, cache
> entry points only
> to blobs, not trees, and that's the reason I wrote this weird function
> to look into the
> index. I understand that sounds not compatible with how git manages
> index, but all
> I want to know is "does this directory exist in the index?" (this
> question is also quasi-correct).
> 
> I tried to find an existing API for this job, but I failed to find
> any. Though I have a hunch
> that there must be something to do it...

>>>                       /* only error if existence is expected. */
>>>                       else if (modes[i] != SPARSE)
>>>                               bad = _("bad source");
>>> @@ -219,7 +246,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>>                               && lstat(dst, &st) == 0)
>>>                       bad = _("cannot move directory over file");
>>>               else if (src_is_dir) {
>>> -                     int first = cache_name_pos(src, length), last;
>>> +                     int first, last;
>>> +dir_check:
>>> +                     first = cache_name_pos(src, length);
>>>
>>>                       if (first >= 0)
>>>                               prepare_move_submodule(src, first,
>>> @@ -230,7 +259,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>>                       else { /* last - first >= 1 */
>>>                               int j, dst_len, n;
>>>
>>> -                             modes[i] = WORKING_DIRECTORY;
>>> +                             if (!modes[i])
>>> +                                     modes[i] = WORKING_DIRECTORY;

>>>                               n = argc + last - first;
>>>                               REALLOC_ARRAY(source, n);
>>>                               REALLOC_ARRAY(destination, n);
>>> @@ -332,7 +362,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>>                       printf(_("Renaming %s to %s\n"), src, dst);
>>>               if (show_only)
>>>                       continue;
>>> -             if (mode != INDEX && mode != SPARSE && rename(src, dst) < 0) {
>>> +             if (mode != INDEX && mode != SPARSE && mode != SPARSE_DIRECTORY &&

>>> +              rename(src, dst) < 0) {
>>>                       if (ignore_errors)
>>>                               continue;
>>>                       die_errno(_("renaming '%s' failed"), src);
>>> @@ -346,7 +377,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>>                                                             1);
>>>               }
>>>
>>> -             if (mode == WORKING_DIRECTORY)
>>> +             if (mode == WORKING_DIRECTORY || mode == SPARSE_DIRECTORY)

>>> @@ -129,7 +148,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>>  		OPT_END(),
>>>  	};
>>>  	const char **source, **destination, **dest_path, **submodule_gitfile;
>>> -	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX, SPARSE } *modes;
>>> +	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX, SPARSE,
>>> +	SPARSE_DIRECTORY } *modes;
>>>  	struct stat st;
>>>  	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
>>>  	struct lock_file lock_file = LOCK_INIT;

>> I'm a bit confused - doesn't this mean the sparse dir move will be skipped?
>> In your commit description, you mention that this 'mv' succeeds with the
>> '--sparse' option, but I don't see any place where the sparse directory
>> would be moved.
> 
> Well, you know the drill, I did not use "sparse directory" correctly, let alone
> 'SPARSE_DIRECTORY' enum bit in this hunk. I think it makes some sense
> if you apply my actual meaning of 'SPARSE_DIRECTORY' here (it should be
> something like OUT_OF_CONE_WORKING_DIRECTORY)? Because such
> directory is not on disk, it cannot be "rename()"d, and should also skip the
> "rename_cache_entry_at()" function. If all the files under the directory are
> moved/renamed, then (in my opinion) the directory is both moved to the
> destination,
> both in the worktree and in the index.

>
> On 4/1/2022 8:49 AM, Shaoxuan Yuan wrote:> On Fri, Apr 1, 2022 at 5:28 AM Victoria Dye <vdye@github.com> wrote:
> >>
> >> 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 SPARSE_DIRECTORY bit to mark
> >>> such directories.
> >>>
> >>
> >> Hmm, I think this patch would fit better in your eventual "sparse index
> >> integration" series than this "prerequisite fixes to sparse-checkout"
> >> series. Sparse directories *only* appear when you're using a sparse index
> >> so, theoretically, this shouldn't ever come up (and thus isn't testable)
> >> until you're using a sparse index.
> >
> > After reading your feedback, I realized that I totally misused
> > the phrase "sparse directory". Clearly, this patch series does not
> > deal with sparse-
> > index yet, as "sparse directory" is a cache entry that points to a
> > tree, if sparse-index
> > is enabled. Silly me ;)
> >
> > What I was *actually* trying to say is: I want to change the checking
> > logic of moving
> > a "directory that exists outside of sparse-checkout cone", and I
> > apparently misused
> > "sparse directory" to reference such a thing.
>
> In the case of a full index (or an expanded sparse index, which is
> currently always the case for `git mv`), you detect a sparse directory
> by looking for the directory in the index, _not_ finding it, and then
> seeing if the cache entry at the position where the directory _would_
> exist is marked with the SKIP_WORKTREE bit.
>
> This works in cone mode and the old mode because I assume you've already
> checked for the existence of the directory, so if there _was_ any
> non-SKIP_WORKTREE cache entry within the directory, then the directory
> would exist in the worktree.
>
> (These are good details to include in the commit message.)

> I read and think about this part a few times, but I'm still confused.
>
> As Victoria pointed out earlier, and I quote, "Sparse directories *only* appear
> when you're using a sparse index, so, theoretically, this shouldn't ever
> come up (and thus isn't testable) until you're using a sparse index."
> So I'm not so sure what do you mean by putting "full index" and "sparse
> directory" together.
>
> Thus, I go ahead and try to detect a directory that is outside of
> sparse-checkout cone, without sparse-index enabled.
>
> I found a problem that if you use cache_name_pos() to do this
> detection, I imagined the following example (I'm trying to imitate an
> output of "git ls-files -t"):
>
> H a
> H b
> S d/file1
> H e/file1
>
> So in this index, I use cache_name_pos() to find a directory "c/". I imagine the
> the value returned would be -3, which indicates this directory would be inserted
> at index position 2. However, the cache entry at position 2 is
> "d/file1", which is
> marked with SKIP_WORKTREE, and this fact cannot guarantee that "c/" is
> a sparse directory, since ''c/" is not in the index per se.
>
> Probably I'm missing something, or I'm just dumb.

> On Mon, Apr 4, 2022 at 3:25 PM Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> wrote:
>> I read and think about this part a few times, but I'm still confused.
>>
>> As Victoria pointed out earlier, and I quote, "Sparse directories *only* appear
>> when you're using a sparse index, so, theoretically, this shouldn't ever
>> come up (and thus isn't testable) until you're using a sparse index."
>> So I'm not so sure what do you mean by putting "full index" and "sparse
>> directory" together.
>>
>> Thus, I go ahead and try to detect a directory that is outside of
>> sparse-checkout cone, without sparse-index enabled.
>>
>> I found a problem that if you use cache_name_pos() to do this
>> detection, I imagined the following example (I'm trying to imitate an
>> output of "git ls-files -t"):
>>
>> H a
>> H b
>> S d/file1
>> H e/file1
>>
>> So in this index, I use cache_name_pos() to find a directory "c/". I imagine the
>> the value returned would be -3, which indicates this directory would be inserted
>> at index position 2. However, the cache entry at position 2 is
>> "d/file1", which is
>> marked with SKIP_WORKTREE, and this fact cannot guarantee that "c/" is
>> a sparse directory, since ''c/" is not in the index per se.

> Originally, the SKIP_WORKTREE bit is not removed when moving an out-of-cone
> file into sparse cone, thus the moved file does not show up in the working tree.
> Hint the user to use "git sparse-checkout reapply" to reapply the sparsity rules
> to the working tree, by which the SKIP_WORKTREE bit is removed.
>
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
> I offered this solution becasue I'm not sure how to turn a cache_entry's 
> ce_flags back to a non-sparse state. I tried directly set it to 0 like this:
>
> 	ce->ce_flags = 0;
>
> But the behavior after this seems undefined. The file still won't show up
> in the working tree.
>
> And I found that "git sparse-checkout reapply" seems to be a nice fit for the
> job. But I guess if there is a way (there must be but I don't know) to do it
> direcly in the code, that could also be nice.
>
>  builtin/mv.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 9da9205e01..5f511fb8da 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -138,6 +138,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 advise_to_reapply = 0;
>  	struct option builtin_mv_options[] = {
>  		OPT__VERBOSE(&verbose, N_("be verbose")),
>  		OPT__DRY_RUN(&show_only, N_("dry run")),
> @@ -383,6 +384,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		pos = cache_name_pos(src, strlen(src));
>  		assert(pos >= 0);
>  		rename_cache_entry_at(pos, dst);
> +		if (!advise_to_reapply &&
> +			!path_in_sparse_checkout(src, &the_index) &&
> +			path_in_sparse_checkout(dst, &the_index)) {
> +				advise_to_reapply = 1;
> +			}

>  	}
>  
>  	if (gitmodules_modified)
> @@ -392,6 +398,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
>  		die(_("Unable to write new index file"));
>  
> +	if (advise_to_reapply)
> +		printf(_("Please use \"git sparse-checkout reapply\" to reapply the sparsity.\n"));
> +

> >       }
> >
> >       if (gitmodules_modified)
> > @@ -392,6 +398,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> >                              COMMIT_LOCK | SKIP_IF_UNCHANGED))
> >               die(_("Unable to write new index file"));
> >
> > +     if (advise_to_reapply)
> > +             printf(_("Please use \"git sparse-checkout reapply\" to reapply the sparsity.\n"));
> > +
>
> Please see 93026558512 (tracking branches: add advice to ambiguous
> refspec error, 2022-03-28) (the OID may change after I send this, as
> it's in "seen") for how to add new advise, i.e. we use advise(), add an
> enum field, config var etc.

> On Thu, Mar 31, 2022 at 6:31 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> More odd indentation, and the braces aren't needed.
>
> Got me again :-( Will make a change.
>
>> >       }
>> >
>> >       if (gitmodules_modified)
>> > @@ -392,6 +398,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>> >                              COMMIT_LOCK | SKIP_IF_UNCHANGED))
>> >               die(_("Unable to write new index file"));
>> >
>> > +     if (advise_to_reapply)
>> > +             printf(_("Please use \"git sparse-checkout reapply\" to reapply the sparsity.\n"));
>> > +
>>
>> Please see 93026558512 (tracking branches: add advice to ambiguous
>> refspec error, 2022-03-28) (the OID may change after I send this, as
>> it's in "seen") for how to add new advise, i.e. we use advise(), add an
>> enum field, config var etc.
>
> I actually did use advise(), but I noticed that it prints to stderr
> and ... nevermind,
> I realized that printing to stderr is OK. But can I print to stdout
> since I think users should
> be "reminded" instead of "warned"?
>
> Anyway, I think using advice() is probably better.

> On Fri, Apr 01 2022, Shaoxuan Yuan wrote:
> > I actually did use advise(), but I noticed that it prints to stderr
> > and ... nevermind,
>
> We've typically used stderr in git not to mean "error", but to
> distinguish "chatty" and non-primary output from non-chatty.
>
> So (leaving aside that we're unlikely to add advice to plumbing) if you
> emitted a warning() or advice from git-ls-tree you should be able to run
> something like:
>
>     git ls-tree -r HEAD >output-for-a-script
>
> And get the advise() on stderr, while the "primary" output is on stdout.
>
> There's a recent-ish (last year or so) thread where I think Jeff King
> explained this better than I'm doing here, but I couldn't find it with a
> quick search.

> Originally, the SKIP_WORKTREE bit is not removed when moving an out-of-cone
> file into sparse cone, thus the moved file does not show up in the working tree.
> Hint the user to use "git sparse-checkout reapply" to reapply the sparsity rules
> to the working tree, by which the SKIP_WORKTREE bit is removed.
> 
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
> I offered this solution becasue I'm not sure how to turn a cache_entry's 
> ce_flags back to a non-sparse state. I tried directly set it to 0 like this:
> 
> 	ce->ce_flags = 0;
> 
> But the behavior after this seems undefined. The file still won't show up
> in the working tree.
> 

>  builtin/mv.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 9da9205e01..5f511fb8da 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -138,6 +138,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 advise_to_reapply = 0;
>  	struct option builtin_mv_options[] = {
>  		OPT__VERBOSE(&verbose, N_("be verbose")),
>  		OPT__DRY_RUN(&show_only, N_("dry run")),
> @@ -383,6 +384,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		pos = cache_name_pos(src, strlen(src));
>  		assert(pos >= 0);
>  		rename_cache_entry_at(pos, dst);
> +		if (!advise_to_reapply &&
> +			!path_in_sparse_checkout(src, &the_index) &&
> +			path_in_sparse_checkout(dst, &the_index)) {
> +				advise_to_reapply = 1;
> +			}
>  	}
>  
>  	if (gitmodules_modified)
> @@ -392,6 +398,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
>  		die(_("Unable to write new index file"));
>  
> +	if (advise_to_reapply)
> +		printf(_("Please use \"git sparse-checkout reapply\" to reapply the sparsity.\n"));

> Originally, the SKIP_WORKTREE bit is not removed when moving an out-of-cone
> file into sparse cone, thus the moved file does not show up in the working tree.
> Hint the user to use "git sparse-checkout reapply" to reapply the sparsity rules
> to the working tree, by which the SKIP_WORKTREE bit is removed.
> 
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
> I offered this solution becasue I'm not sure how to turn a cache_entry's 
> ce_flags back to a non-sparse state. I tried directly set it to 0 like this:
> 
> 	ce->ce_flags = 0;
> 
> But the behavior after this seems undefined. The file still won't show up
> in the working tree.
> 
> And I found that "git sparse-checkout reapply" seems to be a nice fit for the
> job. But I guess if there is a way (there must be but I don't know) to do it
> direcly in the code, that could also be nice.

>  	if (gitmodules_modified)
> @@ -392,6 +398,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
>  		die(_("Unable to write new index file"));
>  
> +	if (advise_to_reapply)
> +		printf(_("Please use \"git sparse-checkout reapply\" to reapply the sparsity.\n"));
> +

> Add corresponding tests to test following situations:
> 
> * 'refuse to move out-of-cone directory without --sparse'
> * 'can move out-of-cone directory with --sparse'
> * 'refuse to move out-of-cone file without --sparse'
> * 'can move out-of-cone file with --sparse'
> 
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>  t/t7002-mv-sparse-checkout.sh | 72 +++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
> index 1d3d2aca21..efb260d015 100755
> --- a/t/t7002-mv-sparse-checkout.sh
> +++ b/t/t7002-mv-sparse-checkout.sh
> @@ -206,4 +206,76 @@ test_expect_success 'refuse to move file to non-skip-worktree sparse path' '
>  	test_cmp expect stderr
>  '
>  
> +test_expect_success 'refuse to move out-of-cone directory without --sparse' '
> +	git sparse-checkout disable &&
> +	git reset --hard &&
> +	mkdir folder1 &&
> +	touch folder1/file1 &&
> +	git add folder1 &&
> +	git sparse-checkout init --cone &&
> +	git sparse-checkout set sub &&
> +
> +	test_must_fail git mv folder1 sub 2>stderr &&
> +	cat sparse_error_header >expect &&
> +	echo folder1/file1 >>expect &&
> +	cat sparse_hint >>expect &&
> +	test_cmp expect stderr
> +'
> +
> +test_expect_success 'can move out-of-cone directory with --sparse' '
> +	git sparse-checkout disable &&
> +	git reset --hard &&
> +	mkdir folder1 &&
> +	touch folder1/file1 &&
> +	git add folder1 &&
> +	git sparse-checkout init --cone &&
> +	git sparse-checkout set sub &&
> +
> +	git mv --sparse folder1 sub 1>actual 2>stderr &&
> +	test_must_be_empty stderr &&
> +	echo "Please use \"git sparse-checkout reapply\" to reapply the sparsity."\
> +	>expect &&
> +	test_cmp actual expect &&
> +
> +	git sparse-checkout reapply &&
> +	test_path_is_dir sub/folder1 &&
> +	test_path_is_file sub/folder1/file1
> +'
> +
> +test_expect_success 'refuse to move out-of-cone file without --sparse' '
> +	git sparse-checkout disable &&
> +	git reset --hard &&
> +	mkdir folder1 &&
> +	touch folder1/file1 &&
> +	git add folder1 &&
> +	git sparse-checkout init --cone &&
> +	git sparse-checkout set sub &&
> +
> +	test_must_fail git mv folder1/file1 sub 2>stderr &&
> +	cat sparse_error_header >expect &&
> +	echo folder1/file1 >>expect &&
> +	cat sparse_hint >>expect &&
> +	test_cmp expect stderr
> +'
> +
> +test_expect_success 'can move out-of-cone file with --sparse' '
> +	git sparse-checkout disable &&
> +	git reset --hard &&
> +	mkdir folder1 &&
> +	touch folder1/file1 &&
> +	git add folder1 &&
> +	git sparse-checkout init --cone &&
> +	git sparse-checkout set sub &&
> +
> +	git mv --sparse folder1/file1 sub 1>actual 2>stderr &&
> +	test_must_be_empty stderr &&
> +	echo "Please use \"git sparse-checkout reapply\" to reapply the sparsity."\
> +	>expect &&
> +	test_cmp actual expect &&
> +
> +	git sparse-checkout reapply &&
> +	! test_path_is_dir sub/folder1 &&
> +	test_path_is_file sub/file1
> +'
> +
>  test_done

>
> 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
>

> 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

> 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!

>
> 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
>

> +test_expect_failure 'refuse to move out-of-cone directory without --sparse' '
> +	git sparse-checkout disable &&
> +	git reset --hard &&
> +	mkdir folder1 &&
> +	touch folder1/file1 &&

> +	git add folder1 &&
> +	git sparse-checkout init --cone &&
> +	git sparse-checkout set sub &&
> +
> +	test_must_fail git mv folder1 sub 2>stderr &&
> +	cat sparse_error_header >expect &&
> +	echo folder1/file1 >>expect &&
> +	cat sparse_hint >>expect &&
> +	test_cmp expect stderr
> +'
> +
> +test_expect_failure 'can move out-of-cone directory with --sparse' '
> +	git sparse-checkout disable &&
> +	git reset --hard &&
> +	mkdir folder1 &&
> +	touch folder1/file1 &&
> +	git add folder1 &&
> +	git sparse-checkout init --cone &&
> +	git sparse-checkout set sub &&
> +
> +	git mv --sparse folder1 sub 1>actual 2>stderr &&

> Add corresponding tests to test following situations:
> 
> * 'refuse to move out-of-cone directory without --sparse'
> * 'can move out-of-cone directory with --sparse'
> * 'refuse to move out-of-cone file without --sparse'
> * 'can move out-of-cone file with --sparse'
> * 'refuse to move sparse file to existing destination'
> * 'move sparse file to existing destination with --force and --sparse'

> Add corresponding tests to test following situations:
> 
> * 'refuse to move out-of-cone directory without --sparse'
> * 'can move out-of-cone directory with --sparse'
> * 'refuse to move out-of-cone file without --sparse'
> * 'can move out-of-cone file with --sparse'
> * 'refuse to move sparse file to existing destination'
> * 'move sparse file to existing destination with --force and --sparse'
> 
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>  t/t7002-mv-sparse-checkout.sh | 98 +++++++++++++++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
> 
> diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
> index 1d3d2aca21..963cb512e2 100755
> --- a/t/t7002-mv-sparse-checkout.sh
> +++ b/t/t7002-mv-sparse-checkout.sh
> @@ -206,4 +206,102 @@ test_expect_success 'refuse to move file to non-skip-worktree sparse path' '
>  	test_cmp expect stderr
>  '
>  

> +test_expect_failure 'refuse to move out-of-cone directory without --sparse' '
> +	git sparse-checkout disable &&
> +	git reset --hard &&
> +	mkdir folder1 &&
> +	touch folder1/file1 &&
> +	git add folder1 &&
> +	git sparse-checkout init --cone &&

> +	test_must_fail git mv folder1 sub 2>stderr &&
> +	cat sparse_error_header >expect &&
> +	echo folder1/file1 >>expect &&
> +	cat sparse_hint >>expect &&
> +	test_cmp expect stderr
> +'
> +
> +test_expect_failure 'can move out-of-cone directory with --sparse' '
> +	git sparse-checkout disable &&
> +	git reset --hard &&
> +	mkdir folder1 &&
> +	touch folder1/file1 &&
> +	git add folder1 &&
> +	git sparse-checkout init --cone &&
> +	git sparse-checkout set sub &&
> +
> +	git mv --sparse folder1 sub 1>actual 2>stderr &&
> +	test_must_be_empty stderr &&
> +
> +	git sparse-checkout reapply &&
> +	test_path_is_dir sub/folder1 &&
> +	test_path_is_file sub/folder1/file1
> +'
> +
> +test_expect_failure 'refuse to move out-of-cone file without --sparse' '
> +	git sparse-checkout disable &&
> +	git reset --hard &&
> +	mkdir folder1 &&
> +	touch folder1/file1 &&
> +	git add folder1 &&
> +	git sparse-checkout init --cone &&
> +	git sparse-checkout set sub &&
> +
> +	test_must_fail git mv folder1/file1 sub 2>stderr &&
> +	cat sparse_error_header >expect &&
> +	echo folder1/file1 >>expect &&
> +	cat sparse_hint >>expect &&
> +	test_cmp expect stderr
> +'
> +
> +test_expect_failure 'can move out-of-cone file with --sparse' '
> +	git sparse-checkout disable &&
> +	git reset --hard &&
> +	mkdir folder1 &&
> +	touch folder1/file1 &&
> +	git add folder1 &&
> +	git sparse-checkout init --cone &&
> +	git sparse-checkout set sub &&
> +
> +	git mv --sparse folder1/file1 sub 1>actual 2>stderr &&
> +	test_must_be_empty stderr &&
> +
> +	git sparse-checkout reapply &&
> +	! test_path_is_dir sub/folder1 &&
> +	test_path_is_file sub/file1
> +'
> +
> +test_expect_failure 'refuse to move sparse file to existing destination' '
> +	git sparse-checkout disable &&
> +	git reset --hard &&
> +	mkdir folder1 &&
> +	touch folder1/file1 &&
> +	touch sub/file1 &&
> +	git add folder1 sub/file1 &&
> +	git sparse-checkout init --cone &&
> +	git sparse-checkout set sub &&
> +
> +	test_must_fail git mv --sparse folder1/file1 sub 2>stderr &&
> +	echo "fatal: destination exists, source=folder1/file1, destination=sub/file1" >expect &&
> +	test_cmp expect stderr
> +'
> +
> +test_expect_failure 'move sparse file to existing destination with --force and --sparse' '
> +	git sparse-checkout disable &&
> +	git reset --hard &&
> +	mkdir folder1 &&
> +	touch folder1/file1 &&
> +	touch sub/file1 &&
> +	echo "overwrite" >folder1/file1 &&
> +	git add folder1 sub/file1 &&
> +	git sparse-checkout init --cone &&
> +	git sparse-checkout set sub &&
> +
> +	git mv --sparse --force folder1/file1 sub 2>stderr &&
> +	test_must_be_empty stderr &&
> +	echo "overwrite" >expect &&
> +	test_cmp expect sub/file1
> +'
> +
>  test_done

> Originally, moving a <source> file which is not on-disk but exists in
> index as a SKIP_WORKTREE enabled cache entry, "giv mv" command errors
> out with "bad source".
> 
> Change the checking logic, so that such <source>
> file 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.
> 
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>  builtin/mv.c                  | 26 +++++++++++++++++++++++++-
>  t/t7002-mv-sparse-checkout.sh |  4 ++--
>  2 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 83a465ba83..32ad4d5682 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -185,8 +185,32 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  
>  		length = strlen(src);
>  		if (lstat(src, &st) < 0) {
> +			/*
> +			 * TODO: for now, when you try to overwrite a <destination>
> +			 * with your <source> as a sparse file, if you supply a "--sparse"
> +			 * flag, then the action will be done without providing "--force"
> +			 * and no warning.
> +			 *
> +			 * This is mainly because the sparse <source>
> +			 * is not on-disk, and this if-else chain will be cut off early in
> +			 * this check, thus the "--force" check is ignored. Need fix.
> +			 */

> +			int pos = cache_name_pos(src, length);
> +			if (pos >= 0) {
> +				const struct cache_entry *ce = active_cache[pos];
> +
> +				if (ce_skip_worktree(ce)) {
> +					if (!ignore_sparse)
> +						string_list_append(&only_match_skip_worktree, src);
> +					else
> +						modes[i] = SPARSE;

> +			}
>  			/* only error if existence is expected. */
> -			if (modes[i] != SPARSE)
> +			else if (modes[i] != SPARSE)
>  				bad = _("bad source");

> 
> 
> On 5/27/2022 6:08 AM, Shaoxuan Yuan wrote:
>> Originally, moving a <source> file which is not on-disk but exists in
>> index as a SKIP_WORKTREE enabled cache entry, "giv mv" command errors
>> out with "bad source".
>>
>> Change the checking logic, so that such <source>
>> file 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.
>>
>> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
>> ---
>>  builtin/mv.c                  | 26 +++++++++++++++++++++++++-
>>  t/t7002-mv-sparse-checkout.sh |  4 ++--
>>  2 files changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/builtin/mv.c b/builtin/mv.c
>> index 83a465ba83..32ad4d5682 100644
>> --- a/builtin/mv.c
>> +++ b/builtin/mv.c
>> @@ -185,8 +185,32 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>  
>>  		length = strlen(src);
>>  		if (lstat(src, &st) < 0) {
>> +			/*
>> +			 * TODO: for now, when you try to overwrite a <destination>
>> +			 * with your <source> as a sparse file, if you supply a "--sparse"
>> +			 * flag, then the action will be done without providing "--force"
>> +			 * and no warning.
>> +			 *
>> +			 * This is mainly because the sparse <source>
>> +			 * is not on-disk, and this if-else chain will be cut off early in
>> +			 * this check, thus the "--force" check is ignored. Need fix.
>> +			 */
> 
> I wonder if this is worth the comment here, or if we'd rather see
> the mention in the commit message. You have documented tests that
> fail in this case, so we already have something that marks this
> as "TODO" in a more discoverable place.
> 
>> +			int pos = cache_name_pos(src, length);
>> +			if (pos >= 0) {
>> +				const struct cache_entry *ce = active_cache[pos];
>> +
>> +				if (ce_skip_worktree(ce)) {
>> +					if (!ignore_sparse)
>> +						string_list_append(&only_match_skip_worktree, src);
>> +					else
>> +						modes[i] = SPARSE;
> 
> 
>> +				}
>> +				else
>> +					bad = _("bad source");
> 
> style nit:
> 
> 	} else {
> 		bad = _("bad source");
> 	}
> 

>> +			}
>>  			/* only error if existence is expected. */
>> -			if (modes[i] != SPARSE)
>> +			else if (modes[i] != SPARSE)
>>  				bad = _("bad source");
> 
> For this one, the comment makes it difficult to connect the 'else
> if' to its corresponding 'if'. Perhaps:
> 
> 	} else if (modes[i] != SPARSE) {
> 		/* only error if existence is expected. */
> 		bad = _("bad source");
> 	}
> 
>>  		} else if (!strncmp(src, dst, length) &&
>>  				(dst[length] == 0 || dst[length] == '/')) {
> 
> In general, I found this if/else-if chain hard to grok, and
> a lot of it is because we have "simple" cases at the end
> and the complicated parts have ever-increasing nesting. This
> is mostly due to the existing if/else-if chain in this method.
> 

> ---- >8 ---
> 
> To me, this is a bit easier to parse, since we find the error
> cases and jump to the action before continuing on the "happy
> path". It does involve that first big refactor first, so I'd
> like to hear opinions of other contributors before you jump to
> taking this suggestion.
> 

> > diff --git a/builtin/mv.c b/builtin/mv.c
> > index 83a465ba83..32ad4d5682 100644
> > --- a/builtin/mv.c
> > +++ b/builtin/mv.c
> > @@ -185,8 +185,32 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> >
> >               length = strlen(src);
> >               if (lstat(src, &st) < 0) {
> > +                     /*
> > +                      * TODO: for now, when you try to overwrite a <destination>
> > +                      * with your <source> as a sparse file, if you supply a "--sparse"
> > +                      * flag, then the action will be done without providing "--force"
> > +                      * and no warning.
> > +                      *
> > +                      * This is mainly because the sparse <source>
> > +                      * is not on-disk, and this if-else chain will be cut off early in
> > +                      * this check, thus the "--force" check is ignored. Need fix.
> > +                      */
>
> I wonder if this is worth the comment here, or if we'd rather see
> the mention in the commit message. You have documented tests that
> fail in this case, so we already have something that marks this
> as "TODO" in a more discoverable place.

> > +                     int pos = cache_name_pos(src, length);
> > +                     if (pos >= 0) {
> > +                             const struct cache_entry *ce = active_cache[pos];
> > +
> > +                             if (ce_skip_worktree(ce)) {
> > +                                     if (!ignore_sparse)
> > +                                             string_list_append(&only_match_skip_worktree, src);
> > +                                     else
> > +                                             modes[i] = SPARSE;
>
>
> > +                             }
> > +                             else
> > +                                     bad = _("bad source");
>
> style nit:
>
>         } else {
>                 bad = _("bad source");
>         }
>
> > +                     }
> >                       /* only error if existence is expected. */
> > -                     if (modes[i] != SPARSE)
> > +                     else if (modes[i] != SPARSE)
> >                               bad = _("bad source");
>
> For this one, the comment makes it difficult to connect the 'else
> if' to its corresponding 'if'. Perhaps:
>
>         } else if (modes[i] != SPARSE) {
>                 /* only error if existence is expected. */
>                 bad = _("bad source");
>         }
>
> >               } else if (!strncmp(src, dst, length) &&
> >                               (dst[length] == 0 || dst[length] == '/')) {
>
> In general, I found this if/else-if chain hard to grok, and
> a lot of it is because we have "simple" cases at the end
> and the complicated parts have ever-increasing nesting. This
> is mostly due to the existing if/else-if chain in this method.
>
> Here is a diff that replaces that if/else-if chain with a
> 'goto' trick to jump ahead, allowing some code to decrease in
> tabbing:
>
> ---- >8 ----

> ---- >8 ----
>
> But mostly the reason for this refactor is that the following
> diff should be equivalent to yours:
>
> ---- >8 ----
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index d8b5c24fb5..add48e23b4 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -185,11 +185,28 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>
>                 length = strlen(src);
>                 if (lstat(src, &st) < 0) {
> -                       /* only error if existence is expected. */
> -                       if (modes[i] != SPARSE) {
> +                       int pos;
> +                       const struct cache_entry *ce;
> +
> +                       pos = cache_name_pos(src, length);
> +                       if (pos < 0) {
> +                               /* only error if existence is expected. */
> +                               if (modes[i] != SPARSE)
> +                                       bad = _("bad source");
> +                               goto act_on_entry;
> +                       }
> +
> +                       ce = active_cache[pos];
> +                       if (!ce_skip_worktree(ce)) {
>                                 bad = _("bad source");
>                                 goto act_on_entry;
>                         }
> +
> +                       if (!ignore_sparse)
> +                               string_list_append(&only_match_skip_worktree, src);
> +                       else
> +                               modes[i] = SPARSE;
> +                       goto act_on_entry;
>                 }
>                 if (!strncmp(src, dst, length) &&
>                     (dst[length] == 0 || dst[length] == '/')) {
> ---- >8 ---
>
> To me, this is a bit easier to parse, since we find the error
> cases and jump to the action before continuing on the "happy
> path". It does involve that first big refactor first, so I'd
> like to hear opinions of other contributors before you jump to
> taking this suggestion.

> Originally, moving a sparse file into cone can result in unwarned
> overwrite of existing entry. The expected behavior is that if the
> <destination> exists in the entry, user should be prompted to supply
> a [-f|--force] to carry out the operation, or the operation should
> fail.
> 
> Add a check mechanism to do that.
> 
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>  builtin/mv.c                  | 23 +++++++++++------------
>  t/t7002-mv-sparse-checkout.sh |  2 +-
>  2 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 32ad4d5682..62284e3f86 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -185,16 +185,6 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  
>  		length = strlen(src);
>  		if (lstat(src, &st) < 0) {
> -			/*
> -			 * TODO: for now, when you try to overwrite a <destination>
> -			 * with your <source> as a sparse file, if you supply a "--sparse"
> -			 * flag, then the action will be done without providing "--force"
> -			 * and no warning.
> -			 *
> -			 * This is mainly because the sparse <source>
> -			 * is not on-disk, and this if-else chain will be cut off early in
> -			 * this check, thus the "--force" check is ignored. Need fix.
> -			 */
>  

>  			int pos = cache_name_pos(src, length);
>  			if (pos >= 0) {
> @@ -203,8 +193,17 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  				if (ce_skip_worktree(ce)) {
>  					if (!ignore_sparse)
>  						string_list_append(&only_match_skip_worktree, src);
> -					else
> -						modes[i] = SPARSE;
> +					else {
> +						/* Check if dst exists in index */
> +						if (cache_name_pos(dst, strlen(dst)) >= 0) {
> +							if (force)
> +								modes[i] = SPARSE;
> +							else
> +								bad = _("destination exists");
> +						}
> +						else
> +							modes[i] = SPARSE;
> +					}
>  				}
>  				else
>  					bad = _("bad source");
> diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
> index 581ef4c0f6..2c9008573a 100755
> --- a/t/t7002-mv-sparse-checkout.sh
> +++ b/t/t7002-mv-sparse-checkout.sh
> @@ -272,7 +272,7 @@ test_expect_success 'can move out-of-cone file with --sparse' '
>  	test_path_is_file sub/file1
>  '
>  
> -test_expect_failure 'refuse to move sparse file to existing destination' '
> +test_expect_success 'refuse to move sparse file to existing destination' '
>  	git sparse-checkout disable &&
>  	git reset --hard &&
>  	mkdir folder1 &&

> +/*
> + * 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.
> + */
> +static int check_dir_in_index(const char *name, int namelen)
> +{
> +	int ret = 1;
> +	const char *with_slash = add_slash(name);
> +	int length = namelen + 1;
> +
> +	int pos = cache_name_pos(with_slash, length);
> +	const struct cache_entry *ce;
> +
> +	if (pos < 0) {
> +		pos = -pos - 1;
> +		if (pos >= the_index.cache_nr)
> +			return ret;
> +		ce = active_cache[pos];
> +		if (strncmp(with_slash, ce->name, length))
> +			return ret;
> +		if (ce_skip_worktree(ce))
> +			return ret = 0;

> +			}
>  			/* only error if existence is expected. */
>  			else if (modes[i] != SPARSE)
>  				bad = _("bad source");
> @@ -218,7 +264,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  				&& lstat(dst, &st) == 0)
>  			bad = _("cannot move directory over file");
>  		else if (src_is_dir) {
> -			int first = cache_name_pos(src, length), last;
> +			int first, last;
> +dir_check:
> +			first = cache_name_pos(src, length);
>  
>  			if (first >= 0)
>  				prepare_move_submodule(src, first,
> @@ -229,7 +277,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			else { /* last - first >= 1 */
>  				int j, dst_len, n;
>  
> -				modes[i] = WORKING_DIRECTORY;
> +				if (!modes[i])
> +					modes[i] |= WORKING_DIRECTORY;

>  				n = argc + last - first;
>  				REALLOC_ARRAY(source, n);
>  				REALLOC_ARRAY(destination, n);
> @@ -331,7 +380,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			printf(_("Renaming %s to %s\n"), src, dst);
>  		if (show_only)
>  			continue;
> -		if (mode != INDEX && mode != SPARSE && rename(src, dst) < 0) {
> +		if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
> +		 	rename(src, dst) < 0) {

>  			if (ignore_errors)
>  				continue;
>  			die_errno(_("renaming '%s' failed"), src);
> @@ -345,7 +395,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  							      1);
>  		}
>  
> -		if (mode == WORKING_DIRECTORY)
> +		if (mode & (WORKING_DIRECTORY | SKIP_WORKTREE_DIR))
>  			continue;

>
> On 5/27/2022 6:08 AM, Shaoxuan Yuan wrote:
> > +/*
> > + * 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.
> > + */
> > +static int check_dir_in_index(const char *name, int namelen)
> > +{
> > +     int ret = 1;
> > +     const char *with_slash = add_slash(name);
> > +     int length = namelen + 1;
> > +
> > +     int pos = cache_name_pos(with_slash, length);
> > +     const struct cache_entry *ce;
> > +
> > +     if (pos < 0) {
> > +             pos = -pos - 1;
> > +             if (pos >= the_index.cache_nr)
> > +                     return ret;
> > +             ce = active_cache[pos];
> > +             if (strncmp(with_slash, ce->name, length))
> > +                     return ret;
> > +             if (ce_skip_worktree(ce))
> > +                     return ret = 0;
>
> This appears to check if the _first_ entry under the directory
> is sparse, but not if _all_ entries are sparse. These are not
> the same thing, even in cone-mode sparse-checkout. The t1092
> test directory has files like "folder1/0/0/a" but if
> "folder1/1" is in the sparse-checkout cone, then that first
> entry has the skip-worktree bit, but "folder1/1/a" and "folder1/a"
> do not.

> > +     }
> > +     return ret;
>
> At the moment, it doesn't seem like we need 'ret' since the
> only place you set it is in "return ret = 0;" (which could
> just be "return 0;" while the others are "return 1;"). But,
> perhaps you intended to create a loop over 'pos' while
> with_slash is a prefix of the cache entry?

> > +                     else if (!check_dir_in_index(src, length) &&
> > +                                      !path_in_sparse_checkout(src_w_slash, &the_index)) {
>
> style-nit: You'll want to align the different parts of your
> logical statement to agree with the end of the "else if (",
>
>         else if (A &&
>                  B) {
>

> > +                             modes[i] = SKIP_WORKTREE_DIR;
>
> If we are moving to a flags-based model, should we convert all
> "modes[i] =" to "modes[i] |=" as a first step (before adding the
> SKIP_WORTKREE_DIR flag)?
>
> > +                             goto dir_check;
>
> Hm. While I did recommend using 'goto' to jump to a common end
> place in the loop body, I'm not sure about jumping into another
> else-if statement. This might be a good time to extract the
> code from "else if (src_is_dir)" below into a helper method that
> can be used in both places.

> > +                     }
> >                       /* only error if existence is expected. */
> >                       else if (modes[i] != SPARSE)
> >                               bad = _("bad source");
> > @@ -218,7 +264,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> >                               && lstat(dst, &st) == 0)
> >                       bad = _("cannot move directory over file");
> >               else if (src_is_dir) {
> > -                     int first = cache_name_pos(src, length), last;
> > +                     int first, last;
> > +dir_check:
> > +                     first = cache_name_pos(src, length);
> >
> >                       if (first >= 0)
> >                               prepare_move_submodule(src, first,
> > @@ -229,7 +277,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> >                       else { /* last - first >= 1 */
> >                               int j, dst_len, n;
> >
> > -                             modes[i] = WORKING_DIRECTORY;
> > +                             if (!modes[i])
> > +                                     modes[i] |= WORKING_DIRECTORY;
>
> This appears to only add the WORKING_DIRECTORY flag if modes[i] is
> already zero. This maybe implies that we wouldn't understand
> "WORKING_DIRECTORY | SKIP_WORKTREE_DIR" as a value.

> >                               n = argc + last - first;
> >                               REALLOC_ARRAY(source, n);
> >                               REALLOC_ARRAY(destination, n);
> > @@ -331,7 +380,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> >                       printf(_("Renaming %s to %s\n"), src, dst);
> >               if (show_only)
> >                       continue;
> > -             if (mode != INDEX && mode != SPARSE && rename(src, dst) < 0) {
> > +             if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
> > +                     rename(src, dst) < 0) {
>
> style-nit: align your logical statements.
>
> >                       if (ignore_errors)
> >                               continue;
> >                       die_errno(_("renaming '%s' failed"), src);
> > @@ -345,7 +395,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> >                                                             1);
> >               }
> >
> > -             if (mode == WORKING_DIRECTORY)
> > +             if (mode & (WORKING_DIRECTORY | SKIP_WORKTREE_DIR))
> >                       continue;
>
> Ok, here you check if _either_ mode is enabled, which is good. Maybe
> you don't need the "if (!mode[i])" part above.
>
> Thanks,
> -Stolee

>> This appears to check if the _first_ entry under the directory
>> is sparse, but not if _all_ entries are sparse. These are not
>> the same thing, even in cone-mode sparse-checkout. The t1092
>> test directory has files like "folder1/0/0/a" but if
>> "folder1/1" is in the sparse-checkout cone, then that first
>> entry has the skip-worktree bit, but "folder1/1/a" and "folder1/a"
>> do not.
> 
> Yes, it is checking the first entry and this would not work without the
> lstat in the front. But I think the "lstat < 0" makes sure that this directory
> cannot be partially sparsified.
> 
> It is either missing both in the worktree and index, or missing in the worktree
> but present in index (with all its content sparsified). And because of that,
> I think only the first entry needs to be checked.

>>> +     }
>>> +     return ret;
>>
>> At the moment, it doesn't seem like we need 'ret' since the
>> only place you set it is in "return ret = 0;" (which could
>> just be "return 0;" while the others are "return 1;"). But,
>> perhaps you intended to create a loop over 'pos' while
>> with_slash is a prefix of the cache entry?
> 
> I agree that this variable is redundant. But I fail to understand
> the logical relation between before "But," and after "But,". Please
> elaborate on that?

>>> +                     else if (!check_dir_in_index(src, length) &&
>>> +                                      !path_in_sparse_checkout(src_w_slash, &the_index)) {
>>
>> style-nit: You'll want to align the different parts of your
>> logical statement to agree with the end of the "else if (",
>>
>>         else if (A &&
>>                  B) {
>>
> 
> This one is interesting because it appears just alright in my VSCode editor.
> Later I found that it is because git-diff is using a tab size of 8 or something,
> but my VSCode uses tab size of 4. After I configured the git-diff tab rendering
> size, it looks alright. Same for another style nit down below.

> +	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;

> 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 &&

> 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]).

> 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.
> 

> > 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>

> Add corresponding tests to test following situations:
> 
> We do not have sufficient coverage of moving files outside
> of a sparse-checkout cone. Create new tests covering this
> behavior, keeping in mind that the user can include --sparse
> (or not), move a file or directory, and the destination can
> already exist in the index (in this case user can use --force
> to overwrite existing entry).
> 
> Helped-by: Victoria Dye <vdye@github.com>
> Helped-by: Derrick Stolee <derrickstolee@github.com>
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>  t/t7002-mv-sparse-checkout.sh | 87 +++++++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
> 
> diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
> index f0f7cbfcdb..d6e7315a5a 100755
> --- a/t/t7002-mv-sparse-checkout.sh
> +++ b/t/t7002-mv-sparse-checkout.sh
> @@ -4,6 +4,18 @@ test_description='git mv in sparse working trees'
>  
>  . ./test-lib.sh
>  
> +setup_sparse_checkout () {
> +	mkdir folder1 &&
> +	touch folder1/file1 &&
> +	git add folder1 &&
> +	git sparse-checkout set --cone sub
> +}
> +
> +cleanup_sparse_checkout () {
> +	git sparse-checkout disable &&
> +	git reset --hard
> +}
> +
>  test_expect_success 'setup' "
>  	mkdir -p sub/dir sub/dir2 &&
>  	touch a b c sub/d sub/dir/e sub/dir2/e &&
> @@ -196,6 +208,7 @@ test_expect_success 'can move files to non-sparse dir' '
>  '
>  
>  test_expect_success 'refuse to move file to non-skip-worktree sparse path' '
> +	test_when_finished "cleanup_sparse_checkout" &&
>  	git reset --hard &&
>  	git sparse-checkout init --no-cone &&
>  	git sparse-checkout set a !/x y/ !x/y/z &&
> @@ -206,4 +219,78 @@ test_expect_success 'refuse to move file to non-skip-worktree sparse path' '
>  	test_cmp expect stderr
>  '
>  
> +test_expect_failure 'refuse to move out-of-cone directory without --sparse' '
> +	test_when_finished "cleanup_sparse_checkout" &&
> +	setup_sparse_checkout &&

> +
> +	test_must_fail git mv folder1 sub 2>stderr &&
> +	cat sparse_error_header >expect &&
> +	echo folder1/file1 >>expect &&
> +	cat sparse_hint >>expect &&
> +	test_cmp expect stderr
> +'
> +
> +test_expect_failure 'can move out-of-cone directory with --sparse' '
> +	test_when_finished "cleanup_sparse_checkout" &&
> +	setup_sparse_checkout &&
> +
> +	git mv --sparse folder1 sub 1>actual 2>stderr &&
> +	test_must_be_empty stderr &&
> +
> +	git sparse-checkout reapply &&

> +	test_path_is_dir sub/folder1 &&
> +	test_path_is_file sub/folder1/file1
> +'
> +
> +test_expect_failure 'refuse to move out-of-cone file without --sparse' '
> +	test_when_finished "cleanup_sparse_checkout" &&
> +	setup_sparse_checkout &&
> +
> +	test_must_fail git mv folder1/file1 sub 2>stderr &&
> +	cat sparse_error_header >expect &&
> +	echo folder1/file1 >>expect &&
> +	cat sparse_hint >>expect &&
> +	test_cmp expect stderr
> +'
> +
> +test_expect_failure 'can move out-of-cone file with --sparse' '
> +	test_when_finished "cleanup_sparse_checkout" &&
> +	setup_sparse_checkout &&
> +
> +	git mv --sparse folder1/file1 sub 1>actual 2>stderr &&
> +	test_must_be_empty stderr &&
> +
> +	git sparse-checkout reapply &&
> +	! test_path_is_dir sub/folder1 &&
> +	test_path_is_file sub/file1

> +'
> +
> +test_expect_failure 'refuse to move sparse file to existing destination' '
> +	test_when_finished "cleanup_sparse_checkout" &&
> +	mkdir folder1 &&
> +	touch folder1/file1 &&
> +	touch sub/file1 &&
> +	git add folder1 sub/file1 &&
> +	git sparse-checkout set --cone sub &&
> +
> +	test_must_fail git mv --sparse folder1/file1 sub 2>stderr &&
> +	echo "fatal: destination exists, source=folder1/file1, destination=sub/file1" >expect &&
> +	test_cmp expect stderr
> +'
> +
> +test_expect_failure 'move sparse file to existing destination with --force and --sparse' '
> +	test_when_finished "cleanup_sparse_checkout" &&
> +	mkdir folder1 &&
> +	touch folder1/file1 &&
> +	touch sub/file1 &&
> +	echo "overwrite" >folder1/file1 &&
> +	git add folder1 sub/file1 &&
> +	git sparse-checkout set --cone sub &&
> +
> +	git mv --sparse --force folder1/file1 sub 2>stderr &&
> +	test_must_be_empty stderr &&
> +	echo "overwrite" >expect &&
> +	test_cmp expect sub/file1
> +'
> +
>  test_done

> As suggested by Derrick [1],
> move the in-line definition of "enum update_mode" to the top
> of the file and make it use "flags" mode (each state is a different
> bit in the word).
> 

> [1] https://lore.kernel.org/git/22aadea2-9330-aa9e-7b6a-834585189144@github.com/
> 
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>  builtin/mv.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index abb90d3266..7ce7992d6c 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -19,6 +19,14 @@ static const char * const builtin_mv_usage[] = {
>  	NULL
>  };
>  
> +enum update_mode {
> +	BOTH = 0,

> +
>  #define DUP_BASENAME 1
>  #define KEEP_TRAILING_SLASH 2
>  
> @@ -129,7 +137,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		OPT_END(),
>  	};
>  	const char **source, **destination, **dest_path, **submodule_gitfile;
> -	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX, SPARSE } *modes;
> +	enum update_mode *modes;
>  	struct stat st;
>  	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
>  	struct lock_file lock_file = LOCK_INIT;
> @@ -191,7 +199,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			pos = cache_name_pos(src, length);
>  			if (pos < 0) {
>  				/* only error if existence is expected. */
> -				if (modes[i] != SPARSE)
> +				if (!(modes[i] & SPARSE))
>  					bad = _("bad source");
>  				goto act_on_entry;
>  			}
> @@ -207,14 +215,14 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			}
>  			/* Check if dst exists in index */
>  			if (cache_name_pos(dst, strlen(dst)) < 0) {
> -				modes[i] = SPARSE;
> +				modes[i] |= SPARSE;
>  				goto act_on_entry;
>  			}
>  			if (!force) {
>  				bad = _("destination exists");
>  				goto act_on_entry;
>  			}
> -			modes[i] = SPARSE;
> +			modes[i] |= SPARSE;
>  			goto act_on_entry;
>  		}
>  		if (!strncmp(src, dst, length) &&
> @@ -242,7 +250,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			}
>  
>  			/* last - first >= 1 */
> -			modes[i] = WORKING_DIRECTORY;
> +			modes[i] |= WORKING_DIRECTORY;
>  			n = argc + last - first;
>  			REALLOC_ARRAY(source, n);
>  			REALLOC_ARRAY(destination, n);
> @@ -258,7 +266,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  				source[argc + j] = path;
>  				destination[argc + j] =
>  					prefix_path(dst, dst_len, path + length + 1);
> -				modes[argc + j] = ce_skip_worktree(ce) ? SPARSE : INDEX;
> +				memset(modes + argc + j, 0, sizeof(enum update_mode));

> +				modes[argc + j] |= ce_skip_worktree(ce) ? SPARSE : INDEX;
>  				submodule_gitfile[argc + j] = NULL;
>  			}
>  			argc += last - first;
> @@ -355,7 +364,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			printf(_("Renaming %s to %s\n"), src, dst);
>  		if (show_only)
>  			continue;
> -		if (mode != INDEX && mode != SPARSE && rename(src, dst) < 0) {
> +		if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
> +			rename(src, dst) < 0) {

>  			if (ignore_errors)
>  				continue;
>  			die_errno(_("renaming '%s' failed"), src);
> @@ -369,7 +379,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  							      1);
>  		}
>  
> -		if (mode == WORKING_DIRECTORY)
> +		if (mode & (WORKING_DIRECTORY | SKIP_WORKTREE_DIR))
>  			continue;
>  
>  		pos = cache_name_pos(src, strlen(src));

> Shaoxuan Yuan wrote:
>> As suggested by Derrick [1],
>> move the in-line definition of "enum update_mode" to the top
>> of the file and make it use "flags" mode (each state is a different
>> bit in the word).
>>
> This message doesn't quite cover all of what's done in the commit. In
> addition to moving the enum definition, you introduce a 'SKIP_WORKTREE_DIR'
> flag and change the flag assignments to '|=' (additive) from '=' (single
> assignment). If those changes belong in this commit (not a later one), they
> should be explained in the message here.

>> [1] https://lore.kernel.org/git/22aadea2-9330-aa9e-7b6a-834585189144@github.com/
>>
>> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
>> ---
>>   builtin/mv.c | 26 ++++++++++++++++++--------
>>   1 file changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/builtin/mv.c b/builtin/mv.c
>> index abb90d3266..7ce7992d6c 100644
>> --- a/builtin/mv.c
>> +++ b/builtin/mv.c
>> @@ -19,6 +19,14 @@ static const char * const builtin_mv_usage[] = {
>>   	NULL
>>   };
>>   
>> +enum update_mode {
>> +	BOTH = 0,
> I know this comes from the original inline enum, but I don't see 'BOTH' used
> anywhere. The name itself is somewhat confusing (I have no idea what "both"
> is referring to - possibly "both" 'WORKING_DIRECTORY' and 'INDEX'??), so
> would you mind removing it in the next re-roll?

>> +	WORKING_DIRECTORY = (1 << 1),
>> +	INDEX = (1 << 2),
>> +	SPARSE = (1 << 3),
>> +	SKIP_WORKTREE_DIR = (1 << 4),
> You're not introducing any assignment of 'SKIP_WORKTREE_DIR' in this commit
> (looks like that's done in the next one, patch [6/7]), so you should
> probably 'SKIP_WORKTREE_DIR' and its corresponding usage in that patch
> instead of this one.

>> +};
> When the update modes were mutually-exclusive, it made sense for them to be
> represented by an enum. Now that they're flags that can be combined, should
> they instead be pre-processor '#define' values (e.g., like the 'RESET_*'
> modes in 'reset.h' or 'CE_*' flags in 'cache.h')? I don't actually know what
> the standard is, since I also see one or two examples of using enums as
> flags (e.g., 'commit_graph_split_flags' in 'commit-graph.h'). Maybe another
> contributor could clarify?

>> +
>>   #define DUP_BASENAME 1
>>   #define KEEP_TRAILING_SLASH 2
>>   
>> @@ -129,7 +137,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>   		OPT_END(),
>>   	};
>>   	const char **source, **destination, **dest_path, **submodule_gitfile;
>> -	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX, SPARSE } *modes;
>> +	enum update_mode *modes;
>>   	struct stat st;
>>   	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
>>   	struct lock_file lock_file = LOCK_INIT;
>> @@ -191,7 +199,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>   			pos = cache_name_pos(src, length);
>>   			if (pos < 0) {
>>   				/* only error if existence is expected. */
>> -				if (modes[i] != SPARSE)
>> +				if (!(modes[i] & SPARSE))
>>   					bad = _("bad source");
>>   				goto act_on_entry;
>>   			}
>> @@ -207,14 +215,14 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>   			}
>>   			/* Check if dst exists in index */
>>   			if (cache_name_pos(dst, strlen(dst)) < 0) {
>> -				modes[i] = SPARSE;
>> +				modes[i] |= SPARSE;
>>   				goto act_on_entry;
>>   			}
>>   			if (!force) {
>>   				bad = _("destination exists");
>>   				goto act_on_entry;
>>   			}
>> -			modes[i] = SPARSE;
>> +			modes[i] |= SPARSE;
>>   			goto act_on_entry;
>>   		}
>>   		if (!strncmp(src, dst, length) &&
>> @@ -242,7 +250,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>   			}
>>   
>>   			/* last - first >= 1 */
>> -			modes[i] = WORKING_DIRECTORY;
>> +			modes[i] |= WORKING_DIRECTORY;
>>   			n = argc + last - first;
>>   			REALLOC_ARRAY(source, n);
>>   			REALLOC_ARRAY(destination, n);
>> @@ -258,7 +266,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>   				source[argc + j] = path;
>>   				destination[argc + j] =
>>   					prefix_path(dst, dst_len, path + length + 1);
>> -				modes[argc + j] = ce_skip_worktree(ce) ? SPARSE : INDEX;
>> +				memset(modes + argc + j, 0, sizeof(enum update_mode));
> One benefit of using '#define' values would be that 'modes' would just be an
> array of unsigned ints, so you could just assign '0' rather than using
> memset. In terms of the implementation as-is, though, I think what you have
> is correct.
>
>> +				modes[argc + j] |= ce_skip_worktree(ce) ? SPARSE : INDEX;
>>   				submodule_gitfile[argc + j] = NULL;
>>   			}
>>   			argc += last - first;
>> @@ -355,7 +364,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>   			printf(_("Renaming %s to %s\n"), src, dst);
>>   		if (show_only)
>>   			continue;
>> -		if (mode != INDEX && mode != SPARSE && rename(src, dst) < 0) {
>> +		if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
>> +			rename(src, dst) < 0) {
> Nit: could you align 'rename' with the line above it (per the highlighted
> section in the CodingGuidelines [1])? As far as I can tell, the "align with
> tabs and spaces" approach is what's *intended* to be used in 'mv.c'
> (although it's admittedly pretty inconsistent).
>
> [1] https://github.com/git/git/blob/master/Documentation/CodingGuidelines#L371-L383

>>   			if (ignore_errors)
>>   				continue;
>>   			die_errno(_("renaming '%s' failed"), src);
>> @@ -369,7 +379,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>   							      1);
>>   		}
>>   
>> -		if (mode == WORKING_DIRECTORY)
>> +		if (mode & (WORKING_DIRECTORY | SKIP_WORKTREE_DIR))
>>   			continue;
>>   
>>   		pos = cache_name_pos(src, strlen(src));

> 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.
> 
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>  builtin/mv.c                  | 49 +++++++++++++++++++++++++++++++----
>  t/t7002-mv-sparse-checkout.sh |  4 +--
>  2 files changed, 46 insertions(+), 7 deletions(-)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 7ce7992d6c..cb3441c7cb 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -123,6 +123,37 @@ 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.
> + */

> +static int check_dir_in_index(const char *name, int namelen)
> +{
> +	int ret = 1;
> +	const char *with_slash = add_slash(name);
> +	int length = namelen + 1;
> +
> +	int pos = cache_name_pos(with_slash, length);
> +	const struct cache_entry *ce;
> +
> +	if (pos < 0) {
> +		pos = -pos - 1;
> +		if (pos >= the_index.cache_nr)
> +			return ret;
> +		ce = active_cache[pos];
> +		if (strncmp(with_slash, ce->name, length))
> +			return ret;
> +		if (ce_skip_worktree(ce))
> +			return ret = 0;
> +	}
> +	return ret;

> +}
> +
>  int cmd_mv(int argc, const char **argv, const char *prefix)
>  {
>  	int i, flags, gitmodules_modified = 0;
> @@ -184,7 +215,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  	/* Checking */
>  	for (i = 0; i < argc; i++) {
>  		const char *src = source[i], *dst = destination[i];
> -		int length, src_is_dir;
> +		int length;
>  		const char *bad = NULL;
>  		int skip_sparse = 0;
>  
> @@ -198,12 +229,17 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  
>  			pos = cache_name_pos(src, length);
>  			if (pos < 0) {
> +				const char *src_w_slash = add_slash(src);
> +				if (!check_dir_in_index(src, length) &&
> +					!path_in_sparse_checkout(src_w_slash, &the_index)) {

> +					modes[i] |= SKIP_WORKTREE_DIR;
> +					goto dir_check;
> +				}
>  				/* only error if existence is expected. */
>  				if (!(modes[i] & SPARSE))
>  					bad = _("bad source");
>  				goto act_on_entry;
>  			}
> -
>  			ce = active_cache[pos];
>  			if (!ce_skip_worktree(ce)) {
>  				bad = _("bad source");
> @@ -230,14 +266,17 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			bad = _("can not move directory into itself");
>  			goto act_on_entry;
>  		}
> -		if ((src_is_dir = S_ISDIR(st.st_mode))
> +		if (S_ISDIR(st.st_mode)
>  		    && lstat(dst, &st) == 0) {
>  			bad = _("cannot move directory over file");
>  			goto act_on_entry;
>  		}
> -		if (src_is_dir) {
> +
> +dir_check:
> +		if (S_ISDIR(st.st_mode)) {
>  			int j, dst_len, n;
> -			int first = cache_name_pos(src, length), last;
> +			int first, last;
> +			first = cache_name_pos(src, length);

>  
>  			if (first >= 0) {
>  				prepare_move_submodule(src, first,
> diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
> index 5b61fbad5f..30e13b9979 100755
> --- a/t/t7002-mv-sparse-checkout.sh
> +++ b/t/t7002-mv-sparse-checkout.sh
> @@ -219,7 +219,7 @@ test_expect_success 'refuse to move file to non-skip-worktree sparse path' '
>  	test_cmp expect stderr
>  '
>  
> -test_expect_failure 'refuse to move out-of-cone directory without --sparse' '
> +test_expect_success 'refuse to move out-of-cone directory without --sparse' '
>  	test_when_finished "cleanup_sparse_checkout" &&
>  	setup_sparse_checkout &&
>  
> @@ -230,7 +230,7 @@ test_expect_failure 'refuse to move out-of-cone directory without --sparse' '
>  	test_cmp expect stderr
>  '
>  
> -test_expect_failure 'can move out-of-cone directory with --sparse' '
> +test_expect_success 'can move out-of-cone directory with --sparse' '
>  	test_when_finished "cleanup_sparse_checkout" &&
>  	setup_sparse_checkout &&
>  

> Originally, "git mv" a sparse file from out-of-cone to
> in-cone does not update the moved file's sparsity (remove its
> SKIP_WORKTREE bit). And the corresponding cache entry is, unexpectedly,
> not checked out in the working tree.
> 
> Update the behavior so that:
> 1. Moving from out-of-cone to in-cone removes the SKIP_WORKTREE bit from
>    corresponding cache entry.
> 2. The moved cache entry is checked out in the working tree to reflect
>    the updated sparsity.
> 
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>  builtin/mv.c                  | 18 ++++++++++++++++++
>  t/t7002-mv-sparse-checkout.sh |  4 +---
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index cb3441c7cb..a8b9f55654 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 "entry.h"
>  
>  static const char * const builtin_mv_usage[] = {
>  	N_("git mv [<options>] <source>... <destination>"),
> @@ -399,6 +400,11 @@ 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;
> +		struct checkout state = CHECKOUT_INIT;
> +		state.istate = &the_index;
> +
> +		if (force)
> +			state.force = 1;
>  		if (show_only || verbose)
>  			printf(_("Renaming %s to %s\n"), src, dst);
>  		if (show_only)
> @@ -424,6 +430,18 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		pos = cache_name_pos(src, strlen(src));
>  		assert(pos >= 0);
>  		rename_cache_entry_at(pos, dst);

> +				int dst_pos;
> +
> +				dst_pos = cache_name_pos(dst, strlen(dst));
> +				active_cache[dst_pos]->ce_flags &= ~CE_SKIP_WORKTREE;
> +
> +				if (checkout_entry(active_cache[dst_pos], &state, NULL, NULL))
> +					die(_("cannot checkout %s"), ce->name);
> +			}
> +		}
>  	}
>  
>  	if (gitmodules_modified)
> diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
> index 30e13b9979..7734119197 100755
> --- a/t/t7002-mv-sparse-checkout.sh
> +++ b/t/t7002-mv-sparse-checkout.sh
> @@ -237,7 +237,6 @@ test_expect_success 'can move out-of-cone directory with --sparse' '
>  	git mv --sparse folder1 sub 1>actual 2>stderr &&
>  	test_must_be_empty stderr &&
>  
> -	git sparse-checkout reapply &&
>  	test_path_is_dir sub/folder1 &&
>  	test_path_is_file sub/folder1/file1
>  '
> @@ -260,7 +259,6 @@ test_expect_success 'can move out-of-cone file with --sparse' '
>  	git mv --sparse folder1/file1 sub 1>actual 2>stderr &&
>  	test_must_be_empty stderr &&
>  
> -	git sparse-checkout reapply &&
>  	! test_path_is_dir sub/folder1 &&
>  	test_path_is_file sub/file1
>  '
> @@ -278,7 +276,7 @@ test_expect_success 'refuse to move sparse file to existing destination' '
>  	test_cmp expect stderr
>  '
>  
> -test_expect_failure 'move sparse file to existing destination with --force and --sparse' '
> +test_expect_success 'move sparse file to existing destination with --force and --sparse' '
>  	test_when_finished "cleanup_sparse_checkout" &&
>  	mkdir folder1 &&
>  	touch folder1/file1 &&

> The range-diff seems a bit messy, because some of the changes are too big
> then I have to tune the --create-factor big enough to reveal these changes.
> 
> ## Changes since WIP v2 ##
> 
> 1. Write helper functions for t7002 to reuse some code.
> 
> 2. Refactor/decouple the if/else-if checking chain.
> 
> 3. Separate out the 'update_mode' refactor into a single commit.
> 
> 4. Stop using update_sparsity() and instead update the SKIP_WORKTREE
>    bit for each cache_entry and check it out to the working tree.

> 
> ## Limitations ##
> 
> At this point, we still don't have in-cone to out-of-cone move, which
> I don't think is too much a problem, since the title says this series is
> around out-of-cone as the <source>.
> 

> But I think it worth discuss if we should implement in-cone to 
> out-of-cone move, since it will be nice (naturally) to have it working.
> 
> However, I noticed this from the mv man page:
> 
> "In the second form, the last argument has to be an existing directory; 
> the given sources will be moved into this directory."
> 
> I think trying to move out-of-cone, the last argument has to be an non-existent
> directory? I'm a bit confused: should we update some of mv basic logic to 
> accomplish this?
> 

> Shaoxuan Yuan (7):
>   t7002: add tests for moving out-of-cone file/directory
>   mv: decouple if/else-if checks using goto
>   mv: check if out-of-cone file exists in index with SKIP_WORKTREE bit
>   mv: check if <destination> exists in index to handle overwriting
>   mv: use flags mode for update_mode
>   mv: add check_dir_in_index() and solve general dir check issue
>   mv: update sparsity after moving from out-of-cone to in-cone
> 
>  builtin/mv.c                  | 244 +++++++++++++++++++++++++---------
>  t/t7002-mv-sparse-checkout.sh |  85 ++++++++++++
>  2 files changed, 264 insertions(+), 65 deletions(-)
> 
> Range-diff against v2:
> 1:  271445205d ! 1:  a08ce96935 t7002: add tests for moving out-of-cone file/directory
>     @@ Commit message
>      
>          Add corresponding tests to test following situations:
>      
>     -    * 'refuse to move out-of-cone directory without --sparse'
>     -    * 'can move out-of-cone directory with --sparse'
>     -    * 'refuse to move out-of-cone file without --sparse'
>     -    * 'can move out-of-cone file with --sparse'
>     -    * 'refuse to move sparse file to existing destination'
>     -    * 'move sparse file to existing destination with --force and --sparse'
>     +    We do not have sufficient coverage of moving files outside
>     +    of a sparse-checkout cone. Create new tests covering this
>     +    behavior, keeping in mind that the user can include --sparse
>     +    (or not), move a file or directory, and the destination can
>     +    already exist in the index (in this case user can use --force
>     +    to overwrite existing entry).
>      
>     +    Helped-by: Victoria Dye <vdye@github.com>
>     +    Helped-by: Derrick Stolee <derrickstolee@github.com>
>          Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
>      
>       ## t/t7002-mv-sparse-checkout.sh ##
>     +@@ t/t7002-mv-sparse-checkout.sh: test_description='git mv in sparse working trees'
>     + 
>     + . ./test-lib.sh
>     + 
>     ++setup_sparse_checkout () {
>     ++	mkdir folder1 &&
>     ++	touch folder1/file1 &&
>     ++	git add folder1 &&
>     ++	git sparse-checkout set --cone sub
>     ++}
>     ++
>     ++cleanup_sparse_checkout () {
>     ++	git sparse-checkout disable &&
>     ++	git reset --hard
>     ++}
>     ++
>     + test_expect_success 'setup' "
>     + 	mkdir -p sub/dir sub/dir2 &&
>     + 	touch a b c sub/d sub/dir/e sub/dir2/e &&
>     +@@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'can move files to non-sparse dir' '
>     + '
>     + 
>     + test_expect_success 'refuse to move file to non-skip-worktree sparse path' '
>     ++	test_when_finished "cleanup_sparse_checkout" &&
>     + 	git reset --hard &&
>     + 	git sparse-checkout init --no-cone &&
>     + 	git sparse-checkout set a !/x y/ !x/y/z &&
>      @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'refuse to move file to non-skip-worktree sparse path' '
>       	test_cmp expect stderr
>       '
>       
>      +test_expect_failure 'refuse to move out-of-cone directory without --sparse' '
>     -+	git sparse-checkout disable &&
>     -+	git reset --hard &&
>     -+	mkdir folder1 &&
>     -+	touch folder1/file1 &&
>     -+	git add folder1 &&
>     -+	git sparse-checkout init --cone &&
>     -+	git sparse-checkout set sub &&
>     ++	test_when_finished "cleanup_sparse_checkout" &&
>     ++	setup_sparse_checkout &&
>      +
>      +	test_must_fail git mv folder1 sub 2>stderr &&
>      +	cat sparse_error_header >expect &&
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'refuse to move file to non-s
>      +'
>      +
>      +test_expect_failure 'can move out-of-cone directory with --sparse' '
>     -+	git sparse-checkout disable &&
>     -+	git reset --hard &&
>     -+	mkdir folder1 &&
>     -+	touch folder1/file1 &&
>     -+	git add folder1 &&
>     -+	git sparse-checkout init --cone &&
>     -+	git sparse-checkout set sub &&
>     ++	test_when_finished "cleanup_sparse_checkout" &&
>     ++	setup_sparse_checkout &&
>      +
>      +	git mv --sparse folder1 sub 1>actual 2>stderr &&
>      +	test_must_be_empty stderr &&
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'refuse to move file to non-s
>      +'
>      +
>      +test_expect_failure 'refuse to move out-of-cone file without --sparse' '
>     -+	git sparse-checkout disable &&
>     -+	git reset --hard &&
>     -+	mkdir folder1 &&
>     -+	touch folder1/file1 &&
>     -+	git add folder1 &&
>     -+	git sparse-checkout init --cone &&
>     -+	git sparse-checkout set sub &&
>     ++	test_when_finished "cleanup_sparse_checkout" &&
>     ++	setup_sparse_checkout &&
>      +
>      +	test_must_fail git mv folder1/file1 sub 2>stderr &&
>      +	cat sparse_error_header >expect &&
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'refuse to move file to non-s
>      +'
>      +
>      +test_expect_failure 'can move out-of-cone file with --sparse' '
>     -+	git sparse-checkout disable &&
>     -+	git reset --hard &&
>     -+	mkdir folder1 &&
>     -+	touch folder1/file1 &&
>     -+	git add folder1 &&
>     -+	git sparse-checkout init --cone &&
>     -+	git sparse-checkout set sub &&
>     ++	test_when_finished "cleanup_sparse_checkout" &&
>     ++	setup_sparse_checkout &&
>      +
>      +	git mv --sparse folder1/file1 sub 1>actual 2>stderr &&
>      +	test_must_be_empty stderr &&
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'refuse to move file to non-s
>      +'
>      +
>      +test_expect_failure 'refuse to move sparse file to existing destination' '
>     -+	git sparse-checkout disable &&
>     -+	git reset --hard &&
>     ++	test_when_finished "cleanup_sparse_checkout" &&
>      +	mkdir folder1 &&
>      +	touch folder1/file1 &&
>      +	touch sub/file1 &&
>      +	git add folder1 sub/file1 &&
>     -+	git sparse-checkout init --cone &&
>     -+	git sparse-checkout set sub &&
>     ++	git sparse-checkout set --cone sub &&
>      +
>      +	test_must_fail git mv --sparse folder1/file1 sub 2>stderr &&
>      +	echo "fatal: destination exists, source=folder1/file1, destination=sub/file1" >expect &&
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'refuse to move file to non-s
>      +'
>      +
>      +test_expect_failure 'move sparse file to existing destination with --force and --sparse' '
>     -+	git sparse-checkout disable &&
>     -+	git reset --hard &&
>     ++	test_when_finished "cleanup_sparse_checkout" &&
>      +	mkdir folder1 &&
>      +	touch folder1/file1 &&
>      +	touch sub/file1 &&
>      +	echo "overwrite" >folder1/file1 &&
>      +	git add folder1 sub/file1 &&
>     -+	git sparse-checkout init --cone &&
>     -+	git sparse-checkout set sub &&
>     ++	git sparse-checkout set --cone sub &&
>      +
>      +	git mv --sparse --force folder1/file1 sub 2>stderr &&
>      +	test_must_be_empty stderr &&
> -:  ---------- > 2:  8065fbc232 mv: decouple if/else-if checks using goto
> 2:  80f485f146 ! 3:  e227fe717b mv: check if out-of-cone file exists in index with SKIP_WORKTREE bit
>     @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>       
>       		length = strlen(src);
>       		if (lstat(src, &st) < 0) {
>     -+			/*
>     -+			 * TODO: for now, when you try to overwrite a <destination>
>     -+			 * with your <source> as a sparse file, if you supply a "--sparse"
>     -+			 * flag, then the action will be done without providing "--force"
>     -+			 * and no warning.
>     -+			 *
>     -+			 * This is mainly because the sparse <source>
>     -+			 * is not on-disk, and this if-else chain will be cut off early in
>     -+			 * this check, thus the "--force" check is ignored. Need fix.
>     -+			 */
>     +-			/* only error if existence is expected. */
>     +-			if (modes[i] != SPARSE) {
>     ++			int pos;
>     ++			const struct cache_entry *ce;
>      +
>     -+			int pos = cache_name_pos(src, length);
>     -+			if (pos >= 0) {
>     -+				const struct cache_entry *ce = active_cache[pos];
>     -+
>     -+				if (ce_skip_worktree(ce)) {
>     -+					if (!ignore_sparse)
>     -+						string_list_append(&only_match_skip_worktree, src);
>     -+					else
>     -+						modes[i] = SPARSE;
>     -+				}
>     -+				else
>     ++			pos = cache_name_pos(src, length);
>     ++			if (pos < 0) {
>     ++				/* only error if existence is expected. */
>     ++				if (modes[i] != SPARSE)
>      +					bad = _("bad source");
>     ++				goto act_on_entry;
>      +			}
>     - 			/* only error if existence is expected. */
>     --			if (modes[i] != SPARSE)
>     -+			else if (modes[i] != SPARSE)
>     ++
>     ++			ce = active_cache[pos];
>     ++			if (!ce_skip_worktree(ce)) {
>       				bad = _("bad source");
>     - 		} else if (!strncmp(src, dst, length) &&
>     - 				(dst[length] == 0 || dst[length] == '/')) {
>     + 				goto act_on_entry;
>     + 			}
>     ++
>     ++			if (!ignore_sparse)
>     ++				string_list_append(&only_match_skip_worktree, src);
>     ++			else
>     ++				modes[i] = SPARSE;
>     ++			goto act_on_entry;
>     + 		}
>     + 		if (!strncmp(src, dst, length) &&
>     + 		    (dst[length] == 0 || dst[length] == '/')) {
>      
>       ## t/t7002-mv-sparse-checkout.sh ##
>      @@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'can move out-of-cone directory with --sparse' '
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'can move out-of-cone directo
>       
>      -test_expect_failure 'refuse to move out-of-cone file without --sparse' '
>      +test_expect_success 'refuse to move out-of-cone file without --sparse' '
>     - 	git sparse-checkout disable &&
>     - 	git reset --hard &&
>     - 	mkdir folder1 &&
>     + 	test_when_finished "cleanup_sparse_checkout" &&
>     + 	setup_sparse_checkout &&
>     + 
>      @@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'refuse to move out-of-cone file without --sparse' '
>       	test_cmp expect stderr
>       '
>       
>      -test_expect_failure 'can move out-of-cone file with --sparse' '
>      +test_expect_success 'can move out-of-cone file with --sparse' '
>     - 	git sparse-checkout disable &&
>     - 	git reset --hard &&
>     - 	mkdir folder1 &&
>     + 	test_when_finished "cleanup_sparse_checkout" &&
>     + 	setup_sparse_checkout &&
>     + 
> 3:  04572e5e6b ! 4:  d0de7678e3 mv: check if <destination> exists in index to handle overwriting
>     @@ Commit message
>      
>       ## builtin/mv.c ##
>      @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - 
>     - 		length = strlen(src);
>     - 		if (lstat(src, &st) < 0) {
>     --			/*
>     --			 * TODO: for now, when you try to overwrite a <destination>
>     --			 * with your <source> as a sparse file, if you supply a "--sparse"
>     --			 * flag, then the action will be done without providing "--force"
>     --			 * and no warning.
>     --			 *
>     --			 * This is mainly because the sparse <source>
>     --			 * is not on-disk, and this if-else chain will be cut off early in
>     --			 * this check, thus the "--force" check is ignored. Need fix.
>     --			 */
>     - 
>     - 			int pos = cache_name_pos(src, length);
>     - 			if (pos >= 0) {
>     -@@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - 				if (ce_skip_worktree(ce)) {
>     - 					if (!ignore_sparse)
>     - 						string_list_append(&only_match_skip_worktree, src);
>     --					else
>     --						modes[i] = SPARSE;
>     -+					else {
>     -+						/* Check if dst exists in index */
>     -+						if (cache_name_pos(dst, strlen(dst)) >= 0) {
>     -+							if (force)
>     -+								modes[i] = SPARSE;
>     -+							else
>     -+								bad = _("destination exists");
>     -+						}
>     -+						else
>     -+							modes[i] = SPARSE;
>     -+					}
>     - 				}
>     - 				else
>     - 					bad = _("bad source");
>     + 				bad = _("bad source");
>     + 				goto act_on_entry;
>     + 			}
>     +-
>     +-			if (!ignore_sparse)
>     ++			if (!ignore_sparse) {
>     + 				string_list_append(&only_match_skip_worktree, src);
>     +-			else
>     ++				goto act_on_entry;
>     ++			}
>     ++			/* Check if dst exists in index */
>     ++			if (cache_name_pos(dst, strlen(dst)) < 0) {
>     + 				modes[i] = SPARSE;
>     ++				goto act_on_entry;
>     ++			}
>     ++			if (!force) {
>     ++				bad = _("destination exists");
>     ++				goto act_on_entry;
>     ++			}
>     ++			modes[i] = SPARSE;
>     + 			goto act_on_entry;
>     + 		}
>     + 		if (!strncmp(src, dst, length) &&
>      
>       ## t/t7002-mv-sparse-checkout.sh ##
>      @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'can move out-of-cone file with --sparse' '
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'can move out-of-cone file wi
>       
>      -test_expect_failure 'refuse to move sparse file to existing destination' '
>      +test_expect_success 'refuse to move sparse file to existing destination' '
>     - 	git sparse-checkout disable &&
>     - 	git reset --hard &&
>     + 	test_when_finished "cleanup_sparse_checkout" &&
>       	mkdir folder1 &&
>     + 	touch folder1/file1 &&
> -:  ---------- > 5:  70540957b6 mv: use flags mode for update_mode
> 4:  4eeae40186 ! 6:  f8302f64e0 mv: add check_dir_in_index() and solve general dir check issue
>     @@ Commit message
>          instead of "bad source"; also user now can supply a "--sparse" flag so
>          this operation can be carried out successfully.
>      
>     -    Also, as suggested by Derrick [1],
>     -    move the in-line definition of "enum update_mode" to the top
>     -    of the file and make it use "flags" mode (each state is a different
>     -    bit in the word).
>     -
>     -    [1] https://lore.kernel.org/git/22aadea2-9330-aa9e-7b6a-834585189144@github.com/
>     -
>          Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
>      
>       ## builtin/mv.c ##
>     -@@ builtin/mv.c: static const char * const builtin_mv_usage[] = {
>     - 	NULL
>     - };
>     - 
>     -+enum update_mode {
>     -+	BOTH = 0,
>     -+	WORKING_DIRECTORY = (1 << 1),
>     -+	INDEX = (1 << 2),
>     -+	SPARSE = (1 << 3),
>     -+	SKIP_WORKTREE_DIR = (1 << 4),
>     -+};
>     -+
>     - #define DUP_BASENAME 1
>     - #define KEEP_TRAILING_SLASH 2
>     - 
>      @@ builtin/mv.c: static int index_range_of_same_dir(const char *src, int length,
>       	return last - first;
>       }
>     @@ builtin/mv.c: static int index_range_of_same_dir(const char *src, int length,
>       {
>       	int i, flags, gitmodules_modified = 0;
>      @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - 		OPT_END(),
>     - 	};
>     - 	const char **source, **destination, **dest_path, **submodule_gitfile;
>     --	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX, SPARSE } *modes;
>     -+	enum update_mode *modes;
>     - 	struct stat st;
>     - 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
>     - 	struct lock_file lock_file = LOCK_INIT;
>     -@@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - 		if (lstat(src, &st) < 0) {
>     - 
>     - 			int pos = cache_name_pos(src, length);
>     -+			const char *src_w_slash = add_slash(src);
>     -+
>     - 			if (pos >= 0) {
>     - 				const struct cache_entry *ce = active_cache[pos];
>     + 	/* Checking */
>     + 	for (i = 0; i < argc; i++) {
>     + 		const char *src = source[i], *dst = destination[i];
>     +-		int length, src_is_dir;
>     ++		int length;
>     + 		const char *bad = NULL;
>     + 		int skip_sparse = 0;
>       
>      @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - 				else
>     + 
>     + 			pos = cache_name_pos(src, length);
>     + 			if (pos < 0) {
>     ++				const char *src_w_slash = add_slash(src);
>     ++				if (!check_dir_in_index(src, length) &&
>     ++					!path_in_sparse_checkout(src_w_slash, &the_index)) {
>     ++					modes[i] |= SKIP_WORKTREE_DIR;
>     ++					goto dir_check;
>     ++				}
>     + 				/* only error if existence is expected. */
>     + 				if (!(modes[i] & SPARSE))
>       					bad = _("bad source");
>     + 				goto act_on_entry;
>       			}
>     -+			else if (!check_dir_in_index(src, length) &&
>     -+					 !path_in_sparse_checkout(src_w_slash, &the_index)) {
>     -+				modes[i] = SKIP_WORKTREE_DIR;
>     -+				goto dir_check;
>     -+			}
>     - 			/* only error if existence is expected. */
>     - 			else if (modes[i] != SPARSE)
>     +-
>     + 			ce = active_cache[pos];
>     + 			if (!ce_skip_worktree(ce)) {
>       				bad = _("bad source");
>      @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - 				&& lstat(dst, &st) == 0)
>     + 			bad = _("can not move directory into itself");
>     + 			goto act_on_entry;
>     + 		}
>     +-		if ((src_is_dir = S_ISDIR(st.st_mode))
>     ++		if (S_ISDIR(st.st_mode)
>     + 		    && lstat(dst, &st) == 0) {
>       			bad = _("cannot move directory over file");
>     - 		else if (src_is_dir) {
>     + 			goto act_on_entry;
>     + 		}
>     +-		if (src_is_dir) {
>     ++
>     ++dir_check:
>     ++		if (S_ISDIR(st.st_mode)) {
>     + 			int j, dst_len, n;
>      -			int first = cache_name_pos(src, length), last;
>      +			int first, last;
>     -+dir_check:
>      +			first = cache_name_pos(src, length);
>       
>     - 			if (first >= 0)
>     + 			if (first >= 0) {
>       				prepare_move_submodule(src, first,
>     -@@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - 			else { /* last - first >= 1 */
>     - 				int j, dst_len, n;
>     - 
>     --				modes[i] = WORKING_DIRECTORY;
>     -+				if (!modes[i])
>     -+					modes[i] |= WORKING_DIRECTORY;
>     - 				n = argc + last - first;
>     - 				REALLOC_ARRAY(source, n);
>     - 				REALLOC_ARRAY(destination, n);
>     -@@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - 			printf(_("Renaming %s to %s\n"), src, dst);
>     - 		if (show_only)
>     - 			continue;
>     --		if (mode != INDEX && mode != SPARSE && rename(src, dst) < 0) {
>     -+		if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
>     -+		 	rename(src, dst) < 0) {
>     - 			if (ignore_errors)
>     - 				continue;
>     - 			die_errno(_("renaming '%s' failed"), src);
>     -@@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - 							      1);
>     - 		}
>     - 
>     --		if (mode == WORKING_DIRECTORY)
>     -+		if (mode & (WORKING_DIRECTORY | SKIP_WORKTREE_DIR))
>     - 			continue;
>     - 
>     - 		pos = cache_name_pos(src, strlen(src));
>      
>       ## t/t7002-mv-sparse-checkout.sh ##
>      @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'refuse to move file to non-skip-worktree sparse path' '
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'refuse to move file to non-s
>       
>      -test_expect_failure 'refuse to move out-of-cone directory without --sparse' '
>      +test_expect_success 'refuse to move out-of-cone directory without --sparse' '
>     - 	git sparse-checkout disable &&
>     - 	git reset --hard &&
>     - 	mkdir folder1 &&
>     + 	test_when_finished "cleanup_sparse_checkout" &&
>     + 	setup_sparse_checkout &&
>     + 
>      @@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'refuse to move out-of-cone directory without --sparse' '
>       	test_cmp expect stderr
>       '
>       
>      -test_expect_failure 'can move out-of-cone directory with --sparse' '
>      +test_expect_success 'can move out-of-cone directory with --sparse' '
>     - 	git sparse-checkout disable &&
>     - 	git reset --hard &&
>     - 	mkdir folder1 &&
>     + 	test_when_finished "cleanup_sparse_checkout" &&
>     + 	setup_sparse_checkout &&
>     + 
> 5:  a3a296c3ef ! 7:  bc996931e2 mv: use update_sparsity() after touching sparse contents
>     @@ Metadata
>      Author: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
>      
>       ## Commit message ##
>     -    mv: use update_sparsity() after touching sparse contents
>     +    mv: update sparsity after moving from out-of-cone to in-cone
>      
>     -    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.
>     +    Originally, "git mv" a sparse file from out-of-cone to
>     +    in-cone does not update the moved file's sparsity (remove its
>     +    SKIP_WORKTREE bit). And the corresponding cache entry is, unexpectedly,
>     +    not checked out in the working tree.
>      
>     -    Use update_sparsity() after touching sparse contents, so the sparsity
>     -    will be updated after the move.
>     +    Update the behavior so that:
>     +    1. Moving from out-of-cone to in-cone removes the SKIP_WORKTREE bit from
>     +       corresponding cache entry.
>     +    2. The moved cache entry is checked out in the working tree to reflect
>     +       the updated sparsity.
>      
>          Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
>      
>     @@ builtin/mv.c
>       #include "string-list.h"
>       #include "parse-options.h"
>       #include "submodule.h"
>     -+#include "unpack-trees.h"
>     ++#include "entry.h"
>       
>       static const char * const builtin_mv_usage[] = {
>       	N_("git mv [<options>] <source>... <destination>"),
>     -@@ builtin/mv.c: 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")),
>      @@ builtin/mv.c: 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;
>     ++		struct checkout state = CHECKOUT_INIT;
>     ++		state.istate = &the_index;
>     ++
>     ++		if (force)
>     ++			state.force = 1;
>       		if (show_only || verbose)
>       			printf(_("Renaming %s to %s\n"), src, dst);
>       		if (show_only)
>      @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     + 		pos = cache_name_pos(src, strlen(src));
>     + 		assert(pos >= 0);
>       		rename_cache_entry_at(pos, dst);
>     ++
>     ++		if (mode & SPARSE) {
>     ++			if (path_in_sparse_checkout(dst, &the_index)) {
>     ++				int dst_pos;
>     ++
>     ++				dst_pos = cache_name_pos(dst, strlen(dst));
>     ++				active_cache[dst_pos]->ce_flags &= ~CE_SKIP_WORKTREE;
>     ++
>     ++				if (checkout_entry(active_cache[dst_pos], &state, NULL, NULL))
>     ++					die(_("cannot checkout %s"), ce->name);
>     ++			}
>     ++		}
>       	}
>       
>     -+	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);
>     - 
>      
>       ## t/t7002-mv-sparse-checkout.sh ##
>     +@@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'can move out-of-cone directory with --sparse' '
>     + 	git mv --sparse folder1 sub 1>actual 2>stderr &&
>     + 	test_must_be_empty stderr &&
>     + 
>     +-	git sparse-checkout reapply &&
>     + 	test_path_is_dir sub/folder1 &&
>     + 	test_path_is_file sub/folder1/file1
>     + '
>     +@@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'can move out-of-cone file with --sparse' '
>     + 	git mv --sparse folder1/file1 sub 1>actual 2>stderr &&
>     + 	test_must_be_empty stderr &&
>     + 
>     +-	git sparse-checkout reapply &&
>     + 	! test_path_is_dir sub/folder1 &&
>     + 	test_path_is_file sub/file1
>     + '
>      @@ t/t7002-mv-sparse-checkout.sh: 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 &&
>     +-test_expect_failure 'move sparse file to existing destination with --force and --sparse' '
>     ++test_expect_success 'move sparse file to existing destination with --force and --sparse' '
>     + 	test_when_finished "cleanup_sparse_checkout" &&
>     + 	mkdir folder1 &&
>     + 	touch folder1/file1 &&
> 
> base-commit: 4f6db706e6ad145a9bf6b26a1ca0970bed27bb72

> Shaoxuan Yuan wrote:
>> But I think it worth discuss if we should implement in-cone to 
>> out-of-cone move, since it will be nice (naturally) to have it working.
>>
>> However, I noticed this from the mv man page:
>>
>> "In the second form, the last argument has to be an existing directory; 
>> the given sources will be moved into this directory."
>>
>> I think trying to move out-of-cone, the last argument has to be an non-existent
>> directory? I'm a bit confused: should we update some of mv basic logic to 
>> accomplish this?
>>
> 
> I suspect this requirement is related to the POSIX 'mv' [1] (and
> corresponding 'rename()', used in 'git mv'), which also requires that the
> destination directory exists. I personally don't think this requirement
> needs to apply to 'git mv' at all, but note that changing the behavior would
> require first creating the necessary directories before calling 'rename()'. 
> 
> As a more conservative solution, you could do the parent directory creation
> *only* in the case of moving to a sparse contents-only directory (using
> something like the 'check_dir_in_index()' function you introduced to
> identify).
> 
> I'm also interested in hearing what others have to say, especially regarding
> historical context/use cases of 'git mv'.
> 
> [1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/mv.html

> On 6/21/2022 7:30 PM, Victoria Dye wrote:
>> Shaoxuan Yuan wrote:
>>> But I think it worth discuss if we should implement in-cone to 
>>> out-of-cone move, since it will be nice (naturally) to have it working.
>>>
>>> However, I noticed this from the mv man page:
>>>
>>> "In the second form, the last argument has to be an existing directory; 
>>> the given sources will be moved into this directory."
>>>
>>> I think trying to move out-of-cone, the last argument has to be an non-existent
>>> directory? I'm a bit confused: should we update some of mv basic logic to 
>>> accomplish this?
>>>
>> 
>> I suspect this requirement is related to the POSIX 'mv' [1] (and
>> corresponding 'rename()', used in 'git mv'), which also requires that the
>> destination directory exists. I personally don't think this requirement
>> needs to apply to 'git mv' at all, but note that changing the behavior would
>> require first creating the necessary directories before calling 'rename()'. 
>> 
>> As a more conservative solution, you could do the parent directory creation
>> *only* in the case of moving to a sparse contents-only directory (using
>> something like the 'check_dir_in_index()' function you introduced to
>> identify).
>> 
>> I'm also interested in hearing what others have to say, especially regarding
>> historical context/use cases of 'git mv'.
>> 
>> [1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/mv.html
>
> I wanted to reply here to maybe get more attention on this point.
>
> My personal opinion is that `git mv` should move to the location requested,
> even if it requires adding parent directories. Changing that behavior might
> need to come as its own topic, before doing the in-cone-to-out-of-cone work.
> Knowing if this behavior can change (or must stay the same) informs how that
> sparse case will work.

> Originally, "git mv" a sparse file from out-of-cone to
> in-cone does not update the moved file's sparsity (remove its
> SKIP_WORKTREE bit). And the corresponding cache entry is, unexpectedly,
> not checked out in the working tree.
> 
> Update the behavior so that:
> 1. Moving from out-of-cone to in-cone removes the SKIP_WORKTREE bit from
>    corresponding cache entry.
> 2. The moved cache entry is checked out in the working tree to reflect
>    the updated sparsity.

> 
> On 6/23/2022 11:08 PM, Derrick Stolee wrote:
>> On 6/23/2022 7:41 AM, Shaoxuan Yuan wrote:
>>> Originally, "git mv" a sparse file from out-of-cone to
>>> in-cone does not update the moved file's sparsity (remove its
>>> SKIP_WORKTREE bit). And the corresponding cache entry is, unexpectedly,
>>> not checked out in the working tree.
>>>
>>> Update the behavior so that:
>>> 1. Moving from out-of-cone to in-cone removes the SKIP_WORKTREE bit from
>>>    corresponding cache entry.
>>> 2. The moved cache entry is checked out in the working tree to reflect
>>>    the updated sparsity.
>>
>> Since this is a behavior change, can we test it? It would be good
>> to verify that the new path exists in the worktree after 'git mv'
>> succeeds.
> 
> I don't think we can effectively test this based on the change per se.
> This change is preparing a correct behavior for the next few
> commits, so I'll say it's tested along with the next few commits
> (i.e. move "sparse" file/directory ones)?

> 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.
> + */

> On 6/23/2022 11:14 PM, Derrick Stolee wrote:
>> On 6/23/2022 7:41 AM, Shaoxuan Yuan wrote:
>>> +/*
>>> + * 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?

> On 6/23/2022 7:41 AM, Shaoxuan Yuan wrote:
>> ## Changes since WIP v3 ##
>
> It's good to keep the main cover letter body around, even as the version
> updates are included. Sometimes reviewers come to a topic late and want
> to see the latest version be completely self-contained.

> ## Changes since PATCH v4 ##
> 
> 1. Fix style-nits.
> 
> 2. Add t1092 tests (2/8) for "mv: add check_dir_in_index() and solve 
>    general dir check issue" (8/8).

> On 6/29/2022 10:37 PM, Shaoxuan Yuan wrote:
>> ## Changes since PATCH v4 ##
>> 
>> 1. Fix style-nits.
>> 
>> 2. Add t1092 tests (2/8) for "mv: add check_dir_in_index() and solve 
>>    general dir check issue" (8/8).
>
> Thank you for these updates. I just took a quick
> re-read and I'm happy with this version.
>
> Thanks,
> -Stolee