Maintenance builtin, allowing 'gc --auto' customization

From: Derrick Stolee <[email protected]>

The first new task in the 'git maintenance' builtin is the 'commit-graph' job. It is based on the sequence of events in the 'commit-graph' job in Scalar [1]. This sequence is as follows:

1. git commit-graph write --reachable --split
2. git commit-graph verify --shallow
3. If the verify succeeds, stop.
4. Delete the commit-graph-chain file.
5. git commit-graph write --reachable --split

By writing an incremental commit-graph file using the "--split" option we minimize the disruption from this operation. The default behavior is to merge layers until the new "top" layer is less than half the size of the layer below. This provides quick writes most of the time, with the longer writes following a power law distribution.

Most importantly, concurrent Git processes only look at the commit-graph-chain file for a very short amount of time, so they will verly likely not be holding a handle to the file when we try to replace it. (This only matters on Windows.)

If a concurrent process reads the old commit-graph-chain file, but our job expires some of the .graph files before they can be read, then those processes will see a warning message (but not fail). This could be avoided by a future update to use the --expire-time argument when writing the commit-graph.

By using 'git commit-graph verify --shallow' we can ensure that the file we just wrote is valid. This is an extra safety precaution that is faster than our 'write' subcommand. In the rare situation that the newest layer of the commit-graph is corrupt, we can "fix" the corruption by deleting the commit-graph-chain file and rewrite the full commit-graph as a new one-layer commit graph. This does not completely prevent _that_ file from being corrupt, but it does recompute the commit-graph by parsing commits from the object database. In our use of this step in Scalar and VFS for Git, we have only seen this issue arise because our microsoft/git fork reverted 43d3561 ("commit-graph write: don't die if the existing graph is corrupt" 2019-03-25) for a while to keep commit-graph writes very fast. We dropped the revert when updating to v2.23.0. The verify still has potential for catching corrupt data across the layer boundary: if the new file has commit X with parent Y in an old file but the commit ID for Y in the old file had a bitswap, then we will notice that in the 'verify' command.

[1] https://github.com/microsoft/scalar/blob/master/Scalar.Common/Maintenance/CommitGraphStep.cs

---
 Documentation/git-maintenance.txt | 18 +++++++++
 builtin/gc.c                      | 63 +++++++++++++++++++++++++++++++
 commit-graph.c                    |  8 ++--
 commit-graph.h                    |  1 +
 t/t7900-maintenance.sh            |  2 +
 5 files changed, 88 insertions(+), 4 deletions(-)

index 089fa4cedc..35b0be7d40 100644
@@ -35,6 +35,24 @@ run::
 TASKS
 -----
 
+commit-graph::
+	The `commit-graph` job updates the `commit-graph` files incrementally,
+	then verifies that the written data is correct. If the new layer has an
+	issue, then the chain file is removed and the `commit-graph` is
+	rewritten from scratch.
++
+The verification only checks the top layer of the `commit-graph` chain.
+If the incremental write merged the new commits with at least one
+existing layer, then there is potential for on-disk corruption being
+carried forward into the new file. This will be noticed and the new
+commit-graph file will be clean as Git reparses the commit data from
+the object database.
++
+The incremental write is safe to run alongside concurrent Git processes
+since it will not expire `.graph` files that were in the previous
+`commit-graph-chain` file. They will be deleted by a later run based on
+the expiration delay.
+
 gc::
 	Cleanup unnecessary files and optimize the local repository. "GC"
 	stands for "garbage collection," but this task performs many

index 1af68c297d..9705d237e4 100644
@@ -172,7 +172,7 @@ static char *get_split_graph_filename(struct object_directory *odb,
 		       oid_hex);
 }
 
-static char *get_chain_filename(struct object_directory *odb)
+char *get_commit_graph_chain_filename(struct object_directory *odb)
 {
 	return xstrfmt("%s/info/commit-graphs/commit-graph-chain", odb->path);
 }

@@ -521,7 +521,7 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,
 	struct stat st;
 	struct object_id *oids;
 	int i = 0, valid = 1, count;
-	char *chain_name = get_chain_filename(odb);
+	char *chain_name = get_commit_graph_chain_filename(odb);
 	FILE *fp;
 	int stat_res;
 

@@ -1619,7 +1619,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 	}
 
 	if (ctx->split) {
-		char *lock_name = get_chain_filename(ctx->odb);
+		char *lock_name = get_commit_graph_chain_filename(ctx->odb);
 
 		hold_lock_file_for_update_mode(&lk, lock_name,
 					       LOCK_DIE_ON_ERROR, 0444);

@@ -1996,7 +1996,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)
 	if (ctx->split_opts && ctx->split_opts->expire_time)
 		expire_time = ctx->split_opts->expire_time;
 	if (!ctx->split) {
-		char *chain_file_name = get_chain_filename(ctx->odb);
+		char *chain_file_name = get_commit_graph_chain_filename(ctx->odb);
 		unlink(chain_file_name);
 		free(chain_file_name);
 		ctx->num_commit_graphs_after = 0;

index 28f89cdf3e..3c202748c3 100644
@@ -25,6 +25,7 @@ struct commit;
 struct bloom_filter_settings;
 
 char *get_commit_graph_filename(struct object_directory *odb);
+char *get_commit_graph_chain_filename(struct object_directory *odb);
 int open_commit_graph(const char *graph_file, int *fd, struct stat *st);
 
 /*

index f08eee0977..ff646abf7c 100755
@@ -4,6 +4,8 @@ test_description='git maintenance builtin'
 
 . ./test-lib.sh
 
+GIT_TEST_COMMIT_GRAPH=0
+
 test_expect_success 'help text' '
 	test_expect_code 129 git maintenance -h 2>err &&
 	test_i18ngrep "usage: git maintenance run" err

gitgitgadget

From: Derrick Stolee <[email protected]>

A user may want to only run certain maintenance tasks in a certain order. Add the --task=<task> option, which allows a user to specify an ordered list of tasks to run. These cannot be run multiple times, however.

Here is where our array of maintenance_task pointers becomes critical. We can sort the array of pointers based on the task order, but we do not want to move the struct data itself in order to preserve the hashmap references. We use the hashmap to match the --task=<task> arguments into the task struct data.

Keep in mind that the 'enabled' member of the maintenance_task struct is a placeholder for a future 'maintenance.<task>.enabled' config option. Thus, we use the 'enabled' member to specify which tasks are run when the user does not specify any --task=<task> arguments. The 'enabled' member should be ignored if --task=<task> appears.

---
 Documentation/git-maintenance.txt |  4 +++
 builtin/gc.c                      | 59 +++++++++++++++++++++++++++++--
 t/t7900-maintenance.sh            | 23 ++++++++++++
 3 files changed, 84 insertions(+), 2 deletions(-)

index 35b0be7d40..9204762e21 100644
@@ -73,6 +73,10 @@ OPTIONS
 --quiet::
 	Do not report progress or other information over `stderr`.
 
+--task=<task>::
+	If this option is specified one or more times, then only run the
+	specified tasks in the specified order.
+
 GIT
 ---
 Part of the linkgit:git[1] suite

index ff646abf7c..3cdccb24df 100755
@@ -20,4 +20,27 @@ test_expect_success 'run [--auto|--quiet]' '
 	grep ",\"gc\",\"--quiet\"" run-quiet.txt
 '
 
+test_expect_success 'run --task=<task>' '
+	GIT_TRACE2_EVENT="$(pwd)/run-commit-graph.txt" git maintenance run --task=commit-graph &&
+	GIT_TRACE2_EVENT="$(pwd)/run-gc.txt" git maintenance run --task=gc &&
+	GIT_TRACE2_EVENT="$(pwd)/run-commit-graph.txt" git maintenance run --task=commit-graph &&
+	GIT_TRACE2_EVENT="$(pwd)/run-both.txt" git maintenance run --task=commit-graph --task=gc &&
+	! grep ",\"gc\"" run-commit-graph.txt  &&
+	grep ",\"gc\"" run-gc.txt  &&
+	grep ",\"gc\"" run-both.txt  &&
+	grep ",\"commit-graph\",\"write\"" run-commit-graph.txt  &&
+	! grep ",\"commit-graph\",\"write\"" run-gc.txt  &&
+	grep ",\"commit-graph\",\"write\"" run-both.txt
+'
+
+test_expect_success 'run --task=bogus' '
+	test_must_fail git maintenance run --task=bogus 2>err &&
+	test_i18ngrep "is not a valid task" err
+'
+
+test_expect_success 'run --task duplicate' '
+	test_must_fail git maintenance run --task=gc --task=gc 2>err &&
+	test_i18ngrep "cannot be selected multiple times" err
+'
+
 test_done

gitgitgadget

From: Junio C Hamano <[email protected]>

If you run fetch but record the result in remote-tracking branches, and either if you do nothing with the fetched refs (e.g. you are merely mirroring) or if you always work from the remote-tracking refs (e.g. you fetch and then merge origin/branchname separately), you can get away with having no FETCH_HEAD at all.

Teach "git fetch" a command line option "--[no-]write-fetch-head" and "fetch.writeFetchHEAD" configuration variable. Without either, the default is to write FETCH_HEAD, and the usual rule that the command line option defeats configured default applies.

Note that under "--dry-run" mode, FETCH_HEAD is never written; otherwise you'd see list of objects in the file that you do not actually have. Passing --write-fetch-head does not force git fetch to write the file.

Also note that this option is explicitly passed when "git pull" internally invokes "git fetch", so that those who configured their "git fetch" not to write FETCH_HEAD would not be able to break the cooperation between these two commands. "git pull" must see what "git fetch" got recorded in FETCH_HEAD to work correctly.

---
 Documentation/config/fetch.txt  |  7 ++++++
 Documentation/fetch-options.txt | 10 +++++++++
 builtin/fetch.c                 | 19 +++++++++++++---
 builtin/pull.c                  |  3 ++-
 t/t5510-fetch.sh                | 39 +++++++++++++++++++++++++++++++--
 t/t5521-pull-options.sh         | 16 ++++++++++++++
 6 files changed, 88 insertions(+), 6 deletions(-)

index b20394038d..0aaa05e8c0 100644
@@ -91,3 +91,10 @@ fetch.writeCommitGraph::
 	merge and the write may take longer. Having an updated commit-graph
 	file helps performance of many Git commands, including `git merge-base`,
 	`git push -f`, and `git log --graph`. Defaults to false.
+
+fetch.writeFetchHEAD::
+	Setting it to false tells `git fetch` not to write the list
+	of remote refs fetched in the `FETCH_HEAD` file directly
+	under `$GIT_DIR`.  Can be countermanded from the command
+	line with the `--[no-]write-fetch-head` option.  Defaults to
+	true.

index 495bc8ab5a..6972ad2522 100644
@@ -64,6 +64,16 @@ documented in linkgit:git-config[1].
 --dry-run::
 	Show what would be done, without making any changes.
 
+ifndef::git-pull[]
+--[no-]write-fetch-head::
+	Write the list of remote refs fetched in the `FETCH_HEAD`
+	file directly under `$GIT_DIR`.  This is the default unless
+	the configuration variable `fetch.writeFetchHEAD` is set to
+	false.  Passing `--no-write-fetch-head` from the command
+	line tells Git not to write the file.  Under `--dry-run`
+	option, the file is never written.
+endif::git-pull[]
+
 -f::
 --force::
 	When 'git fetch' is used with `<src>:<dst>` refspec it may

index c7c8ac0861..30ac57dcf6 100644
@@ -56,6 +56,7 @@ static int prune_tags = -1; /* unspecified */
 #define PRUNE_TAGS_BY_DEFAULT 0 /* do we prune tags by default? */
 
 static int all, append, dry_run, force, keep, multiple, update_head_ok;
+static int write_fetch_head = 1;
 static int verbosity, deepen_relative, set_upstream;
 static int progress = -1;
 static int enable_auto_gc = 1;

@@ -118,6 +119,10 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(k, "fetch.writefetchhead")) {
+		write_fetch_head = git_config_bool(k, v);
+		return 0;
+	}
 	return git_default_config(k, v, cb);
 }
 

@@ -162,6 +167,8 @@ static struct option builtin_fetch_options[] = {
 		    PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),
 	OPT_BOOL(0, "dry-run", &dry_run,
 		 N_("dry run")),
+	OPT_BOOL(0, "write-fetch-head", &write_fetch_head,
+		 N_("write fetched references to the FETCH_HEAD file")),
 	OPT_BOOL('k', "keep", &keep, N_("keep downloaded pack")),
 	OPT_BOOL('u', "update-head-ok", &update_head_ok,
 		    N_("allow updating of HEAD ref")),

@@ -895,7 +902,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 	const char *what, *kind;
 	struct ref *rm;
 	char *url;
-	const char *filename = dry_run ? "/dev/null" : git_path_fetch_head(the_repository);
+	const char *filename = (!write_fetch_head
+				? "/dev/null"
+				: git_path_fetch_head(the_repository));
 	int want_status;
 	int summary_width = transport_summary_width(ref_map);
 

@@ -1329,7 +1338,7 @@ static int do_fetch(struct transport *transport,
 	}
 
 	/* if not appending, truncate FETCH_HEAD */
-	if (!append && !dry_run) {
+	if (!append && write_fetch_head) {
 		retcode = truncate_fetch_head();
 		if (retcode)
 			goto cleanup;

@@ -1596,7 +1605,7 @@ static int fetch_multiple(struct string_list *list, int max_children)
 	int i, result = 0;
 	struct strvec argv = STRVEC_INIT;
 
-	if (!append && !dry_run) {
+	if (!append && write_fetch_head) {
 		int errcode = truncate_fetch_head();
 		if (errcode)
 			return errcode;

@@ -1797,6 +1806,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	if (depth || deepen_since || deepen_not.nr)
 		deepen = 1;
 
+	/* FETCH_HEAD never gets updated in --dry-run mode */
+	if (dry_run)
+		write_fetch_head = 0;
+
 	if (all) {
 		if (argc == 1)
 			die(_("fetch --all does not take a repository argument"));

index 858b492af3..4c66db1468 100644
@@ -527,7 +527,8 @@ static int run_fetch(const char *repo, const char **refspecs)
 	struct strvec args = STRVEC_INIT;
 	int ret;
 
-	strvec_pushl(&args, "fetch", "--update-head-ok", NULL);
+	strvec_pushl(&args, "fetch", "--update-head-ok",
+		     "--write-fetch-head", NULL);
 
 	/* Shared options */
 	argv_push_verbosity(&args);

index 9850ecde5d..31c91d0ed2 100755
@@ -539,13 +539,48 @@ test_expect_success 'fetch into the current branch with --update-head-ok' '
 
 '
 
-test_expect_success 'fetch --dry-run' '
-
+test_expect_success 'fetch --dry-run does not touch FETCH_HEAD' '
 	rm -f .git/FETCH_HEAD &&
 	git fetch --dry-run . &&
 	! test -f .git/FETCH_HEAD
 '
 
+test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD' '
+	rm -f .git/FETCH_HEAD &&
+	git fetch --no-write-fetch-head . &&
+	! test -f .git/FETCH_HEAD
+'
+
+test_expect_success '--write-fetch-head gets defeated by --dry-run' '
+	rm -f .git/FETCH_HEAD &&
+	git fetch --dry-run --write-fetch-head . &&
+	! test -f .git/FETCH_HEAD
+'
+
+test_expect_success 'fetch.writeFetchHEAD and FETCH_HEAD' '
+	rm -f .git/FETCH_HEAD &&
+	git -c fetch.writeFetchHEAD=no fetch . &&
+	! test -f .git/FETCH_HEAD
+'
+
+test_expect_success 'fetch.writeFetchHEAD gets defeated by --dry-run' '
+	rm -f .git/FETCH_HEAD &&
+	git -c fetch.writeFetchHEAD=yes fetch --dry-run . &&
+	! test -f .git/FETCH_HEAD
+'
+
+test_expect_success 'fetch.writeFetchHEAD and --no-write-fetch-head' '
+	rm -f .git/FETCH_HEAD &&
+	git -c fetch.writeFetchHEAD=yes fetch --no-write-fetch-head . &&
+	! test -f .git/FETCH_HEAD
+'
+
+test_expect_success 'fetch.writeFetchHEAD and --write-fetch-head' '
+	rm -f .git/FETCH_HEAD &&
+	git -c fetch.writeFetchHEAD=no fetch --write-fetch-head . &&
+	test -f .git/FETCH_HEAD
+'
+
 test_expect_success "should be able to fetch with duplicate refspecs" '
 	mkdir dups &&
 	(

index 159afa7ac8..1acae3b9a4 100755
@@ -77,6 +77,7 @@ test_expect_success 'git pull -q -v --no-rebase' '
 	test_must_be_empty out &&
 	test -s err)
 '
+
 test_expect_success 'git pull --cleanup errors early on invalid argument' '
 	mkdir clonedcleanup &&
 	(cd clonedcleanup && git init &&

@@ -85,6 +86,21 @@ test_expect_success 'git pull --cleanup errors early on invalid argument' '
 	test -s err)
 '
 
+test_expect_success 'git pull --no-write-fetch-head fails' '
+	mkdir clonedwfh &&
+	(cd clonedwfh && git init &&
+	test_must_fail git pull --no-write-fetch-head "../parent" >out 2>err &&
+	test_must_be_empty out &&
+	test_i18ngrep "no-write-fetch-head" err)
+'
+
+test_expect_success 'git pull succeeds with fetch.writeFetchHEAD=false' '
+	mkdir clonedwfhconfig &&
+	(cd clonedwfhconfig && git init &&
+	git config fetch.writeFetchHEAD false &&
+	git pull "../parent" >out 2>err &&
+	grep FETCH_HEAD err)
+'
 
 test_expect_success 'git pull --force' '
 	mkdir clonedoldstyle &&

gitgitgadget

From: Derrick Stolee <[email protected]>

Performing maintenance on a Git repository involves writing data to the .git directory, which is not safe to do with multiple writers attempting the same operation. Ensure that only one 'git maintenance' process is running at a time by holding a file-based lock. Simply the presence of the .git/maintenance.lock file will prevent future maintenance. This lock is never committed, since it does not represent meaningful data. Instead, it is only a placeholder.

If the lock file already exists, then fail silently. This will become very important later when we implement the 'fetch' task, as this is our stop-gap from creating a recursive process loop between 'git fetch' and 'git maintenance run'.

---
 builtin/gc.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

index b7f64891cd..b57bc7b0ff 100644
@@ -829,6 +829,25 @@ static int maintenance_run(void)
 {
 	int i;
 	int result = 0;
+	struct lock_file lk;
+	struct repository *r = the_repository;
+	char *lock_path = xstrfmt("%s/maintenance", r->objects->odb->path);
+
+	if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) {
+		/*
+		 * Another maintenance command is running.
+		 *
+		 * If --auto was provided, then it is likely due to a
+		 * recursive process stack. Do not report an error in
+		 * that case.
+		 */
+		if (!opts.auto_flag && !opts.quiet)
+			error(_("lock file '%s' exists, skipping maintenance"),
+			      lock_path);
+		free(lock_path);
+		return 0;
+	}
+	free(lock_path);
 
 	if (opts.tasks_selected)
 		QSORT(tasks, TASK__COUNT, compare_tasks_by_selection);

@@ -846,6 +865,7 @@ static int maintenance_run(void)
 		}
 	}
 
+	rollback_lock_file(&lk);
 	return result;
 }
 

gitgitgadget

From: Derrick Stolee <[email protected]>

One goal of background maintenance jobs is to allow a user to disable auto-gc (gc.auto=0) but keep their repository in a clean state. Without any cleanup, loose objects will clutter the object database and slow operations. In addition, the loose objects will take up extra space because they are not stored with deltas against similar objects.

Create a 'loose-objects' task for the 'git maintenance run' command. This helps clean up loose objects without disrupting concurrent Git commands using the following sequence of events:

1. Run 'git prune-packed' to delete any loose objects that exist in a pack-file. Concurrent commands will prefer the packed version of the object to the loose version. (Of course, there are exceptions for commands that specifically care about the location of an object. These are rare for a user to run on purpose, and we hope a user that has selected background maintenance will not be trying to do foreground maintenance.)

2. Run 'git pack-objects' on a batch of loose objects. These objects are grouped by scanning the loose object directories in lexicographic order until listing all loose objects -or- reaching 50,000 objects. This is more than enough if the loose objects are created only by a user doing normal development. We noticed users with _millions_ of loose objects because VFS for Git downloads blobs on-demand when a file read operation requires populating a virtual file. This has potential of happening in partial clones if someone runs 'git grep' or otherwise evades the batch-download feature for requesting promisor objects.

This step is based on a similar step in Scalar [1] and VFS for Git.
[1] https://github.com/microsoft/scalar/blob/master/Scalar.Common/Maintenance/LooseObjectsStep.cs

---
 Documentation/git-maintenance.txt | 11 ++++
 builtin/gc.c                      | 97 +++++++++++++++++++++++++++++++
 t/t7900-maintenance.sh            | 39 +++++++++++++
 3 files changed, 147 insertions(+)

index d134192fa8..077929b691 100644
@@ -76,6 +76,17 @@ gc::
 	It can also be disruptive in some situations, as it deletes stale
 	data.
 
+loose-objects::
+	The `loose-objects` job cleans up loose objects and places them into
+	pack-files. In order to prevent race conditions with concurrent Git
+	commands, it follows a two-step process. First, it deletes any loose
+	objects that already exist in a pack-file; concurrent Git processes
+	will examine the pack-file for the object data instead of the loose
+	object. Second, it creates a new pack-file (starting with "loose-")
+	containing a batch of loose objects. The batch size is limited to 50
+	thousand objects to prevent the job from taking too long on a
+	repository with many loose objects.
+
 OPTIONS
 -------
 --auto::

index 5294396a24..27a423a4f2 100755
@@ -67,4 +67,43 @@ test_expect_success 'prefetch multiple remotes' '
 	git log prefetch/remote2/two
 '
 
+test_expect_success 'loose-objects task' '
+	# Repack everything so we know the state of the object dir
+	git repack -adk &&
+
+	# Hack to stop maintenance from running during "git commit"
+	echo in use >.git/objects/maintenance.lock &&
+
+	# Assuming that "git commit" creates at least one loose object
+	test_commit create-loose-object &&
+	rm .git/objects/maintenance.lock &&
+
+	ls .git/objects >obj-dir-before &&
+	test_file_not_empty obj-dir-before &&
+	ls .git/objects/pack/*.pack >packs-before &&
+	test_line_count = 1 packs-before &&
+
+	# The first run creates a pack-file
+	# but does not delete loose objects.
+	git maintenance run --task=loose-objects &&
+	ls .git/objects >obj-dir-between &&
+	test_cmp obj-dir-before obj-dir-between &&
+	ls .git/objects/pack/*.pack >packs-between &&
+	test_line_count = 2 packs-between &&
+	ls .git/objects/pack/loose-*.pack >loose-packs &&
+	test_line_count = 1 loose-packs &&
+
+	# The second run deletes loose objects
+	# but does not create a pack-file.
+	git maintenance run --task=loose-objects &&
+	ls .git/objects >obj-dir-after &&
+	cat >expect <<-\EOF &&
+	info
+	pack
+	EOF
+	test_cmp expect obj-dir-after &&
+	ls .git/objects/pack/*.pack >packs-after &&
+	test_cmp packs-between packs-after
+'
+
 test_done

gitgitgadget

From: Derrick Stolee <[email protected]>

When working with very large repositories, an incremental 'git fetch' command can download a large amount of data. If there are many other users pushing to a common repo, then this data can rival the initial pack-file size of a 'git clone' of a medium-size repo.

Users may want to keep the data on their local repos as close as possible to the data on the remote repos by fetching periodically in the background. This can break up a large daily fetch into several smaller hourly fetches.

The task is called "prefetch" because it is work done in advance of a foreground fetch to make that 'git fetch' command much faster.

However, if we simply ran 'git fetch <remote>' in the background, then the user running a foregroudn 'git fetch <remote>' would lose some important feedback when a new branch appears or an existing branch updates. This is especially true if a remote branch is force-updated and this isn't noticed by the user because it occurred in the background. Further, the functionality of 'git push --force-with-lease' becomes suspect.

When running 'git fetch <remote> <options>' in the background, use the following options for careful updating:

1. --no-tags prevents getting a new tag when a user wants to see the new tags appear in their foreground fetches.

2. --refmap= removes the configured refspec which usually updates refs/remotes/<remote>/* with the refs advertised by the remote. While this looks confusing, this was documented and tested by b40a50264ac (fetch: document and test --refmap="", 2020-01-21), including this sentence in the documentation:

Providing an empty <refspec> to the --refmap option causes Git to ignore the configured refspecs and rely entirely on the refspecs supplied as command-line arguments.

3. By adding a new refspec "+refs/heads/*:refs/prefetch/<remote>/*" we can ensure that we actually load the new values somewhere in our refspace while not updating refs/heads or refs/remotes. By storing these refs here, the commit-graph job will update the commit-graph with the commits from these hidden refs.

4. --prune will delete the refs/prefetch/<remote> refs that no longer appear on the remote.

5. --no-write-fetch-head prevents updating FETCH_HEAD.

We've been using this step as a critical background job in Scalar
[1] (and VFS for Git). This solved a pain point that was showing up in user reports: fetching was a pain! Users do not like waiting to download the data that was created while they were away from their machines. After implementing background fetch, the foreground fetch commands sped up significantly because they mostly just update refs and download a small amount of new data. The effect is especially dramatic when paried with --no-show-forced-udpates (through fetch.showForcedUpdates=false).

[1] https://github.com/microsoft/scalar/blob/master/Scalar.Common/Maintenance/FetchStep.cs

---
 Documentation/git-maintenance.txt | 15 +++++++++
 builtin/gc.c                      | 52 +++++++++++++++++++++++++++++++
 t/t7900-maintenance.sh            | 24 ++++++++++++++
 3 files changed, 91 insertions(+)

index 9204762e21..d134192fa8 100644
@@ -53,6 +53,21 @@ since it will not expire `.graph` files that were in the previous
 `commit-graph-chain` file. They will be deleted by a later run based on
 the expiration delay.
 
+prefetch::
+	The `prefetch` task updates the object directory with the latest
+	objects from all registered remotes. For each remote, a `git fetch`
+	command is run. The refmap is custom to avoid updating local or remote
+	branches (those in `refs/heads` or `refs/remotes`). Instead, the
+	remote refs are stored in `refs/prefetch/<remote>/`. Also, tags are
+	not updated.
++
+This is done to avoid disrupting the remote-tracking branches. The end users
+expect these refs to stay unmoved unless they initiate a fetch.  With prefetch
+task, however, the objects necessary to complete a later real fetch would
+already be obtained, so the real fetch would go faster.  In the ideal case,
+it will just become an update to bunch of remote-tracking branches without
+any object transfer.
+
 gc::
 	Cleanup unnecessary files and optimize the local repository. "GC"
 	stands for "garbage collection," but this task performs many

index 3cdccb24df..5294396a24 100755
@@ -43,4 +43,28 @@ test_expect_success 'run --task duplicate' '
 	test_i18ngrep "cannot be selected multiple times" err
 '
 
+test_expect_success 'run --task=prefetch with no remotes' '
+	git maintenance run --task=prefetch 2>err &&
+	test_must_be_empty err
+'
+
+test_expect_success 'prefetch multiple remotes' '
+	git clone . clone1 &&
+	git clone . clone2 &&
+	git remote add remote1 "file://$(pwd)/clone1" &&
+	git remote add remote2 "file://$(pwd)/clone2" &&
+	git -C clone1 switch -c one &&
+	git -C clone2 switch -c two &&
+	test_commit -C clone1 one &&
+	test_commit -C clone2 two &&
+	GIT_TRACE2_EVENT="$(pwd)/run-prefetch.txt" git maintenance run --task=prefetch &&
+	grep ",\"fetch\",\"remote1\"" run-prefetch.txt &&
+	grep ",\"fetch\",\"remote2\"" run-prefetch.txt &&
+	test_path_is_missing .git/refs/remotes &&
+	test_cmp clone1/.git/refs/heads/one .git/refs/prefetch/remote1/one &&
+	test_cmp clone2/.git/refs/heads/two .git/refs/prefetch/remote2/two &&
+	git log prefetch/remote1/one &&
+	git log prefetch/remote2/two
+'
+
 test_done

gitgitgadget

From: Derrick Stolee <[email protected]>

The core.multiPackIndex setting has been around since c4d25228ebb (config: create core.multiPackIndex setting, 2018-07-12), but has been disabled by default. If a user wishes to use the multi-pack-index feature, then they must enable this config and run 'git multi-pack-index write'.

The multi-pack-index feature is relatively stable now, so make the config option true by default. For users that do not use a multi-pack-index, the only extra cost will be a file lookup to see if a multi-pack-index file exists (once per process, per object directory).

Also, this config option will be referenced by an upcoming "incremental-repack" task in the maintenance builtin, so move the config option into the repository settings struct. Note that if GIT_TEST_MULTI_PACK_INDEX=1, then we want to ignore the config option and treat core.multiPackIndex as enabled.

---
 Documentation/config/core.txt |  4 ++--
 midx.c                        | 11 +++--------
 repo-settings.c               |  6 ++++++
 repository.h                  |  2 ++
 4 files changed, 13 insertions(+), 10 deletions(-)

index 74619a9c03..86c91d5381 100644
@@ -606,8 +606,8 @@ core.useReplaceRefs::
 
 core.multiPackIndex::
 	Use the multi-pack-index file to track multiple packfiles using a
-	single index. See link:technical/multi-pack-index.html[the
-	multi-pack-index design document].
+	single index. See linkgit:git-multi-pack-index[1] for more
+	information. Defaults to true.
 
 core.sparseCheckout::
 	Enable "sparse checkout" feature. See linkgit:git-sparse-checkout[1]

index a5fb797ede..ef499cf504 100644
@@ -10,6 +10,7 @@
 #include "progress.h"
 #include "trace2.h"
 #include "run-command.h"
+#include "repository.h"
 
 #define MIDX_SIGNATURE 0x4d494458 /* "MIDX" */
 #define MIDX_VERSION 1

@@ -384,15 +385,9 @@ int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, i
 {
 	struct multi_pack_index *m;
 	struct multi_pack_index *m_search;
-	int config_value;
-	static int env_value = -1;
 
-	if (env_value < 0)
-		env_value = git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0);
-
-	if (!env_value &&
-	    (repo_config_get_bool(r, "core.multipackindex", &config_value) ||
-	    !config_value))
+	prepare_repo_settings(r);
+	if (!r->settings.core_multi_pack_index)
 		return 0;
 
 	for (m_search = r->objects->multi_pack_index; m_search; m_search = m_search->next)

index 0918408b34..5bd2c22726 100644
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "config.h"
 #include "repository.h"
+#include "midx.h"
 
 #define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0)
 

@@ -47,6 +48,11 @@ void prepare_repo_settings(struct repository *r)
 		r->settings.pack_use_sparse = value;
 	UPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);
 
+	value = git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0);
+	if (value || !repo_config_get_bool(r, "core.multipackindex", &value))
+		r->settings.core_multi_pack_index = value;
+	UPDATE_DEFAULT_BOOL(r->settings.core_multi_pack_index, 1);
+
 	if (!repo_config_get_bool(r, "feature.manyfiles", &value) && value) {
 		UPDATE_DEFAULT_BOOL(r->settings.index_version, 4);
 		UPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_WRITE);

index 3c1f7d54bd..3901ce0b65 100644
@@ -37,6 +37,8 @@ struct repo_settings {
 
 	int pack_use_sparse;
 	enum fetch_negotiation_setting fetch_negotiation_algorithm;
+
+	int core_multi_pack_index;
 };
 
 struct repository {

gitgitgadget

From: Derrick Stolee <[email protected]>

Currently, a normal run of "git maintenance run" will only run the 'gc' task, as it is the only one enabled. This is mostly for backwards- compatible reasons since "git maintenance run --auto" commands replaced previous "git gc --auto" commands after some Git processes. Users could manually run specific maintenance tasks by calling "git maintenance run --task=<task>" directly.

Allow users to customize which steps are run automatically using config. The 'maintenance.<task>.enabled' option then can turn on these other tasks (or turn off the 'gc' task).

---
 Documentation/config.txt             |  2 ++
 Documentation/config/maintenance.txt |  4 ++++
 Documentation/git-maintenance.txt    |  6 +++++-
 builtin/gc.c                         | 19 +++++++++++++++++++
 t/t7900-maintenance.sh               | 12 ++++++++++++
 5 files changed, 42 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/config/maintenance.txt

index ef0768b91a..2783b825f9 100644
@@ -396,6 +396,8 @@ include::config/mailinfo.txt[]
 
 include::config/mailmap.txt[]
 
+include::config/maintenance.txt[]
+
 include::config/man.txt[]
 
 include::config/merge.txt[]

new file mode 100644
index 0000000000..370cbfb42f
@@ -0,0 +1,4 @@
+maintenance.<task>.enabled::
+	This boolean config option controls whether the maintenance task
+	with name `<task>` is run when no `--task` option is specified.
+	By default, only `maintenance.gc.enabled` is true.

index a598582986..1bd105918f 100644
@@ -30,7 +30,11 @@ SUBCOMMANDS
 -----------
 
 run::
-	Run one or more maintenance tasks.
+	Run one or more maintenance tasks. If one or more `--task` options
+	are specified, then those tasks are run in that order. Otherwise,
+	the tasks are determined by which `maintenance.<task>.enabled`
+	config options are true. By default, only `maintenance.gc.enabled`
+	is true.
 
 TASKS
 -----

index d94eb3e6ad..c599690591 100644
@@ -1171,6 +1171,24 @@ static int maintenance_run(void)
 	return result;
 }
 
+static void initialize_task_config(void)
+{
+	int i;
+	struct strbuf config_name = STRBUF_INIT;
+	for (i = 0; i < TASK__COUNT; i++) {
+		int config_value;
+
+		strbuf_setlen(&config_name, 0);
+		strbuf_addf(&config_name, "maintenance.%s.enabled",
+			    tasks[i].name);
+
+		if (!git_config_get_bool(config_name.buf, &config_value))
+			tasks[i].enabled = config_value;
+	}
+
+	strbuf_release(&config_name);
+}
+
 static int task_option_parse(const struct option *opt,
 			     const char *arg, int unset)
 {

@@ -1224,6 +1242,7 @@ int cmd_maintenance(int argc, const char **argv, const char *prefix)
 				   builtin_maintenance_options);
 
 	opts.quiet = !isatty(2);
+	initialize_task_config();
 
 	argc = parse_options(argc, argv, prefix,
 			     builtin_maintenance_options,

index 4e9c1dfa0f..affa268dec 100755
@@ -21,6 +21,18 @@ test_expect_success 'run [--auto|--quiet]' '
 	grep ",\"gc\",\"--quiet\"" run-quiet.txt
 '
 
+test_expect_success 'maintenance.<task>.enabled' '
+	git config maintenance.gc.enabled false &&
+	git config maintenance.commit-graph.enabled true &&
+	git config maintenance.loose-objects.enabled true &&
+	GIT_TRACE2_EVENT="$(pwd)/run-config.txt" git maintenance run &&
+	! grep ",\"fetch\"" run-config.txt &&
+	! grep ",\"gc\"" run-config.txt &&
+	! grep ",\"multi-pack-index\"" run-config.txt &&
+	grep ",\"commit-graph\"" run-config.txt &&
+	grep ",\"prune-packed\"" run-config.txt
+'
+
 test_expect_success 'run --task=<task>' '
 	GIT_TRACE2_EVENT="$(pwd)/run-commit-graph.txt" git maintenance run --task=commit-graph &&
 	GIT_TRACE2_EVENT="$(pwd)/run-gc.txt" git maintenance run --task=gc &&

gitgitgadget

From: Derrick Stolee <[email protected]>

The 'git maintenance run' command has an '--auto' option. This is used by other Git commands such as 'git commit' or 'git fetch' to check if maintenance should be run after adding data to the repository.

Previously, this --auto option was only used to add the argument to the 'git gc' command as part of the 'gc' task. We will be expanding the other tasks to perform a check to see if they should do work as part of the --auto flag, when they are enabled by config.

First, update the 'gc' task to perform the auto check inside the maintenance process. This prevents running an extra 'git gc --auto' command when not needed. It also shows a model for other tasks.

Second, use the 'auto_condition' function pointer as a signal for whether we enable the maintenance task under '--auto'. For instance, we do not want to enable the 'fetch' task in '--auto' mode, so that function pointer will remain NULL.

Now that we are not automatically calling 'git gc', a test in t5514-fetch-multiple.sh must be changed to watch for 'git maintenance' instead.

We continue to pass the '--auto' option to the 'git gc' command when necessary, because of the gc.autoDetach config option changes behavior. Likely, we will want to absorb the daemonizing behavior implied by gc.autoDetach as a maintenance.autoDetach config option.

---
 builtin/gc.c              | 16 ++++++++++++++++
 t/t5514-fetch-multiple.sh |  2 +-
 t/t7900-maintenance.sh    |  2 +-
 3 files changed, 18 insertions(+), 2 deletions(-)

index c599690591..1c449b3776 100644
@@ -1074,9 +1074,17 @@ static int maintenance_task_incremental_repack(void)
 
 typedef int maintenance_task_fn(void);
 
+/*
+ * An auto condition function returns 1 if the task should run
+ * and 0 if the task should NOT run. See needs_to_gc() for an
+ * example.
+ */
+typedef int maintenance_auto_fn(void);
+
 struct maintenance_task {
 	const char *name;
 	maintenance_task_fn *fn;
+	maintenance_auto_fn *auto_condition;
 	unsigned enabled:1,
 		 selected:1;
 	int selected_order;

@@ -1109,6 +1117,7 @@ static struct maintenance_task tasks[] = {
 	[TASK_GC] = {
 		"gc",
 		maintenance_task_gc,
+		need_to_gc,
 		1,
 	},
 	[TASK_COMMIT_GRAPH] = {

@@ -1161,6 +1170,11 @@ static int maintenance_run(void)
 		if (!opts.tasks_selected && !tasks[i].enabled)
 			continue;
 
+		if (opts.auto_flag &&
+		    (!tasks[i].auto_condition ||
+		     !tasks[i].auto_condition()))
+			continue;
+
 		if (tasks[i].fn()) {
 			error(_("task '%s' failed"), tasks[i].name);
 			result = 1;

@@ -1175,6 +1189,8 @@ static void initialize_task_config(void)
 {
 	int i;
 	struct strbuf config_name = STRBUF_INIT;
+	gc_config();
+
 	for (i = 0; i < TASK__COUNT; i++) {
 		int config_value;
 

index de8e2f1531..bd202ec6f3 100755
@@ -108,7 +108,7 @@ test_expect_success 'git fetch --multiple (two remotes)' '
 	 GIT_TRACE=1 git fetch --multiple one two 2>trace &&
 	 git branch -r > output &&
 	 test_cmp ../expect output &&
-	 grep "built-in: git gc" trace >gc &&
+	 grep "built-in: git maintenance" trace >gc &&
 	 test_line_count = 1 gc
 	)
 '

index affa268dec..19494e6c43 100755
@@ -17,7 +17,7 @@ test_expect_success 'run [--auto|--quiet]' '
 	GIT_TRACE2_EVENT="$(pwd)/run-auto.txt" git maintenance run --auto &&
 	GIT_TRACE2_EVENT="$(pwd)/run-quiet.txt" git maintenance run --quiet &&
 	grep ",\"gc\"]" run-no-auto.txt  &&
-	grep ",\"gc\",\"--auto\"" run-auto.txt &&
+	! grep ",\"gc\",\"--auto\"" run-auto.txt &&
 	grep ",\"gc\",\"--quiet\"" run-quiet.txt
 '
 

gitgitgadget

From: Derrick Stolee <[email protected]>

When repacking during the 'incremental-repack' task, we use the --batch-size option in 'git multi-pack-index repack'. The initial setting used --batch-size=0 to repack everything into a single pack-file. This is not sustainable for a large repository. The amount of work required is also likely to use too many system resources for a background job.

Update the 'incremental-repack' task by dynamically computing a --batch-size option based on the current pack-file structure.

The dynamic default size is computed with this idea in mind for a client repository that was cloned from a very large remote: there is likely one "big" pack-file that was created at clone time. Thus, do not try repacking it as it is likely packed efficiently by the server.

Instead, we select the second-largest pack-file, and create a batch size that is one larger than that pack-file. If there are three or more pack-files, then this guarantees that at least two will be combined into a new pack-file.

Of course, this means that the second-largest pack-file size is likely to grow over time and may eventually surpass the initially-cloned pack-file. Recall that the pack-file batch is selected in a greedy
manner: the packs are considered from oldest to newest and are selected if they have size smaller than the batch size until the total selected size is larger than the batch size. Thus, that oldest "clone" pack will be first to repack after the new data creates a pack larger than that.

We also want to place some limits on how large these pack-files become, in order to bound the amount of time spent repacking. A maximum batch-size of two gigabytes means that large repositories will never be packed into a single pack-file using this job, but also that repack is rather expensive. This is a trade-off that is valuable to have if the maintenance is being run automatically or in the background. Users who truly want to optimize for space and performance (and are willing to pay the upfront cost of a full repack) can use the 'gc' task to do so.

---
 builtin/gc.c           | 43 +++++++++++++++++++++++++++++++++++++++++-
 t/t7900-maintenance.sh |  5 +++--
 2 files changed, 45 insertions(+), 3 deletions(-)

index 99ab1f5e9d..d94eb3e6ad 100644
@@ -988,6 +988,46 @@ static int multi_pack_index_expire(void)
 	return 0;
 }
 
+#define TWO_GIGABYTES (0x7FFF)
+
+static off_t get_auto_pack_size(void)
+{
+	/*
+	 * The "auto" value is special: we optimize for
+	 * one large pack-file (i.e. from a clone) and
+	 * expect the rest to be small and they can be
+	 * repacked quickly.
+	 *
+	 * The strategy we select here is to select a
+	 * size that is one more than the second largest
+	 * pack-file. This ensures that we will repack
+	 * at least two packs if there are three or more
+	 * packs.
+	 */
+	off_t max_size = 0;
+	off_t second_largest_size = 0;
+	off_t result_size;
+	struct packed_git *p;
+	struct repository *r = the_repository;
+
+	reprepare_packed_git(r);
+	for (p = get_all_packs(r); p; p = p->next) {
+		if (p->pack_size > max_size) {
+			second_largest_size = max_size;
+			max_size = p->pack_size;
+		} else if (p->pack_size > second_largest_size)
+			second_largest_size = p->pack_size;
+	}
+
+	result_size = second_largest_size + 1;
+
+	/* But limit ourselves to a batch size of 2g */
+	if (result_size > TWO_GIGABYTES)
+		result_size = TWO_GIGABYTES;
+
+	return result_size;
+}
+
 static int multi_pack_index_repack(void)
 {
 	struct child_process child = CHILD_PROCESS_INIT;

@@ -998,7 +1038,8 @@ static int multi_pack_index_repack(void)
 	if (opts.quiet)
 		strvec_push(&child.args, "--no-progress");
 
-	strvec_push(&child.args, "--batch-size=0");
+	strvec_pushf(&child.args, "--batch-size=%"PRIuMAX,
+				  (uintmax_t)get_auto_pack_size());
 
 	close_object_store(the_repository->objects);
 

index 0cc59adb21..4e9c1dfa0f 100755
@@ -138,10 +138,11 @@ test_expect_success 'incremental-repack task' '
 	test_line_count = 4 packs-between &&
 
 	# the job deletes the two old packs, and does not write
-	# a new one because only one pack remains.
+	# a new one because the batch size is not high enough to
+	# pack the largest pack-file.
 	git maintenance run --task=incremental-repack &&
 	ls .git/objects/pack/*.pack >packs-after &&
-	test_line_count = 1 packs-after
+	test_line_count = 2 packs-after
 '
 
 test_done

gitgitgadget

From: Derrick Stolee <[email protected]>

The previous change cleaned up loose objects using the 'loose-objects' that can be run safely in the background. Add a similar job that performs similar cleanups for pack-files.

One issue with running 'git repack' is that it is designed to repack all pack-files into a single pack-file. While this is the most space-efficient way to store object data, it is not time or memory efficient. This becomes extremely important if the repo is so large that a user struggles to store two copies of the pack on their disk.

Instead, perform an "incremental" repack by collecting a few small pack-files into a new pack-file. The multi-pack-index facilitates this process ever since 'git multi-pack-index expire' was added in 19575c7 (multi-pack-index: implement 'expire' subcommand, 2019-06-10) and 'git multi-pack-index repack' was added in ce1e4a1 (midx: implement midx_repack(), 2019-06-10).

The 'incremental-repack' task runs the following steps:

1. 'git multi-pack-index write' creates a multi-pack-index file if one did not exist, and otherwise will update the multi-pack-index with any new pack-files that appeared since the last write. This is particularly relevant with the background fetch job.

When the multi-pack-index sees two copies of the same object, it stores the offset data into the newer pack-file. This means that some old pack-files could become "unreferenced" which I will use to mean "a pack-file that is in the pack-file list of the multi-pack-index but none of the objects in the multi-pack-index reference a location inside that pack-file."

2. 'git multi-pack-index expire' deletes any unreferenced pack-files and updaes the multi-pack-index to drop those pack-files from the list. This is safe to do as concurrent Git processes will see the multi-pack-index and not open those packs when looking for object contents. (Similar to the 'loose-objects' job, there are some Git commands that open pack-files regardless of the multi-pack-index, but they are rarely used. Further, a user that self-selects to use background operations would likely refrain from using those commands.)

3. 'git multi-pack-index repack --bacth-size=<size>' collects a set of pack-files that are listed in the multi-pack-index and creates a new pack-file containing the objects whose offsets are listed by the multi-pack-index to be in those objects. The set of pack- files is selected greedily by sorting the pack-files by modified time and adding a pack-file to the set if its "expected size" is smaller than the batch size until the total expected size of the selected pack-files is at least the batch size. The "expected size" is calculated by taking the size of the pack-file divided by the number of objects in the pack-file and multiplied by the number of objects from the multi-pack-index with offset in that pack-file. The expected size approximates how much data from that pack-file will contribute to the resulting pack-file size. The intention is that the resulting pack-file will be close in size to the provided batch size.

The next run of the incremental-repack task will delete these repacked pack-files during the 'expire' step.

In this version, the batch size is set to "0" which ignores the size restrictions when selecting the pack-files. It instead selects all pack-files and repacks all packed objects into a single pack-file. This will be updated in the next change, but it requires doing some calculations that are better isolated to a separate change.

Each of the above steps update the multi-pack-index file. After each step, we verify the new multi-pack-index. If the new multi-pack-index is corrupt, then delete the multi-pack-index, rewrite it from scratch, and stop doing the later steps of the job. This is intended to be an extra-safe check without leaving a repo with many pack-files without a multi-pack-index.

These steps are based on a similar background maintenance step in Scalar (and VFS for Git) [1]. This was incredibly effective for users of the Windows OS repository. After using the same VFS for Git repository for over a year, some users had _thousands_ of pack-files that combined to up to 250 GB of data. We noticed a few users were running into the open file descriptor limits (due in part to a bug in the multi-pack-index fixed by af96fe3 (midx: add packs to packed_git linked list, 2019-04-29).

These pack-files were mostly small since they contained the commits and trees that were pushed to the origin in a given hour. The GVFS protocol includes a "prefetch" step that asks for pre-computed pack- files containing commits and trees by timestamp. These pack-files were grouped into "daily" pack-files once a day for up to 30 days. If a user did not request prefetch packs for over 30 days, then they would get the entire history of commits and trees in a new, large pack-file. This led to a large number of pack-files that had poor delta compression.

By running this pack-file maintenance step once per day, these repos with thousands of packs spanning 200+ GB dropped to dozens of pack- files spanning 30-50 GB. This was done all without removing objects from the system and using a constant batch size of two gigabytes. Once the work was done to reduce the pack-files to small sizes, the batch size of two gigabytes means that not every run triggers a repack operation, so the following run will not expire a pack-file. This has kept these repos in a "clean" state.

[1] https://github.com/microsoft/scalar/blob/master/Scalar.Common/Maintenance/PackfileMaintenanceStep.cs

---
 Documentation/git-maintenance.txt |  15 ++++
 builtin/gc.c                      | 112 ++++++++++++++++++++++++++++++
 midx.c                            |   2 +-
 midx.h                            |   1 +
 t/t5319-multi-pack-index.sh       |   1 +
 t/t7900-maintenance.sh            |  38 ++++++++++
 6 files changed, 168 insertions(+), 1 deletion(-)

index 077929b691..a598582986 100644
@@ -87,6 +87,21 @@ loose-objects::
 	thousand objects to prevent the job from taking too long on a
 	repository with many loose objects.
 
+incremental-repack::
+	The `incremental-repack` job repacks the object directory
+	using the `multi-pack-index` feature. In order to prevent race
+	conditions with concurrent Git commands, it follows a two-step
+	process. First, it deletes any pack-files included in the
+	`multi-pack-index` where none of the objects in the
+	`multi-pack-index` reference those pack-files; this only happens
+	if all objects in the pack-file are also stored in a newer
+	pack-file. Second, it selects a group of pack-files whose "expected
+	size" is below the batch size until the group has total expected
+	size at least the batch size; see the `--batch-size` option for
+	the `repack` subcommand in linkgit:git-multi-pack-index[1]. The
+	default batch-size is zero, which is a special case that attempts
+	to repack all pack-files into a single pack-file.
+
 OPTIONS
 -------
 --auto::

index ef499cf504..f0ebee34c8 100644
@@ -37,7 +37,7 @@
 
 #define PACK_EXPIRED UINT_MAX
 
-static char *get_midx_filename(const char *object_dir)
+char *get_midx_filename(const char *object_dir)
 {
 	return xstrfmt("%s/pack/multi-pack-index", object_dir);
 }

index b18cf53bc4..baeecc70c9 100644
@@ -37,6 +37,7 @@ struct multi_pack_index {
 
 #define MIDX_PROGRESS     (1 << 0)
 
+char *get_midx_filename(const char *object_dir);
 struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local);
 int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id);
 int bsearch_midx(const struct object_id *oid, struct multi_pack_index *m, uint32_t *result);

index 7214cab36c..2abd29a007 100755
@@ -3,6 +3,7 @@
 test_description='multi-pack-indexes'
 . ./test-lib.sh
 
+GIT_TEST_MULTI_PACK_INDEX=0
 objdir=.git/objects
 
 midx_read_expect () {

index 27a423a4f2..0cc59adb21 100755
@@ -5,6 +5,7 @@ test_description='git maintenance builtin'
 . ./test-lib.sh
 
 GIT_TEST_COMMIT_GRAPH=0
+GIT_TEST_MULTI_PACK_INDEX=0
 
 test_expect_success 'help text' '
 	test_expect_code 129 git maintenance -h 2>err &&

@@ -106,4 +107,41 @@ test_expect_success 'loose-objects task' '
 	test_cmp packs-between packs-after
 '
 
+test_expect_success 'incremental-repack task' '
+	packDir=.git/objects/pack &&
+	for i in $(test_seq 1 5)
+	do
+		test_commit $i || return 1
+	done &&
+
+	# Create three disjoint pack-files with size BIG, small, small.
+	echo HEAD~2 | git pack-objects --revs $packDir/test-1 &&
+	test_tick &&
+	git pack-objects --revs $packDir/test-2 <<-\EOF &&
+	HEAD~1
+	^HEAD~2
+	EOF
+	test_tick &&
+	git pack-objects --revs $packDir/test-3 <<-\EOF &&
+	HEAD
+	^HEAD~1
+	EOF
+	rm -f $packDir/pack-* &&
+	rm -f $packDir/loose-* &&
+	ls $packDir/*.pack >packs-before &&
+	test_line_count = 3 packs-before &&
+
+	# the job repacks the two into a new pack, but does not
+	# delete the old ones.
+	git maintenance run --task=incremental-repack &&
+	ls $packDir/*.pack >packs-between &&
+	test_line_count = 4 packs-between &&
+
+	# the job deletes the two old packs, and does not write
+	# a new one because only one pack remains.
+	git maintenance run --task=incremental-repack &&
+	ls .git/objects/pack/*.pack >packs-after &&
+	test_line_count = 1 packs-after
+'
+
 test_done

gitgitgadget

From: Derrick Stolee <[email protected]>

---
 builtin/gc.c | 2 ++
 1 file changed, 2 insertions(+)

index 6ccc0cca19..33e889bc71 100644
@@ -1312,10 +1312,12 @@ static int maintenance_run(void)
 		     !tasks[i].auto_condition()))
 			continue;
 
+		trace2_region_enter("maintenance", tasks[i].name, r);
 		if (tasks[i].fn()) {
 			error(_("task '%s' failed"), tasks[i].name);
 			result = 1;
 		}
+		trace2_region_leave("maintenance", tasks[i].name, r);
 	}
 
 	rollback_lock_file(&lk);

gitgitgadget

From: Derrick Stolee <[email protected]>

The incremental-repack task updates the multi-pack-index by deleting pack- files that have been replaced with new packs, then repacking a batch of small pack-files into a larger pack-file. This incremental repack is faster than rewriting all object data, but is slower than some other maintenance activities.

The 'maintenance.incremental-repack.auto' config option specifies how many pack-files should exist outside of the multi-pack-index before running the step. These pack-files could be created by 'git fetch' commands or by the loose-objects task. The default value is 10.

Setting the option to zero disables the task with the '--auto' option, and a negative value makes the task run every time.

---
 Documentation/config/maintenance.txt |  9 ++++++++
 builtin/gc.c                         | 31 ++++++++++++++++++++++++++++
 t/t7900-maintenance.sh               | 30 +++++++++++++++++++++++++++
 3 files changed, 70 insertions(+)

index a9442dd260..22229e7174 100644
@@ -21,3 +21,12 @@ maintenance.loose-objects.auto::
 	positive value implies the command should run when the number of
 	loose objects is at least the value of `maintenance.loose-objects.auto`.
 	The default value is 100.
+
+maintenance.incremental-repack.auto::
+	This integer config option controls how often the `incremental-repack`
+	task should be run as part of `git maintenance run --auto`. If zero,
+	then the `incremental-repack` task will not run with the `--auto`
+	option. A negative value will force the task to run every time.
+	Otherwise, a positive value implies the command should run when the
+	number of pack-files not in the multi-pack-index is at least the value
+	of `maintenance.incremental-repack.auto`. The default value is 10.

index 3b27dc7e7f..6ccc0cca19 100644
@@ -31,6 +31,7 @@
 #include "remote.h"
 #include "midx.h"
 #include "refs.h"
+#include "object-store.h"
 
 #define FAILED_RUN "failed to run %s"
 

@@ -1029,6 +1030,35 @@ static int maintenance_task_loose_objects(void)
 	return prune_packed() || pack_loose();
 }
 
+static int incremental_repack_auto_condition(void)
+{
+	struct packed_git *p;
+	int enabled;
+	int incremental_repack_auto_limit = 10;
+	int count = 0;
+
+	if (git_config_get_bool("core.multiPackIndex", &enabled) ||
+	    !enabled)
+		return 0;
+
+	git_config_get_int("maintenance.incremental-repack.auto",
+			   &incremental_repack_auto_limit);
+
+	if (!incremental_repack_auto_limit)
+		return 0;
+	if (incremental_repack_auto_limit < 0)
+		return 1;
+
+	for (p = get_packed_git(the_repository);
+	     count < incremental_repack_auto_limit && p;
+	     p = p->next) {
+		if (!p->multi_pack_index)
+			count++;
+	}
+
+	return count >= incremental_repack_auto_limit;
+}
+
 static int multi_pack_index_write(void)
 {
 	struct child_process child = CHILD_PROCESS_INIT;

@@ -1218,6 +1248,7 @@ static struct maintenance_task tasks[] = {
 	[TASK_INCREMENTAL_REPACK] = {
 		"incremental-repack",
 		maintenance_task_incremental_repack,
+		incremental_repack_auto_condition,
 	},
 	[TASK_GC] = {
 		"gc",

index 32ac5c9fb7..c8ccaa0a3d 100755
@@ -182,4 +182,34 @@ test_expect_success 'incremental-repack task' '
 	test_line_count = 2 packs-after
 '
 
+test_expect_success 'maintenance.incremental-repack.auto' '
+	git repack -adk &&
+	git config core.multiPackIndex true &&
+	git multi-pack-index write &&
+	GIT_TRACE2_EVENT=1 git -c maintenance.incremental-repack.auto=1 \
+		maintenance run --auto --task=incremental-repack >out &&
+	! grep "\"multi-pack-index\"" out &&
+	for i in 1 2
+	do
+		test_commit A-$i &&
+		git pack-objects --revs .git/objects/pack/pack <<-\EOF &&
+		HEAD
+		^HEAD~1
+		EOF
+		GIT_TRACE2_EVENT=$(pwd)/trace-A-$i git \
+			-c maintenance.incremental-repack.auto=2 \
+			maintenance run --auto --task=incremental-repack &&
+		! grep "\"multi-pack-index\"" trace-A-$i &&
+		test_commit B-$i &&
+		git pack-objects --revs .git/objects/pack/pack <<-\EOF &&
+		HEAD
+		^HEAD~1
+		EOF
+		GIT_TRACE2_EVENT=$(pwd)/trace-B-$i git \
+			-c maintenance.incremental-repack.auto=2 \
+			maintenance run --auto --task=incremental-repack >out &&
+		grep "\"multi-pack-index\"" trace-B-$i >/dev/null || return 1
+	done
+'
+
 test_done

gitgitgadget

From: Derrick Stolee <[email protected]>

The loose-objects task deletes loose objects that already exist in a pack-file, then place the remaining loose objects into a new pack-file. If this step runs all the time, then we risk creating pack-files with very few objects with every 'git commit' process. To prevent overwhelming the packs directory with small pack-files, place a minimum number of objects to justify the task.

The 'maintenance.loose-objects.auto' config option specifies a minimum number of loose objects to justify the task to run under the '--auto' option. This defaults to 100 loose objects. Setting the value to zero will prevent the step from running under '--auto' while a negative value will force it to run every time.

---
 Documentation/config/maintenance.txt |  9 +++++++++
 builtin/gc.c                         | 30 ++++++++++++++++++++++++++++
 t/t7900-maintenance.sh               | 25 +++++++++++++++++++++++
 3 files changed, 64 insertions(+)

index 9bd69b9df3..a9442dd260 100644
@@ -12,3 +12,12 @@ maintenance.commit-graph.auto::
 	reachable commits that are not in the commit-graph file is at least
 	the value of `maintenance.commit-graph.auto`. The default value is
 	100.
+
+maintenance.loose-objects.auto::
+	This integer config option controls how often the `loose-objects` task
+	should be run as part of `git maintenance run --auto`. If zero, then
+	the `loose-objects` task will not run with the `--auto` option. A
+	negative value will force the task to run every time. Otherwise, a
+	positive value implies the command should run when the number of
+	loose objects is at least the value of `maintenance.loose-objects.auto`.
+	The default value is 100.

index c85813fffe..3b27dc7e7f 100644
@@ -927,6 +927,35 @@ struct write_loose_object_data {
 	int batch_size;
 };
 
+static int loose_object_auto_limit = 100;
+
+static int loose_object_count(const struct object_id *oid,
+			       const char *path,
+			       void *data)
+{
+	int *count = (int*)data;
+	if (++(*count) >= loose_object_auto_limit)
+		return 1;
+	return 0;
+}
+
+static int loose_object_auto_condition(void)
+{
+	int count = 0;
+
+	git_config_get_int("maintenance.loose-objects.auto",
+			   &loose_object_auto_limit);
+
+	if (!loose_object_auto_limit)
+		return 0;
+	if (loose_object_auto_limit < 0)
+		return 1;
+
+	return for_each_loose_file_in_objdir(the_repository->objects->odb->path,
+					     loose_object_count,
+					     NULL, NULL, &count);
+}
+
 static int bail_on_loose(const struct object_id *oid,
 			 const char *path,
 			 void *data)

@@ -1184,6 +1213,7 @@ static struct maintenance_task tasks[] = {
 	[TASK_LOOSE_OBJECTS] = {
 		"loose-objects",
 		maintenance_task_loose_objects,
+		loose_object_auto_condition,
 	},
 	[TASK_INCREMENTAL_REPACK] = {
 		"incremental-repack",

index 19494e6c43..32ac5c9fb7 100755
@@ -119,6 +119,31 @@ test_expect_success 'loose-objects task' '
 	test_cmp packs-between packs-after
 '
 
+test_expect_success 'maintenance.loose-objects.auto' '
+	git repack -adk &&
+	GIT_TRACE2_EVENT="$(pwd)/trace-lo1.txt" \
+		git -c maintenance.loose-objects.auto=1 maintenance \
+		run --auto --task=loose-objects &&
+	! grep "\"prune-packed\"" trace-lo1.txt &&
+	for i in 1 2
+	do
+		printf data-A-$i | git hash-object -t blob --stdin -w &&
+		GIT_TRACE2_EVENT="$(pwd)/trace-loA-$i" \
+			git -c maintenance.loose-objects.auto=2 \
+			maintenance run --auto --task=loose-objects &&
+		! grep "\"prune-packed\"" trace-loA-$i &&
+		printf data-B-$i | git hash-object -t blob --stdin -w &&
+		GIT_TRACE2_EVENT="$(pwd)/trace-loB-$i" \
+			git -c maintenance.loose-objects.auto=2 \
+			maintenance run --auto --task=loose-objects &&
+		grep "\"prune-packed\"" trace-loB-$i &&
+		GIT_TRACE2_EVENT="$(pwd)/trace-loC-$i" \
+			git -c maintenance.loose-objects.auto=2 \
+			maintenance run --auto --task=loose-objects &&
+		grep "\"prune-packed\"" trace-loC-$i || return 1
+	done
+'
+
 test_expect_success 'incremental-repack task' '
 	packDir=.git/objects/pack &&
 	for i in $(test_seq 1 5)

gitgitgadget

From: Derrick Stolee <[email protected]>

Now that the multi-pack-index may be written as part of auto maintenance at the end of a command, reduce the progress output when the operations are quick. Use start_delayed_progress() instead of start_progress().

Update t5319-multi-pack-index.sh to use GIT_PROGRESS_DELAY=0 now that the progress indicators are conditional.

---
 midx.c                      | 10 +++++-----
 t/t5319-multi-pack-index.sh | 14 +++++++-------
 2 files changed, 12 insertions(+), 12 deletions(-)

index f0ebee34c8..66d7053d83 100644
@@ -832,7 +832,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *
 
 	packs.pack_paths_checked = 0;
 	if (flags & MIDX_PROGRESS)
-		packs.progress = start_progress(_("Adding packfiles to multi-pack-index"), 0);
+		packs.progress = start_delayed_progress(_("Adding packfiles to multi-pack-index"), 0);
 	else
 		packs.progress = NULL;
 

@@ -969,7 +969,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *
 	}
 
 	if (flags & MIDX_PROGRESS)
-		progress = start_progress(_("Writing chunks to multi-pack-index"),
+		progress = start_delayed_progress(_("Writing chunks to multi-pack-index"),
 					  num_chunks);
 	for (i = 0; i < num_chunks; i++) {
 		if (written != chunk_offsets[i])

@@ -1104,7 +1104,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
 		return 0;
 
 	if (flags & MIDX_PROGRESS)
-		progress = start_progress(_("Looking for referenced packfiles"),
+		progress = start_delayed_progress(_("Looking for referenced packfiles"),
 					  m->num_packs);
 	for (i = 0; i < m->num_packs; i++) {
 		if (prepare_midx_pack(r, m, i))

@@ -1225,7 +1225,7 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla
 	count = xcalloc(m->num_packs, sizeof(uint32_t));
 
 	if (flags & MIDX_PROGRESS)
-		progress = start_progress(_("Counting referenced objects"),
+		progress = start_delayed_progress(_("Counting referenced objects"),
 					  m->num_objects);
 	for (i = 0; i < m->num_objects; i++) {
 		int pack_int_id = nth_midxed_pack_int_id(m, i);

@@ -1235,7 +1235,7 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla
 	stop_progress(&progress);
 
 	if (flags & MIDX_PROGRESS)
-		progress = start_progress(_("Finding and deleting unreferenced packfiles"),
+		progress = start_delayed_progress(_("Finding and deleting unreferenced packfiles"),
 					  m->num_packs);
 	for (i = 0; i < m->num_packs; i++) {
 		char *pack_name;

index 2abd29a007..26f224b0e3 100755
@@ -173,12 +173,12 @@ test_expect_success 'write progress off for redirected stderr' '
 '
 
 test_expect_success 'write force progress on for stderr' '
-	git multi-pack-index --object-dir=$objdir --progress write 2>err &&
+	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --progress write 2>err &&
 	test_file_not_empty err
 '
 
 test_expect_success 'write with the --no-progress option' '
-	git multi-pack-index --object-dir=$objdir --no-progress write 2>err &&
+	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --no-progress write 2>err &&
 	test_line_count = 0 err
 '
 

@@ -335,17 +335,17 @@ test_expect_success 'git-fsck incorrect offset' '
 '
 
 test_expect_success 'repack progress off for redirected stderr' '
-	git multi-pack-index --object-dir=$objdir repack 2>err &&
+	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack 2>err &&
 	test_line_count = 0 err
 '
 
 test_expect_success 'repack force progress on for stderr' '
-	git multi-pack-index --object-dir=$objdir --progress repack 2>err &&
+	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --progress repack 2>err &&
 	test_file_not_empty err
 '
 
 test_expect_success 'repack with the --no-progress option' '
-	git multi-pack-index --object-dir=$objdir --no-progress repack 2>err &&
+	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --no-progress repack 2>err &&
 	test_line_count = 0 err
 '
 

@@ -489,7 +489,7 @@ test_expect_success 'expire progress off for redirected stderr' '
 test_expect_success 'expire force progress on for stderr' '
 	(
 		cd dup &&
-		git multi-pack-index --progress expire 2>err &&
+		GIT_PROGRESS_DELAY=0 git multi-pack-index --progress expire 2>err &&
 		test_file_not_empty err
 	)
 '

@@ -497,7 +497,7 @@ test_expect_success 'expire force progress on for stderr' '
 test_expect_success 'expire with the --no-progress option' '
 	(
 		cd dup &&
-		git multi-pack-index --no-progress expire 2>err &&
+		GIT_PROGRESS_DELAY=0 git multi-pack-index --no-progress expire 2>err &&
 		test_line_count = 0 err
 	)
 '

gitgitgadget

From: Derrick Stolee <[email protected]>

Instead of writing a new commit-graph in every 'git maintenance run --auto' process (when maintenance.commit-graph.enalbed is configured to be true), only write when there are "enough" commits not in a commit-graph file.

This count is controlled by the maintenance.commit-graph.auto config option.

To compute the count, use a depth-first search starting at each ref, and leaving markers using the PARENT1 flag. If this count reaches the limit, then terminate early and start the task. Otherwise, this operation will peel every ref and parse the commit it points to. If these are all in the commit-graph, then this is typically a very fast operation. Users with many refs might feel a slow-down, and hence could consider updating their limit to be very small. A negative value will force the step to run every time.

---
 Documentation/config/maintenance.txt | 10 ++++
 builtin/gc.c                         | 76 ++++++++++++++++++++++++++++
 object.h                             |  1 +
 3 files changed, 87 insertions(+)

index 370cbfb42f..9bd69b9df3 100644
@@ -2,3 +2,13 @@ maintenance.<task>.enabled::
 	This boolean config option controls whether the maintenance task
 	with name `<task>` is run when no `--task` option is specified.
 	By default, only `maintenance.gc.enabled` is true.
+
+maintenance.commit-graph.auto::
+	This integer config option controls how often the `commit-graph` task
+	should be run as part of `git maintenance run --auto`. If zero, then
+	the `commit-graph` task will not run with the `--auto` option. A
+	negative value will force the task to run every time. Otherwise, a
+	positive value implies the command should run when the number of
+	reachable commits that are not in the commit-graph file is at least
+	the value of `maintenance.commit-graph.auto`. The default value is
+	100.

index 96a2105859..30d4e0f6a0 100644
@@ -74,6 +74,7 @@ struct object_array {
  * list-objects-filter.c:                                      21
  * builtin/fsck.c:           0--3
  * builtin/index-pack.c:                                     2021
+ * builtin/maintenance.c:                           16
  * builtin/pack-objects.c:                                   20
  * builtin/reflog.c:                   10--12
  * builtin/show-branch.c:    0-------------------------------------------26

gitgitgadget

The update to tests for "git pull" looks reasonable, too.

Thanks.

It probably has been a good way to gain experience with the renamed API for you to rebase this series, but I think we'd have to spend a similar amount of effort yet again, as I expect that the jk/strvec series would be rerolled at least once more, with the items[] array renamed to something else, like v[].

Let's see how it goes.

Thanks.

Hm, do you mean that this change is too difficult to squash into the first patch?

I mean things like "what exit code does it run if there is no maintenance to do?"

[...]

That doesn't seem idiomatic within the Git codebase.

Can they be locals instead?

[...]

Hm, I thought the contract for GIT_TRACE2_EVENT is that it is JSON, which is a well defined format. To that end, I would sincerely hope that external tools are not assuming anything beyond that the format is JSON.

If that wasn't the intent, I'd rather that we not use a JSON-like format at all. That would make the expectations for evolving this interface clearer.

My proposal was just to factor this out into a function in test-lib-functions.sh so it's easy to evolve over time in one place.

Thanks and hope that helps,
Jonathan

So, ... jk/strvec got updated, and just applying these twenty patches on top no longer compiles, so I rebased them ;-)

I also adjusted the merge-fix for the 6 patches on es/config-hooks topic before making tonight's final push-out of 'seen'.

Thanks.

I think my last reply to this part missed the point a little, so let me try again.

In multi-patch series, requests to make improvements to early parts that would be obsoleted by later parts may seem strange --- isn't this just wasted work? Sure, occasionally someone will stumble on the early part using "git bisect" or "git log", but isn't that rare enough to not be worth worrying too much about?

And I'd agree with that reaction --- for a documentation nit like this one, that benefit alone is probably not enough to be worth the fuss of tweaking things.

When I focus more on early parts of a series, it is for a different
reason: from a release management point of view, it is useful to get the early part of a series "cooked" as soon as possible so it can be exposed to users in "next" and then become a foundation for later patches in "master". So we try to get as much of the early part of a series into that state as we can, and to merge the early part even without the later part as soon as the former is ready and valuable enough to users on its own.

Relatedly, ensuring each commit is "complete" in this sense on its own gives us more flexibility in case a problem is discovered later: we can revert a patch that is causing issues without having to revert the preceding part.

I think that might be part of the context I didn't convey well, and why I want to get this in tip-top shape and start using it.

Thanks,
Jonathan

I mean that there is only one task right now. Until the commit-graph task is implemented, there is no need to have a --task=<task> option.

Creating a --task=<task> option in this patch would unnecessarily bloat the patch by implementing multiple tasks while filling in the boilerplate of creating a new builtin.

Which part do you want to be idiomatic? The fact that the parse-options library typically refers to static global values or the fact that the data is not organized in a struct?

Turning these into locals is _even worse_ than the current model of static globals because that then requires updating the prototypes of all the (static) local methods that operate on the arguments. Every time a new option is added, that would cascade in method prototypes all over again.

The natural thing to do to "fix" that situation is to create a struct that holds the information from the parsed command-line arguments. But then why is it a local parameter that is passed through all of the local methods instead of a global (as presented here in this patch)?

This is a valuable suggestion, but this series is already too large to make such a change in addition to the patches already here. I'd consider this more if there were more tests that checked "did we run this subcommand with these arguments?" Better to extract a helper when this is actually used in multiple places.

Thanks,
-Stolee

Ugh. Of course. Thanks for being diligent here. I hadn't pulled this branch into microsoft/git for testing with Scalar yet, where I would have noticed this.

I'm tempted to create an EXPENSIVE test that actually checks this condition. Let's see what I can do.

Thanks,
-Stolee

[...]

Ah, that wasn't clear to me from the docs.

You're saying that "git maintenance run" runs the "gc" task? Can the documentation say so?

[...]

parse-options has no requirement about the values being global. Some older code does that, but newer code tends to use locals when appropriate.

Putting it in a struct is fine as long as that struct is being passed around. What isn't idiomatic in Git is using a global struct for namespacing.

[...]

I'm having trouble parsing that last sentence. You're saying a global is preferable over passing around a pointer to a local "opts" struct?

[...]

Hm, it's not clear to me that this would make the series significantly larger.

And on the contrary, it would make the code less fragile. I think this is important.

https://chromium.googlesource.com/chromium/src/+/master/docs/cl_respect.md#remember-communication-can-be-hard:
if you'd like to meet out of band, let me know, and I'd be happy to go into this further.

Thanks,
Jonathan

I think what Stolee is trying to say is less about a change that would make the series larger, it's about changing an already-large series.

I'm not sure that I see your argument. What we are really discussing is whether or not we should have a static struct outside of 'cmd_gc()', or a zero-initialized frame local struct within 'cmd_gc()'. I fully understand the arguments in favor of one or the other, but I struggle to grasp that this is worth our time to debate in this much detail.

I assume that the part of this document you were linking to was about meeting out-of-band to discuss an issue instead of emailing back and forth.

I appreciate your willingness to resolve this expediently, but I would much prefer if these discussions (that is, the ones about the technical content of this series) happened on-list for the benefit of other reviewers.

Thanks,
Taylor

Sorry for the lack of clarity. The proposal Stolee is pushing back on above is to have a helper in test-lib-functions.sh that tells a test whether an event it cares about happened in traces (in this example, invocation of "git gc").

I consider it important because if we are getting into the habit of having our tests assume the current exact byte-for-byte trace JSON output, then that is an assumption that is going to be painful to unravel. Factoring out a helper would also make the test easier to read, but that is less important than "an ounce of prevention is worth a pound of cure" to me in this example.

Jonathan

Serves me right for reading too quickly ;).

I still only partially buy into this, though. I had to deal with some rather fragile grepping through trace2 JSON in t4216 (? or something, the log-bloom tests) recently, and found it a little fragile, but not overly so.

I'd rather just move forward, but I do feel strongly that we discuss these matters on the list.

Thanks,
Taylor

This means we're promising to never change the JSON serialization, or that we're willing to pay with time in the future to update all tests that assume the current JSON serialization.

I do not want to make that promise. I've lived through the same work of updating tests that assumed particular sha1s and having to change them to be more generic and do not want to have to do that again.

If you're saying that you are willing to do that work when the time comes, then that counts for a lot. If you're saying that we should not change the JSON serialization, then it makes me worry that we made a mistake in choosing JSON as a format, since using a well defined format with an ecosystem of serialization + deserialization libraries was the whole point.

Jonathan

I've discussed this a bit more at [1]: I do think this is output that we cannot promise to keep stable. Can you say a little more about why extracting a helper would be problematic? For example, would doing that in a separate patch at the end of the series be workable for you?

More generally, one theme I'm sensing in the responses to the code review comments I sent is that the series may have gotten too long to be able to easily amend the beginning of it. To that end, I'm happy to make a reroll or make this easier in any other way I can. This also suggests to me that it may make sense to split off an early part of the series and get that into a shape you're comfortable with before rebasing the rest of the series on top as a separate topic (a tried and true method, used recently for the "git bugreport" command, for example).

Thoughts?

Thanks,
Jonathan

[1] https://lore.kernel.org/git/[email protected]/

Well said.

The benefit of JSON is that it is simple to be _additive_. We can always jam more data into the format.

What I'm doing is making the following assumptions:

1. There will always be an event line corresponding to launching a subcommand.

2. This line will contain the list of arguments used for that subcommand in a JSON array.

3. We don't add extra whitespace in that JSON array.

The first two items are important promises made by the trace2 event output. By depending on them, I'm only adding more places where we expect the JSON *schema* to remain consistent. Rather, I only require that the JSON schema does not _remove_ that data which is expected by tools that are reading and reporting that data.

Only the third item is something that adds an additional restriction on the JSON format. But also: why would we ever add extraneous whitespace characters to the output intended for computer parsing? This concern raises a giant "YAGNI" warning in my head.

What is seems like you are asking instead is for me to create a tool in the test suite that parses each JSON line, extracts a specific member from that JSON object, reconstructs a command-line invocation from the JSON array, and reports whether that process worked for any line in the event output.

That seems overly complicated and puts even more requirements on the JSON schema than I have placed in my current implementation.

If this is to truly be a hard requirement for these tests to move forward, then I would rather do an alternate approach that would be a better test mechanism than scraping trace data: mock the subcommands.

I would create a new directory, "t/mock", and create a new 'git' executable there. By setting --exec-path in our test commands, we would launch this mocked version of Git instead of our built version. The mock could report each set of command-line arguments to a known file. With some care, we could even provide an instructions file to tell it which error code to return or whether it should write data to stdout. This would allow testing things like the "rewrite after verify fails" behavior already in the maintenance builtin.

If I'm to spend time engineering something more complicated just to check "did this subcommand run with these arguments?" then I'd rather do it this way.

Thanks,
-Stolee

No, that isn't what I'm asking.

I'm asking for this patch series to take the existing "grep" lines and put them in a function in test-lib-functions.sh, so that we can change them in one place when the trace emitter changes.

[...]

Yes, from my point of view it really is.

But that "is this truly a hard requirement?" comes up tells me I have not done a good job of communicating in this review. A review is about participants in the project working together to improve a patch, not people making demands at each other.

[...]

I don't see why this is more complicated than what is in patch 1. In fact, I think it would be a little more simple.

Jonathan

Thanks for clarifying, clearing up my misinterpretation of your request. I'm coming around to this idea.

It's also about a balance. A patch author and a reviewer can have disagreements, and it is important to discover exactly how hard each is holding to their opinion.

My interpretation (parsing JSON) _was_ more complicated, hence my reservations. The helper still requires non-trivial script-fu (for someone like me who is bad at scripting languages) but centralizing the grep has value.

Here is my attempt so far:

trace2_subcommand_data () {
	command="$1" &&
	commas=$(echo $command | sed s/\ /\",\"/g) &&
	printf "[\"$commas\"]"
}

test_subcommand_called () {
	data=$(trace2_subcommand_data $1) &&
	grep $data $2
}

test_subcommand_not_called () {
! test_subcommand_called $1 $2
}

with callers looking like

	test_subcommand_called "gc" run-no-auto.txt &&
	test_subcommand_not_called "gc --auto" run-auto.txt &&
	test_subcommand_called "gc --quiet" run-quiet.txt

Thanks,
-Stolee

[...]

I like it. Some tweaks:
- can simplify by using 'printf' instead of sed
- passing the trace in stdin
- following the convention that helpers like test_i18ngrep use of accepting ! as a parameter to negate the exit code

That would make (untested)

# Check that the given command was invoked as part of the

# trace2-format trace on stdin.
#
#	test_subcommand_called [!] <command> <args>... < <trace>
#
# For example, to look for an invocation of "git upload-pack
# /path/to/repo"
#
#	GIT_TRACE2_EVENT=event.log git fetch ... &&
#	test_subcommand_called git upload-pack "$PATH" <event.log
#
# If the first parameter passed is !, this instead checks that
# the given command was not called.
#
test_subcommand_called () {
	local negate=
	if test "$1" = "!"
	then
		negate=t
		shift
	fi

	local expr=$(printf '"%s",' "$@")
	expr="${expr%,}"

	if test -n "$negate"
	then
		! grep "\[$expr\]"
	else
		grep "\[$expr\]"
	fi
}

This works! Thanks. With the ! usage, I think a rename to 'test_subcommand' makes sense. Further, I was somehow wrong about not including "git" in the beginning, so here is my example use of this method;

	test_subcommand git gc <run-no-auto.txt &&
	test_subcommand ! git gc --auto <run-auto.txt &&
	test_subcommand git gc --quiet <run-quiet.txt

Thanks,
-Stolee

Revisiting the first part of this thread, as it got lost in the discussion about trace2 and JSON parsing...

In this patch, the documentation says

Run tasks to optimize Git repository data

the plural is a bit incorrect at this point, but not a huge deal.

The run subcommand says "Run one or more maintenance tasks" which again is a bit misleading since "or more" is not technically possible. This becomes a bit obvious when we see that the task list only contains a 'gc' task.

So no, the documentation in this patch doesn't say "Run the 'gc' task" because that could be gathered from context _and_ it will be incorrect by the time this patch is integrated as part of the series.

So I struggle with finding a way to improve the documentation in this patch to satisfy your concerns. Any change made here will just be un-done by a later patch in the same series.

I'd rather have something vague now and improve the documentation incrementally along with the implementation.

Another point you made earlier, but seems to be dropped in this email:
you wanted the return code documented. This also seems like wasteful text, because the best I can think of is

This command will return 0 unless a maintenance task failed in an unrecoverable way, or other typical failure cases such as incorrect usage or the command being run outside of a Git repository.

Please recommend something better, because that description suffers from being too generic/obvious when talking about the maintenance task and being an obvious repetition of Git's existing error code behavior.

Perhaps this would be easier if there was an established pattern of documenting the return code. So, I went digging.

The examples I could find are:

1. 'git cat-file -e'
2. 'git check-ref-format'
3. 'git commit-graph --object-dir=<dir>'
4. 'git cvsimport'
5. 'git fast-import'
6. 'git filter-branch'
7. 'git grep --quiet'
8. 'git gui citool'
9. 'git ls-remote --exit-code'
10. 'git merge-base --is-ancestor'
11. 'git push --recurse-submodules'
12. 'git reflog exists'
13. 'git rev-parse --parseopt'
14. 'git update-index --test-untracked-cache'

Most of these are for specific options, and the exit code is documented with relation to how that specific option may lead to a non-zero exit code (1, 3, 7, 8, 9, 10, 11, 12, 13, 14).

The one that is most clearly part of the main description is for the plumbing command 'git check-ref-format':

Checks if a given 'refname' is acceptable, and exits with a non-zero status if it is not.

This seems appropriate for something intended for use as a helper in script, with a very simple behavior. The exit code is the only way it provides any feedback to the caller! This use of the exit code as the only output is used by some other cases (1, 7, 9, 10, 11, 12, 13, 14).

Of course, we also have examples like this in git-cvsimport.txt:

Otherwise, success is indicated the Unix way, i.e. by simply exiting with a zero exit status.

This phrase should be _assumed_ as part of Git itself, instead of written in the documentation for specific commands.

The documentation for 'git fast-import' is more useful here:

If fast-import is supplied invalid input it will terminate with a non-zero exit status and create a crash report...

But, you can see that the exit code is not the focus, but the crash report _is_ important to document.

'git filter-branch' has a special exit code for the case that no commits were found to update.

'git gui citool' returns an error code if the window is closed before the command can complete. This is also not a core feature, so is not a good example.

My only conclusion from this investigation is that the exit code is documented when it is useful, but otherwise the standard Unix pattern is assumed and not worth documenting.

If this is something we want to change, then I don't think it valuable to have that discussion here. Instead, let's start that discussion elsewhere with a series that updates existing docs to catch up to a new standard of documenting the exit code.

[1] https://git-scm.com/docs/git-cat-file#Documentation/git-cat-file.txt--e
[2] https://git-scm.com/docs/git-check-ref-format#_description
[3] https://git-scm.com/docs/git-commit-graph#Documentation/git-commit-graph.txt---object-dir
[4] https://git-scm.com/docs/git-cvsimport#_output
[5] https://git-scm.com/docs/git-fast-import#_crash_reports
[6] https://git-scm.com/docs/git-filter-branch#_exit_status
[7] https://git-scm.com/docs/git-grep#Documentation/git-grep.txt---quiet
[8] https://git-scm.com/docs/git-gui#Documentation/git-gui.txt-codegitguicitoolcode
[9] https://git-scm.com/docs/git-ls-remote#Documentation/git-ls-remote.txt---exit-code
[10] https://git-scm.com/docs/git-merge-base#Documentation/git-merge-base.txt---is-ancestor
[11] https://git-scm.com/docs/git-push#Documentation/git-push.txt---recurse-submodulescheckon-demandonlyno
[12] https://git-scm.com/docs/git-reflog
[13] https://git-scm.com/docs/git-rev-parse#_parseopt
[14] https://git-scm.com/docs/git-update-index#Documentation/git-update-index.txt---test-untracked-cache

You're right. Global is not required. The struct needs to be 'static' or initialized for compilation.

While trying to get rid of globals based on your recommendation, I see that the callback functions in the option-parsing _do_ require global state. This is used in "maintenance: add --task option" with the global tasks array, but it also uses a global count of "how many tasks have been selected" currently in the struct. Two ways to get around that issue include (1) making a global int, or (2) looping through the task list to count how many have been selected so far.

Neither of these solutions is clean. (In the diff below, I chose option 2.)

In my opinion, using a global struct would be better than the existing pattern of a bunch of global variables, but I get your point about precedence. More on this below.

Using a "singleton" like the opts struct here, the code is definitely much simpler to read. There is only ever one instance, because the struct definition comes along with the instance declaration.

By making it a local instance, we then need to pass around the struct as a parameter to all of the static methods in the builtin, even though there is only ever one instance of this struct per process.

The two points above are the ones that led me to use the global instance in the first place.

To get back to your point about being idiomatic, these builtins use a struct to group options, then pass it around as a parameter:

* cat-file (struct batch_options)
* checkout (struct checkout_opts)
* rebase (struct rebase_options)
* tag (struct create_tag_options)
* worktree (struct add_opts)

Unfortunately, there _is_ an example of the behavior I was following in this series: my own example in the sparse-checkout builtin. So, I'm just repeating my own mistakes that were not challenged before.

I updated my v4-in-progress branch with that change, and the tip-to-tip diff is below. It required some non-trivial care around the --task=<task> callback (which can't see the local options struct) and other cases.

Thanks,
-Stolee

---

index 1f0c519d14..3e473a1293 100644
@@ -709,11 +709,10 @@ static const char * const builtin_maintenance_usage[] = {
 	NULL
 };
 
-static struct maintenance_opts {
+struct maintenance_opts {
 	int auto_flag;
 	int quiet;
-	int tasks_selected;
-} opts;
+};
 
 /* Remember to update object flag allocation in object.h */
 #define PARENT1		(1u<<16)

@@ -789,7 +788,7 @@ static int should_write_commit_graph(void)
 	return result;
 }
 
-static int run_write_commit_graph(void)
+static int run_write_commit_graph(struct maintenance_opts *opts)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
 

@@ -797,13 +796,13 @@ static int run_write_commit_graph(void)
 	strvec_pushl(&child.args, "commit-graph", "write",
 		     "--split", "--reachable", NULL);
 
-	if (opts.quiet)
+	if (opts->quiet)
 		strvec_push(&child.args, "--no-progress");
 
 	return !!run_command(&child);
 }
 
-static int run_verify_commit_graph(void)
+static int run_verify_commit_graph(struct maintenance_opts *opts)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
 

@@ -811,24 +810,24 @@ static int run_verify_commit_graph(void)
 	strvec_pushl(&child.args, "commit-graph", "verify",
 		     "--shallow", NULL);
 
-	if (opts.quiet)
+	if (opts->quiet)
 		strvec_push(&child.args, "--no-progress");
 
 	return !!run_command(&child);
 }
 
-static int maintenance_task_commit_graph(void)
+static int maintenance_task_commit_graph(struct maintenance_opts *opts)
 {
 	struct repository *r = the_repository;
 	char *chain_path;
 
 	close_object_store(r->objects);
-	if (run_write_commit_graph()) {
+	if (run_write_commit_graph(opts)) {
 		error(_("failed to write commit-graph"));
 		return 1;
 	}
 
-	if (!run_verify_commit_graph())
+	if (!run_verify_commit_graph(opts))
 		return 0;
 
 	warning(_("commit-graph verify caught error, rewriting"));

@@ -840,14 +839,14 @@ static int maintenance_task_commit_graph(void)
 	}
 	free(chain_path);
 
-	if (!run_write_commit_graph())
+	if (!run_write_commit_graph(opts))
 		return 0;
 
 	error(_("failed to rewrite commit-graph"));
 	return 1;
 }
 
-static int fetch_remote(const char *remote)
+static int fetch_remote(const char *remote, struct maintenance_opts *opts)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
 

@@ -855,7 +854,7 @@ static int fetch_remote(const char *remote)
 	strvec_pushl(&child.args, "fetch", remote, "--prune", "--no-tags",
 		     "--no-write-fetch-head", "--refmap=", NULL);
 
-	if (opts.quiet)
+	if (opts->quiet)
 		strvec_push(&child.args, "--quiet");
 
 	strvec_pushf(&child.args, "+refs/heads/*:refs/prefetch/%s/*", remote);

@@ -871,7 +870,7 @@ static int fill_each_remote(struct remote *remote, void *cbdata)
 	return 0;
 }
 
-static int maintenance_task_prefetch(void)
+static int maintenance_task_prefetch(struct maintenance_opts *opts)
 {
 	int result = 0;
 	struct string_list_item *item;

@@ -886,23 +885,23 @@ static int maintenance_task_prefetch(void)
 	for (item = remotes.items;
 	     item && item < remotes.items + remotes.nr;
 	     item++)
-		result |= fetch_remote(item->string);
+		result |= fetch_remote(item->string, opts);
 
 cleanup:
 	string_list_clear(&remotes, 0);
 	return result;
 }
 
-static int maintenance_task_gc(void)
+static int maintenance_task_gc(struct maintenance_opts *opts)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
 
 	child.git_cmd = 1;
 	strvec_push(&child.args, "gc");
 
-	if (opts.auto_flag)
+	if (opts->auto_flag)
 		strvec_push(&child.args, "--auto");
-	if (opts.quiet)
+	if (opts->quiet)
 		strvec_push(&child.args, "--quiet");
 	else
 		strvec_push(&child.args, "--no-quiet");

@@ -911,14 +910,14 @@ static int maintenance_task_gc(void)
 	return run_command(&child);
 }
 
-static int prune_packed(void)
+static int prune_packed(struct maintenance_opts *opts)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
 
 	child.git_cmd = 1;
 	strvec_push(&child.args, "prune-packed");
 
-	if (opts.quiet)
+	if (opts->quiet)
 		strvec_push(&child.args, "--quiet");
 
 	return !!run_command(&child);

@@ -977,7 +976,7 @@ static int write_loose_object_to_stdin(const struct object_id *oid,
 	return ++(d->count) > d->batch_size;
 }
 
-static int pack_loose(void)
+static int pack_loose(struct maintenance_opts *opts)
 {
 	struct repository *r = the_repository;
 	int result = 0;

@@ -996,7 +995,7 @@ static int pack_loose(void)
 	pack_proc.git_cmd = 1;
 
 	strvec_push(&pack_proc.args, "pack-objects");
-	if (opts.quiet)
+	if (opts->quiet)
 		strvec_push(&pack_proc.args, "--quiet");
 	strvec_pushf(&pack_proc.args, "%s/pack/loose", r->objects->odb->path);
 

@@ -1027,9 +1026,9 @@ static int pack_loose(void)
 	return result;
 }
 
-static int maintenance_task_loose_objects(void)
+static int maintenance_task_loose_objects(struct maintenance_opts *opts)
 {
-	return prune_packed() || pack_loose();
+	return prune_packed(opts) || pack_loose(opts);
 }
 
 static int incremental_repack_auto_condition(void)

@@ -1061,14 +1060,14 @@ static int incremental_repack_auto_condition(void)
 	return count >= incremental_repack_auto_limit;
 }
 
-static int multi_pack_index_write(void)
+static int multi_pack_index_write(struct maintenance_opts *opts)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
 
 	child.git_cmd = 1;
 	strvec_pushl(&child.args, "multi-pack-index", "write", NULL);
 
-	if (opts.quiet)
+	if (opts->quiet)
 		strvec_push(&child.args, "--no-progress");
 
 	if (run_command(&child))

@@ -1077,7 +1076,7 @@ static int multi_pack_index_write(void)
 	return 0;
 }
 
-static int rewrite_multi_pack_index(void)
+static int rewrite_multi_pack_index(struct maintenance_opts *opts)
 {
 	struct repository *r = the_repository;
 	char *midx_name = get_midx_filename(r->objects->odb->path);

@@ -1085,17 +1084,18 @@ static int rewrite_multi_pack_index(void)
 	unlink(midx_name);
 	free(midx_name);
 
-	return multi_pack_index_write();
+	return multi_pack_index_write(opts);
 }
 
-static int multi_pack_index_verify(const char *message)
+static int multi_pack_index_verify(struct maintenance_opts *opts,
+				   const char *message)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
 
 	child.git_cmd = 1;
 	strvec_pushl(&child.args, "multi-pack-index", "verify", NULL);
 
-	if (opts.quiet)
+	if (opts->quiet)
 		strvec_push(&child.args, "--no-progress");
 
 	if (run_command(&child)) {

@@ -1106,14 +1106,14 @@ static int multi_pack_index_verify(const char *message)
 	return 0;
 }
 
-static int multi_pack_index_expire(void)
+static int multi_pack_index_expire(struct maintenance_opts *opts)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
 
 	child.git_cmd = 1;
 	strvec_pushl(&child.args, "multi-pack-index", "expire", NULL);
 
-	if (opts.quiet)
+	if (opts->quiet)
 		strvec_push(&child.args, "--no-progress");
 
 	close_object_store(the_repository->objects);

@@ -1164,14 +1164,14 @@ static off_t get_auto_pack_size(void)
 	return result_size;
 }
 
-static int multi_pack_index_repack(void)
+static int multi_pack_index_repack(struct maintenance_opts *opts)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
 
 	child.git_cmd = 1;
 	strvec_pushl(&child.args, "multi-pack-index", "repack", NULL);
 
-	if (opts.quiet)
+	if (opts->quiet)
 		strvec_push(&child.args, "--no-progress");
 
 	strvec_pushf(&child.args, "--batch-size=%"PRIuMAX,

@@ -1185,7 +1185,7 @@ static int multi_pack_index_repack(void)
 	return 0;
 }
 
-static int maintenance_task_incremental_repack(void)
+static int maintenance_task_incremental_repack(struct maintenance_opts *opts)
 {
 	prepare_repo_settings(the_repository);
 	if (!the_repository->settings.core_multi_pack_index) {

@@ -1193,22 +1193,22 @@ static int maintenance_task_incremental_repack(void)
 		return 0;
 	}
 
-	if (multi_pack_index_write())
+	if (multi_pack_index_write(opts))
 		return 1;
-	if (multi_pack_index_verify("after initial write"))
-		return rewrite_multi_pack_index();
-	if (multi_pack_index_expire())
+	if (multi_pack_index_verify(opts, "after initial write"))
+		return rewrite_multi_pack_index(opts);
+	if (multi_pack_index_expire(opts))
 		return 1;
-	if (multi_pack_index_verify("after expire step"))
-		return !!rewrite_multi_pack_index();
-	if (multi_pack_index_repack())
+	if (multi_pack_index_verify(opts, "after expire step"))
+		return !!rewrite_multi_pack_index(opts);
+	if (multi_pack_index_repack(opts))
 		return 1;
-	if (multi_pack_index_verify("after repack step"))
-		return !!rewrite_multi_pack_index();
+	if (multi_pack_index_verify(opts, "after repack step"))
+		return !!rewrite_multi_pack_index(opts);
 	return 0;
 }
 
-typedef int maintenance_task_fn(void);
+typedef int maintenance_task_fn(struct maintenance_opts *opts);
 
 /*
  * An auto condition function returns 1 if the task should run

@@ -1275,9 +1275,9 @@ static int compare_tasks_by_selection(const void *a_, const void *b_)
 	return b->selected_order - a->selected_order;
 }
 
-static int maintenance_run(void)
+static int maintenance_run(struct maintenance_opts *opts)
 {
-	int i;
+	int i, found_selected = 0;
 	int result = 0;
 	struct lock_file lk;
 	struct repository *r = the_repository;

@@ -1291,7 +1291,7 @@ static int maintenance_run(void)
 		 * recursive process stack. Do not report an error in
 		 * that case.
 		 */
-		if (!opts.auto_flag && !opts.quiet)
+		if (!opts->auto_flag && !opts->quiet)
 			error(_("lock file '%s' exists, skipping maintenance"),
 			      lock_path);
 		free(lock_path);

@@ -1299,23 +1299,26 @@ static int maintenance_run(void)
 	}
 	free(lock_path);
 
-	if (opts.tasks_selected)
+	for (i = 0; !found_selected && i < TASK__COUNT; i++)
+		found_selected = tasks[i].selected;
+
+	if (found_selected)
 		QSORT(tasks, TASK__COUNT, compare_tasks_by_selection);
 
 	for (i = 0; i < TASK__COUNT; i++) {
-		if (opts.tasks_selected && !tasks[i].selected)
+		if (found_selected && !tasks[i].selected)
 			continue;
 
-		if (!opts.tasks_selected && !tasks[i].enabled)
+		if (!found_selected && !tasks[i].enabled)
 			continue;
 
-		if (opts.auto_flag &&
+		if (opts->auto_flag &&
 		    (!tasks[i].auto_condition ||
 		     !tasks[i].auto_condition()))
 			continue;
 
 		trace2_region_enter("maintenance", tasks[i].name, r);
-		if (tasks[i].fn()) {
+		if (tasks[i].fn(opts)) {
 			error(_("task '%s' failed"), tasks[i].name);
 			result = 1;
 		}

@@ -1349,17 +1352,15 @@ static void initialize_task_config(void)
 static int task_option_parse(const struct option *opt,
 			     const char *arg, int unset)
 {
-	int i;
+	int i, num_selected = 0;
 	struct maintenance_task *task = NULL;
 
 	BUG_ON_OPT_NEG(unset);
 
-	opts.tasks_selected++;
-
 	for (i = 0; i < TASK__COUNT; i++) {
+		num_selected += tasks[i].selected;
 		if (!strcasecmp(tasks[i].name, arg)) {
 			task = &tasks[i];
-			break;
 		}
 	}
 

@@ -1374,13 +1375,14 @@ static int task_option_parse(const struct option *opt,
 	}
 
 	task->selected = 1;
-	task->selected_order = opts.tasks_selected;
+	task->selected_order = num_selected + 1;
 
 	return 0;
 }
 
 int cmd_maintenance(int argc, const char **argv, const char *prefix)
 {
+	static struct maintenance_opts opts;
 	static struct option builtin_maintenance_options[] = {
 		OPT_BOOL(0, "auto", &opts.auto_flag,
 			 N_("run tasks based on the state of the repository")),

@@ -1408,7 +1410,7 @@ int cmd_maintenance(int argc, const char **argv, const char *prefix)
 
 	if (argc == 1) {
 		if (!strcmp(argv[0], "run"))
-			return maintenance_run();
+			return maintenance_run(&opts);
 	}
 
 	usage_with_options(builtin_maintenance_usage,

Would it be better if we use this C99 feature instead:

	#define TWO_GIGABYTES INT32_MAX

But it seems like it's available everywhere, except (maybe) Windows. (judging from UINT32_MAX's usage)

Danh

Thanks for the recommendation. This appears to work in all of the CI builds, including Windows. I'll use that constant in my next version. Perhaps we will discover a platform that doesn't have it by including it.

-Stolee

Just chiming in on this version to point out that what would have been v4 of this series is now split across two series:

[1] https://lore.kernel.org/git/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/

Thanks,
-Stolee