repack: implement `--expire-to` option

This series polishes off the RFC [1] I sent over the summer teaching git repack how to move pruned objects into a separate cruft pack in a different repository.

We've been using this series at GitHub for some time now as an escape-hatch mechanism for quickly recovering pruned objects that run afoul of the "advertise-then-prune" race described in [2].

The general idea is that we write out a second cruft pack to capture the pruned objects, with two minor changes:

- The second cruft pack counts the first cruft pack as "saved", meaning that we'll exclude its objects (thus ignoring both reachable and unreachable-but-recent objects from the second cruft pack).

- The second cruft pack does *not* exclude objects based on age (i.e., it acts as if --cruft-expiration=never were given) so as to avoid excluding objects which are being pruned due to their age.

The first few patches are preparatory, and the substantive change is implemented in the fourth patch. I would suggest that reviewers start there, and then read the first three patches.

Since this series basically boils down to adding one new conditional, like:

    if (delete_redundant && expire_to) {
        ret = write_cruft_pack(...):
        if (ret)
            return ret;
    }

The (second) call to write_cruft_pack() comes with a rather extensive inline comment explaining what is going on ;-).

Thanks in advance for your review.

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

Taylor Blau (4):
builtin/repack.c: pass "out" to prepare_pack_objects builtin/repack.c: pass "cruft_expiration" to write_cruft_pack builtin/repack.c: write cruft packs to arbitrary locations builtin/repack.c: implement --expire-to for storing pruned objects

 Documentation/git-repack.txt |   6 ++
 builtin/repack.c             |  67 ++++++++++++++++---
 t/t7700-repack.sh            | 121 +++++++++++++++++++++++++++++++++++
 3 files changed, 186 insertions(+), 8 deletions(-)

2.38.0.16.g393fd4c6db

builtin/repack.c's prepare_pack_objects() is used to prepare a set of arguments to a pack-objects process which will generate a desired pack.

A future patch will add an --expire-to option which allows git repack to write a cruft pack containing the pruned objects out to a separate repository. Prepare for this by teaching that function to write packs to an arbitrary location specified by the caller.

All existing callers of prepare_pack_objects() will pass packtmp for out, retaining the existing behavior.

---
 builtin/repack.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

index a5bacc7797..0a7bd57636 100644
@@ -188,7 +188,8 @@ static void remove_redundant_pack(const char *dir_name, const char *base_name)
 }
 
 static void prepare_pack_objects(struct child_process *cmd,
-				 const struct pack_objects_args *args)
+				 const struct pack_objects_args *args,
+				 const char *out)
 {
 	strvec_push(&cmd->args, "pack-objects");
 	if (args->window)

@@ -211,7 +212,7 @@ static void prepare_pack_objects(struct child_process *cmd,
 		strvec_push(&cmd->args,  "--quiet");
 	if (delta_base_offset)
 		strvec_push(&cmd->args,  "--delta-base-offset");
-	strvec_push(&cmd->args, packtmp);
+	strvec_push(&cmd->args, out);
 	cmd->git_cmd = 1;
 	cmd->out = -1;
 }

@@ -275,7 +276,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
 	FILE *out;
 	struct strbuf line = STRBUF_INIT;
 
-	prepare_pack_objects(&cmd, args);
+	prepare_pack_objects(&cmd, args, packtmp);
 	cmd.in = -1;
 
 	/*

@@ -673,7 +674,7 @@ static int write_cruft_pack(const struct pack_objects_args *args,
 	FILE *in, *out;
 	int ret;
 
-	prepare_pack_objects(&cmd, args);
+	prepare_pack_objects(&cmd, args, packtmp);
 
 	strvec_push(&cmd.args, "--cruft");
 	if (cruft_expiration)

@@ -861,7 +862,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	sigchain_push_common(remove_pack_on_signal);
 
-	prepare_pack_objects(&cmd, &po_args);
+	prepare_pack_objects(&cmd, &po_args, packtmp);
 
 	show_progress = !po_args.quiet && isatty(2);
 

2.38.0.16.g393fd4c6db

It does make sense to allow the caller to specify the name of the temporary file to be used, but is "out" a good name for that? The other two arguments are self explanatory both by their type and the name, but this is of type "const char *" that does not convey what the string is about at all, so giging a good name to the parameter is more important than for others.

The patch text itself is very straight-forward. Thanks.

I agree that the patch is nice and simple.

As for a name, this parameter specifies a file prefix. Perhaps 'pack_prefix' would be a good name for this?

Thanks,
-Stolee

builtin/repack.c's write_cruft_pack() is used to generate the cruft pack when --cruft is supplied. It uses a static variable "cruft_expiration" which is filled in by option parsing.

A future patch will add an --expire-to option which allows git repack to write a cruft pack containing the pruned objects out to a separate repository. In order to implement this functionality, some callers will have to pass a value for cruft_expiration different than the one filled out by option parsing.

Prepare for this by teaching write_cruft_pack to take a "cruft_expiration" parameter, instead of reading a single static variable.

The (sole) existing caller of write_cruft_pack() will pass the value for "cruft_expiration" filled in by option parsing, retaining existing behavior. This means that we can make the variable local to cmd_repack(), and eliminate the static declaration.

---
 builtin/repack.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index 0a7bd57636..1184e8c257 100644
@@ -32,7 +32,6 @@ static int write_bitmaps = -1;
 static int use_delta_islands;
 static int run_update_server_info = 1;
 static char *packdir, *packtmp_name, *packtmp;
-static char *cruft_expiration;
 
 static const char *const git_repack_usage[] = {
 	N_("git repack [<options>]"),

@@ -664,6 +663,7 @@ static int write_midx_included_packs(struct string_list *include,
 
 static int write_cruft_pack(const struct pack_objects_args *args,
 			    const char *pack_prefix,
+			    const char *cruft_expiration,
 			    struct string_list *names,
 			    struct string_list *existing_packs,
 			    struct string_list *existing_kept_packs)

@@ -746,6 +746,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	struct pack_objects_args cruft_po_args = {NULL};
 	int geometric_factor = 0;
 	int write_midx = 0;
+	const char *cruft_expiration = NULL;
 
 	struct option builtin_repack_options[] = {
 		OPT_BIT('a', NULL, &pack_everything,

@@ -985,7 +986,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		cruft_po_args.local = po_args.local;
 		cruft_po_args.quiet = po_args.quiet;
 
-		ret = write_cruft_pack(&cruft_po_args, pack_prefix, &names,
+		ret = write_cruft_pack(&cruft_po_args, pack_prefix,
+				       cruft_expiration, &names,
 				       &existing_nonkept_packs,
 				       &existing_kept_packs);
 		if (ret)

2.38.0.16.g393fd4c6db

Looks good to me.

In the following commit, a new write_cruft_pack() caller will be added which wants to write a cruft pack to an arbitrary location. Prepare for this by adding a parameter which controls the destination of the cruft pack.

For now, provide "packtmp" so that this commit does not change any behavior.

---
 builtin/repack.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

index 1184e8c257..a5386ac893 100644
@@ -662,6 +662,7 @@ static int write_midx_included_packs(struct string_list *include,
 }
 
 static int write_cruft_pack(const struct pack_objects_args *args,
+			    const char *destination,
 			    const char *pack_prefix,
 			    const char *cruft_expiration,
 			    struct string_list *names,

@@ -673,8 +674,10 @@ static int write_cruft_pack(const struct pack_objects_args *args,
 	struct string_list_item *item;
 	FILE *in, *out;
 	int ret;
+	const char *scratch;
+	int local = skip_prefix(destination, packdir, &scratch);
 
-	prepare_pack_objects(&cmd, args, packtmp);
+	prepare_pack_objects(&cmd, args, destination);
 
 	strvec_push(&cmd.args, "--cruft");
 	if (cruft_expiration)

@@ -714,7 +717,12 @@ static int write_cruft_pack(const struct pack_objects_args *args,
 		if (line.len != the_hash_algo->hexsz)
 			die(_("repack: Expecting full hex object ID lines only "
 			      "from pack-objects."));
-		string_list_append(names, line.buf);
+                /*
+		 * avoid putting packs written outside of the repository in the
+		 * list of names
+		 */
+		if (local)
+			string_list_append(names, line.buf);
 	}
 	fclose(out);
 

@@ -986,7 +994,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		cruft_po_args.local = po_args.local;
 		cruft_po_args.quiet = po_args.quiet;
 
-		ret = write_cruft_pack(&cruft_po_args, pack_prefix,
+		ret = write_cruft_pack(&cruft_po_args, packtmp, pack_prefix,
 				       cruft_expiration, &names,
 				       &existing_nonkept_packs,
 				       &existing_kept_packs);

2.38.0.16.g393fd4c6db

It would be nice if it were possible to avoid passing names entirely here, but we still need it to determine the set of packs we already wrote a few lines above when we write the input to pack-objects --cruft.

Thanks,
Taylor

Odd. In my copy, which I got from gitster/git before Junio went offline, the alignment looks fine.

Thanks for double checking, though.

Thanks,
Taylor

When pruning objects with --cruft, git repack offers some flexibility when selecting the set of which objects are pruned via the --cruft-expiration option.

This is useful for expiring objects which are older than the grace period, making races where to-be-pruned objects become reachable and then ancestors of freshly pushed objects, leaving the repository in a corrupt state after pruning substantially less likely [1].

But in practice, such races are impossible to avoid entirely, no matter how long the grace period is. To prevent this race, it is often advisable to temporarily put a repository into a read-only state. But in practice, this is not always practical, and so some middle ground would be nice.

This patch introduces a new option, --expire-to, which teaches git repack to write an additional cruft pack containing just the objects which were pruned from the repository. The caller can specify a directory outside of the current repository as the destination for this second cruft pack.

This makes it possible to prune objects from a repository, while still holding onto a supplemental copy of them outside of the original repository. Having this copy on-disk makes it substantially easier to recover objects when the aforementioned race is encountered.

--expire-to is implemented in a somewhat convoluted manner, which is to take advantage of the fact that the first time write_cruft_pack() is called, it adds the name of the cruft pack to the names string list. That means the second time we call write_cruft_pack(), objects in the previously-written cruft pack will be excluded.

As long as the caller ensures that no objects are expired during the second pass, this is sufficient to generate a cruft pack containing all objects which don't appear in any of the new packs written by git repack, including the cruft pack. In other words, all of the objects which are about to be pruned from the repository.

It is important to note that the destination in --expire-to does not necessarily need to be a Git repository (though it can be) Notably, the expired packs do not contain all ancestors of expired objects. So if the source repository contains something like:

              <unreachable>
             /
    C1 --- C2
      \
       refs/heads/master

where C2 is unreachable, but has a parent (C1) which is reachable, and C2 would be pruned, then the expiry pack will contain only C2, not C1.

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

---
 Documentation/git-repack.txt |   6 ++
 builtin/repack.c             |  40 ++++++++++++
 t/t7700-repack.sh            | 121 +++++++++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+)

index 0bf13893d8..4017157949 100644
@@ -74,6 +74,12 @@ to the new separate pack will be written.
 	immediately instead of waiting for the next `git gc` invocation.
 	Only useful with `--cruft -d`.
 
+--expire-to=<dir>::
+	Write a cruft pack containing pruned objects (if any) to the
+	directory `<dir>`. This option is useful for keeping a copy of
+	any pruned objects in a separate directory as a backup. Only
+	useful with `--cruft -d`.
+
 -l::
 	Pass the `--local` option to 'git pack-objects'. See
 	linkgit:git-pack-objects[1].

index a5386ac893..3bc18e0b2f 100644
@@ -702,6 +702,10 @@ static int write_cruft_pack(const struct pack_objects_args *args,
 	 * By the time it is read here, it contains only the pack(s)
 	 * that were just written, which is exactly the set of packs we
 	 * want to consider kept.
+	 *
+	 * If `--expire-to` is given, the double-use served by `names`
+	 * ensures that the pack written to `--expire-to` excludes any
+	 * objects contained in the cruft pack.
 	 */
 	in = xfdopen(cmd.in, "w");
 	for_each_string_list_item(item, names)

@@ -755,6 +759,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	int geometric_factor = 0;
 	int write_midx = 0;
 	const char *cruft_expiration = NULL;
+	const char *expire_to = NULL;
 
 	struct option builtin_repack_options[] = {
 		OPT_BIT('a', NULL, &pack_everything,

@@ -804,6 +809,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 			    N_("find a geometric progression with factor <N>")),
 		OPT_BOOL('m', "write-midx", &write_midx,
 			   N_("write a multi-pack index of the resulting packs")),
+		OPT_STRING(0, "expire-to", &expire_to, N_("dir"),
+			   N_("pack prefix to store a pack containing pruned objects")),
 		OPT_END()
 	};
 

@@ -1000,6 +1007,39 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 				       &existing_kept_packs);
 		if (ret)
 			return ret;
+
+		if (delete_redundant && expire_to) {
+			/*
+			 * If `--expire-to` is given with `-d`, it's possible
+			 * that we're about to prune some objects. With cruft
+			 * packs, pruning is implicit: any objects from existing
+			 * packs that weren't picked up by new packs are removed
+			 * when their packs are deleted.
+			 *
+			 * Generate an additional cruft pack, with one twist:
+			 * `names` now includes the name of the cruft pack
+			 * written in the previous step. So the contents of
+			 * _this_ cruft pack exclude everything contained in the
+			 * existing cruft pack (that is, all of the unreachable
+			 * objects which are no older than
+			 * `--cruft-expiration`).
+			 *
+			 * To make this work, cruft_expiration must become NULL
+			 * so that this cruft pack doesn't actually prune any
+			 * objects. If it were non-NULL, this call would always
+			 * generate an empty pack (since every object not in the
+			 * cruft pack generated above will have an mtime older
+			 * than the expiration).
+			 */
+			ret = write_cruft_pack(&cruft_po_args, expire_to,
+					       pack_prefix,
+					       NULL,
+					       &names,
+					       &existing_nonkept_packs,
+					       &existing_kept_packs);
+			if (ret)
+				return ret;
+		}
 	}
 
 	string_list_sort(&names);

2.38.0.16.g393fd4c6db

This patch looks good.

Creating these cruft packs in the expire directory does help remove the main problem of the race condition. This currently requires some external system to check that no races happened and it is safe to delete the pack in the expire directory. Also, that external tool is needed to move that expired pack (or preferably, only the necessary objects) back into the repository.

Some future series could find a way to handle those situations, perhaps within 'git fsck' to pull missing reachable objects from the expire directory. Definitely not required for this series, though.

Thanks,
-Stolee

Thanks. Let's start merging this one down.

Thanks,
Taylor