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

Re: [PATCH v2 6/9] mv: from in-cone to out-of-cone

From
Victoria Dye <vdye@github.com>
Date
Aug 9, 2022, 00:53 UTC
Message-ID
<9718ab8a-5a1a-d93c-ae8f-aa06f6822577@github.com>
In-Reply-To
<20220805030528.1535376-7-shaoxuan.yuan02@gmail.com>
Shaoxuan Yuan wrote:
Show 13 quoted lines
> Originally, moving an in-cone <source> to an out-of-cone <destination>
> was not possible, mainly because such <destination> is a directory that
> is not present in the working tree.
> 
> Change the behavior so that we can move an in-cone <source> to
> out-of-cone <destination> when --sparse is supplied.
> 
> Such <source> can be either clean or dirty, and moving it results in
> different behaviors:
> 
> A clean move should move the <source> to the <destination>, both in the
> working tree and the index, then remove the resulted path from the
> working tree, and turn on its CE_SKIP_WORKTREE bit.

It looks like this description is the same as what you wrote in patch 1 [1]. That's fine with me but, as noted in [2], I wanted to double-check whether the "move <src> to <dst> in worktree, then remove <dst> from worktree" is an accurate description of what's happening.

[1] https://lore.kernel.org/git/20220805030528.1535376-2-shaoxuan.yuan02@gmail.com/ [2] https://lore.kernel.org/git/bd80881d-b2a3-c220-8f2d-a07a46e14207@github.com/

Show 85 quoted lines
> 
> A dirty move should move the <source> to the <destination>, both in the
> working tree and the index, but should *not* remove the resulted path
> from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit.> 
> 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                  | 55 +++++++++++++++++++++++++++++------
>  t/t7002-mv-sparse-checkout.sh |  8 ++---
>  2 files changed, 50 insertions(+), 13 deletions(-)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 1dc55153ed..e4a14aea2d 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -171,12 +171,13 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  	};
>  	const char **source, **destination, **dest_path, **submodule_gitfile;
>  	const char *dst_w_slash;
> -	enum update_mode *modes;
> +	enum update_mode *modes, dst_mode = 0;
>  	struct stat st;
>  	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
>  	struct lock_file lock_file = LOCK_INIT;
>  	struct cache_entry *ce;
>  	struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
> +	struct string_list dirty_paths = STRING_LIST_INIT_NODUP;
>  
>  	git_config(git_default_config, NULL);
>  
> @@ -214,6 +215,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		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);
> +			dst_mode = SKIP_WORKTREE_DIR;
>  		} else if (argc != 1) {
>  			die(_("destination '%s' is not a directory"), dest_path[0]);
>  		} else {
> @@ -408,6 +410,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		const char *src = source[i], *dst = destination[i];
>  		enum update_mode mode = modes[i];
>  		int pos;
> +		int sparse_and_dirty = 0;
>  		struct checkout state = CHECKOUT_INIT;
>  		state.istate = &the_index;
>  
> @@ -418,6 +421,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		if (show_only)
>  			continue;
>  		if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
> +		    !(dst_mode & SKIP_WORKTREE_DIR) &&
>  		    rename(src, dst) < 0) {
>  			if (ignore_errors)
>  				continue;
> @@ -437,17 +441,49 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  
>  		pos = cache_name_pos(src, strlen(src));
>  		assert(pos >= 0);
> +		if (!(mode & SPARSE) && !lstat(src, &st))
> +			sparse_and_dirty = ce_modified(active_cache[pos], &st, 0);
>  		rename_cache_entry_at(pos, dst);
>  
> -		if ((mode & SPARSE) &&
> -		    (path_in_sparse_checkout(dst, &the_index))) {
> -			int dst_pos;
> +		if (ignore_sparse &&
> +		    core_apply_sparse_checkout &&
> +		    core_sparse_checkout_cone) {
> +			if ((mode & SPARSE) &&
> +			    path_in_sparse_checkout(dst, &the_index)) {
> +				/* from out-of-cone to in-cone */
> +				int dst_pos = cache_name_pos(dst, strlen(dst));
> +				struct cache_entry *dst_ce = active_cache[dst_pos];
> +
> +				dst_ce->ce_flags &= ~CE_SKIP_WORKTREE;
> +
> +				if (checkout_entry(dst_ce, &state, NULL, NULL))
> +					die(_("cannot checkout %s"), dst_ce->name);
> +			} else if ((dst_mode & SKIP_WORKTREE_DIR) &&
> +				   !(mode & SPARSE) &&
> +				   !path_in_sparse_checkout(dst, &the_index)) {
> +				/* from in-cone to out-of-cone */
> +				int dst_pos = cache_name_pos(dst, strlen(dst));
> +				struct cache_entry *dst_ce = active_cache[dst_pos];

It looks like the above conditions assume "out-of-cone <src>" and "out-of-cone <dst>" are mutually exclusive. Have you checked what happens when you try a move like that?

If the behavior is sensible, it would be nice to add a test (in 't7002'?) establishing that. Otherwise, changing that behavior is reasonably outside the scope of this series, so it's fine with me if you add a either "test_expect_failure" test, an "unsupported" warning message earlier in 'mv', or even just a "NEEDSWORK" comment somewhere around this code.

Show 25 quoted lines
>  
> -			dst_pos = cache_name_pos(dst, strlen(dst));
> -			active_cache[dst_pos]->ce_flags &= ~CE_SKIP_WORKTREE;
> -
> -			if (checkout_entry(active_cache[dst_pos], &state, NULL, NULL))
> -				die(_("cannot checkout %s"), active_cache[dst_pos]->name);
> +				/*
> +				 * if src is clean, it will suffice to remove it
> +				 */
> +				if (!sparse_and_dirty) {
> +					dst_ce->ce_flags |= CE_SKIP_WORKTREE;
> +					unlink_or_warn(src);
> +				} else {
> +					/*
> +					 * if src is dirty, move it to the
> +					 * destination and create leading
> +					 * dirs if necessary
> +					 */
> +					char *dst_dup = xstrdup(dst);
> +					string_list_append(&dirty_paths, dst);
> +					safe_create_leading_directories(dst_dup);
> +					FREE_AND_NULL(dst_dup);
> +					rename(src, dst);
> +				}
> +			}

The rest of this is clearly organized and (as far as I can tell) matches the behavior you established with the tests in patch 1. Nice work!

Show 51 quoted lines
>  		}
>  	}
>  
> @@ -459,6 +495,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		die(_("Unable to write new index file"));
>  
>  	string_list_clear(&src_for_dst, 0);
> +	string_list_clear(&dirty_paths, 0);
>  	UNLEAK(source);
>  	UNLEAK(dest_path);
>  	free(submodule_gitfile);
> diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
> index 9b3a9ab4c3..fc9577b2a6 100755
> --- a/t/t7002-mv-sparse-checkout.sh
> +++ b/t/t7002-mv-sparse-checkout.sh
> @@ -290,7 +290,7 @@ test_expect_success 'move sparse file to existing destination with --force and -
>  	test_cmp expect sub/file1
>  '
>  
> -test_expect_failure 'move clean path from in-cone to out-of-cone' '
> +test_expect_success 'move clean path from in-cone to out-of-cone' '
>  	test_when_finished "cleanup_sparse_checkout" &&
>  	setup_sparse_checkout &&
>  
> @@ -343,7 +343,7 @@ test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_failure 'move dirty path from in-cone to out-of-cone' '
> +test_expect_success 'move dirty path from in-cone to out-of-cone' '
>  	test_when_finished "cleanup_sparse_checkout" &&
>  	setup_sparse_checkout &&
>  	echo "modified" >>sub/d &&
> @@ -363,7 +363,7 @@ test_expect_failure 'move dirty path from in-cone to out-of-cone' '
>  	grep "H folder1/d" actual
>  '
>  
> -test_expect_failure 'move dir from in-cone to out-of-cone' '
> +test_expect_success 'move dir from in-cone to out-of-cone' '
>  	test_when_finished "cleanup_sparse_checkout" &&
>  	setup_sparse_checkout &&
>  
> @@ -382,7 +382,7 @@ test_expect_failure 'move dir from in-cone to out-of-cone' '
>  	grep "S folder1/dir/e" actual
>  '
>  
> -test_expect_failure 'move partially-dirty dir from in-cone to out-of-cone' '
> +test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' '
>  	test_when_finished "cleanup_sparse_checkout" &&
>  	setup_sparse_checkout &&
>  	touch sub/dir/e2 sub/dir/e3 &&
Previous: Shaoxuan YuanNext: Shaoxuan Yuan
Message 45 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.