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
