On 7/6/2018 1:39 AM, Eric Sunshine wrote:
On Thu, Jul 5, 2018 at 8:54 PM Derrick Stolee <stolee@xxxxxxxxx> wrote:The core.multiPackIndex config setting controls the multi-pack- index (MIDX) feature. If false, the setting will disable all reads from the multi-pack-index file. Add comparison commands in t5319-multi-pack-index.sh to check typical Git behavior remains the same as the config setting is turned on and off. This currently includes 'git rev-list' and 'git log' commands to trigger several object database reads. Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx> --- diff --git a/cache.h b/cache.h @@ -814,6 +814,7 @@ extern char *git_replace_ref_base; +extern int core_multi_pack_index; diff --git a/config.c b/config.c @@ -1313,6 +1313,11 @@ static int git_default_core_config(const char *var, const char *value) + if (!strcmp(var, "core.multipackindex")) { + core_multi_pack_index = git_config_bool(var, value); + return 0; + }This is a rather unusual commit. This new configuration is assigned, but it's never actually consulted, which means that it's impossible for it to have any impact on functionality, yet tests are being added to check whether it _did_ have any impact on functionality. Confusing. Patch 17 does consult 'core_multi_pack_index', so it's only at that point that it could have any impact. This situation would be less confusing if you swapped patches 16 and 17 (and, of course, move the declaration of 'core_multi_pack_index' to patch 17 with a reasonable default value).
You're right that this commit is a bit too aware of the future, but I disagree with the recommendation to change it.
Yes, in this commit there is no possible way that these tests could fail. The point is that patches 17-23 all change behavior if this setting is on, and we want to make sure we do not break at any point along that journey (or in future iterations of the multi-pack-index feature).
With this in mind, I don't think there is a better commit to place these tests.
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh @@ -61,12 +63,42 @@ test_expect_success 'write midx with one v1 pack' ' +midx_git_two_modes() { + git -c core.multiPackIndex=false $1 >expect && + git -c core.multiPackIndex=true $1 >actual && + test_cmp expect actual +} + +compare_results_with_midx() { + MSG=$1 + test_expect_success "check normal git operations: $MSG" ' + midx_git_two_modes "rev-list --objects --all" && + midx_git_two_modes "log --raw" + ' +}Here, you define midx_git_two_modes() and compare_results_with_midx()...test_expect_success 'write midx with one v2 pack' ' - git pack-objects --index-version=2,0x40 pack/test <obj-list && - git multi-pack-index --object-dir=. write && - midx_read_expect 1 17 4 . + git pack-objects --index-version=2,0x40 $objdir/pack/test <obj-list && + git multi-pack-index --object-dir=$objdir write && + midx_read_expect 1 17 4 $objdir ' +midx_git_two_modes() { + git -c core.multiPackIndex=false $1 >expect && + git -c core.multiPackIndex=true $1 >actual && + test_cmp expect actual +} + +compare_results_with_midx() { + MSG=$1 + test_expect_success "check normal git operations: $MSG" ' + midx_git_two_modes "rev-list --objects --all" && + midx_git_two_modes "log --raw" + ' +}... and here you define both functions again.
This was a mistake. Thanks for catching it. Thanks, -Stolee
