Message ID | f017534243c967caa0137e6899f4e1c69ff02c2e.1607962900.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | merge-ort: add basic rename detection | expand |
On 12/14/2020 11:21 AM, Elijah Newren via GitGitGadget wrote: > From: Elijah Newren <newren@gmail.com> > > Implement rename/delete conflicts, i.e. one side renames a file and the > other deletes the file. This code replaces the following from > merge-recurisve.c: > > * the code relevant to RENAME_DELETE in process_renames() > * the RENAME_DELETE case of process_entry() > * handle_rename_delete() > > Also, there is some shared code from merge-recursive.c for multiple > different rename cases which we will no longer need for this case (or > other rename cases): > > * handle_change_delete() > * setup_rename_conflict_info() > > The consolidation of five separate codepaths into one is made possible > by a change in design: process_renames() tweaks the conflict_info > entries within opt->priv->paths such that process_entry() can then > handle all the non-rename conflict types (directory/file, modify/delete, > etc.) orthogonally. This means we're much less likely to miss special > implementation of some kind of combination of conflict types (see > commits brought in by 66c62eaec6 ("Merge branch 'en/merge-tests'", > 2020-11-18), especially commit ef52778708 ("merge tests: expect improved > directory/file conflict handling in ort", 2020-10-26) for more details). > That, together with letting worktree/index updating be handled > orthogonally in the merge_switch_to_result() function, dramatically > simplifies the code for various special rename cases. > > To be fair, there is a _slight_ tweak to process_entry() here, because > rename/delete cases will also trigger the modify/delete codepath. > However, we only want a modify/delete message to be printed for a > rename/delete conflict if there is a content change in the renamed file > in addition to the rename. So process_renames() and process_entry() > aren't quite fully orthogonal, but they are pretty close. Thanks for adding this warning about the change to process_entry(). > @@ -657,6 +657,7 @@ static int process_renames(struct merge_options *opt, > unsigned int old_sidemask; > int target_index, other_source_index; > int source_deleted, collision, type_changed; > + const char *rename_branch = NULL, *delete_branch = NULL; Ah, here they are! > + if (source_deleted) { > + if (target_index == 1) { > + rename_branch = opt->branch1; > + delete_branch = opt->branch2; > + } else { > + rename_branch = opt->branch2; > + delete_branch = opt->branch1; > + } > } > > assert(source_deleted || oldinfo->filemask & old_sidemask); > @@ -838,13 +847,26 @@ static int process_renames(struct merge_options *opt, > "to %s in %s, but deleted in %s."), > oldpath, newpath, rename_branch, delete_branch); This context line is the previous use of rename_branch and delete_branch. Perhaps the declarations, initialization, and first-use here are worth their own patch? > } else { > + /* > + * a few different cases...start by copying the > + * existing stage(s) from oldinfo over the newinfo > + * and update the pathname(s). > + */ > + memcpy(&newinfo->stages[0], &oldinfo->stages[0], > + sizeof(newinfo->stages[0])); > + newinfo->filemask |= (1 << MERGE_BASE); > + newinfo->pathnames[0] = oldpath; > if (type_changed) { > /* rename vs. typechange */ > die("Not yet implemented"); > } else if (source_deleted) { > /* rename/delete */ > + newinfo->path_conflict = 1; > + path_msg(opt, newpath, 0, > + _("CONFLICT (rename/delete): %s renamed" > + " to %s in %s, but deleted in %s."), > + oldpath, newpath, > + rename_branch, delete_branch); Since the primary purpose of rename_branch and delete_branch appears to be for these error messages, then likely the previous error message about a rename/delete should just be promoted into this patch instead of the previous. In fact, the error messages are the exact same, but with slightly different lines due to wrapping: path_msg(opt, newpath, 0, _("CONFLICT (rename/delete): %s renamed " "to %s in %s, but deleted in %s."), oldpath, newpath, rename_branch, delete_branch); and path_msg(opt, newpath, 0, _("CONFLICT (rename/delete): %s renamed" " to %s in %s, but deleted in %s."), oldpath, newpath, rename_branch, delete_branch); I wonder if there is a way to group these together? Perhaps the nested if/else if/else blocks could store a "conflict state" value that says which CONFLICT message to print after the complicated branching is done. Alternatively, this message appears to be written in the following case: source_deleted && !type_changed your if/else if/else block could be rearranged as follows: if (collision && !source_deleted) /* collision: rename/add or rename/rename(2to1) */ else if (!type_change && source_deleted) /* rename/delete or rename/add/delete or rename/rename(2to1)/delete */ else if (!collision) /* a few different cases */ Of course, the thing I am missing is that copy of oldinfo->stages[0] into newinfo->stages[0] along with changes to the filemask and pathnames! That is likely why you need the two different markers, because the cases truly are different in that subtle way. > /* normal rename */ > die("Not yet implemented"); > @@ -1380,12 +1402,21 @@ static void process_entry(struct merge_options *opt, > modify_branch = (side == 1) ? opt->branch1 : opt->branch2; > delete_branch = (side == 1) ? opt->branch2 : opt->branch1; > > - path_msg(opt, path, 0, > - _("CONFLICT (modify/delete): %s deleted in %s " > - "and modified in %s. Version %s of %s left " > - "in tree."), > - path, delete_branch, modify_branch, > - modify_branch, path); > + if (ci->path_conflict && > + oideq(&ci->stages[0].oid, &ci->stages[side].oid)) { > + /* > + * This came from a rename/delete; no action to take, > + * but avoid printing "modify/delete" conflict notice > + * since the contents were not modified. > + */ > + } else { > + path_msg(opt, path, 0, > + _("CONFLICT (modify/delete): %s deleted in %s " > + "and modified in %s. Version %s of %s left " > + "in tree."), > + path, delete_branch, modify_branch, > + modify_branch, path); > + } Thanks, -Stolee
On 12/14/2020 11:21 AM, Elijah Newren via GitGitGadget wrote: > if (type_changed && collision) { > /* special handling so later blocks can handle this */ > die("Not yet implemented"); > + if (source_deleted) { I didn't catch this in my earlier message, but the opening brace of the if (type_changed && collision) gets squashed here, causing a compiler break. Thanks, -Stolee
On Tue, Dec 15, 2020 at 6:23 AM Derrick Stolee <stolee@gmail.com> wrote: > > On 12/14/2020 11:21 AM, Elijah Newren via GitGitGadget wrote: > > From: Elijah Newren <newren@gmail.com> > > > > Implement rename/delete conflicts, i.e. one side renames a file and the > > other deletes the file. This code replaces the following from > > merge-recurisve.c: > > > > * the code relevant to RENAME_DELETE in process_renames() > > * the RENAME_DELETE case of process_entry() > > * handle_rename_delete() > > > > Also, there is some shared code from merge-recursive.c for multiple > > different rename cases which we will no longer need for this case (or > > other rename cases): > > > > * handle_change_delete() > > * setup_rename_conflict_info() > > > > The consolidation of five separate codepaths into one is made possible > > by a change in design: process_renames() tweaks the conflict_info > > entries within opt->priv->paths such that process_entry() can then > > handle all the non-rename conflict types (directory/file, modify/delete, > > etc.) orthogonally. This means we're much less likely to miss special > > implementation of some kind of combination of conflict types (see > > commits brought in by 66c62eaec6 ("Merge branch 'en/merge-tests'", > > 2020-11-18), especially commit ef52778708 ("merge tests: expect improved > > directory/file conflict handling in ort", 2020-10-26) for more details). > > That, together with letting worktree/index updating be handled > > orthogonally in the merge_switch_to_result() function, dramatically > > simplifies the code for various special rename cases. > > > > To be fair, there is a _slight_ tweak to process_entry() here, because > > rename/delete cases will also trigger the modify/delete codepath. > > However, we only want a modify/delete message to be printed for a > > rename/delete conflict if there is a content change in the renamed file > > in addition to the rename. So process_renames() and process_entry() > > aren't quite fully orthogonal, but they are pretty close. > > Thanks for adding this warning about the change to process_entry(). > > > @@ -657,6 +657,7 @@ static int process_renames(struct merge_options *opt, > > unsigned int old_sidemask; > > int target_index, other_source_index; > > int source_deleted, collision, type_changed; > > + const char *rename_branch = NULL, *delete_branch = NULL; > > Ah, here they are! > > > + if (source_deleted) { > > + if (target_index == 1) { > > + rename_branch = opt->branch1; > > + delete_branch = opt->branch2; > > + } else { > > + rename_branch = opt->branch2; > > + delete_branch = opt->branch1; > > + } > > } > > > > assert(source_deleted || oldinfo->filemask & old_sidemask); > > @@ -838,13 +847,26 @@ static int process_renames(struct merge_options *opt, > > "to %s in %s, but deleted in %s."), > > oldpath, newpath, rename_branch, delete_branch); > > This context line is the previous use of rename_branch and delete_branch. > Perhaps the declarations, initialization, and first-use here are worth > their own patch? > > > } else { > > + /* > > + * a few different cases...start by copying the > > + * existing stage(s) from oldinfo over the newinfo > > + * and update the pathname(s). > > + */ > > + memcpy(&newinfo->stages[0], &oldinfo->stages[0], > > + sizeof(newinfo->stages[0])); > > + newinfo->filemask |= (1 << MERGE_BASE); > > + newinfo->pathnames[0] = oldpath; > > if (type_changed) { > > /* rename vs. typechange */ > > die("Not yet implemented"); > > } else if (source_deleted) { > > /* rename/delete */ > > + newinfo->path_conflict = 1; > > + path_msg(opt, newpath, 0, > > + _("CONFLICT (rename/delete): %s renamed" > > + " to %s in %s, but deleted in %s."), > > + oldpath, newpath, > > + rename_branch, delete_branch); > > Since the primary purpose of rename_branch and delete_branch appears to > be for these error messages, then likely the previous error message about > a rename/delete should just be promoted into this patch instead of the > previous. > > In fact, the error messages are the exact same, but with slightly different > lines due to wrapping: > > path_msg(opt, newpath, 0, > _("CONFLICT (rename/delete): %s renamed " > "to %s in %s, but deleted in %s."), > oldpath, newpath, rename_branch, delete_branch); > > and > > path_msg(opt, newpath, 0, > _("CONFLICT (rename/delete): %s renamed" > " to %s in %s, but deleted in %s."), > oldpath, newpath, > rename_branch, delete_branch); > > I wonder if there is a way to group these together? Perhaps the nested > if/else if/else blocks could store a "conflict state" value that says > which CONFLICT message to print after the complicated branching is done. > > Alternatively, this message appears to be written in the following case: > > source_deleted && !type_changed > > your if/else if/else block could be rearranged as follows: > > if (collision && !source_deleted) > /* collision: rename/add or rename/rename(2to1) */ > else if (!type_change && source_deleted) > /* rename/delete or rename/add/delete or rename/rename(2to1)/delete */ > else if (!collision) > /* a few different cases */ > > Of course, the thing I am missing is that copy of oldinfo->stages[0] into > newinfo->stages[0] along with changes to the filemask and pathnames! That > is likely why you need the two different markers, because the cases truly > are different in that subtle way. Yeah, there is that subtlety and another one -- the rename/add/delete case will also later trigger the "add/add" conflict type within process_entries() for this same path, whereas the rename/delete case from this patch won't. The combination is enough of a difference that I'm worried that trying to make both types run through the same code block might blur the differences and pose a landmine for future folks coming to edit the code; it'd make it too easy to break one or the other conflict type. If the sharing was done a different way, namely saving the basic message in some variable before either if-block and then both places just pass that string to path_msg() instead of both having it typed out, then that'd probably make sense, but then we're not really saving much. > > /* normal rename */ > > die("Not yet implemented"); > > @@ -1380,12 +1402,21 @@ static void process_entry(struct merge_options *opt, > > modify_branch = (side == 1) ? opt->branch1 : opt->branch2; > > delete_branch = (side == 1) ? opt->branch2 : opt->branch1; > > > > - path_msg(opt, path, 0, > > - _("CONFLICT (modify/delete): %s deleted in %s " > > - "and modified in %s. Version %s of %s left " > > - "in tree."), > > - path, delete_branch, modify_branch, > > - modify_branch, path); > > + if (ci->path_conflict && > > + oideq(&ci->stages[0].oid, &ci->stages[side].oid)) { > > + /* > > + * This came from a rename/delete; no action to take, > > + * but avoid printing "modify/delete" conflict notice > > + * since the contents were not modified. > > + */ > > + } else { > > + path_msg(opt, path, 0, > > + _("CONFLICT (modify/delete): %s deleted in %s " > > + "and modified in %s. Version %s of %s left " > > + "in tree."), > > + path, delete_branch, modify_branch, > > + modify_branch, path); > > + } > > Thanks, > -Stolee
diff --git a/merge-ort.c b/merge-ort.c index 04a16837849..4150ccc35e1 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -657,6 +657,7 @@ static int process_renames(struct merge_options *opt, unsigned int old_sidemask; int target_index, other_source_index; int source_deleted, collision, type_changed; + const char *rename_branch = NULL, *delete_branch = NULL; old_ent = strmap_get_entry(&opt->priv->paths, pair->one->path); oldpath = old_ent->key; @@ -778,6 +779,14 @@ static int process_renames(struct merge_options *opt, if (type_changed && collision) { /* special handling so later blocks can handle this */ die("Not yet implemented"); + if (source_deleted) { + if (target_index == 1) { + rename_branch = opt->branch1; + delete_branch = opt->branch2; + } else { + rename_branch = opt->branch2; + delete_branch = opt->branch1; + } } assert(source_deleted || oldinfo->filemask & old_sidemask); @@ -838,13 +847,26 @@ static int process_renames(struct merge_options *opt, "to %s in %s, but deleted in %s."), oldpath, newpath, rename_branch, delete_branch); } else { - /* a few different cases... */ + /* + * a few different cases...start by copying the + * existing stage(s) from oldinfo over the newinfo + * and update the pathname(s). + */ + memcpy(&newinfo->stages[0], &oldinfo->stages[0], + sizeof(newinfo->stages[0])); + newinfo->filemask |= (1 << MERGE_BASE); + newinfo->pathnames[0] = oldpath; if (type_changed) { /* rename vs. typechange */ die("Not yet implemented"); } else if (source_deleted) { /* rename/delete */ - die("Not yet implemented"); + newinfo->path_conflict = 1; + path_msg(opt, newpath, 0, + _("CONFLICT (rename/delete): %s renamed" + " to %s in %s, but deleted in %s."), + oldpath, newpath, + rename_branch, delete_branch); } else { /* normal rename */ die("Not yet implemented"); @@ -1380,12 +1402,21 @@ static void process_entry(struct merge_options *opt, modify_branch = (side == 1) ? opt->branch1 : opt->branch2; delete_branch = (side == 1) ? opt->branch2 : opt->branch1; - path_msg(opt, path, 0, - _("CONFLICT (modify/delete): %s deleted in %s " - "and modified in %s. Version %s of %s left " - "in tree."), - path, delete_branch, modify_branch, - modify_branch, path); + if (ci->path_conflict && + oideq(&ci->stages[0].oid, &ci->stages[side].oid)) { + /* + * This came from a rename/delete; no action to take, + * but avoid printing "modify/delete" conflict notice + * since the contents were not modified. + */ + } else { + path_msg(opt, path, 0, + _("CONFLICT (modify/delete): %s deleted in %s " + "and modified in %s. Version %s of %s left " + "in tree."), + path, delete_branch, modify_branch, + modify_branch, path); + } } else if (ci->filemask == 2 || ci->filemask == 4) { /* Added on one side */ int side = (ci->filemask == 4) ? 2 : 1;