{"thread":{"id":"39907","subject":"Question: .idx without .pack causes performance issues?","startedAt":"2015-07-21T18:41:58Z","lastAt":"2016-01-13T20:23:29Z","messageCount":34,"participants":["Doug Kelly","Junio C Hamano","Eric Sunshine","Jeff King","Thomas Berg"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"266557","messageId":"CAEtYS8QWCg5_DtrJw-e+c50vcG0OpciR6LWon-3GgyngGn+0pQ@mail.gmail.com","threadId":"39907","inReplyTo":null,"subject":"Question: .idx without .pack causes performance issues?","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-07-21T18:41:58Z","receivedAt":"2015-07-21T18:41:58Z","isPatch":false,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"Hi all,\n\nI just wanted to relay an issue we've seen before at my day job (and\nit just recently cropped up again). When moving users from Git for\nWindows 1.8.3 to 1.9.5, we found a few users started having operations\ntake an excruciatingly long amount of time. At some point, we traced\nthe issue to a number of .pack files had been deleted (possibly\ngarbage collected?) -- but their associated .idx files were still\npresent. Upon removing the \"orphaned\" idx files, we found performance\nreturned to normal. Otherwise, git fsck reported no issues with the\nrepositories.\n\nOther users have noted that using git gc would sometimes correct the\nissue for them, but not always.\n\nAnyway, has anyone else experienced this performance degradation? I\nhave some feeling that it's an issue that may be exclusive to Windows\n(or at least, only slow enough to matter on Windows), but I have no\nproof, and I've never heard of an issue like this outside work. (One\nidea that came to mind was even the .idx files were locked, and thus\nnot deleted.) Something tells me deleting the orphaned .idx files\nisn't the \"nicest\" solution, either.\n\nThanks!\n\n--Doug\n\nP.S. In addition to running the Git for Windows/msysgit builds, we\nhave a handful of users running Git Extensions as well, and also have\nbeen seeing an increase in use of Visual Studio 2013 -- which of\ncourse has libgit2 integrated. So, I think the chance that any one of\nthese might be using the repo or holding files open is very high.\n"},{"id":"266560","messageId":"xmqq4mkxwd77.fsf@gitster.dls.corp.google.com","threadId":"39907","inReplyTo":"CAEtYS8QWCg5_DtrJw-e+c50vcG0OpciR6LWon-3GgyngGn+0pQ@mail.gmail.com","subject":"Re: Question: .idx without .pack causes performance issues?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-21T18:57:48Z","receivedAt":"2015-07-21T18:57:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Doug Kelly writes:\n\n> I just wanted to relay an issue we've seen before at my day job (and\n> it just recently cropped up again). When moving users from Git for\n> Windows 1.8.3 to 1.9.5, we found a few users started having operations\n> take an excruciatingly long amount of time. At some point, we traced\n> the issue to a number of .pack files had been deleted (possibly\n> garbage collected?) -- but their associated .idx files were still\n> present. Upon removing the \"orphaned\" idx files, we found performance\n> returned to normal. Otherwise, git fsck reported no issues with the\n> repositories.\n>\n> Other users have noted that using git gc would sometimes correct the\n> issue for them, but not always.\n>\n> Anyway, has anyone else experienced this performance degradation?\n\nI wouldn't be surprised if such a configuration to have leftover\n\".idx\" files that lack \".pack\" affected performance, but I think you\nreally have to work on getting into such a situation (unless your\noperating system is very cooperative and tries hard to corrupt your\nrepository, that is ;-), so I wouldn't be surprised if you were the\nfirst one to report this.\n\nWe open the \".idx\" file and try to keep as many of them in-core,\nwithout opening corresponding \".pack\" until the data is needed. \n\nWhen we need an object, we learn from an \".idx\" file that a\nparticular pack ought to have a copy of it, and then attempt to open\nthe corresponding \".pack\" file. If this fails, we do protect\nourselves from strange repositories with only \".idx\" files by not\nusing that \".idx\" and try to see if the sought-after object exists\nelsewhere (and if there isn't we say \"no such object\", which is also\na correct thing to do).\n\nI however do not think that we mark the in-core structure that\ncorresponds to an open \".idx\" file in any way when such a failure\nhappens. If we really cared enough, we could do so, saying \"we know\nthere is .idx file, but do not bother looking at it again, as we\nknow the corresponding .pack is missing\", and that would speed things\nup a bit, essentially bringing us back to a sane situation without\nany \".idx\" without corresponding \".pack\".\n\nI do not think it is worth the effort, though. It would be more\nfruitful to find out how you end up with \".idx exists but not\ncorresponding .pack\" and if that is some systemic failure, see if\nthere is a way to prevent that from happening in the first place.\n\nAlso, I think it may not be a bad idea to teach \"gc\" to remove stale\n\".idx\" files that do not have corresponding \".pack\" as garbage.\n"},{"id":"266561","messageId":"xmqqzj2puxu2.fsf@gitster.dls.corp.google.com","threadId":"39907","inReplyTo":"xmqq4mkxwd77.fsf@gitster.dls.corp.google.com","subject":"Re: Question: .idx without .pack causes performance issues?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-21T19:15:01Z","receivedAt":"2015-07-21T19:15:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I however do not think that we mark the in-core structure that\n> corresponds to an open \".idx\" file in any way when such a failure\n> happens. If we really cared enough, we could do so, saying \"we know\n> there is .idx file, but do not bother looking at it again, as we\n> know the corresponding .pack is missing\", and that would speed things\n> up a bit, essentially bringing us back to a sane situation without\n> any \".idx\" without corresponding \".pack\".\n>\n> I do not think it is worth the effort, though. It would be more\n> fruitful to find out how you end up with \".idx exists but not\n> corresponding .pack\" and if that is some systemic failure, see if\n> there is a way to prevent that from happening in the first place.\n\nWhile I still think that it is more important to prevent such a\nsituation from occurring in the first place, ignoring .idx that lack\ncorresponding .pack should be fairly simple, perhaps like this.\n\nNote that if we wanted to do this for real, I think such an \".idx\"\nfile should also be added to the \"garbage\" list in the loop in which\nthe second hunk of the following patch appears.\n\n sha1_file.c | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 1cee438..b69298e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1240,6 +1240,19 @@ static void report_pack_garbage(struct string_list *list)\n \treport_helper(list, seen_bits, first, list->nr);\n }\n \n+static int packfile_exists(const char *base, size_t base_len)\n+{\n+\tstruct strbuf path = STRBUF_INIT;\n+\tint status;\n+\n+\tstrbuf_add(&path, base, base_len);\n+\tstrbuf_addstr(&path, \".pack\");\n+\tstatus = file_exists(path.buf);\n+\n+\tstrbuf_release(&path);\n+\treturn status;\n+}\n+\n static void prepare_packed_git_one(char *objdir, int local)\n {\n \tstruct strbuf path = STRBUF_INIT;\n@@ -1281,6 +1294,7 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t\tif (p == NULL &&\n+\t\t\t packfile_exists(path.buf, base_len) &&\n \t\t\t /*\n \t\t\t * See if it really is a valid .idx file with\n \t\t\t * corresponding .pack file that we can map.\n"},{"id":"266570","messageId":"CAEtYS8RUNyhnHWHYRiPr99_p_1x-sHX0cwRRpeVgLL_T4vTG+A@mail.gmail.com","threadId":"39907","inReplyTo":"xmqq4mkxwd77.fsf@gitster.dls.corp.google.com","subject":"Re: Question: .idx without .pack causes performance issues?","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-07-21T19:49:59Z","receivedAt":"2015-07-21T19:49:59Z","isPatch":false,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"On Tue, Jul 21, 2015 at 1:57 PM, Junio C Hamano wrote:\n> I wouldn't be surprised if such a configuration to have leftover\n> \".idx\" files that lack \".pack\" affected performance, but I think you\n> really have to work on getting into such a situation (unless your\n> operating system is very cooperative and tries hard to corrupt your\n> repository, that is ;-), so I wouldn't be surprised if you were the\n> first one to report this.\n\nI'm inclined to believe Windows isn't helping this situation: seems\nlike something it might do, especially because of how it behaves if\none process has a file open. Since I haven't caught a case where these\nfiles show up, maybe adding some tweaks to look for it occurring (such\nas on our Jenkins workers, if it's happening there now) would give us\na better indication of the \"why\" question. It could even be that it\nhas occurred long ago, and the performance issue is just now observed:\nour environment has run 1.7.4, 1.8.3, and now 1.9.5 -- so even an\nunknown bug in a previous version could impact us now.\n\n>\n> We open the \".idx\" file and try to keep as many of them in-core,\n> without opening corresponding \".pack\" until the data is needed.\n>\n> When we need an object, we learn from an \".idx\" file that a\n> particular pack ought to have a copy of it, and then attempt to open\n> the corresponding \".pack\" file. If this fails, we do protect\n> ourselves from strange repositories with only \".idx\" files by not\n> using that \".idx\" and try to see if the sought-after object exists\n> elsewhere (and if there isn't we say \"no such object\", which is also\n> a correct thing to do).\n>\n> I however do not think that we mark the in-core structure that\n> corresponds to an open \".idx\" file in any way when such a failure\n> happens. If we really cared enough, we could do so, saying \"we know\n> there is .idx file, but do not bother looking at it again, as we\n> know the corresponding .pack is missing\", and that would speed things\n> up a bit, essentially bringing us back to a sane situation without\n> any \".idx\" without corresponding \".pack\".\n\nI think this is where the performance hit occurs on Windows: file stat\noperations in general are pretty slow, and I know msysgit did some\nthings to emulate as much of the POSIX API as possible -- which isn't\nalways easy on Windows. But, some of the developers that know\ncompat/win32/ better would know more (I recall the dirent stuff being\npretty complicated, but open/fopen seem rather straightforward). And\nyes -- retrying the operation each time and failing only compounds the\nissue.\n\n>\n> I do not think it is worth the effort, though. It would be more\n> fruitful to find out how you end up with \".idx exists but not\n> corresponding .pack\" and if that is some systemic failure, see if\n> there is a way to prevent that from happening in the first place.\n\nAgreed. It feels like a workaround for a case where you're already in\na bad state...\n\n>\n> Also, I think it may not be a bad idea to teach \"gc\" to remove stale\n> \".idx\" files that do not have corresponding \".pack\" as garbage.\n\nI agree. This seems like a more correct solution -- if gc understands\nto clean up these bad .idx files, it would then be a non-issue when\nsearching the packs. The solution you posted to check if an\nassociated packfile exists -- while perhaps not belonging there --\ncould still be useful to delete orphanend .idx files.\n\nI think you're correct, though -- if you did propose the solution to\nsha1_file.c, it would be necessary to prevent scanning that .idx\nagain, or else any potential gains would be lost continually\nstat()'ing the file. Now, msysgit does have core.fscache to try\ncaching the stat()/lstat() results to lessen the impact, but this\nisn't on by default, I believe.\n"},{"id":"266580","messageId":"xmqqtwsxtey8.fsf@gitster.dls.corp.google.com","threadId":"39907","inReplyTo":"xmqqzj2puxu2.fsf@gitster.dls.corp.google.com","subject":"Re: Question: .idx without .pack causes performance issues?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-21T20:48:15Z","receivedAt":"2015-07-21T20:48:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> While I still think that it is more important to prevent such a\n> situation from occurring in the first place, ignoring .idx that lack\n> corresponding .pack should be fairly simple, perhaps like this.\n> ...\n\nSorry for the noise, but this patch is worthless. We already have\nan equivalent test in add_packed_git() that is called from this same\nplace.\n"},{"id":"266592","messageId":"CAEtYS8QEuEA6d13FH_0_ZbT9YbJ_UdvhkSBYq1xyGCAuznh-GQ@mail.gmail.com","threadId":"39907","inReplyTo":"xmqqtwsxtey8.fsf@gitster.dls.corp.google.com","subject":"Re: Question: .idx without .pack causes performance issues?","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-07-21T21:37:12Z","receivedAt":"2015-07-21T21:37:12Z","isPatch":false,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"On Tue, Jul 21, 2015 at 3:48 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> While I still think that it is more important to prevent such a\n>> situation from occurring in the first place, ignoring .idx that lack\n>> corresponding .pack should be fairly simple, perhaps like this.\n>> ...\n>\n> Sorry for the noise, but this patch is worthless. We already have\n> an equivalent test in add_packed_git() that is called from this same\n> place.\n\nAnd a few extra updates from me: we found that this appears to occur\neven after update to 1.9.5, and setting core.fscache on 2.4.6 has no\nappreciable impact on the time it takes to run \"git fetch\", either.\nOur thought was antivirus (or something else?) might have the file\nopen when git attempts to unlink the .idx, but perhaps it's something\nelse, too? In one case, we had ~560 orphaned .idx files, but 150\nseems sufficient to slow a fetch operation for a few minutes until it\nactually begins transferring objects.\n\nThe \"git gc\" approach to cleaning up the mess is certainly looking\nmore and more attractive... :)\n"},{"id":"267364","messageId":"CAEtYS8SNksc0m5rn_tRk8bGLBeq_8QcBLHyHo=cOfZ+aE6n0gA@mail.gmail.com","threadId":"39907","inReplyTo":"CAEtYS8QEuEA6d13FH_0_ZbT9YbJ_UdvhkSBYq1xyGCAuznh-GQ@mail.gmail.com","subject":"Re: Question: .idx without .pack causes performance issues?","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-08-03T22:17:28Z","receivedAt":"2015-08-03T22:17:28Z","isPatch":false,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"On Tue, Jul 21, 2015 at 4:37 PM, Doug Kelly wrote:\n> On Tue, Jul 21, 2015 at 3:48 PM, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>\n>>> While I still think that it is more important to prevent such a\n>>> situation from occurring in the first place, ignoring .idx that lack\n>>> corresponding .pack should be fairly simple, perhaps like this.\n>>> ...\n>>\n>> Sorry for the noise, but this patch is worthless. We already have\n>> an equivalent test in add_packed_git() that is called from this same\n>> place.\n>\n> And a few extra updates from me: we found that this appears to occur\n> even after update to 1.9.5, and setting core.fscache on 2.4.6 has no\n> appreciable impact on the time it takes to run \"git fetch\", either.\n> Our thought was antivirus (or something else?) might have the file\n> open when git attempts to unlink the .idx, but perhaps it's something\n> else, too? In one case, we had ~560 orphaned .idx files, but 150\n> seems sufficient to slow a fetch operation for a few minutes until it\n> actually begins transferring objects.\n>\n> The \"git gc\" approach to cleaning up the mess is certainly looking\n> more and more attractive... :)\n\nHere's a change to prune.c that at least addresses the issue by removing\n.idx files without an associated pack, but it's by no means pretty. If anyone\nhas any feedback before I turn this into a formal patch, it's more than welcome!\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 10b03d3..8a60282 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"commit.h\"\n #include \"diff.h\"\n+#include \"dir.h\"\n #include \"revision.h\"\n #include \"builtin.h\"\n #include \"reachable.h\"\n@@ -85,15 +86,31 @@ static void remove_temporary_files(const char *path)\n {\n DIR *dir;\n struct dirent *de;\n+ struct strbuf idx, pack;\n\n dir = opendir(path);\n if (!dir) {\n fprintf(stderr, \"Unable to open directory %s\\n\", path);\n return;\n }\n- while ((de = readdir(dir)) != NULL)\n+ while ((de = readdir(dir)) != NULL) {\n if (starts_with(de->d_name, \"tmp_\"))\n prune_tmp_file(mkpath(\"%s/%s\", path, de->d_name));\n+ if (ends_with(de->d_name, \".idx\")) {\n+ strbuf_init(&idx, 0);\n+ strbuf_init(&pack, 0);\n+ strbuf_addstr(&idx, de->d_name);\n+ strbuf_addbuf(&pack, &idx);\n+ if (strbuf_strip_suffix(&pack, \".idx\")) {\n+ strbuf_addstr(&pack, \".pack\");\n+ if (!file_exists(mkpath(\"%s/%s\", path,\npack.buf)))\n+ prune_tmp_file(mkpath(\"%s/%s\",\npath, idx.buf));\n+ }\n+ strbuf_release(&idx);\n+ strbuf_release(&pack);\n+ }\n+\n+ }\n closedir(dir);\n }\n\n--\n"},{"id":"267381","messageId":"xmqqk2tb6dxc.fsf@gitster.dls.corp.google.com","threadId":"39907","inReplyTo":"CAEtYS8SNksc0m5rn_tRk8bGLBeq_8QcBLHyHo=cOfZ+aE6n0gA@mail.gmail.com","subject":"Re: Question: .idx without .pack causes performance issues?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-04T01:27:27Z","receivedAt":"2015-08-04T01:27:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Doug Kelly writes:\n\n> Here's a change to prune.c that at least addresses the issue by removing\n> .idx files without an associated pack, but it's by no means pretty. If anyone\n> has any feedback before I turn this into a formal patch, it's more than welcome!\n\nI'd hesitate to see removal of a file (for that matter, a creation\ntoo) inside a \"while (de = readdir)\" loop. As the original function\nis about temporary files, and the new thing is not about temporary\nfiles at all, I'd further prefer that we do not do it in the same\nloop.\n\nI am wondering if we can add a new mode to report_pack_garbage() in\nsha1_file.c to allow it to remove stale and lone \".idx\". Most of\nthe time we are accessing packs read-only, and I do not want the\nfunction to unconditionally remove lone \".idx\", but perhaps we\ncan teach \"prune\" to set a custom report_garbage() routine and\nreact to a call to its custom report_garbage()?\n\nPerhaps that custom report_garbage() can make a list of \".idx\"\nfiles, iterate over it to pick the lone one without \".pack\" and\nremove them. Or the custom report_garbage() can make a list of lone\n\".idx\" files, if you tweak the interface to report_garbage() to\ncontain th seen_bits value, avoiding the need to check the existence\nof \".pack\" for the second time.\n"},{"id":"267637","messageId":"CAEtYS8SGnFFHM5BFzAo+Z2BzUGbp47AibA3v6qm_uEboRmfaNQ@mail.gmail.com","threadId":"39907","inReplyTo":"xmqqk2tb6dxc.fsf@gitster.dls.corp.google.com","subject":"Re: Question: .idx without .pack causes performance issues?","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-08-07T21:36:34Z","receivedAt":"2015-08-07T21:36:34Z","isPatch":false,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"On Mon, Aug 3, 2015 at 8:27 PM, Junio C Hamano wrote:\n> Doug Kelly writes:\n>\n>> Here's a change to prune.c that at least addresses the issue by removing\n>> .idx files without an associated pack, but it's by no means pretty. If anyone\n>> has any feedback before I turn this into a formal patch, it's more than welcome!\n>\n> I'd hesitate to see removal of a file (for that matter, a creation\n> too) inside a \"while (de = readdir)\" loop. As the original function\n> is about temporary files, and the new thing is not about temporary\n> files at all, I'd further prefer that we do not do it in the same\n> loop.\n>\n> I am wondering if we can add a new mode to report_pack_garbage() in\n> sha1_file.c to allow it to remove stale and lone \".idx\". Most of\n> the time we are accessing packs read-only, and I do not want the\n> function to unconditionally remove lone \".idx\", but perhaps we\n> can teach \"prune\" to set a custom report_garbage() routine and\n> react to a call to its custom report_garbage()?\n>\n> Perhaps that custom report_garbage() can make a list of \".idx\"\n> files, iterate over it to pick the lone one without \".pack\" and\n> remove them. Or the custom report_garbage() can make a list of lone\n> \".idx\" files, if you tweak the interface to report_garbage() to\n> contain th seen_bits value, avoiding the need to check the existence\n> of \".pack\" for the second time.\n\nYeah, I didn't think this was the cleanest solution, and I wasn't even\nthinking about removing while inside the readdir loop, but I can see\nhow that might be a very bad idea. In any case, thanks for the\nsuggestions... I'll be completely blunt in saying I'm far less than\nwell-versed in the Git internals. Looking at the implementation of\nreport_pack_garbage(), it does look like seen_bits already has this\nlogic, and indeed, git count-objects -v reports the files as garbage.\n\nSo, I think you're right: prune would need to set report_garbage\nappropriately, then call count-objects to clean that up. If we wanted\nit to *only* care for lone idx files, we would have to string match on\nthe message (seems fragile), but perhaps a more observant approach\nwould be to add a custom flag to prune to clean *all* garbage in the\nrepository, as passed to report_garbage? Probably wouldn't want to be\nenabled by default, but only on invocation or with careful\nconsideration and setting an appropriate config flag.\n\nThoughts?\n"},{"id":"267640","messageId":"xmqqwpx6wx74.fsf@gitster.dls.corp.google.com","threadId":"39907","inReplyTo":"CAEtYS8SGnFFHM5BFzAo+Z2BzUGbp47AibA3v6qm_uEboRmfaNQ@mail.gmail.com","subject":"Re: Question: .idx without .pack causes performance issues?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-07T22:27:59Z","receivedAt":"2015-08-07T22:27:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Doug Kelly writes:\n\n> So, I think you're right: prune would need to set report_garbage\n> appropriately, then call count-objects to clean that up. If we wanted\n> it to *only* care for lone idx files, we would have to string match on\n> the message (seems fragile), but perhaps a more observant approach\n> would be to add a custom flag to prune to clean *all* garbage in the\n> repository, as passed to report_garbage? Probably wouldn't want to be\n> enabled by default, but only on invocation or with careful\n> consideration and setting an appropriate config flag.\n\nI was thinking along this line.\n\nThen you would set \"report_garbage\" to your own function, call\nprepare_packed_git(), and in your report-garbagte function, collect\npaths with seen_bits set exactly to PACKDIR_FILE_IDX. By the time\nprepare_packed_git() returns, you would have a list of paths only\nwith .idx but without .pack, which you can prune.\n\nWe can later start pruning other garbage, but one step at a time.\n\n-- >8 --\nSubject: prepare_packed_git(): refactor garbage reporting in pack directory\n\nThe hook to report \"garbage\" files in $GIT_OBJECT_DIRECTORY/pack/\ncould be generic but is too specific to count-object's needs.\n\nMove the part to produce human-readable messages to count-objects,\nand refine the interface to callback with the \"bits\" with values\ndefined in the cache.h header file, so that other callers (e.g.\nprune) can later use the same mechanism to enumerate different\nkinds of garbage files and do something intelligent about them,\nother than reporting in textual messages.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/count-objects.c | 26 ++++++++++++++++++++++++--\n cache.h | 7 +++++--\n path.c | 2 +-\n sha1_file.c | 23 ++++++-----------------\n 4 files changed, 36 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex ad0c799..4c3198e 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -15,9 +15,31 @@ static int verbose;\n static unsigned long loose, packed, packed_loose;\n static off_t loose_size;\n \n-static void real_report_garbage(const char *desc, const char *path)\n+const char *bits_to_msg(unsigned seen_bits)\n+{\n+\tswitch (seen_bits) {\n+\tcase 0:\n+\t\treturn \"no corresponding .idx or .pack\";\n+\tcase PACKDIR_FILE_GARBAGE:\n+\t\treturn \"garbage found\";\n+\tcase PACKDIR_FILE_PACK:\n+\t\treturn \"no corresponding .idx\";\n+\tcase PACKDIR_FILE_IDX:\n+\t\treturn \"no corresponding .pack\";\n+\tcase PACKDIR_FILE_PACK|PACKDIR_FILE_IDX:\n+\tdefault:\n+\t\treturn NULL;\n+\t}\n+}\n+\n+static void real_report_garbage(unsigned seen_bits, const char *path)\n {\n \tstruct stat st;\n+\tconst char *desc = bits_to_msg(seen_bits);\n+\n+\tif (!desc)\n+\t\treturn;\n+\n \tif (!stat(path, &st))\n \t\tsize_garbage += st.st_size;\n \twarning(\"%s: %s\", desc, path);\n@@ -27,7 +49,7 @@ static void real_report_garbage(const char *desc, const char *path)\n static void loose_garbage(const char *path)\n {\n \tif (verbose)\n-\t\treport_garbage(\"garbage found\", path);\n+\t\treport_garbage(PACKDIR_FILE_GARBAGE, path);\n }\n \n static int count_loose(const unsigned char *sha1, const char *path, void *data)\ndiff --git a/cache.h b/cache.h\nindex 6bb7119..2d4dedc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1212,8 +1212,11 @@ struct pack_entry {\n \n extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n \n-/* A hook for count-objects to report invalid files in pack directory */\n-extern void (*report_garbage)(const char *desc, const char *path);\n+/* A hook to report invalid files in pack directory */\n+#define PACKDIR_FILE_PACK 1\n+#define PACKDIR_FILE_IDX 2\n+#define PACKDIR_FILE_GARBAGE 4\n+extern void (*report_garbage)(unsigned seen_bits, const char *path);\n \n extern void prepare_packed_git(void);\n extern void reprepare_packed_git(void);\ndiff --git a/path.c b/path.c\nindex 10f4cbf..75ec236 100644\n--- a/path.c\n+++ b/path.c\n@@ -143,7 +143,7 @@ void report_linked_checkout_garbage(void)\n \t\tstrbuf_setlen(&sb, len);\n \t\tstrbuf_addstr(&sb, path);\n \t\tif (file_exists(sb.buf))\n-\t\t\treport_garbage(\"unused in linked checkout\", sb.buf);\n+\t\t\treport_garbage(PACKDIR_FILE_GARBAGE, sb.buf);\n \t}\n \tstrbuf_release(&sb);\n }\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 1cee438..0c0b652 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1183,27 +1183,16 @@ void install_packed_git(struct packed_git *pack)\n \tpacked_git = pack;\n }\n \n-void (*report_garbage)(const char *desc, const char *path);\n+void (*report_garbage)(unsigned seen_bits, const char *path);\n \n static void report_helper(const struct string_list *list,\n \t\t\t int seen_bits, int first, int last)\n {\n-\tconst char *msg;\n-\tswitch (seen_bits) {\n-\tcase 0:\n-\t\tmsg = \"no corresponding .idx or .pack\";\n-\t\tbreak;\n-\tcase 1:\n-\t\tmsg = \"no corresponding .idx\";\n-\t\tbreak;\n-\tcase 2:\n-\t\tmsg = \"no corresponding .pack\";\n-\t\tbreak;\n-\tdefault:\n+\tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX))\n \t\treturn;\n-\t}\n+\n \tfor (; first < last; first++)\n-\t\treport_garbage(msg, list->items[first].string);\n+\t\treport_garbage(seen_bits, list->items[first].string);\n }\n \n static void report_pack_garbage(struct string_list *list)\n@@ -1226,7 +1215,7 @@ static void report_pack_garbage(struct string_list *list)\n \t\tif (baselen == -1) {\n \t\t\tconst char *dot = strrchr(path, '.');\n \t\t\tif (!dot) {\n-\t\t\t\treport_garbage(\"garbage found\", path);\n+\t\t\t\treport_garbage(PACKDIR_FILE_GARBAGE, path);\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tbaselen = dot - path + 1;\n@@ -1298,7 +1287,7 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\t ends_with(de->d_name, \".keep\"))\n \t\t\tstring_list_append(&garbage, path.buf);\n \t\telse\n-\t\t\treport_garbage(\"garbage found\", path.buf);\n+\t\t\treport_garbage(PACKDIR_FILE_GARBAGE, path.buf);\n \t}\n \tclosedir(dir);\n \treport_pack_garbage(&garbage);\n"},{"id":"268002","messageId":"1439488973-11522-1-git-send-email-dougk.ff7@gmail.com","threadId":"39907","inReplyTo":"xmqqwpx6wx74.fsf@gitster.dls.corp.google.com","subject":"[PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-08-13T18:02:52Z","receivedAt":"2015-08-13T18:02:52Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"From: Junio C Hamano \n\nThe hook to report \"garbage\" files in $GIT_OBJECT_DIRECTORY/pack/\ncould be generic but is too specific to count-object's needs.\n\nMove the part to produce human-readable messages to count-objects,\nand refine the interface to callback with the \"bits\" with values\ndefined in the cache.h header file, so that other callers (e.g.\nprune) can later use the same mechanism to enumerate different\nkinds of garbage files and do something intelligent about them,\nother than reporting in textual messages.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/count-objects.c | 26 ++++++++++++++++++++++++--\n cache.h | 7 +++++--\n path.c | 2 +-\n sha1_file.c | 23 ++++++-----------------\n 4 files changed, 36 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex ad0c799..4c3198e 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -15,9 +15,31 @@ static int verbose;\n static unsigned long loose, packed, packed_loose;\n static off_t loose_size;\n \n-static void real_report_garbage(const char *desc, const char *path)\n+const char *bits_to_msg(unsigned seen_bits)\n+{\n+\tswitch (seen_bits) {\n+\tcase 0:\n+\t\treturn \"no corresponding .idx or .pack\";\n+\tcase PACKDIR_FILE_GARBAGE:\n+\t\treturn \"garbage found\";\n+\tcase PACKDIR_FILE_PACK:\n+\t\treturn \"no corresponding .idx\";\n+\tcase PACKDIR_FILE_IDX:\n+\t\treturn \"no corresponding .pack\";\n+\tcase PACKDIR_FILE_PACK|PACKDIR_FILE_IDX:\n+\tdefault:\n+\t\treturn NULL;\n+\t}\n+}\n+\n+static void real_report_garbage(unsigned seen_bits, const char *path)\n {\n \tstruct stat st;\n+\tconst char *desc = bits_to_msg(seen_bits);\n+\n+\tif (!desc)\n+\t\treturn;\n+\n \tif (!stat(path, &st))\n \t\tsize_garbage += st.st_size;\n \twarning(\"%s: %s\", desc, path);\n@@ -27,7 +49,7 @@ static void real_report_garbage(const char *desc, const char *path)\n static void loose_garbage(const char *path)\n {\n \tif (verbose)\n-\t\treport_garbage(\"garbage found\", path);\n+\t\treport_garbage(PACKDIR_FILE_GARBAGE, path);\n }\n \n static int count_loose(const unsigned char *sha1, const char *path, void *data)\ndiff --git a/cache.h b/cache.h\nindex 6bb7119..2d4dedc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1212,8 +1212,11 @@ struct pack_entry {\n \n extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n \n-/* A hook for count-objects to report invalid files in pack directory */\n-extern void (*report_garbage)(const char *desc, const char *path);\n+/* A hook to report invalid files in pack directory */\n+#define PACKDIR_FILE_PACK 1\n+#define PACKDIR_FILE_IDX 2\n+#define PACKDIR_FILE_GARBAGE 4\n+extern void (*report_garbage)(unsigned seen_bits, const char *path);\n \n extern void prepare_packed_git(void);\n extern void reprepare_packed_git(void);\ndiff --git a/path.c b/path.c\nindex 10f4cbf..75ec236 100644\n--- a/path.c\n+++ b/path.c\n@@ -143,7 +143,7 @@ void report_linked_checkout_garbage(void)\n \t\tstrbuf_setlen(&sb, len);\n \t\tstrbuf_addstr(&sb, path);\n \t\tif (file_exists(sb.buf))\n-\t\t\treport_garbage(\"unused in linked checkout\", sb.buf);\n+\t\t\treport_garbage(PACKDIR_FILE_GARBAGE, sb.buf);\n \t}\n \tstrbuf_release(&sb);\n }\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 1cee438..0c0b652 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1183,27 +1183,16 @@ void install_packed_git(struct packed_git *pack)\n \tpacked_git = pack;\n }\n \n-void (*report_garbage)(const char *desc, const char *path);\n+void (*report_garbage)(unsigned seen_bits, const char *path);\n \n static void report_helper(const struct string_list *list,\n \t\t\t int seen_bits, int first, int last)\n {\n-\tconst char *msg;\n-\tswitch (seen_bits) {\n-\tcase 0:\n-\t\tmsg = \"no corresponding .idx or .pack\";\n-\t\tbreak;\n-\tcase 1:\n-\t\tmsg = \"no corresponding .idx\";\n-\t\tbreak;\n-\tcase 2:\n-\t\tmsg = \"no corresponding .pack\";\n-\t\tbreak;\n-\tdefault:\n+\tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX))\n \t\treturn;\n-\t}\n+\n \tfor (; first < last; first++)\n-\t\treport_garbage(msg, list->items[first].string);\n+\t\treport_garbage(seen_bits, list->items[first].string);\n }\n \n static void report_pack_garbage(struct string_list *list)\n@@ -1226,7 +1215,7 @@ static void report_pack_garbage(struct string_list *list)\n \t\tif (baselen == -1) {\n \t\t\tconst char *dot = strrchr(path, '.');\n \t\t\tif (!dot) {\n-\t\t\t\treport_garbage(\"garbage found\", path);\n+\t\t\t\treport_garbage(PACKDIR_FILE_GARBAGE, path);\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tbaselen = dot - path + 1;\n@@ -1298,7 +1287,7 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\t ends_with(de->d_name, \".keep\"))\n \t\t\tstring_list_append(&garbage, path.buf);\n \t\telse\n-\t\t\treport_garbage(\"garbage found\", path.buf);\n+\t\t\treport_garbage(PACKDIR_FILE_GARBAGE, path.buf);\n \t}\n \tclosedir(dir);\n \treport_pack_garbage(&garbage);\n-- \n2.0.5\n"},{"id":"268003","messageId":"1439488973-11522-2-git-send-email-dougk.ff7@gmail.com","threadId":"39907","inReplyTo":"1439488973-11522-1-git-send-email-dougk.ff7@gmail.com","subject":"[PATCH 2/2] gc: Remove garbage .idx files from pack dir","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-08-13T18:02:53Z","receivedAt":"2015-08-13T18:02:53Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"Add a custom report_garbage handler to collect and remove garbage\n.idx files from the pack directory.\n\nSigned-off-by: Doug Kelly \n---\n builtin/gc.c | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex bcc75d9..8352616 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -42,8 +42,18 @@ static struct argv_array prune = ARGV_ARRAY_INIT;\n static struct argv_array prune_worktrees = ARGV_ARRAY_INIT;\n static struct argv_array rerere = ARGV_ARRAY_INIT;\n \n+static struct string_list pack_garbage = STRING_LIST_INIT_DUP;\n+\n static char *pidfile;\n \n+static void clean_pack_garbage(void)\n+{\n+\tint i;\n+\tfor (i = 0; i < pack_garbage.nr; i++)\n+\t\tunlink_or_warn(pack_garbage.items[i].string);\n+\tstring_list_clear(&pack_garbage, 0);\n+}\n+\n static void remove_pidfile(void)\n {\n \tif (pidfile)\n@@ -57,6 +67,12 @@ static void remove_pidfile_on_signal(int signo)\n \traise(signo);\n }\n \n+static void report_pack_garbage(unsigned seen_bits, const char *path)\n+{\n+\tif (seen_bits == PACKDIR_FILE_IDX)\n+\t\tstring_list_append(&pack_garbage, path);\n+}\n+\n static void git_config_date_string(const char *key, const char **output)\n {\n \tif (git_config_get_string_const(key, output))\n@@ -372,6 +388,11 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tif (run_command_v_opt(rerere.argv, RUN_GIT_CMD))\n \t\treturn error(FAILED_RUN, rerere.argv[0]);\n \n+\treport_garbage = report_pack_garbage;\n+\treprepare_packed_git();\n+\tif (pack_garbage.nr > 0)\n+\t\tclean_pack_garbage();\n+\n \tif (auto_gc && too_many_loose_objects())\n \t\twarning(_(\"There are too many unreachable loose objects; \"\n \t\t\t\"run 'git prune' to remove them.\"));\n-- \n2.0.5\n"},{"id":"268009","messageId":"CAPig+cS0ntr1sYzVAPjNCwd8ei4oGQRNs+W=qMBV4Z6NaRWCWA@mail.gmail.com","threadId":"39907","inReplyTo":"1439488973-11522-1-git-send-email-dougk.ff7@gmail.com","subject":"Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-13T18:46:10Z","receivedAt":"2015-08-13T18:46:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 13, 2015 at 2:02 PM, Doug Kelly wrote:\n> From: Junio C Hamano \n>\n> The hook to report \"garbage\" files in $GIT_OBJECT_DIRECTORY/pack/\n> could be generic but is too specific to count-object's needs.\n>\n> Move the part to produce human-readable messages to count-objects,\n> and refine the interface to callback with the \"bits\" with values\n> defined in the cache.h header file, so that other callers (e.g.\n> prune) can later use the same mechanism to enumerate different\n> kinds of garbage files and do something intelligent about them,\n> other than reporting in textual messages.\n>\n> Signed-off-by: Junio C Hamano \n\nSince you're forwarding Junio's patch, you'd also want to sign-off\n(following his).\n\n> ---\n> diff --git a/builtin/count-objects.c b/builtin/count-objects.c\n> index ad0c799..4c3198e 100644\n> --- a/builtin/count-objects.c\n> +++ b/builtin/count-objects.c\n> @@ -15,9 +15,31 @@ static int verbose;\n> static unsigned long loose, packed, packed_loose;\n> static off_t loose_size;\n>\n> -static void real_report_garbage(const char *desc, const char *path)\n> +const char *bits_to_msg(unsigned seen_bits)\n\nIf you don't expect other callers outside this file, then this should\nbe declared 'static'. If you do expect future external callers, then\nthis should be declared in a public header file (but renamed to be\nmore meaningful).\n\n> +{\n> + switch (seen_bits) {\n> + case 0:\n> + return \"no corresponding .idx or .pack\";\n> + case PACKDIR_FILE_GARBAGE:\n> + return \"garbage found\";\n> + case PACKDIR_FILE_PACK:\n> + return \"no corresponding .idx\";\n> + case PACKDIR_FILE_IDX:\n> + return \"no corresponding .pack\";\n> + case PACKDIR_FILE_PACK|PACKDIR_FILE_IDX:\n> + default:\n> + return NULL;\n> + }\n> +}\n"},{"id":"268189","messageId":"xmqq7fotg9fk.fsf@gitster.dls.corp.google.com","threadId":"39907","inReplyTo":"1439488973-11522-2-git-send-email-dougk.ff7@gmail.com","subject":"Re: [PATCH 2/2] gc: Remove garbage .idx files from pack dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-17T16:35:11Z","receivedAt":"2015-08-17T16:35:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Doug Kelly writes:\n\n> Add a custom report_garbage handler to collect and remove garbage\n> .idx files from the pack directory.\n\nYou need to explain \"why\" here. Why do we want to remove them? And\nthe definition of what is \"garbage\" depends on that exact reason why\nwe want to remove them.\n\nYou and I may remember the discussion we had that started with your\ninitial problem description right now. IIRC, it had to do with\nsomething with the performance when there are many lone .idx files\nwithout corresponding .pack file in the repository, or something?\n\nBut those who will be reading \"git log\" output later will not know,\nand we would forget, too.\n\nAs discussed elsewhere, the removal needs to be protected with grace\nperiod, probably controlled via prune_expire.\n\nPerhaps along this line on top of your patch you can squash but this\nis not even compile tested yet, so take it with a grain of salt.\n\nThanks.\n\n builtin/gc.c | 15 ++++++++++++++-\n 1 file changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 4a459f3..a652773 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -49,8 +49,21 @@ static char *pidfile;\n static void clean_pack_garbage(void)\n {\n \tint i;\n-\tfor (i = 0; i < pack_garbage.nr; i++)\n+\tunsigned long expire = approxidate(prune_expire);\n+\n+\tfor (i = 0; i < pack_garbage.nr; i++) {\n+\t\tconst char *path = pack_garbage.items[i].string;\n+\t\tstruct stat st;\n+\n+\t\tif (lstat(path, &st)) {\n+\t\t\terror(\"could not stat '%s'\", path);\n+\t\t\tcontinue; /* don't risk */\n+\t\t}\n+\t\tif (st.st_mtime > expire)\n+\t\t\tcontinue; /* too young */\n+\n \t\tunlink_or_warn(pack_garbage.items[i].string);\n+\t}\n \tstring_list_clear(&pack_garbage, 0);\n }\n \n"},{"id":"268190","messageId":"xmqq37zhg8la.fsf@gitster.dls.corp.google.com","threadId":"39907","inReplyTo":"CAPig+cS0ntr1sYzVAPjNCwd8ei4oGQRNs+W=qMBV4Z6NaRWCWA@mail.gmail.com","subject":"Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-17T16:53:21Z","receivedAt":"2015-08-17T16:53:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> -static void real_report_garbage(const char *desc, const char *path)\n>> +const char *bits_to_msg(unsigned seen_bits)\n>\n> If you don't expect other callers outside this file, then this should\n> be declared 'static'. If you do expect future external callers, then\n> this should be declared in a public header file (but renamed to be\n> more meaningful).\n\nI think this can be private to this file. The sole point of moving\nthis logic to this file is to make it private, after all ;-) Thanks\nfor sharp eyes.\n\nTogether with the need for a description on \"why\", this probably\ndeserves a test or two, probably at the end of t5304.\n\nThanks.\n"},{"id":"268222","messageId":"xmqq37zhd5du.fsf@gitster.dls.corp.google.com","threadId":"39907","inReplyTo":"1439488973-11522-2-git-send-email-dougk.ff7@gmail.com","subject":"Re: [PATCH 2/2] gc: Remove garbage .idx files from pack dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-17T20:30:53Z","receivedAt":"2015-08-17T20:30:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Doug Kelly writes:\n\n> +static struct string_list pack_garbage = STRING_LIST_INIT_DUP;\n> +\n> static char *pidfile;\n> \n> +static void clean_pack_garbage(void)\n> +{\n> +\tint i;\n> +\tfor (i = 0; i < pack_garbage.nr; i++)\n> +\t\tunlink_or_warn(pack_garbage.items[i].string);\n> +\tstring_list_clear(&pack_garbage, 0);\n> +}\n> +\n> static void remove_pidfile(void)\n> {\n> \tif (pidfile)\n> @@ -57,6 +67,12 @@ static void remove_pidfile_on_signal(int signo)\n> \traise(signo);\n> }\n> \n> +static void report_pack_garbage(unsigned seen_bits, const char *path)\n\nThis change makes\"pidfile management\" and \"pack garbage cleaning\"\ntangled in the result. By inserting the definition of the variable\npack_garbage and the function clean_pack_garbage() just before this\nnew function, you can keep everything related to 'pack garbage\ncleaning\" together.\n"},{"id":"272427","messageId":"xmqqbnbilw9u.fsf@gitster.mtv.corp.google.com","threadId":"39907","inReplyTo":"xmqq37zhg8la.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-28T17:48:13Z","receivedAt":"2015-10-28T17:48:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Eric Sunshine writes:\n>\n>>> -static void real_report_garbage(const char *desc, const char *path)\n>>> +const char *bits_to_msg(unsigned seen_bits)\n>>\n>> If you don't expect other callers outside this file, then this should\n>> be declared 'static'. If you do expect future external callers, then\n>> this should be declared in a public header file (but renamed to be\n>> more meaningful).\n>\n> I think this can be private to this file. The sole point of moving\n> this logic to this file is to make it private, after all ;-) Thanks\n> for sharp eyes.\n>\n> Together with the need for a description on \"why\", this probably\n> deserves a test or two, probably at the end of t5304.\n>\n> Thanks.\n\nDoes somebody want to help tying the final loose ends on this topic?\nIt has been listed in the [Stalled] section for too long, I _think_\nwhat it attempts to do is a worthy thing, and it is shame to see the\ninitial implementation and review cycles we have spent so far go to\nwaste.\n\nIf I find nothing else to do before any taker appears, I could\nvolunteer myself, but thought I should ask first.\n\nThanks.\n"},{"id":"272460","messageId":"CAEtYS8TR4mnaGpGDpB3cz_nu2hdCYTWf=PVCJbmzYi6YA53_bg@mail.gmail.com","threadId":"39907","inReplyTo":"xmqqbnbilw9u.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-10-28T22:43:14Z","receivedAt":"2015-10-28T22:43:14Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"On Wed, Oct 28, 2015 at 12:48 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Eric Sunshine writes:\n>>\n>>>> -static void real_report_garbage(const char *desc, const char *path)\n>>>> +const char *bits_to_msg(unsigned seen_bits)\n>>>\n>>> If you don't expect other callers outside this file, then this should\n>>> be declared 'static'. If you do expect future external callers, then\n>>> this should be declared in a public header file (but renamed to be\n>>> more meaningful).\n>>\n>> I think this can be private to this file. The sole point of moving\n>> this logic to this file is to make it private, after all ;-) Thanks\n>> for sharp eyes.\n>>\n>> Together with the need for a description on \"why\", this probably\n>> deserves a test or two, probably at the end of t5304.\n>>\n>> Thanks.\n>\n> Does somebody want to help tying the final loose ends on this topic?\n> It has been listed in the [Stalled] section for too long, I _think_\n> what it attempts to do is a worthy thing, and it is shame to see the\n> initial implementation and review cycles we have spent so far go to\n> waste.\n>\n> If I find nothing else to do before any taker appears, I could\n> volunteer myself, but thought I should ask first.\n>\n> Thanks.\n\nI agree; I've been wanting to get back to it, but had some\nhigher-priority things at work for a while, so I've not had time. I'd\nbe happy to get back into it, but if you get to it first, believe me,\nI'm not going to be offended. :)\n\nI'll see if I can't devote a little extra time to it this upcoming\nweek, though. Hopefully it doesn't need too much additional polishing\nto be ready.\n\nP.S. Does a Googler want to tell the Inbox team that the inability to\nsend plain-text email is really annoying? :P\n"},{"id":"272878","messageId":"1446606308-1668-1-git-send-email-dougk.ff7@gmail.com","threadId":"39907","inReplyTo":"CAEtYS8TR4mnaGpGDpB3cz_nu2hdCYTWf=PVCJbmzYi6YA53_bg@mail.gmail.com","subject":"[PATCH 1/3] prepare_packed_git(): refactor garbage reporting in pack directory","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-11-04T03:05:06Z","receivedAt":"2015-11-04T03:05:06Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"From: Junio C Hamano \n\nThe hook to report \"garbage\" files in $GIT_OBJECT_DIRECTORY/pack/\ncould be generic but is too specific to count-object's needs.\n\nMove the part to produce human-readable messages to count-objects,\nand refine the interface to callback with the \"bits\" with values\ndefined in the cache.h header file, so that other callers (e.g.\nprune) can later use the same mechanism to enumerate different\nkinds of garbage files and do something intelligent about them,\nother than reporting in textual messages.\n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Doug Kelly \n---\n builtin/count-objects.c | 26 ++++++++++++++++++++++++--\n cache.h | 7 +++++--\n path.c | 2 +-\n sha1_file.c | 23 ++++++-----------------\n 4 files changed, 36 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex ad0c799..ba92919 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -15,9 +15,31 @@ static int verbose;\n static unsigned long loose, packed, packed_loose;\n static off_t loose_size;\n \n-static void real_report_garbage(const char *desc, const char *path)\n+static const char *bits_to_msg(unsigned seen_bits)\n+{\n+\tswitch (seen_bits) {\n+\tcase 0:\n+\t\treturn \"no corresponding .idx or .pack\";\n+\tcase PACKDIR_FILE_GARBAGE:\n+\t\treturn \"garbage found\";\n+\tcase PACKDIR_FILE_PACK:\n+\t\treturn \"no corresponding .idx\";\n+\tcase PACKDIR_FILE_IDX:\n+\t\treturn \"no corresponding .pack\";\n+\tcase PACKDIR_FILE_PACK|PACKDIR_FILE_IDX:\n+\tdefault:\n+\t\treturn NULL;\n+\t}\n+}\n+\n+static void real_report_garbage(unsigned seen_bits, const char *path)\n {\n \tstruct stat st;\n+\tconst char *desc = bits_to_msg(seen_bits);\n+\n+\tif (!desc)\n+\t\treturn;\n+\n \tif (!stat(path, &st))\n \t\tsize_garbage += st.st_size;\n \twarning(\"%s: %s\", desc, path);\n@@ -27,7 +49,7 @@ static void real_report_garbage(const char *desc, const char *path)\n static void loose_garbage(const char *path)\n {\n \tif (verbose)\n-\t\treport_garbage(\"garbage found\", path);\n+\t\treport_garbage(PACKDIR_FILE_GARBAGE, path);\n }\n \n static int count_loose(const unsigned char *sha1, const char *path, void *data)\ndiff --git a/cache.h b/cache.h\nindex 3ba0b8f..736abc0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1289,8 +1289,11 @@ struct pack_entry {\n \n extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n \n-/* A hook for count-objects to report invalid files in pack directory */\n-extern void (*report_garbage)(const char *desc, const char *path);\n+/* A hook to report invalid files in pack directory */\n+#define PACKDIR_FILE_PACK 1\n+#define PACKDIR_FILE_IDX 2\n+#define PACKDIR_FILE_GARBAGE 4\n+extern void (*report_garbage)(unsigned seen_bits, const char *path);\n \n extern void prepare_packed_git(void);\n extern void reprepare_packed_git(void);\ndiff --git a/path.c b/path.c\nindex c740c4f..f28ace2 100644\n--- a/path.c\n+++ b/path.c\n@@ -363,7 +363,7 @@ void report_linked_checkout_garbage(void)\n \t\tstrbuf_setlen(&sb, len);\n \t\tstrbuf_addstr(&sb, path);\n \t\tif (file_exists(sb.buf))\n-\t\t\treport_garbage(\"unused in linked checkout\", sb.buf);\n+\t\t\treport_garbage(PACKDIR_FILE_GARBAGE, sb.buf);\n \t}\n \tstrbuf_release(&sb);\n }\ndiff --git a/sha1_file.c b/sha1_file.c\nindex c5b31de..3d56746 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1217,27 +1217,16 @@ void install_packed_git(struct packed_git *pack)\n \tpacked_git = pack;\n }\n \n-void (*report_garbage)(const char *desc, const char *path);\n+void (*report_garbage)(unsigned seen_bits, const char *path);\n \n static void report_helper(const struct string_list *list,\n \t\t\t int seen_bits, int first, int last)\n {\n-\tconst char *msg;\n-\tswitch (seen_bits) {\n-\tcase 0:\n-\t\tmsg = \"no corresponding .idx or .pack\";\n-\t\tbreak;\n-\tcase 1:\n-\t\tmsg = \"no corresponding .idx\";\n-\t\tbreak;\n-\tcase 2:\n-\t\tmsg = \"no corresponding .pack\";\n-\t\tbreak;\n-\tdefault:\n+\tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX))\n \t\treturn;\n-\t}\n+\n \tfor (; first < last; first++)\n-\t\treport_garbage(msg, list->items[first].string);\n+\t\treport_garbage(seen_bits, list->items[first].string);\n }\n \n static void report_pack_garbage(struct string_list *list)\n@@ -1260,7 +1249,7 @@ static void report_pack_garbage(struct string_list *list)\n \t\tif (baselen == -1) {\n \t\t\tconst char *dot = strrchr(path, '.');\n \t\t\tif (!dot) {\n-\t\t\t\treport_garbage(\"garbage found\", path);\n+\t\t\t\treport_garbage(PACKDIR_FILE_GARBAGE, path);\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tbaselen = dot - path + 1;\n@@ -1332,7 +1321,7 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\t ends_with(de->d_name, \".keep\"))\n \t\t\tstring_list_append(&garbage, path.buf);\n \t\telse\n-\t\t\treport_garbage(\"garbage found\", path.buf);\n+\t\t\treport_garbage(PACKDIR_FILE_GARBAGE, path.buf);\n \t}\n \tclosedir(dir);\n \treport_pack_garbage(&garbage);\n-- \n2.5.1\n"},{"id":"272879","messageId":"1446606308-1668-2-git-send-email-dougk.ff7@gmail.com","threadId":"39907","inReplyTo":"1446606308-1668-1-git-send-email-dougk.ff7@gmail.com","subject":"[PATCH 2/3] t5304: Add test for cleaning pack garbage","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-11-04T03:05:07Z","receivedAt":"2015-11-04T03:05:07Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"Pack garbage, noticeably stale .idx files, can be cleaned up during\na garbage collection. This tests to ensure such garbage is properly\ncleaned up.\n\nNote that the prior test for checking pack garbage with count-objects\nleft some stale garbage after the test exited. This has also been\ncorrected.\n\nSigned-off-by: Doug Kelly \n---\n t/t5304-prune.sh | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 023d7c6..0297515 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -219,6 +219,7 @@ test_expect_success 'gc: prune old objects after local clone' '\n \n test_expect_success 'garbage report in count-objects -v' '\n \ttest_when_finished \"rm -f .git/objects/pack/fake*\" &&\n+\ttest_when_finished \"rm -f .git/objects/pack/foo*\" &&\n \t: >.git/objects/pack/foo &&\n \t: >.git/objects/pack/foo.bar &&\n \t: >.git/objects/pack/foo.keep &&\n@@ -244,6 +245,26 @@ EOF\n \ttest_cmp expected actual\n '\n \n+test_expect_failure 'clean pack garbage with gc' '\n+\ttest_when_finished \"rm -f .git/objects/pack/fake*\" &&\n+\ttest_when_finished \"rm -f .git/objects/pack/foo*\" &&\n+\t: >.git/objects/pack/foo.keep &&\n+\t: >.git/objects/pack/foo.pack &&\n+\t: >.git/objects/pack/fake.idx &&\n+\t: >.git/objects/pack/fake2.keep &&\n+\t: >.git/objects/pack/fake2.idx &&\n+\t: >.git/objects/pack/fake3.keep &&\n+\tgit gc &&\n+\tgit count-objects -v 2>stderr &&\n+\tgrep \"^warning:\" stderr | sort >actual &&\n+\tcat >expected <<\\EOF &&\n+warning: no corresponding .idx or .pack: .git/objects/pack/fake3.keep\n+warning: no corresponding .idx: .git/objects/pack/foo.keep\n+warning: no corresponding .idx: .git/objects/pack/foo.pack\n+EOF\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'prune .git/shallow' '\n \tSHA1=`echo hi|git commit-tree HEAD^{tree}` &&\n \techo $SHA1 >.git/shallow &&\n-- \n2.5.1\n"},{"id":"272880","messageId":"1446606308-1668-3-git-send-email-dougk.ff7@gmail.com","threadId":"39907","inReplyTo":"1446606308-1668-1-git-send-email-dougk.ff7@gmail.com","subject":"[PATCH 3/3] gc: Remove garbage .idx files from pack dir","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-11-04T03:05:08Z","receivedAt":"2015-11-04T03:05:08Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"Add a custom report_garbage handler to collect and remove garbage\n.idx files from the pack directory.\n\nSigned-off-by: Doug Kelly \n---\n builtin/gc.c | 20 ++++++++++++++++++++\n t/t5304-prune.sh | 2 +-\n 2 files changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex df3e454..668f975 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -45,6 +45,21 @@ static struct argv_array rerere = ARGV_ARRAY_INIT;\n \n static struct tempfile pidfile;\n static struct lock_file log_lock;\n+static struct string_list pack_garbage = STRING_LIST_INIT_DUP;\n+\n+static void clean_pack_garbage(void)\n+{\n+\tint i;\n+\tfor (i = 0; i < pack_garbage.nr; i++)\n+\t\tunlink_or_warn(pack_garbage.items[i].string);\n+\tstring_list_clear(&pack_garbage, 0);\n+}\n+\n+static void report_pack_garbage(unsigned seen_bits, const char *path)\n+{\n+\tif (seen_bits == PACKDIR_FILE_IDX)\n+\t\tstring_list_append(&pack_garbage, path);\n+}\n \n static void git_config_date_string(const char *key, const char **output)\n {\n@@ -416,6 +431,11 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tif (run_command_v_opt(rerere.argv, RUN_GIT_CMD))\n \t\treturn error(FAILED_RUN, rerere.argv[0]);\n \n+\treport_garbage = report_pack_garbage;\n+\treprepare_packed_git();\n+\tif (pack_garbage.nr > 0)\n+\t\tclean_pack_garbage();\n+\n \tif (auto_gc && too_many_loose_objects())\n \t\twarning(_(\"There are too many unreachable loose objects; \"\n \t\t\t\"run 'git prune' to remove them.\"));\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 0297515..def203c 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -245,7 +245,7 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'clean pack garbage with gc' '\n+test_expect_success 'clean pack garbage with gc' '\n \ttest_when_finished \"rm -f .git/objects/pack/fake*\" &&\n \ttest_when_finished \"rm -f .git/objects/pack/foo*\" &&\n \t: >.git/objects/pack/foo.keep &&\n-- \n2.5.1\n"},{"id":"272881","messageId":"CAEtYS8Q1T-ig2KqZUoCCODs1YbjOmF__vbiH5rL-s6hNaUhZeA@mail.gmail.com","threadId":"39907","inReplyTo":"CAEtYS8TR4mnaGpGDpB3cz_nu2hdCYTWf=PVCJbmzYi6YA53_bg@mail.gmail.com","subject":"Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-11-04T03:12:38Z","receivedAt":"2015-11-04T03:12:38Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"On Wed, Oct 28, 2015 at 5:43 PM, Doug Kelly wrote:\n> On Wed, Oct 28, 2015 at 12:48 PM, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>\n>>> Eric Sunshine writes:\n>>>\n>>>>> -static void real_report_garbage(const char *desc, const char *path)\n>>>>> +const char *bits_to_msg(unsigned seen_bits)\n>>>>\n>>>> If you don't expect other callers outside this file, then this should\n>>>> be declared 'static'. If you do expect future external callers, then\n>>>> this should be declared in a public header file (but renamed to be\n>>>> more meaningful).\n>>>\n>>> I think this can be private to this file. The sole point of moving\n>>> this logic to this file is to make it private, after all ;-) Thanks\n>>> for sharp eyes.\n>>>\n>>> Together with the need for a description on \"why\", this probably\n>>> deserves a test or two, probably at the end of t5304.\n>>>\n>>> Thanks.\n>>\n>> Does somebody want to help tying the final loose ends on this topic?\n>> It has been listed in the [Stalled] section for too long, I _think_\n>> what it attempts to do is a worthy thing, and it is shame to see the\n>> initial implementation and review cycles we have spent so far go to\n>> waste.\n>>\n>> If I find nothing else to do before any taker appears, I could\n>> volunteer myself, but thought I should ask first.\n>>\n>> Thanks.\n>\n> I agree; I've been wanting to get back to it, but had some\n> higher-priority things at work for a while, so I've not had time. I'd\n> be happy to get back into it, but if you get to it first, believe me,\n> I'm not going to be offended. :)\n>\n> I'll see if I can't devote a little extra time to it this upcoming\n> week, though. Hopefully it doesn't need too much additional polishing\n> to be ready.\n>\n> P.S. Does a Googler want to tell the Inbox team that the inability to\n> send plain-text email is really annoying? :P\n\nI think the patches I sent (a bit prematurely) address the remaining\ncomments... I did find there was a relevant test in t5304 already, so\nI added a new test in the same section (and cleaned up some of the\ngarbage it wasn't removing before). I'm not sure if it's poor form to\nmove tests around like this, but I figured it might be best to keep\nthem logically grouped.\n\nLet me know if there's anything I can do, and once again, sorry for the delay!\n"},{"id":"272900","messageId":"xmqqr3k5a76v.fsf@gitster.mtv.corp.google.com","threadId":"39907","inReplyTo":"CAEtYS8Q1T-ig2KqZUoCCODs1YbjOmF__vbiH5rL-s6hNaUhZeA@mail.gmail.com","subject":"Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-04T19:35:52Z","receivedAt":"2015-11-04T19:35:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Doug Kelly writes:\n\n> I think the patches I sent (a bit prematurely) address the\n> remaining comments... I did find there was a relevant test in\n> t5304 already, so I added a new test in the same section (and\n> cleaned up some of the garbage it wasn't removing before). I'm\n> not sure if it's poor form to move tests around like this, but I\n> figured it might be best to keep them logically grouped.\n\nOK, will queue as I didn't spot anything glaringly wrong ;-)\n\nI did wonder if we want to say anything about .bitmap files, though.\nIf there is one without matching .idx and .pack, shouldn't we report\njust like we report .idx without .pack (or vice versa)?\n\nThanks.\n"},{"id":"272907","messageId":"CAEtYS8Rp0Eb7uHB8kJ=muVWy6u+beB7kAAWZqPgTYqfuKx3P2A@mail.gmail.com","threadId":"39907","inReplyTo":"xmqqr3k5a76v.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-11-04T19:56:38Z","receivedAt":"2015-11-04T19:56:38Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"On Wed, Nov 4, 2015 at 1:35 PM, Junio C Hamano wrote:\n> Doug Kelly writes:\n>\n>> I think the patches I sent (a bit prematurely) address the\n>> remaining comments... I did find there was a relevant test in\n>> t5304 already, so I added a new test in the same section (and\n>> cleaned up some of the garbage it wasn't removing before). I'm\n>> not sure if it's poor form to move tests around like this, but I\n>> figured it might be best to keep them logically grouped.\n>\n> OK, will queue as I didn't spot anything glaringly wrong ;-)\n>\n> I did wonder if we want to say anything about .bitmap files, though.\n> If there is one without matching .idx and .pack, shouldn't we report\n> just like we report .idx without .pack (or vice versa)?\n>\n> Thanks.\n\nI think you're right -- this would be something worth following up on.\nAt least, t5304 doesn't cover this case explicitly, but when I tried\nadding an empty bitmap with a bogus name, I did see a \"no\ncorresponding .idx or .pack\" error, similar to the stale .keep file.\n\nI'd trust your (and Jeff's) knowledge on this far more than my own,\nbut would it be a bad idea to clean up .keep and .bitmap files if the\n.idx/.pack pair are missing? I think we may have had a discussion\npreviously on how things along these lines might be racey -- but I\ndon't know what order the .keep file is created in relation to the\n.idx/.pack.\n"},{"id":"272908","messageId":"20151104195649.GB16101@sigill.intra.peff.net","threadId":"39907","inReplyTo":"xmqqr3k5a76v.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-04T19:56:49Z","receivedAt":"2015-11-04T19:56:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 04, 2015 at 11:35:52AM -0800, Junio C Hamano wrote:\n\n> Doug Kelly writes:\n> \n> > I think the patches I sent (a bit prematurely) address the\n> > remaining comments... I did find there was a relevant test in\n> > t5304 already, so I added a new test in the same section (and\n> > cleaned up some of the garbage it wasn't removing before). I'm\n> > not sure if it's poor form to move tests around like this, but I\n> > figured it might be best to keep them logically grouped.\n> \n> OK, will queue as I didn't spot anything glaringly wrong ;-)\n> \n> I did wonder if we want to say anything about .bitmap files, though.\n> If there is one without matching .idx and .pack, shouldn't we report\n> just like we report .idx without .pack (or vice versa)?\n\nYeah, I think so. The logic should really extend to anything without a\nmatching .pack. And I think the sane rule is probably:\n\n If we have pack-$sha.$ext, but not pack-$sha.pack, then:\n\n 1. if $ext is known to us as a cache that can be regenerated from the\n .pack (i.e., .idx, .bitmap), then delete it\n\n 2. if $ext is known to us as precious, do nothing (there is nothing\n in this category right now, though)\n\n 3. if $ext is not known to us, warn but do not delete (in case a\n future version adds something precious)\n\nThe conservatism in (3) is the right thing to do, I think, but I doubt\nit will ever matter, because we probably cannot ever add non-cache\nauxiliary files to the pack. Old versions of git would not delete such\nprecious files, but nor would they carry them forward during a repack.\nSo short of a repo-version bump, I think we are effectively limited to\nadding only caches which can be re-generated from an original .pack.\n\n-Peff\n"},{"id":"272910","messageId":"20151104200249.GC16101@sigill.intra.peff.net","threadId":"39907","inReplyTo":"CAEtYS8Rp0Eb7uHB8kJ=muVWy6u+beB7kAAWZqPgTYqfuKx3P2A@mail.gmail.com","subject":"Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-04T20:02:49Z","receivedAt":"2015-11-04T20:02:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 04, 2015 at 01:56:38PM -0600, Doug Kelly wrote:\n\n> > I did wonder if we want to say anything about .bitmap files, though.\n> > If there is one without matching .idx and .pack, shouldn't we report\n> > just like we report .idx without .pack (or vice versa)?\n> \n> I think you're right -- this would be something worth following up on.\n> At least, t5304 doesn't cover this case explicitly, but when I tried\n> adding an empty bitmap with a bogus name, I did see a \"no\n> corresponding .idx or .pack\" error, similar to the stale .keep file.\n\nYeah, that should be harmless warning (although note because the bitmap\ncode only really handles a single bitmap, it can prevent loading of the\n\"real\" bitmap; so you'd want to clean it up, for sure).\n\n> I'd trust your (and Jeff's) knowledge on this far more than my own,\n> but would it be a bad idea to clean up .keep and .bitmap files if the\n> .idx/.pack pair are missing? I think we may have had a discussion\n> previously on how things along these lines might be racey -- but I\n> don't know what order the .keep file is created in relation to the\n> .idx/.pack.\n\nDefinitely cleaning up the .bitmap is sane and not racy (it's in the\nsame boat as the .idx, I think).\n\n.keep files are more tricky. I'd have to go over the receive-pack code\nto confirm, but I think they _are_ racy. That is, receive-pack will\ncreate them as a lockfile before moving the pack into place. That's OK,\nthough, if we use mtimes to give ourselves a grace period (I haven't\nlooked at your series yet).\n\nBut moreover, .keep files can be created manually by the user. If the\npack they referenced goes away, they are not really serving any purpose.\nBut it's possible that the user would want to salvage the content of the\nfile, or know that it was there.\n\nSo I'd argue we should leave them. Or at least leave ones that do not\nhave the generic \"{receive,fetch}-pack $pid on $host comment in them,\nwhich were clearly created as lockfiles.\n\n-Peff\n"},{"id":"272912","messageId":"CAEtYS8S_ys3jT5ziWd7_u6Dn8b3LwnZYO7Pz6EegsmWpUM5riw@mail.gmail.com","threadId":"39907","inReplyTo":"20151104200249.GC16101@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-11-04T20:08:21Z","receivedAt":"2015-11-04T20:08:21Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"On Wed, Nov 4, 2015 at 2:02 PM, Jeff King wrote:\n> On Wed, Nov 04, 2015 at 01:56:38PM -0600, Doug Kelly wrote:\n>\n>> > I did wonder if we want to say anything about .bitmap files, though.\n>> > If there is one without matching .idx and .pack, shouldn't we report\n>> > just like we report .idx without .pack (or vice versa)?\n>>\n>> I think you're right -- this would be something worth following up on.\n>> At least, t5304 doesn't cover this case explicitly, but when I tried\n>> adding an empty bitmap with a bogus name, I did see a \"no\n>> corresponding .idx or .pack\" error, similar to the stale .keep file.\n>\n> Yeah, that should be harmless warning (although note because the bitmap\n> code only really handles a single bitmap, it can prevent loading of the\n> \"real\" bitmap; so you'd want to clean it up, for sure).\n>\n>> I'd trust your (and Jeff's) knowledge on this far more than my own,\n>> but would it be a bad idea to clean up .keep and .bitmap files if the\n>> .idx/.pack pair are missing? I think we may have had a discussion\n>> previously on how things along these lines might be racey -- but I\n>> don't know what order the .keep file is created in relation to the\n>> .idx/.pack.\n>\n> Definitely cleaning up the .bitmap is sane and not racy (it's in the\n> same boat as the .idx, I think).\n>\n> .keep files are more tricky. I'd have to go over the receive-pack code\n> to confirm, but I think they _are_ racy. That is, receive-pack will\n> create them as a lockfile before moving the pack into place. That's OK,\n> though, if we use mtimes to give ourselves a grace period (I haven't\n> looked at your series yet).\n>\n> But moreover, .keep files can be created manually by the user. If the\n> pack they referenced goes away, they are not really serving any purpose.\n> But it's possible that the user would want to salvage the content of the\n> file, or know that it was there.\n>\n> So I'd argue we should leave them. Or at least leave ones that do not\n> have the generic \"{receive,fetch}-pack $pid on $host comment in them,\n> which were clearly created as lockfiles.\n>\n> -Peff\n\nCurrently there's no mtime-guarding logic (I dug up that conversation\nearlier, though, but after I'd done the respin on this series)... OK,\nin that case, I'll create a separate patch that tests/cleans up\n.bitmap, but doesn't touch .keep. This might be a small series since\nI think the logic for finding pack garbage doesn't know anything about\n.bitmap per-se, so it's looking like I'll extend that relevant code,\nbefore adding the handling in gc and appropriate tests.\n"},{"id":"272914","messageId":"20151104201521.GD16101@sigill.intra.peff.net","threadId":"39907","inReplyTo":"CAEtYS8S_ys3jT5ziWd7_u6Dn8b3LwnZYO7Pz6EegsmWpUM5riw@mail.gmail.com","subject":"Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-04T20:15:22Z","receivedAt":"2015-11-04T20:15:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 04, 2015 at 02:08:21PM -0600, Doug Kelly wrote:\n\n> Currently there's no mtime-guarding logic (I dug up that conversation\n> earlier, though, but after I'd done the respin on this series)... OK,\n> in that case, I'll create a separate patch that tests/cleans up\n> .bitmap, but doesn't touch .keep. This might be a small series since\n> I think the logic for finding pack garbage doesn't know anything about\n> .bitmap per-se, so it's looking like I'll extend that relevant code,\n> before adding the handling in gc and appropriate tests.\n\nI'd hoped you could reuse the list of extensions found in\nbuiltin/repack.c (e.g., see remove_redundant_pack). But I guess that is\nnot connected with the garbage-reporting code. And anyway, the simple\nlist probably does not carry sufficient information (it does not know\nthat \".keep\" is potentially more precious than \".idx\", for example).\n\n-Peff\n"},{"id":"273195","messageId":"CABYiQpmYP=x-Urbwd0e_aa=iAMM4wP2bvdXwDN0=htEr5iOZAw@mail.gmail.com","threadId":"39907","inReplyTo":"CABYiQpn7r2Vcf=S5RaWHBN85eBYGPV_e02+BY=4L98qfUzDT1Q@mail.gmail.com","subject":"Fwd: Question: .idx without .pack causes performance issues?","fromName":"Thomas Berg","fromEmail":"merlin66b@gmail.com","sentAt":"2015-11-11T14:58:09Z","receivedAt":"2015-11-11T14:58:09Z","isPatch":false,"sender":{"key":"merlin66b@gmail.com","avatar":null},"body":"Hi all,\n\n(re-sending because my first e-mail was rejected due to html formatting)\n\nWhile debugging a git fetch performance problem on Windows I came\nacross this thread. The problem in our case was also caused by\norphaned .idx files.\n\nOn Tue, Jul 21, 2015 at 9:15 PM, Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > I however do not think that we mark the in-core structure that\n> > corresponds to an open \".idx\" file in any way when such a failure\n> > happens. If we really cared enough, we could do so, saying \"we know\n> > there is .idx file, but do not bother looking at it again, as we\n> > know the corresponding .pack is missing\", and that would speed things\n> > up a bit, essentially bringing us back to a sane situation without\n> > any \".idx\" without corresponding \".pack\".\n> >\n> > I do not think it is worth the effort, though. It would be more\n> > fruitful to find out how you end up with \".idx exists but not\n> > corresponding .pack\" and if that is some systemic failure, see if\n> > there is a way to prevent that from happening in the first place.\n>\n> While I still think that it is more important to prevent such a\n> situation from occurring in the first place, ignoring .idx that lack\n> corresponding .pack should be fairly simple, perhaps like this.\n\nI have observed the following: if garbage collection is triggered\nduring a git fetch, I always get messages like this:\n\n$ git fetch origin\n> Auto packing the repository for optimum performance. You may also\n> run \"git gc\" manually. See \"git help gc\" for more information.\n> Counting objects: 396468, done.\n> Delta compression using up to 12 threads.\n> Compressing objects: 100% (98683/98683), done.\n> Writing objects: 100% (396468/396468), done.\n> Total 396468 (delta 289422), reused 395212 (delta 288289)\n> Unlink of file '.git/objects/pack/pack-343b6cfdf58171f53c235b900a75d09bd9219e06.pack' failed. Should I try again? (y/n) n\n> Unlink of file '.git/objects/pack/pack-343b6cfdf58171f53c235b900a75d09bd9219e06.idx' failed. Should I try again? (y/n) n\n> Unlink of file '.git/objects/pack/pack-63a6cb5e2a9f72eea72b02ac74a167e1d71d417f.idx' failed. Should I try again? (y/n) n\n> Unlink of file '.git/objects/pack/pack-9b616a2501bb9c13acecf3e981c39868dd2f5ff7.pack' failed. Should I try again? (y/n) n\n> Unlink of file '.git/objects/pack/pack-9b616a2501bb9c13acecf3e981c39868dd2f5ff7.idx' failed. Should I try again? (y/n) n\n> Checking connectivity: 396468, done.\n\nWindows has the property that if a file is open it can't be deleted.\nIf so, it could be that git fetch needs to close the files first. I\ncan't remember observing this problem when running git gc by itself.\n\nIn the repos where we have problems I observed both unnecessary .pack\nfiles and .idx files, but way more .idx files. Maybe, over time,\nunnecessary pack files have been cleaned up but not .idx files?\n\nIf so, this would explain how we get into this situation. I have been\ntesting this with very old git versions on Windows (1.7.4 and 1.8.4),\nsorry if these problems are already fixed in later versions.\n\n- Thomas\n"},{"id":"275170","messageId":"20151230073759.GA785@sigill.intra.peff.net","threadId":"39907","inReplyTo":"CAEtYS8S_ys3jT5ziWd7_u6Dn8b3LwnZYO7Pz6EegsmWpUM5riw@mail.gmail.com","subject":"Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-30T07:37:59Z","receivedAt":"2015-12-30T07:37:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 04, 2015 at 02:08:21PM -0600, Doug Kelly wrote:\n\n> On Wed, Nov 4, 2015 at 2:02 PM, Jeff King wrote:\n> > Definitely cleaning up the .bitmap is sane and not racy (it's in the\n> > same boat as the .idx, I think).\n> >\n> > .keep files are more tricky. I'd have to go over the receive-pack code\n> > to confirm, but I think they _are_ racy. That is, receive-pack will\n> > create them as a lockfile before moving the pack into place. That's OK,\n> > though, if we use mtimes to give ourselves a grace period (I haven't\n> > looked at your series yet).\n> >\n> > But moreover, .keep files can be created manually by the user. If the\n> > pack they referenced goes away, they are not really serving any purpose.\n> > But it's possible that the user would want to salvage the content of the\n> > file, or know that it was there.\n> >\n> > So I'd argue we should leave them. Or at least leave ones that do not\n> > have the generic \"{receive,fetch}-pack $pid on $host comment in them,\n> > which were clearly created as lockfiles.\n> \n> Currently there's no mtime-guarding logic (I dug up that conversation\n> earlier, though, but after I'd done the respin on this series)... OK,\n> in that case, I'll create a separate patch that tests/cleans up\n> .bitmap, but doesn't touch .keep. This might be a small series since\n> I think the logic for finding pack garbage doesn't know anything about\n> .bitmap per-se, so it's looking like I'll extend that relevant code,\n> before adding the handling in gc and appropriate tests.\n\nI happened to be looking over your series again, and I noticed that we\ndidn't end up with any mtime logic at all in what got merged.\n\nI _think_ that is probably OK, because we always write the pack,\nfollowed by the .idx, followed by the .bitmap (if any). And we don't\ndrop .keep files (though I think we would perhaps note them as possible\ncruft?).\n\nSo I don't think there are any races introduced here, but I wonder if we\nwant to be a bit more conservative. Sorry to bring this up so much after\nthe fact; I completely forgot about it when reviewing the patches.\n\nThese changes are slated for the v2.7 release. Like I said, I don't\nthink it's buggy, so we don't necessarily need to address it before the\nrelease. We could add an mtime check in the next cycle as a\nbelt-and-suspenders safety, rather than a fix.\n\n-Peff\n"},{"id":"275933","messageId":"CAEtYS8Qs2B3rP1PDGhoWGAgcj2c_pOTpt=s8qj9tWMjkLLFyhQ@mail.gmail.com","threadId":"39907","inReplyTo":"20151230073759.GA785@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2016-01-13T17:14:39Z","receivedAt":"2016-01-13T17:14:39Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"On Wed, Dec 30, 2015 at 1:37 AM, Jeff King wrote:\n> On Wed, Nov 04, 2015 at 02:08:21PM -0600, Doug Kelly wrote:\n>\n>> On Wed, Nov 4, 2015 at 2:02 PM, Jeff King wrote:\n>> > Definitely cleaning up the .bitmap is sane and not racy (it's in the\n>> > same boat as the .idx, I think).\n>> >\n>> > .keep files are more tricky. I'd have to go over the receive-pack code\n>> > to confirm, but I think they _are_ racy. That is, receive-pack will\n>> > create them as a lockfile before moving the pack into place. That's OK,\n>> > though, if we use mtimes to give ourselves a grace period (I haven't\n>> > looked at your series yet).\n>> >\n>> > But moreover, .keep files can be created manually by the user. If the\n>> > pack they referenced goes away, they are not really serving any purpose.\n>> > But it's possible that the user would want to salvage the content of the\n>> > file, or know that it was there.\n>> >\n>> > So I'd argue we should leave them. Or at least leave ones that do not\n>> > have the generic \"{receive,fetch}-pack $pid on $host comment in them,\n>> > which were clearly created as lockfiles.\n>>\n>> Currently there's no mtime-guarding logic (I dug up that conversation\n>> earlier, though, but after I'd done the respin on this series)... OK,\n>> in that case, I'll create a separate patch that tests/cleans up\n>> .bitmap, but doesn't touch .keep. This might be a small series since\n>> I think the logic for finding pack garbage doesn't know anything about\n>> .bitmap per-se, so it's looking like I'll extend that relevant code,\n>> before adding the handling in gc and appropriate tests.\n>\n> I happened to be looking over your series again, and I noticed that we\n> didn't end up with any mtime logic at all in what got merged.\n>\n> I _think_ that is probably OK, because we always write the pack,\n> followed by the .idx, followed by the .bitmap (if any). And we don't\n> drop .keep files (though I think we would perhaps note them as possible\n> cruft?).\n>\n> So I don't think there are any races introduced here, but I wonder if we\n> want to be a bit more conservative. Sorry to bring this up so much after\n> the fact; I completely forgot about it when reviewing the patches.\n>\n> These changes are slated for the v2.7 release. Like I said, I don't\n> think it's buggy, so we don't necessarily need to address it before the\n> release. We could add an mtime check in the next cycle as a\n> belt-and-suspenders safety, rather than a fix.\n>\n> -Peff\n\nYeah, I know I never got to adding the mtime logic, but for a simple (naive,\nhard-coded) case, I did come up with a basic patch today. I think this could\nbe extended to a configuration option(?) which would allow a default longer\nthan 10 seconds (an hour? a day?), then during the regression tests, we\ncould provide a shorter timeout to ensure the guarding both works and also\nnot wait forever for tests to complete. Thoughts?\n\n---\n builtin/gc.c | 14 ++++++++++++--\n t/t5304-prune.sh | 2 ++\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 79e9886..a4ce616 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -51,8 +51,18 @@ static struct string_list pack_garbage =\nSTRING_LIST_INIT_DUP;\n static void clean_pack_garbage(void)\n {\n int i;\n- for (i = 0; i < pack_garbage.nr; i++)\n- unlink_or_warn(pack_garbage.items[i].string);\n+ /* Define a cutoff time for \"new\" garbage to prevent race conditions */\n+ time_t cutoff = time(NULL) - 10;\n+ for (i = 0; i < pack_garbage.nr; i++) {\n+ struct stat s;\n+ char *garbage = pack_garbage.items[i].string;\n+ if (!stat(garbage, &s)) {\n+ if (s.st_mtime < cutoff)\n+ unlink_or_warn(garbage);\n+ } else\n+ fprintf(stderr, _(\"stat failed on pack garbage: %s\"),\n+ garbage);\n+ }\n string_list_clear(&pack_garbage, 0);\n }\n\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex cbcc0c0..7b4650f 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -272,6 +272,7 @@ test_expect_success 'clean pack garbage with gc' '\n : >.git/objects/pack/fake6.keep &&\n : >.git/objects/pack/fake6.bitmap &&\n : >.git/objects/pack/fake6.idx &&\n+ sleep 10 &&\n git gc &&\n git count-objects -v 2>stderr &&\n grep \"^warning:\" stderr | sort >actual &&\n@@ -291,6 +292,7 @@ test_expect_success 'ensure unknown garbage kept with gc' '\n : >.git/objects/pack/foo.keep &&\n : >.git/objects/pack/fake.pack &&\n : >.git/objects/pack/fake2.foo &&\n+ sleep 10 &&\n git gc &&\n git count-objects -v 2>stderr &&\n grep \"^warning:\" stderr | sort >actual &&\n-- \n2.6.1\n"},{"id":"275968","messageId":"xmqqvb6xmedw.fsf@gitster.mtv.corp.google.com","threadId":"39907","inReplyTo":"CAEtYS8Qs2B3rP1PDGhoWGAgcj2c_pOTpt=s8qj9tWMjkLLFyhQ@mail.gmail.com","subject":"Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-13T20:08:11Z","receivedAt":"2016-01-13T20:08:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Doug Kelly writes:\n\n> Yeah, I know I never got to adding the mtime logic, but for a simple (naive,\n> hard-coded) case, I did come up with a basic patch today. I think this could\n> be extended to a configuration option(?) which would allow a default longer\n> than 10 seconds (an hour? a day?), then during the regression tests, we\n> could provide a shorter timeout to ensure the guarding both works and also\n> not wait forever for tests to complete. Thoughts?\n\nPlease do not sleep in the tests. Instead, please try to see if you\ncan use test-chmtime to set the timestamps of these files to the\nnecessary ages for the purpose of your tests.\n\nThanks.\n\n>\n> ---\n> builtin/gc.c | 14 ++++++++++++--\n> t/t5304-prune.sh | 2 ++\n> 2 files changed, 14 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 79e9886..a4ce616 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -51,8 +51,18 @@ static struct string_list pack_garbage =\n> STRING_LIST_INIT_DUP;\n> static void clean_pack_garbage(void)\n> {\n> int i;\n> - for (i = 0; i < pack_garbage.nr; i++)\n> - unlink_or_warn(pack_garbage.items[i].string);\n> + /* Define a cutoff time for \"new\" garbage to prevent race conditions */\n> + time_t cutoff = time(NULL) - 10;\n> + for (i = 0; i < pack_garbage.nr; i++) {\n> + struct stat s;\n> + char *garbage = pack_garbage.items[i].string;\n> + if (!stat(garbage, &s)) {\n> + if (s.st_mtime < cutoff)\n> + unlink_or_warn(garbage);\n> + } else\n> + fprintf(stderr, _(\"stat failed on pack garbage: %s\"),\n> + garbage);\n> + }\n> string_list_clear(&pack_garbage, 0);\n> }\n>\n> diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\n> index cbcc0c0..7b4650f 100755\n> --- a/t/t5304-prune.sh\n> +++ b/t/t5304-prune.sh\n> @@ -272,6 +272,7 @@ test_expect_success 'clean pack garbage with gc' '\n> : >.git/objects/pack/fake6.keep &&\n> : >.git/objects/pack/fake6.bitmap &&\n> : >.git/objects/pack/fake6.idx &&\n> + sleep 10 &&\n> git gc &&\n> git count-objects -v 2>stderr &&\n> grep \"^warning:\" stderr | sort >actual &&\n> @@ -291,6 +292,7 @@ test_expect_success 'ensure unknown garbage kept with gc' '\n> : >.git/objects/pack/foo.keep &&\n> : >.git/objects/pack/fake.pack &&\n> : >.git/objects/pack/fake2.foo &&\n> + sleep 10 &&\n> git gc &&\n> git count-objects -v 2>stderr &&\n> grep \"^warning:\" stderr | sort >actual &&\n"},{"id":"275971","messageId":"CAEtYS8RA2HONEARQ03EiT8Ch+F2am+YJuRz2hNiSGO25N8Lf4g@mail.gmail.com","threadId":"39907","inReplyTo":"xmqqvb6xmedw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2016-01-13T20:19:05Z","receivedAt":"2016-01-13T20:19:05Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"On Wed, Jan 13, 2016 at 2:08 PM, Junio C Hamano wrote:\n> Doug Kelly writes:\n>\n>> Yeah, I know I never got to adding the mtime logic, but for a simple (naive,\n>> hard-coded) case, I did come up with a basic patch today. I think this could\n>> be extended to a configuration option(?) which would allow a default longer\n>> than 10 seconds (an hour? a day?), then during the regression tests, we\n>> could provide a shorter timeout to ensure the guarding both works and also\n>> not wait forever for tests to complete. Thoughts?\n>\n> Please do not sleep in the tests. Instead, please try to see if you\n> can use test-chmtime to set the timestamps of these files to the\n> necessary ages for the purpose of your tests.\n>\n> Thanks.\n\nAh, thanks -- I actually didn't know about that. I didn't intend this\nto be a formal\npatch (although I'm sorry the mail client clobbered all the\nwhitespace), but I will\nkeep this in mind for when I continue to refine this change.\n\nHowever, back to the point: should the wait value be hard coded? Configurable\nas a new option? What should our default wait be?\n\nThanks!\n"},{"id":"275972","messageId":"20160113202329.GA7678@sigill.intra.peff.net","threadId":"39907","inReplyTo":"CAEtYS8RA2HONEARQ03EiT8Ch+F2am+YJuRz2hNiSGO25N8Lf4g@mail.gmail.com","subject":"Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-13T20:23:29Z","receivedAt":"2016-01-13T20:23:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 13, 2016 at 02:19:05PM -0600, Doug Kelly wrote:\n\n> However, back to the point: should the wait value be hard coded? Configurable\n> as a new option? What should our default wait be?\n\nI'd think it would make sense to match the expiration time we feed to\nprune. That's overly conservative, but I think that's OK (and the user\ncan tweak it down).\n\n-Peff\n"}]}