undef/git-commit-vandalism
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

ref_lock: stop leaking lock_files

Browse Source 
Since the tempfile code recently relaxed the rule that
tempfile structs (and thus locks) need to hang around
forever, we no longer have to leak our lock_file structs.

In fact, we don't even need to heap-allocate them anymore,
since their lifetime can just match that of the surrounding
ref_lock (and if we forget to delete a lock, the effect is
the same as before: it will eventually go away at program
exit).

Note that there is a check in unlock_ref() to only rollback
a lock file if it has been allocated. We don't need that
check anymore; we zero the ref_lock (and thus the
lock_file), so at worst we pass a NULL pointer to
delete_tempfile(), which considers that a noop.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This commit is contained in:
Jeff King 2017-09-05 08:15:15 -04:00 committed by Junio C Hamano
parent 5e7f01c93e
commit ee4d8e455c
1 changed files with 16 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

39
refs/files-backend.c
View File

@ -12,7 +12,7 @@
struct ref_lock { struct ref_lock {
char *ref_name; char *ref_name;
struct lock_file *lk; struct lock_file lk;
struct object_id old_oid; struct object_id old_oid;
}; };
@ -418,9 +418,7 @@ out:
static void unlock_ref(struct ref_lock *lock) static void unlock_ref(struct ref_lock *lock)
{ {
/* Do not free lock->lk -- atexit() still looks at them */ rollback_lock_file(&lock->lk);
if (lock->lk)
rollback_lock_file(lock->lk);
free(lock->ref_name); free(lock->ref_name);
free(lock); free(lock);
} }
@ -534,11 +532,8 @@ retry:
goto error_return; goto error_return;
} }
if (!lock->lk)
lock->lk = xcalloc(1, sizeof(struct lock_file));
if (hold_lock_file_for_update_timeout( if (hold_lock_file_for_update_timeout(
lock->lk, ref_file.buf, LOCK_NO_DEREF, &lock->lk, ref_file.buf, LOCK_NO_DEREF,
get_files_ref_lock_timeout_ms()) < 0) { get_files_ref_lock_timeout_ms()) < 0) {
if (errno == ENOENT && --attempts_remaining > 0) { if (errno == ENOENT && --attempts_remaining > 0) {
/* /*
@ -949,11 +944,9 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,
goto error_return; goto error_return;
} }
lock->lk = xcalloc(1, sizeof(struct lock_file));
lock->ref_name = xstrdup(refname); lock->ref_name = xstrdup(refname);
if (raceproof_create_file(ref_file.buf, create_reflock, lock->lk)) { if (raceproof_create_file(ref_file.buf, create_reflock, &lock->lk)) {
last_errno = errno; last_errno = errno;
unable_to_lock_message(ref_file.buf, errno, err); unable_to_lock_message(ref_file.buf, errno, err);
goto error_return; goto error_return;
@ -1404,14 +1397,14 @@ static int files_rename_ref(struct ref_store *ref_store,
static int close_ref_gently(struct ref_lock *lock) static int close_ref_gently(struct ref_lock *lock)
{ {
if (close_lock_file_gently(lock->lk)) if (close_lock_file_gently(&lock->lk))
return -1; return -1;
return 0; return 0;
} }
static int commit_ref(struct ref_lock *lock) static int commit_ref(struct ref_lock *lock)
{ {
char *path = get_locked_file_path(lock->lk); char *path = get_locked_file_path(&lock->lk);
struct stat st; struct stat st;
if (!lstat(path, &st) && S_ISDIR(st.st_mode)) { if (!lstat(path, &st) && S_ISDIR(st.st_mode)) {
@ -1435,7 +1428,7 @@ static int commit_ref(struct ref_lock *lock)
free(path); free(path);
} }
if (commit_lock_file(lock->lk)) if (commit_lock_file(&lock->lk))
return -1; return -1;
return 0; return 0;
} }
@ -1627,12 +1620,12 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
unlock_ref(lock); unlock_ref(lock);
return -1; return -1;
} }
fd = get_lock_file_fd(lock->lk); fd = get_lock_file_fd(&lock->lk);
if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ || if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||
write_in_full(fd, &term, 1) != 1 || write_in_full(fd, &term, 1) != 1 ||
close_ref_gently(lock) < 0) { close_ref_gently(lock) < 0) {
strbuf_addf(err, strbuf_addf(err,
"couldn't write '%s'", get_lock_file_path(lock->lk)); "couldn't write '%s'", get_lock_file_path(&lock->lk));
unlock_ref(lock); unlock_ref(lock);
return -1; return -1;
} }
@ -1709,7 +1702,7 @@ static int create_ref_symlink(struct ref_lock *lock, const char *target)
{ {
int ret = -1; int ret = -1;
#ifndef NO_SYMLINK_HEAD #ifndef NO_SYMLINK_HEAD
char *ref_path = get_locked_file_path(lock->lk); char *ref_path = get_locked_file_path(&lock->lk);
unlink(ref_path); unlink(ref_path);
ret = symlink(target, ref_path); ret = symlink(target, ref_path);
free(ref_path); free(ref_path);
@ -1745,14 +1738,14 @@ static int create_symref_locked(struct files_ref_store *refs,
return 0; return 0;
} }
if (!fdopen_lock_file(lock->lk, "w")) if (!fdopen_lock_file(&lock->lk, "w"))
return error("unable to fdopen %s: %s", return error("unable to fdopen %s: %s",
lock->lk->tempfile->filename.buf, strerror(errno)); lock->lk.tempfile->filename.buf, strerror(errno));
update_symref_reflog(refs, lock, refname, target, logmsg); update_symref_reflog(refs, lock, refname, target, logmsg);
/* no error check; commit_ref will check ferror */ /* no error check; commit_ref will check ferror */
fprintf(lock->lk->tempfile->fp, "ref: %s\n", target); fprintf(lock->lk.tempfile->fp, "ref: %s\n", target);
if (commit_ref(lock) < 0) if (commit_ref(lock) < 0)
return error("unable to write symref for %s: %s", refname, return error("unable to write symref for %s: %s", refname,
strerror(errno)); strerror(errno));
@ -2853,12 +2846,12 @@ static int files_reflog_expire(struct ref_store *ref_store,
strerror(errno)); strerror(errno));
rollback_lock_file(&reflog_lock); rollback_lock_file(&reflog_lock);
} else if (update && } else if (update &&
(write_in_full(get_lock_file_fd(lock->lk), (write_in_full(get_lock_file_fd(&lock->lk),
oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ || oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||
write_str_in_full(get_lock_file_fd(lock->lk), "\n") != 1 || write_str_in_full(get_lock_file_fd(&lock->lk), "\n") != 1 ||
close_ref_gently(lock) < 0)) { close_ref_gently(lock) < 0)) {
status |= error("couldn't write %s", status |= error("couldn't write %s",
get_lock_file_path(lock->lk)); get_lock_file_path(&lock->lk));
rollback_lock_file(&reflog_lock); rollback_lock_file(&reflog_lock);
} else if (commit_lock_file(&reflog_lock)) { } else if (commit_lock_file(&reflog_lock)) {
status |= error("unable to write reflog '%s' (%s)", status |= error("unable to write reflog '%s' (%s)",