| Message ID | 20210913181221.42635-4-chooglen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Use default values from settings instead of config | expand |
On Mon, Sep 13, 2021 at 11:12:21AM -0700, Glen Choo wrote: > builtin/gc.c has two ways of checking of multi-pack-index is enabled: > - git_config_get_bool() in incremental_repack_auto_condition() > - the_repository->settings.core_multi_pack_index in > maintenance_task_incremental_repack() Is this hinting at why there are no new tests added here? If so, it may need to be explained more clearly, since I was a tad confused after reading it. But if not, this patch message deserves an extra sentence or two saying why tests aren't necessary. Or if none of that is the case, and tests *are* necessary, then they should be added ;). Thanks, Taylor
On 14/09/21 01.12, Glen Choo wrote: > builtin/gc.c has two ways of checking of multi-pack-index is enabled: > - git_config_get_bool() in incremental_repack_auto_condition() > - the_repository->settings.core_multi_pack_index in > maintenance_task_incremental_repack() > > the_repository->settings.core_multi_pack_index should be preferred > because it has default values from prepare_repo_settings(). > > Standardize on the_repository->settings.core_multi_pack_index to check > if multi-pack-index is enabled. I was wonder what the correlations between checking if MIDX is enabled and performing incremental repack are. What are these?
On Mon, Sep 13, 2021 at 03:37:26PM -0400, Taylor Blau wrote: > > builtin/gc.c has two ways of checking of multi-pack-index is enabled: > > - git_config_get_bool() in incremental_repack_auto_condition() > > - the_repository->settings.core_multi_pack_index in > > maintenance_task_incremental_repack() > > Is this hinting at why there are no new tests added here? If so, it may > need to be explained more clearly, since I was a tad confused after > reading it. Looks like I'll need to be clearer; this wasn't my intention at all. I was hoping to describe the current state of affairs and to show that there are two different approaches. Thus, this patch is an attempt to 'standardize' the two approaches. > But if not, this patch message deserves an extra sentence or > two saying why tests aren't necessary. > > Or if none of that is the case, and tests *are* necessary, then they > should be added ;). I initially did not include tests because it *seemed* to me that there were no tests for this. But on a second pass, it looks like that assumption was completely wrong (the tests are in t7900-maintenance.sh). So, tests are necessary, and so I will add them :)
On Tue, Sep 14, 2021 at 11:00:34AM +0700, Bagas Sanjaya wrote: > I was wonder what the correlations between checking if MIDX is enabled and > performing incremental repack are. What are these? (I'm not an expert on the issue, so don't take what I say as definitive :)) Here's what I was able to find in Documentation/git-maintenance.txt: The `incremental-repack` job repacks the object directory using the `multi-pack-index` feature.[...] My guess at the intended behavior is that disabling midx should make git-maintenance behave as if it were unaware of midx features, such as incremental repack. This is quite akin to the documented behavior of fsck in this patch series, where fsck only verifies midx and commit-graphs when the respective features are enabled.
diff --git a/builtin/gc.c b/builtin/gc.c index f05d2f0a1a..070b7dccb1 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1051,12 +1051,11 @@ static int maintenance_task_loose_objects(struct maintenance_run_opts *opts) static int incremental_repack_auto_condition(void) { struct packed_git *p; - int enabled; int incremental_repack_auto_limit = 10; int count = 0; - if (git_config_get_bool("core.multiPackIndex", &enabled) || - !enabled) + prepare_repo_settings(the_repository); + if (!the_repository->settings.core_multi_pack_index) return 0; git_config_get_int("maintenance.incremental-repack.auto",
builtin/gc.c has two ways of checking of multi-pack-index is enabled: - git_config_get_bool() in incremental_repack_auto_condition() - the_repository->settings.core_multi_pack_index in maintenance_task_incremental_repack() the_repository->settings.core_multi_pack_index should be preferred because it has default values from prepare_repo_settings(). Standardize on the_repository->settings.core_multi_pack_index to check if multi-pack-index is enabled. Signed-off-by: Glen Choo <chooglen@google.com> --- builtin/gc.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)