Re: [BUG] git submodule update is not fail safe

Manlio Perillo [off-list ref] writes:

$ git submodule update --init
...
Submodule 'roms/vgabios' (git://git.qemu.org/vgabios.git/) registered
for path 'roms/vgabios'
fatal: unable to connect to anongit.freedesktop.org:
anongit.freedesktop.org[0: 131.252.210.161]: errno=Connection timed out

Unable to fetch in submodule path 'pixman'

$ git submodule update --init
fatal: Needed a single revision
Unable to find current revision in submodule path 'pixman'

The problem is easy to solve: manually remove the pixman directory;
however IMHO git submodule update should not fail this way since it may
confuse the user.

Sounds like a reasonable observation.  Jens, Heiko, comments?

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 04/01/2013 22:51, Junio C Hamano ha scritto:

Manlio Perillo [off-list ref] writes:

quoted

$ git submodule update --init
...
Submodule 'roms/vgabios' (git://git.qemu.org/vgabios.git/) registered
for path 'roms/vgabios'
fatal: unable to connect to anongit.freedesktop.org:
anongit.freedesktop.org[0: 131.252.210.161]: errno=Connection timed out

Unable to fetch in submodule path 'pixman'

$ git submodule update --init
fatal: Needed a single revision
Unable to find current revision in submodule path 'pixman'

The problem is easy to solve: manually remove the pixman directory;
however IMHO git submodule update should not fail this way since it may
confuse the user.

Sounds like a reasonable observation.  Jens, Heiko, comments?

I have found another, related problem.

Today I tried to update qemu submodules again, however the command
failed with an "obscure" error message:

$ git submodule update pixman
fatal: Needed a single revision
Unable to find current revision in submodule path 'pixman'


The pixman submodule is the one that I failed to update in the very begin.
The problem is not with the pixman or qemu repository: if I clone again
qemu (with the --recursive option), all is ok.

The problem is with the private working copy (in .git/modules/pixman)
being corrupted:

$git log
fatal: bad default revision 'HEAD'.

The HEAD file contains "ref: refs/heads/master", but the refs/heads
directory is empty.


By the way: since git submodule is a porcelain command, IMHO it should
not show to the user these low level error message; at least it should
give more details.
As an example, in this case it could say something like:

  the local module "pixmap" seems to be corrupted.
  Run xxx to remove the module and yyy to create it again.

The ideal solution is, for submodule update, to never leave an
incomplete directory; that is: the update command should be atomic.


Regards  Manlio
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAlDoMAEACgkQscQJ24LbaUQVugCggdl36Hx5JIW/hd1SVXWv+ths
zpYAnR+93BfDLaFhXEiaQvu/TickmDA0
=2Mnw
-----END PGP SIGNATURE-----

Am 04.01.2013 22:51, schrieb Junio C Hamano:

Manlio Perillo [off-list ref] writes:

quoted

$ git submodule update --init
...
Submodule 'roms/vgabios' (git://git.qemu.org/vgabios.git/) registered
for path 'roms/vgabios'
fatal: unable to connect to anongit.freedesktop.org:
anongit.freedesktop.org[0: 131.252.210.161]: errno=Connection timed out

Unable to fetch in submodule path 'pixman'

$ git submodule update --init
fatal: Needed a single revision
Unable to find current revision in submodule path 'pixman'

The problem is easy to solve: manually remove the pixman directory;
however IMHO git submodule update should not fail this way since it may
confuse the user.

Sounds like a reasonable observation.  Jens, Heiko, comments?

The reason seems to be that clone leaves a partial initialized .git
directory in case of connection problems. The next time submodule
update runs it tries to revive the submodule from .git/modules/<name>
but fails as there are no objects in it.

This looks like a bug in clone to me, as it takes precautions to clean
up if something goes wrong but doesn't do that in this case. But while
glancing over the code I didn't find out what goes wrong here.

If this isn't seen as a bug in clone, we could also remove the
.git/modules/<name> directory in module_clone() of git-submodule.s
h when the clone fails. Manilo, does the following patch remove the
problems you are seeing (after removing .git/modules/pixman manually)?

diff --git a/git-submodule.sh b/git-submodule.sh
index 2365149..4098702 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -208,7 +208,10 @@ module_clone()
                        git clone $quiet -n ${reference:+"$reference"} \
                                --separate-git-dir "$gitdir" "$url" "$sm_path"
                ) ||
-               die "$(eval_gettext "Clone of '\$url' into submodule path '\$sm_path' failed")"
+               (
+                       rm -rf "$gitdir" &&
+                       die "$(eval_gettext "Clone of '\$url' into submodule path '\$sm_path' failed")"
+               )
        fi

        # We already are at the root of the work tree but cd_to_toplevel will

Am 05.01.2013 14:52, schrieb Manlio Perillo:

Il 04/01/2013 22:51, Junio C Hamano ha scritto:

quoted

Manlio Perillo [off-list ref] writes:

quoted

quoted

$ git submodule update --init
...
Submodule 'roms/vgabios' (git://git.qemu.org/vgabios.git/) registered
for path 'roms/vgabios'
fatal: unable to connect to anongit.freedesktop.org:
anongit.freedesktop.org[0: 131.252.210.161]: errno=Connection timed out

Unable to fetch in submodule path 'pixman'

$ git submodule update --init
fatal: Needed a single revision
Unable to find current revision in submodule path 'pixman'

The problem is easy to solve: manually remove the pixman directory;
however IMHO git submodule update should not fail this way since it may
confuse the user.

quoted

Sounds like a reasonable observation.  Jens, Heiko, comments?

I have found another, related problem.

Today I tried to update qemu submodules again, however the command
failed with an "obscure" error message:

$ git submodule update pixman
fatal: Needed a single revision
Unable to find current revision in submodule path 'pixman'


The pixman submodule is the one that I failed to update in the very begin.
The problem is not with the pixman or qemu repository: if I clone again
qemu (with the --recursive option), all is ok.

The problem is with the private working copy (in .git/modules/pixman)
being corrupted:

$git log
fatal: bad default revision 'HEAD'.

The HEAD file contains "ref: refs/heads/master", but the refs/heads
directory is empty.

Yep, as I explained in my other email the partially set up
.git/modules/pixman is the reason for the trouble you have.

By the way: since git submodule is a porcelain command, IMHO it should
not show to the user these low level error message; at least it should
give more details.
As an example, in this case it could say something like:

  the local module "pixmap" seems to be corrupted.
  Run xxx to remove the module and yyy to create it again.

The ideal solution is, for submodule update, to never leave an
incomplete directory; that is: the update command should be atomic.

I agree that submodule update should not leave an inconsistent state.
In that case you wouldn't see any low level error messages (which I
think is ok if something the porcelain didn't expect to happen occurs,
like it did here).

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 05/01/2013 15:01, Jens Lehmann ha scritto:

[...]

quoted

quoted

$ git submodule update --init
fatal: Needed a single revision
Unable to find current revision in submodule path 'pixman'

The problem is easy to solve: manually remove the pixman directory;
however IMHO git submodule update should not fail this way since it may
confuse the user.

Sounds like a reasonable observation.  Jens, Heiko, comments?

The reason seems to be that clone leaves a partial initialized .git
directory in case of connection problems. The next time submodule
update runs it tries to revive the submodule from .git/modules/<name>
but fails as there are no objects in it.

[...]

If this isn't seen as a bug in clone, we could also remove the
.git/modules/<name> directory in module_clone() of git-submodule.s
h when the clone fails. Manilo,

Its Manlio, not Manilo ;-).

does the following patch remove the
problems you are seeing (after removing .git/modules/pixman manually)?

I don't think I can test it right now, since the problem can only be
reproduced when git clone fails due to network problems.

Without the patch, if I remove the .git/modules/pixman directory,
`git submodule update --init pixamp` fails:

  Unable to find current revision in submodule path 'pixman'
  fatal: Not a git repository: pixman/../.git/modules/pixman


To reproduce the problem, however, it seems all you need to do is to
send SIGINT signal during `git submodule update` :

  $ git submodule update --init pixman
  Cloning into 'pixman'...
  remote: Counting objects: 10137, done.
  ^C

  $ git submodule update pixman
  remote: Counting objects: 10137, done.
  ^C

  $ git submodule update pixman
  fatal: Needed a single revision
  Unable to find current revision in submodule path 'pixman'


Note that I had to send SIGINT two times, in order to corrupt the module.

I suspect your patch does not fix this (since I don't get the "Clone
failed" error message).


I also noted that If I send SIGINT before git starts counting remote
objects, I get a different count number:


  $ git submodule update pixman
  Cloning into 'pixman'...
  ^C

  $ git submodule update pixman
  remote: Counting objects: 9757, done.
  ^C

  $ git submodule update pixman
  fatal: Needed a single revision
  Unable to find current revision in submodule path 'pixman'


Note that git is reporting 9757 remote objects, instead of 10137.


P.S.:
sorry for the mail I sent today.
It reported the exact same problem I reported yesterday: this morning I
was rather sure that I got a different error message from submodule
update...



Regards   Manlio
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAlDoPZMACgkQscQJ24LbaUTfNQCdFvhSQwGlJZlvOr+TIHHyDFJY
d8AAn0zuHKjBGIcqr8RH/rftHjomvPtM
=48RN
-----END PGP SIGNATURE-----

Am 05.01.2013 15:01, schrieb Jens Lehmann:

Am 04.01.2013 22:51, schrieb Junio C Hamano:

quoted

Manlio Perillo [off-list ref] writes:

quoted

$ git submodule update --init
...
Submodule 'roms/vgabios' (git://git.qemu.org/vgabios.git/) registered
for path 'roms/vgabios'
fatal: unable to connect to anongit.freedesktop.org:
anongit.freedesktop.org[0: 131.252.210.161]: errno=Connection timed out

Unable to fetch in submodule path 'pixman'

$ git submodule update --init
fatal: Needed a single revision
Unable to find current revision in submodule path 'pixman'

The problem is easy to solve: manually remove the pixman directory;
however IMHO git submodule update should not fail this way since it may
confuse the user.

Sounds like a reasonable observation.  Jens, Heiko, comments?

The reason seems to be that clone leaves a partial initialized .git
directory in case of connection problems. The next time submodule
update runs it tries to revive the submodule from .git/modules/<name>
but fails as there are no objects in it.

This looks like a bug in clone to me, as it takes precautions to clean
up if something goes wrong but doesn't do that in this case. But while
glancing over the code I didn't find out what goes wrong here.

I dug a bit deeper here and found the reason: In remove_junk() of
builtin/clone.c the junk_git_dir points to the gitfile in the
submodules work tree, not the .git/modules/<name> directory where
clone was told to put the git directory. Will see if I can come up
with a patch soonish ...

Since b57fb80a7d (init, clone: support --separate-git-dir for .git file)
git clone supports the --separate-git-dir option to create the git dir
outside the work tree. But when that option is used, the git dir won't be
deleted in case the clone fails like it would be without this option. This
makes clone lose its atomicity as in case of a failure a partly set up git
dir is left behind. A real world example where this leads to problems is
when "git submodule update" fails to clone a submodule and later calls to
"git submodule update" stumble over the partially set up git dir and try
to revive the submodule from there, which then fails with a not very user
friendly error message.

Fix that by updating the junk_git_dir variable (used to remember if and
what git dir should be removed in case of failure) to the new value given
with the --seperate-git-dir option. Also add a test for this to t5600 (and
while at it fix the former last test to not cd into a directory to test
for its existence but use "test -d" instead).

Reported-by: Manlio Perillo <redacted>
Signed-off-by: Jens Lehmann <redacted>
---


Am 05.01.2013 15:50, schrieb Jens Lehmann:

Am 05.01.2013 15:01, schrieb Jens Lehmann:

quoted

The reason seems to be that clone leaves a partial initialized .git
directory in case of connection problems. The next time submodule
update runs it tries to revive the submodule from .git/modules/<name>
but fails as there are no objects in it.

This looks like a bug in clone to me, as it takes precautions to clean
up if something goes wrong but doesn't do that in this case. But while
glancing over the code I didn't find out what goes wrong here.

I dug a bit deeper here and found the reason: In remove_junk() of
builtin/clone.c the junk_git_dir points to the gitfile in the
submodules work tree, not the .git/modules/<name> directory where
clone was told to put the git directory. Will see if I can come up
with a patch soonish ...

And this fixes it for me. Manlio, it'd be great if you could test
this patch (but please not only remove .git/modules/<name> but also
the submodule work tree before doing that).


 builtin/clone.c               |  4 +++-
 t/t5600-clone-fail-cleanup.sh | 12 +++++++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index ec2f75b..8d23a62 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c

@@ -771,8 +771,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		die(_("could not create leading directories of '%s'"), git_dir);

 	set_git_dir_init(git_dir, real_git_dir, 0);
-	if (real_git_dir)
+	if (real_git_dir) {
 		git_dir = real_git_dir;
+		junk_git_dir = real_git_dir;
+	}

 	if (0 <= option_verbosity) {
 		if (option_bare)

diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh
index ee06d28..4435693 100755
--- a/t/t5600-clone-fail-cleanup.sh
+++ b/t/t5600-clone-fail-cleanup.sh

@@ -37,6 +37,16 @@ test_expect_success \

 test_expect_success \
     'successful clone must leave the directory' \
-    'cd bar'
+    'test -d bar'
+
+test_expect_success 'failed clone --separate-git-dir should not leave any directories' '
+	mkdir foo/.git/objects.bak/ &&
+	mv foo/.git/objects/* foo/.git/objects.bak/ &&
+	test_must_fail git clone --separate-git-dir gitdir foo worktree &&
+	test_must_fail test -e gitdir &&
+	test_must_fail test -e worktree &&
+	mv foo/.git/objects.bak/* foo/.git/objects/ &&
+	rmdir foo/.git/objects.bak
+'

 test_done

-- 
1.8.1.81.gb1e9864

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 05/01/2013 21:17, Jens Lehmann ha scritto:

Since b57fb80a7d (init, clone: support --separate-git-dir for .git file)
git clone supports the --separate-git-dir option to create the git dir
outside the work tree. But when that option is used, the git dir won't be
deleted in case the clone fails like it would be without this option. This
makes clone lose its atomicity as in case of a failure a partly set up git
dir is left behind. A real world example where this leads to problems is
when "git submodule update" fails to clone a submodule and later calls to
"git submodule update" stumble over the partially set up git dir and try
to revive the submodule from there, which then fails with a not very user
friendly error message.

Fix that by updating the junk_git_dir variable (used to remember if and
what git dir should be removed in case of failure) to the new value given
with the --seperate-git-dir option. Also add a test for this to t5600 (and
while at it fix the former last test to not cd into a directory to test
for its existence but use "test -d" instead).

Reported-by: Manlio Perillo <redacted>
Signed-off-by: Jens Lehmann <redacted>
---
[...]
And this fixes it for me. Manlio, it'd be great if you could test
this patch (but please not only remove .git/modules/<name> but also
the submodule work tree before doing that).

I can confirm that the patch solves the problem I reported.


Thanks   Manlio
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAlDomR4ACgkQscQJ24LbaUQszACfV42L9Xcy+mme6RY/vY+K2H4T
QDAAoIIupUSjwv6qUgzUMQV1aNplrWJD
=uN3W
-----END PGP SIGNATURE-----