git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Victoria Dye <vdye@github.com>
Date
Aug 8, 2022, 23:41 UTC
Message-ID
<698b86d0-c906-9a9a-839e-71806ae41f2c@github.com>
In-Reply-To
<20220805030528.1535376-5-shaoxuan.yuan02@gmail.com>
Shaoxuan Yuan wrote:
Show 18 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 <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.
> 
> Also add a `dst_w_slash` to reuse the result from `add_slash()`, which
> was everywhere and can be simplified.
This all makes sense. Stepping through the code...
Show 25 quoted lines
> 
> Helped-by: Derrick Stolee <derrickstolee@github.com>
> Helped-by: Victoria Dye <vdye@github.com>
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>  builtin/mv.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 0a999640c9..f213a92bf6 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -171,6 +171,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		OPT_END(),
>  	};
>  	const char **source, **destination, **dest_path, **submodule_gitfile;
> +	const char *dst_w_slash;
>  	enum update_mode *modes;
>  	struct stat st;
>  	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
> @@ -201,6 +202,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  	if (argc == 1 && is_directory(argv[0]) && !is_directory(argv[1]))
>  		flags = 0;
>  	dest_path = internal_prefix_pathspec(prefix, argv + argc, 1, flags);
> +	dst_w_slash = add_slash(dest_path[0]);
...you pre-compute a reusable 'dst_w_slash' here...
Show 10 quoted lines
>  	submodule_gitfile = xcalloc(argc, sizeof(char *));
>  
>  	if (dest_path[0][0] == '\0')
> @@ -208,12 +210,20 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		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);
> +		destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);

...then remove the in-place 'add_slash()' of 'dest_path[0]' and use 'dst_w_slash' in 'internal_prefix_pathspec()'. Makes sense.

>  	} else {

Then, this block is reached if 'dest_path' is not '.' and it is not a directory that exists on disk.

Previously, reaching this point meant that 'dest_path' *must* refer to a file, not a directory. However, you want to add handling for the case where 'dst_w_slash' doesn't exist on disk because all of its contents are sparse:

> -		if (argc != 1)
> +		if (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
> +		    empty_dir_has_sparse_contents(dst_w_slash)) {
> +			destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);

so the above condition identifies whether 'dest_path[0]' is non-empty in the index, and sets 'destination' accordingly.

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:

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

> +		} else if (argc != 1) {
>  			die(_("destination '%s' is not a directory"), dest_path[0]);
> -		destination = dest_path;
> +		} else {
> +			destination = dest_path;
> +		}
> +	}
> +	if (dst_w_slash != dest_path[0]) {
> +		free((char *)dst_w_slash);
> +		dst_w_slash = NULL;

Looks good.

>  	}
>  
>  	/* Checking */
Previous: Shaoxuan YuanNext: Victoria Dye
Message 40 of 61 in “mv: from in-cone to out-of-cone”
  1. 0/7 mv: from in-cone to out-of-coneShaoxuan Yuan, Jul 19, 2022
  2. 1/7 t7002: add tests for moving from in-cone to out-of-coneShaoxuan Yuan, Jul 19, 2022
  3. Ævar Arnfjörð BjarmasonJul 19, 2022
  4. Derrick StoleeJul 19, 2022
  5. Junio C HamanoJul 19, 2022
  6. 2/7 mv: add documentation for check_dir_in_index()Shaoxuan Yuan, Jul 19, 2022
  7. Derrick StoleeJul 19, 2022
  8. Shaoxuan YuanJul 21, 2022
  9. Victoria DyeJul 19, 2022
  10. Victoria DyeJul 19, 2022
  11. Shaoxuan YuanJul 21, 2022
  12. 3/7 mv: free the *with_slash in check_dir_in_index()Shaoxuan Yuan, Jul 19, 2022
  13. Derrick StoleeJul 19, 2022
  14. 5/7 mv: remove BOTH from enum update_modeShaoxuan Yuan, Jul 19, 2022
  15. Derrick StoleeJul 19, 2022
  16. 4/7 mv: check if <destination> is a SKIP_WORKTREE_DIRShaoxuan Yuan, Jul 19, 2022
  17. Derrick StoleeJul 19, 2022
  18. Shaoxuan YuanJul 21, 2022
  19. Derrick StoleeJul 22, 2022
  20. Junio C HamanoJul 22, 2022
  21. 6/7 mv: from in-cone to out-of-coneShaoxuan Yuan, Jul 19, 2022
  22. Derrick StoleeJul 19, 2022
  23. Shaoxuan YuanAug 3, 2022
  24. Derrick StoleeAug 3, 2022
  25. Shaoxuan YuanAug 4, 2022
  26. 7/7 mv: check overwrite for in-to-out moveShaoxuan Yuan, Jul 19, 2022
  27. Derrick StoleeJul 19, 2022
  28. Derrick StoleeJul 19, 2022
  29. 0/9 mv: from in-cone to out-of-coneShaoxuan Yuan, Aug 5, 2022
  30. 1/9 t7002: add tests for moving from in-cone to out-of-coneShaoxuan Yuan, Aug 5, 2022
  31. Victoria DyeAug 9, 2022
  32. Shaoxuan YuanAug 9, 2022
  33. Shaoxuan YuanAug 9, 2022
  34. Shaoxuan YuanAug 9, 2022
  35. 2/9 mv: rename check_dir_in_index() to empty_dir_has_sparse_contents()Shaoxuan Yuan, Aug 5, 2022
  36. 3/9 mv: free the *with_slash in check_dir_in_index()Shaoxuan Yuan, Aug 5, 2022
  37. Victoria DyeAug 8, 2022
  38. Shaoxuan YuanAug 9, 2022
  39. 4/9 mv: check if <destination> is a SKIP_WORKTREE_DIRShaoxuan Yuan, Aug 5, 2022
  40. Victoria DyeAug 8, 2022
  41. Victoria DyeAug 9, 2022
  42. Shaoxuan YuanAug 9, 2022
  43. 5/9 mv: remove BOTH from enum update_modeShaoxuan Yuan, Aug 5, 2022
  44. 6/9 mv: from in-cone to out-of-coneShaoxuan Yuan, Aug 5, 2022
  45. Victoria DyeAug 9, 2022
  46. Shaoxuan YuanAug 9, 2022
  47. 7/9 mv: cleanup empty WORKING_DIRECTORYShaoxuan Yuan, Aug 5, 2022
  48. 8/9 advice.h: add advise_on_moving_dirty_path()Shaoxuan Yuan, Aug 5, 2022
  49. 9/9 mv: check overwrite for in-to-out moveShaoxuan Yuan, Aug 5, 2022
  50. Victoria DyeAug 8, 2022
  51. 0/9 mv: from in-cone to out-of-coneShaoxuan Yuan, Aug 9, 2022
  52. 1/9 t7002: add tests for moving from in-cone to out-of-coneShaoxuan Yuan, Aug 9, 2022
  53. 2/9 mv: rename check_dir_in_index() to empty_dir_has_sparse_contents()Shaoxuan Yuan, Aug 9, 2022
  54. 3/9 mv: free the with_slash in check_dir_in_index()Shaoxuan Yuan, Aug 9, 2022
  55. 5/9 mv: remove BOTH from enum update_modeShaoxuan Yuan, Aug 9, 2022
  56. 6/9 mv: from in-cone to out-of-coneShaoxuan Yuan, Aug 9, 2022
  57. 7/9 mv: cleanup empty WORKING_DIRECTORYShaoxuan Yuan, Aug 9, 2022
  58. 8/9 advice.h: add advise_on_moving_dirty_path()Shaoxuan Yuan, Aug 9, 2022
  59. 9/9 mv: check overwrite for in-to-out moveShaoxuan Yuan, Aug 9, 2022
  60. 4/9 mv: check if <destination> is a SKIP_WORKTREE_DIRShaoxuan Yuan, Aug 9, 2022
  61. Victoria DyeAug 16, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.