submodule: add more exhaustive up-path testing
PATCH 1/1 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.
---
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(+)
t/t0060-path-utils.sh +13 -0
@@ -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 \
Signature · 1 line
2.18.0.865.gffc8e1a3cd6
Quoted 9 lines · Ævar Arnfjörð Bjarmason
The tests added in
63e95beb08("submodule: portresolve_relative_urlfrom 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
Quoted 6 lines
Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>---
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 3 lines
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?
Quoted 2 lines · Stefan Beller
Thanks for this patch!
Stefan
Thanks, I'd take it as your Acked-by: (please holler if it isn't before the patch hits 'next').
Quoted 12 lines · Stefan Beller
Quoted 9 lines · Ævar Arnfjörð Bjarmason
The tests added in
63e95beb08("submodule: portresolve_relative_urlfrom 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.
Quoted 5 lines
Yeah. Please look at the comment in
builtin/submodule--helper.cin 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...
Quoted 10 lines
Thanks for this patch!
StefanQuoted 6 lines
Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>---
So I think these tests are worthwihle in themselves,
Quoted 8 lines
The reason I put it in the comment instead of tests was the ease of spelling out both the status quo and expectations.
Quoted 3 lines
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.
Quoted 5 lines · Ævar Arnfjörð Bjarmason
Quoted 1 line
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.
Quoted 9 lines · Stefan Beller
Quoted 5 lines · Ævar Arnfjörð Bjarmason
Quoted 1 line
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.
Quoted 7 lines
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?
Quoted 21 lines · Ævar Arnfjörð Bjarmason
Quoted 9 lines · Stefan Beller
Quoted 5 lines · Ævar Arnfjörð Bjarmason
Quoted 1 line
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.
Quoted 7 lines
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.
Quoted 8 lines
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.