Message ID | bf4c03d01d5730503eb710e92a80136d5caa981a.1656572225.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | Fix dual rename into each other plus conflicting adds | expand |
On Thu, Jun 30 2022, Elijah Newren via GitGitGadget wrote: > From: Elijah Newren <newren@gmail.com> > +test_setup_12l () { > + test_create_repo 12l_$1 && Can & should just be "git init", see f0d4d398e28 (test-lib: split up and deprecate test_create_repo(), 2021-05-10). > + ( > + cd 12l_$1 && > + > + mkdir -p sub1 sub2 The "-p" here isn't needed, you're not creating recursive directories. > + git ls-files -s >out && > + test_line_count = 5 out && Can't this just use test_stdout_line_count? Except... > + > + 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 This test seems a bit odd, here we're just checking that we've created scratch files for the internal use of our test, why is it important that we have an "out" file, when the only reason we have it is because we needed it for checking the "ls-files" line count above? > + ) > +' > + > +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 && ditto. > + 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 && ditto
On Thu, Jun 30, 2022 at 3:26 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > On Thu, Jun 30 2022, Elijah Newren via GitGitGadget wrote: > > > From: Elijah Newren <newren@gmail.com> > > > +test_setup_12l () { > > + test_create_repo 12l_$1 && > > Can & should just be "git init", see f0d4d398e28 (test-lib: split up and > deprecate test_create_repo(), 2021-05-10). I've switched to "git init" and even encouraged others to do the same in other test scripts, but since literally every other test in this file uses test_create_repo, I think they should all be converted at once and just be consistent here. But, so we can stop having this conversation, after this series lands, I'll send one in to update the various merge testfiles that make heavy use of test_create_repo and convert them over. > > + ( > > + cd 12l_$1 && > > + > > + mkdir -p sub1 sub2 > > The "-p" here isn't needed, you're not creating recursive directories. Indeed; will fix. > > + git ls-files -s >out && > > + test_line_count = 5 out && > > Can't this just use test_stdout_line_count? Except... Ooh, new function from late last year that I was unaware of. Yeah, I'm happy to start using it. > > + > > + 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 > > This test seems a bit odd, here we're just checking that we've created > scratch files for the internal use of our test, why is it important that > we have an "out" file, when the only reason we have it is because we > needed it for checking the "ls-files" line count above? Nah, you've misunderstood the purpose of the check. It isn't "make sure that these untracked files are present among whatever else might also be present", it's "make sure the merge step did not introduce *any* untracked files" (something the recursive backend periodically did, and they weren't cruft untracked files but stored actual merge results). There wasn't a nice easy check for that, the closest was to translate the requirement to "make sure the only untracked files are the ones explicitly added by this test script", which is the check you see here. I don't actually care about "actual", "expect", or "out", I just care that there aren't any _other_ untracked files.
On Thu, Jun 30 2022, Elijah Newren wrote: > On Thu, Jun 30, 2022 at 3:26 AM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> >> On Thu, Jun 30 2022, Elijah Newren via GitGitGadget wrote: >> >> > From: Elijah Newren <newren@gmail.com> >> >> > +test_setup_12l () { >> > + test_create_repo 12l_$1 && >> >> Can & should just be "git init", see f0d4d398e28 (test-lib: split up and >> deprecate test_create_repo(), 2021-05-10). > > I've switched to "git init" and even encouraged others to do the same > in other test scripts, but since literally every other test in this > file uses test_create_repo, I think they should all be converted at > once and just be consistent here. But, so we can stop having this > conversation, after this series lands, I'll send one in to update the > various merge testfiles that make heavy use of test_create_repo and > convert them over. Sorry, genuinely I didn't mean to mention it again, just saw it scroll past & wondered if it was intentional. I'm fine with keeping it... >> > + ( >> > + cd 12l_$1 && >> > + >> > + mkdir -p sub1 sub2 >> >> The "-p" here isn't needed, you're not creating recursive directories. > > Indeed; will fix. Thanks! >> > + git ls-files -s >out && >> > + test_line_count = 5 out && >> >> Can't this just use test_stdout_line_count? Except... > > Ooh, new function from late last year that I was unaware of. Yeah, > I'm happy to start using it. > >> > + >> > + 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 >> >> This test seems a bit odd, here we're just checking that we've created >> scratch files for the internal use of our test, why is it important that >> we have an "out" file, when the only reason we have it is because we >> needed it for checking the "ls-files" line count above? > > Nah, you've misunderstood the purpose of the check. It isn't "make > sure that these untracked files are present among whatever else might > also be present", it's "make sure the merge step did not introduce > *any* untracked files" (something the recursive backend periodically > did, and they weren't cruft untracked files but stored actual merge > results). Ah, thanks! > There wasn't a nice easy check for that, the closest was to > translate the requirement to "make sure the only untracked files are > the ones explicitly added by this test script", which is the check you > see here. I don't actually care about "actual", "expect", or "out", I > just care that there aren't any _other_ untracked files. I'm fine with keeping this as-is, but FWIW perhaps this pattern is more explicit about the intent: test_expect_success 'do merge stuff' ' ... && rm -f expect actual && git ls-files -o ':!out' >out && test_must_be_empty out ' Or piping it to ".git/out", to avoid the path exclude, but like this is also fine:) Thanks.
diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh index 479db32cd62..db13bb995f3 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 which 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): sub1/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 #