Re: [PATCH v8 25/44] receive-pack.c: use a reference transaction for updating the refs
On Tue, May 20, 2014 at 1:37 PM, Ronnie Sahlberg wrote:
> On Tue, May 20, 2014 at 12:42 PM, Jonathan Nieder 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 25/44] receive-pack.c: use a reference transaction for updating the refs
On Tue, May 20, 2014 at 12:42 PM, Jonathan Nieder 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 25/44] receive-pack.c: use a reference transaction for updating the refs
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v8 25/44] receive-pack.c: use a reference transaction for updating the refs
Wrap all the ref updates inside a transaction to make the update atomic.
Signed-off-by: Ronnie Sahlberg
---
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..d580176 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;
+struct strbuf err = STRBUF_INIT;
+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))
+ 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"
--
2.0.0.rc3.477.g0f8edf7
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
