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

[Derrick Stolee <derrickstolee@github.com>]

On 7/19/2022 9:28 AM, 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.
> 
> 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.
> Instead advise user to "git add" this path and run "git sparse-checkout
> reapply" to re-sparsify that path.

I feel like this patch should be two, instead: you can have one that
makes the commands succeed or fail properly, and another that outputs the
advice. As it is, it's a bit muddled as to what is necessary for the
movement of the index entry versus representing the error message.

This might mean that you want to pull the advice messages out of the tests
that are added in patch 1 and only apply those to the test as you implement
the advice in that later patch. Such a split of the test implementation
would allow this patch to still switch a bunch of test_expect_failure tests
to test_expect_success.

> @@ -424,6 +426,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) &&

Here is a use of dst_mode that might be helpful to put with the change in
patch 4. Alternatively, you could delay declaring dst_mode until now.

> +		if (!(mode & SPARSE) && !lstat(src, &st))
> +			up_to_date = !ce_modified(active_cache[pos], &st, 0);

Here, you are only checking for a dirty file if (mode & SPARSE) and the
file exists.

Perhaps it would be helpful to negate this and rename the variable?

	sparse_and_dirty = ce_modified(active_cache[pos], &st, 0);

This makes it clear that we can't use the variable except in this
particular case.

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

> +			/* 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.

> +			    !(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];

(It took me a while to realize that dst_ce is the right entry because we
already called rename_cache_entry_at() earlier.)

> +				char *src_dir = dirname(xstrdup(src));

The xstrdup(src) return string is being lost here.

> +
> +				if (up_to_date) {

Based on my recommendation earlier, this would become

	if (!sparse_and_dirty) {

> +					dst_ce->ce_flags |= CE_SKIP_WORKTREE;
> +					unlink_or_warn(src);
> +				} else {
> +					string_list_append(&dirty_paths, dst);
> +					safe_create_leading_directories(xstrdup(dst));
> +					rename(src, dst);

Ok, still doing the file rename, but leaving it unstaged.

> +				}

Please provide some whitespace between the close of an if/else chain before
starting the next if.

> +				if ((mode & INDEX) && is_empty_dir(src_dir))
> +					rmdir_or_warn(src_dir);

This is an interesting cleanup of the first-level directory. Should it be
recursive and clean up an entire chain of paths?

Thanks,
-Stolee