On Tue, Dec 15, 2020 at 6:31 AM Derrick Stolee [off-list ref] wrote:

On 12/14/2020 11:21 AM, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

Implement cases where renames are involved in type changes (i.e. the
side of history that didn't rename the file changed its type from a
regular file to a symlink or submodule).  There was some code to handle
this in merge-recursive but only in the special case when the renamed
file had no content changes.  The code here works differently -- it
knows process_entry() can handle mode conflicts, so it does a few
minimal tweaks to ensure process_entry() can just finish the job as
needed.

Signed-off-by: Elijah Newren <redacted>
---
 merge-ort.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index 9aac33c8e31..11e33f56edf 100644
--- a/merge-ort.c
+++ b/merge-ort.c

@@ -778,7 +778,32 @@ static int process_renames(struct merge_options *opt,
                       S_ISREG(newinfo->stages[target_index].mode));
              if (type_changed && collision) {
                      /* special handling so later blocks can handle this */

Perhaps drop this comment, or incorporate it into the lower one?

Will do.

quoted

-                     die("Not yet implemented");
+                     /*
+                      * if type_changed && collision are both true, then this
+                      * was really a double rename, but one side wasn't
+                      * detected due to lack of break detection.  I.e.
+                      * something like
+                      *    orig: has normal file 'foo'
+                      *    side1: renames 'foo' to 'bar', adds 'foo' symlink
+                      *    side2: renames 'foo' to 'bar'
+                      * In this case, the foo->bar rename on side1 won't be
+                      * detected because the new symlink named 'foo' is
+                      * there and we don't do break detection.  But we detect
+                      * this here because we don't want to merge the content
+                      * of the foo symlink with the foo->bar file, so we
+                      * have some logic to handle this special case.  The
+                      * easiest way to do that is make 'bar' on side1 not
+                      * be considered a colliding file but the other part
+                      * of a normal rename.  If the file is very different,
+                      * well we're going to get content merge conflicts
+                      * anyway so it doesn't hurt.  And if the colliding
+                      * file also has a different type, that'll be handled
+                      * by the content merge logic in process_entry() too.
+                      *
+                      * See also t6430, 'rename vs. rename/symlink'

I appreciate the callout to a test that exercises this behavior.

quoted

+                      */
+                     collision = 0;
+             }

Here, we regain that closing curly brace, fixing the compiler errors from
earlier.

So embarrassing.  I was pretty sure I tested the individual patches,
but maybe I somehow missed this series??  Anyway, yeah, I'll fix it
up.

quoted

              if (source_deleted) {
                      if (target_index == 1) {
                              rename_branch = opt->branch1;

@@ -858,7 +883,11 @@ static int process_renames(struct merge_options *opt,
                      newinfo->pathnames[0] = oldpath;
                      if (type_changed) {
                              /* rename vs. typechange */
-                             die("Not yet implemented");
+                             /* Mark the original as resolved by removal */
+                             memcpy(&oldinfo->stages[0].oid, &null_oid,
+                                    sizeof(oldinfo->stages[0].oid));
+                             oldinfo->stages[0].mode = 0;
+                             oldinfo->filemask &= 0x06;

This matches your explanation in the comment above. I wonder if 0x06
could be less magical, but we are really deep in the weeds here already.

Thanks,
-Stolee