{"thread":{"id":"49382","subject":"[PATCH 4/4] merge-recursive: rename merge_file_1() and merge_content()","startedAt":"2018-09-19T16:14:41Z","lastAt":"2018-09-19T23:40:57Z","messageCount":6,"participants":["Elijah Newren","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"358451","messageId":"20180919161434.3272-5-newren@gmail.com","threadId":"49382","inReplyTo":"20180919161434.3272-1-newren@gmail.com","subject":"[PATCH 4/4] merge-recursive: rename merge_file_1() and merge_content()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-09-19T16:14:34Z","receivedAt":"2018-09-19T16:14:41Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Summary:\n  merge_file_1()  -> merge_mode_and_contents()\n  merge_content() -> handle_content_merge()\n\nmerge_file_1() is a very unhelpful name.  Rename it to\nmerge_mode_and_contents() to reflect what it does.\n\nmerge_content() calls merge_mode_and_contents() to do the main part of\nits work, but most of this function was about higher level stuff, e.g.\nprinting out conflict messages, updating skip_worktree bits, checking\nfor ability to avoid updating the working tree or for D/F conflicts\nbeing in the way, etc.  Since there are several handle_*() functions for\nsimilar levels of checking and handling in merge-recursive.c (e.g.\nhandle_change_delete(), handle_rename_rename_2to1()), let's rename this\nfunction to handle_content_merge().\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n merge-recursive.c | 66 ++++++++++++++++++++++++-----------------------\n 1 file changed, 34 insertions(+), 32 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 2654a8a485..5206d6cfb6 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1274,14 +1274,14 @@ static int merge_submodule(struct merge_options *o,\n \treturn 0;\n }\n \n-static int merge_file_1(struct merge_options *o,\n-\t\t\tconst struct diff_filespec *one,\n-\t\t\tconst struct diff_filespec *a,\n-\t\t\tconst struct diff_filespec *b,\n-\t\t\tconst char *filename,\n-\t\t\tconst char *branch1,\n-\t\t\tconst char *branch2,\n-\t\t\tstruct merge_file_info *result)\n+static int merge_mode_and_contents(struct merge_options *o,\n+\t\t\t\t   const struct diff_filespec *one,\n+\t\t\t\t   const struct diff_filespec *a,\n+\t\t\t\t   const struct diff_filespec *b,\n+\t\t\t\t   const char *filename,\n+\t\t\t\t   const char *branch1,\n+\t\t\t\t   const char *branch2,\n+\t\t\t\t   struct merge_file_info *result)\n {\n \tresult->merge = 0;\n \tresult->clean = 1;\n@@ -1609,8 +1609,8 @@ static int handle_rename_rename_1to2(struct merge_options *o,\n \t\tstruct merge_file_info mfi;\n \t\tstruct diff_filespec other;\n \t\tstruct diff_filespec *add;\n-\t\tif (merge_file_1(o, one, a, b, one->path,\n-\t\t\t\t ci->branch1, ci->branch2, &mfi))\n+\t\tif (merge_mode_and_contents(o, one, a, b, one->path,\n+\t\t\t\t\t    ci->branch1, ci->branch2, &mfi))\n \t\t\treturn -1;\n \n \t\t/*\n@@ -1676,10 +1676,10 @@ static int handle_rename_rename_2to1(struct merge_options *o,\n \n \tpath_side_1_desc = xstrfmt(\"%s (was %s)\", path, a->path);\n \tpath_side_2_desc = xstrfmt(\"%s (was %s)\", path, b->path);\n-\tif (merge_file_1(o, a, c1, &ci->ren1_other, path_side_1_desc,\n-\t\t\t o->branch1, o->branch2, &mfi_c1) ||\n-\t    merge_file_1(o, b, &ci->ren2_other, c2, path_side_2_desc,\n-\t\t\t o->branch1, o->branch2, &mfi_c2))\n+\tif (merge_mode_and_contents(o, a, c1, &ci->ren1_other, path_side_1_desc,\n+\t\t\t\t    o->branch1, o->branch2, &mfi_c1) ||\n+\t    merge_mode_and_contents(o, b, &ci->ren2_other, c2, path_side_2_desc,\n+\t\t\t\t    o->branch1, o->branch2, &mfi_c2))\n \t\treturn -1;\n \tfree(path_side_1_desc);\n \tfree(path_side_2_desc);\n@@ -2723,9 +2723,9 @@ static int process_renames(struct merge_options *o,\n \t\t\t\t\tb.mode = dst_other.mode;\n \t\t\t\t\tb.path = one.path;\n \n-\t\t\t\t\tif (merge_file_1(o, &one, &a, &b, ren1_dst,\n-\t\t\t\t\t\t\t branch1, branch2,\n-\t\t\t\t\t\t\t &mfi)) {\n+\t\t\t\t\tif (merge_mode_and_contents(o, &one, &a, &b, ren1_dst,\n+\t\t\t\t\t\t\t\t    branch1, branch2,\n+\t\t\t\t\t\t\t\t    &mfi)) {\n \t\t\t\t\t\tclean_merge = -1;\n \t\t\t\t\t\tgoto cleanup_and_return;\n \t\t\t\t\t}\n@@ -2975,13 +2975,13 @@ static int handle_modify_delete(struct merge_options *o,\n \t\t\t\t    _(\"modify\"), _(\"modified\"));\n }\n \n-static int merge_content(struct merge_options *o,\n-\t\t\t const char *path,\n-\t\t\t int is_dirty,\n-\t\t\t struct object_id *o_oid, int o_mode,\n-\t\t\t struct object_id *a_oid, int a_mode,\n-\t\t\t struct object_id *b_oid, int b_mode,\n-\t\t\t struct rename_conflict_info *rename_conflict_info)\n+static int handle_content_merge(struct merge_options *o,\n+\t\t\t\tconst char *path,\n+\t\t\t\tint is_dirty,\n+\t\t\t\tstruct object_id *o_oid, int o_mode,\n+\t\t\t\tstruct object_id *a_oid, int a_mode,\n+\t\t\t\tstruct object_id *b_oid, int b_mode,\n+\t\t\t\tstruct rename_conflict_info *rename_conflict_info)\n {\n \tconst char *reason = _(\"content\");\n \tconst char *path1 = NULL, *path2 = NULL;\n@@ -3021,8 +3021,8 @@ static int merge_content(struct merge_options *o,\n \t\t\t       S_ISGITLINK(pair1->two->mode)))\n \t\t\tdf_conflict_remains = 1;\n \t}\n-\tif (merge_file_1(o, &one, &a, &b, path,\n-\t\t\t o->branch1, o->branch2, &mfi))\n+\tif (merge_mode_and_contents(o, &one, &a, &b, path,\n+\t\t\t\t    o->branch1, o->branch2, &mfi))\n \t\treturn -1;\n \n \t/*\n@@ -3113,9 +3113,9 @@ static int handle_rename_normal(struct merge_options *o,\n \t\t\t\tstruct rename_conflict_info *ci)\n {\n \t/* Merge the content and write it out */\n-\treturn merge_content(o, path, was_dirty(o, path),\n-\t\t\t     o_oid, o_mode, a_oid, a_mode, b_oid, b_mode,\n-\t\t\t     ci);\n+\treturn handle_content_merge(o, path, was_dirty(o, path),\n+\t\t\t\t    o_oid, o_mode, a_oid, a_mode, b_oid, b_mode,\n+\t\t\t\t    ci);\n }\n \n /* Per entry merge function */\n@@ -3239,9 +3239,11 @@ static int process_entry(struct merge_options *o,\n \t\t/* Case C: Added in both (check for same permissions) and */\n \t\t/* case D: Modified in both, but differently. */\n \t\tint is_dirty = 0; /* unpack_trees would have bailed if dirty */\n-\t\tclean_merge = merge_content(o, path, is_dirty,\n-\t\t\t\t\t    o_oid, o_mode, a_oid, a_mode, b_oid, b_mode,\n-\t\t\t\t\t    NULL);\n+\t\tclean_merge = handle_content_merge(o, path, is_dirty,\n+\t\t\t\t\t\t   o_oid, o_mode,\n+\t\t\t\t\t\t   a_oid, a_mode,\n+\t\t\t\t\t\t   b_oid, b_mode,\n+\t\t\t\t\t\t   NULL);\n \t} else if (!o_oid && !a_oid && !b_oid) {\n \t\t/*\n \t\t * this entry was deleted altogether. a_mode == 0 means\n-- \n2.19.0.12.gc6760fd9a9\n\n"},{"id":"358452","messageId":"20180919161434.3272-4-newren@gmail.com","threadId":"49382","inReplyTo":"20180919161434.3272-1-newren@gmail.com","subject":"[PATCH 3/4] merge-recursive: remove final remaining caller of merge_file_one()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-09-19T16:14:33Z","receivedAt":"2018-09-19T16:14:41Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"The function names merge_file_one() and merge_file_1() aren't\nparticularly intuitive function names, especially since there is no\nassociated merge_file() function that these are related to.  The\nprevious commit showed that merge_file_one() was prone to be called when\nmerge_file_1() should be, and since it is just a thin wrapper around\nmerge_file_1() anyway and only has one caller left, let's just remove\nmerge_file_one() entirely.\n\n(It also turns out that the one remaining caller of merge_file_one()\nhas very broken code that needs to be completely rewritten, but that's\nthe subject of a future patch series; for now, we're just translating\nit into a merge_file_1() call.)\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n merge-recursive.c | 44 +++++++++++++++++---------------------------\n 1 file changed, 17 insertions(+), 27 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 9e4e3da672..2654a8a485 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1366,27 +1366,6 @@ static int merge_file_1(struct merge_options *o,\n \treturn 0;\n }\n \n-static int merge_file_one(struct merge_options *o,\n-\t\t\t  const char *path,\n-\t\t\t  const struct object_id *o_oid, int o_mode,\n-\t\t\t  const struct object_id *a_oid, int a_mode,\n-\t\t\t  const struct object_id *b_oid, int b_mode,\n-\t\t\t  const char *branch1,\n-\t\t\t  const char *branch2,\n-\t\t\t  struct merge_file_info *mfi)\n-{\n-\tstruct diff_filespec one, a, b;\n-\n-\tone.path = a.path = b.path = (char *)path;\n-\toidcpy(&one.oid, o_oid);\n-\tone.mode = o_mode;\n-\toidcpy(&a.oid, a_oid);\n-\ta.mode = a_mode;\n-\toidcpy(&b.oid, b_oid);\n-\tb.mode = b_mode;\n-\treturn merge_file_1(o, &one, &a, &b, path, branch1, branch2, mfi);\n-}\n-\n static int handle_rename_via_dir(struct merge_options *o,\n \t\t\t\t struct diff_filepair *pair,\n \t\t\t\t const char *rename_branch,\n@@ -2730,12 +2709,23 @@ static int process_renames(struct merge_options *o,\n \t\t\t\t       ren1_dst, branch2);\n \t\t\t\tif (o->call_depth) {\n \t\t\t\t\tstruct merge_file_info mfi;\n-\t\t\t\t\tif (merge_file_one(o, ren1_dst, &null_oid, 0,\n-\t\t\t\t\t\t\t   &ren1->pair->two->oid,\n-\t\t\t\t\t\t\t   ren1->pair->two->mode,\n-\t\t\t\t\t\t\t   &dst_other.oid,\n-\t\t\t\t\t\t\t   dst_other.mode,\n-\t\t\t\t\t\t\t   branch1, branch2, &mfi)) {\n+\t\t\t\t\tstruct diff_filespec one, a, b;\n+\n+\t\t\t\t\toidcpy(&one.oid, &null_oid);\n+\t\t\t\t\tone.mode = 0;\n+\t\t\t\t\tone.path = ren1->pair->two->path;\n+\n+\t\t\t\t\toidcpy(&a.oid, &ren1->pair->two->oid);\n+\t\t\t\t\ta.mode = ren1->pair->two->mode;\n+\t\t\t\t\ta.path = one.path;\n+\n+\t\t\t\t\toidcpy(&b.oid, &dst_other.oid);\n+\t\t\t\t\tb.mode = dst_other.mode;\n+\t\t\t\t\tb.path = one.path;\n+\n+\t\t\t\t\tif (merge_file_1(o, &one, &a, &b, ren1_dst,\n+\t\t\t\t\t\t\t branch1, branch2,\n+\t\t\t\t\t\t\t &mfi)) {\n \t\t\t\t\t\tclean_merge = -1;\n \t\t\t\t\t\tgoto cleanup_and_return;\n \t\t\t\t\t}\n-- \n2.19.0.12.gc6760fd9a9\n\n"},{"id":"358453","messageId":"20180919161434.3272-3-newren@gmail.com","threadId":"49382","inReplyTo":"20180919161434.3272-1-newren@gmail.com","subject":"[PATCH 2/4] merge-recursive: avoid wrapper function when unnecessary and wasteful","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-09-19T16:14:32Z","receivedAt":"2018-09-19T16:14:42Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"merge_file_one() is a convenience function taking a bunch of oids and\nmodes, combining each pair into a diff_filespec, and then calling\nmerge_file_1().  When we already start with diff_filespec's, we can\njust call merge_file_1() directly instead of splitting out the oids\nand modes for the wrapper to recombine into what we already had.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n merge-recursive.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 99a7ac5ec7..9e4e3da672 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1630,10 +1630,7 @@ static int handle_rename_rename_1to2(struct merge_options *o,\n \t\tstruct merge_file_info mfi;\n \t\tstruct diff_filespec other;\n \t\tstruct diff_filespec *add;\n-\t\tif (merge_file_one(o, one->path,\n-\t\t\t\t &one->oid, one->mode,\n-\t\t\t\t &a->oid, a->mode,\n-\t\t\t\t &b->oid, b->mode,\n+\t\tif (merge_file_1(o, one, a, b, one->path,\n \t\t\t\t ci->branch1, ci->branch2, &mfi))\n \t\t\treturn -1;\n \n-- \n2.19.0.12.gc6760fd9a9\n\n"},{"id":"358454","messageId":"20180919161434.3272-2-newren@gmail.com","threadId":"49382","inReplyTo":"20180919161434.3272-1-newren@gmail.com","subject":"[PATCH 1/4] merge-recursive: set paths correctly when three-way merging content","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-09-19T16:14:31Z","receivedAt":"2018-09-19T16:14:43Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"merge_3way() has code to mark different sides of the conflict with info\nabout where the content comes from.  If the names of the files involved\nmatch, it simply uses the branch name.  If the names of the files do not\nmatch, it uses branchname:filename.  Unfortunately, merge_content()\npreviously always called it with one.path = a.path = b.path.  Granted,\nit didn't have other path information available to it for years, but\nthat was corrected by passing rename_conflict_info in commit\n3c217c077a86 (\"merge-recursive: Provide more info in conflict markers\nwith file renames\", 2011-08-11).  In that commit, instead of just fixing\nthe bug with the pathnames, it created fake branch names incorporating\nboth the branch name and file name.\n\nThis \"fake branch\" workaround was extended further when I pulled that\nlogic out into a special function in commit dac4741554e7\n(\"merge-recursive: Create function for merging with branchname:file\nmarkers\", 2011-08-11), and a number of other sites outside of\nmerge_content() have been added which call into that.  However, this\nRube-Goldberg-esque setup is not merely duplicate code and unnecessary\nwork, it also risked having other callsites invoke it in a way that\nwould result in markers of the form branchname:filename:filename (i.e.\nwith the filename repeated).\n\nFix this whole mess by:\n  - setting one.path, a.path, and b.path appropriately\n  - calling merge_file_1() directly\n  - deleting the merge_file_special_markers() workaround wrapper\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n merge-recursive.c | 49 +++++++++--------------------------------------\n 1 file changed, 9 insertions(+), 40 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 45a163c555..99a7ac5ec7 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1366,35 +1366,6 @@ static int merge_file_1(struct merge_options *o,\n \treturn 0;\n }\n \n-static int merge_file_special_markers(struct merge_options *o,\n-\t\t\t\t      const struct diff_filespec *one,\n-\t\t\t\t      const struct diff_filespec *a,\n-\t\t\t\t      const struct diff_filespec *b,\n-\t\t\t\t      const char *target_filename,\n-\t\t\t\t      const char *branch1,\n-\t\t\t\t      const char *filename1,\n-\t\t\t\t      const char *branch2,\n-\t\t\t\t      const char *filename2,\n-\t\t\t\t      struct merge_file_info *mfi)\n-{\n-\tchar *side1 = NULL;\n-\tchar *side2 = NULL;\n-\tint ret;\n-\n-\tif (filename1)\n-\t\tside1 = xstrfmt(\"%s:%s\", branch1, filename1);\n-\tif (filename2)\n-\t\tside2 = xstrfmt(\"%s:%s\", branch2, filename2);\n-\n-\tret = merge_file_1(o, one, a, b, target_filename,\n-\t\t\t   side1 ? side1 : branch1,\n-\t\t\t   side2 ? side2 : branch2, mfi);\n-\n-\tfree(side1);\n-\tfree(side2);\n-\treturn ret;\n-}\n-\n static int merge_file_one(struct merge_options *o,\n \t\t\t  const char *path,\n \t\t\t  const struct object_id *o_oid, int o_mode,\n@@ -1729,14 +1700,10 @@ static int handle_rename_rename_2to1(struct merge_options *o,\n \n \tpath_side_1_desc = xstrfmt(\"%s (was %s)\", path, a->path);\n \tpath_side_2_desc = xstrfmt(\"%s (was %s)\", path, b->path);\n-\tif (merge_file_special_markers(o, a, c1, &ci->ren1_other,\n-\t\t\t\t       path_side_1_desc,\n-\t\t\t\t       o->branch1, c1->path,\n-\t\t\t\t       o->branch2, ci->ren1_other.path, &mfi_c1) ||\n-\t    merge_file_special_markers(o, b, &ci->ren2_other, c2,\n-\t\t\t\t       path_side_2_desc,\n-\t\t\t\t       o->branch1, ci->ren2_other.path,\n-\t\t\t\t       o->branch2, c2->path, &mfi_c2))\n+\tif (merge_file_1(o, a, c1, &ci->ren1_other, path_side_1_desc,\n+\t\t\t o->branch1, o->branch2, &mfi_c1) ||\n+\t    merge_file_1(o, b, &ci->ren2_other, c2, path_side_2_desc,\n+\t\t\t o->branch1, o->branch2, &mfi_c2))\n \t\treturn -1;\n \tfree(path_side_1_desc);\n \tfree(path_side_2_desc);\n@@ -3059,14 +3026,16 @@ static int merge_content(struct merge_options *o,\n \t\tpath2 = (rename_conflict_info->pair2 ||\n \t\t\t o->branch2 == rename_conflict_info->branch1) ?\n \t\t\tpair1->two->path : pair1->one->path;\n+\t\tone.path = pair1->one->path;\n+\t\ta.path = (char *)path1;\n+\t\tb.path = (char *)path2;\n \n \t\tif (dir_in_way(path, !o->call_depth,\n \t\t\t       S_ISGITLINK(pair1->two->mode)))\n \t\t\tdf_conflict_remains = 1;\n \t}\n-\tif (merge_file_special_markers(o, &one, &a, &b, path,\n-\t\t\t\t       o->branch1, path1,\n-\t\t\t\t       o->branch2, path2, &mfi))\n+\tif (merge_file_1(o, &one, &a, &b, path,\n+\t\t\t o->branch1, o->branch2, &mfi))\n \t\treturn -1;\n \n \t/*\n-- \n2.19.0.12.gc6760fd9a9\n\n"},{"id":"358455","messageId":"20180919161434.3272-1-newren@gmail.com","threadId":"49382","inReplyTo":null,"subject":"[PATCH 0/4] Cleanup of merge_*() functions in merge-recursive","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-09-19T16:14:30Z","receivedAt":"2018-09-19T16:14:44Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"While working on a series to make file collision conflict handling\nconsistent, I discovered a latent bug in merge_content()...and several\nopportunities for cleanup.  This series fixes that bug, deletes the\nmerge_file_special_markers() and merge_file_one() functions, and\nrenames a pair of functions to make them have a better description.\n\nElijah Newren (4):\n  merge-recursive: set paths correctly when three-way merging content\n  merge-recursive: avoid wrapper function when unnecessary and wasteful\n  merge-recursive: remove final remaining caller of merge_file_one()\n  merge-recursive: rename merge_file_1() and merge_content()\n\n merge-recursive.c | 144 ++++++++++++++++------------------------------\n 1 file changed, 51 insertions(+), 93 deletions(-)\n\n-- \n2.19.0.12.gc6760fd9a9\n\n"},{"id":"358497","messageId":"CAGZ79kayKOmFhNxZyZ7-eumnLdK5-GtOb4OXFwdDP3qe4uQY7Q@mail.gmail.com","threadId":"49382","inReplyTo":"20180919161434.3272-5-newren@gmail.com","subject":"Re: [PATCH 4/4] merge-recursive: rename merge_file_1() and merge_content()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-19T23:40:43Z","receivedAt":"2018-09-19T23:40:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 19, 2018 at 9:15 AM Elijah Newren <newren@gmail.com> wrote:\n>\n> Summary:\n>   merge_file_1()  -> merge_mode_and_contents()\n>   merge_content() -> handle_content_merge()\n>\n> merge_file_1() is a very unhelpful name.  Rename it to\n> merge_mode_and_contents() to reflect what it does.\n>\n> merge_content() calls merge_mode_and_contents() to do the main part of\n> its work, but most of this function was about higher level stuff, e.g.\n> printing out conflict messages, updating skip_worktree bits, checking\n> for ability to avoid updating the working tree or for D/F conflicts\n> being in the way, etc.  Since there are several handle_*() functions for\n> similar levels of checking and handling in merge-recursive.c (e.g.\n> handle_change_delete(), handle_rename_rename_2to1()), let's rename this\n> function to handle_content_merge().\n>\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n\nI looked through the whole series and 1,2,4 are obvious improvements IMHO,\nand 3 in itself is not worth it if it weren't for 4 (as now we don't\nhave neither\n_1 and _one functions in this file).\n\nThe whole series looks good to me.\n\nThanks,\nStefan\n"}]}