On Tue, May 20, 2014 at 12:42 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> 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.
Done.
>
> [...]
>> @@ -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?
Oops. I misread it as a normal error()
>
> 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".
I changed it to return xstrdup(err.buf) which should be as detailed as
we can get.
>
> 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?
I think err.buf is what we want.
>
> 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.
I changed it to use one transaction per ref for now.
Please see the update ref-transactions branch.
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html