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

From: Derrick Stolee <derrickstolee@github.com>
Date: Jul 19, 2022, 17:59 UTC
Message-ID: <95263c88-6894-87bb-4d52-84d8d1cc5671@github.com>
In-Reply-To: <20220719132809.409247-5-shaoxuan.yuan02@gmail.com>

On 7/19/2022 9:28 AM, Shaoxuan Yuan wrote:

Show 11 quoted lines

> 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.)

> 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.

Show 5 quoted lines

>  	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

Show 6 quoted lines

>  	} 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>

> +	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.

Thanks, -Stolee