{"thread":{"id":"36474","subject":"[PATCH 0/3] Use ref transactions for fetch","startedAt":"2014-04-22T18:45:24Z","lastAt":"2014-05-06T20:53:08Z","messageCount":14,"participants":["Ronnie Sahlberg","Eric Sunshine","Michael Haggerty","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"239360","messageId":"1398192327-21302-1-git-send-email-sahlberg@google.com","threadId":"36474","inReplyTo":null,"subject":"[PATCH 0/3] Use ref transactions for fetch","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-22T18:45:24Z","receivedAt":"2014-04-22T18:45:24Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"This change is based on the previous ref transaction patches.\nThis is sent as a separate patch series since it implements a lot more\nnon-trivial changes to the behaviour than the previous patches and thus can\nuse more detailed review.\n\nUpdate fetch.c to use ref transactions when performing updates. Use a single\nref transaction for all updates and only commit the transaction if all other\nchecks and oeprations have been successful. This makes the ref updates during\na fetch (mostly) atomic.\n\nRonnie Sahlberg (3):\n fetch.c: clear errno before calling functions that might set it\n fetch.c: change s_update_ref to use a ref transaction\n fetch.c: use a single ref transaction for all ref updates\n\n builtin/fetch.c | 39 ++++++++++++++++++++++-----------------\n 1 file changed, 22 insertions(+), 17 deletions(-)\n\n-- \n1.9.1.518.g16976cb.dirty\n"},{"id":"239361","messageId":"1398192327-21302-2-git-send-email-sahlberg@google.com","threadId":"36474","inReplyTo":"1398192327-21302-1-git-send-email-sahlberg@google.com","subject":"[PATCH 1/3] fetch.c: clear errno before calling functions that might set it","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-22T18:45:25Z","receivedAt":"2014-04-22T18:45:25Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"In s_update_ref there are two calls that when they fail we return an error\nbased on the errno value. In particular we want to return a specific error\nif ENOTDIR happened. Both these functions do have failure modes where they\nmay return an error without updating errno, in which case a previous and\nunrelated ENOTDIT may cause us to return the wrong error. Clear errno before\ncalling any functions if we check errno afterwards.\n\nAlso skip initializing a static variable to 0. Sstatics live in .bss and\nare all automatically initialized to 0.\n\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/fetch.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 55f457c..a93c893 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -44,7 +44,7 @@ static struct transport *gtransport;\n static struct transport *gsecondary;\n static const char *submodule_prefix = \"\";\n static const char *recurse_submodules_default;\n-static int shown_url = 0;\n+static int shown_url;\n \n static int option_parse_recurse_submodules(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n@@ -382,6 +382,8 @@ static int s_update_ref(const char *action,\n \tif (!rla)\n \t\trla = default_rla.buf;\n \tsnprintf(msg, sizeof(msg), \"%s: %s\", rla, action);\n+\n+\terrno = 0;\n \tlock = lock_any_ref_for_update(ref->name,\n \t\t\t\t check_old ? ref->old_sha1 : NULL,\n \t\t\t\t 0, NULL);\n-- \n1.9.1.518.g16976cb.dirty\n"},{"id":"239362","messageId":"1398192327-21302-3-git-send-email-sahlberg@google.com","threadId":"36474","inReplyTo":"1398192327-21302-1-git-send-email-sahlberg@google.com","subject":"[PATCH 2/3] fetch.c: change s_update_ref to use a ref transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-22T18:45:26Z","receivedAt":"2014-04-22T18:45:26Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change s_update_ref to use a ref transaction for the ref update.\nSigned-off-by: Ronnie Sahlberg \n\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/fetch.c | 15 +++++++--------\n 1 file changed, 7 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a93c893..5c15584 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -375,7 +375,7 @@ static int s_update_ref(const char *action,\n {\n \tchar msg[1024];\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n-\tstatic struct ref_lock *lock;\n+\tstruct ref_transaction *transaction;\n \n \tif (dry_run)\n \t\treturn 0;\n@@ -384,15 +384,14 @@ static int s_update_ref(const char *action,\n \tsnprintf(msg, sizeof(msg), \"%s: %s\", rla, action);\n \n \terrno = 0;\n-\tlock = lock_any_ref_for_update(ref->name,\n-\t\t\t\t check_old ? ref->old_sha1 : NULL,\n-\t\t\t\t 0, NULL);\n-\tif (!lock)\n-\t\treturn errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n-\t\t\t\t\t STORE_REF_ERROR_OTHER;\n-\tif (write_ref_sha1(lock, ref->new_sha1, msg) < 0)\n+\ttransaction = ref_transaction_begin();\n+\tif (!transaction ||\n+\t ref_transaction_update(transaction, ref->name, ref->new_sha1,\n+\t\t\t\t ref->old_sha1, 0, check_old) ||\n+\t ref_transaction_commit(transaction, msg, UPDATE_REFS_QUIET_ON_ERR))\n \t\treturn errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n \t\t\t\t\t STORE_REF_ERROR_OTHER;\n+\n \treturn 0;\n }\n \n-- \n1.9.1.518.g16976cb.dirty\n"},{"id":"239363","messageId":"1398192327-21302-4-git-send-email-sahlberg@google.com","threadId":"36474","inReplyTo":"1398192327-21302-1-git-send-email-sahlberg@google.com","subject":"[PATCH 3/3] fetch.c: use a single ref transaction for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-22T18:45:27Z","receivedAt":"2014-04-22T18:45:27Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change store_updated_refs to use a single ref transaction for all refs that\nare updated during the fetch. This makes the fetch more atomic when update\nfailures occur.\n\nSince ref update failures will now no longer occur in the code path for\nupdating a single ref in s_update_ref, we no longer have as detailed error\nmessage logging the exact reference and the ref log action as in the old code.\nInstead since we fail the entire transaction we log a much more generic\nmessage. But since we commit the transaction using MSG_ON_ERR we will log\nan error containing the ref name if either locking of writing the ref would\nso the regression in the log message is minor.\n\nThis will also change the order in which errors are checked for and logged\nwhich may alter which error will be logged if there are multiple errors\noccuring during a fetch.\n\nFor example, assuming we have a fetch for two refs that both would fail.\nWhere the first ref would fail with ENOTDIR due to a directory in the ref\npath not existing, and the second ref in the fetch would fail due to\nthe check in update_logical_ref():\n\tif (current_branch &&\n\t !strcmp(ref->name, current_branch->name) &&\n\t !(update_head_ok || is_bare_repository()) &&\n\t !is_null_sha1(ref->old_sha1)) {\n\t\t/*\n\t\t * If this is the head, and it's not okay to update\n\t\t * the head, and the old value of the head isn't empty...\n\t\t */\n\nIn the old code sicne we would update the refs one ref at a time we would\nfirst fail the ENOTDIR and then fail the second update of HEAD as well.\nBut since the first ref failed with ENOTDIR we would eventually fail the whole\nfetch with STORE_REF_ERROR_DF_CONFLICT\n\nIn the new code, since we defer committing the transaction until all refs\nhas been processed, we would now detect that the second ref was bad and\nrollback the transaction before we would even try start writing the update to\ndisk and thus we would not return STORE_REF_ERROR_DF_CONFLICT for this case.\n\nI think this new behaviour is more correct, since if there was a problem\nwe would not even try to commit the transaction but need to highlight this\nchange in how/what errors are reported.\nThis change in what error is returned only occurs if there are multiple\nrefs that fail to update and only some, but not all, of them fail due to\nENOTDIR.\n\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/fetch.c | 34 +++++++++++++++++++---------------\n 1 file changed, 19 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 5c15584..97c6b9a 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -45,6 +45,7 @@ static struct transport *gsecondary;\n static const char *submodule_prefix = \"\";\n static const char *recurse_submodules_default;\n static int shown_url;\n+struct ref_transaction *transaction;\n \n static int option_parse_recurse_submodules(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n@@ -373,24 +374,12 @@ static int s_update_ref(const char *action,\n \t\t\tstruct ref *ref,\n \t\t\tint check_old)\n {\n-\tchar msg[1024];\n-\tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n-\tstruct ref_transaction *transaction;\n-\n \tif (dry_run)\n \t\treturn 0;\n-\tif (!rla)\n-\t\trla = default_rla.buf;\n-\tsnprintf(msg, sizeof(msg), \"%s: %s\", rla, action);\n \n-\terrno = 0;\n-\ttransaction = ref_transaction_begin();\n-\tif (!transaction ||\n-\t ref_transaction_update(transaction, ref->name, ref->new_sha1,\n-\t\t\t\t ref->old_sha1, 0, check_old) ||\n-\t ref_transaction_commit(transaction, msg, UPDATE_REFS_QUIET_ON_ERR))\n-\t\treturn errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n-\t\t\t\t\t STORE_REF_ERROR_OTHER;\n+\tif (ref_transaction_update(transaction, ref->name, ref->new_sha1,\n+\t\t\t ref->old_sha1, 0, check_old))\n+\t\treturn STORE_REF_ERROR_OTHER;\n \n \treturn 0;\n }\n@@ -563,6 +552,13 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\tgoto abort;\n \t}\n \n+\terrno = 0;\n+\ttransaction = ref_transaction_begin();\n+\tif (!transaction) {\n+\t\trc = error(_(\"cannot start ref transaction\\n\"));\n+\t\tgoto abort;\n+\t}\n+\n \t/*\n \t * We do a pass for each fetch_head_status type in their enum order, so\n \t * merged entries are written before not-for-merge. That lets readers\n@@ -674,6 +670,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t}\n \t\t}\n \t}\n+\tif (rc)\n+\t\tref_transaction_rollback(transaction);\n+\telse {\n+\t\tif (ref_transaction_commit(transaction, \"fetch_ref transaction\",\n+\t\t\t\t\t UPDATE_REFS_MSG_ON_ERR))\n+\t\t\trc |= errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n+\t\t\t\t\t STORE_REF_ERROR_OTHER;\n+\t}\n \n \tif (rc & STORE_REF_ERROR_DF_CONFLICT)\n \t\terror(_(\"some local refs could not be updated; try running\\n\"\n-- \n1.9.1.518.g16976cb.dirty\n"},{"id":"239479","messageId":"CAPig+cTu4hUCF4r6wAHnGpfcoQP9SqDUWgA2SjjKbjDNrDG8fg@mail.gmail.com","threadId":"36474","inReplyTo":"1398192327-21302-2-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 1/3] fetch.c: clear errno before calling functions that might set it","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-04-23T20:12:03Z","receivedAt":"2014-04-23T20:12:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 22, 2014 at 2:45 PM, Ronnie Sahlberg wrote:\n> In s_update_ref there are two calls that when they fail we return an error\n> based on the errno value. In particular we want to return a specific error\n> if ENOTDIR happened. Both these functions do have failure modes where they\n> may return an error without updating errno, in which case a previous and\n> unrelated ENOTDIT may cause us to return the wrong error. Clear errno before\n> calling any functions if we check errno afterwards.\n>\n> Also skip initializing a static variable to 0. Sstatics live in .bss and\n\ns/Sstatics/Statics/\n\n> are all automatically initialized to 0.\n>\n> Signed-off-by: Ronnie Sahlberg \n> ---\n> builtin/fetch.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 55f457c..a93c893 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -44,7 +44,7 @@ static struct transport *gtransport;\n> static struct transport *gsecondary;\n> static const char *submodule_prefix = \"\";\n> static const char *recurse_submodules_default;\n> -static int shown_url = 0;\n> +static int shown_url;\n>\n> static int option_parse_recurse_submodules(const struct option *opt,\n> const char *arg, int unset)\n> @@ -382,6 +382,8 @@ static int s_update_ref(const char *action,\n> if (!rla)\n> rla = default_rla.buf;\n> snprintf(msg, sizeof(msg), \"%s: %s\", rla, action);\n> +\n> + errno = 0;\n> lock = lock_any_ref_for_update(ref->name,\n> check_old ? ref->old_sha1 : NULL,\n> 0, NULL);\n> --\n> 1.9.1.518.g16976cb.dirty\n"},{"id":"239481","messageId":"CAPig+cQUAVK7O7yRJGjvRmuPg06C1ZEi876gMsihFpGLgVa=Mg@mail.gmail.com","threadId":"36474","inReplyTo":"1398192327-21302-3-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 2/3] fetch.c: change s_update_ref to use a ref transaction","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-04-23T20:12:54Z","receivedAt":"2014-04-23T20:12:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 22, 2014 at 2:45 PM, Ronnie Sahlberg wrote:\n> Change s_update_ref to use a ref transaction for the ref update.\n> Signed-off-by: Ronnie Sahlberg \n>\n> Signed-off-by: Ronnie Sahlberg \n\nDoubled sign-off.\n\n> ---\n> builtin/fetch.c | 15 +++++++--------\n> 1 file changed, 7 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index a93c893..5c15584 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -375,7 +375,7 @@ static int s_update_ref(const char *action,\n> {\n> char msg[1024];\n> char *rla = getenv(\"GIT_REFLOG_ACTION\");\n> - static struct ref_lock *lock;\n> + struct ref_transaction *transaction;\n>\n> if (dry_run)\n> return 0;\n> @@ -384,15 +384,14 @@ static int s_update_ref(const char *action,\n> snprintf(msg, sizeof(msg), \"%s: %s\", rla, action);\n>\n> errno = 0;\n> - lock = lock_any_ref_for_update(ref->name,\n> - check_old ? ref->old_sha1 : NULL,\n> - 0, NULL);\n> - if (!lock)\n> - return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n> - STORE_REF_ERROR_OTHER;\n> - if (write_ref_sha1(lock, ref->new_sha1, msg) < 0)\n> + transaction = ref_transaction_begin();\n> + if (!transaction ||\n> + ref_transaction_update(transaction, ref->name, ref->new_sha1,\n> + ref->old_sha1, 0, check_old) ||\n> + ref_transaction_commit(transaction, msg, UPDATE_REFS_QUIET_ON_ERR))\n> return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n> STORE_REF_ERROR_OTHER;\n> +\n> return 0;\n> }\n>\n> --\n> 1.9.1.518.g16976cb.dirty\n"},{"id":"239483","messageId":"CAPig+cTxT3gZn+pOwCOyKVVzYrFO7CMeLP_TuCrRsUOQRFYz5w@mail.gmail.com","threadId":"36474","inReplyTo":"1398192327-21302-4-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 3/3] fetch.c: use a single ref transaction for all ref updates","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-04-23T20:17:54Z","receivedAt":"2014-04-23T20:17:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 22, 2014 at 2:45 PM, Ronnie Sahlberg wrote:\n> Change store_updated_refs to use a single ref transaction for all refs that\n> are updated during the fetch. This makes the fetch more atomic when update\n> failures occur.\n>\n> Since ref update failures will now no longer occur in the code path for\n> updating a single ref in s_update_ref, we no longer have as detailed error\n> message logging the exact reference and the ref log action as in the old code.\n> Instead since we fail the entire transaction we log a much more generic\n> message. But since we commit the transaction using MSG_ON_ERR we will log\n> an error containing the ref name if either locking of writing the ref would\n> so the regression in the log message is minor.\n>\n> This will also change the order in which errors are checked for and logged\n> which may alter which error will be logged if there are multiple errors\n> occuring during a fetch.\n>\n> For example, assuming we have a fetch for two refs that both would fail.\n\ns/assuming/assume/ perhaps?\n\n> Where the first ref would fail with ENOTDIR due to a directory in the ref\n> path not existing, and the second ref in the fetch would fail due to\n> the check in update_logical_ref():\n> if (current_branch &&\n> !strcmp(ref->name, current_branch->name) &&\n> !(update_head_ok || is_bare_repository()) &&\n> !is_null_sha1(ref->old_sha1)) {\n> /*\n> * If this is the head, and it's not okay to update\n> * the head, and the old value of the head isn't empty...\n> */\n>\n> In the old code sicne we would update the refs one ref at a time we would\n\ns/sicne/since/\n\n> first fail the ENOTDIR and then fail the second update of HEAD as well.\n> But since the first ref failed with ENOTDIR we would eventually fail the whole\n> fetch with STORE_REF_ERROR_DF_CONFLICT\n>\n> In the new code, since we defer committing the transaction until all refs\n> has been processed, we would now detect that the second ref was bad and\n\ns/has/have/\n\n> rollback the transaction before we would even try start writing the update to\n> disk and thus we would not return STORE_REF_ERROR_DF_CONFLICT for this case.\n>\n> I think this new behaviour is more correct, since if there was a problem\n> we would not even try to commit the transaction but need to highlight this\n> change in how/what errors are reported.\n> This change in what error is returned only occurs if there are multiple\n> refs that fail to update and only some, but not all, of them fail due to\n> ENOTDIR.\n>\n> Signed-off-by: Ronnie Sahlberg \n"},{"id":"239570","messageId":"CAL=YDWk9xd3d4Z8YoXvy7Z=5q=3=6+s_KaBS3W0yyESZ=ueDSA@mail.gmail.com","threadId":"36474","inReplyTo":"CAPig+cTu4hUCF4r6wAHnGpfcoQP9SqDUWgA2SjjKbjDNrDG8fg@mail.gmail.com","subject":"Re: [PATCH 1/3] fetch.c: clear errno before calling functions that might set it","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-24T15:21:32Z","receivedAt":"2014-04-24T15:21:32Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Fixed. Thanks.\n\nOn Wed, Apr 23, 2014 at 1:12 PM, Eric Sunshine wrote:\n> On Tue, Apr 22, 2014 at 2:45 PM, Ronnie Sahlberg wrote:\n>> In s_update_ref there are two calls that when they fail we return an error\n>> based on the errno value. In particular we want to return a specific error\n>> if ENOTDIR happened. Both these functions do have failure modes where they\n>> may return an error without updating errno, in which case a previous and\n>> unrelated ENOTDIT may cause us to return the wrong error. Clear errno before\n>> calling any functions if we check errno afterwards.\n>>\n>> Also skip initializing a static variable to 0. Sstatics live in .bss and\n>\n> s/Sstatics/Statics/\n>\n>> are all automatically initialized to 0.\n>>\n>> Signed-off-by: Ronnie Sahlberg \n>> ---\n>> builtin/fetch.c | 4 +++-\n>> 1 file changed, 3 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> index 55f457c..a93c893 100644\n>> --- a/builtin/fetch.c\n>> +++ b/builtin/fetch.c\n>> @@ -44,7 +44,7 @@ static struct transport *gtransport;\n>> static struct transport *gsecondary;\n>> static const char *submodule_prefix = \"\";\n>> static const char *recurse_submodules_default;\n>> -static int shown_url = 0;\n>> +static int shown_url;\n>>\n>> static int option_parse_recurse_submodules(const struct option *opt,\n>> const char *arg, int unset)\n>> @@ -382,6 +382,8 @@ static int s_update_ref(const char *action,\n>> if (!rla)\n>> rla = default_rla.buf;\n>> snprintf(msg, sizeof(msg), \"%s: %s\", rla, action);\n>> +\n>> + errno = 0;\n>> lock = lock_any_ref_for_update(ref->name,\n>> check_old ? ref->old_sha1 : NULL,\n>> 0, NULL);\n>> --\n>> 1.9.1.518.g16976cb.dirty\n"},{"id":"239569","messageId":"CAL=YDWm5NqkBSCkxabCO3_tDqPyuugD8K9FHZ5dsw9dAEXf5Eg@mail.gmail.com","threadId":"36474","inReplyTo":"CAPig+cQUAVK7O7yRJGjvRmuPg06C1ZEi876gMsihFpGLgVa=Mg@mail.gmail.com","subject":"Re: [PATCH 2/3] fetch.c: change s_update_ref to use a ref transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-24T15:22:29Z","receivedAt":"2014-04-24T15:22:29Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Fixed. Thanks.\n\nOn Wed, Apr 23, 2014 at 1:12 PM, Eric Sunshine wrote:\n> On Tue, Apr 22, 2014 at 2:45 PM, Ronnie Sahlberg wrote:\n>> Change s_update_ref to use a ref transaction for the ref update.\n>> Signed-off-by: Ronnie Sahlberg \n>>\n>> Signed-off-by: Ronnie Sahlberg \n>\n> Doubled sign-off.\n>\n>> ---\n>> builtin/fetch.c | 15 +++++++--------\n>> 1 file changed, 7 insertions(+), 8 deletions(-)\n>>\n>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> index a93c893..5c15584 100644\n>> --- a/builtin/fetch.c\n>> +++ b/builtin/fetch.c\n>> @@ -375,7 +375,7 @@ static int s_update_ref(const char *action,\n>> {\n>> char msg[1024];\n>> char *rla = getenv(\"GIT_REFLOG_ACTION\");\n>> - static struct ref_lock *lock;\n>> + struct ref_transaction *transaction;\n>>\n>> if (dry_run)\n>> return 0;\n>> @@ -384,15 +384,14 @@ static int s_update_ref(const char *action,\n>> snprintf(msg, sizeof(msg), \"%s: %s\", rla, action);\n>>\n>> errno = 0;\n>> - lock = lock_any_ref_for_update(ref->name,\n>> - check_old ? ref->old_sha1 : NULL,\n>> - 0, NULL);\n>> - if (!lock)\n>> - return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n>> - STORE_REF_ERROR_OTHER;\n>> - if (write_ref_sha1(lock, ref->new_sha1, msg) < 0)\n>> + transaction = ref_transaction_begin();\n>> + if (!transaction ||\n>> + ref_transaction_update(transaction, ref->name, ref->new_sha1,\n>> + ref->old_sha1, 0, check_old) ||\n>> + ref_transaction_commit(transaction, msg, UPDATE_REFS_QUIET_ON_ERR))\n>> return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n>> STORE_REF_ERROR_OTHER;\n>> +\n>> return 0;\n>> }\n>>\n>> --\n>> 1.9.1.518.g16976cb.dirty\n"},{"id":"239571","messageId":"CAL=YDWkVEvDY2fbne1VomqFxyZ3tfnykd+k_y8aA6vx7Bu0Gpg@mail.gmail.com","threadId":"36474","inReplyTo":"CAPig+cTxT3gZn+pOwCOyKVVzYrFO7CMeLP_TuCrRsUOQRFYz5w@mail.gmail.com","subject":"Re: [PATCH 3/3] fetch.c: use a single ref transaction for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-24T15:23:52Z","receivedAt":"2014-04-24T15:23:52Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Fixed, thanks!\n\nOn Wed, Apr 23, 2014 at 1:17 PM, Eric Sunshine wrote:\n> On Tue, Apr 22, 2014 at 2:45 PM, Ronnie Sahlberg wrote:\n>> Change store_updated_refs to use a single ref transaction for all refs that\n>> are updated during the fetch. This makes the fetch more atomic when update\n>> failures occur.\n>>\n>> Since ref update failures will now no longer occur in the code path for\n>> updating a single ref in s_update_ref, we no longer have as detailed error\n>> message logging the exact reference and the ref log action as in the old code.\n>> Instead since we fail the entire transaction we log a much more generic\n>> message. But since we commit the transaction using MSG_ON_ERR we will log\n>> an error containing the ref name if either locking of writing the ref would\n>> so the regression in the log message is minor.\n>>\n>> This will also change the order in which errors are checked for and logged\n>> which may alter which error will be logged if there are multiple errors\n>> occuring during a fetch.\n>>\n>> For example, assuming we have a fetch for two refs that both would fail.\n>\n> s/assuming/assume/ perhaps?\n>\n>> Where the first ref would fail with ENOTDIR due to a directory in the ref\n>> path not existing, and the second ref in the fetch would fail due to\n>> the check in update_logical_ref():\n>> if (current_branch &&\n>> !strcmp(ref->name, current_branch->name) &&\n>> !(update_head_ok || is_bare_repository()) &&\n>> !is_null_sha1(ref->old_sha1)) {\n>> /*\n>> * If this is the head, and it's not okay to update\n>> * the head, and the old value of the head isn't empty...\n>> */\n>>\n>> In the old code sicne we would update the refs one ref at a time we would\n>\n> s/sicne/since/\n>\n>> first fail the ENOTDIR and then fail the second update of HEAD as well.\n>> But since the first ref failed with ENOTDIR we would eventually fail the whole\n>> fetch with STORE_REF_ERROR_DF_CONFLICT\n>>\n>> In the new code, since we defer committing the transaction until all refs\n>> has been processed, we would now detect that the second ref was bad and\n>\n> s/has/have/\n>\n>> rollback the transaction before we would even try start writing the update to\n>> disk and thus we would not return STORE_REF_ERROR_DF_CONFLICT for this case.\n>>\n>> I think this new behaviour is more correct, since if there was a problem\n>> we would not even try to commit the transaction but need to highlight this\n>> change in how/what errors are reported.\n>> This change in what error is returned only occurs if there are multiple\n>> refs that fail to update and only some, but not all, of them fail due to\n>> ENOTDIR.\n>>\n>> Signed-off-by: Ronnie Sahlberg \n"},{"id":"240723","messageId":"53677483.4050409@alum.mit.edu","threadId":"36474","inReplyTo":"1398192327-21302-1-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 0/3] Use ref transactions for fetch","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-05-05T11:22:43Z","receivedAt":"2014-05-05T11:22:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/22/2014 08:45 PM, Ronnie Sahlberg wrote:\n> This change is based on the previous ref transaction patches.\n> This is sent as a separate patch series since it implements a lot more\n> non-trivial changes to the behaviour than the previous patches and thus can\n> use more detailed review.\n> \n> Update fetch.c to use ref transactions when performing updates. Use a single\n> ref transaction for all updates and only commit the transaction if all other\n> checks and oeprations have been successful. This makes the ref updates during\n> a fetch (mostly) atomic.\n\nIs this always an improvement? What kind of checks are there that might\nfail?\n\nIt would be pretty annoying to spend a lot of time fetching a big pack,\nonly to have the fetch fail because one reference out of many couldn't\nbe updated. This would force the user to download the entire pack\nagain, whereas if the successful reference updates had been allowed,\nthen probably most or all of the second download would have been avoidable.\n\nOn the other hand, if a reference was renamed on the remote side,\nallowing a partial reference update could cause history to be discarded\nlocally if the old name's delete was accepted but the new name's\naddition was rejected. This wouldn't be the end of the world, because\nthe history is presumably still available remotely to fetch again, but\nit's not ideal either.\n\nI'm not sure myself what I would prefer, but I wanted to point out that\nit is IMO not obvious that atomicity here is an improvement.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"240738","messageId":"CAL=YDWmG8Q0kk+4fc6C15Ai2cTr-K8bNn--J7MrjNnHVuW44jg@mail.gmail.com","threadId":"36474","inReplyTo":"53677483.4050409@alum.mit.edu","subject":"Re: [PATCH 0/3] Use ref transactions for fetch","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-05T15:08:43Z","receivedAt":"2014-05-05T15:08:43Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Mon, May 5, 2014 at 4:22 AM, Michael Haggerty wrote:\n> On 04/22/2014 08:45 PM, Ronnie Sahlberg wrote:\n>> This change is based on the previous ref transaction patches.\n>> This is sent as a separate patch series since it implements a lot more\n>> non-trivial changes to the behaviour than the previous patches and thus can\n>> use more detailed review.\n>>\n>> Update fetch.c to use ref transactions when performing updates. Use a single\n>> ref transaction for all updates and only commit the transaction if all other\n>> checks and oeprations have been successful. This makes the ref updates during\n>> a fetch (mostly) atomic.\n>\n> Is this always an improvement? What kind of checks are there that might\n> fail?\n>\n> It would be pretty annoying to spend a lot of time fetching a big pack,\n> only to have the fetch fail because one reference out of many couldn't\n> be updated. This would force the user to download the entire pack\n> again, whereas if the successful reference updates had been allowed,\n> then probably most or all of the second download would have been avoidable.\n>\n> On the other hand, if a reference was renamed on the remote side,\n> allowing a partial reference update could cause history to be discarded\n> locally if the old name's delete was accepted but the new name's\n> addition was rejected. This wouldn't be the end of the world, because\n> the history is presumably still available remotely to fetch again, but\n> it's not ideal either.\n>\n> I'm not sure myself what I would prefer, but I wanted to point out that\n> it is IMO not obvious that atomicity here is an improvement.\n>\n\nWe could make it a .git/config option ?\n"},{"id":"240815","messageId":"xmqq61lig38j.fsf@gitster.dls.corp.google.com","threadId":"36474","inReplyTo":"53677483.4050409@alum.mit.edu","subject":"Re: [PATCH 0/3] Use ref transactions for fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-06T18:40:28Z","receivedAt":"2014-05-06T18:40:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> It would be pretty annoying to spend a lot of time fetching a big pack,\n> only to have the fetch fail because one reference out of many couldn't\n> be updated. This would force the user to download the entire pack\n> again,...\n\nIs that really true? Doesn't quickfetch optimization kick in for\nthe second fetch?\n"},{"id":"240837","messageId":"53694BB4.2030904@alum.mit.edu","threadId":"36474","inReplyTo":"xmqq61lig38j.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/3] Use ref transactions for fetch","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-05-06T20:53:08Z","receivedAt":"2014-05-06T20:53:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/06/2014 08:40 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> It would be pretty annoying to spend a lot of time fetching a big pack,\n>> only to have the fetch fail because one reference out of many couldn't\n>> be updated. This would force the user to download the entire pack\n>> again,...\n> \n> Is that really true? Doesn't quickfetch optimization kick in for\n> the second fetch?\n\nYes, I guess it would. I wasn't aware of that optimization. Thanks for\nthe pointer.\n\nI withdraw my objection to using atomic reference updates for fetch.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}