git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] Make update refs more atomic
@ 2014-04-11 20:39 Ronnie Sahlberg
  2014-04-11 20:39 ` [PATCH v3 1/3] refs.c: split writing and commiting a ref into two separate functions Ronnie Sahlberg
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Ronnie Sahlberg @ 2014-04-11 20:39 UTC (permalink / raw)
  To: git; +Cc: Ronnie Sahlberg

refs.c:ref_transaction_commit() intermingles doing updates and checks with
actually applying changes to the refs in loops that abort on error.
This is done one ref at a time and means that if an error is detected that
will fail the operation partway through the list of refs to update we
will end up with some changes applied to disk and others not.

Without having transaction support from the filesystem, it is hard to
make an update that involves multiple refs to guarantee atomicity, but we
can do a somewhat better than we currently do.

These patches change the update and delete functions to use a three
call pattern of

1, lock
2, update, or flag for deletion
3, apply on disk  (rename() or unlink())

When a transaction is commited we first do all the locking, preparations
and most of the error checking before we actually start applying any changes
to the filesystem store.

This means that more of the error cases that will fail the commit
will trigger before we start doing any changes to the actual files.


This should make the changes of refs in refs_transaction_commit slightly
more atomic.



Version 3:
* Rebased onto mhagger/ref-transactions.
* Removed the patch to do update/delete from a single loop.

Version 2:
Updates and fixes based on Junio's feedback.
* Fix the subject line for patches so they comply with the project standard.
* Redo the update/delete loops so that we maintain the correct order of
  operations. Perform all updates first, then perform the deletes.
* Add an additional patch that allows us to do the update/delete in the correct
  order from within a single loop by first sorting the refs so that deletes
  are after all non-deletes.



Ronnie Sahlberg (3):
  refs.c: split writing and commiting a ref into two separate functions
  refs.c: split delete_ref_loose() into a separate flag-for-deletion and
    commit phase
  refs.c: change update_refs to run the commit loops once all work is
    finished

 branch.c               | 10 ++++-
 builtin/commit.c       |  5 +++
 builtin/fetch.c        |  7 +++-
 builtin/receive-pack.c |  4 ++
 builtin/replace.c      |  6 ++-
 builtin/tag.c          |  6 ++-
 fast-import.c          |  7 +++- refs.c                 | 99 ++++++++++++++++++++++++++++++++++----------------
 refs.h                 |  6 +++
 sequencer.c            |  4 ++
 walker.c               |  4 ++
 11 files changed, 120 insertions(+), 38 deletions(-)

-- 
1.9.1.505.g6fcb662

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 1/3] refs.c: split writing and commiting a ref into two separate functions
  2014-04-11 20:39 [PATCH v3 0/3] Make update refs more atomic Ronnie Sahlberg
@ 2014-04-11 20:39 ` Ronnie Sahlberg
  2014-04-11 20:39 ` [PATCH v3 2/3] refs.c: split delete_ref_loose() into a separate flag-for-deletion and commit phase Ronnie Sahlberg
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Ronnie Sahlberg @ 2014-04-11 20:39 UTC (permalink / raw)
  To: git; +Cc: Ronnie Sahlberg

Change the function write_ref_sha1() to just write the ref but not
commit the ref or the lockfile.
Add a new function commit_ref_lock() that will commit the change done by
a previous write_ref_sha1().
Update all callers of write_ref_sha1() to call commit_ref_lock().

The new pattern for updating a ref is now :

lock = lock_ref_sha1_basic() (or varient of)
write_ref_sha1(lock)
unlock_ref(lock) | commit_ref_lock(lock)

Once write_ref_sha1() returns, the new ref has been written and the lock
file has been closed.
At that stage we can then either call unlock_ref() which will abort the
update and delete the lock file withouth applying it, or call
commit_ref_lock() which will rename the lock file onto the ref file.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 branch.c               | 10 ++++++++--
 builtin/commit.c       |  5 +++++
 builtin/fetch.c        |  7 ++++++-
 builtin/receive-pack.c |  4 ++++
 builtin/replace.c      |  6 +++++-
 builtin/tag.c          |  6 +++++-
 fast-import.c          |  7 ++++++-
 refs.c                 | 41 +++++++++++++++++++++++++++++++----------
 refs.h                 |  4 ++++
 sequencer.c            |  4 ++++
 walker.c               |  4 ++++
 11 files changed, 82 insertions(+), 16 deletions(-)

diff --git a/branch.c b/branch.c
index 660097b..903ea75 100644
--- a/branch.c
+++ b/branch.c
@@ -304,9 +304,15 @@ void create_branch(const char *head,
 	if (real_ref && track)
 		setup_tracking(ref.buf + 11, real_ref, track, quiet);
 
-	if (!dont_change_ref)
-		if (write_ref_sha1(lock, sha1, msg) < 0)
+	if (!dont_change_ref) {
+		if (write_ref_sha1(lock, sha1, msg) < 0) {
+			unlock_ref(lock);
 			die_errno(_("Failed to write ref"));
+		}
+		if (commit_ref_lock(lock) < 0) {
+			die_errno(_("Failed to commit ref"));
+		}
+	}
 
 	strbuf_release(&ref);
 	free(real_ref);
diff --git a/builtin/commit.c b/builtin/commit.c
index d9550c5..3d8a3a8 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1686,9 +1686,14 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		die(_("cannot lock HEAD ref"));
 	}
 	if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
+		unlock_ref(ref_lock);
 		rollback_index_files();
 		die(_("cannot update HEAD ref"));
 	}
+	if (commit_ref_lock(ref_lock) < 0) {
+		rollback_index_files();
+		die(_("cannot commit HEAD ref"));
+	}
 
 	unlink(git_path("CHERRY_PICK_HEAD"));
 	unlink(git_path("REVERT_HEAD"));
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 55f457c..ebfb854 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -388,7 +388,12 @@ static int s_update_ref(const char *action,
 	if (!lock)
 		return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
 					  STORE_REF_ERROR_OTHER;
-	if (write_ref_sha1(lock, ref->new_sha1, msg) < 0)
+	if (write_ref_sha1(lock, ref->new_sha1, msg) < 0) {
+		unlock_ref(lock);
+		return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
+					  STORE_REF_ERROR_OTHER;
+	}
+	if (commit_ref_lock(lock) < 0)
 		return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
 					  STORE_REF_ERROR_OTHER;
 	return 0;
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..4760274 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -587,8 +587,12 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 			return "failed to lock";
 		}
 		if (write_ref_sha1(lock, new_sha1, "push")) {
+			unlock_ref(lock);
 			return "failed to write"; /* error() already called */
 		}
+		if (commit_ref_lock(lock))
+			return "failed to commit"; /* error() already called */
+
 		return NULL; /* good */
 	}
 }
diff --git a/builtin/replace.c b/builtin/replace.c
index b62420a..c09ff49 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -160,8 +160,12 @@ static int replace_object(const char *object_ref, const char *replace_ref,
 	lock = lock_any_ref_for_update(ref, prev, 0, NULL);
 	if (!lock)
 		die("%s: cannot lock the ref", ref);
-	if (write_ref_sha1(lock, repl, NULL) < 0)
+	if (write_ref_sha1(lock, repl, NULL) < 0) {
+		unlock_ref(lock);
 		die("%s: cannot update the ref", ref);
+	}
+	if (commit_ref_lock(lock) < 0)
+		die("%s: cannot commit the ref", ref);
 
 	return 0;
 }
diff --git a/builtin/tag.c b/builtin/tag.c
index 40356e3..8653a64 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -644,8 +644,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
 	if (!lock)
 		die(_("%s: cannot lock the ref"), ref.buf);
-	if (write_ref_sha1(lock, object, NULL) < 0)
+	if (write_ref_sha1(lock, object, NULL) < 0) {
+		unlock_ref(lock);
 		die(_("%s: cannot update the ref"), ref.buf);
+	}
+	if (commit_ref_lock(lock) < 0)
+		die(_("%s: cannot commit the ref"), ref.buf);
 	if (force && !is_null_sha1(prev) && hashcmp(prev, object))
 		printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));
 
diff --git a/fast-import.c b/fast-import.c
index fb4738d..65c41ad 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1706,8 +1706,13 @@ static int update_branch(struct branch *b)
 			return -1;
 		}
 	}
-	if (write_ref_sha1(lock, b->sha1, msg) < 0)
+	if (write_ref_sha1(lock, b->sha1, msg) < 0) {
+		unlock_ref(lock);
 		return error("Unable to update %s", b->name);
+	}
+	if (commit_ref_lock(lock) < 0) {
+		return error("Unable to commit %s", b->name);
+	}
 	return 0;
 }
 
diff --git a/refs.c b/refs.c
index 728a761..815e328 100644
--- a/refs.c
+++ b/refs.c
@@ -2633,9 +2633,14 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	lock->force_write = 1;
 	hashcpy(lock->old_sha1, orig_sha1);
 	if (write_ref_sha1(lock, orig_sha1, logmsg)) {
+		unlock_ref(lock);
 		error("unable to write current sha1 into %s", newrefname);
 		goto rollback;
 	}
+	if (commit_ref_lock(lock)) {
+		error("unable to commit current sha1 into %s", newrefname);
+		goto rollback;
+	}
 
 	return 0;
 
@@ -2649,8 +2654,12 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	lock->force_write = 1;
 	flag = log_all_ref_updates;
 	log_all_ref_updates = 0;
-	if (write_ref_sha1(lock, orig_sha1, NULL))
+	if (write_ref_sha1(lock, orig_sha1, NULL)) {
+		unlock_ref(lock);
 		error("unable to write current sha1 into %s", oldrefname);
+	}
+	if (commit_ref_lock(lock))
+		error("unable to commit current sha1 into %s", oldrefname);
 	log_all_ref_updates = flag;
 
  rollbacklog:
@@ -2807,34 +2816,30 @@ int write_ref_sha1(struct ref_lock *lock,
 	if (!lock)
 		return -1;
 	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
-		unlock_ref(lock);
+		lock->skipped_write = 1;
 		return 0;
 	}
 	o = parse_object(sha1);
 	if (!o) {
 		error("Trying to write ref %s with nonexistent object %s",
 			lock->ref_name, sha1_to_hex(sha1));
-		unlock_ref(lock);
 		return -1;
 	}
 	if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {
 		error("Trying to write non-commit object %s to branch %s",
 			sha1_to_hex(sha1), lock->ref_name);
-		unlock_ref(lock);
 		return -1;
 	}
 	if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||
 	    write_in_full(lock->lock_fd, &term, 1) != 1
 		|| close_ref(lock) < 0) {
 		error("Couldn't write %s", lock->lk->filename);
-		unlock_ref(lock);
 		return -1;
 	}
 	clear_loose_ref_cache(&ref_cache);
 	if (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||
 	    (strcmp(lock->ref_name, lock->orig_ref_name) &&
 	     log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {
-		unlock_ref(lock);
 		return -1;
 	}
 	if (strcmp(lock->orig_ref_name, "HEAD") != 0) {
@@ -2858,7 +2863,12 @@ int write_ref_sha1(struct ref_lock *lock,
 		    !strcmp(head_ref, lock->ref_name))
 			log_ref_write("HEAD", lock->old_sha1, sha1, logmsg);
 	}
-	if (commit_ref(lock)) {
+	return 0;
+}
+
+int commit_ref_lock(struct ref_lock *lock)
+{
+        if (!lock->skipped_write && commit_ref(lock)) {
 		error("Couldn't set %s", lock->ref_name);
 		unlock_ref(lock);
 		return -1;
@@ -3375,10 +3385,17 @@ int update_ref(const char *action, const char *refname,
 	       int flags, enum action_on_err onerr)
 {
 	struct ref_lock *lock;
+	int ret;
+
 	lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
 	if (!lock)
 		return 1;
-	return update_ref_write(action, refname, sha1, lock, onerr);
+	ret = update_ref_write(action, refname, sha1, lock, onerr);
+	if (ret)
+		unlock_ref(lock);
+	else
+		ret = commit_ref_lock(lock);
+	return ret;
 }
 
 static int ref_update_compare(const void *r1, const void *r2)
@@ -3453,7 +3470,11 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 					       update->refname,
 					       update->new_sha1,
 					       update->lock, onerr);
-			update->lock = NULL; /* freed by update_ref_write */
+			if (ret)
+				unlock_ref(update->lock);
+			else
+				commit_ref_lock(update->lock);
+			update->lock = NULL;
 			if (ret)
 				goto cleanup;
 		}
@@ -3464,7 +3485,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		struct ref_update *update = updates[i];
 
 		if (update->lock) {
-			delnames[delnum++] = update->lock->ref_name;
+			delnames[delnum++] = update->refname;
 			ret |= delete_ref_loose(update->lock, update->type);
 		}
 	}
diff --git a/refs.h b/refs.h
index 0f08def..f14a417 100644
--- a/refs.h
+++ b/refs.h
@@ -8,6 +8,7 @@ struct ref_lock {
 	unsigned char old_sha1[20];
 	int lock_fd;
 	int force_write;
+	int skipped_write;
 };
 
 struct ref_transaction;
@@ -153,6 +154,9 @@ extern void unlock_ref(struct ref_lock *lock);
 /** Writes sha1 into the ref specified by the lock. **/
 extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);
 
+/** Commit any changes done to the ref specified by the lock. **/
+extern int commit_ref_lock(struct ref_lock *lock);
+
 /** Setup reflog before using. **/
 int log_ref_setup(const char *refname, char *logfile, int bufsize);
 
diff --git a/sequencer.c b/sequencer.c
index bde5f04..ffadf82 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -283,6 +283,10 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 					   0, NULL);
 	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
 	ret = write_ref_sha1(ref_lock, to, sb.buf);
+	if (ret)
+		unlock_ref(ref_lock);
+	else
+		ret |= commit_ref_lock(ref_lock);
 	strbuf_release(&sb);
 	return ret;
 }
diff --git a/walker.c b/walker.c
index 1dd86b8..5ce5a1d 100644
--- a/walker.c
+++ b/walker.c
@@ -295,6 +295,10 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 		if (!write_ref || !write_ref[i])
 			continue;
 		ret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : "fetch (unknown)");
+		if (ret)
+			unlock_ref(lock[i]);
+		else
+			ret |= commit_ref_lock(lock[i]);
 		lock[i] = NULL;
 		if (ret)
 			goto unlock_and_fail;
-- 
1.9.1.505.g4f1e74f

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 2/3] refs.c: split delete_ref_loose() into a separate flag-for-deletion and commit phase
  2014-04-11 20:39 [PATCH v3 0/3] Make update refs more atomic Ronnie Sahlberg
  2014-04-11 20:39 ` [PATCH v3 1/3] refs.c: split writing and commiting a ref into two separate functions Ronnie Sahlberg
@ 2014-04-11 20:39 ` Ronnie Sahlberg
  2014-04-11 21:51   ` Junio C Hamano
  2014-04-11 20:39 ` [PATCH v3 3/3] refs.c: change ref_transaction_commit to run the commit loops once all work is finished Ronnie Sahlberg
  2014-04-11 22:27 ` [PATCH v3 0/3] Make update refs more atomic Junio C Hamano
  3 siblings, 1 reply; 6+ messages in thread
From: Ronnie Sahlberg @ 2014-04-11 20:39 UTC (permalink / raw)
  To: git; +Cc: Ronnie Sahlberg

Change delete_ref_loose()) to just flag that a ref is to be deleted but do
not actually unlink the files.
Change commit_ref_lock() so that it will unlink refs that are flagged for
deletion.
Change all callers of delete_ref_loose() to explicitely call commit_ref_lock()
to commit the deletion.

The new pattern for deleting loose refs thus become:

lock = lock_ref_sha1_basic() (or varient of)
delete_ref_loose(lock)
unlock_ref(lock) | commit_ref_lock(lock)

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 31 ++++++++++++++++++++-----------
 refs.h |  2 ++
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/refs.c b/refs.c
index 815e328..ec104f2 100644
--- a/refs.c
+++ b/refs.c
@@ -2484,16 +2484,9 @@ static int repack_without_ref(const char *refname)
 
 static int delete_ref_loose(struct ref_lock *lock, int flag)
 {
-	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
-		/* loose */
-		int err, i = strlen(lock->lk->filename) - 5; /* .lock */
-
-		lock->lk->filename[i] = 0;
-		err = unlink_or_warn(lock->lk->filename);
-		lock->lk->filename[i] = '.';
-		if (err && errno != ENOENT)
-			return 1;
-	}
+	lock->delete_ref = 1;
+	lock->delete_flag = flag;
+
 	return 0;
 }
 
@@ -2515,7 +2508,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 
 	unlink_or_warn(git_path("logs/%s", lock->ref_name));
 	clear_loose_ref_cache(&ref_cache);
-	unlock_ref(lock);
+	ret |= commit_ref_lock(lock);
 	return ret;
 }
 
@@ -2868,6 +2861,20 @@ int write_ref_sha1(struct ref_lock *lock,
 
 int commit_ref_lock(struct ref_lock *lock)
 {
+	if (lock->delete_ref) {
+		int flag = lock->delete_flag;
+
+		if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
+			/* loose */
+			int err, i = strlen(lock->lk->filename) - 5; /* .lock */
+
+			lock->lk->filename[i] = 0;
+			err = unlink_or_warn(lock->lk->filename);
+			lock->lk->filename[i] = '.';
+			if (err && errno != ENOENT)
+				return 1;
+		}
+	} else
         if (!lock->skipped_write && commit_ref(lock)) {
 		error("Couldn't set %s", lock->ref_name);
 		unlock_ref(lock);
@@ -3487,6 +3494,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		if (update->lock) {
 			delnames[delnum++] = update->refname;
 			ret |= delete_ref_loose(update->lock, update->type);
+			ret |= commit_ref_lock(update->lock);
+			update->lock = NULL;
 		}
 	}
 
diff --git a/refs.h b/refs.h
index f14a417..223be30 100644
--- a/refs.h
+++ b/refs.h
@@ -9,6 +9,8 @@ struct ref_lock {
 	int lock_fd;
 	int force_write;
 	int skipped_write;
+	int delete_ref;
+	int delete_flag;
 };
 
 struct ref_transaction;
-- 
1.9.1.505.g4f1e74f

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 3/3] refs.c: change ref_transaction_commit to run the commit loops once all work is finished
  2014-04-11 20:39 [PATCH v3 0/3] Make update refs more atomic Ronnie Sahlberg
  2014-04-11 20:39 ` [PATCH v3 1/3] refs.c: split writing and commiting a ref into two separate functions Ronnie Sahlberg
  2014-04-11 20:39 ` [PATCH v3 2/3] refs.c: split delete_ref_loose() into a separate flag-for-deletion and commit phase Ronnie Sahlberg
@ 2014-04-11 20:39 ` Ronnie Sahlberg
  2014-04-11 22:27 ` [PATCH v3 0/3] Make update refs more atomic Junio C Hamano
  3 siblings, 0 replies; 6+ messages in thread
From: Ronnie Sahlberg @ 2014-04-11 20:39 UTC (permalink / raw)
  To: git; +Cc: Ronnie Sahlberg

During a transaction commit we will both update and delete refs.
Since both update and delete now use the same pattern

    lock = lock_ref_sha1_basic() (or varient of)
    write_ref_sha1(lock)/delete_ref_loose(lock)
    unlock_ref(lock) | commit_ref_lock(lock)

we can now simplify ref_transaction_commit to have one loop that locks all
involved refs.
A second loop that writes or flags for deletion, but does not commit, all
the refs.
And a final third loop that commits all the refs once all the work and
preparations are complete.

This makes updating/deleting multiple refs more atomic since we will not start
the commit phase until all the preparations have completed successfully.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/refs.c b/refs.c
index ec104f2..293bb43 100644
--- a/refs.c
+++ b/refs.c
@@ -3468,42 +3468,47 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		}
 	}
 
-	/* Perform updates first so live commits remain referenced */
+	/* Prepare all the updates/deletes */
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
-		if (!is_null_sha1(update->new_sha1)) {
+		if (!is_null_sha1(update->new_sha1))
 			ret = update_ref_write(msg,
 					       update->refname,
 					       update->new_sha1,
 					       update->lock, onerr);
-			if (ret)
-				unlock_ref(update->lock);
-			else
-				commit_ref_lock(update->lock);
-			update->lock = NULL;
-			if (ret)
-				goto cleanup;
+		else {
+			delnames[delnum++] = update->refname;
+			ret = delete_ref_loose(update->lock, update->type);
 		}
+		if (ret)
+			goto cleanup;
 	}
 
-	/* Perform deletes now that updates are safely completed */
+	ret |= repack_without_refs(delnames, delnum);
+	for (i = 0; i < delnum; i++)
+		unlink_or_warn(git_path("logs/%s", delnames[i]));
+	clear_loose_ref_cache(&ref_cache);
+
+	/* Perform updates first so live commits remain referenced */
+	for (i = 0; i < n; i++) {
+		struct ref_update *update = updates[i];
+
+		if (update->lock && !update->lock->delete_ref) {
+			ret |= commit_ref_lock(update->lock);
+			update->lock = NULL;
+		}
+	}
+	/* And finally perform all deletes */
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
 		if (update->lock) {
-			delnames[delnum++] = update->refname;
-			ret |= delete_ref_loose(update->lock, update->type);
 			ret |= commit_ref_lock(update->lock);
 			update->lock = NULL;
 		}
 	}
 
-	ret |= repack_without_refs(delnames, delnum);
-	for (i = 0; i < delnum; i++)
-		unlink_or_warn(git_path("logs/%s", delnames[i]));
-	clear_loose_ref_cache(&ref_cache);
-
 cleanup:
 	for (i = 0; i < n; i++)
 		if (updates[i]->lock)
-- 
1.9.1.505.g4f1e74f

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/3] refs.c: split delete_ref_loose() into a separate flag-for-deletion and commit phase
  2014-04-11 20:39 ` [PATCH v3 2/3] refs.c: split delete_ref_loose() into a separate flag-for-deletion and commit phase Ronnie Sahlberg
@ 2014-04-11 21:51   ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2014-04-11 21:51 UTC (permalink / raw)
  To: Ronnie Sahlberg; +Cc: git

Ronnie Sahlberg <sahlberg@google.com> writes:

>  int commit_ref_lock(struct ref_lock *lock)
>  {
> +	if (lock->delete_ref) {
> +		int flag = lock->delete_flag;
> +
> +		if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
> +			/* loose */
> +			int err, i = strlen(lock->lk->filename) - 5; /* .lock */
> +
> +			lock->lk->filename[i] = 0;
> +			err = unlink_or_warn(lock->lk->filename);
> +			lock->lk->filename[i] = '.';
> +			if (err && errno != ENOENT)
> +				return 1;
> +		}
> +	} else
>          if (!lock->skipped_write && commit_ref(lock)) {

	} else if (...) {

Also the previous patch indents the above "if" line in the context
with spaces; please use a tab.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 0/3] Make update refs more atomic
  2014-04-11 20:39 [PATCH v3 0/3] Make update refs more atomic Ronnie Sahlberg
                   ` (2 preceding siblings ...)
  2014-04-11 20:39 ` [PATCH v3 3/3] refs.c: change ref_transaction_commit to run the commit loops once all work is finished Ronnie Sahlberg
@ 2014-04-11 22:27 ` Junio C Hamano
  3 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2014-04-11 22:27 UTC (permalink / raw)
  To: Ronnie Sahlberg; +Cc: git

Ronnie Sahlberg <sahlberg@google.com> writes:

> refs.c:ref_transaction_commit() intermingles doing updates and checks with
> actually applying changes to the refs in loops that abort on error.
> This is done one ref at a time and means that if an error is detected that
> will fail the operation partway through the list of refs to update we
> will end up with some changes applied to disk and others not.
>
> Without having transaction support from the filesystem, it is hard to
> make an update that involves multiple refs to guarantee atomicity, but we
> can do a somewhat better than we currently do.
>
> These patches change the update and delete functions to use a three
> call pattern of
>
> 1, lock
> 2, update, or flag for deletion
> 3, apply on disk  (rename() or unlink())
>
> When a transaction is commited we first do all the locking, preparations
> and most of the error checking before we actually start applying any changes
> to the filesystem store.
>
> This means that more of the error cases that will fail the commit
> will trigger before we start doing any changes to the actual files.
>
>
> This should make the changes of refs in refs_transaction_commit slightly
> more atomic.

Hmph.  At least 5801 9350 and 9300 seem to fail with these three
queued on top of mh/ref-transaction series.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2014-04-11 22:27 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-11 20:39 [PATCH v3 0/3] Make update refs more atomic Ronnie Sahlberg
2014-04-11 20:39 ` [PATCH v3 1/3] refs.c: split writing and commiting a ref into two separate functions Ronnie Sahlberg
2014-04-11 20:39 ` [PATCH v3 2/3] refs.c: split delete_ref_loose() into a separate flag-for-deletion and commit phase Ronnie Sahlberg
2014-04-11 21:51   ` Junio C Hamano
2014-04-11 20:39 ` [PATCH v3 3/3] refs.c: change ref_transaction_commit to run the commit loops once all work is finished Ronnie Sahlberg
2014-04-11 22:27 ` [PATCH v3 0/3] Make update refs more atomic Junio C Hamano

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).