[PATCH 0/3] Add testcases showing suboptimal submodule/path conflict handling

[PATCH 0/3] Add testcases showing suboptimal submodule/path conflict handling

[Elijah Newren]

Last November, Stefan asked me about an issue with submodules.  In
addition to providing a fix for that issue, I mentioned a few other
problems I noticed with submodules and merging[1].  Turn those issues
into testcases (as I probably should have done back then).

[1] https://public-inbox.org/git/CABPp-BHDrw_dAESic3xK7kC3jMgKeNQuPQF69OpbVYhRkbhJsw@mail.gmail.com/

Elijah Newren (3):
  t7405: add a file/submodule conflict
  t7405: add a directory/submodule conflict
  t7405: verify 'merge --abort' works after submodule/path conflicts

 t/t7405-submodule-merge.sh | 173 +++++++++++++++++++++++++++++++++++++
 1 file changed, 173 insertions(+)

-- 
2.18.0.134.gafc206209.dirty

[PATCH 1/3] t7405: add a file/submodule conflict

[Elijah Newren]

In the case of a file/submodule conflict, although both cannot exist at
the same path, we expect both to be present somewhere for the user to be
able to resolve the conflict with.  Add a testcase for this.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7405-submodule-merge.sh | 56 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
index 7bfb2f498..95fd05d83 100755
--- a/t/t7405-submodule-merge.sh
+++ b/t/t7405-submodule-merge.sh
@@ -279,4 +279,60 @@ test_expect_success 'recursive merge with submodule' '
 	 grep "$(cat expect3)" actual > /dev/null)
 '
 
+# File/submodule conflict
+#   Commit O: <empty>
+#   Commit A: path (submodule)
+#   Commit B: path
+#   Expected: path/ is submodule and file contents for B's path are somewhere
+
+test_expect_success 'setup file/submodule conflict' '
+	test_create_repo file-submodule &&
+	(
+		cd file-submodule &&
+
+		git commit --allow-empty -m O &&
+
+		git branch A &&
+		git branch B &&
+
+		git checkout B &&
+		echo contents >path &&
+		git add path &&
+		git commit -m B &&
+
+		git checkout A &&
+		test_create_repo path &&
+		(
+			cd path &&
+
+			echo hello >world &&
+			git add world &&
+			git commit -m "submodule"
+		) &&
+		git add path &&
+		git commit -m A
+	)
+'
+
+test_expect_failure 'file/submodule conflict' '
+	test_when_finished "git -C file-submodule reset --hard" &&
+	(
+		cd file-submodule &&
+
+		git checkout A^0 &&
+		test_must_fail git merge B^0 >out 2>err &&
+
+		git ls-files -s >out &&
+		test_line_count = 2 out &&
+		git ls-files -u >out &&
+		test_line_count = 2 out &&
+
+		# path/ is still a submodule
+		test_path_is_dir path/.git &&
+
+		echo Checking if contents from B:path showed up anywhere &&
+		grep -q content * 2>/dev/null
+	)
+'
+
 test_done
-- 
2.18.0.134.gafc206209.dirty

[PATCH 2/3] t7405: add a directory/submodule conflict

[Elijah Newren]

For a directory/submodule conflict, we want contents from both the
directory and the submodule to be present for the user to use to resolve
the conflict, but we do not want paths under the directory being written
into the submodule and we do not want the merge being confused by paths
under the submodule being in the way.  Add testcases for these situations.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7405-submodule-merge.sh | 91 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
index 95fd05d83..6cb51c966 100755
--- a/t/t7405-submodule-merge.sh
+++ b/t/t7405-submodule-merge.sh
@@ -335,4 +335,95 @@ test_expect_failure 'file/submodule conflict' '
 	)
 '
 
+# Directory/submodule conflict
+#   Commit O: <empty>
+#   Commit A: path (submodule), with sole tracked file named 'world'
+#   Commit B1: path/file
+#   Commit B2: path/world
+#
+#   Expected from merge of A & B1:
+#     Contents under path/ from commit B1 are renamed elsewhere; we do not
+#     want to write files from one of our tracked directories into a submodule
+#
+#   Expected from merge of A & B2:
+#     Similar to last merge, but with a slight twist: we don't want paths
+#     under the submodule to be treated as untracked or in the way.
+
+test_expect_success 'setup directory/submodule conflict' '
+	test_create_repo directory-submodule &&
+	(
+		cd directory-submodule &&
+
+		git commit --allow-empty -m O &&
+
+		git branch A &&
+		git branch B1 &&
+		git branch B2 &&
+
+		git checkout B1 &&
+		mkdir path &&
+		echo contents >path/file &&
+		git add path/file &&
+		git commit -m B1 &&
+
+		git checkout B2 &&
+		mkdir path &&
+		echo contents >path/world &&
+		git add path/world &&
+		git commit -m B2 &&
+
+		git checkout A &&
+		test_create_repo path &&
+		(
+			cd path &&
+
+			echo hello >world &&
+			git add world &&
+			git commit -m "submodule"
+		) &&
+		git add path &&
+		git commit -m A
+	)
+'
+
+test_expect_failure 'directory/submodule conflict; keep submodule clean' '
+	test_when_finished "git -C directory-submodule reset --hard" &&
+	(
+		cd directory-submodule &&
+
+		git checkout A^0 &&
+		test_must_fail git merge B1^0 &&
+
+		git ls-files -s >out &&
+		test_line_count = 2 out &&
+		git ls-files -u >out &&
+		test_line_count = 1 out &&
+
+		# path/ is still a submodule
+		test_path_is_dir path/.git &&
+
+		echo Checking if contents from B1:path/file showed up &&
+		# Would rather use grep -r, but that is GNU extension...
+		git ls-files -co | xargs grep -q contents 2>/dev/null &&
+		test_path_is_missing path/file
+	)
+'
+
+test_expect_failure 'directory/submodule conflict; should not treat submodule files as untracked or in the way' '
+	test_when_finished "git -C directory-submodule/path reset --hard" &&
+	test_when_finished "git -C directory-submodule reset --hard" &&
+	(
+		cd directory-submodule &&
+
+		git checkout A^0 &&
+		test_must_fail git merge B2^0 >out 2>err &&
+
+		# We do not want files within the submodule to prevent the
+		# merge from starting; we should not be writing to such paths
+		# anyway.
+		test_i18ngrep ! "refusing to lose untracked file at" err
+
+	)
+'
+
 test_done
-- 
2.18.0.134.gafc206209.dirty

[PATCH 3/3] t7405: verify 'merge --abort' works after submodule/path conflicts

[Elijah Newren]

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7405-submodule-merge.sh | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
index 6cb51c966..9f71a4859 100755
--- a/t/t7405-submodule-merge.sh
+++ b/t/t7405-submodule-merge.sh
@@ -335,6 +335,19 @@ test_expect_failure 'file/submodule conflict' '
 	)
 '
 
+test_expect_success 'file/submodule conflict; merge --abort works afterward' '
+	test_when_finished "git -C file-submodule reset --hard" &&
+	(
+		cd file-submodule &&
+
+		git checkout A^0 &&
+		test_must_fail git merge B^0 >out 2>err &&
+
+		test_path_is_file .git/MERGE_HEAD &&
+		git merge --abort
+	)
+'
+
 # Directory/submodule conflict
 #   Commit O: <empty>
 #   Commit A: path (submodule), with sole tracked file named 'world'
@@ -422,7 +435,20 @@ test_expect_failure 'directory/submodule conflict; should not treat submodule fi
 		# merge from starting; we should not be writing to such paths
 		# anyway.
 		test_i18ngrep ! "refusing to lose untracked file at" err
+	)
+'
+
+test_expect_failure 'directory/submodule conflict; merge --abort works afterward' '
+	test_when_finished "git -C directory-submodule/path reset --hard" &&
+	test_when_finished "git -C directory-submodule reset --hard" &&
+	(
+		cd directory-submodule &&
+
+		git checkout A^0 &&
+		test_must_fail git merge B2^0 >out 2>err &&
 
+		test_path_is_file .git/MERGE_HEAD &&
+		git merge --abort
 	)
 '
 
-- 
2.18.0.134.gafc206209.dirty

Re: [PATCH 1/3] t7405: add a file/submodule conflict

[Stefan Beller]

On Sat, Jul 7, 2018 at 1:44 PM Elijah Newren <newren@gmail.com> wrote:
>
> In the case of a file/submodule conflict, although both cannot exist at
> the same path, we expect both to be present somewhere for the user to be
> able to resolve the conflict with.  Add a testcase for this.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  t/t7405-submodule-merge.sh | 56 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>
> diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
> index 7bfb2f498..95fd05d83 100755
> --- a/t/t7405-submodule-merge.sh
> +++ b/t/t7405-submodule-merge.sh
> @@ -279,4 +279,60 @@ test_expect_success 'recursive merge with submodule' '
>          grep "$(cat expect3)" actual > /dev/null)
>  '
>
> +# File/submodule conflict
> +#   Commit O: <empty>
> +#   Commit A: path (submodule)
> +#   Commit B: path
> +#   Expected: path/ is submodule and file contents for B's path are somewhere
> +
> +test_expect_success 'setup file/submodule conflict' '
> +       test_create_repo file-submodule &&
> +       (
> +               cd file-submodule &&
> +
> +               git commit --allow-empty -m O &&
> +
> +               git branch A &&
> +               git branch B &&
> +
> +               git checkout B &&
> +               echo contents >path &&
> +               git add path &&
> +               git commit -m B &&
> +
> +               git checkout A &&
> +               test_create_repo path &&
> +               (
> +                       cd path &&
> +
> +                       echo hello >world &&

    test_commit -C path &&

or do we need a dirty submodule specifically?
If so what is important, the untracked file or
would changes in the index be sufficient?

> +                       git add world &&

when observing this one in verbose mode , you may be
asked to use 'git submodule add' instead
https://github.com/git/git/blob/master/builtin/add.c#L323

> +                       git commit -m "submodule"

Not sure if we'd need the shell it is only test_commit,
the [submodule] add and commit, so maybe we can get away with
3 times -C ?

> +test_expect_failure 'file/submodule conflict' '

Which part fails here?

> +       test_when_finished "git -C file-submodule reset --hard" &&
> +       (
> +               cd file-submodule &&
> +
> +               git checkout A^0 &&
> +               test_must_fail git merge B^0 >out 2>err &&
> +
> +               git ls-files -s >out &&
> +               test_line_count = 2 out &&
> +               git ls-files -u >out &&
> +               test_line_count = 2 out &&
> +
> +               # path/ is still a submodule
> +               test_path_is_dir path/.git &&
> +
> +               echo Checking if contents from B:path showed up anywhere &&

This could be a comment instead?

> +               grep -q content * 2>/dev/null

Ah that is why we needed the specific echo above.

Sorry for being dense here, I am not quite following the last part of the test,
as it is unclear what ought to fail in this test.

Thanks,
Stefan

Re: [PATCH 1/3] t7405: add a file/submodule conflict

[Elijah Newren]

On Mon, Jul 9, 2018 at 2:11 PM, Stefan Beller <sbeller@google.com> wrote:
> On Sat, Jul 7, 2018 at 1:44 PM Elijah Newren <newren@gmail.com> wrote:
>>
>> In the case of a file/submodule conflict, although both cannot exist at
>> the same path, we expect both to be present somewhere for the user to be
>> able to resolve the conflict with.  Add a testcase for this.
>>
>> Signed-off-by: Elijah Newren <newren@gmail.com>
>> ---
>>  t/t7405-submodule-merge.sh | 56 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>>
>> diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
>> index 7bfb2f498..95fd05d83 100755
>> --- a/t/t7405-submodule-merge.sh
>> +++ b/t/t7405-submodule-merge.sh
>> @@ -279,4 +279,60 @@ test_expect_success 'recursive merge with submodule' '
>>          grep "$(cat expect3)" actual > /dev/null)
>>  '
>>
>> +# File/submodule conflict
>> +#   Commit O: <empty>
>> +#   Commit A: path (submodule)
>> +#   Commit B: path
>> +#   Expected: path/ is submodule and file contents for B's path are somewhere
>> +
>> +test_expect_success 'setup file/submodule conflict' '
>> +       test_create_repo file-submodule &&
>> +       (
>> +               cd file-submodule &&
>> +
>> +               git commit --allow-empty -m O &&
>> +
>> +               git branch A &&
>> +               git branch B &&
>> +
>> +               git checkout B &&
>> +               echo contents >path &&

This should have been 'content' rather than 'contents', given my grep below...

>> +               git add path &&
>> +               git commit -m B &&
>> +
>> +               git checkout A &&
>> +               test_create_repo path &&
>> +               (
>> +                       cd path &&
>> +
>> +                       echo hello >world &&
>
>     test_commit -C path &&
>
> or do we need a dirty submodule specifically?
> If so what is important, the untracked file or
> would changes in the index be sufficient?

Do you mean
    test_commit -C path hello
because test_commit needs at least one positional argument (which will
serve as contents, message, filename, and tag)?  Anyway, yeah, doing
this would remove the whole innermost subshell -- the cd, the echo,
the git add, and the git commit.

>
>> +                       git add world &&
>
> when observing this one in verbose mode , you may be
> asked to use 'git submodule add' instead
> https://github.com/git/git/blob/master/builtin/add.c#L323

Um, at this point, I'm adding a regular file -- not a submodule.  Also,
this git add will disappear if I use test_commit.  Perhaps you meant
one of the other 'git add's?

>
>> +                       git commit -m "submodule"
>
> Not sure if we'd need the shell it is only test_commit,
> the [submodule] add and commit, so maybe we can get away with
> 3 times -C ?

This also disappears with test_commit.

>
>> +test_expect_failure 'file/submodule conflict' '
>
> Which part fails here?

Addressed below...

>> +       test_when_finished "git -C file-submodule reset --hard" &&
>> +       (
>> +               cd file-submodule &&
>> +
>> +               git checkout A^0 &&
>> +               test_must_fail git merge B^0 >out 2>err &&

I probably shouldn't have redirected stdout and stderr here; makes it
harder for you to see what's happening, especially since I don't test
either in any way.

>> +
>> +               git ls-files -s >out &&
>> +               test_line_count = 2 out &&
>> +               git ls-files -u >out &&
>> +               test_line_count = 2 out &&
>> +
>> +               # path/ is still a submodule
>> +               test_path_is_dir path/.git &&
>> +
>> +               echo Checking if contents from B:path showed up anywhere &&
>
> This could be a comment instead?
>
>> +               grep -q content * 2>/dev/null
>
> Ah that is why we needed the specific echo above.

Yeah, if there was a handy
test_string_exists_in_some_file_in_current_directory I could have used
that, but seems like an oddly specific special test function, so I
just added an echo so someone watching the test output under --verbose
could see how far the test got.

> Sorry for being dense here, I am not quite following the last part of the test,
> as it is unclear what ought to fail in this test.

Hmm, I obviously wasn't nearly as clear as I thought I was.  Thinking
it over, two things may have thrown you off:

1) I had a typo ('content' vs. 'contents')
2) I didn't just check what was currently failing but other things
that should be true for this test.

For item 2, I've had multiple cases in the past where I created a
minimal test, only to find that if I had more carefully checked the
expected results that it would have prevented a future bug.  Also, it
was harder in the future to figure out, because I no longer remembered
the context for the test setup.  So, in this test I check the contents
of the index, make sure that the submodule is still present, and then
I finally check for the thing that currently fails:

commit B added a file called 'path'; its contents should appear
somewhere in the directory for the user to use when trying to resolve
the failed merge.  It cannot appear at 'path' (there's a submodule in
the way from commit A), but it should be present somewhere, and in
particular I'd expect it in the same directory.  So, I simply grep all
files in the current directory for the string (and ignore errors about
'grep: path is a directory').

Does that help?  If so, I'll submit a reroll with the changes and a
few extra comments.

Re: [PATCH 1/3] t7405: add a file/submodule conflict

[Stefan Beller]

On Tue, Jul 10, 2018 at 8:28 AM Elijah Newren <newren@gmail.com> wrote:
>
> On Mon, Jul 9, 2018 at 2:11 PM, Stefan Beller <sbeller@google.com> wrote:
> > On Sat, Jul 7, 2018 at 1:44 PM Elijah Newren <newren@gmail.com> wrote:
> >>
> >> In the case of a file/submodule conflict, although both cannot exist at
> >> the same path, we expect both to be present somewhere for the user to be
> >> able to resolve the conflict with.  Add a testcase for this.
> >>
> >> Signed-off-by: Elijah Newren <newren@gmail.com>
> >> ---
> >>  t/t7405-submodule-merge.sh | 56 ++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 56 insertions(+)
> >>
> >> diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
> >> index 7bfb2f498..95fd05d83 100755
> >> --- a/t/t7405-submodule-merge.sh
> >> +++ b/t/t7405-submodule-merge.sh
> >> @@ -279,4 +279,60 @@ test_expect_success 'recursive merge with submodule' '
> >>          grep "$(cat expect3)" actual > /dev/null)
> >>  '
> >>
> >> +# File/submodule conflict
> >> +#   Commit O: <empty>
> >> +#   Commit A: path (submodule)
> >> +#   Commit B: path
> >> +#   Expected: path/ is submodule and file contents for B's path are somewhere
> >> +
> >> +test_expect_success 'setup file/submodule conflict' '
> >> +       test_create_repo file-submodule &&
> >> +       (
> >> +               cd file-submodule &&
> >> +
> >> +               git commit --allow-empty -m O &&
> >> +
> >> +               git branch A &&
> >> +               git branch B &&
> >> +
> >> +               git checkout B &&
> >> +               echo contents >path &&
>
> This should have been 'content' rather than 'contents', given my grep below...
>
> >> +               git add path &&
> >> +               git commit -m B &&
> >> +
> >> +               git checkout A &&
> >> +               test_create_repo path &&
> >> +               (
> >> +                       cd path &&
> >> +
> >> +                       echo hello >world &&
> >
> >     test_commit -C path &&
> >
> > or do we need a dirty submodule specifically?
> > If so what is important, the untracked file or
> > would changes in the index be sufficient?
>
> Do you mean
>     test_commit -C path hello
> because test_commit needs at least one positional argument (which will
> serve as contents, message, filename, and tag)?  Anyway, yeah, doing
> this would remove the whole innermost subshell -- the cd, the echo,
> the git add, and the git commit.
>
> >
> >> +                       git add world &&
> >
> > when observing this one in verbose mode , you may be
> > asked to use 'git submodule add' instead
> > https://github.com/git/git/blob/master/builtin/add.c#L323
>
> Um, at this point, I'm adding a regular file -- not a submodule.  Also,
> this git add will disappear if I use test_commit.  Perhaps you meant
> one of the other 'git add's?
>
> >
> >> +                       git commit -m "submodule"
> >
> > Not sure if we'd need the shell it is only test_commit,
> > the [submodule] add and commit, so maybe we can get away with
> > 3 times -C ?
>
> This also disappears with test_commit.

Sorry, I got ahead of my thinking as normally in submodule tests a
shell+commit (in the superproject) is the rough pattern of its setup.

I was wondering if you want to test *repo* vs file or *submodule* vs file
conflict and if we want to treat them differently?
(A submodule really belongs to the project, so we could expect users
to have moved a tree into a submodule or vice versa. But if a repo
appears, we might have a hard time caring as we (or I at the moment)
really have no clue what the setup is intended for.
(With these comments, I think I meant to annotate the part

+               git add path &&
+               git commit -m A

to use "git submodule add ./path")


> >> +               test_must_fail git merge B^0 >out 2>err &&
>
> I probably shouldn't have redirected stdout and stderr here; makes it
> harder for you to see what's happening, especially since I don't test
> either in any way.
>


> >> +               grep -q content * 2>/dev/null
> >
> > Ah that is why we needed the specific echo above.
>
> Yeah, if there was a handy
> test_string_exists_in_some_file_in_current_directory I could have used
> that, but seems like an oddly specific special test function, so I
> just added an echo so someone watching the test output under --verbose
> could see how far the test got.
>
> > Sorry for being dense here, I am not quite following the last part of the test,
> > as it is unclear what ought to fail in this test.
>
> Hmm, I obviously wasn't nearly as clear as I thought I was.  Thinking
> it over, two things may have thrown you off:
>
> 1) I had a typo ('content' vs. 'contents')

I understood that part, or rather I did not spot the typo.

> 2) I didn't just check what was currently failing but other things
> that should be true for this test.
>
> For item 2, I've had multiple cases in the past where I created a
> minimal test, only to find that if I had more carefully checked the
> expected results that it would have prevented a future bug.  Also, it
> was harder in the future to figure out, because I no longer remembered
> the context for the test setup.  So, in this test I check the contents
> of the index, make sure that the submodule is still present, and then
> I finally check for the thing that currently fails:
>
> commit B added a file called 'path'; its contents should appear
> somewhere in the directory for the user to use when trying to resolve
> the failed merge.  It cannot appear at 'path' (there's a submodule in
> the way from commit A), but it should be present somewhere, and in
> particular I'd expect it in the same directory.  So, I simply grep all
> files in the current directory for the string (and ignore errors about
> 'grep: path is a directory').
>
> Does that help?  If so, I'll submit a reroll with the changes and a
> few extra comments.

That does help; yes, I thought
* we want to check for the file content of the file to be somewhere
  (maybe path.file?)
* equally we'd want to have the submodule somewhere; you seem
  to expect it at path (we could have moved it to path.sub or path.git,
  but that involves extra work as we have to update the .gitmodules
  file to have the correct path <->name mapping)
* good call with the index, I skimmed over the ls-files calls not thinking
  what they'd check.

So for now only the "file content is missing" part is failing the test,
whereas the rest is successful.

Thanks for the explanation!
Stefan

Re: [PATCH 1/3] t7405: add a file/submodule conflict

[Elijah Newren]

On Tue, Jul 10, 2018 at 8:53 AM, Stefan Beller <sbeller@google.com> wrote:
> On Tue, Jul 10, 2018 at 8:28 AM Elijah Newren <newren@gmail.com> wrote:

>> 2) I didn't just check what was currently failing but other things
>> that should be true for this test.
>>
>> For item 2, I've had multiple cases in the past where I created a
>> minimal test, only to find that if I had more carefully checked the
>> expected results that it would have prevented a future bug.  Also, it
>> was harder in the future to figure out, because I no longer remembered
>> the context for the test setup.  So, in this test I check the contents
>> of the index, make sure that the submodule is still present, and then
>> I finally check for the thing that currently fails:
>>
>> commit B added a file called 'path'; its contents should appear
>> somewhere in the directory for the user to use when trying to resolve
>> the failed merge.  It cannot appear at 'path' (there's a submodule in
>> the way from commit A), but it should be present somewhere, and in
>> particular I'd expect it in the same directory.  So, I simply grep all
>> files in the current directory for the string (and ignore errors about
>> 'grep: path is a directory').
>>
>> Does that help?  If so, I'll submit a reroll with the changes and a
>> few extra comments.
>
> That does help; yes, I thought
> * we want to check for the file content of the file to be somewhere
>   (maybe path.file?)

Yes.  I wanted to avoid nailing down the expected pathname until the
code was in place that handled this case.  merge-recursive currently
elsewhere uses path~$BRANCH to workaround conflicting paths, but I had
a (half-baked-so-far) idea for changing some of the path-conflict
stuff which would involve different names.  So, I just left it
undecided until implemented.

> * equally we'd want to have the submodule somewhere; you seem
>   to expect it at path (we could have moved it to path.sub or path.git,
>   but that involves extra work as we have to update the .gitmodules
>   file to have the correct path <->name mapping)

Yes I wanted it at path for a specific reason: git can detect renames
of files, and now of directories, but not of submodules, so it seems
more important to keep the submodule where it is when possible.
(merge_file_1() has code running counter to this, but that pre-dates
submodules and should be updated, IMO.)

> * good call with the index, I skimmed over the ls-files calls not thinking
>   what they'd check.
>
> So for now only the "file content is missing" part is failing the test,
> whereas the rest is successful.
>
> Thanks for the explanation!
> Stefan

:-)

[PATCH v2 0/3] Add testcases showing suboptimal submodule/path conflict handling

[Elijah Newren]

This patch series documents a few problems with submodules and merging,
first mentioned at [1].

Changes since v1 (full range-diff below):
  - Incorporate suggestions from Stefan: use test_commit and git
    submodule add (note that git submodule add means there will also
    be a .gitmodules file, so the count of tracked files goes up by
    1)
  - Explain the tests a little better
  - Test the merge --abort case a little more thoroughly.

[1] https://public-inbox.org/git/CABPp-BHDrw_dAESic3xK7kC3jMgKeNQuPQF69OpbVYhRkbhJsw@mail.gmail.com/

Elijah Newren (3):
  t7405: add a file/submodule conflict
  t7405: add a directory/submodule conflict
  t7405: verify 'merge --abort' works after submodule/path conflicts

 t/t7405-submodule-merge.sh | 173 +++++++++++++++++++++++++++++++++++++
 1 file changed, 173 insertions(+)

1:  44bc2a05f ! 1:  16f6622ce t7405: add a file/submodule conflict
    @@ -32,20 +32,14 @@
     +		git branch B &&
     +
     +		git checkout B &&
    -+		echo contents >path &&
    ++		echo content >path &&
     +		git add path &&
     +		git commit -m B &&
     +
     +		git checkout A &&
     +		test_create_repo path &&
    -+		(
    -+			cd path &&
    -+
    -+			echo hello >world &&
    -+			git add world &&
    -+			git commit -m "submodule"
    -+		) &&
    -+		git add path &&
    ++		test_commit -C path world &&
    ++		git submodule add ./path &&
     +		git commit -m A
     +	)
     +'
    @@ -56,16 +50,20 @@
     +		cd file-submodule &&
     +
     +		git checkout A^0 &&
    -+		test_must_fail git merge B^0 >out 2>err &&
    ++		test_must_fail git merge B^0 &&
     +
     +		git ls-files -s >out &&
    -+		test_line_count = 2 out &&
    ++		test_line_count = 3 out &&
     +		git ls-files -u >out &&
     +		test_line_count = 2 out &&
     +
     +		# path/ is still a submodule
     +		test_path_is_dir path/.git &&
     +
    ++		# There is a submodule at "path", so B:path cannot be written
    ++		# there.  We expect it to be written somewhere in the same
    ++		# directory, though, so just grep for its content in all
    ++		# files, and ignore "grep: path: Is a directory" message
     +		echo Checking if contents from B:path showed up anywhere &&
     +		grep -q content * 2>/dev/null
     +	)
2:  2fa139d3b ! 2:  31dc3bc4e t7405: add a directory/submodule conflict
    @@ -56,14 +56,8 @@
     +
     +		git checkout A &&
     +		test_create_repo path &&
    -+		(
    -+			cd path &&
    -+
    -+			echo hello >world &&
    -+			git add world &&
    -+			git commit -m "submodule"
    -+		) &&
    -+		git add path &&
    ++		test_commit -C path hello world &&
    ++		git submodule add ./path &&
     +		git commit -m A
     +	)
     +'
    @@ -77,7 +71,7 @@
     +		test_must_fail git merge B1^0 &&
     +
     +		git ls-files -s >out &&
    -+		test_line_count = 2 out &&
    ++		test_line_count = 3 out &&
     +		git ls-files -u >out &&
     +		test_line_count = 1 out &&
     +
    @@ -87,6 +81,9 @@
     +		echo Checking if contents from B1:path/file showed up &&
     +		# Would rather use grep -r, but that is GNU extension...
     +		git ls-files -co | xargs grep -q contents 2>/dev/null &&
    ++
    ++		# However, B1:path/file should NOT have shown up at path/file,
    ++		# because we should not write into the submodule
     +		test_path_is_missing path/file
     +	)
     +'
3:  b2a44bedf ! 3:  49719952b t7405: verify 'merge --abort' works after submodule/path conflicts
    @@ -39,12 +39,17 @@
     +	test_when_finished "git -C directory-submodule reset --hard" &&
     +	(
     +		cd directory-submodule &&
    -+
    -+		git checkout A^0 &&
    -+		test_must_fail git merge B2^0 >out 2>err &&
      
    ++		git checkout A^0 &&
    ++		test_must_fail git merge B2^0 &&
     +		test_path_is_file .git/MERGE_HEAD &&
    -+		git merge --abort
    ++
    ++		# merge --abort should succeed, should clear .git/MERGE_HEAD,
    ++		# and should not leave behind any conflicted files
    ++		git merge --abort &&
    ++		test_path_is_missing .git/MERGE_HEAD &&
    ++		git ls-files -u >conflicts &&
    ++		test_must_be_empty conflicts
      	)
      '
      
-- 
2.18.0.132.g6e63b23f4

[PATCH v2 1/3] t7405: add a file/submodule conflict

[Elijah Newren]

In the case of a file/submodule conflict, although both cannot exist at
the same path, we expect both to be present somewhere for the user to be
able to resolve the conflict with.  Add a testcase for this.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7405-submodule-merge.sh | 54 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
index 7bfb2f498..62888c2c5 100755
--- a/t/t7405-submodule-merge.sh
+++ b/t/t7405-submodule-merge.sh
@@ -279,4 +279,58 @@ test_expect_success 'recursive merge with submodule' '
 	 grep "$(cat expect3)" actual > /dev/null)
 '
 
+# File/submodule conflict
+#   Commit O: <empty>
+#   Commit A: path (submodule)
+#   Commit B: path
+#   Expected: path/ is submodule and file contents for B's path are somewhere
+
+test_expect_success 'setup file/submodule conflict' '
+	test_create_repo file-submodule &&
+	(
+		cd file-submodule &&
+
+		git commit --allow-empty -m O &&
+
+		git branch A &&
+		git branch B &&
+
+		git checkout B &&
+		echo content >path &&
+		git add path &&
+		git commit -m B &&
+
+		git checkout A &&
+		test_create_repo path &&
+		test_commit -C path world &&
+		git submodule add ./path &&
+		git commit -m A
+	)
+'
+
+test_expect_failure 'file/submodule conflict' '
+	test_when_finished "git -C file-submodule reset --hard" &&
+	(
+		cd file-submodule &&
+
+		git checkout A^0 &&
+		test_must_fail git merge B^0 &&
+
+		git ls-files -s >out &&
+		test_line_count = 3 out &&
+		git ls-files -u >out &&
+		test_line_count = 2 out &&
+
+		# path/ is still a submodule
+		test_path_is_dir path/.git &&
+
+		# There is a submodule at "path", so B:path cannot be written
+		# there.  We expect it to be written somewhere in the same
+		# directory, though, so just grep for its content in all
+		# files, and ignore "grep: path: Is a directory" message
+		echo Checking if contents from B:path showed up anywhere &&
+		grep -q content * 2>/dev/null
+	)
+'
+
 test_done
-- 
2.18.0.132.g6e63b23f4

[PATCH v2 2/3] t7405: add a directory/submodule conflict

[Elijah Newren]

For a directory/submodule conflict, we want contents from both the
directory and the submodule to be present for the user to use to resolve
the conflict, but we do not want paths under the directory being written
into the submodule and we do not want the merge being confused by paths
under the submodule being in the way.  Add testcases for these situations.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7405-submodule-merge.sh | 88 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
index 62888c2c5..45d1779d2 100755
--- a/t/t7405-submodule-merge.sh
+++ b/t/t7405-submodule-merge.sh
@@ -333,4 +333,92 @@ test_expect_failure 'file/submodule conflict' '
 	)
 '
 
+# Directory/submodule conflict
+#   Commit O: <empty>
+#   Commit A: path (submodule), with sole tracked file named 'world'
+#   Commit B1: path/file
+#   Commit B2: path/world
+#
+#   Expected from merge of A & B1:
+#     Contents under path/ from commit B1 are renamed elsewhere; we do not
+#     want to write files from one of our tracked directories into a submodule
+#
+#   Expected from merge of A & B2:
+#     Similar to last merge, but with a slight twist: we don't want paths
+#     under the submodule to be treated as untracked or in the way.
+
+test_expect_success 'setup directory/submodule conflict' '
+	test_create_repo directory-submodule &&
+	(
+		cd directory-submodule &&
+
+		git commit --allow-empty -m O &&
+
+		git branch A &&
+		git branch B1 &&
+		git branch B2 &&
+
+		git checkout B1 &&
+		mkdir path &&
+		echo contents >path/file &&
+		git add path/file &&
+		git commit -m B1 &&
+
+		git checkout B2 &&
+		mkdir path &&
+		echo contents >path/world &&
+		git add path/world &&
+		git commit -m B2 &&
+
+		git checkout A &&
+		test_create_repo path &&
+		test_commit -C path hello world &&
+		git submodule add ./path &&
+		git commit -m A
+	)
+'
+
+test_expect_failure 'directory/submodule conflict; keep submodule clean' '
+	test_when_finished "git -C directory-submodule reset --hard" &&
+	(
+		cd directory-submodule &&
+
+		git checkout A^0 &&
+		test_must_fail git merge B1^0 &&
+
+		git ls-files -s >out &&
+		test_line_count = 3 out &&
+		git ls-files -u >out &&
+		test_line_count = 1 out &&
+
+		# path/ is still a submodule
+		test_path_is_dir path/.git &&
+
+		echo Checking if contents from B1:path/file showed up &&
+		# Would rather use grep -r, but that is GNU extension...
+		git ls-files -co | xargs grep -q contents 2>/dev/null &&
+
+		# However, B1:path/file should NOT have shown up at path/file,
+		# because we should not write into the submodule
+		test_path_is_missing path/file
+	)
+'
+
+test_expect_failure 'directory/submodule conflict; should not treat submodule files as untracked or in the way' '
+	test_when_finished "git -C directory-submodule/path reset --hard" &&
+	test_when_finished "git -C directory-submodule reset --hard" &&
+	(
+		cd directory-submodule &&
+
+		git checkout A^0 &&
+		test_must_fail git merge B2^0 >out 2>err &&
+
+		# We do not want files within the submodule to prevent the
+		# merge from starting; we should not be writing to such paths
+		# anyway.
+		test_i18ngrep ! "refusing to lose untracked file at" err
+
+	)
+'
+
 test_done
-- 
2.18.0.132.g6e63b23f4

[PATCH v2 3/3] t7405: verify 'merge --abort' works after submodule/path conflicts

[Elijah Newren]

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7405-submodule-merge.sh | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
index 45d1779d2..7855bd864 100755
--- a/t/t7405-submodule-merge.sh
+++ b/t/t7405-submodule-merge.sh
@@ -333,6 +333,19 @@ test_expect_failure 'file/submodule conflict' '
 	)
 '
 
+test_expect_success 'file/submodule conflict; merge --abort works afterward' '
+	test_when_finished "git -C file-submodule reset --hard" &&
+	(
+		cd file-submodule &&
+
+		git checkout A^0 &&
+		test_must_fail git merge B^0 >out 2>err &&
+
+		test_path_is_file .git/MERGE_HEAD &&
+		git merge --abort
+	)
+'
+
 # Directory/submodule conflict
 #   Commit O: <empty>
 #   Commit A: path (submodule), with sole tracked file named 'world'
@@ -417,7 +430,25 @@ test_expect_failure 'directory/submodule conflict; should not treat submodule fi
 		# merge from starting; we should not be writing to such paths
 		# anyway.
 		test_i18ngrep ! "refusing to lose untracked file at" err
+	)
+'
+
+test_expect_failure 'directory/submodule conflict; merge --abort works afterward' '
+	test_when_finished "git -C directory-submodule/path reset --hard" &&
+	test_when_finished "git -C directory-submodule reset --hard" &&
+	(
+		cd directory-submodule &&
 
+		git checkout A^0 &&
+		test_must_fail git merge B2^0 &&
+		test_path_is_file .git/MERGE_HEAD &&
+
+		# merge --abort should succeed, should clear .git/MERGE_HEAD,
+		# and should not leave behind any conflicted files
+		git merge --abort &&
+		test_path_is_missing .git/MERGE_HEAD &&
+		git ls-files -u >conflicts &&
+		test_must_be_empty conflicts
 	)
 '
 
-- 
2.18.0.132.g6e63b23f4