threads / patch / 36377

patchRemove the close_ref function.

Subject: [PATCH] Remove the close_ref function.

## tl;dr

5 messages between Apr 8, 2014 and Apr 8, 2014. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Ronnie Sahlberg· Apr 8, 2014, 21:17 UTC · lore

[PATCH] Remove redundant close_ref function

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· Apr 8, 2014, 21:17 UTC · re: Ronnie Sahlberg · lore

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(-)
Show changes to 3 files +2 −15

builtin/reflog.c, refs.c, refs.h

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· Apr 8, 2014, 21:50 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH] Remove the close_ref function.

Ronnie Sahlberg <sahlberg@google.com> writes:
Show 7 quoted lines
> @@ -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· Apr 8, 2014, 22:23 UTC · re: Junio C Hamano · lore

Re: [PATCH] Remove the close_ref function.

On Tue, Apr 8, 2014 at 2:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 32 quoted lines
> 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.

Junio C Hamano· Apr 8, 2014, 21:32 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH] Remove redundant close_ref function

Ronnie Sahlberg <sahlberg@google.com> writes:
Show 13 quoted lines
> 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).

← back to recent threads