Subject: Re: [RFC PATCH 1/5] Add testcases for improved file collision conflict handling

From: SZEDER Gábor <[email-masked]>
Date: Thu, 8 Mar 2018 13:25:23 +0100

> Adds testcases dealing with file collisions for the following types of
> conflicts:
>   * add/add
>   * rename/add
>   * rename/rename(2to1)
> These tests include expectations for proposed smarter behavior which has
> not yet been implemented and thus are currently expected to fail.
> Subsequent commits will correct that and explain the new behavior.
> 
> Signed-off-by: Elijah Newren <[email-masked]>
> ---
>  t/t6042-merge-rename-corner-cases.sh | 220 +++++++++++++++++++++++++++++++++++
>  1 file changed, 220 insertions(+)
> 
> diff --git a/t/t6042-merge-rename-corner-cases.sh b/t/t6042-merge-rename-corner-cases.sh
> index 411550d2b6..a6c151ef95 100755
> --- a/t/t6042-merge-rename-corner-cases.sh
> +++ b/t/t6042-merge-rename-corner-cases.sh
> @@ -575,4 +575,224 @@ test_expect_success 'rename/rename/add-dest merge still knows about conflicting
>  	test ! -f c
>  '
>
> +test_conflicts_with_adds_and_renames() {
> +	test $1 != 0 && side1=rename || side1=add
> +	test $2 != 0 && side2=rename || side2=add

For additonal context I'm going to quote the callsites of this
function from the end of the test script:

> +test_conflicts_with_adds_and_renames 1 1
> +test_conflicts_with_adds_and_renames 1 0
> +test_conflicts_with_adds_and_renames 0 1
> +test_conflicts_with_adds_and_renames 0 0

Instead of the two conditions at the beginning of the function and the
1 and 0 sort-of magic numbers at the callsites, you could just pass
the words "rename" and "add" as parameters to the function.  The
callsites would be clearer and the function could start with two
simple variable assignments side1=$1 ; side2=$2.

Please feel free to dismiss this as bikeshedding: since the branches
are called 'L' and 'R', maybe calling the variables $sideL and $sideR
would match better the rest of the test?  Dunno.

> +	# Setup:
> +	#          L
> +	#         / \
> +	#   master   ?
> +	#         \ /
> +	#          R
> +	#
> +	# Where:
> +	#   Both L and R have files named 'three-unrelated' and
> +	#   'three-related' which collide (i.e. 4 files colliding at two
> +	#   pathnames).  Each of the colliding files could have been
> +	#   involved in a rename, in which case there was a file named
> +	#   'one-[un]related' or 'two-[un]related' that was modified on the
> +	#   opposite side of history and renamed into the collision on this
> +	#   side of history.
> +	#
> +	# Questions for both sets of collisions:
> +	#   1) The index should contain both a stage 2 and stage 3 entry
> +	#      for the colliding file.  Does it?
> +	#   2) When renames are involved, the content merges are clean, so
> +	#      the index should reflect the content merges, not merely the
> +	#      version of the colliding file from the prior commit.  Does
> +	#      it?
> +	#
> +	# Questions for three-unrelated:
> +	#   3) There should be files in the worktree named
> +	#      'three-unrelated~HEAD' and 'three-unrelated~R^0' with the
> +	#      (content-merged) version of 'three-unrelated' from the
> +	#      appropriate side of the merge.  Are they present?
> +	#   4) There should be no file named 'three-unrelated' in the
> +	#      working tree.  That'd make it too likely that users would
> +	#      use it instead of carefully looking at both
> +	#      three-unrelated~HEAD and three-unrelated~R^0.  Is it
> +	#      correctly missing?
> +	#
> +	# Questions for three-related:
> +	#   3) There should be a file in the worktree named three-related
> +	#      containing the two-way merged contents of the content-merged
> +	#      versions of three-related from each of the two colliding
> +	#      files.  Is it present?
> +	#   4) There should not be any three-related~* files in the working
> +	#      tree.
> +	test_expect_success "setup simple $side1/$side2 conflict" '
> +		test_create_repo simple_${side1}_${side2} &&
> +		(
> +			cd simple_${side1}_${side2} &&
> +
> +			# Create a simple file with 10 lines
> +			ten="0 1 2 3 4 5 6 7 8 9" &&
> +			for i in $ten
> +			do
> +				echo line $i in a sample file
> +			done >unrelated1_v1 &&
> +			# Create a 2nd version of same file with one more line
> +			cat unrelated1_v1 >unrelated1_v2 &&

'cp unrelated1_v1 unrelated1_v2', perhaps?

> +			echo another line >>unrelated1_v2 &&
> +
> +			# Create an unrelated simple file with 10 lines
> +			for i in $ten
> +			do
> +				echo line $i in another sample file
> +			done >unrelated2_v1 &&
> +			# Create a 2nd version of same file with one more line
> +			cat unrelated2_v1 >unrelated2_v2 &&

Likewise.

> +			echo another line >>unrelated2_v2 &&
> +
> +			# Create some related files now
> +			for i in $ten
> +			do
> +				echo Random base content line $i
> +			done >related1_v1 &&
> +			cp -a related1_v1 related1_v2 &&

Wouldn't a "plain" 'cp', i.e. without '-a', be sufficient?

> +			echo modification >>related1_v2 &&
> +
> +			cp -a related1_v1 related2_v1 &&
> +			echo more stuff >>related2_v1 &&
> +			cp -a related2_v1 related2_v2 &&
> +			echo yet more stuff >>related2_v2 &&
> +
> +			# Use a tag to record both these files for simple
> +			# access, and clean out these untracked files
> +			git tag unrelated1_v1 `git hash-object -w unrelated1_v1` &&
> +			git tag unrelated1_v2 `git hash-object -w unrelated1_v2` &&
> +			git tag unrelated2_v1 `git hash-object -w unrelated2_v1` &&
> +			git tag unrelated2_v2 `git hash-object -w unrelated2_v2` &&
> +			git tag related1_v1 `git hash-object -w related1_v1` &&
> +			git tag related1_v2 `git hash-object -w related1_v2` &&
> +			git tag related2_v1 `git hash-object -w related2_v1` &&
> +			git tag related2_v2 `git hash-object -w related2_v2` &&

Style nit: please use $() for command substitutions instead of
backticks.

> +			git clean -f &&
> +
> +			# Setup merge-base, consisting of files named "one-*"
> +			# and "two-*" if renames were involved.
> +			touch irrelevant_file &&
> +			git add irrelevant_file &&
> +			if [ $side1 == "rename" ]; then

Another style nit:

        if test $side1 = "rename"
        then
                ...

> +				git show unrelated1_v1 >one-unrelated &&
> +				git add one-unrelated

Broken && chain.
Please check the subsequent if statements as well, there are more
places where the && is missing.

Also note that you can run 'git add one-unrelated one-related', i.e.
add more than one file at once, sparing a couple of lines and git
executions.

> +				git show related1_v1 >one-related &&
> +				git add one-related
> +			fi &&
> +			if [ $side2 == "rename" ]; then
> +				git show unrelated2_v1 >two-unrelated &&
> +				git add two-unrelated
> +				git show related2_v1 >two-related &&
> +				git add two-related
> +			fi &&
> +			test_tick && git commit -m initial &&
> +
> +			git branch L &&
> +			git branch R &&
> +
> +			# Handle the left side
> +			git checkout L &&
> +			if [ $side1 == "rename" ]; then
> +				git mv one-unrelated three-unrelated
> +				git mv one-related   three-related
> +			else
> +				git show unrelated1_v2 >three-unrelated &&
> +				git add three-unrelated
> +				git show related1_v2 >three-related &&
> +				git add three-related
> +			fi &&
> +			if [ $side2 == "rename" ]; then
> +				git show unrelated2_v2 >two-unrelated &&
> +				git add two-unrelated
> +				git show related2_v2 >two-related &&
> +				git add two-related
> +			fi &&
> +			test_tick && git commit -m L &&
> +
> +			# Handle the right side
> +			git checkout R &&
> +			if [ $side1 == "rename" ]; then
> +				git show unrelated1_v2 >one-unrelated &&
> +				git add one-unrelated
> +				git show related1_v2 >one-related &&
> +				git add one-related
> +			fi &&
> +			if [ $side2 == "rename" ]; then
> +				git mv two-unrelated three-unrelated
> +				git mv two-related three-related
> +			else
> +				git show unrelated2_v2 >three-unrelated &&
> +				git add three-unrelated
> +				git show related2_v2 >three-related &&
> +				git add three-related
> +			fi &&
> +			test_tick && git commit -m R
> +		)
> +	'

This setup test is enormous, and the conditions for the combination of
the two sides and the add/rename conflicts are somewhat distracting.
I don't know how it could be structured better/shorter/clearer...  I
couldn't come up with anything useful during lunch.

> +
> +	test_expect_failure "check simple $side1/$side2 conflict" '
> +		(
> +			cd simple_${side1}_${side2} &&
> +
> +			git checkout L^0 &&
> +
> +			# Merge must fail; there is a conflict
> +			test_must_fail git merge -s recursive R^0 &&
> +
> +			# Make sure the index has the right number of entries
> +			git ls-files -s >out &&
> +			test_line_count = 5 out &&
> +			git ls-files -u >out &&
> +			test_line_count = 4 out &&
> +
> +			# Nothing should have touched irrelevant_file
> +			git rev-parse >actual \
> +				:0:irrelevant_file \
> +				:2:three-unrelated :3:three-unrelated \
> +				:2:three-related   :3:three-related   &&
> +			git rev-parse >expected \
> +				master:irrelevant_file \
> +				unrelated1_v2      unrelated2_v2 \
> +				related1_v2        related2_v2   &&

Missing 'test_cmp'?  The above lines write the files 'actual' and
'expected', but they are never looked at.

> +			# Ensure we have the correct number of untracked files
> +			git ls-files -o >out &&
> +			test_line_count = 5 out &&
> +
> +			# Make sure each file (with merging if rename
> +			# involved) is present in the working tree for the
> +			# user to work with.
> +			git hash-object >actual \
> +				three-unrelated~HEAD three-unrelated~R^0 &&
> +			git rev-parse >expected \
> +				unrelated1_v2        unrelated2_v2 &&

Again, missing 'test_cmp'?

> +			# "three-unrelated" should not exist because there is
> +			# no reason to give preference to either
> +			# three-unrelated~HEAD or three-unrelated~R^0
> +			test_path_is_missing three-unrelated &&
> +
> +			# Make sure we have the correct merged contents for
> +			# three-related
> +			git show related1_v1 >expected &&
> +			cat <<EOF >>expected &&
> +<<<<<<< HEAD
> +modification
> +=======
> +more stuff
> +yet more stuff
> +>>>>>>> R^0
> +EOF

You could use 'cat <<-EOF ....' and indent the here doc with tabs, so
it won't look so out-of-place.  Or even '<<-\EOF' to indicate that
there is nothing to be expanded in the here doc. 

> +
> +			test_cmp expected three-related
> +		)
> +	'
> +}
> +
> +test_conflicts_with_adds_and_renames 1 1
> +test_conflicts_with_adds_and_renames 1 0
> +test_conflicts_with_adds_and_renames 0 1
> +test_conflicts_with_adds_and_renames 0 0
> +
>  test_done
> --
> 2.16.0.41.g6a66043158
> 
>