{"thread":{"id":"15624","subject":"[PATCH] Fix submodule sync with relative submodule URLs","startedAt":"2008-09-22T16:08:31Z","lastAt":"2008-09-25T14:58:46Z","messageCount":10,"participants":["Johan Herland","David Aguilar","Shawn O. Pearce","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"91323","messageId":"200809221808.31730.johan@herland.net","threadId":"15624","inReplyTo":null,"subject":"[PATCH] Fix submodule sync with relative submodule URLs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2008-09-22T16:08:31Z","receivedAt":"2008-09-22T16:08:31Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Signed-off-by: Johan Herland <johan@herland.net>\n---\n git-submodule.sh |    8 ++++++++\n 1 files changed, 8 insertions(+), 0 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 1c39b59..92be0fe 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -634,6 +634,14 @@ cmd_sync()\n \tdo\n \t\tname=$(module_name \"$path\")\n \t\turl=$(git config -f .gitmodules --get submodule.\"$name\".url)\n+\n+\t\t# Possibly a url relative to parent\n+\t\tcase \"$url\" in\n+\t\t./*|../*)\n+\t\t\turl=$(resolve_relative_url \"$url\") || exit\n+\t\t\t;;\n+\t\tesac\n+\n \t\tif test -e \"$path\"/.git\n \t\tthen\n \t\t(\n-- \n1.6.0.1.400.gd2470\n"},{"id":"91451","messageId":"402731c90809240029l308726aeme14c0e7fe2d54a3c@mail.gmail.com","threadId":"15624","inReplyTo":"200809221808.31730.johan@herland.net","subject":"Re: [PATCH] Fix submodule sync with relative submodule URLs","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2008-09-24T07:29:21Z","receivedAt":"2008-09-24T07:29:21Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Mon, Sep 22, 2008 at 9:08 AM, Johan Herland <johan@herland.net> wrote:\n> Signed-off-by: Johan Herland <johan@herland.net>\n> ---\n>  git-submodule.sh |    8 ++++++++\n>  1 files changed, 8 insertions(+), 0 deletions(-)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 1c39b59..92be0fe 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -634,6 +634,14 @@ cmd_sync()\n>        do\n>                name=$(module_name \"$path\")\n>                url=$(git config -f .gitmodules --get submodule.\"$name\".url)\n> +\n> +               # Possibly a url relative to parent\n> +               case \"$url\" in\n> +               ./*|../*)\n> +                       url=$(resolve_relative_url \"$url\") || exit\n> +                       ;;\n> +               esac\n> +\n>                if test -e \"$path\"/.git\n>                then\n>                (\n> --\n> 1.6.0.1.400.gd2470\n>\n\nInstead of just doing an \"|| exit\" shouldn't it report an explanation\nof the error?\nOther than that, it looks good to me.\n\nMark, Junio?\n\n\n-- \n David\n"},{"id":"91455","messageId":"200809241131.16327.johan@herland.net","threadId":"15624","inReplyTo":"402731c90809240029l308726aeme14c0e7fe2d54a3c@mail.gmail.com","subject":"[PATCH] Fix submodule sync with relative submodule URLs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2008-09-24T09:31:16Z","receivedAt":"2008-09-24T09:31:16Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Signed-off-by: Johan Herland <johan@herland.net>\n---\n\nOn Wednesday 24 September 2008, David Aguilar wrote:\n> Instead of just doing an \"|| exit\" shouldn't it report an explanation\n> of the error?\n> Other than that, it looks good to me.\n\nFixed. Thanks.\n\n...Johan\n\n\n git-submodule.sh |    9 +++++++++\n 1 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 1c39b59..f89bdbe 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -634,6 +634,15 @@ cmd_sync()\n \tdo\n \t\tname=$(module_name \"$path\")\n \t\turl=$(git config -f .gitmodules --get submodule.\"$name\".url)\n+\n+\t\t# Possibly a url relative to parent\n+\t\tcase \"$url\" in\n+\t\t./*|../*)\n+\t\t\turl=$(resolve_relative_url \"$url\") ||\n+\t\t\t\tdie \"failed to resolve relative submodule url for '$name'\"\n+\t\t\t;;\n+\t\tesac\n+\n \t\tif test -e \"$path\"/.git\n \t\tthen\n \t\t(\n-- \n1.6.0.2.463.g7f0eb\n"},{"id":"91485","messageId":"20080924161902.GT3669@spearce.org","threadId":"15624","inReplyTo":"200809241131.16327.johan@herland.net","subject":"Re: [PATCH] Fix submodule sync with relative submodule URLs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-09-24T16:19:02Z","receivedAt":"2008-09-24T16:19:02Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johan Herland <johan@herland.net> wrote:\n> On Wednesday 24 September 2008, David Aguilar wrote:\n> > Instead of just doing an \"|| exit\" shouldn't it report an explanation\n> > of the error?\n> > Other than that, it looks good to me.\n> \n> Fixed. Thanks.\n\nOK, time for the drive-by patch commenting.  I've largely stayed\nout of git-submodule related code, but I just looked at in the\ncontext of applying this patch.\n\nThere are three callers to resolve_relative_url in master and next.\nAll three callers just \"|| exit\" when resolve_relative_url fails.\n\nThe only reason resolve_relative_url can fail is when there is no\nremote.$remote.url configuration option set for the current default\nremote (\"origin\"?).\n\nI guess I'm unclear about why cmd_sync is different from the\nexisting callers.\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 1c39b59..f89bdbe 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -634,6 +634,15 @@ cmd_sync()\n>  \tdo\n>  \t\tname=$(module_name \"$path\")\n>  \t\turl=$(git config -f .gitmodules --get submodule.\"$name\".url)\n> +\n> +\t\t# Possibly a url relative to parent\n> +\t\tcase \"$url\" in\n> +\t\t./*|../*)\n> +\t\t\turl=$(resolve_relative_url \"$url\") ||\n> +\t\t\t\tdie \"failed to resolve relative submodule url for '$name'\"\n> +\t\t\t;;\n> +\t\tesac\n> +\n>  \t\tif test -e \"$path\"/.git\n>  \t\tthen\n>  \t\t(\n\n-- \nShawn.\n"},{"id":"91491","messageId":"200809241841.48934.johan@herland.net","threadId":"15624","inReplyTo":"20080924161902.GT3669@spearce.org","subject":"Re: [PATCH] Fix submodule sync with relative submodule URLs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2008-09-24T16:41:48Z","receivedAt":"2008-09-24T16:41:48Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Wednesday 24 September 2008, Shawn O. Pearce wrote:\n> I guess I'm unclear about why cmd_sync is different from the\n> existing callers.\n\nIt's not any different as far as I'm concerned. We should probably add \nhelpful error messages to the other callers as well.\n\n\n...Johan\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"91578","messageId":"17c3c33dd7aa6803d7ac046f3e4dc0d5bb4c7234.1222341013.git.davvid@gmail.com","threadId":"15624","inReplyTo":"200809221808.31730.johan@herland.net","subject":"[PATCH] git-submodule: remove unnecessary exits when calling resolve_relative_url","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2008-09-25T11:21:49Z","receivedAt":"2008-09-25T11:21:49Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"resolve_relative_url calls die() when no remote url exists so these calls to\nexit can be removed.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n\nThis applies on top of the first\n\"Fix submodule sync with relative submodule URLs\" patch by Johan Herland.\nShawn's comments made me realize that the resolve_relative_url callers\ncould be cleaned up.\n\n git-submodule.sh |    8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 92be0fe..533d1cc 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -155,7 +155,7 @@ cmd_add()\n \tcase \"$repo\" in\n \t./*|../*)\n \t\t# dereference source url relative to parent's url\n-\t\trealrepo=$(resolve_relative_url \"$repo\") || exit\n+\t\trealrepo=$(resolve_relative_url \"$repo\")\n \t\t;;\n \t*:*|/*)\n \t\t# absolute url\n@@ -184,7 +184,7 @@ cmd_add()\n \n \t\tcase \"$repo\" in\n \t\t./*|../*)\n-\t\t\turl=$(resolve_relative_url \"$repo\") || exit\n+\t\t\turl=$(resolve_relative_url \"$repo\")\n \t\t    ;;\n \t\t*)\n \t\t\turl=\"$repo\"\n@@ -270,7 +270,7 @@ cmd_init()\n \t\t# Possibly a url relative to parent\n \t\tcase \"$url\" in\n \t\t./*|../*)\n-\t\t\turl=$(resolve_relative_url \"$url\") || exit\n+\t\t\turl=$(resolve_relative_url \"$url\")\n \t\t\t;;\n \t\tesac\n \n@@ -638,7 +638,7 @@ cmd_sync()\n \t\t# Possibly a url relative to parent\n \t\tcase \"$url\" in\n \t\t./*|../*)\n-\t\t\turl=$(resolve_relative_url \"$url\") || exit\n+\t\t\turl=$(resolve_relative_url \"$url\")\n \t\t\t;;\n \t\tesac\n \n-- \n1.6.0.2.307.gc4275\n"},{"id":"91580","messageId":"20080925113849.GA18543@gmail.com","threadId":"15624","inReplyTo":"20080924161902.GT3669@spearce.org","subject":"Re: [PATCH] Fix submodule sync with relative submodule URLs","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2008-09-25T11:38:51Z","receivedAt":"2008-09-25T11:38:51Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On  0, \"Shawn O. Pearce\" <spearce@spearce.org> wrote:\n> Johan Herland <johan@herland.net> wrote:\n> > On Wednesday 24 September 2008, David Aguilar wrote:\n> > > Instead of just doing an \"|| exit\" shouldn't it report an explanation\n> > > of the error?\n> > > Other than that, it looks good to me.\n> > \n> > Fixed. Thanks.\n> \n> OK, time for the drive-by patch commenting.  I've largely stayed\n> out of git-submodule related code, but I just looked at in the\n> context of applying this patch.\n> \n> There are three callers to resolve_relative_url in master and next.\n> All three callers just \"|| exit\" when resolve_relative_url fails.\n> \n> The only reason resolve_relative_url can fail is when there is no\n> remote.$remote.url configuration option set for the current default\n> remote (\"origin\"?).\n> \n> I guess I'm unclear about why cmd_sync is different from the\n> existing callers.\n\nRight, it's not.  resolve_relative_url() is already calling\ndie() when that error condition is met, so the \"|| exit\" thing\ncan be removed entirely.  I sent a patch on top of Johan's\nfirst patch to remove the \"|| exit\" calls in all callers of\nresolve_relative_url.  That seems like the right thing to do;\nin the very least it makes it easier to read.  What do you\nthink?\n\n\n\n\n> \n> > diff --git a/git-submodule.sh b/git-submodule.sh\n> > index 1c39b59..f89bdbe 100755\n> > --- a/git-submodule.sh\n> > +++ b/git-submodule.sh\n> > @@ -634,6 +634,15 @@ cmd_sync()\n> >  \tdo\n> >  \t\tname=$(module_name \"$path\")\n> >  \t\turl=$(git config -f .gitmodules --get submodule.\"$name\".url)\n> > +\n> > +\t\t# Possibly a url relative to parent\n> > +\t\tcase \"$url\" in\n> > +\t\t./*|../*)\n> > +\t\t\turl=$(resolve_relative_url \"$url\") ||\n> > +\t\t\t\tdie \"failed to resolve relative submodule url for '$name'\"\n> > +\t\t\t;;\n> > +\t\tesac\n> > +\n> >  \t\tif test -e \"$path\"/.git\n> >  \t\tthen\n> >  \t\t(\n> \n> -- \n> Shawn.\n\n-- \n\n\tDavid\n"},{"id":"91585","messageId":"48DB81DE.6070600@viscovery.net","threadId":"15624","inReplyTo":"17c3c33dd7aa6803d7ac046f3e4dc0d5bb4c7234.1222341013.git.davvid@gmail.com","subject":"Re: [PATCH] git-submodule: remove unnecessary exits when calling resolve_relative_url","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-09-25T12:19:42Z","receivedAt":"2008-09-25T12:19:42Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"David Aguilar schrieb:\n> resolve_relative_url calls die() when no remote url exists so these calls to\n> exit can be removed.\n...\n> @@ -155,7 +155,7 @@ cmd_add()\n>  \tcase \"$repo\" in\n>  \t./*|../*)\n>  \t\t# dereference source url relative to parent's url\n> -\t\trealrepo=$(resolve_relative_url \"$repo\") || exit\n> +\t\trealrepo=$(resolve_relative_url \"$repo\")\n>  \t\t;;\n\nDid you test it? The command inside $(...) is run in its own sub-process,\ntherefore, the die() does not exit the caller, just the sub-process, and\nthe || exit *is* required.\n\nBTW, I think that || exit is sufficient; you don't need to add another\nerror message - the one that resolve_relative_url() prints is sufficient.\n\n-- Hannes\n"},{"id":"91604","messageId":"20080925145407.GM3669@spearce.org","threadId":"15624","inReplyTo":"48DB81DE.6070600@viscovery.net","subject":"Re: [PATCH] git-submodule: remove unnecessary exits when calling resolve_relative_url","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-09-25T14:54:07Z","receivedAt":"2008-09-25T14:54:07Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> wrote:\n> David Aguilar schrieb:\n> > resolve_relative_url calls die() when no remote url exists so these calls to\n> > exit can be removed.\n> ...\n> > @@ -155,7 +155,7 @@ cmd_add()\n> >  \tcase \"$repo\" in\n> >  \t./*|../*)\n> >  \t\t# dereference source url relative to parent's url\n> > -\t\trealrepo=$(resolve_relative_url \"$repo\") || exit\n> > +\t\trealrepo=$(resolve_relative_url \"$repo\")\n> >  \t\t;;\n> \n> Did you test it? The command inside $(...) is run in its own sub-process,\n> therefore, the die() does not exit the caller, just the sub-process, and\n> the || exit *is* required.\n> \n> BTW, I think that || exit is sufficient; you don't need to add another\n> error message - the one that resolve_relative_url() prints is sufficient.\n\nExactly.\n\nI think we just need a \"|| exit\" after each of these\n$(resolve_relative_url) calls.  The original patch that\nstarted this discussion just needs a \"|| exit\".\n\ndie with an additional message is just too verbose.\n\n\n-- \nShawn.\n"},{"id":"91605","messageId":"200809251658.46642.johan@herland.net","threadId":"15624","inReplyTo":"20080925145407.GM3669@spearce.org","subject":"Re: [PATCH] git-submodule: remove unnecessary exits when calling resolve_relative_url","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2008-09-25T14:58:46Z","receivedAt":"2008-09-25T14:58:46Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Thursday 25 September 2008, Shawn O. Pearce wrote:\n> Johannes Sixt <j.sixt@viscovery.net> wrote:\n> > Did you test it? The command inside $(...) is run in its own\n> > sub-process, therefore, the die() does not exit the caller, just\n> > the sub-process, and the || exit *is* required.\n> >\n> > BTW, I think that || exit is sufficient; you don't need to add\n> > another error message - the one that resolve_relative_url() prints\n> > is sufficient.\n>\n> Exactly.\n>\n> I think we just need a \"|| exit\" after each of these\n> $(resolve_relative_url) calls.  The original patch that\n> started this discussion just needs a \"|| exit\".\n\nThe original patch did exactly that: \nhttp://article.gmane.org/gmane.comp.version-control.git/96493\n\n\nHave fun!\n\n...Johan\n\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"}]}