[PATCH v2 00/24] Add reflog_expire() to the references API

[PATCH v2 00/24] Add reflog_expire() to the references API

[Michael Haggerty]

This is v2 of the series. Thanks to Jonathan, Stefan, Junio, and
Ronnie for their feedback on v1 [1]. I think I have addressed all of
the issues that were raised.

Changes since v1:

* Several improvements to commit messages and comments; added some
  Reviewed-by comments from the mailing list.

* Change the signature of expire_reflog() early in the series to cast
  off its heritage as an each_ref_fn.

* Move the static lock_file object into expire_reflog(), and explain
  the locking policy better.

* Report errors if hold_lock_file_for_update() or fdopen_lock_file()
  fails.

* Fix the capitalization in some error messages.

* When "enum expire_reflog_flags" is first introduced, put its
  definition earlier in the file so that a later patch in the series
  doesn't have to move it.

* Rename reflog_expiry_select_fn to reflog_expiry_should_prune_fn.

* Append Stefan's patch 24/24 "refs.c: let fprintf handle the
  formatting"

This branch is also available on GitHub:

    https://github.com/mhagger/git.git, branch reflog-expire-api-v2

Michael

[1] http://thread.gmane.org/gmane.comp.version-control.git/260812

Michael Haggerty (18):
  expire_reflog(): it's not an each_ref_fn anymore
  expire_reflog(): rename "ref" parameter to "refname"
  expire_reflog(): return early if the reference has no reflog
  expire_reflog(): use a lock_file for rewriting the reflog file
  Extract function should_expire_reflog_ent()
  expire_reflog(): extract two policy-related functions
  expire_reflog(): add a "flags" argument
  expire_reflog(): move dry_run to flags argument
  expire_reflog(): move updateref to flags argument
  Rename expire_reflog_cb to expire_reflog_policy_cb
  struct expire_reflog_cb: a new callback data type
  expire_reflog(): pass flags through to expire_reflog_ent()
  expire_reflog(): move verbose to flags argument
  expire_reflog(): move rewrite to flags argument
  Move newlog and last_kept_sha1 to "struct expire_reflog_cb"
  expire_reflog(): treat the policy callback data as opaque
  reflog_expire(): new function in the reference API
  lock_any_ref_for_update(): inline function

Ronnie Sahlberg (4):
  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: remove unlock_ref/close_ref/commit_ref from the refs api

Stefan Beller (2):
  refs.c: don't expose the internal struct ref_lock in the header file
  refs.c: let fprintf handle the formatting

 builtin/reflog.c | 259 ++++++++++++++++++++++--------------------------------
 refs.c           | 263 ++++++++++++++++++++++++++++++++++++++-----------------
 refs.h           |  75 ++++++++++------
 3 files changed, 332 insertions(+), 265 deletions(-)

-- 
2.1.3

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

[Michael Haggerty]

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>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 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.1.3

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

[Michael Haggerty]

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>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 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.1.3

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

[Michael Haggerty]

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. It is a nice unit of work.

For now this is only used from log_ref_write, but in the future it
might have other callers.

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

[PATCH v2 04/24] expire_reflog(): it's not an each_ref_fn anymore

[Michael Haggerty]

Prior to v1.5.4~14, expire_reflog() had to be an each_ref_fn because
it was passed to for_each_reflog(). Since then, there has been no
reason for it to implement the each_ref_fn interface. So...

* Remove the "unused" parameter (which took the place of "flags", but
  was really unused).

* Declare the last parameter to be (struct cmd_reflog_expire_cb *)
  rather than (void *).

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/reflog.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 2d85d26..160541a 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -349,9 +349,9 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
 	return 0;
 }
 
-static int expire_reflog(const char *ref, const unsigned char *sha1, int unused, void *cb_data)
+static int expire_reflog(const char *ref, const unsigned char *sha1,
+			 struct cmd_reflog_expire_cb *cmd)
 {
-	struct cmd_reflog_expire_cb *cmd = cb_data;
 	struct expire_reflog_cb cb;
 	struct ref_lock *lock;
 	char *log_file, *newlog_path = NULL;
@@ -663,7 +663,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		for (i = 0; i < collected.nr; i++) {
 			struct collected_reflog *e = collected.e[i];
 			set_reflog_expiry_param(&cb, explicit_expiry, e->reflog);
-			status |= expire_reflog(e->reflog, e->sha1, 0, &cb);
+			status |= expire_reflog(e->reflog, e->sha1, &cb);
 			free(e);
 		}
 		free(collected.e);
@@ -677,7 +677,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 		set_reflog_expiry_param(&cb, explicit_expiry, ref);
-		status |= expire_reflog(ref, sha1, 0, &cb);
+		status |= expire_reflog(ref, sha1, &cb);
 	}
 	return status;
 }
@@ -748,7 +748,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 			cb.expire_total = 0;
 		}
 
-		status |= expire_reflog(ref, sha1, 0, &cb);
+		status |= expire_reflog(ref, sha1, &cb);
 		free(ref);
 	}
 	return status;
-- 
2.1.3

[PATCH v2 05/24] expire_reflog(): rename "ref" parameter to "refname"

[Michael Haggerty]

This is our usual convention.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/reflog.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 160541a..ff51dbf 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -349,7 +349,7 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
 	return 0;
 }
 
-static int expire_reflog(const char *ref, const unsigned char *sha1,
+static int expire_reflog(const char *refname, const unsigned char *sha1,
 			 struct cmd_reflog_expire_cb *cmd)
 {
 	struct expire_reflog_cb cb;
@@ -365,20 +365,20 @@ static int expire_reflog(const char *ref, const unsigned char *sha1,
 	 * we take the lock for the ref itself to prevent it from
 	 * getting updated.
 	 */
-	lock = lock_any_ref_for_update(ref, sha1, 0, NULL);
+	lock = lock_any_ref_for_update(refname, sha1, 0, NULL);
 	if (!lock)
-		return error("cannot lock ref '%s'", ref);
-	log_file = git_pathdup("logs/%s", ref);
-	if (!reflog_exists(ref))
+		return error("cannot lock ref '%s'", refname);
+	log_file = git_pathdup("logs/%s", refname);
+	if (!reflog_exists(refname))
 		goto finish;
 	if (!cmd->dry_run) {
-		newlog_path = git_pathdup("logs/%s.lock", ref);
+		newlog_path = git_pathdup("logs/%s.lock", refname);
 		cb.newlog = fopen(newlog_path, "w");
 	}
 
 	cb.cmd = cmd;
 
-	if (!cmd->expire_unreachable || !strcmp(ref, "HEAD")) {
+	if (!cmd->expire_unreachable || !strcmp(refname, "HEAD")) {
 		tip_commit = NULL;
 		cb.unreachable_expire_kind = UE_HEAD;
 	} else {
@@ -407,7 +407,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1,
 		mark_reachable(&cb);
 	}
 
-	for_each_reflog_ent(ref, expire_reflog_ent, &cb);
+	for_each_reflog_ent(refname, expire_reflog_ent, &cb);
 
 	if (cb.unreachable_expire_kind != UE_ALWAYS) {
 		if (cb.unreachable_expire_kind == UE_HEAD) {
-- 
2.1.3

[PATCH v2 06/24] expire_reflog(): return early if the reference has no reflog

[Michael Haggerty]

There is very little cleanup needed if the reference has no reflog. If
we move the initialization of log_file down a bit, there's even less.
So instead of jumping to the cleanup code at the end of the function,
just do the cleanup and return inline.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/reflog.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index ff51dbf..37b33c9 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -368,9 +368,11 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 	lock = lock_any_ref_for_update(refname, sha1, 0, NULL);
 	if (!lock)
 		return error("cannot lock ref '%s'", refname);
+	if (!reflog_exists(refname)) {
+		unlock_ref(lock);
+		return 0;
+	}
 	log_file = git_pathdup("logs/%s", refname);
-	if (!reflog_exists(refname))
-		goto finish;
 	if (!cmd->dry_run) {
 		newlog_path = git_pathdup("logs/%s.lock", refname);
 		cb.newlog = fopen(newlog_path, "w");
@@ -419,7 +421,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 			clear_commit_marks(tip_commit, REACHABLE);
 		}
 	}
- finish:
+
 	if (cb.newlog) {
 		if (fclose(cb.newlog)) {
 			status |= error("%s: %s", strerror(errno),
-- 
2.1.3

[PATCH v2 07/24] expire_reflog(): use a lock_file for rewriting the reflog file

[Michael Haggerty]

We don't actually need the locking functionality, because we already
hold the lock on the reference itself, which is how the reflog file is
locked. But the lock_file code can do some of the bookkeeping for us,
and it is more careful than the old code here was. For example:

* It correctly handles the case that the reflog lock file already
  exists for some reason or cannot be opened.

* It correctly cleans up the lockfile if the program dies.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/reflog.c | 60 ++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 19 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 37b33c9..ba5b3d3 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -352,9 +352,10 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
 static int expire_reflog(const char *refname, const unsigned char *sha1,
 			 struct cmd_reflog_expire_cb *cmd)
 {
+	static struct lock_file reflog_lock;
 	struct expire_reflog_cb cb;
 	struct ref_lock *lock;
-	char *log_file, *newlog_path = NULL;
+	char *log_file;
 	struct commit *tip_commit;
 	struct commit_list *tips;
 	int status = 0;
@@ -362,8 +363,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 	memset(&cb, 0, sizeof(cb));
 
 	/*
-	 * we take the lock for the ref itself to prevent it from
-	 * getting updated.
+	 * The reflog file is locked by holding the lock on the
+	 * reference itself, plus we might need to update the
+	 * reference if --updateref was specified:
 	 */
 	lock = lock_any_ref_for_update(refname, sha1, 0, NULL);
 	if (!lock)
@@ -372,10 +374,29 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 		unlock_ref(lock);
 		return 0;
 	}
+
 	log_file = git_pathdup("logs/%s", refname);
 	if (!cmd->dry_run) {
-		newlog_path = git_pathdup("logs/%s.lock", refname);
-		cb.newlog = fopen(newlog_path, "w");
+		/*
+		 * Even though holding $GIT_DIR/logs/$reflog.lock has
+		 * no locking implications, we use the lock_file
+		 * machinery here anyway because it does a lot of the
+		 * work we need, including cleaning up if the program
+		 * exits unexpectedly.
+		 */
+		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0) {
+			struct strbuf err = STRBUF_INIT;
+			unable_to_lock_message(log_file, errno, &err);
+			error("%s", err.buf);
+			strbuf_release(&err);
+			goto failure;
+		}
+		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
+		if (!cb.newlog) {
+			error("cannot fdopen %s (%s)",
+			      reflog_lock.filename.buf, strerror(errno));
+			goto failure;
+		}
 	}
 
 	cb.cmd = cmd;
@@ -423,32 +444,33 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 	}
 
 	if (cb.newlog) {
-		if (fclose(cb.newlog)) {
-			status |= error("%s: %s", strerror(errno),
-					newlog_path);
-			unlink(newlog_path);
+		if (close_lock_file(&reflog_lock)) {
+			status |= error("couldn't write %s: %s", log_file,
+					strerror(errno));
 		} 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",
+			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);
+			rollback_lock_file(&reflog_lock);
+		} else if (commit_lock_file(&reflog_lock)) {
+			status |= error("unable to commit reflog '%s' (%s)",
+					log_file, strerror(errno));
 		} else if (cmd->updateref && commit_ref(lock)) {
-			status |= error("Couldn't set %s", lock->ref_name);
-		} else {
-			adjust_shared_perm(log_file);
+			status |= error("couldn't set %s", lock->ref_name);
 		}
 	}
-	free(newlog_path);
 	free(log_file);
 	unlock_ref(lock);
 	return status;
+
+ failure:
+	rollback_lock_file(&reflog_lock);
+	free(log_file);
+	unlock_ref(lock);
+	return -1;
 }
 
 static int collect_reflog(const char *ref, const unsigned char *sha1, int unused, void *cb_data)
-- 
2.1.3

[PATCH v2 08/24] Extract function should_expire_reflog_ent()

[Michael Haggerty]

Extract from expire_reflog_ent() a function that is solely responsible
for deciding whether a reflog entry should be expired. By separating
this "business logic" from the mechanics of actually expiring entries,
we are working towards the goal of encapsulating reflog expiry within
the refs API, with policy decided by a callback function passed to it
by its caller.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/reflog.c | 70 +++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 42 insertions(+), 28 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index ba5b3d3..06ce8b1 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -288,51 +288,65 @@ static int unreachable(struct expire_reflog_cb *cb, struct commit *commit, unsig
 	return !(commit->object.flags & REACHABLE);
 }
 
-static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
-		const char *email, unsigned long timestamp, int tz,
-		const char *message, void *cb_data)
+/*
+ * Return true iff the specified reflog entry should be expired.
+ */
+static int should_expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+				    const char *email, unsigned long timestamp, int tz,
+				    const char *message, void *cb_data)
 {
 	struct expire_reflog_cb *cb = cb_data;
 	struct commit *old, *new;
 
 	if (timestamp < cb->cmd->expire_total)
-		goto prune;
-
-	if (cb->cmd->rewrite)
-		osha1 = cb->last_kept_sha1;
+		return 1;
 
 	old = new = NULL;
 	if (cb->cmd->stalefix &&
 	    (!keep_entry(&old, osha1) || !keep_entry(&new, nsha1)))
-		goto prune;
+		return 1;
 
 	if (timestamp < cb->cmd->expire_unreachable) {
 		if (cb->unreachable_expire_kind == UE_ALWAYS)
-			goto prune;
+			return 1;
 		if (unreachable(cb, old, osha1) || unreachable(cb, new, nsha1))
-			goto prune;
+			return 1;
 	}
 
 	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);
-		hashcpy(cb->last_kept_sha1, nsha1);
-	}
-	if (cb->cmd->verbose)
-		printf("keep %s", message);
+		return 1;
+
 	return 0;
- prune:
-	if (!cb->newlog)
-		printf("would prune %s", message);
-	else if (cb->cmd->verbose)
-		printf("prune %s", message);
+}
+
+static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+		const char *email, unsigned long timestamp, int tz,
+		const char *message, void *cb_data)
+{
+	struct expire_reflog_cb *cb = cb_data;
+
+	if (cb->cmd->rewrite)
+		osha1 = cb->last_kept_sha1;
+
+	if (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,
+				     message, cb_data)) {
+		if (!cb->newlog)
+			printf("would prune %s", message);
+		else if (cb->cmd->verbose)
+			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",
+				sha1_to_hex(osha1), sha1_to_hex(nsha1),
+				email, timestamp, sign, zone,
+				message);
+			hashcpy(cb->last_kept_sha1, nsha1);
+		}
+		if (cb->cmd->verbose)
+			printf("keep %s", message);
+	}
 	return 0;
 }
 
-- 
2.1.3

[PATCH v2 09/24] expire_reflog(): extract two policy-related functions

[Michael Haggerty]

Extract two functions, reflog_expiry_prepare() and
reflog_expiry_cleanup(), from expire_reflog(). This is a further step
towards separating the code for deciding on expiration policy from the
code that manages the physical deletion of reflog entries.

This change requires a couple of local variables from expire_reflog()
to be turned into fields of "struct expire_reflog_cb". More
reorganization of the callback data will follow in later commits.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 builtin/reflog.c | 94 +++++++++++++++++++++++++++++++-------------------------
 1 file changed, 52 insertions(+), 42 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 06ce8b1..8db52d7 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -43,6 +43,8 @@ struct expire_reflog_cb {
 	unsigned long mark_limit;
 	struct cmd_reflog_expire_cb *cmd;
 	unsigned char last_kept_sha1[20];
+	struct commit *tip_commit;
+	struct commit_list *tips;
 };
 
 struct collected_reflog {
@@ -363,6 +365,54 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
 	return 0;
 }
 
+static void reflog_expiry_prepare(const char *refname,
+				  const unsigned char *sha1,
+				  struct expire_reflog_cb *cb)
+{
+	if (!cb->cmd->expire_unreachable || !strcmp(refname, "HEAD")) {
+		cb->tip_commit = NULL;
+		cb->unreachable_expire_kind = UE_HEAD;
+	} else {
+		cb->tip_commit = lookup_commit_reference_gently(sha1, 1);
+		if (!cb->tip_commit)
+			cb->unreachable_expire_kind = UE_ALWAYS;
+		else
+			cb->unreachable_expire_kind = UE_NORMAL;
+	}
+
+	if (cb->cmd->expire_unreachable <= cb->cmd->expire_total)
+		cb->unreachable_expire_kind = UE_ALWAYS;
+
+	cb->mark_list = NULL;
+	cb->tips = NULL;
+	if (cb->unreachable_expire_kind != UE_ALWAYS) {
+		if (cb->unreachable_expire_kind == UE_HEAD) {
+			struct commit_list *elem;
+			for_each_ref(push_tip_to_list, &cb->tips);
+			for (elem = cb->tips; elem; elem = elem->next)
+				commit_list_insert(elem->item, &cb->mark_list);
+		} else {
+			commit_list_insert(cb->tip_commit, &cb->mark_list);
+		}
+		cb->mark_limit = cb->cmd->expire_total;
+		mark_reachable(cb);
+	}
+}
+
+static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)
+{
+	if (cb->unreachable_expire_kind != UE_ALWAYS) {
+		if (cb->unreachable_expire_kind == UE_HEAD) {
+			struct commit_list *elem;
+			for (elem = cb->tips; elem; elem = elem->next)
+				clear_commit_marks(elem->item, REACHABLE);
+			free_commit_list(cb->tips);
+		} else {
+			clear_commit_marks(cb->tip_commit, REACHABLE);
+		}
+	}
+}
+
 static int expire_reflog(const char *refname, const unsigned char *sha1,
 			 struct cmd_reflog_expire_cb *cmd)
 {
@@ -370,8 +420,6 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 	struct expire_reflog_cb cb;
 	struct ref_lock *lock;
 	char *log_file;
-	struct commit *tip_commit;
-	struct commit_list *tips;
 	int status = 0;
 
 	memset(&cb, 0, sizeof(cb));
@@ -415,47 +463,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 
 	cb.cmd = cmd;
 
-	if (!cmd->expire_unreachable || !strcmp(refname, "HEAD")) {
-		tip_commit = NULL;
-		cb.unreachable_expire_kind = UE_HEAD;
-	} else {
-		tip_commit = lookup_commit_reference_gently(sha1, 1);
-		if (!tip_commit)
-			cb.unreachable_expire_kind = UE_ALWAYS;
-		else
-			cb.unreachable_expire_kind = UE_NORMAL;
-	}
-
-	if (cmd->expire_unreachable <= cmd->expire_total)
-		cb.unreachable_expire_kind = UE_ALWAYS;
-
-	cb.mark_list = NULL;
-	tips = NULL;
-	if (cb.unreachable_expire_kind != UE_ALWAYS) {
-		if (cb.unreachable_expire_kind == UE_HEAD) {
-			struct commit_list *elem;
-			for_each_ref(push_tip_to_list, &tips);
-			for (elem = tips; elem; elem = elem->next)
-				commit_list_insert(elem->item, &cb.mark_list);
-		} else {
-			commit_list_insert(tip_commit, &cb.mark_list);
-		}
-		cb.mark_limit = cmd->expire_total;
-		mark_reachable(&cb);
-	}
-
+	reflog_expiry_prepare(refname, sha1, &cb);
 	for_each_reflog_ent(refname, expire_reflog_ent, &cb);
-
-	if (cb.unreachable_expire_kind != UE_ALWAYS) {
-		if (cb.unreachable_expire_kind == UE_HEAD) {
-			struct commit_list *elem;
-			for (elem = tips; elem; elem = elem->next)
-				clear_commit_marks(elem->item, REACHABLE);
-			free_commit_list(tips);
-		} else {
-			clear_commit_marks(tip_commit, REACHABLE);
-		}
-	}
+	reflog_expiry_cleanup(&cb);
 
 	if (cb.newlog) {
 		if (close_lock_file(&reflog_lock)) {
-- 
2.1.3

[PATCH v2 10/24] expire_reflog(): add a "flags" argument

[Michael Haggerty]

We want to separate the options relevant to the expiry machinery from
the options affecting the expiration policy. So add a "flags" argument
to expire_reflog() to hold the former.

The argument doesn't yet do anything.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 builtin/reflog.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 8db52d7..dfff5f2 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -414,7 +414,7 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)
 }
 
 static int expire_reflog(const char *refname, const unsigned char *sha1,
-			 struct cmd_reflog_expire_cb *cmd)
+			 unsigned int flags, struct cmd_reflog_expire_cb *cmd)
 {
 	static struct lock_file reflog_lock;
 	struct expire_reflog_cb cb;
@@ -642,6 +642,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	unsigned long now = time(NULL);
 	int i, status, do_all;
 	int explicit_expiry = 0;
+	unsigned int flags = 0;
 
 	default_reflog_expire_unreachable = now - 30 * 24 * 3600;
 	default_reflog_expire = now - 90 * 24 * 3600;
@@ -711,7 +712,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		for (i = 0; i < collected.nr; i++) {
 			struct collected_reflog *e = collected.e[i];
 			set_reflog_expiry_param(&cb, explicit_expiry, e->reflog);
-			status |= expire_reflog(e->reflog, e->sha1, &cb);
+			status |= expire_reflog(e->reflog, e->sha1, flags, &cb);
 			free(e);
 		}
 		free(collected.e);
@@ -725,7 +726,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 		set_reflog_expiry_param(&cb, explicit_expiry, ref);
-		status |= expire_reflog(ref, sha1, &cb);
+		status |= expire_reflog(ref, sha1, flags, &cb);
 	}
 	return status;
 }
@@ -744,6 +745,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 {
 	struct cmd_reflog_expire_cb cb;
 	int i, status = 0;
+	unsigned int flags = 0;
 
 	memset(&cb, 0, sizeof(cb));
 
@@ -796,7 +798,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 			cb.expire_total = 0;
 		}
 
-		status |= expire_reflog(ref, sha1, &cb);
+		status |= expire_reflog(ref, sha1, flags, &cb);
 		free(ref);
 	}
 	return status;
-- 
2.1.3

[PATCH v2 11/24] expire_reflog(): move dry_run to flags argument

[Michael Haggerty]

The policy objects don't care about "--dry-run". So move it to
expire_reflog()'s flags parameter.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 builtin/reflog.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index dfff5f2..2f21735 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -20,9 +20,12 @@ static const char reflog_delete_usage[] =
 static unsigned long default_reflog_expire;
 static unsigned long default_reflog_expire_unreachable;
 
+enum expire_reflog_flags {
+	EXPIRE_REFLOGS_DRY_RUN = 1 << 0
+};
+
 struct cmd_reflog_expire_cb {
 	struct rev_info revs;
-	int dry_run;
 	int stalefix;
 	int rewrite;
 	int updateref;
@@ -438,7 +441,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 	}
 
 	log_file = git_pathdup("logs/%s", refname);
-	if (!cmd->dry_run) {
+	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
 		/*
 		 * Even though holding $GIT_DIR/logs/$reflog.lock has
 		 * no locking implications, we use the lock_file
@@ -467,7 +470,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 	for_each_reflog_ent(refname, expire_reflog_ent, &cb);
 	reflog_expiry_cleanup(&cb);
 
-	if (cb.newlog) {
+	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
 		if (close_lock_file(&reflog_lock)) {
 			status |= error("couldn't write %s: %s", log_file,
 					strerror(errno));
@@ -658,7 +661,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
-			cb.dry_run = 1;
+			flags |= EXPIRE_REFLOGS_DRY_RUN;
 		else if (starts_with(arg, "--expire=")) {
 			if (parse_expiry_date(arg + 9, &cb.expire_total))
 				die(_("'%s' is not a valid timestamp"), arg);
@@ -752,7 +755,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
-			cb.dry_run = 1;
+			flags |= EXPIRE_REFLOGS_DRY_RUN;
 		else if (!strcmp(arg, "--rewrite"))
 			cb.rewrite = 1;
 		else if (!strcmp(arg, "--updateref"))
-- 
2.1.3

[PATCH v2 12/24] expire_reflog(): move updateref to flags argument

[Michael Haggerty]

The policy objects don't care about "--updateref". So move it to
expire_reflog()'s flags parameter.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 builtin/reflog.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 2f21735..e238fe0 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -21,14 +21,14 @@ static unsigned long default_reflog_expire;
 static unsigned long default_reflog_expire_unreachable;
 
 enum expire_reflog_flags {
-	EXPIRE_REFLOGS_DRY_RUN = 1 << 0
+	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
+	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1
 };
 
 struct cmd_reflog_expire_cb {
 	struct rev_info revs;
 	int stalefix;
 	int rewrite;
-	int updateref;
 	int verbose;
 	unsigned long expire_total;
 	unsigned long expire_unreachable;
@@ -474,7 +474,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 		if (close_lock_file(&reflog_lock)) {
 			status |= error("couldn't write %s: %s", log_file,
 					strerror(errno));
-		} else if (cmd->updateref &&
+		} 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 ||
@@ -485,7 +485,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 		} else if (commit_lock_file(&reflog_lock)) {
 			status |= error("unable to commit reflog '%s' (%s)",
 					log_file, strerror(errno));
-		} else if (cmd->updateref && commit_ref(lock)) {
+		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {
 			status |= error("couldn't set %s", lock->ref_name);
 		}
 	}
@@ -677,7 +677,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		else if (!strcmp(arg, "--rewrite"))
 			cb.rewrite = 1;
 		else if (!strcmp(arg, "--updateref"))
-			cb.updateref = 1;
+			flags |= EXPIRE_REFLOGS_UPDATE_REF;
 		else if (!strcmp(arg, "--all"))
 			do_all = 1;
 		else if (!strcmp(arg, "--verbose"))
@@ -759,7 +759,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 		else if (!strcmp(arg, "--rewrite"))
 			cb.rewrite = 1;
 		else if (!strcmp(arg, "--updateref"))
-			cb.updateref = 1;
+			flags |= EXPIRE_REFLOGS_UPDATE_REF;
 		else if (!strcmp(arg, "--verbose"))
 			cb.verbose = 1;
 		else if (!strcmp(arg, "--")) {
-- 
2.1.3

[PATCH v2 13/24] Rename expire_reflog_cb to expire_reflog_policy_cb

[Michael Haggerty]

This is the first step towards separating the data needed by the
policy code from the data needed by the reflog expiration machinery.

(In a moment we will add a *new* "struct expire_reflog_cb" for the use
of expire_reflog() itself, then move fields selectively from
expire_reflog_policy_cb to expire_reflog_cb.)

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 builtin/reflog.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index e238fe0..94c34fc 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -35,7 +35,7 @@ struct cmd_reflog_expire_cb {
 	int recno;
 };
 
-struct expire_reflog_cb {
+struct expire_reflog_policy_cb {
 	FILE *newlog;
 	enum {
 		UE_NORMAL,
@@ -225,7 +225,7 @@ static int keep_entry(struct commit **it, unsigned char *sha1)
  * the expire_limit and queue them back, so that the caller can call
  * us again to restart the traversal with longer expire_limit.
  */
-static void mark_reachable(struct expire_reflog_cb *cb)
+static void mark_reachable(struct expire_reflog_policy_cb *cb)
 {
 	struct commit *commit;
 	struct commit_list *pending;
@@ -264,7 +264,7 @@ static void mark_reachable(struct expire_reflog_cb *cb)
 	cb->mark_list = leftover;
 }
 
-static int unreachable(struct expire_reflog_cb *cb, struct commit *commit, unsigned char *sha1)
+static int unreachable(struct expire_reflog_policy_cb *cb, struct commit *commit, unsigned char *sha1)
 {
 	/*
 	 * We may or may not have the commit yet - if not, look it
@@ -300,7 +300,7 @@ static int should_expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 				    const char *email, unsigned long timestamp, int tz,
 				    const char *message, void *cb_data)
 {
-	struct expire_reflog_cb *cb = cb_data;
+	struct expire_reflog_policy_cb *cb = cb_data;
 	struct commit *old, *new;
 
 	if (timestamp < cb->cmd->expire_total)
@@ -328,7 +328,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 		const char *email, unsigned long timestamp, int tz,
 		const char *message, void *cb_data)
 {
-	struct expire_reflog_cb *cb = cb_data;
+	struct expire_reflog_policy_cb *cb = cb_data;
 
 	if (cb->cmd->rewrite)
 		osha1 = cb->last_kept_sha1;
@@ -355,7 +355,8 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 	return 0;
 }
 
-static int push_tip_to_list(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
+static int push_tip_to_list(const char *refname, const unsigned char *sha1,
+			    int flags, void *cb_data)
 {
 	struct commit_list **list = cb_data;
 	struct commit *tip_commit;
@@ -370,7 +371,7 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
 
 static void reflog_expiry_prepare(const char *refname,
 				  const unsigned char *sha1,
-				  struct expire_reflog_cb *cb)
+				  struct expire_reflog_policy_cb *cb)
 {
 	if (!cb->cmd->expire_unreachable || !strcmp(refname, "HEAD")) {
 		cb->tip_commit = NULL;
@@ -402,7 +403,7 @@ static void reflog_expiry_prepare(const char *refname,
 	}
 }
 
-static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)
+static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)
 {
 	if (cb->unreachable_expire_kind != UE_ALWAYS) {
 		if (cb->unreachable_expire_kind == UE_HEAD) {
@@ -420,7 +421,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 			 unsigned int flags, struct cmd_reflog_expire_cb *cmd)
 {
 	static struct lock_file reflog_lock;
-	struct expire_reflog_cb cb;
+	struct expire_reflog_policy_cb cb;
 	struct ref_lock *lock;
 	char *log_file;
 	int status = 0;
-- 
2.1.3

[PATCH v2 14/24] struct expire_reflog_cb: a new callback data type

[Michael Haggerty]

Add a new data type, "struct expire_reflog_cb", for holding the data
that expire_reflog() passes to expire_reflog_ent() via
for_each_reflog_ent(). For now it only holds a pointer to a "struct
expire_reflog_policy_cb", which still contains all of the actual data.
In future commits we will move some fields from the latter to the
former.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 builtin/reflog.c | 43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 94c34fc..9ee66d4 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -50,10 +50,15 @@ struct expire_reflog_policy_cb {
 	struct commit_list *tips;
 };
 
+struct expire_reflog_cb {
+	void *policy_cb;
+};
+
 struct collected_reflog {
 	unsigned char sha1[20];
 	char reflog[FLEX_ARRAY];
 };
+
 struct collect_reflog_cb {
 	struct collected_reflog **e;
 	int alloc;
@@ -328,28 +333,29 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 		const char *email, unsigned long timestamp, int tz,
 		const char *message, void *cb_data)
 {
-	struct expire_reflog_policy_cb *cb = cb_data;
+	struct expire_reflog_cb *cb = cb_data;
+	struct expire_reflog_policy_cb *policy_cb = cb->policy_cb;
 
-	if (cb->cmd->rewrite)
-		osha1 = cb->last_kept_sha1;
+	if (policy_cb->cmd->rewrite)
+		osha1 = policy_cb->last_kept_sha1;
 
 	if (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,
-				     message, cb_data)) {
-		if (!cb->newlog)
+				     message, policy_cb)) {
+		if (!policy_cb->newlog)
 			printf("would prune %s", message);
-		else if (cb->cmd->verbose)
+		else if (policy_cb->cmd->verbose)
 			printf("prune %s", message);
 	} else {
-		if (cb->newlog) {
+		if (policy_cb->newlog) {
 			char sign = (tz < 0) ? '-' : '+';
 			int zone = (tz < 0) ? (-tz) : tz;
-			fprintf(cb->newlog, "%s %s %s %lu %c%04d\t%s",
+			fprintf(policy_cb->newlog, "%s %s %s %lu %c%04d\t%s",
 				sha1_to_hex(osha1), sha1_to_hex(nsha1),
 				email, timestamp, sign, zone,
 				message);
-			hashcpy(cb->last_kept_sha1, nsha1);
+			hashcpy(policy_cb->last_kept_sha1, nsha1);
 		}
-		if (cb->cmd->verbose)
+		if (policy_cb->cmd->verbose)
 			printf("keep %s", message);
 	}
 	return 0;
@@ -421,12 +427,15 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 			 unsigned int flags, struct cmd_reflog_expire_cb *cmd)
 {
 	static struct lock_file reflog_lock;
-	struct expire_reflog_policy_cb cb;
+	struct expire_reflog_cb cb;
+	struct expire_reflog_policy_cb policy_cb;
 	struct ref_lock *lock;
 	char *log_file;
 	int status = 0;
 
 	memset(&cb, 0, sizeof(cb));
+	memset(&policy_cb, 0, sizeof(policy_cb));
+	cb.policy_cb = &policy_cb;
 
 	/*
 	 * The reflog file is locked by holding the lock on the
@@ -457,19 +466,19 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 			strbuf_release(&err);
 			goto failure;
 		}
-		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
-		if (!cb.newlog) {
+		policy_cb.newlog = fdopen_lock_file(&reflog_lock, "w");
+		if (!policy_cb.newlog) {
 			error("cannot fdopen %s (%s)",
 			      reflog_lock.filename.buf, strerror(errno));
 			goto failure;
 		}
 	}
 
-	cb.cmd = cmd;
+	policy_cb.cmd = cmd;
 
-	reflog_expiry_prepare(refname, sha1, &cb);
+	reflog_expiry_prepare(refname, sha1, &policy_cb);
 	for_each_reflog_ent(refname, expire_reflog_ent, &cb);
-	reflog_expiry_cleanup(&cb);
+	reflog_expiry_cleanup(&policy_cb);
 
 	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
 		if (close_lock_file(&reflog_lock)) {
@@ -477,7 +486,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 					strerror(errno));
 		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
 			(write_in_full(lock->lock_fd,
-				sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
+				sha1_to_hex(policy_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",
-- 
2.1.3

[PATCH v2 15/24] expire_reflog(): pass flags through to expire_reflog_ent()

[Michael Haggerty]

Add a flags field to "struct expire_reflog_cb", and pass the flags
argument through to expire_reflog_ent(). In a moment we will start
using it to pass through flags that expire_reflog_ent() needs.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 builtin/reflog.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 9ee66d4..08867a2 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -51,6 +51,7 @@ struct expire_reflog_policy_cb {
 };
 
 struct expire_reflog_cb {
+	unsigned int flags;
 	void *policy_cb;
 };
 
@@ -435,6 +436,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 
 	memset(&cb, 0, sizeof(cb));
 	memset(&policy_cb, 0, sizeof(policy_cb));
+	cb.flags = flags;
 	cb.policy_cb = &policy_cb;
 
 	/*
-- 
2.1.3

[PATCH v2 16/24] expire_reflog(): move verbose to flags argument

[Michael Haggerty]

The policy objects don't care about "--verbose". So move it to
expire_reflog()'s flags parameter.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 builtin/reflog.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 08867a2..15d3cd5 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -22,14 +22,14 @@ static unsigned long default_reflog_expire_unreachable;
 
 enum expire_reflog_flags {
 	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
-	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1
+	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
+	EXPIRE_REFLOGS_VERBOSE = 1 << 2
 };
 
 struct cmd_reflog_expire_cb {
 	struct rev_info revs;
 	int stalefix;
 	int rewrite;
-	int verbose;
 	unsigned long expire_total;
 	unsigned long expire_unreachable;
 	int recno;
@@ -344,7 +344,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 				     message, policy_cb)) {
 		if (!policy_cb->newlog)
 			printf("would prune %s", message);
-		else if (policy_cb->cmd->verbose)
+		else if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
 			printf("prune %s", message);
 	} else {
 		if (policy_cb->newlog) {
@@ -356,7 +356,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 				message);
 			hashcpy(policy_cb->last_kept_sha1, nsha1);
 		}
-		if (policy_cb->cmd->verbose)
+		if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
 			printf("keep %s", message);
 	}
 	return 0;
@@ -693,7 +693,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		else if (!strcmp(arg, "--all"))
 			do_all = 1;
 		else if (!strcmp(arg, "--verbose"))
-			cb.verbose = 1;
+			flags |= EXPIRE_REFLOGS_VERBOSE;
 		else if (!strcmp(arg, "--")) {
 			i++;
 			break;
@@ -711,10 +711,10 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	 */
 	if (cb.stalefix) {
 		init_revisions(&cb.revs, prefix);
-		if (cb.verbose)
+		if (flags & EXPIRE_REFLOGS_VERBOSE)
 			printf("Marking reachable objects...");
 		mark_reachable_objects(&cb.revs, 0, 0, NULL);
-		if (cb.verbose)
+		if (flags & EXPIRE_REFLOGS_VERBOSE)
 			putchar('\n');
 	}
 
@@ -773,7 +773,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 		else if (!strcmp(arg, "--updateref"))
 			flags |= EXPIRE_REFLOGS_UPDATE_REF;
 		else if (!strcmp(arg, "--verbose"))
-			cb.verbose = 1;
+			flags |= EXPIRE_REFLOGS_VERBOSE;
 		else if (!strcmp(arg, "--")) {
 			i++;
 			break;
-- 
2.1.3

[PATCH v2 17/24] expire_reflog(): move rewrite to flags argument

[Michael Haggerty]

The policy objects don't care about "--rewrite". So move it to
expire_reflog()'s flags parameter.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 builtin/reflog.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 15d3cd5..f86e7de 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -23,13 +23,13 @@ static unsigned long default_reflog_expire_unreachable;
 enum expire_reflog_flags {
 	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
 	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
-	EXPIRE_REFLOGS_VERBOSE = 1 << 2
+	EXPIRE_REFLOGS_VERBOSE = 1 << 2,
+	EXPIRE_REFLOGS_REWRITE = 1 << 3
 };
 
 struct cmd_reflog_expire_cb {
 	struct rev_info revs;
 	int stalefix;
-	int rewrite;
 	unsigned long expire_total;
 	unsigned long expire_unreachable;
 	int recno;
@@ -337,7 +337,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 	struct expire_reflog_cb *cb = cb_data;
 	struct expire_reflog_policy_cb *policy_cb = cb->policy_cb;
 
-	if (policy_cb->cmd->rewrite)
+	if (cb->flags & EXPIRE_REFLOGS_REWRITE)
 		osha1 = policy_cb->last_kept_sha1;
 
 	if (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,
@@ -687,7 +687,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		else if (!strcmp(arg, "--stale-fix"))
 			cb.stalefix = 1;
 		else if (!strcmp(arg, "--rewrite"))
-			cb.rewrite = 1;
+			flags |= EXPIRE_REFLOGS_REWRITE;
 		else if (!strcmp(arg, "--updateref"))
 			flags |= EXPIRE_REFLOGS_UPDATE_REF;
 		else if (!strcmp(arg, "--all"))
@@ -769,7 +769,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
 			flags |= EXPIRE_REFLOGS_DRY_RUN;
 		else if (!strcmp(arg, "--rewrite"))
-			cb.rewrite = 1;
+			flags |= EXPIRE_REFLOGS_REWRITE;
 		else if (!strcmp(arg, "--updateref"))
 			flags |= EXPIRE_REFLOGS_UPDATE_REF;
 		else if (!strcmp(arg, "--verbose"))
-- 
2.1.3

[PATCH v2 18/24] Move newlog and last_kept_sha1 to "struct expire_reflog_cb"

[Michael Haggerty]

These members are not needed by the policy functions.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 builtin/reflog.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index f86e7de..d36c2c6 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -36,7 +36,6 @@ struct cmd_reflog_expire_cb {
 };
 
 struct expire_reflog_policy_cb {
-	FILE *newlog;
 	enum {
 		UE_NORMAL,
 		UE_ALWAYS,
@@ -45,7 +44,6 @@ struct expire_reflog_policy_cb {
 	struct commit_list *mark_list;
 	unsigned long mark_limit;
 	struct cmd_reflog_expire_cb *cmd;
-	unsigned char last_kept_sha1[20];
 	struct commit *tip_commit;
 	struct commit_list *tips;
 };
@@ -53,6 +51,8 @@ struct expire_reflog_policy_cb {
 struct expire_reflog_cb {
 	unsigned int flags;
 	void *policy_cb;
+	FILE *newlog;
+	unsigned char last_kept_sha1[20];
 };
 
 struct collected_reflog {
@@ -338,23 +338,23 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 	struct expire_reflog_policy_cb *policy_cb = cb->policy_cb;
 
 	if (cb->flags & EXPIRE_REFLOGS_REWRITE)
-		osha1 = policy_cb->last_kept_sha1;
+		osha1 = cb->last_kept_sha1;
 
 	if (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,
 				     message, policy_cb)) {
-		if (!policy_cb->newlog)
+		if (!cb->newlog)
 			printf("would prune %s", message);
 		else if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
 			printf("prune %s", message);
 	} else {
-		if (policy_cb->newlog) {
+		if (cb->newlog) {
 			char sign = (tz < 0) ? '-' : '+';
 			int zone = (tz < 0) ? (-tz) : tz;
-			fprintf(policy_cb->newlog, "%s %s %s %lu %c%04d\t%s",
+			fprintf(cb->newlog, "%s %s %s %lu %c%04d\t%s",
 				sha1_to_hex(osha1), sha1_to_hex(nsha1),
 				email, timestamp, sign, zone,
 				message);
-			hashcpy(policy_cb->last_kept_sha1, nsha1);
+			hashcpy(cb->last_kept_sha1, nsha1);
 		}
 		if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
 			printf("keep %s", message);
@@ -468,8 +468,8 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 			strbuf_release(&err);
 			goto failure;
 		}
-		policy_cb.newlog = fdopen_lock_file(&reflog_lock, "w");
-		if (!policy_cb.newlog) {
+		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
+		if (!cb.newlog) {
 			error("cannot fdopen %s (%s)",
 			      reflog_lock.filename.buf, strerror(errno));
 			goto failure;
@@ -488,7 +488,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 					strerror(errno));
 		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
 			(write_in_full(lock->lock_fd,
-				sha1_to_hex(policy_cb.last_kept_sha1), 40) != 40 ||
+				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",
-- 
2.1.3

[PATCH v2 19/24] expire_reflog(): treat the policy callback data as opaque

[Michael Haggerty]

Now that expire_reflog() doesn't actually look in the
expire_reflog_policy_cb data structure, we can make it opaque:

* Change the callers of expire_reflog() to pass it a pointer to an
  entire "struct expire_reflog_policy_cb" rather than a pointer to a
  "struct cmd_reflog_expire_cb".

* Change expire_reflog() to accept the argument as a "void *" and
  simply pass it through to the policy functions.

* Change the policy functions, reflog_expiry_prepare(),
  reflog_expiry_cleanup(), and should_expire_reflog_ent(), to accept
  "void *cb_data" arguments and cast them back to "struct
  expire_reflog_policy_cb" internally.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 builtin/reflog.c | 72 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index d36c2c6..0df5721 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -43,7 +43,7 @@ struct expire_reflog_policy_cb {
 	} unreachable_expire_kind;
 	struct commit_list *mark_list;
 	unsigned long mark_limit;
-	struct cmd_reflog_expire_cb *cmd;
+	struct cmd_reflog_expire_cb cmd;
 	struct commit *tip_commit;
 	struct commit_list *tips;
 };
@@ -309,22 +309,22 @@ static int should_expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 	struct expire_reflog_policy_cb *cb = cb_data;
 	struct commit *old, *new;
 
-	if (timestamp < cb->cmd->expire_total)
+	if (timestamp < cb->cmd.expire_total)
 		return 1;
 
 	old = new = NULL;
-	if (cb->cmd->stalefix &&
+	if (cb->cmd.stalefix &&
 	    (!keep_entry(&old, osha1) || !keep_entry(&new, nsha1)))
 		return 1;
 
-	if (timestamp < cb->cmd->expire_unreachable) {
+	if (timestamp < cb->cmd.expire_unreachable) {
 		if (cb->unreachable_expire_kind == UE_ALWAYS)
 			return 1;
 		if (unreachable(cb, old, osha1) || unreachable(cb, new, nsha1))
 			return 1;
 	}
 
-	if (cb->cmd->recno && --(cb->cmd->recno) == 0)
+	if (cb->cmd.recno && --(cb->cmd.recno) == 0)
 		return 1;
 
 	return 0;
@@ -378,9 +378,11 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1,
 
 static void reflog_expiry_prepare(const char *refname,
 				  const unsigned char *sha1,
-				  struct expire_reflog_policy_cb *cb)
+				  void *cb_data)
 {
-	if (!cb->cmd->expire_unreachable || !strcmp(refname, "HEAD")) {
+	struct expire_reflog_policy_cb *cb = cb_data;
+
+	if (!cb->cmd.expire_unreachable || !strcmp(refname, "HEAD")) {
 		cb->tip_commit = NULL;
 		cb->unreachable_expire_kind = UE_HEAD;
 	} else {
@@ -391,7 +393,7 @@ static void reflog_expiry_prepare(const char *refname,
 			cb->unreachable_expire_kind = UE_NORMAL;
 	}
 
-	if (cb->cmd->expire_unreachable <= cb->cmd->expire_total)
+	if (cb->cmd.expire_unreachable <= cb->cmd.expire_total)
 		cb->unreachable_expire_kind = UE_ALWAYS;
 
 	cb->mark_list = NULL;
@@ -405,13 +407,15 @@ static void reflog_expiry_prepare(const char *refname,
 		} else {
 			commit_list_insert(cb->tip_commit, &cb->mark_list);
 		}
-		cb->mark_limit = cb->cmd->expire_total;
+		cb->mark_limit = cb->cmd.expire_total;
 		mark_reachable(cb);
 	}
 }
 
-static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)
+static void reflog_expiry_cleanup(void *cb_data)
 {
+	struct expire_reflog_policy_cb *cb = cb_data;
+
 	if (cb->unreachable_expire_kind != UE_ALWAYS) {
 		if (cb->unreachable_expire_kind == UE_HEAD) {
 			struct commit_list *elem;
@@ -425,19 +429,17 @@ static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)
 }
 
 static int expire_reflog(const char *refname, const unsigned char *sha1,
-			 unsigned int flags, struct cmd_reflog_expire_cb *cmd)
+			 unsigned int flags, void *policy_cb_data)
 {
 	static struct lock_file reflog_lock;
 	struct expire_reflog_cb cb;
-	struct expire_reflog_policy_cb policy_cb;
 	struct ref_lock *lock;
 	char *log_file;
 	int status = 0;
 
 	memset(&cb, 0, sizeof(cb));
-	memset(&policy_cb, 0, sizeof(policy_cb));
 	cb.flags = flags;
-	cb.policy_cb = &policy_cb;
+	cb.policy_cb = policy_cb_data;
 
 	/*
 	 * The reflog file is locked by holding the lock on the
@@ -476,11 +478,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 		}
 	}
 
-	policy_cb.cmd = cmd;
-
-	reflog_expiry_prepare(refname, sha1, &policy_cb);
+	reflog_expiry_prepare(refname, sha1, cb.policy_cb);
 	for_each_reflog_ent(refname, expire_reflog_ent, &cb);
-	reflog_expiry_cleanup(&policy_cb);
+	reflog_expiry_cleanup(cb.policy_cb);
 
 	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
 		if (close_lock_file(&reflog_lock)) {
@@ -653,7 +653,7 @@ static void set_reflog_expiry_param(struct cmd_reflog_expire_cb *cb, int slot, c
 
 static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 {
-	struct cmd_reflog_expire_cb cb;
+	struct expire_reflog_policy_cb cb;
 	unsigned long now = time(NULL);
 	int i, status, do_all;
 	int explicit_expiry = 0;
@@ -667,25 +667,25 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	do_all = status = 0;
 	memset(&cb, 0, sizeof(cb));
 
-	cb.expire_total = default_reflog_expire;
-	cb.expire_unreachable = default_reflog_expire_unreachable;
+	cb.cmd.expire_total = default_reflog_expire;
+	cb.cmd.expire_unreachable = default_reflog_expire_unreachable;
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
 			flags |= EXPIRE_REFLOGS_DRY_RUN;
 		else if (starts_with(arg, "--expire=")) {
-			if (parse_expiry_date(arg + 9, &cb.expire_total))
+			if (parse_expiry_date(arg + 9, &cb.cmd.expire_total))
 				die(_("'%s' is not a valid timestamp"), arg);
 			explicit_expiry |= EXPIRE_TOTAL;
 		}
 		else if (starts_with(arg, "--expire-unreachable=")) {
-			if (parse_expiry_date(arg + 21, &cb.expire_unreachable))
+			if (parse_expiry_date(arg + 21, &cb.cmd.expire_unreachable))
 				die(_("'%s' is not a valid timestamp"), arg);
 			explicit_expiry |= EXPIRE_UNREACH;
 		}
 		else if (!strcmp(arg, "--stale-fix"))
-			cb.stalefix = 1;
+			cb.cmd.stalefix = 1;
 		else if (!strcmp(arg, "--rewrite"))
 			flags |= EXPIRE_REFLOGS_REWRITE;
 		else if (!strcmp(arg, "--updateref"))
@@ -709,11 +709,11 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	 * even in older repository.  We cannot trust what's reachable
 	 * from reflog if the repository was pruned with older git.
 	 */
-	if (cb.stalefix) {
-		init_revisions(&cb.revs, prefix);
+	if (cb.cmd.stalefix) {
+		init_revisions(&cb.cmd.revs, prefix);
 		if (flags & EXPIRE_REFLOGS_VERBOSE)
 			printf("Marking reachable objects...");
-		mark_reachable_objects(&cb.revs, 0, 0, NULL);
+		mark_reachable_objects(&cb.cmd.revs, 0, 0, NULL);
 		if (flags & EXPIRE_REFLOGS_VERBOSE)
 			putchar('\n');
 	}
@@ -726,7 +726,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		for_each_reflog(collect_reflog, &collected);
 		for (i = 0; i < collected.nr; i++) {
 			struct collected_reflog *e = collected.e[i];
-			set_reflog_expiry_param(&cb, explicit_expiry, e->reflog);
+			set_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);
 			status |= expire_reflog(e->reflog, e->sha1, flags, &cb);
 			free(e);
 		}
@@ -740,7 +740,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 			status |= error("%s points nowhere!", argv[i]);
 			continue;
 		}
-		set_reflog_expiry_param(&cb, explicit_expiry, ref);
+		set_reflog_expiry_param(&cb.cmd, explicit_expiry, ref);
 		status |= expire_reflog(ref, sha1, flags, &cb);
 	}
 	return status;
@@ -750,15 +750,15 @@ static int count_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 		const char *email, unsigned long timestamp, int tz,
 		const char *message, void *cb_data)
 {
-	struct cmd_reflog_expire_cb *cb = cb_data;
-	if (!cb->expire_total || timestamp < cb->expire_total)
-		cb->recno++;
+	struct expire_reflog_policy_cb *cb = cb_data;
+	if (!cb->cmd.expire_total || timestamp < cb->cmd.expire_total)
+		cb->cmd.recno++;
 	return 0;
 }
 
 static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 {
-	struct cmd_reflog_expire_cb cb;
+	struct expire_reflog_policy_cb cb;
 	int i, status = 0;
 	unsigned int flags = 0;
 
@@ -805,12 +805,12 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 
 		recno = strtoul(spec + 2, &ep, 10);
 		if (*ep == '}') {
-			cb.recno = -recno;
+			cb.cmd.recno = -recno;
 			for_each_reflog_ent(ref, count_reflog_ent, &cb);
 		} else {
-			cb.expire_total = approxidate(spec + 2);
+			cb.cmd.expire_total = approxidate(spec + 2);
 			for_each_reflog_ent(ref, count_reflog_ent, &cb);
-			cb.expire_total = 0;
+			cb.cmd.expire_total = 0;
 		}
 
 		status |= expire_reflog(ref, sha1, flags, &cb);
-- 
2.1.3

[PATCH v2 20/24] reflog_expire(): new function in the reference API

[Michael Haggerty]

Move expire_reflog() into refs.c and rename it to reflog_expire().
Turn the three policy functions into function pointers that are passed
into reflog_expire(). Add function prototypes and documentation to
refs.h.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 builtin/reflog.c | 148 ++++++-------------------------------------------------
 refs.c           | 129 ++++++++++++++++++++++++++++++++++++++++++++++++
 refs.h           |  46 +++++++++++++++++
 3 files changed, 190 insertions(+), 133 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 0df5721..49c64f9 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -20,13 +20,6 @@ static const char reflog_delete_usage[] =
 static unsigned long default_reflog_expire;
 static unsigned long default_reflog_expire_unreachable;
 
-enum expire_reflog_flags {
-	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
-	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
-	EXPIRE_REFLOGS_VERBOSE = 1 << 2,
-	EXPIRE_REFLOGS_REWRITE = 1 << 3
-};
-
 struct cmd_reflog_expire_cb {
 	struct rev_info revs;
 	int stalefix;
@@ -48,13 +41,6 @@ struct expire_reflog_policy_cb {
 	struct commit_list *tips;
 };
 
-struct expire_reflog_cb {
-	unsigned int flags;
-	void *policy_cb;
-	FILE *newlog;
-	unsigned char last_kept_sha1[20];
-};
-
 struct collected_reflog {
 	unsigned char sha1[20];
 	char reflog[FLEX_ARRAY];
@@ -330,38 +316,6 @@ static int should_expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 	return 0;
 }
 
-static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
-		const char *email, unsigned long timestamp, int tz,
-		const char *message, void *cb_data)
-{
-	struct expire_reflog_cb *cb = cb_data;
-	struct expire_reflog_policy_cb *policy_cb = cb->policy_cb;
-
-	if (cb->flags & EXPIRE_REFLOGS_REWRITE)
-		osha1 = cb->last_kept_sha1;
-
-	if (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,
-				     message, policy_cb)) {
-		if (!cb->newlog)
-			printf("would prune %s", message);
-		else if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
-			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",
-				sha1_to_hex(osha1), sha1_to_hex(nsha1),
-				email, timestamp, sign, zone,
-				message);
-			hashcpy(cb->last_kept_sha1, nsha1);
-		}
-		if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
-			printf("keep %s", message);
-	}
-	return 0;
-}
-
 static int push_tip_to_list(const char *refname, const unsigned char *sha1,
 			    int flags, void *cb_data)
 {
@@ -428,90 +382,6 @@ static void reflog_expiry_cleanup(void *cb_data)
 	}
 }
 
-static int expire_reflog(const char *refname, const unsigned char *sha1,
-			 unsigned int flags, void *policy_cb_data)
-{
-	static struct lock_file reflog_lock;
-	struct expire_reflog_cb cb;
-	struct ref_lock *lock;
-	char *log_file;
-	int status = 0;
-
-	memset(&cb, 0, sizeof(cb));
-	cb.flags = flags;
-	cb.policy_cb = policy_cb_data;
-
-	/*
-	 * The reflog file is locked by holding the lock on the
-	 * reference itself, plus we might need to update the
-	 * reference if --updateref was specified:
-	 */
-	lock = lock_any_ref_for_update(refname, sha1, 0, NULL);
-	if (!lock)
-		return error("cannot lock ref '%s'", refname);
-	if (!reflog_exists(refname)) {
-		unlock_ref(lock);
-		return 0;
-	}
-
-	log_file = git_pathdup("logs/%s", refname);
-	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
-		/*
-		 * Even though holding $GIT_DIR/logs/$reflog.lock has
-		 * no locking implications, we use the lock_file
-		 * machinery here anyway because it does a lot of the
-		 * work we need, including cleaning up if the program
-		 * exits unexpectedly.
-		 */
-		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0) {
-			struct strbuf err = STRBUF_INIT;
-			unable_to_lock_message(log_file, errno, &err);
-			error("%s", err.buf);
-			strbuf_release(&err);
-			goto failure;
-		}
-		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
-		if (!cb.newlog) {
-			error("cannot fdopen %s (%s)",
-			      reflog_lock.filename.buf, strerror(errno));
-			goto failure;
-		}
-	}
-
-	reflog_expiry_prepare(refname, sha1, cb.policy_cb);
-	for_each_reflog_ent(refname, expire_reflog_ent, &cb);
-	reflog_expiry_cleanup(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("unable to commit reflog '%s' (%s)",
-					log_file, strerror(errno));
-		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {
-			status |= error("couldn't set %s", lock->ref_name);
-		}
-	}
-	free(log_file);
-	unlock_ref(lock);
-	return status;
-
- failure:
-	rollback_lock_file(&reflog_lock);
-	free(log_file);
-	unlock_ref(lock);
-	return -1;
-}
-
 static int collect_reflog(const char *ref, const unsigned char *sha1, int unused, void *cb_data)
 {
 	struct collected_reflog *e;
@@ -727,7 +597,11 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		for (i = 0; i < collected.nr; i++) {
 			struct collected_reflog *e = collected.e[i];
 			set_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);
-			status |= expire_reflog(e->reflog, e->sha1, flags, &cb);
+			status |= reflog_expire(e->reflog, e->sha1, flags,
+						reflog_expiry_prepare,
+						should_expire_reflog_ent,
+						reflog_expiry_cleanup,
+						&cb);
 			free(e);
 		}
 		free(collected.e);
@@ -741,7 +615,11 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 		set_reflog_expiry_param(&cb.cmd, explicit_expiry, ref);
-		status |= expire_reflog(ref, sha1, flags, &cb);
+		status |= reflog_expire(ref, sha1, flags,
+					reflog_expiry_prepare,
+					should_expire_reflog_ent,
+					reflog_expiry_cleanup,
+					&cb);
 	}
 	return status;
 }
@@ -813,7 +691,11 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 			cb.cmd.expire_total = 0;
 		}
 
-		status |= expire_reflog(ref, sha1, flags, &cb);
+		status |= reflog_expire(ref, sha1, flags,
+					reflog_expiry_prepare,
+					should_expire_reflog_ent,
+					reflog_expiry_cleanup,
+					&cb);
 		free(ref);
 	}
 	return status;
diff --git a/refs.c b/refs.c
index 150c980..0252fcc 100644
--- a/refs.c
+++ b/refs.c
@@ -3943,3 +3943,132 @@ int ref_is_hidden(const char *refname)
 	}
 	return 0;
 }
+
+struct expire_reflog_cb {
+	unsigned int flags;
+	reflog_expiry_should_prune_fn *should_prune_fn;
+	void *policy_cb;
+	FILE *newlog;
+	unsigned char last_kept_sha1[20];
+};
+
+static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+			     const char *email, unsigned long timestamp, int tz,
+			     const char *message, void *cb_data)
+{
+	struct expire_reflog_cb *cb = cb_data;
+	struct expire_reflog_policy_cb *policy_cb = cb->policy_cb;
+
+	if (cb->flags & EXPIRE_REFLOGS_REWRITE)
+		osha1 = cb->last_kept_sha1;
+
+	if ((*cb->should_prune_fn)(osha1, nsha1, email, timestamp, tz,
+				   message, policy_cb)) {
+		if (!cb->newlog)
+			printf("would prune %s", message);
+		else if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
+			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",
+				sha1_to_hex(osha1), sha1_to_hex(nsha1),
+				email, timestamp, sign, zone,
+				message);
+			hashcpy(cb->last_kept_sha1, nsha1);
+		}
+		if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
+			printf("keep %s", message);
+	}
+	return 0;
+}
+
+extern int reflog_expire(const char *refname, const unsigned char *sha1,
+			 unsigned int flags,
+			 reflog_expiry_prepare_fn prepare_fn,
+			 reflog_expiry_should_prune_fn should_prune_fn,
+			 reflog_expiry_cleanup_fn cleanup_fn,
+			 void *policy_cb_data)
+{
+	static struct lock_file reflog_lock;
+	struct expire_reflog_cb cb;
+	struct ref_lock *lock;
+	char *log_file;
+	int status = 0;
+
+	memset(&cb, 0, sizeof(cb));
+	cb.flags = flags;
+	cb.policy_cb = policy_cb_data;
+	cb.should_prune_fn = should_prune_fn;
+
+	/*
+	 * The reflog file is locked by holding the lock on the
+	 * reference itself, plus we might need to update the
+	 * reference if --updateref was specified:
+	 */
+	lock = lock_any_ref_for_update(refname, sha1, 0, NULL);
+	if (!lock)
+		return error("cannot lock ref '%s'", refname);
+	if (!reflog_exists(refname)) {
+		unlock_ref(lock);
+		return 0;
+	}
+
+	log_file = git_pathdup("logs/%s", refname);
+	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
+		/*
+		 * Even though holding $GIT_DIR/logs/$reflog.lock has
+		 * no locking implications, we use the lock_file
+		 * machinery here anyway because it does a lot of the
+		 * work we need, including cleaning up if the program
+		 * exits unexpectedly.
+		 */
+		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0) {
+			struct strbuf err = STRBUF_INIT;
+			unable_to_lock_message(log_file, errno, &err);
+			error("%s", err.buf);
+			strbuf_release(&err);
+			goto failure;
+		}
+		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
+		if (!cb.newlog) {
+			error("cannot fdopen %s (%s)",
+			      reflog_lock.filename.buf, strerror(errno));
+			goto failure;
+		}
+	}
+
+	(*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("unable to commit reflog '%s' (%s)",
+					log_file, strerror(errno));
+		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {
+			status |= error("couldn't set %s", lock->ref_name);
+		}
+	}
+	free(log_file);
+	unlock_ref(lock);
+	return status;
+
+ failure:
+	rollback_lock_file(&reflog_lock);
+	free(log_file);
+	unlock_ref(lock);
+	return -1;
+}
diff --git a/refs.h b/refs.h
index 7d675b7..99e707b 100644
--- a/refs.h
+++ b/refs.h
@@ -353,4 +353,50 @@ int update_ref(const char *action, const char *refname,
 extern int parse_hide_refs_config(const char *var, const char *value, const char *);
 extern int ref_is_hidden(const char *);
 
+enum expire_reflog_flags {
+	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
+	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
+	EXPIRE_REFLOGS_VERBOSE = 1 << 2,
+	EXPIRE_REFLOGS_REWRITE = 1 << 3
+};
+
+/*
+ * The following interface is used for reflog expiration. The caller
+ * calls reflog_expire(), supplying it with three callback functions,
+ * of the following types. The callback functions define the
+ * expiration policy that is desired.
+ *
+ * reflog_expiry_prepare_fn -- Called once after the reference is
+ *     locked.
+ *
+ * reflog_expiry_should_prune_fn -- Called once for each entry in the
+ *     existing reflog. It should return true iff that entry should be
+ *     pruned.
+ *
+ * reflog_expiry_cleanup_fn -- Called once before the reference is
+ *     unlocked again.
+ */
+typedef void reflog_expiry_prepare_fn(const char *refname,
+				      const unsigned char *sha1,
+				      void *cb_data);
+typedef int reflog_expiry_should_prune_fn(unsigned char *osha1,
+					  unsigned char *nsha1,
+					  const char *email,
+					  unsigned long timestamp, int tz,
+					  const char *message, void *cb_data);
+typedef void reflog_expiry_cleanup_fn(void *cb_data);
+
+/*
+ * Expire reflog entries for the specified reference. sha1 is the old
+ * value of the reference. flags is a combination of the constants in
+ * enum expire_reflog_flags. The three function pointers are described
+ * above. On success, return zero.
+ */
+extern int reflog_expire(const char *refname, const unsigned char *sha1,
+			 unsigned int flags,
+			 reflog_expiry_prepare_fn prepare_fn,
+			 reflog_expiry_should_prune_fn should_prune_fn,
+			 reflog_expiry_cleanup_fn cleanup_fn,
+			 void *policy_cb_data);
+
 #endif /* REFS_H */
-- 
2.1.3

[PATCH v2 21/24] refs.c: remove unlock_ref/close_ref/commit_ref from the refs api

[Michael Haggerty]

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>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 24 ++++++++++++------------
 refs.h |  9 ---------
 2 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/refs.c b/refs.c
index 0252fcc..618ef9c 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 99e707b..4bb58b9 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);
-
 /*
  * Setup reflog before using. Set errno to something meaningful on failure.
  */
-- 
2.1.3

[PATCH v2 22/24] lock_any_ref_for_update(): inline function

[Michael Haggerty]

Inline the function at its one remaining caller (which is within
refs.c) and remove it.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 9 +--------
 refs.h | 9 +--------
 2 files changed, 2 insertions(+), 16 deletions(-)

diff --git a/refs.c b/refs.c
index 618ef9c..166c0f6 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.
@@ -4007,7 +4000,7 @@ extern int reflog_expire(const char *refname, const unsigned char *sha1,
 	 * reference itself, plus we might need to update the
 	 * reference if --updateref was specified:
 	 */
-	lock = lock_any_ref_for_update(refname, sha1, 0, NULL);
+	lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, NULL);
 	if (!lock)
 		return error("cannot lock ref '%s'", refname);
 	if (!reflog_exists(refname)) {
diff --git a/refs.h b/refs.h
index 4bb58b9..28e7834 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(), ref_transaction_update(),
- * ref_transaction_create(), etc.
+ * Flags controlling ref_transaction_update(), ref_transaction_create(), 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);
 
 /*
  * Setup reflog before using. Set errno to something meaningful on failure.
-- 
2.1.3

[PATCH v2 23/24] refs.c: don't expose the internal struct ref_lock in the header file

[Michael Haggerty]

From: Stefan Beller <sbeller@google.com>

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>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 9 +++++++++
 refs.h | 9 ---------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index 166c0f6..582a051 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 28e7834..fc88ba6 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 ref_transaction represents a collection of ref updates
  * that should succeed or fail together.
-- 
2.1.3

[PATCH v2 24/24] refs.c: let fprintf handle the formatting

[Michael Haggerty]

From: Stefan Beller <sbeller@google.com>

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>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index 582a051..9e6d21c 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.1.3

Re: [PATCH v2 04/24] expire_reflog(): it's not an each_ref_fn anymore

[Stefan Beller]

On Fri, Dec 12, 2014 at 12:56 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Prior to v1.5.4~14, expire_reflog() had to be an each_ref_fn because
> it was passed to for_each_reflog(). Since then, there has been no
> reason for it to implement the each_ref_fn interface. So...
>
> * Remove the "unused" parameter (which took the place of "flags", but
>   was really unused).
>
> * Declare the last parameter to be (struct cmd_reflog_expire_cb *)
>   rather than (void *).
>
> Helped-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

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

> ---
>  builtin/reflog.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 2d85d26..160541a 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -349,9 +349,9 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
>         return 0;
>  }
>
> -static int expire_reflog(const char *ref, const unsigned char *sha1, int unused, void *cb_data)
> +static int expire_reflog(const char *ref, const unsigned char *sha1,
> +                        struct cmd_reflog_expire_cb *cmd)
>  {
> -       struct cmd_reflog_expire_cb *cmd = cb_data;
>         struct expire_reflog_cb cb;
>         struct ref_lock *lock;
>         char *log_file, *newlog_path = NULL;
> @@ -663,7 +663,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>                 for (i = 0; i < collected.nr; i++) {
>                         struct collected_reflog *e = collected.e[i];
>                         set_reflog_expiry_param(&cb, explicit_expiry, e->reflog);
> -                       status |= expire_reflog(e->reflog, e->sha1, 0, &cb);
> +                       status |= expire_reflog(e->reflog, e->sha1, &cb);
>                         free(e);
>                 }
>                 free(collected.e);
> @@ -677,7 +677,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>                         continue;
>                 }
>                 set_reflog_expiry_param(&cb, explicit_expiry, ref);
> -               status |= expire_reflog(ref, sha1, 0, &cb);
> +               status |= expire_reflog(ref, sha1, &cb);
>         }
>         return status;
>  }
> @@ -748,7 +748,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
>                         cb.expire_total = 0;
>                 }
>
> -               status |= expire_reflog(ref, sha1, 0, &cb);
> +               status |= expire_reflog(ref, sha1, &cb);
>                 free(ref);
>         }
>         return status;
> --
> 2.1.3
>

Re: [PATCH v2 06/24] expire_reflog(): return early if the reference has no reflog

[Stefan Beller]

On Fri, Dec 12, 2014 at 12:56 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> There is very little cleanup needed if the reference has no reflog. If
> we move the initialization of log_file down a bit, there's even less.
> So instead of jumping to the cleanup code at the end of the function,
> just do the cleanup and return inline.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

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

> ---
>  builtin/reflog.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index ff51dbf..37b33c9 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -368,9 +368,11 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>         lock = lock_any_ref_for_update(refname, sha1, 0, NULL);
>         if (!lock)
>                 return error("cannot lock ref '%s'", refname);
> +       if (!reflog_exists(refname)) {
> +               unlock_ref(lock);
> +               return 0;
> +       }
>         log_file = git_pathdup("logs/%s", refname);
> -       if (!reflog_exists(refname))
> -               goto finish;
>         if (!cmd->dry_run) {
>                 newlog_path = git_pathdup("logs/%s.lock", refname);
>                 cb.newlog = fopen(newlog_path, "w");
> @@ -419,7 +421,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>                         clear_commit_marks(tip_commit, REACHABLE);
>                 }
>         }
> - finish:
> +
>         if (cb.newlog) {
>                 if (fclose(cb.newlog)) {
>                         status |= error("%s: %s", strerror(errno),
> --
> 2.1.3
>

Re: [PATCH v2 07/24] expire_reflog(): use a lock_file for rewriting the reflog file

[Stefan Beller]

On Fri, Dec 12, 2014 at 12:56 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> We don't actually need the locking functionality, because we already
> hold the lock on the reference itself, which is how the reflog file is
> locked. But the lock_file code can do some of the bookkeeping for us,
> and it is more careful than the old code here was. For example:
>
> * It correctly handles the case that the reflog lock file already
>   exists for some reason or cannot be opened.
>
> * It correctly cleans up the lockfile if the program dies.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  builtin/reflog.c | 60 ++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 41 insertions(+), 19 deletions(-)
>
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 37b33c9..ba5b3d3 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -352,9 +352,10 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
>  static int expire_reflog(const char *refname, const unsigned char *sha1,
>                          struct cmd_reflog_expire_cb *cmd)
>  {
> +       static struct lock_file reflog_lock;
>         struct expire_reflog_cb cb;
>         struct ref_lock *lock;
> -       char *log_file, *newlog_path = NULL;
> +       char *log_file;
>         struct commit *tip_commit;
>         struct commit_list *tips;
>         int status = 0;
> @@ -362,8 +363,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>         memset(&cb, 0, sizeof(cb));
>
>         /*
> -        * we take the lock for the ref itself to prevent it from
> -        * getting updated.
> +        * The reflog file is locked by holding the lock on the
> +        * reference itself, plus we might need to update the
> +        * reference if --updateref was specified:
>          */
>         lock = lock_any_ref_for_update(refname, sha1, 0, NULL);
>         if (!lock)
> @@ -372,10 +374,29 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>                 unlock_ref(lock);
>                 return 0;
>         }
> +
>         log_file = git_pathdup("logs/%s", refname);
>         if (!cmd->dry_run) {
> -               newlog_path = git_pathdup("logs/%s.lock", refname);
> -               cb.newlog = fopen(newlog_path, "w");
> +               /*
> +                * Even though holding $GIT_DIR/logs/$reflog.lock has
> +                * no locking implications, we use the lock_file
> +                * machinery here anyway because it does a lot of the
> +                * work we need, including cleaning up if the program
> +                * exits unexpectedly.
> +                */
> +               if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0) {
> +                       struct strbuf err = STRBUF_INIT;
> +                       unable_to_lock_message(log_file, errno, &err);
> +                       error("%s", err.buf);
> +                       strbuf_release(&err);
> +                       goto failure;
> +               }
> +               cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> +               if (!cb.newlog) {
> +                       error("cannot fdopen %s (%s)",
> +                             reflog_lock.filename.buf, strerror(errno));
> +                       goto failure;
> +               }
>         }
>
>         cb.cmd = cmd;
> @@ -423,32 +444,33 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>         }
>
>         if (cb.newlog) {
> -               if (fclose(cb.newlog)) {
> -                       status |= error("%s: %s", strerror(errno),
> -                                       newlog_path);
> -                       unlink(newlog_path);
> +               if (close_lock_file(&reflog_lock)) {
> +                       status |= error("couldn't write %s: %s", log_file,
> +                                       strerror(errno));
>                 } else if (cmd->updateref &&
>                         (write_in_full(lock->lock_fd,
>                                 sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||

optional nit:
While being here, you may fix the indentation of the sha1_to_hex to align to the
opening parentheses of write_in_full the line before.

>                          write_str_in_full(lock->lock_fd, "\n") != 1 ||
>                          close_ref(lock) < 0)) {
> -                       status |= error("Couldn't write %s",
> +                       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);
> +                       rollback_lock_file(&reflog_lock);
> +               } else if (commit_lock_file(&reflog_lock)) {
> +                       status |= error("unable to commit reflog '%s' (%s)",
> +                                       log_file, strerror(errno));
>                 } else if (cmd->updateref && commit_ref(lock)) {
> -                       status |= error("Couldn't set %s", lock->ref_name);
> -               } else {
> -                       adjust_shared_perm(log_file);

So we don't need adjust_shared_perm anymore?

I have been looking at this excerpt for a while now and I am confused by the
first and third statement in the origin/master state as well as after
your changes.
So it's essentially:
  * write out the reflog lock file
  * update ref in the lock file
  * rename the reflog lock into place
  * rename the ref lock into place
  * (in origin/master) if everything went fine, call adjust_shared_perm.
  * In your patch we actually don't need it as on locking the lock
file there is a call
    to  adjust_shared_perm already.

So I think it's clear to me now and looks good. It took me sometime to
understand
it. Maybe that's because I just got up or it's in fact hard to
understand. Would comments
help in laying out the sequence of actions beforehand?

I think the most confusing thing was the close_lock_file as a first
statement in the if chain.
close_lock_file is primarily used here to have the file written out to
disk, but the function
name suggests that the primary functionality is the closing of the fd.


> +                       status |= error("couldn't set %s", lock->ref_name);
>                 }
>         }
> -       free(newlog_path);
>         free(log_file);
>         unlock_ref(lock);
>         return status;
> +
> + failure:
> +       rollback_lock_file(&reflog_lock);
> +       free(log_file);
> +       unlock_ref(lock);
> +       return -1;
>  }
>
>  static int collect_reflog(const char *ref, const unsigned char *sha1, int unused, void *cb_data)
> --
> 2.1.3
>

Re: [PATCH v2 08/24] Extract function should_expire_reflog_ent()

[Stefan Beller]

On Fri, Dec 12, 2014 at 12:56 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Extract from expire_reflog_ent() a function that is solely responsible
> for deciding whether a reflog entry should be expired. By separating
> this "business logic" from the mechanics of actually expiring entries,
> we are working towards the goal of encapsulating reflog expiry within
> the refs API, with policy decided by a callback function passed to it
> by its caller.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

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

> ---
>  builtin/reflog.c | 70 +++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 42 insertions(+), 28 deletions(-)
>
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index ba5b3d3..06ce8b1 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -288,51 +288,65 @@ static int unreachable(struct expire_reflog_cb *cb, struct commit *commit, unsig
>         return !(commit->object.flags & REACHABLE);
>  }
>
> -static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
> -               const char *email, unsigned long timestamp, int tz,
> -               const char *message, void *cb_data)
> +/*
> + * Return true iff the specified reflog entry should be expired.
> + */
> +static int should_expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
> +                                   const char *email, unsigned long timestamp, int tz,
> +                                   const char *message, void *cb_data)
>  {
>         struct expire_reflog_cb *cb = cb_data;
>         struct commit *old, *new;
>
>         if (timestamp < cb->cmd->expire_total)
> -               goto prune;
> -
> -       if (cb->cmd->rewrite)
> -               osha1 = cb->last_kept_sha1;
> +               return 1;
>
>         old = new = NULL;
>         if (cb->cmd->stalefix &&
>             (!keep_entry(&old, osha1) || !keep_entry(&new, nsha1)))
> -               goto prune;
> +               return 1;
>
>         if (timestamp < cb->cmd->expire_unreachable) {
>                 if (cb->unreachable_expire_kind == UE_ALWAYS)
> -                       goto prune;
> +                       return 1;
>                 if (unreachable(cb, old, osha1) || unreachable(cb, new, nsha1))
> -                       goto prune;
> +                       return 1;
>         }
>
>         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);
> -               hashcpy(cb->last_kept_sha1, nsha1);
> -       }
> -       if (cb->cmd->verbose)
> -               printf("keep %s", message);
> +               return 1;
> +
>         return 0;
> - prune:
> -       if (!cb->newlog)
> -               printf("would prune %s", message);
> -       else if (cb->cmd->verbose)
> -               printf("prune %s", message);
> +}
> +
> +static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
> +               const char *email, unsigned long timestamp, int tz,
> +               const char *message, void *cb_data)
> +{
> +       struct expire_reflog_cb *cb = cb_data;
> +
> +       if (cb->cmd->rewrite)
> +               osha1 = cb->last_kept_sha1;
> +
> +       if (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,
> +                                    message, cb_data)) {
> +               if (!cb->newlog)
> +                       printf("would prune %s", message);
> +               else if (cb->cmd->verbose)
> +                       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",
> +                               sha1_to_hex(osha1), sha1_to_hex(nsha1),
> +                               email, timestamp, sign, zone,
> +                               message);
> +                       hashcpy(cb->last_kept_sha1, nsha1);
> +               }
> +               if (cb->cmd->verbose)
> +                       printf("keep %s", message);
> +       }
>         return 0;
>  }
>
> --
> 2.1.3
>

Re: [PATCH v2 22/24] lock_any_ref_for_update(): inline function

[Stefan Beller]

On Fri, Dec 12, 2014 at 12:57 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Inline the function at its one remaining caller (which is within
> refs.c) and remove it.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

Reviewed-by: Stefan Beller <sbeller@google.com>
> ---
>  refs.c | 9 +--------
>  refs.h | 9 +--------
>  2 files changed, 2 insertions(+), 16 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 618ef9c..166c0f6 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.
> @@ -4007,7 +4000,7 @@ extern int reflog_expire(const char *refname, const unsigned char *sha1,
>          * reference itself, plus we might need to update the
>          * reference if --updateref was specified:
>          */
> -       lock = lock_any_ref_for_update(refname, sha1, 0, NULL);
> +       lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, NULL);
>         if (!lock)
>                 return error("cannot lock ref '%s'", refname);
>         if (!reflog_exists(refname)) {
> diff --git a/refs.h b/refs.h
> index 4bb58b9..28e7834 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(), ref_transaction_update(),
> - * ref_transaction_create(), etc.
> + * Flags controlling ref_transaction_update(), ref_transaction_create(), 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);
>
>  /*
>   * Setup reflog before using. Set errno to something meaningful on failure.
> --
> 2.1.3
>

Re: [PATCH v2 00/24] Add reflog_expire() to the references API

[Junio C Hamano]

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

> This is v2 of the series. Thanks to Jonathan, Stefan, Junio, and
> Ronnie for their feedback on v1 [1]. I think I have addressed all of
> the issues that were raised.
>
> Changes since v1:
>
> * Several improvements to commit messages and comments; added some
>   Reviewed-by comments from the mailing list.
>
> * Change the signature of expire_reflog() early in the series to cast
>   off its heritage as an each_ref_fn.
>
> * Move the static lock_file object into expire_reflog(), and explain
>   the locking policy better.
>
> * Report errors if hold_lock_file_for_update() or fdopen_lock_file()
>   fails.
>
> * Fix the capitalization in some error messages.
>
> * When "enum expire_reflog_flags" is first introduced, put its
>   definition earlier in the file so that a later patch in the series
>   doesn't have to move it.
>
> * Rename reflog_expiry_select_fn to reflog_expiry_should_prune_fn.
>
> * Append Stefan's patch 24/24 "refs.c: let fprintf handle the
>   formatting"

All looked reasonable (three patches in the series somehow stood out
in a funny way in shortlog output, though).

Will queue.  I think a few topics from Stefan I have in 'pu' can
safely be dropped as the early part of this series has the same.

Thanks.


>
> This branch is also available on GitHub:
>
>     https://github.com/mhagger/git.git, branch reflog-expire-api-v2
>
> Michael
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/260812
>
> Michael Haggerty (18):
>   expire_reflog(): it's not an each_ref_fn anymore
>   expire_reflog(): rename "ref" parameter to "refname"
>   expire_reflog(): return early if the reference has no reflog
>   expire_reflog(): use a lock_file for rewriting the reflog file
>   Extract function should_expire_reflog_ent()
>   expire_reflog(): extract two policy-related functions
>   expire_reflog(): add a "flags" argument
>   expire_reflog(): move dry_run to flags argument
>   expire_reflog(): move updateref to flags argument
>   Rename expire_reflog_cb to expire_reflog_policy_cb
>   struct expire_reflog_cb: a new callback data type
>   expire_reflog(): pass flags through to expire_reflog_ent()
>   expire_reflog(): move verbose to flags argument
>   expire_reflog(): move rewrite to flags argument
>   Move newlog and last_kept_sha1 to "struct expire_reflog_cb"
>   expire_reflog(): treat the policy callback data as opaque
>   reflog_expire(): new function in the reference API
>   lock_any_ref_for_update(): inline function
>
> Ronnie Sahlberg (4):
>   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: remove unlock_ref/close_ref/commit_ref from the refs api
>
> Stefan Beller (2):
>   refs.c: don't expose the internal struct ref_lock in the header file
>   refs.c: let fprintf handle the formatting
>
>  builtin/reflog.c | 259 ++++++++++++++++++++++--------------------------------
>  refs.c           | 263 ++++++++++++++++++++++++++++++++++++++-----------------
>  refs.h           |  75 ++++++++++------
>  3 files changed, 332 insertions(+), 265 deletions(-)