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

Re: [WIP v1 1/4] mv: check if out-of-cone file exists in index with SKIP_WORKTREE bit

From
Derrick Stolee <derrickstolee@github.com>
Date
Apr 1, 2022, 14:30 UTC
Message-ID
<edbbd81e-2117-c9b9-76a5-4713e6326d2f@github.com>
In-Reply-To
<180efaaf-7bb5-6ed7-2fc6-3c5d5f1304db@github.com>
On 3/31/2022 12:39 PM, Victoria Dye wrote:
> Shaoxuan Yuan wrote:
Show 25 quoted lines
>>  		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.
>> +			 */
>> +
> 
> I can clarify this a bit. 'mv' is done in two steps: first the file-on-disk
> rename (in the call to 'rename()'), then the index entry (in
> 'rename_cache_entry_at()'). In the case of a sparse file, you're only
> dealing with the latter. However, 'rename_cache_entry_at()' moves the index
> entry with the flag 'ADD_CACHE_OK_TO_REPLACE', since it leaves it up to
> 'cmd_mv()' to enforce the "no overwrite" rule. 
> 
> So, in the case of moving *to* a SKIP_WORKTREE entry (where a file being
> present won't trigger the failure), you'll want to check that the
> destination *index entry* doesn't exist in addition to the 'lstat()' check.
> It might require some rearranging of if-statements in this block, but I
> think it can be done in 'cmd_mv'.

This also explains the issue when going from sparse to non-sparse: the file move is the expected way to populate the end-result, but we skip that part in the sparse case. We need to do an extra step to populate the file from the version in the index (after moving the cache entry).

Related to this chain of if/else if/else blocks, it might be worth refactoring them to be sequential "if ()" blocks where we jump to a "cleanup:" label via a 'goto' if we know that we are in a failure mode.

The previous organization made sense because any of the if () or else if () conditions were a failure mode. However, it might be better to rearrange things to be clearer about the situation.

Here is a diff from what I was playing with. It's... unclear if this is a better arrangement, but I thought it worth discussing.

--- >8 ---
diff --git a/builtin/mv.c b/builtin/mv.c
index 83a465ba831..683a412a3fc 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -186,15 +186,22 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		length = strlen(src);
 		if (lstat(src, &st) < 0) {
 			/* only error if existence is expected. */
-			if (modes[i] != SPARSE)
+			if (modes[i] != SPARSE) {
 				bad = _("bad source");
-		} else if (!strncmp(src, dst, length) &&
+				goto checked_move;
+			}
+		}
+		if (!strncmp(src, dst, length) &&
 				(dst[length] == 0 || dst[length] == '/')) {
 			bad = _("can not move directory into itself");
-		} else if ((src_is_dir = S_ISDIR(st.st_mode))
-				&& lstat(dst, &st) == 0)
+			goto checked_move;
+		}
+		if ((src_is_dir = S_ISDIR(st.st_mode))
+				&& lstat(dst, &st) == 0) {
 			bad = _("cannot move directory over file");
-		else if (src_is_dir) {
+			goto checked_move;
+		}
+		if (src_is_dir) {
 			int first = cache_name_pos(src, length), last;
 
 			if (first >= 0)
@@ -227,11 +234,18 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				}
 				argc += last - first;
 			}
-		} else if (!(ce = cache_file_exists(src, length, 0))) {
+
+			goto checked_move;
+		}
+		if (!(ce = cache_file_exists(src, length, 0))) {
 			bad = _("not under version control");
-		} else if (ce_stage(ce)) {
+			goto checked_move;
+		}
+		if (ce_stage(ce)) {
 			bad = _("conflicted");
-		} else if (lstat(dst, &st) == 0 &&
+			goto checked_move;
+		}
+		if (lstat(dst, &st) == 0 &&
 			 (!ignore_case || strcasecmp(src, dst))) {
 			bad = _("destination exists");
 			if (force) {
@@ -246,34 +260,40 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				} else
 					bad = _("Cannot overwrite");
 			}
-		} else if (string_list_has_string(&src_for_dst, dst))
+			goto checked_move;
+		}
+		if (string_list_has_string(&src_for_dst, dst)) {
 			bad = _("multiple sources for the same target");
-		else if (is_dir_sep(dst[strlen(dst) - 1]))
+			goto checked_move;
+		}
+		if (is_dir_sep(dst[strlen(dst) - 1])) {
 			bad = _("destination directory does not exist");
-		else {
-			/*
-			 * We check if the paths are in the sparse-checkout
-			 * definition as a very final check, since that
-			 * allows us to point the user to the --sparse
-			 * option as a way to have a successful run.
-			 */
-			if (!ignore_sparse &&
-			    !path_in_sparse_checkout(src, &the_index)) {
-				string_list_append(&only_match_skip_worktree, src);
-				skip_sparse = 1;
-			}
-			if (!ignore_sparse &&
-			    !path_in_sparse_checkout(dst, &the_index)) {
-				string_list_append(&only_match_skip_worktree, dst);
-				skip_sparse = 1;
-			}
-
-			if (skip_sparse)
-				goto remove_entry;
+			goto checked_move;
+		}
 
-			string_list_insert(&src_for_dst, dst);
+		/*
+		 * We check if the paths are in the sparse-checkout
+		 * definition as a very final check, since that
+		 * allows us to point the user to the --sparse
+		 * option as a way to have a successful run.
+		 */
+		if (!ignore_sparse &&
+		    !path_in_sparse_checkout(src, &the_index)) {
+			string_list_append(&only_match_skip_worktree, src);
+			skip_sparse = 1;
+		}
+		if (!ignore_sparse &&
+		    !path_in_sparse_checkout(dst, &the_index)) {
+			string_list_append(&only_match_skip_worktree, dst);
+			skip_sparse = 1;
 		}
 
+		if (skip_sparse)
+			goto remove_entry;
+
+		string_list_insert(&src_for_dst, dst);
+
+checked_move:
 		if (!bad)
 			continue;
 		if (!ignore_errors)

--- >8 --- 
>> +			}
>>  			/* only error if existence is expected. */
>> -			if (modes[i] != SPARSE)
>> +			else if (modes[i] != SPARSE)
>>  				bad = _("bad source");
>>  		} else if (!strncmp(src, dst, length) &&
>>  				(dst[length] == 0 || dst[length] == '/')) {
> 
> For a change like this, it would be really helpful to include the tests
> showing how sparse file moves should now be treated in this commit. I see
> that you've added some in patch 4 - could you move the ones related tothis
> change into this commit?

I completely agree: it's nice to see how behavior is intended to change
next to your code change.

> Another way you could do this is to put your "add tests" commit first in
> this series, changing the condition on the ones that are fixed later in the
> series to "test_expect_failure". Then, in each commit that "fixes" a test's
> behavior, change that test to "test_expect_success". This approach had the
> added benefit of showing that, before this series, the tests would fail and
> that this series explicitly fixes those scenarios.

And this would be easier to adapt your current patch structure to this model:
move the last commit to be first, but flip the expectation. Then modify the
expectation for the tests that pass as you go.

This only works as long as you can make an entire test pass with each change.
If multiple changes are needed to make any one test pass, then we don't get
the benefit we're looking for. In that case, your test might be covering too
much behavior in a single test, so it would be worth rewriting the tests to
check a smaller part of the behavior.

Thanks,
-Stolee
Previous: Victoria DyeNext: Shaoxuan Yuan
Message 4 of 95 in “[WIP v1 0/4] mv: fix out-of-cone file/directory move logic”
  1. Shaoxuan YuanMar 31, 2022
  2. 1/4 mv: check if out-of-cone file exists in index with SKIP_WORKTREE bitShaoxuan Yuan, Mar 31, 2022
  3. Victoria DyeMar 31, 2022
  4. Derrick StoleeApr 1, 2022
  5. 2/4 mv: add check_dir_in_index() and solve general dir check issueShaoxuan Yuan, Mar 31, 2022
  6. Ævar Arnfjörð BjarmasonMar 31, 2022
  7. Shaoxuan YuanApr 1, 2022
  8. Victoria DyeMar 31, 2022
  9. Shaoxuan YuanApr 1, 2022
  10. Derrick StoleeApr 1, 2022
  11. Shaoxuan YuanApr 4, 2022
  12. Shaoxuan YuanApr 4, 2022
  13. Derrick StoleeApr 4, 2022
  14. 3/4 mv: add advise_to_reapply hint for moving file into coneShaoxuan Yuan, Mar 31, 2022
  15. Ævar Arnfjörð BjarmasonMar 31, 2022
  16. Shaoxuan YuanApr 1, 2022
  17. Ævar Arnfjörð BjarmasonApr 1, 2022
  18. Eric SunshineApr 3, 2022
  19. Victoria DyeMar 31, 2022
  20. Derrick StoleeApr 1, 2022
  21. 4/4 t7002: add tests for moving out-of-cone file/directoryShaoxuan Yuan, Mar 31, 2022
  22. Ævar Arnfjörð BjarmasonMar 31, 2022
  23. Victoria DyeMar 31, 2022
  24. Shaoxuan YuanMar 31, 2022
  25. Victoria DyeMar 31, 2022
  26. Shaoxuan YuanApr 1, 2022
  27. Shaoxuan YuanApr 8, 2022
  28. 0/5 mv: fix out-of-cone file/directory move logicShaoxuan Yuan, May 27, 2022
  29. 1/5 t7002: add tests for moving out-of-cone file/directoryShaoxuan Yuan, May 27, 2022
  30. Ævar Arnfjörð BjarmasonMay 27, 2022
  31. Derrick StoleeMay 27, 2022
  32. Victoria DyeMay 27, 2022
  33. 2/5 mv: check if out-of-cone file exists in index with SKIP_WORKTREE bitShaoxuan Yuan, May 27, 2022
  34. Derrick StoleeMay 27, 2022
  35. Victoria DyeMay 27, 2022
  36. Shaoxuan YuanMay 31, 2022
  37. 3/5 mv: check if <destination> exists in index to handle overwritingShaoxuan Yuan, May 27, 2022
  38. Victoria DyeMay 27, 2022
  39. 4/5 mv: add check_dir_in_index() and solve general dir check issueShaoxuan Yuan, May 27, 2022
  40. Derrick StoleeMay 27, 2022
  41. Shaoxuan YuanMay 31, 2022
  42. Derrick StoleeMay 31, 2022
  43. 5/5 mv: use update_sparsity() after touching sparse contentsShaoxuan Yuan, May 27, 2022
  44. Ævar Arnfjörð BjarmasonMay 27, 2022
  45. Victoria DyeMay 27, 2022
  46. Junio C HamanoMay 27, 2022
  47. Victoria DyeMay 27, 2022
  48. Shaoxuan YuanJun 16, 2022
  49. Victoria DyeJun 16, 2022
  50. Shaoxuan YuanJun 17, 2022
  51. 0/7 mv: fix out-of-cone file/directory move logicShaoxuan Yuan, Jun 19, 2022
  52. 1/7 t7002: add tests for moving out-of-cone file/directoryShaoxuan Yuan, Jun 19, 2022
  53. Victoria DyeJun 21, 2022
  54. 2/7 mv: decouple if/else-if checks using gotoShaoxuan Yuan, Jun 19, 2022
  55. 3/7 mv: check if out-of-cone file exists in index with SKIP_WORKTREE bitShaoxuan Yuan, Jun 19, 2022
  56. 4/7 mv: check if <destination> exists in index to handle overwritingShaoxuan Yuan, Jun 19, 2022
  57. 5/7 mv: use flags mode for update_modeShaoxuan Yuan, Jun 19, 2022
  58. Victoria DyeJun 21, 2022
  59. Shaoxuan YuanJun 22, 2022
  60. 6/7 mv: add check_dir_in_index() and solve general dir check issueShaoxuan Yuan, Jun 19, 2022
  61. Victoria DyeJun 21, 2022
  62. 7/7 mv: update sparsity after moving from out-of-cone to in-coneShaoxuan Yuan, Jun 19, 2022
  63. Victoria DyeJun 21, 2022
  64. Victoria DyeJun 21, 2022
  65. Derrick StoleeJun 23, 2022
  66. Junio C HamanoJun 23, 2022
  67. Shaoxuan YuanJun 24, 2022
  68. 0/7 mv: fix out-of-cone file/directory move logicShaoxuan Yuan, Jun 23, 2022
  69. 1/7 t7002: add tests for moving out-of-cone file/directoryShaoxuan Yuan, Jun 23, 2022
  70. 2/7 mv: update sparsity after moving from out-of-cone to in-coneShaoxuan Yuan, Jun 23, 2022
  71. Derrick StoleeJun 23, 2022
  72. Shaoxuan YuanJun 24, 2022
  73. Derrick StoleeJun 27, 2022
  74. 3/7 mv: decouple if/else-if checks using gotoShaoxuan Yuan, Jun 23, 2022
  75. 4/7 mv: check if out-of-cone file exists in index with SKIP_WORKTREE bitShaoxuan Yuan, Jun 23, 2022
  76. 5/7 mv: check if <destination> exists in index to handle overwritingShaoxuan Yuan, Jun 23, 2022
  77. 6/7 mv: use flags mode for update_modeShaoxuan Yuan, Jun 23, 2022
  78. Derrick StoleeJun 23, 2022
  79. 7/7 mv: add check_dir_in_index() and solve general dir check issueShaoxuan Yuan, Jun 23, 2022
  80. Derrick StoleeJun 23, 2022
  81. Shaoxuan YuanJun 24, 2022
  82. Derrick StoleeJun 27, 2022
  83. Derrick StoleeJun 23, 2022
  84. Junio C HamanoJun 23, 2022
  85. 0/8 mv: fix out-of-cone file/directory move logicShaoxuan Yuan, Jun 30, 2022
  86. 1/8 t7002: add tests for moving out-of-cone file/directoryShaoxuan Yuan, Jun 30, 2022
  87. 2/8 t1092: mv directory from out-of-cone to in-coneShaoxuan Yuan, Jun 30, 2022
  88. 3/8 mv: update sparsity after moving from out-of-cone to in-coneShaoxuan Yuan, Jun 30, 2022
  89. 4/8 mv: decouple if/else-if checks using gotoShaoxuan Yuan, Jun 30, 2022
  90. 5/8 mv: check if out-of-cone file exists in index with SKIP_WORKTREE bitShaoxuan Yuan, Jun 30, 2022
  91. 6/8 mv: check if <destination> exists in index to handle overwritingShaoxuan Yuan, Jun 30, 2022
  92. 7/8 mv: use flags mode for update_modeShaoxuan Yuan, Jun 30, 2022
  93. 8/8 mv: add check_dir_in_index() and solve general dir check issueShaoxuan Yuan, Jun 30, 2022
  94. Derrick StoleeJul 1, 2022
  95. Junio C HamanoJul 1, 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.