Re: [WIP v1 1/4] mv: check if out-of-cone file exists in index with SKIP_WORKTREE bit

[Victoria Dye <vdye@github.com>]

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

Good commit message, this clearly explains your changes!

> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
> I found a new problem introduced by this patch, it is written in the TODO.
> I still haven't found a better way to reconcile this conflict. Please enlighten
> me on this :-)
> 
>  builtin/mv.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> 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 can clarify this a bit. 'mv' is done in two steps: first the file-on-disk
rename (in the call to 'rename()'), then the index entry (in
'rename_cache_entry_at()'). In the case of a sparse file, you're only
dealing with the latter. However, 'rename_cache_entry_at()' moves the index
entry with the flag 'ADD_CACHE_OK_TO_REPLACE', since it leaves it up to
'cmd_mv()' to enforce the "no overwrite" rule. 

So, in the case of moving *to* a SKIP_WORKTREE entry (where a file being
present won't trigger the failure), you'll want to check that the
destination *index entry* doesn't exist in addition to the 'lstat()' check.
It might require some rearranging of if-statements in this block, but I
think it can be done in 'cmd_mv'. 

> +			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");

This block is good. At first, I thought it was mishandling the
'!ignore_sparse' case (i.e., that case should have included the "bad source"
assignment), but using the 'only_match_skip_worktree' list is the
appropriate way to handle it.

> +			}
>  			/* only error if existence is expected. */
> -			if (modes[i] != SPARSE)
> +			else if (modes[i] != SPARSE)
>  				bad = _("bad source");
>  		} else if (!strncmp(src, dst, length) &&
>  				(dst[length] == 0 || dst[length] == '/')) {

For a change like this, it would be really helpful to include the tests
showing how sparse file moves should now be treated in this commit. I see
that you've added some in patch 4 - could you move the ones related to this
change into this commit?

Another way you could do this is to put your "add tests" commit first in
this series, changing the condition on the ones that are fixed later in the
series to "test_expect_failure". Then, in each commit that "fixes" a test's
behavior, change that test to "test_expect_success". This approach had the
added benefit of showing that, before this series, the tests would fail and
that this series explicitly fixes those scenarios.