Re: [PATCH v4] gc: reject if another gc is running, unless --force is given

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 9, 2013, 17:41 UTC
Message-ID: <7vmwoq69td.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAPrKj1bO1jBsv73beA6LoeN09S-jWq8FYOP+WQ-AFwb1dn4Wsw@mail.gmail.com>

Andres Perera <andres.p@zoho.com> writes:

Show 12 quoted lines

>> +/* return NULL on success, else hostname running the gc */
>> +static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
>> +{
>> +       static struct lock_file lock;
>> +       static char locking_host[128];
>> +       char my_host[128];
>> +       struct strbuf sb = STRBUF_INIT;
>> +       struct stat st;
>> +       uintmax_t pid;
>
> pid_t is always an signed type, therefore unintmax_t does not make
> sense as a catch all value

Good eyes.

Show 11 quoted lines

>> +                       !strcmp(locking_host, my_host) &&
>> +                       !kill(pid, 0);
>> +               if (fp != NULL)
>> +                       fclose(fp);
>> +               if (should_exit) {
>> +                       if (fd >= 0)
>> +                               rollback_lock_file(&lock);
>> +                       *ret_pid = pid;
>> +                       return locking_host;
>
> why not exponential backoff?

If the other guy is doing a GC, and we decide that we should exit, it is *not* because we want to wait until the other guy is done. It is because we know we do not have to do the work --- the other guy is doing what we were about to do, and it will do it for us anyway.

So I do not think it makes any sense to do exponential backoff if "gc --auto" is asking "should we exit" to this logic.

An explicit "gc", on the other hand, may benefit from backoff, but then the user can choose to do so himself, and more importantly, the user can see "ah, another one is running so enough cruft will be cleaned up anyway" and choose not to run it.