Subject: Re: [PATCH 3/3] refs.c: remove lock_fd from struct ref_lock

From: Junio C Hamano <[email-masked]>
Date: Tue, 14 Apr 2015 16:12:03 -0700

Stefan Beller <[email-masked]> writes:

> The 'lock_fd' is the same as 'lk->fd'. No need to store it twice so remove
> it. You may argue this introduces more coupling as we need to know more
> about the internals of the lock file mechanism, but this will be solved in
> a later patch.
>
> No functional changes intended.

It is somewhat strange to hear "in a later patch" in [PATCH 3/3] of
a 3-patch series ;-), but I think this makes sense.  Whenever
we take a ref-lock, and we are going to actually write something
into the filesystem, we would go thru the lock_file API, so we can
depend on lk to have its own file descriptor field.

>
> Signed-off-by: Stefan Beller <[email-masked]>
> Signed-off-by: Junio C Hamano <[email-masked]>
> ---
>  refs.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 14e52ca..4066752 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -11,7 +11,6 @@ struct ref_lock {
>  	char *orig_ref_name;
>  	struct lock_file *lk;
>  	unsigned char old_sha1[20];
> -	int lock_fd;
>  	int force_write;
>  };
>  
> @@ -2259,7 +2258,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>  	int attempts_remaining = 3;
>  
>  	lock = xcalloc(1, sizeof(struct ref_lock));
> -	lock->lock_fd = -1;
>  
>  	if (mustexist)
>  		resolve_flags |= RESOLVE_REF_READING;
> @@ -2335,8 +2333,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>  		goto error_return;
>  	}
>  
> -	lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);
> -	if (lock->lock_fd < 0) {
> +	if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {
> +		last_errno = errno;
>  		if (errno == ENOENT && --attempts_remaining > 0)
>  			/*
>  			 * Maybe somebody just deleted one of the
> @@ -2904,7 +2902,6 @@ static int close_ref(struct ref_lock *lock)
>  {
>  	if (close_lock_file(lock->lk))
>  		return -1;
> -	lock->lock_fd = -1;
>  	return 0;
>  }
>  
> @@ -2912,7 +2909,6 @@ static int commit_ref(struct ref_lock *lock)
>  {
>  	if (commit_lock_file(lock->lk))
>  		return -1;
> -	lock->lock_fd = -1;
>  	return 0;
>  }
>  
> @@ -3090,8 +3086,8 @@ static int write_ref_sha1(struct ref_lock *lock,
>  		errno = EINVAL;
>  		return -1;
>  	}
> -	if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||
> -	    write_in_full(lock->lock_fd, &term, 1) != 1 ||
> +	if (write_in_full(lock->lk->fd, sha1_to_hex(sha1), 40) != 40 ||
> +	    write_in_full(lock->lk->fd, &term, 1) != 1 ||
>  	    close_ref(lock) < 0) {
>  		int save_errno = errno;
>  		error("Couldn't write %s", lock->lk->filename.buf);
> @@ -4047,9 +4043,9 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
>  			status |= error("couldn't write %s: %s", log_file,
>  					strerror(errno));
>  		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
> -			(write_in_full(lock->lock_fd,
> +			(write_in_full(lock->lk->fd,
>  				sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
> -			 write_str_in_full(lock->lock_fd, "\n") != 1 ||
> +			 write_str_in_full(lock->lk->fd, "\n") != 1 ||
>  			 close_ref(lock) < 0)) {
>  			status |= error("couldn't write %s",
>  					lock->lk->filename.buf);