[BUG] git submodule update is not fail safe

[BUG] git submodule update is not fail safe

[Manlio Perillo]

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

Hi.

My network connection at times is rather unstable, so I can experience
all sort of network problems.

Today I tried to clone the qemu repository, and then to update all
submodules.

I'm using git from a recent master (790c83 - 14 December).

This is what happened:

$ git submodule update --init pixman
Submodule 'pixman' (git://anongit.freedesktop.org/pixman) registered for
path 'pixman'
Cloning into 'pixman'...
fatal: Unable to look up anongit.freedesktop.org (port 9418) (Name or
service not known)
Clone of 'git://anongit.freedesktop.org/pixman' into submodule path
'pixman' failed


$ git submodule update --init
Submodule 'roms/SLOF' (git://git.qemu.org/SLOF.git) registered for path
'roms/SLOF'
Submodule 'roms/ipxe' (git://git.qemu.org/ipxe.git) registered for path
'roms/ipxe'
Submodule 'roms/openbios' (git://git.qemu.org/openbios.git) registered
for path 'roms/openbios'
Submodule 'roms/qemu-palcode' (git://repo.or.cz/qemu-palcode.git)
registered for path 'roms/qemu-palcode'
Submodule 'roms/seabios' (git://git.qemu.org/seabios.git/) registered
for path 'roms/seabios'
Submodule 'roms/sgabios' (git://git.qemu.org/sgabios.git) registered for
path 'roms/sgabios'
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.

I'm sorry for not sending a patch.



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

iEYEARECAAYFAlDnQUUACgkQscQJ24LbaUSgVACglJjFxB51VINOCe9Z39/XEEUH
6+QAnAwdQerBSjVSS1/3eNXSBfnR0yba
=eOJT
-----END PGP SIGNATURE-----

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

[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?

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

[Manlio Perillo]

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

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.


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

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

[Jens Lehmann]

Am 05.01.2013 14:52, schrieb Manlio Perillo:
> 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.

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

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

[Jens Lehmann]

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.

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

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

[Manlio Perillo]

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

Il 05/01/2013 15:01, Jens Lehmann ha scritto:
> [...]
>>> $ 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-----

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

[Jens Lehmann]

Am 05.01.2013 15:01, schrieb Jens Lehmann:
> 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.

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

[PATCH] clone: support atomic operation with --separate-git-dir

[Jens Lehmann]

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


Am 05.01.2013 15:50, schrieb Jens Lehmann:
> 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 ...

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

Re: [PATCH] clone: support atomic operation with --separate-git-dir

[Manlio Perillo]

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

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

Re: [PATCH] clone: support atomic operation with --separate-git-dir

[Junio C Hamano]

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

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

I hate to see that git_link is not an argument to init_db() but is a
file-scope static in init-db.c to be used to communicate between
set_git_dir_init() and init_db(), but that would be a separate thing
to be cleaned up, I guess.

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

Thanks.

Re: [PATCH] clone: support atomic operation with --separate-git-dir

[Duy Nguyen]

On Sun, Jan 6, 2013 at 1:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> How is the file that points at the real git dir removed with this
> fix, by the way?

It's part of the worktree cleanup, pointed by junk_work_tree. 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.
-- 
Duy

Re: [PATCH] clone: support atomic operation with --separate-git-dir

[Jonathan Nieder]

Duy Nguyen wrote:

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

[PATCH] clone: forbid --bare --separate-git-dir <dir>

[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 <pclouds@gmail.com>
---
 On Sun, Jan 6, 2013 at 4:16 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
 > Duy Nguyen wrote:
 >
 >>                                                               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>".

 That command does not work because --separate-git-dir requires an
 argument in addition to <repo>, which is taken as the target repo.
 Still it's hard to find a use case for

 git clone --bare --separate-git-dir <real-repo> <symlink-repo>
 
 > In the meantime,
 >
 >         printf '%s\n' 'gitdir: /path/to/repo' >repo.git
 >
 > works fine.

 Yeah. A separate clone operation following by that printf should be
 clearer.


 builtin/clone.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index ec2f75b..360e430 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -704,6 +704,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		if (option_origin)
 			die(_("--bare and --origin %s options are incompatible."),
 			    option_origin);
+		if (real_git_dir)
+			/*
+			 * If you lift this restriction, remember to
+			 * clean .git in this case in remove_junk_on_signal
+			 */
+			die(_("--bare and --separate-git-dir are incompatible."));
 		option_no_checkout = 1;
 	}
 
-- 
1.8.0.rc2.23.g1fb49df

Re: [PATCH] clone: forbid --bare --separate-git-dir <dir>

[Jonathan Nieder]

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?

Re: [PATCH] clone: forbid --bare --separate-git-dir <dir>

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

Re: [PATCH] clone: forbid --bare --separate-git-dir <dir>

[Duy Nguyen]

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"?
-- 
Duy

Re: [PATCH] clone: forbid --bare --separate-git-dir <dir>

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> 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"?

The latter, "Thanks for a review".  I may have picked it up in 'pu',
but that merely means, as usual, that I wanted to add a reminder in
What's cooking to expect a reroll, and nothing more.

Thanks.

Re: [PATCH] clone: forbid --bare --separate-git-dir <dir>

[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?

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

Re: [PATCH] clone: forbid --bare --separate-git-dir <dir>

[Jens Lehmann]

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). But I don't care much either way, what do others
think?

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

Re: [PATCH] clone: forbid --bare --separate-git-dir <dir>

[Junio C Hamano]

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.

Re: [PATCH] clone: forbid --bare --separate-git-dir <dir>

[Duy Nguyen]

On Wed, Jan 9, 2013 at 12:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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

Re: [PATCH] clone: forbid --bare --separate-git-dir <dir>

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

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

When a user comes and asks how "git clone --bare --separate-git-dir"
is meant to be used, you are saying that your answer will be "Eh, it
does something random that I cannot explain, and I cannot even
suggest a good use case for it, but somebody may find it useful."?

If we get rid of it, we do not have to explain what such a useless
combination would/should do, no?

[PATCH v2] clone: forbid --bare --separate-git-dir <dir>

[Nguyễn Thái Ngọc Duy]

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

Forbid the combination, making the command easier to explain.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Just reword the commit message (or copying it from Jonathan
 actually). No comments about remove_junk_on_signal because we're less
 likely to re-enable it again.

 builtin/clone.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index ec2f75b..b30189f 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -704,6 +704,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		if (option_origin)
 			die(_("--bare and --origin %s options are incompatible."),
 			    option_origin);
+		if (real_git_dir)
+			die(_("--bare and --separate-git-dir are incompatible."));
 		option_no_checkout = 1;
 	}
 
-- 
1.8.0.rc2.23.g1fb49df

Re: [PATCH v2] clone: forbid --bare --separate-git-dir <dir>

[Junio C Hamano]

Thanks.