{"thread":{"id":"42659","subject":"[PATCH] prune: keep files created after process start","startedAt":"2016-06-19T03:13:42Z","lastAt":"2016-06-19T09:34:46Z","messageCount":6,"participants":["Eric Wong","Junio C Hamano","Andreas Schwab","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"289601","messageId":"20160619031338.GA5161@dcvr.yhbt.net","threadId":"42659","inReplyTo":null,"subject":"[PATCH] prune: keep files created after process start","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-06-19T03:13:38Z","receivedAt":"2016-06-19T03:13:42Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Avoid pruning files which were written after the prune process\nstarts, as it possible to concurrently create new objects while\n\"git prune\" is running.\n\nTested on git.git by starting \"git prune\" in one terminal,\ncreating a random loose object via \"git hash-object --stdin -w\"\nin a different terminal, and ensuring the loose object remains\nafter \"git prune\" completes.\n\nSigned-off-by: Eric Wong \n---\n I'm somewhat surprised this check didn't already exist;\n but maybe nobody else runs prune manually, anymore.\n\n builtin/prune.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 8f4f052..d4cd054 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -14,6 +14,7 @@ static const char * const prune_usage[] = {\n static int show_only;\n static int verbose;\n static unsigned long expire;\n+static time_t start;\n static int show_progress = -1;\n \n static int prune_tmp_file(const char *fullpath)\n@@ -21,7 +22,7 @@ static int prune_tmp_file(const char *fullpath)\n \tstruct stat st;\n \tif (lstat(fullpath, &st))\n \t\treturn error(\"Could not stat '%s'\", fullpath);\n-\tif (st.st_mtime > expire)\n+\tif (st.st_mtime > expire || st.st_ctime >= start)\n \t\treturn 0;\n \tif (show_only || verbose)\n \t\tprintf(\"Removing stale temporary file %s\\n\", fullpath);\n@@ -47,7 +48,7 @@ static int prune_object(const unsigned char *sha1, const char *fullpath,\n \t\terror(\"Could not stat '%s'\", fullpath);\n \t\treturn 0;\n \t}\n-\tif (st.st_mtime > expire)\n+\tif (st.st_mtime > expire || st.st_ctime >= start)\n \t\treturn 0;\n \tif (show_only || verbose) {\n \t\tenum object_type type = sha1_object_info(sha1, NULL);\n@@ -111,6 +112,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \t};\n \tchar *s;\n \n+\tstart = time(NULL);\n \texpire = ULONG_MAX;\n \tsave_commit_buffer = 0;\n \tcheck_replace_refs = 0;\n"},{"id":"289603","messageId":"xmqqa8ihygms.fsf@gitster.mtv.corp.google.com","threadId":"42659","inReplyTo":"20160619031338.GA5161@dcvr.yhbt.net","subject":"Re: [PATCH] prune: keep files created after process start","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-19T03:58:03Z","receivedAt":"2016-06-19T03:58:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> Avoid pruning files which were written after the prune process\n> starts, as it possible to concurrently create new objects while\n> \"git prune\" is running.\n>\n> Tested on git.git by starting \"git prune\" in one terminal,\n> creating a random loose object via \"git hash-object --stdin -w\"\n> in a different terminal, and ensuring the loose object remains\n> after \"git prune\" completes.\n>\n> Signed-off-by: Eric Wong \n> ---\n> I'm somewhat surprised this check didn't already exist;\n> but maybe nobody else runs prune manually, anymore.\n\nThe only time an end user would run \"git prune\" in their\nrepositories with working trees these days is \"git repack\" followed\nby \"git prune\", I would guess.\n\nThe current behaviour is totally expected and that is why you do not\nrun \"prune\" without any grace period in an active repository.\n\nAdding the proposed change however would not make anything worse, I\nwould think. Those who run \"git prune\" without grace period accept\nthat the repository must be quiescent or otherwise the newly created\nobjects will be lost. So they are unlikely to be doing anything to\ncreate such objects, hence \"things newer than 'start'\" are unlikely\nto appear, and their expectation that \"repack && prune\" will remove\nall loose objects will not be broken.\n\nYou generally cannot compare a timestamp you read from the\nfilesystem and the timestamp you obtain from time(2) when network\nfilesystems are involved, so I am not sure the implementation is\nquite right, though.\n\n> diff --git a/builtin/prune.c b/builtin/prune.c\n> index 8f4f052..d4cd054 100644\n> --- a/builtin/prune.c\n> +++ b/builtin/prune.c\n> @@ -14,6 +14,7 @@ static const char * const prune_usage[] = {\n> static int show_only;\n> static int verbose;\n> static unsigned long expire;\n> +static time_t start;\n> static int show_progress = -1;\n> \n> static int prune_tmp_file(const char *fullpath)\n> @@ -21,7 +22,7 @@ static int prune_tmp_file(const char *fullpath)\n> \tstruct stat st;\n> \tif (lstat(fullpath, &st))\n> \t\treturn error(\"Could not stat '%s'\", fullpath);\n> -\tif (st.st_mtime > expire)\n> +\tif (st.st_mtime > expire || st.st_ctime >= start)\n> \t\treturn 0;\n> \tif (show_only || verbose)\n> \t\tprintf(\"Removing stale temporary file %s\\n\", fullpath);\n> @@ -47,7 +48,7 @@ static int prune_object(const unsigned char *sha1, const char *fullpath,\n> \t\terror(\"Could not stat '%s'\", fullpath);\n> \t\treturn 0;\n> \t}\n> -\tif (st.st_mtime > expire)\n> +\tif (st.st_mtime > expire || st.st_ctime >= start)\n> \t\treturn 0;\n> \tif (show_only || verbose) {\n> \t\tenum object_type type = sha1_object_info(sha1, NULL);\n> @@ -111,6 +112,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n> \t};\n> \tchar *s;\n> \n> +\tstart = time(NULL);\n> \texpire = ULONG_MAX;\n> \tsave_commit_buffer = 0;\n> \tcheck_replace_refs = 0;\n"},{"id":"289604","messageId":"20160619043243.GA26443@dcvr.yhbt.net","threadId":"42659","inReplyTo":"xmqqa8ihygms.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] prune: keep files created after process start","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-06-19T04:32:43Z","receivedAt":"2016-06-19T04:32:47Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano wrote:\n> Eric Wong writes:\n> > I'm somewhat surprised this check didn't already exist;\n> > but maybe nobody else runs prune manually, anymore.\n> \n> The only time an end user would run \"git prune\" in their\n> repositories with working trees these days is \"git repack\" followed\n> by \"git prune\", I would guess.\n\nRight, I wanted to drop some sensitive data with that.\n\n> You generally cannot compare a timestamp you read from the\n> filesystem and the timestamp you obtain from time(2) when network\n> filesystems are involved, so I am not sure the implementation is\n> quite right, though.\n\nYes, but I'm not aware of a good way to deal with this;\nI would expect machines on the same network would have\nsynchronized times.\n\nPerhaps having a small slack time (one second?) could mitigate\nsome problems with machines being slightly off:\n\n start = time(NULL) - 1;\n\nAnd then warning if it encountered files within the slack\nperiod and asking the user to rerun \"prune\" if needed.\nBut I'm not sure it's worth it.\n\nAny other suggestions?\n\nThanks.\n\nHaving prune prevent object creation entirely while running\nseems unacceptable.\n"},{"id":"289609","messageId":"m2wpllhcut.fsf@linux-m68k.org","threadId":"42659","inReplyTo":"20160619031338.GA5161@dcvr.yhbt.net","subject":"Re: [PATCH] prune: keep files created after process start","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2016-06-19T07:11:38Z","receivedAt":"2016-06-19T07:11:51Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Eric Wong writes:\n\n> @@ -21,7 +22,7 @@ static int prune_tmp_file(const char *fullpath)\n> \tstruct stat st;\n> \tif (lstat(fullpath, &st))\n> \t\treturn error(\"Could not stat '%s'\", fullpath);\n> -\tif (st.st_mtime > expire)\n> +\tif (st.st_mtime > expire || st.st_ctime >= start)\n\nThat will also mean objects created (or their inode changed) up to a\nsecond before the start of prune will not be removed. For example,\nobjects ejected out of a pack by a previous repack may be affected.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"289613","messageId":"20160619084837.GA22090@dcvr.yhbt.net","threadId":"42659","inReplyTo":"m2wpllhcut.fsf@linux-m68k.org","subject":"Re: [PATCH] prune: keep files created after process start","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-06-19T08:48:37Z","receivedAt":"2016-06-19T08:48:45Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Andreas Schwab wrote:\n> Eric Wong writes:\n> \n> > @@ -21,7 +22,7 @@ static int prune_tmp_file(const char *fullpath)\n> > \tstruct stat st;\n> > \tif (lstat(fullpath, &st))\n> > \t\treturn error(\"Could not stat '%s'\", fullpath);\n> > -\tif (st.st_mtime > expire)\n> > +\tif (st.st_mtime > expire || st.st_ctime >= start)\n> \n> That will also mean objects created (or their inode changed) up to a\n> second before the start of prune will not be removed. For example,\n> objects ejected out of a pack by a previous repack may be affected.\n\nTrue, but I prefer to err on the side of keeping data rather than\nremoving it. But keeping it can also be a liability (as it was\nin my case :) So, perhaps warn users instead:\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex d4cd054..c1642d1 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -16,14 +16,22 @@ static int verbose;\n static unsigned long expire;\n static time_t start;\n static int show_progress = -1;\n+static unsigned long ctime_matches;\n \n static int prune_tmp_file(const char *fullpath)\n {\n \tstruct stat st;\n \tif (lstat(fullpath, &st))\n \t\treturn error(\"Could not stat '%s'\", fullpath);\n-\tif (st.st_mtime > expire || st.st_ctime >= start)\n+\tif (st.st_mtime > expire || st.st_ctime > start)\n \t\treturn 0;\n+\n+\tif (st.st_ctime == start) {\n+\t\tctime_matches++;\n+\t\twarning(\"Keeping %s since it changed as we started\", fullpath);\n+\t\treturn 0;\n+\t}\n+\n \tif (show_only || verbose)\n \t\tprintf(\"Removing stale temporary file %s\\n\", fullpath);\n \tif (!show_only)\n@@ -48,8 +56,13 @@ static int prune_object(const unsigned char *sha1, const char *fullpath,\n \t\terror(\"Could not stat '%s'\", fullpath);\n \t\treturn 0;\n \t}\n-\tif (st.st_mtime > expire || st.st_ctime >= start)\n+\tif (st.st_mtime > expire || st.st_ctime > start)\n \t\treturn 0;\n+\tif (st.st_ctime == start) {\n+\t\tctime_matches++;\n+\t\twarning(\"Keeping %s since it changed as we started\", fullpath);\n+\t\treturn 0;\n+\t}\n \tif (show_only || verbose) {\n \t\tenum object_type type = sha1_object_info(sha1, NULL);\n \t\tprintf(\"%s %s\\n\", sha1_to_hex(sha1),\n@@ -155,5 +168,12 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \tif (is_repository_shallow())\n \t\tprune_shallow(show_only);\n \n+\tif (ctime_matches) {\n+\t\twarning(\"%lu files kept since they changed as prune started\",\n+\t\t\tctime_matches);\n+\t\twarning(\"rerun prune after %s\",\n+\t\t\tshow_date(start, 0, DATE_MODE(NORMAL)));\n+\t}\n+\n \treturn 0;\n }\n"},{"id":"289616","messageId":"20160619093439.GB12221@sigill.intra.peff.net","threadId":"42659","inReplyTo":"xmqqa8ihygms.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] prune: keep files created after process start","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-19T09:34:39Z","receivedAt":"2016-06-19T09:34:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 18, 2016 at 08:58:03PM -0700, Junio C Hamano wrote:\n\n> Eric Wong writes:\n> \n> > Avoid pruning files which were written after the prune process\n> > starts, as it possible to concurrently create new objects while\n> > \"git prune\" is running.\n> >\n> > Tested on git.git by starting \"git prune\" in one terminal,\n> > creating a random loose object via \"git hash-object --stdin -w\"\n> > in a different terminal, and ensuring the loose object remains\n> > after \"git prune\" completes.\n> >\n> > Signed-off-by: Eric Wong \n> > ---\n> > I'm somewhat surprised this check didn't already exist;\n> > but maybe nobody else runs prune manually, anymore.\n> \n> The only time an end user would run \"git prune\" in their\n> repositories with working trees these days is \"git repack\" followed\n> by \"git prune\", I would guess.\n> \n> The current behaviour is totally expected and that is why you do not\n> run \"prune\" without any grace period in an active repository.\n> \n> Adding the proposed change however would not make anything worse, I\n> would think. Those who run \"git prune\" without grace period accept\n> that the repository must be quiescent or otherwise the newly created\n> objects will be lost. So they are unlikely to be doing anything to\n> create such objects, hence \"things newer than 'start'\" are unlikely\n> to appear, and their expectation that \"repack && prune\" will remove\n> all loose objects will not be broken.\n\nI agree it does not making anything worse from a correctness standpoint,\nbut I am mildly negative in that it adds complexity without any real\nsafety. Even after this patch, running git prune without a grace period\nisn't safe, as you are racing simultaneous processes that started before\nprune, or started after prune but before it marks the \"start\" timestamp.\n\nI know it isn't much code, but it makes things harder to reason about.\nI think it also runs contrary to the logic in 3d27b9b (date.c: add\nparse_expiry_date(), 2013-04-17)[1].\n\n-Peff\n\n[1] More context in http://mid.gmane.org/7vbo9ceqb3.fsf@alter.siamese.dyndns.org\n and the surrounding thread.\n"}]}