Derrick Stolee wrote:
>
>
> 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@xxxxxxxxx>
>> ---
>> 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();
}
>> + }
>> /* 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]
> ---- >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
