git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: git@vger.kernel.org
Cc: sbeller@google.com, gitster@pobox.com, Elijah Newren <newren@gmail.com>
Subject: [PATCH v4 26/34] merge-recursive: check for file level conflicts then get new name
Date: Tue, 28 Nov 2017 17:42:29 -0800	[thread overview]
Message-ID: <20171129014237.32570-27-newren@gmail.com> (raw)
In-Reply-To: <20171129014237.32570-1-newren@gmail.com>

Before trying to apply directory renames to paths within the given
directories, we want to make sure that there aren't conflicts at the
file level either.  If there aren't any, then get the new name from
any directory renames.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c                   | 174 ++++++++++++++++++++++++++++++++++--
 strbuf.c                            |  16 ++++
 strbuf.h                            |  16 ++++
 t/t6043-merge-rename-directories.sh |   2 +-
 4 files changed, 199 insertions(+), 9 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 9e31baaf33..78f707d0d7 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1508,6 +1508,91 @@ static void remove_hashmap_entries(struct hashmap *dir_renames,
 	string_list_clear(items_to_remove, 0);
 }
 
+/*
+ * See if there is a directory rename for path, and if there are any file
+ * level conflicts for the renamed location.  If there is a rename and
+ * there are no conflicts, return the new name.  Otherwise, return NULL.
+ */
+static char *handle_path_level_conflicts(struct merge_options *o,
+					 const char *path,
+					 struct dir_rename_entry *entry,
+					 struct hashmap *collisions,
+					 struct tree *tree)
+{
+	char *new_path = NULL;
+	struct collision_entry *collision_ent;
+	int clean = 1;
+	struct strbuf collision_paths = STRBUF_INIT;
+
+	/*
+	 * entry has the mapping of old directory name to new directory name
+	 * that we want to apply to path.
+	 */
+	new_path = apply_dir_rename(entry, path);
+
+	if (!new_path) {
+		/* This should only happen when entry->non_unique_new_dir set */
+		if (!entry->non_unique_new_dir)
+			BUG("entry->non_unqiue_dir not set and !new_path");
+		output(o, 1, _("CONFLICT (directory rename split): "
+			       "Unclear where to place %s because directory "
+			       "%s was renamed to multiple other directories, "
+			       "with no destination getting a majority of the "
+			       "files."),
+		       path, entry->dir);
+		clean = 0;
+		return NULL;
+	}
+
+	/*
+	 * The caller needs to have ensured that it has pre-populated
+	 * collisions with all paths that map to new_path.  Do a quick check
+	 * to ensure that's the case.
+	 */
+	collision_ent = collision_find_entry(collisions, new_path);
+	if (collision_ent == NULL)
+		BUG("collision_ent is NULL");
+
+	/*
+	 * Check for one-sided add/add/.../add conflicts, i.e.
+	 * where implicit renames from the other side doing
+	 * directory rename(s) can affect this side of history
+	 * to put multiple paths into the same location.  Warn
+	 * and bail on directory renames for such paths.
+	 */
+	if (collision_ent->reported_already) {
+		clean = 0;
+	} else if (tree_has_path(tree, new_path)) {
+		collision_ent->reported_already = 1;
+		strbuf_add_separated_string_list(&collision_paths, ", ",
+						 &collision_ent->source_files);
+		output(o, 1, _("CONFLICT (implicit dir rename): Existing "
+			       "file/dir at %s in the way of implicit "
+			       "directory rename(s) putting the following "
+			       "path(s) there: %s."),
+		       new_path, collision_paths.buf);
+		clean = 0;
+	} else if (collision_ent->source_files.nr > 1) {
+		collision_ent->reported_already = 1;
+		strbuf_add_separated_string_list(&collision_paths, ", ",
+						 &collision_ent->source_files);
+		output(o, 1, _("CONFLICT (implicit dir rename): Cannot map "
+			       "more than one path to %s; implicit directory "
+			       "renames tried to put these paths there: %s"),
+		       new_path, collision_paths.buf);
+		clean = 0;
+	}
+
+	/* Free memory we no longer need */
+	strbuf_release(&collision_paths);
+	if (!clean && new_path) {
+		free(new_path);
+		return NULL;
+	}
+
+	return new_path;
+}
+
 /*
  * There are a couple things we want to do at the directory level:
  *   1. Check for both sides renaming to the same thing, in order to avoid
@@ -1757,6 +1842,59 @@ static void compute_collisions(struct hashmap *collisions,
 	}
 }
 
+static char *check_for_directory_rename(struct merge_options *o,
+					const char *path,
+					struct tree *tree,
+					struct hashmap *dir_renames,
+					struct hashmap *dir_rename_exclusions,
+					struct hashmap *collisions,
+					int *clean_merge)
+{
+	char *new_path = NULL;
+	struct dir_rename_entry *entry = check_dir_renamed(path, dir_renames);
+	struct dir_rename_entry *oentry = NULL;
+
+	if (!entry)
+		return new_path;
+
+	/*
+	 * This next part is a little weird.  We do not want to do an
+	 * implicit rename into a directory we renamed on our side, because
+	 * that will result in a spurious rename/rename(1to2) conflict.  An
+	 * example:
+	 *   Base commit: dumbdir/afile, otherdir/bfile
+	 *   Side 1:      smrtdir/afile, otherdir/bfile
+	 *   Side 2:      dumbdir/afile, dumbdir/bfile
+	 * Here, while working on Side 1, we could notice that otherdir was
+	 * renamed/merged to dumbdir, and change the diff_filepair for
+	 * otherdir/bfile into a rename into dumbdir/bfile.  However, Side
+	 * 2 will notice the rename from dumbdir to smrtdir, and do the
+	 * transitive rename to move it from dumbdir/bfile to
+	 * smrtdir/bfile.  That gives us bfile in dumbdir vs being in
+	 * smrtdir, a rename/rename(1to2) conflict.  We really just want
+	 * the file to end up in smrtdir.  And the way to achieve that is
+	 * to not let Side1 do the rename to dumbdir, since we know that is
+	 * the source of one of our directory renames.
+	 *
+	 * That's why oentry and dir_rename_exclusions is here.
+	 *
+	 * As it turns out, this also prevents N-way transient rename
+	 * confusion; See testcases 9c and 9d of t6043.
+	 */
+	oentry = dir_rename_find_entry(dir_rename_exclusions, entry->new_dir.buf);
+	if (oentry) {
+		output(o, 1, _("WARNING: Avoiding applying %s -> %s rename "
+			       "to %s, because %s itself was renamed."),
+		       entry->dir, entry->new_dir.buf, path, entry->new_dir.buf);
+	} else {
+		new_path = handle_path_level_conflicts(o, path, entry,
+						       collisions, tree);
+		*clean_merge &= (new_path != NULL);
+	}
+
+	return new_path;
+}
+
 /*
  * Get information of all renames which occurred in 'pairs', making use of
  * any implicit directory renames inferred from the other side of history.
@@ -1767,11 +1905,13 @@ static void compute_collisions(struct hashmap *collisions,
 static struct string_list *get_renames(struct merge_options *o,
 				       struct diff_queue_struct *pairs,
 				       struct hashmap *dir_renames,
+				       struct hashmap *dir_rename_exclusions,
 				       struct tree *tree,
 				       struct tree *o_tree,
 				       struct tree *a_tree,
 				       struct tree *b_tree,
-				       struct string_list *entries)
+				       struct string_list *entries,
+				       int *clean_merge)
 {
 	int i;
 	struct hashmap collisions;
@@ -1786,11 +1926,22 @@ static struct string_list *get_renames(struct merge_options *o,
 		struct string_list_item *item;
 		struct rename *re;
 		struct diff_filepair *pair = pairs->queue[i];
+		char *new_path; /* non-NULL only with directory renames */
 
-		if (pair->status != 'R') {
+		if (pair->status == 'D') {
+			diff_free_filepair(pair);
+			continue;
+		}
+		new_path = check_for_directory_rename(o, pair->two->path, tree,
+						      dir_renames,
+						      dir_rename_exclusions,
+						      &collisions,
+						      clean_merge);
+		if (pair->status != 'R' && !new_path) {
 			diff_free_filepair(pair);
 			continue;
 		}
+
 		re = xmalloc(sizeof(*re));
 		re->processed = 0;
 		re->pair = pair;
@@ -2108,7 +2259,7 @@ static int handle_renames(struct merge_options *o,
 {
 	struct diff_queue_struct *head_pairs, *merge_pairs;
 	struct hashmap *dir_re_head, *dir_re_merge;
-	int clean;
+	int clean = 1;
 
 	ri->head_renames = NULL;
 	ri->merge_renames = NULL;
@@ -2127,13 +2278,20 @@ static int handle_renames(struct merge_options *o,
 					 dir_re_merge, merge);
 
 	ri->head_renames  = get_renames(o, head_pairs,
-					dir_re_merge, head,
-					common, head, merge, entries);
+					dir_re_merge, dir_re_head, head,
+					common, head, merge, entries,
+					&clean);
+	if (clean < 0)
+		goto cleanup;
 	ri->merge_renames = get_renames(o, merge_pairs,
-					dir_re_head, merge,
-					 common, head, merge, entries);
-	clean = process_renames(o, ri->head_renames, ri->merge_renames);
+					dir_re_head, dir_re_merge, merge,
+					common, head, merge, entries,
+					&clean);
+	if (clean < 0)
+		goto cleanup;
+	clean &= process_renames(o, ri->head_renames, ri->merge_renames);
 
+cleanup:
 	/*
 	 * Some cleanup is deferred until cleanup_renames() because the
 	 * data structures are still needed and referenced in
diff --git a/strbuf.c b/strbuf.c
index 323c49ceb3..6ae384f9cd 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "refs.h"
+#include "string-list.h"
 #include "utf8.h"
 
 int starts_with(const char *str, const char *prefix)
@@ -141,6 +142,21 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,
 	return ret;
 }
 
+void strbuf_add_separated_string_list(struct strbuf *str,
+				      const char *sep,
+				      struct string_list *slist)
+{
+	struct string_list_item *item;
+	int sep_needed = 0;
+
+	for_each_string_list_item(item, slist) {
+		if (sep_needed)
+			strbuf_addstr(str, sep);
+		strbuf_addstr(str, item->string);
+		sep_needed = 1;
+	}
+}
+
 void strbuf_list_free(struct strbuf **sbs)
 {
 	struct strbuf **s = sbs;
diff --git a/strbuf.h b/strbuf.h
index 0a74acb236..fd6d787a7b 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -1,6 +1,8 @@
 #ifndef STRBUF_H
 #define STRBUF_H
 
+struct string_list;
+
 /**
  * strbuf's are meant to be used with all the usual C string and memory
  * APIs. Given that the length of the buffer is known, it's often better to
@@ -537,6 +539,20 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,
 	return strbuf_split_max(sb, terminator, 0);
 }
 
+/*
+ * Adds all strings of a string list to the strbuf, separated by the given
+ * separator.  For example, if sep is
+ *   ', '
+ * and slist contains
+ *   ['element1', 'element2', ..., 'elementN'],
+ * then write:
+ *   'element1, element2, ..., elementN'
+ * to str.  If only one element, just write "element1" to str.
+ */
+extern void strbuf_add_separated_string_list(struct strbuf *str,
+					     const char *sep,
+					     struct string_list *slist);
+
 /**
  * Free a NULL-terminated list of strbufs (for example, the return
  * values of the strbuf_split*() functions).
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
index 0644b95ec5..836e57c0a3 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -477,7 +477,7 @@ test_expect_success '2a-setup: Directory split into two on one side, with equal
 	)
 '
 
-test_expect_failure '2a-check: Directory split into two on one side, with equal numbers of paths' '
+test_expect_success '2a-check: Directory split into two on one side, with equal numbers of paths' '
 	(
 		cd 2a &&
 
-- 
2.15.0.408.g850bc54b15


  parent reply	other threads:[~2017-11-29  1:43 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-29  1:42 [PATCH v4 00/34] Add directory rename detection to git Elijah Newren
2017-11-29  1:42 ` [PATCH v4 01/34] Tighten and correct a few testcases for merging and cherry-picking Elijah Newren
2017-11-29  1:42 ` [PATCH v4 02/34] merge-recursive: fix logic ordering issue Elijah Newren
2017-11-29  1:42 ` [PATCH v4 03/34] merge-recursive: add explanation for src_entry and dst_entry Elijah Newren
2017-11-29  1:42 ` [PATCH v4 04/34] directory rename detection: basic testcases Elijah Newren
2017-11-29  1:42 ` [PATCH v4 05/34] directory rename detection: directory splitting testcases Elijah Newren
2017-11-29  1:42 ` [PATCH v4 06/34] directory rename detection: testcases to avoid taking detection too far Elijah Newren
2017-11-29  1:42 ` [PATCH v4 07/34] directory rename detection: partially renamed directory testcase/discussion Elijah Newren
2017-11-29  1:42 ` [PATCH v4 08/34] directory rename detection: files/directories in the way of some renames Elijah Newren
2017-11-29  1:42 ` [PATCH v4 09/34] directory rename detection: testcases checking which side did the rename Elijah Newren
2017-11-29  1:42 ` [PATCH v4 10/34] directory rename detection: more involved edge/corner testcases Elijah Newren
2017-11-29  1:42 ` [PATCH v4 11/34] directory rename detection: testcases exploring possibly suboptimal merges Elijah Newren
2017-11-29  1:42 ` [PATCH v4 12/34] directory rename detection: miscellaneous testcases to complete coverage Elijah Newren
2017-11-29  1:42 ` [PATCH v4 13/34] directory rename detection: tests for handling overwriting untracked files Elijah Newren
2017-11-29  1:42 ` [PATCH v4 14/34] directory rename detection: tests for handling overwriting dirty files Elijah Newren
2017-11-29  1:42 ` [PATCH v4 15/34] merge-recursive: move the get_renames() function Elijah Newren
2017-11-29  1:42 ` [PATCH v4 16/34] merge-recursive: introduce new functions to handle rename logic Elijah Newren
2017-11-29  1:42 ` [PATCH v4 17/34] merge-recursive: fix leaks of allocated renames and diff_filepairs Elijah Newren
2017-11-29  1:42 ` [PATCH v4 18/34] merge-recursive: make !o->detect_rename codepath more obvious Elijah Newren
2017-11-29  1:42 ` [PATCH v4 19/34] merge-recursive: split out code for determining diff_filepairs Elijah Newren
2017-11-29  1:42 ` [PATCH v4 20/34] merge-recursive: add a new hashmap for storing directory renames Elijah Newren
2017-11-29  1:42 ` [PATCH v4 21/34] merge-recursive: make a helper function for cleanup for handle_renames Elijah Newren
2017-11-29  1:42 ` [PATCH v4 22/34] merge-recursive: add get_directory_renames() Elijah Newren
2017-11-29  1:42 ` [PATCH v4 23/34] merge-recursive: check for directory level conflicts Elijah Newren
2017-11-29  1:42 ` [PATCH v4 24/34] merge-recursive: add a new hashmap for storing file collisions Elijah Newren
2017-11-29  1:42 ` [PATCH v4 25/34] merge-recursive: add computation of collisions due to dir rename & merging Elijah Newren
2017-11-29  1:42 ` Elijah Newren [this message]
2017-11-29  1:42 ` [PATCH v4 27/34] merge-recursive: when comparing files, don't include trees Elijah Newren
2017-11-29  1:42 ` [PATCH v4 28/34] merge-recursive: apply necessary modifications for directory renames Elijah Newren
2017-11-29  1:42 ` [PATCH v4 29/34] merge-recursive: avoid clobbering untracked files with " Elijah Newren
2017-11-29  1:42 ` [PATCH v4 30/34] merge-recursive: fix overwriting dirty files involved in renames Elijah Newren
2017-11-29  1:42 ` [PATCH v4 31/34] merge-recursive: fix remaining directory rename + dirty overwrite cases Elijah Newren
2017-11-29  1:42 ` [PATCH v4 32/34] directory rename detection: new testcases showcasing a pair of bugs Elijah Newren
2017-11-29  1:42 ` [PATCH v4 33/34] merge-recursive: avoid spurious rename/rename conflict from dir renames Elijah Newren
2017-11-29  1:42 ` [PATCH v4 34/34] merge-recursive: ensure we write updates for directory-renamed file Elijah Newren
2017-12-13  1:06 ` [PATCH v4 00/34] Add directory rename detection to git Junio C Hamano
2017-12-13  2:01   ` Junio C Hamano
2017-12-13 15:38     ` Elijah Newren
2017-12-13 18:15   ` Ramsay Jones
2017-12-13 19:05     ` 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=20171129014237.32570-27-newren@gmail.com \
    --to=newren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sbeller@google.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).