Re: [PATCH v10 25/44] receive-pack.c: use a reference transaction for updating the refs

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

On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
> Wrap all the ref updates inside a transaction to make the update atomic.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  builtin/receive-pack.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index c323081..5534138 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -46,6 +46,8 @@ static void *head_name_to_free;
>  static int sent_capabilities;
>  static int shallow_update;
>  static const char *alt_shallow_file;
> +static struct strbuf err = STRBUF_INIT;
> +static struct ref_transaction *transaction;
>  
>  static enum deny_action parse_deny_action(const char *var, const char *value)
>  {
> @@ -475,7 +477,6 @@ static const char *update(struct command *cmd, struct shallow_info *si)
>  	const char *namespaced_name;
>  	unsigned char *old_sha1 = cmd->old_sha1;
>  	unsigned char *new_sha1 = cmd->new_sha1;
> -	struct ref_lock *lock;
>  
>  	/* only refs/... are allowed */
>  	if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
> @@ -580,15 +581,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)
>  		    update_shallow_ref(cmd, si))
>  			return "shallow error";
>  
> -		lock = lock_any_ref_for_update(namespaced_name, old_sha1,
> -					       0, NULL);
> -		if (!lock) {
> -			rp_error("failed to lock %s", name);
> -			return "failed to lock";
> -		}
> -		if (write_ref_sha1(lock, new_sha1, "push")) {
> -			return "failed to write"; /* error() already called */
> -		}
> +		if (ref_transaction_update(transaction, namespaced_name,
> +					   new_sha1, old_sha1, 0, 1, &err))
> +			return "failed to update";
>  		return NULL; /* good */
>  	}
>  }
> @@ -812,6 +807,7 @@ static void execute_commands(struct command *commands,
>  	head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
>  
>  	checked_connectivity = 1;
> +	transaction = ref_transaction_begin();
>  	for (cmd = commands; cmd; cmd = cmd->next) {
>  		if (cmd->error_string)
>  			continue;
> @@ -827,6 +823,10 @@ static void execute_commands(struct command *commands,
>  			checked_connectivity = 0;
>  		}
>  	}
> +	if (ref_transaction_commit(transaction, "push", &err))
> +		error("%s", err.buf);
> +	ref_transaction_free(transaction);
> +	strbuf_release(&err);
>  
>  	if (shallow_update && !checked_connectivity)
>  		error("BUG: run 'git fsck' for safety.\n"
> 

This patch is strange, because even if one ref_transaction_update() call
fails, subsequent updates are nevertheless also attempted, and the
ref_transaction_commit() is also attempted.  Is this an officially
sanctioned use of the ref_transactions API?  Should it be?  It might be
a way to give feedback to the user on multiple attempted reference
updates at once (i.e., address my comment about the last patch).

If this is sanctioned, then it might be appropriate for the transaction
to keep track of the fact that one or more reference updates failed, and
when *_commit() is called to fail the whole transaction.

In any case, I think it is important to document, as part of the API
docs, whether this is sanctioned or not, and if so, what exactly are its
semantics.

I've run out of time for today so I'm going to have to stop here.  FWIW
patches 01-23 looked OK aside from the comments that I have made.

Michael

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