Re: [WIP v2 2/5] mv: check if out-of-cone file exists in index with SKIP_WORKTREE bit

From: Victoria Dye <vdye@github.com>
Date: May 27, 2022, 22:38 UTC
Message-ID: <b4c94cd9-4afe-cb0b-f60b-48a359f06ae8@github.com>
In-Reply-To: <0884b97b-0745-5cad-3034-a679be5d6c3a@github.com>

Derrick Stolee wrote:

Show 63 quoted lines

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

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();
		}

Show 22 quoted lines

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

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]

Show 8 quoted lines

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