[PATCH v10 00/44] Use ref transactions for all ref updates

[PATCH v10 00/44] Use ref transactions for all ref updates

[Ronnie Sahlberg]

This patch series can also be found at
https://github.com/rsahlberg/git/tree/ref-transactions


This patch series is based on next and expands on the transaction API. It
converts all ref updates, inside refs.c as well as external, to use the
transaction API for updates. This makes most of the ref updates to become
atomic when there are failures locking or writing to a ref.

This version completes the work to convert all ref updates to use transactions.
Now that all updates are through transactions I will start working on
cleaning up the reading of refs and to create an api for managing reflogs but
all that will go in a different patch series.

Version 10:
 - Add err argument to _update/_delete/_create
 - Remove the ref_transaction_rollback function
 - Other changes based on JN's reviews.

Version 9:
 - Lots of updates to error messages based on JN's review.

Version 8:
 - Updates after review by JN
 - Improve commit messages
 - Add a patch that adds an err argument to repack_without_refs
 - Add a patch that adds an err argument to delete_loose_ref
 - Better document that a _update/_delete/_create failure means the whole
   transaction has failed and needs rollback.

Version 7:
 - Updated commit messages per JNs review comments.
 - Changed REF_ISPRUNING and REF_ISPACKONLY to be private flags and not
   exposed through refs.h

Version 6:
 - Convert all updates in refs.c to use transactions too.

Version 5:
 - Reword commit messages for having _create/_delete/_update returning
   success/failure. There are no conditions yet that return an error from
   these failures but there will be in the future. So we still check the
   return from these functions in the callers in preparation for this.
 - Don't leak memory by just passing a strbuf_detach() pointer to functions.
   Use <obj>.buf and explicitely strbuf_release the data afterwards.
 - Remove the function update_ref_lock.
 - Remove the function update_ref_write.
 - Track transaction status and die(BUG:) if we call _create/_delete/_update/
   _commit for a transaction that is not OPEN.

Version 4:
 - Rename patch series from "Use ref transactions from most callers" to
   "Use ref transactions for all ref updates".
 - Convert all external ref writes to use transactions and make write_ref_sha1
   and lock_ref_sha1 static functions.
 - Change the ref commit and free handling so we no longer pass pointer to
   pointer to _commit. _commit no longer frees the transaction. The caller
   MUST call _free itself.
 - Change _commit to take a strbuf pointer instead of a char* for error
   reporting back to the caller.
 - Re-add the walker patch after fixing it.

Version 3:
 - Remove the walker patch for now. Walker needs more complex solution
   so defer it until the basics are done.
 - Remove the onerr argument to ref_transaction_commit(). All callers
   that need to die() on error now have to do this explicitely.
 - Pass an error string from ref_transaction_commit() back to the callers
   so that they can craft a nice error message upon failures.
 - Make ref_transaction_rollback() accept NULL as argument.
 - Change ref_transaction_commit() to take a pointer to pointer argument for
   the transaction and have it clear the callers pointer to NULL when
   invoked. This allows for much nicer handling of transaction rollback on
   failure.

Version 2:
 - Add a patch to ref_transaction_commit to make it honor onerr even if the
   error triggered in ref_Transaction_commit itself rather than in a call
   to other functions (that already honor onerr).
 - Add a patch to make the update_ref() helper function use transactions
   internally.
 - Change ref_transaction_update to die() instead of error() if we pass
   if a NULL old_sha1 but have have_old == true.
 - Change ref_transaction_create to die() instead of error() if new_sha1
   is false but we pass it a null_sha1.
 - Change ref_transaction_delete die() instead of error() if we pass
   if a NULL old_sha1 but have have_old == true.
 - Change several places to do  if(!transaction || ref_transaction_update()
   || ref_Transaction_commit()) die(generic-message) instead of checking each
   step separately and having a different message for each failure.
   Most users are likely not interested in what step of the transaction
   failed and only whether it failed or not.
 - Change commit.c to only pass a pointer to ref_transaction_update
   iff current_head is non-NULL.
   The previous patch used to compute a garbage pointer for
   current_head->object.sha1 and relied on the fact that ref_transaction_update
   would not try to dereference this pointer if !!current_head was 0.
 - Updated commit message for the walker_fetch change to try to justify why
   the change in locking semantics should not be harmful.



Ronnie Sahlberg (44):
  refs.c: constify the sha arguments for
    ref_transaction_create|delete|update
  refs.c: allow passing NULL to ref_transaction_free
  refs.c: add a strbuf argument to ref_transaction_commit for error
    logging
  refs.c: add an err argument to repack_without_refs
  refs.c: make ref_update_reject_duplicates take a strbuf argument for
    errors
  refs.c: add an err argument to delete_ref_loose
  refs.c: make update_ref_write update a strbuf on failure
  update-ref.c: log transaction error from the update_ref
  refs.c: remove the onerr argument to ref_transaction_commit
  refs.c: change ref_transaction_update() to do error checking and
    return status
  refs.c: change ref_transaction_create to do error checking and return
    status
  refs.c: ref_transaction_delete to check for error and return status
  tag.c: use ref transactions when doing updates
  replace.c: use the ref transaction functions for updates
  commit.c: use ref transactions for updates
  sequencer.c: use ref transactions for all ref updates
  fast-import.c: change update_branch to use ref transactions
  branch.c: use ref transaction for all ref updates
  refs.c: change update_ref to use a transaction
  refs.c: free the transaction before returning when number of updates
    is 0
  refs.c: ref_transaction_commit should not free the transaction
  fetch.c: clear errno before calling functions that might set it
  fetch.c: change s_update_ref to use a ref transaction
  fetch.c: use a single ref transaction for all ref updates
  receive-pack.c: use a reference transaction for updating the refs
  fast-import.c: use a ref transaction when dumping tags
  walker.c: use ref transaction for ref updates
  refs.c: make write_ref_sha1 static
  refs.c: make lock_ref_sha1 static
  refs.c: add transaction.status and track OPEN/CLOSED/ERROR
  refs.c: remove the update_ref_lock function
  refs.c: remove the update_ref_write function
  refs.c: remove lock_ref_sha1
  refs.c: make prune_ref use a transaction to delete the ref
  refs.c: make delete_ref use a transaction
  refs.c: pass the ref log message to _create/delete/update instead of
    _commit
  refs.c: pass NULL as *flags to read_ref_full
  refs.c: pack all refs before we start to rename a ref
  refs.c: move the check for valid refname to lock_ref_sha1_basic
  refs.c: call lock_ref_sha1_basic directly from commit
  refs.c: add a new flag for transaction delete for refs we know are
    packed only
  refs.c: pass a skip list to name_conflict_fn
  refs.c: make rename_ref use a transaction
  refs.c: remove forward declaration of write_ref_sha1

 branch.c               |  30 ++--
 builtin/commit.c       |  24 ++-
 builtin/fetch.c        |  29 +--
 builtin/receive-pack.c |  21 +--
 builtin/replace.c      |  15 +-
 builtin/tag.c          |  15 +-
 builtin/update-ref.c   |  32 ++--
 cache.h                |   2 +
 fast-import.c          |  42 +++--
 lockfile.c             |  21 ++-
 refs.c                 | 468 +++++++++++++++++++++++++++++--------------------
 refs.h                 |  54 +++---
 sequencer.c            |  24 ++-
 t/t3200-branch.sh      |   2 +-
 walker.c               |  51 +++---
 15 files changed, 491 insertions(+), 339 deletions(-)

-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 01/44] refs.c: constify the sha arguments for ref_transaction_create|delete|update

[Ronnie Sahlberg]

ref_transaction_create|delete|update has no need to modify the sha1
arguments passed to it so it should use const unsigned char* instead
of unsigned char*.

Some functions, such as fast_forward_to(), already have its old/new
sha1 arguments as consts. This function will at some point need to
use ref_transaction_update() in which case this change is required.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 7 ++++---
 refs.h | 7 ++++---
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 48573e3..3b7e604 100644
--- a/refs.c
+++ b/refs.c
@@ -3333,7 +3333,8 @@ static struct ref_update *add_update(struct ref_transaction *transaction,
 
 void ref_transaction_update(struct ref_transaction *transaction,
 			    const char *refname,
-			    unsigned char *new_sha1, unsigned char *old_sha1,
+			    const unsigned char *new_sha1,
+			    const unsigned char *old_sha1,
 			    int flags, int have_old)
 {
 	struct ref_update *update = add_update(transaction, refname);
@@ -3347,7 +3348,7 @@ void ref_transaction_update(struct ref_transaction *transaction,
 
 void ref_transaction_create(struct ref_transaction *transaction,
 			    const char *refname,
-			    unsigned char *new_sha1,
+			    const unsigned char *new_sha1,
 			    int flags)
 {
 	struct ref_update *update = add_update(transaction, refname);
@@ -3361,7 +3362,7 @@ void ref_transaction_create(struct ref_transaction *transaction,
 
 void ref_transaction_delete(struct ref_transaction *transaction,
 			    const char *refname,
-			    unsigned char *old_sha1,
+			    const unsigned char *old_sha1,
 			    int flags, int have_old)
 {
 	struct ref_update *update = add_update(transaction, refname);
diff --git a/refs.h b/refs.h
index a07a5d0..50ca06a 100644
--- a/refs.h
+++ b/refs.h
@@ -238,7 +238,8 @@ struct ref_transaction *ref_transaction_begin(void);
  */
 void ref_transaction_update(struct ref_transaction *transaction,
 			    const char *refname,
-			    unsigned char *new_sha1, unsigned char *old_sha1,
+			    const unsigned char *new_sha1,
+			    const unsigned char *old_sha1,
 			    int flags, int have_old);
 
 /*
@@ -249,7 +250,7 @@ void ref_transaction_update(struct ref_transaction *transaction,
  */
 void ref_transaction_create(struct ref_transaction *transaction,
 			    const char *refname,
-			    unsigned char *new_sha1,
+			    const unsigned char *new_sha1,
 			    int flags);
 
 /*
@@ -259,7 +260,7 @@ void ref_transaction_create(struct ref_transaction *transaction,
  */
 void ref_transaction_delete(struct ref_transaction *transaction,
 			    const char *refname,
-			    unsigned char *old_sha1,
+			    const unsigned char *old_sha1,
 			    int flags, int have_old);
 
 /*
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 02/44] refs.c: allow passing NULL to ref_transaction_free

[Ronnie Sahlberg]

Allow ref_transaction_free(NULL) and hence ref_transaction_rollback(NULL)
as no-ops. This makes ref_transaction_rollback easier to use and more similar
to plain 'free'.

In particular, it lets us rollback unconditionally as part of cleanup code
after setting 'transaction = NULL' if a transaction has been committed or
rolled back already.

This allows us to write code like
  if ( (!transaction ||
        ref_transaction_update(...))  ||
      (ref_transaction_commit(...) && !(transaction = NULL)) {
          ref_transaction_rollback(transaction);
          ...
  }

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/refs.c b/refs.c
index 3b7e604..6357089 100644
--- a/refs.c
+++ b/refs.c
@@ -3312,6 +3312,9 @@ void ref_transaction_free(struct ref_transaction *transaction)
 {
 	int i;
 
+	if (!transaction)
+		return;
+
 	for (i = 0; i < transaction->nr; i++)
 		free(transaction->updates[i]);
 
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 03/44] refs.c: add a strbuf argument to ref_transaction_commit for error logging

[Ronnie Sahlberg]

Add a strbuf argument to _commit so that we can pass an error string back to
the caller. So that we can do error logging from the caller instead of from
_commit.

Longer term plan is to first convert all callers to use onerr==QUIET_ON_ERR
and craft any log messages from the callers themselves and finally remove the
onerr argument completely.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/update-ref.c | 2 +-
 refs.c               | 6 +++++-
 refs.h               | 5 ++++-
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 405267f..aaa06aa 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -367,7 +367,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		if (end_null)
 			line_termination = '\0';
 		update_refs_stdin();
-		ret = ref_transaction_commit(transaction, msg,
+		ret = ref_transaction_commit(transaction, msg, NULL,
 					     UPDATE_REFS_DIE_ON_ERR);
 		return ret;
 	}
diff --git a/refs.c b/refs.c
index 6357089..686b802 100644
--- a/refs.c
+++ b/refs.c
@@ -3418,7 +3418,8 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 }
 
 int ref_transaction_commit(struct ref_transaction *transaction,
-			   const char *msg, enum action_on_err onerr)
+			   const char *msg, struct strbuf *err,
+			   enum action_on_err onerr)
 {
 	int ret = 0, delnum = 0, i;
 	const char **delnames;
@@ -3447,6 +3448,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 					       update->flags,
 					       &update->type, onerr);
 		if (!update->lock) {
+			if (err)
+				strbuf_addf(err, "Cannot lock the ref '%s'.",
+					    update->refname);
 			ret = 1;
 			goto cleanup;
 		}
diff --git a/refs.h b/refs.h
index 50ca06a..8669fc9 100644
--- a/refs.h
+++ b/refs.h
@@ -267,9 +267,12 @@ void ref_transaction_delete(struct ref_transaction *transaction,
  * Commit all of the changes that have been queued in transaction, as
  * atomically as possible.  Return a nonzero value if there is a
  * problem.  The ref_transaction is freed by this function.
+ * If err is non-NULL we will add an error string to it to explain why
+ * the transaction failed. The string does not end in newline.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
-			   const char *msg, enum action_on_err onerr);
+			   const char *msg, struct strbuf *err,
+			   enum action_on_err onerr);
 
 /*
  * Free an existing transaction and all associated data.
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 04/44] refs.c: add an err argument to repack_without_refs

[Ronnie Sahlberg]

Update repack_without_refs to take an err argument and update it if there
is a failure. Pass the err variable from ref_transaction_commit to this
function so that callers can print a meaningful error message if _commit
fails due to a problem in repack_without_refs.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 cache.h    |  2 ++
 lockfile.c | 21 ++++++++++++---------
 refs.c     | 25 +++++++++++++++++++------
 3 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/cache.h b/cache.h
index 8c6cdc2..48548d6 100644
--- a/cache.h
+++ b/cache.h
@@ -559,6 +559,8 @@ struct lock_file {
 #define LOCK_DIE_ON_ERROR 1
 #define LOCK_NODEREF 2
 extern int unable_to_lock_error(const char *path, int err);
+extern void unable_to_lock_strbuf(const char *path, int err,
+				  struct strbuf *buf);
 extern NORETURN void unable_to_lock_index_die(const char *path, int err);
 extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
 extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
diff --git a/lockfile.c b/lockfile.c
index 8fbcb6a..9e04b43 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -157,33 +157,36 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	return lk->fd;
 }
 
-static char *unable_to_lock_message(const char *path, int err)
+void unable_to_lock_strbuf(const char *path, int err, struct strbuf *buf)
 {
-	struct strbuf buf = STRBUF_INIT;
 
 	if (err == EEXIST) {
-		strbuf_addf(&buf, "Unable to create '%s.lock': %s.\n\n"
+		strbuf_addf(buf, "Unable to create '%s.lock': %s.\n\n"
 		    "If no other git process is currently running, this probably means a\n"
 		    "git process crashed in this repository earlier. Make sure no other git\n"
 		    "process is running and remove the file manually to continue.",
 			    absolute_path(path), strerror(err));
 	} else
-		strbuf_addf(&buf, "Unable to create '%s.lock': %s",
+		strbuf_addf(buf, "Unable to create '%s.lock': %s",
 			    absolute_path(path), strerror(err));
-	return strbuf_detach(&buf, NULL);
 }
 
 int unable_to_lock_error(const char *path, int err)
 {
-	char *msg = unable_to_lock_message(path, err);
-	error("%s", msg);
-	free(msg);
+	struct strbuf buf = STRBUF_INIT;
+
+	unable_to_lock_strbuf(path, err, &buf);
+	error("%s", buf.buf);
+	strbuf_release(&buf);
 	return -1;
 }
 
 NORETURN void unable_to_lock_index_die(const char *path, int err)
 {
-	die("%s", unable_to_lock_message(path, err));
+	struct strbuf buf = STRBUF_INIT;
+
+	unable_to_lock_strbuf(path, err, &buf);
+	die("%s", buf.buf);
 }
 
 int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
diff --git a/refs.c b/refs.c
index 686b802..a470e51 100644
--- a/refs.c
+++ b/refs.c
@@ -2208,6 +2208,7 @@ int commit_packed_refs(void)
 	struct packed_ref_cache *packed_ref_cache =
 		get_packed_ref_cache(&ref_cache);
 	int error = 0;
+	int save_errno;
 
 	if (!packed_ref_cache->lock)
 		die("internal error: packed-refs not locked");
@@ -2217,10 +2218,13 @@ int commit_packed_refs(void)
 	do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
 				 0, write_packed_entry_fn,
 				 &packed_ref_cache->lock->fd);
-	if (commit_lock_file(packed_ref_cache->lock))
+	if (commit_lock_file(packed_ref_cache->lock)) {
+		save_errno = errno;
 		error = -1;
+	}
 	packed_ref_cache->lock = NULL;
 	release_packed_ref_cache(packed_ref_cache);
+	errno = save_errno;
 	return error;
 }
 
@@ -2427,12 +2431,12 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
 	return 0;
 }
 
-static int repack_without_refs(const char **refnames, int n)
+static int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 {
 	struct ref_dir *packed;
 	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
 	struct string_list_item *ref_to_delete;
-	int i, removed = 0;
+	int i, ret, removed = 0;
 
 	/* Look for a packed ref */
 	for (i = 0; i < n; i++)
@@ -2444,6 +2448,11 @@ static int repack_without_refs(const char **refnames, int n)
 		return 0; /* no refname exists in packed refs */
 
 	if (lock_packed_refs(0)) {
+		if (err) {
+			unable_to_lock_strbuf(git_path("packed-refs"), errno,
+					      err);
+			return 1;
+		}
 		unable_to_lock_error(git_path("packed-refs"), errno);
 		return error("cannot delete '%s' from packed refs", refnames[i]);
 	}
@@ -2470,12 +2479,16 @@ static int repack_without_refs(const char **refnames, int n)
 	}
 
 	/* Write what remains */
-	return commit_packed_refs();
+	ret = commit_packed_refs();
+	if (ret && err)
+		strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
+			    strerror(errno));
+	return ret;
 }
 
 static int repack_without_ref(const char *refname)
 {
-	return repack_without_refs(&refname, 1);
+	return repack_without_refs(&refname, 1, NULL);
 }
 
 static int delete_ref_loose(struct ref_lock *lock, int flag)
@@ -3481,7 +3494,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		}
 	}
 
-	ret |= repack_without_refs(delnames, delnum);
+	ret |= repack_without_refs(delnames, delnum, err);
 	for (i = 0; i < delnum; i++)
 		unlink_or_warn(git_path("logs/%s", delnames[i]));
 	clear_loose_ref_cache(&ref_cache);
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 05/44] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors

[Ronnie Sahlberg]

Make ref_update_reject_duplicates return any error that occurs through a
new strbuf argument. This means that when a transaction commit fails in
this function we will now be able to pass a helpful error message back to the
caller.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index a470e51..57ec72a 100644
--- a/refs.c
+++ b/refs.c
@@ -3410,6 +3410,7 @@ static int ref_update_compare(const void *r1, const void *r2)
 }
 
 static int ref_update_reject_duplicates(struct ref_update **updates, int n,
+					struct strbuf *err,
 					enum action_on_err onerr)
 {
 	int i;
@@ -3417,6 +3418,9 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 		if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {
 			const char *str =
 				"Multiple updates for ref '%s' not allowed.";
+			if (err)
+				strbuf_addf(err, str, updates[i]->refname);
+
 			switch (onerr) {
 			case UPDATE_REFS_MSG_ON_ERR:
 				error(str, updates[i]->refname); break;
@@ -3447,7 +3451,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 
 	/* Copy, sort, and reject duplicate refs */
 	qsort(updates, n, sizeof(*updates), ref_update_compare);
-	ret = ref_update_reject_duplicates(updates, n, onerr);
+	ret = ref_update_reject_duplicates(updates, n, err, onerr);
 	if (ret)
 		goto cleanup;
 
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 06/44] refs.c: add an err argument to delete_ref_loose

[Ronnie Sahlberg]

Add an err argument to delete_loose_ref so that we can pass a descriptive
error string back to the caller. Pass the err argument from transaction
commit to this function so that transaction users will have a nice error
string if the transaction failed due to delete_loose_ref.

Add a new function unlink_or_err that we can call from delete_ref_loose. This
function is similar to unlink_or_warn except that we can pass it an err
argument. If err is non-NULL the function will populate err instead of
printing a warning().

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

diff --git a/refs.c b/refs.c
index 57ec72a..bc444c7 100644
--- a/refs.c
+++ b/refs.c
@@ -2491,17 +2491,43 @@ static int repack_without_ref(const char *refname)
 	return repack_without_refs(&refname, 1, NULL);
 }
 
-static int delete_ref_loose(struct ref_lock *lock, int flag)
+static int add_err_if_unremovable(const char *op, const char *file,
+				  struct strbuf *e, int rc)
+{
+	int err = errno;
+	if (rc < 0 && err != ENOENT) {
+		strbuf_addf(e, "unable to %s %s: %s",
+			    op, file, strerror(errno));
+		errno = err;
+	}
+	return rc;
+}
+
+static int unlink_or_err(const char *file, struct strbuf *err)
+{
+	if (err)
+		return add_err_if_unremovable("unlink", file, err,
+					      unlink(file));
+	else
+		return unlink_or_warn(file);
+}
+
+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
 {
 	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
 		/* loose */
-		int err, i = strlen(lock->lk->filename) - 5; /* .lock */
+		int res, i = strlen(lock->lk->filename) - 5; /* .lock */
 
 		lock->lk->filename[i] = 0;
-		err = unlink_or_warn(lock->lk->filename);
+		res = unlink_or_err(lock->lk->filename, err);
 		lock->lk->filename[i] = '.';
-		if (err && errno != ENOENT)
+		if (res && errno != ENOENT) {
+			if (err)
+				strbuf_addf(err,
+					    "failed to delete loose ref '%s'",
+					    lock->lk->filename);
 			return 1;
+		}
 	}
 	return 0;
 }
@@ -2514,7 +2540,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 	lock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);
 	if (!lock)
 		return 1;
-	ret |= delete_ref_loose(lock, flag);
+	ret |= delete_ref_loose(lock, flag, NULL);
 
 	/* removing the loose one could have resurrected an earlier
 	 * packed one.  Also, if it was not loose we need to repack
@@ -3494,7 +3520,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 
 		if (update->lock) {
 			delnames[delnum++] = update->lock->ref_name;
-			ret |= delete_ref_loose(update->lock, update->type);
+			ret |= delete_ref_loose(update->lock, update->type,
+						err);
 		}
 	}
 
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 07/44] refs.c: make update_ref_write update a strbuf on failure

[Ronnie Sahlberg]

Change update_ref_write to also update an error strbuf on failure.
This makes the error available to ref_transaction_commit callers if the
transaction failed due to update_ref_sha1/write_ref_sha1 failures.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index bc444c7..582c74b 100644
--- a/refs.c
+++ b/refs.c
@@ -3301,10 +3301,13 @@ static struct ref_lock *update_ref_lock(const char *refname,
 
 static int update_ref_write(const char *action, const char *refname,
 			    const unsigned char *sha1, struct ref_lock *lock,
-			    enum action_on_err onerr)
+			    struct strbuf *err, enum action_on_err onerr)
 {
 	if (write_ref_sha1(lock, sha1, action) < 0) {
 		const char *str = "Cannot update the ref '%s'.";
+		if (err)
+			strbuf_addf(err, str, refname);
+
 		switch (onerr) {
 		case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
 		case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
@@ -3425,7 +3428,7 @@ int update_ref(const char *action, const char *refname,
 	lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
 	if (!lock)
 		return 1;
-	return update_ref_write(action, refname, sha1, lock, onerr);
+	return update_ref_write(action, refname, sha1, lock, NULL, onerr);
 }
 
 static int ref_update_compare(const void *r1, const void *r2)
@@ -3507,7 +3510,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			ret = update_ref_write(msg,
 					       update->refname,
 					       update->new_sha1,
-					       update->lock, onerr);
+					       update->lock, err, onerr);
 			update->lock = NULL; /* freed by update_ref_write */
 			if (ret)
 				goto cleanup;
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 08/44] update-ref.c: log transaction error from the update_ref

[Ronnie Sahlberg]

Call ref_transaction_commit with QUIET_ON_ERR and use the strbuf that is
returned to print a log message if/after the transaction fails.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/update-ref.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index aaa06aa..207e24d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 	const char *refname, *oldval, *msg = NULL;
 	unsigned char sha1[20], oldsha1[20];
 	int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
+	struct strbuf err = STRBUF_INIT;
 	struct option options[] = {
 		OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the update")),
 		OPT_BOOL('d', NULL, &delete, N_("delete the reference")),
@@ -359,17 +360,16 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		die("Refusing to perform update with empty message.");
 
 	if (read_stdin) {
-		int ret;
 		transaction = ref_transaction_begin();
-
 		if (delete || no_deref || argc > 0)
 			usage_with_options(git_update_ref_usage, options);
 		if (end_null)
 			line_termination = '\0';
 		update_refs_stdin();
-		ret = ref_transaction_commit(transaction, msg, NULL,
-					     UPDATE_REFS_DIE_ON_ERR);
-		return ret;
+		if (ref_transaction_commit(transaction, msg, &err,
+					   UPDATE_REFS_QUIET_ON_ERR))
+			die("%s", err.buf);
+		return 0;
 	}
 
 	if (end_null)
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 09/44] refs.c: remove the onerr argument to ref_transaction_commit

[Ronnie Sahlberg]

Since all callers now use QUIET_ON_ERR we no longer need to provide an onerr
argument any more. Remove the onerr argument from the ref_transaction_commit
signature.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/update-ref.c |  3 +--
 refs.c               | 22 +++++++---------------
 refs.h               |  3 +--
 3 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 207e24d..2bef2a0 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -366,8 +366,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		if (end_null)
 			line_termination = '\0';
 		update_refs_stdin();
-		if (ref_transaction_commit(transaction, msg, &err,
-					   UPDATE_REFS_QUIET_ON_ERR))
+		if (ref_transaction_commit(transaction, msg, &err))
 			die("%s", err.buf);
 		return 0;
 	}
diff --git a/refs.c b/refs.c
index 582c74b..29cfe78 100644
--- a/refs.c
+++ b/refs.c
@@ -3439,8 +3439,7 @@ static int ref_update_compare(const void *r1, const void *r2)
 }
 
 static int ref_update_reject_duplicates(struct ref_update **updates, int n,
-					struct strbuf *err,
-					enum action_on_err onerr)
+					struct strbuf *err)
 {
 	int i;
 	for (i = 1; i < n; i++)
@@ -3450,22 +3449,13 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 			if (err)
 				strbuf_addf(err, str, updates[i]->refname);
 
-			switch (onerr) {
-			case UPDATE_REFS_MSG_ON_ERR:
-				error(str, updates[i]->refname); break;
-			case UPDATE_REFS_DIE_ON_ERR:
-				die(str, updates[i]->refname); break;
-			case UPDATE_REFS_QUIET_ON_ERR:
-				break;
-			}
 			return 1;
 		}
 	return 0;
 }
 
 int ref_transaction_commit(struct ref_transaction *transaction,
-			   const char *msg, struct strbuf *err,
-			   enum action_on_err onerr)
+			   const char *msg, struct strbuf *err)
 {
 	int ret = 0, delnum = 0, i;
 	const char **delnames;
@@ -3480,7 +3470,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 
 	/* Copy, sort, and reject duplicate refs */
 	qsort(updates, n, sizeof(*updates), ref_update_compare);
-	ret = ref_update_reject_duplicates(updates, n, err, onerr);
+	ret = ref_update_reject_duplicates(updates, n, err);
 	if (ret)
 		goto cleanup;
 
@@ -3492,7 +3482,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 					       (update->have_old ?
 						update->old_sha1 : NULL),
 					       update->flags,
-					       &update->type, onerr);
+					       &update->type,
+					       UPDATE_REFS_QUIET_ON_ERR);
 		if (!update->lock) {
 			if (err)
 				strbuf_addf(err, "Cannot lock the ref '%s'.",
@@ -3510,7 +3501,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			ret = update_ref_write(msg,
 					       update->refname,
 					       update->new_sha1,
-					       update->lock, err, onerr);
+					       update->lock, err,
+					       UPDATE_REFS_QUIET_ON_ERR);
 			update->lock = NULL; /* freed by update_ref_write */
 			if (ret)
 				goto cleanup;
diff --git a/refs.h b/refs.h
index 8669fc9..05e65fa 100644
--- a/refs.h
+++ b/refs.h
@@ -271,8 +271,7 @@ void ref_transaction_delete(struct ref_transaction *transaction,
  * the transaction failed. The string does not end in newline.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
-			   const char *msg, struct strbuf *err,
-			   enum action_on_err onerr);
+			   const char *msg, struct strbuf *err);
 
 /*
  * Free an existing transaction and all associated data.
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 10/44] refs.c: change ref_transaction_update() to do error checking and return status

[Ronnie Sahlberg]

Update ref_transaction_update() do some basic error checking and return
non-zero on error. Update all callers to check ref_transaction_update() for
error. There are currently no conditions in _update that will return error but
there will be in the future. Add an err argument that will be updated on
failure.

Also check for BUGs during update and die(BUG:...) if we are calling
_update with have_old but the old_sha1 pointer is NULL.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/update-ref.c | 12 +++++++-----
 refs.c               | 18 ++++++++++++------
 refs.h               | 14 +++++++++-----
 3 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 2bef2a0..9f328b2 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -16,6 +16,7 @@ static struct ref_transaction *transaction;
 
 static char line_termination = '\n';
 static int update_flags;
+static struct strbuf err = STRBUF_INIT;
 
 /*
  * Parse one whitespace- or NUL-terminated, possibly C-quoted argument
@@ -197,8 +198,9 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)
 	if (*next != line_termination)
 		die("update %s: extra input: %s", refname, next);
 
-	ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-			       update_flags, have_old);
+	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+				   update_flags, have_old, &err))
+		die("update %s: failed: %s", refname, err.buf);
 
 	update_flags = 0;
 	free(refname);
@@ -286,8 +288,9 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)
 	if (*next != line_termination)
 		die("verify %s: extra input: %s", refname, next);
 
-	ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-			       update_flags, have_old);
+	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+				   update_flags, have_old, &err))
+		die("verify %s: %s", refname, err.buf);
 
 	update_flags = 0;
 	free(refname);
@@ -342,7 +345,6 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 	const char *refname, *oldval, *msg = NULL;
 	unsigned char sha1[20], oldsha1[20];
 	int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
-	struct strbuf err = STRBUF_INIT;
 	struct option options[] = {
 		OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the update")),
 		OPT_BOOL('d', NULL, &delete, N_("delete the reference")),
diff --git a/refs.c b/refs.c
index 29cfe78..a4fdfc7 100644
--- a/refs.c
+++ b/refs.c
@@ -3376,19 +3376,25 @@ static struct ref_update *add_update(struct ref_transaction *transaction,
 	return update;
 }
 
-void ref_transaction_update(struct ref_transaction *transaction,
-			    const char *refname,
-			    const unsigned char *new_sha1,
-			    const unsigned char *old_sha1,
-			    int flags, int have_old)
+int ref_transaction_update(struct ref_transaction *transaction,
+			   const char *refname,
+			   const unsigned char *new_sha1,
+			   const unsigned char *old_sha1,
+			   int flags, int have_old,
+			   struct strbuf *err)
 {
-	struct ref_update *update = add_update(transaction, refname);
+	struct ref_update *update;
+
+	if (have_old && !old_sha1)
+		die("BUG: have_old is true but old_sha1 is NULL");
 
+	update = add_update(transaction, refname);
 	hashcpy(update->new_sha1, new_sha1);
 	update->flags = flags;
 	update->have_old = have_old;
 	if (have_old)
 		hashcpy(update->old_sha1, old_sha1);
+	return 0;
 }
 
 void ref_transaction_create(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 05e65fa..454871a 100644
--- a/refs.h
+++ b/refs.h
@@ -235,12 +235,16 @@ struct ref_transaction *ref_transaction_begin(void);
  * be deleted.  If have_old is true, then old_sha1 holds the value
  * that the reference should have had before the update, or zeros if
  * it must not have existed beforehand.
+ * Function returns 0 on success and non-zero on failure. A failure to update
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back. On failure the err buffer will be updated.
  */
-void ref_transaction_update(struct ref_transaction *transaction,
-			    const char *refname,
-			    const unsigned char *new_sha1,
-			    const unsigned char *old_sha1,
-			    int flags, int have_old);
+int ref_transaction_update(struct ref_transaction *transaction,
+			   const char *refname,
+			   const unsigned char *new_sha1,
+			   const unsigned char *old_sha1,
+			   int flags, int have_old,
+			   struct strbuf *err);
 
 /*
  * Add a reference creation to transaction.  new_sha1 is the value
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 11/44] refs.c: change ref_transaction_create to do error checking and return status

[Ronnie Sahlberg]

Do basic error checking in ref_transaction_create() and make it return
non-zero on error. Update all callers to check the result of
ref_transaction_create(). There are currently no conditions in _create that
will return error but there will be in the future. Add an err argument that
will be updated on failure.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/update-ref.c |  4 +++-
 refs.c               | 18 ++++++++++++------
 refs.h               | 12 ++++++++----
 3 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 9f328b2..e9c216e 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -226,7 +226,9 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)
 	if (*next != line_termination)
 		die("create %s: extra input: %s", refname, next);
 
-	ref_transaction_create(transaction, refname, new_sha1, update_flags);
+	if (ref_transaction_create(transaction, refname, new_sha1,
+				   update_flags, NULL))
+		die("cannot create ref '%s'", refname);
 
 	update_flags = 0;
 	free(refname);
diff --git a/refs.c b/refs.c
index a4fdfc7..06a4fed 100644
--- a/refs.c
+++ b/refs.c
@@ -3397,18 +3397,24 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	return 0;
 }
 
-void ref_transaction_create(struct ref_transaction *transaction,
-			    const char *refname,
-			    const unsigned char *new_sha1,
-			    int flags)
+int ref_transaction_create(struct ref_transaction *transaction,
+			   const char *refname,
+			   const unsigned char *new_sha1,
+			   int flags,
+			   struct strbuf *err)
 {
-	struct ref_update *update = add_update(transaction, refname);
+	struct ref_update *update;
+
+	if (!new_sha1 || is_null_sha1(new_sha1))
+		die("BUG: create ref with null new_sha1");
+
+	update = add_update(transaction, refname);
 
-	assert(!is_null_sha1(new_sha1));
 	hashcpy(update->new_sha1, new_sha1);
 	hashclr(update->old_sha1);
 	update->flags = flags;
 	update->have_old = 1;
+	return 0;
 }
 
 void ref_transaction_delete(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 454871a..bcae348 100644
--- a/refs.h
+++ b/refs.h
@@ -251,11 +251,15 @@ int ref_transaction_update(struct ref_transaction *transaction,
  * that the reference should have after the update; it must not be the
  * null SHA-1.  It is verified that the reference does not exist
  * already.
+ * Function returns 0 on success and non-zero on failure. A failure to create
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back. On failure the err buffer will be updated.
  */
-void ref_transaction_create(struct ref_transaction *transaction,
-			    const char *refname,
-			    const unsigned char *new_sha1,
-			    int flags);
+int ref_transaction_create(struct ref_transaction *transaction,
+			   const char *refname,
+			   const unsigned char *new_sha1,
+			   int flags,
+			   struct strbuf *err);
 
 /*
  * Add a reference deletion to transaction.  If have_old is true, then
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 12/44] refs.c: ref_transaction_delete to check for error and return status

[Ronnie Sahlberg]

Change ref_transaction_delete() to do basic error checking and return
non-zero of error. Update all callers to check the return for
ref_transaction_delete(). There are currently no conditions in _delete that
will return error but there will be in the future. Add an err argument that
will be updated on failure.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/update-ref.c |  5 +++--
 refs.c               | 16 +++++++++++-----
 refs.h               | 12 ++++++++----
 3 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index e9c216e..cdb71a8 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -258,8 +258,9 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)
 	if (*next != line_termination)
 		die("delete %s: extra input: %s", refname, next);
 
-	ref_transaction_delete(transaction, refname, old_sha1,
-			       update_flags, have_old);
+	if (ref_transaction_delete(transaction, refname, old_sha1,
+				   update_flags, have_old, NULL))
+		die("failed transaction delete for %s", refname);
 
 	update_flags = 0;
 	free(refname);
diff --git a/refs.c b/refs.c
index 06a4fed..a588194 100644
--- a/refs.c
+++ b/refs.c
@@ -3417,19 +3417,25 @@ int ref_transaction_create(struct ref_transaction *transaction,
 	return 0;
 }
 
-void ref_transaction_delete(struct ref_transaction *transaction,
-			    const char *refname,
-			    const unsigned char *old_sha1,
-			    int flags, int have_old)
+int ref_transaction_delete(struct ref_transaction *transaction,
+			   const char *refname,
+			   const unsigned char *old_sha1,
+			   int flags, int have_old,
+			   struct strbuf *err)
 {
-	struct ref_update *update = add_update(transaction, refname);
+	struct ref_update *update;
 
+	if (have_old && !old_sha1)
+		die("BUG: have_old is true but old_sha1 is NULL");
+
+	update = add_update(transaction, refname);
 	update->flags = flags;
 	update->have_old = have_old;
 	if (have_old) {
 		assert(!is_null_sha1(old_sha1));
 		hashcpy(update->old_sha1, old_sha1);
 	}
+	return 0;
 }
 
 int update_ref(const char *action, const char *refname,
diff --git a/refs.h b/refs.h
index bcae348..9b978da 100644
--- a/refs.h
+++ b/refs.h
@@ -265,11 +265,15 @@ int ref_transaction_create(struct ref_transaction *transaction,
  * Add a reference deletion to transaction.  If have_old is true, then
  * old_sha1 holds the value that the reference should have had before
  * the update (which must not be the null SHA-1).
+ * Function returns 0 on success and non-zero on failure. A failure to delete
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back. On failure the err buffer will be updated.
  */
-void ref_transaction_delete(struct ref_transaction *transaction,
-			    const char *refname,
-			    const unsigned char *old_sha1,
-			    int flags, int have_old);
+int ref_transaction_delete(struct ref_transaction *transaction,
+			   const char *refname,
+			   const unsigned char *old_sha1,
+			   int flags, int have_old,
+			   struct strbuf *err);
 
 /*
  * Commit all of the changes that have been queued in transaction, as
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 13/44] tag.c: use ref transactions when doing updates

[Ronnie Sahlberg]

Change tag.c to use ref transactions for all ref updates.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/tag.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index c6e8a71..b05f9a5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -548,7 +548,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct strbuf ref = STRBUF_INIT;
 	unsigned char object[20], prev[20];
 	const char *object_ref, *tag;
-	struct ref_lock *lock;
 	struct create_tag_options opt;
 	char *cleanup_arg = NULL;
 	int annotate = 0, force = 0, lines = -1;
@@ -556,6 +555,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	const char *msgfile = NULL, *keyid = NULL;
 	struct msg_arg msg = { 0, STRBUF_INIT };
 	struct commit_list *with_commit = NULL;
+	struct ref_transaction *transaction;
+	struct strbuf err = STRBUF_INIT;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
@@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (annotate)
 		create_tag(object, tag, &buf, &opt, prev, object);
 
-	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)
-		die(_("%s: cannot update the ref"), ref.buf);
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_update(transaction, ref.buf, object, prev,
+				   0, !is_null_sha1(prev), &err) ||
+	    ref_transaction_commit(transaction, NULL, &err))
+		die("%s", err.buf);
 	if (force && !is_null_sha1(prev) && hashcmp(prev, object))
 		printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));
 
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 14/44] replace.c: use the ref transaction functions for updates

[Ronnie Sahlberg]

Update replace.c to use ref transactions for updates.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/replace.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 3da1bae..e8932cd 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -133,7 +133,8 @@ static int replace_object_sha1(const char *object_ref,
 	unsigned char prev[20];
 	enum object_type obj_type, repl_type;
 	char ref[PATH_MAX];
-	struct ref_lock *lock;
+	struct ref_transaction *transaction;
+	struct strbuf err = STRBUF_INIT;
 
 	if (snprintf(ref, sizeof(ref),
 		     "refs/replace/%s",
@@ -156,11 +157,12 @@ static int replace_object_sha1(const char *object_ref,
 	else if (!force)
 		die("replace ref '%s' already exists", 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)
-		die("%s: cannot update the ref", ref);
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_update(transaction, ref, repl, prev,
+				   0, !is_null_sha1(prev), &err) ||
+	    ref_transaction_commit(transaction, NULL, &err))
+		die("%s", err.buf);
 
 	return 0;
 }
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 15/44] commit.c: use ref transactions for updates

[Ronnie Sahlberg]

Change commit.c to use ref transactions for all ref updates.
Make sure we pass a NULL pointer to ref_transaction_update if have_old
is false.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/commit.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d28505a..c429216 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1581,11 +1581,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	const char *index_file, *reflog_msg;
 	char *nl;
 	unsigned char sha1[20];
-	struct ref_lock *ref_lock;
 	struct commit_list *parents = NULL, **pptr = &parents;
 	struct stat statbuf;
 	struct commit *current_head = NULL;
 	struct commit_extra_header *extra = NULL;
+	struct ref_transaction *transaction;
+	struct strbuf err = STRBUF_INIT;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_commit_usage, builtin_commit_options);
@@ -1707,16 +1708,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	strbuf_release(&author_ident);
 	free_commit_extra_headers(extra);
 
-	ref_lock = lock_any_ref_for_update("HEAD",
-					   !current_head
-					   ? NULL
-					   : current_head->object.sha1,
-					   0, NULL);
-	if (!ref_lock) {
-		rollback_index_files();
-		die(_("cannot lock HEAD ref"));
-	}
-
 	nl = strchr(sb.buf, '\n');
 	if (nl)
 		strbuf_setlen(&sb, nl + 1 - sb.buf);
@@ -1725,9 +1716,15 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
 	strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
 
-	if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_update(transaction, "HEAD", sha1,
+				   current_head ?
+				   current_head->object.sha1 : NULL,
+				   0, !!current_head, &err) ||
+	    ref_transaction_commit(transaction, sb.buf, &err)) {
 		rollback_index_files();
-		die(_("cannot update HEAD ref"));
+		die("%s", err.buf);
 	}
 
 	unlink(git_path("CHERRY_PICK_HEAD"));
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 16/44] sequencer.c: use ref transactions for all ref updates

[Ronnie Sahlberg]

Change to use ref transactions for all updates to refs.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 sequencer.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 0a80c58..b047fb8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -272,23 +272,31 @@ static int error_dirty_index(struct replay_opts *opts)
 static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 			int unborn, struct replay_opts *opts)
 {
-	struct ref_lock *ref_lock;
+	struct ref_transaction *transaction;
 	struct strbuf sb = STRBUF_INIT;
-	int ret;
+	struct strbuf err = STRBUF_INIT;
 
 	read_cache();
 	if (checkout_fast_forward(from, to, 1))
 		exit(1); /* the callee should have complained already */
-	ref_lock = lock_any_ref_for_update("HEAD", unborn ? null_sha1 : from,
-					   0, NULL);
-	if (!ref_lock)
-		return error(_("Failed to lock HEAD during fast_forward_to"));
 
 	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
-	ret = write_ref_sha1(ref_lock, to, sb.buf);
+
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_update(transaction, "HEAD", to, from,
+				   0, !unborn, &err) ||
+	    (ref_transaction_commit(transaction, sb.buf, &err) &&
+	     !(transaction = NULL))) {
+		ref_transaction_free(transaction);
+		error("%s", err.buf);
+		strbuf_release(&sb);
+		strbuf_release(&err);
+		return -1;
+	}
 
 	strbuf_release(&sb);
-	return ret;
+	return 0;
 }
 
 static int do_recursive_merge(struct commit *base, struct commit *next,
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 17/44] fast-import.c: change update_branch to use ref transactions

[Ronnie Sahlberg]

Change update_branch() to use ref transactions for updates.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 fast-import.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 6707a66..60d4538 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1679,39 +1679,44 @@ found_entry:
 static int update_branch(struct branch *b)
 {
 	static const char *msg = "fast-import";
-	struct ref_lock *lock;
+	struct ref_transaction *transaction;
 	unsigned char old_sha1[20];
+	struct strbuf err = STRBUF_INIT;
 
 	if (read_ref(b->name, old_sha1))
 		hashclr(old_sha1);
+
 	if (is_null_sha1(b->sha1)) {
 		if (b->delete)
 			delete_ref(b->name, old_sha1, 0);
 		return 0;
 	}
-	lock = lock_any_ref_for_update(b->name, old_sha1, 0, NULL);
-	if (!lock)
-		return error("Unable to lock %s", b->name);
 	if (!force_update && !is_null_sha1(old_sha1)) {
 		struct commit *old_cmit, *new_cmit;
 
 		old_cmit = lookup_commit_reference_gently(old_sha1, 0);
 		new_cmit = lookup_commit_reference_gently(b->sha1, 0);
-		if (!old_cmit || !new_cmit) {
-			unlock_ref(lock);
+		if (!old_cmit || !new_cmit)
 			return error("Branch %s is missing commits.", b->name);
-		}
 
 		if (!in_merge_bases(old_cmit, new_cmit)) {
-			unlock_ref(lock);
 			warning("Not updating %s"
 				" (new tip %s does not contain %s)",
 				b->name, sha1_to_hex(b->sha1), sha1_to_hex(old_sha1));
 			return -1;
 		}
 	}
-	if (write_ref_sha1(lock, b->sha1, msg) < 0)
-		return error("Unable to update %s", b->name);
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
+				   0, 1, &err) ||
+	    (ref_transaction_commit(transaction, msg, &err) &&
+	     !(transaction = NULL))) {
+		ref_transaction_free(transaction);
+		error("%s", err.buf);
+		strbuf_release(&err);
+		return -1;
+	}
 	return 0;
 }
 
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 18/44] branch.c: use ref transaction for all ref updates

[Ronnie Sahlberg]

Change create_branch to use a ref transaction when creating the new branch.
ref_transaction_create will check that the ref does not already exist and fail
otherwise meaning that we no longer need to keep a lock on the ref during the
setup_tracking. This simplifies the code since we can now do the transaction
in one single step.

If the forcing flag is false then use ref_transaction_create since this will
fail if the ref already exist. Otherwise use ref_transaction_update.

This also fixes a race condition in the old code where two concurrent
create_branch could race since the lock_any_ref_for_update/write_ref_sha1
did not protect against the ref already existing. I.e. one thread could end up
overwriting a branch even if the forcing flag is false.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 branch.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/branch.c b/branch.c
index 660097b..8bf7588 100644
--- a/branch.c
+++ b/branch.c
@@ -226,7 +226,6 @@ void create_branch(const char *head,
 		   int force, int reflog, int clobber_head,
 		   int quiet, enum branch_track track)
 {
-	struct ref_lock *lock = NULL;
 	struct commit *commit;
 	unsigned char sha1[20];
 	char *real_ref, msg[PATH_MAX + 20];
@@ -285,15 +284,6 @@ void create_branch(const char *head,
 		die(_("Not a valid branch point: '%s'."), start_name);
 	hashcpy(sha1, commit->object.sha1);
 
-	if (!dont_change_ref) {
-		lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
-		if (!lock)
-			die_errno(_("Failed to lock ref for update"));
-	}
-
-	if (reflog)
-		log_all_ref_updates = 1;
-
 	if (forcing)
 		snprintf(msg, sizeof msg, "branch: Reset to %s",
 			 start_name);
@@ -301,13 +291,24 @@ void create_branch(const char *head,
 		snprintf(msg, sizeof msg, "branch: Created from %s",
 			 start_name);
 
+	if (reflog)
+		log_all_ref_updates = 1;
+
+	if (!dont_change_ref) {
+		struct ref_transaction *transaction;
+		struct strbuf err = STRBUF_INIT;
+
+		transaction = ref_transaction_begin();
+		if (!transaction ||
+		    ref_transaction_update(transaction, ref.buf, sha1,
+					   null_sha1, 0, !forcing, &err) ||
+		    ref_transaction_commit(transaction, msg, &err))
+			die("%s", err.buf);
+	}
+
 	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)
-			die_errno(_("Failed to write ref"));
-
 	strbuf_release(&ref);
 	free(real_ref);
 }
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 19/44] refs.c: change update_ref to use a transaction

[Ronnie Sahlberg]

Change the update_ref helper function to use a ref transaction internally.

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

diff --git a/refs.c b/refs.c
index a588194..78312b5 100644
--- a/refs.c
+++ b/refs.c
@@ -3442,11 +3442,28 @@ int update_ref(const char *action, const char *refname,
 	       const unsigned char *sha1, const unsigned char *oldval,
 	       int flags, enum action_on_err onerr)
 {
-	struct ref_lock *lock;
-	lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
-	if (!lock)
+	struct ref_transaction *t;
+	struct strbuf err = STRBUF_INIT;
+
+	t = ref_transaction_begin();
+	if ((!t ||
+	    ref_transaction_update(t, refname, sha1, oldval, flags,
+				   !!oldval, &err)) ||
+	    (ref_transaction_commit(t, action, &err) && !(t = NULL))) {
+		const char *str = "update_ref failed for ref '%s': %s";
+
+		ref_transaction_free(t);
+		switch (onerr) {
+		case UPDATE_REFS_MSG_ON_ERR:
+			error(str, refname, err.buf); break;
+		case UPDATE_REFS_DIE_ON_ERR:
+			die(str, refname, err.buf); break;
+		case UPDATE_REFS_QUIET_ON_ERR: break;
+		}
+		strbuf_release(&err);
 		return 1;
-	return update_ref_write(action, refname, sha1, lock, NULL, onerr);
+	}
+	return 0;
 }
 
 static int ref_update_compare(const void *r1, const void *r2)
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 20/44] refs.c: free the transaction before returning when number of updates is 0

[Ronnie Sahlberg]

We have to free the transaction before returning in the early check for
'return early if number of updates == 0' or else the following code would
create a memory leak with the transaction never being freed :
   t = ref_transaction_begin()
   ref_transaction_commit(t)

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 78312b5..2c3f070 100644
--- a/refs.c
+++ b/refs.c
@@ -3497,8 +3497,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
 
-	if (!n)
+	if (!n) {
+		ref_transaction_free(transaction);
 		return 0;
+	}
 
 	/* Allocate work space */
 	delnames = xmalloc(sizeof(*delnames) * n);
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 21/44] refs.c: ref_transaction_commit should not free the transaction

[Ronnie Sahlberg]

Change ref_transaction_commit so that it does not free the transaction.
Instead require that a caller will end a transaction by calling
ref_transaction_free.

By having the transaction object remaining valid after _commit returns allows
us to write much nicer code and still be able to call ref_transaction_free
safely. Instead of this horribleness
	t = ref_transaction_begin();
	if ((!t ||
	    ref_transaction_update(t, refname, sha1, oldval, flags,
				   !!oldval)) ||
	    (ref_transaction_commit(t, action, &err) && !(t = NULL))) {
		ref_transaction_free(t);

we can now just do the much nicer
	t = ref_transaction_begin();
	if (!t ||
	    ref_transaction_update(t, refname, sha1, oldval, flags,
				   !!oldval) ||
	    ref_transaction_commit(&t, action, &err)) {
		ref_transaction_free(t);
		... die/return ...

	ref_transaction_free(transaction);

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 branch.c             |  1 +
 builtin/commit.c     |  1 +
 builtin/replace.c    |  1 +
 builtin/tag.c        |  1 +
 builtin/update-ref.c |  1 +
 fast-import.c        |  4 ++--
 refs.c               | 12 +++++-------
 refs.h               |  8 +++-----
 sequencer.c          |  4 ++--
 9 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/branch.c b/branch.c
index 8bf7588..f78a28b 100644
--- a/branch.c
+++ b/branch.c
@@ -304,6 +304,7 @@ void create_branch(const char *head,
 					   null_sha1, 0, !forcing, &err) ||
 		    ref_transaction_commit(transaction, msg, &err))
 			die("%s", err.buf);
+		ref_transaction_free(transaction);
 	}
 
 	if (real_ref && track)
diff --git a/builtin/commit.c b/builtin/commit.c
index c429216..6b888f2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1726,6 +1726,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		rollback_index_files();
 		die("%s", err.buf);
 	}
+	ref_transaction_free(transaction);
 
 	unlink(git_path("CHERRY_PICK_HEAD"));
 	unlink(git_path("REVERT_HEAD"));
diff --git a/builtin/replace.c b/builtin/replace.c
index e8932cd..af7f72d 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -164,6 +164,7 @@ static int replace_object_sha1(const char *object_ref,
 	    ref_transaction_commit(transaction, NULL, &err))
 		die("%s", err.buf);
 
+	ref_transaction_free(transaction);
 	return 0;
 }
 
diff --git a/builtin/tag.c b/builtin/tag.c
index b05f9a5..30b471c 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -708,6 +708,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 				   0, !is_null_sha1(prev), &err) ||
 	    ref_transaction_commit(transaction, NULL, &err))
 		die("%s", err.buf);
+	ref_transaction_free(transaction);
 	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/builtin/update-ref.c b/builtin/update-ref.c
index cdb71a8..4c2145e 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -373,6 +373,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		update_refs_stdin();
 		if (ref_transaction_commit(transaction, msg, &err))
 			die("%s", err.buf);
+		ref_transaction_free(transaction);
 		return 0;
 	}
 
diff --git a/fast-import.c b/fast-import.c
index 60d4538..cb3f5af 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1710,13 +1710,13 @@ static int update_branch(struct branch *b)
 	if (!transaction ||
 	    ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
 				   0, 1, &err) ||
-	    (ref_transaction_commit(transaction, msg, &err) &&
-	     !(transaction = NULL))) {
+	    ref_transaction_commit(transaction, msg, &err)) {
 		ref_transaction_free(transaction);
 		error("%s", err.buf);
 		strbuf_release(&err);
 		return -1;
 	}
+	ref_transaction_free(transaction);
 	return 0;
 }
 
diff --git a/refs.c b/refs.c
index 2c3f070..266a792 100644
--- a/refs.c
+++ b/refs.c
@@ -3446,10 +3446,10 @@ int update_ref(const char *action, const char *refname,
 	struct strbuf err = STRBUF_INIT;
 
 	t = ref_transaction_begin();
-	if ((!t ||
+	if (!t ||
 	    ref_transaction_update(t, refname, sha1, oldval, flags,
-				   !!oldval, &err)) ||
-	    (ref_transaction_commit(t, action, &err) && !(t = NULL))) {
+				   !!oldval, &err) ||
+	    ref_transaction_commit(t, action, &err)) {
 		const char *str = "update_ref failed for ref '%s': %s";
 
 		ref_transaction_free(t);
@@ -3463,6 +3463,7 @@ int update_ref(const char *action, const char *refname,
 		strbuf_release(&err);
 		return 1;
 	}
+	ref_transaction_free(t);
 	return 0;
 }
 
@@ -3497,10 +3498,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
 
-	if (!n) {
-		ref_transaction_free(transaction);
+	if (!n)
 		return 0;
-	}
 
 	/* Allocate work space */
 	delnames = xmalloc(sizeof(*delnames) * n);
@@ -3567,7 +3566,6 @@ cleanup:
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
 	free(delnames);
-	ref_transaction_free(transaction);
 	return ret;
 }
 
diff --git a/refs.h b/refs.h
index 9b978da..e0f5f67 100644
--- a/refs.h
+++ b/refs.h
@@ -216,8 +216,7 @@ enum action_on_err {
 
 /*
  * Begin a reference transaction.  The reference transaction must
- * eventually be commited using ref_transaction_commit() or freed by
- * calling ref_transaction_free().
+ * eventually be freed by calling ref_transaction_free().
  */
 struct ref_transaction *ref_transaction_begin(void);
 
@@ -278,9 +277,8 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 /*
  * Commit all of the changes that have been queued in transaction, as
  * atomically as possible.  Return a nonzero value if there is a
- * problem.  The ref_transaction is freed by this function.
- * If err is non-NULL we will add an error string to it to explain why
- * the transaction failed. The string does not end in newline.
+ * problem. If err is non-NULL we will add an error string to it to explain
+ * why the transaction failed. The string does not end in newline.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   const char *msg, struct strbuf *err);
diff --git a/sequencer.c b/sequencer.c
index b047fb8..a2bc9e1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -286,14 +286,14 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 	if (!transaction ||
 	    ref_transaction_update(transaction, "HEAD", to, from,
 				   0, !unborn, &err) ||
-	    (ref_transaction_commit(transaction, sb.buf, &err) &&
-	     !(transaction = NULL))) {
+	    ref_transaction_commit(transaction, sb.buf, &err)) {
 		ref_transaction_free(transaction);
 		error("%s", err.buf);
 		strbuf_release(&sb);
 		strbuf_release(&err);
 		return -1;
 	}
+	ref_transaction_free(transaction);
 
 	strbuf_release(&sb);
 	return 0;
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 22/44] fetch.c: clear errno before calling functions that might set it

[Ronnie Sahlberg]

In s_update_ref there are two calls that when they fail we return an error
based on the errno value. In particular we want to return a specific error
if ENOTDIR happened. Both these functions do have failure modes where they
may return an error without updating errno, in which case a previous and
unrelated ENOTDIR may cause us to return the wrong error. Clear errno before
calling any functions if we check errno afterwards.

Also skip initializing a static variable to 0. Statics live in .bss and
are all automatically initialized to 0.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/fetch.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 55f457c..a93c893 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -44,7 +44,7 @@ static struct transport *gtransport;
 static struct transport *gsecondary;
 static const char *submodule_prefix = "";
 static const char *recurse_submodules_default;
-static int shown_url = 0;
+static int shown_url;
 
 static int option_parse_recurse_submodules(const struct option *opt,
 				   const char *arg, int unset)
@@ -382,6 +382,8 @@ static int s_update_ref(const char *action,
 	if (!rla)
 		rla = default_rla.buf;
 	snprintf(msg, sizeof(msg), "%s: %s", rla, action);
+
+	errno = 0;
 	lock = lock_any_ref_for_update(ref->name,
 				       check_old ? ref->old_sha1 : NULL,
 				       0, NULL);
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 23/44] fetch.c: change s_update_ref to use a ref transaction

[Ronnie Sahlberg]

Change s_update_ref to use a ref transaction for the ref update.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/fetch.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index a93c893..8cf70cd 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -375,7 +375,7 @@ static int s_update_ref(const char *action,
 {
 	char msg[1024];
 	char *rla = getenv("GIT_REFLOG_ACTION");
-	static struct ref_lock *lock;
+	struct ref_transaction *transaction;
 
 	if (dry_run)
 		return 0;
@@ -384,15 +384,16 @@ static int s_update_ref(const char *action,
 	snprintf(msg, sizeof(msg), "%s: %s", rla, action);
 
 	errno = 0;
-	lock = lock_any_ref_for_update(ref->name,
-				       check_old ? ref->old_sha1 : NULL,
-				       0, NULL);
-	if (!lock)
-		return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
-					  STORE_REF_ERROR_OTHER;
-	if (write_ref_sha1(lock, ref->new_sha1, msg) < 0)
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_update(transaction, ref->name, ref->new_sha1,
+				   ref->old_sha1, 0, check_old, NULL) ||
+	    ref_transaction_commit(transaction, msg, NULL)) {
+		ref_transaction_free(transaction);
 		return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
 					  STORE_REF_ERROR_OTHER;
+	}
+	ref_transaction_free(transaction);
 	return 0;
 }
 
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 24/44] fetch.c: use a single ref transaction for all ref updates

[Ronnie Sahlberg]

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 8cf70cd..5b0cc31 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;
+static 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, NULL) ||
-	    ref_transaction_commit(transaction, msg, NULL)) {
-		ref_transaction_free(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, NULL))
+		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"
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 25/44] receive-pack.c: use a reference transaction for updating the refs

[Ronnie Sahlberg]

Wrap all the ref updates inside a transaction to make the update atomic.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 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..5534138 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;
+static struct strbuf err = STRBUF_INIT;
+static 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, &err))
+			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.510.g20c254b

[PATCH v10 26/44] fast-import.c: use a ref transaction when dumping tags

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 fast-import.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index cb3f5af..9c76c73 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1735,15 +1735,22 @@ static void dump_tags(void)
 {
 	static const char *msg = "fast-import";
 	struct tag *t;
-	struct ref_lock *lock;
 	char ref_name[PATH_MAX];
+	struct strbuf err = STRBUF_INIT;
+	struct ref_transaction *transaction;
 
+	transaction = ref_transaction_begin();
 	for (t = first_tag; t; t = t->next_tag) {
-		sprintf(ref_name, "tags/%s", t->name);
-		lock = lock_ref_sha1(ref_name, NULL);
-		if (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)
-			failure |= error("Unable to update %s", ref_name);
+		sprintf(ref_name, "refs/tags/%s", t->name);
+
+		if (ref_transaction_update(transaction, ref_name, t->sha1,
+					   NULL, 0, 0, &err))
+			failure |= error("%s", err.buf);
 	}
+	if (failure || ref_transaction_commit(transaction, msg, &err))
+		failure |= error("%s", err.buf);
+	ref_transaction_free(transaction);
+	strbuf_release(&err);
 }
 
 static void dump_marks_helper(FILE *f,
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 27/44] walker.c: use ref transaction for ref updates

[Ronnie Sahlberg]

Switch to using ref transactions in walker_fetch(). As part of the refactoring
to use ref transactions we also fix a potential memory leak where in the
original code if write_ref_sha1() would fail we would end up returning from
the function without free()ing the msg string.

This changes the locking slightly for walker_fetch. Previously the code would
lock all refs before writing them but now we do not lock the refs until the
commit stage. There is thus a very short window where changes could be done
locally during the fetch which would be overwritten when the fetch completes
and commits its transaction. But this window should be reasonably short.
Even if this race does trigger, since both the old code and the new code
just overwrites the refs to the new values without checking or comparing
them with the previous value, this is not too dissimilar to a similar scenario
where you first do a ref change locally and then later do a fetch that
overwrites the local change. With this in mind I do not see the change in
locking semantics to be critical.

Note that this function is only called when fetching from a remote HTTP
repository onto the local (most of the time single-user) repository which
likely means that the type of collissions that the previous locking would
protect against and cause the fetch to fail for to be even more rare.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 walker.c | 51 ++++++++++++++++++++++++++-------------------------
 1 file changed, 26 insertions(+), 25 deletions(-)

diff --git a/walker.c b/walker.c
index 1dd86b8..cbe5686 100644
--- a/walker.c
+++ b/walker.c
@@ -251,24 +251,18 @@ void walker_targets_free(int targets, char **target, const char **write_ref)
 int walker_fetch(struct walker *walker, int targets, char **target,
 		 const char **write_ref, const char *write_ref_log_details)
 {
-	struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));
+	char ref_name[PATH_MAX];
+	struct strbuf err = STRBUF_INIT;
+	struct ref_transaction *transaction;
 	unsigned char *sha1 = xmalloc(targets * 20);
-	char *msg;
-	int ret;
+	char *msg = NULL;
 	int i;
 
 	save_commit_buffer = 0;
 
-	for (i = 0; i < targets; i++) {
-		if (!write_ref || !write_ref[i])
-			continue;
-
-		lock[i] = lock_ref_sha1(write_ref[i], NULL);
-		if (!lock[i]) {
-			error("Can't lock ref %s", write_ref[i]);
-			goto unlock_and_fail;
-		}
-	}
+	transaction = ref_transaction_begin();
+	if (!transaction)
+		return -1;
 
 	if (!walker->get_recover)
 		for_each_ref(mark_complete, NULL);
@@ -276,14 +270,14 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 	for (i = 0; i < targets; i++) {
 		if (interpret_target(walker, target[i], &sha1[20 * i])) {
 			error("Could not interpret response from server '%s' as something to pull", target[i]);
-			goto unlock_and_fail;
+			goto rollback_and_fail;
 		}
 		if (process(walker, lookup_unknown_object(&sha1[20 * i])))
-			goto unlock_and_fail;
+			goto rollback_and_fail;
 	}
 
 	if (loop(walker))
-		goto unlock_and_fail;
+		goto rollback_and_fail;
 
 	if (write_ref_log_details) {
 		msg = xmalloc(strlen(write_ref_log_details) + 12);
@@ -294,19 +288,26 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 	for (i = 0; i < targets; i++) {
 		if (!write_ref || !write_ref[i])
 			continue;
-		ret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : "fetch (unknown)");
-		lock[i] = NULL;
-		if (ret)
-			goto unlock_and_fail;
+		sprintf(ref_name, "refs/%s", write_ref[i]);
+		if (ref_transaction_update(transaction, ref_name,
+					   &sha1[20 * i], NULL,
+					   0, 0, &err))
+			goto rollback_and_fail;
+	}
+
+	if (ref_transaction_commit(transaction, msg ? msg : "fetch (unknown)",
+				   &err)) {
+		error("%s", err.buf);
+		goto rollback_and_fail;
 	}
-	free(msg);
 
+	free(msg);
 	return 0;
 
-unlock_and_fail:
-	for (i = 0; i < targets; i++)
-		if (lock[i])
-			unlock_ref(lock[i]);
+rollback_and_fail:
+	free(msg);
+	strbuf_release(&err);
+	ref_transaction_free(transaction);
 
 	return -1;
 }
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 28/44] refs.c: make write_ref_sha1 static

[Ronnie Sahlberg]

No external users call write_ref_sha1 any more so lets declare it static.

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

diff --git a/refs.c b/refs.c
index 266a792..93e2cd2 100644
--- a/refs.c
+++ b/refs.c
@@ -251,6 +251,8 @@ struct ref_entry {
 };
 
 static void read_loose_refs(const char *dirname, struct ref_dir *dir);
+static int write_ref_sha1(struct ref_lock *lock,
+			  const unsigned char *sha1, const char *logmsg);
 
 static struct ref_dir *get_ref_dir(struct ref_entry *entry)
 {
@@ -2833,7 +2835,7 @@ static int is_branch(const char *refname)
 	return !strcmp(refname, "HEAD") || starts_with(refname, "refs/heads/");
 }
 
-int write_ref_sha1(struct ref_lock *lock,
+static int write_ref_sha1(struct ref_lock *lock,
 	const unsigned char *sha1, const char *logmsg)
 {
 	static char term = '\n';
diff --git a/refs.h b/refs.h
index e0f5f67..0c4d043 100644
--- a/refs.h
+++ b/refs.h
@@ -150,9 +150,6 @@ extern int commit_ref(struct ref_lock *lock);
 /** Release any lock taken but not written. **/
 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);
-
 /** Setup reflog before using. **/
 int log_ref_setup(const char *refname, char *logfile, int bufsize);
 
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 29/44] refs.c: make lock_ref_sha1 static

[Ronnie Sahlberg]

No external callers reference lock_ref_sha1 any more so lets declare it static.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 2 +-
 refs.h | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 93e2cd2..2d9789f 100644
--- a/refs.c
+++ b/refs.c
@@ -2126,7 +2126,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	return NULL;
 }
 
-struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1)
+static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1)
 {
 	char refpath[PATH_MAX];
 	if (check_refname_format(refname, 0))
diff --git a/refs.h b/refs.h
index 0c4d043..90c7fb4 100644
--- a/refs.h
+++ b/refs.h
@@ -132,9 +132,6 @@ extern int ref_exists(const char *);
  */
 extern int peel_ref(const char *refname, unsigned char *sha1);
 
-/** Locks a "refs/" ref returning the lock on success and NULL on failure. **/
-extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1);
-
 /** Locks any ref (for 'HEAD' type refs). */
 #define REF_NODEREF	0x01
 extern struct ref_lock *lock_any_ref_for_update(const char *refname,
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 30/44] refs.c: add transaction.status and track OPEN/CLOSED/ERROR

[Ronnie Sahlberg]

Track the status of a transaction in a new status field. Check the field for
sanity, i.e. that status must be OPEN when _commit/_create/_delete or
_update is called or else die(BUG:...)

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 2d9789f..4f7ae92 100644
--- a/refs.c
+++ b/refs.c
@@ -3336,6 +3336,12 @@ struct ref_update {
 	const char refname[FLEX_ARRAY];
 };
 
+enum ref_transaction_status {
+	REF_TRANSACTION_OPEN   = 0,
+	REF_TRANSACTION_CLOSED = 1,
+	REF_TRANSACTION_ERROR  = 2,
+};
+
 /*
  * Data structure for holding a reference transaction, which can
  * consist of checks and updates to multiple references, carried out
@@ -3345,6 +3351,7 @@ struct ref_transaction {
 	struct ref_update **updates;
 	size_t alloc;
 	size_t nr;
+	enum ref_transaction_status status;
 };
 
 struct ref_transaction *ref_transaction_begin(void)
@@ -3390,6 +3397,9 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	if (have_old && !old_sha1)
 		die("BUG: have_old is true but old_sha1 is NULL");
 
+	if (transaction->status != REF_TRANSACTION_OPEN)
+		die("BUG: update on transaction that is not open");
+
 	update = add_update(transaction, refname);
 	hashcpy(update->new_sha1, new_sha1);
 	update->flags = flags;
@@ -3410,6 +3420,9 @@ int ref_transaction_create(struct ref_transaction *transaction,
 	if (!new_sha1 || is_null_sha1(new_sha1))
 		die("BUG: create ref with null new_sha1");
 
+	if (transaction->status != REF_TRANSACTION_OPEN)
+		die("BUG: create on transaction that is not open");
+
 	update = add_update(transaction, refname);
 
 	hashcpy(update->new_sha1, new_sha1);
@@ -3430,6 +3443,9 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 	if (have_old && !old_sha1)
 		die("BUG: have_old is true but old_sha1 is NULL");
 
+	if (transaction->status != REF_TRANSACTION_OPEN)
+		die("BUG: delete on transaction that is not open");
+
 	update = add_update(transaction, refname);
 	update->flags = flags;
 	update->have_old = have_old;
@@ -3500,8 +3516,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
 
-	if (!n)
+	if (transaction->status != REF_TRANSACTION_OPEN)
+		die("BUG: commit on transaction that is not open");
+
+	if (!n) {
+		transaction->status = REF_TRANSACTION_CLOSED;
 		return 0;
+	}
 
 	/* Allocate work space */
 	delnames = xmalloc(sizeof(*delnames) * n);
@@ -3564,6 +3585,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	clear_loose_ref_cache(&ref_cache);
 
 cleanup:
+	transaction->status = ret ? REF_TRANSACTION_ERROR
+	  : REF_TRANSACTION_CLOSED;
+
 	for (i = 0; i < n; i++)
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 31/44] refs.c: remove the update_ref_lock function

[Ronnie Sahlberg]

Since we now only call update_ref_lock with onerr==QUIET_ON_ERR we no longer
need this function and can replace it with just calling lock_any_ref_for_update
directly.

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

diff --git a/refs.c b/refs.c
index 4f7ae92..04de777 100644
--- a/refs.c
+++ b/refs.c
@@ -3283,24 +3283,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
 	return retval;
 }
 
-static struct ref_lock *update_ref_lock(const char *refname,
-					const unsigned char *oldval,
-					int flags, int *type_p,
-					enum action_on_err onerr)
-{
-	struct ref_lock *lock;
-	lock = lock_any_ref_for_update(refname, oldval, flags, type_p);
-	if (!lock) {
-		const char *str = "Cannot lock the ref '%s'.";
-		switch (onerr) {
-		case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
-		case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
-		case UPDATE_REFS_QUIET_ON_ERR: break;
-		}
-	}
-	return lock;
-}
-
 static int update_ref_write(const char *action, const char *refname,
 			    const unsigned char *sha1, struct ref_lock *lock,
 			    struct strbuf *err, enum action_on_err onerr)
@@ -3537,12 +3519,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
-		update->lock = update_ref_lock(update->refname,
-					       (update->have_old ?
-						update->old_sha1 : NULL),
-					       update->flags,
-					       &update->type,
-					       UPDATE_REFS_QUIET_ON_ERR);
+		update->lock = lock_any_ref_for_update(update->refname,
+						       (update->have_old ?
+							update->old_sha1 :
+							NULL),
+						       update->flags,
+						       &update->type);
 		if (!update->lock) {
 			if (err)
 				strbuf_addf(err, "Cannot lock the ref '%s'.",
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 32/44] refs.c: remove the update_ref_write function

[Ronnie Sahlberg]

Since we only call update_ref_write from a single place and we only call it
with onerr==QUIET_ON_ERR we can just as well get rid of it and just call
write_ref_sha1 directly.

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

diff --git a/refs.c b/refs.c
index 04de777..b1acf51 100644
--- a/refs.c
+++ b/refs.c
@@ -3283,25 +3283,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
 	return retval;
 }
 
-static int update_ref_write(const char *action, const char *refname,
-			    const unsigned char *sha1, struct ref_lock *lock,
-			    struct strbuf *err, enum action_on_err onerr)
-{
-	if (write_ref_sha1(lock, sha1, action) < 0) {
-		const char *str = "Cannot update the ref '%s'.";
-		if (err)
-			strbuf_addf(err, str, refname);
-
-		switch (onerr) {
-		case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
-		case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
-		case UPDATE_REFS_QUIET_ON_ERR: break;
-		}
-		return 1;
-	}
-	return 0;
-}
-
 /**
  * Information needed for a single ref update.  Set new_sha1 to the
  * new value or to zero to delete the ref.  To check the old value
@@ -3539,14 +3520,16 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		struct ref_update *update = updates[i];
 
 		if (!is_null_sha1(update->new_sha1)) {
-			ret = update_ref_write(msg,
-					       update->refname,
-					       update->new_sha1,
-					       update->lock, err,
-					       UPDATE_REFS_QUIET_ON_ERR);
-			update->lock = NULL; /* freed by update_ref_write */
-			if (ret)
+			ret = write_ref_sha1(update->lock, update->new_sha1,
+					     msg);
+			update->lock = NULL; /* freed by write_ref_sha1 */
+			if (ret) {
+				const char *str = "Cannot update the ref '%s'.";
+
+				if (err)
+					strbuf_addf(err, str, update->refname);
 				goto cleanup;
+			}
 		}
 	}
 
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 33/44] refs.c: remove lock_ref_sha1

[Ronnie Sahlberg]

lock_ref_sha1 was only called from one place in refc.c and only provided
a check that the refname was sane before adding back the initial "refs/"
part of the ref path name, the initial "refs/" that this caller had already
stripped off before calling lock_ref_sha1.

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

diff --git a/refs.c b/refs.c
index b1acf51..69623e4 100644
--- a/refs.c
+++ b/refs.c
@@ -2126,15 +2126,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	return NULL;
 }
 
-static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1)
-{
-	char refpath[PATH_MAX];
-	if (check_refname_format(refname, 0))
-		return NULL;
-	strcpy(refpath, mkpath("refs/%s", refname));
-	return lock_ref_sha1_basic(refpath, old_sha1, 0, NULL);
-}
-
 struct ref_lock *lock_any_ref_for_update(const char *refname,
 					 const unsigned char *old_sha1,
 					 int flags, int *type_p)
@@ -2339,8 +2330,12 @@ static void try_remove_empty_parents(char *name)
 /* make sure nobody touched the ref, and unlink */
 static void prune_ref(struct ref_to_prune *r)
 {
-	struct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);
+	struct ref_lock *lock;
+
+	if (check_refname_format(r->name + 5, 0))
+		return;
 
+	lock = lock_ref_sha1_basic(r->name, r->sha1, 0, NULL);
 	if (lock) {
 		unlink_or_warn(git_path("%s", r->name));
 		unlock_ref(lock);
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 34/44] refs.c: make prune_ref use a transaction to delete the ref

[Ronnie Sahlberg]

Change prune_ref to delete the ref using a ref transaction. To do this we also
need to add a new flag REF_ISPRUNING that will tell the transaction that we
do not want to delete this ref from the packed refs. This flag is private to
refs.c and not exposed to external callers.

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

diff --git a/refs.c b/refs.c
index 69623e4..3a5f1e5 100644
--- a/refs.c
+++ b/refs.c
@@ -29,6 +29,11 @@ static inline int bad_ref_char(int ch)
 	return 0;
 }
 
+/** Used as a flag to ref_transaction_delete when a loose ref is being
+ *  pruned.
+ */
+#define REF_ISPRUNING	0x0100
+
 /*
  * Try to read one refname component from the front of refname.  Return
  * the length of the component found, or -1 if the component is not
@@ -2330,17 +2335,24 @@ static void try_remove_empty_parents(char *name)
 /* make sure nobody touched the ref, and unlink */
 static void prune_ref(struct ref_to_prune *r)
 {
-	struct ref_lock *lock;
+	struct ref_transaction *transaction;
+	struct strbuf err = STRBUF_INIT;
 
 	if (check_refname_format(r->name + 5, 0))
 		return;
 
-	lock = lock_ref_sha1_basic(r->name, r->sha1, 0, NULL);
-	if (lock) {
-		unlink_or_warn(git_path("%s", r->name));
-		unlock_ref(lock);
-		try_remove_empty_parents(r->name);
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_delete(transaction, r->name, r->sha1,
+				   REF_ISPRUNING, 1, &err) ||
+	    ref_transaction_commit(transaction, NULL, &err)) {
+		ref_transaction_free(transaction);
+		warning("prune_ref: %s", err.buf);
+		strbuf_release(&err);
+		return;
 	}
+	ref_transaction_free(transaction);
+	try_remove_empty_parents(r->name);
 }
 
 static void prune_refs(struct ref_to_prune *r)
@@ -3533,9 +3545,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		struct ref_update *update = updates[i];
 
 		if (update->lock) {
-			delnames[delnum++] = update->lock->ref_name;
 			ret |= delete_ref_loose(update->lock, update->type,
 						err);
+			if (!(update->flags & REF_ISPRUNING))
+				delnames[delnum++] = update->lock->ref_name;
 		}
 	}
 
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 35/44] refs.c: make delete_ref use a transaction

[Ronnie Sahlberg]

Change delete_ref to use a ref transaction for the deletion. At the same time
since we no longer have any callers of repack_without_ref we can now delete
this function.

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

diff --git a/refs.c b/refs.c
index 3a5f1e5..b16cf4c 100644
--- a/refs.c
+++ b/refs.c
@@ -2495,11 +2495,6 @@ static int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 	return ret;
 }
 
-static int repack_without_ref(const char *refname)
-{
-	return repack_without_refs(&refname, 1, NULL);
-}
-
 static int add_err_if_unremovable(const char *op, const char *file,
 				  struct strbuf *e, int rc)
 {
@@ -2543,24 +2538,18 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
 
 int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 {
-	struct ref_lock *lock;
-	int ret = 0, flag = 0;
+	struct ref_transaction *transaction;
 
-	lock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);
-	if (!lock)
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_delete(transaction, refname, sha1, delopt,
+				   sha1 && !is_null_sha1(sha1), NULL) ||
+	    ref_transaction_commit(transaction, NULL, NULL)) {
+		ref_transaction_free(transaction);
 		return 1;
-	ret |= delete_ref_loose(lock, flag, NULL);
-
-	/* removing the loose one could have resurrected an earlier
-	 * packed one.  Also, if it was not loose we need to repack
-	 * without it.
-	 */
-	ret |= repack_without_ref(lock->ref_name);
-
-	unlink_or_warn(git_path("logs/%s", lock->ref_name));
-	clear_loose_ref_cache(&ref_cache);
-	unlock_ref(lock);
-	return ret;
+	}
+	ref_transaction_free(transaction);
+	return 0;
 }
 
 /*
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 36/44] refs.c: pass the ref log message to _create/delete/update instead of _commit

[Ronnie Sahlberg]

Change the reference transactions so that we pass the reflog message
through to the create/delete/update function instead of the commit message.
This allows for individual messages for each change in a multi ref
transaction.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 branch.c               |  4 ++--
 builtin/commit.c       |  4 ++--
 builtin/fetch.c        | 10 ++++++++--
 builtin/receive-pack.c |  5 +++--
 builtin/replace.c      |  4 ++--
 builtin/tag.c          |  4 ++--
 builtin/update-ref.c   | 13 +++++++------
 fast-import.c          |  8 ++++----
 refs.c                 | 31 +++++++++++++++++++++----------
 refs.h                 |  5 ++++-
 sequencer.c            |  4 ++--
 walker.c               |  6 +++---
 12 files changed, 60 insertions(+), 38 deletions(-)

diff --git a/branch.c b/branch.c
index f78a28b..6dfdc2e 100644
--- a/branch.c
+++ b/branch.c
@@ -301,8 +301,8 @@ void create_branch(const char *head,
 		transaction = ref_transaction_begin();
 		if (!transaction ||
 		    ref_transaction_update(transaction, ref.buf, sha1,
-					   null_sha1, 0, !forcing, &err) ||
-		    ref_transaction_commit(transaction, msg, &err))
+					   null_sha1, 0, !forcing, msg, &err) ||
+		    ref_transaction_commit(transaction, &err))
 			die("%s", err.buf);
 		ref_transaction_free(transaction);
 	}
diff --git a/builtin/commit.c b/builtin/commit.c
index 6b888f2..b361a13 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1721,8 +1721,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	    ref_transaction_update(transaction, "HEAD", sha1,
 				   current_head ?
 				   current_head->object.sha1 : NULL,
-				   0, !!current_head, &err) ||
-	    ref_transaction_commit(transaction, sb.buf, &err)) {
+				   0, !!current_head, sb.buf, &err) ||
+	    ref_transaction_commit(transaction, &err)) {
 		rollback_index_files();
 		die("%s", err.buf);
 	}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 5b0cc31..4603cb6 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -374,11 +374,17 @@ static int s_update_ref(const char *action,
 			struct ref *ref,
 			int check_old)
 {
+	char msg[1024];
+	char *rla = getenv("GIT_REFLOG_ACTION");
+
 	if (dry_run)
 		return 0;
+	if (!rla)
+		rla = default_rla.buf;
+	snprintf(msg, sizeof(msg), "%s: %s", rla, action);
 
 	if (ref_transaction_update(transaction, ref->name, ref->new_sha1,
-				   ref->old_sha1, 0, check_old, NULL))
+				   ref->old_sha1, 0, check_old, msg, NULL))
 		return STORE_REF_ERROR_OTHER;
 
 	return 0;
@@ -670,7 +676,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 			}
 		}
 	}
-	if (ref_transaction_commit(transaction, "fetch_ref transaction", NULL))
+	if (ref_transaction_commit(transaction, NULL))
 		rc |= errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
 		  STORE_REF_ERROR_OTHER;
 	ref_transaction_free(transaction);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 5534138..324a220 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -582,7 +582,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 			return "shallow error";
 
 		if (ref_transaction_update(transaction, namespaced_name,
-					   new_sha1, old_sha1, 0, 1, &err))
+					   new_sha1, old_sha1, 0, 1, "push",
+					   &err))
 			return "failed to update";
 		return NULL; /* good */
 	}
@@ -823,7 +824,7 @@ static void execute_commands(struct command *commands,
 			checked_connectivity = 0;
 		}
 	}
-	if (ref_transaction_commit(transaction, "push", &err))
+	if (ref_transaction_commit(transaction, &err))
 		error("%s", err.buf);
 	ref_transaction_free(transaction);
 	strbuf_release(&err);
diff --git a/builtin/replace.c b/builtin/replace.c
index af7f72d..4fa74c1 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -160,8 +160,8 @@ static int replace_object_sha1(const char *object_ref,
 	transaction = ref_transaction_begin();
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref, repl, prev,
-				   0, !is_null_sha1(prev), &err) ||
-	    ref_transaction_commit(transaction, NULL, &err))
+				   0, !is_null_sha1(prev), NULL, &err) ||
+	    ref_transaction_commit(transaction, &err))
 		die("%s", err.buf);
 
 	ref_transaction_free(transaction);
diff --git a/builtin/tag.c b/builtin/tag.c
index 30b471c..8d39068 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -705,8 +705,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	transaction = ref_transaction_begin();
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref.buf, object, prev,
-				   0, !is_null_sha1(prev), &err) ||
-	    ref_transaction_commit(transaction, NULL, &err))
+				   0, !is_null_sha1(prev), NULL, &err) ||
+	    ref_transaction_commit(transaction, &err))
 		die("%s", err.buf);
 	ref_transaction_free(transaction);
 	if (force && !is_null_sha1(prev) && hashcmp(prev, object))
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 4c2145e..8f13d54 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -16,6 +16,7 @@ static struct ref_transaction *transaction;
 
 static char line_termination = '\n';
 static int update_flags;
+static const char *msg;
 static struct strbuf err = STRBUF_INIT;
 
 /*
@@ -199,7 +200,7 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)
 		die("update %s: extra input: %s", refname, next);
 
 	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-				   update_flags, have_old, &err))
+				   update_flags, have_old, msg, &err))
 		die("update %s: failed: %s", refname, err.buf);
 
 	update_flags = 0;
@@ -227,7 +228,7 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)
 		die("create %s: extra input: %s", refname, next);
 
 	if (ref_transaction_create(transaction, refname, new_sha1,
-				   update_flags, NULL))
+				   update_flags, msg, &err))
 		die("cannot create ref '%s'", refname);
 
 	update_flags = 0;
@@ -259,7 +260,7 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)
 		die("delete %s: extra input: %s", refname, next);
 
 	if (ref_transaction_delete(transaction, refname, old_sha1,
-				   update_flags, have_old, NULL))
+				   update_flags, have_old, msg, &err))
 		die("failed transaction delete for %s", refname);
 
 	update_flags = 0;
@@ -292,7 +293,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)
 		die("verify %s: extra input: %s", refname, next);
 
 	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-				   update_flags, have_old, &err))
+				   update_flags, have_old, msg, &err))
 		die("verify %s: %s", refname, err.buf);
 
 	update_flags = 0;
@@ -345,7 +346,7 @@ static void update_refs_stdin(void)
 
 int cmd_update_ref(int argc, const char **argv, const char *prefix)
 {
-	const char *refname, *oldval, *msg = NULL;
+	const char *refname, *oldval;
 	unsigned char sha1[20], oldsha1[20];
 	int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
 	struct option options[] = {
@@ -371,7 +372,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		if (end_null)
 			line_termination = '\0';
 		update_refs_stdin();
-		if (ref_transaction_commit(transaction, msg, &err))
+		if (ref_transaction_commit(transaction, &err))
 			die("%s", err.buf);
 		ref_transaction_free(transaction);
 		return 0;
diff --git a/fast-import.c b/fast-import.c
index 9c76c73..a4049a2 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1709,8 +1709,8 @@ static int update_branch(struct branch *b)
 	transaction = ref_transaction_begin();
 	if (!transaction ||
 	    ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
-				   0, 1, &err) ||
-	    ref_transaction_commit(transaction, msg, &err)) {
+				   0, 1, msg, &err) ||
+	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_free(transaction);
 		error("%s", err.buf);
 		strbuf_release(&err);
@@ -1744,10 +1744,10 @@ static void dump_tags(void)
 		sprintf(ref_name, "refs/tags/%s", t->name);
 
 		if (ref_transaction_update(transaction, ref_name, t->sha1,
-					   NULL, 0, 0, &err))
+					   NULL, 0, 0, msg, &err))
 			failure |= error("%s", err.buf);
 	}
-	if (failure || ref_transaction_commit(transaction, msg, &err))
+	if (failure || ref_transaction_commit(transaction, &err))
 		failure |= error("%s", err.buf);
 	ref_transaction_free(transaction);
 	strbuf_release(&err);
diff --git a/refs.c b/refs.c
index b16cf4c..98f3c85 100644
--- a/refs.c
+++ b/refs.c
@@ -2344,8 +2344,8 @@ static void prune_ref(struct ref_to_prune *r)
 	transaction = ref_transaction_begin();
 	if (!transaction ||
 	    ref_transaction_delete(transaction, r->name, r->sha1,
-				   REF_ISPRUNING, 1, &err) ||
-	    ref_transaction_commit(transaction, NULL, &err)) {
+				   REF_ISPRUNING, 1, NULL, &err) ||
+	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_free(transaction);
 		warning("prune_ref: %s", err.buf);
 		strbuf_release(&err);
@@ -2543,8 +2543,8 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 	transaction = ref_transaction_begin();
 	if (!transaction ||
 	    ref_transaction_delete(transaction, refname, sha1, delopt,
-				   sha1 && !is_null_sha1(sha1), NULL) ||
-	    ref_transaction_commit(transaction, NULL, NULL)) {
+				   sha1 && !is_null_sha1(sha1), NULL, NULL) ||
+	    ref_transaction_commit(transaction, NULL)) {
 		ref_transaction_free(transaction);
 		return 1;
 	}
@@ -3292,6 +3292,7 @@ struct ref_update {
 	int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
 	struct ref_lock *lock;
 	int type;
+	const char *msg;
 	const char refname[FLEX_ARRAY];
 };
 
@@ -3325,9 +3326,10 @@ void ref_transaction_free(struct ref_transaction *transaction)
 	if (!transaction)
 		return;
 
-	for (i = 0; i < transaction->nr; i++)
+	for (i = 0; i < transaction->nr; i++) {
+	  free((char *)transaction->updates[i]->msg);
 		free(transaction->updates[i]);
-
+	}
 	free(transaction->updates);
 	free(transaction);
 }
@@ -3349,6 +3351,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
 			   const unsigned char *new_sha1,
 			   const unsigned char *old_sha1,
 			   int flags, int have_old,
+			   const char *msg,
 			   struct strbuf *err)
 {
 	struct ref_update *update;
@@ -3365,6 +3368,8 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	update->have_old = have_old;
 	if (have_old)
 		hashcpy(update->old_sha1, old_sha1);
+	if (msg)
+		update->msg = xstrdup(msg);
 	return 0;
 }
 
@@ -3372,6 +3377,7 @@ int ref_transaction_create(struct ref_transaction *transaction,
 			   const char *refname,
 			   const unsigned char *new_sha1,
 			   int flags,
+			   const char *msg,
 			   struct strbuf *err)
 {
 	struct ref_update *update;
@@ -3388,6 +3394,8 @@ int ref_transaction_create(struct ref_transaction *transaction,
 	hashclr(update->old_sha1);
 	update->flags = flags;
 	update->have_old = 1;
+	if (msg)
+		update->msg = xstrdup(msg);
 	return 0;
 }
 
@@ -3395,6 +3403,7 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 			   const char *refname,
 			   const unsigned char *old_sha1,
 			   int flags, int have_old,
+			   const char *msg,
 			   struct strbuf *err)
 {
 	struct ref_update *update;
@@ -3412,6 +3421,8 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 		assert(!is_null_sha1(old_sha1));
 		hashcpy(update->old_sha1, old_sha1);
 	}
+	if (msg)
+		update->msg = xstrdup(msg);
 	return 0;
 }
 
@@ -3425,8 +3436,8 @@ int update_ref(const char *action, const char *refname,
 	t = ref_transaction_begin();
 	if (!t ||
 	    ref_transaction_update(t, refname, sha1, oldval, flags,
-				   !!oldval, &err) ||
-	    ref_transaction_commit(t, action, &err)) {
+				   !!oldval, action, NULL) ||
+	    ref_transaction_commit(t, &err)) {
 		const char *str = "update_ref failed for ref '%s': %s";
 
 		ref_transaction_free(t);
@@ -3468,7 +3479,7 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 }
 
 int ref_transaction_commit(struct ref_transaction *transaction,
-			   const char *msg, struct strbuf *err)
+			   struct strbuf *err)
 {
 	int ret = 0, delnum = 0, i;
 	const char **delnames;
@@ -3517,7 +3528,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 
 		if (!is_null_sha1(update->new_sha1)) {
 			ret = write_ref_sha1(update->lock, update->new_sha1,
-					     msg);
+					     update->msg);
 			update->lock = NULL; /* freed by write_ref_sha1 */
 			if (ret) {
 				const char *str = "Cannot update the ref '%s'.";
diff --git a/refs.h b/refs.h
index 90c7fb4..57e7cd1 100644
--- a/refs.h
+++ b/refs.h
@@ -237,6 +237,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
 			   const unsigned char *new_sha1,
 			   const unsigned char *old_sha1,
 			   int flags, int have_old,
+			   const char *msg,
 			   struct strbuf *err);
 
 /*
@@ -252,6 +253,7 @@ int ref_transaction_create(struct ref_transaction *transaction,
 			   const char *refname,
 			   const unsigned char *new_sha1,
 			   int flags,
+			   const char *msg,
 			   struct strbuf *err);
 
 /*
@@ -266,6 +268,7 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 			   const char *refname,
 			   const unsigned char *old_sha1,
 			   int flags, int have_old,
+			   const char *msg,
 			   struct strbuf *err);
 
 /*
@@ -275,7 +278,7 @@ int ref_transaction_delete(struct ref_transaction *transaction,
  * why the transaction failed. The string does not end in newline.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
-			   const char *msg, struct strbuf *err);
+			   struct strbuf *err);
 
 /*
  * Free an existing transaction and all associated data.
diff --git a/sequencer.c b/sequencer.c
index a2bc9e1..ebf8feb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -285,8 +285,8 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 	transaction = ref_transaction_begin();
 	if (!transaction ||
 	    ref_transaction_update(transaction, "HEAD", to, from,
-				   0, !unborn, &err) ||
-	    ref_transaction_commit(transaction, sb.buf, &err)) {
+				   0, !unborn, sb.buf, &err) ||
+	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_free(transaction);
 		error("%s", err.buf);
 		strbuf_release(&sb);
diff --git a/walker.c b/walker.c
index cbe5686..0403feb 100644
--- a/walker.c
+++ b/walker.c
@@ -291,12 +291,12 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 		sprintf(ref_name, "refs/%s", write_ref[i]);
 		if (ref_transaction_update(transaction, ref_name,
 					   &sha1[20 * i], NULL,
-					   0, 0, &err))
+					   0, 0,
+					   msg ? msg : "fetch (unknown)", &err))
 			goto rollback_and_fail;
 	}
 
-	if (ref_transaction_commit(transaction, msg ? msg : "fetch (unknown)",
-				   &err)) {
+	if (ref_transaction_commit(transaction, &err)) {
 		error("%s", err.buf);
 		goto rollback_and_fail;
 	}
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 37/44] refs.c: pass NULL as *flags to read_ref_full

[Ronnie Sahlberg]

We call read_ref_full with a pointer to flags from rename_ref but since
we never actually use the returned flags we can just pass NULL here instead.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 98f3c85..1210345 100644
--- a/refs.c
+++ b/refs.c
@@ -2640,7 +2640,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 		goto rollback;
 	}
 
-	if (!read_ref_full(newrefname, sha1, 1, &flag) &&
+	if (!read_ref_full(newrefname, sha1, 1, NULL) &&
 	    delete_ref(newrefname, sha1, REF_NODEREF)) {
 		if (errno==EISDIR) {
 			if (remove_empty_directories(git_path("%s", newrefname))) {
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 38/44] refs.c: pack all refs before we start to rename a ref

[Ronnie Sahlberg]

This means that most loose refs will no longer be present after the rename
which triggered a test failure since it assumes the file for an unrelated
ref would still be present after the rename.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c            | 3 +++
 t/t3200-branch.sh | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 1210345..f4899e0 100644
--- a/refs.c
+++ b/refs.c
@@ -2635,6 +2635,9 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 		return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
 			oldrefname, strerror(errno));
 
+	if (pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE))
+		return error("unable to pack refs");
+
 	if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
 		error("unable to delete old %s", oldrefname);
 		goto rollback;
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index ac31b71..fafd38a 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -289,7 +289,7 @@ test_expect_success 'renaming a symref is not allowed' '
 	git symbolic-ref refs/heads/master2 refs/heads/master &&
 	test_must_fail git branch -m master2 master3 &&
 	git symbolic-ref refs/heads/master2 &&
-	test_path_is_file .git/refs/heads/master &&
+	test_path_is_missing .git/refs/heads/master &&
 	test_path_is_missing .git/refs/heads/master3
 '
 
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 39/44] refs.c: move the check for valid refname to lock_ref_sha1_basic

[Ronnie Sahlberg]

Move the check for check_refname_format from lock_any_ref_for_update
to lock_ref_sha1_basic. At some later stage we will get rid of
lock_any_ref_for_update completely.

This leaves lock_any_ref_for_updates as a no-op wrapper which could be removed.
But this wrapper is also called from an external caller and we will soon
make changes to the signature to lock_ref_sha1_basic that we do not want to
expose to that caller.

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

diff --git a/refs.c b/refs.c
index f4899e0..f63a356 100644
--- a/refs.c
+++ b/refs.c
@@ -2044,6 +2044,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	int missing = 0;
 	int attempts_remaining = 3;
 
+	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
+		return NULL;
+
 	lock = xcalloc(1, sizeof(struct ref_lock));
 	lock->lock_fd = -1;
 
@@ -2135,8 +2138,6 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,
 					 const unsigned char *old_sha1,
 					 int flags, int *type_p)
 {
-	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
-		return NULL;
 	return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
 }
 
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 40/44] refs.c: call lock_ref_sha1_basic directly from commit

[Ronnie Sahlberg]

Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic
directly from the commit function.

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

diff --git a/refs.c b/refs.c
index f63a356..564feb6 100644
--- a/refs.c
+++ b/refs.c
@@ -3511,12 +3511,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
-		update->lock = lock_any_ref_for_update(update->refname,
-						       (update->have_old ?
-							update->old_sha1 :
-							NULL),
-						       update->flags,
-						       &update->type);
+		update->lock = lock_ref_sha1_basic(update->refname,
+						   (update->have_old ?
+						    update->old_sha1 :
+						    NULL),
+						   update->flags,
+						   &update->type);
 		if (!update->lock) {
 			if (err)
 				strbuf_addf(err, "Cannot lock the ref '%s'.",
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 41/44] refs.c: add a new flag for transaction delete for refs we know are packed only

[Ronnie Sahlberg]

Add a new flag REF_ISPACKONLY that we can use in ref_transaction_delete.
This flag indicates that the ref does not exist as a loose ref andf only as
a packed ref. If this is the case we then change the commit code so that
we skip taking out a lock file and we skip calling delete_ref_loose.
Check for this flag and die(BUG:...) if used with _update or _create.

At the start of the transaction, before we even start locking any refs,
we add all such REF_ISPACKONLY refs to delnames so that we have a list of
all pack only refs that we will be deleting during this transaction.

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

diff --git a/refs.c b/refs.c
index 564feb6..c5d41bb 100644
--- a/refs.c
+++ b/refs.c
@@ -33,6 +33,10 @@ static inline int bad_ref_char(int ch)
  *  pruned.
  */
 #define REF_ISPRUNING	0x0100
+/** Deletion of a ref that only exists as a packed ref in which case we do not
+ *  need to lock the loose ref during the transaction.
+ */
+#define REF_ISPACKONLY	0x0200
 
 /*
  * Try to read one refname component from the front of refname.  Return
@@ -3366,6 +3370,9 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	if (transaction->status != REF_TRANSACTION_OPEN)
 		die("BUG: update on transaction that is not open");
 
+	if (flags & REF_ISPACKONLY)
+		die("BUG: REF_ISPACKONLY can not be used with updates");
+
 	update = add_update(transaction, refname);
 	hashcpy(update->new_sha1, new_sha1);
 	update->flags = flags;
@@ -3392,6 +3399,9 @@ int ref_transaction_create(struct ref_transaction *transaction,
 	if (transaction->status != REF_TRANSACTION_OPEN)
 		die("BUG: create on transaction that is not open");
 
+	if (flags & REF_ISPACKONLY)
+		die("BUG: REF_ISPACKONLY can not be used with creates");
+
 	update = add_update(transaction, refname);
 
 	hashcpy(update->new_sha1, new_sha1);
@@ -3507,10 +3517,20 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	if (ret)
 		goto cleanup;
 
+	for (i = 0; i < n; i++) {
+		struct ref_update *update = updates[i];
+
+		if (update->flags & REF_ISPACKONLY)
+			delnames[delnum++] = update->refname;
+	}
+
 	/* Acquire all locks while verifying old values */
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
+		if (update->flags & REF_ISPACKONLY)
+			continue;
+
 		update->lock = lock_ref_sha1_basic(update->refname,
 						   (update->have_old ?
 						    update->old_sha1 :
@@ -3548,6 +3568,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
+		if (update->flags & REF_ISPACKONLY)
+			continue;
+
 		if (update->lock) {
 			ret |= delete_ref_loose(update->lock, update->type,
 						err);
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 42/44] refs.c: pass a skip list to name_conflict_fn

[Ronnie Sahlberg]

Allow passing a list of refs to skip checking to name_conflict_fn.
There are some conditions where we want to allow a temporary conflict and skip
checking those refs. For example if we have a transaction that
1, guarantees that m is a packed refs and there is no loose ref for m
2, the transaction will delete m from the packed ref
3, the transaction will create conflicting m/m

For this case we want to be able to lock and create m/m since we know that the
conflict is only transient. I.e. the conflict will be automatically resolved
by the transaction when it deletes m.

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

diff --git a/refs.c b/refs.c
index c5d41bb..3967333 100644
--- a/refs.c
+++ b/refs.c
@@ -798,11 +798,19 @@ struct name_conflict_cb {
 	const char *refname;
 	const char *oldrefname;
 	const char *conflicting_refname;
+	const char **skip;
+	int skipnum;
 };
 
 static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
 {
 	struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
+	int i;
+	for(i = 0; i < data->skipnum; i++) {
+		if (!strcmp(entry->name, data->skip[i])) {
+			return 0;
+		}
+	}
 	if (data->oldrefname && !strcmp(data->oldrefname, entry->name))
 		return 0;
 	if (names_conflict(data->refname, entry->name)) {
@@ -817,15 +825,21 @@ static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
  * conflicting with the name of an existing reference in dir.  If
  * oldrefname is non-NULL, ignore potential conflicts with oldrefname
  * (e.g., because oldrefname is scheduled for deletion in the same
- * operation).
+ * operation). skip contains a list of refs we want to skip checking for
+ * conflicts with. Refs may be skipped due to us knowing that it will
+ * be deleted later during a transaction that deletes one reference and then
+ * creates a new conflicting reference. For example a rename from m to m/m.
  */
 static int is_refname_available(const char *refname, const char *oldrefname,
-				struct ref_dir *dir)
+				struct ref_dir *dir,
+				const char **skip, int skipnum)
 {
 	struct name_conflict_cb data;
 	data.refname = refname;
 	data.oldrefname = oldrefname;
 	data.conflicting_refname = NULL;
+	data.skip = skip;
+	data.skipnum = skipnum;
 
 	sort_ref_dir(dir);
 	if (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, &data)) {
@@ -2037,7 +2051,8 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 
 static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 					    const unsigned char *old_sha1,
-					    int flags, int *type_p)
+					    int flags, int *type_p,
+					    const char **skip, int skipnum)
 {
 	char *ref_file;
 	const char *orig_refname = refname;
@@ -2084,7 +2099,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	 * name is a proper prefix of our refname.
 	 */
 	if (missing &&
-	     !is_refname_available(refname, NULL, get_packed_refs(&ref_cache))) {
+	     !is_refname_available(refname, NULL,
+				   get_packed_refs(&ref_cache),
+				   skip, skipnum)) {
 		last_errno = ENOTDIR;
 		goto error_return;
 	}
@@ -2142,7 +2159,7 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,
 					 const unsigned char *old_sha1,
 					 int flags, int *type_p)
 {
-	return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
+	return lock_ref_sha1_basic(refname, old_sha1, flags, type_p, NULL, 0);
 }
 
 /*
@@ -2620,6 +2637,9 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
 	const char *symref = NULL;
 
+	if (!strcmp(oldrefname, newrefname))
+		return 0;
+
 	if (log && S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldrefname);
 
@@ -2630,10 +2650,12 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	if (!symref)
 		return error("refname %s not found", oldrefname);
 
-	if (!is_refname_available(newrefname, oldrefname, get_packed_refs(&ref_cache)))
+	if (!is_refname_available(newrefname, oldrefname,
+				  get_packed_refs(&ref_cache), NULL, 0))
 		return 1;
 
-	if (!is_refname_available(newrefname, oldrefname, get_loose_refs(&ref_cache)))
+	if (!is_refname_available(newrefname, oldrefname,
+				  get_loose_refs(&ref_cache), NULL, 0))
 		return 1;
 
 	if (log && rename(git_path("logs/%s", oldrefname), git_path(TMP_RENAMED_LOG)))
@@ -2666,7 +2688,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 
 	logmoved = log;
 
-	lock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL);
+	lock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL, NULL, 0);
 	if (!lock) {
 		error("unable to lock %s for update", newrefname);
 		goto rollback;
@@ -2681,7 +2703,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	return 0;
 
  rollback:
-	lock = lock_ref_sha1_basic(oldrefname, NULL, 0, NULL);
+	lock = lock_ref_sha1_basic(oldrefname, NULL, 0, NULL, NULL, 0);
 	if (!lock) {
 		error("unable to lock %s for rollback", oldrefname);
 		goto rollbacklog;
@@ -3536,7 +3558,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 						    update->old_sha1 :
 						    NULL),
 						   update->flags,
-						   &update->type);
+						   &update->type,
+						   delnames, delnum);
 		if (!update->lock) {
 			if (err)
 				strbuf_addf(err, "Cannot lock the ref '%s'.",
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 43/44] refs.c: make rename_ref use a transaction

[Ronnie Sahlberg]

Change rename_ref to use a single transaction to perform the ref rename.

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

diff --git a/refs.c b/refs.c
index 3967333..71e3059 100644
--- a/refs.c
+++ b/refs.c
@@ -2630,9 +2630,10 @@ static int rename_tmp_log(const char *newrefname)
 
 int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)
 {
-	unsigned char sha1[20], orig_sha1[20];
-	int flag = 0, logmoved = 0;
-	struct ref_lock *lock;
+	unsigned char sha1[20];
+	int flag = 0;
+	struct ref_transaction *transaction;
+	struct strbuf err = STRBUF_INIT;
 	struct stat loginfo;
 	int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
 	const char *symref = NULL;
@@ -2643,7 +2644,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	if (log && S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldrefname);
 
-	symref = resolve_ref_unsafe(oldrefname, orig_sha1, 1, &flag);
+	symref = resolve_ref_unsafe(oldrefname, sha1, 1, &flag);
 	if (flag & REF_ISSYMREF)
 		return error("refname %s is a symbolic ref, renaming it is not supported",
 			oldrefname);
@@ -2665,62 +2666,28 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	if (pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE))
 		return error("unable to pack refs");
 
-	if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
-		error("unable to delete old %s", oldrefname);
-		goto rollback;
-	}
-
-	if (!read_ref_full(newrefname, sha1, 1, NULL) &&
-	    delete_ref(newrefname, sha1, REF_NODEREF)) {
-		if (errno==EISDIR) {
-			if (remove_empty_directories(git_path("%s", newrefname))) {
-				error("Directory not empty: %s", newrefname);
-				goto rollback;
-			}
-		} else {
-			error("unable to delete existing %s", newrefname);
-			goto rollback;
-		}
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_delete(transaction, oldrefname, sha1,
+				   REF_NODEREF | REF_ISPACKONLY,
+				   1, NULL, &err) ||
+	    ref_transaction_update(transaction, newrefname, sha1,
+				   NULL, 0, 0, logmsg, &err) ||
+	    ref_transaction_commit(transaction, &err)) {
+		ref_transaction_free(transaction);
+		error("rename_ref failed: %s", err.buf);
+		strbuf_release(&err);
+		goto rollbacklog;
 	}
+	ref_transaction_free(transaction);
 
 	if (log && rename_tmp_log(newrefname))
-		goto rollback;
-
-	logmoved = log;
-
-	lock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL, NULL, 0);
-	if (!lock) {
-		error("unable to lock %s for update", newrefname);
-		goto rollback;
-	}
-	lock->force_write = 1;
-	hashcpy(lock->old_sha1, orig_sha1);
-	if (write_ref_sha1(lock, orig_sha1, logmsg)) {
-		error("unable to write current sha1 into %s", newrefname);
-		goto rollback;
-	}
-
-	return 0;
-
- rollback:
-	lock = lock_ref_sha1_basic(oldrefname, NULL, 0, NULL, NULL, 0);
-	if (!lock) {
-		error("unable to lock %s for rollback", oldrefname);
 		goto rollbacklog;
-	}
 
-	lock->force_write = 1;
-	flag = log_all_ref_updates;
-	log_all_ref_updates = 0;
-	if (write_ref_sha1(lock, orig_sha1, NULL))
-		error("unable to write current sha1 into %s", oldrefname);
-	log_all_ref_updates = flag;
+	return 0;
 
  rollbacklog:
-	if (logmoved && rename(git_path("logs/%s", newrefname), git_path("logs/%s", oldrefname)))
-		error("unable to restore logfile %s from %s: %s",
-			oldrefname, newrefname, strerror(errno));
-	if (!logmoved && log &&
+	if (log &&
 	    rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", oldrefname)))
 		error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s",
 			oldrefname, strerror(errno));
-- 
2.0.0.rc3.510.g20c254b

[PATCH v10 44/44] refs.c: remove forward declaration of write_ref_sha1

[Ronnie Sahlberg]

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

diff --git a/refs.c b/refs.c
index 71e3059..bf84306 100644
--- a/refs.c
+++ b/refs.c
@@ -260,8 +260,6 @@ struct ref_entry {
 };
 
 static void read_loose_refs(const char *dirname, struct ref_dir *dir);
-static int write_ref_sha1(struct ref_lock *lock,
-			  const unsigned char *sha1, const char *logmsg);
 
 static struct ref_dir *get_ref_dir(struct ref_entry *entry)
 {
-- 
2.0.0.rc3.510.g20c254b

Re: [PATCH v10 00/44] Use ref transactions for all ref updates

[Jonathan Nieder]

Ronnie Sahlberg wrote:

> This patch series can also be found at
> https://github.com/rsahlberg/git/tree/ref-transactions

I think the rerolls are coming too fast.  I've been using your
repository on github to check if I should not send any particular
comment because you've already fixed it, so the extra mail is not
needed for that purpose, at least.

In general, I think it's best to stick with one version of a series,
gather review comments on it, comment inline to solicit feedback about
approaches to particular problems, point reviewers to an up-to-date
branch which is more in flux, etc, and then only resend when reviewers
have had a chance to get through it to make it easier to keep track of
the review without getting lost.

Thanks for the quick work so far, by the way.  It's been fun watching
the API evolve.

My two cents,
Jonathan

Re: [PATCH v10 04/44] refs.c: add an err argument to repack_without_refs

[Michael Haggerty]

On 05/16/2014 07:36 PM, Ronnie Sahlberg wrote:
> Update repack_without_refs to take an err argument and update it if there
> is a failure. Pass the err variable from ref_transaction_commit to this
> function so that callers can print a meaningful error message if _commit
> fails due to a problem in repack_without_refs.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  cache.h    |  2 ++
>  lockfile.c | 21 ++++++++++++---------
>  refs.c     | 25 +++++++++++++++++++------
>  3 files changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/cache.h b/cache.h
> index 8c6cdc2..48548d6 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -559,6 +559,8 @@ struct lock_file {
>  #define LOCK_DIE_ON_ERROR 1
>  #define LOCK_NODEREF 2
>  extern int unable_to_lock_error(const char *path, int err);
> +extern void unable_to_lock_strbuf(const char *path, int err,
> +				  struct strbuf *buf);
>  extern NORETURN void unable_to_lock_index_die(const char *path, int err);
>  extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
>  extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
> diff --git a/lockfile.c b/lockfile.c
> index 8fbcb6a..9e04b43 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -157,33 +157,36 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>  	return lk->fd;
>  }
>  
> -static char *unable_to_lock_message(const char *path, int err)
> +void unable_to_lock_strbuf(const char *path, int err, struct strbuf *buf)
>  {
> -	struct strbuf buf = STRBUF_INIT;
>  
>  	if (err == EEXIST) {
> -		strbuf_addf(&buf, "Unable to create '%s.lock': %s.\n\n"
> +		strbuf_addf(buf, "Unable to create '%s.lock': %s.\n\n"
>  		    "If no other git process is currently running, this probably means a\n"
>  		    "git process crashed in this repository earlier. Make sure no other git\n"
>  		    "process is running and remove the file manually to continue.",
>  			    absolute_path(path), strerror(err));
>  	} else
> -		strbuf_addf(&buf, "Unable to create '%s.lock': %s",
> +		strbuf_addf(buf, "Unable to create '%s.lock': %s",
>  			    absolute_path(path), strerror(err));
> -	return strbuf_detach(&buf, NULL);
>  }
>  
>  int unable_to_lock_error(const char *path, int err)
>  {
> -	char *msg = unable_to_lock_message(path, err);
> -	error("%s", msg);
> -	free(msg);
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	unable_to_lock_strbuf(path, err, &buf);
> +	error("%s", buf.buf);
> +	strbuf_release(&buf);
>  	return -1;
>  }
>  
>  NORETURN void unable_to_lock_index_die(const char *path, int err)
>  {
> -	die("%s", unable_to_lock_message(path, err));
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	unable_to_lock_strbuf(path, err, &buf);
> +	die("%s", buf.buf);
>  }
>  
>  int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
> diff --git a/refs.c b/refs.c
> index 686b802..a470e51 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2208,6 +2208,7 @@ int commit_packed_refs(void)
>  	struct packed_ref_cache *packed_ref_cache =
>  		get_packed_ref_cache(&ref_cache);
>  	int error = 0;
> +	int save_errno;
>  
>  	if (!packed_ref_cache->lock)
>  		die("internal error: packed-refs not locked");
> @@ -2217,10 +2218,13 @@ int commit_packed_refs(void)
>  	do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
>  				 0, write_packed_entry_fn,
>  				 &packed_ref_cache->lock->fd);
> -	if (commit_lock_file(packed_ref_cache->lock))
> +	if (commit_lock_file(packed_ref_cache->lock)) {
> +		save_errno = errno;
>  		error = -1;
> +	}
>  	packed_ref_cache->lock = NULL;
>  	release_packed_ref_cache(packed_ref_cache);
> +	errno = save_errno;

This code involving save_errno looks like a bug fix orthogonal to the
rest of the patch.  It should at least be mentioned in the commit
message if not broken into a separate patch.

>  	return error;
>  }
>  
> @@ -2427,12 +2431,12 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
>  	return 0;
>  }
>  
> -static int repack_without_refs(const char **refnames, int n)
> +static int repack_without_refs(const char **refnames, int n, struct strbuf *err)
>  {
>  	struct ref_dir *packed;
>  	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
>  	struct string_list_item *ref_to_delete;
> -	int i, removed = 0;
> +	int i, ret, removed = 0;
>  
>  	/* Look for a packed ref */
>  	for (i = 0; i < n; i++)
> @@ -2444,6 +2448,11 @@ static int repack_without_refs(const char **refnames, int n)
>  		return 0; /* no refname exists in packed refs */
>  
>  	if (lock_packed_refs(0)) {
> +		if (err) {
> +			unable_to_lock_strbuf(git_path("packed-refs"), errno,
> +					      err);
> +			return 1;

error() returns -1, but here you have changed the return value to 1.  Is
there a reason for the change?

> +		}
>  		unable_to_lock_error(git_path("packed-refs"), errno);
>  		return error("cannot delete '%s' from packed refs", refnames[i]);
>  	}
> @@ -2470,12 +2479,16 @@ static int repack_without_refs(const char **refnames, int n)
>  	}
>  
>  	/* Write what remains */
> -	return commit_packed_refs();
> +	ret = commit_packed_refs();
> +	if (ret && err)
> +		strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
> +			    strerror(errno));
> +	return ret;
>  }
>  
>  static int repack_without_ref(const char *refname)
>  {
> -	return repack_without_refs(&refname, 1);
> +	return repack_without_refs(&refname, 1, NULL);
>  }
>  
>  static int delete_ref_loose(struct ref_lock *lock, int flag)
> @@ -3481,7 +3494,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  		}
>  	}
>  
> -	ret |= repack_without_refs(delnames, delnum);
> +	ret |= repack_without_refs(delnames, delnum, err);
>  	for (i = 0; i < delnum; i++)
>  		unlink_or_warn(git_path("logs/%s", delnames[i]));
>  	clear_loose_ref_cache(&ref_cache);
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v10 13/44] tag.c: use ref transactions when doing updates

[Michael Haggerty]

On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
> Change tag.c to use ref transactions for all ref updates.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  builtin/tag.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/tag.c b/builtin/tag.c
> index c6e8a71..b05f9a5 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -548,7 +548,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	struct strbuf ref = STRBUF_INIT;
>  	unsigned char object[20], prev[20];
>  	const char *object_ref, *tag;
> -	struct ref_lock *lock;
>  	struct create_tag_options opt;
>  	char *cleanup_arg = NULL;
>  	int annotate = 0, force = 0, lines = -1;
> @@ -556,6 +555,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	const char *msgfile = NULL, *keyid = NULL;
>  	struct msg_arg msg = { 0, STRBUF_INIT };
>  	struct commit_list *with_commit = NULL;
> +	struct ref_transaction *transaction;
> +	struct strbuf err = STRBUF_INIT;
>  	struct option options[] = {
>  		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
>  		{ OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
> @@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	if (annotate)
>  		create_tag(object, tag, &buf, &opt, prev, object);
>  
> -	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)
> -		die(_("%s: cannot update the ref"), ref.buf);
> +	transaction = ref_transaction_begin();
> +	if (!transaction ||
> +	    ref_transaction_update(transaction, ref.buf, object, prev,
> +				   0, !is_null_sha1(prev), &err) ||
> +	    ref_transaction_commit(transaction, NULL, &err))
> +		die("%s", err.buf);

If ref_transaction_begin() fails, then won't err still be empty?  (I
know it can't happen, and you know it can't happen, but should the
caller have to know that?)  It almost seems like ref_transaction_begin()
should have an err parameter, too.

It's kindof late for this question to pop into my head, but: have you
thought about embedding the err strbuf in the transaction object?  I
admit it would make the problem with ref_transaction_begin() even worse,
but maybe it would be a net win in terms of boilerplate?

>  	if (force && !is_null_sha1(prev) && hashcmp(prev, object))
>  		printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));
>  
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v10 14/44] replace.c: use the ref transaction functions for updates

[Michael Haggerty]

On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
> Update replace.c to use ref transactions for updates.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  builtin/replace.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 3da1bae..e8932cd 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -133,7 +133,8 @@ static int replace_object_sha1(const char *object_ref,
>  	unsigned char prev[20];
>  	enum object_type obj_type, repl_type;
>  	char ref[PATH_MAX];
> -	struct ref_lock *lock;
> +	struct ref_transaction *transaction;
> +	struct strbuf err = STRBUF_INIT;
>  
>  	if (snprintf(ref, sizeof(ref),
>  		     "refs/replace/%s",
> @@ -156,11 +157,12 @@ static int replace_object_sha1(const char *object_ref,
>  	else if (!force)
>  		die("replace ref '%s' already exists", 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)
> -		die("%s: cannot update the ref", ref);
> +	transaction = ref_transaction_begin();
> +	if (!transaction ||
> +	    ref_transaction_update(transaction, ref, repl, prev,
> +				   0, !is_null_sha1(prev), &err) ||
> +	    ref_transaction_commit(transaction, NULL, &err))
> +		die("%s", err.buf);

Same here: err can be empty if ref_transaction_begin() fails.  Please
check later patches for the same error.

>  
>  	return 0;
>  }
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v10 18/44] branch.c: use ref transaction for all ref updates

[Michael Haggerty]

On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
> Change create_branch to use a ref transaction when creating the new branch.
> ref_transaction_create will check that the ref does not already exist and fail
> otherwise meaning that we no longer need to keep a lock on the ref during the
> setup_tracking. This simplifies the code since we can now do the transaction
> in one single step.
> 
> If the forcing flag is false then use ref_transaction_create since this will
> fail if the ref already exist. Otherwise use ref_transaction_update.

s/exist/exists/

And the references to ref_transaction_create() in the commit message are
obsolete.

> 
> This also fixes a race condition in the old code where two concurrent
> create_branch could race since the lock_any_ref_for_update/write_ref_sha1
> did not protect against the ref already existing. I.e. one thread could end up
> overwriting a branch even if the forcing flag is false.
> 
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  branch.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/branch.c b/branch.c
> index 660097b..8bf7588 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -226,7 +226,6 @@ void create_branch(const char *head,
>  		   int force, int reflog, int clobber_head,
>  		   int quiet, enum branch_track track)
>  {
> -	struct ref_lock *lock = NULL;
>  	struct commit *commit;
>  	unsigned char sha1[20];
>  	char *real_ref, msg[PATH_MAX + 20];
> @@ -285,15 +284,6 @@ void create_branch(const char *head,
>  		die(_("Not a valid branch point: '%s'."), start_name);
>  	hashcpy(sha1, commit->object.sha1);
>  
> -	if (!dont_change_ref) {
> -		lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
> -		if (!lock)
> -			die_errno(_("Failed to lock ref for update"));
> -	}
> -
> -	if (reflog)
> -		log_all_ref_updates = 1;
> -
>  	if (forcing)
>  		snprintf(msg, sizeof msg, "branch: Reset to %s",
>  			 start_name);
> @@ -301,13 +291,24 @@ void create_branch(const char *head,
>  		snprintf(msg, sizeof msg, "branch: Created from %s",
>  			 start_name);
>  
> +	if (reflog)
> +		log_all_ref_updates = 1;
> +
> +	if (!dont_change_ref) {
> +		struct ref_transaction *transaction;
> +		struct strbuf err = STRBUF_INIT;
> +
> +		transaction = ref_transaction_begin();
> +		if (!transaction ||
> +		    ref_transaction_update(transaction, ref.buf, sha1,
> +					   null_sha1, 0, !forcing, &err) ||
> +		    ref_transaction_commit(transaction, msg, &err))
> +			die("%s", err.buf);
> +	}
> +
>  	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)
> -			die_errno(_("Failed to write ref"));
> -
>  	strbuf_release(&ref);
>  	free(real_ref);
>  }
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v10 22/44] fetch.c: clear errno before calling functions that might set it

[Michael Haggerty]

On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
> In s_update_ref there are two calls that when they fail we return an error
> based on the errno value. In particular we want to return a specific error
> if ENOTDIR happened. Both these functions do have failure modes where they
> may return an error without updating errno, in which case a previous and
> unrelated ENOTDIR may cause us to return the wrong error. Clear errno before
> calling any functions if we check errno afterwards.

If I understand correctly, this is a workaround for some other broken
functions that don't handle errno correctly.  Long-term, wouldn't it be
better to fix the other functions?  In other words, they should change
errno if an only if they return an error status, no?

Of course you are under no obligation to fix the universe, so this
change may be an expedient workaround anyway.  But if you go this route,
then I think a comment would be helpful to explain the unusual clearing
of errno.

Michael

> 
> Also skip initializing a static variable to 0. Statics live in .bss and
> are all automatically initialized to 0.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  builtin/fetch.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 55f457c..a93c893 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -44,7 +44,7 @@ static struct transport *gtransport;
>  static struct transport *gsecondary;
>  static const char *submodule_prefix = "";
>  static const char *recurse_submodules_default;
> -static int shown_url = 0;
> +static int shown_url;
>  
>  static int option_parse_recurse_submodules(const struct option *opt,
>  				   const char *arg, int unset)
> @@ -382,6 +382,8 @@ static int s_update_ref(const char *action,
>  	if (!rla)
>  		rla = default_rla.buf;
>  	snprintf(msg, sizeof(msg), "%s: %s", rla, action);
> +
> +	errno = 0;
>  	lock = lock_any_ref_for_update(ref->name,
>  				       check_old ? ref->old_sha1 : NULL,
>  				       0, NULL);
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v10 24/44] fetch.c: use a single ref transaction for all ref updates

[Michael Haggerty]

On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
> 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

s/cod/code/ ?

> 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

s/of/or/ ?
s/would/would fail,/ ?

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

s/occuring/occurring/

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

s/who/whole/

> 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

s/try/try to/
s/t$/to/

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

Thanks for the detailed explanation.  The change in behavior seems
reasonable to me.

> 
> 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 8cf70cd..5b0cc31 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;
> +static 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, NULL) ||
> -	    ref_transaction_commit(transaction, msg, NULL)) {
> -		ref_transaction_free(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, NULL))
> +		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"
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v10 24/44] fetch.c: use a single ref transaction for all ref updates

[Michael Haggerty]

On 05/17/2014 05:05 PM, Michael Haggerty wrote:
> On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
>> [...]
>> 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.
> 
> Thanks for the detailed explanation.  The change in behavior seems
> reasonable to me.

Wait, now I'm having second thoughts.

Won't the old code display *all* of the errors that occur, each time
proceeding nevertheless?  Whereas the new code displays only the *first*
error that would occur and then aborts?

This could be very frustrating if three references have problems, but
the user only finds out about one problem each time she runs "git
fetch".  Instead of

    git fetch # oops, ref1, ref2, and ref3 failed
    fix, fix, fix
    git fetch # :-)

she'd have to do

    git fetch # oops, ref1 failed
    fix ref1
    git fetch # oops, ref2 failed
    fix ref2
    git fetch # oops, ref3 failed
    fix ref3
    git fetch # :-( git I hate you

What kind of failures are we talking about here?  Are we talking about
errors like non-ff updates that happen routinely, or rarer/more serious
errors that would be expected to happen singly?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v10 25/44] receive-pack.c: use a reference transaction for updating the refs

[Michael Haggerty]

On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
> Wrap all the ref updates inside a transaction to make the update atomic.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  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..5534138 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;
> +static struct strbuf err = STRBUF_INIT;
> +static 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, &err))
> +			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"
> 

This patch is strange, because even if one ref_transaction_update() call
fails, subsequent updates are nevertheless also attempted, and the
ref_transaction_commit() is also attempted.  Is this an officially
sanctioned use of the ref_transactions API?  Should it be?  It might be
a way to give feedback to the user on multiple attempted reference
updates at once (i.e., address my comment about the last patch).

If this is sanctioned, then it might be appropriate for the transaction
to keep track of the fact that one or more reference updates failed, and
when *_commit() is called to fail the whole transaction.

In any case, I think it is important to document, as part of the API
docs, whether this is sanctioned or not, and if so, what exactly are its
semantics.

I've run out of time for today so I'm going to have to stop here.  FWIW
patches 01-23 looked OK aside from the comments that I have made.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v10 13/44] tag.c: use ref transactions when doing updates

[Ronnie Sahlberg]

On Sat, May 17, 2014 at 6:09 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
>> Change tag.c to use ref transactions for all ref updates.
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  builtin/tag.c | 14 ++++++++------
>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index c6e8a71..b05f9a5 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -548,7 +548,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>       struct strbuf ref = STRBUF_INIT;
>>       unsigned char object[20], prev[20];
>>       const char *object_ref, *tag;
>> -     struct ref_lock *lock;
>>       struct create_tag_options opt;
>>       char *cleanup_arg = NULL;
>>       int annotate = 0, force = 0, lines = -1;
>> @@ -556,6 +555,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>       const char *msgfile = NULL, *keyid = NULL;
>>       struct msg_arg msg = { 0, STRBUF_INIT };
>>       struct commit_list *with_commit = NULL;
>> +     struct ref_transaction *transaction;
>> +     struct strbuf err = STRBUF_INIT;
>>       struct option options[] = {
>>               OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
>>               { OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
>> @@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>       if (annotate)
>>               create_tag(object, tag, &buf, &opt, prev, object);
>>
>> -     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)
>> -             die(_("%s: cannot update the ref"), ref.buf);
>> +     transaction = ref_transaction_begin();
>> +     if (!transaction ||
>> +         ref_transaction_update(transaction, ref.buf, object, prev,
>> +                                0, !is_null_sha1(prev), &err) ||
>> +         ref_transaction_commit(transaction, NULL, &err))
>> +             die("%s", err.buf);
>
> If ref_transaction_begin() fails, then won't err still be empty?  (I
> know it can't happen, and you know it can't happen, but should the
> caller have to know that?)  It almost seems like ref_transaction_begin()
> should have an err parameter, too.

I add an err argument in the next series. I would prefer we let that
patch remain in the next series to
avoid unbounded growth of the current one.

Ok ?



>
> It's kindof late for this question to pop into my head, but: have you
> thought about embedding the err strbuf in the transaction object?  I
> admit it would make the problem with ref_transaction_begin() even worse,
> but maybe it would be a net win in terms of boilerplate?

I think it is more flexible to allow the caller to manage the lifetime
of the error buffer independently of the transaction.
It would allow a caller to free the transaction early but delay
printing the error message until later.

Or it could be used for a multi transaction caller to use a single err
buffer for all transactions and finally print
all errors in a single error() call at the end.

struct strbuf err = STRBUF_INIT;
... first transaction (... &err)...
... second transaction (... &err)...
... third transaction (... &err)...
error("%s", err.buf);



Similar to how rsync handles errors:
sahlberg@sahlberg1:~$ mkdir foo
sahlberg@sahlberg1:~$ touch foo/foo.1
sahlberg@sahlberg1:~$ touch foo/foo.2
sahlberg@sahlberg1:~$ mkdir bar
sahlberg@sahlberg1:~$ chmod 0500 bar
sahlberg@sahlberg1:~$ rsync -Pav foo/* bar
sending incremental file list
foo.1
           0 100%    0.00kB/s    0:00:00 (xfer#1, to-check=1/2)
foo.2
           0 100%    0.00kB/s    0:00:00 (xfer#2, to-check=0/2)
rsync: mkstemp "/usr/local/google/home/sahlberg/bar/.foo.1.K7dFIP"
failed: Permission denied (13)
rsync: mkstemp "/usr/local/google/home/sahlberg/bar/.foo.2.4WdRsW"
failed: Permission denied (13)

sent 136 bytes  received 50 bytes  372.00 bytes/sec
total size is 0  speedup is 0.00
rsync error: some files/attrs were not transferred (see previous
errors) (code 23) at main.c(1070) [sender=3.0.9]



>
>>       if (force && !is_null_sha1(prev) && hashcmp(prev, object))
>>               printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));
>>
>>
>
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/

Re: [PATCH v10 18/44] branch.c: use ref transaction for all ref updates

[Ronnie Sahlberg]

On Sat, May 17, 2014 at 6:33 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
>> Change create_branch to use a ref transaction when creating the new branch.
>> ref_transaction_create will check that the ref does not already exist and fail
>> otherwise meaning that we no longer need to keep a lock on the ref during the
>> setup_tracking. This simplifies the code since we can now do the transaction
>> in one single step.
>>
>> If the forcing flag is false then use ref_transaction_create since this will
>> fail if the ref already exist. Otherwise use ref_transaction_update.
>
> s/exist/exists/
>
> And the references to ref_transaction_create() in the commit message are
> obsolete.
>

Thanks. Fixed typo and deleted obsolete text.

>>
>> This also fixes a race condition in the old code where two concurrent
>> create_branch could race since the lock_any_ref_for_update/write_ref_sha1
>> did not protect against the ref already existing. I.e. one thread could end up
>> overwriting a branch even if the forcing flag is false.
>>
>> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  branch.c | 29 +++++++++++++++--------------
>>  1 file changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/branch.c b/branch.c
>> index 660097b..8bf7588 100644
>> --- a/branch.c
>> +++ b/branch.c
>> @@ -226,7 +226,6 @@ void create_branch(const char *head,
>>                  int force, int reflog, int clobber_head,
>>                  int quiet, enum branch_track track)
>>  {
>> -     struct ref_lock *lock = NULL;
>>       struct commit *commit;
>>       unsigned char sha1[20];
>>       char *real_ref, msg[PATH_MAX + 20];
>> @@ -285,15 +284,6 @@ void create_branch(const char *head,
>>               die(_("Not a valid branch point: '%s'."), start_name);
>>       hashcpy(sha1, commit->object.sha1);
>>
>> -     if (!dont_change_ref) {
>> -             lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
>> -             if (!lock)
>> -                     die_errno(_("Failed to lock ref for update"));
>> -     }
>> -
>> -     if (reflog)
>> -             log_all_ref_updates = 1;
>> -
>>       if (forcing)
>>               snprintf(msg, sizeof msg, "branch: Reset to %s",
>>                        start_name);
>> @@ -301,13 +291,24 @@ void create_branch(const char *head,
>>               snprintf(msg, sizeof msg, "branch: Created from %s",
>>                        start_name);
>>
>> +     if (reflog)
>> +             log_all_ref_updates = 1;
>> +
>> +     if (!dont_change_ref) {
>> +             struct ref_transaction *transaction;
>> +             struct strbuf err = STRBUF_INIT;
>> +
>> +             transaction = ref_transaction_begin();
>> +             if (!transaction ||
>> +                 ref_transaction_update(transaction, ref.buf, sha1,
>> +                                        null_sha1, 0, !forcing, &err) ||
>> +                 ref_transaction_commit(transaction, msg, &err))
>> +                     die("%s", err.buf);
>> +     }
>> +
>>       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)
>> -                     die_errno(_("Failed to write ref"));
>> -
>>       strbuf_release(&ref);
>>       free(real_ref);
>>  }
>>
>
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/

Re: [PATCH v10 13/44] tag.c: use ref transactions when doing updates

[Ronnie Sahlberg]

I have moved the patch to add &err to ref_transaction_begin to the
current patch series.

Please see
https://github.com/rsahlberg/git/tree/ref-transactions

On Mon, May 19, 2014 at 10:16 AM, Ronnie Sahlberg <sahlberg@google.com> wrote:
> On Sat, May 17, 2014 at 6:09 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
>>> Change tag.c to use ref transactions for all ref updates.
>>>
>>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>>> ---
>>>  builtin/tag.c | 14 ++++++++------
>>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/builtin/tag.c b/builtin/tag.c
>>> index c6e8a71..b05f9a5 100644
>>> --- a/builtin/tag.c
>>> +++ b/builtin/tag.c
>>> @@ -548,7 +548,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>>       struct strbuf ref = STRBUF_INIT;
>>>       unsigned char object[20], prev[20];
>>>       const char *object_ref, *tag;
>>> -     struct ref_lock *lock;
>>>       struct create_tag_options opt;
>>>       char *cleanup_arg = NULL;
>>>       int annotate = 0, force = 0, lines = -1;
>>> @@ -556,6 +555,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>>       const char *msgfile = NULL, *keyid = NULL;
>>>       struct msg_arg msg = { 0, STRBUF_INIT };
>>>       struct commit_list *with_commit = NULL;
>>> +     struct ref_transaction *transaction;
>>> +     struct strbuf err = STRBUF_INIT;
>>>       struct option options[] = {
>>>               OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
>>>               { OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
>>> @@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>>       if (annotate)
>>>               create_tag(object, tag, &buf, &opt, prev, object);
>>>
>>> -     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)
>>> -             die(_("%s: cannot update the ref"), ref.buf);
>>> +     transaction = ref_transaction_begin();
>>> +     if (!transaction ||
>>> +         ref_transaction_update(transaction, ref.buf, object, prev,
>>> +                                0, !is_null_sha1(prev), &err) ||
>>> +         ref_transaction_commit(transaction, NULL, &err))
>>> +             die("%s", err.buf);
>>
>> If ref_transaction_begin() fails, then won't err still be empty?  (I
>> know it can't happen, and you know it can't happen, but should the
>> caller have to know that?)  It almost seems like ref_transaction_begin()
>> should have an err parameter, too.
>
> I add an err argument in the next series. I would prefer we let that
> patch remain in the next series to
> avoid unbounded growth of the current one.
>
> Ok ?
>
>
>
>>
>> It's kindof late for this question to pop into my head, but: have you
>> thought about embedding the err strbuf in the transaction object?  I
>> admit it would make the problem with ref_transaction_begin() even worse,
>> but maybe it would be a net win in terms of boilerplate?
>
> I think it is more flexible to allow the caller to manage the lifetime
> of the error buffer independently of the transaction.
> It would allow a caller to free the transaction early but delay
> printing the error message until later.
>
> Or it could be used for a multi transaction caller to use a single err
> buffer for all transactions and finally print
> all errors in a single error() call at the end.
>
> struct strbuf err = STRBUF_INIT;
> ... first transaction (... &err)...
> ... second transaction (... &err)...
> ... third transaction (... &err)...
> error("%s", err.buf);
>
>
>
> Similar to how rsync handles errors:
> sahlberg@sahlberg1:~$ mkdir foo
> sahlberg@sahlberg1:~$ touch foo/foo.1
> sahlberg@sahlberg1:~$ touch foo/foo.2
> sahlberg@sahlberg1:~$ mkdir bar
> sahlberg@sahlberg1:~$ chmod 0500 bar
> sahlberg@sahlberg1:~$ rsync -Pav foo/* bar
> sending incremental file list
> foo.1
>            0 100%    0.00kB/s    0:00:00 (xfer#1, to-check=1/2)
> foo.2
>            0 100%    0.00kB/s    0:00:00 (xfer#2, to-check=0/2)
> rsync: mkstemp "/usr/local/google/home/sahlberg/bar/.foo.1.K7dFIP"
> failed: Permission denied (13)
> rsync: mkstemp "/usr/local/google/home/sahlberg/bar/.foo.2.4WdRsW"
> failed: Permission denied (13)
>
> sent 136 bytes  received 50 bytes  372.00 bytes/sec
> total size is 0  speedup is 0.00
> rsync error: some files/attrs were not transferred (see previous
> errors) (code 23) at main.c(1070) [sender=3.0.9]
>
>
>
>>
>>>       if (force && !is_null_sha1(prev) && hashcmp(prev, object))
>>>               printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));
>>>
>>>
>>
>>
>> --
>> Michael Haggerty
>> mhagger@alum.mit.edu
>> http://softwareswirl.blogspot.com/

Re: [PATCH v10 14/44] replace.c: use the ref transaction functions for updates

[Ronnie Sahlberg]

I have moved the patch to add an err argument to this branch and
update all patches that adds
calls to ref_Transaction_begin to the new signature.

Please see
https://github.com/rsahlberg/git/tree/ref-transactions

Thanks!


On Sat, May 17, 2014 at 6:14 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
>> Update replace.c to use ref transactions for updates.
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  builtin/replace.c | 14 ++++++++------
>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/builtin/replace.c b/builtin/replace.c
>> index 3da1bae..e8932cd 100644
>> --- a/builtin/replace.c
>> +++ b/builtin/replace.c
>> @@ -133,7 +133,8 @@ static int replace_object_sha1(const char *object_ref,
>>       unsigned char prev[20];
>>       enum object_type obj_type, repl_type;
>>       char ref[PATH_MAX];
>> -     struct ref_lock *lock;
>> +     struct ref_transaction *transaction;
>> +     struct strbuf err = STRBUF_INIT;
>>
>>       if (snprintf(ref, sizeof(ref),
>>                    "refs/replace/%s",
>> @@ -156,11 +157,12 @@ static int replace_object_sha1(const char *object_ref,
>>       else if (!force)
>>               die("replace ref '%s' already exists", 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)
>> -             die("%s: cannot update the ref", ref);
>> +     transaction = ref_transaction_begin();
>> +     if (!transaction ||
>> +         ref_transaction_update(transaction, ref, repl, prev,
>> +                                0, !is_null_sha1(prev), &err) ||
>> +         ref_transaction_commit(transaction, NULL, &err))
>> +             die("%s", err.buf);
>
> Same here: err can be empty if ref_transaction_begin() fails.  Please
> check later patches for the same error.
>
>>
>>       return 0;
>>  }
>>
>
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/

Re: [PATCH v10 25/44] receive-pack.c: use a reference transaction for updating the refs

[Ronnie Sahlberg]

On Sat, May 17, 2014 at 8:35 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
>> Wrap all the ref updates inside a transaction to make the update atomic.
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  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..5534138 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;
>> +static struct strbuf err = STRBUF_INIT;
>> +static 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, &err))
>> +                     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"
>>
>
> This patch is strange, because even if one ref_transaction_update() call
> fails, subsequent updates are nevertheless also attempted, and the
> ref_transaction_commit() is also attempted.  Is this an officially
> sanctioned use of the ref_transactions API?  Should it be?

I think it should be supported. Because otherwise, unless you have the
entire transaction localized in a single block you would end up having
to check and recheck the return value everywhere.

It makes the API much easier to use if you can continue calling
transaction functions even after the transaction has failed. If the
transaction has already failed then _update/_create/_delete will do
nothing except return an error.

If _commit is called on a failed transaction then the commit will fail
with an error
and do nothing.


I think it is convenient, and it allows things like :

struct ref_transaction *transaction;
void foo()
{
   ...
   ref_transaction_update(transaction, ... , &err);
   ...
}


transaction = ref_transaction_begin(&err);
... doing stuff and call things that eventually ends up calling foo,
possible multiple times ...
ret = ref_transaction_commit(transaction, &err);


In foo() we ignore checking the return value so we will not see/care
if it failed. IF it fails however it will mark the transaction as
failed and update &err. (Note that this can not yet happen since
_update can not really fail, ever, but the next series will introduce
_update failures when we move locking there.)

Instead we can depend on that IF _update failed, then the call to
_commit will fail too and &err is already updated so we can defer any
checking for errors until _commit time.

This will make the API much more convenient for use cases where you
begin/commit the transaction in one function but the calls to
_update/_delete/_create are somewhere else, possible many function
calls away.
It does not mean that a caller must ignore the return value from
ref_transaction_update, just that the caller can do so and defer
checking for errors until later when it would be more convenient.


Please see current:
https://github.com/rsahlberg/git/tree/ref-transactions
and patch:
refs.c: add transaction.status and track OPEN/CLOSED/ERROR


  It might be
> a way to give feedback to the user on multiple attempted reference
> updates at once (i.e., address my comment about the last patch).
>
> If this is sanctioned, then it might be appropriate for the transaction
> to keep track of the fact that one or more reference updates failed, and
> when *_commit() is called to fail the whole transaction.

Yes. I updated refs.h to indicate that you can continue using
_update/_create/_delete even if a previous call has failed but that
these calls will now just return an error.

This does mean that on the first update that fails for a ref we fail
the transaction and abort any further _update calls to fail
immediately so if there would be additional refs that would fail we
would not log this. I think this is what we want to do since once we
have had a ref update fail it would be really hard to determine if the
next failure was just a side effect of the first failure or not.


>
> In any case, I think it is important to document, as part of the API
> docs, whether this is sanctioned or not, and if so, what exactly are its
> semantics.
>
> I've run out of time for today so I'm going to have to stop here.  FWIW
> patches 01-23 looked OK aside from the comments that I have made.
>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/

Re: [PATCH v10 25/44] receive-pack.c: use a reference transaction for updating the refs

[Michael Haggerty]

On 05/19/2014 09:02 PM, Ronnie Sahlberg wrote:
> On Sat, May 17, 2014 at 8:35 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
>>> Wrap all the ref updates inside a transaction to make the update atomic.
>>>
>>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>>> ---
>>>  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..5534138 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;
>>> +static struct strbuf err = STRBUF_INIT;
>>> +static 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, &err))
>>> +                     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"
>>>
>>
>> This patch is strange, because even if one ref_transaction_update() call
>> fails, subsequent updates are nevertheless also attempted, and the
>> ref_transaction_commit() is also attempted.  Is this an officially
>> sanctioned use of the ref_transactions API?  Should it be?
> 
> I think it should be supported. Because otherwise, unless you have the
> entire transaction localized in a single block you would end up having
> to check and recheck the return value everywhere.
> 
> It makes the API much easier to use if you can continue calling
> transaction functions even after the transaction has failed. If the
> transaction has already failed then _update/_create/_delete will do
> nothing except return an error.

I agree that it is convenient to be able to keep calling functions
blindly without worrying that an earlier function call already failed.
As you point out below, this allows a style of use of the API where you
choose *not* to check intermediate results at all, and only check
whether the final commit succeeds.

Meanwhile, remember the awkwardness in your patch that made fetch use a
transaction to update the references.  In that case, the switch to using
a transaction had the big disadvantage that the user would only get an
error message for the first failing reference update.

When I combine these two lines of thought, it suggests to me that we
could do a better job of supporting *both* use cases.  What if the
transaction object contained not an err strbuf but a string_list?  If an
error occurs while building up the transaction, a message would be added
to the string list and the function would return an error status.  The
caller can monitor errors while it is building up the transaction and
abort immediately if it wants, or it can ignore the return values and
let the error messages accumulate in the string list.  When the caller
attempts the commit, it would notice that the transaction failed, and at
that time the caller could emit *all* of the accumulated error messages
by reading them out of the string list; e.g.,

    Error fetching from $REMOTE:   <- this is generated by caller
        $ERR[0]    <- these come from the error string list,
        $ERR[1]       printed with indentation by caller
        $ERR[2]
        $ERR[3]

This style would have another advantage: we might have some back ends
for which transactions have a high overhead.  Such a back end would
probably choose not to do any checks while the transaction is being
built up, e.g., to avoid a round-trip to a database.  When commit() is
called, it would learn about all of the errors at once.  (1) It would
need a way to return all of the errors to the caller.  (2) It would be
nice for the caller to be able to treat such a back end the same as it
treats a back end that is able to report errors immediately.  It seems
to me that having a way to report multiple errors at the same time would
solve both problems nicely.

> If _commit is called on a failed transaction then the commit will fail
> with an error and do nothing.
> 
> I think it is convenient, and it allows things like :
> 
> struct ref_transaction *transaction;
> void foo()
> {
>    ...
>    ref_transaction_update(transaction, ... , &err);
>    ...
> }
> 
> 
> transaction = ref_transaction_begin(&err);
> ... doing stuff and call things that eventually ends up calling foo,
> possible multiple times ...
> ret = ref_transaction_commit(transaction, &err);
> 
> 
> In foo() we ignore checking the return value so we will not see/care
> if it failed. IF it fails however it will mark the transaction as
> failed and update &err. (Note that this can not yet happen since
> _update can not really fail, ever, but the next series will introduce
> _update failures when we move locking there.)
> 
> Instead we can depend on that IF _update failed, then the call to
> _commit will fail too and &err is already updated so we can defer any
> checking for errors until _commit time.
> 
> This will make the API much more convenient for use cases where you
> begin/commit the transaction in one function but the calls to
> _update/_delete/_create are somewhere else, possible many function
> calls away.
> It does not mean that a caller must ignore the return value from
> ref_transaction_update, just that the caller can do so and defer
> checking for errors until later when it would be more convenient.
> 
> 
> Please see current:
> https://github.com/rsahlberg/git/tree/ref-transactions
> and patch:
> refs.c: add transaction.status and track OPEN/CLOSED/ERROR
> 
> 
>   It might be
>> a way to give feedback to the user on multiple attempted reference
>> updates at once (i.e., address my comment about the last patch).
>>
>> If this is sanctioned, then it might be appropriate for the transaction
>> to keep track of the fact that one or more reference updates failed, and
>> when *_commit() is called to fail the whole transaction.
> 
> Yes. I updated refs.h to indicate that you can continue using
> _update/_create/_delete even if a previous call has failed but that
> these calls will now just return an error.
> 
> This does mean that on the first update that fails for a ref we fail
> the transaction and abort any further _update calls to fail
> immediately so if there would be additional refs that would fail we
> would not log this. I think this is what we want to do since once we
> have had a ref update fail it would be really hard to determine if the
> next failure was just a side effect of the first failure or not.

It could be that errors cascade, for example if I update reference R to
value A, then (maybe a few steps later) verify that R has value A.  If
the update fails, then the verify will also fail.  But it would be silly
for our code to generate such a sequence of operations.  And if that
sequence of operations came from the user (e.g., from "git update-ref
--stdin"), it would be pretty churlish of the user to complain that we
report two errors.  So I don't think your "side effect" worry is a
problem in practice.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v10 25/44] receive-pack.c: use a reference transaction for updating the refs

[Ronnie Sahlberg]

On Fri, May 23, 2014 at 6:49 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 05/19/2014 09:02 PM, Ronnie Sahlberg wrote:
>> On Sat, May 17, 2014 at 8:35 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
>>>> Wrap all the ref updates inside a transaction to make the update atomic.
>>>>
>>>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>>>> ---
>>>>  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..5534138 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;
>>>> +static struct strbuf err = STRBUF_INIT;
>>>> +static 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, &err))
>>>> +                     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"
>>>>
>>>
>>> This patch is strange, because even if one ref_transaction_update() call
>>> fails, subsequent updates are nevertheless also attempted, and the
>>> ref_transaction_commit() is also attempted.  Is this an officially
>>> sanctioned use of the ref_transactions API?  Should it be?
>>
>> I think it should be supported. Because otherwise, unless you have the
>> entire transaction localized in a single block you would end up having
>> to check and recheck the return value everywhere.
>>
>> It makes the API much easier to use if you can continue calling
>> transaction functions even after the transaction has failed. If the
>> transaction has already failed then _update/_create/_delete will do
>> nothing except return an error.
>
> I agree that it is convenient to be able to keep calling functions
> blindly without worrying that an earlier function call already failed.
> As you point out below, this allows a style of use of the API where you
> choose *not* to check intermediate results at all, and only check
> whether the final commit succeeds.
>
> Meanwhile, remember the awkwardness in your patch that made fetch use a
> transaction to update the references.  In that case, the switch to using
> a transaction had the big disadvantage that the user would only get an
> error message for the first failing reference update.
>
> When I combine these two lines of thought, it suggests to me that we
> could do a better job of supporting *both* use cases.  What if the
> transaction object contained not an err strbuf but a string_list?  If an
> error occurs while building up the transaction, a message would be added
> to the string list and the function would return an error status.  The
> caller can monitor errors while it is building up the transaction and
> abort immediately if it wants, or it can ignore the return values and
> let the error messages accumulate in the string list.  When the caller
> attempts the commit, it would notice that the transaction failed, and at
> that time the caller could emit *all* of the accumulated error messages
> by reading them out of the string list; e.g.,
>
>     Error fetching from $REMOTE:   <- this is generated by caller
>         $ERR[0]    <- these come from the error string list,
>         $ERR[1]       printed with indentation by caller
>         $ERR[2]
>         $ERR[3]
>
> This style would have another advantage: we might have some back ends
> for which transactions have a high overhead.  Such a back end would
> probably choose not to do any checks while the transaction is being
> built up, e.g., to avoid a round-trip to a database.  When commit() is
> called, it would learn about all of the errors at once.  (1) It would
> need a way to return all of the errors to the caller.  (2) It would be
> nice for the caller to be able to treat such a back end the same as it
> treats a back end that is able to report errors immediately.  It seems
> to me that having a way to report multiple errors at the same time would
> solve both problems nicely.

Inretesting.
That would mean changing all functions to take a string_list provided
by the caller instead of a strbuf.
And then have _update/_create/_delete do actual work instead of
bailing out after the first error.

Users that want to check for error and log after each call to
_update/_create/_delete could do so and
just use the last entry added to the string list or otherwise they
could just wait until _commit time and if it fails log
all the strings.


>
>> If _commit is called on a failed transaction then the commit will fail
>> with an error and do nothing.
>>
>> I think it is convenient, and it allows things like :
>>
>> struct ref_transaction *transaction;
>> void foo()
>> {
>>    ...
>>    ref_transaction_update(transaction, ... , &err);
>>    ...
>> }
>>
>>
>> transaction = ref_transaction_begin(&err);
>> ... doing stuff and call things that eventually ends up calling foo,
>> possible multiple times ...
>> ret = ref_transaction_commit(transaction, &err);
>>
>>
>> In foo() we ignore checking the return value so we will not see/care
>> if it failed. IF it fails however it will mark the transaction as
>> failed and update &err. (Note that this can not yet happen since
>> _update can not really fail, ever, but the next series will introduce
>> _update failures when we move locking there.)
>>
>> Instead we can depend on that IF _update failed, then the call to
>> _commit will fail too and &err is already updated so we can defer any
>> checking for errors until _commit time.
>>
>> This will make the API much more convenient for use cases where you
>> begin/commit the transaction in one function but the calls to
>> _update/_delete/_create are somewhere else, possible many function
>> calls away.
>> It does not mean that a caller must ignore the return value from
>> ref_transaction_update, just that the caller can do so and defer
>> checking for errors until later when it would be more convenient.
>>
>>
>> Please see current:
>> https://github.com/rsahlberg/git/tree/ref-transactions
>> and patch:
>> refs.c: add transaction.status and track OPEN/CLOSED/ERROR
>>
>>
>>   It might be
>>> a way to give feedback to the user on multiple attempted reference
>>> updates at once (i.e., address my comment about the last patch).
>>>
>>> If this is sanctioned, then it might be appropriate for the transaction
>>> to keep track of the fact that one or more reference updates failed, and
>>> when *_commit() is called to fail the whole transaction.
>>
>> Yes. I updated refs.h to indicate that you can continue using
>> _update/_create/_delete even if a previous call has failed but that
>> these calls will now just return an error.
>>
>> This does mean that on the first update that fails for a ref we fail
>> the transaction and abort any further _update calls to fail
>> immediately so if there would be additional refs that would fail we
>> would not log this. I think this is what we want to do since once we
>> have had a ref update fail it would be really hard to determine if the
>> next failure was just a side effect of the first failure or not.
>
> It could be that errors cascade, for example if I update reference R to
> value A, then (maybe a few steps later) verify that R has value A.  If
> the update fails, then the verify will also fail.  But it would be silly
> for our code to generate such a sequence of operations.  And if that
> sequence of operations came from the user (e.g., from "git update-ref
> --stdin"), it would be pretty churlish of the user to complain that we
> report two errors.  So I don't think your "side effect" worry is a
> problem in practice.

If we accept that users could do bad things causing cascading errors
to be logged
and that the user is to blame for the cascading errors in the logged
output, then I
am fine with doing these changes you suggest.

I will try this change today and see what it looks like.

Re: [PATCH v10 25/44] receive-pack.c: use a reference transaction for updating the refs

[Michael Haggerty]

On 05/23/2014 06:14 PM, Ronnie Sahlberg wrote:
> On Fri, May 23, 2014 at 6:49 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> [...]
>> When I combine these two lines of thought, it suggests to me that we
>> could do a better job of supporting *both* use cases.  What if the
>> transaction object contained not an err strbuf but a string_list?  If an
>> error occurs while building up the transaction, a message would be added
>> to the string list and the function would return an error status.  The
>> caller can monitor errors while it is building up the transaction and
>> abort immediately if it wants, or it can ignore the return values and
>> let the error messages accumulate in the string list.  When the caller
>> attempts the commit, it would notice that the transaction failed, and at
>> that time the caller could emit *all* of the accumulated error messages
>> by reading them out of the string list; e.g.,
>>
>>     Error fetching from $REMOTE:   <- this is generated by caller
>>         $ERR[0]    <- these come from the error string list,
>>         $ERR[1]       printed with indentation by caller
>>         $ERR[2]
>>         $ERR[3]
>>
>> This style would have another advantage: we might have some back ends
>> for which transactions have a high overhead.  Such a back end would
>> probably choose not to do any checks while the transaction is being
>> built up, e.g., to avoid a round-trip to a database.  When commit() is
>> called, it would learn about all of the errors at once.  (1) It would
>> need a way to return all of the errors to the caller.  (2) It would be
>> nice for the caller to be able to treat such a back end the same as it
>> treats a back end that is able to report errors immediately.  It seems
>> to me that having a way to report multiple errors at the same time would
>> solve both problems nicely.
> 
> Inretesting.
> That would mean changing all functions to take a string_list provided
> by the caller instead of a strbuf.
> And then have _update/_create/_delete do actual work instead of
> bailing out after the first error.
> 
> Users that want to check for error and log after each call to
> _update/_create/_delete could do so and
> just use the last entry added to the string list or otherwise they
> could just wait until _commit time and if it fails log
> all the strings.

I still think we should consider storing the err string_list within the
transaction object; otherwise it's annoying to have to pass that
parameter around everywhere.  And if there were also a policy that any
caller that checks and reports any error should report *all* of the
accumulated errors and abort the transaction, then we don't have to
worry about error messages being output multiple times or zero times.

It might be nice to have a printf-style helper function like

    ref_transaction_perror(transaction, fmt, ...)

(or maybe ref_transaction_die()) that outputs the accumulated errors
with msg as a header (like my example output above), to make error
handling easy and uniform.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v10 22/44] fetch.c: clear errno before calling functions that might set it

[Ronnie Sahlberg]

On Sat, May 17, 2014 at 7:56 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
>> In s_update_ref there are two calls that when they fail we return an error
>> based on the errno value. In particular we want to return a specific error
>> if ENOTDIR happened. Both these functions do have failure modes where they
>> may return an error without updating errno, in which case a previous and
>> unrelated ENOTDIR may cause us to return the wrong error. Clear errno before
>> calling any functions if we check errno afterwards.
>
> If I understand correctly, this is a workaround for some other broken
> functions that don't handle errno correctly.  Long-term, wouldn't it be
> better to fix the other functions?  In other words, they should change
> errno if an only if they return an error status, no?

Yeah,  but this patch is gone now.
Longer term I think we should get rid of passing errno around.
errno is best to only be checked immediately after a failed system
call and never else.
(otherwise you end up with a hairy forest of save_errno variables.)

>
> Of course you are under no obligation to fix the universe, so this
> change may be an expedient workaround anyway.  But if you go this route,
> then I think a comment would be helpful to explain the unusual clearing
> of errno.
>
> Michael
>
>>
>> Also skip initializing a static variable to 0. Statics live in .bss and
>> are all automatically initialized to 0.
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  builtin/fetch.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 55f457c..a93c893 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -44,7 +44,7 @@ static struct transport *gtransport;
>>  static struct transport *gsecondary;
>>  static const char *submodule_prefix = "";
>>  static const char *recurse_submodules_default;
>> -static int shown_url = 0;
>> +static int shown_url;
>>
>>  static int option_parse_recurse_submodules(const struct option *opt,
>>                                  const char *arg, int unset)
>> @@ -382,6 +382,8 @@ static int s_update_ref(const char *action,
>>       if (!rla)
>>               rla = default_rla.buf;
>>       snprintf(msg, sizeof(msg), "%s: %s", rla, action);
>> +
>> +     errno = 0;
>>       lock = lock_any_ref_for_update(ref->name,
>>                                      check_old ? ref->old_sha1 : NULL,
>>                                      0, NULL);
>>
>
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/

Re: [PATCH v10 04/44] refs.c: add an err argument to repack_without_refs

[Ronnie Sahlberg]

On Sat, May 17, 2014 at 5:40 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 05/16/2014 07:36 PM, Ronnie Sahlberg wrote:
>> Update repack_without_refs to take an err argument and update it if there
>> is a failure. Pass the err variable from ref_transaction_commit to this
>> function so that callers can print a meaningful error message if _commit
>> fails due to a problem in repack_without_refs.
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  cache.h    |  2 ++
>>  lockfile.c | 21 ++++++++++++---------
>>  refs.c     | 25 +++++++++++++++++++------
>>  3 files changed, 33 insertions(+), 15 deletions(-)
>>
>> diff --git a/cache.h b/cache.h
>> index 8c6cdc2..48548d6 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -559,6 +559,8 @@ struct lock_file {
>>  #define LOCK_DIE_ON_ERROR 1
>>  #define LOCK_NODEREF 2
>>  extern int unable_to_lock_error(const char *path, int err);
>> +extern void unable_to_lock_strbuf(const char *path, int err,
>> +                               struct strbuf *buf);
>>  extern NORETURN void unable_to_lock_index_die(const char *path, int err);
>>  extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
>>  extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
>> diff --git a/lockfile.c b/lockfile.c
>> index 8fbcb6a..9e04b43 100644
>> --- a/lockfile.c
>> +++ b/lockfile.c
>> @@ -157,33 +157,36 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>>       return lk->fd;
>>  }
>>
>> -static char *unable_to_lock_message(const char *path, int err)
>> +void unable_to_lock_strbuf(const char *path, int err, struct strbuf *buf)
>>  {
>> -     struct strbuf buf = STRBUF_INIT;
>>
>>       if (err == EEXIST) {
>> -             strbuf_addf(&buf, "Unable to create '%s.lock': %s.\n\n"
>> +             strbuf_addf(buf, "Unable to create '%s.lock': %s.\n\n"
>>                   "If no other git process is currently running, this probably means a\n"
>>                   "git process crashed in this repository earlier. Make sure no other git\n"
>>                   "process is running and remove the file manually to continue.",
>>                           absolute_path(path), strerror(err));
>>       } else
>> -             strbuf_addf(&buf, "Unable to create '%s.lock': %s",
>> +             strbuf_addf(buf, "Unable to create '%s.lock': %s",
>>                           absolute_path(path), strerror(err));
>> -     return strbuf_detach(&buf, NULL);
>>  }
>>
>>  int unable_to_lock_error(const char *path, int err)
>>  {
>> -     char *msg = unable_to_lock_message(path, err);
>> -     error("%s", msg);
>> -     free(msg);
>> +     struct strbuf buf = STRBUF_INIT;
>> +
>> +     unable_to_lock_strbuf(path, err, &buf);
>> +     error("%s", buf.buf);
>> +     strbuf_release(&buf);
>>       return -1;
>>  }
>>
>>  NORETURN void unable_to_lock_index_die(const char *path, int err)
>>  {
>> -     die("%s", unable_to_lock_message(path, err));
>> +     struct strbuf buf = STRBUF_INIT;
>> +
>> +     unable_to_lock_strbuf(path, err, &buf);
>> +     die("%s", buf.buf);
>>  }
>>
>>  int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
>> diff --git a/refs.c b/refs.c
>> index 686b802..a470e51 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2208,6 +2208,7 @@ int commit_packed_refs(void)
>>       struct packed_ref_cache *packed_ref_cache =
>>               get_packed_ref_cache(&ref_cache);
>>       int error = 0;
>> +     int save_errno;
>>
>>       if (!packed_ref_cache->lock)
>>               die("internal error: packed-refs not locked");
>> @@ -2217,10 +2218,13 @@ int commit_packed_refs(void)
>>       do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
>>                                0, write_packed_entry_fn,
>>                                &packed_ref_cache->lock->fd);
>> -     if (commit_lock_file(packed_ref_cache->lock))
>> +     if (commit_lock_file(packed_ref_cache->lock)) {
>> +             save_errno = errno;
>>               error = -1;
>> +     }
>>       packed_ref_cache->lock = NULL;
>>       release_packed_ref_cache(packed_ref_cache);
>> +     errno = save_errno;
>
> This code involving save_errno looks like a bug fix orthogonal to the
> rest of the patch.  It should at least be mentioned in the commit
> message if not broken into a separate patch.

Text updated.

>
>>       return error;
>>  }
>>
>> @@ -2427,12 +2431,12 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
>>       return 0;
>>  }
>>
>> -static int repack_without_refs(const char **refnames, int n)
>> +static int repack_without_refs(const char **refnames, int n, struct strbuf *err)
>>  {
>>       struct ref_dir *packed;
>>       struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
>>       struct string_list_item *ref_to_delete;
>> -     int i, removed = 0;
>> +     int i, ret, removed = 0;
>>
>>       /* Look for a packed ref */
>>       for (i = 0; i < n; i++)
>> @@ -2444,6 +2448,11 @@ static int repack_without_refs(const char **refnames, int n)
>>               return 0; /* no refname exists in packed refs */
>>
>>       if (lock_packed_refs(0)) {
>> +             if (err) {
>> +                     unable_to_lock_strbuf(git_path("packed-refs"), errno,
>> +                                           err);
>> +                     return 1;
>
> error() returns -1, but here you have changed the return value to 1.  Is
> there a reason for the change?

Fixed.

>
>> +             }
>>               unable_to_lock_error(git_path("packed-refs"), errno);
>>               return error("cannot delete '%s' from packed refs", refnames[i]);
>>       }
>> @@ -2470,12 +2479,16 @@ static int repack_without_refs(const char **refnames, int n)
>>       }
>>
>>       /* Write what remains */
>> -     return commit_packed_refs();
>> +     ret = commit_packed_refs();
>> +     if (ret && err)
>> +             strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
>> +                         strerror(errno));
>> +     return ret;
>>  }
>>
>>  static int repack_without_ref(const char *refname)
>>  {
>> -     return repack_without_refs(&refname, 1);
>> +     return repack_without_refs(&refname, 1, NULL);
>>  }
>>
>>  static int delete_ref_loose(struct ref_lock *lock, int flag)
>> @@ -3481,7 +3494,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>>               }
>>       }
>>
>> -     ret |= repack_without_refs(delnames, delnum);
>> +     ret |= repack_without_refs(delnames, delnum, err);
>>       for (i = 0; i < delnum; i++)
>>               unlink_or_warn(git_path("logs/%s", delnames[i]));
>>       clear_loose_ref_cache(&ref_cache);
>>
>
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/

Re: [PATCH v10 25/44] receive-pack.c: use a reference transaction for updating the refs

[Ronnie Sahlberg]

On Fri, May 23, 2014 at 2:02 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 05/23/2014 06:14 PM, Ronnie Sahlberg wrote:
>> On Fri, May 23, 2014 at 6:49 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> [...]
>>> When I combine these two lines of thought, it suggests to me that we
>>> could do a better job of supporting *both* use cases.  What if the
>>> transaction object contained not an err strbuf but a string_list?  If an
>>> error occurs while building up the transaction, a message would be added
>>> to the string list and the function would return an error status.  The
>>> caller can monitor errors while it is building up the transaction and
>>> abort immediately if it wants, or it can ignore the return values and
>>> let the error messages accumulate in the string list.  When the caller
>>> attempts the commit, it would notice that the transaction failed, and at
>>> that time the caller could emit *all* of the accumulated error messages
>>> by reading them out of the string list; e.g.,
>>>
>>>     Error fetching from $REMOTE:   <- this is generated by caller
>>>         $ERR[0]    <- these come from the error string list,
>>>         $ERR[1]       printed with indentation by caller
>>>         $ERR[2]
>>>         $ERR[3]
>>>
>>> This style would have another advantage: we might have some back ends
>>> for which transactions have a high overhead.  Such a back end would
>>> probably choose not to do any checks while the transaction is being
>>> built up, e.g., to avoid a round-trip to a database.  When commit() is
>>> called, it would learn about all of the errors at once.  (1) It would
>>> need a way to return all of the errors to the caller.  (2) It would be
>>> nice for the caller to be able to treat such a back end the same as it
>>> treats a back end that is able to report errors immediately.  It seems
>>> to me that having a way to report multiple errors at the same time would
>>> solve both problems nicely.
>>
>> Inretesting.
>> That would mean changing all functions to take a string_list provided
>> by the caller instead of a strbuf.
>> And then have _update/_create/_delete do actual work instead of
>> bailing out after the first error.
>>
>> Users that want to check for error and log after each call to
>> _update/_create/_delete could do so and
>> just use the last entry added to the string list or otherwise they
>> could just wait until _commit time and if it fails log
>> all the strings.
>
> I still think we should consider storing the err string_list within the
> transaction object; otherwise it's annoying to have to pass that
> parameter around everywhere.  And if there were also a policy that any
> caller that checks and reports any error should report *all* of the
> accumulated errors and abort the transaction, then we don't have to
> worry about error messages being output multiple times or zero times.
>
> It might be nice to have a printf-style helper function like
>
>     ref_transaction_perror(transaction, fmt, ...)
>
> (or maybe ref_transaction_die()) that outputs the accumulated errors
> with msg as a header (like my example output above), to make error
> handling easy and uniform.

We can add this later once we get the basic transaction stuff in.
It will make it easier to experiment with different types of error
reporting then.


>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/