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

> Manlio Perillo <manlio.perillo@gmail.com> 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?

> Il 04/01/2013 22:51, Junio C Hamano ha scritto:
>> Manlio Perillo <manlio.perillo@gmail.com> 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?
> 
> 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.

> Manlio Perillo <manlio.perillo@gmail.com> 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?

> [...]
>>> $ 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,

> Am 04.01.2013 22:51, schrieb Junio C Hamano:
>> Manlio Perillo <manlio.perillo@gmail.com> 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?
> 
> 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:
>> 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 <manlio.perillo@gmail.com>
> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
> ---
> [...]
> 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 <manlio.perillo@gmail.com>
> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
> ---

> --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 <pclouds@gmail.com>

> Nguyễn Thái Ngọc Duy wrote:
>
>> --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 <pclouds@gmail.com>
>
> 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?

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

> On Mon, Jan 7, 2013 at 6:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> 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.
>
> 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?

> On Sun, Jan 06, 2013 at 02:19:48AM -0800, Jonathan Nieder wrote:
>> 	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?
> 
> 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< --
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 8d23a62..c8b09ad 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -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;
>  	}
> diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh
> index 4435693..231bc8a 100755
> --- a/t/t5600-clone-fail-cleanup.sh
> +++ b/t/t5600-clone-fail-cleanup.sh
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

> Am 08.01.2013 15:16, schrieb Duy Nguyen:
>> On Sun, Jan 06, 2013 at 02:19:48AM -0800, Jonathan Nieder wrote:
>>> 	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?
>> 
>> 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).

> Jens Lehmann <Jens.Lehmann@web.de> writes:
>
>> Am 08.01.2013 15:16, schrieb Duy Nguyen:
>>> On Sun, Jan 06, 2013 at 02:19:48AM -0800, Jonathan Nieder wrote:
>>>>     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?
>>>
>>> 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.

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