From 52396e1d3dd1417f042aa5269399bdcffe0a6c66 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Wed, 19 Sep 2018 09:14:31 -0700 Subject: merge-recursive: set paths correctly when three-way merging content merge_3way() has code to mark different sides of the conflict with info about where the content comes from. If the names of the files involved match, it simply uses the branch name. If the names of the files do not match, it uses branchname:filename. Unfortunately, merge_content() previously always called it with one.path = a.path = b.path. Granted, it didn't have other path information available to it for years, but that was corrected by passing rename_conflict_info in commit 3c217c077a86 ("merge-recursive: Provide more info in conflict markers with file renames", 2011-08-11). In that commit, instead of just fixing the bug with the pathnames, it created fake branch names incorporating both the branch name and file name. This "fake branch" workaround was extended further when I pulled that logic out into a special function in commit dac4741554e7 ("merge-recursive: Create function for merging with branchname:file markers", 2011-08-11), and a number of other sites outside of merge_content() have been added which call into that. However, this Rube-Goldberg-esque setup is not merely duplicate code and unnecessary work, it also risked having other callsites invoke it in a way that would result in markers of the form branchname:filename:filename (i.e. with the filename repeated). Fix this whole mess by: - setting one.path, a.path, and b.path appropriately - calling merge_file_1() directly - deleting the merge_file_special_markers() workaround wrapper Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 49 +++++++++---------------------------------------- 1 file changed, 9 insertions(+), 40 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 45a163c555..99a7ac5ec7 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1366,35 +1366,6 @@ static int merge_file_1(struct merge_options *o, 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 *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; - int ret; - - if (filename1) - side1 = xstrfmt("%s:%s", branch1, filename1); - if (filename2) - side2 = xstrfmt("%s:%s", branch2, filename2); - - 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, @@ -1729,14 +1700,10 @@ static int handle_rename_rename_2to1(struct merge_options *o, 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)) + if (merge_file_1(o, a, c1, &ci->ren1_other, path_side_1_desc, + o->branch1, o->branch2, &mfi_c1) || + merge_file_1(o, b, &ci->ren2_other, c2, path_side_2_desc, + o->branch1, o->branch2, &mfi_c2)) return -1; free(path_side_1_desc); free(path_side_2_desc); @@ -3059,14 +3026,16 @@ static int merge_content(struct merge_options *o, path2 = (rename_conflict_info->pair2 || o->branch2 == rename_conflict_info->branch1) ? pair1->two->path : pair1->one->path; + one.path = pair1->one->path; + a.path = (char *)path1; + b.path = (char *)path2; if (dir_in_way(path, !o->call_depth, S_ISGITLINK(pair1->two->mode))) df_conflict_remains = 1; } - if (merge_file_special_markers(o, &one, &a, &b, path, - o->branch1, path1, - o->branch2, path2, &mfi)) + if (merge_file_1(o, &one, &a, &b, path, + o->branch1, o->branch2, &mfi)) return -1; /* -- cgit v1.2.1 From 75f3fa79c39a39fd6ca5e4bae73df4073ca97bd4 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Wed, 19 Sep 2018 09:14:32 -0700 Subject: merge-recursive: avoid wrapper function when unnecessary and wasteful merge_file_one() is a convenience function taking a bunch of oids and modes, combining each pair into a diff_filespec, and then calling merge_file_1(). When we already start with diff_filespec's, we can just call merge_file_1() directly instead of splitting out the oids and modes for the wrapper to recombine into what we already had. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 99a7ac5ec7..9e4e3da672 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1630,10 +1630,7 @@ static int handle_rename_rename_1to2(struct merge_options *o, struct merge_file_info mfi; struct diff_filespec other; struct diff_filespec *add; - if (merge_file_one(o, one->path, - &one->oid, one->mode, - &a->oid, a->mode, - &b->oid, b->mode, + if (merge_file_1(o, one, a, b, one->path, ci->branch1, ci->branch2, &mfi)) return -1; -- cgit v1.2.1 From 0270a07ad0b22734920fb92f58052b213c2f9bc8 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Wed, 19 Sep 2018 09:14:33 -0700 Subject: merge-recursive: remove final remaining caller of merge_file_one() The function names merge_file_one() and merge_file_1() aren't particularly intuitive function names, especially since there is no associated merge_file() function that these are related to. The previous commit showed that merge_file_one() was prone to be called when merge_file_1() should be, and since it is just a thin wrapper around merge_file_1() anyway and only has one caller left, let's just remove merge_file_one() entirely. (It also turns out that the one remaining caller of merge_file_one() has very broken code that needs to be completely rewritten, but that's the subject of a future patch series; for now, we're just translating it into a merge_file_1() call.) Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 44 +++++++++++++++++--------------------------- 1 file changed, 17 insertions(+), 27 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 9e4e3da672..2654a8a485 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1366,27 +1366,6 @@ static int merge_file_1(struct merge_options *o, return 0; } -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) -{ - struct diff_filespec one, a, b; - - one.path = a.path = b.path = (char *)path; - oidcpy(&one.oid, o_oid); - one.mode = o_mode; - oidcpy(&a.oid, a_oid); - a.mode = a_mode; - oidcpy(&b.oid, b_oid); - b.mode = b_mode; - return merge_file_1(o, &one, &a, &b, path, branch1, branch2, mfi); -} - static int handle_rename_via_dir(struct merge_options *o, struct diff_filepair *pair, const char *rename_branch, @@ -2730,12 +2709,23 @@ static int process_renames(struct merge_options *o, ren1_dst, branch2); if (o->call_depth) { struct merge_file_info mfi; - if (merge_file_one(o, ren1_dst, &null_oid, 0, - &ren1->pair->two->oid, - ren1->pair->two->mode, - &dst_other.oid, - dst_other.mode, - branch1, branch2, &mfi)) { + struct diff_filespec one, a, b; + + oidcpy(&one.oid, &null_oid); + one.mode = 0; + one.path = ren1->pair->two->path; + + oidcpy(&a.oid, &ren1->pair->two->oid); + a.mode = ren1->pair->two->mode; + a.path = one.path; + + oidcpy(&b.oid, &dst_other.oid); + b.mode = dst_other.mode; + b.path = one.path; + + if (merge_file_1(o, &one, &a, &b, ren1_dst, + branch1, branch2, + &mfi)) { clean_merge = -1; goto cleanup_and_return; } -- cgit v1.2.1 From d95735567550e87c8e58a92126c45f1a3875603a Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Wed, 19 Sep 2018 09:14:34 -0700 Subject: merge-recursive: rename merge_file_1() and merge_content() Summary: merge_file_1() -> merge_mode_and_contents() merge_content() -> handle_content_merge() merge_file_1() is a very unhelpful name. Rename it to merge_mode_and_contents() to reflect what it does. merge_content() calls merge_mode_and_contents() to do the main part of its work, but most of this function was about higher level stuff, e.g. printing out conflict messages, updating skip_worktree bits, checking for ability to avoid updating the working tree or for D/F conflicts being in the way, etc. Since there are several handle_*() functions for similar levels of checking and handling in merge-recursive.c (e.g. handle_change_delete(), handle_rename_rename_2to1()), let's rename this function to handle_content_merge(). Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 66 ++++++++++++++++++++++++++++--------------------------- 1 file changed, 34 insertions(+), 32 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 2654a8a485..5206d6cfb6 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1274,14 +1274,14 @@ static int merge_submodule(struct merge_options *o, return 0; } -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 *filename, - const char *branch1, - const char *branch2, - struct merge_file_info *result) +static int merge_mode_and_contents(struct merge_options *o, + 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; @@ -1609,8 +1609,8 @@ static int handle_rename_rename_1to2(struct merge_options *o, struct merge_file_info mfi; struct diff_filespec other; struct diff_filespec *add; - if (merge_file_1(o, one, a, b, one->path, - ci->branch1, ci->branch2, &mfi)) + if (merge_mode_and_contents(o, one, a, b, one->path, + ci->branch1, ci->branch2, &mfi)) return -1; /* @@ -1676,10 +1676,10 @@ static int handle_rename_rename_2to1(struct merge_options *o, 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_1(o, a, c1, &ci->ren1_other, path_side_1_desc, - o->branch1, o->branch2, &mfi_c1) || - merge_file_1(o, b, &ci->ren2_other, c2, path_side_2_desc, - o->branch1, o->branch2, &mfi_c2)) + if (merge_mode_and_contents(o, a, c1, &ci->ren1_other, path_side_1_desc, + o->branch1, o->branch2, &mfi_c1) || + merge_mode_and_contents(o, b, &ci->ren2_other, c2, path_side_2_desc, + o->branch1, o->branch2, &mfi_c2)) return -1; free(path_side_1_desc); free(path_side_2_desc); @@ -2723,9 +2723,9 @@ static int process_renames(struct merge_options *o, b.mode = dst_other.mode; b.path = one.path; - if (merge_file_1(o, &one, &a, &b, ren1_dst, - branch1, branch2, - &mfi)) { + if (merge_mode_and_contents(o, &one, &a, &b, ren1_dst, + branch1, branch2, + &mfi)) { clean_merge = -1; goto cleanup_and_return; } @@ -2975,13 +2975,13 @@ static int handle_modify_delete(struct merge_options *o, _("modify"), _("modified")); } -static int merge_content(struct merge_options *o, - const char *path, - 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, - struct rename_conflict_info *rename_conflict_info) +static int handle_content_merge(struct merge_options *o, + const char *path, + 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, + struct rename_conflict_info *rename_conflict_info) { const char *reason = _("content"); const char *path1 = NULL, *path2 = NULL; @@ -3021,8 +3021,8 @@ static int merge_content(struct merge_options *o, S_ISGITLINK(pair1->two->mode))) df_conflict_remains = 1; } - if (merge_file_1(o, &one, &a, &b, path, - o->branch1, o->branch2, &mfi)) + if (merge_mode_and_contents(o, &one, &a, &b, path, + o->branch1, o->branch2, &mfi)) return -1; /* @@ -3113,9 +3113,9 @@ static int handle_rename_normal(struct merge_options *o, struct rename_conflict_info *ci) { /* Merge the content and write it out */ - return merge_content(o, path, was_dirty(o, path), - o_oid, o_mode, a_oid, a_mode, b_oid, b_mode, - ci); + return handle_content_merge(o, path, was_dirty(o, path), + o_oid, o_mode, a_oid, a_mode, b_oid, b_mode, + ci); } /* Per entry merge function */ @@ -3239,9 +3239,11 @@ static int process_entry(struct merge_options *o, /* Case C: Added in both (check for same permissions) and */ /* case D: Modified in both, but differently. */ 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); + clean_merge = handle_content_merge(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) { /* * this entry was deleted altogether. a_mode == 0 means -- cgit v1.2.1