git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jens Lehmann <Jens.Lehmann@web.de>
To: Marc Branchaud <marcnarc@xiplink.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: [PATCH v2] submodule add: improve message when resolving a relative url fails
Date: Tue, 31 May 2011 23:34:47 +0200	[thread overview]
Message-ID: <4DE55EF7.9050609@web.de> (raw)
In-Reply-To: <4DE5561C.3010200@xiplink.com>

A "git submodule add ../sub" interprets "../sub" relative to the default
remote of the superproject. To be able to do that, a url for that remote
has to be set in the superproject's .git/config. If that is not the case
the command fails with:
	"remote (origin) does not have a url defined in .git/config"

This neither mentions the relative repository nor that the .git/config of
the superproject is the one with the missing url. And as a novice user
could assume that relative paths would work just like absolute paths do
in the filesystem and run into this by accident, the message is not very
helpful.

So change that to
	Cannot resolve "../sub" relative to this repository's "origin"
	remote: The remote's URL is not set in .git/config
to give the user a clue that "git submodule add" interprets a relative
path as being relative to its default remote, not the work tree.

Thanks-to: Marc Branchaud <marcnarc@xiplink.com>
Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

Thanks for your review, here is the updated patch.

Am 31.05.2011 22:57, schrieb Marc Branchaud:
> However, overall I think this is a pretty fragile way to handle relative
> paths.  Consider:
> 
>  - The super-repo must be a clone in order for this to work at all.
> 
>  - The super-repo cannot be checked out on a detached HEAD.
> 
>  - The current code rewrites the URL so that any relative path is either
>    rejected or munged into an absolute remote URL.
> 
> It seems to me that this feature will only work in a fairly narrow set of
> circumstances, and even when it does work it's likely to do something
> unexpected (think of a super-repo with several remotes).

And even worse: it defies the principle of least surprise when I can't
just replace an absolute filesystem path with a relative one just like
almost everywhere else ... an option enabling this behavior might have
been a better way in hindsight.

> Back when Junio accepted the original patch, he said "If you maintain and
> serve a set related projects you need to give the users a single URL (per
> where the user is and how to reach the server)."  I'm not sure I understand
> that:  Why would the users be adding their own submodules to the
> superproject?  Wouldn't the superproject define the submodules in for them?

I can't tell about that reasoning, but it might make sense in a way I don't
understand yet ...

> I think it would be better to either just reject relative paths entirely, or
> accept any relative path as-is and display a warning that the submodule is
> only valid on the local machine.  (Perhaps one day receive-pack could even be
> taught to reject any pushes with a .gitmodules file containing a relative URL.)

Breaking backwards compatibility should not be considered easily. I tend to
not change such things until I understand the use cases for the solution,
especially as submodule use cases tend to be very different ...


 git-submodule.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index d189a24..587d74e 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -34,7 +34,7 @@ resolve_relative_url ()
 {
 	remote=$(get_default_remote)
 	remoteurl=$(git config "remote.$remote.url") ||
-		die "remote ($remote) does not have a url defined in .git/config"
+		die "Cannot resolve \"$1\" relative to this repository's \"$remote\" remote: The remote's URL is not set in .git/config"
 	url="$1"
 	remoteurl=${remoteurl%/}
 	sep=/
-- 
1.7.5.3.445.g9c0c23

  reply	other threads:[~2011-05-31 21:36 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-05-30 21:51 [PATCH 0/2] Tests for some submodule corner cases Marc Branchaud
2011-05-30 21:51 ` [PATCH 1/2] Added a test for "submodule add" using a ../relative/path/to/the/submodule/repo Marc Branchaud
2011-05-30 21:51 ` [PATCH 2/2] Added a test for "submodule status" when the submodule's working directory has deleted files Marc Branchaud
2011-05-31 19:30 ` [PATCH 0/2] Tests for some submodule corner cases Jens Lehmann
2011-05-31 20:00   ` [PATCH] submodule add: improve message when resolving a relative url fails Jens Lehmann
2011-05-31 20:57     ` Marc Branchaud
2011-05-31 21:34       ` Jens Lehmann [this message]
2011-05-31 22:04       ` Phil Hord
2011-06-01 15:55         ` Marc Branchaud
2011-07-27 19:00           ` Phil Hord
2011-07-29 20:10             ` Marc Branchaud
2011-05-31 23:23       ` Junio C Hamano
2011-06-01 15:56         ` [PATCH] Clarified how "git submodule add" handles relative paths Marc Branchaud
2011-06-01 16:59           ` Junio C Hamano
2011-06-01 19:55             ` Jens Lehmann
2011-06-02 17:14               ` Junio C Hamano
2011-06-03 19:51                 ` Jens Lehmann
2011-06-03 23:16                   ` Junio C Hamano
2011-06-04  2:23                     ` Mark Levedahl
2011-06-04 15:39                       ` Jens Lehmann
2011-06-04 16:19                     ` Jens Lehmann
2011-06-05 18:27                   ` Junio C Hamano
2011-06-06 19:56                     ` [PATCH 0/3] submodule add: allow relative repository path even when no url is set Jens Lehmann
2011-06-06 19:57                       ` [PATCH 1/3] submodule add: test failure when url is not configured in superproject Jens Lehmann
2011-06-06 19:58                       ` [PATCH 2/3] submodule add: allow relative repository path even when no url is set Jens Lehmann
2011-06-06 20:49                         ` [PATCH 0/2] Improve "git submodule add" documentation Marc Branchaud
2011-06-06 20:49                           ` [PATCH 1/2] More precisely described how "git submodule add" handles relative submodule URLs Marc Branchaud
2011-06-06 20:49                           ` [PATCH 2/2] Moved paragraph describing the utility of " Marc Branchaud
2011-06-06 19:58                       ` [PATCH 3/3] submodule add: clean up duplicated code Jens Lehmann
2011-06-06 21:00                       ` [PATCH 0/3] submodule add: allow relative repository path even when no url is set Junio C Hamano
2011-06-06 21:23                         ` Marc Branchaud
2011-06-06 21:39                           ` Jens Lehmann
2011-06-07 21:03                           ` Jens Lehmann
2011-06-08 13:16                             ` Phil Hord
2011-06-02 14:21             ` [PATCHv2] Clarified how "git submodule add" handles relative paths Marc Branchaud
2011-05-31 21:06   ` [PATCH 0/2] Tests for some submodule corner cases Marc Branchaud
2011-05-31 21:26     ` Jens Lehmann
2011-06-01 16:11       ` Marc Branchaud
2011-06-01 17:44         ` Junio C Hamano
2011-06-01 19:26         ` Jens Lehmann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4DE55EF7.9050609@web.de \
    --to=jens.lehmann@web.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=marcnarc@xiplink.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).