Derrick Stolee wrote:

On 5/27/2022 6:08 AM, Shaoxuan Yuan wrote:

quoted

Originally, moving a <source> file which is not on-disk but exists in
index as a SKIP_WORKTREE enabled cache entry, "giv mv" command errors
out with "bad source".

Change the checking logic, so that such <source>
file makes "giv mv" command warns with "advise_on_updating_sparse_paths()"
instead of "bad source"; also user now can supply a "--sparse" flag so
this operation can be carried out successfully.

Signed-off-by: Shaoxuan Yuan <redacted>
---
 builtin/mv.c                  | 26 +++++++++++++++++++++++++-
 t/t7002-mv-sparse-checkout.sh |  4 ++--
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 83a465ba83..32ad4d5682 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c

@@ -185,8 +185,32 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 
 		length = strlen(src);
 		if (lstat(src, &st) < 0) {
+			/*
+			 * TODO: for now, when you try to overwrite a <destination>
+			 * with your <source> as a sparse file, if you supply a "--sparse"
+			 * flag, then the action will be done without providing "--force"
+			 * and no warning.
+			 *
+			 * This is mainly because the sparse <source>
+			 * is not on-disk, and this if-else chain will be cut off early in
+			 * this check, thus the "--force" check is ignored. Need fix.
+			 */

I wonder if this is worth the comment here, or if we'd rather see
the mention in the commit message. You have documented tests that
fail in this case, so we already have something that marks this
as "TODO" in a more discoverable place.

quoted

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

quoted

+				}
+				else
+					bad = _("bad source");

style nit:

	} else {
		bad = _("bad source");
	}

In case this advice seems contradictory with past style suggestions, from 'Documentation/CodingGuidelines':

	- When there are multiple arms to a conditional and some of them
	  require braces, enclose even a single line block in braces for
	  consistency. E.g.:

		if (foo) {
			doit();
		} else {
			one();
			two();
			three();
		}

quoted

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

For this one, the comment makes it difficult to connect the 'else
if' to its corresponding 'if'. Perhaps:

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

quoted

 		} else if (!strncmp(src, dst, length) &&
 				(dst[length] == 0 || dst[length] == '/')) {

In general, I found this if/else-if chain hard to grok, and
a lot of it is because we have "simple" cases at the end
and the complicated parts have ever-increasing nesting. This
is mostly due to the existing if/else-if chain in this method.

Agreed that the if/else-if chains make 'cmd_mv' complicated. The most
frustrating thing about its current state (unrelated to this patch) is how
unclear it is whether any given conditions are mutually-exclusive vs.
dependent vs. one taking precedence over another. On that note... 

Here is a diff that replaces that if/else-if chain with a
'goto' trick to jump ahead, allowing some code to decrease in
tabbing:

...while I'm usually hesitant to add more 'goto' labels to the code if it
can be avoided, I think that model fits this use case well.

---- >8 ----

[cutting the proposed refactor for space]

---- >8 ---

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

I like how the refactored version simplifies 'cmd_mv', and how it
correspondingly simplifies the new checks in this (Shaoxuan's) patch. It
does still leave us with one big, monolithic 'cmd_mv', so in an ideal world
I'd probably lean towards pulling the innards of the main for-loop(s) into a
few dedicated functions (like 'validate_move_candidate', 'move_entry').
However, I'm happy with any improvement, and this refactor would certainly
give us that!

Thanks,
-Stolee