{"thread":{"id":"45109","subject":"[PATCH v5] gc: ignore old gc.log files","startedAt":"2017-02-10T21:00:27Z","lastAt":"2017-02-10T21:32:33Z","messageCount":4,"participants":["David Turner","Jeff King"],"isPatch":true,"patchVersion":5,"patchTotal":null},"messages":[{"id":"311309","messageId":"20170210205931.5348-1-dturner@twosigma.com","threadId":"45109","inReplyTo":null,"subject":"[PATCH v5] gc: ignore old gc.log files","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2017-02-10T20:59:31Z","receivedAt":"2017-02-10T21:00:27Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"A server can end up in a state where there are lots of unreferenced\nloose objects (say, because many users are doing a bunch of rebasing\nand pushing their rebased branches).  Running \"git gc --auto\" in\nthis state would cause a gc.log file to be created, preventing\nfuture auto gcs, causing pack files to pile up.  Since many git\noperations are O(n) in the number of pack files, this would lead to\npoor performance.\n\nGit 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.\n\nTeach Git to ignore gc.log files which are older than (by default)\none day old, which can be tweaked via the gc.logExpiry configuration\nvariable.  That way, these pack files will get cleaned up, if\nnecessary, at least once per day.  And operators who find a need for\nmore-frequent gcs can adjust gc.logExpiry to meet their needs.\n\nThere is also some cleanup: a successful manual gc, or a\nwarning-free auto gc with an old log file, will remove any old\ngc.log files.\n\nIt might still happen that manual intervention is required\n(e.g. because the repo is corrupt), but at the very least it won't\nbe because Git is too dumb to try again.\n\nSigned-off-by: David Turner <dturner@twosigma.com>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/config.txt |  6 +++++\n builtin/gc.c             | 59 ++++++++++++++++++++++++++++++++++++++++++------\n t/t6500-gc.sh            | 15 ++++++++++++\n 3 files changed, 73 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex fc5a28a32..a684b7e3e 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1402,6 +1402,12 @@ 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.logExpiry::\n+\tIf the file gc.log exists, then `git gc --auto` won't run\n+\tunless that file is more than 'gc.logExpiry' old.  Default is\n+\t\"1.day\".  See `gc.pruneExpire` for more ways to specify its\n+\tvalue.\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..8d355feb0 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -33,6 +33,8 @@ 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 unsigned long gc_log_expire_time;\n+static const char *gc_log_expire = \"1.day.ago\";\n static const char *prune_expire = \"2.weeks.ago\";\n static const char *prune_worktrees_expire = \"3.months.ago\";\n \n@@ -76,10 +78,30 @@ static void git_config_date_string(const char *key, const char **output)\n static void process_log_file(void)\n {\n \tstruct stat st;\n-\tif (!fstat(get_lock_file_fd(&log_lock), &st) && st.st_size)\n+\tif (fstat(get_lock_file_fd(&log_lock), &st)) {\n+\t\t/*\n+\t\t * Perhaps there was an i/o error or another\n+\t\t * unlikely situation.  Try to make a note of\n+\t\t * this in gc.log along with any existing\n+\t\t * messages.\n+\t\t */\n+\t\tFILE *fp;\n+\t\tint saved_errno = errno;\n+\t\tfp = fdopen(log_lock.tempfile.fd, \"a\");\n+\t\tfprintf(fp, _(\"Failed to fstat %s: %s\"),\n+\t\t\tget_tempfile_path(&log_lock.tempfile),\n+\t\t\tstrerror(saved_errno));\n+\t\tfclose(fp);\n \t\tcommit_lock_file(&log_lock);\n-\telse\n+\t\terrno = saved_errno;\n+\t} else if (st.st_size) {\n+\t\t/* There was some error recorded in the lock file */\n+\t\tcommit_lock_file(&log_lock);\n+\t} else {\n+\t\t/* No error, clean up any old gc.log */\n+\t\tunlink(git_path(\"gc.log\"));\n \t\trollback_lock_file(&log_lock);\n+\t}\n }\n \n static void process_log_file_at_exit(void)\n@@ -113,6 +135,8 @@ static void gc_config(void)\n \tgit_config_get_bool(\"gc.autodetach\", &detach_auto);\n \tgit_config_date_string(\"gc.pruneexpire\", &prune_expire);\n \tgit_config_date_string(\"gc.worktreepruneexpire\", &prune_worktrees_expire);\n+\tgit_config_date_string(\"gc.logexpiry\", &gc_log_expire);\n+\n \tgit_config(git_default_config, NULL);\n }\n \n@@ -290,19 +314,34 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n static int report_last_gc_error(void)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tint ret;\n+\tint ret = 0;\n+\tstruct stat st;\n+\tchar *gc_log_path = git_pathdup(\"gc.log\");\n \n-\tret = strbuf_read_file(&sb, git_path(\"gc.log\"), 0);\n+\tif (stat(gc_log_path, &st)) {\n+\t\tif (errno == ENOENT)\n+\t\t\tgoto done;\n+\n+\t\tret = error_errno(_(\"Can't stat %s\"), gc_log_path);\n+\t\tgoto done;\n+\t}\n+\n+\tif (st.st_mtime < gc_log_expire_time)\n+\t\tgoto done;\n+\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\tret = error(_(\"The last gc run reported the following. \"\n \t\t\t       \"Please correct the root cause\\n\"\n \t\t\t       \"and remove %s.\\n\"\n \t\t\t       \"Automatic cleanup will not be performed \"\n \t\t\t       \"until the file is removed.\\n\\n\"\n \t\t\t       \"%s\"),\n-\t\t\t     git_path(\"gc.log\"), sb.buf);\n+\t\t\t    gc_log_path, sb.buf);\n \tstrbuf_release(&sb);\n-\treturn 0;\n+done:\n+\tfree(gc_log_path);\n+\treturn ret;\n }\n \n static int gc_before_repack(void)\n@@ -349,7 +388,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \targv_array_pushl(&prune_worktrees, \"worktree\", \"prune\", \"--expire\", NULL);\n \targv_array_pushl(&rerere, \"rerere\", \"gc\", NULL);\n \n+\t/* default expiry time, overwritten in gc_config */\n \tgc_config();\n+\tif (parse_expiry_date(gc_log_expire, &gc_log_expire_time))\n+\t\tdie(_(\"Failed to parse gc.logexpiry value %s\"), gc_log_expire);\n \n \tif (pack_refs < 0)\n \t\tpack_refs = !is_bare_repository();\n@@ -448,5 +490,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\twarning(_(\"There are too many unreachable loose objects; \"\n \t\t\t\"run 'git prune' to remove them.\"));\n \n+\tif (!daemonized)\n+\t\tunlink(git_path(\"gc.log\"));\n+\n \treturn 0;\n }\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 1762dfa6a..08de2e8ab 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -67,5 +67,20 @@ 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+\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_config gc.logexpiry 5.days &&\n+\ttest-chmtime =-345600 .git/gc.log &&\n+\ttest_must_fail git gc --auto &&\n+\ttest_config gc.logexpiry 2.days &&\n+\tgit gc --auto\n+'\n \n test_done\n-- \n2.11.GIT\n\n"},{"id":"311311","messageId":"20170210211510.zbustcl5ickfiqdy@sigill.intra.peff.net","threadId":"45109","inReplyTo":"20170210205931.5348-1-dturner@twosigma.com","subject":"Re: [PATCH v5] gc: ignore old gc.log files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-10T21:15:10Z","receivedAt":"2017-02-10T21:15:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"> @@ -76,10 +78,30 @@ static void git_config_date_string(const char *key, const char **output)\n>  static void process_log_file(void)\n>  {\n>  \tstruct stat st;\n> -\tif (!fstat(get_lock_file_fd(&log_lock), &st) && st.st_size)\n> +\tif (fstat(get_lock_file_fd(&log_lock), &st)) {\n> +\t\t/*\n> +\t\t * Perhaps there was an i/o error or another\n> +\t\t * unlikely situation.  Try to make a note of\n> +\t\t * this in gc.log along with any existing\n> +\t\t * messages.\n> +\t\t */\n> +\t\tFILE *fp;\n> +\t\tint saved_errno = errno;\n> +\t\tfp = fdopen(log_lock.tempfile.fd, \"a\");\n\nWe usually use xfdopen() to handle (unlikely) errors rather than\nsegfaulting.  But I think you'd actually want fdopen_lock_file(), which\nattaches the fd to the tempfile for flushing and cleanup purposes.\n\nThat said, I'm not sure I understand why you're opening a new stdio\nfilehandle. We know that stderr already points to our logfile (that's\nhow content gets there in the first place). If there's a problem with\nthe file or the descriptor, opening a new filehandle around the same\ndescriptor won't help.\n\nSpeaking of stderr, I wonder if this function should be calling\nfflush(stderr) before looking at the fstat result. There could be\ncontents buffered there that haven't been written out yet (not from\nchild processes, but perhaps ones written in this process itself).\nProbably unlikely in practice, since stderr is typically unbuffered by\ndefault.\n\n-Peff\n"},{"id":"311315","messageId":"069b229470204235b7155b33da52421c@exmbdft7.ad.twosigma.com","threadId":"45109","inReplyTo":"20170210211510.zbustcl5ickfiqdy@sigill.intra.peff.net","subject":"RE: [PATCH v5] gc: ignore old gc.log files","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-02-10T21:23:15Z","receivedAt":"2017-02-10T21:31:06Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"\n\n> -----Original Message-----\n> From: Jeff King [mailto:peff@peff.net]\n> Sent: Friday, February 10, 2017 4:15 PM\n> To: David Turner <David.Turner@twosigma.com>\n> Cc: git@vger.kernel.org; pclouds@gmail.com; Junio C Hamano\n> <gitster@pobox.com>\n> Subject: Re: [PATCH v5] gc: ignore old gc.log files\n> \n> > @@ -76,10 +78,30 @@ static void git_config_date_string(const char\n> > *key, const char **output)  static void process_log_file(void)  {\n> >  \tstruct stat st;\n> > -\tif (!fstat(get_lock_file_fd(&log_lock), &st) && st.st_size)\n> > +\tif (fstat(get_lock_file_fd(&log_lock), &st)) {\n> > +\t\t/*\n> > +\t\t * Perhaps there was an i/o error or another\n> > +\t\t * unlikely situation.  Try to make a note of\n> > +\t\t * this in gc.log along with any existing\n> > +\t\t * messages.\n> > +\t\t */\n> > +\t\tFILE *fp;\n> > +\t\tint saved_errno = errno;\n> > +\t\tfp = fdopen(log_lock.tempfile.fd, \"a\");\n> \n> We usually use xfdopen() to handle (unlikely) errors rather than segfaulting.  But\n> I think you'd actually want fdopen_lock_file(), which attaches the fd to the\n> tempfile for flushing and cleanup purposes.\n> \n> That said, I'm not sure I understand why you're opening a new stdio filehandle.\n> We know that stderr already points to our logfile (that's how content gets there\n> in the first place). If there's a problem with the file or the descriptor, opening a\n> new filehandle around the same descriptor won't help.\n> \n> Speaking of stderr, I wonder if this function should be calling\n> fflush(stderr) before looking at the fstat result. There could be contents buffered\n> there that haven't been written out yet (not from child processes, but perhaps\n> ones written in this process itself).\n> Probably unlikely in practice, since stderr is typically unbuffered by default.\n\nProcess_log_file_at_exit calls fflush.  Will fix the other.\n"},{"id":"311316","messageId":"20170210213227.wviuppcqb42kk3ww@sigill.intra.peff.net","threadId":"45109","inReplyTo":"069b229470204235b7155b33da52421c@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH v5] gc: ignore old gc.log files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-10T21:32:27Z","receivedAt":"2017-02-10T21:32:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 10, 2017 at 09:23:15PM +0000, David Turner wrote:\n\n> > Speaking of stderr, I wonder if this function should be calling\n> > fflush(stderr) before looking at the fstat result. There could be contents buffered\n> > there that haven't been written out yet (not from child processes, but perhaps\n> > ones written in this process itself).\n> > Probably unlikely in practice, since stderr is typically unbuffered by default.\n> \n> Process_log_file_at_exit calls fflush.  Will fix the other.\n\nAh, good. That makes sense, since we might deadlock if we do it in a\nsignal handler. Perhaps that is a reason not to use stderr here again\n(though if we want to be that careful, a new fdopen() call is also a bad\nidea, as we can deadlock over the malloc() lock; you'd have to snprintf\nto a small buffer and dump it with write()).\n\n-Peff\n"}]}