René Scharfe <l.s.r@xxxxxx> writes:
> How important is it to scan the whole file in one call? We could split
> it up like this and use a strbuf to handle host names of any length. We
> need to be permissive here to allow machines with different values for
> HOST_NAME_MAX to work with the same file on a network file system, so
> this would have to be the first patch, right?
Absolutely. FWIW, I agree with Peff that we do not need to use
fscanf here; just reading a line into strbuf and picking pieces
would be sufficient.
> NB: That && cascade has enough meat for a whole function.
True, too.
>
> René
>
> ---
> builtin/gc.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 1fca84c19d..d5e880028e 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -251,10 +251,9 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
> fd = hold_lock_file_for_update(&lock, pidfile_path,
> LOCK_DIE_ON_ERROR);
> if (!force) {
> - static char locking_host[128];
> + static struct strbuf locking_host = STRBUF_INIT;
> int should_exit;
> fp = fopen(pidfile_path, "r");
> - memset(locking_host, 0, sizeof(locking_host));
> should_exit =
> fp != NULL &&
> !fstat(fileno(fp), &st) &&
> @@ -268,9 +267,10 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
> * running.
> */
> time(NULL) - st.st_mtime <= 12 * 3600 &&
> - fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2 &&
> + fscanf(fp, "%"SCNuMAX" ", &pid) == 1 &&
> + !strbuf_getwholeline(&locking_host, fp, '\0') &&
> /* be gentle to concurrent "gc" on remote hosts */
> - (strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);
> + (strcmp(locking_host.buf, my_host) || !kill(pid, 0) || errno == EPERM);
> if (fp != NULL)
> fclose(fp);
> if (should_exit) {
> @@ -278,7 +278,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
> rollback_lock_file(&lock);
> *ret_pid = pid;
> free(pidfile_path);
> - return locking_host;
> + return locking_host.buf;
> }
> }