{"thread":{"id":"32526","subject":"[BUG] git submodule update is not fail safe","startedAt":"2013-01-04T20:53:25Z","lastAt":"2013-01-11T03:15:58Z","messageCount":24,"participants":["Manlio Perillo","Junio C Hamano","Jens Lehmann","Duy Nguyen","Jonathan Nieder","Nguyễn Thái Ngọc Duy"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"205976","messageId":"50E74145.4020701@gmail.com","threadId":"32526","inReplyTo":null,"subject":"[BUG] git submodule update is not fail safe","fromName":"Manlio Perillo","fromEmail":"manlio.perillo@gmail.com","sentAt":"2013-01-04T20:53:25Z","receivedAt":"2013-01-04T20:53:25Z","isPatch":false,"sender":{"key":"manlio.perillo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6217088?v=4"},"body":"-----BEGIN PGP SIGNED MESSAGE-----\nHash: SHA1\n\nHi.\n\nMy network connection at times is rather unstable, so I can experience\nall sort of network problems.\n\nToday I tried to clone the qemu repository, and then to update all\nsubmodules.\n\nI'm using git from a recent master (790c83 - 14 December).\n\nThis is what happened:\n\n$ git submodule update --init pixman\nSubmodule 'pixman' (git://anongit.freedesktop.org/pixman) registered for\npath 'pixman'\nCloning into 'pixman'...\nfatal: Unable to look up anongit.freedesktop.org (port 9418) (Name or\nservice not known)\nClone of 'git://anongit.freedesktop.org/pixman' into submodule path\n'pixman' failed\n\n\n$ git submodule update --init\nSubmodule 'roms/SLOF' (git://git.qemu.org/SLOF.git) registered for path\n'roms/SLOF'\nSubmodule 'roms/ipxe' (git://git.qemu.org/ipxe.git) registered for path\n'roms/ipxe'\nSubmodule 'roms/openbios' (git://git.qemu.org/openbios.git) registered\nfor path 'roms/openbios'\nSubmodule 'roms/qemu-palcode' (git://repo.or.cz/qemu-palcode.git)\nregistered for path 'roms/qemu-palcode'\nSubmodule 'roms/seabios' (git://git.qemu.org/seabios.git/) registered\nfor path 'roms/seabios'\nSubmodule 'roms/sgabios' (git://git.qemu.org/sgabios.git) registered for\npath 'roms/sgabios'\nSubmodule 'roms/vgabios' (git://git.qemu.org/vgabios.git/) registered\nfor path 'roms/vgabios'\nfatal: unable to connect to anongit.freedesktop.org:\nanongit.freedesktop.org[0: 131.252.210.161]: errno=Connection timed out\n\nUnable to fetch in submodule path 'pixman'\n\n\n$ git submodule update --init\nfatal: Needed a single revision\nUnable to find current revision in submodule path 'pixman'\n\n\nThe problem is easy to solve: manually remove the pixman directory;\nhowever IMHO git submodule update should not fail this way since it may\nconfuse the user.\n\nI'm sorry for not sending a patch.\n\n\n\nRegards Manlio\n-----BEGIN PGP SIGNATURE-----\nVersion: GnuPG v1.4.10 (GNU/Linux)\nComment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/\n\niEYEARECAAYFAlDnQUUACgkQscQJ24LbaUSgVACglJjFxB51VINOCe9Z39/XEEUH\n6+QAnAwdQerBSjVSS1/3eNXSBfnR0yba\n=eOJT\n-----END PGP SIGNATURE-----\n"},{"id":"205984","messageId":"7vzk0osjli.fsf@alter.siamese.dyndns.org","threadId":"32526","inReplyTo":"50E74145.4020701@gmail.com","subject":"Re: [BUG] git submodule update is not fail safe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-04T21:51:05Z","receivedAt":"2013-01-04T21:51:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Manlio Perillo writes:\n\n> $ git submodule update --init\n> ...\n> Submodule 'roms/vgabios' (git://git.qemu.org/vgabios.git/) registered\n> for path 'roms/vgabios'\n> fatal: unable to connect to anongit.freedesktop.org:\n> anongit.freedesktop.org[0: 131.252.210.161]: errno=Connection timed out\n>\n> Unable to fetch in submodule path 'pixman'\n>\n> $ git submodule update --init\n> fatal: Needed a single revision\n> Unable to find current revision in submodule path 'pixman'\n>\n> The problem is easy to solve: manually remove the pixman directory;\n> however IMHO git submodule update should not fail this way since it may\n> confuse the user.\n\nSounds like a reasonable observation. Jens, Heiko, comments?\n"},{"id":"206022","messageId":"50E83001.9000505@gmail.com","threadId":"32526","inReplyTo":"7vzk0osjli.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git submodule update is not fail safe","fromName":"Manlio Perillo","fromEmail":"manlio.perillo@gmail.com","sentAt":"2013-01-05T13:52:01Z","receivedAt":"2013-01-05T13:52:01Z","isPatch":false,"sender":{"key":"manlio.perillo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6217088?v=4"},"body":"-----BEGIN PGP SIGNED MESSAGE-----\nHash: SHA1\n\nIl 04/01/2013 22:51, Junio C Hamano ha scritto:\n> Manlio Perillo writes:\n> \n>> $ git submodule update --init\n>> ...\n>> Submodule 'roms/vgabios' (git://git.qemu.org/vgabios.git/) registered\n>> for path 'roms/vgabios'\n>> fatal: unable to connect to anongit.freedesktop.org:\n>> anongit.freedesktop.org[0: 131.252.210.161]: errno=Connection timed out\n>>\n>> Unable to fetch in submodule path 'pixman'\n>>\n>> $ git submodule update --init\n>> fatal: Needed a single revision\n>> Unable to find current revision in submodule path 'pixman'\n>>\n>> The problem is easy to solve: manually remove the pixman directory;\n>> however IMHO git submodule update should not fail this way since it may\n>> confuse the user.\n> \n> Sounds like a reasonable observation. Jens, Heiko, comments?\n\nI have found another, related problem.\n\nToday I tried to update qemu submodules again, however the command\nfailed with an \"obscure\" error message:\n\n$ git submodule update pixman\nfatal: Needed a single revision\nUnable to find current revision in submodule path 'pixman'\n\n\nThe pixman submodule is the one that I failed to update in the very begin.\nThe problem is not with the pixman or qemu repository: if I clone again\nqemu (with the --recursive option), all is ok.\n\nThe problem is with the private working copy (in .git/modules/pixman)\nbeing corrupted:\n\n$git log\nfatal: bad default revision 'HEAD'.\n\nThe HEAD file contains \"ref: refs/heads/master\", but the refs/heads\ndirectory is empty.\n\n\nBy the way: since git submodule is a porcelain command, IMHO it should\nnot show to the user these low level error message; at least it should\ngive more details.\nAs an example, in this case it could say something like:\n\n the local module \"pixmap\" seems to be corrupted.\n Run xxx to remove the module and yyy to create it again.\n\nThe ideal solution is, for submodule update, to never leave an\nincomplete directory; that is: the update command should be atomic.\n\n\nRegards Manlio\n-----BEGIN PGP SIGNATURE-----\nVersion: GnuPG v1.4.10 (GNU/Linux)\nComment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/\n\niEYEARECAAYFAlDoMAEACgkQscQJ24LbaUQVugCggdl36Hx5JIW/hd1SVXWv+ths\nzpYAnR+93BfDLaFhXEiaQvu/TickmDA0\n=2Mnw\n-----END PGP SIGNATURE-----\n"},{"id":"206023","messageId":"50E83224.2070701@web.de","threadId":"32526","inReplyTo":"7vzk0osjli.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git submodule update is not fail safe","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-01-05T14:01:08Z","receivedAt":"2013-01-05T14:01:08Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 04.01.2013 22:51, schrieb Junio C Hamano:\n> Manlio Perillo writes:\n> \n>> $ git submodule update --init\n>> ...\n>> Submodule 'roms/vgabios' (git://git.qemu.org/vgabios.git/) registered\n>> for path 'roms/vgabios'\n>> fatal: unable to connect to anongit.freedesktop.org:\n>> anongit.freedesktop.org[0: 131.252.210.161]: errno=Connection timed out\n>>\n>> Unable to fetch in submodule path 'pixman'\n>>\n>> $ git submodule update --init\n>> fatal: Needed a single revision\n>> Unable to find current revision in submodule path 'pixman'\n>>\n>> The problem is easy to solve: manually remove the pixman directory;\n>> however IMHO git submodule update should not fail this way since it may\n>> confuse the user.\n> \n> Sounds like a reasonable observation. Jens, Heiko, comments?\n\nThe reason seems to be that clone leaves a partial initialized .git\ndirectory in case of connection problems. The next time submodule\nupdate runs it tries to revive the submodule from .git/modules/\nbut fails as there are no objects in it.\n\nThis looks like a bug in clone to me, as it takes precautions to clean\nup if something goes wrong but doesn't do that in this case. But while\nglancing over the code I didn't find out what goes wrong here.\n\nIf this isn't seen as a bug in clone, we could also remove the\n.git/modules/ directory in module_clone() of git-submodule.s\nh when the clone fails. Manilo, does the following patch remove the\nproblems you are seeing (after removing .git/modules/pixman manually)?\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 2365149..4098702 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -208,7 +208,10 @@ module_clone()\n git clone $quiet -n ${reference:+\"$reference\"} \\\n --separate-git-dir \"$gitdir\" \"$url\" \"$sm_path\"\n ) ||\n- die \"$(eval_gettext \"Clone of '\\$url' into submodule path '\\$sm_path' failed\")\"\n+ (\n+ rm -rf \"$gitdir\" &&\n+ die \"$(eval_gettext \"Clone of '\\$url' into submodule path '\\$sm_path' failed\")\"\n+ )\n fi\n\n # We already are at the root of the work tree but cd_to_toplevel will\n"},{"id":"206026","messageId":"50E8338D.4080703@web.de","threadId":"32526","inReplyTo":"50E83001.9000505@gmail.com","subject":"Re: [BUG] git submodule update is not fail safe","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-01-05T14:07:09Z","receivedAt":"2013-01-05T14:07:09Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 05.01.2013 14:52, schrieb Manlio Perillo:\n> Il 04/01/2013 22:51, Junio C Hamano ha scritto:\n>> Manlio Perillo writes:\n> \n>>> $ git submodule update --init\n>>> ...\n>>> Submodule 'roms/vgabios' (git://git.qemu.org/vgabios.git/) registered\n>>> for path 'roms/vgabios'\n>>> fatal: unable to connect to anongit.freedesktop.org:\n>>> anongit.freedesktop.org[0: 131.252.210.161]: errno=Connection timed out\n>>>\n>>> Unable to fetch in submodule path 'pixman'\n>>>\n>>> $ git submodule update --init\n>>> fatal: Needed a single revision\n>>> Unable to find current revision in submodule path 'pixman'\n>>>\n>>> The problem is easy to solve: manually remove the pixman directory;\n>>> however IMHO git submodule update should not fail this way since it may\n>>> confuse the user.\n> \n>> Sounds like a reasonable observation. Jens, Heiko, comments?\n> \n> I have found another, related problem.\n> \n> Today I tried to update qemu submodules again, however the command\n> failed with an \"obscure\" error message:\n> \n> $ git submodule update pixman\n> fatal: Needed a single revision\n> Unable to find current revision in submodule path 'pixman'\n> \n> \n> The pixman submodule is the one that I failed to update in the very begin.\n> The problem is not with the pixman or qemu repository: if I clone again\n> qemu (with the --recursive option), all is ok.\n> \n> The problem is with the private working copy (in .git/modules/pixman)\n> being corrupted:\n> \n> $git log\n> fatal: bad default revision 'HEAD'.\n> \n> The HEAD file contains \"ref: refs/heads/master\", but the refs/heads\n> directory is empty.\n\nYep, as I explained in my other email the partially set up\n.git/modules/pixman is the reason for the trouble you have.\n\n> By the way: since git submodule is a porcelain command, IMHO it should\n> not show to the user these low level error message; at least it should\n> give more details.\n> As an example, in this case it could say something like:\n> \n> the local module \"pixmap\" seems to be corrupted.\n> Run xxx to remove the module and yyy to create it again.\n> \n> The ideal solution is, for submodule update, to never leave an\n> incomplete directory; that is: the update command should be atomic.\n\nI agree that submodule update should not leave an inconsistent state.\nIn that case you wouldn't see any low level error messages (which I\nthink is ok if something the porcelain didn't expect to happen occurs,\nlike it did here).\n"},{"id":"206028","messageId":"50E83D93.80600@gmail.com","threadId":"32526","inReplyTo":"50E83224.2070701@web.de","subject":"Re: [BUG] git submodule update is not fail safe","fromName":"Manlio Perillo","fromEmail":"manlio.perillo@gmail.com","sentAt":"2013-01-05T14:49:55Z","receivedAt":"2013-01-05T14:49:55Z","isPatch":false,"sender":{"key":"manlio.perillo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6217088?v=4"},"body":"-----BEGIN PGP SIGNED MESSAGE-----\nHash: SHA1\n\nIl 05/01/2013 15:01, Jens Lehmann ha scritto:\n> [...]\n>>> $ git submodule update --init\n>>> fatal: Needed a single revision\n>>> Unable to find current revision in submodule path 'pixman'\n>>>\n>>> The problem is easy to solve: manually remove the pixman directory;\n>>> however IMHO git submodule update should not fail this way since it may\n>>> confuse the user.\n>>\n>> Sounds like a reasonable observation. Jens, Heiko, comments?\n> \n> The reason seems to be that clone leaves a partial initialized .git\n> directory in case of connection problems. The next time submodule\n> update runs it tries to revive the submodule from .git/modules/\n> but fails as there are no objects in it.\n> \n> [...]\n>\n> If this isn't seen as a bug in clone, we could also remove the\n> .git/modules/ directory in module_clone() of git-submodule.s\n> h when the clone fails. Manilo,\n\nIts Manlio, not Manilo ;-).\n\n> does the following patch remove the\n> problems you are seeing (after removing .git/modules/pixman manually)?\n> \n\nI don't think I can test it right now, since the problem can only be\nreproduced when git clone fails due to network problems.\n\nWithout the patch, if I remove the .git/modules/pixman directory,\n`git submodule update --init pixamp` fails:\n\n Unable to find current revision in submodule path 'pixman'\n fatal: Not a git repository: pixman/../.git/modules/pixman\n\n\nTo reproduce the problem, however, it seems all you need to do is to\nsend SIGINT signal during `git submodule update` :\n\n $ git submodule update --init pixman\n Cloning into 'pixman'...\n remote: Counting objects: 10137, done.\n ^C\n\n $ git submodule update pixman\n remote: Counting objects: 10137, done.\n ^C\n\n $ git submodule update pixman\n fatal: Needed a single revision\n Unable to find current revision in submodule path 'pixman'\n\n\nNote that I had to send SIGINT two times, in order to corrupt the module.\n\nI suspect your patch does not fix this (since I don't get the \"Clone\nfailed\" error message).\n\n\nI also noted that If I send SIGINT before git starts counting remote\nobjects, I get a different count number:\n\n\n $ git submodule update pixman\n Cloning into 'pixman'...\n ^C\n\n $ git submodule update pixman\n remote: Counting objects: 9757, done.\n ^C\n\n $ git submodule update pixman\n fatal: Needed a single revision\n Unable to find current revision in submodule path 'pixman'\n\n\nNote that git is reporting 9757 remote objects, instead of 10137.\n\n\nP.S.:\nsorry for the mail I sent today.\nIt reported the exact same problem I reported yesterday: this morning I\nwas rather sure that I got a different error message from submodule\nupdate...\n\n\n\nRegards Manlio\n-----BEGIN PGP SIGNATURE-----\nVersion: GnuPG v1.4.10 (GNU/Linux)\nComment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/\n\niEYEARECAAYFAlDoPZMACgkQscQJ24LbaUTfNQCdFvhSQwGlJZlvOr+TIHHyDFJY\nd8AAn0zuHKjBGIcqr8RH/rftHjomvPtM\n=48RN\n-----END PGP SIGNATURE-----\n"},{"id":"206029","messageId":"50E83DAE.1080500@web.de","threadId":"32526","inReplyTo":"50E83224.2070701@web.de","subject":"Re: [BUG] git submodule update is not fail safe","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-01-05T14:50:22Z","receivedAt":"2013-01-05T14:50:22Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 05.01.2013 15:01, schrieb Jens Lehmann:\n> Am 04.01.2013 22:51, schrieb Junio C Hamano:\n>> Manlio Perillo writes:\n>>\n>>> $ git submodule update --init\n>>> ...\n>>> Submodule 'roms/vgabios' (git://git.qemu.org/vgabios.git/) registered\n>>> for path 'roms/vgabios'\n>>> fatal: unable to connect to anongit.freedesktop.org:\n>>> anongit.freedesktop.org[0: 131.252.210.161]: errno=Connection timed out\n>>>\n>>> Unable to fetch in submodule path 'pixman'\n>>>\n>>> $ git submodule update --init\n>>> fatal: Needed a single revision\n>>> Unable to find current revision in submodule path 'pixman'\n>>>\n>>> The problem is easy to solve: manually remove the pixman directory;\n>>> however IMHO git submodule update should not fail this way since it may\n>>> confuse the user.\n>>\n>> Sounds like a reasonable observation. Jens, Heiko, comments?\n> \n> The reason seems to be that clone leaves a partial initialized .git\n> directory in case of connection problems. The next time submodule\n> update runs it tries to revive the submodule from .git/modules/\n> but fails as there are no objects in it.\n> \n> This looks like a bug in clone to me, as it takes precautions to clean\n> up if something goes wrong but doesn't do that in this case. But while\n> glancing over the code I didn't find out what goes wrong here.\n\nI dug a bit deeper here and found the reason: In remove_junk() of\nbuiltin/clone.c the junk_git_dir points to the gitfile in the\nsubmodules work tree, not the .git/modules/ directory where\nclone was told to put the git directory. Will see if I can come up\nwith a patch soonish ...\n"},{"id":"206044","messageId":"50E88A40.9010904@web.de","threadId":"32526","inReplyTo":"50E83DAE.1080500@web.de","subject":"[PATCH] clone: support atomic operation with --separate-git-dir","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-01-05T20:17:04Z","receivedAt":"2013-01-05T20:17:04Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Since b57fb80a7d (init, clone: support --separate-git-dir for .git file)\ngit clone supports the --separate-git-dir option to create the git dir\noutside the work tree. But when that option is used, the git dir won't be\ndeleted in case the clone fails like it would be without this option. This\nmakes clone lose its atomicity as in case of a failure a partly set up git\ndir is left behind. A real world example where this leads to problems is\nwhen \"git submodule update\" fails to clone a submodule and later calls to\n\"git submodule update\" stumble over the partially set up git dir and try\nto revive the submodule from there, which then fails with a not very user\nfriendly error message.\n\nFix that by updating the junk_git_dir variable (used to remember if and\nwhat git dir should be removed in case of failure) to the new value given\nwith the --seperate-git-dir option. Also add a test for this to t5600 (and\nwhile at it fix the former last test to not cd into a directory to test\nfor its existence but use \"test -d\" instead).\n\nReported-by: Manlio Perillo \nSigned-off-by: Jens Lehmann \n---\n\n\nAm 05.01.2013 15:50, schrieb Jens Lehmann:\n> Am 05.01.2013 15:01, schrieb Jens Lehmann:\n>> The reason seems to be that clone leaves a partial initialized .git\n>> directory in case of connection problems. The next time submodule\n>> update runs it tries to revive the submodule from .git/modules/\n>> but fails as there are no objects in it.\n>>\n>> This looks like a bug in clone to me, as it takes precautions to clean\n>> up if something goes wrong but doesn't do that in this case. But while\n>> glancing over the code I didn't find out what goes wrong here.\n> \n> I dug a bit deeper here and found the reason: In remove_junk() of\n> builtin/clone.c the junk_git_dir points to the gitfile in the\n> submodules work tree, not the .git/modules/ directory where\n> clone was told to put the git directory. Will see if I can come up\n> with a patch soonish ...\n\nAnd this fixes it for me. Manlio, it'd be great if you could test\nthis patch (but please not only remove .git/modules/ but also\nthe submodule work tree before doing that).\n\n\n builtin/clone.c | 4 +++-\n t/t5600-clone-fail-cleanup.sh | 12 +++++++++++-\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex ec2f75b..8d23a62 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -771,8 +771,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"could not create leading directories of '%s'\"), git_dir);\n\n \tset_git_dir_init(git_dir, real_git_dir, 0);\n-\tif (real_git_dir)\n+\tif (real_git_dir) {\n \t\tgit_dir = real_git_dir;\n+\t\tjunk_git_dir = real_git_dir;\n+\t}\n\n \tif (0 <= option_verbosity) {\n \t\tif (option_bare)\ndiff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh\nindex ee06d28..4435693 100755\n--- a/t/t5600-clone-fail-cleanup.sh\n+++ b/t/t5600-clone-fail-cleanup.sh\n@@ -37,6 +37,16 @@ test_expect_success \\\n\n test_expect_success \\\n 'successful clone must leave the directory' \\\n- 'cd bar'\n+ 'test -d bar'\n+\n+test_expect_success 'failed clone --separate-git-dir should not leave any directories' '\n+\tmkdir foo/.git/objects.bak/ &&\n+\tmv foo/.git/objects/* foo/.git/objects.bak/ &&\n+\ttest_must_fail git clone --separate-git-dir gitdir foo worktree &&\n+\ttest_must_fail test -e gitdir &&\n+\ttest_must_fail test -e worktree &&\n+\tmv foo/.git/objects.bak/* foo/.git/objects/ &&\n+\trmdir foo/.git/objects.bak\n+'\n\n test_done\n-- \n1.8.1.81.gb1e9864\n"},{"id":"206048","messageId":"50E8991E.4090605@gmail.com","threadId":"32526","inReplyTo":"50E88A40.9010904@web.de","subject":"Re: [PATCH] clone: support atomic operation with --separate-git-dir","fromName":"Manlio Perillo","fromEmail":"manlio.perillo@gmail.com","sentAt":"2013-01-05T21:20:30Z","receivedAt":"2013-01-05T21:20:30Z","isPatch":true,"sender":{"key":"manlio.perillo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6217088?v=4"},"body":"-----BEGIN PGP SIGNED MESSAGE-----\nHash: SHA1\n\nIl 05/01/2013 21:17, Jens Lehmann ha scritto:\n> Since b57fb80a7d (init, clone: support --separate-git-dir for .git file)\n> git clone supports the --separate-git-dir option to create the git dir\n> outside the work tree. But when that option is used, the git dir won't be\n> deleted in case the clone fails like it would be without this option. This\n> makes clone lose its atomicity as in case of a failure a partly set up git\n> dir is left behind. A real world example where this leads to problems is\n> when \"git submodule update\" fails to clone a submodule and later calls to\n> \"git submodule update\" stumble over the partially set up git dir and try\n> to revive the submodule from there, which then fails with a not very user\n> friendly error message.\n> \n> Fix that by updating the junk_git_dir variable (used to remember if and\n> what git dir should be removed in case of failure) to the new value given\n> with the --seperate-git-dir option. Also add a test for this to t5600 (and\n> while at it fix the former last test to not cd into a directory to test\n> for its existence but use \"test -d\" instead).\n> \n> Reported-by: Manlio Perillo \n> Signed-off-by: Jens Lehmann \n> ---\n> [...]\n> And this fixes it for me. Manlio, it'd be great if you could test\n> this patch (but please not only remove .git/modules/ but also\n> the submodule work tree before doing that).\n> \n\nI can confirm that the patch solves the problem I reported.\n\n\nThanks Manlio\n-----BEGIN PGP SIGNATURE-----\nVersion: GnuPG v1.4.10 (GNU/Linux)\nComment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/\n\niEYEARECAAYFAlDomR4ACgkQscQJ24LbaUQszACfV42L9Xcy+mme6RY/vY+K2H4T\nQDAAoIIupUSjwv6qUgzUMQV1aNplrWJD\n=uN3W\n-----END PGP SIGNATURE-----\n"},{"id":"206111","messageId":"7vfw2eq0a0.fsf@alter.siamese.dyndns.org","threadId":"32526","inReplyTo":"50E88A40.9010904@web.de","subject":"Re: [PATCH] clone: support atomic operation with --separate-git-dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-06T06:43:35Z","receivedAt":"2013-01-06T06:43:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann writes:\n\n> Since b57fb80a7d (init, clone: support --separate-git-dir for .git file)\n> git clone supports the --separate-git-dir option to create the git dir\n> outside the work tree. But when that option is used, the git dir won't be\n> deleted in case the clone fails like it would be without this option. This\n> makes clone lose its atomicity as in case of a failure a partly set up git\n> dir is left behind. A real world example where this leads to problems is\n> when \"git submodule update\" fails to clone a submodule and later calls to\n> \"git submodule update\" stumble over the partially set up git dir and try\n> to revive the submodule from there, which then fails with a not very user\n> friendly error message.\n>\n> Fix that by updating the junk_git_dir variable (used to remember if and\n> what git dir should be removed in case of failure) to the new value given\n> with the --seperate-git-dir option. Also add a test for this to t5600 (and\n> while at it fix the former last test to not cd into a directory to test\n> for its existence but use \"test -d\" instead).\n>\n> Reported-by: Manlio Perillo \n> Signed-off-by: Jens Lehmann \n> ---\n\nI hate to see that git_link is not an argument to init_db() but is a\nfile-scope static in init-db.c to be used to communicate between\nset_git_dir_init() and init_db(), but that would be a separate thing\nto be cleaned up, I guess.\n\nHow is the file that points at the real git dir removed with this\nfix, by the way?\n\nThanks.\n"},{"id":"206118","messageId":"CACsJy8Dbe1+e4-XjB+S=kvXyi_Hdt4CQ6K1Z1V-4sqaYekKPWw@mail.gmail.com","threadId":"32526","inReplyTo":"7vfw2eq0a0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] clone: support atomic operation with --separate-git-dir","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-06T08:49:14Z","receivedAt":"2013-01-06T08:49:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Jan 6, 2013 at 1:43 PM, Junio C Hamano wrote:\n> How is the file that points at the real git dir removed with this\n> fix, by the way?\n\nIt's part of the worktree cleanup, pointed by junk_work_tree. And\nbecause junk_work_tree is not set up for --bare, I guess we still need\nto fix \"--bare --separate-git-dir\" case, or forbid it because i'm not\nsure if that case makes sense at all.\n-- \nDuy\n"},{"id":"206120","messageId":"20130106091642.GA10956@elie.Belkin","threadId":"32526","inReplyTo":"CACsJy8Dbe1+e4-XjB+S=kvXyi_Hdt4CQ6K1Z1V-4sqaYekKPWw@mail.gmail.com","subject":"Re: [PATCH] clone: support atomic operation with --separate-git-dir","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-06T09:16:42Z","receivedAt":"2013-01-06T09:16:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Duy Nguyen wrote:\n\n> And\n> because junk_work_tree is not set up for --bare, I guess we still need\n> to fix \"--bare --separate-git-dir\" case, or forbid it because i'm not\n> sure if that case makes sense at all.\n\nForbidding it makes sense to me. If some day we want a git command to\ncreate a .git file, I don't think it should be spelled as \"git clone\n--bare --separate-git-dir \". In the meantime,\n\n\tprintf '%s\\n' 'gitdir: /path/to/repo' >repo.git\n\nworks fine.\n"},{"id":"206123","messageId":"1357465670-32766-1-git-send-email-pclouds@gmail.com","threadId":"32526","inReplyTo":"20130106091642.GA10956@elie.Belkin","subject":"[PATCH] clone: forbid --bare --separate-git-dir ","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-06T09:47:50Z","receivedAt":"2013-01-06T09:47:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"--separate-git-dir was added to clone with the repository away from\nstandard position /.git. It does not make sense to use it\nwithout creating working directory.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n On Sun, Jan 6, 2013 at 4:16 PM, Jonathan Nieder wrote:\n > Duy Nguyen wrote:\n >\n >> And\n >> because junk_work_tree is not set up for --bare, I guess we still need\n >> to fix \"--bare --separate-git-dir\" case, or forbid it because i'm not\n >> sure if that case makes sense at all.\n >\n > Forbidding it makes sense to me. If some day we want a git command to\n > create a .git file, I don't think it should be spelled as \"git clone\n > --bare --separate-git-dir \".\n\n That command does not work because --separate-git-dir requires an\n argument in addition to , which is taken as the target repo.\n Still it's hard to find a use case for\n\n git clone --bare --separate-git-dir \n \n > In the meantime,\n >\n > printf '%s\\n' 'gitdir: /path/to/repo' >repo.git\n >\n > works fine.\n\n Yeah. A separate clone operation following by that printf should be\n clearer.\n\n\n builtin/clone.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex ec2f75b..360e430 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -704,6 +704,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tif (option_origin)\n \t\t\tdie(_(\"--bare and --origin %s options are incompatible.\"),\n \t\t\t option_origin);\n+\t\tif (real_git_dir)\n+\t\t\t/*\n+\t\t\t * If you lift this restriction, remember to\n+\t\t\t * clean .git in this case in remove_junk_on_signal\n+\t\t\t */\n+\t\t\tdie(_(\"--bare and --separate-git-dir are incompatible.\"));\n \t\toption_no_checkout = 1;\n \t}\n \n-- \n1.8.0.rc2.23.g1fb49df\n"},{"id":"206125","messageId":"20130106101948.GD10956@elie.Belkin","threadId":"32526","inReplyTo":"1357465670-32766-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] clone: forbid --bare --separate-git-dir ","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-06T10:19:48Z","receivedAt":"2013-01-06T10:19:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyễn Thái Ngọc Duy wrote:\n\n> --separate-git-dir was added to clone with the repository away from\n> standard position /.git. It does not make sense to use it\n> without creating working directory.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n\nThe patch correctly implements the above. The description leaves out\ndetail. I'd say something like\n\n\tThe --separate-git-dir option was introduced to make it simple\n\tto put the git directory somewhere outside the worktree, for\n\texample when cloning a repository for use as a submodule.\n\n\tIt was not intended for use when creating a bare repository.\n\tIn that case there is no worktree and it is more natural to\n\tdirectly clone the repository and create a .git file as\n\tseparate steps:\n\n\t\tgit clone --bare /path/to/repo.git bar.git\n\t\tprintf 'gitdir: bar.git\\n' >foo.git\n\n\tUnfortunately we forgot to forbid the --bare\n\t--separate-git-dir combination. In practice, we know no one\n\tcould be using --bare with --separate-git-dir because it is\n\tbroken in the following way: . So it is\n\tsafe to make good on our mistake and forbid the combination,\n\tmaking the command easier to explain.\n\nI don't know what would go in the blank above,\nthough. Is it possible that some people are relying on this option\ncombination?\n"},{"id":"206187","messageId":"7v6239nbw0.fsf@alter.siamese.dyndns.org","threadId":"32526","inReplyTo":"20130106101948.GD10956@elie.Belkin","subject":"Re: [PATCH] clone: forbid --bare --separate-git-dir ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-06T23:13:19Z","receivedAt":"2013-01-06T23:13:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Nguyễn Thái Ngọc Duy wrote:\n>\n>> --separate-git-dir was added to clone with the repository away from\n>> standard position /.git. It does not make sense to use it\n>> without creating working directory.\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>\n> The patch correctly implements the above. The description leaves out\n> detail. I'd say something like\n>\n> \tThe --separate-git-dir option was introduced to make it simple\n> \tto put the git directory somewhere outside the worktree, for\n> \texample when cloning a repository for use as a submodule.\n>\n> \tIt was not intended for use when creating a bare repository.\n> \tIn that case there is no worktree and it is more natural to\n> \tdirectly clone the repository and create a .git file as\n> \tseparate steps:\n>\n> \t\tgit clone --bare /path/to/repo.git bar.git\n> \t\tprintf 'gitdir: bar.git\\n' >foo.git\n>\n> \tUnfortunately we forgot to forbid the --bare\n> \t--separate-git-dir combination. In practice, we know no one\n> \tcould be using --bare with --separate-git-dir because it is\n> \tbroken in the following way: . So it is\n> \tsafe to make good on our mistake and forbid the combination,\n> \tmaking the command easier to explain.\n>\n> I don't know what would go in the blank above,\n> though. Is it possible that some people are relying on this option\n> combination?\n\nI do not necessarily think we must say \"it happens not to work\nalready for such and such reasons, lucky us!\", but it is indeed a\ngood idea to think things through, justifying why this cannot be a\nregression, and record the fact that we did that thinking, in the\nlog message.\n\nThanks.\n"},{"id":"206195","messageId":"CACsJy8A6soMEJ3FXY8MyeERAGN483M2UjqRS4muzzD6uj1QWow@mail.gmail.com","threadId":"32526","inReplyTo":"7v6239nbw0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] clone: forbid --bare --separate-git-dir ","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-07T01:18:43Z","receivedAt":"2013-01-07T01:18:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jan 7, 2013 at 6:13 AM, Junio C Hamano wrote:\n>> I don't know what would go in the blank above,\n>> though. Is it possible that some people are relying on this option\n>> combination?\n>\n> I do not necessarily think we must say \"it happens not to work\n> already for such and such reasons, lucky us!\", but it is indeed a\n> good idea to think things through, justifying why this cannot be a\n> regression, and record the fact that we did that thinking, in the\n> log message.\n>\n> Thanks.\n\nI wanted to give a day or two or think about the .\nDoes \"Thanks.\" mean you have picked up the patch and adjusted the\ncommit message appropriately, or should I go with my original plan and\nresend it later with \"explanantion there\"?\n-- \nDuy\n"},{"id":"206199","messageId":"7vpq1hlpeb.fsf@alter.siamese.dyndns.org","threadId":"32526","inReplyTo":"CACsJy8A6soMEJ3FXY8MyeERAGN483M2UjqRS4muzzD6uj1QWow@mail.gmail.com","subject":"Re: [PATCH] clone: forbid --bare --separate-git-dir ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-07T02:04:28Z","receivedAt":"2013-01-07T02:04:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Mon, Jan 7, 2013 at 6:13 AM, Junio C Hamano wrote:\n>>> I don't know what would go in the blank above,\n>>> though. Is it possible that some people are relying on this option\n>>> combination?\n>>\n>> I do not necessarily think we must say \"it happens not to work\n>> already for such and such reasons, lucky us!\", but it is indeed a\n>> good idea to think things through, justifying why this cannot be a\n>> regression, and record the fact that we did that thinking, in the\n>> log message.\n>>\n>> Thanks.\n>\n> I wanted to give a day or two or think about the .\n> Does \"Thanks.\" mean you have picked up the patch and adjusted the\n> commit message appropriately, or should I go with my original plan and\n> resend it later with \"explanantion there\"?\n\nThe latter, \"Thanks for a review\". I may have picked it up in 'pu',\nbut that merely means, as usual, that I wanted to add a reminder in\nWhat's cooking to expect a reroll, and nothing more.\n\nThanks.\n"},{"id":"206293","messageId":"20130108141650.GA18637@lanh","threadId":"32526","inReplyTo":"20130106101948.GD10956@elie.Belkin","subject":"Re: [PATCH] clone: forbid --bare --separate-git-dir ","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-08T14:16:50Z","receivedAt":"2013-01-08T14:16:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Jan 06, 2013 at 02:19:48AM -0800, Jonathan Nieder wrote:\n> \tUnfortunately we forgot to forbid the --bare\n> \t--separate-git-dir combination. In practice, we know no one\n> \tcould be using --bare with --separate-git-dir because it is\n> \tbroken in the following way: . So it is\n> \tsafe to make good on our mistake and forbid the combination,\n> \tmaking the command easier to explain.\n> \n> I don't know what would go in the blank above,\n> though. Is it possible that some people are relying on this option\n> combination?\n\nI can't say it's broken in what way. Maybe it's easier to just support\nthis case, it's not much work anyway. Jens, maybe squash this to your\noriginal patch?\n\n-- 8< --\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 8d23a62..c8b09ad 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -375,6 +375,7 @@ static void clone_local(const char *src_repo, const char *dest_repo)\n \n static const char *junk_work_tree;\n static const char *junk_git_dir;\n+static const char *junk_git_file;\n static pid_t junk_pid;\n \n static void remove_junk(void)\n@@ -392,6 +393,8 @@ static void remove_junk(void)\n \t\tremove_dir_recursively(&sb, 0);\n \t\tstrbuf_reset(&sb);\n \t}\n+\tif (junk_git_file)\n+\t\tunlink(junk_git_file);\n }\n \n static void remove_junk_on_signal(int signo)\n@@ -772,6 +775,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \tset_git_dir_init(git_dir, real_git_dir, 0);\n \tif (real_git_dir) {\n+\t\tif (option_bare)\n+\t\t\tjunk_git_file = git_dir;\n \t\tgit_dir = real_git_dir;\n \t\tjunk_git_dir = real_git_dir;\n \t}\ndiff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh\nindex 4435693..231bc8a 100755\n--- a/t/t5600-clone-fail-cleanup.sh\n+++ b/t/t5600-clone-fail-cleanup.sh\n@@ -49,4 +49,14 @@ test_expect_success 'failed clone --separate-git-dir should not leave any direct\n \trmdir foo/.git/objects.bak\n '\n \n+test_expect_success 'failed clone --separate-git-dir --bare should not leave any directories' '\n+\tmkdir foo/.git/objects.bak/ &&\n+\tmv foo/.git/objects/* foo/.git/objects.bak/ &&\n+\ttest_must_fail git clone --bare --separate-git-dir gitdir foo baaar &&\n+\ttest_must_fail test -e gitdir &&\n+\ttest_must_fail test -e baaar &&\n+\tmv foo/.git/objects.bak/* foo/.git/objects/ &&\n+\trmdir foo/.git/objects.bak\n+'\n+\n test_done\n-- 8< --\n"},{"id":"206304","messageId":"50EC543D.5090100@web.de","threadId":"32526","inReplyTo":"20130108141650.GA18637@lanh","subject":"Re: [PATCH] clone: forbid --bare --separate-git-dir ","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-01-08T17:15:41Z","receivedAt":"2013-01-08T17:15:41Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 08.01.2013 15:16, schrieb Duy Nguyen:\n> On Sun, Jan 06, 2013 at 02:19:48AM -0800, Jonathan Nieder wrote:\n>> \tUnfortunately we forgot to forbid the --bare\n>> \t--separate-git-dir combination. In practice, we know no one\n>> \tcould be using --bare with --separate-git-dir because it is\n>> \tbroken in the following way: . So it is\n>> \tsafe to make good on our mistake and forbid the combination,\n>> \tmaking the command easier to explain.\n>>\n>> I don't know what would go in the blank above,\n>> though. Is it possible that some people are relying on this option\n>> combination?\n> \n> I can't say it's broken in what way. Maybe it's easier to just support\n> this case, it's not much work anyway. Jens, maybe squash this to your\n> original patch?\n\nI'd be fine with that, though my gut feeling is that this should\nbe a patch of its own (My patch handles the git dir, your's handles\na work tree issue). But I don't care much either way, what do others\nthink?\n\n> -- 8< --\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 8d23a62..c8b09ad 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -375,6 +375,7 @@ static void clone_local(const char *src_repo, const char *dest_repo)\n> \n> static const char *junk_work_tree;\n> static const char *junk_git_dir;\n> +static const char *junk_git_file;\n> static pid_t junk_pid;\n> \n> static void remove_junk(void)\n> @@ -392,6 +393,8 @@ static void remove_junk(void)\n> \t\tremove_dir_recursively(&sb, 0);\n> \t\tstrbuf_reset(&sb);\n> \t}\n> +\tif (junk_git_file)\n> +\t\tunlink(junk_git_file);\n> }\n> \n> static void remove_junk_on_signal(int signo)\n> @@ -772,6 +775,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \n> \tset_git_dir_init(git_dir, real_git_dir, 0);\n> \tif (real_git_dir) {\n> +\t\tif (option_bare)\n> +\t\t\tjunk_git_file = git_dir;\n> \t\tgit_dir = real_git_dir;\n> \t\tjunk_git_dir = real_git_dir;\n> \t}\n> diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh\n> index 4435693..231bc8a 100755\n> --- a/t/t5600-clone-fail-cleanup.sh\n> +++ b/t/t5600-clone-fail-cleanup.sh\n> @@ -49,4 +49,14 @@ test_expect_success 'failed clone --separate-git-dir should not leave any direct\n> \trmdir foo/.git/objects.bak\n> '\n> \n> +test_expect_success 'failed clone --separate-git-dir --bare should not leave any directories' '\n> +\tmkdir foo/.git/objects.bak/ &&\n> +\tmv foo/.git/objects/* foo/.git/objects.bak/ &&\n> +\ttest_must_fail git clone --bare --separate-git-dir gitdir foo baaar &&\n> +\ttest_must_fail test -e gitdir &&\n> +\ttest_must_fail test -e baaar &&\n> +\tmv foo/.git/objects.bak/* foo/.git/objects/ &&\n> +\trmdir foo/.git/objects.bak\n> +'\n> +\n> test_done\n> -- 8< --\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"206310","messageId":"7v4nirfu1p.fsf@alter.siamese.dyndns.org","threadId":"32526","inReplyTo":"50EC543D.5090100@web.de","subject":"Re: [PATCH] clone: forbid --bare --separate-git-dir ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-08T17:45:06Z","receivedAt":"2013-01-08T17:45:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann writes:\n\n> Am 08.01.2013 15:16, schrieb Duy Nguyen:\n>> On Sun, Jan 06, 2013 at 02:19:48AM -0800, Jonathan Nieder wrote:\n>>> \tUnfortunately we forgot to forbid the --bare\n>>> \t--separate-git-dir combination. In practice, we know no one\n>>> \tcould be using --bare with --separate-git-dir because it is\n>>> \tbroken in the following way: . So it is\n>>> \tsafe to make good on our mistake and forbid the combination,\n>>> \tmaking the command easier to explain.\n>>>\n>>> I don't know what would go in the blank above,\n>>> though. Is it possible that some people are relying on this option\n>>> combination?\n>> \n>> I can't say it's broken in what way. Maybe it's easier to just support\n>> this case, it's not much work anyway. Jens, maybe squash this to your\n>> original patch?\n>\n> I'd be fine with that, though my gut feeling is that this should\n> be a patch of its own (My patch handles the git dir, your's handles\n> a work tree issue).\n\nI agree that these are totally unrelated issues.\n\nAfter all, Jonathan's suggestion to forbid it was because the\ncombination does not make sense and does not have practical uses,\nand forbidding it would make the command easier to explain than\nleaving it accepted from the command line. If you choose to go in\nthe opposite direction and make \"clone --bare --separate-git-dir\" do\nsomething useful, it should be explained very well in the\ndocumentation part of the patch why such a combination is a good\nidea, and in what situation the behaviour is useful and the user may\nwant to consider using it, I think.\n"},{"id":"206342","messageId":"CACsJy8B=h04QAeb0D-PWvT=0n_+QfW27NuUg3KEFUN3C4MOJVQ@mail.gmail.com","threadId":"32526","inReplyTo":"7v4nirfu1p.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] clone: forbid --bare --separate-git-dir ","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-08T23:34:21Z","receivedAt":"2013-01-08T23:34:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jan 9, 2013 at 12:45 AM, Junio C Hamano wrote:\n> Jens Lehmann writes:\n>\n>> Am 08.01.2013 15:16, schrieb Duy Nguyen:\n>>> On Sun, Jan 06, 2013 at 02:19:48AM -0800, Jonathan Nieder wrote:\n>>>> Unfortunately we forgot to forbid the --bare\n>>>> --separate-git-dir combination. In practice, we know no one\n>>>> could be using --bare with --separate-git-dir because it is\n>>>> broken in the following way: . So it is\n>>>> safe to make good on our mistake and forbid the combination,\n>>>> making the command easier to explain.\n>>>>\n>>>> I don't know what would go in the blank above,\n>>>> though. Is it possible that some people are relying on this option\n>>>> combination?\n>>>\n>>> I can't say it's broken in what way. Maybe it's easier to just support\n>>> this case, it's not much work anyway. Jens, maybe squash this to your\n>>> original patch?\n>>\n>> I'd be fine with that, though my gut feeling is that this should\n>> be a patch of its own (My patch handles the git dir, your's handles\n>> a work tree issue).\n>\n> I agree that these are totally unrelated issues.\n>\n> After all, Jonathan's suggestion to forbid it was because the\n> combination does not make sense and does not have practical uses,\n> and forbidding it would make the command easier to explain than\n> leaving it accepted from the command line. If you choose to go in\n> the opposite direction and make \"clone --bare --separate-git-dir\" do\n> something useful, it should be explained very well in the\n> documentation part of the patch why such a combination is a good\n> idea, and in what situation the behaviour is useful and the user may\n> want to consider using it, I think.\n\nIt is more like postponing the usefulness evaluation of the\ncombination until later (maybe someone will come up with an actual use\ncase). As of now, --separate-git-dir --bare is a valid combination.\nJens' patch fixes one case but leave the other case broken, which is\nwhy I think it should be in one patch. It's rather ducking head in the\nsand than actually declaring that the combination is useful.\n-- \nDuy\n"},{"id":"206344","messageId":"7vmwwjb5td.fsf@alter.siamese.dyndns.org","threadId":"32526","inReplyTo":"CACsJy8B=h04QAeb0D-PWvT=0n_+QfW27NuUg3KEFUN3C4MOJVQ@mail.gmail.com","subject":"Re: [PATCH] clone: forbid --bare --separate-git-dir ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-08T23:42:06Z","receivedAt":"2013-01-08T23:42:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> After all, Jonathan's suggestion to forbid it was because the\n>> combination does not make sense and does not have practical uses,\n>> and forbidding it would make the command easier to explain than\n>> leaving it accepted from the command line. If you choose to go in\n>> the opposite direction and make \"clone --bare --separate-git-dir\" do\n>> something useful, it should be explained very well in the\n>> documentation part of the patch why such a combination is a good\n>> idea, and in what situation the behaviour is useful and the user may\n>> want to consider using it, I think.\n>\n> It is more like postponing the usefulness evaluation of the\n> combination until later (maybe someone will come up with an actual use\n> case). As of now, --separate-git-dir --bare is a valid combination.\n> Jens' patch fixes one case but leave the other case broken, which is\n> why I think it should be in one patch. It's rather ducking head in the\n> sand than actually declaring that the combination is useful.\n\nWhen a user comes and asks how \"git clone --bare --separate-git-dir\"\nis meant to be used, you are saying that your answer will be \"Eh, it\ndoes something random that I cannot explain, and I cannot even\nsuggest a good use case for it, but somebody may find it useful.\"?\n\nIf we get rid of it, we do not have to explain what such a useless\ncombination would/should do, no?\n"},{"id":"206482","messageId":"1357873799-17413-1-git-send-email-pclouds@gmail.com","threadId":"32526","inReplyTo":"1357465670-32766-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2] clone: forbid --bare --separate-git-dir ","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-11T03:09:59Z","receivedAt":"2013-01-11T03:09:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The --separate-git-dir option was introduced to make it simple to put\nthe git directory somewhere outside the worktree, for example when\ncloning a repository for use as a submodule.\n\nIt was not intended for use when creating a bare repository. In that\ncase there is no worktree and it is more natural to directly clone the\nrepository and create a .git file as separate steps:\n\n git clone --bare /path/to/repo.git bar.git\n printf 'gitdir: bar.git\\n' >foo.git\n\nForbid the combination, making the command easier to explain.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Just reword the commit message (or copying it from Jonathan\n actually). No comments about remove_junk_on_signal because we're less\n likely to re-enable it again.\n\n builtin/clone.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex ec2f75b..b30189f 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -704,6 +704,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tif (option_origin)\n \t\t\tdie(_(\"--bare and --origin %s options are incompatible.\"),\n \t\t\t option_origin);\n+\t\tif (real_git_dir)\n+\t\t\tdie(_(\"--bare and --separate-git-dir are incompatible.\"));\n \t\toption_no_checkout = 1;\n \t}\n \n-- \n1.8.0.rc2.23.g1fb49df\n"},{"id":"206483","messageId":"7v1udse7f5.fsf@alter.siamese.dyndns.org","threadId":"32526","inReplyTo":"1357873799-17413-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2] clone: forbid --bare --separate-git-dir ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-11T03:15:58Z","receivedAt":"2013-01-11T03:15:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"}]}