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

Shaoxuan Yuan <shaoxuan.yuan02@xxxxxxxxx> · Tue, 9 Aug 2022 11:16:03 +0800

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@xxxxxxxxx/
[2] https://lore.kernel.org/git/bd80881d-b2a3-c220-8f2d-a07a46e14207@xxxxxxxxxx/

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

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@xxxxxxxxxx>
Helped-by: Victoria Dye <vdye@xxxxxxxxxx>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@xxxxxxxxx>
---
  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!








