[PATCH v3 00/14] ref-transactions-reflog

[PATCH v3 00/14] ref-transactions-reflog

[Ronnie Sahlberg]

These patches can also be found at:
https://github.com/rsahlberg/git/tree/ref-transactions-reflog
This series is based on, and applies ontop of, the previous 48 patch long
ref-transaction series that is now in origin/pu.

 
This series introduces support for reflog updates to the transaction framework
and ends up re-factoring reflog.c to use a single atomic transaction for
updating both the ref and its reflog.

With these changes we also reduce the number of places where we build and write
a reflog entry to a single function which makes maintenance easier.
Several functions that act on reflogs can now be made private to refs.c
since we no longer need to export them.


This is version 3:
 - Update and rebased ontop of the current ref-transactions series.

Ronnie Sahlberg (14):
  refs.c make ref_transaction_create a wrapper to ref_transaction_update
  refs.c: make ref_transaction_delete a wrapper for
    ref_transaction_update
  refs.c: rename the transaction functions
  refs.c: add a new update_type field to ref_update
  refs.c: add a function to append a reflog entry to a fd
  lockfile.c: make hold_lock_file_for_append preserve meaningful errno
  refs.c: add a transaction function to append a reflog entry
  refs.c: add a flag to allow reflog updates to truncate the log
  refs.c: only write reflog update if msg is non-NULL
  refs.c: allow multiple reflog updates during a single transaction
  reflog.c: use a reflog transaction when writing during expire
  refs.c: rename log_ref_setup to create_reflog
  refs.c: make unlock_ref/close_ref/commit_ref static
  refs.c: remove lock_any_ref_for_update

 branch.c               |  11 +-
 builtin/checkout.c     |   8 +-
 builtin/commit.c       |  14 +-
 builtin/fetch.c        |  12 +-
 builtin/receive-pack.c |  14 +-
 builtin/reflog.c       |  84 +++++------
 builtin/replace.c      |  10 +-
 builtin/tag.c          |  10 +-
 builtin/update-ref.c   |  22 +--
 copy.c                 |  20 ++-
 fast-import.c          |  22 +--
 lockfile.c             |   7 +-
 refs.c                 | 377 ++++++++++++++++++++++++++++++++++---------------
 refs.h                 | 109 +++++++-------
 sequencer.c            |  12 +-
 walker.c               |  16 +--
 16 files changed, 448 insertions(+), 300 deletions(-)

-- 
2.0.0.467.g08c0633

[PATCH v3 01/14] refs.c make ref_transaction_create a wrapper to ref_transaction_update

[Ronnie Sahlberg]

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

diff --git a/refs.c b/refs.c
index dfbf003..a9f91ab 100644
--- a/refs.c
+++ b/refs.c
@@ -3487,23 +3487,14 @@ int ref_transaction_create(struct ref_transaction *transaction,
 			   int flags, const char *msg,
 			   struct strbuf *err)
 {
-	struct ref_update *update;
-
 	if (transaction->state != REF_TRANSACTION_OPEN)
 		die("BUG: create called for transaction that is not open");
 
 	if (!new_sha1 || is_null_sha1(new_sha1))
 		die("BUG: create ref with null new_sha1");
 
-	update = add_update(transaction, refname);
-
-	hashcpy(update->new_sha1, new_sha1);
-	hashclr(update->old_sha1);
-	update->flags = flags;
-	update->have_old = 1;
-	if (msg)
-		update->msg = xstrdup(msg);
-	return 0;
+	return ref_transaction_update(transaction, refname, new_sha1,
+				      null_sha1, flags, 1, msg, err);
 }
 
 int ref_transaction_delete(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index db463d0..495740d 100644
--- a/refs.h
+++ b/refs.h
@@ -283,9 +283,10 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
 /*
  * Add a reference update to transaction.  new_sha1 is the value that
  * the reference should have after the update, or zeros if it should
- * 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.
+ * be deleted.  If have_old is true and old_sha is not the null_sha1
+ * then the previous value of the ref must match or the update will fail.
+ * If have_old is true and old_sha1 is the null_sha1 then the ref must not
+ * already exist and a new ref will be created with new_sha1.
  * 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.
-- 
2.0.0.467.g08c0633

[PATCH v3 02/14] refs.c: make ref_transaction_delete a wrapper for ref_transaction_update

[Ronnie Sahlberg]

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

diff --git a/refs.c b/refs.c
index a9f91ab..0eace70 100644
--- a/refs.c
+++ b/refs.c
@@ -3503,24 +3503,17 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 			   int flags, int have_old, const char *msg,
 			   struct strbuf *err)
 {
-	struct ref_update *update;
-
 	if (transaction->state != REF_TRANSACTION_OPEN)
 		die("BUG: delete called for transaction that is not open");
 
 	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);
-	}
-	if (msg)
-		update->msg = xstrdup(msg);
-	return 0;
+	if (have_old && is_null_sha1(old_sha1))
+		die("BUG: have_old is true but old_sha1 is null_sha1");
+
+	return ref_transaction_update(transaction, refname, null_sha1,
+				      old_sha1, flags, have_old, msg, err);
 }
 
 int update_ref(const char *action, const char *refname,
diff --git a/refs.h b/refs.h
index 495740d..20bb152 100644
--- a/refs.h
+++ b/refs.h
@@ -282,7 +282,7 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
 
 /*
  * Add a reference update to transaction.  new_sha1 is the value that
- * the reference should have after the update, or zeros if it should
+ * the reference should have after the update, or null_sha1 if it should
  * be deleted.  If have_old is true and old_sha is not the null_sha1
  * then the previous value of the ref must match or the update will fail.
  * If have_old is true and old_sha1 is the null_sha1 then the ref must not
-- 
2.0.0.467.g08c0633

[PATCH v3 03/14] refs.c: rename the transaction functions

[Ronnie Sahlberg]

Rename the transaction functions. Remove the leading ref_ from the
names and append _sha1 to the names for functions that create/delete/
update sha1 refs.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 branch.c               | 11 ++++---
 builtin/commit.c       | 14 ++++-----
 builtin/fetch.c        | 12 ++++----
 builtin/receive-pack.c | 14 ++++-----
 builtin/replace.c      | 10 +++---
 builtin/tag.c          | 10 +++---
 builtin/update-ref.c   | 22 +++++++-------
 fast-import.c          | 22 +++++++-------
 refs.c                 | 82 +++++++++++++++++++++++++-------------------------
 refs.h                 | 56 +++++++++++++++++-----------------
 sequencer.c            | 12 ++++----
 walker.c               | 16 +++++-----
 12 files changed, 141 insertions(+), 140 deletions(-)

diff --git a/branch.c b/branch.c
index e0439af..6fa6d78 100644
--- a/branch.c
+++ b/branch.c
@@ -298,13 +298,14 @@ void create_branch(const char *head,
 		struct ref_transaction *transaction;
 		struct strbuf err = STRBUF_INIT;
 
-		transaction = ref_transaction_begin(&err);
+		transaction = transaction_begin(&err);
 		if (!transaction ||
-		    ref_transaction_update(transaction, ref.buf, sha1,
-					   null_sha1, 0, !forcing, msg, &err) ||
-		    ref_transaction_commit(transaction, &err))
+		    transaction_update_sha1(transaction, ref.buf, sha1,
+					    null_sha1, 0, !forcing, msg,
+					    &err) ||
+		    transaction_commit(transaction, &err))
 			die("%s", err.buf);
-		ref_transaction_free(transaction);
+		transaction_free(transaction);
 	}
 
 	if (real_ref && track)
diff --git a/builtin/commit.c b/builtin/commit.c
index c499826..bf8d85a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1762,17 +1762,17 @@ 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);
 
-	transaction = ref_transaction_begin(&err);
+	transaction = transaction_begin(&err);
 	if (!transaction ||
-	    ref_transaction_update(transaction, "HEAD", sha1,
-				   current_head ?
-				   current_head->object.sha1 : NULL,
-				   0, !!current_head, sb.buf, &err) ||
-	    ref_transaction_commit(transaction, &err)) {
+	    transaction_update_sha1(transaction, "HEAD", sha1,
+				    current_head ?
+				    current_head->object.sha1 : NULL,
+				    0, !!current_head, sb.buf, &err) ||
+	    transaction_commit(transaction, &err)) {
 		rollback_index_files();
 		die("%s", err.buf);
 	}
-	ref_transaction_free(transaction);
+	transaction_free(transaction);
 
 	unlink(git_path("CHERRY_PICK_HEAD"));
 	unlink(git_path("REVERT_HEAD"));
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 52f1ebc..baf7929 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -385,22 +385,22 @@ static int s_update_ref(const char *action,
 		rla = default_rla.buf;
 	snprintf(msg, sizeof(msg), "%s: %s", rla, action);
 
-	transaction = ref_transaction_begin(&err);
+	transaction = transaction_begin(&err);
 	if (!transaction ||
-	    ref_transaction_update(transaction, ref->name, ref->new_sha1,
-				   ref->old_sha1, 0, check_old, msg, &err))
+	    transaction_update_sha1(transaction, ref->name, ref->new_sha1,
+				    ref->old_sha1, 0, check_old, msg, &err))
 		goto fail;
 
-	ret = ref_transaction_commit(transaction, &err);
+	ret = transaction_commit(transaction, &err);
 	if (ret == UPDATE_REFS_NAME_CONFLICT)
 		df_conflict = 1;
 	if (ret)
 		goto fail;
 
-	ref_transaction_free(transaction);
+	transaction_free(transaction);
 	return 0;
 fail:
-	ref_transaction_free(transaction);
+	transaction_free(transaction);
 	error("%s", err.buf);
 	strbuf_release(&err);
 	return df_conflict ? STORE_REF_ERROR_DF_CONFLICT
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 0ed1ddd..dcd1862 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -582,20 +582,20 @@ static char *update(struct command *cmd, struct shallow_info *si)
 		    update_shallow_ref(cmd, si))
 			return xstrdup("shallow error");
 
-		transaction = ref_transaction_begin(&err);
+		transaction = transaction_begin(&err);
 		if (!transaction ||
-		    ref_transaction_update(transaction, namespaced_name,
-					   new_sha1, old_sha1, 0, 1, "push",
-					   &err) ||
-		    ref_transaction_commit(transaction, &err)) {
+		    transaction_update_sha1(transaction, namespaced_name,
+					    new_sha1, old_sha1, 0, 1, "push",
+					    &err) ||
+		    transaction_commit(transaction, &err)) {
 			char *str = strbuf_detach(&err, NULL);
-			ref_transaction_free(transaction);
+			transaction_free(transaction);
 
 			rp_error("%s", str);
 			return str;
 		}
 
-		ref_transaction_free(transaction);
+		transaction_free(transaction);
 		strbuf_release(&err);
 		return NULL; /* good */
 	}
diff --git a/builtin/replace.c b/builtin/replace.c
index 8e699f2..9e3f7e0 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -167,14 +167,14 @@ static int replace_object_sha1(const char *object_ref,
 
 	check_ref_valid(object, prev, ref, sizeof(ref), force);
 
-	transaction = ref_transaction_begin(&err);
+	transaction = transaction_begin(&err);
 	if (!transaction ||
-	    ref_transaction_update(transaction, ref, repl, prev,
-				   0, !is_null_sha1(prev), NULL, &err) ||
-	    ref_transaction_commit(transaction, &err))
+	    transaction_update_sha1(transaction, ref, repl, prev,
+				    0, !is_null_sha1(prev), NULL, &err) ||
+	    transaction_commit(transaction, &err))
 		die("%s", err.buf);
  
-	ref_transaction_free(transaction);
+	transaction_free(transaction);
 	return 0;
 }
 
diff --git a/builtin/tag.c b/builtin/tag.c
index 74af63e..4525c69 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -702,13 +702,13 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (annotate)
 		create_tag(object, tag, &buf, &opt, prev, object);
 
-	transaction = ref_transaction_begin(&err);
+	transaction = transaction_begin(&err);
 	if (!transaction ||
-	    ref_transaction_update(transaction, ref.buf, object, prev,
-				   0, !is_null_sha1(prev), NULL, &err) ||
-	    ref_transaction_commit(transaction, &err))
+	    transaction_update_sha1(transaction, ref.buf, object, prev,
+				    0, !is_null_sha1(prev), NULL, &err) ||
+	    transaction_commit(transaction, &err))
 		die("%s", err.buf);
-	ref_transaction_free(transaction);
+	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 28b478a..d62871d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -199,8 +199,8 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)
 	if (*next != line_termination)
 		die("update %s: extra input: %s", refname, next);
 
-	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-				   update_flags, have_old, msg, &err))
+	if (transaction_update_sha1(transaction, refname, new_sha1, old_sha1,
+				    update_flags, have_old, msg, &err))
 		die("%s", err.buf);
 
 	update_flags = 0;
@@ -227,8 +227,8 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)
 	if (*next != line_termination)
 		die("create %s: extra input: %s", refname, next);
 
-	if (ref_transaction_create(transaction, refname, new_sha1,
-				   update_flags, msg, &err))
+	if (transaction_create_sha1(transaction, refname, new_sha1,
+				    update_flags, msg, &err))
 		die("%s", err.buf);
 
 	update_flags = 0;
@@ -259,8 +259,8 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)
 	if (*next != line_termination)
 		die("delete %s: extra input: %s", refname, next);
 
-	if (ref_transaction_delete(transaction, refname, old_sha1,
-				   update_flags, have_old, msg, &err))
+	if (transaction_delete_sha1(transaction, refname, old_sha1,
+				    update_flags, have_old, msg, &err))
 		die("%s", err.buf);
 
 	update_flags = 0;
@@ -292,8 +292,8 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)
 	if (*next != line_termination)
 		die("verify %s: extra input: %s", refname, next);
 
-	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-				   update_flags, have_old, msg, &err))
+	if (transaction_update_sha1(transaction, refname, new_sha1, old_sha1,
+				    update_flags, have_old, msg, &err))
 		die("%s", err.buf);
 
 	update_flags = 0;
@@ -366,15 +366,15 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		die("Refusing to perform update with empty message.");
 
 	if (read_stdin) {
-		transaction = ref_transaction_begin(&err);
+		transaction = transaction_begin(&err);
 		if (delete || no_deref || argc > 0)
 			usage_with_options(git_update_ref_usage, options);
 		if (end_null)
 			line_termination = '\0';
 		update_refs_stdin();
-		if (ref_transaction_commit(transaction, &err))
+		if (transaction_commit(transaction, &err))
 			die("%s", err.buf);
-		ref_transaction_free(transaction);
+		transaction_free(transaction);
 		return 0;
 	}
 
diff --git a/fast-import.c b/fast-import.c
index f5ccf4e..778b6ea 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1705,17 +1705,17 @@ static int update_branch(struct branch *b)
 			return -1;
 		}
 	}
-	transaction = ref_transaction_begin(&err);
+	transaction = transaction_begin(&err);
 	if (!transaction ||
-	    ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
-				   0, 1, msg, &err) ||
-	    ref_transaction_commit(transaction, &err)) {
-		ref_transaction_free(transaction);
+	    transaction_update_sha1(transaction, b->name, b->sha1, old_sha1,
+				    0, 1, msg, &err) ||
+	    transaction_commit(transaction, &err)) {
+		transaction_free(transaction);
 		error("%s", err.buf);
 		strbuf_release(&err);
 		return -1;
 	}
-	ref_transaction_free(transaction);
+	transaction_free(transaction);
 	return 0;
 }
 
@@ -1738,7 +1738,7 @@ static void dump_tags(void)
 	struct strbuf err = STRBUF_INIT;
 	struct ref_transaction *transaction;
 
-	transaction = ref_transaction_begin(&err);
+	transaction = transaction_begin(&err);
 	if (!transaction) {
 		failure |= error("%s", err.buf);
 		goto cleanup;
@@ -1747,17 +1747,17 @@ static void dump_tags(void)
 		strbuf_reset(&ref_name);
 		strbuf_addf(&ref_name, "refs/tags/%s", t->name);
 
-		if (ref_transaction_update(transaction, ref_name.buf, t->sha1,
-					   NULL, 0, 0, msg, &err)) {
+		if (transaction_update_sha1(transaction, ref_name.buf, t->sha1,
+					    NULL, 0, 0, msg, &err)) {
 			failure |= error("%s", err.buf);
 			goto cleanup;
 		}
 	}
-	if (ref_transaction_commit(transaction, &err))
+	if (transaction_commit(transaction, &err))
 		failure |= error("%s", err.buf);
 
  cleanup:
-	ref_transaction_free(transaction);
+	transaction_free(transaction);
 	strbuf_release(&ref_name);
 	strbuf_release(&err);
 }
diff --git a/refs.c b/refs.c
index 0eace70..4e3d4c3 100644
--- a/refs.c
+++ b/refs.c
@@ -25,7 +25,7 @@ static unsigned char refname_disposition[256] = {
 };
 
 /*
- * Used as a flag to ref_transaction_delete when a loose ref is being
+ * Used as a flag to transaction_delete_sha1 when a loose ref is being
  * pruned.
  */
 #define REF_ISPRUNING	0x0100
@@ -2402,17 +2402,17 @@ static void prune_ref(struct ref_to_prune *r)
 	if (check_refname_format(r->name + 5, 0))
 		return;
 
-	transaction = ref_transaction_begin(&err);
+	transaction = transaction_begin(&err);
 	if (!transaction ||
-	    ref_transaction_delete(transaction, r->name, r->sha1,
-				   REF_ISPRUNING, 1, NULL, &err) ||
-	    ref_transaction_commit(transaction, &err)) {
-		ref_transaction_free(transaction);
+	    transaction_delete_sha1(transaction, r->name, r->sha1,
+				    REF_ISPRUNING, 1, NULL, &err) ||
+	    transaction_commit(transaction, &err)) {
+		transaction_free(transaction);
 		error("%s", err.buf);
 		strbuf_release(&err);
 		return;
  	}
-	ref_transaction_free(transaction);
+	transaction_free(transaction);
 	try_remove_empty_parents(r->name);
 }
 
@@ -2598,17 +2598,17 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
 
-	transaction = ref_transaction_begin(&err);
+	transaction = transaction_begin(&err);
 	if (!transaction ||
-	    ref_transaction_delete(transaction, refname, sha1, delopt,
-				   sha1 && !is_null_sha1(sha1), NULL, &err) ||
-	    ref_transaction_commit(transaction, &err)) {
+	    transaction_delete_sha1(transaction, refname, sha1, delopt,
+				    sha1 && !is_null_sha1(sha1), NULL, &err) ||
+	    transaction_commit(transaction, &err)) {
 		error("%s", err.buf);
-		ref_transaction_free(transaction);
+		transaction_free(transaction);
 		strbuf_release(&err);
 		return 1;
 	}
-	ref_transaction_free(transaction);
+	transaction_free(transaction);
 	return 0;
 }
 
@@ -3423,12 +3423,12 @@ struct ref_transaction {
 	int status;
 };
 
-struct ref_transaction *ref_transaction_begin(struct strbuf *err)
+struct ref_transaction *transaction_begin(struct strbuf *err)
 {
 	return xcalloc(1, sizeof(struct ref_transaction));
 }
 
-void ref_transaction_free(struct ref_transaction *transaction)
+void transaction_free(struct ref_transaction *transaction)
 {
 	int i;
 
@@ -3455,12 +3455,12 @@ static struct ref_update *add_update(struct ref_transaction *transaction,
 	return update;
 }
 
-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, const char *msg,
-			   struct strbuf *err)
+int transaction_update_sha1(struct ref_transaction *transaction,
+			    const char *refname,
+			    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;
 
@@ -3481,11 +3481,11 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	return 0;
 }
 
-int ref_transaction_create(struct ref_transaction *transaction,
-			   const char *refname,
-			   const unsigned char *new_sha1,
-			   int flags, const char *msg,
-			   struct strbuf *err)
+int transaction_create_sha1(struct ref_transaction *transaction,
+			    const char *refname,
+			    const unsigned char *new_sha1,
+			    int flags, const char *msg,
+			    struct strbuf *err)
 {
 	if (transaction->state != REF_TRANSACTION_OPEN)
 		die("BUG: create called for transaction that is not open");
@@ -3493,15 +3493,15 @@ int ref_transaction_create(struct ref_transaction *transaction,
 	if (!new_sha1 || is_null_sha1(new_sha1))
 		die("BUG: create ref with null new_sha1");
 
-	return ref_transaction_update(transaction, refname, new_sha1,
-				      null_sha1, flags, 1, msg, err);
+	return transaction_update_sha1(transaction, refname, new_sha1,
+				       null_sha1, flags, 1, msg, err);
 }
 
-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)
+int transaction_delete_sha1(struct ref_transaction *transaction,
+			    const char *refname,
+			    const unsigned char *old_sha1,
+			    int flags, int have_old, const char *msg,
+			    struct strbuf *err)
 {
 	if (transaction->state != REF_TRANSACTION_OPEN)
 		die("BUG: delete called for transaction that is not open");
@@ -3512,7 +3512,7 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 	if (have_old && is_null_sha1(old_sha1))
 		die("BUG: have_old is true but old_sha1 is null_sha1");
 
-	return ref_transaction_update(transaction, refname, null_sha1,
+	return transaction_update_sha1(transaction, refname, null_sha1,
 				      old_sha1, flags, have_old, msg, err);
 }
 
@@ -3523,14 +3523,14 @@ int update_ref(const char *action, const char *refname,
 	struct ref_transaction *t;
 	struct strbuf err = STRBUF_INIT;
 
-	t = ref_transaction_begin(&err);
+	t = transaction_begin(&err);
 	if (!t ||
-	    ref_transaction_update(t, refname, sha1, oldval, flags,
-				   !!oldval, action, &err) ||
-	    ref_transaction_commit(t, &err)) {
+	    transaction_update_sha1(t, refname, sha1, oldval, flags,
+				    !!oldval, action, &err) ||
+	    transaction_commit(t, &err)) {
 		const char *str = "update_ref failed for ref '%s': %s";
 
-		ref_transaction_free(t);
+		transaction_free(t);
 		switch (onerr) {
 		case UPDATE_REFS_MSG_ON_ERR:
 			error(str, refname, err.buf); break;
@@ -3567,8 +3567,8 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 	return 0;
 }
 
-int ref_transaction_commit(struct ref_transaction *transaction,
-			   struct strbuf *err)
+int transaction_commit(struct ref_transaction *transaction,
+		       struct strbuf *err)
 {
 	int ret = 0, delnum = 0, i, df_conflict = 0;
 	const char **delnames;
diff --git a/refs.h b/refs.h
index 20bb152..cf48351 100644
--- a/refs.h
+++ b/refs.h
@@ -17,16 +17,16 @@ struct ref_lock {
  * Calling sequence
  * ----------------
  * - Allocate and initialize a `struct ref_transaction` by calling
- *   `ref_transaction_begin()`.
+ *   `transaction_begin()`.
  *
  * - List intended ref updates by calling functions like
- *   `ref_transaction_update()` and `ref_transaction_create()`.
+ *   `transaction_update_sha1()` and `transaction_create_sha1()`.
  *
- * - Call `ref_transaction_commit()` to execute the transaction.
+ * - Call `transaction_commit()` to execute the transaction.
  *   If this succeeds, the ref updates will have taken place and
  *   the transaction cannot be rolled back.
  *
- * - At any time call `ref_transaction_free()` to discard the
+ * - At any time call `transaction_free()` to discard the
  *   transaction and free associated resources.  In particular,
  *   this rolls back the transaction if it has not been
  *   successfully committed.
@@ -44,7 +44,7 @@ struct ref_lock {
  * error message:
  *
  *     strbuf_addf(&err, "Error while doing foo-bar: ");
- *     if (ref_transaction_update(..., &err)) {
+ *     if (transaction_update_sha1(..., &err)) {
  *         ret = error("%s", err.buf);
  *         goto cleanup;
  *     }
@@ -178,8 +178,8 @@ extern int ref_exists(const char *);
 extern int peel_ref(const char *refname, unsigned char *sha1);
 
 /*
- * Flags controlling lock_any_ref_for_update(), ref_transaction_update(),
- * ref_transaction_create(), etc.
+ * Flags controlling lock_any_ref_for_update(), transaction_update_sha1(),
+ * transaction_create_sha1(), etc.
  * REF_NODEREF: act on the ref directly, instead of dereferencing
  *              symbolic references.
  *
@@ -268,9 +268,9 @@ enum action_on_err {
 
 /*
  * Begin a reference transaction.  The reference transaction must
- * be freed by calling ref_transaction_free().
+ * be freed by calling transaction_free().
  */
-struct ref_transaction *ref_transaction_begin(struct strbuf *err);
+struct ref_transaction *transaction_begin(struct strbuf *err);
 
 /*
  * The following functions add a reference check or update to a
@@ -291,12 +291,12 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
  * 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,
-			   const unsigned char *new_sha1,
-			   const unsigned char *old_sha1,
-			   int flags, int have_old, const char *msg,
-			   struct strbuf *err);
+int transaction_update_sha1(struct ref_transaction *transaction,
+			    const char *refname,
+			    const unsigned char *new_sha1,
+			    const unsigned char *old_sha1,
+			    int flags, int have_old, const char *msg,
+			    struct strbuf *err);
 
 /*
  * Add a reference creation to transaction.  new_sha1 is the value
@@ -307,11 +307,11 @@ int ref_transaction_update(struct ref_transaction *transaction,
  * 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,
-			   const unsigned char *new_sha1,
-			   int flags, const char *msg,
-			   struct strbuf *err);
+int transaction_create_sha1(struct ref_transaction *transaction,
+			    const char *refname,
+			    const unsigned char *new_sha1,
+			    int flags, const char *msg,
+			    struct strbuf *err);
 
 /*
  * Add a reference deletion to transaction.  If have_old is true, then
@@ -321,11 +321,11 @@ int ref_transaction_create(struct ref_transaction *transaction,
  * 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,
-			   const unsigned char *old_sha1,
-			   int flags, int have_old, const char *msg,
-			   struct strbuf *err);
+int transaction_delete_sha1(struct ref_transaction *transaction,
+			    const char *refname,
+			    const unsigned char *old_sha1,
+			    int flags, int have_old, const char *msg,
+			    struct strbuf *err);
 
 /*
  * Commit all of the changes that have been queued in transaction, as
@@ -338,13 +338,13 @@ int ref_transaction_delete(struct ref_transaction *transaction,
  * collision (ENOTDIR).
  */
 #define UPDATE_REFS_NAME_CONFLICT -2
-int ref_transaction_commit(struct ref_transaction *transaction,
-			   struct strbuf *err);
+int transaction_commit(struct ref_transaction *transaction,
+		       struct strbuf *err);
 
 /*
  * Free an existing transaction and all associated data.
  */
-void ref_transaction_free(struct ref_transaction *transaction);
+void transaction_free(struct ref_transaction *transaction);
 
 /** Lock a ref and then write its file */
 int update_ref(const char *action, const char *refname,
diff --git a/sequencer.c b/sequencer.c
index f9906ef..5080287 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -282,12 +282,12 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 
 	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
 
-	transaction = ref_transaction_begin(&err);
+	transaction = transaction_begin(&err);
 	if (!transaction ||
-	    ref_transaction_update(transaction, "HEAD", to, from,
-				   0, !unborn, sb.buf, &err) ||
-	    ref_transaction_commit(transaction, &err)) {
-		ref_transaction_free(transaction);
+	    transaction_update_sha1(transaction, "HEAD", to, from,
+				    0, !unborn, sb.buf, &err) ||
+	    transaction_commit(transaction, &err)) {
+		transaction_free(transaction);
 		error("%s", err.buf);
 		strbuf_release(&sb);
 		strbuf_release(&err);
@@ -295,7 +295,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 	}
 
 	strbuf_release(&sb);
-	ref_transaction_free(transaction);
+	transaction_free(transaction);
 	return 0;
 }
 
diff --git a/walker.c b/walker.c
index fd9ef87..d22d07a 100644
--- a/walker.c
+++ b/walker.c
@@ -261,7 +261,7 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 	save_commit_buffer = 0;
 
 	if (write_ref) {
-		transaction = ref_transaction_begin(&err);
+		transaction = transaction_begin(&err);
 		if (!transaction) {
 			error("%s", err.buf);
 			goto rollback_and_fail;
@@ -293,27 +293,27 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 			continue;
 		strbuf_reset(&ref_name);
 		strbuf_addf(&ref_name, "refs/%s", write_ref[i]);
-		if (ref_transaction_update(transaction, ref_name.buf,
-					   &sha1[20 * i], NULL, 0, 0,
-					   msg ? msg : "fetch (unknown)",
-					   &err)) {
+		if (transaction_update_sha1(transaction, ref_name.buf,
+					    &sha1[20 * i], NULL, 0, 0,
+					    msg ? msg : "fetch (unknown)",
+					    &err)) {
 			error("%s", err.buf);
 			goto rollback_and_fail;
 		}
 	}
 	if (write_ref) {
-		if (ref_transaction_commit(transaction, &err)) {
+		if (transaction_commit(transaction, &err)) {
 			error("%s", err.buf);
 			goto rollback_and_fail;
 		}
-		ref_transaction_free(transaction);
+		transaction_free(transaction);
 	}
 
 	free(msg);
 	return 0;
 
 rollback_and_fail:
-	ref_transaction_free(transaction);
+	transaction_free(transaction);
 	free(msg);
 	strbuf_release(&err);
 	strbuf_release(&ref_name);
-- 
2.0.0.467.g08c0633

[PATCH v3 04/14] refs.c: add a new update_type field to ref_update

[Ronnie Sahlberg]

Add a field that describes what type of update this refers to. For now
the only type is UPDATE_SHA1 but we will soon add more types.

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 4e3d4c3..4129de6 100644
--- a/refs.c
+++ b/refs.c
@@ -3374,6 +3374,10 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
 	return retval;
 }
 
+enum transaction_update_type {
+       UPDATE_SHA1 = 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
@@ -3381,6 +3385,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
  * value or to zero to ensure the ref does not exist before update.
  */
 struct ref_update {
+	enum transaction_update_type update_type;
 	unsigned char new_sha1[20];
 	unsigned char old_sha1[20];
 	int flags; /* REF_NODEREF? */
@@ -3444,12 +3449,14 @@ void transaction_free(struct ref_transaction *transaction)
 }
 
 static struct ref_update *add_update(struct ref_transaction *transaction,
-				     const char *refname)
+				     const char *refname,
+				     enum transaction_update_type update_type)
 {
 	size_t len = strlen(refname);
 	struct ref_update *update = xcalloc(1, sizeof(*update) + len + 1);
 
 	strcpy((char *)update->refname, refname);
+	update->update_type = update_type;
 	ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
 	transaction->updates[transaction->nr++] = update;
 	return update;
@@ -3470,7 +3477,7 @@ int transaction_update_sha1(struct ref_transaction *transaction,
 	if (have_old && !old_sha1)
 		die("BUG: have_old is true but old_sha1 is NULL");
 
-	update = add_update(transaction, refname);
+	update = add_update(transaction, refname, UPDATE_SHA1);
 	hashcpy(update->new_sha1, new_sha1);
 	update->flags = flags;
 	update->have_old = have_old;
@@ -3555,7 +3562,10 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 					struct strbuf *err)
 {
 	int i;
-	for (i = 1; i < n; i++)
+	for (i = 1; i < n; i++) {
+		if (updates[i - 1]->update_type != UPDATE_SHA1 ||
+		    updates[i]->update_type != UPDATE_SHA1)
+			continue;
 		if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {
 			const char *str =
 				"Multiple updates for ref '%s' not allowed.";
@@ -3564,6 +3574,7 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 
 			return 1;
 		}
+	}
 	return 0;
 }
 
@@ -3593,10 +3604,12 @@ int transaction_commit(struct ref_transaction *transaction,
 		goto cleanup;
 	}
 
-	/* Acquire all locks while verifying old values */
+	/* Acquire all ref locks while verifying old values */
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
+		if (update->update_type != UPDATE_SHA1)
+			continue;
 		update->lock = lock_ref_sha1_basic(update->refname,
 						   (update->have_old ?
 						    update->old_sha1 :
@@ -3619,6 +3632,8 @@ int transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
+		if (update->update_type != UPDATE_SHA1)
+			continue;
 		if (!is_null_sha1(update->new_sha1)) {
 			ret = write_ref_sha1(update->lock, update->new_sha1,
 					     update->msg);
@@ -3638,6 +3653,8 @@ int transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
+		if (update->update_type != UPDATE_SHA1)
+			continue;
 		if (update->lock) {
 			if (delete_ref_loose(update->lock, update->type, err))
 				ret = -1;
-- 
2.0.0.467.g08c0633

[PATCH v3 05/14] refs.c: add a function to append a reflog entry to a fd

[Ronnie Sahlberg]

Break out the code to create the string and writing it to the file
descriptor from log_ref_write and into a dedicated function log_ref_write_fd.
For now this is only used from log_ref_write but later on we will call
this function from reflog transactions too which means that we will end
up with only a single place where we write a reflog entry to a file instead
of the current two places (log_ref_write and builtin/reflog.c).

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

diff --git a/refs.c b/refs.c
index 4129de6..f203285 100644
--- a/refs.c
+++ b/refs.c
@@ -2865,15 +2865,37 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)
 	return 0;
 }
 
+static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
+			    const unsigned char *new_sha1,
+			    const char *committer, const char *msg)
+{
+	int msglen, written;
+	unsigned maxlen, len;
+	char *logrec;
+
+	msglen = msg ? strlen(msg) : 0;
+	maxlen = strlen(committer) + msglen + 100;
+	logrec = xmalloc(maxlen);
+	len = sprintf(logrec, "%s %s %s\n",
+		      sha1_to_hex(old_sha1),
+		      sha1_to_hex(new_sha1),
+		      committer);
+	if (msglen)
+		len += copy_msg(logrec + len - 1, msg) - 1;
+
+	written = len <= maxlen ? write_in_full(fd, logrec, len) : -1;
+	free(logrec);
+	if (written != len)
+		return -1;
+
+	return 0;
+}
+
 static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 			 const unsigned char *new_sha1, const char *msg)
 {
-	int logfd, result, written, oflags = O_APPEND | O_WRONLY;
-	unsigned maxlen, len;
-	int msglen;
+	int logfd, result, oflags = O_APPEND | O_WRONLY;
 	char log_file[PATH_MAX];
-	char *logrec;
-	const char *committer;
 
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
@@ -2885,19 +2907,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 	logfd = open(log_file, oflags);
 	if (logfd < 0)
 		return 0;
-	msglen = msg ? strlen(msg) : 0;
-	committer = git_committer_info(0);
-	maxlen = strlen(committer) + msglen + 100;
-	logrec = xmalloc(maxlen);
-	len = sprintf(logrec, "%s %s %s\n",
-		      sha1_to_hex(old_sha1),
-		      sha1_to_hex(new_sha1),
-		      committer);
-	if (msglen)
-		len += copy_msg(logrec + len - 1, msg) - 1;
-	written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;
-	free(logrec);
-	if (written != len) {
+	result = log_ref_write_fd(logfd, old_sha1, new_sha1,
+				  git_committer_info(0), msg);
+	if (result) {
 		int save_errno = errno;
 		close(logfd);
 		error("Unable to append to %s", log_file);
-- 
2.0.0.467.g08c0633

[PATCH v3 06/14] lockfile.c: make hold_lock_file_for_append preserve meaningful errno

[Ronnie Sahlberg]

Update hold_lock_file_for_append and copy_fd to return a meaningful errno
on failure.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 copy.c     | 20 +++++++++++++-------
 lockfile.c |  7 ++++++-
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/copy.c b/copy.c
index a7f58fd..5cb8679 100644
--- a/copy.c
+++ b/copy.c
@@ -9,10 +9,12 @@ int copy_fd(int ifd, int ofd)
 		if (!len)
 			break;
 		if (len < 0) {
-			int read_error = errno;
+			int save_errno = errno;
 			close(ifd);
-			return error("copy-fd: read returned %s",
-				     strerror(read_error));
+			error("copy-fd: read returned %s",
+			      strerror(save_errno));
+			errno = save_errno;
+			return -1;
 		}
 		while (len) {
 			int written = xwrite(ofd, buf, len);
@@ -22,12 +24,16 @@ int copy_fd(int ifd, int ofd)
 			}
 			else if (!written) {
 				close(ifd);
-				return error("copy-fd: write returned 0");
+				error("copy-fd: write returned 0");
+				errno = EAGAIN;
+				return -1;
 			} else {
-				int write_error = errno;
+				int save_errno = errno;
 				close(ifd);
-				return error("copy-fd: write returned %s",
-					     strerror(write_error));
+				error("copy-fd: write returned %s",
+				      strerror(save_errno));
+				errno = save_errno;
+				return -1;
 			}
 		}
 	}
diff --git a/lockfile.c b/lockfile.c
index a921d77..32f4681 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -217,15 +217,20 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)
 	orig_fd = open(path, O_RDONLY);
 	if (orig_fd < 0) {
 		if (errno != ENOENT) {
+			int save_errno = errno;
 			if (flags & LOCK_DIE_ON_ERROR)
 				die("cannot open '%s' for copying", path);
 			close(fd);
-			return error("cannot open '%s' for copying", path);
+			error("cannot open '%s' for copying", path);
+			errno = save_errno;
+			return -1;
 		}
 	} else if (copy_fd(orig_fd, fd)) {
+		int save_errno = errno;
 		if (flags & LOCK_DIE_ON_ERROR)
 			exit(128);
 		close(fd);
+		errno = save_errno;
 		return -1;
 	}
 	return fd;
-- 
2.0.0.467.g08c0633

[PATCH v3 07/14] refs.c: add a transaction function to append a reflog entry

[Ronnie Sahlberg]

Define a new transaction update type, UPDATE_LOG, and a new function
transaction_update_reflog. This function will lock the reflog and append
an entry to it during transaction commit.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 refs.h |  12 ++++++++
 2 files changed, 112 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index f203285..d673a0f 100644
--- a/refs.c
+++ b/refs.c
@@ -3388,6 +3388,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
 
 enum transaction_update_type {
        UPDATE_SHA1 = 0,
+       UPDATE_LOG = 1,
 };
 
 /**
@@ -3405,6 +3406,12 @@ struct ref_update {
 	struct ref_lock *lock;
 	int type;
 	char *msg;
+
+	/* used by reflog updates */
+	int reflog_fd;
+	struct lock_file reflog_lock;
+	char *committer;
+
 	const char refname[FLEX_ARRAY];
 };
 
@@ -3454,6 +3461,7 @@ void transaction_free(struct ref_transaction *transaction)
 
 	for (i = 0; i < transaction->nr; i++) {
 		free(transaction->updates[i]->msg);
+		free(transaction->updates[i]->committer);
 		free(transaction->updates[i]);
 	}
 	free(transaction->updates);
@@ -3474,6 +3482,42 @@ static struct ref_update *add_update(struct ref_transaction *transaction,
 	return update;
 }
 
+int transaction_update_reflog(struct ref_transaction *transaction,
+			      const char *refname,
+			      const unsigned char *new_sha1,
+			      const unsigned char *old_sha1,
+			      const unsigned char *email,
+			      unsigned long timestamp, int tz,
+			      const char *msg, int flags,
+			      struct strbuf *err)
+{
+	struct ref_update *update;
+
+	if (transaction->state != REF_TRANSACTION_OPEN)
+		die("BUG: update_reflog called for transaction that is not "
+		    "open");
+
+	update = add_update(transaction, refname, UPDATE_LOG);
+	hashcpy(update->new_sha1, new_sha1);
+	hashcpy(update->old_sha1, old_sha1);
+	update->reflog_fd = -1;
+	if (email) {
+		struct strbuf buf = STRBUF_INIT;
+		char sign = (tz < 0) ? '-' : '+';
+		int zone = (tz < 0) ? (-tz) : tz;
+
+		strbuf_addf(&buf, "%s %lu %c%04d", email, timestamp, sign,
+			    zone);
+		update->committer = xstrdup(buf.buf);
+		strbuf_release(&buf);
+	}
+	if (msg)
+		update->msg = xstrdup(msg);
+	update->flags = flags;
+
+	return 0;
+}
+
 int transaction_update_sha1(struct ref_transaction *transaction,
 			    const char *refname,
 			    const unsigned char *new_sha1,
@@ -3640,7 +3684,28 @@ int transaction_commit(struct ref_transaction *transaction,
 		}
 	}
 
-	/* Perform updates first so live commits remain referenced */
+	/* Lock all reflog files */
+	for (i = 0; i < n; i++) {
+		struct ref_update *update = updates[i];
+
+		if (update->update_type != UPDATE_LOG)
+			continue;
+		update->reflog_fd = hold_lock_file_for_append(
+					&update->reflog_lock,
+					git_path("logs/%s", update->refname),
+					0);
+		if (update->reflog_fd < 0) {
+			const char *str = "Cannot lock reflog for '%s'. %s";
+
+			ret = -1;
+			if (err)
+				  strbuf_addf(err, str, update->refname,
+					      strerror(errno));
+			goto cleanup;
+		}
+	}
+
+	/* Perform ref updates first so live commits remain referenced */
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
@@ -3676,6 +3741,40 @@ int transaction_commit(struct ref_transaction *transaction,
 		}
 	}
 
+	/* Update all reflog files */
+	for (i = 0; i < n; i++) {
+		struct ref_update *update = updates[i];
+
+		if (update->update_type != UPDATE_LOG)
+			continue;
+		if (update->reflog_fd == -1)
+			continue;
+
+		if (log_ref_write_fd(update->reflog_fd, update->old_sha1,
+				     update->new_sha1,
+				     update->committer, update->msg)) {
+			error("Could write to reflog: %s. %s",
+			      update->refname, strerror(errno));
+			rollback_lock_file(&update->reflog_lock);
+			update->reflog_fd = -1;
+		}
+	}
+
+	/* Commit all reflog files */
+	for (i = 0; i < n; i++) {
+		struct ref_update *update = updates[i];
+
+		if (update->update_type != UPDATE_LOG)
+			continue;
+		if (update->reflog_fd == -1)
+			continue;
+		if (commit_lock_file(&update->reflog_lock)) {
+			error("Could not commit reflog: %s. %s",
+			      update->refname, strerror(errno));
+			update->reflog_fd = -1;
+		}
+	}
+
 	if (repack_without_refs(delnames, delnum, err))
 		ret = -1;
 	for (i = 0; i < delnum; i++)
diff --git a/refs.h b/refs.h
index cf48351..bf52068 100644
--- a/refs.h
+++ b/refs.h
@@ -328,6 +328,18 @@ int transaction_delete_sha1(struct ref_transaction *transaction,
 			    struct strbuf *err);
 
 /*
+ * Append a reflog entry for refname.
+ */
+int transaction_update_reflog(struct ref_transaction *transaction,
+			      const char *refname,
+			      const unsigned char *new_sha1,
+			      const unsigned char *old_sha1,
+			      const unsigned char *email,
+			      unsigned long timestamp, int tz,
+			      const char *msg, int flags,
+			      struct strbuf *err);
+
+/*
  * Commit all of the changes that have been queued in transaction, as
  * atomically as possible.  Return a nonzero value if there is a
  * problem.
-- 
2.0.0.467.g08c0633

[PATCH v3 08/14] refs.c: add a flag to allow reflog updates to truncate the log

[Ronnie Sahlberg]

Add a flag that allows us to truncate the reflog before we write the
update.

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

diff --git a/refs.c b/refs.c
index d673a0f..c33d19e 100644
--- a/refs.c
+++ b/refs.c
@@ -3741,7 +3741,12 @@ int transaction_commit(struct ref_transaction *transaction,
 		}
 	}
 
-	/* Update all reflog files */
+	/*
+	 * Update all reflog files
+	 * We have already done all ref updates and deletes.
+	 * There is not much we can do here if there are any reflog
+	 * update errors other than complain.
+	 */
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
@@ -3749,7 +3754,15 @@ int transaction_commit(struct ref_transaction *transaction,
 			continue;
 		if (update->reflog_fd == -1)
 			continue;
-
+		if (update->flags & REFLOG_TRUNCATE)
+			if (lseek(update->reflog_fd, 0, SEEK_SET) < 0 ||
+				ftruncate(update->reflog_fd, 0)) {
+				error("Could not truncate reflog: %s. %s",
+				      update->refname, strerror(errno));
+				rollback_lock_file(&update->reflog_lock);
+				update->reflog_fd = -1;
+				continue;
+			}
 		if (log_ref_write_fd(update->reflog_fd, update->old_sha1,
 				     update->new_sha1,
 				     update->committer, update->msg)) {
diff --git a/refs.h b/refs.h
index bf52068..f14c8db 100644
--- a/refs.h
+++ b/refs.h
@@ -328,7 +328,15 @@ int transaction_delete_sha1(struct ref_transaction *transaction,
 			    struct strbuf *err);
 
 /*
- * Append a reflog entry for refname.
+ * Flags controlling transaction_update_reflog().
+ * REFLOG_TRUNCATE: Truncate the reflog.
+ *
+ * Flags >= 0x100 are reserved for internal use.
+ */
+#define REFLOG_TRUNCATE 0x01
+/*
+ * Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set
+ * this update will first truncate the reflog before writing the entry.
  */
 int transaction_update_reflog(struct ref_transaction *transaction,
 			      const char *refname,
-- 
2.0.0.467.g08c0633

[PATCH v3 09/14] refs.c: only write reflog update if msg is non-NULL

[Ronnie Sahlberg]

When performing a reflog transaction update, only write to the reflog iff
msg is non-NULL. This can then be combined with REFLOG_TRUNCATE to perform
an update that only truncates but does not write.

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

diff --git a/refs.c b/refs.c
index c33d19e..d6df28d 100644
--- a/refs.c
+++ b/refs.c
@@ -3763,8 +3763,9 @@ int transaction_commit(struct ref_transaction *transaction,
 				update->reflog_fd = -1;
 				continue;
 			}
-		if (log_ref_write_fd(update->reflog_fd, update->old_sha1,
-				     update->new_sha1,
+		if (update->msg &&
+		    log_ref_write_fd(update->reflog_fd,
+				     update->old_sha1, update->new_sha1,
 				     update->committer, update->msg)) {
 			error("Could write to reflog: %s. %s",
 			      update->refname, strerror(errno));
diff --git a/refs.h b/refs.h
index f14c8db..1d7906c 100644
--- a/refs.h
+++ b/refs.h
@@ -337,6 +337,7 @@ int transaction_delete_sha1(struct ref_transaction *transaction,
 /*
  * Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set
  * this update will first truncate the reflog before writing the entry.
+ * If msg is NULL no update will be written to the log.
  */
 int transaction_update_reflog(struct ref_transaction *transaction,
 			      const char *refname,
-- 
2.0.0.467.g08c0633

[PATCH v3 10/14] refs.c: allow multiple reflog updates during a single transaction

[Ronnie Sahlberg]

Allow to make multiple reflog updates to the same ref during a transaction.
This means we only need to lock the reflog once, during the first update
that touches the reflog, and that all further updates can just write the
reflog entry since the reflog is already locked.

This allows us to write code such as:

t = transaction_begin()
transaction_reflog_update(t, "foo", REFLOG_TRUNCATE, NULL);
loop-over-somehting...
   transaction_reflog_update(t, "foo", 0, <message>);
transaction_commit(t)

where we first truncate the reflog and then build the new content one line at a
time.

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

diff --git a/refs.c b/refs.c
index d6df28d..ad60231 100644
--- a/refs.c
+++ b/refs.c
@@ -30,6 +30,12 @@ static unsigned char refname_disposition[256] = {
  */
 #define REF_ISPRUNING	0x0100
 /*
+ * Only the first reflog update needs to lock the reflog file. Further updates
+ * just use the lock taken by the first update.
+ */
+#define UPDATE_REFLOG_NOLOCK 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 legal.  It is legal if it is something reasonable to have under
@@ -3391,7 +3397,7 @@ enum transaction_update_type {
        UPDATE_LOG = 1,
 };
 
-/**
+/*
  * 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
  * while locking the ref, set have_old to 1 and set old_sha1 to the
@@ -3401,7 +3407,7 @@ struct ref_update {
 	enum transaction_update_type update_type;
 	unsigned char new_sha1[20];
 	unsigned char old_sha1[20];
-	int flags; /* REF_NODEREF? */
+	int flags; /* REF_NODEREF? or private flags */
 	int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
 	struct ref_lock *lock;
 	int type;
@@ -3409,8 +3415,9 @@ struct ref_update {
 
 	/* used by reflog updates */
 	int reflog_fd;
-	struct lock_file reflog_lock;
+	struct lock_file *reflog_lock;
 	char *committer;
+	struct ref_update *orig_update; /* For UPDATE_REFLOG_NOLOCK */
 
 	const char refname[FLEX_ARRAY];
 };
@@ -3492,12 +3499,27 @@ int transaction_update_reflog(struct ref_transaction *transaction,
 			      struct strbuf *err)
 {
 	struct ref_update *update;
+	int i;
 
 	if (transaction->state != REF_TRANSACTION_OPEN)
 		die("BUG: update_reflog called for transaction that is not "
 		    "open");
 
 	update = add_update(transaction, refname, UPDATE_LOG);
+	update->flags = flags;
+	for (i = 0; i < transaction->nr - 1; i++) {
+		if (transaction->updates[i]->update_type != UPDATE_LOG)
+			continue;
+		if (!strcmp(transaction->updates[i]->refname,
+			    update->refname)) {
+			update->flags |= UPDATE_REFLOG_NOLOCK;
+			update->orig_update = transaction->updates[i];
+			break;
+		}
+	}
+	if (!(update->flags & UPDATE_REFLOG_NOLOCK))
+		update->reflog_lock = xcalloc(1, sizeof(struct lock_file));
+
 	hashcpy(update->new_sha1, new_sha1);
 	hashcpy(update->old_sha1, old_sha1);
 	update->reflog_fd = -1;
@@ -3513,7 +3535,6 @@ int transaction_update_reflog(struct ref_transaction *transaction,
 	}
 	if (msg)
 		update->msg = xstrdup(msg);
-	update->flags = flags;
 
 	return 0;
 }
@@ -3690,10 +3711,15 @@ int transaction_commit(struct ref_transaction *transaction,
 
 		if (update->update_type != UPDATE_LOG)
 			continue;
+		if (update->flags & UPDATE_REFLOG_NOLOCK) {
+			update->reflog_fd = update->orig_update->reflog_fd;
+			update->reflog_lock = update->orig_update->reflog_lock;
+			continue;
+		}
 		update->reflog_fd = hold_lock_file_for_append(
-					&update->reflog_lock,
+					update->reflog_lock,
 					git_path("logs/%s", update->refname),
-					0);
+					LOCK_NODEREF);
 		if (update->reflog_fd < 0) {
 			const char *str = "Cannot lock reflog for '%s'. %s";
 
@@ -3759,7 +3785,7 @@ int transaction_commit(struct ref_transaction *transaction,
 				ftruncate(update->reflog_fd, 0)) {
 				error("Could not truncate reflog: %s. %s",
 				      update->refname, strerror(errno));
-				rollback_lock_file(&update->reflog_lock);
+				rollback_lock_file(update->reflog_lock);
 				update->reflog_fd = -1;
 				continue;
 			}
@@ -3769,7 +3795,7 @@ int transaction_commit(struct ref_transaction *transaction,
 				     update->committer, update->msg)) {
 			error("Could write to reflog: %s. %s",
 			      update->refname, strerror(errno));
-			rollback_lock_file(&update->reflog_lock);
+			rollback_lock_file(update->reflog_lock);
 			update->reflog_fd = -1;
 		}
 	}
@@ -3780,9 +3806,11 @@ int transaction_commit(struct ref_transaction *transaction,
 
 		if (update->update_type != UPDATE_LOG)
 			continue;
+		if (update->flags & UPDATE_REFLOG_NOLOCK)
+			continue;
 		if (update->reflog_fd == -1)
 			continue;
-		if (commit_lock_file(&update->reflog_lock)) {
+		if (commit_lock_file(update->reflog_lock)) {
 			error("Could not commit reflog: %s. %s",
 			      update->refname, strerror(errno));
 			update->reflog_fd = -1;
-- 
2.0.0.467.g08c0633

[PATCH v3 11/14] reflog.c: use a reflog transaction when writing during expire

[Ronnie Sahlberg]

Use a transaction for all updates during expire_reflog.

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

diff --git a/builtin/reflog.c b/builtin/reflog.c
index e8a8fb1..f11fee3 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -32,8 +32,11 @@ struct cmd_reflog_expire_cb {
 	int recno;
 };
 
+static struct strbuf err = STRBUF_INIT;
+
 struct expire_reflog_cb {
-	FILE *newlog;
+	struct ref_transaction *t;
+	const char *refname;
 	enum {
 		UE_NORMAL,
 		UE_ALWAYS,
@@ -316,20 +319,18 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 	if (cb->cmd->recno && --(cb->cmd->recno) == 0)
 		goto prune;
 
-	if (cb->newlog) {
-		char sign = (tz < 0) ? '-' : '+';
-		int zone = (tz < 0) ? (-tz) : tz;
-		fprintf(cb->newlog, "%s %s %s %lu %c%04d\t%s",
-			sha1_to_hex(osha1), sha1_to_hex(nsha1),
-			email, timestamp, sign, zone,
-			message);
+	if (cb->t) {
+		if (transaction_update_reflog(cb->t, cb->refname, nsha1, osha1,
+					      email, timestamp, tz, message, 0,
+					      &err))
+			return -1;
 		hashcpy(cb->last_kept_sha1, nsha1);
 	}
 	if (cb->cmd->verbose)
 		printf("keep %s", message);
 	return 0;
  prune:
-	if (!cb->newlog)
+	if (!cb->t)
 		printf("would prune %s", message);
 	else if (cb->cmd->verbose)
 		printf("prune %s", message);
@@ -353,29 +354,26 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,
 {
 	struct cmd_reflog_expire_cb *cmd = cb_data;
 	struct expire_reflog_cb cb;
-	struct ref_lock *lock;
-	char *log_file, *newlog_path = NULL;
 	struct commit *tip_commit;
 	struct commit_list *tips;
 	int status = 0;
 
 	memset(&cb, 0, sizeof(cb));
+	cb.refname = ref;
 
-	/*
-	 * we take the lock for the ref itself to prevent it from
-	 * getting updated.
-	 */
-	lock = lock_any_ref_for_update(ref, sha1, 0, NULL);
-	if (!lock)
-		return error("cannot lock ref '%s'", ref);
-	log_file = git_pathdup("logs/%s", ref);
 	if (!reflog_exists(ref))
 		goto finish;
-	if (!cmd->dry_run) {
-		newlog_path = git_pathdup("logs/%s.lock", ref);
-		cb.newlog = fopen(newlog_path, "w");
+	cb.t = transaction_begin(&err);
+	if (!cb.t) {
+		status |= error("%s", err.buf);
+		goto cleanup;
+	}
+	if (transaction_update_reflog(cb.t, cb.refname, null_sha1, null_sha1,
+				      NULL, 0, 0, NULL, REFLOG_TRUNCATE,
+				      &err)) {
+		status |= error("%s", err.buf);
+		goto cleanup;
 	}
-
 	cb.cmd = cmd;
 
 	if (!cmd->expire_unreachable || !strcmp(ref, "HEAD")) {
@@ -407,7 +405,10 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,
 		mark_reachable(&cb);
 	}
 
-	for_each_reflog_ent(ref, expire_reflog_ent, &cb);
+	if (for_each_reflog_ent(ref, expire_reflog_ent, &cb)) {
+		status |= error("%s", err.buf);
+		goto cleanup;
+	}
 
 	if (cb.unreachable_expire_kind != UE_ALWAYS) {
 		if (cb.unreachable_expire_kind == UE_HEAD) {
@@ -420,32 +421,19 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,
 		}
 	}
  finish:
-	if (cb.newlog) {
-		if (fclose(cb.newlog)) {
-			status |= error("%s: %s", strerror(errno),
-					newlog_path);
-			unlink(newlog_path);
-		} else if (cmd->updateref &&
-			(write_in_full(lock->lock_fd,
-				sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
-			 write_str_in_full(lock->lock_fd, "\n") != 1 ||
-			 close_ref(lock) < 0)) {
-			status |= error("Couldn't write %s",
-				lock->lk->filename);
-			unlink(newlog_path);
-		} else if (rename(newlog_path, log_file)) {
-			status |= error("cannot rename %s to %s",
-					newlog_path, log_file);
-			unlink(newlog_path);
-		} else if (cmd->updateref && commit_ref(lock)) {
-			status |= error("Couldn't set %s", lock->ref_name);
-		} else {
-			adjust_shared_perm(log_file);
+	if (!cmd->dry_run) {
+		if (cmd->updateref &&
+		    transaction_update_sha1(cb.t, cb.refname,
+					    cb.last_kept_sha1, sha1,
+					    0, 1, NULL, &err)) {
+			status |= error("%s", err.buf);
+		} else if (transaction_commit(cb.t, &err)) {
+			status |= error("%s", err.buf);
 		}
 	}
-	free(newlog_path);
-	free(log_file);
-	unlock_ref(lock);
+ cleanup:
+	transaction_free(cb.t);
+	strbuf_release(&err);
 	return status;
 }
 
diff --git a/refs.c b/refs.c
index ad60231..1288c49 100644
--- a/refs.c
+++ b/refs.c
@@ -3493,7 +3493,7 @@ int transaction_update_reflog(struct ref_transaction *transaction,
 			      const char *refname,
 			      const unsigned char *new_sha1,
 			      const unsigned char *old_sha1,
-			      const unsigned char *email,
+			      const char *email,
 			      unsigned long timestamp, int tz,
 			      const char *msg, int flags,
 			      struct strbuf *err)
@@ -3769,7 +3769,7 @@ int transaction_commit(struct ref_transaction *transaction,
 
 	/*
 	 * Update all reflog files
-	 * We have already done all ref updates and deletes.
+	 * We have already committed all ref updates and deletes.
 	 * There is not much we can do here if there are any reflog
 	 * update errors other than complain.
 	 */
diff --git a/refs.h b/refs.h
index 1d7906c..0564955 100644
--- a/refs.h
+++ b/refs.h
@@ -343,7 +343,7 @@ int transaction_update_reflog(struct ref_transaction *transaction,
 			      const char *refname,
 			      const unsigned char *new_sha1,
 			      const unsigned char *old_sha1,
-			      const unsigned char *email,
+			      const char *email,
 			      unsigned long timestamp, int tz,
 			      const char *msg, int flags,
 			      struct strbuf *err);
-- 
2.0.0.467.g08c0633

[PATCH v3 12/14] refs.c: rename log_ref_setup to create_reflog

[Ronnie Sahlberg]

log_ref_setup is used to do several semi-related things :
* sometimes it will create a new reflog including missing parent directories
  and cleaning up any conflicting stale directories in the path.
* fill in a filename buffer for the full path to the reflog.
* unconditionally re-adjust the permissions for the file.

This function is only called from two places: checkout.c where it is always
used to create a reflog and refs.c:log_ref_write where it sometimes are
used to create a reflog and sometimes just used to fill in the filename.

Rename log_ref_setup to create_reflog and change it to only take the
refname as argument to make its signature similar to delete_reflog and
reflog_exists. Change create_reflog to ignore log_all_ref_updates and
"unconditionally" create the reflog when called. Since checkout.c always
wants to create a reflog we can call create_reflog directly and avoid the
temp-and-log_all_ref_update dance.

In log_ref_write, only call create_reflog iff we want to create a reflog
and if the reflog does not yet exist. This means that for the common case
where the log already exists we now only need to perform a single lstat()
instead of a open(O_CREAT)+lstat()+close().

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/checkout.c |  8 +-------
 refs.c             | 22 +++++++++++++---------
 refs.h             |  8 +++-----
 3 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index f1dc56e..808c58f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -583,19 +583,13 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 	if (opts->new_branch) {
 		if (opts->new_orphan_branch) {
 			if (opts->new_branch_log && !log_all_ref_updates) {
-				int temp;
-				char log_file[PATH_MAX];
 				char *ref_name = mkpath("refs/heads/%s", opts->new_orphan_branch);
 
-				temp = log_all_ref_updates;
-				log_all_ref_updates = 1;
-				if (log_ref_setup(ref_name, log_file, sizeof(log_file))) {
+				if (create_reflog(ref_name)) {
 					fprintf(stderr, _("Can not do reflog for '%s'\n"),
 					    opts->new_orphan_branch);
-					log_all_ref_updates = temp;
 					return;
 				}
-				log_all_ref_updates = temp;
 			}
 		}
 		else
diff --git a/refs.c b/refs.c
index 1288c49..9653a01 100644
--- a/refs.c
+++ b/refs.c
@@ -2822,16 +2822,16 @@ static int copy_msg(char *buf, const char *msg)
 }
 
 /* This function must set a meaningful errno on failure */
-int log_ref_setup(const char *refname, char *logfile, int bufsize)
+int create_reflog(const char *refname)
 {
 	int logfd, oflags = O_APPEND | O_WRONLY;
+	char logfile[PATH_MAX];
 
-	git_snpath(logfile, bufsize, "logs/%s", refname);
-	if (log_all_ref_updates &&
-	    (starts_with(refname, "refs/heads/") ||
-	     starts_with(refname, "refs/remotes/") ||
-	     starts_with(refname, "refs/notes/") ||
-	     !strcmp(refname, "HEAD"))) {
+	git_snpath(logfile, sizeof(logfile), "logs/%s", refname);
+	if (starts_with(refname, "refs/heads/") ||
+	    starts_with(refname, "refs/remotes/") ||
+	    starts_with(refname, "refs/notes/") ||
+	    !strcmp(refname, "HEAD")) {
 		if (safe_create_leading_directories(logfile) < 0) {
 			int save_errno = errno;
 			error("unable to create directory for %s", logfile);
@@ -2900,16 +2900,20 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
 static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 			 const unsigned char *new_sha1, const char *msg)
 {
-	int logfd, result, oflags = O_APPEND | O_WRONLY;
+	int logfd, result = 0, oflags = O_APPEND | O_WRONLY;
 	char log_file[PATH_MAX];
 
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
 
-	result = log_ref_setup(refname, log_file, sizeof(log_file));
+	if (log_all_ref_updates && !reflog_exists(refname))
+		result = create_reflog(refname);
+
 	if (result)
 		return result;
 
+	git_snpath(log_file, sizeof(log_file), "logs/%s", refname);
+
 	logfd = open(log_file, oflags);
 	if (logfd < 0)
 		return 0;
diff --git a/refs.h b/refs.h
index 0564955..e7892fc 100644
--- a/refs.h
+++ b/refs.h
@@ -203,11 +203,6 @@ extern int commit_ref(struct ref_lock *lock);
 /** Release any lock taken but not written. **/
 extern void unlock_ref(struct ref_lock *lock);
 
-/*
- * Setup reflog before using. Set errno to something meaningful on failure.
- */
-int log_ref_setup(const char *refname, char *logfile, int bufsize);
-
 /** Reads log for the value of ref during at_time. **/
 extern int read_ref_at(const char *refname, unsigned long at_time, int cnt,
 		       unsigned char *sha1, char **msg,
@@ -216,6 +211,9 @@ extern int read_ref_at(const char *refname, unsigned long at_time, int cnt,
 /** Check if a particular reflog exists */
 extern int reflog_exists(const char *refname);
 
+/** Create reflog. Set errno to something meaningful on failure. */
+extern int create_reflog(const char *refname);
+
 /** Delete a reflog */
 extern int delete_reflog(const char *refname);
 
-- 
2.0.0.467.g08c0633

[PATCH v3 13/14] refs.c: make unlock_ref/close_ref/commit_ref static

[Ronnie Sahlberg]

unlock|close|commit_ref can be made static since there are no more external
callers.

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

diff --git a/refs.c b/refs.c
index 9653a01..ff98682 100644
--- a/refs.c
+++ b/refs.c
@@ -1960,6 +1960,16 @@ int refname_match(const char *abbrev_name, const char *full_name)
 	return 0;
 }
 
+static void unlock_ref(struct ref_lock *lock)
+{
+	/* Do not free lock->lk -- atexit() still looks at them */
+	if (lock->lk)
+		rollback_lock_file(lock->lk);
+	free(lock->ref_name);
+	free(lock->orig_ref_name);
+	free(lock);
+}
+
 /* This function should make sure errno is meaningful on error */
 static struct ref_lock *verify_lock(struct ref_lock *lock,
 	const unsigned char *old_sha1, int mustexist)
@@ -2769,7 +2779,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	return 1;
 }
 
-int close_ref(struct ref_lock *lock)
+static int close_ref(struct ref_lock *lock)
 {
 	if (close_lock_file(lock->lk))
 		return -1;
@@ -2777,7 +2787,7 @@ int close_ref(struct ref_lock *lock)
 	return 0;
 }
 
-int commit_ref(struct ref_lock *lock)
+static int commit_ref(struct ref_lock *lock)
 {
 	if (commit_lock_file(lock->lk))
 		return -1;
@@ -2785,16 +2795,6 @@ int commit_ref(struct ref_lock *lock)
 	return 0;
 }
 
-void unlock_ref(struct ref_lock *lock)
-{
-	/* Do not free lock->lk -- atexit() still looks at them */
-	if (lock->lk)
-		rollback_lock_file(lock->lk);
-	free(lock->ref_name);
-	free(lock->orig_ref_name);
-	free(lock);
-}
-
 /*
  * copy the reflog message msg to buf, which has been allocated sufficiently
  * large, while cleaning up the whitespaces.  Especially, convert LF to space,
diff --git a/refs.h b/refs.h
index e7892fc..5054388 100644
--- a/refs.h
+++ b/refs.h
@@ -194,15 +194,6 @@ extern struct ref_lock *lock_any_ref_for_update(const char *refname,
 						const unsigned char *old_sha1,
 						int flags, int *type_p);
 
-/** Close the file descriptor owned by a lock and return the status */
-extern int close_ref(struct ref_lock *lock);
-
-/** Close and commit the ref locked by the lock */
-extern int commit_ref(struct ref_lock *lock);
-
-/** Release any lock taken but not written. **/
-extern void unlock_ref(struct ref_lock *lock);
-
 /** Reads log for the value of ref during at_time. **/
 extern int read_ref_at(const char *refname, unsigned long at_time, int cnt,
 		       unsigned char *sha1, char **msg,
-- 
2.0.0.467.g08c0633

[PATCH v3 14/14] refs.c: remove lock_any_ref_for_update

[Ronnie Sahlberg]

No one is using this function so we can delete it.

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

diff --git a/refs.c b/refs.c
index ff98682..95c3eb8 100644
--- a/refs.c
+++ b/refs.c
@@ -2205,13 +2205,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	return NULL;
 }
 
-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, NULL, 0);
-}
-
 /*
  * Write an entry to the packed-refs file for the specified refname.
  * If peeled is non-NULL, write it as the entry's peeled value.
diff --git a/refs.h b/refs.h
index 5054388..b674c20 100644
--- a/refs.h
+++ b/refs.h
@@ -178,21 +178,13 @@ extern int ref_exists(const char *);
 extern int peel_ref(const char *refname, unsigned char *sha1);
 
 /*
- * Flags controlling lock_any_ref_for_update(), transaction_update_sha1(),
- * transaction_create_sha1(), etc.
+ * Flags controlling transaction_update_sha1(), transaction_create_sha1(), etc.
  * REF_NODEREF: act on the ref directly, instead of dereferencing
  *              symbolic references.
  *
  * Flags >= 0x100 are reserved for internal use.
  */
 #define REF_NODEREF	0x01
-/*
- * Locks any ref (for 'HEAD' type refs) and sets errno to something
- * meaningful on failure.
- */
-extern struct ref_lock *lock_any_ref_for_update(const char *refname,
-						const unsigned char *old_sha1,
-						int flags, int *type_p);
 
 /** Reads log for the value of ref during at_time. **/
 extern int read_ref_at(const char *refname, unsigned long at_time, int cnt,
-- 
2.0.0.467.g08c0633

Re: [PATCH v3 01/14] refs.c make ref_transaction_create a wrapper to ref_transaction_update

[Junio C Hamano]

Ronnie Sahlberg <sahlberg@google.com> writes:

> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  refs.c | 13 ++-----------
>  refs.h |  7 ++++---
>  2 files changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index dfbf003..a9f91ab 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3487,23 +3487,14 @@ int ref_transaction_create(struct ref_transaction *transaction,
>  			   int flags, const char *msg,
>  			   struct strbuf *err)
>  {
> -	struct ref_update *update;
> -
>  	if (transaction->state != REF_TRANSACTION_OPEN)
>  		die("BUG: create called for transaction that is not open");
>  
>  	if (!new_sha1 || is_null_sha1(new_sha1))
>  		die("BUG: create ref with null new_sha1");
>  
> -	update = add_update(transaction, refname);
> -
> -	hashcpy(update->new_sha1, new_sha1);
> -	hashclr(update->old_sha1);
> -	update->flags = flags;
> -	update->have_old = 1;
> -	if (msg)
> -		update->msg = xstrdup(msg);
> -	return 0;
> +	return ref_transaction_update(transaction, refname, new_sha1,
> +				      null_sha1, flags, 1, msg, err);
>  }

Hmmm.

This probably logically belongs to the end of the previous series
and not necessarily tied to reflog transaction, no?

At the beginning of ref_transaction_update() there also is the same
guard on transaction->state, and having both feels somewhat iffy.
Of course it will give a wrong BUG message if we removed the check
from this function, so perhaps the code is OK as-is.

>  int ref_transaction_delete(struct ref_transaction *transaction,
> diff --git a/refs.h b/refs.h
> index db463d0..495740d 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -283,9 +283,10 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
>  /*
>   * Add a reference update to transaction.  new_sha1 is the value that
>   * the reference should have after the update, or zeros if it should
> - * 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.
> + * be deleted.  If have_old is true and old_sha is not the null_sha1
> + * then the previous value of the ref must match or the update will fail.
> + * If have_old is true and old_sha1 is the null_sha1 then the ref must not
> + * already exist and a new ref will be created with new_sha1.
>   * 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.

Re: [PATCH v3 04/14] refs.c: add a new update_type field to ref_update

[Junio C Hamano]

Ronnie Sahlberg <sahlberg@google.com> writes:

> Add a field that describes what type of update this refers to. For now
> the only type is UPDATE_SHA1 but we will soon add more types.
>
> 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 4e3d4c3..4129de6 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3374,6 +3374,10 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
>  	return retval;
>  }
>  
> +enum transaction_update_type {
> +       UPDATE_SHA1 = 0,

indent with SP?

Unlike an array initialisation, e.g.

	int foo[] = { 1,2,3,4,5, };

some compilers we support complain if enum definition ends with a
trailing comma.

Re* [PATCH v3 04/14] refs.c: add a new update_type field to ref_update

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Ronnie Sahlberg <sahlberg@google.com> writes:
>
>> Add a field that describes what type of update this refers to. For now
>> the only type is UPDATE_SHA1 but we will soon add more types.
>>
>> 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 4e3d4c3..4129de6 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3374,6 +3374,10 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
>>  	return retval;
>>  }
>>  
>> +enum transaction_update_type {
>> +       UPDATE_SHA1 = 0,
>
> indent with SP?
>
> Unlike an array initialisation, e.g.
>
> 	int foo[] = { 1,2,3,4,5, };
>
> some compilers we support complain if enum definition ends with a
> trailing comma.

I do recall we had fixes to drop the comma after the last element in
enum definition in the past, in response real compilation breakages
on some platforms.  But there is a curious thing:

    git grep -A<somenumber> 'enum ' master -- \*.c

tells me that builtin/clean.c would fail to compile for those folks.

Here is an off-topic "fix" that may no longer be needed.

 builtin/clean.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 9a91515..27701d2 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -48,7 +48,7 @@ enum color_clean {
 	CLEAN_COLOR_PROMPT = 2,
 	CLEAN_COLOR_HEADER = 3,
 	CLEAN_COLOR_HELP = 4,
-	CLEAN_COLOR_ERROR = 5,
+	CLEAN_COLOR_ERROR = 5
 };
 
 #define MENU_OPTS_SINGLETON		01

Re: Re* [PATCH v3 04/14] refs.c: add a new update_type field to ref_update

[Ronnie Sahlberg]

On Wed, Jun 18, 2014 at 2:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Ronnie Sahlberg <sahlberg@google.com> writes:
>>
>>> Add a field that describes what type of update this refers to. For now
>>> the only type is UPDATE_SHA1 but we will soon add more types.
>>>
>>> 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 4e3d4c3..4129de6 100644
>>> --- a/refs.c
>>> +++ b/refs.c
>>> @@ -3374,6 +3374,10 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
>>>      return retval;
>>>  }
>>>
>>> +enum transaction_update_type {
>>> +       UPDATE_SHA1 = 0,
>>
>> indent with SP?
>>
>> Unlike an array initialisation, e.g.
>>
>>       int foo[] = { 1,2,3,4,5, };
>>
>> some compilers we support complain if enum definition ends with a
>> trailing comma.
>
> I do recall we had fixes to drop the comma after the last element in
> enum definition in the past, in response real compilation breakages
> on some platforms.

You are right. I think I recall this too on old c compilers on legacy
unix boxens.

I have fixed this and will resend the series.


I have also checked additional enums in the sources and found 3 more places.
I sent this as a single patch to the list with the subject :

"Subject: [PATCH] enums: remove trailing ',' after last item in enum"



>  But there is a curious thing:
>
>     git grep -A<somenumber> 'enum ' master -- \*.c
>
> tells me that builtin/clean.c would fail to compile for those folks.

Most likely.  It might be that there are less and less people on
really old c-compilers these days.


>
> Here is an off-topic "fix" that may no longer be needed.
>
>  builtin/clean.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/clean.c b/builtin/clean.c
> index 9a91515..27701d2 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -48,7 +48,7 @@ enum color_clean {
>         CLEAN_COLOR_PROMPT = 2,
>         CLEAN_COLOR_HEADER = 3,
>         CLEAN_COLOR_HELP = 4,
> -       CLEAN_COLOR_ERROR = 5,
> +       CLEAN_COLOR_ERROR = 5
>  };
>
>  #define MENU_OPTS_SINGLETON            01
>

Thanks
ronnie sahlberg

[PATCH v3 00/14] ref-transactions-reflog

[Stefan Beller]

Hi,

The following patch series updates the reflog handling to use transactions.
This patch series has previously been sent to the list[1].

This series converts the reflog handling and builtin/reflog.c to use
a transaction for both the ref as well as the reflog updates.
As a side effect of this it simplifies the reflog marshalling code so that we
only have one place where we marshall the entry.
It also means that we can remove several functions from the public api
towards the end of the series since we no longer need those functions.

This series can also be found at github[2] or at googlesource[3].
Feel free to review, where it suits you best.


Version 3:
 * Go over the commit messages and reword them slightly where appropriate.
   (only cosmetics, like missing/double words, spelling, clarify)
 * As Ronnie announced to change employers soon, he'll have only limited 
   time to work on git in the near future. As this is a rather large patch
   series, he is handing this work over to me. That's why I'm sending the
   patches this time.

Thanks,
Stefan

[1] http://www.spinics.net/lists/git/msg241186.html
[2] https://github.com/stefanbeller/git/tree/ref-transactions-reflog
[3] https://code-review.googlesource.com/#/q/topic:ref-transaction-reflog

Ronnie Sahlberg (14):
  refs.c: make ref_transaction_create a wrapper for
    ref_transaction_update
  refs.c: make ref_transaction_delete a wrapper for
    ref_transaction_update
  refs.c: rename the transaction functions
  refs.c: add a function to append a reflog entry to a fd
  refs.c: add a new update_type field to ref_update
  refs.c: add a transaction function to append a reflog entry
  refs.c: add a flag to allow reflog updates to truncate the log
  refs.c: only write reflog update if msg is non-NULL
  refs.c: allow multiple reflog updates during a single transaction
  reflog.c: use a reflog transaction when writing during expire
  refs.c: rename log_ref_setup to create_reflog
  refs.c: Remove unlock_ref/close_ref/commit_ref from the refs api
  refs.c: remove lock_any_ref_for_update
  refs.c: allow deleting refs with a broken sha1

 branch.c                    |  13 +-
 builtin/branch.c            |   5 +-
 builtin/checkout.c          |   8 +-
 builtin/commit.c            |  10 +-
 builtin/fetch.c             |  12 +-
 builtin/receive-pack.c      |  13 +-
 builtin/reflog.c            |  85 ++++------
 builtin/replace.c           |  10 +-
 builtin/tag.c               |  10 +-
 builtin/update-ref.c        |  26 +--
 cache.h                     |   7 +
 fast-import.c               |  22 +--
 refs.c                      | 403 +++++++++++++++++++++++++++++---------------
 refs.h                      |  87 +++++-----
 sequencer.c                 |  12 +-
 t/t1402-check-ref-format.sh |   8 +
 walker.c                    |  10 +-
 17 files changed, 440 insertions(+), 301 deletions(-)

-- 
2.2.0.rc2.5.gf7b9fb2

Re: [PATCH v3 00/14] ref-transactions-reflog

[Michael Haggerty]

On 11/18/2014 02:35 AM, Stefan Beller wrote:
> The following patch series updates the reflog handling to use transactions.
> This patch series has previously been sent to the list[1].
> [...]

I was reviewing this patch series (I left some comments in Gerrit about
the first few patches) when I realized that I'm having trouble
understanding the big picture of where you want to go with this. I have
the feeling that the operations that you are implementing are at too low
a level of abstraction.

What are the elementary write operations that are needed for a reflog?
Off the top of my head,

1. Add a reflog entry when a reference is updated in a transaction.
2. Rename a reflog file when the corresponding reference is renamed.
3. Delete the reflog when the corresponding reference is deleted [1].
4. Configure a reference to be reflogged.
5. Configure a reference to not be reflogged anymore and delete any
   existing reflog.
6. Selectively expire old reflog entries, e.g., based on their age.

Have I forgotten any?

The first three should be side-effects of the corresponding reference
updates. Aside from the fact that renames are not yet done within a
transaction, I think this is already the case.

Number 4, I think, currently only happens in conjunction with adding a
line to the reflog. So it could be implemented, say, as a
FORCE_CREATE_REFLOG flag on a ref_update within a transaction.

Number 5 is not very interesting, I think. For example, it could be a
separate API function, disconnected from any transactions.

Number 6 is more interesting, and from my quick reading, it looks like a
lot of the work of this patch series is to allow number 6 to be
implemented in builtin/reflog.c:expire_reflog(). But it seems to me that
you are building API calls at the wrong level of abstraction. Expiring a
reflog should be a single API call to the refs API, and ultimately it
should be left up to the refs backend to decide how to implement it. For
a filesystem-based backend, it would do what it does now. But (for
example) a SQL-based backend might implement this as a single SELECT
statement.

I also don't have the feeling that reflog expiration has to be done
within a ref_transaction. For example, is there ever a reason to combine
expiration with other reference updates in a single atomic transaction?
I think not.

So it seems to me that it would be more practical to have a separate API
function that is called to expire selected entries from a reflog [2],
unconnected with any transaction.

I am not nearly as steeped in this code as you and Ronnie, and it could
be that I'm forgetting lots of details that make your design preferable.
But other reviewers are probably in the same boat. So I think it would
be really helpful if you would provide a high-level description of the
API that you are proposing, and some discussion of its design and
tradeoffs. A big part of this description could go straight into a file
Documentation/technical/api-ref-transactions.txt, which will be a great
(and necessary) resource soon anyway.

Michael

[1] Though hopefully there will be future reference backends that don't
have to discard reflogs when a reference is deleted, so let's not bake
this behavior too fundamentally into the API.

[2] ...and/or possibly one to expire reflogs for multiple references, if
performance would benefit significantly.

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v3 00/14] ref-transactions-reflog

[Ronnie Sahlberg]

On Tue, Nov 18, 2014 at 3:26 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 11/18/2014 02:35 AM, Stefan Beller wrote:
>> The following patch series updates the reflog handling to use transactions.
>> This patch series has previously been sent to the list[1].
>> [...]
>
> I was reviewing this patch series (I left some comments in Gerrit about
> the first few patches) when I realized that I'm having trouble
> understanding the big picture of where you want to go with this. I have
> the feeling that the operations that you are implementing are at too low
> a level of abstraction.
>
> What are the elementary write operations that are needed for a reflog?
> Off the top of my head,
>
> 1. Add a reflog entry when a reference is updated in a transaction.
> 2. Rename a reflog file when the corresponding reference is renamed.
> 3. Delete the reflog when the corresponding reference is deleted [1].
> 4. Configure a reference to be reflogged.
> 5. Configure a reference to not be reflogged anymore and delete any
>    existing reflog.
> 6. Selectively expire old reflog entries, e.g., based on their age.
>
> Have I forgotten any?
>
> The first three should be side-effects of the corresponding reference
> updates. Aside from the fact that renames are not yet done within a
> transaction, I think this is already the case.
>
> Number 4, I think, currently only happens in conjunction with adding a
> line to the reflog. So it could be implemented, say, as a
> FORCE_CREATE_REFLOG flag on a ref_update within a transaction.
>
> Number 5 is not very interesting, I think. For example, it could be a
> separate API function, disconnected from any transactions.
>
> Number 6 is more interesting, and from my quick reading, it looks like a
> lot of the work of this patch series is to allow number 6 to be
> implemented in builtin/reflog.c:expire_reflog(). But it seems to me that
> you are building API calls at the wrong level of abstraction. Expiring a
> reflog should be a single API call to the refs API, and ultimately it
> should be left up to the refs backend to decide how to implement it. For
> a filesystem-based backend, it would do what it does now. But (for
> example) a SQL-based backend might implement this as a single SELECT
> statement.

I agree in principle. But things are more difficult since
expire_reflog() has very complex semantics.
To keep things simple for the reviews at this stage the logic is the
same as the original code:
  loop over all entries:
     use very complex conditionals to decide which entries to keep/remove
     optionally modify the sha1 values for the records we keep
     write records we keep back to the file one record at a time

So that as far as possible, we keep the same rules and behavior but we
use a different API for the actual
"write entry to new reflog".


We could wrap this inside a new specific transaction_expire_reflog()
function so that other types of backends, for example an SQL backend,
could optimize, but I think that should be in a separate later patch
because expire_reflog is almost impossibly complex.
It will not be a simple SELECT unfortunately.

The current expire logic is something like :
  1, expire all entries older than timestamp
  2, optionally, also expire all entries that refer to unreachable
objects using a different timestamp
      This involves actually reading the objects that the sha1 points
to and parsing them!
  3, optionally, if the sha1 objects can not be referenced, they are
not commit objects or if they don't exist, then expire them too.
      This also involves reading the objects behind the sha1.
  4, optionally, delete reflog entry #foo
  5, optionally, if any log entries were discarded due to 2,3,4 then
we might also re-write and modify some of the reflog entries we keep.
or any combination thereof

  (6, if --dry-run is specified, just print what we would have expired)


2 and 3 requires that we need to read the objects for the entry
4 allows us to delete a specific entry
5 means that even for entries we keep we will need to mutate them.






>
> I also don't have the feeling that reflog expiration has to be done
> within a ref_transaction. For example, is there ever a reason to combine
> expiration with other reference updates in a single atomic transaction?

--updateref
In expire_reflog() we not only prune the reflog. When --updateref is
used we update the actual ref itself.
I think we want to have both the ref update and also the reflog update
both be part of a single atomic transaction.


> I think not.
>
> So it seems to me that it would be more practical to have a separate API
> function that is called to expire selected entries from a reflog [2],
> unconnected with any transaction.

I think it makes the API cleaner if we have a
'you can only update a ref/reflog/<other things added in the future>/
from within a transaction.'

Since we need to do reflog changes within a transaction for the expire
reflog case as well as the rename ref case
I think it makes sense to enforce that reflog changes must be done
within a transaction to just make it consistent.



>
> I am not nearly as steeped in this code as you and Ronnie, and it could
> be that I'm forgetting lots of details that make your design preferable.
> But other reviewers are probably in the same boat. So I think it would
> be really helpful if you would provide a high-level description of the
> API that you are proposing, and some discussion of its design and
> tradeoffs. A big part of this description could go straight into a file
> Documentation/technical/api-ref-transactions.txt, which will be a great
> (and necessary) resource soon anyway.
>
> Michael
>
> [1] Though hopefully there will be future reference backends that don't
> have to discard reflogs when a reference is deleted, so let's not bake
> this behavior too fundamentally into the API.
>
> [2] ...and/or possibly one to expire reflogs for multiple references, if
> performance would benefit significantly.
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 00/14] ref-transactions-reflog

[Michael Haggerty]

On 11/18/2014 07:36 PM, Ronnie Sahlberg wrote:
> On Tue, Nov 18, 2014 at 3:26 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> On 11/18/2014 02:35 AM, Stefan Beller wrote:
>>> The following patch series updates the reflog handling to use transactions.
>>> This patch series has previously been sent to the list[1].
>>> [...]
>>
>> I was reviewing this patch series (I left some comments in Gerrit about
>> the first few patches) when I realized that I'm having trouble
>> understanding the big picture of where you want to go with this. I have
>> the feeling that the operations that you are implementing are at too low
>> a level of abstraction.
>>
>> What are the elementary write operations that are needed for a reflog?
>> Off the top of my head,
>>
>> 1. Add a reflog entry when a reference is updated in a transaction.
>> 2. Rename a reflog file when the corresponding reference is renamed.
>> 3. Delete the reflog when the corresponding reference is deleted [1].
>> 4. Configure a reference to be reflogged.
>> 5. Configure a reference to not be reflogged anymore and delete any
>>    existing reflog.
>> 6. Selectively expire old reflog entries, e.g., based on their age.
>>
>> Have I forgotten any?
>>
>> The first three should be side-effects of the corresponding reference
>> updates. Aside from the fact that renames are not yet done within a
>> transaction, I think this is already the case.
>>
>> Number 4, I think, currently only happens in conjunction with adding a
>> line to the reflog. So it could be implemented, say, as a
>> FORCE_CREATE_REFLOG flag on a ref_update within a transaction.
>>
>> Number 5 is not very interesting, I think. For example, it could be a
>> separate API function, disconnected from any transactions.
>>
>> Number 6 is more interesting, and from my quick reading, it looks like a
>> lot of the work of this patch series is to allow number 6 to be
>> implemented in builtin/reflog.c:expire_reflog(). But it seems to me that
>> you are building API calls at the wrong level of abstraction. Expiring a
>> reflog should be a single API call to the refs API, and ultimately it
>> should be left up to the refs backend to decide how to implement it. For
>> a filesystem-based backend, it would do what it does now. But (for
>> example) a SQL-based backend might implement this as a single SELECT
>> statement.
> 
> I agree in principle. But things are more difficult since
> expire_reflog() has very complex semantics.
> To keep things simple for the reviews at this stage the logic is the
> same as the original code:
>   loop over all entries:
>      use very complex conditionals to decide which entries to keep/remove
>      optionally modify the sha1 values for the records we keep
>      write records we keep back to the file one record at a time
> 
> So that as far as possible, we keep the same rules and behavior but we
> use a different API for the actual
> "write entry to new reflog".
> 
> 
> We could wrap this inside a new specific transaction_expire_reflog()
> function so that other types of backends, for example an SQL backend,
> could optimize, but I think that should be in a separate later patch
> because expire_reflog is almost impossibly complex.
> It will not be a simple SELECT unfortunately.
> 
> The current expire logic is something like :
>   1, expire all entries older than timestamp
>   2, optionally, also expire all entries that refer to unreachable
> objects using a different timestamp
>       This involves actually reading the objects that the sha1 points
> to and parsing them!
>   3, optionally, if the sha1 objects can not be referenced, they are
> not commit objects or if they don't exist, then expire them too.
>       This also involves reading the objects behind the sha1.
>   4, optionally, delete reflog entry #foo
>   5, optionally, if any log entries were discarded due to 2,3,4 then
> we might also re-write and modify some of the reflog entries we keep.
> or any combination thereof
> 
>   (6, if --dry-run is specified, just print what we would have expired)
> 
> 
> 2 and 3 requires that we need to read the objects for the entry
> 4 allows us to delete a specific entry
> 5 means that even for entries we keep we will need to mutate them.

Thanks for the explanation. I now understand that it might be more than
a single SELECT statement.

Regarding the complicated rules for expiring reflogs (1, 2, 3, 4): For
now I think it would be fine for the new expire_reflog() API function to
take a callback function as an argument.

Regarding the stitching together of the survivors (5), it seems like the
API function would be the right place to handle that.

Regarding 6, it sounds like you could run the reflog entries through
your callback and report what it *would* have expired.

>> I also don't have the feeling that reflog expiration has to be done
>> within a ref_transaction. For example, is there ever a reason to combine
>> expiration with other reference updates in a single atomic transaction?
> 
> --updateref
> In expire_reflog() we not only prune the reflog. When --updateref is
> used we update the actual ref itself.
> I think we want to have both the ref update and also the reflog update
> both be part of a single atomic transaction.

ISTM that --updateref is another aspect of stitching together the
surviving reflog entries and could properly be done by the API
expire_reflog() function. Maybe the implementation would use an
*internal* transaction. But I still don't see a need for the caller to
be able to combine *arbitrary* reflog changes with *arbitrary* reference
updates in a single transaction, and the unneeded flexibility seems to
require the API to become more complicated than necessary.

>> I think not.
>>
>> So it seems to me that it would be more practical to have a separate API
>> function that is called to expire selected entries from a reflog [2],
>> unconnected with any transaction.
> 
> I think it makes the API cleaner if we have a
> 'you can only update a ref/reflog/<other things added in the future>/
> from within a transaction.'
> 
> Since we need to do reflog changes within a transaction for the expire
> reflog case as well as the rename ref case
> I think it makes sense to enforce that reflog changes must be done
> within a transaction to just make it consistent.

I'm still not convinced. For me, "reflog_expire()" is an unusual outlier
operation, much like "git gc" or "git pack-refs" or "git fsck". None of
these are part of the beautiful Git data model; they are messy
maintenance operations. Forcing reference transactions to be general
enough to allow reflog expiration to be implemented *outside* the refs
API sacrificies their simplicity for lots of infrastructure that will
probably only be used to implement this single operation. Better to
implement reflog expiration *inside* the refs API.

That's my take on it, anyway.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v3 00/14] ref-transactions-reflog

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> I'm still not convinced. For me, "reflog_expire()" is an unusual outlier
> operation, much like "git gc" or "git pack-refs" or "git fsck". None of
> these are part of the beautiful Git data model; they are messy
> maintenance operations. Forcing reference transactions to be general
> enough to allow reflog expiration to be implemented *outside* the refs
> API sacrificies their simplicity for lots of infrastructure that will
> probably only be used to implement this single operation. Better to
> implement reflog expiration *inside* the refs API.

Sorry, but I lost track---which one is inside and which one is
outside?

Re: [PATCH v3 00/14] ref-transactions-reflog

[Michael Haggerty]

On 11/18/2014 09:30 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> I'm still not convinced. For me, "reflog_expire()" is an unusual outlier
>> operation, much like "git gc" or "git pack-refs" or "git fsck". None of
>> these are part of the beautiful Git data model; they are messy
>> maintenance operations. Forcing reference transactions to be general
>> enough to allow reflog expiration to be implemented *outside* the refs
>> API sacrificies their simplicity for lots of infrastructure that will
>> probably only be used to implement this single operation. Better to
>> implement reflog expiration *inside* the refs API.
> 
> Sorry, but I lost track---which one is inside and which one is
> outside?

By "inside" I mean the code that would be within the reference-handling
library if we had such a thing; i.e., implemented in refs.c. By
"outside" I mean in the code that calls the library; in this case the
"outside" code would live in builtin/reflog.c.

In other words, I'd prefer the "outside" code in builtin/reflog.c to
look vaguely like

    expire_reflogs_for_me_please(refname,
                                 should_expire_cb, cbdata, flags)

rather than

    transaction = ...
    for_each_reflog_entry {
        if should_expire()
            adjust neighbor reflog entries if necessary (actually,
                   they're transaction entries so we would have to
                   preprocess them before putting them in the
                   transaction)
        else
            add reflog entry to transaction
    }
    ref_transaction_commit()

and instead handle as much of the iteration, bookkeeping, and rewriting
as possible inside expire_reflogs_for_me_please().

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v3 00/14] ref-transactions-reflog

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

>> Sorry, but I lost track---which one is inside and which one is
>> outside?
>
> By "inside" I mean the code that would be within the reference-handling
> library if we had such a thing; i.e., implemented in refs.c. By
> "outside" I mean in the code that calls the library; in this case the
> "outside" code would live in builtin/reflog.c.
>
> In other words, I'd prefer the "outside" code in builtin/reflog.c to
> look vaguely like
>
>     expire_reflogs_for_me_please(refname,
>                                  should_expire_cb, cbdata, flags)
>
> rather than
> ... (written as a client of the ref API) ...

OK, I very much agree with that.

Re: [PATCH v3 00/14] ref-transactions-reflog

[Stefan Beller]

Sorry for the long delay.
Thanks for the explanation and discussion.

So do I understand it right, that you are not opposing
the introduction of "everything should go through transactions"
but rather the detail and abstraction level of the API?

So starting from Michaels proposal in the first response:

1. Add a reflog entry when a reference is updated in a transaction.

ok

2. Rename a reflog file when the corresponding reference is renamed.

This should happen within the same transaction as the reference is
renamed, right?
So we don't have a multistep process here, which may abort in between having the
reference updated and a broken reflog or vice versa. We want to either
have both
the ref and the reflog updated or neither.

3. Delete the reflog when the corresponding reference is deleted [1].

also as one transaction?

4. Configure a reference to be reflogged.
5. Configure a reference to not be reflogged anymore and delete any
   existing reflog.

Why do we need 4 and 5 here? Wouldn't all refs be reflog by default and
why do I want to exclude some?

6. Selectively expire old reflog entries, e.g., based on their age.

This is the maintenance operation, which you were talking about.
In my vision, this also should go into one transaction. So you have the
business logic figuring out all the changes ("drop reflog entry a b and d")
and within one transaction we can perform all of the changes.

Thanks,
Stefan


On Tue, Nov 18, 2014 at 1:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>>> Sorry, but I lost track---which one is inside and which one is
>>> outside?
>>
>> By "inside" I mean the code that would be within the reference-handling
>> library if we had such a thing; i.e., implemented in refs.c. By
>> "outside" I mean in the code that calls the library; in this case the
>> "outside" code would live in builtin/reflog.c.
>>
>> In other words, I'd prefer the "outside" code in builtin/reflog.c to
>> look vaguely like
>>
>>     expire_reflogs_for_me_please(refname,
>>                                  should_expire_cb, cbdata, flags)
>>
>> rather than
>> ... (written as a client of the ref API) ...
>
> OK, I very much agree with that.

Re: [PATCH v3 00/14] ref-transactions-reflog

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> Sorry for the long delay.
> Thanks for the explanation and discussion.
>
> So do I understand it right, that you are not opposing
> the introduction of "everything should go through transactions"
> but rather the detail and abstraction level of the API?

For what it's worth, I don't personally think it makes sense to put
the options supported by 'git reflog expire' into the transaction API
as top-level functions.

Instead, I think it makes sense, to start off, to support the same
building block operations that are used in the current file-based
code.  That may mean having an API that can't express tricks that e.g.
an SQL-based backend would be able to optimize (removing some items
from a reflog without copying the rest, filtering based on conditions
that can be expressed in SQL such as date, etc) but I think it's fine
as a starting point.  Later we can add new operations, change existing
ones, and so on, based on experience with real backends.

The write operations for file-based reflog handling are simple:

	- create a new reflog with a single reflog entry

	- add an entry to an existing reflog

	- (optional) copy a reflog wholesale --- this can be
	  implemented in terms of "add an entry", but copying in
	  blocks (or making a reflink, on filesystems that support
	  that) can make this faster

	- remove a reflog

The reflog bookkeeping involved in renaming a ref can be implemented
as copy + delete.

I also have some thoughts about how those operations can be
implemented without such a performance hit (reading the whole reflog
into memory as part of the transaction seems problematic to me), but
that should probably wait for a separate message (and I've talked
about it a bit in person).

[...]
> 4. Configure a reference to be reflogged.
> 5. Configure a reference to not be reflogged anymore and delete any
>    existing reflog.
>
> Why do we need 4 and 5 here? Wouldn't all refs be reflog by default and
> why do I want to exclude some?

See --create-reflog in git-branch(1) and core.logallrefupdates in
git-config(1).

Reflogs are disabled by default in bare repositories, which makes it
easier for unnecessary objects on a server to be more promptly removed
by gcs after a non-fast-forward push.  I prefer to turn on reflogs
when setting up a git server for my personal use.  It might be worth
flipping that default (as an orthogonal change).

> 6. Selectively expire old reflog entries, e.g., based on their age.
>
> This is the maintenance operation, which you were talking about.
> In my vision, this also should go into one transaction. So you have the
> business logic figuring out all the changes ("drop reflog entry a b and d")
> and within one transaction we can perform all of the changes.

Makes sense.

Thanks,
Jonathan

Re: [PATCH v3 00/14] ref-transactions-reflog

[Michael Haggerty]

On 11/20/2014 12:22 AM, Stefan Beller wrote:
> Sorry for the long delay.
> Thanks for the explanation and discussion.
> 
> So do I understand it right, that you are not opposing
> the introduction of "everything should go through transactions"
> but rather the detail and abstraction level of the API?

Correct.

> So starting from Michaels proposal in the first response:
> 
> 1. Add a reflog entry when a reference is updated in a transaction.
> 
> ok
> 
> 2. Rename a reflog file when the corresponding reference is renamed.
> 
> This should happen within the same transaction as the reference is
> renamed, right?

Yes. Maybe there should be a "rename reference" operation that can be
added to a transaction, and it simply knows to rename any associated
reflogs. Then the calling code wouldn't have to worry about reflogs
explicitly in this case at all.

> So we don't have a multistep process here, which may abort in between having the
> reference updated and a broken reflog or vice versa. We want to either
> have both
> the ref and the reflog updated or neither.

Yes.

> 3. Delete the reflog when the corresponding reference is deleted [1].
> 
> also as one transaction?

It would be a side-effect of committing a transaction that contains a
reference deletion. The deletion of the reflog would be done at the same
time that the rest of the transaction is committed, and again the
calling code wouldn't have to explicitly worry about the reflogs.

> 4. Configure a reference to be reflogged.
> 5. Configure a reference to not be reflogged anymore and delete any
>    existing reflog.
> 
> Why do we need 4 and 5 here? Wouldn't all refs be reflog by default and
> why do I want to exclude some?
> 
> 6. Selectively expire old reflog entries, e.g., based on their age.
> 
> This is the maintenance operation, which you were talking about.
> In my vision, this also should go into one transaction. So you have the
> business logic figuring out all the changes ("drop reflog entry a b and d")
> and within one transaction we can perform all of the changes.

But if we take the approach described above, AFAICT this operation is
the only one that would require the caller to manipulate reflog entries
explicitly. And it has to iterate through the old reflog entries, decide
which ones to keep, possibly change its neighbors to eliminate gaps in
the chain, then stuff each of the reflog entries into a transaction one
by one. To allow this to be implemented on the caller side, the
transaction API has to be complicated in the following ways:

* Add a transaction_update_type (UPDATE_SHA1 vs. UPDATE_LOG).
* Add reflog_fd, reflog_lock, and committer members to struct ref_update.
* New function transaction_update_reflog().
* A new flag REFLOG_TRUNCATE that allows the reflog file to be truncated
before writing.
* Machinery that recognizes that a transaction contains multiple reflog
updates for the same reference and processes them specially to avoid
locking and rewriting the reflog file multiple times.

So this design has the caller serializing all reflog entries into
separate ref_update structs (which implies that they are held in RAM!)
only for ref_transaction_commit() to scan through all ref_updates
looking for reflog updates that go together so that they can be
processed as a whole. In other words, the caller picks the reflog apart
and then ref_transaction_commit() glues it back together. It's all very
contrived.

I suggest that the caller only be responsible for deciding which reflog
entries to keep (by supplying a callback function), and a new
expire_reflogs_for_me_please() API function be responsible for taking
out a lock, feeding the old reflog entries to the callback, expiring the
unwanted entries, optionally eliminating gaps in the chain (for the use
of "reflog [expire|delete] --rewrite"), writing the new reflog entries,
and optionally updating the reference itself (for the use of "reflog
[expire|delete] --updateref").

The benefit will be simpler code, a better separation of
responsibilities, and a simpler VTABLE that future reference backends
have to implement.

I would love to work on this but unfortunately have way too much on my
plate right now.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v3 00/14] ref-transactions-reflog

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I also have some thoughts about how those operations can be
> implemented without such a performance hit (reading the whole
> reflog into memory as part of the transaction seems problematic to
> me), but that should probably wait for a separate message (and
> I've talked about it a bit in person).

Perhaps iterator interface such as for_each_reflog_ent() would help?

Re: [PATCH v3 00/14] ref-transactions-reflog

[Jonathan Nieder]

Michael Haggerty wrote:
> On 11/20/2014 12:22 AM, Stefan Beller wrote:

>> 3. Delete the reflog when the corresponding reference is deleted [1].
>>
>> also as one transaction?
>
> It would be a side-effect of committing a transaction that contains a
> reference deletion. The deletion of the reflog would be done at the same
> time that the rest of the transaction is committed, and again the
> calling code wouldn't have to explicitly worry about the reflogs.

There is "git reflog delete <ref>", for when you have logallrefupdates
disabled and had explicitly enabled reflogs for a particular ref and
now want to turn it off.

[...]
> So this design has the caller serializing all reflog entries into
> separate ref_update structs (which implies that they are held in RAM!)
> only for ref_transaction_commit() to scan through all ref_updates
> looking for reflog updates that go together so that they can be
> processed as a whole. In other words, the caller picks the reflog apart
> and then ref_transaction_commit() glues it back together. It's all very
> contrived.

I think there is a simpler and more efficient way to implement this.

transaction_update_reflog() can append to a .lock file.
transaction_commit() then would rename it into place.

There is some fuss about naming the .lock file to avoid D/F conflicts,
which is a topic for a separate message.

> I suggest that the caller only be responsible for deciding which reflog
> entries to keep (by supplying a callback function),

That could be handy.  The basic operations described before would still
be needed, though:

	create a new reflog with one entry, for new refs

	append an entry to a reflog, for ref updates (and the associated
		symref reflog update)

	copy (or rename --- that's a more minor detail) a reflog, for
		renaming refs

	delete a reflog, for "git reflog delete"

And the "filter reflog" operation you are describing is implementable
using those four operations, with no performance hit when dealing with
reflogs stored in the files backend.

Providing these operations doesn't prevent adding "filter reflog using
callback" later if it turns out to be the right operation for other
backends.  It could turn out that some other primitive operations that
are easy as an SQL operation is more useful, like "delete reflog
entry" (without iterating over the others) or "expire entries older
than <date>".  The nice thing is that adding those wouldn't break any
code using the initial four operations described above.  So this seems
like a good starting point.

[...]
> I would love to work on this but unfortunately have way too much on my
> plate right now.

Of course code is always an easy way to change my mind, when the time
comes. ;-)

Thanks,
Jonathan