Re: [PATCH v1 6/7] mv: from in-cone to out-of-cone

On 7/20/2022 2:14 AM, Derrick Stolee wrote:
 >> -        if ((mode & SPARSE) &&
 >> -            (path_in_sparse_checkout(dst, &the_index))) {
 >> -            int dst_pos;
 >> +        if (ignore_sparse &&
 >> +            core_apply_sparse_checkout &&
 >> +            core_sparse_checkout_cone) {
 >>
 >> -            dst_pos = cache_name_pos(dst, strlen(dst));
 >> -            active_cache[dst_pos]->ce_flags &= ~CE_SKIP_WORKTREE;
 >> +            /* from out-of-cone to in-cone */
 >> +            if ((mode & SPARSE) &&
 >> +                path_in_sparse_checkout(dst, &the_index)) {
 >> +                int dst_pos = cache_name_pos(dst, strlen(dst));
 >> +                struct cache_entry *dst_ce = active_cache[dst_pos];
 >>
 >> -            if (checkout_entry(active_cache[dst_pos], &state, NULL, 
NULL))
 >> -                die(_("cannot checkout %s"), 
active_cache[dst_pos]->name);
 >> +                dst_ce->ce_flags &= ~CE_SKIP_WORKTREE;
 >> +
 >> +                if (checkout_entry(dst_ce, &state, NULL, NULL))
 >> +                    die(_("cannot checkout %s"), dst_ce->name);
 >> +                continue;
 >> +            }
 >
 > Here, it helps to ignore whitespace changes. This out to in was already
 > handled by the existing implementation.

Yes, I think it would be better to let `diff` ignore the existing implementation. Are you suggesting the `-w` (--ignore-all-space) option of `diff`? I tried this option and it does not work. But another reason is that there *are* some changes that are different from the original out-to-in implementation, so even though it looks a bit messy, I think it makes sense.

 >> +            /* from in-cone to out-of-cone */
 >> +            if ((dst_mode & SKIP_WORKTREE_DIR) &&
 >
 > This is disjoint from the other case (because of 
!path_in_sparse_checkout()),
 > so maybe we could short-circuit with an "else if" here? You could put 
your
 > comments about the in-to-out or out-to-in inside the if blocks.

I tried an else-if but it does clutter the code a bit. I think I'll leave it as-is. Or do you mind show me a diff of your approach? To be honest, this disjoint here looks logically cleaner to me.

-- Thanks, Shaoxuan