[PATCH 00/12] reflog + gc: refactor, progress output, reftable-readyness

[PATCH 00/12] reflog + gc: refactor, progress output, reftable-readyness

[Ævar Arnfjörð Bjarmason]

This is a follow-up topic I had for my "errno" refs API changes that
just landed[1]. I initially thought I'd split this into multiple
submissions, but I think it's probably better to consider it in
unison.

Comments on individual patches below:

1. https://lore.kernel.org/git/cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (12):
  reflog delete: narrow scope of "cmd" passed to count_reflog_ent()
  reflog expire: narrow scope of "cb" in cmd_reflog_expire()
  reflog: change one->many worktree->refnames to use a string_list
  reflog expire: don't do negative comparison on enum values
  reflog expire: refactor & use "tip_commit" only for UE_NORMAL
  reflog expire: don't use lookup_commit_reference_gently()
  reflog: reduce scope of "struct rev_info"
  refs files-backend: assume cb->newlog if !EXPIRE_REFLOGS_DRY_RUN

This is all refactorings to make the reflog-related code smaller,
easier to read, using string-list instead of a custom home-grown flex
array API etc, and to make it clear what variables and data is used
where. This really helps for subsequent steps.

  reflog + refs-backend: move "verbose" out of the backend

As noted in
https://lore.kernel.org/git/211123.864k83w3y4.gmgdl@evledraar.gmail.com/
this makes the "verbose" API independent, so the reftable backend
we'll have soon won't need to do anything special to support it.

It used to conflict with Han-Wen's topic, but in his v2 he picked
another approach.

  reflog expire: add progress output on --stale-fix
  gc + reflog: emit progress output from "reflog expire --all"
  gc + reflog: don't stall before initial "git gc" progress output

These are all progress additions to "reflog expire" including fixing
the long-standing oddity that on large repos "git gc"'s progress seems
to simply hang before reaching "Enumerating objects".

As noted in 12/12 this is partly a bit of a hack, but I've got other
in-flight progress.c API changes that'll eventually lead to supporting
"nested" progress bars, which this code will be one of the first
consumers of.

 Documentation/git-reflog.txt |   8 ++
 builtin/gc.c                 |   4 +-
 builtin/reflog.c             | 260 ++++++++++++++++++++++-------------
 refs.h                       |   3 +-
 refs/files-backend.c         |  44 +++---
 5 files changed, 194 insertions(+), 125 deletions(-)

-- 
2.34.1.877.g7d5b0a3b8a6

[PATCH 01/12] reflog delete: narrow scope of "cmd" passed to count_reflog_ent()

[Ævar Arnfjörð Bjarmason]

Change the "cb_data" we pass to the count_reflog_ent() to be the
&cb.cmd itself, instead of passing &cb and having the callback lookup
cb->cmd.

This makes it clear that the "cb" itself is the same memzero'd
structure on each iteration of the for-loop that uses &cb, except for
the "cmd" member.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 175c83e7cc2..4c15d71f3e9 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -662,20 +662,18 @@ static int count_reflog_ent(struct object_id *ooid, struct object_id *noid,
 		const char *email, timestamp_t timestamp, int tz,
 		const char *message, void *cb_data)
 {
-	struct expire_reflog_policy_cb *cb = cb_data;
-	if (!cb->cmd.expire_total || timestamp < cb->cmd.expire_total)
-		cb->cmd.recno++;
+	struct cmd_reflog_expire_cb *cb = cb_data;
+	if (!cb->expire_total || timestamp < cb->expire_total)
+		cb->recno++;
 	return 0;
 }
 
 static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 {
-	struct expire_reflog_policy_cb cb;
+	struct cmd_reflog_expire_cb cmd = { 0 };
 	int i, status = 0;
 	unsigned int flags = 0;
 
-	memset(&cb, 0, sizeof(cb));
-
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
@@ -703,6 +701,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 		const char *spec = strstr(argv[i], "@{");
 		char *ep, *ref;
 		int recno;
+		struct expire_reflog_policy_cb cb = { 0 };
 
 		if (!spec) {
 			status |= error(_("not a reflog: %s"), argv[i]);
@@ -716,14 +715,15 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 
 		recno = strtoul(spec + 2, &ep, 10);
 		if (*ep == '}') {
-			cb.cmd.recno = -recno;
-			for_each_reflog_ent(ref, count_reflog_ent, &cb);
+			cmd.recno = -recno;
+			for_each_reflog_ent(ref, count_reflog_ent, &cmd);
 		} else {
-			cb.cmd.expire_total = approxidate(spec + 2);
-			for_each_reflog_ent(ref, count_reflog_ent, &cb);
-			cb.cmd.expire_total = 0;
+			cmd.expire_total = approxidate(spec + 2);
+			for_each_reflog_ent(ref, count_reflog_ent, &cmd);
+			cmd.expire_total = 0;
 		}
 
+		cb.cmd = cmd;
 		status |= reflog_expire(ref, flags,
 					reflog_expiry_prepare,
 					should_expire_reflog_ent,
-- 
2.34.1.877.g7d5b0a3b8a6

[PATCH 02/12] reflog expire: narrow scope of "cb" in cmd_reflog_expire()

[Ævar Arnfjörð Bjarmason]

As with the preceding change for "reflog delete", change the "cb_data"
we pass to callbacks to be &cb.cmd itself, instead of passing &cb and
having the callback lookup cb->cmd.

This makes it clear that the "cb" itself is the same memzero'd
structure on each iteration of the for-loops that use &cb, except for
the "cmd" member.

The "struct expire_reflog_policy_cb" we pass to reflog_expire() will
have the members that aren't "cmd" modified by the callbacks, but
before we invoke them everything except "cmd" is zero'd out.

This included the "tip_commit", "mark_list" and "tips". It might have
looked as though we were re-using those between iterations, but the
first thing we did in reflog_expiry_prepare() was to either NULL them,
or clobber them with another value.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 4c15d71f3e9..6989492bf5c 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -357,7 +357,6 @@ static void reflog_expiry_prepare(const char *refname,
 	struct expire_reflog_policy_cb *cb = cb_data;
 
 	if (!cb->cmd.expire_unreachable || is_head(refname)) {
-		cb->tip_commit = NULL;
 		cb->unreachable_expire_kind = UE_HEAD;
 	} else {
 		cb->tip_commit = lookup_commit_reference_gently(the_repository,
@@ -371,8 +370,6 @@ static void reflog_expiry_prepare(const char *refname,
 	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;
@@ -541,7 +538,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 expire_reflog_policy_cb cb;
+	struct cmd_reflog_expire_cb cmd = { 0 };
 	timestamp_t now = time(NULL);
 	int i, status, do_all, all_worktrees = 1;
 	int explicit_expiry = 0;
@@ -553,10 +550,9 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 
 	save_commit_buffer = 0;
 	do_all = status = 0;
-	memset(&cb, 0, sizeof(cb));
 
-	cb.cmd.expire_total = default_reflog_expire;
-	cb.cmd.expire_unreachable = default_reflog_expire_unreachable;
+	cmd.expire_total = default_reflog_expire;
+	cmd.expire_unreachable = default_reflog_expire_unreachable;
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
@@ -564,17 +560,17 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
 			flags |= EXPIRE_REFLOGS_DRY_RUN;
 		else if (skip_prefix(arg, "--expire=", &arg)) {
-			if (parse_expiry_date(arg, &cb.cmd.expire_total))
+			if (parse_expiry_date(arg, &cmd.expire_total))
 				die(_("'%s' is not a valid timestamp"), arg);
 			explicit_expiry |= EXPIRE_TOTAL;
 		}
 		else if (skip_prefix(arg, "--expire-unreachable=", &arg)) {
-			if (parse_expiry_date(arg, &cb.cmd.expire_unreachable))
+			if (parse_expiry_date(arg, &cmd.expire_unreachable))
 				die(_("'%s' is not a valid timestamp"), arg);
 			explicit_expiry |= EXPIRE_UNREACH;
 		}
 		else if (!strcmp(arg, "--stale-fix"))
-			cb.cmd.stalefix = 1;
+			cmd.stalefix = 1;
 		else if (!strcmp(arg, "--rewrite"))
 			flags |= EXPIRE_REFLOGS_REWRITE;
 		else if (!strcmp(arg, "--updateref"))
@@ -600,14 +596,14 @@ 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.cmd.stalefix) {
-		repo_init_revisions(the_repository, &cb.cmd.revs, prefix);
-		cb.cmd.revs.do_not_die_on_missing_tree = 1;
-		cb.cmd.revs.ignore_missing = 1;
-		cb.cmd.revs.ignore_missing_links = 1;
+	if (cmd.stalefix) {
+		repo_init_revisions(the_repository, &cmd.revs, prefix);
+		cmd.revs.do_not_die_on_missing_tree = 1;
+		cmd.revs.ignore_missing = 1;
+		cmd.revs.ignore_missing_links = 1;
 		if (flags & EXPIRE_REFLOGS_VERBOSE)
 			printf(_("Marking reachable objects..."));
-		mark_reachable_objects(&cb.cmd.revs, 0, 0, NULL);
+		mark_reachable_objects(&cmd.revs, 0, 0, NULL);
 		if (flags & EXPIRE_REFLOGS_VERBOSE)
 			putchar('\n');
 	}
@@ -629,6 +625,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		free_worktrees(worktrees);
 		for (i = 0; i < collected.nr; i++) {
 			struct collected_reflog *e = collected.e[i];
+			struct expire_reflog_policy_cb cb = { .cmd = cmd };
 
 			set_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);
 			status |= reflog_expire(e->reflog, flags,
@@ -643,6 +640,8 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 
 	for (; i < argc; i++) {
 		char *ref;
+		struct expire_reflog_policy_cb cb = { .cmd = cmd };
+
 		if (!dwim_log(argv[i], strlen(argv[i]), NULL, &ref)) {
 			status |= error(_("%s points nowhere!"), argv[i]);
 			continue;
-- 
2.34.1.877.g7d5b0a3b8a6

[PATCH 03/12] reflog: change one->many worktree->refnames to use a string_list

[Ævar Arnfjörð Bjarmason]

Change the FLEX_ARRAY pattern added in bda3a31cc79 (reflog-expire:
Avoid creating new files in a directory inside readdir(3) loop,
2008-01-25) the string-list API instead.

This does not change any behavior, allows us to delete much of this
code as it's replaced by things we get from the string-list API for
free, as a result we need just one struct to keep track of this data,
instead of two.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 48 +++++++++++++++++++-----------------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 6989492bf5c..0fb46ade19f 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -48,16 +48,9 @@ struct expire_reflog_policy_cb {
 	struct commit_list *tips;
 };
 
-struct collected_reflog {
-	struct object_id oid;
-	char reflog[FLEX_ARRAY];
-};
-
-struct collect_reflog_cb {
-	struct collected_reflog **e;
-	int alloc;
-	int nr;
-	struct worktree *wt;
+struct worktree_reflogs {
+	struct worktree *worktree;
+	struct string_list reflogs;
 };
 
 /* Remember to update object flag allocation in object.h */
@@ -403,24 +396,20 @@ static void reflog_expiry_cleanup(void *cb_data)
 
 static int collect_reflog(const char *ref, const struct object_id *oid, int unused, void *cb_data)
 {
-	struct collected_reflog *e;
-	struct collect_reflog_cb *cb = cb_data;
+	struct worktree_reflogs *cb = cb_data;
+	struct worktree *worktree = cb->worktree;
 	struct strbuf newref = STRBUF_INIT;
 
 	/*
 	 * Avoid collecting the same shared ref multiple times because
 	 * they are available via all worktrees.
 	 */
-	if (!cb->wt->is_current && ref_type(ref) == REF_TYPE_NORMAL)
+	if (!worktree->is_current && ref_type(ref) == REF_TYPE_NORMAL)
 		return 0;
 
-	strbuf_worktree_ref(cb->wt, &newref, ref);
-	FLEX_ALLOC_STR(e, reflog, newref.buf);
-	strbuf_release(&newref);
+	strbuf_worktree_ref(worktree, &newref, ref);
+	string_list_append(&cb->reflogs, strbuf_detach(&newref, NULL));
 
-	oidcpy(&e->oid, oid);
-	ALLOC_GROW(cb->e, cb->nr + 1, cb->alloc);
-	cb->e[cb->nr++] = e;
 	return 0;
 }
 
@@ -609,33 +598,34 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	}
 
 	if (do_all) {
-		struct collect_reflog_cb collected;
+		struct worktree_reflogs collected = {
+			.reflogs = STRING_LIST_INIT_NODUP,
+		};
+		struct string_list_item *item;
 		struct worktree **worktrees, **p;
-		int i;
 
-		memset(&collected, 0, sizeof(collected));
 		worktrees = get_worktrees();
 		for (p = worktrees; *p; p++) {
 			if (!all_worktrees && !(*p)->is_current)
 				continue;
-			collected.wt = *p;
+			collected.worktree = *p;
 			refs_for_each_reflog(get_worktree_ref_store(*p),
 					     collect_reflog, &collected);
 		}
 		free_worktrees(worktrees);
-		for (i = 0; i < collected.nr; i++) {
-			struct collected_reflog *e = collected.e[i];
+
+		for_each_string_list_item(item, &collected.reflogs) {
 			struct expire_reflog_policy_cb cb = { .cmd = cmd };
 
-			set_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);
-			status |= reflog_expire(e->reflog, flags,
+			set_reflog_expiry_param(&cb.cmd, explicit_expiry, item->string);
+			status |= reflog_expire(item->string, flags,
 						reflog_expiry_prepare,
 						should_expire_reflog_ent,
 						reflog_expiry_cleanup,
 						&cb);
-			free(e);
 		}
-		free(collected.e);
+		collected.reflogs.strdup_strings = 1;
+		string_list_clear(&collected.reflogs, 0);
 	}
 
 	for (; i < argc; i++) {
-- 
2.34.1.877.g7d5b0a3b8a6

[PATCH 04/12] reflog expire: don't do negative comparison on enum values

[Ævar Arnfjörð Bjarmason]

Change code added in 03cb91b18cc (reflog --expire-unreachable: special
case entries in "HEAD" reflog, 2010-04-09) to not do positive instead
of negative comparisons on enum values, i.e. not to assume that "x !=
UE_ALWAYS" means "(x == UE_HEAD || x || UE_NORMAL)".

That assumption is true now, but we'd introduce subtle bugs here if
that were to change, now the compiler will notice and error out on
such errors.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 57 ++++++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 24 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 0fb46ade19f..f8a24f1aa26 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -303,10 +303,15 @@ static int should_expire_reflog_ent(struct object_id *ooid, struct object_id *no
 		return 1;
 
 	if (timestamp < cb->cmd.expire_unreachable) {
-		if (cb->unreachable_expire_kind == UE_ALWAYS)
-			return 1;
-		if (unreachable(cb, old_commit, ooid) || unreachable(cb, new_commit, noid))
+		switch (cb->unreachable_expire_kind) {
+		case UE_ALWAYS:
 			return 1;
+		case UE_NORMAL:
+		case UE_HEAD:
+			if (unreachable(cb, old_commit, ooid) || unreachable(cb, new_commit, noid))
+				return 1;
+			break;
+		}
 	}
 
 	if (cb->cmd.recno && --(cb->cmd.recno) == 0)
@@ -348,6 +353,7 @@ static void reflog_expiry_prepare(const char *refname,
 				  void *cb_data)
 {
 	struct expire_reflog_policy_cb *cb = cb_data;
+	struct commit_list *elem;
 
 	if (!cb->cmd.expire_unreachable || is_head(refname)) {
 		cb->unreachable_expire_kind = UE_HEAD;
@@ -363,34 +369,37 @@ static void reflog_expiry_prepare(const char *refname,
 	if (cb->cmd.expire_unreachable <= cb->cmd.expire_total)
 		cb->unreachable_expire_kind = UE_ALWAYS;
 
-	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);
+	switch (cb->unreachable_expire_kind) {
+	case UE_ALWAYS:
+		return;
+	case UE_HEAD:
+		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);
+		break;
+	case UE_NORMAL:
+		commit_list_insert(cb->tip_commit, &cb->mark_list);
 	}
+	cb->mark_limit = cb->cmd.expire_total;
+	mark_reachable(cb);
 }
 
 static void reflog_expiry_cleanup(void *cb_data)
 {
 	struct expire_reflog_policy_cb *cb = cb_data;
+	struct commit_list *elem;
 
-	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);
-		}
+	switch (cb->unreachable_expire_kind) {
+	case UE_ALWAYS:
+		return;
+	case UE_HEAD:
+		for (elem = cb->tips; elem; elem = elem->next)
+			clear_commit_marks(elem->item, REACHABLE);
+		free_commit_list(cb->tips);
+		break;
+	case UE_NORMAL:
+		clear_commit_marks(cb->tip_commit, REACHABLE);
+		break;
 	}
 }
 
-- 
2.34.1.877.g7d5b0a3b8a6

[PATCH 05/12] reflog expire: refactor & use "tip_commit" only for UE_NORMAL

[Ævar Arnfjörð Bjarmason]

Add an intermediate variable for "tip_commit" in
reflog_expiry_prepare(), and only add it to the struct if we're
handling the UE_NORMAL case.

The code behaves the same way as before, but this makes the control
flow clearer, and the shorter name allows us to fold a 4-line i/else
int a one-line terany instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index f8a24f1aa26..ec0c6051135 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -354,16 +354,14 @@ static void reflog_expiry_prepare(const char *refname,
 {
 	struct expire_reflog_policy_cb *cb = cb_data;
 	struct commit_list *elem;
+	struct commit *commit = NULL;
 
 	if (!cb->cmd.expire_unreachable || is_head(refname)) {
 		cb->unreachable_expire_kind = UE_HEAD;
 	} else {
-		cb->tip_commit = lookup_commit_reference_gently(the_repository,
-								oid, 1);
-		if (!cb->tip_commit)
-			cb->unreachable_expire_kind = UE_ALWAYS;
-		else
-			cb->unreachable_expire_kind = UE_NORMAL;
+		commit = lookup_commit_reference_gently(the_repository,
+							oid, 1);
+		cb->unreachable_expire_kind = commit ? UE_NORMAL : UE_ALWAYS;
 	}
 
 	if (cb->cmd.expire_unreachable <= cb->cmd.expire_total)
@@ -378,7 +376,9 @@ static void reflog_expiry_prepare(const char *refname,
 			commit_list_insert(elem->item, &cb->mark_list);
 		break;
 	case UE_NORMAL:
-		commit_list_insert(cb->tip_commit, &cb->mark_list);
+		commit_list_insert(commit, &cb->mark_list);
+		/* For reflog_expiry_cleanup() below */
+		cb->tip_commit = commit;
 	}
 	cb->mark_limit = cb->cmd.expire_total;
 	mark_reachable(cb);
-- 
2.34.1.877.g7d5b0a3b8a6

[PATCH 06/12] reflog expire: don't use lookup_commit_reference_gently()

[Ævar Arnfjörð Bjarmason]

In the initial implementation of "git reflog" in 4264dc15e19 (git
reflog expire, 2006-12-19) we had this
lookup_commit_reference_gently().

I don't think we've ever found tags that we need to recursively
dereference in reflogs, so this should at least be changed to a
"lookup commit" as I'm doing here, although I can't think of a way
where it mattered in practice.

I also think we'd probably like to just die here if we have a NULL
object, but as this code needs to handle potentially broken
repositories let's just show an "error" but continue, the non-quiet
lookup_commit() will do for us. None of our tests cover the case where
"commit" is NULL after this lookup.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index ec0c6051135..29dcd91abca 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -359,8 +359,7 @@ static void reflog_expiry_prepare(const char *refname,
 	if (!cb->cmd.expire_unreachable || is_head(refname)) {
 		cb->unreachable_expire_kind = UE_HEAD;
 	} else {
-		commit = lookup_commit_reference_gently(the_repository,
-							oid, 1);
+		commit = lookup_commit(the_repository, oid);
 		cb->unreachable_expire_kind = commit ? UE_NORMAL : UE_ALWAYS;
 	}
 
-- 
2.34.1.877.g7d5b0a3b8a6

[PATCH 07/12] reflog: reduce scope of "struct rev_info"

[Ævar Arnfjörð Bjarmason]

Change the "cmd.stalefix" handling added in 1389d9ddaa6 (reflog expire
--fix-stale, 2007-01-06) to use a locally scoped "struct
rev_info". This code relies on mark_reachable_objects() twiddling
flags in the walked objects.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 29dcd91abca..48e4f5887b0 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -28,7 +28,6 @@ static timestamp_t default_reflog_expire;
 static timestamp_t default_reflog_expire_unreachable;
 
 struct cmd_reflog_expire_cb {
-	struct rev_info revs;
 	int stalefix;
 	timestamp_t expire_total;
 	timestamp_t expire_unreachable;
@@ -594,13 +593,15 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	 * from reflog if the repository was pruned with older git.
 	 */
 	if (cmd.stalefix) {
-		repo_init_revisions(the_repository, &cmd.revs, prefix);
-		cmd.revs.do_not_die_on_missing_tree = 1;
-		cmd.revs.ignore_missing = 1;
-		cmd.revs.ignore_missing_links = 1;
+		struct rev_info revs;
+
+		repo_init_revisions(the_repository, &revs, prefix);
+		revs.do_not_die_on_missing_tree = 1;
+		revs.ignore_missing = 1;
+		revs.ignore_missing_links = 1;
 		if (flags & EXPIRE_REFLOGS_VERBOSE)
 			printf(_("Marking reachable objects..."));
-		mark_reachable_objects(&cmd.revs, 0, 0, NULL);
+		mark_reachable_objects(&revs, 0, 0, NULL);
 		if (flags & EXPIRE_REFLOGS_VERBOSE)
 			putchar('\n');
 	}
-- 
2.34.1.877.g7d5b0a3b8a6

[PATCH 08/12] refs files-backend: assume cb->newlog if !EXPIRE_REFLOGS_DRY_RUN

[Ævar Arnfjörð Bjarmason]

It's not possible for "cb->newlog" to be NULL if
!EXPIRE_REFLOGS_DRY_RUN, since files_reflog_expire() would have
error()'d and taken the "goto failure" branch if it couldn't open the
file. By not using the "newlog" field private to "file-backend.c"'s
"struct expire_reflog_cb", we can move this verbosity logging to
"builtin/reflog.c" in a subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4b14f30d48f..451c4e2a052 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3099,12 +3099,12 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
 
 	if ((*cb->should_prune_fn)(ooid, noid, email, timestamp, tz,
 				   message, policy_cb)) {
-		if (!cb->newlog)
+		if (cb->flags & EXPIRE_REFLOGS_DRY_RUN)
 			printf("would prune %s", message);
 		else if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
 			printf("prune %s", message);
 	} else {
-		if (cb->newlog) {
+		if (!(cb->flags & EXPIRE_REFLOGS_DRY_RUN)) {
 			fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s",
 				oid_to_hex(ooid), oid_to_hex(noid),
 				email, timestamp, tz, message);
-- 
2.34.1.877.g7d5b0a3b8a6

[PATCH 09/12] reflog + refs-backend: move "verbose" out of the backend

[Ævar Arnfjörð Bjarmason]

Move the handling of the "verbose" flag entirely out of
"refs/files-backend.c" and into "builtin/reflog.c". This allows the
backend to stop knowing about the EXPIRE_REFLOGS_VERBOSE flag.

The expire_reflog_ent() function shouldn't need to deal with the
implementation detail of whether or not we're emitting verbose output,
by doing this the --verbose output becomes backend-agnostic, so
reftable will get the same output.

I think the output is rather bad currently, and should e.g. be
implemented with some better future mode of progress.[ch], but that's
a topic for another improvement.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c     | 42 +++++++++++++++++++++++++++++++-----------
 refs.h               |  3 +--
 refs/files-backend.c | 44 ++++++++++++++++++++------------------------
 3 files changed, 52 insertions(+), 37 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 48e4f5887b0..a77c0d96dce 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -45,6 +45,8 @@ struct expire_reflog_policy_cb {
 	struct cmd_reflog_expire_cb cmd;
 	struct commit *tip_commit;
 	struct commit_list *tips;
+	unsigned int dry_run:1,
+		     verbose:1;
 };
 
 struct worktree_reflogs {
@@ -294,29 +296,38 @@ static int should_expire_reflog_ent(struct object_id *ooid, struct object_id *no
 	struct commit *old_commit, *new_commit;
 
 	if (timestamp < cb->cmd.expire_total)
-		return 1;
+		goto expire;
 
 	old_commit = new_commit = NULL;
 	if (cb->cmd.stalefix &&
 	    (!keep_entry(&old_commit, ooid) || !keep_entry(&new_commit, noid)))
-		return 1;
+		goto expire;
 
 	if (timestamp < cb->cmd.expire_unreachable) {
 		switch (cb->unreachable_expire_kind) {
 		case UE_ALWAYS:
-			return 1;
+			goto expire;
 		case UE_NORMAL:
 		case UE_HEAD:
 			if (unreachable(cb, old_commit, ooid) || unreachable(cb, new_commit, noid))
-				return 1;
+				goto expire;
 			break;
 		}
 	}
 
 	if (cb->cmd.recno && --(cb->cmd.recno) == 0)
-		return 1;
+		goto expire;
+
+	if (cb->verbose)
+		printf("keep %s", message);
 
 	return 0;
+expire:
+	if (cb->dry_run)
+		printf("would prune %s", message);
+	else if (cb->verbose)
+		printf("prune %s", message);
+	return 1;
 }
 
 static int push_tip_to_list(const char *refname, const struct object_id *oid,
@@ -539,6 +550,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	int i, status, do_all, all_worktrees = 1;
 	int explicit_expiry = 0;
 	unsigned int flags = 0;
+	int verbose = 0;
 
 	default_reflog_expire_unreachable = now - 30 * 24 * 3600;
 	default_reflog_expire = now - 90 * 24 * 3600;
@@ -576,7 +588,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		else if (!strcmp(arg, "--single-worktree"))
 			all_worktrees = 0;
 		else if (!strcmp(arg, "--verbose"))
-			flags |= EXPIRE_REFLOGS_VERBOSE;
+			verbose = 1;
 		else if (!strcmp(arg, "--")) {
 			i++;
 			break;
@@ -599,10 +611,10 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		revs.do_not_die_on_missing_tree = 1;
 		revs.ignore_missing = 1;
 		revs.ignore_missing_links = 1;
-		if (flags & EXPIRE_REFLOGS_VERBOSE)
+		if (verbose)
 			printf(_("Marking reachable objects..."));
 		mark_reachable_objects(&revs, 0, 0, NULL);
-		if (flags & EXPIRE_REFLOGS_VERBOSE)
+		if (verbose)
 			putchar('\n');
 	}
 
@@ -624,7 +636,11 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		free_worktrees(worktrees);
 
 		for_each_string_list_item(item, &collected.reflogs) {
-			struct expire_reflog_policy_cb cb = { .cmd = cmd };
+			struct expire_reflog_policy_cb cb = {
+				.cmd = cmd,
+				.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
+				.verbose = verbose,
+			};
 
 			set_reflog_expiry_param(&cb.cmd, explicit_expiry, item->string);
 			status |= reflog_expire(item->string, flags,
@@ -671,6 +687,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 	struct cmd_reflog_expire_cb cmd = { 0 };
 	int i, status = 0;
 	unsigned int flags = 0;
+	int verbose = 0;
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
@@ -681,7 +698,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"))
-			flags |= EXPIRE_REFLOGS_VERBOSE;
+			verbose = 1;
 		else if (!strcmp(arg, "--")) {
 			i++;
 			break;
@@ -699,7 +716,10 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 		const char *spec = strstr(argv[i], "@{");
 		char *ep, *ref;
 		int recno;
-		struct expire_reflog_policy_cb cb = { 0 };
+		struct expire_reflog_policy_cb cb = {
+			.verbose = verbose,
+			.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
+		};
 
 		if (!spec) {
 			status |= error(_("not a reflog: %s"), argv[i]);
diff --git a/refs.h b/refs.h
index 45c34e99e3a..0c3374b405c 100644
--- a/refs.h
+++ b/refs.h
@@ -786,8 +786,7 @@ enum ref_type ref_type(const char *refname);
 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
+	EXPIRE_REFLOGS_REWRITE = 1 << 2,
 };
 
 /*
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 451c4e2a052..c154c3c4a23 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3080,11 +3080,12 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 }
 
 struct expire_reflog_cb {
-	unsigned int flags;
 	reflog_expiry_should_prune_fn *should_prune_fn;
 	void *policy_cb;
 	FILE *newlog;
 	struct object_id last_kept_oid;
+	unsigned int rewrite:1,
+		     dry_run:1;
 };
 
 static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
@@ -3092,33 +3093,27 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
 			     const char *message, void *cb_data)
 {
 	struct expire_reflog_cb *cb = cb_data;
-	struct expire_reflog_policy_cb *policy_cb = cb->policy_cb;
+	reflog_expiry_should_prune_fn *fn = cb->should_prune_fn;
 
-	if (cb->flags & EXPIRE_REFLOGS_REWRITE)
+	if (cb->rewrite)
 		ooid = &cb->last_kept_oid;
 
-	if ((*cb->should_prune_fn)(ooid, noid, email, timestamp, tz,
-				   message, policy_cb)) {
-		if (cb->flags & EXPIRE_REFLOGS_DRY_RUN)
-			printf("would prune %s", message);
-		else if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
-			printf("prune %s", message);
-	} else {
-		if (!(cb->flags & EXPIRE_REFLOGS_DRY_RUN)) {
-			fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s",
-				oid_to_hex(ooid), oid_to_hex(noid),
-				email, timestamp, tz, message);
-			oidcpy(&cb->last_kept_oid, noid);
-		}
-		if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
-			printf("keep %s", message);
-	}
+	if (fn(ooid, noid, email, timestamp, tz, message, cb->policy_cb))
+		return 0;
+
+	if (cb->dry_run)
+		return 0; /* --dry-run */
+
+	fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s", oid_to_hex(ooid),
+		oid_to_hex(noid), email, timestamp, tz, message);
+	oidcpy(&cb->last_kept_oid, noid);
+
 	return 0;
 }
 
 static int files_reflog_expire(struct ref_store *ref_store,
 			       const char *refname,
-			       unsigned int flags,
+			       unsigned int expire_flags,
 			       reflog_expiry_prepare_fn prepare_fn,
 			       reflog_expiry_should_prune_fn should_prune_fn,
 			       reflog_expiry_cleanup_fn cleanup_fn,
@@ -3136,7 +3131,8 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	const struct object_id *oid;
 
 	memset(&cb, 0, sizeof(cb));
-	cb.flags = flags;
+	cb.rewrite = !!(expire_flags & EXPIRE_REFLOGS_REWRITE);
+	cb.dry_run = !!(expire_flags & EXPIRE_REFLOGS_DRY_RUN);
 	cb.policy_cb = policy_cb_data;
 	cb.should_prune_fn = should_prune_fn;
 
@@ -3172,7 +3168,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 
 	files_reflog_path(refs, &log_file_sb, refname);
 	log_file = strbuf_detach(&log_file_sb, NULL);
-	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
+	if (!cb.dry_run) {
 		/*
 		 * Even though holding $GIT_DIR/logs/$reflog.lock has
 		 * no locking implications, we use the lock_file
@@ -3199,7 +3195,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	refs_for_each_reflog_ent(ref_store, refname, expire_reflog_ent, &cb);
 	(*cleanup_fn)(cb.policy_cb);
 
-	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
+	if (!cb.dry_run) {
 		/*
 		 * It doesn't make sense to adjust a reference pointed
 		 * to by a symbolic ref based on expiring entries in
@@ -3209,7 +3205,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 		 */
 		int update = 0;
 
-		if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
+		if ((expire_flags & EXPIRE_REFLOGS_UPDATE_REF) &&
 		    !is_null_oid(&cb.last_kept_oid)) {
 			int ignore_errno;
 			int type;
-- 
2.34.1.877.g7d5b0a3b8a6

[PATCH 10/12] reflog expire: add progress output on --stale-fix

[Ævar Arnfjörð Bjarmason]

Add progress output when the "git reflog expire --stale-fix" option is
used. This output was previously only emitted under --verbose, but we
shouldn't treat it the same way as the actually verbose "--verbose"
output emitted in should_expire_reflog_ent().

Note that this code isn't going to be affected by the sort of bug we
had to fix in 6b89a34c89f (gc: fix regression in 7b0f229222 impacting
--quiet, 2018-09-19). I.e. "git gc" won't call it with the
"--stale-fix" flag, that option is purely used as a one-off.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-reflog.txt |  8 ++++++++
 builtin/reflog.c             | 18 +++++++++++++-----
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
index ff487ff77d3..1735bbea9fb 100644
--- a/Documentation/git-reflog.txt
+++ b/Documentation/git-reflog.txt
@@ -69,6 +69,14 @@ Options for `show`
 Options for `expire`
 ~~~~~~~~~~~~~~~~~~~~
 
+--progress::
+--no-progress::
+	Progress status is reported on the standard error stream by
+	default when it is attached to a terminal. The `--progress
+	flag enables progress reporting even if not attached to a
+	terminal. Supplying `--no-progress` will suppress all progress
+	output.
+
 --all::
 	Process the reflogs of all references.
 
diff --git a/builtin/reflog.c b/builtin/reflog.c
index a77c0d96dce..cf0ef68d82d 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -11,6 +11,7 @@
 #include "revision.h"
 #include "reachable.h"
 #include "worktree.h"
+#include "progress.h"
 
 /* NEEDSWORK: switch to using parse_options */
 static const char reflog_expire_usage[] =
@@ -551,6 +552,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	int explicit_expiry = 0;
 	unsigned int flags = 0;
 	int verbose = 0;
+	int show_progress = -1;
 
 	default_reflog_expire_unreachable = now - 30 * 24 * 3600;
 	default_reflog_expire = now - 90 * 24 * 3600;
@@ -579,6 +581,10 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		}
 		else if (!strcmp(arg, "--stale-fix"))
 			cmd.stalefix = 1;
+		else if (!strcmp(arg, "--progress"))
+			show_progress = 1;
+		else if (!strcmp(arg, "--no-progress"))
+			show_progress = 0;
 		else if (!strcmp(arg, "--rewrite"))
 			flags |= EXPIRE_REFLOGS_REWRITE;
 		else if (!strcmp(arg, "--updateref"))
@@ -598,6 +604,8 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		else
 			break;
 	}
+	if (show_progress == -1)
+		show_progress = isatty(2);
 
 	/*
 	 * We can trust the commits and objects reachable from refs
@@ -606,16 +614,16 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	 */
 	if (cmd.stalefix) {
 		struct rev_info revs;
+		struct progress *progress = NULL;
 
+		if (show_progress)
+			progress = start_delayed_progress(_("Marking reachable objects"), 0);
 		repo_init_revisions(the_repository, &revs, prefix);
 		revs.do_not_die_on_missing_tree = 1;
 		revs.ignore_missing = 1;
 		revs.ignore_missing_links = 1;
-		if (verbose)
-			printf(_("Marking reachable objects..."));
-		mark_reachable_objects(&revs, 0, 0, NULL);
-		if (verbose)
-			putchar('\n');
+		mark_reachable_objects(&revs, 0, 0, progress);
+		stop_progress(&progress);
 	}
 
 	if (do_all) {
-- 
2.34.1.877.g7d5b0a3b8a6

[PATCH 11/12] gc + reflog: emit progress output from "reflog expire --all"

[Ævar Arnfjörð Bjarmason]

Emit progress output on "git reflog expire --all", which is what "git
gc" runs. On my git.git checkout I'll now get:

    $ time GIT_PROGRESS_DELAY=0 ~/g/git/git reflog expire --all --dry-run
    Enumerating reflogs: 23782, done.
    Expiring reflogs: 100% (23782/23782), done.

    real    0m3.264s
    user    0m2.308s
    sys     0m0.941s

The "Enumerating reflogs" is too fast to appear for me except with
GIT_PROGRESS_DELAY=0. We'll also emit this at the top of "git gc"
output:

    $ ~/g/git/git --exec-path=$PWD gc
    Expiring reflogs: 100% (23782/23782), done.
    Enumerating objects: [...]

This goes a long way (but not quite, a subsequent commit will) to
addressing the seeming halting of "git gc" on startup. That usually
happens because of the "HEAD" case in "reflog expire --all" and
unreachable().

Note that this code isn't going to be affected by the sort of bug we
had to fix in 6b89a34c89f (gc: fix regression in 7b0f229222 impacting
--quiet, 2018-09-19).

This is because "git gc" even with "--auto" won't detach until after
it runs "git reflog -expire", so whatever output we emit will never
end up in the gc.log. We should still obey its --quiet to mean our
--no-progress, but we don't need a special-case for "daemonized" as
write_commit_graph_reachable() does.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c     |  4 +++-
 builtin/reflog.c | 19 ++++++++++++++++++-
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index bcef6a4c8da..872209e083e 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -593,8 +593,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		if (aggressive_window > 0)
 			strvec_pushf(&repack, "--window=%d", aggressive_window);
 	}
-	if (quiet)
+	if (quiet) {
+		strvec_push(&reflog, "--no-progress");
 		strvec_push(&repack, "-q");
+	}
 
 	if (auto_gc) {
 		/*
diff --git a/builtin/reflog.c b/builtin/reflog.c
index cf0ef68d82d..1a2a210ecf1 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -53,6 +53,7 @@ struct expire_reflog_policy_cb {
 struct worktree_reflogs {
 	struct worktree *worktree;
 	struct string_list reflogs;
+	struct progress *progress;
 };
 
 /* Remember to update object flag allocation in object.h */
@@ -324,7 +325,7 @@ static int should_expire_reflog_ent(struct object_id *ooid, struct object_id *no
 
 	return 0;
 expire:
-	if (cb->dry_run)
+	if (cb->verbose && cb->dry_run)
 		printf("would prune %s", message);
 	else if (cb->verbose)
 		printf("prune %s", message);
@@ -426,6 +427,7 @@ static int collect_reflog(const char *ref, const struct object_id *oid, int unus
 	if (!worktree->is_current && ref_type(ref) == REF_TYPE_NORMAL)
 		return 0;
 
+	display_progress(cb->progress, cb->reflogs.nr + 1);
 	strbuf_worktree_ref(worktree, &newref, ref);
 	string_list_append(&cb->reflogs, strbuf_detach(&newref, NULL));
 
@@ -627,11 +629,17 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	}
 
 	if (do_all) {
+		struct progress *progress = NULL;
 		struct worktree_reflogs collected = {
 			.reflogs = STRING_LIST_INIT_NODUP,
 		};
 		struct string_list_item *item;
 		struct worktree **worktrees, **p;
+		uint64_t progress_cnt;
+
+		if (show_progress)
+			collected.progress = start_delayed_progress(_("Enumerating reflogs"),
+								    0);
 
 		worktrees = get_worktrees();
 		for (p = worktrees; *p; p++) {
@@ -642,6 +650,13 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 					     collect_reflog, &collected);
 		}
 		free_worktrees(worktrees);
+		stop_progress(&collected.progress);
+
+		if (show_progress) {
+			progress_cnt = 0;
+			progress = start_delayed_progress(_("Expiring reflogs"),
+							  collected.reflogs.nr);
+		}
 
 		for_each_string_list_item(item, &collected.reflogs) {
 			struct expire_reflog_policy_cb cb = {
@@ -650,6 +665,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 				.verbose = verbose,
 			};
 
+			display_progress(progress, ++progress_cnt);
 			set_reflog_expiry_param(&cb.cmd, explicit_expiry, item->string);
 			status |= reflog_expire(item->string, flags,
 						reflog_expiry_prepare,
@@ -657,6 +673,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 						reflog_expiry_cleanup,
 						&cb);
 		}
+		stop_progress(&progress);
 		collected.reflogs.strdup_strings = 1;
 		string_list_clear(&collected.reflogs, 0);
 	}
-- 
2.34.1.877.g7d5b0a3b8a6

[PATCH 12/12] gc + reflog: don't stall before initial "git gc" progress output

[Ævar Arnfjörð Bjarmason]

Change the "git reflog expire --all" progress output to show progress
on mark_reachable() in addition to the "Expiring reflogs" output added
in the preceding commit.

This is a major UI improvement to the "git gc" progress output, which
in large repositories would seem to hang before the "initial"
"Enumerating objects" phase. E.g. on a linux.git checkout I've got it
previously took me around 10 seconds before I saw any output.

With the change to add the "Expiring reflogs" output in the preceding
commit that ~10 second delay was perhaps ~8-9, now it's pretty much
guaranteed to show output within the first couple of seconds (our
default progress delay).

Why? Because as we iterate through our reflogs to expire in
reflog_exire() we'd always check the reflog for "HEAD" first, and as
we hadn't previously marked anything REACHABLE in mark_reachable()
would almost certainly end up needing to walk a substantial amount of
history to mark commits. In linux.git north of 1 million commits.

Now we'll instead show:

    $ git gc
    Iterating (un)reachable HEAD: 1137354, done.
    Expiring reflogs: 100% (45/45), done.
    ^Enmerating objects: 662499^C
    [...]

The implementation is a bit of a hack. Ideally we'd support nested
progress bars, and the ability to "attach" a sub-progress bar to a
running one. I.e. to optimistically show either:

    Expiring reflogs: X (Y/Z)

Or:

    Expiring reflogs: X (Y/Z) iterating unreachable HEAD: 123456

In this case the problem is that if we have done our initial big
"REACHABLE" iteration we're usually going to process the reflog quite
quickly, but if we haven't we'd stall.

So as a hack pass down a "show_progress" to reflog_expiry_prepare(),
which will only be shown if we haven't processed the UE_HEAD
case. We'll then start a progress bar, which when passed through to
mark_reachable() will almost certainly result in a substantial initial
iteration.

Then when we've done that initial walk we'll stop that progress bar in
our main loop, and start the "Expiring reflogs" progress bar. We'll
usually start it at 2/X, since our "HEAD" was the 1/X usurped by the
"Iterating (un)reachable HEAD" progress bar.

The "usually" would assume that we wouldn't hit the
"cb->no_reachable_progress = 1" case being added here. I.e. if our
configuration is such that we're not going to do the UE_HEAD walk
we'll just fall back on only using "Expiring reflogs". We'll still
start the count at the 2nd reflog, but it shouldn't matter, on e.g. my
linux.git checkout it doesn't, the progress bar goes by too fast to
notice.

It would have been nicer to be able to compute the
"cb->unreachable_expire_kind" before we call reflog_expire(), but to
do that we'd need the "oid", which we'll only know once we lock it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 53 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 16 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 1a2a210ecf1..ef4f039a20c 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -47,7 +47,11 @@ struct expire_reflog_policy_cb {
 	struct commit *tip_commit;
 	struct commit_list *tips;
 	unsigned int dry_run:1,
-		     verbose:1;
+		     verbose:1,
+		     show_progress:1,
+		     no_reachable_progress:1;
+	struct progress *reachable_progress;
+	uint64_t reachable_progress_cnt;
 };
 
 struct worktree_reflogs {
@@ -235,6 +239,8 @@ static void mark_reachable(struct expire_reflog_policy_cb *cb)
 	while (pending) {
 		struct commit_list *parent;
 		struct commit *commit = pop_commit(&pending);
+
+		display_progress(cb->reachable_progress, ++cb->reachable_progress_cnt);
 		if (commit->object.flags & REACHABLE)
 			continue;
 		if (parse_commit(commit))
@@ -375,16 +381,27 @@ static void reflog_expiry_prepare(const char *refname,
 		cb->unreachable_expire_kind = commit ? UE_NORMAL : UE_ALWAYS;
 	}
 
-	if (cb->cmd.expire_unreachable <= cb->cmd.expire_total)
+	if (cb->cmd.expire_unreachable <= cb->cmd.expire_total) {
+		if (cb->show_progress &&
+		    cb->unreachable_expire_kind == UE_HEAD)
+			cb->no_reachable_progress = 1;
 		cb->unreachable_expire_kind = UE_ALWAYS;
+	}
 
 	switch (cb->unreachable_expire_kind) {
 	case UE_ALWAYS:
 		return;
 	case UE_HEAD:
+		if (cb->show_progress) {
+			const char *s = _("Iterating (un)reachable HEAD");
+			cb->reachable_progress = start_delayed_progress(s, 0);
+		}
 		for_each_ref(push_tip_to_list, &cb->tips);
-		for (elem = cb->tips; elem; elem = elem->next)
+		for (elem = cb->tips; elem; elem = elem->next) {
+			display_progress(cb->reachable_progress,
+					 ++cb->reachable_progress_cnt);
 			commit_list_insert(elem->item, &cb->mark_list);
+		}
 		break;
 	case UE_NORMAL:
 		commit_list_insert(commit, &cb->mark_list);
@@ -633,9 +650,9 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		struct worktree_reflogs collected = {
 			.reflogs = STRING_LIST_INIT_NODUP,
 		};
-		struct string_list_item *item;
 		struct worktree **worktrees, **p;
-		uint64_t progress_cnt;
+		int show_head_progress = show_progress;
+		size_t j;
 
 		if (show_progress)
 			collected.progress = start_delayed_progress(_("Enumerating reflogs"),
@@ -652,27 +669,31 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		free_worktrees(worktrees);
 		stop_progress(&collected.progress);
 
-		if (show_progress) {
-			progress_cnt = 0;
-			progress = start_delayed_progress(_("Expiring reflogs"),
-							  collected.reflogs.nr);
-		}
-
-		for_each_string_list_item(item, &collected.reflogs) {
+		for (j = 0; j < collected.reflogs.nr; j++) {
+			const char *string = collected.reflogs.items[j].string;
 			struct expire_reflog_policy_cb cb = {
 				.cmd = cmd,
 				.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
 				.verbose = verbose,
+				.show_progress = show_head_progress,
 			};
-
-			display_progress(progress, ++progress_cnt);
-			set_reflog_expiry_param(&cb.cmd, explicit_expiry, item->string);
-			status |= reflog_expire(item->string, flags,
+			display_progress(progress, j + 1);
+			set_reflog_expiry_param(&cb.cmd, explicit_expiry, string);
+			status |= reflog_expire(string, flags,
 						reflog_expiry_prepare,
 						should_expire_reflog_ent,
 						reflog_expiry_cleanup,
 						&cb);
+
+			if (show_head_progress &&
+			    (cb.reachable_progress || cb.no_reachable_progress)) {
+				stop_progress(&cb.reachable_progress);
+				show_head_progress = 0;
+				progress = start_delayed_progress(_("Expiring reflogs"),
+								  collected.reflogs.nr);
+			}
 		}
+		display_progress(progress, collected.reflogs.nr); /* only HEAD? */
 		stop_progress(&progress);
 		collected.reflogs.strdup_strings = 1;
 		string_list_clear(&collected.reflogs, 0);
-- 
2.34.1.877.g7d5b0a3b8a6

[PATCH v2 0/9] reftable prep: have builtin/reflog.c handle "--verbose"

[Ævar Arnfjörð Bjarmason]

This series refactors various small bits of builtin/reflog.c
(e.g. using a "struct string_list" now), and finally makes it handle
the "--verbose" output, instead of telling the files backend to emit
that same verbose output.

This means that when we start to integrate "reftable" the new backend
won't need to implement verbose reflog output, it will just work.

This is a sort-of v2[1]. I ejected the changes at the end to add
better --progress output to "git reflog". Those fixes are worthwhile,
but hopefully this smaller & easier to review series can be queued up
first, we can do those UX improvements later.

1. https://lore.kernel.org/git/cover-00.12-00000000000-20211130T213319Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (9):
  reflog delete: narrow scope of "cmd" passed to count_reflog_ent()
  reflog expire: narrow scope of "cb" in cmd_reflog_expire()
  reflog: change one->many worktree->refnames to use a string_list
  reflog expire: don't do negative comparison on enum values
  reflog expire: refactor & use "tip_commit" only for UE_NORMAL
  reflog expire: don't use lookup_commit_reference_gently()
  reflog: reduce scope of "struct rev_info"
  refs files-backend: assume cb->newlog if !EXPIRE_REFLOGS_DRY_RUN
  reflog + refs-backend: move "verbose" out of the backend

 builtin/reflog.c     | 208 +++++++++++++++++++++++--------------------
 refs.h               |   3 +-
 refs/files-backend.c |  44 +++++----
 3 files changed, 134 insertions(+), 121 deletions(-)

Range-diff against v1:
 1:  99e8a639163 =  1:  22c8119640c reflog delete: narrow scope of "cmd" passed to count_reflog_ent()
 2:  c424b26b4fe =  2:  b8e84538427 reflog expire: narrow scope of "cb" in cmd_reflog_expire()
 3:  5a54b04a13e =  3:  c0e190e46cf reflog: change one->many worktree->refnames to use a string_list
 4:  a7a2dfd1406 =  4:  e42fac1b518 reflog expire: don't do negative comparison on enum values
 5:  de162a476c1 =  5:  39263cd00ae reflog expire: refactor & use "tip_commit" only for UE_NORMAL
 6:  eb3dd3fa8b9 =  6:  c71aab5845e reflog expire: don't use lookup_commit_reference_gently()
 7:  3aab4a4a436 =  7:  2fb33ef2546 reflog: reduce scope of "struct rev_info"
 8:  adbec242a7a =  8:  f9fe6a2cfb0 refs files-backend: assume cb->newlog if !EXPIRE_REFLOGS_DRY_RUN
 9:  6a8f3915898 =  9:  28aa0aa6e30 reflog + refs-backend: move "verbose" out of the backend
10:  f54dee1f1cc <  -:  ----------- reflog expire: add progress output on --stale-fix
11:  794e6e677a8 <  -:  ----------- gc + reflog: emit progress output from "reflog expire --all"
12:  fc2b15d0abe <  -:  ----------- gc + reflog: don't stall before initial "git gc" progress output
-- 
2.34.1.1020.gc80c40b6642

[PATCH v2 1/9] reflog delete: narrow scope of "cmd" passed to count_reflog_ent()

[Ævar Arnfjörð Bjarmason]

Change the "cb_data" we pass to the count_reflog_ent() to be the
&cb.cmd itself, instead of passing &cb and having the callback lookup
cb->cmd.

This makes it clear that the "cb" itself is the same memzero'd
structure on each iteration of the for-loop that uses &cb, except for
the "cmd" member.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 175c83e7cc2..4c15d71f3e9 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -662,20 +662,18 @@ static int count_reflog_ent(struct object_id *ooid, struct object_id *noid,
 		const char *email, timestamp_t timestamp, int tz,
 		const char *message, void *cb_data)
 {
-	struct expire_reflog_policy_cb *cb = cb_data;
-	if (!cb->cmd.expire_total || timestamp < cb->cmd.expire_total)
-		cb->cmd.recno++;
+	struct cmd_reflog_expire_cb *cb = cb_data;
+	if (!cb->expire_total || timestamp < cb->expire_total)
+		cb->recno++;
 	return 0;
 }
 
 static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 {
-	struct expire_reflog_policy_cb cb;
+	struct cmd_reflog_expire_cb cmd = { 0 };
 	int i, status = 0;
 	unsigned int flags = 0;
 
-	memset(&cb, 0, sizeof(cb));
-
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
@@ -703,6 +701,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 		const char *spec = strstr(argv[i], "@{");
 		char *ep, *ref;
 		int recno;
+		struct expire_reflog_policy_cb cb = { 0 };
 
 		if (!spec) {
 			status |= error(_("not a reflog: %s"), argv[i]);
@@ -716,14 +715,15 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 
 		recno = strtoul(spec + 2, &ep, 10);
 		if (*ep == '}') {
-			cb.cmd.recno = -recno;
-			for_each_reflog_ent(ref, count_reflog_ent, &cb);
+			cmd.recno = -recno;
+			for_each_reflog_ent(ref, count_reflog_ent, &cmd);
 		} else {
-			cb.cmd.expire_total = approxidate(spec + 2);
-			for_each_reflog_ent(ref, count_reflog_ent, &cb);
-			cb.cmd.expire_total = 0;
+			cmd.expire_total = approxidate(spec + 2);
+			for_each_reflog_ent(ref, count_reflog_ent, &cmd);
+			cmd.expire_total = 0;
 		}
 
+		cb.cmd = cmd;
 		status |= reflog_expire(ref, flags,
 					reflog_expiry_prepare,
 					should_expire_reflog_ent,
-- 
2.34.1.1020.gc80c40b6642

[PATCH v2 2/9] reflog expire: narrow scope of "cb" in cmd_reflog_expire()

[Ævar Arnfjörð Bjarmason]

As with the preceding change for "reflog delete", change the "cb_data"
we pass to callbacks to be &cb.cmd itself, instead of passing &cb and
having the callback lookup cb->cmd.

This makes it clear that the "cb" itself is the same memzero'd
structure on each iteration of the for-loops that use &cb, except for
the "cmd" member.

The "struct expire_reflog_policy_cb" we pass to reflog_expire() will
have the members that aren't "cmd" modified by the callbacks, but
before we invoke them everything except "cmd" is zero'd out.

This included the "tip_commit", "mark_list" and "tips". It might have
looked as though we were re-using those between iterations, but the
first thing we did in reflog_expiry_prepare() was to either NULL them,
or clobber them with another value.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 4c15d71f3e9..6989492bf5c 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -357,7 +357,6 @@ static void reflog_expiry_prepare(const char *refname,
 	struct expire_reflog_policy_cb *cb = cb_data;
 
 	if (!cb->cmd.expire_unreachable || is_head(refname)) {
-		cb->tip_commit = NULL;
 		cb->unreachable_expire_kind = UE_HEAD;
 	} else {
 		cb->tip_commit = lookup_commit_reference_gently(the_repository,
@@ -371,8 +370,6 @@ static void reflog_expiry_prepare(const char *refname,
 	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;
@@ -541,7 +538,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 expire_reflog_policy_cb cb;
+	struct cmd_reflog_expire_cb cmd = { 0 };
 	timestamp_t now = time(NULL);
 	int i, status, do_all, all_worktrees = 1;
 	int explicit_expiry = 0;
@@ -553,10 +550,9 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 
 	save_commit_buffer = 0;
 	do_all = status = 0;
-	memset(&cb, 0, sizeof(cb));
 
-	cb.cmd.expire_total = default_reflog_expire;
-	cb.cmd.expire_unreachable = default_reflog_expire_unreachable;
+	cmd.expire_total = default_reflog_expire;
+	cmd.expire_unreachable = default_reflog_expire_unreachable;
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
@@ -564,17 +560,17 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
 			flags |= EXPIRE_REFLOGS_DRY_RUN;
 		else if (skip_prefix(arg, "--expire=", &arg)) {
-			if (parse_expiry_date(arg, &cb.cmd.expire_total))
+			if (parse_expiry_date(arg, &cmd.expire_total))
 				die(_("'%s' is not a valid timestamp"), arg);
 			explicit_expiry |= EXPIRE_TOTAL;
 		}
 		else if (skip_prefix(arg, "--expire-unreachable=", &arg)) {
-			if (parse_expiry_date(arg, &cb.cmd.expire_unreachable))
+			if (parse_expiry_date(arg, &cmd.expire_unreachable))
 				die(_("'%s' is not a valid timestamp"), arg);
 			explicit_expiry |= EXPIRE_UNREACH;
 		}
 		else if (!strcmp(arg, "--stale-fix"))
-			cb.cmd.stalefix = 1;
+			cmd.stalefix = 1;
 		else if (!strcmp(arg, "--rewrite"))
 			flags |= EXPIRE_REFLOGS_REWRITE;
 		else if (!strcmp(arg, "--updateref"))
@@ -600,14 +596,14 @@ 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.cmd.stalefix) {
-		repo_init_revisions(the_repository, &cb.cmd.revs, prefix);
-		cb.cmd.revs.do_not_die_on_missing_tree = 1;
-		cb.cmd.revs.ignore_missing = 1;
-		cb.cmd.revs.ignore_missing_links = 1;
+	if (cmd.stalefix) {
+		repo_init_revisions(the_repository, &cmd.revs, prefix);
+		cmd.revs.do_not_die_on_missing_tree = 1;
+		cmd.revs.ignore_missing = 1;
+		cmd.revs.ignore_missing_links = 1;
 		if (flags & EXPIRE_REFLOGS_VERBOSE)
 			printf(_("Marking reachable objects..."));
-		mark_reachable_objects(&cb.cmd.revs, 0, 0, NULL);
+		mark_reachable_objects(&cmd.revs, 0, 0, NULL);
 		if (flags & EXPIRE_REFLOGS_VERBOSE)
 			putchar('\n');
 	}
@@ -629,6 +625,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		free_worktrees(worktrees);
 		for (i = 0; i < collected.nr; i++) {
 			struct collected_reflog *e = collected.e[i];
+			struct expire_reflog_policy_cb cb = { .cmd = cmd };
 
 			set_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);
 			status |= reflog_expire(e->reflog, flags,
@@ -643,6 +640,8 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 
 	for (; i < argc; i++) {
 		char *ref;
+		struct expire_reflog_policy_cb cb = { .cmd = cmd };
+
 		if (!dwim_log(argv[i], strlen(argv[i]), NULL, &ref)) {
 			status |= error(_("%s points nowhere!"), argv[i]);
 			continue;
-- 
2.34.1.1020.gc80c40b6642

[PATCH v2 3/9] reflog: change one->many worktree->refnames to use a string_list

[Ævar Arnfjörð Bjarmason]

Change the FLEX_ARRAY pattern added in bda3a31cc79 (reflog-expire:
Avoid creating new files in a directory inside readdir(3) loop,
2008-01-25) the string-list API instead.

This does not change any behavior, allows us to delete much of this
code as it's replaced by things we get from the string-list API for
free, as a result we need just one struct to keep track of this data,
instead of two.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 48 +++++++++++++++++++-----------------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 6989492bf5c..0fb46ade19f 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -48,16 +48,9 @@ struct expire_reflog_policy_cb {
 	struct commit_list *tips;
 };
 
-struct collected_reflog {
-	struct object_id oid;
-	char reflog[FLEX_ARRAY];
-};
-
-struct collect_reflog_cb {
-	struct collected_reflog **e;
-	int alloc;
-	int nr;
-	struct worktree *wt;
+struct worktree_reflogs {
+	struct worktree *worktree;
+	struct string_list reflogs;
 };
 
 /* Remember to update object flag allocation in object.h */
@@ -403,24 +396,20 @@ static void reflog_expiry_cleanup(void *cb_data)
 
 static int collect_reflog(const char *ref, const struct object_id *oid, int unused, void *cb_data)
 {
-	struct collected_reflog *e;
-	struct collect_reflog_cb *cb = cb_data;
+	struct worktree_reflogs *cb = cb_data;
+	struct worktree *worktree = cb->worktree;
 	struct strbuf newref = STRBUF_INIT;
 
 	/*
 	 * Avoid collecting the same shared ref multiple times because
 	 * they are available via all worktrees.
 	 */
-	if (!cb->wt->is_current && ref_type(ref) == REF_TYPE_NORMAL)
+	if (!worktree->is_current && ref_type(ref) == REF_TYPE_NORMAL)
 		return 0;
 
-	strbuf_worktree_ref(cb->wt, &newref, ref);
-	FLEX_ALLOC_STR(e, reflog, newref.buf);
-	strbuf_release(&newref);
+	strbuf_worktree_ref(worktree, &newref, ref);
+	string_list_append(&cb->reflogs, strbuf_detach(&newref, NULL));
 
-	oidcpy(&e->oid, oid);
-	ALLOC_GROW(cb->e, cb->nr + 1, cb->alloc);
-	cb->e[cb->nr++] = e;
 	return 0;
 }
 
@@ -609,33 +598,34 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	}
 
 	if (do_all) {
-		struct collect_reflog_cb collected;
+		struct worktree_reflogs collected = {
+			.reflogs = STRING_LIST_INIT_NODUP,
+		};
+		struct string_list_item *item;
 		struct worktree **worktrees, **p;
-		int i;
 
-		memset(&collected, 0, sizeof(collected));
 		worktrees = get_worktrees();
 		for (p = worktrees; *p; p++) {
 			if (!all_worktrees && !(*p)->is_current)
 				continue;
-			collected.wt = *p;
+			collected.worktree = *p;
 			refs_for_each_reflog(get_worktree_ref_store(*p),
 					     collect_reflog, &collected);
 		}
 		free_worktrees(worktrees);
-		for (i = 0; i < collected.nr; i++) {
-			struct collected_reflog *e = collected.e[i];
+
+		for_each_string_list_item(item, &collected.reflogs) {
 			struct expire_reflog_policy_cb cb = { .cmd = cmd };
 
-			set_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);
-			status |= reflog_expire(e->reflog, flags,
+			set_reflog_expiry_param(&cb.cmd, explicit_expiry, item->string);
+			status |= reflog_expire(item->string, flags,
 						reflog_expiry_prepare,
 						should_expire_reflog_ent,
 						reflog_expiry_cleanup,
 						&cb);
-			free(e);
 		}
-		free(collected.e);
+		collected.reflogs.strdup_strings = 1;
+		string_list_clear(&collected.reflogs, 0);
 	}
 
 	for (; i < argc; i++) {
-- 
2.34.1.1020.gc80c40b6642

[PATCH v2 4/9] reflog expire: don't do negative comparison on enum values

[Ævar Arnfjörð Bjarmason]

Change code added in 03cb91b18cc (reflog --expire-unreachable: special
case entries in "HEAD" reflog, 2010-04-09) to not do positive instead
of negative comparisons on enum values, i.e. not to assume that "x !=
UE_ALWAYS" means "(x == UE_HEAD || x || UE_NORMAL)".

That assumption is true now, but we'd introduce subtle bugs here if
that were to change, now the compiler will notice and error out on
such errors.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 57 ++++++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 24 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 0fb46ade19f..f8a24f1aa26 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -303,10 +303,15 @@ static int should_expire_reflog_ent(struct object_id *ooid, struct object_id *no
 		return 1;
 
 	if (timestamp < cb->cmd.expire_unreachable) {
-		if (cb->unreachable_expire_kind == UE_ALWAYS)
-			return 1;
-		if (unreachable(cb, old_commit, ooid) || unreachable(cb, new_commit, noid))
+		switch (cb->unreachable_expire_kind) {
+		case UE_ALWAYS:
 			return 1;
+		case UE_NORMAL:
+		case UE_HEAD:
+			if (unreachable(cb, old_commit, ooid) || unreachable(cb, new_commit, noid))
+				return 1;
+			break;
+		}
 	}
 
 	if (cb->cmd.recno && --(cb->cmd.recno) == 0)
@@ -348,6 +353,7 @@ static void reflog_expiry_prepare(const char *refname,
 				  void *cb_data)
 {
 	struct expire_reflog_policy_cb *cb = cb_data;
+	struct commit_list *elem;
 
 	if (!cb->cmd.expire_unreachable || is_head(refname)) {
 		cb->unreachable_expire_kind = UE_HEAD;
@@ -363,34 +369,37 @@ static void reflog_expiry_prepare(const char *refname,
 	if (cb->cmd.expire_unreachable <= cb->cmd.expire_total)
 		cb->unreachable_expire_kind = UE_ALWAYS;
 
-	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);
+	switch (cb->unreachable_expire_kind) {
+	case UE_ALWAYS:
+		return;
+	case UE_HEAD:
+		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);
+		break;
+	case UE_NORMAL:
+		commit_list_insert(cb->tip_commit, &cb->mark_list);
 	}
+	cb->mark_limit = cb->cmd.expire_total;
+	mark_reachable(cb);
 }
 
 static void reflog_expiry_cleanup(void *cb_data)
 {
 	struct expire_reflog_policy_cb *cb = cb_data;
+	struct commit_list *elem;
 
-	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);
-		}
+	switch (cb->unreachable_expire_kind) {
+	case UE_ALWAYS:
+		return;
+	case UE_HEAD:
+		for (elem = cb->tips; elem; elem = elem->next)
+			clear_commit_marks(elem->item, REACHABLE);
+		free_commit_list(cb->tips);
+		break;
+	case UE_NORMAL:
+		clear_commit_marks(cb->tip_commit, REACHABLE);
+		break;
 	}
 }
 
-- 
2.34.1.1020.gc80c40b6642

[PATCH v2 5/9] reflog expire: refactor & use "tip_commit" only for UE_NORMAL

[Ævar Arnfjörð Bjarmason]

Add an intermediate variable for "tip_commit" in
reflog_expiry_prepare(), and only add it to the struct if we're
handling the UE_NORMAL case.

The code behaves the same way as before, but this makes the control
flow clearer, and the shorter name allows us to fold a 4-line i/else
int a one-line terany instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index f8a24f1aa26..ec0c6051135 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -354,16 +354,14 @@ static void reflog_expiry_prepare(const char *refname,
 {
 	struct expire_reflog_policy_cb *cb = cb_data;
 	struct commit_list *elem;
+	struct commit *commit = NULL;
 
 	if (!cb->cmd.expire_unreachable || is_head(refname)) {
 		cb->unreachable_expire_kind = UE_HEAD;
 	} else {
-		cb->tip_commit = lookup_commit_reference_gently(the_repository,
-								oid, 1);
-		if (!cb->tip_commit)
-			cb->unreachable_expire_kind = UE_ALWAYS;
-		else
-			cb->unreachable_expire_kind = UE_NORMAL;
+		commit = lookup_commit_reference_gently(the_repository,
+							oid, 1);
+		cb->unreachable_expire_kind = commit ? UE_NORMAL : UE_ALWAYS;
 	}
 
 	if (cb->cmd.expire_unreachable <= cb->cmd.expire_total)
@@ -378,7 +376,9 @@ static void reflog_expiry_prepare(const char *refname,
 			commit_list_insert(elem->item, &cb->mark_list);
 		break;
 	case UE_NORMAL:
-		commit_list_insert(cb->tip_commit, &cb->mark_list);
+		commit_list_insert(commit, &cb->mark_list);
+		/* For reflog_expiry_cleanup() below */
+		cb->tip_commit = commit;
 	}
 	cb->mark_limit = cb->cmd.expire_total;
 	mark_reachable(cb);
-- 
2.34.1.1020.gc80c40b6642

[PATCH v2 6/9] reflog expire: don't use lookup_commit_reference_gently()

[Ævar Arnfjörð Bjarmason]

In the initial implementation of "git reflog" in 4264dc15e19 (git
reflog expire, 2006-12-19) we had this
lookup_commit_reference_gently().

I don't think we've ever found tags that we need to recursively
dereference in reflogs, so this should at least be changed to a
"lookup commit" as I'm doing here, although I can't think of a way
where it mattered in practice.

I also think we'd probably like to just die here if we have a NULL
object, but as this code needs to handle potentially broken
repositories let's just show an "error" but continue, the non-quiet
lookup_commit() will do for us. None of our tests cover the case where
"commit" is NULL after this lookup.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index ec0c6051135..29dcd91abca 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -359,8 +359,7 @@ static void reflog_expiry_prepare(const char *refname,
 	if (!cb->cmd.expire_unreachable || is_head(refname)) {
 		cb->unreachable_expire_kind = UE_HEAD;
 	} else {
-		commit = lookup_commit_reference_gently(the_repository,
-							oid, 1);
+		commit = lookup_commit(the_repository, oid);
 		cb->unreachable_expire_kind = commit ? UE_NORMAL : UE_ALWAYS;
 	}
 
-- 
2.34.1.1020.gc80c40b6642

[PATCH v2 7/9] reflog: reduce scope of "struct rev_info"

[Ævar Arnfjörð Bjarmason]

Change the "cmd.stalefix" handling added in 1389d9ddaa6 (reflog expire
--fix-stale, 2007-01-06) to use a locally scoped "struct
rev_info". This code relies on mark_reachable_objects() twiddling
flags in the walked objects.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 29dcd91abca..48e4f5887b0 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -28,7 +28,6 @@ static timestamp_t default_reflog_expire;
 static timestamp_t default_reflog_expire_unreachable;
 
 struct cmd_reflog_expire_cb {
-	struct rev_info revs;
 	int stalefix;
 	timestamp_t expire_total;
 	timestamp_t expire_unreachable;
@@ -594,13 +593,15 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	 * from reflog if the repository was pruned with older git.
 	 */
 	if (cmd.stalefix) {
-		repo_init_revisions(the_repository, &cmd.revs, prefix);
-		cmd.revs.do_not_die_on_missing_tree = 1;
-		cmd.revs.ignore_missing = 1;
-		cmd.revs.ignore_missing_links = 1;
+		struct rev_info revs;
+
+		repo_init_revisions(the_repository, &revs, prefix);
+		revs.do_not_die_on_missing_tree = 1;
+		revs.ignore_missing = 1;
+		revs.ignore_missing_links = 1;
 		if (flags & EXPIRE_REFLOGS_VERBOSE)
 			printf(_("Marking reachable objects..."));
-		mark_reachable_objects(&cmd.revs, 0, 0, NULL);
+		mark_reachable_objects(&revs, 0, 0, NULL);
 		if (flags & EXPIRE_REFLOGS_VERBOSE)
 			putchar('\n');
 	}
-- 
2.34.1.1020.gc80c40b6642

[PATCH v2 8/9] refs files-backend: assume cb->newlog if !EXPIRE_REFLOGS_DRY_RUN

[Ævar Arnfjörð Bjarmason]

It's not possible for "cb->newlog" to be NULL if
!EXPIRE_REFLOGS_DRY_RUN, since files_reflog_expire() would have
error()'d and taken the "goto failure" branch if it couldn't open the
file. By not using the "newlog" field private to "file-backend.c"'s
"struct expire_reflog_cb", we can move this verbosity logging to
"builtin/reflog.c" in a subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 90b671025a7..5f8586a36e3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3105,12 +3105,12 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
 
 	if ((*cb->should_prune_fn)(ooid, noid, email, timestamp, tz,
 				   message, policy_cb)) {
-		if (!cb->newlog)
+		if (cb->flags & EXPIRE_REFLOGS_DRY_RUN)
 			printf("would prune %s", message);
 		else if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
 			printf("prune %s", message);
 	} else {
-		if (cb->newlog) {
+		if (!(cb->flags & EXPIRE_REFLOGS_DRY_RUN)) {
 			fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s",
 				oid_to_hex(ooid), oid_to_hex(noid),
 				email, timestamp, tz, message);
-- 
2.34.1.1020.gc80c40b6642

[PATCH v2 9/9] reflog + refs-backend: move "verbose" out of the backend

[Ævar Arnfjörð Bjarmason]

Move the handling of the "verbose" flag entirely out of
"refs/files-backend.c" and into "builtin/reflog.c". This allows the
backend to stop knowing about the EXPIRE_REFLOGS_VERBOSE flag.

The expire_reflog_ent() function shouldn't need to deal with the
implementation detail of whether or not we're emitting verbose output,
by doing this the --verbose output becomes backend-agnostic, so
reftable will get the same output.

I think the output is rather bad currently, and should e.g. be
implemented with some better future mode of progress.[ch], but that's
a topic for another improvement.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c     | 42 +++++++++++++++++++++++++++++++-----------
 refs.h               |  3 +--
 refs/files-backend.c | 44 ++++++++++++++++++++------------------------
 3 files changed, 52 insertions(+), 37 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 48e4f5887b0..a77c0d96dce 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -45,6 +45,8 @@ struct expire_reflog_policy_cb {
 	struct cmd_reflog_expire_cb cmd;
 	struct commit *tip_commit;
 	struct commit_list *tips;
+	unsigned int dry_run:1,
+		     verbose:1;
 };
 
 struct worktree_reflogs {
@@ -294,29 +296,38 @@ static int should_expire_reflog_ent(struct object_id *ooid, struct object_id *no
 	struct commit *old_commit, *new_commit;
 
 	if (timestamp < cb->cmd.expire_total)
-		return 1;
+		goto expire;
 
 	old_commit = new_commit = NULL;
 	if (cb->cmd.stalefix &&
 	    (!keep_entry(&old_commit, ooid) || !keep_entry(&new_commit, noid)))
-		return 1;
+		goto expire;
 
 	if (timestamp < cb->cmd.expire_unreachable) {
 		switch (cb->unreachable_expire_kind) {
 		case UE_ALWAYS:
-			return 1;
+			goto expire;
 		case UE_NORMAL:
 		case UE_HEAD:
 			if (unreachable(cb, old_commit, ooid) || unreachable(cb, new_commit, noid))
-				return 1;
+				goto expire;
 			break;
 		}
 	}
 
 	if (cb->cmd.recno && --(cb->cmd.recno) == 0)
-		return 1;
+		goto expire;
+
+	if (cb->verbose)
+		printf("keep %s", message);
 
 	return 0;
+expire:
+	if (cb->dry_run)
+		printf("would prune %s", message);
+	else if (cb->verbose)
+		printf("prune %s", message);
+	return 1;
 }
 
 static int push_tip_to_list(const char *refname, const struct object_id *oid,
@@ -539,6 +550,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	int i, status, do_all, all_worktrees = 1;
 	int explicit_expiry = 0;
 	unsigned int flags = 0;
+	int verbose = 0;
 
 	default_reflog_expire_unreachable = now - 30 * 24 * 3600;
 	default_reflog_expire = now - 90 * 24 * 3600;
@@ -576,7 +588,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		else if (!strcmp(arg, "--single-worktree"))
 			all_worktrees = 0;
 		else if (!strcmp(arg, "--verbose"))
-			flags |= EXPIRE_REFLOGS_VERBOSE;
+			verbose = 1;
 		else if (!strcmp(arg, "--")) {
 			i++;
 			break;
@@ -599,10 +611,10 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		revs.do_not_die_on_missing_tree = 1;
 		revs.ignore_missing = 1;
 		revs.ignore_missing_links = 1;
-		if (flags & EXPIRE_REFLOGS_VERBOSE)
+		if (verbose)
 			printf(_("Marking reachable objects..."));
 		mark_reachable_objects(&revs, 0, 0, NULL);
-		if (flags & EXPIRE_REFLOGS_VERBOSE)
+		if (verbose)
 			putchar('\n');
 	}
 
@@ -624,7 +636,11 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		free_worktrees(worktrees);
 
 		for_each_string_list_item(item, &collected.reflogs) {
-			struct expire_reflog_policy_cb cb = { .cmd = cmd };
+			struct expire_reflog_policy_cb cb = {
+				.cmd = cmd,
+				.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
+				.verbose = verbose,
+			};
 
 			set_reflog_expiry_param(&cb.cmd, explicit_expiry, item->string);
 			status |= reflog_expire(item->string, flags,
@@ -671,6 +687,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 	struct cmd_reflog_expire_cb cmd = { 0 };
 	int i, status = 0;
 	unsigned int flags = 0;
+	int verbose = 0;
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
@@ -681,7 +698,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"))
-			flags |= EXPIRE_REFLOGS_VERBOSE;
+			verbose = 1;
 		else if (!strcmp(arg, "--")) {
 			i++;
 			break;
@@ -699,7 +716,10 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 		const char *spec = strstr(argv[i], "@{");
 		char *ep, *ref;
 		int recno;
-		struct expire_reflog_policy_cb cb = { 0 };
+		struct expire_reflog_policy_cb cb = {
+			.verbose = verbose,
+			.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
+		};
 
 		if (!spec) {
 			status |= error(_("not a reflog: %s"), argv[i]);
diff --git a/refs.h b/refs.h
index 92360e55a20..8f91a7f9ff2 100644
--- a/refs.h
+++ b/refs.h
@@ -820,8 +820,7 @@ enum ref_type ref_type(const char *refname);
 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
+	EXPIRE_REFLOGS_REWRITE = 1 << 2,
 };
 
 /*
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5f8586a36e3..6178ad8c77c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3086,11 +3086,12 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 }
 
 struct expire_reflog_cb {
-	unsigned int flags;
 	reflog_expiry_should_prune_fn *should_prune_fn;
 	void *policy_cb;
 	FILE *newlog;
 	struct object_id last_kept_oid;
+	unsigned int rewrite:1,
+		     dry_run:1;
 };
 
 static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
@@ -3098,33 +3099,27 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
 			     const char *message, void *cb_data)
 {
 	struct expire_reflog_cb *cb = cb_data;
-	struct expire_reflog_policy_cb *policy_cb = cb->policy_cb;
+	reflog_expiry_should_prune_fn *fn = cb->should_prune_fn;
 
-	if (cb->flags & EXPIRE_REFLOGS_REWRITE)
+	if (cb->rewrite)
 		ooid = &cb->last_kept_oid;
 
-	if ((*cb->should_prune_fn)(ooid, noid, email, timestamp, tz,
-				   message, policy_cb)) {
-		if (cb->flags & EXPIRE_REFLOGS_DRY_RUN)
-			printf("would prune %s", message);
-		else if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
-			printf("prune %s", message);
-	} else {
-		if (!(cb->flags & EXPIRE_REFLOGS_DRY_RUN)) {
-			fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s",
-				oid_to_hex(ooid), oid_to_hex(noid),
-				email, timestamp, tz, message);
-			oidcpy(&cb->last_kept_oid, noid);
-		}
-		if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
-			printf("keep %s", message);
-	}
+	if (fn(ooid, noid, email, timestamp, tz, message, cb->policy_cb))
+		return 0;
+
+	if (cb->dry_run)
+		return 0; /* --dry-run */
+
+	fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s", oid_to_hex(ooid),
+		oid_to_hex(noid), email, timestamp, tz, message);
+	oidcpy(&cb->last_kept_oid, noid);
+
 	return 0;
 }
 
 static int files_reflog_expire(struct ref_store *ref_store,
 			       const char *refname,
-			       unsigned int flags,
+			       unsigned int expire_flags,
 			       reflog_expiry_prepare_fn prepare_fn,
 			       reflog_expiry_should_prune_fn should_prune_fn,
 			       reflog_expiry_cleanup_fn cleanup_fn,
@@ -3142,7 +3137,8 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	const struct object_id *oid;
 
 	memset(&cb, 0, sizeof(cb));
-	cb.flags = flags;
+	cb.rewrite = !!(expire_flags & EXPIRE_REFLOGS_REWRITE);
+	cb.dry_run = !!(expire_flags & EXPIRE_REFLOGS_DRY_RUN);
 	cb.policy_cb = policy_cb_data;
 	cb.should_prune_fn = should_prune_fn;
 
@@ -3178,7 +3174,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 
 	files_reflog_path(refs, &log_file_sb, refname);
 	log_file = strbuf_detach(&log_file_sb, NULL);
-	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
+	if (!cb.dry_run) {
 		/*
 		 * Even though holding $GIT_DIR/logs/$reflog.lock has
 		 * no locking implications, we use the lock_file
@@ -3205,7 +3201,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	refs_for_each_reflog_ent(ref_store, refname, expire_reflog_ent, &cb);
 	(*cleanup_fn)(cb.policy_cb);
 
-	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
+	if (!cb.dry_run) {
 		/*
 		 * It doesn't make sense to adjust a reference pointed
 		 * to by a symbolic ref based on expiring entries in
@@ -3215,7 +3211,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 		 */
 		int update = 0;
 
-		if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
+		if ((expire_flags & EXPIRE_REFLOGS_UPDATE_REF) &&
 		    !is_null_oid(&cb.last_kept_oid)) {
 			int ignore_errno;
 			int type;
-- 
2.34.1.1020.gc80c40b6642

Re: [PATCH v2 0/9] reftable prep: have builtin/reflog.c handle "--verbose"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> This series refactors various small bits of builtin/reflog.c
> (e.g. using a "struct string_list" now), and finally makes it handle
> the "--verbose" output, instead of telling the files backend to emit
> that same verbose output.

I was quite confused even after spending quite some time until I
realized that you are primarily talking about the "-v" option of
"git reflog expire" (and "delete", perhaps) and not about "git
reflog -v" having different output from the same command without
"-v" option.

I'll refrain from commenting on, or picking up, the series until
Han-Wen has a chance to take a look and give his input as it is
unclear if it helps or crashes with his plan.

Re: [PATCH v2 0/9] reftable prep: have builtin/reflog.c handle "--verbose"

[Ævar Arnfjörð Bjarmason]

On Thu, Dec 16 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> This series refactors various small bits of builtin/reflog.c
>> (e.g. using a "struct string_list" now), and finally makes it handle
>> the "--verbose" output, instead of telling the files backend to emit
>> that same verbose output.
>
> I was quite confused even after spending quite some time until I
> realized that you are primarily talking about the "-v" option of
> "git reflog expire" (and "delete", perhaps) and not about "git
> reflog -v" having different output from the same command without
> "-v" option.

Will try to clear that up in an eventual re-roll, but considering this a
"look at only if re-rolling for other reasons" for now.

> I'll refrain from commenting on, or picking up, the series until
> Han-Wen has a chance to take a look and give his input as it is
> unclear if it helps or crashes with his plan.

He hasn't reviewed this series.

But I think I/he have looked at it sufficiently that you might want to
consider picking it up, particularly since he doesn't work on Friday's &
with the holidays etc. it might be a while.

He commented on this helping with his plans recently here:

    https://lore.kernel.org/git/CAFQ2z_PSS9zOzR6nGYZ8DBK+6oOQzkJsEy_7y+NprwJ1OHNs7w@mail.gmail.com/

The snippet he's referring to can (last seen on-list, AFAICT) be seen
here:

    https://lore.kernel.org/git/3d57f7c443082fd2a7f01aee003a9cd3ca2dd910.1629207607.git.gitgitgadget@gmail.com/

Which from my reading of the code (I haven't actually tried to combine
this with reftable/* for real) would Just Work when combined with this
series.

I.e. he had "dry_run" implemented, but not "verbose". Now he'll get
"verbose" for free. The "XXX" comment there about dry_run appears to be
a TODO about not doing needless work within the reflog backend.

Re: [PATCH v2 0/9] reftable prep: have builtin/reflog.c handle "--verbose"

[Han-Wen Nienhuys]

On Thu, Dec 16, 2021 at 2:45 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> This series refactors various small bits of builtin/reflog.c
> (e.g. using a "struct string_list" now), and finally makes it handle
> the "--verbose" output, instead of telling the files backend to emit
> that same verbose output.
>
> This means that when we start to integrate "reftable" the new backend
> won't need to implement verbose reflog output, it will just work.
>
> This is a sort-of v2[1]. I ejected the changes at the end to add
> better --progress output to "git reflog". Those fixes are worthwhile,
> but hopefully this smaller & easier to review series can be queued up
> first, we can do those UX improvements later.

Thanks for sending this.

I looked over all patches separately. Overall, the series looks good to me.

> int a one-line terany instead.

ternary.

> "don't do negative comparison on enum values"

I would describe it as "use switch over enum values", as this doesn't
involve negative numbers.

> collected.reflogs.strdup_strings = 1;

This puzzled me. Why isn't the init done as _DUP ? Warrants a comment
at the least.

>  .. goto expire
> ..
>    return 0;
> expire:

(personal opinion) this is going overboard with gotos and labels. Either

  if ( .. ) {
    expire = 1;
    goto done;
  }
done:
  if (expire) {  print stuff }
  return expire

Or wrap the existing function (without changes) in a callback that
does the print for you.

your call.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: [PATCH v2 0/9] reftable prep: have builtin/reflog.c handle "--verbose"

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 21 2021, Han-Wen Nienhuys wrote:

> On Thu, Dec 16, 2021 at 2:45 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> This series refactors various small bits of builtin/reflog.c
>> (e.g. using a "struct string_list" now), and finally makes it handle
>> the "--verbose" output, instead of telling the files backend to emit
>> that same verbose output.
>>
>> This means that when we start to integrate "reftable" the new backend
>> won't need to implement verbose reflog output, it will just work.
>>
>> This is a sort-of v2[1]. I ejected the changes at the end to add
>> better --progress output to "git reflog". Those fixes are worthwhile,
>> but hopefully this smaller & easier to review series can be queued up
>> first, we can do those UX improvements later.
>
> Thanks for sending this.
>
> I looked over all patches separately. Overall, the series looks good to me.
>
>> int a one-line terany instead.
>
> ternary.
>
>> "don't do negative comparison on enum values"

Will fix.

> I would describe it as "use switch over enum values", as this doesn't
> involve negative numbers.

*nod*

>> collected.reflogs.strdup_strings = 1;
>
> This puzzled me. Why isn't the init done as _DUP ? Warrants a comment
> at the least.

Will comment on it. FWWI this is a common pattern with the string_list
API.

If you declare it "dup" and push into it you'll end up double-duping,
but if you don't declare it dup'd and free it you'll leak memory. It
won't free() a non-duped list.

The other option is to declare it "dup" and then
"string_list_append_nodup", will try and see...

>>  .. goto expire
>> ..
>>    return 0;
>> expire:
>
> (personal opinion) this is going overboard with gotos and labels. Either
>
>   if ( .. ) {
>     expire = 1;
>     goto done;
>   }
> done:
>   if (expire) {  print stuff }
>   return expire
>
> Or wrap the existing function (without changes) in a callback that
> does the print for you.
>
> your call.
>

Will experiment & see, looks like a wrapper might be easiest here.

Thanks!

[PATCH v3 0/9] reftable prep: have builtin/reflog.c handle "--verbose"

[Ævar Arnfjörð Bjarmason]

This series refactors various small bits of builtin/reflog.c
(e.g. using a "struct string_list" now), and finally makes it handle
the "--verbose" output, instead of telling the files backend to emit
that same verbose output.

This means that when we start to integrate "reftable" the new backend
won't need to implement verbose reflog output, it will just work.

This v3 addresses comments Han-Wen had on the v2[1] in [2]. Those are
all addressed here.

I thought that eliminating the "goto" as he suggested might lead to
needlessly verbose code, but I think this new approach is much
better. We now have a "verbose" callback that's a wrapper around the
non-verbose version. For the common case of non-verbose we won't
execute any "verbose" code at all, which makes this code easier to
follow.

1. https://lore.kernel.org/git/cover-v2-0.9-00000000000-20211216T134028Z-avarab@gmail.com/
2. https://lore.kernel.org/git/CAFQ2z_McOfm545Xd8hF7YDgzyOjDmcGxpWZ6pQ-yaKAEWMMbgg@mail.gmail.com/

Ævar Arnfjörð Bjarmason (9):
  reflog delete: narrow scope of "cmd" passed to count_reflog_ent()
  reflog expire: narrow scope of "cb" in cmd_reflog_expire()
  reflog: change one->many worktree->refnames to use a string_list
  reflog expire: use "switch" over enum values
  reflog expire: refactor & use "tip_commit" only for UE_NORMAL
  reflog expire: don't use lookup_commit_reference_gently()
  reflog: reduce scope of "struct rev_info"
  refs files-backend: assume cb->newlog if !EXPIRE_REFLOGS_DRY_RUN
  reflog + refs-backend: move "verbose" out of the backend

 builtin/reflog.c     | 223 +++++++++++++++++++++++++------------------
 refs.h               |   3 +-
 refs/files-backend.c |  44 ++++-----
 3 files changed, 150 insertions(+), 120 deletions(-)

Range-diff against v2:
 1:  22c8119640c =  1:  7fac198f485 reflog delete: narrow scope of "cmd" passed to count_reflog_ent()
 2:  b8e84538427 =  2:  1ffc8ef8a8b reflog expire: narrow scope of "cb" in cmd_reflog_expire()
 3:  c0e190e46cf !  3:  ba7679e6fc0 reflog: change one->many worktree->refnames to use a string_list
    @@ Commit message
         free, as a result we need just one struct to keep track of this data,
         instead of two.
     
    +    The "DUP" -> "string_list_append_nodup(..., strbuf_detach(...))"
    +    pattern here is the same as that used in a recent memory leak fix in
    +    b202e51b154 (grep: fix a "path_list" memory leak, 2021-10-22).
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/reflog.c ##
    @@ builtin/reflog.c: static void reflog_expiry_cleanup(void *cb_data)
     -	FLEX_ALLOC_STR(e, reflog, newref.buf);
     -	strbuf_release(&newref);
     +	strbuf_worktree_ref(worktree, &newref, ref);
    -+	string_list_append(&cb->reflogs, strbuf_detach(&newref, NULL));
    ++	string_list_append_nodup(&cb->reflogs, strbuf_detach(&newref, NULL));
      
     -	oidcpy(&e->oid, oid);
     -	ALLOC_GROW(cb->e, cb->nr + 1, cb->alloc);
    @@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, cons
      	if (do_all) {
     -		struct collect_reflog_cb collected;
     +		struct worktree_reflogs collected = {
    -+			.reflogs = STRING_LIST_INIT_NODUP,
    ++			.reflogs = STRING_LIST_INIT_DUP,
     +		};
     +		struct string_list_item *item;
      		struct worktree **worktrees, **p;
    @@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, cons
     -			free(e);
      		}
     -		free(collected.e);
    -+		collected.reflogs.strdup_strings = 1;
     +		string_list_clear(&collected.reflogs, 0);
      	}
      
 4:  e42fac1b518 !  4:  74447de0413 reflog expire: don't do negative comparison on enum values
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    reflog expire: don't do negative comparison on enum values
    +    reflog expire: use "switch" over enum values
     
         Change code added in 03cb91b18cc (reflog --expire-unreachable: special
    -    case entries in "HEAD" reflog, 2010-04-09) to not do positive instead
    -    of negative comparisons on enum values, i.e. not to assume that "x !=
    -    UE_ALWAYS" means "(x == UE_HEAD || x || UE_NORMAL)".
    +    case entries in "HEAD" reflog, 2010-04-09) to use a "switch" statement
    +    with an exhaustive list of "case" statements instead of doing numeric
    +    comparisons against the enum labels.
     
    -    That assumption is true now, but we'd introduce subtle bugs here if
    -    that were to change, now the compiler will notice and error out on
    -    such errors.
    +    Now we won't assume that "x != UE_ALWAYS" means "(x == UE_HEAD || x ||
    +    UE_NORMAL)". That assumption is true now, but we'd introduce subtle
    +    bugs here if that were to change, now the compiler will notice and
    +    error out on such errors.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
 5:  39263cd00ae !  5:  896ae9f73eb reflog expire: refactor & use "tip_commit" only for UE_NORMAL
    @@ Commit message
     
         The code behaves the same way as before, but this makes the control
         flow clearer, and the shorter name allows us to fold a 4-line i/else
    -    int a one-line terany instead.
    +    into a one-line ternary instead.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
 6:  c71aab5845e =  6:  cfa80e84c6d reflog expire: don't use lookup_commit_reference_gently()
 7:  2fb33ef2546 =  7:  b3a62b9b177 reflog: reduce scope of "struct rev_info"
 8:  f9fe6a2cfb0 =  8:  6748298a782 refs files-backend: assume cb->newlog if !EXPIRE_REFLOGS_DRY_RUN
 9:  28aa0aa6e30 !  9:  2fb9de8ae51 reflog + refs-backend: move "verbose" out of the backend
    @@ builtin/reflog.c: struct expire_reflog_policy_cb {
      	struct cmd_reflog_expire_cb cmd;
      	struct commit *tip_commit;
      	struct commit_list *tips;
    -+	unsigned int dry_run:1,
    -+		     verbose:1;
    ++	unsigned int dry_run:1;
      };
      
      struct worktree_reflogs {
     @@ builtin/reflog.c: static int should_expire_reflog_ent(struct object_id *ooid, struct object_id *no
    - 	struct commit *old_commit, *new_commit;
    - 
    - 	if (timestamp < cb->cmd.expire_total)
    --		return 1;
    -+		goto expire;
    - 
    - 	old_commit = new_commit = NULL;
    - 	if (cb->cmd.stalefix &&
    - 	    (!keep_entry(&old_commit, ooid) || !keep_entry(&new_commit, noid)))
    --		return 1;
    -+		goto expire;
    - 
    - 	if (timestamp < cb->cmd.expire_unreachable) {
    - 		switch (cb->unreachable_expire_kind) {
    - 		case UE_ALWAYS:
    --			return 1;
    -+			goto expire;
    - 		case UE_NORMAL:
    - 		case UE_HEAD:
    - 			if (unreachable(cb, old_commit, ooid) || unreachable(cb, new_commit, noid))
    --				return 1;
    -+				goto expire;
    - 			break;
    - 		}
    - 	}
    + 	return 0;
    + }
      
    - 	if (cb->cmd.recno && --(cb->cmd.recno) == 0)
    --		return 1;
    -+		goto expire;
    ++static int should_expire_reflog_ent_verbose(struct object_id *ooid,
    ++					    struct object_id *noid,
    ++					    const char *email,
    ++					    timestamp_t timestamp, int tz,
    ++					    const char *message, void *cb_data)
    ++{
    ++	struct expire_reflog_policy_cb *cb = cb_data;
    ++	int expire;
     +
    -+	if (cb->verbose)
    ++	expire = should_expire_reflog_ent(ooid, noid, email, timestamp, tz,
    ++					  message, cb);
    ++
    ++	if (!expire)
     +		printf("keep %s", message);
    - 
    - 	return 0;
    -+expire:
    -+	if (cb->dry_run)
    ++	else if (cb->dry_run)
     +		printf("would prune %s", message);
    -+	else if (cb->verbose)
    ++	else
     +		printf("prune %s", message);
    -+	return 1;
    - }
    - 
    ++
    ++	return expire;
    ++}
    ++
      static int push_tip_to_list(const char *refname, const struct object_id *oid,
    + 			    int flags, void *cb_data)
    + {
     @@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
      	int i, status, do_all, all_worktrees = 1;
      	int explicit_expiry = 0;
      	unsigned int flags = 0;
     +	int verbose = 0;
    ++	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
      
      	default_reflog_expire_unreachable = now - 30 * 24 * 3600;
      	default_reflog_expire = now - 90 * 24 * 3600;
    @@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, cons
      		else if (!strcmp(arg, "--")) {
      			i++;
      			break;
    +@@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
    + 			break;
    + 	}
    + 
    ++	if (verbose)
    ++		should_prune_fn = should_expire_reflog_ent_verbose;
    ++
    + 	/*
    + 	 * We can trust the commits and objects reachable from refs
    + 	 * even in older repository.  We cannot trust what's reachable
     @@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
      		revs.do_not_die_on_missing_tree = 1;
      		revs.ignore_missing = 1;
    @@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, cons
     +			struct expire_reflog_policy_cb cb = {
     +				.cmd = cmd,
     +				.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
    -+				.verbose = verbose,
     +			};
      
      			set_reflog_expiry_param(&cb.cmd, explicit_expiry, item->string);
      			status |= reflog_expire(item->string, flags,
    + 						reflog_expiry_prepare,
    +-						should_expire_reflog_ent,
    ++						should_prune_fn,
    + 						reflog_expiry_cleanup,
    + 						&cb);
    + 		}
    +@@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
    + 		set_reflog_expiry_param(&cb.cmd, explicit_expiry, ref);
    + 		status |= reflog_expire(ref, flags,
    + 					reflog_expiry_prepare,
    +-					should_expire_reflog_ent,
    ++					should_prune_fn,
    + 					reflog_expiry_cleanup,
    + 					&cb);
    + 		free(ref);
     @@ builtin/reflog.c: static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
      	struct cmd_reflog_expire_cb cmd = { 0 };
      	int i, status = 0;
      	unsigned int flags = 0;
     +	int verbose = 0;
    ++	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
      
      	for (i = 1; i < argc; i++) {
      		const char *arg = argv[i];
    @@ builtin/reflog.c: static int cmd_reflog_delete(int argc, const char **argv, cons
      		else if (!strcmp(arg, "--")) {
      			i++;
      			break;
    +@@ builtin/reflog.c: static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
    + 			break;
    + 	}
    + 
    ++	if (verbose)
    ++		should_prune_fn = should_expire_reflog_ent_verbose;
    ++
    + 	if (argc - i < 1)
    + 		return error(_("no reflog specified to delete"));
    + 
     @@ builtin/reflog.c: static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
      		const char *spec = strstr(argv[i], "@{");
      		char *ep, *ref;
      		int recno;
     -		struct expire_reflog_policy_cb cb = { 0 };
     +		struct expire_reflog_policy_cb cb = {
    -+			.verbose = verbose,
     +			.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
     +		};
      
      		if (!spec) {
      			status |= error(_("not a reflog: %s"), argv[i]);
    +@@ builtin/reflog.c: static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
    + 		cb.cmd = cmd;
    + 		status |= reflog_expire(ref, flags,
    + 					reflog_expiry_prepare,
    +-					should_expire_reflog_ent,
    ++					should_prune_fn,
    + 					reflog_expiry_cleanup,
    + 					&cb);
    + 		free(ref);
     
      ## refs.h ##
     @@ refs.h: enum ref_type ref_type(const char *refname);
-- 
2.34.1.1146.gb52885e7c44

[PATCH v3 1/9] reflog delete: narrow scope of "cmd" passed to count_reflog_ent()

[Ævar Arnfjörð Bjarmason]

Change the "cb_data" we pass to the count_reflog_ent() to be the
&cb.cmd itself, instead of passing &cb and having the callback lookup
cb->cmd.

This makes it clear that the "cb" itself is the same memzero'd
structure on each iteration of the for-loop that uses &cb, except for
the "cmd" member.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 175c83e7cc2..4c15d71f3e9 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -662,20 +662,18 @@ static int count_reflog_ent(struct object_id *ooid, struct object_id *noid,
 		const char *email, timestamp_t timestamp, int tz,
 		const char *message, void *cb_data)
 {
-	struct expire_reflog_policy_cb *cb = cb_data;
-	if (!cb->cmd.expire_total || timestamp < cb->cmd.expire_total)
-		cb->cmd.recno++;
+	struct cmd_reflog_expire_cb *cb = cb_data;
+	if (!cb->expire_total || timestamp < cb->expire_total)
+		cb->recno++;
 	return 0;
 }
 
 static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 {
-	struct expire_reflog_policy_cb cb;
+	struct cmd_reflog_expire_cb cmd = { 0 };
 	int i, status = 0;
 	unsigned int flags = 0;
 
-	memset(&cb, 0, sizeof(cb));
-
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
@@ -703,6 +701,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 		const char *spec = strstr(argv[i], "@{");
 		char *ep, *ref;
 		int recno;
+		struct expire_reflog_policy_cb cb = { 0 };
 
 		if (!spec) {
 			status |= error(_("not a reflog: %s"), argv[i]);
@@ -716,14 +715,15 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 
 		recno = strtoul(spec + 2, &ep, 10);
 		if (*ep == '}') {
-			cb.cmd.recno = -recno;
-			for_each_reflog_ent(ref, count_reflog_ent, &cb);
+			cmd.recno = -recno;
+			for_each_reflog_ent(ref, count_reflog_ent, &cmd);
 		} else {
-			cb.cmd.expire_total = approxidate(spec + 2);
-			for_each_reflog_ent(ref, count_reflog_ent, &cb);
-			cb.cmd.expire_total = 0;
+			cmd.expire_total = approxidate(spec + 2);
+			for_each_reflog_ent(ref, count_reflog_ent, &cmd);
+			cmd.expire_total = 0;
 		}
 
+		cb.cmd = cmd;
 		status |= reflog_expire(ref, flags,
 					reflog_expiry_prepare,
 					should_expire_reflog_ent,
-- 
2.34.1.1146.gb52885e7c44

[PATCH v3 2/9] reflog expire: narrow scope of "cb" in cmd_reflog_expire()

[Ævar Arnfjörð Bjarmason]

As with the preceding change for "reflog delete", change the "cb_data"
we pass to callbacks to be &cb.cmd itself, instead of passing &cb and
having the callback lookup cb->cmd.

This makes it clear that the "cb" itself is the same memzero'd
structure on each iteration of the for-loops that use &cb, except for
the "cmd" member.

The "struct expire_reflog_policy_cb" we pass to reflog_expire() will
have the members that aren't "cmd" modified by the callbacks, but
before we invoke them everything except "cmd" is zero'd out.

This included the "tip_commit", "mark_list" and "tips". It might have
looked as though we were re-using those between iterations, but the
first thing we did in reflog_expiry_prepare() was to either NULL them,
or clobber them with another value.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 4c15d71f3e9..6989492bf5c 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -357,7 +357,6 @@ static void reflog_expiry_prepare(const char *refname,
 	struct expire_reflog_policy_cb *cb = cb_data;
 
 	if (!cb->cmd.expire_unreachable || is_head(refname)) {
-		cb->tip_commit = NULL;
 		cb->unreachable_expire_kind = UE_HEAD;
 	} else {
 		cb->tip_commit = lookup_commit_reference_gently(the_repository,
@@ -371,8 +370,6 @@ static void reflog_expiry_prepare(const char *refname,
 	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;
@@ -541,7 +538,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 expire_reflog_policy_cb cb;
+	struct cmd_reflog_expire_cb cmd = { 0 };
 	timestamp_t now = time(NULL);
 	int i, status, do_all, all_worktrees = 1;
 	int explicit_expiry = 0;
@@ -553,10 +550,9 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 
 	save_commit_buffer = 0;
 	do_all = status = 0;
-	memset(&cb, 0, sizeof(cb));
 
-	cb.cmd.expire_total = default_reflog_expire;
-	cb.cmd.expire_unreachable = default_reflog_expire_unreachable;
+	cmd.expire_total = default_reflog_expire;
+	cmd.expire_unreachable = default_reflog_expire_unreachable;
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
@@ -564,17 +560,17 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
 			flags |= EXPIRE_REFLOGS_DRY_RUN;
 		else if (skip_prefix(arg, "--expire=", &arg)) {
-			if (parse_expiry_date(arg, &cb.cmd.expire_total))
+			if (parse_expiry_date(arg, &cmd.expire_total))
 				die(_("'%s' is not a valid timestamp"), arg);
 			explicit_expiry |= EXPIRE_TOTAL;
 		}
 		else if (skip_prefix(arg, "--expire-unreachable=", &arg)) {
-			if (parse_expiry_date(arg, &cb.cmd.expire_unreachable))
+			if (parse_expiry_date(arg, &cmd.expire_unreachable))
 				die(_("'%s' is not a valid timestamp"), arg);
 			explicit_expiry |= EXPIRE_UNREACH;
 		}
 		else if (!strcmp(arg, "--stale-fix"))
-			cb.cmd.stalefix = 1;
+			cmd.stalefix = 1;
 		else if (!strcmp(arg, "--rewrite"))
 			flags |= EXPIRE_REFLOGS_REWRITE;
 		else if (!strcmp(arg, "--updateref"))
@@ -600,14 +596,14 @@ 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.cmd.stalefix) {
-		repo_init_revisions(the_repository, &cb.cmd.revs, prefix);
-		cb.cmd.revs.do_not_die_on_missing_tree = 1;
-		cb.cmd.revs.ignore_missing = 1;
-		cb.cmd.revs.ignore_missing_links = 1;
+	if (cmd.stalefix) {
+		repo_init_revisions(the_repository, &cmd.revs, prefix);
+		cmd.revs.do_not_die_on_missing_tree = 1;
+		cmd.revs.ignore_missing = 1;
+		cmd.revs.ignore_missing_links = 1;
 		if (flags & EXPIRE_REFLOGS_VERBOSE)
 			printf(_("Marking reachable objects..."));
-		mark_reachable_objects(&cb.cmd.revs, 0, 0, NULL);
+		mark_reachable_objects(&cmd.revs, 0, 0, NULL);
 		if (flags & EXPIRE_REFLOGS_VERBOSE)
 			putchar('\n');
 	}
@@ -629,6 +625,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		free_worktrees(worktrees);
 		for (i = 0; i < collected.nr; i++) {
 			struct collected_reflog *e = collected.e[i];
+			struct expire_reflog_policy_cb cb = { .cmd = cmd };
 
 			set_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);
 			status |= reflog_expire(e->reflog, flags,
@@ -643,6 +640,8 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 
 	for (; i < argc; i++) {
 		char *ref;
+		struct expire_reflog_policy_cb cb = { .cmd = cmd };
+
 		if (!dwim_log(argv[i], strlen(argv[i]), NULL, &ref)) {
 			status |= error(_("%s points nowhere!"), argv[i]);
 			continue;
-- 
2.34.1.1146.gb52885e7c44

[PATCH v3 3/9] reflog: change one->many worktree->refnames to use a string_list

[Ævar Arnfjörð Bjarmason]

Change the FLEX_ARRAY pattern added in bda3a31cc79 (reflog-expire:
Avoid creating new files in a directory inside readdir(3) loop,
2008-01-25) the string-list API instead.

This does not change any behavior, allows us to delete much of this
code as it's replaced by things we get from the string-list API for
free, as a result we need just one struct to keep track of this data,
instead of two.

The "DUP" -> "string_list_append_nodup(..., strbuf_detach(...))"
pattern here is the same as that used in a recent memory leak fix in
b202e51b154 (grep: fix a "path_list" memory leak, 2021-10-22).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 47 ++++++++++++++++++-----------------------------
 1 file changed, 18 insertions(+), 29 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 6989492bf5c..27851c6efb7 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -48,16 +48,9 @@ struct expire_reflog_policy_cb {
 	struct commit_list *tips;
 };
 
-struct collected_reflog {
-	struct object_id oid;
-	char reflog[FLEX_ARRAY];
-};
-
-struct collect_reflog_cb {
-	struct collected_reflog **e;
-	int alloc;
-	int nr;
-	struct worktree *wt;
+struct worktree_reflogs {
+	struct worktree *worktree;
+	struct string_list reflogs;
 };
 
 /* Remember to update object flag allocation in object.h */
@@ -403,24 +396,20 @@ static void reflog_expiry_cleanup(void *cb_data)
 
 static int collect_reflog(const char *ref, const struct object_id *oid, int unused, void *cb_data)
 {
-	struct collected_reflog *e;
-	struct collect_reflog_cb *cb = cb_data;
+	struct worktree_reflogs *cb = cb_data;
+	struct worktree *worktree = cb->worktree;
 	struct strbuf newref = STRBUF_INIT;
 
 	/*
 	 * Avoid collecting the same shared ref multiple times because
 	 * they are available via all worktrees.
 	 */
-	if (!cb->wt->is_current && ref_type(ref) == REF_TYPE_NORMAL)
+	if (!worktree->is_current && ref_type(ref) == REF_TYPE_NORMAL)
 		return 0;
 
-	strbuf_worktree_ref(cb->wt, &newref, ref);
-	FLEX_ALLOC_STR(e, reflog, newref.buf);
-	strbuf_release(&newref);
+	strbuf_worktree_ref(worktree, &newref, ref);
+	string_list_append_nodup(&cb->reflogs, strbuf_detach(&newref, NULL));
 
-	oidcpy(&e->oid, oid);
-	ALLOC_GROW(cb->e, cb->nr + 1, cb->alloc);
-	cb->e[cb->nr++] = e;
 	return 0;
 }
 
@@ -609,33 +598,33 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	}
 
 	if (do_all) {
-		struct collect_reflog_cb collected;
+		struct worktree_reflogs collected = {
+			.reflogs = STRING_LIST_INIT_DUP,
+		};
+		struct string_list_item *item;
 		struct worktree **worktrees, **p;
-		int i;
 
-		memset(&collected, 0, sizeof(collected));
 		worktrees = get_worktrees();
 		for (p = worktrees; *p; p++) {
 			if (!all_worktrees && !(*p)->is_current)
 				continue;
-			collected.wt = *p;
+			collected.worktree = *p;
 			refs_for_each_reflog(get_worktree_ref_store(*p),
 					     collect_reflog, &collected);
 		}
 		free_worktrees(worktrees);
-		for (i = 0; i < collected.nr; i++) {
-			struct collected_reflog *e = collected.e[i];
+
+		for_each_string_list_item(item, &collected.reflogs) {
 			struct expire_reflog_policy_cb cb = { .cmd = cmd };
 
-			set_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);
-			status |= reflog_expire(e->reflog, flags,
+			set_reflog_expiry_param(&cb.cmd, explicit_expiry, item->string);
+			status |= reflog_expire(item->string, flags,
 						reflog_expiry_prepare,
 						should_expire_reflog_ent,
 						reflog_expiry_cleanup,
 						&cb);
-			free(e);
 		}
-		free(collected.e);
+		string_list_clear(&collected.reflogs, 0);
 	}
 
 	for (; i < argc; i++) {
-- 
2.34.1.1146.gb52885e7c44

[PATCH v3 4/9] reflog expire: use "switch" over enum values

[Ævar Arnfjörð Bjarmason]

Change code added in 03cb91b18cc (reflog --expire-unreachable: special
case entries in "HEAD" reflog, 2010-04-09) to use a "switch" statement
with an exhaustive list of "case" statements instead of doing numeric
comparisons against the enum labels.

Now we won't assume that "x != UE_ALWAYS" means "(x == UE_HEAD || x ||
UE_NORMAL)". That assumption is true now, but we'd introduce subtle
bugs here if that were to change, now the compiler will notice and
error out on such errors.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 57 ++++++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 24 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 27851c6efb7..8d05660e644 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -303,10 +303,15 @@ static int should_expire_reflog_ent(struct object_id *ooid, struct object_id *no
 		return 1;
 
 	if (timestamp < cb->cmd.expire_unreachable) {
-		if (cb->unreachable_expire_kind == UE_ALWAYS)
-			return 1;
-		if (unreachable(cb, old_commit, ooid) || unreachable(cb, new_commit, noid))
+		switch (cb->unreachable_expire_kind) {
+		case UE_ALWAYS:
 			return 1;
+		case UE_NORMAL:
+		case UE_HEAD:
+			if (unreachable(cb, old_commit, ooid) || unreachable(cb, new_commit, noid))
+				return 1;
+			break;
+		}
 	}
 
 	if (cb->cmd.recno && --(cb->cmd.recno) == 0)
@@ -348,6 +353,7 @@ static void reflog_expiry_prepare(const char *refname,
 				  void *cb_data)
 {
 	struct expire_reflog_policy_cb *cb = cb_data;
+	struct commit_list *elem;
 
 	if (!cb->cmd.expire_unreachable || is_head(refname)) {
 		cb->unreachable_expire_kind = UE_HEAD;
@@ -363,34 +369,37 @@ static void reflog_expiry_prepare(const char *refname,
 	if (cb->cmd.expire_unreachable <= cb->cmd.expire_total)
 		cb->unreachable_expire_kind = UE_ALWAYS;
 
-	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);
+	switch (cb->unreachable_expire_kind) {
+	case UE_ALWAYS:
+		return;
+	case UE_HEAD:
+		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);
+		break;
+	case UE_NORMAL:
+		commit_list_insert(cb->tip_commit, &cb->mark_list);
 	}
+	cb->mark_limit = cb->cmd.expire_total;
+	mark_reachable(cb);
 }
 
 static void reflog_expiry_cleanup(void *cb_data)
 {
 	struct expire_reflog_policy_cb *cb = cb_data;
+	struct commit_list *elem;
 
-	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);
-		}
+	switch (cb->unreachable_expire_kind) {
+	case UE_ALWAYS:
+		return;
+	case UE_HEAD:
+		for (elem = cb->tips; elem; elem = elem->next)
+			clear_commit_marks(elem->item, REACHABLE);
+		free_commit_list(cb->tips);
+		break;
+	case UE_NORMAL:
+		clear_commit_marks(cb->tip_commit, REACHABLE);
+		break;
 	}
 }
 
-- 
2.34.1.1146.gb52885e7c44

[PATCH v3 5/9] reflog expire: refactor & use "tip_commit" only for UE_NORMAL

[Ævar Arnfjörð Bjarmason]

Add an intermediate variable for "tip_commit" in
reflog_expiry_prepare(), and only add it to the struct if we're
handling the UE_NORMAL case.

The code behaves the same way as before, but this makes the control
flow clearer, and the shorter name allows us to fold a 4-line i/else
into a one-line ternary instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 8d05660e644..f18a63751f3 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -354,16 +354,14 @@ static void reflog_expiry_prepare(const char *refname,
 {
 	struct expire_reflog_policy_cb *cb = cb_data;
 	struct commit_list *elem;
+	struct commit *commit = NULL;
 
 	if (!cb->cmd.expire_unreachable || is_head(refname)) {
 		cb->unreachable_expire_kind = UE_HEAD;
 	} else {
-		cb->tip_commit = lookup_commit_reference_gently(the_repository,
-								oid, 1);
-		if (!cb->tip_commit)
-			cb->unreachable_expire_kind = UE_ALWAYS;
-		else
-			cb->unreachable_expire_kind = UE_NORMAL;
+		commit = lookup_commit_reference_gently(the_repository,
+							oid, 1);
+		cb->unreachable_expire_kind = commit ? UE_NORMAL : UE_ALWAYS;
 	}
 
 	if (cb->cmd.expire_unreachable <= cb->cmd.expire_total)
@@ -378,7 +376,9 @@ static void reflog_expiry_prepare(const char *refname,
 			commit_list_insert(elem->item, &cb->mark_list);
 		break;
 	case UE_NORMAL:
-		commit_list_insert(cb->tip_commit, &cb->mark_list);
+		commit_list_insert(commit, &cb->mark_list);
+		/* For reflog_expiry_cleanup() below */
+		cb->tip_commit = commit;
 	}
 	cb->mark_limit = cb->cmd.expire_total;
 	mark_reachable(cb);
-- 
2.34.1.1146.gb52885e7c44

[PATCH v3 6/9] reflog expire: don't use lookup_commit_reference_gently()

[Ævar Arnfjörð Bjarmason]

In the initial implementation of "git reflog" in 4264dc15e19 (git
reflog expire, 2006-12-19) we had this
lookup_commit_reference_gently().

I don't think we've ever found tags that we need to recursively
dereference in reflogs, so this should at least be changed to a
"lookup commit" as I'm doing here, although I can't think of a way
where it mattered in practice.

I also think we'd probably like to just die here if we have a NULL
object, but as this code needs to handle potentially broken
repositories let's just show an "error" but continue, the non-quiet
lookup_commit() will do for us. None of our tests cover the case where
"commit" is NULL after this lookup.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index f18a63751f3..fe0bd353829 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -359,8 +359,7 @@ static void reflog_expiry_prepare(const char *refname,
 	if (!cb->cmd.expire_unreachable || is_head(refname)) {
 		cb->unreachable_expire_kind = UE_HEAD;
 	} else {
-		commit = lookup_commit_reference_gently(the_repository,
-							oid, 1);
+		commit = lookup_commit(the_repository, oid);
 		cb->unreachable_expire_kind = commit ? UE_NORMAL : UE_ALWAYS;
 	}
 
-- 
2.34.1.1146.gb52885e7c44

[PATCH v3 7/9] reflog: reduce scope of "struct rev_info"

[Ævar Arnfjörð Bjarmason]

Change the "cmd.stalefix" handling added in 1389d9ddaa6 (reflog expire
--fix-stale, 2007-01-06) to use a locally scoped "struct
rev_info". This code relies on mark_reachable_objects() twiddling
flags in the walked objects.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index fe0bd353829..4ff63846058 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -28,7 +28,6 @@ static timestamp_t default_reflog_expire;
 static timestamp_t default_reflog_expire_unreachable;
 
 struct cmd_reflog_expire_cb {
-	struct rev_info revs;
 	int stalefix;
 	timestamp_t expire_total;
 	timestamp_t expire_unreachable;
@@ -594,13 +593,15 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	 * from reflog if the repository was pruned with older git.
 	 */
 	if (cmd.stalefix) {
-		repo_init_revisions(the_repository, &cmd.revs, prefix);
-		cmd.revs.do_not_die_on_missing_tree = 1;
-		cmd.revs.ignore_missing = 1;
-		cmd.revs.ignore_missing_links = 1;
+		struct rev_info revs;
+
+		repo_init_revisions(the_repository, &revs, prefix);
+		revs.do_not_die_on_missing_tree = 1;
+		revs.ignore_missing = 1;
+		revs.ignore_missing_links = 1;
 		if (flags & EXPIRE_REFLOGS_VERBOSE)
 			printf(_("Marking reachable objects..."));
-		mark_reachable_objects(&cmd.revs, 0, 0, NULL);
+		mark_reachable_objects(&revs, 0, 0, NULL);
 		if (flags & EXPIRE_REFLOGS_VERBOSE)
 			putchar('\n');
 	}
-- 
2.34.1.1146.gb52885e7c44

[PATCH v3 8/9] refs files-backend: assume cb->newlog if !EXPIRE_REFLOGS_DRY_RUN

[Ævar Arnfjörð Bjarmason]

It's not possible for "cb->newlog" to be NULL if
!EXPIRE_REFLOGS_DRY_RUN, since files_reflog_expire() would have
error()'d and taken the "goto failure" branch if it couldn't open the
file. By not using the "newlog" field private to "file-backend.c"'s
"struct expire_reflog_cb", we can move this verbosity logging to
"builtin/reflog.c" in a subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 90b671025a7..5f8586a36e3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3105,12 +3105,12 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
 
 	if ((*cb->should_prune_fn)(ooid, noid, email, timestamp, tz,
 				   message, policy_cb)) {
-		if (!cb->newlog)
+		if (cb->flags & EXPIRE_REFLOGS_DRY_RUN)
 			printf("would prune %s", message);
 		else if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
 			printf("prune %s", message);
 	} else {
-		if (cb->newlog) {
+		if (!(cb->flags & EXPIRE_REFLOGS_DRY_RUN)) {
 			fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s",
 				oid_to_hex(ooid), oid_to_hex(noid),
 				email, timestamp, tz, message);
-- 
2.34.1.1146.gb52885e7c44

[PATCH v3 9/9] reflog + refs-backend: move "verbose" out of the backend

[Ævar Arnfjörð Bjarmason]

Move the handling of the "verbose" flag entirely out of
"refs/files-backend.c" and into "builtin/reflog.c". This allows the
backend to stop knowing about the EXPIRE_REFLOGS_VERBOSE flag.

The expire_reflog_ent() function shouldn't need to deal with the
implementation detail of whether or not we're emitting verbose output,
by doing this the --verbose output becomes backend-agnostic, so
reftable will get the same output.

I think the output is rather bad currently, and should e.g. be
implemented with some better future mode of progress.[ch], but that's
a topic for another improvement.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c     | 56 +++++++++++++++++++++++++++++++++++++-------
 refs.h               |  3 +--
 refs/files-backend.c | 44 ++++++++++++++++------------------
 3 files changed, 68 insertions(+), 35 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 4ff63846058..a4b1dd27e13 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -45,6 +45,7 @@ struct expire_reflog_policy_cb {
 	struct cmd_reflog_expire_cb cmd;
 	struct commit *tip_commit;
 	struct commit_list *tips;
+	unsigned int dry_run:1;
 };
 
 struct worktree_reflogs {
@@ -319,6 +320,28 @@ static int should_expire_reflog_ent(struct object_id *ooid, struct object_id *no
 	return 0;
 }
 
+static int should_expire_reflog_ent_verbose(struct object_id *ooid,
+					    struct object_id *noid,
+					    const char *email,
+					    timestamp_t timestamp, int tz,
+					    const char *message, void *cb_data)
+{
+	struct expire_reflog_policy_cb *cb = cb_data;
+	int expire;
+
+	expire = should_expire_reflog_ent(ooid, noid, email, timestamp, tz,
+					  message, cb);
+
+	if (!expire)
+		printf("keep %s", message);
+	else if (cb->dry_run)
+		printf("would prune %s", message);
+	else
+		printf("prune %s", message);
+
+	return expire;
+}
+
 static int push_tip_to_list(const char *refname, const struct object_id *oid,
 			    int flags, void *cb_data)
 {
@@ -539,6 +562,8 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	int i, status, do_all, all_worktrees = 1;
 	int explicit_expiry = 0;
 	unsigned int flags = 0;
+	int verbose = 0;
+	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
 
 	default_reflog_expire_unreachable = now - 30 * 24 * 3600;
 	default_reflog_expire = now - 90 * 24 * 3600;
@@ -576,7 +601,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		else if (!strcmp(arg, "--single-worktree"))
 			all_worktrees = 0;
 		else if (!strcmp(arg, "--verbose"))
-			flags |= EXPIRE_REFLOGS_VERBOSE;
+			verbose = 1;
 		else if (!strcmp(arg, "--")) {
 			i++;
 			break;
@@ -587,6 +612,9 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 			break;
 	}
 
+	if (verbose)
+		should_prune_fn = should_expire_reflog_ent_verbose;
+
 	/*
 	 * We can trust the commits and objects reachable from refs
 	 * even in older repository.  We cannot trust what's reachable
@@ -599,10 +627,10 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		revs.do_not_die_on_missing_tree = 1;
 		revs.ignore_missing = 1;
 		revs.ignore_missing_links = 1;
-		if (flags & EXPIRE_REFLOGS_VERBOSE)
+		if (verbose)
 			printf(_("Marking reachable objects..."));
 		mark_reachable_objects(&revs, 0, 0, NULL);
-		if (flags & EXPIRE_REFLOGS_VERBOSE)
+		if (verbose)
 			putchar('\n');
 	}
 
@@ -624,12 +652,15 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		free_worktrees(worktrees);
 
 		for_each_string_list_item(item, &collected.reflogs) {
-			struct expire_reflog_policy_cb cb = { .cmd = cmd };
+			struct expire_reflog_policy_cb cb = {
+				.cmd = cmd,
+				.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
+			};
 
 			set_reflog_expiry_param(&cb.cmd, explicit_expiry, item->string);
 			status |= reflog_expire(item->string, flags,
 						reflog_expiry_prepare,
-						should_expire_reflog_ent,
+						should_prune_fn,
 						reflog_expiry_cleanup,
 						&cb);
 		}
@@ -647,7 +678,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		set_reflog_expiry_param(&cb.cmd, explicit_expiry, ref);
 		status |= reflog_expire(ref, flags,
 					reflog_expiry_prepare,
-					should_expire_reflog_ent,
+					should_prune_fn,
 					reflog_expiry_cleanup,
 					&cb);
 		free(ref);
@@ -670,6 +701,8 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 	struct cmd_reflog_expire_cb cmd = { 0 };
 	int i, status = 0;
 	unsigned int flags = 0;
+	int verbose = 0;
+	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
@@ -680,7 +713,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"))
-			flags |= EXPIRE_REFLOGS_VERBOSE;
+			verbose = 1;
 		else if (!strcmp(arg, "--")) {
 			i++;
 			break;
@@ -691,6 +724,9 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 			break;
 	}
 
+	if (verbose)
+		should_prune_fn = should_expire_reflog_ent_verbose;
+
 	if (argc - i < 1)
 		return error(_("no reflog specified to delete"));
 
@@ -698,7 +734,9 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 		const char *spec = strstr(argv[i], "@{");
 		char *ep, *ref;
 		int recno;
-		struct expire_reflog_policy_cb cb = { 0 };
+		struct expire_reflog_policy_cb cb = {
+			.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
+		};
 
 		if (!spec) {
 			status |= error(_("not a reflog: %s"), argv[i]);
@@ -723,7 +761,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 		cb.cmd = cmd;
 		status |= reflog_expire(ref, flags,
 					reflog_expiry_prepare,
-					should_expire_reflog_ent,
+					should_prune_fn,
 					reflog_expiry_cleanup,
 					&cb);
 		free(ref);
diff --git a/refs.h b/refs.h
index 92360e55a20..8f91a7f9ff2 100644
--- a/refs.h
+++ b/refs.h
@@ -820,8 +820,7 @@ enum ref_type ref_type(const char *refname);
 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
+	EXPIRE_REFLOGS_REWRITE = 1 << 2,
 };
 
 /*
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5f8586a36e3..6178ad8c77c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3086,11 +3086,12 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 }
 
 struct expire_reflog_cb {
-	unsigned int flags;
 	reflog_expiry_should_prune_fn *should_prune_fn;
 	void *policy_cb;
 	FILE *newlog;
 	struct object_id last_kept_oid;
+	unsigned int rewrite:1,
+		     dry_run:1;
 };
 
 static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
@@ -3098,33 +3099,27 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
 			     const char *message, void *cb_data)
 {
 	struct expire_reflog_cb *cb = cb_data;
-	struct expire_reflog_policy_cb *policy_cb = cb->policy_cb;
+	reflog_expiry_should_prune_fn *fn = cb->should_prune_fn;
 
-	if (cb->flags & EXPIRE_REFLOGS_REWRITE)
+	if (cb->rewrite)
 		ooid = &cb->last_kept_oid;
 
-	if ((*cb->should_prune_fn)(ooid, noid, email, timestamp, tz,
-				   message, policy_cb)) {
-		if (cb->flags & EXPIRE_REFLOGS_DRY_RUN)
-			printf("would prune %s", message);
-		else if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
-			printf("prune %s", message);
-	} else {
-		if (!(cb->flags & EXPIRE_REFLOGS_DRY_RUN)) {
-			fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s",
-				oid_to_hex(ooid), oid_to_hex(noid),
-				email, timestamp, tz, message);
-			oidcpy(&cb->last_kept_oid, noid);
-		}
-		if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
-			printf("keep %s", message);
-	}
+	if (fn(ooid, noid, email, timestamp, tz, message, cb->policy_cb))
+		return 0;
+
+	if (cb->dry_run)
+		return 0; /* --dry-run */
+
+	fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s", oid_to_hex(ooid),
+		oid_to_hex(noid), email, timestamp, tz, message);
+	oidcpy(&cb->last_kept_oid, noid);
+
 	return 0;
 }
 
 static int files_reflog_expire(struct ref_store *ref_store,
 			       const char *refname,
-			       unsigned int flags,
+			       unsigned int expire_flags,
 			       reflog_expiry_prepare_fn prepare_fn,
 			       reflog_expiry_should_prune_fn should_prune_fn,
 			       reflog_expiry_cleanup_fn cleanup_fn,
@@ -3142,7 +3137,8 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	const struct object_id *oid;
 
 	memset(&cb, 0, sizeof(cb));
-	cb.flags = flags;
+	cb.rewrite = !!(expire_flags & EXPIRE_REFLOGS_REWRITE);
+	cb.dry_run = !!(expire_flags & EXPIRE_REFLOGS_DRY_RUN);
 	cb.policy_cb = policy_cb_data;
 	cb.should_prune_fn = should_prune_fn;
 
@@ -3178,7 +3174,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 
 	files_reflog_path(refs, &log_file_sb, refname);
 	log_file = strbuf_detach(&log_file_sb, NULL);
-	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
+	if (!cb.dry_run) {
 		/*
 		 * Even though holding $GIT_DIR/logs/$reflog.lock has
 		 * no locking implications, we use the lock_file
@@ -3205,7 +3201,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	refs_for_each_reflog_ent(ref_store, refname, expire_reflog_ent, &cb);
 	(*cleanup_fn)(cb.policy_cb);
 
-	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
+	if (!cb.dry_run) {
 		/*
 		 * It doesn't make sense to adjust a reference pointed
 		 * to by a symbolic ref based on expiring entries in
@@ -3215,7 +3211,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 		 */
 		int update = 0;
 
-		if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
+		if ((expire_flags & EXPIRE_REFLOGS_UPDATE_REF) &&
 		    !is_null_oid(&cb.last_kept_oid)) {
 			int ignore_errno;
 			int type;
-- 
2.34.1.1146.gb52885e7c44