On Tue, May 20, 2014 at 1:37 PM, Ronnie Sahlberg <sahlberg@xxxxxxxxxx> wrote:
> 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.
I have added support for atomic pushes towards the end of the -next series.
https://github.com/rsahlberg/git/tree/ref-transactions-next
It is activated by a new --atomic-push argument to send-pack and is
then negotiated by a new option in the wire protocol.
>
> 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