Maintenance builtin, allowing 'gc --auto' customization

This is a second attempt at redesigning Git's repository maintenance patterns. The first attempt [1] included a way to run jobs in the background using a long-lived process; that idea was rejected and is not included in this series. A future series will use the OS to handle scheduling tasks.

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

As mentioned before, git gc already plays the role of maintaining Git repositories. It has accumulated several smaller pieces in its long history, including:

1. Repacking all reachable objects into one pack-file (and deleting unreachable objects).
2. Packing refs.
3. Expiring reflogs.
4. Clearing rerere logs.
5. Updating the commit-graph file.

While expiring reflogs, clearing rererelogs, and deleting unreachable objects are suitable under the guise of "garbage collection", packing refs and updating the commit-graph file are not as obviously fitting. Further, these operations are "all or nothing" in that they rewrite almost all repository data, which does not perform well at extremely large scales. These operations can also be disruptive to foreground Git commands when git gc --auto triggers during routine use.

This series does not intend to change what git gc does, but instead create new choices for automatic maintenance activities, of which git gc remains the only one enabled by default.

The new maintenance tasks are:

* 'commit-graph' : write and verify a single layer of an incremental commit-graph.
* 'loose-objects' : prune packed loose objects, then create a new pack from a batch of loose objects.
* 'pack-files' : expire redundant packs from the multi-pack-index, then repack using the multi-pack-index's incremental repack strategy.
* 'fetch' : fetch from each remote, storing the refs in 'refs/hidden//'.

These tasks are all disabled by default, but can be enabled with config options or run explicitly using "git maintenance run --task=". There are additional config options to allow customizing the conditions for which the tasks run during the '--auto' option. ('fetch' will never run with the '--auto' option.)

Because 'gc' is implemented as a maintenance task, the most dramatic change
of this series is to convert the 'git gc --auto' calls into 'git maintenance run --auto' calls at the end of some Git commands. By default, the only change is that 'git gc --auto' will be run below an additional 'git maintenance' process.

The 'git maintenance' builtin has a 'run' subcommand so it can be extended later with subcommands that manage background maintenance, such as 'start', 'stop', 'pause', or 'schedule'. These are not the subject of this series, as it is important to focus on the maintenance activities themselves.

An expert user could set up scheduled background maintenance themselves with the current series. I have the following crontab data set up to run maintenance on an hourly basis:

0 * * * * git -C /<path-to-repo> maintenance run --no-quiet >>/<path-to-repo>/.git/maintenance.log

My config includes all tasks except the 'gc' task. The hourly run is over-aggressive, but is sufficient for testing. I'll replace it with daily when I feel satisfied.

Hopefully this direction is seen as a positive one. My goal was to add more options for expert users, along with the flexibility to create background maintenance via the OS in a later series.

OUTLINE
=======

Patches 1-4 remove some references to the_repository in builtin/gc.c before we start depending on code in that builtin.

Patches 5-7 create the 'git maintenance run' builtin and subcommand as a simple shim over 'git gc' and replaces calls to 'git gc --auto' from other commands.

Patches 8-15 create new maintenance tasks. These are the same tasks sent in the previous RFC.

Patches 16-21 create more customization through config and perform other polish items.

FUTURE WORK
===========

* Add 'start', 'stop', and 'schedule' subcommands to initialize the commands run in the background.

* Split the 'gc' builtin into smaller maintenance tasks that are enabled by default, but might have different '--auto' conditions and more config options.

* Replace config like 'gc.writeCommitGraph' and 'fetch.writeCommitGraph' with use of the 'commit-graph' task.

Thanks, -Stolee

Derrick Stolee (21):
gc: use the_repository less often
gc: use repository in too_many_loose_objects()
gc: use repo config
gc: drop the_repository in log location
maintenance: create basic maintenance runner
maintenance: add --quiet option
maintenance: replace run_auto_gc()
maintenance: initialize task array and hashmap
maintenance: add commit-graph task
maintenance: add --task option
maintenance: take a lock on the objects directory
maintenance: add fetch task
maintenance: add loose-objects task
maintenance: add pack-files task
maintenance: auto-size pack-files batch
maintenance: create maintenance.<task>.enabled config
maintenance: use pointers to check --auto
maintenance: add auto condition for commit-graph task
maintenance: create auto condition for loose-objects
maintenance: add pack-files auto condition
midx: use start_delayed_progress()

 .gitignore                           |   1 +
 Documentation/config.txt             |   2 +
 Documentation/config/maintenance.txt |  32 +
 Documentation/fetch-options.txt      |   5 +-
 Documentation/git-clone.txt          |   7 +-
 Documentation/git-maintenance.txt    | 124 ++++
 builtin.h                            |   1 +
 builtin/am.c                         |   2 +-
 builtin/commit.c                     |   2 +-
 builtin/fetch.c                      |   6 +-
 builtin/gc.c                         | 881 +++++++++++++++++++++++++--
 builtin/merge.c                      |   2 +-
 builtin/rebase.c                     |   4 +-
 commit-graph.c                       |   8 +-
 commit-graph.h                       |   1 +
 config.c                             |  24 +-
 config.h                             |   2 +
 git.c                                |   1 +
 midx.c                               |  12 +-
 midx.h                               |   1 +
 object.h                             |   1 +
 run-command.c                        |   7 +-
 run-command.h                        |   2 +-
 t/t5319-multi-pack-index.sh          |  14 +-
 t/t5510-fetch.sh                     |   2 +-
 t/t5514-fetch-multiple.sh            |   2 +-
 t/t7900-maintenance.sh               | 211 +++++++
 27 files changed, 1265 insertions(+), 92 deletions(-)
 create mode 100644 Documentation/config/maintenance.txt
 create mode 100644 Documentation/git-maintenance.txt
 create mode 100755 t/t7900-maintenance.sh

base-commit: 4a0fcf9f760c9774be77f51e1e88a7499b53d2e2
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-671%2Fderrickstolee%2Fmaintenance%2Fgc-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-671/derrickstolee/maintenance/gc-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/671

gitgitgadget

From: Derrick Stolee <[email protected]>

In a later change, we will consume several static methods in builtin/gc.c for another builtin. Before doing so, let's clean up some uses of the_repository. These specifically are centered around accesses to the packed_git list.

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

index 8e0b9cf41b..5c5e0df5bf 100644
@@ -192,12 +192,13 @@ static int too_many_loose_objects(void)
 	return needed;
 }
 
-static struct packed_git *find_base_packs(struct string_list *packs,
+static struct packed_git *find_base_packs(struct repository *r,
+					  struct string_list *packs,
 					  unsigned long limit)
 {
 	struct packed_git *p, *base = NULL;
 
-	for (p = get_all_packs(the_repository); p; p = p->next) {
+	for (p = get_all_packs(r); p; p = p->next) {
 		if (!p->pack_local)
 			continue;
 		if (limit) {

@@ -214,7 +215,7 @@ static struct packed_git *find_base_packs(struct string_list *packs,
 	return base;
 }
 
-static int too_many_packs(void)
+static int too_many_packs(struct repository *r)
 {
 	struct packed_git *p;
 	int cnt;

@@ -222,7 +223,7 @@ static int too_many_packs(void)
 	if (gc_auto_pack_limit <= 0)
 		return 0;
 
-	for (cnt = 0, p = get_all_packs(the_repository); p; p = p->next) {
+	for (cnt = 0, p = get_all_packs(r); p; p = p->next) {
 		if (!p->pack_local)
 			continue;
 		if (p->pack_keep)

@@ -334,7 +335,7 @@ static void add_repack_incremental_option(void)
 	argv_array_push(&repack, "--no-write-bitmap-index");
 }
 
-static int need_to_gc(void)
+static int need_to_gc(struct repository *r)
 {
 	/*
 	 * Setting gc.auto to 0 or negative can disable the

@@ -349,18 +350,18 @@ static int need_to_gc(void)
 	 * we run "repack -A -d -l".  Otherwise we tell the caller
 	 * there is no need.
 	 */
-	if (too_many_packs()) {
+	if (too_many_packs(r)) {
 		struct string_list keep_pack = STRING_LIST_INIT_NODUP;
 
 		if (big_pack_threshold) {
-			find_base_packs(&keep_pack, big_pack_threshold);
+			find_base_packs(r, &keep_pack, big_pack_threshold);
 			if (keep_pack.nr >= gc_auto_pack_limit) {
 				big_pack_threshold = 0;
 				string_list_clear(&keep_pack, 0);
-				find_base_packs(&keep_pack, 0);
+				find_base_packs(r, &keep_pack, 0);
 			}
 		} else {
-			struct packed_git *p = find_base_packs(&keep_pack, 0);
+			struct packed_git *p = find_base_packs(r, &keep_pack, 0);
 			uint64_t mem_have, mem_want;
 
 			mem_have = total_ram();

@@ -523,6 +524,7 @@ static void gc_before_repack(void)
 
 int cmd_gc(int argc, const char **argv, const char *prefix)
 {
+	struct repository *r = the_repository;
 	int aggressive = 0;
 	int auto_gc = 0;
 	int quiet = 0;

@@ -589,7 +591,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		/*
 		 * Auto-gc should be least intrusive as possible.
 		 */
-		if (!need_to_gc())
+		if (!need_to_gc(r))
 			return 0;
 		if (!quiet) {
 			if (detach_auto)

@@ -623,9 +625,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 
 		if (keep_base_pack != -1) {
 			if (keep_base_pack)
-				find_base_packs(&keep_pack, 0);
+				find_base_packs(r, &keep_pack, 0);
 		} else if (big_pack_threshold) {
-			find_base_packs(&keep_pack, big_pack_threshold);
+			find_base_packs(r, &keep_pack, big_pack_threshold);
 		}
 
 		add_repack_all_option(&keep_pack);

@@ -652,7 +654,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	gc_before_repack();
 
 	if (!repository_format_precious_objects) {
-		close_object_store(the_repository->objects);
+		close_object_store(r->objects);
 		if (run_command_v_opt(repack.argv, RUN_GIT_CMD))
 			die(FAILED_RUN, repack.argv[0]);
 

@@ -678,15 +680,15 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		die(FAILED_RUN, rerere.argv[0]);
 
 	report_garbage = report_pack_garbage;
-	reprepare_packed_git(the_repository);
+	reprepare_packed_git(r);
 	if (pack_garbage.nr > 0) {
-		close_object_store(the_repository->objects);
+		close_object_store(r->objects);
 		clean_pack_garbage();
 	}
 
-	prepare_repo_settings(the_repository);
-	if (the_repository->settings.gc_write_commit_graph == 1)
-		write_commit_graph_reachable(the_repository->objects->odb,
+	prepare_repo_settings(r);
+	if (r->settings.gc_write_commit_graph == 1)
+		write_commit_graph_reachable(r->objects->odb,
 					     !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,
 					     NULL);
 

gitgitgadget

From: Derrick Stolee <[email protected]>

The previous change performed a mechanical change to swap the_repository for a struct repository pointer when the use of the_repository was obvious. However, the too_many_loose_objects() method uses git_path() instead of repo_git_path(), which implies a hidden dependence on the_repository.

---
 builtin/gc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 5c5e0df5bf..6d8267cecb 100644
@@ -159,7 +159,7 @@ static void gc_config(void)
 	git_config(git_default_config, NULL);
 }
 
-static int too_many_loose_objects(void)
+static int too_many_loose_objects(struct repository *r)
 {
 	/*
 	 * Quickly check if a "gc" is needed, by estimating how

@@ -174,7 +174,7 @@ static int too_many_loose_objects(void)
 	int needed = 0;
 	const unsigned hexsz_loose = the_hash_algo->hexsz - 2;
 
-	dir = opendir(git_path("objects/17"));
+	dir = opendir(repo_git_path(r, "objects/17"));
 	if (!dir)
 		return 0;
 

@@ -378,7 +378,7 @@ static int need_to_gc(struct repository *r)
 
 		add_repack_all_option(&keep_pack);
 		string_list_clear(&keep_pack, 0);
-	} else if (too_many_loose_objects())
+	} else if (too_many_loose_objects(r))
 		add_repack_incremental_option();
 	else
 		return 0;

@@ -692,7 +692,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 					     !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,
 					     NULL);
 
-	if (auto_gc && too_many_loose_objects())
+	if (auto_gc && too_many_loose_objects(r))
 		warning(_("There are too many unreachable loose objects; "
 			"run 'git prune' to remove them."));
 

gitgitgadget

From: Derrick Stolee <[email protected]>

The gc builtin is working towards using a repository pointer instead of relying on the_repository everywhere. Some of the instances of the_repository are hidden underneath git_config...() methods. Expose them by using repo_config...() instead.

One method did not previously have a repo equivalent:
git_config_get_expiry().

---
 builtin/gc.c | 37 +++++++++++++++++++------------------
 config.c     | 24 +++++++++++++++---------
 config.h     |  2 ++
 3 files changed, 36 insertions(+), 27 deletions(-)

index 6d8267cecb..888b6444d6 100644
@@ -116,12 +116,13 @@ static void process_log_file_on_signal(int signo)
 	raise(signo);
 }
 
-static int gc_config_is_timestamp_never(const char *var)
+static int gc_config_is_timestamp_never(struct repository *r,
+					const char *var)
 {
 	const char *value;
 	timestamp_t expire;
 
-	if (!git_config_get_value(var, &value) && value) {
+	if (!repo_config_get_value(r, var, &value) && value) {
 		if (parse_expiry_date(value, &expire))
 			die(_("failed to parse '%s' value '%s'"), var, value);
 		return expire == 0;

@@ -129,34 +130,34 @@ static int gc_config_is_timestamp_never(const char *var)
 	return 0;
 }
 
-static void gc_config(void)
+static void gc_config(struct repository *r)
 {
 	const char *value;
 
-	if (!git_config_get_value("gc.packrefs", &value)) {
+	if (!repo_config_get_value(r, "gc.packrefs", &value)) {
 		if (value && !strcmp(value, "notbare"))
 			pack_refs = -1;
 		else
 			pack_refs = git_config_bool("gc.packrefs", value);
 	}
 
-	if (gc_config_is_timestamp_never("gc.reflogexpire") &&
-	    gc_config_is_timestamp_never("gc.reflogexpireunreachable"))
+	if (gc_config_is_timestamp_never(r, "gc.reflogexpire") &&
+	    gc_config_is_timestamp_never(r, "gc.reflogexpireunreachable"))
 		prune_reflogs = 0;
 
-	git_config_get_int("gc.aggressivewindow", &aggressive_window);
-	git_config_get_int("gc.aggressivedepth", &aggressive_depth);
-	git_config_get_int("gc.auto", &gc_auto_threshold);
-	git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit);
-	git_config_get_bool("gc.autodetach", &detach_auto);
-	git_config_get_expiry("gc.pruneexpire", &prune_expire);
-	git_config_get_expiry("gc.worktreepruneexpire", &prune_worktrees_expire);
-	git_config_get_expiry("gc.logexpiry", &gc_log_expire);
+	repo_config_get_int(r, "gc.aggressivewindow", &aggressive_window);
+	repo_config_get_int(r, "gc.aggressivedepth", &aggressive_depth);
+	repo_config_get_int(r, "gc.auto", &gc_auto_threshold);
+	repo_config_get_int(r, "gc.autopacklimit", &gc_auto_pack_limit);
+	repo_config_get_bool(r, "gc.autodetach", &detach_auto);
+	repo_config_get_expiry(r, "gc.pruneexpire", &prune_expire);
+	repo_config_get_expiry(r, "gc.worktreepruneexpire", &prune_worktrees_expire);
+	repo_config_get_expiry(r, "gc.logexpiry", &gc_log_expire);
 
-	git_config_get_ulong("gc.bigpackthreshold", &big_pack_threshold);
-	git_config_get_ulong("pack.deltacachesize", &max_delta_cache_size);
+	repo_config_get_ulong(r, "gc.bigpackthreshold", &big_pack_threshold);
+	repo_config_get_ulong(r, "pack.deltacachesize", &max_delta_cache_size);
 
-	git_config(git_default_config, NULL);
+	repo_config(r, git_default_config, NULL);
 }
 
 static int too_many_loose_objects(struct repository *r)

@@ -562,7 +563,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	argv_array_pushl(&rerere, "rerere", "gc", NULL);
 
 	/* default expiry time, overwritten in gc_config */
-	gc_config();
+	gc_config(r);
 	if (parse_expiry_date(gc_log_expire, &gc_log_expire_time))
 		die(_("failed to parse gc.logexpiry value %s"), gc_log_expire);
 

index 8db9c77098..fe83f8b67c 100644
@@ -2211,6 +2211,20 @@ int repo_config_get_pathname(struct repository *repo,
 	return ret;
 }
 
+int repo_config_get_expiry(struct repository *repo,
+			   const char *key, const char **output)
+{
+	int ret = repo_config_get_string_const(repo, key, output);
+	if (ret)
+		return ret;
+	if (strcmp(*output, "now")) {
+		timestamp_t now = approxidate("now");
+		if (approxidate(*output) >= now)
+			git_die_config(key, _("Invalid %s: '%s'"), key, *output);
+	}
+	return ret;
+}
+
 /* Functions used historically to read configuration from 'the_repository' */
 void git_config(config_fn_t fn, void *data)
 {

@@ -2274,15 +2288,7 @@ int git_config_get_pathname(const char *key, const char **dest)
 
 int git_config_get_expiry(const char *key, const char **output)
 {
-	int ret = git_config_get_string_const(key, output);
-	if (ret)
-		return ret;
-	if (strcmp(*output, "now")) {
-		timestamp_t now = approxidate("now");
-		if (approxidate(*output) >= now)
-			git_die_config(key, _("Invalid %s: '%s'"), key, *output);
-	}
-	return ret;
+	return repo_config_get_expiry(the_repository, key, output);
 }
 
 int git_config_get_expiry_in_days(const char *key, timestamp_t *expiry, timestamp_t now)

index 060874488f..085bcf6917 100644
@@ -490,6 +490,8 @@ int repo_config_get_maybe_bool(struct repository *repo,
 			       const char *key, int *dest);
 int repo_config_get_pathname(struct repository *repo,
 			     const char *key, const char **dest);
+int repo_config_get_expiry(struct repository *repo,
+			   const char *key, const char **output);
 
 /**
  * Querying For Specific Variables

gitgitgadget

From: Derrick Stolee <[email protected]>

The report_last_gc_error() method use git_pathdup() which implicitly uses the_repository. Replace this with strbuf_repo_path() to get a path buffer we control that uses a given repository pointer.

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

index 888b6444d6..58a00be5af 100644
@@ -462,28 +462,30 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
  * gc should not proceed due to an error in the last run. Prints a
  * message and returns -1 if an error occurred while reading gc.log
  */
-static int report_last_gc_error(void)
+static int report_last_gc_error(struct repository *r)
 {
 	struct strbuf sb = STRBUF_INIT;
 	int ret = 0;
 	ssize_t len;
 	struct stat st;
-	char *gc_log_path = git_pathdup("gc.log");
+	struct strbuf gc_log_path = STRBUF_INIT;
 
-	if (stat(gc_log_path, &st)) {
+	strbuf_repo_git_path(&gc_log_path, r, "gc.log");
+
+	if (stat(gc_log_path.buf, &st)) {
 		if (errno == ENOENT)
 			goto done;
 
-		ret = error_errno(_("cannot stat '%s'"), gc_log_path);
+		ret = error_errno(_("cannot stat '%s'"), gc_log_path.buf);
 		goto done;
 	}
 
 	if (st.st_mtime < gc_log_expire_time)
 		goto done;
 
-	len = strbuf_read_file(&sb, gc_log_path, 0);
+	len = strbuf_read_file(&sb, gc_log_path.buf, 0);
 	if (len < 0)
-		ret = error_errno(_("cannot read '%s'"), gc_log_path);
+		ret = error_errno(_("cannot read '%s'"), gc_log_path.buf);
 	else if (len > 0) {
 		/*
 		 * A previous gc failed.  Report the error, and don't

@@ -496,12 +498,12 @@ static int report_last_gc_error(void)
 			       "Automatic cleanup will not be performed "
 			       "until the file is removed.\n\n"
 			       "%s"),
-			    gc_log_path, sb.buf);
+			    gc_log_path.buf, sb.buf);
 		ret = 1;
 	}
 	strbuf_release(&sb);
 done:
-	free(gc_log_path);
+	strbuf_release(&gc_log_path);
 	return ret;
 }
 

@@ -602,7 +604,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 			fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
 		}
 		if (detach_auto) {
-			int ret = report_last_gc_error();
+			int ret = report_last_gc_error(r);
 			if (ret < 0)
 				/* an I/O error occurred, already reported */
 				exit(128);

gitgitgadget

Regarding the first 4 patches, it would be great if there was a test like the one in test_repository.c (together with a code coverage report, perhaps) that verifies that all code paths do not use the_repository.

But set aside that test for now - I don't think gc.c fully supports arbitrary repositories. In particular, when running a subprocess, it inherits the environment from the current process. I see that future patches try to resolve this by passing "-C", but that does not work if environment variables like GIT_DIR are set (because the environment variables override the "-C"). Perhaps we need a function that runs a subprocess in a specific repository. I ran into the same problem when attempting to make fetch-pack (which runs index-pack as a subprocess) support arbitrary repositories, but I haven't looked deeply into resolving this yet (and I haven't looked at that problem in a while).

Having said that, I'm fine with these patches being in the set - the only negative is that perhaps a reader would be misled into thinking that GC supports arbitrary repositories.

I agree. I hope that I do not give the impression that GC is now safe for arbitrary repositories. I only thought that this was prudent to do before I start taking new dependencies on the code.

It' probably time for someone to do a round of the_repository cleanups again, and perhaps the RC window is a good time to think about that (for submission after the release).

Thanks,
-Stolee

From: Derrick Stolee <[email protected]>

The 'gc' builtin is our current entrypoint for automatically maintaining a repository. This one tool does many operations, such as repacking the repository, packing refs, and rewriting the commit-graph file. The name implies it performs "garbage collection" which means several different things, and some users may not want to use this operation that rewrites the entire object database.

Create a new 'maintenance' builtin that will become a more general- purpose command. To start, it will only support the 'run' subcommand, but will later expand to add subcommands for scheduling maintenance in the background.

For now, the 'maintenance' builtin is a thin shim over the 'gc' builtin. In fact, the only option is the '--auto' toggle, which is handed directly to the 'gc' builtin. The current change is isolated to this simple operation to prevent more interesting logic from being lost in all of the boilerplate of adding a new builtin.

Use existing builtin/gc.c file because we want to share code between the two builtins. It is possible that we will have 'maintenance' replace the 'gc' builtin entirely at some point, leaving 'git gc' as an alias for some specific arguments to 'git maintenance run'.

---
 .gitignore                        |  1 +
 Documentation/git-maintenance.txt | 57 +++++++++++++++++++++++++++++
 builtin.h                         |  1 +
 builtin/gc.c                      | 61 +++++++++++++++++++++++++++++++
 git.c                             |  1 +
 t/t7900-maintenance.sh            | 22 +++++++++++
 6 files changed, 143 insertions(+)
 create mode 100644 Documentation/git-maintenance.txt
 create mode 100755 t/t7900-maintenance.sh

index ee509a2ad2..a5808fa30d 100644
@@ -90,6 +90,7 @@
 /git-ls-tree
 /git-mailinfo
 /git-mailsplit
+/git-maintenance
 /git-merge
 /git-merge-base
 /git-merge-index

index a5ae15bfe5..17c1c0ce49 100644
@@ -167,6 +167,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix);
 int cmd_ls_remote(int argc, const char **argv, const char *prefix);
 int cmd_mailinfo(int argc, const char **argv, const char *prefix);
 int cmd_mailsplit(int argc, const char **argv, const char *prefix);
+int cmd_maintenance(int argc, const char **argv, const char *prefix);
 int cmd_merge(int argc, const char **argv, const char *prefix);
 int cmd_merge_base(int argc, const char **argv, const char *prefix);
 int cmd_merge_index(int argc, const char **argv, const char *prefix);

index 2f021b97f3..ff56d1df24 100644
@@ -527,6 +527,7 @@ static struct cmd_struct commands[] = {
 	{ "ls-tree", cmd_ls_tree, RUN_SETUP },
 	{ "mailinfo", cmd_mailinfo, RUN_SETUP_GENTLY | NO_PARSEOPT },
 	{ "mailsplit", cmd_mailsplit, NO_PARSEOPT },
+	{ "maintenance", cmd_maintenance, RUN_SETUP_GENTLY | NO_PARSEOPT },
 	{ "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
 	{ "merge-base", cmd_merge_base, RUN_SETUP },
 	{ "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },

new file mode 100755
index 0000000000..d00641c4dd
@@ -0,0 +1,22 @@
+#!/bin/sh
+
+test_description='git maintenance builtin'
+
+GIT_TEST_COMMIT_GRAPH=0
+GIT_TEST_MULTI_PACK_INDEX=0
+
+. ./test-lib.sh
+
+test_expect_success 'help text' '
+	test_must_fail git maintenance -h 2>err &&
+	test_i18ngrep "usage: git maintenance run" err
+'
+
+test_expect_success 'gc [--auto]' '
+	GIT_TRACE2_EVENT="$(pwd)/run-no-auto.txt" git maintenance run &&
+	GIT_TRACE2_EVENT="$(pwd)/run-auto.txt" git maintenance run --auto &&
+	grep ",\"gc\"]" run-no-auto.txt  &&
+	grep ",\"gc\",\"--auto\"]" run-auto.txt
+'
+
+test_done

gitgitgadget

From: Derrick Stolee <[email protected]>

Maintenance activities are commonly used as steps in larger scripts. Providing a '--quiet' option allows those scripts to be less noisy when run on a terminal window. Turn this mode on by default when stderr is not a terminal.

Pipe the option to the 'git gc' child process.

---
 Documentation/git-maintenance.txt | 3 +++
 builtin/gc.c                      | 7 +++++++
 t/t7900-maintenance.sh            | 8 +++++---
 3 files changed, 15 insertions(+), 3 deletions(-)

index 34cd2b4417..089fa4cedc 100644
@@ -52,6 +52,9 @@ OPTIONS
 	in the `gc.auto` config setting, or when the number of pack-files
 	exceeds the `gc.autoPackLimit` config setting.
 
+--quiet::
+	Do not report progress or other information over `stderr`.
+
 GIT
 ---
 Part of the linkgit:git[1] suite

index 07cc5f46ae..3881a99e9d 100644
@@ -712,6 +712,7 @@ static const char * const builtin_maintenance_usage[] = {
 
 struct maintenance_opts {
 	int auto_flag;
+	int quiet;
 } opts;
 
 static int maintenance_task_gc(struct repository *r)

@@ -723,6 +724,8 @@ static int maintenance_task_gc(struct repository *r)
 
 	if (opts.auto_flag)
 		argv_array_pushl(&cmd, "--auto", NULL);
+	if (opts.quiet)
+		argv_array_pushl(&cmd, "--quiet", NULL);
 
 	close_object_store(r->objects);
 	result = run_command_v_opt(cmd.argv, RUN_GIT_CMD);

@@ -743,6 +746,8 @@ int cmd_maintenance(int argc, const char **argv, const char *prefix)
 	static struct option builtin_maintenance_options[] = {
 		OPT_BOOL(0, "auto", &opts.auto_flag,
 			 N_("run tasks based on the state of the repository")),
+		OPT_BOOL(0, "quiet", &opts.quiet,
+			 N_("do not report progress or other information over stderr")),
 		OPT_END()
 	};
 

@@ -752,6 +757,8 @@ int cmd_maintenance(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_maintenance_usage,
 				   builtin_maintenance_options);
 
+	opts.quiet = !isatty(2);
+
 	argc = parse_options(argc, argv, prefix,
 			     builtin_maintenance_options,
 			     builtin_maintenance_usage,

index d00641c4dd..e4e4036e50 100755
@@ -12,11 +12,13 @@ test_expect_success 'help text' '
 	test_i18ngrep "usage: git maintenance run" err
 '
 
-test_expect_success 'gc [--auto]' '
-	GIT_TRACE2_EVENT="$(pwd)/run-no-auto.txt" git maintenance run &&
+test_expect_success 'gc [--auto|--quiet]' '
+	GIT_TRACE2_EVENT="$(pwd)/run-no-auto.txt" git maintenance run --no-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
 '
 
 test_done

gitgitgadget

From: Derrick Stolee <[email protected]>

The run_auto_gc() method is used in several places to trigger a check for repo maintenance after some Git commands, such as 'git commit' or 'git fetch'.

To allow for extra customization of this maintenance activity, replace the 'git gc --auto [--quiet]' call with one to 'git maintenance run --auto [--quiet]'. As we extend the maintenance builtin with other steps, users will be able to select different maintenance activities.

Rename run_auto_gc() to run_auto_maintenance() to be clearer what is happening on this call, and to expose all callers in the current diff.

Since 'git fetch' already allows disabling the 'git gc --auto' subprocess, add an equivalent option with a different name to be more descriptive of the new behavior: '--[no-]maintenance'. Update the documentation to include these options at the same time.

---
 Documentation/fetch-options.txt | 5 +++--
 Documentation/git-clone.txt     | 7 ++++---
 builtin/am.c                    | 2 +-
 builtin/commit.c                | 2 +-
 builtin/fetch.c                 | 6 ++++--
 builtin/merge.c                 | 2 +-
 builtin/rebase.c                | 4 ++--
 run-command.c                   | 7 +++++--
 run-command.h                   | 2 +-
 t/t5510-fetch.sh                | 2 +-
 10 files changed, 23 insertions(+), 16 deletions(-)

index 6e2a160a47..d73224844e 100644
@@ -86,9 +86,10 @@ ifndef::git-pull[]
 	Allow several <repository> and <group> arguments to be
 	specified. No <refspec>s may be specified.
 
+--[no-]maintenance::
 --[no-]auto-gc::
-	Run `git gc --auto` at the end to perform garbage collection
-	if needed. This is enabled by default.
+	Run `git maintenance run --auto` at the end to perform garbage
+	collection if needed. This is enabled by default.
 
 --[no-]write-commit-graph::
 	Write a commit-graph after fetching. This overrides the config

index c898310099..aa25aba7d9 100644
@@ -78,9 +78,10 @@ repository using this option and then delete branches (or use any
 other Git command that makes any existing commit unreferenced) in the
 source repository, some objects may become unreferenced (or dangling).
 These objects may be removed by normal Git operations (such as `git commit`)
-which automatically call `git gc --auto`. (See linkgit:git-gc[1].)
-If these objects are removed and were referenced by the cloned repository,
-then the cloned repository will become corrupt.
+which automatically call `git maintenance run --auto` and `git gc --auto`.
+(See linkgit:git-maintenance[1] and linkgit:git-gc[1].) If these objects
+are removed and were referenced by the cloned repository, then the cloned
+repository will become corrupt.
 +
 Note that running `git repack` without the `--local` option in a repository
 cloned with `--shared` will copy objects from the source repository into a pack

index 69e50de018..ff895125f6 100644
@@ -1795,7 +1795,7 @@ static void am_run(struct am_state *state, int resume)
 	if (!state->rebasing) {
 		am_destroy(state);
 		close_object_store(the_repository->objects);
-		run_auto_gc(state->quiet);
+		run_auto_maintenance(state->quiet);
 	}
 }
 

index d1b7396052..658b158659 100644
@@ -1702,7 +1702,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	git_test_write_commit_graph_or_die();
 
 	repo_rerere(the_repository, 0);
-	run_auto_gc(quiet);
+	run_auto_maintenance(quiet);
 	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
 	if (amend && !no_post_rewrite) {
 		commit_post_rewrite(the_repository, current_head, &oid);

index 82ac4be8a5..49a4d727d4 100644
@@ -196,8 +196,10 @@ static struct option builtin_fetch_options[] = {
 	OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
 			N_("report that we have only objects reachable from this object")),
 	OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
+	OPT_BOOL(0, "maintenance", &enable_auto_gc,
+		 N_("run 'maintenance --auto' after fetching")),
 	OPT_BOOL(0, "auto-gc", &enable_auto_gc,
-		 N_("run 'gc --auto' after fetching")),
+		 N_("run 'maintenance --auto' after fetching")),
 	OPT_BOOL(0, "show-forced-updates", &fetch_show_forced_updates,
 		 N_("check for forced-updates on all updated branches")),
 	OPT_BOOL(0, "write-commit-graph", &fetch_write_commit_graph,

@@ -1882,7 +1884,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	close_object_store(the_repository->objects);
 
 	if (enable_auto_gc)
-		run_auto_gc(verbosity < 0);
+		run_auto_maintenance(verbosity < 0);
 
 	return result;
 }

index 7da707bf55..c068e73037 100644
@@ -457,7 +457,7 @@ static void finish(struct commit *head_commit,
 			 * user should see them.
 			 */
 			close_object_store(the_repository->objects);
-			run_auto_gc(verbosity < 0);
+			run_auto_maintenance(verbosity < 0);
 		}
 	}
 	if (new_head && show_diffstat) {

index 37ba76ac3d..0c4ee98f08 100644
@@ -728,10 +728,10 @@ static int finish_rebase(struct rebase_options *opts)
 	apply_autostash(state_dir_path("autostash", opts));
 	close_object_store(the_repository->objects);
 	/*
-	 * We ignore errors in 'gc --auto', since the
+	 * We ignore errors in 'git maintenance run --auto', since the
 	 * user should see them.
 	 */
-	run_auto_gc(!(opts->flags & (REBASE_NO_QUIET|REBASE_VERBOSE)));
+	run_auto_maintenance(!(opts->flags & (REBASE_NO_QUIET|REBASE_VERBOSE)));
 	if (opts->type == REBASE_MERGE) {
 		struct replay_opts replay = REPLAY_OPTS_INIT;
 

index 9b3a57d1e3..82ad241638 100644
@@ -1865,14 +1865,17 @@ int run_processes_parallel_tr2(int n, get_next_task_fn get_next_task,
 	return result;
 }
 
-int run_auto_gc(int quiet)
+int run_auto_maintenance(int quiet)
 {
 	struct argv_array argv_gc_auto = ARGV_ARRAY_INIT;
 	int status;
 
-	argv_array_pushl(&argv_gc_auto, "gc", "--auto", NULL);
+	argv_array_pushl(&argv_gc_auto, "maintenance", "run", "--auto", NULL);
 	if (quiet)
 		argv_array_push(&argv_gc_auto, "--quiet");
+	else
+		argv_array_push(&argv_gc_auto, "--no-quiet");
+
 	status = run_command_v_opt(argv_gc_auto.argv, RUN_GIT_CMD);
 	argv_array_clear(&argv_gc_auto);
 	return status;

index 191dfcdafe..d9a800e700 100644
@@ -221,7 +221,7 @@ int run_hook_ve(const char *const *env, const char *name, va_list args);
 /*
  * Trigger an auto-gc
  */
-int run_auto_gc(int quiet);
+int run_auto_maintenance(int quiet);
 
 #define RUN_COMMAND_NO_STDIN 1
 #define RUN_GIT_CMD	     2	/*If this is to be git sub-command */

index a66dbe0bde..9850ecde5d 100755
@@ -919,7 +919,7 @@ test_expect_success 'fetching with auto-gc does not lock up' '
 		git config fetch.unpackLimit 1 &&
 		git config gc.autoPackLimit 1 &&
 		git config gc.autoDetach false &&
-		GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 &&
+		GIT_ASK_YESNO="$D/askyesno" git fetch --verbose >fetch.out 2>&1 &&
 		test_i18ngrep "Auto packing the repository" fetch.out &&
 		! grep "Should I try again" fetch.out
 	)

gitgitgadget

From: Derrick Stolee <[email protected]>

In anticipation of implementing multiple maintenance tasks inside the 'maintenance' builtin, use a list and hashmap of structs to describe the work to be done.

The struct maintenance_task stores the name of the task (as given by a future command-line argument) along with a function pointer to its implementation and a boolean for whether the step is enabled.

A list of pointers to these structs are initialized with the full list of implemented tasks along with a default order. For now, this list only contains the "gc" task. This task is also the only task enabled by default.

This list is also inserted into a hashmap. This allows command-line arguments to quickly find the tasks by name, not sensitive to case. To ensure this list and hashmap work well together, the list only contains pointers to the struct information. This will allow a sort on the list while preserving the hashmap data.

---
 builtin/gc.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

index 3881a99e9d..c143bf50df 100644
@@ -705,6 +705,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+#define MAX_NUM_TASKS 1
+
 static const char * const builtin_maintenance_usage[] = {
 	N_("git maintenance run [<options>]"),
 	NULL

@@ -734,9 +736,67 @@ static int maintenance_task_gc(struct repository *r)
 	return result;
 }
 
+typedef int maintenance_task_fn(struct repository *r);
+
+struct maintenance_task {
+	struct hashmap_entry ent;
+	const char *name;
+	maintenance_task_fn *fn;
+	unsigned enabled:1;
+};
+
+static int task_entry_cmp(const void *unused_cmp_data,
+			  const struct hashmap_entry *eptr,
+			  const struct hashmap_entry *entry_or_key,
+			  const void *keydata)
+{
+	const struct maintenance_task *e1, *e2;
+	const char *name = keydata;
+
+	e1 = container_of(eptr, const struct maintenance_task, ent);
+	e2 = container_of(entry_or_key, const struct maintenance_task, ent);
+
+	return strcasecmp(e1->name, name ? name : e2->name);
+}
+
+struct maintenance_task *tasks[MAX_NUM_TASKS];
+int num_tasks;
+struct hashmap task_map;
+
 static int maintenance_run(struct repository *r)
 {
-	return maintenance_task_gc(r);
+	int i;
+	int result = 0;
+
+	for (i = 0; !result && i < num_tasks; i++) {
+		if (!tasks[i]->enabled)
+			continue;
+		result = tasks[i]->fn(r);
+	}
+
+	return result;
+}
+
+static void initialize_tasks(void)
+{
+	int i;
+	num_tasks = 0;
+
+	for (i = 0; i < MAX_NUM_TASKS; i++)
+		tasks[i] = xcalloc(1, sizeof(struct maintenance_task));
+
+	tasks[num_tasks]->name = "gc";
+	tasks[num_tasks]->fn = maintenance_task_gc;
+	tasks[num_tasks]->enabled = 1;
+	num_tasks++;
+
+	hashmap_init(&task_map, task_entry_cmp, NULL, MAX_NUM_TASKS);
+
+	for (i = 0; i < num_tasks; i++) {
+		hashmap_entry_init(&tasks[i]->ent,
+				   strihash(tasks[i]->name));
+		hashmap_add(&task_map, &tasks[i]->ent);
+	}
 }
 
 int cmd_maintenance(int argc, const char **argv, const char *prefix)

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

gitgitgadget

I think having the hashmap is unnecessarily complicated in this case - with the small number of tasks, a list would be fine. But I don't feel strongly about this.

You're probably right that iterating through a list with (hopefully) at most a dozen entries is fast enough that a hashmap is overkill here.

Now is the real test: can I change this patch in v2 without needing to mess with any of the others? The intention here was to make adding tasks as simple as possible, so we shall see. :D

Thanks,
-Stolee

Adding a new element to a list would be simple no matter how the list is represented. But I think the real question is what access pattern we expect. Do we need to look up by name a single one or selected few? Do we need the iteration/enumeration be stable? etc.

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                      | 76 ++++++++++++++++++++++++++++++-
 commit-graph.c                    |  8 ++--
 commit-graph.h                    |  1 +
 t/t7900-maintenance.sh            |  2 +-
 5 files changed, 99 insertions(+), 6 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 fdd1c4fa7c..57278a9ab5 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);
 }

@@ -520,7 +520,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;
 

@@ -1635,7 +1635,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);

@@ -2012,7 +2012,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 e4e4036e50..216ac0b19e 100755
@@ -12,7 +12,7 @@ test_expect_success 'help text' '
 	test_i18ngrep "usage: git maintenance run" err
 '
 
-test_expect_success 'gc [--auto|--quiet]' '
+test_expect_success 'run [--auto|--quiet]' '
 	GIT_TRACE2_EVENT="$(pwd)/run-no-auto.txt" git maintenance run --no-quiet &&
 	GIT_TRACE2_EVENT="$(pwd)/run-auto.txt" git maintenance run --auto &&
 	GIT_TRACE2_EVENT="$(pwd)/run-quiet.txt" git maintenance run --quiet &&

gitgitgadget

Do we not pass GIT_DIR to the subcommand? Or does using "-C" override the GIT_DIR?

Thanks,
-Stolee

We do pass GIT_DIR to the subcommand, and "-C" does not override it. I think this code would work as long as "r" is the_repository, which it would be in the current code. But then the "-C" would be doing nothing useful (it might change to the top of the worktree if we weren't there for some reason, but I don't think "commit-graph write" would care either way).

But if "r" is some other repository, "commit-graph" would continue to operate in the parent process repository because of the inherited GIT_DIR. Using "--git-dir" would solve that, but as a general practice, if you're spawning a sub-process that might be in another repository, you should clear any repo-specific environment variables. The list is in local_repo_env, which you can feed to the "env" or "env_array" parameter of a child_process (see the use in connect.c for an example).

Even in the current scheme where "r" is always the_repository, I suspect this might still be buggy. If we're in a bare repository, presumably r->worktree would be NULL.

-Peff

Ah. I'll investigate this more and work to create a way to run a subcommand in a given repository. Your pointers will help a lot.

Thanks,
-Stolee

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.

---
 Documentation/git-maintenance.txt |  4 ++
 builtin/gc.c                      | 62 ++++++++++++++++++++++++++++++-
 t/t7900-maintenance.sh            | 23 ++++++++++++
 3 files changed, 87 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 216ac0b19e..c09a9eb90b 100755
@@ -21,4 +21,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: 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 | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

index 8f2143862c..e3c634fc3b 100644
@@ -849,6 +849,24 @@ static int maintenance_run(struct repository *r)
 {
 	int i;
 	int result = 0;
+	struct lock_file lk;
+	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, num_tasks, compare_tasks_by_selection);

@@ -863,6 +881,7 @@ static int maintenance_run(struct repository *r)
 		result = tasks[i]->fn(r);
 	}
 
+	rollback_lock_file(&lk);
 	return result;
 }
 

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.

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.

3. By adding a new refspec "+refs/heads/*:refs/hidden/<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/hidden/<remote> refs that no longer appear on the remote.

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 | 12 ++++++
 builtin/gc.c                      | 65 ++++++++++++++++++++++++++++++-
 t/t7900-maintenance.sh            | 24 ++++++++++++
 3 files changed, 100 insertions(+), 1 deletion(-)

index 9204762e21..e0be3f520f 100644
@@ -53,6 +53,18 @@ 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.
 
+fetch::
+	The `fetch` job 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/hidden/<remote>/`. Also, no tags are
+	updated.
++
+This means that foreground fetches are still required to update the
+remote refs, but the users is notified when the branches and tags are
+updated on the remote.
+
 gc::
 	Cleanup unnecessary files and optimize the local repository. "GC"
 	stands for "garbage collection," but this task performs many

index c09a9eb90b..0abfc4a9da 100755
@@ -44,4 +44,28 @@ test_expect_success 'run --task duplicate' '
 	test_i18ngrep "cannot be selected multiple times" err
 '
 
+test_expect_success 'run --task=fetch with no remotes' '
+	git maintenance run --task=fetch 2>err &&
+	test_must_be_empty err
+'
+
+test_expect_success 'fetch 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-fetch.txt" git maintenance run --task=fetch &&
+	grep ",\"fetch\",\"remote1\"" run-fetch.txt &&
+	grep ",\"fetch\",\"remote2\"" run-fetch.txt &&
+	test_path_is_missing .git/refs/remotes &&
+	test_cmp clone1/.git/refs/heads/one .git/refs/hidden/remote1/one &&
+	test_cmp clone2/.git/refs/heads/two .git/refs/hidden/remote2/two &&
+	git log hidden/remote1/one &&
+	git log hidden/remote2/two
+'
+
 test_done

gitgitgadget

Having a ref path where Git can place commit IDs that it needs persisted is useful, not only in this case but in other cases (e.g. when fetching a submodule commit by hash, we might not have a ref name for that commit but want to persist it anyway), so I look forward to having something like this.

The name of this special ref path and its specific nature could be discussed further, but maybe it is sufficient for now to just say that the refs under this special ref path are controlled by Git, and their layout is experimental and subject to change (e.g. future versions of Git could just erase the entire path and rewrite the refs its own way).

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                      | 107 +++++++++++++++++++++++++++++-
 t/t7900-maintenance.sh            |  35 ++++++++++
 3 files changed, 152 insertions(+), 1 deletion(-)

index e0be3f520f..bf792d446f 100644
@@ -73,6 +73,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 0abfc4a9da..8cb33624c6 100755
@@ -68,4 +68,39 @@ test_expect_success 'fetch multiple remotes' '
 	git log hidden/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 &&
+	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 &&
+
+	# 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]>

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 'pack-files' job 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 approximats 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 pack-files job 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                      | 124 +++++++++++++++++++++++++++++-
 midx.c                            |   2 +-
 midx.h                            |   1 +
 t/t7900-maintenance.sh            |  37 +++++++++
 5 files changed, 177 insertions(+), 2 deletions(-)

index bf792d446f..945fda368b 100644
@@ -84,6 +84,21 @@ loose-objects::
 	thousand objects to prevent the job from taking too long on a
 	repository with many loose objects.
 
+pack-files::
+	The `pack-files` job incrementally 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 6d1584ca51..57a8a00082 100644
@@ -36,7 +36,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 8cb33624c6..a6be8456aa 100755
@@ -103,4 +103,41 @@ test_expect_success 'loose-objects task' '
 	test_cmp packs-between packs-after
 '
 
+test_expect_success 'pack-files 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=pack-files &&
+	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=pack-files &&
+	ls .git/objects/pack/*.pack >packs-after &&
+	test_line_count = 1 packs-after
+'
+
 test_done

gitgitgadget

From: Derrick Stolee <[email protected]>

When repacking during the 'pack-files' job, 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 sustaintable for a large repository. The amount of work required is also likely to use too many system resources for a background job.

Update the 'pack-files' maintenance 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           | 47 +++++++++++++++++++++++++++++++++++++++++-
 t/t7900-maintenance.sh |  5 +++--
 2 files changed, 49 insertions(+), 3 deletions(-)

index 259b0475c0..582219156a 100644
@@ -1032,20 +1032,65 @@ static int multi_pack_index_expire(struct repository *r)
 	return result;
 }
 
+#define TWO_GIGABYTES (2147483647)
+#define UNSET_BATCH_SIZE ((unsigned long)-1)
+
+static off_t get_auto_pack_size(struct repository *r)
+{
+	/*
+	 * 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;
+
+	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(struct repository *r)
 {
 	int result;
 	struct argv_array cmd = ARGV_ARRAY_INIT;
+	struct strbuf batch_arg = STRBUF_INIT;
+
 	argv_array_pushl(&cmd, "-C", r->worktree,
 			 "multi-pack-index", "repack", NULL);
 
 	if (opts.quiet)
 		argv_array_push(&cmd, "--no-progress");
 
-	argv_array_push(&cmd, "--batch-size=0");
+	strbuf_addf(&batch_arg, "--batch-size=%"PRIuMAX,
+			    (uintmax_t)get_auto_pack_size(r));
+	argv_array_push(&cmd, batch_arg.buf);
 
 	close_object_store(r->objects);
 	result = run_command_v_opt(cmd.argv, RUN_GIT_CMD);
+	strbuf_release(&batch_arg);
 
 	if (result && multi_pack_index_verify(r)) {
 		warning(_("multi-pack-index verify failed after repack"));

index a6be8456aa..43d32c131b 100755
@@ -134,10 +134,11 @@ test_expect_success 'pack-files 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=pack-files &&
 	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]>

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                         | 15 +++++++++++++--
 t/t7900-maintenance.sh               | 12 ++++++++++++
 5 files changed, 36 insertions(+), 3 deletions(-)
 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 945fda368b..261d2e0ee1 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 582219156a..6ffe2213b4 100644
@@ -1208,9 +1208,10 @@ static int maintenance_run(struct repository *r)
 	return result;
 }
 
-static void initialize_tasks(void)
+static void initialize_tasks(struct repository *r)
 {
 	int i;
+	struct strbuf config_name = STRBUF_INIT;
 	num_tasks = 0;
 
 	for (i = 0; i < MAX_NUM_TASKS; i++)

@@ -1240,10 +1241,20 @@ static void initialize_tasks(void)
 	hashmap_init(&task_map, task_entry_cmp, NULL, MAX_NUM_TASKS);
 
 	for (i = 0; i < num_tasks; i++) {
+		int config_value;
+
 		hashmap_entry_init(&tasks[i]->ent,
 				   strihash(tasks[i]->name));
 		hashmap_add(&task_map, &tasks[i]->ent);
+
+		strbuf_setlen(&config_name, 0);
+		strbuf_addf(&config_name, "maintenance.%s.enabled", tasks[i]->name);
+
+		if (!repo_config_get_bool(r, config_name.buf, &config_value))
+			tasks[i]->enabled = config_value;
 	}
+
+	strbuf_release(&config_name);
 }
 
 static int task_option_parse(const struct option *opt,

@@ -1304,7 +1315,7 @@ int cmd_maintenance(int argc, const char **argv, const char *prefix)
 				   builtin_maintenance_options);
 
 	opts.quiet = !isatty(2);
-	initialize_tasks();
+	initialize_tasks(r);
 
 	argc = parse_options(argc, argv, prefix,
 			     builtin_maintenance_options,

index 43d32c131b..08aa50a724 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              | 15 +++++++++++++++
 t/t5514-fetch-multiple.sh |  2 +-
 t/t7900-maintenance.sh    |  2 +-
 3 files changed, 17 insertions(+), 2 deletions(-)

index 6ffe2213b4..dd24026b41 100644
@@ -1132,10 +1132,18 @@ static int maintenance_task_pack_files(struct repository *r)
 
 typedef int maintenance_task_fn(struct repository *r);
 
+/*
+ * 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(struct repository *r);
+
 struct maintenance_task {
 	struct hashmap_entry ent;
 	const char *name;
 	maintenance_task_fn *fn;
+	maintenance_auto_fn *auto_condition;
 	int task_order;
 	unsigned enabled:1,
 		 selected:1;

@@ -1201,6 +1209,11 @@ static int maintenance_run(struct repository *r)
 		if (!opts.tasks_selected && !tasks[i]->enabled)
 			continue;
 
+		if (opts.auto_flag &&
+		    (!tasks[i]->auto_condition ||
+		     !tasks[i]->auto_condition(r)))
+			continue;
+
 		result = tasks[i]->fn(r);
 	}
 

@@ -1231,6 +1244,7 @@ static void initialize_tasks(struct repository *r)
 
 	tasks[num_tasks]->name = "gc";
 	tasks[num_tasks]->fn = maintenance_task_gc;
+	tasks[num_tasks]->auto_condition = need_to_gc;
 	tasks[num_tasks]->enabled = 1;
 	num_tasks++;
 

@@ -1315,6 +1329,7 @@ int cmd_maintenance(int argc, const char **argv, const char *prefix)
 				   builtin_maintenance_options);
 
 	opts.quiet = !isatty(2);
+	gc_config(r);
 	initialize_tasks(r);
 
 	argc = parse_options(argc, argv, prefix,

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 08aa50a724..315bba2447 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]>

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 38dc2d5a6c..4f886495d7 100644
@@ -73,6 +73,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

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 81b076b012..391e1e2121 100644
@@ -960,6 +960,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(struct repository *r)
+{
+	int count = 0;
+
+	repo_config_get_int(r, "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(r->objects->odb->path,
+					     loose_object_count,
+					     NULL, NULL, &count);
+}
+
 static int loose_object_exists(const struct object_id *oid,
 			       const char *path,
 			       void *data)

@@ -1311,6 +1340,7 @@ static void initialize_tasks(struct repository *r)
 
 	tasks[num_tasks]->name = "loose-objects";
 	tasks[num_tasks]->fn = maintenance_task_loose_objects;
+	tasks[num_tasks]->auto_condition = loose_object_auto_condition;
 	num_tasks++;
 
 	tasks[num_tasks]->name = "pack-files";

index 315bba2447..a55c36d249 100755
@@ -115,6 +115,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 'pack-files task' '
 	packDir=.git/objects/pack &&
 	for i in $(test_seq 1 5)

gitgitgadget

From: Derrick Stolee <[email protected]>

The pack-files 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.pack-files.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..77b255318c 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.pack-files.auto::
+	This integer config option controls how often the `pack-files` task
+	should be run as part of `git maintenance run --auto`. If zero, then
+	the `pack-files` 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.pack-files.auto`. The default value is 10.

index 391e1e2121..c3531561c2 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"
 

@@ -1072,6 +1073,35 @@ static int maintenance_task_loose_objects(struct repository *r)
 	return prune_packed(r) || pack_loose(r);
 }
 
+static int pack_files_auto_condition(struct repository *r)
+{
+	struct packed_git *p;
+	int enabled;
+	int pack_files_auto_limit = 10;
+	int count = 0;
+
+	if (repo_config_get_bool(r, "core.multiPackIndex", &enabled) ||
+	    !enabled)
+		return 0;
+
+	repo_config_get_int(r, "maintenance.pack-files.auto",
+			    &pack_files_auto_limit);
+
+	if (!pack_files_auto_limit)
+		return 0;
+	if (pack_files_auto_limit < 0)
+		return 1;
+
+	for (p = get_packed_git(r);
+	     count < pack_files_auto_limit && p;
+	     p = p->next) {
+		if (!p->multi_pack_index)
+			count++;
+	}
+
+	return count >= pack_files_auto_limit;
+}
+
 static int multi_pack_index_write(struct repository *r)
 {
 	int result;

@@ -1345,6 +1375,7 @@ static void initialize_tasks(struct repository *r)
 
 	tasks[num_tasks]->name = "pack-files";
 	tasks[num_tasks]->fn = maintenance_task_pack_files;
+	tasks[num_tasks]->auto_condition = pack_files_auto_condition;
 	num_tasks++;
 
 	tasks[num_tasks]->name = "gc";

index a55c36d249..1714d11bd9 100755
@@ -178,4 +178,34 @@ test_expect_success 'pack-files task' '
 	test_line_count = 2 packs-after
 '
 
+test_expect_success 'maintenance.pack-files.auto' '
+	git repack -adk &&
+	git config core.multiPackIndex true &&
+	git multi-pack-index write &&
+	GIT_TRACE2_EVENT=1 git -c maintenance.pack-files.auto=1 maintenance \
+		run --auto --task=pack-files >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.pack-files.auto=2 \
+			maintenance run --auto --task=pack-files &&
+		! 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.pack-files.auto=2 \
+			maintenance run --auto --task=pack-files >out &&
+		grep "\"multi-pack-index\"" trace-B-$i >/dev/null || return 1
+	done
+'
+
 test_done

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 57a8a00082..d4022e4aef 100644
@@ -837,7 +837,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;
 

@@ -974,7 +974,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])

@@ -1109,7 +1109,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))

@@ -1230,7 +1230,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);

@@ -1240,7 +1240,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 7214cab36c..12f41dfc18 100755
@@ -172,12 +172,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
 '
 

@@ -334,17 +334,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
 '
 

@@ -488,7 +488,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
 	)
 '

@@ -496,7 +496,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

One thing I wonder about - now I have to go and make a new crontab (which is easy) or Task Scheduler task (which is a pain) for every repo, right?

Is it infeasible to ask for 'git maintenance' to learn something like '--on /<path-to-repo> --on /<path-to-second-repo>'? Or better yet, some config like "maintenance.targetRepo = /<path-to-repo>"?

For me, I'd prefer to see 'git maintenance run' get bigger and 'git gc --auto' get smaller or disappear. Is there a plan towards that direction, or is that out of scope for 'git maintenance run'? Similar examples I can think of include 'git annotate' and 'git pickaxe'.

Like I mentioned above, for me, I'd rather just see the 'gc' builtin go away :)

Thanks for these examples of prior work. I'll keep them in mind.

My hope is that we can absolutely do that. I didn't want to start that exercise yet, as I don't want to disrupt existing workflows more than I already am.

It is important to recognize that there are already several "tasks" that run inside 'gc' including:

1. Expiring reflogs.
2. Repacking all reachable objects.
3. Deleting unreachable objects.
4. Packing refs.

Before trying to "remove" the gc builtin, we would want these to be represented in the 'git maintenance run' as tasks.

In that direction, I realized after submitting that I should rename the 'pack-files' task in this submission to 'incremental-repack' instead, allowing a later 'full-repack' task to represent the role of that step in the 'gc' task. Some users will prefer one over the other. Perhaps this incremental/full distinction makes it clear that there are trade-offs in both directions.

Thanks,
-Stolee

Sorry that I missed this comment on my first reply.

The intention is that this cron entry will be simpler after I follow up with the "background" part of maintenance. The idea is to use global or system config to register a list of repositories that want background maintenance and have cron execute something like "git maintenance run --all-repos" to span "git -C <repo> maintenance run --scheduled" for all repos in the config.

For now, this manual setup does end up a bit cluttered if you have a lot of repos to maintain.

Thanks,
-Stolee

I think it might be useful to have a general command to run a subcommand in a bunch of repositories. Something like:

git for-each-repo --recurse /path/to/repos git maintenance ...

which would root around in /path/to/repos for any git-dirs and run "git --git-dir=$GIT_DIR maintenance ..." on each of them.

And/or:

git for-each-repo --config maintenance.repos git maintenance ...

which would pull the set of repos from the named config variable instead of looking around the filesystem.

You could use either as a one-liner in the crontab (depending on which is easier with your repo layout).

-Peff

Yes! This! That's a good way to make something generic that solves the problem at hand, but might also have other applications! Most excellent.

The hope is that we can have such a clean layout. I'm particularly fond of the config option because users may want to opt-in to background maintenance only on some repos, even if they put them in a consistent location.

In the _far_ future, we might even want to add a repo to this "maintenance.repos" list during 'git init' and 'git clone' so this is automatic. It then becomes opt-out at that point, which is why I saw the _far, far_ future.

Thanks,
-Stolee

I'm glad I wasn't the only one super geeked when I read this idea. I'd use the heck out of this in my .bashrc too. Sounds awesome. I actually had a short-lived fling last year with a script to summarize my uncommitted changes in all repos at the beginning of every session (dropped because it became one more thing to gloss over) and could have really used this command.

Oh, I like this idea a lot. Then I can do something silly like

  alias reproclone="git clone --no-maintainenance"

and get the benefits on everything else that I plan to be using frequently.

- Emily

I was planning to build a CLI tool that help manage multiple repos maintenance like what was just described here.
My experience using my poor-man-scalar [1] bash script is: For multiple repositories, the process count could get out of control quite quickly and there are probably other issues that I have not thought of / encountered...

There is definitely a need to keep all the repos updated with pre-fetch and updated commit-graph, while staying compact / garbage free. Having this in Git does simplify a lot of daily operations for end users.

This started to remind me of automatic updates in some of the popular OS. Where download/install/cleanup update of multiple software components are managed under a single tool.

I wonder if this is the path git should take in the 'new world' that Junio mentioned. [2]

But I am also super geeked reading this. :)

Regards,
Son Luong.

[1]: https://github.com/sluongng/git-care
[2]: https://lore.kernel.org/git/[email protected]/

Yup. I like the overall direction of this approach to (1) have a single subcommand that helps all the housekeeping tasks, and to (2) make sure existing housekeeping tasks are supported by the new one.

I can understand why it is tempting to start with a new 'main()' under a new subcommand name because we expect to add a lot more tasks, but the name of that subcommand is much less important.

As can be seen in the list you have above, "gc" already does a lot more than garbage collection (just #3 is the "garbage collection" proper), as it has grown by following the same approach.

What's more important is (2) above. While the tool has grown under the same "gc" name, it was easier to arrange---it fell out naturally as a consequence of the development being an enhancement on top of the prior work. Now that we are reimplementing, we need to actively care. As long as we recognize that, I am perfectly happy with the current effort.

For existing callers, "git gc --auto" may want to be left alive, merely as a thin wrapper around "git maintenance --auto", and as long as the latter is done in the same spirit of the former, i.e. perform a lightweight check to see if the repository is so out of shape and then do a minimum cleaning, it would be welcomed by users if it does a lot more than the current "git gc --auto".

Thanks.

It's entirely possible that (after the 'maintenance' builtin stabilizes) that we make 'git gc --auto' become an alias of something like 'git maintenance run --task=gc --auto' (or itemize all of the sub-tasks) so that 'git gc --auto' doesn't change behavior.

That's a big motivation for adding all code into builtin/gc.c so we can access these tasks inside GC without needing to move or copy the code. I'm trying to preserve history as much as possible.

Thanks,
-Stolee

Yes, it is possible, but I doubt it is desirable.

The current users of "gc --auto" do not (and should not) care the details of what tasks are performed. We surely have added more stuff that need maintenance since "gc --auto" was originally written, and after people have started using "gc --auto" in their workflows. For example, I think "gc --auto" predates "rerere gc" and those who had "gc --auto" in their script had a moment when suddenly it started to clean stale entries in the rerere database.

Were they got upset when it happened? Will they get upset when it starts cleaning up stale commit-graph leftover files?

As long as "gc --auto" kept the same spirit of doing a lightweight check to see if the repository is so out of shape to require cleaning and performing a minimum maintenance when it started calling "rerere gc", and as long as "maintenance --auto" does the same, I would think the users would be delighted without complaints.

So, I wouldn't worry too much about what exactly happens with the future versions of "gc --auto". The world has changed, and we have more items in the repository that needs maintenance/cruft removal. The command in the new world should deal with these new stuff, too.

Thanks.

Sounds good to me. The extra context around this helps a lot!

-Stolee

This is a second attempt at redesigning Git's repository maintenance patterns. The first attempt [1] included a way to run jobs in the background using a long-lived process; that idea was rejected and is not included in this series. A future series will use the OS to handle scheduling tasks.

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

As mentioned before, git gc already plays the role of maintaining Git repositories. It has accumulated several smaller pieces in its long history, including:

1. Repacking all reachable objects into one pack-file (and deleting unreachable objects).
2. Packing refs.
3. Expiring reflogs.
4. Clearing rerere logs.
5. Updating the commit-graph file.
6. Pruning worktrees.

While expiring reflogs, clearing rererelogs, and deleting unreachable objects are suitable under the guise of "garbage collection", packing refs and updating the commit-graph file are not as obviously fitting. Further, these operations are "all or nothing" in that they rewrite almost all repository data, which does not perform well at extremely large scales. These operations can also be disruptive to foreground Git commands when git gc --auto triggers during routine use.

This series does not intend to change what git gc does, but instead create new choices for automatic maintenance activities, of which git gc remains the only one enabled by default.

The new maintenance tasks are:

* 'commit-graph' : write and verify a single layer of an incremental commit-graph.
* 'loose-objects' : prune packed loose objects, then create a new pack from a batch of loose objects.
* 'pack-files' : expire redundant packs from the multi-pack-index, then repack using the multi-pack-index's incremental repack strategy.
* 'fetch' : fetch from each remote, storing the refs in 'refs/prefetch//'.

These tasks are all disabled by default, but can be enabled with config options or run explicitly using "git maintenance run --task=". There are additional config options to allow customizing the conditions for which the tasks run during the '--auto' option. ('fetch' will never run with the '--auto' option.)

Because 'gc' is implemented as a maintenance task, the most dramatic change
of this series is to convert the 'git gc --auto' calls into 'git maintenance run --auto' calls at the end of some Git commands. By default, the only change is that 'git gc --auto' will be run below an additional 'git maintenance' process.

The 'git maintenance' builtin has a 'run' subcommand so it can be extended later with subcommands that manage background maintenance, such as 'start', 'stop', 'pause', or 'schedule'. These are not the subject of this series, as it is important to focus on the maintenance activities themselves.

An expert user could set up scheduled background maintenance themselves with the current series. I have the following crontab data set up to run maintenance on an hourly basis:

0 * * * * git -C /<path-to-repo> maintenance run --no-quiet >>/<path-to-repo>/.git/maintenance.log

My config includes all tasks except the 'gc' task. The hourly run is over-aggressive, but is sufficient for testing. I'll replace it with daily when I feel satisfied.

Hopefully this direction is seen as a positive one. My goal was to add more options for expert users, along with the flexibility to create background maintenance via the OS in a later series.

OUTLINE
=======

Patches 1-4 remove some references to the_repository in builtin/gc.c before we start depending on code in that builtin.

Patches 5-7 create the 'git maintenance run' builtin and subcommand as a simple shim over 'git gc' and replaces calls to 'git gc --auto' from other commands.

Patches 8-15 create new maintenance tasks. These are the same tasks sent in the previous RFC.

Patches 16-21 create more customization through config and perform other polish items.

FUTURE WORK
===========

* Add 'start', 'stop', and 'schedule' subcommands to initialize the commands run in the background. You can see my progress towards this goal
here: https://github.com/gitgitgadget/git/pull/680

* Split the 'gc' builtin into smaller maintenance tasks that are enabled by default, but might have different '--auto' conditions and more config options.

* Replace config like 'gc.writeCommitGraph' and 'fetch.writeCommitGraph' with use of the 'commit-graph' task.

* Update the builtin to use struct repository *r properly, especially when calling subcommands.

UPDATES in V2
=============

I'm sending this between v2.28.0-rc2 adn v2.28.0 as the release things have become a bit quiet.

* The biggest disruption to the range-diff is that I removed the premature use of struct repository *r and instead continue to rely on the_repository. This means several patches were dropped that did prep work in builtin/gc.c.

* I dropped the task hashmap and opted for a linear scan. This task list will always be too small to justify the extra complication of the hashmap.

* struct maintenance_opts is properly static now.

* Some tasks are renamed: fetch -> prefetch, pack-files -> incremental-repack.

* With the rename, the prefetch task uses refs/prefetch/ instead of refs/hidden/.

* A trace2 region around the task executions are added.

Thanks, -Stolee

Derrick Stolee (18):
maintenance: create basic maintenance runner
maintenance: add --quiet option
maintenance: replace run_auto_gc()
maintenance: initialize task array
maintenance: add commit-graph task
maintenance: add --task option
maintenance: take a lock on the objects directory
maintenance: add prefetch task
maintenance: add loose-objects task
maintenance: add incremental-repack task
maintenance: auto-size incremental-repack batch
maintenance: create maintenance.<task>.enabled config
maintenance: use pointers to check --auto
maintenance: add auto condition for commit-graph task
maintenance: create auto condition for loose-objects
maintenance: add incremental-repack auto condition
midx: use start_delayed_progress()
maintenance: add trace2 regions for task execution

 .gitignore                           |   1 +
 Documentation/config.txt             |   2 +
 Documentation/config/maintenance.txt |  32 ++
 Documentation/fetch-options.txt      |   5 +-
 Documentation/git-clone.txt          |   7 +-
 Documentation/git-maintenance.txt    | 124 +++++
 builtin.h                            |   1 +
 builtin/am.c                         |   2 +-
 builtin/commit.c                     |   2 +-
 builtin/fetch.c                      |   6 +-
 builtin/gc.c                         | 753 +++++++++++++++++++++++++++
 builtin/merge.c                      |   2 +-
 builtin/rebase.c                     |   4 +-
 commit-graph.c                       |   8 +-
 commit-graph.h                       |   1 +
 git.c                                |   1 +
 midx.c                               |  12 +-
 midx.h                               |   1 +
 object.h                             |   1 +
 run-command.c                        |   7 +-
 run-command.h                        |   2 +-
 t/t5319-multi-pack-index.sh          |  14 +-
 t/t5510-fetch.sh                     |   2 +-
 t/t5514-fetch-multiple.sh            |   2 +-
 t/t7900-maintenance.sh               | 211 ++++++++
 25 files changed, 1169 insertions(+), 34 deletions(-)
 create mode 100644 Documentation/config/maintenance.txt
 create mode 100644 Documentation/git-maintenance.txt
 create mode 100755 t/t7900-maintenance.sh

base-commit: 4a0fcf9f760c9774be77f51e1e88a7499b53d2e2
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-671%2Fderrickstolee%2Fmaintenance%2Fgc-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-671/derrickstolee/maintenance/gc-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/671

Range-diff vs v1:

  1:  85dda7db28 <  -:  ---------- gc: use the_repository less often
  2:  303ad4bdc7 <  -:  ---------- gc: use repository in too_many_loose_objects()
  3:  9dfa9e8f6f <  -:  ---------- gc: use repo config
  4:  e13d351e9f <  -:  ---------- gc: drop the_repository in log location
  5:  5f89e0ec9b !  1:  63ec602a07 maintenance: create basic maintenance runner
     @@ builtin/gc.c: int cmd_gc(int argc, const char **argv, const char *prefix)
      +	NULL
      +};
      +
     -+struct maintenance_opts {
     ++static struct maintenance_opts {
      +	int auto_flag;
      +} opts;
      +
     -+static int maintenance_task_gc(struct repository *r)
     ++static int maintenance_task_gc(void)
      +{
      +	int result;
      +	struct argv_array cmd = ARGV_ARRAY_INIT;
     @@ builtin/gc.c: int cmd_gc(int argc, const char **argv, const char *prefix)
      +	if (opts.auto_flag)
      +		argv_array_pushl(&cmd, "--auto", NULL);
      +
     -+	close_object_store(r->objects);
     ++	close_object_store(the_repository->objects);
      +	result = run_command_v_opt(cmd.argv, RUN_GIT_CMD);
      +	argv_array_clear(&cmd);
      +
      +	return result;
      +}
      +
     -+static int maintenance_run(struct repository *r)
     ++static int maintenance_run(void)
      +{
     -+	return maintenance_task_gc(r);
     ++	return maintenance_task_gc();
      +}
      +
      +int cmd_maintenance(int argc, const char **argv, const char *prefix)
      +{
     -+	struct repository *r = the_repository;
     -+
      +	static struct option builtin_maintenance_options[] = {
      +		OPT_BOOL(0, "auto", &opts.auto_flag,
      +			 N_("run tasks based on the state of the repository")),
     @@ builtin/gc.c: int cmd_gc(int argc, const char **argv, const char *prefix)
      +
      +	if (argc == 1) {
      +		if (!strcmp(argv[0], "run"))
     -+			return maintenance_run(r);
     ++			return maintenance_run();
      +	}
      +
      +	usage_with_options(builtin_maintenance_usage,

6: 018a9331e2 ! 2: 1d37e55cb7 maintenance: add --quiet option
@@ Documentation/git-maintenance.txt: OPTIONS
## builtin/gc.c ##
@@ builtin/gc.c: static const char * const builtin_maintenance_usage[] = {

- struct maintenance_opts {
+ static struct maintenance_opts {
int auto_flag;
+ int quiet;
} opts;

- static int maintenance_task_gc(struct repository *r)
-@@ builtin/gc.c: static int maintenance_task_gc(struct repository *r)
+ static int maintenance_task_gc(void)
+@@ builtin/gc.c: static int maintenance_task_gc(void)

if (opts.auto_flag)
argv_array_pushl(&cmd, "--auto", NULL);
+ if (opts.quiet)
+ argv_array_pushl(&cmd, "--quiet", NULL);

- close_object_store(r->objects);
+ close_object_store(the_repository->objects);
result = run_command_v_opt(cmd.argv, RUN_GIT_CMD);
@@ builtin/gc.c: int cmd_maintenance(int argc, const char **argv, const char *prefix)
static struct option builtin_maintenance_options[] = {

  7:  335a8938c6 =  3:  f164d1a0b4 maintenance: replace run_auto_gc()
  8:  5cdd38afa6 !  4:  8e260bccf1 maintenance: initialize task array and hashmap
     @@ Metadata
      Author: Derrick Stolee <[email protected]>
      
       ## Commit message ##
     -    maintenance: initialize task array and hashmap
     +    maintenance: initialize task array
      
          In anticipation of implementing multiple maintenance tasks inside the
     -    'maintenance' builtin, use a list and hashmap of structs to describe the
     -    work to be done.
     +    'maintenance' builtin, use a list of structs to describe the work to be
     +    done.
      
          The struct maintenance_task stores the name of the task (as given by a
          future command-line argument) along with a function pointer to its
     @@ Commit message
          contains the "gc" task. This task is also the only task enabled by
          default.
      
     -    This list is also inserted into a hashmap. This allows command-line
     -    arguments to quickly find the tasks by name, not sensitive to case. To
     -    ensure this list and hashmap work well together, the list only contains
     -    pointers to the struct information. This will allow a sort on the list
     -    while preserving the hashmap data.
     -
          Signed-off-by: Derrick Stolee <[email protected]>
      
       ## builtin/gc.c ##
     @@ builtin/gc.c: int cmd_gc(int argc, const char **argv, const char *prefix)
       static const char * const builtin_maintenance_usage[] = {
       	N_("git maintenance run [<options>]"),
       	NULL
     -@@ builtin/gc.c: static int maintenance_task_gc(struct repository *r)
     +@@ builtin/gc.c: static int maintenance_task_gc(void)
       	return result;
       }
       
     -+typedef int maintenance_task_fn(struct repository *r);
     ++typedef int maintenance_task_fn(void);
      +
      +struct maintenance_task {
     -+	struct hashmap_entry ent;
      +	const char *name;
      +	maintenance_task_fn *fn;
      +	unsigned enabled:1;
      +};
      +
     -+static int task_entry_cmp(const void *unused_cmp_data,
     -+			  const struct hashmap_entry *eptr,
     -+			  const struct hashmap_entry *entry_or_key,
     -+			  const void *keydata)
     -+{
     -+	const struct maintenance_task *e1, *e2;
     -+	const char *name = keydata;
     -+
     -+	e1 = container_of(eptr, const struct maintenance_task, ent);
     -+	e2 = container_of(entry_or_key, const struct maintenance_task, ent);
     -+
     -+	return strcasecmp(e1->name, name ? name : e2->name);
     -+}
     ++static struct maintenance_task *tasks[MAX_NUM_TASKS];
     ++static int num_tasks;
      +
     -+struct maintenance_task *tasks[MAX_NUM_TASKS];
     -+int num_tasks;
     -+struct hashmap task_map;
     -+
     - static int maintenance_run(struct repository *r)
     + static int maintenance_run(void)
       {
     --	return maintenance_task_gc(r);
     +-	return maintenance_task_gc();
      +	int i;
      +	int result = 0;
      +
      +	for (i = 0; !result && i < num_tasks; i++) {
      +		if (!tasks[i]->enabled)
      +			continue;
     -+		result = tasks[i]->fn(r);
     ++		result = tasks[i]->fn();
      +	}
      +
      +	return result;
     @@ builtin/gc.c: static int maintenance_task_gc(struct repository *r)
      +	tasks[num_tasks]->fn = maintenance_task_gc;
      +	tasks[num_tasks]->enabled = 1;
      +	num_tasks++;
     -+
     -+	hashmap_init(&task_map, task_entry_cmp, NULL, MAX_NUM_TASKS);
     -+
     -+	for (i = 0; i < num_tasks; i++) {
     -+		hashmap_entry_init(&tasks[i]->ent,
     -+				   strihash(tasks[i]->name));
     -+		hashmap_add(&task_map, &tasks[i]->ent);
     -+	}
       }
       
       int cmd_maintenance(int argc, const char **argv, const char *prefix)
  9:  c8fbd14d41 !  5:  04552b1d2e maintenance: add commit-graph task
     @@ builtin/gc.c: int cmd_gc(int argc, const char **argv, const char *prefix)
       
       static const char * const builtin_maintenance_usage[] = {
       	N_("git maintenance run [<options>]"),
     -@@ builtin/gc.c: struct maintenance_opts {
     +@@ builtin/gc.c: static struct maintenance_opts {
       	int quiet;
       } opts;
       
     -+static int run_write_commit_graph(struct repository *r)
     ++static int run_write_commit_graph(void)
      +{
      +	int result;
      +	struct argv_array cmd = ARGV_ARRAY_INIT;
      +
     -+	argv_array_pushl(&cmd, "-C", r->worktree,
     -+			 "commit-graph", "write",
     -+			 "--split", "--reachable",
     -+			 NULL);
     ++	argv_array_pushl(&cmd, "commit-graph", "write",
     ++			 "--split", "--reachable", NULL);
      +
      +	if (opts.quiet)
     -+		argv_array_pushl(&cmd, "--no-progress", NULL);
     ++		argv_array_push(&cmd, "--no-progress");
      +
      +	result = run_command_v_opt(cmd.argv, RUN_GIT_CMD);
      +	argv_array_clear(&cmd);
     @@ builtin/gc.c: struct maintenance_opts {
      +	return result;
      +}
      +
     -+static int run_verify_commit_graph(struct repository *r)
     ++static int run_verify_commit_graph(void)
      +{
      +	int result;
      +	struct argv_array cmd = ARGV_ARRAY_INIT;
      +
     -+	argv_array_pushl(&cmd, "-C", r->worktree,
     -+			 "commit-graph", "verify",
     ++	argv_array_pushl(&cmd, "commit-graph", "verify",
      +			 "--shallow", NULL);
      +
      +	if (opts.quiet)
     -+		argv_array_pushl(&cmd, "--no-progress", NULL);
     ++		argv_array_push(&cmd, "--no-progress");
      +
      +	result = run_command_v_opt(cmd.argv, RUN_GIT_CMD);
      +	argv_array_clear(&cmd);
     @@ builtin/gc.c: struct maintenance_opts {
      +	return result;
      +}
      +
     -+static int maintenance_task_commit_graph(struct repository *r)
     ++static int maintenance_task_commit_graph(void)
      +{
     ++	struct repository *r = the_repository;
      +	char *chain_path;
      +
      +	/* Skip commit-graph when --auto is specified. */
     @@ builtin/gc.c: struct maintenance_opts {
      +		return 0;
      +
      +	close_object_store(r->objects);
     -+	if (run_write_commit_graph(r)) {
     ++	if (run_write_commit_graph()) {
      +		error(_("failed to write commit-graph"));
      +		return 1;
      +	}
      +
     -+	if (!run_verify_commit_graph(r))
     ++	if (!run_verify_commit_graph())
      +		return 0;
      +
      +	warning(_("commit-graph verify caught error, rewriting"));
     @@ builtin/gc.c: struct maintenance_opts {
      +	}
      +	free(chain_path);
      +
     -+	if (!run_write_commit_graph(r))
     ++	if (!run_write_commit_graph())
      +		return 0;
      +
      +	error(_("failed to rewrite commit-graph"));
      +	return 1;
      +}
      +
     - static int maintenance_task_gc(struct repository *r)
     + static int maintenance_task_gc(void)
       {
       	int result;
      @@ builtin/gc.c: static void initialize_tasks(void)
     + 	tasks[num_tasks]->fn = maintenance_task_gc;
       	tasks[num_tasks]->enabled = 1;
       	num_tasks++;
     - 
     ++
      +	tasks[num_tasks]->name = "commit-graph";
      +	tasks[num_tasks]->fn = maintenance_task_commit_graph;
      +	num_tasks++;
     -+
     - 	hashmap_init(&task_map, task_entry_cmp, NULL, MAX_NUM_TASKS);
     + }
       
     - 	for (i = 0; i < num_tasks; i++) {
     + int cmd_maintenance(int argc, const char **argv, const char *prefix)
      
       ## commit-graph.c ##
      @@ commit-graph.c: static char *get_split_graph_filename(struct object_directory *odb,
 10:  c081a3bd29 !  6:  a09b1c1687 maintenance: add --task option
     @@ Documentation/git-maintenance.txt: OPTIONS
      
       ## builtin/gc.c ##
      @@ builtin/gc.c: static const char * const builtin_maintenance_usage[] = {
     - struct maintenance_opts {
     + static struct maintenance_opts {
       	int auto_flag;
       	int quiet;
      +	int tasks_selected;
       } opts;
       
     - static int run_write_commit_graph(struct repository *r)
     -@@ builtin/gc.c: struct maintenance_task {
     - 	struct hashmap_entry ent;
     + static int run_write_commit_graph(void)
     +@@ builtin/gc.c: typedef int maintenance_task_fn(void);
     + struct maintenance_task {
       	const char *name;
       	maintenance_task_fn *fn;
      -	unsigned enabled:1;
     @@ builtin/gc.c: struct maintenance_task {
      +		 selected:1;
       };
       
     - static int task_entry_cmp(const void *unused_cmp_data,
     -@@ builtin/gc.c: struct maintenance_task *tasks[MAX_NUM_TASKS];
     - int num_tasks;
     - struct hashmap task_map;
     + static struct maintenance_task *tasks[MAX_NUM_TASKS];
     + static int num_tasks;
       
      +static int compare_tasks_by_selection(const void *a_, const void *b_)
      +{
     @@ builtin/gc.c: struct maintenance_task *tasks[MAX_NUM_TASKS];
      +	return b->task_order - a->task_order;
      +}
      +
     - static int maintenance_run(struct repository *r)
     + static int maintenance_run(void)
       {
       	int i;
       	int result = 0;
     @@ builtin/gc.c: struct maintenance_task *tasks[MAX_NUM_TASKS];
      +		if (!opts.tasks_selected && !tasks[i]->enabled)
       			continue;
      +
     - 		result = tasks[i]->fn(r);
     + 		result = tasks[i]->fn();
       	}
       
      @@ builtin/gc.c: static void initialize_tasks(void)
     - 	}
     + 	num_tasks++;
       }
       
      +static int task_option_parse(const struct option *opt,
      +			     const char *arg, int unset)
      +{
     -+	struct maintenance_task *task;
     -+	struct maintenance_task key;
     ++	int i;
     ++	struct maintenance_task *task = NULL;
      +
      +	BUG_ON_OPT_NEG(unset);
      +
     @@ builtin/gc.c: static void initialize_tasks(void)
      +
      +	opts.tasks_selected++;
      +
     -+	key.name = arg;
     -+	hashmap_entry_init(&key.ent, strihash(key.name));
     -+
     -+	task = hashmap_get_entry(&task_map, &key, ent, NULL);
     ++	for (i = 0; i < MAX_NUM_TASKS; i++) {
     ++		if (tasks[i] && !strcasecmp(tasks[i]->name, arg)) {
     ++			task = tasks[i];
     ++			break;
     ++		}
     ++	}
      +
      +	if (!task) {
      +		error(_("'%s' is not a valid task"), arg);
     @@ builtin/gc.c: static void initialize_tasks(void)
      +
       int cmd_maintenance(int argc, const char **argv, const char *prefix)
       {
     - 	struct repository *r = the_repository;
     + 	static struct option builtin_maintenance_options[] = {
      @@ builtin/gc.c: int cmd_maintenance(int argc, const char **argv, const char *prefix)
       			 N_("run tasks based on the state of the repository")),
       		OPT_BOOL(0, "quiet", &opts.quiet,
 11:  fc1fb5f3cc !  7:  e9260a9c3f maintenance: take a lock on the objects directory
     @@ Commit message
          Signed-off-by: Derrick Stolee <[email protected]>
      
       ## builtin/gc.c ##
     -@@ builtin/gc.c: static int maintenance_run(struct repository *r)
     +@@ builtin/gc.c: 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) {
     @@ builtin/gc.c: static int maintenance_run(struct repository *r)
       
       	if (opts.tasks_selected)
       		QSORT(tasks, num_tasks, compare_tasks_by_selection);
     -@@ builtin/gc.c: static int maintenance_run(struct repository *r)
     - 		result = tasks[i]->fn(r);
     +@@ builtin/gc.c: static int maintenance_run(void)
     + 		result = tasks[i]->fn();
       	}
       
      +	rollback_lock_file(&lk);
 12:  cbaa5ecc4f !  8:  3165b8916d maintenance: add fetch task
     @@ Metadata
      Author: Derrick Stolee <[email protected]>
      
       ## Commit message ##
     -    maintenance: add fetch task
     +    maintenance: add prefetch task
      
          When working with very large repositories, an incremental 'git fetch'
          command can download a large amount of data. If there are many other
     @@ Commit message
          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
     @@ Commit message
          2. --refmap= removes the configured refspec which usually updates
             refs/remotes/<remote>/* with the refs advertised by the remote.
      
     -    3. By adding a new refspec "+refs/heads/*:refs/hidden/<remote>/*"
     +    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/hidden/<remote> refs that no
     +    4. --prune will delete the refs/prefetch/<remote> refs that no
             longer appear on the remote.
      
          We've been using this step as a critical background job in Scalar
     @@ Documentation/git-maintenance.txt: since it will not expire `.graph` files that
       `commit-graph-chain` file. They will be deleted by a later run based on
       the expiration delay.
       
     -+fetch::
     -+	The `fetch` job updates the object directory with the latest objects
     ++prefetch::
     ++	The `fetch` 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/hidden/<remote>/`. Also, no tags are
     -+	updated.
     ++	remote refs are stored in `refs/prefetch/<remote>/`. Also, tags are
     ++	not updated.
      ++
      +This means that foreground fetches are still required to update the
      +remote refs, but the users is notified when the branches and tags are
     @@ builtin/gc.c: int cmd_gc(int argc, const char **argv, const char *prefix)
       
       static const char * const builtin_maintenance_usage[] = {
       	N_("git maintenance run [<options>]"),
     -@@ builtin/gc.c: static int maintenance_task_commit_graph(struct repository *r)
     +@@ builtin/gc.c: static int maintenance_task_commit_graph(void)
       	return 1;
       }
       
     -+static int fetch_remote(struct repository *r, const char *remote)
     ++static int fetch_remote(const char *remote)
      +{
      +	int result;
      +	struct argv_array cmd = ARGV_ARRAY_INIT;
      +	struct strbuf refmap = STRBUF_INIT;
      +
     -+	argv_array_pushl(&cmd, "-C", r->worktree,
     -+			 "fetch", remote, "--prune",
     ++	argv_array_pushl(&cmd, "fetch", remote, "--prune",
      +			 "--no-tags", "--refmap=", NULL);
      +
     -+	strbuf_addf(&refmap, "+refs/heads/*:refs/hidden/%s/*", remote);
     ++	strbuf_addf(&refmap, "+refs/heads/*:refs/prefetch/%s/*", remote);
      +	argv_array_push(&cmd, refmap.buf);
      +
      +	if (opts.quiet)
     @@ builtin/gc.c: static int maintenance_task_commit_graph(struct repository *r)
      +	return 0;
      +}
      +
     -+static int maintenance_task_fetch(struct repository *r)
     ++static int maintenance_task_prefetch(void)
      +{
      +	int result = 0;
      +	struct string_list_item *item;
     @@ builtin/gc.c: static int maintenance_task_commit_graph(struct repository *r)
      +	for (item = remotes.items;
      +	     item && item < remotes.items + remotes.nr;
      +	     item++)
     -+		fetch_remote(r, item->string);
     ++		fetch_remote(item->string);
      +
      +cleanup:
      +	string_list_clear(&remotes, 0);
      +	return result;
      +}
      +
     - static int maintenance_task_gc(struct repository *r)
     + static int maintenance_task_gc(void)
       {
       	int result;
      @@ builtin/gc.c: static void initialize_tasks(void)
       	for (i = 0; i < MAX_NUM_TASKS; i++)
       		tasks[i] = xcalloc(1, sizeof(struct maintenance_task));
       
     -+	tasks[num_tasks]->name = "fetch";
     -+	tasks[num_tasks]->fn = maintenance_task_fetch;
     ++	tasks[num_tasks]->name = "prefetch";
     ++	tasks[num_tasks]->fn = maintenance_task_prefetch;
      +	num_tasks++;
      +
       	tasks[num_tasks]->name = "gc";
     @@ t/t7900-maintenance.sh: test_expect_success 'run --task duplicate' '
       	test_i18ngrep "cannot be selected multiple times" err
       '
       
     -+test_expect_success 'run --task=fetch with no remotes' '
     -+	git maintenance run --task=fetch 2>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 'fetch multiple remotes' '
     ++test_expect_success 'prefetch multiple remotes' '
      +	git clone . clone1 &&
      +	git clone . clone2 &&
      +	git remote add remote1 "file://$(pwd)/clone1" &&
     @@ t/t7900-maintenance.sh: test_expect_success 'run --task duplicate' '
      +	git -C clone2 switch -c two &&
      +	test_commit -C clone1 one &&
      +	test_commit -C clone2 two &&
     -+	GIT_TRACE2_EVENT="$(pwd)/run-fetch.txt" git maintenance run --task=fetch &&
     -+	grep ",\"fetch\",\"remote1\"" run-fetch.txt &&
     -+	grep ",\"fetch\",\"remote2\"" run-fetch.txt &&
     ++	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/hidden/remote1/one &&
     -+	test_cmp clone2/.git/refs/heads/two .git/refs/hidden/remote2/two &&
     -+	git log hidden/remote1/one &&
     -+	git log hidden/remote2/two
     ++	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
 13:  66a1f662ce !  9:  83648f4865 maintenance: add loose-objects task
     @@ builtin/gc.c: int cmd_gc(int argc, const char **argv, const char *prefix)
       
       static const char * const builtin_maintenance_usage[] = {
       	N_("git maintenance run [<options>]"),
     -@@ builtin/gc.c: static int maintenance_task_gc(struct repository *r)
     +@@ builtin/gc.c: static int maintenance_task_gc(void)
       	return result;
       }
       
     -+
     -+static int prune_packed(struct repository *r)
     ++static int prune_packed(void)
      +{
      +	struct argv_array cmd = ARGV_ARRAY_INIT;
     -+	argv_array_pushl(&cmd, "-C", r->worktree, "prune-packed", NULL);
     ++	argv_array_pushl(&cmd, "prune-packed", NULL);
      +
      +	if (opts.quiet)
      +		argv_array_push(&cmd, "--quiet");
     @@ builtin/gc.c: static int maintenance_task_gc(struct repository *r)
      +	return ++(d->count) > d->batch_size;
      +}
      +
     -+static int pack_loose(struct repository *r)
     ++static int pack_loose(void)
      +{
     ++	struct repository *r = the_repository;
      +	int result = 0;
      +	struct write_loose_object_data data;
      +	struct strbuf prefix = STRBUF_INIT;
     @@ builtin/gc.c: static int maintenance_task_gc(struct repository *r)
      +	strbuf_addstr(&prefix, r->objects->odb->path);
      +	strbuf_addstr(&prefix, "/pack/loose");
      +
     -+	argv_array_pushl(&pack_proc->args, "git", "-C", r->worktree,
     -+			 "pack-objects", NULL);
     ++	argv_array_pushl(&pack_proc->args, "git", "pack-objects", NULL);
      +	if (opts.quiet)
      +		argv_array_push(&pack_proc->args, "--quiet");
      +	argv_array_push(&pack_proc->args, prefix.buf);
     @@ builtin/gc.c: static int maintenance_task_gc(struct repository *r)
      +	return result;
      +}
      +
     -+static int maintenance_task_loose_objects(struct repository *r)
     ++static int maintenance_task_loose_objects(void)
      +{
     -+	return prune_packed(r) || pack_loose(r);
     ++	return prune_packed() || pack_loose();
      +}
      +
     - typedef int maintenance_task_fn(struct repository *r);
     + typedef int maintenance_task_fn(void);
       
       struct maintenance_task {
      @@ builtin/gc.c: static void initialize_tasks(void)
     - 	tasks[num_tasks]->fn = maintenance_task_fetch;
     + 	tasks[num_tasks]->fn = maintenance_task_prefetch;
       	num_tasks++;
       
      +	tasks[num_tasks]->name = "loose-objects";
     @@ builtin/gc.c: static void initialize_tasks(void)
       	tasks[num_tasks]->enabled = 1;
      
       ## t/t7900-maintenance.sh ##
     -@@ t/t7900-maintenance.sh: test_expect_success 'fetch multiple remotes' '
     - 	git log hidden/remote2/two
     +@@ t/t7900-maintenance.sh: test_expect_success 'prefetch multiple remotes' '
     + 	git log prefetch/remote2/two
       '
       
      +test_expect_success 'loose-objects task' '
 14:  f98790024f ! 10:  b6328c2106 maintenance: add pack-files task
     @@ Metadata
      Author: Derrick Stolee <[email protected]>
      
       ## Commit message ##
     -    maintenance: add pack-files task
     +    maintenance: add incremental-repack task
      
          The previous change cleaned up loose objects using the
          'loose-objects' that can be run safely in the background. Add a
     @@ Commit message
          2019-06-10) and 'git multi-pack-index repack' was added in ce1e4a1
          (midx: implement midx_repack(), 2019-06-10).
      
     -    The 'pack-files' job runs the following steps:
     +    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
     @@ Commit message
             intention is that the resulting pack-file will be close in size
             to the provided batch size.
      
     -       The next run of the pack-files job will delete these repacked
     -       pack-files during the 'expire' step.
     +       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
     @@ Documentation/git-maintenance.txt: loose-objects::
       	thousand objects to prevent the job from taking too long on a
       	repository with many loose objects.
       
     -+pack-files::
     -+	The `pack-files` job incrementally repacks the object directory
     ++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
     @@ builtin/gc.c: int cmd_gc(int argc, const char **argv, const char *prefix)
       
       static const char * const builtin_maintenance_usage[] = {
       	N_("git maintenance run [<options>]"),
     -@@ builtin/gc.c: static int maintenance_task_loose_objects(struct repository *r)
     - 	return prune_packed(r) || pack_loose(r);
     +@@ builtin/gc.c: static int maintenance_task_loose_objects(void)
     + 	return prune_packed() || pack_loose();
       }
       
     -+static int multi_pack_index_write(struct repository *r)
     ++static int multi_pack_index_write(void)
      +{
      +	int result;
      +	struct argv_array cmd = ARGV_ARRAY_INIT;
     -+	argv_array_pushl(&cmd, "-C", r->worktree,
     -+			 "multi-pack-index", "write", NULL);
     ++	argv_array_pushl(&cmd, "multi-pack-index", "write", NULL);
      +
      +	if (opts.quiet)
      +		argv_array_push(&cmd, "--no-progress");
     @@ builtin/gc.c: static int maintenance_task_loose_objects(struct repository *r)
      +	return result;
      +}
      +
     -+static int rewrite_multi_pack_index(struct repository *r)
     ++static int rewrite_multi_pack_index(void)
      +{
     ++	struct repository *r = the_repository;
      +	char *midx_name = get_midx_filename(r->objects->odb->path);
      +
      +	unlink(midx_name);
      +	free(midx_name);
      +
     -+	if (multi_pack_index_write(r)) {
     ++	if (multi_pack_index_write()) {
      +		error(_("failed to rewrite multi-pack-index"));
      +		return 1;
      +	}
     @@ builtin/gc.c: static int maintenance_task_loose_objects(struct repository *r)
      +	return 0;
      +}
      +
     -+static int multi_pack_index_verify(struct repository *r)
     ++static int multi_pack_index_verify(void)
      +{
      +	int result;
      +	struct argv_array cmd = ARGV_ARRAY_INIT;
     -+	argv_array_pushl(&cmd, "-C", r->worktree,
     -+			 "multi-pack-index", "verify", NULL);
     ++	argv_array_pushl(&cmd, "multi-pack-index", "verify", NULL);
      +
      +	if (opts.quiet)
      +		argv_array_push(&cmd, "--no-progress");
     @@ builtin/gc.c: static int maintenance_task_loose_objects(struct repository *r)
      +	return result;
      +}
      +
     -+static int multi_pack_index_expire(struct repository *r)
     ++static int multi_pack_index_expire(void)
      +{
      +	int result;
      +	struct argv_array cmd = ARGV_ARRAY_INIT;
     -+	argv_array_pushl(&cmd, "-C", r->worktree,
     -+			 "multi-pack-index", "expire", NULL);
     ++	argv_array_pushl(&cmd, "multi-pack-index", "expire", NULL);
      +
      +	if (opts.quiet)
      +		argv_array_push(&cmd, "--no-progress");
      +
     -+	close_object_store(r->objects);
     ++	close_object_store(the_repository->objects);
      +	result = run_command_v_opt(cmd.argv, RUN_GIT_CMD);
      +	argv_array_clear(&cmd);
      +
      +	return result;
      +}
      +
     -+static int multi_pack_index_repack(struct repository *r)
     ++static int multi_pack_index_repack(void)
      +{
      +	int result;
      +	struct argv_array cmd = ARGV_ARRAY_INIT;
     -+	argv_array_pushl(&cmd, "-C", r->worktree,
     -+			 "multi-pack-index", "repack", NULL);
     ++	argv_array_pushl(&cmd, "multi-pack-index", "repack", NULL);
      +
      +	if (opts.quiet)
      +		argv_array_push(&cmd, "--no-progress");
      +
      +	argv_array_push(&cmd, "--batch-size=0");
      +
     -+	close_object_store(r->objects);
     ++	close_object_store(the_repository->objects);
      +	result = run_command_v_opt(cmd.argv, RUN_GIT_CMD);
      +
     -+	if (result && multi_pack_index_verify(r)) {
     ++	if (result && multi_pack_index_verify()) {
      +		warning(_("multi-pack-index verify failed after repack"));
     -+		result = rewrite_multi_pack_index(r);
     ++		result = rewrite_multi_pack_index();
      +	}
      +
      +	return result;
      +}
      +
     -+static int maintenance_task_pack_files(struct repository *r)
     ++static int maintenance_task_incremental_repack(void)
      +{
     -+	if (multi_pack_index_write(r)) {
     ++	if (multi_pack_index_write()) {
      +		error(_("failed to write multi-pack-index"));
      +		return 1;
      +	}
      +
     -+	if (multi_pack_index_verify(r)) {
     ++	if (multi_pack_index_verify()) {
      +		warning(_("multi-pack-index verify failed after initial write"));
     -+		return rewrite_multi_pack_index(r);
     ++		return rewrite_multi_pack_index();
      +	}
      +
     -+	if (multi_pack_index_expire(r)) {
     ++	if (multi_pack_index_expire()) {
      +		error(_("multi-pack-index expire failed"));
      +		return 1;
      +	}
      +
     -+	if (multi_pack_index_verify(r)) {
     ++	if (multi_pack_index_verify()) {
      +		warning(_("multi-pack-index verify failed after expire"));
     -+		return rewrite_multi_pack_index(r);
     ++		return rewrite_multi_pack_index();
      +	}
      +
     -+	if (multi_pack_index_repack(r)) {
     ++	if (multi_pack_index_repack()) {
      +		error(_("multi-pack-index repack failed"));
      +		return 1;
      +	}
     @@ builtin/gc.c: static int maintenance_task_loose_objects(struct repository *r)
      +	return 0;
      +}
      +
     - typedef int maintenance_task_fn(struct repository *r);
     + typedef int maintenance_task_fn(void);
       
       struct maintenance_task {
      @@ builtin/gc.c: static void initialize_tasks(void)
       	tasks[num_tasks]->fn = maintenance_task_loose_objects;
       	num_tasks++;
       
     -+	tasks[num_tasks]->name = "pack-files";
     -+	tasks[num_tasks]->fn = maintenance_task_pack_files;
     ++	tasks[num_tasks]->name = "incremental-repack";
     ++	tasks[num_tasks]->fn = maintenance_task_incremental_repack;
      +	num_tasks++;
      +
       	tasks[num_tasks]->name = "gc";
     @@ t/t7900-maintenance.sh: test_expect_success 'loose-objects task' '
       	test_cmp packs-between packs-after
       '
       
     -+test_expect_success 'pack-files task' '
     ++test_expect_success 'incremental-repack task' '
      +	packDir=.git/objects/pack &&
      +	for i in $(test_seq 1 5)
      +	do
     @@ t/t7900-maintenance.sh: test_expect_success 'loose-objects task' '
      +
      +	# the job repacks the two into a new pack, but does not
      +	# delete the old ones.
     -+	git maintenance run --task=pack-files &&
     ++	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=pack-files &&
     ++	git maintenance run --task=incremental-repack &&
      +	ls .git/objects/pack/*.pack >packs-after &&
      +	test_line_count = 1 packs-after
      +'

15: 8be89707d2 ! 11: 478c7f1d0b maintenance: auto-size pack-files batch
@@ Metadata
Author: Derrick Stolee <[email protected]>

## Commit message ##
- maintenance: auto-size pack-files batch
+ maintenance: auto-size incremental-repack batch

- When repacking during the 'pack-files' job, 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
- sustaintable for a large repository. The amount of work required is also
- likely to use too many system resources for a background job.
+ 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 sustaintable for a large repository. The amount of work required is
+ also likely to use too many system resources for a background job.

- Update the 'pack-files' maintenance task by dynamically computing a
+ 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
@@ Commit message

## builtin/gc.c ##
-@@ builtin/gc.c: static int multi_pack_index_expire(struct repository *r) +@@ builtin/gc.c: static int multi_pack_index_expire(void)
return result;
}

+#define TWO_GIGABYTES (2147483647)
+#define UNSET_BATCH_SIZE ((unsigned long)-1)
+
-+static off_t get_auto_pack_size(struct repository *r) ++static off_t get_auto_pack_size(void)
+{
+ /*
+ * The "auto" value is special: we optimize for
@@ builtin/gc.c: static int multi_pack_index_expire(struct repository *r)
+ 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) {
@@ builtin/gc.c: static int multi_pack_index_expire(struct repository *r)
+ return result_size;
+}
+
- static int multi_pack_index_repack(struct repository *r)
+ static int multi_pack_index_repack(void)
{
int result;
struct argv_array cmd = ARGV_ARRAY_INIT;
+ struct strbuf batch_arg = STRBUF_INIT;
+
- argv_array_pushl(&cmd, "-C", r->worktree,
- "multi-pack-index", "repack", NULL);
+ argv_array_pushl(&cmd, "multi-pack-index", "repack", NULL);

if (opts.quiet)
argv_array_push(&cmd, "--no-progress");

- argv_array_push(&cmd, "--batch-size=0");
+ strbuf_addf(&batch_arg, "--batch-size=%"PRIuMAX,
-+ (uintmax_t)get_auto_pack_size(r));
++ (uintmax_t)get_auto_pack_size());
+ argv_array_push(&cmd, batch_arg.buf);

- close_object_store(r->objects);
+ close_object_store(the_repository->objects);
result = run_command_v_opt(cmd.argv, RUN_GIT_CMD);
+ strbuf_release(&batch_arg);

- if (result && multi_pack_index_verify(r)) {
+ if (result && multi_pack_index_verify()) {
warning(_("multi-pack-index verify failed after repack"));

## t/t7900-maintenance.sh ##
-@@ t/t7900-maintenance.sh: test_expect_success 'pack-files task' ' +@@ t/t7900-maintenance.sh: 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=pack-files &&
+ 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
16: 1551aec4fd ! 12: a3c64930a0 maintenance: create maintenance.<task>.enabled config

     @@ Documentation/git-maintenance.txt: SUBCOMMANDS
       -----
      
       ## builtin/gc.c ##
     -@@ builtin/gc.c: static int maintenance_run(struct repository *r)
     - 	return result;
     - }
     - 
     --static void initialize_tasks(void)
     -+static void initialize_tasks(struct repository *r)
     +@@ builtin/gc.c: static int maintenance_run(void)
     + static void initialize_tasks(void)
       {
       	int i;
      +	struct strbuf config_name = STRBUF_INIT;
     @@ builtin/gc.c: static int maintenance_run(struct repository *r)
       
       	for (i = 0; i < MAX_NUM_TASKS; i++)
      @@ builtin/gc.c: static void initialize_tasks(void)
     - 	hashmap_init(&task_map, task_entry_cmp, NULL, MAX_NUM_TASKS);
     - 
     - 	for (i = 0; i < num_tasks; i++) {
     -+		int config_value;
     + 	tasks[num_tasks]->name = "commit-graph";
     + 	tasks[num_tasks]->fn = maintenance_task_commit_graph;
     + 	num_tasks++;
      +
     - 		hashmap_entry_init(&tasks[i]->ent,
     - 				   strihash(tasks[i]->name));
     - 		hashmap_add(&task_map, &tasks[i]->ent);
     ++	for (i = 0; i < num_tasks; i++) {
     ++		int config_value;
      +
      +		strbuf_setlen(&config_name, 0);
      +		strbuf_addf(&config_name, "maintenance.%s.enabled", tasks[i]->name);
      +
     -+		if (!repo_config_get_bool(r, config_name.buf, &config_value))
     ++		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,
     -@@ builtin/gc.c: int cmd_maintenance(int argc, const char **argv, const char *prefix)
     - 				   builtin_maintenance_options);
     - 
     - 	opts.quiet = !isatty(2);
     --	initialize_tasks();
     -+	initialize_tasks(r);
     - 
     - 	argc = parse_options(argc, argv, prefix,
     - 			     builtin_maintenance_options,
      
       ## t/t7900-maintenance.sh ##
      @@ t/t7900-maintenance.sh: test_expect_success 'run [--auto|--quiet]' '
 17:  130130b662 ! 13:  dbacc2b76c maintenance: use pointers to check --auto
     @@ Commit message
          Signed-off-by: Derrick Stolee <[email protected]>
      
       ## builtin/gc.c ##
     -@@ builtin/gc.c: static int maintenance_task_pack_files(struct repository *r)
     +@@ builtin/gc.c: static int maintenance_task_incremental_repack(void)
       
     - typedef int maintenance_task_fn(struct repository *r);
     + 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(struct repository *r);
     ++typedef int maintenance_auto_fn(void);
      +
       struct maintenance_task {
     - 	struct hashmap_entry ent;
       	const char *name;
       	maintenance_task_fn *fn;
      +	maintenance_auto_fn *auto_condition;
       	int task_order;
       	unsigned enabled:1,
       		 selected:1;
     -@@ builtin/gc.c: static int maintenance_run(struct repository *r)
     +@@ builtin/gc.c: 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(r)))
     ++		     !tasks[i]->auto_condition()))
      +			continue;
      +
     - 		result = tasks[i]->fn(r);
     + 		result = tasks[i]->fn();
       	}
       
     -@@ builtin/gc.c: static void initialize_tasks(struct repository *r)
     +@@ builtin/gc.c: static void initialize_tasks(void)
       
       	tasks[num_tasks]->name = "gc";
       	tasks[num_tasks]->fn = maintenance_task_gc;
     @@ builtin/gc.c: int cmd_maintenance(int argc, const char **argv, const char *prefi
       				   builtin_maintenance_options);
       
       	opts.quiet = !isatty(2);
     -+	gc_config(r);
     - 	initialize_tasks(r);
     ++	gc_config();
     + 	initialize_tasks();
       
       	argc = parse_options(argc, argv, prefix,

18: 37fa3f6157 ! 14: 9af2309f08 maintenance: add auto condition for commit-graph task

     @@ builtin/gc.c
       
       #define FAILED_RUN "failed to run %s"
       
     -@@ builtin/gc.c: struct maintenance_opts {
     +@@ builtin/gc.c: static struct maintenance_opts {
       	int tasks_selected;
       } opts;
       
     @@ builtin/gc.c: struct maintenance_opts {
      +	return result;
      +}
      +
     -+static int should_write_commit_graph(struct repository *r)
     ++static int should_write_commit_graph(void)
      +{
      +	int result;
      +
     -+	repo_config_get_int(r, "maintenance.commit-graph.auto",
     -+			    &limit_commits_not_in_graph);
     ++	git_config_get_int("maintenance.commit-graph.auto",
     ++			   &limit_commits_not_in_graph);
      +
      +	if (!limit_commits_not_in_graph)
      +		return 0;
     @@ builtin/gc.c: struct maintenance_opts {
      +	return result;
      +}
      +
     - static int run_write_commit_graph(struct repository *r)
     + static int run_write_commit_graph(void)
       {
       	int result;
     -@@ builtin/gc.c: static void initialize_tasks(struct repository *r)
     +@@ builtin/gc.c: static void initialize_tasks(void)
       
       	tasks[num_tasks]->name = "commit-graph";
       	tasks[num_tasks]->fn = maintenance_task_commit_graph;
      +	tasks[num_tasks]->auto_condition = should_write_commit_graph;
       	num_tasks++;
       
     - 	hashmap_init(&task_map, task_entry_cmp, NULL, MAX_NUM_TASKS);
     + 	for (i = 0; i < num_tasks; i++) {
      
       ## object.h ##
      @@ object.h: struct object_array {
 19:  4744fdaae9 ! 15:  42e316ca58 maintenance: create auto condition for loose-objects
     @@ builtin/gc.c: struct write_loose_object_data {
      +	return 0;
      +}
      +
     -+static int loose_object_auto_condition(struct repository *r)
     ++static int loose_object_auto_condition(void)
      +{
      +	int count = 0;
      +
     -+	repo_config_get_int(r, "maintenance.loose-objects.auto",
     -+			    &loose_object_auto_limit);
     ++	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(r->objects->odb->path,
     ++	return for_each_loose_file_in_objdir(the_repository->objects->odb->path,
      +					     loose_object_count,
      +					     NULL, NULL, &count);
      +}
     @@ builtin/gc.c: struct write_loose_object_data {
       static int loose_object_exists(const struct object_id *oid,
       			       const char *path,
       			       void *data)
     -@@ builtin/gc.c: static void initialize_tasks(struct repository *r)
     +@@ builtin/gc.c: static void initialize_tasks(void)
       
       	tasks[num_tasks]->name = "loose-objects";
       	tasks[num_tasks]->fn = maintenance_task_loose_objects;
      +	tasks[num_tasks]->auto_condition = loose_object_auto_condition;
       	num_tasks++;
       
     - 	tasks[num_tasks]->name = "pack-files";
     + 	tasks[num_tasks]->name = "incremental-repack";
      
       ## t/t7900-maintenance.sh ##
      @@ t/t7900-maintenance.sh: test_expect_success 'loose-objects task' '
     @@ t/t7900-maintenance.sh: test_expect_success 'loose-objects task' '
      +	done
      +'
      +
     - test_expect_success 'pack-files task' '
     + test_expect_success 'incremental-repack task' '
       	packDir=.git/objects/pack &&
       	for i in $(test_seq 1 5)
 20:  fbe03b9af9 ! 16:  3d527cb0dd maintenance: add pack-files auto condition
     @@ Metadata
      Author: Derrick Stolee <[email protected]>
      
       ## Commit message ##
     -    maintenance: add pack-files auto condition
     +    maintenance: add incremental-repack auto condition
      
     -    The pack-files 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
     +    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.pack-files.auto' config option specifies how many
     +    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.
     @@ Documentation/config/maintenance.txt: maintenance.loose-objects.auto::
       	loose objects is at least the value of `maintenance.loose-objects.auto`.
       	The default value is 100.
      +
     -+maintenance.pack-files.auto::
     -+	This integer config option controls how often the `pack-files` task
     -+	should be run as part of `git maintenance run --auto`. If zero, then
     -+	the `pack-files` 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.pack-files.auto`. The default value is 10.
     ++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.
      
       ## builtin/gc.c ##
      @@
     @@ builtin/gc.c
       
       #define FAILED_RUN "failed to run %s"
       
     -@@ builtin/gc.c: static int maintenance_task_loose_objects(struct repository *r)
     - 	return prune_packed(r) || pack_loose(r);
     +@@ builtin/gc.c: static int maintenance_task_loose_objects(void)
     + 	return prune_packed() || pack_loose();
       }
       
     -+static int pack_files_auto_condition(struct repository *r)
     ++static int incremental_repack_auto_condition(void)
      +{
      +	struct packed_git *p;
      +	int enabled;
     -+	int pack_files_auto_limit = 10;
     ++	int incremental_repack_auto_limit = 10;
      +	int count = 0;
      +
     -+	if (repo_config_get_bool(r, "core.multiPackIndex", &enabled) ||
     ++	if (git_config_get_bool("core.multiPackIndex", &enabled) ||
      +	    !enabled)
      +		return 0;
      +
     -+	repo_config_get_int(r, "maintenance.pack-files.auto",
     -+			    &pack_files_auto_limit);
     ++	git_config_get_int("maintenance.incremental-repack.auto",
     ++			   &incremental_repack_auto_limit);
      +
     -+	if (!pack_files_auto_limit)
     ++	if (!incremental_repack_auto_limit)
      +		return 0;
     -+	if (pack_files_auto_limit < 0)
     ++	if (incremental_repack_auto_limit < 0)
      +		return 1;
      +
     -+	for (p = get_packed_git(r);
     -+	     count < pack_files_auto_limit && p;
     ++	for (p = get_packed_git(the_repository);
     ++	     count < incremental_repack_auto_limit && p;
      +	     p = p->next) {
      +		if (!p->multi_pack_index)
      +			count++;
      +	}
      +
     -+	return count >= pack_files_auto_limit;
     ++	return count >= incremental_repack_auto_limit;
      +}
      +
     - static int multi_pack_index_write(struct repository *r)
     + static int multi_pack_index_write(void)
       {
       	int result;
     -@@ builtin/gc.c: static void initialize_tasks(struct repository *r)
     +@@ builtin/gc.c: static void initialize_tasks(void)
       
     - 	tasks[num_tasks]->name = "pack-files";
     - 	tasks[num_tasks]->fn = maintenance_task_pack_files;
     -+	tasks[num_tasks]->auto_condition = pack_files_auto_condition;
     + 	tasks[num_tasks]->name = "incremental-repack";
     + 	tasks[num_tasks]->fn = maintenance_task_incremental_repack;
     ++	tasks[num_tasks]->auto_condition = incremental_repack_auto_condition;
       	num_tasks++;
       
       	tasks[num_tasks]->name = "gc";
      
       ## t/t7900-maintenance.sh ##
     -@@ t/t7900-maintenance.sh: test_expect_success 'pack-files task' '
     +@@ t/t7900-maintenance.sh: test_expect_success 'incremental-repack task' '
       	test_line_count = 2 packs-after
       '
       
     -+test_expect_success 'maintenance.pack-files.auto' '
     ++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.pack-files.auto=1 maintenance \
     -+		run --auto --task=pack-files >out &&
     ++	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
     @@ t/t7900-maintenance.sh: test_expect_success 'pack-files task' '
      +		^HEAD~1
      +		EOF
      +		GIT_TRACE2_EVENT=$(pwd)/trace-A-$i git \
     -+			-c maintenance.pack-files.auto=2 \
     -+			maintenance run --auto --task=pack-files &&
     ++			-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 &&
     @@ t/t7900-maintenance.sh: test_expect_success 'pack-files task' '
      +		^HEAD~1
      +		EOF
      +		GIT_TRACE2_EVENT=$(pwd)/trace-B-$i git \
     -+			-c maintenance.pack-files.auto=2 \
     -+			maintenance run --auto --task=pack-files >out &&
     ++			-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
      +'
 21:  f7fdf72e9d = 17:  a0f00f8ab8 midx: use start_delayed_progress()
  -:  ---------- > 18:  f24db7739f maintenance: add trace2 regions for task execution

gitgitgadget

From: Derrick Stolee <[email protected]>

The 'gc' builtin is our current entrypoint for automatically maintaining a repository. This one tool does many operations, such as repacking the repository, packing refs, and rewriting the commit-graph file. The name implies it performs "garbage collection" which means several different things, and some users may not want to use this operation that rewrites the entire object database.

Create a new 'maintenance' builtin that will become a more general- purpose command. To start, it will only support the 'run' subcommand, but will later expand to add subcommands for scheduling maintenance in the background.

For now, the 'maintenance' builtin is a thin shim over the 'gc' builtin. In fact, the only option is the '--auto' toggle, which is handed directly to the 'gc' builtin. The current change is isolated to this simple operation to prevent more interesting logic from being lost in all of the boilerplate of adding a new builtin.

Use existing builtin/gc.c file because we want to share code between the two builtins. It is possible that we will have 'maintenance' replace the 'gc' builtin entirely at some point, leaving 'git gc' as an alias for some specific arguments to 'git maintenance run'.

---
 .gitignore                        |  1 +
 Documentation/git-maintenance.txt | 57 +++++++++++++++++++++++++++++
 builtin.h                         |  1 +
 builtin/gc.c                      | 59 +++++++++++++++++++++++++++++++
 git.c                             |  1 +
 t/t7900-maintenance.sh            | 22 ++++++++++++
 6 files changed, 141 insertions(+)
 create mode 100644 Documentation/git-maintenance.txt
 create mode 100755 t/t7900-maintenance.sh

index ee509a2ad2..a5808fa30d 100644
@@ -90,6 +90,7 @@
 /git-ls-tree
 /git-mailinfo
 /git-mailsplit
+/git-maintenance
 /git-merge
 /git-merge-base
 /git-merge-index

index a5ae15bfe5..17c1c0ce49 100644
@@ -167,6 +167,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix);
 int cmd_ls_remote(int argc, const char **argv, const char *prefix);
 int cmd_mailinfo(int argc, const char **argv, const char *prefix);
 int cmd_mailsplit(int argc, const char **argv, const char *prefix);
+int cmd_maintenance(int argc, const char **argv, const char *prefix);
 int cmd_merge(int argc, const char **argv, const char *prefix);
 int cmd_merge_base(int argc, const char **argv, const char *prefix);
 int cmd_merge_index(int argc, const char **argv, const char *prefix);

index 2f021b97f3..ff56d1df24 100644
@@ -527,6 +527,7 @@ static struct cmd_struct commands[] = {
 	{ "ls-tree", cmd_ls_tree, RUN_SETUP },
 	{ "mailinfo", cmd_mailinfo, RUN_SETUP_GENTLY | NO_PARSEOPT },
 	{ "mailsplit", cmd_mailsplit, NO_PARSEOPT },
+	{ "maintenance", cmd_maintenance, RUN_SETUP_GENTLY | NO_PARSEOPT },
 	{ "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
 	{ "merge-base", cmd_merge_base, RUN_SETUP },
 	{ "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },

new file mode 100755
index 0000000000..d00641c4dd
@@ -0,0 +1,22 @@
+#!/bin/sh
+
+test_description='git maintenance builtin'
+
+GIT_TEST_COMMIT_GRAPH=0
+GIT_TEST_MULTI_PACK_INDEX=0
+
+. ./test-lib.sh
+
+test_expect_success 'help text' '
+	test_must_fail git maintenance -h 2>err &&
+	test_i18ngrep "usage: git maintenance run" err
+'
+
+test_expect_success 'gc [--auto]' '
+	GIT_TRACE2_EVENT="$(pwd)/run-no-auto.txt" git maintenance run &&
+	GIT_TRACE2_EVENT="$(pwd)/run-auto.txt" git maintenance run --auto &&
+	grep ",\"gc\"]" run-no-auto.txt  &&
+	grep ",\"gc\",\"--auto\"]" run-auto.txt
+'
+
+test_done

gitgitgadget

Everything in this patch looks very sensible to me, and I think that implementing 'git maintenance' in the same file as the current 'git gc' builtin makes a lot of sense to me.

Thanks,
Taylor

Hi,

Seems sensible, and a good way to set up for the later patches. Let's take a look at how it does that.

[...]

[...]

	run <task>...::

[...]

Hopefully this is documented to your satisfaction when the ability to customize the tasks is implemented.

Is this worth doing? Do we really want every command to document that "0 means success, everything else means failure, and some of those exit codes mean a specific kind of failure that is global to Git"?

This will grow, and I'd rather have one global struct than many individual global items. It makes it clearer when I use "opts.auto_flag" that this corresponds to whether "--auto" was provided as a command-line option.

Thanks. I noticed a few of these myself. Luckily, I'll be going through all of these invocations and replacing them with new methods soon ;)

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

Yes, these don't need to be here yet.

Consider this a "unit test" of the builtin. I'm testing whether the command-line arguments had an effect on the child process.

I'm specifically using GIT_TRACE2_EVENT, which is intended for consumption by computer, not humans. Adding whitespace or otherwise changing the output format would be an unnecessary disruption that is more likely to have downstream effects with external tools.

In some way, adding extra dependencies like this in our own test suite can help ensure the output format is more stable.

If there is a better way to ask "Did my command call 'git gc' (with no arguments|with these arguments)?" then I'm happy to consider it.

Thanks,
-Stolee

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

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

[...]

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

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