On Tue, Jul 10, 2018 at 8:28 AM Elijah Newren <newren@xxxxxxxxx> wrote: > > On Mon, Jul 9, 2018 at 2:11 PM, Stefan Beller <sbeller@xxxxxxxxxx> wrote: > > On Sat, Jul 7, 2018 at 1:44 PM Elijah Newren <newren@xxxxxxxxx> 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@xxxxxxxxx> > >> --- > >> 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
