Re: [PATCH] gc: remove gc.pid file at end of execution

[Duy Nguyen <pclouds@gmail.com>]

On Sat, Sep 28, 2013 at 7:33 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Matthieu Moy wrote:
>
>> This file isn't really harmful, but isn't useful either, and can create
>> minor annoyance for the user:
>
> Would something like the following make sense, to ensure the gc.pid file is
> always removed on normal exit?
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>
> diff --git c/builtin/gc.c i/builtin/gc.c
> index 6e0d81a..fbbc144 100644
> --- c/builtin/gc.c
> +++ i/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"
> @@ -167,6 +168,21 @@ static int need_to_gc(void)
>         return 1;
>  }
>
> +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);
> +}
> +
>  /* return NULL on success, else hostname running the gc */
>  static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
>  {
> @@ -179,13 +195,19 @@ 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");
>
> -       fd = hold_lock_file_for_update(&lock, git_path("gc.pid"),
> +       pidfile = git_pathdup("gc.pid");
> +
> +       fd = hold_lock_file_for_update(&lock, pidfile,
>                                        LOCK_DIE_ON_ERROR);
>         if (!force) {
> -               fp = fopen(git_path("gc.pid"), "r");
> +               fp = fopen(pidfile, "r");
>                 memset(locking_host, 0, sizeof(locking_host));
>                 should_exit =
>                         fp != NULL &&
> @@ -208,6 +230,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
>                 if (should_exit) {
>                         if (fd >= 0)
>                                 rollback_lock_file(&lock);
> +                       pidfile = NULL;
>                         *ret_pid = pid;
>                         return locking_host;
>                 }
> @@ -219,6 +242,9 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
>         strbuf_release(&sb);

It may be a bit simpler to delay setting pidfile until we get here.
lock.filename still contains gc.pid until commit_lock_file is called.

>         commit_lock_file(&lock);
>
> +       sigchain_push_common(remove_pidfile_on_signal);
> +       atexit(remove_pidfile);
> +
>         return NULL;
>  }
-- 
Duy