Fwd: Git Submodule Misbehaviour With ./

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

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(-)

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(-)

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(-)

Michael J Gruber schrieb:

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

Johannes Sixt venit, vidit, dixit 25.02.2009 12:21:

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...

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.

Michael J Gruber schrieb:

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

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

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(-)

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(-)

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(-)

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(-)

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(-)

Michael J Gruber schrieb:

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

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 /../).

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

Johannes Sixt venit, vidit, dixit 25.02.2009 15:06:

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?

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.

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...

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

Well, how is

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

I meant: how portable is...

Cheers, Michael

Michael J Gruber schrieb:

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

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

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

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.

Junio C Hamano venit, vidit, dixit 25.02.2009 22:25:

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

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

No, I understand J6t's comment:

to mean that this is bad:

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

while this works

	sed -e '
        	a
                b
                c
                d
                e
	'

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(-)

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(-)

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(-)

Michael J Gruber schrieb:

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!

-- Hannes

Johannes Sixt venit, vidit, dixit 03.03.2009 14:08:

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

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.

I learned that from you ;)

Michael

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(-)

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(-)

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(-)

Michael J Gruber schrieb:

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

Johannes Sixt venit, vidit, dixit 03.03.2009 16:28:

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

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

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.

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

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.

Junio C Hamano venit, vidit, dixit 03.03.2009 16:36:

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

Hi,

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

Maybe it is time to turn this into a builtin?

Ciao, Dscho

Johannes Schindelin venit, vidit, dixit 03.03.2009 15:29:

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

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

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.

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

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

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

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

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.