{"thread":{"id":"11902","subject":"Re: [PATCH] Make git prune remove temporary packs that look like write failures","startedAt":"2008-02-05T18:49:31Z","lastAt":"2008-02-06T20:43:37Z","messageCount":20,"participants":["Nicolas Pitre","David Steven Tweed","Johannes Schindelin","Junio C Hamano","Brandon Casey","David Tweed"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"67539","messageId":"Pine.GSO.4.63.0802051844220.15867@suma3","threadId":"11902","inReplyTo":null,"subject":"[PATCH] Make git prune remove temporary packs that look like write failures","fromName":"David Steven Tweed","fromEmail":"d.s.tweed@reading.ac.uk","sentAt":"2008-02-05T18:49:31Z","receivedAt":"2008-02-05T18:49:31Z","isPatch":true,"sender":{"key":"d.s.tweed@reading.ac.uk","avatar":null},"body":"Write errors when repacking (eg, due to out-of-space conditions)\ncan leave temporary packs (and possibly other files beginning\nwith \"tmp_\") lying around which no existing\ncodepath removes and which aren't obvious to the casual user.\nThese can also be multi-megabyte files wasting noticeable space.\nUnfortunately there's no way to definitely tell in builtin-prune\nthat a tmp_ file is not being used by a concurrent process.\nHowever, it is documented that pruning should only be done\non a quiet repository. The names of removed files are printed.\n\nSigned-off-by: David Tweed (david.tweed@gmail.com)\n---\n\nPer discussion of previous version, this now unconditionally\nremoves any tmp_ file existing when prune is run.\n\n builtin-prune.c | 25 +++++++++++++++++++++++++\n 1 files changed, 25 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-prune.c b/builtin-prune.c\nindex b5e7684..9db3cf0 100644\n--- a/builtin-prune.c\n+++ b/builtin-prune.c\n@@ -83,6 +83,30 @@ static void prune_object_dir(const char *path)\n \t}\n }\n\n+/*\n+ * Write errors (particularly out of space) can result in\n+ * failed temporary packs (and more rarely indexes and other\n+ * files begining with \"tmp_\") accumulating in the\n+ * object directory.\n+ */\n+static void remove_temporary_files(void)\n+{\n+\tDIR *dir;\n+\tstruct dirent *de;\n+\tchar* dirname=get_object_directory();\n+\n+\tdir = opendir(dirname);\n+\twhile ((de = readdir(dir)) != NULL) {\n+\t\tif (strncmp(de->d_name, \"tmp_\", 4) == 0) {\n+\t\t\tchar name[4096];\n+\t\t\tsprintf(name, \"%s/%s\", dirname, de->d_name);\n+\t\t\tprintf(\"Removing abandoned pack %s\\n\", name);\n+\t\t\tunlink(name);\n+\t\t}\n+\t}\n+\tclosedir(dir);\n+}\n+\n int cmd_prune(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n@@ -115,5 +139,6 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n\n \tsync();\n \tprune_packed_objects(show_only);\n+\tremove_temporary_files();\n \treturn 0;\n }\n-- \n1.5.4.19.g40d1a-dirty\n"},{"id":"67536","messageId":"alpine.LFD.1.00.0802051357420.2732@xanadu.home","threadId":"11902","inReplyTo":"Pine.GSO.4.63.0802051844220.15867@suma3","subject":"Re: [PATCH] Make git prune remove temporary packs that look like write failures","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-02-05T19:02:26Z","receivedAt":"2008-02-05T19:02:26Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 5 Feb 2008, David Steven Tweed wrote:\n\n> @@ -115,5 +139,6 @@ int cmd_prune(int argc, const char **argv, const char\n> *prefix)\n> \n> \tsync();\n> \tprune_packed_objects(show_only);\n> +\tremove_temporary_files();\n\nMaybe you could implement the \"show_only\" mode for \nremove_temporary_files() as well? Otherwise the -n option would not be \nrespected.\n\nAlso you should consider honoring the --expire option as well.\n\n\nNicolas\n"},{"id":"67544","messageId":"alpine.LSU.1.00.0802052005200.8543@racer.site","threadId":"11902","inReplyTo":"alpine.LFD.1.00.0802051357420.2732@xanadu.home","subject":"[PATCH] prune: heed --expire for stale packs, add a test","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-05T20:06:41Z","receivedAt":"2008-02-05T20:06:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nFollow the same logic as for loose objects when removing stale packs: they\nmight be in use (for example when fetching, or repacking in a cron job),\nso give the user a chance to say (via --expire) what is considered too\nyoung an age to die for stale packs.\n\nAlso add a simple test to verify that the stale packs are actually\nexpired.\n\nSigned-off-by: Johannes Schindelin \n---\n\n\n\tOn Tue, 5 Feb 2008, Nicolas Pitre wrote:\n\n\t> On Tue, 5 Feb 2008, David Steven Tweed wrote:\n\t> \n\t> > @@ -115,5 +139,6 @@ int cmd_prune(int argc, const char **argv, \n\t> > const char *prefix)\n\t> > \n\t> > \tsync();\n\t> > \tprune_packed_objects(show_only);\n\t> > +\tremove_temporary_files();\n\t> \n\t> Maybe you could implement the \"show_only\" mode for \n\t> remove_temporary_files() as well? Otherwise the -n option would \n\t> not be respected.\n\t> \n\t> Also you should consider honoring the --expire option as well.\n\n\tHow about this on top of David's patch?\n\n builtin-prune.c | 9 ++++++++-\n t/t5304-prune.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 40 insertions(+), 1 deletions(-)\n create mode 100644 t/t5304-prune.sh\n\ndiff --git a/builtin-prune.c b/builtin-prune.c\nindex 9152984..d5a3b60 100644\n--- a/builtin-prune.c\n+++ b/builtin-prune.c\n@@ -100,7 +100,14 @@ static void remove_temporary_files(void)\n \t\tif (strncmp(de->d_name, \"tmp_\", 4) == 0) {\n \t\t\tchar name[4096];\n \t\t\tsprintf(name, \"%s/%s\", dirname, de->d_name);\n-\t\t\tprintf(\"Removing abandoned pack %s\\n\", name);\n+\t\t\tif (expire) {\n+\t\t\t\tstruct stat st;\n+\t\t\t\tif (stat(name, &st) || st.st_mtime >= expire)\n+\t\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tprintf(\"Removing stale pack %s\\n\", name);\n+\t\t\tif (show_only)\n+\t\t\t\tcontinue;\n \t\t\tunlink(name);\n \t\t}\n \t}\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nnew file mode 100644\nindex 0000000..6560af7\n--- /dev/null\n+++ b/t/t5304-prune.sh\n@@ -0,0 +1,32 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2008 Johannes E. Schindelin\n+#\n+\n+test_description='prune'\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\n+\t: > file &&\n+\tgit add file &&\n+\ttest_tick &&\n+\tgit commit -m initial &&\n+\tgit gc\n+\n+'\n+\n+test_expect_success 'prune stale packs' '\n+\n+\torig_pack=$(echo .git/objects/pack/*.pack) &&\n+\t: > .git/objects/tmp_1.pack &&\n+\t: > .git/objects/tmp_2.pack &&\n+\ttest-chmtime -86501 .git/objects/tmp_1.pack &&\n+\tgit prune --expire 1.day &&\n+\ttest -f $orig_pack &&\n+\ttest -f .git/objects/tmp_2.pack &&\n+\t! test -f .git/objects/tmp_1.pack\n+\n+'\n+\n+test_done\n-- \n1.5.4.1230.g4ecf8\n"},{"id":"67545","messageId":"alpine.LFD.1.00.0802051512370.2732@xanadu.home","threadId":"11902","inReplyTo":"alpine.LSU.1.00.0802052005200.8543@racer.site","subject":"Re: [PATCH] prune: heed --expire for stale packs, add a test","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-02-05T20:13:27Z","receivedAt":"2008-02-05T20:13:27Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 5 Feb 2008, Johannes Schindelin wrote:\n\n> \n> Follow the same logic as for loose objects when removing stale packs: they\n> might be in use (for example when fetching, or repacking in a cron job),\n> so give the user a chance to say (via --expire) what is considered too\n> young an age to die for stale packs.\n> \n> Also add a simple test to verify that the stale packs are actually\n> expired.\n> \n> Signed-off-by: Johannes Schindelin \n\nAcked-by: Nicolas Pitre \n\n\nNicolas\n"},{"id":"67611","messageId":"7v7ihi64xm.fsf@gitster.siamese.dyndns.org","threadId":"11902","inReplyTo":"alpine.LFD.1.00.0802051512370.2732@xanadu.home","subject":"Re: [PATCH] prune: heed --expire for stale packs, add a test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-06T05:15:49Z","receivedAt":"2008-02-06T05:15:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> On Tue, 5 Feb 2008, Johannes Schindelin wrote:\n>\n>> Follow the same logic as for loose objects when removing stale packs: they\n>> might be in use (for example when fetching, or repacking in a cron job),\n>> so give the user a chance to say (via --expire) what is considered too\n>> young an age to die for stale packs.\n>> \n>> Also add a simple test to verify that the stale packs are actually\n>> expired.\n>> \n>> Signed-off-by: Johannes Schindelin \n>\n> Acked-by: Nicolas Pitre \n>\n> Nicolas\n\nThey are not \"stale packs\", but temporary files that wanted to\nbecome pack but did not succeed. Perhaps \"stale temporary\npacks\"?\n\nShouldn't we do something similar to objects/pack/pack-*.temp\nfiles and objects/??/*.temp that http walker leaves?\n"},{"id":"67622","messageId":"alpine.LSU.1.00.0802060741380.8543@racer.site","threadId":"11902","inReplyTo":"7v7ihi64xm.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] prune: heed --expire for stale packs, add a test","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-06T07:41:45Z","receivedAt":"2008-02-06T07:41:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 5 Feb 2008, Junio C Hamano wrote:\n\n> Nicolas Pitre writes:\n> \n> > On Tue, 5 Feb 2008, Johannes Schindelin wrote:\n> >\n> >> Follow the same logic as for loose objects when removing stale packs: they\n> >> might be in use (for example when fetching, or repacking in a cron job),\n> >> so give the user a chance to say (via --expire) what is considered too\n> >> young an age to die for stale packs.\n> >> \n> >> Also add a simple test to verify that the stale packs are actually\n> >> expired.\n> >> \n> >> Signed-off-by: Johannes Schindelin \n> >\n> > Acked-by: Nicolas Pitre \n> >\n> > Nicolas\n> \n> They are not \"stale packs\", but temporary files that wanted to\n> become pack but did not succeed. Perhaps \"stale temporary\n> packs\"?\n> \n> Shouldn't we do something similar to objects/pack/pack-*.temp\n> files and objects/??/*.temp that http walker leaves?\n\nYep.\n\nCiao,\nDscho\n"},{"id":"67644","messageId":"7vve52z9lf.fsf@gitster.siamese.dyndns.org","threadId":"11902","inReplyTo":"Pine.GSO.4.63.0802051844220.15867@suma3","subject":"Re: [PATCH] Make git prune remove temporary packs that look like write failures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-06T10:02:20Z","receivedAt":"2008-02-06T10:02:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Steven Tweed writes:\n\n> Write errors when repacking (eg, due to out-of-space conditions)\n> can leave temporary packs (and possibly other files beginning\n> with \"tmp_\") lying around which no existing\n> codepath removes and which aren't obvious to the casual user.\n> These can also be multi-megabyte files wasting noticeable space.\n> Unfortunately there's no way to definitely tell in builtin-prune\n> that a tmp_ file is not being used by a concurrent process.\n> However, it is documented that pruning should only be done\n> on a quiet repository. The names of removed files are printed.\n>\n> Signed-off-by: David Tweed (david.tweed@gmail.com)\n> ---\n>\n> Per discussion of previous version, this now unconditionally\n> removes any tmp_ file existing when prune is run.\n\nSorry, can't apply\n\n\tContent-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed\n"},{"id":"67665","messageId":"alpine.LFD.1.00.0802060910340.2732@xanadu.home","threadId":"11902","inReplyTo":"7v7ihi64xm.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] prune: heed --expire for stale packs, add a test","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-02-06T14:12:38Z","receivedAt":"2008-02-06T14:12:38Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 5 Feb 2008, Junio C Hamano wrote:\n\n> They are not \"stale packs\", but temporary files that wanted to\n> become pack but did not succeed. Perhaps \"stale temporary\n> packs\"?\n> \n> Shouldn't we do something similar to objects/pack/pack-*.temp\n> files and objects/??/*.temp that http walker leaves?\n\nInstead, I think http walker should be made to use the same location and \nfilename pattern for its temporary files as the rest of the code.\n\n\nNicolas\n"},{"id":"67675","messageId":"47A9E4F9.8050100@nrlssc.navy.mil","threadId":"11902","inReplyTo":"alpine.LFD.1.00.0802051357420.2732@xanadu.home","subject":"Re: [PATCH] Make git prune remove temporary packs that look like write failures","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-02-06T16:48:57Z","receivedAt":"2008-02-06T16:48:57Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Nicolas Pitre wrote:\n> On Tue, 5 Feb 2008, David Steven Tweed wrote:\n> \n>> @@ -115,5 +139,6 @@ int cmd_prune(int argc, const char **argv, const char\n>> *prefix)\n>>\n>> \tsync();\n>> \tprune_packed_objects(show_only);\n>> +\tremove_temporary_files();\n> \n> Maybe you could implement the \"show_only\" mode for \n> remove_temporary_files() as well? Otherwise the -n option would not be \n> respected.\n\nI also suggest taking a look at the functions in builtin-prune-packed.c to see\nhow similar functions are implemented there.\n\nUse strlcpy instead of sprintf.\nUse prefixcmp instead of strncmp.\nUse same messages as prune_dir() for show_only path and unlink failure.\nYou could also check the opendir call and print a suitable message on failure.\n\n-brandon\n"},{"id":"67682","messageId":"e1dab3980802061059m5bf9c291s892da586248e229c@mail.gmail.com","threadId":"11902","inReplyTo":"47A9E4F9.8050100@nrlssc.navy.mil","subject":"Re: [PATCH] Make git prune remove temporary packs that look like write failures","fromName":"David Tweed","fromEmail":"david.tweed@gmail.com","sentAt":"2008-02-06T18:59:15Z","receivedAt":"2008-02-06T18:59:15Z","isPatch":true,"sender":{"key":"david.tweed@gmail.com","avatar":null},"body":"On Feb 6, 2008 4:48 PM, Brandon Casey wrote:\n> I also suggest taking a look at the functions in builtin-prune-packed.c to see\n> how similar functions are implemented there.\n>\n> Use strlcpy instead of sprintf.\n> Use prefixcmp instead of strncmp.\n> Use same messages as prune_dir() for show_only path and unlink failure.\n> You could also check the opendir call and print a suitable message on failure.\n\nAll the other path creation in builtin-prune.c is using sprintf; is\ndoing 3 strlcpy's much better? (I backed off from using snprintf when\nthe other element in the if that tested it vanished; I probably ought\nto put that back.) I'll use prefixcmp and check the opendir call\n(although if get_object_directory() doesn't return something sensible\npresumably bigger problems are in the mix.)\n\n-- \ncheers, dave tweed__________________________\ndavid.tweed@gmail.com\nRm 124, School of Systems Engineering, University of Reading.\n\"while having code so boring anyone can maintain it, use Python.\" --\nattempted insult seen on slashdot\n"},{"id":"67684","messageId":"e1dab3980802061110p2c1dad1ep8a46eeda93839bb9@mail.gmail.com","threadId":"11902","inReplyTo":"alpine.LFD.1.00.0802051357420.2732@xanadu.home","subject":"Re: [PATCH] Make git prune remove temporary packs that look like write failures","fromName":"David Tweed","fromEmail":"david.tweed@gmail.com","sentAt":"2008-02-06T19:10:36Z","receivedAt":"2008-02-06T19:10:36Z","isPatch":true,"sender":{"key":"david.tweed@gmail.com","avatar":null},"body":"On Feb 5, 2008 7:02 PM, Nicolas Pitre wrote:\n> On Tue, 5 Feb 2008, David Steven Tweed wrote:\n>\n> > @@ -115,5 +139,6 @@ int cmd_prune(int argc, const char **argv, const char\n> > *prefix)\n> >\n> > sync();\n> > prune_packed_objects(show_only);\n> > + remove_temporary_files();\n>\n> Maybe you could implement the \"show_only\" mode for\n> remove_temporary_files() as well? Otherwise the -n option would not be\n> respected.\n>\n> Also you should consider honoring the --expire option as well.\n\nI guess the -n ought to be honoured. However, unless I'm missing\nsomething, the case of expiring objects is different. The primary\nreason is that objects can get orphaned by \"semantic\" decisions\n(delete this branch, rewind, etc) so they contain valid content that\nyou might want to later rescue (using low-level command like git cat\nif necessary). In contrast, the only way to get a temporary pack when\nthe repository is quiescent is resulting from a _write error_ and thus\nis a corrupt entity which it would take a great deal of work to\nextract any valid data from. (To be honest, I wouldn't be bothering to\ndelete them if it weren't for the fact that they can be quite big\nfiles, and once you've got one from out-of-space you're more likely to\nget another in future because you've got even less space.) So it's not\nobvious that the same conditions should apply as to valid objects.\n\nDoes it really make sense to apply expire to these?\n\n-- \ncheers, dave tweed__________________________\ndavid.tweed@gmail.com\nRm 124, School of Systems Engineering, University of Reading.\n\"while having code so boring anyone can maintain it, use Python.\" --\nattempted insult seen on slashdot\n"},{"id":"67687","messageId":"alpine.LFD.1.00.0802061420510.2732@xanadu.home","threadId":"11902","inReplyTo":"e1dab3980802061110p2c1dad1ep8a46eeda93839bb9@mail.gmail.com","subject":"Re: [PATCH] Make git prune remove temporary packs that look like write failures","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-02-06T19:31:47Z","receivedAt":"2008-02-06T19:31:47Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 6 Feb 2008, David Tweed wrote:\n\n> I guess the -n ought to be honoured. However, unless I'm missing\n> something, the case of expiring objects is different. The primary\n> reason is that objects can get orphaned by \"semantic\" decisions\n> (delete this branch, rewind, etc) so they contain valid content that\n> you might want to later rescue (using low-level command like git cat\n> if necessary).\n\nYou can also get loose unconnected objects when fetching and the number \nof objects is lower than the transfer.unpackLimit value.\n\n> In contrast, the only way to get a temporary pack when\n> the repository is quiescent is resulting from a _write error_ and thus\n> is a corrupt entity which it would take a great deal of work to\n> extract any valid data from.\n\nOr when a fetch is in progress, just like the case above, but with the \nnumber of objects greater than transfer.unpackLimit.\n\nThis is uncommon to have a prune occurring at the same time as a fetch, \nbut the --expire argument is there if for example you do a prune from a \ncron job but still want to be safe by giving a grace period to garbage \nfiles which might not be so after all.\n\n\nNicolas\n"},{"id":"67689","messageId":"47AA0D60.60504@nrlssc.navy.mil","threadId":"11902","inReplyTo":"e1dab3980802061059m5bf9c291s892da586248e229c@mail.gmail.com","subject":"Re: [PATCH] Make git prune remove temporary packs that look like write failures","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-02-06T19:41:20Z","receivedAt":"2008-02-06T19:41:20Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"David Tweed wrote:\n> On Feb 6, 2008 4:48 PM, Brandon Casey wrote:\n>> I also suggest taking a look at the functions in builtin-prune-packed.c to see\n>> how similar functions are implemented there.\n>>\n>> Use strlcpy instead of sprintf.\n>> Use prefixcmp instead of strncmp.\n>> Use same messages as prune_dir() for show_only path and unlink failure.\n>> You could also check the opendir call and print a suitable message on failure.\n> \n> All the other path creation in builtin-prune.c is using sprintf; is\n> doing 3 strlcpy's much better? (I backed off from using snprintf when\n> the other element in the if that tested it vanished; I probably ought\n> to put that back.)\n\nThey use sprintf for the \"%02x\" part, but they use memcpy to copy the return\nof get_object_directory() into a fixed string and then append onto that,\nrather than repeatedly writing the same string over and over. Ok, there is one\ninstance in builtin-prune.c that repeatedly writes path, but builtin-prune-packed.c\ndoes the memcpy thing.\n\nSomething like:\n\n\tchar pathname[PATH_MAX];\n\t...\n\n\t/* check length of dirname not too long */\n\n\tmemcpy(pathname, dirname, len);\n\n\tif (len && pathname[len-1] != '/')\n\t\tpathname[len++] = '/';\n\n\t...\n\n\twhile ((de = readdir(dir)...) {\n\t\tif (!prefixcmp(...\n\t\t\tif (strlcpy(pathname + len, de->d_name, PATH_MAX - len)\n\t\t\t >= PATH_MAX - len) {\n\t\t\t\twarning(\"too long path encountered: %s%s\",\n\t\t\t\t\tpathname, de->d_name);\n\t\t\t\tcontinue;\n\t\t\t}\n\n\t\t\t...\n\n\n-brandon\n"},{"id":"67694","messageId":"e1dab3980802061157r36dfa8b9uab49af013cb8e963@mail.gmail.com","threadId":"11902","inReplyTo":"47AA0D60.60504@nrlssc.navy.mil","subject":"Re: [PATCH] Make git prune remove temporary packs that look like write failures","fromName":"David Tweed","fromEmail":"david.tweed@gmail.com","sentAt":"2008-02-06T19:57:41Z","receivedAt":"2008-02-06T19:57:41Z","isPatch":true,"sender":{"key":"david.tweed@gmail.com","avatar":null},"body":"On Feb 6, 2008 7:41 PM, Brandon Casey wrote:\n> They use sprintf for the \"%02x\" part, but they use memcpy to copy the return\n> of get_object_directory() into a fixed string and then append onto that,\n> rather than repeatedly writing the same string over and over. Ok, there is one\n> instance in builtin-prune.c that repeatedly writes path, but builtin-prune-packed.c\n> does the memcpy thing.\n\nGiven I'm the only person (AFAICS from the list archives) who's ever\ntalked about failed temporary packs, I assume that almost everyone\nusing git is using filesystems with sufficient space that they don't\nget write errors, so the path building will generally be done 0 times\nper --prune. (Using a USB disk that's almost full, along with\noccasionally filling up my /home with experiment output, is the\noccasions I get them.) So I don't think efficiency is an issue. So the\nbig (genuine) question is: is a properly checked snprintf more or less\nreadable/canonical git style than doing more complicated strlcpy\nthings? (I know I need to really think about what the below is doing,\nand what the correctness conditions are.)\n\n> Something like:\n>\n> char pathname[PATH_MAX];\n> ...\n>\n> /* check length of dirname not too long */\n>\n> memcpy(pathname, dirname, len);\n>\n> if (len && pathname[len-1] != '/')\n> pathname[len++] = '/';\n>\n> ...\n>\n> while ((de = readdir(dir)...) {\n> if (!prefixcmp(...\n> if (strlcpy(pathname + len, de->d_name, PATH_MAX - len)\n> >= PATH_MAX - len) {\n> warning(\"too long path encountered: %s%s\",\n> pathname, de->d_name);\n> continue;\n> }\n>\n> ...\n\n-- \ncheers, dave tweed__________________________\ndavid.tweed@gmail.com\nRm 124, School of Systems Engineering, University of Reading.\n\"while having code so boring anyone can maintain it, use Python.\" --\nattempted insult seen on slashdot\n"},{"id":"67695","messageId":"e1dab3980802061202i39c42432k4dd9f95560a7ea62@mail.gmail.com","threadId":"11902","inReplyTo":"alpine.LFD.1.00.0802061420510.2732@xanadu.home","subject":"Re: [PATCH] Make git prune remove temporary packs that look like write failures","fromName":"David Tweed","fromEmail":"david.tweed@gmail.com","sentAt":"2008-02-06T20:02:34Z","receivedAt":"2008-02-06T20:02:34Z","isPatch":true,"sender":{"key":"david.tweed@gmail.com","avatar":null},"body":"On Feb 6, 2008 7:31 PM, Nicolas Pitre wrote:\n> On Wed, 6 Feb 2008, David Tweed wrote:\n>\n> > I guess the -n ought to be honoured. However, unless I'm missing\n> > something, the case of expiring objects is different. The primary\n> > reason is that objects can get orphaned by \"semantic\" decisions\n> > (delete this branch, rewind, etc) so they contain valid content that\n> > you might want to later rescue (using low-level command like git cat\n> > if necessary).\n>\n> You can also get loose unconnected objects when fetching and the number\n> of objects is lower than the transfer.unpackLimit value.\n\nI didn't know that, but the key point is that these are still\nwell-formed objects, so rescuing data from them makes sense. In\ncontrast, I'm concerned with packs which are ill-formed from a\nwrite-error, which I don't believe git will even read. (I've just\ntried truncating a finished pack file and none of the git commands\nI've tried will read any objects in it. So you'll have to do low-level\nhacking to get data out of a write-error pack. WARNING: be aware of\nhow to do a non-hardlinked clone before trying this.)\n\n> > In contrast, the only way to get a temporary pack when\n> > the repository is quiescent is resulting from a _write error_ and thus\n> > is a corrupt entity which it would take a great deal of work to\n> > extract any valid data from.\n>\n> Or when a fetch is in progress, just like the case above, but with the\n> number of objects greater than transfer.unpackLimit.\n\nBeing \"in progress\" contradicts the presumption of \"being quiescent\".\n\n> This is uncommon to have a prune occurring at the same time as a fetch,\n> but the --expire argument is there if for example you do a prune from a\n> cron job but still want to be safe by giving a grace period to garbage\n> files which might not be so after all.\n\nAh, I hadn't realised this was an intended usage of --expire. Since as\nyou note there's no way to tell an abandoned tmp pack from one that's\nin the process of being written, following expire is probably\nnecessary for safety. I'll look at adding support for that.\n\nMany thanks for the advice,\n\n-- \ncheers, dave tweed__________________________\ndavid.tweed@gmail.com\nRm 124, School of Systems Engineering, University of Reading.\n\"while having code so boring anyone can maintain it, use Python.\" --\nattempted insult seen on slashdot\n"},{"id":"67696","messageId":"alpine.LFD.1.00.0802061505390.2732@xanadu.home","threadId":"11902","inReplyTo":"e1dab3980802061157r36dfa8b9uab49af013cb8e963@mail.gmail.com","subject":"Re: [PATCH] Make git prune remove temporary packs that look like write failures","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-02-06T20:07:48Z","receivedAt":"2008-02-06T20:07:48Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 6 Feb 2008, David Tweed wrote:\n\n> Given I'm the only person (AFAICS from the list archives) who's ever\n> talked about failed temporary packs, I assume that almost everyone\n> using git is using filesystems with sufficient space that they don't\n> get write errors,\n\nNote that doing ^C during a fetch will also leave a temporary pack file \nthere. So your patch is good even for people with plenty of disk space.\n\n> so the path building will generally be done 0 times\n> per --prune. (Using a USB disk that's almost full, along with\n> occasionally filling up my /home with experiment output, is the\n> occasions I get them.) So I don't think efficiency is an issue.\n\nClearly not a hot path indeed.\n\n\nNicolas\n"},{"id":"67699","messageId":"alpine.LFD.1.00.0802061511440.2732@xanadu.home","threadId":"11902","inReplyTo":"e1dab3980802061202i39c42432k4dd9f95560a7ea62@mail.gmail.com","subject":"Re: [PATCH] Make git prune remove temporary packs that look like write failures","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-02-06T20:16:22Z","receivedAt":"2008-02-06T20:16:22Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 6 Feb 2008, David Tweed wrote:\n> On Feb 6, 2008 7:31 PM, Nicolas Pitre wrote:\n> > This is uncommon to have a prune occurring at the same time as a fetch,\n> > but the --expire argument is there if for example you do a prune from a\n> > cron job but still want to be safe by giving a grace period to garbage\n> > files which might not be so after all.\n> \n> Ah, I hadn't realised this was an intended usage of --expire. Since as\n> you note there's no way to tell an abandoned tmp pack from one that's\n> in the process of being written, following expire is probably\n> necessary for safety. I'll look at adding support for that.\n\nDid you miss Johannes ' patch? He posted it yesterday and you were even \nCC'd. It did exactly that on top of yours already.\n\n\nNicolas\n"},{"id":"67700","messageId":"e1dab3980802061225l21cc35b5u288695924f56f3ca@mail.gmail.com","threadId":"11902","inReplyTo":"alpine.LFD.1.00.0802061511440.2732@xanadu.home","subject":"Re: [PATCH] Make git prune remove temporary packs that look like write failures","fromName":"David Tweed","fromEmail":"david.tweed@gmail.com","sentAt":"2008-02-06T20:25:01Z","receivedAt":"2008-02-06T20:25:01Z","isPatch":true,"sender":{"key":"david.tweed@gmail.com","avatar":null},"body":"On Feb 6, 2008 8:16 PM, Nicolas Pitre wrote:\n> On Wed, 6 Feb 2008, David Tweed wrote:\n> > Ah, I hadn't realised this was an intended usage of --expire. Since as\n> > you note there's no way to tell an abandoned tmp pack from one that's\n> > in the process of being written, following expire is probably\n> > necessary for safety. I'll look at adding support for that.\n>\n> Did you miss Johannes ' patch? He posted it yesterday and you were even\n> CC'd. It did exactly that on top of yours already.\n\nI had missed it (I've only just started reading email). Thanks for\npointing it out.\n\n\n\n-- \ncheers, dave tweed__________________________\ndavid.tweed@gmail.com\nRm 124, School of Systems Engineering, University of Reading.\n\"while having code so boring anyone can maintain it, use Python.\" --\nattempted insult seen on slashdot\n"},{"id":"67702","messageId":"7vfxw5x1yp.fsf@gitster.siamese.dyndns.org","threadId":"11902","inReplyTo":"alpine.LFD.1.00.0802060910340.2732@xanadu.home","subject":"Re: [PATCH] prune: heed --expire for stale packs, add a test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-06T20:30:06Z","receivedAt":"2008-02-06T20:30:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> On Tue, 5 Feb 2008, Junio C Hamano wrote:\n>\n>> They are not \"stale packs\", but temporary files that wanted to\n>> become pack but did not succeed. Perhaps \"stale temporary\n>> packs\"?\n>> \n>> Shouldn't we do something similar to objects/pack/pack-*.temp\n>> files and objects/??/*.temp that http walker leaves?\n>\n> Instead, I think http walker should be made to use the same location and \n> filename pattern for its temporary files as the rest of the code.\n\nI concur. That's much cleaner.\n"},{"id":"67707","messageId":"47AA1BF9.5030803@nrlssc.navy.mil","threadId":"11902","inReplyTo":"e1dab3980802061157r36dfa8b9uab49af013cb8e963@mail.gmail.com","subject":"Re: [PATCH] Make git prune remove temporary packs that look like write failures","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-02-06T20:43:37Z","receivedAt":"2008-02-06T20:43:37Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"David Tweed wrote:\n> On Feb 6, 2008 7:41 PM, Brandon Casey wrote:\n>> They use sprintf for the \"%02x\" part, but they use memcpy to copy the return\n>> of get_object_directory() into a fixed string and then append onto that,\n>> rather than repeatedly writing the same string over and over. Ok, there is one\n>> instance in builtin-prune.c that repeatedly writes path, but builtin-prune-packed.c\n>> does the memcpy thing.\n> \n\n>So I don't think efficiency is an issue.\n\nYeah not performance critical, readability is definitely more important.\n\nI just don't like repeatedly doing:\n\n sprintf(dst, \"static string %s\", s)\n\nwhen the \"static string\" part doesn't change.\n\nEither way it doesn't make much difference here.\n\n-brandon\n"}]}