Re: [PATCH 1/8] files-backend: extract out `create_symref_lock`

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3625 bytes --]

On Sat, Mar 30, 2024 at 11:46:16PM +0100, Karthik Nayak wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
> 
> The function `create_symref_locked` creates a symref by creating a
> '<symref>.lock' file and then committing the symref lock, which creates
> the final symref.
> 
> Split this into two individual functions `create_and_commit_symref` and
> `create_symref_locked`. This way we can create the symref lock and
> commit it at different times. This will be used to provide symref
> support in `git-update-ref(1)`.
> 
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs/files-backend.c | 40 +++++++++++++++++++++++++++-------------
>  1 file changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index a098d14ea0..3f0f9521cb 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1920,26 +1920,39 @@ static void update_symref_reflog(struct files_ref_store *refs,
>  	}
>  }
>  
> -static int create_symref_locked(struct files_ref_store *refs,
> -				struct ref_lock *lock, const char *refname,
> -				const char *target, const char *logmsg)
> +static int create_symref_lock(struct files_ref_store *refs,
> +			      struct ref_lock *lock, const char *refname,
> +			      const char *target)
>  {
> +	if (!fdopen_lock_file(&lock->lk, "w"))
> +		return error("unable to fdopen %s: %s",
> +			     get_lock_file_path(&lock->lk), strerror(errno));
> +
> +	/* no error check; commit_ref will check ferror */
> +	fprintf(get_lock_file_fp(&lock->lk), "ref: %s\n", target);
> +	return 0;
> +}
> +
> +static int create_and_commit_symref(struct files_ref_store *refs,
> +				    struct ref_lock *lock, const char *refname,
> +				    const char *target, const char *logmsg)
> +{
> +	int ret;
> +
>  	if (prefer_symlink_refs && !create_ref_symlink(lock, target)) {
>  		update_symref_reflog(refs, lock, refname, target, logmsg);
>  		return 0;
>  	}
>  
> -	if (!fdopen_lock_file(&lock->lk, "w"))
> -		return error("unable to fdopen %s: %s",
> -			     get_lock_file_path(&lock->lk), strerror(errno));
> +	ret = create_symref_lock(refs, lock, refname, target);
> +	if (!ret) {
> +		update_symref_reflog(refs, lock, refname, target, logmsg);

I feel like the resulting code here is a bit hard to read because the
successful path is now nested into the condition. This does not really
conform to our typical coding style. Exiting early in case the function
returns an error would be easier to read.

> -	update_symref_reflog(refs, lock, refname, target, logmsg);
> +		if (commit_ref(lock) < 0)
> +			return error("unable to write symref for %s: %s", refname,
> +				     strerror(errno));
> +	}
>  
> -	/* no error check; commit_ref will check ferror */
> -	fprintf(get_lock_file_fp(&lock->lk), "ref: %s\n", target);
> -	if (commit_ref(lock) < 0)
> -		return error("unable to write symref for %s: %s", refname,
> -			     strerror(errno));
>  	return 0;

Also, is it correct to `return 0` here in case `create_symref_lock()`
returns an error? If so it certainly requires an in-code comment to
explain what is going on. If this is a bug I feel like we have
identified a test gap that we might want to plug.

Patrick

>  }
>  
> @@ -1960,7 +1973,8 @@ static int files_create_symref(struct ref_store *ref_store,
>  		return -1;
>  	}
>  
> -	ret = create_symref_locked(refs, lock, refname, target, logmsg);
> +	ret = create_and_commit_symref(refs, lock, refname, target, logmsg);
> +
>  	unlock_ref(lock);
>  	return ret;
>  }
> -- 
> 2.43.GIT
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]