{"thread":{"id":"45090","subject":"[PATCH] gc: ignore old gc.log files","startedAt":"2017-02-09T02:05:06Z","lastAt":"2017-02-10T00:36:59Z","messageCount":5,"participants":["David Turner","Jeff King","Eric Wong","Philip Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"311135","messageId":"20170209020222.23642-1-dturner@twosigma.com","threadId":"45090","inReplyTo":null,"subject":"[PATCH] gc: ignore old gc.log files","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2017-02-09T02:02:22Z","receivedAt":"2017-02-09T02:05:06Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"The intent of automatic gc is to have a git repository be relatively\nlow-maintenance from a server-operator perspective.  Of course, large\noperators like GitHub will need a more complicated management strategy,\nbut for ordinary usage, git should just work.\n\nIn this commit, git learns to ignore gc.log files which are older than\n(by default) one day old.  It also learns about a config, gc.maxLogAge\nto manage this.\n\nSo git should never get itself into a state where it refuses to do any\nmaintenance, just because at some point some piece of the maintenance\ndidn't make progress.  That might still happen (e.g. because the repo\nis corrupt), but at the very least it won't be because Git is too dumb\nto try again.\n\nSigned-off-by: David Turner <dturner@twosigma.com>\nHelped-by: Jeff King <peff@peff.net>\n---\n Documentation/config.txt |  5 +++++\n builtin/gc.c             | 15 ++++++++++++++-\n t/t6500-gc.sh            | 13 +++++++++++++\n 3 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex fc5a28a32..6751371cf 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1402,6 +1402,11 @@ gc.autoDetach::\n \tMake `git gc --auto` return immediately and run in background\n \tif the system supports it. Default is true.\n \n+gc.maxLogAge::\n+\tIf the file gc.log exists, then `git gc --auto` won't run\n+\tunless that file is more than maxLogAge seconds old.  Default\n+\tis 86400, one day.\n+\n gc.packRefs::\n \tRunning `git pack-refs` in a repository renders it\n \tunclonable by Git versions prior to 1.5.1.2 over dumb\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 331f21926..62fc84815 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -33,6 +33,7 @@ static int aggressive_window = 250;\n static int gc_auto_threshold = 6700;\n static int gc_auto_pack_limit = 50;\n static int detach_auto = 1;\n+static int gc_max_log_age_seconds = 86400;\n static const char *prune_expire = \"2.weeks.ago\";\n static const char *prune_worktrees_expire = \"3.months.ago\";\n \n@@ -111,6 +112,7 @@ static void gc_config(void)\n \tgit_config_get_int(\"gc.auto\", &gc_auto_threshold);\n \tgit_config_get_int(\"gc.autopacklimit\", &gc_auto_pack_limit);\n \tgit_config_get_bool(\"gc.autodetach\", &detach_auto);\n+\tgit_config_get_int(\"gc.maxlogage\", &gc_max_log_age_seconds);\n \tgit_config_date_string(\"gc.pruneexpire\", &prune_expire);\n \tgit_config_date_string(\"gc.worktreepruneexpire\", &prune_worktrees_expire);\n \tgit_config(git_default_config, NULL);\n@@ -291,8 +293,19 @@ static int report_last_gc_error(void)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tint ret;\n+\tstruct stat st;\n+\tconst char *gc_log_path = git_path(\"gc.log\");\n+\n+\tif (stat(gc_log_path, &st)) {\n+\t\tif (errno == ENOENT)\n+\t\t\treturn 0;\n+\t\treturn error(_(\"Can't read %s\"), gc_log_path);\n+\t}\n+\n+\tif (time(NULL) - st.st_mtime > gc_max_log_age_seconds)\n+\t\treturn 0;\n \n-\tret = strbuf_read_file(&sb, git_path(\"gc.log\"), 0);\n+\tret = strbuf_read_file(&sb, gc_log_path, 0);\n \tif (ret > 0)\n \t\treturn error(_(\"The last gc run reported the following. \"\n \t\t\t       \"Please correct the root cause\\n\"\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 1762dfa6a..b69c2c190 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -67,5 +67,18 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre\n \ttest_line_count = 2 new # There is one new pack and its .idx\n '\n \n+test_expect_success 'background auto gc does not run if gc.log is present and recent but does if it is old' '\n+\tkeep=$(ls .git/objects/pack/*.pack|head -1|sed -e \"s/pack$/keep/\") &&\n+\ttest_commit foo &&\n+\ttest_commit bar &&\n+\tgit repack &&\n+\ttest_config gc.autopacklimit 1 &&\n+\ttest_config gc.autodetach true &&\n+\techo fleem> .git/gc.log &&\n+\ttest_must_fail git gc --auto 2>err &&\n+\ttest_i18ngrep \"^error:\" err &&\n+\ttest-chmtime =-86401 .git/gc.log &&\n+\tgit gc --auto\n+'\n \n test_done\n-- \n2.11.GIT\n\n"},{"id":"311139","messageId":"20170209032325.bspll66ux6n2pj4n@sigill.intra.peff.net","threadId":"45090","inReplyTo":"20170209020222.23642-1-dturner@twosigma.com","subject":"Re: [PATCH] gc: ignore old gc.log files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-09T03:23:25Z","receivedAt":"2017-02-09T03:30:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 08, 2017 at 09:02:22PM -0500, David Turner wrote:\n\n> The intent of automatic gc is to have a git repository be relatively\n> low-maintenance from a server-operator perspective.  Of course, large\n> operators like GitHub will need a more complicated management strategy,\n> but for ordinary usage, git should just work.\n> \n> In this commit, git learns to ignore gc.log files which are older than\n> (by default) one day old.  It also learns about a config, gc.maxLogAge\n> to manage this.\n> \n> So git should never get itself into a state where it refuses to do any\n> maintenance, just because at some point some piece of the maintenance\n> didn't make progress.  That might still happen (e.g. because the repo\n> is corrupt), but at the very least it won't be because Git is too dumb\n> to try again.\n\nSounds like a good goal and approach.\n\n> +gc.maxLogAge::\n> +\tIf the file gc.log exists, then `git gc --auto` won't run\n> +\tunless that file is more than maxLogAge seconds old.  Default\n> +\tis 86400, one day.\n\nFor other time-based config, we use approxidate with a relative time,\nlike \"1 day ago\". I think it would make sense for this to match, as it\nmakes the config a little more readable.\n\nYou can follow the prune_expire example which is right below your new\nconfig variable in all of the hunks of your patch. Though I think\nultimately that isn't parsed inside gc, so you'd eventually look at how\n\"prune --expire\" is handled (which I think is via parse_expiry_date()).\n\n> @@ -291,8 +293,19 @@ static int report_last_gc_error(void)\n>  {\n>  \tstruct strbuf sb = STRBUF_INIT;\n>  \tint ret;\n> +\tstruct stat st;\n> +\tconst char *gc_log_path = git_path(\"gc.log\");\n\nI usually try to avoid assigning the result of git_path(), because it's\na static buffer. In this case it's fine, because you don't call any\ncomplex calls during the lifetime of the variable. But I consider\nassigning to be a bad practice.\n\nUsing git_pathdup() or git_path_buf() is safer (but you do need to\nremember to free() as appropriate).\n\nSpeaking of leaks, I think report_last_gc_error() will always leak the\nstrbuf contents when it is non-empty.\n\n> +\tif (stat(gc_log_path, &st)) {\n> +\t\tif (errno == ENOENT)\n> +\t\t\treturn 0;\n> +\t\treturn error(_(\"Can't read %s\"), gc_log_path);\n> +\t}\n\nI'm not quite clear on the life-cycle of this log file.\n\nI think the current code works like this:\n\n  - if a non-empty gc.log exists, we bail\n\n  - when we daemonize, we take a lock via gc.log.lock\n\n  - if we wrote anything to the lockfile log, then we move it into place\n    (essentially blocking further auto-gc)\n\n  - otherwise, we rollback the lockfile and leave gc.log untouched\n\nThat leaves a few quirks with your new strategy:\n\n  - if our new run was unsuccessful (as judged by having non-empty log\n    output), we'd probably want to overwrite the old logfile with our\n    new log. Following the current-code logic we do, which is good.\n\n  - if our new run is successful (empty log), we'll leave the old,\n    crufty log in place. Probably process_log_file() should unlink() the\n    original gc.log while holding the lock.\n\nAnd here are a few things I noticed that I think aren't caused by your\npatch, but are in the same area and might be worth examining:\n\n  - we're not very atomic. After a day, two simultaneous auto-gc's might\n    both ignore the gc.log file and continue to run. I don't think it\n    matters, though. One of them will win the race to pick up\n    gc.log.lock, and the other will immediately fail.\n\n  - It looks like we clear the gc.log file only under another detached\n    auto-gc. It seems like manually running a successful \"git gc\" should\n    clear it, too.\n\n  - We block further gc only based on the presence of data on stderr\n    from the sub-programs. But _not_ if the sub-programs fail. So a\n    program silently exiting with code 128 will stop further gc\n    processing, but not prevent another auto-gc from running. Which\n    is...weird. Maybe this can't happen, though, because I think we\n    write our own error() in such cases, which makes such a failure\n    inherently non-silent.\n\n> +\tif (time(NULL) - st.st_mtime > gc_max_log_age_seconds)\n> +\t\treturn 0;\n\nHmm. What happens if the file has a timestamp in the future due to clock\nskew? As long as time_t is signed, I think it's OK, but if it wraps, it\ndoes the wrong thing here. You could rearrange the subtraction to handle\nthis. But I think if you start using approxidate, it will give you an\na cutoff time, and you can just do:\n\n  if (st.st_mtime < gc_log_expiration)\n\treturn 0; /* too old to care about */\n\n> -\tret = strbuf_read_file(&sb, git_path(\"gc.log\"), 0);\n> +\tret = strbuf_read_file(&sb, gc_log_path, 0);\n\nI would have written this as an open(), followed by an fstat() on the\nfile we opened, and then finally reading the contents if it's fresh\nenough. But I'm not sure if that level of atomicity matters. We're not\ndoing any of this under lock.\n\n-Peff\n"},{"id":"311144","messageId":"20170209045156.GA2567@dcvr.yhbt.net","threadId":"45090","inReplyTo":"20170209020222.23642-1-dturner@twosigma.com","subject":"Re: [PATCH] gc: ignore old gc.log files","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2017-02-09T04:51:56Z","receivedAt":"2017-02-09T04:52:58Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"David Turner <dturner@twosigma.com> wrote:\n> +\techo fleem> .git/gc.log &&\n\nA minor nit:\n\n\techo fleem >.git/gc.log &&\n\nOtherwise, it's good to know there's attention paid to this\nissue.  I've been ignoring cron mails from my mirrors for too\nlong :x  Thanks.\n"},{"id":"311240","messageId":"1637154470724DABAC99AF6199707A4B@PhilipOakley","threadId":"45090","inReplyTo":"20170209032325.bspll66ux6n2pj4n@sigill.intra.peff.net","subject":"Re: [PATCH] gc: ignore old gc.log files","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2017-02-09T23:57:25Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Jeff King\" <peff@peff.net>\n> On Wed, Feb 08, 2017 at 09:02:22PM -0500, David Turner wrote:\n>\n>> The intent of automatic gc is to have a git repository be relatively\n>> low-maintenance from a server-operator perspective.  Of course, large\n>> operators like GitHub will need a more complicated management strategy,\n>> but for ordinary usage, git should just work.\n>>\n>> In this commit, git learns to ignore gc.log files which are older than\n>> (by default) one day old.  It also learns about a config, gc.maxLogAge\n>> to manage this.\n>>\n>> So git should never get itself into a state where it refuses to do any\n>> maintenance, just because at some point some piece of the maintenance\n>> didn't make progress.  That might still happen (e.g. because the repo\n>> is corrupt), but at the very least it won't be because Git is too dumb\n>> to try again.\n>\n> Sounds like a good goal and approach.\n>\n>> +gc.maxLogAge::\n>> + If the file gc.log exists, then `git gc --auto` won't run\n>> + unless that file is more than maxLogAge seconds old.  Default\n>> + is 86400, one day.\n\nIs there a reason why one day is chosen? If maintenance staff are available \n24/7 then a shorter time would be appropriate, but if it's a 5 day work week \nthen they may want longer. Is there a particular case it targets?\n\n>\n> For other time-based config, we use approxidate with a relative time,\n> like \"1 day ago\". I think it would make sense for this to match, as it\n> makes the config a little more readable.\n>\n> You can follow the prune_expire example which is right below your new\n> config variable in all of the hunks of your patch. Though I think\n> ultimately that isn't parsed inside gc, so you'd eventually look at how\n> \"prune --expire\" is handled (which I think is via parse_expiry_date()).\n>\n[...]\n\nPhilip \n\n"},{"id":"311241","messageId":"20170210003632.7ghlgfulwdsntlag@sigill.intra.peff.net","threadId":"45090","inReplyTo":"1637154470724DABAC99AF6199707A4B@PhilipOakley","subject":"Re: [PATCH] gc: ignore old gc.log files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-10T00:36:32Z","receivedAt":"2017-02-10T00:36:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 09, 2017 at 11:57:12PM -0000, Philip Oakley wrote:\n\n> > > +gc.maxLogAge::\n> > > + If the file gc.log exists, then `git gc --auto` won't run\n> > > + unless that file is more than maxLogAge seconds old.  Default\n> > > + is 86400, one day.\n> \n> Is there a reason why one day is chosen? If maintenance staff are available\n> 24/7 then a shorter time would be appropriate, but if it's a 5 day work week\n> then they may want longer. Is there a particular case it targets?\n\nI'm pretty sure the one-day time limit isn't scientific. It's just a\nnumber we've been throwing around.\n\nI'm not sure what maintenance staff matters, though. It basically needs\nlong enough that we're not doing _too_ many fruitless gc's, because it\nwastes resources. But you'd prefer to not go too long without a gc for a\nrepository that needs it.\n\nThe root cause of the error could be any number of issues. But for the\ncase that David cares about most, you basically want to keep trying\nuntil the too-many-objects condition goes away. That's usually on a\n2-week timer. So trying once per day to see if the 2-week timer feels\nabout right.\n\nThat's certainly not science, but hopefully it at least frames the\ngeneral ballpark.\n\nOne possible option would be to auto-scale it with the pruneExpire time.\nI don't know if people actually tweak that value or not.\n\n-Peff\n"}]}