On Mon, Jun 27, 2022 at 11:20 AM Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: Elijah Newren <newren@xxxxxxxxx>
> >
> > This is an attempt at minimalizing a testcase reported by Glen Choo
> > with tensorflow where merge-ort would report an assertion failure:
> >
> > Assertion failed: (ci->filemask == 2 || ci->filemask == 4), function apply_directory_rename_modifications, file merge-ort.c, line 2410
>
> First of all, thanks for this fix - I've verified with Glen Choo's test
> cases and it does fix it.
:-)
> > diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
> > index 479db32cd62..296c04f8046 100755
> > --- a/t/t6423-merge-rename-directories.sh
> > +++ b/t/t6423-merge-rename-directories.sh
> > @@ -5199,6 +5199,108 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '
> > )
> > '
> >
> > +# Testcase 12l, Both sides rename a directory into the other side, both add
> > +# a file with after directory renames are the same filename
> > +# Commit O: sub1/file, sub2/other
> > +# Commit A: sub3/file, sub2/{other, new_add_add_file_1}
> > +# Commit B: sub1/{file, newfile}, sub1/sub2/{other, new_add_add_file_2}
> > +#
> > +# In words:
> > +# A: sub1/ -> sub3/, add sub2/new_add_add_file_1
> > +# B: sub2/ -> sub1/sub2, add sub1/newfile, add sub1/sub2/new_add_add_file_2
> > +#
> > +# Expected: sub3/{file, newfile, sub2/other}
> > +# CONFLICT (add/add): sub3/sub2/new_add_add_file
>
> The "expected" might need to be updated.
Oops! Yes, I kept changing and editing the testcase and the
comments...but didn't quite get all the updates I needed when I was
revising.
> After all patches are applied,
> this is the tree I get (note that it's "sub1/sub2/new_add_add_file, not
> "sub3/sub2/new_add_add_file"):
>
> .
> |-- sub1
> | `-- sub2
> | `-- new_add_add_file
> `-- sub3
> |-- file
> |-- newfile
> `-- sub2
> `-- other
Yes, that's right.
> Also, at first glance, "newfile" shouldn't belong in a minimal
> reproduction
Ah, I can see you've looked at this very closely. Thanks for digging
in! I know it's counter-intuitive at first, but the file is
necessary in order to get the sub1/ -> sub3/ rename. The reasoning is
as follows: We don't need to detect a directory rename for a directory
where the other side added no new files into that directory...because
the whole point of directory renames is to move new files in a
directory to the new location. Therefore, no new files in the
directory on one side, means no need to detect a directory rename for
it on the other side. For a deeper discussion of this, see commit
c64432aacd (t6423: more involved rules for renaming directories into
each other, 2020-10-15).
>, but it somehow changes the output. When I apply this diff
> (to the state after all patches are applied):
>
> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
> index 4286ae987c..9472fb7233 100755
> --- a/t/t6423-merge-rename-directories.sh
> +++ b/t/t6423-merge-rename-directories.sh
> @@ -5237,7 +5237,6 @@ test_setup_12l () {
>
> git checkout B &&
> echo dissimilar >sub2/new_add_add_file &&
> - echo brand >sub1/newfile &&
> git add sub1 sub2 &&
> git mv sub2 sub1 &&
> test_tick &&
> @@ -5255,14 +5254,14 @@ test_expect_merge_algorithm failure success '12l (B into A): Rename into each ot
> test_must_fail git -c merge.directoryRenames=true merge -s recursive B^0 &&
>
> git ls-files -s >out &&
> - test_line_count = 5 out &&
> + test_line_count = 4 out &&
>
> git rev-parse >actual \
> - :0:sub3/file :0:sub3/newfile :0:sub3/sub2/other \
> + :0:sub3/file :0:sub3/sub2/other \
> :2:sub1/sub2/new_add_add_file \
> :3:sub1/sub2/new_add_add_file &&
> git rev-parse >expect \
> - O:sub1/file B:sub1/newfile O:sub2/other \
> + O:sub1/file O:sub2/other \
> A:sub2/new_add_add_file \
> B:sub1/sub2/new_add_add_file &&
> test_cmp expect actual &&
>
> I get:
>
> .
> |-- sub1
> | `-- sub2
> | |-- new_add_add_file
> | `-- other
> `-- sub3
> `-- file
>
> (Note the path to "other".) I haven't figured out what's going on,
> though.
Yeah, this is the result of having no directory rename due to having
no new files that need to be moved by a directory rename.
