| Message ID | 69d6204184363e491acb68f744ded0991be63a47.1655871652.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix dual rename into each other plus conflicting adds | expand |
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Elijah Newren <newren@gmail.com> > > 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. 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 Also, at first glance, "newfile" shouldn't belong in a minimal reproduction, 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.
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > +# 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 Grammatically, I could not understand "both add a file with after directory renames are the same filename" I also found the same issue with `Expected` that Jonathan mentions. I ran the command separately and got CONFLICT (add/add): Merge conflict in sub1/sub2/new_add_add_file
On Mon, Jun 27, 2022 at 11:20 AM Jonathan Tan <jonathantanmy@google.com> wrote: > > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Elijah Newren <newren@gmail.com> > > > > 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.
On Mon, Jun 27, 2022 at 3:30 PM Calvin Wan <calvinwan@google.com> wrote: > > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > +# 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 > > Grammatically, I could not understand > > "both add a file with after directory renames are the same filename" Oops, that should be "with" -> "which". > I also found the same issue with `Expected` that Jonathan mentions. I ran the > command separately and got > > CONFLICT (add/add): Merge conflict in sub1/sub2/new_add_add_file Yeah, repeatedly revising stuff is fine as long as you remember to update all the parts. (I was swapping which directories were named "sub1", "sub2" and "sub3" to see if lexicographic ordering might be related to why this simpler test triggered a bug in the "recursive" strategy but Glen's bigger testcase didn't.). Anyway, I just missed updating one of the locations while doing that revision; sorry about that. Will fix.
Elijah Newren <newren@gmail.com> writes: > 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). Thanks! This makes sense. Might be worth including as a comment (explaining why "newfile" is there) in the test.
On Thu, Jun 30, 2022 at 3:32 PM Jonathan Tan <jonathantanmy@google.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > 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). > > Thanks! This makes sense. Might be worth including as a comment > (explaining why "newfile" is there) in the test. Sure, will do.
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 + +test_setup_12l () { + test_create_repo 12l_$1 && + ( + cd 12l_$1 && + + mkdir -p sub1 sub2 + echo file >sub1/file && + echo other >sub2/other && + git add sub1 sub2 && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + git mv sub1 sub3 && + echo conflicting >sub2/new_add_add_file && + git add sub2 && + test_tick && + git add -u && + git commit -m "A" && + + git checkout B && + echo dissimilar >sub2/new_add_add_file && + echo brand >sub1/newfile && + git add sub1 sub2 && + git mv sub2 sub1 && + test_tick && + git commit -m "B" + ) +} + +test_expect_merge_algorithm failure failure '12l (B into A): Rename into each other + add/add conflict' ' + test_setup_12l BintoA && + ( + cd 12l_BintoA && + + git checkout -q A^0 && + + test_must_fail git -c merge.directoryRenames=true merge -s recursive B^0 && + + git ls-files -s >out && + test_line_count = 5 out && + + git rev-parse >actual \ + :0:sub3/file :0:sub3/newfile :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 \ + A:sub2/new_add_add_file \ + B:sub1/sub2/new_add_add_file && + test_cmp expect actual && + + git ls-files -o >actual && + test_write_lines actual expect out >expect && + test_cmp expect actual + ) +' + +test_expect_merge_algorithm failure failure '12l (A into B): Rename into each other + add/add conflict' ' + test_setup_12l AintoB && + ( + cd 12l_AintoB && + + git checkout -q B^0 && + + test_must_fail git -c merge.directoryRenames=true merge -s recursive A^0 && + + git ls-files -s >out && + test_line_count = 5 out && + + git rev-parse >actual \ + :0:sub3/file :0:sub3/newfile :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 \ + B:sub1/sub2/new_add_add_file \ + A:sub2/new_add_add_file && + test_cmp expect actual && + + git ls-files -o >actual && + test_write_lines actual expect out >expect && + test_cmp expect actual + ) +' + ########################################################################### # SECTION 13: Checking informational and conflict messages #