diff mbox series

[07/11] t6428: new test for SKIP_WORKTREE handling and conflicts

Message ID 6ccb24b557fc9c9d8e3d307d3e142d8393920414.1614905738.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Complete merge-ort implementation...almost | expand

Commit Message

Elijah Newren March 5, 2021, 12:55 a.m. UTC 
From: Elijah Newren <newren@gmail.com>

If there is a conflict during a merge for a SKIP_WORKTREE entry, we
expect that file to be written to the working copy and have the
SKIP_WORKTREE bit cleared in the index.  If the user had manually
created a file in the working tree despite SKIP_WORKTREE being set, we
do not want to clobber their changes to that file, but want to move it
out of the way.  Add tests that check for these behaviors.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6428-merge-conflicts-sparse.sh | 158 ++++++++++++++++++++++++++++++
 1 file changed, 158 insertions(+)
 create mode 100755 t/t6428-merge-conflicts-sparse.sh

Comments

Ævar Arnfjörð Bjarmason March 8, 2021, 1:03 p.m. UTC | #1 
On Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> If there is a conflict during a merge for a SKIP_WORKTREE entry, we
> expect that file to be written to the working copy and have the
> SKIP_WORKTREE bit cleared in the index.  If the user had manually
> created a file in the working tree despite SKIP_WORKTREE being set, we
> do not want to clobber their changes to that file, but want to move it
> out of the way.  Add tests that check for these behaviors.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  t/t6428-merge-conflicts-sparse.sh | 158 ++++++++++++++++++++++++++++++
>  1 file changed, 158 insertions(+)
>  create mode 100755 t/t6428-merge-conflicts-sparse.sh
>
> diff --git a/t/t6428-merge-conflicts-sparse.sh b/t/t6428-merge-conflicts-sparse.sh
> new file mode 100755
> index 000000000000..1bb52ff6f38c
> --- /dev/null
> +++ b/t/t6428-merge-conflicts-sparse.sh
> @@ -0,0 +1,158 @@
> +#!/bin/sh
> +
> +test_description="merge cases"
> +
> +# The setup for all of them, pictorially, is:
> +#
> +#      A
> +#      o
> +#     / \
> +#  O o   ?
> +#     \ /
> +#      o
> +#      B
> +#
> +# To help make it easier to follow the flow of tests, they have been
> +# divided into sections and each test will start with a quick explanation
> +# of what commits O, A, and B contain.
> +#
> +# Notation:
> +#    z/{b,c}   means  files z/b and z/c both exist
> +#    x/d_1     means  file x/d exists with content d1.  (Purpose of the
> +#                     underscore notation is to differentiate different
> +#                     files that might be renamed into each other's paths.)
> +
> +. ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-merge.sh
> +
> +
> +# Testcase basic, conflicting changes in 'numerals'
> +
> +test_setup_numerals () {
> +	test_create_repo numerals_$1 &&
> +	(
> +		cd numerals_$1 &&
> +
> +		>README &&
> +		test_write_lines I II III >numerals &&
> +		git add README numerals &&
> +		test_tick &&
> +		git commit -m "O" &&

As an aside this could use the --printf option to test_commit I've got
in next, but that's also a bit painful to use since you can't use
test_write_lines.

I've wanted to just support something like this for this use-case of
using an existing file:

    test_write_lines A B C D >lines &&
    test_commit --add O lines &&


> +
> +		git branch O &&
> +		git branch A &&
> +		git branch B &&
> +
> +		git checkout A &&
> +		test_write_lines I II III IIII >numerals &&
> +		git add numerals &&
> +		test_tick &&
> +		git commit -m "A" &&
> +
> +		git checkout B &&
> +		test_write_lines I II III IV >numerals &&
> +		git add numerals &&
> +		test_tick &&
> +		git commit -m "B" &&
> +
> +		cat <<-EOF >expected-index &&
> +		H README
> +		M numerals
> +		M numerals
> +		M numerals
> +		EOF
> +
> +		cat <<-EOF >expected-merge
> +		I
> +		II
> +		III
> +		<<<<<<< HEAD
> +		IIII
> +		=======
> +		IV
> +		>>>>>>> B^0
> +		EOF
> +
> +	)
> +}
> +
> +test_expect_merge_algorithm success failure 'conflicting entries written to worktree even if sparse' '
> +	test_setup_numerals plain &&

A small nit, but makes it easier to debug things: I think having what
you have in "test_setup_numerals" above in a test_expect_success is a
better pattern, then if it fails we can see where exactly.

Then instead of calling "test_setup_numerals" here you'd do:

    cp -R template plain &&

To just copy over that existing setup template, or re-use it and have
have the tests call a small helper to "test_when_finish" reset --hard
back as appropriate.
Elijah Newren March 8, 2021, 8:52 p.m. UTC | #2 
On Mon, Mar 8, 2021 at 5:03 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
>
> On Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > If there is a conflict during a merge for a SKIP_WORKTREE entry, we
> > expect that file to be written to the working copy and have the
> > SKIP_WORKTREE bit cleared in the index.  If the user had manually
> > created a file in the working tree despite SKIP_WORKTREE being set, we
> > do not want to clobber their changes to that file, but want to move it
> > out of the way.  Add tests that check for these behaviors.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  t/t6428-merge-conflicts-sparse.sh | 158 ++++++++++++++++++++++++++++++
> >  1 file changed, 158 insertions(+)
> >  create mode 100755 t/t6428-merge-conflicts-sparse.sh
> >
> > diff --git a/t/t6428-merge-conflicts-sparse.sh b/t/t6428-merge-conflicts-sparse.sh
> > new file mode 100755
> > index 000000000000..1bb52ff6f38c
> > --- /dev/null
> > +++ b/t/t6428-merge-conflicts-sparse.sh
> > @@ -0,0 +1,158 @@
> > +#!/bin/sh
> > +
> > +test_description="merge cases"
> > +
> > +# The setup for all of them, pictorially, is:
> > +#
> > +#      A
> > +#      o
> > +#     / \
> > +#  O o   ?
> > +#     \ /
> > +#      o
> > +#      B
> > +#
> > +# To help make it easier to follow the flow of tests, they have been
> > +# divided into sections and each test will start with a quick explanation
> > +# of what commits O, A, and B contain.
> > +#
> > +# Notation:
> > +#    z/{b,c}   means  files z/b and z/c both exist
> > +#    x/d_1     means  file x/d exists with content d1.  (Purpose of the
> > +#                     underscore notation is to differentiate different
> > +#                     files that might be renamed into each other's paths.)
> > +
> > +. ./test-lib.sh
> > +. "$TEST_DIRECTORY"/lib-merge.sh
> > +
> > +
> > +# Testcase basic, conflicting changes in 'numerals'
> > +
> > +test_setup_numerals () {
> > +     test_create_repo numerals_$1 &&
> > +     (
> > +             cd numerals_$1 &&
> > +
> > +             >README &&
> > +             test_write_lines I II III >numerals &&
> > +             git add README numerals &&
> > +             test_tick &&
> > +             git commit -m "O" &&
>
> As an aside this could use the --printf option to test_commit I've got
> in next, but that's also a bit painful to use since you can't use
> test_write_lines.
>
> I've wanted to just support something like this for this use-case of
> using an existing file:
>
>     test_write_lines A B C D >lines &&
>     test_commit --add O lines &&
>
>
> > +
> > +             git branch O &&
> > +             git branch A &&
> > +             git branch B &&
> > +
> > +             git checkout A &&
> > +             test_write_lines I II III IIII >numerals &&
> > +             git add numerals &&
> > +             test_tick &&
> > +             git commit -m "A" &&
> > +
> > +             git checkout B &&
> > +             test_write_lines I II III IV >numerals &&
> > +             git add numerals &&
> > +             test_tick &&
> > +             git commit -m "B" &&
> > +
> > +             cat <<-EOF >expected-index &&
> > +             H README
> > +             M numerals
> > +             M numerals
> > +             M numerals
> > +             EOF
> > +
> > +             cat <<-EOF >expected-merge
> > +             I
> > +             II
> > +             III
> > +             <<<<<<< HEAD
> > +             IIII
> > +             =======
> > +             IV
> > +             >>>>>>> B^0
> > +             EOF
> > +
> > +     )
> > +}
> > +
> > +test_expect_merge_algorithm success failure 'conflicting entries written to worktree even if sparse' '
> > +     test_setup_numerals plain &&
>
> A small nit, but makes it easier to debug things: I think having what
> you have in "test_setup_numerals" above in a test_expect_success is a
> better pattern, then if it fails we can see where exactly.
>
> Then instead of calling "test_setup_numerals" here you'd do:
>
>     cp -R template plain &&
>
> To just copy over that existing setup template, or re-use it and have
> have the tests call a small helper to "test_when_finish" reset --hard
> back as appropriate.

I used to have tests follow that pattern, but Dscho objected
strenuously to it; see e.g.
https://lore.kernel.org/git/nycvar.QRO.7.76.6.1910141211130.46@tvgsbejvaqbjf.bet/

I went through and replaced most of the merge-recursive-related ones
to the current style to help him out.
diff mbox series

Patch

diff --git a/t/t6428-merge-conflicts-sparse.sh b/t/t6428-merge-conflicts-sparse.sh
new file mode 100755
index 000000000000..1bb52ff6f38c
--- /dev/null
+++ b/t/t6428-merge-conflicts-sparse.sh
@@ -0,0 +1,158 @@ 
+#!/bin/sh
+
+test_description="merge cases"
+
+# The setup for all of them, pictorially, is:
+#
+#      A
+#      o
+#     / \
+#  O o   ?
+#     \ /
+#      o
+#      B
+#
+# To help make it easier to follow the flow of tests, they have been
+# divided into sections and each test will start with a quick explanation
+# of what commits O, A, and B contain.
+#
+# Notation:
+#    z/{b,c}   means  files z/b and z/c both exist
+#    x/d_1     means  file x/d exists with content d1.  (Purpose of the
+#                     underscore notation is to differentiate different
+#                     files that might be renamed into each other's paths.)
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-merge.sh
+
+
+# Testcase basic, conflicting changes in 'numerals'
+
+test_setup_numerals () {
+	test_create_repo numerals_$1 &&
+	(
+		cd numerals_$1 &&
+
+		>README &&
+		test_write_lines I II III >numerals &&
+		git add README numerals &&
+		test_tick &&
+		git commit -m "O" &&
+
+		git branch O &&
+		git branch A &&
+		git branch B &&
+
+		git checkout A &&
+		test_write_lines I II III IIII >numerals &&
+		git add numerals &&
+		test_tick &&
+		git commit -m "A" &&
+
+		git checkout B &&
+		test_write_lines I II III IV >numerals &&
+		git add numerals &&
+		test_tick &&
+		git commit -m "B" &&
+
+		cat <<-EOF >expected-index &&
+		H README
+		M numerals
+		M numerals
+		M numerals
+		EOF
+
+		cat <<-EOF >expected-merge
+		I
+		II
+		III
+		<<<<<<< HEAD
+		IIII
+		=======
+		IV
+		>>>>>>> B^0
+		EOF
+
+	)
+}
+
+test_expect_merge_algorithm success failure 'conflicting entries written to worktree even if sparse' '
+	test_setup_numerals plain &&
+	(
+		cd numerals_plain &&
+
+		git checkout A^0 &&
+
+		test_path_is_file README &&
+		test_path_is_file numerals &&
+
+		git sparse-checkout init &&
+		git sparse-checkout set README &&
+
+		test_path_is_file README &&
+		test_path_is_missing numerals &&
+
+		test_must_fail git merge -s recursive B^0 &&
+
+		git ls-files -t >index_files &&
+		test_cmp expected-index index_files &&
+
+		test_path_is_file README &&
+		test_path_is_file numerals &&
+
+		test_cmp expected-merge numerals &&
+
+		# 4 other files:
+		#   * expected-merge
+		#   * expected-index
+		#   * index_files
+		#   * others
+		git ls-files -o >others &&
+		test_line_count = 4 others
+	)
+'
+
+test_expect_merge_algorithm failure failure 'present-despite-SKIP_WORKTREE handled reasonably' '
+	test_setup_numerals in_the_way &&
+	(
+		cd numerals_in_the_way &&
+
+		git checkout A^0 &&
+
+		test_path_is_file README &&
+		test_path_is_file numerals &&
+
+		git sparse-checkout init &&
+		git sparse-checkout set README &&
+
+		test_path_is_file README &&
+		test_path_is_missing numerals &&
+
+		echo foobar >numerals &&
+
+		test_must_fail git merge -s recursive B^0 &&
+
+		git ls-files -t >index_files &&
+		test_cmp expected-index index_files &&
+
+		test_path_is_file README &&
+		test_path_is_file numerals &&
+
+		test_cmp expected-merge numerals &&
+
+		# There should still be a file with "foobar" in it
+		grep foobar * &&
+
+		# 5 other files:
+		#   * expected-merge
+		#   * expected-index
+		#   * index_files
+		#   * others
+		#   * whatever name was given to the numerals file that had
+		#     "foobar" in it
+		git ls-files -o >others &&
+		test_line_count = 5 others
+	)
+'
+
+test_done