gc: remove gc.pid file at end of execution

This file isn't really harmful, but isn't useful either, and can create
minor annoyance for the user:

* It's confusing, as the presence of a *.pid file often implies that a
process is currently running. A user running "ls .git/" and finding
this file may incorrectly guess that a "git gc" is currently running.

* Leaving this file means that a "git gc" in an already gc-ed repo is
no-longer a no-op. A user running "git gc" in a set of repositories,
and then synchronizing this set (e.g. rsync -av, unison, ...) will see
all the gc.pid files as changed, which creates useless noise.

This patch unlinks the file after the garbage collection is done, so that
gc.pid is actually present only during execution.

Future versions of Git may want to use the information left in the gc.pid
file (e.g. for policies like "don't attempt to run a gc if one has
already been ran less than X hours ago"). If so, this patch can safely be
reverted. For now, let's not bother the users.

Explained-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Improved-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/gc.c b/builtin/gc.c
index 6e0d81ab32aa691d24dc86ba30f3bc282bdc2890..dd910fbc374a067046670448815aaf790e1d629b 100644 (file)
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -14,6 +14,7 @@
 #include "cache.h"
 #include "parse-options.h"
 #include "run-command.h"
+#include "sigchain.h"
 #include "argv-array.h"
 
 #define FAILED_RUN "failed to run %s"
@@ -35,6 +36,21 @@ static struct argv_array repack = ARGV_ARRAY_INIT;
 static struct argv_array prune = ARGV_ARRAY_INIT;
 static struct argv_array rerere = ARGV_ARRAY_INIT;
 
+static char *pidfile;
+
+static void remove_pidfile(void)
+{
+       if (pidfile)
+               unlink(pidfile);
+}
+
+static void remove_pidfile_on_signal(int signo)
+{
+       remove_pidfile();
+       sigchain_pop(signo);
+       raise(signo);
+}
+
 static int gc_config(const char *var, const char *value, void *cb)
 {
        if (!strcmp(var, "gc.packrefs")) {
@@ -179,6 +195,10 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
        FILE *fp;
        int fd, should_exit;
 
+       if (pidfile)
+               /* already locked */
+               return NULL;
+
        if (gethostname(my_host, sizeof(my_host)))
                strcpy(my_host, "unknown");
 
@@ -219,6 +239,10 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
        strbuf_release(&sb);
        commit_lock_file(&lock);
 
+       pidfile = git_pathdup("gc.pid");
+       sigchain_push_common(remove_pidfile_on_signal);
+       atexit(remove_pidfile);
+
        return NULL;
 }
 

diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index b1a63655f98dfc8d19c1a80f86652654cd9cade6..63194d819efe07af241573662c660e6e92b503f5 100755 (executable)
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -9,6 +9,11 @@ test_expect_success 'gc empty repository' '
        git gc
 '
 
+test_expect_success 'gc does not leave behind pid file' '
+       git gc &&
+       test_path_is_missing .git/gc.pid
+'
+
 test_expect_success 'gc --gobbledegook' '
        test_expect_code 129 git gc --nonsense 2>err &&
        test_i18ngrep "[Uu]sage: git gc" err