Bug: Recursive submodules fail when the repo path contains spaces

Bug: Recursive submodules fail when the repo path contains spaces

[Justin Spahr-Summers]

This occurs on Mac OS X 10.7.4, on git versions 1.7.10.2 (Apple Git-33) and 1.7.11.3.

Steps:
 1. Create or clone a repository to an absolute path that contains spaces.
 2. Add a submodule to the repository, if it does not already have one.
 3. Within that submodule, attempt to add another submodule.

The result is an error "fatal: Not a git repository", followed by the relative path to the submodule directory within .git/modules of the top-level repository.

Similarly, using "git submodule update --init --recursive" in a freshly-cloned repository that matches the above configuration will fail with the same error. "git clone --recursive" does not seem to suffer from the same problem at clone time, but will still fail to add recursive submodules.

-- 
Justin Spahr-Summers

Re: Bug: Recursive submodules fail when the repo path contains spaces

[Jens Lehmann]

Am 24.07.2012 21:01, schrieb Justin Spahr-Summers:
> This occurs on Mac OS X 10.7.4, on git versions 1.7.10.2 (Apple Git-33) and 1.7.11.3.
> 
> Steps:
>  1. Create or clone a repository to an absolute path that contains spaces.
>  2. Add a submodule to the repository, if it does not already have one.
>  3. Within that submodule, attempt to add another submodule.
> 
> The result is an error "fatal: Not a git repository", followed by the relative path to the submodule directory within .git/modules of the top-level repository.
> 
> Similarly, using "git submodule update --init --recursive" in a freshly-cloned repository that matches the above configuration will fail with the same error. "git clone --recursive" does not seem to suffer from the same problem at clone time, but will still fail to add recursive submodules.

Hmm, I don't understand how that is different from what t7407 does, it uses
"git submodule update --init --recursive" in to populate recursive submodules
in a freshly cloned repository whose path contains a space (in the trash
directory name) in test number 8.

Does the git test suite succeed for you?

Re: Bug: Recursive submodules fail when the repo path contains spaces

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> Am 24.07.2012 21:01, schrieb Justin Spahr-Summers:
>> This occurs on Mac OS X 10.7.4, on git versions 1.7.10.2 (Apple Git-33) and 1.7.11.3.
>> 
>> Steps:
>>  1. Create or clone a repository to an absolute path that contains spaces.
>>  2. Add a submodule to the repository, if it does not already have one.
>>  3. Within that submodule, attempt to add another submodule.
>> 
>> The result is an error "fatal: Not a git repository", followed by the relative path to the submodule directory within .git/modules of the top-level repository.
>> 
>> Similarly, using "git submodule update --init --recursive" in a freshly-cloned repository that matches the above configuration will fail with the same error. "git clone --recursive" does not seem to suffer from the same problem at clone time, but will still fail to add recursive submodules.
>
> Hmm, I don't understand how that is different from what t7407 does, it uses
> "git submodule update --init --recursive" in to populate recursive submodules
> in a freshly cloned repository whose path contains a space (in the trash
> directory name) in test number 8.

I can see one codepath that would behave incorrectly, especially if
the submodule path relative to the superproject has whitespaces in
it.  In module_clone(), you have:

	# We already are at the root of the work tree but cd_to_toplevel will
	# resolve any symlinks that might be present in $PWD
	a=$(cd_to_toplevel && cd "$gitdir" && pwd)/
	b=$(cd_to_toplevel && cd "$sm_path" && pwd)/
	...
	# Turn each leading "*/" component into "../"
	rel=$(echo $b | sed -e 's|[^/][^/]*|..|g')
	echo "gitdir: $rel/$a" >"$sm_path/.git"

I _think_ $sm_path is computed correctly by the codeflow leading to
this place, and $b is also computed correctly, but notice the lack
of quoting around $b when you echo it?  It will be split at $IFS, so
if b='/Program Files/My  Stupidity/', the sed script will see a
single SP between My and Stupidity, which is different from what you
wanted to feed, I presume.

Having said that, I do not think git-submodule is prepared to take
paths with path-unsafe characters in it, given that many part of it
has loops like "while read mode sha1 stage sm_path" that reads from
ls-files/ls-tree output without -z (which means it cannot handle
pathnames with LF in them).

My recommendation at this point (i.e. not a long term) for people
with problems Justin saw is "Don't do it then".

Re: Bug: Recursive submodules fail when the repo path contains spaces

[Justin Spahr-Summers]

On Tuesday, 24. July 2012 at 13:08, Jens Lehmann wrote:
> Am 24.07.2012 21:01, schrieb Justin Spahr-Summers:
> > This occurs on Mac OS X 10.7.4, on git versions 1.7.10.2 (Apple Git-33) and 1.7.11.3.
> >  
> > Steps:
> > 1. Create or clone a repository to an absolute path that contains spaces.
> > 2. Add a submodule to the repository, if it does not already have one.
> > 3. Within that submodule, attempt to add another submodule.
> >  
> > The result is an error "fatal: Not a git repository", followed by the relative path to the submodule directory within .git/modules of the top-level repository.
> >  
> > Similarly, using "git submodule update --init --recursive" in a freshly-cloned repository that matches the above configuration will fail with the same error. "git clone --recursive" does not seem to suffer from the same problem at clone time, but will still fail to add recursive submodules.
>  
> Hmm, I don't understand how that is different from what t7407 does, it uses
> "git submodule update --init --recursive" in to populate recursive submodules
> in a freshly cloned repository whose path contains a space (in the trash
> directory name) in test number 8.
>  
> Does the git test suite succeed for you?  
The only test that fails is:  

*** t7501-commit.sh ***
*** t7502-commit.sh ***
…
not ok - 13 commit message from template with whitespace issue



(which I assume is unrelated)

Re: Bug: Recursive submodules fail when the repo path contains spaces

[Justin Spahr-Summers]

On Tuesday, 24. July 2012 at 13:26, Junio C Hamano wrote:
> Jens Lehmann <Jens.Lehmann@web.de (http://web.de)> writes:
> 
> > Am 24.07.2012 21:01, schrieb Justin Spahr-Summers:
> > > This occurs on Mac OS X 10.7.4, on git versions 1.7.10.2 (Apple Git-33) and 1.7.11.3.
> > > 
> > > Steps:
> > > 1. Create or clone a repository to an absolute path that contains spaces.
> > > 2. Add a submodule to the repository, if it does not already have one.
> > > 3. Within that submodule, attempt to add another submodule.
> > > 
> > > The result is an error "fatal: Not a git repository", followed by the relative path to the submodule directory within .git/modules of the top-level repository.
> > > 
> > > Similarly, using "git submodule update --init --recursive" in a freshly-cloned repository that matches the above configuration will fail with the same error. "git clone --recursive" does not seem to suffer from the same problem at clone time, but will still fail to add recursive submodules.
> > 
> > Hmm, I don't understand how that is different from what t7407 does, it uses
> > "git submodule update --init --recursive" in to populate recursive submodules
> > in a freshly cloned repository whose path contains a space (in the trash
> > directory name) in test number 8.
> 
> 
> 
> I can see one codepath that would behave incorrectly, especially if
> the submodule path relative to the superproject has whitespaces in
> it. In module_clone(), you have:
> 
> # We already are at the root of the work tree but cd_to_toplevel will
> # resolve any symlinks that might be present in $PWD
> a=$(cd_to_toplevel && cd "$gitdir" && pwd)/
> b=$(cd_to_toplevel && cd "$sm_path" && pwd)/
> ...
> # Turn each leading "*/" component into "../"
> rel=$(echo $b | sed -e 's|[^/][^/]*|..|g')
> echo "gitdir: $rel/$a" >"$sm_path/.git"
> 
> I _think_ $sm_path is computed correctly by the codeflow leading to
> this place, and $b is also computed correctly, but notice the lack
> of quoting around $b when you echo it? It will be split at $IFS, so
> if b='/Program Files/My Stupidity/', the sed script will see a
> single SP between My and Stupidity, which is different from what you
> wanted to feed, I presume.
> 
> Having said that, I do not think git-submodule is prepared to take
> paths with path-unsafe characters in it, given that many part of it
> has loops like "while read mode sha1 stage sm_path" that reads from
> ls-files/ls-tree output without -z (which means it cannot handle
> pathnames with LF in them).
> 
> My recommendation at this point (i.e. not a long term) for people
> with problems Justin saw is "Don't do it then".

I appreciate the debugging work. Unfortunately, none of the relative submodule paths have had whitespace in them, so I'm not sure that's the issue. 

Here's some real output, with a couple specific names removed, starting from the root of the top-level repository (where External/twui is a submodule):

$ cd External/twui
$ git submodule add git://github.com/petejkim/expecta.git TwUITests/expecta
Cloning into 'TwUITests/expecta'...
remote: Counting objects: 988, done.
remote: Compressing objects: 100% (404/404), done.
remote: Total 988 (delta 680), reused 842 (delta 535)
Receiving objects: 100% (988/988), 156.30 KiB, done.
Resolving deltas: 100% (680/680), done.
fatal: Not a git repository: ../../../../../../../../Volumes/drive name with spaces/Users/justin/Documents/Programming/project name with spaces/.git/modules/External/twui/modules/TwUITests/expecta

Re: Bug: Recursive submodules fail when the repo path contains spaces

[Junio C Hamano]

Justin Spahr-Summers <justin.spahrsummers@gmail.com> writes:

> On Tuesday, 24. July 2012 at 13:26, Junio C Hamano wrote:
> ...
>> I can see one codepath that would behave incorrectly,...
>> ...
>> My recommendation at this point (i.e. not a long term) for people
>> with problems Justin saw is "Don't do it then".
>
> I appreciate the debugging work.

That was not a debug, and I didn't mean to say that was the only
place that is problematic.  In fact I think I said the script is not
prepared to work with paths with paths-unsafe characters because it
has many problematic constructs, and ended the message with "Don't
do it then".

Re: Bug: Recursive submodules fail when the repo path contains spaces

[Heiko Voigt]

On Tue, Jul 24, 2012 at 01:33:44PM -0700, Justin Spahr-Summers wrote:
> Here's some real output, with a couple specific names removed, starting from the root of the top-level repository (where External/twui is a submodule):
> 
> $ cd External/twui
> $ git submodule add git://github.com/petejkim/expecta.git TwUITests/expecta
> Cloning into 'TwUITests/expecta'...
> remote: Counting objects: 988, done.
> remote: Compressing objects: 100% (404/404), done.
> remote: Total 988 (delta 680), reused 842 (delta 535)
> Receiving objects: 100% (988/988), 156.30 KiB, done.
> Resolving deltas: 100% (680/680), done.
> fatal: Not a git repository: ../../../../../../../../Volumes/drive name with spaces/Users/justin/Documents/Programming/project name with spaces/.git/modules/External/twui/modules/TwUITests/expecta

Is this a copy&paste artefact or is the path in the error truncated?

Cheers Heiko

Re: Bug: Recursive submodules fail when the repo path contains spaces

[Justin Spahr-Summers]

On Saturday, 28. July 2012 at 09:21, Heiko Voigt wrote:
> On Tue, Jul 24, 2012 at 01:33:44PM -0700, Justin Spahr-Summers wrote:
> > Here's some real output, with a couple specific names removed, starting from the root of the top-level repository (where External/twui is a submodule):
> > 
> > $ cd External/twui
> > $ git submodule add git://github.com/petejkim/expecta.git (http://github.com/petejkim/expecta.git) TwUITests/expecta
> > Cloning into 'TwUITests/expecta'...
> > remote: Counting objects: 988, done.
> > remote: Compressing objects: 100% (404/404), done.
> > remote: Total 988 (delta 680), reused 842 (delta 535)
> > Receiving objects: 100% (988/988), 156.30 KiB, done.
> > Resolving deltas: 100% (680/680), done.
> > fatal: Not a git repository: ../../../../../../../../Volumes/drive name with spaces/Users/justin/Documents/Programming/project name with spaces/.git/modules/External/twui/modules/TwUITests/expecta
> 
> 
> 
> Is this a copy&paste artefact or is the path in the error truncated? 
There's apparently some weird wrapping from my email client (the git error is on a single line in the terminal), but there is no truncation. That's the full path. Thanks for clarifying, though.

Re: Bug: Recursive submodules fail when the repo path contains spaces

[Phil Hord]

On Tue, Jul 24, 2012 at 4:33 PM, Justin Spahr-Summers
<justin.spahrsummers@gmail.com> wrote:
> On Tuesday, 24. July 2012 at 13:26, Junio C Hamano wrote:
>> Jens Lehmann <Jens.Lehmann@web.de (http://web.de)> writes:
>>
>> > Am 24.07.2012 21:01, schrieb Justin Spahr-Summers:
>> > > This occurs on Mac OS X 10.7.4, on git versions 1.7.10.2 (Apple Git-33) and 1.7.11.3.
>> > >
>> > > Steps:
>> > > 1. Create or clone a repository to an absolute path that contains spaces.
>> > > 2. Add a submodule to the repository, if it does not already have one.
>> > > 3. Within that submodule, attempt to add another submodule.
>> > >
>> > > The result is an error "fatal: Not a git repository", followed by the relative path to the submodule directory within .git/modules of the top-level repository.
>> > >
>> > > Similarly, using "git submodule update --init --recursive" in a freshly-cloned repository that matches the above configuration will fail with the same error. "git clone --recursive" does not seem to suffer from the same problem at clone time, but will still fail to add recursive submodules.
>> >
>> > Hmm, I don't understand how that is different from what t7407 does, it uses
>> > "git submodule update --init --recursive" in to populate recursive submodules
>> > in a freshly cloned repository whose path contains a space (in the trash
>> > directory name) in test number 8.
>>
>>
>>
>> I can see one codepath that would behave incorrectly, especially if
>> the submodule path relative to the superproject has whitespaces in
>> it. In module_clone(), you have:
>>
>> # We already are at the root of the work tree but cd_to_toplevel will
>> # resolve any symlinks that might be present in $PWD
>> a=$(cd_to_toplevel && cd "$gitdir" && pwd)/
>> b=$(cd_to_toplevel && cd "$sm_path" && pwd)/
>> ...
>> # Turn each leading "*/" component into "../"
>> rel=$(echo $b | sed -e 's|[^/][^/]*|..|g')
>> echo "gitdir: $rel/$a" >"$sm_path/.git"
>>
>> I _think_ $sm_path is computed correctly by the codeflow leading to
>> this place, and $b is also computed correctly, but notice the lack
>> of quoting around $b when you echo it? It will be split at $IFS, so
>> if b='/Program Files/My Stupidity/', the sed script will see a
>> single SP between My and Stupidity, which is different from what you
>> wanted to feed, I presume.
>>
>> Having said that, I do not think git-submodule is prepared to take
>> paths with path-unsafe characters in it, given that many part of it
>> has loops like "while read mode sha1 stage sm_path" that reads from
>> ls-files/ls-tree output without -z (which means it cannot handle
>> pathnames with LF in them).
>>
>> My recommendation at this point (i.e. not a long term) for people
>> with problems Justin saw is "Don't do it then".
>
> I appreciate the debugging work. Unfortunately, none of the relative submodule paths have had whitespace in them, so I'm not sure that's the issue.
>
> Here's some real output, with a couple specific names removed, starting from the root of the top-level repository (where External/twui is a submodule):
>
> $ cd External/twui
> $ git submodule add git://github.com/petejkim/expecta.git TwUITests/expecta
> Cloning into 'TwUITests/expecta'...
> remote: Counting objects: 988, done.
> remote: Compressing objects: 100% (404/404), done.
> remote: Total 988 (delta 680), reused 842 (delta 535)
> Receiving objects: 100% (988/988), 156.30 KiB, done.
> Resolving deltas: 100% (680/680), done.
> fatal: Not a git repository: ../../../../../../../../Volumes/drive name with spaces/Users/justin/Documents/Programming/project name with spaces/.git/modules/External/twui/modules/TwUITests/expecta

I saw something similar before but I was unable to reproduce the
conditions.  In my case, my superproject was cloned with an earlier
version of git and some new submodule behaviors seemed to cause me
problems.

Is "project name with spaces" a freshly cloned project also using,
say, 1.7.11.3?

I am not able to reproduce the problem here on Linux using your same
example command.

Phil

Re: Bug: Recursive submodules fail when the repo path contains spaces

[Justin Spahr-Summers]

On Sunday, 29. July 2012 at 11:31, Phil Hord wrote:
> On Tue, Jul 24, 2012 at 4:33 PM, Justin Spahr-Summers
> <justin.spahrsummers@gmail.com (mailto:justin.spahrsummers@gmail.com)> wrote:
> > On Tuesday, 24. July 2012 at 13:26, Junio C Hamano wrote:
> > > Jens Lehmann <Jens.Lehmann@web.de (http://web.de)> writes:
> > > 
> > > > Am 24.07.2012 21:01, schrieb Justin Spahr-Summers:
> > > > > This occurs on Mac OS X 10.7.4, on git versions 1.7.10.2 (Apple Git-33) and 1.7.11.3.
> > > > > 
> > > > > Steps:
> > > > > 1. Create or clone a repository to an absolute path that contains spaces.
> > > > > 2. Add a submodule to the repository, if it does not already have one.
> > > > > 3. Within that submodule, attempt to add another submodule.
> > > > > 
> > > > > The result is an error "fatal: Not a git repository", followed by the relative path to the submodule directory within .git/modules of the top-level repository.
> > > > > 
> > > > > Similarly, using "git submodule update --init --recursive" in a freshly-cloned repository that matches the above configuration will fail with the same error. "git clone --recursive" does not seem to suffer from the same problem at clone time, but will still fail to add recursive submodules.
> > > > 
> > > > Hmm, I don't understand how that is different from what t7407 does, it uses
> > > > "git submodule update --init --recursive" in to populate recursive submodules
> > > > in a freshly cloned repository whose path contains a space (in the trash
> > > > directory name) in test number 8.
> > > 
> > > 
> > > 
> > > 
> > > 
> > > I can see one codepath that would behave incorrectly, especially if
> > > the submodule path relative to the superproject has whitespaces in
> > > it. In module_clone(), you have:
> > > 
> > > # We already are at the root of the work tree but cd_to_toplevel will
> > > # resolve any symlinks that might be present in $PWD
> > > a=$(cd_to_toplevel && cd "$gitdir" && pwd)/
> > > b=$(cd_to_toplevel && cd "$sm_path" && pwd)/
> > > ...
> > > # Turn each leading "*/" component into "../"
> > > rel=$(echo $b | sed -e 's|[^/][^/]*|..|g')
> > > echo "gitdir: $rel/$a" >"$sm_path/.git"
> > > 
> > > I _think_ $sm_path is computed correctly by the codeflow leading to
> > > this place, and $b is also computed correctly, but notice the lack
> > > of quoting around $b when you echo it? It will be split at $IFS, so
> > > if b='/Program Files/My Stupidity/', the sed script will see a
> > > single SP between My and Stupidity, which is different from what you
> > > wanted to feed, I presume.
> > > 
> > > Having said that, I do not think git-submodule is prepared to take
> > > paths with path-unsafe characters in it, given that many part of it
> > > has loops like "while read mode sha1 stage sm_path" that reads from
> > > ls-files/ls-tree output without -z (which means it cannot handle
> > > pathnames with LF in them).
> > > 
> > > My recommendation at this point (i.e. not a long term) for people
> > > with problems Justin saw is "Don't do it then".
> > 
> > 
> > 
> > I appreciate the debugging work. Unfortunately, none of the relative submodule paths have had whitespace in them, so I'm not sure that's the issue.
> > 
> > Here's some real output, with a couple specific names removed, starting from the root of the top-level repository (where External/twui is a submodule):
> > 
> > $ cd External/twui
> > $ git submodule add git://github.com/petejkim/expecta.git (http://github.com/petejkim/expecta.git) TwUITests/expecta
> > Cloning into 'TwUITests/expecta'...
> > remote: Counting objects: 988, done.
> > remote: Compressing objects: 100% (404/404), done.
> > remote: Total 988 (delta 680), reused 842 (delta 535)
> > Receiving objects: 100% (988/988), 156.30 KiB, done.
> > Resolving deltas: 100% (680/680), done.
> > fatal: Not a git repository: ../../../../../../../../Volumes/drive name with spaces/Users/justin/Documents/Programming/project name with spaces/.git/modules/External/twui/modules/TwUITests/expecta
> 
> 
> 
> I saw something similar before but I was unable to reproduce the
> conditions. In my case, my superproject was cloned with an earlier
> version of git and some new submodule behaviors seemed to cause me
> problems.
> 
> Is "project name with spaces" a freshly cloned project also using,
> say, 1.7.11.3?
> 
> I am not able to reproduce the problem here on Linux using your same
> example command.

Hmm, you're right. After further investigation, this actually looks like a weird issue with soft links. Here's some exact output (with spacing only for clarity), using a public test repository at https://github.com/jspahrsummers/recursive-submodules-test:

justin:~/ $ ls -l HDD
lrwxr-xr-x 1 justin staff 34 5 Jan 2012 HDD -> /Volumes/fadelah data/Users/justin

justin:~/ $ cd HDD
justin:HDD/ $ pwd
/Users/justin/HDD

justin:HDD/ $ git clone git@github.com:jspahrsummers/recursive-submodules-test.git
Cloning into 'recursive-submodules-test'...
remote: Counting objects: 6, done.
remote: Compressing objects: 100% (4/4), done.
remote: Total 6 (delta 1), reused 5 (delta 0)
Receiving objects: 100% (6/6), done.
Resolving deltas: 100% (1/1), done.

justin:HDD/ $ cd recursive-submodules-test
justin:recursive-submodules-test/ (git)-[master] $ git submodule update --init --recursive
Submodule 'objective-git' (git://github.com/libgit2/objective-git.git) registered for path 'objective-git'
Cloning into 'objective-git'...
remote: Counting objects: 2990, done.
remote: Compressing objects: 100% (981/981), done.
remote: Total 2990 (delta 2138), reused 2751 (delta 1915)
Receiving objects: 100% (2990/2990), 7.20 MiB | 1.21 MiB/s, done.
Resolving deltas: 100% (2138/2138), done.
Submodule path 'objective-git': checked out '58d0dc708876038e0db34447a176614956f3f480'
Submodule 'libgit2' (git://github.com/libgit2/libgit2.git) registered for path 'libgit2'
Cloning into 'libgit2'...
remote: Counting objects: 19703, done.
remote: Compressing objects: 100% (5743/5743), done.
remote: Total 19703 (delta 14448), reused 18712 (delta 13585)
Receiving objects: 100% (19703/19703), 5.87 MiB | 498 KiB/s, done.
Resolving deltas: 100% (14448/14448), done.
fatal: Not a git repository: ../../../../../../Volumes/fadelah data/Users/justin/recursive-submodules-test/.git/modules/objective-git/modules/libgit2
Failed to recurse into submodule path 'objective-git'

Re: Bug: Recursive submodules fail when the repo path contains spaces

[Phil Hord]

On Sun, Jul 29, 2012 at 3:30 PM, Justin Spahr-Summers
<justin.spahrsummers@gmail.com> wrote:
> Hmm, you're right. After further investigation, this actually looks like a weird issue with soft links. Here's some exact output (with spacing only for clarity), using a public test repository at https://github.com/jspahrsummers/recursive-submodules-test:

I can reproduce the problem now on 1.7.10.3.  However the problem goes
away in 1.7.12 since it was fixed in

6eafa6d096ce6b0ae20e4c0fbb248958559daf64
Author: Jens Lehmann <Jens.Lehmann@web.de>
Date:   Thu Jul 12 19:45:32 2012 +0200
Subject: submodules: don't stumble over symbolic links when cloning recursively

Regards,
Phil

Re: Bug: Recursive submodules fail when the repo path contains spaces

[Justin Spahr-Summers]

On Sunday, 29. July 2012 at 14:10, Phil Hord wrote:
> On Sun, Jul 29, 2012 at 3:30 PM, Justin Spahr-Summers
> <justin.spahrsummers@gmail.com (mailto:justin.spahrsummers@gmail.com)> wrote:
> > Hmm, you're right. After further investigation, this actually looks like a weird issue with soft links. Here's some exact output (with spacing only for clarity), using a public test repository at https://github.com/jspahrsummers/recursive-submodules-test:
> 
> 
> 
> I can reproduce the problem now on 1.7.10.3. However the problem goes
> away in 1.7.12 since it was fixed in
> 
> 6eafa6d096ce6b0ae20e4c0fbb248958559daf64
> Author: Jens Lehmann <Jens.Lehmann@web.de (http://web.de)>
> Date: Thu Jul 12 19:45:32 2012 +0200
> Subject: submodules: don't stumble over symbolic links when cloning recursively
> 
> Regards,
> Phil

Ah, good to know. Thanks, everyone, for the help.