Re: [PATCH v1 4/7] mv: check if <destination> is a SKIP_WORKTREE_DIR

[Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>]

On 7/20/2022 1:59 AM, Derrick Stolee wrote:
 > On 7/19/2022 9:28 AM, Shaoxuan Yuan wrote:
 >> Originally, <destination> is assumed to be in the working tree. If it is
 >> not found as a directory, then it is determined to be either a 
regular file
 >> path, or error out if used under the second form (move into a directory)
 >> of 'git-mv'. Such behavior is not ideal, mainly because Git does not
 >> look into the index for <destination>, which could potentially be a
 >> SKIP_WORKTREE_DIR, which we need to determine for the later "moving from
 >> in-cone to out-of-cone" patch.
 >>
 >> Change the logic so that Git first check if <destination> is a directory
 >> with all its contents sparsified (a SKIP_WORKTREE_DIR). If yes,
 >> then treat <destination> as a directory exists in the working tree, and
 >
 > "treat <destination> as a directory exists in the working tree" is a bit
 > akward, and the rest of the sentence struggles with that. Starting with
 > "If yes," we could rewrite as follows:
 >
 >   If <destination> is such a sparse directory, then we should modify the
 >   index the same way as we would if this were a non-sparse directory. We
 >   must be careful to ensure that the <destination> is marked with
 >   SKIP_WORKTREE_DIR.
 >
 > (Note that I don't equate this as doing the same thing, just operating on
 > the index.)

This wording sounds more natural, thanks for the rewrite!

 >> If no, continue the original checking logic.
 >
 > I think this part doesn't need to be there, but I don't feel strongly
 > about it.
 >
 >> Also add a `dst_w_slash` to reuse the result from `add_slash()`, which
 >> was everywhere and can be simplified.
 >
 >>      else if (!lstat(dest_path[0], &st) &&
 >>              S_ISDIR(st.st_mode)) {
 >> -        dest_path[0] = add_slash(dest_path[0]);
 >> -        destination = internal_prefix_pathspec(dest_path[0], argv, 
argc, DUP_BASENAME);
 >> +        destination = internal_prefix_pathspec(dst_w_slash, argv, 
argc, DUP_BASENAME);
 >
 > Hm. I find it interesting how this works even if the directory is _not_
 > present in the index. Is there a test that checks this kind of setup?
 >
 >     git init &&
 >     >file &&
 >     git add file &&
 >     git commit -m init &&
 >     mkdir dir &&
 >     git mv file dir/
 >
 > Locally, this indeed works with the following 'git status' detail:
 >
 >         renamed:    file -> dir/file

In my impression, 'git-mv' does not seem to care about whether the
<destination> directory is in index? Given that `rename()` works so
long as the directory is in the working tree, and `rename_index_entry_at()`
cares even less about the <destination> dir?

 >>      } else {
 >> -        if (argc != 1)
 >> +        if (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
 >> +            !check_dir_in_index(dst_w_slash)) {
 >> +            destination = internal_prefix_pathspec(dst_w_slash, 
argv, argc, DUP_BASENAME);
 >> +            dst_mode |= SKIP_WORKTREE_DIR;
 >
 > Looks like we are assigning dst_mode here, but not using it. I wonder if
 > you'll get a compiler error if you build this patch with DEVELOPER=1.
 >
 > You can test all of the commits in your series using this command:
 >
 >   git rebase -x "make -j12 DEVELOPER=1" <base>

Oops, it seems that I was doing the wrong way. I only run test on the
tip of the branch, without actually testing individual commits.
Will do this.

 >> +    if (dst_w_slash != dest_path[0])
 >> +        free((char *)dst_w_slash);
 >
 > Good that you are freeing this here. You might also want to set it to 
NULL
 > just in case.

I was using the `FREE_AND_NULL()` macro, but I wasn't sure since other
places in 'git-mv' only use `free()`. Though I think it is better to
`FREE_AND_NULL()`.

--
Thanks,
Shaoxuan