git submodule update is not fail safe

$ 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 13 lines · Manlio Perillo

$ 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'

Quoted 3 lines

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?

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

Quoted 15 lines

Quoted 13 lines · Manlio Perillo

$ 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'

Quoted 5 lines

Quoted 3 lines

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.

Quoted 13 lines · Manlio Perillo

$ 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'

Quoted 3 lines

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?

[...]

Quoted 9 lines

Quoted 7 lines

$ 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,

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

Am 04.01.2013 22:51, schrieb Junio C Hamano:

Quoted 15 lines

Quoted 13 lines · Manlio Perillo

$ 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'

Quoted 5 lines

Quoted 3 lines

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.

Am 05.01.2013 15:01, schrieb Jens Lehmann:

Quoted 8 lines

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 <[email protected]>

Signed-off-by Jens Lehmann <[email protected]>

---
[...]
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).

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 <[email protected]>

Signed-off-by Jens Lehmann <[email protected]>

---

How is the file that points at the real git dir removed with this fix, by the way?

And
because junk_work_tree is not set up for --bare, I guess we still need to fix "--bare --separate-git-dir" case, or forbid it because i'm not sure if that case makes sense at all.

Quoted 4 lines · Duy Nguyen

And
because junk_work_tree is not set up for --bare, I guess we still need to fix "--bare --separate-git-dir" case, or forbid it because i'm not sure if that case makes sense at all.

Forbidding it makes sense to me. If some day we want a git command to create a .git file, I don't think it should be spelled as "git clone --bare --separate-git-dir <repo>".

In the meantime,

        printf '%s\n' 'gitdir: /path/to/repo' >repo.git

works fine.

--separate-git-dir was added to clone with the repository away from standard position <worktree>/.git. It does not make sense to use it without creating working directory.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

Quoted 5 lines · Nguyễn Thái Ngọc Duy

--separate-git-dir was added to clone with the repository away from standard position <worktree>/.git. It does not make sense to use it without creating working directory.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

The patch correctly implements the above. The description leaves out detail. I'd say something like

The --separate-git-dir option was introduced to make it simple to put the git directory somewhere outside the worktree, for example when cloning a repository for use as a submodule.

It was not intended for use when creating a bare repository. In that case there is no worktree and it is more natural to directly clone the repository and create a .git file as separate steps:

git clone --bare /path/to/repo.git bar.git printf 'gitdir: bar.git\n' >foo.git

Unfortunately we forgot to forbid the --bare --separate-git-dir combination. In practice, we know no one could be using --bare with --separate-git-dir because it is broken in the following way: <explanation here>. So it is safe to make good on our mistake and forbid the combination, making the command easier to explain.

I don't know what would go in the <explanation here> blank above, though. Is it possible that some people are relying on this option combination?

Quoted 3 lines

I don't know what would go in the <explanation here> blank above, though. Is it possible that some people are relying on this option combination?

I do not necessarily think we must say "it happens not to work already for such and such reasons, lucky us!", but it is indeed a good idea to think things through, justifying why this cannot be a regression, and record the fact that we did that thinking, in the log message.

Thanks.

Quoted 10 lines · Junio C Hamano

Quoted 3 lines

I don't know what would go in the <explanation here> blank above, though. Is it possible that some people are relying on this option combination?

I do not necessarily think we must say "it happens not to work already for such and such reasons, lucky us!", but it is indeed a good idea to think things through, justifying why this cannot be a regression, and record the fact that we did that thinking, in the log message.

Quoted 1 line

Thanks.

I wanted to give a day or two or think about the <explanation here>. Does "Thanks." mean you have picked up the patch and adjusted the commit message appropriately, or should I go with my original plan and resend it later with "explanantion there"?

Unfortunately we forgot to forbid the --bare --separate-git-dir combination. In practice, we know no one could be using --bare with --separate-git-dir because it is broken in the following way: <explanation here>. So it is safe to make good on our mistake and forbid the combination, making the command easier to explain.

I don't know what would go in the <explanation here> blank above, though. Is it possible that some people are relying on this option combination?

Quoted 7 lines

Unfortunately we forgot to forbid the --bare --separate-git-dir combination. In practice, we know no one could be using --bare with --separate-git-dir because it is broken in the following way: <explanation here>. So it is safe to make good on our mistake and forbid the combination, making the command easier to explain.

Quoted 3 lines

I don't know what would go in the <explanation here> blank above, though. Is it possible that some people are relying on this option combination?

I can't say it's broken in what way. Maybe it's easier to just support this case, it's not much work anyway. Jens, maybe squash this to your original patch?

-- 8< --

builtin/clone.c +5 -0

index 8d23a62..c8b09ad 100644
@@ -375,6 +375,7 @@ static void clone_local(const char *src_repo, const char *dest_repo)
 
 static const char *junk_work_tree;
 static const char *junk_git_dir;
+static const char *junk_git_file;
 static pid_t junk_pid;
 
 static void remove_junk(void)

@@ -392,6 +393,8 @@ static void remove_junk(void)
 		remove_dir_recursively(&sb, 0);
 		strbuf_reset(&sb);
 	}
+	if (junk_git_file)
+		unlink(junk_git_file);
 }
 
 static void remove_junk_on_signal(int signo)

@@ -772,6 +775,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 	set_git_dir_init(git_dir, real_git_dir, 0);
 	if (real_git_dir) {
+		if (option_bare)
+			junk_git_file = git_dir;
 		git_dir = real_git_dir;
 		junk_git_dir = real_git_dir;
 	}

t/t5600-clone-fail-cleanup.sh +10 -2

index 4435693..231bc8a 100755
@@ -49,4 +49,14 @@ test_expect_success 'failed clone --separate-git-dir should not leave any direct
 	rmdir foo/.git/objects.bak
 '
 
+test_expect_success 'failed clone --separate-git-dir --bare should not leave any directories' '
+	mkdir foo/.git/objects.bak/ &&
+	mv foo/.git/objects/* foo/.git/objects.bak/ &&
+	test_must_fail git clone --bare --separate-git-dir gitdir foo baaar &&
+	test_must_fail test -e gitdir &&
+	test_must_fail test -e baaar &&
+	mv foo/.git/objects.bak/* foo/.git/objects/ &&
+	rmdir foo/.git/objects.bak
+'
+
 test_done
-- 8< --
--

To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Am 08.01.2013 15:16, schrieb Duy Nguyen:

Quoted 12 lines · Jonathan Nieder

Quoted 10 lines

Unfortunately we forgot to forbid the --bare --separate-git-dir combination. In practice, we know no one could be using --bare with --separate-git-dir because it is broken in the following way: <explanation here>. So it is safe to make good on our mistake and forbid the combination, making the command easier to explain.

I don't know what would go in the <explanation here> blank above, though. Is it possible that some people are relying on this option combination?

Quoted 3 lines

I can't say it's broken in what way. Maybe it's easier to just support this case, it's not much work anyway. Jens, maybe squash this to your original patch?

I'd be fine with that, though my gut feeling is that this should be a patch of its own (My patch handles the git dir, your's handles a work tree issue).

Quoted 13 lines · Jens Lehmann

Am 08.01.2013 15:16, schrieb Duy Nguyen:

Quoted 12 lines · Jonathan Nieder

Quoted 10 lines

Unfortunately we forgot to forbid the --bare --separate-git-dir combination. In practice, we know no one could be using --bare with --separate-git-dir because it is broken in the following way: <explanation here>. So it is safe to make good on our mistake and forbid the combination, making the command easier to explain.

I don't know what would go in the <explanation here> blank above, though. Is it possible that some people are relying on this option combination?

Quoted 7 lines

Quoted 3 lines

I can't say it's broken in what way. Maybe it's easier to just support this case, it's not much work anyway. Jens, maybe squash this to your original patch?

I'd be fine with that, though my gut feeling is that this should be a patch of its own (My patch handles the git dir, your's handles a work tree issue).

I agree that these are totally unrelated issues.

After all, Jonathan's suggestion to forbid it was because the combination does not make sense and does not have practical uses, and forbidding it would make the command easier to explain than leaving it accepted from the command line. If you choose to go in the opposite direction and make "clone --bare --separate-git-dir" do something useful, it should be explained very well in the documentation part of the patch why such a combination is a good idea, and in what situation the behaviour is useful and the user may want to consider using it, I think.

Quoted 9 lines

After all, Jonathan's suggestion to forbid it was because the combination does not make sense and does not have practical uses, and forbidding it would make the command easier to explain than leaving it accepted from the command line. If you choose to go in the opposite direction and make "clone --bare --separate-git-dir" do something useful, it should be explained very well in the documentation part of the patch why such a combination is a good idea, and in what situation the behaviour is useful and the user may want to consider using it, I think.

It is more like postponing the usefulness evaluation of the combination until later (maybe someone will come up with an actual use case). As of now, --separate-git-dir --bare is a valid combination. Jens' patch fixes one case but leave the other case broken, which is why I think it should be in one patch. It's rather ducking head in the sand than actually declaring that the combination is useful.