[PATCH v1 0/7] mv: from in-cone to out-of-cone

[PATCH v1 0/7] mv: from in-cone to out-of-cone

[Shaoxuan Yuan]

This is a sister series to the previous "from out-of-cone to in-cone" [1]
series. This series is trying to make the opposite operation possible for
'mv', namely move <source>, which is in-cone, to <destination>, which is
out-of-cone.

Other than the main task, there are also some minor fixes done.

[1] https://lore.kernel.org/git/20220331091755.385961-1-shaoxuan.yuan02@gmail.com/

Shaoxuan Yuan (7):
  t7002: add tests for moving from in-cone to out-of-cone
  mv: add documentation for check_dir_in_index()
  mv: free the *with_slash in check_dir_in_index()
  mv: check if <destination> is a SKIP_WORKTREE_DIR
  mv: remove BOTH from enum update_mode
  mv: from in-cone to out-of-cone
  mv: check overwrite for in-to-out move

 advice.c                      |  19 +++++
 advice.h                      |   1 +
 builtin/mv.c                  | 100 +++++++++++++++++++----
 t/t7002-mv-sparse-checkout.sh | 148 +++++++++++++++++++++++++++++++++-
 4 files changed, 250 insertions(+), 18 deletions(-)


base-commit: 71a8fab31b70c417e8f5b5f716581f89955a7082
-- 
2.37.0

[PATCH v1 1/7] t7002: add tests for moving from in-cone to out-of-cone

[Shaoxuan Yuan]

Add corresponding tests to test that user can move an in-cone <source>
to out-of-cone <destination> when --sparse is supplied.

Such <source> can be either clean or dirty, and moving it results in
different behaviors:

A clean move should move the <source> to the <destination>, both in the
working tree and the index, then remove the resulted path from the
working tree, and turn on its CE_SKIP_WORKTREE bit.

A dirty move should move the <source> to the <destination>, both in the
working tree and the index, but should *not* remove the resulted path
from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit.
Instead advise user to "git add" this path and run "git sparse-checkout
reapply" to re-sparsify that path.

Also make sure that if <destination> exists in the index (existing
check for if <destination> is in the worktree is not enough in
in-to-out moves), warn user against the overwrite. And Git should force
the overwrite when supplied with -f or --force.

Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 t/t7002-mv-sparse-checkout.sh | 148 +++++++++++++++++++++++++++++++++-
 1 file changed, 147 insertions(+), 1 deletion(-)

diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
index 71fe29690f..c27fcdbfd0 100755
--- a/t/t7002-mv-sparse-checkout.sh
+++ b/t/t7002-mv-sparse-checkout.sh
@@ -28,12 +28,25 @@ test_expect_success 'setup' "
 	updated in the index:
 	EOF
 
-	cat >sparse_hint <<-EOF
+	cat >sparse_hint <<-EOF &&
 	hint: If you intend to update such entries, try one of the following:
 	hint: * Use the --sparse option.
 	hint: * Disable or modify the sparsity rules.
 	hint: Disable this message with \"git config advice.updateSparsePath false\"
 	EOF
+
+	cat >dirty_error_header <<-EOF &&
+	The following paths have been moved outside the
+	sparse-checkout definition but are not sparse due to local
+	modifications.
+	EOF
+
+	cat >dirty_hint <<-EOF
+	hint: To correct the sparsity of these paths, do the following:
+	hint: * Use \"git add --sparse <paths>\" to update the index
+	hint: * Use \"git sparse-checkout reapply\" to apply the sparsity rules
+	hint: Disable this message with \"git config advice.updateSparsePath false\"
+	EOF
 "
 
 test_expect_success 'mv refuses to move sparse-to-sparse' '
@@ -290,4 +303,137 @@ test_expect_success 'move sparse file to existing destination with --force and -
 	test_cmp expect sub/file1
 '
 
+test_expect_failure 'move clean path from in-cone to out-of-cone' '
+	test_when_finished "cleanup_sparse_checkout" &&
+	setup_sparse_checkout &&
+
+	test_must_fail git mv sub/d folder1 2>stderr &&
+	cat sparse_error_header >expect &&
+	echo "folder1/d" >>expect &&
+	cat sparse_hint >>expect &&
+	test_cmp expect stderr &&
+
+	git mv --sparse sub/d folder1 2>stderr &&
+	test_must_be_empty stderr &&
+
+	test_path_is_missing sub/d &&
+	test_path_is_missing folder1/d &&
+	git ls-files -t >actual &&
+	! grep -x "H sub/d" actual &&
+	grep -x "S folder1/d" actual
+'
+
+test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
+	test_when_finished "cleanup_sparse_checkout" &&
+	setup_sparse_checkout &&
+	echo "sub/file1 overwrite" >sub/file1 &&
+	git add sub/file1 &&
+
+	test_must_fail git mv sub/file1 folder1 2>stderr &&
+	cat sparse_error_header >expect &&
+	echo "folder1/file1" >>expect &&
+	cat sparse_hint >>expect &&
+	test_cmp expect stderr &&
+
+	test_must_fail git mv --sparse sub/file1 folder1 2>stderr &&
+	echo "fatal: destination exists in the index, source=sub/file1, destination=folder1/file1" \
+	>expect &&
+	test_cmp expect stderr &&
+
+	git mv --sparse -f sub/file1 folder1 2>stderr &&
+	test_must_be_empty stderr &&
+
+	test_path_is_missing sub/file1 &&
+	test_path_is_missing folder1/file1 &&
+	git ls-files -t >actual &&
+	! grep -x "H sub/file1" actual &&
+	grep -x "S folder1/file1" actual &&
+
+	# compare file content before move and after move
+	echo "sub/file1 overwrite" >expect &&
+	git ls-files -s -- folder1/file1 | awk "{print \$2}" >oid &&
+	git cat-file blob $(cat oid) >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'move dirty path from in-cone to out-of-cone' '
+	test_when_finished "cleanup_sparse_checkout" &&
+	setup_sparse_checkout &&
+	echo "modified" >>sub/d &&
+
+	test_must_fail git mv sub/d folder1 2>stderr &&
+	cat sparse_error_header >expect &&
+	echo "folder1/d" >>expect &&
+	cat sparse_hint >>expect &&
+	test_cmp expect stderr &&
+
+	git mv --sparse sub/d folder1 2>stderr &&
+	cat dirty_error_header >expect &&
+	echo "folder1/d" >>expect &&
+	cat dirty_hint >>expect &&
+	test_cmp expect stderr &&
+
+	test_path_is_missing sub/d &&
+	test_path_is_file folder1/d &&
+	git ls-files -t >actual &&
+	! grep -x "H sub/d" actual &&
+	grep -x "H folder1/d" actual
+'
+
+test_expect_failure 'move dir from in-cone to out-of-cone' '
+	test_when_finished "cleanup_sparse_checkout" &&
+	setup_sparse_checkout &&
+
+	test_must_fail git mv sub/dir folder1 2>stderr &&
+	cat sparse_error_header >expect &&
+	echo "folder1/dir/e" >>expect &&
+	cat sparse_hint >>expect &&
+	test_cmp expect stderr &&
+
+	git mv --sparse sub/dir folder1 2>stderr &&
+	test_must_be_empty stderr &&
+
+	test_path_is_missing sub/dir &&
+	test_path_is_missing folder1 &&
+	git ls-files -t >actual &&
+	! grep -x "H sub/dir/e" actual &&
+	grep -x "S folder1/dir/e" actual
+'
+
+test_expect_failure 'move partially-dirty dir from in-cone to out-of-cone' '
+	test_when_finished "cleanup_sparse_checkout" &&
+	setup_sparse_checkout &&
+	touch sub/dir/e2 sub/dir/e3 &&
+	git add sub/dir/e2 sub/dir/e3 &&
+	echo "modified" >>sub/dir/e2 &&
+	echo "modified" >>sub/dir/e3 &&
+
+	test_must_fail git mv sub/dir folder1 2>stderr &&
+	cat sparse_error_header >expect &&
+	echo "folder1/dir/e" >>expect &&
+	echo "folder1/dir/e2" >>expect &&
+	echo "folder1/dir/e3" >>expect &&
+	cat sparse_hint >>expect &&
+	test_cmp expect stderr &&
+
+	git mv --sparse sub/dir folder1 2>stderr &&
+	cat dirty_error_header >expect &&
+	echo "folder1/dir/e2" >>expect &&
+	echo "folder1/dir/e3" >>expect &&
+	cat dirty_hint >>expect &&
+	test_cmp expect stderr &&
+
+	test_path_is_missing sub/dir &&
+	test_path_is_missing folder1/dir/e &&
+	test_path_is_file folder1/dir/e2 &&
+	test_path_is_file folder1/dir/e3 &&
+	git ls-files -t >actual &&
+	! grep -x "H sub/dir/e" actual &&
+	! grep -x "H sub/dir/e2" actual &&
+	! grep -x "H sub/dir/e3" actual &&
+	grep -x "S folder1/dir/e" actual &&
+	grep -x "H folder1/dir/e2" actual &&
+	grep -x "H folder1/dir/e3" actual
+'
+
 test_done
-- 
2.37.0

Re: [PATCH v1 1/7] t7002: add tests for moving from in-cone to out-of-cone

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 19 2022, Shaoxuan Yuan wrote:

> +	! grep -x "H sub/d" actual &&
> +	grep -x "S folder1/d" actual

This *might* be a portability problem, but probably not. It's listed in
POSIX:
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html

But it appears we have no "grep" use of the "-x" option in-tree, at
least from what I found with grepping..

Re: [PATCH v1 1/7] t7002: add tests for moving from in-cone to out-of-cone

[Derrick Stolee]

On 7/19/2022 10:52 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Jul 19 2022, Shaoxuan Yuan wrote:
> 
>> +	! grep -x "H sub/d" actual &&
>> +	grep -x "S folder1/d" actual
> 
> This *might* be a portability problem, but probably not. It's listed in
> POSIX:
> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html
> 
> But it appears we have no "grep" use of the "-x" option in-tree, at
> least from what I found with grepping..

It is helpful to recommend a working alternative when pointing out
an issue like this.

Here, the '-x' option means to match the entire line.

Would using ^ and $ help here?

	! grep "^H sub/d\$" actual &&
	grep "^S folder1/d\$" actual

I haven't tested this, but hopefully the idea is sound. If not,
then you could make the names be sufficiently unique that
whole-lines are the only way they could match. We do have some
expectation about what lines are possible in this index, since
we constructed the example repo from scratch.

Thanks,
-Stolee

Re: [PATCH v1 1/7] t7002: add tests for moving from in-cone to out-of-cone

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> On 7/19/2022 10:52 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Tue, Jul 19 2022, Shaoxuan Yuan wrote:
>> 
>>> +	! grep -x "H sub/d" actual &&
>>> +	grep -x "S folder1/d" actual
>> 
>> This *might* be a portability problem, but probably not. It's listed in
>> POSIX:
>> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html
>> 
>> But it appears we have no "grep" use of the "-x" option in-tree, at
>> least from what I found with grepping..
>
> It is helpful to recommend a working alternative when pointing out
> an issue like this.
>
> Here, the '-x' option means to match the entire line.
>
> Would using ^ and $ help here?

Yup, that would be quite reasonable and removes the need to use,
learn about, or have your readers wonder about the "-x" option; I do
not think I saw a grep that did not understand the option, though.

Thanks.

[PATCH v1 2/7] mv: add documentation for check_dir_in_index()

[Shaoxuan Yuan]

Using check_dir_in_index without checking if the directory is on-disk
could get a false positive for partially sparsified directory.

Add a note in the documentation to warn potential user.

Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/builtin/mv.c b/builtin/mv.c
index 4729bb1a1a..c8b9069db8 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -132,6 +132,11 @@ static int index_range_of_same_dir(const char *src, int length,
  * 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.
+ *
+ * Note: *always* check the directory is not on-disk before this function
+ * (i.e. using lstat());
+ * otherwise it may return a false positive for a partially sparsified
+ * directory.
  */
 static int check_dir_in_index(const char *name)
 {
-- 
2.37.0

Re: [PATCH v1 2/7] mv: add documentation for check_dir_in_index()

[Derrick Stolee]

On 7/19/2022 9:28 AM, Shaoxuan Yuan wrote:
> Using check_dir_in_index without checking if the directory is on-disk
> could get a false positive for partially sparsified directory.

It can be helpful to point out that this is a relatively new
method, introduced in b91a2b6594 (mv: add check_dir_in_index()
and solve general dir check issue, 2022-06-30).

> + *
> + * Note: *always* check the directory is not on-disk before this function
> + * (i.e. using lstat());
> + * otherwise it may return a false positive for a partially sparsified
> + * directory.

I'm not sure what you mean by a "false positive" in this case.
The directory exists in the index, which is what the method is
defined as checking. This does not say anything about the
worktree.

Perhaps that's the real problem? Someone might interpret this
as meaning the directory does not exist in the worktree? That
would mean that this doc update needs to be changed significantly
to say "Note that this does not imply anything about the state
of the worktree" or something.

But I think I'd rather just see this patch be dropped, unless I
am missing something important.

Thanks,
-Stolee

Re: [PATCH v1 2/7] mv: add documentation for check_dir_in_index()

[Shaoxuan Yuan]

On 7/20/2022 1:43 AM, Derrick Stolee wrote:
 >> + *
 >> + * Note: *always* check the directory is not on-disk before this 
function
 >> + * (i.e. using lstat());
 >> + * otherwise it may return a false positive for a partially sparsified
 >> + * directory.
 >
 > I'm not sure what you mean by a "false positive" in this case.
 > The directory exists in the index, which is what the method is
 > defined as checking. This does not say anything about the
 > worktree.
 >
 > Perhaps that's the real problem? Someone might interpret this
 > as meaning the directory does not exist in the worktree? That
 > would mean that this doc update needs to be changed significantly
 > to say "Note that this does not imply anything about the state
 > of the worktree" or something.

This method assumes that the directory being checking does not exist
in the working tree, but the method itself does not check this. And
if the user does not make sure the directory is absent from the
worktree, this method may return a success for a partially sparsified
directory, which is not intended.

 > But I think I'd rather just see this patch be dropped, unless I
 > am missing something important.

I found Victoria's paraphrase [1] makes my point much clearer.

--
Thanks,
Shaoxuan

Re: [PATCH v1 2/7] mv: add documentation for check_dir_in_index()

[Victoria Dye]

Shaoxuan Yuan wrote:
> Using check_dir_in_index without checking if the directory is on-disk
> could get a false positive for partially sparsified directory.
> 
> Add a note in the documentation to warn potential user.
> 
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>  builtin/mv.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 4729bb1a1a..c8b9069db8 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -132,6 +132,11 @@ static int index_range_of_same_dir(const char *src, int length,
>   * 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.
> + *
> + * Note: *always* check the directory is not on-disk before this function
> + * (i.e. using lstat());
> + * otherwise it may return a false positive for a partially sparsified
> + * directory.

To me, a comment like this indicates that either the function is not doing
what it should be doing, or its name doesn't properly describe the
function's behavior.

Per the function description:

	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.

But neither the name of the function ('check_dir_in_index') nor the
function's behavior (hence the comment you're adding here) match that
description.

Since this function is intended to verify that a directory 1) exists in the
index, and 2) is *entirely* sparse, I have two suggestions:

* Change the description to specify that the non-existence of the directory
  on disk is an *assumption*, not an opportunity for a "false positive."
  It's a subtle distinction, but it clarifies that the function should be
  used only when the caller already knows the directory is empty. For
  example:

	/*
	 * Given the path of a directory that does not exist on-disk, check whether the
	 * directory contains any entries in the index with the SKIP_WORKTREE flag 
	 * enabled.
	 *
	 * Return 1 if such index entries exist.
	 * Return 0 otherwise.
	 */

* 'check_dir_in_index' doesn't reflect the "is not on disk" prerequisite, so
  the function name can be updated to clarify that (e.g.,
  'empty_dir_has_sparse_contents')

>   */
>  static int check_dir_in_index(const char *name)
>  {

Re: [PATCH v1 2/7] mv: add documentation for check_dir_in_index()

[Victoria Dye]

Victoria Dye wrote:
> Shaoxuan Yuan wrote:
>> Using check_dir_in_index without checking if the directory is on-disk
>> could get a false positive for partially sparsified directory.
>>
>> Add a note in the documentation to warn potential user.
>>
>> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
>> ---
>>  builtin/mv.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/builtin/mv.c b/builtin/mv.c
>> index 4729bb1a1a..c8b9069db8 100644
>> --- a/builtin/mv.c
>> +++ b/builtin/mv.c
>> @@ -132,6 +132,11 @@ static int index_range_of_same_dir(const char *src, int length,
>>   * 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.
>> + *
>> + * Note: *always* check the directory is not on-disk before this function
>> + * (i.e. using lstat());
>> + * otherwise it may return a false positive for a partially sparsified
>> + * directory.
> 
> To me, a comment like this indicates that either the function is not doing
> what it should be doing, or its name doesn't properly describe the
> function's behavior.
> 
> Per the function description:
> 
> 	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.
> 
> But neither the name of the function ('check_dir_in_index') nor the
> function's behavior (hence the comment you're adding here) match that
> description.
> 
> Since this function is intended to verify that a directory 1) exists in the
> index, and 2) is *entirely* sparse, I have two suggestions:
> 
> * Change the description to specify that the non-existence of the directory
>   on disk is an *assumption*, not an opportunity for a "false positive."
>   It's a subtle distinction, but it clarifies that the function should be
>   used only when the caller already knows the directory is empty. For
>   example:
> 
> 	/*
> 	 * Given the path of a directory that does not exist on-disk, check whether the
> 	 * directory contains any entries in the index with the SKIP_WORKTREE flag 
> 	 * enabled.
> 	 *
> 	 * Return 1 if such index entries exist.
> 	 * Return 0 otherwise.
> 	 */

Whoops, I mixed up the return values (I assumed this returned a boolean
based on the 'check' in the function name). In that case...
> 
> * 'check_dir_in_index' doesn't reflect the "is not on disk" prerequisite, so
>   the function name can be updated to clarify that (e.g.,
>   'empty_dir_has_sparse_contents')

...there are two options. Either you can use a more "boolean-y" name (like
the one I suggest above) and flip the return values (where "1" means "the
empty dir has sparse contents"), or change the name to something that
explicitly *doesn't* imply boolean. I'm personally in favor of the former
(I'm really struggling to come up with a descriptive, non-boolean name for
this function), but I'm fine with either if you can come up with a good
function name.

> 
>>   */
>>  static int check_dir_in_index(const char *name)
>>  {
> 

Re: [PATCH v1 2/7] mv: add documentation for check_dir_in_index()

[Shaoxuan Yuan]

On 7/20/2022 2:01 AM, Victoria Dye wrote:
 > Shaoxuan Yuan wrote:
 >> Using check_dir_in_index without checking if the directory is on-disk
 >> could get a false positive for partially sparsified directory.
 >>
 >> Add a note in the documentation to warn potential user.
 >>
 >> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
 >> ---
 >>  builtin/mv.c | 5 +++++
 >>  1 file changed, 5 insertions(+)
 >>
 >> diff --git a/builtin/mv.c b/builtin/mv.c
 >> index 4729bb1a1a..c8b9069db8 100644
 >> --- a/builtin/mv.c
 >> +++ b/builtin/mv.c
 >> @@ -132,6 +132,11 @@ static int index_range_of_same_dir(const char 
*src, int length,
 >>   * 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.
 >> + *
 >> + * Note: *always* check the directory is not on-disk before this 
function
 >> + * (i.e. using lstat());
 >> + * otherwise it may return a false positive for a partially sparsified
 >> + * directory.
 >
 > To me, a comment like this indicates that either the function is not 
doing
 > what it should be doing, or its name doesn't properly describe the
 > function's behavior.
 >
 > Per the function description:
 >
 >     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.
 >
 > But neither the name of the function ('check_dir_in_index') nor the
 > function's behavior (hence the comment you're adding here) match that
 > description.
 >
 > Since this function is intended to verify that a directory 1) exists 
in the
 > index, and 2) is *entirely* sparse, I have two suggestions:
 >
 > * Change the description to specify that the non-existence of the 
directory
 >   on disk is an *assumption*, not an opportunity for a "false positive."
 >   It's a subtle distinction, but it clarifies that the function should be
 >   used only when the caller already knows the directory is empty. For
 >   example:
 >
 >     /*
 >      * Given the path of a directory that does not exist on-disk, 
check whether the
 >      * directory contains any entries in the index with the 
SKIP_WORKTREE flag
 >      * enabled.
 >      *
 >      * Return 1 if such index entries exist.
 >      * Return 0 otherwise.
 >      */
 >
 > * 'check_dir_in_index' doesn't reflect the "is not on disk" 
prerequisite, so
 >   the function name can be updated to clarify that (e.g.,
 >   'empty_dir_has_sparse_contents')
 >

I really breathed a sigh of relief after seeing these two points :-)
They word things out much better than the original ones.

--
Thanks,
Shaoxuan

[PATCH v1 3/7] mv: free the *with_slash in check_dir_in_index()

[Shaoxuan Yuan]

*with_slash may be a malloc'd pointer, and when it is, free it.

Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index c8b9069db8..23a297d6b8 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -140,6 +140,7 @@ static int index_range_of_same_dir(const char *src, int length,
  */
 static int check_dir_in_index(const char *name)
 {
+	int ret = 1;
 	const char *with_slash = add_slash(name);
 	int length = strlen(with_slash);
 
@@ -149,14 +150,18 @@ static int check_dir_in_index(const char *name)
 	if (pos < 0) {
 		pos = -pos - 1;
 		if (pos >= the_index.cache_nr)
-			return 1;
+			goto free_return;
 		ce = active_cache[pos];
 		if (strncmp(with_slash, ce->name, length))
-			return 1;
+			goto free_return;
 		if (ce_skip_worktree(ce))
-			return 0;
+			ret = 0;
 	}
-	return 1;
+
+free_return:
+	if (with_slash != name)
+		free((char *)with_slash);
+	return ret;
 }
 
 int cmd_mv(int argc, const char **argv, const char *prefix)
-- 
2.37.0

Re: [PATCH v1 3/7] mv: free the *with_slash in check_dir_in_index()

[Derrick Stolee]

On 7/19/2022 9:28 AM, Shaoxuan Yuan wrote:> *with_slash may be a malloc'd pointer, and when it is, free it.
It might be worth mentioning that you are reorganizing the
method to use gotos.

>  static int check_dir_in_index(const char *name)
>  {
> +	int ret = 1;

Interesting to "assume error", but that works since there is only one
place where the success is guaranteed.

Should we use -1 here?

>  		if (ce_skip_worktree(ce))
> -			return 0;
> +			ret = 0;

This could be

	ret = ce_skip_worktree(ce);

unless you really rely on the "1" value.

>  	}
> -	return 1;
> +
> +free_return:
> +	if (with_slash != name)
> +		free((char *)with_slash);

Good check to not clear 'name'. It's unfortunate that we need to cast here,
but it makes sense.

Thanks,
-Stolee

[PATCH v1 4/7] mv: check if <destination> is a SKIP_WORKTREE_DIR

[Shaoxuan Yuan]

Originally, <destination> is assumed to be in the working tree. If it is
not found as a directory, then it is determined to be either a regular file
path, or error out if used under the second form (move into a directory)
of 'git-mv'. Such behavior is not ideal, mainly because Git does not
look into the index for <destination>, which could potentially be a
SKIP_WORKTREE_DIR, which we need to determine for the later "moving from
in-cone to out-of-cone" patch.

Change the logic so that Git first check if <destination> is a directory
with all its contents sparsified (a SKIP_WORKTREE_DIR). If yes,
then treat <destination> as a directory exists in the working tree, and
thus using the second form of 'git-mv', i.e. move into this
<destination>, and mark <destination> as a SKIP_WORKTREE_DIR.
If no, continue the original checking logic.

Also add a `dst_w_slash` to reuse the result from `add_slash()`, which
was everywhere and can be simplified.

Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 23a297d6b8..2e9d577227 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -178,7 +178,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 *modes;
+	const char *dst_w_slash;
+	enum update_mode *modes, dst_mode = 0;
 	struct stat st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
 	struct lock_file lock_file = LOCK_INIT;
@@ -208,6 +209,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	if (argc == 1 && is_directory(argv[0]) && !is_directory(argv[1]))
 		flags = 0;
 	dest_path = internal_prefix_pathspec(prefix, argv + argc, 1, flags);
+	dst_w_slash = add_slash(dest_path[0]);
 	submodule_gitfile = xcalloc(argc, sizeof(char *));
 
 	if (dest_path[0][0] == '\0')
@@ -215,13 +217,20 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
 	else if (!lstat(dest_path[0], &st) &&
 			S_ISDIR(st.st_mode)) {
-		dest_path[0] = add_slash(dest_path[0]);
-		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
+		destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
 	} else {
-		if (argc != 1)
+		if (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
+		    !check_dir_in_index(dst_w_slash)) {
+			destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
+			dst_mode |= SKIP_WORKTREE_DIR;
+		} else if (argc != 1) {
 			die(_("destination '%s' is not a directory"), dest_path[0]);
-		destination = dest_path;
+		} else {
+			destination = dest_path;
+		}
 	}
+	if (dst_w_slash != dest_path[0])
+		free((char *)dst_w_slash);
 
 	/* Checking */
 	for (i = 0; i < argc; i++) {
-- 
2.37.0

Re: [PATCH v1 4/7] mv: check if <destination> is a SKIP_WORKTREE_DIR

[Derrick Stolee]

On 7/19/2022 9:28 AM, Shaoxuan Yuan wrote:
> Originally, <destination> is assumed to be in the working tree. If it is
> not found as a directory, then it is determined to be either a regular file
> path, or error out if used under the second form (move into a directory)
> of 'git-mv'. Such behavior is not ideal, mainly because Git does not
> look into the index for <destination>, which could potentially be a
> SKIP_WORKTREE_DIR, which we need to determine for the later "moving from
> in-cone to out-of-cone" patch.
> 
> Change the logic so that Git first check if <destination> is a directory
> with all its contents sparsified (a SKIP_WORKTREE_DIR). If yes,
> then treat <destination> as a directory exists in the working tree, and

"treat <destination> as a directory exists in the working tree" is a bit
akward, and the rest of the sentence struggles with that. Starting with
"If yes," we could rewrite as follows:

  If <destination> is such a sparse directory, then we should modify the
  index the same way as we would if this were a non-sparse directory. We
  must be careful to ensure that the <destination> is marked with 
  SKIP_WORKTREE_DIR.

(Note that I don't equate this as doing the same thing, just operating on
the index.)

> If no, continue the original checking logic.

I think this part doesn't need to be there, but I don't feel strongly
about it.

> Also add a `dst_w_slash` to reuse the result from `add_slash()`, which
> was everywhere and can be simplified.

>  	else if (!lstat(dest_path[0], &st) &&
>  			S_ISDIR(st.st_mode)) {
> -		dest_path[0] = add_slash(dest_path[0]);
> -		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
> +		destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);

Hm. I find it interesting how this works even if the directory is _not_
present in the index. Is there a test that checks this kind of setup?

	git init &&
	>file &&
	git add file &&
	git commit -m init &&
	mkdir dir &&
	git mv file dir/

Locally, this indeed works with the following 'git status' detail:

        renamed:    file -> dir/file

>  	} else {
> -		if (argc != 1)
> +		if (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
> +		    !check_dir_in_index(dst_w_slash)) {
> +			destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
> +			dst_mode |= SKIP_WORKTREE_DIR;

Looks like we are assigning dst_mode here, but not using it. I wonder if
you'll get a compiler error if you build this patch with DEVELOPER=1.

You can test all of the commits in your series using this command:

  git rebase -x "make -j12 DEVELOPER=1" <base>

> +	if (dst_w_slash != dest_path[0])
> +		free((char *)dst_w_slash);

Good that you are freeing this here. You might also want to set it to NULL
just in case.

Thanks,
-Stolee

Re: [PATCH v1 4/7] mv: check if <destination> is a SKIP_WORKTREE_DIR

[Shaoxuan Yuan]

On 7/20/2022 1:59 AM, Derrick Stolee wrote:
 > On 7/19/2022 9:28 AM, Shaoxuan Yuan wrote:
 >> Originally, <destination> is assumed to be in the working tree. If it is
 >> not found as a directory, then it is determined to be either a 
regular file
 >> path, or error out if used under the second form (move into a directory)
 >> of 'git-mv'. Such behavior is not ideal, mainly because Git does not
 >> look into the index for <destination>, which could potentially be a
 >> SKIP_WORKTREE_DIR, which we need to determine for the later "moving from
 >> in-cone to out-of-cone" patch.
 >>
 >> Change the logic so that Git first check if <destination> is a directory
 >> with all its contents sparsified (a SKIP_WORKTREE_DIR). If yes,
 >> then treat <destination> as a directory exists in the working tree, and
 >
 > "treat <destination> as a directory exists in the working tree" is a bit
 > akward, and the rest of the sentence struggles with that. Starting with
 > "If yes," we could rewrite as follows:
 >
 >   If <destination> is such a sparse directory, then we should modify the
 >   index the same way as we would if this were a non-sparse directory. We
 >   must be careful to ensure that the <destination> is marked with
 >   SKIP_WORKTREE_DIR.
 >
 > (Note that I don't equate this as doing the same thing, just operating on
 > the index.)

This wording sounds more natural, thanks for the rewrite!

 >> If no, continue the original checking logic.
 >
 > I think this part doesn't need to be there, but I don't feel strongly
 > about it.
 >
 >> Also add a `dst_w_slash` to reuse the result from `add_slash()`, which
 >> was everywhere and can be simplified.
 >
 >>      else if (!lstat(dest_path[0], &st) &&
 >>              S_ISDIR(st.st_mode)) {
 >> -        dest_path[0] = add_slash(dest_path[0]);
 >> -        destination = internal_prefix_pathspec(dest_path[0], argv, 
argc, DUP_BASENAME);
 >> +        destination = internal_prefix_pathspec(dst_w_slash, argv, 
argc, DUP_BASENAME);
 >
 > Hm. I find it interesting how this works even if the directory is _not_
 > present in the index. Is there a test that checks this kind of setup?
 >
 >     git init &&
 >     >file &&
 >     git add file &&
 >     git commit -m init &&
 >     mkdir dir &&
 >     git mv file dir/
 >
 > Locally, this indeed works with the following 'git status' detail:
 >
 >         renamed:    file -> dir/file

In my impression, 'git-mv' does not seem to care about whether the
<destination> directory is in index? Given that `rename()` works so
long as the directory is in the working tree, and `rename_index_entry_at()`
cares even less about the <destination> dir?

 >>      } else {
 >> -        if (argc != 1)
 >> +        if (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
 >> +            !check_dir_in_index(dst_w_slash)) {
 >> +            destination = internal_prefix_pathspec(dst_w_slash, 
argv, argc, DUP_BASENAME);
 >> +            dst_mode |= SKIP_WORKTREE_DIR;
 >
 > Looks like we are assigning dst_mode here, but not using it. I wonder if
 > you'll get a compiler error if you build this patch with DEVELOPER=1.
 >
 > You can test all of the commits in your series using this command:
 >
 >   git rebase -x "make -j12 DEVELOPER=1" <base>

Oops, it seems that I was doing the wrong way. I only run test on the
tip of the branch, without actually testing individual commits.
Will do this.

 >> +    if (dst_w_slash != dest_path[0])
 >> +        free((char *)dst_w_slash);
 >
 > Good that you are freeing this here. You might also want to set it to 
NULL
 > just in case.

I was using the `FREE_AND_NULL()` macro, but I wasn't sure since other
places in 'git-mv' only use `free()`. Though I think it is better to
`FREE_AND_NULL()`.

--
Thanks,
Shaoxuan

Re: [PATCH v1 4/7] mv: check if <destination> is a SKIP_WORKTREE_DIR

[Derrick Stolee]

On 7/21/2022 10:13 AM, Shaoxuan Yuan wrote:
> On 7/20/2022 1:59 AM, Derrick Stolee wrote:
>> On 7/19/2022 9:28 AM, Shaoxuan Yuan wrote:
>>> Also add a `dst_w_slash` to reuse the result from `add_slash()`, which
>>> was everywhere and can be simplified.
>>
>>>      else if (!lstat(dest_path[0], &st) &&
>>>              S_ISDIR(st.st_mode)) {
>>> -        dest_path[0] = add_slash(dest_path[0]);
>>> -        destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
>>> +        destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
>>
>> Hm. I find it interesting how this works even if the directory is _not_
>> present in the index. Is there a test that checks this kind of setup?
>>
>>     git init &&
>>     >file &&
>>     git add file &&
>>     git commit -m init &&
>>     mkdir dir &&
>>     git mv file dir/
>>
>> Locally, this indeed works with the following 'git status' detail:
>>
>>         renamed:    file -> dir/file
> 
> In my impression, 'git-mv' does not seem to care about whether the
> <destination> directory is in index? Given that `rename()` works so
> long as the directory is in the working tree, and `rename_index_entry_at()`
> cares even less about the <destination> dir?

Right. Instead of changing the behavior, I'm asking you to double-check that
this behavior is tested. If not, then please add a test.

>>> +    if (dst_w_slash != dest_path[0])
>>> +        free((char *)dst_w_slash);
>>
>> Good that you are freeing this here. You might also want to set it to NULL
>> just in case.
> 
> I was using the `FREE_AND_NULL()` macro, but I wasn't sure since other
> places in 'git-mv' only use `free()`. Though I think it is better to
> `FREE_AND_NULL()`.

free() is generally the way to go if it is clear that the variable
is about to go out-of-scope and could not possibly be referenced
again. Since there is a lot more of the current code block to go,
nulling the variable is good defensive programming.

Thanks,
-Stolee

Re: [PATCH v1 4/7] mv: check if <destination> is a SKIP_WORKTREE_DIR

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

>>> Good that you are freeing this here. You might also want to set it to NULL
>>> just in case.
>> 
>> I was using the `FREE_AND_NULL()` macro, but I wasn't sure since other
>> places in 'git-mv' only use `free()`. Though I think it is better to
>> `FREE_AND_NULL()`.
>
> free() is generally the way to go if it is clear that the variable
> is about to go out-of-scope and could not possibly be referenced
> again. Since there is a lot more of the current code block to go,
> nulling the variable is good defensive programming.

NULLing it out is better when a potential misuse of the pointer
after it got freed will be caught by dereferencing NULL.

There however are pointer members of structures wher they represent
optional data.  Access to such a member goes like so:

	if (structure->optinal_member)
		do_things(structure->optional_member);

When you are done using such a structure and clearing it, after
releasing the resource held by the member, it is better to leave it
dangling than assigning NULL to it.  If somebody reuses that
structure and the control enters a codepath like the above one to
use the "optional" pointer, uncleared dangling pointer will likely
be caught at runtime; setting it to NULL will paper over it.  We've
seen many bugs caused by a premature releasing of a member that was
hidden exactly by such a use of FREE_AND_NULL() few relases ago.

Thanks.

[PATCH v1 5/7] mv: remove BOTH from enum update_mode

[Shaoxuan Yuan]

Since BOTH is not practically used anywhere in the code and its meaning
is unclear, remove it.

Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 2e9d577227..d05914a233 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -21,7 +21,6 @@ static const char * const builtin_mv_usage[] = {
 };
 
 enum update_mode {
-	BOTH = 0,
 	WORKING_DIRECTORY = (1 << 1),
 	INDEX = (1 << 2),
 	SPARSE = (1 << 3),
-- 
2.37.0

Re: [PATCH v1 5/7] mv: remove BOTH from enum update_mode

[Derrick Stolee]

On 7/19/2022 9:28 AM, Shaoxuan Yuan wrote:
> Since BOTH is not practically used anywhere in the code and its meaning
> is unclear, remove it.

It's literally not used any where. (I would just drop "practically".)

Also, now that enum update_mode is a flag-based enum, the 0 value cannot
have any meaning to it except "none of the other flags are on".
 
Thanks,
-Stolee

[PATCH v1 6/7] mv: from in-cone to out-of-cone

[Shaoxuan Yuan]

Originally, moving an in-cone <source> to an out-of-cone <destination>
was not possible, mainly because such <destination> is a directory that
is not present in the working tree.

Change the behavior so that we can move an in-cone <source> to
out-of-cone <destination> when --sparse is supplied.

Such <source> can be either clean or dirty, and moving it results in
different behaviors:

A clean move should move the <source> to the <destination>, both in the
working tree and the index, then remove the resulted path from the
working tree, and turn on its CE_SKIP_WORKTREE bit.

A dirty move should move the <source> to the <destination>, both in the
working tree and the index, but should *not* remove the resulted path
from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit.
Instead advise user to "git add" this path and run "git sparse-checkout
reapply" to re-sparsify that path.

Helped-by: Victoria Dye <vdye@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 advice.c                      | 19 +++++++++++++
 advice.h                      |  1 +
 builtin/mv.c                  | 50 ++++++++++++++++++++++++++++++-----
 t/t7002-mv-sparse-checkout.sh |  8 +++---
 4 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/advice.c b/advice.c
index 6fda9edbc2..fd18968943 100644
--- a/advice.c
+++ b/advice.c
@@ -261,3 +261,22 @@ void detach_advice(const char *new_name)
 
 	fprintf(stderr, fmt, new_name);
 }
+
+void advise_on_moving_dirty_path(struct string_list *pathspec_list)
+{
+	struct string_list_item *item;
+
+	if (!pathspec_list->nr)
+		return;
+
+	fprintf(stderr, _("The following paths have been moved outside the\n"
+			  "sparse-checkout definition but are not sparse due to local\n"
+			  "modifications.\n"));
+	for_each_string_list_item(item, pathspec_list)
+		fprintf(stderr, "%s\n", item->string);
+
+	advise_if_enabled(ADVICE_UPDATE_SPARSE_PATH,
+			  _("To correct the sparsity of these paths, do the following:\n"
+			    "* Use \"git add --sparse <paths>\" to update the index\n"
+			    "* Use \"git sparse-checkout reapply\" to apply the sparsity rules"));
+}
diff --git a/advice.h b/advice.h
index 7ddc6cbc1a..07e0f76833 100644
--- a/advice.h
+++ b/advice.h
@@ -74,5 +74,6 @@ void NORETURN die_conclude_merge(void);
 void NORETURN die_ff_impossible(void);
 void advise_on_updating_sparse_paths(struct string_list *pathspec_list);
 void detach_advice(const char *new_name);
+void advise_on_moving_dirty_path(struct string_list *pathspec_list);
 
 #endif /* ADVICE_H */
diff --git a/builtin/mv.c b/builtin/mv.c
index d05914a233..d35994c443 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -184,6 +184,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	struct lock_file lock_file = LOCK_INIT;
 	struct cache_entry *ce;
 	struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
+	struct string_list dirty_paths = STRING_LIST_INIT_NODUP;
 
 	git_config(git_default_config, NULL);
 
@@ -414,6 +415,7 @@ 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;
+		int up_to_date = 0;
 		struct checkout state = CHECKOUT_INIT;
 		state.istate = &the_index;
 
@@ -424,6 +426,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		if (show_only)
 			continue;
 		if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
+		    !(dst_mode & SKIP_WORKTREE_DIR) &&
 		    rename(src, dst) < 0) {
 			if (ignore_errors)
 				continue;
@@ -443,20 +446,52 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 
 		pos = cache_name_pos(src, strlen(src));
 		assert(pos >= 0);
+		if (!(mode & SPARSE) && !lstat(src, &st))
+			up_to_date = !ce_modified(active_cache[pos], &st, 0);
 		rename_cache_entry_at(pos, dst);
 
-		if ((mode & SPARSE) &&
-		    (path_in_sparse_checkout(dst, &the_index))) {
-			int dst_pos;
+		if (ignore_sparse &&
+		    core_apply_sparse_checkout &&
+		    core_sparse_checkout_cone) {
 
-			dst_pos = cache_name_pos(dst, strlen(dst));
-			active_cache[dst_pos]->ce_flags &= ~CE_SKIP_WORKTREE;
+			/* from out-of-cone to in-cone */
+			if ((mode & SPARSE) &&
+			    path_in_sparse_checkout(dst, &the_index)) {
+				int dst_pos = cache_name_pos(dst, strlen(dst));
+				struct cache_entry *dst_ce = active_cache[dst_pos];
 
-			if (checkout_entry(active_cache[dst_pos], &state, NULL, NULL))
-				die(_("cannot checkout %s"), active_cache[dst_pos]->name);
+				dst_ce->ce_flags &= ~CE_SKIP_WORKTREE;
+
+				if (checkout_entry(dst_ce, &state, NULL, NULL))
+					die(_("cannot checkout %s"), dst_ce->name);
+				continue;
+			}
+
+			/* from in-cone to out-of-cone */
+			if ((dst_mode & SKIP_WORKTREE_DIR) &&
+			    !(mode & SPARSE) &&
+			    !path_in_sparse_checkout(dst, &the_index)) {
+				int dst_pos = cache_name_pos(dst, strlen(dst));
+				struct cache_entry *dst_ce = active_cache[dst_pos];
+				char *src_dir = dirname(xstrdup(src));
+
+				if (up_to_date) {
+					dst_ce->ce_flags |= CE_SKIP_WORKTREE;
+					unlink_or_warn(src);
+				} else {
+					string_list_append(&dirty_paths, dst);
+					safe_create_leading_directories(xstrdup(dst));
+					rename(src, dst);
+				}
+				if ((mode & INDEX) && is_empty_dir(src_dir))
+					rmdir_or_warn(src_dir);
+			}
 		}
 	}
 
+	if (dirty_paths.nr)
+		advise_on_moving_dirty_path(&dirty_paths);
+
 	if (gitmodules_modified)
 		stage_updated_gitmodules(&the_index);
 
@@ -465,6 +500,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		die(_("Unable to write new index file"));
 
 	string_list_clear(&src_for_dst, 0);
+	string_list_clear(&dirty_paths, 0);
 	UNLEAK(source);
 	UNLEAK(dest_path);
 	free(submodule_gitfile);
diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
index c27fcdbfd0..dafe15b9cf 100755
--- a/t/t7002-mv-sparse-checkout.sh
+++ b/t/t7002-mv-sparse-checkout.sh
@@ -303,7 +303,7 @@ test_expect_success 'move sparse file to existing destination with --force and -
 	test_cmp expect sub/file1
 '
 
-test_expect_failure 'move clean path from in-cone to out-of-cone' '
+test_expect_success 'move clean path from in-cone to out-of-cone' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 
@@ -356,7 +356,7 @@ test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'move dirty path from in-cone to out-of-cone' '
+test_expect_success 'move dirty path from in-cone to out-of-cone' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 	echo "modified" >>sub/d &&
@@ -380,7 +380,7 @@ test_expect_failure 'move dirty path from in-cone to out-of-cone' '
 	grep -x "H folder1/d" actual
 '
 
-test_expect_failure 'move dir from in-cone to out-of-cone' '
+test_expect_success 'move dir from in-cone to out-of-cone' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 
@@ -400,7 +400,7 @@ test_expect_failure 'move dir from in-cone to out-of-cone' '
 	grep -x "S folder1/dir/e" actual
 '
 
-test_expect_failure 'move partially-dirty dir from in-cone to out-of-cone' '
+test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 	touch sub/dir/e2 sub/dir/e3 &&
-- 
2.37.0

Re: [PATCH v1 6/7] mv: from in-cone to out-of-cone

[Derrick Stolee]

On 7/19/2022 9:28 AM, Shaoxuan Yuan wrote:
> Originally, moving an in-cone <source> to an out-of-cone <destination>
> was not possible, mainly because such <destination> is a directory that
> is not present in the working tree.
> 
> Change the behavior so that we can move an in-cone <source> to
> out-of-cone <destination> when --sparse is supplied.
> 
> Such <source> can be either clean or dirty, and moving it results in
> different behaviors:
> 
> A clean move should move the <source> to the <destination>, both in the
> working tree and the index, then remove the resulted path from the
> working tree, and turn on its CE_SKIP_WORKTREE bit.
> 
> A dirty move should move the <source> to the <destination>, both in the
> working tree and the index, but should *not* remove the resulted path
> from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit.
> Instead advise user to "git add" this path and run "git sparse-checkout
> reapply" to re-sparsify that path.

I feel like this patch should be two, instead: you can have one that
makes the commands succeed or fail properly, and another that outputs the
advice. As it is, it's a bit muddled as to what is necessary for the
movement of the index entry versus representing the error message.

This might mean that you want to pull the advice messages out of the tests
that are added in patch 1 and only apply those to the test as you implement
the advice in that later patch. Such a split of the test implementation
would allow this patch to still switch a bunch of test_expect_failure tests
to test_expect_success.

> @@ -424,6 +426,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		if (show_only)
>  			continue;
>  		if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
> +		    !(dst_mode & SKIP_WORKTREE_DIR) &&

Here is a use of dst_mode that might be helpful to put with the change in
patch 4. Alternatively, you could delay declaring dst_mode until now.

> +		if (!(mode & SPARSE) && !lstat(src, &st))
> +			up_to_date = !ce_modified(active_cache[pos], &st, 0);

Here, you are only checking for a dirty file if (mode & SPARSE) and the
file exists.

Perhaps it would be helpful to negate this and rename the variable?

	sparse_and_dirty = ce_modified(active_cache[pos], &st, 0);

This makes it clear that we can't use the variable except in this
particular case.

> -		if ((mode & SPARSE) &&
> -		    (path_in_sparse_checkout(dst, &the_index))) {
> -			int dst_pos;
> +		if (ignore_sparse &&
> +		    core_apply_sparse_checkout &&
> +		    core_sparse_checkout_cone) {
>  
> -			dst_pos = cache_name_pos(dst, strlen(dst));
> -			active_cache[dst_pos]->ce_flags &= ~CE_SKIP_WORKTREE;
> +			/* from out-of-cone to in-cone */
> +			if ((mode & SPARSE) &&
> +			    path_in_sparse_checkout(dst, &the_index)) {
> +				int dst_pos = cache_name_pos(dst, strlen(dst));
> +				struct cache_entry *dst_ce = active_cache[dst_pos];
>  
> -			if (checkout_entry(active_cache[dst_pos], &state, NULL, NULL))
> -				die(_("cannot checkout %s"), active_cache[dst_pos]->name);
> +				dst_ce->ce_flags &= ~CE_SKIP_WORKTREE;
> +
> +				if (checkout_entry(dst_ce, &state, NULL, NULL))
> +					die(_("cannot checkout %s"), dst_ce->name);
> +				continue;
> +			}

Here, it helps to ignore whitespace changes. This out to in was already
handled by the existing implementation.

> +			/* from in-cone to out-of-cone */
> +			if ((dst_mode & SKIP_WORKTREE_DIR) &&

This is disjoint from the other case (because of !path_in_sparse_checkout()),
so maybe we could short-circuit with an "else if" here? You could put your
comments about the in-to-out or out-to-in inside the if blocks.

> +			    !(mode & SPARSE) &&
> +			    !path_in_sparse_checkout(dst, &the_index)) {
> +				int dst_pos = cache_name_pos(dst, strlen(dst));
> +				struct cache_entry *dst_ce = active_cache[dst_pos];

(It took me a while to realize that dst_ce is the right entry because we
already called rename_cache_entry_at() earlier.)

> +				char *src_dir = dirname(xstrdup(src));

The xstrdup(src) return string is being lost here.

> +
> +				if (up_to_date) {

Based on my recommendation earlier, this would become

	if (!sparse_and_dirty) {

> +					dst_ce->ce_flags |= CE_SKIP_WORKTREE;
> +					unlink_or_warn(src);
> +				} else {
> +					string_list_append(&dirty_paths, dst);
> +					safe_create_leading_directories(xstrdup(dst));
> +					rename(src, dst);

Ok, still doing the file rename, but leaving it unstaged.

> +				}

Please provide some whitespace between the close of an if/else chain before
starting the next if.

> +				if ((mode & INDEX) && is_empty_dir(src_dir))
> +					rmdir_or_warn(src_dir);

This is an interesting cleanup of the first-level directory. Should it be
recursive and clean up an entire chain of paths?

Thanks,
-Stolee

Re: [PATCH v1 6/7] mv: from in-cone to out-of-cone

[Shaoxuan Yuan]

On 7/20/2022 2:14 AM, Derrick Stolee wrote:
 >> -        if ((mode & SPARSE) &&
 >> -            (path_in_sparse_checkout(dst, &the_index))) {
 >> -            int dst_pos;
 >> +        if (ignore_sparse &&
 >> +            core_apply_sparse_checkout &&
 >> +            core_sparse_checkout_cone) {
 >>
 >> -            dst_pos = cache_name_pos(dst, strlen(dst));
 >> -            active_cache[dst_pos]->ce_flags &= ~CE_SKIP_WORKTREE;
 >> +            /* from out-of-cone to in-cone */
 >> +            if ((mode & SPARSE) &&
 >> +                path_in_sparse_checkout(dst, &the_index)) {
 >> +                int dst_pos = cache_name_pos(dst, strlen(dst));
 >> +                struct cache_entry *dst_ce = active_cache[dst_pos];
 >>
 >> -            if (checkout_entry(active_cache[dst_pos], &state, NULL, 
NULL))
 >> -                die(_("cannot checkout %s"), 
active_cache[dst_pos]->name);
 >> +                dst_ce->ce_flags &= ~CE_SKIP_WORKTREE;
 >> +
 >> +                if (checkout_entry(dst_ce, &state, NULL, NULL))
 >> +                    die(_("cannot checkout %s"), dst_ce->name);
 >> +                continue;
 >> +            }
 >
 > Here, it helps to ignore whitespace changes. This out to in was already
 > handled by the existing implementation.

Yes, I think it would be better to let `diff` ignore the existing
implementation. Are you suggesting the `-w` (--ignore-all-space) option
of `diff`? I tried this option and it does not work. But another reason
is that there *are* some changes that are different from the original
out-to-in implementation, so even though it looks a bit messy, I think
it makes sense.

 >> +            /* from in-cone to out-of-cone */
 >> +            if ((dst_mode & SKIP_WORKTREE_DIR) &&
 >
 > This is disjoint from the other case (because of 
!path_in_sparse_checkout()),
 > so maybe we could short-circuit with an "else if" here? You could put 
your
 > comments about the in-to-out or out-to-in inside the if blocks.

I tried an else-if but it does clutter the code a bit. I think I'll
leave it as-is. Or do you mind show me a diff of your approach? To be
honest, this disjoint here looks logically cleaner to me.

--
Thanks,
Shaoxuan

Re: [PATCH v1 6/7] mv: from in-cone to out-of-cone

[Derrick Stolee]

On 8/3/2022 7:50 AM, Shaoxuan Yuan wrote:
> On 7/20/2022 2:14 AM, Derrick Stolee wrote:
>>> -        if ((mode & SPARSE) &&
>>> -            (path_in_sparse_checkout(dst, &the_index))) {
>>> -            int dst_pos;
>>> +        if (ignore_sparse &&
>>> +            core_apply_sparse_checkout &&
>>> +            core_sparse_checkout_cone) {
>>>
>>> -            dst_pos = cache_name_pos(dst, strlen(dst));
>>> -            active_cache[dst_pos]->ce_flags &= ~CE_SKIP_WORKTREE;
>>> +            /* from out-of-cone to in-cone */
>>> +            if ((mode & SPARSE) &&
>>> +                path_in_sparse_checkout(dst, &the_index)) {
>>> +                int dst_pos = cache_name_pos(dst, strlen(dst));
>>> +                struct cache_entry *dst_ce = active_cache[dst_pos];
>>>
>>> -            if (checkout_entry(active_cache[dst_pos], &state, NULL, NULL))
>>> -                die(_("cannot checkout %s"), active_cache[dst_pos]->name);
>>> +                dst_ce->ce_flags &= ~CE_SKIP_WORKTREE;
>>> +
>>> +                if (checkout_entry(dst_ce, &state, NULL, NULL))
>>> +                    die(_("cannot checkout %s"), dst_ce->name);
>>> +                continue;
>>> +            }
>>
>> Here, it helps to ignore whitespace changes. This out to in was already
>> handled by the existing implementation.
> 
> Yes, I think it would be better to let `diff` ignore the existing
> implementation. Are you suggesting the `-w` (--ignore-all-space) option
> of `diff`? I tried this option and it does not work. But another reason
> is that there *are* some changes that are different from the original
> out-to-in implementation, so even though it looks a bit messy, I think
> it makes sense.

I'm just making a note that I looked at this not in its patch form,
but as a commit diff where I could use the '-w' option to get a nice
view. It's not possible to do that in the patch.
 
>>> +            /* from in-cone to out-of-cone */
>>> +            if ((dst_mode & SKIP_WORKTREE_DIR) &&
>>
>> This is disjoint from the other case (because of !path_in_sparse_checkout()),
>> so maybe we could short-circuit with an "else if" here? You could put your
>> comments about the in-to-out or out-to-in inside the if blocks.
> 
> I tried an else-if but it does clutter the code a bit. I think I'll
> leave it as-is. Or do you mind show me a diff of your approach? To be
> honest, this disjoint here looks logically cleaner to me.

Here's the diff I had in mind:

--- >8 ---

diff --git a/builtin/mv.c b/builtin/mv.c
index df1f69f1a7..111aafb69a 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -455,10 +455,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		if (ignore_sparse &&
 		    core_apply_sparse_checkout &&
 		    core_sparse_checkout_cone) {
-
-			/* from out-of-cone to in-cone */
 			if ((mode & SPARSE) &&
 			    path_in_sparse_checkout(dst, &the_index)) {
+				/* from out-of-cone to in-cone */
 				int dst_pos = cache_name_pos(dst, strlen(dst));
 				struct cache_entry *dst_ce = active_cache[dst_pos];
 
@@ -466,13 +465,10 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 
 				if (checkout_entry(dst_ce, &state, NULL, NULL))
 					die(_("cannot checkout %s"), dst_ce->name);
-				continue;
-			}
-
-			/* from in-cone to out-of-cone */
-			if ((dst_mode & SKIP_WORKTREE_DIR) &&
-			    !(mode & SPARSE) &&
-			    !path_in_sparse_checkout(dst, &the_index)) {
+			} else if ((dst_mode & SKIP_WORKTREE_DIR) &&
+				   !(mode & SPARSE) &&
+				   !path_in_sparse_checkout(dst, &the_index)) {
+				/* from in-cone to out-of-cone */
 				int dst_pos = cache_name_pos(dst, strlen(dst));
 				struct cache_entry *dst_ce = active_cache[dst_pos];
 				char *src_dir = dirname(xstrdup(src));

--- >8 ---

I agree with you that the whitespace breaking the two cases is nice,
but relying on that "continue;" to keep these cases disjoint is easy
to miss and I'd rather let the code be clearer about the cases.

Thanks,
-Stolee

Re: [PATCH v1 6/7] mv: from in-cone to out-of-cone

[Shaoxuan Yuan]

On 7/20/2022 2:14 AM, Derrick Stolee wrote:
 >> +                if ((mode & INDEX) && is_empty_dir(src_dir))
 >> +                    rmdir_or_warn(src_dir);
 >
 > This is an interesting cleanup of the first-level directory. Should it be
 > recursive and clean up an entire chain of paths?

Indeed. I'm planning to use an array to record the possible `src_dir`,
i.e. WORKING_DIRECTORYs in `git-mv`, and use a loop to cleanup the
empty directories recursively.

--
Thanks,
Shaoxuan

[PATCH v1 7/7] mv: check overwrite for in-to-out move

[Shaoxuan Yuan]

Add checking logic for overwritng when moving from in-cone to
out-of-cone. It is the index version of the original overwrite logic.

Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c                  | 12 ++++++++++++
 t/t7002-mv-sparse-checkout.sh |  2 +-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index d35994c443..5ed3bd3431 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -365,6 +365,18 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			goto act_on_entry;
 		}
 
+		if (ignore_sparse &&
+		    (dst_mode & SKIP_WORKTREE_DIR) &&
+		    index_entry_exists(&the_index, dst, strlen(dst))) {
+			bad = _("destination exists in the index");
+			if (force) {
+				if (verbose)
+					warning(_("overwriting '%s'"), dst);
+				bad = NULL;
+			} else {
+				goto act_on_entry;
+			}
+		}
 		/*
 		 * We check if the paths are in the sparse-checkout
 		 * definition as a very final check, since that
diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
index dafe15b9cf..5d810f3af0 100755
--- a/t/t7002-mv-sparse-checkout.sh
+++ b/t/t7002-mv-sparse-checkout.sh
@@ -323,7 +323,7 @@ test_expect_success 'move clean path from in-cone to out-of-cone' '
 	grep -x "S folder1/d" actual
 '
 
-test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
+test_expect_success 'move clean path from in-cone to out-of-cone overwrite' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 	echo "sub/file1 overwrite" >sub/file1 &&
-- 
2.37.0

Re: [PATCH v1 7/7] mv: check overwrite for in-to-out move

[Derrick Stolee]

On 7/19/2022 9:28 AM, Shaoxuan Yuan wrote:
> Add checking logic for overwritng when moving from in-cone to

s/overwritng/overwriting/

> out-of-cone. It is the index version of the original overwrite logic.

Code looks good to me.

Thanks,
-Stolee

Re: [PATCH v1 0/7] mv: from in-cone to out-of-cone

[Derrick Stolee]

On 7/19/2022 9:28 AM, Shaoxuan Yuan wrote:
> This is a sister series to the previous "from out-of-cone to in-cone" [1]
> series. This series is trying to make the opposite operation possible for
> 'mv', namely move <source>, which is in-cone, to <destination>, which is
> out-of-cone.
> 
> Other than the main task, there are also some minor fixes done.

I'm happy to see that the main task is less complicated than the previous
case, because of your hard work refactoring some things.

I still have some ideas about how to break up the change to be a bit more
readable. Most of my comments on this v1 are nits, though. Good work!

Thanks,
-Stolee

[PATCH v2 0/9] mv: from in-cone to out-of-cone

[Shaoxuan Yuan]

## Changes since PATCH v1 ##

1. Change "grep -x" to ^ and $ in t7002 test, and remove some
   useless "-x" (lines that are *not* ambiguous).

2. Rename check_dir_in_index() to empty_dir_has_sparse_contents()
   and add more precise documentation.

3. Reverse return values from empty_dir_has_sparse_contents() to
   comply with the method's name.

4. Make some commit messages more natural/fluent/without typos.

5. Split commit "mv: from in-cone to out-of-cone" to two commits,
   one does in-to-out functionality, the other one does advice.

6. Take care a few memory leaks (`xstrdup`s).

7. Add a new way to recursively cleanup empty WORKING_DIRECTORY.

## leftover bits ##

I decided *not* to solve these bits in v2, but in a future v3,
so things can be handled one at a time, not exceeding my capability.

1. When trying to move from-in-to-out, the mechanism assumes that
   the <destination> is out-of-cone, and by cone-mode definition,
   <destination> is basically an invisible directory 
   (SKIP_WORKTREE_DIR). However, the dirty move mechanism materializes
   the moved file to make sure the information is not lost, and this
   mechanism brings the invisible directory back into the worktree.
   Hence, if we want to perform a second move from-in-to-out, the
   assumption that <destination> is not on-disk is broken, and
   everything follows breaks too. 

2. A logic loophole introduced in the previous out-to-in patch,
   especially in b91a2b6594 (mv: add check_dir_in_index() and solve 
   general dir check issue). Please detach your head to b91a2b6594
   for context. Copy and paste: 
   
                    git switch b91a2b6594
   
   When moving from out-to-in, <source> is an invisible 
   SKIP_WORKTREE_DIR. Line 226 uses `lstat()` to make
   sure the <source> is not on-disk; then line 233-237 checks if
   <source> is a SKIP_WORKTREE_DIR. If the check passes, line 236
   jump to line 276 and try to verify <source> is a directory using
   `S_ISDIR`. However, because the prerequisite is that the line 226
   `lstat()` fails so we can go through the steps said above; and when
   it fails, the `st` stat, especially the `st_mode` member, is either
   an uninitialized chunk of garbage, or the result from previous
   `lstat()`. In this case, `st_mode` *is* from a previous `lstat()`,
   on line 205. This `lstat()` is testing the <destination>, which,
   under the out-to-in situation, is always an on-disk directory.
   Thus, by a series of coincidence, the `S_ISDIR()` on line 276
   succeed and everything *looks* OK test-wise. But clearly the `st`
   at this point is an impostor: it does not reflect the actual stat
   situation of <source> and it luckily slips through.

   This needs to be fixed.

## PATCH v1 info ##

This is a sister series to the previous "from out-of-cone to in-cone" [1]
series. This series is trying to make the opposite operation possible for
'mv', namely move <source>, which is in-cone, to <destination>, which is
out-of-cone.

Other than the main task, there are also some minor fixes done.

[1] https://lore.kernel.org/git/20220331091755.385961-1-shaoxuan.yuan02@gmail.com/

Shaoxuan Yuan (9):
  t7002: add tests for moving from in-cone to out-of-cone
  mv: rename check_dir_in_index() to empty_dir_has_sparse_contents()
  mv: free the *with_slash in check_dir_in_index()
  mv: check if <destination> is a SKIP_WORKTREE_DIR
  mv: remove BOTH from enum update_mode
  mv: from in-cone to out-of-cone
  mv: cleanup empty WORKING_DIRECTORY
  advice.h: add advise_on_moving_dirty_path()
  mv: check overwrite for in-to-out move

 advice.c                      |  19 +++++
 advice.h                      |   1 +
 builtin/mv.c                  | 143 ++++++++++++++++++++++++++------
 t/t7002-mv-sparse-checkout.sh | 150 +++++++++++++++++++++++++++++++++-
 4 files changed, 286 insertions(+), 27 deletions(-)

Range-diff against v1:
 1:  31d1136d40 !  1:  5de0a2585f t7002: add tests for moving from in-cone to out-of-cone
    @@ Commit message
         A dirty move should move the <source> to the <destination>, both in the
         working tree and the index, but should *not* remove the resulted path
         from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit.
    -    Instead advise user to "git add" this path and run "git sparse-checkout
    -    reapply" to re-sparsify that path.
     
         Also make sure that if <destination> exists in the index (existing
         check for if <destination> is in the worktree is not enough in
         in-to-out moves), warn user against the overwrite. And Git should force
         the overwrite when supplied with -f or --force.
     
    +    Helped-by: Derrick Stolee <derrickstolee@github.com>
    +    Helped-by: Victoria Dye <vdye@github.com>
         Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
     
      ## t/t7002-mv-sparse-checkout.sh ##
    -@@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'setup' "
    - 	updated in the index:
    - 	EOF
    - 
    --	cat >sparse_hint <<-EOF
    -+	cat >sparse_hint <<-EOF &&
    - 	hint: If you intend to update such entries, try one of the following:
    - 	hint: * Use the --sparse option.
    - 	hint: * Disable or modify the sparsity rules.
    - 	hint: Disable this message with \"git config advice.updateSparsePath false\"
    - 	EOF
    -+
    -+	cat >dirty_error_header <<-EOF &&
    -+	The following paths have been moved outside the
    -+	sparse-checkout definition but are not sparse due to local
    -+	modifications.
    -+	EOF
    -+
    -+	cat >dirty_hint <<-EOF
    -+	hint: To correct the sparsity of these paths, do the following:
    -+	hint: * Use \"git add --sparse <paths>\" to update the index
    -+	hint: * Use \"git sparse-checkout reapply\" to apply the sparsity rules
    -+	hint: Disable this message with \"git config advice.updateSparsePath false\"
    -+	EOF
    - "
    - 
    - test_expect_success 'mv refuses to move sparse-to-sparse' '
     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move sparse file to existing destination with --force and -
      	test_cmp expect sub/file1
      '
    @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move sparse file to existing
     +	test_path_is_missing sub/d &&
     +	test_path_is_missing folder1/d &&
     +	git ls-files -t >actual &&
    -+	! grep -x "H sub/d" actual &&
    -+	grep -x "S folder1/d" actual
    ++	! grep "^H sub/d\$" actual &&
    ++	grep "S folder1/d" actual
     +'
     +
     +test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
    @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move sparse file to existing
     +	test_path_is_missing sub/file1 &&
     +	test_path_is_missing folder1/file1 &&
     +	git ls-files -t >actual &&
    -+	! grep -x "H sub/file1" actual &&
    -+	grep -x "S folder1/file1" actual &&
    ++	! grep "H sub/file1" actual &&
    ++	grep "S folder1/file1" actual &&
     +
     +	# compare file content before move and after move
     +	echo "sub/file1 overwrite" >expect &&
    @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move sparse file to existing
     +	test_cmp expect stderr &&
     +
     +	git mv --sparse sub/d folder1 2>stderr &&
    -+	cat dirty_error_header >expect &&
    -+	echo "folder1/d" >>expect &&
    -+	cat dirty_hint >>expect &&
    -+	test_cmp expect stderr &&
     +
     +	test_path_is_missing sub/d &&
     +	test_path_is_file folder1/d &&
     +	git ls-files -t >actual &&
    -+	! grep -x "H sub/d" actual &&
    -+	grep -x "H folder1/d" actual
    ++	! grep "^H sub/d\$" actual &&
    ++	grep "H folder1/d" actual
     +'
     +
     +test_expect_failure 'move dir from in-cone to out-of-cone' '
    @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move sparse file to existing
     +	git mv --sparse sub/dir folder1 2>stderr &&
     +	test_must_be_empty stderr &&
     +
    -+	test_path_is_missing sub/dir &&
     +	test_path_is_missing folder1 &&
     +	git ls-files -t >actual &&
    -+	! grep -x "H sub/dir/e" actual &&
    -+	grep -x "S folder1/dir/e" actual
    ++	! grep "H sub/dir/e" actual &&
    ++	grep "S folder1/dir/e" actual
     +'
     +
     +test_expect_failure 'move partially-dirty dir from in-cone to out-of-cone' '
    @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move sparse file to existing
     +	test_cmp expect stderr &&
     +
     +	git mv --sparse sub/dir folder1 2>stderr &&
    -+	cat dirty_error_header >expect &&
    -+	echo "folder1/dir/e2" >>expect &&
    -+	echo "folder1/dir/e3" >>expect &&
    -+	cat dirty_hint >>expect &&
    -+	test_cmp expect stderr &&
     +
    -+	test_path_is_missing sub/dir &&
     +	test_path_is_missing folder1/dir/e &&
     +	test_path_is_file folder1/dir/e2 &&
     +	test_path_is_file folder1/dir/e3 &&
     +	git ls-files -t >actual &&
    -+	! grep -x "H sub/dir/e" actual &&
    -+	! grep -x "H sub/dir/e2" actual &&
    -+	! grep -x "H sub/dir/e3" actual &&
    -+	grep -x "S folder1/dir/e" actual &&
    -+	grep -x "H folder1/dir/e2" actual &&
    -+	grep -x "H folder1/dir/e3" actual
    ++	! grep "H sub/dir/e" actual &&
    ++	! grep "H sub/dir/e2" actual &&
    ++	! grep "H sub/dir/e3" actual &&
    ++	grep "S folder1/dir/e" actual &&
    ++	grep "H folder1/dir/e2" actual &&
    ++	grep "H folder1/dir/e3" actual
     +'
     +
      test_done
 2:  df17173219 <  -:  ---------- mv: add documentation for check_dir_in_index()
 -:  ---------- >  2:  ebf8382f15 mv: rename check_dir_in_index() to empty_dir_has_sparse_contents()
 3:  61af1a2546 !  3:  26871b1070 mv: free the *with_slash in check_dir_in_index()
    @@ Commit message
     
         *with_slash may be a malloc'd pointer, and when it is, free it.
     
    +    Helped-by: Derrick Stolee <derrickstolee@github.com>
    +    Helped-by: Victoria Dye <vdye@github.com>
         Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
     
      ## builtin/mv.c ##
     @@ builtin/mv.c: static int index_range_of_same_dir(const char *src, int length,
       */
    - static int check_dir_in_index(const char *name)
    + static int empty_dir_has_sparse_contents(const char *name)
      {
    -+	int ret = 1;
    ++	int ret = 0;
      	const char *with_slash = add_slash(name);
      	int length = strlen(with_slash);
      
    -@@ builtin/mv.c: static int check_dir_in_index(const char *name)
    +@@ builtin/mv.c: static int empty_dir_has_sparse_contents(const char *name)
      	if (pos < 0) {
      		pos = -pos - 1;
      		if (pos >= the_index.cache_nr)
    --			return 1;
    +-			return 0;
     +			goto free_return;
      		ce = active_cache[pos];
      		if (strncmp(with_slash, ce->name, length))
    --			return 1;
    +-			return 0;
     +			goto free_return;
      		if (ce_skip_worktree(ce))
    --			return 0;
    -+			ret = 0;
    +-			return 1;
    ++			ret = 1;
      	}
    --	return 1;
    +-	return 0;
     +
     +free_return:
     +	if (with_slash != name)
 4:  c83fa51784 !  4:  872b3ebdce mv: check if <destination> is a SKIP_WORKTREE_DIR
    @@ Commit message
         in-cone to out-of-cone" patch.
     
         Change the logic so that Git first check if <destination> is a directory
    -    with all its contents sparsified (a SKIP_WORKTREE_DIR). If yes,
    -    then treat <destination> as a directory exists in the working tree, and
    -    thus using the second form of 'git-mv', i.e. move into this
    -    <destination>, and mark <destination> as a SKIP_WORKTREE_DIR.
    -    If no, continue the original checking logic.
    +    with all its contents sparsified (a SKIP_WORKTREE_DIR).
    +
    +    If <destination> is such a sparse directory, then we should modify the
    +    index the same way as we would if this were a non-sparse directory. We
    +    must be careful to ensure that the <destination> is marked with
    +    SKIP_WORKTREE_DIR.
     
         Also add a `dst_w_slash` to reuse the result from `add_slash()`, which
         was everywhere and can be simplified.
     
    +    Helped-by: Derrick Stolee <derrickstolee@github.com>
    +    Helped-by: Victoria Dye <vdye@github.com>
         Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
     
      ## builtin/mv.c ##
    @@ 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 *modes;
     +	const char *dst_w_slash;
    -+	enum update_mode *modes, dst_mode = 0;
    + 	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 (argc == 1 && is_directory(argv[0]) && !is_directory(argv[1]))
      		flags = 0;
    @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
      	} else {
     -		if (argc != 1)
     +		if (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
    -+		    !check_dir_in_index(dst_w_slash)) {
    ++		    empty_dir_has_sparse_contents(dst_w_slash)) {
     +			destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
    -+			dst_mode |= SKIP_WORKTREE_DIR;
     +		} else if (argc != 1) {
      			die(_("destination '%s' is not a directory"), dest_path[0]);
     -		destination = dest_path;
     +		} else {
     +			destination = dest_path;
     +		}
    - 	}
    -+	if (dst_w_slash != dest_path[0])
    ++	}
    ++	if (dst_w_slash != dest_path[0]) {
     +		free((char *)dst_w_slash);
    ++		dst_w_slash = NULL;
    + 	}
      
      	/* Checking */
    - 	for (i = 0; i < argc; i++) {
 5:  e50ff529c5 !  5:  df625cf852 mv: remove BOTH from enum update_mode
    @@ Metadata
      ## Commit message ##
         mv: remove BOTH from enum update_mode
     
    -    Since BOTH is not practically used anywhere in the code and its meaning
    -    is unclear, remove it.
    +    Since BOTH is not used anywhere in the code and its meaning is unclear,
    +    remove it.
     
    +    Helped-by: Derrick Stolee <derrickstolee@github.com>
    +    Helped-by: Victoria Dye <vdye@github.com>
         Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
     
      ## builtin/mv.c ##
 6:  1ae2e26237 !  6:  e4a4742d8b mv: from in-cone to out-of-cone
    @@ Commit message
         A dirty move should move the <source> to the <destination>, both in the
         working tree and the index, but should *not* remove the resulted path
         from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit.
    -    Instead advise user to "git add" this path and run "git sparse-checkout
    -    reapply" to re-sparsify that path.
     
    +    Helped-by: Derrick Stolee <derrickstolee@github.com>
         Helped-by: Victoria Dye <vdye@github.com>
         Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
     
    - ## advice.c ##
    -@@ advice.c: void detach_advice(const char *new_name)
    - 
    - 	fprintf(stderr, fmt, new_name);
    - }
    -+
    -+void advise_on_moving_dirty_path(struct string_list *pathspec_list)
    -+{
    -+	struct string_list_item *item;
    -+
    -+	if (!pathspec_list->nr)
    -+		return;
    -+
    -+	fprintf(stderr, _("The following paths have been moved outside the\n"
    -+			  "sparse-checkout definition but are not sparse due to local\n"
    -+			  "modifications.\n"));
    -+	for_each_string_list_item(item, pathspec_list)
    -+		fprintf(stderr, "%s\n", item->string);
    -+
    -+	advise_if_enabled(ADVICE_UPDATE_SPARSE_PATH,
    -+			  _("To correct the sparsity of these paths, do the following:\n"
    -+			    "* Use \"git add --sparse <paths>\" to update the index\n"
    -+			    "* Use \"git sparse-checkout reapply\" to apply the sparsity rules"));
    -+}
    -
    - ## advice.h ##
    -@@ advice.h: void NORETURN die_conclude_merge(void);
    - void NORETURN die_ff_impossible(void);
    - void advise_on_updating_sparse_paths(struct string_list *pathspec_list);
    - void detach_advice(const char *new_name);
    -+void advise_on_moving_dirty_path(struct string_list *pathspec_list);
    - 
    - #endif /* ADVICE_H */
    -
      ## builtin/mv.c ##
     @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
    + 	};
    + 	const char **source, **destination, **dest_path, **submodule_gitfile;
    + 	const char *dst_w_slash;
    +-	enum update_mode *modes;
    ++	enum update_mode *modes, dst_mode = 0;
    + 	struct stat st;
    + 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
      	struct lock_file lock_file = LOCK_INIT;
      	struct cache_entry *ce;
      	struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
    @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
      
      	git_config(git_default_config, NULL);
      
    +@@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
    + 		if (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
    + 		    empty_dir_has_sparse_contents(dst_w_slash)) {
    + 			destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
    ++			dst_mode = SKIP_WORKTREE_DIR;
    + 		} else if (argc != 1) {
    + 			die(_("destination '%s' is not a directory"), dest_path[0]);
    + 		} else {
     @@ 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;
    -+		int up_to_date = 0;
    ++		int sparse_and_dirty = 0;
      		struct checkout state = CHECKOUT_INIT;
      		state.istate = &the_index;
      
    @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
      		pos = cache_name_pos(src, strlen(src));
      		assert(pos >= 0);
     +		if (!(mode & SPARSE) && !lstat(src, &st))
    -+			up_to_date = !ce_modified(active_cache[pos], &st, 0);
    ++			sparse_and_dirty = ce_modified(active_cache[pos], &st, 0);
      		rename_cache_entry_at(pos, dst);
      
     -		if ((mode & SPARSE) &&
    @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
     +		if (ignore_sparse &&
     +		    core_apply_sparse_checkout &&
     +		    core_sparse_checkout_cone) {
    - 
    --			dst_pos = cache_name_pos(dst, strlen(dst));
    --			active_cache[dst_pos]->ce_flags &= ~CE_SKIP_WORKTREE;
    -+			/* from out-of-cone to in-cone */
     +			if ((mode & SPARSE) &&
     +			    path_in_sparse_checkout(dst, &the_index)) {
    ++				/* from out-of-cone to in-cone */
     +				int dst_pos = cache_name_pos(dst, strlen(dst));
     +				struct cache_entry *dst_ce = active_cache[dst_pos];
    - 
    --			if (checkout_entry(active_cache[dst_pos], &state, NULL, NULL))
    --				die(_("cannot checkout %s"), active_cache[dst_pos]->name);
    ++
     +				dst_ce->ce_flags &= ~CE_SKIP_WORKTREE;
     +
     +				if (checkout_entry(dst_ce, &state, NULL, NULL))
     +					die(_("cannot checkout %s"), dst_ce->name);
    -+				continue;
    -+			}
    -+
    -+			/* from in-cone to out-of-cone */
    -+			if ((dst_mode & SKIP_WORKTREE_DIR) &&
    -+			    !(mode & SPARSE) &&
    -+			    !path_in_sparse_checkout(dst, &the_index)) {
    ++			} else if ((dst_mode & SKIP_WORKTREE_DIR) &&
    ++				   !(mode & SPARSE) &&
    ++				   !path_in_sparse_checkout(dst, &the_index)) {
    ++				/* from in-cone to out-of-cone */
     +				int dst_pos = cache_name_pos(dst, strlen(dst));
     +				struct cache_entry *dst_ce = active_cache[dst_pos];
    -+				char *src_dir = dirname(xstrdup(src));
    -+
    -+				if (up_to_date) {
    + 
    +-			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"), active_cache[dst_pos]->name);
    ++				/*
    ++				 * if src is clean, it will suffice to remove it
    ++				 */
    ++				if (!sparse_and_dirty) {
     +					dst_ce->ce_flags |= CE_SKIP_WORKTREE;
     +					unlink_or_warn(src);
     +				} else {
    ++					/*
    ++					 * if src is dirty, move it to the
    ++					 * destination and create leading
    ++					 * dirs if necessary
    ++					 */
    ++					char *dst_dup = xstrdup(dst);
     +					string_list_append(&dirty_paths, dst);
    -+					safe_create_leading_directories(xstrdup(dst));
    ++					safe_create_leading_directories(dst_dup);
    ++					FREE_AND_NULL(dst_dup);
     +					rename(src, dst);
     +				}
    -+				if ((mode & INDEX) && is_empty_dir(src_dir))
    -+					rmdir_or_warn(src_dir);
     +			}
      		}
      	}
      
    -+	if (dirty_paths.nr)
    -+		advise_on_moving_dirty_path(&dirty_paths);
    -+
    - 	if (gitmodules_modified)
    - 		stage_updated_gitmodules(&the_index);
    - 
     @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
      		die(_("Unable to write new index file"));
      
    @@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'move clean path from in-cone
      	setup_sparse_checkout &&
      	echo "modified" >>sub/d &&
     @@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'move dirty path from in-cone to out-of-cone' '
    - 	grep -x "H folder1/d" actual
    + 	grep "H folder1/d" actual
      '
      
     -test_expect_failure 'move dir from in-cone to out-of-cone' '
    @@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'move dirty path from in-cone
      	setup_sparse_checkout &&
      
     @@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'move dir from in-cone to out-of-cone' '
    - 	grep -x "S folder1/dir/e" actual
    + 	grep "S folder1/dir/e" actual
      '
      
     -test_expect_failure 'move partially-dirty dir from in-cone to out-of-cone' '
 -:  ---------- >  7:  fec084aaae mv: cleanup empty WORKING_DIRECTORY
 -:  ---------- >  8:  ac5345c013 advice.h: add advise_on_moving_dirty_path()
 7:  672b4053cf !  9:  70d5a67731 mv: check overwrite for in-to-out move
    @@ Metadata
      ## Commit message ##
         mv: check overwrite for in-to-out move
     
    -    Add checking logic for overwritng when moving from in-cone to
    +    Add checking logic for overwriting when moving from in-cone to
         out-of-cone. It is the index version of the original overwrite logic.
     
    +    Helped-by: Derrick Stolee <derrickstolee@github.com>
         Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
     
      ## builtin/mv.c ##
    @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
     
      ## t/t7002-mv-sparse-checkout.sh ##
     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move clean path from in-cone to out-of-cone' '
    - 	grep -x "S folder1/d" actual
    + 	grep "S folder1/d" actual
      '
      
     -test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '

base-commit: 4af7188bc97f70277d0f10d56d5373022b1fa385
-- 
2.37.0

[PATCH v2 1/9] t7002: add tests for moving from in-cone to out-of-cone

[Shaoxuan Yuan]

Add corresponding tests to test that user can move an in-cone <source>
to out-of-cone <destination> when --sparse is supplied.

Such <source> can be either clean or dirty, and moving it results in
different behaviors:

A clean move should move the <source> to the <destination>, both in the
working tree and the index, then remove the resulted path from the
working tree, and turn on its CE_SKIP_WORKTREE bit.

A dirty move should move the <source> to the <destination>, both in the
working tree and the index, but should *not* remove the resulted path
from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit.

Also make sure that if <destination> exists in the index (existing
check for if <destination> is in the worktree is not enough in
in-to-out moves), warn user against the overwrite. And Git should force
the overwrite when supplied with -f or --force.

Helped-by: Derrick Stolee <derrickstolee@github.com>
Helped-by: Victoria Dye <vdye@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 t/t7002-mv-sparse-checkout.sh | 122 ++++++++++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)

diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
index 71fe29690f..9b3a9ab4c3 100755
--- a/t/t7002-mv-sparse-checkout.sh
+++ b/t/t7002-mv-sparse-checkout.sh
@@ -290,4 +290,126 @@ test_expect_success 'move sparse file to existing destination with --force and -
 	test_cmp expect sub/file1
 '
 
+test_expect_failure 'move clean path from in-cone to out-of-cone' '
+	test_when_finished "cleanup_sparse_checkout" &&
+	setup_sparse_checkout &&
+
+	test_must_fail git mv sub/d folder1 2>stderr &&
+	cat sparse_error_header >expect &&
+	echo "folder1/d" >>expect &&
+	cat sparse_hint >>expect &&
+	test_cmp expect stderr &&
+
+	git mv --sparse sub/d folder1 2>stderr &&
+	test_must_be_empty stderr &&
+
+	test_path_is_missing sub/d &&
+	test_path_is_missing folder1/d &&
+	git ls-files -t >actual &&
+	! grep "^H sub/d\$" actual &&
+	grep "S folder1/d" actual
+'
+
+test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
+	test_when_finished "cleanup_sparse_checkout" &&
+	setup_sparse_checkout &&
+	echo "sub/file1 overwrite" >sub/file1 &&
+	git add sub/file1 &&
+
+	test_must_fail git mv sub/file1 folder1 2>stderr &&
+	cat sparse_error_header >expect &&
+	echo "folder1/file1" >>expect &&
+	cat sparse_hint >>expect &&
+	test_cmp expect stderr &&
+
+	test_must_fail git mv --sparse sub/file1 folder1 2>stderr &&
+	echo "fatal: destination exists in the index, source=sub/file1, destination=folder1/file1" \
+	>expect &&
+	test_cmp expect stderr &&
+
+	git mv --sparse -f sub/file1 folder1 2>stderr &&
+	test_must_be_empty stderr &&
+
+	test_path_is_missing sub/file1 &&
+	test_path_is_missing folder1/file1 &&
+	git ls-files -t >actual &&
+	! grep "H sub/file1" actual &&
+	grep "S folder1/file1" actual &&
+
+	# compare file content before move and after move
+	echo "sub/file1 overwrite" >expect &&
+	git ls-files -s -- folder1/file1 | awk "{print \$2}" >oid &&
+	git cat-file blob $(cat oid) >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'move dirty path from in-cone to out-of-cone' '
+	test_when_finished "cleanup_sparse_checkout" &&
+	setup_sparse_checkout &&
+	echo "modified" >>sub/d &&
+
+	test_must_fail git mv sub/d folder1 2>stderr &&
+	cat sparse_error_header >expect &&
+	echo "folder1/d" >>expect &&
+	cat sparse_hint >>expect &&
+	test_cmp expect stderr &&
+
+	git mv --sparse sub/d folder1 2>stderr &&
+
+	test_path_is_missing sub/d &&
+	test_path_is_file folder1/d &&
+	git ls-files -t >actual &&
+	! grep "^H sub/d\$" actual &&
+	grep "H folder1/d" actual
+'
+
+test_expect_failure 'move dir from in-cone to out-of-cone' '
+	test_when_finished "cleanup_sparse_checkout" &&
+	setup_sparse_checkout &&
+
+	test_must_fail git mv sub/dir folder1 2>stderr &&
+	cat sparse_error_header >expect &&
+	echo "folder1/dir/e" >>expect &&
+	cat sparse_hint >>expect &&
+	test_cmp expect stderr &&
+
+	git mv --sparse sub/dir folder1 2>stderr &&
+	test_must_be_empty stderr &&
+
+	test_path_is_missing folder1 &&
+	git ls-files -t >actual &&
+	! grep "H sub/dir/e" actual &&
+	grep "S folder1/dir/e" actual
+'
+
+test_expect_failure 'move partially-dirty dir from in-cone to out-of-cone' '
+	test_when_finished "cleanup_sparse_checkout" &&
+	setup_sparse_checkout &&
+	touch sub/dir/e2 sub/dir/e3 &&
+	git add sub/dir/e2 sub/dir/e3 &&
+	echo "modified" >>sub/dir/e2 &&
+	echo "modified" >>sub/dir/e3 &&
+
+	test_must_fail git mv sub/dir folder1 2>stderr &&
+	cat sparse_error_header >expect &&
+	echo "folder1/dir/e" >>expect &&
+	echo "folder1/dir/e2" >>expect &&
+	echo "folder1/dir/e3" >>expect &&
+	cat sparse_hint >>expect &&
+	test_cmp expect stderr &&
+
+	git mv --sparse sub/dir folder1 2>stderr &&
+
+	test_path_is_missing folder1/dir/e &&
+	test_path_is_file folder1/dir/e2 &&
+	test_path_is_file folder1/dir/e3 &&
+	git ls-files -t >actual &&
+	! grep "H sub/dir/e" actual &&
+	! grep "H sub/dir/e2" actual &&
+	! grep "H sub/dir/e3" actual &&
+	grep "S folder1/dir/e" actual &&
+	grep "H folder1/dir/e2" actual &&
+	grep "H folder1/dir/e3" actual
+'
+
 test_done
-- 
2.37.0

Re: [PATCH v2 1/9] t7002: add tests for moving from in-cone to out-of-cone

[Victoria Dye]

Shaoxuan Yuan wrote:
> Add corresponding tests to test that user can move an in-cone <source>
> to out-of-cone <destination> when --sparse is supplied.
> 
> Such <source> can be either clean or dirty, and moving it results in
> different behaviors:
> 
> A clean move should move the <source> to the <destination>, both in the
> working tree and the index, then remove the resulted path from the
> working tree, and turn on its CE_SKIP_WORKTREE bit.

Looking ahead to patch 6, I think the part about "move it in the working
tree, then delete from the working tree" doesn't quite match your
implementation. Instead, if I'm not mistaken, what happens is:

1. Move <source> to <destination> in the index (do *not* create
   <destination> in the worktree)
2. Delete <source> from the working tree

> 
> A dirty move should move the <source> to the <destination>, both in the
> working tree and the index, but should *not* remove the resulted path
> from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit.

Makes sense.

> 
> Also make sure that if <destination> exists in the index (existing
> check for if <destination> is in the worktree is not enough in
> in-to-out moves), warn user against the overwrite. And Git should force
> the overwrite when supplied with -f or --force.

Also makes sense. 

> 
> Helped-by: Derrick Stolee <derrickstolee@github.com>
> Helped-by: Victoria Dye <vdye@github.com>
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>  t/t7002-mv-sparse-checkout.sh | 122 ++++++++++++++++++++++++++++++++++
>  1 file changed, 122 insertions(+)
> 
> diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
> index 71fe29690f..9b3a9ab4c3 100755
> --- a/t/t7002-mv-sparse-checkout.sh
> +++ b/t/t7002-mv-sparse-checkout.sh
> @@ -290,4 +290,126 @@ test_expect_success 'move sparse file to existing destination with --force and -
>  	test_cmp expect sub/file1
>  '
>  
> +test_expect_failure 'move clean path from in-cone to out-of-cone' '
> +	test_when_finished "cleanup_sparse_checkout" &&
> +	setup_sparse_checkout &&
> +
> +	test_must_fail git mv sub/d folder1 2>stderr &&
> +	cat sparse_error_header >expect &&
> +	echo "folder1/d" >>expect &&
> +	cat sparse_hint >>expect &&
> +	test_cmp expect stderr &&
> +
> +	git mv --sparse sub/d folder1 2>stderr &&
> +	test_must_be_empty stderr &&
> +
> +	test_path_is_missing sub/d &&
> +	test_path_is_missing folder1/d &&
> +	git ls-files -t >actual &&
> +	! grep "^H sub/d\$" actual &&
> +	grep "S folder1/d" actual
> +'
> +
> +test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
> +	test_when_finished "cleanup_sparse_checkout" &&
> +	setup_sparse_checkout &&
> +	echo "sub/file1 overwrite" >sub/file1 &&
> +	git add sub/file1 &&
> +
> +	test_must_fail git mv sub/file1 folder1 2>stderr &&
> +	cat sparse_error_header >expect &&
> +	echo "folder1/file1" >>expect &&
> +	cat sparse_hint >>expect &&
> +	test_cmp expect stderr &&
> +
> +	test_must_fail git mv --sparse sub/file1 folder1 2>stderr &&
> +	echo "fatal: destination exists in the index, source=sub/file1, destination=folder1/file1" \
> +	>expect &&
> +	test_cmp expect stderr &&
> +
> +	git mv --sparse -f sub/file1 folder1 2>stderr &&
> +	test_must_be_empty stderr &&
> +
> +	test_path_is_missing sub/file1 &&
> +	test_path_is_missing folder1/file1 &&
> +	git ls-files -t >actual &&
> +	! grep "H sub/file1" actual &&
> +	grep "S folder1/file1" actual &&
> +
> +	# compare file content before move and after move
> +	echo "sub/file1 overwrite" >expect &&
> +	git ls-files -s -- folder1/file1 | awk "{print \$2}" >oid &&
> +	git cat-file blob $(cat oid) >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_failure 'move dirty path from in-cone to out-of-cone' '
> +	test_when_finished "cleanup_sparse_checkout" &&
> +	setup_sparse_checkout &&
> +	echo "modified" >>sub/d &&
> +
> +	test_must_fail git mv sub/d folder1 2>stderr &&
> +	cat sparse_error_header >expect &&
> +	echo "folder1/d" >>expect &&
> +	cat sparse_hint >>expect &&
> +	test_cmp expect stderr &&
> +
> +	git mv --sparse sub/d folder1 2>stderr &&
> +
> +	test_path_is_missing sub/d &&
> +	test_path_is_file folder1/d &&
> +	git ls-files -t >actual &&
> +	! grep "^H sub/d\$" actual &&
> +	grep "H folder1/d" actual
> +'
> +
> +test_expect_failure 'move dir from in-cone to out-of-cone' '
> +	test_when_finished "cleanup_sparse_checkout" &&
> +	setup_sparse_checkout &&
> +
> +	test_must_fail git mv sub/dir folder1 2>stderr &&
> +	cat sparse_error_header >expect &&
> +	echo "folder1/dir/e" >>expect &&
> +	cat sparse_hint >>expect &&
> +	test_cmp expect stderr &&
> +
> +	git mv --sparse sub/dir folder1 2>stderr &&
> +	test_must_be_empty stderr &&
> +
> +	test_path_is_missing folder1 &&
> +	git ls-files -t >actual &&
> +	! grep "H sub/dir/e" actual &&
> +	grep "S folder1/dir/e" actual
> +'
> +
> +test_expect_failure 'move partially-dirty dir from in-cone to out-of-cone' '
> +	test_when_finished "cleanup_sparse_checkout" &&
> +	setup_sparse_checkout &&
> +	touch sub/dir/e2 sub/dir/e3 &&
> +	git add sub/dir/e2 sub/dir/e3 &&
> +	echo "modified" >>sub/dir/e2 &&
> +	echo "modified" >>sub/dir/e3 &&
> +
> +	test_must_fail git mv sub/dir folder1 2>stderr &&
> +	cat sparse_error_header >expect &&
> +	echo "folder1/dir/e" >>expect &&
> +	echo "folder1/dir/e2" >>expect &&
> +	echo "folder1/dir/e3" >>expect &&
> +	cat sparse_hint >>expect &&
> +	test_cmp expect stderr &&
> +
> +	git mv --sparse sub/dir folder1 2>stderr &&
> +
> +	test_path_is_missing folder1/dir/e &&
> +	test_path_is_file folder1/dir/e2 &&
> +	test_path_is_file folder1/dir/e3 &&
> +	git ls-files -t >actual &&
> +	! grep "H sub/dir/e" actual &&
> +	! grep "H sub/dir/e2" actual &&
> +	! grep "H sub/dir/e3" actual &&
> +	grep "S folder1/dir/e" actual &&
> +	grep "H folder1/dir/e2" actual &&
> +	grep "H folder1/dir/e3" actual
> +'
> +

There are two other test cases I'd be interested in seeing:

1. Move a (clean or dirty) in-cone source file to an out-of-cone destination
   *file*. For example:

	echo test >sub/dir/file1 && 
	git add sub/dir/file1 && 
	git mv --sparse sub/dir/file1 folder1/file1

   I'm assuming this should behave the same way as show in 'move clean path
   from in-cone to out-of-cone overwrite'. 

2. Move a (clean or dirty) in-cone source directory to an out-of-cone
   destination where one or more files in <src> overwrite files in <dst>.
   For example, something like:

	echo test >sub/dir/file1 &&
	git add sub/dir/file1 &&
	git mv --sparse sub/dir folder1

   I don't have a strong opinion on the behavior (does it fail the whole
   'mv' operation? move everything except the files that overwrite
   something?), but it would help to have it documented via test here.

>  test_done

Re: [PATCH v2 1/9] t7002: add tests for moving from in-cone to out-of-cone

[Shaoxuan Yuan]

On 8/9/2022 8:51 AM, Victoria Dye wrote:
> Shaoxuan Yuan wrote:
>> Add corresponding tests to test that user can move an in-cone <source>
>> to out-of-cone <destination> when --sparse is supplied.
>>
>> Such <source> can be either clean or dirty, and moving it results in
>> different behaviors:
>>
>> A clean move should move the <source> to the <destination>, both in the
>> working tree and the index, then remove the resulted path from the
>> working tree, and turn on its CE_SKIP_WORKTREE bit.
> Looking ahead to patch 6, I think the part about "move it in the working
> tree, then delete from the working tree" doesn't quite match your
> implementation. Instead, if I'm not mistaken, what happens is:
>
> 1. Move <source> to <destination> in the index (do *not* create
>     <destination> in the worktree)
> 2. Delete <source> from the working tree
You are right. The description here seems to be my first in-mind 
implementation, which
I wrote down for record. Later I changed the implementation to the one 
you mentioned
here, but somehow forgot to change the description here.

Thanks for the catch!
>> A dirty move should move the <source> to the <destination>, both in the
>> working tree and the index, but should *not* remove the resulted path
>> from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit.
> Makes sense.
>
>> Also make sure that if <destination> exists in the index (existing
>> check for if <destination> is in the worktree is not enough in
>> in-to-out moves), warn user against the overwrite. And Git should force
>> the overwrite when supplied with -f or --force.
> Also makes sense.
>
>> Helped-by: Derrick Stolee <derrickstolee@github.com>
>> Helped-by: Victoria Dye <vdye@github.com>
>> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
>> ---
>>   t/t7002-mv-sparse-checkout.sh | 122 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 122 insertions(+)
>>
>> diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
>> index 71fe29690f..9b3a9ab4c3 100755
>> --- a/t/t7002-mv-sparse-checkout.sh
>> +++ b/t/t7002-mv-sparse-checkout.sh
>> @@ -290,4 +290,126 @@ test_expect_success 'move sparse file to existing destination with --force and -
>>   	test_cmp expect sub/file1
>>   '
>>   
>> +test_expect_failure 'move clean path from in-cone to out-of-cone' '
>> +	test_when_finished "cleanup_sparse_checkout" &&
>> +	setup_sparse_checkout &&
>> +
>> +	test_must_fail git mv sub/d folder1 2>stderr &&
>> +	cat sparse_error_header >expect &&
>> +	echo "folder1/d" >>expect &&
>> +	cat sparse_hint >>expect &&
>> +	test_cmp expect stderr &&
>> +
>> +	git mv --sparse sub/d folder1 2>stderr &&
>> +	test_must_be_empty stderr &&
>> +
>> +	test_path_is_missing sub/d &&
>> +	test_path_is_missing folder1/d &&
>> +	git ls-files -t >actual &&
>> +	! grep "^H sub/d\$" actual &&
>> +	grep "S folder1/d" actual
>> +'
>> +
>> +test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
>> +	test_when_finished "cleanup_sparse_checkout" &&
>> +	setup_sparse_checkout &&
>> +	echo "sub/file1 overwrite" >sub/file1 &&
>> +	git add sub/file1 &&
>> +
>> +	test_must_fail git mv sub/file1 folder1 2>stderr &&
>> +	cat sparse_error_header >expect &&
>> +	echo "folder1/file1" >>expect &&
>> +	cat sparse_hint >>expect &&
>> +	test_cmp expect stderr &&
>> +
>> +	test_must_fail git mv --sparse sub/file1 folder1 2>stderr &&
>> +	echo "fatal: destination exists in the index, source=sub/file1, destination=folder1/file1" \
>> +	>expect &&
>> +	test_cmp expect stderr &&
>> +
>> +	git mv --sparse -f sub/file1 folder1 2>stderr &&
>> +	test_must_be_empty stderr &&
>> +
>> +	test_path_is_missing sub/file1 &&
>> +	test_path_is_missing folder1/file1 &&
>> +	git ls-files -t >actual &&
>> +	! grep "H sub/file1" actual &&
>> +	grep "S folder1/file1" actual &&
>> +
>> +	# compare file content before move and after move
>> +	echo "sub/file1 overwrite" >expect &&
>> +	git ls-files -s -- folder1/file1 | awk "{print \$2}" >oid &&
>> +	git cat-file blob $(cat oid) >actual &&
>> +	test_cmp expect actual
>> +'
>> +
>> +test_expect_failure 'move dirty path from in-cone to out-of-cone' '
>> +	test_when_finished "cleanup_sparse_checkout" &&
>> +	setup_sparse_checkout &&
>> +	echo "modified" >>sub/d &&
>> +
>> +	test_must_fail git mv sub/d folder1 2>stderr &&
>> +	cat sparse_error_header >expect &&
>> +	echo "folder1/d" >>expect &&
>> +	cat sparse_hint >>expect &&
>> +	test_cmp expect stderr &&
>> +
>> +	git mv --sparse sub/d folder1 2>stderr &&
>> +
>> +	test_path_is_missing sub/d &&
>> +	test_path_is_file folder1/d &&
>> +	git ls-files -t >actual &&
>> +	! grep "^H sub/d\$" actual &&
>> +	grep "H folder1/d" actual
>> +'
>> +
>> +test_expect_failure 'move dir from in-cone to out-of-cone' '
>> +	test_when_finished "cleanup_sparse_checkout" &&
>> +	setup_sparse_checkout &&
>> +
>> +	test_must_fail git mv sub/dir folder1 2>stderr &&
>> +	cat sparse_error_header >expect &&
>> +	echo "folder1/dir/e" >>expect &&
>> +	cat sparse_hint >>expect &&
>> +	test_cmp expect stderr &&
>> +
>> +	git mv --sparse sub/dir folder1 2>stderr &&
>> +	test_must_be_empty stderr &&
>> +
>> +	test_path_is_missing folder1 &&
>> +	git ls-files -t >actual &&
>> +	! grep "H sub/dir/e" actual &&
>> +	grep "S folder1/dir/e" actual
>> +'
>> +
>> +test_expect_failure 'move partially-dirty dir from in-cone to out-of-cone' '
>> +	test_when_finished "cleanup_sparse_checkout" &&
>> +	setup_sparse_checkout &&
>> +	touch sub/dir/e2 sub/dir/e3 &&
>> +	git add sub/dir/e2 sub/dir/e3 &&
>> +	echo "modified" >>sub/dir/e2 &&
>> +	echo "modified" >>sub/dir/e3 &&
>> +
>> +	test_must_fail git mv sub/dir folder1 2>stderr &&
>> +	cat sparse_error_header >expect &&
>> +	echo "folder1/dir/e" >>expect &&
>> +	echo "folder1/dir/e2" >>expect &&
>> +	echo "folder1/dir/e3" >>expect &&
>> +	cat sparse_hint >>expect &&
>> +	test_cmp expect stderr &&
>> +
>> +	git mv --sparse sub/dir folder1 2>stderr &&
>> +
>> +	test_path_is_missing folder1/dir/e &&
>> +	test_path_is_file folder1/dir/e2 &&
>> +	test_path_is_file folder1/dir/e3 &&
>> +	git ls-files -t >actual &&
>> +	! grep "H sub/dir/e" actual &&
>> +	! grep "H sub/dir/e2" actual &&
>> +	! grep "H sub/dir/e3" actual &&
>> +	grep "S folder1/dir/e" actual &&
>> +	grep "H folder1/dir/e2" actual &&
>> +	grep "H folder1/dir/e3" actual
>> +'
>> +
> There are two other test cases I'd be interested in seeing:
>
> 1. Move a (clean or dirty) in-cone source file to an out-of-cone destination
>     *file*. For example:
>
> 	echo test >sub/dir/file1 &&
> 	git add sub/dir/file1 &&
> 	git mv --sparse sub/dir/file1 folder1/file1
>
>     I'm assuming this should behave the same way as show in 'move clean path
>     from in-cone to out-of-cone overwrite'.
OK.
>
> 2. Move a (clean or dirty) in-cone source directory to an out-of-cone
>     destination where one or more files in <src> overwrite files in <dst>.
>     For example, something like:
>
> 	echo test >sub/dir/file1 &&
> 	git add sub/dir/file1 &&
> 	git mv --sparse sub/dir folder1
>
>     I don't have a strong opinion on the behavior (does it fail the whole
>     'mv' operation? move everything except the files that overwrite
>     something?), but it would help to have it documented via test here.
OK. I think it will fail the whole `mv` operation. The program will 
report unspecified
overwrite during the early checking phase. The actual "moving" phase 
won't be touched
at all because Git complains early.

--
Thanks,
Shaoxuan

Re: [PATCH v2 1/9] t7002: add tests for moving from in-cone to out-of-cone

[Shaoxuan Yuan]

On 8/9/2022 10:55 AM, Shaoxuan Yuan wrote:
 >> 2. Move a (clean or dirty) in-cone source directory to an out-of-cone
 >>     destination where one or more files in <src> overwrite files in 
<dst>.
 >>     For example, something like:
 >>
 >>     echo test >sub/dir/file1 &&
 >>     git add sub/dir/file1 &&
 >>     git mv --sparse sub/dir folder1
 >>
 >>     I don't have a strong opinion on the behavior (does it fail the 
whole
 >>     'mv' operation? move everything except the files that overwrite
 >>     something?), but it would help to have it documented via test here.
 > OK. I think it will fail the whole `mv` operation. The program will 
report unspecified
 > overwrite during the early checking phase. The actual "moving" phase 
won't be touched
 > at all because Git complains early.

No, actually this example does not fail anything. Because
"sub/dir/file1" after the move will be "folder1/dir/file1", and this
move does not involve overwrite at all.

I'm still adding a test case that precisely reflects the main idea
here, though. Thanks for the suggestion :)

--
Thanks,
Shaoxuan

Re: [PATCH v2 1/9] t7002: add tests for moving from in-cone to out-of-cone

[Shaoxuan Yuan]

On 8/9/2022 8:51 AM, Victoria Dye wrote:
 > There are two other test cases I'd be interested in seeing:
 >
 > 1. Move a (clean or dirty) in-cone source file to an out-of-cone 
destination
 >    *file*. For example:
 >
 >     echo test >sub/dir/file1 &&
 >     git add sub/dir/file1 &&
 >     git mv --sparse sub/dir/file1 folder1/file1
 >
 >    I'm assuming this should behave the same way as show in 'move 
clean path
 >    from in-cone to out-of-cone overwrite'.

It's interesting that this test covers an aspect that was not properly
considered. When designing in-to-out, I only thought <destination> to
be a sparse directory, rather than a possible sparse file, which is
a totally valid argument. Hence, all the logics and mechanisms about
in-to-out are skipped because Git is completely blind to this file
<destination>.

It seems like an easy fix, but I was completely unaware.
Thanks for catching this!

[PATCH v2 2/9] mv: rename check_dir_in_index() to empty_dir_has_sparse_contents()

[Shaoxuan Yuan]

Method check_dir_in_index() introduced in b91a2b6594 (mv: add
check_dir_in_index() and solve general dir check issue, 2022-06-30)
does not describe its intent and behavior well.

Change its name to empty_dir_has_sparse_contents(), which more
precisely describes its purpose.

Reverse the return values, check_dir_in_index() return 0 for success
and 1 for failure; reverse the values so empty_dir_has_sparse_contents()
return 1 for success and 0 for failure. These values are more intuitive
because 1 usually means "has" and 0 means "not found".

Also modify the documentation to better align with the method's
intent and behavior.

Helped-by: Derrick Stolee <derrickstolee@github.com>
Helped-by: Victoria Dye <vdye@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 4729bb1a1a..7c11b8f995 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -125,15 +125,13 @@ static int index_range_of_same_dir(const char *src, int length,
 }
 
 /*
- * 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.
+ * Given the path of a directory that does not exist on-disk, check whether the
+ * directory contains any entries in the index with the SKIP_WORKTREE flag
+ * enabled.
+ * Return 1 if such index entries exist.
+ * Return 0 otherwise.
  */
-static int check_dir_in_index(const char *name)
+static int empty_dir_has_sparse_contents(const char *name)
 {
 	const char *with_slash = add_slash(name);
 	int length = strlen(with_slash);
@@ -144,14 +142,14 @@ static int check_dir_in_index(const char *name)
 	if (pos < 0) {
 		pos = -pos - 1;
 		if (pos >= the_index.cache_nr)
-			return 1;
+			return 0;
 		ce = active_cache[pos];
 		if (strncmp(with_slash, ce->name, length))
-			return 1;
-		if (ce_skip_worktree(ce))
 			return 0;
+		if (ce_skip_worktree(ce))
+			return 1;
 	}
-	return 1;
+	return 0;
 }
 
 int cmd_mv(int argc, const char **argv, const char *prefix)
@@ -232,7 +230,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			if (pos < 0) {
 				const char *src_w_slash = add_slash(src);
 				if (!path_in_sparse_checkout(src_w_slash, &the_index) &&
-				    !check_dir_in_index(src)) {
+				    empty_dir_has_sparse_contents(src)) {
 					modes[i] |= SKIP_WORKTREE_DIR;
 					goto dir_check;
 				}
-- 
2.37.0

[PATCH v2 3/9] mv: free the *with_slash in check_dir_in_index()

[Shaoxuan Yuan]

*with_slash may be a malloc'd pointer, and when it is, free it.

Helped-by: Derrick Stolee <derrickstolee@github.com>
Helped-by: Victoria Dye <vdye@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 7c11b8f995..0a999640c9 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -133,6 +133,7 @@ static int index_range_of_same_dir(const char *src, int length,
  */
 static int empty_dir_has_sparse_contents(const char *name)
 {
+	int ret = 0;
 	const char *with_slash = add_slash(name);
 	int length = strlen(with_slash);
 
@@ -142,14 +143,18 @@ static int empty_dir_has_sparse_contents(const char *name)
 	if (pos < 0) {
 		pos = -pos - 1;
 		if (pos >= the_index.cache_nr)
-			return 0;
+			goto free_return;
 		ce = active_cache[pos];
 		if (strncmp(with_slash, ce->name, length))
-			return 0;
+			goto free_return;
 		if (ce_skip_worktree(ce))
-			return 1;
+			ret = 1;
 	}
-	return 0;
+
+free_return:
+	if (with_slash != name)
+		free((char *)with_slash);
+	return ret;
 }
 
 int cmd_mv(int argc, const char **argv, const char *prefix)
-- 
2.37.0

Re: [PATCH v2 3/9] mv: free the *with_slash in check_dir_in_index()

[Victoria Dye]

Shaoxuan Yuan wrote:
> *with_slash may be a malloc'd pointer, and when it is, free it.

Super-nit: technically, `with_slash` (no `*`) is how you'd refer to the
pointer. `*` is the dereference operator [1], so `*with_slash` has type
`const char` and refers to the first character in the `with_slash` string.

[1] https://en.wikipedia.org/wiki/Dereference_operator

> 
> Helped-by: Derrick Stolee <derrickstolee@github.com>
> Helped-by: Victoria Dye <vdye@github.com>
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>  builtin/mv.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 7c11b8f995..0a999640c9 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -133,6 +133,7 @@ static int index_range_of_same_dir(const char *src, int length,
>   */
>  static int empty_dir_has_sparse_contents(const char *name)
>  {
> +	int ret = 0;
>  	const char *with_slash = add_slash(name);
>  	int length = strlen(with_slash);
>  
> @@ -142,14 +143,18 @@ static int empty_dir_has_sparse_contents(const char *name)
>  	if (pos < 0) {
>  		pos = -pos - 1;
>  		if (pos >= the_index.cache_nr)
> -			return 0;
> +			goto free_return;
>  		ce = active_cache[pos];
>  		if (strncmp(with_slash, ce->name, length))
> -			return 0;
> +			goto free_return;
>  		if (ce_skip_worktree(ce))
> -			return 1;
> +			ret = 1;
>  	}
> -	return 0;
> +
> +free_return:
> +	if (with_slash != name)
> +		free((char *)with_slash);
> +	return ret;
>  }
>  
>  int cmd_mv(int argc, const char **argv, const char *prefix)

The rest of this looks good, nice catch on the potential memory leak.

Re: [PATCH v2 3/9] mv: free the *with_slash in check_dir_in_index()

[Shaoxuan Yuan]

On 8/9/2022 7:41 AM, Victoria Dye wrote:
> Shaoxuan Yuan wrote:
>> *with_slash may be a malloc'd pointer, and when it is, free it.
> Super-nit: technically, `with_slash` (no `*`) is how you'd refer to the
> pointer. `*` is the dereference operator [1], so `*with_slash` has type
> `const char` and refers to the first character in the `with_slash` string.
>
> [1] https://en.wikipedia.org/wiki/Dereference_operator
Oh! Thanks, I was completely unaware. Will fix.

--
Thanks,
Shaoxuan

[PATCH v2 4/9] mv: check if <destination> is a SKIP_WORKTREE_DIR

[Shaoxuan Yuan]

Originally, <destination> is assumed to be in the working tree. If it is
not found as a directory, then it is determined to be either a regular file
path, or error out if used under the second form (move into a directory)
of 'git-mv'. Such behavior is not ideal, mainly because Git does not
look into the index for <destination>, which could potentially be a
SKIP_WORKTREE_DIR, which we need to determine for the later "moving from
in-cone to out-of-cone" patch.

Change the logic so that Git first check if <destination> is a directory
with all its contents sparsified (a SKIP_WORKTREE_DIR).

If <destination> is such a sparse directory, then we should modify the
index the same way as we would if this were a non-sparse directory. We
must be careful to ensure that the <destination> is marked with
SKIP_WORKTREE_DIR.

Also add a `dst_w_slash` to reuse the result from `add_slash()`, which
was everywhere and can be simplified.

Helped-by: Derrick Stolee <derrickstolee@github.com>
Helped-by: Victoria Dye <vdye@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 0a999640c9..f213a92bf6 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -171,6 +171,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 	const char **source, **destination, **dest_path, **submodule_gitfile;
+	const char *dst_w_slash;
 	enum update_mode *modes;
 	struct stat st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
@@ -201,6 +202,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	if (argc == 1 && is_directory(argv[0]) && !is_directory(argv[1]))
 		flags = 0;
 	dest_path = internal_prefix_pathspec(prefix, argv + argc, 1, flags);
+	dst_w_slash = add_slash(dest_path[0]);
 	submodule_gitfile = xcalloc(argc, sizeof(char *));
 
 	if (dest_path[0][0] == '\0')
@@ -208,12 +210,20 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
 	else if (!lstat(dest_path[0], &st) &&
 			S_ISDIR(st.st_mode)) {
-		dest_path[0] = add_slash(dest_path[0]);
-		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
+		destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
 	} else {
-		if (argc != 1)
+		if (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
+		    empty_dir_has_sparse_contents(dst_w_slash)) {
+			destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
+		} else if (argc != 1) {
 			die(_("destination '%s' is not a directory"), dest_path[0]);
-		destination = dest_path;
+		} else {
+			destination = dest_path;
+		}
+	}
+	if (dst_w_slash != dest_path[0]) {
+		free((char *)dst_w_slash);
+		dst_w_slash = NULL;
 	}
 
 	/* Checking */
-- 
2.37.0

Re: [PATCH v2 4/9] mv: check if <destination> is a SKIP_WORKTREE_DIR

[Victoria Dye]

Shaoxuan Yuan wrote:
> Originally, <destination> is assumed to be in the working tree. If it is
> not found as a directory, then it is determined to be either a regular file
> path, or error out if used under the second form (move into a directory)
> of 'git-mv'. Such behavior is not ideal, mainly because Git does not
> look into the index for <destination>, which could potentially be a
> SKIP_WORKTREE_DIR, which we need to determine for the later "moving from
> in-cone to out-of-cone" patch.
> 
> Change the logic so that Git first check if <destination> is a directory
> with all its contents sparsified (a SKIP_WORKTREE_DIR).
> 
> If <destination> is such a sparse directory, then we should modify the
> index the same way as we would if this were a non-sparse directory. We
> must be careful to ensure that the <destination> is marked with
> SKIP_WORKTREE_DIR.
> 
> Also add a `dst_w_slash` to reuse the result from `add_slash()`, which
> was everywhere and can be simplified.

This all makes sense. Stepping through the code...

> 
> Helped-by: Derrick Stolee <derrickstolee@github.com>
> Helped-by: Victoria Dye <vdye@github.com>
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>  builtin/mv.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 0a999640c9..f213a92bf6 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -171,6 +171,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		OPT_END(),
>  	};
>  	const char **source, **destination, **dest_path, **submodule_gitfile;
> +	const char *dst_w_slash;
>  	enum update_mode *modes;
>  	struct stat st;
>  	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
> @@ -201,6 +202,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  	if (argc == 1 && is_directory(argv[0]) && !is_directory(argv[1]))
>  		flags = 0;
>  	dest_path = internal_prefix_pathspec(prefix, argv + argc, 1, flags);
> +	dst_w_slash = add_slash(dest_path[0]);

...you pre-compute a reusable 'dst_w_slash' here...

>  	submodule_gitfile = xcalloc(argc, sizeof(char *));
>  
>  	if (dest_path[0][0] == '\0')
> @@ -208,12 +210,20 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
>  	else if (!lstat(dest_path[0], &st) &&
>  			S_ISDIR(st.st_mode)) {
> -		dest_path[0] = add_slash(dest_path[0]);
> -		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
> +		destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);

...then remove the in-place 'add_slash()' of 'dest_path[0]' and use
'dst_w_slash' in 'internal_prefix_pathspec()'. Makes sense.

>  	} else {

Then, this block is reached if 'dest_path' is not '.' and it is not a
directory that exists on disk.

Previously, reaching this point meant that 'dest_path' *must* refer to a
file, not a directory. However, you want to add handling for the case where
'dst_w_slash' doesn't exist on disk because all of its contents are sparse:

> -		if (argc != 1)
> +		if (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
> +		    empty_dir_has_sparse_contents(dst_w_slash)) {
> +			destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);

so the above condition identifies whether 'dest_path[0]' is non-empty in the
index, and sets 'destination' accordingly. 

It took me some time to understand what all of these (nested) conditions are
doing; one suggestion I have (feel free to ignore it, since it's really just
a matter of stylistic preference) is reduce some duplicate code/simplify the
change a bit by moving the sparse directory check into the main "if-else"
block:

------------->8------------->8------------->8------------->8------------->8-------------
diff --git a/builtin/mv.c b/builtin/mv.c
index 4729bb1a1a..1c1b9559f6 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -203,10 +203,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	if (dest_path[0][0] == '\0')
 		/* special case: "." was normalized to "" */
 		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
-	else if (!lstat(dest_path[0], &st) &&
-			S_ISDIR(st.st_mode)) {
-		dest_path[0] = add_slash(dest_path[0]);
-		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
+	else if ((!lstat(dest_path[0], &st) && S_ISDIR(st.st_mode)) ||
+		 (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
+		  empty_dir_has_sparse_contents(dst_w_slash))) {
+		/* directory dest_path[0] exists on-disk or in the index */
+		destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
 	} else {
 		if (argc != 1)
 			die(_("destination '%s' is not a directory"), dest_path[0]);

-------------8<-------------8<-------------8<-------------8<-------------8<-------------

It doesn't make for the prettiest condition (so your current approach might
be better in terms of readability) but, to me, it creates a clearer
distinction between the "if" and "else if" blocks (which handle the case
where 'dest_path[0]' is a directory), and the "else" block (which handles
the case where 'dest_path[0]' is a file).

> +		} else if (argc != 1) {
>  			die(_("destination '%s' is not a directory"), dest_path[0]);
> -		destination = dest_path;
> +		} else {
> +			destination = dest_path;
> +		}
> +	}
> +	if (dst_w_slash != dest_path[0]) {
> +		free((char *)dst_w_slash);
> +		dst_w_slash = NULL;

Looks good.

>  	}
>  
>  	/* Checking */

Re: [PATCH v2 4/9] mv: check if <destination> is a SKIP_WORKTREE_DIR

[Victoria Dye]

Victoria Dye wrote:
> Shaoxuan Yuan wrote:
>> Originally, <destination> is assumed to be in the working tree. If it is
>> not found as a directory, then it is determined to be either a regular file
>> path, or error out if used under the second form (move into a directory)
>> of 'git-mv'. Such behavior is not ideal, mainly because Git does not
>> look into the index for <destination>, which could potentially be a
>> SKIP_WORKTREE_DIR, which we need to determine for the later "moving from
>> in-cone to out-of-cone" patch.
>>
>> Change the logic so that Git first check if <destination> is a directory
>> with all its contents sparsified (a SKIP_WORKTREE_DIR).
>>
>> If <destination> is such a sparse directory, then we should modify the
>> index the same way as we would if this were a non-sparse directory. We
>> must be careful to ensure that the <destination> is marked with
>> SKIP_WORKTREE_DIR.
>>
>> Also add a `dst_w_slash` to reuse the result from `add_slash()`, which
>> was everywhere and can be simplified.
> 
> This all makes sense. Stepping through the code...
> 
>>
>> Helped-by: Derrick Stolee <derrickstolee@github.com>
>> Helped-by: Victoria Dye <vdye@github.com>
>> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
>> ---
>>  builtin/mv.c | 18 ++++++++++++++----
>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/builtin/mv.c b/builtin/mv.c
>> index 0a999640c9..f213a92bf6 100644
>> --- a/builtin/mv.c
>> +++ b/builtin/mv.c
>> @@ -171,6 +171,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>  		OPT_END(),
>>  	};
>>  	const char **source, **destination, **dest_path, **submodule_gitfile;
>> +	const char *dst_w_slash;
>>  	enum update_mode *modes;
>>  	struct stat st;
>>  	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
>> @@ -201,6 +202,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>  	if (argc == 1 && is_directory(argv[0]) && !is_directory(argv[1]))
>>  		flags = 0;
>>  	dest_path = internal_prefix_pathspec(prefix, argv + argc, 1, flags);
>> +	dst_w_slash = add_slash(dest_path[0]);
> 
> ...you pre-compute a reusable 'dst_w_slash' here...
> 
>>  	submodule_gitfile = xcalloc(argc, sizeof(char *));
>>  
>>  	if (dest_path[0][0] == '\0')
>> @@ -208,12 +210,20 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>  		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
>>  	else if (!lstat(dest_path[0], &st) &&
>>  			S_ISDIR(st.st_mode)) {
>> -		dest_path[0] = add_slash(dest_path[0]);
>> -		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
>> +		destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
> 
> ...then remove the in-place 'add_slash()' of 'dest_path[0]' and use
> 'dst_w_slash' in 'internal_prefix_pathspec()'. Makes sense.
> 
>>  	} else {
> 
> Then, this block is reached if 'dest_path' is not '.' and it is not a
> directory that exists on disk.
> 
> Previously, reaching this point meant that 'dest_path' *must* refer to a
> file, not a directory. However, you want to add handling for the case where
> 'dst_w_slash' doesn't exist on disk because all of its contents are sparse:
> 
>> -		if (argc != 1)
>> +		if (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
>> +		    empty_dir_has_sparse_contents(dst_w_slash)) {
>> +			destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
> 
> so the above condition identifies whether 'dest_path[0]' is non-empty in the
> index, and sets 'destination' accordingly. 
> 
> It took me some time to understand what all of these (nested) conditions are
> doing; one suggestion I have (feel free to ignore it, since it's really just
> a matter of stylistic preference) is reduce some duplicate code/simplify the
> change a bit by moving the sparse directory check into the main "if-else"
> block:
> 
> ------------->8------------->8------------->8------------->8------------->8-------------
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 4729bb1a1a..1c1b9559f6 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -203,10 +203,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  	if (dest_path[0][0] == '\0')
>  		/* special case: "." was normalized to "" */
>  		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
> -	else if (!lstat(dest_path[0], &st) &&
> -			S_ISDIR(st.st_mode)) {
> -		dest_path[0] = add_slash(dest_path[0]);
> -		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
> +	else if ((!lstat(dest_path[0], &st) && S_ISDIR(st.st_mode)) ||
> +		 (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
> +		  empty_dir_has_sparse_contents(dst_w_slash))) {
> +		/* directory dest_path[0] exists on-disk or in the index */
> +		destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
>  	} else {
>  		if (argc != 1)
>  			die(_("destination '%s' is not a directory"), dest_path[0]);
> 
> -------------8<-------------8<-------------8<-------------8<-------------8<-------------
> 
> It doesn't make for the prettiest condition (so your current approach might
> be better in terms of readability) but, to me, it creates a clearer
> distinction between the "if" and "else if" blocks (which handle the case
> where 'dest_path[0]' is a directory), and the "else" block (which handles
> the case where 'dest_path[0]' is a file).

Now that I've read patch 6 [1], I can see that you need the "sparse
directory" condition block to stand alone. I think it might still help to
put that block in the top-level condition:

------------->8------------->8------------->8------------->8------------->8-------------
diff --git a/builtin/mv.c b/builtin/mv.c
index 4729bb1a1a..4a16a5e602 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -205,8 +205,10 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
 	else if (!lstat(dest_path[0], &st) &&
 			S_ISDIR(st.st_mode)) {
-		dest_path[0] = add_slash(dest_path[0]);
-		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
+		destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
+	} else if (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
+		 empty_dir_has_sparse_contents(dst_w_slash)) {
+		destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
 	} else {
 		if (argc != 1)
 			die(_("destination '%s' is not a directory"), dest_path[0]);
-------------8<-------------8<-------------8<-------------8<-------------8<---------

...but, as before, I'm happy with whichever approach you decide to take.

[1] https://lore.kernel.org/git/20220805030528.1535376-7-shaoxuan.yuan02@gmail.com/

> 
>> +		} else if (argc != 1) {
>>  			die(_("destination '%s' is not a directory"), dest_path[0]);
>> -		destination = dest_path;
>> +		} else {
>> +			destination = dest_path;
>> +		}
>> +	}
>> +	if (dst_w_slash != dest_path[0]) {
>> +		free((char *)dst_w_slash);
>> +		dst_w_slash = NULL;
> 
> Looks good.
> 
>>  	}
>>  
>>  	/* Checking */
> 

Re: [PATCH v2 4/9] mv: check if <destination> is a SKIP_WORKTREE_DIR

[Shaoxuan Yuan]

On 8/9/2022 7:41 AM, Victoria Dye wrote:
...truncated...
> It took me some time to understand what all of these (nested) conditions are
> doing; one suggestion I have (feel free to ignore it, since it's really just
> a matter of stylistic preference) is reduce some duplicate code/simplify the
> change a bit by moving the sparse directory check into the main "if-else"
> block:
Yes, I acknowledge this part is cluttered slightly ;)
> ------------->8------------->8------------->8------------->8------------->8-------------
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 4729bb1a1a..1c1b9559f6 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -203,10 +203,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>   	if (dest_path[0][0] == '\0')
>   		/* special case: "." was normalized to "" */
>   		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
> -	else if (!lstat(dest_path[0], &st) &&
> -			S_ISDIR(st.st_mode)) {
> -		dest_path[0] = add_slash(dest_path[0]);
> -		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
> +	else if ((!lstat(dest_path[0], &st) && S_ISDIR(st.st_mode)) ||
> +		 (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
> +		  empty_dir_has_sparse_contents(dst_w_slash))) {
> +		/* directory dest_path[0] exists on-disk or in the index */
> +		destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
>   	} else {
>   		if (argc != 1)
>   			die(_("destination '%s' is not a directory"), dest_path[0]);
>
> -------------8<-------------8<-------------8<-------------8<-------------8<-------------
>
> It doesn't make for the prettiest condition (so your current approach might
> be better in terms of readability) but, to me, it creates a clearer
> distinction between the "if" and "else if" blocks (which handle the case
> where 'dest_path[0]' is a directory), and the "else" block (which handles
> the case where 'dest_path[0]' is a file).

I also find this way clearer! Thanks for the suggestion!

[PATCH v2 5/9] mv: remove BOTH from enum update_mode

[Shaoxuan Yuan]

Since BOTH is not used anywhere in the code and its meaning is unclear,
remove it.

Helped-by: Derrick Stolee <derrickstolee@github.com>
Helped-by: Victoria Dye <vdye@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index f213a92bf6..1dc55153ed 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -21,7 +21,6 @@ static const char * const builtin_mv_usage[] = {
 };
 
 enum update_mode {
-	BOTH = 0,
 	WORKING_DIRECTORY = (1 << 1),
 	INDEX = (1 << 2),
 	SPARSE = (1 << 3),
-- 
2.37.0

[PATCH v2 6/9] mv: from in-cone to out-of-cone

[Shaoxuan Yuan]

Originally, moving an in-cone <source> to an out-of-cone <destination>
was not possible, mainly because such <destination> is a directory that
is not present in the working tree.

Change the behavior so that we can move an in-cone <source> to
out-of-cone <destination> when --sparse is supplied.

Such <source> can be either clean or dirty, and moving it results in
different behaviors:

A clean move should move the <source> to the <destination>, both in the
working tree and the index, then remove the resulted path from the
working tree, and turn on its CE_SKIP_WORKTREE bit.

A dirty move should move the <source> to the <destination>, both in the
working tree and the index, but should *not* remove the resulted path
from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit.

Helped-by: Derrick Stolee <derrickstolee@github.com>
Helped-by: Victoria Dye <vdye@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c                  | 55 +++++++++++++++++++++++++++++------
 t/t7002-mv-sparse-checkout.sh |  8 ++---
 2 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 1dc55153ed..e4a14aea2d 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -171,12 +171,13 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	};
 	const char **source, **destination, **dest_path, **submodule_gitfile;
 	const char *dst_w_slash;
-	enum update_mode *modes;
+	enum update_mode *modes, dst_mode = 0;
 	struct stat st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
 	struct lock_file lock_file = LOCK_INIT;
 	struct cache_entry *ce;
 	struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
+	struct string_list dirty_paths = STRING_LIST_INIT_NODUP;
 
 	git_config(git_default_config, NULL);
 
@@ -214,6 +215,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		if (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
 		    empty_dir_has_sparse_contents(dst_w_slash)) {
 			destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
+			dst_mode = SKIP_WORKTREE_DIR;
 		} else if (argc != 1) {
 			die(_("destination '%s' is not a directory"), dest_path[0]);
 		} else {
@@ -408,6 +410,7 @@ 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;
+		int sparse_and_dirty = 0;
 		struct checkout state = CHECKOUT_INIT;
 		state.istate = &the_index;
 
@@ -418,6 +421,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		if (show_only)
 			continue;
 		if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
+		    !(dst_mode & SKIP_WORKTREE_DIR) &&
 		    rename(src, dst) < 0) {
 			if (ignore_errors)
 				continue;
@@ -437,17 +441,49 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 
 		pos = cache_name_pos(src, strlen(src));
 		assert(pos >= 0);
+		if (!(mode & SPARSE) && !lstat(src, &st))
+			sparse_and_dirty = ce_modified(active_cache[pos], &st, 0);
 		rename_cache_entry_at(pos, dst);
 
-		if ((mode & SPARSE) &&
-		    (path_in_sparse_checkout(dst, &the_index))) {
-			int dst_pos;
+		if (ignore_sparse &&
+		    core_apply_sparse_checkout &&
+		    core_sparse_checkout_cone) {
+			if ((mode & SPARSE) &&
+			    path_in_sparse_checkout(dst, &the_index)) {
+				/* from out-of-cone to in-cone */
+				int dst_pos = cache_name_pos(dst, strlen(dst));
+				struct cache_entry *dst_ce = active_cache[dst_pos];
+
+				dst_ce->ce_flags &= ~CE_SKIP_WORKTREE;
+
+				if (checkout_entry(dst_ce, &state, NULL, NULL))
+					die(_("cannot checkout %s"), dst_ce->name);
+			} else if ((dst_mode & SKIP_WORKTREE_DIR) &&
+				   !(mode & SPARSE) &&
+				   !path_in_sparse_checkout(dst, &the_index)) {
+				/* from in-cone to out-of-cone */
+				int dst_pos = cache_name_pos(dst, strlen(dst));
+				struct cache_entry *dst_ce = active_cache[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"), active_cache[dst_pos]->name);
+				/*
+				 * if src is clean, it will suffice to remove it
+				 */
+				if (!sparse_and_dirty) {
+					dst_ce->ce_flags |= CE_SKIP_WORKTREE;
+					unlink_or_warn(src);
+				} else {
+					/*
+					 * if src is dirty, move it to the
+					 * destination and create leading
+					 * dirs if necessary
+					 */
+					char *dst_dup = xstrdup(dst);
+					string_list_append(&dirty_paths, dst);
+					safe_create_leading_directories(dst_dup);
+					FREE_AND_NULL(dst_dup);
+					rename(src, dst);
+				}
+			}
 		}
 	}
 
@@ -459,6 +495,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		die(_("Unable to write new index file"));
 
 	string_list_clear(&src_for_dst, 0);
+	string_list_clear(&dirty_paths, 0);
 	UNLEAK(source);
 	UNLEAK(dest_path);
 	free(submodule_gitfile);
diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
index 9b3a9ab4c3..fc9577b2a6 100755
--- a/t/t7002-mv-sparse-checkout.sh
+++ b/t/t7002-mv-sparse-checkout.sh
@@ -290,7 +290,7 @@ test_expect_success 'move sparse file to existing destination with --force and -
 	test_cmp expect sub/file1
 '
 
-test_expect_failure 'move clean path from in-cone to out-of-cone' '
+test_expect_success 'move clean path from in-cone to out-of-cone' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 
@@ -343,7 +343,7 @@ test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'move dirty path from in-cone to out-of-cone' '
+test_expect_success 'move dirty path from in-cone to out-of-cone' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 	echo "modified" >>sub/d &&
@@ -363,7 +363,7 @@ test_expect_failure 'move dirty path from in-cone to out-of-cone' '
 	grep "H folder1/d" actual
 '
 
-test_expect_failure 'move dir from in-cone to out-of-cone' '
+test_expect_success 'move dir from in-cone to out-of-cone' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 
@@ -382,7 +382,7 @@ test_expect_failure 'move dir from in-cone to out-of-cone' '
 	grep "S folder1/dir/e" actual
 '
 
-test_expect_failure 'move partially-dirty dir from in-cone to out-of-cone' '
+test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 	touch sub/dir/e2 sub/dir/e3 &&
-- 
2.37.0

Re: [PATCH v2 6/9] mv: from in-cone to out-of-cone

[Victoria Dye]

Shaoxuan Yuan wrote:
> Originally, moving an in-cone <source> to an out-of-cone <destination>
> was not possible, mainly because such <destination> is a directory that
> is not present in the working tree.
> 
> Change the behavior so that we can move an in-cone <source> to
> out-of-cone <destination> when --sparse is supplied.
> 
> Such <source> can be either clean or dirty, and moving it results in
> different behaviors:
> 
> A clean move should move the <source> to the <destination>, both in the
> working tree and the index, then remove the resulted path from the
> working tree, and turn on its CE_SKIP_WORKTREE bit.

It looks like this description is the same as what you wrote in patch 1 [1].
That's fine with me but, as noted in [2], I wanted to double-check whether
the "move <src> to <dst> in worktree, then remove <dst> from worktree" is an
accurate description of what's happening.

[1] https://lore.kernel.org/git/20220805030528.1535376-2-shaoxuan.yuan02@gmail.com/
[2] https://lore.kernel.org/git/bd80881d-b2a3-c220-8f2d-a07a46e14207@github.com/

> 
> A dirty move should move the <source> to the <destination>, both in the
> working tree and the index, but should *not* remove the resulted path
> from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit.> 
> Helped-by: Derrick Stolee <derrickstolee@github.com>
> Helped-by: Victoria Dye <vdye@github.com>
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>  builtin/mv.c                  | 55 +++++++++++++++++++++++++++++------
>  t/t7002-mv-sparse-checkout.sh |  8 ++---
>  2 files changed, 50 insertions(+), 13 deletions(-)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 1dc55153ed..e4a14aea2d 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -171,12 +171,13 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  	};
>  	const char **source, **destination, **dest_path, **submodule_gitfile;
>  	const char *dst_w_slash;
> -	enum update_mode *modes;
> +	enum update_mode *modes, dst_mode = 0;
>  	struct stat st;
>  	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
>  	struct lock_file lock_file = LOCK_INIT;
>  	struct cache_entry *ce;
>  	struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
> +	struct string_list dirty_paths = STRING_LIST_INIT_NODUP;
>  
>  	git_config(git_default_config, NULL);
>  
> @@ -214,6 +215,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		if (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
>  		    empty_dir_has_sparse_contents(dst_w_slash)) {
>  			destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
> +			dst_mode = SKIP_WORKTREE_DIR;
>  		} else if (argc != 1) {
>  			die(_("destination '%s' is not a directory"), dest_path[0]);
>  		} else {
> @@ -408,6 +410,7 @@ 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;
> +		int sparse_and_dirty = 0;
>  		struct checkout state = CHECKOUT_INIT;
>  		state.istate = &the_index;
>  
> @@ -418,6 +421,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		if (show_only)
>  			continue;
>  		if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
> +		    !(dst_mode & SKIP_WORKTREE_DIR) &&
>  		    rename(src, dst) < 0) {
>  			if (ignore_errors)
>  				continue;
> @@ -437,17 +441,49 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  
>  		pos = cache_name_pos(src, strlen(src));
>  		assert(pos >= 0);
> +		if (!(mode & SPARSE) && !lstat(src, &st))
> +			sparse_and_dirty = ce_modified(active_cache[pos], &st, 0);
>  		rename_cache_entry_at(pos, dst);
>  
> -		if ((mode & SPARSE) &&
> -		    (path_in_sparse_checkout(dst, &the_index))) {
> -			int dst_pos;
> +		if (ignore_sparse &&
> +		    core_apply_sparse_checkout &&
> +		    core_sparse_checkout_cone) {
> +			if ((mode & SPARSE) &&
> +			    path_in_sparse_checkout(dst, &the_index)) {
> +				/* from out-of-cone to in-cone */
> +				int dst_pos = cache_name_pos(dst, strlen(dst));
> +				struct cache_entry *dst_ce = active_cache[dst_pos];
> +
> +				dst_ce->ce_flags &= ~CE_SKIP_WORKTREE;
> +
> +				if (checkout_entry(dst_ce, &state, NULL, NULL))
> +					die(_("cannot checkout %s"), dst_ce->name);
> +			} else if ((dst_mode & SKIP_WORKTREE_DIR) &&
> +				   !(mode & SPARSE) &&
> +				   !path_in_sparse_checkout(dst, &the_index)) {
> +				/* from in-cone to out-of-cone */
> +				int dst_pos = cache_name_pos(dst, strlen(dst));
> +				struct cache_entry *dst_ce = active_cache[dst_pos];

It looks like the above conditions assume "out-of-cone <src>" and
"out-of-cone <dst>" are mutually exclusive. Have you checked what happens
when you try a move like that? 

If the behavior is sensible, it would be nice to add a test (in 't7002'?)
establishing that. Otherwise, changing that behavior is reasonably outside
the scope of this series, so it's fine with me if you add a either
"test_expect_failure" test, an "unsupported" warning message earlier in
'mv', or even just a "NEEDSWORK" comment somewhere around this code.

>  
> -			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"), active_cache[dst_pos]->name);
> +				/*
> +				 * if src is clean, it will suffice to remove it
> +				 */
> +				if (!sparse_and_dirty) {
> +					dst_ce->ce_flags |= CE_SKIP_WORKTREE;
> +					unlink_or_warn(src);
> +				} else {
> +					/*
> +					 * if src is dirty, move it to the
> +					 * destination and create leading
> +					 * dirs if necessary
> +					 */
> +					char *dst_dup = xstrdup(dst);
> +					string_list_append(&dirty_paths, dst);
> +					safe_create_leading_directories(dst_dup);
> +					FREE_AND_NULL(dst_dup);
> +					rename(src, dst);
> +				}
> +			}

The rest of this is clearly organized and (as far as I can tell) matches the
behavior you established with the tests in patch 1. Nice work!

>  		}
>  	}
>  
> @@ -459,6 +495,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		die(_("Unable to write new index file"));
>  
>  	string_list_clear(&src_for_dst, 0);
> +	string_list_clear(&dirty_paths, 0);
>  	UNLEAK(source);
>  	UNLEAK(dest_path);
>  	free(submodule_gitfile);
> diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
> index 9b3a9ab4c3..fc9577b2a6 100755
> --- a/t/t7002-mv-sparse-checkout.sh
> +++ b/t/t7002-mv-sparse-checkout.sh
> @@ -290,7 +290,7 @@ test_expect_success 'move sparse file to existing destination with --force and -
>  	test_cmp expect sub/file1
>  '
>  
> -test_expect_failure 'move clean path from in-cone to out-of-cone' '
> +test_expect_success 'move clean path from in-cone to out-of-cone' '
>  	test_when_finished "cleanup_sparse_checkout" &&
>  	setup_sparse_checkout &&
>  
> @@ -343,7 +343,7 @@ test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_failure 'move dirty path from in-cone to out-of-cone' '
> +test_expect_success 'move dirty path from in-cone to out-of-cone' '
>  	test_when_finished "cleanup_sparse_checkout" &&
>  	setup_sparse_checkout &&
>  	echo "modified" >>sub/d &&
> @@ -363,7 +363,7 @@ test_expect_failure 'move dirty path from in-cone to out-of-cone' '
>  	grep "H folder1/d" actual
>  '
>  
> -test_expect_failure 'move dir from in-cone to out-of-cone' '
> +test_expect_success 'move dir from in-cone to out-of-cone' '
>  	test_when_finished "cleanup_sparse_checkout" &&
>  	setup_sparse_checkout &&
>  
> @@ -382,7 +382,7 @@ test_expect_failure 'move dir from in-cone to out-of-cone' '
>  	grep "S folder1/dir/e" actual
>  '
>  
> -test_expect_failure 'move partially-dirty dir from in-cone to out-of-cone' '
> +test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' '
>  	test_when_finished "cleanup_sparse_checkout" &&
>  	setup_sparse_checkout &&
>  	touch sub/dir/e2 sub/dir/e3 &&

Re: [PATCH v2 6/9] mv: from in-cone to out-of-cone

[Shaoxuan Yuan]

On 8/9/2022 8:53 AM, Victoria Dye wrote:
> Shaoxuan Yuan wrote:
>> Originally, moving an in-cone <source> to an out-of-cone <destination>
>> was not possible, mainly because such <destination> is a directory that
>> is not present in the working tree.
>>
>> Change the behavior so that we can move an in-cone <source> to
>> out-of-cone <destination> when --sparse is supplied.
>>
>> Such <source> can be either clean or dirty, and moving it results in
>> different behaviors:
>>
>> A clean move should move the <source> to the <destination>, both in the
>> working tree and the index, then remove the resulted path from the
>> working tree, and turn on its CE_SKIP_WORKTREE bit.
> It looks like this description is the same as what you wrote in patch 1 [1].
> That's fine with me but, as noted in [2], I wanted to double-check whether
> the "move <src> to <dst> in worktree, then remove <dst> from worktree" is an
> accurate description of what's happening.
>
> [1] https://lore.kernel.org/git/20220805030528.1535376-2-shaoxuan.yuan02@gmail.com/
> [2] https://lore.kernel.org/git/bd80881d-b2a3-c220-8f2d-a07a46e14207@github.com/

This description is incorrect, as I mentioned in a previous email [3] :)
[3] 
https://lore.kernel.org/git/651d89e2-5282-2cf8-ffc3-8650a023c80a@gmail.com/

>> A dirty move should move the <source> to the <destination>, both in the
>> working tree and the index, but should *not* remove the resulted path
>> from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit.>
>> Helped-by: Derrick Stolee <derrickstolee@github.com>
>> Helped-by: Victoria Dye <vdye@github.com>
>> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
>> ---
>>   builtin/mv.c                  | 55 +++++++++++++++++++++++++++++------
>>   t/t7002-mv-sparse-checkout.sh |  8 ++---
>>   2 files changed, 50 insertions(+), 13 deletions(-)
>>
>> diff --git a/builtin/mv.c b/builtin/mv.c
>> index 1dc55153ed..e4a14aea2d 100644
>> --- a/builtin/mv.c
>> +++ b/builtin/mv.c
>> @@ -171,12 +171,13 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>   	};
>>   	const char **source, **destination, **dest_path, **submodule_gitfile;
>>   	const char *dst_w_slash;
>> -	enum update_mode *modes;
>> +	enum update_mode *modes, dst_mode = 0;
>>   	struct stat st;
>>   	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
>>   	struct lock_file lock_file = LOCK_INIT;
>>   	struct cache_entry *ce;
>>   	struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
>> +	struct string_list dirty_paths = STRING_LIST_INIT_NODUP;
>>   
>>   	git_config(git_default_config, NULL);
>>   
>> @@ -214,6 +215,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>   		if (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
>>   		    empty_dir_has_sparse_contents(dst_w_slash)) {
>>   			destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
>> +			dst_mode = SKIP_WORKTREE_DIR;
>>   		} else if (argc != 1) {
>>   			die(_("destination '%s' is not a directory"), dest_path[0]);
>>   		} else {
>> @@ -408,6 +410,7 @@ 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;
>> +		int sparse_and_dirty = 0;
>>   		struct checkout state = CHECKOUT_INIT;
>>   		state.istate = &the_index;
>>   
>> @@ -418,6 +421,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>   		if (show_only)
>>   			continue;
>>   		if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
>> +		    !(dst_mode & SKIP_WORKTREE_DIR) &&
>>   		    rename(src, dst) < 0) {
>>   			if (ignore_errors)
>>   				continue;
>> @@ -437,17 +441,49 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>   
>>   		pos = cache_name_pos(src, strlen(src));
>>   		assert(pos >= 0);
>> +		if (!(mode & SPARSE) && !lstat(src, &st))
>> +			sparse_and_dirty = ce_modified(active_cache[pos], &st, 0);
>>   		rename_cache_entry_at(pos, dst);
>>   
>> -		if ((mode & SPARSE) &&
>> -		    (path_in_sparse_checkout(dst, &the_index))) {
>> -			int dst_pos;
>> +		if (ignore_sparse &&
>> +		    core_apply_sparse_checkout &&
>> +		    core_sparse_checkout_cone) {
>> +			if ((mode & SPARSE) &&
>> +			    path_in_sparse_checkout(dst, &the_index)) {
>> +				/* from out-of-cone to in-cone */
>> +				int dst_pos = cache_name_pos(dst, strlen(dst));
>> +				struct cache_entry *dst_ce = active_cache[dst_pos];
>> +
>> +				dst_ce->ce_flags &= ~CE_SKIP_WORKTREE;
>> +
>> +				if (checkout_entry(dst_ce, &state, NULL, NULL))
>> +					die(_("cannot checkout %s"), dst_ce->name);
>> +			} else if ((dst_mode & SKIP_WORKTREE_DIR) &&
>> +				   !(mode & SPARSE) &&
>> +				   !path_in_sparse_checkout(dst, &the_index)) {
>> +				/* from in-cone to out-of-cone */
>> +				int dst_pos = cache_name_pos(dst, strlen(dst));
>> +				struct cache_entry *dst_ce = active_cache[dst_pos];
> It looks like the above conditions assume "out-of-cone <src>" and
> "out-of-cone <dst>" are mutually exclusive. Have you checked what happens
> when you try a move like that?

Do you mean can <src>  and <dst> both be out-of-cone (out-to-out move)?
If that's your question, I'm not sure about the answer. As for now, `mv` has
addressed the following conditions:

1. in-to-in
2. in-to-out
3. out-to-in

But out-to-out is something mysterious at this point.

> If the behavior is sensible, it would be nice to add a test (in 't7002'?)
> establishing that. Otherwise, changing that behavior is reasonably outside
> the scope of this series, so it's fine with me if you add a either
> "test_expect_failure" test, an "unsupported" warning message earlier in
> 'mv', or even just a "NEEDSWORK" comment somewhere around this code.

Yes, I will try test this mode and see where it goes. Based on the test 
result I'll
decide if that's an easy fix or should be marked as "NEEDSWORK". I 
prefer to leave
it to a future patch, though :)

Thanks for the catch!

[PATCH v2 7/9] mv: cleanup empty WORKING_DIRECTORY

[Shaoxuan Yuan]

Originally, moving from-in-to-out may leave an empty <source>
directory on-disk (this kind of directory is marked as
WORKING_DIRECTORY).

Cleanup such directories if they are empty (don't have any entries
under them).

Modify two tests that take <source> as WORKING_DIRECTORY to test
this behavior.

Suggested-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c                  | 27 +++++++++++++++++++++++++++
 t/t7002-mv-sparse-checkout.sh |  4 ++++
 2 files changed, 31 insertions(+)

diff --git a/builtin/mv.c b/builtin/mv.c
index e4a14aea2d..a58387a986 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -171,6 +171,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	};
 	const char **source, **destination, **dest_path, **submodule_gitfile;
 	const char *dst_w_slash;
+	const char **src_dir = NULL;
+	int src_dir_nr = 0, src_dir_alloc = 0;
+	struct strbuf a_src_dir = STRBUF_INIT;
 	enum update_mode *modes, dst_mode = 0;
 	struct stat st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
@@ -304,6 +307,10 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 
 			/* last - first >= 1 */
 			modes[i] |= WORKING_DIRECTORY;
+
+			ALLOC_GROW(src_dir, src_dir_nr + 1, src_dir_alloc);
+			src_dir[src_dir_nr++] = src;
+
 			n = argc + last - first;
 			REALLOC_ARRAY(source, n);
 			REALLOC_ARRAY(destination, n);
@@ -487,6 +494,26 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		}
 	}
 
+	/*
+	 * cleanup the empty src_dirs
+	 */
+	for (i = 0; i < src_dir_nr; i++) {
+		int dummy;
+		strbuf_addstr(&a_src_dir, src_dir[i]);
+		/*
+		 * if entries under a_src_dir are all moved away,
+		 * recursively remove a_src_dir to cleanup
+		 */
+		if (index_range_of_same_dir(a_src_dir.buf, a_src_dir.len,
+					    &dummy, &dummy) < 1) {
+			remove_dir_recursively(&a_src_dir, 0);
+		}
+		strbuf_reset(&a_src_dir);
+	}
+
+	strbuf_release(&a_src_dir);
+	free(src_dir);
+
 	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 fc9577b2a6..23d0c4cec6 100755
--- a/t/t7002-mv-sparse-checkout.sh
+++ b/t/t7002-mv-sparse-checkout.sh
@@ -366,6 +366,7 @@ test_expect_success 'move dirty path from in-cone to out-of-cone' '
 test_expect_success 'move dir from in-cone to out-of-cone' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
+	mkdir sub/dir/deep &&
 
 	test_must_fail git mv sub/dir folder1 2>stderr &&
 	cat sparse_error_header >expect &&
@@ -376,6 +377,7 @@ test_expect_success 'move dir from in-cone to out-of-cone' '
 	git mv --sparse sub/dir folder1 2>stderr &&
 	test_must_be_empty stderr &&
 
+	test_path_is_missing sub/dir &&
 	test_path_is_missing folder1 &&
 	git ls-files -t >actual &&
 	! grep "H sub/dir/e" actual &&
@@ -385,6 +387,7 @@ test_expect_success 'move dir from in-cone to out-of-cone' '
 test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
+	mkdir sub/dir/deep &&
 	touch sub/dir/e2 sub/dir/e3 &&
 	git add sub/dir/e2 sub/dir/e3 &&
 	echo "modified" >>sub/dir/e2 &&
@@ -400,6 +403,7 @@ test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' '
 
 	git mv --sparse sub/dir folder1 2>stderr &&
 
+	test_path_is_missing sub/dir &&
 	test_path_is_missing folder1/dir/e &&
 	test_path_is_file folder1/dir/e2 &&
 	test_path_is_file folder1/dir/e3 &&
-- 
2.37.0

[PATCH v2 8/9] advice.h: add advise_on_moving_dirty_path()

[Shaoxuan Yuan]

Add an advice.

When the user use `git mv --sparse <dirty-path> <destination>`, Git
will warn the user to use `git add --sparse <paths>` then use
`git sparse-checkout reapply` to apply the sparsity rules.

Add a few lines to previous "move dirty path" tests so we can test
this new advice is working.

Suggested-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 advice.c                      | 19 +++++++++++++++++++
 advice.h                      |  1 +
 builtin/mv.c                  |  3 +++
 t/t7002-mv-sparse-checkout.sh | 24 +++++++++++++++++++++++-
 4 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/advice.c b/advice.c
index 6fda9edbc2..fd18968943 100644
--- a/advice.c
+++ b/advice.c
@@ -261,3 +261,22 @@ void detach_advice(const char *new_name)
 
 	fprintf(stderr, fmt, new_name);
 }
+
+void advise_on_moving_dirty_path(struct string_list *pathspec_list)
+{
+	struct string_list_item *item;
+
+	if (!pathspec_list->nr)
+		return;
+
+	fprintf(stderr, _("The following paths have been moved outside the\n"
+			  "sparse-checkout definition but are not sparse due to local\n"
+			  "modifications.\n"));
+	for_each_string_list_item(item, pathspec_list)
+		fprintf(stderr, "%s\n", item->string);
+
+	advise_if_enabled(ADVICE_UPDATE_SPARSE_PATH,
+			  _("To correct the sparsity of these paths, do the following:\n"
+			    "* Use \"git add --sparse <paths>\" to update the index\n"
+			    "* Use \"git sparse-checkout reapply\" to apply the sparsity rules"));
+}
diff --git a/advice.h b/advice.h
index 7ddc6cbc1a..07e0f76833 100644
--- a/advice.h
+++ b/advice.h
@@ -74,5 +74,6 @@ void NORETURN die_conclude_merge(void);
 void NORETURN die_ff_impossible(void);
 void advise_on_updating_sparse_paths(struct string_list *pathspec_list);
 void detach_advice(const char *new_name);
+void advise_on_moving_dirty_path(struct string_list *pathspec_list);
 
 #endif /* ADVICE_H */
diff --git a/builtin/mv.c b/builtin/mv.c
index a58387a986..765a1e8eb5 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -514,6 +514,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	strbuf_release(&a_src_dir);
 	free(src_dir);
 
+	if (dirty_paths.nr)
+		advise_on_moving_dirty_path(&dirty_paths);
+
 	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 23d0c4cec6..f0b32a2f70 100755
--- a/t/t7002-mv-sparse-checkout.sh
+++ b/t/t7002-mv-sparse-checkout.sh
@@ -28,12 +28,25 @@ test_expect_success 'setup' "
 	updated in the index:
 	EOF
 
-	cat >sparse_hint <<-EOF
+	cat >sparse_hint <<-EOF &&
 	hint: If you intend to update such entries, try one of the following:
 	hint: * Use the --sparse option.
 	hint: * Disable or modify the sparsity rules.
 	hint: Disable this message with \"git config advice.updateSparsePath false\"
 	EOF
+
+	cat >dirty_error_header <<-EOF &&
+	The following paths have been moved outside the
+	sparse-checkout definition but are not sparse due to local
+	modifications.
+	EOF
+
+	cat >dirty_hint <<-EOF
+	hint: To correct the sparsity of these paths, do the following:
+	hint: * Use \"git add --sparse <paths>\" to update the index
+	hint: * Use \"git sparse-checkout reapply\" to apply the sparsity rules
+	hint: Disable this message with \"git config advice.updateSparsePath false\"
+	EOF
 "
 
 test_expect_success 'mv refuses to move sparse-to-sparse' '
@@ -355,6 +368,10 @@ test_expect_success 'move dirty path from in-cone to out-of-cone' '
 	test_cmp expect stderr &&
 
 	git mv --sparse sub/d folder1 2>stderr &&
+	cat dirty_error_header >expect &&
+	echo "folder1/d" >>expect &&
+	cat dirty_hint >>expect &&
+	test_cmp expect stderr &&
 
 	test_path_is_missing sub/d &&
 	test_path_is_file folder1/d &&
@@ -402,6 +419,11 @@ test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' '
 	test_cmp expect stderr &&
 
 	git mv --sparse sub/dir folder1 2>stderr &&
+	cat dirty_error_header >expect &&
+	echo "folder1/dir/e2" >>expect &&
+	echo "folder1/dir/e3" >>expect &&
+	cat dirty_hint >>expect &&
+	test_cmp expect stderr &&
 
 	test_path_is_missing sub/dir &&
 	test_path_is_missing folder1/dir/e &&
-- 
2.37.0

[PATCH v2 9/9] mv: check overwrite for in-to-out move

[Shaoxuan Yuan]

Add checking logic for overwriting when moving from in-cone to
out-of-cone. It is the index version of the original overwrite logic.

Helped-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c                  | 12 ++++++++++++
 t/t7002-mv-sparse-checkout.sh |  2 +-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 765a1e8eb5..70996d582f 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -367,6 +367,18 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			goto act_on_entry;
 		}
 
+		if (ignore_sparse &&
+		    (dst_mode & SKIP_WORKTREE_DIR) &&
+		    index_entry_exists(&the_index, dst, strlen(dst))) {
+			bad = _("destination exists in the index");
+			if (force) {
+				if (verbose)
+					warning(_("overwriting '%s'"), dst);
+				bad = NULL;
+			} else {
+				goto act_on_entry;
+			}
+		}
 		/*
 		 * We check if the paths are in the sparse-checkout
 		 * definition as a very final check, since that
diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
index f0b32a2f70..50bcca583c 100755
--- a/t/t7002-mv-sparse-checkout.sh
+++ b/t/t7002-mv-sparse-checkout.sh
@@ -323,7 +323,7 @@ test_expect_success 'move clean path from in-cone to out-of-cone' '
 	grep "S folder1/d" actual
 '
 
-test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
+test_expect_success 'move clean path from in-cone to out-of-cone overwrite' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 	echo "sub/file1 overwrite" >sub/file1 &&
-- 
2.37.0

Re: [PATCH v2 9/9] mv: check overwrite for in-to-out move

[Victoria Dye]

Shaoxuan Yuan wrote:
> Add checking logic for overwriting when moving from in-cone to
> out-of-cone. It is the index version of the original overwrite logic.
> 
> Helped-by: Derrick Stolee <derrickstolee@github.com>
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>  builtin/mv.c                  | 12 ++++++++++++
>  t/t7002-mv-sparse-checkout.sh |  2 +-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 765a1e8eb5..70996d582f 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -367,6 +367,18 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			goto act_on_entry;
>  		}
>  
> +		if (ignore_sparse &&

If '--sparse' is specified...

> +		    (dst_mode & SKIP_WORKTREE_DIR) &&

...and the destination's parent directory is outside the sparse cone...

> +		    index_entry_exists(&the_index, dst, strlen(dst))) {

...and the destination file exists in the index, then we're going to be
overwriting an existing index entry. 

> +			bad = _("destination exists in the index");
> +			if (force) {
> +				if (verbose)
> +					warning(_("overwriting '%s'"), dst);
> +				bad = NULL;
> +			} else {
> +				goto act_on_entry;
> +			}
> +		}

The rest of this aligns with what's done for a normal (exists on-disk)
overwrite. There's not much in the original overwrite-handling logic that
can be reused here (it's checking for overwrite based on 'stat' rather than
the contents of the index), so code structure-wise this makes sense as a
standalone check.

Looking good!

>  		/*
>  		 * We check if the paths are in the sparse-checkout
>  		 * definition as a very final check, since that
> diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
> index f0b32a2f70..50bcca583c 100755
> --- a/t/t7002-mv-sparse-checkout.sh
> +++ b/t/t7002-mv-sparse-checkout.sh
> @@ -323,7 +323,7 @@ test_expect_success 'move clean path from in-cone to out-of-cone' '
>  	grep "S folder1/d" actual
>  '
>  
> -test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
> +test_expect_success 'move clean path from in-cone to out-of-cone overwrite' '
>  	test_when_finished "cleanup_sparse_checkout" &&
>  	setup_sparse_checkout &&
>  	echo "sub/file1 overwrite" >sub/file1 &&

[PATCH v3 0/9] mv: from in-cone to out-of-cone

[Shaoxuan Yuan]

## Changes since PATCH v2 ##

1. Rephrase the descriptions for clean moves to match the actual
   implementation.

2. Add test for moving to a file path <destination> with overwrite.

3. Add test for moving a directory as <source>, with one of its file
   overwrites file entry in <destination>.

4. *with_slash -> with_slash, remove the dereference sign.

5. Take care of in-to-out where <destination> is a file path, rather
   than a directory, this is what point 2 is testing.

6. Add a NEEDSWORK doc to address "out-to-out" moves.

## Changes since PATCH v1 ##

1. Change "grep -x" to ^ and $ in t7002 test, and remove some
   useless "-x" (lines that are *not* ambiguous).

2. Rename check_dir_in_index() to empty_dir_has_sparse_contents()
   and add more precise documentation.

3. Reverse return values from empty_dir_has_sparse_contents() to
   comply with the method's name.

4. Make some commit messages more natural/fluent/without typos.

5. Split commit "mv: from in-cone to out-of-cone" to two commits,
   one does in-to-out functionality, the other one does advice.

6. Take care a few memory leaks (`xstrdup`s).

7. Add a new way to recursively cleanup empty WORKING_DIRECTORY.

## leftover bits ##

I decided *not* to solve these bits in this series but in a future 
one, so things can be handled one at a time without exceeding my
capability.

1. When trying to move from-in-to-out, the mechanism assumes that
   the <destination> is out-of-cone, and by cone-mode definition,
   <destination> is basically an invisible directory 
   (SKIP_WORKTREE_DIR). However, the dirty move mechanism materializes
   the moved file to make sure the information is not lost, and this
   mechanism brings the invisible directory back into the worktree.
   Hence, if we want to perform a second move from-in-to-out, the
   assumption that <destination> is not on-disk is broken, and
   everything follows breaks too. 

2. A logic loophole introduced in the previous out-to-in patch,
   especially in b91a2b6594 (mv: add check_dir_in_index() and solve 
   general dir check issue). Please detach your head to b91a2b6594
   for context. Copy and paste: 
   
                    git switch b91a2b6594
   
   When moving from out-to-in, <source> is an invisible 
   SKIP_WORKTREE_DIR. Line 226 uses `lstat()` to make
   sure the <source> is not on-disk; then line 233-237 checks if
   <source> is a SKIP_WORKTREE_DIR. If the check passes, line 236
   jump to line 276 and try to verify <source> is a directory using
   `S_ISDIR`. However, because the prerequisite is that the line 226
   `lstat()` fails so we can go through the steps said above; and when
   it fails, the `st` stat, especially the `st_mode` member, is either
   an uninitialized chunk of garbage, or the result from previous
   `lstat()`. In this case, `st_mode` *is* from a previous `lstat()`,
   on line 205. This `lstat()` is testing the <destination>, which,
   under the out-to-in situation, is always an on-disk directory.
   Thus, by a series of coincidence, the `S_ISDIR()` on line 276
   succeed and everything *looks* OK test-wise. But clearly the `st`
   at this point is an impostor: it does not reflect the actual stat
   situation of <source> and it luckily slips through.

   This needs to be fixed.

3. A NEEDSWORK added to address "out-to-out" move.

## PATCH v1 info ##

This is a sister series to the previous "from out-of-cone to in-cone" [1]
series. This series is trying to make the opposite operation possible for
'mv', namely move <source>, which is in-cone, to <destination>, which is
out-of-cone.

Other than the main task, there are also some minor fixes done.

[1] https://lore.kernel.org/git/20220331091755.385961-1-shaoxuan.yuan02@gmail.com/

Shaoxuan Yuan (9):
  t7002: add tests for moving from in-cone to out-of-cone
  mv: rename check_dir_in_index() to empty_dir_has_sparse_contents()
  mv: free the with_slash in check_dir_in_index()
  mv: check if <destination> is a SKIP_WORKTREE_DIR
  mv: remove BOTH from enum update_mode
  mv: from in-cone to out-of-cone
  mv: cleanup empty WORKING_DIRECTORY
  advice.h: add advise_on_moving_dirty_path()
  mv: check overwrite for in-to-out move

 advice.c                      |  19 +++
 advice.h                      |   1 +
 builtin/mv.c                  | 159 ++++++++++++++++++++----
 t/t7002-mv-sparse-checkout.sh | 226 +++++++++++++++++++++++++++++++++-
 4 files changed, 378 insertions(+), 27 deletions(-)

Range-diff against v2:
 1:  a8c360c083 !  1:  8134fa5990 t7002: add tests for moving from in-cone to out-of-cone
    @@ Commit message
         Such <source> can be either clean or dirty, and moving it results in
         different behaviors:
     
    -    A clean move should move the <source> to the <destination>, both in the
    -    working tree and the index, then remove the resulted path from the
    -    working tree, and turn on its CE_SKIP_WORKTREE bit.
    +    A clean move should move <source> to <destination> in the index (do
    +    *not* create <destination> in the worktree), then delete <source> from
    +    the worktree.
     
         A dirty move should move the <source> to the <destination>, both in the
         working tree and the index, but should *not* remove the resulted path
    @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move sparse file to existing
     +	test_cmp expect actual
     +'
     +
    ++# This test is testing the same behavior as the
    ++# "move clean path from in-cone to out-of-cone overwrite" above.
    ++# The only difference is the <destination> changes from "folder1" to "folder1/file1"
    ++test_expect_failure 'move clean path from in-cone to out-of-cone file overwrite' '
    ++	test_when_finished "cleanup_sparse_checkout" &&
    ++	setup_sparse_checkout &&
    ++	echo "sub/file1 overwrite" >sub/file1 &&
    ++	git add sub/file1 &&
    ++
    ++	test_must_fail git mv sub/file1 folder1/file1 2>stderr &&
    ++	cat sparse_error_header >expect &&
    ++	echo "folder1/file1" >>expect &&
    ++	cat sparse_hint >>expect &&
    ++	test_cmp expect stderr &&
    ++
    ++	test_must_fail git mv --sparse sub/file1 folder1/file1 2>stderr &&
    ++	echo "fatal: destination exists in the index, source=sub/file1, destination=folder1/file1" \
    ++	>expect &&
    ++	test_cmp expect stderr &&
    ++
    ++	git mv --sparse -f sub/file1 folder1/file1 2>stderr &&
    ++	test_must_be_empty stderr &&
    ++
    ++	test_path_is_missing sub/file1 &&
    ++	test_path_is_missing folder1/file1 &&
    ++	git ls-files -t >actual &&
    ++	! grep "H sub/file1" actual &&
    ++	grep "S folder1/file1" actual &&
    ++
    ++	# compare file content before move and after move
    ++	echo "sub/file1 overwrite" >expect &&
    ++	git ls-files -s -- folder1/file1 | awk "{print \$2}" >oid &&
    ++	git cat-file blob $(cat oid) >actual &&
    ++	test_cmp expect actual
    ++'
    ++
    ++test_expect_failure 'move directory with one of the files overwrite' '
    ++	test_when_finished "cleanup_sparse_checkout" &&
    ++	mkdir -p folder1/dir &&
    ++	touch folder1/dir/file1 &&
    ++	git add folder1 &&
    ++	git sparse-checkout set --cone sub &&
    ++
    ++	echo test >sub/dir/file1 &&
    ++	git add sub/dir/file1 &&
    ++
    ++	test_must_fail git mv sub/dir folder1 2>stderr &&
    ++	cat sparse_error_header >expect &&
    ++	echo "folder1/dir/e" >>expect &&
    ++	echo "folder1/dir/file1" >>expect &&
    ++	cat sparse_hint >>expect &&
    ++	test_cmp expect stderr &&
    ++
    ++	test_must_fail git mv --sparse sub/dir folder1 2>stderr &&
    ++	echo "fatal: destination exists in the index, source=sub/dir/file1, destination=folder1/dir/file1" \
    ++	>expect &&
    ++	test_cmp expect stderr &&
    ++
    ++	git mv --sparse -f sub/dir folder1 2>stderr &&
    ++	test_must_be_empty stderr &&
    ++
    ++	test_path_is_missing sub/dir/file1 &&
    ++	test_path_is_missing sub/dir/e &&
    ++	test_path_is_missing folder1/file1 &&
    ++	git ls-files -t >actual &&
    ++	! grep "H sub/dir/file1" actual &&
    ++	! grep "H sub/dir/e" actual &&
    ++	grep "S folder1/dir/file1" actual &&
    ++
    ++	# compare file content before move and after move
    ++	echo test >expect &&
    ++	git ls-files -s -- folder1/dir/file1 | awk "{print \$2}" >oid &&
    ++	git cat-file blob $(cat oid) >actual &&
    ++	test_cmp expect actual
    ++'
    ++
     +test_expect_failure 'move dirty path from in-cone to out-of-cone' '
     +	test_when_finished "cleanup_sparse_checkout" &&
     +	setup_sparse_checkout &&
 2:  725a83c575 =  2:  cc5f2770de mv: rename check_dir_in_index() to empty_dir_has_sparse_contents()
 3:  1d4a0c16a6 !  3:  1780f36825 mv: free the *with_slash in check_dir_in_index()
    @@ Metadata
     Author: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
     
      ## Commit message ##
    -    mv: free the *with_slash in check_dir_in_index()
    +    mv: free the with_slash in check_dir_in_index()
     
    -    *with_slash may be a malloc'd pointer, and when it is, free it.
    +    with_slash may be a malloc'd pointer, and when it is, free it.
     
         Helped-by: Derrick Stolee <derrickstolee@github.com>
         Helped-by: Victoria Dye <vdye@github.com>
 4:  1f35f0eb34 =  4:  d935bd8d7a mv: check if <destination> is a SKIP_WORKTREE_DIR
 5:  17a871a06b =  5:  26f29df8c8 mv: remove BOTH from enum update_mode
 6:  32b9f85aa1 !  6:  2169b873f7 mv: from in-cone to out-of-cone
    @@ Commit message
         Change the behavior so that we can move an in-cone <source> to
         out-of-cone <destination> when --sparse is supplied.
     
    +    Notice that <destination> can also be an out-of-cone file path, rather
    +    than a directory.
    +
         Such <source> can be either clean or dirty, and moving it results in
         different behaviors:
     
    -    A clean move should move the <source> to the <destination>, both in the
    -    working tree and the index, then remove the resulted path from the
    -    working tree, and turn on its CE_SKIP_WORKTREE bit.
    +    A clean move should move <source> to <destination> in the index (do
    +    *not* create <destination> in the worktree), then delete <source> from
    +    the worktree.
     
         A dirty move should move the <source> to the <destination>, both in the
         working tree and the index, but should *not* remove the resulted path
         from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit.
     
    +    Optional reading
    +    ================
    +
    +    We are strict about cone mode when <destination> is a file path.
    +    The reason is that some of the previous tests that use no-cone mode in
    +    t7002 are keep breaking, mainly because the `dst_mode = SPARSE;` line
    +    added in this patch.
    +
    +    Most features developed in both "from-out-to-in" and "from-in-to-out"
    +    only care about cone mode situation, as no-cone mode is becoming
    +    irrelevant. And because assigning `SPARSE` to `dst_mode` when the
    +    repo is in no-cone mode causes miscellaneous bugs, we should just leave
    +    this new functionality to be exclusive cone mode and save some time.
    +
         Helped-by: Derrick Stolee <derrickstolee@github.com>
         Helped-by: Victoria Dye <vdye@github.com>
         Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
    @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
      		} else if (argc != 1) {
      			die(_("destination '%s' is not a directory"), dest_path[0]);
      		} else {
    + 			destination = dest_path;
    ++			/*
    ++			 * <destination> is a file outside of sparse-checkout
    ++			 * cone. Insist on cone mode here for backward
    ++			 * compatibility. We don't want dst_mode to be assigned
    ++			 * for a file when the repo is using no-cone mode (which
    ++			 * is deprecated at this point) sparse-checkout. As
    ++			 * SPARSE here is only considering cone-mode situation.
    ++			 */
    ++			if (!path_in_cone_mode_sparse_checkout(destination[0], &the_index))
    ++				dst_mode = SPARSE;
    + 		}
    + 	}
    + 	if (dst_w_slash != dest_path[0]) {
     @@ 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];
    @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
      		if (show_only)
      			continue;
      		if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
    -+		    !(dst_mode & SKIP_WORKTREE_DIR) &&
    ++		    !(dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
      		    rename(src, dst) < 0) {
      			if (ignore_errors)
      				continue;
    @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
     +		if (ignore_sparse &&
     +		    core_apply_sparse_checkout &&
     +		    core_sparse_checkout_cone) {
    ++			/*
    ++			 * NEEDSWORK: we are *not* paying attention to
    ++			 * "out-to-out" move (<source> is out-of-cone and
    ++			 * <destination> is out-of-cone) at this point. It
    ++			 * should be added in a future patch.
    ++			 */
     +			if ((mode & SPARSE) &&
     +			    path_in_sparse_checkout(dst, &the_index)) {
     +				/* from out-of-cone to in-cone */
    @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
     +
     +				if (checkout_entry(dst_ce, &state, NULL, NULL))
     +					die(_("cannot checkout %s"), dst_ce->name);
    -+			} else if ((dst_mode & SKIP_WORKTREE_DIR) &&
    ++			} else if ((dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
     +				   !(mode & SPARSE) &&
     +				   !path_in_sparse_checkout(dst, &the_index)) {
     +				/* from in-cone to out-of-cone */
    @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move sparse file to existing
      	test_when_finished "cleanup_sparse_checkout" &&
      	setup_sparse_checkout &&
      
    -@@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
    +@@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'move directory with one of the files overwrite' '
      	test_cmp expect actual
      '
      
 7:  449dba3853 =  7:  78a55e2a46 mv: cleanup empty WORKING_DIRECTORY
 8:  875756480e =  8:  43ce1c07ec advice.h: add advise_on_moving_dirty_path()
 9:  cd5d30505b !  9:  2e7c6a33c2 mv: check overwrite for in-to-out move
    @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
      		}
      
     +		if (ignore_sparse &&
    -+		    (dst_mode & SKIP_WORKTREE_DIR) &&
    ++		    (dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
     +		    index_entry_exists(&the_index, dst, strlen(dst))) {
     +			bad = _("destination exists in the index");
     +			if (force) {
    @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move clean path from in-cone
      	test_when_finished "cleanup_sparse_checkout" &&
      	setup_sparse_checkout &&
      	echo "sub/file1 overwrite" >sub/file1 &&
    +@@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
    + # This test is testing the same behavior as the
    + # "move clean path from in-cone to out-of-cone overwrite" above.
    + # The only difference is the <destination> changes from "folder1" to "folder1/file1"
    +-test_expect_failure 'move clean path from in-cone to out-of-cone file overwrite' '
    ++test_expect_success 'move clean path from in-cone to out-of-cone file overwrite' '
    + 	test_when_finished "cleanup_sparse_checkout" &&
    + 	setup_sparse_checkout &&
    + 	echo "sub/file1 overwrite" >sub/file1 &&
    +@@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'move clean path from in-cone to out-of-cone file overwrite'
    + 	test_cmp expect actual
    + '
    + 
    +-test_expect_failure 'move directory with one of the files overwrite' '
    ++test_expect_success 'move directory with one of the files overwrite' '
    + 	test_when_finished "cleanup_sparse_checkout" &&
    + 	mkdir -p folder1/dir &&
    + 	touch folder1/dir/file1 &&

base-commit: c50926e1f48891e2671e1830dbcd2912a4563450
-- 
2.37.0

[PATCH v3 1/9] t7002: add tests for moving from in-cone to out-of-cone

[Shaoxuan Yuan]

Add corresponding tests to test that user can move an in-cone <source>
to out-of-cone <destination> when --sparse is supplied.

Such <source> can be either clean or dirty, and moving it results in
different behaviors:

A clean move should move <source> to <destination> in the index (do
*not* create <destination> in the worktree), then delete <source> from
the worktree.

A dirty move should move the <source> to the <destination>, both in the
working tree and the index, but should *not* remove the resulted path
from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit.

Also make sure that if <destination> exists in the index (existing
check for if <destination> is in the worktree is not enough in
in-to-out moves), warn user against the overwrite. And Git should force
the overwrite when supplied with -f or --force.

Helped-by: Derrick Stolee <derrickstolee@github.com>
Helped-by: Victoria Dye <vdye@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 t/t7002-mv-sparse-checkout.sh | 198 ++++++++++++++++++++++++++++++++++
 1 file changed, 198 insertions(+)

diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
index 71fe29690f..1ac78edde6 100755
--- a/t/t7002-mv-sparse-checkout.sh
+++ b/t/t7002-mv-sparse-checkout.sh
@@ -290,4 +290,202 @@ test_expect_success 'move sparse file to existing destination with --force and -
 	test_cmp expect sub/file1
 '
 
+test_expect_failure 'move clean path from in-cone to out-of-cone' '
+	test_when_finished "cleanup_sparse_checkout" &&
+	setup_sparse_checkout &&
+
+	test_must_fail git mv sub/d folder1 2>stderr &&
+	cat sparse_error_header >expect &&
+	echo "folder1/d" >>expect &&
+	cat sparse_hint >>expect &&
+	test_cmp expect stderr &&
+
+	git mv --sparse sub/d folder1 2>stderr &&
+	test_must_be_empty stderr &&
+
+	test_path_is_missing sub/d &&
+	test_path_is_missing folder1/d &&
+	git ls-files -t >actual &&
+	! grep "^H sub/d\$" actual &&
+	grep "S folder1/d" actual
+'
+
+test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
+	test_when_finished "cleanup_sparse_checkout" &&
+	setup_sparse_checkout &&
+	echo "sub/file1 overwrite" >sub/file1 &&
+	git add sub/file1 &&
+
+	test_must_fail git mv sub/file1 folder1 2>stderr &&
+	cat sparse_error_header >expect &&
+	echo "folder1/file1" >>expect &&
+	cat sparse_hint >>expect &&
+	test_cmp expect stderr &&
+
+	test_must_fail git mv --sparse sub/file1 folder1 2>stderr &&
+	echo "fatal: destination exists in the index, source=sub/file1, destination=folder1/file1" \
+	>expect &&
+	test_cmp expect stderr &&
+
+	git mv --sparse -f sub/file1 folder1 2>stderr &&
+	test_must_be_empty stderr &&
+
+	test_path_is_missing sub/file1 &&
+	test_path_is_missing folder1/file1 &&
+	git ls-files -t >actual &&
+	! grep "H sub/file1" actual &&
+	grep "S folder1/file1" actual &&
+
+	# compare file content before move and after move
+	echo "sub/file1 overwrite" >expect &&
+	git ls-files -s -- folder1/file1 | awk "{print \$2}" >oid &&
+	git cat-file blob $(cat oid) >actual &&
+	test_cmp expect actual
+'
+
+# This test is testing the same behavior as the
+# "move clean path from in-cone to out-of-cone overwrite" above.
+# The only difference is the <destination> changes from "folder1" to "folder1/file1"
+test_expect_failure 'move clean path from in-cone to out-of-cone file overwrite' '
+	test_when_finished "cleanup_sparse_checkout" &&
+	setup_sparse_checkout &&
+	echo "sub/file1 overwrite" >sub/file1 &&
+	git add sub/file1 &&
+
+	test_must_fail git mv sub/file1 folder1/file1 2>stderr &&
+	cat sparse_error_header >expect &&
+	echo "folder1/file1" >>expect &&
+	cat sparse_hint >>expect &&
+	test_cmp expect stderr &&
+
+	test_must_fail git mv --sparse sub/file1 folder1/file1 2>stderr &&
+	echo "fatal: destination exists in the index, source=sub/file1, destination=folder1/file1" \
+	>expect &&
+	test_cmp expect stderr &&
+
+	git mv --sparse -f sub/file1 folder1/file1 2>stderr &&
+	test_must_be_empty stderr &&
+
+	test_path_is_missing sub/file1 &&
+	test_path_is_missing folder1/file1 &&
+	git ls-files -t >actual &&
+	! grep "H sub/file1" actual &&
+	grep "S folder1/file1" actual &&
+
+	# compare file content before move and after move
+	echo "sub/file1 overwrite" >expect &&
+	git ls-files -s -- folder1/file1 | awk "{print \$2}" >oid &&
+	git cat-file blob $(cat oid) >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'move directory with one of the files overwrite' '
+	test_when_finished "cleanup_sparse_checkout" &&
+	mkdir -p folder1/dir &&
+	touch folder1/dir/file1 &&
+	git add folder1 &&
+	git sparse-checkout set --cone sub &&
+
+	echo test >sub/dir/file1 &&
+	git add sub/dir/file1 &&
+
+	test_must_fail git mv sub/dir folder1 2>stderr &&
+	cat sparse_error_header >expect &&
+	echo "folder1/dir/e" >>expect &&
+	echo "folder1/dir/file1" >>expect &&
+	cat sparse_hint >>expect &&
+	test_cmp expect stderr &&
+
+	test_must_fail git mv --sparse sub/dir folder1 2>stderr &&
+	echo "fatal: destination exists in the index, source=sub/dir/file1, destination=folder1/dir/file1" \
+	>expect &&
+	test_cmp expect stderr &&
+
+	git mv --sparse -f sub/dir folder1 2>stderr &&
+	test_must_be_empty stderr &&
+
+	test_path_is_missing sub/dir/file1 &&
+	test_path_is_missing sub/dir/e &&
+	test_path_is_missing folder1/file1 &&
+	git ls-files -t >actual &&
+	! grep "H sub/dir/file1" actual &&
+	! grep "H sub/dir/e" actual &&
+	grep "S folder1/dir/file1" actual &&
+
+	# compare file content before move and after move
+	echo test >expect &&
+	git ls-files -s -- folder1/dir/file1 | awk "{print \$2}" >oid &&
+	git cat-file blob $(cat oid) >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'move dirty path from in-cone to out-of-cone' '
+	test_when_finished "cleanup_sparse_checkout" &&
+	setup_sparse_checkout &&
+	echo "modified" >>sub/d &&
+
+	test_must_fail git mv sub/d folder1 2>stderr &&
+	cat sparse_error_header >expect &&
+	echo "folder1/d" >>expect &&
+	cat sparse_hint >>expect &&
+	test_cmp expect stderr &&
+
+	git mv --sparse sub/d folder1 2>stderr &&
+
+	test_path_is_missing sub/d &&
+	test_path_is_file folder1/d &&
+	git ls-files -t >actual &&
+	! grep "^H sub/d\$" actual &&
+	grep "H folder1/d" actual
+'
+
+test_expect_failure 'move dir from in-cone to out-of-cone' '
+	test_when_finished "cleanup_sparse_checkout" &&
+	setup_sparse_checkout &&
+
+	test_must_fail git mv sub/dir folder1 2>stderr &&
+	cat sparse_error_header >expect &&
+	echo "folder1/dir/e" >>expect &&
+	cat sparse_hint >>expect &&
+	test_cmp expect stderr &&
+
+	git mv --sparse sub/dir folder1 2>stderr &&
+	test_must_be_empty stderr &&
+
+	test_path_is_missing folder1 &&
+	git ls-files -t >actual &&
+	! grep "H sub/dir/e" actual &&
+	grep "S folder1/dir/e" actual
+'
+
+test_expect_failure 'move partially-dirty dir from in-cone to out-of-cone' '
+	test_when_finished "cleanup_sparse_checkout" &&
+	setup_sparse_checkout &&
+	touch sub/dir/e2 sub/dir/e3 &&
+	git add sub/dir/e2 sub/dir/e3 &&
+	echo "modified" >>sub/dir/e2 &&
+	echo "modified" >>sub/dir/e3 &&
+
+	test_must_fail git mv sub/dir folder1 2>stderr &&
+	cat sparse_error_header >expect &&
+	echo "folder1/dir/e" >>expect &&
+	echo "folder1/dir/e2" >>expect &&
+	echo "folder1/dir/e3" >>expect &&
+	cat sparse_hint >>expect &&
+	test_cmp expect stderr &&
+
+	git mv --sparse sub/dir folder1 2>stderr &&
+
+	test_path_is_missing folder1/dir/e &&
+	test_path_is_file folder1/dir/e2 &&
+	test_path_is_file folder1/dir/e3 &&
+	git ls-files -t >actual &&
+	! grep "H sub/dir/e" actual &&
+	! grep "H sub/dir/e2" actual &&
+	! grep "H sub/dir/e3" actual &&
+	grep "S folder1/dir/e" actual &&
+	grep "H folder1/dir/e2" actual &&
+	grep "H folder1/dir/e3" actual
+'
+
 test_done
-- 
2.37.0

[PATCH v3 2/9] mv: rename check_dir_in_index() to empty_dir_has_sparse_contents()

[Shaoxuan Yuan]

Method check_dir_in_index() introduced in b91a2b6594 (mv: add
check_dir_in_index() and solve general dir check issue, 2022-06-30)
does not describe its intent and behavior well.

Change its name to empty_dir_has_sparse_contents(), which more
precisely describes its purpose.

Reverse the return values, check_dir_in_index() return 0 for success
and 1 for failure; reverse the values so empty_dir_has_sparse_contents()
return 1 for success and 0 for failure. These values are more intuitive
because 1 usually means "has" and 0 means "not found".

Also modify the documentation to better align with the method's
intent and behavior.

Helped-by: Derrick Stolee <derrickstolee@github.com>
Helped-by: Victoria Dye <vdye@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 4729bb1a1a..7c11b8f995 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -125,15 +125,13 @@ static int index_range_of_same_dir(const char *src, int length,
 }
 
 /*
- * 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.
+ * Given the path of a directory that does not exist on-disk, check whether the
+ * directory contains any entries in the index with the SKIP_WORKTREE flag
+ * enabled.
+ * Return 1 if such index entries exist.
+ * Return 0 otherwise.
  */
-static int check_dir_in_index(const char *name)
+static int empty_dir_has_sparse_contents(const char *name)
 {
 	const char *with_slash = add_slash(name);
 	int length = strlen(with_slash);
@@ -144,14 +142,14 @@ static int check_dir_in_index(const char *name)
 	if (pos < 0) {
 		pos = -pos - 1;
 		if (pos >= the_index.cache_nr)
-			return 1;
+			return 0;
 		ce = active_cache[pos];
 		if (strncmp(with_slash, ce->name, length))
-			return 1;
-		if (ce_skip_worktree(ce))
 			return 0;
+		if (ce_skip_worktree(ce))
+			return 1;
 	}
-	return 1;
+	return 0;
 }
 
 int cmd_mv(int argc, const char **argv, const char *prefix)
@@ -232,7 +230,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			if (pos < 0) {
 				const char *src_w_slash = add_slash(src);
 				if (!path_in_sparse_checkout(src_w_slash, &the_index) &&
-				    !check_dir_in_index(src)) {
+				    empty_dir_has_sparse_contents(src)) {
 					modes[i] |= SKIP_WORKTREE_DIR;
 					goto dir_check;
 				}
-- 
2.37.0

[PATCH v3 3/9] mv: free the with_slash in check_dir_in_index()

[Shaoxuan Yuan]

with_slash may be a malloc'd pointer, and when it is, free it.

Helped-by: Derrick Stolee <derrickstolee@github.com>
Helped-by: Victoria Dye <vdye@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 7c11b8f995..0a999640c9 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -133,6 +133,7 @@ static int index_range_of_same_dir(const char *src, int length,
  */
 static int empty_dir_has_sparse_contents(const char *name)
 {
+	int ret = 0;
 	const char *with_slash = add_slash(name);
 	int length = strlen(with_slash);
 
@@ -142,14 +143,18 @@ static int empty_dir_has_sparse_contents(const char *name)
 	if (pos < 0) {
 		pos = -pos - 1;
 		if (pos >= the_index.cache_nr)
-			return 0;
+			goto free_return;
 		ce = active_cache[pos];
 		if (strncmp(with_slash, ce->name, length))
-			return 0;
+			goto free_return;
 		if (ce_skip_worktree(ce))
-			return 1;
+			ret = 1;
 	}
-	return 0;
+
+free_return:
+	if (with_slash != name)
+		free((char *)with_slash);
+	return ret;
 }
 
 int cmd_mv(int argc, const char **argv, const char *prefix)
-- 
2.37.0

[PATCH v3 4/9] mv: check if <destination> is a SKIP_WORKTREE_DIR

[Shaoxuan Yuan]

Originally, <destination> is assumed to be in the working tree. If it is
not found as a directory, then it is determined to be either a regular file
path, or error out if used under the second form (move into a directory)
of 'git-mv'. Such behavior is not ideal, mainly because Git does not
look into the index for <destination>, which could potentially be a
SKIP_WORKTREE_DIR, which we need to determine for the later "moving from
in-cone to out-of-cone" patch.

Change the logic so that Git first check if <destination> is a directory
with all its contents sparsified (a SKIP_WORKTREE_DIR).

If <destination> is such a sparse directory, then we should modify the
index the same way as we would if this were a non-sparse directory. We
must be careful to ensure that the <destination> is marked with
SKIP_WORKTREE_DIR.

Also add a `dst_w_slash` to reuse the result from `add_slash()`, which
was everywhere and can be simplified.

Helped-by: Derrick Stolee <derrickstolee@github.com>
Helped-by: Victoria Dye <vdye@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 0a999640c9..f213a92bf6 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -171,6 +171,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 	const char **source, **destination, **dest_path, **submodule_gitfile;
+	const char *dst_w_slash;
 	enum update_mode *modes;
 	struct stat st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
@@ -201,6 +202,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	if (argc == 1 && is_directory(argv[0]) && !is_directory(argv[1]))
 		flags = 0;
 	dest_path = internal_prefix_pathspec(prefix, argv + argc, 1, flags);
+	dst_w_slash = add_slash(dest_path[0]);
 	submodule_gitfile = xcalloc(argc, sizeof(char *));
 
 	if (dest_path[0][0] == '\0')
@@ -208,12 +210,20 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
 	else if (!lstat(dest_path[0], &st) &&
 			S_ISDIR(st.st_mode)) {
-		dest_path[0] = add_slash(dest_path[0]);
-		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
+		destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
 	} else {
-		if (argc != 1)
+		if (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
+		    empty_dir_has_sparse_contents(dst_w_slash)) {
+			destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
+		} else if (argc != 1) {
 			die(_("destination '%s' is not a directory"), dest_path[0]);
-		destination = dest_path;
+		} else {
+			destination = dest_path;
+		}
+	}
+	if (dst_w_slash != dest_path[0]) {
+		free((char *)dst_w_slash);
+		dst_w_slash = NULL;
 	}
 
 	/* Checking */
-- 
2.37.0

[PATCH v3 5/9] mv: remove BOTH from enum update_mode

[Shaoxuan Yuan]

Since BOTH is not used anywhere in the code and its meaning is unclear,
remove it.

Helped-by: Derrick Stolee <derrickstolee@github.com>
Helped-by: Victoria Dye <vdye@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index f213a92bf6..1dc55153ed 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -21,7 +21,6 @@ static const char * const builtin_mv_usage[] = {
 };
 
 enum update_mode {
-	BOTH = 0,
 	WORKING_DIRECTORY = (1 << 1),
 	INDEX = (1 << 2),
 	SPARSE = (1 << 3),
-- 
2.37.0

[PATCH v3 6/9] mv: from in-cone to out-of-cone

[Shaoxuan Yuan]

Originally, moving an in-cone <source> to an out-of-cone <destination>
was not possible, mainly because such <destination> is a directory that
is not present in the working tree.

Change the behavior so that we can move an in-cone <source> to
out-of-cone <destination> when --sparse is supplied.

Notice that <destination> can also be an out-of-cone file path, rather
than a directory.

Such <source> can be either clean or dirty, and moving it results in
different behaviors:

A clean move should move <source> to <destination> in the index (do
*not* create <destination> in the worktree), then delete <source> from
the worktree.

A dirty move should move the <source> to the <destination>, both in the
working tree and the index, but should *not* remove the resulted path
from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit.

Optional reading
================

We are strict about cone mode when <destination> is a file path.
The reason is that some of the previous tests that use no-cone mode in
t7002 are keep breaking, mainly because the `dst_mode = SPARSE;` line
added in this patch.

Most features developed in both "from-out-to-in" and "from-in-to-out"
only care about cone mode situation, as no-cone mode is becoming
irrelevant. And because assigning `SPARSE` to `dst_mode` when the
repo is in no-cone mode causes miscellaneous bugs, we should just leave
this new functionality to be exclusive cone mode and save some time.

Helped-by: Derrick Stolee <derrickstolee@github.com>
Helped-by: Victoria Dye <vdye@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c                  | 71 ++++++++++++++++++++++++++++++-----
 t/t7002-mv-sparse-checkout.sh |  8 ++--
 2 files changed, 66 insertions(+), 13 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 1dc55153ed..6a6420c984 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -171,12 +171,13 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	};
 	const char **source, **destination, **dest_path, **submodule_gitfile;
 	const char *dst_w_slash;
-	enum update_mode *modes;
+	enum update_mode *modes, dst_mode = 0;
 	struct stat st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
 	struct lock_file lock_file = LOCK_INIT;
 	struct cache_entry *ce;
 	struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
+	struct string_list dirty_paths = STRING_LIST_INIT_NODUP;
 
 	git_config(git_default_config, NULL);
 
@@ -214,10 +215,21 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		if (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
 		    empty_dir_has_sparse_contents(dst_w_slash)) {
 			destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
+			dst_mode = SKIP_WORKTREE_DIR;
 		} else if (argc != 1) {
 			die(_("destination '%s' is not a directory"), dest_path[0]);
 		} else {
 			destination = dest_path;
+			/*
+			 * <destination> is a file outside of sparse-checkout
+			 * cone. Insist on cone mode here for backward
+			 * compatibility. We don't want dst_mode to be assigned
+			 * for a file when the repo is using no-cone mode (which
+			 * is deprecated at this point) sparse-checkout. As
+			 * SPARSE here is only considering cone-mode situation.
+			 */
+			if (!path_in_cone_mode_sparse_checkout(destination[0], &the_index))
+				dst_mode = SPARSE;
 		}
 	}
 	if (dst_w_slash != dest_path[0]) {
@@ -408,6 +420,7 @@ 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;
+		int sparse_and_dirty = 0;
 		struct checkout state = CHECKOUT_INIT;
 		state.istate = &the_index;
 
@@ -418,6 +431,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		if (show_only)
 			continue;
 		if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
+		    !(dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
 		    rename(src, dst) < 0) {
 			if (ignore_errors)
 				continue;
@@ -437,17 +451,55 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 
 		pos = cache_name_pos(src, strlen(src));
 		assert(pos >= 0);
+		if (!(mode & SPARSE) && !lstat(src, &st))
+			sparse_and_dirty = ce_modified(active_cache[pos], &st, 0);
 		rename_cache_entry_at(pos, dst);
 
-		if ((mode & SPARSE) &&
-		    (path_in_sparse_checkout(dst, &the_index))) {
-			int dst_pos;
+		if (ignore_sparse &&
+		    core_apply_sparse_checkout &&
+		    core_sparse_checkout_cone) {
+			/*
+			 * NEEDSWORK: we are *not* paying attention to
+			 * "out-to-out" move (<source> is out-of-cone and
+			 * <destination> is out-of-cone) at this point. It
+			 * should be added in a future patch.
+			 */
+			if ((mode & SPARSE) &&
+			    path_in_sparse_checkout(dst, &the_index)) {
+				/* from out-of-cone to in-cone */
+				int dst_pos = cache_name_pos(dst, strlen(dst));
+				struct cache_entry *dst_ce = active_cache[dst_pos];
+
+				dst_ce->ce_flags &= ~CE_SKIP_WORKTREE;
+
+				if (checkout_entry(dst_ce, &state, NULL, NULL))
+					die(_("cannot checkout %s"), dst_ce->name);
+			} else if ((dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
+				   !(mode & SPARSE) &&
+				   !path_in_sparse_checkout(dst, &the_index)) {
+				/* from in-cone to out-of-cone */
+				int dst_pos = cache_name_pos(dst, strlen(dst));
+				struct cache_entry *dst_ce = active_cache[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"), active_cache[dst_pos]->name);
+				/*
+				 * if src is clean, it will suffice to remove it
+				 */
+				if (!sparse_and_dirty) {
+					dst_ce->ce_flags |= CE_SKIP_WORKTREE;
+					unlink_or_warn(src);
+				} else {
+					/*
+					 * if src is dirty, move it to the
+					 * destination and create leading
+					 * dirs if necessary
+					 */
+					char *dst_dup = xstrdup(dst);
+					string_list_append(&dirty_paths, dst);
+					safe_create_leading_directories(dst_dup);
+					FREE_AND_NULL(dst_dup);
+					rename(src, dst);
+				}
+			}
 		}
 	}
 
@@ -459,6 +511,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		die(_("Unable to write new index file"));
 
 	string_list_clear(&src_for_dst, 0);
+	string_list_clear(&dirty_paths, 0);
 	UNLEAK(source);
 	UNLEAK(dest_path);
 	free(submodule_gitfile);
diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
index 1ac78edde6..d875f492dd 100755
--- a/t/t7002-mv-sparse-checkout.sh
+++ b/t/t7002-mv-sparse-checkout.sh
@@ -290,7 +290,7 @@ test_expect_success 'move sparse file to existing destination with --force and -
 	test_cmp expect sub/file1
 '
 
-test_expect_failure 'move clean path from in-cone to out-of-cone' '
+test_expect_success 'move clean path from in-cone to out-of-cone' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 
@@ -419,7 +419,7 @@ test_expect_failure 'move directory with one of the files overwrite' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'move dirty path from in-cone to out-of-cone' '
+test_expect_success 'move dirty path from in-cone to out-of-cone' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 	echo "modified" >>sub/d &&
@@ -439,7 +439,7 @@ test_expect_failure 'move dirty path from in-cone to out-of-cone' '
 	grep "H folder1/d" actual
 '
 
-test_expect_failure 'move dir from in-cone to out-of-cone' '
+test_expect_success 'move dir from in-cone to out-of-cone' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 
@@ -458,7 +458,7 @@ test_expect_failure 'move dir from in-cone to out-of-cone' '
 	grep "S folder1/dir/e" actual
 '
 
-test_expect_failure 'move partially-dirty dir from in-cone to out-of-cone' '
+test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 	touch sub/dir/e2 sub/dir/e3 &&
-- 
2.37.0

[PATCH v3 7/9] mv: cleanup empty WORKING_DIRECTORY

[Shaoxuan Yuan]

Originally, moving from-in-to-out may leave an empty <source>
directory on-disk (this kind of directory is marked as
WORKING_DIRECTORY).

Cleanup such directories if they are empty (don't have any entries
under them).

Modify two tests that take <source> as WORKING_DIRECTORY to test
this behavior.

Suggested-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c                  | 27 +++++++++++++++++++++++++++
 t/t7002-mv-sparse-checkout.sh |  4 ++++
 2 files changed, 31 insertions(+)

diff --git a/builtin/mv.c b/builtin/mv.c
index 6a6420c984..32e7ac4896 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -171,6 +171,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	};
 	const char **source, **destination, **dest_path, **submodule_gitfile;
 	const char *dst_w_slash;
+	const char **src_dir = NULL;
+	int src_dir_nr = 0, src_dir_alloc = 0;
+	struct strbuf a_src_dir = STRBUF_INIT;
 	enum update_mode *modes, dst_mode = 0;
 	struct stat st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
@@ -314,6 +317,10 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 
 			/* last - first >= 1 */
 			modes[i] |= WORKING_DIRECTORY;
+
+			ALLOC_GROW(src_dir, src_dir_nr + 1, src_dir_alloc);
+			src_dir[src_dir_nr++] = src;
+
 			n = argc + last - first;
 			REALLOC_ARRAY(source, n);
 			REALLOC_ARRAY(destination, n);
@@ -503,6 +510,26 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		}
 	}
 
+	/*
+	 * cleanup the empty src_dirs
+	 */
+	for (i = 0; i < src_dir_nr; i++) {
+		int dummy;
+		strbuf_addstr(&a_src_dir, src_dir[i]);
+		/*
+		 * if entries under a_src_dir are all moved away,
+		 * recursively remove a_src_dir to cleanup
+		 */
+		if (index_range_of_same_dir(a_src_dir.buf, a_src_dir.len,
+					    &dummy, &dummy) < 1) {
+			remove_dir_recursively(&a_src_dir, 0);
+		}
+		strbuf_reset(&a_src_dir);
+	}
+
+	strbuf_release(&a_src_dir);
+	free(src_dir);
+
 	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 d875f492dd..df8c0fa572 100755
--- a/t/t7002-mv-sparse-checkout.sh
+++ b/t/t7002-mv-sparse-checkout.sh
@@ -442,6 +442,7 @@ test_expect_success 'move dirty path from in-cone to out-of-cone' '
 test_expect_success 'move dir from in-cone to out-of-cone' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
+	mkdir sub/dir/deep &&
 
 	test_must_fail git mv sub/dir folder1 2>stderr &&
 	cat sparse_error_header >expect &&
@@ -452,6 +453,7 @@ test_expect_success 'move dir from in-cone to out-of-cone' '
 	git mv --sparse sub/dir folder1 2>stderr &&
 	test_must_be_empty stderr &&
 
+	test_path_is_missing sub/dir &&
 	test_path_is_missing folder1 &&
 	git ls-files -t >actual &&
 	! grep "H sub/dir/e" actual &&
@@ -461,6 +463,7 @@ test_expect_success 'move dir from in-cone to out-of-cone' '
 test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
+	mkdir sub/dir/deep &&
 	touch sub/dir/e2 sub/dir/e3 &&
 	git add sub/dir/e2 sub/dir/e3 &&
 	echo "modified" >>sub/dir/e2 &&
@@ -476,6 +479,7 @@ test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' '
 
 	git mv --sparse sub/dir folder1 2>stderr &&
 
+	test_path_is_missing sub/dir &&
 	test_path_is_missing folder1/dir/e &&
 	test_path_is_file folder1/dir/e2 &&
 	test_path_is_file folder1/dir/e3 &&
-- 
2.37.0

[PATCH v3 8/9] advice.h: add advise_on_moving_dirty_path()

[Shaoxuan Yuan]

Add an advice.

When the user use `git mv --sparse <dirty-path> <destination>`, Git
will warn the user to use `git add --sparse <paths>` then use
`git sparse-checkout reapply` to apply the sparsity rules.

Add a few lines to previous "move dirty path" tests so we can test
this new advice is working.

Suggested-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 advice.c                      | 19 +++++++++++++++++++
 advice.h                      |  1 +
 builtin/mv.c                  |  3 +++
 t/t7002-mv-sparse-checkout.sh | 24 +++++++++++++++++++++++-
 4 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/advice.c b/advice.c
index 6fda9edbc2..fd18968943 100644
--- a/advice.c
+++ b/advice.c
@@ -261,3 +261,22 @@ void detach_advice(const char *new_name)
 
 	fprintf(stderr, fmt, new_name);
 }
+
+void advise_on_moving_dirty_path(struct string_list *pathspec_list)
+{
+	struct string_list_item *item;
+
+	if (!pathspec_list->nr)
+		return;
+
+	fprintf(stderr, _("The following paths have been moved outside the\n"
+			  "sparse-checkout definition but are not sparse due to local\n"
+			  "modifications.\n"));
+	for_each_string_list_item(item, pathspec_list)
+		fprintf(stderr, "%s\n", item->string);
+
+	advise_if_enabled(ADVICE_UPDATE_SPARSE_PATH,
+			  _("To correct the sparsity of these paths, do the following:\n"
+			    "* Use \"git add --sparse <paths>\" to update the index\n"
+			    "* Use \"git sparse-checkout reapply\" to apply the sparsity rules"));
+}
diff --git a/advice.h b/advice.h
index 7ddc6cbc1a..07e0f76833 100644
--- a/advice.h
+++ b/advice.h
@@ -74,5 +74,6 @@ void NORETURN die_conclude_merge(void);
 void NORETURN die_ff_impossible(void);
 void advise_on_updating_sparse_paths(struct string_list *pathspec_list);
 void detach_advice(const char *new_name);
+void advise_on_moving_dirty_path(struct string_list *pathspec_list);
 
 #endif /* ADVICE_H */
diff --git a/builtin/mv.c b/builtin/mv.c
index 32e7ac4896..a396a030c7 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -530,6 +530,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	strbuf_release(&a_src_dir);
 	free(src_dir);
 
+	if (dirty_paths.nr)
+		advise_on_moving_dirty_path(&dirty_paths);
+
 	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 df8c0fa572..5e5eb70e7a 100755
--- a/t/t7002-mv-sparse-checkout.sh
+++ b/t/t7002-mv-sparse-checkout.sh
@@ -28,12 +28,25 @@ test_expect_success 'setup' "
 	updated in the index:
 	EOF
 
-	cat >sparse_hint <<-EOF
+	cat >sparse_hint <<-EOF &&
 	hint: If you intend to update such entries, try one of the following:
 	hint: * Use the --sparse option.
 	hint: * Disable or modify the sparsity rules.
 	hint: Disable this message with \"git config advice.updateSparsePath false\"
 	EOF
+
+	cat >dirty_error_header <<-EOF &&
+	The following paths have been moved outside the
+	sparse-checkout definition but are not sparse due to local
+	modifications.
+	EOF
+
+	cat >dirty_hint <<-EOF
+	hint: To correct the sparsity of these paths, do the following:
+	hint: * Use \"git add --sparse <paths>\" to update the index
+	hint: * Use \"git sparse-checkout reapply\" to apply the sparsity rules
+	hint: Disable this message with \"git config advice.updateSparsePath false\"
+	EOF
 "
 
 test_expect_success 'mv refuses to move sparse-to-sparse' '
@@ -431,6 +444,10 @@ test_expect_success 'move dirty path from in-cone to out-of-cone' '
 	test_cmp expect stderr &&
 
 	git mv --sparse sub/d folder1 2>stderr &&
+	cat dirty_error_header >expect &&
+	echo "folder1/d" >>expect &&
+	cat dirty_hint >>expect &&
+	test_cmp expect stderr &&
 
 	test_path_is_missing sub/d &&
 	test_path_is_file folder1/d &&
@@ -478,6 +495,11 @@ test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' '
 	test_cmp expect stderr &&
 
 	git mv --sparse sub/dir folder1 2>stderr &&
+	cat dirty_error_header >expect &&
+	echo "folder1/dir/e2" >>expect &&
+	echo "folder1/dir/e3" >>expect &&
+	cat dirty_hint >>expect &&
+	test_cmp expect stderr &&
 
 	test_path_is_missing sub/dir &&
 	test_path_is_missing folder1/dir/e &&
-- 
2.37.0

[PATCH v3 9/9] mv: check overwrite for in-to-out move

[Shaoxuan Yuan]

Add checking logic for overwriting when moving from in-cone to
out-of-cone. It is the index version of the original overwrite logic.

Helped-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 builtin/mv.c                  | 12 ++++++++++++
 t/t7002-mv-sparse-checkout.sh |  6 +++---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index a396a030c7..2d64c1e80f 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -377,6 +377,18 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			goto act_on_entry;
 		}
 
+		if (ignore_sparse &&
+		    (dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
+		    index_entry_exists(&the_index, dst, strlen(dst))) {
+			bad = _("destination exists in the index");
+			if (force) {
+				if (verbose)
+					warning(_("overwriting '%s'"), dst);
+				bad = NULL;
+			} else {
+				goto act_on_entry;
+			}
+		}
 		/*
 		 * We check if the paths are in the sparse-checkout
 		 * definition as a very final check, since that
diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
index 5e5eb70e7a..26582ae4e5 100755
--- a/t/t7002-mv-sparse-checkout.sh
+++ b/t/t7002-mv-sparse-checkout.sh
@@ -323,7 +323,7 @@ test_expect_success 'move clean path from in-cone to out-of-cone' '
 	grep "S folder1/d" actual
 '
 
-test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
+test_expect_success 'move clean path from in-cone to out-of-cone overwrite' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 	echo "sub/file1 overwrite" >sub/file1 &&
@@ -359,7 +359,7 @@ test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
 # This test is testing the same behavior as the
 # "move clean path from in-cone to out-of-cone overwrite" above.
 # The only difference is the <destination> changes from "folder1" to "folder1/file1"
-test_expect_failure 'move clean path from in-cone to out-of-cone file overwrite' '
+test_expect_success 'move clean path from in-cone to out-of-cone file overwrite' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 	echo "sub/file1 overwrite" >sub/file1 &&
@@ -392,7 +392,7 @@ test_expect_failure 'move clean path from in-cone to out-of-cone file overwrite'
 	test_cmp expect actual
 '
 
-test_expect_failure 'move directory with one of the files overwrite' '
+test_expect_success 'move directory with one of the files overwrite' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	mkdir -p folder1/dir &&
 	touch folder1/dir/file1 &&
-- 
2.37.0

Re: [PATCH v3 0/9] mv: from in-cone to out-of-cone

[Victoria Dye]

Shaoxuan Yuan wrote:
> ## Changes since PATCH v2 ##
> 
> 1. Rephrase the descriptions for clean moves to match the actual
>    implementation.
> 
> 2. Add test for moving to a file path <destination> with overwrite.
> 
> 3. Add test for moving a directory as <source>, with one of its file
>    overwrites file entry in <destination>.
> 
> 4. *with_slash -> with_slash, remove the dereference sign.
> 
> 5. Take care of in-to-out where <destination> is a file path, rather
>    than a directory, this is what point 2 is testing.
> 
> 6. Add a NEEDSWORK doc to address "out-to-out" moves.
> 
> ## Changes since PATCH v1 ##
> 
> 1. Change "grep -x" to ^ and $ in t7002 test, and remove some
>    useless "-x" (lines that are *not* ambiguous).
> 
> 2. Rename check_dir_in_index() to empty_dir_has_sparse_contents()
>    and add more precise documentation.
> 
> 3. Reverse return values from empty_dir_has_sparse_contents() to
>    comply with the method's name.
> 
> 4. Make some commit messages more natural/fluent/without typos.
> 
> 5. Split commit "mv: from in-cone to out-of-cone" to two commits,
>    one does in-to-out functionality, the other one does advice.
> 
> 6. Take care a few memory leaks (`xstrdup`s).
> 
> 7. Add a new way to recursively cleanup empty WORKING_DIRECTORY.
> 
> ## leftover bits ##
> 
> I decided *not* to solve these bits in this series but in a future 
> one, so things can be handled one at a time without exceeding my
> capability.
> 
> 1. When trying to move from-in-to-out, the mechanism assumes that
>    the <destination> is out-of-cone, and by cone-mode definition,
>    <destination> is basically an invisible directory 
>    (SKIP_WORKTREE_DIR). However, the dirty move mechanism materializes
>    the moved file to make sure the information is not lost, and this
>    mechanism brings the invisible directory back into the worktree.
>    Hence, if we want to perform a second move from-in-to-out, the
>    assumption that <destination> is not on-disk is broken, and
>    everything follows breaks too. 
> 
> 2. A logic loophole introduced in the previous out-to-in patch,
>    especially in b91a2b6594 (mv: add check_dir_in_index() and solve 
>    general dir check issue). Please detach your head to b91a2b6594
>    for context. Copy and paste: 
>    
>                     git switch b91a2b6594
>    
>    When moving from out-to-in, <source> is an invisible 
>    SKIP_WORKTREE_DIR. Line 226 uses `lstat()` to make
>    sure the <source> is not on-disk; then line 233-237 checks if
>    <source> is a SKIP_WORKTREE_DIR. If the check passes, line 236
>    jump to line 276 and try to verify <source> is a directory using
>    `S_ISDIR`. However, because the prerequisite is that the line 226
>    `lstat()` fails so we can go through the steps said above; and when
>    it fails, the `st` stat, especially the `st_mode` member, is either
>    an uninitialized chunk of garbage, or the result from previous
>    `lstat()`. In this case, `st_mode` *is* from a previous `lstat()`,
>    on line 205. This `lstat()` is testing the <destination>, which,
>    under the out-to-in situation, is always an on-disk directory.
>    Thus, by a series of coincidence, the `S_ISDIR()` on line 276
>    succeed and everything *looks* OK test-wise. But clearly the `st`
>    at this point is an impostor: it does not reflect the actual stat
>    situation of <source> and it luckily slips through.
> 
>    This needs to be fixed.
> 
> 3. A NEEDSWORK added to address "out-to-out" move.
> 
> ## PATCH v1 info ##
> 
> This is a sister series to the previous "from out-of-cone to in-cone" [1]
> series. This series is trying to make the opposite operation possible for
> 'mv', namely move <source>, which is in-cone, to <destination>, which is
> out-of-cone.
> 
> Other than the main task, there are also some minor fixes done.
> 
> [1] https://lore.kernel.org/git/20220331091755.385961-1-shaoxuan.yuan02@gmail.com/
> 

(Sorry for the slow response) 

The range-diff below looks good to me (w.r.t feedback on the last version).
I do still find patch 4 [1] a bit cluttered, but it's a minor point and
otherwise everything looks functional; I think this is probably ready for
'next'.

Thanks!

> Shaoxuan Yuan (9):
>   t7002: add tests for moving from in-cone to out-of-cone
>   mv: rename check_dir_in_index() to empty_dir_has_sparse_contents()
>   mv: free the with_slash in check_dir_in_index()
>   mv: check if <destination> is a SKIP_WORKTREE_DIR
>   mv: remove BOTH from enum update_mode
>   mv: from in-cone to out-of-cone
>   mv: cleanup empty WORKING_DIRECTORY
>   advice.h: add advise_on_moving_dirty_path()
>   mv: check overwrite for in-to-out move
> 
>  advice.c                      |  19 +++
>  advice.h                      |   1 +
>  builtin/mv.c                  | 159 ++++++++++++++++++++----
>  t/t7002-mv-sparse-checkout.sh | 226 +++++++++++++++++++++++++++++++++-
>  4 files changed, 378 insertions(+), 27 deletions(-)
> 
> Range-diff against v2:
>  1:  a8c360c083 !  1:  8134fa5990 t7002: add tests for moving from in-cone to out-of-cone
>     @@ Commit message
>          Such <source> can be either clean or dirty, and moving it results in
>          different behaviors:
>      
>     -    A clean move should move the <source> to the <destination>, both in the
>     -    working tree and the index, then remove the resulted path from the
>     -    working tree, and turn on its CE_SKIP_WORKTREE bit.
>     +    A clean move should move <source> to <destination> in the index (do
>     +    *not* create <destination> in the worktree), then delete <source> from
>     +    the worktree.
>      
>          A dirty move should move the <source> to the <destination>, both in the
>          working tree and the index, but should *not* remove the resulted path
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move sparse file to existing
>      +	test_cmp expect actual
>      +'
>      +
>     ++# This test is testing the same behavior as the
>     ++# "move clean path from in-cone to out-of-cone overwrite" above.
>     ++# The only difference is the <destination> changes from "folder1" to "folder1/file1"
>     ++test_expect_failure 'move clean path from in-cone to out-of-cone file overwrite' '
>     ++	test_when_finished "cleanup_sparse_checkout" &&
>     ++	setup_sparse_checkout &&
>     ++	echo "sub/file1 overwrite" >sub/file1 &&
>     ++	git add sub/file1 &&
>     ++
>     ++	test_must_fail git mv sub/file1 folder1/file1 2>stderr &&
>     ++	cat sparse_error_header >expect &&
>     ++	echo "folder1/file1" >>expect &&
>     ++	cat sparse_hint >>expect &&
>     ++	test_cmp expect stderr &&
>     ++
>     ++	test_must_fail git mv --sparse sub/file1 folder1/file1 2>stderr &&
>     ++	echo "fatal: destination exists in the index, source=sub/file1, destination=folder1/file1" \
>     ++	>expect &&
>     ++	test_cmp expect stderr &&
>     ++
>     ++	git mv --sparse -f sub/file1 folder1/file1 2>stderr &&
>     ++	test_must_be_empty stderr &&
>     ++
>     ++	test_path_is_missing sub/file1 &&
>     ++	test_path_is_missing folder1/file1 &&
>     ++	git ls-files -t >actual &&
>     ++	! grep "H sub/file1" actual &&
>     ++	grep "S folder1/file1" actual &&
>     ++
>     ++	# compare file content before move and after move
>     ++	echo "sub/file1 overwrite" >expect &&
>     ++	git ls-files -s -- folder1/file1 | awk "{print \$2}" >oid &&
>     ++	git cat-file blob $(cat oid) >actual &&
>     ++	test_cmp expect actual
>     ++'
>     ++
>     ++test_expect_failure 'move directory with one of the files overwrite' '
>     ++	test_when_finished "cleanup_sparse_checkout" &&
>     ++	mkdir -p folder1/dir &&
>     ++	touch folder1/dir/file1 &&
>     ++	git add folder1 &&
>     ++	git sparse-checkout set --cone sub &&
>     ++
>     ++	echo test >sub/dir/file1 &&
>     ++	git add sub/dir/file1 &&
>     ++
>     ++	test_must_fail git mv sub/dir folder1 2>stderr &&
>     ++	cat sparse_error_header >expect &&
>     ++	echo "folder1/dir/e" >>expect &&
>     ++	echo "folder1/dir/file1" >>expect &&
>     ++	cat sparse_hint >>expect &&
>     ++	test_cmp expect stderr &&
>     ++
>     ++	test_must_fail git mv --sparse sub/dir folder1 2>stderr &&
>     ++	echo "fatal: destination exists in the index, source=sub/dir/file1, destination=folder1/dir/file1" \
>     ++	>expect &&
>     ++	test_cmp expect stderr &&
>     ++
>     ++	git mv --sparse -f sub/dir folder1 2>stderr &&
>     ++	test_must_be_empty stderr &&
>     ++
>     ++	test_path_is_missing sub/dir/file1 &&
>     ++	test_path_is_missing sub/dir/e &&
>     ++	test_path_is_missing folder1/file1 &&
>     ++	git ls-files -t >actual &&
>     ++	! grep "H sub/dir/file1" actual &&
>     ++	! grep "H sub/dir/e" actual &&
>     ++	grep "S folder1/dir/file1" actual &&
>     ++
>     ++	# compare file content before move and after move
>     ++	echo test >expect &&
>     ++	git ls-files -s -- folder1/dir/file1 | awk "{print \$2}" >oid &&
>     ++	git cat-file blob $(cat oid) >actual &&
>     ++	test_cmp expect actual
>     ++'
>     ++
>      +test_expect_failure 'move dirty path from in-cone to out-of-cone' '
>      +	test_when_finished "cleanup_sparse_checkout" &&
>      +	setup_sparse_checkout &&
>  2:  725a83c575 =  2:  cc5f2770de mv: rename check_dir_in_index() to empty_dir_has_sparse_contents()
>  3:  1d4a0c16a6 !  3:  1780f36825 mv: free the *with_slash in check_dir_in_index()
>     @@ Metadata
>      Author: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
>      
>       ## Commit message ##
>     -    mv: free the *with_slash in check_dir_in_index()
>     +    mv: free the with_slash in check_dir_in_index()
>      
>     -    *with_slash may be a malloc'd pointer, and when it is, free it.
>     +    with_slash may be a malloc'd pointer, and when it is, free it.
>      
>          Helped-by: Derrick Stolee <derrickstolee@github.com>
>          Helped-by: Victoria Dye <vdye@github.com>
>  4:  1f35f0eb34 =  4:  d935bd8d7a mv: check if <destination> is a SKIP_WORKTREE_DIR
>  5:  17a871a06b =  5:  26f29df8c8 mv: remove BOTH from enum update_mode
>  6:  32b9f85aa1 !  6:  2169b873f7 mv: from in-cone to out-of-cone
>     @@ Commit message
>          Change the behavior so that we can move an in-cone <source> to
>          out-of-cone <destination> when --sparse is supplied.
>      
>     +    Notice that <destination> can also be an out-of-cone file path, rather
>     +    than a directory.
>     +
>          Such <source> can be either clean or dirty, and moving it results in
>          different behaviors:
>      
>     -    A clean move should move the <source> to the <destination>, both in the
>     -    working tree and the index, then remove the resulted path from the
>     -    working tree, and turn on its CE_SKIP_WORKTREE bit.
>     +    A clean move should move <source> to <destination> in the index (do
>     +    *not* create <destination> in the worktree), then delete <source> from
>     +    the worktree.
>      
>          A dirty move should move the <source> to the <destination>, both in the
>          working tree and the index, but should *not* remove the resulted path
>          from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit.
>      
>     +    Optional reading
>     +    ================
>     +
>     +    We are strict about cone mode when <destination> is a file path.
>     +    The reason is that some of the previous tests that use no-cone mode in
>     +    t7002 are keep breaking, mainly because the `dst_mode = SPARSE;` line
>     +    added in this patch.
>     +
>     +    Most features developed in both "from-out-to-in" and "from-in-to-out"
>     +    only care about cone mode situation, as no-cone mode is becoming
>     +    irrelevant. And because assigning `SPARSE` to `dst_mode` when the
>     +    repo is in no-cone mode causes miscellaneous bugs, we should just leave
>     +    this new functionality to be exclusive cone mode and save some time.
>     +
>          Helped-by: Derrick Stolee <derrickstolee@github.com>
>          Helped-by: Victoria Dye <vdye@github.com>
>          Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
>     @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>       		} else if (argc != 1) {
>       			die(_("destination '%s' is not a directory"), dest_path[0]);
>       		} else {
>     + 			destination = dest_path;
>     ++			/*
>     ++			 * <destination> is a file outside of sparse-checkout
>     ++			 * cone. Insist on cone mode here for backward
>     ++			 * compatibility. We don't want dst_mode to be assigned
>     ++			 * for a file when the repo is using no-cone mode (which
>     ++			 * is deprecated at this point) sparse-checkout. As
>     ++			 * SPARSE here is only considering cone-mode situation.
>     ++			 */
>     ++			if (!path_in_cone_mode_sparse_checkout(destination[0], &the_index))
>     ++				dst_mode = SPARSE;
>     + 		}
>     + 	}
>     + 	if (dst_w_slash != dest_path[0]) {
>      @@ 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];
>     @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>       		if (show_only)
>       			continue;
>       		if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
>     -+		    !(dst_mode & SKIP_WORKTREE_DIR) &&
>     ++		    !(dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
>       		    rename(src, dst) < 0) {
>       			if (ignore_errors)
>       				continue;
>     @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>      +		if (ignore_sparse &&
>      +		    core_apply_sparse_checkout &&
>      +		    core_sparse_checkout_cone) {
>     ++			/*
>     ++			 * NEEDSWORK: we are *not* paying attention to
>     ++			 * "out-to-out" move (<source> is out-of-cone and
>     ++			 * <destination> is out-of-cone) at this point. It
>     ++			 * should be added in a future patch.
>     ++			 */
>      +			if ((mode & SPARSE) &&
>      +			    path_in_sparse_checkout(dst, &the_index)) {
>      +				/* from out-of-cone to in-cone */
>     @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>      +
>      +				if (checkout_entry(dst_ce, &state, NULL, NULL))
>      +					die(_("cannot checkout %s"), dst_ce->name);
>     -+			} else if ((dst_mode & SKIP_WORKTREE_DIR) &&
>     ++			} else if ((dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
>      +				   !(mode & SPARSE) &&
>      +				   !path_in_sparse_checkout(dst, &the_index)) {
>      +				/* from in-cone to out-of-cone */
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move sparse file to existing
>       	test_when_finished "cleanup_sparse_checkout" &&
>       	setup_sparse_checkout &&
>       
>     -@@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
>     +@@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'move directory with one of the files overwrite' '
>       	test_cmp expect actual
>       '
>       
>  7:  449dba3853 =  7:  78a55e2a46 mv: cleanup empty WORKING_DIRECTORY
>  8:  875756480e =  8:  43ce1c07ec advice.h: add advise_on_moving_dirty_path()
>  9:  cd5d30505b !  9:  2e7c6a33c2 mv: check overwrite for in-to-out move
>     @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>       		}
>       
>      +		if (ignore_sparse &&
>     -+		    (dst_mode & SKIP_WORKTREE_DIR) &&
>     ++		    (dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
>      +		    index_entry_exists(&the_index, dst, strlen(dst))) {
>      +			bad = _("destination exists in the index");
>      +			if (force) {
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move clean path from in-cone
>       	test_when_finished "cleanup_sparse_checkout" &&
>       	setup_sparse_checkout &&
>       	echo "sub/file1 overwrite" >sub/file1 &&
>     +@@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
>     + # This test is testing the same behavior as the
>     + # "move clean path from in-cone to out-of-cone overwrite" above.
>     + # The only difference is the <destination> changes from "folder1" to "folder1/file1"
>     +-test_expect_failure 'move clean path from in-cone to out-of-cone file overwrite' '
>     ++test_expect_success 'move clean path from in-cone to out-of-cone file overwrite' '
>     + 	test_when_finished "cleanup_sparse_checkout" &&
>     + 	setup_sparse_checkout &&
>     + 	echo "sub/file1 overwrite" >sub/file1 &&
>     +@@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'move clean path from in-cone to out-of-cone file overwrite'
>     + 	test_cmp expect actual
>     + '
>     + 
>     +-test_expect_failure 'move directory with one of the files overwrite' '
>     ++test_expect_success 'move directory with one of the files overwrite' '
>     + 	test_when_finished "cleanup_sparse_checkout" &&
>     + 	mkdir -p folder1/dir &&
>     + 	touch folder1/dir/file1 &&
> 
> base-commit: c50926e1f48891e2671e1830dbcd2912a4563450