{"thread":{"id":"45105","subject":"[PATCH v3] gc: ignore old gc.log files","startedAt":"2017-02-10T19:20:51Z","lastAt":"2017-02-10T21:04:16Z","messageCount":7,"participants":["David Turner","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"311292","messageId":"20170210192019.13927-1-dturner@twosigma.com","threadId":"45105","inReplyTo":null,"subject":"[PATCH v3] gc: ignore old gc.log files","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2017-02-10T19:20:19Z","receivedAt":"2017-02-10T19:20:51Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"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.\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\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 be\nbecause Git is too dumb to try again.\n\nAutomatic gc was intended to make client repositories be\nself-maintaining.  It would be good if automatic gc were also useful\nto server operators.  A server can end up in a state whre there are\nlots of unreferenced loose objects (say, because many users are doing\na bunch of rebasing and pushing their rebased branches). Before this\npatch, this state would cause a gc.log file to be created, preventing\nfuture auto gcs.  Then pack files could pile up.  Since many git\noperations are O(n) in the number of pack files, this would lead to\npoor performance.  Now, the 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\nSigned-off-by: David Turner <dturner@twosigma.com>\nHelped-by: Jeff King <peff@peff.net>\n---\n Documentation/config.txt |  6 ++++\n builtin/gc.c             | 75 +++++++++++++++++++++++++++++++++++++++++++-----\n t/t6500-gc.sh            | 13 +++++++++\n 3 files changed, 87 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..6f297aa91 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,47 @@ 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\tif (errno == ENOENT) {\n+\t\t\t/*\n+\t\t\t * The user has probably intentionally deleted\n+\t\t\t * gc.log.lock (perhaps because they're blowing\n+\t\t\t * away the whole repo), so thre's no need to\n+\t\t\t * report anything here.  But we also won't\n+\t\t\t * delete gc.log, because we don't know what\n+\t\t\t * the user's intentions are.\n+\t\t\t */\n+\t\t} else {\n+\t\t\tFILE *fp;\n+\t\t\tint fd;\n+\t\t\tint saved_errno = errno;\n+\t\t\t/*\n+\t\t\t * Perhaps there was an i/o error or another\n+\t\t\t * unlikely situation.  Try to make a note of\n+\t\t\t * this in gc.log.  If this fails again,\n+\t\t\t * give up and leave gc.log as it was.\n+\t\t\t */\n+\t\t\trollback_lock_file(&log_lock);\n+\t\t\tfd = hold_lock_file_for_update(&log_lock,\n+\t\t\t\t\t\t       git_path(\"gc.log\"),\n+\t\t\t\t\t\t       LOCK_DIE_ON_ERROR);\n+\n+\t\t\tfp = fdopen(fd, \"w\");\n+\t\t\tfprintf(fp, _(\"Failed to fstat %s: %s\"),\n+\t\t\t\tget_tempfile_path(&log_lock.tempfile),\n+\t\t\t\tstrerror(errno));\n+\t\t\tfclose(fp);\n+\t\t\tcommit_lock_file(&log_lock);\n+\t\t}\n+\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-\telse\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 +151,9 @@ 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+\tif (!git_config_get_value(\"gc.logexpiry\", &value))\n+\t\tparse_expiry_date(value, &gc_log_expire_time);\n+\n \tgit_config(git_default_config, NULL);\n }\n \n@@ -290,19 +331,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+\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, 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\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 +405,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 +506,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":"311297","messageId":"xmqqy3xdkeuw.fsf@gitster.mtv.corp.google.com","threadId":"45105","inReplyTo":"20170210192019.13927-1-dturner@twosigma.com","subject":"Re: [PATCH v3] gc: ignore old gc.log files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-10T19:47:19Z","receivedAt":"2017-02-10T19:47:27Z","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> It would be good if automatic gc were useful to server operators.\n> A server can end up in a state whre there are\n> lots of unreferenced loose objects (say, because many users are doing\n> a bunch of rebasing and pushing their rebased branches). Before this\n> patch, this state would cause a gc.log file to be created, preventing\n> future auto gcs.  Then pack files could pile up.  Since many git\n> operations are O(n) in the number of pack files, this would lead to\n> poor performance.  Now, the pack files will get cleaned up, if\n> necessary, at least once per day.  And operators who find a need for\n> more-frequent gcs can adjust gc.logExpiry to meet their needs.\n>\n> 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.\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> It might still happen that manual intervention is required\n> (e.g. because the repo is corrupt), but at the very least it won't be\n> because Git is too dumb to try again.\n\nThanks, nicely explained.\n\n> +\tif (fstat(get_lock_file_fd(&log_lock), &st)) {\n> +\t\tif (errno == ENOENT) {\n> +\t\t\t/*\n> +\t\t\t * The user has probably intentionally deleted\n> +\t\t\t * gc.log.lock (perhaps because they're blowing\n> +\t\t\t * away the whole repo), so thre's no need to\n> +\t\t\t * report anything here.  But we also won't\n> +\t\t\t * delete gc.log, because we don't know what\n> +\t\t\t * the user's intentions are.\n> +\t\t\t */\n\nOK.\n\n> +\t\t} else {\n> +\t\t\tFILE *fp;\n> +\t\t\tint fd;\n> +\t\t\tint saved_errno = errno;\n> +\t\t\t/*\n> +\t\t\t * Perhaps there was an i/o error or another\n> +\t\t\t * unlikely situation.  Try to make a note of\n> +\t\t\t * this in gc.log.  If this fails again,\n> +\t\t\t * give up and leave gc.log as it was.\n> +\t\t\t */\n> +\t\t\trollback_lock_file(&log_lock);\n> +\t\t\tfd = hold_lock_file_for_update(&log_lock,\n> +\t\t\t\t\t\t       git_path(\"gc.log\"),\n> +\t\t\t\t\t\t       LOCK_DIE_ON_ERROR);\n> +\n> +\t\t\tfp = fdopen(fd, \"w\");\n> +\t\t\tfprintf(fp, _(\"Failed to fstat %s: %s\"),\n> +\t\t\t\tget_tempfile_path(&log_lock.tempfile),\n> +\t\t\t\tstrerror(errno));\n\nI think you meant to use saved_errno in this message and then\n\n> +\t\t\tfclose(fp);\n> +\t\t\tcommit_lock_file(&log_lock);\n\npossibly assign it back to errno around here?\n\n> +\t\t}\n> +\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> -\telse\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> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index 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\nYou could save one process with:\n\n    ls .git/objects/pack/*.pack | sed -e \"s/pack$/keep/\" -e q\n\nbut do you even need $keep?  I do not see it used below.\n\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\nOther than that I didn't spot anything suspicious.  I'll wait to see\nwhat others may want to add.\n\nThanks.\n"},{"id":"311300","messageId":"xmqqtw81kef3.fsf@gitster.mtv.corp.google.com","threadId":"45105","inReplyTo":"xmqqy3xdkeuw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3] gc: ignore old gc.log files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-10T19:56:48Z","receivedAt":"2017-02-10T19:58:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> David Turner <dturner@twosigma.com> writes:\n> ...\n>> It might still happen that manual intervention is required\n>> (e.g. because the repo is corrupt), but at the very least it won't be\n>> because Git is too dumb to try again.\n>\n> Thanks, nicely explained.\n\nSorry but I spotted a typo \"whre\" and ended up updating the proposed\nlog message by reordering them, omitting redundant info, etc.  Here\nis a proposed amend with the \"where does saved-errno go?\" and \"we do\nnot seem to use keep\" fix-ups squashed in.\n\n-- >8 --\nSubject: gc: ignore old gc.log files\n\nA 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             | 76 +++++++++++++++++++++++++++++++++++++++++++-----\n t/t6500-gc.sh            | 12 ++++++++\n 3 files changed, 87 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1fee83ca42..d385711b70 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1397,6 +1397,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 331f219260..55c441115d 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,48 @@ 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\tif (errno == ENOENT) {\n+\t\t\t/*\n+\t\t\t * The user has probably intentionally deleted\n+\t\t\t * gc.log.lock (perhaps because they're blowing\n+\t\t\t * away the whole repo), so thre's no need to\n+\t\t\t * report anything here.  But we also won't\n+\t\t\t * delete gc.log, because we don't know what\n+\t\t\t * the user's intentions are.\n+\t\t\t */\n+\t\t} else {\n+\t\t\tFILE *fp;\n+\t\t\tint fd;\n+\t\t\tint saved_errno = errno;\n+\t\t\t/*\n+\t\t\t * Perhaps there was an i/o error or another\n+\t\t\t * unlikely situation.  Try to make a note of\n+\t\t\t * this in gc.log.  If this fails again,\n+\t\t\t * give up and leave gc.log as it was.\n+\t\t\t */\n+\t\t\trollback_lock_file(&log_lock);\n+\t\t\tfd = hold_lock_file_for_update(&log_lock,\n+\t\t\t\t\t\t       git_path(\"gc.log\"),\n+\t\t\t\t\t\t       LOCK_DIE_ON_ERROR);\n+\n+\t\t\tfp = fdopen(fd, \"w\");\n+\t\t\tfprintf(fp, _(\"Failed to fstat %s: %s\"),\n+\t\t\t\tget_tempfile_path(&log_lock.tempfile),\n+\t\t\t\tstrerror(saved_errno));\n+\t\t\tfclose(fp);\n+\t\t\tcommit_lock_file(&log_lock);\n+\t\t\terrno = saved_errno;\n+\t\t}\n+\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-\telse\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 +152,9 @@ 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+\tif (!git_config_get_value(\"gc.logexpiry\", &value))\n+\t\tparse_expiry_date(value, &gc_log_expire_time);\n+\n \tgit_config(git_default_config, NULL);\n }\n \n@@ -290,19 +332,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+\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, 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\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 +406,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 +507,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 1762dfa6a3..e1fb9b4d5b 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -67,5 +67,17 @@ 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-chmtime =-86401 .git/gc.log &&\n+\tgit gc --auto\n+'\n \n test_done\n"},{"id":"311303","messageId":"20170210200838.kuwpldsgzvkjlmri@sigill.intra.peff.net","threadId":"45105","inReplyTo":"20170210192019.13927-1-dturner@twosigma.com","subject":"Re: [PATCH v3] gc: ignore old gc.log files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-10T20:08:39Z","receivedAt":"2017-02-10T20:08:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 10, 2017 at 02:20:19PM -0500, David Turner wrote:\n\n> @@ -76,10 +77,47 @@ 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\tif (errno == ENOENT) {\n> +\t\t\t/*\n> +\t\t\t * The user has probably intentionally deleted\n> +\t\t\t * gc.log.lock (perhaps because they're blowing\n> +\t\t\t * away the whole repo), so thre's no need to\n> +\t\t\t * report anything here.  But we also won't\n> +\t\t\t * delete gc.log, because we don't know what\n> +\t\t\t * the user's intentions are.\n> +\t\t\t */\n\nHrm. Does fstat actually trigger ENOENT in that case? There's no\npathname lookup happening at all. A simple test on Linux seems to show\nthat it does not. Build:\n\n\t#include <unistd.h>\n\t#include <fcntl.h>\n\t#include <sys/stat.h>\n\t\n\tint main(int argc, char **argv)\n\t{\n\t\tstruct stat st;\n\t\tint fd = open(argv[1], O_WRONLY|O_CREAT|O_EXCL, 0600);\n\t\tunlink(argv[1]);\n\t\tfstat(fd, &st);\n\t\treturn 0;\n\t}\n\nand run:\n\n  strace ./a.out tmp\n\nwhich shows:\n\n  open(\"tmp\", O_WRONLY|O_CREAT|O_EXCL, 056660) = 3\n  unlink(\"tmp\")                           = 0\n  fstat(3, {st_mode=S_IFREG|S_ISUID|S_ISGID|0640, st_size=0, ...}) = 0\n\nBut maybe there are other systems emulating fstat() would trigger here.\nI dunno.\n\n> +\t\t} else {\n> +\t\t\tFILE *fp;\n> +\t\t\tint fd;\n> +\t\t\tint saved_errno = errno;\n> +\t\t\t/*\n> +\t\t\t * Perhaps there was an i/o error or another\n> +\t\t\t * unlikely situation.  Try to make a note of\n> +\t\t\t * this in gc.log.  If this fails again,\n> +\t\t\t * give up and leave gc.log as it was.\n> +\t\t\t */\n> +\t\t\trollback_lock_file(&log_lock);\n> +\t\t\tfd = hold_lock_file_for_update(&log_lock,\n> +\t\t\t\t\t\t       git_path(\"gc.log\"),\n> +\t\t\t\t\t\t       LOCK_DIE_ON_ERROR);\n\nIf there was an i/o error, then gc.log.lock still exists. And this\nattempt to lock will therefore fail, calling die(). Which would trigger\nour atexit() to call process_log(), which would hit this code again, and\nso forth. I'm not sure if we'd actually recurse when an atexit handler\ncalls exit(). But it seems questionable.\n\nI'm also not sure why we need to re-open the file in the first place. We\nhave an open descriptor (and we even redirected stderr to it already).\nWhy don't we just write to it?\n\n> @@ -113,6 +151,9 @@ 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> +\tif (!git_config_get_value(\"gc.logexpiry\", &value))\n> +\t\tparse_expiry_date(value, &gc_log_expire_time);\n> +\n\nShould you be using git_config_date_string() here? It looks like it does\nsome extra sanity-checking. It annoyingly just gets the string, and\ndoesn't parse it. Perhaps it would be worth adding a\ngit_config_date_value() helper.\n\nOr alternatively, save the date string here, and then parse once later\non, after having resolved all config (and overwritten the default\nvalue).\n\n> @@ -448,5 +506,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\nI think this is a good thing to do, though I'd have probably put it in a\nseparate patch. I guess that's a matter of taste.\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\nThis gives only 1 second of leeway. I wonder if we could end up getting\nbogus failures due to system clock adjustments, or even skew between the\nfilesystem and OS clocks. Perhaps we should set it farther back, like a\nfew days.\n\n(It also relies on the 1-day default. That's probably OK, though we\ncould also set an explicit default for the test, which would exercise\nthe config code path, too).\n\n-Peff\n"},{"id":"311306","messageId":"20170210205138.5nnexap32pkbjjrk@sigill.intra.peff.net","threadId":"45105","inReplyTo":"7852bf6688ed487097d4f997ac72dcba@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH v3] gc: ignore old gc.log files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-10T20:51:38Z","receivedAt":"2017-02-10T20:51:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 10, 2017 at 08:44:27PM +0000, David Turner wrote:\n\n> > But maybe there are other systems emulating fstat() would trigger here.\n> > I dunno.\n> \n> Yeah, I'm also not sure.  On the other hand, if we're going to catch fstat \n> errors anyway, we might as well do something sensible with this one.\n\nI'd say it's probably not worth worrying about here. We don't think it\ncan happen, and it would just fall-through to the \"woah, fstat failed\"\ncode path if it does.\n\n> > If there was an i/o error, then gc.log.lock still exists. And this attempt to lock will\n> > therefore fail, calling die(). Which would trigger our atexit() to call process_log(),\n> > which would hit this code again, and so forth. I'm not sure if we'd actually\n> > recurse when an atexit handler calls exit(). But it seems questionable.\n> \n> No, because  we call rollback_log_file first.\n\nAh, right, sorry I missed that.\n\n> > I'm also not sure why we need to re-open the file in the first place. We have an\n> > open descriptor (and we even redirected stderr to it already).\n> > Why don't we just write to it?\n> \n> If fstat failed, that probably indicates something bad about the old fd.  I'm not \n> actually sure why fstat would ever fail, since in all likelihood, the kernel keeps \n> information about inodes corresponding to open fds in-memory.  Maybe someone\n> forcibly unmounted the drive?\n\nIt seems like the re-open would fail then, too. And the error message\nfor that would go to stderr, which goes to...the old file.\n\nI dunno. This seems like a lot of manual scrambling to try to overcome\nunlikely errors just to make our stderr heard (and we'd still fail in\nsome cases). It seems like:\n\n  if (fstat(...)) {\n\t/* weird, fstat failed; try our best to mention it */\n\terror_errno(\"unable to fstat gc.log.lock\");\n\tcommit_lock_file(&log_lock));\n  } else if (st.st_size) {\n\t/* we have new errors to report */\n\tcommit_lock_file(&log_lock);\n  } else {\n\t/* no new errors; clean up old ones */\n\tunlink(git_path(\"gc.log\"));\n\trollback_lock_file(&log_lock);\n  }\n\nwould be sufficient.\n\n-Peff\n"},{"id":"311307","messageId":"7852bf6688ed487097d4f997ac72dcba@exmbdft7.ad.twosigma.com","threadId":"45105","inReplyTo":"20170210200838.kuwpldsgzvkjlmri@sigill.intra.peff.net","subject":"RE: [PATCH v3] gc: ignore old gc.log files","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-02-10T20:44:27Z","receivedAt":"2017-02-10T20:54:18Z","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 3:09 PM\n> To: David Turner <David.Turner@twosigma.com>\n> Cc: git@vger.kernel.org; pclouds@gmail.com\n> Subject: Re: [PATCH v3] gc: ignore old gc.log files\n> \n> On Fri, Feb 10, 2017 at 02:20:19PM -0500, David Turner wrote:\n> \n> > @@ -76,10 +77,47 @@ 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\tif (errno == ENOENT) {\n> > +\t\t\t/*\n> > +\t\t\t * The user has probably intentionally deleted\n> > +\t\t\t * gc.log.lock (perhaps because they're blowing\n> > +\t\t\t * away the whole repo), so thre's no need to\n> > +\t\t\t * report anything here.  But we also won't\n> > +\t\t\t * delete gc.log, because we don't know what\n> > +\t\t\t * the user's intentions are.\n> > +\t\t\t */\n> \n> Hrm. Does fstat actually trigger ENOENT in that case? There's no pathname\n> lookup happening at all. A simple test on Linux seems to show that it does not.\n> Build:\n> \n> \t#include <unistd.h>\n> \t#include <fcntl.h>\n> \t#include <sys/stat.h>\n> \n> \tint main(int argc, char **argv)\n> \t{\n> \t\tstruct stat st;\n> \t\tint fd = open(argv[1], O_WRONLY|O_CREAT|O_EXCL, 0600);\n> \t\tunlink(argv[1]);\n> \t\tfstat(fd, &st);\n> \t\treturn 0;\n> \t}\n> \n> and run:\n> \n>   strace ./a.out tmp\n> \n> which shows:\n> \n>   open(\"tmp\", O_WRONLY|O_CREAT|O_EXCL, 056660) = 3\n>   unlink(\"tmp\")                           = 0\n>   fstat(3, {st_mode=S_IFREG|S_ISUID|S_ISGID|0640, st_size=0, ...}) = 0\n> \n> But maybe there are other systems emulating fstat() would trigger here.\n> I dunno.\n\nYeah, I'm also not sure.  On the other hand, if we're going to catch fstat \nerrors anyway, we might as well do something sensible with this one.\n\n> > +\t\t} else {\n> > +\t\t\tFILE *fp;\n> > +\t\t\tint fd;\n> > +\t\t\tint saved_errno = errno;\n> > +\t\t\t/*\n> > +\t\t\t * Perhaps there was an i/o error or another\n> > +\t\t\t * unlikely situation.  Try to make a note of\n> > +\t\t\t * this in gc.log.  If this fails again,\n> > +\t\t\t * give up and leave gc.log as it was.\n> > +\t\t\t */\n> > +\t\t\trollback_lock_file(&log_lock);\n> > +\t\t\tfd = hold_lock_file_for_update(&log_lock,\n> > +\t\t\t\t\t\t       git_path(\"gc.log\"),\n> > +\t\t\t\t\t\t       LOCK_DIE_ON_ERROR);\n> \n> If there was an i/o error, then gc.log.lock still exists. And this attempt to lock will\n> therefore fail, calling die(). Which would trigger our atexit() to call process_log(),\n> which would hit this code again, and so forth. I'm not sure if we'd actually\n> recurse when an atexit handler calls exit(). But it seems questionable.\n\nNo, because  we call rollback_log_file first.\n\n> I'm also not sure why we need to re-open the file in the first place. We have an\n> open descriptor (and we even redirected stderr to it already).\n> Why don't we just write to it?\n\nIf fstat failed, that probably indicates something bad about the old fd.  I'm not \nactually sure why fstat would ever fail, since in all likelihood, the kernel keeps \ninformation about inodes corresponding to open fds in-memory.  Maybe someone\nforcibly unmounted the drive?  \n\n> > @@ -113,6 +151,9 @@ 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\",\n> > &prune_worktrees_expire);\n> > +\tif (!git_config_get_value(\"gc.logexpiry\", &value))\n> > +\t\tparse_expiry_date(value, &gc_log_expire_time);\n> > +\n> \n> Should you be using git_config_date_string() here? It looks like it does some\n> extra sanity-checking. It annoyingly just gets the string, and doesn't parse it.\n> Perhaps it would be worth adding a\n> git_config_date_value() helper.\n\nThat seems like a good idea, but I'm going to skip it for now and promise to\ndo it next time I need a date config.\n\n> Or alternatively, save the date string here, and then parse once later on, after\n> having resolved all config (and overwritten the default value).\n\nSure.\n\n> > @@ -448,5 +506,8 @@ int cmd_gc(int argc, const char **argv, const char\n> *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> \n> I think this is a good thing to do, though I'd have probably put it in a separate\n> patch. I guess that's a matter of taste.\n\nI could go either way, but since I've already gone this way, I'll stick with it.\n\n> > +test_expect_success 'background auto gc does not run if gc.log is present\n> 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> This gives only 1 second of leeway. I wonder if we could end up getting bogus\n> failures due to system clock adjustments, or even skew between the filesystem\n> and OS clocks. Perhaps we should set it farther back, like a few days.\n> \n> (It also relies on the 1-day default. That's probably OK, though we could also set\n> an explicit default for the test, which would exercise the config code path, too).\n\nSure, I'll re-roll with that change.\n"},{"id":"311310","messageId":"xmqqlgtdkbau.fsf@gitster.mtv.corp.google.com","threadId":"45105","inReplyTo":"20170210205138.5nnexap32pkbjjrk@sigill.intra.peff.net","subject":"Re: [PATCH v3] gc: ignore old gc.log files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-10T21:04:09Z","receivedAt":"2017-02-10T21:04:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I dunno. This seems like a lot of manual scrambling to try to overcome\n> unlikely errors just to make our stderr heard (and we'd still fail in\n> some cases). It seems like:\n>\n>   if (fstat(...)) {\n> \t/* weird, fstat failed; try our best to mention it */\n> \terror_errno(\"unable to fstat gc.log.lock\");\n> \tcommit_lock_file(&log_lock));\n>   } else if (st.st_size) {\n> \t/* we have new errors to report */\n> \tcommit_lock_file(&log_lock);\n>   } else {\n> \t/* no new errors; clean up old ones */\n> \tunlink(git_path(\"gc.log\"));\n> \trollback_lock_file(&log_lock);\n>   }\n>\n> would be sufficient.\n\nYeah, that should be sufficient.\n"}]}