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

[Victoria Dye <vdye@github.com>]

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