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

[Jonathan Nieder <jrnieder@gmail.com>]

Ronnie Sahlberg wrote:

> This patch series is based on next and expands on the transaction API.

Thanks.  Will pick up in v8 where I left off with v6.

Applies with just one minor conflict on top of a merge of
mh/ref-transaction, rs/ref-update-check-errors-early, and
rs/reflog-exists.  Here's an interdiff against version 6 for those
following along.

> 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

diff --git a/builtin/commit.c b/builtin/commit.c
index 0f4e1fc..07ccc97 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1684,7 +1684,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 				   0, !!current_head, sb.buf) ||
 	    ref_transaction_commit(transaction, &err)) {
 		rollback_index_files();
-		die(_("HEAD: cannot update ref: %s"), err.buf);
+		die("%s", err.buf);
 	}
 	ref_transaction_free(transaction);
 
diff --git a/builtin/replace.c b/builtin/replace.c
index 47c360c..952b589 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -163,7 +163,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,
 	    ref_transaction_update(transaction, ref, repl, prev,
 				   0, !is_null_sha1(prev), NULL) ||
 	    ref_transaction_commit(transaction, &err))
-		die(_("%s: failed to replace ref: %s"), ref, err.buf);
+		die("%s: failed to replace ref: %s", ref, err.buf);
 
 	ref_transaction_free(transaction);
 	return 0;
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index c5aff92..bd7e96f 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -228,7 +228,7 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)
 
 	if (ref_transaction_create(transaction, refname, new_sha1,
 				   update_flags, msg))
-		die("failed transaction create for %s", refname);
+		die("cannot create ref '%s'", refname);
 
 	update_flags = 0;
 	free(refname);
diff --git a/refs.c b/refs.c
index 302a2b3..ed93b75 100644
--- a/refs.c
+++ b/refs.c
@@ -29,6 +29,15 @@ static inline int bad_ref_char(int ch)
 	return 0;
 }
 
+/** Used as a flag to ref_transaction_delete when a loose ref is beeing
+ *  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
  * the length of the component found, or -1 if the component is not
@@ -2447,12 +2456,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++)
@@ -2465,6 +2474,9 @@ static int repack_without_refs(const char **refnames, int n)
 
 	if (lock_packed_refs(0)) {
 		unable_to_lock_error(git_path("packed-refs"), errno);
+		if (err)
+			strbuf_addf(err, "cannot delete '%s' from packed refs",
+				    refnames[i]);
 		return error("cannot delete '%s' from packed refs", refnames[i]);
 	}
 	packed = get_packed_refs(&ref_cache);
@@ -2490,20 +2502,28 @@ 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");
+	return ret;
 }
 
-static int delete_ref_loose(struct ref_lock *lock, int flag)
+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_warn(lock->lk->filename);
 		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;
 }
@@ -3483,7 +3503,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 						   delnames, delnum);
 		if (!update->lock) {
 			if (err)
-				strbuf_addf(err ,"Cannot lock the ref '%s'.",
+				strbuf_addf(err, "Cannot lock the ref '%s'.",
 					    update->refname);
 			ret = 1;
 			goto cleanup;
@@ -3516,13 +3536,14 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			continue;
 
 		if (update->lock) {
-			ret |= delete_ref_loose(update->lock, update->type);
+			ret |= delete_ref_loose(update->lock, update->type,
+						err);
 			if (!(update->flags & REF_ISPRUNING))
 				delnames[delnum++] = update->lock->ref_name;
 		}
 	}
 
-	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);
diff --git a/refs.h b/refs.h
index 03b4a7e..0e6d416 100644
--- a/refs.h
+++ b/refs.h
@@ -134,10 +134,6 @@ extern int peel_ref(const char *refname, unsigned char *sha1);
 
 /** Locks any ref (for 'HEAD' type refs). */
 #define REF_NODEREF	0x01
-/** Deleting a loose ref during prune */
-#define REF_ISPRUNING	0x02
-/** Deletion of a ref that only exists as a packed ref */
-#define REF_ISPACKONLY	0x04
 extern struct ref_lock *lock_any_ref_for_update(const char *refname,
 						const unsigned char *old_sha1,
 						int flags, int *type_p);
@@ -240,6 +236,9 @@ void ref_transaction_rollback(struct ref_transaction *transaction);
  * 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.
  */
 int ref_transaction_update(struct ref_transaction *transaction,
 			   const char *refname,
@@ -252,6 +251,9 @@ 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.
  */
 int ref_transaction_create(struct ref_transaction *transaction,
 			   const char *refname,
@@ -262,6 +264,9 @@ 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.
  */
 int ref_transaction_delete(struct ref_transaction *transaction,
 			   const char *refname,
@@ -272,7 +277,7 @@ 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. If err is non-NULL we will add an error string to it to explain
- * why the transaction failed.
+ * why the transaction failed. The string does not end in newline.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   struct strbuf *err);