Fwd: Git Submodule Misbehaviour With ./

Fwd: Git Submodule Misbehaviour With ./

[Andrei Thorp]

Hello,

Git (I have version 1.6.1.3) seems to misbehave when it comes to git
submodule add and ./. Here is an example.

================================================
$ cd Configs
$ git submodule add git@git.mercenariesguild.net:obvious.git ./awesome/obvious
Initialized empty Git repository in /home/garoth/Configs/awesome/obvious/.git/
stdin: is not a tty
remote: Counting objects: 32, done.
remote: Compressing objects: 100% (29/29), done.
Receiving objects: 100% (32/32), 5.10 KiB, done.
remote: Total 32 (delta 8), reused 0 (delta 0)
Resolving deltas: 100% (8/8), done.
$ git submodule init
No submodule mapping found in .gitmodules for path 'awesome/obvious'
$ git commit -m "Added obvious submodule"
$ cd ~/Desktop
$ git clone ~/Configs
$ git submodule init
No submodule mapping found in .gitmodules for path 'awesome/obvious'
================================================

Note how when I did git submodule add, I used ./awesome/obvious. The behaviour
of "No submodule mapping found in .gitmodules for path 'awesome/obvious'" only
occurs when you use ./. If I were to use:

$ git submodule add git@git.mercenariesguild.net:obvious.git awesome/obvious

instead, then git handles everything just fine. Thanks to dsal in #git
for helping me
figure this out. Perhaps there is an error with regards to how the
.gitmodules file
is parsed or encoded. I'm not really a git developer, but I wanted to
have this issue
documented at least somewhere.

Thanks,

-Andrei Thorp

[PATCH 0/2] Fix git submodule add for paths with ./

[Michael J Gruber]

As reported by Andrei Thorp <garoth@gmail.com>, git submodule add does
not work with paths containing './'. This series exposes the problem
with tests and fixes it. (For details see the commit messages.)

On a side note: There were no tests at all so far for git submodule add.
Now there are at least two, but there should be more, especially for
adding repos in place and such.

Michael J Gruber (2):
  git submodule: Add test cases for git submodule add
  git submodule: Fix adding of submodules at paths with ./

 git-submodule.sh           |    3 +++
 t/t7400-submodule-basic.sh |   19 +++++++++++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

[PATCH 1/2] git submodule: Add test cases for git submodule add

[Michael J Gruber]

Add two simple test cases for adding and initialising submodules. The
init step is necessary in order to verify the added information.

The second test exposes a known breakage due to './' in the path: git
ls-files simplifies the path but git add does not, which leads to git
init looking for different lines in .gitmodules than git add adds.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Note: Tests for add and init are somewhat interdependent in the sense
that you need either in order to test success for the other. I put the
add tests first because you need to add a submodule before you can init
it.

 t/t7400-submodule-basic.sh |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index b8cb2df..b154050 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -47,6 +47,25 @@ test_expect_success 'Prepare submodule testing' '
 	GIT_CONFIG=.gitmodules git config submodule.example.url git://example.com/init.git
 '
 
+test_expect_success 'Prepare submodule add testing' '
+	submodurl=$(pwd)
+	mkdir addtest &&
+	pushd addtest &&
+	git init
+'
+
+test_expect_success 'submodule add' '
+	git submodule add "$submodurl" submod &&
+	git submodule init
+'
+
+test_expect_failure 'submodule add with ./ in path' '
+	git submodule add "$submodurl" ././dotsubmod/./frotz/./ &&
+	git submodule init
+'
+# end of submodule add testing
+popd >/dev/null
+
 test_expect_success 'status should fail for unmapped paths' '
 	if git submodule status
 	then
-- 
1.6.2.rc1.30.gd43c

[PATCH 2/2] git submodule: Fix adding of submodules at paths with ./

[Michael J Gruber]

Make 'git submodule add' strip './' from the submodule path in the same
way as 'git ls-files' does, so that 'git submodule init' looks up the
information in .gitmodules with the same key under which 'git submodule
add' stores it.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 git-submodule.sh           |    3 +++
 t/t7400-submodule-basic.sh |    2 +-
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 204aab6..4f26be4 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -167,6 +167,9 @@ cmd_add()
 	;;
 	esac
 
+	# strip superfluous ./ from path
+	path=$(echo "$path" | sed -e 's|^\(\./\)*||' -e's|/\./|/|g')
+
 	# strip trailing slashes from path
 	path=$(echo "$path" | sed -e 's|/*$||')
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index b154050..1757732 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -59,7 +59,7 @@ test_expect_success 'submodule add' '
 	git submodule init
 '
 
-test_expect_failure 'submodule add with ./ in path' '
+test_expect_success 'submodule add with ./ in path' '
 	git submodule add "$submodurl" ././dotsubmod/./frotz/./ &&
 	git submodule init
 '
-- 
1.6.2.rc1.30.gd43c

Re: [PATCH 2/2] git submodule: Fix adding of submodules at paths with ./

[Johannes Sixt]

Michael J Gruber schrieb:
> +	# strip superfluous ./ from path
> +	path=$(echo "$path" | sed -e 's|^\(\./\)*||' -e's|/\./|/|g')

At a minimum:

	path=$(echo "$path" | sed -e 's|^/\(\./\)*|/|g' -e's|^\./||')

Otherwise you would turn "foo./bar" into "foobar", right?

But why only care about ./ but not ../ or /// or trailing / as well?

-- Hannes

Re: [PATCH 2/2] git submodule: Fix adding of submodules at paths with ./

[Michael J Gruber]

Johannes Sixt venit, vidit, dixit 25.02.2009 12:21:
> Michael J Gruber schrieb:
>> +	# strip superfluous ./ from path
>> +	path=$(echo "$path" | sed -e 's|^\(\./\)*||' -e's|/\./|/|g')
> 
> At a minimum:
> 
> 	path=$(echo "$path" | sed -e 's|^/\(\./\)*|/|g' -e's|^\./||')
> 
> Otherwise you would turn "foo./bar" into "foobar", right?

Wrong.

My regexp:
echo "foo./bar"  | sed -e 's|^\(\./\)*||' -e's|/\./|/|g'
foo./bar

echo "foo/./bar" | sed -e 's|^\(\./\)*||' -e's|/\./|/|g'
foo/bar


Your regexp:
echo "foo./bar"  | sed -e 's|^/\(\./\)*|/|g' -e's|^\./||'
foo./bar

echo "foo/./bar" | sed -e 's|^/\(\./\)*|/|g' -e's|^\./||'
foo/./bar

Testing your claim isn't that hard to do before hand...

> But why only care about ./ but not ../ or /// or trailing / as well?

You really haven't even looked at the included context of the patch
(which you stripped), have you? I mean, I'm open for suggestions and
criticism, but please don't shoot blindly.

./ have been reported and are a common use case.

Trailing / are dealt with by the code (see context).

Now, the /// are in fact a valid concern[1], although probably not that
common an isue. If we really want to cater for that (and more) we need
to implement normalize_path_copy() (from path.c). There it says:

* Performs the following normalizations on src, storing the result in dst:
 * - Ensures that components are separated by '/' (Windows only)
 * - Squashes sequences of '/'.
 * - Removes "." components.
 * - Removes ".." components, and the components the precede them.
 * Returns failure (non-zero) if a ".." component appears as first path
 * component anytime during the normalization. Otherwise, returns
success (0).

OK, two more regexps for sed. Thanks, I don't need suggestions for the
regexps ;)

I can add /// and .. testing and handling in a v2, but I'll definitely
leave the Windoze paths to someone who can actually test on Win.

Michael

[1] git sm init uses git ls-files, which in turn,..., which calls
normalize_path_copy. git sm add can't use git ls-files because it has to
deal with an as yet unknown path.

Re: [PATCH 2/2] git submodule: Fix adding of submodules at paths with ./

[Johannes Sixt]

Michael J Gruber schrieb:
> Johannes Sixt venit, vidit, dixit 25.02.2009 12:21:
>> Michael J Gruber schrieb:
>>> +	# strip superfluous ./ from path
>>> +	path=$(echo "$path" | sed -e 's|^\(\./\)*||' -e's|/\./|/|g')
>> At a minimum:
>>
>> 	path=$(echo "$path" | sed -e 's|^/\(\./\)*|/|g' -e's|^\./||')
>>
>> Otherwise you would turn "foo./bar" into "foobar", right?
> 
> Wrong.

Ouch! Point taken. I didn't notice the ^ in the first expression, and I
even copy-pasted and edited it!

> Now, the /// are in fact a valid concern[1], although probably not that
> common an isue.

I'm fine if only common use cases are taken care of. If you say that ./ is
common and /// is not, then I'll believe it because I'm not a git
submodule user (yet) and can't argue about this with my own experience.

-- Hannes

[PATCHv2 0/4] Fix git submodule add for funky paths

[Michael J Gruber]

As reported by Andrei Thorp <garoth@gmail.com>, git submodule add does
not work with paths containing './'. This series exposes the problem
with tests and fixes it. (For details see the commit messages.)

On a side note: There were no tests at all so far for git submodule add.
Now there are at least two, but there should be more, especially for
adding repos in place and such.

v2 adds 2 more tests for paths with // and /.. and helps g sm add pass the tests.

Michael J Gruber (4):
  git submodule: Add test cases for git submodule add
  git submodule: Fix adding of submodules at paths with ./
  git submodule: Add more tests for add with funky paths
  git submodule: Fix handling of // and /.. in paths for added
    submodules

 git-submodule.sh           |    9 +++++++++
 t/t7400-submodule-basic.sh |   29 +++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 0 deletions(-)

[PATCHv2 1/4] git submodule: Add test cases for git submodule add

[Michael J Gruber]

Add two simple test cases for adding and initialising submodules. The
init step is necessary in order to verify the added information.

The second test exposes a known breakage due to './' in the path: git
ls-files simplifies the path but git add does not, which leads to git
init looking for different lines in .gitmodules than git add adds.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Note: Tests for add and init are somewhat interdependent in the sense
that you need either in order to test success for the other. I put the
add tests first because you need to add a submodule before you can init
it.

 t/t7400-submodule-basic.sh |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index b8cb2df..b154050 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -47,6 +47,25 @@ test_expect_success 'Prepare submodule testing' '
 	GIT_CONFIG=.gitmodules git config submodule.example.url git://example.com/init.git
 '
 
+test_expect_success 'Prepare submodule add testing' '
+	submodurl=$(pwd)
+	mkdir addtest &&
+	pushd addtest &&
+	git init
+'
+
+test_expect_success 'submodule add' '
+	git submodule add "$submodurl" submod &&
+	git submodule init
+'
+
+test_expect_failure 'submodule add with ./ in path' '
+	git submodule add "$submodurl" ././dotsubmod/./frotz/./ &&
+	git submodule init
+'
+# end of submodule add testing
+popd >/dev/null
+
 test_expect_success 'status should fail for unmapped paths' '
 	if git submodule status
 	then
-- 
1.6.2.rc1.30.gd43c

[PATCHv2 2/4] git submodule: Fix adding of submodules at paths with ./

[Michael J Gruber]

Make 'git submodule add' strip './' from the submodule path in the same
way as 'git ls-files' does, so that 'git submodule init' looks up the
information in .gitmodules with the same key under which 'git submodule
add' stores it.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Note that the init step implicitly tests whether add recorded the correct path.
 
   git-submodule.sh           |    3 +++
 t/t7400-submodule-basic.sh |    2 +-
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 204aab6..4f26be4 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -167,6 +167,9 @@ cmd_add()
 	;;
 	esac
 
+	# strip superfluous ./ from path
+	path=$(echo "$path" | sed -e 's|^\(\./\)*||' -e's|/\./|/|g')
+
 	# strip trailing slashes from path
 	path=$(echo "$path" | sed -e 's|/*$||')
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index b154050..1757732 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -59,7 +59,7 @@ test_expect_success 'submodule add' '
 	git submodule init
 '
 
-test_expect_failure 'submodule add with ./ in path' '
+test_expect_success 'submodule add with ./ in path' '
 	git submodule add "$submodurl" ././dotsubmod/./frotz/./ &&
 	git submodule init
 '
-- 
1.6.2.rc1.30.gd43c

[PATCHv2 3/4] git submodule: Add more tests for add with funky paths

[Michael J Gruber]

Add 2 more tests which test paths with // and /.., both of which fail
currently.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7400-submodule-basic.sh |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 1757732..21bdfeb 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -63,6 +63,16 @@ test_expect_success 'submodule add with ./ in path' '
 	git submodule add "$submodurl" ././dotsubmod/./frotz/./ &&
 	git submodule init
 '
+
+test_expect_failure 'submodule add with // in path' '
+	git submodule add "$submodurl" slashslashsubmod///frotz// &&
+	git submodule init
+'
+
+test_expect_failure 'submodule add with /.. in path' '
+	git submodule add "$submodurl" dotdotsubmod/../realsubmod/frotz/.. &&
+	git submodule init
+'
 # end of submodule add testing
 popd >/dev/null
 
-- 
1.6.2.rc1.30.gd43c

[PATCHv2 4/4] git submodule: Fix handling of // and /.. in paths for added submodules

[Michael J Gruber]

This hopefully brings "git submodule add"'s parsing of paths in line
with "git submodule init"'s (which uses "git ls-files"), at least for
practical purposes.

It fixes 2 known breakages which are marked "success" now.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 git-submodule.sh           |    6 ++++++
 t/t7400-submodule-basic.sh |    4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 4f26be4..375362e 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -167,6 +167,12 @@ cmd_add()
 	;;
 	esac
 
+	# simplify multiple /
+	path=$(echo "$path" | sed -e 's|/\+|/|g')
+
+	# resolve /.. (add trailing / for matching /..$)
+	path=$(echo "$path/" | sed -e 's|\([^/]*\)/../||g')
+
 	# strip superfluous ./ from path
 	path=$(echo "$path" | sed -e 's|^\(\./\)*||' -e's|/\./|/|g')
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 21bdfeb..473d0e8 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -64,12 +64,12 @@ test_expect_success 'submodule add with ./ in path' '
 	git submodule init
 '
 
-test_expect_failure 'submodule add with // in path' '
+test_expect_success 'submodule add with // in path' '
 	git submodule add "$submodurl" slashslashsubmod///frotz// &&
 	git submodule init
 '
 
-test_expect_failure 'submodule add with /.. in path' '
+test_expect_success 'submodule add with /.. in path' '
 	git submodule add "$submodurl" dotdotsubmod/../realsubmod/frotz/.. &&
 	git submodule init
 '
-- 
1.6.2.rc1.30.gd43c

Re: [PATCHv2 4/4] git submodule: Fix handling of // and /.. in paths for added submodules

[Johannes Sixt]

Michael J Gruber schrieb:
> @@ -167,6 +167,12 @@ cmd_add()
>  	;;
>  	esac
>  
> +	# simplify multiple /
> +	path=$(echo "$path" | sed -e 's|/\+|/|g')

I think we have so far avoided \+ in sed expressions for portability reasons.

> +
> +	# resolve /.. (add trailing / for matching /..$)
> +	path=$(echo "$path/" | sed -e 's|\([^/]*\)/../||g')

This does not work if there are more than two ../ in a row:

	$ echo a/b/c/../../d | sed -e 's|\([^/]*\)/\.\./||g'
	a/b/../d

(and make this /\.\./ instead of /../).

> +
>  	# strip superfluous ./ from path
>  	path=$(echo "$path" | sed -e 's|^\(\./\)*||' -e's|/\./|/|g')

The latter two transformations should be swapped; otherwise you would
transform foo/./../bar into foo/bar.

Perhaps it's now time to write this as:

	# normalize path
	path=$(printf '%s\n' "$path" |
		 sed -e '
			# simplify multiple /
			s|//*|/|g
			# strip superfluous ./
			s|^\(\./\)*||
			s|/\./|/|g
			# resolve /..
			s|\([^/]*\)/\.\./||g
			# strip trailing slashes
			s|/*$||
		')

But unless you know how to solve the ../../ case with a sed program, I
suggest that you keep things simple and take care only of the common cases.

-- Hannes

Re: [PATCHv2 4/4] git submodule: Fix handling of // and /.. in paths for added submodules

[Michael J Gruber]

Johannes Sixt venit, vidit, dixit 25.02.2009 15:06:
> Michael J Gruber schrieb:
>> @@ -167,6 +167,12 @@ cmd_add()
>>  	;;
>>  	esac
>>  
>> +	# simplify multiple /
>> +	path=$(echo "$path" | sed -e 's|/\+|/|g')
> 
> I think we have so far avoided \+ in sed expressions for portability reasons.

Hmmpf. Is it that new, or gnu specific? I'm always afraid of portability
issues with bash but wasn't aware of sed being an issue as well.

In any case, would 's|\\*|/|g' be better (more portable) then?

>> +
>> +	# resolve /.. (add trailing / for matching /..$)
>> +	path=$(echo "$path/" | sed -e 's|\([^/]*\)/../||g')
> 
> This does not work if there are more than two ../ in a row:
> 
> 	$ echo a/b/c/../../d | sed -e 's|\([^/]*\)/\.\./||g'
> 	a/b/../d

Now you got me ;)
It seems I misunderstood the meaning of 'g'. It's really multiple
matches in one step, not recursively matching multiple times.

> (and make this /\.\./ instead of /../).

Oh yes, you're right, my oversight. Thanks!

I guess this is why test writing and coding should not be done by the
same person...

>> +
>>  	# strip superfluous ./ from path
>>  	path=$(echo "$path" | sed -e 's|^\(\./\)*||' -e's|/\./|/|g')
> 
> The latter two transformations should be swapped; otherwise you would
> transform foo/./../bar into foo/bar.

Yes! I just checked, and git ls-files get's this right, so g-sm-add
should as well.

> Perhaps it's now time to write this as:
> 
> 	# normalize path
> 	path=$(printf '%s\n' "$path" |
> 		 sed -e '
> 			# simplify multiple /
> 			s|//*|/|g
> 			# strip superfluous ./
> 			s|^\(\./\)*||
> 			s|/\./|/|g
> 			# resolve /..
> 			s|\([^/]*\)/\.\./||g
> 			# strip trailing slashes
> 			s|/*$||
> 		')
> 
> But unless you know how to solve the ../../ case with a sed program, I
> suggest that you keep things simple and take care only of the common cases.
> 
> -- Hannes

Well, how is

echo a/b/c/../../d | sed -e ':start;s|\([^/]*\)/\.\./||g;tstart'
a/d

I meant: how portable is...

Cheers,
Michael

Re: [PATCHv2 4/4] git submodule: Fix handling of // and /.. in paths for added submodules

[Johannes Sixt]

Michael J Gruber schrieb:
> Johannes Sixt venit, vidit, dixit 25.02.2009 15:06:
>> I think we have so far avoided \+ in sed expressions for portability reasons.
> 
> Hmmpf. Is it that new, or gnu specific? I'm always afraid of portability
> issues with bash but wasn't aware of sed being an issue as well.
> 
> In any case, would 's|\\*|/|g' be better (more portable) then?

You mean 's|//*|/|g'; yes, that is definitly portable.

> Well, how is
> 
> echo a/b/c/../../d | sed -e ':start;s|\([^/]*\)/\.\./||g;tstart'
> a/d
> 
> I meant: how portable is...

I don't know... Let's see: My AIX 4.3.3 sed understands it if it is not
all on a single line, and that says a lot. Specifically, I tried this:

echo a/b/c/../../d | sed -e ':start
             s|\([^/]*\)/\.\./||
             tstart
'

and got the desired result:

a/d

Note that the 'g' flag is not necessary in this case.

OTOH, this sed doesn't understand #comments :-/

-- Hannes

Re: [PATCHv2 4/4] git submodule: Fix handling of // and /.. in paths for added submodules

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Michael J Gruber schrieb:
>> Johannes Sixt venit, vidit, dixit 25.02.2009 15:06:
>>> I think we have so far avoided \+ in sed expressions for portability reasons.
>> 
>> Hmmpf. Is it that new, or gnu specific? I'm always afraid of portability
>> issues with bash but wasn't aware of sed being an issue as well.
>> 
>> In any case, would 's|\\*|/|g' be better (more portable) then?
>
> You mean 's|//*|/|g'; yes, that is definitly portable.
>
>> Well, how is
>> 
>> echo a/b/c/../../d | sed -e ':start;s|\([^/]*\)/\.\./||g;tstart'
>> a/d
>> 
>> I meant: how portable is...
>
> I don't know... Let's see: My AIX 4.3.3 sed understands it if it is not
> all on a single line, and that says a lot. Specifically, I tried this:
>
> echo a/b/c/../../d | sed -e ':start
>              s|\([^/]*\)/\.\./||
>              tstart
> '
>
> and got the desired result:
>
> a/d
>
> Note that the 'g' flag is not necessary in this case.
>
> OTOH, this sed doesn't understand #comments :-/

Historically, sed was much worse than the shell when it came to the
portability issues, especially before people started to use bash, which
tipped the balance a bit by worsening the situation for the shell side.

The sed scripts in the more important parts of scripted Porcelains avoid
multiple commands on a single line concatenated with ";" mostly by inertia
on my side, but it was acquired exactly from this kind of portability
mess.  IIRC, AIX's was the worst offender. It also got "/A/../B/ { ... }"
wrong in earlier versions.

Re: [PATCHv2 4/4] git submodule: Fix handling of // and /.. in paths for added submodules

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 25.02.2009 22:25:
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
>> Michael J Gruber schrieb:
>>> Johannes Sixt venit, vidit, dixit 25.02.2009 15:06:
>>>> I think we have so far avoided \+ in sed expressions for portability reasons.
>>> Hmmpf. Is it that new, or gnu specific? I'm always afraid of portability
>>> issues with bash but wasn't aware of sed being an issue as well.
>>>
>>> In any case, would 's|\\*|/|g' be better (more portable) then?
>> You mean 's|//*|/|g'; yes, that is definitly portable.
>>
>>> Well, how is
>>>
>>> echo a/b/c/../../d | sed -e ':start;s|\([^/]*\)/\.\./||g;tstart'
>>> a/d
>>>
>>> I meant: how portable is...
>> I don't know... Let's see: My AIX 4.3.3 sed understands it if it is not
>> all on a single line, and that says a lot. Specifically, I tried this:
>>
>> echo a/b/c/../../d | sed -e ':start
>>              s|\([^/]*\)/\.\./||
>>              tstart
>> '
>>
>> and got the desired result:
>>
>> a/d
>>
>> Note that the 'g' flag is not necessary in this case.
>>
>> OTOH, this sed doesn't understand #comments :-/
> 
> Historically, sed was much worse than the shell when it came to the
> portability issues, especially before people started to use bash, which
> tipped the balance a bit by worsening the situation for the shell side.
> 
> The sed scripts in the more important parts of scripted Porcelains avoid
> multiple commands on a single line concatenated with ";" mostly by inertia
> on my side, but it was acquired exactly from this kind of portability
> mess.  IIRC, AIX's was the worst offender. It also got "/A/../B/ { ... }"
> wrong in earlier versions.

I'm a bit confused now. Are you saying that "git-submodule.sh" should
avoid the multiple lines in sed (which work in AIX 4.3.3)? I don't know
how to simplify a/b/c/../../ easily then. Of course one could loop
around in shell rather than using sed's "goto", but that looks ugly.

I think that's my only question before doing a v3 which will be a good
"cd citizen" in tests and test for more idiosyncracies.

Oh wait: Would it be worthwhile to have that shell version of
normalize_path_copy() in git-sh-setup instead?

Michael

Re: [PATCHv2 4/4] git submodule: Fix handling of // and /.. in paths for added submodules

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Junio C Hamano venit, vidit, dixit 25.02.2009 22:25:
>
>> The sed scripts in the more important parts of scripted Porcelains avoid
>> multiple commands on a single line concatenated with ";" mostly by inertia
>> on my side, but it was acquired exactly from this kind of portability
>> mess.  IIRC, AIX's was the worst offender. It also got "/A/../B/ { ... }"
>> wrong in earlier versions.
>
> I'm a bit confused now. Are you saying that "git-submodule.sh" should
> avoid the multiple lines in sed (which work in AIX 4.3.3)?

No, I understand J6t's comment:

>>> I don't know... Let's see: My AIX 4.3.3 sed understands it if it is not
>>> all on a single line, and that says a lot. Specifically, I tried this:

to mean that this is bad:

	sed -e 'a;b;c;d;e'

while this works

	sed -e '
        	a
                b
                c
                d
                e
	'

[PATCHv3 0/2] git submodule: normalize paths before adding

[Michael J Gruber]

This is a rewrite taking into account the advice given by Junio and J6t.
In particular, the tests are good citizens w.r.t. cd'ing around now, the
sed expressions work at least on AIX 4.3.3, and iterations of .. are
tested for and handled correctly.

Sorry I didn't get around to finishing this earlier. Hope this doesn't
mess up any schedules.

Michael J Gruber (2):
  git submodule: Add test cases for git submodule add
  git submodule: Fix adding of submodules at paths with ./, .. and //

 git-submodule.sh           |   15 ++++++++++--
 t/t7400-submodule-basic.sh |   49 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 3 deletions(-)

[PATCHv3 1/2] git submodule: Add test cases for git submodule add

[Michael J Gruber]

Add simple test cases for adding and initialising submodules. The
init step is necessary in order to verify the added information.

The second test exposes a known breakage due to './' in the path: git
ls-files simplifies the path but git add does not, which leads to git
init looking for different lines in .gitmodules than git add adds.

The other tests add test cases for '//' and '..' in the path which
currently fail for the same reason.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
The init step test implicitly whether add and init treat the path in the
same way.

 t/t7400-submodule-basic.sh |   49 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index b8cb2df..4f42585 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -47,6 +47,55 @@ test_expect_success 'Prepare submodule testing' '
 	GIT_CONFIG=.gitmodules git config submodule.example.url git://example.com/init.git
 '
 
+test_expect_success 'Prepare submodule add testing' '
+	submodurl=$(pwd)
+	(
+		mkdir addtest &&
+		cd addtest &&
+		git init
+	)
+'
+
+test_expect_success 'submodule add' '
+	(
+		cd addtest &&
+		git submodule add "$submodurl" submod &&
+		git submodule init
+	)
+'
+
+test_expect_failure 'submodule add with ./ in path' '
+	(
+		cd addtest &&
+		git submodule add "$submodurl" ././dotsubmod/./frotz/./ &&
+		git submodule init
+	)
+'
+
+test_expect_failure 'submodule add with // in path' '
+	(
+		cd addtest &&
+		git submodule add "$submodurl" slashslashsubmod///frotz// &&
+		git submodule init
+	)
+'
+
+test_expect_failure 'submodule add with /.. in path' '
+	(
+		cd addtest &&
+		git submodule add "$submodurl" dotdotsubmod/../realsubmod/frotz/.. &&
+		git submodule init
+	)
+'
+
+test_expect_failure 'submodule add with ./, /.. and // in path' '
+	(
+		cd addtest &&
+		git submodule add "$submodurl" dot/dotslashsubmod/./../..////realsubmod2/frotz//.. &&
+		git submodule init
+	)
+'
+
 test_expect_success 'status should fail for unmapped paths' '
 	if git submodule status
 	then
-- 
1.6.2.rc2

[PATCHv3 2/2] git submodule: Fix adding of submodules at paths with ./, .. and //

[Michael J Gruber]

Make 'git submodule add' normalize the submodule path in the
same way as 'git ls-files' does, so that 'git submodule init' looks up
the information in .gitmodules with the same key under which 'git
submodule add' stores it.

This fixes 4 known breakages.
---
Note that sed on AIX does not like comments, which is why they are not
in the sed expression.

 git-submodule.sh           |   15 ++++++++++++---
 t/t7400-submodule-basic.sh |    8 ++++----
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 204aab6..7a25f47 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -167,9 +167,18 @@ cmd_add()
 	;;
 	esac
 
-	# strip trailing slashes from path
-	path=$(echo "$path" | sed -e 's|/*$||')
-
+	# normalize path:
+	# multiple //; leading ./; /./; /../; trailing /
+	path=$(printf '%s/\n' "$path" |
+		sed -e '
+			s|//*|/|g
+			s|^\(\./\)*||
+			s|/\./|/|g
+			:start
+			s|\([^/]*\)/\.\./||g
+			tstart
+			s|/*$||
+		')
 	git ls-files --error-unmatch "$path" > /dev/null 2>&1 &&
 	die "'$path' already exists in the index"
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 4f42585..1c82f8c 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -64,7 +64,7 @@ test_expect_success 'submodule add' '
 	)
 '
 
-test_expect_failure 'submodule add with ./ in path' '
+test_expect_success 'submodule add with ./ in path' '
 	(
 		cd addtest &&
 		git submodule add "$submodurl" ././dotsubmod/./frotz/./ &&
@@ -72,7 +72,7 @@ test_expect_failure 'submodule add with ./ in path' '
 	)
 '
 
-test_expect_failure 'submodule add with // in path' '
+test_expect_success 'submodule add with // in path' '
 	(
 		cd addtest &&
 		git submodule add "$submodurl" slashslashsubmod///frotz// &&
@@ -80,7 +80,7 @@ test_expect_failure 'submodule add with // in path' '
 	)
 '
 
-test_expect_failure 'submodule add with /.. in path' '
+test_expect_success 'submodule add with /.. in path' '
 	(
 		cd addtest &&
 		git submodule add "$submodurl" dotdotsubmod/../realsubmod/frotz/.. &&
@@ -88,7 +88,7 @@ test_expect_failure 'submodule add with /.. in path' '
 	)
 '
 
-test_expect_failure 'submodule add with ./, /.. and // in path' '
+test_expect_success 'submodule add with ./, /.. and // in path' '
 	(
 		cd addtest &&
 		git submodule add "$submodurl" dot/dotslashsubmod/./../..////realsubmod2/frotz//.. &&
-- 
1.6.2.rc2

Re: [PATCHv3 2/2] git submodule: Fix adding of submodules at paths with ./, .. and //

[Johannes Sixt]

Michael J Gruber schrieb:
> +	# normalize path:
> +	# multiple //; leading ./; /./; /../; trailing /
> +	path=$(printf '%s/\n' "$path" |
> +		sed -e '
> +			s|//*|/|g
> +			s|^\(\./\)*||
> +			s|/\./|/|g
> +			:start
> +			s|\([^/]*\)/\.\./||g

Sorry to say: not yet. This turns "a/b/c/d/../../../d" into "a/b/c/d"
instead of "a/d". Drop the 'g'.

Once this is fixed, I have to ask what should happen with path names like
"../a/b", "../../a/b"? Should there be a warning or error?

Other than that, this expression works on AIX 4.3.3! Note in particular
that '\n' in the printf format string is essential!

> +			tstart
> +			s|/*$||
> +		')

-- Hannes

Re: [PATCHv3 2/2] git submodule: Fix adding of submodules at paths with ./, .. and //

[Michael J Gruber]

Johannes Sixt venit, vidit, dixit 03.03.2009 14:08:
> Michael J Gruber schrieb:
>> +	# normalize path:
>> +	# multiple //; leading ./; /./; /../; trailing /
>> +	path=$(printf '%s/\n' "$path" |
>> +		sed -e '
>> +			s|//*|/|g
>> +			s|^\(\./\)*||
>> +			s|/\./|/|g
>> +			:start
>> +			s|\([^/]*\)/\.\./||g
> 
> Sorry to say: not yet. This turns "a/b/c/d/../../../d" into "a/b/c/d"
> instead of "a/d". Drop the 'g'.

Shoot, I thought I had a complete test case...
Thanks for spotting.

> Once this is fixed, I have to ask what should happen with path names like
> "../a/b", "../../a/b"? Should there be a warning or error?

That triggers the error message when trying to git add a submodule which
is not within the current wc. Same before and after the patch, and I
think that's how it should be.

> Other than that, this expression works on AIX 4.3.3! Note in particular
> that '\n' in the printf format string is essential!

I learned that from you ;)

Michael

> 
>> +			tstart
>> +			s|/*$||
>> +		')
> 
> -- Hannes
> 

[PATCHv4 0/2] git submodule: normalize paths before adding

[Michael J Gruber]

This is a rewrite taking into account the advice given by Junio and J6t.
In particular, the tests are good citizens w.r.t. cd'ing around now, the
sed expressions work at least on AIX 4.3.3, and iterations of .. are
tested for and handled correctly.

Sorry I didn't get around to finishing this earlier. Hope this doesn't
mess up any schedules.

v4 incorporates a fix by J6t regarding a spurious g specifier in the sed expression for "..".

Michael J Gruber (2):
  git submodule: Add test cases for git submodule add
  git submodule: Fix adding of submodules at paths with ./, .. and //

 git-submodule.sh           |   15 ++++++++++--
 t/t7400-submodule-basic.sh |   49 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 3 deletions(-)

[PATCHv4 1/2] git submodule: Add test cases for git submodule add

[Michael J Gruber]

Add simple test cases for adding and initialising submodules. The
init step is necessary in order to verify the added information.

The second test exposes a known breakage due to './' in the path: git
ls-files simplifies the path but git add does not, which leads to git
init looking for different lines in .gitmodules than git add adds.

The other tests add test cases for '//' and '..' in the path which
currently fail for the same reason.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7400-submodule-basic.sh |   49 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index b8cb2df..35a0ede 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -47,6 +47,55 @@ test_expect_success 'Prepare submodule testing' '
 	GIT_CONFIG=.gitmodules git config submodule.example.url git://example.com/init.git
 '
 
+test_expect_success 'Prepare submodule add testing' '
+	submodurl=$(pwd)
+	(
+		mkdir addtest &&
+		cd addtest &&
+		git init
+	)
+'
+
+test_expect_success 'submodule add' '
+	(
+		cd addtest &&
+		git submodule add "$submodurl" submod &&
+		git submodule init
+	)
+'
+
+test_expect_failure 'submodule add with ./ in path' '
+	(
+		cd addtest &&
+		git submodule add "$submodurl" ././dotsubmod/./frotz/./ &&
+		git submodule init
+	)
+'
+
+test_expect_failure 'submodule add with // in path' '
+	(
+		cd addtest &&
+		git submodule add "$submodurl" slashslashsubmod///frotz// &&
+		git submodule init
+	)
+'
+
+test_expect_failure 'submodule add with /.. in path' '
+	(
+		cd addtest &&
+		git submodule add "$submodurl" dotdotsubmod/../realsubmod/frotz/.. &&
+		git submodule init
+	)
+'
+
+test_expect_failure 'submodule add with ./, /.. and // in path' '
+	(
+		cd addtest &&
+		git submodule add "$submodurl" dot/dotslashsubmod/./../..////realsubmod2/a/b/c/d/../../../../frotz//.. &&
+		git submodule init
+	)
+'
+
 test_expect_success 'status should fail for unmapped paths' '
 	if git submodule status
 	then
-- 
1.6.2.rc2

[PATCHv4 2/2] git submodule: Fix adding of submodules at paths with ./, .. and //

[Michael J Gruber]

Make 'git submodule add' normalize the submodule path in the
same way as 'git ls-files' does, so that 'git submodule init' looks up
the information in .gitmodules with the same key under which 'git
submodule add' stores it.

This fixes 4 known breakages.
---
 git-submodule.sh           |   15 ++++++++++++---
 t/t7400-submodule-basic.sh |    8 ++++----
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 204aab6..0a27232 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -167,9 +167,18 @@ cmd_add()
 	;;
 	esac
 
-	# strip trailing slashes from path
-	path=$(echo "$path" | sed -e 's|/*$||')
-
+	# normalize path:
+	# multiple //; leading ./; /./; /../; trailing /
+	path=$(printf '%s/\n' "$path" |
+		sed -e '
+			s|//*|/|g
+			s|^\(\./\)*||
+			s|/\./|/|g
+			:start
+			s|\([^/]*\)/\.\./||
+			tstart
+			s|/*$||
+		')
 	git ls-files --error-unmatch "$path" > /dev/null 2>&1 &&
 	die "'$path' already exists in the index"
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 35a0ede..af690ec 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -64,7 +64,7 @@ test_expect_success 'submodule add' '
 	)
 '
 
-test_expect_failure 'submodule add with ./ in path' '
+test_expect_success 'submodule add with ./ in path' '
 	(
 		cd addtest &&
 		git submodule add "$submodurl" ././dotsubmod/./frotz/./ &&
@@ -72,7 +72,7 @@ test_expect_failure 'submodule add with ./ in path' '
 	)
 '
 
-test_expect_failure 'submodule add with // in path' '
+test_expect_success 'submodule add with // in path' '
 	(
 		cd addtest &&
 		git submodule add "$submodurl" slashslashsubmod///frotz// &&
@@ -80,7 +80,7 @@ test_expect_failure 'submodule add with // in path' '
 	)
 '
 
-test_expect_failure 'submodule add with /.. in path' '
+test_expect_success 'submodule add with /.. in path' '
 	(
 		cd addtest &&
 		git submodule add "$submodurl" dotdotsubmod/../realsubmod/frotz/.. &&
@@ -88,7 +88,7 @@ test_expect_failure 'submodule add with /.. in path' '
 	)
 '
 
-test_expect_failure 'submodule add with ./, /.. and // in path' '
+test_expect_success 'submodule add with ./, /.. and // in path' '
 	(
 		cd addtest &&
 		git submodule add "$submodurl" dot/dotslashsubmod/./../..////realsubmod2/a/b/c/d/../../../../frotz//.. &&
-- 
1.6.2.rc2

Re: [PATCHv4 2/2] git submodule: Fix adding of submodules at paths with ./, .. and //

[Johannes Sixt]

Michael J Gruber schrieb:
> Make 'git submodule add' normalize the submodule path in the
> same way as 'git ls-files' does, so that 'git submodule init' looks up
> the information in .gitmodules with the same key under which 'git
> submodule add' stores it.
> 
> This fixes 4 known breakages.
> ---

OK!

Tested-by: Johannes Sixt <j6t@kdbg.org> (AIX)

but really, I ran only: cd t && make *submodule*

You certainly will sign off this patch?

-- Hannes

Re: [PATCHv4 2/2] git submodule: Fix adding of submodules at paths with ./, .. and //

[Michael J Gruber]

Johannes Sixt venit, vidit, dixit 03.03.2009 16:28:
> Michael J Gruber schrieb:
>> Make 'git submodule add' normalize the submodule path in the
>> same way as 'git ls-files' does, so that 'git submodule init' looks up
>> the information in .gitmodules with the same key under which 'git
>> submodule add' stores it.
>>
>> This fixes 4 known breakages.
>> ---
> 
> OK!
> 
> Tested-by: Johannes Sixt <j6t@kdbg.org> (AIX)
> 
> but really, I ran only: cd t && make *submodule*
> 
> You certainly will sign off this patch?
> 
> -- Hannes

Grrmmml. Yes, 2/2 is sob me just as 1/2 is. This comes from switching
from "git f-p -s" to "git ci -s" recently and forgetting during
rewrite... So please consider all of PATCHv4 to be

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>

and

Sanity-added-by: Johannes Sixt <j.sixt@viscovery.net>

<goes and does git config alias.ci 'commit -s'>

Michael

Re: [PATCHv4 0/2] git submodule: normalize paths before adding

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> This is a rewrite taking into account the advice given by Junio and J6t.
> In particular, the tests are good citizens w.r.t. cd'ing around now, the
> sed expressions work at least on AIX 4.3.3, and iterations of .. are
> tested for and handled correctly.
>
> Sorry I didn't get around to finishing this earlier. Hope this doesn't
> mess up any schedules.

No worries.

Other than obvious documentation fixes, anything sent after -rc0 will not
hit 'master' unless it is a fix, and after -rc1 nothing will hit 'master'
unless it is a regression fix.  Even if some future version of your patch
is queued to 'pu' or 'next, it won't affect the schedule for 1.6.2.

Traditionally, those patches that are irrelevant to the upcoming release
during the -rc period have been discussed on the list and then discarded,
with a request to be resubmit after the release.  Even though I have been
queuing them to 'pu' or 'next' as an experiment during this cycle, it has
not changed that during the -rc period, any new topic will not disrupt the
upcoming release.

Re: [PATCHv3 2/2] git submodule: Fix adding of submodules at paths with ./, .. and //

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Michael J Gruber schrieb:
>> +	# normalize path:
>> +	# multiple //; leading ./; /./; /../; trailing /
>> +	path=$(printf '%s/\n' "$path" |
>> +		sed -e '
>> +			s|//*|/|g
>> +			s|^\(\./\)*||
>> +			s|/\./|/|g
>> +			:start
>> +			s|\([^/]*\)/\.\./||g
>
> Sorry to say: not yet. This turns "a/b/c/d/../../../d" into "a/b/c/d"
> instead of "a/d". Drop the 'g'.
>
> Once this is fixed, I have to ask what should happen with path names like
> "../a/b", "../../a/b"? Should there be a warning or error?
>
> Other than that, this expression works on AIX 4.3.3! Note in particular
> that '\n' in the printf format string is essential!
>
>> +			tstart
>> +			s|/*$||
>> +		')

At some point you should wonder if all of this complication is worth it,
or it makes sense to reject when you see // or /\.\./ in the input.

Re: [PATCHv3 2/2] git submodule: Fix adding of submodules at paths with ./, .. and //

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 03.03.2009 16:36:
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
>> Michael J Gruber schrieb:
>>> +	# normalize path:
>>> +	# multiple //; leading ./; /./; /../; trailing /
>>> +	path=$(printf '%s/\n' "$path" |
>>> +		sed -e '
>>> +			s|//*|/|g
>>> +			s|^\(\./\)*||
>>> +			s|/\./|/|g
>>> +			:start
>>> +			s|\([^/]*\)/\.\./||g
>> Sorry to say: not yet. This turns "a/b/c/d/../../../d" into "a/b/c/d"
>> instead of "a/d". Drop the 'g'.
>>
>> Once this is fixed, I have to ask what should happen with path names like
>> "../a/b", "../../a/b"? Should there be a warning or error?
>>
>> Other than that, this expression works on AIX 4.3.3! Note in particular
>> that '\n' in the printf format string is essential!
>>
>>> +			tstart
>>> +			s|/*$||
>>> +		')
> 
> At some point you should wonder if all of this complication is worth it,
> or it makes sense to reject when you see // or /\.\./ in the input.

I surely do wonder now! This started off treating merely leading ./ as
reported problematic by AT. Then the "do it really right" competition
started, and I think J6t and I came out as clear winners. There were no
other contenders.

Seriously, "git submodule init" does that normalization (by using
ls-files), so I think it does make sense to have it for add as well. git
submodule itself may have semi-porc/semi-plumb character, but if someone
wants to add submodules programmatically there is no simple way around
using "git submodule add", and paths may very well be constructed
relatively.

Michael

Re: [PATCHv3 2/2] git submodule: Fix adding of submodules at paths with ./, .. and //

[Johannes Schindelin]

Hi,

On Tue, 3 Mar 2009, Michael J Gruber wrote:

> Make 'git submodule add' normalize the submodule path in the
> same way as 'git ls-files' does, so that 'git submodule init' looks up
> the information in .gitmodules with the same key under which 'git
> submodule add' stores it.
> 
> This fixes 4 known breakages.

Maybe it is time to turn this into a builtin?

Ciao,
Dscho

Re: [PATCHv3 2/2] git submodule: Fix adding of submodules at paths with ./, .. and //

[Michael J Gruber]

Johannes Schindelin venit, vidit, dixit 03.03.2009 15:29:
> Hi,
> 
> On Tue, 3 Mar 2009, Michael J Gruber wrote:
> 
>> Make 'git submodule add' normalize the submodule path in the
>> same way as 'git ls-files' does, so that 'git submodule init' looks up
>> the information in .gitmodules with the same key under which 'git
>> submodule add' stores it.
>>
>> This fixes 4 known breakages.
> 
> Maybe it is time to turn this into a builtin?

Uhm, you mean all of submodule or just the normalize part? Isn't git
submodule up for some rewriting (.gitmodule...) anyways?

I suggest applying the tests and fix before Bob the In-Builder rolls in.
And no, Bob's not my alter ego...

Michael

Re: [PATCH 2/2] git submodule: Fix adding of submodules at paths with ./

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Michael J Gruber schrieb:
> ...
>> Now, the /// are in fact a valid concern[1], although probably not that
>> common an isue.
>
> I'm fine if only common use cases are taken care of. If you say that ./ is
> common and /// is not, then I'll believe it because I'm not a git
> submodule user (yet) and can't argue about this with my own experience.

Even if it is not worth the complication to the code to fix /// or /../
cases, I think it is worth *detecting* such an incoming path and error
out, telling the user not to give a "funky" path.

Re: [PATCH 1/2] git submodule: Add test cases for git submodule add

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index b8cb2df..b154050 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -47,6 +47,25 @@ test_expect_success 'Prepare submodule testing' '
>  	GIT_CONFIG=.gitmodules git config submodule.example.url git://example.com/init.git
>  '

Please don't chdir around in individual tests.  Instead have each test
begin and end in the original directory given.  Like:

> +test_expect_success 'Prepare submodule add testing' '
> +	submodurl=$(pwd)
> +	mkdir addtest &&
> +	pushd addtest &&
> +	git init
> +'

	(
        	mkdir addtest &&
                cd addtest &&
                git init
	)

> +test_expect_success 'submodule add' '
> +	git submodule add "$submodurl" submod &&
> +	git submodule init
> +'

	(
        	cd addtest &&
                git submodule add ...
	)                

> +test_expect_failure 'submodule add with ./ in path' '
> +	git submodule add "$submodurl" ././dotsubmod/./frotz/./ &&
> +	git submodule init
> +'
> +# end of submodule add testing
> +popd >/dev/null

The rule of thumb is minimize things done outside of tests, like this
popd.

This is to prevent mistaken test sequence to go up too many levels.  E.g.
an earlier test that is supposed to chdir down and make the subsequent
tests begin at one level deeper than the original could fail, but when
tests are run without the "-i" option, the failed test would not abort the
whole sequence.  The subsequent tests run in a wrong place, and one of
them would try to chdir up to come back to the original directory, but
their "cd .." will go up too high, because the first chdir that tried to
go down failed.