mv from in-cone to out-of-cone #3

ffyuanda · 2022-07-06T14:16:57Z

This is the latest update on in-to-out, it is roughly working. Please give it some suggestions!

Thanks!

Add corresponding tests to test that user 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. 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. Instead advise user to "git add" this path and run "git sparse-checkout reapply" to re-sparsify that path. Signed-off-by: Shaoxuan Yuan <[email protected]>

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 first form of 'git-mv'. Change the logic so that when supplied with --sparse, first check if <destination> is a directory with all its contents sparsified (called a SKIP_WORKTREE_DIR). If yes, then treat <destination> as a directory exists in the working tree. If no, continue the original checking logic. Signed-off-by: Shaoxuan Yuan <[email protected]>

Rename BOTH flag to DEFAULT, since BOTH seems to does not stand for any useful meanings, it should be named DEFAULT instead. Signed-off-by: Shaoxuan Yuan <[email protected]>

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. 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. Instead advise user to "git add" this path and run "git sparse-checkout reapply" to re-sparsify that path. Signed-off-by: Shaoxuan Yuan <[email protected]>

vdye

Overall looks good! Added a couple of comments, but they're (hopefully) relatively minor nits.

As for the CI tests - as far as I can tell, the failures don't come from your changes, so you're probably in the clear. Before submitting upstream, though, you should rebase on top of next (or master, if your sy/mv-out-of-cone branch is merged in by the time you want to submit this series). Hopefully, the test failures will be cleared by then.

vdye · 2022-07-11T23:42:58Z

t/t7002-mv-sparse-checkout.sh

+	git mv --sparse sub/dir folder1 2>stderr &&
+	cat >expect <<-EOF &&
+	The following dirty paths and/or pathspecs are moved
+	but not sparsified. Use "git add" to stage them then
+	use "git sparse-checkout reapply" to sparsify them.
+	folder1/dir/e2
+	folder1/dir/e3
+	EOF
+	test_cmp expect stderr &&


After this section, you might want to verify that the non-dirty contents of the folder were not moved, i.e.:

test_path_is_file sub/dir/e

EDIT: nevermind, didn't read the warning message. Instead, you can check that sub/dir/e was moved, and exists on disk:

test_path_is_empty sub/dir && test_path_is_empty folder1/e && test_path_is_file folder1/e2 &&

The logic in this in-cone to out-of-cone move is that:

Clean paths are sparsified.

Dirty paths are moved to the required (usually requires mkdir -p), and they are not sparsified.

So I think the "non-dirty contents of the folder" should actually be moved and sparsified?

Correct, I added an update after the "EDIT" in my previous comment.

builtin/mv.c

vdye · 2022-07-11T23:48:47Z

builtin/mv.c

 		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
 	} else {
-		if (argc != 1)
+		if (!check_dir_in_index(dest_path[0])) {


The commit message mentions that this should only be done if '--sparse' is provided ("Change the logic so that when supplied with --sparse...", but I don't see any validation of that flag here. Is that done implicitly as part of 'check_dir_in_index()'? If so, it would help to have a comment in this if/else block clarifying that.

vdye · 2022-07-11T23:52:55Z

builtin/mv.c


 enum update_mode {
-	BOTH = 0,
+	DEFAULT = 0,


Rather than rename this, why not remove it entirely? The elements of this enum are being treated as flags, but 0 isn't really a "flag" value - you don't |= it to "apply" it (it's not even really "applied"), and it's mutually exclusive with all the other flag values.

Since it's (as far as I can tell?) not used explicitly anywhere, I think it's more confusing than not to keep it.

vdye · 2022-07-12T00:13:42Z

advice.c

+	fprintf(stderr, _("The following dirty paths and/or pathspecs are moved\n"
+			  "but not sparsified. Use \"git add\" to stage them then\n"
+			  "use \"git sparse-checkout reapply\" to sparsify them.\n"));
+	for_each_string_list_item(item, pathspec_list)
+		fprintf(stderr, "%s\n", item->string);


A couple of suggestions to make the advice clearer:

Separate the warning message from the advice to fix the issue and allow the user to disable the non-warning advice with the advice.updateSparsePath config (see advise_on_updating_sparse_paths()).

Recommend --sparse flag for git add (since the user would be adding paths outside the sparse checkout definition).

Change "sparsify" to more precise references to "sparse-checkout definition" and/or "sparsity rules" (for consistency with advise_on_updating_sparse_paths())

You don't need to use my exact wording (below) if you'd rather write something else, but at a high level these suggestions should clarify things for users.

Suggested change

fprintf(stderr, _("The following dirty paths and/or pathspecs are moved\n"

"but not sparsified. Use \"git add\" to stage them then\n"

"use \"git sparse-checkout reapply\" to sparsify them.\n"));

for_each_string_list_item(item, pathspec_list)

fprintf(stderr, "%s\n", item->string);

fprintf(stderr, _("The following paths have been moved outside the\n"

"sparse-checkout definition but are not sparse due to local\n"

"modifications. then use \"git sparse-checkout reapply\" to\n"

"reapply sparsity rules patterns.\n"));

for_each_string_list_item(item, pathspec_list)

fprintf(stderr, "%s\n", item->string);

advise_if_enabled(ADVICE_UPDATE_SPARSE_PATH,

_("To correct the sparsity of these paths, do the following:\n"

"* Use \"git add --sparse <paths>\" to update the index\n"

"* Use \"git sparse-checkout reapply\" to apply the sparsity rules"));

Thanks for the comments, they are really helpful! I was thinking about adding a new advice config, but you pointed out that I can just use ADVICE_UPDATE_SPARSE_PATH, which is neat!

vdye · 2022-07-12T00:18:38Z

builtin/mv.c

+		if (ignore_sparse &&
+		    !init_sparse_checkout_patterns(&the_index) &&
+		    the_index.sparse_checkout_patterns->use_cone_patterns) {


path_in_sparse_checkout() will initialize the patterns implicitly, so there's no need to setup the patterns early (especially because subsequent conditions could make it unnecessary). A less computationally expensive way to do this condition's check would be:

Suggested change

if (ignore_sparse &&

!init_sparse_checkout_patterns(&the_index) &&

the_index.sparse_checkout_patterns->use_cone_patterns) {

if (ignore_sparse &&

core_apply_sparse_checkout &&

core_sparse_checkout_cone) {

Sure! This looks a lot cheaper, I did not know these two flags.

Since commit fcc07e9 (is_promisor_object(): free tree buffer after parsing, 2021-04-13), we'll always free the buffers attached to a "struct tree" after searching them for promisor links. But there's an important case where we don't want to do so: if somebody else is already using the tree! This can happen during a "rev-list --missing=allow-promisor" traversal in a partial clone that is missing one or more trees or blobs. The backtrace for the free looks like this: #1 free_tree_buffer tree.c:147 #2 add_promisor_object packfile.c:2250 #3 for_each_object_in_pack packfile.c:2190 #4 for_each_packed_object packfile.c:2215 #5 is_promisor_object packfile.c:2272 #6 finish_object__ma builtin/rev-list.c:245 #7 finish_object builtin/rev-list.c:261 #8 show_object builtin/rev-list.c:274 #9 process_blob list-objects.c:63 git#10 process_tree_contents list-objects.c:145 git#11 process_tree list-objects.c:201 git#12 traverse_trees_and_blobs list-objects.c:344 [...] We're in the middle of walking through the entries of a tree object via process_tree_contents(). We see a blob (or it could even be another tree entry) that we don't have, so we call is_promisor_object() to check it. That function loops over all of the objects in the promisor packfile, including the tree we're currently walking. When we're done with it there, we free the tree buffer. But as we return to the walk in process_tree_contents(), it's still holding on to a pointer to that buffer, via its tree_desc iterator, and it accesses the freed memory. Even a trivial use of "--missing=allow-promisor" triggers this problem, as the included test demonstrates (it's just a vanilla --blob:none clone). We can detect this case by only freeing the tree buffer if it was allocated on our behalf. This is a little tricky since that happens inside parse_object(), and it doesn't tell us whether the object was already parsed, or whether it allocated the buffer itself. But by checking for an already-parsed tree beforehand, we can distinguish the two cases. That feels a little hacky, and does incur an extra lookup in the object-hash table. But that cost is fairly minimal compared to actually loading objects (and since we're iterating the whole pack here, we're likely to be loading most objects, rather than reusing cached results). It may also be a good direction for this function in general, as there are other possible optimizations that rely on doing some analysis before parsing: - we could detect blobs and avoid reading their contents; they can't link to other objects, but parse_object() doesn't know that we don't care about checking their hashes. - we could avoid allocating object structs entirely for most objects (since we really only need them in the oidset), which would save some memory. - promisor commits could use the commit-graph rather than loading the object from disk This commit doesn't do any of those optimizations, but I think it argues that this direction is reasonable, rather than relying on parse_object() and trying to teach it to give us more information about whether it parsed. The included test fails reliably under SANITIZE=address just when running "rev-list --missing=allow-promisor". Checking the output isn't strictly necessary to detect the bug, but it seems like a reasonable addition given the general lack of coverage for "allow-promisor" in the test suite. Reported-by: Andrew Olsen <[email protected]> Signed-off-by: Jeff King <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>

Fix a memory leak occuring in case of pathspec copy in preload_index. Direct leak of 8 byte(s) in 8 object(s) allocated from: #0 0x7f0a353ead47 in __interceptor_malloc (/usr/lib/gcc/x86_64-pc-linux-gnu/11.3.0/libasan.so.6+0xb5d47) #1 0x55750995e840 in do_xmalloc /home/anthony/src/c/git/wrapper.c:51 #2 0x55750995e840 in xmalloc /home/anthony/src/c/git/wrapper.c:72 #3 0x55750970f824 in copy_pathspec /home/anthony/src/c/git/pathspec.c:684 #4 0x557509717278 in preload_index /home/anthony/src/c/git/preload-index.c:135 #5 0x55750975f21e in refresh_index /home/anthony/src/c/git/read-cache.c:1633 #6 0x55750915b926 in cmd_status builtin/commit.c:1547 #7 0x5575090e1680 in run_builtin /home/anthony/src/c/git/git.c:466 #8 0x5575090e1680 in handle_builtin /home/anthony/src/c/git/git.c:720 #9 0x5575090e284a in run_argv /home/anthony/src/c/git/git.c:787 git#10 0x5575090e284a in cmd_main /home/anthony/src/c/git/git.c:920 git#11 0x5575090dbf82 in main /home/anthony/src/c/git/common-main.c:56 git#12 0x7f0a348230ab (/lib64/libc.so.6+0x290ab) Signed-off-by: Anthony Delannoy <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>

ffyuanda added 4 commits July 6, 2022 21:06

mv: rename BOTH flag to DEFAULT

044cd40

Rename BOTH flag to DEFAULT, since BOTH seems to does not stand for any useful meanings, it should be named DEFAULT instead. Signed-off-by: Shaoxuan Yuan <[email protected]>

ffyuanda force-pushed the mv/in-to-out-v1.2 branch from 4b11e55 to 34b53e9 Compare July 9, 2022 07:46

vdye reviewed Jul 12, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

mv from in-cone to out-of-cone #3

mv from in-cone to out-of-cone #3

Uh oh!

ffyuanda commented Jul 6, 2022

Uh oh!

vdye left a comment

Uh oh!

vdye Jul 11, 2022 •

edited

Loading

Uh oh!

ffyuanda Jul 14, 2022 •

edited

Loading

Uh oh!

vdye Jul 14, 2022

Uh oh!

Uh oh!

vdye Jul 11, 2022

Uh oh!

vdye Jul 11, 2022

Uh oh!

vdye Jul 12, 2022

Uh oh!

ffyuanda Jul 15, 2022 •

edited

Loading

Uh oh!

vdye Jul 12, 2022

Uh oh!

ffyuanda Jul 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-	fprintf(stderr, _("The following dirty paths and/or pathspecs are moved\n"
-			  "but not sparsified. Use \"git add\" to stage them then\n"
-			  "use \"git sparse-checkout reapply\" to sparsify them.\n"));
-	for_each_string_list_item(item, pathspec_list)
-		fprintf(stderr, "%s\n", item->string);
+	fprintf(stderr, _("The following paths have been moved outside the\n"
+			  "sparse-checkout definition but are not sparse due to local\n"
+			  "modifications.  then use \"git sparse-checkout reapply\" to\n"
+			  "reapply sparsity rules patterns.\n"));
+	for_each_string_list_item(item, pathspec_list)
+		fprintf(stderr, "%s\n", item->string);
+	advise_if_enabled(ADVICE_UPDATE_SPARSE_PATH,
+			  _("To correct the sparsity of these paths, do the following:\n"
+			    "* Use \"git add --sparse <paths>\" to update the index\n"
+			    "* Use \"git sparse-checkout reapply\" to apply the sparsity rules"));

mv from in-cone to out-of-cone #3

Are you sure you want to change the base?

mv from in-cone to out-of-cone #3

Uh oh!

Conversation

ffyuanda commented Jul 6, 2022

Uh oh!

vdye left a comment

Choose a reason for hiding this comment

Uh oh!

vdye Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ffyuanda Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdye Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vdye Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

vdye Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

vdye Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

ffyuanda Jul 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdye Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

ffyuanda Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vdye Jul 11, 2022 •

edited

Loading

ffyuanda Jul 14, 2022 •

edited

Loading

ffyuanda Jul 15, 2022 •

edited

Loading