{"thread":{"id":"54752","subject":"[PATCH 0/7] merge-ort: some groundwork for further implementation","startedAt":"2020-12-03T16:00:32Z","lastAt":"2020-12-04T18:41:35Z","messageCount":13,"participants":["Elijah Newren via GitGitGadget","Derrick Stolee","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"411229","messageId":"pull.803.git.1607011187.gitgitgadget@gmail.com","threadId":"54752","inReplyTo":null,"subject":"[PATCH 0/7] merge-ort: some groundwork for further implementation","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T15:59:39Z","receivedAt":"2020-12-03T16:00:32Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series is based on en/merge-ort-impl. This series sets up three future\npatch series (to add recursive merges, three-way content merging, and rename\ndetection) for the merge-ort implementation, and allows the future series to\nbe submitted, reviewed, and merged in any order. Since those three things\nactually do have some minor dependencies between them, this preparatory\nseries is needed to make a few small changes to set things up to allow them\nto be submitted independently. \n\nThe first six patches are trivial. They should be easy to review, and as a\nbonus you get to find how I mess up even the trivial stuff. ;-) The final\npatch is more substantive and represents one of the big changes between\nmerge-recursive and merge-ort -- namely, how notice/warning/conflict\nmessages are reported to the user (I possibly should have included it in\nmerge-ort-impl, but that series seemed so long already...).\n\nElijah Newren (7):\n merge-ort: add a few includes\n merge-ort: add a clear_internal_opts helper\n merge-ort: add a path_conflict field to merge_options_internal\n merge-ort: add a paths_to_free field to merge_options_internal\n merge-ort: add function grouping comments\n merge-ort: add die-not-implemented stub handle_content_merge()\n function\n merge-ort: add modify/delete handling and delayed output processing\n\n merge-ort.c | 210 +++++++++++++++++++++++++++++++++++++++++++++++-----\n 1 file changed, 191 insertions(+), 19 deletions(-)\n\n\nbase-commit: 00de8a7763e29fb8a034030afbd0fbfc4c818e07\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-803%2Fnewren%2Fort-common-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-803/newren/ort-common-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/803\n-- \ngitgitgadget\n"},{"id":"411230","messageId":"fa0881892de9f0089e2ee79f616fbd8fc5a95467.1607011187.git.gitgitgadget@gmail.com","threadId":"54752","inReplyTo":"pull.803.git.1607011187.gitgitgadget@gmail.com","subject":"[PATCH 3/7] merge-ort: add a path_conflict field to merge_options_internal","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T15:59:42Z","receivedAt":"2020-12-03T16:01:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThis field is not yet used, but will be used by both the rename handling\ncode, and the conflict type handling code in process_entry().\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 0654c76c8c..bcd53d3799 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -148,6 +148,13 @@ struct conflict_info {\n \t/* Whether this path is/was involved in a directory/file conflict */\n \tunsigned df_conflict:1;\n \n+\t/*\n+\t * Whether this path is/was involved in a non-content conflict other\n+\t * than a directory/file conflict (e.g. rename/rename, rename/delete,\n+\t * file location based on possible directory rename).\n+\t */\n+\tunsigned path_conflict:1;\n+\n \t/*\n \t * For filemask and dirmask, see tree-walk.h's struct traverse_info,\n \t * particularly the documentation above the \"fn\" member. Note that\n-- \ngitgitgadget\n\n"},{"id":"411231","messageId":"5d73827b8d6c254f5bc4a99afa421ae34f07182e.1607011187.git.gitgitgadget@gmail.com","threadId":"54752","inReplyTo":"pull.803.git.1607011187.gitgitgadget@gmail.com","subject":"[PATCH 2/7] merge-ort: add a clear_internal_opts helper","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T15:59:41Z","receivedAt":"2020-12-03T16:01:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nMove most of merge_finalize() into a new helper function,\nclear_internal_opts(). This is a step to facilitate recursive merges,\nas well as some future optimizations.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 40 ++++++++++++++++++++++++----------------\n 1 file changed, 24 insertions(+), 16 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex b556897bc0..0654c76c8c 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -194,6 +194,29 @@ static void free_strmap_strings(struct strmap *map)\n \t}\n }\n \n+static void clear_internal_opts(struct merge_options_internal *opti,\n+\t\t\t\tint reinitialize)\n+{\n+\tassert(!reinitialize);\n+\n+\t/*\n+\t * We marked opti->paths with strdup_strings = 0, so that we\n+\t * wouldn't have to make another copy of the fullpath created by\n+\t * make_traverse_path from setup_path_info(). But, now that we've\n+\t * used it and have no other references to these strings, it is time\n+\t * to deallocate them.\n+\t */\n+\tfree_strmap_strings(&opti->paths);\n+\tstrmap_clear(&opti->paths, 1);\n+\n+\t/*\n+\t * All keys and values in opti->conflicted are a subset of those in\n+\t * opti->paths. We don't want to deallocate anything twice, so we\n+\t * don't free the keys and we pass 0 for free_values.\n+\t */\n+\tstrmap_clear(&opti->conflicted, 0);\n+}\n+\n static int err(struct merge_options *opt, const char *err, ...)\n {\n \tva_list params;\n@@ -1132,22 +1155,7 @@ void merge_finalize(struct merge_options *opt,\n \n \tassert(opt->priv == NULL);\n \n-\t/*\n-\t * We marked opti->paths with strdup_strings = 0, so that we\n-\t * wouldn't have to make another copy of the fullpath created by\n-\t * make_traverse_path from setup_path_info(). But, now that we've\n-\t * used it and have no other references to these strings, it is time\n-\t * to deallocate them.\n-\t */\n-\tfree_strmap_strings(&opti->paths);\n-\tstrmap_clear(&opti->paths, 1);\n-\n-\t/*\n-\t * All keys and values in opti->conflicted are a subset of those in\n-\t * opti->paths. We don't want to deallocate anything twice, so we\n-\t * don't free the keys and we pass 0 for free_values.\n-\t */\n-\tstrmap_clear(&opti->conflicted, 0);\n+\tclear_internal_opts(opti, 0);\n \tFREE_AND_NULL(opti);\n }\n \n-- \ngitgitgadget\n\n"},{"id":"411232","messageId":"d37724031f2bced7b7f6fb3a0a5266a20a99b1cc.1607011187.git.gitgitgadget@gmail.com","threadId":"54752","inReplyTo":"pull.803.git.1607011187.gitgitgadget@gmail.com","subject":"[PATCH 4/7] merge-ort: add a paths_to_free field to merge_options_internal","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T15:59:43Z","receivedAt":"2020-12-03T16:01:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThis field will be used in future patches to allow removal of paths from\nopt->priv->paths.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 26 +++++++++++++++++++++++++-\n 1 file changed, 25 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex bcd53d3799..89b9fdb04a 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -41,6 +41,8 @@ struct merge_options_internal {\n \t * * these keys serve to intern all the path strings, which allows\n \t * us to do pointer comparison on directory names instead of\n \t * strcmp; we just have to be careful to use the interned strings.\n+\t * (Technically paths_to_free may track some strings that were\n+\t * removed from froms paths.)\n \t *\n \t * The values of paths:\n \t * * either a pointer to a merged_info, or a conflict_info struct\n@@ -75,6 +77,16 @@ struct merge_options_internal {\n \t */\n \tstruct strmap conflicted;\n \n+\t/*\n+\t * paths_to_free: additional list of strings to free\n+\t *\n+\t * If keys are removed from \"paths\", they are added to paths_to_free\n+\t * to ensure they are later freed. We avoid free'ing immediately since\n+\t * other places (e.g. conflict_info.pathnames[]) may still be\n+\t * referencing these paths.\n+\t */\n+\tstruct string_list paths_to_free;\n+\n \t/*\n \t * current_dir_name: temporary var used in collect_merge_info_callback()\n \t *\n@@ -222,6 +234,17 @@ static void clear_internal_opts(struct merge_options_internal *opti,\n \t * don't free the keys and we pass 0 for free_values.\n \t */\n \tstrmap_clear(&opti->conflicted, 0);\n+\n+\t/*\n+\t * opti->paths_to_free is similar to opti->paths; we created it with\n+\t * strdup_strings = 0 to avoid making _another_ copy of the fullpath\n+\t * but now that we've used it and have no other references to these\n+\t * strings, it is time to deallocate them. We do so by temporarily\n+\t * setting strdup_strings to 1.\n+\t */\n+\topti->paths_to_free.strdup_strings = 1;\n+\tstring_list_clear(&opti->paths_to_free, 0);\n+\topti->paths_to_free.strdup_strings = 0;\n }\n \n static int err(struct merge_options *opt, const char *err, ...)\n@@ -1206,13 +1229,14 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)\n \t * Although we initialize opt->priv->paths with strdup_strings=0,\n \t * that's just to avoid making yet another copy of an allocated\n \t * string. Putting the entry into paths means we are taking\n-\t * ownership, so we will later free it.\n+\t * ownership, so we will later free it. paths_to_free is similar.\n \t *\n \t * In contrast, conflicted just has a subset of keys from paths, so\n \t * we don't want to free those (it'd be a duplicate free).\n \t */\n \tstrmap_init_with_options(&opt->priv->paths, NULL, 0);\n \tstrmap_init_with_options(&opt->priv->conflicted, NULL, 0);\n+\tstring_list_init(&opt->priv->paths_to_free, 0);\n }\n \n /*\n-- \ngitgitgadget\n\n"},{"id":"411233","messageId":"181627c30cc1995b66eb5659e54737d05964f8a1.1607011187.git.gitgitgadget@gmail.com","threadId":"54752","inReplyTo":"pull.803.git.1607011187.gitgitgadget@gmail.com","subject":"[PATCH 5/7] merge-ort: add function grouping comments","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T15:59:44Z","receivedAt":"2020-12-03T16:01:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nCommit b658536f59 (\"merge-ort: add some high-level algorithm structure\",\n2020-10-27) added high-level structure of the ort merge algorithm. As\nwe have added more and more functions, that high-level structure has\nbeen slightly obscured. Since functions are still grouped according to\nthis high-level structure, add comments denoting sections where all the\nfunctions are specifically tied to a piece of the high-level structure.\n\nThis function groupings include a few sub-divisions of the original\nhigh-level structure, including some sub-divisions that are yet to be\nsubmitted. Each has (or will have) several functions all serving as\nhelpers to one or two main functions for each section.\n\nAs an added bonus, the comments will serve to provide a small textual\nseparation between nearby sections and allow the next three patch series\nto be submitted independently and merge cleanly.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 89b9fdb04a..e653ba35ea 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -183,6 +183,8 @@ struct conflict_info {\n \tunsigned match_mask:3;\n };\n \n+/*** Function Grouping: various utility functions ***/\n+\n /*\n * For the next three macros, see warning for conflict_info.merged.\n *\n@@ -263,6 +265,8 @@ static int err(struct merge_options *opt, const char *err, ...)\n \treturn -1;\n }\n \n+/*** Function Grouping: functions related to collect_merge_info() ***/\n+\n static void setup_path_info(struct merge_options *opt,\n \t\t\t struct string_list_item *result,\n \t\t\t const char *current_dir_name,\n@@ -517,6 +521,15 @@ static int collect_merge_info(struct merge_options *opt,\n \treturn ret;\n }\n \n+/*** Function Grouping: functions related to threeway content merges ***/\n+\n+/*** Function Grouping: functions related to detect_and_process_renames(), ***\n+ *** which are split into directory and regular rename detection sections. ***/\n+\n+/*** Function Grouping: functions related to directory rename detection ***/\n+\n+/*** Function Grouping: functions related to regular rename detection ***/\n+\n static int detect_and_process_renames(struct merge_options *opt,\n \t\t\t\t struct tree *merge_base,\n \t\t\t\t struct tree *side1,\n@@ -534,6 +547,8 @@ static int detect_and_process_renames(struct merge_options *opt,\n \treturn clean;\n }\n \n+/*** Function Grouping: functions related to process_entries() ***/\n+\n static int string_list_df_name_compare(const char *one, const char *two)\n {\n \tint onelen = strlen(one);\n@@ -1001,6 +1016,8 @@ static void process_entries(struct merge_options *opt,\n \tstring_list_clear(&dir_metadata.offsets, 0);\n }\n \n+/*** Function Grouping: functions related to merge_switch_to_result() ***/\n+\n static int checkout(struct merge_options *opt,\n \t\t struct tree *prev,\n \t\t struct tree *next)\n@@ -1189,6 +1206,8 @@ void merge_finalize(struct merge_options *opt,\n \tFREE_AND_NULL(opti);\n }\n \n+/*** Function Grouping: helper functions for merge_incore_*() ***/\n+\n static void merge_start(struct merge_options *opt, struct merge_result *result)\n {\n \t/* Sanity checks on opt */\n@@ -1239,6 +1258,8 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)\n \tstring_list_init(&opt->priv->paths_to_free, 0);\n }\n \n+/*** Function Grouping: merge_incore_*() and their internal variants ***/\n+\n /*\n * Originally from merge_trees_internal(); heavily adapted, though.\n */\n-- \ngitgitgadget\n\n"},{"id":"411235","messageId":"6cd8ee01c7107f54bdba177f27089321df047eac.1607011187.git.gitgitgadget@gmail.com","threadId":"54752","inReplyTo":"pull.803.git.1607011187.gitgitgadget@gmail.com","subject":"[PATCH 1/7] merge-ort: add a few includes","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T15:59:40Z","receivedAt":"2020-12-03T16:01:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nInclude blob.h for definition of blob_type, and commit-reach.h for\ndeclarations of get_merge_bases() and in_merge_bases(). While none of\nthese are used yet, we want to avoid cross-dependencies in the next\nthree series of patches for merge-ort and merge them at the end; adding\nthese \"#include\"s now avoids textual conflicts.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex ea6a9d7348..b556897bc0 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -17,7 +17,9 @@\n #include \"cache.h\"\n #include \"merge-ort.h\"\n \n+#include \"blob.h\"\n #include \"cache-tree.h\"\n+#include \"commit-reach.h\"\n #include \"diff.h\"\n #include \"diffcore.h\"\n #include \"dir.h\"\n-- \ngitgitgadget\n\n"},{"id":"411234","messageId":"9f1ac20e31886ad7cd56ff582c58cce4ee743fa1.1607011187.git.gitgitgadget@gmail.com","threadId":"54752","inReplyTo":"pull.803.git.1607011187.gitgitgadget@gmail.com","subject":"[PATCH 6/7] merge-ort: add die-not-implemented stub handle_content_merge() function","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T15:59:45Z","receivedAt":"2020-12-03T16:01:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThis simplistic and weird-looking patch is here to facilitate future\npatch submissions. Adding this stub allows rename detection code to\nreference it in one patch series, while a separate patch series can\ndefine the implementation, and then both series can merge cleanly and\nwork nicely together at that point.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex e653ba35ea..e7220cbbb4 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -523,6 +523,18 @@ static int collect_merge_info(struct merge_options *opt,\n \n /*** Function Grouping: functions related to threeway content merges ***/\n \n+static int handle_content_merge(struct merge_options *opt,\n+\t\t\t\tconst char *path,\n+\t\t\t\tconst struct version_info *o,\n+\t\t\t\tconst struct version_info *a,\n+\t\t\t\tconst struct version_info *b,\n+\t\t\t\tconst char *pathnames[3],\n+\t\t\t\tconst int extra_marker_size,\n+\t\t\t\tstruct version_info *result)\n+{\n+\tdie(\"Not yet implemented\");\n+}\n+\n /*** Function Grouping: functions related to detect_and_process_renames(), ***\n *** which are split into directory and regular rename detection sections. ***/\n \n@@ -919,6 +931,8 @@ static void process_entry(struct merge_options *opt,\n \t\tci->merged.clean = 0;\n \t\tci->merged.result.mode = ci->stages[1].mode;\n \t\toidcpy(&ci->merged.result.oid, &ci->stages[1].oid);\n+\t\t/* When we fix above, we'll call handle_content_merge() */\n+\t\t(void)handle_content_merge;\n \t} else if (ci->filemask == 3 || ci->filemask == 5) {\n \t\t/* Modify/delete */\n \t\tdie(\"Not yet implemented.\");\n-- \ngitgitgadget\n\n"},{"id":"411236","messageId":"8ab55a6ecb0b3c7139520d09d752420d7e90f6c2.1607011187.git.gitgitgadget@gmail.com","threadId":"54752","inReplyTo":"pull.803.git.1607011187.gitgitgadget@gmail.com","subject":"[PATCH 7/7] merge-ort: add modify/delete handling and delayed output processing","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T15:59:46Z","receivedAt":"2020-12-03T16:01:12Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe focus here is on adding a path_msg() which will queue up\nwarning/conflict/notice messages about the merge for later processing,\nstoring these in a pathname -> strbuf map. It might seem like a big\nchange, but it really just is:\n\n * declaration of necessary map with some comments\n * initialization and recording of data\n * a bunch of code to iterate over the map at print/free time\n * at least one caller in order to avoid an error about having an\n unused function (which we provide in the form of implementing\n modify/delete conflict handling).\n\nAt this stage, it is probably not clear why I am opting for delayed\noutput processing. There are multiple reasons:\n\n 1. Merges are supposed to abort if they would overwrite dirty changes\n in the working tree. We cannot correctly determine whether changes\n would be overwritten until both rename detection has occurred and\n full processing of entries with the renames has finalized.\n Warning/conflict/notice messages come up at intermediate codepaths\n along the way, so unless we want spurious conflict/warning messages\n being printed when the merge will be aborted anyway, we need to\n save these messages and only print them when relevant.\n\n 2. There can be multiple messages for a single path, and we want all\n messages for a give path to appear together instead of having them\n grouped by conflict/warning type. This was a problem already with\n merge-recursive.c but became even more important due to the\n splitting apart of conflict types as discussed in the commit\n message for 1f3c9ba707 (\"t6425: be more flexible with rename/delete\n conflict messages\", 2020-08-10)\n\n 3. Some callers might want to avoid showing the output in certain\n cases, such as if the end result is a clean merge. Rebases have\n typically done this.\n\n 4. Some callers might not want the output to go to stdout or even\n stderr, but might want to do something else with it entirely.\n For example, a --remerge-diff option to `git show` or `git log\n -p` that remerges on the fly and diffs merge commits against the\n remerged version would benefit from stdout/stderr not being\n written to in the standard form.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 98 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex e7220cbbb4..64468f0706 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -87,6 +87,15 @@ struct merge_options_internal {\n \t */\n \tstruct string_list paths_to_free;\n \n+\t/*\n+\t * output: special messages and conflict notices for various paths\n+\t *\n+\t * This is a map of pathnames (a subset of the keys in \"paths\" above)\n+\t * to strbufs. It gathers various warning/conflict/notice messages\n+\t * for later processing.\n+\t */\n+\tstruct strmap output;\n+\n \t/*\n \t * current_dir_name: temporary var used in collect_merge_info_callback()\n \t *\n@@ -247,6 +256,27 @@ static void clear_internal_opts(struct merge_options_internal *opti,\n \topti->paths_to_free.strdup_strings = 1;\n \tstring_list_clear(&opti->paths_to_free, 0);\n \topti->paths_to_free.strdup_strings = 0;\n+\n+\tif (!reinitialize) {\n+\t\tstruct hashmap_iter iter;\n+\t\tstruct strmap_entry *e;\n+\n+\t\t/* Release and free each strbuf found in output */\n+\t\tstrmap_for_each_entry(&opti->output, &iter, e) {\n+\t\t\tstruct strbuf *sb = e->value;\n+\t\t\tstrbuf_release(sb);\n+\t\t\t/*\n+\t\t\t * While strictly speaking we don't need to free(sb)\n+\t\t\t * here because we could pass free_values=1 when\n+\t\t\t * calling strmap_clear() on opti->output, that would\n+\t\t\t * require strmap_clear to do another\n+\t\t\t * strmap_for_each_entry() loop, so we just free it\n+\t\t\t * while we're iterating anyway.\n+\t\t\t */\n+\t\t\tfree(sb);\n+\t\t}\n+\t\tstrmap_clear(&opti->output, 0);\n+\t}\n }\n \n static int err(struct merge_options *opt, const char *err, ...)\n@@ -265,6 +295,27 @@ static int err(struct merge_options *opt, const char *err, ...)\n \treturn -1;\n }\n \n+__attribute__((format (printf, 4, 5)))\n+static void path_msg(struct merge_options *opt,\n+\t\t const char *path,\n+\t\t int omittable_hint, /* skippable under --remerge-diff */\n+\t\t const char *fmt, ...)\n+{\n+\tva_list ap;\n+\tstruct strbuf *sb = strmap_get(&opt->priv->output, path);\n+\tif (!sb) {\n+\t\tsb = xmalloc(sizeof(*sb));\n+\t\tstrbuf_init(sb, 0);\n+\t\tstrmap_put(&opt->priv->output, path, sb);\n+\t}\n+\n+\tva_start(ap, fmt);\n+\tstrbuf_vaddf(sb, fmt, ap);\n+\tva_end(ap);\n+\n+\tstrbuf_addch(sb, '\\n');\n+}\n+\n /*** Function Grouping: functions related to collect_merge_info() ***/\n \n static void setup_path_info(struct merge_options *opt,\n@@ -935,7 +986,23 @@ static void process_entry(struct merge_options *opt,\n \t\t(void)handle_content_merge;\n \t} else if (ci->filemask == 3 || ci->filemask == 5) {\n \t\t/* Modify/delete */\n-\t\tdie(\"Not yet implemented.\");\n+\t\tconst char *modify_branch, *delete_branch;\n+\t\tint side = (ci->filemask == 5) ? 2 : 1;\n+\t\tint index = opt->priv->call_depth ? 0 : side;\n+\n+\t\tci->merged.result.mode = ci->stages[index].mode;\n+\t\toidcpy(&ci->merged.result.oid, &ci->stages[index].oid);\n+\t\tci->merged.clean = 0;\n+\n+\t\tmodify_branch = (side == 1) ? opt->branch1 : opt->branch2;\n+\t\tdelete_branch = (side == 1) ? opt->branch2 : opt->branch1;\n+\n+\t\tpath_msg(opt, path, 0,\n+\t\t\t _(\"CONFLICT (modify/delete): %s deleted in %s \"\n+\t\t\t \"and modified in %s. Version %s of %s left \"\n+\t\t\t \"in tree.\"),\n+\t\t\t path, delete_branch, modify_branch,\n+\t\t\t modify_branch, path);\n \t} else if (ci->filemask == 2 || ci->filemask == 4) {\n \t\t/* Added on one side */\n \t\tint side = (ci->filemask == 4) ? 2 : 1;\n@@ -1203,7 +1270,29 @@ void merge_switch_to_result(struct merge_options *opt,\n \t}\n \n \tif (display_update_msgs) {\n-\t\t/* TODO: print out CONFLICT and other informational messages. */\n+\t\tstruct merge_options_internal *opti = result->priv;\n+\t\tstruct hashmap_iter iter;\n+\t\tstruct strmap_entry *e;\n+\t\tstruct string_list olist = STRING_LIST_INIT_NODUP;\n+\t\tint i;\n+\n+\t\t/* Hack to pre-allocate olist to the desired size */\n+\t\tALLOC_GROW(olist.items, strmap_get_size(&opti->output),\n+\t\t\t olist.alloc);\n+\n+\t\t/* Put every entry from output into olist, then sort */\n+\t\tstrmap_for_each_entry(&opti->output, &iter, e) {\n+\t\t\tstring_list_append(&olist, e->key)->util = e->value;\n+\t\t}\n+\t\tstring_list_sort(&olist);\n+\n+\t\t/* Iterate over the items, printing them */\n+\t\tfor (i = 0; i < olist.nr; ++i) {\n+\t\t\tstruct strbuf *sb = olist.items[i].util;\n+\n+\t\t\tprintf(\"%s\", sb->buf);\n+\t\t}\n+\t\tstring_list_clear(&olist, 0);\n \t}\n \n \tmerge_finalize(opt, result);\n@@ -1270,6 +1359,13 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)\n \tstrmap_init_with_options(&opt->priv->paths, NULL, 0);\n \tstrmap_init_with_options(&opt->priv->conflicted, NULL, 0);\n \tstring_list_init(&opt->priv->paths_to_free, 0);\n+\n+\t/*\n+\t * keys & strbufs in output will sometimes need to outlive \"paths\",\n+\t * so it will have a copy of relevant keys. It's probably a small\n+\t * subset of the overall paths that have special output.\n+\t */\n+\tstrmap_init(&opt->priv->output);\n }\n \n /*** Function Grouping: merge_incore_*() and their internal variants ***/\n-- \ngitgitgadget\n"},{"id":"411258","messageId":"42a99014-573f-cc5d-ffa0-b22aec288425@gmail.com","threadId":"54752","inReplyTo":"5d73827b8d6c254f5bc4a99afa421ae34f07182e.1607011187.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/7] merge-ort: add a clear_internal_opts helper","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-12-03T17:00:26Z","receivedAt":"2020-12-03T17:01:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/3/2020 10:59 AM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> Move most of merge_finalize() into a new helper function,\n> clear_internal_opts(). This is a step to facilitate recursive merges,\n> as well as some future optimizations.\n> \n> Signed-off-by: Elijah Newren \n> ---\n> merge-ort.c | 40 ++++++++++++++++++++++++----------------\n> 1 file changed, 24 insertions(+), 16 deletions(-)\n> \n> diff --git a/merge-ort.c b/merge-ort.c\n> index b556897bc0..0654c76c8c 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -194,6 +194,29 @@ static void free_strmap_strings(struct strmap *map)\n> \t}\n> }\n> \n> +static void clear_internal_opts(struct merge_options_internal *opti,\n> +\t\t\t\tint reinitialize)\n> +{\n> +\tassert(!reinitialize);\n\nI was first confused by this new assert, but you are essentially\nsaying \"this parameter doesn't do anything (yet)\" which makes sense.\n\n> +\n> +\t/*\n> +\t * We marked opti->paths with strdup_strings = 0, so that we\n> +\t * wouldn't have to make another copy of the fullpath created by\n> +\t * make_traverse_path from setup_path_info(). But, now that we've\n> +\t * used it and have no other references to these strings, it is time\n> +\t * to deallocate them.\n> +\t */\n> +\tfree_strmap_strings(&opti->paths);\n> +\tstrmap_clear(&opti->paths, 1);\n> +\n> +\t/*\n> +\t * All keys and values in opti->conflicted are a subset of those in\n> +\t * opti->paths. We don't want to deallocate anything twice, so we\n> +\t * don't free the keys and we pass 0 for free_values.\n> +\t */\n> +\tstrmap_clear(&opti->conflicted, 0);\n...\n> -\t/*\n> -\t * We marked opti->paths with strdup_strings = 0, so that we\n> -\t * wouldn't have to make another copy of the fullpath created by\n> -\t * make_traverse_path from setup_path_info(). But, now that we've\n> -\t * used it and have no other references to these strings, it is time\n> -\t * to deallocate them.\n> -\t */\n> -\tfree_strmap_strings(&opti->paths);\n> -\tstrmap_clear(&opti->paths, 1);\n> -\n> -\t/*\n> -\t * All keys and values in opti->conflicted are a subset of those in\n> -\t * opti->paths. We don't want to deallocate anything twice, so we\n> -\t * don't free the keys and we pass 0 for free_values.\n> -\t */\n> -\tstrmap_clear(&opti->conflicted, 0);\n\nthe rest of this is a clear code move.\n\nThanks,\n-Stolee\n"},{"id":"411266","messageId":"8e861bc8-4da1-01aa-7de6-c6c128590170@gmail.com","threadId":"54752","inReplyTo":"9f1ac20e31886ad7cd56ff582c58cce4ee743fa1.1607011187.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 6/7] merge-ort: add die-not-implemented stub handle_content_merge() function","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-12-03T18:40:09Z","receivedAt":"2020-12-03T18:40:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/3/2020 10:59 AM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> This simplistic and weird-looking patch is here to facilitate future\n> patch submissions. Adding this stub allows rename detection code to\n> reference it in one patch series, while a separate patch series can\n> define the implementation, and then both series can merge cleanly and\n> work nicely together at that point.\n> \n> Signed-off-by: Elijah Newren \n> ---\n> merge-ort.c | 14 ++++++++++++++\n> 1 file changed, 14 insertions(+)\n> \n> diff --git a/merge-ort.c b/merge-ort.c\n> index e653ba35ea..e7220cbbb4 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -523,6 +523,18 @@ static int collect_merge_info(struct merge_options *opt,\n> \n> /*** Function Grouping: functions related to threeway content merges ***/\n> \n> +static int handle_content_merge(struct merge_options *opt,\n> +\t\t\t\tconst char *path,\n> +\t\t\t\tconst struct version_info *o,\n> +\t\t\t\tconst struct version_info *a,\n> +\t\t\t\tconst struct version_info *b,\n> +\t\t\t\tconst char *pathnames[3],\n> +\t\t\t\tconst int extra_marker_size,\n> +\t\t\t\tstruct version_info *result)\n> +{\n> +\tdie(\"Not yet implemented\");\n> +}\n> +\n> /*** Function Grouping: functions related to detect_and_process_renames(), ***\n> *** which are split into directory and regular rename detection sections. ***/\n> \n> @@ -919,6 +931,8 @@ static void process_entry(struct merge_options *opt,\n> \t\tci->merged.clean = 0;\n> \t\tci->merged.result.mode = ci->stages[1].mode;\n> \t\toidcpy(&ci->merged.result.oid, &ci->stages[1].oid);\n> +\t\t/* When we fix above, we'll call handle_content_merge() */\n> +\t\t(void)handle_content_merge;\n\nI'm not exactly sure what the value is of this line. Is it just to\nmake sure we have a reference to the 'static' method without actually\ncalling it anywhere?\n\n\"weird-looking patch\" indeed! I'm more confused than anything.\n\nThanks,\n-Stolee\n\n"},{"id":"411275","messageId":"CABPp-BFytQ5d2s8VpMbNXDGzG3=Jd2Tm2=G_mh+OBYqzhDeHBg@mail.gmail.com","threadId":"54752","inReplyTo":"8e861bc8-4da1-01aa-7de6-c6c128590170@gmail.com","subject":"Re: [PATCH 6/7] merge-ort: add die-not-implemented stub handle_content_merge() function","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-12-03T19:56:07Z","receivedAt":"2020-12-03T19:57:16Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Dec 3, 2020 at 10:40 AM Derrick Stolee wrote:\n>\n> On 12/3/2020 10:59 AM, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > This simplistic and weird-looking patch is here to facilitate future\n> > patch submissions. Adding this stub allows rename detection code to\n> > reference it in one patch series, while a separate patch series can\n> > define the implementation, and then both series can merge cleanly and\n> > work nicely together at that point.\n> >\n> > Signed-off-by: Elijah Newren \n> > ---\n> > merge-ort.c | 14 ++++++++++++++\n> > 1 file changed, 14 insertions(+)\n> >\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > index e653ba35ea..e7220cbbb4 100644\n> > --- a/merge-ort.c\n> > +++ b/merge-ort.c\n> > @@ -523,6 +523,18 @@ static int collect_merge_info(struct merge_options *opt,\n> >\n> > /*** Function Grouping: functions related to threeway content merges ***/\n> >\n> > +static int handle_content_merge(struct merge_options *opt,\n> > + const char *path,\n> > + const struct version_info *o,\n> > + const struct version_info *a,\n> > + const struct version_info *b,\n> > + const char *pathnames[3],\n> > + const int extra_marker_size,\n> > + struct version_info *result)\n> > +{\n> > + die(\"Not yet implemented\");\n> > +}\n> > +\n> > /*** Function Grouping: functions related to detect_and_process_renames(), ***\n> > *** which are split into directory and regular rename detection sections. ***/\n> >\n> > @@ -919,6 +931,8 @@ static void process_entry(struct merge_options *opt,\n> > ci->merged.clean = 0;\n> > ci->merged.result.mode = ci->stages[1].mode;\n> > oidcpy(&ci->merged.result.oid, &ci->stages[1].oid);\n> > + /* When we fix above, we'll call handle_content_merge() */\n> > + (void)handle_content_merge;\n>\n> I'm not exactly sure what the value is of this line. Is it just to\n> make sure we have a reference to the 'static' method without actually\n> calling it anywhere?\n\nYes; without the reference the compiler fails with an unused function\nerror message. I know it's not used yet, but I really need it there,\nso I have to fake the compiler out with a lame expression (take the\naddress of the function, cast to void, and discard the result since I\ndon't assign it anywhere or anything).\n\n> \"weird-looking patch\" indeed! I'm more confused than anything.\n\nIn general, rename detection occurs before process_entry() and thus\nprocess_entry() can handle the content merging. However, some unusual\nrename conflicts require multiple content merges (and possibly result\nin nested conflict markers) and so the rename code needs to be able to\ncall handle_content_merge() for the first of those.\n\nI really wanted to split apart the series for rename detection (12\npatches), and the one for more conflict handling (10 patches),\nespecially since the latter series includes 6 patches for building up\nhandle_content_merge() (3 for regular file content merging and another\n3 for submodule \"merging\"). The two series are nearly orthogonal, but\nI had to somehow allow the rename side to call handle_content_merge()\nwithout having both series try to introduce the same function. Hence\nthis patch.\n"},{"id":"411337","messageId":"2ea0aab8-934f-3eaa-e3d0-9ae35a278748@gmail.com","threadId":"54752","inReplyTo":"pull.803.git.1607011187.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/7] merge-ort: some groundwork for further implementation","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-12-04T17:26:18Z","receivedAt":"2020-12-04T17:27:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/3/2020 10:59 AM, Elijah Newren via GitGitGadget wrote:\n> This series is based on en/merge-ort-impl. This series sets up three future\n> patch series (to add recursive merges, three-way content merging, and rename\n> detection) for the merge-ort implementation, and allows the future series to\n> be submitted, reviewed, and merged in any order. Since those three things\n> actually do have some minor dependencies between them, this preparatory\n> series is needed to make a few small changes to set things up to allow them\n> to be submitted independently. \n> \n> The first six patches are trivial. They should be easy to review, and as a\n> bonus you get to find how I mess up even the trivial stuff. ;-) The final\n> patch is more substantive and represents one of the big changes between\n> merge-recursive and merge-ort -- namely, how notice/warning/conflict\n> messages are reported to the user (I possibly should have included it in\n> merge-ort-impl, but that series seemed so long already...).\n> \n> Elijah Newren (7):\n> merge-ort: add a few includes\n> merge-ort: add a clear_internal_opts helper\n> merge-ort: add a path_conflict field to merge_options_internal\n> merge-ort: add a paths_to_free field to merge_options_internal\n> merge-ort: add function grouping comments\n> merge-ort: add die-not-implemented stub handle_content_merge()\n> function\n> merge-ort: add modify/delete handling and delayed output processing\n\nComing back to say that I finished reading PATCH 7 and this series\nlooks good overall. Tough to be confident in it when the implementation\nisn't connected to tests, but the patches do a good job of describing\nthe isolated changes. If there _are_ problems, it will be easy to\nidentify the reasoning behind the code using log/blame.\n\nThanks,\n-Stolee\n"},{"id":"411341","messageId":"CABPp-BGrNS2Rcue9JZBC=BFyedPP8JnPyJn_P8eT2itOEds36A@mail.gmail.com","threadId":"54752","inReplyTo":"2ea0aab8-934f-3eaa-e3d0-9ae35a278748@gmail.com","subject":"Re: [PATCH 0/7] merge-ort: some groundwork for further implementation","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-12-04T18:40:36Z","receivedAt":"2020-12-04T18:41:35Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Dec 4, 2020 at 9:26 AM Derrick Stolee wrote:\n>\n> On 12/3/2020 10:59 AM, Elijah Newren via GitGitGadget wrote:\n> > This series is based on en/merge-ort-impl. This series sets up three future\n> > patch series (to add recursive merges, three-way content merging, and rename\n> > detection) for the merge-ort implementation, and allows the future series to\n> > be submitted, reviewed, and merged in any order. Since those three things\n> > actually do have some minor dependencies between them, this preparatory\n> > series is needed to make a few small changes to set things up to allow them\n> > to be submitted independently.\n> >\n> > The first six patches are trivial. They should be easy to review, and as a\n> > bonus you get to find how I mess up even the trivial stuff. ;-) The final\n> > patch is more substantive and represents one of the big changes between\n> > merge-recursive and merge-ort -- namely, how notice/warning/conflict\n> > messages are reported to the user (I possibly should have included it in\n> > merge-ort-impl, but that series seemed so long already...).\n> >\n> > Elijah Newren (7):\n> > merge-ort: add a few includes\n> > merge-ort: add a clear_internal_opts helper\n> > merge-ort: add a path_conflict field to merge_options_internal\n> > merge-ort: add a paths_to_free field to merge_options_internal\n> > merge-ort: add function grouping comments\n> > merge-ort: add die-not-implemented stub handle_content_merge()\n> > function\n> > merge-ort: add modify/delete handling and delayed output processing\n>\n> Coming back to say that I finished reading PATCH 7 and this series\n> looks good overall. Tough to be confident in it when the implementation\n> isn't connected to tests, but the patches do a good job of describing\n> the isolated changes. If there _are_ problems, it will be easy to\n> identify the reasoning behind the code using log/blame.\n\nDoh, I should have mentioned that this series drops the number of\nfailures in the testsuite when run under GIT_TEST_MERGE_ALGORITHM=ort\nfrom 1453 to 1448. (Due to the final patch adding handling for\nmodify/delete conflicts.) It feels almost like an oversight since the\nnumber only dropped by 5 and the focus was on setting up the next\nthree series which will drop it much more. The next three, which I'm\nplanning to submit soon, will collectively drop the number of failures\nby 1388 down to just 60.\n\nAnyway, thanks for looking it over!\n"}]}