* Re: [PATCH v3 03/17] commit-graph: ensure Bloom filters are read with consistent settings
From: Patrick Steinhardt @ 2023-10-17 8:45 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Jonathan Tan, Junio C Hamano, Jeff King, SZEDER Gábor
In-Reply-To: <2ecc0a2d58432b149d73a3e2abfa948eb1f0aa0b.1696969994.git.me@ttaylorr.com>
[-- Attachment #1: Type: text/plain, Size: 8299 bytes --]
On Tue, Oct 10, 2023 at 04:33:26PM -0400, Taylor Blau wrote:
> The changed-path Bloom filter mechanism is parameterized by a couple of
> variables, notably the number of bits per hash (typically "m" in Bloom
> filter literature) and the number of hashes themselves (typically "k").
>
> It is critically important that filters are read with the Bloom filter
> settings that they were written with. Failing to do so would mean that
> each query is liable to compute different fingerprints, meaning that the
> filter itself could return a false negative. This goes against a basic
> assumption of using Bloom filters (that they may return false positives,
> but never false negatives) and can lead to incorrect results.
>
> We have some existing logic to carry forward existing Bloom filter
> settings from one layer to the next. In `write_commit_graph()`, we have
> something like:
>
> if (!(flags & COMMIT_GRAPH_NO_WRITE_BLOOM_FILTERS)) {
> struct commit_graph *g = ctx->r->objects->commit_graph;
>
> /* We have changed-paths already. Keep them in the next graph */
> if (g && g->chunk_bloom_data) {
> ctx->changed_paths = 1;
> ctx->bloom_settings = g->bloom_filter_settings;
> }
> }
>
> , which drags forward Bloom filter settings across adjacent layers.
>
> This doesn't quite address all cases, however, since it is possible for
> intermediate layers to contain no Bloom filters at all. For example,
> suppose we have two layers in a commit-graph chain, say, {G1, G2}. If G1
> contains Bloom filters, but G2 doesn't, a new G3 (whose base graph is
> G2) may be written with arbitrary Bloom filter settings, because we only
> check the immediately adjacent layer's settings for compatibility.
>
> This behavior has existed since the introduction of changed-path Bloom
> filters. But in practice, this is not such a big deal, since the only
> way up until this point to modify the Bloom filter settings at write
> time is with the undocumented environment variables:
>
> - GIT_TEST_BLOOM_SETTINGS_BITS_PER_ENTRY
> - GIT_TEST_BLOOM_SETTINGS_NUM_HASHES
> - GIT_TEST_BLOOM_SETTINGS_MAX_CHANGED_PATHS
>
> (it is still possible to tweak MAX_CHANGED_PATHS between layers, but
> this does not affect reads, so is allowed to differ across multiple
> graph layers).
>
> But in future commits, we will introduce another parameter to change the
> hash algorithm used to compute Bloom fingerprints itself. This will be
> exposed via a configuration setting, making this foot-gun easier to use.
>
> To prevent this potential issue, validate that all layers of a split
> commit-graph have compatible settings with the newest layer which
> contains Bloom filters.
>
> Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
> Original-test-by: SZEDER Gábor <szeder.dev@gmail.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> commit-graph.c | 25 +++++++++++++++++
> t/t4216-log-bloom.sh | 64 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 89 insertions(+)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 1a56efcf69..ae0902f7f4 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -498,6 +498,30 @@ static int validate_mixed_generation_chain(struct commit_graph *g)
> return 0;
> }
>
> +static void validate_mixed_bloom_settings(struct commit_graph *g)
> +{
> + struct bloom_filter_settings *settings = NULL;
> + for (; g; g = g->base_graph) {
> + if (!g->bloom_filter_settings)
> + continue;
> + if (!settings) {
> + settings = g->bloom_filter_settings;
> + continue;
> + }
> +
> + if (g->bloom_filter_settings->bits_per_entry != settings->bits_per_entry ||
> + g->bloom_filter_settings->num_hashes != settings->num_hashes) {
> + g->chunk_bloom_indexes = NULL;
> + g->chunk_bloom_data = NULL;
> + FREE_AND_NULL(g->bloom_filter_settings);
> +
> + warning(_("disabling Bloom filters for commit-graph "
> + "layer '%s' due to incompatible settings"),
> + oid_to_hex(&g->oid));
> + }
> + }
> +}
> +
> static int add_graph_to_chain(struct commit_graph *g,
> struct commit_graph *chain,
> struct object_id *oids,
> @@ -614,6 +638,7 @@ struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,
> }
>
> validate_mixed_generation_chain(graph_chain);
> + validate_mixed_bloom_settings(graph_chain);
>
> free(oids);
> fclose(fp);
> diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
> index 322640feeb..f49a8f2fbf 100755
> --- a/t/t4216-log-bloom.sh
> +++ b/t/t4216-log-bloom.sh
> @@ -420,4 +420,68 @@ test_expect_success 'Bloom generation backfills empty commits' '
> )
> '
>
> +graph=.git/objects/info/commit-graph
> +graphdir=.git/objects/info/commit-graphs
> +chain=$graphdir/commit-graph-chain
> +
> +test_expect_success 'setup for mixed Bloom setting tests' '
> + repo=mixed-bloom-settings &&
> +
> + git init $repo &&
> + for i in one two three
> + do
> + test_commit -C $repo $i file || return 1
> + done
> +'
> +
> +test_expect_success 'split' '
> + # 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 \
> + --stdin-commits --changed-paths --split <in &&
> + layer=$(head -n 1 $repo/$chain) &&
> +
> + # A commit-graph layer without Bloom filters "hides" the layers
> + # below ...
> + git -C $repo rev-parse two >in &&
> + git -C $repo commit-graph write --stdin-commits --no-changed-paths \
> + --split=no-merge <in &&
> +
> + # Another commit-graph layer that has Bloom filters, but with
> + # standard settings, and is thus incompatible with the base
> + # layer written above.
> + git -C $repo rev-parse HEAD >in &&
> + git -C $repo commit-graph write --stdin-commits --changed-paths \
> + --split=no-merge <in &&
> +
> + test_line_count = 3 $repo/$chain &&
> +
> + # Ensure that incompatible Bloom filters are ignored.
> + git -C $repo -c core.commitGraph=false log --oneline --no-decorate -- file \
> + >expect 2>err &&
> + git -C $repo log --oneline --no-decorate -- file >actual 2>err &&
> + test_cmp expect actual &&
> + grep "disabling Bloom filters for commit-graph layer .$layer." err
> +'
Up to this point everything looks sensible to me.
> +test_expect_success 'merge graph layers with incompatible Bloom settings' '
> + # Ensure that incompatible Bloom filters are ignored when
> + # generating new layers.
> + git -C $repo commit-graph write --reachable --changed-paths 2>err &&
> + grep "disabling Bloom filters for commit-graph layer .$layer." err &&
> +
> + test_path_is_file $repo/$graph &&
> + test_dir_is_empty $repo/$graphdir &&
> +
> + # ...and merging existing ones.
> + git -C $repo -c core.commitGraph=false log --oneline --no-decorate -- file \
> + >expect 2>err &&
> + GIT_TRACE2_PERF="$(pwd)/trace.perf" \
> + git -C $repo log --oneline --no-decorate -- file >actual 2>err &&
But this test is a bit confusing to me, to be honest, also because the
comment for the second block here reads funny. We don't really merge
anything, do we? We only generate logs and compare that the log with and
without the resulting merged commit graph is the same. The actual logic
happened before.
> + test_cmp expect actual && cat err &&
The `cat err` looks like a leftover from debugging.
> + grep "statistics:{\"filter_not_present\":0" trace.perf &&
Also, why should the filter not be present here? If we merge the
commit-graphs with `--changed-paths` I'd have expected that we either
carry over bloom filters from preexisting commit graphs if compatible,
or otherwise generate them if they are either incompatible or don't
exist.
I feel like I'm missing something obvious, so this may be me just
missing the bigger picture.
> + ! grep "disabling Bloom filters" err
Can we make this assertion stricter and verify that `err` is empty? I
always think that `! grep` is quite a fragile pattern as it is quite
prone to becoming stale, e.g. when the error message itself would
change.
Patrick
> +'
> +
> test_done
> --
> 2.42.0.342.g8bb3a896ee
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 08/17] t4216: test changed path filters with high bit paths
From: Patrick Steinhardt @ 2023-10-17 8:45 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Jonathan Tan, Junio C Hamano, Jeff King, SZEDER Gábor
In-Reply-To: <cba766f224b0d2b4fd952b11bef8068c07dfcf88.1696969994.git.me@ttaylorr.com>
[-- Attachment #1: Type: text/plain, Size: 3157 bytes --]
On Tue, Oct 10, 2023 at 04:33:42PM -0400, Taylor Blau wrote:
> From: Jonathan Tan <jonathantanmy@google.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>
Nit: the signoffs are still funny here.
> ---
> t/t4216-log-bloom.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
> index f49a8f2fbf..da67c40134 100755
> --- a/t/t4216-log-bloom.sh
> +++ b/t/t4216-log-bloom.sh
> @@ -484,4 +484,56 @@ test_expect_success 'merge graph layers with incompatible Bloom settings' '
> ! grep "disabling Bloom filters" 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 &&
> + test_cmp expect 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
> +'
Doesn't this mean that the preceding test where we `test_cmp expect
actual` can fail on some platforms depending on the signedness of
`char`?
Patrick
> +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"
> + )
> +'
> +
> test_done
> --
> 2.42.0.342.g8bb3a896ee
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 05/17] t/helper/test-read-graph.c: extract `dump_graph_info()`
From: Patrick Steinhardt @ 2023-10-17 8:45 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Jonathan Tan, Junio C Hamano, Jeff King, SZEDER Gábor
In-Reply-To: <94552abf455c6d341a0811333ae4edb4a8cea259.1696969994.git.me@ttaylorr.com>
[-- Attachment #1: Type: text/plain, Size: 2160 bytes --]
On Tue, Oct 10, 2023 at 04:33:33PM -0400, Taylor Blau wrote:
> 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>
Nit: your signoff is duplicated here. This is also still the case for
some of the other commits.
Patrick
> ---
> 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.42.0.342.g8bb3a896ee
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 10/17] commit-graph: new filter ver. that fixes murmur3
From: Patrick Steinhardt @ 2023-10-17 8:45 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Jonathan Tan, Junio C Hamano, Jeff King, SZEDER Gábor
In-Reply-To: <61d44519a5ffaf2c040198cf8d80d05a09de5de5.1696969994.git.me@ttaylorr.com>
[-- Attachment #1: Type: text/plain, Size: 17868 bytes --]
On Tue, Oct 10, 2023 at 04:33:49PM -0400, Taylor Blau wrote:
> From: Jonathan Tan <jonathantanmy@google.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 | 32 ++++++--
> t/helper/test-bloom.c | 9 ++-
> t/t0095-bloom.sh | 8 ++
> t/t4216-log-bloom.sh | 105 +++++++++++++++++++++++++++
> 7 files changed, 223 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/config/commitgraph.txt b/Documentation/config/commitgraph.txt
> index 2dc9170622..acc74a2f27 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.
> + write. May be -1, 0, 1, or 2.
> +
> Defaults to -1.
> +
> @@ -28,4 +28,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 3e78cfe79d..ebef5cfd2f 100644
> --- a/bloom.c
> +++ b/bloom.c
> @@ -66,7 +66,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;
> @@ -131,8 +188,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 ea677c87fb..db623afd09 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -314,17 +314,26 @@ static int graph_read_oid_lookup(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;
> - g->chunk_bloom_data = chunk_start;
> hash_version = get_be32(chunk_start);
>
> - if (hash_version != 1)
> + 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;
> + }
In case we have `c->commit_graph_changed_paths_version == -1` we lose
the check that the hash version is something that we know and support,
don't we? And while we do start to handle `-1` in the writing path, I
think we don't in the reading path unless I missed something.
> + g->chunk_bloom_data = chunk_start;
> 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);
> @@ -412,10 +421,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
> + };
> pair_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,
> &graph->chunk_bloom_indexes);
> 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) {
> @@ -2441,6 +2454,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;
> @@ -2453,6 +2473,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",
> @@ -2482,7 +2504,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 aabe31d724..3cbc0a5b50 100644
> --- a/t/helper/test-bloom.c
> +++ b/t/helper/test-bloom.c
> @@ -50,6 +50,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";
>
> @@ -64,7 +65,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 da67c40134..8f8b5d4966 100755
> --- a/t/t4216-log-bloom.sh
> +++ b/t/t4216-log-bloom.sh
> @@ -536,4 +536,109 @@ 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 &&
> + 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
> + )
> +'
> +
With the supposedly missing check in mind, should we also add tests for
currently unknown versions like 3 or -2?
Patrick
> test_done
> --
> 2.42.0.342.g8bb3a896ee
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 13/17] commit-graph.c: unconditionally load Bloom filters
From: Patrick Steinhardt @ 2023-10-17 8:45 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Jonathan Tan, Junio C Hamano, Jeff King, SZEDER Gábor
In-Reply-To: <09d8669c3a074e7a2ace9d650a345244b2362f7e.1696969994.git.me@ttaylorr.com>
[-- Attachment #1: Type: text/plain, Size: 4073 bytes --]
On Tue, Oct 10, 2023 at 04:33:59PM -0400, Taylor Blau wrote:
> In 9e4df4da07 (commit-graph: new filter ver. that fixes murmur3,
Nit: It's a bit funny to read this reference to a commit ID when the
commit in question is part of the same series. Isn't it likely to grow
stale?
> 2023-08-01), 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 | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index db623afd09..fa3b58e762 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -327,12 +327,6 @@ static int graph_read_bloom_data(const unsigned char *chunk_start,
> uint32_t hash_version;
> 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->bloom_filter_settings = xmalloc(sizeof(struct bloom_filter_settings));
> g->bloom_filter_settings->hash_version = hash_version;
> @@ -2473,8 +2467,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",
> @@ -2506,10 +2499,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;
> +
What if there is a future version of Git that writes Bloom filters with
hash version 3? Should we really normalize that to `1`?
Patrick
> if (ctx->split) {
> struct commit_graph *g = ctx->r->objects->commit_graph;
>
> --
> 2.42.0.342.g8bb3a896ee
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 00/17] bloom: changed-path Bloom filters v2 (& sundries)
From: Patrick Steinhardt @ 2023-10-17 8:45 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Jonathan Tan, Junio C Hamano, Jeff King, SZEDER Gábor
In-Reply-To: <cover.1696969994.git.me@ttaylorr.com>
[-- Attachment #1: Type: text/plain, Size: 10543 bytes --]
On Tue, Oct 10, 2023 at 04:33:17PM -0400, Taylor Blau wrote:
> (Rebased onto the tip of 'master', which is 3a06386e31 (The fifteenth
> batch, 2023-10-04), at the time of writing).
>
> This series is a reroll of the combined efforts of [1] and [2] to
> introduce the v2 changed-path Bloom filters, which fixes a bug in our
> existing implementation of murmur3 paths with non-ASCII characters (when
> the "char" type is signed).
>
> In large part, this is the same as the previous round. But this round
> includes some extra bits that address issues pointed out by SZEDER
> Gábor, which are:
>
> - not reading Bloom filters for root commits
> - corrupting Bloom filter reads by tweaking the filter settings
> between layers.
>
> These issues were discussed in (among other places) [3], and [4],
> respectively.
>
> Thanks to Jonathan, Peff, and SZEDER who have helped a great deal in
> assembling these patches. As usual, a range-diff is included below.
> Thanks in advance for your
> review!
As this patch series has been sitting around without reviews for a week
I've tried my best to give it a go. Note though that this area is mostly
outside of my own comfort zone, so some of the questions and suggestions
might ultimately not apply.
Patrick
> [1]: https://lore.kernel.org/git/cover.1684790529.git.jonathantanmy@google.com/
> [2]: https://lore.kernel.org/git/cover.1691426160.git.me@ttaylorr.com/
> [3]: https://public-inbox.org/git/20201015132147.GB24954@szeder.dev/
> [4]: https://lore.kernel.org/git/20230830200218.GA5147@szeder.dev/
>
> Jonathan Tan (4):
> gitformat-commit-graph: describe version 2 of BDAT
> t4216: test changed path filters with high bit paths
> repo-settings: introduce commitgraph.changedPathsVersion
> commit-graph: new filter ver. that fixes murmur3
>
> Taylor Blau (13):
> t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
> revision.c: consult Bloom filters for root commits
> commit-graph: ensure Bloom filters are read with consistent settings
> t/helper/test-read-graph.c: extract `dump_graph_info()`
> bloom.h: make `load_bloom_filter_from_graph()` public
> t/helper/test-read-graph: implement `bloom-filters` mode
> bloom: annotate filters with hash version
> bloom: prepare to discard incompatible Bloom filters
> commit-graph.c: unconditionally load Bloom filters
> commit-graph: drop unnecessary `graph_read_bloom_data_context`
> object.h: fix mis-aligned flag bits table
> commit-graph: reuse existing Bloom filters where possible
> bloom: introduce `deinit_bloom_filters()`
>
> Documentation/config/commitgraph.txt | 26 ++-
> Documentation/gitformat-commit-graph.txt | 9 +-
> bloom.c | 208 ++++++++++++++++-
> bloom.h | 38 +++-
> commit-graph.c | 61 ++++-
> object.h | 3 +-
> oss-fuzz/fuzz-commit-graph.c | 2 +-
> repo-settings.c | 6 +-
> repository.h | 2 +-
> revision.c | 26 ++-
> t/helper/test-bloom.c | 9 +-
> t/helper/test-read-graph.c | 67 ++++--
> t/t0095-bloom.sh | 8 +
> t/t4216-log-bloom.sh | 272 ++++++++++++++++++++++-
> 14 files changed, 682 insertions(+), 55 deletions(-)
>
> Range-diff against v2:
> 10: 002a06d1e9 ! 1: fe671d616c t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
> @@ Commit message
> indicating that no filters were used.
>
> 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 ##
> @@ t/t4216-log-bloom.sh: test_bloom_filters_used () {
> -: ---------- > 2: 7d0fa93543 revision.c: consult Bloom filters for root commits
> -: ---------- > 3: 2ecc0a2d58 commit-graph: ensure Bloom filters are read with consistent settings
> 1: 5fa681b58e ! 4: 17703ed89a gitformat-commit-graph: describe version 2 of BDAT
> @@ Commit message
> 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/gitformat-commit-graph.txt ##
> @@ Documentation/gitformat-commit-graph.txt: All multi-byte numbers are in network byte order.
> 2: 623d840575 ! 5: 94552abf45 t/helper/test-read-graph.c: extract `dump_graph_info()`
> @@ Commit message
> 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/helper/test-read-graph.c ##
> @@
> 3: bc9d77ae60 ! 6: 3d81efa27b bloom.h: make `load_bloom_filter_from_graph()` public
> @@ Commit message
> 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>
>
> ## bloom.c ##
> @@ bloom.c: static inline unsigned char get_bitmask(uint32_t pos)
> 4: ac7008aed3 ! 7: d23cd89037 t/helper/test-read-graph: implement `bloom-filters` mode
> @@ Commit message
> 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/helper/test-read-graph.c ##
> @@ t/helper/test-read-graph.c: static void dump_graph_info(struct commit_graph *graph)
> @@ t/helper/test-read-graph.c: int cmd__read_graph(int argc UNUSED, const char **ar
> - return 0;
> + return ret;
> }
> ++
> ++
> 5: 71755ba856 ! 8: cba766f224 t4216: test changed path filters with high bit paths
> @@ Commit message
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
>
> ## t/t4216-log-bloom.sh ##
> -@@ t/t4216-log-bloom.sh: test_expect_success 'Bloom generation backfills empty commits' '
> - )
> +@@ t/t4216-log-bloom.sh: test_expect_success 'merge graph layers with incompatible Bloom settings' '
> + ! grep "disabling Bloom filters" err
> '
>
> +get_first_changed_path_filter () {
> 6: 9768d92c0f ! 9: a08a961f41 repo-settings: introduce commitgraph.changedPathsVersion
> @@ Commit message
> 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 ##
> @@ Documentation/config/commitgraph.txt: commitGraph.maxNewFilters::
> 7: f911b4bfab = 10: 61d44519a5 commit-graph: new filter ver. that fixes murmur3
> 8: 35009900df ! 11: a8c10f8de8 bloom: annotate filters with hash version
> @@ Commit message
> Bloom filter.
>
> 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 ##
> @@ bloom.c: int load_bloom_filter_from_graph(struct commit_graph *g,
> 9: 138bc16905 ! 12: 2ba10a4b4b bloom: prepare to discard incompatible Bloom filters
> @@ Commit message
> `get_or_compute_bloom_filter()`.
>
> 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 ##
> @@ bloom.c: static void init_truncated_large_filter(struct bloom_filter *filter,
> 11: 2437e62813 ! 13: 09d8669c3a commit-graph.c: unconditionally load Bloom filters
> @@ Commit message
> either "1" or "2".
>
> 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>
>
> ## commit-graph.c ##
> @@ commit-graph.c: static int graph_read_bloom_data(const unsigned char *chunk_start,
> 12: fe8fb2f5fe ! 14: 0d4f9dc4ee commit-graph: drop unnecessary `graph_read_bloom_data_context`
> @@ Commit message
>
> Noticed-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>
>
> ## commit-graph.c ##
> @@ commit-graph.c: static int graph_read_oid_lookup(const unsigned char *chunk_start,
> 13: 825af91e11 ! 15: 1f7f27bc47 object.h: fix mis-aligned flag bits table
> @@ Commit message
> Bit position 23 is one column too far to the left.
>
> 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>
>
> ## object.h ##
> @@ object.h: void object_array_init(struct object_array *array);
> 14: 593b317192 ! 16: abbef95ae8 commit-graph: reuse existing Bloom filters where possible
> @@ Commit message
> commits by their generation number.
>
> 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 ##
> @@
> 15: 8bf2c9cf98 = 17: ca362408d5 bloom: introduce `deinit_bloom_filters()`
> --
> 2.42.0.342.g8bb3a896ee
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Git Pathspec bug
From: Moritz Widmann @ 2023-10-17 9:45 UTC (permalink / raw)
To: git
I executed the following command in zsh (added `command` just to be sure
that there's no aliases or functions)
command git submodule add
'git@github.com:moritz-t-w/Godot-Onscreen-Keyboard.git' '.'
fatal: empty string is not a valid pathspec. please use . instead if you
meant to match all paths
Git Version: 2.42.0
OS: Arch Linux
^ permalink raw reply
* Re: Git Pathspec bug
From: Kristoffer Haugsbakk @ 2023-10-17 10:51 UTC (permalink / raw)
To: Moritz Widmann; +Cc: git
In-Reply-To: <7368e4ad-b05b-4b8f-a13b-0a68b442e72b@tweaklab.org>
On Tue, Oct 17, 2023, at 11:45, Moritz Widmann wrote:
> I executed the following command in zsh (added `command` just to be sure
> that there's no aliases or functions)
>
> command git submodule add
> 'git@github.com:moritz-t-w/Godot-Onscreen-Keyboard.git' '.'
> fatal: empty string is not a valid pathspec. please use . instead if you
> meant to match all paths
>
> Git Version: 2.42.0
>
> OS: Arch Linux
Is this the same issue?: https://stackoverflow.com/a/53441183/1725151
^ permalink raw reply
* Re: Method for Calculating Statistics of Developer Contribution to a Specified Branch.
From: Hongyi Zhao @ 2023-10-17 11:37 UTC (permalink / raw)
To: brian m. carlson, Hongyi Zhao, Git List
In-Reply-To: <ZS2qZtYDvItovjqg@tapette.crustytoothpaste.net>
On Tue, Oct 17, 2023 at 5:26 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2023-10-16 at 14:10:01, Hongyi Zhao wrote:
> > Dear Git Mailing List,
> >
> > I am a developer currently working on a project and I wanted to
> > establish statistics for each team member's contribution to a specific
> > branch.
> >
> > Say, for a user "JianboLin", I am currently using the following method:
> >
> > $ git clone https://github.com/OrderN/CONQUEST-release.git
> > $ cd CONQUEST-release
> > $ git log --author="JianboLin" --stat --summary origin/f-mlff | awk
> > 'NF ==4 && $2 =="|" && $3 ~/[0-9]+/ && $4 ~/[+-]+|[+]+|[-]+/ {s+=$3}
> > END {print s}'
> >
> > Using the above command, I am able to calculate the number of lines
> > contributed by a specific author on a specific branch, which allows me
> > to quantify the contribution to a branch by each team member.
> >
> > However, I would like to know if a more efficient or accurate method
> > exists to carry out this task. Are there any other parameters,
> > commands, or aspects I need to consider to get a more comprehensive
> > measure of contribution?
>
> Can you maybe explain what you want to measure and what your goal is in
> doing so?
>
> The problem is that lines of code isn't really that useful as a measure
> of contribution value or developer productivity, which are the reasons
> people typically measure that metric. For example, with three lines, a
> colleague fixed a persistently difficult-to-reproduce problem which had
> been affecting many of our largest customers. That was a very valuable
> contribution, but not very large. I've made similar kinds of changes
> myself, both at work and in open source projects.
>
> Certainly you can compute the number of lines of code changed by a
> developer, but that is not typically a very useful metric, since it
> doesn't lead you to any interesting conclusions about the benefits or
> value of the contributions or developer in question. However, perhaps
> you have a different goal in mind, and if you can explain what that is,
> we may be able to help you find a better way of doing it.
I want to calculate a certain developer's contribution based on
different standards of code line count and the importance of the code.
> --
> brian m. carlson (he/him or they/them)
> Toronto, Ontario, CA
Regards,
Zhao
^ permalink raw reply
* [PATCH v2 1/1] [OUTREACHY] add: standardize die() messages output.
From: Naomi Ibe @ 2023-10-17 11:39 UTC (permalink / raw)
To: git; +Cc: Naomi Ibe
builtin/add.c: clean up die() messages
As described in the CodingGuidelines document, a single line
message given to die() and its friends should not capitalize its
first word, and should not add full-stop at the end.
Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
---
builtin/add.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index c27254a5cd..5126d2ede3 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -182,7 +182,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
if (repo_read_index(the_repository) < 0)
- die(_("Could not read the index"));
+ die(_("could not read the index"));
repo_init_revisions(the_repository, &rev, prefix);
rev.diffopt.context = 7;
@@ -200,15 +200,15 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
die(_("editing patch failed"));
if (stat(file, &st))
- die_errno(_("Could not stat '%s'"), file);
+ die_errno(_("could not stat '%s'"), file);
if (!st.st_size)
- die(_("Empty patch. Aborted."));
+ die(_("empty patch. aborted"));
child.git_cmd = 1;
strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
NULL);
if (run_command(&child))
- die(_("Could not apply '%s'"), file);
+ die(_("could not apply '%s'"), file);
unlink(file);
free(file);
@@ -568,7 +568,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
finish:
if (write_locked_index(&the_index, &lock_file,
COMMIT_LOCK | SKIP_IF_UNCHANGED))
- die(_("Unable to write new index file"));
+ die(_("unable to write new index file"));
dir_clear(&dir);
clear_pathspec(&pathspec);
--
2.36.1.windows.1
^ permalink raw reply related
* Re: [PATCH v2 2/2] Prevent git from rehashing 4GiB files
From: Jason Hatton @ 2023-10-17 14:49 UTC (permalink / raw)
To: Jeff King, brian m. carlson
Cc: git@vger.kernel.org, Junio C Hamano, Taylor Blau
In-Reply-To: <20231017000019.GB551672@coredump.intra.peff.net>
From: Jeff King <peff@peff.net>
> On Thu, Oct 12, 2023 at 04:09:30PM +0000, brian m. carlson wrote:
>
> > +static unsigned int munge_st_size(off_t st_size) {
> > + unsigned int sd_size = st_size;
> > +
> > + /*
> > + * If the file is an exact multiple of 4 GiB, modify the value so it
> > + * doesn't get marked as racily clean (zero).
> > + */
> > + if (!sd_size && st_size)
> > + return 0x80000000;
> > + else
> > + return sd_size;
> > +}
>
> Coverity complained that the "true" side of this conditional is
> unreachable, since sd_size is assigned from st_size, so the two values
> cannot be both true and false. But obviously we are depending here on
> the truncation of off_t to "unsigned int". I'm not sure if Coverity is
> just dumb, or if it somehow has a different size for off_t.
>
> I don't _think_ this would ever cause confusion in a real compiler, as
> assignment from a larger type to a smaller has well-defined truncation,
> as far as I know.
>
> But I do wonder if an explicit "& 0xFFFFFFFF" would make it more obvious
> what is happening (which would also do the right thing if in some
> hypothetical platform "unsigned int" ended up larger than 32 bits).
>
> -Peff
I originally wrote the code this way to work exactly like the original
code with one exception: Never truncate a nonzero st_size to a zero
sd_size. The original code is here in fill_stat_data:
I was attempting to use exactly the same implicit type conversion and
types as the original.
We could probably write the below to do the same thing.
void fill_stat_data(struct stat_data *sd, struct stat *st)
{
sd->sd_ctime.sec = (unsigned int)st->st_ctime;
sd->sd_mtime.sec = (unsigned int)st->st_mtime;
sd->sd_ctime.nsec = ST_CTIME_NSEC(*st);
sd->sd_mtime.nsec = ST_MTIME_NSEC(*st);
sd->sd_dev = st->st_dev;
sd->sd_ino = st->st_ino;
sd->sd_uid = st->st_uid;
sd->sd_gid = st->st_gid;
sd->sd_size = st->st_size;
+ if (sd->sd_size == 0 && st->st_size!= 0) {
+ sd->sd_size = 1;
+ }
}
- Jason D. Hatton
^ permalink raw reply
* Re: Git Pathspec bug
From: Junio C Hamano @ 2023-10-17 16:02 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Moritz Widmann, git
In-Reply-To: <2c45e813-738a-480f-8c77-8c646df9c0e3@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> On Tue, Oct 17, 2023, at 11:45, Moritz Widmann wrote:
>> I executed the following command in zsh (added `command` just to be sure
>> that there's no aliases or functions)
>>
>> command git submodule add
>> 'git@github.com:moritz-t-w/Godot-Onscreen-Keyboard.git' '.'
>> fatal: empty string is not a valid pathspec. please use . instead if you
>> meant to match all paths
>>
>> Git Version: 2.42.0
>>
>> OS: Arch Linux
>
> Is this the same issue?: https://stackoverflow.com/a/53441183/1725151
It does look so.
It is correct to reject such a request to attempt to add a submodule
as if it is overlayed at the same level as its superproject [*].
But the error message is totally bogus, I think. It is not that the
pathspec the end-user gave us is wrong (the user does not even give
a pathspec in this case---the last one must be a concrete path in
the superproject where the newly added submodule is), and the user
should not be told anything about "valid" pathspec.
Patches welcome ;-)
Thanks.
[Footnote]
* Our submodules do not allow such a layout (and "git add foo" in
such an environment would not know to which repository between the
submodule or the superproject that new file "foo" should be added,
which is just one example why such a layout is not usable).
^ permalink raw reply
* [PATCH v2 0/7] merge-ort: implement support for packing objects together
From: Taylor Blau @ 2023-10-17 16:31 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1696629697.git.me@ttaylorr.com>
(Previously based on 'eb/limit-bulk-checkin-to-blobs', which has since
been merged. This series is now based on the tip of 'master', which is
a9ecda2788 (The eighteenth batch, 2023-10-13) at the time of writing).
This series implements support for a new merge-tree option,
`--write-pack`, which causes any newly-written objects to be packed
together instead of being stored individually as loose.
Much is unchanged since last time, except for a small tweak to one of
the commit messages in response to feedback from Eric W. Biederman. The
series has also been rebased onto 'master', which had a couple of
conflicts that I resolved pertaining to:
- 9eb5419799 (bulk-checkin: only support blobs in index_bulk_checkin,
2023-09-26)
- e0b8c84240 (treewide: fix various bugs w/ OpenSSL 3+ EVP API,
2023-09-01)
They were mostly trivial resolutions, and the results can be viewed in
the range-diff included below.
(From last time: the motivating use-case behind these changes is to
better support repositories who invoke merge-tree frequently, generating
a potentially large number of loose objects, resulting in a possible
adverse effect on performance.)
Thanks in advance for any review!
Taylor Blau (7):
bulk-checkin: factor out `format_object_header_hash()`
bulk-checkin: factor out `prepare_checkpoint()`
bulk-checkin: factor out `truncate_checkpoint()`
bulk-checkin: factor our `finalize_checkpoint()`
bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
builtin/merge-tree.c: implement support for `--write-pack`
Documentation/git-merge-tree.txt | 4 +
builtin/merge-tree.c | 5 +
bulk-checkin.c | 258 ++++++++++++++++++++++++++-----
bulk-checkin.h | 8 +
merge-ort.c | 42 +++--
merge-recursive.h | 1 +
t/t4301-merge-tree-write-tree.sh | 93 +++++++++++
7 files changed, 363 insertions(+), 48 deletions(-)
Range-diff against v1:
1: 37f4072815 ! 1: edf1cbafc1 bulk-checkin: factor out `format_object_header_hash()`
@@ bulk-checkin.c: static void prepare_to_stream(struct bulk_checkin_packfile *stat
}
+static void format_object_header_hash(const struct git_hash_algo *algop,
-+ git_hash_ctx *ctx, enum object_type type,
++ git_hash_ctx *ctx,
++ struct hashfile_checkpoint *checkpoint,
++ enum object_type type,
+ size_t size)
+{
+ unsigned char header[16384];
@@ bulk-checkin.c: static void prepare_to_stream(struct bulk_checkin_packfile *stat
+
+ algop->init_fn(ctx);
+ algop->update_fn(ctx, header, header_len);
++ algop->init_fn(&checkpoint->ctx);
+}
+
static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
@@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st
- OBJ_BLOB, size);
- the_hash_algo->init_fn(&ctx);
- the_hash_algo->update_fn(&ctx, obuf, header_len);
-+ format_object_header_hash(the_hash_algo, &ctx, OBJ_BLOB, size);
+- the_hash_algo->init_fn(&checkpoint.ctx);
++ format_object_header_hash(the_hash_algo, &ctx, &checkpoint, OBJ_BLOB,
++ size);
/* Note: idx is non-NULL when we are writing */
if ((flags & HASH_WRITE_OBJECT) != 0)
2: 9cc1f3014a ! 2: b3f89d5853 bulk-checkin: factor out `prepare_checkpoint()`
@@ Commit message
## bulk-checkin.c ##
@@ bulk-checkin.c: static void format_object_header_hash(const struct git_hash_algo *algop,
- algop->update_fn(ctx, header, header_len);
+ algop->init_fn(&checkpoint->ctx);
}
+static void prepare_checkpoint(struct bulk_checkin_packfile *state,
3: f392ed2211 = 3: abe4fb0a59 bulk-checkin: factor out `truncate_checkpoint()`
4: 9c6ca564ad = 4: 0b855a6eb7 bulk-checkin: factor our `finalize_checkpoint()`
5: 30ca7334c7 ! 5: 239bf39bfb bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
@@ bulk-checkin.c: static void finalize_checkpoint(struct bulk_checkin_packfile *st
+static int deflate_obj_contents_to_pack_incore(struct bulk_checkin_packfile *state,
+ git_hash_ctx *ctx,
++ struct hashfile_checkpoint *checkpoint,
+ struct object_id *result_oid,
+ const void *buf, size_t size,
+ enum object_type type,
+ const char *path, unsigned flags)
+{
-+ struct hashfile_checkpoint checkpoint = {0};
+ struct pack_idx_entry *idx = NULL;
+ off_t already_hashed_to = 0;
+
@@ bulk-checkin.c: static void finalize_checkpoint(struct bulk_checkin_packfile *st
+ CALLOC_ARRAY(idx, 1);
+
+ while (1) {
-+ prepare_checkpoint(state, &checkpoint, idx, flags);
++ prepare_checkpoint(state, checkpoint, idx, flags);
+ if (!stream_obj_to_pack_incore(state, ctx, &already_hashed_to,
+ buf, size, type, path, flags))
+ break;
-+ truncate_checkpoint(state, &checkpoint, idx);
++ truncate_checkpoint(state, checkpoint, idx);
+ }
+
-+ finalize_checkpoint(state, ctx, &checkpoint, idx, result_oid);
++ finalize_checkpoint(state, ctx, checkpoint, idx, result_oid);
+
+ return 0;
+}
@@ bulk-checkin.c: static void finalize_checkpoint(struct bulk_checkin_packfile *st
+ const char *path, unsigned flags)
+{
+ git_hash_ctx ctx;
++ struct hashfile_checkpoint checkpoint = {0};
+
-+ format_object_header_hash(the_hash_algo, &ctx, OBJ_BLOB, size);
++ format_object_header_hash(the_hash_algo, &ctx, &checkpoint, OBJ_BLOB,
++ size);
+
-+ return deflate_obj_contents_to_pack_incore(state, &ctx, result_oid,
-+ buf, size, OBJ_BLOB, path,
-+ flags);
++ return deflate_obj_contents_to_pack_incore(state, &ctx, &checkpoint,
++ result_oid, buf, size,
++ OBJ_BLOB, path, flags);
+}
+
static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
6: cb0f79cabb ! 6: 57613807d8 bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
@@ Commit message
Within `deflate_tree_to_pack_incore()`, the changes should be limited
to something like:
+ struct strbuf converted = STRBUF_INIT;
if (the_repository->compat_hash_algo) {
- struct strbuf converted = STRBUF_INIT;
if (convert_object_file(&compat_obj,
the_repository->hash_algo,
the_repository->compat_hash_algo, ...) < 0)
@@ Commit message
format_object_header_hash(the_repository->compat_hash_algo,
OBJ_TREE, size);
-
- strbuf_release(&converted);
}
+ /* compute the converted tree's hash using the compat algorithm */
+ strbuf_release(&converted);
, assuming related changes throughout the rest of the bulk-checkin
machinery necessary to update the hash of the converted object, which
@@ Commit message
## bulk-checkin.c ##
@@ bulk-checkin.c: static int deflate_blob_to_pack_incore(struct bulk_checkin_packfile *state,
- flags);
+ OBJ_BLOB, path, flags);
}
+static int deflate_tree_to_pack_incore(struct bulk_checkin_packfile *state,
@@ bulk-checkin.c: static int deflate_blob_to_pack_incore(struct bulk_checkin_packf
+ const char *path, unsigned flags)
+{
+ git_hash_ctx ctx;
++ struct hashfile_checkpoint checkpoint = {0};
+
-+ format_object_header_hash(the_hash_algo, &ctx, OBJ_TREE, size);
++ format_object_header_hash(the_hash_algo, &ctx, &checkpoint, OBJ_TREE,
++ size);
+
-+ return deflate_obj_contents_to_pack_incore(state, &ctx, result_oid,
-+ buf, size, OBJ_TREE, path,
-+ flags);
++ return deflate_obj_contents_to_pack_incore(state, &ctx, &checkpoint,
++ result_oid, buf, size,
++ OBJ_TREE, path, flags);
+}
+
static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
7: e969210145 ! 7: f21400f56c builtin/merge-tree.c: implement support for `--write-pack`
@@ merge-ort.c
* We have many arrays of size 3. Whenever we have such an array, the
@@ merge-ort.c: static int handle_content_merge(struct merge_options *opt,
if ((merge_status < 0) || !result_buf.ptr)
- ret = err(opt, _("Failed to execute internal merge"));
+ ret = error(_("failed to execute internal merge"));
- if (!ret &&
- write_object_file(result_buf.ptr, result_buf.size,
- OBJ_BLOB, &result->oid))
-- ret = err(opt, _("Unable to add %s to database"),
-- path);
+- ret = error(_("unable to add %s to database"), path);
+ if (!ret) {
+ ret = opt->write_pack
+ ? index_blob_bulk_checkin_incore(&result->oid,
@@ merge-ort.c: static int handle_content_merge(struct merge_options *opt,
+ result_buf.size,
+ OBJ_BLOB, &result->oid);
+ if (ret)
-+ ret = err(opt, _("Unable to add %s to database"),
-+ path);
++ ret = error(_("unable to add %s to database"),
++ path);
+ }
free(result_buf.ptr);
--
2.42.0.405.gdb2a2f287e
^ permalink raw reply
* [PATCH v2 1/7] bulk-checkin: factor out `format_object_header_hash()`
From: Taylor Blau @ 2023-10-17 16:31 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697560266.git.me@ttaylorr.com>
Before deflating a blob into a pack, the bulk-checkin mechanism prepares
the pack object header by calling `format_object_header()`, and writing
into a scratch buffer, the contents of which eventually makes its way
into the pack.
Future commits will add support for deflating multiple kinds of objects
into a pack, and will likewise need to perform a similar operation as
below.
This is a mostly straightforward extraction, with one notable exception.
Instead of hard-coding `the_hash_algo`, pass it in to the new function
as an argument. This isn't strictly necessary for our immediate purposes
here, but will prove useful in the future if/when the bulk-checkin
mechanism grows support for the hash transition plan.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 6ce62999e5..fd3c110d1c 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -247,6 +247,22 @@ static void prepare_to_stream(struct bulk_checkin_packfile *state,
die_errno("unable to write pack header");
}
+static void format_object_header_hash(const struct git_hash_algo *algop,
+ git_hash_ctx *ctx,
+ struct hashfile_checkpoint *checkpoint,
+ enum object_type type,
+ size_t size)
+{
+ unsigned char header[16384];
+ unsigned header_len = format_object_header((char *)header,
+ sizeof(header),
+ type, size);
+
+ algop->init_fn(ctx);
+ algop->update_fn(ctx, header, header_len);
+ algop->init_fn(&checkpoint->ctx);
+}
+
static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
struct object_id *result_oid,
int fd, size_t size,
@@ -254,8 +270,6 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
{
off_t seekback, already_hashed_to;
git_hash_ctx ctx;
- unsigned char obuf[16384];
- unsigned header_len;
struct hashfile_checkpoint checkpoint = {0};
struct pack_idx_entry *idx = NULL;
@@ -263,11 +277,8 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
if (seekback == (off_t) -1)
return error("cannot find the current offset");
- header_len = format_object_header((char *)obuf, sizeof(obuf),
- OBJ_BLOB, size);
- the_hash_algo->init_fn(&ctx);
- the_hash_algo->update_fn(&ctx, obuf, header_len);
- the_hash_algo->init_fn(&checkpoint.ctx);
+ format_object_header_hash(the_hash_algo, &ctx, &checkpoint, OBJ_BLOB,
+ size);
/* Note: idx is non-NULL when we are writing */
if ((flags & HASH_WRITE_OBJECT) != 0)
--
2.42.0.405.gdb2a2f287e
^ permalink raw reply related
* [PATCH v2 2/7] bulk-checkin: factor out `prepare_checkpoint()`
From: Taylor Blau @ 2023-10-17 16:31 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697560266.git.me@ttaylorr.com>
In a similar spirit as the previous commit, factor out the routine to
prepare streaming into a bulk-checkin pack into its own function. Unlike
the previous patch, this is a verbatim copy and paste.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index fd3c110d1c..c1f5450583 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -263,6 +263,19 @@ static void format_object_header_hash(const struct git_hash_algo *algop,
algop->init_fn(&checkpoint->ctx);
}
+static void prepare_checkpoint(struct bulk_checkin_packfile *state,
+ struct hashfile_checkpoint *checkpoint,
+ struct pack_idx_entry *idx,
+ unsigned flags)
+{
+ prepare_to_stream(state, flags);
+ if (idx) {
+ hashfile_checkpoint(state->f, checkpoint);
+ idx->offset = state->offset;
+ crc32_begin(state->f);
+ }
+}
+
static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
struct object_id *result_oid,
int fd, size_t size,
@@ -287,12 +300,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
already_hashed_to = 0;
while (1) {
- prepare_to_stream(state, flags);
- if (idx) {
- hashfile_checkpoint(state->f, &checkpoint);
- idx->offset = state->offset;
- crc32_begin(state->f);
- }
+ prepare_checkpoint(state, &checkpoint, idx, flags);
if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
fd, size, path, flags))
break;
--
2.42.0.405.gdb2a2f287e
^ permalink raw reply related
* [PATCH v2 3/7] bulk-checkin: factor out `truncate_checkpoint()`
From: Taylor Blau @ 2023-10-17 16:31 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697560266.git.me@ttaylorr.com>
In a similar spirit as previous commits, factor our the routine to
truncate a bulk-checkin packfile when writing past the pack size limit.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index c1f5450583..b92d7a6f5a 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -276,6 +276,22 @@ static void prepare_checkpoint(struct bulk_checkin_packfile *state,
}
}
+static void truncate_checkpoint(struct bulk_checkin_packfile *state,
+ struct hashfile_checkpoint *checkpoint,
+ struct pack_idx_entry *idx)
+{
+ /*
+ * Writing this object to the current pack will make
+ * it too big; we need to truncate it, start a new
+ * pack, and write into it.
+ */
+ if (!idx)
+ BUG("should not happen");
+ hashfile_truncate(state->f, checkpoint);
+ state->offset = checkpoint->offset;
+ flush_bulk_checkin_packfile(state);
+}
+
static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
struct object_id *result_oid,
int fd, size_t size,
@@ -304,16 +320,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
fd, size, path, flags))
break;
- /*
- * Writing this object to the current pack will make
- * it too big; we need to truncate it, start a new
- * pack, and write into it.
- */
- if (!idx)
- BUG("should not happen");
- hashfile_truncate(state->f, &checkpoint);
- state->offset = checkpoint.offset;
- flush_bulk_checkin_packfile(state);
+ truncate_checkpoint(state, &checkpoint, idx);
if (lseek(fd, seekback, SEEK_SET) == (off_t) -1)
return error("cannot seek back");
}
--
2.42.0.405.gdb2a2f287e
^ permalink raw reply related
* [PATCH v2 4/7] bulk-checkin: factor our `finalize_checkpoint()`
From: Taylor Blau @ 2023-10-17 16:31 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697560266.git.me@ttaylorr.com>
In a similar spirit as previous commits, factor out the routine to
finalize the just-written object from the bulk-checkin mechanism.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 41 +++++++++++++++++++++++++----------------
1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index b92d7a6f5a..f4914fb6d1 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -292,6 +292,30 @@ static void truncate_checkpoint(struct bulk_checkin_packfile *state,
flush_bulk_checkin_packfile(state);
}
+static void finalize_checkpoint(struct bulk_checkin_packfile *state,
+ git_hash_ctx *ctx,
+ struct hashfile_checkpoint *checkpoint,
+ struct pack_idx_entry *idx,
+ struct object_id *result_oid)
+{
+ the_hash_algo->final_oid_fn(result_oid, ctx);
+ if (!idx)
+ return;
+
+ idx->crc32 = crc32_end(state->f);
+ if (already_written(state, result_oid)) {
+ hashfile_truncate(state->f, checkpoint);
+ state->offset = checkpoint->offset;
+ free(idx);
+ } else {
+ oidcpy(&idx->oid, result_oid);
+ ALLOC_GROW(state->written,
+ state->nr_written + 1,
+ state->alloc_written);
+ state->written[state->nr_written++] = idx;
+ }
+}
+
static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
struct object_id *result_oid,
int fd, size_t size,
@@ -324,22 +348,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
if (lseek(fd, seekback, SEEK_SET) == (off_t) -1)
return error("cannot seek back");
}
- the_hash_algo->final_oid_fn(result_oid, &ctx);
- if (!idx)
- return 0;
-
- idx->crc32 = crc32_end(state->f);
- if (already_written(state, result_oid)) {
- hashfile_truncate(state->f, &checkpoint);
- state->offset = checkpoint.offset;
- free(idx);
- } else {
- oidcpy(&idx->oid, result_oid);
- ALLOC_GROW(state->written,
- state->nr_written + 1,
- state->alloc_written);
- state->written[state->nr_written++] = idx;
- }
+ finalize_checkpoint(state, &ctx, &checkpoint, idx, result_oid);
return 0;
}
--
2.42.0.405.gdb2a2f287e
^ permalink raw reply related
* [PATCH v2 5/7] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
From: Taylor Blau @ 2023-10-17 16:31 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697560266.git.me@ttaylorr.com>
Now that we have factored out many of the common routines necessary to
index a new object into a pack created by the bulk-checkin machinery, we
can introduce a variant of `index_blob_bulk_checkin()` that acts on
blobs whose contents we can fit in memory.
This will be useful in a couple of more commits in order to provide the
`merge-tree` builtin with a mechanism to create a new pack containing
any objects it created during the merge, instead of storing those
objects individually as loose.
Similar to the existing `index_blob_bulk_checkin()` function, the
entrypoint delegates to `deflate_blob_to_pack_incore()`, which is
responsible for formatting the pack header and then deflating the
contents into the pack. The latter is accomplished by calling
deflate_blob_contents_to_pack_incore(), which takes advantage of the
earlier refactoring and is responsible for writing the object to the
pack and handling any overage from pack.packSizeLimit.
The bulk of the new functionality is implemented in the function
`stream_obj_to_pack_incore()`, which is a generic implementation for
writing objects of arbitrary type (whose contents we can fit in-core)
into a bulk-checkin pack.
The new function shares an unfortunate degree of similarity to the
existing `stream_blob_to_pack()` function. But DRY-ing up these two
would likely be more trouble than it's worth, since the latter has to
deal with reading and writing the contents of the object.
Consistent with the rest of the bulk-checkin mechanism, there are no
direct tests here. In future commits when we expose this new
functionality via the `merge-tree` builtin, we will test it indirectly
there.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++++
bulk-checkin.h | 4 ++
2 files changed, 122 insertions(+)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index f4914fb6d1..25cd1ffa25 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -140,6 +140,69 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id
return 0;
}
+static int stream_obj_to_pack_incore(struct bulk_checkin_packfile *state,
+ git_hash_ctx *ctx,
+ off_t *already_hashed_to,
+ const void *buf, size_t size,
+ enum object_type type,
+ const char *path, unsigned flags)
+{
+ git_zstream s;
+ unsigned char obuf[16384];
+ unsigned hdrlen;
+ int status = Z_OK;
+ int write_object = (flags & HASH_WRITE_OBJECT);
+
+ git_deflate_init(&s, pack_compression_level);
+
+ hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);
+ s.next_out = obuf + hdrlen;
+ s.avail_out = sizeof(obuf) - hdrlen;
+
+ if (*already_hashed_to < size) {
+ size_t hsize = size - *already_hashed_to;
+ if (hsize) {
+ the_hash_algo->update_fn(ctx, buf, hsize);
+ }
+ *already_hashed_to = size;
+ }
+ s.next_in = (void *)buf;
+ s.avail_in = size;
+
+ while (status != Z_STREAM_END) {
+ status = git_deflate(&s, Z_FINISH);
+ if (!s.avail_out || status == Z_STREAM_END) {
+ if (write_object) {
+ size_t written = s.next_out - obuf;
+
+ /* would we bust the size limit? */
+ if (state->nr_written &&
+ pack_size_limit_cfg &&
+ pack_size_limit_cfg < state->offset + written) {
+ git_deflate_abort(&s);
+ return -1;
+ }
+
+ hashwrite(state->f, obuf, written);
+ state->offset += written;
+ }
+ s.next_out = obuf;
+ s.avail_out = sizeof(obuf);
+ }
+
+ switch (status) {
+ case Z_OK:
+ case Z_BUF_ERROR:
+ case Z_STREAM_END:
+ continue;
+ default:
+ die("unexpected deflate failure: %d", status);
+ }
+ }
+ git_deflate_end(&s);
+ return 0;
+}
+
/*
* Read the contents from fd for size bytes, streaming it to the
* packfile in state while updating the hash in ctx. Signal a failure
@@ -316,6 +379,50 @@ static void finalize_checkpoint(struct bulk_checkin_packfile *state,
}
}
+static int deflate_obj_contents_to_pack_incore(struct bulk_checkin_packfile *state,
+ git_hash_ctx *ctx,
+ struct hashfile_checkpoint *checkpoint,
+ struct object_id *result_oid,
+ const void *buf, size_t size,
+ enum object_type type,
+ const char *path, unsigned flags)
+{
+ struct pack_idx_entry *idx = NULL;
+ off_t already_hashed_to = 0;
+
+ /* Note: idx is non-NULL when we are writing */
+ if (flags & HASH_WRITE_OBJECT)
+ CALLOC_ARRAY(idx, 1);
+
+ while (1) {
+ prepare_checkpoint(state, checkpoint, idx, flags);
+ if (!stream_obj_to_pack_incore(state, ctx, &already_hashed_to,
+ buf, size, type, path, flags))
+ break;
+ truncate_checkpoint(state, checkpoint, idx);
+ }
+
+ finalize_checkpoint(state, ctx, checkpoint, idx, result_oid);
+
+ return 0;
+}
+
+static int deflate_blob_to_pack_incore(struct bulk_checkin_packfile *state,
+ struct object_id *result_oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags)
+{
+ git_hash_ctx ctx;
+ struct hashfile_checkpoint checkpoint = {0};
+
+ format_object_header_hash(the_hash_algo, &ctx, &checkpoint, OBJ_BLOB,
+ size);
+
+ return deflate_obj_contents_to_pack_incore(state, &ctx, &checkpoint,
+ result_oid, buf, size,
+ OBJ_BLOB, path, flags);
+}
+
static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
struct object_id *result_oid,
int fd, size_t size,
@@ -396,6 +503,17 @@ int index_blob_bulk_checkin(struct object_id *oid,
return status;
}
+int index_blob_bulk_checkin_incore(struct object_id *oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags)
+{
+ int status = deflate_blob_to_pack_incore(&bulk_checkin_packfile, oid,
+ buf, size, path, flags);
+ if (!odb_transaction_nesting)
+ flush_bulk_checkin_packfile(&bulk_checkin_packfile);
+ return status;
+}
+
void begin_odb_transaction(void)
{
odb_transaction_nesting += 1;
diff --git a/bulk-checkin.h b/bulk-checkin.h
index aa7286a7b3..1b91daeaee 100644
--- a/bulk-checkin.h
+++ b/bulk-checkin.h
@@ -13,6 +13,10 @@ int index_blob_bulk_checkin(struct object_id *oid,
int fd, size_t size,
const char *path, unsigned flags);
+int index_blob_bulk_checkin_incore(struct object_id *oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags);
+
/*
* Tell the object database to optimize for adding
* multiple objects. end_odb_transaction must be called
--
2.42.0.405.gdb2a2f287e
^ permalink raw reply related
* [PATCH v2 6/7] bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
From: Taylor Blau @ 2023-10-17 16:31 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697560266.git.me@ttaylorr.com>
The remaining missing piece in order to teach the `merge-tree` builtin
how to write the contents of a merge into a pack is a function to index
tree objects into a bulk-checkin pack.
This patch implements that missing piece, which is a thin wrapper around
all of the functionality introduced in previous commits.
If and when Git gains support for a "compatibility" hash algorithm, the
changes to support that here will be minimal. The bulk-checkin machinery
will need to convert the incoming tree to compute its length under the
compatibility hash, necessary to reconstruct its header. With that
information (and the converted contents of the tree), the bulk-checkin
machinery will have enough to keep track of the converted object's hash
in order to update the compatibility mapping.
Within `deflate_tree_to_pack_incore()`, the changes should be limited
to something like:
struct strbuf converted = STRBUF_INIT;
if (the_repository->compat_hash_algo) {
if (convert_object_file(&compat_obj,
the_repository->hash_algo,
the_repository->compat_hash_algo, ...) < 0)
die(...);
format_object_header_hash(the_repository->compat_hash_algo,
OBJ_TREE, size);
}
/* compute the converted tree's hash using the compat algorithm */
strbuf_release(&converted);
, assuming related changes throughout the rest of the bulk-checkin
machinery necessary to update the hash of the converted object, which
are likewise minimal in size.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 27 +++++++++++++++++++++++++++
bulk-checkin.h | 4 ++++
2 files changed, 31 insertions(+)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 25cd1ffa25..fe13100e04 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -423,6 +423,22 @@ static int deflate_blob_to_pack_incore(struct bulk_checkin_packfile *state,
OBJ_BLOB, path, flags);
}
+static int deflate_tree_to_pack_incore(struct bulk_checkin_packfile *state,
+ struct object_id *result_oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags)
+{
+ git_hash_ctx ctx;
+ struct hashfile_checkpoint checkpoint = {0};
+
+ format_object_header_hash(the_hash_algo, &ctx, &checkpoint, OBJ_TREE,
+ size);
+
+ return deflate_obj_contents_to_pack_incore(state, &ctx, &checkpoint,
+ result_oid, buf, size,
+ OBJ_TREE, path, flags);
+}
+
static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
struct object_id *result_oid,
int fd, size_t size,
@@ -514,6 +530,17 @@ int index_blob_bulk_checkin_incore(struct object_id *oid,
return status;
}
+int index_tree_bulk_checkin_incore(struct object_id *oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags)
+{
+ int status = deflate_tree_to_pack_incore(&bulk_checkin_packfile, oid,
+ buf, size, path, flags);
+ if (!odb_transaction_nesting)
+ flush_bulk_checkin_packfile(&bulk_checkin_packfile);
+ return status;
+}
+
void begin_odb_transaction(void)
{
odb_transaction_nesting += 1;
diff --git a/bulk-checkin.h b/bulk-checkin.h
index 1b91daeaee..89786b3954 100644
--- a/bulk-checkin.h
+++ b/bulk-checkin.h
@@ -17,6 +17,10 @@ int index_blob_bulk_checkin_incore(struct object_id *oid,
const void *buf, size_t size,
const char *path, unsigned flags);
+int index_tree_bulk_checkin_incore(struct object_id *oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags);
+
/*
* Tell the object database to optimize for adding
* multiple objects. end_odb_transaction must be called
--
2.42.0.405.gdb2a2f287e
^ permalink raw reply related
* [PATCH v2 7/7] builtin/merge-tree.c: implement support for `--write-pack`
From: Taylor Blau @ 2023-10-17 16:31 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697560266.git.me@ttaylorr.com>
When using merge-tree often within a repository[^1], it is possible to
generate a relatively large number of loose objects, which can result in
degraded performance, and inode exhaustion in extreme cases.
Building on the functionality introduced in previous commits, the
bulk-checkin machinery now has support to write arbitrary blob and tree
objects which are small enough to be held in-core. We can use this to
write any blob/tree objects generated by ORT into a separate pack
instead of writing them out individually as loose.
This functionality is gated behind a new `--write-pack` option to
`merge-tree` that works with the (non-deprecated) `--write-tree` mode.
The implementation is relatively straightforward. There are two spots
within the ORT mechanism where we call `write_object_file()`, one for
content differences within blobs, and another to assemble any new trees
necessary to construct the merge. In each of those locations,
conditionally replace calls to `write_object_file()` with
`index_blob_bulk_checkin_incore()` or `index_tree_bulk_checkin_incore()`
depending on which kind of object we are writing.
The only remaining task is to begin and end the transaction necessary to
initialize the bulk-checkin machinery, and move any new pack(s) it
created into the main object store.
[^1]: Such is the case at GitHub, where we run presumptive "test merges"
on open pull requests to see whether or not we can light up the merge
button green depending on whether or not the presumptive merge was
conflicted.
This is done in response to a number of user-initiated events,
including viewing an open pull request whose last test merge is stale
with respect to the current base and tip of the pull request. As a
result, merge-tree can be run very frequently on large, active
repositories.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
Documentation/git-merge-tree.txt | 4 ++
builtin/merge-tree.c | 5 ++
merge-ort.c | 42 +++++++++++----
merge-recursive.h | 1 +
t/t4301-merge-tree-write-tree.sh | 93 ++++++++++++++++++++++++++++++++
5 files changed, 136 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
index ffc4fbf7e8..9d37609ef1 100644
--- a/Documentation/git-merge-tree.txt
+++ b/Documentation/git-merge-tree.txt
@@ -69,6 +69,10 @@ OPTIONS
specify a merge-base for the merge, and specifying multiple bases is
currently not supported. This option is incompatible with `--stdin`.
+--write-pack::
+ Write any new objects into a separate packfile instead of as
+ individual loose objects.
+
[[OUTPUT]]
OUTPUT
------
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 0de42aecf4..672ebd4c54 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -18,6 +18,7 @@
#include "quote.h"
#include "tree.h"
#include "config.h"
+#include "bulk-checkin.h"
static int line_termination = '\n';
@@ -414,6 +415,7 @@ struct merge_tree_options {
int show_messages;
int name_only;
int use_stdin;
+ int write_pack;
};
static int real_merge(struct merge_tree_options *o,
@@ -440,6 +442,7 @@ static int real_merge(struct merge_tree_options *o,
init_merge_options(&opt, the_repository);
opt.show_rename_progress = 0;
+ opt.write_pack = o->write_pack;
opt.branch1 = branch1;
opt.branch2 = branch2;
@@ -548,6 +551,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
&merge_base,
N_("commit"),
N_("specify a merge-base for the merge")),
+ OPT_BOOL(0, "write-pack", &o.write_pack,
+ N_("write new objects to a pack instead of as loose")),
OPT_END()
};
diff --git a/merge-ort.c b/merge-ort.c
index 7857ce9fbd..e198d2bc2b 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -48,6 +48,7 @@
#include "tree.h"
#include "unpack-trees.h"
#include "xdiff-interface.h"
+#include "bulk-checkin.h"
/*
* We have many arrays of size 3. Whenever we have such an array, the
@@ -2107,10 +2108,19 @@ static int handle_content_merge(struct merge_options *opt,
if ((merge_status < 0) || !result_buf.ptr)
ret = error(_("failed to execute internal merge"));
- if (!ret &&
- write_object_file(result_buf.ptr, result_buf.size,
- OBJ_BLOB, &result->oid))
- ret = error(_("unable to add %s to database"), path);
+ if (!ret) {
+ ret = opt->write_pack
+ ? index_blob_bulk_checkin_incore(&result->oid,
+ result_buf.ptr,
+ result_buf.size,
+ path, 1)
+ : write_object_file(result_buf.ptr,
+ result_buf.size,
+ OBJ_BLOB, &result->oid);
+ if (ret)
+ ret = error(_("unable to add %s to database"),
+ path);
+ }
free(result_buf.ptr);
if (ret)
@@ -3596,7 +3606,8 @@ static int tree_entry_order(const void *a_, const void *b_)
b->string, strlen(b->string), bmi->result.mode);
}
-static int write_tree(struct object_id *result_oid,
+static int write_tree(struct merge_options *opt,
+ struct object_id *result_oid,
struct string_list *versions,
unsigned int offset,
size_t hash_size)
@@ -3630,8 +3641,14 @@ static int write_tree(struct object_id *result_oid,
}
/* Write this object file out, and record in result_oid */
- if (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))
+ ret = opt->write_pack
+ ? index_tree_bulk_checkin_incore(result_oid,
+ buf.buf, buf.len, "", 1)
+ : write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);
+
+ if (ret)
ret = -1;
+
strbuf_release(&buf);
return ret;
}
@@ -3796,8 +3813,8 @@ static int write_completed_directory(struct merge_options *opt,
*/
dir_info->is_null = 0;
dir_info->result.mode = S_IFDIR;
- if (write_tree(&dir_info->result.oid, &info->versions, offset,
- opt->repo->hash_algo->rawsz) < 0)
+ if (write_tree(opt, &dir_info->result.oid, &info->versions,
+ offset, opt->repo->hash_algo->rawsz) < 0)
ret = -1;
}
@@ -4331,9 +4348,13 @@ static int process_entries(struct merge_options *opt,
fflush(stdout);
BUG("dir_metadata accounting completely off; shouldn't happen");
}
- if (write_tree(result_oid, &dir_metadata.versions, 0,
+ if (write_tree(opt, result_oid, &dir_metadata.versions, 0,
opt->repo->hash_algo->rawsz) < 0)
ret = -1;
+
+ if (opt->write_pack)
+ end_odb_transaction();
+
cleanup:
string_list_clear(&plist, 0);
string_list_clear(&dir_metadata.versions, 0);
@@ -4877,6 +4898,9 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
*/
strmap_init(&opt->priv->conflicts);
+ if (opt->write_pack)
+ begin_odb_transaction();
+
trace2_region_leave("merge", "allocate/init", opt->repo);
}
diff --git a/merge-recursive.h b/merge-recursive.h
index b88000e3c2..156e160876 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -48,6 +48,7 @@ struct merge_options {
unsigned renormalize : 1;
unsigned record_conflict_msgs_as_headers : 1;
const char *msg_header_prefix;
+ unsigned write_pack : 1;
/* internal fields used by the implementation */
struct merge_options_internal *priv;
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index 250f721795..2d81ff4de5 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -922,4 +922,97 @@ test_expect_success 'check the input format when --stdin is passed' '
test_cmp expect actual
'
+packdir=".git/objects/pack"
+
+test_expect_success 'merge-tree can pack its result with --write-pack' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+
+ # base has lines [3, 4, 5]
+ # - side adds to the beginning, resulting in [1, 2, 3, 4, 5]
+ # - other adds to the end, resulting in [3, 4, 5, 6, 7]
+ #
+ # merging the two should result in a new blob object containing
+ # [1, 2, 3, 4, 5, 6, 7], along with a new tree.
+ test_commit -C repo base file "$(test_seq 3 5)" &&
+ git -C repo branch -M main &&
+ git -C repo checkout -b side main &&
+ test_commit -C repo side file "$(test_seq 1 5)" &&
+ git -C repo checkout -b other main &&
+ test_commit -C repo other file "$(test_seq 3 7)" &&
+
+ find repo/$packdir -type f -name "pack-*.idx" >packs.before &&
+ tree="$(git -C repo merge-tree --write-pack \
+ refs/tags/side refs/tags/other)" &&
+ blob="$(git -C repo rev-parse $tree:file)" &&
+ find repo/$packdir -type f -name "pack-*.idx" >packs.after &&
+
+ test_must_be_empty packs.before &&
+ test_line_count = 1 packs.after &&
+
+ git show-index <$(cat packs.after) >objects &&
+ test_line_count = 2 objects &&
+ grep "^[1-9][0-9]* $tree" objects &&
+ grep "^[1-9][0-9]* $blob" objects
+'
+
+test_expect_success 'merge-tree can write multiple packs with --write-pack' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+
+ git config pack.packSizeLimit 512 &&
+
+ test_seq 512 >f &&
+
+ # "f" contains roughly ~2,000 bytes.
+ #
+ # Each side ("foo" and "bar") adds a small amount of data at the
+ # beginning and end of "base", respectively.
+ git add f &&
+ test_tick &&
+ git commit -m base &&
+ git branch -M main &&
+
+ git checkout -b foo main &&
+ {
+ echo foo && cat f
+ } >f.tmp &&
+ mv f.tmp f &&
+ git add f &&
+ test_tick &&
+ git commit -m foo &&
+
+ git checkout -b bar main &&
+ echo bar >>f &&
+ git add f &&
+ test_tick &&
+ git commit -m bar &&
+
+ find $packdir -type f -name "pack-*.idx" >packs.before &&
+ # Merging either side should result in a new object which is
+ # larger than 1M, thus the result should be split into two
+ # separate packs.
+ tree="$(git merge-tree --write-pack \
+ refs/heads/foo refs/heads/bar)" &&
+ blob="$(git rev-parse $tree:f)" &&
+ find $packdir -type f -name "pack-*.idx" >packs.after &&
+
+ test_must_be_empty packs.before &&
+ test_line_count = 2 packs.after &&
+ for idx in $(cat packs.after)
+ do
+ git show-index <$idx || return 1
+ done >objects &&
+
+ # The resulting set of packs should contain one copy of both
+ # objects, each in a separate pack.
+ test_line_count = 2 objects &&
+ grep "^[1-9][0-9]* $tree" objects &&
+ grep "^[1-9][0-9]* $blob" objects
+
+ )
+'
+
test_done
--
2.42.0.405.gdb2a2f287e
^ permalink raw reply related
* Re: [PATCH] grep: die gracefully when outside repository
From: Junio C Hamano @ 2023-10-17 16:42 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, ks1322
In-Reply-To: <087c92e3904dd774f672373727c300bf7f5f6369.1697317276.git.code@khaugsbakk.name>
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> diff --git a/pathspec.c b/pathspec.c
> index 3a3a5724c44..e115832f17a 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -468,6 +468,9 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
> &prefixlen, copyfrom);
> if (!match) {
> const char *hint_path = get_git_work_tree();
> + if (!have_git_dir())
> + die(_("'%s' is outside the directory tree"),
> + copyfrom);
> if (!hint_path)
> hint_path = get_git_dir();
> die(_("%s: '%s' is outside repository at '%s'"), elt,
It is curious that the original has two sources of hint_path (i.e.,
get_git_dir() is used as a fallback for get_git_work_tree()). Are
we certain that the check is at the right place? If we do not have
a repository, then both would fail by returning NULL, so it should
not matter if we add the new check before we check either or both,
or even after we checked both before dying.
I wonder if
const char *hint_path = get_git_work_tree();
if (!hint_path)
hint_path = get_git_dir();
if (hint_path)
die(_("%s: '%s' is outside repository at '%s'"),
elt, copyfrom, absolute_path(hint_path));
else
die(_("%s: '%s' is outside the directory tree"),
elt, copyfrom);
makes the intent of the code clearer. We want to hint the location
of the repository by computing hint_path, and if we can compute it,
we use it in the error message, but otherwise we don't add hint. And
we apply that conditional whether we have repository or not---what we
care about is the NULL-ness of the hint string we computed.
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 39d6d713ecb..b976f81a166 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -1234,6 +1234,19 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '
> )
> '
>
> +test_expect_success 'outside of git repository with pathspec outside the directory tree' '
> + test_when_finished rm -fr non &&
> + rm -fr non &&
> + mkdir -p non/git/sub &&
> + (
> + GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
> + export GIT_CEILING_DIRECTORIES &&
> + cd non/git &&
> + test_expect_code 128 git grep --no-index search .. 2>error &&
> + grep "is outside the directory tree" error
Excellent. This is a very good use of the GIT_CEILING_DIRECTORIES
facility.
> + )
> +'
> +
> test_expect_success 'inside git repository but with --no-index' '
> rm -fr is &&
> mkdir -p is/git/sub &&
>
> base-commit: 43c8a30d150ecede9709c1f2527c8fba92c65f40
^ permalink raw reply
* Re:[PATCH] t/t7601: Modernize test scripts using functions
From: Dorcas Litunya @ 2023-10-17 16:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: anonolitunya, git, christian.couder
In-Reply-To: <ZS2ESFGP2H3CTJSK@dorcaslitunya-virtual-machine>
On Mon, Oct 16, 2023 at 09:43:24PM +0300, Dorcas Litunya wrote:
> Bcc:
> Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions
> Reply-To:
> In-Reply-To: <xmqq1qdumrto.fsf@gitster.g>
>
> On Mon, Oct 16, 2023 at 09:53:55AM -0700, Junio C Hamano wrote:
> > Dorcas AnonoLitunya <anonolitunya@gmail.com> writes:
> >
> > > Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions
> >
> > Let's try if we can pack a bit more information. For example
> >
> > Subject: [PATCH] t7601: use "test_path_is_file" etc. instead of "test -f"
> >
> > would clarify what kind of modernization is done by this patch.
> >
> > > The test script is currently using the command format 'test -f' to
> > > check for existence or absence of files.
> >
> > "is currently using" -> "uses".
> >
> > > Replace it with new helper functions following the format
> > > 'test_path_is_file'.
> >
> > I am not sure what role "the format" plays in this picture.
> > test_path_is_file is not new---it has been around for quite a while.
> >
> > > Consequently, the patch also replaces the inverse command '! test -f' or
> > > 'test ! -f' with new helper function following the format
> > > 'test_path_is_missing'
> >
> > A bit more on this later.
> >
> So should I replace this in the next version or leave this as is?
Hello Junio,
Following up on this? What are your thoughts on it?
Thanks!
Dorcas
> > > This adjustment using helper functions makes the code more readable and
> > > easier to understand.
> >
> > Looking good. If I were writing this, I'll make the whole thing
> > more like this, though:
> >
> > t7601: use "test_path_is_file" etc. instead of "test -f"
> >
> > Some tests in t7601 use "test -f" and "test ! -f" to see if a
> > path exists or is missing. Use test_path_is_file and
> > test_path_is_missing helper functions to clarify these tests a
> > bit better. This especially matters for the "missing" case,
> > because "test ! -f F" will be happy if "F" exists as a
> > directory, but the intent of the test is that "F" should not
> > exist, even as a directory.
> >
> >
> > > diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
> > > index bd238d89b0..e08767df66 100755
> > > --- a/t/t7601-merge-pull-config.sh
> > > +++ b/t/t7601-merge-pull-config.sh
> > > @@ -349,13 +349,13 @@ test_expect_success 'Cannot rebase with multiple heads' '
> > >
> > > test_expect_success 'merge c1 with c2' '
> > > git reset --hard c1 &&
> > > - test -f c0.c &&
> > > - test -f c1.c &&
> > > - test ! -f c2.c &&
> > > - test ! -f c3.c &&
> > > + test_path_is_file c0.c &&
> > > + test_path_is_file c1.c &&
> > > + test_path_is_missing c2.c &&
> > > + test_path_is_missing c3.c &&
> >
> > The original says "We are happy if c2.c is not a file", so it would
> > have been happy if by some mistake "git reset" created a directory
> > there. But the _intent_ of the test is that we do not have anything
> > at c2.c, and the updated code expresses it better.
^ permalink raw reply
* Re: [PATCH v2 2/2] Prevent git from rehashing 4GiB files
From: Junio C Hamano @ 2023-10-17 17:02 UTC (permalink / raw)
To: Jason Hatton
Cc: Jeff King, brian m. carlson, git@vger.kernel.org, Taylor Blau
In-Reply-To: <MW5PR16MB50791EDD3D006EFBE8DBBCB3AFD6A@MW5PR16MB5079.namprd16.prod.outlook.com>
Jason Hatton <jhatton@globalfinishing.com> writes:
> We could probably write the below to do the same thing.
>
> void fill_stat_data(struct stat_data *sd, struct stat *st)
> {
> sd->sd_ctime.sec = (unsigned int)st->st_ctime;
> sd->sd_mtime.sec = (unsigned int)st->st_mtime;
> sd->sd_ctime.nsec = ST_CTIME_NSEC(*st);
> sd->sd_mtime.nsec = ST_MTIME_NSEC(*st);
> sd->sd_dev = st->st_dev;
> sd->sd_ino = st->st_ino;
> sd->sd_uid = st->st_uid;
> sd->sd_gid = st->st_gid;
> sd->sd_size = st->st_size;
> + if (sd->sd_size == 0 && st->st_size!= 0) {
> + sd->sd_size = 1;
> + }
> }
The above is a fairly straight-forward inlining (except that it does
explicit comparisons with zero) of the helper function the version
of patch under discussion added, and uses 1 instead of (1<<31) as an
arbigrary nonzero number that can be used to work around the issue.
So I agree with you that it would do the same thing. I am not
surprised if it also gets scolded by Coverity the same way, though.
^ permalink raw reply
* Re: [PATCH v3 0/5] config-parse: create config parsing library
From: Junio C Hamano @ 2023-10-17 17:13 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, jonathantanmy, calvinwan, glencbz
In-Reply-To: <cover.1695330852.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> Config parsing no longer uses global state as of gc/config-context, so the
> natural next step for libification is to turn that into its own library.
> This series starts that process by moving config parsing into
> config-parse.[c|h] so that other programs can include this functionality
> without pulling in all of config.[c|h].
This has been in list archive collecting dust. It is unfortunate
that not many people appear to be interested in reviewing others'
patches?
> Open questions:
> - How do folks feel about the do_event() refactor in patches 2 & 3?
I gave a quick re-read and found that the code after patch 2 made it
easier to see how config.c::do_event() does its thing (even though
the patch text of that exact step was somehow a bit hard to follow).
However, the helper added by patch 3, do_event_and_flush(), that
duplicates exactly what do_event() does, is hard to reason about, at
least for me. It returns early without setting .previous_type to
EOF and the value returned from the helper signals if that is the
case (the two early return points both return what flush_event()
gave us), but the only caller of the helper does not even inspect
the return value, unlike all the callers of do_event(), which also
looks a bit fishy.
Thanks.
^ permalink raw reply
* Re: [PATCH] commit: detect commits that exist in commit-graph but not in the ODB
From: Junio C Hamano @ 2023-10-17 18:34 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak, Taylor Blau
In-Reply-To: <ZS4rmtBTYnp2RMiY@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> Fair point indeed. The following is a worst-case scenario benchmark of
> of the change where we do a full topological walk of all reachable
> commits in the graph, executed in linux.git. We parse commit parents via
> `repo_parse_commit_gently()`, so the new code path now basically has to
> check for object existence of every reachable commit:
> ...
> The added check does lead to a performance regression indeed, which is
> not all that unexpected. That being said, the commit-graph still results
> in a significant speedup compared to the case where we don't have it.
Yeah, I agree that both points are expected. An extra check that is
much cheaper than the full parsing is paying a small price to be a
bit more careful than before. The question is if the price is small
enough. I am still not sure if the extra carefulness is warranted
for all normal cases to spend 30% extra cycles
Yeah, I agree that both points are expected. An extra check that is
much cheaper than the full parsing is paying a small price to be a
bit more careful than before. The question is if the price is small
enough. I am still not sure if the extra carefulness is warranted
for all normal cases to spend 30% extra cycles.
Thanks.
^ 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