refs.c: int close_ref(struct ref_lock *lock) { if (close_lock_file(lock->lk)) return -1; lock->lock_fd = -1; return 0; } When the close() fails, fd is still >= 0, even if the file is closed. Could it be written like this ? int close_ref(struct ref_lock *lock) { return close_lock_file(lock->lk); } ===================== lockfile.c, line 49 * - filename holds the filename of the lockfile I think we have a strbuf here? (which is a good thing), should we write: * - strbuf filename holds the filename of the lockfile ---------- (and at some places filename[0] is mentioned, should that be filename.buf[0] ?) ========================= int commit_lock_file(struct lock_file *lk) { static struct strbuf result_file = STRBUF_INIT; int err; if (lk->fd >= 0 && close_lock_file(lk)) return -1; ##What happens if close() fails and close_lock_file() returns != 0? ##Is the lk now in a good shaped state? I think the file which had been open by lk->fd is in an unkown state, and should not be used for anything. When I remember it right, an error on close() can mean "the file could not be written and closed as expected, it can be truncated or corrupted. This can happen on a network file system like NFS, or probably even other FS. For me the failing of close() means I smell a need for a rollback. if (!lk->active) die("BUG: attempt to commit unlocked object"); /* remove ".lock": */ strbuf_add(&result_file, lk->filename.buf, lk->filename.len - LOCK_SUFFIX_LEN); err = rename(lk->filename.buf, result_file.buf); strbuf_reset(&result_file); if (err) return -1; lk->active = 0; strbuf_reset(&lk->filename); return 0; } ================ Please treat my comments more than questions rather than answers, thanks for an interesting reading -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html