[PATCH] submodule: add more exhaustive up-path testing

The tests added in 63e95beb08 ("submodule: port resolve_relative_url
from shell to C", 2016-04-15) didn't do a good job of testing various
up-path invocations where the up-path would bring us beyond even the
URL in question without emitting an error.

These results look nonsensical, but it's worth exhaustively testing
them before fixing any of this code, so we can see which of these
cases were changed.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---

The reason I started to poke at this was because I wanted to add
support for relative URLs like //group/project.

This solves the problem of there being e.g. a myhost.com/group/project
which includes a ../submodule submodule pointing to
myhost.com/group/submodule , and me wanting to fork it to
myhost.com/myuser/project.

But of course with ../submodule the relative URL will point to
myhost.com/myuser/submodule, which won't exist.

So I was hoping to find some point in the relative URL code where we
were actually aware of what the host boundary was, so I could just cut
off everything after that if //group/project was provided, strip off
one slash to make it /group/project, and call it a day.

But as the tests below show the code isn't aware of that at all, and
will happily trample over the host(name) to produce nonsensical
results.

So I think these tests are worthwihle in themselves, but would like
some advice on how to proceed with that from someone more familiar
with submodules.

 t/t0060-path-utils.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 21a8b53132..cd74c0a471 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh

@@ -330,6 +330,9 @@ test_submodule_relative_url "(null)" "../foo" "../submodule" "../submodule"
 test_submodule_relative_url "(null)" "./foo/bar" "../submodule" "foo/submodule"
 test_submodule_relative_url "(null)" "./foo" "../submodule" "submodule"
 test_submodule_relative_url "(null)" "//somewhere else/repo" "../subrepo" "//somewhere else/subrepo"
+test_submodule_relative_url "(null)" "//somewhere else/repo" "../../subrepo" "//subrepo"
+test_submodule_relative_url "(null)" "//somewhere else/repo" "../../../subrepo" "/subrepo"
+test_submodule_relative_url "(null)" "//somewhere else/repo" "../../../../subrepo" "subrepo"
 test_submodule_relative_url "(null)" "$(pwd)/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
 test_submodule_relative_url "(null)" "$(pwd)/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
 test_submodule_relative_url "(null)" "$(pwd)/." "../." "$(pwd)/."

@@ -344,10 +347,20 @@ test_submodule_relative_url "(null)" "file:///tmp/repo" "../subrepo" "file:///tm
 test_submodule_relative_url "(null)" "foo/bar" "../submodule" "foo/submodule"
 test_submodule_relative_url "(null)" "foo" "../submodule" "submodule"
 test_submodule_relative_url "(null)" "helper:://hostname/repo" "../subrepo" "helper:://hostname/subrepo"
+test_submodule_relative_url "(null)" "helper:://hostname/repo" "../../subrepo" "helper:://subrepo"
+test_submodule_relative_url "(null)" "helper:://hostname/repo" "../../../subrepo" "helper::/subrepo"
+test_submodule_relative_url "(null)" "helper:://hostname/repo" "../../../../subrepo" "helper::subrepo"
+test_submodule_relative_url "(null)" "helper:://hostname/repo" "../../../../../subrepo" "helper:subrepo"
+test_submodule_relative_url "(null)" "helper:://hostname/repo" "../../../../../../subrepo" ".:subrepo"
 test_submodule_relative_url "(null)" "ssh://hostname/repo" "../subrepo" "ssh://hostname/subrepo"
+test_submodule_relative_url "(null)" "ssh://hostname/repo" "../../subrepo" "ssh://subrepo"
+test_submodule_relative_url "(null)" "ssh://hostname/repo" "../../../subrepo" "ssh:/subrepo"
+test_submodule_relative_url "(null)" "ssh://hostname/repo" "../../../../subrepo" "ssh:subrepo"
+test_submodule_relative_url "(null)" "ssh://hostname/repo" "../../../../../subrepo" ".:subrepo"
 test_submodule_relative_url "(null)" "ssh://hostname:22/repo" "../subrepo" "ssh://hostname:22/subrepo"
 test_submodule_relative_url "(null)" "user@host:path/to/repo" "../subrepo" "user@host:path/to/subrepo"
 test_submodule_relative_url "(null)" "user@host:repo" "../subrepo" "user@host:subrepo"
+test_submodule_relative_url "(null)" "user@host:repo" "../../subrepo" ".:subrepo"
 
 test_expect_success 'match .gitmodules' '
 	test-tool path-utils is_dotgitmodules \

-- 
2.18.0.865.gffc8e1a3cd6

On Tue, Aug 14, 2018 at 11:59 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

The tests added in 63e95beb08 ("submodule: port resolve_relative_url
from shell to C", 2016-04-15) didn't do a good job of testing various
up-path invocations where the up-path would bring us beyond even the
URL in question without emitting an error.

These results look nonsensical, but it's worth exhaustively testing
them before fixing any of this code, so we can see which of these
cases were changed.

Yeah. Please look at the comment in builtin/submodule--helper.c
in that commit, where I described my expectations.

I should have put them into tests instead with the expectations
spelled out there.

Thanks for this patch!
Stefan

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---

So I think these tests are worthwihle in themselves,

The reason I put it in the comment instead of tests was the
ease of spelling out both the status quo and expectations.

but would like
some advice on how to proceed with that from someone more familiar
with submodules.

So ideally we'd also error out as soon as the host name is touched?

On Tue, Aug 14 2018, Stefan Beller wrote:

On Tue, Aug 14, 2018 at 11:59 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

The tests added in 63e95beb08 ("submodule: port resolve_relative_url
from shell to C", 2016-04-15) didn't do a good job of testing various
up-path invocations where the up-path would bring us beyond even the
URL in question without emitting an error.

These results look nonsensical, but it's worth exhaustively testing
them before fixing any of this code, so we can see which of these
cases were changed.

Yeah. Please look at the comment in builtin/submodule--helper.c
in that commit, where I described my expectations.

I should have put them into tests instead with the expectations
spelled out there.

I'll check that out thanks. I saw that comment, but have been skimming
most of this code...

Thanks for this patch!
Stefan

quoted

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---

quoted

So I think these tests are worthwihle in themselves,

The reason I put it in the comment instead of tests was the
ease of spelling out both the status quo and expectations.

quoted

but would like
some advice on how to proceed with that from someone more familiar
with submodules.

So ideally we'd also error out as soon as the host name is touched?

Do we have some utility function that'll take whatever we have in
remote.<name>.url and spew out the username / host / path? We must,
since the clone protocol needs it, but I haven't found it.

On Tue, Aug 14, 2018 at 2:05 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

So ideally we'd also error out as soon as the host name is touched?

Do we have some utility function that'll take whatever we have in
remote.<name>.url and spew out the username / host / path? We must,
since the clone protocol needs it, but I haven't found it.

Nope. Welcome to the wonderful world of submodules.
As submodules are in the transition state towards "in C",
we could do some refactorings that would allow for easy access to
these properties, but the transition is done via faithful conversion from
shell to C, which did not have these functions either, but could just
do some string hackery. And that is how we ended up with this
function in the first place.

On Tue, Aug 14 2018, Stefan Beller wrote:

On Tue, Aug 14, 2018 at 2:05 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

quoted

So ideally we'd also error out as soon as the host name is touched?

Do we have some utility function that'll take whatever we have in
remote.<name>.url and spew out the username / host / path? We must,
since the clone protocol needs it, but I haven't found it.

Nope. Welcome to the wonderful world of submodules.
As submodules are in the transition state towards "in C",
we could do some refactorings that would allow for easy access to
these properties, but the transition is done via faithful conversion from
shell to C, which did not have these functions either, but could just
do some string hackery. And that is how we ended up with this
function in the first place.

The remote/clone machinery is in C and so is the resolve-relative-url
helper. What's the missing bridge here?

Maybe we don't have that function yet, but we can presumably expose it,
and this seems unrelated to some other bits of submodules being in
shellscript.

No?

On Tue, Aug 14, 2018 at 2:16 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Tue, Aug 14 2018, Stefan Beller wrote:

quoted

On Tue, Aug 14, 2018 at 2:05 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

quoted

So ideally we'd also error out as soon as the host name is touched?

Do we have some utility function that'll take whatever we have in
remote.<name>.url and spew out the username / host / path? We must,
since the clone protocol needs it, but I haven't found it.

Nope. Welcome to the wonderful world of submodules.
As submodules are in the transition state towards "in C",
we could do some refactorings that would allow for easy access to
these properties, but the transition is done via faithful conversion from
shell to C, which did not have these functions either, but could just
do some string hackery. And that is how we ended up with this
function in the first place.

The remote/clone machinery is in C and so is the resolve-relative-url
helper. What's the missing bridge here?

Maybe we don't have that function yet, but we can presumably expose it,
and this seems unrelated to some other bits of submodules being in
shellscript.

No?

Gah, I misread your original question. Sorry about that.