David Turner [off-list ref] writes:
It would be good if automatic gc were useful to server operators.
A server can end up in a state whre there are
lots of unreferenced loose objects (say, because many users are doing
a bunch of rebasing and pushing their rebased branches). Before this
patch, this state would cause a gc.log file to be created, preventing
future auto gcs. Then pack files could pile up. Since many git
operations are O(n) in the number of pack files, this would lead to
poor performance. Now, the pack files will get cleaned up, if
necessary, at least once per day. And operators who find a need for
more-frequent gcs can adjust gc.logExpiry to meet their needs.
Git should never get itself into a state where it refuses to do any
maintenance, just because at some point some piece of the maintenance
didn't make progress.
In this commit, git learns to ignore gc.log files which are older than
(by default) one day old. It also learns about a config, gc.logExpiry
to manage this. There is also some cleanup: a successful manual gc,
or a warning-free auto gc with an old log file, will remove any old
gc.log files.
It might still happen that manual intervention is required
(e.g. because the repo is corrupt), but at the very least it won't be
because Git is too dumb to try again.
Thanks, nicely explained.
quoted hunk
+ if (fstat(get_lock_file_fd(&log_lock), &st)) {
+ if (errno == ENOENT) {
+ /*
+ * The user has probably intentionally deleted
+ * gc.log.lock (perhaps because they're blowing
+ * away the whole repo), so thre's no need to
+ * report anything here. But we also won't
+ * delete gc.log, because we don't know what
+ * the user's intentions are.
+ */
OK.
quoted hunk
+ } else {
+ FILE *fp;
+ int fd;
+ int saved_errno = errno;
+ /*
+ * Perhaps there was an i/o error or another
+ * unlikely situation. Try to make a note of
+ * this in gc.log. If this fails again,
+ * give up and leave gc.log as it was.
+ */
+ rollback_lock_file(&log_lock);
+ fd = hold_lock_file_for_update(&log_lock,
+ git_path("gc.log"),
+ LOCK_DIE_ON_ERROR);
+
+ fp = fdopen(fd, "w");
+ fprintf(fp, _("Failed to fstat %s: %s"),
+ get_tempfile_path(&log_lock.tempfile),
+ strerror(errno));
I think you meant to use saved_errno in this message and then
+ fclose(fp);
+ commit_lock_file(&log_lock);
possibly assign it back to errno around here?
quoted hunk
+ }
+
+ } else if (st.st_size) {
+ /* There was some error recorded in the lock file */
commit_lock_file(&log_lock);
- else
+ } else {
+ /* No error, clean up any old gc.log */
+ unlink(git_path("gc.log"));
rollback_lock_file(&log_lock);
+ }
}
quoted hunk
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 1762dfa6a..84ad07eb2 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -67,5 +67,18 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre
test_line_count = 2 new # There is one new pack and its .idx
'
+test_expect_success 'background auto gc does not run if gc.log is present and recent but does if it is old' '
+ keep=$(ls .git/objects/pack/*.pack|head -1|sed -e "s/pack$/keep/") &&
You could save one process with:
ls .git/objects/pack/*.pack | sed -e "s/pack$/keep/" -e q
but do you even need $keep? I do not see it used below.
quoted hunk
+ test_commit foo &&
+ test_commit bar &&
+ git repack &&
+ test_config gc.autopacklimit 1 &&
+ test_config gc.autodetach true &&
+ echo fleem >.git/gc.log &&
+ test_must_fail git gc --auto 2>err &&
+ test_i18ngrep "^error:" err &&
+ test-chmtime =-86401 .git/gc.log &&
+ git gc --auto
+'
test_done
Other than that I didn't spot anything suspicious. I'll wait to see
what others may want to add.
Thanks.