diff mbox series

[v2,1/3] t6423: add tests of dual directory rename plus add/add conflict

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

Commit Message

Elijah Newren June 30, 2022, 6:57 a.m. UTC 
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

reversing the direction of the merge provides a different error:

    error: cache entry has null sha1: ...
    fatal: unable to write .git/index

so we add testcases for both.  With these new testcases, the
recursive strategy differs in that it returns the latter error for
both merge directions.

These testcases are somehow a little different than Glen's original
tensorflow testcase in that these ones trigger a bug with the recursive
algorithm whereas his testcase didn't.  I figure that means these
testcases somehow manage to be more comprehensive.

Reported-by: Glen Choo <chooglen@google.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6423-merge-rename-directories.sh | 102 ++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

Comments

Ævar Arnfjörð Bjarmason June 30, 2022, 10:21 a.m. UTC | #1 
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
Elijah Newren July 1, 2022, 2:57 a.m. UTC | #2 
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.
Ævar Arnfjörð Bjarmason July 1, 2022, 9:29 a.m. UTC | #3 
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 mbox series

Patch

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
 #