Merge branch 'ps/pack-refs-auto'

"git pack-refs" learned the "--auto" option, which is a useful
addition to be triggered from "git gc --auto".

Acked-by: Karthik Nayak <karthik.188@gmail.com>
cf. <CAOLa=ZRAEA7rSUoYL0h-2qfEELdbPHbeGpgBJRqesyhHi9Q6WQ@mail.gmail.com>

* ps/pack-refs-auto:
  builtin/gc: pack refs when using `git maintenance run --auto`
  builtin/gc: forward git-gc(1)'s `--auto` flag when packing refs
  t6500: extract objects with "17" prefix
  builtin/gc: move `struct maintenance_run_opts`
  builtin/pack-refs: introduce new "--auto" flag
  builtin/pack-refs: release allocated memory
  refs/reftable: expose auto compaction via new flag
  refs: remove `PACK_REFS_ALL` flag
  refs: move `struct pack_refs_opts` to where it's used
  t/helper: drop pack-refs wrapper
  refs/reftable: print errors on compaction failure
  reftable/stack: gracefully handle failed auto-compaction due to locks
  reftable/stack: use error codes when locking fails during compaction
  reftable/error: discern locked/outdated errors
  reftable/stack: fix error handling in `reftable_stack_init_addition()`

14 files changed, 308 insertions, 125 deletions

diff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt
index 284956acb3..2dcabaf74c 100644
--- a/Documentation/git-pack-refs.txt
+++ b/Documentation/git-pack-refs.txt
@@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access
 SYNOPSIS
 --------
 [verse]
-'git pack-refs' [--all] [--no-prune] [--include <pattern>] [--exclude <pattern>]
+'git pack-refs' [--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]
 
 DESCRIPTION
 -----------
@@ -60,6 +60,19 @@ with many branches of historical interests.
 The command usually removes loose refs under `$GIT_DIR/refs`
 hierarchy after packing them.  This option tells it not to.
 
+--auto::
+
+Pack refs as needed depending on the current state of the ref database. The
+behavior depends on the ref format used by the repository and may change in the
+future.
++
+	- "files": No special handling for `--auto` has been implemented.
++
+	- "reftable": Tables are compacted such that they form a geometric
+	  sequence. For two tables N and N+1, where N+1 is newer, this
+	  maintains the property that N is at least twice as big as N+1. Only
+	  tables that violate this property are compacted.
+
 --include <pattern>::
 
 Pack refs based on a `glob(7)` pattern. Repetitions of this option
diff --git a/builtin/gc.c b/builtin/gc.c
index 342907f7bd..d187cec1ea 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -180,13 +180,51 @@ static void gc_config(void)
 	git_config(git_default_config, NULL);
 }
 
-struct maintenance_run_opts;
+enum schedule_priority {
+	SCHEDULE_NONE = 0,
+	SCHEDULE_WEEKLY = 1,
+	SCHEDULE_DAILY = 2,
+	SCHEDULE_HOURLY = 3,
+};
+
+static enum schedule_priority parse_schedule(const char *value)
+{
+	if (!value)
+		return SCHEDULE_NONE;
+	if (!strcasecmp(value, "hourly"))
+		return SCHEDULE_HOURLY;
+	if (!strcasecmp(value, "daily"))
+		return SCHEDULE_DAILY;
+	if (!strcasecmp(value, "weekly"))
+		return SCHEDULE_WEEKLY;
+	return SCHEDULE_NONE;
+}
+
+struct maintenance_run_opts {
+	int auto_flag;
+	int quiet;
+	enum schedule_priority schedule;
+};
+
+static int pack_refs_condition(void)
+{
+	/*
+	 * The auto-repacking logic for refs is handled by the ref backends and
+	 * exposed via `git pack-refs --auto`. We thus always return truish
+	 * here and let the backend decide for us.
+	 */
+	return 1;
+}
+
 static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 
 	cmd.git_cmd = 1;
 	strvec_pushl(&cmd.args, "pack-refs", "--all", "--prune", NULL);
+	if (opts->auto_flag)
+		strvec_push(&cmd.args, "--auto");
+
 	return run_command(&cmd);
 }
 
@@ -547,7 +585,7 @@ done:
 	return ret;
 }
 
-static void gc_before_repack(void)
+static void gc_before_repack(struct maintenance_run_opts *opts)
 {
 	/*
 	 * We may be called twice, as both the pre- and
@@ -558,7 +596,7 @@ static void gc_before_repack(void)
 	if (done++)
 		return;
 
-	if (pack_refs && maintenance_task_pack_refs(NULL))
+	if (pack_refs && maintenance_task_pack_refs(opts))
 		die(FAILED_RUN, "pack-refs");
 
 	if (prune_reflogs) {
@@ -574,7 +612,6 @@ static void gc_before_repack(void)
 int cmd_gc(int argc, const char **argv, const char *prefix)
 {
 	int aggressive = 0;
-	int auto_gc = 0;
 	int quiet = 0;
 	int force = 0;
 	const char *name;
@@ -583,6 +620,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	int keep_largest_pack = -1;
 	timestamp_t dummy;
 	struct child_process rerere_cmd = CHILD_PROCESS_INIT;
+	struct maintenance_run_opts opts = {0};
 
 	struct option builtin_gc_options[] = {
 		OPT__QUIET(&quiet, N_("suppress progress reporting")),
@@ -593,7 +631,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		OPT_MAGNITUDE(0, "max-cruft-size", &max_cruft_size,
 			      N_("with --cruft, limit the size of new cruft packs")),
 		OPT_BOOL(0, "aggressive", &aggressive, N_("be more thorough (increased runtime)")),
-		OPT_BOOL_F(0, "auto", &auto_gc, N_("enable auto-gc mode"),
+		OPT_BOOL_F(0, "auto", &opts.auto_flag, N_("enable auto-gc mode"),
 			   PARSE_OPT_NOCOMPLETE),
 		OPT_BOOL_F(0, "force", &force,
 			   N_("force running gc even if there may be another gc running"),
@@ -638,7 +676,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	if (quiet)
 		strvec_push(&repack, "-q");
 
-	if (auto_gc) {
+	if (opts.auto_flag) {
 		/*
 		 * Auto-gc should be least intrusive as possible.
 		 */
@@ -663,7 +701,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 
 			if (lock_repo_for_gc(force, &pid))
 				return 0;
-			gc_before_repack(); /* dies on failure */
+			gc_before_repack(&opts); /* dies on failure */
 			delete_tempfile(&pidfile);
 
 			/*
@@ -688,7 +726,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 
 	name = lock_repo_for_gc(force, &pid);
 	if (name) {
-		if (auto_gc)
+		if (opts.auto_flag)
 			return 0; /* be quiet on --auto */
 		die(_("gc is already running on machine '%s' pid %"PRIuMAX" (use --force if not)"),
 		    name, (uintmax_t)pid);
@@ -703,7 +741,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		atexit(process_log_file_at_exit);
 	}
 
-	gc_before_repack();
+	gc_before_repack(&opts);
 
 	if (!repository_format_precious_objects) {
 		struct child_process repack_cmd = CHILD_PROCESS_INIT;
@@ -758,7 +796,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 					     !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,
 					     NULL);
 
-	if (auto_gc && too_many_loose_objects())
+	if (opts.auto_flag && too_many_loose_objects())
 		warning(_("There are too many unreachable loose objects; "
 			"run 'git prune' to remove them."));
 
@@ -773,26 +811,6 @@ static const char *const builtin_maintenance_run_usage[] = {
 	NULL
 };
 
-enum schedule_priority {
-	SCHEDULE_NONE = 0,
-	SCHEDULE_WEEKLY = 1,
-	SCHEDULE_DAILY = 2,
-	SCHEDULE_HOURLY = 3,
-};
-
-static enum schedule_priority parse_schedule(const char *value)
-{
-	if (!value)
-		return SCHEDULE_NONE;
-	if (!strcasecmp(value, "hourly"))
-		return SCHEDULE_HOURLY;
-	if (!strcasecmp(value, "daily"))
-		return SCHEDULE_DAILY;
-	if (!strcasecmp(value, "weekly"))
-		return SCHEDULE_WEEKLY;
-	return SCHEDULE_NONE;
-}
-
 static int maintenance_opt_schedule(const struct option *opt, const char *arg,
 				    int unset)
 {
@@ -809,12 +827,6 @@ static int maintenance_opt_schedule(const struct option *opt, const char *arg,
 	return 0;
 }
 
-struct maintenance_run_opts {
-	int auto_flag;
-	int quiet;
-	enum schedule_priority schedule;
-};
-
 /* Remember to update object flag allocation in object.h */
 #define SEEN		(1u<<0)
 
@@ -1296,7 +1308,7 @@ static struct maintenance_task tasks[] = {
 	[TASK_PACK_REFS] = {
 		"pack-refs",
 		maintenance_task_pack_refs,
-		NULL,
+		pack_refs_condition,
 	},
 };
 
diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index bcf383cac9..db40825666 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -7,24 +7,28 @@
 #include "revision.h"
 
 static char const * const pack_refs_usage[] = {
-	N_("git pack-refs [--all] [--no-prune] [--include <pattern>] [--exclude <pattern>]"),
+	N_("git pack-refs [--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]"),
 	NULL
 };
 
 int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 {
-	unsigned int flags = PACK_REFS_PRUNE;
-	static struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;
-	static struct string_list included_refs = STRING_LIST_INIT_NODUP;
-	struct pack_refs_opts pack_refs_opts = { .exclusions = &excludes,
-						 .includes = &included_refs,
-						 .flags = flags };
-	static struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;
+	struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;
+	struct string_list included_refs = STRING_LIST_INIT_NODUP;
+	struct pack_refs_opts pack_refs_opts = {
+		.exclusions = &excludes,
+		.includes = &included_refs,
+		.flags = PACK_REFS_PRUNE,
+	};
+	struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;
 	struct string_list_item *item;
+	int pack_all = 0;
+	int ret;
 
 	struct option opts[] = {
-		OPT_BIT(0, "all",   &pack_refs_opts.flags, N_("pack everything"), PACK_REFS_ALL),
+		OPT_BOOL(0, "all",   &pack_all, N_("pack everything")),
 		OPT_BIT(0, "prune", &pack_refs_opts.flags, N_("prune loose refs (default)"), PACK_REFS_PRUNE),
+		OPT_BIT(0, "auto", &pack_refs_opts.flags, N_("auto-pack refs as needed"), PACK_REFS_AUTO),
 		OPT_STRING_LIST(0, "include", pack_refs_opts.includes, N_("pattern"),
 			N_("references to include")),
 		OPT_STRING_LIST(0, "exclude", &option_excluded_refs, N_("pattern"),
@@ -38,11 +42,16 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 	for_each_string_list_item(item, &option_excluded_refs)
 		add_ref_exclusion(pack_refs_opts.exclusions, item->string);
 
-	if (pack_refs_opts.flags & PACK_REFS_ALL)
+	if (pack_all)
 		string_list_append(pack_refs_opts.includes, "*");
 
 	if (!pack_refs_opts.includes->nr)
 		string_list_append(pack_refs_opts.includes, "refs/tags/*");
 
-	return refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);
+	ret = refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);
+
+	clear_ref_exclusions(&excludes);
+	string_list_clear(&included_refs, 0);
+	string_list_clear(&option_excluded_refs, 0);
+	return ret;
 }
diff --git a/refs.h b/refs.h
index 298caf6c61..d278775e08 100644
--- a/refs.h
+++ b/refs.h
@@ -66,12 +66,6 @@ const char *ref_storage_format_to_name(unsigned int ref_storage_format);
 #define RESOLVE_REF_NO_RECURSE 0x02
 #define RESOLVE_REF_ALLOW_BAD_NAME 0x04
 
-struct pack_refs_opts {
-	unsigned int flags;
-	struct ref_exclusions *exclusions;
-	struct string_list *includes;
-};
-
 const char *refs_resolve_ref_unsafe(struct ref_store *refs,
 				    const char *refname,
 				    int resolve_flags,
@@ -428,10 +422,18 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,
 /*
  * Flags for controlling behaviour of pack_refs()
  * PACK_REFS_PRUNE: Prune loose refs after packing
- * PACK_REFS_ALL:   Pack _all_ refs, not just tags and already packed refs
+ * PACK_REFS_AUTO: Pack refs on a best effort basis. The heuristics and end
+ *                 result are decided by the ref backend. Backends may ignore
+ *                 this flag and fall back to a normal repack.
  */
-#define PACK_REFS_PRUNE 0x0001
-#define PACK_REFS_ALL   0x0002
+#define PACK_REFS_PRUNE (1 << 0)
+#define PACK_REFS_AUTO  (1 << 1)
+
+struct pack_refs_opts {
+	unsigned int flags;
+	struct ref_exclusions *exclusions;
+	struct string_list *includes;
+};
 
 /*
  * Write a packed-refs file for the current repository.
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index e206d5a073..0bed6d2ab4 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1203,9 +1203,16 @@ static int reftable_be_pack_refs(struct ref_store *ref_store,
 	if (!stack)
 		stack = refs->main_stack;
 
-	ret = reftable_stack_compact_all(stack, NULL);
-	if (ret)
+	if (opts->flags & PACK_REFS_AUTO)
+		ret = reftable_stack_auto_compact(stack);
+	else
+		ret = reftable_stack_compact_all(stack, NULL);
+	if (ret < 0) {
+		ret = error(_("unable to compact stack: %s"),
+			    reftable_error_str(ret));
 		goto out;
+	}
+
 	ret = reftable_stack_clean(stack);
 	if (ret)
 		goto out;
diff --git a/reftable/error.c b/reftable/error.c
index 0d1766735e..cfb7a0fda4 100644
--- a/reftable/error.c
+++ b/reftable/error.c
@@ -22,7 +22,7 @@ const char *reftable_error_str(int err)
 	case REFTABLE_NOT_EXIST_ERROR:
 		return "file does not exist";
 	case REFTABLE_LOCK_ERROR:
-		return "data is outdated";
+		return "data is locked";
 	case REFTABLE_API_ERROR:
 		return "misuse of the reftable API";
 	case REFTABLE_ZLIB_ERROR:
@@ -35,6 +35,8 @@ const char *reftable_error_str(int err)
 		return "invalid refname";
 	case REFTABLE_ENTRY_TOO_BIG_ERROR:
 		return "entry too large";
+	case REFTABLE_OUTDATED_ERROR:
+		return "data concurrently modified";
 	case -1:
 		return "general error";
 	default:
diff --git a/reftable/reftable-error.h b/reftable/reftable-error.h
index 4c457aaaf8..e9b07c9f36 100644
--- a/reftable/reftable-error.h
+++ b/reftable/reftable-error.h
@@ -25,7 +25,7 @@ enum reftable_error {
 	 */
 	REFTABLE_NOT_EXIST_ERROR = -4,
 
-	/* Trying to write out-of-date data. */
+	/* Trying to access locked data. */
 	REFTABLE_LOCK_ERROR = -5,
 
 	/* Misuse of the API:
@@ -57,6 +57,9 @@ enum reftable_error {
 	/* Entry does not fit. This can happen when writing outsize reflog
 	   messages. */
 	REFTABLE_ENTRY_TOO_BIG_ERROR = -11,
+
+	/* Trying to write out-of-date data. */
+	REFTABLE_OUTDATED_ERROR = -12,
 };
 
 /* convert the numeric error code to a string. The string should not be
diff --git a/reftable/stack.c b/reftable/stack.c
index 1ecf1b9751..dde50b61d6 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -529,9 +529,9 @@ int reftable_stack_add(struct reftable_stack *st,
 {
 	int err = stack_try_add(st, write, arg);
 	if (err < 0) {
-		if (err == REFTABLE_LOCK_ERROR) {
+		if (err == REFTABLE_OUTDATED_ERROR) {
 			/* Ignore error return, we want to propagate
-			   REFTABLE_LOCK_ERROR.
+			   REFTABLE_OUTDATED_ERROR.
 			*/
 			reftable_stack_reload(st);
 		}
@@ -590,9 +590,8 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 	err = stack_uptodate(st);
 	if (err < 0)
 		goto done;
-
-	if (err > 1) {
-		err = REFTABLE_LOCK_ERROR;
+	if (err > 0) {
+		err = REFTABLE_OUTDATED_ERROR;
 		goto done;
 	}
 
@@ -681,8 +680,19 @@ int reftable_addition_commit(struct reftable_addition *add)
 	if (err)
 		goto done;
 
-	if (!add->stack->disable_auto_compact)
+	if (!add->stack->disable_auto_compact) {
+		/*
+		 * Auto-compact the stack to keep the number of tables in
+		 * control. It is possible that a concurrent writer is already
+		 * trying to compact parts of the stack, which would lead to a
+		 * `REFTABLE_LOCK_ERROR` because parts of the stack are locked
+		 * already. This is a benign error though, so we ignore it.
+		 */
 		err = reftable_stack_auto_compact(add->stack);
+		if (err < 0 && err != REFTABLE_LOCK_ERROR)
+			goto done;
+		err = 0;
+	}
 
 done:
 	reftable_addition_close(add);
@@ -713,10 +723,6 @@ static int stack_try_add(struct reftable_stack *st,
 	int err = reftable_stack_init_addition(&add, st);
 	if (err < 0)
 		goto done;
-	if (err > 0) {
-		err = REFTABLE_LOCK_ERROR;
-		goto done;
-	}
 
 	err = reftable_addition_add(&add, write_table, arg);
 	if (err < 0)
@@ -978,7 +984,15 @@ done:
 	return err;
 }
 
-/* <  0: error. 0 == OK, > 0 attempt failed; could retry. */
+/*
+ * Compact all tables in the range `[first, last)` into a single new table.
+ *
+ * This function returns `0` on success or a code `< 0` on failure. When the
+ * stack or any of the tables in the specified range are already locked then
+ * this function returns `REFTABLE_LOCK_ERROR`. This is a benign error that
+ * callers can either ignore, or they may choose to retry compaction after some
+ * amount of time.
+ */
 static int stack_compact_range(struct reftable_stack *st,
 			       size_t first, size_t last,
 			       struct reftable_log_expiry_config *expiry)
@@ -1008,7 +1022,7 @@ static int stack_compact_range(struct reftable_stack *st,
 					LOCK_NO_DEREF);
 	if (err < 0) {
 		if (errno == EEXIST)
-			err = 1;
+			err = REFTABLE_LOCK_ERROR;
 		else
 			err = REFTABLE_IO_ERROR;
 		goto done;
@@ -1030,7 +1044,7 @@ static int stack_compact_range(struct reftable_stack *st,
 						table_name.buf, LOCK_NO_DEREF);
 		if (err < 0) {
 			if (errno == EEXIST)
-				err = 1;
+				err = REFTABLE_LOCK_ERROR;
 			else
 				err = REFTABLE_IO_ERROR;
 			goto done;
@@ -1080,7 +1094,7 @@ static int stack_compact_range(struct reftable_stack *st,
 					LOCK_NO_DEREF);
 	if (err < 0) {
 		if (errno == EEXIST)
-			err = 1;
+			err = REFTABLE_LOCK_ERROR;
 		else
 			err = REFTABLE_IO_ERROR;
 		goto done;
@@ -1192,7 +1206,7 @@ static int stack_compact_range_stats(struct reftable_stack *st,
 				     struct reftable_log_expiry_config *config)
 {
 	int err = stack_compact_range(st, first, last, config);
-	if (err > 0)
+	if (err == REFTABLE_LOCK_ERROR)
 		st->stats.failures++;
 	return err;
 }
diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index 0dc9a44648..351e35bd86 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -242,7 +242,7 @@ static void test_reftable_stack_uptodate(void)
 	EXPECT_ERR(err);
 
 	err = reftable_stack_add(st2, &write_test_ref, &ref2);
-	EXPECT(err == REFTABLE_LOCK_ERROR);
+	EXPECT(err == REFTABLE_OUTDATED_ERROR);
 
 	err = reftable_stack_reload(st2);
 	EXPECT_ERR(err);
@@ -353,6 +353,49 @@ static void test_reftable_stack_transaction_api_performs_auto_compaction(void)
 	clear_dir(dir);
 }
 
+static void test_reftable_stack_auto_compaction_fails_gracefully(void)
+{
+	struct reftable_ref_record ref = {
+		.refname = "refs/heads/master",
+		.update_index = 1,
+		.value_type = REFTABLE_REF_VAL1,
+		.value.val1 = {0x01},
+	};
+	struct reftable_write_options cfg = {0};
+	struct reftable_stack *st;
+	struct strbuf table_path = STRBUF_INIT;
+	char *dir = get_tmp_dir(__LINE__);
+	int err;
+
+	err = reftable_new_stack(&st, dir, cfg);
+	EXPECT_ERR(err);
+
+	err = reftable_stack_add(st, write_test_ref, &ref);
+	EXPECT_ERR(err);
+	EXPECT(st->merged->stack_len == 1);
+	EXPECT(st->stats.attempts == 0);
+	EXPECT(st->stats.failures == 0);
+
+	/*
+	 * Lock the newly written table such that it cannot be compacted.
+	 * Adding a new table to the stack should not be impacted by this, even
+	 * though auto-compaction will now fail.
+	 */
+	strbuf_addf(&table_path, "%s/%s.lock", dir, st->readers[0]->name);
+	write_file_buf(table_path.buf, "", 0);
+
+	ref.update_index = 2;
+	err = reftable_stack_add(st, write_test_ref, &ref);
+	EXPECT_ERR(err);
+	EXPECT(st->merged->stack_len == 2);
+	EXPECT(st->stats.attempts == 1);
+	EXPECT(st->stats.failures == 1);
+
+	reftable_stack_destroy(st);
+	strbuf_release(&table_path);
+	clear_dir(dir);
+}
+
 static void test_reftable_stack_validate_refname(void)
 {
 	struct reftable_write_options cfg = { 0 };
@@ -1095,6 +1138,7 @@ int stack_test_main(int argc, const char *argv[])
 	RUN_TEST(test_reftable_stack_tombstone);
 	RUN_TEST(test_reftable_stack_transaction_api);
 	RUN_TEST(test_reftable_stack_transaction_api_performs_auto_compaction);
+	RUN_TEST(test_reftable_stack_auto_compaction_fails_gracefully);
 	RUN_TEST(test_reftable_stack_update_index_check);
 	RUN_TEST(test_reftable_stack_uptodate);
 	RUN_TEST(test_reftable_stack_validate_refname);
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 7a0f6cac53..82bbf6e2e6 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -112,25 +112,6 @@ static const char **get_store(const char **argv, struct ref_store **refs)
 	return argv + 1;
 }
 
-static struct flag_definition pack_flags[] = { FLAG_DEF(PACK_REFS_PRUNE),
-					       FLAG_DEF(PACK_REFS_ALL),
-					       { NULL, 0 } };
-
-static int cmd_pack_refs(struct ref_store *refs, const char **argv)
-{
-	unsigned int flags = arg_flags(*argv++, "flags", pack_flags);
-	static struct ref_exclusions exclusions = REF_EXCLUSIONS_INIT;
-	static struct string_list included_refs = STRING_LIST_INIT_NODUP;
-	struct pack_refs_opts pack_opts = { .flags = flags,
-					    .exclusions = &exclusions,
-					    .includes = &included_refs };
-
-	if (pack_opts.flags & PACK_REFS_ALL)
-		string_list_append(pack_opts.includes, "*");
-
-	return refs_pack_refs(refs, &pack_opts);
-}
-
 static int cmd_create_symref(struct ref_store *refs, const char **argv)
 {
 	const char *refname = notnull(*argv++, "refname");
@@ -326,7 +307,6 @@ struct command {
 };
 
 static struct command commands[] = {
-	{ "pack-refs", cmd_pack_refs },
 	{ "create-symref", cmd_create_symref },
 	{ "delete-refs", cmd_delete_refs },
 	{ "rename-ref", cmd_rename_ref },
diff --git a/t/oid-info/hash-info b/t/oid-info/hash-info
index d0736dd1a0..b8a5bcb187 100644
--- a/t/oid-info/hash-info
+++ b/t/oid-info/hash-info
@@ -15,3 +15,15 @@ empty_blob sha256:473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a3037218
 
 empty_tree sha1:4b825dc642cb6eb9a060e54bf8d69288fbee4904
 empty_tree sha256:6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321
+
+blob17_1 sha1:263
+blob17_1 sha256:34
+
+blob17_2 sha1:410
+blob17_2 sha256:174
+
+blob17_3 sha1:523
+blob17_3 sha256:313
+
+blob17_4 sha1:790
+blob17_4 sha256:481
diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
index c309d2bae8..7d4ab0b91a 100755
--- a/t/t0601-reffiles-pack-refs.sh
+++ b/t/t0601-reffiles-pack-refs.sh
@@ -32,11 +32,16 @@ test_expect_success 'prepare a trivial repository' '
 	HEAD=$(git rev-parse --verify HEAD)
 '
 
-test_expect_success 'pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE)' '
-	N=`find .git/refs -type f | wc -l` &&
+test_expect_success 'pack-refs --prune --all' '
+	test_path_is_missing .git/packed-refs &&
+	git pack-refs --no-prune --all &&
+	test_path_is_file .git/packed-refs &&
+	N=$(find .git/refs -type f | wc -l) &&
 	test "$N" != 0 &&
-	test-tool ref-store main pack-refs PACK_REFS_PRUNE,PACK_REFS_ALL &&
-	N=`find .git/refs -type f` &&
+
+	git pack-refs --prune --all &&
+	test_path_is_file .git/packed-refs &&
+	N=$(find .git/refs -type f) &&
 	test -z "$N"
 '
 
@@ -159,6 +164,13 @@ test_expect_success 'test --exclude takes precedence over --include' '
 	git pack-refs --include "refs/heads/pack*" --exclude "refs/heads/pack*" &&
 	test -f .git/refs/heads/dont_pack5'
 
+test_expect_success '--auto packs and prunes refs as usual' '
+	git branch auto &&
+	test_path_is_file .git/refs/heads/auto &&
+	git pack-refs --auto --all &&
+	test_path_is_missing .git/refs/heads/auto
+'
+
 test_expect_success 'see if up-to-date packed refs are preserved' '
 	git branch q &&
 	git pack-refs --all --prune &&
@@ -358,4 +370,14 @@ test_expect_success 'pack-refs does not drop broken refs during deletion' '
 	test_cmp expect actual
 '
 
+test_expect_success 'maintenance --auto unconditionally packs loose refs' '
+	git update-ref refs/heads/something HEAD &&
+	test_path_is_file .git/refs/heads/something &&
+	git rev-parse refs/heads/something >expect &&
+	git maintenance run --task=pack-refs --auto &&
+	test_path_is_missing .git/refs/heads/something &&
+	git rev-parse refs/heads/something >actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index 686781192e..931d888bbb 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -340,6 +340,26 @@ test_expect_success 'ref transaction: empty transaction in empty repo' '
 	EOF
 '
 
+test_expect_success 'ref transaction: fails gracefully when auto compaction fails' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		test_commit A &&
+		for i in $(test_seq 10)
+		do
+			git branch branch-$i &&
+			for table in .git/reftable/*.ref
+			do
+				touch "$table.lock" || exit 1
+			done ||
+			exit 1
+		done &&
+		test_line_count = 13 .git/reftable/tables.list
+	)
+'
+
 test_expect_success 'pack-refs: compacts tables' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&
@@ -355,6 +375,65 @@ test_expect_success 'pack-refs: compacts tables' '
 	test_line_count = 1 repo/.git/reftable/tables.list
 '
 
+test_expect_success 'pack-refs: compaction raises locking errors' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	test_commit -C repo A &&
+	touch repo/.git/reftable/tables.list.lock &&
+	cat >expect <<-EOF &&
+	error: unable to compact stack: data is locked
+	EOF
+	test_must_fail git -C repo pack-refs 2>err &&
+	test_cmp expect err
+'
+
+for command in pack-refs gc "maintenance run --task=pack-refs"
+do
+test_expect_success "$command: auto compaction" '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		test_commit A &&
+
+		# We need a bit of setup to ensure that git-gc(1) actually
+		# triggers, and that it does not write anything to the refdb.
+		git config gc.auto 1 &&
+		git config gc.autoDetach 0 &&
+		git config gc.reflogExpire never &&
+		git config gc.reflogExpireUnreachable never &&
+		test_oid blob17_1 | git hash-object -w --stdin &&
+
+		# The tables should have been auto-compacted, and thus auto
+		# compaction should not have to do anything.
+		ls -1 .git/reftable >tables-expect &&
+		test_line_count = 4 tables-expect &&
+		git $command --auto &&
+		ls -1 .git/reftable >tables-actual &&
+		test_cmp tables-expect tables-actual &&
+
+		test_oid blob17_2 | git hash-object -w --stdin &&
+
+		# Lock all tables write some refs. Auto-compaction will be
+		# unable to compact tables and thus fails gracefully, leaving
+		# the stack in a sub-optimal state.
+		ls .git/reftable/*.ref |
+		while read table
+		do
+			touch "$table.lock" || exit 1
+		done &&
+		git branch B &&
+		git branch C &&
+		rm .git/reftable/*.lock &&
+		test_line_count = 5 .git/reftable/tables.list &&
+
+		git $command --auto &&
+		test_line_count = 1 .git/reftable/tables.list
+	)
+'
+done
+
 test_expect_success 'pack-refs: prunes stale tables' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 18fe1c25e6..43d40175f8 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -11,23 +11,7 @@ test_expect_success 'setup' '
 	# behavior, make sure we always pack everything to one pack by
 	# default
 	git config gc.bigPackThreshold 2g &&
-
-	# These are simply values which, when hashed as a blob with a newline,
-	# produce a hash where the first byte is 0x17 in their respective
-	# algorithms.
-	test_oid_cache <<-EOF
-	obj1 sha1:263
-	obj1 sha256:34
-
-	obj2 sha1:410
-	obj2 sha256:174
-
-	obj3 sha1:523
-	obj3 sha256:313
-
-	obj4 sha1:790
-	obj4 sha256:481
-	EOF
+	test_oid_init
 '
 
 test_expect_success 'gc empty repository' '
@@ -114,8 +98,8 @@ test_expect_success 'pre-auto-gc hook can stop auto gc' '
 		# We need to create two object whose sha1s start with 17
 		# since this is what git gc counts.  As it happens, these
 		# two blobs will do so.
-		test_commit "$(test_oid obj1)" &&
-		test_commit "$(test_oid obj2)" &&
+		test_commit "$(test_oid blob17_1)" &&
+		test_commit "$(test_oid blob17_2)" &&
 
 		git gc --auto >../out.actual 2>../err.actual
 	) &&
@@ -146,13 +130,13 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre
 	# We need to create two object whose sha1s start with 17
 	# since this is what git gc counts.  As it happens, these
 	# two blobs will do so.
-	test_commit "$(test_oid obj1)" &&
-	test_commit "$(test_oid obj2)" &&
+	test_commit "$(test_oid blob17_1)" &&
+	test_commit "$(test_oid blob17_2)" &&
 	# Our first gc will create a pack; our second will create a second pack
 	git gc --auto &&
 	ls .git/objects/pack/pack-*.pack | sort >existing_packs &&
-	test_commit "$(test_oid obj3)" &&
-	test_commit "$(test_oid obj4)" &&
+	test_commit "$(test_oid blob17_3)" &&
+	test_commit "$(test_oid blob17_4)" &&
 
 	git gc --auto 2>err &&
 	test_grep ! "^warning:" err &&