# [PATCH] Remove the close_ref function. 5 messages from 2014-04-08 to 2014-04-08. Participants: Ronnie Sahlberg, Junio C Hamano. Thread: https://gitlist.dev/t/36377 ## Ronnie Sahlberg, 2014-04-08 21:17 Subject: [PATCH] Remove redundant close_ref function Message-ID: <1396991830-20938-1-git-send-email-sahlberg@google.com> URL: https://gitlist.dev/e/1396991830-20938-1-git-send-email-sahlberg%40google.com ``` List, This is a trivial patch that removes the function close_ref() from refs.c. This function was only called from two codepaths and can be removed since both codepaths shortly afterwards both call unlock_ref() which implicitely closes the file anyway. By removing this function we simplify the api to refs slightly. This also means that the lifetime of the filedescriptor becomes the same as the lifetime for the 'struct ref_lock' object. The filedescriptor is opened at the same time ref_lock is allocated and the descriptor is closed when ref_lock is released. regards ronnie sahlberg ``` ## Ronnie Sahlberg, 2014-04-08 21:17 Subject: [PATCH] Remove the close_ref function. Message-ID: <1396991830-20938-2-git-send-email-sahlberg@google.com> URL: https://gitlist.dev/e/1396991830-20938-2-git-send-email-sahlberg%40google.com In-Reply-To: <1396991830-20938-1-git-send-email-sahlberg@google.com> ``` close_ref() is only called from two codepaths semi-immediately before unlock_ref() is called. Since unlock_ref() will also close the file if it is still open, we can delete close_ref() and let the file become closed during unlock_ref(). This simplifies the refs.c api by removing a redundant function. Signed-off-by: Ronnie Sahlberg --- builtin/reflog.c | 3 +-- refs.c | 11 +---------- refs.h | 3 --- 3 files changed, 2 insertions(+), 15 deletions(-) diff --git a/builtin/reflog.c b/builtin/reflog.c index c12a9784..b67fbe6 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c @@ -428,8 +428,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused, } else if (cmd->updateref && (write_in_full(lock->lock_fd, sha1_to_hex(cb.last_kept_sha1), 40) != 40 || - write_str_in_full(lock->lock_fd, "\n") != 1 || - close_ref(lock) < 0)) { + write_str_in_full(lock->lock_fd, "\n") != 1)) { status |= error("Couldn't write %s", lock->lk->filename); unlink(newlog_path); diff --git a/refs.c b/refs.c index 28d5eca..094b047 100644 --- a/refs.c +++ b/refs.c @@ -2665,14 +2665,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms return 1; } -int close_ref(struct ref_lock *lock) -{ - if (close_lock_file(lock->lk)) - return -1; - lock->lock_fd = -1; - return 0; -} - int commit_ref(struct ref_lock *lock) { if (commit_lock_file(lock->lk)) @@ -2824,8 +2816,7 @@ int write_ref_sha1(struct ref_lock *lock, return -1; } if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 || - write_in_full(lock->lock_fd, &term, 1) != 1 - || close_ref(lock) < 0) { + write_in_full(lock->lock_fd, &term, 1) != 1) { error("Couldn't write %s", lock->lk->filename); unlock_ref(lock); return -1; diff --git a/refs.h b/refs.h index 87a1a79..d8732a5 100644 --- a/refs.h +++ b/refs.h @@ -153,9 +153,6 @@ extern struct ref_lock *lock_any_ref_for_update(const char *refname, const unsigned char *old_sha1, int flags, int *type_p); -/** Close the file descriptor owned by a lock and return the status */ -extern int close_ref(struct ref_lock *lock); - /** Close and commit the ref locked by the lock */ extern int commit_ref(struct ref_lock *lock); -- 1.9.1.423.g4596e3a ``` ## Junio C Hamano, 2014-04-08 21:32 Subject: Re: [PATCH] Remove redundant close_ref function Message-ID: URL: https://gitlist.dev/e/xmqqioqj33tu.fsf%40gitster.dls.corp.google.com In-Reply-To: <1396991830-20938-1-git-send-email-sahlberg@google.com> ``` Ronnie Sahlberg writes: > List, > > This is a trivial patch that removes the function close_ref() from refs.c. > This function was only called from two codepaths and can be removed since both codepaths shortly afterwards > both call unlock_ref() which implicitely closes the file anyway. > > By removing this function we simplify the api to refs slightly. > This also means that the lifetime of the filedescriptor becomes the same as the lifetime for the 'struct ref_lock' object. > The filedescriptor is opened at the same time ref_lock is allocated and the descriptor is closed when ref_lock is released. > > > regards > ronnie sahlberg Thanks. A few tips: - wrap your lines at around 72 columns. - "git format-patch --cover-letter" will give you a skeletal message with "Subject: [PATCH 0/n]" with list of individual patches and diffstat to show the overall damage, with two placeholders "*** SUBJECT HERE ***" and "*** BLURB HERE ***" for you to fill in the remainder. - For a short/single patch, you do not have to add a cover letter (it is not a crime to add one, though). ``` ## Junio C Hamano, 2014-04-08 21:50 Subject: Re: [PATCH] Remove the close_ref function. Message-ID: URL: https://gitlist.dev/e/xmqqeh173301.fsf%40gitster.dls.corp.google.com In-Reply-To: <1396991830-20938-2-git-send-email-sahlberg@google.com> ``` Ronnie Sahlberg writes: > @@ -2824,8 +2816,7 @@ int write_ref_sha1(struct ref_lock *lock, > return -1; > } > if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 || > - write_in_full(lock->lock_fd, &term, 1) != 1 > - || close_ref(lock) < 0) { > + write_in_full(lock->lock_fd, &term, 1) != 1) { In the original code, we try to write the new object name and the line terminator, and also try to make sure that the file descriptor is successfully closed here. Only when all of that is done successfully we go update the reflog and then after that, we commit the lockfile by renaming. With the updated code, these two write(2)s may succeed, but we would not know if the close(2) would succeed, until commit_lock_file() is called much later in this codepath. We would end up updating the reflog even when the close(2) of the ref fails. To be really paranoid, we should probably be doing an fsync(2) to make sure that the bytes written hit the disk platter, not just close(2), and such a change may be a good step in the direction to make the code more robust; in that light, the patch goes in the opposite direction "what it does is not robust enough anyway, so let's loosen it further". Hmm... ``` ## Ronnie Sahlberg, 2014-04-08 22:23 Subject: Re: [PATCH] Remove the close_ref function. Message-ID: URL: https://gitlist.dev/e/CAL%3DYDWk3CkLG5P18GFhUKKQ9bEqOeJayNxKCiTMRtg-d4K_smw%40mail.gmail.com In-Reply-To: ``` On Tue, Apr 8, 2014 at 2:50 PM, Junio C Hamano wrote: > Ronnie Sahlberg writes: > >> @@ -2824,8 +2816,7 @@ int write_ref_sha1(struct ref_lock *lock, >> return -1; >> } >> if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 || >> - write_in_full(lock->lock_fd, &term, 1) != 1 >> - || close_ref(lock) < 0) { >> + write_in_full(lock->lock_fd, &term, 1) != 1) { > > In the original code, we try to write the new object name and the > line terminator, and also try to make sure that the file descriptor > is successfully closed here. Only when all of that is done > successfully we go update the reflog and then after that, we commit > the lockfile by renaming. > > With the updated code, these two write(2)s may succeed, but we would > not know if the close(2) would succeed, until commit_lock_file() is > called much later in this codepath. > > We would end up updating the reflog even when the close(2) of the > ref fails. > > To be really paranoid, we should probably be doing an fsync(2) > to make sure that the bytes written hit the disk platter, not just > close(2), and such a change may be a good step in the direction to > make the code more robust; in that light, the patch goes in the > opposite direction "what it does is not robust enough anyway, so > let's loosen it further". > > Hmm... > Good point. Thanks. Please consider this patch abandoned. ```