| Message ID | 71f4800d36bcc77d8f36c5fc7b48eccbb90e6a93.1710706119.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refs: introduce `--auto` to pack refs as needed | expand |
Patrick Steinhardt <ps@pks.im> writes: > When running `git maintenance run --auto`, then the various subtasks > will only run as needed. Thus, we for example end up only packing loose > objects if we hit a certain threshold. > > Interestingly enough, the "pack-refs" task is actually _never_ executed > when the auto-flag is set because it does not have a condition at all. > As 41abfe15d9 (maintenance: add pack-refs task, 2021-02-09) mentions: > > The 'auto_condition' function pointer is left NULL for now. We could > extend this in the future to have a condition check if pack-refs > should be run during 'git maintenance run --auto'. > Ok, this answers my question in the previous email. > It is not quite clear from that quote whether it is actually intended > that the task doesn't run at all in this mode. Also, no test was added > to verify this behaviour. Ultimately though, it feels quite surprising > that `git maintenance run --auto --task=pack-refs` would quietly never > do anything at all. > > In any case, now that we do have the logic in place to let ref backends > decide whether or not to repack refs, it does make sense to wire it up > accordingly. With the "reftable" backend we will thus now perform > auto-compaction, which optimizes the refdb as needed. > > But for the "files" backend we now unconditionally pack refs as it does > not yet know to handle the "auto" flag. Arguably, this can be seen as a > bug fix given that previously the task never did anything at all. > Eventually though we should amend the "files" backend to use some > heuristics for auto compaction, as well. > Agreed, thanks for the clear explanation here.
On Wed, Mar 20, 2024 at 04:59:06PM -0700, Karthik Nayak wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > When running `git maintenance run --auto`, then the various subtasks > > will only run as needed. Thus, we for example end up only packing loose > > objects if we hit a certain threshold. > > > > Interestingly enough, the "pack-refs" task is actually _never_ executed > > when the auto-flag is set because it does not have a condition at all. > > As 41abfe15d9 (maintenance: add pack-refs task, 2021-02-09) mentions: > > > > The 'auto_condition' function pointer is left NULL for now. We could > > extend this in the future to have a condition check if pack-refs > > should be run during 'git maintenance run --auto'. > > > > Ok, this answers my question in the previous email. > > > It is not quite clear from that quote whether it is actually intended > > that the task doesn't run at all in this mode. Also, no test was added > > to verify this behaviour. Ultimately though, it feels quite surprising > > that `git maintenance run --auto --task=pack-refs` would quietly never > > do anything at all. > > > > In any case, now that we do have the logic in place to let ref backends > > decide whether or not to repack refs, it does make sense to wire it up > > accordingly. With the "reftable" backend we will thus now perform > > auto-compaction, which optimizes the refdb as needed. > > > > But for the "files" backend we now unconditionally pack refs as it does > > not yet know to handle the "auto" flag. Arguably, this can be seen as a > > bug fix given that previously the task never did anything at all. > > Eventually though we should amend the "files" backend to use some > > heuristics for auto compaction, as well. > > > > Agreed, thanks for the clear explanation here. Thanks for your thorough review! Patrick
diff --git a/builtin/gc.c b/builtin/gc.c index bf1f2a621a..3c874b248b 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -206,6 +206,16 @@ struct maintenance_run_opts { enum schedule_priority schedule; }; +static int pack_refs_condition(void) +{ + /* + * The auto-repacking logic for refs is handled by the ref backends and + * exposed via `git pack-refs --auto`. We thus always return truish + * here and let the backend decide for us. + */ + return 1; +} + static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts) { struct child_process cmd = CHILD_PROCESS_INIT; @@ -1298,7 +1308,7 @@ static struct maintenance_task tasks[] = { [TASK_PACK_REFS] = { "pack-refs", maintenance_task_pack_refs, - NULL, + pack_refs_condition, }, }; diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh index 219a495451..7d4ab0b91a 100755 --- a/t/t0601-reffiles-pack-refs.sh +++ b/t/t0601-reffiles-pack-refs.sh @@ -370,4 +370,14 @@ test_expect_success 'pack-refs does not drop broken refs during deletion' ' test_cmp expect actual ' +test_expect_success 'maintenance --auto unconditionally packs loose refs' ' + git update-ref refs/heads/something HEAD && + test_path_is_file .git/refs/heads/something && + git rev-parse refs/heads/something >expect && + git maintenance run --task=pack-refs --auto && + test_path_is_missing .git/refs/heads/something && + git rev-parse refs/heads/something >actual && + test_cmp expect actual +' + test_done diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index b37d8bf3b1..931d888bbb 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -387,7 +387,7 @@ test_expect_success 'pack-refs: compaction raises locking errors' ' test_cmp expect err ' -for command in pack-refs gc +for command in pack-refs gc "maintenance run --task=pack-refs" do test_expect_success "$command: auto compaction" ' test_when_finished "rm -rf repo" &&
When running `git maintenance run --auto`, then the various subtasks will only run as needed. Thus, we for example end up only packing loose objects if we hit a certain threshold. Interestingly enough, the "pack-refs" task is actually _never_ executed when the auto-flag is set because it does not have a condition at all. As 41abfe15d9 (maintenance: add pack-refs task, 2021-02-09) mentions: The 'auto_condition' function pointer is left NULL for now. We could extend this in the future to have a condition check if pack-refs should be run during 'git maintenance run --auto'. It is not quite clear from that quote whether it is actually intended that the task doesn't run at all in this mode. Also, no test was added to verify this behaviour. Ultimately though, it feels quite surprising that `git maintenance run --auto --task=pack-refs` would quietly never do anything at all. In any case, now that we do have the logic in place to let ref backends decide whether or not to repack refs, it does make sense to wire it up accordingly. With the "reftable" backend we will thus now perform auto-compaction, which optimizes the refdb as needed. But for the "files" backend we now unconditionally pack refs as it does not yet know to handle the "auto" flag. Arguably, this can be seen as a bug fix given that previously the task never did anything at all. Eventually though we should amend the "files" backend to use some heuristics for auto compaction, as well. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/gc.c | 12 +++++++++++- t/t0601-reffiles-pack-refs.sh | 10 ++++++++++ t/t0610-reftable-basics.sh | 2 +- 3 files changed, 22 insertions(+), 2 deletions(-)