git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Victoria Dye <vdye@github.com>
To: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
Cc: derrickstolee@github.com, git@vger.kernel.org, gitster@pobox.com,
	newren@gmail.com
Subject: Re: [WIP v3 0/7] mv: fix out-of-cone file/directory move logic
Date: Tue, 21 Jun 2022 16:30:38 -0700	[thread overview]
Message-ID: <d3aa35e4-5bad-2bbb-2a25-d82064d6ec81@github.com> (raw)
In-Reply-To: <20220619032549.156335-1-shaoxuan.yuan02@gmail.com>

Shaoxuan Yuan wrote:
> The range-diff seems a bit messy, because some of the changes are too big
> then I have to tune the --create-factor big enough to reveal these changes.
> 
> ## Changes since WIP v2 ##
> 
> 1. Write helper functions for t7002 to reuse some code.
> 
> 2. Refactor/decouple the if/else-if checking chain.
> 
> 3. Separate out the 'update_mode' refactor into a single commit.
> 
> 4. Stop using update_sparsity() and instead update the SKIP_WORKTREE
>    bit for each cache_entry and check it out to the working tree.

All of this looks great! My comments are all minor syntax nits - the
functionality is sound. Thanks again for working through this tangled mess
of a problem!

> 
> ## Limitations ##
> 
> At this point, we still don't have in-cone to out-of-cone move, which
> I don't think is too much a problem, since the title says this series is
> around out-of-cone as the <source>.
> 

While it would be nice to include in-cone -> out-of-cone here, I'm also
content with you moving it to a later series (the first couple patches in a
sparse index integration or as a standalone series; up to you). 

> But I think it worth discuss if we should implement in-cone to 
> out-of-cone move, since it will be nice (naturally) to have it working.
> 
> However, I noticed this from the mv man page:
> 
> "In the second form, the last argument has to be an existing directory; 
> the given sources will be moved into this directory."
> 
> I think trying to move out-of-cone, the last argument has to be an non-existent
> directory? I'm a bit confused: should we update some of mv basic logic to 
> accomplish this?
> 

I suspect this requirement is related to the POSIX 'mv' [1] (and
corresponding 'rename()', used in 'git mv'), which also requires that the
destination directory exists. I personally don't think this requirement
needs to apply to 'git mv' at all, but note that changing the behavior would
require first creating the necessary directories before calling 'rename()'. 

As a more conservative solution, you could do the parent directory creation
*only* in the case of moving to a sparse contents-only directory (using
something like the 'check_dir_in_index()' function you introduced to
identify).

I'm also interested in hearing what others have to say, especially regarding
historical context/use cases of 'git mv'.

[1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/mv.html

> Shaoxuan Yuan (7):
>   t7002: add tests for moving out-of-cone file/directory
>   mv: decouple if/else-if checks using goto
>   mv: check if out-of-cone file exists in index with SKIP_WORKTREE bit
>   mv: check if <destination> exists in index to handle overwriting
>   mv: use flags mode for update_mode
>   mv: add check_dir_in_index() and solve general dir check issue
>   mv: update sparsity after moving from out-of-cone to in-cone
> 
>  builtin/mv.c                  | 244 +++++++++++++++++++++++++---------
>  t/t7002-mv-sparse-checkout.sh |  85 ++++++++++++
>  2 files changed, 264 insertions(+), 65 deletions(-)
> 
> Range-diff against v2:
> 1:  271445205d ! 1:  a08ce96935 t7002: add tests for moving out-of-cone file/directory
>     @@ Commit message
>      
>          Add corresponding tests to test following situations:
>      
>     -    * 'refuse to move out-of-cone directory without --sparse'
>     -    * 'can move out-of-cone directory with --sparse'
>     -    * 'refuse to move out-of-cone file without --sparse'
>     -    * 'can move out-of-cone file with --sparse'
>     -    * 'refuse to move sparse file to existing destination'
>     -    * 'move sparse file to existing destination with --force and --sparse'
>     +    We do not have sufficient coverage of moving files outside
>     +    of a sparse-checkout cone. Create new tests covering this
>     +    behavior, keeping in mind that the user can include --sparse
>     +    (or not), move a file or directory, and the destination can
>     +    already exist in the index (in this case user can use --force
>     +    to overwrite existing entry).
>      
>     +    Helped-by: Victoria Dye <vdye@github.com>
>     +    Helped-by: Derrick Stolee <derrickstolee@github.com>
>          Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
>      
>       ## t/t7002-mv-sparse-checkout.sh ##
>     +@@ t/t7002-mv-sparse-checkout.sh: test_description='git mv in sparse working trees'
>     + 
>     + . ./test-lib.sh
>     + 
>     ++setup_sparse_checkout () {
>     ++	mkdir folder1 &&
>     ++	touch folder1/file1 &&
>     ++	git add folder1 &&
>     ++	git sparse-checkout set --cone sub
>     ++}
>     ++
>     ++cleanup_sparse_checkout () {
>     ++	git sparse-checkout disable &&
>     ++	git reset --hard
>     ++}
>     ++
>     + test_expect_success 'setup' "
>     + 	mkdir -p sub/dir sub/dir2 &&
>     + 	touch a b c sub/d sub/dir/e sub/dir2/e &&
>     +@@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'can move files to non-sparse dir' '
>     + '
>     + 
>     + test_expect_success 'refuse to move file to non-skip-worktree sparse path' '
>     ++	test_when_finished "cleanup_sparse_checkout" &&
>     + 	git reset --hard &&
>     + 	git sparse-checkout init --no-cone &&
>     + 	git sparse-checkout set a !/x y/ !x/y/z &&
>      @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'refuse to move file to non-skip-worktree sparse path' '
>       	test_cmp expect stderr
>       '
>       
>      +test_expect_failure 'refuse to move out-of-cone directory without --sparse' '
>     -+	git sparse-checkout disable &&
>     -+	git reset --hard &&
>     -+	mkdir folder1 &&
>     -+	touch folder1/file1 &&
>     -+	git add folder1 &&
>     -+	git sparse-checkout init --cone &&
>     -+	git sparse-checkout set sub &&
>     ++	test_when_finished "cleanup_sparse_checkout" &&
>     ++	setup_sparse_checkout &&
>      +
>      +	test_must_fail git mv folder1 sub 2>stderr &&
>      +	cat sparse_error_header >expect &&
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'refuse to move file to non-s
>      +'
>      +
>      +test_expect_failure 'can move out-of-cone directory with --sparse' '
>     -+	git sparse-checkout disable &&
>     -+	git reset --hard &&
>     -+	mkdir folder1 &&
>     -+	touch folder1/file1 &&
>     -+	git add folder1 &&
>     -+	git sparse-checkout init --cone &&
>     -+	git sparse-checkout set sub &&
>     ++	test_when_finished "cleanup_sparse_checkout" &&
>     ++	setup_sparse_checkout &&
>      +
>      +	git mv --sparse folder1 sub 1>actual 2>stderr &&
>      +	test_must_be_empty stderr &&
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'refuse to move file to non-s
>      +'
>      +
>      +test_expect_failure 'refuse to move out-of-cone file without --sparse' '
>     -+	git sparse-checkout disable &&
>     -+	git reset --hard &&
>     -+	mkdir folder1 &&
>     -+	touch folder1/file1 &&
>     -+	git add folder1 &&
>     -+	git sparse-checkout init --cone &&
>     -+	git sparse-checkout set sub &&
>     ++	test_when_finished "cleanup_sparse_checkout" &&
>     ++	setup_sparse_checkout &&
>      +
>      +	test_must_fail git mv folder1/file1 sub 2>stderr &&
>      +	cat sparse_error_header >expect &&
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'refuse to move file to non-s
>      +'
>      +
>      +test_expect_failure 'can move out-of-cone file with --sparse' '
>     -+	git sparse-checkout disable &&
>     -+	git reset --hard &&
>     -+	mkdir folder1 &&
>     -+	touch folder1/file1 &&
>     -+	git add folder1 &&
>     -+	git sparse-checkout init --cone &&
>     -+	git sparse-checkout set sub &&
>     ++	test_when_finished "cleanup_sparse_checkout" &&
>     ++	setup_sparse_checkout &&
>      +
>      +	git mv --sparse folder1/file1 sub 1>actual 2>stderr &&
>      +	test_must_be_empty stderr &&
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'refuse to move file to non-s
>      +'
>      +
>      +test_expect_failure 'refuse to move sparse file to existing destination' '
>     -+	git sparse-checkout disable &&
>     -+	git reset --hard &&
>     ++	test_when_finished "cleanup_sparse_checkout" &&
>      +	mkdir folder1 &&
>      +	touch folder1/file1 &&
>      +	touch sub/file1 &&
>      +	git add folder1 sub/file1 &&
>     -+	git sparse-checkout init --cone &&
>     -+	git sparse-checkout set sub &&
>     ++	git sparse-checkout set --cone sub &&
>      +
>      +	test_must_fail git mv --sparse folder1/file1 sub 2>stderr &&
>      +	echo "fatal: destination exists, source=folder1/file1, destination=sub/file1" >expect &&
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'refuse to move file to non-s
>      +'
>      +
>      +test_expect_failure 'move sparse file to existing destination with --force and --sparse' '
>     -+	git sparse-checkout disable &&
>     -+	git reset --hard &&
>     ++	test_when_finished "cleanup_sparse_checkout" &&
>      +	mkdir folder1 &&
>      +	touch folder1/file1 &&
>      +	touch sub/file1 &&
>      +	echo "overwrite" >folder1/file1 &&
>      +	git add folder1 sub/file1 &&
>     -+	git sparse-checkout init --cone &&
>     -+	git sparse-checkout set sub &&
>     ++	git sparse-checkout set --cone sub &&
>      +
>      +	git mv --sparse --force folder1/file1 sub 2>stderr &&
>      +	test_must_be_empty stderr &&
> -:  ---------- > 2:  8065fbc232 mv: decouple if/else-if checks using goto
> 2:  80f485f146 ! 3:  e227fe717b mv: check if out-of-cone file exists in index with SKIP_WORKTREE bit
>     @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>       
>       		length = strlen(src);
>       		if (lstat(src, &st) < 0) {
>     -+			/*
>     -+			 * TODO: for now, when you try to overwrite a <destination>
>     -+			 * with your <source> as a sparse file, if you supply a "--sparse"
>     -+			 * flag, then the action will be done without providing "--force"
>     -+			 * and no warning.
>     -+			 *
>     -+			 * This is mainly because the sparse <source>
>     -+			 * is not on-disk, and this if-else chain will be cut off early in
>     -+			 * this check, thus the "--force" check is ignored. Need fix.
>     -+			 */
>     +-			/* only error if existence is expected. */
>     +-			if (modes[i] != SPARSE) {
>     ++			int pos;
>     ++			const struct cache_entry *ce;
>      +
>     -+			int pos = cache_name_pos(src, length);
>     -+			if (pos >= 0) {
>     -+				const struct cache_entry *ce = active_cache[pos];
>     -+
>     -+				if (ce_skip_worktree(ce)) {
>     -+					if (!ignore_sparse)
>     -+						string_list_append(&only_match_skip_worktree, src);
>     -+					else
>     -+						modes[i] = SPARSE;
>     -+				}
>     -+				else
>     ++			pos = cache_name_pos(src, length);
>     ++			if (pos < 0) {
>     ++				/* only error if existence is expected. */
>     ++				if (modes[i] != SPARSE)
>      +					bad = _("bad source");
>     ++				goto act_on_entry;
>      +			}
>     - 			/* only error if existence is expected. */
>     --			if (modes[i] != SPARSE)
>     -+			else if (modes[i] != SPARSE)
>     ++
>     ++			ce = active_cache[pos];
>     ++			if (!ce_skip_worktree(ce)) {
>       				bad = _("bad source");
>     - 		} else if (!strncmp(src, dst, length) &&
>     - 				(dst[length] == 0 || dst[length] == '/')) {
>     + 				goto act_on_entry;
>     + 			}
>     ++
>     ++			if (!ignore_sparse)
>     ++				string_list_append(&only_match_skip_worktree, src);
>     ++			else
>     ++				modes[i] = SPARSE;
>     ++			goto act_on_entry;
>     + 		}
>     + 		if (!strncmp(src, dst, length) &&
>     + 		    (dst[length] == 0 || dst[length] == '/')) {
>      
>       ## t/t7002-mv-sparse-checkout.sh ##
>      @@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'can move out-of-cone directory with --sparse' '
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'can move out-of-cone directo
>       
>      -test_expect_failure 'refuse to move out-of-cone file without --sparse' '
>      +test_expect_success 'refuse to move out-of-cone file without --sparse' '
>     - 	git sparse-checkout disable &&
>     - 	git reset --hard &&
>     - 	mkdir folder1 &&
>     + 	test_when_finished "cleanup_sparse_checkout" &&
>     + 	setup_sparse_checkout &&
>     + 
>      @@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'refuse to move out-of-cone file without --sparse' '
>       	test_cmp expect stderr
>       '
>       
>      -test_expect_failure 'can move out-of-cone file with --sparse' '
>      +test_expect_success 'can move out-of-cone file with --sparse' '
>     - 	git sparse-checkout disable &&
>     - 	git reset --hard &&
>     - 	mkdir folder1 &&
>     + 	test_when_finished "cleanup_sparse_checkout" &&
>     + 	setup_sparse_checkout &&
>     + 
> 3:  04572e5e6b ! 4:  d0de7678e3 mv: check if <destination> exists in index to handle overwriting
>     @@ Commit message
>      
>       ## builtin/mv.c ##
>      @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - 
>     - 		length = strlen(src);
>     - 		if (lstat(src, &st) < 0) {
>     --			/*
>     --			 * TODO: for now, when you try to overwrite a <destination>
>     --			 * with your <source> as a sparse file, if you supply a "--sparse"
>     --			 * flag, then the action will be done without providing "--force"
>     --			 * and no warning.
>     --			 *
>     --			 * This is mainly because the sparse <source>
>     --			 * is not on-disk, and this if-else chain will be cut off early in
>     --			 * this check, thus the "--force" check is ignored. Need fix.
>     --			 */
>     - 
>     - 			int pos = cache_name_pos(src, length);
>     - 			if (pos >= 0) {
>     -@@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - 				if (ce_skip_worktree(ce)) {
>     - 					if (!ignore_sparse)
>     - 						string_list_append(&only_match_skip_worktree, src);
>     --					else
>     --						modes[i] = SPARSE;
>     -+					else {
>     -+						/* Check if dst exists in index */
>     -+						if (cache_name_pos(dst, strlen(dst)) >= 0) {
>     -+							if (force)
>     -+								modes[i] = SPARSE;
>     -+							else
>     -+								bad = _("destination exists");
>     -+						}
>     -+						else
>     -+							modes[i] = SPARSE;
>     -+					}
>     - 				}
>     - 				else
>     - 					bad = _("bad source");
>     + 				bad = _("bad source");
>     + 				goto act_on_entry;
>     + 			}
>     +-
>     +-			if (!ignore_sparse)
>     ++			if (!ignore_sparse) {
>     + 				string_list_append(&only_match_skip_worktree, src);
>     +-			else
>     ++				goto act_on_entry;
>     ++			}
>     ++			/* Check if dst exists in index */
>     ++			if (cache_name_pos(dst, strlen(dst)) < 0) {
>     + 				modes[i] = SPARSE;
>     ++				goto act_on_entry;
>     ++			}
>     ++			if (!force) {
>     ++				bad = _("destination exists");
>     ++				goto act_on_entry;
>     ++			}
>     ++			modes[i] = SPARSE;
>     + 			goto act_on_entry;
>     + 		}
>     + 		if (!strncmp(src, dst, length) &&
>      
>       ## t/t7002-mv-sparse-checkout.sh ##
>      @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'can move out-of-cone file with --sparse' '
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'can move out-of-cone file wi
>       
>      -test_expect_failure 'refuse to move sparse file to existing destination' '
>      +test_expect_success 'refuse to move sparse file to existing destination' '
>     - 	git sparse-checkout disable &&
>     - 	git reset --hard &&
>     + 	test_when_finished "cleanup_sparse_checkout" &&
>       	mkdir folder1 &&
>     + 	touch folder1/file1 &&
> -:  ---------- > 5:  70540957b6 mv: use flags mode for update_mode
> 4:  4eeae40186 ! 6:  f8302f64e0 mv: add check_dir_in_index() and solve general dir check issue
>     @@ Commit message
>          instead of "bad source"; also user now can supply a "--sparse" flag so
>          this operation can be carried out successfully.
>      
>     -    Also, as suggested by Derrick [1],
>     -    move the in-line definition of "enum update_mode" to the top
>     -    of the file and make it use "flags" mode (each state is a different
>     -    bit in the word).
>     -
>     -    [1] https://lore.kernel.org/git/22aadea2-9330-aa9e-7b6a-834585189144@github.com/
>     -
>          Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
>      
>       ## builtin/mv.c ##
>     -@@ builtin/mv.c: static const char * const builtin_mv_usage[] = {
>     - 	NULL
>     - };
>     - 
>     -+enum update_mode {
>     -+	BOTH = 0,
>     -+	WORKING_DIRECTORY = (1 << 1),
>     -+	INDEX = (1 << 2),
>     -+	SPARSE = (1 << 3),
>     -+	SKIP_WORKTREE_DIR = (1 << 4),
>     -+};
>     -+
>     - #define DUP_BASENAME 1
>     - #define KEEP_TRAILING_SLASH 2
>     - 
>      @@ builtin/mv.c: static int index_range_of_same_dir(const char *src, int length,
>       	return last - first;
>       }
>     @@ builtin/mv.c: static int index_range_of_same_dir(const char *src, int length,
>       {
>       	int i, flags, gitmodules_modified = 0;
>      @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - 		OPT_END(),
>     - 	};
>     - 	const char **source, **destination, **dest_path, **submodule_gitfile;
>     --	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX, SPARSE } *modes;
>     -+	enum update_mode *modes;
>     - 	struct stat st;
>     - 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
>     - 	struct lock_file lock_file = LOCK_INIT;
>     -@@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - 		if (lstat(src, &st) < 0) {
>     - 
>     - 			int pos = cache_name_pos(src, length);
>     -+			const char *src_w_slash = add_slash(src);
>     -+
>     - 			if (pos >= 0) {
>     - 				const struct cache_entry *ce = active_cache[pos];
>     + 	/* Checking */
>     + 	for (i = 0; i < argc; i++) {
>     + 		const char *src = source[i], *dst = destination[i];
>     +-		int length, src_is_dir;
>     ++		int length;
>     + 		const char *bad = NULL;
>     + 		int skip_sparse = 0;
>       
>      @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - 				else
>     + 
>     + 			pos = cache_name_pos(src, length);
>     + 			if (pos < 0) {
>     ++				const char *src_w_slash = add_slash(src);
>     ++				if (!check_dir_in_index(src, length) &&
>     ++					!path_in_sparse_checkout(src_w_slash, &the_index)) {
>     ++					modes[i] |= SKIP_WORKTREE_DIR;
>     ++					goto dir_check;
>     ++				}
>     + 				/* only error if existence is expected. */
>     + 				if (!(modes[i] & SPARSE))
>       					bad = _("bad source");
>     + 				goto act_on_entry;
>       			}
>     -+			else if (!check_dir_in_index(src, length) &&
>     -+					 !path_in_sparse_checkout(src_w_slash, &the_index)) {
>     -+				modes[i] = SKIP_WORKTREE_DIR;
>     -+				goto dir_check;
>     -+			}
>     - 			/* only error if existence is expected. */
>     - 			else if (modes[i] != SPARSE)
>     +-
>     + 			ce = active_cache[pos];
>     + 			if (!ce_skip_worktree(ce)) {
>       				bad = _("bad source");
>      @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - 				&& lstat(dst, &st) == 0)
>     + 			bad = _("can not move directory into itself");
>     + 			goto act_on_entry;
>     + 		}
>     +-		if ((src_is_dir = S_ISDIR(st.st_mode))
>     ++		if (S_ISDIR(st.st_mode)
>     + 		    && lstat(dst, &st) == 0) {
>       			bad = _("cannot move directory over file");
>     - 		else if (src_is_dir) {
>     + 			goto act_on_entry;
>     + 		}
>     +-		if (src_is_dir) {
>     ++
>     ++dir_check:
>     ++		if (S_ISDIR(st.st_mode)) {
>     + 			int j, dst_len, n;
>      -			int first = cache_name_pos(src, length), last;
>      +			int first, last;
>     -+dir_check:
>      +			first = cache_name_pos(src, length);
>       
>     - 			if (first >= 0)
>     + 			if (first >= 0) {
>       				prepare_move_submodule(src, first,
>     -@@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - 			else { /* last - first >= 1 */
>     - 				int j, dst_len, n;
>     - 
>     --				modes[i] = WORKING_DIRECTORY;
>     -+				if (!modes[i])
>     -+					modes[i] |= WORKING_DIRECTORY;
>     - 				n = argc + last - first;
>     - 				REALLOC_ARRAY(source, n);
>     - 				REALLOC_ARRAY(destination, n);
>     -@@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - 			printf(_("Renaming %s to %s\n"), src, dst);
>     - 		if (show_only)
>     - 			continue;
>     --		if (mode != INDEX && mode != SPARSE && rename(src, dst) < 0) {
>     -+		if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
>     -+		 	rename(src, dst) < 0) {
>     - 			if (ignore_errors)
>     - 				continue;
>     - 			die_errno(_("renaming '%s' failed"), src);
>     -@@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - 							      1);
>     - 		}
>     - 
>     --		if (mode == WORKING_DIRECTORY)
>     -+		if (mode & (WORKING_DIRECTORY | SKIP_WORKTREE_DIR))
>     - 			continue;
>     - 
>     - 		pos = cache_name_pos(src, strlen(src));
>      
>       ## t/t7002-mv-sparse-checkout.sh ##
>      @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'refuse to move file to non-skip-worktree sparse path' '
>     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'refuse to move file to non-s
>       
>      -test_expect_failure 'refuse to move out-of-cone directory without --sparse' '
>      +test_expect_success 'refuse to move out-of-cone directory without --sparse' '
>     - 	git sparse-checkout disable &&
>     - 	git reset --hard &&
>     - 	mkdir folder1 &&
>     + 	test_when_finished "cleanup_sparse_checkout" &&
>     + 	setup_sparse_checkout &&
>     + 
>      @@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'refuse to move out-of-cone directory without --sparse' '
>       	test_cmp expect stderr
>       '
>       
>      -test_expect_failure 'can move out-of-cone directory with --sparse' '
>      +test_expect_success 'can move out-of-cone directory with --sparse' '
>     - 	git sparse-checkout disable &&
>     - 	git reset --hard &&
>     - 	mkdir folder1 &&
>     + 	test_when_finished "cleanup_sparse_checkout" &&
>     + 	setup_sparse_checkout &&
>     + 
> 5:  a3a296c3ef ! 7:  bc996931e2 mv: use update_sparsity() after touching sparse contents
>     @@ Metadata
>      Author: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
>      
>       ## Commit message ##
>     -    mv: use update_sparsity() after touching sparse contents
>     +    mv: update sparsity after moving from out-of-cone to in-cone
>      
>     -    Originally, "git mv" a sparse file/directory from out/in-cone to
>     -    in/out-cone does not update the sparsity following the sparse-checkout
>     -    patterns.
>     +    Originally, "git mv" a sparse file from out-of-cone to
>     +    in-cone does not update the moved file's sparsity (remove its
>     +    SKIP_WORKTREE bit). And the corresponding cache entry is, unexpectedly,
>     +    not checked out in the working tree.
>      
>     -    Use update_sparsity() after touching sparse contents, so the sparsity
>     -    will be updated after the move.
>     +    Update the behavior so that:
>     +    1. Moving from out-of-cone to in-cone removes the SKIP_WORKTREE bit from
>     +       corresponding cache entry.
>     +    2. The moved cache entry is checked out in the working tree to reflect
>     +       the updated sparsity.
>      
>          Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
>      
>     @@ builtin/mv.c
>       #include "string-list.h"
>       #include "parse-options.h"
>       #include "submodule.h"
>     -+#include "unpack-trees.h"
>     ++#include "entry.h"
>       
>       static const char * const builtin_mv_usage[] = {
>       	N_("git mv [<options>] <source>... <destination>"),
>     -@@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     - {
>     - 	int i, flags, gitmodules_modified = 0;
>     - 	int verbose = 0, show_only = 0, force = 0, ignore_errors = 0, ignore_sparse = 0;
>     -+	int sparse_moved = 0;
>     - 	struct option builtin_mv_options[] = {
>     - 		OPT__VERBOSE(&verbose, N_("be verbose")),
>     - 		OPT__DRY_RUN(&show_only, N_("dry run")),
>      @@ builtin/mv.c: 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;
>     -+		if (!sparse_moved && mode & (SPARSE | SKIP_WORKTREE_DIR))
>     -+			sparse_moved = 1;
>     ++		struct checkout state = CHECKOUT_INIT;
>     ++		state.istate = &the_index;
>     ++
>     ++		if (force)
>     ++			state.force = 1;
>       		if (show_only || verbose)
>       			printf(_("Renaming %s to %s\n"), src, dst);
>       		if (show_only)
>      @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
>     + 		pos = cache_name_pos(src, strlen(src));
>     + 		assert(pos >= 0);
>       		rename_cache_entry_at(pos, dst);
>     ++
>     ++		if (mode & SPARSE) {
>     ++			if (path_in_sparse_checkout(dst, &the_index)) {
>     ++				int dst_pos;
>     ++
>     ++				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"), ce->name);
>     ++			}
>     ++		}
>       	}
>       
>     -+	if (sparse_moved) {
>     -+		struct unpack_trees_options o;
>     -+		memset(&o, 0, sizeof(o));
>     -+		o.verbose_update = isatty(2);
>     -+		o.update = 1;
>     -+		o.head_idx = -1;
>     -+		o.src_index = &the_index;
>     -+		o.dst_index = &the_index;
>     -+		o.skip_sparse_checkout = 0;
>     -+		o.pl = the_index.sparse_checkout_patterns;
>     -+		setup_unpack_trees_porcelain(&o, "mv");
>     -+		update_sparsity(&o);
>     -+		clear_unpack_trees_porcelain(&o);
>     -+	}
>     -+
>       	if (gitmodules_modified)
>     - 		stage_updated_gitmodules(&the_index);
>     - 
>      
>       ## t/t7002-mv-sparse-checkout.sh ##
>     +@@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'can move out-of-cone directory with --sparse' '
>     + 	git mv --sparse folder1 sub 1>actual 2>stderr &&
>     + 	test_must_be_empty stderr &&
>     + 
>     +-	git sparse-checkout reapply &&
>     + 	test_path_is_dir sub/folder1 &&
>     + 	test_path_is_file sub/folder1/file1
>     + '
>     +@@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'can move out-of-cone file with --sparse' '
>     + 	git mv --sparse folder1/file1 sub 1>actual 2>stderr &&
>     + 	test_must_be_empty stderr &&
>     + 
>     +-	git sparse-checkout reapply &&
>     + 	! test_path_is_dir sub/folder1 &&
>     + 	test_path_is_file sub/file1
>     + '
>      @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'refuse to move sparse file to existing destination' '
>       	test_cmp expect stderr
>       '
>       
>     -+# Need fix.
>     -+#
>     -+# The *expected* behavior:
>     -+#
>     -+# Using --sparse to accept a sparse file, --force to overwrite the destination.
>     -+# The folder1/file1 should replace the sub/file1 without error.
>     -+#
>     -+# The *actual* behavior:
>     -+#
>     -+# It emits a warning:
>     -+#
>     -+# warning: Path ' sub/file1
>     -+# ' already present; will not overwrite with sparse update.
>     -+# After fixing the above paths, you may want to run `git sparse-checkout
>     -+# reapply`.
>     -+
>     - test_expect_failure 'move sparse file to existing destination with --force and --sparse' '
>     - 	git sparse-checkout disable &&
>     - 	git reset --hard &&
>     +-test_expect_failure 'move sparse file to existing destination with --force and --sparse' '
>     ++test_expect_success 'move sparse file to existing destination with --force and --sparse' '
>     + 	test_when_finished "cleanup_sparse_checkout" &&
>     + 	mkdir folder1 &&
>     + 	touch folder1/file1 &&
> 
> base-commit: 4f6db706e6ad145a9bf6b26a1ca0970bed27bb72


  parent reply	other threads:[~2022-06-21 23:32 UTC|newest]

Thread overview: 95+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-31  9:17 [WIP v1 0/4] mv: fix out-of-cone file/directory move logic Shaoxuan Yuan
2022-03-31  9:17 ` [WIP v1 1/4] mv: check if out-of-cone file exists in index with SKIP_WORKTREE bit Shaoxuan Yuan
2022-03-31 16:39   ` Victoria Dye
2022-04-01 14:30     ` Derrick Stolee
2022-03-31  9:17 ` [WIP v1 2/4] mv: add check_dir_in_index() and solve general dir check issue Shaoxuan Yuan
2022-03-31 10:25   ` Ævar Arnfjörð Bjarmason
2022-04-01  3:51     ` Shaoxuan Yuan
2022-03-31 21:28   ` Victoria Dye
2022-04-01 12:49     ` Shaoxuan Yuan
2022-04-01 14:49       ` Derrick Stolee
2022-04-04  7:25         ` Shaoxuan Yuan
2022-04-04  7:49           ` Shaoxuan Yuan
2022-04-04 12:43             ` Derrick Stolee
2022-03-31  9:17 ` [WIP v1 3/4] mv: add advise_to_reapply hint for moving file into cone Shaoxuan Yuan
2022-03-31 10:30   ` Ævar Arnfjörð Bjarmason
2022-04-01  4:00     ` Shaoxuan Yuan
2022-04-01  8:02       ` Ævar Arnfjörð Bjarmason
2022-04-03  2:01         ` Eric Sunshine
2022-03-31 21:56   ` Victoria Dye
2022-04-01 14:55   ` Derrick Stolee
2022-03-31  9:17 ` [WIP v1 4/4] t7002: add tests for moving out-of-cone file/directory Shaoxuan Yuan
2022-03-31 10:33   ` Ævar Arnfjörð Bjarmason
2022-03-31 22:11   ` Victoria Dye
2022-03-31  9:28 ` [WIP v1 0/4] mv: fix out-of-cone file/directory move logic Shaoxuan Yuan
2022-03-31 22:21 ` Victoria Dye
2022-04-01 12:18   ` Shaoxuan Yuan
2022-04-08 12:22 ` Shaoxuan Yuan
2022-05-27 10:07 ` [WIP v2 0/5] " Shaoxuan Yuan
2022-05-27 10:08   ` [WIP v2 1/5] t7002: add tests for moving out-of-cone file/directory Shaoxuan Yuan
2022-05-27 12:07     ` Ævar Arnfjörð Bjarmason
2022-05-27 14:48     ` Derrick Stolee
2022-05-27 15:51     ` Victoria Dye
2022-05-27 10:08   ` [WIP v2 2/5] mv: check if out-of-cone file exists in index with SKIP_WORKTREE bit Shaoxuan Yuan
2022-05-27 15:13     ` Derrick Stolee
2022-05-27 22:38       ` Victoria Dye
2022-05-31  8:06       ` Shaoxuan Yuan
2022-05-27 10:08   ` [WIP v2 3/5] mv: check if <destination> exists in index to handle overwriting Shaoxuan Yuan
2022-05-27 22:04     ` Victoria Dye
2022-05-27 10:08   ` [WIP v2 4/5] mv: add check_dir_in_index() and solve general dir check issue Shaoxuan Yuan
2022-05-27 15:27     ` Derrick Stolee
2022-05-31  9:56       ` Shaoxuan Yuan
2022-05-31 15:49         ` Derrick Stolee
2022-05-27 10:08   ` [WIP v2 5/5] mv: use update_sparsity() after touching sparse contents Shaoxuan Yuan
2022-05-27 12:10     ` Ævar Arnfjörð Bjarmason
2022-05-27 19:36     ` Victoria Dye
2022-05-27 19:59       ` Junio C Hamano
2022-05-27 21:24         ` Victoria Dye
2022-06-16 13:51           ` Shaoxuan Yuan
2022-06-16 16:42             ` Victoria Dye
2022-06-17  2:15               ` Shaoxuan Yuan
2022-06-19  3:25 ` [WIP v3 0/7] mv: fix out-of-cone file/directory move logic Shaoxuan Yuan
2022-06-19  3:25   ` [WIP v3 1/7] t7002: add tests for moving out-of-cone file/directory Shaoxuan Yuan
2022-06-21 21:23     ` Victoria Dye
2022-06-19  3:25   ` [WIP v3 2/7] mv: decouple if/else-if checks using goto Shaoxuan Yuan
2022-06-19  3:25   ` [WIP v3 3/7] mv: check if out-of-cone file exists in index with SKIP_WORKTREE bit Shaoxuan Yuan
2022-06-19  3:25   ` [WIP v3 4/7] mv: check if <destination> exists in index to handle overwriting Shaoxuan Yuan
2022-06-19  3:25   ` [WIP v3 5/7] mv: use flags mode for update_mode Shaoxuan Yuan
2022-06-21 22:32     ` Victoria Dye
2022-06-22  9:37       ` Shaoxuan Yuan
2022-06-19  3:25   ` [WIP v3 6/7] mv: add check_dir_in_index() and solve general dir check issue Shaoxuan Yuan
2022-06-21 22:55     ` Victoria Dye
2022-06-19  3:25   ` [WIP v3 7/7] mv: update sparsity after moving from out-of-cone to in-cone Shaoxuan Yuan
2022-06-21 23:11     ` Victoria Dye
2022-06-21 23:30   ` Victoria Dye [this message]
2022-06-23 15:06     ` [WIP v3 0/7] mv: fix out-of-cone file/directory move logic Derrick Stolee
2022-06-23 16:19       ` Junio C Hamano
2022-06-24  8:26         ` Shaoxuan Yuan
2022-06-23 11:41 ` [PATCH v4 " Shaoxuan Yuan
2022-06-23 11:41   ` [PATCH v4 1/7] t7002: add tests for moving out-of-cone file/directory Shaoxuan Yuan
2022-06-23 11:41   ` [PATCH v4 2/7] mv: update sparsity after moving from out-of-cone to in-cone Shaoxuan Yuan
2022-06-23 15:08     ` Derrick Stolee
2022-06-24  8:04       ` Shaoxuan Yuan
2022-06-27 13:55         ` Derrick Stolee
2022-06-23 11:41   ` [PATCH v4 3/7] mv: decouple if/else-if checks using goto Shaoxuan Yuan
2022-06-23 11:41   ` [PATCH v4 4/7] mv: check if out-of-cone file exists in index with SKIP_WORKTREE bit Shaoxuan Yuan
2022-06-23 11:41   ` [PATCH v4 5/7] mv: check if <destination> exists in index to handle overwriting Shaoxuan Yuan
2022-06-23 11:41   ` [PATCH v4 6/7] mv: use flags mode for update_mode Shaoxuan Yuan
2022-06-23 15:10     ` Derrick Stolee
2022-06-23 11:41   ` [PATCH v4 7/7] mv: add check_dir_in_index() and solve general dir check issue Shaoxuan Yuan
2022-06-23 15:14     ` Derrick Stolee
2022-06-24  7:57       ` Shaoxuan Yuan
2022-06-27 13:59         ` Derrick Stolee
2022-06-23 15:16   ` [PATCH v4 0/7] mv: fix out-of-cone file/directory move logic Derrick Stolee
2022-06-23 18:05     ` Junio C Hamano
2022-06-30  2:37 ` [PATCH v5 0/8] " Shaoxuan Yuan
2022-06-30  2:37   ` [PATCH v5 1/8] t7002: add tests for moving out-of-cone file/directory Shaoxuan Yuan
2022-06-30  2:37   ` [PATCH v5 2/8] t1092: mv directory from out-of-cone to in-cone Shaoxuan Yuan
2022-06-30  2:37   ` [PATCH v5 3/8] mv: update sparsity after moving " Shaoxuan Yuan
2022-06-30  2:37   ` [PATCH v5 4/8] mv: decouple if/else-if checks using goto Shaoxuan Yuan
2022-06-30  2:37   ` [PATCH v5 5/8] mv: check if out-of-cone file exists in index with SKIP_WORKTREE bit Shaoxuan Yuan
2022-06-30  2:37   ` [PATCH v5 6/8] mv: check if <destination> exists in index to handle overwriting Shaoxuan Yuan
2022-06-30  2:37   ` [PATCH v5 7/8] mv: use flags mode for update_mode Shaoxuan Yuan
2022-06-30  2:37   ` [PATCH v5 8/8] mv: add check_dir_in_index() and solve general dir check issue Shaoxuan Yuan
2022-07-01 19:43   ` [PATCH v5 0/8] mv: fix out-of-cone file/directory move logic Derrick Stolee
2022-07-01 21:50     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d3aa35e4-5bad-2bbb-2a25-d82064d6ec81@github.com \
    --to=vdye@github.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=newren@gmail.com \
    --cc=shaoxuan.yuan02@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).