{"thread":{"id":"55312","subject":"[PATCH 6/8] diffcore-rename: add computation of number of unknown renames","startedAt":"2021-03-13T22:27:22Z","lastAt":"2021-03-28T02:14:52Z","messageCount":14,"participants":["Elijah Newren via GitGitGadget","Derrick Stolee","Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"419051","messageId":"b62e26fb1213b69a2008d4ec2c2cb862329ee0b9.1615674128.git.gitgitgadget@gmail.com","threadId":"55312","inReplyTo":"pull.853.git.1615674128.gitgitgadget@gmail.com","subject":"[PATCH 6/8] diffcore-rename: add computation of number of unknown renames","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-13T22:22:06Z","receivedAt":"2021-03-13T22:27:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe previous commit can only be effective if we have a computation of\nthe number of paths under a given directory which are still have pending\nrenames, and expected this number to be recorded in the dir_rename_count\nmap under the key UNKNOWN_DIR. Add the code necessary to compute these\nvalues.\n\nNote that this change means dir_rename_count might have a directory\nwhose only entry (for UNKNOWN_DIR) was removed by the time merge-ort\ngoes to check it. To account for this, merge-ort needs to check for the\ncase where the max count is 0.\n\nWith this change we are now computing the necessary value for each\ndirectory in dirs_removed, but are not using that value anywhere. The\nnext two commits will make use of the values stored in dirs_removed in\norder to compute whether each relevant_source (that is needed only for\ndirectory rename detection) has become unnecessary.\n\nSigned-off-by: Elijah Newren \n---\n diffcore-rename.c | 41 +++++++++++++++++++++++++++++++++++++----\n merge-ort.c | 3 +++\n 2 files changed, 40 insertions(+), 4 deletions(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 8fa29076e0aa..9844cd48788e 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -699,7 +699,8 @@ static void cleanup_dir_rename_info(struct dir_rename_info *info,\n \t/*\n \t * Although dir_rename_count was passed in\n \t * diffcore_rename_extended() and we want to keep it around and\n-\t * return it to that caller, we first want to remove any data\n+\t * return it to that caller, we first want to remove any counts in\n+\t * the maps associated with UNKNOWN_DIR entries and any data\n \t * associated with directories that weren't renamed.\n \t */\n \tstrmap_for_each_entry(info->dir_rename_count, &iter, entry) {\n@@ -711,6 +712,9 @@ static void cleanup_dir_rename_info(struct dir_rename_info *info,\n \t\t\tstrintmap_clear(counts);\n \t\t\tcontinue;\n \t\t}\n+\n+\t\tif (strintmap_contains(counts, UNKNOWN_DIR))\n+\t\t\tstrintmap_remove(counts, UNKNOWN_DIR);\n \t}\n \tfor (i = 0; i < to_remove.nr; ++i)\n \t\tstrmap_remove(info->dir_rename_count,\n@@ -1125,6 +1129,7 @@ static void handle_early_known_dir_renames(struct dir_rename_info *info,\n \t * a majority.\n \t */\n \n+\tint i;\n \tstruct hashmap_iter iter;\n \tstruct strmap_entry *entry;\n \n@@ -1134,10 +1139,38 @@ static void handle_early_known_dir_renames(struct dir_rename_info *info,\n \t\treturn; /* culling incompatbile with break detection */\n \n \t/*\n-\t * FIXME: Supplement dir_rename_count with number of potential\n-\t * renames, marking all potential rename sources as mapping to\n-\t * UNKNOWN_DIR.\n+\t * Supplement dir_rename_count with number of potential renames,\n+\t * marking all potential rename sources as mapping to UNKNOWN_DIR.\n \t */\n+\tfor (i = 0; i < rename_src_nr; i++) {\n+\t\tchar *old_dir;\n+\t\tstruct diff_filespec *one = rename_src[i].p->one;\n+\n+\t\t/*\n+\t\t * sources that are part of a rename will have already been\n+\t\t * removed by a prior call to remove_unneeded_paths_from_src()\n+\t\t */\n+\t\tassert(!one->rename_used);\n+\n+\t\told_dir = get_dirname(one->path);\n+\t\twhile (*old_dir != '\\0' &&\n+\t\t NOT_RELEVANT != strintmap_get(dirs_removed, old_dir)) {\n+\t\t\tchar *freeme = old_dir;\n+\n+\t\t\tincrement_count(info, old_dir, UNKNOWN_DIR);\n+\t\t\told_dir = get_dirname(old_dir);\n+\n+\t\t\t/* Free resources we don't need anymore */\n+\t\t\tfree(freeme);\n+\t\t}\n+\t\t/*\n+\t\t * old_dir and new_dir free'd in increment_count, but\n+\t\t * get_dirname() gives us a new pointer we need to free for\n+\t\t * old_dir. Also, if the loop runs 0 times we need old_dir\n+\t\t * to be freed.\n+\t\t */\n+\t\tfree(old_dir);\n+\t}\n \n \t/*\n \t * For any directory which we need a potential rename detected for\ndiff --git a/merge-ort.c b/merge-ort.c\nindex e2606c73ad88..f2b259986e22 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1546,6 +1546,9 @@ static void get_provisional_directory_renames(struct merge_options *opt,\n \t\t\t}\n \t\t}\n \n+\t\tif (max == 0)\n+\t\t\tcontinue;\n+\n \t\tif (bad_max == max) {\n \t\t\tpath_msg(opt, source_dir, 0,\n \t\t\t _(\"CONFLICT (directory rename split): \"\n-- \ngitgitgadget\n\n"},{"id":"419052","messageId":"9e30f4399abefc8af005a2de97eefe692fdb4848.1615674128.git.gitgitgadget@gmail.com","threadId":"55312","inReplyTo":"pull.853.git.1615674128.gitgitgadget@gmail.com","subject":"[PATCH 4/8] diffcore-rename: only compute dir_rename_count for relevant directories","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-13T22:22:04Z","receivedAt":"2021-03-13T22:27:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen one side adds files to a directory that the other side renamed,\ndirectory rename detection is used to either move the new paths to the\nnewer directory or warn the user about the fact that another path\nlocation might be better.\n\nIf a parent of the given directory had new files added to it, any\nrenames in the current directory are also part of determining where the\nparent directory is renamed to. Thus, naively, we need to record each\nrename N times for a path at depth N. However, we can use the\nadditional information added to dirs_removed in the last commit to avoid\ntraversing all N parent directories in many cases. Let's use an example\nto explain how this works. If we have a path named\n src/old_dir/a/b/file.c\nand src/old_dir doesn't exist on one side of history, but the other\nadded a file named src/old_dir/newfile.c, then if one side renamed\n src/old_dir/a/b/file.c => source/new_dir/a/b/file.c\nthen this file would affect potential directory rename detection counts\nfor\n src/old_dir/a/b => source/new_dir/a/b\n src/old_dir/a => source/new_dir/a\n src/old_dir => source/new_dir\n src => source\nadding a weight of 1 to each in dir_rename_counts. However, if src/\nexists on both sides of history, then we don't need to track any entries\nfor it in dir_rename_counts. That was implemented previously. What we\nare adding now, is that if no new files were added to src/old_dir/a or\nsrc/old_dir/b, then we don't need to have counts in dir_rename_count\nfor those directories either.\n\nIn short, we only need to track counts in dir_rename_count for\ndirectories whose dirs_removed value is RELEVANT_FOR_SELF. And as soon\nas we reach a directory that isn't in dirs_removed (signalled by\nreturning the default value of NOT_RELEVANT from strintmap_get()), we\ncan stop looking any further up the directory hierarchy.\n\nSigned-off-by: Elijah Newren \n---\n diffcore-rename.c | 27 ++++++++++++++++++++++-----\n 1 file changed, 22 insertions(+), 5 deletions(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex fafec66b29e9..0a17abd46691 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -461,6 +461,8 @@ static void update_dir_rename_counts(struct dir_rename_info *info,\n \t\treturn;\n \n \twhile (1) {\n+\t\tint drd_flag = NOT_RELEVANT;\n+\n \t\t/* Get old_dir, skip if its directory isn't relevant. */\n \t\tdirname_munge(old_dir);\n \t\tif (info->relevant_source_dirs &&\n@@ -509,16 +511,31 @@ static void update_dir_rename_counts(struct dir_rename_info *info,\n \t\t\t}\n \t\t}\n \n-\t\tif (strintmap_contains(dirs_removed, old_dir))\n+\t\t/*\n+\t\t * Above we suggested that we'd keep recording renames for\n+\t\t * all ancestor directories where the trailing directories\n+\t\t * matched, i.e. for\n+\t\t * \"a/b/c/d/e/foo.c\" -> \"a/b/some/thing/else/e/foo.c\"\n+\t\t * we'd increment rename counts for each of\n+\t\t * a/b/c/d/e/ => a/b/some/thing/else/e/\n+\t\t * a/b/c/d/ => a/b/some/thing/else/\n+\t\t * However, we only need the rename counts for directories\n+\t\t * in dirs_removed whose value is RELEVANT_FOR_SELF.\n+\t\t * However, we add one special case of also recording it for\n+\t\t * first_time_in_loop because find_basename_matches() can\n+\t\t * use that as a hint to find a good pairing.\n+\t\t */\n+\t\tif (dirs_removed)\n+\t\t\tdrd_flag = strintmap_get(dirs_removed, old_dir);\n+\t\tif (drd_flag == RELEVANT_FOR_SELF || first_time_in_loop)\n \t\t\tincrement_count(info, old_dir, new_dir);\n-\t\telse\n-\t\t\tbreak;\n \n+\t\tfirst_time_in_loop = 0;\n+\t\tif (drd_flag == NOT_RELEVANT)\n+\t\t\tbreak;\n \t\t/* If we hit toplevel directory (\"\") for old or new dir, quit */\n \t\tif (!*old_dir || !*new_dir)\n \t\t\tbreak;\n-\n-\t\tfirst_time_in_loop = 0;\n \t}\n \n \t/* Free resources we don't need anymore */\n-- \ngitgitgadget\n\n"},{"id":"419053","messageId":"960dcf365b350ef7166bd4f92d48ed016502f101.1615674128.git.gitgitgadget@gmail.com","threadId":"55312","inReplyTo":"pull.853.git.1615674128.gitgitgadget@gmail.com","subject":"[PATCH 1/8] diffcore-rename: take advantage of \"majority rules\" to skip more renames","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-13T22:22:01Z","receivedAt":"2021-03-13T22:27:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIn directory rename detection (when a directory is removed on one side\nof history and the other side adds new files to that directory), we work\nto find where the greatest number of files within that directory were\nrenamed to so that the new files can be moved with the majority of the\nfiles.\n\nNaively, we can just do this by detecting renames for *all* files within\nthe removed/renamed directory, looking at all the destination\ndirectories where files within that directory were moved, and if there\nis more than one such directory then taking the one with the greatest\nnumber of files as the directory where the old directory was renamed to.\n\nHowever, sometimes there are enough renames from exact rename detection\nor basename-guided rename detection that we have enough information to\ndetermine the majority winner already. Add a function meant to compute\nwhether particular renames are still needed based on this majority rules\ncheck. The next several commits will then add the necessary\ninfrastructure to get the information we need to compute which\nadditional rename sources we can skip.\n\nAn important side note for future further optimization:\n\nThere is a possible improvement to this optimization that I have not yet\nattempted and will not be included in this series of patches: we could\nfirst check whether exact renames provide enough information for us to\ndetermine directory renames, and avoid doing basename-guided rename\ndetection on some or all of the RELEVANT_LOCATION files within those\ndirectories. In effect, this variant would mean doing the\nhandle_early_known_dir_renames() both after exact rename detection and\nagain after basename-guided rename detection, though it would also mean\ndecrementing the number of \"unknown\" renames for each rename we found\nfrom basename-guided rename detection. Adding this additional check for\nskippable renames right after exact rename detection might turn out to\nbe valuable, especially for partial clones where it might allow us to\ndownload certain source files entirely. However, this particular\noptimization was actually the last one I did in original implementation\norder, and by the time I implemented this idea, every testcase I had was\nsufficiently fast that further optimization was unwarranted. If future\ntestcases arise that tax rename detection more heavily (or perhaps\npartial clones can benefit from avoiding loading more objects), it may\nbe worth implementing this more involved variant.\n\nSigned-off-by: Elijah Newren \n---\n diffcore-rename.c | 25 +++++++++++++++++++++++++\n 1 file changed, 25 insertions(+)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex e8508541be14..a5d10afa221a 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -1073,6 +1073,24 @@ static void remove_unneeded_paths_from_src(int detecting_copies,\n \trename_src_nr = new_num_src;\n }\n \n+static void handle_early_known_dir_renames(struct dir_rename_info *info,\n+\t\t\t\t\t struct strset *relevant_sources,\n+\t\t\t\t\t struct strset *dirs_removed)\n+{\n+\t/*\n+\t * Not yet implemented; directory renames are determined via an\n+\t * aggregate of all renames under them and using a \"majority wins\"\n+\t * rule. The fact that \"majority wins\", though, means we don't need\n+\t * all the renames under the given directory, we only need enough to\n+\t * ensure we have a majority.\n+\t *\n+\t * For now, we don't have enough information to know if we have a\n+\t * majority after exact renames and basename-guided rename detection,\n+\t * so just return early without doing any extra filtering.\n+\t */\n+\treturn;\n+}\n+\n void diffcore_rename_extended(struct diff_options *options,\n \t\t\t struct strset *relevant_sources,\n \t\t\t struct strset *dirs_removed,\n@@ -1208,9 +1226,16 @@ void diffcore_rename_extended(struct diff_options *options,\n \t\t * Cull sources, again:\n \t\t * - remove ones involved in renames (found via basenames)\n \t\t * - remove ones not found in relevant_sources\n+\t\t * and\n+\t\t * - remove ones in relevant_sources which are needed only\n+\t\t * for directory renames IF no ancestory directory\n+\t\t * actually needs to know any more individual path\n+\t\t * renames under them\n \t\t */\n \t\ttrace2_region_enter(\"diff\", \"cull basename\", options->repo);\n \t\tremove_unneeded_paths_from_src(want_copies, relevant_sources);\n+\t\thandle_early_known_dir_renames(&info, relevant_sources,\n+\t\t\t\t\t dirs_removed);\n \t\ttrace2_region_leave(\"diff\", \"cull basename\", options->repo);\n \t}\n \n-- \ngitgitgadget\n\n"},{"id":"419054","messageId":"a2dadbdf0d92f5c7cf466950e119e5c51014f118.1615674128.git.gitgitgadget@gmail.com","threadId":"55312","inReplyTo":"pull.853.git.1615674128.gitgitgadget@gmail.com","subject":"[PATCH 5/8] diffcore-rename: check if we have enough renames for directories early on","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-13T22:22:05Z","receivedAt":"2021-03-13T22:27:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nAs noted in the past few commits, if we can determine that a directory\nalready has enough renames to determine how directory rename detection\nwill be decided for that directory, then we can mark that directory as\nno longer needing any more renames detected for files underneath it.\nFor such directories, we change the value in the dirs_removed map from\nRELEVANT_TO_SELF to RELEVANT_FOR_ANCESTOR.\n\nA subsequent patch will use this information while iterating over the\nremaining potential rename sources to mark ones that were only\nlocation_relevant as unneeded if no containing directory is still marked\nas RELEVANT_TO_SELF.\n\nSigned-off-by: Elijah Newren \n---\n diffcore-rename.c | 73 ++++++++++++++++++++++++++++++++++++++++-------\n 1 file changed, 63 insertions(+), 10 deletions(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 0a17abd46691..8fa29076e0aa 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -407,6 +407,28 @@ static const char *get_highest_rename_path(struct strintmap *counts)\n \treturn highest_destination_dir;\n }\n \n+static char *UNKNOWN_DIR = \"/\"; /* placeholder -- short, illegal directory */\n+\n+static int dir_rename_already_determinable(struct strintmap *counts)\n+{\n+\tstruct hashmap_iter iter;\n+\tstruct strmap_entry *entry;\n+\tint first = 0, second = 0, unknown = 0;\n+\tstrintmap_for_each_entry(counts, &iter, entry) {\n+\t\tconst char *destination_dir = entry->key;\n+\t\tintptr_t count = (intptr_t)entry->value;\n+\t\tif (!strcmp(destination_dir, UNKNOWN_DIR)) {\n+\t\t\tunknown = count;\n+\t\t} else if (count >= first) {\n+\t\t\tsecond = first;\n+\t\t\tfirst = count;\n+\t\t} else if (count >= second) {\n+\t\t\tsecond = count;\n+\t\t}\n+\t}\n+\treturn first > second + unknown;\n+}\n+\n static void increment_count(struct dir_rename_info *info,\n \t\t\t char *old_dir,\n \t\t\t char *new_dir)\n@@ -1096,17 +1118,48 @@ static void handle_early_known_dir_renames(struct dir_rename_info *info,\n \t\t\t\t\t struct strintmap *dirs_removed)\n {\n \t/*\n-\t * Not yet implemented; directory renames are determined via an\n-\t * aggregate of all renames under them and using a \"majority wins\"\n-\t * rule. The fact that \"majority wins\", though, means we don't need\n-\t * all the renames under the given directory, we only need enough to\n-\t * ensure we have a majority.\n-\t *\n-\t * For now, we don't have enough information to know if we have a\n-\t * majority after exact renames and basename-guided rename detection,\n-\t * so just return early without doing any extra filtering.\n+\t * Directory renames are determined via an aggregate of all renames\n+\t * under them and using a \"majority wins\" rule. The fact that\n+\t * \"majority wins\", though, means we don't need all the renames\n+\t * under the given directory, we only need enough to ensure we have\n+\t * a majority.\n \t */\n-\treturn;\n+\n+\tstruct hashmap_iter iter;\n+\tstruct strmap_entry *entry;\n+\n+\tif (!dirs_removed || !relevant_sources)\n+\t\treturn; /* nothing to cull */\n+\tif (break_idx)\n+\t\treturn; /* culling incompatbile with break detection */\n+\n+\t/*\n+\t * FIXME: Supplement dir_rename_count with number of potential\n+\t * renames, marking all potential rename sources as mapping to\n+\t * UNKNOWN_DIR.\n+\t */\n+\n+\t/*\n+\t * For any directory which we need a potential rename detected for\n+\t * (i.e. those marked as RELEVANT_FOR_SELF in dirs_removed), check\n+\t * whether we have enough renames to satisfy the \"majority rules\"\n+\t * requirement such that detecting any more renames of files under\n+\t * it won't change the result. For any such directory, mark that\n+\t * we no longer need to detect a rename for it. However, since we\n+\t * might need to still detect renames for an ancestor of that\n+\t * directory, use RELEVANT_FOR_ANCESTOR.\n+\t */\n+\tstrmap_for_each_entry(info->dir_rename_count, &iter, entry) {\n+\t\t/* entry->key is source_dir */\n+\t\tstruct strintmap *counts = entry->value;\n+\n+\t\tif (strintmap_get(dirs_removed, entry->key) ==\n+\t\t RELEVANT_FOR_SELF &&\n+\t\t dir_rename_already_determinable(counts)) {\n+\t\t\tstrintmap_set(dirs_removed, entry->key,\n+\t\t\t\t RELEVANT_FOR_ANCESTOR);\n+\t\t}\n+\t}\n }\n \n void diffcore_rename_extended(struct diff_options *options,\n-- \ngitgitgadget\n\n"},{"id":"419055","messageId":"927252a1cb45cc34ee964fdf59fe06558f4d792f.1615674128.git.gitgitgadget@gmail.com","threadId":"55312","inReplyTo":"pull.853.git.1615674128.gitgitgadget@gmail.com","subject":"[PATCH 2/8] merge-ort, diffcore-rename: tweak dirs_removed and relevant_source type","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-13T22:22:02Z","receivedAt":"2021-03-13T22:27:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nAs noted in the previous commit, we want to be able to take advantage of\nthe \"majority rules\" portion of directory rename detection to avoid\ndetecting more renames than necessary. However, for diffcore-rename to\ntake advantage of that, it needs to know whether a rename source file\nwas needed for just directory rename detection reasons, or if it is\nwanted for potential three-way content merging. Modify relevant_sources\nfrom a strset to a strintmap, so we can encode additional information.\n\nWe also modify dirs_removed from a strset to a strintmap at the same\ntime because trying to determine what files are needed for directory\nrename detection will require us tracking a bit more information for\neach directory.\n\nThis commit only changes the types of the two variables from strset to\nstrintmap; it does not actually store any special values yet and for now\nonly checks for presence of entries in the strintmap. Thus, the code is\nfunctionally identical to how it behaved before. Future commits will\nstart associating values with each key for these two maps.\n\nSigned-off-by: Elijah Newren \n---\n diffcore-rename.c | 47 ++++++++++++++++++++++++-----------------------\n diffcore.h | 6 +++---\n merge-ort.c | 28 ++++++++++++++--------------\n 3 files changed, 41 insertions(+), 40 deletions(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex a5d10afa221a..6487825317ff 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -371,7 +371,7 @@ struct dir_rename_info {\n \tstruct strintmap idx_map;\n \tstruct strmap dir_rename_guess;\n \tstruct strmap *dir_rename_count;\n-\tstruct strset *relevant_source_dirs;\n+\tstruct strintmap *relevant_source_dirs;\n \tunsigned setup;\n };\n \n@@ -429,7 +429,7 @@ static void increment_count(struct dir_rename_info *info,\n }\n \n static void update_dir_rename_counts(struct dir_rename_info *info,\n-\t\t\t\t struct strset *dirs_removed,\n+\t\t\t\t struct strintmap *dirs_removed,\n \t\t\t\t const char *oldname,\n \t\t\t\t const char *newname)\n {\n@@ -464,7 +464,7 @@ static void update_dir_rename_counts(struct dir_rename_info *info,\n \t\t/* Get old_dir, skip if its directory isn't relevant. */\n \t\tdirname_munge(old_dir);\n \t\tif (info->relevant_source_dirs &&\n-\t\t !strset_contains(info->relevant_source_dirs, old_dir))\n+\t\t !strintmap_contains(info->relevant_source_dirs, old_dir))\n \t\t\tbreak;\n \n \t\t/* Get new_dir */\n@@ -509,7 +509,7 @@ static void update_dir_rename_counts(struct dir_rename_info *info,\n \t\t\t}\n \t\t}\n \n-\t\tif (strset_contains(dirs_removed, old_dir))\n+\t\tif (strintmap_contains(dirs_removed, old_dir))\n \t\t\tincrement_count(info, old_dir, new_dir);\n \t\telse\n \t\t\tbreak;\n@@ -527,8 +527,8 @@ static void update_dir_rename_counts(struct dir_rename_info *info,\n }\n \n static void initialize_dir_rename_info(struct dir_rename_info *info,\n-\t\t\t\t struct strset *relevant_sources,\n-\t\t\t\t struct strset *dirs_removed,\n+\t\t\t\t struct strintmap *relevant_sources,\n+\t\t\t\t struct strintmap *dirs_removed,\n \t\t\t\t struct strmap *dir_rename_count)\n {\n \tstruct hashmap_iter iter;\n@@ -555,12 +555,13 @@ static void initialize_dir_rename_info(struct dir_rename_info *info,\n \t\tinfo->relevant_source_dirs = dirs_removed; /* might be NULL */\n \t} else {\n \t\tinfo->relevant_source_dirs = xmalloc(sizeof(struct strintmap));\n-\t\tstrset_init(info->relevant_source_dirs);\n-\t\tstrset_for_each_entry(relevant_sources, &iter, entry) {\n+\t\tstrintmap_init(info->relevant_source_dirs, 0 /* unused */);\n+\t\tstrintmap_for_each_entry(relevant_sources, &iter, entry) {\n \t\t\tchar *dirname = get_dirname(entry->key);\n \t\t\tif (!dirs_removed ||\n-\t\t\t strset_contains(dirs_removed, dirname))\n-\t\t\t\tstrset_add(info->relevant_source_dirs, dirname);\n+\t\t\t strintmap_contains(dirs_removed, dirname))\n+\t\t\t\tstrintmap_set(info->relevant_source_dirs,\n+\t\t\t\t\t dirname, 0 /* value irrelevant */);\n \t\t\tfree(dirname);\n \t\t}\n \t}\n@@ -624,7 +625,7 @@ void partial_clear_dir_rename_count(struct strmap *dir_rename_count)\n }\n \n static void cleanup_dir_rename_info(struct dir_rename_info *info,\n-\t\t\t\t struct strset *dirs_removed,\n+\t\t\t\t struct strintmap *dirs_removed,\n \t\t\t\t int keep_dir_rename_count)\n {\n \tstruct hashmap_iter iter;\n@@ -644,7 +645,7 @@ static void cleanup_dir_rename_info(struct dir_rename_info *info,\n \t/* relevant_source_dirs */\n \tif (info->relevant_source_dirs &&\n \t info->relevant_source_dirs != dirs_removed) {\n-\t\tstrset_clear(info->relevant_source_dirs);\n+\t\tstrintmap_clear(info->relevant_source_dirs);\n \t\tFREE_AND_NULL(info->relevant_source_dirs);\n \t}\n \n@@ -666,7 +667,7 @@ static void cleanup_dir_rename_info(struct dir_rename_info *info,\n \t\tconst char *source_dir = entry->key;\n \t\tstruct strintmap *counts = entry->value;\n \n-\t\tif (!strset_contains(dirs_removed, source_dir)) {\n+\t\tif (!strintmap_contains(dirs_removed, source_dir)) {\n \t\t\tstring_list_append(&to_remove, source_dir);\n \t\t\tstrintmap_clear(counts);\n \t\t\tcontinue;\n@@ -770,8 +771,8 @@ static int idx_possible_rename(char *filename, struct dir_rename_info *info)\n static int find_basename_matches(struct diff_options *options,\n \t\t\t\t int minimum_score,\n \t\t\t\t struct dir_rename_info *info,\n-\t\t\t\t struct strset *relevant_sources,\n-\t\t\t\t struct strset *dirs_removed)\n+\t\t\t\t struct strintmap *relevant_sources,\n+\t\t\t\t struct strintmap *dirs_removed)\n {\n \t/*\n \t * When I checked in early 2020, over 76% of file renames in linux\n@@ -863,7 +864,7 @@ static int find_basename_matches(struct diff_options *options,\n \n \t\t/* Skip irrelevant sources */\n \t\tif (relevant_sources &&\n-\t\t !strset_contains(relevant_sources, filename))\n+\t\t !strintmap_contains(relevant_sources, filename))\n \t\t\tcontinue;\n \n \t\t/*\n@@ -994,7 +995,7 @@ static int find_renames(struct diff_score *mx,\n \t\t\tint minimum_score,\n \t\t\tint copies,\n \t\t\tstruct dir_rename_info *info,\n-\t\t\tstruct strset *dirs_removed)\n+\t\t\tstruct strintmap *dirs_removed)\n {\n \tint count = 0, i;\n \n@@ -1019,7 +1020,7 @@ static int find_renames(struct diff_score *mx,\n }\n \n static void remove_unneeded_paths_from_src(int detecting_copies,\n-\t\t\t\t\t struct strset *interesting)\n+\t\t\t\t\t struct strintmap *interesting)\n {\n \tint i, new_num_src;\n \n@@ -1061,7 +1062,7 @@ static void remove_unneeded_paths_from_src(int detecting_copies,\n \t\t\tcontinue;\n \n \t\t/* If we don't care about the source path, skip it */\n-\t\tif (interesting && !strset_contains(interesting, one->path))\n+\t\tif (interesting && !strintmap_contains(interesting, one->path))\n \t\t\tcontinue;\n \n \t\tif (new_num_src < i)\n@@ -1074,8 +1075,8 @@ static void remove_unneeded_paths_from_src(int detecting_copies,\n }\n \n static void handle_early_known_dir_renames(struct dir_rename_info *info,\n-\t\t\t\t\t struct strset *relevant_sources,\n-\t\t\t\t\t struct strset *dirs_removed)\n+\t\t\t\t\t struct strintmap *relevant_sources,\n+\t\t\t\t\t struct strintmap *dirs_removed)\n {\n \t/*\n \t * Not yet implemented; directory renames are determined via an\n@@ -1092,8 +1093,8 @@ static void handle_early_known_dir_renames(struct dir_rename_info *info,\n }\n \n void diffcore_rename_extended(struct diff_options *options,\n-\t\t\t struct strset *relevant_sources,\n-\t\t\t struct strset *dirs_removed,\n+\t\t\t struct strintmap *relevant_sources,\n+\t\t\t struct strintmap *dirs_removed,\n \t\t\t struct strmap *dir_rename_count)\n {\n \tint detect_rename = options->detect_rename;\ndiff --git a/diffcore.h b/diffcore.h\nindex 737c93a6cc79..4f168b385fde 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -8,8 +8,8 @@\n \n struct diff_options;\n struct repository;\n+struct strintmap;\n struct strmap;\n-struct strset;\n struct userdiff_driver;\n \n /* This header file is internal between diff.c and its diff transformers\n@@ -166,8 +166,8 @@ void partial_clear_dir_rename_count(struct strmap *dir_rename_count);\n void diffcore_break(struct repository *, int);\n void diffcore_rename(struct diff_options *);\n void diffcore_rename_extended(struct diff_options *options,\n-\t\t\t struct strset *relevant_sources,\n-\t\t\t struct strset *dirs_removed,\n+\t\t\t struct strintmap *relevant_sources,\n+\t\t\t struct strintmap *dirs_removed,\n \t\t\t struct strmap *dir_rename_count);\n void diffcore_merge_broken(void);\n void diffcore_pickaxe(struct diff_options *);\ndiff --git a/merge-ort.c b/merge-ort.c\nindex bd089cb9a76f..6fa670396ce4 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -74,7 +74,7 @@ struct rename_info {\n \t/*\n \t * dirs_removed: directories removed on a given side of history.\n \t */\n-\tstruct strset dirs_removed[3];\n+\tstruct strintmap dirs_removed[3];\n \n \t/*\n \t * dir_rename_count: tracking where parts of a directory were renamed to\n@@ -106,7 +106,7 @@ struct rename_info {\n \t * If neither of those are true, we can skip rename detection for\n \t * that path.\n \t */\n-\tstruct strset relevant_sources[3];\n+\tstruct strintmap relevant_sources[3];\n \n \t/*\n \t * dir_rename_mask:\n@@ -362,8 +362,8 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,\n \tint i;\n \tvoid (*strmap_func)(struct strmap *, int) =\n \t\treinitialize ? strmap_partial_clear : strmap_clear;\n-\tvoid (*strset_func)(struct strset *) =\n-\t\treinitialize ? strset_partial_clear : strset_clear;\n+\tvoid (*strintmap_func)(struct strintmap *) =\n+\t\treinitialize ? strintmap_partial_clear : strintmap_clear;\n \n \t/*\n \t * We marked opti->paths with strdup_strings = 0, so that we\n@@ -395,7 +395,7 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,\n \n \t/* Free memory used by various renames maps */\n \tfor (i = MERGE_SIDE1; i <= MERGE_SIDE2; ++i) {\n-\t\tstrset_func(&renames->dirs_removed[i]);\n+\t\tstrintmap_func(&renames->dirs_removed[i]);\n \n \t\tpartial_clear_dir_rename_count(&renames->dir_rename_count[i]);\n \t\tif (!reinitialize)\n@@ -403,7 +403,7 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,\n \n \t\tstrmap_func(&renames->dir_renames[i], 0);\n \n-\t\tstrset_func(&renames->relevant_sources[i]);\n+\t\tstrintmap_func(&renames->relevant_sources[i]);\n \t}\n \n \tif (!reinitialize) {\n@@ -674,7 +674,7 @@ static void add_pair(struct merge_options *opt,\n \t\tunsigned location_relevant = (dir_rename_mask == 0x07);\n \n \t\tif (content_relevant || location_relevant)\n-\t\t\tstrset_add(&renames->relevant_sources[side], pathname);\n+\t\t\tstrintmap_set(&renames->relevant_sources[side], pathname, 1);\n \t}\n \n \tone = alloc_filespec(pathname);\n@@ -730,9 +730,9 @@ static void collect_rename_info(struct merge_options *opt,\n \t\t/* absent_mask = 0x07 - dirmask; sides = absent_mask/2 */\n \t\tunsigned sides = (0x07 - dirmask)/2;\n \t\tif (sides & 1)\n-\t\t\tstrset_add(&renames->dirs_removed[1], fullname);\n+\t\t\tstrintmap_set(&renames->dirs_removed[1], fullname, 1);\n \t\tif (sides & 2)\n-\t\t\tstrset_add(&renames->dirs_removed[2], fullname);\n+\t\t\tstrintmap_set(&renames->dirs_removed[2], fullname, 1);\n \t}\n \n \tif (filemask == 0 || filemask == 7)\n@@ -2161,7 +2161,7 @@ static inline int possible_side_renames(struct rename_info *renames,\n \t\t\t\t\tunsigned side_index)\n {\n \treturn renames->pairs[side_index].nr > 0 &&\n-\t !strset_empty(&renames->relevant_sources[side_index]);\n+\t !strintmap_empty(&renames->relevant_sources[side_index]);\n }\n \n static inline int possible_renames(struct rename_info *renames)\n@@ -3445,14 +3445,14 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)\n \t/* Initialization of various renames fields */\n \trenames = &opt->priv->renames;\n \tfor (i = MERGE_SIDE1; i <= MERGE_SIDE2; i++) {\n-\t\tstrset_init_with_options(&renames->dirs_removed[i],\n-\t\t\t\t\t NULL, 0);\n+\t\tstrintmap_init_with_options(&renames->dirs_removed[i],\n+\t\t\t\t\t 0, NULL, 0);\n \t\tstrmap_init_with_options(&renames->dir_rename_count[i],\n \t\t\t\t\t NULL, 1);\n \t\tstrmap_init_with_options(&renames->dir_renames[i],\n \t\t\t\t\t NULL, 0);\n-\t\tstrset_init_with_options(&renames->relevant_sources[i],\n-\t\t\t\t\t NULL, 0);\n+\t\tstrintmap_init_with_options(&renames->relevant_sources[i],\n+\t\t\t\t\t 0, NULL, 0);\n \t}\n \n \t/*\n-- \ngitgitgadget\n\n"},{"id":"419056","messageId":"pull.853.git.1615674128.gitgitgadget@gmail.com","threadId":"55312","inReplyTo":null,"subject":"[PATCH 0/8] Optimization batch 10: avoid detecting even more irrelevant renames","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-13T22:22:00Z","receivedAt":"2021-03-13T22:27:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series depends on ort-perf-batch-9.\n\n=== Basic Optimization idea ===\n\nThis series adds additional special cases where detection of renames is\nirrelevant, where the irrelevance is due to the fact that the merge\nmachinery will arrive at the same result regardless of whether a rename is\ndetected for any of those paths. That high level wording makes it sound the\nsame as ort-perf-batch-9, and basically it is, it's just trying to take the\noptimization a step further.\n\nAs noted in the last series, there are two reasons that the merge machinery\nneeds renames:\n\n * in order to do three-way content merging (pairing appropriate files)\n * in order to find where directories have been renamed\n\nort-perf-batch-9 provided a rough approximation for the second criteria that\nwas good enough, but which still left us detecting more renames than\nnecessary. This series focuses further on that criteria and finds ways to\navoid the need to detect as many renames while still detecting directory\nrenames identically to before. Thus, this series is an improvement on\n\"Optimization #2\" from my Git Merge 2020 talk[1].\n\n=== Results ===\n\nFor the testcases mentioned in commit 557ac03 (\"merge-ort: begin performance\nwork; instrument with trace2_region_* calls\", 2020-10-28), the changes in\njust this series improves the performance as follows:\n\n Before Series After Series\nno-renames: 5.680 s ± 0.096 s 5.665 s ± 0.129 s \nmega-renames: 13.812 s ± 0.162 s 11.435 s ± 0.158 s\njust-one-mega: 506.0 ms ± 3.9 ms 494.2 ms ± 6.1 ms\n\n\nWhile those results may look somewhat meager, it is important to note that\nthe previous optimizations have already reduced rename detection time to\nnearly 0 for these particular testcases so there just isn't much left to\nimprove. The final patch in the series shows an alternate testcase where the\nprevious optimizations aren't as effective (a simple cherry-pick of a commit\nthat simply adds one new empty file), where there was a speedup factor of\napproximately 3 due to this series:\n\n Before Series After Series\npick-empty: 1.936 s ± 0.024 s 688.1 ms ± 4.2 ms\n\n\nThere was also another testcase at $DAYJOB where I saw a factor 7\nimprovement from this particular optimization, so it certainly has the\npotential to help when the previous optimizations are not quite enough.\n\nAs a reminder, before any merge-ort/diffcore-rename performance work, the\nperformance results we started with (as noted in the same commit message)\nwere:\n\nno-renames-am: 6.940 s ± 0.485 s\nno-renames: 18.912 s ± 0.174 s\nmega-renames: 5964.031 s ± 10.459 s\njust-one-mega: 149.583 s ± 0.751 s\n\n\n[1]\nhttps://github.com/newren/presentations/blob/pdfs/merge-performance/merge-performance-slides.pdf\n\nElijah Newren (8):\n diffcore-rename: take advantage of \"majority rules\" to skip more\n renames\n merge-ort, diffcore-rename: tweak dirs_removed and relevant_source\n type\n merge-ort: record the reason that we want a rename for a directory\n diffcore-rename: only compute dir_rename_count for relevant\n directories\n diffcore-rename: check if we have enough renames for directories early\n on\n diffcore-rename: add computation of number of unknown renames\n merge-ort: record the reason that we want a rename for a file\n diffcore-rename: determine which relevant_sources are no longer\n relevant\n\n diffcore-rename.c | 230 ++++++++++++++++++++++++++++++++++++++++------\n diffcore.h | 19 +++-\n merge-ort.c | 79 ++++++++++++----\n 3 files changed, 281 insertions(+), 47 deletions(-)\n\n\nbase-commit: 98b0c7de5e70d62d47c3eeb3d290c6a234214f40\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-853%2Fnewren%2Fort-perf-batch-10-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-853/newren/ort-perf-batch-10-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/853\n-- \ngitgitgadget\n"},{"id":"419057","messageId":"05850cb49823ea9c6d6c7cfd3b4adbbc1c47abc6.1615674128.git.gitgitgadget@gmail.com","threadId":"55312","inReplyTo":"pull.853.git.1615674128.gitgitgadget@gmail.com","subject":"[PATCH 3/8] merge-ort: record the reason that we want a rename for a directory","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-13T22:22:03Z","receivedAt":"2021-03-13T22:27:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen one side of history renames a directory, and the other side of\nhistory added files to the old directory, directory rename detection is\nused to warn about the location of the added files so the user can\nmove them to the old directory or keep them with the new one.\n\nThis sets up three different types of directories:\n * directories that had new files added to them\n * directories underneath a directory that had new files added to them\n * directories where no new files were added to it or any leading path\n\nSave this information in dirs_removed; the next several commits will\nmake use of this information.\n\nSigned-off-by: Elijah Newren \n---\n diffcore-rename.c | 2 +-\n diffcore.h | 7 +++++++\n merge-ort.c | 41 ++++++++++++++++++++++++++++++++++++++---\n 3 files changed, 46 insertions(+), 4 deletions(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 6487825317ff..fafec66b29e9 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -667,7 +667,7 @@ static void cleanup_dir_rename_info(struct dir_rename_info *info,\n \t\tconst char *source_dir = entry->key;\n \t\tstruct strintmap *counts = entry->value;\n \n-\t\tif (!strintmap_contains(dirs_removed, source_dir)) {\n+\t\tif (!strintmap_get(dirs_removed, source_dir)) {\n \t\t\tstring_list_append(&to_remove, source_dir);\n \t\t\tstrintmap_clear(counts);\n \t\t\tcontinue;\ndiff --git a/diffcore.h b/diffcore.h\nindex 4f168b385fde..d5a497b7a162 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -161,6 +161,13 @@ struct diff_filepair *diff_queue(struct diff_queue_struct *,\n \t\t\t\t struct diff_filespec *);\n void diff_q(struct diff_queue_struct *, struct diff_filepair *);\n \n+/* dir_rename_relevance: the reason we want rename information for a dir */\n+enum dir_rename_relevance {\n+\tNOT_RELEVANT = 0,\n+\tRELEVANT_FOR_ANCESTOR = 1,\n+\tRELEVANT_FOR_SELF = 2\n+};\n+\n void partial_clear_dir_rename_count(struct strmap *dir_rename_count);\n \n void diffcore_break(struct repository *, int);\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 6fa670396ce4..e2606c73ad88 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -73,6 +73,10 @@ struct rename_info {\n \n \t/*\n \t * dirs_removed: directories removed on a given side of history.\n+\t *\n+\t * The keys of dirs_removed[side] are the directories that were removed\n+\t * on the given side of history. The value of the strintmap for each\n+\t * directory is a value from enum dir_rename_relevance.\n \t */\n \tstruct strintmap dirs_removed[3];\n \n@@ -729,10 +733,41 @@ static void collect_rename_info(struct merge_options *opt,\n \tif (dirmask == 1 || dirmask == 3 || dirmask == 5) {\n \t\t/* absent_mask = 0x07 - dirmask; sides = absent_mask/2 */\n \t\tunsigned sides = (0x07 - dirmask)/2;\n+\t\tunsigned relevance = (renames->dir_rename_mask == 0x07) ?\n+\t\t\t\t\tRELEVANT_FOR_ANCESTOR : NOT_RELEVANT;\n+\t\t/*\n+\t\t * Record relevance of this directory. However, note that\n+\t\t * when collect_merge_info_callback() recurses into this\n+\t\t * directory and calls collect_rename_info() on paths\n+\t\t * within that directory, if we find a path that was added\n+\t\t * to this directory on the other side of history, we will\n+\t\t * upgrade this value to RELEVANT_FOR_SELF; see below.\n+\t\t */\n \t\tif (sides & 1)\n-\t\t\tstrintmap_set(&renames->dirs_removed[1], fullname, 1);\n+\t\t\tstrintmap_set(&renames->dirs_removed[1], fullname,\n+\t\t\t\t relevance);\n \t\tif (sides & 2)\n-\t\t\tstrintmap_set(&renames->dirs_removed[2], fullname, 1);\n+\t\t\tstrintmap_set(&renames->dirs_removed[2], fullname,\n+\t\t\t\t relevance);\n+\t}\n+\n+\t/*\n+\t * Here's the block that potentially upgrades to RELEVANT_FOR_SELF.\n+\t * When we run across a file added to a directory. In such a case,\n+\t * find the directory of the file and upgrade its relevance.\n+\t */\n+\tif (renames->dir_rename_mask == 0x07 &&\n+\t (filemask == 2 || filemask == 4)) {\n+\t\t/*\n+\t\t * Need directory rename for parent directory on other side\n+\t\t * of history from added file. Thus\n+\t\t * side = (~filemask & 0x06) >> 1\n+\t\t * or\n+\t\t * side = 3 - (filemask/2).\n+\t\t */\n+\t\tunsigned side = 3 - (filemask >> 1);\n+\t\tstrintmap_set(&renames->dirs_removed[side], dirname,\n+\t\t\t RELEVANT_FOR_SELF);\n \t}\n \n \tif (filemask == 0 || filemask == 7)\n@@ -3446,7 +3481,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)\n \trenames = &opt->priv->renames;\n \tfor (i = MERGE_SIDE1; i <= MERGE_SIDE2; i++) {\n \t\tstrintmap_init_with_options(&renames->dirs_removed[i],\n-\t\t\t\t\t 0, NULL, 0);\n+\t\t\t\t\t NOT_RELEVANT, NULL, 0);\n \t\tstrmap_init_with_options(&renames->dir_rename_count[i],\n \t\t\t\t\t NULL, 1);\n \t\tstrmap_init_with_options(&renames->dir_renames[i],\n-- \ngitgitgadget\n\n"},{"id":"419058","messageId":"495c10937b7f2b23b7d2a52c254e02ae9ce810a1.1615674128.git.gitgitgadget@gmail.com","threadId":"55312","inReplyTo":"pull.853.git.1615674128.gitgitgadget@gmail.com","subject":"[PATCH 8/8] diffcore-rename: determine which relevant_sources are no longer relevant","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-13T22:22:08Z","receivedAt":"2021-03-13T22:27:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nAs noted a few commits ago (\"diffcore-rename: only compute\ndir_rename_count for relevant directories\"), when a source file rename\nis used as part of directory rename detection, we need to increment\ncounts for each ancestor directory in dirs_removed with value\nRELEVANT_FOR_SELF. However, a few commits ago (\"diffcore-rename: check\nif we have enough renames for directories early on\"), we may have\ndowngraded all relevant ancestor directories from RELEVANT_FOR_SELF to\nRELEVANT_FOR_ANCESTOR.\n\nFor a given file, if no ancestor directory is found in dirs_removed with\na value of RELEVANT_FOR_SELF, then we can downgrade\nrelevant_source[PATH] from RELEVANT_LOCATION to RELEVANT_NO_MORE. This\nmeans we can skip detecting a rename for that particular path (and any\nother paths in the same directory).\n\nFor the testcases mentioned in commit 557ac0350d (\"merge-ort: begin\nperformance work; instrument with trace2_region_* calls\", 2020-10-28),\nthis change improves the performance as follows:\n\n Before After\n no-renames: 5.680 s ± 0.096 s 5.665 s ± 0.129 s\n mega-renames: 13.812 s ± 0.162 s 11.435 s ± 0.158 s\n just-one-mega: 506.0 ms ± 3.9 ms 494.2 ms ± 6.1 ms\n\nWhile this improvement looks rather modest for these testcases (because\nall the previous optimizations were sufficient to nearly remove all time\nspent in rename detection already), consider this alternative testcase\ntweaked from the ones in commit 557ac0350d as follows\n\n \n $ git switch -c add-empty-file v5.5\n $ >drivers/gpu/drm/i915/new-empty-file\n $ git add drivers/gpu/drm/i915/new-empty-file\n $ git commit -m \"new file\"\n $ git switch 5.4-rename\n $ git cherry-pick --strategy=ort add-empty-file\n\nFor this testcase, we see the following improvement:\n\n Before After\n pick-empty: 1.936 s ± 0.024 s 688.1 ms ± 4.2 ms\n\nSo roughly a factor of 3 speedup. At $DAYJOB, there was a particular\nrepository and cherry-pick that inspired this optimization; for that\ncase I saw a speedup factor of 7 with this optimization.\n\nSigned-off-by: Elijah Newren \n---\n diffcore-rename.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 50 insertions(+), 1 deletion(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 9844cd48788e..7cc24592617e 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -1129,7 +1129,7 @@ static void handle_early_known_dir_renames(struct dir_rename_info *info,\n \t * a majority.\n \t */\n \n-\tint i;\n+\tint i, new_num_src;\n \tstruct hashmap_iter iter;\n \tstruct strmap_entry *entry;\n \n@@ -1193,6 +1193,55 @@ static void handle_early_known_dir_renames(struct dir_rename_info *info,\n \t\t\t\t RELEVANT_FOR_ANCESTOR);\n \t\t}\n \t}\n+\n+\tfor (i = 0, new_num_src = 0; i < rename_src_nr; i++) {\n+\t\tstruct diff_filespec *one = rename_src[i].p->one;\n+\t\tint val;\n+\n+\t\tval = strintmap_get(relevant_sources, one->path);\n+\n+\t\t/*\n+\t\t * sources that were not found in relevant_sources should\n+\t\t * have already been removed by a prior call to\n+\t\t * remove_unneeded_paths_from_src()\n+\t\t */\n+\t\tassert(val != -1);\n+\n+\t\tif (val == RELEVANT_LOCATION) {\n+\t\t\tint removable = 1;\n+\t\t\tchar *dir = get_dirname(one->path);\n+\t\t\twhile (1) {\n+\t\t\t\tchar *freeme = dir;\n+\t\t\t\tint res = strintmap_get(dirs_removed, dir);\n+\n+\t\t\t\t/* Quit if not found or irrelevant */\n+\t\t\t\tif (res == NOT_RELEVANT)\n+\t\t\t\t\tbreak;\n+\t\t\t\t/* If RELEVANT_FOR_SELF, can't remove */\n+\t\t\t\tif (res == RELEVANT_FOR_SELF) {\n+\t\t\t\t\tremovable = 0;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t\t/* Else continue searching upwards */\n+\t\t\t\tassert(res == RELEVANT_FOR_ANCESTOR);\n+\t\t\t\tdir = get_dirname(dir);\n+\t\t\t\tfree(freeme);\n+\t\t\t}\n+\t\t\tfree(dir);\n+\t\t\tif (removable) {\n+\t\t\t\tstrintmap_set(relevant_sources, one->path,\n+\t\t\t\t\t RELEVANT_NO_MORE);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (new_num_src < i)\n+\t\t\tmemcpy(&rename_src[new_num_src], &rename_src[i],\n+\t\t\t sizeof(struct diff_rename_src));\n+\t\tnew_num_src++;\n+\t}\n+\n+\trename_src_nr = new_num_src;\n }\n \n void diffcore_rename_extended(struct diff_options *options,\n-- \ngitgitgadget\n"},{"id":"419059","messageId":"1de40b1b88adee4d40a141f1eb796676b18da670.1615674128.git.gitgitgadget@gmail.com","threadId":"55312","inReplyTo":"pull.853.git.1615674128.gitgitgadget@gmail.com","subject":"[PATCH 7/8] merge-ort: record the reason that we want a rename for a file","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-13T22:22:07Z","receivedAt":"2021-03-13T22:27:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThere are two different reasons we might want a rename for a file -- for\nthree-way content merging or as part of directory rename detection.\nRecord the reason. diffcore-rename will potentially be able to filter\nsome of the ones marked as needed only for directory rename detection,\nif it can determine those directory renames based solely on renames\nfound via exact rename detection and basename-guided rename detection.\n\nSigned-off-by: Elijah Newren \n---\n diffcore.h | 6 ++++++\n merge-ort.c | 15 ++++++++++-----\n 2 files changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/diffcore.h b/diffcore.h\nindex d5a497b7a162..cf8d4cb8617d 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -167,6 +167,12 @@ enum dir_rename_relevance {\n \tRELEVANT_FOR_ANCESTOR = 1,\n \tRELEVANT_FOR_SELF = 2\n };\n+/* file_rename_relevance: the reason(s) we want rename information for a file */\n+enum file_rename_relevance {\n+\tRELEVANT_NO_MORE = 0, /* i.e. NOT relevant */\n+\tRELEVANT_CONTENT = 1,\n+\tRELEVANT_LOCATION = 2\n+};\n \n void partial_clear_dir_rename_count(struct strmap *dir_rename_count);\n \ndiff --git a/merge-ort.c b/merge-ort.c\nindex f2b259986e22..7f5750ce6ab0 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -99,16 +99,18 @@ struct rename_info {\n \tstruct strmap dir_renames[3];\n \n \t/*\n-\t * relevant_sources: deleted paths for which we need rename detection\n+\t * relevant_sources: deleted paths wanted in rename detection, and why\n \t *\n \t * relevant_sources is a set of deleted paths on each side of\n \t * history for which we need rename detection. If a path is deleted\n \t * on one side of history, we need to detect if it is part of a\n \t * rename if either\n-\t * * we need to detect renames for an ancestor directory\n \t * * the file is modified/deleted on the other side of history\n+\t * * we need to detect renames for an ancestor directory\n \t * If neither of those are true, we can skip rename detection for\n-\t * that path.\n+\t * that path. The reason is stored as a value from enum\n+\t * file_rename_relevance, as the reason can inform the algorithm in\n+\t * diffcore_rename_extended().\n \t */\n \tstruct strintmap relevant_sources[3];\n \n@@ -677,8 +679,11 @@ static void add_pair(struct merge_options *opt,\n \t\tunsigned content_relevant = (match_mask == 0);\n \t\tunsigned location_relevant = (dir_rename_mask == 0x07);\n \n-\t\tif (content_relevant || location_relevant)\n-\t\t\tstrintmap_set(&renames->relevant_sources[side], pathname, 1);\n+\t\tif (content_relevant || location_relevant) {\n+\t\t\t/* content_relevant trumps location_relevant */\n+\t\t\tstrintmap_set(&renames->relevant_sources[side], pathname,\n+\t\t\t\t content_relevant ? RELEVANT_CONTENT : RELEVANT_LOCATION);\n+\t\t}\n \t}\n \n \tone = alloc_filespec(pathname);\n-- \ngitgitgadget\n\n"},{"id":"419163","messageId":"baca2700-0959-c4ca-6ba9-ceb5188f96f4@gmail.com","threadId":"55312","inReplyTo":"05850cb49823ea9c6d6c7cfd3b4adbbc1c47abc6.1615674128.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/8] merge-ort: record the reason that we want a rename for a directory","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-03-15T14:31:41Z","receivedAt":"2021-03-15T14:33:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/13/2021 5:22 PM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> When one side of history renames a directory, and the other side of\n> history added files to the old directory, directory rename detection is\n> used to warn about the location of the added files so the user can\n> move them to the old directory or keep them with the new one.\n> \n> This sets up three different types of directories:\n> * directories that had new files added to them\n> * directories underneath a directory that had new files added to them\n> * directories where no new files were added to it or any leading path\n> \n> Save this information in dirs_removed; the next several commits will\n> make use of this information.\n...\n> +/* dir_rename_relevance: the reason we want rename information for a dir */\n> +enum dir_rename_relevance {\n> +\tNOT_RELEVANT = 0,\n> +\tRELEVANT_FOR_ANCESTOR = 1,\n> +\tRELEVANT_FOR_SELF = 2\n> +};\n\nIs this potentially a flag list? It's hard to tell because we don't\nhave another item (3 or 4?).\n\n> \t\tunsigned sides = (0x07 - dirmask)/2;\n> +\t\tunsigned relevance = (renames->dir_rename_mask == 0x07) ?\n> +\t\t\t\t\tRELEVANT_FOR_ANCESTOR : NOT_RELEVANT;\n> +\t\t/*\n> +\t\t * Record relevance of this directory. However, note that\n> +\t\t * when collect_merge_info_callback() recurses into this\n> +\t\t * directory and calls collect_rename_info() on paths\n> +\t\t * within that directory, if we find a path that was added\n> +\t\t * to this directory on the other side of history, we will\n> +\t\t * upgrade this value to RELEVANT_FOR_SELF; see below.\n> +\t\t */\n\nThis comment seems to imply that RELEVANT_FOR_SELF is \"more important\"\nthan RELEVANT_FOR_ANCESTOR, so the value will just be changed (not a\nflag).\n\n> +\t/*\n> +\t * Here's the block that potentially upgrades to RELEVANT_FOR_SELF.\n> +\t * When we run across a file added to a directory. In such a case,\n> +\t * find the directory of the file and upgrade its relevance.\n> +\t */\n> +\tif (renames->dir_rename_mask == 0x07 &&\n> +\t (filemask == 2 || filemask == 4)) {\n> +\t\t/*\n> +\t\t * Need directory rename for parent directory on other side\n> +\t\t * of history from added file. Thus\n> +\t\t * side = (~filemask & 0x06) >> 1\n> +\t\t * or\n> +\t\t * side = 3 - (filemask/2).\n> +\t\t */\n> +\t\tunsigned side = 3 - (filemask >> 1);\n> +\t\tstrintmap_set(&renames->dirs_removed[side], dirname,\n> +\t\t\t RELEVANT_FOR_SELF);\n\nYes, using \"RELEVANT_FOR_SELF\" here, not \"relevance | RELEVANT_FOR_SELF\".\nOK. This should make the later consumers simpler.\n\nThanks,\n-Stolee\n"},{"id":"419164","messageId":"8422759a-a4a3-4dc6-4ae7-4a61896b9946@gmail.com","threadId":"55312","inReplyTo":"pull.853.git.1615674128.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/8] Optimization batch 10: avoid detecting even more irrelevant renames","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-03-15T15:21:10Z","receivedAt":"2021-03-15T15:22:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/13/2021 5:22 PM, Elijah Newren via GitGitGadget wrote:> === Results ===\n> \n> For the testcases mentioned in commit 557ac03 (\"merge-ort: begin performance\n> work; instrument with trace2_region_* calls\", 2020-10-28), the changes in\n> just this series improves the performance as follows:\n> \n> Before Series After Series\n> no-renames: 5.680 s ± 0.096 s 5.665 s ± 0.129 s \n> mega-renames: 13.812 s ± 0.162 s 11.435 s ± 0.158 s\n> just-one-mega: 506.0 ms ± 3.9 ms 494.2 ms ± 6.1 ms\n> \n> \n> While those results may look somewhat meager, it is important to note that\n> the previous optimizations have already reduced rename detection time to\n> nearly 0 for these particular testcases so there just isn't much left to\n> improve. The final patch in the series shows an alternate testcase where the\n> previous optimizations aren't as effective (a simple cherry-pick of a commit\n> that simply adds one new empty file), where there was a speedup factor of\n> approximately 3 due to this series:\n> \n> Before Series After Series\n> pick-empty: 1.936 s ± 0.024 s 688.1 ms ± 4.2 ms\n> \n> \n> There was also another testcase at $DAYJOB where I saw a factor 7\n> improvement from this particular optimization, so it certainly has the\n> potential to help when the previous optimizations are not quite enough.\n\nThese performance numbers continue to be impressive.\n\nI read through this series and found it clearly described. I can't\ncompletely vouch for its correctness, because there are a lot of\nmoving parts at this point. I'll trust the test cases and Elijah's\nadditional manual testing at this point.\n\nOutside of me talking out loud about an enum (which you can ignore)\nI didn't see anything unusual about the code.\n\nThanks,\n-Stolee\n"},{"id":"419165","messageId":"CABPp-BHLL_u_E-Ss=AYWUZM+_9hbC6D6WzRjxEUwmexTEgQ2zw@mail.gmail.com","threadId":"55312","inReplyTo":"baca2700-0959-c4ca-6ba9-ceb5188f96f4@gmail.com","subject":"Re: [PATCH 3/8] merge-ort: record the reason that we want a rename for a directory","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-03-15T15:27:48Z","receivedAt":"2021-03-15T15:29:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Mar 15, 2021 at 7:31 AM Derrick Stolee wrote:\n>\n> On 3/13/2021 5:22 PM, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > When one side of history renames a directory, and the other side of\n> > history added files to the old directory, directory rename detection is\n> > used to warn about the location of the added files so the user can\n> > move them to the old directory or keep them with the new one.\n> >\n> > This sets up three different types of directories:\n> > * directories that had new files added to them\n> > * directories underneath a directory that had new files added to them\n> > * directories where no new files were added to it or any leading path\n> >\n> > Save this information in dirs_removed; the next several commits will\n> > make use of this information.\n> ...\n> > +/* dir_rename_relevance: the reason we want rename information for a dir */\n> > +enum dir_rename_relevance {\n> > + NOT_RELEVANT = 0,\n> > + RELEVANT_FOR_ANCESTOR = 1,\n> > + RELEVANT_FOR_SELF = 2\n> > +};\n>\n> Is this potentially a flag list? It's hard to tell because we don't\n> have another item (3 or 4?).\n>\n> > unsigned sides = (0x07 - dirmask)/2;\n> > + unsigned relevance = (renames->dir_rename_mask == 0x07) ?\n> > + RELEVANT_FOR_ANCESTOR : NOT_RELEVANT;\n> > + /*\n> > + * Record relevance of this directory. However, note that\n> > + * when collect_merge_info_callback() recurses into this\n> > + * directory and calls collect_rename_info() on paths\n> > + * within that directory, if we find a path that was added\n> > + * to this directory on the other side of history, we will\n> > + * upgrade this value to RELEVANT_FOR_SELF; see below.\n> > + */\n>\n> This comment seems to imply that RELEVANT_FOR_SELF is \"more important\"\n> than RELEVANT_FOR_ANCESTOR, so the value will just be changed (not a\n> flag).\n\nYes.\n\n> > + /*\n> > + * Here's the block that potentially upgrades to RELEVANT_FOR_SELF.\n> > + * When we run across a file added to a directory. In such a case,\n> > + * find the directory of the file and upgrade its relevance.\n> > + */\n> > + if (renames->dir_rename_mask == 0x07 &&\n> > + (filemask == 2 || filemask == 4)) {\n> > + /*\n> > + * Need directory rename for parent directory on other side\n> > + * of history from added file. Thus\n> > + * side = (~filemask & 0x06) >> 1\n> > + * or\n> > + * side = 3 - (filemask/2).\n> > + */\n> > + unsigned side = 3 - (filemask >> 1);\n> > + strintmap_set(&renames->dirs_removed[side], dirname,\n> > + RELEVANT_FOR_SELF);\n>\n> Yes, using \"RELEVANT_FOR_SELF\" here, not \"relevance | RELEVANT_FOR_SELF\".\n> OK. This should make the later consumers simpler.\n\nYep, indeed. Would it make it clearer earlier if I were to stop\nassigning the explicit values in the enum? Would adding a comment\nwhen I introduce the enum be easier? Or was it just \"thinking out\nloud\"?\n"},{"id":"419167","messageId":"CABPp-BGBrmKhca9Lf7UF6AHkT7P5RD8uVpXQiqxhwMsQnRHeeg@mail.gmail.com","threadId":"55312","inReplyTo":"8422759a-a4a3-4dc6-4ae7-4a61896b9946@gmail.com","subject":"Re: [PATCH 0/8] Optimization batch 10: avoid detecting even more irrelevant renames","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-03-15T15:34:05Z","receivedAt":"2021-03-15T15:34:57Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Mar 15, 2021 at 8:21 AM Derrick Stolee wrote:\n>\n> On 3/13/2021 5:22 PM, Elijah Newren via GitGitGadget wrote:> === Results ===\n> >\n> > For the testcases mentioned in commit 557ac03 (\"merge-ort: begin performance\n> > work; instrument with trace2_region_* calls\", 2020-10-28), the changes in\n> > just this series improves the performance as follows:\n> >\n> > Before Series After Series\n> > no-renames: 5.680 s ± 0.096 s 5.665 s ± 0.129 s\n> > mega-renames: 13.812 s ± 0.162 s 11.435 s ± 0.158 s\n> > just-one-mega: 506.0 ms ± 3.9 ms 494.2 ms ± 6.1 ms\n> >\n> >\n> > While those results may look somewhat meager, it is important to note that\n> > the previous optimizations have already reduced rename detection time to\n> > nearly 0 for these particular testcases so there just isn't much left to\n> > improve. The final patch in the series shows an alternate testcase where the\n> > previous optimizations aren't as effective (a simple cherry-pick of a commit\n> > that simply adds one new empty file), where there was a speedup factor of\n> > approximately 3 due to this series:\n> >\n> > Before Series After Series\n> > pick-empty: 1.936 s ± 0.024 s 688.1 ms ± 4.2 ms\n> >\n> >\n> > There was also another testcase at $DAYJOB where I saw a factor 7\n> > improvement from this particular optimization, so it certainly has the\n> > potential to help when the previous optimizations are not quite enough.\n>\n> These performance numbers continue to be impressive.\n>\n> I read through this series and found it clearly described. I can't\n> completely vouch for its correctness, because there are a lot of\n> moving parts at this point. I'll trust the test cases and Elijah's\n> additional manual testing at this point.\n\nThanks for reading through it. I understand the comment; in my\nopinion this was the most complex of the nine \"Optimization batch\"\nseries to review (batch 8 was the second most complex, and batch 13\nwill be the third); I tried to simplify it as much as possible, but\nthere's just more stuff to simultaneously track in order to get this\none implemented. Thanks for reading through it.\n\n> Outside of me talking out loud about an enum (which you can ignore)\n> I didn't see anything unusual about the code.\n\nAh, that answers my other question then. :-)\n\nThanks!\n"},{"id":"420331","messageId":"xmqq4kgwox2u.fsf@gitster.g","threadId":"55312","inReplyTo":"CABPp-BHLL_u_E-Ss=AYWUZM+_9hbC6D6WzRjxEUwmexTEgQ2zw@mail.gmail.com","subject":"Re: [PATCH 3/8] merge-ort: record the reason that we want a rename for a directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-28T02:01:45Z","receivedAt":"2021-03-28T02:14:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> On Mon, Mar 15, 2021 at 7:31 AM Derrick Stolee wrote:\n>>\n>> On 3/13/2021 5:22 PM, Elijah Newren via GitGitGadget wrote:\n>> > From: Elijah Newren \n>> >\n>> > When one side of history renames a directory, and the other side of\n>> > history added files to the old directory, directory rename detection is\n>> > used to warn about the location of the added files so the user can\n>> > move them to the old directory or keep them with the new one.\n>> >\n>> > This sets up three different types of directories:\n>> > * directories that had new files added to them\n>> > * directories underneath a directory that had new files added to them\n>> > * directories where no new files were added to it or any leading path\n>> >\n>> > Save this information in dirs_removed; the next several commits will\n>> > make use of this information.\n>> ...\n>> > +/* dir_rename_relevance: the reason we want rename information for a dir */\n>> > +enum dir_rename_relevance {\n>> > + NOT_RELEVANT = 0,\n>> > + RELEVANT_FOR_ANCESTOR = 1,\n>> > + RELEVANT_FOR_SELF = 2\n>> > +};\n>>\n>> Is this potentially a flag list? It's hard to tell because we don't\n>> have another item (3 or 4?).\n>>\n>> > unsigned sides = (0x07 - dirmask)/2;\n>> > + unsigned relevance = (renames->dir_rename_mask == 0x07) ?\n>> > + RELEVANT_FOR_ANCESTOR : NOT_RELEVANT;\n>> > + /*\n>> > + * Record relevance of this directory. However, note that\n>> > + * when collect_merge_info_callback() recurses into this\n>> > + * directory and calls collect_rename_info() on paths\n>> > + * within that directory, if we find a path that was added\n>> > + * to this directory on the other side of history, we will\n>> > + * upgrade this value to RELEVANT_FOR_SELF; see below.\n>> > + */\n>>\n>> This comment seems to imply that RELEVANT_FOR_SELF is \"more important\"\n>> than RELEVANT_FOR_ANCESTOR, so the value will just be changed (not a\n>> flag).\n>\n> Yes.\n>\n>> > + /*\n>> > + * Here's the block that potentially upgrades to RELEVANT_FOR_SELF.\n>> > + * When we run across a file added to a directory. In such a case,\n>> > + * find the directory of the file and upgrade its relevance.\n>> > + */\n>> > + if (renames->dir_rename_mask == 0x07 &&\n>> > + (filemask == 2 || filemask == 4)) {\n>> > + /*\n>> > + * Need directory rename for parent directory on other side\n>> > + * of history from added file. Thus\n>> > + * side = (~filemask & 0x06) >> 1\n>> > + * or\n>> > + * side = 3 - (filemask/2).\n>> > + */\n>> > + unsigned side = 3 - (filemask >> 1);\n>> > + strintmap_set(&renames->dirs_removed[side], dirname,\n>> > + RELEVANT_FOR_SELF);\n>>\n>> Yes, using \"RELEVANT_FOR_SELF\" here, not \"relevance | RELEVANT_FOR_SELF\".\n>> OK. This should make the later consumers simpler.\n>\n> Yep, indeed. Would it make it clearer earlier if I were to stop\n> assigning the explicit values in the enum? Would adding a comment\n> when I introduce the enum be easier? Or was it just \"thinking out\n> loud\"?\n\nYou are not asking me, but if you were, I'd say not using enum for\nbitmask would be a good discipline to follow, and an enum like this\none that is used only for uniqueness of the values would benefit from\nnot having explicit value assignments.\n"}]}