{"thread":{"id":"45098","subject":"[PATCH v2] gc: ignore old gc.log files","startedAt":"2017-02-10T07:21:00Z","lastAt":"2017-02-10T09:17:05Z","messageCount":2,"participants":["David Turner","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"311250","messageId":"20170209191724.3987-1-dturner@twosigma.com","threadId":"45098","inReplyTo":null,"subject":"[PATCH v2] gc: ignore old gc.log files","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2017-02-09T19:17:24Z","receivedAt":"2017-02-10T07:21:00Z","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.logExpiry\nto manage this.  There is also some cleanup: a successful manual gc,\nor a warning-free auto gc with an old log file, will remove any old\ngc.log files.\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             | 42 +++++++++++++++++++++++++++++++++++-------\n t/t6500-gc.sh            | 13 +++++++++++++\n 3 files changed, 53 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex fc5a28a32..2c2c9c75c 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.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 possible values.\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..46edcff30 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 unsigned long gc_log_expire_time;\n static const char *prune_expire = \"2.weeks.ago\";\n static const char *prune_worktrees_expire = \"3.months.ago\";\n \n@@ -76,10 +77,12 @@ 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) && st.st_size) {\n \t\tcommit_lock_file(&log_lock);\n-\telse\n+\t} else {\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@@ -111,6 +114,11 @@ 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+\n+\tif (!git_config_get_value(\"gc.logexpiry\", &value)) {\n+\t\tparse_expiry_date(value, &gc_log_expire_time);\n+\t}\n+\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@@ -290,19 +298,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(_(\"Can't read %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,6 +372,8 @@ 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+\tparse_expiry_date(\"1.day\", &gc_log_expire_time);\n \tgc_config();\n \n \tif (pack_refs < 0)\n@@ -448,5 +473,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..84ad07eb2 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":"311251","messageId":"xmqq37fmmmma.fsf@gitster.mtv.corp.google.com","threadId":"45098","inReplyTo":"20170209191724.3987-1-dturner@twosigma.com","subject":"Re: [PATCH v2] gc: ignore old gc.log files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-10T09:16:45Z","receivedAt":"2017-02-10T09:17:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twosigma.com> writes:\n\n> The intent of automatic gc is to have a git repository be relatively\n> low-maintenance from a server-operator perspective.  \n\nThis is diametrically opposite from how I recall the auto-gc came\nabout in Sep 2007.  The primary purpose was to help desktop clients\nthat never runs repack.\n\nBy pointing this out, I do not mean that we shouldn't make auto-gc\nwork well in the server settings.  I however do not want our log\nmessages to distort history in order to justify a change that is\nworth making, and I do not think this change needs to do that.\nFor example, a paragraph like this:\n\n    It would be really nice if the auto gc mechanism can be used to\n    help server operators, even though the original purpose it was\n    introduced was primarily to help desktop clients that never\n    repacks.\n\nfollowed by a description of what makes it not exactly helpful for\nserver operators in the current behaviour (iow, \"what is it that you\nare fixing?\"), would be a useful justification that is faithful to\nthe history.  Of course, \", even though...\" part is irrelevant\nand/or unnecessary in that description of the motivation, and if you\nomit it, I wouldn't call that is distorting the history.\n\n> Of course, large\n> operators like GitHub will need a more complicated management strategy,\n> but for ordinary usage, git should just work.\n\nTrue.  \"should just work\" may want to be replaced by what exactly\nare the things it currently does that you view as its problems.\n\nOnce you say that, \"git learns to do x and y\" in the next paragraph,\ni.e. the description of the solution to the problem, starts making\nsense.\n\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.logExpiry\n> to manage this.  There is also some cleanup: a successful manual gc,\n> or a warning-free auto gc with an old log file, will remove any old\n> gc.log files.\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\nIOW, what you wrote in this last paragraph can come earlier to\nexplain what you perceive as problems the current behaviour has.\n\n> Signed-off-by: David Turner <dturner@twosigma.com>\n> Helped-by: Jeff King <peff@peff.net>\n> ---\n\nA v2 patch is unfriendly to reviewers unless it is sent with summary\nof what got changed since v1, taking input from the discussion on\nthe previous round, and here before the diffstat is a good place to\ndo so.\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 possible values.\n> +\n\nMicronit.  Perhaps you meant by \"more possible values\" \"more ways to\nspecify its values\", IOW, you didn't mean to say \"instead of 1.day,\nyou can say 2.days\".\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 331f21926..46edcff30 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 unsigned long gc_log_expire_time;\n>  static const char *prune_expire = \"2.weeks.ago\";\n>  static const char *prune_worktrees_expire = \"3.months.ago\";\n>  \n> @@ -76,10 +77,12 @@ 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) && st.st_size) {\n>  \t\tcommit_lock_file(&log_lock);\n> -\telse\n> +\t} else {\n> +\t\tunlink(git_path(\"gc.log\"));\n>  \t\trollback_lock_file(&log_lock);\n\nAfter we grab a lock by creating gc.log.lock, if we fail to fstat(2),\nwe remove gc.log?  That does not sound quite right, as the failure\nto fstat(2) sounds like a log-worthy event.  Removing the log after\nnoticing that we didn't write anything (i.e. st.st_size being 0) is\nquite sensible, though.\n\n> @@ -111,6 +114,11 @@ 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> +\n> +\tif (!git_config_get_value(\"gc.logexpiry\", &value)) {\n> +\t\tparse_expiry_date(value, &gc_log_expire_time);\n> +\t}\n\nDrop {}?\n\n> @@ -290,19 +298,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(_(\"Can't read %s\"), gc_log_path);\n\nYou probably want to use error_errno() instead here.  This is not\n\"can't read\"; your stat() noticed there is something wrong and you\ngave up before you even attempted to read.\n\n> +\t\tgoto done;\n> +\t}\n> +\n> +\tif (st.st_mtime < gc_log_expire_time)\n> +\t\tgoto done;\n\nOK.\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\nOK.\n\n> @@ -349,6 +372,8 @@ 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> +\tparse_expiry_date(\"1.day\", &gc_log_expire_time);\n\nAlternatively, we can mimick the way in which prune_expire and\nprune_worktrees_expire are set up (i.e. they are kept as strings,\nconfiguration overwrites the string), and then turn the final string\ninto value after gc_config() returns.  I think what you wrote here\nmay be simpler.  Nice.\n\n>  \tgc_config();\n>  \n>  \tif (pack_refs < 0)\n> @@ -448,5 +473,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\nOK.  We want to remove \"gc.log\" after running a successful\nforeground gc and this does exactly that.\n"}]}