From: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
Date: Tue, 09 Aug 2022 02:31:29 GMT
Subject: Re: [PATCH v2 4/9] mv: check if <destination> is a SKIP_WORKTREE_DIR
Message-ID: <542b40f5-0c7f-5f3d-eb81-6b8e1ab8924d@gmail.com>
In-Reply-To: <698b86d0-c906-9a9a-839e-71806ae41f2c@github.com>



On 8/9/2022 7:41 AM, Victoria Dye wrote:
...truncated...
> It took me some time to understand what all of these (nested) conditions are
> doing; one suggestion I have (feel free to ignore it, since it's really just
> a matter of stylistic preference) is reduce some duplicate code/simplify the
> change a bit by moving the sparse directory check into the main "if-else"
> block:
Yes, I acknowledge this part is cluttered slightly ;)
> ------------->8------------->8------------->8------------->8------------->8-------------
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 4729bb1a1a..1c1b9559f6 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -203,10 +203,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>   	if (dest_path[0][0] == '\0')
>   		/* special case: "." was normalized to "" */
>   		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
> -	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);
> +	else if ((!lstat(dest_path[0], &st) && S_ISDIR(st.st_mode)) ||
> +		 (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
> +		  empty_dir_has_sparse_contents(dst_w_slash))) {
> +		/* directory dest_path[0] exists on-disk or in the index */
> +		destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
>   	} else {
>   		if (argc != 1)
>   			die(_("destination '%s' is not a directory"), dest_path[0]);
>
> -------------8<-------------8<-------------8<-------------8<-------------8<-------------
>
> It doesn't make for the prettiest condition (so your current approach might
> be better in terms of readability) but, to me, it creates a clearer
> distinction between the "if" and "else if" blocks (which handle the case
> where 'dest_path[0]' is a directory), and the "else" block (which handles
> the case where 'dest_path[0]' is a file).

I also find this way clearer! Thanks for the suggestion!