# [PATCH] Fix submodule sync with relative submodule URLs 10 messages from 2008-09-22 to 2008-09-25. Participants: Johan Herland, David Aguilar, Shawn O. Pearce, Johannes Sixt. Thread: https://gitlist.dev/t/15624 ## Johan Herland, 2008-09-22 16:08 Subject: [PATCH] Fix submodule sync with relative submodule URLs Message-ID: <200809221808.31730.johan@herland.net> URL: https://gitlist.dev/e/200809221808.31730.johan%40herland.net ``` Signed-off-by: Johan Herland --- git-submodule.sh | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index 1c39b59..92be0fe 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -634,6 +634,14 @@ cmd_sync() do name=$(module_name "$path") url=$(git config -f .gitmodules --get submodule."$name".url) + + # Possibly a url relative to parent + case "$url" in + ./*|../*) + url=$(resolve_relative_url "$url") || exit + ;; + esac + if test -e "$path"/.git then ( -- 1.6.0.1.400.gd2470 ``` ## David Aguilar, 2008-09-24 07:29 Subject: Re: [PATCH] Fix submodule sync with relative submodule URLs Message-ID: <402731c90809240029l308726aeme14c0e7fe2d54a3c@mail.gmail.com> URL: https://gitlist.dev/e/402731c90809240029l308726aeme14c0e7fe2d54a3c%40mail.gmail.com In-Reply-To: <200809221808.31730.johan@herland.net> ``` On Mon, Sep 22, 2008 at 9:08 AM, Johan Herland wrote: > Signed-off-by: Johan Herland > --- > git-submodule.sh | 8 ++++++++ > 1 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/git-submodule.sh b/git-submodule.sh > index 1c39b59..92be0fe 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -634,6 +634,14 @@ cmd_sync() > do > name=$(module_name "$path") > url=$(git config -f .gitmodules --get submodule."$name".url) > + > + # Possibly a url relative to parent > + case "$url" in > + ./*|../*) > + url=$(resolve_relative_url "$url") || exit > + ;; > + esac > + > if test -e "$path"/.git > then > ( > -- > 1.6.0.1.400.gd2470 > Instead of just doing an "|| exit" shouldn't it report an explanation of the error? Other than that, it looks good to me. Mark, Junio? -- David ``` ## Johan Herland, 2008-09-24 09:31 Subject: [PATCH] Fix submodule sync with relative submodule URLs Message-ID: <200809241131.16327.johan@herland.net> URL: https://gitlist.dev/e/200809241131.16327.johan%40herland.net In-Reply-To: <402731c90809240029l308726aeme14c0e7fe2d54a3c@mail.gmail.com> ``` Signed-off-by: Johan Herland --- On Wednesday 24 September 2008, David Aguilar wrote: > Instead of just doing an "|| exit" shouldn't it report an explanation > of the error? > Other than that, it looks good to me. Fixed. Thanks. ...Johan git-submodule.sh | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index 1c39b59..f89bdbe 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -634,6 +634,15 @@ cmd_sync() do name=$(module_name "$path") url=$(git config -f .gitmodules --get submodule."$name".url) + + # Possibly a url relative to parent + case "$url" in + ./*|../*) + url=$(resolve_relative_url "$url") || + die "failed to resolve relative submodule url for '$name'" + ;; + esac + if test -e "$path"/.git then ( -- 1.6.0.2.463.g7f0eb ``` ## Shawn O. Pearce, 2008-09-24 16:19 Subject: Re: [PATCH] Fix submodule sync with relative submodule URLs Message-ID: <20080924161902.GT3669@spearce.org> URL: https://gitlist.dev/e/20080924161902.GT3669%40spearce.org In-Reply-To: <200809241131.16327.johan@herland.net> ``` Johan Herland wrote: > On Wednesday 24 September 2008, David Aguilar wrote: > > Instead of just doing an "|| exit" shouldn't it report an explanation > > of the error? > > Other than that, it looks good to me. > > Fixed. Thanks. OK, time for the drive-by patch commenting. I've largely stayed out of git-submodule related code, but I just looked at in the context of applying this patch. There are three callers to resolve_relative_url in master and next. All three callers just "|| exit" when resolve_relative_url fails. The only reason resolve_relative_url can fail is when there is no remote.$remote.url configuration option set for the current default remote ("origin"?). I guess I'm unclear about why cmd_sync is different from the existing callers. > diff --git a/git-submodule.sh b/git-submodule.sh > index 1c39b59..f89bdbe 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -634,6 +634,15 @@ cmd_sync() > do > name=$(module_name "$path") > url=$(git config -f .gitmodules --get submodule."$name".url) > + > + # Possibly a url relative to parent > + case "$url" in > + ./*|../*) > + url=$(resolve_relative_url "$url") || > + die "failed to resolve relative submodule url for '$name'" > + ;; > + esac > + > if test -e "$path"/.git > then > ( -- Shawn. ``` ## Johan Herland, 2008-09-24 16:41 Subject: Re: [PATCH] Fix submodule sync with relative submodule URLs Message-ID: <200809241841.48934.johan@herland.net> URL: https://gitlist.dev/e/200809241841.48934.johan%40herland.net In-Reply-To: <20080924161902.GT3669@spearce.org> ``` On Wednesday 24 September 2008, Shawn O. Pearce wrote: > I guess I'm unclear about why cmd_sync is different from the > existing callers. It's not any different as far as I'm concerned. We should probably add helpful error messages to the other callers as well. ...Johan -- Johan Herland, www.herland.net ``` ## David Aguilar, 2008-09-25 11:21 Subject: [PATCH] git-submodule: remove unnecessary exits when calling resolve_relative_url Message-ID: <17c3c33dd7aa6803d7ac046f3e4dc0d5bb4c7234.1222341013.git.davvid@gmail.com> URL: https://gitlist.dev/e/17c3c33dd7aa6803d7ac046f3e4dc0d5bb4c7234.1222341013.git.davvid%40gmail.com In-Reply-To: <200809221808.31730.johan@herland.net> ``` resolve_relative_url calls die() when no remote url exists so these calls to exit can be removed. Signed-off-by: David Aguilar --- This applies on top of the first "Fix submodule sync with relative submodule URLs" patch by Johan Herland. Shawn's comments made me realize that the resolve_relative_url callers could be cleaned up. git-submodule.sh | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index 92be0fe..533d1cc 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -155,7 +155,7 @@ cmd_add() case "$repo" in ./*|../*) # dereference source url relative to parent's url - realrepo=$(resolve_relative_url "$repo") || exit + realrepo=$(resolve_relative_url "$repo") ;; *:*|/*) # absolute url @@ -184,7 +184,7 @@ cmd_add() case "$repo" in ./*|../*) - url=$(resolve_relative_url "$repo") || exit + url=$(resolve_relative_url "$repo") ;; *) url="$repo" @@ -270,7 +270,7 @@ cmd_init() # Possibly a url relative to parent case "$url" in ./*|../*) - url=$(resolve_relative_url "$url") || exit + url=$(resolve_relative_url "$url") ;; esac @@ -638,7 +638,7 @@ cmd_sync() # Possibly a url relative to parent case "$url" in ./*|../*) - url=$(resolve_relative_url "$url") || exit + url=$(resolve_relative_url "$url") ;; esac -- 1.6.0.2.307.gc4275 ``` ## David Aguilar, 2008-09-25 11:38 Subject: Re: [PATCH] Fix submodule sync with relative submodule URLs Message-ID: <20080925113849.GA18543@gmail.com> URL: https://gitlist.dev/e/20080925113849.GA18543%40gmail.com In-Reply-To: <20080924161902.GT3669@spearce.org> ``` On 0, "Shawn O. Pearce" wrote: > Johan Herland wrote: > > On Wednesday 24 September 2008, David Aguilar wrote: > > > Instead of just doing an "|| exit" shouldn't it report an explanation > > > of the error? > > > Other than that, it looks good to me. > > > > Fixed. Thanks. > > OK, time for the drive-by patch commenting. I've largely stayed > out of git-submodule related code, but I just looked at in the > context of applying this patch. > > There are three callers to resolve_relative_url in master and next. > All three callers just "|| exit" when resolve_relative_url fails. > > The only reason resolve_relative_url can fail is when there is no > remote.$remote.url configuration option set for the current default > remote ("origin"?). > > I guess I'm unclear about why cmd_sync is different from the > existing callers. Right, it's not. resolve_relative_url() is already calling die() when that error condition is met, so the "|| exit" thing can be removed entirely. I sent a patch on top of Johan's first patch to remove the "|| exit" calls in all callers of resolve_relative_url. That seems like the right thing to do; in the very least it makes it easier to read. What do you think? > > > diff --git a/git-submodule.sh b/git-submodule.sh > > index 1c39b59..f89bdbe 100755 > > --- a/git-submodule.sh > > +++ b/git-submodule.sh > > @@ -634,6 +634,15 @@ cmd_sync() > > do > > name=$(module_name "$path") > > url=$(git config -f .gitmodules --get submodule."$name".url) > > + > > + # Possibly a url relative to parent > > + case "$url" in > > + ./*|../*) > > + url=$(resolve_relative_url "$url") || > > + die "failed to resolve relative submodule url for '$name'" > > + ;; > > + esac > > + > > if test -e "$path"/.git > > then > > ( > > -- > Shawn. -- David ``` ## Johannes Sixt, 2008-09-25 12:19 Subject: Re: [PATCH] git-submodule: remove unnecessary exits when calling resolve_relative_url Message-ID: <48DB81DE.6070600@viscovery.net> URL: https://gitlist.dev/e/48DB81DE.6070600%40viscovery.net In-Reply-To: <17c3c33dd7aa6803d7ac046f3e4dc0d5bb4c7234.1222341013.git.davvid@gmail.com> ``` David Aguilar schrieb: > resolve_relative_url calls die() when no remote url exists so these calls to > exit can be removed. ... > @@ -155,7 +155,7 @@ cmd_add() > case "$repo" in > ./*|../*) > # dereference source url relative to parent's url > - realrepo=$(resolve_relative_url "$repo") || exit > + realrepo=$(resolve_relative_url "$repo") > ;; Did you test it? The command inside $(...) is run in its own sub-process, therefore, the die() does not exit the caller, just the sub-process, and the || exit *is* required. BTW, I think that || exit is sufficient; you don't need to add another error message - the one that resolve_relative_url() prints is sufficient. -- Hannes ``` ## Shawn O. Pearce, 2008-09-25 14:54 Subject: Re: [PATCH] git-submodule: remove unnecessary exits when calling resolve_relative_url Message-ID: <20080925145407.GM3669@spearce.org> URL: https://gitlist.dev/e/20080925145407.GM3669%40spearce.org In-Reply-To: <48DB81DE.6070600@viscovery.net> ``` Johannes Sixt wrote: > David Aguilar schrieb: > > resolve_relative_url calls die() when no remote url exists so these calls to > > exit can be removed. > ... > > @@ -155,7 +155,7 @@ cmd_add() > > case "$repo" in > > ./*|../*) > > # dereference source url relative to parent's url > > - realrepo=$(resolve_relative_url "$repo") || exit > > + realrepo=$(resolve_relative_url "$repo") > > ;; > > Did you test it? The command inside $(...) is run in its own sub-process, > therefore, the die() does not exit the caller, just the sub-process, and > the || exit *is* required. > > BTW, I think that || exit is sufficient; you don't need to add another > error message - the one that resolve_relative_url() prints is sufficient. Exactly. I think we just need a "|| exit" after each of these $(resolve_relative_url) calls. The original patch that started this discussion just needs a "|| exit". die with an additional message is just too verbose. -- Shawn. ``` ## Johan Herland, 2008-09-25 14:58 Subject: Re: [PATCH] git-submodule: remove unnecessary exits when calling resolve_relative_url Message-ID: <200809251658.46642.johan@herland.net> URL: https://gitlist.dev/e/200809251658.46642.johan%40herland.net In-Reply-To: <20080925145407.GM3669@spearce.org> ``` On Thursday 25 September 2008, Shawn O. Pearce wrote: > Johannes Sixt wrote: > > Did you test it? The command inside $(...) is run in its own > > sub-process, therefore, the die() does not exit the caller, just > > the sub-process, and the || exit *is* required. > > > > BTW, I think that || exit is sufficient; you don't need to add > > another error message - the one that resolve_relative_url() prints > > is sufficient. > > Exactly. > > I think we just need a "|| exit" after each of these > $(resolve_relative_url) calls. The original patch that > started this discussion just needs a "|| exit". The original patch did exactly that: http://article.gmane.org/gmane.comp.version-control.git/96493 Have fun! ...Johan -- Johan Herland, www.herland.net ```