From: Ronnie Sahlberg <sahlberg@google.com>
To: git@vger.kernel.org
Cc: mhagger@alum.mit.edu, Ronnie Sahlberg <sahlberg@google.com>
Subject: [PATCH v5 22/30] fetch.c: use a single ref transaction for all ref updates
Date: Tue, 29 Apr 2014 15:19:07 -0700 [thread overview]
Message-ID: <1398809955-32008-23-git-send-email-sahlberg@google.com> (raw)
In-Reply-To: <1398809955-32008-1-git-send-email-sahlberg@google.com>
Change store_updated_refs to use a single ref transaction for all refs that
are updated during the fetch. This makes the fetch more atomic when update
failures occur.
Since ref update failures will now no longer occur in the code path for
updating a single ref in s_update_ref, we no longer have as detailed error
message logging the exact reference and the ref log action as in the old cod
Instead since we fail the entire transaction we log a much more generic
message. But since we commit the transaction using MSG_ON_ERR we will log
an error containing the ref name if either locking of writing the ref would
so the regression in the log message is minor.
This will also change the order in which errors are checked for and logged
which may alter which error will be logged if there are multiple errors
occuring during a fetch.
For example, assume we have a fetch for two refs that both would fail.
Where the first ref would fail with ENOTDIR due to a directory in the ref
path not existing, and the second ref in the fetch would fail due to
the check in update_logical_ref():
if (current_branch &&
!strcmp(ref->name, current_branch->name) &&
!(update_head_ok || is_bare_repository()) &&
!is_null_sha1(ref->old_sha1)) {
/*
* If this is the head, and it's not okay to update
* the head, and the old value of the head isn't empty...
*/
In the old code since we would update the refs one ref at a time we would
first fail the ENOTDIR and then fail the second update of HEAD as well.
But since the first ref failed with ENOTDIR we would eventually fail the who
fetch with STORE_REF_ERROR_DF_CONFLICT
In the new code, since we defer committing the transaction until all refs
have been processed, we would now detect that the second ref was bad and
rollback the transaction before we would even try start writing the update t
disk and thus we would not return STORE_REF_ERROR_DF_CONFLICT for this case.
I think this new behaviour is more correct, since if there was a problem
we would not even try to commit the transaction but need to highlight this
change in how/what errors are reported.
This change in what error is returned only occurs if there are multiple
refs that fail to update and only some, but not all, of them fail due to
ENOTDIR.
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
builtin/fetch.c | 34 ++++++++++++++++------------------
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b41d7b7..5dbd0f0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -45,6 +45,7 @@ static struct transport *gsecondary;
static const char *submodule_prefix = "";
static const char *recurse_submodules_default;
static int shown_url;
+struct ref_transaction *transaction;
static int option_parse_recurse_submodules(const struct option *opt,
const char *arg, int unset)
@@ -373,27 +374,13 @@ static int s_update_ref(const char *action,
struct ref *ref,
int check_old)
{
- char msg[1024];
- char *rla = getenv("GIT_REFLOG_ACTION");
- struct ref_transaction *transaction;
-
if (dry_run)
return 0;
- if (!rla)
- rla = default_rla.buf;
- snprintf(msg, sizeof(msg), "%s: %s", rla, action);
- errno = 0;
- transaction = ref_transaction_begin();
- if (!transaction ||
- ref_transaction_update(transaction, ref->name, ref->new_sha1,
- ref->old_sha1, 0, check_old) ||
- ref_transaction_commit(transaction, msg, NULL)) {
- ref_transaction_rollback(transaction);
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
- }
- ref_transaction_free(transaction);
+ if (ref_transaction_update(transaction, ref->name, ref->new_sha1,
+ ref->old_sha1, 0, check_old))
+ return STORE_REF_ERROR_OTHER;
+
return 0;
}
@@ -565,6 +552,13 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
goto abort;
}
+ errno = 0;
+ transaction = ref_transaction_begin();
+ if (!transaction) {
+ rc = error(_("cannot start ref transaction\n"));
+ goto abort;
+ }
+
/*
* We do a pass for each fetch_head_status type in their enum order, so
* merged entries are written before not-for-merge. That lets readers
@@ -676,6 +670,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
}
}
}
+ if (ref_transaction_commit(transaction, "fetch_ref transaction", NULL))
+ rc |= errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
+ STORE_REF_ERROR_OTHER;
+ ref_transaction_free(transaction);
if (rc & STORE_REF_ERROR_DF_CONFLICT)
error(_("some local refs could not be updated; try running\n"
--
1.9.1.532.gf8485a6
next prev parent reply other threads:[~2014-04-29 22:20 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-04-29 22:18 [PATCH v5 00/30] Use ref transactions for all ref updates Ronnie Sahlberg
2014-04-29 22:18 ` [PATCH v5 01/30] refs.c: constify the sha arguments for ref_transaction_create|delete|update Ronnie Sahlberg
2014-04-29 22:18 ` [PATCH v5 02/30] refs.c: allow passing NULL to ref_transaction_free Ronnie Sahlberg
2014-04-29 22:18 ` [PATCH v5 03/30] refs.c: add a strbuf argument to ref_transaction_commit for error logging Ronnie Sahlberg
2014-04-29 22:18 ` [PATCH v5 04/30] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors Ronnie Sahlberg
2014-04-29 22:18 ` [PATCH v5 05/30] update-ref.c: log transaction error from the update_ref Ronnie Sahlberg
2014-04-29 22:18 ` [PATCH v5 06/30] refs.c: make update_ref_write update a strbuf on failure Ronnie Sahlberg
2014-04-29 22:18 ` [PATCH v5 07/30] refs.c: remove the onerr argument to ref_transaction_commit Ronnie Sahlberg
2014-04-29 22:18 ` [PATCH v5 08/30] refs.c: change ref_transaction_update() to do error checking and return status Ronnie Sahlberg
2014-04-29 22:18 ` [PATCH v5 09/30] refs.c: change ref_transaction_create " Ronnie Sahlberg
2014-04-29 22:18 ` [PATCH v5 10/30] refs.c: ref_transaction_delete to check for error " Ronnie Sahlberg
2014-04-29 22:18 ` [PATCH v5 11/30] tag.c: use ref transactions when doing updates Ronnie Sahlberg
2014-04-29 22:18 ` [PATCH v5 12/30] replace.c: use the ref transaction functions for updates Ronnie Sahlberg
2014-04-29 22:18 ` [PATCH v5 13/30] commit.c: use ref transactions " Ronnie Sahlberg
2014-04-29 22:18 ` [PATCH v5 14/30] sequencer.c: use ref transactions for all ref updates Ronnie Sahlberg
2014-04-29 22:19 ` [PATCH v5 15/30] fast-import.c: change update_branch to use ref transactions Ronnie Sahlberg
2014-04-29 22:19 ` [PATCH v5 16/30] branch.c: use ref transaction for all ref updates Ronnie Sahlberg
2014-04-29 22:19 ` [PATCH v5 17/30] refs.c: change update_ref to use a transaction Ronnie Sahlberg
2014-04-29 22:19 ` [PATCH v5 18/30] refs.c: free the transaction before returning when number of updates is 0 Ronnie Sahlberg
2014-04-29 22:19 ` [PATCH v5 19/30] refs.c: ref_transaction_commit should not free the transaction Ronnie Sahlberg
2014-04-29 22:19 ` [PATCH v5 20/30] fetch.c: clear errno before calling functions that might set it Ronnie Sahlberg
2014-04-29 22:19 ` [PATCH v5 21/30] fetch.c: change s_update_ref to use a ref transaction Ronnie Sahlberg
2014-04-29 22:19 ` Ronnie Sahlberg [this message]
2014-04-29 22:19 ` [PATCH v5 23/30] receive-pack.c: use a reference transaction for updating the refs Ronnie Sahlberg
2014-04-29 22:19 ` [PATCH v5 24/30] fast-import.c: use a ref transaction when dumping tags Ronnie Sahlberg
2014-04-29 22:19 ` [PATCH v5 25/30] walker.c: use ref transaction for ref updates Ronnie Sahlberg
2014-04-29 22:19 ` [PATCH v5 26/30] refs.c: make write_ref_sha1 static Ronnie Sahlberg
2014-04-29 22:19 ` [PATCH v5 27/30] refs.c: make lock_ref_sha1 static Ronnie Sahlberg
2014-04-29 22:19 ` [PATCH v5 28/30] refs.c: add transaction.status and track OPEN/CLOSED/ERROR Ronnie Sahlberg
2014-04-29 22:19 ` [PATCH v5 29/30] refs.c: remove the update_ref_lock function Ronnie Sahlberg
2014-04-29 22:19 ` [PATCH v5 30/30] refs.c: remove the update_ref_write function Ronnie Sahlberg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1398809955-32008-23-git-send-email-sahlberg@google.com \
--to=sahlberg@google.com \
--cc=git@vger.kernel.org \
--cc=mhagger@alum.mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).