[PATCH] Remove redundant close_ref function

[PATCH] Remove redundant close_ref function

[Ronnie Sahlberg]

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

[PATCH] Remove the close_ref function.

[Ronnie Sahlberg]

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 <sahlberg@google.com>
---
 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

Re: [PATCH] Remove redundant close_ref function

[Junio C Hamano]

Ronnie Sahlberg <sahlberg@google.com> 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).

Re: [PATCH] Remove the close_ref function.

[Junio C Hamano]

Ronnie Sahlberg <sahlberg@google.com> 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...

Re: [PATCH] Remove the close_ref function.

[Ronnie Sahlberg]

On Tue, Apr 8, 2014 at 2:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ronnie Sahlberg <sahlberg@google.com> 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.