[PATCH 0/8] Making reflog modifications part of the transactions API

[PATCH 0/8] Making reflog modifications part of the transactions API

[Stefan Beller]

This goes on top of Michaels series. The idea of this series is make the
reflogs being part of the transaction API, so it will be part of the contract
of transaction_commit to either commit all the changes or none at all.

Currently when using the transaction API to change refs, also reflogs are changed.
But the changes to the reflogs just happen as a side effect and not as part of
the atomic part of changes we want to commit altogether.

Ronnie Sahlberg (3):
  refs.c: use a reflog transaction when writing during expire
  refs.c: rename log_ref_setup to create_reflog
  refs.c: allow deleting refs with a broken sha1

Stefan Beller (5):
  refs.c: let fprintf handle the formatting
  refs.c: rename the transaction functions
  refs.c: rename transaction.updates to transaction.ref_updates
  refs.c: add transaction function to append to the reflog
  refs.c: add transaction function to delete the reflog

 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/replace.c      |  10 +-
 builtin/tag.c          |  10 +-
 builtin/update-ref.c   |  26 ++--
 cache.h                |   7 +
 fast-import.c          |  22 +--
 refs.c                 | 359 ++++++++++++++++++++++++++++++++++---------------
 refs.h                 |  43 +++---
 sequencer.c            |  12 +-
 t/t3200-branch.sh      |   8 ++
 walker.c               |  10 +-
 16 files changed, 359 insertions(+), 209 deletions(-)

-- 
2.2.0

[PATCH 1/8] refs.c: let fprintf handle the formatting

[Stefan Beller]

Instead of calculating, whether to put a plus or minus sign, offload
the responsibilty to the fprintf function.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    This patch was already sent as a single patch to the list,
    but not yet acknowledge in any way, so I am including it here.
    
    This series goes on top of Michaels series
    https://github.com/mhagger/git/tree/reflog-expire-api-v1

 refs.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index 40c5591..1147216 100644
--- a/refs.c
+++ b/refs.c
@@ -3972,12 +3972,9 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 			printf("prune %s", message);
 	} else {
 		if (cb->newlog) {
-			char sign = (tz < 0) ? '-' : '+';
-			int zone = (tz < 0) ? (-tz) : tz;
-			fprintf(cb->newlog, "%s %s %s %lu %c%04d\t%s",
+			fprintf(cb->newlog, "%s %s %s %lu %+05d\t%s",
 				sha1_to_hex(osha1), sha1_to_hex(nsha1),
-				email, timestamp, sign, zone,
-				message);
+				email, timestamp, tz, message);
 			hashcpy(cb->last_kept_sha1, nsha1);
 		}
 		if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
-- 
2.2.0

[PATCH 2/8] refs.c: rename the transaction functions

[Stefan Beller]

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

The goal in the long term is to have different things running through
the transaction api, such as the .git/config file. The dream is to
have any possible modification passing the transaction api.
Even if we don't achieve this goal, this change still makes sense
as it's slightly easier to read without the ref_ prefixes.

[sb: cherry-picked on top of mhagger/reflog-expire-api-v1]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---

Notes:
    no changes since last sending it.

 branch.c               | 13 +++++----
 builtin/commit.c       | 10 +++----
 builtin/fetch.c        | 12 ++++----
 builtin/receive-pack.c | 13 ++++-----
 builtin/replace.c      | 10 +++----
 builtin/tag.c          | 10 +++----
 builtin/update-ref.c   | 26 ++++++++---------
 fast-import.c          | 22 +++++++-------
 refs.c                 | 78 +++++++++++++++++++++++++-------------------------
 refs.h                 | 35 +++++++++++-----------
 sequencer.c            | 12 ++++----
 walker.c               | 10 +++----
 12 files changed, 126 insertions(+), 125 deletions(-)

diff --git a/branch.c b/branch.c
index 4bab55a..c8462de 100644
--- a/branch.c
+++ b/branch.c
@@ -279,16 +279,17 @@ void create_branch(const char *head,
 		log_all_ref_updates = 1;
 
 	if (!dont_change_ref) {
-		struct ref_transaction *transaction;
+		struct 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_ref(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);
 		strbuf_release(&err);
 	}
 
diff --git a/builtin/commit.c b/builtin/commit.c
index e108c53..f50b7df 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1673,7 +1673,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	struct stat statbuf;
 	struct commit *current_head = NULL;
 	struct commit_extra_header *extra = NULL;
-	struct ref_transaction *transaction;
+	struct transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
@@ -1804,17 +1804,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,
+	    transaction_update_ref(transaction, "HEAD", sha1,
 				   current_head
 				   ? current_head->object.sha1 : NULL,
 				   0, !!current_head, sb.buf, &err) ||
-	    ref_transaction_commit(transaction, &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 7b84d35..0be0b09 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -404,7 +404,7 @@ static int s_update_ref(const char *action,
 {
 	char msg[1024];
 	char *rla = getenv("GIT_REFLOG_ACTION");
-	struct ref_transaction *transaction;
+	struct transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
 	int ret, df_conflict = 0;
 
@@ -414,23 +414,23 @@ 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,
+	    transaction_update_ref(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) {
 		df_conflict = (ret == TRANSACTION_NAME_CONFLICT);
 		goto fail;
 	}
 
-	ref_transaction_free(transaction);
+	transaction_free(transaction);
 	strbuf_release(&err);
 	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 32fc540..397abc9 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -838,26 +838,25 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 	}
 	else {
 		struct strbuf err = STRBUF_INIT;
-		struct ref_transaction *transaction;
+		struct transaction *transaction;
 
 		if (shallow_update && si->shallow_ref[cmd->index] &&
 		    update_shallow_ref(cmd, si))
 			return "shallow error";
 
-		transaction = ref_transaction_begin(&err);
+		transaction = transaction_begin(&err);
 		if (!transaction ||
-		    ref_transaction_update(transaction, namespaced_name,
+		    transaction_update_ref(transaction, namespaced_name,
 					   new_sha1, old_sha1, 0, 1, "push",
 					   &err) ||
-		    ref_transaction_commit(transaction, &err)) {
-			ref_transaction_free(transaction);
-
+		    transaction_commit(transaction, &err)) {
+			transaction_free(transaction);
 			rp_error("%s", err.buf);
 			strbuf_release(&err);
 			return "failed to update ref";
 		}
 
-		ref_transaction_free(transaction);
+		transaction_free(transaction);
 		strbuf_release(&err);
 		return NULL; /* good */
 	}
diff --git a/builtin/replace.c b/builtin/replace.c
index 85d39b5..5a7ab1f 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -155,7 +155,7 @@ static int replace_object_sha1(const char *object_ref,
 	unsigned char prev[20];
 	enum object_type obj_type, repl_type;
 	char ref[PATH_MAX];
-	struct ref_transaction *transaction;
+	struct transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
 
 	obj_type = sha1_object_info(object, NULL);
@@ -169,14 +169,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,
+	    transaction_update_ref(transaction, ref, repl, prev,
 				   0, 1, NULL, &err) ||
-	    ref_transaction_commit(transaction, &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 e633f4e..5f3554b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -583,7 +583,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	const char *msgfile = NULL, *keyid = NULL;
 	struct msg_arg msg = { 0, STRBUF_INIT };
 	struct commit_list *with_commit = NULL;
-	struct ref_transaction *transaction;
+	struct transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
@@ -730,13 +730,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,
+	    transaction_update_ref(transaction, ref.buf, object, prev,
 				   0, 1, NULL, &err) ||
-	    ref_transaction_commit(transaction, &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 6c9be05..af08dd9 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -175,7 +175,7 @@ static int parse_next_sha1(struct strbuf *input, const char **next,
  * depending on how line_termination is set.
  */
 
-static const char *parse_cmd_update(struct ref_transaction *transaction,
+static const char *parse_cmd_update(struct transaction *transaction,
 				    struct strbuf *input, const char *next)
 {
 	struct strbuf err = STRBUF_INIT;
@@ -198,7 +198,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 	if (*next != line_termination)
 		die("update %s: extra input: %s", refname, next);
 
-	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+	if (transaction_update_ref(transaction, refname, new_sha1, old_sha1,
 				   update_flags, have_old, msg, &err))
 		die("%s", err.buf);
 
@@ -209,7 +209,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 	return next;
 }
 
-static const char *parse_cmd_create(struct ref_transaction *transaction,
+static const char *parse_cmd_create(struct transaction *transaction,
 				    struct strbuf *input, const char *next)
 {
 	struct strbuf err = STRBUF_INIT;
@@ -229,7 +229,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 	if (*next != line_termination)
 		die("create %s: extra input: %s", refname, next);
 
-	if (ref_transaction_create(transaction, refname, new_sha1,
+	if (transaction_create_ref(transaction, refname, new_sha1,
 				   update_flags, msg, &err))
 		die("%s", err.buf);
 
@@ -240,7 +240,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 	return next;
 }
 
-static const char *parse_cmd_delete(struct ref_transaction *transaction,
+static const char *parse_cmd_delete(struct transaction *transaction,
 				    struct strbuf *input, const char *next)
 {
 	struct strbuf err = STRBUF_INIT;
@@ -264,7 +264,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 	if (*next != line_termination)
 		die("delete %s: extra input: %s", refname, next);
 
-	if (ref_transaction_delete(transaction, refname, old_sha1,
+	if (transaction_delete_ref(transaction, refname, old_sha1,
 				   update_flags, have_old, msg, &err))
 		die("%s", err.buf);
 
@@ -275,7 +275,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 	return next;
 }
 
-static const char *parse_cmd_verify(struct ref_transaction *transaction,
+static const char *parse_cmd_verify(struct transaction *transaction,
 				    struct strbuf *input, const char *next)
 {
 	struct strbuf err = STRBUF_INIT;
@@ -300,7 +300,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 	if (*next != line_termination)
 		die("verify %s: extra input: %s", refname, next);
 
-	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+	if (transaction_update_ref(transaction, refname, new_sha1, old_sha1,
 				   update_flags, have_old, msg, &err))
 		die("%s", err.buf);
 
@@ -320,7 +320,7 @@ static const char *parse_cmd_option(struct strbuf *input, const char *next)
 	return next + 8;
 }
 
-static void update_refs_stdin(struct ref_transaction *transaction)
+static void update_refs_stdin(struct transaction *transaction)
 {
 	struct strbuf input = STRBUF_INIT;
 	const char *next;
@@ -376,9 +376,9 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 
 	if (read_stdin) {
 		struct strbuf err = STRBUF_INIT;
-		struct ref_transaction *transaction;
+		struct transaction *transaction;
 
-		transaction = ref_transaction_begin(&err);
+		transaction = transaction_begin(&err);
 		if (!transaction)
 			die("%s", err.buf);
 		if (delete || no_deref || argc > 0)
@@ -386,9 +386,9 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		if (end_null)
 			line_termination = '\0';
 		update_refs_stdin(transaction);
-		if (ref_transaction_commit(transaction, &err))
+		if (transaction_commit(transaction, &err))
 			die("%s", err.buf);
-		ref_transaction_free(transaction);
+		transaction_free(transaction);
 		strbuf_release(&err);
 		return 0;
 	}
diff --git a/fast-import.c b/fast-import.c
index d0bd285..152d944 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1687,7 +1687,7 @@ found_entry:
 static int update_branch(struct branch *b)
 {
 	static const char *msg = "fast-import";
-	struct ref_transaction *transaction;
+	struct transaction *transaction;
 	unsigned char old_sha1[20];
 	struct strbuf err = STRBUF_INIT;
 
@@ -1713,17 +1713,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,
+	    transaction_update_ref(transaction, b->name, b->sha1, old_sha1,
 				   0, 1, msg, &err) ||
-	    ref_transaction_commit(transaction, &err)) {
-		ref_transaction_free(transaction);
+	    transaction_commit(transaction, &err)) {
+		transaction_free(transaction);
 		error("%s", err.buf);
 		strbuf_release(&err);
 		return -1;
 	}
-	ref_transaction_free(transaction);
+	transaction_free(transaction);
 	strbuf_release(&err);
 	return 0;
 }
@@ -1745,9 +1745,9 @@ static void dump_tags(void)
 	struct tag *t;
 	struct strbuf ref_name = STRBUF_INIT;
 	struct strbuf err = STRBUF_INIT;
-	struct ref_transaction *transaction;
+	struct transaction *transaction;
 
-	transaction = ref_transaction_begin(&err);
+	transaction = transaction_begin(&err);
 	if (!transaction) {
 		failure |= error("%s", err.buf);
 		goto cleanup;
@@ -1756,17 +1756,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,
+		if (transaction_update_ref(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 1147216..9f2628b 100644
--- a/refs.c
+++ b/refs.c
@@ -35,7 +35,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_ref when a loose ref is being
  * pruned.
  */
 #define REF_ISPRUNING	0x0100
@@ -2545,23 +2545,23 @@ static void try_remove_empty_parents(char *name)
 /* make sure nobody touched the ref, and unlink */
 static void prune_ref(struct ref_to_prune *r)
 {
-	struct ref_transaction *transaction;
+	struct transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
 
 	if (check_refname_format(r->name, 0))
 		return;
 
-	transaction = ref_transaction_begin(&err);
+	transaction = transaction_begin(&err);
 	if (!transaction ||
-	    ref_transaction_delete(transaction, r->name, r->sha1,
+	    transaction_delete_ref(transaction, r->name, r->sha1,
 				   REF_ISPRUNING, 1, NULL, &err) ||
-	    ref_transaction_commit(transaction, &err)) {
-		ref_transaction_free(transaction);
+	    transaction_commit(transaction, &err)) {
+		transaction_free(transaction);
 		error("%s", err.buf);
 		strbuf_release(&err);
 		return;
 	}
-	ref_transaction_free(transaction);
+	transaction_free(transaction);
 	strbuf_release(&err);
 	try_remove_empty_parents(r->name);
 }
@@ -2723,20 +2723,20 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
 
 int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 {
-	struct ref_transaction *transaction;
+	struct 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,
+	    transaction_delete_ref(transaction, refname, sha1, delopt,
 				   sha1 && !is_null_sha1(sha1), NULL, &err) ||
-	    ref_transaction_commit(transaction, &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);
 	strbuf_release(&err);
 	return 0;
 }
@@ -3545,9 +3545,9 @@ struct ref_update {
  *         an active transaction or if there is a failure while building
  *         the transaction thus rendering it failed/inactive.
  */
-enum ref_transaction_state {
-	REF_TRANSACTION_OPEN   = 0,
-	REF_TRANSACTION_CLOSED = 1
+enum transaction_state {
+	TRANSACTION_OPEN   = 0,
+	TRANSACTION_CLOSED = 1
 };
 
 /*
@@ -3555,21 +3555,21 @@ enum ref_transaction_state {
  * consist of checks and updates to multiple references, carried out
  * as atomically as possible.  This structure is opaque to callers.
  */
-struct ref_transaction {
+struct transaction {
 	struct ref_update **updates;
 	size_t alloc;
 	size_t nr;
-	enum ref_transaction_state state;
+	enum transaction_state state;
 };
 
-struct ref_transaction *ref_transaction_begin(struct strbuf *err)
+struct transaction *transaction_begin(struct strbuf *err)
 {
 	assert(err);
 
-	return xcalloc(1, sizeof(struct ref_transaction));
+	return xcalloc(1, sizeof(struct transaction));
 }
 
-void ref_transaction_free(struct ref_transaction *transaction)
+void transaction_free(struct transaction *transaction)
 {
 	int i;
 
@@ -3584,7 +3584,7 @@ void ref_transaction_free(struct ref_transaction *transaction)
 	free(transaction);
 }
 
-static struct ref_update *add_update(struct ref_transaction *transaction,
+static struct ref_update *add_update(struct transaction *transaction,
 				     const char *refname)
 {
 	size_t len = strlen(refname);
@@ -3596,7 +3596,7 @@ static struct ref_update *add_update(struct ref_transaction *transaction,
 	return update;
 }
 
-int ref_transaction_update(struct ref_transaction *transaction,
+int transaction_update_ref(struct transaction *transaction,
 			   const char *refname,
 			   const unsigned char *new_sha1,
 			   const unsigned char *old_sha1,
@@ -3607,7 +3607,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
 
 	assert(err);
 
-	if (transaction->state != REF_TRANSACTION_OPEN)
+	if (transaction->state != TRANSACTION_OPEN)
 		die("BUG: update called for transaction that is not open");
 
 	if (have_old && !old_sha1)
@@ -3631,23 +3631,23 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	return 0;
 }
 
-int ref_transaction_create(struct ref_transaction *transaction,
+int transaction_create_ref(struct transaction *transaction,
 			   const char *refname,
 			   const unsigned char *new_sha1,
 			   int flags, const char *msg,
 			   struct strbuf *err)
 {
-	return ref_transaction_update(transaction, refname, new_sha1,
+	return transaction_update_ref(transaction, refname, new_sha1,
 				      null_sha1, flags, 1, msg, err);
 }
 
-int ref_transaction_delete(struct ref_transaction *transaction,
+int transaction_delete_ref(struct transaction *transaction,
 			   const char *refname,
 			   const unsigned char *old_sha1,
 			   int flags, int have_old, const char *msg,
 			   struct strbuf *err)
 {
-	return ref_transaction_update(transaction, refname, null_sha1,
+	return transaction_update_ref(transaction, refname, null_sha1,
 				      old_sha1, flags, have_old, msg, err);
 }
 
@@ -3655,17 +3655,17 @@ int update_ref(const char *action, const char *refname,
 	       const unsigned char *sha1, const unsigned char *oldval,
 	       int flags, enum action_on_err onerr)
 {
-	struct ref_transaction *t;
+	struct 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,
+	    transaction_update_ref(t, refname, sha1, oldval, flags,
 				   !!oldval, action, &err) ||
-	    ref_transaction_commit(t, &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);
@@ -3680,7 +3680,7 @@ int update_ref(const char *action, const char *refname,
 		return 1;
 	}
 	strbuf_release(&err);
-	ref_transaction_free(t);
+	transaction_free(t);
 	return 0;
 }
 
@@ -3708,8 +3708,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 transaction *transaction,
+		       struct strbuf *err)
 {
 	int ret = 0, delnum = 0, i;
 	const char **delnames;
@@ -3718,11 +3718,11 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 
 	assert(err);
 
-	if (transaction->state != REF_TRANSACTION_OPEN)
+	if (transaction->state != TRANSACTION_OPEN)
 		die("BUG: commit called for transaction that is not open");
 
 	if (!n) {
-		transaction->state = REF_TRANSACTION_CLOSED;
+		transaction->state = TRANSACTION_CLOSED;
 		return 0;
 	}
 
@@ -3801,7 +3801,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	clear_loose_ref_cache(&ref_cache);
 
 cleanup:
-	transaction->state = REF_TRANSACTION_CLOSED;
+	transaction->state = TRANSACTION_CLOSED;
 
 	for (i = 0; i < n; i++)
 		if (updates[i]->lock)
diff --git a/refs.h b/refs.h
index 2957641..40607d6 100644
--- a/refs.h
+++ b/refs.h
@@ -2,22 +2,22 @@
 #define REFS_H
 
 /*
- * A ref_transaction represents a collection of ref updates
+ * A transaction represents a collection of ref updates
  * that should succeed or fail together.
  *
  * Calling sequence
  * ----------------
- * - Allocate and initialize a `struct ref_transaction` by calling
- *   `ref_transaction_begin()`.
+ * - Allocate and initialize a `struct transaction` by calling
+ *   `transaction_begin()`.
  *
  * - List intended ref updates by calling functions like
- *   `ref_transaction_update()` and `ref_transaction_create()`.
+ *   `transaction_update_ref()` and `transaction_create_ref()`.
  *
- * - 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.
@@ -33,7 +33,7 @@
  * The message is appended to err without first clearing err.
  * err will not be '\n' terminated.
  */
-struct ref_transaction;
+struct transaction;
 
 /*
  * Bit values set in the flags argument passed to each_ref_fn():
@@ -172,7 +172,8 @@ extern int is_branch(const char *refname);
 extern int peel_ref(const char *refname, unsigned char *sha1);
 
 /*
- * Flags controlling ref_transaction_update(), ref_transaction_create(), etc.
+ * Flags controlling lock_any_ref_for_update(), transaction_update_ref(),
+ * transaction_create_ref(), etc.
  * REF_NODEREF: act on the ref directly, instead of dereferencing
  *              symbolic references.
  * REF_DELETING: tolerate broken refs
@@ -244,13 +245,13 @@ 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 transaction *transaction_begin(struct strbuf *err);
 
 /*
  * The following functions add a reference check or update to a
- * ref_transaction.  In all of them, refname is the name of the
+ * transaction.  In all of them, refname is the name of the
  * reference to be affected.  The functions make internal copies of
  * refname and msg, so the caller retains ownership of these parameters.
  * flags can be REF_NODEREF; it is passed to update_ref_lock().
@@ -266,7 +267,7 @@ 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,
+int transaction_update_ref(struct transaction *transaction,
 			   const char *refname,
 			   const unsigned char *new_sha1,
 			   const unsigned char *old_sha1,
@@ -282,7 +283,7 @@ 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,
+int transaction_create_ref(struct transaction *transaction,
 			   const char *refname,
 			   const unsigned char *new_sha1,
 			   int flags, const char *msg,
@@ -296,7 +297,7 @@ 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,
+int transaction_delete_ref(struct transaction *transaction,
 			   const char *refname,
 			   const unsigned char *old_sha1,
 			   int flags, int have_old, const char *msg,
@@ -312,13 +313,13 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 #define TRANSACTION_NAME_CONFLICT -1
 /* All other errors. */
 #define TRANSACTION_GENERIC_ERROR -2
-int ref_transaction_commit(struct ref_transaction *transaction,
-			   struct strbuf *err);
+int transaction_commit(struct 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 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 a03d4fa..f888005 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -238,7 +238,7 @@ static int error_dirty_index(struct replay_opts *opts)
 static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 			int unborn, struct replay_opts *opts)
 {
-	struct ref_transaction *transaction;
+	struct transaction *transaction;
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf err = STRBUF_INIT;
 
@@ -248,13 +248,13 @@ 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",
+	    transaction_update_ref(transaction, "HEAD",
 				   to, unborn ? null_sha1 : from,
 				   0, 1, sb.buf, &err) ||
-	    ref_transaction_commit(transaction, &err)) {
-		ref_transaction_free(transaction);
+	    transaction_commit(transaction, &err)) {
+		transaction_free(transaction);
 		error("%s", err.buf);
 		strbuf_release(&sb);
 		strbuf_release(&err);
@@ -263,7 +263,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 
 	strbuf_release(&sb);
 	strbuf_release(&err);
-	ref_transaction_free(transaction);
+	transaction_free(transaction);
 	return 0;
 }
 
diff --git a/walker.c b/walker.c
index f149371..f1d5e9b 100644
--- a/walker.c
+++ b/walker.c
@@ -253,7 +253,7 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 {
 	struct strbuf refname = STRBUF_INIT;
 	struct strbuf err = STRBUF_INIT;
-	struct ref_transaction *transaction = NULL;
+	struct transaction *transaction = NULL;
 	unsigned char *sha1 = xmalloc(targets * 20);
 	char *msg = NULL;
 	int i, ret = -1;
@@ -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 done;
@@ -298,7 +298,7 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 			continue;
 		strbuf_reset(&refname);
 		strbuf_addf(&refname, "refs/%s", write_ref[i]);
-		if (ref_transaction_update(transaction, refname.buf,
+		if (transaction_update_ref(transaction, refname.buf,
 					   &sha1[20 * i], NULL, 0, 0,
 					   msg ? msg : "fetch (unknown)",
 					   &err)) {
@@ -306,7 +306,7 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 			goto done;
 		}
 	}
-	if (ref_transaction_commit(transaction, &err)) {
+	if (transaction_commit(transaction, &err)) {
 		error("%s", err.buf);
 		goto done;
 	}
@@ -314,7 +314,7 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 	ret = 0;
 
 done:
-	ref_transaction_free(transaction);
+	transaction_free(transaction);
 	free(msg);
 	free(sha1);
 	strbuf_release(&err);
-- 
2.2.0

Re: [PATCH 2/8] refs.c: rename the transaction functions

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Rename the transaction functions. Remove the leading ref_ from the
> names and append _ref to the names for functions that create/delete/
> update sha1 refs.
>
> The goal in the long term is to have different things running through
> the transaction api, such as the .git/config file.
>
> The dream is to...

I was trying to polish somebody else's topic that does not have
anythning to do with refs/reflogs/transactions today, and had to
suffer from conflicts from the textual difference between
transaction_begin and ref_transaction_begin, which I feel is totally
unnecessary at this moment.

I'd really prefer to see this done long after all the dust from
"ref" changes from various people have settled, especially given
that we are not doing anything for that "dream" right now.

Thanks.

Re: [PATCH 2/8] refs.c: rename the transaction functions

[Stefan Beller]

On Thu, Dec 11, 2014 at 1:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Rename the transaction functions. Remove the leading ref_ from the
>> names and append _ref to the names for functions that create/delete/
>> update sha1 refs.
>>
>> The goal in the long term is to have different things running through
>> the transaction api, such as the .git/config file.
>>
>> The dream is to...
>
> I was trying to polish somebody else's topic that does not have
> anythning to do with refs/reflogs/transactions today, and had to
> suffer from conflicts from the textual difference between
> transaction_begin and ref_transaction_begin, which I feel is totally
> unnecessary at this moment.
>
> I'd really prefer to see this done long after all the dust from
> "ref" changes from various people have settled, especially given
> that we are not doing anything for that "dream" right now.
>
> Thanks.

Good point. For the last days I was trying to write less invasive
patches as I guessed this may become a problem.

Feel free to drop these patches,
I'll resend them later if they still make sense.

[PATCH 3/8] refs.c: rename transaction.updates to transaction.ref_updates

[Stefan Beller]

The updates are only holding refs not reflogs, so express it to the reader.

Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---

Notes:
    no changes since last sending it.

 refs.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index 9f2628b..aad44d5 100644
--- a/refs.c
+++ b/refs.c
@@ -3556,7 +3556,7 @@ enum transaction_state {
  * as atomically as possible.  This structure is opaque to callers.
  */
 struct transaction {
-	struct ref_update **updates;
+	struct ref_update **ref_updates;
 	size_t alloc;
 	size_t nr;
 	enum transaction_state state;
@@ -3577,10 +3577,10 @@ void transaction_free(struct transaction *transaction)
 		return;
 
 	for (i = 0; i < transaction->nr; i++) {
-		free(transaction->updates[i]->msg);
-		free(transaction->updates[i]);
+		free(transaction->ref_updates[i]->msg);
+		free(transaction->ref_updates[i]);
 	}
-	free(transaction->updates);
+	free(transaction->ref_updates);
 	free(transaction);
 }
 
@@ -3591,8 +3591,8 @@ static struct ref_update *add_update(struct transaction *transaction,
 	struct ref_update *update = xcalloc(1, sizeof(*update) + len + 1);
 
 	strcpy((char *)update->refname, refname);
-	ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
-	transaction->updates[transaction->nr++] = update;
+	ALLOC_GROW(transaction->ref_updates, transaction->nr + 1, transaction->alloc);
+	transaction->ref_updates[transaction->nr++] = update;
 	return update;
 }
 
@@ -3714,7 +3714,7 @@ int transaction_commit(struct transaction *transaction,
 	int ret = 0, delnum = 0, i;
 	const char **delnames;
 	int n = transaction->nr;
-	struct ref_update **updates = transaction->updates;
+	struct ref_update **updates = transaction->ref_updates;
 
 	assert(err);
 
-- 
2.2.0

[PATCH 4/8] refs.c: add transaction function to append to the reflog

[Stefan Beller]

Currently a transaction can include one or more reflog updates. But these
are not part of the contract of the transaction committing all or nothing.

Introduce a function transaction_update_reflog, which adds a reflog update
to a transaction. Later patches will update code that writes to reflogs to
use this function.

This will allow us to write code such as:

	t = transaction_begin()
	transaction_truncate_reflog(t, "foo"); // introduced by next patch
	loop-over-something...
		if (want_reflog_entry(...))
			transaction_reflog_update(t, "foo", 0, <message>);
	transaction_commit(t)

transaction_ref_update still updates the reflog along with its ref update.
A later patch will make it use transaction_update_reflog internally.

Unlike transaction_update_ref, this writes out the proposed contents of the
reflog to a temporary file at transaction_reflog_update time instead of
waiting for the transaction waiting to be committed. This avoids an
explosion of memory usage when writing lots of reflog updates within a
single transaction.

This requires changing where the temporary file is located. If the temporary
file is located at $GIT_DIR/logs/$REF_NAME.lock, then a transaction that
tries to remove refs/heads/foo and introduce refs/heads/foo/bar would run
into a directory/file conflict when trying to write to
$GIT_DIR/logs/refs/heads/foo/bar.lock because the reflog for branch foo is
not safe to remove yet.

We work around this by placing the temporary files at
$GIT_DIR/logs/lock/$REF_NAME.lock Putting the temporary file under .git/logs
ensures it's likely that it will be in the same file system as the reflogs
and can be atomically renamed into place.

During one transaction we allow to make multiple reflog updates to the same
ref. 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.

transaction_update_reflog should lock the corresponding ref to avoid having
to compete with other reflog updates, but this will be deferred to a later
patch for simplicity.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    This has been sent to the list before, but now it is split up
    into this and the next patch.
    The commit message was overhauled, a huge thanks to Jonathan!
    
    There are no major changes in the code.

 refs.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 92 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index aad44d5..d767418 100644
--- a/refs.c
+++ b/refs.c
@@ -3559,19 +3559,30 @@ struct transaction {
 	struct ref_update **ref_updates;
 	size_t alloc;
 	size_t nr;
+
+	/*
+	 * Sorted list of reflogs to be committed.
+	 * the util points to the lock_file.
+	 */
+	struct string_list reflog_updates;
 	enum transaction_state state;
 };
 
 struct transaction *transaction_begin(struct strbuf *err)
 {
+	struct transaction *ret;
+
 	assert(err);
 
-	return xcalloc(1, sizeof(struct transaction));
+	ret = xcalloc(1, sizeof(struct transaction));
+	ret->reflog_updates.strdup_strings = 1;
+	return ret;
 }
 
 void transaction_free(struct transaction *transaction)
 {
 	int i;
+	struct string_list_item *item;
 
 	if (!transaction)
 		return;
@@ -3581,6 +3592,12 @@ void transaction_free(struct transaction *transaction)
 		free(transaction->ref_updates[i]);
 	}
 	free(transaction->ref_updates);
+
+	for_each_string_list_item(item, &transaction->reflog_updates) {
+		rollback_lock_file(item->util);
+	}
+	string_list_clear(&transaction->reflog_updates, 0);
+
 	free(transaction);
 }
 
@@ -3631,6 +3648,71 @@ int transaction_update_ref(struct transaction *transaction,
 	return 0;
 }
 
+/*
+ * Append a reflog entry for refname.
+ */
+static int transaction_update_reflog(struct transaction *transaction,
+				     const char *refname,
+				     const unsigned char *new_sha1,
+				     const unsigned char *old_sha1,
+				     const char *email,
+				     unsigned long timestamp, int tz,
+				     const char *msg,
+				     struct strbuf *err)
+{
+	struct lock_file *lock;
+	struct strbuf buf = STRBUF_INIT;
+	struct string_list_item *item;
+
+	if (transaction->state != TRANSACTION_OPEN)
+		die("BUG: update_reflog called for transaction that is not open");
+
+	item = string_list_insert(&transaction->reflog_updates, refname);
+	if (!item->util) {
+		int infd;
+		char *path = git_path("logs/locks/%s", refname);
+		lock = xcalloc(1, sizeof(struct lock_file));
+		item->util = lock;
+		if (safe_create_leading_directories(path)) {
+			strbuf_addf(err, "could not create leading directories of '%s': %s",
+				    path, strerror(errno));
+			goto failure;
+		}
+
+		if (hold_lock_file_for_update(lock, path, 0) < 0) {
+			unable_to_lock_message(path, errno, err);
+			goto failure;
+		}
+
+		/* copy existing reflog over */
+		if ((infd = open(git_path("logs/%s", refname), O_RDONLY)) < 0) {
+			strbuf_addf(err, "could not open the reflog '%s': %s",
+				    path, strerror(errno));
+			goto failure;
+		}
+		if (copy_fd(infd, lock->fd))
+			goto failure;
+		close(infd);
+	}
+	lock = item->util;
+
+	if (email)
+		strbuf_addf(&buf, "%s %lu %+05d", email, timestamp, tz);
+
+	if (log_ref_write_fd(lock->fd, old_sha1, new_sha1, buf.buf, msg)) {
+		strbuf_addf(err, "Could not write to reflog: '%s': %s",
+			    refname, strerror(errno));
+		goto failure;
+	}
+
+	strbuf_release(&buf);
+	return 0;
+failure:
+	strbuf_release(&buf);
+	transaction->state = TRANSACTION_CLOSED;
+	return -1;
+}
+
 int transaction_create_ref(struct transaction *transaction,
 			   const char *refname,
 			   const unsigned char *new_sha1,
@@ -3715,13 +3797,14 @@ int transaction_commit(struct transaction *transaction,
 	const char **delnames;
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->ref_updates;
+	struct string_list_item *item;
 
 	assert(err);
 
 	if (transaction->state != TRANSACTION_OPEN)
 		die("BUG: commit called for transaction that is not open");
 
-	if (!n) {
+	if (!n && !transaction->reflog_updates.nr) {
 		transaction->state = TRANSACTION_CLOSED;
 		return 0;
 	}
@@ -3798,6 +3881,13 @@ int transaction_commit(struct transaction *transaction,
 	}
 	for (i = 0; i < delnum; i++)
 		unlink_or_warn(git_path("logs/%s", delnames[i]));
+
+	/* Commit all reflog updates*/
+	for_each_string_list_item(item, &transaction->reflog_updates) {
+		struct lock_file *lock = item->util;
+		commit_lock_file_to(lock, git_path("logs/%s", item->string));
+	}
+
 	clear_loose_ref_cache(&ref_cache);
 
 cleanup:
-- 
2.2.0

Re: [PATCH 4/8] refs.c: add transaction function to append to the reflog

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Unlike transaction_update_ref, this writes out the proposed contents of the
> reflog to a temporary file at transaction_reflog_update time instead of
> waiting for the transaction waiting to be committed. This avoids an
> explosion of memory usage when writing lots of reflog updates within a
> single transaction.

Copying an existing reflog with thousands of entries over so that I
can append a single new entry, just so that I can rollback by not
renaming?

After ensuring that you are the only process that holds a write fd
to the reflog file (e.g. by taking a lock on the ref itself),
shouldn't you be able to ftell(), write() and then truncate() to
roll back sanely before close()?  After all you are not protecting
from power loss and other kinds of glitches that would leave *.lock
file behind, so...

[PATCH 5/8] refs.c: add transaction function to delete the reflog

[Stefan Beller]

This continues the work of the previous patch as reflogs not
only grow, but also need a cut sometimes. This patch introduces
transaction_delete_reflog as part of the transaction API to
delete the reflog.

This function serves two purposes. It can be used to actually
delete the reflog as the name indicates. The other purpose is
truncation of the reflog and rewriting it.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index d767418..57f4941 100644
--- a/refs.c
+++ b/refs.c
@@ -3649,6 +3649,56 @@ int transaction_update_ref(struct transaction *transaction,
 }
 
 /*
+ * Delete the reflog for the given refname.
+ *
+ */
+static int transaction_delete_reflog(struct transaction *transaction,
+				       const char *refname,
+				       struct strbuf *err)
+{
+	struct lock_file *lock;
+	struct string_list_item *item;
+
+	if (transaction->state != TRANSACTION_OPEN)
+		die("BUG: delete_reflog called for transaction that is not open");
+
+	item = string_list_insert(&transaction->reflog_updates, refname);
+
+	if (!item->util) {
+		char *path = git_path("logs/locks/%s", refname);
+		lock = xcalloc(1, sizeof(struct lock_file));
+		item->util = lock;
+		if (safe_create_leading_directories(path)) {
+			strbuf_addf(err, "could not create leading directories of '%s': %s",
+				    path, strerror(errno));
+			goto failure;
+		}
+
+		if (hold_lock_file_for_update(lock, path, 0) < 0) {
+			unable_to_lock_message(path, errno, err);
+			goto failure;
+		}
+		/* The empty file indicates transaction_commit to
+		 * delete the reflog */
+		return 0;
+	}
+
+	/* The transaction already writes to this reflog.  Clear it. */
+	lock = item->util;
+	if (lseek(lock->fd, 0, SEEK_SET) < 0 ||
+	    ftruncate(lock->fd, 0)) {
+		strbuf_addf(err, "cannot truncate reflog '%s': %s",
+			    refname, strerror(errno));
+		goto failure;
+	}
+	return 0;
+
+failure:
+	transaction->state = TRANSACTION_CLOSED;
+	return -1;
+}
+
+/*
  * Append a reflog entry for refname.
  */
 static int transaction_update_reflog(struct transaction *transaction,
@@ -3885,7 +3935,17 @@ int transaction_commit(struct transaction *transaction,
 	/* Commit all reflog updates*/
 	for_each_string_list_item(item, &transaction->reflog_updates) {
 		struct lock_file *lock = item->util;
-		commit_lock_file_to(lock, git_path("logs/%s", item->string));
+
+		/* If the lock file is empty we want to delete the reflog*/
+		off_t filepos = lseek(lock->fd, 0, SEEK_END);
+		if (filepos < 0) {
+			ret = TRANSACTION_GENERIC_ERROR;
+			goto cleanup;
+		}
+		if (filepos)
+			commit_lock_file_to(lock, git_path("logs/%s", item->string));
+		else
+			remove_path(git_path("logs/%s", item->string));
 	}
 
 	clear_loose_ref_cache(&ref_cache);
-- 
2.2.0

[PATCH 6/8] refs.c: use a reflog transaction when writing during expire

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

Use a transaction for all updates during expire_reflog.

[sb: This was once a patch series on its own. I cherry-picked these
patches on top of Michaels series to cleanup the refs api. So any
anomalies and bugs may be introduced by me.]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    Maybe we can leave out the first patch of the series
    as this one deletes the changes made in the first patch of the series.
    
    Originally authored by Ronnie for the reflogs.c file,
    cherrypicked over to the refs.c file by Stefan

 refs.c | 85 +++++++++++++++++++++++++++---------------------------------------
 1 file changed, 35 insertions(+), 50 deletions(-)

diff --git a/refs.c b/refs.c
index 57f4941..295ea09 100644
--- a/refs.c
+++ b/refs.c
@@ -4100,8 +4100,10 @@ struct expire_reflog_cb {
 	unsigned int flags;
 	reflog_expiry_select_fn *select_fn;
 	void *policy_cb;
-	FILE *newlog;
+	struct transaction *t;
+	const char *refname;
 	unsigned char last_kept_sha1[20];
+	struct strbuf *err;
 };
 
 static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
@@ -4116,15 +4118,18 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 
 	if ((*cb->select_fn)(osha1, nsha1, email, timestamp, tz,
 			     message, policy_cb)) {
-		if (!cb->newlog)
+		if (!cb->t)
 			printf("would prune %s", message);
 		else if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
 			printf("prune %s", message);
 	} else {
-		if (cb->newlog) {
-			fprintf(cb->newlog, "%s %s %s %lu %+05d\t%s",
-				sha1_to_hex(osha1), sha1_to_hex(nsha1),
-				email, timestamp, tz, message);
+		if (cb->t) {
+			if (transaction_update_reflog(cb->t, cb->refname,
+						      nsha1, osha1,
+						      email, timestamp,
+						      tz, message,
+						      cb->err))
+				return -1;
 			hashcpy(cb->last_kept_sha1, nsha1);
 		}
 		if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
@@ -4133,8 +4138,6 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 	return 0;
 }
 
-static struct lock_file reflog_lock;
-
 extern int reflog_expire(const char *refname, const unsigned char *sha1,
 			 unsigned int flags,
 			 reflog_expiry_prepare_fn prepare_fn,
@@ -4143,66 +4146,48 @@ extern int reflog_expire(const char *refname, const unsigned char *sha1,
 			 void *policy_cb_data)
 {
 	struct expire_reflog_cb cb;
-	struct ref_lock *lock;
-	char *log_file;
 	int status = 0;
+	struct strbuf err = STRBUF_INIT;
 
 	memset(&cb, 0, sizeof(cb));
 	cb.flags = flags;
 	cb.policy_cb = policy_cb_data;
 	cb.select_fn = select_fn;
+	cb.err = &err;
+	cb.refname = refname;
 
 	/*
-	 * we take the lock for the ref itself to prevent it from
-	 * getting updated.
+	 * todo: we need to take the lock for the ref itself to
+	 * prevent it from getting updated.
 	 */
-	lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, NULL);
-	if (!lock)
-		return error("cannot lock ref '%s'", refname);
-	if (!reflog_exists(refname)) {
-		unlock_ref(lock);
-		return 0;
+	cb.t = transaction_begin(&err);
+	if (!cb.t) {
+		status |= error("%s", err.buf);
+		goto cleanup;
 	}
-
-	log_file = git_pathdup("logs/%s", refname);
-	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
-		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
-			goto failure;
-		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
-		if (!cb.newlog)
-			goto failure;
+	if (transaction_delete_reflog(cb.t, cb.refname, &err)) {
+		status |= error("%s", err.buf);
+		goto cleanup;
 	}
 
 	(*prepare_fn)(refname, sha1, cb.policy_cb);
 	for_each_reflog_ent(refname, expire_reflog_ent, &cb);
 	(*cleanup_fn)(cb.policy_cb);
 
+
 	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
-		if (close_lock_file(&reflog_lock)) {
-			status |= error("Couldn't write %s: %s", log_file,
-					strerror(errno));
-		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
-			(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.buf);
-			rollback_lock_file(&reflog_lock);
-		} else if (commit_lock_file(&reflog_lock)) {
-			status |= error("cannot rename %s.lock to %s",
-					log_file, log_file);
-		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {
-			status |= error("Couldn't set %s", lock->ref_name);
+		if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
+		    transaction_update_ref(cb.t, cb.refname,
+					   cb.last_kept_sha1, sha1,
+					   0, 1, NULL, &err)) {
+			status |= error("%s", err.buf);
+			goto cleanup;
 		}
+		if (transaction_commit(cb.t, &err))
+			status |= error("%s", err.buf);
 	}
-	free(log_file);
-	unlock_ref(lock);
+cleanup:
+	transaction_free(cb.t);
+	strbuf_release(&err);
 	return status;
-
- failure:
-	rollback_lock_file(&reflog_lock);
-	free(log_file);
-	unlock_ref(lock);
-	return -1;
 }
-- 
2.2.0

[PATCH 7/8] refs.c: rename log_ref_setup to create_reflog

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

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: In checkout.c, where
it is always used to create a reflog and in refs.c log_ref_write,
where it is used to create a reflog sometimes, and sometimes just
to fill in the filename.

Rename log_ref_setup to create_reflog and change it to only take the
refname as an 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>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    This is also just carried along from sending it last time.

 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 5410dac..8550b6d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -587,19 +587,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 295ea09..2effd66 100644
--- a/refs.c
+++ b/refs.c
@@ -2943,16 +2943,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);
@@ -3021,16 +3021,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 40607d6..7599f81 100644
--- a/refs.h
+++ b/refs.h
@@ -183,11 +183,6 @@ extern int peel_ref(const char *refname, unsigned char *sha1);
 #define REF_NODEREF	0x01
 #define REF_DELETING	0x02
 
-/*
- * 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 int flags,
 		       unsigned long at_time, int cnt,
@@ -197,6 +192,9 @@ extern int read_ref_at(const char *refname, unsigned int flags,
 /** 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.2.0

[PATCH 8/8] refs.c: allow deleting refs with a broken sha1

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

Add back support to make it possible to delete refs that have a broken
sha1.

Add new internal flags REF_ALLOW_BROKEN and RESOLVE_REF_ALLOW_BAD_SHA1
to pass intent from branch.c that we are willing to allow
resolve_ref_unsafe and lock_ref_sha1_basic to allow broken refs.
Since these refs can not actually be resolved to a sha1, they instead resolve
to null_sha1 when these flags are used.

For example, the ref:

   echo "Broken ref" > .git/refs/heads/foo-broken-1

can now be deleted using git branch -d foo-broken-1

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    Changes to previously sent version:
     * do not introduce yet another flag, but stick with REF_DELETING
       which was meant for this purpose.

 builtin/branch.c  | 5 +++--
 cache.h           | 7 +++++++
 refs.c            | 5 +++++
 t/t3200-branch.sh | 8 ++++++++
 4 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 3b79c50..5fe1cac 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -238,7 +238,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		target = resolve_ref_unsafe(name,
 					    RESOLVE_REF_READING
 					    | RESOLVE_REF_NO_RECURSE
-					    | RESOLVE_REF_ALLOW_BAD_NAME,
+					    | RESOLVE_REF_ALLOW_BAD_NAME
+					    | RESOLVE_REF_ALLOW_BAD_SHA1,
 					    sha1, &flags);
 		if (!target) {
 			error(remote_branch
@@ -255,7 +256,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 			continue;
 		}
 
-		if (delete_ref(name, sha1, REF_NODEREF)) {
+		if (delete_ref(name, sha1, REF_NODEREF|REF_DELETING)) {
 			error(remote_branch
 			      ? _("Error deleting remote branch '%s'")
 			      : _("Error deleting branch '%s'"),
diff --git a/cache.h b/cache.h
index 99ed096..61e61af 100644
--- a/cache.h
+++ b/cache.h
@@ -1000,10 +1000,17 @@ extern int read_ref(const char *refname, unsigned char *sha1);
  * resolved. The function returns NULL for such ref names.
  * Caps and underscores refers to the special refs, such as HEAD,
  * FETCH_HEAD and friends, that all live outside of the refs/ directory.
+ *
+ * RESOLVE_REF_ALLOW_BAD_SHA1 when this flag is set and the ref contains
+ * an invalid sha1, resolve_ref_unsafe will clear the sha1 argument,
+ * set the REF_ISBROKEN flag and return the refname.
+ * This allows using resolve_ref_unsafe to check for existence of such
+ * broken refs.
  */
 #define RESOLVE_REF_READING 0x01
 #define RESOLVE_REF_NO_RECURSE 0x02
 #define RESOLVE_REF_ALLOW_BAD_NAME 0x04
+#define RESOLVE_REF_ALLOW_BAD_SHA1 0x08
 extern const char *resolve_ref_unsafe(const char *ref, int resolve_flags, unsigned char *sha1, int *flags);
 extern char *resolve_refdup(const char *ref, int resolve_flags, unsigned char *sha1, int *flags);
 
diff --git a/refs.c b/refs.c
index 2effd66..94b766f 100644
--- a/refs.c
+++ b/refs.c
@@ -1587,6 +1587,10 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags, unsigned
 			    (buffer[40] != '\0' && !isspace(buffer[40]))) {
 				if (flags)
 					*flags |= REF_ISBROKEN;
+				if (resolve_flags & RESOLVE_REF_ALLOW_BAD_SHA1) {
+					hashclr(sha1);
+					return refname;
+				}
 				errno = EINVAL;
 				return NULL;
 			}
@@ -2265,6 +2269,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		resolve_flags |= RESOLVE_REF_READING;
 	if (flags & REF_DELETING) {
 		resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
+		resolve_flags |= RESOLVE_REF_ALLOW_BAD_SHA1;
 		if (flags & REF_NODEREF)
 			resolve_flags |= RESOLVE_REF_NO_RECURSE;
 	}
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 432921b..1ea0d2c 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -163,6 +163,14 @@ test_expect_success 'git branch --list -d t should fail' '
 	test_path_is_missing .git/refs/heads/t
 '
 
+test_expect_success 'git branch -d can delete ref with broken sha1' '
+	echo "pointing nowhere" >.git/refs/heads/brokensha1 &&
+	test_when_finished "rm -f .git/refs/heads/brokensha1" &&
+	git branch -d brokensha1 &&
+	git branch >output &&
+	! grep brokensha1 output
+'
+
 test_expect_success 'git branch --column' '
 	COLUMNS=81 git branch --column=column >actual &&
 	cat >expected <<\EOF &&
-- 
2.2.0

Re: [PATCH 0/8] Making reflog modifications part of the transactions API

[Stefan Beller]

So I reviewed and examined this series and run into some problems.

How do we view an empty reflog, i.e. an empty file at logs/refs/heads/master?
I was told this is not a valid state for a reflog. However even the
test suite sometimes
produces an empty reflog files.
Below there is a patch, which highlights the empty reflog when running
t1301.sh verbosely.


---
 t/t1301-shared-repo.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index de42d21..6a35fc0 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -113,7 +113,9 @@ done

 test_expect_success POSIXPERM 'git reflog expire honors
core.sharedRepository' '
  git config core.sharedRepository group &&
- git reflog expire --all &&
+ git reflog master &&
+ git reflog expire --all --verbose &&
+ git reflog master &&
  actual="$(ls -l .git/logs/refs/heads/master)" &&
  case "$actual" in
  -rw-rw-*)
-- 
2.2.0

Re: [PATCH 0/8] Making reflog modifications part of the transactions API

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> How do we view an empty reflog, i.e. an empty file at logs/refs/heads/master?

That's a good question.  I'm a little concerned about what 'git reflog
expire --updateref' would do in this case.

It looks like the longstanding behavior is for 'git reflog expire' to
expire any entry that is old enough, even if that entry describes the
current value of the ref and the reflog becomes empty.  The empty
reflog file is kind of like having no reflog at all, except it means
git should log updates to that ref even if core.logallrefupdates is
false.

That suggests that two separate operations delete_reflog (for 'git
reflog delete') and truncate_reflog (for 'git reflog expire') would be
needed.

Thanks for catching it.
Jonathan

Re: [PATCH 0/8] Making reflog modifications part of the transactions API

[Michael Haggerty]

On 12/06/2014 03:46 AM, Stefan Beller wrote:
> This goes on top of Michaels series. The idea of this series is make the
> reflogs being part of the transaction API, so it will be part of the contract
> of transaction_commit to either commit all the changes or none at all.
> 
> Currently when using the transaction API to change refs, also reflogs are changed.
> But the changes to the reflogs just happen as a side effect and not as part of
> the atomic part of changes we want to commit altogether.

Would you please explain why this patch series is still needed if my
"reflog_expire()" series is accepted?

reflog_expire() already has its own little transaction. Reflog
expiration never needs to be combined with other reference changes, so
there is no need for reflog expiration to be done via a ref_transaction.

ref_transaction_commit() already updates the reflog if necessary when a
reference is updated. That update is part of the containing
ref_transaction, because it is locked by the lock on the reference
itself at the beginning of the transaction and unlocked at the end of
the transaction [1]. It can't fail in normal circumstances because the
preconditions for the transaction have already been checked.

As far as I can tell, the only thing left to do is provide a substitute
for log_ref_setup() a.k.a. create_reflog() that can be triggered within
a transaction. In my opinion the easiest way to do that is to give
ref_transaction_update()'s flag parameter an additional option,
REF_CREATE_REFLOG, which forces the reference's reflog to be created if
it does not already exist.

What am I missing?

Michael

[1] The reflog updates are not atomic in the face of disk errors and
power outages and stuff, but neither are reference updates. In other
words, I think reflog updates in ref_transaction_commit() are done as
safely as reference updates, which is surely good enough for this
reference backend. Other reference backends can do a better job with
both while staying within the existing API.

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 0/8] Making reflog modifications part of the transactions API

[Stefan Beller]

On Fri, Dec 12, 2014 at 8:17 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 12/06/2014 03:46 AM, Stefan Beller wrote:
>> This goes on top of Michaels series. The idea of this series is make the
>> reflogs being part of the transaction API, so it will be part of the contract
>> of transaction_commit to either commit all the changes or none at all.
>>
>> Currently when using the transaction API to change refs, also reflogs are changed.
>> But the changes to the reflogs just happen as a side effect and not as part of
>> the atomic part of changes we want to commit altogether.
>
> Would you please explain why this patch series is still needed if my
> "reflog_expire()" series is accepted?

If we can live with the fact that reflog and refs have different APIs
then, we can probably
drop that series. That series may also have performance issues, Junio
pointed out.
Most likely I'll look at the remaining 4 patch series from Ronnie, if
I can put them on
top of your series now.

>
> reflog_expire() already has its own little transaction. Reflog
> expiration never needs to be combined with other reference changes, so
> there is no need for reflog expiration to be done via a ref_transaction.
>
> ref_transaction_commit() already updates the reflog if necessary when a
> reference is updated. That update is part of the containing
> ref_transaction, because it is locked by the lock on the reference
> itself at the beginning of the transaction and unlocked at the end of
> the transaction [1]. It can't fail in normal circumstances because the
> preconditions for the transaction have already been checked.


>
> As far as I can tell, the only thing left to do is provide a substitute
> for log_ref_setup() a.k.a. create_reflog() that can be triggered within
> a transaction. In my opinion the easiest way to do that is to give
> ref_transaction_update()'s flag parameter an additional option,
> REF_CREATE_REFLOG, which forces the reference's reflog to be created if
> it does not already exist.

I already ported the patch which replaces log_ref_setup() by create_reflog()
on top of your series-v1, but I refrained to send it out as Junio
seems to dislike
invasive patches[1] or possible merge conflicts so I was waiting for
discussion on
your patch series calm down and be merged to next at least.

Apart from that rename, I also looked into flagging REF_CREATE_REFLOG
additionally.
Though I run into problems there (test suite doesn't pass and I did
not find the mistake for 5 hours :/)

>
> What am I missing?

I think the original plan of Ronnie was to have one and only one transaction
API, which would be used for anything eventually (refs, reflogs,
config, what have you)
If we go for a relaxed version of that, we'd be fine.

Thanks,
Stefan


[1] http://www.spinics.net/lists/git/msg243451.html
     The patch in question is not an invasive patch, but still touching places
     which may be touched currently by your series. So I just wanted
to wait a bit.

Re: [PATCH 0/8] Making reflog modifications part of the transactions API

[ronnie sahlberg]

On Fri, Dec 12, 2014 at 11:17 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 12/06/2014 03:46 AM, Stefan Beller wrote:
>> This goes on top of Michaels series. The idea of this series is make the
>> reflogs being part of the transaction API, so it will be part of the contract
>> of transaction_commit to either commit all the changes or none at all.
>>
>> Currently when using the transaction API to change refs, also reflogs are changed.
>> But the changes to the reflogs just happen as a side effect and not as part of
>> the atomic part of changes we want to commit altogether.
>
> Would you please explain why this patch series is still needed if my
> "reflog_expire()" series is accepted?
>
> reflog_expire() already has its own little transaction. Reflog
> expiration never needs to be combined with other reference changes, so
> there is no need for reflog expiration to be done via a ref_transaction.
>
> ref_transaction_commit() already updates the reflog if necessary when a
> reference is updated. That update is part of the containing
> ref_transaction, because it is locked by the lock on the reference
> itself at the beginning of the transaction and unlocked at the end of
> the transaction [1]. It can't fail in normal circumstances because the
> preconditions for the transaction have already been checked.
>
> As far as I can tell, the only thing left to do is provide a substitute
> for log_ref_setup() a.k.a. create_reflog() that can be triggered within
> a transaction. In my opinion the easiest way to do that is to give
> ref_transaction_update()'s flag parameter an additional option,
> REF_CREATE_REFLOG, which forces the reference's reflog to be created if
> it does not already exist.
>
> What am I missing?
>

My original idea was to clean up a bit of the reflog handling API and
have one single transaction API for all  ref and reflog operations.

Think future use cases where you have a database backend for both refs
and reflogs. It would be very nice to have a single atomic transaction
that would either commit or fail atomically any update to refs and/or
reflogs.
Otherwise you would have all consumers of the API have to invent their
own transaction and rewind support : 'oh the ref transaction failed
and I have already done the reflog commit,   have to manually uncommit
...".
And this quickly becomes quite burdensome for consumers.

I think a transaction API should remove this burden from consumers and
make it as easy as possible to use the API.

Conditional of if it is desireable to have transactions for reflogs at all.



About the cleanup part. The current API, and I think also the current
direction of both my old patches (which I think did not go far enough
in the transactifications) or this current patchseries is that they
all
have a very confusing and inconsistent API for reflog updates.
With this I mean,   sometimes reflog updates happen within a
transaction as a side effect of a ref_transaction_update().
Other times reflog updates happens ooutside of transactions by calling
a special reflog API.

I.e.  reflogs sometimes update as part of a transaction and sometimes not.
A follow up question then on this API is what should happen if you
have a transaction open, but not committed, and while the transaction
is open you call the non-transactional reflog API for a reflog for the
same ref that is already beeing/or going to be/ updated as the
ref-update-side-effect ?


I think an api where "sometimes you operate on foo from within a
transaction and sometimes you do not, and if you do the latter when a
transaction is open, it is unclear what should happen" is not great.
IMHO, refs and reflog updates are related and I think:

* a transaction should be the ONLY way to mutate either a ref or a
reflog or both.
* if you update both a ref and a reflog then that should happen as
part of one single transaction.
* (later) it would probably make the API better if the code was
refactored so write_ref_sha1() will NOT call log_ref_write() anymore
and instead make the reflog update that happens explicit perhaps by
calling something like a ref_transaction_update_reflog() as part of
the ref_transaction_update() call.






> Michael
>
> [1] The reflog updates are not atomic in the face of disk errors and
> power outages and stuff, but neither are reference updates. In other
> words, I think reflog updates in ref_transaction_commit() are done as
> safely as reference updates, which is surely good enough for this
> reference backend. Other reference backends can do a better job with
> both while staying within the existing API.
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
>

Re: [PATCH 0/8] Making reflog modifications part of the transactions API

[Michael Haggerty]

On 12/12/2014 10:16 PM, ronnie sahlberg wrote:
> On Fri, Dec 12, 2014 at 11:17 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> [...]
>> What am I missing?
> 
> My original idea was to clean up a bit of the reflog handling API and
> have one single transaction API for all  ref and reflog operations.
> 
> Think future use cases where you have a database backend for both refs
> and reflogs. It would be very nice to have a single atomic transaction
> that would either commit or fail atomically any update to refs and/or
> reflogs.
> Otherwise you would have all consumers of the API have to invent their
> own transaction and rewind support : 'oh the ref transaction failed
> and I have already done the reflog commit,   have to manually uncommit
> ...".
> And this quickly becomes quite burdensome for consumers.
> 
> I think a transaction API should remove this burden from consumers and
> make it as easy as possible to use the API.
> 
> Conditional of if it is desireable to have transactions for reflogs at all.

Nobody is against ACID. But the API to trigger an ACID update doesn't
always have to look like

    ref_transaction_begin()
    ref_transaction_update_XXX()
    /* ... */
    ref_transaction_commit()

The reflog_expire() function that I wrote does everything within an
internal transaction that the caller doesn't have to know about.
Similarly, the reflog update that happens when a reference is updated
also occurs within a transaction, even without the caller having to ask
for the reflog update explicitly.

> About the cleanup part. The current API, and I think also the current
> direction of both my old patches (which I think did not go far enough
> in the transactifications) or this current patchseries is that they
> all
> have a very confusing and inconsistent API for reflog updates.
> With this I mean,   sometimes reflog updates happen within a
> transaction as a side effect of a ref_transaction_update().
> Other times reflog updates happens ooutside of transactions by calling
> a special reflog API.
> 
> I.e.  reflogs sometimes update as part of a transaction and sometimes not.
> A follow up question then on this API is what should happen if you
> have a transaction open, but not committed, and while the transaction
> is open you call the non-transactional reflog API for a reflog for the
> same ref that is already beeing/or going to be/ updated as the
> ref-update-side-effect ?

The same thing happens as when two independent processes try to update
the same reference simultaneously: one fails. But there is currently no
need for any command that would do such a thing.

> I think an api where "sometimes you operate on foo from within a
> transaction and sometimes you do not, and if you do the latter when a
> transaction is open, it is unclear what should happen" is not great.
> IMHO, refs and reflog updates are related and I think:
> 
> * a transaction should be the ONLY way to mutate either a ref or a
> reflog or both.
> * if you update both a ref and a reflog then that should happen as
> part of one single transaction.
> * (later) it would probably make the API better if the code was
> refactored so write_ref_sha1() will NOT call log_ref_write() anymore
> and instead make the reflog update that happens explicit perhaps by
> calling something like a ref_transaction_update_reflog() as part of
> the ref_transaction_update() call.

I disagree. The reflog should be updated *whenever* a reference is
updated (for references that have reflogs enabled). So why should
callers have to remember to trigger the reflog update as an extra step?
It's better that the reflog update is an intrinsic part of updating the
reference; that way nobody can forget it.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu