* [PATCH v5 05/17] t/helper/test-read-graph.c: extract `dump_graph_info()`
From: Taylor Blau @ 2024-01-16 22:09 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt, Jonathan Tan, SZEDER Gábor
In-Reply-To: <cover.1705442923.git.me@ttaylorr.com>
Prepare for the 'read-graph' test helper to perform other tasks besides
dumping high-level information about the commit-graph by extracting its
main routine into a separate function.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
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>
---
t/helper/test-read-graph.c | 31 ++++++++++++++++++-------------
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/t/helper/test-read-graph.c b/t/helper/test-read-graph.c
index 8c7a83f578..3375392f6c 100644
--- a/t/helper/test-read-graph.c
+++ b/t/helper/test-read-graph.c
@@ -5,20 +5,8 @@
#include "bloom.h"
#include "setup.h"
-int cmd__read_graph(int argc UNUSED, const char **argv UNUSED)
+static void dump_graph_info(struct commit_graph *graph)
{
- struct commit_graph *graph = NULL;
- struct object_directory *odb;
-
- setup_git_directory();
- odb = the_repository->objects->odb;
-
- prepare_repo_settings(the_repository);
-
- graph = read_commit_graph_one(the_repository, odb);
- if (!graph)
- return 1;
-
printf("header: %08x %d %d %d %d\n",
ntohl(*(uint32_t*)graph->data),
*(unsigned char*)(graph->data + 4),
@@ -57,6 +45,23 @@ int cmd__read_graph(int argc UNUSED, const char **argv UNUSED)
if (graph->topo_levels)
printf(" topo_levels");
printf("\n");
+}
+
+int cmd__read_graph(int argc UNUSED, const char **argv UNUSED)
+{
+ struct commit_graph *graph = NULL;
+ struct object_directory *odb;
+
+ setup_git_directory();
+ odb = the_repository->objects->odb;
+
+ prepare_repo_settings(the_repository);
+
+ graph = read_commit_graph_one(the_repository, odb);
+ if (!graph)
+ return 1;
+
+ dump_graph_info(graph);
UNLEAK(graph);
--
2.43.0.334.gd4dbce1db5.dirty
^ permalink raw reply related
* [PATCH v5 06/17] bloom.h: make `load_bloom_filter_from_graph()` public
From: Taylor Blau @ 2024-01-16 22:09 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt, Jonathan Tan, SZEDER Gábor
In-Reply-To: <cover.1705442923.git.me@ttaylorr.com>
Prepare for a future commit to use the load_bloom_filter_from_graph()
function directly to load specific Bloom filters out of the commit-graph
for manual inspection (to be used during tests).
Signed-off-by: Taylor Blau <me@ttaylorr.com>
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>
---
bloom.c | 6 +++---
bloom.h | 5 +++++
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/bloom.c b/bloom.c
index e529f7605c..401999ed3c 100644
--- a/bloom.c
+++ b/bloom.c
@@ -48,9 +48,9 @@ static int check_bloom_offset(struct commit_graph *g, uint32_t pos,
return -1;
}
-static int load_bloom_filter_from_graph(struct commit_graph *g,
- struct bloom_filter *filter,
- uint32_t graph_pos)
+int load_bloom_filter_from_graph(struct commit_graph *g,
+ struct bloom_filter *filter,
+ uint32_t graph_pos)
{
uint32_t lex_pos, start_index, end_index;
diff --git a/bloom.h b/bloom.h
index adde6dfe21..1e4f612d2c 100644
--- a/bloom.h
+++ b/bloom.h
@@ -3,6 +3,7 @@
struct commit;
struct repository;
+struct commit_graph;
struct bloom_filter_settings {
/*
@@ -68,6 +69,10 @@ struct bloom_key {
uint32_t *hashes;
};
+int load_bloom_filter_from_graph(struct commit_graph *g,
+ struct bloom_filter *filter,
+ uint32_t graph_pos);
+
/*
* Calculate the murmur3 32-bit hash value for the given data
* using the given seed.
--
2.43.0.334.gd4dbce1db5.dirty
^ permalink raw reply related
* [PATCH v5 07/17] t/helper/test-read-graph: implement `bloom-filters` mode
From: Taylor Blau @ 2024-01-16 22:09 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt, Jonathan Tan, SZEDER Gábor
In-Reply-To: <cover.1705442923.git.me@ttaylorr.com>
Implement a mode of the "read-graph" test helper to dump out the
hexadecimal contents of the Bloom filter(s) contained in a commit-graph.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
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>
---
t/helper/test-read-graph.c | 44 +++++++++++++++++++++++++++++++++-----
1 file changed, 39 insertions(+), 5 deletions(-)
diff --git a/t/helper/test-read-graph.c b/t/helper/test-read-graph.c
index 3375392f6c..da9ac8584d 100644
--- a/t/helper/test-read-graph.c
+++ b/t/helper/test-read-graph.c
@@ -47,10 +47,32 @@ static void dump_graph_info(struct commit_graph *graph)
printf("\n");
}
-int cmd__read_graph(int argc UNUSED, const char **argv UNUSED)
+static void dump_graph_bloom_filters(struct commit_graph *graph)
+{
+ uint32_t i;
+
+ for (i = 0; i < graph->num_commits + graph->num_commits_in_base; i++) {
+ struct bloom_filter filter = { 0 };
+ size_t j;
+
+ if (load_bloom_filter_from_graph(graph, &filter, i) < 0) {
+ fprintf(stderr, "missing Bloom filter for graph "
+ "position %"PRIu32"\n", i);
+ continue;
+ }
+
+ for (j = 0; j < filter.len; j++)
+ printf("%02x", filter.data[j]);
+ if (filter.len)
+ printf("\n");
+ }
+}
+
+int cmd__read_graph(int argc, const char **argv)
{
struct commit_graph *graph = NULL;
struct object_directory *odb;
+ int ret = 0;
setup_git_directory();
odb = the_repository->objects->odb;
@@ -58,12 +80,24 @@ int cmd__read_graph(int argc UNUSED, const char **argv UNUSED)
prepare_repo_settings(the_repository);
graph = read_commit_graph_one(the_repository, odb);
- if (!graph)
- return 1;
+ if (!graph) {
+ ret = 1;
+ goto done;
+ }
- dump_graph_info(graph);
+ if (argc <= 1)
+ dump_graph_info(graph);
+ else if (!strcmp(argv[1], "bloom-filters"))
+ dump_graph_bloom_filters(graph);
+ else {
+ fprintf(stderr, "unknown sub-command: '%s'\n", argv[1]);
+ ret = 1;
+ }
+done:
UNLEAK(graph);
- return 0;
+ return ret;
}
+
+
--
2.43.0.334.gd4dbce1db5.dirty
^ permalink raw reply related
* [PATCH v5 08/17] t4216: test changed path filters with high bit paths
From: Taylor Blau @ 2024-01-16 22:09 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt, Jonathan Tan, SZEDER Gábor
In-Reply-To: <cover.1705442923.git.me@ttaylorr.com>
Subsequent commits will teach Git another version of changed path
filter that has different behavior with paths that contain at least
one character with its high bit set, so test the existing behavior as
a baseline.
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>
---
t/t4216-log-bloom.sh | 51 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index 20b0cf0c0e..484dd093cd 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -485,6 +485,57 @@ test_expect_success 'merge graph layers with incompatible Bloom settings' '
test_must_be_empty err
'
+get_first_changed_path_filter () {
+ test-tool read-graph bloom-filters >filters.dat &&
+ head -n 1 filters.dat
+}
+
+# chosen to be the same under all Unicode normalization forms
+CENT=$(printf "\302\242")
+
+test_expect_success 'set up repo with high bit path, version 1 changed-path' '
+ git init highbit1 &&
+ test_commit -C highbit1 c1 "$CENT" &&
+ git -C highbit1 commit-graph write --reachable --changed-paths
+'
+
+test_expect_success 'setup check value of version 1 changed-path' '
+ (
+ cd highbit1 &&
+ echo "52a9" >expect &&
+ get_first_changed_path_filter >actual
+ )
+'
+
+# expect will not match actual if char is unsigned by default. Write the test
+# in this way, so that a user running this test script can still see if the two
+# files match. (It will appear as an ordinary success if they match, and a skip
+# if not.)
+if test_cmp highbit1/expect highbit1/actual
+then
+ test_set_prereq SIGNED_CHAR_BY_DEFAULT
+fi
+test_expect_success SIGNED_CHAR_BY_DEFAULT 'check value of version 1 changed-path' '
+ # Only the prereq matters for this test.
+ true
+'
+
+test_expect_success 'setup make another commit' '
+ # "git log" does not use Bloom filters for root commits - see how, in
+ # revision.c, rev_compare_tree() (the only code path that eventually calls
+ # get_bloom_filter()) is only called by try_to_simplify_commit() when the commit
+ # has one parent. Therefore, make another commit so that we perform the tests on
+ # a non-root commit.
+ test_commit -C highbit1 anotherc1 "another$CENT"
+'
+
+test_expect_success 'version 1 changed-path used when version 1 requested' '
+ (
+ cd highbit1 &&
+ test_bloom_filters_used "-- another$CENT"
+ )
+'
+
corrupt_graph () {
test_when_finished "rm -rf $graph" &&
git commit-graph write --reachable --changed-paths &&
--
2.43.0.334.gd4dbce1db5.dirty
^ permalink raw reply related
* [PATCH v5 09/17] repo-settings: introduce commitgraph.changedPathsVersion
From: Taylor Blau @ 2024-01-16 22:09 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt, Jonathan Tan, SZEDER Gábor
In-Reply-To: <cover.1705442923.git.me@ttaylorr.com>
A subsequent commit will introduce another version of the changed-path
filter in the commit graph file. In order to control which version to
write (and read), a config variable is needed.
Therefore, introduce this config variable. For forwards compatibility,
teach Git to not read commit graphs when the config variable
is set to an unsupported version. Because we teach Git this,
commitgraph.readChangedPaths is now redundant, so deprecate it and
define its behavior in terms of the config variable we introduce.
This commit does not change the behavior of writing (Git writes changed
path filters when explicitly instructed regardless of any config
variable), but a subsequent commit will restrict Git such that it will
only write when commitgraph.changedPathsVersion is a recognized value.
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>
---
Documentation/config/commitgraph.txt | 26 ++++++++++++++++++++++---
commit-graph.c | 5 +++--
oss-fuzz/fuzz-commit-graph.c | 2 +-
repo-settings.c | 6 +++++-
repository.h | 2 +-
t/t4216-log-bloom.sh | 29 +++++++++++++++++++++++++++-
6 files changed, 61 insertions(+), 9 deletions(-)
diff --git a/Documentation/config/commitgraph.txt b/Documentation/config/commitgraph.txt
index 30604e4a4c..e68cdededa 100644
--- a/Documentation/config/commitgraph.txt
+++ b/Documentation/config/commitgraph.txt
@@ -9,6 +9,26 @@ commitGraph.maxNewFilters::
commit-graph write` (c.f., linkgit:git-commit-graph[1]).
commitGraph.readChangedPaths::
- If true, then git will use the changed-path Bloom filters in the
- commit-graph file (if it exists, and they are present). Defaults to
- true. See linkgit:git-commit-graph[1] for more information.
+ Deprecated. Equivalent to commitGraph.changedPathsVersion=-1 if true, and
+ commitGraph.changedPathsVersion=0 if false. (If commitGraph.changedPathVersion
+ is also set, commitGraph.changedPathsVersion takes precedence.)
+
+commitGraph.changedPathsVersion::
+ Specifies the version of the changed-path Bloom filters that Git will read and
+ write. May be -1, 0 or 1. Note that values greater than 1 may be
+ incompatible with older versions of Git which do not yet understand
+ those versions. Use caution when operating in a mixed-version
+ environment.
++
+Defaults to -1.
++
+If -1, Git will use the version of the changed-path Bloom filters in the
+repository, defaulting to 1 if there are none.
++
+If 0, Git will not read any Bloom filters, and will write version 1 Bloom
+filters when instructed to write.
++
+If 1, Git will only read version 1 Bloom filters, and will write version 1
+Bloom filters.
++
+See linkgit:git-commit-graph[1] for more information.
diff --git a/commit-graph.c b/commit-graph.c
index 00113b0f62..91c98ebc6c 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -459,7 +459,7 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
graph->read_generation_data = 1;
}
- if (s->commit_graph_read_changed_paths) {
+ if (s->commit_graph_changed_paths_version) {
read_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,
graph_read_bloom_index, graph);
read_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,
@@ -555,7 +555,8 @@ static void validate_mixed_bloom_settings(struct commit_graph *g)
}
if (g->bloom_filter_settings->bits_per_entry != settings->bits_per_entry ||
- g->bloom_filter_settings->num_hashes != settings->num_hashes) {
+ g->bloom_filter_settings->num_hashes != settings->num_hashes ||
+ g->bloom_filter_settings->hash_version != settings->hash_version) {
g->chunk_bloom_indexes = NULL;
g->chunk_bloom_data = NULL;
FREE_AND_NULL(g->bloom_filter_settings);
diff --git a/oss-fuzz/fuzz-commit-graph.c b/oss-fuzz/fuzz-commit-graph.c
index 2992079dd9..325c0b991a 100644
--- a/oss-fuzz/fuzz-commit-graph.c
+++ b/oss-fuzz/fuzz-commit-graph.c
@@ -19,7 +19,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
* possible.
*/
the_repository->settings.commit_graph_generation_version = 2;
- the_repository->settings.commit_graph_read_changed_paths = 1;
+ the_repository->settings.commit_graph_changed_paths_version = 1;
g = parse_commit_graph(&the_repository->settings, (void *)data, size);
repo_clear(the_repository);
free_commit_graph(g);
diff --git a/repo-settings.c b/repo-settings.c
index 30cd478762..c821583fe5 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -23,6 +23,7 @@ void prepare_repo_settings(struct repository *r)
int value;
const char *strval;
int manyfiles;
+ int read_changed_paths;
if (!r->gitdir)
BUG("Cannot add settings for uninitialized repository");
@@ -53,7 +54,10 @@ void prepare_repo_settings(struct repository *r)
/* Commit graph config or default, does not cascade (simple) */
repo_cfg_bool(r, "core.commitgraph", &r->settings.core_commit_graph, 1);
repo_cfg_int(r, "commitgraph.generationversion", &r->settings.commit_graph_generation_version, 2);
- repo_cfg_bool(r, "commitgraph.readchangedpaths", &r->settings.commit_graph_read_changed_paths, 1);
+ repo_cfg_bool(r, "commitgraph.readchangedpaths", &read_changed_paths, 1);
+ repo_cfg_int(r, "commitgraph.changedpathsversion",
+ &r->settings.commit_graph_changed_paths_version,
+ read_changed_paths ? -1 : 0);
repo_cfg_bool(r, "gc.writecommitgraph", &r->settings.gc_write_commit_graph, 1);
repo_cfg_bool(r, "fetch.writecommitgraph", &r->settings.fetch_write_commit_graph, 0);
diff --git a/repository.h b/repository.h
index 5f18486f64..f71154e12c 100644
--- a/repository.h
+++ b/repository.h
@@ -29,7 +29,7 @@ struct repo_settings {
int core_commit_graph;
int commit_graph_generation_version;
- int commit_graph_read_changed_paths;
+ int commit_graph_changed_paths_version;
int gc_write_commit_graph;
int fetch_write_commit_graph;
int command_requires_full_index;
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index 484dd093cd..642b960893 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -435,7 +435,7 @@ test_expect_success 'setup for mixed Bloom setting tests' '
done
'
-test_expect_success 'ensure incompatible Bloom filters are ignored' '
+test_expect_success 'ensure Bloom filters with incompatible settings are ignored' '
# Compute Bloom filters with "unusual" settings.
git -C $repo rev-parse one >in &&
GIT_TEST_BLOOM_SETTINGS_NUM_HASHES=3 git -C $repo commit-graph write \
@@ -485,6 +485,33 @@ test_expect_success 'merge graph layers with incompatible Bloom settings' '
test_must_be_empty err
'
+test_expect_success 'ensure Bloom filter with incompatible versions are ignored' '
+ rm "$repo/$graph" &&
+
+ git -C $repo log --oneline --no-decorate -- $CENT >expect &&
+
+ # Compute v1 Bloom filters for commits at the bottom.
+ git -C $repo rev-parse HEAD^ >in &&
+ git -C $repo commit-graph write --stdin-commits --changed-paths \
+ --split <in &&
+
+ # Compute v2 Bloomfilters for the rest of the commits at the top.
+ git -C $repo rev-parse HEAD >in &&
+ git -C $repo -c commitGraph.changedPathsVersion=2 commit-graph write \
+ --stdin-commits --changed-paths --split=no-merge <in &&
+
+ test_line_count = 2 $repo/$chain &&
+
+ git -C $repo log --oneline --no-decorate -- $CENT >actual 2>err &&
+ test_cmp expect actual &&
+
+ layer="$(head -n 1 $repo/$chain)" &&
+ cat >expect.err <<-EOF &&
+ warning: disabling Bloom filters for commit-graph layer $SQ$layer$SQ due to incompatible settings
+ EOF
+ test_cmp expect.err err
+'
+
get_first_changed_path_filter () {
test-tool read-graph bloom-filters >filters.dat &&
head -n 1 filters.dat
--
2.43.0.334.gd4dbce1db5.dirty
^ permalink raw reply related
* [PATCH v5 10/17] commit-graph: new Bloom filter version that fixes murmur3
From: Taylor Blau @ 2024-01-16 22:09 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt, Jonathan Tan, SZEDER Gábor
In-Reply-To: <cover.1705442923.git.me@ttaylorr.com>
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>
---
Documentation/config/commitgraph.txt | 5 +-
bloom.c | 69 +++++++++++++++-
bloom.h | 8 +-
commit-graph.c | 37 +++++++--
t/helper/test-bloom.c | 9 ++-
t/t0095-bloom.sh | 8 ++
t/t4216-log-bloom.sh | 114 +++++++++++++++++++++++++++
7 files changed, 234 insertions(+), 16 deletions(-)
diff --git a/Documentation/config/commitgraph.txt b/Documentation/config/commitgraph.txt
index e68cdededa..7f8c9d6638 100644
--- a/Documentation/config/commitgraph.txt
+++ b/Documentation/config/commitgraph.txt
@@ -15,7 +15,7 @@ commitGraph.readChangedPaths::
commitGraph.changedPathsVersion::
Specifies the version of the changed-path Bloom filters that Git will read and
- write. May be -1, 0 or 1. Note that values greater than 1 may be
+ write. May be -1, 0, 1, or 2. Note that values greater than 1 may be
incompatible with older versions of Git which do not yet understand
those versions. Use caution when operating in a mixed-version
environment.
@@ -31,4 +31,7 @@ filters when instructed to write.
If 1, Git will only read version 1 Bloom filters, and will write version 1
Bloom filters.
+
+If 2, Git will only read version 2 Bloom filters, and will write version 2
+Bloom filters.
++
See linkgit:git-commit-graph[1] for more information.
diff --git a/bloom.c b/bloom.c
index 401999ed3c..e9361b1c1f 100644
--- a/bloom.c
+++ b/bloom.c
@@ -99,7 +99,64 @@ int load_bloom_filter_from_graph(struct commit_graph *g,
* Not considered to be cryptographically secure.
* Implemented as described in https://en.wikipedia.org/wiki/MurmurHash#Algorithm
*/
-uint32_t murmur3_seeded(uint32_t seed, const char *data, size_t len)
+uint32_t murmur3_seeded_v2(uint32_t seed, const char *data, size_t len)
+{
+ const uint32_t c1 = 0xcc9e2d51;
+ const uint32_t c2 = 0x1b873593;
+ const uint32_t r1 = 15;
+ const uint32_t r2 = 13;
+ const uint32_t m = 5;
+ const uint32_t n = 0xe6546b64;
+ int i;
+ uint32_t k1 = 0;
+ const char *tail;
+
+ int len4 = len / sizeof(uint32_t);
+
+ uint32_t k;
+ for (i = 0; i < len4; i++) {
+ uint32_t byte1 = (uint32_t)(unsigned char)data[4*i];
+ uint32_t byte2 = ((uint32_t)(unsigned char)data[4*i + 1]) << 8;
+ uint32_t byte3 = ((uint32_t)(unsigned char)data[4*i + 2]) << 16;
+ uint32_t byte4 = ((uint32_t)(unsigned char)data[4*i + 3]) << 24;
+ k = byte1 | byte2 | byte3 | byte4;
+ k *= c1;
+ k = rotate_left(k, r1);
+ k *= c2;
+
+ seed ^= k;
+ seed = rotate_left(seed, r2) * m + n;
+ }
+
+ tail = (data + len4 * sizeof(uint32_t));
+
+ switch (len & (sizeof(uint32_t) - 1)) {
+ case 3:
+ k1 ^= ((uint32_t)(unsigned char)tail[2]) << 16;
+ /*-fallthrough*/
+ case 2:
+ k1 ^= ((uint32_t)(unsigned char)tail[1]) << 8;
+ /*-fallthrough*/
+ case 1:
+ k1 ^= ((uint32_t)(unsigned char)tail[0]) << 0;
+ k1 *= c1;
+ k1 = rotate_left(k1, r1);
+ k1 *= c2;
+ seed ^= k1;
+ break;
+ }
+
+ seed ^= (uint32_t)len;
+ seed ^= (seed >> 16);
+ seed *= 0x85ebca6b;
+ seed ^= (seed >> 13);
+ seed *= 0xc2b2ae35;
+ seed ^= (seed >> 16);
+
+ return seed;
+}
+
+static uint32_t murmur3_seeded_v1(uint32_t seed, const char *data, size_t len)
{
const uint32_t c1 = 0xcc9e2d51;
const uint32_t c2 = 0x1b873593;
@@ -164,8 +221,14 @@ void fill_bloom_key(const char *data,
int i;
const uint32_t seed0 = 0x293ae76f;
const uint32_t seed1 = 0x7e646e2c;
- const uint32_t hash0 = murmur3_seeded(seed0, data, len);
- const uint32_t hash1 = murmur3_seeded(seed1, data, len);
+ uint32_t hash0, hash1;
+ if (settings->hash_version == 2) {
+ hash0 = murmur3_seeded_v2(seed0, data, len);
+ hash1 = murmur3_seeded_v2(seed1, data, len);
+ } else {
+ hash0 = murmur3_seeded_v1(seed0, data, len);
+ hash1 = murmur3_seeded_v1(seed1, data, len);
+ }
key->hashes = (uint32_t *)xcalloc(settings->num_hashes, sizeof(uint32_t));
for (i = 0; i < settings->num_hashes; i++)
diff --git a/bloom.h b/bloom.h
index 1e4f612d2c..138d57a86b 100644
--- a/bloom.h
+++ b/bloom.h
@@ -8,9 +8,11 @@ struct commit_graph;
struct bloom_filter_settings {
/*
* The version of the hashing technique being used.
- * We currently only support version = 1 which is
+ * The newest version is 2, which is
* the seeded murmur3 hashing technique implemented
- * in bloom.c.
+ * in bloom.c. Bloom filters of version 1 were created
+ * with prior versions of Git, which had a bug in the
+ * implementation of the hash function.
*/
uint32_t hash_version;
@@ -80,7 +82,7 @@ int load_bloom_filter_from_graph(struct commit_graph *g,
* Not considered to be cryptographically secure.
* Implemented as described in https://en.wikipedia.org/wiki/MurmurHash#Algorithm
*/
-uint32_t murmur3_seeded(uint32_t seed, const char *data, size_t len);
+uint32_t murmur3_seeded_v2(uint32_t seed, const char *data, size_t len);
void fill_bloom_key(const char *data,
size_t len,
diff --git a/commit-graph.c b/commit-graph.c
index 91c98ebc6c..22237e7dfc 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -340,10 +340,16 @@ static int graph_read_bloom_index(const unsigned char *chunk_start,
return 0;
}
+struct graph_read_bloom_data_context {
+ struct commit_graph *g;
+ int *commit_graph_changed_paths_version;
+};
+
static int graph_read_bloom_data(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
- struct commit_graph *g = data;
+ struct graph_read_bloom_data_context *c = data;
+ struct commit_graph *g = c->g;
uint32_t hash_version;
if (chunk_size < BLOOMDATA_CHUNK_HEADER_SIZE) {
@@ -354,13 +360,15 @@ static int graph_read_bloom_data(const unsigned char *chunk_start,
return -1;
}
+ hash_version = get_be32(chunk_start);
+
+ if (*c->commit_graph_changed_paths_version == -1)
+ *c->commit_graph_changed_paths_version = hash_version;
+ else if (hash_version != *c->commit_graph_changed_paths_version)
+ return 0;
+
g->chunk_bloom_data = chunk_start;
g->chunk_bloom_data_size = chunk_size;
- hash_version = get_be32(chunk_start);
-
- if (hash_version != 1)
- return 0;
-
g->bloom_filter_settings = xmalloc(sizeof(struct bloom_filter_settings));
g->bloom_filter_settings->hash_version = hash_version;
g->bloom_filter_settings->num_hashes = get_be32(chunk_start + 4);
@@ -460,10 +468,14 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
}
if (s->commit_graph_changed_paths_version) {
+ struct graph_read_bloom_data_context context = {
+ .g = graph,
+ .commit_graph_changed_paths_version = &s->commit_graph_changed_paths_version
+ };
read_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,
graph_read_bloom_index, graph);
read_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,
- graph_read_bloom_data, graph);
+ graph_read_bloom_data, &context);
}
if (graph->chunk_bloom_indexes && graph->chunk_bloom_data) {
@@ -2501,6 +2513,13 @@ int write_commit_graph(struct object_directory *odb,
}
if (!commit_graph_compatible(r))
return 0;
+ if (r->settings.commit_graph_changed_paths_version < -1
+ || r->settings.commit_graph_changed_paths_version > 2) {
+ warning(_("attempting to write a commit-graph, but "
+ "'commitgraph.changedPathsVersion' (%d) is not supported"),
+ r->settings.commit_graph_changed_paths_version);
+ return 0;
+ }
CALLOC_ARRAY(ctx, 1);
ctx->r = r;
@@ -2513,6 +2532,8 @@ int write_commit_graph(struct object_directory *odb,
ctx->write_generation_data = (get_configured_generation_version(r) == 2);
ctx->num_generation_data_overflows = 0;
+ bloom_settings.hash_version = r->settings.commit_graph_changed_paths_version == 2
+ ? 2 : 1;
bloom_settings.bits_per_entry = git_env_ulong("GIT_TEST_BLOOM_SETTINGS_BITS_PER_ENTRY",
bloom_settings.bits_per_entry);
bloom_settings.num_hashes = git_env_ulong("GIT_TEST_BLOOM_SETTINGS_NUM_HASHES",
@@ -2542,7 +2563,7 @@ int write_commit_graph(struct object_directory *odb,
g = ctx->r->objects->commit_graph;
/* We have changed-paths already. Keep them in the next graph */
- if (g && g->chunk_bloom_data) {
+ if (g && g->bloom_filter_settings) {
ctx->changed_paths = 1;
ctx->bloom_settings = g->bloom_filter_settings;
}
diff --git a/t/helper/test-bloom.c b/t/helper/test-bloom.c
index 1281e66876..eefc1668c7 100644
--- a/t/helper/test-bloom.c
+++ b/t/helper/test-bloom.c
@@ -49,6 +49,7 @@ static void get_bloom_filter_for_commit(const struct object_id *commit_oid)
static const char *bloom_usage = "\n"
" test-tool bloom get_murmur3 <string>\n"
+" test-tool bloom get_murmur3_seven_highbit\n"
" test-tool bloom generate_filter <string> [<string>...]\n"
" test-tool bloom get_filter_for_commit <commit-hex>\n";
@@ -63,7 +64,13 @@ int cmd__bloom(int argc, const char **argv)
uint32_t hashed;
if (argc < 3)
usage(bloom_usage);
- hashed = murmur3_seeded(0, argv[2], strlen(argv[2]));
+ hashed = murmur3_seeded_v2(0, argv[2], strlen(argv[2]));
+ printf("Murmur3 Hash with seed=0:0x%08x\n", hashed);
+ }
+
+ if (!strcmp(argv[1], "get_murmur3_seven_highbit")) {
+ uint32_t hashed;
+ hashed = murmur3_seeded_v2(0, "\x99\xaa\xbb\xcc\xdd\xee\xff", 7);
printf("Murmur3 Hash with seed=0:0x%08x\n", hashed);
}
diff --git a/t/t0095-bloom.sh b/t/t0095-bloom.sh
index b567383eb8..c8d84ab606 100755
--- a/t/t0095-bloom.sh
+++ b/t/t0095-bloom.sh
@@ -29,6 +29,14 @@ test_expect_success 'compute unseeded murmur3 hash for test string 2' '
test_cmp expect actual
'
+test_expect_success 'compute unseeded murmur3 hash for test string 3' '
+ cat >expect <<-\EOF &&
+ Murmur3 Hash with seed=0:0xa183ccfd
+ EOF
+ test-tool bloom get_murmur3_seven_highbit >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'compute bloom key for empty string' '
cat >expect <<-\EOF &&
Hashes:0x5615800c|0x5b966560|0x61174ab4|0x66983008|0x6c19155c|0x7199fab0|0x771ae004|
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index 642b960893..a7bf3a7dca 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -563,6 +563,120 @@ test_expect_success 'version 1 changed-path used when version 1 requested' '
)
'
+test_expect_success 'version 1 changed-path not used when version 2 requested' '
+ (
+ cd highbit1 &&
+ git config --add commitgraph.changedPathsVersion 2 &&
+ test_bloom_filters_not_used "-- another$CENT"
+ )
+'
+
+test_expect_success 'version 1 changed-path used when autodetect requested' '
+ (
+ cd highbit1 &&
+ git config --add commitgraph.changedPathsVersion -1 &&
+ test_bloom_filters_used "-- another$CENT"
+ )
+'
+
+test_expect_success 'when writing another commit graph, preserve existing version 1 of changed-path' '
+ test_commit -C highbit1 c1double "$CENT$CENT" &&
+ git -C highbit1 commit-graph write --reachable --changed-paths &&
+ (
+ cd highbit1 &&
+ git config --add commitgraph.changedPathsVersion -1 &&
+ echo "options: bloom(1,10,7) read_generation_data" >expect &&
+ test-tool read-graph >full &&
+ grep options full >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'set up repo with high bit path, version 2 changed-path' '
+ git init highbit2 &&
+ git -C highbit2 config --add commitgraph.changedPathsVersion 2 &&
+ test_commit -C highbit2 c2 "$CENT" &&
+ git -C highbit2 commit-graph write --reachable --changed-paths
+'
+
+test_expect_success 'check value of version 2 changed-path' '
+ (
+ cd highbit2 &&
+ echo "c01f" >expect &&
+ get_first_changed_path_filter >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'setup make another commit' '
+ # "git log" does not use Bloom filters for root commits - see how, in
+ # revision.c, rev_compare_tree() (the only code path that eventually calls
+ # get_bloom_filter()) is only called by try_to_simplify_commit() when the commit
+ # has one parent. Therefore, make another commit so that we perform the tests on
+ # a non-root commit.
+ test_commit -C highbit2 anotherc2 "another$CENT"
+'
+
+test_expect_success 'version 2 changed-path used when version 2 requested' '
+ (
+ cd highbit2 &&
+ test_bloom_filters_used "-- another$CENT"
+ )
+'
+
+test_expect_success 'version 2 changed-path not used when version 1 requested' '
+ (
+ cd highbit2 &&
+ git config --add commitgraph.changedPathsVersion 1 &&
+ test_bloom_filters_not_used "-- another$CENT"
+ )
+'
+
+test_expect_success 'version 2 changed-path used when autodetect requested' '
+ (
+ cd highbit2 &&
+ git config --add commitgraph.changedPathsVersion -1 &&
+ test_bloom_filters_used "-- another$CENT"
+ )
+'
+
+test_expect_success 'when writing another commit graph, preserve existing version 2 of changed-path' '
+ test_commit -C highbit2 c2double "$CENT$CENT" &&
+ git -C highbit2 commit-graph write --reachable --changed-paths &&
+ (
+ cd highbit2 &&
+ git config --add commitgraph.changedPathsVersion -1 &&
+ echo "options: bloom(2,10,7) read_generation_data" >expect &&
+ test-tool read-graph >full &&
+ grep options full >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'when writing commit graph, do not reuse changed-path of another version' '
+ git init doublewrite &&
+ test_commit -C doublewrite c "$CENT" &&
+ git -C doublewrite config --add commitgraph.changedPathsVersion 1 &&
+ git -C doublewrite commit-graph write --reachable --changed-paths &&
+ for v in -2 3
+ do
+ git -C doublewrite config --add commitgraph.changedPathsVersion $v &&
+ git -C doublewrite commit-graph write --reachable --changed-paths 2>err &&
+ cat >expect <<-EOF &&
+ warning: attempting to write a commit-graph, but ${SQ}commitgraph.changedPathsVersion${SQ} ($v) is not supported
+ EOF
+ test_cmp expect err || return 1
+ done &&
+ git -C doublewrite config --add commitgraph.changedPathsVersion 2 &&
+ git -C doublewrite commit-graph write --reachable --changed-paths &&
+ (
+ cd doublewrite &&
+ echo "c01f" >expect &&
+ get_first_changed_path_filter >actual &&
+ test_cmp expect actual
+ )
+'
+
corrupt_graph () {
test_when_finished "rm -rf $graph" &&
git commit-graph write --reachable --changed-paths &&
--
2.43.0.334.gd4dbce1db5.dirty
^ permalink raw reply related
* [PATCH v5 11/17] bloom: annotate filters with hash version
From: Taylor Blau @ 2024-01-16 22:09 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt, Jonathan Tan, SZEDER Gábor
In-Reply-To: <cover.1705442923.git.me@ttaylorr.com>
In subsequent commits, we will want to load existing Bloom filters out
of a commit-graph, even when the hash version they were computed with
does not match the value of `commitGraph.changedPathVersion`.
In order to differentiate between the two, add a "version" field to each
Bloom filter.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bloom.c | 11 ++++++++---
bloom.h | 1 +
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/bloom.c b/bloom.c
index e9361b1c1f..9284b88e95 100644
--- a/bloom.c
+++ b/bloom.c
@@ -88,6 +88,7 @@ int load_bloom_filter_from_graph(struct commit_graph *g,
filter->data = (unsigned char *)(g->chunk_bloom_data +
sizeof(unsigned char) * start_index +
BLOOMDATA_CHUNK_HEADER_SIZE);
+ filter->version = g->bloom_filter_settings->hash_version;
return 1;
}
@@ -273,11 +274,13 @@ static int pathmap_cmp(const void *hashmap_cmp_fn_data UNUSED,
return strcmp(e1->path, e2->path);
}
-static void init_truncated_large_filter(struct bloom_filter *filter)
+static void init_truncated_large_filter(struct bloom_filter *filter,
+ int version)
{
filter->data = xmalloc(1);
filter->data[0] = 0xFF;
filter->len = 1;
+ filter->version = version;
}
struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
@@ -362,13 +365,15 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
}
if (hashmap_get_size(&pathmap) > settings->max_changed_paths) {
- init_truncated_large_filter(filter);
+ init_truncated_large_filter(filter,
+ settings->hash_version);
if (computed)
*computed |= BLOOM_TRUNC_LARGE;
goto cleanup;
}
filter->len = (hashmap_get_size(&pathmap) * settings->bits_per_entry + BITS_PER_WORD - 1) / BITS_PER_WORD;
+ filter->version = settings->hash_version;
if (!filter->len) {
if (computed)
*computed |= BLOOM_TRUNC_EMPTY;
@@ -388,7 +393,7 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
} else {
for (i = 0; i < diff_queued_diff.nr; i++)
diff_free_filepair(diff_queued_diff.queue[i]);
- init_truncated_large_filter(filter);
+ init_truncated_large_filter(filter, settings->hash_version);
if (computed)
*computed |= BLOOM_TRUNC_LARGE;
diff --git a/bloom.h b/bloom.h
index 138d57a86b..330a140520 100644
--- a/bloom.h
+++ b/bloom.h
@@ -55,6 +55,7 @@ struct bloom_filter_settings {
struct bloom_filter {
unsigned char *data;
size_t len;
+ int version;
};
/*
--
2.43.0.334.gd4dbce1db5.dirty
^ permalink raw reply related
* [PATCH v5 12/17] bloom: prepare to discard incompatible Bloom filters
From: Taylor Blau @ 2024-01-16 22:09 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt, Jonathan Tan, SZEDER Gábor
In-Reply-To: <cover.1705442923.git.me@ttaylorr.com>
Callers use the inline `get_bloom_filter()` implementation as a thin
wrapper around `get_or_compute_bloom_filter()`. The former calls the
latter with a value of "0" for `compute_if_not_present`, making
`get_bloom_filter()` the default read-only path for fetching an existing
Bloom filter.
Callers expect the value returned from `get_bloom_filter()` is usable,
that is that it's compatible with the configured value corresponding to
`commitGraph.changedPathsVersion`.
This is OK, since the commit-graph machinery only initializes its BDAT
chunk (thereby enabling it to service Bloom filter queries) when the
Bloom filter hash_version is compatible with our settings. So any value
returned by `get_bloom_filter()` is trivially useable.
However, subsequent commits will load the BDAT chunk even when the Bloom
filters are built with incompatible hash versions. Prepare to handle
this by teaching `get_bloom_filter()` to discard filters that are
incompatible with the configured hash version.
Callers who wish to read incompatible filters (e.g., for upgrading
filters from v1 to v2) may use the lower level routine,
`get_or_compute_bloom_filter()`.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bloom.c | 20 +++++++++++++++++++-
bloom.h | 20 ++++++++++++++++++--
2 files changed, 37 insertions(+), 3 deletions(-)
diff --git a/bloom.c b/bloom.c
index 9284b88e95..323d8012b8 100644
--- a/bloom.c
+++ b/bloom.c
@@ -283,6 +283,23 @@ static void init_truncated_large_filter(struct bloom_filter *filter,
filter->version = version;
}
+struct bloom_filter *get_bloom_filter(struct repository *r, struct commit *c)
+{
+ struct bloom_filter *filter;
+ int hash_version;
+
+ filter = get_or_compute_bloom_filter(r, c, 0, NULL, NULL);
+ if (!filter)
+ return NULL;
+
+ prepare_repo_settings(r);
+ hash_version = r->settings.commit_graph_changed_paths_version;
+
+ if (!(hash_version == -1 || hash_version == filter->version))
+ return NULL; /* unusable filter */
+ return filter;
+}
+
struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
struct commit *c,
int compute_if_not_present,
@@ -308,7 +325,8 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
filter, graph_pos);
}
- if (filter->data && filter->len)
+ if ((filter->data && filter->len) &&
+ (!settings || settings->hash_version == filter->version))
return filter;
if (!compute_if_not_present)
return NULL;
diff --git a/bloom.h b/bloom.h
index 330a140520..bfe389e29c 100644
--- a/bloom.h
+++ b/bloom.h
@@ -110,8 +110,24 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
const struct bloom_filter_settings *settings,
enum bloom_filter_computed *computed);
-#define get_bloom_filter(r, c) get_or_compute_bloom_filter( \
- (r), (c), 0, NULL, NULL)
+/*
+ * Find the Bloom filter associated with the given commit "c".
+ *
+ * If any of the following are true
+ *
+ * - the repository does not have a commit-graph, or
+ * - the repository disables reading from the commit-graph, or
+ * - the given commit does not have a Bloom filter computed, or
+ * - there is a Bloom filter for commit "c", but it cannot be read
+ * because the filter uses an incompatible version of murmur3
+ *
+ * , then `get_bloom_filter()` will return NULL. Otherwise, the corresponding
+ * Bloom filter will be returned.
+ *
+ * For callers who wish to inspect Bloom filters with incompatible hash
+ * versions, use get_or_compute_bloom_filter().
+ */
+struct bloom_filter *get_bloom_filter(struct repository *r, struct commit *c);
int bloom_filter_contains(const struct bloom_filter *filter,
const struct bloom_key *key,
--
2.43.0.334.gd4dbce1db5.dirty
^ permalink raw reply related
* [PATCH v5 13/17] commit-graph.c: unconditionally load Bloom filters
From: Taylor Blau @ 2024-01-16 22:09 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt, Jonathan Tan, SZEDER Gábor
In-Reply-To: <cover.1705442923.git.me@ttaylorr.com>
In an earlier commit, we began ignoring the Bloom data ("BDAT") chunk
for commit-graphs whose Bloom filters were computed using a hash version
incompatible with the value of `commitGraph.changedPathVersion`.
Now that the Bloom API has been hardened to discard these incompatible
filters (with the exception of low-level APIs), we can safely load these
Bloom filters unconditionally.
We no longer want to return early from `graph_read_bloom_data()`, and
similarly do not want to set the bloom_settings' `hash_version` field as
a side-effect. The latter is because we want to wait until we know which
Bloom settings we're using (either the defaults, from the GIT_TEST
variables, or from the previous commit-graph layer) before deciding what
hash_version to use.
If we detect an existing BDAT chunk, we'll infer the rest of the
settings (e.g., number of hashes, bits per entry, and maximum number of
changed paths) from the earlier graph layer. The hash_version will be
inferred from the previous layer as well, unless one has already been
specified via configuration.
Once all of that is done, we normalize the value of the hash_version to
either "1" or "2".
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
commit-graph.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 22237e7dfc..a2063d5f91 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -362,11 +362,6 @@ static int graph_read_bloom_data(const unsigned char *chunk_start,
hash_version = get_be32(chunk_start);
- if (*c->commit_graph_changed_paths_version == -1)
- *c->commit_graph_changed_paths_version = hash_version;
- else if (hash_version != *c->commit_graph_changed_paths_version)
- return 0;
-
g->chunk_bloom_data = chunk_start;
g->chunk_bloom_data_size = chunk_size;
g->bloom_filter_settings = xmalloc(sizeof(struct bloom_filter_settings));
@@ -2532,8 +2527,7 @@ int write_commit_graph(struct object_directory *odb,
ctx->write_generation_data = (get_configured_generation_version(r) == 2);
ctx->num_generation_data_overflows = 0;
- bloom_settings.hash_version = r->settings.commit_graph_changed_paths_version == 2
- ? 2 : 1;
+ bloom_settings.hash_version = r->settings.commit_graph_changed_paths_version;
bloom_settings.bits_per_entry = git_env_ulong("GIT_TEST_BLOOM_SETTINGS_BITS_PER_ENTRY",
bloom_settings.bits_per_entry);
bloom_settings.num_hashes = git_env_ulong("GIT_TEST_BLOOM_SETTINGS_NUM_HASHES",
@@ -2565,10 +2559,18 @@ int write_commit_graph(struct object_directory *odb,
/* We have changed-paths already. Keep them in the next graph */
if (g && g->bloom_filter_settings) {
ctx->changed_paths = 1;
- ctx->bloom_settings = g->bloom_filter_settings;
+
+ /* don't propagate the hash_version unless unspecified */
+ if (bloom_settings.hash_version == -1)
+ bloom_settings.hash_version = g->bloom_filter_settings->hash_version;
+ bloom_settings.bits_per_entry = g->bloom_filter_settings->bits_per_entry;
+ bloom_settings.num_hashes = g->bloom_filter_settings->num_hashes;
+ bloom_settings.max_changed_paths = g->bloom_filter_settings->max_changed_paths;
}
}
+ bloom_settings.hash_version = bloom_settings.hash_version == 2 ? 2 : 1;
+
if (ctx->split) {
struct commit_graph *g = ctx->r->objects->commit_graph;
--
2.43.0.334.gd4dbce1db5.dirty
^ permalink raw reply related
* [PATCH v5 14/17] commit-graph: drop unnecessary `graph_read_bloom_data_context`
From: Taylor Blau @ 2024-01-16 22:09 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt, Jonathan Tan, SZEDER Gábor
In-Reply-To: <cover.1705442923.git.me@ttaylorr.com>
The `graph_read_bloom_data_context` struct was introduced in an earlier
commit in order to pass pointers to the commit-graph and changed-path
Bloom filter version when reading the BDAT chunk.
The previous commit no longer writes through the changed_paths_version
pointer, making the surrounding context structure unnecessary. Drop it
and pass a pointer to the commit-graph directly when reading the BDAT
chunk.
Noticed-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
commit-graph.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index a2063d5f91..a02556716d 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -340,16 +340,10 @@ static int graph_read_bloom_index(const unsigned char *chunk_start,
return 0;
}
-struct graph_read_bloom_data_context {
- struct commit_graph *g;
- int *commit_graph_changed_paths_version;
-};
-
static int graph_read_bloom_data(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
- struct graph_read_bloom_data_context *c = data;
- struct commit_graph *g = c->g;
+ struct commit_graph *g = data;
uint32_t hash_version;
if (chunk_size < BLOOMDATA_CHUNK_HEADER_SIZE) {
@@ -463,14 +457,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
}
if (s->commit_graph_changed_paths_version) {
- struct graph_read_bloom_data_context context = {
- .g = graph,
- .commit_graph_changed_paths_version = &s->commit_graph_changed_paths_version
- };
read_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,
graph_read_bloom_index, graph);
read_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,
- graph_read_bloom_data, &context);
+ graph_read_bloom_data, graph);
}
if (graph->chunk_bloom_indexes && graph->chunk_bloom_data) {
--
2.43.0.334.gd4dbce1db5.dirty
^ permalink raw reply related
* [PATCH v5 15/17] object.h: fix mis-aligned flag bits table
From: Taylor Blau @ 2024-01-16 22:09 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt, Jonathan Tan, SZEDER Gábor
In-Reply-To: <cover.1705442923.git.me@ttaylorr.com>
Bit position 23 is one column too far to the left.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
object.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/object.h b/object.h
index 114d45954d..db25714b4e 100644
--- a/object.h
+++ b/object.h
@@ -62,7 +62,7 @@ void object_array_init(struct object_array *array);
/*
* object flag allocation:
- * revision.h: 0---------10 15 23------27
+ * revision.h: 0---------10 15 23------27
* fetch-pack.c: 01 67
* negotiator/default.c: 2--5
* walker.c: 0-2
--
2.43.0.334.gd4dbce1db5.dirty
^ permalink raw reply related
* [PATCH v5 16/17] commit-graph: reuse existing Bloom filters where possible
From: Taylor Blau @ 2024-01-16 22:09 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt, Jonathan Tan, SZEDER Gábor
In-Reply-To: <cover.1705442923.git.me@ttaylorr.com>
In an earlier commit, a bug was described where it's possible for Git to
produce non-murmur3 hashes when the platform's "char" type is signed,
and there are paths with characters whose highest bit is set (i.e. all
characters >= 0x80).
That patch allows the caller to control which version of Bloom filters
are read and written. However, even on platforms with a signed "char"
type, it is possible to reuse existing Bloom filters if and only if
there are no changed paths in any commit's first parent tree-diff whose
characters have their highest bit set.
When this is the case, we can reuse the existing filter without having
to compute a new one. This is done by marking trees which are known to
have (or not have) any such paths. When a commit's root tree is verified
to not have any such paths, we mark it as such and declare that the
commit's Bloom filter is reusable.
Note that this heuristic only goes in one direction. If neither a commit
nor its first parent have any paths in their trees with non-ASCII
characters, then we know for certain that a path with non-ASCII
characters will not appear in a tree-diff against that commit's first
parent. The reverse isn't necessarily true: just because the tree-diff
doesn't contain any such paths does not imply that no such paths exist
in either tree.
So we end up recomputing some Bloom filters that we don't strictly have
to (i.e. their bits are the same no matter which version of murmur3 we
use). But culling these out is impossible, since we'd have to perform
the full tree-diff, which is the same effort as computing the Bloom
filter from scratch.
But because we can cache our results in each tree's flag bits, we can
often avoid recomputing many filters, thereby reducing the time it takes
to run
$ git commit-graph write --changed-paths --reachable
when upgrading from v1 to v2 Bloom filters.
To benchmark this, let's generate a commit-graph in linux.git with v1
changed-paths in generation order[^1]:
$ git clone git@github.com:torvalds/linux.git
$ cd linux
$ git commit-graph write --reachable --changed-paths
$ graph=".git/objects/info/commit-graph"
$ mv $graph{,.bak}
Then let's time how long it takes to go from v1 to v2 filters (with and
without the upgrade path enabled), resetting the state of the
commit-graph each time:
$ git config commitGraph.changedPathsVersion 2
$ hyperfine -p 'cp -f $graph.bak $graph' -L v 0,1 \
'GIT_TEST_UPGRADE_BLOOM_FILTERS={v} git.compile commit-graph write --reachable --changed-paths'
On linux.git (where there aren't any non-ASCII paths), the timings
indicate that this patch represents a speed-up over recomputing all
Bloom filters from scratch:
Benchmark 1: GIT_TEST_UPGRADE_BLOOM_FILTERS=0 git.compile commit-graph write --reachable --changed-paths
Time (mean ± σ): 124.873 s ± 0.316 s [User: 124.081 s, System: 0.643 s]
Range (min … max): 124.621 s … 125.227 s 3 runs
Benchmark 2: GIT_TEST_UPGRADE_BLOOM_FILTERS=1 git.compile commit-graph write --reachable --changed-paths
Time (mean ± σ): 79.271 s ± 0.163 s [User: 74.611 s, System: 4.521 s]
Range (min … max): 79.112 s … 79.437 s 3 runs
Summary
'GIT_TEST_UPGRADE_BLOOM_FILTERS=1 git.compile commit-graph write --reachable --changed-paths' ran
1.58 ± 0.01 times faster than 'GIT_TEST_UPGRADE_BLOOM_FILTERS=0 git.compile commit-graph write --reachable --changed-paths'
On git.git, we do have some non-ASCII paths, giving us a more modest
improvement from 4.163 seconds to 3.348 seconds, for a 1.24x speed-up.
On my machine, the stats for git.git are:
- 8,285 Bloom filters computed from scratch
- 10 Bloom filters generated as empty
- 4 Bloom filters generated as truncated due to too many changed paths
- 65,114 Bloom filters were reused when transitioning from v1 to v2.
[^1]: Note that this is is important, since `--stdin-packs` or
`--stdin-commits` orders commits in the commit-graph by their pack
position (with `--stdin-packs`) or in the raw input (with
`--stdin-commits`).
Since we compute Bloom filters in the same order that commits appear
in the graph, we must see a commit's (first) parent before we process
the commit itself. This is only guaranteed to happen when sorting
commits by their generation number.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bloom.c | 90 ++++++++++++++++++++++++++++++++++++++++++--
bloom.h | 1 +
commit-graph.c | 5 +++
object.h | 1 +
t/t4216-log-bloom.sh | 35 ++++++++++++++++-
5 files changed, 128 insertions(+), 4 deletions(-)
diff --git a/bloom.c b/bloom.c
index 323d8012b8..a1c616bc71 100644
--- a/bloom.c
+++ b/bloom.c
@@ -6,6 +6,9 @@
#include "commit-graph.h"
#include "commit.h"
#include "commit-slab.h"
+#include "tree.h"
+#include "tree-walk.h"
+#include "config.h"
define_commit_slab(bloom_filter_slab, struct bloom_filter);
@@ -283,6 +286,73 @@ static void init_truncated_large_filter(struct bloom_filter *filter,
filter->version = version;
}
+#define VISITED (1u<<21)
+#define HIGH_BITS (1u<<22)
+
+static int has_entries_with_high_bit(struct repository *r, struct tree *t)
+{
+ if (parse_tree(t))
+ return 1;
+
+ if (!(t->object.flags & VISITED)) {
+ struct tree_desc desc;
+ struct name_entry entry;
+
+ init_tree_desc(&desc, t->buffer, t->size);
+ while (tree_entry(&desc, &entry)) {
+ size_t i;
+ for (i = 0; i < entry.pathlen; i++) {
+ if (entry.path[i] & 0x80) {
+ t->object.flags |= HIGH_BITS;
+ goto done;
+ }
+ }
+
+ if (S_ISDIR(entry.mode)) {
+ struct tree *sub = lookup_tree(r, &entry.oid);
+ if (sub && has_entries_with_high_bit(r, sub)) {
+ t->object.flags |= HIGH_BITS;
+ goto done;
+ }
+ }
+
+ }
+
+done:
+ t->object.flags |= VISITED;
+ }
+
+ return !!(t->object.flags & HIGH_BITS);
+}
+
+static int commit_tree_has_high_bit_paths(struct repository *r,
+ struct commit *c)
+{
+ struct tree *t;
+ if (repo_parse_commit(r, c))
+ return 1;
+ t = repo_get_commit_tree(r, c);
+ if (!t)
+ return 1;
+ return has_entries_with_high_bit(r, t);
+}
+
+static struct bloom_filter *upgrade_filter(struct repository *r, struct commit *c,
+ struct bloom_filter *filter,
+ int hash_version)
+{
+ struct commit_list *p = c->parents;
+ if (commit_tree_has_high_bit_paths(r, c))
+ return NULL;
+
+ if (p && commit_tree_has_high_bit_paths(r, p->item))
+ return NULL;
+
+ filter->version = hash_version;
+
+ return filter;
+}
+
struct bloom_filter *get_bloom_filter(struct repository *r, struct commit *c)
{
struct bloom_filter *filter;
@@ -325,9 +395,23 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
filter, graph_pos);
}
- if ((filter->data && filter->len) &&
- (!settings || settings->hash_version == filter->version))
- return filter;
+ if (filter->data && filter->len) {
+ struct bloom_filter *upgrade;
+ if (!settings || settings->hash_version == filter->version)
+ return filter;
+
+ /* version mismatch, see if we can upgrade */
+ if (compute_if_not_present &&
+ git_env_bool("GIT_TEST_UPGRADE_BLOOM_FILTERS", 1)) {
+ upgrade = upgrade_filter(r, c, filter,
+ settings->hash_version);
+ if (upgrade) {
+ if (computed)
+ *computed |= BLOOM_UPGRADED;
+ return upgrade;
+ }
+ }
+ }
if (!compute_if_not_present)
return NULL;
diff --git a/bloom.h b/bloom.h
index bfe389e29c..e3a9b68905 100644
--- a/bloom.h
+++ b/bloom.h
@@ -102,6 +102,7 @@ enum bloom_filter_computed {
BLOOM_COMPUTED = (1 << 1),
BLOOM_TRUNC_LARGE = (1 << 2),
BLOOM_TRUNC_EMPTY = (1 << 3),
+ BLOOM_UPGRADED = (1 << 4),
};
struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
diff --git a/commit-graph.c b/commit-graph.c
index a02556716d..b285e32043 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1167,6 +1167,7 @@ struct write_commit_graph_context {
int count_bloom_filter_not_computed;
int count_bloom_filter_trunc_empty;
int count_bloom_filter_trunc_large;
+ int count_bloom_filter_upgraded;
};
static int write_graph_chunk_fanout(struct hashfile *f,
@@ -1774,6 +1775,8 @@ static void trace2_bloom_filter_write_statistics(struct write_commit_graph_conte
ctx->count_bloom_filter_trunc_empty);
trace2_data_intmax("commit-graph", ctx->r, "filter-trunc-large",
ctx->count_bloom_filter_trunc_large);
+ trace2_data_intmax("commit-graph", ctx->r, "filter-upgraded",
+ ctx->count_bloom_filter_upgraded);
}
static void compute_bloom_filters(struct write_commit_graph_context *ctx)
@@ -1815,6 +1818,8 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)
ctx->count_bloom_filter_trunc_empty++;
if (computed & BLOOM_TRUNC_LARGE)
ctx->count_bloom_filter_trunc_large++;
+ } else if (computed & BLOOM_UPGRADED) {
+ ctx->count_bloom_filter_upgraded++;
} else if (computed & BLOOM_NOT_COMPUTED)
ctx->count_bloom_filter_not_computed++;
ctx->total_bloom_filter_data_size += filter
diff --git a/object.h b/object.h
index db25714b4e..2e5e08725f 100644
--- a/object.h
+++ b/object.h
@@ -75,6 +75,7 @@ void object_array_init(struct object_array *array);
* commit-reach.c: 16-----19
* sha1-name.c: 20
* list-objects-filter.c: 21
+ * bloom.c: 2122
* builtin/fsck.c: 0--3
* builtin/gc.c: 0
* builtin/index-pack.c: 2021
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index a7bf3a7dca..823d1cf773 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -222,6 +222,10 @@ test_filter_trunc_large () {
grep "\"key\":\"filter-trunc-large\",\"value\":\"$1\"" $2
}
+test_filter_upgraded () {
+ grep "\"key\":\"filter-upgraded\",\"value\":\"$1\"" $2
+}
+
test_expect_success 'correctly report changes over limit' '
git init limits &&
(
@@ -656,7 +660,13 @@ test_expect_success 'when writing another commit graph, preserve existing versio
test_expect_success 'when writing commit graph, do not reuse changed-path of another version' '
git init doublewrite &&
test_commit -C doublewrite c "$CENT" &&
+
git -C doublewrite config --add commitgraph.changedPathsVersion 1 &&
+ GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
+ git -C doublewrite commit-graph write --reachable --changed-paths &&
+ test_filter_computed 1 trace2.txt &&
+ test_filter_upgraded 0 trace2.txt &&
+
git -C doublewrite commit-graph write --reachable --changed-paths &&
for v in -2 3
do
@@ -667,8 +677,13 @@ test_expect_success 'when writing commit graph, do not reuse changed-path of ano
EOF
test_cmp expect err || return 1
done &&
+
git -C doublewrite config --add commitgraph.changedPathsVersion 2 &&
- git -C doublewrite commit-graph write --reachable --changed-paths &&
+ GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
+ git -C doublewrite commit-graph write --reachable --changed-paths &&
+ test_filter_computed 1 trace2.txt &&
+ test_filter_upgraded 0 trace2.txt &&
+
(
cd doublewrite &&
echo "c01f" >expect &&
@@ -677,6 +692,24 @@ test_expect_success 'when writing commit graph, do not reuse changed-path of ano
)
'
+test_expect_success 'when writing commit graph, reuse changed-path of another version where possible' '
+ git init upgrade &&
+
+ test_commit -C upgrade base no-high-bits &&
+
+ git -C upgrade config --add commitgraph.changedPathsVersion 1 &&
+ GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
+ git -C upgrade commit-graph write --reachable --changed-paths &&
+ test_filter_computed 1 trace2.txt &&
+ test_filter_upgraded 0 trace2.txt &&
+
+ git -C upgrade config --add commitgraph.changedPathsVersion 2 &&
+ GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
+ git -C upgrade commit-graph write --reachable --changed-paths &&
+ test_filter_computed 0 trace2.txt &&
+ test_filter_upgraded 1 trace2.txt
+'
+
corrupt_graph () {
test_when_finished "rm -rf $graph" &&
git commit-graph write --reachable --changed-paths &&
--
2.43.0.334.gd4dbce1db5.dirty
^ permalink raw reply related
* [PATCH v5 17/17] bloom: introduce `deinit_bloom_filters()`
From: Taylor Blau @ 2024-01-16 22:09 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt, Jonathan Tan, SZEDER Gábor
In-Reply-To: <cover.1705442923.git.me@ttaylorr.com>
After we are done using Bloom filters, we do not currently clean up any
memory allocated by the commit slab used to store those filters in the
first place.
Besides the bloom_filter structures themselves, there is mostly nothing
to free() in the first place, since in the read-only path all Bloom
filter's `data` members point to a memory mapped region in the
commit-graph file itself.
But when generating Bloom filters from scratch (or initializing
truncated filters) we allocate additional memory to store the filter's
data.
Keep track of when we need to free() this additional chunk of memory by
using an extra pointer `to_free`. Most of the time this will be NULL
(indicating that we are representing an existing Bloom filter stored in
a memory mapped region). When it is non-NULL, free it before discarding
the Bloom filters slab.
Suggested-by: Jonathan Tan <jonathantanmy@google.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>
---
bloom.c | 16 +++++++++++++++-
bloom.h | 3 +++
commit-graph.c | 4 ++++
3 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/bloom.c b/bloom.c
index a1c616bc71..dbcc0f4f50 100644
--- a/bloom.c
+++ b/bloom.c
@@ -92,6 +92,7 @@ int load_bloom_filter_from_graph(struct commit_graph *g,
sizeof(unsigned char) * start_index +
BLOOMDATA_CHUNK_HEADER_SIZE);
filter->version = g->bloom_filter_settings->hash_version;
+ filter->to_free = NULL;
return 1;
}
@@ -264,6 +265,18 @@ void init_bloom_filters(void)
init_bloom_filter_slab(&bloom_filters);
}
+static void free_one_bloom_filter(struct bloom_filter *filter)
+{
+ if (!filter)
+ return;
+ free(filter->to_free);
+}
+
+void deinit_bloom_filters(void)
+{
+ deep_clear_bloom_filter_slab(&bloom_filters, free_one_bloom_filter);
+}
+
static int pathmap_cmp(const void *hashmap_cmp_fn_data UNUSED,
const struct hashmap_entry *eptr,
const struct hashmap_entry *entry_or_key,
@@ -280,7 +293,7 @@ static int pathmap_cmp(const void *hashmap_cmp_fn_data UNUSED,
static void init_truncated_large_filter(struct bloom_filter *filter,
int version)
{
- filter->data = xmalloc(1);
+ filter->data = filter->to_free = xmalloc(1);
filter->data[0] = 0xFF;
filter->len = 1;
filter->version = version;
@@ -482,6 +495,7 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
filter->len = 1;
}
CALLOC_ARRAY(filter->data, filter->len);
+ filter->to_free = filter->data;
hashmap_for_each_entry(&pathmap, &iter, e, entry) {
struct bloom_key key;
diff --git a/bloom.h b/bloom.h
index e3a9b68905..d20e64bfbb 100644
--- a/bloom.h
+++ b/bloom.h
@@ -56,6 +56,8 @@ struct bloom_filter {
unsigned char *data;
size_t len;
int version;
+
+ void *to_free;
};
/*
@@ -96,6 +98,7 @@ void add_key_to_filter(const struct bloom_key *key,
const struct bloom_filter_settings *settings);
void init_bloom_filters(void);
+void deinit_bloom_filters(void);
enum bloom_filter_computed {
BLOOM_NOT_COMPUTED = (1 << 0),
diff --git a/commit-graph.c b/commit-graph.c
index b285e32043..1841638801 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -830,6 +830,7 @@ struct bloom_filter_settings *get_bloom_filter_settings(struct repository *r)
void close_commit_graph(struct raw_object_store *o)
{
clear_commit_graph_data_slab(&commit_graph_data_slab);
+ deinit_bloom_filters();
free_commit_graph(o->commit_graph);
o->commit_graph = NULL;
}
@@ -2648,6 +2649,9 @@ int write_commit_graph(struct object_directory *odb,
res = write_commit_graph_file(ctx);
+ if (ctx->changed_paths)
+ deinit_bloom_filters();
+
if (ctx->split)
mark_commit_graphs(ctx);
--
2.43.0.334.gd4dbce1db5.dirty
^ permalink raw reply related
* [RFC PATCH 0/4] test-tool: add unit test suite runner
From: Josh Steadmon @ 2024-01-16 22:22 UTC (permalink / raw)
To: git; +Cc: johannes.schindelin, peff, phillip.wood
For various reasons (see discussion at [1]) we would like an alternative
to `prove` for running test suites (including the unit tests) on
Windows.
This series extends the existing `test-tool run-command testsuite` to
support running unit tests. In addition, it includes some small
cleanups: we move t-basic out of the unit-tests directory, and add a
test wrapper script to allow unit tests and the shell test suite to run
in a single `prove` process.
Some known remaining bits of work:
* We need to filter out cmake *.pdb files when running with `test-tool`.
* We should investigate switching the Windows CI to use `test-tool`
instead of prove.
* We should determine whether it is confusing or otherwise harmful to
people's workflow to have the unit tests run in parallel with shell
tests when using prove as the default test target.
[1] https://lore.kernel.org/git/pull.1613.git.1699894837844.gitgitgadget@gmail.com/
Jeff King (1):
t/Makefile: run unit tests alongside shell tests
Josh Steadmon (3):
t0080: turn t-basic unit test into a helper
test-tool run-command testsuite: support unit tests
unit tests: add rule for running with test-tool
Makefile | 19 +++++++++++----
t/Makefile | 13 +++++++---
t/helper/test-run-command.c | 40 ++++++++++++++++++++++++-------
t/run-test.sh | 13 ++++++++++
t/t0080-unit-test-output.sh | 24 +++++++++----------
t/t0080/.gitignore | 1 +
t/{unit-tests => t0080}/t-basic.c | 2 +-
7 files changed, 83 insertions(+), 29 deletions(-)
create mode 100755 t/run-test.sh
create mode 100644 t/t0080/.gitignore
rename t/{unit-tests => t0080}/t-basic.c (98%)
base-commit: a26002b62827b89a19b1084bd75d9371d565d03c
--
2.43.0.381.gb435a96ce8-goog
^ permalink raw reply
* [RFC PATCH 1/4] t0080: turn t-basic unit test into a helper
From: Josh Steadmon @ 2024-01-16 22:22 UTC (permalink / raw)
To: git; +Cc: johannes.schindelin, peff, phillip.wood
In-Reply-To: <cover.1705443632.git.steadmon@google.com>
While t/unit-tests/t-basic.c uses the unit-test framework added in
e137fe3b29 (unit tests: add TAP unit test framework, 2023-11-09), it is
not a true unit test in that it intentionally fails in order to exercise
various codepaths in the unit-test framework. Thus, we intentionally
exclude it when running unit tests through the various t/Makefile
targets. Instead, it is executed by t0080-unit-test-output.sh, which
verifies its output follows the TAP format expected for the various
pass, skip, or fail cases.
As such, it makes more sense for t-basic to be a helper item for
t0080-unit-test-output.sh, so let's move it to t/t0080/t-basic.c and
adjust Makefiles and .gitignores as necessary.
This has the additional benefit that test harnesses seeking to run all
unit tests can find them with a simple glob of "t/unit-tests/bin/t-*",
with no exceptions needed. This will be important in a later patch where
we add support for running the unit tests via a test-tool subcommand.
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Makefile | 17 +++++++++++++----
t/Makefile | 2 +-
t/t0080-unit-test-output.sh | 24 ++++++++++++------------
t/t0080/.gitignore | 1 +
t/{unit-tests => t0080}/t-basic.c | 2 +-
5 files changed, 28 insertions(+), 18 deletions(-)
create mode 100644 t/t0080/.gitignore
rename t/{unit-tests => t0080}/t-basic.c (98%)
diff --git a/Makefile b/Makefile
index 88ba7a3c51..ab32ec1101 100644
--- a/Makefile
+++ b/Makefile
@@ -683,6 +683,7 @@ TEST_OBJS =
TEST_PROGRAMS_NEED_X =
THIRD_PARTY_SOURCES =
UNIT_TEST_PROGRAMS =
+UNIT_TEST_HELPERS =
UNIT_TEST_DIR = t/unit-tests
UNIT_TEST_BIN = $(UNIT_TEST_DIR)/bin
@@ -1339,10 +1340,12 @@ THIRD_PARTY_SOURCES += compat/regex/%
THIRD_PARTY_SOURCES += sha1collisiondetection/%
THIRD_PARTY_SOURCES += sha1dc/%
-UNIT_TEST_PROGRAMS += t-basic
UNIT_TEST_PROGRAMS += t-strbuf
UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
+UNIT_TEST_HELPERS += t/t0080/t-basic
+UNIT_TEST_HELPER_PROGS = $(patsubst %,%$X,$(UNIT_TEST_HELPERS))
UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
+UNIT_TEST_OBJS += $(patsubst %,%.o,$(UNIT_TEST_HELPERS))
UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
# xdiff and reftable libs may in turn depend on what is in libgit.a
@@ -3189,7 +3192,9 @@ endif
test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
-all:: $(TEST_PROGRAMS) $(test_bindir_programs) $(UNIT_TEST_PROGS)
+all:: $(TEST_PROGRAMS) $(test_bindir_programs)
+
+all:: $(UNIT_TEST_PROGS) $(UNIT_TEST_HELPER_PROGS)
bin-wrappers/%: wrap-for-bin.sh
$(call mkdir_p_parent_template)
@@ -3620,7 +3625,7 @@ endif
artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
- $(UNIT_TEST_PROGS) $(MOFILES)
+ $(UNIT_TEST_PROGS) $(UNIT_TEST_HELPER_PROGS) $(MOFILES)
$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
test -n "$(ARTIFACTS_DIRECTORY)"
@@ -3682,7 +3687,7 @@ clean: profile-clean coverage-clean cocciclean
$(RM) headless-git.o
$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
- $(RM) $(TEST_PROGRAMS) $(UNIT_TEST_PROGS)
+ $(RM) $(TEST_PROGRAMS) $(UNIT_TEST_PROGS) $(UNIT_TEST_HELPER_PROGS)
$(RM) $(FUZZ_PROGRAMS)
$(RM) $(SP_OBJ)
$(RM) $(HCC)
@@ -3869,6 +3874,10 @@ $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
$(filter %.o,$^) $(filter %.a,$^) $(LIBS)
+$(UNIT_TEST_HELPER_PROGS): %$X: %.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS
+ $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
+ $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
+
.PHONY: build-unit-tests unit-tests
build-unit-tests: $(UNIT_TEST_PROGS)
unit-tests: $(UNIT_TEST_PROGS)
diff --git a/t/Makefile b/t/Makefile
index b7a6fefe28..0bee7bc6ea 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -42,7 +42,7 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
-UNIT_TESTS = $(sort $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))
+UNIT_TESTS = $(sort $(filter-out %.pdb,$(wildcard unit-tests/bin/t-*)))
# `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
# checks all tests in all scripts via a single invocation, so tell individual
diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh
index 961b54b06c..7431023d97 100755
--- a/t/t0080-unit-test-output.sh
+++ b/t/t0080-unit-test-output.sh
@@ -8,50 +8,50 @@ test_expect_success 'TAP output from unit tests' '
cat >expect <<-EOF &&
ok 1 - passing test
ok 2 - passing test and assertion return 1
- # check "1 == 2" failed at t/unit-tests/t-basic.c:76
+ # check "1 == 2" failed at t/t0080/t-basic.c:76
# left: 1
# right: 2
not ok 3 - failing test
ok 4 - failing test and assertion return 0
not ok 5 - passing TEST_TODO() # TODO
ok 6 - passing TEST_TODO() returns 1
- # todo check ${SQ}check(x)${SQ} succeeded at t/unit-tests/t-basic.c:25
+ # todo check ${SQ}check(x)${SQ} succeeded at t/t0080/t-basic.c:25
not ok 7 - failing TEST_TODO()
ok 8 - failing TEST_TODO() returns 0
- # check "0" failed at t/unit-tests/t-basic.c:30
+ # check "0" failed at t/t0080/t-basic.c:30
# skipping test - missing prerequisite
- # skipping check ${SQ}1${SQ} at t/unit-tests/t-basic.c:32
+ # skipping check ${SQ}1${SQ} at t/t0080/t-basic.c:32
ok 9 - test_skip() # SKIP
ok 10 - skipped test returns 1
# skipping test - missing prerequisite
ok 11 - test_skip() inside TEST_TODO() # SKIP
ok 12 - test_skip() inside TEST_TODO() returns 1
- # check "0" failed at t/unit-tests/t-basic.c:48
+ # check "0" failed at t/t0080/t-basic.c:48
not ok 13 - TEST_TODO() after failing check
ok 14 - TEST_TODO() after failing check returns 0
- # check "0" failed at t/unit-tests/t-basic.c:56
+ # check "0" failed at t/t0080/t-basic.c:56
not ok 15 - failing check after TEST_TODO()
ok 16 - failing check after TEST_TODO() returns 0
- # check "!strcmp("\thello\\\\", "there\"\n")" failed at t/unit-tests/t-basic.c:61
+ # check "!strcmp("\thello\\\\", "there\"\n")" failed at t/t0080/t-basic.c:61
# left: "\011hello\\\\"
# right: "there\"\012"
- # check "!strcmp("NULL", NULL)" failed at t/unit-tests/t-basic.c:62
+ # check "!strcmp("NULL", NULL)" failed at t/t0080/t-basic.c:62
# left: "NULL"
# right: NULL
- # check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/unit-tests/t-basic.c:63
+ # check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/t0080/t-basic.c:63
# left: ${SQ}a${SQ}
# right: ${SQ}\012${SQ}
- # check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/unit-tests/t-basic.c:64
+ # check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/t0080/t-basic.c:64
# left: ${SQ}\\\\${SQ}
# right: ${SQ}\\${SQ}${SQ}
not ok 17 - messages from failing string and char comparison
- # BUG: test has no checks at t/unit-tests/t-basic.c:91
+ # BUG: test has no checks at t/t0080/t-basic.c:91
not ok 18 - test with no checks
ok 19 - test with no checks returns 0
1..19
EOF
- ! "$GIT_BUILD_DIR"/t/unit-tests/bin/t-basic >actual &&
+ ! "$GIT_BUILD_DIR"/t/t0080/t-basic >actual &&
test_cmp expect actual
'
diff --git a/t/t0080/.gitignore b/t/t0080/.gitignore
new file mode 100644
index 0000000000..1903542827
--- /dev/null
+++ b/t/t0080/.gitignore
@@ -0,0 +1 @@
+/t-basic
diff --git a/t/unit-tests/t-basic.c b/t/t0080/t-basic.c
similarity index 98%
rename from t/unit-tests/t-basic.c
rename to t/t0080/t-basic.c
index fda1ae59a6..83727221b1 100644
--- a/t/unit-tests/t-basic.c
+++ b/t/t0080/t-basic.c
@@ -1,4 +1,4 @@
-#include "test-lib.h"
+#include "../unit-tests/test-lib.h"
/*
* The purpose of this "unit test" is to verify a few invariants of the unit
--
2.43.0.381.gb435a96ce8-goog
^ permalink raw reply related
* [RFC PATCH 2/4] test-tool run-command testsuite: support unit tests
From: Josh Steadmon @ 2024-01-16 22:22 UTC (permalink / raw)
To: git; +Cc: johannes.schindelin, peff, phillip.wood
In-Reply-To: <cover.1705443632.git.steadmon@google.com>
Teach the testsuite runner in `test-tool run-command testsuite` how to
run unit tests, by adding two new flags:
First, "--(no-)run-in-shell" allows the test-tool to exec the unit-test
binaries directly, rather than trying to interpret them as shell
scripts.
Second "--(no-)require-shell-test-pattern" bypasses the check that the
test filenames match the expected t####-*.sh pattern.
With these changes, you can now use test-tool to run the unit tests:
$ make
$ cd t/unit-tests/bin
$ ../../helper/test-tool run-command testsuite --no-run-in-shell \
--no-require-shell-test-pattern
This should be helpful on Windows to allow running tests without
requiring Perl (for `prove`), as discussed in [1] and [2].
[1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
[2] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
t/helper/test-run-command.c | 40 +++++++++++++++++++++++++++++--------
1 file changed, 32 insertions(+), 8 deletions(-)
diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index c0ed8722c8..2db7e1ef03 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -64,11 +64,12 @@ static int task_finished(int result UNUSED,
struct testsuite {
struct string_list tests, failed;
int next;
- int quiet, immediate, verbose, verbose_log, trace, write_junit_xml;
+ int quiet, immediate, verbose, verbose_log, trace, write_junit_xml, run_in_shell;
};
#define TESTSUITE_INIT { \
.tests = STRING_LIST_INIT_DUP, \
.failed = STRING_LIST_INIT_DUP, \
+ .run_in_shell = 1, \
}
static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
@@ -80,7 +81,9 @@ static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
return 0;
test = suite->tests.items[suite->next++].string;
- strvec_pushl(&cp->args, "sh", test, NULL);
+ if (suite->run_in_shell)
+ strvec_push(&cp->args, "sh");
+ strvec_push(&cp->args, test);
if (suite->quiet)
strvec_push(&cp->args, "--quiet");
if (suite->immediate)
@@ -133,7 +136,7 @@ static const char * const testsuite_usage[] = {
static int testsuite(int argc, const char **argv)
{
struct testsuite suite = TESTSUITE_INIT;
- int max_jobs = 1, i, ret = 0;
+ int max_jobs = 1, i, ret = 0, require_shell_test_pattern = 1;
DIR *dir;
struct dirent *d;
struct option options[] = {
@@ -147,6 +150,10 @@ static int testsuite(int argc, const char **argv)
OPT_BOOL('x', "trace", &suite.trace, "trace shell commands"),
OPT_BOOL(0, "write-junit-xml", &suite.write_junit_xml,
"write JUnit-style XML files"),
+ OPT_BOOL(0, "run-in-shell", &suite.run_in_shell,
+ "run programs in the suite via `sh`"),
+ OPT_BOOL(0, "require-shell-test-pattern", &require_shell_test_pattern,
+ "require programs to match 't####-*.sh'"),
OPT_END()
};
struct run_process_parallel_opts opts = {
@@ -155,12 +162,21 @@ static int testsuite(int argc, const char **argv)
.task_finished = test_finished,
.data = &suite,
};
+ struct strbuf progpath = STRBUF_INIT;
+ size_t path_prefix_len;
argc = parse_options(argc, argv, NULL, options,
testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
if (max_jobs <= 0)
max_jobs = online_cpus();
+ /*
+ * If we run without a shell, we have to provide the relative path to
+ * the executables.
+ */
+ if (!suite.run_in_shell)
+ strbuf_addstr(&progpath, "./");
+ path_prefix_len = progpath.len;
dir = opendir(".");
if (!dir)
@@ -168,20 +184,27 @@ static int testsuite(int argc, const char **argv)
while ((d = readdir(dir))) {
const char *p = d->d_name;
- if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) ||
- !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' ||
- !ends_with(p, ".sh"))
+ if (!strcmp(p, ".") || !strcmp(p, ".."))
continue;
+ if (require_shell_test_pattern)
+ if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) ||
+ !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' ||
+ !ends_with(p, ".sh"))
+ continue;
/* No pattern: match all */
if (!argc) {
- string_list_append(&suite.tests, p);
+ strbuf_setlen(&progpath, path_prefix_len);
+ strbuf_addstr(&progpath, p);
+ string_list_append(&suite.tests, progpath.buf);
continue;
}
for (i = 0; i < argc; i++)
if (!wildmatch(argv[i], p, 0)) {
- string_list_append(&suite.tests, p);
+ strbuf_setlen(&progpath, path_prefix_len);
+ strbuf_addstr(&progpath, p);
+ string_list_append(&suite.tests, progpath.buf);
break;
}
}
@@ -208,6 +231,7 @@ static int testsuite(int argc, const char **argv)
string_list_clear(&suite.tests, 0);
string_list_clear(&suite.failed, 0);
+ strbuf_release(&progpath);
return ret;
}
--
2.43.0.381.gb435a96ce8-goog
^ permalink raw reply related
* [RFC PATCH 3/4] unit tests: add rule for running with test-tool
From: Josh Steadmon @ 2024-01-16 22:23 UTC (permalink / raw)
To: git; +Cc: johannes.schindelin, peff, phillip.wood
In-Reply-To: <cover.1705443632.git.steadmon@google.com>
In the previous commit, we added support in test-tool for running
collections of unit tests. Now, add rules in t/Makefile for running in
this way.
This new rule can be executed from the top-level Makefile via
`make DEFAULT_UNIT_TEST_TARGET=unit-tests-test-tool unit-tests`, or by
setting DEFAULT_UNIT_TEST_TARGET in config.mak.
NEEDS WORK: we need to exclude .pdb files generated by cmake [see
0df903d402 (unit-tests: do not mistake `.pdb` files for being
executable, 2023-09-25)]
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Makefile | 2 +-
t/Makefile | 9 ++++++++-
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index ab32ec1101..ce43ad2ae8 100644
--- a/Makefile
+++ b/Makefile
@@ -3880,5 +3880,5 @@ $(UNIT_TEST_HELPER_PROGS): %$X: %.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-L
.PHONY: build-unit-tests unit-tests
build-unit-tests: $(UNIT_TEST_PROGS)
-unit-tests: $(UNIT_TEST_PROGS)
+unit-tests: $(UNIT_TEST_PROGS) t/helper/test-tool$X
$(MAKE) -C t/ unit-tests
diff --git a/t/Makefile b/t/Makefile
index 0bee7bc6ea..ad57ec0a41 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -70,7 +70,7 @@ $(T):
$(UNIT_TESTS):
@echo "*** $@ ***"; $@
-.PHONY: unit-tests unit-tests-raw unit-tests-prove
+.PHONY: unit-tests unit-tests-raw unit-tests-prove unit-tests-test-tool
unit-tests: $(DEFAULT_UNIT_TEST_TARGET)
unit-tests-raw: $(UNIT_TESTS)
@@ -78,6 +78,13 @@ unit-tests-raw: $(UNIT_TESTS)
unit-tests-prove:
@echo "*** prove - unit tests ***"; $(PROVE) $(GIT_PROVE_OPTS) $(UNIT_TESTS)
+unit-tests-test-tool:
+ @echo "*** test-tool - unit tests **"
+ ( \
+ cd unit-tests/bin && \
+ ../../helper/test-tool run-command testsuite --no-run-in-shell --no-require-shell-test-pattern \
+ )
+
pre-clean:
$(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
--
2.43.0.381.gb435a96ce8-goog
^ permalink raw reply related
* [RFC PATCH 4/4] t/Makefile: run unit tests alongside shell tests
From: Josh Steadmon @ 2024-01-16 22:23 UTC (permalink / raw)
To: git; +Cc: johannes.schindelin, peff, phillip.wood
In-Reply-To: <cover.1705443632.git.steadmon@google.com>
From: Jeff King <peff@peff.net>
Add a wrapper script to allow `prove` to run both shell tests and unit
tests from a single invocation. This avoids issues around running prove
twice in CI, as discussed in [1].
Additionally, this moves the unit tests into the main dev workflow, so
that errors can be spotted more quickly.
NEEDS WORK: as discussed in previous commits in this series, there's a
desire to avoid `prove` specifically and (IIUC) unnecessary
fork()/exec()ing in general on Windows. This change adds an extra exec()
for each shell and unit test execution, will that be a problem for
Windows?
[1] https://lore.kernel.org/git/pull.1613.git.1699894837844.gitgitgadget@gmail.com/
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
t/Makefile | 2 +-
t/run-test.sh | 13 +++++++++++++
2 files changed, 14 insertions(+), 1 deletion(-)
create mode 100755 t/run-test.sh
diff --git a/t/Makefile b/t/Makefile
index ad57ec0a41..0038a25e33 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -61,7 +61,7 @@ failed:
test -z "$$failed" || $(MAKE) $$failed
prove: pre-clean check-chainlint $(TEST_LINT)
- @echo "*** prove ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
+ @echo "*** prove (shell & unit tests) ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec ./run-test.sh $(GIT_PROVE_OPTS) $(T) $(UNIT_TESTS) :: $(GIT_TEST_OPTS)
$(MAKE) clean-except-prove-cache
$(T):
diff --git a/t/run-test.sh b/t/run-test.sh
new file mode 100755
index 0000000000..c29fef48dc
--- /dev/null
+++ b/t/run-test.sh
@@ -0,0 +1,13 @@
+#!/bin/sh
+
+# A simple wrapper to run shell tests via TEST_SHELL_PATH,
+# or exec unit tests directly.
+
+case "$1" in
+*.sh)
+ exec ${TEST_SHELL_PATH:-/bin/sh} "$@"
+ ;;
+*)
+ exec "$@"
+ ;;
+esac
--
2.43.0.381.gb435a96ce8-goog
^ permalink raw reply related
* Re: [PATCH] bisect: add --force flag to force checkout
From: Junio C Hamano @ 2024-01-16 22:30 UTC (permalink / raw)
To: Kevin Wang via GitGitGadget; +Cc: git, Kevin Wang
In-Reply-To: <pull.1641.git.git.1705302307639.gitgitgadget@gmail.com>
"Kevin Wang via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Kevin Wang <kevmo314@gmail.com>
>
> Adds a `--force`/`-f` flag to `git bisect good/bad` and `git bisect run` to
> force a checkout. Currently, if the repository state adds any local changes
> the user must manually reset the repository state before moving to the next
> bisection step. This can happen with package lock files or log output data,
> for example. With this change, a developer can run `git bisect run --force`
> to automatically reset the repository state after each evaluation. The flag
> is also supported as `git bisect (good|bad) --force` as well.
The usual way to compose a log message is to
- Give an observation on how the current system work in the present
tense (so no need to say "Currently X is Y", just "X is Y"), and
discuss what you perceive as a problem in it.
- Propose a solution (optional---often, problem description
trivially leads to an obvious solution in reader's minds).
- Give commands to the make codebase "like so".
in this order.
To those who have been intimately following the discussion, it often
is understandable without both, but we are not writing for those who
review the patches. We are writing for future readers who are not
aware of the review discussion we have on list, so we should give
something to prepare them by setting the stage and stating the
objective first, before going into how the patch solved it.
Having said all that.
I highly doubt that this patch is a good idea. If your "bisect run"
script needs to update something in the working tree before it runs
some test, the script is in a much better place than Git, which is
unaware of what your run script is doing, to prepare the working
tree into pristine state. The best Git would be able to do would be
to "reset --hard", but that will lose local modifications that are
deliberately there and has nothing to do with what your run script
did.
Adding some description to the documentation of "bisect run" and
teaching readers a common trick of structuring their run script
better might be a more productive approach, I would have to say.
For example, when I bisect some old code, I may have to apply a
temporary patch to some of the sources to get them compile with more
recent compilers (I usually do this with a cherry-picking of a local
fixup). So my "bisect run" script might go like so:
#!/bin/sh
# bisect run
git apply local-fixup || exit 125
make test
status=$?
make distclean
git apply -R local-fixup || exit 125
exit $status
That is, I'd apply some local fix-up to the working tree files
before running tests, and once done, I revert the local fix-up
and exit from the run script with the exit status of the test
I wanted to perform.
This way, I can keep other local changes (things like changes to
documentation files that I am working on, which has nothing to do
with the problem I am bisecting but I know they do not interfere)
without wiping them away with a sledgehammer "reset --hard". Your
"bisect good/bad -f" sounds like the sledgehammer approach to me.
Thanks.
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)
From: Junio C Hamano @ 2024-01-16 22:45 UTC (permalink / raw)
To: Taylor Blau; +Cc: SZEDER Gábor, git
In-Reply-To: <Zabr1Glljjgl/UUB@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> OK, everything seems fine thus far, until we inspect the value of
> g->bloom_filter_settings, which is NULL, becuase of this hunk from
> commit-graph.c::graph_read_bloom_data():
>
> if (hash_version != 1)
> return 0;
>
> which terminates the function before we assign g->bloom_filter_settings
> for the existing (written with v2 Bloom filters) graph layer.
>
> I don't think that there is a way to fix this in a backwards compatible
> way, but I'm comfortable with that in this instance since we don't
> expect users to upgrading to v2 Bloom filters and then writing new graph
> layers using a non-v2 compatible version of Git.
A big red button solution to avoid this would be to uprev the
repository format version once you start writing v2 Bloom filters
anywhere in the layers. That way, existing Git clients would not be
able to touch it. I do not know if the cure is more severe than the
disease in that case, though.
In any case, at least, we should be able to prepare the code that we
teach to grok v2 today so that they do not trigger the same segfault
when they see a commit graph layer containing v3 Bloom filters (or
later). Then we won't have to have the same conversation when we
somehow need to update Bloom filters again.
^ permalink raw reply
* Re: [RFC PATCH 1/4] t0080: turn t-basic unit test into a helper
From: Junio C Hamano @ 2024-01-16 22:54 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, johannes.schindelin, peff, phillip.wood
In-Reply-To: <a9f67ed703c8314f0f0507ffb83b503717b846b3.1705443632.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> As such, it makes more sense for t-basic to be a helper item for
> t0080-unit-test-output.sh, ...
Up to this part was very understandable and agreeable, but I was
surprised to see ...
> ... so let's move it to t/t0080/t-basic.c and
> adjust Makefiles and .gitignores as necessary.
... this conclusion. I somehow thought that t-basic part would be a
good test-tool subcommand, as it is run from the suite of shell
scripts for end-to-end testing CLI interaction.
Do we have any precedent to place programs placed under t/tXXXX/ and
get them compiled?
> This has the additional benefit that test harnesses seeking to run all
> unit tests can find them with a simple glob of "t/unit-tests/bin/t-*",
> with no exceptions needed.
And this motivation is very much understandable, and for that, as
long as it is outside t/unit-tests/, it would be good. I just did
not expect it to hang under t/t0080/, inviting more unit test pieces
that are triggerable from the numberd shell scripts to hang under
random t/t[0-9][0-9][0-9][0-9]/ directories.
^ permalink raw reply
* Re: [RFC PATCH 2/4] test-tool run-command testsuite: support unit tests
From: Junio C Hamano @ 2024-01-16 23:18 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, johannes.schindelin, peff, phillip.wood
In-Reply-To: <5ecbc976e6216b941e760e096e166ab432ee7784.1705443632.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> Teach the testsuite runner in `test-tool run-command testsuite` how to
> run unit tests, by adding two new flags:
>
> First, "--(no-)run-in-shell" allows the test-tool to exec the unit-test
> binaries directly, rather than trying to interpret them as shell
> scripts.
Makes perfect sense.
> Second "--(no-)require-shell-test-pattern" bypasses the check that the
> test filenames match the expected t####-*.sh pattern.
This one I am not so sure. Do we still have situations where
erroring out when fed a non t[0-9][0-9][0-9][0-9]-*.sh script is
problematic? IOW, do we need to keep it as conditional?
... goes and looks ...
Ah, this variable/option is misnamed and that is what invited the
above nonsense question out of me. The logic this option disables
does not "require" (and error out if the requirement is not met); it
is used in a loop over "ls *" and "filtering" files out that are not
the numbered scripts.
But that confusion makes me wonder if non-script side of tests would
also want some filtering in the longer run, even if the directory we
feed to "test-tool run" happens to contain nothing but what we want
to run right now. I wonder if we instead want a variable that holds
a pattern used to match programs readdir() discovers and skip those
that do not match the pattern? Its default value may be something
like "t[0-9][0-9][0-9][0-9]-*.sh" but alternatively you can give,
say, "*" to pass everything, or something like that.
> With these changes, you can now use test-tool to run the unit tests:
> $ make
> $ cd t/unit-tests/bin
> $ ../../helper/test-tool run-command testsuite --no-run-in-shell \
> --no-require-shell-test-pattern
This makes me wonder why we want to do the readdir() loop ourselves.
Instead of saying --no-require-shell-test-pattern there, wouldn't it
be simpler to say "*" right there, and have testsuite() run the test
programs named from the command line?
But that is orthogonal to the enhancement we have here.
^ permalink raw reply
* Re: [RFC PATCH 0/4] test-tool: add unit test suite runner
From: Junio C Hamano @ 2024-01-16 23:24 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, johannes.schindelin, peff, phillip.wood
In-Reply-To: <cover.1705443632.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> For various reasons (see discussion at [1]) we would like an alternative
> to `prove` for running test suites (including the unit tests) on
> Windows.
>
> This series extends the existing `test-tool run-command testsuite` to
> support running unit tests. In addition, it includes some small
> cleanups: we move t-basic out of the unit-tests directory, and add a
> test wrapper script to allow unit tests and the shell test suite to run
> in a single `prove` process.
>
> Some known remaining bits of work:
> * We need to filter out cmake *.pdb files when running with `test-tool`.
;-)
I should have read this before I commented on that "--require
is misnamed---what you really want is a custom filter" thing.
> * We should investigate switching the Windows CI to use `test-tool`
> instead of prove.
OK.
> * We should determine whether it is confusing or otherwise harmful to
> people's workflow to have the unit tests run in parallel with shell
> tests when using prove as the default test target.
It is probably confusing especially given that the testsuite() thing
does its own parallel spawning of tests while prove does its own.
Is there a reason to choose unit-tests-test-tool Makefile target on
prove-capable platforms (other than for testing the test-tool)?
Thanks for an entertaining read.
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)
From: Taylor Blau @ 2024-01-16 23:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: SZEDER Gábor, git
In-Reply-To: <xmqqfrywyk16.fsf@gitster.g>
On Tue, Jan 16, 2024 at 02:45:41PM -0800, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > OK, everything seems fine thus far, until we inspect the value of
> > g->bloom_filter_settings, which is NULL, becuase of this hunk from
> > commit-graph.c::graph_read_bloom_data():
> >
> > if (hash_version != 1)
> > return 0;
> >
> > which terminates the function before we assign g->bloom_filter_settings
> > for the existing (written with v2 Bloom filters) graph layer.
> >
> > I don't think that there is a way to fix this in a backwards compatible
> > way, but I'm comfortable with that in this instance since we don't
> > expect users to upgrading to v2 Bloom filters and then writing new graph
> > layers using a non-v2 compatible version of Git.
>
> A big red button solution to avoid this would be to uprev the
> repository format version once you start writing v2 Bloom filters
> anywhere in the layers. That way, existing Git clients would not be
> able to touch it. I do not know if the cure is more severe than the
> disease in that case, though.
I tend to think that in this case the cure is probably worse than the
disease. I expect it to be extremely rare that a user would upgrade to a
modern version of Git, write commit-graphs, then downgrade, and try to
write more commit-graphs.
> In any case, at least, we should be able to prepare the code that we
> teach to grok v2 today so that they do not trigger the same segfault
> when they see a commit graph layer containing v3 Bloom filters (or
> later). Then we won't have to have the same conversation when we
> somehow need to update Bloom filters again.
This series should accomplish that by loading the Bloom chunk
unconditionally, and only reading its filters when they match the
given hash_version.
Thanks,
Taylor
^ permalink raw reply
* Re: [RFC PATCH 2/4] test-tool run-command testsuite: support unit tests
From: Junio C Hamano @ 2024-01-16 23:40 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, johannes.schindelin, peff, phillip.wood
In-Reply-To: <xmqqv87sx3y2.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Josh Steadmon <steadmon@google.com> writes:
>
>> Teach the testsuite runner in `test-tool run-command testsuite` how to
>> run unit tests, by adding two new flags:
>>
>> First, "--(no-)run-in-shell" allows the test-tool to exec the unit-test
>> binaries directly, rather than trying to interpret them as shell
>> scripts.
>
> Makes perfect sense.
This may be a stupid question, but do we even need the current "push
'sh' to the strvec"? If our executable shell scripts run just fine
without, then this may not have to be conditional.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox