Regression test for submodules with relative paths

[mbolivar-nordic]

The goal of this PR is to add a regression test for #545.

I'm starting with a regression test that I think captures all the cases:

• a project with submodules: true
• a project with submodules: <list-of-mappings>
• a submodule with both name: and path:
• a submodule with only path:
• a submodule with submodules of its own

All submodule paths are relative. The test is currently failing as expected and is marked xfail as such.

I'm hoping to get some confirmation that this addresses the problem and I haven't missed any use cases as the discussion in the issue is quite long at this point.

[marc-hb]

Thanks for doing this! Just an early look.

[marc-hb]

I'm trying to think of different names for "top" submodules known to west versus sub-submodules not know to west. This is obviously a big difference. How about top-submodule-x-y and rec-submodule-x-y-z? Or project-1-submodule-1 for the top ones?

[mbolivar-nordic]

I think the names are clear enough as-is, especially given the generous comments.

[marc-hb]

submodule-2-2 and submodule-2-2-1 look the same. They look like one is "only one level down" and that's it. But from a west perspective they are totally different. The last one is invisible to west.

[mbolivar-nordic]

I am sorry, but I find this comment very nitpicky. This is just a test case, and a heavily commented test case at that. It isn't like this is user-facing documentation. Can we please just leave this be?

[marc-hb]

For sure this can be modified later if someone finds better names, no urgent need to do anything right now.

I thought you missed my point so I merely tried to rephrase it.

I think good names beat comments any day; one less level of indirection.

Nesting one hierarchy of git repos inside another hierarchy of git repos typically makes my head spin, so I was just trying to find a way to keep it more still. No big deal.

[marc-hb]

I think you need to add these from the bottom up, not top down otherwise the commits at the top will be lagging behind (git status should show that)

[mbolivar-nordic]

I don't follow, sorry. How can I add a sub-submodule for a submodule that isn't added yet?

[marc-hb]

OK so let's say we want A->B->C from parent to child. You certainly don't need A to add C to B (B->C). Plus it's faster to link B->C first because when you add B to A later then C comes with it "for free" without having to go back and perform another commit.

If you do A->B first, then after doing B->C then you need to go back to A to update B to the newer and "bigger" B that includes C.

BTW this test code does not seem to have any commit yet?

[mbolivar-nordic]

You certainly don't need A to add C to B (B->C).

I must be missing something. I think I definitely do need A to add C to B.

I need somewhere to add C. That place is B. I cannot create B without creating A, since B lives inside A.

What am I missing? Can you phrase this suggestion in the form of a diff?

[marc-hb]

I'm not familiar with the repo initialization routines and I'm not sure I have the best solution, but the initialization problem I noticed when reading the code seems confirmed when trying to run it:

tox -- -k submodules_rel --runxfail
cd /tmp/blah/pytest-6/test_update_submodules_relpath0/remotes/

tree project-2

project-2/
├── submodule-2-1
└── submodule-2-2
    └── submodule-2-2-1

git -C project-2/ status

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   submodule-2-2 (new commits)

git clone --recursive project-2 incomplete

tree incomplete/
incomplete/
├── submodule-2-1
└── submodule-2-2

Also, the remotes should all be at the same level. In a real-world example they would all be bare repos at the same level. Relative paths are complicated enough, better not add relative path divergences. It may be possible to simply move the remotes after initialization. As long as they're only used as remotes, only their .git/ will be used so it should hopefully not matter whether the checkout is in some inconsistent state.

[marc-hb]

I cannot create B without creating A, since B lives inside A

I think you're confusing or conflating initial repo creation with adding a submodule.

$ tree
├── A
├── B
└── C

$ git -C B submodule add ../C
Cloning into 'submtest/test/B/C'...

$ git -C B commit -m 'add ../C to B'
[master 616d32033d4a] add ../C to B
 2 files changed, 4 insertions(+)
 create mode 100644 .gitmodules
 create mode 160000 C

$ git -C A submodule add ../B
Cloning into 'submtest/test/A/B'...
done.

$ tree A/
A/
└── B
    └── C

[marc-hb]

I'm working on a fix in parallel along the lines of what we've been discussing, and will update the PR when I have it.

I really recommend getting the test merged test first before any tentative fix: much lower risk of controversy and will help anyone suggesting anything actually test their suggestions (and even submit test fixes if needed)

[mbolivar-nordic]

I really recommend getting the test merged test first

Very well, let's go that way then. Removed wip and addressed comments.

[mbolivar-nordic]

I am cordially inviting @tejlmand to the party :)

[mbolivar-nordic]

Ping on this one

[Mierunski]

Looks good to me

[marc-hb]

Another problem I just noticed is that submodule-1-1, submodule-2-1 etc. all have the same initial commit with the same SHA1. Probably because they're created in the same second. This is a risk of false negatives where a test fetches from the wrong place and passes anyway. Adding a few random characters in the commit message should be enough to address this. EDIT: adding the name of the submodule would probably be better: more useful information and would keep the tests deterministic.

[carlescufi]

LGTM

[Mierunski]

Any progress on this?