From: Shaoxuan Yuan Date: Thu, 21 Jul 2022 14:13:44 GMT Subject: Re: [PATCH v1 4/7] mv: check if is a SKIP_WORKTREE_DIR Message-ID: In-Reply-To: <95263c88-6894-87bb-4d52-84d8d1cc5671@github.com> On 7/20/2022 1:59 AM, Derrick Stolee wrote: > On 7/19/2022 9:28 AM, Shaoxuan Yuan wrote: >> Originally, 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 , 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 is a directory >> with all its contents sparsified (a SKIP_WORKTREE_DIR). If yes, >> then treat as a directory exists in the working tree, and > > "treat 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 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 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 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 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" 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