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

[Jonathan Nieder <jrnieder@gmail.com>]

Ronnie Sahlberg wrote:

> Wrap all the ref updates inside a transaction to make the update atomic.

Interesting.

[...]
> --- 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;

I think it would be cleaner for err to be local.  It isn't used for
communication between functions.

[...]
> @@ -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";

The original used rp_error to send an error message immediately via
sideband.  This drops that --- intended?

The old error string shown on the push status line was was "failed to
lock" or "failed to write" which makes it clear that the cause is
contention or database problems or filesystem problems, respectively.
After this change it would say "failed to update" which is about as
clear as "failed".

Would it be safe to send err.buf as-is over the wire, or does it
contain information or formatting that wouldn't be suitable for the
client?  (I haven't thought this through completely yet.)  Is there
some easy way to distinguish between failure to lock and failure to
write?  Or is there some one-size-fits-all error for this case?

When the transaction fails, we need to make sure that all ref updates
emit 'ng' and not 'ok' in receive-pack.c::report (see the example at
the end of Documentation/technical/pack-protocol.txt for what this
means).  What error string should they use?  Is there some way to make
it clear to the user which ref was the culprit?

What should happen when checks outside the ref transaction system
cause a ref update to fail?  I'm thinking of

 * per-ref 'update' hook (see githooks(5))
 * fast-forward check
 * ref creation/deletion checks
 * attempt to push to the currently checked out branch

I think the natural thing to do would be to put each ref update in its
own transaction to start so the semantics do not change right away.
If there are obvious answers to all these questions, then a separate
patch could combine all these into a single transaction; or if there
are no obvious answers, we could make the single-transaction-per-push
semantics optional (using a configuration variable or protocol
capability or something) to make it possible to experiment.

Hope that helps,
Jonathan