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
