* [PATCH v4 0/2] submodule: fix handling of relative superproject origin URLs
@ 2012-05-23 16:45 Jon Seymour
2012-05-23 16:45 ` [PATCH v4 1/2] submodule: document " Jon Seymour
2012-05-23 16:45 ` [PATCH v4 2/2] submodule: fix " Jon Seymour
0 siblings, 2 replies; 10+ messages in thread
From: Jon Seymour @ 2012-05-23 16:45 UTC (permalink / raw)
To: git; +Cc: gitster, Jens.Lehmann, Jon Seymour
This series ensures that git submodule handles relative superproject origin URLs correctly.
Current behaviour:
* does not guarantee that the submodule.{name}.url property of the superproject is
always confgured with a valid path to the submodule's origin repo that is
relative to the working tree of the superproject.
* does not guarantee that the remote.origin.url property of the submodule is
always configured with a valid path to the submodule's origin repo that is
relative to the working tree of the submodule
* errors out (origin URL = foo) even though this is not strictly necessary.
These changes:
* guarantee that the configured paths are always valid relative paths
from the working tree of the repo containing to the configuration to the
origin repo of the described submodule.
Jon Seymour (2):
submodule: document handling of relative superproject origin URLs
submodule: fix handling of relative superproject origin URLs
git-submodule.sh | 17 ++++++++++--
t/t7400-submodule-basic.sh | 60 +++++++++++++++++++++++++++++++++++++++++++
t/t7403-submodule-sync.sh | 64 +++++++++++++++++++++++++++++++++++++++++++++-
3 files changed, 138 insertions(+), 3 deletions(-)
--
1.7.10.2.594.g24e850d
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v4 1/2] submodule: document handling of relative superproject origin URLs 2012-05-23 16:45 [PATCH v4 0/2] submodule: fix handling of relative superproject origin URLs Jon Seymour @ 2012-05-23 16:45 ` Jon Seymour 2012-05-23 20:58 ` Jens Lehmann 2012-05-23 16:45 ` [PATCH v4 2/2] submodule: fix " Jon Seymour 1 sibling, 1 reply; 10+ messages in thread From: Jon Seymour @ 2012-05-23 16:45 UTC (permalink / raw) To: git; +Cc: gitster, Jens.Lehmann, Jon Seymour These tests document how submodule sync and init handle various kinds of relative super project origin URLs. The submodule URL path is ../subrepo. 6 cases are documented: foo foo/bar ./foo ./foo/bar ../foo ../foo/bar Signed-off-by: Jon Seymour <jon.seymour@gmail.com> --- t/t7400-submodule-basic.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++ t/t7403-submodule-sync.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 124 insertions(+), 1 deletion(-) diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 81827e6..97e7a73 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -507,6 +507,68 @@ test_expect_success 'relative path works with user@host:path' ' ) ' +test_expect_success 'relative path works with foo' " + ( + cd reltest && + cp pristine-.git-config .git/config && + git config remote.origin.url foo && + echo \"cannot strip one component off url 'foo'\" >expect && + test_must_fail git submodule init 2>actual && + cat actual && + test_cmp expect actual + ) +" + +test_expect_success 'relative path works with foo/bar' ' + ( + cd reltest && + cp pristine-.git-config .git/config && + git config remote.origin.url foo/bar && + git submodule init && + test "$(git config submodule.sub.url)" = foo/subrepo + ) +' + +test_expect_success 'relative path works with ./foo' ' + ( + cd reltest && + cp pristine-.git-config .git/config && + git config remote.origin.url ./foo && + git submodule init && + test "$(git config submodule.sub.url)" = ./subrepo + ) +' + +test_expect_success 'relative path works with ./foo/bar' ' + ( + cd reltest && + cp pristine-.git-config .git/config && + git config remote.origin.url ./foo/bar && + git submodule init && + test "$(git config submodule.sub.url)" = ./foo/subrepo + ) +' + +test_expect_success 'relative path works with ../foo' ' + ( + cd reltest && + cp pristine-.git-config .git/config && + git config remote.origin.url ../foo && + git submodule init && + test "$(git config submodule.sub.url)" = ../subrepo + ) +' + +test_expect_success 'relative path works with ../foo/bar' ' + ( + cd reltest && + cp pristine-.git-config .git/config && + git config remote.origin.url ../foo/bar && + git submodule init && + test "$(git config submodule.sub.url)" = ../foo/subrepo + ) +' + test_expect_success 'moving the superproject does not break submodules' ' ( cd addtest && diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh index 3620215..3784c9b 100755 --- a/t/t7403-submodule-sync.sh +++ b/t/t7403-submodule-sync.sh @@ -26,7 +26,9 @@ test_expect_success setup ' (cd super-clone && git submodule update --init) && git clone super empty-clone && (cd empty-clone && git submodule init) && - git clone super top-only-clone + git clone super top-only-clone && + git clone super relative-clone && + (cd relative-clone && git submodule update --init) ' test_expect_success 'change submodule' ' @@ -86,4 +88,63 @@ test_expect_success '"git submodule sync" should not vivify uninteresting submod ) ' +test_expect_success '"git submodule sync" handles origin URL of the form foo' " + (cd relative-clone && + git remote set-url origin foo + echo \"cannot strip one component off url 'foo'\" > expect && + test_must_fail git submodule sync 2> actual && + test_cmp expect actual + ) +" + +test_expect_success '"git submodule sync" handles origin URL of the form foo/bar' ' + (cd relative-clone && + git remote set-url origin foo/bar + git submodule sync && + (cd submodule && + test "$(git config remote.origin.url)" == "foo/submodule" + ) + ) +' + +test_expect_success '"git submodule sync" handles origin URL of the form ./foo' ' + (cd relative-clone && + git remote set-url origin ./foo + git submodule sync && + (cd submodule && + test "$(git config remote.origin.url)" == "./submodule" + ) + ) +' + +test_expect_success '"git submodule sync" handles origin URL of the form ./foo/bar' ' + (cd relative-clone && + git remote set-url origin ./foo/bar + git submodule sync && + (cd submodule && + test "$(git config remote.origin.url)" == "./foo/submodule" + ) + ) +' + +test_expect_success '"git submodule sync" handles origin URL of the form ../foo' ' + (cd relative-clone && + git remote set-url origin ../foo + git submodule sync && + (cd submodule && + test "$(git config remote.origin.url)" == "../submodule" + ) + ) +' + +test_expect_success '"git submodule sync" handles origin URL of the form ../foo/bar' ' + (cd relative-clone && + git remote set-url origin ../foo/bar + git submodule sync && + (cd submodule && + test "$(git config remote.origin.url)" == "../foo/submodule" + ) + ) +' + test_done -- 1.7.10.2.594.g24e850d ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/2] submodule: document handling of relative superproject origin URLs 2012-05-23 16:45 ` [PATCH v4 1/2] submodule: document " Jon Seymour @ 2012-05-23 20:58 ` Jens Lehmann 2012-05-23 21:14 ` Junio C Hamano 0 siblings, 1 reply; 10+ messages in thread From: Jens Lehmann @ 2012-05-23 20:58 UTC (permalink / raw) To: Jon Seymour; +Cc: git, gitster Maybe the subject should rather be: "submodule: document failures handling relative superproject origin URLs" Am 23.05.2012 18:45, schrieb Jon Seymour: > These tests document how submodule sync and init handle various > kinds of relative super project origin URLs. The submodule URL > path is ../subrepo. > > 6 cases are documented: > foo > foo/bar > ./foo > ./foo/bar > ../foo > ../foo/bar Nice test coverage! > Signed-off-by: Jon Seymour <jon.seymour@gmail.com> > --- > t/t7400-submodule-basic.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++ > t/t7403-submodule-sync.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 124 insertions(+), 1 deletion(-) > > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > index 81827e6..97e7a73 100755 > --- a/t/t7400-submodule-basic.sh > +++ b/t/t7400-submodule-basic.sh > @@ -507,6 +507,68 @@ test_expect_success 'relative path works with user@host:path' ' > ) > ' But please use test_expect_failure for all tests that fail due to current bugs and document what should succeed in the commands used inside the test case (just like you did in v3). The only change needed when the bug is fixed should be changing each test_expect_failure to test_expect_success. > +test_expect_success 'relative path works with foo' " > + ( > + cd reltest && > + cp pristine-.git-config .git/config && > + git config remote.origin.url foo && > + echo \"cannot strip one component off url 'foo'\" >expect && > + test_must_fail git submodule init 2>actual && > + cat actual && > + test_cmp expect actual > + ) > +" > + > +test_expect_success 'relative path works with foo/bar' ' > + ( > + cd reltest && > + cp pristine-.git-config .git/config && > + git config remote.origin.url foo/bar && > + git submodule init && > + test "$(git config submodule.sub.url)" = foo/subrepo > + ) > +' > + > +test_expect_success 'relative path works with ./foo' ' > + ( > + cd reltest && > + cp pristine-.git-config .git/config && > + git config remote.origin.url ./foo && > + git submodule init && > + test "$(git config submodule.sub.url)" = ./subrepo > + ) > +' > + > +test_expect_success 'relative path works with ./foo/bar' ' > + ( > + cd reltest && > + cp pristine-.git-config .git/config && > + git config remote.origin.url ./foo/bar && > + git submodule init && > + test "$(git config submodule.sub.url)" = ./foo/subrepo > + ) > +' > + > +test_expect_success 'relative path works with ../foo' ' > + ( > + cd reltest && > + cp pristine-.git-config .git/config && > + git config remote.origin.url ../foo && > + git submodule init && > + test "$(git config submodule.sub.url)" = ../subrepo > + ) > +' > + > +test_expect_success 'relative path works with ../foo/bar' ' > + ( > + cd reltest && > + cp pristine-.git-config .git/config && > + git config remote.origin.url ../foo/bar && > + git submodule init && > + test "$(git config submodule.sub.url)" = ../foo/subrepo > + ) > +' > + > test_expect_success 'moving the superproject does not break submodules' ' > ( > cd addtest && > diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh > index 3620215..3784c9b 100755 > --- a/t/t7403-submodule-sync.sh > +++ b/t/t7403-submodule-sync.sh > @@ -26,7 +26,9 @@ test_expect_success setup ' > (cd super-clone && git submodule update --init) && > git clone super empty-clone && > (cd empty-clone && git submodule init) && > - git clone super top-only-clone > + git clone super top-only-clone && > + git clone super relative-clone && > + (cd relative-clone && git submodule update --init) > ' > > test_expect_success 'change submodule' ' > @@ -86,4 +88,63 @@ test_expect_success '"git submodule sync" should not vivify uninteresting submod > ) > ' > > +test_expect_success '"git submodule sync" handles origin URL of the form foo' " > + (cd relative-clone && > + git remote set-url origin foo > + echo \"cannot strip one component off url 'foo'\" > expect && > + test_must_fail git submodule sync 2> actual && > + test_cmp expect actual > + ) > +" > + > +test_expect_success '"git submodule sync" handles origin URL of the form foo/bar' ' > + (cd relative-clone && > + git remote set-url origin foo/bar > + git submodule sync && > + (cd submodule && > + test "$(git config remote.origin.url)" == "foo/submodule" > + ) > + ) > +' > + > +test_expect_success '"git submodule sync" handles origin URL of the form ./foo' ' > + (cd relative-clone && > + git remote set-url origin ./foo > + git submodule sync && > + (cd submodule && > + test "$(git config remote.origin.url)" == "./submodule" > + ) > + ) > +' > + > +test_expect_success '"git submodule sync" handles origin URL of the form ./foo/bar' ' > + (cd relative-clone && > + git remote set-url origin ./foo/bar > + git submodule sync && > + (cd submodule && > + test "$(git config remote.origin.url)" == "./foo/submodule" > + ) > + ) > +' > + > +test_expect_success '"git submodule sync" handles origin URL of the form ../foo' ' > + (cd relative-clone && > + git remote set-url origin ../foo > + git submodule sync && > + (cd submodule && > + test "$(git config remote.origin.url)" == "../submodule" > + ) > + ) > +' > + > +test_expect_success '"git submodule sync" handles origin URL of the form ../foo/bar' ' > + (cd relative-clone && > + git remote set-url origin ../foo/bar > + git submodule sync && > + (cd submodule && > + test "$(git config remote.origin.url)" == "../foo/submodule" > + ) > + ) > +' > + > test_done ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/2] submodule: document handling of relative superproject origin URLs 2012-05-23 20:58 ` Jens Lehmann @ 2012-05-23 21:14 ` Junio C Hamano 2012-05-23 21:45 ` Jens Lehmann 2012-05-23 21:55 ` Jon Seymour 0 siblings, 2 replies; 10+ messages in thread From: Junio C Hamano @ 2012-05-23 21:14 UTC (permalink / raw) To: Jens Lehmann; +Cc: Jon Seymour, git, gitster Jens Lehmann <Jens.Lehmann@web.de> writes: > Maybe the subject should rather be: > "submodule: document failures handling relative superproject origin URLs" > > Am 23.05.2012 18:45, schrieb Jon Seymour: >> These tests document how submodule sync and init handle various >> kinds of relative super project origin URLs. The submodule URL >> path is ../subrepo. >> >> 6 cases are documented: >> foo >> foo/bar >> ./foo >> ./foo/bar >> ../foo >> ../foo/bar > > Nice test coverage! I recall correctly, the original use case for relative URL entries in the .gitmodules file (to be copied to .git/config as submodule.$name.path) was so that by looking at the top-level, the locations of the origins for submodule repositories can be known from where the top-level was cloned. The above cases do not seem to be relevant, so in the sense, they are of secondary importance (and I do not find the "sneakernet tool" example convincing---the sneakernet tool that is distributed in the scenario can be written differently so that it does not require the other repositories to be named relative to it). As long as you and submodule stakeholders believe this is a reasonable addition and does not break the existing use cases, I am perfectly fine with it, though. >> Signed-off-by: Jon Seymour <jon.seymour@gmail.com> >> --- >> t/t7400-submodule-basic.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++ >> t/t7403-submodule-sync.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 124 insertions(+), 1 deletion(-) >> >> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh >> index 81827e6..97e7a73 100755 >> --- a/t/t7400-submodule-basic.sh >> +++ b/t/t7400-submodule-basic.sh >> @@ -507,6 +507,68 @@ test_expect_success 'relative path works with user@host:path' ' >> ) >> ' > > But please use test_expect_failure for all tests that fail due to > current bugs and document what should succeed in the commands used > inside the test case (just like you did in v3). The only change > needed when the bug is fixed should be changing each > test_expect_failure to test_expect_success. Very true. Thanks for a review. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/2] submodule: document handling of relative superproject origin URLs 2012-05-23 21:14 ` Junio C Hamano @ 2012-05-23 21:45 ` Jens Lehmann 2012-05-23 21:55 ` Jon Seymour 1 sibling, 0 replies; 10+ messages in thread From: Jens Lehmann @ 2012-05-23 21:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jon Seymour, git Am 23.05.2012 23:14, schrieb Junio C Hamano: > Jens Lehmann <Jens.Lehmann@web.de> writes: >> Am 23.05.2012 18:45, schrieb Jon Seymour: >>> These tests document how submodule sync and init handle various >>> kinds of relative super project origin URLs. The submodule URL >>> path is ../subrepo. >>> >>> 6 cases are documented: >>> foo >>> foo/bar >>> ./foo >>> ./foo/bar >>> ../foo >>> ../foo/bar >> >> Nice test coverage! > > I recall correctly, the original use case for relative URL entries in the > .gitmodules file (to be copied to .git/config as submodule.$name.path) was > so that by looking at the top-level, the locations of the origins for > submodule repositories can be known from where the top-level was cloned. > The above cases do not seem to be relevant, so in the sense, they are of > secondary importance (and I do not find the "sneakernet tool" example > convincing---the sneakernet tool that is distributed in the scenario can > be written differently so that it does not require the other repositories > to be named relative to it). Remember this patch series is not about relative /submodule/ urls but relative /superproject/ urls, so the .gitmodules file is not involved here. But while reviewing 2/2 I started to suspect I was a bit too hasty judging the coverage, as I realized only "git submodule init" and "git submodule sync" are tested. "git submodule add" should also be tested here to make sure it behaves well too (and only when the tests cases are not split across two commits they can be properly reviewed). My remark aimed at the attempt to test all possible relative paths in at least two depths, which I deem very helpful to find problems early. > As long as you and submodule stakeholders believe this is a reasonable > addition and does not break the existing use cases, I am perfectly fine > with it, though. Me thinks relative /superproject/ urls starting with ./ or ../ should work just fine (which they don't right now). And if relative superproject urls inside the superproject are handled too without too much hassle I won't argue against it, even though I'm not convinced of the use case either. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/2] submodule: document handling of relative superproject origin URLs 2012-05-23 21:14 ` Junio C Hamano 2012-05-23 21:45 ` Jens Lehmann @ 2012-05-23 21:55 ` Jon Seymour 1 sibling, 0 replies; 10+ messages in thread From: Jon Seymour @ 2012-05-23 21:55 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jens Lehmann, git On Thu, May 24, 2012 at 7:14 AM, Junio C Hamano <gitster@pobox.com> wrote: > Jens Lehmann <Jens.Lehmann@web.de> writes: > >> Maybe the subject should rather be: >> "submodule: document failures handling relative superproject origin URLs" > Jens: Sure. > > I recall correctly, the original use case for relative URL entries in the > .gitmodules file (to be copied to .git/config as submodule.$name.path) was > so that by looking at the top-level, the locations of the origins for > submodule repositories can be known from where the top-level was cloned. > The above cases do not seem to be relevant, so in the sense, they are of > secondary importance (and I do not find the "sneakernet tool" example > convincing---the sneakernet tool that is distributed in the scenario can > be written differently so that it does not require the other repositories > to be named relative to it). Agreed, that this is not required for the original use case. In the 'sneakernet tool' case, it makes sense for me to have ./mnt/usb around in any case (for the purposes of repos that are being managed by the sneakernet tool). Since that link exists, I can use it for the purposes of managing the tool's own origin repo and doing so does, I think, reduce the number of configuration items I have to tweak to the absolute minimum, but you are right that I don't absolutely have to do it that way. > > As long as you and submodule stakeholders believe this is a reasonable > addition and does not break the existing use cases, I am perfectly fine > with it, though. Ok, thanks for that. Whatever the use case, I think the change is a positive if simply because, by respecting an implicit invariant, it makes the behaviour consistent and predictable. I'll fix the tests, per Jens and your comments and resubmit. Jens: thanks for the review. Regards, jon. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 2/2] submodule: fix handling of relative superproject origin URLs 2012-05-23 16:45 [PATCH v4 0/2] submodule: fix handling of relative superproject origin URLs Jon Seymour 2012-05-23 16:45 ` [PATCH v4 1/2] submodule: document " Jon Seymour @ 2012-05-23 16:45 ` Jon Seymour 2012-05-23 21:50 ` Jens Lehmann 1 sibling, 1 reply; 10+ messages in thread From: Jon Seymour @ 2012-05-23 16:45 UTC (permalink / raw) To: git; +Cc: gitster, Jens.Lehmann, Jon Seymour When the origin URL of the superproject is itself relative, an operation such as git submodule add, init or sync may result in either the submodule.{name}.url configuration property of the superproject referring to the incorrect location or remote.origin.url configuration property of the submodule referring to the incorrect location or both these conditions. In some cases, git submodule fails to update the configuration and fails with an error condition. The issue arises in these cases because the origin URL of the superproject needs to be prepended with a prefix that navigates from the submodule to the superproject so that when the submodule URL is concatenated the resulting URL is relative to the working tree of the submodule. This change fixes handling for relative superproject origin URLs for 6 cases: foo foo/bar ./foo ./foo/bar ../foo ../foo/bar In each case, the configuration properties in the superproject's configuration and the submodule's configuration refer to the correct, relative, location of the submodule's origin repo. In all cases, the configured paths are relative to the working trees of the repositories containing the configuration. Signed-off-by: Jon Seymour <jon.seymour@gmail.com> --- git-submodule.sh | 17 +++++++++++++++-- t/t7400-submodule-basic.sh | 12 +++++------- t/t7403-submodule-sync.sh | 21 +++++++++++---------- 3 files changed, 31 insertions(+), 19 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index 64a70d6..3e943de 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -37,6 +37,8 @@ resolve_relative_url () remoteurl=$(git config "remote.$remote.url") || remoteurl=$(pwd) # the repository is its own authoritative upstream url="$1" + prefix="$2" + remoteurl=$(echo "$remoteurl" | sed "s|^[^/:\\.][^:]*\$|./&|") remoteurl=${remoteurl%/} sep=/ while test -n "$url" @@ -45,6 +47,11 @@ resolve_relative_url () ../*) url="${url#../}" case "$remoteurl" in + .*/*) + remoteurl="${remoteurl%/*}" + remoteurl="${remoteurl#./}" + remoteurl="${prefix}${remoteurl}" + ;; */*) remoteurl="${remoteurl%/*}" ;; @@ -64,7 +71,7 @@ resolve_relative_url () break;; esac done - echo "$remoteurl$sep${url%/}" + echo "${remoteurl%/.}$sep${url%/}" } # @@ -964,8 +971,14 @@ cmd_sync() # Possibly a url relative to parent case "$url" in ./*|../*) + up_path="$(echo "$sm_path" | sed "s/[^/]*/../g")" && + up_path=${up_path%/}/ && + remoteurl=$(resolve_relative_url "$url" "$up_path") && url=$(resolve_relative_url "$url") || exit ;; + *) + remoteurl="$url" + ;; esac if git config "submodule.$name.url" >/dev/null 2>/dev/null @@ -979,7 +992,7 @@ cmd_sync() clear_local_git_env cd "$sm_path" remote=$(get_default_remote) - git config remote."$remote".url "$url" + git config remote."$remote".url "$remoteurl" ) fi fi diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 97e7a73..f2f907f 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -507,17 +507,15 @@ test_expect_success 'relative path works with user@host:path' ' ) ' -test_expect_success 'relative path works with foo' " +test_expect_success 'relative path works with foo' ' ( cd reltest && cp pristine-.git-config .git/config && git config remote.origin.url foo && - echo \"cannot strip one component off url 'foo'\" >expect && - test_must_fail git submodule init 2>actual && - cat actual && - test_cmp expect actual + git submodule init && + test "$(git config submodule.sub.url)" = ./subrepo ) -" +' test_expect_success 'relative path works with foo/bar' ' ( @@ -545,7 +543,7 @@ test_expect_success 'relative path works with ./foo/bar' ' cp pristine-.git-config .git/config && git config remote.origin.url ./foo/bar && git submodule init && - test "$(git config submodule.sub.url)" = ./foo/subrepo + test "$(git config submodule.sub.url)" = foo/subrepo ) ' diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh index 3784c9b..583fe21 100755 --- a/t/t7403-submodule-sync.sh +++ b/t/t7403-submodule-sync.sh @@ -88,21 +88,22 @@ test_expect_success '"git submodule sync" should not vivify uninteresting submod ) ' -test_expect_success '"git submodule sync" handles origin URL of the form foo' " +test_expect_success '"git submodule sync" handles origin URL of the form foo' ' (cd relative-clone && git remote set-url origin foo - echo \"cannot strip one component off url 'foo'\" > expect && - test_must_fail git submodule sync 2> actual && - test_cmp expect actual + git submodule sync && + (cd submodule && + test "$(git config remote.origin.url)" == "../submodule" ) -" + ) +' test_expect_success '"git submodule sync" handles origin URL of the form foo/bar' ' (cd relative-clone && git remote set-url origin foo/bar git submodule sync && (cd submodule && - test "$(git config remote.origin.url)" == "foo/submodule" + test "$(git config remote.origin.url)" == "../foo/submodule" ) ) ' @@ -112,7 +113,7 @@ test_expect_success '"git submodule sync" handles origin URL of the form ./foo' git remote set-url origin ./foo git submodule sync && (cd submodule && - test "$(git config remote.origin.url)" == "./submodule" + test "$(git config remote.origin.url)" == "../submodule" ) ) ' @@ -122,7 +123,7 @@ test_expect_success '"git submodule sync" handles origin URL of the form ./foo/b git remote set-url origin ./foo/bar git submodule sync && (cd submodule && - test "$(git config remote.origin.url)" == "./foo/submodule" + test "$(git config remote.origin.url)" == "../foo/submodule" ) ) ' @@ -132,7 +133,7 @@ test_expect_success '"git submodule sync" handles origin URL of the form ../foo' git remote set-url origin ../foo git submodule sync && (cd submodule && - test "$(git config remote.origin.url)" == "../submodule" + test "$(git config remote.origin.url)" == "../../submodule" ) ) ' @@ -142,7 +143,7 @@ test_expect_success '"git submodule sync" handles origin URL of the form ../foo/ git remote set-url origin ../foo/bar git submodule sync && (cd submodule && - test "$(git config remote.origin.url)" == "../foo/submodule" + test "$(git config remote.origin.url)" == "../../foo/submodule" ) ) ' -- 1.7.10.2.594.g24e850d ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v4 2/2] submodule: fix handling of relative superproject origin URLs 2012-05-23 16:45 ` [PATCH v4 2/2] submodule: fix " Jon Seymour @ 2012-05-23 21:50 ` Jens Lehmann 2012-05-23 22:17 ` Jon Seymour 0 siblings, 1 reply; 10+ messages in thread From: Jens Lehmann @ 2012-05-23 21:50 UTC (permalink / raw) To: Jon Seymour; +Cc: git, gitster Am 23.05.2012 18:45, schrieb Jon Seymour: > When the origin URL of the superproject is itself relative, an operation > such as git submodule add, init or sync may result in either the > submodule.{name}.url configuration property of the superproject > referring to the incorrect location or remote.origin.url configuration > property of the submodule referring to the incorrect location or both > these conditions. In some cases, git submodule fails to update > the configuration and fails with an error condition. > > The issue arises in these cases because the origin URL of > the superproject needs to be prepended with a prefix that navigates > from the submodule to the superproject so that when the submodule > URL is concatenated the resulting URL is relative to the working tree > of the submodule. > > This change fixes handling for relative superproject origin URLs > for 6 cases: > foo > foo/bar > ./foo > ./foo/bar > ../foo > ../foo/bar > > In each case, the configuration properties in the superproject's > configuration and the submodule's configuration refer to the > correct, relative, location of the submodule's origin repo. In all cases, > the configured paths are relative to the working trees of the > repositories containing the configuration. > > Signed-off-by: Jon Seymour <jon.seymour@gmail.com> > --- > git-submodule.sh | 17 +++++++++++++++-- > t/t7400-submodule-basic.sh | 12 +++++------- > t/t7403-submodule-sync.sh | 21 +++++++++++---------- > 3 files changed, 31 insertions(+), 19 deletions(-) > > diff --git a/git-submodule.sh b/git-submodule.sh > index 64a70d6..3e943de 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -37,6 +37,8 @@ resolve_relative_url () > remoteurl=$(git config "remote.$remote.url") || > remoteurl=$(pwd) # the repository is its own authoritative upstream > url="$1" > + prefix="$2" As mentioned last time I'd rather use $2 directly at the only site where $prefix is used. (On the other hand it might also make sense to give the parameters a descriptive name at the beginning of the function, but then I'd vote for a descriptive name like "urlprefix" or similar to make its meaning clearer) > + remoteurl=$(echo "$remoteurl" | sed "s|^[^/:\\.][^:]*\$|./&|") A comment describing what this line actually does would be nice. > remoteurl=${remoteurl%/} > sep=/ > while test -n "$url" > @@ -45,6 +47,11 @@ resolve_relative_url () > ../*) > url="${url#../}" > case "$remoteurl" in > + .*/*) > + remoteurl="${remoteurl%/*}" > + remoteurl="${remoteurl#./}" > + remoteurl="${prefix}${remoteurl}" > + ;; > */*) > remoteurl="${remoteurl%/*}" > ;; > @@ -64,7 +71,7 @@ resolve_relative_url () > break;; > esac > done > - echo "$remoteurl$sep${url%/}" > + echo "${remoteurl%/.}$sep${url%/}" Wouldn't that better be handled in the ".*/*)" case above to avoid accidentally affecting the other cases? > } > > # > @@ -964,8 +971,14 @@ cmd_sync() > # Possibly a url relative to parent > case "$url" in > ./*|../*) > + up_path="$(echo "$sm_path" | sed "s/[^/]*/../g")" && > + up_path=${up_path%/}/ && > + remoteurl=$(resolve_relative_url "$url" "$up_path") && > url=$(resolve_relative_url "$url") || exit > ;; > + *) > + remoteurl="$url" > + ;; > esac > > if git config "submodule.$name.url" >/dev/null 2>/dev/null > @@ -979,7 +992,7 @@ cmd_sync() > clear_local_git_env > cd "$sm_path" > remote=$(get_default_remote) > - git config remote."$remote".url "$url" > + git config remote."$remote".url "$remoteurl" > ) > fi > fi I still have to wrap my head around these two hunks (I suspect it's too late for that in my timezone ;-), but I really wonder how you get away without changing cmd_add and cmd_init like you did last time. This looks much different than #2 and #4 of your v3 combined, which makes me suspicious in what direction this is evolving. Maybe you could tell us what you found out addressing the last problem you mentioned and how you handled it? The only changes following here should be from test_expect_failure to test_expect_success as mentioned in my response to your first patch. > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > index 97e7a73..f2f907f 100755 > --- a/t/t7400-submodule-basic.sh > +++ b/t/t7400-submodule-basic.sh > @@ -507,17 +507,15 @@ test_expect_success 'relative path works with user@host:path' ' > ) > ' > > -test_expect_success 'relative path works with foo' " > +test_expect_success 'relative path works with foo' ' > ( > cd reltest && > cp pristine-.git-config .git/config && > git config remote.origin.url foo && > - echo \"cannot strip one component off url 'foo'\" >expect && > - test_must_fail git submodule init 2>actual && > - cat actual && > - test_cmp expect actual > + git submodule init && > + test "$(git config submodule.sub.url)" = ./subrepo > ) > -" > +' > > test_expect_success 'relative path works with foo/bar' ' > ( > @@ -545,7 +543,7 @@ test_expect_success 'relative path works with ./foo/bar' ' > cp pristine-.git-config .git/config && > git config remote.origin.url ./foo/bar && > git submodule init && > - test "$(git config submodule.sub.url)" = ./foo/subrepo > + test "$(git config submodule.sub.url)" = foo/subrepo > ) > ' > > diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh > index 3784c9b..583fe21 100755 > --- a/t/t7403-submodule-sync.sh > +++ b/t/t7403-submodule-sync.sh > @@ -88,21 +88,22 @@ test_expect_success '"git submodule sync" should not vivify uninteresting submod > ) > ' > > -test_expect_success '"git submodule sync" handles origin URL of the form foo' " > +test_expect_success '"git submodule sync" handles origin URL of the form foo' ' > (cd relative-clone && > git remote set-url origin foo > - echo \"cannot strip one component off url 'foo'\" > expect && > - test_must_fail git submodule sync 2> actual && > - test_cmp expect actual > + git submodule sync && > + (cd submodule && > + test "$(git config remote.origin.url)" == "../submodule" > ) > -" > + ) > +' > > test_expect_success '"git submodule sync" handles origin URL of the form foo/bar' ' > (cd relative-clone && > git remote set-url origin foo/bar > git submodule sync && > (cd submodule && > - test "$(git config remote.origin.url)" == "foo/submodule" > + test "$(git config remote.origin.url)" == "../foo/submodule" > ) > ) > ' > @@ -112,7 +113,7 @@ test_expect_success '"git submodule sync" handles origin URL of the form ./foo' > git remote set-url origin ./foo > git submodule sync && > (cd submodule && > - test "$(git config remote.origin.url)" == "./submodule" > + test "$(git config remote.origin.url)" == "../submodule" > ) > ) > ' > @@ -122,7 +123,7 @@ test_expect_success '"git submodule sync" handles origin URL of the form ./foo/b > git remote set-url origin ./foo/bar > git submodule sync && > (cd submodule && > - test "$(git config remote.origin.url)" == "./foo/submodule" > + test "$(git config remote.origin.url)" == "../foo/submodule" > ) > ) > ' > @@ -132,7 +133,7 @@ test_expect_success '"git submodule sync" handles origin URL of the form ../foo' > git remote set-url origin ../foo > git submodule sync && > (cd submodule && > - test "$(git config remote.origin.url)" == "../submodule" > + test "$(git config remote.origin.url)" == "../../submodule" > ) > ) > ' > @@ -142,7 +143,7 @@ test_expect_success '"git submodule sync" handles origin URL of the form ../foo/ > git remote set-url origin ../foo/bar > git submodule sync && > (cd submodule && > - test "$(git config remote.origin.url)" == "../foo/submodule" > + test "$(git config remote.origin.url)" == "../../foo/submodule" > ) > ) > ' ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 2/2] submodule: fix handling of relative superproject origin URLs 2012-05-23 21:50 ` Jens Lehmann @ 2012-05-23 22:17 ` Jon Seymour 2012-05-24 2:32 ` Jon Seymour 0 siblings, 1 reply; 10+ messages in thread From: Jon Seymour @ 2012-05-23 22:17 UTC (permalink / raw) To: Jens Lehmann; +Cc: git, gitster On Thu, May 24, 2012 at 7:50 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote: > Am 23.05.2012 18:45, schrieb Jon Seymour: > > As mentioned last time I'd rather use $2 directly at the only site > where $prefix is used. (On the other hand it might also make sense > to give the parameters a descriptive name at the beginning of the > function, but then I'd vote for a descriptive name like "urlprefix" > or similar to make its meaning clearer) Ok, I was favouring your latter heuristic. Are you ok if I use up_path here? > >> + remoteurl=$(echo "$remoteurl" | sed "s|^[^/:\\.][^:]*\$|./&|") > > A comment describing what this line actually does would be nice. > Sure, will do. What it does is to allow URLs of the form foo/ or foo/bar to be handled by the .*/* case in the switch below by rewriting the URL to prefix ./ in that case. It doesn't do it for URLs that already begin with . (don't need it), / (mustn't have it) or : (assuming that any path beginning with : should probably be handled by the *:* case). >> remoteurl=${remoteurl%/} >> sep=/ >> while test -n "$url" >> @@ -45,6 +47,11 @@ resolve_relative_url () >> ../*) >> url="${url#../}" >> case "$remoteurl" in >> + .*/*) >> + remoteurl="${remoteurl%/*}" >> + remoteurl="${remoteurl#./}" >> + remoteurl="${prefix}${remoteurl}" >> + ;; >> */*) >> remoteurl="${remoteurl%/*}" >> ;; >> @@ -64,7 +71,7 @@ resolve_relative_url () >> break;; >> esac >> done >> - echo "$remoteurl$sep${url%/}" >> + echo "${remoteurl%/.}$sep${url%/}" > > Wouldn't that better be handled in the ".*/*)" case above to avoid > accidentally affecting the other cases? Yes, I think you are right. Thanks. > >> } >> >> # >> @@ -964,8 +971,14 @@ cmd_sync() >> # Possibly a url relative to parent >> case "$url" in >> ./*|../*) >> + up_path="$(echo "$sm_path" | sed "s/[^/]*/../g")" && >> + up_path=${up_path%/}/ && >> + remoteurl=$(resolve_relative_url "$url" "$up_path") && >> url=$(resolve_relative_url "$url") || exit >> ;; >> + *) >> + remoteurl="$url" >> + ;; >> esac >> >> if git config "submodule.$name.url" >/dev/null 2>/dev/null >> @@ -979,7 +992,7 @@ cmd_sync() >> clear_local_git_env >> cd "$sm_path" >> remote=$(get_default_remote) >> - git config remote."$remote".url "$url" >> + git config remote."$remote".url "$remoteurl" >> ) >> fi >> fi > > I still have to wrap my head around these two hunks (I suspect it's > too late for that in my timezone ;-), but I really wonder how you get > away without changing cmd_add and cmd_init like you did last time. > This looks much different than #2 and #4 of your v3 combined, which > makes me suspicious in what direction this is evolving. Maybe you could > tell us what you found out addressing the last problem you mentioned > and how you handled it? So, I subsequently noticed that v3 broke the clone done by a subsequent update in the case of paths of the form ../foo/bar That was because I had violated an implicit invariant - namely that the submodule.{name}.url configuration variable in the superproject is always a path relative to the working tree of the superproject. In v3, I had effectively made this value relative to the working tree of the submodule. I mostly only need to modify sync behaviour, because that is the only path that modifies the remote.origin.url of the submodule - the other two paths modify the submodule.{name}.url variable of the superproject and this behaviour is mostly correct (sans normalisation issues). This series respects the implicit invariant that relative paths in configuration properties (submodule.{name}.url and remote.{remote}.url) are always relative to the working tree of the repository in which the configuration variable is defined. I'll try to find a way to inject aspects of this commentary into the comments. > > > The only changes following here should be from test_expect_failure > to test_expect_success as mentioned in my response to your first patch. > Sure. Thanks for your review. jon. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 2/2] submodule: fix handling of relative superproject origin URLs 2012-05-23 22:17 ` Jon Seymour @ 2012-05-24 2:32 ` Jon Seymour 0 siblings, 0 replies; 10+ messages in thread From: Jon Seymour @ 2012-05-24 2:32 UTC (permalink / raw) To: Jens Lehmann; +Cc: git, gitster On Thu, May 24, 2012 at 8:17 AM, Jon Seymour <jon.seymour@gmail.com> wrote: > On Thu, May 24, 2012 at 7:50 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote: >> Am 23.05.2012 18:45, schrieb Jon Seymour: >> Wouldn't that better be handled in the ".*/*)" case above to avoid >> accidentally affecting the other cases? > > Yes, I think you are right. Thanks. > Turns out this case was necessary, but I don't fully understand why as yet. I'll work that out and see if I can avoid doing it, otherwise I will document why it is reasonable. jon. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-05-24 2:32 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-05-23 16:45 [PATCH v4 0/2] submodule: fix handling of relative superproject origin URLs Jon Seymour 2012-05-23 16:45 ` [PATCH v4 1/2] submodule: document " Jon Seymour 2012-05-23 20:58 ` Jens Lehmann 2012-05-23 21:14 ` Junio C Hamano 2012-05-23 21:45 ` Jens Lehmann 2012-05-23 21:55 ` Jon Seymour 2012-05-23 16:45 ` [PATCH v4 2/2] submodule: fix " Jon Seymour 2012-05-23 21:50 ` Jens Lehmann 2012-05-23 22:17 ` Jon Seymour 2012-05-24 2:32 ` Jon Seymour
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).