{"thread":{"id":"29855","subject":"[PATCH] fix deletion of .git/objects sub-directories in git-prune/repack","startedAt":"2012-03-06T09:18:41Z","lastAt":"2012-03-07T10:50:34Z","messageCount":9,"participants":["karsten.blees@dcon.de","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"186189","messageId":"OF93114E93.E64C1F7D-ONC12579B9.0032A592-C12579B9.00332D78@dcon.de","threadId":"29855","inReplyTo":"CABPQNSbo5yEosEid_SKoiCS4c8eYAaOHy4skSOBJsef+E6H6Sw@mail.gmail.com","subject":"[PATCH] fix deletion of .git/objects sub-directories in git-prune/repack","fromName":"","fromEmail":"karsten.blees@dcon.de","sentAt":"2012-03-06T09:18:41Z","receivedAt":"2012-03-06T09:18:41Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"On some systems (e.g. Windows XP), directories cannot be deleted while\nthey're open. Both git-prune and git-repack (and thus, git-gc) try to\nrmdir while holding a DIR* handle on the directory, leaving dangling\nempty directories in the .git/objects store.\n\nFix it by swapping the rmdir / closedir calls.\n\nReported-by: John Chen \nReported-by: Stefan Naewe \nSigned-off-by: Karsten Blees \n---\n\nErik Faye-Lund wrote on 05.03.2012 11:26:08:\n> On Mon, Mar 5, 2012 at 10:57 AM, wrote:\n[...]\n> > I thought the delete / rename problems were a Windows illness (at\n> > least the retry-ask-user-yes-no logic is exclusive to mingw.c)?\n> \n> POSIX allows for this behavior:\n> \n> http://pubs.opengroup.org/onlinepubs/009695399/functions/rmdir.html\n> \n> (EBUSY: \"The directory to be removed is currently in use by the system\n> or some process and the implementation considers this to be an\n> error.\")\n> \n> I've found traces of similar issues on exotic unices, so I don't think\n> it's completely hypothetical either...\n> \n> > And as all mingw stuff\n> > is eventually sent upstream (I hope :-)), I don't see a particular\n> > reason to rush this one.\n> >\n> \n> There's no automatic system for this. I tend to send all my patches\n> that cleanly fix problems present in git.git directly upstream, so\n> people on Windows who doesn't use our branch can benefit from them as\n> well.\n\nSo what about this one? It applies cleanly to git master, and when\nmerged / cherry-picked to msysgit devel, the prune-packed.c fix we\nalready have in b4cb824d is silently ignored.\n\nNote: the is_dir_empty() function in mingw.c is broken in upstream git\nas well, but I'm reluctant to backport this as it will clash with the\nUnicode patches.\n\nThe upstream patch can be pulled from here:\nhttps://github.com/kblees/git/commit/56e1bc62\n\nAnd cherry-picked to msysgit devel from here:\nhttps://github.com/kblees/git/commit/b07bfd51\n\nRegards,\nKarsten\n\n\n builtin/prune-packed.c | 2 +-\n builtin/prune.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/prune-packed.c b/builtin/prune-packed.c\nindex f9463de..a834417 100644\n--- a/builtin/prune-packed.c\n+++ b/builtin/prune-packed.c\n@@ -36,7 +36,6 @@ static void prune_dir(int i, DIR *dir, char *pathname, \nint len, int opts)\n display_progress(progress, i + 1);\n }\n pathname[len] = 0;\n- rmdir(pathname);\n }\n \n void prune_packed_objects(int opts)\n@@ -65,6 +64,7 @@ void prune_packed_objects(int opts)\n continue;\n prune_dir(i, d, pathname, len + 3, opts);\n closedir(d);\n+ rmdir(pathname);\n }\n stop_progress(&progress);\n }\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 58d7cb8..b99b635 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -85,9 +85,9 @@ static int prune_dir(int i, char *path)\n }\n fprintf(stderr, \"bad sha1 file: %s/%s\\n\", path, \nde->d_name);\n }\n+ closedir(dir);\n if (!show_only)\n rmdir(path);\n- closedir(dir);\n return 0;\n }\n \n-- \n1.7.9.msysgit.1.364.g3e2096\n"},{"id":"186233","messageId":"4F565849.80303@kdbg.org","threadId":"29855","inReplyTo":"OF93114E93.E64C1F7D-ONC12579B9.0032A592-C12579B9.00332D78@dcon.de","subject":"Re: [msysGit] [PATCH] fix deletion of .git/objects sub-directories in git-prune/repack","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2012-03-06T18:32:41Z","receivedAt":"2012-03-06T18:32:41Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 06.03.2012 10:18, schrieb karsten.blees@dcon.de:\n> On some systems (e.g. Windows XP), directories cannot be deleted while\n> they're open. Both git-prune and git-repack (and thus, git-gc) try to\n> rmdir while holding a DIR* handle on the directory, leaving dangling\n> empty directories in the .git/objects store.\n> \n> Fix it by swapping the rmdir / closedir calls.\n\nThe reasoning makes a lot of sense. I wonder why object directories are\npruned nevertheless when I run git gc --prune (I run git master plus a\nfew topics from pu).\n\n> diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c\n> index f9463de..a834417 100644\n> --- a/builtin/prune-packed.c\n> +++ b/builtin/prune-packed.c\n> @@ -36,7 +36,6 @@ static void prune_dir(int i, DIR *dir, char *pathname, \n> int len, int opts)\n> display_progress(progress, i + 1);\n> }\n> pathname[len] = 0;\n> - rmdir(pathname);\n\nAfter moving the rmdir() away from prune_dir(), the truncation of the\npathname does not logically belong here anymore. It should be moved with\nthe rmdir(). Looks good otherwise.\n\n> }\n> \n> void prune_packed_objects(int opts)\n> @@ -65,6 +64,7 @@ void prune_packed_objects(int opts)\n> continue;\n> prune_dir(i, d, pathname, len + 3, opts);\n> closedir(d);\n> + rmdir(pathname);\n> }\n> stop_progress(&progress);\n> }\n> diff --git a/builtin/prune.c b/builtin/prune.c\n> index 58d7cb8..b99b635 100644\n> --- a/builtin/prune.c\n> +++ b/builtin/prune.c\n> @@ -85,9 +85,9 @@ static int prune_dir(int i, char *path)\n> }\n> fprintf(stderr, \"bad sha1 file: %s/%s\\n\", path, \n> de->d_name);\n> }\n> + closedir(dir);\n> if (!show_only)\n> rmdir(path);\n> - closedir(dir);\n> return 0;\n> }\n> \n"},{"id":"186247","messageId":"7v4nu1scsl.fsf@alter.siamese.dyndns.org","threadId":"29855","inReplyTo":"4F565849.80303@kdbg.org","subject":"Re: [msysGit] [PATCH] fix deletion of .git/objects sub-directories in git-prune/repack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-06T20:19:06Z","receivedAt":"2012-03-06T20:19:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n>> diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c\n>> index f9463de..a834417 100644\n>> --- a/builtin/prune-packed.c\n>> +++ b/builtin/prune-packed.c\n>> @@ -36,7 +36,6 @@ static void prune_dir(int i, DIR *dir, char *pathname, \n>> int len, int opts)\n>> display_progress(progress, i + 1);\n>> }\n>> pathname[len] = 0;\n>> - rmdir(pathname);\n>\n> After moving the rmdir() away from prune_dir(), the truncation of the\n> pathname does not logically belong here anymore. It should be moved with\n> the rmdir(). Looks good otherwise.\n\nI agree that it is better to have the NUL termination close to where\nit actually matters.\n\nDo you want me to take it after locally fixing it up, or do you\nprefer to feed this as part of msysgit related updates to me later?\n"},{"id":"186259","messageId":"4F567F4E.5020501@kdbg.org","threadId":"29855","inReplyTo":"7v4nu1scsl.fsf@alter.siamese.dyndns.org","subject":"Re: [msysGit] [PATCH] fix deletion of .git/objects sub-directories in git-prune/repack","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2012-03-06T21:19:10Z","receivedAt":"2012-03-06T21:19:10Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 06.03.2012 21:19, schrieb Junio C Hamano:\n> Johannes Sixt writes:\n> \n>>> diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c\n>>> index f9463de..a834417 100644\n>>> --- a/builtin/prune-packed.c\n>>> +++ b/builtin/prune-packed.c\n>>> @@ -36,7 +36,6 @@ static void prune_dir(int i, DIR *dir, char *pathname, \n>>> int len, int opts)\n>>> display_progress(progress, i + 1);\n>>> }\n>>> pathname[len] = 0;\n>>> - rmdir(pathname);\n>>\n>> After moving the rmdir() away from prune_dir(), the truncation of the\n>> pathname does not logically belong here anymore. It should be moved with\n>> the rmdir(). Looks good otherwise.\n> \n> I agree that it is better to have the NUL termination close to where\n> it actually matters.\n> \n> Do you want me to take it after locally fixing it up, or do you\n> prefer to feed this as part of msysgit related updates to me later?\n\nThis patch is fairly independent from other topics that are queued in\nmsysgit, IIRC. Please take it with the mentioned fixup.\n\nThanks,\n-- Hannes\n"},{"id":"186261","messageId":"OFBC9E02C9.526DB40C-ONC12579B9.00726ECA-C12579B9.00763181@dcon.de","threadId":"29855","inReplyTo":"7v4nu1scsl.fsf@alter.siamese.dyndns.org","subject":"Re: [msysGit] [PATCH] fix deletion of .git/objects sub-directories in git-prune/repack","fromName":"","fromEmail":"karsten.blees@dcon.de","sentAt":"2012-03-06T21:30:40Z","receivedAt":"2012-03-06T21:30:40Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Junio C Hamano wrote on 06.03.2012 21:19:06:\n> Johannes Sixt writes:\n> \n> >> diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c\n> >> index f9463de..a834417 100644\n> >> --- a/builtin/prune-packed.c\n> >> +++ b/builtin/prune-packed.c\n> >> @@ -36,7 +36,6 @@ static void prune_dir(int i, DIR *dir, char \n*pathname, \n> >> int len, int opts)\n> >> display_progress(progress, i + 1);\n> >> }\n> >> pathname[len] = 0;\n> >> - rmdir(pathname);\n> >\n> > After moving the rmdir() away from prune_dir(), the truncation of the\n> > pathname does not logically belong here anymore. It should be moved \nwith\n> > the rmdir(). Looks good otherwise.\n> \n> I agree that it is better to have the NUL termination close to where\n> it actually matters.\n> \n\nThe pathname is extended in prune_dir, so I think it should be reset there\nas well; moving it to prune_packed_objects would be quite obscure:\n\nd = opendir(pathname);\nprune_dir(d, pathname);\npathname[len] = 0;\nrmdir(pathname);\n\n\nOT: While looking at the code I just stumbled across this immediately\nabove the patch (prune-packed.c line 32ff):\n\n memcpy(pathname + len, de->d_name, 38);\n if (opts & DRY_RUN)\n printf(\"rm -f %s\\n\", pathname);\n else\n unlink_or_warn(pathname);\n\nShouldn't this be memcpy(..., 39) (i.e. including '\\0')?\n\n> Do you want me to take it after locally fixing it up, or do you\n> prefer to feed this as part of msysgit related updates to me later?\n\nThe msysgit queue is quite long, and I think it makes sense to fast-track \nthis one.\n"},{"id":"186263","messageId":"OF619F6614.68A2AA34-ONC12579B9.00763235-C12579B9.0077B76F@dcon.de","threadId":"29855","inReplyTo":"4F565849.80303@kdbg.org","subject":"Re: [PATCH] fix deletion of .git/objects sub-directories in git-prune/repack","fromName":"","fromEmail":"karsten.blees@dcon.de","sentAt":"2012-03-06T21:47:19Z","receivedAt":"2012-03-06T21:47:19Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Johannes Sixt wrote on 06.03.2012 19:32:41:\n> Am 06.03.2012 10:18, schrieb karsten.blees@dcon.de:\n> > On some systems (e.g. Windows XP), directories cannot be deleted while\n> > they're open. Both git-prune and git-repack (and thus, git-gc) try to\n> > rmdir while holding a DIR* handle on the directory, leaving dangling\n> > empty directories in the .git/objects store.\n> > \n> > Fix it by swapping the rmdir / closedir calls.\n> \n> The reasoning makes a lot of sense. I wonder why object directories are\n> pruned nevertheless when I run git gc --prune (I run git master plus a\n> few topics from pu).\n> \n\nYou're right...in upstream git, closedir() is essentially a NOOP. The OS\nhandle is closed in readdir() on reading the last entry. The dirent.c\n'improvements' that move FindFirstFile() to opendir() and FindClose() to\nclosedir() are in the msysgit fork only [1] (and entirely my fault, I\nguess :-)).\n\nSo this patch actually isn't currently required for Windows XP (probably\nother platforms, I don't know).\n\n[1] https://github.com/msysgit/git/commit/e2b3f70b\n\n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n\n*** Please avoid top-posting. ***\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n"},{"id":"186264","messageId":"7vmx7tqu0q.fsf@alter.siamese.dyndns.org","threadId":"29855","inReplyTo":"OFBC9E02C9.526DB40C-ONC12579B9.00726ECA-C12579B9.00763181@dcon.de","subject":"Re: [msysGit] [PATCH] fix deletion of .git/objects sub-directories in git-prune/repack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-06T21:49:57Z","receivedAt":"2012-03-06T21:49:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"karsten.blees@dcon.de writes:\n\n> Junio C Hamano wrote on 06.03.2012 21:19:06:\n>> Johannes Sixt writes:\n>> \n>> >> diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c\n>> >> index f9463de..a834417 100644\n>> >> --- a/builtin/prune-packed.c\n>> >> +++ b/builtin/prune-packed.c\n>> >> @@ -36,7 +36,6 @@ static void prune_dir(int i, DIR *dir, char \n> *pathname, \n>> >> int len, int opts)\n>> >> display_progress(progress, i + 1);\n>> >> }\n>> >> pathname[len] = 0;\n>> >> - rmdir(pathname);\n>> >\n>> > After moving the rmdir() away from prune_dir(), the truncation of the\n>> > pathname does not logically belong here anymore. It should be moved \n> with\n>> > the rmdir(). Looks good otherwise.\n>> \n>> I agree that it is better to have the NUL termination close to where\n>> it actually matters.\n>> \n>\n> The pathname is extended in prune_dir, so I think it should be reset there\n> as well; moving it to prune_packed_objects would be quite obscure:\n\nThis depends entirely on how you look at it.\n\nYou can certainly stare at the original code and declare that the\ncontract between the caller and the callee was that the caller gives\npathname[] and len (len+3 for the caller) to the callee, and allows\nthe callee to play with the rest of the pathname[] array but expects\nthat pathname[] to be properly NUL-terminated when the callee comes\nback. From that point of view, \"pathname[len] = 0\" can belong to\nthe callee.\n\nBut while you are staring the original code, notice that \"expects\nthat pathname[] to be NUL-terminated when the callee comes back\" is\nnot something the caller even depends on. That expectation starts\nto matter _only_ if you move rmdir(pathname) to the caller.\n\nThat is why I said \"where it actually matters.\"\n\nIn other words, \"The caller allows the callee to play with the rest\nof the pathname[]; as long as the callee does not touch earlier\nparts of the array, it can do anything before it returns\", without\nrequiring the callee to NUL-terminate the pathname[] to restore to\nits original state, is equally a valid contract between the caller\nand the callee in the original code.\n\nAnd that also holds true for the updated code that has rmdir() in\nthe caller.\n\n> OT: While looking at the code I just stumbled across this immediately\n> above the patch (prune-packed.c line 32ff):\n>\n> memcpy(pathname + len, de->d_name, 38);\n> if (opts & DRY_RUN)\n> printf(\"rm -f %s\\n\", pathname);\n> else\n> unlink_or_warn(pathname);\n>\n> Shouldn't this be memcpy(..., 39) (i.e. including '\\0')?\n\nThat is not necessary, I think, as get_sha1_hex() does not look at\nthat NUL in the first place.\n"},{"id":"186265","messageId":"7vipihqtns.fsf@alter.siamese.dyndns.org","threadId":"29855","inReplyTo":"OFBC9E02C9.526DB40C-ONC12579B9.00726ECA-C12579B9.00763181@dcon.de","subject":"Re: [msysGit] [PATCH] fix deletion of .git/objects sub-directories in git-prune/repack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-06T21:57:43Z","receivedAt":"2012-03-06T21:57:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"karsten.blees@dcon.de writes:\n\n> OT: While looking at the code I just stumbled across this immediately\n> above the patch (prune-packed.c line 32ff):\n>\n> memcpy(pathname + len, de->d_name, 38);\n> if (opts & DRY_RUN)\n> printf(\"rm -f %s\\n\", pathname);\n> else\n> unlink_or_warn(pathname);\n>\n> Shouldn't this be memcpy(..., 39) (i.e. including '\\0')?\n\nI think the only thing that is guaranteeing that pathname[len+38] is\nNUL is that we do not hop around repositories, so once we fill the\nstatic char pathname[PATH_MAX] in prune_packed_objects(), nobody\nwrites to that location, because the length of the leading part\n(i.e. \"len\" given to prune_dir()) will stay constant during the\nlifetime of the process.\n\nSo it is not currently a problem, but it would be better to clear\nthat byte here.\n\nGood eyes.\n"},{"id":"186296","messageId":"OF193C81EB.4E07375C-ONC12579BA.00363763-C12579BA.003B9773@dcon.de","threadId":"29855","inReplyTo":"7vmx7tqu0q.fsf@alter.siamese.dyndns.org","subject":"Re: [msysGit] [PATCH] fix deletion of .git/objects sub-directories in git-prune/repack","fromName":"","fromEmail":"karsten.blees@dcon.de","sentAt":"2012-03-07T10:50:34Z","receivedAt":"2012-03-07T10:50:34Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Junio C Hamano wrote on 06.03.2012 22:49:57:\n> karsten.blees@dcon.de writes:\n> \n> > Junio C Hamano wrote on 06.03.2012 21:19:06:\n> >> Johannes Sixt writes:\n> >> \n> >> >> diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c\n> >> >> index f9463de..a834417 100644\n> >> >> --- a/builtin/prune-packed.c\n> >> >> +++ b/builtin/prune-packed.c\n> >> >> @@ -36,7 +36,6 @@ static void prune_dir(int i, DIR *dir, char \n> > *pathname, \n> >> >> int len, int opts)\n> >> >> display_progress(progress, i + 1);\n> >> >> }\n> >> >> pathname[len] = 0;\n> >> >> - rmdir(pathname);\n> >> >\n> >> > After moving the rmdir() away from prune_dir(), the truncation of \nthe\n> >> > pathname does not logically belong here anymore. It should be moved \n\n> > with\n> >> > the rmdir(). Looks good otherwise.\n> >> \n> >> I agree that it is better to have the NUL termination close to where\n> >> it actually matters.\n> >> \n> >\n> > The pathname is extended in prune_dir, so I think it should be reset \nthere\n> > as well; moving it to prune_packed_objects would be quite obscure:\n> \n> This depends entirely on how you look at it.\n> \n> You can certainly stare at the original code and declare that the\n> contract between the caller and the callee was that the caller gives\n> pathname[] and len (len+3 for the caller) to the callee, and allows\n> the callee to play with the rest of the pathname[] array but expects\n> that pathname[] to be properly NUL-terminated when the callee comes\n> back. From that point of view, \"pathname[len] = 0\" can belong to\n> the callee.\n> \n> But while you are staring the original code, notice that \"expects\n> that pathname[] to be NUL-terminated when the callee comes back\" is\n> not something the caller even depends on. That expectation starts\n> to matter _only_ if you move rmdir(pathname) to the caller.\n> \n> That is why I said \"where it actually matters.\"\n> \n\nWell, I just don't like that a function designed to prune a directory\nmodifies its input parameters as a side effect (it is bad enough that\nprune_dir uses the caller's buffer at all). You know, trying to limit\nside effects as a general programming principle.\n\nIn my opinion, it doesn't matter at all if the caller actually depends on\nunmodified parameters or not, it just makes robust APIs that encourage\nreuse.\n\nJust my 2 cents, though, prune_packed_objects and prune_dir are so tightly\ncoupled that it probably doesn't make any difference...(on the other hand,\nwe have near identical code in prune.c, so thinking about reuse is not so\nfar fetched)\n"}]}