[PATCH] fix "git-submodule add a/b/c/repository"

[PATCH] fix "git-submodule add a/b/c/repository"

[Sylvain Joyeux]

The 'master' version of git-submodule.sh fails to see that
a/b/c/repository is an already existing repository and messes up the
whole thing. The following patch fixes that.
-- 
Sylvain

>From 2bca2e17a01cd81ce30f81750583ab943ab57ff0 Mon Sep 17 00:00:00 2001
From: Sylvain Joyeux <sylvain.joyeux@dfki.de>
Date: Tue, 1 Jul 2008 16:45:04 +0200
Subject: [PATCH] fix submodule add for non-toplevel in-project directories

This patch fixes git-submodule add for submodules that
already exist in the current package tree, in a folder
which is not at toplevel, i.e.:

  git submodule add a/b/c/repository

Signed-off-by: Sylvain Joyeux <sylvain.joyeux@dfki.de>
---
 git-submodule.sh           |    6 +++++-
 t/t7400-submodule-basic.sh |    9 +++++++++
 2 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index e2b91f6..3fa8ff3 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -156,7 +156,11 @@ cmd_add()
 
 	# Guess path from repo if not specified or strip trailing slashes
 	if test -z "$path"; then
-		path=$(echo "$repo" | sed -e 's|/*$||' -e 's|:*/*\.git$||' -e 's|.*[/:]||g')
+                if echo "$repo" | grep -q "^\(\.\.\|\/\)"; then
+                        path=$(echo "$repo" | sed -e 's|/*$||' -e 's|:*/*\.git$||' -e 's|.*[/:]||g')
+                else
+                        path=$(echo "$repo" | sed -e 's|/*$||')
+                fi
 	else
 		path=$(echo "$path" | sed -e 's|/*$||')
 	fi
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index ffaa932..84ea6e9 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -215,4 +215,13 @@ test_expect_success 'update --init' '
 
 '
 
+test_expect_success 'adding an already-existing repository deep in the directory hierarchy' '
+
+        mkdir dir0 &&
+        mkdir dir0/dir1 &&
+        git clone init dir0/dir1/init &&
+        git-submodule add dir0/dir1/init &&
+        git-submodule status | grep "dir0/dir1/init"
+'
+
 test_done
-- 
1.5.6

Re: [PATCH] fix "git-submodule add a/b/c/repository"

[Junio C Hamano]

Sylvain Joyeux <sylvain.joyeux@dfki.de> writes:

> The 'master' version of git-submodule.sh fails to see that
> a/b/c/repository is an already existing repository and messes up the
> whole thing. The following patch fixes that.
> -- 
> Sylvain
>
>>From 2bca2e17a01cd81ce30f81750583ab943ab57ff0 Mon Sep 17 00:00:00 2001
> From: Sylvain Joyeux <sylvain.joyeux@dfki.de>
> Date: Tue, 1 Jul 2008 16:45:04 +0200
> Subject: [PATCH] fix submodule add for non-toplevel in-project directories
>
> This patch fixes git-submodule add for submodules that
> already exist in the current package tree, in a folder
> which is not at toplevel, i.e.:
>
>   git submodule add a/b/c/repository

Which one is the commit log message ;-)?

Perhaps Documentation/SubmittingPatches needs a review?

> +test_expect_success 'adding an already-existing repository deep in the directory hierarchy' '
> +
> +        mkdir dir0 &&
> +        mkdir dir0/dir1 &&
> +        git clone init dir0/dir1/init &&
> +        git-submodule add dir0/dir1/init &&
> +        git-submodule status | grep "dir0/dir1/init"
> +'

I am not sure if this is fixing a sane use case.  "submodule add" is
documented to take:

    'git submodule' [--quiet] add [-b branch] [--] <repository> [<path>]

and you are adding at dir0/dir1/init a submodule that will interact with "init"
repository, so shouldn't that command line be something like:

	git submodule add init dir0/dir1/init

???

Confused..

Re: [PATCH] fix "git-submodule add a/b/c/repository"

[Sylvain Joyeux]

> > +test_expect_success 'adding an already-existing repository deep in the directory hierarchy' '
> > +
> > +        mkdir dir0 &&
> > +        mkdir dir0/dir1 &&
> > +        git clone init dir0/dir1/init &&
> > +        git-submodule add dir0/dir1/init &&
> > +        git-submodule status | grep "dir0/dir1/init"
> > +'
> 
> I am not sure if this is fixing a sane use case.  "submodule add" is
> documented to take:
> 
>     'git submodule' [--quiet] add [-b branch] [--] <repository> [<path>]
> 
> and you are adding at dir0/dir1/init a submodule that will interact with "init"
> repository, so shouldn't that command line be something like:
> 
> 	git submodule add init dir0/dir1/init

 	git submodule add dir0/dir1/init

Is supposed to add the repository already checked-out in dir0/dir1/init as a
submodule, at the same location.

 	git submodule add init dir0/dir1/init
    
Would clone dir0/dir1/init at ./init and add ./init as a submodule. This is
actually what the current git-submodule (wrongly) does.

Sylvain

Re: [PATCH] fix "git-submodule add a/b/c/repository"

[Mark Levedahl]

Sylvain Joyeux wrote:
>  	git submodule add init dir0/dir1/init
>     
> Would clone dir0/dir1/init at ./init and add ./init as a submodule. This is
> actually what the current git-submodule (wrongly) does.
>
> Sylvain
>   
...after some prep work...

 >git submodule add init dir0/dir1/init
Adding existing repo at 'dir0/dir1/init' to the index

So, what's the problem?

Mark

Re: [PATCH] fix "git-submodule add a/b/c/repository"

[Sylvain Joyeux]

On Sun, Jul 06, 2008 at 03:05:38PM -0400, Mark Levedahl wrote:
> Sylvain Joyeux wrote:
>>  	git submodule add init dir0/dir1/init
>>     Would clone dir0/dir1/init at ./init and add ./init as a submodule. 
>> This is
>> actually what the current git-submodule (wrongly) does.
>>
>> Sylvain
>>   
> ...after some prep work...
>
> >git submodule add init dir0/dir1/init
> Adding existing repo at 'dir0/dir1/init' to the index
>
> So, what's the problem?

Redo the prep work, the clone and now

git submodule add dir0/dir1/init

(i.e. don't expect dir0/dir1/init to be the clone of ./init, that was just a
shortcut for the test. Expect it to be a clone of "something, somewhere")

[~/tmp/test]% git-submodule add dir0/dir1/init
Initialize init/.git
Initialized empty Git repository in /home/doudou/tmp/test/init/.git/

[~/tmp/test]% git status
# On branch master
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#       modified:   .gitmodules
#       new file:   init
#
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#       dir0/

What it is supposed to do (according to the documentation) is register
dir0/dir1/init as the submodule, not clone it and register init/.

Sylvain

Re: [PATCH] fix "git-submodule add a/b/c/repository"

[Mark Levedahl]

Sylvain Joyeux wrote:
>
> Redo the prep work, the clone and now
>
> git submodule add dir0/dir1/init
>
> (i.e. don't expect dir0/dir1/init to be the clone of ./init, that was just a
> shortcut for the test. Expect it to be a clone of "something, somewhere")
>
>   
Per the man-page,
    git submodule [--quiet] add [-b branch] [--] <repository> [<path>]

which means, that the *repository* url is mandatory, the path is 
optional. What you specifically asked git-submodule to do was to *clone* 
from dir0/dir1/init, and because you gave no path to put the submodule 
in, git-submodule deduced the name as "init", and cloned to there.

Mark

Re: [PATCH] fix "git-submodule add a/b/c/repository"

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> Sylvain Joyeux wrote:
>>
>> Redo the prep work, the clone and now
>>
>> git submodule add dir0/dir1/init
>>
>> (i.e. don't expect dir0/dir1/init to be the clone of ./init, that was just a
>> shortcut for the test. Expect it to be a clone of "something, somewhere")
>>
>>
> Per the man-page,
>    git submodule [--quiet] add [-b branch] [--] <repository> [<path>]
>
> which means, that the *repository* url is mandatory, the path is
> optional. What you specifically asked git-submodule to do was to
> *clone* from dir0/dir1/init, and because you gave no path to put the
> submodule in, git-submodule deduced the name as "init", and cloned to
> there.

I'd like to hear clarifications on two counts, please?

 (1) If Sylvain wanted to have that appear at dir0/dir1/init not init,
     would it have been sufficient to give that path twice (once for
     <repository> and another for <path> parameter) to make things work as
     expected?

 (2) Is it generally considered a sane use case to specify an existing
     repository inside the working tree of a superproject as a submodule
     using "git submodule add" like Sylvain's example did?

     I would have understood if the command were "git add dir0/dir1/init",
     but I have this vague recolleciton that "git submodule add" is about
     telling our repository about a submodule that comes from _outside_.

Re: [PATCH] fix "git-submodule add a/b/c/repository"

[Mark Levedahl]

Junio C Hamano wrote:
> I'd like to hear clarifications on two counts, please?
>  (1) If Sylvain wanted to have that appear at dir0/dir1/init not init,
>      would it have been sufficient to give that path twice (once for
>      <repository> and another for <path> parameter) to make things work as
>      expected?
>   
git-submodule really requires two arguments:

    $ git submodule add <URL> <relative-path-to-module-in-tree>

and supports two modes:

1) relative-path exists and is a valid repo: just add the module, it was 
created in tree, the user is expected to eventually push this to the 
given URL so other users will get this as normal. This exists to 
simplify the process of creating a repo to begin with.

2) relative-path doesn't exist: clone from the URL. This is the normal use.
submodule supports adding a module in one of two ways:

So,

    $ git submodule add   dir0/dir1/init   dir0/dir1/init

will add the repo, but also makes the repo its own origin. I don't think 
this makes sense.
>  (2) Is it generally considered a sane use case to specify an existing
>      repository inside the working tree of a superproject as a submodule
>      using "git submodule add" like Sylvain's example did?
>
>      I would have understood if the command were "git add dir0/dir1/init",
>      but I have this vague recolleciton that "git submodule add" is about
>      telling our repository about a submodule that comes from _outside_.
>
>
>   
Adding an existing in-tree repo, ala

 $ git submodule add <intended-URL> <path>

is there to ease the initial creation of a submodule. It can be created 
and registered in-tree, and later pushed to the server. This is sane, 
but is not the normal usage (makes sense only on creation).

Mark

Re: [PATCH] fix "git-submodule add a/b/c/repository"

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> Junio C Hamano wrote:
>> I'd like to hear clarifications on two counts, please?
>>  (1) If Sylvain wanted to have that appear at dir0/dir1/init not init,
>>      would it have been sufficient to give that path twice (once for
>>      <repository> and another for <path> parameter) to make things work as
>>      expected?
>>
> git-submodule really requires two arguments:
>
>    $ git submodule add <URL> <relative-path-to-module-in-tree>
>
> and supports two modes:
>
> 1) relative-path exists and is a valid repo: just add the module, it
> was created in tree, the user is expected to eventually push this to
> the given URL so other users will get this as normal. This exists to
> simplify the process of creating a repo to begin with.
>
> 2) relative-path doesn't exist: clone from the URL. This is the normal use.
> submodule supports adding a module in one of two ways:
>
> So,
>
>    $ git submodule add   dir0/dir1/init   dir0/dir1/init
>
> will add the repo, but also makes the repo its own origin. I don't
> think this makes sense.
>>  (2) Is it generally considered a sane use case to specify an existing
>>      repository inside the working tree of a superproject as a submodule
>>      using "git submodule add" like Sylvain's example did?
>>
>>      I would have understood if the command were "git add dir0/dir1/init",
>>      but I have this vague recolleciton that "git submodule add" is about
>>      telling our repository about a submodule that comes from _outside_.
>>
>>
>>
> Adding an existing in-tree repo, ala
>
> $ git submodule add <intended-URL> <path>
>
> is there to ease the initial creation of a submodule. It can be
> created and registered in-tree, and later pushed to the server. This
> is sane, but is not the normal usage (makes sense only on creation).

Thanks.

The above is quite a bit more information than I can read from
Documentation/git-submodule.txt; care to send it in in a patch form?

Re: [PATCH] fix "git-submodule add a/b/c/repository"

[Mark Levedahl]

Junio C Hamano wrote:
> Thanks.
>
> The above is quite a bit more information than I can read from
> Documentation/git-submodule.txt; care to send it in in a patch form?
>
>   
Will do, but I think it makes more sense to clean things up a bit so 
they are more explicable:

1) *require* two arguments for add: <URL> <relative-path-in-repo>
2) Remove one option for URL. Currently, we accept:
    a) absolute URL : origin is at URL
    b) top-level-relative URL (./foo | ../foo) : locates origin relative 
to top-level origin
    c) path-relative URL : locates origin relative to current working 
directory

I don't understand the use-case for item (c), and in any case it is 
easily replaced as an abolute url (e.g., $(pwd)/URL). So, I propose to 
restrict to (a) or (b) only, and reject (c).

I think these two changes in concert will reduce a lot of confusion 
without removing any real capability. Absent negative comments, I will 
prepare a patch to do this, *and* update the documentation to better 
define the options.

Mark

[PATCH] git-submodule - make "submodule add" more strict, and document it

[Mark Levedahl]

This change makes "submodule add" much more strict in the arguments it
takes, and is intended to address confusion as recently noted on the
git-list. With this change, the required syntax is:
	$ git submodule add URL path

Specifically, this eliminates the form
	$git submodule add URL
which was confused by more than one person as
	$git submodule add path

The change eliminates one form of URL: a path relative to the current
working directory. This form was confusing, and does not seem to
correspond to an important use-case. Specifically, no-one has identified
the use to clone from a repository already in the tree, but if this is
needed it is easily done using an absolute URL: $(pwd)/relative-path. So,
no functionality is lost.

Following this change, there are exactly four variants of
submodule-add, as both arguments have two flavors:

URL can be absolute, or can begin with ./|../ and thus name the origin
relative to the top-level origin.

Note: With this patch, "submodule add" discerns an absolute URL as
matching /*|*:*: e.g., URL begins with /, or it contains a :. This works
for all valid URLs, an absolute path in POSIX, as well as an absolute
path on Windows).

path can either already exist as a valid git repo, or will be cloned from
the given URL. The first form here allows easy addition of a new
submodule to an existing project as the module can be added and tested
in-tree before pushing to the public repository. However, the more
usual form is the second, where the repo is cloned from the given URL.

This specifically addresses the issue of
	$ git submodule add a/b/c
attempting to clone from a repository at "a/b/c" to create a new module
in "c". This also simplifies description of "relative URL" as there is now
exactly *one* form: a URL relative to the parent's origin repo.

Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
 Documentation/git-submodule.txt |   31 +++++++++++++++------
 git-submodule.sh                |   55 ++++++++++++++------------------------
 2 files changed, 42 insertions(+), 44 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 105fc2d..6f24f92 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -9,7 +9,7 @@ git-submodule - Initialize, update or inspect submodules
 SYNOPSIS
 --------
 [verse]
-'git submodule' [--quiet] add [-b branch] [--] <repository> [<path>]
+'git submodule' [--quiet] add [-b branch] [--] <repository> <path>
 'git submodule' [--quiet] status [--cached] [--] [<path>...]
 'git submodule' [--quiet] init [--] [<path>...]
 'git submodule' [--quiet] update [--init] [--] [<path>...]
@@ -20,14 +20,26 @@ COMMANDS
 --------
 add::
 	Add the given repository as a submodule at the given path
-	to the changeset to be committed next.  If path is a valid
-	repository within the project, it is added as is. Otherwise,
-	repository is cloned at the specified path. path is added to the
-	changeset and registered in .gitmodules.   If no path is
-	specified, the path is deduced from the repository specification.
-	If the repository url begins with ./ or ../, it is stored as
-	given but resolved as a relative path from the main project's
-	url when cloning.
+	to the changeset to be committed next. This requires two arguments,
+	<repository> and <path>. <repository> is the URL of the new
+	submodule's origin repository. This may be either an absolute URL,
+	or (if it begins with ./ or ../), the location relative
+	to the parent repository's origin.
+
+	<path> is the relative location for the cloned submodule to
+	exist in the current tree. If <path> does not exist, then the
+	module is created by cloning from the named URL. If <path> does
+	exist and is already a valid git repository, then this is added
+	to the changeset without cloning. This second form is provided
+	to ease adding a submodule to a project the first time, and presumes
+	the user will later push the new submodule to the given URL.
+
+	In either case, the given URL is recorded into .gitmodules for
+	use by subsequent users cloning the project. If the URL is given
+	relative to the parent, the presumption is the main and sub-modules
+	will be kept together in the same relative location, and only the
+	top-level URL need be provided: git-submodule will correctly
+	locat the submodules using the hint in .gitmodules.
 
 status::
 	Show the status of the submodules. This will print the SHA-1 of the
@@ -85,6 +97,7 @@ OPTIONS
 <path>::
 	Path to submodule(s). When specified this will restrict the command
 	to only operate on the submodules found at the specified paths.
+	(This argument is required with add).
 
 FILES
 -----
diff --git a/git-submodule.sh b/git-submodule.sh
index 3eb78cc..d227907 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -5,7 +5,7 @@
 # Copyright (c) 2007 Lars Hjemli
 
 USAGE="[--quiet] [--cached] \
-[add <repo> [-b branch]|status|init|update [-i|--init]|summary [-n|--summary-limit <n>] [<commit>]] \
+[add <repo> [-b branch] <path>]|[status|init|update [-i|--init]|summary [-n|--summary-limit <n>] [<commit>]] \
 [--] [<path>...]"
 OPTIONS_SPEC=
 . git-sh-setup
@@ -27,18 +27,6 @@ say()
 	fi
 }
 
-# NEEDSWORK: identical function exists in get_repo_base in clone.sh
-get_repo_base() {
-	(
-		cd "`/bin/pwd`" &&
-		cd "$1" || cd "$1.git" &&
-		{
-			cd .git
-			pwd
-		}
-	) 2>/dev/null
-}
-
 # Resolve relative url by appending to parent's url
 resolve_relative_url ()
 {
@@ -115,7 +103,7 @@ module_clone()
 #
 # Add a new submodule to the working tree, .gitmodules and the index
 #
-# $@ = repo [path]
+# $@ = repo path
 #
 # optional branch is stored in global branch variable
 #
@@ -150,16 +138,27 @@ cmd_add()
 	repo=$1
 	path=$2
 
-	if test -z "$repo"; then
+	if test -z "$repo" -o -z "$path"; then
 		usage
 	fi
 
-	# Guess path from repo if not specified or strip trailing slashes
-	if test -z "$path"; then
-		path=$(echo "$repo" | sed -e 's|/*$||' -e 's|:*/*\.git$||' -e 's|.*[/:]||g')
-	else
-		path=$(echo "$path" | sed -e 's|/*$||')
-	fi
+	# assure repo is absolute or relative to parent
+	case "$repo" in
+		./*|../*)
+			# dereference source url relative to parent's url
+			realrepo=$(resolve_relative_url "$repo") || exit
+			;;
+	    *:*|/*)
+			# absolute url
+			realrepo=$repo
+			;;
+		*)
+			die "repo URL: '$repo' must be absolute or begin with ./|../"
+	    ;;
+	esac
+
+	# strip trailing slashes from path
+	path=$(echo "$path" | sed -e 's|/*$||')
 
 	git ls-files --error-unmatch "$path" > /dev/null 2>&1 &&
 	die "'$path' already exists in the index"
@@ -175,20 +174,6 @@ cmd_add()
 			die "'$path' already exists and is not a valid git repo"
 		fi
 	else
-		case "$repo" in
-		./*|../*)
-			# dereference source url relative to parent's url
-			realrepo=$(resolve_relative_url "$repo") || exit
-			;;
-		*)
-			# Turn the source into an absolute path if
-			# it is local
-			if base=$(get_repo_base "$repo"); then
-				repo="$base"
-			fi
-			realrepo=$repo
-			;;
-		esac
 
 		module_clone "$path" "$realrepo" || exit
 		(unset GIT_DIR; cd "$path" && git checkout -q ${branch:+-b "$branch" "origin/$branch"}) ||
-- 
1.5.6.2.271.g73ad8

Re: [PATCH] git-submodule - make "submodule add" more strict, and document it

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> This change makes "submodule add" much more strict in the arguments it
> takes, and is intended to address confusion as recently noted on the
> git-list. With this change, the required syntax is:
> 	$ git submodule add URL path
>

Please have an extra blank line on both sides of examples.

> Specifically, this eliminates the form
> 	$git submodule add URL
> which was confused by more than one person as
> 	$git submodule add path

s/\$/& /;

> The change eliminates one form of URL: a path relative to the current
> working directory....
> ...
> Following this change, there are exactly four variants of
> submodule-add, as both arguments have two flavors:
>
> URL can be absolute, or can begin with ./|../ and thus name the origin
> relative to the top-level origin.

Now this _is_ confusing.  Examples of a path relative to the current
working directory would be ./foo or ../../foo but the previous paragraph
says the form is eliminated.  I think I know what you want to say, but it
still is confusing.

>  add::
>  	Add the given repository as a submodule at the given path
> +	to the changeset to be committed next. This requires two arguments,
> +	<repository> and <path>. <repository> is the URL of the new
> +	submodule's origin repository. This may be either an absolute URL,
> +	or (if it begins with ./ or ../), the location relative
> +	to the parent repository's origin.

This is much better than the part I found confusing above.  Here, "parent
repository" actually means "this repository", right?  It is the one that
owns the index you are adding the gitlink to and tracks the .gitmodules
file you are adding an entry for this submodule to.

> +	<path> is the relative location for the cloned submodule to
> +	exist in the current tree. If <path> does not exist, then the
> +	module is created by cloning from the named URL. If <path> does
> +	exist and is already a valid git repository, then this is added
> +	to the changeset without cloning. This second form is provided
> +	to ease adding a submodule to a project the first time, and presumes
> +	the user will later push the new submodule to the given URL.
> +
> +	In either case, the given URL is recorded into .gitmodules for
> +	use by subsequent users cloning the project. If the URL is given
> +	relative to the parent, the presumption is the main and sub-modules

We seem to say "superproject" and "submodule" elsewhere, including Linus's
talk.

> +	will be kept together in the same relative location, and only the
> +	top-level URL need be provided: git-submodule will correctly
> +	locat the submodules using the hint in .gitmodules.

s/locat/&e/;

> @@ -150,16 +138,27 @@ cmd_add()
> ...
> +	# assure repo is absolute or relative to parent
> +	case "$repo" in
> +		./*|../*)
> +			# dereference source url relative to parent's url
> +			realrepo=$(resolve_relative_url "$repo") || exit
> +			;;
> +	    *:*|/*)

Funny indentation; "case/esac" and its arms align, like so:

	case "$repo" in
        ./* | ../*)
        	...
		;;
	esac

[PATCH] git-submodule - make "submodule add" more strict, and document it

[Mark Levedahl]

This change makes "submodule add" much more strict in the arguments it
takes, and is intended to address confusion as recently noted on the
git-list. With this change, the required syntax is:

	$ git submodule add URL path

Specifically, this eliminates the form

	$ git submodule add URL

which was confused by more than one person as

	$ git submodule add path

With this patch, the URL locating the submodule's origin repository can be
either an absolute URL, or (if it begins with ./ or ../) can express the
submodule's repository location relative to the superproject's origin.

This patch also eliminates a third form of URL, which was relative to the
superproject's top-level directory (not its repository).  Any URL that was
neither absolute nor matched ./*|../* was assumed to point to a
subdirectory of the superproject as the location of the submodule's origin
repository.  This URL form was confusing and does not seem to correspond
to an important use-case.  Specifically, no-one has identified the need to
clone from a repository already in the superproject's tree, but if this is
needed it is easily done using an absolute URL: $(pwd)/relative-path.  So,
no functionality is lost with this patch. (t6008-rev-list-submodule.sh did
rely upon this relative URL, fixed by using $(pwd).)

Following this change, there are exactly four variants of
submodule-add, as both arguments have two flavors:

URL can be absolute, or can begin with ./|../ and thus names the
submodule's origin relative to the superproject's origin.

Note: With this patch, "submodule add" discerns an absolute URL as
matching /*|*:*: e.g., URL begins with /, or it contains a :.  This works
for all valid URLs, an absolute path in POSIX, as well as an absolute path
on Windows).

path can either already exist as a valid git repo, or will be cloned from
the given URL.  The first form here eases creation of a new submodule in
an existing superproject as the submodule can be added and tested in-tree
before pushing to the public repository.  However, the more usual form is
the second, where the repo is cloned from the given URL.

This specifically addresses the issue of

	$ git submodule add a/b/c

attempting to clone from a repository at "a/b/c" to create a new module
in "c". This also simplifies description of "relative URL" as there is now
exactly *one* form: a URL relative to the parent's origin repo.

Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
This should address Junio's issues. In addition, I found one test
affected by this change and fixed that (the test actually used
the relative path URL to clone, replaced with $(pwd)).

 Documentation/git-submodule.txt |   36 +++++++++++++++++++------
 git-submodule.sh                |   55 ++++++++++++++------------------------
 t/t6008-rev-list-submodule.sh   |    2 +-
 3 files changed, 48 insertions(+), 45 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 105fc2d..1fe13a6 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -9,7 +9,7 @@ git-submodule - Initialize, update or inspect submodules
 SYNOPSIS
 --------
 [verse]
-'git submodule' [--quiet] add [-b branch] [--] <repository> [<path>]
+'git submodule' [--quiet] add [-b branch] [--] <repository> <path>
 'git submodule' [--quiet] status [--cached] [--] [<path>...]
 'git submodule' [--quiet] init [--] [<path>...]
 'git submodule' [--quiet] update [--init] [--] [<path>...]
@@ -20,14 +20,31 @@ COMMANDS
 --------
 add::
 	Add the given repository as a submodule at the given path
-	to the changeset to be committed next.  If path is a valid
-	repository within the project, it is added as is. Otherwise,
-	repository is cloned at the specified path. path is added to the
-	changeset and registered in .gitmodules.   If no path is
-	specified, the path is deduced from the repository specification.
-	If the repository url begins with ./ or ../, it is stored as
-	given but resolved as a relative path from the main project's
-	url when cloning.
+	to the changeset to be committed next to the current
+	project: the current project is termed termed the "superproject".
+
+	This requires two arguments: <repository> and <path>.
+
+	<repository> is the URL of the new submodule's origin repository.
+	This may be either an absolute URL, or (if it begins with ./
+	or ../), the location relative to the superproject's origin
+	repository.
+
+	<path> is the relative location for the cloned submodule to
+	exist in the superproject. If <path> does not exist, then the
+	submodule is created by cloning from the named URL. If <path> does
+	exist and is already a valid git repository, then this is added
+	to the changeset without cloning. This second form is provided
+	to ease creating a new submodule from scratch, and presumes
+	the user will later push the submodule to the given URL.
+
+	In either case, the given URL is recorded into .gitmodules for
+	use by subsequent users cloning the superproject. If the URL is
+	given relative to the superproject's repository, the presumption
+	is the superproject and submodule repositories will be kept
+	together in the same relative location, and only the
+	superproject's URL need be provided: git-submodule will correctly
+	locate the submodule using the relative URL in .gitmodules.

 status::
 	Show the status of the submodules. This will print the SHA-1 of the
@@ -85,6 +102,7 @@ OPTIONS
 <path>::
 	Path to submodule(s). When specified this will restrict the command
 	to only operate on the submodules found at the specified paths.
+	(This argument is required with add).

 FILES
 -----
diff --git a/git-submodule.sh b/git-submodule.sh
index 099a7d7..c2ce2fb 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -5,7 +5,7 @@
 # Copyright (c) 2007 Lars Hjemli

 USAGE="[--quiet] [--cached] \
-[add <repo> [-b branch]|status|init|update [-i|--init]|summary [-n|--summary-limit <n>] [<commit>]] \
+[add <repo> [-b branch] <path>]|[status|init|update [-i|--init]|summary [-n|--summary-limit <n>] [<commit>]] \
 [--] [<path>...]"
 OPTIONS_SPEC=
 . git-sh-setup
@@ -27,18 +27,6 @@ say()
 	fi
 }

-# NEEDSWORK: identical function exists in get_repo_base in clone.sh
-get_repo_base() {
-	(
-		cd "`/bin/pwd`" &&
-		cd "$1" || cd "$1.git" &&
-		{
-			cd .git
-			pwd
-		}
-	) 2>/dev/null
-}
-
 # Resolve relative url by appending to parent's url
 resolve_relative_url ()
 {
@@ -115,7 +103,7 @@ module_clone()
 #
 # Add a new submodule to the working tree, .gitmodules and the index
 #
-# $@ = repo [path]
+# $@ = repo path
 #
 # optional branch is stored in global branch variable
 #
@@ -150,16 +138,27 @@ cmd_add()
 	repo=$1
 	path=$2

-	if test -z "$repo"; then
+	if test -z "$repo" -o -z "$path"; then
 		usage
 	fi

-	# Guess path from repo if not specified or strip trailing slashes
-	if test -z "$path"; then
-		path=$(echo "$repo" | sed -e 's|/*$||' -e 's|:*/*\.git$||' -e 's|.*[/:]||g')
-	else
-		path=$(echo "$path" | sed -e 's|/*$||')
-	fi
+	# assure repo is absolute or relative to parent
+	case "$repo" in
+	./*|../*)
+		# dereference source url relative to parent's url
+		realrepo=$(resolve_relative_url "$repo") || exit
+		;;
+	*:*|/*)
+		# absolute url
+		realrepo=$repo
+		;;
+	*)
+		die "repo URL: '$repo' must be absolute or begin with ./|../"
+	;;
+	esac
+
+	# strip trailing slashes from path
+	path=$(echo "$path" | sed -e 's|/*$||')

 	git ls-files --error-unmatch "$path" > /dev/null 2>&1 &&
 	die "'$path' already exists in the index"
@@ -174,20 +173,6 @@ cmd_add()
 			die "'$path' already exists and is not a valid git repo"
 		fi
 	else
-		case "$repo" in
-		./*|../*)
-			# dereference source url relative to parent's url
-			realrepo=$(resolve_relative_url "$repo") || exit
-			;;
-		*)
-			# Turn the source into an absolute path if
-			# it is local
-			if base=$(get_repo_base "$repo"); then
-				repo="$base"
-			fi
-			realrepo=$repo
-			;;
-		esac

 		module_clone "$path" "$realrepo" || exit
 		(unset GIT_DIR; cd "$path" && git checkout -q ${branch:+-b "$branch" "origin/$branch"}) ||
diff --git a/t/t6008-rev-list-submodule.sh b/t/t6008-rev-list-submodule.sh
index 88e96fb..c4af9ca 100755
--- a/t/t6008-rev-list-submodule.sh
+++ b/t/t6008-rev-list-submodule.sh
@@ -23,7 +23,7 @@ test_expect_success 'setup' '

 	: > super-file &&
 	git add super-file &&
-	git submodule add . sub &&
+	git submodule add "$(pwd)" sub &&
 	git symbolic-ref HEAD refs/heads/super &&
 	test_tick &&
 	git commit -m super-initial &&
--
1.5.6.2.271.g73ad8

[PATCH] git-submodule - register submodule URL if adding in place

[Mark Levedahl]

When adding a new submodule in place, meaning the user created the
submodule as a git repo in the superproject's tree first, we don't go
through "git submodule init" to register the module.  Thus, the
submodule's origin repository URL is not stored in .git/config, and no
subsequent submodule operation will ever do so.  In this case, assume the
URL the user supplies to "submodule add" is the one that should be
registered, and do so.

Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
Dscho's issue, so far as I can tell, was not really with this patch
but is addressed by the preceding patch eliminating the
confusing single argument "submodule add" format that lead to the
error he wanted to avoid. This patch is not changed except that it
is rebased.

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

diff --git a/git-submodule.sh b/git-submodule.sh
index c2ce2fb..9228f56 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -172,6 +172,16 @@ cmd_add()
 		else
 			die "'$path' already exists and is not a valid git repo"
 		fi
+
+		case "$repo" in
+		./*|../*)
+			url=$(resolve_relative_url "$repo") || exit
+		    ;;
+		*)
+			url="$repo"
+			;;
+		esac
+		git config submodule."$path".url "$url"
 	else

 		module_clone "$path" "$realrepo" || exit
--
1.5.6.2.271.g73ad8

Re: [PATCH] fix "git-submodule add a/b/c/repository"

[Sylvain Joyeux]

On Mon, Jul 07, 2008 at 11:26:12PM -0400, Mark Levedahl wrote:
> Junio C Hamano wrote:
>> I'd like to hear clarifications on two counts, please?
>>  (1) If Sylvain wanted to have that appear at dir0/dir1/init not init,
>>      would it have been sufficient to give that path twice (once for
>>      <repository> and another for <path> parameter) to make things work as
>>      expected?
>>   
> git-submodule really requires two arguments:

Then it should raise an error when the following is given

  git submodule add ./relative-path-in-repo

which, for now, is accepted as

  git submodule add ./relative-path-in-repo ./relative-path-in-repo

(and confused me into thinking it was a normal behaviour)

Sylvain