From 9ba915577ff8bd0ef4677d7d6b5de857be52a108 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:57:59 -0700 Subject: merge-recursive: move the get_renames() function Move this function so it can re-use some others (without either moving all of them or adding an annoying split between function declarations and definitions). Cheat slightly by adding a blank line for readability, and in order to silence checkpatch.pl. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 139 +++++++++++++++++++++++++++--------------------------- 1 file changed, 70 insertions(+), 69 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 0c0d48624d..973b6e2985 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -537,75 +537,6 @@ struct rename { unsigned processed:1; }; -/* - * Get information of all renames which occurred between 'o_tree' and - * 'tree'. We need the three trees in the merge ('o_tree', 'a_tree' and - * 'b_tree') to be able to associate the correct cache entries with - * the rename information. 'tree' is always equal to either a_tree or b_tree. - */ -static struct string_list *get_renames(struct merge_options *o, - struct tree *tree, - struct tree *o_tree, - struct tree *a_tree, - struct tree *b_tree, - struct string_list *entries) -{ - int i; - struct string_list *renames; - struct diff_options opts; - - renames = xcalloc(1, sizeof(struct string_list)); - if (!o->detect_rename) - return renames; - - diff_setup(&opts); - opts.flags.recursive = 1; - opts.flags.rename_empty = 0; - opts.detect_rename = DIFF_DETECT_RENAME; - opts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit : - o->diff_rename_limit >= 0 ? o->diff_rename_limit : - 1000; - opts.rename_score = o->rename_score; - opts.show_rename_progress = o->show_rename_progress; - opts.output_format = DIFF_FORMAT_NO_OUTPUT; - diff_setup_done(&opts); - diff_tree_oid(&o_tree->object.oid, &tree->object.oid, "", &opts); - diffcore_std(&opts); - if (opts.needed_rename_limit > o->needed_rename_limit) - o->needed_rename_limit = opts.needed_rename_limit; - for (i = 0; i < diff_queued_diff.nr; ++i) { - struct string_list_item *item; - struct rename *re; - struct diff_filepair *pair = diff_queued_diff.queue[i]; - if (pair->status != 'R') { - diff_free_filepair(pair); - continue; - } - re = xmalloc(sizeof(*re)); - re->processed = 0; - re->pair = pair; - item = string_list_lookup(entries, re->pair->one->path); - if (!item) - re->src_entry = insert_stage_data(re->pair->one->path, - o_tree, a_tree, b_tree, entries); - else - re->src_entry = item->util; - - item = string_list_lookup(entries, re->pair->two->path); - if (!item) - re->dst_entry = insert_stage_data(re->pair->two->path, - o_tree, a_tree, b_tree, entries); - else - re->dst_entry = item->util; - item = string_list_insert(renames, pair->one->path); - item->util = re; - } - opts.output_format = DIFF_FORMAT_NO_OUTPUT; - diff_queued_diff.nr = 0; - diff_flush(&opts); - return renames; -} - static int update_stages(struct merge_options *opt, const char *path, const struct diff_filespec *o, const struct diff_filespec *a, @@ -1390,6 +1321,76 @@ static int conflict_rename_rename_2to1(struct merge_options *o, return ret; } +/* + * Get information of all renames which occurred between 'o_tree' and + * 'tree'. We need the three trees in the merge ('o_tree', 'a_tree' and + * 'b_tree') to be able to associate the correct cache entries with + * the rename information. 'tree' is always equal to either a_tree or b_tree. + */ +static struct string_list *get_renames(struct merge_options *o, + struct tree *tree, + struct tree *o_tree, + struct tree *a_tree, + struct tree *b_tree, + struct string_list *entries) +{ + int i; + struct string_list *renames; + struct diff_options opts; + + renames = xcalloc(1, sizeof(struct string_list)); + if (!o->detect_rename) + return renames; + + diff_setup(&opts); + opts.flags.recursive = 1; + opts.flags.rename_empty = 0; + opts.detect_rename = DIFF_DETECT_RENAME; + opts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit : + o->diff_rename_limit >= 0 ? o->diff_rename_limit : + 1000; + opts.rename_score = o->rename_score; + opts.show_rename_progress = o->show_rename_progress; + opts.output_format = DIFF_FORMAT_NO_OUTPUT; + diff_setup_done(&opts); + diff_tree_oid(&o_tree->object.oid, &tree->object.oid, "", &opts); + diffcore_std(&opts); + if (opts.needed_rename_limit > o->needed_rename_limit) + o->needed_rename_limit = opts.needed_rename_limit; + for (i = 0; i < diff_queued_diff.nr; ++i) { + struct string_list_item *item; + struct rename *re; + struct diff_filepair *pair = diff_queued_diff.queue[i]; + + if (pair->status != 'R') { + diff_free_filepair(pair); + continue; + } + re = xmalloc(sizeof(*re)); + re->processed = 0; + re->pair = pair; + item = string_list_lookup(entries, re->pair->one->path); + if (!item) + re->src_entry = insert_stage_data(re->pair->one->path, + o_tree, a_tree, b_tree, entries); + else + re->src_entry = item->util; + + item = string_list_lookup(entries, re->pair->two->path); + if (!item) + re->dst_entry = insert_stage_data(re->pair->two->path, + o_tree, a_tree, b_tree, entries); + else + re->dst_entry = item->util; + item = string_list_insert(renames, pair->one->path); + item->util = re; + } + opts.output_format = DIFF_FORMAT_NO_OUTPUT; + diff_queued_diff.nr = 0; + diff_flush(&opts); + return renames; +} + static int process_renames(struct merge_options *o, struct string_list *a_renames, struct string_list *b_renames) -- cgit 1.2.3-korg From f172589e599fca939ab6bb5f2dae4d58f6d201fc Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:00 -0700 Subject: merge-recursive: introduce new functions to handle rename logic The amount of logic in merge_trees() relative to renames was just a few lines, but split it out into new handle_renames() and cleanup_renames() functions to prepare for additional logic to be added to each. No code or logic changes, just a new place to put stuff for when the rename detection gains additional checks. Note that process_renames() records pointers to various information (such as diff_filepairs) into rename_conflict_info structs. Even though the rename string_lists are not directly used once handle_renames() completes, we should not immediately free the lists at the end of that function because they store the information referenced in the rename_conflict_info, which is used later in process_entry(). Thus the reason for a separate cleanup_renames(). Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 43 +++++++++++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 10 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 973b6e2985..40e142efdb 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1646,6 +1646,32 @@ static int process_renames(struct merge_options *o, return clean_merge; } +struct rename_info { + struct string_list *head_renames; + struct string_list *merge_renames; +}; + +static int handle_renames(struct merge_options *o, + struct tree *common, + struct tree *head, + struct tree *merge, + struct string_list *entries, + struct rename_info *ri) +{ + ri->head_renames = get_renames(o, head, common, head, merge, entries); + ri->merge_renames = get_renames(o, merge, common, head, merge, entries); + return process_renames(o, ri->head_renames, ri->merge_renames); +} + +static void cleanup_renames(struct rename_info *re_info) +{ + string_list_clear(re_info->head_renames, 0); + string_list_clear(re_info->merge_renames, 0); + + free(re_info->head_renames); + free(re_info->merge_renames); +} + static struct object_id *stage_oid(const struct object_id *oid, unsigned mode) { return (is_null_oid(oid) || mode == 0) ? NULL: (struct object_id *)oid; @@ -2005,7 +2031,8 @@ int merge_trees(struct merge_options *o, } if (unmerged_cache()) { - struct string_list *entries, *re_head, *re_merge; + struct string_list *entries; + struct rename_info re_info; int i; /* * Only need the hashmap while processing entries, so @@ -2019,9 +2046,8 @@ int merge_trees(struct merge_options *o, get_files_dirs(o, merge); entries = get_unmerged(); - re_head = get_renames(o, head, common, head, merge, entries); - re_merge = get_renames(o, merge, common, head, merge, entries); - clean = process_renames(o, re_head, re_merge); + clean = handle_renames(o, common, head, merge, entries, + &re_info); record_df_conflict_files(o, entries); if (clean < 0) goto cleanup; @@ -2046,16 +2072,13 @@ int merge_trees(struct merge_options *o, } cleanup: - string_list_clear(re_merge, 0); - string_list_clear(re_head, 0); + cleanup_renames(&re_info); + string_list_clear(entries, 1); + free(entries); hashmap_free(&o->current_file_dir_set, 1); - free(re_merge); - free(re_head); - free(entries); - if (clean < 0) return clean; } -- cgit 1.2.3-korg From 9cfee25a823d972250409b5c8bdfd91d1cdf7edb Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:01 -0700 Subject: merge-recursive: fix leaks of allocated renames and diff_filepairs get_renames() has always zero'ed out diff_queued_diff.nr while only manually free'ing diff_filepairs that did not correspond to renames. Further, it allocated struct renames that were tucked away in the return string_list. Make sure all of these are deallocated when we are done with them. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 40e142efdb..fc96653f63 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1663,13 +1663,23 @@ static int handle_renames(struct merge_options *o, return process_renames(o, ri->head_renames, ri->merge_renames); } -static void cleanup_renames(struct rename_info *re_info) +static void cleanup_rename(struct string_list *rename) { - string_list_clear(re_info->head_renames, 0); - string_list_clear(re_info->merge_renames, 0); + const struct rename *re; + int i; - free(re_info->head_renames); - free(re_info->merge_renames); + for (i = 0; i < rename->nr; i++) { + re = rename->items[i].util; + diff_free_filepair(re->pair); + } + string_list_clear(rename, 1); + free(rename); +} + +static void cleanup_renames(struct rename_info *re_info) +{ + cleanup_rename(re_info->head_renames); + cleanup_rename(re_info->merge_renames); } static struct object_id *stage_oid(const struct object_id *oid, unsigned mode) -- cgit 1.2.3-korg From 3992ff0c4caf925373b61aa17ab507207bf7e645 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:02 -0700 Subject: merge-recursive: make !o->detect_rename codepath more obvious Previously, if !o->detect_rename then get_renames() would return an empty string_list, and then process_renames() would have nothing to iterate over. It seems more straightforward to simply avoid calling either function in that case. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index fc96653f63..5da60b9516 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1339,8 +1339,6 @@ static struct string_list *get_renames(struct merge_options *o, struct diff_options opts; renames = xcalloc(1, sizeof(struct string_list)); - if (!o->detect_rename) - return renames; diff_setup(&opts); opts.flags.recursive = 1; @@ -1658,6 +1656,12 @@ static int handle_renames(struct merge_options *o, struct string_list *entries, struct rename_info *ri) { + ri->head_renames = NULL; + ri->merge_renames = NULL; + + if (!o->detect_rename) + return 1; + ri->head_renames = get_renames(o, head, common, head, merge, entries); ri->merge_renames = get_renames(o, merge, common, head, merge, entries); return process_renames(o, ri->head_renames, ri->merge_renames); @@ -1668,6 +1672,9 @@ static void cleanup_rename(struct string_list *rename) const struct rename *re; int i; + if (rename == NULL) + return; + for (i = 0; i < rename->nr; i++) { re = rename->items[i].util; diff_free_filepair(re->pair); -- cgit 1.2.3-korg From e5257b2a0d72010cea8150a520347a206394efd0 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:03 -0700 Subject: merge-recursive: split out code for determining diff_filepairs Create a new function, get_diffpairs() to compute the diff_filepairs between two trees. While these are currently only used in get_renames(), I want them to be available to some new functions. No actual logic changes yet. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 84 ++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 62 insertions(+), 22 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 5da60b9516..55a8ace948 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1322,24 +1322,15 @@ static int conflict_rename_rename_2to1(struct merge_options *o, } /* - * Get information of all renames which occurred between 'o_tree' and - * 'tree'. We need the three trees in the merge ('o_tree', 'a_tree' and - * 'b_tree') to be able to associate the correct cache entries with - * the rename information. 'tree' is always equal to either a_tree or b_tree. + * Get the diff_filepairs changed between o_tree and tree. */ -static struct string_list *get_renames(struct merge_options *o, - struct tree *tree, - struct tree *o_tree, - struct tree *a_tree, - struct tree *b_tree, - struct string_list *entries) +static struct diff_queue_struct *get_diffpairs(struct merge_options *o, + struct tree *o_tree, + struct tree *tree) { - int i; - struct string_list *renames; + struct diff_queue_struct *ret; struct diff_options opts; - renames = xcalloc(1, sizeof(struct string_list)); - diff_setup(&opts); opts.flags.recursive = 1; opts.flags.rename_empty = 0; @@ -1355,10 +1346,41 @@ static struct string_list *get_renames(struct merge_options *o, diffcore_std(&opts); if (opts.needed_rename_limit > o->needed_rename_limit) o->needed_rename_limit = opts.needed_rename_limit; - for (i = 0; i < diff_queued_diff.nr; ++i) { + + ret = xmalloc(sizeof(*ret)); + *ret = diff_queued_diff; + + opts.output_format = DIFF_FORMAT_NO_OUTPUT; + diff_queued_diff.nr = 0; + diff_queued_diff.queue = NULL; + diff_flush(&opts); + return ret; +} + +/* + * Get information of all renames which occurred in 'pairs', making use of + * any implicit directory renames inferred from the other side of history. + * We need the three trees in the merge ('o_tree', 'a_tree' and 'b_tree') + * to be able to associate the correct cache entries with the rename + * information; tree is always equal to either a_tree or b_tree. + */ +static struct string_list *get_renames(struct merge_options *o, + struct diff_queue_struct *pairs, + struct tree *tree, + struct tree *o_tree, + struct tree *a_tree, + struct tree *b_tree, + struct string_list *entries) +{ + int i; + struct string_list *renames; + + renames = xcalloc(1, sizeof(struct string_list)); + + for (i = 0; i < pairs->nr; ++i) { struct string_list_item *item; struct rename *re; - struct diff_filepair *pair = diff_queued_diff.queue[i]; + struct diff_filepair *pair = pairs->queue[i]; if (pair->status != 'R') { diff_free_filepair(pair); @@ -1383,9 +1405,6 @@ static struct string_list *get_renames(struct merge_options *o, item = string_list_insert(renames, pair->one->path); item->util = re; } - opts.output_format = DIFF_FORMAT_NO_OUTPUT; - diff_queued_diff.nr = 0; - diff_flush(&opts); return renames; } @@ -1656,15 +1675,36 @@ static int handle_renames(struct merge_options *o, struct string_list *entries, struct rename_info *ri) { + struct diff_queue_struct *head_pairs, *merge_pairs; + int clean; + ri->head_renames = NULL; ri->merge_renames = NULL; if (!o->detect_rename) return 1; - ri->head_renames = get_renames(o, head, common, head, merge, entries); - ri->merge_renames = get_renames(o, merge, common, head, merge, entries); - return process_renames(o, ri->head_renames, ri->merge_renames); + head_pairs = get_diffpairs(o, common, head); + merge_pairs = get_diffpairs(o, common, merge); + + ri->head_renames = get_renames(o, head_pairs, head, + common, head, merge, entries); + ri->merge_renames = get_renames(o, merge_pairs, merge, + common, head, merge, entries); + clean = process_renames(o, ri->head_renames, ri->merge_renames); + + /* + * Some cleanup is deferred until cleanup_renames() because the + * data structures are still needed and referenced in + * process_entry(). But there are a few things we can free now. + */ + + free(head_pairs->queue); + free(head_pairs); + free(merge_pairs->queue); + free(merge_pairs); + + return clean; } static void cleanup_rename(struct string_list *rename) -- cgit 1.2.3-korg From ffc16c490ad533bc07e6d8ec0226b426166e8442 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:04 -0700 Subject: merge-recursive: make a helper function for cleanup for handle_renames In anticipation of more involved cleanup to come, make a helper function for doing the cleanup at the end of handle_renames. Rename the already existing cleanup_rename[s]() to final_cleanup_rename[s](), name the new helper initial_cleanup_rename(), and leave the big comment in the code about why we can't do all the cleanup at once. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 55a8ace948..30894c1cc7 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1668,6 +1668,12 @@ struct rename_info { struct string_list *merge_renames; }; +static void initial_cleanup_rename(struct diff_queue_struct *pairs) +{ + free(pairs->queue); + free(pairs); +} + static int handle_renames(struct merge_options *o, struct tree *common, struct tree *head, @@ -1698,16 +1704,13 @@ static int handle_renames(struct merge_options *o, * data structures are still needed and referenced in * process_entry(). But there are a few things we can free now. */ - - free(head_pairs->queue); - free(head_pairs); - free(merge_pairs->queue); - free(merge_pairs); + initial_cleanup_rename(head_pairs); + initial_cleanup_rename(merge_pairs); return clean; } -static void cleanup_rename(struct string_list *rename) +static void final_cleanup_rename(struct string_list *rename) { const struct rename *re; int i; @@ -1723,10 +1726,10 @@ static void cleanup_rename(struct string_list *rename) free(rename); } -static void cleanup_renames(struct rename_info *re_info) +static void final_cleanup_renames(struct rename_info *re_info) { - cleanup_rename(re_info->head_renames); - cleanup_rename(re_info->merge_renames); + final_cleanup_rename(re_info->head_renames); + final_cleanup_rename(re_info->merge_renames); } static struct object_id *stage_oid(const struct object_id *oid, unsigned mode) @@ -2129,7 +2132,7 @@ int merge_trees(struct merge_options *o, } cleanup: - cleanup_renames(&re_info); + final_cleanup_renames(&re_info); string_list_clear(entries, 1); free(entries); -- cgit 1.2.3-korg From 7fe40b88efc721bfcbe69f00f950eb9f7a9cd236 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:05 -0700 Subject: merge-recursive: add get_directory_renames() This populates a set of directory renames for us. The set of directory renames is not yet used, but will be in subsequent commits. Note that the use of a string_list for possible_new_dirs in the new dir_rename_entry struct implies an O(n^2) algorithm; however, in practice I expect the number of distinct directories that files were renamed into from a single original directory to be O(1). My guess is that n has a mode of 1 and a mean of less than 2, so, for now, string_list seems good enough for possible_new_dirs. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 224 +++++++++++++++++++++++++++++++++++++++++++++++++++++- merge-recursive.h | 18 +++++ 2 files changed, 239 insertions(+), 3 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 30894c1cc7..a1a9e62a8d 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -49,6 +49,44 @@ static unsigned int path_hash(const char *path) return ignore_case ? strihash(path) : strhash(path); } +static struct dir_rename_entry *dir_rename_find_entry(struct hashmap *hashmap, + char *dir) +{ + struct dir_rename_entry key; + + if (dir == NULL) + return NULL; + hashmap_entry_init(&key, strhash(dir)); + key.dir = dir; + return hashmap_get(hashmap, &key, NULL); +} + +static int dir_rename_cmp(const void *unused_cmp_data, + const void *entry, + const void *entry_or_key, + const void *unused_keydata) +{ + const struct dir_rename_entry *e1 = entry; + const struct dir_rename_entry *e2 = entry_or_key; + + return strcmp(e1->dir, e2->dir); +} + +static void dir_rename_init(struct hashmap *map) +{ + hashmap_init(map, dir_rename_cmp, NULL, 0); +} + +static void dir_rename_entry_init(struct dir_rename_entry *entry, + char *directory) +{ + hashmap_entry_init(entry, strhash(directory)); + entry->dir = directory; + entry->non_unique_new_dir = 0; + strbuf_init(&entry->new_dir, 0); + string_list_init(&entry->possible_new_dirs, 0); +} + static void flush_output(struct merge_options *o) { if (o->buffer_output < 2 && o->obuf.len) { @@ -1357,6 +1395,169 @@ static struct diff_queue_struct *get_diffpairs(struct merge_options *o, return ret; } +static void get_renamed_dir_portion(const char *old_path, const char *new_path, + char **old_dir, char **new_dir) +{ + char *end_of_old, *end_of_new; + int old_len, new_len; + + *old_dir = NULL; + *new_dir = NULL; + + /* + * For + * "a/b/c/d/e/foo.c" -> "a/b/some/thing/else/e/foo.c" + * the "e/foo.c" part is the same, we just want to know that + * "a/b/c/d" was renamed to "a/b/some/thing/else" + * so, for this example, this function returns "a/b/c/d" in + * *old_dir and "a/b/some/thing/else" in *new_dir. + * + * Also, if the basename of the file changed, we don't care. We + * want to know which portion of the directory, if any, changed. + */ + end_of_old = strrchr(old_path, '/'); + end_of_new = strrchr(new_path, '/'); + + if (end_of_old == NULL || end_of_new == NULL) + return; + while (*--end_of_new == *--end_of_old && + end_of_old != old_path && + end_of_new != new_path) + ; /* Do nothing; all in the while loop */ + /* + * We've found the first non-matching character in the directory + * paths. That means the current directory we were comparing + * represents the rename. Move end_of_old and end_of_new back + * to the full directory name. + */ + if (*end_of_old == '/') + end_of_old++; + if (*end_of_old != '/') + end_of_new++; + end_of_old = strchr(end_of_old, '/'); + end_of_new = strchr(end_of_new, '/'); + + /* + * It may have been the case that old_path and new_path were the same + * directory all along. Don't claim a rename if they're the same. + */ + old_len = end_of_old - old_path; + new_len = end_of_new - new_path; + + if (old_len != new_len || strncmp(old_path, new_path, old_len)) { + *old_dir = xstrndup(old_path, old_len); + *new_dir = xstrndup(new_path, new_len); + } +} + +static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs, + struct tree *tree) +{ + struct hashmap *dir_renames; + struct hashmap_iter iter; + struct dir_rename_entry *entry; + int i; + + /* + * Typically, we think of a directory rename as all files from a + * certain directory being moved to a target directory. However, + * what if someone first moved two files from the original + * directory in one commit, and then renamed the directory + * somewhere else in a later commit? At merge time, we just know + * that files from the original directory went to two different + * places, and that the bulk of them ended up in the same place. + * We want each directory rename to represent where the bulk of the + * files from that directory end up; this function exists to find + * where the bulk of the files went. + * + * The first loop below simply iterates through the list of file + * renames, finding out how often each directory rename pair + * possibility occurs. + */ + dir_renames = xmalloc(sizeof(*dir_renames)); + dir_rename_init(dir_renames); + for (i = 0; i < pairs->nr; ++i) { + struct string_list_item *item; + int *count; + struct diff_filepair *pair = pairs->queue[i]; + char *old_dir, *new_dir; + + /* File not part of directory rename if it wasn't renamed */ + if (pair->status != 'R') + continue; + + get_renamed_dir_portion(pair->one->path, pair->two->path, + &old_dir, &new_dir); + if (!old_dir) + /* Directory didn't change at all; ignore this one. */ + continue; + + entry = dir_rename_find_entry(dir_renames, old_dir); + if (!entry) { + entry = xmalloc(sizeof(*entry)); + dir_rename_entry_init(entry, old_dir); + hashmap_put(dir_renames, entry); + } else { + free(old_dir); + } + item = string_list_lookup(&entry->possible_new_dirs, new_dir); + if (!item) { + item = string_list_insert(&entry->possible_new_dirs, + new_dir); + item->util = xcalloc(1, sizeof(int)); + } else { + free(new_dir); + } + count = item->util; + *count += 1; + } + + /* + * For each directory with files moved out of it, we find out which + * target directory received the most files so we can declare it to + * be the "winning" target location for the directory rename. This + * winner gets recorded in new_dir. If there is no winner + * (multiple target directories received the same number of files), + * we set non_unique_new_dir. Once we've determined the winner (or + * that there is no winner), we no longer need possible_new_dirs. + */ + hashmap_iter_init(dir_renames, &iter); + while ((entry = hashmap_iter_next(&iter))) { + int max = 0; + int bad_max = 0; + char *best = NULL; + + for (i = 0; i < entry->possible_new_dirs.nr; i++) { + int *count = entry->possible_new_dirs.items[i].util; + + if (*count == max) + bad_max = max; + else if (*count > max) { + max = *count; + best = entry->possible_new_dirs.items[i].string; + } + } + if (bad_max == max) + entry->non_unique_new_dir = 1; + else { + assert(entry->new_dir.len == 0); + strbuf_addstr(&entry->new_dir, best); + } + /* + * The relevant directory sub-portion of the original full + * filepaths were xstrndup'ed before inserting into + * possible_new_dirs, and instead of manually iterating the + * list and free'ing each, just lie and tell + * possible_new_dirs that it did the strdup'ing so that it + * will free them for us. + */ + entry->possible_new_dirs.strdup_strings = 1; + string_list_clear(&entry->possible_new_dirs, 1); + } + + return dir_renames; +} + /* * Get information of all renames which occurred in 'pairs', making use of * any implicit directory renames inferred from the other side of history. @@ -1668,8 +1869,21 @@ struct rename_info { struct string_list *merge_renames; }; -static void initial_cleanup_rename(struct diff_queue_struct *pairs) +static void initial_cleanup_rename(struct diff_queue_struct *pairs, + struct hashmap *dir_renames) { + struct hashmap_iter iter; + struct dir_rename_entry *e; + + hashmap_iter_init(dir_renames, &iter); + while ((e = hashmap_iter_next(&iter))) { + free(e->dir); + strbuf_release(&e->new_dir); + /* possible_new_dirs already cleared in get_directory_renames */ + } + hashmap_free(dir_renames, 1); + free(dir_renames); + free(pairs->queue); free(pairs); } @@ -1682,6 +1896,7 @@ static int handle_renames(struct merge_options *o, struct rename_info *ri) { struct diff_queue_struct *head_pairs, *merge_pairs; + struct hashmap *dir_re_head, *dir_re_merge; int clean; ri->head_renames = NULL; @@ -1693,6 +1908,9 @@ static int handle_renames(struct merge_options *o, head_pairs = get_diffpairs(o, common, head); merge_pairs = get_diffpairs(o, common, merge); + dir_re_head = get_directory_renames(head_pairs, head); + dir_re_merge = get_directory_renames(merge_pairs, merge); + ri->head_renames = get_renames(o, head_pairs, head, common, head, merge, entries); ri->merge_renames = get_renames(o, merge_pairs, merge, @@ -1704,8 +1922,8 @@ static int handle_renames(struct merge_options *o, * data structures are still needed and referenced in * process_entry(). But there are a few things we can free now. */ - initial_cleanup_rename(head_pairs); - initial_cleanup_rename(merge_pairs); + initial_cleanup_rename(head_pairs, dir_re_head); + initial_cleanup_rename(merge_pairs, dir_re_merge); return clean; } diff --git a/merge-recursive.h b/merge-recursive.h index 80d69d1401..fe64c78de4 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -29,6 +29,24 @@ struct merge_options { struct string_list df_conflict_file_set; }; +/* + * For dir_rename_entry, directory names are stored as a full path from the + * toplevel of the repository and do not include a trailing '/'. Also: + * + * dir: original name of directory being renamed + * non_unique_new_dir: if true, could not determine new_dir + * new_dir: final name of directory being renamed + * possible_new_dirs: temporary used to help determine new_dir; see comments + * in get_directory_renames() for details + */ +struct dir_rename_entry { + struct hashmap_entry ent; /* must be the first member! */ + char *dir; + unsigned non_unique_new_dir:1; + struct strbuf new_dir; + struct string_list possible_new_dirs; +}; + /* merge_trees() but with recursive ancestor consolidation */ int merge_recursive(struct merge_options *o, struct commit *h1, -- cgit 1.2.3-korg From 96e7ffbdc31d9e02e2548df5d9a9701e5a83651d Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:06 -0700 Subject: merge-recursive: check for directory level conflicts Before trying to apply directory renames to paths within the given directories, we want to make sure that there aren't conflicts at the directory level. There will be additional checks at the individual file level too, which will be added later. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index a1a9e62a8d..a67bb58da7 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1395,6 +1395,15 @@ static struct diff_queue_struct *get_diffpairs(struct merge_options *o, return ret; } +static int tree_has_path(struct tree *tree, const char *path) +{ + struct object_id hashy; + unsigned int mode_o; + + return !get_tree_entry(&tree->object.oid, path, + &hashy, &mode_o); +} + static void get_renamed_dir_portion(const char *old_path, const char *new_path, char **old_dir, char **new_dir) { @@ -1450,6 +1459,112 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path, } } +static void remove_hashmap_entries(struct hashmap *dir_renames, + struct string_list *items_to_remove) +{ + int i; + struct dir_rename_entry *entry; + + for (i = 0; i < items_to_remove->nr; i++) { + entry = items_to_remove->items[i].util; + hashmap_remove(dir_renames, entry, NULL); + } + string_list_clear(items_to_remove, 0); +} + +/* + * 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 + * implicit renaming of files that should be left in place. (See + * testcase 6b in t6043 for details.) + * 2. Prune directory renames if there are still files left in the + * the original directory. These represent a partial directory rename, + * i.e. a rename where only some of the files within the directory + * were renamed elsewhere. (Technically, this could be done earlier + * in get_directory_renames(), except that would prevent us from + * doing the previous check and thus failing testcase 6b.) + * 3. Check for rename/rename(1to2) conflicts (at the directory level). + * In the future, we could potentially record this info as well and + * omit reporting rename/rename(1to2) conflicts for each path within + * the affected directories, thus cleaning up the merge output. + * NOTE: We do NOT check for rename/rename(2to1) conflicts at the + * directory level, because merging directories is fine. If it + * causes conflicts for files within those merged directories, then + * that should be detected at the individual path level. + */ +static void handle_directory_level_conflicts(struct merge_options *o, + struct hashmap *dir_re_head, + struct tree *head, + struct hashmap *dir_re_merge, + struct tree *merge) +{ + struct hashmap_iter iter; + struct dir_rename_entry *head_ent; + struct dir_rename_entry *merge_ent; + + struct string_list remove_from_head = STRING_LIST_INIT_NODUP; + struct string_list remove_from_merge = STRING_LIST_INIT_NODUP; + + hashmap_iter_init(dir_re_head, &iter); + while ((head_ent = hashmap_iter_next(&iter))) { + merge_ent = dir_rename_find_entry(dir_re_merge, head_ent->dir); + if (merge_ent && + !head_ent->non_unique_new_dir && + !merge_ent->non_unique_new_dir && + !strbuf_cmp(&head_ent->new_dir, &merge_ent->new_dir)) { + /* 1. Renamed identically; remove it from both sides */ + string_list_append(&remove_from_head, + head_ent->dir)->util = head_ent; + strbuf_release(&head_ent->new_dir); + string_list_append(&remove_from_merge, + merge_ent->dir)->util = merge_ent; + strbuf_release(&merge_ent->new_dir); + } else if (tree_has_path(head, head_ent->dir)) { + /* 2. This wasn't a directory rename after all */ + string_list_append(&remove_from_head, + head_ent->dir)->util = head_ent; + strbuf_release(&head_ent->new_dir); + } + } + + remove_hashmap_entries(dir_re_head, &remove_from_head); + remove_hashmap_entries(dir_re_merge, &remove_from_merge); + + hashmap_iter_init(dir_re_merge, &iter); + while ((merge_ent = hashmap_iter_next(&iter))) { + head_ent = dir_rename_find_entry(dir_re_head, merge_ent->dir); + if (tree_has_path(merge, merge_ent->dir)) { + /* 2. This wasn't a directory rename after all */ + string_list_append(&remove_from_merge, + merge_ent->dir)->util = merge_ent; + } else if (head_ent && + !head_ent->non_unique_new_dir && + !merge_ent->non_unique_new_dir) { + /* 3. rename/rename(1to2) */ + /* + * We can assume it's not rename/rename(1to1) because + * that was case (1), already checked above. So we + * know that head_ent->new_dir and merge_ent->new_dir + * are different strings. + */ + output(o, 1, _("CONFLICT (rename/rename): " + "Rename directory %s->%s in %s. " + "Rename directory %s->%s in %s"), + head_ent->dir, head_ent->new_dir.buf, o->branch1, + head_ent->dir, merge_ent->new_dir.buf, o->branch2); + string_list_append(&remove_from_head, + head_ent->dir)->util = head_ent; + strbuf_release(&head_ent->new_dir); + string_list_append(&remove_from_merge, + merge_ent->dir)->util = merge_ent; + strbuf_release(&merge_ent->new_dir); + } + } + + remove_hashmap_entries(dir_re_head, &remove_from_head); + remove_hashmap_entries(dir_re_merge, &remove_from_merge); +} + static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs, struct tree *tree) { @@ -1911,6 +2026,10 @@ static int handle_renames(struct merge_options *o, dir_re_head = get_directory_renames(head_pairs, head); dir_re_merge = get_directory_renames(merge_pairs, merge); + handle_directory_level_conflicts(o, + dir_re_head, head, + dir_re_merge, merge); + ri->head_renames = get_renames(o, head_pairs, head, common, head, merge, entries); ri->merge_renames = get_renames(o, merge_pairs, merge, -- cgit 1.2.3-korg From e95ab70aac4f38f1d365cdc03a7a041920c9c27b Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:07 -0700 Subject: merge-recursive: add computation of collisions due to dir rename & merging directory renaming and merging can cause one or more files to be moved to where an existing file is, or to cause several files to all be moved to the same (otherwise vacant) location. Add checking and reporting for such cases, falling back to no-directory-rename handling for such paths. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- merge-recursive.h | 7 +++ 2 files changed, 150 insertions(+), 3 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index a67bb58da7..50eb6c70dd 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -87,6 +87,29 @@ static void dir_rename_entry_init(struct dir_rename_entry *entry, string_list_init(&entry->possible_new_dirs, 0); } +static struct collision_entry *collision_find_entry(struct hashmap *hashmap, + char *target_file) +{ + struct collision_entry key; + + hashmap_entry_init(&key, strhash(target_file)); + key.target_file = target_file; + return hashmap_get(hashmap, &key, NULL); +} + +static int collision_cmp(void *unused_cmp_data, + const struct collision_entry *e1, + const struct collision_entry *e2, + const void *unused_keydata) +{ + return strcmp(e1->target_file, e2->target_file); +} + +static void collision_init(struct hashmap *map) +{ + hashmap_init(map, (hashmap_cmp_fn) collision_cmp, NULL, 0); +} + static void flush_output(struct merge_options *o) { if (o->buffer_output < 2 && o->obuf.len) { @@ -1404,6 +1427,31 @@ static int tree_has_path(struct tree *tree, const char *path) &hashy, &mode_o); } +/* + * Return a new string that replaces the beginning portion (which matches + * entry->dir), with entry->new_dir. In perl-speak: + * new_path_name = (old_path =~ s/entry->dir/entry->new_dir/); + * NOTE: + * Caller must ensure that old_path starts with entry->dir + '/'. + */ +static char *apply_dir_rename(struct dir_rename_entry *entry, + const char *old_path) +{ + struct strbuf new_path = STRBUF_INIT; + int oldlen, newlen; + + if (entry->non_unique_new_dir) + return NULL; + + oldlen = strlen(entry->dir); + newlen = entry->new_dir.len + (strlen(old_path) - oldlen) + 1; + strbuf_grow(&new_path, newlen); + strbuf_addbuf(&new_path, &entry->new_dir); + strbuf_addstr(&new_path, &old_path[oldlen]); + + return strbuf_detach(&new_path, NULL); +} + static void get_renamed_dir_portion(const char *old_path, const char *new_path, char **old_dir, char **new_dir) { @@ -1673,6 +1721,84 @@ static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs, return dir_renames; } +static struct dir_rename_entry *check_dir_renamed(const char *path, + struct hashmap *dir_renames) +{ + char temp[PATH_MAX]; + char *end; + struct dir_rename_entry *entry; + + strcpy(temp, path); + while ((end = strrchr(temp, '/'))) { + *end = '\0'; + entry = dir_rename_find_entry(dir_renames, temp); + if (entry) + return entry; + } + return NULL; +} + +static void compute_collisions(struct hashmap *collisions, + struct hashmap *dir_renames, + struct diff_queue_struct *pairs) +{ + int i; + + /* + * Multiple files can be mapped to the same path due to directory + * renames done by the other side of history. Since that other + * side of history could have merged multiple directories into one, + * if our side of history added the same file basename to each of + * those directories, then all N of them would get implicitly + * renamed by the directory rename detection into the same path, + * and we'd get an add/add/.../add conflict, and all those adds + * from *this* side of history. This is not representable in the + * index, and users aren't going to easily be able to make sense of + * it. So we need to provide a good warning about what's + * happening, and fall back to no-directory-rename detection + * behavior for those paths. + * + * See testcases 9e and all of section 5 from t6043 for examples. + */ + collision_init(collisions); + + for (i = 0; i < pairs->nr; ++i) { + struct dir_rename_entry *dir_rename_ent; + struct collision_entry *collision_ent; + char *new_path; + struct diff_filepair *pair = pairs->queue[i]; + + if (pair->status == 'D') + continue; + dir_rename_ent = check_dir_renamed(pair->two->path, + dir_renames); + if (!dir_rename_ent) + continue; + + new_path = apply_dir_rename(dir_rename_ent, pair->two->path); + if (!new_path) + /* + * dir_rename_ent->non_unique_new_path is true, which + * means there is no directory rename for us to use, + * which means it won't cause us any additional + * collisions. + */ + continue; + collision_ent = collision_find_entry(collisions, new_path); + if (!collision_ent) { + collision_ent = xcalloc(1, + sizeof(struct collision_entry)); + hashmap_entry_init(collision_ent, strhash(new_path)); + hashmap_put(collisions, collision_ent); + collision_ent->target_file = new_path; + } else { + free(new_path); + } + string_list_insert(&collision_ent->source_files, + pair->two->path); + } +} + /* * Get information of all renames which occurred in 'pairs', making use of * any implicit directory renames inferred from the other side of history. @@ -1682,6 +1808,7 @@ static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs, */ static struct string_list *get_renames(struct merge_options *o, struct diff_queue_struct *pairs, + struct hashmap *dir_renames, struct tree *tree, struct tree *o_tree, struct tree *a_tree, @@ -1689,8 +1816,12 @@ static struct string_list *get_renames(struct merge_options *o, struct string_list *entries) { int i; + struct hashmap collisions; + struct hashmap_iter iter; + struct collision_entry *e; struct string_list *renames; + compute_collisions(&collisions, dir_renames, pairs); renames = xcalloc(1, sizeof(struct string_list)); for (i = 0; i < pairs->nr; ++i) { @@ -1721,6 +1852,13 @@ static struct string_list *get_renames(struct merge_options *o, item = string_list_insert(renames, pair->one->path); item->util = re; } + + hashmap_iter_init(&collisions, &iter); + while ((e = hashmap_iter_next(&iter))) { + free(e->target_file); + string_list_clear(&e->source_files, 0); + } + hashmap_free(&collisions, 1); return renames; } @@ -2030,9 +2168,11 @@ static int handle_renames(struct merge_options *o, dir_re_head, head, dir_re_merge, merge); - ri->head_renames = get_renames(o, head_pairs, head, - common, head, merge, entries); - ri->merge_renames = get_renames(o, merge_pairs, merge, + ri->head_renames = get_renames(o, head_pairs, + dir_re_merge, head, + common, head, merge, entries); + 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); diff --git a/merge-recursive.h b/merge-recursive.h index fe64c78de4..50a4e6af4e 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -47,6 +47,13 @@ struct dir_rename_entry { struct string_list possible_new_dirs; }; +struct collision_entry { + struct hashmap_entry ent; /* must be the first member! */ + char *target_file; + struct string_list source_files; + unsigned reported_already:1; +}; + /* merge_trees() but with recursive ancestor consolidation */ int merge_recursive(struct merge_options *o, struct commit *h1, -- cgit 1.2.3-korg From f6f775591881036a3c4bfcf6737c9119c251537d Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:08 -0700 Subject: merge-recursive: check for file level conflicts then get new name 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. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 174 ++++++++++++++++++++++++++++++++++-- strbuf.c | 16 ++++ strbuf.h | 16 ++++ t/t6043-merge-rename-directories.sh | 2 +- 4 files changed, 199 insertions(+), 9 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 50eb6c70dd..7a5f6fb5ec 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1520,6 +1520,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 @@ -1799,6 +1884,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. @@ -1809,11 +1947,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; @@ -1828,11 +1968,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; @@ -2150,7 +2301,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; @@ -2169,13 +2320,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 43a840c67b..83d05024e6 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) @@ -171,6 +172,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 4efa80c1de..c4de5e4588 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 8ea9ec49bc..b24562b849 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -489,7 +489,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 && -- cgit 1.2.3-korg From 5b047ac07084b7b8508ea706a9950fc19709358c Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:09 -0700 Subject: merge-recursive: when comparing files, don't include trees get_renames() would look up stage data that already existed (populated in get_unmerged(), taken from whatever unpack_trees() created), and if it didn't exist, would call insert_stage_data() to create the necessary entry for the given file. The insert_stage_data() fallback becomes much more important for directory rename detection, because that creates a mechanism to have a file in the resulting merge that didn't exist on either side of history. However, insert_stage_data(), due to calling get_tree_entry() loaded up trees as readily as files. We aren't interested in comparing trees to files; the D/F conflict handling is done elsewhere. This code is just concerned with what entries existed for a given path on the different sides of the merge, so create a get_tree_entry_if_blob() helper function and use it. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 7a5f6fb5ec..f1be349449 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -421,6 +421,21 @@ static void get_files_dirs(struct merge_options *o, struct tree *tree) read_tree_recursive(tree, "", 0, 0, &match_all, save_files_dirs, o); } +static int get_tree_entry_if_blob(const struct object_id *tree, + const char *path, + struct object_id *hashy, + unsigned int *mode_o) +{ + int ret; + + ret = get_tree_entry(tree, path, hashy, mode_o); + if (S_ISDIR(*mode_o)) { + oidcpy(hashy, &null_oid); + *mode_o = 0; + } + return ret; +} + /* * Returns an index_entry instance which doesn't have to correspond to * a real cache entry in Git's index. @@ -431,12 +446,12 @@ static struct stage_data *insert_stage_data(const char *path, { struct string_list_item *item; struct stage_data *e = xcalloc(1, sizeof(struct stage_data)); - get_tree_entry(&o->object.oid, path, - &e->stages[1].oid, &e->stages[1].mode); - get_tree_entry(&a->object.oid, path, - &e->stages[2].oid, &e->stages[2].mode); - get_tree_entry(&b->object.oid, path, - &e->stages[3].oid, &e->stages[3].mode); + get_tree_entry_if_blob(&o->object.oid, path, + &e->stages[1].oid, &e->stages[1].mode); + get_tree_entry_if_blob(&a->object.oid, path, + &e->stages[2].oid, &e->stages[2].mode); + get_tree_entry_if_blob(&b->object.oid, path, + &e->stages[3].oid, &e->stages[3].mode); item = string_list_insert(entries, path); item->util = e; return e; -- cgit 1.2.3-korg From 9c0743fe1e45b3a0ffe166ac949a27f95a3e5c34 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:10 -0700 Subject: merge-recursive: apply necessary modifications for directory renames This commit hooks together all the directory rename logic by making the necessary changes to the rename struct, it's dst_entry, and the diff_filepair under consideration. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 187 +++++++++++++++++++++++++++++++++++- t/t6043-merge-rename-directories.sh | 50 +++++----- 2 files changed, 211 insertions(+), 26 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index f1be349449..48a49bd1c0 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -180,6 +180,7 @@ static int oid_eq(const struct object_id *a, const struct object_id *b) enum rename_type { RENAME_NORMAL = 0, + RENAME_DIR, RENAME_DELETE, RENAME_ONE_FILE_TO_ONE, RENAME_ONE_FILE_TO_TWO, @@ -610,6 +611,7 @@ struct rename { */ struct stage_data *src_entry; struct stage_data *dst_entry; + unsigned add_turned_into_rename:1; unsigned processed:1; }; @@ -644,6 +646,27 @@ static int update_stages(struct merge_options *opt, const char *path, return 0; } +static int update_stages_for_stage_data(struct merge_options *opt, + const char *path, + const struct stage_data *stage_data) +{ + struct diff_filespec o, a, b; + + o.mode = stage_data->stages[1].mode; + oidcpy(&o.oid, &stage_data->stages[1].oid); + + a.mode = stage_data->stages[2].mode; + oidcpy(&a.oid, &stage_data->stages[2].oid); + + b.mode = stage_data->stages[3].mode; + oidcpy(&b.oid, &stage_data->stages[3].oid); + + return update_stages(opt, path, + is_null_oid(&o.oid) ? NULL : &o, + is_null_oid(&a.oid) ? NULL : &a, + is_null_oid(&b.oid) ? NULL : &b); +} + static void update_entry(struct stage_data *entry, struct diff_filespec *o, struct diff_filespec *a, @@ -1121,6 +1144,18 @@ static int merge_file_one(struct merge_options *o, return merge_file_1(o, &one, &a, &b, branch1, branch2, mfi); } +static int conflict_rename_dir(struct merge_options *o, + struct diff_filepair *pair, + const char *rename_branch, + const char *other_branch) +{ + const struct diff_filespec *dest = pair->two; + + if (update_file(o, 1, &dest->oid, dest->mode, dest->path)) + return -1; + return 0; +} + static int handle_change_delete(struct merge_options *o, const char *path, const char *old_path, const struct object_id *o_oid, int o_mode, @@ -1390,6 +1425,24 @@ static int conflict_rename_rename_2to1(struct merge_options *o, if (!ret) ret = update_file(o, 0, &mfi_c2.oid, mfi_c2.mode, new_path2); + /* + * unpack_trees() actually populates the index for us for + * "normal" rename/rename(2to1) situtations so that the + * correct entries are at the higher stages, which would + * make the call below to update_stages_for_stage_data + * unnecessary. However, if either of the renames came + * from a directory rename, then unpack_trees() will not + * have gotten the right data loaded into the index, so we + * need to do so now. (While it'd be tempting to move this + * call to update_stages_for_stage_data() to + * apply_directory_rename_modifications(), that would break + * our intermediate calls to would_lose_untracked() since + * those rely on the current in-memory index. See also the + * big "NOTE" in update_stages()). + */ + if (update_stages_for_stage_data(o, path, ci->dst_entry1)) + ret = -1; + free(new_path2); free(new_path1); } @@ -1952,6 +2005,111 @@ static char *check_for_directory_rename(struct merge_options *o, return new_path; } +static void apply_directory_rename_modifications(struct merge_options *o, + struct diff_filepair *pair, + char *new_path, + struct rename *re, + struct tree *tree, + struct tree *o_tree, + struct tree *a_tree, + struct tree *b_tree, + struct string_list *entries, + int *clean) +{ + struct string_list_item *item; + int stage = (tree == a_tree ? 2 : 3); + + /* + * In all cases where we can do directory rename detection, + * unpack_trees() will have read pair->two->path into the + * index and the working copy. We need to remove it so that + * we can instead place it at new_path. It is guaranteed to + * not be untracked (unpack_trees() would have errored out + * saying the file would have been overwritten), but it might + * be dirty, though. + */ + remove_file(o, 1, pair->two->path, 0 /* no_wd */); + + /* Find or create a new re->dst_entry */ + item = string_list_lookup(entries, new_path); + if (item) { + /* + * Since we're renaming on this side of history, and it's + * due to a directory rename on the other side of history + * (which we only allow when the directory in question no + * longer exists on the other side of history), the + * original entry for re->dst_entry is no longer + * necessary... + */ + re->dst_entry->processed = 1; + + /* + * ...because we'll be using this new one. + */ + re->dst_entry = item->util; + } else { + /* + * re->dst_entry is for the before-dir-rename path, and we + * need it to hold information for the after-dir-rename + * path. Before creating a new entry, we need to mark the + * old one as unnecessary (...unless it is shared by + * src_entry, i.e. this didn't use to be a rename, in which + * case we can just allow the normal processing to happen + * for it). + */ + if (pair->status == 'R') + re->dst_entry->processed = 1; + + re->dst_entry = insert_stage_data(new_path, + o_tree, a_tree, b_tree, + entries); + item = string_list_insert(entries, new_path); + item->util = re->dst_entry; + } + + /* + * Update the stage_data with the information about the path we are + * moving into place. That slot will be empty and available for us + * to write to because of the collision checks in + * handle_path_level_conflicts(). In other words, + * re->dst_entry->stages[stage].oid will be the null_oid, so it's + * open for us to write to. + * + * It may be tempting to actually update the index at this point as + * well, using update_stages_for_stage_data(), but as per the big + * "NOTE" in update_stages(), doing so will modify the current + * in-memory index which will break calls to would_lose_untracked() + * that we need to make. Instead, we need to just make sure that + * the various conflict_rename_*() functions update the index + * explicitly rather than relying on unpack_trees() to have done it. + */ + get_tree_entry(&tree->object.oid, + pair->two->path, + &re->dst_entry->stages[stage].oid, + &re->dst_entry->stages[stage].mode); + + /* Update pair status */ + if (pair->status == 'A') { + /* + * Recording rename information for this add makes it look + * like a rename/delete conflict. Make sure we can + * correctly handle this as an add that was moved to a new + * directory instead of reporting a rename/delete conflict. + */ + re->add_turned_into_rename = 1; + } + /* + * We don't actually look at pair->status again, but it seems + * pedagogically correct to adjust it. + */ + pair->status = 'R'; + + /* + * Finally, record the new location. + */ + pair->two->path = 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. @@ -2001,6 +2159,7 @@ static struct string_list *get_renames(struct merge_options *o, re = xmalloc(sizeof(*re)); re->processed = 0; + re->add_turned_into_rename = 0; re->pair = pair; item = string_list_lookup(entries, re->pair->one->path); if (!item) @@ -2017,6 +2176,12 @@ static struct string_list *get_renames(struct merge_options *o, re->dst_entry = item->util; item = string_list_insert(renames, pair->one->path); item->util = re; + if (new_path) + apply_directory_rename_modifications(o, pair, new_path, + re, tree, o_tree, + a_tree, b_tree, + entries, + clean_merge); } hashmap_iter_init(&collisions, &iter); @@ -2186,7 +2351,19 @@ static int process_renames(struct merge_options *o, dst_other.mode = ren1->dst_entry->stages[other_stage].mode; try_merge = 0; - if (oid_eq(&src_other.oid, &null_oid)) { + if (oid_eq(&src_other.oid, &null_oid) && + ren1->add_turned_into_rename) { + setup_rename_conflict_info(RENAME_DIR, + ren1->pair, + NULL, + branch1, + branch2, + ren1->dst_entry, + NULL, + o, + NULL, + NULL); + } else if (oid_eq(&src_other.oid, &null_oid)) { setup_rename_conflict_info(RENAME_DELETE, ren1->pair, NULL, @@ -2603,6 +2780,14 @@ static int process_entry(struct merge_options *o, o_oid, o_mode, a_oid, a_mode, b_oid, b_mode, conflict_info); break; + case RENAME_DIR: + clean_merge = 1; + if (conflict_rename_dir(o, + conflict_info->pair1, + conflict_info->branch1, + conflict_info->branch2)) + clean_merge = -1; + break; case RENAME_DELETE: clean_merge = 0; if (conflict_rename_delete(o, diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index b24562b849..3525c54bb4 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -69,7 +69,7 @@ test_expect_success '1a-setup: Simple directory rename detection' ' ) ' -test_expect_failure '1a-check: Simple directory rename detection' ' +test_expect_success '1a-check: Simple directory rename detection' ' ( cd 1a && @@ -136,7 +136,7 @@ test_expect_success '1b-setup: Merge a directory with another' ' ) ' -test_expect_failure '1b-check: Merge a directory with another' ' +test_expect_success '1b-check: Merge a directory with another' ' ( cd 1b && @@ -194,7 +194,7 @@ test_expect_success '1c-setup: Transitive renaming' ' ) ' -test_expect_failure '1c-check: Transitive renaming' ' +test_expect_success '1c-check: Transitive renaming' ' ( cd 1c && @@ -263,7 +263,7 @@ test_expect_success '1d-setup: Directory renames cause a rename/rename(2to1) con ) ' -test_expect_failure '1d-check: Directory renames cause a rename/rename(2to1) conflict' ' +test_expect_success '1d-check: Directory renames cause a rename/rename(2to1) conflict' ' ( cd 1d && @@ -342,7 +342,7 @@ test_expect_success '1e-setup: Renamed directory, with all files being renamed t ) ' -test_expect_failure '1e-check: Renamed directory, with all files being renamed too' ' +test_expect_success '1e-check: Renamed directory, with all files being renamed too' ' ( cd 1e && @@ -408,7 +408,7 @@ test_expect_success '1f-setup: Split a directory into two other directories' ' ) ' -test_expect_failure '1f-check: Split a directory into two other directories' ' +test_expect_success '1f-check: Split a directory into two other directories' ' ( cd 1f && @@ -907,7 +907,7 @@ test_expect_success '5a-setup: Merge directories, other side adds files to origi ) ' -test_expect_failure '5a-check: Merge directories, other side adds files to original and target' ' +test_expect_success '5a-check: Merge directories, other side adds files to original and target' ' ( cd 5a && @@ -981,7 +981,7 @@ test_expect_success '5b-setup: Rename/delete in order to get add/add/add conflic ) ' -test_expect_failure '5b-check: Rename/delete in order to get add/add/add conflict' ' +test_expect_success '5b-check: Rename/delete in order to get add/add/add conflict' ' ( cd 5b && @@ -1061,7 +1061,7 @@ test_expect_success '5c-setup: Transitive rename would cause rename/rename/renam ) ' -test_expect_failure '5c-check: Transitive rename would cause rename/rename/rename/add/add/add' ' +test_expect_success '5c-check: Transitive rename would cause rename/rename/rename/add/add/add' ' ( cd 5c && @@ -1145,7 +1145,7 @@ test_expect_success '5d-setup: Directory/file/file conflict due to directory ren ) ' -test_expect_failure '5d-check: Directory/file/file conflict due to directory rename' ' +test_expect_success '5d-check: Directory/file/file conflict due to directory rename' ' ( cd 5d && @@ -1583,7 +1583,7 @@ test_expect_success '7a-setup: rename-dir vs. rename-dir (NOT split evenly) PLUS ) ' -test_expect_failure '7a-check: rename-dir vs. rename-dir (NOT split evenly) PLUS add-other-file' ' +test_expect_success '7a-check: rename-dir vs. rename-dir (NOT split evenly) PLUS add-other-file' ' ( cd 7a && @@ -1655,7 +1655,7 @@ test_expect_success '7b-setup: rename/rename(2to1), but only due to transitive r ) ' -test_expect_failure '7b-check: rename/rename(2to1), but only due to transitive rename' ' +test_expect_success '7b-check: rename/rename(2to1), but only due to transitive rename' ' ( cd 7b && @@ -1731,7 +1731,7 @@ test_expect_success '7c-setup: rename/rename(1to...2or3); transitive rename may ) ' -test_expect_failure '7c-check: rename/rename(1to...2or3); transitive rename may add complexity' ' +test_expect_success '7c-check: rename/rename(1to...2or3); transitive rename may add complexity' ' ( cd 7c && @@ -1795,7 +1795,7 @@ test_expect_success '7d-setup: transitive rename involved in rename/delete; how ) ' -test_expect_failure '7d-check: transitive rename involved in rename/delete; how is it reported?' ' +test_expect_success '7d-check: transitive rename involved in rename/delete; how is it reported?' ' ( cd 7d && @@ -1885,7 +1885,7 @@ test_expect_success '7e-setup: transitive rename in rename/delete AND dirs in th ) ' -test_expect_failure '7e-check: transitive rename in rename/delete AND dirs in the way' ' +test_expect_success '7e-check: transitive rename in rename/delete AND dirs in the way' ' ( cd 7e && @@ -1976,7 +1976,7 @@ test_expect_success '8a-setup: Dual-directory rename, one into the others way' ' ) ' -test_expect_failure '8a-check: Dual-directory rename, one into the others way' ' +test_expect_success '8a-check: Dual-directory rename, one into the others way' ' ( cd 8a && @@ -2121,7 +2121,7 @@ test_expect_success '8c-setup: rename+modify/delete' ' ) ' -test_expect_failure '8c-check: rename+modify/delete' ' +test_expect_success '8c-check: rename+modify/delete' ' ( cd 8c && @@ -2208,7 +2208,7 @@ test_expect_success '8d-setup: rename/delete...or not?' ' ) ' -test_expect_failure '8d-check: rename/delete...or not?' ' +test_expect_success '8d-check: rename/delete...or not?' ' ( cd 8d && @@ -2283,7 +2283,7 @@ test_expect_success '8e-setup: Both sides rename, one side adds to original dire ) ' -test_expect_failure '8e-check: Both sides rename, one side adds to original directory' ' +test_expect_success '8e-check: Both sides rename, one side adds to original directory' ' ( cd 8e && @@ -2370,7 +2370,7 @@ test_expect_success '9a-setup: Inner renamed directory within outer renamed dire ) ' -test_expect_failure '9a-check: Inner renamed directory within outer renamed directory' ' +test_expect_success '9a-check: Inner renamed directory within outer renamed directory' ' ( cd 9a && @@ -2440,7 +2440,7 @@ test_expect_success '9b-setup: Transitive rename with content merge' ' ) ' -test_expect_failure '9b-check: Transitive rename with content merge' ' +test_expect_success '9b-check: Transitive rename with content merge' ' ( cd 9b && @@ -2530,7 +2530,7 @@ test_expect_success '9c-setup: Doubly transitive rename?' ' ) ' -test_expect_failure '9c-check: Doubly transitive rename?' ' +test_expect_success '9c-check: Doubly transitive rename?' ' ( cd 9c && @@ -2618,7 +2618,7 @@ test_expect_success '9d-setup: N-way transitive rename?' ' ) ' -test_expect_failure '9d-check: N-way transitive rename?' ' +test_expect_success '9d-check: N-way transitive rename?' ' ( cd 9d && @@ -2700,7 +2700,7 @@ test_expect_success '9e-setup: N-to-1 whammo' ' ) ' -test_expect_failure C_LOCALE_OUTPUT '9e-check: N-to-1 whammo' ' +test_expect_success C_LOCALE_OUTPUT '9e-check: N-to-1 whammo' ' ( cd 9e && @@ -2778,7 +2778,7 @@ test_expect_success '9f-setup: Renamed directory that only contained immediate s ) ' -test_expect_failure '9f-check: Renamed directory that only contained immediate subdirs' ' +test_expect_success '9f-check: Renamed directory that only contained immediate subdirs' ' ( cd 9f && -- cgit 1.2.3-korg From 79c47598f5c8c0008ae9281f20c2a041d4cabd93 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:11 -0700 Subject: merge-recursive: avoid clobbering untracked files with directory renames Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 42 +++++++++++++++++++++++++++++++++++-- t/t6043-merge-rename-directories.sh | 6 +++--- 2 files changed, 43 insertions(+), 5 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 48a49bd1c0..87f2479e1a 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1151,6 +1151,26 @@ static int conflict_rename_dir(struct merge_options *o, { const struct diff_filespec *dest = pair->two; + if (!o->call_depth && would_lose_untracked(dest->path)) { + char *alt_path = unique_path(o, dest->path, rename_branch); + + output(o, 1, _("Error: Refusing to lose untracked file at %s; " + "writing to %s instead."), + dest->path, alt_path); + /* + * Write the file in worktree at alt_path, but not in the + * index. Instead, write to dest->path for the index but + * only at the higher appropriate stage. + */ + if (update_file(o, 0, &dest->oid, dest->mode, alt_path)) + return -1; + free(alt_path); + return update_stages(o, dest->path, NULL, + rename_branch == o->branch1 ? dest : NULL, + rename_branch == o->branch1 ? NULL : dest); + } + + /* Update dest->path both in index and in worktree */ if (update_file(o, 1, &dest->oid, dest->mode, dest->path)) return -1; return 0; @@ -1169,7 +1189,8 @@ static int handle_change_delete(struct merge_options *o, const char *update_path = path; int ret = 0; - if (dir_in_way(path, !o->call_depth, 0)) { + if (dir_in_way(path, !o->call_depth, 0) || + (!o->call_depth && would_lose_untracked(path))) { update_path = alt_path = unique_path(o, path, change_branch); } @@ -1295,6 +1316,12 @@ static int handle_file(struct merge_options *o, dst_name = unique_path(o, rename->path, cur_branch); output(o, 1, _("%s is a directory in %s adding as %s instead"), rename->path, other_branch, dst_name); + } else if (!o->call_depth && + would_lose_untracked(rename->path)) { + dst_name = unique_path(o, rename->path, cur_branch); + output(o, 1, _("Refusing to lose untracked file at %s; " + "adding as %s instead"), + rename->path, dst_name); } } if ((ret = update_file(o, 0, &rename->oid, rename->mode, dst_name))) @@ -1420,7 +1447,18 @@ static int conflict_rename_rename_2to1(struct merge_options *o, char *new_path2 = unique_path(o, path, ci->branch2); output(o, 1, _("Renaming %s to %s and %s to %s instead"), a->path, new_path1, b->path, new_path2); - remove_file(o, 0, path, 0); + if (would_lose_untracked(path)) + /* + * Only way we get here is if both renames were from + * a directory rename AND user had an untracked file + * at the location where both files end up after the + * two directory renames. See testcase 10d of t6043. + */ + output(o, 1, _("Refusing to lose untracked file at " + "%s, even though it's in the way."), + path); + else + remove_file(o, 0, path, 0); ret = update_file(o, 0, &mfi_c1.oid, mfi_c1.mode, new_path1); if (!ret) ret = update_file(o, 0, &mfi_c2.oid, mfi_c2.mode, diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index 3525c54bb4..0b60eb8053 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -2992,7 +2992,7 @@ test_expect_success '10b-setup: Overwrite untracked with dir rename + delete' ' ) ' -test_expect_failure '10b-check: Overwrite untracked with dir rename + delete' ' +test_expect_success '10b-check: Overwrite untracked with dir rename + delete' ' ( cd 10b && @@ -3070,7 +3070,7 @@ test_expect_success '10c-setup: Overwrite untracked with dir rename/rename(1to2) ) ' -test_expect_failure '10c-check: Overwrite untracked with dir rename/rename(1to2)' ' +test_expect_success '10c-check: Overwrite untracked with dir rename/rename(1to2)' ' ( cd 10c && @@ -3145,7 +3145,7 @@ test_expect_success '10d-setup: Delete untracked with dir rename/rename(2to1)' ' ) ' -test_expect_failure '10d-check: Delete untracked with dir rename/rename(2to1)' ' +test_expect_success '10d-check: Delete untracked with dir rename/rename(2to1)' ' ( cd 10d && -- cgit 1.2.3-korg From 64b1abe962b44e6bad84b980e8ea2811302e71c7 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:12 -0700 Subject: merge-recursive: fix overwriting dirty files involved in renames 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 Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- 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(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 87f2479e1a..4fb5f797da 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, -- cgit 1.2.3-korg From 18797a3b10e4077473740b953651b0032828d0fb Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:13 -0700 Subject: merge-recursive: fix remaining directory rename + dirty overwrite cases Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 25 ++++++++++++++++++++++--- t/t6043-merge-rename-directories.sh | 8 ++++---- 2 files changed, 26 insertions(+), 7 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 4fb5f797da..08c2dc094e 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1324,11 +1324,22 @@ static int handle_file(struct merge_options *o, add = filespec_from_entry(&other, dst_entry, stage ^ 1); if (add) { + int ren_src_was_dirty = was_dirty(o, rename->path); char *add_name = unique_path(o, rename->path, other_branch); if (update_file(o, 0, &add->oid, add->mode, add_name)) return -1; - remove_file(o, 0, rename->path, 0); + if (ren_src_was_dirty) { + output(o, 1, _("Refusing to lose dirty file at %s"), + rename->path); + } + /* + * Because the double negatives somehow keep confusing me... + * 1) update_wd iff !ren_src_was_dirty. + * 2) no_wd iff !update_wd + * 3) so, no_wd == !!ren_src_was_dirty == ren_src_was_dirty + */ + remove_file(o, 0, rename->path, ren_src_was_dirty); dst_name = unique_path(o, rename->path, cur_branch); } else { if (dir_in_way(rename->path, !o->call_depth, 0)) { @@ -1466,7 +1477,10 @@ static int conflict_rename_rename_2to1(struct merge_options *o, char *new_path2 = unique_path(o, path, ci->branch2); output(o, 1, _("Renaming %s to %s and %s to %s instead"), a->path, new_path1, b->path, new_path2); - if (would_lose_untracked(path)) + if (was_dirty(o, path)) + output(o, 1, _("Refusing to lose dirty file at %s"), + path); + else if (would_lose_untracked(path)) /* * Only way we get here is if both renames were from * a directory rename AND user had an untracked file @@ -2075,6 +2089,7 @@ static void apply_directory_rename_modifications(struct merge_options *o, { struct string_list_item *item; int stage = (tree == a_tree ? 2 : 3); + int update_wd; /* * In all cases where we can do directory rename detection, @@ -2085,7 +2100,11 @@ static void apply_directory_rename_modifications(struct merge_options *o, * saying the file would have been overwritten), but it might * be dirty, though. */ - remove_file(o, 1, pair->two->path, 0 /* no_wd */); + update_wd = !was_dirty(o, pair->two->path); + if (!update_wd) + output(o, 1, _("Refusing to lose dirty file at %s"), + pair->two->path); + remove_file(o, 1, pair->two->path, !update_wd); /* Find or create a new re->dst_entry */ item = string_list_lookup(entries, new_path); diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index b94ba066fe..33e2649824 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -3370,7 +3370,7 @@ test_expect_success '11b-setup: Avoid losing dirty file involved in directory re ) ' -test_expect_failure '11b-check: Avoid losing dirty file involved in directory rename' ' +test_expect_success '11b-check: Avoid losing dirty file involved in directory rename' ' ( cd 11b && @@ -3512,7 +3512,7 @@ test_expect_success '11d-setup: Avoid losing not-uptodate with rename + D/F conf ) ' -test_expect_failure '11d-check: Avoid losing not-uptodate with rename + D/F conflict' ' +test_expect_success '11d-check: Avoid losing not-uptodate with rename + D/F conflict' ' ( cd 11d && @@ -3591,7 +3591,7 @@ test_expect_success '11e-setup: Avoid deleting not-uptodate with dir rename/rena ) ' -test_expect_failure '11e-check: Avoid deleting not-uptodate with dir rename/rename(1to2)/add' ' +test_expect_success '11e-check: Avoid deleting not-uptodate with dir rename/rename(1to2)/add' ' ( cd 11e && @@ -3667,7 +3667,7 @@ test_expect_success '11f-setup: Avoid deleting not-uptodate with dir rename/rena ) ' -test_expect_failure '11f-check: Avoid deleting not-uptodate with dir rename/rename(2to1)' ' +test_expect_success '11f-check: Avoid deleting not-uptodate with dir rename/rename(2to1)' ' ( cd 11f && -- cgit 1.2.3-korg From 6e7e027fe5f4a8d61597a86e7f2b6087e23a759c Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:15 -0700 Subject: merge-recursive: avoid spurious rename/rename conflict from dir renames If a file on one side of history was renamed, and merely modified on the other side, then applying a directory rename to the modified side gives us a rename/rename(1to2) conflict. We should only apply directory renames to pairs representing either adds or renames. Making this change means that a directory rename testcase that was previously reported as a rename/delete conflict will now be reported as a modify/delete conflict. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 4 +-- t/t6043-merge-rename-directories.sh | 55 +++++++++++++++++-------------------- 2 files changed, 27 insertions(+), 32 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 08c2dc094e..d079d6783b 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1992,7 +1992,7 @@ static void compute_collisions(struct hashmap *collisions, char *new_path; struct diff_filepair *pair = pairs->queue[i]; - if (pair->status == 'D') + if (pair->status != 'A' && pair->status != 'R') continue; dir_rename_ent = check_dir_renamed(pair->two->path, dir_renames); @@ -2219,7 +2219,7 @@ static struct string_list *get_renames(struct merge_options *o, struct diff_filepair *pair = pairs->queue[i]; char *new_path; /* non-NULL only with directory renames */ - if (pair->status == 'D') { + if (pair->status != 'A' && pair->status != 'R') { diff_free_filepair(pair); continue; } diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index 5b84591445..45f620633f 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -2078,18 +2078,23 @@ test_expect_success '8b-check: Dual-directory rename, one into the others way, w ) ' -# Testcase 8c, rename+modify/delete -# (Related to testcases 5b and 8d) +# Testcase 8c, modify/delete or rename+modify/delete? +# (Related to testcases 5b, 8d, and 9h) # Commit O: z/{b,c,d} # Commit A: y/{b,c} # Commit B: z/{b,c,d_modified,e} -# Expected: y/{b,c,e}, CONFLICT(rename+modify/delete: x/d -> y/d or deleted) +# Expected: y/{b,c,e}, CONFLICT(modify/delete: on z/d) # -# Note: This testcase doesn't present any concerns for me...until you -# compare it with testcases 5b and 8d. See notes in 8d for more -# details. - -test_expect_success '8c-setup: rename+modify/delete' ' +# Note: It could easily be argued that the correct resolution here is +# y/{b,c,e}, CONFLICT(rename/delete: z/d -> y/d vs deleted) +# and that the modifed version of d should be present in y/ after +# the merge, just marked as conflicted. Indeed, I previously did +# argue that. But applying directory renames to the side of +# history where a file is merely modified results in spurious +# rename/rename(1to2) conflicts -- see testcase 9h. See also +# notes in 8d. + +test_expect_success '8c-setup: modify/delete or rename+modify/delete?' ' test_create_repo 8c && ( cd 8c && @@ -2122,32 +2127,32 @@ test_expect_success '8c-setup: rename+modify/delete' ' ) ' -test_expect_success '8c-check: rename+modify/delete' ' +test_expect_success '8c-check: modify/delete or rename+modify/delete' ' ( cd 8c && git checkout A^0 && test_must_fail git merge -s recursive B^0 >out && - test_i18ngrep "CONFLICT (rename/delete).* z/d.*y/d" out && + test_i18ngrep "CONFLICT (modify/delete).* z/d" out && git ls-files -s >out && - test_line_count = 4 out && + test_line_count = 5 out && git ls-files -u >out && - test_line_count = 1 out && + test_line_count = 2 out && git ls-files -o >out && test_line_count = 1 out && git rev-parse >actual \ - :0:y/b :0:y/c :0:y/e :3:y/d && + :0:y/b :0:y/c :0:y/e :1:z/d :3:z/d && git rev-parse >expect \ - O:z/b O:z/c B:z/e B:z/d && + O:z/b O:z/c B:z/e O:z/d B:z/d && test_cmp expect actual && - test_must_fail git rev-parse :1:y/d && - test_must_fail git rev-parse :2:y/d && - git ls-files -s y/d | grep ^100755 && - test_path_is_file y/d + test_must_fail git rev-parse :2:z/d && + git ls-files -s z/d | grep ^100755 && + test_path_is_file z/d && + test_path_is_missing y/d ) ' @@ -2161,16 +2166,6 @@ test_expect_success '8c-check: rename+modify/delete' ' # # Note: It would also be somewhat reasonable to resolve this as # y/{b,c,e}, CONFLICT(rename/delete: x/d -> y/d or deleted) -# The logic being that the only difference between this testcase and 8c -# is that there is no modification to d. That suggests that instead of a -# rename/modify vs. delete conflict, we should just have a rename/delete -# conflict, otherwise we are being inconsistent. -# -# However...as far as consistency goes, we didn't report a conflict for -# path d_1 in testcase 5b due to a different file being in the way. So, -# we seem to be forced to have cases where users can change things -# slightly and get what they may perceive as inconsistent results. It -# would be nice to avoid that, but I'm not sure I see how. # # In this case, I'm leaning towards: commit A was the one that deleted z/d # and it did the rename of z to y, so the two "conflicts" (rename vs. @@ -2915,7 +2910,7 @@ test_expect_success '9h-setup: Avoid dir rename on merely modified path' ' ) ' -test_expect_failure '9h-check: Avoid dir rename on merely modified path' ' +test_expect_success '9h-check: Avoid dir rename on merely modified path' ' ( cd 9h && @@ -3959,7 +3954,7 @@ test_expect_success '12c-setup: Moving one directory hierarchy into another w/ c ) ' -test_expect_failure '12c-check: Moving one directory hierarchy into another w/ content merge' ' +test_expect_success '12c-check: Moving one directory hierarchy into another w/ content merge' ' ( cd 12c && -- cgit 1.2.3-korg From fd53b7ffd1bd499d24fa48b43a79703355bde512 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:16 -0700 Subject: merge-recursive: improve add_cacheinfo error handling Four closely related changes all with the purpose of fixing error handling in this function: - fix reported function name in add_cacheinfo error messages - differentiate between the two error messages - abort early when we hit the error (stop ignoring return code) - mark a test which was hitting this error as failing until we get the right fix In more detail... In commit 0424138d5715 ("Fix bogus error message from merge-recursive error path", 2007-04-01), it was noted that the name of the function which the error message claimed it was reported from did not match the actual function name. This was changed to something closer to the real function name, but it still didn't match the actual function name. Fix the reported name to match. Second, the two errors in this function had identical messages, preventing us from knowing which error had been triggered. Add a couple words to the second error message to differentiate the two. Next, make sure callers do not ignore the return code so that it will stop processing further entries (processing further entries could result in more output which could cause the error to scroll off the screen, or at least be missed by the user) and make it clear the error is the cause of the early abort. These errors should never be triggered in production; if either one is, it represents a bug in the calling path somewhere and is likely to have resulted in mis-merged content. The combination of ignoring of the return code and continuing to print other standard messages after hitting the error resulted in the following bug report from Junio: "...the command pretends that everything went well and merged cleanly in that path...[Behaving] in a buggy and unexplainable way is bad enough, doing so silently is unexcusable." Fix this. Finally, there was one test in the testsuite that did hit this error path, but was passing anyway. This would have been easy to miss since it had a test_must_fail and thus could have failed for the wrong reason, but in a separate testing step I added an intentional NULL-dereference to the codepath where these error messages are printed in order to flush out such cases. I could modify that test to explicitly check for this error and fail the test if it is hit, but since this test operates in a bit of a gray area and needed other changes, I went for a different fix. The gray area this test operates in is the following: If the merge of a certain file results in the same version of the file that existed in HEAD, but there are dirty modifications to the file, is that an error with a "Refusing to overwrite existing file" expected, or a case where the merge should succeed since we shouldn't have to touch the dirty file anyway? Recent discussion on the list leaned towards saying it should be a success. Therefore, change the expected behavior of this test to match. As a side effect, this makes the failed-due-to-hitting-add_cacheinfo-error very clear, and we can mark the test as test_expect_failure. A subsequent commit will implement the necessary changes to get this test to pass again. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 13 ++++++++----- t/t3501-revert-cherry-pick.sh | 7 +++---- 2 files changed, 11 insertions(+), 9 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index d079d6783b..d3771e3626 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -316,7 +316,7 @@ static int add_cacheinfo(struct merge_options *o, ce = make_cache_entry(mode, oid ? oid->hash : null_sha1, path, stage, 0); if (!ce) - return err(o, _("addinfo_cache failed for path '%s'"), path); + return err(o, _("add_cacheinfo failed for path '%s'; merge aborting."), path); ret = add_cache_entry(ce, options); if (refresh) { @@ -324,7 +324,7 @@ static int add_cacheinfo(struct merge_options *o, nce = refresh_cache_entry(ce, CE_MATCH_REFRESH | CE_MATCH_IGNORE_MISSING); if (!nce) - return err(o, _("addinfo_cache failed for path '%s'"), path); + return err(o, _("add_cacheinfo failed to refresh for path '%s'; merge aborting."), path); if (nce != ce) ret = add_cache_entry(nce, options); } @@ -942,7 +942,9 @@ static int update_file_flags(struct merge_options *o, } update_index: if (!ret && update_cache) - add_cacheinfo(o, mode, oid, path, 0, update_wd, ADD_CACHE_OK_TO_ADD); + if (add_cacheinfo(o, mode, oid, path, 0, update_wd, + ADD_CACHE_OK_TO_ADD)) + return -1; return ret; } @@ -2783,8 +2785,9 @@ static int merge_content(struct merge_options *o, */ path_renamed_outside_HEAD = !path2 || !strcmp(path, path2); if (!path_renamed_outside_HEAD) { - add_cacheinfo(o, mfi.mode, &mfi.oid, path, - 0, (!o->call_depth), 0); + if (add_cacheinfo(o, mfi.mode, &mfi.oid, path, + 0, (!o->call_depth), 0)) + return -1; return mfi.clean; } } else diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh index c9a1f783f5..3871807d09 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_success 'cherry-pick works with dirty renamed file' ' +test_expect_failure 'cherry-pick works with dirty renamed file' ' test_commit to-rename && git checkout -b unrelated && test_commit unrelated && @@ -150,9 +150,8 @@ test_expect_success 'cherry-pick works with dirty renamed file' ' test_tick && git commit -m renamed && echo modified >renamed && - test_must_fail git cherry-pick refs/heads/unrelated >out && - test_i18ngrep "Refusing to lose dirty file at renamed" out && - test $(git rev-parse :0:renamed) = $(git rev-parse HEAD^:to-rename.t) && + git cherry-pick refs/heads/unrelated >out && + test $(git rev-parse :0:renamed) = $(git rev-parse HEAD~2:to-rename.t) && grep -q "^modified$" renamed ' -- cgit 1.2.3-korg From bd42380ef1ec773d1557b28ba6924f4020333143 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:17 -0700 Subject: merge-recursive: move more is_dirty handling to merge_content conflict_rename_normal() was doing some handling for dirty files that more naturally belonged in merge_content. Move it, and rename a parameter for clarity while at it. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index d3771e3626..bffcd5fa51 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2727,7 +2727,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, + int is_dirty, struct object_id *o_oid, int o_mode, struct object_id *a_oid, int a_mode, struct object_id *b_oid, int b_mode, @@ -2803,7 +2803,7 @@ static int merge_content(struct merge_options *o, return -1; } - if (df_conflict_remains || file_in_way) { + if (df_conflict_remains || is_dirty) { char *new_path; if (o->call_depth) { remove_file_from_cache(path); @@ -2825,6 +2825,10 @@ static int merge_content(struct merge_options *o, } new_path = unique_path(o, path, rename_conflict_info->branch1); + if (is_dirty) { + output(o, 1, _("Refusing to lose dirty file at %s"), + path); + } output(o, 1, _("Adding as %s instead"), new_path); if (update_file(o, 0, &mfi.oid, mfi.mode, new_path)) { free(new_path); @@ -2834,7 +2838,7 @@ static int merge_content(struct merge_options *o, mfi.clean = 0; } else if (update_file(o, mfi.clean, &mfi.oid, mfi.mode, path)) return -1; - return mfi.clean; + return !is_dirty && mfi.clean; } static int conflict_rename_normal(struct merge_options *o, @@ -2844,21 +2848,10 @@ static int conflict_rename_normal(struct merge_options *o, 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; + return merge_content(o, path, was_dirty(o, path), + o_oid, o_mode, a_oid, a_mode, b_oid, b_mode, + ci); } /* Per entry merge function */ @@ -2981,7 +2974,8 @@ 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, 0 /* file_in_way */, + int is_dirty = 0; /* unpack_trees would have bailed if dirty */ + clean_merge = merge_content(o, path, is_dirty, o_oid, o_mode, a_oid, a_mode, b_oid, b_mode, NULL); } else if (!o_oid && !a_oid && !b_oid) { -- cgit 1.2.3-korg From 2f682e21a6dc3b7403fd0162252183fdef019274 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:18 -0700 Subject: merge-recursive: avoid triggering add_cacheinfo error with dirty mod If a cherry-pick or merge with a rename results in a skippable update (due to the merged content matching what HEAD already had), but the working directory is dirty, avoid trying to refresh the index as that will fail. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 2 +- t/t3501-revert-cherry-pick.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index bffcd5fa51..804dfefd15 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2786,7 +2786,7 @@ static int merge_content(struct merge_options *o, path_renamed_outside_HEAD = !path2 || !strcmp(path, path2); if (!path_renamed_outside_HEAD) { if (add_cacheinfo(o, mfi.mode, &mfi.oid, path, - 0, (!o->call_depth), 0)) + 0, (!o->call_depth && !is_dirty), 0)) return -1; return mfi.clean; } diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh index 3871807d09..d1c68af8c5 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 && -- cgit 1.2.3-korg From a35edc84bdd6134d7aaa7bb0d220941154fe9e7e Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:20 -0700 Subject: merge-recursive: fix was_tracked() to quit lying with some renamed paths In commit aacb82de3ff8 ("merge-recursive: Split was_tracked() out of would_lose_untracked()", 2011-08-11), was_tracked() was split out of would_lose_untracked() with the intent to provide a function that could answer whether a path was tracked in the index before the merge. Sadly, it instead returned whether the path was in the working tree due to having been tracked in the index before the merge OR having been written there by unpack_trees(). The distinction is important when renames are involved, e.g. for a merge where: HEAD: modifies path b other: renames b->c In this case, c was not tracked in the index before the merge, but would have been added to the index at stage 0 and written to the working tree by unpack_trees(). would_lose_untracked() is more interested in the in-working-copy-for-either-reason behavior, while all other uses of was_tracked() want just was-it-tracked-in-index-before-merge behavior. Unsplit would_lose_untracked() and write a new was_tracked() function which answers whether a path was tracked in the index before the merge started. This will also affect was_dirty(), helping it to return better results since it can base answers off the original index rather than an index that possibly only copied over some of the stat information. However, was_dirty() will need an additional change that will be made in a subsequent patch. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 91 ++++++++++++++++++++++++++++++++++++++++--------------- merge-recursive.h | 1 + 2 files changed, 68 insertions(+), 24 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 804dfefd15..f7478622f2 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -344,6 +344,7 @@ static int git_merge_trees(struct merge_options *o, { int rc; struct tree_desc t[3]; + struct index_state tmp_index = { NULL }; memset(&o->unpack_opts, 0, sizeof(o->unpack_opts)); if (o->call_depth) @@ -354,7 +355,7 @@ static int git_merge_trees(struct merge_options *o, 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; + o->unpack_opts.dst_index = &tmp_index; setup_unpack_trees_porcelain(&o->unpack_opts, "merge"); init_tree_desc_from_tree(t+0, common); @@ -362,13 +363,18 @@ static int git_merge_trees(struct merge_options *o, init_tree_desc_from_tree(t+2, merge); rc = unpack_trees(3, t, &o->unpack_opts); + cache_tree_free(&active_cache_tree); + /* - * 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. + * Update the_index to match the new results, AFTER saving a copy + * in o->orig_index. Update src_index to point to the saved copy. + * (verify_uptodate() checks src_index, and the original index is + * the one that had the necessary modification timestamps.) */ - o->unpack_opts.src_index = &the_index; - cache_tree_free(&active_cache_tree); + o->orig_index = the_index; + the_index = tmp_index; + o->unpack_opts.src_index = &o->orig_index; + return rc; } @@ -773,31 +779,59 @@ static int dir_in_way(const char *path, int check_working_copy, int empty_ok) !(empty_ok && is_empty_dir(path)); } -static int was_tracked(const char *path) +/* + * Returns whether path was tracked in the index before the merge started + */ +static int was_tracked(struct merge_options *o, const char *path) { - int pos = cache_name_pos(path, strlen(path)); + int pos = index_name_pos(&o->orig_index, path, strlen(path)); if (0 <= pos) - /* we have been tracking this path */ + /* we were tracking this path before the merge */ return 1; - /* - * Look for an unmerged entry for the path, - * specifically stage #2, which would indicate - * that "our" side before the merge started - * had the path tracked (and resulted in a conflict). - */ - for (pos = -1 - pos; - pos < active_nr && !strcmp(path, active_cache[pos]->name); - pos++) - if (ce_stage(active_cache[pos]) == 2) - return 1; return 0; } static int would_lose_untracked(const char *path) { - return !was_tracked(path) && file_exists(path); + /* + * This may look like it can be simplified to: + * return !was_tracked(o, path) && file_exists(path) + * but it can't. This function needs to know whether path was in + * the working tree due to EITHER having been tracked in the index + * before the merge OR having been put into the working copy and + * index by unpack_trees(). Due to that either-or requirement, we + * check the current index instead of the original one. + * + * Note that we do not need to worry about merge-recursive itself + * updating the index after unpack_trees() and before calling this + * function, because we strictly require all code paths in + * merge-recursive to update the working tree first and the index + * second. Doing otherwise would break + * update_file()/would_lose_untracked(); see every comment in this + * file which mentions "update_stages". + */ + int pos = cache_name_pos(path, strlen(path)); + + if (pos < 0) + pos = -1 - pos; + while (pos < active_nr && + !strcmp(path, active_cache[pos]->name)) { + /* + * If stage #0, it is definitely tracked. + * If it has stage #2 then it was tracked + * before this merge started. All other + * cases the path was not tracked. + */ + switch (ce_stage(active_cache[pos])) { + case 0: + case 2: + return 0; + } + pos++; + } + return file_exists(path); } static int was_dirty(struct merge_options *o, const char *path) @@ -805,7 +839,7 @@ 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)) + if (o->call_depth || !was_tracked(o, path)) return !dirty; ce = cache_file_exists(path, strlen(path), ignore_case); @@ -2419,7 +2453,7 @@ static int process_renames(struct merge_options *o, * add-source case). */ remove_file(o, 1, ren1_src, - renamed_stage == 2 || !was_tracked(ren1_src)); + renamed_stage == 2 || !was_tracked(o, ren1_src)); oidcpy(&src_other.oid, &ren1->src_entry->stages[other_stage].oid); @@ -2812,7 +2846,7 @@ static int merge_content(struct merge_options *o, if (update_stages(o, path, &one, &a, &b)) return -1; } else { - int file_from_stage2 = was_tracked(path); + int file_from_stage2 = was_tracked(o, path); struct diff_filespec merged; oidcpy(&merged.oid, &mfi.oid); merged.mode = mfi.mode; @@ -3081,6 +3115,15 @@ int merge_trees(struct merge_options *o, else clean = 1; + /* Free the extra index left from git_merge_trees() */ + /* + * FIXME: Need to also free data allocated by + * setup_unpack_trees_porcelain() tucked away in o->unpack_opts.msgs, + * but the problem is that only half of it refers to dynamically + * allocated data, while the other half points at static strings. + */ + discard_index(&o->orig_index); + if (o->call_depth && !(*result = write_tree_from_memory(o))) return -1; diff --git a/merge-recursive.h b/merge-recursive.h index d863cf8867..248093e407 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -29,6 +29,7 @@ struct merge_options { struct hashmap current_file_dir_set; struct string_list df_conflict_file_set; struct unpack_trees_options unpack_opts; + struct index_state orig_index; }; /* -- cgit 1.2.3-korg From 277292d5ae27993d36f35fdd8d561c369b1e0962 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:21 -0700 Subject: merge-recursive: fix remainder of was_dirty() to use original index was_dirty() uses was_tracked(), which has been updated to use the original index rather than the current one. However, was_dirty() also had a separate call to cache_file_exists(), causing it to still implicitly use the current index. Update that to instead use index_file_exists(). Also, was_dirty() had a hack where it would mark any file as non-dirty if we simply didn't know its modification time. This was due to using the current index rather than the original index, because D/F conflicts and such would cause unpack_trees() to not copy the modification times from the original index to the current one. Now that we are using the original index, we can dispense with this hack. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index f7478622f2..9015be55be 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -842,9 +842,9 @@ static int was_dirty(struct merge_options *o, const char *path) if (o->call_depth || !was_tracked(o, 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); + ce = index_file_exists(o->unpack_opts.src_index, + path, strlen(path), ignore_case); + dirty = verify_uptodate(ce, &o->unpack_opts) != 0; return dirty; } -- cgit 1.2.3-korg From 05cf21eba29d18eb6cdece25613c6b1b036872d0 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:22 -0700 Subject: merge-recursive: make "Auto-merging" comment show for other merges Previously, merge_content() would print "Auto-merging" whenever the final content and mode aren't already available from HEAD. There are a few problems with this: 1) There are other code paths doing merges that should probably have the same message printed, in particular rename/rename(2to1) which cannot call into the normal rename logic. 2) If both sides of the merge have modifications, then a content merge is needed. It may turn out that the end result matches one of the sides (because the other only had a subset of the same changes), but the merge was still needed. Currently, the message will not print in that case, though it seems like it should. Move the printing of this message to merge_file_1() in order to address both issues. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 65 +++++++++++++++++++++++++++++++++---------------------- 1 file changed, 39 insertions(+), 26 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 9015be55be..f9021dd650 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1063,12 +1063,13 @@ static int merge_3way(struct merge_options *o, } static int merge_file_1(struct merge_options *o, - const struct diff_filespec *one, - const struct diff_filespec *a, - const struct diff_filespec *b, - const char *branch1, - const char *branch2, - struct merge_file_info *result) + const struct diff_filespec *one, + const struct diff_filespec *a, + const struct diff_filespec *b, + const char *filename, + const char *branch1, + const char *branch2, + struct merge_file_info *result) { result->merge = 0; result->clean = 1; @@ -1148,18 +1149,22 @@ static int merge_file_1(struct merge_options *o, die("BUG: unsupported object type in the tree"); } + if (result->merge) + output(o, 2, _("Auto-merging %s"), filename); + return 0; } static int merge_file_special_markers(struct merge_options *o, - const struct diff_filespec *one, - const struct diff_filespec *a, - const struct diff_filespec *b, - const char *branch1, - const char *filename1, - const char *branch2, - const char *filename2, - struct merge_file_info *mfi) + const struct diff_filespec *one, + const struct diff_filespec *a, + const struct diff_filespec *b, + const char *target_filename, + const char *branch1, + const char *filename1, + const char *branch2, + const char *filename2, + struct merge_file_info *mfi) { char *side1 = NULL; char *side2 = NULL; @@ -1170,22 +1175,23 @@ static int merge_file_special_markers(struct merge_options *o, if (filename2) side2 = xstrfmt("%s:%s", branch2, filename2); - ret = merge_file_1(o, one, a, b, + ret = merge_file_1(o, one, a, b, target_filename, side1 ? side1 : branch1, side2 ? side2 : branch2, mfi); + free(side1); free(side2); return ret; } static int merge_file_one(struct merge_options *o, - const char *path, - const struct object_id *o_oid, int o_mode, - const struct object_id *a_oid, int a_mode, - const struct object_id *b_oid, int b_mode, - const char *branch1, - const char *branch2, - struct merge_file_info *mfi) + const char *path, + const struct object_id *o_oid, int o_mode, + const struct object_id *a_oid, int a_mode, + const struct object_id *b_oid, int b_mode, + const char *branch1, + const char *branch2, + struct merge_file_info *mfi) { struct diff_filespec one, a, b; @@ -1196,7 +1202,7 @@ static int merge_file_one(struct merge_options *o, a.mode = a_mode; oidcpy(&b.oid, b_oid); b.mode = b_mode; - return merge_file_1(o, &one, &a, &b, branch1, branch2, mfi); + return merge_file_1(o, &one, &a, &b, path, branch1, branch2, mfi); } static int conflict_rename_dir(struct merge_options *o, @@ -1474,6 +1480,8 @@ static int conflict_rename_rename_2to1(struct merge_options *o, struct diff_filespec *c1 = ci->pair1->two; struct diff_filespec *c2 = ci->pair2->two; char *path = c1->path; /* == c2->path */ + char *path_side_1_desc; + char *path_side_2_desc; struct merge_file_info mfi_c1; struct merge_file_info mfi_c2; int ret; @@ -1487,13 +1495,19 @@ static int conflict_rename_rename_2to1(struct merge_options *o, remove_file(o, 1, a->path, o->call_depth || would_lose_untracked(a->path)); remove_file(o, 1, b->path, o->call_depth || would_lose_untracked(b->path)); + path_side_1_desc = xstrfmt("%s (was %s)", path, a->path); + path_side_2_desc = xstrfmt("%s (was %s)", path, b->path); if (merge_file_special_markers(o, a, c1, &ci->ren1_other, + path_side_1_desc, o->branch1, c1->path, o->branch2, ci->ren1_other.path, &mfi_c1) || merge_file_special_markers(o, b, &ci->ren2_other, c2, + path_side_2_desc, o->branch1, ci->ren2_other.path, o->branch2, c2->path, &mfi_c2)) return -1; + free(path_side_1_desc); + free(path_side_2_desc); if (o->call_depth) { /* @@ -2802,7 +2816,7 @@ static int merge_content(struct merge_options *o, S_ISGITLINK(pair1->two->mode))) df_conflict_remains = 1; } - if (merge_file_special_markers(o, &one, &a, &b, + if (merge_file_special_markers(o, &one, &a, &b, path, o->branch1, path1, o->branch2, path2, &mfi)) return -1; @@ -2824,8 +2838,7 @@ static int merge_content(struct merge_options *o, return -1; return mfi.clean; } - } else - output(o, 2, _("Auto-merging %s"), path); + } if (!mfi.clean) { if (S_ISGITLINK(mfi.mode)) -- cgit 1.2.3-korg From 1de70dbd1ada0069d1b6cd6345323906cc9a9ed3 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:23 -0700 Subject: merge-recursive: fix check for skipability of working tree updates The can-working-tree-updates-be-skipped check has had a long and blemished history. The update can be skipped iff: a) The merge is clean b) The merge matches what was in HEAD (content, mode, pathname) c) The target path is usable (i.e. not involved in D/F conflict) Traditionally, we split b into parts: b1) The merged result matches the content and mode found in HEAD b2) The merged target path existed in HEAD Steps a & b1 are easy to check; we have always gotten those right. While it is easy to overlook step c, this was fixed seven years ago with commit 4ab9a157d069 ("merge_content(): Check whether D/F conflicts are still present", 2010-09-20). merge-recursive didn't have a readily available way to directly check step b2, so various approximations were used: * In commit b2c8c0a76274 ("merge-recursive: When we detect we can skip an update, actually skip it", 2011-02-28), it was noted that although the code claimed it was skipping the update, it did not actually skip the update. The code was made to skip it, but used lstat(path, ...) as an approximation to path-was-tracked-in-index-before-merge. * In commit 5b448b853030 ("merge-recursive: When we detect we can skip an update, actually skip it", 2011-08-11), the problem with using lstat was noted. It was changed to the approximation path2 && strcmp(path, path2) which is also wrong. !path2 || strcmp(path, path2) would have been better, but would have fallen short with directory renames. * In c5b761fb2711 ("merge-recursive: ensure we write updates for directory-renamed file", 2018-02-14), the problem with the previous approximation was noted and changed to was_tracked(path) That looks close to what we were trying to answer, but was_tracked() as implemented at the time should have been named is_tracked(); it returned something different than what we were looking for. * To make matters more complex, fixing was_tracked() isn't sufficient because the splitting of b into b1 and b2 is wrong. Consider the following merge with a rename/add conflict: side A: modify foo, add unrelated bar side B: rename foo->bar (but don't modify the mode or contents) In this case, the three-way merge of original foo, A's foo, and B's bar will result in a desired pathname of bar with the same mode/contents that A had for foo. Thus, A had the right mode and contents for the file, and it had the right pathname present (namely, bar), but the bar that was present was unrelated to the contents, so the working tree update was not skippable. Fix this by introducing a new function: was_tracked_and_matches(o, path, &mfi.oid, mfi.mode) and use it to directly check for condition b. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 48 ++++++++++++++++++++++------------ t/t6022-merge-rename.sh | 2 +- t/t6043-merge-rename-directories.sh | 2 +- t/t6046-merge-skip-unneeded-updates.sh | 10 +++---- 4 files changed, 39 insertions(+), 23 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index f9021dd650..13b4762971 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -779,6 +779,25 @@ static int dir_in_way(const char *path, int check_working_copy, int empty_ok) !(empty_ok && is_empty_dir(path)); } +/* + * Returns whether path was tracked in the index before the merge started, + * and its oid and mode match the specified values + */ +static int was_tracked_and_matches(struct merge_options *o, const char *path, + const struct object_id *oid, unsigned mode) +{ + int pos = index_name_pos(&o->orig_index, path, strlen(path)); + struct cache_entry *ce; + + if (0 > pos) + /* we were not tracking this path before the merge */ + return 0; + + /* See if the file we were tracking before matches */ + ce = o->orig_index.cache[pos]; + return (oid_eq(&ce->oid, oid) && ce->ce_mode == mode); +} + /* * Returns whether path was tracked in the index before the merge started */ @@ -2821,23 +2840,20 @@ static int merge_content(struct merge_options *o, o->branch2, path2, &mfi)) return -1; - if (mfi.clean && !df_conflict_remains && - oid_eq(&mfi.oid, a_oid) && mfi.mode == a_mode) { - int path_renamed_outside_HEAD; + /* + * We can skip updating the working tree file iff: + * a) The merge is clean + * b) The merge matches what was in HEAD (content, mode, pathname) + * c) The target path is usable (i.e. not involved in D/F conflict) + */ + if (mfi.clean && + was_tracked_and_matches(o, path, &mfi.oid, mfi.mode) && + !df_conflict_remains) { output(o, 3, _("Skipped %s (merged same as existing)"), path); - /* - * The content merge resulted in the same file contents we - * already had. We can return early if those file contents - * are recorded at the correct path (which may not be true - * if the merge involves a rename). - */ - path_renamed_outside_HEAD = !path2 || !strcmp(path, path2); - if (!path_renamed_outside_HEAD) { - if (add_cacheinfo(o, mfi.mode, &mfi.oid, path, - 0, (!o->call_depth && !is_dirty), 0)) - return -1; - return mfi.clean; - } + if (add_cacheinfo(o, mfi.mode, &mfi.oid, path, + 0, (!o->call_depth && !is_dirty), 0)) + return -1; + return mfi.clean; } if (!mfi.clean) { diff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh index a1fad6980b..6df2650c03 100755 --- a/t/t6022-merge-rename.sh +++ b/t/t6022-merge-rename.sh @@ -247,7 +247,7 @@ test_expect_success 'merge of identical changes in a renamed file' ' git reset --hard HEAD^ && git checkout change && GIT_MERGE_VERBOSITY=3 git merge change+rename >out && - test_i18ngrep "^Skipped B" out + test_i18ngrep ! "^Skipped B" out ' test_expect_success 'setup for rename + d/f conflicts' ' diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index 45f620633f..2e28f2908d 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -3884,7 +3884,7 @@ test_expect_success '12b-setup: Moving one directory hierarchy into another' ' ) ' -test_expect_failure '12b-check: Moving one directory hierarchy into another' ' +test_expect_success '12b-check: Moving one directory hierarchy into another' ' ( cd 12b && diff --git a/t/t6046-merge-skip-unneeded-updates.sh b/t/t6046-merge-skip-unneeded-updates.sh index a0f482e232..fcefffcaec 100755 --- a/t/t6046-merge-skip-unneeded-updates.sh +++ b/t/t6046-merge-skip-unneeded-updates.sh @@ -64,7 +64,7 @@ test_expect_success '1a-setup: Modify(A)/Modify(B), change on B subset of A' ' ) ' -test_expect_failure '1a-check-L: Modify(A)/Modify(B), change on B subset of A' ' +test_expect_success '1a-check-L: Modify(A)/Modify(B), change on B subset of A' ' test_when_finished "git -C 1a reset --hard" && test_when_finished "git -C 1a clean -fd" && ( @@ -160,7 +160,7 @@ test_expect_success '2a-setup: Modify(A)/rename(B)' ' ) ' -test_expect_failure '2a-check-L: Modify/rename, merge into modify side' ' +test_expect_success '2a-check-L: Modify/rename, merge into modify side' ' test_when_finished "git -C 2a reset --hard" && test_when_finished "git -C 2a clean -fd" && ( @@ -360,7 +360,7 @@ test_expect_success '2c-setup: Modify b & add c VS rename b->c' ' ) ' -test_expect_failure '2c-check: Modify b & add c VS rename b->c' ' +test_expect_success '2c-check: Modify b & add c VS rename b->c' ' ( cd 2c && @@ -456,7 +456,7 @@ test_expect_success '3a-setup: bq_1->foo/bq_2 on A, foo/->bar/ on B' ' ) ' -test_expect_failure '3a-check-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' ' +test_expect_success '3a-check-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' ' test_when_finished "git -C 3a reset --hard" && test_when_finished "git -C 3a clean -fd" && ( @@ -579,7 +579,7 @@ test_expect_success '3b-check-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' ' ) ' -test_expect_failure '3b-check-R: bq_1->foo/bq_2 on A, foo/->bar/ on B' ' +test_expect_success '3b-check-R: bq_1->foo/bq_2 on A, foo/->bar/ on B' ' test_when_finished "git -C 3b reset --hard" && test_when_finished "git -C 3b clean -fd" && ( -- cgit 1.2.3-korg