Re: [PATCH v2 4/8] refs: factor delete_ref loose ref step into a helper

[Michael Haggerty <mhagger@alum.mit.edu>]

On 08/30/2013 08:12 PM, Brad King wrote:
> Factor loose ref deletion into helper function delete_ref_loose to allow
> later use elsewhere.
> 
> Signed-off-by: Brad King <brad.king@kitware.com>
> ---
>  refs.c |   22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 2e755b4..5dd86ee 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2450,14 +2450,9 @@ static int repack_without_ref(const char *refname)
>  	return commit_packed_refs();
>  }
>  
> -int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
> +static int delete_ref_loose(struct ref_lock *lock, int flag)
>  {
> -	struct ref_lock *lock;
> -	int err, i = 0, ret = 0, flag = 0;
> -
> -	lock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);
> -	if (!lock)
> -		return 1;
> +	int err, i, ret = 0;
>  	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
>  		/* loose */
>  		i = strlen(lock->lk->filename) - 5; /* .lock */
> @@ -2468,6 +2463,19 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
>  
>  		lock->lk->filename[i] = '.';
>  	}
> +	return ret;
> +}
> +


At first glance it is odd that delete_ref_loose() takes a (struct
ref_lock *) argument but only actually uses lock->lk->filename.  But I
guess that the function is so specific to the contents of struct
ref_lock and indeed struct lock_file that it wouldn't make sense to pass
it only the filename attribute.  So OK.

Given that ret is only returned, you could restore the filename before
the if statement and replace the ret variable with an immediate return
statement:

static int delete_ref_loose(struct ref_lock *lock, int flag)
{
	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
		/* loose */
		int err, i = strlen(lock->lk->filename) - 5; /* .lock */

		lock->lk->filename[i] = 0;
		err = unlink_or_warn(lock->lk->filename);
		lock->lk->filename[i] = '.';
		if (err && errno != ENOENT)
			return 1;
	}
	return 0;
}

> +int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
> +{
> +	struct ref_lock *lock;
> +	int ret = 0, flag = 0;
> +
> +	lock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);
> +	if (!lock)
> +		return 1;
> +	ret |= delete_ref_loose(lock, flag);
> +
>  	/* removing the loose one could have resurrected an earlier
>  	 * packed one.  Also, if it was not loose we need to repack
>  	 * without it.
> 

Otherwise looks good.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/