Re: [PATCH v20 33/48] walker.c: use ref transaction for ref updates

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

On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> Switch to using ref transactions in walker_fetch(). As part of the refactoring
> to use ref transactions we also fix a potential memory leak where in the
> original code if write_ref_sha1() would fail we would end up returning from
> the function without free()ing the msg string.
> 
> Note that this function is only called when fetching from a remote HTTP
> repository onto the local (most of the time single-user) repository which
> likely means that the type of collissions that the previous locking would

s/collissions/collisions/

> protect against and cause the fetch to fail for to be even more rare.

Grammatico: s/to be/are/ ?

> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  walker.c | 59 +++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 35 insertions(+), 24 deletions(-)
> 
> diff --git a/walker.c b/walker.c
> index 1dd86b8..60d9f9e 100644
> --- a/walker.c
> +++ b/walker.c
> @@ -251,39 +251,36 @@ void walker_targets_free(int targets, char **target, const char **write_ref)
>  int walker_fetch(struct walker *walker, int targets, char **target,
>  		 const char **write_ref, const char *write_ref_log_details)
>  {
> -	struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));
> +	struct strbuf ref_name = STRBUF_INIT;
> +	struct strbuf err = STRBUF_INIT;
> +	struct ref_transaction *transaction = NULL;
>  	unsigned char *sha1 = xmalloc(targets * 20);
> -	char *msg;
> -	int ret;
> +	char *msg = NULL;
>  	int i;
>  
>  	save_commit_buffer = 0;
>  
> -	for (i = 0; i < targets; i++) {
> -		if (!write_ref || !write_ref[i])
> -			continue;
> -
> -		lock[i] = lock_ref_sha1(write_ref[i], NULL);
> -		if (!lock[i]) {
> -			error("Can't lock ref %s", write_ref[i]);
> -			goto unlock_and_fail;
> +	if (write_ref) {
> +		transaction = ref_transaction_begin(&err);
> +		if (!transaction) {
> +			error("%s", err.buf);
> +			goto rollback_and_fail;
>  		}
>  	}
> -

Is there some reason why the transaction cannot be built up during a
single iteration over targets, thereby also avoiding the need for the
sha1[20*i] stuff?  This seems like exactly the kind of situation where
transactions should *save* code.  But perhaps I've overlooked a
dependency between the two loops.

>  	if (!walker->get_recover)
>  		for_each_ref(mark_complete, NULL);
>  
>  	for (i = 0; i < targets; i++) {
>  		if (interpret_target(walker, target[i], &sha1[20 * i])) {
>  			error("Could not interpret response from server '%s' as something to pull", target[i]);
> -			goto unlock_and_fail;
> +			goto rollback_and_fail;
>  		}
>  		if (process(walker, lookup_unknown_object(&sha1[20 * i])))
> -			goto unlock_and_fail;
> +			goto rollback_and_fail;
>  	}
>  
>  	if (loop(walker))
> -		goto unlock_and_fail;
> +		goto rollback_and_fail;
>  
>  	if (write_ref_log_details) {
>  		msg = xmalloc(strlen(write_ref_log_details) + 12);
> @@ -294,19 +291,33 @@ int walker_fetch(struct walker *walker, int targets, char **target,
>  	for (i = 0; i < targets; i++) {
>  		if (!write_ref || !write_ref[i])
>  			continue;
> -		ret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : "fetch (unknown)");
> -		lock[i] = NULL;
> -		if (ret)
> -			goto unlock_and_fail;
> +		strbuf_reset(&ref_name);
> +		strbuf_addf(&ref_name, "refs/%s", write_ref[i]);
> +		if (ref_transaction_update(transaction, ref_name.buf,
> +					   &sha1[20 * i], NULL, 0, 0,
> +					   &err)) {
> +			error("%s", err.buf);
> +			goto rollback_and_fail;
> +		}
> +	}
> +	if (write_ref) {
> +		if (ref_transaction_commit(transaction,
> +					   msg ? msg : "fetch (unknown)",
> +					   &err)) {
> +			error("%s", err.buf);
> +			goto rollback_and_fail;
> +		}
> +		ref_transaction_free(transaction);
>  	}
> -	free(msg);
>  
> +	free(msg);
>  	return 0;
>  
> -unlock_and_fail:
> -	for (i = 0; i < targets; i++)
> -		if (lock[i])
> -			unlock_ref(lock[i]);
> +rollback_and_fail:
> +	ref_transaction_free(transaction);
> +	free(msg);
> +	strbuf_release(&err);
> +	strbuf_release(&ref_name);
>  
>  	return -1;
>  }
> 

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu