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

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

[Michael Haggerty]

I propose this patch series as an alternative to Ronnie's "reflog
transactions" series.

Both alternatives have been discussed exhaustively on the list [1,2].
My opinion is that there is no need to allow arbitrary reflog changes
via the ref_transaction API, because just about the only things we
want to do are

* Append an entry to a reflog when a reference is updated. This should
  (and is already) done as a side effect of the reference update.

* Expire or delete old reflog entries based on relatively complicated
  logic, possibly repairing the remaining entries so as to preserve
  the continuity of the reflog chain.

This patch series shows that the latter can be done with a single,
fairly simple, purpose-made function, expire_reflog(), in the
references API. The policy for what reflog entries should be expired
is specified by the caller via three callback functions that don't
have to know anything about how reflogs are stored. The locking,
iteration, repair, and writing is implemented within the references
module, in a function that can easily be swapped out when pluggable
reference backends are implemented.

The remaining reflog operations (enabling/disabling reflogs for a
reference, renaming the reflog when a reference is renamed) are not
especially difficult but will be brought into the same framework in a
future patch series.

The first few patches and the last few are taken from Ronnie's and
Stefan's work. I chose *not* to rename the ref_transaction functions
for obvious reasons. A couple of the later patches from their series
would make sense but are not duplicated here.

This branch is also available on GitHub:

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

Michael

[1] http://thread.gmane.org/gmane.comp.version-control.git/259712/focus=259770
[2] http://thread.gmane.org/gmane.comp.version-control.git/260731/focus=260767

Michael Haggerty (17):
  expire_reflog(): remove unused parameter
  expire_reflog(): rename "ref" parameter to "refname"
  expire_reflog(): exit 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

Ronnie Sahlberg (5):
  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
  lock_any_ref_for_update(): inline function

Stefan Beller (1):
  refs.c: don't expose the internal struct ref_lock in the header file

 builtin/reflog.c | 259 ++++++++++++++++++++++---------------------------------
 refs.c           | 251 +++++++++++++++++++++++++++++++++++------------------
 refs.h           |  74 ++++++++++------
 3 files changed, 319 insertions(+), 265 deletions(-)

-- 
2.1.3

[PATCH 01/23] 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 02/23] 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 03/23] 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. For now this is only used from log_ref_write,
but later on we will call this function from reflog transactions too,
which means that we will end up with only a single place,
where we write a reflog entry to a file instead of the current two
places (log_ref_write and builtin/reflog.c).

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
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 04/23] expire_reflog(): remove unused parameter

[Michael Haggerty]

It was called "unused", so at least it was self-consistent.

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

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 2d85d26..3e11bee 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, int unused, void *cb_data)
+static int expire_reflog(const char *ref, const unsigned char *sha1, void *cb_data)
 {
 	struct cmd_reflog_expire_cb *cmd = cb_data;
 	struct expire_reflog_cb cb;
@@ -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 05/23] expire_reflog(): rename "ref" parameter to "refname"

[Michael Haggerty]

This is our usual convention.

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

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 3e11bee..df302f4 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, void *cb_data)
+static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)
 {
 	struct cmd_reflog_expire_cb *cmd = cb_data;
 	struct expire_reflog_cb cb;
@@ -365,20 +365,20 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, void *cb_da
 	 * 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, void *cb_da
 		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 06/23] expire_reflog(): exit 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>
---
 builtin/reflog.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index df302f4..a282e60 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -368,9 +368,11 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
 	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, void *c
 			clear_commit_marks(tip_commit, REACHABLE);
 		}
 	}
- finish:
+
 	if (cb.newlog) {
 		if (fclose(cb.newlog)) {
 			status |= error("%s: %s", strerror(errno),
-- 
2.1.3

[PATCH 07/23] 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 still does some of the bookkeeping for
us and is more careful than the old code here was.

For example:

* Correctly handle the case that the reflog lock file already exists
  for some reason or cannot be opened.

* Correctly clean up the lockfile if the program dies.

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

diff --git a/builtin/reflog.c b/builtin/reflog.c
index a282e60..d344d45 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
 	return 0;
 }
 
+static struct lock_file reflog_lock;
+
 static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)
 {
 	struct cmd_reflog_expire_cb *cmd = cb_data;
 	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;
@@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
 		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");
+		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
+			goto failure;
+		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
+		if (!cb.newlog)
+			goto failure;
 	}
 
 	cb.cmd = cmd;
@@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
 	}
 
 	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 ||
@@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
 			 close_ref(lock) < 0)) {
 			status |= error("Couldn't write %s",
 					lock->lk->filename.buf);
-			unlink(newlog_path);
-		} else if (rename(newlog_path, log_file)) {
-			status |= error("cannot rename %s to %s",
-					newlog_path, log_file);
-			unlink(newlog_path);
+			rollback_lock_file(&reflog_lock);
+		} else if (commit_lock_file(&reflog_lock)) {
+			status |= error("cannot rename %s.lock to %s",
+					log_file, log_file);
 		} else if (cmd->updateref && commit_ref(lock)) {
 			status |= error("Couldn't set %s", lock->ref_name);
-		} else {
-			adjust_shared_perm(log_file);
 		}
 	}
-	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 08/23] Extract function should_expire_reflog_ent()

[Michael Haggerty]

Extracted 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 d344d45..7bc6e0f 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 09/23] 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 expiration.

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>
---
In fact, the work done in reflog_expire_cleanup() doesn't really need
to be done via a callback, because it doesn't need to be done while
the reference lock is held. But the symmetry between prepare and
cleanup is kindof nice. Perhaps some future policy decision will want
to do some final work under the reference lock?

But it would be easy to get rid of this third callback function and
have the callers do the work themselves after calling expire_reflog().
I don't have a string feeling either way.

 builtin/reflog.c | 94 +++++++++++++++++++++++++++++++-------------------------
 1 file changed, 52 insertions(+), 42 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 7bc6e0f..ebfa635 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 struct lock_file reflog_lock;
 
 static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)
@@ -371,8 +421,6 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
 	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));
@@ -400,47 +448,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
 
 	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 10/23] 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>
---
 builtin/reflog.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index ebfa635..319f0d2 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -415,7 +415,8 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)
 
 static struct lock_file reflog_lock;
 
-static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)
+static int expire_reflog(const char *refname, const unsigned char *sha1,
+			 unsigned int flags, void *cb_data)
 {
 	struct cmd_reflog_expire_cb *cmd = cb_data;
 	struct expire_reflog_cb cb;
@@ -627,6 +628,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;
@@ -696,7 +698,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);
@@ -710,7 +712,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;
 }
@@ -729,6 +731,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));
 
@@ -781,7 +784,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 11/23] 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>
---
 builtin/reflog.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 319f0d2..a490193 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -22,7 +22,6 @@ static unsigned long default_reflog_expire_unreachable;
 
 struct cmd_reflog_expire_cb {
 	struct rev_info revs;
-	int dry_run;
 	int stalefix;
 	int rewrite;
 	int updateref;
@@ -415,6 +414,10 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)
 
 static struct lock_file reflog_lock;
 
+enum expire_reflog_flags {
+	EXPIRE_REFLOGS_DRY_RUN = 1 << 0
+};
+
 static int expire_reflog(const char *refname, const unsigned char *sha1,
 			 unsigned int flags, void *cb_data)
 {
@@ -439,7 +442,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)) {
 		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
 			goto failure;
 		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
@@ -453,7 +456,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));
@@ -644,7 +647,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);
@@ -738,7 +741,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 12/23] 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>
---
 builtin/reflog.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index a490193..597c547 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -24,7 +24,6 @@ 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;
@@ -415,7 +414,8 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)
 static struct lock_file reflog_lock;
 
 enum expire_reflog_flags {
-	EXPIRE_REFLOGS_DRY_RUN = 1 << 0
+	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
+	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1
 };
 
 static int expire_reflog(const char *refname, const unsigned char *sha1,
@@ -460,7 +460,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 ||
@@ -471,7 +471,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 		} else if (commit_lock_file(&reflog_lock)) {
 			status |= error("cannot rename %s.lock to %s",
 					log_file, log_file);
-		} 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);
 		}
 	}
@@ -663,7 +663,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"))
@@ -745,7 +745,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 13/23] 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.

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

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 597c547..3538e4b 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -30,7 +30,7 @@ struct cmd_reflog_expire_cb {
 	int recno;
 };
 
-struct expire_reflog_cb {
+struct expire_reflog_policy_cb {
 	FILE *newlog;
 	enum {
 		UE_NORMAL,
@@ -220,7 +220,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;
@@ -259,7 +259,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
@@ -295,7 +295,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)
@@ -323,7 +323,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;
@@ -350,7 +350,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;
@@ -365,7 +366,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;
@@ -397,7 +398,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) {
@@ -422,7 +423,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 			 unsigned int flags, void *cb_data)
 {
 	struct cmd_reflog_expire_cb *cmd = cb_data;
-	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 14/23] 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 "struct
expire_reflog_policy_cb". In future commits we will move some data
from the latter to the former.

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

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 3538e4b..5dfa53a 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -45,10 +45,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;
@@ -323,28 +328,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;
@@ -423,12 +429,15 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 			 unsigned int flags, void *cb_data)
 {
 	struct cmd_reflog_expire_cb *cmd = cb_data;
-	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;
 
 	/*
 	 * we take the lock for the ref itself to prevent it from
@@ -446,16 +455,16 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
 		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
 			goto failure;
-		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
-		if (!cb.newlog)
+		policy_cb.newlog = fdopen_lock_file(&reflog_lock, "w");
+		if (!policy_cb.newlog)
 			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)) {
@@ -463,7 +472,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 15/23] 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>
---
 builtin/reflog.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 5dfa53a..1512b67 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -46,6 +46,7 @@ struct expire_reflog_policy_cb {
 };
 
 struct expire_reflog_cb {
+	unsigned int flags;
 	void *policy_cb;
 };
 
@@ -437,6 +438,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 16/23] 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>
---
 builtin/reflog.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 1512b67..cc7a220 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -20,11 +20,16 @@ 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
+};
+
 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;
@@ -339,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) {
@@ -351,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;
@@ -421,11 +426,6 @@ static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)
 
 static struct lock_file reflog_lock;
 
-enum expire_reflog_flags {
-	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
-	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1
-};
-
 static int expire_reflog(const char *refname, const unsigned char *sha1,
 			 unsigned int flags, void *cb_data)
 {
@@ -679,7 +679,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;
@@ -697,10 +697,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');
 	}
 
@@ -759,7 +759,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 17/23] 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>
---
 builtin/reflog.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index cc7a220..6294406 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,
@@ -673,7 +673,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"))
@@ -755,7 +755,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 18/23] 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>
---
 builtin/reflog.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 6294406..01b76d0 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);
@@ -457,8 +457,8 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
 		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
 			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)
 			goto failure;
 	}
 
@@ -474,7 +474,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 19/23] 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 its callers to pass it a pointer to an entire "struct
  expire_reflog_policy_cb".

* Change it to pass the pointer through as a "void *".

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

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

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 01b76d0..c30936bb 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;
@@ -427,19 +431,16 @@ static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)
 static struct lock_file reflog_lock;
 
 static int expire_reflog(const char *refname, const unsigned char *sha1,
-			 unsigned int flags, void *cb_data)
+			 unsigned int flags, void *policy_cb_data)
 {
-	struct cmd_reflog_expire_cb *cmd = cb_data;
 	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;
 
 	/*
 	 * we take the lock for the ref itself to prevent it from
@@ -462,11 +463,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
 			goto failure;
 	}
 
-	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)) {
@@ -639,7 +638,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;
@@ -653,25 +652,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"))
@@ -695,11 +694,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');
 	}
@@ -712,7 +711,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);
 		}
@@ -726,7 +725,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;
@@ -736,15 +735,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;
 
@@ -791,12 +790,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 20/23] 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>
---
 builtin/reflog.c | 133 +++++++------------------------------------------------
 refs.c           | 114 +++++++++++++++++++++++++++++++++++++++++++++++
 refs.h           |  45 +++++++++++++++++++
 3 files changed, 174 insertions(+), 118 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index c30936bb..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,75 +382,6 @@ static void reflog_expiry_cleanup(void *cb_data)
 	}
 }
 
-static struct lock_file reflog_lock;
-
-static int expire_reflog(const char *refname, const unsigned char *sha1,
-			 unsigned int flags, void *policy_cb_data)
-{
-	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;
-
-	/*
-	 * we take the lock for the ref itself to prevent it from
-	 * getting updated.
-	 */
-	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)) {
-		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
-			goto failure;
-		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
-		if (!cb.newlog)
-			goto failure;
-	}
-
-	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("cannot rename %s.lock to %s",
-					log_file, log_file);
-		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {
-			status |= error("Couldn't set %s", lock->ref_name);
-		}
-	}
-	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;
@@ -712,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);
@@ -726,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;
 }
@@ -798,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..5a12d37 100644
--- a/refs.c
+++ b/refs.c
@@ -3943,3 +3943,117 @@ int ref_is_hidden(const char *refname)
 	}
 	return 0;
 }
+
+struct expire_reflog_cb {
+	unsigned int flags;
+	reflog_expiry_select_fn *select_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->select_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;
+}
+
+static struct lock_file reflog_lock;
+
+extern int reflog_expire(const char *refname, const unsigned char *sha1,
+			 unsigned int flags,
+			 reflog_expiry_prepare_fn prepare_fn,
+			 reflog_expiry_select_fn select_fn,
+			 reflog_expiry_cleanup_fn cleanup_fn,
+			 void *policy_cb_data)
+{
+	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.select_fn = select_fn;
+
+	/*
+	 * we take the lock for the ref itself to prevent it from
+	 * getting updated.
+	 */
+	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)) {
+		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
+			goto failure;
+		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
+		if (!cb.newlog)
+			goto failure;
+	}
+
+	(*prepare_fn)(refname, sha1, cb.policy_cb);
+	for_each_reflog_ent(refname, expire_reflog_ent, &cb);
+	(*cleanup_fn)(cb.policy_cb);
+
+	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
+		if (close_lock_file(&reflog_lock)) {
+			status |= error("Couldn't write %s: %s", log_file,
+					strerror(errno));
+		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
+			(write_in_full(lock->lock_fd,
+				sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
+			 write_str_in_full(lock->lock_fd, "\n") != 1 ||
+			 close_ref(lock) < 0)) {
+			status |= error("Couldn't write %s",
+					lock->lk->filename.buf);
+			rollback_lock_file(&reflog_lock);
+		} else if (commit_lock_file(&reflog_lock)) {
+			status |= error("cannot rename %s.lock to %s",
+					log_file, log_file);
+		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {
+			status |= error("Couldn't set %s", lock->ref_name);
+		}
+	}
+	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..d777f76 100644
--- a/refs.h
+++ b/refs.h
@@ -353,4 +353,49 @@ 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_select_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_select_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_select_fn select_fn,
+			 reflog_expiry_cleanup_fn cleanup_fn,
+			 void *policy_cb_data);
+
 #endif /* REFS_H */
-- 
2.1.3

[PATCH 21/23] 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 5a12d37..c6ee775 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 d777f76..82611b5 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 22/23] lock_any_ref_for_update(): inline function

[Michael Haggerty]

From: Ronnie Sahlberg <sahlberg@google.com>

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 c6ee775..f3564cf 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,
 	 * we take the lock for the ref itself to prevent it from
 	 * getting updated.
 	 */
-	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 82611b5..174863d 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 23/23] 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 f3564cf..40c5591 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 174863d..2957641 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

Re: [PATCH 04/23] expire_reflog(): remove unused parameter

[Jonathan Nieder]

Michael Haggerty wrote:

> It was called "unused", so at least it was self-consistent.

The missing context is that this was a callback function that had to
match the each_ref_fn signature (where that parameter is 'flags')
until v1.5.4~14 (reflog-expire: avoid creating new files in a
directory inside readdir(3) loop, 2008-01-25).  v1.5.4~14 forgot to
clean up.

With or without a note in the commit message explaining that,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 04/23] expire_reflog(): remove unused parameter

[Jonathan Nieder]

Michael Haggerty wrote:

> --- 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, int unused, void *cb_data)
> +static int expire_reflog(const char *ref, const unsigned char *sha1, void *cb_data)
>  {
>  	struct cmd_reflog_expire_cb *cmd = cb_data;

On second thought: why not update the last parameter to be a 'struct
cmd_reflog_expire_cb *' instead of 'void *' while at it, like this?

 builtin/reflog.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git i/builtin/reflog.c w/builtin/reflog.c
index 3e11bee..d860624 100644
--- i/builtin/reflog.c
+++ w/builtin/reflog.c
@@ -349,9 +349,8 @@ 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, 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;

Re: [PATCH 05/23] expire_reflog(): rename "ref" parameter to "refname"

[Jonathan Nieder]

Michael Haggerty wrote:

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

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

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

[Junio C Hamano]

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

> I propose this patch series as an alternative to Ronnie's "reflog
> transactions" series.

After coasting my eyes over individual patches, it was delightful to
look at the endgame state of the expiry codepath in builtin/reflog.c
that is left ;-)

Re: [PATCH 06/23] expire_reflog(): exit early if the reference has no reflog

[Jonathan Nieder]

Michael Haggerty wrote:

> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -368,9 +368,11 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>  	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;

Okay.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 06/23] expire_reflog(): exit early if the reference has no reflog

[Jonathan Nieder]

Michael Haggerty wrote:

> [Subject: expire_reflog(): exit early if the reference has no reflog]

The caller moves on to expire other reflogs, so it's not exiting.
"return early", maybe?

Except the function already returned early.  I think the purpose of
this patch is to simplify the no-reflog case by handling it separately.

Anyway, that's just nitpicking about the subject line.  With
s/exit/return/ it should be clear that this is a refactoring change,
which for someone looking at the shortlog is the important thing.

Thanks,
Jonathan

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

[Jonathan Nieder]

Michael Haggerty 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 still does some of the bookkeeping for
> us and is more careful than the old code here was.

As you say, the ref lock takes care of mutual exclusion, so we do not
have to be too careful about compatibility with other tools that might
not know to lock the reflog.  And this is not tying our hands for a
future when I might want to lock logs/refs/heads/topic/1 while
logs/refs/heads/topic still exists as part of the implementation of
"git mv topic/1 topic".

Stefan and I had forgotten about that guarantee when looking at that
kind of operation --- thanks for the reminder.

Should updates to the HEAD reflog acquire HEAD.lock?  (They don't
currently.)

[...]
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
>  	return 0;
>  }
>  
> +static struct lock_file reflog_lock;

If this lockfile is only used in that one function, it can be declared
inside the function.

If it is meant to be used throughout the 'git reflog' command, then it
can go near the top of the file.

> +
>  static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)
>  {
>  	struct cmd_reflog_expire_cb *cmd = cb_data;
>  	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;
> @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>  		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");
> +		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
> +			goto failure;

hold_lock_file_for_update doesn't print a message.  Code to print one
looks like

	if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0) {
		unable_to_lock_message(log_file, errno, &err);
		error("%s", err.buf);
		goto failure;
	}

(A patch in flight changes that to

	if (hold_lock_file_for_update(&reflog_lock, log_file, 0, &err) < 0) {
		error("%s", err.buf);
		goto failure;
	}

)

> +		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> +		if (!cb.newlog)
> +			goto failure;

Hm.  lockfile.c::fdopen_lock_file ought to use xfdopen to make this
case impossible.  And xfdopen should use try_to_free_routine() and
try again on failure.

[...]
> @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>  	}
>  
>  	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));

Style nit: error messages usually start with a lowercase letter
(though I realize nearby examples are already inconsistent).

commit_lock_file() can take care of the close_lock_file automatically.

[...]
> @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>  			 close_ref(lock) < 0)) {
>  			status |= error("Couldn't write %s",
>  					lock->lk->filename.buf);
> -			unlink(newlog_path);
> -		} else if (rename(newlog_path, log_file)) {
> -			status |= error("cannot rename %s to %s",
> -					newlog_path, log_file);
> -			unlink(newlog_path);
> +			rollback_lock_file(&reflog_lock);
> +		} else if (commit_lock_file(&reflog_lock)) {
> +			status |= error("cannot rename %s.lock to %s",
> +					log_file, log_file);

Most callers say "unable to commit reflog '%s'", log_file to hedge their
bets in case the close failed (which may be what you were avoiding
above.

errno is meaningful when commit_lock_file fails, making a more
detailed diagnosis from strerror(errno) possible.

Thanks,
Jonathan

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

[Stefan Beller]

On Thu, Dec 04, 2014 at 04:23:31PM -0800, Jonathan Nieder wrote:
> Michael Haggerty 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 still does some of the bookkeeping for
> > us and is more careful than the old code here was.
> 
> As you say, the ref lock takes care of mutual exclusion, so we do not
> have to be too careful about compatibility with other tools that might
> not know to lock the reflog.  And this is not tying our hands for a
> future when I might want to lock logs/refs/heads/topic/1 while
> logs/refs/heads/topic still exists as part of the implementation of
> "git mv topic/1 topic".
> 
> Stefan and I had forgotten about that guarantee when looking at that
> kind of operation --- thanks for the reminder.

I did not forget about it, I did not know about that in the first hand.
We don't seem to have documentation on it?

So sorry for heading in a direction, which would have been avoidable.

Thanks,
Stefan

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

[ronnie sahlberg]

On Thu, Dec 4, 2014 at 3:08 PM, 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,

No. You do need the lock.
The ref is locked only during transaction_commit()

If you don't want to lock the reflog file and instead rely on the lock
on the ref itself you will need to
rework your patches so that the lock on the ref is taken already
during, for example, transaction_update_ref() instead.

But without doing those changes and moving the ref locking from
_commit() to _update_ref() you will risk reflog corruption/surprises
if two operations collide and both rewrite the reflog without any lock held.


> which is how the reflog file is
> locked. But the lock_file code still does some of the bookkeeping for
> us and is more careful than the old code here was.
>
> For example:
>
> * Correctly handle the case that the reflog lock file already exists
>   for some reason or cannot be opened.
>
> * Correctly clean up the lockfile if the program dies.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  builtin/reflog.c | 37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index a282e60..d344d45 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
>         return 0;
>  }
>
> +static struct lock_file reflog_lock;
> +
>  static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)
>  {
>         struct cmd_reflog_expire_cb *cmd = cb_data;
>         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;
> @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>                 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");
> +               if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
> +                       goto failure;
> +               cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> +               if (!cb.newlog)
> +                       goto failure;
>         }
>
>         cb.cmd = cmd;
> @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>         }
>
>         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 ||
> @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>                          close_ref(lock) < 0)) {
>                         status |= error("Couldn't write %s",
>                                         lock->lk->filename.buf);
> -                       unlink(newlog_path);
> -               } else if (rename(newlog_path, log_file)) {
> -                       status |= error("cannot rename %s to %s",
> -                                       newlog_path, log_file);
> -                       unlink(newlog_path);
> +                       rollback_lock_file(&reflog_lock);
> +               } else if (commit_lock_file(&reflog_lock)) {
> +                       status |= error("cannot rename %s.lock to %s",
> +                                       log_file, log_file);
>                 } else if (cmd->updateref && commit_ref(lock)) {
>                         status |= error("Couldn't set %s", lock->ref_name);
> -               } else {
> -                       adjust_shared_perm(log_file);
>                 }
>         }
> -       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 04/23] expire_reflog(): remove unused parameter

[Michael Haggerty]

On 12/05/2014 12:28 AM, Jonathan Nieder wrote:> Michael Haggerty wrote:
>> It was called "unused", so at least it was self-consistent.
>
> The missing context is that this was a callback function that had to
> match the each_ref_fn signature [...]
>
> With or without a note in the commit message explaining that,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[...]

>> --- 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,
int unused, void *cb_data)
>> +static int expire_reflog(const char *ref, const unsigned char *sha1,
void *cb_data)
>>  {
>>  	struct cmd_reflog_expire_cb *cmd = cb_data;
>
> On second thought: why not update the last parameter to be a 'struct
> cmd_reflog_expire_cb *' instead of 'void *' while at it, like this?
> [...]

Thanks for the explanation, the review, and the suggestion. I will
expand the commit to be "don't implement each_ref_fn anymore" and
incorporate all of your suggestions.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 06/23] expire_reflog(): exit early if the reference has no reflog

[Michael Haggerty]

On 12/05/2014 12:53 AM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> [Subject: expire_reflog(): exit early if the reference has no reflog]
> 
> The caller moves on to expire other reflogs, so it's not exiting.
> "return early", maybe?
> 
> Except the function already returned early.  I think the purpose of
> this patch is to simplify the no-reflog case by handling it separately.
> 
> Anyway, that's just nitpicking about the subject line.  With
> s/exit/return/ it should be clear that this is a refactoring change,
> which for someone looking at the shortlog is the important thing.

Good suggestion. I will change the commit message.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

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

[Stefan Beller]

On Thu, Dec 04, 2014 at 06:51:36PM -0800, ronnie sahlberg wrote:
> On Thu, Dec 4, 2014 at 3:08 PM, 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,
> 
> 
> No. You do need the lock.
> The ref is locked only during transaction_commit()
> 

Michael is saying, that you never want to modify the reflog, if you're not holding
the lock on the corresponding ref. Or in other words, you may modify the reflog, if
the corresponding ref is held. This used to be this way back then. 

> If you don't want to lock the reflog file and instead rely on the lock on
> the ref itself you will need to
> rework your patches so that the lock on the ref is taken already during,
> for example, transaction_update_ref() instead.
> 
> But without doing those changes and moving the ref locking from _commit()
> to _update_ref() you will risk reflog corruption/surprises
> if two operations collide and both rewrite the reflog without any lock held.
> 
> 
> 
> 
> 
> > which is how the reflog file is
> > locked. But the lock_file code still does some of the bookkeeping for
> > us and is more careful than the old code here was.
> >
> > For example:
> >
> > * Correctly handle the case that the reflog lock file already exists
> >   for some reason or cannot be opened.
> >
> > * Correctly clean up the lockfile if the program dies.
> >
> > Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> > ---
> >  builtin/reflog.c | 37 ++++++++++++++++++++++---------------
> >  1 file changed, 22 insertions(+), 15 deletions(-)
> >
> > diff --git a/builtin/reflog.c b/builtin/reflog.c
> > index a282e60..d344d45 100644
> > --- a/builtin/reflog.c
> > +++ b/builtin/reflog.c
> > @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname,
> > const unsigned char *sha1, int
> >         return 0;
> >  }
> >
> > +static struct lock_file reflog_lock;
> > +
> >  static int expire_reflog(const char *refname, const unsigned char *sha1,
> > void *cb_data)
> >  {
> >         struct cmd_reflog_expire_cb *cmd = cb_data;
> >         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;
> > @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const
> > unsigned char *sha1, void *c
> >                 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");
> > +               if (hold_lock_file_for_update(&reflog_lock, log_file, 0) <
> > 0)
> > +                       goto failure;
> > +               cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> > +               if (!cb.newlog)
> > +                       goto failure;
> >         }
> >
> >         cb.cmd = cmd;
> > @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const
> > unsigned char *sha1, void *c
> >         }
> >
> >         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
> > ||
> > @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const
> > unsigned char *sha1, void *c
> >                          close_ref(lock) < 0)) {
> >                         status |= error("Couldn't write %s",
> >                                         lock->lk->filename.buf);
> > -                       unlink(newlog_path);
> > -               } else if (rename(newlog_path, log_file)) {
> > -                       status |= error("cannot rename %s to %s",
> > -                                       newlog_path, log_file);
> > -                       unlink(newlog_path);
> > +                       rollback_lock_file(&reflog_lock);
> > +               } else if (commit_lock_file(&reflog_lock)) {
> > +                       status |= error("cannot rename %s.lock to %s",
> > +                                       log_file, log_file);
> >                 } else if (cmd->updateref && commit_ref(lock)) {
> >                         status |= error("Couldn't set %s", lock->ref_name);
> > -               } else {
> > -                       adjust_shared_perm(log_file);
> >                 }
> >         }
> > -       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 09/23] expire_reflog(): extract two policy-related functions

[Stefan Beller]

On Fri, Dec 05, 2014 at 12:08:21AM +0100, Michael Haggerty wrote:
> 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 expiration.
> 
> 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>

> ---
> In fact, the work done in reflog_expire_cleanup() doesn't really need
> to be done via a callback, because it doesn't need to be done while
> the reference lock is held. But the symmetry between prepare and
> cleanup is kindof nice. Perhaps some future policy decision will want
> to do some final work under the reference lock?
> 
> But it would be easy to get rid of this third callback function and
> have the callers do the work themselves after calling expire_reflog().
> I don't have a string feeling either way.
> 
>  builtin/reflog.c | 94 +++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 52 insertions(+), 42 deletions(-)
> 
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 7bc6e0f..ebfa635 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 struct lock_file reflog_lock;
>  
>  static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)
> @@ -371,8 +421,6 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>  	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));
> @@ -400,47 +448,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>  
>  	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
> 

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

[Stefan Beller]

On Thu, Dec 04, 2014 at 04:23:31PM -0800, Jonathan Nieder wrote:
> Michael Haggerty 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 still does some of the bookkeeping for
> > us and is more careful than the old code here was.
> 
> As you say, the ref lock takes care of mutual exclusion, so we do not
> have to be too careful about compatibility with other tools that might
> not know to lock the reflog.  And this is not tying our hands for a
> future when I might want to lock logs/refs/heads/topic/1 while
> logs/refs/heads/topic still exists as part of the implementation of
> "git mv topic/1 topic".
> 
> Stefan and I had forgotten about that guarantee when looking at that
> kind of operation --- thanks for the reminder.
> 
> Should updates to the HEAD reflog acquire HEAD.lock?  (They don't
> currently.)
> 
> [...]
> > --- a/builtin/reflog.c
> > +++ b/builtin/reflog.c
> > @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
> >  	return 0;
> >  }
> >  
> > +static struct lock_file reflog_lock;
> 
> If this lockfile is only used in that one function, it can be declared
> inside the function.
> 
> If it is meant to be used throughout the 'git reflog' command, then it
> can go near the top of the file.
> 

After the series completes, this lock is only used in reflog_expire.
So I'd rather move it inside the function? Then we could run the reflog_expire
function in parallel for different locks in theory?

> > +
> >  static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)
> >  {
> >  	struct cmd_reflog_expire_cb *cmd = cb_data;
> >  	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;
> > @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
> >  		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");
> > +		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
> > +			goto failure;
> 
> hold_lock_file_for_update doesn't print a message.  Code to print one
> looks like
> 
> 	if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0) {
> 		unable_to_lock_message(log_file, errno, &err);
> 		error("%s", err.buf);
> 		goto failure;
> 	}
> 
> (A patch in flight changes that to
> 
> 	if (hold_lock_file_for_update(&reflog_lock, log_file, 0, &err) < 0) {
> 		error("%s", err.buf);
> 		goto failure;
> 	}
> 
> )
> 
> > +		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> > +		if (!cb.newlog)
> > +			goto failure;
> 
> Hm.  lockfile.c::fdopen_lock_file ought to use xfdopen to make this
> case impossible.  And xfdopen should use try_to_free_routine() and
> try again on failure.
> 
> [...]
> > @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
> >  	}
> >  
> >  	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));
> 
> Style nit: error messages usually start with a lowercase letter
> (though I realize nearby examples are already inconsistent).
> 
> commit_lock_file() can take care of the close_lock_file automatically.
> 
> [...]
> > @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
> >  			 close_ref(lock) < 0)) {
> >  			status |= error("Couldn't write %s",
> >  					lock->lk->filename.buf);
> > -			unlink(newlog_path);
> > -		} else if (rename(newlog_path, log_file)) {
> > -			status |= error("cannot rename %s to %s",
> > -					newlog_path, log_file);
> > -			unlink(newlog_path);
> > +			rollback_lock_file(&reflog_lock);
> > +		} else if (commit_lock_file(&reflog_lock)) {
> > +			status |= error("cannot rename %s.lock to %s",
> > +					log_file, log_file);
> 
> Most callers say "unable to commit reflog '%s'", log_file to hedge their
> bets in case the close failed (which may be what you were avoiding
> above.
> 
> errno is meaningful when commit_lock_file fails, making a more
> detailed diagnosis from strerror(errno) possible.
> 
> Thanks,
> Jonathan

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> > +static struct lock_file reflog_lock;
>> 
>> If this lockfile is only used in that one function, it can be declared
>> inside the function.
>> 
>> If it is meant to be used throughout the 'git reflog' command, then it
>> can go near the top of the file.
>> 
>
> After the series completes, this lock is only used in reflog_expire.
> So I'd rather move it inside the function? Then we could run the reflog_expire
> function in parallel for different locks in theory?

I am not sure about the "parallel" part, but I would imagine that it
is an essential prerequisite to move this outside the "client" code
if we want to later replace the backing storage of refs and reflogs
outside the filesystem, so from that point of view,  I think the
suggestion makes sense.

Thanks.

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

[Stefan Beller]

On Fri, Dec 5, 2014 at 11:32 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> > +static struct lock_file reflog_lock;
>>>
>>> If this lockfile is only used in that one function, it can be declared
>>> inside the function.
>>>
>>> If it is meant to be used throughout the 'git reflog' command, then it
>>> can go near the top of the file.
>>>
>>
>> After the series completes, this lock is only used in reflog_expire.
>> So I'd rather move it inside the function? Then we could run the reflog_expire
>> function in parallel for different locks in theory?
>
> I am not sure about the "parallel" part, but I would imagine that it
> is an essential prerequisite to move this outside the "client" code
> if we want to later replace the backing storage of refs and reflogs
> outside the filesystem, so from that point of view,  I think the
> suggestion makes sense.
>
> Thanks.
>

Sorry for the confusion. With parallel I mean, that in theory we could have
multiple threads running reflog expire at the same time in the same program.
Having the "static struct lock_file reflog_lock;" around, we can only
process one
reflog at a time, as that is holding the lock_file struct.

I am not saying we want to go multi-threading any time soon if at all.
Just that it would
be easier to do, if not having the lock file as a file-global variable
instead of a
variable inside a function.

Thanks,
Stefan

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>>> After the series completes, this lock is only used in reflog_expire.
>>> So I'd rather move it inside the function? Then we could run the reflog_expire
>>> function in parallel for different locks in theory?
>>
>> I am not sure about the "parallel" part, but I would imagine that it
>> is an essential prerequisite to move this outside the "client" code
>> if we want to later replace the backing storage of refs and reflogs
>> outside the filesystem, so from that point of view,  I think the
>> suggestion makes sense.
>>
>> Thanks.
>>
>
> Sorry for the confusion. With parallel I mean,...

There is no confusion.  I understand exactly what you meant.

What I said was not sure was if "parallel" is a practical enough
possiblity to include into the set of value propositions the
suggested change to move the lock out of the "client" may give us.

In other words, "With this change, doing a parallel will become a
lot easier"---"Really?  It probably is not one of the harder part of
the problem if you really want to go parallel" was the discourse I
had in my mind.

;-)

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

[Michael Haggerty]

On 12/05/2014 03:19 AM, Stefan Beller wrote:
> On Thu, Dec 04, 2014 at 04:23:31PM -0800, Jonathan Nieder wrote:
>> Michael Haggerty 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 still does some of the bookkeeping for
>>> us and is more careful than the old code here was.
>>
>> As you say, the ref lock takes care of mutual exclusion, so we do not
>> have to be too careful about compatibility with other tools that might
>> not know to lock the reflog.  And this is not tying our hands for a
>> future when I might want to lock logs/refs/heads/topic/1 while
>> logs/refs/heads/topic still exists as part of the implementation of
>> "git mv topic/1 topic".
>>
>> Stefan and I had forgotten about that guarantee when looking at that
>> kind of operation --- thanks for the reminder.
> 
> I did not forget about it, I did not know about that in the first hand.
> We don't seem to have documentation on it?
> 
> So sorry for heading in a direction, which would have been avoidable.

This isn't documented very well. I thought I saw a comment somewhere in
the code that stated it explicitly, but I can't find it now. In any
case, my understanding of the locking protocol for reflogs is:

    The reflog for "$refname", which is stored at
    "$GIT_DIR/logs/$refname", is locked by holding
    "$GIT_DIR/refs/$refname.lock", *even if the corresponding
    reference is packed*.

This implies that readers, who don't pay attention to locks, have to be
prepared for the possibility that the reflog is in the middle of an
update and that the last line is incomplete. This is handled by
show_one_reflog_ent(), which discards incomplete lines.

This protocol avoids the need to rewrite the reflog from scratch for
each reference update.

Given how poorly-documented this point is, I wonder whether other
implementations of Git (e.g., libgit2, JGit, Dulwich, ...) got it right.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

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

[Michael Haggerty]

On 12/05/2014 03:59 AM, ronnie sahlberg wrote:
> On Thu, Dec 4, 2014 at 3:08 PM, 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,
> 
> No. You do need the lock.
> The ref is locked only during transaction_commit()
> 
> If you don't want to lock the reflog file and instead rely on the lock
> on the ref itself you will need to
> rework your patches so that the lock on the ref is taken already
> during, for example, transaction_update_ref() instead.
> 
> But without doing those changes and moving the ref locking from
> _commit() to _update_ref() you will risk reflog corruption/surprises
> if two operations collide and both rewrite the reflog without any lock held.

Ronnie, I don't understand your comments.

It is a statement of fact (to the best of my knowledge) that reflogs are
supposed to be modified only under a lock on the corresponding
reference, namely "$GIT_DIR/refs/$refname.lock". We do not require
reflog writers to hold "$GIT_DIR/logs/$refname.lock".

In this function, "$GIT_DIR/logs/$refname.lock" happens to be the name
of the temporary file being used to stage the new contents of the
reflog. But that is more or less a coincidence; we could call the
temporary file whatever we want because it has no locking implications.
However, what we want to do with the file in this code path (write a new
version then rename the new version on top of the old version, deleting
the temporary file if the program is interrupted) is the same as what we
do with lockfiles, so we use the lockfile code because it is convenient.

This patch series has nothing to do with ref_transaction_commit() or any
of the transaction machinery. It has to do with expire_reflog(), which
is invoked outside of any transaction.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

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

[Michael Haggerty]

On 12/05/2014 01:23 AM, Jonathan Nieder wrote:
> Michael Haggerty 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 still does some of the bookkeeping for
>> us and is more careful than the old code here was.
> 
> As you say, the ref lock takes care of mutual exclusion, so we do not
> have to be too careful about compatibility with other tools that might
> not know to lock the reflog.  And this is not tying our hands for a
> future when I might want to lock logs/refs/heads/topic/1 while
> logs/refs/heads/topic still exists as part of the implementation of
> "git mv topic/1 topic".
> 
> Stefan and I had forgotten about that guarantee when looking at that
> kind of operation --- thanks for the reminder.

This reminder is important (and forgettable) enough that I will add a
comment within the function explaining it.

> Should updates to the HEAD reflog acquire HEAD.lock?  (They don't
> currently.)

Yes, they should; good catch. I assume that you are referring to the
code at the bottom of write_ref_sha1()? Or did you find a problem in
this patch series?

If the former, then I propose that we address this bug in a separate
patch series.

> [...]
>> --- a/builtin/reflog.c
>> +++ b/builtin/reflog.c
>> @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
>>  	return 0;
>>  }
>>  
>> +static struct lock_file reflog_lock;
> 
> If this lockfile is only used in that one function, it can be declared
> inside the function.
> 
> If it is meant to be used throughout the 'git reflog' command, then it
> can go near the top of the file.

For now it is only used within this function, so I will move it into the
function as you suggest. (As you know, it does need to remain static,
because of the way the lock_file module takes over ownership of these
objects.)

>> +
>>  static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)
>>  {
>>  	struct cmd_reflog_expire_cb *cmd = cb_data;
>>  	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;
>> @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>>  		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");
>> +		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
>> +			goto failure;
> 
> hold_lock_file_for_update doesn't print a message.  Code to print one
> looks like
> 
> 	if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0) {
> 		unable_to_lock_message(log_file, errno, &err);
> 		error("%s", err.buf);
> 		goto failure;
> 	}

Thanks; will add.

> (A patch in flight changes that to
> 
> 	if (hold_lock_file_for_update(&reflog_lock, log_file, 0, &err) < 0) {
> 		error("%s", err.buf);
> 		goto failure;
> 	}
> 
> )

Thanks for the heads-up. The compiler will complain when the branches
are merged, and hopefully the fix will be obvious.

>> +		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
>> +		if (!cb.newlog)
>> +			goto failure;
> 
> Hm.  lockfile.c::fdopen_lock_file ought to use xfdopen to make this
> case impossible.  And xfdopen should use try_to_free_routine() and
> try again on failure.

That sounds reasonable, but it is not manifestly obvious given that at
least one caller of fdopen_lock_file() (in fast-import.c) tries to
recover if fdopen_lock_file() fails. Let's address this in a separate
patch series if that is OK with you. For now I will add explicit
error-reporting code here before "goto failure".

> [...]
>> @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>>  	}
>>  
>>  	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));
> 
> Style nit: error messages usually start with a lowercase letter
> (though I realize nearby examples are already inconsistent).

Thanks; will fix.

> commit_lock_file() can take care of the close_lock_file automatically.

The existing code is a tiny bit safer: first make sure both files can be
written, *then* rename each of them into place. If either write fails,
then both files will get rolled back. But if we switch to using
commit_lock_file(), then a failure when writing the reference would
leave the reflog updated but the reference rolled back.

> [...]
>> @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>>  			 close_ref(lock) < 0)) {
>>  			status |= error("Couldn't write %s",
>>  					lock->lk->filename.buf);
>> -			unlink(newlog_path);
>> -		} else if (rename(newlog_path, log_file)) {
>> -			status |= error("cannot rename %s to %s",
>> -					newlog_path, log_file);
>> -			unlink(newlog_path);
>> +			rollback_lock_file(&reflog_lock);
>> +		} else if (commit_lock_file(&reflog_lock)) {
>> +			status |= error("cannot rename %s.lock to %s",
>> +					log_file, log_file);
> 
> Most callers say "unable to commit reflog '%s'", log_file to hedge their
> bets in case the close failed (which may be what you were avoiding
> above.
> 
> errno is meaningful when commit_lock_file fails, making a more
> detailed diagnosis from strerror(errno) possible.

I will improve the error message.

Thanks for your detailed review!
Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

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

[Stefan Beller]

On Fri, Dec 05, 2014 at 12:08:20AM +0100, Michael Haggerty wrote:
> Extracted 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>

The comments below are just thoughts, which don't need to be
included into this commit.


> +	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);

While this commit is just shoveling code around, we don't want to introduce
changes here. So a question for a possible later follow up:
"git reflog" is listed as an ancillary manipulator, which still is porcelain.
So we maybe want to translate "[would] prune"?


> +			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);

This is fine for just moving code around and reviewing.
I send a patch on top of this one to remove the manual calculation of the 
sign and zone and let the fprintf function figure it out.

Re: [PATCH 10/23] expire_reflog(): add a "flags" argument

[Stefan Beller]

On Fri, Dec 05, 2014 at 12:08:22AM +0100, Michael Haggerty wrote:
> 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: <sbeller@google.com>

> ---
>  builtin/reflog.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index ebfa635..319f0d2 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -415,7 +415,8 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)
>  
>  static struct lock_file reflog_lock;
>  
> -static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)
> +static int expire_reflog(const char *refname, const unsigned char *sha1,
> +			 unsigned int flags, void *cb_data)
>  {
>  	struct cmd_reflog_expire_cb *cmd = cb_data;
>  	struct expire_reflog_cb cb;
> @@ -627,6 +628,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;
> @@ -696,7 +698,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);
> @@ -710,7 +712,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;
>  }
> @@ -729,6 +731,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));
>  
> @@ -781,7 +784,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
> 

Re: [PATCH 11/23] expire_reflog(): move dry_run to flags argument

[Stefan Beller]

On Fri, Dec 05, 2014 at 12:08:23AM +0100, Michael Haggerty wrote:
> 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: <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 319f0d2..a490193 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -22,7 +22,6 @@ static unsigned long default_reflog_expire_unreachable;
>  
>  struct cmd_reflog_expire_cb {
>  	struct rev_info revs;
> -	int dry_run;
>  	int stalefix;
>  	int rewrite;
>  	int updateref;
> @@ -415,6 +414,10 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)
>  
>  static struct lock_file reflog_lock;
>  
> +enum expire_reflog_flags {
> +	EXPIRE_REFLOGS_DRY_RUN = 1 << 0
> +};
> +
>  static int expire_reflog(const char *refname, const unsigned char *sha1,
>  			 unsigned int flags, void *cb_data)
>  {
> @@ -439,7 +442,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)) {
>  		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
>  			goto failure;
>  		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> @@ -453,7 +456,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));
> @@ -644,7 +647,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);
> @@ -738,7 +741,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
> 

Re: [PATCH 12/23] expire_reflog(): move updateref to flags argument

[Stefan Beller]

On Fri, Dec 05, 2014 at 12:08:24AM +0100, Michael Haggerty wrote:
> 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: <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 a490193..597c547 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -24,7 +24,6 @@ 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;
> @@ -415,7 +414,8 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)
>  static struct lock_file reflog_lock;
>  
>  enum expire_reflog_flags {
> -	EXPIRE_REFLOGS_DRY_RUN = 1 << 0
> +	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
> +	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1
>  };
>  
>  static int expire_reflog(const char *refname, const unsigned char *sha1,
> @@ -460,7 +460,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 ||
> @@ -471,7 +471,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>  		} else if (commit_lock_file(&reflog_lock)) {
>  			status |= error("cannot rename %s.lock to %s",
>  					log_file, log_file);
> -		} 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);
>  		}
>  	}
> @@ -663,7 +663,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"))
> @@ -745,7 +745,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
> 

Re: [PATCH 13/23] Rename expire_reflog_cb to expire_reflog_policy_cb

[Stefan Beller]

On Fri, Dec 05, 2014 at 12:08:25AM +0100, Michael Haggerty wrote:
> This is the first step towards separating the data needed by the
> policy code from the data needed by the reflog expiration machinery.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

Reviewed-by: <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 597c547..3538e4b 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -30,7 +30,7 @@ struct cmd_reflog_expire_cb {
>  	int recno;
>  };
>  
> -struct expire_reflog_cb {
> +struct expire_reflog_policy_cb {
>  	FILE *newlog;
>  	enum {
>  		UE_NORMAL,
> @@ -220,7 +220,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;
> @@ -259,7 +259,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
> @@ -295,7 +295,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)
> @@ -323,7 +323,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;
> @@ -350,7 +350,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;
> @@ -365,7 +366,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;
> @@ -397,7 +398,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) {
> @@ -422,7 +423,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>  			 unsigned int flags, void *cb_data)
>  {
>  	struct cmd_reflog_expire_cb *cmd = cb_data;
> -	struct expire_reflog_cb cb;
> +	struct expire_reflog_policy_cb cb;
>  	struct ref_lock *lock;
>  	char *log_file;
>  	int status = 0;
> -- 
> 2.1.3
> 

Re: [PATCH 14/23] struct expire_reflog_cb: a new callback data type

[Stefan Beller]

On Fri, Dec 05, 2014 at 12:08:26AM +0100, Michael Haggerty wrote:
> 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 "struct
> expire_reflog_policy_cb". In future commits we will move some data
> from the latter to the former.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

Reviewed-by: <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 3538e4b..5dfa53a 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -45,10 +45,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;
> @@ -323,28 +328,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;
> @@ -423,12 +429,15 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>  			 unsigned int flags, void *cb_data)
>  {
>  	struct cmd_reflog_expire_cb *cmd = cb_data;
> -	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;
>  
>  	/*
>  	 * we take the lock for the ref itself to prevent it from
> @@ -446,16 +455,16 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>  	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
>  		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
>  			goto failure;
> -		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> -		if (!cb.newlog)
> +		policy_cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> +		if (!policy_cb.newlog)
>  			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)) {
> @@ -463,7 +472,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
> 

Re: [PATCH 15/23] expire_reflog(): pass flags through to expire_reflog_ent()

[Stefan Beller]

On Fri, Dec 05, 2014 at 12:08:27AM +0100, Michael Haggerty wrote:
> 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: <sbeller@google.com>

> ---
>  builtin/reflog.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 5dfa53a..1512b67 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -46,6 +46,7 @@ struct expire_reflog_policy_cb {
>  };
>  
>  struct expire_reflog_cb {
> +	unsigned int flags;
>  	void *policy_cb;
>  };
>  
> @@ -437,6 +438,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
> 

Re: [PATCH 16/23] expire_reflog(): move verbose to flags argument

[Stefan Beller]

On Fri, Dec 05, 2014 at 12:08:28AM +0100, Michael Haggerty wrote:
> 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 | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 1512b67..cc7a220 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -20,11 +20,16 @@ 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
> +};
> +
>  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;
> @@ -339,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) {
> @@ -351,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;
> @@ -421,11 +426,6 @@ static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)
>  
>  static struct lock_file reflog_lock;
>  
> -enum expire_reflog_flags {
> -	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
> -	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1
> -};
> -
>  static int expire_reflog(const char *refname, const unsigned char *sha1,
>  			 unsigned int flags, void *cb_data)
>  {
> @@ -679,7 +679,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;
> @@ -697,10 +697,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');
>  	}
>  
> @@ -759,7 +759,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
> 

Re: [PATCH 17/23] expire_reflog(): move rewrite to flags argument

[Stefan Beller]

On Fri, Dec 05, 2014 at 12:08:29AM +0100, Michael Haggerty wrote:
> 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 cc7a220..6294406 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,
> @@ -673,7 +673,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"))
> @@ -755,7 +755,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
> 

Re: [PATCH 18/23] Move newlog and last_kept_sha1 to "struct expire_reflog_cb"

[Stefan Beller]

On Fri, Dec 05, 2014 at 12:08:30AM +0100, Michael Haggerty wrote:
> 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 6294406..01b76d0 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);
> @@ -457,8 +457,8 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>  	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
>  		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
>  			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)
>  			goto failure;
>  	}
>  
> @@ -474,7 +474,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
> 

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

[Stefan Beller]

On Fri, Dec 05, 2014 at 12:08:31AM +0100, Michael Haggerty wrote:
> Now that expire_reflog() doesn't actually look in the
> expire_reflog_policy_cb data structure, we can make it opaque:
> 
> * Change its callers to pass it a pointer to an entire "struct
>   expire_reflog_policy_cb".
> 
> * Change it to pass the pointer through as a "void *".
> 
> * Change the policy functions, reflog_expiry_prepare(),
>   reflog_expiry_cleanup(), and should_expire_reflog_ent(), to accept
>   "void *cb_data" arguments and cast them 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 | 73 ++++++++++++++++++++++++++++----------------------------
>  1 file changed, 36 insertions(+), 37 deletions(-)
> 
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 01b76d0..c30936bb 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;
> @@ -427,19 +431,16 @@ static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)
>  static struct lock_file reflog_lock;
>  
>  static int expire_reflog(const char *refname, const unsigned char *sha1,
> -			 unsigned int flags, void *cb_data)
> +			 unsigned int flags, void *policy_cb_data)
>  {
> -	struct cmd_reflog_expire_cb *cmd = cb_data;
>  	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;
>  
>  	/*
>  	 * we take the lock for the ref itself to prevent it from
> @@ -462,11 +463,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>  			goto failure;
>  	}
>  
> -	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)) {
> @@ -639,7 +638,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;
> @@ -653,25 +652,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"))
> @@ -695,11 +694,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');
>  	}
> @@ -712,7 +711,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);
>  		}
> @@ -726,7 +725,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;
> @@ -736,15 +735,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;
>  
> @@ -791,12 +790,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
> 

Re: [PATCH 20/23] reflog_expire(): new function in the reference API

[Stefan Beller]

On Fri, Dec 05, 2014 at 12:08:32AM +0100, Michael Haggerty wrote:
> 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>

With or without the nits fixed
Reviewed-by: Stefan Beller <sbeller@google.com>
as the nits are not degrading functionality.

> ---
>  builtin/reflog.c | 133 +++++++------------------------------------------------
>  refs.c           | 114 +++++++++++++++++++++++++++++++++++++++++++++++
>  refs.h           |  45 +++++++++++++++++++
>  3 files changed, 174 insertions(+), 118 deletions(-)
> 



> +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)

Nit: According to our Codingguidelines we want to indent it further, so it aligns with
the arguments from the first line.

+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 0;

Why do we need the return value for expire_reflog_ent?
The "return 0:" at the very end of the function is the only return I see here.

> +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
> +};

Sometimes we align the assigned numbers and sometimes we don't in git, so an alternative would be

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
}

Do we have a preference in the coding style on this one?




> + *
> + * reflog_expiry_select_fn -- Called once for each entry in the
> + *     existing reflog. It should return true iff that entry should be
> + *     pruned.

Also I know how we got here, I wonder if we should inverse the logic here
(in a later patch). "select" sounds to me as if the line is selected to keep it.
However the opposite is true. To actually select (keep) the line we need to return
0. Would it make sense to rename this to reflog_expiry_should_prune_fn ?

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

[Stefan Beller]

On Fri, Dec 05, 2014 at 12:08:34AM +0100, Michael Haggerty wrote:
> From: Ronnie Sahlberg <sahlberg@google.com>
> 
> 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>

It's originally from Ronnie, but his sign off is missing?

If that sign off is found again, 
Reviewed-by: Stefan Beller <sbeller@google.com>

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

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

[Junio C Hamano]

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

> This isn't documented very well. I thought I saw a comment somewhere in
> the code that stated it explicitly, but I can't find it now. In any
> case, my understanding of the locking protocol for reflogs is:
>
>     The reflog for "$refname", which is stored at
>     "$GIT_DIR/logs/$refname", is locked by holding
>     "$GIT_DIR/refs/$refname.lock", *even if the corresponding
>     reference is packed*.
>
> This implies that readers, who don't pay attention to locks, have to be
> prepared for the possibility that the reflog is in the middle of an
> update and that the last line is incomplete. This is handled by
> show_one_reflog_ent(), which discards incomplete lines.

Interesting, as I think I saw Peff did something around that area
recently.

I have some more thought around the "transaction" in general, but
it will be in a separate message.

Thanks.

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

[Jeff King]

On Tue, Dec 09, 2014 at 10:47:24AM -0800, Junio C Hamano wrote:

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
> > This isn't documented very well. I thought I saw a comment somewhere in
> > the code that stated it explicitly, but I can't find it now. In any
> > case, my understanding of the locking protocol for reflogs is:
> >
> >     The reflog for "$refname", which is stored at
> >     "$GIT_DIR/logs/$refname", is locked by holding
> >     "$GIT_DIR/refs/$refname.lock", *even if the corresponding
> >     reference is packed*.
> >
> > This implies that readers, who don't pay attention to locks, have to be
> > prepared for the possibility that the reflog is in the middle of an
> > update and that the last line is incomplete. This is handled by
> > show_one_reflog_ent(), which discards incomplete lines.
> 
> Interesting, as I think I saw Peff did something around that area
> recently.

Yeah, and I had no idea about the truncated-line race which Michael
described at the time. It makes me glad that I took the time to do the
more careful thing[1] (fixing the reverse-reflog parser to properly
include the newline when present) and not the quick-and-easy thing[2]
(teaching show_one_reflog_ent to accept entries without newlines).

What you have queued in jk/for-each-reflog-ent-reverse should be fine,
even in light of what Michael said.

-Peff

[1] http://article.gmane.org/gmane.comp.version-control.git/260849

[2] http://article.gmane.org/gmane.comp.version-control.git/260807

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

[Michael Haggerty]

On 12/09/2014 12:34 AM, Stefan Beller wrote:
> On Fri, Dec 05, 2014 at 12:08:34AM +0100, Michael Haggerty wrote:
>> From: Ronnie Sahlberg <sahlberg@google.com>
>>
>> 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>
> 
> It's originally from Ronnie, but his sign off is missing?
> 
> If that sign off is found again, 
> Reviewed-by: Stefan Beller <sbeller@google.com>

Sorry for the confusion.

This patch ultimately descends from a patch by Ronnie that completely
deleted lock_any_ref_for_update() because (in the context that he wrote
it) there were no remaining callers of that function.

In the context of this patch, there *is* a remaining caller. So this
patch is different--it deletes the obvious stuff as in Ronnie's patch,
but it also inlines the function at its last caller. And the commit
message is completely different.

Given those differences and the fact that the only overlap with Ronnie's
original patch is the *deletion* of content, I thought it most
appropriate to change the authorship of the patch. So I removed Ronnie's
Signed-off-by line but I forgot to remove his authorship.

If anybody thinks it would be more appropriate to leave Ronnie as author
of the new patch or leave his Signed-off-by line, I am totally OK with
doing so. I'm just trying to figure out what's right.

Otherwise, I will change the authorship of the patch to myself in the
upcoming reroll and include only my own Signed-off-by line.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 20/23] reflog_expire(): new function in the reference API

[Michael Haggerty]

On 12/09/2014 12:32 AM, Stefan Beller wrote:
> On Fri, Dec 05, 2014 at 12:08:32AM +0100, Michael Haggerty wrote:
>> 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>
> 
> With or without the nits fixed
> Reviewed-by: Stefan Beller <sbeller@google.com>
> as the nits are not degrading functionality.
> 
>> ---
>>  builtin/reflog.c | 133 +++++++------------------------------------------------
>>  refs.c           | 114 +++++++++++++++++++++++++++++++++++++++++++++++
>>  refs.h           |  45 +++++++++++++++++++
>>  3 files changed, 174 insertions(+), 118 deletions(-)
>>
> 
> 
> 
>> +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)
> 
> Nit: According to our Codingguidelines we want to indent it further, so it aligns with
> the arguments from the first line.

Will fix.

> +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 0;
> 
> Why do we need the return value for expire_reflog_ent?
> The "return 0:" at the very end of the function is the only return I see here.

expire_reflog_ent() is passed to for_each_reflog_ent() and therefore
must be an each_reflog_ent_fn. If it returns a nonzero value, the
iteration is ended prematurely and the value is returned to the caller
of for_each_reflog_ent(). We don't ever want to end the iteration
prematurely here, so we always return 0.

>> +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
>> +};
> 
> Sometimes we align the assigned numbers and sometimes we don't in git, so an alternative would be
> 
> 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
> }
> 
> Do we have a preference in the coding style on this one?

Both styles are used in our codebase, and I don't think the style guide
says anything about it. My practice in such cases is:

* If I'm modifying existing code, preserve the existing style (to avoid
unnecessary churn)
* If most of our code uses one style, then use that style
* If our code uses both styles frequently, just use whatever style looks
better to me

If and when somebody cares enough to build a consensus for one policy or
the other and to submit a patch to the CodingGuidelines I will be happy
to follow it.

>> + *
>> + * reflog_expiry_select_fn -- Called once for each entry in the
>> + *     existing reflog. It should return true iff that entry should be
>> + *     pruned.
> 
> Also I know how we got here, I wonder if we should inverse the logic here
> (in a later patch). "select" sounds to me as if the line is selected to keep it.
> However the opposite is true. To actually select (keep) the line we need to return
> 0. Would it make sense to rename this to reflog_expiry_should_prune_fn ?

Yes, that would be clearer. I will make the change.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 20/23] reflog_expire(): new function in the reference API

[Jeff King]

On Fri, Dec 12, 2014 at 09:23:05AM +0100, Michael Haggerty wrote:

> On 12/09/2014 12:32 AM, Stefan Beller wrote:
> >> +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
> >> +};
> > 
> > Sometimes we align the assigned numbers and sometimes we don't in git, so an alternative would be
> > 
> > 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
> > }
> > 
> > Do we have a preference in the coding style on this one?

I think vertically aligned lists look really nice. But they often wreak
havoc with diffs, because introducing one longer line means re-aligning
the whole thing. IMHO, it's not worth it (but if you're going to do it,
leave lots of extra room for expansion).

Just my two cents, of course. I don't recall this particular style point
coming up before.

> Both styles are used in our codebase, and I don't think the style guide
> says anything about it. My practice in such cases is:
> 
> * If I'm modifying existing code, preserve the existing style (to avoid
> unnecessary churn)
> * If most of our code uses one style, then use that style
> * If our code uses both styles frequently, just use whatever style looks
> better to me

I think that is a very good philosophy in general.

-Peff

Re: [PATCH 20/23] reflog_expire(): new function in the reference API

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> > 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
>> > }
>> > 
>> > Do we have a preference in the coding style on this one?
>
> I think vertically aligned lists look really nice. But they often wreak
> havoc with diffs, because introducing one longer line means re-aligning
> the whole thing. IMHO, it's not worth it (but if you're going to do it,
> leave lots of extra room for expansion).
>
> Just my two cents, of course. I don't recall this particular style point
> coming up before.
>
>> Both styles are used in our codebase, and I don't think the style guide
>> says anything about it. My practice in such cases is:
>> 
>> * If I'm modifying existing code, preserve the existing style (to avoid
>> unnecessary churn)
>> * If most of our code uses one style, then use that style
>> * If our code uses both styles frequently, just use whatever style looks
>> better to me
>
> I think that is a very good philosophy in general.

Thanks.

About the indentation on the second and subsequent lines of a
logical line that is split into multiple lines, we explicitly say
"Both ar valid, and we use both."  Following the above three-bullet
list would be a good practice for this one, too.