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

[PATCH v10 25/36] merge-recursive: fix overwriting dirty files involved in renames

From
Elijah Newren <newren@gmail.com>
Date
Apr 19, 2018, 17:58 UTC
Message-ID
<20180419175823.7946-26-newren@gmail.com>
In-Reply-To
<20180419175823.7946-1-newren@gmail.com>

This fixes an issue that existed before my directory rename detection patches that affects both normal renames and renames implied by directory rename detection. Additional codepaths that only affect overwriting of dirty files that are involved in directory rename detection will be added in a subsequent commit.

Reviewed-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 merge-recursive.c                   | 85 ++++++++++++++++++++++-------
 merge-recursive.h                   |  2 +
 t/t3501-revert-cherry-pick.sh       |  2 +-
 t/t6043-merge-rename-directories.sh |  2 +-
 t/t7607-merge-overwrite.sh          |  2 +-
 unpack-trees.c                      |  4 +-
 unpack-trees.h                      |  4 ++
 7 files changed, 77 insertions(+), 24 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index c1c4faf61e..7fdcba4f22 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -337,32 +337,37 @@ static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)
 	init_tree_desc(desc, tree->buffer, tree->size);
 }
 
-static int git_merge_trees(int index_only,
+static int git_merge_trees(struct merge_options *o,
 			   struct tree *common,
 			   struct tree *head,
 			   struct tree *merge)
 {
 	int rc;
 	struct tree_desc t[3];
-	struct unpack_trees_options opts;
 
-	memset(&opts, 0, sizeof(opts));
-	if (index_only)
-		opts.index_only = 1;
+	memset(&o->unpack_opts, 0, sizeof(o->unpack_opts));
+	if (o->call_depth)
+		o->unpack_opts.index_only = 1;
 	else
-		opts.update = 1;
-	opts.merge = 1;
-	opts.head_idx = 2;
-	opts.fn = threeway_merge;
-	opts.src_index = &the_index;
-	opts.dst_index = &the_index;
-	setup_unpack_trees_porcelain(&opts, "merge");
+		o->unpack_opts.update = 1;
+	o->unpack_opts.merge = 1;
+	o->unpack_opts.head_idx = 2;
+	o->unpack_opts.fn = threeway_merge;
+	o->unpack_opts.src_index = &the_index;
+	o->unpack_opts.dst_index = &the_index;
+	setup_unpack_trees_porcelain(&o->unpack_opts, "merge");
 
 	init_tree_desc_from_tree(t+0, common);
 	init_tree_desc_from_tree(t+1, head);
 	init_tree_desc_from_tree(t+2, merge);
 
-	rc = unpack_trees(3, t, &opts);
+	rc = unpack_trees(3, t, &o->unpack_opts);
+	/*
+	 * unpack_trees NULLifies src_index, but it's used in verify_uptodate,
+	 * so set to the new index which will usually have modification
+	 * timestamp info copied over.
+	 */
+	o->unpack_opts.src_index = &the_index;
 	cache_tree_free(&active_cache_tree);
 	return rc;
 }
@@ -795,6 +800,20 @@ static int would_lose_untracked(const char *path)
 	return !was_tracked(path) && file_exists(path);
 }
 
+static int was_dirty(struct merge_options *o, const char *path)
+{
+	struct cache_entry *ce;
+	int dirty = 1;
+
+	if (o->call_depth || !was_tracked(path))
+		return !dirty;
+
+	ce = cache_file_exists(path, strlen(path), ignore_case);
+	dirty = (ce->ce_stat_data.sd_mtime.sec > 0 &&
+		 verify_uptodate(ce, &o->unpack_opts) != 0);
+	return dirty;
+}
+
 static int make_room_for_path(struct merge_options *o, const char *path)
 {
 	int status, i;
@@ -2687,6 +2706,7 @@ static int handle_modify_delete(struct merge_options *o,
 
 static int merge_content(struct merge_options *o,
 			 const char *path,
+			 int file_in_way,
 			 struct object_id *o_oid, int o_mode,
 			 struct object_id *a_oid, int a_mode,
 			 struct object_id *b_oid, int b_mode,
@@ -2761,7 +2781,7 @@ static int merge_content(struct merge_options *o,
 				return -1;
 	}
 
-	if (df_conflict_remains) {
+	if (df_conflict_remains || file_in_way) {
 		char *new_path;
 		if (o->call_depth) {
 			remove_file_from_cache(path);
@@ -2795,6 +2815,30 @@ static int merge_content(struct merge_options *o,
 	return mfi.clean;
 }
 
+static int conflict_rename_normal(struct merge_options *o,
+				  const char *path,
+				  struct object_id *o_oid, unsigned int o_mode,
+				  struct object_id *a_oid, unsigned int a_mode,
+				  struct object_id *b_oid, unsigned int b_mode,
+				  struct rename_conflict_info *ci)
+{
+	int clean_merge;
+	int file_in_the_way = 0;
+
+	if (was_dirty(o, path)) {
+		file_in_the_way = 1;
+		output(o, 1, _("Refusing to lose dirty file at %s"), path);
+	}
+
+	/* Merge the content and write it out */
+	clean_merge = merge_content(o, path, file_in_the_way,
+				    o_oid, o_mode, a_oid, a_mode, b_oid, b_mode,
+				    ci);
+	if (clean_merge > 0 && file_in_the_way)
+		clean_merge = 0;
+	return clean_merge;
+}
+
 /* Per entry merge function */
 static int process_entry(struct merge_options *o,
 			 const char *path, struct stage_data *entry)
@@ -2814,9 +2858,12 @@ static int process_entry(struct merge_options *o,
 		switch (conflict_info->rename_type) {
 		case RENAME_NORMAL:
 		case RENAME_ONE_FILE_TO_ONE:
-			clean_merge = merge_content(o, path,
-						    o_oid, o_mode, a_oid, a_mode, b_oid, b_mode,
-						    conflict_info);
+			clean_merge = conflict_rename_normal(o,
+							     path,
+							     o_oid, o_mode,
+							     a_oid, a_mode,
+							     b_oid, b_mode,
+							     conflict_info);
 			break;
 		case RENAME_DIR:
 			clean_merge = 1;
@@ -2912,7 +2959,7 @@ static int process_entry(struct merge_options *o,
 	} else if (a_oid && b_oid) {
 		/* Case C: Added in both (check for same permissions) and */
 		/* case D: Modified in both, but differently. */
-		clean_merge = merge_content(o, path,
+		clean_merge = merge_content(o, path, 0 /* file_in_way */,
 					    o_oid, o_mode, a_oid, a_mode, b_oid, b_mode,
 					    NULL);
 	} else if (!o_oid && !a_oid && !b_oid) {
@@ -2953,7 +3000,7 @@ int merge_trees(struct merge_options *o,
 		return 1;
 	}
 
-	code = git_merge_trees(o->call_depth, common, head, merge);
+	code = git_merge_trees(o, common, head, merge);
 
 	if (code != 0) {
 		if (show(o, 4) || o->call_depth)
diff --git a/merge-recursive.h b/merge-recursive.h
index 50a4e6af4e..d863cf8867 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -1,6 +1,7 @@
 #ifndef MERGE_RECURSIVE_H
 #define MERGE_RECURSIVE_H
 
+#include "unpack-trees.h"
 #include "string-list.h"
 
 struct merge_options {
@@ -27,6 +28,7 @@ struct merge_options {
 	struct strbuf obuf;
 	struct hashmap current_file_dir_set;
 	struct string_list df_conflict_file_set;
+	struct unpack_trees_options unpack_opts;
 };
 
 /*
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index ccbc118514..c9a1f783f5 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -141,7 +141,7 @@ test_expect_success 'cherry-pick "-" works with arguments' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'cherry-pick works with dirty renamed file' '
+test_expect_success 'cherry-pick works with dirty renamed file' '
 	test_commit to-rename &&
 	git checkout -b unrelated &&
 	test_commit unrelated &&
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
index 0b60eb8053..b94ba066fe 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -3298,7 +3298,7 @@ test_expect_success '11a-setup: Avoid losing dirty contents with simple rename'
 	)
 '
 
-test_expect_failure '11a-check: Avoid losing dirty contents with simple rename' '
+test_expect_success '11a-check: Avoid losing dirty contents with simple rename' '
 	(
 		cd 11a &&
 
diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh
index 9c422bcd7c..dd8ab7ede1 100755
--- a/t/t7607-merge-overwrite.sh
+++ b/t/t7607-merge-overwrite.sh
@@ -92,7 +92,7 @@ test_expect_success 'will not overwrite removed file with staged changes' '
 	test_cmp important c1.c
 '
 
-test_expect_failure 'will not overwrite unstaged changes in renamed file' '
+test_expect_success 'will not overwrite unstaged changes in renamed file' '
 	git reset --hard c1 &&
 	git mv c1.c other.c &&
 	git commit -m rename &&
diff --git a/unpack-trees.c b/unpack-trees.c
index e73745051e..79fd97074e 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1509,8 +1509,8 @@ static int verify_uptodate_1(const struct cache_entry *ce,
 		add_rejected_path(o, error_type, ce->name);
 }
 
-static int verify_uptodate(const struct cache_entry *ce,
-			   struct unpack_trees_options *o)
+int verify_uptodate(const struct cache_entry *ce,
+		    struct unpack_trees_options *o)
 {
 	if (!o->skip_sparse_checkout && (ce->ce_flags & CE_NEW_SKIP_WORKTREE))
 		return 0;
diff --git a/unpack-trees.h b/unpack-trees.h
index 6c48117b84..41178ada94 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -1,6 +1,7 @@
 #ifndef UNPACK_TREES_H
 #define UNPACK_TREES_H
 
+#include "tree-walk.h"
 #include "string-list.h"
 
 #define MAX_UNPACK_TREES 8
@@ -78,6 +79,9 @@ struct unpack_trees_options {
 extern int unpack_trees(unsigned n, struct tree_desc *t,
 		struct unpack_trees_options *options);
 
+int verify_uptodate(const struct cache_entry *ce,
+		    struct unpack_trees_options *o);
+
 int threeway_merge(const struct cache_entry * const *stages,
 		   struct unpack_trees_options *o);
 int twoway_merge(const struct cache_entry * const *src,

-- 
2.17.0.290.ge988e9ce2a
Previous: Johannes SchindelinNext: Martin Ågren
Message 46 of 78 in “Add directory rename detection to git”
  1. 00/36 Add directory rename detection to gitElijah Newren, Apr 19, 2018
  2. 36/36 merge-recursive: fix check for skipability of working tree updatesElijah Newren, Apr 19, 2018
  3. 29/36 merge-recursive: improve add_cacheinfo error handlingElijah Newren, Apr 19, 2018
  4. 19/36 merge-recursive: check for directory level conflictsElijah Newren, Apr 19, 2018
  5. 07/36 directory rename detection: more involved edge/corner testcasesElijah Newren, Apr 19, 2018
  6. 26/36 merge-recursive: fix remaining directory rename + dirty overwrite casesElijah Newren, Apr 19, 2018
  7. 14/36 merge-recursive: fix leaks of allocated renames and diff_filepairsElijah Newren, Apr 19, 2018
  8. 33/36 merge-recursive: fix was_tracked() to quit lying with some renamed pathsElijah Newren, Apr 19, 2018
  9. Martin ÅgrenApr 19, 2018
  10. Elijah NewrenApr 19, 2018
  11. SZEDER GáborApr 20, 2018
  12. Elijah NewrenApr 20, 2018
  13. 5/36 unpack_trees: fix memory corruption with split_index when src != dstElijah Newren, Apr 21, 2018
  14. Elijah NewrenApr 21, 2018
  15. Duy NguyenApr 22, 2018
  16. Elijah NewrenApr 23, 2018
  17. Duy NguyenApr 23, 2018
  18. Elijah NewrenApr 23, 2018
  19. unpack_trees: fix breakage when o->src_index != o->dst_indexElijah Newren, Apr 24, 2018
  20. Junio C HamanoApr 24, 2018
  21. Junio C HamanoApr 24, 2018
  22. unpack_trees: fix breakage when o->src_index != o->dst_indexElijah Newren, Apr 24, 2018
  23. Duy NguyenApr 29, 2018
  24. Johannes SchindelinApr 29, 2018
  25. Duy NguyenApr 30, 2018
  26. Duy NguyenApr 30, 2018
  27. Elijah NewrenApr 30, 2018
  28. Duy NguyenApr 30, 2018
  29. 24/36 merge-recursive: avoid clobbering untracked files with directory renamesElijah Newren, Apr 19, 2018
  30. 34/36 merge-recursive: fix remainder of was_dirty() to use original indexElijah Newren, Apr 19, 2018
  31. 31/36 merge-recursive: avoid triggering add_cacheinfo error with dirty modElijah Newren, Apr 19, 2018
  32. 20/36 merge-recursive: add computation of collisions due to dir rename & mergingElijah Newren, Apr 19, 2018
  33. 32/36 t6046: testcases checking whether updates can be skipped in a mergeElijah Newren, Apr 19, 2018
  34. SZEDER GáborApr 19, 2018
  35. Elijah NewrenApr 19, 2018
  36. 35/36 merge-recursive: make "Auto-merging" comment show for other mergesElijah Newren, Apr 19, 2018
  37. 23/36 merge-recursive: apply necessary modifications for directory renamesElijah Newren, Apr 19, 2018
  38. 30/36 merge-recursive: move more is_dirty handling to merge_contentElijah Newren, Apr 19, 2018
  39. 28/36 merge-recursive: avoid spurious rename/rename conflict from dir renamesElijah Newren, Apr 19, 2018
  40. 18/36 merge-recursive: add get_directory_renames()Elijah Newren, Apr 19, 2018
  41. SZEDER GáborMay 6, 2018
  42. fixup! merge-recursive: add get_directory_renames()Elijah Newren, May 7, 2018
  43. Johannes SchindelinOct 9, 2019
  44. Elijah NewrenOct 11, 2019
  45. Johannes SchindelinOct 12, 2019
  46. 25/36 merge-recursive: fix overwriting dirty files involved in renamesElijah Newren, Apr 19, 2018
  47. Martin ÅgrenApr 19, 2018
  48. Martin ÅgrenApr 19, 2018
  49. Elijah NewrenApr 19, 2018
  50. 22/36 merge-recursive: when comparing files, don't include treesElijah Newren, Apr 19, 2018
  51. 11/36 directory rename detection: tests for handling overwriting dirty filesElijah Newren, Apr 19, 2018
  52. 06/36 directory rename detection: testcases checking which side did the renameElijah Newren, Apr 19, 2018
  53. 13/36 merge-recursive: introduce new functions to handle rename logicElijah Newren, Apr 19, 2018
  54. 10/36 directory rename detection: tests for handling overwriting untracked filesElijah Newren, Apr 19, 2018
  55. 21/36 merge-recursive: check for file level conflicts then get new nameElijah Newren, Apr 19, 2018
  56. 17/36 merge-recursive: make a helper function for cleanup for handle_renamesElijah Newren, Apr 19, 2018
  57. 16/36 merge-recursive: split out code for determining diff_filepairsElijah Newren, Apr 19, 2018
  58. 15/36 merge-recursive: make !o->detect_rename codepath more obviousElijah Newren, Apr 19, 2018
  59. 27/36 directory rename detection: new testcases showcasing a pair of bugsElijah Newren, Apr 19, 2018
  60. 01/36 directory rename detection: basic testcasesElijah Newren, Apr 19, 2018
  61. 08/36 directory rename detection: testcases exploring possibly suboptimal mergesElijah Newren, Apr 19, 2018
  62. 04/36 directory rename detection: partially renamed directory testcase/discussionElijah Newren, Apr 19, 2018
  63. 09/36 directory rename detection: miscellaneous testcases to complete coverageElijah Newren, Apr 19, 2018
  64. 02/36 directory rename detection: directory splitting testcasesElijah Newren, Apr 19, 2018
  65. 12/36 merge-recursive: move the get_renames() functionElijah Newren, Apr 19, 2018
  66. 03/36 directory rename detection: testcases to avoid taking detection too farElijah Newren, Apr 19, 2018
  67. 05/36 directory rename detection: files/directories in the way of some renamesElijah Newren, Apr 19, 2018
  68. Elijah NewrenApr 19, 2018
  69. Stefan BellerApr 19, 2018
  70. Derrick StoleeApr 19, 2018
  71. Elijah NewrenApr 19, 2018
  72. Junio C HamanoApr 20, 2018
  73. Elijah NewrenApr 23, 2018
  74. 1/2 fixup! merge-recursive: fix was_tracked() to quit lying with some renamed pathsElijah Newren, Apr 24, 2018
  75. 2/2 fixup! t6046: testcases checking whether updates can be skipped in a mergeElijah Newren, Apr 24, 2018
  76. Elijah NewrenApr 23, 2018
  77. Junio C HamanoApr 23, 2018
  78. Elijah NewrenApr 24, 2018

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.