Re: [PATCH v3 16/24] config: create core.multiPackIndex setting
SZEDER Gábor writes: >> Thanks for this point. It seems to work using >> "link:technical/multi-pack-index[1]", which is what I'll use in the next >> version. > > It doesn't work, it merely works around the build failure. Sorry. I fell into the same trap X-<. link:techincal/multi-pack-index.html[the technical documentation for it] or something like that, perhaps.
Re: [PATCH v3 16/24] config: create core.multiPackIndex setting
On 7/12/2018 9:19 AM, Derrick Stolee wrote:
On 7/6/2018 1:39 AM, Eric Sunshine wrote:
On Thu, Jul 5, 2018 at 8:54 PM Derrick Stolee 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
---
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.
Of course, as I convert this global config variable into an on-demand
check as promised [1] this commit seems even more trivial. I'm going to
squash it with PATCH 17.
[1]
https://public-inbox.org/git/b5733625-29c8-4317-ff44-d27c2fca1...@gmail.com/
Re: [PATCH v3 16/24] config: create core.multiPackIndex setting
On 7/12/2018 9:31 AM, SZEDER Gábor wrote: On Thu, Jul 12, 2018 at 3:01 PM Derrick Stolee wrote: On 7/11/2018 5:48 AM, SZEDER Gábor wrote: diff --git a/Documentation/config.txt b/Documentation/config.txt index ab641bf5a9..ab895ebb32 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -908,6 +908,10 @@ core.commitGraph:: Enable git commit graph feature. Allows reading from the commit-graph file. +core.multiPackIndex:: +Use the multi-pack-index file to track multiple packfiles using a +single index. See linkgit:technical/multi-pack-index[1]. The 'linkgit' macro should be used to create links to other man pages, but 'technical/multi-pack-index' is not a man page and this causes 'make check-docs' to complain: LINT lint-docs ./config.txt:929: nongit link: technical/multi-pack-index[1] Makefile:456: recipe for target 'lint-docs' failed make[1]: *** [lint-docs] Error 1 Thanks for this point. It seems to work using "link:technical/multi-pack-index[1]", which is what I'll use in the next version. It doesn't work, it merely works around the build failure. The generated man page looks like this: core.multiPackIndex Use the multi-pack-index file to track multiple packfiles using a single index. See 1[1]. And the resulting html page looks similar: core.multiPackIndex Use the multi-pack-index file to track multiple packfiles using a single index. See 1. where that "1" is a link pointing to the non-existing URL file:///home/me/src/git/Documentation/technical/multi-pack-index Right. Sorry. I also see that I use the correct kind of links in Documentation/git-multi-pack-index.txt (see below) so I will use it here, too. SEE ALSO See link:technical/multi-pack-index.html[The Multi-Pack-Index Design Document] and link:technical/pack-format.html[The Multi-Pack-Index Format] for more information on the multi-pack-index feature. Thanks, -Stolee
Re: [PATCH v3 16/24] config: create core.multiPackIndex setting
On Thu, Jul 12, 2018 at 3:01 PM Derrick Stolee wrote: > > On 7/11/2018 5:48 AM, SZEDER Gábor wrote: > >> diff --git a/Documentation/config.txt b/Documentation/config.txt > >> index ab641bf5a9..ab895ebb32 100644 > >> --- a/Documentation/config.txt > >> +++ b/Documentation/config.txt > >> @@ -908,6 +908,10 @@ core.commitGraph:: > >> Enable git commit graph feature. Allows reading from the > >> commit-graph file. > >> > >> +core.multiPackIndex:: > >> +Use the multi-pack-index file to track multiple packfiles using a > >> +single index. See linkgit:technical/multi-pack-index[1]. > > The 'linkgit' macro should be used to create links to other man pages, > > but 'technical/multi-pack-index' is not a man page and this causes > > 'make check-docs' to complain: > > > >LINT lint-docs > >./config.txt:929: nongit link: technical/multi-pack-index[1] > >Makefile:456: recipe for target 'lint-docs' failed > >make[1]: *** [lint-docs] Error 1 > > > Thanks for this point. It seems to work using > "link:technical/multi-pack-index[1]", which is what I'll use in the next > version. It doesn't work, it merely works around the build failure. The generated man page looks like this: core.multiPackIndex Use the multi-pack-index file to track multiple packfiles using a single index. See 1[1]. And the resulting html page looks similar: core.multiPackIndex Use the multi-pack-index file to track multiple packfiles using a single index. See 1. where that "1" is a link pointing to the non-existing URL file:///home/me/src/git/Documentation/technical/multi-pack-index
Re: [PATCH v3 16/24] config: create core.multiPackIndex setting
On 7/6/2018 1:39 AM, Eric Sunshine wrote:
On Thu, Jul 5, 2018 at 8:54 PM Derrick Stolee 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
---
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 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
Re: [PATCH v3 16/24] config: create core.multiPackIndex setting
On 7/11/2018 5:48 AM, SZEDER Gábor wrote: diff --git a/Documentation/config.txt b/Documentation/config.txt index ab641bf5a9..ab895ebb32 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -908,6 +908,10 @@ core.commitGraph:: Enable git commit graph feature. Allows reading from the commit-graph file. +core.multiPackIndex:: + Use the multi-pack-index file to track multiple packfiles using a + single index. See linkgit:technical/multi-pack-index[1]. The 'linkgit' macro should be used to create links to other man pages, but 'technical/multi-pack-index' is not a man page and this causes 'make check-docs' to complain: LINT lint-docs ./config.txt:929: nongit link: technical/multi-pack-index[1] Makefile:456: recipe for target 'lint-docs' failed make[1]: *** [lint-docs] Error 1 Thanks for this point. It seems to work using "link:technical/multi-pack-index[1]", which is what I'll use in the next version. -Stolee
Re: [PATCH v3 16/24] config: create core.multiPackIndex setting
> diff --git a/Documentation/config.txt b/Documentation/config.txt > index ab641bf5a9..ab895ebb32 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -908,6 +908,10 @@ core.commitGraph:: > Enable git commit graph feature. Allows reading from the > commit-graph file. > > +core.multiPackIndex:: > + Use the multi-pack-index file to track multiple packfiles using a > + single index. See linkgit:technical/multi-pack-index[1]. The 'linkgit' macro should be used to create links to other man pages, but 'technical/multi-pack-index' is not a man page and this causes 'make check-docs' to complain: LINT lint-docs ./config.txt:929: nongit link: technical/multi-pack-index[1] Makefile:456: recipe for target 'lint-docs' failed make[1]: *** [lint-docs] Error 1 > + > core.sparseCheckout:: > Enable "sparse checkout" feature. See section "Sparse checkout" in > linkgit:git-read-tree[1] for more information.
Re: [PATCH v3 16/24] config: create core.multiPackIndex setting
On Thu, Jul 5, 2018 at 8:54 PM Derrick Stolee 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
> ---
> 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).
> 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 - git multi-pack-index --object-dir=. write &&
> - midx_read_expect 1 17 4 .
> + git pack-objects --index-version=2,0x40 $objdir/pack/test + 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.
> +
> +compare_results_with_midx "one v2 pack"
[PATCH v3 16/24] config: create core.multiPackIndex setting
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
---
Documentation/config.txt| 4 +++
cache.h | 1 +
config.c| 5
environment.c | 1 +
t/t5319-multi-pack-index.sh | 56 +++--
5 files changed, 58 insertions(+), 9 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf5a9..ab895ebb32 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -908,6 +908,10 @@ core.commitGraph::
Enable git commit graph feature. Allows reading from the
commit-graph file.
+core.multiPackIndex::
+ Use the multi-pack-index file to track multiple packfiles using a
+ single index. See linkgit:technical/multi-pack-index[1].
+
core.sparseCheckout::
Enable "sparse checkout" feature. See section "Sparse checkout" in
linkgit:git-read-tree[1] for more information.
diff --git a/cache.h b/cache.h
index 89a107a7f7..d12aa49710 100644
--- a/cache.h
+++ b/cache.h
@@ -814,6 +814,7 @@ extern char *git_replace_ref_base;
extern int fsync_object_files;
extern int core_preload_index;
extern int core_commit_graph;
+extern int core_multi_pack_index;
extern int core_apply_sparse_checkout;
extern int precomposed_unicode;
extern int protect_hfs;
diff --git a/config.c b/config.c
index fbbf0f8e9f..95d8da4243 100644
--- a/config.c
+++ b/config.c
@@ -1313,6 +1313,11 @@ static int git_default_core_config(const char *var,
const char *value)
return 0;
}
+ if (!strcmp(var, "core.multipackindex")) {
+ core_multi_pack_index = git_config_bool(var, value);
+ return 0;
+ }
+
if (!strcmp(var, "core.sparsecheckout")) {
core_apply_sparse_checkout = git_config_bool(var, value);
return 0;
diff --git a/environment.c b/environment.c
index 2a6de2330b..b9bc919cdb 100644
--- a/environment.c
+++ b/environment.c
@@ -67,6 +67,7 @@ enum object_creation_mode object_creation_mode =
OBJECT_CREATION_MODE;
char *notes_ref_name;
int grafts_replace_parents = 1;
int core_commit_graph;
+int core_multi_pack_index;
int core_apply_sparse_checkout;
int merge_log_config = -1;
int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index ae6c9d4d02..fc582c9a59 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -3,6 +3,8 @@
test_description='multi-pack-indexes'
. ./test-lib.sh
+objdir=.git/objects
+
midx_read_expect () {
NUM_PACKS=$1
NUM_OBJECTS=$2
@@ -61,12 +63,42 @@ test_expect_success 'write midx with one v1 pack' '
midx_read_expect 1 17 4 .
'
+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"
+ '
+}
+
test_expect_success 'write midx with one v2 pack' '
- git pack-objects --index-version=2,0x40 pack/test 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"
+ '
+}
+
+compare_results_with_midx "one v2 pack"
+
test_expect_success 'Add more objects' '
for i in $(test_seq 6 10)
do
@@ -92,11 +124,13 @@ test_expect_success 'Add more objects' '
'
test_expect_success 'write midx with two packs' '
- git pack-objects --index-version=1 pack/test-2 obj-list &&
git update-ref HEAD $commit &&
- git pack-objects --index-version=2 pack/test-pack []
corrupt_data() {
file=$1
--
2.18.0.118.gd4f65b8d14
