[PATCHv3 00/13] the refs-transactions-reflog series

[PATCHv3 00/13] the refs-transactions-reflog series

[Stefan Beller]

This is the whole refs-transactions-reflog series[1],
which was in discussion for a bit already. It applies to origin/master.

The idea is to have the reflog being part of the transactions, which
the refs are already using, so the we're moving towards a database
like API in the long run. This makes git easier to maintain as well
as opening the possibility to replace the backend with a real database.

If you've followed the topic a bit, start reading at patch
"[PATCH 06/13] refs.c: add a transaction function to append a reflog"
as the first 5 patches have been discussed a lot separately and can be found in
origin/pu already[2].
The first two patches are deduplicating code.
The third patch is ripping some code out of log_ref_write and introduces
log_ref_write_fd, which does the actual writing.
The patches 4+5 are renaming variables for clarity.

The patch 6 and 7 are the entree in this series. Patch 6 adds two functions to
the refs API: transaction_truncate_reflog and transaction_update_reflog. Both
functions do not affect the files directy, but rather become effective once
transaction_commit function is called. The transaction_truncate_reflog will
wipe the reflog, which can be used for rebuilding the reflog, whereas the
transaction_update_reflog function will just append one entry to the reflog.
The patch 7 will make use of the functions introduced in patch 6. The command
git reflog expire will then use the reflog transactions.

Patch 8 is renaming log_ref_setup to create_reflog and should not

Patches 9-12 are removing functions from the public refs API, which are unused then.

Patch 13 is adding new functionality again, you can finally delete broken refs without
having to know the details on git.

[1] http://comments.gmane.org/gmane.comp.version-control.git/259712 or
    http://www.spinics.net/lists/git/msg242502.html 

[2] patch 1 + 2:
        origin/sb/ref-transaction-unify-to-update
    patch 3:
        origin/sb/log-ref-write-fd
    patch 4 + 5:
        origin/sb/ref-transaction-reflog
    patch 1-5 is unchanged in this series.


Ronnie Sahlberg (9):
  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: add a function to append a reflog entry to a fd
  refs.c: rename the transaction functions
  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

Stefan Beller (4):
  refs.c: rename transaction.updates to transaction.ref_updates
  refs.c: add a transaction function to truncate or append a reflog
    entry
  refs.c: don't expose the internal struct ref_lock in the header file
  refs.c: use a bit for ref_update have_old

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

-- 
2.2.0

[PATCH 01/13] refs.c: make ref_transaction_create a wrapper for ref_transaction_update

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

The ref_transaction_update function can already be used to create refs by
passing null_sha1 as the old_sha1 parameter. Simplify by replacing
transaction_create with a thin wrapper.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---

Notes:
    origin/sb/ref-transaction-unify-to-update
    as well as
    origin/sb/ref-transaction-reflog
    
    no changes since sending them last time

 refs.c | 27 ++-------------------------
 1 file changed, 2 insertions(+), 25 deletions(-)

diff --git a/refs.c b/refs.c
index 5ff457e..005eb18 100644
--- a/refs.c
+++ b/refs.c
@@ -3623,31 +3623,8 @@ int ref_transaction_create(struct ref_transaction *transaction,
 			   int flags, const char *msg,
 			   struct strbuf *err)
 {
-	struct ref_update *update;
-
-	assert(err);
-
-	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");
-
-	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-		strbuf_addf(err, "refusing to create ref with bad name %s",
-			    refname);
-		return -1;
-	}
-
-	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,
-- 
2.2.0

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

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---

Notes:
    origin/sb/ref-transaction-unify-to-update
    as well as
    origin/sb/ref-transaction-reflog
    
    no changes since sending last time.

 refs.c | 22 ++--------------------
 refs.h |  2 +-
 2 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/refs.c b/refs.c
index 005eb18..05cb299 100644
--- a/refs.c
+++ b/refs.c
@@ -3633,26 +3633,8 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 			   int flags, int have_old, const char *msg,
 			   struct strbuf *err)
 {
-	struct ref_update *update;
-
-	assert(err);
-
-	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;
+	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 2bc3556..7d675b7 100644
--- a/refs.h
+++ b/refs.h
@@ -283,7 +283,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, then old_sha1 holds the value
  * that the reference should have had before the update, or zeros if
  * it must not have existed beforehand.
-- 
2.2.0

[PATCH 03/13] refs.c: add a function to append a reflog entry to a fd

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

Break out the code to create the string and writing it to the file
descriptor from log_ref_write and add it 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>
Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---

Notes:
    being part of origin/sb/log-ref-write-fd
    
    no changes since sending last time.

 refs.c | 48 ++++++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/refs.c b/refs.c
index 05cb299..150c980 100644
--- a/refs.c
+++ b/refs.c
@@ -2990,15 +2990,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();
@@ -3010,19 +3032,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.2.0

[PATCH 04/13] refs.c: rename the transaction functions

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

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.

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

Notes:
    remotes/origin/sb/ref-transaction-reflog
    
    no changes since last review
    
    This commit can be reproduced via
    find . -name "*.[ch]" -print | xargs sed -i ' \
            s/REF_TRANSACTION_OPEN/TRANSACTION_OPEN/; \
            s/REF_TRANSACTION_CLOSED/TRANSACTION_CLOSED/; \
            s/ref_transaction_begin/transaction_begin/; \
            s/ref_transaction_commit/transaction_commit/; \
            s/ref_transaction_create/transaction_create_ref/; \
            s/ref_transaction_delete/transaction_delete_ref/; \
            s/ref_transaction_free/transaction_free/; \
            s/ref_transaction_update/transaction_update_ref/; \
            s/ref_transaction/transaction/'
    modulo white space changes for alignment.

 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                 | 36 +++++++++++------------
 sequencer.c            | 12 ++++----
 walker.c               | 10 +++----
 12 files changed, 126 insertions(+), 126 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 150c980..f0f0d23 100644
--- a/refs.c
+++ b/refs.c
@@ -26,7 +26,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
@@ -2533,23 +2533,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);
 }
@@ -2711,20 +2711,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;
 }
@@ -3543,9 +3543,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
 };
 
 /*
@@ -3553,21 +3553,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;
 
@@ -3582,7 +3582,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);
@@ -3594,7 +3594,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,
@@ -3605,7 +3605,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)
@@ -3629,23 +3629,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);
 }
 
@@ -3653,17 +3653,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);
@@ -3678,7 +3678,7 @@ int update_ref(const char *action, const char *refname,
 		return 1;
 	}
 	strbuf_release(&err);
-	ref_transaction_free(t);
+	transaction_free(t);
 	return 0;
 }
 
@@ -3706,8 +3706,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;
@@ -3716,11 +3716,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;
 	}
 
@@ -3799,7 +3799,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 7d675b7..556adfd 100644
--- a/refs.h
+++ b/refs.h
@@ -11,22 +11,22 @@ struct ref_lock {
 };
 
 /*
- * 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.
@@ -42,7 +42,7 @@ struct ref_lock {
  * 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():
@@ -181,8 +181,8 @@ extern int is_branch(const char *refname);
 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_ref(),
+ * transaction_create_ref(), etc.
  * REF_NODEREF: act on the ref directly, instead of dereferencing
  *              symbolic references.
  * REF_DELETING: tolerate broken refs
@@ -269,13 +269,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().
@@ -291,7 +291,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,
@@ -307,7 +307,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,
@@ -321,7 +321,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,
@@ -337,13 +337,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

[PATCH 05/13] 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:
    remotes/origin/sb/ref-transaction-reflog
    
    no changes since last review

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

diff --git a/refs.c b/refs.c
index f0f0d23..58de60c 100644
--- a/refs.c
+++ b/refs.c
@@ -3554,7 +3554,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;
@@ -3575,10 +3575,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);
 }
 
@@ -3589,8 +3589,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;
 }
 
@@ -3712,7 +3712,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 06/13] refs.c: add a transaction function to truncate or append a reflog entry

[Stefan Beller]

This patch introduces two transaction functions for dealing with reflog
changes. The first function transaction_truncate_reflog can be used,
when a rebuilding of the reflog is desired, e.g. on reflog expire.
The transaction_update_reflog function can be used to amend a line to the
reflog.

We cannot handle reflogs the same way we handle refs because
of naming conflicts in the file system.
If renaming a ref from foo/foo to foo or the other way round,
we need to be careful as we need the basic foo/ from being a
directory to be a file or vice versa. When handling the refs
this can be solved easily by first recording all we want into
the packed refs file and then deleting all the loose refs. By
doing it that way, we always have a valid state on disk.

We don't have an equivalent of a packed refs file when dealing
with reflog updates, but the API for updating the refs turned
out to be conveniant, so let's introduce an intermediate file
outside the $GIT_DIR/logs/refs/heads/ directory helping us
avoiding this naming conflict for the reflogs. The intermediate
lock file, in which we build up the new reflog will live under
$GIT_DIR/logs/lock/refs/heads/ so the files
        $GIT_DIR/logs/lock/refs/heads/foo.lock and
        $GIT_DIR/logs/lock/refs/heads/foo/foo.lock
do not collide.

When using transaction_update_reflog, only write to the reflog iff
msg is non-NULL.

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.

This allows us to write code such as:

  t = transaction_begin()
  transaction_truncate_reflog(t, "foo");
  loop-over-something...
     if (want_reflog_entry(...))
         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: Stefan Beller <sbeller@google.com>
---

Notes:
    This is a complete rewrite of this patch, lots of design ideas
    have been born in fruitful discussions with Jonathan.
    
    Personally I am not yet happy about the file copying in
    transaction_update_reflog.

 refs.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 refs.h |  20 +++++++++++
 2 files changed, 146 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 58de60c..c411af9 100644
--- a/refs.c
+++ b/refs.c
@@ -3557,6 +3557,12 @@ 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;
 };
 
@@ -3564,7 +3570,10 @@ struct transaction *transaction_begin(struct strbuf *err)
 {
 	assert(err);
 
-	return xcalloc(1, sizeof(struct transaction));
+	struct transaction *ret = xcalloc(1, sizeof(struct transaction));
+	ret->reflog_updates.strdup_strings = 1;
+
+	return ret;
 }
 
 void transaction_free(struct transaction *transaction)
@@ -3629,6 +3638,113 @@ int transaction_update_ref(struct transaction *transaction,
 	return 0;
 }
 
+int transaction_truncate_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: update_reflog called for transaction that is not open");
+
+	item = string_list_insert(&transaction->reflog_updates, refname);
+	if (item->util) {
+		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;
+		}
+
+	} else {
+		char *path = git_path("logs/locks/%s", refname);
+		item->util = xcalloc(1, sizeof(struct lock_file));
+		lock = item->util;
+		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 file is empty, no need to truncate. */
+	}
+	return 0;
+failure:
+	transaction->state = TRANSACTION_CLOSED;
+	return -1;
+}
+
+
+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) {
+		char buf[1024];
+		int infd, nread;
+		char *path = git_path("logs/locks/%s", refname);
+		item->util = xcalloc(1, sizeof(struct lock_file));
+		lock = item->util;
+		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 */
+		infd = open(git_path("logs/%s", refname), O_RDONLY);
+		while ((nread = read(infd, buf, 1024)) > 0)
+			write_in_full(lock->fd, buf, nread);
+		if (nread < 0) {
+			strbuf_addf(err, "could not read reflog '%s': %s", refname, strerror(errno));
+			goto failure;
+		}
+		close(infd);
+	}
+	lock = item->util;
+
+	if (email)
+		strbuf_addf(&buf, "%s %lu %+05d", email, timestamp, tz);
+
+	if (msg &&
+	    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,
@@ -3713,13 +3829,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;
 	}
@@ -3796,6 +3913,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:
diff --git a/refs.h b/refs.h
index 556adfd..1afc72c 100644
--- a/refs.h
+++ b/refs.h
@@ -328,6 +328,26 @@ int transaction_delete_ref(struct transaction *transaction,
 			   struct strbuf *err);
 
 /*
+ * Truncate the reflog for the given refname.
+ */
+int transaction_truncate_reflog(struct transaction *transaction,
+				const char *refname,
+				struct strbuf *err);
+
+/*
+ * Append a reflog entry for refname. If msg is NULL no update will be
+ * written to the log.
+ */
+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);
+
+/*
  * Commit all of the changes that have been queued in transaction, as
  * atomically as possible.
  *
-- 
2.2.0

[PATCH 07/13] reflog.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.

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

Notes:
    Jonathan writes:
    > This doesn't match the signature of each_reflog_ent_fn.  Would putting
    > err in the cb_data struct work?
    
    That's what I do in this update.

 builtin/reflog.c | 84 ++++++++++++++++++++++++--------------------------------
 1 file changed, 36 insertions(+), 48 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 2d85d26..e13427c 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -33,7 +33,8 @@ struct cmd_reflog_expire_cb {
 };
 
 struct expire_reflog_cb {
-	FILE *newlog;
+	struct transaction *t;
+	const char *refname;
 	enum {
 		UE_NORMAL,
 		UE_ALWAYS,
@@ -43,6 +44,7 @@ struct expire_reflog_cb {
 	unsigned long mark_limit;
 	struct cmd_reflog_expire_cb *cmd;
 	unsigned char last_kept_sha1[20];
+	struct strbuf *err;
 };
 
 struct collected_reflog {
@@ -316,20 +318,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,
+					      cb->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 +353,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;
+	struct strbuf err = STRBUF_INIT;
 	int status = 0;
 
 	memset(&cb, 0, sizeof(cb));
+	cb.refname = ref;
+	cb.err = &err;
 
-	/*
-	 * 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_truncate_reflog(cb.t, cb.refname, &err)) {
+		status |= error("%s", err.buf);
+		goto cleanup;
 	}
-
 	cb.cmd = cmd;
 
 	if (!cmd->expire_unreachable || !strcmp(ref, "HEAD")) {
@@ -407,7 +404,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 +420,20 @@ 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.buf);
-			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_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(newlog_path);
-	free(log_file);
-	unlock_ref(lock);
+ cleanup:
+	transaction_free(cb.t);
+	strbuf_release(&err);
 	return status;
 }
 
-- 
2.2.0

[PATCH 08/13] 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>
---
 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 c411af9..bdd3f75 100644
--- a/refs.c
+++ b/refs.c
@@ -2941,16 +2941,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);
@@ -3019,16 +3019,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 1afc72c..0c75217 100644
--- a/refs.h
+++ b/refs.h
@@ -207,11 +207,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 int flags,
 		       unsigned long at_time, int cnt,
@@ -221,6 +216,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 09/13] refs.c: remove unlock_ref/close_ref/commit_ref from the refs api

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

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

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

diff --git a/refs.c b/refs.c
index bdd3f75..1468c00 100644
--- a/refs.c
+++ b/refs.c
@@ -2090,6 +2090,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)
@@ -2888,7 +2898,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;
@@ -2896,7 +2906,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;
@@ -2904,16 +2914,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 0c75217..fdbdea6 100644
--- a/refs.h
+++ b/refs.h
@@ -198,15 +198,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 int flags,
 		       unsigned long at_time, int cnt,
-- 
2.2.0

[PATCH 10/13] refs.c: remove lock_any_ref_for_update

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

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

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

diff --git a/refs.c b/refs.c
index 1468c00..796b7cc 100644
--- a/refs.c
+++ b/refs.c
@@ -2346,13 +2346,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, NULL, flags, type_p);
-}
-
 /*
  * 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 fdbdea6..166aab8 100644
--- a/refs.h
+++ b/refs.h
@@ -181,8 +181,7 @@ extern int is_branch(const char *refname);
 extern int peel_ref(const char *refname, unsigned char *sha1);
 
 /*
- * Flags controlling lock_any_ref_for_update(), transaction_update_ref(),
- * transaction_create_ref(), etc.
+ * Flags controlling transaction_update_ref(), transaction_create_ref(), etc.
  * REF_NODEREF: act on the ref directly, instead of dereferencing
  *              symbolic references.
  * REF_DELETING: tolerate broken refs
@@ -191,12 +190,6 @@ extern int peel_ref(const char *refname, unsigned char *sha1);
  */
 #define REF_NODEREF	0x01
 #define REF_DELETING	0x02
-/*
- * This function 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 int flags,
-- 
2.2.0

[PATCH 11/13] refs.c: don't expose the internal struct ref_lock in the header file

[Stefan Beller]

Now the struct ref_lock is used completely internally, so let's
remove it from the header file.

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

Notes:
    This patch is brand new in this series,
    so digest it while it's hot!

 refs.c | 9 +++++++++
 refs.h | 9 ---------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index 796b7cc..b54b5b3 100644
--- a/refs.c
+++ b/refs.c
@@ -6,6 +6,15 @@
 #include "dir.h"
 #include "string-list.h"
 
+struct ref_lock {
+	char *ref_name;
+	char *orig_ref_name;
+	struct lock_file *lk;
+	unsigned char old_sha1[20];
+	int lock_fd;
+	int force_write;
+};
+
 /*
  * How to handle various characters in refnames:
  * 0: An acceptable character for refs
diff --git a/refs.h b/refs.h
index 166aab8..cfc8d90 100644
--- a/refs.h
+++ b/refs.h
@@ -1,15 +1,6 @@
 #ifndef REFS_H
 #define REFS_H
 
-struct ref_lock {
-	char *ref_name;
-	char *orig_ref_name;
-	struct lock_file *lk;
-	unsigned char old_sha1[20];
-	int lock_fd;
-	int force_write;
-};
-
 /*
  * A transaction represents a collection of ref updates
  * that should succeed or fail together.
-- 
2.2.0

[PATCH 12/13] refs.c: use a bit for ref_update have_old

[Stefan Beller]

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

Notes:
    Also a patch, which hasn't been posted on the mailing list before.

 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index b54b5b3..2942227 100644
--- a/refs.c
+++ b/refs.c
@@ -3532,7 +3532,7 @@ struct ref_update {
 	unsigned char new_sha1[20];
 	unsigned char old_sha1[20];
 	int flags; /* REF_NODEREF? */
-	int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
+	int have_old:1; /* 1 if old_sha1 is valid, 0 otherwise */
 	struct ref_lock *lock;
 	int type;
 	char *msg;
-- 
2.2.0

Re: [PATCH 12/13] refs.c: use a bit for ref_update have_old

[Torsten Bögershausen]

On 12/04/2014 09:29 AM, Stefan Beller wrote:
Somewhat short commit message, especially the motivation is missing.

What do we gain with this patch ?
In the struct the compiler needs to layout 2*20 bytes for the sha's.
It follows an int, typically 4 bytes.
It follows another int, now with one bit.
Then we have the pointer "struct ref_lock *lock",
which needs to be aligned on 4 byte boundary for a 32 bit processor,
or an 8 byte boundary for a 64 bit machine.

Our "1 bit int" is padded with 31 bits.
We do not gain anything in memory consumption. (unless we declare int flags
to be 31 bits, and the compiler may join "have_old" and "flags" together
into one int in memory.

But there is a price to pay:
The generated code to fiddle out the bits from an int becomes more complicated.
You need to fetch the memory from one int, mask and shift.
Some processors can do this, out of my mind some ARM can, some can not.

We need to run the compiler to look at the generated code of course.
 


> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
> Notes:
>     Also a patch, which hasn't been posted on the mailing list before.
> 
>  refs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/refs.c b/refs.c
> index b54b5b3..2942227 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3532,7 +3532,7 @@ struct ref_update {
>  	unsigned char new_sha1[20];
>  	unsigned char old_sha1[20];
>  	int flags; /* REF_NODEREF? */
> -	int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
> +	int have_old:1; /* 1 if old_sha1 is valid, 0 otherwise */
>  	struct ref_lock *lock;
>  	int type;
>  	char *msg;
> 

Re: [PATCH 12/13] refs.c: use a bit for ref_update have_old

[Andreas Schwab]

Stefan Beller <sbeller@google.com> writes:

> diff --git a/refs.c b/refs.c
> index b54b5b3..2942227 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3532,7 +3532,7 @@ struct ref_update {
>  	unsigned char new_sha1[20];
>  	unsigned char old_sha1[20];
>  	int flags; /* REF_NODEREF? */
> -	int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
> +	int have_old:1; /* 1 if old_sha1 is valid, 0 otherwise */

A signed one-bit field cannot store a value of 1.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

[PATCH 13/13] 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>
---
 builtin/branch.c  | 5 +++--
 cache.h           | 7 +++++++
 refs.c            | 6 ++++++
 refs.h            | 6 ++++--
 t/t3200-branch.sh | 8 ++++++++
 5 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 3b79c50..04f57d4 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_ALLOW_BROKEN)) {
 			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 2942227..759536c 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;
 			}
@@ -2268,6 +2272,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		if (flags & REF_NODEREF)
 			resolve_flags |= RESOLVE_REF_NO_RECURSE;
 	}
+	if (flags & REF_ALLOW_BROKEN)
+		resolve_flags |= RESOLVE_REF_ALLOW_BAD_SHA1;
 
 	refname = resolve_ref_unsafe(refname, resolve_flags,
 				     lock->old_sha1, &type);
diff --git a/refs.h b/refs.h
index cfc8d90..735f59f 100644
--- a/refs.h
+++ b/refs.h
@@ -176,11 +176,13 @@ extern int peel_ref(const char *refname, unsigned char *sha1);
  * REF_NODEREF: act on the ref directly, instead of dereferencing
  *              symbolic references.
  * REF_DELETING: tolerate broken refs
+ * REF_ALLOW_BROKEN: allow locking refs that are broken.
  *
  * Flags >= 0x100 are reserved for internal use.
  */
-#define REF_NODEREF	0x01
-#define REF_DELETING	0x02
+#define REF_NODEREF		0x01
+#define REF_DELETING		0x02
+#define REF_ALLOW_BROKEN	0x04
 
 /** Reads log for the value of ref during at_time. **/
 extern int read_ref_at(const char *refname, unsigned int flags,
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 432921b..fa7d7bd 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_failure '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: [PATCHv3 00/13] the refs-transactions-reflog series

[Michael Haggerty]

On 12/04/2014 09:29 AM, Stefan Beller wrote:
> This is the whole refs-transactions-reflog series[1],
> which was in discussion for a bit already. It applies to origin/master.

I am still unhappy with the approach of this series, for the reasons
that I explained earlier [1]. In short, I think that the abstraction
level is wrong. In my opinion, consumers of the refs API should barely
even have to *know* about reflogs, let alone implement reflog expiration
themselves.

Of course, reflog expiration *should* be done atomically. But that is
the business of the refs module; callers shouldn't have to do all the
complicated work of building the transaction themselves.

I spent the day working on an alternate proposal, to convert
expire_reflogs() to take callback functions that decide *what* to
expire, but which itself does the work of acquiring locks, iterating
through the reflog entries, rewriting the file, and overwriting the old
file with the new one. The goal is to move this function into refs.c and
make builtin/reflog.c much simpler--it will only have to implement the
callbacks that determine the expiration policy.

I'm almost done with the series but won't be able to finish it until
tomorrow. For those who are impatient, here is my work in progress [2].

Michael

[1]
http://thread.gmane.org/gmane.comp.version-control.git/259712/focus=259770
[2] https://github.com/mhagger/git, branch "reflog-expire-api".

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCHv3 00/13] the refs-transactions-reflog series

[Jonathan Nieder]

Hi,

Michael Haggerty wrote:
> On 12/04/2014 09:29 AM, Stefan Beller wrote:

>> This is the whole refs-transactions-reflog series[1],
>> which was in discussion for a bit already. It applies to origin/master.
>
> I am still unhappy with the approach of this series, for the reasons
> that I explained earlier [1]. In short, I think that the abstraction
> level is wrong. In my opinion, consumers of the refs API should barely
> even have to *know* about reflogs, let alone implement reflog expiration
> themselves.

Would it make sense to propose competing documentation patches (to
Documentation/technical/api-ref-transactions.txt, or to refs.h), so
we can work out the API that way?

I don't think the API questions that we're talking about would end up
affecting the details of how the files backend implements them too
much.

Jonathan

Re: [PATCHv3 00/13] the refs-transactions-reflog series

[Jonathan Nieder]

Michael Haggerty wrote:

> I am still unhappy with the approach of this series, for the reasons
> that I explained earlier [1]. In short, I think that the abstraction
> level is wrong. In my opinion, consumers of the refs API should barely
> even have to *know* about reflogs, let alone implement reflog expiration
> themselves.

Okay, now returning to the substance of this comment.  This is
revisiting themes from [3], so my opinions are probably not a
surprise.

I think that the API changes that you and Stefan are proposing are
consistent and could both go in.

You suggested refactoring expire_reflogs() to use a callback that
decides what to expire.  Then it doesn't have to care that the
expiration happens by creating a new reflog and copying over the
reflog entries that are not being expired.  The result is a clean
reflog expiration API.

The ref-transaction-reflog series allows those low-level steps to be
part of a ref transaction.  Any ref backend (the current files-based
backend or a future other one) would get a chance to reimplement those
low-level steps, which are part of what happens during ref updates and
reflog deletion.  The goal is for all reflog updates to use the
transaction API, so that new ref/reflog backends only need to
implement the transaction functions.

So *both* are making good changes, with different goals.

The implementation of the reflog expiration API can use the ref
transaction API.

> Of course, reflog expiration *should* be done atomically. But that is
> the business of the refs module; callers shouldn't have to do all the
> complicated work of building the transaction themselves.

I don't understand this comment.  After the ref-transaction-reflog
series, a transaction_update_ref() call still takes care of the
corresponding reflog update without the caller having to worry about
it.

Thanks for looking it over,
Jonathan

> [1] http://thread.gmane.org/gmane.comp.version-control.git/259712/focus=259770
[3] http://thread.gmane.org/gmane.comp.version-control.git/259939/focus=259967

Re: [PATCHv3 00/13] the refs-transactions-reflog series

[Stefan Beller]

On Thu, Dec 04, 2014 at 10:14:04AM -0800, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
> > I am still unhappy with the approach of this series, for the reasons
> > that I explained earlier [1]. In short, I think that the abstraction
> > level is wrong. In my opinion, consumers of the refs API should barely
> > even have to *know* about reflogs, let alone implement reflog expiration
> > themselves.
> 
Ok, what is a consumer for you? In my understanding the builtin/reflog.c is
a consumer of the refs API, so there we want to see clean code just telling the
refs backend to do its thing.

I think Jonathan made a good point by saying our patch series have 
different goals.

So I really like the code, which leads to 

    reflog_expiry_prepare(refname, sha1, cb.policy_cb);
    for_each_reflog_ent(refname, expire_reflog_ent, &cb);
    reflog_expiry_cleanup(cb.policy_cb);

but look at the surrounding code:
    
	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
			...
	}


	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
		if (close_lock_file(&reflog_lock)) {
			...
		}
	}

That part should also go into the refs.c backend, so in the expire_reflog
function you can just write:

	transaction_begin(...)  // This does all the hold_lock_file_for_update magic 
				// lines 457-464 in mhagger/reflog-expire-api

 	reflog_expiry_prepare(refname, sha1, cb.policy_cb);
	for_each_reflog_ent(refname, expire_reflog_ent, &cb);
	reflog_expiry_cleanup(cb.policy_cb);

	transaction_commit(...) // This does all the close_lock_file/rename/write_in_full
				// lines 470-488 in mhagger/reflog-expire-api


> 
> So *both* are making good changes, with different goals.

If you want to I can rebase the reflog series on top of yours to show
they can work together quite nicely.

Thanks for your draft series and feedback,
Stefan

Re: [PATCHv3 00/13] the refs-transactions-reflog series

[Michael Haggerty]

On 12/04/2014 07:32 PM, Stefan Beller wrote:
> On Thu, Dec 04, 2014 at 10:14:04AM -0800, Jonathan Nieder wrote:
>> Michael Haggerty wrote:
>>
>>> I am still unhappy with the approach of this series, for the reasons
>>> that I explained earlier [1]. In short, I think that the abstraction
>>> level is wrong. In my opinion, consumers of the refs API should barely
>>> even have to *know* about reflogs, let alone implement reflog expiration
>>> themselves.
>>
> Ok, what is a consumer for you? In my understanding the builtin/reflog.c is
> a consumer of the refs API, so there we want to see clean code just telling the
> refs backend to do its thing.
> 
> I think Jonathan made a good point by saying our patch series have 
> different goals.
> 
> So I really like the code, which leads to 
> 
>     reflog_expiry_prepare(refname, sha1, cb.policy_cb);
>     for_each_reflog_ent(refname, expire_reflog_ent, &cb);
>     reflog_expiry_cleanup(cb.policy_cb);
> 
> but look at the surrounding code:
>     
> 	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
> 		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
> 			...
> 	}
> 
> 
> 	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
> 		if (close_lock_file(&reflog_lock)) {
> 			...
> 		}
> 	}
> 
> That part should also go into the refs.c backend, so in the expire_reflog
> function you can just write:
> 
> 	transaction_begin(...)  // This does all the hold_lock_file_for_update magic 
> 				// lines 457-464 in mhagger/reflog-expire-api
> 
>  	reflog_expiry_prepare(refname, sha1, cb.policy_cb);
> 	for_each_reflog_ent(refname, expire_reflog_ent, &cb);
> 	reflog_expiry_cleanup(cb.policy_cb);
> 
> 	transaction_commit(...) // This does all the close_lock_file/rename/write_in_full
> 				// lines 470-488 in mhagger/reflog-expire-api

I'm pleasantly surprised that you guys have already looked at my work in
progress. I wish I had had more time earlier today to explain what is
still to be done:

The whole point of all of the refactoring is to move expire_reflog()
(and supporting functions like expire_reflog_ent()) to refs.c. The
"surrounding code" that you mention above would be moved there and would
*not* need to be done by callers.

expire_reflog() will gain three callback function pointers as
parameters. The caller (in this case reflog.c) will pass pointers to
reflog_expiry_prepare(), reflog_expiry_cleanup(), and
should_expire_reflog_ent() into expire_reflog().

There is no need to wrap the code in a transaction, because the
"surrounding code" that you mentioned implements all the "transaction"
that is needed! There is no need to complicated the *ref_transaction*
interface to allow arbitrary reflog updates when all we need is this one
very special case, plus of course the reflog appends that (I claim)
should happen implicitly whenever a reference is updated [1].

>> So *both* are making good changes, with different goals.
> 
> If you want to I can rebase the reflog series on top of yours to show
> they can work together quite nicely.

Feel free to do what you want, but I really don't think we will ever
need transactions to handle generic reflog updates.

Meanwhile I'll try to get my series finished, including API docs as
Jonathan requested. I hope the code will be more convincing than my
prose :-)

Michael

[1] Of course, only for references that have reflogs enabled.

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCHv3 00/13] the refs-transactions-reflog series

[Junio C Hamano]

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

> ... an alternate proposal, to convert
> expire_reflogs() to take callback functions that decide *what* to
> expire, but which itself does the work of acquiring locks, iterating
> through the reflog entries, rewriting the file, and overwriting the old
> file with the new one. The goal is to move this function into refs.c and
> make builtin/reflog.c much simpler--it will only have to implement the
> callbacks that determine the expiration policy.

It sounds like a very sensible approach to me.

Re: [PATCHv3 00/13] the refs-transactions-reflog series

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> This is the whole refs-transactions-reflog series[1],
> which was in discussion for a bit already. It applies to origin/master.
>
> The idea is to have the reflog being part of the transactions, which
> the refs are already using, so the we're moving towards a database
> like API in the long run. This makes git easier to maintain as well
> as opening the possibility to replace the backend with a real database.
>
> If you've followed the topic a bit, start reading at patch
> "[PATCH 06/13] refs.c: add a transaction function to append a reflog"
> as the first 5 patches have been discussed a lot separately and
> can be found in origin/pu already[2].

> The first two patches are deduplicating code.
> The third patch is ripping some code out of log_ref_write and introduces
> log_ref_write_fd, which does the actual writing.
> The patches 4+5 are renaming variables for clarity.

Thanks.  It seems that we have a bit of hashing out the approaches
that is necessary between Michael's and this one.  I'll refrain from
picking this up for today (even though I would read it through as
time permits).  It might turn out to be necessary to drop those five
early patches from my tree when this series gets rerolled to take
input from the alternative Michael's working on, but that droppage
does not have to happen today.

Re: [PATCHv3 00/13] the refs-transactions-reflog series

[Stefan Beller]

On Thu, Dec 4, 2014 at 10:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> This is the whole refs-transactions-reflog series[1],
>> which was in discussion for a bit already. It applies to origin/master.
>>
>> The idea is to have the reflog being part of the transactions, which
>> the refs are already using, so the we're moving towards a database
>> like API in the long run. This makes git easier to maintain as well
>> as opening the possibility to replace the backend with a real database.
>>
>> If you've followed the topic a bit, start reading at patch
>> "[PATCH 06/13] refs.c: add a transaction function to append a reflog"
>> as the first 5 patches have been discussed a lot separately and
>> can be found in origin/pu already[2].
>
>> The first two patches are deduplicating code.
>> The third patch is ripping some code out of log_ref_write and introduces
>> log_ref_write_fd, which does the actual writing.
>> The patches 4+5 are renaming variables for clarity.
>
> Thanks.  It seems that we have a bit of hashing out the approaches
> that is necessary between Michael's and this one.  I'll refrain from
> picking this up for today (even though I would read it through as
> time permits).  It might turn out to be necessary to drop those five
> early patches from my tree when this series gets rerolled to take
> input from the alternative Michael's working on, but that droppage
> does not have to happen today.
>

Ok, let's see how Michaels series evolves and if I can base the
transactions series
on that afterwards. Michael has picked up 3 out the 5 patches to get
his series started,
so maybe we can resolve it nicely.

Re: [PATCHv3 00/13] the refs-transactions-reflog series

[Jonathan Nieder]

Stefan Beller wrote:

> Ok, let's see how Michaels series evolves and if I can base the
> transactions series
> on that

That sounds good to me, too, FWIW.  That way we don't have to fuss
with conflicts in refs.c. :)

Thanks,
Jonathan