[PATCH 00/15] refs: introduce `--auto` to pack refs as needed

[PATCH 00/15] refs: introduce `--auto` to pack refs as needed

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4220 bytes --]

Hi,

right now, it is left to the caller of `refs_pack_refs()` to decide
whether or not to repack refs and how exactly to do it. This is
inefficient at times given that the caller typically has no idea whether
or not the refdb is close to optimal already. So even if the refdb is
already in a close-to-optimal state we would end up repacking all of it.

This patch series aims to address this shortcoming. It introduces a new
flag `PACK_REFS_AUTO` that tells the ref backend to repack refs as
needed. For the "files" backend we don't honor this flag yet and thus
end up behaving the exact same as if that flag wasn't set. But for the
"reftable" backend we will use the same auto-compaction algorithm that
is already used during writes to the refdb. Thus, in most of the cases
it wouldn't actually do anything except when the refdb is suboptimally
packed.

Eventually we'll probably also want to wire up heuristics for the
"files" backend to honor the `PACK_REFS_AUTO` flag. Using something like
a ratio of packed-refs size/number of loose refs might be viable. I
punted on that though as I feared that it might lead to bikeshedding and
thus distract from the main goal of this topic, which is to prepare the
ref code and relevant commands to perform optimizations as required. I'm
happy to add such a patch to this series though in case anybody feels
strongly about this.

The `PACK_REFS_AUTO` flag is exposed via a new `git pack-refs --auto`
flag. It is wired up in both `git gc --auto` and `git maintenance run
--auto`.

The series is structured as follows:

    - Patches 1 - 5: Bugfixes and improvements for the reftable-specific
      compaction code. These are issues that I've found while working on
      this series.

    - Patches 6 - 8: Refactorings to drop the `PACK_REFS_ALL` flag,
      which isn't actually used by the ref backends anymore and confused
      me multiple times.

    - Patches 9 - 15: Handling of `PACK_REFS_ALL` in git-pack-refs(1),
      git-gc(1) and git-maintenance(1).

The patch series is built on top of 2953d95d40 (The eighth batch,
2024-03-15) with "ps/reftable-stack-tempfile" at 60c4c42515
(reftable/stack: register compacted tables as tempfiles, 2024-03-07)
merged into it due to a merge conflict in "reftable/stack.c".

Patrick

PS: I polished this patch series while traveling and am still out of
    office until Thursday. I'll thus only get to respond to threads that
    are waiting for my input at the end of this week.

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

 Documentation/git-pack-refs.txt | 15 +++++-
 builtin/gc.c                    | 86 +++++++++++++++++++--------------
 builtin/pack-refs.c             | 31 +++++++-----
 refs.h                          | 20 ++++----
 refs/reftable-backend.c         | 11 ++++-
 reftable/error.c                |  4 +-
 reftable/reftable-error.h       |  5 +-
 reftable/stack.c                | 34 +++++++------
 reftable/stack_test.c           |  2 +-
 t/helper/test-ref-store.c       | 20 --------
 t/oid-info/hash-info            | 12 +++++
 t/t0601-reffiles-pack-refs.sh   | 30 ++++++++++--
 t/t0610-reftable-basics.sh      | 79 ++++++++++++++++++++++++++++++
 t/t6500-gc.sh                   | 30 +++---------
 14 files changed, 255 insertions(+), 124 deletions(-)


base-commit: f94fee98b456ef39a5f85fff5802f4e538a4dc7b
-- 
2.44.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 01/15] reftable/stack: fix error handling in `reftable_stack_init_addition()`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1566 bytes --]

In `reftable_stack_init_addition()` we call `stack_uptodate()` after
having created the lockfile to check whether the stack was modified
concurrently, which is indicated by a positive return code from the
latter function. If so, we return a `REFTABLE_LOCK_ERROR` to the caller
and abort the addition.

The error handling has an off-by-one though because we check whether the
error code is `> 1` instead of `> 0`. Thus, instead of returning the
locking error, we would return a positive value. One of the callers of
`reftable_stack_init_addition()` works around this bug by repeating the
error code check without the off-by-one. But other callers are subtly
broken by this bug.

Fix this by checking for `err > 0` instead.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index 1ecf1b9751..92d9a7facb 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -590,8 +590,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 	err = stack_uptodate(st);
 	if (err < 0)
 		goto done;
-
-	if (err > 1) {
+	if (err > 0) {
 		err = REFTABLE_LOCK_ERROR;
 		goto done;
 	}
@@ -713,10 +712,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)
-- 
2.44.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 02/15] reftable/error: discern locked/outdated errors

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3431 bytes --]

We currently throw two different errors into a similar-but-different
error code:

  - Errors when trying to lock the reftable stack.

  - Errors when trying to write to the reftable stack which has been
    modified concurrently.

This results in unclear error handling and user-visible error messages.

Create a new `REFTABLE_OUTDATED_ERROR` so that those error conditions
can be clearly told apart from each other. Adjust users of the old
`REFTABLE_LOCK_ERROR` to use the new error code as required.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/error.c          | 4 +++-
 reftable/reftable-error.h | 5 ++++-
 reftable/stack.c          | 6 +++---
 reftable/stack_test.c     | 2 +-
 4 files changed, 11 insertions(+), 6 deletions(-)

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 92d9a7facb..eaa8bb9c99 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);
 		}
@@ -591,7 +591,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 	if (err < 0)
 		goto done;
 	if (err > 0) {
-		err = REFTABLE_LOCK_ERROR;
+		err = REFTABLE_OUTDATED_ERROR;
 		goto done;
 	}
 
diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index 509f486623..b0c7041a4f 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -232,7 +232,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);
-- 
2.44.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 03/15] reftable/stack: use error codes when locking fails during compaction

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2424 bytes --]

Compaction of a reftable stack may fail gracefully when there is a
concurrent process that writes to the reftable stack and which has thus
locked either the "tables.list" file or one of the tables. This is
expected and can be handled gracefully by some of the callers which
invoke compaction. Thus, to indicate this situation to our callers, we
return a positive return code from `stack_compact_range()` and bubble it
up to the caller.

This kind of error handling is somewhat awkward though as many callers
in the call chain never even think of handling positive return values.
Thus, the result is either that such errors are swallowed by accident,
or that we abort operations with an unhelpful error message.

Make the code more robust by always using negative error codes when
compaction fails, with `REFTABLE_LOCK_ERROR` for the described benign
error case.

Note that only a single callsite knew to handle positive error codes
gracefully in the first place. Subsequent commits will touch up some of
the other sites to handle those errors better.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index eaa8bb9c99..ba15c48ddd 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -1003,7 +1003,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;
@@ -1025,7 +1025,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;
@@ -1075,7 +1075,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;
@@ -1187,7 +1187,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;
 }
-- 
2.44.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 04/15] reftable/stack: gracefully handle failed auto-compaction due to locks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2388 bytes --]

Whenever we commit a new table to the reftable stack we will end up
invoking auto-compaction of the stack to keep the total number of tables
at bay. This auto-compaction may fail though in case at least one of the
tables which we are about to compact is locked. This is indicated by the
compaction function returning a positive value. We do not handle this
case though, and thus bubble that return value up the calling chain,
which will ultimately cause a failure.

Fix this bug by handling positive return values.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c           | 13 ++++++++++++-
 t/t0610-reftable-basics.sh | 20 ++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index ba15c48ddd..dd5160d751 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -680,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. Note that we explicitly ignore locking issues which
+		 * may indicate that a concurrent process is already trying to
+		 * compact tables. This is fine, so we simply ignore that error
+		 * condition.
+		 */
 		err = reftable_stack_auto_compact(add->stack);
+		if (err < 0 && err != REFTABLE_LOCK_ERROR)
+			goto done;
+		err = 0;
+	}
 
 done:
 	reftable_addition_close(add);
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index 686781192e..5f2f9baa9b 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 &&
-- 
2.44.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 05/15] refs/reftable: print errors on compaction failure

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1703 bytes --]

When git-pack-refs(1) fails in the reftable backend we end up printing
no error message at all, leaving the caller puzzled as to why compaction
has failed. Fix this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c    |  6 +++++-
 t/t0610-reftable-basics.sh | 12 ++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 74dab18eda..66cdbbdb24 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1221,8 +1221,12 @@ static int reftable_be_pack_refs(struct ref_store *ref_store,
 		stack = refs->main_stack;
 
 	ret = reftable_stack_compact_all(stack, NULL);
-	if (ret)
+	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/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index 5f2f9baa9b..a53d1dc493 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -375,6 +375,18 @@ 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
+'
+
 test_expect_success 'pack-refs: prunes stale tables' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&
-- 
2.44.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 06/15] t/helper: drop pack-refs wrapper

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2972 bytes --]

The test helper provides a "ref-store <store> pack-refs" wrapper that
more or less directly invokes `refs_pack_refs()`. This helper is only
used in a single test with the "PACK_REFS_PRUNE" and "PACK_REFS_ALL"
flags. Both of these flags can directly be accessed via git-pack-refs(1)
though via the `--all` and `--prune` flags, which makes the helper
superfluous.

Refactor the test to use git-pack-refs(1) instead of the test helper.
Drop the now-unused test helper command.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/helper/test-ref-store.c     | 20 --------------------
 t/t0601-reffiles-pack-refs.sh | 13 +++++++++----
 2 files changed, 9 insertions(+), 24 deletions(-)

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/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
index c309d2bae8..b1cf587347 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"
 '
 
-- 
2.44.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 07/15] refs: move `struct pack_refs_opts` to where it's used

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1223 bytes --]

The declaration of `struct pack_refs_opts` is in a seemingly random
place. Move it so that it's located right next to its flags and
functions that use it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/refs.h b/refs.h
index 298caf6c61..139ce7113b 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,
@@ -433,6 +427,12 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,
 #define PACK_REFS_PRUNE 0x0001
 #define PACK_REFS_ALL   0x0002
 
+struct pack_refs_opts {
+	unsigned int flags;
+	struct ref_exclusions *exclusions;
+	struct string_list *includes;
+};
+
 /*
  * Write a packed-refs file for the current repository.
  * flags: Combination of the above PACK_REFS_* flags.
-- 
2.44.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 08/15] refs: remove `PACK_REFS_ALL` flag

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2464 bytes --]

The intent of the `PACK_REFS_ALL` flag is to ask the backend to compact
all refs instead of only a subset of them. Thus, this flag gets passed
down to `refs_pack_refs()` via `struct pack_refs_opts::flags`.

But starting with 4fe42f326e (pack-refs: teach pack-refs --include
option, 2023-05-12), the flag's semantics have changed. Instead of being
handled by the respective backends, this flag is now getting handled by
the callers of `refs_pack_refs()` which will add a single glob ("*") to
the list of refs-to-be-packed. Thus, the flag serves no purpose to the
ref backends anymore.

Remove the flag and replace it with a local variable.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/pack-refs.c | 5 +++--
 refs.h              | 2 --
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index bcf383cac9..97921beef2 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -21,9 +21,10 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 						 .flags = flags };
 	static struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;
 	struct string_list_item *item;
+	int pack_all = 0;
 
 	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_STRING_LIST(0, "include", pack_refs_opts.includes, N_("pattern"),
 			N_("references to include")),
@@ -38,7 +39,7 @@ 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)
diff --git a/refs.h b/refs.h
index 139ce7113b..8c8994cb29 100644
--- a/refs.h
+++ b/refs.h
@@ -422,10 +422,8 @@ 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
  */
 #define PACK_REFS_PRUNE 0x0001
-#define PACK_REFS_ALL   0x0002
 
 struct pack_refs_opts {
 	unsigned int flags;
-- 
2.44.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 09/15] refs/reftable: expose auto compaction via new flag

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3297 bytes --]

Under normal circumstances, the "reftable" backend will automatically
perform compaction after appending to the stack. It is thus not
necessary and may even be considered wasteful to run git-pack-refs(1) in
"reftable"-backed repositories as it will cause the backend to compact
all tables into a single one. We do exactly that though when running
`git maintenance run --auto` or `git gc --auto`, which gets spawned by
Git after running some specific commands.

The `--auto` mode is typically only executing optimizations as needed.
To do so, we already use several heuristics for the various different
data structures in Git to determine whether to optimize them or not.
We do not use any heuristics for refs though and instead always optimize
them.

Introduce a new `PACK_REFS_AUTO` flag that can be passed to the backend.
When not handled by the backend we will continue to behave the exact
same as we do right now, that is we optimize refs unconditionally. This
is done for the "files" backend for now to retain current behaviour,
even though we may eventually also want to introduce heuristics here.
For the "reftable" backend though we already do have auto-compaction, so
we can easily reuse that logic to implement the new auto-packing flag.

Note that under normal circumstances, this should always end up being a
no-op. After all, we already invoke the code for every single addition
to the stack. But there are special cases where it can still be helpful
to execute the auto-compaction code explicitly:

  - Concurrent writers may cause compaction to not run due to locks.

  - Callers may decide to disable compaction altogether and then pack
    refs at a later point due to various reasons.

  - Other implementations of the reftable format may do compaction
    differently or even not at all.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.h                  | 6 +++++-
 refs/reftable-backend.c | 5 ++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/refs.h b/refs.h
index 8c8994cb29..d278775e08 100644
--- a/refs.h
+++ b/refs.h
@@ -422,8 +422,12 @@ 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_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_PRUNE (1 << 0)
+#define PACK_REFS_AUTO  (1 << 1)
 
 struct pack_refs_opts {
 	unsigned int flags;
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 66cdbbdb24..135bd4e268 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1220,7 +1220,10 @@ 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 (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));
-- 
2.44.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 10/15] builtin/pack-refs: release allocated memory

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2237 bytes --]

Some of the command line options in `cmd_pack_refs()` require us to
allocate memory. This memory is never released and thus leaking, but we
paper over this leak by declaring the respective variables as `static`
function-level variables, which is somewhat awkward.

Refactor the code to release the allocated memory and drop the `static`
declaration. While at it, remove the useless `flags` variable.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/pack-refs.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index 97921beef2..ea2baeec76 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -13,15 +13,17 @@ static char const * const pack_refs_usage[] = {
 
 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_BOOL(0, "all",   &pack_all, N_("pack everything")),
@@ -45,5 +47,10 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 	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;
 }
-- 
2.44.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 11/15] builtin/pack-refs: introduce new "--auto" flag

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5609 bytes --]

Calling git-pack-refs(1) will unconditionally cause it to pack all
requested refs regardless of the current state of the ref database. For
example:

  - With the "files" backend we will end up rewriting the complete
    "packed-refs" file even if only a single ref would require
    compaction.

  - With the "reftable" backend we will end up always compacting all
    tables into a single table.

This behaviour can be completely unnecessary depending on the backend
and is thus wasteful.

With the introduction of the `PACK_REFS_AUTO` flag in the preceding
commit we can improve this and let the backends decide for themselves
whether to pack refs in the first place. Expose this functionality via a
new "--auto" flag in git-pack-refs(1), which mirrors the same flag in
both git-gc(1) and git-maintenance(1).

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git-pack-refs.txt | 15 ++++++++++++++-
 builtin/pack-refs.c             |  3 ++-
 t/t0601-reffiles-pack-refs.sh   |  7 +++++++
 t/t0610-reftable-basics.sh      | 34 +++++++++++++++++++++++++++++++++
 4 files changed, 57 insertions(+), 2 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/pack-refs.c b/builtin/pack-refs.c
index ea2baeec76..db40825666 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -7,7 +7,7 @@
 #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
 };
 
@@ -28,6 +28,7 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 	struct option opts[] = {
 		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"),
diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
index b1cf587347..219a495451 100755
--- a/t/t0601-reffiles-pack-refs.sh
+++ b/t/t0601-reffiles-pack-refs.sh
@@ -164,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 &&
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index a53d1dc493..6de7529575 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -387,6 +387,40 @@ test_expect_success 'pack-refs: compaction raises locking errors' '
 	test_cmp expect err
 '
 
+test_expect_success 'pack-refs: auto compaction' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		test_commit A &&
+
+		# 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 pack-refs --auto &&
+		ls -1 .git/reftable >tables-actual &&
+		test_cmp tables-expect tables-actual &&
+
+		# 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 pack-refs --auto &&
+		test_line_count = 1 .git/reftable/tables.list
+	)
+'
+
 test_expect_success 'pack-refs: prunes stale tables' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&
-- 
2.44.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 12/15] builtin/gc: move `struct maintenance_run_opts`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2284 bytes --]

We're about to start using `struct maintenance_run_opts` in
`maintenance_task_pack_refs()`. Move its definition up to prepare for
this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 53 ++++++++++++++++++++++++++--------------------------
 1 file changed, 26 insertions(+), 27 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index cb80ced6cb..e0029c88f9 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -180,7 +180,32 @@ 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 maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
@@ -773,26 +798,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 +814,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)
 
-- 
2.44.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 13/15] t6500: extract objects with "17" prefix

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3221 bytes --]

The ".git/obects/17/" shard is somewhat special because it is used by
git-gc(1) to estimate how many objects there are by extrapolating the
number of objects in that shard, only. In t6500 we thus have a hard
coded set of data that, when written to the object database, result in
blobs starting with that prefix.

We are about to need such "17"-prefixed objects in another test suite.
Extract them into "t/oid-info/hash-info" so that they can be reused by
other tests.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/oid-info/hash-info | 12 ++++++++++++
 t/t6500-gc.sh        | 30 +++++++-----------------------
 2 files changed, 19 insertions(+), 23 deletions(-)

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/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 &&
-- 
2.44.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 14/15] builtin/gc: forward git-gc(1)'s `--auto` flag when packing refs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 6080 bytes --]

Forward the `--auto` flag to git-pack-refs(1) when it has been invoked
with this flag itself. This does not change anything for the "files"
backend, which will continue to eagerly pack refs. But it does ensure
that the "reftable" backend only compacts refs as required.

This change does not impact git-maintenance(1) as it won't ever end up
executing the pack-refs task when run with `--auto`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c               | 21 ++++++++++++---------
 t/t0610-reftable-basics.sh | 19 ++++++++++++++++---
 2 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index e0029c88f9..bf1f2a621a 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -212,6 +212,9 @@ static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *
 
 	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);
 }
 
@@ -572,7 +575,7 @@ static int report_last_gc_error(void)
 	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
@@ -583,7 +586,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) {
@@ -599,7 +602,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;
@@ -608,6 +610,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")),
@@ -618,7 +621,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"),
@@ -663,7 +666,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.
 		 */
@@ -688,7 +691,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);
 
 			/*
@@ -713,7 +716,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);
@@ -728,7 +731,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;
@@ -783,7 +786,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."));
 
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index 6de7529575..b37d8bf3b1 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -387,7 +387,9 @@ test_expect_success 'pack-refs: compaction raises locking errors' '
 	test_cmp expect err
 '
 
-test_expect_success 'pack-refs: auto compaction' '
+for command in pack-refs gc
+do
+test_expect_success "$command: auto compaction" '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&
 	(
@@ -395,14 +397,24 @@ test_expect_success 'pack-refs: auto compaction' '
 
 		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 pack-refs --auto &&
+		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.
@@ -416,10 +428,11 @@ test_expect_success 'pack-refs: auto compaction' '
 		rm .git/reftable/*.lock &&
 		test_line_count = 5 .git/reftable/tables.list &&
 
-		git pack-refs --auto &&
+		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" &&
-- 
2.44.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 15/15] builtin/gc: pack refs when using `git maintenance run --auto`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3804 bytes --]

When running `git maintenance run --auto`, then the various subtasks
will only run as needed. Thus, we for example end up only packing loose
objects if we hit a certain threshold.

Interestingly enough, the "pack-refs" task is actually _never_ executed
when the auto-flag is set because it does not have a condition at all.
As 41abfe15d9 (maintenance: add pack-refs task, 2021-02-09) mentions:

    The 'auto_condition' function pointer is left NULL for now. We could
    extend this in the future to have a condition check if pack-refs
    should be run during 'git maintenance run --auto'.

It is not quite clear from that quote whether it is actually intended
that the task doesn't run at all in this mode. Also, no test was added
to verify this behaviour. Ultimately though, it feels quite surprising
that `git maintenance run --auto --task=pack-refs` would quietly never
do anything at all.

In any case, now that we do have the logic in place to let ref backends
decide whether or not to repack refs, it does make sense to wire it up
accordingly. With the "reftable" backend we will thus now perform
auto-compaction, which optimizes the refdb as needed.

But for the "files" backend we now unconditionally pack refs as it does
not yet know to handle the "auto" flag. Arguably, this can be seen as a
bug fix given that previously the task never did anything at all.
Eventually though we should amend the "files" backend to use some
heuristics for auto compaction, as well.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c                  | 12 +++++++++++-
 t/t0601-reffiles-pack-refs.sh | 10 ++++++++++
 t/t0610-reftable-basics.sh    |  2 +-
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index bf1f2a621a..3c874b248b 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -206,6 +206,16 @@ struct maintenance_run_opts {
 	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;
@@ -1298,7 +1308,7 @@ static struct maintenance_task tasks[] = {
 	[TASK_PACK_REFS] = {
 		"pack-refs",
 		maintenance_task_pack_refs,
-		NULL,
+		pack_refs_condition,
 	},
 };
 
diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
index 219a495451..7d4ab0b91a 100755
--- a/t/t0601-reffiles-pack-refs.sh
+++ b/t/t0601-reffiles-pack-refs.sh
@@ -370,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 b37d8bf3b1..931d888bbb 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -387,7 +387,7 @@ test_expect_success 'pack-refs: compaction raises locking errors' '
 	test_cmp expect err
 '
 
-for command in pack-refs gc
+for command in pack-refs gc "maintenance run --task=pack-refs"
 do
 test_expect_success "$command: auto compaction" '
 	test_when_finished "rm -rf repo" &&
-- 
2.44.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 00/15] refs: introduce `--auto` to pack refs as needed

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 821 bytes --]

Patrick Steinhardt <ps@pks.im> writes:
> The `PACK_REFS_AUTO` flag is exposed via a new `git pack-refs --auto`
> flag. It is wired up in both `git gc --auto` and `git maintenance run
> --auto`.
>

Wondering if this is also exposed as a config option.

> The series is structured as follows:
>
>     - Patches 1 - 5: Bugfixes and improvements for the reftable-specific
>       compaction code. These are issues that I've found while working on
>       this series.
>
>     - Patches 6 - 8: Refactorings to drop the `PACK_REFS_ALL` flag,
>       which isn't actually used by the ref backends anymore and confused
>       me multiple times.
>
>     - Patches 9 - 15: Handling of `PACK_REFS_ALL` in git-pack-refs(1),
>       git-gc(1) and git-maintenance(1).
>

I'm assuming this means `PACK_REFS_AUTO` in the last sentence.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 01/15] reftable/stack: fix error handling in `reftable_stack_init_addition()`

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 867 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> diff --git a/reftable/stack.c b/reftable/stack.c
> index 1ecf1b9751..92d9a7facb 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -590,8 +590,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
>  	err = stack_uptodate(st);
>  	if (err < 0)
>  		goto done;
> -
> -	if (err > 1) {
> +	if (err > 0) {
>  		err = REFTABLE_LOCK_ERROR;
>  		goto done;
>  	}
> @@ -713,10 +712,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;
> -	}

This changes the behavior though, since now we skip the `goto done`. It
would be best to change the previous line to `if (err)`, which is what
the other function (`reftable_stack_new_addition`) does.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 03/15] reftable/stack: use error codes when locking fails during compaction

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> diff --git a/reftable/stack.c b/reftable/stack.c
> index eaa8bb9c99..ba15c48ddd 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -1003,7 +1003,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;
>

The comment at the top of the function now needs to be re-written with
this change.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 04/15] reftable/stack: gracefully handle failed auto-compaction due to locks

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2777 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> Whenever we commit a new table to the reftable stack we will end up
> invoking auto-compaction of the stack to keep the total number of tables
> at bay. This auto-compaction may fail though in case at least one of the
> tables which we are about to compact is locked. This is indicated by the
> compaction function returning a positive value. We do not handle this

We no longer return a positive value, do we?

> case though, and thus bubble that return value up the calling chain,
> which will ultimately cause a failure.
>
> Fix this bug by handling positive return values.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  reftable/stack.c           | 13 ++++++++++++-
>  t/t0610-reftable-basics.sh | 20 ++++++++++++++++++++
>  2 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/reftable/stack.c b/reftable/stack.c
> index ba15c48ddd..dd5160d751 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -680,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. Note that we explicitly ignore locking issues which
> +		 * may indicate that a concurrent process is already trying to
> +		 * compact tables. This is fine, so we simply ignore that error
> +		 * condition.
> +		 */
>

Nit: The last two sentences are somewhat the same, it'd be perhaps nicer
to explain why "this is fine".

>  		err = reftable_stack_auto_compact(add->stack);
> +		if (err < 0 && err != REFTABLE_LOCK_ERROR)
> +			goto done;
> +		err = 0;
> +	}
>
>  done:
>  	reftable_addition_close(add);
> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> index 686781192e..5f2f9baa9b 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
> +	)
> +'

I'm not sure about the testing setup for reftables, but maybe if we
moved this to the unit tests, we could've also checked the
`reftable_stack_compaction_stats(st)->failures` value. This is fine, but
it doesn't really tell us if the compaction was even attempted.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 10/15] builtin/pack-refs: release allocated memory

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 658 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> Some of the command line options in `cmd_pack_refs()` require us to
> allocate memory. This memory is never released and thus leaking, but we
> paper over this leak by declaring the respective variables as `static`
> function-level variables, which is somewhat awkward.
>
> Refactor the code to release the allocated memory and drop the `static`
> declaration. While at it, remove the useless `flags` variable.
>

Tangent: I was looking at `files_pack_refs` and it seems like there are
a bunch of `die(...)` calls there. I wonder if it would be nicer to use
`error()` instead so we could do a better job at cleanup.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 14/15] builtin/gc: forward git-gc(1)'s `--auto` flag when packing refs

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 595 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> Forward the `--auto` flag to git-pack-refs(1) when it has been invoked
> with this flag itself. This does not change anything for the "files"
> backend, which will continue to eagerly pack refs. But it does ensure
> that the "reftable" backend only compacts refs as required.
>
> This change does not impact git-maintenance(1) as it won't ever end up
> executing the pack-refs task when run with `--auto`.
>

I'm not sure I follow this one, the man page for git-maintenance(1)
doesn't mention anything about not running pack-refs when `--auto` is
used.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 15/15] builtin/gc: pack refs when using `git maintenance run --auto`

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> When running `git maintenance run --auto`, then the various subtasks
> will only run as needed. Thus, we for example end up only packing loose
> objects if we hit a certain threshold.
>
> Interestingly enough, the "pack-refs" task is actually _never_ executed
> when the auto-flag is set because it does not have a condition at all.
> As 41abfe15d9 (maintenance: add pack-refs task, 2021-02-09) mentions:
>
>     The 'auto_condition' function pointer is left NULL for now. We could
>     extend this in the future to have a condition check if pack-refs
>     should be run during 'git maintenance run --auto'.
>

Ok, this answers my question in the previous email.

> It is not quite clear from that quote whether it is actually intended
> that the task doesn't run at all in this mode. Also, no test was added
> to verify this behaviour. Ultimately though, it feels quite surprising
> that `git maintenance run --auto --task=pack-refs` would quietly never
> do anything at all.
>
> In any case, now that we do have the logic in place to let ref backends
> decide whether or not to repack refs, it does make sense to wire it up
> accordingly. With the "reftable" backend we will thus now perform
> auto-compaction, which optimizes the refdb as needed.
>
> But for the "files" backend we now unconditionally pack refs as it does
> not yet know to handle the "auto" flag. Arguably, this can be seen as a
> bug fix given that previously the task never did anything at all.
> Eventually though we should amend the "files" backend to use some
> heuristics for auto compaction, as well.
>

Agreed, thanks for the clear explanation here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 00/15] refs: introduce `--auto` to pack refs as needed

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1882 bytes --]

On Wed, Mar 20, 2024 at 12:30:58PM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > The `PACK_REFS_AUTO` flag is exposed via a new `git pack-refs --auto`
> > flag. It is wired up in both `git gc --auto` and `git maintenance run
> > --auto`.
> >
> 
> Wondering if this is also exposed as a config option.

It's not, no, and I don't quite think it would make sense. We don't
expose the current `git gc --auto` flag as config option, either. While
we have "gc.auto", it doesn't configure whether or not the `--auto` flag
is set, but rather configures the boundary at which objects would be
repacked.

In hindsight that config option is too narrowly focussed on objects
while pretending to be quite general. It would've been great if this was
called "gc.autoLooseObjectLimit" or something similar instead to not
cause confusion and to be more easily extendable in the future.

I can certainly see that we might eventually want to have ref backend
specific configuration here. For the "files" backend this could be be
the number of loose refs that are allowed to exist before repacking,
whereas for the "reftable" backend this could be the geometric factor.
But I'll leave that for a future series.

> > The series is structured as follows:
> >
> >     - Patches 1 - 5: Bugfixes and improvements for the reftable-specific
> >       compaction code. These are issues that I've found while working on
> >       this series.
> >
> >     - Patches 6 - 8: Refactorings to drop the `PACK_REFS_ALL` flag,
> >       which isn't actually used by the ref backends anymore and confused
> >       me multiple times.
> >
> >     - Patches 9 - 15: Handling of `PACK_REFS_ALL` in git-pack-refs(1),
> >       git-gc(1) and git-maintenance(1).
> >
> 
> I'm assuming this means `PACK_REFS_AUTO` in the last sentence.

Ah, yes.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 01/15] reftable/stack: fix error handling in `reftable_stack_init_addition()`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]

On Wed, Mar 20, 2024 at 02:50:43PM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > diff --git a/reftable/stack.c b/reftable/stack.c
> > index 1ecf1b9751..92d9a7facb 100644
> > --- a/reftable/stack.c
> > +++ b/reftable/stack.c
> > @@ -590,8 +590,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
> >  	err = stack_uptodate(st);
> >  	if (err < 0)
> >  		goto done;
> > -
> > -	if (err > 1) {
> > +	if (err > 0) {
> >  		err = REFTABLE_LOCK_ERROR;
> >  		goto done;
> >  	}
> > @@ -713,10 +712,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;
> > -	}
> 
> This changes the behavior though, since now we skip the `goto done`. It
> would be best to change the previous line to `if (err)`, which is what
> the other function (`reftable_stack_new_addition`) does.

It actually doesn't because `reftable_stack_init_addition()` does not
return a positive value anymore. And as it returns `REFTABLE_LOCK_ERROR`
in the case where it previously did return a positive value the
behaviour is exactly the same because we go into thee `if (err < 0)`
branch.

I'll amend the commit message to clariyf.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 03/15] reftable/stack: use error codes when locking fails during compaction

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 679 bytes --]

On Wed, Mar 20, 2024 at 03:14:10PM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > diff --git a/reftable/stack.c b/reftable/stack.c
> > index eaa8bb9c99..ba15c48ddd 100644
> > --- a/reftable/stack.c
> > +++ b/reftable/stack.c
> > @@ -1003,7 +1003,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;
> >
> 
> The comment at the top of the function now needs to be re-written with
> this change.

Good catch, will fix.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 04/15] reftable/stack: gracefully handle failed auto-compaction due to locks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3203 bytes --]

On Wed, Mar 20, 2024 at 03:22:55PM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Whenever we commit a new table to the reftable stack we will end up
> > invoking auto-compaction of the stack to keep the total number of tables
> > at bay. This auto-compaction may fail though in case at least one of the
> > tables which we are about to compact is locked. This is indicated by the
> > compaction function returning a positive value. We do not handle this
> 
> We no longer return a positive value, do we?

Yup, this is stale.

> > case though, and thus bubble that return value up the calling chain,
> > which will ultimately cause a failure.
> >
> > Fix this bug by handling positive return values.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  reftable/stack.c           | 13 ++++++++++++-
> >  t/t0610-reftable-basics.sh | 20 ++++++++++++++++++++
> >  2 files changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/reftable/stack.c b/reftable/stack.c
> > index ba15c48ddd..dd5160d751 100644
> > --- a/reftable/stack.c
> > +++ b/reftable/stack.c
> > @@ -680,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. Note that we explicitly ignore locking issues which
> > +		 * may indicate that a concurrent process is already trying to
> > +		 * compact tables. This is fine, so we simply ignore that error
> > +		 * condition.
> > +		 */
> >
> 
> Nit: The last two sentences are somewhat the same, it'd be perhaps nicer
> to explain why "this is fine".

Fair enough.

> >  		err = reftable_stack_auto_compact(add->stack);
> > +		if (err < 0 && err != REFTABLE_LOCK_ERROR)
> > +			goto done;
> > +		err = 0;
> > +	}
> >
> >  done:
> >  	reftable_addition_close(add);
> > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> > index 686781192e..5f2f9baa9b 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
> > +	)
> > +'
> 
> I'm not sure about the testing setup for reftables, but maybe if we
> moved this to the unit tests, we could've also checked the
> `reftable_stack_compaction_stats(st)->failures` value. This is fine, but
> it doesn't really tell us if the compaction was even attempted.

Both tests have their merit, I'd say. Let's thus just add both.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 10/15] builtin/pack-refs: release allocated memory

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]

On Wed, Mar 20, 2024 at 04:23:54PM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Some of the command line options in `cmd_pack_refs()` require us to
> > allocate memory. This memory is never released and thus leaking, but we
> > paper over this leak by declaring the respective variables as `static`
> > function-level variables, which is somewhat awkward.
> >
> > Refactor the code to release the allocated memory and drop the `static`
> > declaration. While at it, remove the useless `flags` variable.
> >
> 
> Tangent: I was looking at `files_pack_refs` and it seems like there are
> a bunch of `die(...)` calls there. I wonder if it would be nicer to use
> `error()` instead so we could do a better job at cleanup.

I agree that this would be a nice refactoring.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 14/15] builtin/gc: forward git-gc(1)'s `--auto` flag when packing refs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 929 bytes --]

On Wed, Mar 20, 2024 at 04:56:31PM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Forward the `--auto` flag to git-pack-refs(1) when it has been invoked
> > with this flag itself. This does not change anything for the "files"
> > backend, which will continue to eagerly pack refs. But it does ensure
> > that the "reftable" backend only compacts refs as required.
> >
> > This change does not impact git-maintenance(1) as it won't ever end up
> > executing the pack-refs task when run with `--auto`.
> >
> 
> I'm not sure I follow this one, the man page for git-maintenance(1)
> doesn't mention anything about not running pack-refs when `--auto` is
> used.

I'm clarifying this further in patch 15. It's quite confusing in the
first place that git-maintenance(1) doesn't ever pack refs at all when
using `--auto`. I will amend the commit message to explain this better.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 15/15] builtin/gc: pack refs when using `git maintenance run --auto`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1892 bytes --]

On Wed, Mar 20, 2024 at 04:59:06PM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > When running `git maintenance run --auto`, then the various subtasks
> > will only run as needed. Thus, we for example end up only packing loose
> > objects if we hit a certain threshold.
> >
> > Interestingly enough, the "pack-refs" task is actually _never_ executed
> > when the auto-flag is set because it does not have a condition at all.
> > As 41abfe15d9 (maintenance: add pack-refs task, 2021-02-09) mentions:
> >
> >     The 'auto_condition' function pointer is left NULL for now. We could
> >     extend this in the future to have a condition check if pack-refs
> >     should be run during 'git maintenance run --auto'.
> >
> 
> Ok, this answers my question in the previous email.
> 
> > It is not quite clear from that quote whether it is actually intended
> > that the task doesn't run at all in this mode. Also, no test was added
> > to verify this behaviour. Ultimately though, it feels quite surprising
> > that `git maintenance run --auto --task=pack-refs` would quietly never
> > do anything at all.
> >
> > In any case, now that we do have the logic in place to let ref backends
> > decide whether or not to repack refs, it does make sense to wire it up
> > accordingly. With the "reftable" backend we will thus now perform
> > auto-compaction, which optimizes the refdb as needed.
> >
> > But for the "files" backend we now unconditionally pack refs as it does
> > not yet know to handle the "auto" flag. Arguably, this can be seen as a
> > bug fix given that previously the task never did anything at all.
> > Eventually though we should amend the "files" backend to use some
> > heuristics for auto compaction, as well.
> >
> 
> Agreed, thanks for the clear explanation here.

Thanks for your thorough review!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 00/15] refs: introduce `--auto` to pack refs as needed

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 10110 bytes --]

Hi,

this is the second version of my patch series that introduces the
`--auto` flag to repack refs as neeeded to git-pack-refs(1), git-gc(1)
and git-maintenance(1).

Changes compared to v1:

    - Clarified some of the commit messages.

    - Adjusted the stale comment of `stack_compact_range()`.

    - Added a unit test for failing auto-compaction.

    - Clarified a comment to explain why it is fine for auto-compaction
      to fail.

Thanks!

Patrick

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

 Documentation/git-pack-refs.txt | 15 +++++-
 builtin/gc.c                    | 86 +++++++++++++++++++--------------
 builtin/pack-refs.c             | 31 +++++++-----
 refs.h                          | 20 ++++----
 refs/reftable-backend.c         | 11 ++++-
 reftable/error.c                |  4 +-
 reftable/reftable-error.h       |  5 +-
 reftable/stack.c                | 44 +++++++++++------
 reftable/stack_test.c           | 45 ++++++++++++++++-
 t/helper/test-ref-store.c       | 20 --------
 t/oid-info/hash-info            | 12 +++++
 t/t0601-reffiles-pack-refs.sh   | 30 ++++++++++--
 t/t0610-reftable-basics.sh      | 79 ++++++++++++++++++++++++++++++
 t/t6500-gc.sh                   | 30 +++---------
 14 files changed, 307 insertions(+), 125 deletions(-)

Range-diff against v1:
 1:  1e39d93a45 !  1:  b41b9b27cb reftable/stack: fix error handling in `reftable_stack_init_addition()`
    @@ Commit message
         error code check without the off-by-one. But other callers are subtly
         broken by this bug.
     
    -    Fix this by checking for `err > 0` instead.
    +    Fix this by checking for `err > 0` instead. This has the consequence
    +    that `reftable_stack_init_addition()` won't ever return a positive error
    +    code anymore, but will instead return `REFTABLE_LOCK_ERROR` now. Thus,
    +    we can drop the check for a positive error code in `stack_try_add()`
    +    now.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
 2:  e837703ca1 =  2:  be7212006b reftable/error: discern locked/outdated errors
 3:  ae2130ffda !  3:  95dda44672 reftable/stack: use error codes when locking fails during compaction
    @@ Commit message
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## reftable/stack.c ##
    +@@ reftable/stack.c: static int stack_write_compact(struct reftable_stack *st,
    + 	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)
     @@ reftable/stack.c: static int stack_compact_range(struct reftable_stack *st,
      					LOCK_NO_DEREF);
      	if (err < 0) {
 4:  37a18b91ca !  4:  50a3c37f92 reftable/stack: gracefully handle failed auto-compaction due to locks
    @@ Commit message
         invoking auto-compaction of the stack to keep the total number of tables
         at bay. This auto-compaction may fail though in case at least one of the
         tables which we are about to compact is locked. This is indicated by the
    -    compaction function returning a positive value. We do not handle this
    -    case though, and thus bubble that return value up the calling chain,
    -    which will ultimately cause a failure.
    +    compaction function returning `REFTABLE_LOCK_ERROR`. We do not handle
    +    this case though, and thus bubble that return value up the calling
    +    chain, which will ultimately cause a failure.
     
    -    Fix this bug by handling positive return values.
    +    Fix this bug by ignoring `REFTABLE_LOCK_ERROR`.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ reftable/stack.c: int reftable_addition_commit(struct reftable_addition *add)
     +	if (!add->stack->disable_auto_compact) {
     +		/*
     +		 * Auto-compact the stack to keep the number of tables in
    -+		 * control. Note that we explicitly ignore locking issues which
    -+		 * may indicate that a concurrent process is already trying to
    -+		 * compact tables. This is fine, so we simply ignore that error
    -+		 * condition.
    ++		 * 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)
    @@ reftable/stack.c: int reftable_addition_commit(struct reftable_addition *add)
      done:
      	reftable_addition_close(add);
     
    + ## reftable/stack_test.c ##
    +@@ reftable/stack_test.c: 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);
    ++	clear_dir(dir);
    ++}
    ++
    + static void test_reftable_stack_validate_refname(void)
    + {
    + 	struct reftable_write_options cfg = { 0 };
    +@@ reftable/stack_test.c: 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);
    +
      ## t/t0610-reftable-basics.sh ##
     @@ t/t0610-reftable-basics.sh: test_expect_success 'ref transaction: empty transaction in empty repo' '
      	EOF
 5:  f336db817c =  5:  6615f25b08 refs/reftable: print errors on compaction failure
 6:  999d0c2bb8 =  6:  e84b797728 t/helper: drop pack-refs wrapper
 7:  072627d82c =  7:  809094ffec refs: move `struct pack_refs_opts` to where it's used
 8:  919abe8eb9 =  8:  cf966fc584 refs: remove `PACK_REFS_ALL` flag
 9:  61ebcb2d52 =  9:  5d7af236d4 refs/reftable: expose auto compaction via new flag
10:  ff163a621d = 10:  23c6c20e4e builtin/pack-refs: release allocated memory
11:  8727f08bab = 11:  eb48ac0329 builtin/pack-refs: introduce new "--auto" flag
12:  65c9ff3ee5 = 12:  94cb036345 builtin/gc: move `struct maintenance_run_opts`
13:  817de1a88f = 13:  9657c67b3b t6500: extract objects with "17" prefix
14:  474cf66b26 ! 14:  3eaff8289b builtin/gc: forward git-gc(1)'s `--auto` flag when packing refs
    @@ Commit message
         backend, which will continue to eagerly pack refs. But it does ensure
         that the "reftable" backend only compacts refs as required.
     
    -    This change does not impact git-maintenance(1) as it won't ever end up
    -    executing the pack-refs task when run with `--auto`.
    +    This change does not impact git-maintenance(1) because this command will
    +    in fact never run the pack-refs task when run with `--auto`. This issue
    +    will be addressed in a subsequent commit.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
15:  71f4800d36 = 15:  1bdea3b316 builtin/gc: pack refs when using `git maintenance run --auto`
-- 
2.44.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 01/15] reftable/stack: fix error handling in `reftable_stack_init_addition()`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1815 bytes --]

In `reftable_stack_init_addition()` we call `stack_uptodate()` after
having created the lockfile to check whether the stack was modified
concurrently, which is indicated by a positive return code from the
latter function. If so, we return a `REFTABLE_LOCK_ERROR` to the caller
and abort the addition.

The error handling has an off-by-one though because we check whether the
error code is `> 1` instead of `> 0`. Thus, instead of returning the
locking error, we would return a positive value. One of the callers of
`reftable_stack_init_addition()` works around this bug by repeating the
error code check without the off-by-one. But other callers are subtly
broken by this bug.

Fix this by checking for `err > 0` instead. This has the consequence
that `reftable_stack_init_addition()` won't ever return a positive error
code anymore, but will instead return `REFTABLE_LOCK_ERROR` now. Thus,
we can drop the check for a positive error code in `stack_try_add()`
now.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index 1ecf1b9751..92d9a7facb 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -590,8 +590,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 	err = stack_uptodate(st);
 	if (err < 0)
 		goto done;
-
-	if (err > 1) {
+	if (err > 0) {
 		err = REFTABLE_LOCK_ERROR;
 		goto done;
 	}
@@ -713,10 +712,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)
-- 
2.44.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 02/15] reftable/error: discern locked/outdated errors

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3433 bytes --]

We currently throw two different errors into a similar-but-different
error code:

  - Errors when trying to lock the reftable stack.

  - Errors when trying to write to the reftable stack which has been
    modified concurrently.

This results in unclear error handling and user-visible error messages.

Create a new `REFTABLE_OUTDATED_ERROR` so that those error conditions
can be clearly told apart from each other. Adjust users of the old
`REFTABLE_LOCK_ERROR` to use the new error code as required.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/error.c          | 4 +++-
 reftable/reftable-error.h | 5 ++++-
 reftable/stack.c          | 6 +++---
 reftable/stack_test.c     | 2 +-
 4 files changed, 11 insertions(+), 6 deletions(-)

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 92d9a7facb..eaa8bb9c99 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);
 		}
@@ -591,7 +591,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 	if (err < 0)
 		goto done;
 	if (err > 0) {
-		err = REFTABLE_LOCK_ERROR;
+		err = REFTABLE_OUTDATED_ERROR;
 		goto done;
 	}
 
diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index 7336757cf5..2c3540d9e6 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -232,7 +232,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);
-- 
2.44.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 03/15] reftable/stack: use error codes when locking fails during compaction

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3190 bytes --]

Compaction of a reftable stack may fail gracefully when there is a
concurrent process that writes to the reftable stack and which has thus
locked either the "tables.list" file or one of the tables. This is
expected and can be handled gracefully by some of the callers which
invoke compaction. Thus, to indicate this situation to our callers, we
return a positive return code from `stack_compact_range()` and bubble it
up to the caller.

This kind of error handling is somewhat awkward though as many callers
in the call chain never even think of handling positive return values.
Thus, the result is either that such errors are swallowed by accident,
or that we abort operations with an unhelpful error message.

Make the code more robust by always using negative error codes when
compaction fails, with `REFTABLE_LOCK_ERROR` for the described benign
error case.

Note that only a single callsite knew to handle positive error codes
gracefully in the first place. Subsequent commits will touch up some of
the other sites to handle those errors better.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index eaa8bb9c99..79856b6565 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -973,7 +973,15 @@ static int stack_write_compact(struct reftable_stack *st,
 	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)
@@ -1003,7 +1011,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;
@@ -1025,7 +1033,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;
@@ -1075,7 +1083,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;
@@ -1187,7 +1195,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;
 }
-- 
2.44.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 04/15] reftable/stack: gracefully handle failed auto-compaction due to locks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4609 bytes --]

Whenever we commit a new table to the reftable stack we will end up
invoking auto-compaction of the stack to keep the total number of tables
at bay. This auto-compaction may fail though in case at least one of the
tables which we are about to compact is locked. This is indicated by the
compaction function returning `REFTABLE_LOCK_ERROR`. We do not handle
this case though, and thus bubble that return value up the calling
chain, which will ultimately cause a failure.

Fix this bug by ignoring `REFTABLE_LOCK_ERROR`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c           | 13 +++++++++++-
 reftable/stack_test.c      | 43 ++++++++++++++++++++++++++++++++++++++
 t/t0610-reftable-basics.sh | 20 ++++++++++++++++++
 3 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index 79856b6565..dde50b61d6 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -680,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);
diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index 2c3540d9e6..822e681028 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -343,6 +343,48 @@ 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);
+	clear_dir(dir);
+}
+
 static void test_reftable_stack_validate_refname(void)
 {
 	struct reftable_write_options cfg = { 0 };
@@ -1085,6 +1127,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/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index 686781192e..5f2f9baa9b 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 &&
-- 
2.44.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 05/15] refs/reftable: print errors on compaction failure

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1705 bytes --]

When git-pack-refs(1) fails in the reftable backend we end up printing
no error message at all, leaving the caller puzzled as to why compaction
has failed. Fix this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c    |  6 +++++-
 t/t0610-reftable-basics.sh | 12 ++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index e206d5a073..694dc4845f 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1204,8 +1204,12 @@ static int reftable_be_pack_refs(struct ref_store *ref_store,
 		stack = refs->main_stack;
 
 	ret = reftable_stack_compact_all(stack, NULL);
-	if (ret)
+	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/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index 5f2f9baa9b..a53d1dc493 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -375,6 +375,18 @@ 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
+'
+
 test_expect_success 'pack-refs: prunes stale tables' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&
-- 
2.44.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 06/15] t/helper: drop pack-refs wrapper

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2974 bytes --]

The test helper provides a "ref-store <store> pack-refs" wrapper that
more or less directly invokes `refs_pack_refs()`. This helper is only
used in a single test with the "PACK_REFS_PRUNE" and "PACK_REFS_ALL"
flags. Both of these flags can directly be accessed via git-pack-refs(1)
though via the `--all` and `--prune` flags, which makes the helper
superfluous.

Refactor the test to use git-pack-refs(1) instead of the test helper.
Drop the now-unused test helper command.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/helper/test-ref-store.c     | 20 --------------------
 t/t0601-reffiles-pack-refs.sh | 13 +++++++++----
 2 files changed, 9 insertions(+), 24 deletions(-)

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/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
index c309d2bae8..b1cf587347 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"
 '
 
-- 
2.44.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 07/15] refs: move `struct pack_refs_opts` to where it's used

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

The declaration of `struct pack_refs_opts` is in a seemingly random
place. Move it so that it's located right next to its flags and
functions that use it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/refs.h b/refs.h
index 298caf6c61..139ce7113b 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,
@@ -433,6 +427,12 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,
 #define PACK_REFS_PRUNE 0x0001
 #define PACK_REFS_ALL   0x0002
 
+struct pack_refs_opts {
+	unsigned int flags;
+	struct ref_exclusions *exclusions;
+	struct string_list *includes;
+};
+
 /*
  * Write a packed-refs file for the current repository.
  * flags: Combination of the above PACK_REFS_* flags.
-- 
2.44.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 08/15] refs: remove `PACK_REFS_ALL` flag

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2466 bytes --]

The intent of the `PACK_REFS_ALL` flag is to ask the backend to compact
all refs instead of only a subset of them. Thus, this flag gets passed
down to `refs_pack_refs()` via `struct pack_refs_opts::flags`.

But starting with 4fe42f326e (pack-refs: teach pack-refs --include
option, 2023-05-12), the flag's semantics have changed. Instead of being
handled by the respective backends, this flag is now getting handled by
the callers of `refs_pack_refs()` which will add a single glob ("*") to
the list of refs-to-be-packed. Thus, the flag serves no purpose to the
ref backends anymore.

Remove the flag and replace it with a local variable.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/pack-refs.c | 5 +++--
 refs.h              | 2 --
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index bcf383cac9..97921beef2 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -21,9 +21,10 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 						 .flags = flags };
 	static struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;
 	struct string_list_item *item;
+	int pack_all = 0;
 
 	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_STRING_LIST(0, "include", pack_refs_opts.includes, N_("pattern"),
 			N_("references to include")),
@@ -38,7 +39,7 @@ 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)
diff --git a/refs.h b/refs.h
index 139ce7113b..8c8994cb29 100644
--- a/refs.h
+++ b/refs.h
@@ -422,10 +422,8 @@ 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
  */
 #define PACK_REFS_PRUNE 0x0001
-#define PACK_REFS_ALL   0x0002
 
 struct pack_refs_opts {
 	unsigned int flags;
-- 
2.44.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 09/15] refs/reftable: expose auto compaction via new flag

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3299 bytes --]

Under normal circumstances, the "reftable" backend will automatically
perform compaction after appending to the stack. It is thus not
necessary and may even be considered wasteful to run git-pack-refs(1) in
"reftable"-backed repositories as it will cause the backend to compact
all tables into a single one. We do exactly that though when running
`git maintenance run --auto` or `git gc --auto`, which gets spawned by
Git after running some specific commands.

The `--auto` mode is typically only executing optimizations as needed.
To do so, we already use several heuristics for the various different
data structures in Git to determine whether to optimize them or not.
We do not use any heuristics for refs though and instead always optimize
them.

Introduce a new `PACK_REFS_AUTO` flag that can be passed to the backend.
When not handled by the backend we will continue to behave the exact
same as we do right now, that is we optimize refs unconditionally. This
is done for the "files" backend for now to retain current behaviour,
even though we may eventually also want to introduce heuristics here.
For the "reftable" backend though we already do have auto-compaction, so
we can easily reuse that logic to implement the new auto-packing flag.

Note that under normal circumstances, this should always end up being a
no-op. After all, we already invoke the code for every single addition
to the stack. But there are special cases where it can still be helpful
to execute the auto-compaction code explicitly:

  - Concurrent writers may cause compaction to not run due to locks.

  - Callers may decide to disable compaction altogether and then pack
    refs at a later point due to various reasons.

  - Other implementations of the reftable format may do compaction
    differently or even not at all.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.h                  | 6 +++++-
 refs/reftable-backend.c | 5 ++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/refs.h b/refs.h
index 8c8994cb29..d278775e08 100644
--- a/refs.h
+++ b/refs.h
@@ -422,8 +422,12 @@ 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_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_PRUNE (1 << 0)
+#define PACK_REFS_AUTO  (1 << 1)
 
 struct pack_refs_opts {
 	unsigned int flags;
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 694dc4845f..0bed6d2ab4 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1203,7 +1203,10 @@ 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 (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));
-- 
2.44.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 10/15] builtin/pack-refs: release allocated memory

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2239 bytes --]

Some of the command line options in `cmd_pack_refs()` require us to
allocate memory. This memory is never released and thus leaking, but we
paper over this leak by declaring the respective variables as `static`
function-level variables, which is somewhat awkward.

Refactor the code to release the allocated memory and drop the `static`
declaration. While at it, remove the useless `flags` variable.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/pack-refs.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index 97921beef2..ea2baeec76 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -13,15 +13,17 @@ static char const * const pack_refs_usage[] = {
 
 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_BOOL(0, "all",   &pack_all, N_("pack everything")),
@@ -45,5 +47,10 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 	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;
 }
-- 
2.44.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 11/15] builtin/pack-refs: introduce new "--auto" flag

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5611 bytes --]

Calling git-pack-refs(1) will unconditionally cause it to pack all
requested refs regardless of the current state of the ref database. For
example:

  - With the "files" backend we will end up rewriting the complete
    "packed-refs" file even if only a single ref would require
    compaction.

  - With the "reftable" backend we will end up always compacting all
    tables into a single table.

This behaviour can be completely unnecessary depending on the backend
and is thus wasteful.

With the introduction of the `PACK_REFS_AUTO` flag in the preceding
commit we can improve this and let the backends decide for themselves
whether to pack refs in the first place. Expose this functionality via a
new "--auto" flag in git-pack-refs(1), which mirrors the same flag in
both git-gc(1) and git-maintenance(1).

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git-pack-refs.txt | 15 ++++++++++++++-
 builtin/pack-refs.c             |  3 ++-
 t/t0601-reffiles-pack-refs.sh   |  7 +++++++
 t/t0610-reftable-basics.sh      | 34 +++++++++++++++++++++++++++++++++
 4 files changed, 57 insertions(+), 2 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/pack-refs.c b/builtin/pack-refs.c
index ea2baeec76..db40825666 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -7,7 +7,7 @@
 #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
 };
 
@@ -28,6 +28,7 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 	struct option opts[] = {
 		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"),
diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
index b1cf587347..219a495451 100755
--- a/t/t0601-reffiles-pack-refs.sh
+++ b/t/t0601-reffiles-pack-refs.sh
@@ -164,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 &&
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index a53d1dc493..6de7529575 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -387,6 +387,40 @@ test_expect_success 'pack-refs: compaction raises locking errors' '
 	test_cmp expect err
 '
 
+test_expect_success 'pack-refs: auto compaction' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		test_commit A &&
+
+		# 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 pack-refs --auto &&
+		ls -1 .git/reftable >tables-actual &&
+		test_cmp tables-expect tables-actual &&
+
+		# 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 pack-refs --auto &&
+		test_line_count = 1 .git/reftable/tables.list
+	)
+'
+
 test_expect_success 'pack-refs: prunes stale tables' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&
-- 
2.44.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 12/15] builtin/gc: move `struct maintenance_run_opts`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2286 bytes --]

We're about to start using `struct maintenance_run_opts` in
`maintenance_task_pack_refs()`. Move its definition up to prepare for
this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 53 ++++++++++++++++++++++++++--------------------------
 1 file changed, 26 insertions(+), 27 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index cb80ced6cb..e0029c88f9 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -180,7 +180,32 @@ 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 maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
@@ -773,26 +798,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 +814,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)
 
-- 
2.44.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 13/15] t6500: extract objects with "17" prefix

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3223 bytes --]

The ".git/obects/17/" shard is somewhat special because it is used by
git-gc(1) to estimate how many objects there are by extrapolating the
number of objects in that shard, only. In t6500 we thus have a hard
coded set of data that, when written to the object database, result in
blobs starting with that prefix.

We are about to need such "17"-prefixed objects in another test suite.
Extract them into "t/oid-info/hash-info" so that they can be reused by
other tests.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/oid-info/hash-info | 12 ++++++++++++
 t/t6500-gc.sh        | 30 +++++++-----------------------
 2 files changed, 19 insertions(+), 23 deletions(-)

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/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 &&
-- 
2.44.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 14/15] builtin/gc: forward git-gc(1)'s `--auto` flag when packing refs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 6146 bytes --]

Forward the `--auto` flag to git-pack-refs(1) when it has been invoked
with this flag itself. This does not change anything for the "files"
backend, which will continue to eagerly pack refs. But it does ensure
that the "reftable" backend only compacts refs as required.

This change does not impact git-maintenance(1) because this command will
in fact never run the pack-refs task when run with `--auto`. This issue
will be addressed in a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c               | 21 ++++++++++++---------
 t/t0610-reftable-basics.sh | 19 ++++++++++++++++---
 2 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index e0029c88f9..bf1f2a621a 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -212,6 +212,9 @@ static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *
 
 	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);
 }
 
@@ -572,7 +575,7 @@ static int report_last_gc_error(void)
 	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
@@ -583,7 +586,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) {
@@ -599,7 +602,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;
@@ -608,6 +610,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")),
@@ -618,7 +621,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"),
@@ -663,7 +666,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.
 		 */
@@ -688,7 +691,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);
 
 			/*
@@ -713,7 +716,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);
@@ -728,7 +731,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;
@@ -783,7 +786,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."));
 
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index 6de7529575..b37d8bf3b1 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -387,7 +387,9 @@ test_expect_success 'pack-refs: compaction raises locking errors' '
 	test_cmp expect err
 '
 
-test_expect_success 'pack-refs: auto compaction' '
+for command in pack-refs gc
+do
+test_expect_success "$command: auto compaction" '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&
 	(
@@ -395,14 +397,24 @@ test_expect_success 'pack-refs: auto compaction' '
 
 		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 pack-refs --auto &&
+		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.
@@ -416,10 +428,11 @@ test_expect_success 'pack-refs: auto compaction' '
 		rm .git/reftable/*.lock &&
 		test_line_count = 5 .git/reftable/tables.list &&
 
-		git pack-refs --auto &&
+		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" &&
-- 
2.44.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 15/15] builtin/gc: pack refs when using `git maintenance run --auto`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3806 bytes --]

When running `git maintenance run --auto`, then the various subtasks
will only run as needed. Thus, we for example end up only packing loose
objects if we hit a certain threshold.

Interestingly enough, the "pack-refs" task is actually _never_ executed
when the auto-flag is set because it does not have a condition at all.
As 41abfe15d9 (maintenance: add pack-refs task, 2021-02-09) mentions:

    The 'auto_condition' function pointer is left NULL for now. We could
    extend this in the future to have a condition check if pack-refs
    should be run during 'git maintenance run --auto'.

It is not quite clear from that quote whether it is actually intended
that the task doesn't run at all in this mode. Also, no test was added
to verify this behaviour. Ultimately though, it feels quite surprising
that `git maintenance run --auto --task=pack-refs` would quietly never
do anything at all.

In any case, now that we do have the logic in place to let ref backends
decide whether or not to repack refs, it does make sense to wire it up
accordingly. With the "reftable" backend we will thus now perform
auto-compaction, which optimizes the refdb as needed.

But for the "files" backend we now unconditionally pack refs as it does
not yet know to handle the "auto" flag. Arguably, this can be seen as a
bug fix given that previously the task never did anything at all.
Eventually though we should amend the "files" backend to use some
heuristics for auto compaction, as well.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c                  | 12 +++++++++++-
 t/t0601-reffiles-pack-refs.sh | 10 ++++++++++
 t/t0610-reftable-basics.sh    |  2 +-
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index bf1f2a621a..3c874b248b 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -206,6 +206,16 @@ struct maintenance_run_opts {
 	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;
@@ -1298,7 +1308,7 @@ static struct maintenance_task tasks[] = {
 	[TASK_PACK_REFS] = {
 		"pack-refs",
 		maintenance_task_pack_refs,
-		NULL,
+		pack_refs_condition,
 	},
 };
 
diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
index 219a495451..7d4ab0b91a 100755
--- a/t/t0601-reffiles-pack-refs.sh
+++ b/t/t0601-reffiles-pack-refs.sh
@@ -370,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 b37d8bf3b1..931d888bbb 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -387,7 +387,7 @@ test_expect_success 'pack-refs: compaction raises locking errors' '
 	test_cmp expect err
 '
 
-for command in pack-refs gc
+for command in pack-refs gc "maintenance run --task=pack-refs"
 do
 test_expect_success "$command: auto compaction" '
 	test_when_finished "rm -rf repo" &&
-- 
2.44.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 04/15] reftable/stack: gracefully handle failed auto-compaction due to locks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2216 bytes --]

On Mon, Mar 25, 2024 at 11:02:51AM +0100, Patrick Steinhardt wrote:
[snip]
> diff --git a/reftable/stack_test.c b/reftable/stack_test.c
> index 2c3540d9e6..822e681028 100644
> --- a/reftable/stack_test.c
> +++ b/reftable/stack_test.c
> @@ -343,6 +343,48 @@ 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);
> +	clear_dir(dir);
> +}
> +

I forgot to free the `table_path` buffer here. So this needs the
following patch on top:

diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index 822e681028..7b2a8b1afd 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -382,6 +382,7 @@ static void test_reftable_stack_auto_compaction_fails_gracefully(void)
        EXPECT(st->stats.failures == 1);

        reftable_stack_destroy(st);
+       strbuf_release(&table_path);
        clear_dir(dir);
 }

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 00/15] refs: introduce `--auto` to pack refs as needed

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 587 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> Hi,
>
> this is the second version of my patch series that introduces the
> `--auto` flag to repack refs as neeeded to git-pack-refs(1), git-gc(1)
> and git-maintenance(1).
>
> Changes compared to v1:
>
>     - Clarified some of the commit messages.
>
>     - Adjusted the stale comment of `stack_compact_range()`.
>
>     - Added a unit test for failing auto-compaction.
>
>     - Clarified a comment to explain why it is fine for auto-compaction
>       to fail.
>
> Thanks!
>
> Patrick
>

The range diff from this version looks good.
Thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v2 00/15] refs: introduce `--auto` to pack refs as needed

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
>> Hi,
>>
>> this is the second version of my patch series that introduces the
>> `--auto` flag to repack refs as neeeded to git-pack-refs(1), git-gc(1)
>> and git-maintenance(1).
>>
>> Changes compared to v1:
>>
>>     - Clarified some of the commit messages.
>>
>>     - Adjusted the stale comment of `stack_compact_range()`.
>>
>>     - Added a unit test for failing auto-compaction.
>>
>>     - Clarified a comment to explain why it is fine for auto-compaction
>>       to fail.
>>
>> Thanks!
>>
>> Patrick
>>
>
> The range diff from this version looks good.
> Thanks!

Let's mark the topic for 'next', then.

Thanks, both.  Will queue.