Re: [PATCH v2 3/8] refs: factor update_ref steps into helpers

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 1, 2013, 06:08 UTC
Message-ID: <xmqqvc2lul9g.fsf@gitster.dls.corp.google.com>
In-Reply-To: <d327c12d90c0c2500e51fb07c06f516f94438ff0.1377885441.git.brad.king@kitware.com>

Brad King <brad.king@kitware.com> writes:

Show 26 quoted lines

> Factor the lock and write steps and error handling into helper functions
> update_ref_lock and update_ref_write to allow later use elsewhere.
> Expose lock_any_ref_for_update's type_p to update_ref_lock callers.
>
> Signed-off-by: Brad King <brad.king@kitware.com>
> ---
>  refs.c |   28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index c69fd68..2e755b4 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3170,12 +3170,13 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
>  	return retval;
>  }
>  
> -int update_ref(const char *action, const char *refname,
> -		const unsigned char *sha1, const unsigned char *oldval,
> -		int flags, enum action_on_err onerr)
> +static struct ref_lock *update_ref_lock(const char *refname,
> +					const unsigned char *oldval,
> +					int flags, int *type_p,
> +					enum action_on_err onerr)
>  {
>  	static struct ref_lock *lock;

Not the fault of this patch, as the original update_ref() had it this way, but it is not necessary to keep the value of this variable across invocations. Let's drop "static" from here, and also the corresponding variable in the new update_ref().

Will locally tweak while queuing.

Show 39 quoted lines

> -	lock = lock_any_ref_for_update(refname, oldval, flags, NULL);
> +	lock = lock_any_ref_for_update(refname, oldval, flags, type_p);
>  	if (!lock) {
>  		const char *str = "Cannot lock the ref '%s'.";
>  		switch (onerr) {
> @@ -3183,8 +3184,14 @@ int update_ref(const char *action, const char *refname,
>  		case DIE_ON_ERR: die(str, refname); break;
>  		case QUIET_ON_ERR: break;
>  		}
> -		return 1;
>  	}
> +	return lock;
> +}
> +
> +static int update_ref_write(const char *action, const char *refname,
> +			    const unsigned char *sha1, struct ref_lock *lock,
> +			    enum action_on_err onerr)
> +{
>  	if (write_ref_sha1(lock, sha1, action) < 0) {
>  		const char *str = "Cannot update the ref '%s'.";
>  		switch (onerr) {
> @@ -3197,6 +3204,17 @@ int update_ref(const char *action, const char *refname,
>  	return 0;
>  }
>  
> +int update_ref(const char *action, const char *refname,
> +	       const unsigned char *sha1, const unsigned char *oldval,
> +	       int flags, enum action_on_err onerr)
> +{
> +	static struct ref_lock *lock;
> +	lock = update_ref_lock(refname, oldval, flags, 0, onerr);
> +	if (!lock)
> +		return 1;
> +	return update_ref_write(action, refname, sha1, lock, onerr);
> +}
> +
>  struct ref *find_ref_by_name(const struct ref *list, const char *name)
>  {
>  	for ( ; list; list = list->next)