Re: [WIP v2 4/5] mv: add check_dir_in_index() and solve general dir check issue

[Derrick Stolee <derrickstolee@github.com>]

On 5/27/2022 6:08 AM, Shaoxuan Yuan wrote:
> +/*
> + * Check if an out-of-cone directory should be in the index. Imagine this case
> + * that all the files under a directory are marked with 'CE_SKIP_WORKTREE' bit
> + * and thus the directory is sparsified.
> + *
> + * Return 0 if such directory exist (i.e. with any of its contained files not
> + * marked with CE_SKIP_WORKTREE, the directory would be present in working tree).
> + * Return 1 otherwise.
> + */
> +static int check_dir_in_index(const char *name, int namelen)
> +{
> +	int ret = 1;
> +	const char *with_slash = add_slash(name);
> +	int length = namelen + 1;
> +
> +	int pos = cache_name_pos(with_slash, length);
> +	const struct cache_entry *ce;
> +
> +	if (pos < 0) {
> +		pos = -pos - 1;
> +		if (pos >= the_index.cache_nr)
> +			return ret;
> +		ce = active_cache[pos];
> +		if (strncmp(with_slash, ce->name, length))
> +			return ret;
> +		if (ce_skip_worktree(ce))
> +			return ret = 0;

This appears to check if the _first_ entry under the directory
is sparse, but not if _all_ entries are sparse. These are not
the same thing, even in cone-mode sparse-checkout. The t1092
test directory has files like "folder1/0/0/a" but if
"folder1/1" is in the sparse-checkout cone, then that first
entry has the skip-worktree bit, but "folder1/1/a" and "folder1/a"
do not.

> +	}
> +	return ret;

At the moment, it doesn't seem like we need 'ret' since the
only place you set it is in "return ret = 0;" (which could
just be "return 0;" while the others are "return 1;"). But,
perhaps you intended to create a loop over 'pos' while
with_slash is a prefix of the cache entry?

> +			else if (!check_dir_in_index(src, length) &&
> +					 !path_in_sparse_checkout(src_w_slash, &the_index)) {

style-nit: You'll want to align the different parts of your
logical statement to agree with the end of the "else if (",

	else if (A &&
		 B) {


> +				modes[i] = SKIP_WORKTREE_DIR;

If we are moving to a flags-based model, should we convert all
"modes[i] =" to "modes[i] |=" as a first step (before adding the
SKIP_WORTKREE_DIR flag)?

> +				goto dir_check;

Hm. While I did recommend using 'goto' to jump to a common end
place in the loop body, I'm not sure about jumping into another
else-if statement. This might be a good time to extract the
code from "else if (src_is_dir)" below into a helper method that
can be used in both places.

> +			}
>  			/* only error if existence is expected. */
>  			else if (modes[i] != SPARSE)
>  				bad = _("bad source");
> @@ -218,7 +264,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  				&& lstat(dst, &st) == 0)
>  			bad = _("cannot move directory over file");
>  		else if (src_is_dir) {
> -			int first = cache_name_pos(src, length), last;
> +			int first, last;
> +dir_check:
> +			first = cache_name_pos(src, length);
>  
>  			if (first >= 0)
>  				prepare_move_submodule(src, first,
> @@ -229,7 +277,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			else { /* last - first >= 1 */
>  				int j, dst_len, n;
>  
> -				modes[i] = WORKING_DIRECTORY;
> +				if (!modes[i])
> +					modes[i] |= WORKING_DIRECTORY;

This appears to only add the WORKING_DIRECTORY flag if modes[i] is
already zero. This maybe implies that we wouldn't understand
"WORKING_DIRECTORY | SKIP_WORKTREE_DIR" as a value.

>  				n = argc + last - first;
>  				REALLOC_ARRAY(source, n);
>  				REALLOC_ARRAY(destination, n);
> @@ -331,7 +380,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			printf(_("Renaming %s to %s\n"), src, dst);
>  		if (show_only)
>  			continue;
> -		if (mode != INDEX && mode != SPARSE && rename(src, dst) < 0) {
> +		if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
> +		 	rename(src, dst) < 0) {

style-nit: align your logical statements.

>  			if (ignore_errors)
>  				continue;
>  			die_errno(_("renaming '%s' failed"), src);
> @@ -345,7 +395,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  							      1);
>  		}
>  
> -		if (mode == WORKING_DIRECTORY)
> +		if (mode & (WORKING_DIRECTORY | SKIP_WORKTREE_DIR))
>  			continue;

Ok, here you check if _either_ mode is enabled, which is good. Maybe
you don't need the "if (!mode[i])" part above.

Thanks,
-Stolee