From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 20:02:02
[This is a resend of
https://lore.kernel.org/git/20210104235006.2867404-1-newren@gmail.com/ from
Monday without any changes; I wanted to move it to gitgitgadget for testing
and such after I remembered I had perms to create temporary branches for the
pull request base.]
This series depends on a merge of en/merge-ort-3 and en/merge-ort-recursive.
It does not depend on the en/ort-conflict-handling.
This series mostly implements directory rename detection for merge-ort; I'll
cover the "mostly" bit below. If one merges this series with en/merge-tests
and en/ort-conflict-handling, then this series drops the number of failing
tests in the testsuite under GIT_TEST_MERGE_ALGORITHM=ort from 60 down to 8.
There's a lot of code here, but almost all of the logic is just copied over
from similarly named functions in merge-recursive.c, as repeatedly noted in
the commit messages. There are several minor differences spread throughout
that make it not be a direct copy:
* using strmap API instead of direct hashmap calls
* ort keeps track of all files and directories and their shas in
opt->priv->paths; no need to re-walk tree objects
* keeping the necessary invariants for opt->priv->paths
* we can pre-compute which directories are removed (stored in
dirs_removed), avoiding the need for some post-processing
* since ort already has struct rename_info, add the extra data there and
allocate/free it with the rest of the rename_info
* no non_unique_new_dir field, leading to the failure of test 2b; this will
be addressed in a different way with upcoming performance work.
These differences make a direct comparison difficult, but there's not really
any new or novel logic; the logic for how directory rename detection is
performed is identical to what is found in merge-recursive; it's just
packaged slightly differently.
...with one exception -- the final patch in the series modifies the logic
and makes it different than merge-recursive in order to fix a known bug
(testcase 12f of t6423).
There are still four failing tests in t6423 (directory rename tests) after
this series:
* one test (2b) where merge-ort erroneously prints a "directory rename
split" conflict message, despite the fact that there is no new file and
thus no need for a directory rename to be detected. This comes from the
lack of a non_unique_new_dir field that I didn't bother copying, since
performance work will address it in a completely different way.
* two tests (12b1 and 12c1) where merge-ort produces the same result at
merge-recursive (these tests are marked as test_expect_failure for
merge-recursive). Some performance work will fix these two tests.
* one test (12f) where merge-ort produces a better result than
merge-recursive.c (this test is marked as test_expect_failure for
merge-recursive), but where merge-ort does not yet manage to pass the
final four lines of the test related to performance checking.
Elijah Newren (18):
merge-ort: add new data structures for directory rename detection
merge-ort: initialize and free new directory rename data structures
merge-ort: collect which directories are removed in dirs_removed
merge-ort: add outline for computing directory renames
merge-ort: add outline of get_provisional_directory_renames()
merge-ort: copy get_renamed_dir_portion() from merge-recursive.c
merge-ort: implement compute_rename_counts()
merge-ort: implement handle_directory_level_conflicts()
merge-ort: modify collect_renames() for directory rename handling
merge-ort: implement compute_collisions()
merge-ort: implement apply_dir_rename() and check_dir_renamed()
merge-ort: implement check_for_directory_rename()
merge-ort: implement handle_path_level_conflicts()
merge-ort: add a new toplevel_dir field
merge-ort: implement apply_directory_rename_modifications()
merge-ort: process_renames() now needs more defensiveness
merge-ort: fix a directory rename detection bug
DO NOT SUBMIT: directory rename stuff for redo_after_renames
merge-ort.c | 876 ++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 857 insertions(+), 19 deletions(-)
base-commit: 9c85b62e817e83401855e4f2e11283be8386739e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-835%2Fnewren%2Fort-directory-renames-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-835/newren/ort-directory-renames-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/835
--
gitgitgadget
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 20:02:03
From: Elijah Newren <redacted>
This is nearly a wholesale copy of compute_collisions() from
merge-recursive.c, and the logic remains the same, but it has been
tweaked slightly due to:
* using strmap.h API (instead of direct hashmaps)
* allocation/freeing of data structures were done separately in
merge_start() and clear_or_reinit_internal_opts() in an earlier
patch in this series
* there is no non_unique_new_dir data field in merge-ort; that will
be handled a different way
It does depend on two new functions, apply_dir_rename() and
check_dir_renamed() which were introduced with simple
die-not-yet-implemented shells and will be implemented in subsequent
patches.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 67 insertions(+), 1 deletion(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 20:02:10
From: Elijah Newren <redacted>
This is copied from merge-recursive.c, with minor tweaks due to using strmap
API and the fact that it can use opt->priv->paths to get all pathnames that
exist instead of taking a tree object.
This depends on a new function, handle_path_level_conflicts(), which
just has a placeholder die-not-yet-implemented implementation for now; a
subsequent patch will implement it.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 66 insertions(+), 1 deletion(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 20:02:18
From: Elijah Newren <redacted>
This function is based on the first half of get_directory_renames() from
merge-recursive.c
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 51 insertions(+), 2 deletions(-)
@@ -721,7 +721,6 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/-MAYBE_UNUSEDstaticvoidget_renamed_dir_portion(constchar*old_path,constchar*new_path,char**old_dir,char**new_dir){
@@ -825,11 +824,61 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,*new_dir=xstrndup(new_path,end_of_new-new_path);}+staticvoidincrement_count(structstrmap*dir_rename_count,+char*old_dir,+char*new_dir)+{+structstrintmap*counts;+structstrmap_entry*e;++/* Get the {new_dirs -> counts} mapping using old_dir */+e=strmap_get_entry(dir_rename_count,old_dir);+if(e){+counts=e->value;+}else{+counts=xmalloc(sizeof(*counts));+strintmap_init_with_options(counts,0,NULL,1);+strmap_put(dir_rename_count,old_dir,counts);+}++/* Increment the count for new_dir */+strintmap_incr(counts,new_dir,1);+}+staticvoidcompute_rename_counts(structdiff_queue_struct*pairs,structstrmap*dir_rename_count,structstrset*dirs_removed){-die("Not yet implemented!");+inti;++for(i=0;i<pairs->nr;++i){+char*old_dir,*new_dir;+structdiff_filepair*pair=pairs->queue[i];++if(pair->status!='R')+continue;++/* Get the old and new directory names */+get_renamed_dir_portion(pair->one->path,pair->two->path,+&old_dir,&new_dir);+if(!old_dir)+/* Directory didn't change at all; ignore this one. */+continue;++/*+*Makedir_rename_countcontainamapofamap:+*old_directory->{new_directory->count}+*Inotherwords,foreverypairlookatthedirectoriesfor+*theoldfilenameandthenewfilenameandcounthowmany+*timesthatpairingoccurs.+*/+if(strset_contains(dirs_removed,old_dir))+increment_count(dir_rename_count,old_dir,new_dir);++/* Free resources we don't need anymore */+free(old_dir);+free(new_dir);+}}staticvoidget_provisional_directory_renames(structmerge_options*opt,
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 20:02:20
From: Elijah Newren <redacted>
Due to the string-equality-iff-pointer-equality requirements placed on
merged_info.directory_name, apply_directory_rename_modifications() will
need to have access to the exact toplevel directory name string pointer
and can't just use a new empty string. Store it in a field that we can
use.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 20:02:22
From: Elijah Newren <redacted>
This function is based on merge-recursive.c's get_directory_renames(),
except that the first half has been split out into a not-yet-implemented
compute_rename_counts(). The primary difference here is our lack of the
non_unique_new_dir boolean in our strmap. The lack of that field will
at first cause us to fail testcase 2b of t6423; however, future
optimizations will obviate the need for that ugly field so we have just
left it out.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 59 insertions(+), 1 deletion(-)
@@ -721,11 +721,69 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/+staticvoidcompute_rename_counts(structdiff_queue_struct*pairs,+structstrmap*dir_rename_count,+structstrset*dirs_removed)+{+die("Not yet implemented!");+}+staticvoidget_provisional_directory_renames(structmerge_options*opt,unsignedside,int*clean){-die("Not yet implemented!");+structhashmap_iteriter;+structstrmap_entry*entry;+structrename_info*renames=&opt->priv->renames;++compute_rename_counts(&renames->pairs[side],+&renames->dir_rename_count[side],+&renames->dirs_removed[side]);+/*+*Collapse+*dir_rename_count:old_directory->{new_directory->count}+*downto+*dir_renames:old_directory->best_new_directory+*wherebest_new_directoryistheonewiththeuniquehighestcount.+*/+strmap_for_each_entry(&renames->dir_rename_count[side],&iter,entry){+constchar*source_dir=entry->key;+structstrintmap*counts=entry->value;+structhashmap_itercount_iter;+structstrmap_entry*count_entry;+intmax=0;+intbad_max=0;+constchar*best=NULL;++strintmap_for_each_entry(counts,&count_iter,count_entry){+constchar*target_dir=count_entry->key;+intptr_tcount=(intptr_t)count_entry->value;++if(count==max)+bad_max=max;+elseif(count>max){+max=count;+best=target_dir;+}+}++if(max==0)+continue;++if(bad_max==max){+path_msg(opt,source_dir,0,+_("CONFLICT (directory rename split): "+"Unclear where to rename %s to; it was "+"renamed to multiple other directories, with "+"no destination getting a majority of the "+"files."),+source_dir);+*clean&=0;+}else{+strmap_put(&renames->dir_renames[side],+source_dir,(void*)best);+}+}}staticvoidhandle_directory_level_conflicts(structmerge_options*opt)
@@ -721,6 +721,110 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/+MAYBE_UNUSED+staticvoidget_renamed_dir_portion(constchar*old_path,constchar*new_path,+char**old_dir,char**new_dir)+{+char*end_of_old,*end_of_new;++/* Default return values: NULL, meaning no rename */+*old_dir=NULL;+*new_dir=NULL;++/*+*For+*"a/b/c/d/e/foo.c"->"a/b/some/thing/else/e/foo.c"+*the"e/foo.c"partisthesame,wejustwanttoknowthat+*"a/b/c/d"wasrenamedto"a/b/some/thing/else"+*so,forthisexample,thisfunctionreturns"a/b/c/d"in+**old_dirand"a/b/some/thing/else"in*new_dir.+*/++/*+*Ifthebasenameofthefilechanged,wedon'tcare.Wewant+*toknowwhichportionofthedirectory,ifany,changed.+*/+end_of_old=strrchr(old_path,'/');+end_of_new=strrchr(new_path,'/');++/*+*Ifend_of_oldisNULL,old_pathwasn'tinadirectory,sothere+*couldnotbeadirectoryrename(ourruleelsewherethata+*directorywhichstillexistsisnotconsideredtohavebeen+*renamedmeanstherootdirectorycanneverberenamed--because+*therootdirectoryalwaysexists).+*/+if(end_of_old==NULL)+return;/* Note: *old_dir and *new_dir are still NULL */++/*+*Ifnew_pathcontainsnodirectory(end_of_newisNULL),thenwe+*havearenameofold_path'sdirectorytotherootdirectory.+*/+if(end_of_new==NULL){+*old_dir=xstrndup(old_path,end_of_old-old_path);+*new_dir=xstrdup("");+return;+}++/* Find the first non-matching character traversing backwards */+while(*--end_of_new==*--end_of_old&&+end_of_old!=old_path&&+end_of_new!=new_path)+;/* Do nothing; all in the while loop */++/*+*Ifbothgotbacktothebeginningoftheirstrings,thenthe+*directorydidn'tchangeatall,onlythebasenamedid.+*/+if(end_of_old==old_path&&end_of_new==new_path&&+*end_of_old==*end_of_new)+return;/* Note: *old_dir and *new_dir are still NULL */++/*+*Ifend_of_newgotbacktothebeginningofitsstring,and+*end_of_oldgotbacktothebeginningofsomesubdirectory,then+*wehavearename/mergeofasubdirectoryintotheroot,which+*needsslightlyspecialhandling.+*+*Note:Thereisnoneedtoconsidertheoppositecase,witha+*rename/mergeoftherootdirectoryintosomesubdirectory+*becauseasnotedabovetherootdirectoryalwaysexistssoit+*cannotbeconsideredtoberenamed.+*/+if(end_of_new==new_path&&+end_of_old!=old_path&&end_of_old[-1]=='/'){+*old_dir=xstrndup(old_path,--end_of_old-old_path);+*new_dir=xstrdup("");+return;+}++/*+*We'vefoundthefirstnon-matchingcharacterinthedirectory+*paths.Thatmeansthecurrentcharacterswewerelookingat+*werepartofthefirstnon-matchingsubdirnamegoingbackfrom+*theendofthestrings.Getthewholenamebyadvancingboth+*end_of_oldandend_of_newtotheNEXT'/'character.Thatwill+*representtheentiredirectoryrename.+*+*Thereasonfortheincrementiscaseslike+*a/b/star/foo/whatever.c->a/b/tar/foo/random.c+*Afterdroppingthebasenameandgoingbacktothefirst+*non-matchingcharacter,we'renowcomparing:+*a/b/sanda/b/+*andwewanttobecomparing:+*a/b/star/anda/b/tar/+*butwithoutthepre-increment,theoneontherightwouldstay+*a/b/.+*/+end_of_old=strchr(++end_of_old,'/');+end_of_new=strchr(++end_of_new,'/');++/* Copy the old and new directories into *old_dir and *new_dir. */+*old_dir=xstrndup(old_path,end_of_old-old_path);+*new_dir=xstrndup(new_path,end_of_new-new_path);+}+staticvoidcompute_rename_counts(structdiff_queue_struct*pairs,structstrmap*dir_rename_count,structstrset*dirs_removed)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 20:02:24
From: Elijah Newren <redacted>
This is modelled on the version of handle_directory_level_conflicts()
from merge-recursive.c, but is massively simplified due to the following
factors:
* strmap API provides simplifications over using direct hashamp
* we have a dirs_removed field in struct rename_info that we have an
easy way to populate from collect_merge_info(); this was already
used in compute_rename_counts() and thus we do not need to check
for condition #2.
* The removal of condition #2 by handling it earlier in the code also
obviates the need to check for condition #3 -- if both sides renamed
a directory, meaning that the directory no longer exists on either
side, then neither side could have added any new files to that
directory, and thus there are no files whose locations we need to
move due to such a directory rename.
In fact, the same logic that makes condition #3 irrelevant means
condition #1 is also irrelevant so we could drop this function.
However, it is cheap to check if both sides rename the same directory,
and doing so can save future computation. So, simply remove any
directories that both sides renamed from the list of directory renames.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 20:02:25
From: Elijah Newren <redacted>
collect_renames() is similar to merge-recursive.c's get_renames(), but
lacks the directory rename handling found in the latter. Port that code
structure over to merge-ort. This introduces three new
die-not-yet-implemented functions that will be defined in future
commits.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 74 insertions(+), 4 deletions(-)
@@ -721,6 +721,11 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/+structcollision_info{+structstring_listsource_files;+unsignedreported_already:1;+};+staticvoidget_renamed_dir_portion(constchar*old_path,constchar*new_path,char**old_dir,char**new_dir){
@@ -960,6 +965,31 @@ static void handle_directory_level_conflicts(struct merge_options *opt)string_list_clear(&duplicated,0);}+staticvoidcompute_collisions(structstrmap*collisions,+structstrmap*dir_renames,+structdiff_queue_struct*pairs)+{+die("Not yet implemented.");+}++staticchar*check_for_directory_rename(structmerge_options*opt,+constchar*path,+unsignedside_index,+structstrmap*dir_renames,+structstrmap*dir_rename_exclusions,+structstrmap*collisions,+int*clean_merge)+{+die("Not yet implemented.");+}++staticvoidapply_directory_rename_modifications(structmerge_options*opt,+structdiff_filepair*pair,+char*new_path)+{+die("Not yet implemented.");+}+/*** Function Grouping: functions related to regular rename detection ***/staticintprocess_renames(structmerge_options*opt,
@@ -1285,22 +1315,44 @@ static void detect_regular_renames(struct merge_options *opt,*/staticintcollect_renames(structmerge_options*opt,structdiff_queue_struct*result,-unsignedside_index)+unsignedside_index,+structstrmap*dir_renames_for_side,+structstrmap*rename_exclusions){inti,clean=1;+structstrmapcollisions;structdiff_queue_struct*side_pairs;+structhashmap_iteriter;+structstrmap_entry*entry;structrename_info*renames=&opt->priv->renames;side_pairs=&renames->pairs[side_index];+compute_collisions(&collisions,dir_renames_for_side,side_pairs);for(i=0;i<side_pairs->nr;++i){structdiff_filepair*p=side_pairs->queue[i];+char*new_path;/* non-NULL only with directory renames */-if(p->status!='R'){+if(p->status!='A'&&p->status!='R'){diff_free_filepair(p);continue;}+new_path=check_for_directory_rename(opt,p->two->path,+side_index,+dir_renames_for_side,+rename_exclusions,+&collisions,+&clean);++if(p->status!='R'&&!new_path){+diff_free_filepair(p);+continue;+}++if(new_path)+apply_directory_rename_modifications(opt,p,new_path);+/**p->scorecomesbackfromdiffcore_rename_extended()with*thesimilarityoftherenamedfile.Thesimilarityis
@@ -1315,6 +1367,20 @@ static int collect_renames(struct merge_options *opt,result->queue[result->nr++]=p;}+/* Free each value in the collisions map */+strmap_for_each_entry(&collisions,&iter,entry){+structcollision_info*info=entry->value;+string_list_clear(&info->source_files,0);+}+/*+*Incompute_collisions(),wesetcollisions.strdup_stringsto0+*sothatwewouldn'thavetomakeanothercopyofthenew_path+*allocatedbyapply_dir_rename().Butnowthatwe'veusedthem+*andhavenootherreferencestothesestrings,itistimeto+*deallocatethem.+*/+free_strmap_strings(&collisions);+strmap_clear(&collisions,1);returnclean;}
@@ -1346,8 +1412,12 @@ static int detect_and_process_renames(struct merge_options *opt,ALLOC_GROW(combined.queue,renames->pairs[1].nr+renames->pairs[2].nr,combined.alloc);-clean&=collect_renames(opt,&combined,MERGE_SIDE1);-clean&=collect_renames(opt,&combined,MERGE_SIDE2);+clean&=collect_renames(opt,&combined,MERGE_SIDE1,+&renames->dir_renames[2],+&renames->dir_renames[1]);+clean&=collect_renames(opt,&combined,MERGE_SIDE2,+&renames->dir_renames[1],+&renames->dir_renames[2]);QSORT(combined.queue,combined.nr,compare_pairs);clean&=process_renames(opt,&combined);
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 20:02:26
From: Elijah Newren <redacted>
Port some directory rename handling changes from merge-recursive.c's
detect_and_process_renames() to the same-named function of merge-ort.c.
This does not yet add any use or handling of directory renames, just the
outline for where we start to compute them. Thus, a future patch will
add port additional changes to merge-ort's detect_and_process_renames().
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
@@ -721,6 +721,18 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/+staticvoidget_provisional_directory_renames(structmerge_options*opt,+unsignedside,+int*clean)+{+die("Not yet implemented!");+}++staticvoidhandle_directory_level_conflicts(structmerge_options*opt)+{+die("Not yet implemented!");+}+/*** Function Grouping: functions related to regular rename detection ***/staticintprocess_renames(structmerge_options*opt,
@@ -1086,13 +1098,24 @@ static int detect_and_process_renames(struct merge_options *opt,{structdiff_queue_structcombined;structrename_info*renames=&opt->priv->renames;-ints,clean=1;+intneed_dir_renames,s,clean=1;memset(&combined,0,sizeof(combined));detect_regular_renames(opt,merge_base,side1,MERGE_SIDE1);detect_regular_renames(opt,merge_base,side2,MERGE_SIDE2);+need_dir_renames=+!opt->priv->call_depth&&+(opt->detect_directory_renames==MERGE_DIRECTORY_RENAMES_TRUE||+opt->detect_directory_renames==MERGE_DIRECTORY_RENAMES_CONFLICT);++if(need_dir_renames){+for(s=MERGE_SIDE1;s<=MERGE_SIDE2;s++)+get_provisional_directory_renames(opt,s,&clean);+handle_directory_level_conflicts(opt);+}+ALLOC_GROW(combined.queue,renames->pairs[1].nr+renames->pairs[2].nr,combined.alloc);
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 20:02:27
From: Elijah Newren <redacted>
Since directory rename detection adds new paths to opt->priv->paths and
removes old ones, process_renames() needs to now check whether
pair->one->path actually exists in opt->priv->paths instead of just
assuming it does.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 26 +++++++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 20:02:27
From: Elijah Newren <redacted>
Both of these are copied from merge-recursive.c, with just minor tweaks
due to using strmap API and not having a non_unique_new_dir field.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 37 +++++++++++++++++++++++++++++++++++--
1 file changed, 35 insertions(+), 2 deletions(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 20:02:30
From: Elijah Newren <redacted>
I believe the
ci->dirmask & sidemask
stuff was leftovers from before I added dirs_removed. I think it's
dead, useless code; just need to check.
Also, the
!mi || mi->clean
portions should only be triggerable once I do the redo_after_renames
stuff.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 20:02:36
From: Elijah Newren <redacted>
This is copied from merge-recursive.c, with minor tweaks due to:
* using strmap API
* merge-ort not using the non_unique_new_dir field, since it'll
obviate its need entirely later with performance improvements
* adding a new path_in_way() function that uses opt->priv->paths
instead of doing an expensive tree_has_path() lookup to see if
a tree has a given path.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 71 insertions(+), 1 deletion(-)
@@ -876,7 +886,67 @@ static char *handle_path_level_conflicts(struct merge_options *opt,structstrmap_entry*rename_info,structstrmap*collisions){-die("Not yet implemented");+char*new_path=NULL;+structcollision_info*c_info;+intclean=1;+structstrbufcollision_paths=STRBUF_INIT;++/*+*entryhasthemappingofolddirectorynametonewdirectoryname+*thatwewanttoapplytopath.+*/+new_path=apply_dir_rename(rename_info,path);+if(!new_path)+BUG("Failed to apply directory rename!");++/*+*Thecallerneedstohaveensuredthatithaspre-populated+*collisionswithallpathsthatmaptonew_path.Doaquickcheck+*toensurethat'sthecase.+*/+c_info=strmap_get(collisions,new_path);+if(c_info==NULL)+BUG("c_info is NULL");++/*+*Checkforone-sidedadd/add/.../addconflicts,i.e.+*whereimplicitrenamesfromtheothersidedoing+*directoryrename(s)canaffectthissideofhistory+*toputmultiplepathsintothesamelocation.Warn+*andbailondirectoryrenamesforsuchpaths.+*/+if(c_info->reported_already){+clean=0;+}elseif(path_in_way(&opt->priv->paths,new_path,1<<side_index)){+c_info->reported_already=1;+strbuf_add_separated_string_list(&collision_paths,", ",+&c_info->source_files);+path_msg(opt,new_path,0,+_("CONFLICT (implicit dir rename): Existing file/dir "+"at %s in the way of implicit directory rename(s) "+"putting the following path(s) there: %s."),+new_path,collision_paths.buf);+clean=0;+}elseif(c_info->source_files.nr>1){+c_info->reported_already=1;+strbuf_add_separated_string_list(&collision_paths,", ",+&c_info->source_files);+path_msg(opt,new_path,0,+_("CONFLICT (implicit dir rename): Cannot map more "+"than one path to %s; implicit directory renames "+"tried to put these paths there: %s"),+new_path,collision_paths.buf);+clean=0;+}++/* Free memory we no longer need */+strbuf_release(&collision_paths);+if(!clean&&new_path){+free(new_path);+returnNULL;+}++returnnew_path;}staticvoidincrement_count(structstrmap*dir_rename_count,
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 20:02:58
From: Elijah Newren <redacted>
This function roughly follows the same outline as the function of the
same name from merge-recursive.c, but the code diverges in multiple
ways due to some special considerations:
* merge-ort's version needs to update opt->priv->paths with any new
paths (and opt->priv->paths points to struct conflict_infos which
track quite a bit of metadata for each path); merge-recursive's
version would directly update the index
* merge-ort requires that opt->priv->paths has any leading directories
of any relevant files also be included in the set of paths. And
due to pointer equality requirements on merged_info.directory_name,
we have to be careful how we compute and insert these.
* due to the above requirements on opt->priv->paths, merge-ort's
version starts with a long comment to explain all the special
considerations that need to be handled
* merge-ort can use the full data stored in opt->priv->paths to avoid
making expensive get_tree_entry() calls to regather the necessary
data.
* due to messages being deferred automatically in merge-ort, this is
the best place to handle conflict messages whereas in
merge-recursive.c they are deferred manually so that processing of
entries does all the printing
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 168 +++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 167 insertions(+), 1 deletion(-)
@@ -1224,7 +1224,173 @@ static void apply_directory_rename_modifications(struct merge_options *opt,structdiff_filepair*pair,char*new_path){-die("Not yet implemented.");+/*+*Thebasicideaistogettheconflict_infofromopt->priv->paths+*atoldpath,andinsertitintonew_path;basicallyjustthis:+*ci=strmap_get(&opt->priv->paths,old_path);+*strmap_remove(&opt->priv->paths,old_path,0);+*strmap_put(&opt->priv->paths,new_path,ci);+*However,therearesomefactorscomplicatingthis:+*-opt->priv->pathsmayalreadyhaveanentryatnew_path+*-Eachcitracksitscontainingdirectory,soweneedto+*updatethat+*-Ifanothercihasthesamecontainingdirectory,then+*thetwochar*'sMUSTpointtothesamelocation.Seethe+*commentinstructmerged_info.strcmpequalityisnot+*enough;weneedpointerequality.+*-opt->priv->pathsmustholdtheparentdirectoriesofany+*entriesthatareadded.So,ifthisdirectoryrename+*causesentirelynewdirectories,wemustrecursivelyadd+*parentdirectories.+*-Foreachparentdirectoryaddedtoopt->priv->paths,we+*alsoneedtogetitsparentdirectorystoredinits+*conflict_info->merged.directory_namewithallthesame+*requirementsaboutpointerequality.+*/+structstring_listdirs_to_insert=STRING_LIST_INIT_NODUP;+structconflict_info*ci,*new_ci;+structstrmap_entry*entry;+constchar*branch_with_new_path,*branch_with_dir_rename;+constchar*old_path=pair->two->path;+constchar*parent_name;+constchar*cur_path;+inti,len;++entry=strmap_get_entry(&opt->priv->paths,old_path);+old_path=entry->key;+ci=entry->value;+VERIFY_CI(ci);++/* Find parent directories missing from opt->priv->paths */+cur_path=new_path;+while(1){+/* Find the parent directory of cur_path */+char*last_slash=strrchr(cur_path,'/');+if(last_slash){+parent_name=xstrndup(cur_path,last_slash-cur_path);+}else{+parent_name=opt->priv->toplevel_dir;+break;+}++/* Look it up in opt->priv->paths */+entry=strmap_get_entry(&opt->priv->paths,parent_name);+if(entry){+free((char*)parent_name);+parent_name=entry->key;/* reuse known pointer */+break;+}++/* Record this is one of the directories we need to insert */+string_list_append(&dirs_to_insert,parent_name);+cur_path=parent_name;+}++/* Traverse dirs_to_insert and insert them into opt->priv->paths */+for(i=dirs_to_insert.nr-1;i>=0;--i){+structconflict_info*dir_ci;+char*cur_dir=dirs_to_insert.items[i].string;++dir_ci=xcalloc(1,sizeof(*dir_ci));++dir_ci->merged.directory_name=parent_name;+len=strlen(parent_name);+/* len+1 because of trailing '/' character */+dir_ci->merged.basename_offset=(len>0?len+1:len);+dir_ci->dirmask=ci->filemask;+strmap_put(&opt->priv->paths,cur_dir,dir_ci);++parent_name=cur_dir;+}++/*+*Weareremovingold_pathfromopt->priv->paths.old_pathalsowill+*eventuallyneedtobefreed,butitmaystillbeusedbye.g.+*ci->pathnames.So,storeitinanotherstring-listfornow.+*/+string_list_append(&opt->priv->paths_to_free,old_path);++assert(ci->filemask==2||ci->filemask==4);+assert(ci->dirmask==0);+strmap_remove(&opt->priv->paths,old_path,0);++branch_with_new_path=(ci->filemask==2)?opt->branch1:opt->branch2;+branch_with_dir_rename=(ci->filemask==2)?opt->branch2:opt->branch1;++/* Now, finally update ci and stick it into opt->priv->paths */+ci->merged.directory_name=parent_name;+len=strlen(parent_name);+ci->merged.basename_offset=(len>0?len+1:len);+new_ci=strmap_get(&opt->priv->paths,new_path);+if(!new_ci){+/* Place ci back into opt->priv->paths, but at new_path */+strmap_put(&opt->priv->paths,new_path,ci);+}else{+intindex;++/* A few sanity checks */+VERIFY_CI(new_ci);+assert(ci->filemask==2||ci->filemask==4);+assert((new_ci->filemask&ci->filemask)==0);+assert(!new_ci->merged.clean);++/* Copy stuff from ci into new_ci */+new_ci->filemask|=ci->filemask;+if(new_ci->dirmask)+new_ci->df_conflict=1;+index=(ci->filemask>>1);+new_ci->pathnames[index]=ci->pathnames[index];+new_ci->stages[index].mode=ci->stages[index].mode;+oidcpy(&new_ci->stages[index].oid,&ci->stages[index].oid);++free(ci);+ci=new_ci;+}++if(opt->detect_directory_renames==MERGE_DIRECTORY_RENAMES_TRUE){+/* Notify user of updated path */+if(pair->status=='A')+path_msg(opt,new_path,1,+_("Path updated: %s added in %s inside a "+"directory that was renamed in %s; moving "+"it to %s."),+old_path,branch_with_new_path,+branch_with_dir_rename,new_path);+else+path_msg(opt,new_path,1,+_("Path updated: %s renamed to %s in %s, "+"inside a directory that was renamed in %s; "+"moving it to %s."),+pair->one->path,old_path,branch_with_new_path,+branch_with_dir_rename,new_path);+}else{+/*+*opt->detect_directory_renameshasthevalue+*MERGE_DIRECTORY_RENAMES_CONFLICT,somarktheseasconflicts.+*/+ci->path_conflict=1;+if(pair->status=='A')+path_msg(opt,new_path,0,+_("CONFLICT (file location): %s added in %s "+"inside a directory that was renamed in %s, "+"suggesting it should perhaps be moved to "+"%s."),+old_path,branch_with_new_path,+branch_with_dir_rename,new_path);+else+path_msg(opt,new_path,0,+_("CONFLICT (file location): %s renamed to %s "+"in %s, inside a directory that was renamed "+"in %s, suggesting it should perhaps be "+"moved to %s."),+pair->one->path,old_path,branch_with_new_path,+branch_with_dir_rename,new_path);+}++/*+*Finally,recordthenewlocation.+*/+pair->two->path=new_path;}/*** Function Grouping: functions related to regular rename detection ***/
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 20:02:58
From: Elijah Newren <redacted>
As noted in commit 902c521a35 ("t6423: more involved directory rename
test", 2020-10-15), when we have a case where
* dir/subdir/ has several files
* almost all files in dir/subdir/ are renamed to folder/subdir/
* one of the files in dir/subdir/ is renamed to folder/subdir/newsubdir/
* the other side of history (that doesn't do the renames) adds a
new file to dir/subdir/
Then for the majority of the file renames, the directory rename of
dir/subdir/ -> folder/subdir/
is actually not represented that way but as
dir/ -> folder/
We also had one rename that was represented as
dir/subdir/ -> folder/subdir/newsubdir/
Now, since there's a new file in dir/subdir/, where does it go? Well,
there's only one rule for dir/subdir/, so the code previously noted that
this rule had the "majority" of the one "relevant" rename and thus
erroneously used it to place the file in folder/subdir/newsubdir/. We
really want the heavy weight associated with dir/ -> folder/ to also be
treated as dir/subdir/ -> folder/subdir/, so that we correctly place the
file in folder/subdir/.
Add a bunch of logic to make sure that we use all relevant renamings in
directory rename detection.
Note that testcase 12f of t6423 still fails after this, but it gets
further than merge-recursive does. There are some performance related
bits in that testcase (the region_enter messages) that do not yet
succeed, but the rest of the testcase works after this patch.
Subsequent patch series will fix up the performance side.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 198 +++++++++++++++++++++-------------------------------
1 file changed, 81 insertions(+), 117 deletions(-)
@@ -764,109 +764,6 @@ static char *apply_dir_rename(struct strmap_entry *rename_info,returnstrbuf_detach(&new_path,NULL);}-staticvoidget_renamed_dir_portion(constchar*old_path,constchar*new_path,-char**old_dir,char**new_dir)-{-char*end_of_old,*end_of_new;--/* Default return values: NULL, meaning no rename */-*old_dir=NULL;-*new_dir=NULL;--/*-*For-*"a/b/c/d/e/foo.c"->"a/b/some/thing/else/e/foo.c"-*the"e/foo.c"partisthesame,wejustwanttoknowthat-*"a/b/c/d"wasrenamedto"a/b/some/thing/else"-*so,forthisexample,thisfunctionreturns"a/b/c/d"in-**old_dirand"a/b/some/thing/else"in*new_dir.-*/--/*-*Ifthebasenameofthefilechanged,wedon'tcare.Wewant-*toknowwhichportionofthedirectory,ifany,changed.-*/-end_of_old=strrchr(old_path,'/');-end_of_new=strrchr(new_path,'/');--/*-*Ifend_of_oldisNULL,old_pathwasn'tinadirectory,sothere-*couldnotbeadirectoryrename(ourruleelsewherethata-*directorywhichstillexistsisnotconsideredtohavebeen-*renamedmeanstherootdirectorycanneverberenamed--because-*therootdirectoryalwaysexists).-*/-if(end_of_old==NULL)-return;/* Note: *old_dir and *new_dir are still NULL */--/*-*Ifnew_pathcontainsnodirectory(end_of_newisNULL),thenwe-*havearenameofold_path'sdirectorytotherootdirectory.-*/-if(end_of_new==NULL){-*old_dir=xstrndup(old_path,end_of_old-old_path);-*new_dir=xstrdup("");-return;-}--/* Find the first non-matching character traversing backwards */-while(*--end_of_new==*--end_of_old&&-end_of_old!=old_path&&-end_of_new!=new_path)-;/* Do nothing; all in the while loop */--/*-*Ifbothgotbacktothebeginningoftheirstrings,thenthe-*directorydidn'tchangeatall,onlythebasenamedid.-*/-if(end_of_old==old_path&&end_of_new==new_path&&-*end_of_old==*end_of_new)-return;/* Note: *old_dir and *new_dir are still NULL */--/*-*Ifend_of_newgotbacktothebeginningofitsstring,and-*end_of_oldgotbacktothebeginningofsomesubdirectory,then-*wehavearename/mergeofasubdirectoryintotheroot,which-*needsslightlyspecialhandling.-*-*Note:Thereisnoneedtoconsidertheoppositecase,witha-*rename/mergeoftherootdirectoryintosomesubdirectory-*becauseasnotedabovetherootdirectoryalwaysexistssoit-*cannotbeconsideredtoberenamed.-*/-if(end_of_new==new_path&&-end_of_old!=old_path&&end_of_old[-1]=='/'){-*old_dir=xstrndup(old_path,--end_of_old-old_path);-*new_dir=xstrdup("");-return;-}--/*-*We'vefoundthefirstnon-matchingcharacterinthedirectory-*paths.Thatmeansthecurrentcharacterswewerelookingat-*werepartofthefirstnon-matchingsubdirnamegoingbackfrom-*theendofthestrings.Getthewholenamebyadvancingboth-*end_of_oldandend_of_newtotheNEXT'/'character.Thatwill-*representtheentiredirectoryrename.-*-*Thereasonfortheincrementiscaseslike-*a/b/star/foo/whatever.c->a/b/tar/foo/random.c-*Afterdroppingthebasenameandgoingbacktothefirst-*non-matchingcharacter,we'renowcomparing:-*a/b/sanda/b/-*andwewanttobecomparing:-*a/b/star/anda/b/tar/-*butwithoutthepre-increment,theoneontherightwouldstay-*a/b/.-*/-end_of_old=strchr(++end_of_old,'/');-end_of_new=strchr(++end_of_new,'/');--/* Copy the old and new directories into *old_dir and *new_dir. */-*old_dir=xstrndup(old_path,end_of_old-old_path);-*new_dir=xstrndup(new_path,end_of_new-new_path);-}-staticintpath_in_way(structstrmap*paths,constchar*path,unsignedside_mask){structmerged_info*mi=strmap_get(paths,path);
@@ -973,6 +878,76 @@ static void increment_count(struct strmap *dir_rename_count,strintmap_incr(counts,new_dir,1);}+staticvoidupdate_dir_rename_counts(structstrmap*dir_rename_count,+structstrset*dirs_removed,+constchar*oldname,+constchar*newname)+{+char*old_dir=xstrdup(oldname);+char*new_dir=xstrdup(newname);+charnew_dir_first_char=new_dir[0];+intfirst_time_in_loop=1;++while(1){+dirname_munge(old_dir);+dirname_munge(new_dir);++/*+*Whenrenaming+*"a/b/c/d/e/foo.c"->"a/b/some/thing/else/e/foo.c"+*thenthissuggeststhatboth+*a/b/c/d/e/=>a/b/some/thing/else/e/+*a/b/c/d/=>a/b/some/thing/else/+*sowewanttoincrementcountersforboth.WedoNOT,+*however,alsowanttosuggestthattherewasthefollowing+*rename:+*a/b/c/=>a/b/some/thing/+*soweneedtoquitatthatpoint.+*+*Notethewhenfirst_time_in_loop,weonlystripoffthe+*basename,andwedon'tcareifthat'sdifferent.+*/+if(!first_time_in_loop){+char*old_sub_dir=strchr(old_dir,'\0')+1;+char*new_sub_dir=strchr(new_dir,'\0')+1;+if(!*new_dir){+/*+*Specialcasewhenrenamingtorootdirectory,+*i.e.whennew_dir=="".Inthiscase,wehad+*somethinglike+*a/b/subdir=>subdir+*andsodirname_munge()setsthingsupsothat+*old_dir="a/b\0subdir\0"+*new_dir="\0ubdir\0"+*Wedidn'thavea'/'tooverwritea'\0'onto+*innew_dir,sowehavetocomparedifferently.+*/+if(new_dir_first_char!=old_sub_dir[0]||+strcmp(old_sub_dir+1,new_sub_dir))+break;+}else{+if(strcmp(old_sub_dir,new_sub_dir))+break;+}+}++if(strset_contains(dirs_removed,old_dir))+increment_count(dir_rename_count,old_dir,new_dir);+else+break;++/* If we hit toplevel directory ("") for old or new dir, quit */+if(!*old_dir||!*new_dir)+break;++first_time_in_loop=0;+}++/* Free resources we don't need anymore */+free(old_dir);+free(new_dir);+}+staticvoidcompute_rename_counts(structdiff_queue_struct*pairs,structstrmap*dir_rename_count,structstrset*dirs_removed)
@@ -980,19 +955,11 @@ static void compute_rename_counts(struct diff_queue_struct *pairs,inti;for(i=0;i<pairs->nr;++i){-char*old_dir,*new_dir;structdiff_filepair*pair=pairs->queue[i];if(pair->status!='R')continue;-/* Get the old and new directory names */-get_renamed_dir_portion(pair->one->path,pair->two->path,-&old_dir,&new_dir);-if(!old_dir)-/* Directory didn't change at all; ignore this one. */-continue;-/**Makedir_rename_countcontainamapofamap:*old_directory->{new_directory->count}
@@ -1000,12 +967,9 @@ static void compute_rename_counts(struct diff_queue_struct *pairs,*theoldfilenameandthenewfilenameandcounthowmany*timesthatpairingoccurs.*/-if(strset_contains(dirs_removed,old_dir))-increment_count(dir_rename_count,old_dir,new_dir);--/* Free resources we don't need anymore */-free(old_dir);-free(new_dir);+update_dir_rename_counts(dir_rename_count,dirs_removed,+pair->one->path,+pair->two->path);}}
Whoops. Ignore this patch, of course.
On Thu, Jan 7, 2021 at 12:01 PM Elijah Newren via GitGitGadget
[off-list ref] wrote:
quoted hunk
From: Elijah Newren <redacted>
I believe the
ci->dirmask & sidemask
stuff was leftovers from before I added dirs_removed. I think it's
dead, useless code; just need to check.
Also, the
!mi || mi->clean
portions should only be triggerable once I do the redo_after_renames
stuff.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 21:36:50
[Sorry for the deluge of email. Only change since v1 is dropping the "DO NOT
SUBMIT" patch at the end that was meant to be part of later optimization
work. Now this series actually matches what I sent on Monday at
https://lore.kernel.org/git/20210104235006.2867404-1-newren@gmail.com/, as I
meant to do with v1.]
This series depends on a merge of en/merge-ort-3 and en/merge-ort-recursive.
It does not depend on the en/ort-conflict-handling.
This series mostly implements directory rename detection for merge-ort; I'll
cover the "mostly" bit below. If one merges this series with en/merge-tests
and en/ort-conflict-handling, then this series drops the number of failing
tests in the testsuite under GIT_TEST_MERGE_ALGORITHM=ort from 60 down to 8.
There's a lot of code here, but almost all of the logic is just copied over
from similarly named functions in merge-recursive.c, as repeatedly noted in
the commit messages. There are several minor differences spread throughout
that make it not be a direct copy:
* using strmap API instead of direct hashmap calls
* ort keeps track of all files and directories and their shas in
opt->priv->paths; no need to re-walk tree objects
* keeping the necessary invariants for opt->priv->paths
* we can pre-compute which directories are removed (stored in
dirs_removed), avoiding the need for some post-processing
* since ort already has struct rename_info, add the extra data there and
allocate/free it with the rest of the rename_info
* no non_unique_new_dir field, leading to the failure of test 2b; this will
be addressed in a different way with upcoming performance work.
These differences make a direct comparison difficult, but there's not really
any new or novel logic; the logic for how directory rename detection is
performed is identical to what is found in merge-recursive; it's just
packaged slightly differently.
...with one exception -- the final patch in the series modifies the logic
and makes it different than merge-recursive in order to fix a known bug
(testcase 12f of t6423).
There are still four failing tests in t6423 (directory rename tests) after
this series:
* one test (2b) where merge-ort erroneously prints a "directory rename
split" conflict message, despite the fact that there is no new file and
thus no need for a directory rename to be detected. This comes from the
lack of a non_unique_new_dir field that I didn't bother copying, since
performance work will address it in a completely different way.
* two tests (12b1 and 12c1) where merge-ort produces the same result at
merge-recursive (these tests are marked as test_expect_failure for
merge-recursive). Some performance work will fix these two tests.
* one test (12f) where merge-ort produces a better result than
merge-recursive.c (this test is marked as test_expect_failure for
merge-recursive), but where merge-ort does not yet manage to pass the
final four lines of the test related to performance checking.
Elijah Newren (17):
merge-ort: add new data structures for directory rename detection
merge-ort: initialize and free new directory rename data structures
merge-ort: collect which directories are removed in dirs_removed
merge-ort: add outline for computing directory renames
merge-ort: add outline of get_provisional_directory_renames()
merge-ort: copy get_renamed_dir_portion() from merge-recursive.c
merge-ort: implement compute_rename_counts()
merge-ort: implement handle_directory_level_conflicts()
merge-ort: modify collect_renames() for directory rename handling
merge-ort: implement compute_collisions()
merge-ort: implement apply_dir_rename() and check_dir_renamed()
merge-ort: implement check_for_directory_rename()
merge-ort: implement handle_path_level_conflicts()
merge-ort: add a new toplevel_dir field
merge-ort: implement apply_directory_rename_modifications()
merge-ort: process_renames() now needs more defensiveness
merge-ort: fix a directory rename detection bug
merge-ort.c | 831 ++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 812 insertions(+), 19 deletions(-)
base-commit: 9c85b62e817e83401855e4f2e11283be8386739e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-835%2Fnewren%2Fort-directory-renames-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-835/newren/ort-directory-renames-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/835
Range-diff vs v1:
1: 41a99640cc5 = 1: 41a99640cc5 merge-ort: add new data structures for directory rename detection
2: 762151802be = 2: 762151802be merge-ort: initialize and free new directory rename data structures
3: bb4b6d20480 = 3: bb4b6d20480 merge-ort: collect which directories are removed in dirs_removed
4: ccb30dfc3c4 = 4: ccb30dfc3c4 merge-ort: add outline for computing directory renames
5: bb4285250cd = 5: bb4285250cd merge-ort: add outline of get_provisional_directory_renames()
6: 4e79a96ba1c = 6: 4e79a96ba1c merge-ort: copy get_renamed_dir_portion() from merge-recursive.c
7: 1e48cde01b9 = 7: 1e48cde01b9 merge-ort: implement compute_rename_counts()
8: f6efa4350d6 = 8: f6efa4350d6 merge-ort: implement handle_directory_level_conflicts()
9: bdd9d6cd702 = 9: bdd9d6cd702 merge-ort: modify collect_renames() for directory rename handling
10: 9a06c698857 = 10: 9a06c698857 merge-ort: implement compute_collisions()
11: 2ffb93c37ac = 11: 2ffb93c37ac merge-ort: implement apply_dir_rename() and check_dir_renamed()
12: cbfdf4d9ba0 = 12: cbfdf4d9ba0 merge-ort: implement check_for_directory_rename()
13: 734891cb315 = 13: 734891cb315 merge-ort: implement handle_path_level_conflicts()
14: 4b912f2c025 = 14: 4b912f2c025 merge-ort: add a new toplevel_dir field
15: d74417e86c5 = 15: d74417e86c5 merge-ort: implement apply_directory_rename_modifications()
16: 11e45af831d = 16: 11e45af831d merge-ort: process_renames() now needs more defensiveness
17: 551878bd84d = 17: 551878bd84d merge-ort: fix a directory rename detection bug
18: 786dc8f0c44 < -: ----------- DO NOT SUBMIT: directory rename stuff for redo_after_renames
--
gitgitgadget
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 21:37:13
From: Elijah Newren <redacted>
This function is based on merge-recursive.c's get_directory_renames(),
except that the first half has been split out into a not-yet-implemented
compute_rename_counts(). The primary difference here is our lack of the
non_unique_new_dir boolean in our strmap. The lack of that field will
at first cause us to fail testcase 2b of t6423; however, future
optimizations will obviate the need for that ugly field so we have just
left it out.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 59 insertions(+), 1 deletion(-)
@@ -721,11 +721,69 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/+staticvoidcompute_rename_counts(structdiff_queue_struct*pairs,+structstrmap*dir_rename_count,+structstrset*dirs_removed)+{+die("Not yet implemented!");+}+staticvoidget_provisional_directory_renames(structmerge_options*opt,unsignedside,int*clean){-die("Not yet implemented!");+structhashmap_iteriter;+structstrmap_entry*entry;+structrename_info*renames=&opt->priv->renames;++compute_rename_counts(&renames->pairs[side],+&renames->dir_rename_count[side],+&renames->dirs_removed[side]);+/*+*Collapse+*dir_rename_count:old_directory->{new_directory->count}+*downto+*dir_renames:old_directory->best_new_directory+*wherebest_new_directoryistheonewiththeuniquehighestcount.+*/+strmap_for_each_entry(&renames->dir_rename_count[side],&iter,entry){+constchar*source_dir=entry->key;+structstrintmap*counts=entry->value;+structhashmap_itercount_iter;+structstrmap_entry*count_entry;+intmax=0;+intbad_max=0;+constchar*best=NULL;++strintmap_for_each_entry(counts,&count_iter,count_entry){+constchar*target_dir=count_entry->key;+intptr_tcount=(intptr_t)count_entry->value;++if(count==max)+bad_max=max;+elseif(count>max){+max=count;+best=target_dir;+}+}++if(max==0)+continue;++if(bad_max==max){+path_msg(opt,source_dir,0,+_("CONFLICT (directory rename split): "+"Unclear where to rename %s to; it was "+"renamed to multiple other directories, with "+"no destination getting a majority of the "+"files."),+source_dir);+*clean&=0;+}else{+strmap_put(&renames->dir_renames[side],+source_dir,(void*)best);+}+}}staticvoidhandle_directory_level_conflicts(structmerge_options*opt)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 21:38:04
From: Elijah Newren <redacted>
collect_renames() is similar to merge-recursive.c's get_renames(), but
lacks the directory rename handling found in the latter. Port that code
structure over to merge-ort. This introduces three new
die-not-yet-implemented functions that will be defined in future
commits.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 74 insertions(+), 4 deletions(-)
@@ -721,6 +721,11 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/+structcollision_info{+structstring_listsource_files;+unsignedreported_already:1;+};+staticvoidget_renamed_dir_portion(constchar*old_path,constchar*new_path,char**old_dir,char**new_dir){
@@ -960,6 +965,31 @@ static void handle_directory_level_conflicts(struct merge_options *opt)string_list_clear(&duplicated,0);}+staticvoidcompute_collisions(structstrmap*collisions,+structstrmap*dir_renames,+structdiff_queue_struct*pairs)+{+die("Not yet implemented.");+}++staticchar*check_for_directory_rename(structmerge_options*opt,+constchar*path,+unsignedside_index,+structstrmap*dir_renames,+structstrmap*dir_rename_exclusions,+structstrmap*collisions,+int*clean_merge)+{+die("Not yet implemented.");+}++staticvoidapply_directory_rename_modifications(structmerge_options*opt,+structdiff_filepair*pair,+char*new_path)+{+die("Not yet implemented.");+}+/*** Function Grouping: functions related to regular rename detection ***/staticintprocess_renames(structmerge_options*opt,
@@ -1285,22 +1315,44 @@ static void detect_regular_renames(struct merge_options *opt,*/staticintcollect_renames(structmerge_options*opt,structdiff_queue_struct*result,-unsignedside_index)+unsignedside_index,+structstrmap*dir_renames_for_side,+structstrmap*rename_exclusions){inti,clean=1;+structstrmapcollisions;structdiff_queue_struct*side_pairs;+structhashmap_iteriter;+structstrmap_entry*entry;structrename_info*renames=&opt->priv->renames;side_pairs=&renames->pairs[side_index];+compute_collisions(&collisions,dir_renames_for_side,side_pairs);for(i=0;i<side_pairs->nr;++i){structdiff_filepair*p=side_pairs->queue[i];+char*new_path;/* non-NULL only with directory renames */-if(p->status!='R'){+if(p->status!='A'&&p->status!='R'){diff_free_filepair(p);continue;}+new_path=check_for_directory_rename(opt,p->two->path,+side_index,+dir_renames_for_side,+rename_exclusions,+&collisions,+&clean);++if(p->status!='R'&&!new_path){+diff_free_filepair(p);+continue;+}++if(new_path)+apply_directory_rename_modifications(opt,p,new_path);+/**p->scorecomesbackfromdiffcore_rename_extended()with*thesimilarityoftherenamedfile.Thesimilarityis
@@ -1315,6 +1367,20 @@ static int collect_renames(struct merge_options *opt,result->queue[result->nr++]=p;}+/* Free each value in the collisions map */+strmap_for_each_entry(&collisions,&iter,entry){+structcollision_info*info=entry->value;+string_list_clear(&info->source_files,0);+}+/*+*Incompute_collisions(),wesetcollisions.strdup_stringsto0+*sothatwewouldn'thavetomakeanothercopyofthenew_path+*allocatedbyapply_dir_rename().Butnowthatwe'veusedthem+*andhavenootherreferencestothesestrings,itistimeto+*deallocatethem.+*/+free_strmap_strings(&collisions);+strmap_clear(&collisions,1);returnclean;}
@@ -1346,8 +1412,12 @@ static int detect_and_process_renames(struct merge_options *opt,ALLOC_GROW(combined.queue,renames->pairs[1].nr+renames->pairs[2].nr,combined.alloc);-clean&=collect_renames(opt,&combined,MERGE_SIDE1);-clean&=collect_renames(opt,&combined,MERGE_SIDE2);+clean&=collect_renames(opt,&combined,MERGE_SIDE1,+&renames->dir_renames[2],+&renames->dir_renames[1]);+clean&=collect_renames(opt,&combined,MERGE_SIDE2,+&renames->dir_renames[1],+&renames->dir_renames[2]);QSORT(combined.queue,combined.nr,compare_pairs);clean&=process_renames(opt,&combined);
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 21:38:04
From: Elijah Newren <redacted>
This function roughly follows the same outline as the function of the
same name from merge-recursive.c, but the code diverges in multiple
ways due to some special considerations:
* merge-ort's version needs to update opt->priv->paths with any new
paths (and opt->priv->paths points to struct conflict_infos which
track quite a bit of metadata for each path); merge-recursive's
version would directly update the index
* merge-ort requires that opt->priv->paths has any leading directories
of any relevant files also be included in the set of paths. And
due to pointer equality requirements on merged_info.directory_name,
we have to be careful how we compute and insert these.
* due to the above requirements on opt->priv->paths, merge-ort's
version starts with a long comment to explain all the special
considerations that need to be handled
* merge-ort can use the full data stored in opt->priv->paths to avoid
making expensive get_tree_entry() calls to regather the necessary
data.
* due to messages being deferred automatically in merge-ort, this is
the best place to handle conflict messages whereas in
merge-recursive.c they are deferred manually so that processing of
entries does all the printing
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 168 +++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 167 insertions(+), 1 deletion(-)
@@ -1224,7 +1224,173 @@ static void apply_directory_rename_modifications(struct merge_options *opt,structdiff_filepair*pair,char*new_path){-die("Not yet implemented.");+/*+*Thebasicideaistogettheconflict_infofromopt->priv->paths+*atoldpath,andinsertitintonew_path;basicallyjustthis:+*ci=strmap_get(&opt->priv->paths,old_path);+*strmap_remove(&opt->priv->paths,old_path,0);+*strmap_put(&opt->priv->paths,new_path,ci);+*However,therearesomefactorscomplicatingthis:+*-opt->priv->pathsmayalreadyhaveanentryatnew_path+*-Eachcitracksitscontainingdirectory,soweneedto+*updatethat+*-Ifanothercihasthesamecontainingdirectory,then+*thetwochar*'sMUSTpointtothesamelocation.Seethe+*commentinstructmerged_info.strcmpequalityisnot+*enough;weneedpointerequality.+*-opt->priv->pathsmustholdtheparentdirectoriesofany+*entriesthatareadded.So,ifthisdirectoryrename+*causesentirelynewdirectories,wemustrecursivelyadd+*parentdirectories.+*-Foreachparentdirectoryaddedtoopt->priv->paths,we+*alsoneedtogetitsparentdirectorystoredinits+*conflict_info->merged.directory_namewithallthesame+*requirementsaboutpointerequality.+*/+structstring_listdirs_to_insert=STRING_LIST_INIT_NODUP;+structconflict_info*ci,*new_ci;+structstrmap_entry*entry;+constchar*branch_with_new_path,*branch_with_dir_rename;+constchar*old_path=pair->two->path;+constchar*parent_name;+constchar*cur_path;+inti,len;++entry=strmap_get_entry(&opt->priv->paths,old_path);+old_path=entry->key;+ci=entry->value;+VERIFY_CI(ci);++/* Find parent directories missing from opt->priv->paths */+cur_path=new_path;+while(1){+/* Find the parent directory of cur_path */+char*last_slash=strrchr(cur_path,'/');+if(last_slash){+parent_name=xstrndup(cur_path,last_slash-cur_path);+}else{+parent_name=opt->priv->toplevel_dir;+break;+}++/* Look it up in opt->priv->paths */+entry=strmap_get_entry(&opt->priv->paths,parent_name);+if(entry){+free((char*)parent_name);+parent_name=entry->key;/* reuse known pointer */+break;+}++/* Record this is one of the directories we need to insert */+string_list_append(&dirs_to_insert,parent_name);+cur_path=parent_name;+}++/* Traverse dirs_to_insert and insert them into opt->priv->paths */+for(i=dirs_to_insert.nr-1;i>=0;--i){+structconflict_info*dir_ci;+char*cur_dir=dirs_to_insert.items[i].string;++dir_ci=xcalloc(1,sizeof(*dir_ci));++dir_ci->merged.directory_name=parent_name;+len=strlen(parent_name);+/* len+1 because of trailing '/' character */+dir_ci->merged.basename_offset=(len>0?len+1:len);+dir_ci->dirmask=ci->filemask;+strmap_put(&opt->priv->paths,cur_dir,dir_ci);++parent_name=cur_dir;+}++/*+*Weareremovingold_pathfromopt->priv->paths.old_pathalsowill+*eventuallyneedtobefreed,butitmaystillbeusedbye.g.+*ci->pathnames.So,storeitinanotherstring-listfornow.+*/+string_list_append(&opt->priv->paths_to_free,old_path);++assert(ci->filemask==2||ci->filemask==4);+assert(ci->dirmask==0);+strmap_remove(&opt->priv->paths,old_path,0);++branch_with_new_path=(ci->filemask==2)?opt->branch1:opt->branch2;+branch_with_dir_rename=(ci->filemask==2)?opt->branch2:opt->branch1;++/* Now, finally update ci and stick it into opt->priv->paths */+ci->merged.directory_name=parent_name;+len=strlen(parent_name);+ci->merged.basename_offset=(len>0?len+1:len);+new_ci=strmap_get(&opt->priv->paths,new_path);+if(!new_ci){+/* Place ci back into opt->priv->paths, but at new_path */+strmap_put(&opt->priv->paths,new_path,ci);+}else{+intindex;++/* A few sanity checks */+VERIFY_CI(new_ci);+assert(ci->filemask==2||ci->filemask==4);+assert((new_ci->filemask&ci->filemask)==0);+assert(!new_ci->merged.clean);++/* Copy stuff from ci into new_ci */+new_ci->filemask|=ci->filemask;+if(new_ci->dirmask)+new_ci->df_conflict=1;+index=(ci->filemask>>1);+new_ci->pathnames[index]=ci->pathnames[index];+new_ci->stages[index].mode=ci->stages[index].mode;+oidcpy(&new_ci->stages[index].oid,&ci->stages[index].oid);++free(ci);+ci=new_ci;+}++if(opt->detect_directory_renames==MERGE_DIRECTORY_RENAMES_TRUE){+/* Notify user of updated path */+if(pair->status=='A')+path_msg(opt,new_path,1,+_("Path updated: %s added in %s inside a "+"directory that was renamed in %s; moving "+"it to %s."),+old_path,branch_with_new_path,+branch_with_dir_rename,new_path);+else+path_msg(opt,new_path,1,+_("Path updated: %s renamed to %s in %s, "+"inside a directory that was renamed in %s; "+"moving it to %s."),+pair->one->path,old_path,branch_with_new_path,+branch_with_dir_rename,new_path);+}else{+/*+*opt->detect_directory_renameshasthevalue+*MERGE_DIRECTORY_RENAMES_CONFLICT,somarktheseasconflicts.+*/+ci->path_conflict=1;+if(pair->status=='A')+path_msg(opt,new_path,0,+_("CONFLICT (file location): %s added in %s "+"inside a directory that was renamed in %s, "+"suggesting it should perhaps be moved to "+"%s."),+old_path,branch_with_new_path,+branch_with_dir_rename,new_path);+else+path_msg(opt,new_path,0,+_("CONFLICT (file location): %s renamed to %s "+"in %s, inside a directory that was renamed "+"in %s, suggesting it should perhaps be "+"moved to %s."),+pair->one->path,old_path,branch_with_new_path,+branch_with_dir_rename,new_path);+}++/*+*Finally,recordthenewlocation.+*/+pair->two->path=new_path;}/*** Function Grouping: functions related to regular rename detection ***/
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 21:38:04
From: Elijah Newren <redacted>
Since directory rename detection adds new paths to opt->priv->paths and
removes old ones, process_renames() needs to now check whether
pair->one->path actually exists in opt->priv->paths instead of just
assuming it does.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 26 +++++++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 21:38:04
From: Elijah Newren <redacted>
Port some directory rename handling changes from merge-recursive.c's
detect_and_process_renames() to the same-named function of merge-ort.c.
This does not yet add any use or handling of directory renames, just the
outline for where we start to compute them. Thus, a future patch will
add port additional changes to merge-ort's detect_and_process_renames().
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
@@ -721,6 +721,18 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/+staticvoidget_provisional_directory_renames(structmerge_options*opt,+unsignedside,+int*clean)+{+die("Not yet implemented!");+}++staticvoidhandle_directory_level_conflicts(structmerge_options*opt)+{+die("Not yet implemented!");+}+/*** Function Grouping: functions related to regular rename detection ***/staticintprocess_renames(structmerge_options*opt,
@@ -1086,13 +1098,24 @@ static int detect_and_process_renames(struct merge_options *opt,{structdiff_queue_structcombined;structrename_info*renames=&opt->priv->renames;-ints,clean=1;+intneed_dir_renames,s,clean=1;memset(&combined,0,sizeof(combined));detect_regular_renames(opt,merge_base,side1,MERGE_SIDE1);detect_regular_renames(opt,merge_base,side2,MERGE_SIDE2);+need_dir_renames=+!opt->priv->call_depth&&+(opt->detect_directory_renames==MERGE_DIRECTORY_RENAMES_TRUE||+opt->detect_directory_renames==MERGE_DIRECTORY_RENAMES_CONFLICT);++if(need_dir_renames){+for(s=MERGE_SIDE1;s<=MERGE_SIDE2;s++)+get_provisional_directory_renames(opt,s,&clean);+handle_directory_level_conflicts(opt);+}+ALLOC_GROW(combined.queue,renames->pairs[1].nr+renames->pairs[2].nr,combined.alloc);
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 21:38:04
From: Elijah Newren <redacted>
This is nearly a wholesale copy of compute_collisions() from
merge-recursive.c, and the logic remains the same, but it has been
tweaked slightly due to:
* using strmap.h API (instead of direct hashmaps)
* allocation/freeing of data structures were done separately in
merge_start() and clear_or_reinit_internal_opts() in an earlier
patch in this series
* there is no non_unique_new_dir data field in merge-ort; that will
be handled a different way
It does depend on two new functions, apply_dir_rename() and
check_dir_renamed() which were introduced with simple
die-not-yet-implemented shells and will be implemented in subsequent
patches.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 67 insertions(+), 1 deletion(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 21:38:05
From: Elijah Newren <redacted>
This is copied from merge-recursive.c, with minor tweaks due to:
* using strmap API
* merge-ort not using the non_unique_new_dir field, since it'll
obviate its need entirely later with performance improvements
* adding a new path_in_way() function that uses opt->priv->paths
instead of doing an expensive tree_has_path() lookup to see if
a tree has a given path.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 71 insertions(+), 1 deletion(-)
@@ -876,7 +886,67 @@ static char *handle_path_level_conflicts(struct merge_options *opt,structstrmap_entry*rename_info,structstrmap*collisions){-die("Not yet implemented");+char*new_path=NULL;+structcollision_info*c_info;+intclean=1;+structstrbufcollision_paths=STRBUF_INIT;++/*+*entryhasthemappingofolddirectorynametonewdirectoryname+*thatwewanttoapplytopath.+*/+new_path=apply_dir_rename(rename_info,path);+if(!new_path)+BUG("Failed to apply directory rename!");++/*+*Thecallerneedstohaveensuredthatithaspre-populated+*collisionswithallpathsthatmaptonew_path.Doaquickcheck+*toensurethat'sthecase.+*/+c_info=strmap_get(collisions,new_path);+if(c_info==NULL)+BUG("c_info is NULL");++/*+*Checkforone-sidedadd/add/.../addconflicts,i.e.+*whereimplicitrenamesfromtheothersidedoing+*directoryrename(s)canaffectthissideofhistory+*toputmultiplepathsintothesamelocation.Warn+*andbailondirectoryrenamesforsuchpaths.+*/+if(c_info->reported_already){+clean=0;+}elseif(path_in_way(&opt->priv->paths,new_path,1<<side_index)){+c_info->reported_already=1;+strbuf_add_separated_string_list(&collision_paths,", ",+&c_info->source_files);+path_msg(opt,new_path,0,+_("CONFLICT (implicit dir rename): Existing file/dir "+"at %s in the way of implicit directory rename(s) "+"putting the following path(s) there: %s."),+new_path,collision_paths.buf);+clean=0;+}elseif(c_info->source_files.nr>1){+c_info->reported_already=1;+strbuf_add_separated_string_list(&collision_paths,", ",+&c_info->source_files);+path_msg(opt,new_path,0,+_("CONFLICT (implicit dir rename): Cannot map more "+"than one path to %s; implicit directory renames "+"tried to put these paths there: %s"),+new_path,collision_paths.buf);+clean=0;+}++/* Free memory we no longer need */+strbuf_release(&collision_paths);+if(!clean&&new_path){+free(new_path);+returnNULL;+}++returnnew_path;}staticvoidincrement_count(structstrmap*dir_rename_count,
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 21:38:05
From: Elijah Newren <redacted>
Both of these are copied from merge-recursive.c, with just minor tweaks
due to using strmap API and not having a non_unique_new_dir field.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 37 +++++++++++++++++++++++++++++++++++--
1 file changed, 35 insertions(+), 2 deletions(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 21:38:05
From: Elijah Newren <redacted>
Due to the string-equality-iff-pointer-equality requirements placed on
merged_info.directory_name, apply_directory_rename_modifications() will
need to have access to the exact toplevel directory name string pointer
and can't just use a new empty string. Store it in a field that we can
use.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 21:38:05
From: Elijah Newren <redacted>
This is modelled on the version of handle_directory_level_conflicts()
from merge-recursive.c, but is massively simplified due to the following
factors:
* strmap API provides simplifications over using direct hashamp
* we have a dirs_removed field in struct rename_info that we have an
easy way to populate from collect_merge_info(); this was already
used in compute_rename_counts() and thus we do not need to check
for condition #2.
* The removal of condition #2 by handling it earlier in the code also
obviates the need to check for condition #3 -- if both sides renamed
a directory, meaning that the directory no longer exists on either
side, then neither side could have added any new files to that
directory, and thus there are no files whose locations we need to
move due to such a directory rename.
In fact, the same logic that makes condition #3 irrelevant means
condition #1 is also irrelevant so we could drop this function.
However, it is cheap to check if both sides rename the same directory,
and doing so can save future computation. So, simply remove any
directories that both sides renamed from the list of directory renames.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
@@ -721,6 +721,110 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/+MAYBE_UNUSED+staticvoidget_renamed_dir_portion(constchar*old_path,constchar*new_path,+char**old_dir,char**new_dir)+{+char*end_of_old,*end_of_new;++/* Default return values: NULL, meaning no rename */+*old_dir=NULL;+*new_dir=NULL;++/*+*For+*"a/b/c/d/e/foo.c"->"a/b/some/thing/else/e/foo.c"+*the"e/foo.c"partisthesame,wejustwanttoknowthat+*"a/b/c/d"wasrenamedto"a/b/some/thing/else"+*so,forthisexample,thisfunctionreturns"a/b/c/d"in+**old_dirand"a/b/some/thing/else"in*new_dir.+*/++/*+*Ifthebasenameofthefilechanged,wedon'tcare.Wewant+*toknowwhichportionofthedirectory,ifany,changed.+*/+end_of_old=strrchr(old_path,'/');+end_of_new=strrchr(new_path,'/');++/*+*Ifend_of_oldisNULL,old_pathwasn'tinadirectory,sothere+*couldnotbeadirectoryrename(ourruleelsewherethata+*directorywhichstillexistsisnotconsideredtohavebeen+*renamedmeanstherootdirectorycanneverberenamed--because+*therootdirectoryalwaysexists).+*/+if(end_of_old==NULL)+return;/* Note: *old_dir and *new_dir are still NULL */++/*+*Ifnew_pathcontainsnodirectory(end_of_newisNULL),thenwe+*havearenameofold_path'sdirectorytotherootdirectory.+*/+if(end_of_new==NULL){+*old_dir=xstrndup(old_path,end_of_old-old_path);+*new_dir=xstrdup("");+return;+}++/* Find the first non-matching character traversing backwards */+while(*--end_of_new==*--end_of_old&&+end_of_old!=old_path&&+end_of_new!=new_path)+;/* Do nothing; all in the while loop */++/*+*Ifbothgotbacktothebeginningoftheirstrings,thenthe+*directorydidn'tchangeatall,onlythebasenamedid.+*/+if(end_of_old==old_path&&end_of_new==new_path&&+*end_of_old==*end_of_new)+return;/* Note: *old_dir and *new_dir are still NULL */++/*+*Ifend_of_newgotbacktothebeginningofitsstring,and+*end_of_oldgotbacktothebeginningofsomesubdirectory,then+*wehavearename/mergeofasubdirectoryintotheroot,which+*needsslightlyspecialhandling.+*+*Note:Thereisnoneedtoconsidertheoppositecase,witha+*rename/mergeoftherootdirectoryintosomesubdirectory+*becauseasnotedabovetherootdirectoryalwaysexistssoit+*cannotbeconsideredtoberenamed.+*/+if(end_of_new==new_path&&+end_of_old!=old_path&&end_of_old[-1]=='/'){+*old_dir=xstrndup(old_path,--end_of_old-old_path);+*new_dir=xstrdup("");+return;+}++/*+*We'vefoundthefirstnon-matchingcharacterinthedirectory+*paths.Thatmeansthecurrentcharacterswewerelookingat+*werepartofthefirstnon-matchingsubdirnamegoingbackfrom+*theendofthestrings.Getthewholenamebyadvancingboth+*end_of_oldandend_of_newtotheNEXT'/'character.Thatwill+*representtheentiredirectoryrename.+*+*Thereasonfortheincrementiscaseslike+*a/b/star/foo/whatever.c->a/b/tar/foo/random.c+*Afterdroppingthebasenameandgoingbacktothefirst+*non-matchingcharacter,we'renowcomparing:+*a/b/sanda/b/+*andwewanttobecomparing:+*a/b/star/anda/b/tar/+*butwithoutthepre-increment,theoneontherightwouldstay+*a/b/.+*/+end_of_old=strchr(++end_of_old,'/');+end_of_new=strchr(++end_of_new,'/');++/* Copy the old and new directories into *old_dir and *new_dir. */+*old_dir=xstrndup(old_path,end_of_old-old_path);+*new_dir=xstrndup(new_path,end_of_new-new_path);+}+staticvoidcompute_rename_counts(structdiff_queue_struct*pairs,structstrmap*dir_rename_count,structstrset*dirs_removed)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 21:38:05
From: Elijah Newren <redacted>
This is copied from merge-recursive.c, with minor tweaks due to using strmap
API and the fact that it can use opt->priv->paths to get all pathnames that
exist instead of taking a tree object.
This depends on a new function, handle_path_level_conflicts(), which
just has a placeholder die-not-yet-implemented implementation for now; a
subsequent patch will implement it.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 66 insertions(+), 1 deletion(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 21:38:05
From: Elijah Newren <redacted>
This function is based on the first half of get_directory_renames() from
merge-recursive.c
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 51 insertions(+), 2 deletions(-)
@@ -721,7 +721,6 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/-MAYBE_UNUSEDstaticvoidget_renamed_dir_portion(constchar*old_path,constchar*new_path,char**old_dir,char**new_dir){
@@ -825,11 +824,61 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,*new_dir=xstrndup(new_path,end_of_new-new_path);}+staticvoidincrement_count(structstrmap*dir_rename_count,+char*old_dir,+char*new_dir)+{+structstrintmap*counts;+structstrmap_entry*e;++/* Get the {new_dirs -> counts} mapping using old_dir */+e=strmap_get_entry(dir_rename_count,old_dir);+if(e){+counts=e->value;+}else{+counts=xmalloc(sizeof(*counts));+strintmap_init_with_options(counts,0,NULL,1);+strmap_put(dir_rename_count,old_dir,counts);+}++/* Increment the count for new_dir */+strintmap_incr(counts,new_dir,1);+}+staticvoidcompute_rename_counts(structdiff_queue_struct*pairs,structstrmap*dir_rename_count,structstrset*dirs_removed){-die("Not yet implemented!");+inti;++for(i=0;i<pairs->nr;++i){+char*old_dir,*new_dir;+structdiff_filepair*pair=pairs->queue[i];++if(pair->status!='R')+continue;++/* Get the old and new directory names */+get_renamed_dir_portion(pair->one->path,pair->two->path,+&old_dir,&new_dir);+if(!old_dir)+/* Directory didn't change at all; ignore this one. */+continue;++/*+*Makedir_rename_countcontainamapofamap:+*old_directory->{new_directory->count}+*Inotherwords,foreverypairlookatthedirectoriesfor+*theoldfilenameandthenewfilenameandcounthowmany+*timesthatpairingoccurs.+*/+if(strset_contains(dirs_removed,old_dir))+increment_count(dir_rename_count,old_dir,new_dir);++/* Free resources we don't need anymore */+free(old_dir);+free(new_dir);+}}staticvoidget_provisional_directory_renames(structmerge_options*opt,
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-07 21:38:05
From: Elijah Newren <redacted>
As noted in commit 902c521a35 ("t6423: more involved directory rename
test", 2020-10-15), when we have a case where
* dir/subdir/ has several files
* almost all files in dir/subdir/ are renamed to folder/subdir/
* one of the files in dir/subdir/ is renamed to folder/subdir/newsubdir/
* the other side of history (that doesn't do the renames) adds a
new file to dir/subdir/
Then for the majority of the file renames, the directory rename of
dir/subdir/ -> folder/subdir/
is actually not represented that way but as
dir/ -> folder/
We also had one rename that was represented as
dir/subdir/ -> folder/subdir/newsubdir/
Now, since there's a new file in dir/subdir/, where does it go? Well,
there's only one rule for dir/subdir/, so the code previously noted that
this rule had the "majority" of the one "relevant" rename and thus
erroneously used it to place the file in folder/subdir/newsubdir/. We
really want the heavy weight associated with dir/ -> folder/ to also be
treated as dir/subdir/ -> folder/subdir/, so that we correctly place the
file in folder/subdir/.
Add a bunch of logic to make sure that we use all relevant renamings in
directory rename detection.
Note that testcase 12f of t6423 still fails after this, but it gets
further than merge-recursive does. There are some performance related
bits in that testcase (the region_enter messages) that do not yet
succeed, but the rest of the testcase works after this patch.
Subsequent patch series will fix up the performance side.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 198 +++++++++++++++++++++-------------------------------
1 file changed, 81 insertions(+), 117 deletions(-)
@@ -764,109 +764,6 @@ static char *apply_dir_rename(struct strmap_entry *rename_info,returnstrbuf_detach(&new_path,NULL);}-staticvoidget_renamed_dir_portion(constchar*old_path,constchar*new_path,-char**old_dir,char**new_dir)-{-char*end_of_old,*end_of_new;--/* Default return values: NULL, meaning no rename */-*old_dir=NULL;-*new_dir=NULL;--/*-*For-*"a/b/c/d/e/foo.c"->"a/b/some/thing/else/e/foo.c"-*the"e/foo.c"partisthesame,wejustwanttoknowthat-*"a/b/c/d"wasrenamedto"a/b/some/thing/else"-*so,forthisexample,thisfunctionreturns"a/b/c/d"in-**old_dirand"a/b/some/thing/else"in*new_dir.-*/--/*-*Ifthebasenameofthefilechanged,wedon'tcare.Wewant-*toknowwhichportionofthedirectory,ifany,changed.-*/-end_of_old=strrchr(old_path,'/');-end_of_new=strrchr(new_path,'/');--/*-*Ifend_of_oldisNULL,old_pathwasn'tinadirectory,sothere-*couldnotbeadirectoryrename(ourruleelsewherethata-*directorywhichstillexistsisnotconsideredtohavebeen-*renamedmeanstherootdirectorycanneverberenamed--because-*therootdirectoryalwaysexists).-*/-if(end_of_old==NULL)-return;/* Note: *old_dir and *new_dir are still NULL */--/*-*Ifnew_pathcontainsnodirectory(end_of_newisNULL),thenwe-*havearenameofold_path'sdirectorytotherootdirectory.-*/-if(end_of_new==NULL){-*old_dir=xstrndup(old_path,end_of_old-old_path);-*new_dir=xstrdup("");-return;-}--/* Find the first non-matching character traversing backwards */-while(*--end_of_new==*--end_of_old&&-end_of_old!=old_path&&-end_of_new!=new_path)-;/* Do nothing; all in the while loop */--/*-*Ifbothgotbacktothebeginningoftheirstrings,thenthe-*directorydidn'tchangeatall,onlythebasenamedid.-*/-if(end_of_old==old_path&&end_of_new==new_path&&-*end_of_old==*end_of_new)-return;/* Note: *old_dir and *new_dir are still NULL */--/*-*Ifend_of_newgotbacktothebeginningofitsstring,and-*end_of_oldgotbacktothebeginningofsomesubdirectory,then-*wehavearename/mergeofasubdirectoryintotheroot,which-*needsslightlyspecialhandling.-*-*Note:Thereisnoneedtoconsidertheoppositecase,witha-*rename/mergeoftherootdirectoryintosomesubdirectory-*becauseasnotedabovetherootdirectoryalwaysexistssoit-*cannotbeconsideredtoberenamed.-*/-if(end_of_new==new_path&&-end_of_old!=old_path&&end_of_old[-1]=='/'){-*old_dir=xstrndup(old_path,--end_of_old-old_path);-*new_dir=xstrdup("");-return;-}--/*-*We'vefoundthefirstnon-matchingcharacterinthedirectory-*paths.Thatmeansthecurrentcharacterswewerelookingat-*werepartofthefirstnon-matchingsubdirnamegoingbackfrom-*theendofthestrings.Getthewholenamebyadvancingboth-*end_of_oldandend_of_newtotheNEXT'/'character.Thatwill-*representtheentiredirectoryrename.-*-*Thereasonfortheincrementiscaseslike-*a/b/star/foo/whatever.c->a/b/tar/foo/random.c-*Afterdroppingthebasenameandgoingbacktothefirst-*non-matchingcharacter,we'renowcomparing:-*a/b/sanda/b/-*andwewanttobecomparing:-*a/b/star/anda/b/tar/-*butwithoutthepre-increment,theoneontherightwouldstay-*a/b/.-*/-end_of_old=strchr(++end_of_old,'/');-end_of_new=strchr(++end_of_new,'/');--/* Copy the old and new directories into *old_dir and *new_dir. */-*old_dir=xstrndup(old_path,end_of_old-old_path);-*new_dir=xstrndup(new_path,end_of_new-new_path);-}-staticintpath_in_way(structstrmap*paths,constchar*path,unsignedside_mask){structmerged_info*mi=strmap_get(paths,path);
@@ -973,6 +878,76 @@ static void increment_count(struct strmap *dir_rename_count,strintmap_incr(counts,new_dir,1);}+staticvoidupdate_dir_rename_counts(structstrmap*dir_rename_count,+structstrset*dirs_removed,+constchar*oldname,+constchar*newname)+{+char*old_dir=xstrdup(oldname);+char*new_dir=xstrdup(newname);+charnew_dir_first_char=new_dir[0];+intfirst_time_in_loop=1;++while(1){+dirname_munge(old_dir);+dirname_munge(new_dir);++/*+*Whenrenaming+*"a/b/c/d/e/foo.c"->"a/b/some/thing/else/e/foo.c"+*thenthissuggeststhatboth+*a/b/c/d/e/=>a/b/some/thing/else/e/+*a/b/c/d/=>a/b/some/thing/else/+*sowewanttoincrementcountersforboth.WedoNOT,+*however,alsowanttosuggestthattherewasthefollowing+*rename:+*a/b/c/=>a/b/some/thing/+*soweneedtoquitatthatpoint.+*+*Notethewhenfirst_time_in_loop,weonlystripoffthe+*basename,andwedon'tcareifthat'sdifferent.+*/+if(!first_time_in_loop){+char*old_sub_dir=strchr(old_dir,'\0')+1;+char*new_sub_dir=strchr(new_dir,'\0')+1;+if(!*new_dir){+/*+*Specialcasewhenrenamingtorootdirectory,+*i.e.whennew_dir=="".Inthiscase,wehad+*somethinglike+*a/b/subdir=>subdir+*andsodirname_munge()setsthingsupsothat+*old_dir="a/b\0subdir\0"+*new_dir="\0ubdir\0"+*Wedidn'thavea'/'tooverwritea'\0'onto+*innew_dir,sowehavetocomparedifferently.+*/+if(new_dir_first_char!=old_sub_dir[0]||+strcmp(old_sub_dir+1,new_sub_dir))+break;+}else{+if(strcmp(old_sub_dir,new_sub_dir))+break;+}+}++if(strset_contains(dirs_removed,old_dir))+increment_count(dir_rename_count,old_dir,new_dir);+else+break;++/* If we hit toplevel directory ("") for old or new dir, quit */+if(!*old_dir||!*new_dir)+break;++first_time_in_loop=0;+}++/* Free resources we don't need anymore */+free(old_dir);+free(new_dir);+}+staticvoidcompute_rename_counts(structdiff_queue_struct*pairs,structstrmap*dir_rename_count,structstrset*dirs_removed)
@@ -980,19 +955,11 @@ static void compute_rename_counts(struct diff_queue_struct *pairs,inti;for(i=0;i<pairs->nr;++i){-char*old_dir,*new_dir;structdiff_filepair*pair=pairs->queue[i];if(pair->status!='R')continue;-/* Get the old and new directory names */-get_renamed_dir_portion(pair->one->path,pair->two->path,-&old_dir,&new_dir);-if(!old_dir)-/* Directory didn't change at all; ignore this one. */-continue;-/**Makedir_rename_countcontainamapofamap:*old_directory->{new_directory->count}
@@ -1000,12 +967,9 @@ static void compute_rename_counts(struct diff_queue_struct *pairs,*theoldfilenameandthenewfilenameandcounthowmany*timesthatpairingoccurs.*/-if(strset_contains(dirs_removed,old_dir))-increment_count(dir_rename_count,old_dir,new_dir);--/* Free resources we don't need anymore */-free(old_dir);-free(new_dir);+update_dir_rename_counts(dir_rename_count,dirs_removed,+pair->one->path,+pair->two->path);}}
From: Taylor Blau <hidden> Date: 2021-01-18 19:58:59
On Thu, Jan 07, 2021 at 09:35:52PM +0000, Elijah Newren via GitGitGadget wrote:
From: Elijah Newren <redacted>
Port some directory rename handling changes from merge-recursive.c's
detect_and_process_renames() to the same-named function of merge-ort.c.
Thanks, having the source be explicitly named makes it much easier to
check over the reimplementation.
This does not yet add any use or handling of directory renames, just the
outline for where we start to compute them. Thus, a future patch will
add port additional changes to merge-ort's detect_and_process_renames().
Would it be worth it to DRY up this and merge-recursive.c's
implementation, perhaps:
int needs_dir_renames(struct merge_options *opt)
{
return !opt->priv->call_depth &&
(opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_TRUE ||
opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT);
}
and then calling that in both places?
+ if (need_dir_renames) {
+ for (s = MERGE_SIDE1; s <= MERGE_SIDE2; s++)
+ get_provisional_directory_renames(opt, s, &clean);
Not necessarily related to this patch, but just something that I noticed
while reading the series: I don't find this for-loop to be any clearer
than:
if (need_dir_renames) {
get_provisional_directory_renames(opt, MERGE_SIDE1, &clean);
get_provisional_directory_renames(opt, MERGE_SIDE2, &clean);
/* ... */
}
In fact, I think that I find the above clearer than the for-loop.
There's no opportunity to write (...; s < MERGE_SIDE2) when you meant
(...; s <= MERGE_SIDE2), and seeing two lines explicitly makes it
clearer that you're really doing the same thing to each side of the
merge.
Anyway, I may feel totally different than others, and of course I
haven't read many of the previous series as closely as this (and so this
may already be an established pattern and I'm just cutting against the
grain here), but those are my $.02.
Thanks,
Taylor
On Mon, Jan 18, 2021 at 11:54 AM Taylor Blau [off-list ref] wrote:
On Thu, Jan 07, 2021 at 09:35:52PM +0000, Elijah Newren via GitGitGadget wrote:
quoted
From: Elijah Newren <redacted>
Port some directory rename handling changes from merge-recursive.c's
detect_and_process_renames() to the same-named function of merge-ort.c.
Thanks, having the source be explicitly named makes it much easier to
check over the reimplementation.
quoted
This does not yet add any use or handling of directory renames, just the
outline for where we start to compute them. Thus, a future patch will
add port additional changes to merge-ort's detect_and_process_renames().
Would it be worth it to DRY up this and merge-recursive.c's
implementation, perhaps:
int needs_dir_renames(struct merge_options *opt)
{
return !opt->priv->call_depth &&
(opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_TRUE ||
opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT);
}
and then calling that in both places?
If the intent was to keep merge-recursive.c indefinitely, then yes it
would. However, the intent is to (1) avoid touching merge-recursive.c
so I don't destabilize it and so folks can fall back to it, (2) get
merge-ort.c completed, and people to adopt and feel confident in it,
(3) delete merge-recursive.[ch].
This has come up a few other times in a review on the series, because
there are even examples of copied-and-unmodified functions; see
https://lore.kernel.org/git/CABPp-BGtpXRSz+ngFz20j8W4qgpb8juogsLf6HF7b0-Pt=s6=g@mail.gmail.com/
and https://lore.kernel.org/git/CABPp-BEEoqOer11BYrqSVE9E4HcT5MNWcRm7ZHBQ7MVZRUDVXw@mail.gmail.com/.
I know it seems weird to intentionally repeat, but since the goal is
to nuke merge-recursive.c, I'm doing it as a temporary measure.
quoted
+ if (need_dir_renames) {
+ for (s = MERGE_SIDE1; s <= MERGE_SIDE2; s++)
+ get_provisional_directory_renames(opt, s, &clean);
Not necessarily related to this patch, but just something that I noticed
while reading the series: I don't find this for-loop to be any clearer
than:
if (need_dir_renames) {
get_provisional_directory_renames(opt, MERGE_SIDE1, &clean);
get_provisional_directory_renames(opt, MERGE_SIDE2, &clean);
/* ... */
}
That also makes it more similar to the calls I make to
detect_regular_renames() above, where I explicitly called it twice
instead of using a loop. I like the suggested change; I'll update it.
In fact, I think that I find the above clearer than the for-loop.
There's no opportunity to write (...; s < MERGE_SIDE2) when you meant
(...; s <= MERGE_SIDE2), and seeing two lines explicitly makes it
clearer that you're really doing the same thing to each side of the
merge.
Anyway, I may feel totally different than others, and of course I
haven't read many of the previous series as closely as this (and so this
may already be an established pattern and I'm just cutting against the
grain here), but those are my $.02.
Thanks,
Taylor
From: Taylor Blau <hidden> Date: 2021-01-18 20:24:34
On Thu, Jan 07, 2021 at 09:35:53PM +0000, Elijah Newren via GitGitGadget wrote:
quoted hunk
From: Elijah Newren <redacted>
This function is based on merge-recursive.c's get_directory_renames(),
except that the first half has been split out into a not-yet-implemented
compute_rename_counts(). The primary difference here is our lack of the
non_unique_new_dir boolean in our strmap. The lack of that field will
at first cause us to fail testcase 2b of t6423; however, future
optimizations will obviate the need for that ugly field so we have just
left it out.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 59 insertions(+), 1 deletion(-)
@@ -721,11 +721,69 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/+staticvoidcompute_rename_counts(structdiff_queue_struct*pairs,+structstrmap*dir_rename_count,+structstrset*dirs_removed)+{+die("Not yet implemented!");+}+staticvoidget_provisional_directory_renames(structmerge_options*opt,unsignedside,int*clean){-die("Not yet implemented!");+structhashmap_iteriter;+structstrmap_entry*entry;+structrename_info*renames=&opt->priv->renames;++compute_rename_counts(&renames->pairs[side],+&renames->dir_rename_count[side],+&renames->dirs_removed[side]);+/*+*Collapse+*dir_rename_count:old_directory->{new_directory->count}+*downto+*dir_renames:old_directory->best_new_directory+*wherebest_new_directoryistheonewiththeuniquehighestcount.+*/+strmap_for_each_entry(&renames->dir_rename_count[side],&iter,entry){+constchar*source_dir=entry->key;+structstrintmap*counts=entry->value;+structhashmap_itercount_iter;+structstrmap_entry*count_entry;+intmax=0;+intbad_max=0;+constchar*best=NULL;++strintmap_for_each_entry(counts,&count_iter,count_entry){+constchar*target_dir=count_entry->key;+intptr_tcount=(intptr_t)count_entry->value;
Just to make sure I understand what's going on here: you're storing the
whole int inside the pointer (and not pointing at it)?
+ if (count == max)
+ bad_max = max;
+ else if (count > max) {
+ max = count;
+ best = target_dir;
+ }
+ }
+
+ if (max == 0)
+ continue;
This is new from merge_recursive.c:get_directory_renames(). What is it
doing here?
+ if (bad_max == max) {
+ path_msg(opt, source_dir, 0,
+ _("CONFLICT (directory rename split): "
+ "Unclear where to rename %s to; it was "
+ "renamed to multiple other directories, with "
+ "no destination getting a majority of the "
+ "files."),
+ source_dir);
+ *clean &= 0;
Also not a big deal, but why not simply '*clean = 0'?
From: Taylor Blau <hidden> Date: 2021-01-18 20:29:35
On Mon, Jan 18, 2021 at 12:15:11PM -0800, Elijah Newren wrote:
If the intent was to keep merge-recursive.c indefinitely, then yes it
would. However, the intent is to (1) avoid touching merge-recursive.c
so I don't destabilize it and so folks can fall back to it, (2) get
merge-ort.c completed, and people to adopt and feel confident in it,
(3) delete merge-recursive.[ch].
This has come up a few other times in a review on the series, because
there are even examples of copied-and-unmodified functions; see
https://lore.kernel.org/git/CABPp-BGtpXRSz+ngFz20j8W4qgpb8juogsLf6HF7b0-Pt=s6=g@mail.gmail.com/
and https://lore.kernel.org/git/CABPp-BEEoqOer11BYrqSVE9E4HcT5MNWcRm7ZHBQ7MVZRUDVXw@mail.gmail.com/.
I know it seems weird to intentionally repeat, but since the goal is
to nuke merge-recursive.c, I'm doing it as a temporary measure.
That all makes sense, and is very helpful information to have. I'll keep
it in mind as I continue to review this series.
Thanks,
Taylor
On Mon, Jan 18, 2021 at 12:10 PM Taylor Blau [off-list ref] wrote:
On Thu, Jan 07, 2021 at 09:35:53PM +0000, Elijah Newren via GitGitGadget wrote:
quoted
From: Elijah Newren <redacted>
This function is based on merge-recursive.c's get_directory_renames(),
except that the first half has been split out into a not-yet-implemented
compute_rename_counts(). The primary difference here is our lack of the
non_unique_new_dir boolean in our strmap. The lack of that field will
at first cause us to fail testcase 2b of t6423; however, future
optimizations will obviate the need for that ugly field so we have just
left it out.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 59 insertions(+), 1 deletion(-)
@@ -721,11 +721,69 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/+staticvoidcompute_rename_counts(structdiff_queue_struct*pairs,+structstrmap*dir_rename_count,+structstrset*dirs_removed)+{+die("Not yet implemented!");+}+staticvoidget_provisional_directory_renames(structmerge_options*opt,unsignedside,int*clean){-die("Not yet implemented!");+structhashmap_iteriter;+structstrmap_entry*entry;+structrename_info*renames=&opt->priv->renames;++compute_rename_counts(&renames->pairs[side],+&renames->dir_rename_count[side],+&renames->dirs_removed[side]);+/*+*Collapse+*dir_rename_count:old_directory->{new_directory->count}+*downto+*dir_renames:old_directory->best_new_directory+*wherebest_new_directoryistheonewiththeuniquehighestcount.+*/+strmap_for_each_entry(&renames->dir_rename_count[side],&iter,entry){+constchar*source_dir=entry->key;+structstrintmap*counts=entry->value;+structhashmap_itercount_iter;+structstrmap_entry*count_entry;+intmax=0;+intbad_max=0;+constchar*best=NULL;++strintmap_for_each_entry(counts,&count_iter,count_entry){+constchar*target_dir=count_entry->key;+intptr_tcount=(intptr_t)count_entry->value;
Just to make sure I understand what's going on here: you're storing the
whole int inside the pointer (and not pointing at it)?
Right, strmap just has (char *)keys and (void *)values. The fact that
strintmap stores integers inside those values instead of pointers
means that I have to do casting. That casting is often hidden behind
strintmap_get() calls, but since I'm not calling such a function here
but just accessing the raw strmap_entry fields, I need to cast myself.
quoted
+ if (count == max)
+ bad_max = max;
+ else if (count > max) {
+ max = count;
+ best = target_dir;
+ }
+ }
+
+ if (max == 0)
+ continue;
This is new from merge_recursive.c:get_directory_renames(). What is it
doing here?
Ah, good catch. That became necessary with an optimization I haven't
submitted yet. I should probably pull these two lines out of this
patch and resubmit later with the other optimization.
quoted
+ if (bad_max == max) {
+ path_msg(opt, source_dir, 0,
+ _("CONFLICT (directory rename split): "
+ "Unclear where to rename %s to; it was "
+ "renamed to multiple other directories, with "
+ "no destination getting a majority of the "
+ "files."),
+ source_dir);
+ *clean &= 0;
Also not a big deal, but why not simply '*clean = 0'?
Good question. There might be a joke in there about this being a
dirty way of recording uncleanness, but what you suggest just looks
cleaner. I'll fix it up.
Ah, answering my onw question: this is indeed shoving the int into your
void*. That said, shouldn't this be '(void*)(intptr_t)best'?
?? best is not an int; it's a directory name.
You are correct that we shove an int into a void* elsewhere;
dir_rename_count is a map of {old_dir => {new_dir => int}} (as noted
in a comment where it was declared).
But this line of code is not for "dir_rename_count" but "dir_renames".
dir_renames is a map of {old_dir => new_dir}. Thus, the value for the
strmap is meant to not be int but char*. Here, best is a const char*
and the cast here is just to remove the constness so I can store in a
strmap -- I know that the strmap isn't going to modify the pointed-to
directory.
From: Taylor Blau <hidden> Date: 2021-01-18 20:38:20
On Thu, Jan 07, 2021 at 09:35:55PM +0000, Elijah Newren via GitGitGadget wrote:
quoted hunk
From: Elijah Newren <redacted>
This function is based on the first half of get_directory_renames() from
merge-recursive.c
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 51 insertions(+), 2 deletions(-)
@@ -721,7 +721,6 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/-MAYBE_UNUSEDstaticvoidget_renamed_dir_portion(constchar*old_path,constchar*new_path,char**old_dir,char**new_dir){
@@ -825,11 +824,61 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,*new_dir=xstrndup(new_path,end_of_new-new_path);}+staticvoidincrement_count(structstrmap*dir_rename_count,+char*old_dir,+char*new_dir)+{+structstrintmap*counts;+structstrmap_entry*e;++/* Get the {new_dirs -> counts} mapping using old_dir */+e=strmap_get_entry(dir_rename_count,old_dir);+if(e){+counts=e->value;+}else{+counts=xmalloc(sizeof(*counts));+strintmap_init_with_options(counts,0,NULL,1);+strmap_put(dir_rename_count,old_dir,counts);+}++/* Increment the count for new_dir */+strintmap_incr(counts,new_dir,1);+}+staticvoidcompute_rename_counts(structdiff_queue_struct*pairs,structstrmap*dir_rename_count,structstrset*dirs_removed){-die("Not yet implemented!");+inti;++for(i=0;i<pairs->nr;++i){+char*old_dir,*new_dir;+structdiff_filepair*pair=pairs->queue[i];++if(pair->status!='R')+continue;
This had a useful comment in merge-recursive.c that I think would help
future readers here. Would you consider brining over the comment that
starts with "File not part of directory rename ..." here?
+ /* Get the old and new directory names */
+ get_renamed_dir_portion(pair->one->path, pair->two->path,
+ &old_dir, &new_dir);
This spacing is a little odd to me, but it does come directly from
merge-recursive.c. I don't feel strongly about it.
+ if (!old_dir)
+ /* Directory didn't change at all; ignore this one. */
+ continue;
+
+ /*
+ * Make dir_rename_count contain a map of a map:
+ * old_directory -> {new_directory -> count}
+ * In other words, for every pair look at the directories for
+ * the old filename and the new filename and count how many
+ * times that pairing occurs.
+ */
+ if (strset_contains(dirs_removed, old_dir))
+ increment_count(dir_rename_count, old_dir, new_dir);
Much clearer. If you're rerolling anyway, it may be worth it to say
something about this movement in the patch message: "along the way,
factor out the routine to update the bookkeeping to track the number of
items renamed into new directories".
+ /* Free resources we don't need anymore */
+ free(old_dir);
+ free(new_dir);
+ }
And obviously this is new, but it makes sense here. Thanks.
Thanks,
Taylor
On Mon, Jan 18, 2021 at 12:36 PM Taylor Blau [off-list ref] wrote:
On Thu, Jan 07, 2021 at 09:35:55PM +0000, Elijah Newren via GitGitGadget wrote:
quoted
From: Elijah Newren <redacted>
This function is based on the first half of get_directory_renames() from
merge-recursive.c
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 51 insertions(+), 2 deletions(-)
@@ -721,7 +721,6 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/-MAYBE_UNUSEDstaticvoidget_renamed_dir_portion(constchar*old_path,constchar*new_path,char**old_dir,char**new_dir){
@@ -825,11 +824,61 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,*new_dir=xstrndup(new_path,end_of_new-new_path);}+staticvoidincrement_count(structstrmap*dir_rename_count,+char*old_dir,+char*new_dir)+{+structstrintmap*counts;+structstrmap_entry*e;++/* Get the {new_dirs -> counts} mapping using old_dir */+e=strmap_get_entry(dir_rename_count,old_dir);+if(e){+counts=e->value;+}else{+counts=xmalloc(sizeof(*counts));+strintmap_init_with_options(counts,0,NULL,1);+strmap_put(dir_rename_count,old_dir,counts);+}++/* Increment the count for new_dir */+strintmap_incr(counts,new_dir,1);+}+staticvoidcompute_rename_counts(structdiff_queue_struct*pairs,structstrmap*dir_rename_count,structstrset*dirs_removed){-die("Not yet implemented!");+inti;++for(i=0;i<pairs->nr;++i){+char*old_dir,*new_dir;+structdiff_filepair*pair=pairs->queue[i];++if(pair->status!='R')+continue;
This had a useful comment in merge-recursive.c that I think would help
future readers here. Would you consider brining over the comment that
starts with "File not part of directory rename ..." here?
Sure, will do.
quoted
+ /* Get the old and new directory names */
+ get_renamed_dir_portion(pair->one->path, pair->two->path,
+ &old_dir, &new_dir);
This spacing is a little odd to me, but it does come directly from
merge-recursive.c. I don't feel strongly about it.
quoted
+ if (!old_dir)
+ /* Directory didn't change at all; ignore this one. */
+ continue;
+
+ /*
+ * Make dir_rename_count contain a map of a map:
+ * old_directory -> {new_directory -> count}
+ * In other words, for every pair look at the directories for
+ * the old filename and the new filename and count how many
+ * times that pairing occurs.
+ */
+ if (strset_contains(dirs_removed, old_dir))
+ increment_count(dir_rename_count, old_dir, new_dir);
Much clearer. If you're rerolling anyway, it may be worth it to say
something about this movement in the patch message: "along the way,
factor out the routine to update the bookkeeping to track the number of
items renamed into new directories".
Sure, makes sense.
quoted
+ /* Free resources we don't need anymore */
+ free(old_dir);
+ free(new_dir);
+ }
And obviously this is new, but it makes sense here. Thanks.
Thanks,
Taylor
From: Taylor Blau <hidden> Date: 2021-01-18 21:11:23
On Thu, Jan 07, 2021 at 09:35:56PM +0000, Elijah Newren via GitGitGadget wrote:
From: Elijah Newren <redacted>
This is modelled on the version of handle_directory_level_conflicts()
from merge-recursive.c, but is massively simplified due to the following
factors:
* strmap API provides simplifications over using direct hashamp
* we have a dirs_removed field in struct rename_info that we have an
easy way to populate from collect_merge_info(); this was already
used in compute_rename_counts() and thus we do not need to check
for condition #2.
* The removal of condition #2 by handling it earlier in the code also
obviates the need to check for condition #3 -- if both sides renamed
a directory, meaning that the directory no longer exists on either
side, then neither side could have added any new files to that
directory, and thus there are no files whose locations we need to
move due to such a directory rename.
In fact, the same logic that makes condition #3 irrelevant means
condition #1 is also irrelevant so we could drop this function.
However, it is cheap to check if both sides rename the same directory,
and doing so can save future computation. So, simply remove any
directories that both sides renamed from the list of directory renames.
On Mon, Jan 18, 2021 at 1:00 PM Taylor Blau [off-list ref] wrote:
On Thu, Jan 07, 2021 at 09:35:56PM +0000, Elijah Newren via GitGitGadget wrote:
quoted
From: Elijah Newren <redacted>
This is modelled on the version of handle_directory_level_conflicts()
from merge-recursive.c, but is massively simplified due to the following
factors:
* strmap API provides simplifications over using direct hashamp
Whoops, should be "hashmap"
quoted
* we have a dirs_removed field in struct rename_info that we have an
easy way to populate from collect_merge_info(); this was already
used in compute_rename_counts() and thus we do not need to check
for condition #2.
* The removal of condition #2 by handling it earlier in the code also
obviates the need to check for condition #3 -- if both sides renamed
a directory, meaning that the directory no longer exists on either
side, then neither side could have added any new files to that
directory, and thus there are no files whose locations we need to
move due to such a directory rename.
In fact, the same logic that makes condition #3 irrelevant means
condition #1 is also irrelevant so we could drop this function.
However, it is cheap to check if both sides rename the same directory,
and doing so can save future computation. So, simply remove any
directories that both sides renamed from the list of directory renames.
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-19 19:55:03
This series depends on a merge of en/merge-ort-3 (en/merge-ort-recursive has
since merged to master and need not be listed as a dependent series
anymore).
This series mostly implements directory rename detection for merge-ort; ;
I'll cover the "mostly" bit below. If one merges this series with
en/ort-conflict-handling, then this series drops the number of failing tests
in the testsuite under GIT_TEST_MERGE_ALGORITHM=ort from 60 down to 8.
As noted in the earlier submissions, most all the logic is copied from
merge-recursive.c, with numerous but minor modifications due to differences
in data structures. The final patch, however, is new and fixes a known bug
in merge-recursive (showcased by testcase 12f of t6423).
Changes since v2:
* numerous small fixups highlighted by Taylor in his reviews
Elijah Newren (17):
merge-ort: add new data structures for directory rename detection
merge-ort: initialize and free new directory rename data structures
merge-ort: collect which directories are removed in dirs_removed
merge-ort: add outline for computing directory renames
merge-ort: add outline of get_provisional_directory_renames()
merge-ort: copy get_renamed_dir_portion() from merge-recursive.c
merge-ort: implement compute_rename_counts()
merge-ort: implement handle_directory_level_conflicts()
merge-ort: modify collect_renames() for directory rename handling
merge-ort: implement compute_collisions()
merge-ort: implement apply_dir_rename() and check_dir_renamed()
merge-ort: implement check_for_directory_rename()
merge-ort: implement handle_path_level_conflicts()
merge-ort: add a new toplevel_dir field
merge-ort: implement apply_directory_rename_modifications()
merge-ort: process_renames() now needs more defensiveness
merge-ort: fix a directory rename detection bug
merge-ort.c | 830 ++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 811 insertions(+), 19 deletions(-)
base-commit: 8f894b22636d5d0cdfca0ae5fd88d327cc3349b3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-835%2Fnewren%2Fort-directory-renames-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-835/newren/ort-directory-renames-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/835
Range-diff vs v2:
1: 41a99640cc5 = 1: 3b14afd4129 merge-ort: add new data structures for directory rename detection
2: 762151802be = 2: 9da04816c3b merge-ort: initialize and free new directory rename data structures
3: bb4b6d20480 = 3: 3061a6ae69c merge-ort: collect which directories are removed in dirs_removed
4: ccb30dfc3c4 ! 4: 0dd6e83d2b0 merge-ort: add outline for computing directory renames
@@ merge-ort.c: static int detect_and_process_renames(struct merge_options *opt,
+ opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT);
+
+ if (need_dir_renames) {
-+ for (s = MERGE_SIDE1; s <= MERGE_SIDE2; s++)
-+ get_provisional_directory_renames(opt, s, &clean);
++ get_provisional_directory_renames(opt, MERGE_SIDE1, &clean);
++ get_provisional_directory_renames(opt, MERGE_SIDE2, &clean);
+ handle_directory_level_conflicts(opt);
+ }
+
5: bb4285250cd ! 5: b008ba35712 merge-ort: add outline of get_provisional_directory_renames()
@@ merge-ort.c: static int handle_content_merge(struct merge_options *opt,
+ }
+ }
+
-+ if (max == 0)
-+ continue;
-+
+ if (bad_max == max) {
+ path_msg(opt, source_dir, 0,
+ _("CONFLICT (directory rename split): "
@@ merge-ort.c: static int handle_content_merge(struct merge_options *opt,
+ "no destination getting a majority of the "
+ "files."),
+ source_dir);
-+ *clean &= 0;
++ *clean = 0;
+ } else {
+ strmap_put(&renames->dir_renames[side],
+ source_dir, (void*)best);
6: 4e79a96ba1c = 6: 54c18505706 merge-ort: copy get_renamed_dir_portion() from merge-recursive.c
7: 1e48cde01b9 ! 7: 5b5c8368174 merge-ort: implement compute_rename_counts()
@@ Commit message
merge-ort: implement compute_rename_counts()
This function is based on the first half of get_directory_renames() from
- merge-recursive.c
+ merge-recursive.c; as part of the implementation, factor out a routine,
+ increment_count(), to update the bookkeeping to track the number of
+ items renamed into new directories.
Signed-off-by: Elijah Newren [off-list ref]
@@ merge-ort.c: static void get_renamed_dir_portion(const char *old_path, const cha
+ char *old_dir, *new_dir;
+ struct diff_filepair *pair = pairs->queue[i];
+
++ /* File not part of directory rename if it wasn't renamed */
+ if (pair->status != 'R')
+ continue;
+
8: f6efa4350d6 ! 8: cafff61893a merge-ort: implement handle_directory_level_conflicts()
@@ Commit message
This is modelled on the version of handle_directory_level_conflicts()
from merge-recursive.c, but is massively simplified due to the following
factors:
- * strmap API provides simplifications over using direct hashamp
+ * strmap API provides simplifications over using direct hashmap
* we have a dirs_removed field in struct rename_info that we have an
easy way to populate from collect_merge_info(); this was already
used in compute_rename_counts() and thus we do not need to check
@@ merge-ort.c: static void get_provisional_directory_renames(struct merge_options
+ struct hashmap_iter iter;
+ struct strmap_entry *entry;
+ struct string_list duplicated = STRING_LIST_INIT_NODUP;
-+ struct strmap *side1_dir_renames = &opt->priv->renames.dir_renames[1];
-+ struct strmap *side2_dir_renames = &opt->priv->renames.dir_renames[2];
++ struct rename_info *renames = &opt->priv->renames;
++ struct strmap *side1_dir_renames = &renames->dir_renames[MERGE_SIDE1];
++ struct strmap *side2_dir_renames = &renames->dir_renames[MERGE_SIDE2];
+ int i;
+
+ strmap_for_each_entry(side1_dir_renames, &iter, entry) {
@@ merge-ort.c: static void get_provisional_directory_renames(struct merge_options
+ string_list_append(&duplicated, entry->key);
+ }
+
-+ for (i=0; i<duplicated.nr; ++i) {
++ for (i = 0; i < duplicated.nr; i++) {
+ strmap_remove(side1_dir_renames, duplicated.items[i].string, 0);
+ strmap_remove(side2_dir_renames, duplicated.items[i].string, 0);
+ }
9: bdd9d6cd702 = 9: e9e621a0b70 merge-ort: modify collect_renames() for directory rename handling
10: 9a06c698857 = 10: 28ae21bcb46 merge-ort: implement compute_collisions()
11: 2ffb93c37ac = 11: 95f6e119072 merge-ort: implement apply_dir_rename() and check_dir_renamed()
12: cbfdf4d9ba0 = 12: ad08c6ece4d merge-ort: implement check_for_directory_rename()
13: 734891cb315 = 13: 1bece72e37a merge-ort: implement handle_path_level_conflicts()
14: 4b912f2c025 = 14: ee1398e3793 merge-ort: add a new toplevel_dir field
15: d74417e86c5 = 15: ef86b7c07e3 merge-ort: implement apply_directory_rename_modifications()
16: 11e45af831d = 16: f1c7ce8123f merge-ort: process_renames() now needs more defensiveness
17: 551878bd84d ! 17: 7c24f9f7aed merge-ort: fix a directory rename detection bug
@@ merge-ort.c: static void compute_rename_counts(struct diff_queue_struct *pairs,
- char *old_dir, *new_dir;
struct diff_filepair *pair = pairs->queue[i];
+ /* File not part of directory rename if it wasn't renamed */
if (pair->status != 'R')
continue;
--
gitgitgadget
@@ -721,6 +721,110 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/+MAYBE_UNUSED+staticvoidget_renamed_dir_portion(constchar*old_path,constchar*new_path,+char**old_dir,char**new_dir)+{+char*end_of_old,*end_of_new;++/* Default return values: NULL, meaning no rename */+*old_dir=NULL;+*new_dir=NULL;++/*+*For+*"a/b/c/d/e/foo.c"->"a/b/some/thing/else/e/foo.c"+*the"e/foo.c"partisthesame,wejustwanttoknowthat+*"a/b/c/d"wasrenamedto"a/b/some/thing/else"+*so,forthisexample,thisfunctionreturns"a/b/c/d"in+**old_dirand"a/b/some/thing/else"in*new_dir.+*/++/*+*Ifthebasenameofthefilechanged,wedon'tcare.Wewant+*toknowwhichportionofthedirectory,ifany,changed.+*/+end_of_old=strrchr(old_path,'/');+end_of_new=strrchr(new_path,'/');++/*+*Ifend_of_oldisNULL,old_pathwasn'tinadirectory,sothere+*couldnotbeadirectoryrename(ourruleelsewherethata+*directorywhichstillexistsisnotconsideredtohavebeen+*renamedmeanstherootdirectorycanneverberenamed--because+*therootdirectoryalwaysexists).+*/+if(end_of_old==NULL)+return;/* Note: *old_dir and *new_dir are still NULL */++/*+*Ifnew_pathcontainsnodirectory(end_of_newisNULL),thenwe+*havearenameofold_path'sdirectorytotherootdirectory.+*/+if(end_of_new==NULL){+*old_dir=xstrndup(old_path,end_of_old-old_path);+*new_dir=xstrdup("");+return;+}++/* Find the first non-matching character traversing backwards */+while(*--end_of_new==*--end_of_old&&+end_of_old!=old_path&&+end_of_new!=new_path)+;/* Do nothing; all in the while loop */++/*+*Ifbothgotbacktothebeginningoftheirstrings,thenthe+*directorydidn'tchangeatall,onlythebasenamedid.+*/+if(end_of_old==old_path&&end_of_new==new_path&&+*end_of_old==*end_of_new)+return;/* Note: *old_dir and *new_dir are still NULL */++/*+*Ifend_of_newgotbacktothebeginningofitsstring,and+*end_of_oldgotbacktothebeginningofsomesubdirectory,then+*wehavearename/mergeofasubdirectoryintotheroot,which+*needsslightlyspecialhandling.+*+*Note:Thereisnoneedtoconsidertheoppositecase,witha+*rename/mergeoftherootdirectoryintosomesubdirectory+*becauseasnotedabovetherootdirectoryalwaysexistssoit+*cannotbeconsideredtoberenamed.+*/+if(end_of_new==new_path&&+end_of_old!=old_path&&end_of_old[-1]=='/'){+*old_dir=xstrndup(old_path,--end_of_old-old_path);+*new_dir=xstrdup("");+return;+}++/*+*We'vefoundthefirstnon-matchingcharacterinthedirectory+*paths.Thatmeansthecurrentcharacterswewerelookingat+*werepartofthefirstnon-matchingsubdirnamegoingbackfrom+*theendofthestrings.Getthewholenamebyadvancingboth+*end_of_oldandend_of_newtotheNEXT'/'character.Thatwill+*representtheentiredirectoryrename.+*+*Thereasonfortheincrementiscaseslike+*a/b/star/foo/whatever.c->a/b/tar/foo/random.c+*Afterdroppingthebasenameandgoingbacktothefirst+*non-matchingcharacter,we'renowcomparing:+*a/b/sanda/b/+*andwewanttobecomparing:+*a/b/star/anda/b/tar/+*butwithoutthepre-increment,theoneontherightwouldstay+*a/b/.+*/+end_of_old=strchr(++end_of_old,'/');+end_of_new=strchr(++end_of_new,'/');++/* Copy the old and new directories into *old_dir and *new_dir. */+*old_dir=xstrndup(old_path,end_of_old-old_path);+*new_dir=xstrndup(new_path,end_of_new-new_path);+}+staticvoidcompute_rename_counts(structdiff_queue_struct*pairs,structstrmap*dir_rename_count,structstrset*dirs_removed)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-19 19:55:59
From: Elijah Newren <redacted>
Port some directory rename handling changes from merge-recursive.c's
detect_and_process_renames() to the same-named function of merge-ort.c.
This does not yet add any use or handling of directory renames, just the
outline for where we start to compute them. Thus, a future patch will
add port additional changes to merge-ort's detect_and_process_renames().
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
@@ -721,6 +721,18 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/+staticvoidget_provisional_directory_renames(structmerge_options*opt,+unsignedside,+int*clean)+{+die("Not yet implemented!");+}++staticvoidhandle_directory_level_conflicts(structmerge_options*opt)+{+die("Not yet implemented!");+}+/*** Function Grouping: functions related to regular rename detection ***/staticintprocess_renames(structmerge_options*opt,
@@ -1086,13 +1098,24 @@ static int detect_and_process_renames(struct merge_options *opt,{structdiff_queue_structcombined;structrename_info*renames=&opt->priv->renames;-ints,clean=1;+intneed_dir_renames,s,clean=1;memset(&combined,0,sizeof(combined));detect_regular_renames(opt,merge_base,side1,MERGE_SIDE1);detect_regular_renames(opt,merge_base,side2,MERGE_SIDE2);+need_dir_renames=+!opt->priv->call_depth&&+(opt->detect_directory_renames==MERGE_DIRECTORY_RENAMES_TRUE||+opt->detect_directory_renames==MERGE_DIRECTORY_RENAMES_CONFLICT);++if(need_dir_renames){+get_provisional_directory_renames(opt,MERGE_SIDE1,&clean);+get_provisional_directory_renames(opt,MERGE_SIDE2,&clean);+handle_directory_level_conflicts(opt);+}+ALLOC_GROW(combined.queue,renames->pairs[1].nr+renames->pairs[2].nr,combined.alloc);
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-19 19:56:04
From: Elijah Newren <redacted>
This function is based on merge-recursive.c's get_directory_renames(),
except that the first half has been split out into a not-yet-implemented
compute_rename_counts(). The primary difference here is our lack of the
non_unique_new_dir boolean in our strmap. The lack of that field will
at first cause us to fail testcase 2b of t6423; however, future
optimizations will obviate the need for that ugly field so we have just
left it out.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 56 insertions(+), 1 deletion(-)
@@ -721,11 +721,66 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/+staticvoidcompute_rename_counts(structdiff_queue_struct*pairs,+structstrmap*dir_rename_count,+structstrset*dirs_removed)+{+die("Not yet implemented!");+}+staticvoidget_provisional_directory_renames(structmerge_options*opt,unsignedside,int*clean){-die("Not yet implemented!");+structhashmap_iteriter;+structstrmap_entry*entry;+structrename_info*renames=&opt->priv->renames;++compute_rename_counts(&renames->pairs[side],+&renames->dir_rename_count[side],+&renames->dirs_removed[side]);+/*+*Collapse+*dir_rename_count:old_directory->{new_directory->count}+*downto+*dir_renames:old_directory->best_new_directory+*wherebest_new_directoryistheonewiththeuniquehighestcount.+*/+strmap_for_each_entry(&renames->dir_rename_count[side],&iter,entry){+constchar*source_dir=entry->key;+structstrintmap*counts=entry->value;+structhashmap_itercount_iter;+structstrmap_entry*count_entry;+intmax=0;+intbad_max=0;+constchar*best=NULL;++strintmap_for_each_entry(counts,&count_iter,count_entry){+constchar*target_dir=count_entry->key;+intptr_tcount=(intptr_t)count_entry->value;++if(count==max)+bad_max=max;+elseif(count>max){+max=count;+best=target_dir;+}+}++if(bad_max==max){+path_msg(opt,source_dir,0,+_("CONFLICT (directory rename split): "+"Unclear where to rename %s to; it was "+"renamed to multiple other directories, with "+"no destination getting a majority of the "+"files."),+source_dir);+*clean=0;+}else{+strmap_put(&renames->dir_renames[side],+source_dir,(void*)best);+}+}}staticvoidhandle_directory_level_conflicts(structmerge_options*opt)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-19 19:56:11
From: Elijah Newren <redacted>
This is nearly a wholesale copy of compute_collisions() from
merge-recursive.c, and the logic remains the same, but it has been
tweaked slightly due to:
* using strmap.h API (instead of direct hashmaps)
* allocation/freeing of data structures were done separately in
merge_start() and clear_or_reinit_internal_opts() in an earlier
patch in this series
* there is no non_unique_new_dir data field in merge-ort; that will
be handled a different way
It does depend on two new functions, apply_dir_rename() and
check_dir_renamed() which were introduced with simple
die-not-yet-implemented shells and will be implemented in subsequent
patches.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 67 insertions(+), 1 deletion(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-19 19:56:40
From: Elijah Newren <redacted>
This function roughly follows the same outline as the function of the
same name from merge-recursive.c, but the code diverges in multiple
ways due to some special considerations:
* merge-ort's version needs to update opt->priv->paths with any new
paths (and opt->priv->paths points to struct conflict_infos which
track quite a bit of metadata for each path); merge-recursive's
version would directly update the index
* merge-ort requires that opt->priv->paths has any leading directories
of any relevant files also be included in the set of paths. And
due to pointer equality requirements on merged_info.directory_name,
we have to be careful how we compute and insert these.
* due to the above requirements on opt->priv->paths, merge-ort's
version starts with a long comment to explain all the special
considerations that need to be handled
* merge-ort can use the full data stored in opt->priv->paths to avoid
making expensive get_tree_entry() calls to regather the necessary
data.
* due to messages being deferred automatically in merge-ort, this is
the best place to handle conflict messages whereas in
merge-recursive.c they are deferred manually so that processing of
entries does all the printing
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 168 +++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 167 insertions(+), 1 deletion(-)
@@ -1223,7 +1223,173 @@ static void apply_directory_rename_modifications(struct merge_options *opt,structdiff_filepair*pair,char*new_path){-die("Not yet implemented.");+/*+*Thebasicideaistogettheconflict_infofromopt->priv->paths+*atoldpath,andinsertitintonew_path;basicallyjustthis:+*ci=strmap_get(&opt->priv->paths,old_path);+*strmap_remove(&opt->priv->paths,old_path,0);+*strmap_put(&opt->priv->paths,new_path,ci);+*However,therearesomefactorscomplicatingthis:+*-opt->priv->pathsmayalreadyhaveanentryatnew_path+*-Eachcitracksitscontainingdirectory,soweneedto+*updatethat+*-Ifanothercihasthesamecontainingdirectory,then+*thetwochar*'sMUSTpointtothesamelocation.Seethe+*commentinstructmerged_info.strcmpequalityisnot+*enough;weneedpointerequality.+*-opt->priv->pathsmustholdtheparentdirectoriesofany+*entriesthatareadded.So,ifthisdirectoryrename+*causesentirelynewdirectories,wemustrecursivelyadd+*parentdirectories.+*-Foreachparentdirectoryaddedtoopt->priv->paths,we+*alsoneedtogetitsparentdirectorystoredinits+*conflict_info->merged.directory_namewithallthesame+*requirementsaboutpointerequality.+*/+structstring_listdirs_to_insert=STRING_LIST_INIT_NODUP;+structconflict_info*ci,*new_ci;+structstrmap_entry*entry;+constchar*branch_with_new_path,*branch_with_dir_rename;+constchar*old_path=pair->two->path;+constchar*parent_name;+constchar*cur_path;+inti,len;++entry=strmap_get_entry(&opt->priv->paths,old_path);+old_path=entry->key;+ci=entry->value;+VERIFY_CI(ci);++/* Find parent directories missing from opt->priv->paths */+cur_path=new_path;+while(1){+/* Find the parent directory of cur_path */+char*last_slash=strrchr(cur_path,'/');+if(last_slash){+parent_name=xstrndup(cur_path,last_slash-cur_path);+}else{+parent_name=opt->priv->toplevel_dir;+break;+}++/* Look it up in opt->priv->paths */+entry=strmap_get_entry(&opt->priv->paths,parent_name);+if(entry){+free((char*)parent_name);+parent_name=entry->key;/* reuse known pointer */+break;+}++/* Record this is one of the directories we need to insert */+string_list_append(&dirs_to_insert,parent_name);+cur_path=parent_name;+}++/* Traverse dirs_to_insert and insert them into opt->priv->paths */+for(i=dirs_to_insert.nr-1;i>=0;--i){+structconflict_info*dir_ci;+char*cur_dir=dirs_to_insert.items[i].string;++dir_ci=xcalloc(1,sizeof(*dir_ci));++dir_ci->merged.directory_name=parent_name;+len=strlen(parent_name);+/* len+1 because of trailing '/' character */+dir_ci->merged.basename_offset=(len>0?len+1:len);+dir_ci->dirmask=ci->filemask;+strmap_put(&opt->priv->paths,cur_dir,dir_ci);++parent_name=cur_dir;+}++/*+*Weareremovingold_pathfromopt->priv->paths.old_pathalsowill+*eventuallyneedtobefreed,butitmaystillbeusedbye.g.+*ci->pathnames.So,storeitinanotherstring-listfornow.+*/+string_list_append(&opt->priv->paths_to_free,old_path);++assert(ci->filemask==2||ci->filemask==4);+assert(ci->dirmask==0);+strmap_remove(&opt->priv->paths,old_path,0);++branch_with_new_path=(ci->filemask==2)?opt->branch1:opt->branch2;+branch_with_dir_rename=(ci->filemask==2)?opt->branch2:opt->branch1;++/* Now, finally update ci and stick it into opt->priv->paths */+ci->merged.directory_name=parent_name;+len=strlen(parent_name);+ci->merged.basename_offset=(len>0?len+1:len);+new_ci=strmap_get(&opt->priv->paths,new_path);+if(!new_ci){+/* Place ci back into opt->priv->paths, but at new_path */+strmap_put(&opt->priv->paths,new_path,ci);+}else{+intindex;++/* A few sanity checks */+VERIFY_CI(new_ci);+assert(ci->filemask==2||ci->filemask==4);+assert((new_ci->filemask&ci->filemask)==0);+assert(!new_ci->merged.clean);++/* Copy stuff from ci into new_ci */+new_ci->filemask|=ci->filemask;+if(new_ci->dirmask)+new_ci->df_conflict=1;+index=(ci->filemask>>1);+new_ci->pathnames[index]=ci->pathnames[index];+new_ci->stages[index].mode=ci->stages[index].mode;+oidcpy(&new_ci->stages[index].oid,&ci->stages[index].oid);++free(ci);+ci=new_ci;+}++if(opt->detect_directory_renames==MERGE_DIRECTORY_RENAMES_TRUE){+/* Notify user of updated path */+if(pair->status=='A')+path_msg(opt,new_path,1,+_("Path updated: %s added in %s inside a "+"directory that was renamed in %s; moving "+"it to %s."),+old_path,branch_with_new_path,+branch_with_dir_rename,new_path);+else+path_msg(opt,new_path,1,+_("Path updated: %s renamed to %s in %s, "+"inside a directory that was renamed in %s; "+"moving it to %s."),+pair->one->path,old_path,branch_with_new_path,+branch_with_dir_rename,new_path);+}else{+/*+*opt->detect_directory_renameshasthevalue+*MERGE_DIRECTORY_RENAMES_CONFLICT,somarktheseasconflicts.+*/+ci->path_conflict=1;+if(pair->status=='A')+path_msg(opt,new_path,0,+_("CONFLICT (file location): %s added in %s "+"inside a directory that was renamed in %s, "+"suggesting it should perhaps be moved to "+"%s."),+old_path,branch_with_new_path,+branch_with_dir_rename,new_path);+else+path_msg(opt,new_path,0,+_("CONFLICT (file location): %s renamed to %s "+"in %s, inside a directory that was renamed "+"in %s, suggesting it should perhaps be "+"moved to %s."),+pair->one->path,old_path,branch_with_new_path,+branch_with_dir_rename,new_path);+}++/*+*Finally,recordthenewlocation.+*/+pair->two->path=new_path;}/*** Function Grouping: functions related to regular rename detection ***/
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-19 19:56:46
From: Elijah Newren <redacted>
As noted in commit 902c521a35 ("t6423: more involved directory rename
test", 2020-10-15), when we have a case where
* dir/subdir/ has several files
* almost all files in dir/subdir/ are renamed to folder/subdir/
* one of the files in dir/subdir/ is renamed to folder/subdir/newsubdir/
* the other side of history (that doesn't do the renames) adds a
new file to dir/subdir/
Then for the majority of the file renames, the directory rename of
dir/subdir/ -> folder/subdir/
is actually not represented that way but as
dir/ -> folder/
We also had one rename that was represented as
dir/subdir/ -> folder/subdir/newsubdir/
Now, since there's a new file in dir/subdir/, where does it go? Well,
there's only one rule for dir/subdir/, so the code previously noted that
this rule had the "majority" of the one "relevant" rename and thus
erroneously used it to place the file in folder/subdir/newsubdir/. We
really want the heavy weight associated with dir/ -> folder/ to also be
treated as dir/subdir/ -> folder/subdir/, so that we correctly place the
file in folder/subdir/.
Add a bunch of logic to make sure that we use all relevant renamings in
directory rename detection.
Note that testcase 12f of t6423 still fails after this, but it gets
further than merge-recursive does. There are some performance related
bits in that testcase (the region_enter messages) that do not yet
succeed, but the rest of the testcase works after this patch.
Subsequent patch series will fix up the performance side.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 198 +++++++++++++++++++++-------------------------------
1 file changed, 81 insertions(+), 117 deletions(-)
@@ -764,109 +764,6 @@ static char *apply_dir_rename(struct strmap_entry *rename_info,returnstrbuf_detach(&new_path,NULL);}-staticvoidget_renamed_dir_portion(constchar*old_path,constchar*new_path,-char**old_dir,char**new_dir)-{-char*end_of_old,*end_of_new;--/* Default return values: NULL, meaning no rename */-*old_dir=NULL;-*new_dir=NULL;--/*-*For-*"a/b/c/d/e/foo.c"->"a/b/some/thing/else/e/foo.c"-*the"e/foo.c"partisthesame,wejustwanttoknowthat-*"a/b/c/d"wasrenamedto"a/b/some/thing/else"-*so,forthisexample,thisfunctionreturns"a/b/c/d"in-**old_dirand"a/b/some/thing/else"in*new_dir.-*/--/*-*Ifthebasenameofthefilechanged,wedon'tcare.Wewant-*toknowwhichportionofthedirectory,ifany,changed.-*/-end_of_old=strrchr(old_path,'/');-end_of_new=strrchr(new_path,'/');--/*-*Ifend_of_oldisNULL,old_pathwasn'tinadirectory,sothere-*couldnotbeadirectoryrename(ourruleelsewherethata-*directorywhichstillexistsisnotconsideredtohavebeen-*renamedmeanstherootdirectorycanneverberenamed--because-*therootdirectoryalwaysexists).-*/-if(end_of_old==NULL)-return;/* Note: *old_dir and *new_dir are still NULL */--/*-*Ifnew_pathcontainsnodirectory(end_of_newisNULL),thenwe-*havearenameofold_path'sdirectorytotherootdirectory.-*/-if(end_of_new==NULL){-*old_dir=xstrndup(old_path,end_of_old-old_path);-*new_dir=xstrdup("");-return;-}--/* Find the first non-matching character traversing backwards */-while(*--end_of_new==*--end_of_old&&-end_of_old!=old_path&&-end_of_new!=new_path)-;/* Do nothing; all in the while loop */--/*-*Ifbothgotbacktothebeginningoftheirstrings,thenthe-*directorydidn'tchangeatall,onlythebasenamedid.-*/-if(end_of_old==old_path&&end_of_new==new_path&&-*end_of_old==*end_of_new)-return;/* Note: *old_dir and *new_dir are still NULL */--/*-*Ifend_of_newgotbacktothebeginningofitsstring,and-*end_of_oldgotbacktothebeginningofsomesubdirectory,then-*wehavearename/mergeofasubdirectoryintotheroot,which-*needsslightlyspecialhandling.-*-*Note:Thereisnoneedtoconsidertheoppositecase,witha-*rename/mergeoftherootdirectoryintosomesubdirectory-*becauseasnotedabovetherootdirectoryalwaysexistssoit-*cannotbeconsideredtoberenamed.-*/-if(end_of_new==new_path&&-end_of_old!=old_path&&end_of_old[-1]=='/'){-*old_dir=xstrndup(old_path,--end_of_old-old_path);-*new_dir=xstrdup("");-return;-}--/*-*We'vefoundthefirstnon-matchingcharacterinthedirectory-*paths.Thatmeansthecurrentcharacterswewerelookingat-*werepartofthefirstnon-matchingsubdirnamegoingbackfrom-*theendofthestrings.Getthewholenamebyadvancingboth-*end_of_oldandend_of_newtotheNEXT'/'character.Thatwill-*representtheentiredirectoryrename.-*-*Thereasonfortheincrementiscaseslike-*a/b/star/foo/whatever.c->a/b/tar/foo/random.c-*Afterdroppingthebasenameandgoingbacktothefirst-*non-matchingcharacter,we'renowcomparing:-*a/b/sanda/b/-*andwewanttobecomparing:-*a/b/star/anda/b/tar/-*butwithoutthepre-increment,theoneontherightwouldstay-*a/b/.-*/-end_of_old=strchr(++end_of_old,'/');-end_of_new=strchr(++end_of_new,'/');--/* Copy the old and new directories into *old_dir and *new_dir. */-*old_dir=xstrndup(old_path,end_of_old-old_path);-*new_dir=xstrndup(new_path,end_of_new-new_path);-}-staticintpath_in_way(structstrmap*paths,constchar*path,unsignedside_mask){structmerged_info*mi=strmap_get(paths,path);
@@ -973,6 +878,76 @@ static void increment_count(struct strmap *dir_rename_count,strintmap_incr(counts,new_dir,1);}+staticvoidupdate_dir_rename_counts(structstrmap*dir_rename_count,+structstrset*dirs_removed,+constchar*oldname,+constchar*newname)+{+char*old_dir=xstrdup(oldname);+char*new_dir=xstrdup(newname);+charnew_dir_first_char=new_dir[0];+intfirst_time_in_loop=1;++while(1){+dirname_munge(old_dir);+dirname_munge(new_dir);++/*+*Whenrenaming+*"a/b/c/d/e/foo.c"->"a/b/some/thing/else/e/foo.c"+*thenthissuggeststhatboth+*a/b/c/d/e/=>a/b/some/thing/else/e/+*a/b/c/d/=>a/b/some/thing/else/+*sowewanttoincrementcountersforboth.WedoNOT,+*however,alsowanttosuggestthattherewasthefollowing+*rename:+*a/b/c/=>a/b/some/thing/+*soweneedtoquitatthatpoint.+*+*Notethewhenfirst_time_in_loop,weonlystripoffthe+*basename,andwedon'tcareifthat'sdifferent.+*/+if(!first_time_in_loop){+char*old_sub_dir=strchr(old_dir,'\0')+1;+char*new_sub_dir=strchr(new_dir,'\0')+1;+if(!*new_dir){+/*+*Specialcasewhenrenamingtorootdirectory,+*i.e.whennew_dir=="".Inthiscase,wehad+*somethinglike+*a/b/subdir=>subdir+*andsodirname_munge()setsthingsupsothat+*old_dir="a/b\0subdir\0"+*new_dir="\0ubdir\0"+*Wedidn'thavea'/'tooverwritea'\0'onto+*innew_dir,sowehavetocomparedifferently.+*/+if(new_dir_first_char!=old_sub_dir[0]||+strcmp(old_sub_dir+1,new_sub_dir))+break;+}else{+if(strcmp(old_sub_dir,new_sub_dir))+break;+}+}++if(strset_contains(dirs_removed,old_dir))+increment_count(dir_rename_count,old_dir,new_dir);+else+break;++/* If we hit toplevel directory ("") for old or new dir, quit */+if(!*old_dir||!*new_dir)+break;++first_time_in_loop=0;+}++/* Free resources we don't need anymore */+free(old_dir);+free(new_dir);+}+staticvoidcompute_rename_counts(structdiff_queue_struct*pairs,structstrmap*dir_rename_count,structstrset*dirs_removed)
@@ -980,20 +955,12 @@ static void compute_rename_counts(struct diff_queue_struct *pairs,inti;for(i=0;i<pairs->nr;++i){-char*old_dir,*new_dir;structdiff_filepair*pair=pairs->queue[i];/* File not part of directory rename if it wasn't renamed */if(pair->status!='R')continue;-/* Get the old and new directory names */-get_renamed_dir_portion(pair->one->path,pair->two->path,-&old_dir,&new_dir);-if(!old_dir)-/* Directory didn't change at all; ignore this one. */-continue;-/**Makedir_rename_countcontainamapofamap:*old_directory->{new_directory->count}
@@ -1001,12 +968,9 @@ static void compute_rename_counts(struct diff_queue_struct *pairs,*theoldfilenameandthenewfilenameandcounthowmany*timesthatpairingoccurs.*/-if(strset_contains(dirs_removed,old_dir))-increment_count(dir_rename_count,old_dir,new_dir);--/* Free resources we don't need anymore */-free(old_dir);-free(new_dir);+update_dir_rename_counts(dir_rename_count,dirs_removed,+pair->one->path,+pair->two->path);}}
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-19 19:56:47
From: Elijah Newren <redacted>
Both of these are copied from merge-recursive.c, with just minor tweaks
due to using strmap API and not having a non_unique_new_dir field.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 37 +++++++++++++++++++++++++++++++++++--
1 file changed, 35 insertions(+), 2 deletions(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-19 19:58:42
From: Elijah Newren <redacted>
Since directory rename detection adds new paths to opt->priv->paths and
removes old ones, process_renames() needs to now check whether
pair->one->path actually exists in opt->priv->paths instead of just
assuming it does.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 26 +++++++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-19 19:58:55
From: Elijah Newren <redacted>
Due to the string-equality-iff-pointer-equality requirements placed on
merged_info.directory_name, apply_directory_rename_modifications() will
need to have access to the exact toplevel directory name string pointer
and can't just use a new empty string. Store it in a field that we can
use.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-19 19:59:15
From: Elijah Newren <redacted>
This is copied from merge-recursive.c, with minor tweaks due to:
* using strmap API
* merge-ort not using the non_unique_new_dir field, since it'll
obviate its need entirely later with performance improvements
* adding a new path_in_way() function that uses opt->priv->paths
instead of doing an expensive tree_has_path() lookup to see if
a tree has a given path.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 71 insertions(+), 1 deletion(-)
@@ -876,7 +886,67 @@ static char *handle_path_level_conflicts(struct merge_options *opt,structstrmap_entry*rename_info,structstrmap*collisions){-die("Not yet implemented");+char*new_path=NULL;+structcollision_info*c_info;+intclean=1;+structstrbufcollision_paths=STRBUF_INIT;++/*+*entryhasthemappingofolddirectorynametonewdirectoryname+*thatwewanttoapplytopath.+*/+new_path=apply_dir_rename(rename_info,path);+if(!new_path)+BUG("Failed to apply directory rename!");++/*+*Thecallerneedstohaveensuredthatithaspre-populated+*collisionswithallpathsthatmaptonew_path.Doaquickcheck+*toensurethat'sthecase.+*/+c_info=strmap_get(collisions,new_path);+if(c_info==NULL)+BUG("c_info is NULL");++/*+*Checkforone-sidedadd/add/.../addconflicts,i.e.+*whereimplicitrenamesfromtheothersidedoing+*directoryrename(s)canaffectthissideofhistory+*toputmultiplepathsintothesamelocation.Warn+*andbailondirectoryrenamesforsuchpaths.+*/+if(c_info->reported_already){+clean=0;+}elseif(path_in_way(&opt->priv->paths,new_path,1<<side_index)){+c_info->reported_already=1;+strbuf_add_separated_string_list(&collision_paths,", ",+&c_info->source_files);+path_msg(opt,new_path,0,+_("CONFLICT (implicit dir rename): Existing file/dir "+"at %s in the way of implicit directory rename(s) "+"putting the following path(s) there: %s."),+new_path,collision_paths.buf);+clean=0;+}elseif(c_info->source_files.nr>1){+c_info->reported_already=1;+strbuf_add_separated_string_list(&collision_paths,", ",+&c_info->source_files);+path_msg(opt,new_path,0,+_("CONFLICT (implicit dir rename): Cannot map more "+"than one path to %s; implicit directory renames "+"tried to put these paths there: %s"),+new_path,collision_paths.buf);+clean=0;+}++/* Free memory we no longer need */+strbuf_release(&collision_paths);+if(!clean&&new_path){+free(new_path);+returnNULL;+}++returnnew_path;}staticvoidincrement_count(structstrmap*dir_rename_count,
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-19 19:59:18
From: Elijah Newren <redacted>
This is copied from merge-recursive.c, with minor tweaks due to using strmap
API and the fact that it can use opt->priv->paths to get all pathnames that
exist instead of taking a tree object.
This depends on a new function, handle_path_level_conflicts(), which
just has a placeholder die-not-yet-implemented implementation for now; a
subsequent patch will implement it.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 66 insertions(+), 1 deletion(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-19 19:59:59
From: Elijah Newren <redacted>
This is modelled on the version of handle_directory_level_conflicts()
from merge-recursive.c, but is massively simplified due to the following
factors:
* strmap API provides simplifications over using direct hashmap
* we have a dirs_removed field in struct rename_info that we have an
easy way to populate from collect_merge_info(); this was already
used in compute_rename_counts() and thus we do not need to check
for condition #2.
* The removal of condition #2 by handling it earlier in the code also
obviates the need to check for condition #3 -- if both sides renamed
a directory, meaning that the directory no longer exists on either
side, then neither side could have added any new files to that
directory, and thus there are no files whose locations we need to
move due to such a directory rename.
In fact, the same logic that makes condition #3 irrelevant means
condition #1 is also irrelevant so we could drop this function.
However, it is cheap to check if both sides rename the same directory,
and doing so can save future computation. So, simply remove any
directories that both sides renamed from the list of directory renames.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-19 20:00:00
From: Elijah Newren <redacted>
collect_renames() is similar to merge-recursive.c's get_renames(), but
lacks the directory rename handling found in the latter. Port that code
structure over to merge-ort. This introduces three new
die-not-yet-implemented functions that will be defined in future
commits.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 74 insertions(+), 4 deletions(-)
@@ -721,6 +721,11 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/+structcollision_info{+structstring_listsource_files;+unsignedreported_already:1;+};+staticvoidget_renamed_dir_portion(constchar*old_path,constchar*new_path,char**old_dir,char**new_dir){
@@ -959,6 +964,31 @@ static void handle_directory_level_conflicts(struct merge_options *opt)string_list_clear(&duplicated,0);}+staticvoidcompute_collisions(structstrmap*collisions,+structstrmap*dir_renames,+structdiff_queue_struct*pairs)+{+die("Not yet implemented.");+}++staticchar*check_for_directory_rename(structmerge_options*opt,+constchar*path,+unsignedside_index,+structstrmap*dir_renames,+structstrmap*dir_rename_exclusions,+structstrmap*collisions,+int*clean_merge)+{+die("Not yet implemented.");+}++staticvoidapply_directory_rename_modifications(structmerge_options*opt,+structdiff_filepair*pair,+char*new_path)+{+die("Not yet implemented.");+}+/*** Function Grouping: functions related to regular rename detection ***/staticintprocess_renames(structmerge_options*opt,
@@ -1284,22 +1314,44 @@ static void detect_regular_renames(struct merge_options *opt,*/staticintcollect_renames(structmerge_options*opt,structdiff_queue_struct*result,-unsignedside_index)+unsignedside_index,+structstrmap*dir_renames_for_side,+structstrmap*rename_exclusions){inti,clean=1;+structstrmapcollisions;structdiff_queue_struct*side_pairs;+structhashmap_iteriter;+structstrmap_entry*entry;structrename_info*renames=&opt->priv->renames;side_pairs=&renames->pairs[side_index];+compute_collisions(&collisions,dir_renames_for_side,side_pairs);for(i=0;i<side_pairs->nr;++i){structdiff_filepair*p=side_pairs->queue[i];+char*new_path;/* non-NULL only with directory renames */-if(p->status!='R'){+if(p->status!='A'&&p->status!='R'){diff_free_filepair(p);continue;}+new_path=check_for_directory_rename(opt,p->two->path,+side_index,+dir_renames_for_side,+rename_exclusions,+&collisions,+&clean);++if(p->status!='R'&&!new_path){+diff_free_filepair(p);+continue;+}++if(new_path)+apply_directory_rename_modifications(opt,p,new_path);+/**p->scorecomesbackfromdiffcore_rename_extended()with*thesimilarityoftherenamedfile.Thesimilarityis
@@ -1314,6 +1366,20 @@ static int collect_renames(struct merge_options *opt,result->queue[result->nr++]=p;}+/* Free each value in the collisions map */+strmap_for_each_entry(&collisions,&iter,entry){+structcollision_info*info=entry->value;+string_list_clear(&info->source_files,0);+}+/*+*Incompute_collisions(),wesetcollisions.strdup_stringsto0+*sothatwewouldn'thavetomakeanothercopyofthenew_path+*allocatedbyapply_dir_rename().Butnowthatwe'veusedthem+*andhavenootherreferencestothesestrings,itistimeto+*deallocatethem.+*/+free_strmap_strings(&collisions);+strmap_clear(&collisions,1);returnclean;}
@@ -1345,8 +1411,12 @@ static int detect_and_process_renames(struct merge_options *opt,ALLOC_GROW(combined.queue,renames->pairs[1].nr+renames->pairs[2].nr,combined.alloc);-clean&=collect_renames(opt,&combined,MERGE_SIDE1);-clean&=collect_renames(opt,&combined,MERGE_SIDE2);+clean&=collect_renames(opt,&combined,MERGE_SIDE1,+&renames->dir_renames[2],+&renames->dir_renames[1]);+clean&=collect_renames(opt,&combined,MERGE_SIDE2,+&renames->dir_renames[1],+&renames->dir_renames[2]);QSORT(combined.queue,combined.nr,compare_pairs);clean&=process_renames(opt,&combined);
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-01-19 20:01:07
From: Elijah Newren <redacted>
This function is based on the first half of get_directory_renames() from
merge-recursive.c; as part of the implementation, factor out a routine,
increment_count(), to update the bookkeeping to track the number of
items renamed into new directories.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 52 insertions(+), 2 deletions(-)
@@ -721,7 +721,6 @@ static int handle_content_merge(struct merge_options *opt,/*** Function Grouping: functions related to directory rename detection ***/-MAYBE_UNUSEDstaticvoidget_renamed_dir_portion(constchar*old_path,constchar*new_path,char**old_dir,char**new_dir){
@@ -825,11 +824,62 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,*new_dir=xstrndup(new_path,end_of_new-new_path);}+staticvoidincrement_count(structstrmap*dir_rename_count,+char*old_dir,+char*new_dir)+{+structstrintmap*counts;+structstrmap_entry*e;++/* Get the {new_dirs -> counts} mapping using old_dir */+e=strmap_get_entry(dir_rename_count,old_dir);+if(e){+counts=e->value;+}else{+counts=xmalloc(sizeof(*counts));+strintmap_init_with_options(counts,0,NULL,1);+strmap_put(dir_rename_count,old_dir,counts);+}++/* Increment the count for new_dir */+strintmap_incr(counts,new_dir,1);+}+staticvoidcompute_rename_counts(structdiff_queue_struct*pairs,structstrmap*dir_rename_count,structstrset*dirs_removed){-die("Not yet implemented!");+inti;++for(i=0;i<pairs->nr;++i){+char*old_dir,*new_dir;+structdiff_filepair*pair=pairs->queue[i];++/* File not part of directory rename if it wasn't renamed */+if(pair->status!='R')+continue;++/* Get the old and new directory names */+get_renamed_dir_portion(pair->one->path,pair->two->path,+&old_dir,&new_dir);+if(!old_dir)+/* Directory didn't change at all; ignore this one. */+continue;++/*+*Makedir_rename_countcontainamapofamap:+*old_directory->{new_directory->count}+*Inotherwords,foreverypairlookatthedirectoriesfor+*theoldfilenameandthenewfilenameandcounthowmany+*timesthatpairingoccurs.+*/+if(strset_contains(dirs_removed,old_dir))+increment_count(dir_rename_count,old_dir,new_dir);++/* Free resources we don't need anymore */+free(old_dir);+free(new_dir);+}}staticvoidget_provisional_directory_renames(structmerge_options*opt,
From: Taylor Blau <hidden> Date: 2021-01-19 22:50:29
On Tue, Jan 19, 2021 at 07:53:36PM +0000, Elijah Newren via GitGitGadget wrote:
Changes since v2:
Thanks, all of these look good to me. I appreciate you addressing all of
my nits :-).
I couldn't find anything to write about in the last clump of patches, so
with these recent cleanups this series LGTM.
Reviewed-by: Taylor Blau [off-list ref]
Thanks,
Taylor