| Message ID | 73ad7b90e1fe6c15f41ff828651f7ab06076ffd8.1683072587.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] builtin/pack-objects.c: introduce `pack.extraCruftTips` | expand |
On 5/2/2023 8:09 PM, Taylor Blau wrote: > This patch introduces a new multi-valued configuration option, > `pack.extraCruftTips` as an alternative means to mark certain objects in > the cruft pack as rescued, even if they would otherwise be pruned. > Range-diff against v1: > 1: 8af478ebe3 ! 1: 73ad7b90e1 builtin/pack-objects.c: introduce `pack.extraCruftTips` > @@ Commit message > > - to point a reference at them, which may be undesirable or > infeasible, > + - to track them via the reflog, which may be undesirable since the > + reflog's lifetime is limited to that of the reference it's tracking > + (and callers may want to keep those unreachable objects around for > + longer) > - to extend the grace period, which may keep around other objects that > the caller *does* want to discard, > - or, to force the caller to construct the pack of objects they want > @@ Documentation/config/pack.txt: pack.deltaCacheLimit:: > ++ > +Output must contain exactly one hex object ID per line, and nothing > +else. Objects which cannot be found in the repository are ignored. > ++Multiple hooks are supported, but all must exit successfully, else no > ++cruft pack will be generated. Thanks for these updates. > pack.threads:: > Specifies the number of threads to spawn when searching for best > @@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct > + add_pending_object(revs, obj, ""); > + } > + > ++ ret = finish_command(&cmd); > ++ > +done: > + if (out) > + fclose(out); > @@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct > + for (i = 0; i < programs->nr; i++) { > + ret = enumerate_extra_cruft_tips_1(&revs, > + programs->items[i].string); > -+ if (!ret) > ++ if (ret) > + break; > + } If we have a failure, then we stop immediately and report a failure (from this bit outside the range-diff's context): > + if (ret) > + die(_("unable to enumerate additional cruft tips")); > @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v > + cruft_new="$(git rev-parse HEAD)" && > + > + git checkout main && > -+ git branch -D old new && > ++ git branch -D discard old new && Good update. > + git reflog expire --all --expire=all && > + > + # mark cruft.old with an mtime that is many minutes > @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v > + mtimes="$(ls .git/objects/pack/pack-*.mtimes)" && > + git show-index <${mtimes%.mtimes}.idx >cruft && > + cut -d" " -f2 cruft | sort >cruft.actual && > -+ test_cmp cruft.expect cruft.actual > ++ test_cmp cruft.expect cruft.actual && > ++ > ++ # Ensure that the "old" objects are removed after > ++ # dropping the pack.extraCruftTips hook. > ++ git config --unset pack.extraCruftTips && > ++ git repack --cruft --cruft-expiration=now -d && Double-checking that removing the tips correctly removes the objects at this time is good, but... > ++ mtimes="$(ls .git/objects/pack/pack-*.mtimes)" && > ++ git show-index <${mtimes%.mtimes}.idx >cruft && > ++ cut -d" " -f2 cruft | sort >cruft.actual && > ++ > ++ git rev-list --objects --no-object-names $cruft_new >cruft.raw && > ++ cp cruft.expect cruft.old && > ++ sort cruft.raw >cruft.expect && > ++ test_cmp cruft.expect cruft.actual && > ++ > ++ # ensure objects which are no longer in the cruft pack were > ++ # removed from the repository > ++ for object in $(comm -13 cruft.expect cruft.old) > ++ do > ++ test_must_fail git cat-file -t $object || return 1 > ++ done ...it would be good to do this check before the removal of the hook to be sure the objects from 'discard' are removed as part of the step with the hook. I can imagine a world where the hook-based approach erroneously keeps the 'discard' objects in the non-cruft pack only for them to be deleted by the repack where hooks are disabled, and having a test would help guarantee we don't live in that hypothetical world. I would also be satisfied with checking just the commits that were previously referenced by 'discard' and 'old', but this more thorough check is also good. > ++test_expect_success 'multi-valued pack.extraCruftTips' ' > ++ git init repo && > ++ test_when_finished "rm -fr repo" && > ++ ( > ++ cd repo && > ++ > ++ test_commit base && > ++ git branch -M main && > ++ > ++ git checkout --orphan cruft.a && > ++ git rm -fr . && > ++ test_commit --no-tag cruft.a && > ++ cruft_a="$(git rev-parse HEAD)" && > ++ > ++ git checkout --orphan cruft.b && > ++ git rm -fr . && > ++ test_commit --no-tag cruft.b && > ++ cruft_b="$(git rev-parse HEAD)" && > ++ > ++ git checkout main && > ++ git branch -D cruft.a cruft.b && > ++ git reflog expire --all --expire=all && > ++ > ++ echo "echo $cruft_a" | write_script extra-tips.a && > ++ echo "echo $cruft_b" | write_script extra-tips.b && > ++ echo "false" | write_script extra-tips.c && > ++ > ++ git rev-list --objects --no-object-names $cruft_a $cruft_b \ > ++ >cruft.raw && > ++ sort cruft.raw >cruft.expect && > ++ > ++ # ensure that each extra cruft tip is saved by its > ++ # respective hook > ++ git config --add pack.extraCruftTips ./extra-tips.a && > ++ git config --add pack.extraCruftTips ./extra-tips.b && > ++ git repack --cruft --cruft-expiration=now -d && > ++ > ++ mtimes="$(ls .git/objects/pack/pack-*.mtimes)" && > ++ git show-index <${mtimes%.mtimes}.idx >cruft && > ++ cut -d" " -f2 cruft | sort >cruft.actual && > ++ test_cmp cruft.expect cruft.actual && > ++ > ++ # ensure that a dirty exit halts cruft pack generation > ++ git config --add pack.extraCruftTips ./extra-tips.c && > ++ test_must_fail git repack --cruft -d 2>err && > ++ grep "unable to enumerate additional cruft tips" err && > ++ > ++ # and that the existing cruft pack is left alone > ++ test_path_is_file "$mtimes" > + ) This new test looks good to me. Thanks, -Stolee
On Tue, May 02, 2023 at 08:09:47PM -0400, Taylor Blau wrote: > However, there is no option to be able to keep around certain objects > that have otherwise aged out of the grace period. The only way to retain > those objects is: > > - to point a reference at them, which may be undesirable or > infeasible, > - to track them via the reflog, which may be undesirable since the > reflog's lifetime is limited to that of the reference it's tracking > (and callers may want to keep those unreachable objects around for > longer) > - to extend the grace period, which may keep around other objects that > the caller *does* want to discard, > - or, to force the caller to construct the pack of objects they want > to keep themselves, and then mark the pack as kept by adding a > ".keep" file. OK, I understand the use case you're trying to support, and your approach mostly makes sense. But there are two things I was surprised by in the implementation: 1. Does this need to be tied to cruft packs? The same logic would apply to "repack -A" which turns objects loose (of course you probably don't want to do that in the long term for performance reasons, but it's conceptually the same thing; see below). 2. Why is there a separate walk distinct from the one that rescues recent-but-unreachable objects? Conceptually it seems to me that the simplest and most flexible way to think of this new feature is: pretend these objects are recent enough to be kept in the grace period, even though their mtimes do not qualify". And then everything else would just fall out naturally. Am I missing something? In a pre-cruft-pack world, I'd have just expected the patch to look like this: diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index bd6ad016d6..1d655dc758 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -4077,6 +4077,7 @@ static void get_object_list(struct rev_info *revs, int ac, const char **av) if (add_unseen_recent_objects_to_traversal(revs, unpack_unreachable_expiration, NULL, 0)) die(_("unable to add recent objects")); + add_tips_from_program(revs); if (prepare_revision_walk(revs)) die(_("revision walk setup failed")); traverse_commit_list(revs, record_recent_commit, Sadly the cruft-pack feature doesn't really share this code, but I think it does something similar, and could just consider those synthetic tips as "recent" for this run. > +static int enumerate_extra_cruft_tips_1(struct rev_info *revs, const char *args) > +{ > + struct child_process cmd = CHILD_PROCESS_INIT; > + struct strbuf buf = STRBUF_INIT; > + FILE *out = NULL; > + int ret = 0; > + > + cmd.use_shell = 1; > + cmd.out = -1; > + > + strvec_push(&cmd.args, args); > + strvec_pushv(&cmd.env, (const char **)local_repo_env); Why does this clear the environment of local-repo variables? That seems unlike most other hooks we have, and would cause confusion if $GIT_DIR or various config variables are important (e.g., should "git -c foo.bar gc" persist foo.bar when running the hook? It usually does). I know that some hooks that try to change repositories by changing directories have the opposite confusion ($GIT_DIR is set, but they did not want it). But it makes more sense to me to remain consistent with how other hooks behave. > + if (start_command(&cmd)) { > + ret = -1; > + goto done; > + } This may be a matter of taste, but you can "return -1" directly here, as nothing has been allocated (and a failed start_command() will call child_process_clear() for you). This would mean "out" is always set in the "done:" label, so it wouldn't need a NULL-check (nor an initialization). > + > + out = xfdopen(cmd.out, "r"); > + while (strbuf_getline_lf(&buf, out) != EOF) { is there any reason to be a stickler for LF versus CRLF here? I.e., wouldn't strbuf_getline() be more friendly, with little chance that we misinterpret the result? > + struct object_id oid; > + struct object *obj; > + const char *rest; > + > + if (parse_oid_hex(buf.buf, &oid, &rest) || *rest) { > + ret = error(_("invalid extra cruft tip: '%s'"), buf.buf); > + goto done; > + } > + > + obj = parse_object(the_repository, &oid); > + if (!obj) > + continue; This parse_object() can be pretty expensive, especially if you are rescuing a lot of objects (or if your program directly references large blobs). Can we use oid_object_info() here in combination with lookup_object_by_type()? -Peff
On Wed, May 03, 2023 at 03:59:06PM -0400, Jeff King wrote: > On Tue, May 02, 2023 at 08:09:47PM -0400, Taylor Blau wrote: > > > However, there is no option to be able to keep around certain objects > > that have otherwise aged out of the grace period. The only way to retain > > those objects is: > > > > - to point a reference at them, which may be undesirable or > > infeasible, > > - to track them via the reflog, which may be undesirable since the > > reflog's lifetime is limited to that of the reference it's tracking > > (and callers may want to keep those unreachable objects around for > > longer) > > - to extend the grace period, which may keep around other objects that > > the caller *does* want to discard, > > - or, to force the caller to construct the pack of objects they want > > to keep themselves, and then mark the pack as kept by adding a > > ".keep" file. > > OK, I understand the use case you're trying to support, and your > approach mostly makes sense. But there are two things I was surprised by > in the implementation: > > 1. Does this need to be tied to cruft packs? The same logic would > apply to "repack -A" which turns objects loose (of course you > probably don't want to do that in the long term for performance > reasons, but it's conceptually the same thing; see below). > > 2. Why is there a separate walk distinct from the one that rescues > recent-but-unreachable objects? > Conceptually it seems to me that the simplest and most flexible way to > think of this new feature is: pretend these objects are recent enough to > be kept in the grace period, even though their mtimes do not qualify". > > And then everything else would just fall out naturally. Am I missing > something? I originally shied away from it, thinking that I wouldn't want to do an expensive walk with all of the recent objects during a non-pruning repack. The two code paths are quite different in practice. In the pruning case, we add only new objects from the kept packs and then start our traversal there. In the non-pruning case, we just do `add_objects_in_unpacked_packs()` which is really just a call to `for_each_packed_object()`. So it gets tricky when you have a pack.extraCruftTips program and want to invoke it in a non-pruning case. You could do something like: - call enumerate_and_traverse_cruft_objects() *always*, either because we were doing a pruning GC, or calling it after `enumerate_cruft_objects()` (in the non-pruning case) - ensure that enumerate_and_traverse_cruft_objects() is a noop when (a) cruft_expiration is set to zero, and (b) there are no pack.extraCruftTips programs specified > > +static int enumerate_extra_cruft_tips_1(struct rev_info *revs, const char *args) > > +{ > > + struct child_process cmd = CHILD_PROCESS_INIT; > > + struct strbuf buf = STRBUF_INIT; > > + FILE *out = NULL; > > + int ret = 0; > > + > > + cmd.use_shell = 1; > > + cmd.out = -1; > > + > > + strvec_push(&cmd.args, args); > > + strvec_pushv(&cmd.env, (const char **)local_repo_env); > > Why does this clear the environment of local-repo variables? That seems > unlike most other hooks we have, and would cause confusion if $GIT_DIR > or various config variables are important (e.g., should "git -c foo.bar > gc" persist foo.bar when running the hook? It usually does). > > I know that some hooks that try to change repositories by changing > directories have the opposite confusion ($GIT_DIR is set, but they did > not want it). But it makes more sense to me to remain consistent with > how other hooks behave. Copy-and-pasted from the core.alternateRefsCommand stuff, there is no need for this here. > > + if (start_command(&cmd)) { > > + ret = -1; > > + goto done; > > + } > > This may be a matter of taste, but you can "return -1" directly here, as > nothing has been allocated (and a failed start_command() will call > child_process_clear() for you). This would mean "out" is always set in > the "done:" label, so it wouldn't need a NULL-check (nor an > initialization). Very nice, thanks for the suggestion. > > + > > + out = xfdopen(cmd.out, "r"); > > + while (strbuf_getline_lf(&buf, out) != EOF) { > > is there any reason to be a stickler for LF versus CRLF here? I.e., > wouldn't strbuf_getline() be more friendly, with little chance that we > misinterpret the result? No reason, other than I happened to type this one out first ;-). We should definitely be more permissive here, I agree that strbuf_getline() is the friendlier choice. > > + struct object_id oid; > > + struct object *obj; > > + const char *rest; > > + > > + if (parse_oid_hex(buf.buf, &oid, &rest) || *rest) { > > + ret = error(_("invalid extra cruft tip: '%s'"), buf.buf); > > + goto done; > > + } > > + > > + obj = parse_object(the_repository, &oid); > > + if (!obj) > > + continue; > > This parse_object() can be pretty expensive, especially if you are > rescuing a lot of objects (or if your program directly references large > blobs). Can we use oid_object_info() here in combination with > lookup_object_by_type()? Yep, definitely. Nice catch ;-). Thanks, Taylor
On Wed, May 03, 2023 at 03:59:06PM -0400, Jeff King wrote: > On Tue, May 02, 2023 at 08:09:47PM -0400, Taylor Blau wrote: > > > However, there is no option to be able to keep around certain objects > > that have otherwise aged out of the grace period. The only way to retain > > those objects is: > > > > - to point a reference at them, which may be undesirable or > > infeasible, > > - to track them via the reflog, which may be undesirable since the > > reflog's lifetime is limited to that of the reference it's tracking > > (and callers may want to keep those unreachable objects around for > > longer) > > - to extend the grace period, which may keep around other objects that > > the caller *does* want to discard, > > - or, to force the caller to construct the pack of objects they want > > to keep themselves, and then mark the pack as kept by adding a > > ".keep" file. > > OK, I understand the use case you're trying to support, and your > approach mostly makes sense. But there are two things I was surprised by > in the implementation: > > 1. Does this need to be tied to cruft packs? The same logic would > apply to "repack -A" which turns objects loose (of course you > probably don't want to do that in the long term for performance > reasons, but it's conceptually the same thing; see below). I agree that you wouldn't want to do it for performance reasons, but I'm comfortable with the asymmetry here, since this is `pack.extraCruftTips` (emphasis on "cruft"), so it's not clear that it has to be related to "repack -A". Happy to change things up if you feel strongly, though. Thanks, Taylor
On Wed, May 03, 2023 at 05:22:04PM -0400, Taylor Blau wrote: > > OK, I understand the use case you're trying to support, and your > > approach mostly makes sense. But there are two things I was surprised by > > in the implementation: > > > > 1. Does this need to be tied to cruft packs? The same logic would > > apply to "repack -A" which turns objects loose (of course you > > probably don't want to do that in the long term for performance > > reasons, but it's conceptually the same thing; see below). > > > > 2. Why is there a separate walk distinct from the one that rescues > > recent-but-unreachable objects? > > > Conceptually it seems to me that the simplest and most flexible way to > > think of this new feature is: pretend these objects are recent enough to > > be kept in the grace period, even though their mtimes do not qualify". > > > > And then everything else would just fall out naturally. Am I missing > > something? > > I originally shied away from it, thinking that I wouldn't want to do an > expensive walk with all of the recent objects during a non-pruning > repack. > > The two code paths are quite different in practice. In the pruning case, > we add only new objects from the kept packs and then start our traversal > there. In the non-pruning case, we just do > `add_objects_in_unpacked_packs()` which is really just a call to > `for_each_packed_object()`. Right, that's what I'd expect. I think you are describing a cruft-pack world here (because you say "kept packs"), but the traditional "repack -k" versus "repack -A" is similar (if we are not doing something special with recent objects, then there is no need to figure out what they reach; we can just add them all). > So it gets tricky when you have a pack.extraCruftTips program and want > to invoke it in a non-pruning case. You could do something like: > > - call enumerate_and_traverse_cruft_objects() *always*, either because > we were doing a pruning GC, or calling it after > `enumerate_cruft_objects()` (in the non-pruning case) > > - ensure that enumerate_and_traverse_cruft_objects() is a noop when > (a) cruft_expiration is set to zero, and (b) there are no > pack.extraCruftTips programs specified I'm not sure why you'd need to traverse, though. If we are in "-k" mode, we are keeping everything anyway (so I don't even see the point of asking the helper about extra tips). And all of those objects that are not reachable from the regular traversal are by definition "cruft" and go into the cruft pack. Maybe I don't understand what you mean by "non-pruning" here. -Peff
On Wed, May 03, 2023 at 05:28:45PM -0400, Taylor Blau wrote: > > OK, I understand the use case you're trying to support, and your > > approach mostly makes sense. But there are two things I was surprised by > > in the implementation: > > > > 1. Does this need to be tied to cruft packs? The same logic would > > apply to "repack -A" which turns objects loose (of course you > > probably don't want to do that in the long term for performance > > reasons, but it's conceptually the same thing; see below). > > I agree that you wouldn't want to do it for performance reasons, but I'm > comfortable with the asymmetry here, since this is `pack.extraCruftTips` > (emphasis on "cruft"), so it's not clear that it has to be related to > "repack -A". > > Happy to change things up if you feel strongly, though. I don't feel strongly. I certainly have no intent to run "git repack -A" with extra tips specified. Mostly I just thought that it would be simpler to just apply it everywhere, both conceptually to the user and within the implementation. But since the cruft-pack implementation diverges quite a bit from the regular "-A" handling, I guess it makes the code more complex rather than less. The asymmetry feels like a wart to me, but I guess in the long run we'd hope that the "turn unreachable loose" strategy is deprecated anyway. -Peff
On Fri, May 05, 2023 at 05:26:31PM -0400, Jeff King wrote: > But since the cruft-pack implementation diverges quite a bit from the > regular "-A" handling, I guess it makes the code more complex rather > than less. The asymmetry feels like a wart to me, but I guess in the > long run we'd hope that the "turn unreachable loose" strategy is > deprecated anyway. One thing that could make this a lot simpler is if the code was added to "are we recent" code paths in the first place. Something like this: diff --git a/reachable.c b/reachable.c index 55bb114353..86bb5e021e 100644 --- a/reachable.c +++ b/reachable.c @@ -16,6 +16,9 @@ #include "object-store.h" #include "pack-bitmap.h" #include "pack-mtimes.h" +#include "oidset.h" +#include "run-command.h" +#include "config.h" struct connectivity_progress { struct progress *progress; @@ -67,8 +70,71 @@ struct recent_data { timestamp_t timestamp; report_recent_object_fn *cb; int ignore_in_core_kept_packs; + struct oidset fake_recent_oids; + int fake_recent_oids_loaded; }; +static int run_one_recent_cmd(struct oidset *set, const char *args) +{ + struct child_process cmd = CHILD_PROCESS_INIT; + struct strbuf buf = STRBUF_INIT; + FILE *out; + int ret = 0; + + cmd.use_shell = 1; + cmd.out = -1; + + strvec_push(&cmd.args, args); + + if (start_command(&cmd)) + return -1; + + out = xfdopen(cmd.out, "r"); + while (strbuf_getline(&buf, out) != EOF) { + struct object_id oid; + const char *rest; + + if (parse_oid_hex(buf.buf, &oid, &rest) || *rest) + die(_("invalid extra cruft tip: '%s'"), buf.buf); + + oidset_insert(set, &oid); + } + + if (finish_command(&cmd)) + die(_("cruft tip hook returned error")); + + fclose(out); + strbuf_release(&buf); + + return ret; +} + +static int obj_is_recent(const struct object_id *oid, timestamp_t mtime, + struct recent_data *data) +{ + if (mtime > data->timestamp) + return 1; + + if (!data->fake_recent_oids_loaded) { + const struct string_list *programs; + + if (!git_config_get_string_multi("pack.extracrufttips", &programs)) { + size_t i; + for (i = 0; i < programs->nr; i++) { + if (run_one_recent_cmd(&data->fake_recent_oids, + programs->items[i].string) < 0) + die(_("unable to enumerate additional cruft tips")); + } + } + data->fake_recent_oids_loaded = 1; + } + + if (oidset_contains(&data->fake_recent_oids, oid)) + return 1; + + return 0; +} + static void add_recent_object(const struct object_id *oid, struct packed_git *pack, off_t offset, @@ -78,7 +144,7 @@ static void add_recent_object(const struct object_id *oid, struct object *obj; enum object_type type; - if (mtime <= data->timestamp) + if (!obj_is_recent(oid, mtime, data)) return; /* @@ -193,6 +259,10 @@ int add_unseen_recent_objects_to_traversal(struct rev_info *revs, data.cb = cb; data.ignore_in_core_kept_packs = ignore_in_core_kept_packs; + /* XXX must free before exiting function */ + oidset_init(&data.fake_recent_oids, 0); + data.fake_recent_oids_loaded = 0; + r = for_each_loose_object(add_recent_loose, &data, FOR_EACH_OBJECT_LOCAL_ONLY); if (r) That would affect both "repack -A" and "repack --cruft". It would also affect "git prune", but that seems like a good thing to me. It would not affect a straight "repack -ad", since there we do not ask about recency at all. I'm not sure if that use case is important or not. If it is, we could easily trigger the "is it recent" code in that case only if a hook is configured (we'd just set the cutoff to "0" so that everything looks "too old"). Note that this inverts the loop logic from what you have. In your patch, you ask the hook to enumerate all faux-recent objects, and then you treat them separately from objects we found on disk. Here we enumerate the objects on disk as usual, and just adjust our idea of "recent" based on the hook. I think this is a bit simpler because objects we don't even bother to ask it about objects we've already handled, are already marked as recent, or are not present in the repository. -Peff
On Fri, May 05, 2023 at 05:23:22PM -0400, Jeff King wrote: > On Wed, May 03, 2023 at 05:22:04PM -0400, Taylor Blau wrote: > > > So it gets tricky when you have a pack.extraCruftTips program and want > > to invoke it in a non-pruning case. You could do something like: > > > > - call enumerate_and_traverse_cruft_objects() *always*, either because > > we were doing a pruning GC, or calling it after > > `enumerate_cruft_objects()` (in the non-pruning case) > > > > - ensure that enumerate_and_traverse_cruft_objects() is a noop when > > (a) cruft_expiration is set to zero, and (b) there are no > > pack.extraCruftTips programs specified > > I'm not sure why you'd need to traverse, though. If we are in "-k" mode, > we are keeping everything anyway (so I don't even see the point of > asking the helper about extra tips). And all of those objects that are > not reachable from the regular traversal are by definition "cruft" and > go into the cruft pack. > > Maybe I don't understand what you mean by "non-pruning" here. By non-pruning, I meant something like "git gc --prune=never", which would run the equivalent of `git repack -A` to generate the pack containing just reachable objects, and then invoke `git pack-objects --cruft` to generate the cruft pack. Thanks, Taylor
On Fri, May 05, 2023 at 06:13:57PM -0400, Jeff King wrote: > One thing that could make this a lot simpler is if the code was added to > "are we recent" code paths in the first place. > > Something like this: This is quite nice, and I think that it's a good direction to push this ~series~ patch in before queuing. That said, I wasn't sure about... > @@ -78,7 +144,7 @@ static void add_recent_object(const struct object_id *oid, > struct object *obj; > enum object_type type; > > - if (mtime <= data->timestamp) > + if (!obj_is_recent(oid, mtime, data)) > return; > > /* ...this hunk. That only kicks in if you have other recent object(s), since the hooks are consulted as a side-effect of calling your new `obj_is_recent()` function. I think in most cases that's fine, but if you had no otherwise-recent objects around, then this code wouldn't kick in in the first place. I wonder if it might make more sense to call the hooks directly in add_unseen_recent_objects_to_traversal(). > That would affect both "repack -A" and "repack --cruft". It would also > affect "git prune", but that seems like a good thing to me. I was going to say, I'm not sure this would work since we don't use any of this code from enumerate_cruft_objects() when the cruft_expiration is set to "never", but that's fine since we're keeping all of those objects anyway. OK, my bad, I think that was the point you were trying to make in your previous email, but I didn't quite grok it at the time. Yes, I agree, this code only needs to kick in when pruning. Thanks, Taylor
On Fri, May 05, 2023 at 08:06:14PM -0400, Taylor Blau wrote: > On Fri, May 05, 2023 at 05:23:22PM -0400, Jeff King wrote: > > On Wed, May 03, 2023 at 05:22:04PM -0400, Taylor Blau wrote: > > > > > So it gets tricky when you have a pack.extraCruftTips program and want > > > to invoke it in a non-pruning case. You could do something like: > > > > > > - call enumerate_and_traverse_cruft_objects() *always*, either because > > > we were doing a pruning GC, or calling it after > > > `enumerate_cruft_objects()` (in the non-pruning case) > > > > > > - ensure that enumerate_and_traverse_cruft_objects() is a noop when > > > (a) cruft_expiration is set to zero, and (b) there are no > > > pack.extraCruftTips programs specified > > > > I'm not sure why you'd need to traverse, though. If we are in "-k" mode, > > we are keeping everything anyway (so I don't even see the point of > > asking the helper about extra tips). And all of those objects that are > > not reachable from the regular traversal are by definition "cruft" and > > go into the cruft pack. > > > > Maybe I don't understand what you mean by "non-pruning" here. > > By non-pruning, I meant something like "git gc --prune=never", which > would run the equivalent of `git repack -A` to generate the pack > containing just reachable objects, and then invoke `git pack-objects > --cruft` to generate the cruft pack. Oops, my misunderstanding. I see what you're saying: in a `git gc --prune=never`, it does not matter whether we ask for extra cruft tips, since we're keeping all of those objects anyway. Duh. Thanks, Taylor
On Fri, May 05, 2023 at 08:13:45PM -0400, Taylor Blau wrote: > On Fri, May 05, 2023 at 06:13:57PM -0400, Jeff King wrote: > > One thing that could make this a lot simpler is if the code was added to > > "are we recent" code paths in the first place. > > > > Something like this: > > This is quite nice, and I think that it's a good direction to push this > ~series~ patch in before queuing. That said, I wasn't sure about... > > > @@ -78,7 +144,7 @@ static void add_recent_object(const struct object_id *oid, > > struct object *obj; > > enum object_type type; > > > > - if (mtime <= data->timestamp) > > + if (!obj_is_recent(oid, mtime, data)) > > return; > > > > /* > > ...this hunk. That only kicks in if you have other recent object(s), > since the hooks are consulted as a side-effect of calling your new > `obj_is_recent()` function. > > I think in most cases that's fine, but if you had no otherwise-recent > objects around, then this code wouldn't kick in in the first place. > > I wonder if it might make more sense to call the hooks directly in > add_unseen_recent_objects_to_traversal(). OK, no, this is fine, but we'd need to make sure that want_recent_object() also understood the fake recent set here, since add_recent_loose() and add_recent_packed() both bail early when want_recent_object() returns 0. Thanks, Taylor
On Fri, May 05, 2023 at 08:13:45PM -0400, Taylor Blau wrote: > > @@ -78,7 +144,7 @@ static void add_recent_object(const struct object_id *oid, > > struct object *obj; > > enum object_type type; > > > > - if (mtime <= data->timestamp) > > + if (!obj_is_recent(oid, mtime, data)) > > return; > > > > /* > > ...this hunk. That only kicks in if you have other recent object(s), > since the hooks are consulted as a side-effect of calling your new > `obj_is_recent()` function. I think we'll evaluate each object in the repo for recent-ness, via for_each_loose_object and for_each_packed_object. So if an object exists in the repo it will be evaluated here, as long as we are checking for recent objects at all. And if it doesn't exist, then having the hook tell us about it won't help; there is nothing for us to save (nor even to use as a source of reachability, since we don't know what's in it). Modulo the want_recent_object() thing you mentioned, of course, which is evaluated first. And I could see that yeah, that might need to let the hook override it, which shouldn't be too hard to do. I'm not super familiar with this aspect of the cruft pack code. That function is trying to avoid looking at objects that are in in-core kept packs, which implies repack feeding those via stdin. Looking at repack.c, we are feeding the existing packs we just wrote (so all of the reachable stuff). So I can see why we might skip those packs, but at the same time, we know we are keeping them (they were just written!) so they are not really cruft candidates anyway. If an extraCruftTips hook told us about them, would it matter? They are already being kept as non-cruft. But that is just an analysis based on 5 minutes of poking at the code. I won't be surprised if I'm misunderstanding it or missing subtle cases. -Peff
diff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt index d4c7c9d4e4..0b79bd1751 100644 --- a/Documentation/config/pack.txt +++ b/Documentation/config/pack.txt @@ -67,6 +67,17 @@ pack.deltaCacheLimit:: result once the best match for all objects is found. Defaults to 1000. Maximum value is 65535. +pack.extraCruftTips:: + When generating a cruft pack, use the shell to execute the + specified command(s), and interpret their output as additional + tips of objects to keep in the cruft pack, regardless of their + age. ++ +Output must contain exactly one hex object ID per line, and nothing +else. Objects which cannot be found in the repository are ignored. +Multiple hooks are supported, but all must exit successfully, else no +cruft pack will be generated. + pack.threads:: Specifies the number of threads to spawn when searching for best delta matches. This requires that linkgit:git-pack-objects[1] diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index a5b466839b..79a29796fe 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -44,6 +44,7 @@ #include "pack-mtimes.h" #include "parse-options.h" #include "wrapper.h" +#include "run-command.h" /* * Objects we are going to pack are collected in the `to_pack` structure. @@ -3587,6 +3588,106 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs stop_progress(&progress_state); } +static int enumerate_extra_cruft_tips_1(struct rev_info *revs, const char *args) +{ + struct child_process cmd = CHILD_PROCESS_INIT; + struct strbuf buf = STRBUF_INIT; + FILE *out = NULL; + int ret = 0; + + cmd.use_shell = 1; + cmd.out = -1; + + strvec_push(&cmd.args, args); + strvec_pushv(&cmd.env, (const char **)local_repo_env); + + if (start_command(&cmd)) { + ret = -1; + goto done; + } + + out = xfdopen(cmd.out, "r"); + while (strbuf_getline_lf(&buf, out) != EOF) { + struct object_id oid; + struct object *obj; + const char *rest; + + if (parse_oid_hex(buf.buf, &oid, &rest) || *rest) { + ret = error(_("invalid extra cruft tip: '%s'"), buf.buf); + goto done; + } + + obj = parse_object(the_repository, &oid); + if (!obj) + continue; + + display_progress(progress_state, ++nr_seen); + add_pending_object(revs, obj, ""); + } + + ret = finish_command(&cmd); + +done: + if (out) + fclose(out); + strbuf_release(&buf); + child_process_clear(&cmd); + + return ret; +} + +static int enumerate_extra_cruft_tips(void) +{ + struct rev_info revs; + const struct string_list *programs; + int ret = 0; + size_t i; + + if (git_config_get_string_multi("pack.extracrufttips", &programs)) + return ret; + + repo_init_revisions(the_repository, &revs, NULL); + + revs.tag_objects = 1; + revs.tree_objects = 1; + revs.blob_objects = 1; + + revs.include_check = cruft_include_check; + revs.include_check_obj = cruft_include_check_obj; + + revs.ignore_missing_links = 1; + + if (progress) + progress_state = start_progress(_("Enumerating extra cruft tips"), 0); + nr_seen = 0; + for (i = 0; i < programs->nr; i++) { + ret = enumerate_extra_cruft_tips_1(&revs, + programs->items[i].string); + if (ret) + break; + } + stop_progress(&progress_state); + + if (ret) + die(_("unable to enumerate additional cruft tips")); + + if (prepare_revision_walk(&revs)) + die(_("revision walk setup failed")); + if (progress) + progress_state = start_progress(_("Traversing extra cruft objects"), 0); + nr_seen = 0; + + /* + * Retain all objects reachable from extra tips via + * show_cruft_commit(), and show_cruft_object(), regardless of + * their mtime. + */ + traverse_commit_list(&revs, show_cruft_commit, show_cruft_object, NULL); + stop_progress(&progress_state); + + return ret; +} + static void read_cruft_objects(void) { struct strbuf buf = STRBUF_INIT; @@ -3642,6 +3743,8 @@ static void read_cruft_objects(void) else enumerate_cruft_objects(); + enumerate_extra_cruft_tips(); + strbuf_release(&buf); string_list_clear(&discard_packs, 0); string_list_clear(&fresh_packs, 0); diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh index 303f7a5d84..3ad16a9fca 100755 --- a/t/t5329-pack-objects-cruft.sh +++ b/t/t5329-pack-objects-cruft.sh @@ -739,4 +739,175 @@ test_expect_success 'cruft objects are freshend via loose' ' ) ' +test_expect_success 'additional cruft tips may be specified via pack.extraCruftTips' ' + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + + # Create a handful of objects. + # + # - one reachable commit, "base", designated for the reachable + # pack + # - one unreachable commit, "cruft.discard", which is marked + # for deletion + # - one unreachable commit, "cruft.old", which would be marked + # for deletion, but is rescued as an extra cruft tip + # - one unreachable commit, "cruft.new", which is not marked + # for deletion + test_commit base && + git branch -M main && + + git checkout --orphan discard && + git rm -fr . && + test_commit --no-tag cruft.discard && + + git checkout --orphan old && + git rm -fr . && + test_commit --no-tag cruft.old && + cruft_old="$(git rev-parse HEAD)" && + + git checkout --orphan new && + git rm -fr . && + test_commit --no-tag cruft.new && + cruft_new="$(git rev-parse HEAD)" && + + git checkout main && + git branch -D discard old new && + git reflog expire --all --expire=all && + + # mark cruft.old with an mtime that is many minutes + # older than the expiration period, and mark cruft.new + # with an mtime that is in the future (and thus not + # eligible for pruning). + test-tool chmtime -2000 "$objdir/$(test_oid_to_path $cruft_old)" && + test-tool chmtime +1000 "$objdir/$(test_oid_to_path $cruft_new)" && + + # Write the list of cruft objects we expect to + # accumulate, which is comprised of everything reachable + # from cruft.old and cruft.new, but not cruft.discard. + git rev-list --objects --no-object-names \ + $cruft_old $cruft_new >cruft.raw && + sort cruft.raw >cruft.expect && + + # Write the script to list extra tips, which are limited + # to cruft.old, in this case. + write_script extra-tips <<-EOF && + echo $cruft_old + EOF + git config pack.extraCruftTips ./extra-tips && + + git repack --cruft --cruft-expiration=now -d && + + mtimes="$(ls .git/objects/pack/pack-*.mtimes)" && + git show-index <${mtimes%.mtimes}.idx >cruft && + cut -d" " -f2 cruft | sort >cruft.actual && + test_cmp cruft.expect cruft.actual && + + # Ensure that the "old" objects are removed after + # dropping the pack.extraCruftTips hook. + git config --unset pack.extraCruftTips && + git repack --cruft --cruft-expiration=now -d && + + mtimes="$(ls .git/objects/pack/pack-*.mtimes)" && + git show-index <${mtimes%.mtimes}.idx >cruft && + cut -d" " -f2 cruft | sort >cruft.actual && + + git rev-list --objects --no-object-names $cruft_new >cruft.raw && + cp cruft.expect cruft.old && + sort cruft.raw >cruft.expect && + test_cmp cruft.expect cruft.actual && + + # ensure objects which are no longer in the cruft pack were + # removed from the repository + for object in $(comm -13 cruft.expect cruft.old) + do + test_must_fail git cat-file -t $object || return 1 + done + ) +' + +test_expect_success 'multi-valued pack.extraCruftTips' ' + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + + test_commit base && + git branch -M main && + + git checkout --orphan cruft.a && + git rm -fr . && + test_commit --no-tag cruft.a && + cruft_a="$(git rev-parse HEAD)" && + + git checkout --orphan cruft.b && + git rm -fr . && + test_commit --no-tag cruft.b && + cruft_b="$(git rev-parse HEAD)" && + + git checkout main && + git branch -D cruft.a cruft.b && + git reflog expire --all --expire=all && + + echo "echo $cruft_a" | write_script extra-tips.a && + echo "echo $cruft_b" | write_script extra-tips.b && + echo "false" | write_script extra-tips.c && + + git rev-list --objects --no-object-names $cruft_a $cruft_b \ + >cruft.raw && + sort cruft.raw >cruft.expect && + + # ensure that each extra cruft tip is saved by its + # respective hook + git config --add pack.extraCruftTips ./extra-tips.a && + git config --add pack.extraCruftTips ./extra-tips.b && + git repack --cruft --cruft-expiration=now -d && + + mtimes="$(ls .git/objects/pack/pack-*.mtimes)" && + git show-index <${mtimes%.mtimes}.idx >cruft && + cut -d" " -f2 cruft | sort >cruft.actual && + test_cmp cruft.expect cruft.actual && + + # ensure that a dirty exit halts cruft pack generation + git config --add pack.extraCruftTips ./extra-tips.c && + test_must_fail git repack --cruft -d 2>err && + grep "unable to enumerate additional cruft tips" err && + + # and that the existing cruft pack is left alone + test_path_is_file "$mtimes" + ) +' + +test_expect_success 'additional cruft blobs via pack.extraCruftTips' ' + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + + test_commit base && + + blob=$(echo "unreachable" | git hash-object -w --stdin) && + + # mark the unreachable blob we wrote above as having + # aged out of the retention period + test-tool chmtime -2000 "$objdir/$(test_oid_to_path $blob)" && + + # Write the script to list extra tips, which is just the + # extra blob as above. + write_script extra-tips <<-EOF && + echo $blob + EOF + git config pack.extraCruftTips ./extra-tips && + + git repack --cruft --cruft-expiration=now -d && + + mtimes="$(ls .git/objects/pack/pack-*.mtimes)" && + git show-index <${mtimes%.mtimes}.idx >cruft && + cut -d" " -f2 cruft >actual && + echo $blob >expect && + test_cmp expect actual + ) +' + test_done
This patch introduces a new multi-valued configuration option, `pack.extraCruftTips` as an alternative means to mark certain objects in the cruft pack as rescued, even if they would otherwise be pruned. Traditionally, cruft packs are generated in one of two ways: - When not pruning, all packed and loose objects which do not appear in the any of the packs which wil remain (as indicated over stdin, with `--cruft`) are packed separately into a cruft pack. - When pruning, the above process happens, with two additional tweaks. Instead of adding every object into the cruft pack, only objects which have mtime that is within the grace period are kept. In addition, a "rescuing" traversal is performed over the remaining cruft objects to keep around cruft objects which would have aged out of the repository, but are reachable from other cruft objects which have not yet aged out. This works well for repositories which have all of their "interesting" objects worth keeping around reachable via at least one reference. However, there is no option to be able to keep around certain objects that have otherwise aged out of the grace period. The only way to retain those objects is: - to point a reference at them, which may be undesirable or infeasible, - to track them via the reflog, which may be undesirable since the reflog's lifetime is limited to that of the reference it's tracking (and callers may want to keep those unreachable objects around for longer) - to extend the grace period, which may keep around other objects that the caller *does* want to discard, - or, to force the caller to construct the pack of objects they want to keep themselves, and then mark the pack as kept by adding a ".keep" file. This patch introduces a new configuration, `pack.extraCruftTips` which allows the caller to specify a program (or set of programs) whose output is treated as a set of objects to treat as "kept", even if they are unreachable and have aged out of the retention period. The implementation is straightforward: after determining the set of objects to pack into the cruft pack (either by calling `enumerate_cruft_objects()` or `enumerate_and_traverse_cruft_objects()`) we figure out if there are any program(s) we need to consult in order to determine if there are any objects which we need to "rescue". We then add those as tips to another reachability traversal, marking every object along the way as cruft (and thus retaining it in the cruft pack). A potential alternative here is to introduce a new mode to alter the contents of the reachable pack instead of the cruft one. One could imagine a new option to `pack-objects`, say `--extra-tips` that does the same thing as above, adding the visited set of objects along the traversal to the pack. But this has the unfortunate side-effect of altering the reachability closure of that pack. If parts of the unreachable object graph mentioned by one or more of the "extra tips" programs is not closed, then the resulting pack won't be either. This makes it impossible in the general case to write out reachability bitmaps for that pack, since closure is a requirement there. Instead, keep these unreachable objects in the cruft pack instead, to ensure that we can continue to have a pack containing just reachable objects, which is always safe to write a bitmap over. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Range-diff against v1: 1: 8af478ebe3 ! 1: 73ad7b90e1 builtin/pack-objects.c: introduce `pack.extraCruftTips` @@ Commit message - to point a reference at them, which may be undesirable or infeasible, + - to track them via the reflog, which may be undesirable since the + reflog's lifetime is limited to that of the reference it's tracking + (and callers may want to keep those unreachable objects around for + longer) - to extend the grace period, which may keep around other objects that the caller *does* want to discard, - or, to force the caller to construct the pack of objects they want @@ Documentation/config/pack.txt: pack.deltaCacheLimit:: ++ +Output must contain exactly one hex object ID per line, and nothing +else. Objects which cannot be found in the repository are ignored. ++Multiple hooks are supported, but all must exit successfully, else no ++cruft pack will be generated. + pack.threads:: Specifies the number of threads to spawn when searching for best @@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct + add_pending_object(revs, obj, ""); + } + ++ ret = finish_command(&cmd); ++ +done: + if (out) + fclose(out); @@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct + for (i = 0; i < programs->nr; i++) { + ret = enumerate_extra_cruft_tips_1(&revs, + programs->items[i].string); -+ if (!ret) ++ if (ret) + break; + } + stop_progress(&progress_state); @@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct + if (progress) + progress_state = start_progress(_("Traversing extra cruft objects"), 0); + nr_seen = 0; ++ ++ /* ++ * Retain all objects reachable from extra tips via ++ * show_cruft_commit(), and show_cruft_object(), regardless of ++ * their mtime. ++ */ + traverse_commit_list(&revs, show_cruft_commit, show_cruft_object, NULL); + stop_progress(&progress_state); + @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v + cruft_new="$(git rev-parse HEAD)" && + + git checkout main && -+ git branch -D old new && ++ git branch -D discard old new && + git reflog expire --all --expire=all && + + # mark cruft.old with an mtime that is many minutes @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v + mtimes="$(ls .git/objects/pack/pack-*.mtimes)" && + git show-index <${mtimes%.mtimes}.idx >cruft && + cut -d" " -f2 cruft | sort >cruft.actual && -+ test_cmp cruft.expect cruft.actual ++ test_cmp cruft.expect cruft.actual && ++ ++ # Ensure that the "old" objects are removed after ++ # dropping the pack.extraCruftTips hook. ++ git config --unset pack.extraCruftTips && ++ git repack --cruft --cruft-expiration=now -d && ++ ++ mtimes="$(ls .git/objects/pack/pack-*.mtimes)" && ++ git show-index <${mtimes%.mtimes}.idx >cruft && ++ cut -d" " -f2 cruft | sort >cruft.actual && ++ ++ git rev-list --objects --no-object-names $cruft_new >cruft.raw && ++ cp cruft.expect cruft.old && ++ sort cruft.raw >cruft.expect && ++ test_cmp cruft.expect cruft.actual && ++ ++ # ensure objects which are no longer in the cruft pack were ++ # removed from the repository ++ for object in $(comm -13 cruft.expect cruft.old) ++ do ++ test_must_fail git cat-file -t $object || return 1 ++ done ++ ) ++' ++ ++test_expect_success 'multi-valued pack.extraCruftTips' ' ++ git init repo && ++ test_when_finished "rm -fr repo" && ++ ( ++ cd repo && ++ ++ test_commit base && ++ git branch -M main && ++ ++ git checkout --orphan cruft.a && ++ git rm -fr . && ++ test_commit --no-tag cruft.a && ++ cruft_a="$(git rev-parse HEAD)" && ++ ++ git checkout --orphan cruft.b && ++ git rm -fr . && ++ test_commit --no-tag cruft.b && ++ cruft_b="$(git rev-parse HEAD)" && ++ ++ git checkout main && ++ git branch -D cruft.a cruft.b && ++ git reflog expire --all --expire=all && ++ ++ echo "echo $cruft_a" | write_script extra-tips.a && ++ echo "echo $cruft_b" | write_script extra-tips.b && ++ echo "false" | write_script extra-tips.c && ++ ++ git rev-list --objects --no-object-names $cruft_a $cruft_b \ ++ >cruft.raw && ++ sort cruft.raw >cruft.expect && ++ ++ # ensure that each extra cruft tip is saved by its ++ # respective hook ++ git config --add pack.extraCruftTips ./extra-tips.a && ++ git config --add pack.extraCruftTips ./extra-tips.b && ++ git repack --cruft --cruft-expiration=now -d && ++ ++ mtimes="$(ls .git/objects/pack/pack-*.mtimes)" && ++ git show-index <${mtimes%.mtimes}.idx >cruft && ++ cut -d" " -f2 cruft | sort >cruft.actual && ++ test_cmp cruft.expect cruft.actual && ++ ++ # ensure that a dirty exit halts cruft pack generation ++ git config --add pack.extraCruftTips ./extra-tips.c && ++ test_must_fail git repack --cruft -d 2>err && ++ grep "unable to enumerate additional cruft tips" err && ++ ++ # and that the existing cruft pack is left alone ++ test_path_is_file "$mtimes" + ) +' + Documentation/config/pack.txt | 11 +++ builtin/pack-objects.c | 103 ++++++++++++++++++++ t/t5329-pack-objects-cruft.sh | 171 ++++++++++++++++++++++++++++++++++ 3 files changed, 285 insertions(+)