{"thread":{"id":"49125","subject":"[PATCH] submodule: add more exhaustive up-path testing","startedAt":"2018-08-14T18:59:21Z","lastAt":"2018-08-14T21:32:44Z","messageCount":7,"participants":["Ævar Arnfjörð Bjarmason","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"355594","messageId":"20180814185906.2680-1-avarab@gmail.com","threadId":"49125","inReplyTo":null,"subject":"[PATCH] submodule: add more exhaustive up-path testing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-14T18:59:06Z","receivedAt":"2018-08-14T18:59:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The tests added in 63e95beb08 (\"submodule: port resolve_relative_url\nfrom shell to C\", 2016-04-15) didn't do a good job of testing various\nup-path invocations where the up-path would bring us beyond even the\nURL in question without emitting an error.\n\nThese results look nonsensical, but it's worth exhaustively testing\nthem before fixing any of this code, so we can see which of these\ncases were changed.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nThe reason I started to poke at this was because I wanted to add\nsupport for relative URLs like //group/project.\n\nThis solves the problem of there being e.g. a myhost.com/group/project\nwhich includes a ../submodule submodule pointing to\nmyhost.com/group/submodule , and me wanting to fork it to\nmyhost.com/myuser/project.\n\nBut of course with ../submodule the relative URL will point to\nmyhost.com/myuser/submodule, which won't exist.\n\nSo I was hoping to find some point in the relative URL code where we\nwere actually aware of what the host boundary was, so I could just cut\noff everything after that if //group/project was provided, strip off\none slash to make it /group/project, and call it a day.\n\nBut as the tests below show the code isn't aware of that at all, and\nwill happily trample over the host(name) to produce nonsensical\nresults.\n\nSo I think these tests are worthwihle in themselves, but would like\nsome advice on how to proceed with that from someone more familiar\nwith submodules.\n\n t/t0060-path-utils.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 21a8b53132..cd74c0a471 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -330,6 +330,9 @@ test_submodule_relative_url \"(null)\" \"../foo\" \"../submodule\" \"../submodule\"\n test_submodule_relative_url \"(null)\" \"./foo/bar\" \"../submodule\" \"foo/submodule\"\n test_submodule_relative_url \"(null)\" \"./foo\" \"../submodule\" \"submodule\"\n test_submodule_relative_url \"(null)\" \"//somewhere else/repo\" \"../subrepo\" \"//somewhere else/subrepo\"\n+test_submodule_relative_url \"(null)\" \"//somewhere else/repo\" \"../../subrepo\" \"//subrepo\"\n+test_submodule_relative_url \"(null)\" \"//somewhere else/repo\" \"../../../subrepo\" \"/subrepo\"\n+test_submodule_relative_url \"(null)\" \"//somewhere else/repo\" \"../../../../subrepo\" \"subrepo\"\n test_submodule_relative_url \"(null)\" \"$(pwd)/subsuper_update_r\" \"../subsubsuper_update_r\" \"$(pwd)/subsubsuper_update_r\"\n test_submodule_relative_url \"(null)\" \"$(pwd)/super_update_r2\" \"../subsuper_update_r\" \"$(pwd)/subsuper_update_r\"\n test_submodule_relative_url \"(null)\" \"$(pwd)/.\" \"../.\" \"$(pwd)/.\"\n@@ -344,10 +347,20 @@ test_submodule_relative_url \"(null)\" \"file:///tmp/repo\" \"../subrepo\" \"file:///tm\n test_submodule_relative_url \"(null)\" \"foo/bar\" \"../submodule\" \"foo/submodule\"\n test_submodule_relative_url \"(null)\" \"foo\" \"../submodule\" \"submodule\"\n test_submodule_relative_url \"(null)\" \"helper:://hostname/repo\" \"../subrepo\" \"helper:://hostname/subrepo\"\n+test_submodule_relative_url \"(null)\" \"helper:://hostname/repo\" \"../../subrepo\" \"helper:://subrepo\"\n+test_submodule_relative_url \"(null)\" \"helper:://hostname/repo\" \"../../../subrepo\" \"helper::/subrepo\"\n+test_submodule_relative_url \"(null)\" \"helper:://hostname/repo\" \"../../../../subrepo\" \"helper::subrepo\"\n+test_submodule_relative_url \"(null)\" \"helper:://hostname/repo\" \"../../../../../subrepo\" \"helper:subrepo\"\n+test_submodule_relative_url \"(null)\" \"helper:://hostname/repo\" \"../../../../../../subrepo\" \".:subrepo\"\n test_submodule_relative_url \"(null)\" \"ssh://hostname/repo\" \"../subrepo\" \"ssh://hostname/subrepo\"\n+test_submodule_relative_url \"(null)\" \"ssh://hostname/repo\" \"../../subrepo\" \"ssh://subrepo\"\n+test_submodule_relative_url \"(null)\" \"ssh://hostname/repo\" \"../../../subrepo\" \"ssh:/subrepo\"\n+test_submodule_relative_url \"(null)\" \"ssh://hostname/repo\" \"../../../../subrepo\" \"ssh:subrepo\"\n+test_submodule_relative_url \"(null)\" \"ssh://hostname/repo\" \"../../../../../subrepo\" \".:subrepo\"\n test_submodule_relative_url \"(null)\" \"ssh://hostname:22/repo\" \"../subrepo\" \"ssh://hostname:22/subrepo\"\n test_submodule_relative_url \"(null)\" \"user@host:path/to/repo\" \"../subrepo\" \"user@host:path/to/subrepo\"\n test_submodule_relative_url \"(null)\" \"user@host:repo\" \"../subrepo\" \"user@host:subrepo\"\n+test_submodule_relative_url \"(null)\" \"user@host:repo\" \"../../subrepo\" \".:subrepo\"\n \n test_expect_success 'match .gitmodules' '\n \ttest-tool path-utils is_dotgitmodules \\\n-- \n2.18.0.865.gffc8e1a3cd6\n\n"},{"id":"355596","messageId":"CAGZ79kaK8Wt0TGvo-PyDZRLWr9PU0BRo4=DiYUXvv8c8nZ+M8A@mail.gmail.com","threadId":"49125","inReplyTo":"20180814185906.2680-1-avarab@gmail.com","subject":"Re: [PATCH] submodule: add more exhaustive up-path testing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-14T19:10:41Z","receivedAt":"2018-08-14T19:10:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 14, 2018 at 11:59 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> The tests added in 63e95beb08 (\"submodule: port resolve_relative_url\n> from shell to C\", 2016-04-15) didn't do a good job of testing various\n> up-path invocations where the up-path would bring us beyond even the\n> URL in question without emitting an error.\n>\n> These results look nonsensical, but it's worth exhaustively testing\n> them before fixing any of this code, so we can see which of these\n> cases were changed.\n\nYeah. Please look at the comment in builtin/submodule--helper.c\nin that commit, where I described my expectations.\n\nI should have put them into tests instead with the expectations\nspelled out there.\n\nThanks for this patch!\nStefan\n\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n\n\n> So I think these tests are worthwihle in themselves,\n\nThe reason I put it in the comment instead of tests was the\nease of spelling out both the status quo and expectations.\n\n> but would like\n> some advice on how to proceed with that from someone more familiar\n> with submodules.\n\nSo ideally we'd also error out as soon as the host name is touched?\n"},{"id":"355604","messageId":"xmqqbma42083.fsf@gitster-ct.c.googlers.com","threadId":"49125","inReplyTo":"CAGZ79kaK8Wt0TGvo-PyDZRLWr9PU0BRo4=DiYUXvv8c8nZ+M8A@mail.gmail.com","subject":"Re: [PATCH] submodule: add more exhaustive up-path testing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-14T19:54:04Z","receivedAt":"2018-08-14T19:54:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Thanks for this patch!\n> Stefan\n\nThanks, I'd take it as your Acked-by: (please holler if it isn't\nbefore the patch hits 'next').\n"},{"id":"355621","messageId":"87d0ukhd5g.fsf@evledraar.gmail.com","threadId":"49125","inReplyTo":"CAGZ79kaK8Wt0TGvo-PyDZRLWr9PU0BRo4=DiYUXvv8c8nZ+M8A@mail.gmail.com","subject":"Re: [PATCH] submodule: add more exhaustive up-path testing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-14T21:05:47Z","receivedAt":"2018-08-14T21:05:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 14 2018, Stefan Beller wrote:\n\n> On Tue, Aug 14, 2018 at 11:59 AM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>>\n>> The tests added in 63e95beb08 (\"submodule: port resolve_relative_url\n>> from shell to C\", 2016-04-15) didn't do a good job of testing various\n>> up-path invocations where the up-path would bring us beyond even the\n>> URL in question without emitting an error.\n>>\n>> These results look nonsensical, but it's worth exhaustively testing\n>> them before fixing any of this code, so we can see which of these\n>> cases were changed.\n>\n> Yeah. Please look at the comment in builtin/submodule--helper.c\n> in that commit, where I described my expectations.\n>\n> I should have put them into tests instead with the expectations\n> spelled out there.\n\nI'll check that out thanks. I saw that comment, but have been skimming\nmost of this code...\n\n> Thanks for this patch!\n> Stefan\n>\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> ---\n>\n>\n>> So I think these tests are worthwihle in themselves,\n>\n> The reason I put it in the comment instead of tests was the\n> ease of spelling out both the status quo and expectations.\n>\n>> but would like\n>> some advice on how to proceed with that from someone more familiar\n>> with submodules.\n>\n> So ideally we'd also error out as soon as the host name is touched?\n\nDo we have some utility function that'll take whatever we have in\nremote.<name>.url and spew out the username / host / path? We must,\nsince the clone protocol needs it, but I haven't found it.\n"},{"id":"355625","messageId":"CAGZ79kZuH8DKkoNb3AgHFxwYccOGHeVWOE7ov5QvavtomSxyAQ@mail.gmail.com","threadId":"49125","inReplyTo":"87d0ukhd5g.fsf@evledraar.gmail.com","subject":"Re: [PATCH] submodule: add more exhaustive up-path testing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-14T21:10:31Z","receivedAt":"2018-08-14T21:10:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 14, 2018 at 2:05 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n\n> > So ideally we'd also error out as soon as the host name is touched?\n>\n> Do we have some utility function that'll take whatever we have in\n> remote.<name>.url and spew out the username / host / path? We must,\n> since the clone protocol needs it, but I haven't found it.\n\nNope. Welcome to the wonderful world of submodules.\nAs submodules are in the transition state towards \"in C\",\nwe could do some refactorings that would allow for easy access to\nthese properties, but the transition is done via faithful conversion from\nshell to C, which did not have these functions either, but could just\ndo some string hackery. And that is how we ended up with this\nfunction in the first place.\n"},{"id":"355627","messageId":"87bma4hcnl.fsf@evledraar.gmail.com","threadId":"49125","inReplyTo":"CAGZ79kZuH8DKkoNb3AgHFxwYccOGHeVWOE7ov5QvavtomSxyAQ@mail.gmail.com","subject":"Re: [PATCH] submodule: add more exhaustive up-path testing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-14T21:16:30Z","receivedAt":"2018-08-14T21:16:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 14 2018, Stefan Beller wrote:\n\n> On Tue, Aug 14, 2018 at 2:05 PM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>\n>> > So ideally we'd also error out as soon as the host name is touched?\n>>\n>> Do we have some utility function that'll take whatever we have in\n>> remote.<name>.url and spew out the username / host / path? We must,\n>> since the clone protocol needs it, but I haven't found it.\n>\n> Nope. Welcome to the wonderful world of submodules.\n> As submodules are in the transition state towards \"in C\",\n> we could do some refactorings that would allow for easy access to\n> these properties, but the transition is done via faithful conversion from\n> shell to C, which did not have these functions either, but could just\n> do some string hackery. And that is how we ended up with this\n> function in the first place.\n\nThe remote/clone machinery is in C and so is the resolve-relative-url\nhelper. What's the missing bridge here?\n\nMaybe we don't have that function yet, but we can presumably expose it,\nand this seems unrelated to some other bits of submodules being in\nshellscript.\n\nNo?\n"},{"id":"355629","messageId":"CAGZ79kZq745X2PjRpGJGWdnd+HJtPuwCnJUcePXtRoLkaTWrsg@mail.gmail.com","threadId":"49125","inReplyTo":"87bma4hcnl.fsf@evledraar.gmail.com","subject":"Re: [PATCH] submodule: add more exhaustive up-path testing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-14T21:32:30Z","receivedAt":"2018-08-14T21:32:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 14, 2018 at 2:16 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n>\n> On Tue, Aug 14 2018, Stefan Beller wrote:\n>\n> > On Tue, Aug 14, 2018 at 2:05 PM Ævar Arnfjörð Bjarmason\n> > <avarab@gmail.com> wrote:\n> >\n> >> > So ideally we'd also error out as soon as the host name is touched?\n> >>\n> >> Do we have some utility function that'll take whatever we have in\n> >> remote.<name>.url and spew out the username / host / path? We must,\n> >> since the clone protocol needs it, but I haven't found it.\n> >\n> > Nope. Welcome to the wonderful world of submodules.\n> > As submodules are in the transition state towards \"in C\",\n> > we could do some refactorings that would allow for easy access to\n> > these properties, but the transition is done via faithful conversion from\n> > shell to C, which did not have these functions either, but could just\n> > do some string hackery. And that is how we ended up with this\n> > function in the first place.\n>\n> The remote/clone machinery is in C and so is the resolve-relative-url\n> helper. What's the missing bridge here?\n>\n> Maybe we don't have that function yet, but we can presumably expose it,\n> and this seems unrelated to some other bits of submodules being in\n> shellscript.\n>\n> No?\n\nGah, I misread your original question. Sorry about that.\n"}]}