commit-graph: new Bloom filter version that fixes murmur3
The murmur3 implementation in bloom.c has a bug when converting series of 4 bytes into network-order integers when char is signed (which is controllable by a compiler option, and the default signedness of char is platform-specific). When a string contains characters with the high bit set, this bug causes results that, although internally consistent within Git, does not accord with other implementations of murmur3 (thus, the changed path filters wouldn't be readable by other off-the-shelf implementatios of murmur3) and even with Git binaries that were compiled with different signedness of char. This bug affects both how Git writes changed path filters to disk and how Git interprets changed path filters on disk. Therefore, introduce a new version (2) of changed path filters that corrects this problem. The existing version (1) is still supported and is still the default, but users should migrate away from it as soon as possible. Because this bug only manifests with characters that have the high bit set, it may be possible that some (or all) commits in a given repo would have the same changed path filter both before and after this fix is applied. However, in order to determine whether this is the case, the changed paths would first have to be computed, at which point it is not much more expensive to just compute a new changed path filter. So this patch does not include any mechanism to "salvage" changed path filters from repositories. There is also no "mixed" mode - for each invocation of Git, reading and writing changed path filters are done with the same version number; this version number may be explicitly stated (typically if the user knows which version they need) or automatically determined from the version of the existing changed path filters in the repository. There is a change in write_commit_graph(). graph_read_bloom_data() makes it possible for chunk_bloom_data to be non-NULL but bloom_filter_settings to be NULL, which causes a segfault later on. I produced such a segfault while developing this patch, but couldn't find a way to reproduce it neither after this complete patch (or before), but in any case it seemed like a good thing to include that might help future patch authors. The value in t0095 was obtained from another murmur3 implementation using the following Go source code: package main import "fmt" import "github.com/spaolacci/murmur3" func main() { fmt.Printf("%x\n", murmur3.Sum32([]byte("Hello world!"))) fmt.Printf("%x\n", murmur3.Sum32([]byte{0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff})) } Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau committed
Jun 25, 2024 at 13:40 UTC
ba5a81d52b9a4c4608b6a645d2cc6f508bcb66b4
7 files changed
+252
-15
Documentation/config/commitgraph.txt
+4
-1
@@ -15,7 +15,7 @@ commitGraph.readChangedPaths::
15
16
commitGraph.changedPathsVersion::
17
Specifies the version of the changed-path Bloom filters that Git will read and
18
- write. May be -1, 0 or 1. Note that values greater than 1 may be
18
+ write. May be -1, 0, 1, or 2. Note that values greater than 1 may be
19
incompatible with older versions of Git which do not yet understand
20
those versions. Use caution when operating in a mixed-version
21
environment.
@@ -31,4 +31,7 @@ filters when instructed to write.
31
If 1, Git will only read version 1 Bloom filters, and will write version 1
32
Bloom filters.
33
+
34
+If 2, Git will only read version 2 Bloom filters, and will write version 2
35
+Bloom filters.
36
++
37
See linkgit:git-commit-graph[1] for more information.
bloom.c
+66
-3
@@ -100,7 +100,64 @@ int load_bloom_filter_from_graph(struct commit_graph *g,
100
* Not considered to be cryptographically secure.
101
* Implemented as described in https://en.wikipedia.org/wiki/MurmurHash#Algorithm
102
*/
103
-uint32_t murmur3_seeded(uint32_t seed, const char *data, size_t len)
103
+uint32_t murmur3_seeded_v2(uint32_t seed, const char *data, size_t len)
104
+{
105
+ const uint32_t c1 = 0xcc9e2d51;
106
+ const uint32_t c2 = 0x1b873593;
107
+ const uint32_t r1 = 15;
108
+ const uint32_t r2 = 13;
109
+ const uint32_t m = 5;
110
+ const uint32_t n = 0xe6546b64;
111
+ int i;
112
+ uint32_t k1 = 0;
113
+ const char *tail;
114
+
115
+ int len4 = len / sizeof(uint32_t);
116
+
117
+ uint32_t k;
118
+ for (i = 0; i < len4; i++) {
119
+ uint32_t byte1 = (uint32_t)(unsigned char)data[4*i];
120
+ uint32_t byte2 = ((uint32_t)(unsigned char)data[4*i + 1]) << 8;
121
+ uint32_t byte3 = ((uint32_t)(unsigned char)data[4*i + 2]) << 16;
122
+ uint32_t byte4 = ((uint32_t)(unsigned char)data[4*i + 3]) << 24;
123
+ k = byte1 | byte2 | byte3 | byte4;
124
+ k *= c1;
125
+ k = rotate_left(k, r1);
126
+ k *= c2;
127
+
128
+ seed ^= k;
129
+ seed = rotate_left(seed, r2) * m + n;
130
+ }
131
+
132
+ tail = (data + len4 * sizeof(uint32_t));
133
+
134
+ switch (len & (sizeof(uint32_t) - 1)) {
135
+ case 3:
136
+ k1 ^= ((uint32_t)(unsigned char)tail[2]) << 16;
137
+ /*-fallthrough*/
138
+ case 2:
139
+ k1 ^= ((uint32_t)(unsigned char)tail[1]) << 8;
140
+ /*-fallthrough*/
141
+ case 1:
142
+ k1 ^= ((uint32_t)(unsigned char)tail[0]) << 0;
143
+ k1 *= c1;
144
+ k1 = rotate_left(k1, r1);
145
+ k1 *= c2;
146
+ seed ^= k1;
147
+ break;
148
+ }
149
+
150
+ seed ^= (uint32_t)len;
151
+ seed ^= (seed >> 16);
152
+ seed *= 0x85ebca6b;
153
+ seed ^= (seed >> 13);
154
+ seed *= 0xc2b2ae35;
155
+ seed ^= (seed >> 16);
156
+
157
+ return seed;
158
+}
159
+
160
+static uint32_t murmur3_seeded_v1(uint32_t seed, const char *data, size_t len)
161
{
162
const uint32_t c1 = 0xcc9e2d51;
163
const uint32_t c2 = 0x1b873593;
@@ -165,8 +222,14 @@ void fill_bloom_key(const char *data,
222
int i;
223
const uint32_t seed0 = 0x293ae76f;
224
const uint32_t seed1 = 0x7e646e2c;
168
- const uint32_t hash0 = murmur3_seeded(seed0, data, len);
169
- const uint32_t hash1 = murmur3_seeded(seed1, data, len);
225
+ uint32_t hash0, hash1;
226
+ if (settings->hash_version == 2) {
227
+ hash0 = murmur3_seeded_v2(seed0, data, len);
228
+ hash1 = murmur3_seeded_v2(seed1, data, len);
229
+ } else {
230
+ hash0 = murmur3_seeded_v1(seed0, data, len);
231
+ hash1 = murmur3_seeded_v1(seed1, data, len);
232
+ }
233
234
key->hashes = (uint32_t *)xcalloc(settings->num_hashes, sizeof(uint32_t));
235
for (i = 0; i < settings->num_hashes; i++)
bloom.h
+5
-3
@@ -8,9 +8,11 @@ struct commit_graph;
8
struct bloom_filter_settings {
9
/*
10
* The version of the hashing technique being used.
11
- * We currently only support version = 1 which is
11
+ * The newest version is 2, which is
12
* the seeded murmur3 hashing technique implemented
13
- * in bloom.c.
13
+ * in bloom.c. Bloom filters of version 1 were created
14
+ * with prior versions of Git, which had a bug in the
15
+ * implementation of the hash function.
16
*/
17
uint32_t hash_version;
18
@@ -81,7 +83,7 @@ int load_bloom_filter_from_graph(struct commit_graph *g,
83
* Not considered to be cryptographically secure.
84
* Implemented as described in https://en.wikipedia.org/wiki/MurmurHash#Algorithm
85
*/
84
-uint32_t murmur3_seeded(uint32_t seed, const char *data, size_t len);
86
+uint32_t murmur3_seeded_v2(uint32_t seed, const char *data, size_t len);
87
88
void fill_bloom_key(const char *data,
89
size_t len,
commit-graph.c
+9
-4
@@ -344,7 +344,6 @@ static int graph_read_bloom_data(const unsigned char *chunk_start,
344
size_t chunk_size, void *data)
345
{
346
struct commit_graph *g = data;
347
- uint32_t hash_version;
347
348
if (chunk_size < BLOOMDATA_CHUNK_HEADER_SIZE) {
349
warning(_("ignoring too-small changed-path chunk"
@@ -356,10 +355,9 @@ static int graph_read_bloom_data(const unsigned char *chunk_start,
355
356
g->chunk_bloom_data = chunk_start;
357
g->chunk_bloom_data_size = chunk_size;
359
- hash_version = get_be32(chunk_start);
358
359
g->bloom_filter_settings = xmalloc(sizeof(struct bloom_filter_settings));
362
- g->bloom_filter_settings->hash_version = hash_version;
360
+ g->bloom_filter_settings->hash_version = get_be32(chunk_start);
361
g->bloom_filter_settings->num_hashes = get_be32(chunk_start + 4);
362
g->bloom_filter_settings->bits_per_entry = get_be32(chunk_start + 8);
363
g->bloom_filter_settings->max_changed_paths = DEFAULT_BLOOM_MAX_CHANGES;
@@ -2498,6 +2496,13 @@ int write_commit_graph(struct object_directory *odb,
2496
}
2497
if (!commit_graph_compatible(r))
2498
return 0;
2499
+ if (r->settings.commit_graph_changed_paths_version < -1
2500
+ || r->settings.commit_graph_changed_paths_version > 2) {
2501
+ warning(_("attempting to write a commit-graph, but "
2502
+ "'commitGraph.changedPathsVersion' (%d) is not supported"),
2503
+ r->settings.commit_graph_changed_paths_version);
2504
+ return 0;
2505
+ }
2506
2507
CALLOC_ARRAY(ctx, 1);
2508
ctx->r = r;
@@ -2540,7 +2545,7 @@ int write_commit_graph(struct object_directory *odb,
2545
g = ctx->r->objects->commit_graph;
2546
2547
/* We have changed-paths already. Keep them in the next graph */
2543
- if (g && g->chunk_bloom_data) {
2548
+ if (g && g->bloom_filter_settings) {
2549
ctx->changed_paths = 1;
2550
2551
/* don't propagate the hash_version unless unspecified */
t/helper/test-bloom.c
+8
-1
@@ -49,6 +49,7 @@ static void get_bloom_filter_for_commit(const struct object_id *commit_oid)
49
50
static const char *bloom_usage = "\n"
51
" test-tool bloom get_murmur3 <string>\n"
52
+" test-tool bloom get_murmur3_seven_highbit\n"
53
" test-tool bloom generate_filter <string> [<string>...]\n"
54
" test-tool bloom get_filter_for_commit <commit-hex>\n";
55
@@ -63,7 +64,13 @@ int cmd__bloom(int argc, const char **argv)
64
uint32_t hashed;
65
if (argc < 3)
66
usage(bloom_usage);
66
- hashed = murmur3_seeded(0, argv[2], strlen(argv[2]));
67
+ hashed = murmur3_seeded_v2(0, argv[2], strlen(argv[2]));
68
+ printf("Murmur3 Hash with seed=0:0x%08x\n", hashed);
69
+ }
70
+
71
+ if (!strcmp(argv[1], "get_murmur3_seven_highbit")) {
72
+ uint32_t hashed;
73
+ hashed = murmur3_seeded_v2(0, "\x99\xaa\xbb\xcc\xdd\xee\xff", 7);
74
printf("Murmur3 Hash with seed=0:0x%08x\n", hashed);
75
}
76
t/t0095-bloom.sh
+8
@@ -29,6 +29,14 @@ test_expect_success 'compute unseeded murmur3 hash for test string 2' '
29
test_cmp expect actual
30
'
31
32
+test_expect_success 'compute unseeded murmur3 hash for test string 3' '
33
+ cat >expect <<-\EOF &&
34
+ Murmur3 Hash with seed=0:0xa183ccfd
35
+ EOF
36
+ test-tool bloom get_murmur3_seven_highbit >actual &&
37
+ test_cmp expect actual
38
+'
39
+
40
test_expect_success 'compute bloom key for empty string' '
41
cat >expect <<-\EOF &&
42
Hashes:0x5615800c|0x5b966560|0x61174ab4|0x66983008|0x6c19155c|0x7199fab0|0x771ae004|
t/t4216-log-bloom.sh
+152
-3
@@ -488,14 +488,49 @@ test_expect_success 'merge graph layers with incompatible Bloom settings' '
488
test_must_be_empty err
489
'
490
491
+# chosen to be the same under all Unicode normalization forms
492
+CENT=$(printf "\302\242")
493
+
494
+test_expect_success 'ensure Bloom filter with incompatible versions are ignored' '
495
+ rm "$repo/$graph" &&
496
+
497
+ git -C $repo log --oneline --no-decorate -- $CENT >expect &&
498
+
499
+ # Compute v1 Bloom filters for commits at the bottom.
500
+ git -C $repo rev-parse HEAD^ >in &&
501
+ git -C $repo commit-graph write --stdin-commits --changed-paths \
502
+ --split <in &&
503
+
504
+ # Compute v2 Bloomfilters for the rest of the commits at the top.
505
+ git -C $repo rev-parse HEAD >in &&
506
+ git -C $repo -c commitGraph.changedPathsVersion=2 commit-graph write \
507
+ --stdin-commits --changed-paths --split=no-merge <in &&
508
+
509
+ test_line_count = 2 $repo/$chain &&
510
+
511
+ git -C $repo log --oneline --no-decorate -- $CENT >actual 2>err &&
512
+ test_cmp expect actual &&
513
+
514
+ layer="$(head -n 1 $repo/$chain)" &&
515
+ cat >expect.err <<-EOF &&
516
+ warning: disabling Bloom filters for commit-graph layer $SQ$layer$SQ due to incompatible settings
517
+ EOF
518
+ test_cmp expect.err err &&
519
+
520
+ # Merge the two layers with incompatible bloom filter versions,
521
+ # ensuring that the v2 filters are used.
522
+ >trace2.txt &&
523
+ GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
524
+ git -C $repo -c commitGraph.changedPathsVersion=2 commit-graph write --reachable --changed-paths 2>err &&
525
+ grep "disabling Bloom filters for commit-graph layer .$layer." err &&
526
+ grep "{\"hash_version\":2,\"num_hashes\":7,\"bits_per_entry\":10,\"max_changed_paths\":512" trace2.txt
527
+'
528
+
529
get_first_changed_path_filter () {
530
test-tool read-graph bloom-filters >filters.dat &&
531
head -n 1 filters.dat
532
}
533
496
-# chosen to be the same under all Unicode normalization forms
497
-CENT=$(printf "\302\242")
498
-
534
test_expect_success 'set up repo with high bit path, version 1 changed-path' '
535
git init highbit1 &&
536
test_commit -C highbit1 c1 "$CENT" &&
@@ -539,6 +574,120 @@ test_expect_success 'version 1 changed-path used when version 1 requested' '
574
)
575
'
576
577
+test_expect_success 'version 1 changed-path not used when version 2 requested' '
578
+ (
579
+ cd highbit1 &&
580
+ git config --add commitGraph.changedPathsVersion 2 &&
581
+ test_bloom_filters_not_used "-- another$CENT"
582
+ )
583
+'
584
+
585
+test_expect_success 'version 1 changed-path used when autodetect requested' '
586
+ (
587
+ cd highbit1 &&
588
+ git config --add commitGraph.changedPathsVersion -1 &&
589
+ test_bloom_filters_used "-- another$CENT"
590
+ )
591
+'
592
+
593
+test_expect_success 'when writing another commit graph, preserve existing version 1 of changed-path' '
594
+ test_commit -C highbit1 c1double "$CENT$CENT" &&
595
+ git -C highbit1 commit-graph write --reachable --changed-paths &&
596
+ (
597
+ cd highbit1 &&
598
+ git config --add commitGraph.changedPathsVersion -1 &&
599
+ echo "options: bloom(1,10,7) read_generation_data" >expect &&
600
+ test-tool read-graph >full &&
601
+ grep options full >actual &&
602
+ test_cmp expect actual
603
+ )
604
+'
605
+
606
+test_expect_success 'set up repo with high bit path, version 2 changed-path' '
607
+ git init highbit2 &&
608
+ git -C highbit2 config --add commitGraph.changedPathsVersion 2 &&
609
+ test_commit -C highbit2 c2 "$CENT" &&
610
+ git -C highbit2 commit-graph write --reachable --changed-paths
611
+'
612
+
613
+test_expect_success 'check value of version 2 changed-path' '
614
+ (
615
+ cd highbit2 &&
616
+ echo "c01f" >expect &&
617
+ get_first_changed_path_filter >actual &&
618
+ test_cmp expect actual
619
+ )
620
+'
621
+
622
+test_expect_success 'setup make another commit' '
623
+ # "git log" does not use Bloom filters for root commits - see how, in
624
+ # revision.c, rev_compare_tree() (the only code path that eventually calls
625
+ # get_bloom_filter()) is only called by try_to_simplify_commit() when the commit
626
+ # has one parent. Therefore, make another commit so that we perform the tests on
627
+ # a non-root commit.
628
+ test_commit -C highbit2 anotherc2 "another$CENT"
629
+'
630
+
631
+test_expect_success 'version 2 changed-path used when version 2 requested' '
632
+ (
633
+ cd highbit2 &&
634
+ test_bloom_filters_used "-- another$CENT"
635
+ )
636
+'
637
+
638
+test_expect_success 'version 2 changed-path not used when version 1 requested' '
639
+ (
640
+ cd highbit2 &&
641
+ git config --add commitGraph.changedPathsVersion 1 &&
642
+ test_bloom_filters_not_used "-- another$CENT"
643
+ )
644
+'
645
+
646
+test_expect_success 'version 2 changed-path used when autodetect requested' '
647
+ (
648
+ cd highbit2 &&
649
+ git config --add commitGraph.changedPathsVersion -1 &&
650
+ test_bloom_filters_used "-- another$CENT"
651
+ )
652
+'
653
+
654
+test_expect_success 'when writing another commit graph, preserve existing version 2 of changed-path' '
655
+ test_commit -C highbit2 c2double "$CENT$CENT" &&
656
+ git -C highbit2 commit-graph write --reachable --changed-paths &&
657
+ (
658
+ cd highbit2 &&
659
+ git config --add commitGraph.changedPathsVersion -1 &&
660
+ echo "options: bloom(2,10,7) read_generation_data" >expect &&
661
+ test-tool read-graph >full &&
662
+ grep options full >actual &&
663
+ test_cmp expect actual
664
+ )
665
+'
666
+
667
+test_expect_success 'when writing commit graph, do not reuse changed-path of another version' '
668
+ git init doublewrite &&
669
+ test_commit -C doublewrite c "$CENT" &&
670
+ git -C doublewrite config --add commitGraph.changedPathsVersion 1 &&
671
+ git -C doublewrite commit-graph write --reachable --changed-paths &&
672
+ for v in -2 3
673
+ do
674
+ git -C doublewrite config --add commitGraph.changedPathsVersion $v &&
675
+ git -C doublewrite commit-graph write --reachable --changed-paths 2>err &&
676
+ cat >expect <<-EOF &&
677
+ warning: attempting to write a commit-graph, but ${SQ}commitGraph.changedPathsVersion${SQ} ($v) is not supported
678
+ EOF
679
+ test_cmp expect err || return 1
680
+ done &&
681
+ git -C doublewrite config --add commitGraph.changedPathsVersion 2 &&
682
+ git -C doublewrite commit-graph write --reachable --changed-paths &&
683
+ (
684
+ cd doublewrite &&
685
+ echo "c01f" >expect &&
686
+ get_first_changed_path_filter >actual &&
687
+ test_cmp expect actual
688
+ )
689
+'
690
+
691
corrupt_graph () {
692
test_when_finished "rm -rf $graph" &&
693
git commit-graph write --reachable --changed-paths &&