* [PATCH v4 01/17] t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
From: Taylor Blau @ 2023-10-18 18:32 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697653929.git.me@ttaylorr.com>
The existing implementation of test_bloom_filters_not_used() asserts
that the Bloom filter sub-system has not been initialized at all, by
checking for the absence of any data from it from trace2.
In the following commit, it will become possible to load Bloom filters
without using them (e.g., because `commitGraph.changedPathVersion` is
incompatible with the hash version with which the commit-graph's Bloom
filters were written).
When this is the case, it's possible to initialize the Bloom filter
sub-system, while still not using any Bloom filters. When this is the
case, check that the data dump from the Bloom sub-system is all zeros,
indicating that no filters were used.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
t/t4216-log-bloom.sh | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index fa9d32facf..487fc3d6b9 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -81,7 +81,19 @@ test_bloom_filters_used () {
test_bloom_filters_not_used () {
log_args=$1
setup "$log_args" &&
- ! grep -q "statistics:{\"filter_not_present\":" "$TRASH_DIRECTORY/trace.perf" &&
+
+ if grep -q "statistics:{\"filter_not_present\":" "$TRASH_DIRECTORY/trace.perf"
+ then
+ # if the Bloom filter system is initialized, ensure that no
+ # filters were used
+ data="statistics:{"
+ data="$data\"filter_not_present\":0,"
+ data="$data\"maybe\":0,"
+ data="$data\"definitely_not\":0,"
+ data="$data\"false_positive\":0}"
+
+ grep -q "$data" "$TRASH_DIRECTORY/trace.perf"
+ fi &&
test_cmp log_wo_bloom log_w_bloom
}
--
2.42.0.415.g8942f205c8
^ permalink raw reply related
* [PATCH v4 00/17] bloom: changed-path Bloom filters v2 (& sundries)
From: Taylor Blau @ 2023-10-18 18:32 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>
(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!
[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 | 282 ++++++++++++++++++++++-
14 files changed, 692 insertions(+), 55 deletions(-)
Range-diff against v3:
1: fe671d616c = 1: e0fc51c3fb t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
2: 7d0fa93543 = 2: 87b09e6266 revision.c: consult Bloom filters for root commits
3: 2ecc0a2d58 ! 3: 46d8a41005 commit-graph: ensure Bloom filters are read with consistent settings
@@ t/t4216-log-bloom.sh: test_expect_success 'Bloom generation backfills empty comm
+ done
+'
+
-+test_expect_success 'split' '
++test_expect_success 'ensure incompatible Bloom filters are ignored' '
+ # Compute Bloom filters with "unusual" settings.
+ git -C $repo rev-parse one >in &&
+ GIT_TEST_BLOOM_SETTINGS_NUM_HASHES=3 git -C $repo commit-graph write \
@@ t/t4216-log-bloom.sh: test_expect_success 'Bloom generation backfills empty comm
+
+test_expect_success 'merge graph layers with incompatible Bloom settings' '
+ # Ensure that incompatible Bloom filters are ignored when
-+ # generating new layers.
++ # merging existing 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 -c core.commitGraph=false log --oneline --no-decorate -- \
++ file >expect &&
++ trace_out="$(pwd)/trace.perf" &&
++ GIT_TRACE2_PERF="$trace_out" \
+ git -C $repo log --oneline --no-decorate -- file >actual 2>err &&
+
-+ test_cmp expect actual && cat err &&
-+ grep "statistics:{\"filter_not_present\":0" trace.perf &&
-+ ! grep "disabling Bloom filters" err
++ test_cmp expect actual &&
++ grep "statistics:{\"filter_not_present\":0," trace.perf &&
++ test_must_be_empty err
+'
+
test_done
4: 17703ed89a = 4: 4d0190a992 gitformat-commit-graph: describe version 2 of BDAT
5: 94552abf45 = 5: 3c2057c11c t/helper/test-read-graph.c: extract `dump_graph_info()`
6: 3d81efa27b = 6: e002e35004 bloom.h: make `load_bloom_filter_from_graph()` public
7: d23cd89037 = 7: c7016f51cd t/helper/test-read-graph: implement `bloom-filters` mode
8: cba766f224 ! 8: cef2aac8ba t4216: test changed path filters with high bit paths
@@ Commit message
## t/t4216-log-bloom.sh ##
@@ t/t4216-log-bloom.sh: test_expect_success 'merge graph layers with incompatible Bloom settings' '
- ! grep "disabling Bloom filters" err
+ test_must_be_empty err
'
+get_first_changed_path_filter () {
@@ t/t4216-log-bloom.sh: test_expect_success 'merge graph layers with incompatible
+ (
+ cd highbit1 &&
+ echo "52a9" >expect &&
-+ get_first_changed_path_filter >actual &&
-+ test_cmp expect actual
++ get_first_changed_path_filter >actual
+ )
+'
+
9: a08a961f41 = 9: 36d4e2202e repo-settings: introduce commitgraph.changedPathsVersion
10: 61d44519a5 ! 10: f6ab427ead commit-graph: new filter ver. that fixes murmur3
@@ t/t4216-log-bloom.sh: test_expect_success 'version 1 changed-path used when vers
+ test_commit -C doublewrite c "$CENT" &&
+ git -C doublewrite config --add commitgraph.changedPathsVersion 1 &&
+ git -C doublewrite commit-graph write --reachable --changed-paths &&
++ for v in -2 3
++ do
++ git -C doublewrite config --add commitgraph.changedPathsVersion $v &&
++ git -C doublewrite commit-graph write --reachable --changed-paths 2>err &&
++ cat >expect <<-EOF &&
++ warning: attempting to write a commit-graph, but ${SQ}commitgraph.changedPathsVersion${SQ} ($v) is not supported
++ EOF
++ test_cmp expect err || return 1
++ done &&
+ git -C doublewrite config --add commitgraph.changedPathsVersion 2 &&
+ git -C doublewrite commit-graph write --reachable --changed-paths &&
+ (
11: a8c10f8de8 = 11: dc69b28329 bloom: annotate filters with hash version
12: 2ba10a4b4b = 12: 85dbdc4ed2 bloom: prepare to discard incompatible Bloom filters
13: 09d8669c3a = 13: 3ff669a622 commit-graph.c: unconditionally load Bloom filters
14: 0d4f9dc4ee = 14: 1c78e3d178 commit-graph: drop unnecessary `graph_read_bloom_data_context`
15: 1f7f27bc47 = 15: a289514faa object.h: fix mis-aligned flag bits table
16: abbef95ae8 ! 16: 6a12e39e7f commit-graph: reuse existing Bloom filters where possible
@@ t/t4216-log-bloom.sh: test_expect_success 'when writing another commit graph, pr
test_commit -C doublewrite c "$CENT" &&
+
git -C doublewrite config --add commitgraph.changedPathsVersion 1 &&
-- git -C doublewrite commit-graph write --reachable --changed-paths &&
+ GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
+ git -C doublewrite commit-graph write --reachable --changed-paths &&
+ test_filter_computed 1 trace2.txt &&
+ test_filter_upgraded 0 trace2.txt &&
++
+ git -C doublewrite commit-graph write --reachable --changed-paths &&
+ for v in -2 3
+ do
+@@ t/t4216-log-bloom.sh: test_expect_success 'when writing commit graph, do not reuse changed-path of ano
+ EOF
+ test_cmp expect err || return 1
+ done &&
+
git -C doublewrite config --add commitgraph.changedPathsVersion 2 &&
- git -C doublewrite commit-graph write --reachable --changed-paths &&
17: ca362408d5 ! 17: 8942f205c8 bloom: introduce `deinit_bloom_filters()`
@@ bloom.h: void add_key_to_filter(const struct bloom_key *key,
BLOOM_NOT_COMPUTED = (1 << 0),
## commit-graph.c ##
-@@ commit-graph.c: static void close_commit_graph_one(struct commit_graph *g)
+@@ commit-graph.c: struct bloom_filter_settings *get_bloom_filter_settings(struct repository *r)
void close_commit_graph(struct raw_object_store *o)
{
- close_commit_graph_one(o->commit_graph);
+ clear_commit_graph_data_slab(&commit_graph_data_slab);
+ deinit_bloom_filters();
+ free_commit_graph(o->commit_graph);
o->commit_graph = NULL;
}
-
@@ commit-graph.c: int write_commit_graph(struct object_directory *odb,
res = write_commit_graph_file(ctx);
--
2.42.0.415.g8942f205c8
^ permalink raw reply
* Re: [Outreachy][PATCH] branch.c: adjust error messages to coding guidelines
From: Rubén Justo @ 2023-10-18 18:19 UTC (permalink / raw)
To: Isoken June Ibizugbe, git; +Cc: christian.couder, Junio C Hamano
In-Reply-To: <20231018051223.13955-1-isokenjune@gmail.com>
On 18-oct-2023 06:12:22, Isoken June Ibizugbe wrote:
> Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
> ---
> builtin/branch.c | 66 ++++++++++++++++++++++++------------------------
> 1 file changed, 33 insertions(+), 33 deletions(-)
Only builtin/branch.c is touched.
The changes in this patch break some tests, therefore this patch must
also include the fixes for those tests.
> @@ -965,11 +965,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> const char *start_name = argc == 2 ? argv[1] : head;
>
> if (filter.kind != FILTER_REFS_BRANCHES)
> - die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n"
> + die(_("the -a, and -r, options to 'git branch' do not take a branch name\n"
> "Did you mean to use: -a|-r --list <pattern>?"));
OK. The initial 'T' is fixed, but as Junio explained [1], the full stop
must stay.
Thanks.
[1] https://lore.kernel.org/git/xmqqttqxkmaq.fsf@gitster.g/
^ permalink raw reply
* Re: [PATCH v3 00/17] bloom: changed-path Bloom filters v2 (& sundries)
From: Taylor Blau @ 2023-10-18 17:47 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Jonathan Tan, Junio C Hamano, Jeff King, SZEDER Gábor
In-Reply-To: <ZS5JsDKk8RioQfOA@tanuki>
On Tue, Oct 17, 2023 at 10:45:36AM +0200, Patrick Steinhardt wrote:
> > 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.
Thanks for giving it a look! I generated a few small tweaks on top of
what I already had here based on your review, so I'll send a reroll
shortly.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v3 10/17] commit-graph: new filter ver. that fixes murmur3
From: Taylor Blau @ 2023-10-18 17:46 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Jonathan Tan, Junio C Hamano, Jeff King, SZEDER Gábor
In-Reply-To: <ZS5JpJCw-MY256xo@tanuki>
On Tue, Oct 17, 2023 at 10:45:24AM +0200, Patrick Steinhardt wrote:
> > @@ -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.
We don't have to deal with c->commit_graph_changed_paths_version being
-1 here, since we normalize it when reading the BDAT chunk. See
commit-graph.c::graph_read_bloom_data(), particularly:
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;
> > +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?
Good idea, I'll update the test to reflect.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v3 08/17] t4216: test changed path filters with high bit paths
From: Taylor Blau @ 2023-10-18 17:41 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Jonathan Tan, Junio C Hamano, Jeff King, SZEDER Gábor
In-Reply-To: <ZS5JmQib3onjirC6@tanuki>
On Tue, Oct 17, 2023 at 10:45:13AM +0200, Patrick Steinhardt wrote:
> > +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`?
Great catch, I am surprised this slipped by in earlier rounds. This
should do the trick, since we don't actually care about conditioning
that test's passing on test_cmp coming up clean. We check that in the if
statement you pointed out here, so:
--- 8< ---
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index 114672e904..400dce2193 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -502,8 +502,7 @@ 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
+ get_first_changed_path_filter >actual
)
'
--- >8 ---
Thanks,
Taylor
^ permalink raw reply related
* Re: [PATCH v3 05/17] t/helper/test-read-graph.c: extract `dump_graph_info()`
From: Taylor Blau @ 2023-10-18 17:37 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Jonathan Tan, Junio C Hamano, Jeff King, SZEDER Gábor
In-Reply-To: <ZS5JnkybxvetTUzu@tanuki>
On Tue, Oct 17, 2023 at 10:45:18AM +0200, Patrick Steinhardt wrote:
> 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.
Yeah, this is an artifact of having tossed these patches back and forth
(originally Jonathan sent some of these, then I sent another round, then
Jonathan, now me again). It's a little verbose, but accurately tracks
the DCO across multiple rounds.
Thanks,
Taylor
^ permalink raw reply
* [PATCH v3 06/10] bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`
From: Taylor Blau @ 2023-10-18 17:08 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697648864.git.me@ttaylorr.com>
Continue to prepare for streaming an object's contents directly from
memory by teaching `bulk_checkin_source` how to perform reads and seeks
based on an address in memory.
Unlike file descriptors, which manage their own offset internally, we
have to keep track of how many bytes we've read out of the buffer, and
make sure we don't read past the end of the buffer.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index fc1d902018..133e02ce36 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -141,11 +141,15 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id
}
struct bulk_checkin_source {
- enum { SOURCE_FILE } type;
+ enum { SOURCE_FILE, SOURCE_INCORE } type;
/* SOURCE_FILE fields */
int fd;
+ /* SOURCE_INCORE fields */
+ const void *buf;
+ size_t read;
+
/* common fields */
size_t size;
const char *path;
@@ -157,6 +161,11 @@ static off_t bulk_checkin_source_seek_to(struct bulk_checkin_source *source,
switch (source->type) {
case SOURCE_FILE:
return lseek(source->fd, offset, SEEK_SET);
+ case SOURCE_INCORE:
+ if (!(0 <= offset && offset < source->size))
+ return (off_t)-1;
+ source->read = offset;
+ return source->read;
default:
BUG("unknown bulk-checkin source: %d", source->type);
}
@@ -168,6 +177,13 @@ static ssize_t bulk_checkin_source_read(struct bulk_checkin_source *source,
switch (source->type) {
case SOURCE_FILE:
return read_in_full(source->fd, buf, nr);
+ case SOURCE_INCORE:
+ assert(source->read <= source->size);
+ if (nr > source->size - source->read)
+ nr = source->size - source->read;
+ memcpy(buf, (unsigned char *)source->buf + source->read, nr);
+ source->read += nr;
+ return nr;
default:
BUG("unknown bulk-checkin source: %d", source->type);
}
--
2.42.0.408.g97fac66ae4
^ permalink raw reply related
* [PATCH v3 05/10] bulk-checkin: extract abstract `bulk_checkin_source`
From: Taylor Blau @ 2023-10-18 17:08 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697648864.git.me@ttaylorr.com>
A future commit will want to implement a very similar routine as in
`stream_blob_to_pack()` with two notable changes:
- Instead of streaming just OBJ_BLOBs, this new function may want to
stream objects of arbitrary type.
- Instead of streaming the object's contents from an open
file-descriptor, this new function may want to "stream" its contents
from memory.
To avoid duplicating a significant chunk of code between the existing
`stream_blob_to_pack()`, extract an abstract `bulk_checkin_source`. This
concept currently is a thin layer of `lseek()` and `read_in_full()`, but
will grow to understand how to perform analogous operations when writing
out an object's contents from memory.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 61 +++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 53 insertions(+), 8 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index f4914fb6d1..fc1d902018 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -140,8 +140,41 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id
return 0;
}
+struct bulk_checkin_source {
+ enum { SOURCE_FILE } type;
+
+ /* SOURCE_FILE fields */
+ int fd;
+
+ /* common fields */
+ size_t size;
+ const char *path;
+};
+
+static off_t bulk_checkin_source_seek_to(struct bulk_checkin_source *source,
+ off_t offset)
+{
+ switch (source->type) {
+ case SOURCE_FILE:
+ return lseek(source->fd, offset, SEEK_SET);
+ default:
+ BUG("unknown bulk-checkin source: %d", source->type);
+ }
+}
+
+static ssize_t bulk_checkin_source_read(struct bulk_checkin_source *source,
+ void *buf, size_t nr)
+{
+ switch (source->type) {
+ case SOURCE_FILE:
+ return read_in_full(source->fd, buf, nr);
+ default:
+ BUG("unknown bulk-checkin source: %d", source->type);
+ }
+}
+
/*
- * Read the contents from fd for size bytes, streaming it to the
+ * Read the contents from 'source' for 'size' bytes, streaming it to the
* packfile in state while updating the hash in ctx. Signal a failure
* by returning a negative value when the resulting pack would exceed
* the pack size limit and this is not the first object in the pack,
@@ -157,7 +190,7 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id
*/
static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
git_hash_ctx *ctx, off_t *already_hashed_to,
- int fd, size_t size, const char *path,
+ struct bulk_checkin_source *source,
unsigned flags)
{
git_zstream s;
@@ -167,22 +200,28 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
int status = Z_OK;
int write_object = (flags & HASH_WRITE_OBJECT);
off_t offset = 0;
+ size_t size = source->size;
git_deflate_init(&s, pack_compression_level);
- hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB, size);
+ hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB,
+ size);
s.next_out = obuf + hdrlen;
s.avail_out = sizeof(obuf) - hdrlen;
while (status != Z_STREAM_END) {
if (size && !s.avail_in) {
ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
- ssize_t read_result = read_in_full(fd, ibuf, rsize);
+ ssize_t read_result;
+
+ read_result = bulk_checkin_source_read(source, ibuf,
+ rsize);
if (read_result < 0)
- die_errno("failed to read from '%s'", path);
+ die_errno("failed to read from '%s'",
+ source->path);
if (read_result != rsize)
die("failed to read %d bytes from '%s'",
- (int)rsize, path);
+ (int)rsize, source->path);
offset += rsize;
if (*already_hashed_to < offset) {
size_t hsize = offset - *already_hashed_to;
@@ -325,6 +364,12 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
git_hash_ctx ctx;
struct hashfile_checkpoint checkpoint = {0};
struct pack_idx_entry *idx = NULL;
+ struct bulk_checkin_source source = {
+ .type = SOURCE_FILE,
+ .fd = fd,
+ .size = size,
+ .path = path,
+ };
seekback = lseek(fd, 0, SEEK_CUR);
if (seekback == (off_t) -1)
@@ -342,10 +387,10 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
while (1) {
prepare_checkpoint(state, &checkpoint, idx, flags);
if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
- fd, size, path, flags))
+ &source, flags))
break;
truncate_checkpoint(state, &checkpoint, idx);
- if (lseek(fd, seekback, SEEK_SET) == (off_t) -1)
+ if (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)
return error("cannot seek back");
}
finalize_checkpoint(state, &ctx, &checkpoint, idx, result_oid);
--
2.42.0.408.g97fac66ae4
^ permalink raw reply related
* [PATCH v3 04/10] bulk-checkin: factor out `finalize_checkpoint()`
From: Taylor Blau @ 2023-10-18 17:07 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697648864.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.408.g97fac66ae4
^ permalink raw reply related
* [PATCH v3 02/10] bulk-checkin: factor out `prepare_checkpoint()`
From: Taylor Blau @ 2023-10-18 17:07 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697648864.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.408.g97fac66ae4
^ permalink raw reply related
* [PATCH v3 01/10] bulk-checkin: factor out `format_object_header_hash()`
From: Taylor Blau @ 2023-10-18 17:07 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697648864.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.408.g97fac66ae4
^ permalink raw reply related
* [PATCH v3 09/10] bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
From: Taylor Blau @ 2023-10-18 17:08 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697648864.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 9ae43648ba..d088a9c10b 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,
@@ -520,6 +536,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.408.g97fac66ae4
^ permalink raw reply related
* [PATCH v3 00/10] merge-ort: implement support for packing objects together
From: Taylor Blau @ 2023-10-18 17:07 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>
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.
The notable change from last time is in response to a suggestion[1] from
Junio to factor out an abstract bulk-checkin "source", which ended up
reducing the duplication between a couple of functions in the earlier
round by a significant degree.
Beyond that, the changes since last time can be viewed in the range-diff
below. Thanks in advance for any review!
[1]: https://lore.kernel.org/git/xmqq5y34wu5f.fsf@gitster.g/
Taylor Blau (10):
bulk-checkin: factor out `format_object_header_hash()`
bulk-checkin: factor out `prepare_checkpoint()`
bulk-checkin: factor out `truncate_checkpoint()`
bulk-checkin: factor out `finalize_checkpoint()`
bulk-checkin: extract abstract `bulk_checkin_source`
bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`
bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
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 | 288 +++++++++++++++++++++++++------
bulk-checkin.h | 8 +
merge-ort.c | 42 ++++-
merge-recursive.h | 1 +
t/t4301-merge-tree-write-tree.sh | 93 ++++++++++
7 files changed, 381 insertions(+), 60 deletions(-)
Range-diff against v2:
1: edf1cbafc1 = 1: 2dffa45183 bulk-checkin: factor out `format_object_header_hash()`
2: b3f89d5853 = 2: 7a10dc794a bulk-checkin: factor out `prepare_checkpoint()`
3: abe4fb0a59 = 3: 20c32d2178 bulk-checkin: factor out `truncate_checkpoint()`
4: 0b855a6eb7 ! 4: 893051d0b7 bulk-checkin: factor our `finalize_checkpoint()`
@@ Metadata
Author: Taylor Blau <me@ttaylorr.com>
## Commit message ##
- bulk-checkin: factor our `finalize_checkpoint()`
+ bulk-checkin: factor out `finalize_checkpoint()`
In a similar spirit as previous commits, factor out the routine to
finalize the just-written object from the bulk-checkin mechanism.
-: ---------- > 5: da52ec8380 bulk-checkin: extract abstract `bulk_checkin_source`
-: ---------- > 6: 4e9bac5bc1 bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`
-: ---------- > 7: 04ec74e357 bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
5: 239bf39bfb ! 8: 8667b76365 bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
@@ Commit message
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
+ deflate_obj_contents_to_pack_incore(), which takes advantage of the
+ earlier refactorings 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.
+ `stream_obj_to_pack()`, which can handle streaming objects from memory
+ to the bulk-checkin pack as a result of the earlier refactoring.
Consistent with the rest of the bulk-checkin mechanism, there are no
direct tests here. In future commits when we expose this new
@@ Commit message
Signed-off-by: Taylor Blau <me@ttaylorr.com>
## bulk-checkin.c ##
-@@ bulk-checkin.c: 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
@@ bulk-checkin.c: static void finalize_checkpoint(struct bulk_checkin_packfile *state,
}
}
@@ bulk-checkin.c: static void finalize_checkpoint(struct bulk_checkin_packfile *st
+{
+ struct pack_idx_entry *idx = NULL;
+ off_t already_hashed_to = 0;
++ struct bulk_checkin_source source = {
++ .type = SOURCE_INCORE,
++ .buf = buf,
++ .size = size,
++ .read = 0,
++ .path = path,
++ };
+
+ /* Note: idx is non-NULL when we are writing */
+ if (flags & HASH_WRITE_OBJECT)
@@ bulk-checkin.c: static void finalize_checkpoint(struct bulk_checkin_packfile *st
+
+ while (1) {
+ prepare_checkpoint(state, checkpoint, idx, flags);
-+ if (!stream_obj_to_pack_incore(state, ctx, &already_hashed_to,
-+ buf, size, type, path, flags))
++
++ if (!stream_obj_to_pack(state, ctx, &already_hashed_to, &source,
++ type, flags))
+ break;
+ truncate_checkpoint(state, checkpoint, idx);
++ bulk_checkin_source_seek_to(&source, 0);
+ }
+
+ finalize_checkpoint(state, ctx, checkpoint, idx, result_oid);
6: 57613807d8 = 9: cba043ef14 bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
7: f21400f56c = 10: ae70508037 builtin/merge-tree.c: implement support for `--write-pack`
--
2.42.0.408.g97fac66ae4
^ permalink raw reply
* Re: [PATCH 01/11] t: add helpers to test for reference existence
From: Eric Sunshine @ 2023-10-18 17:08 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <e947feb1c77f7e9f3c7f983bbe47137fbce42367.1697607222.git.ps@pks.im>
On Wed, Oct 18, 2023 at 1:35 AM Patrick Steinhardt <ps@pks.im> wrote:
> Introduce a new subcommand for our ref-store test helper that explicitly
> checks only for the presence or absence of a reference. This addresses
> these limitations:
> [...]
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
> @@ -221,6 +221,30 @@ static int cmd_verify_ref(struct ref_store *refs, const char **argv)
> +static int cmd_ref_exists(struct ref_store *refs, const char **argv)
> +{
> + const char *refname = notnull(*argv++, "refname");
> + struct strbuf unused_referent = STRBUF_INIT;
> + struct object_id unused_oid;
> + unsigned int unused_type;
> + int failure_errno;
> +
> + if (refs_read_raw_ref(refs, refname, &unused_oid, &unused_referent,
> + &unused_type, &failure_errno)) {
> + /*
> + * We handle ENOENT separately here such that it is possible to
> + * distinguish actually-missing references from any kind of
> + * generic error.
> + */
> + if (failure_errno == ENOENT)
> + return 17;
> + return -1;
> + }
> +
> + strbuf_release(&unused_referent);
> + return 0;
> +}
Unless refs_read_raw_ref() guarantees that `unused_referent` remains
unallocated upon failure[*], then the early returns inside the
conditional leak the strbuf. True, the program is exiting immediately
anyhow, so this (potential) leak isn't significant, but it seems odd
to clean up in one case (return 0) but not in the others (return -1 &
17).
[*] In my (admittedly brief) scan of the code and documentation, I
didn't see any such promise.
^ permalink raw reply
* [PATCH v3 08/10] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
From: Taylor Blau @ 2023-10-18 17:08 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697648864.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_obj_contents_to_pack_incore(), which takes advantage of the
earlier refactorings 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()`, which can handle streaming objects from memory
to the bulk-checkin pack as a result of the earlier refactoring.
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 | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++
bulk-checkin.h | 4 ++++
2 files changed, 68 insertions(+)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index f0115efb2e..9ae43648ba 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -370,6 +370,59 @@ 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;
+ struct bulk_checkin_source source = {
+ .type = SOURCE_INCORE,
+ .buf = buf,
+ .size = size,
+ .read = 0,
+ .path = path,
+ };
+
+ /* 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(state, ctx, &already_hashed_to, &source,
+ type, flags))
+ break;
+ truncate_checkpoint(state, checkpoint, idx);
+ bulk_checkin_source_seek_to(&source, 0);
+ }
+
+ 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,
@@ -456,6 +509,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.408.g97fac66ae4
^ permalink raw reply related
* [PATCH v3 07/10] bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
From: Taylor Blau @ 2023-10-18 17:08 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697648864.git.me@ttaylorr.com>
The existing `stream_blob_to_pack()` function is named based on the fact
that it knows only how to stream blobs into a bulk-checkin pack.
But there is no longer anything in this function which prevents us from
writing objects of arbitrary types to the bulk-checkin pack. Prepare to
write OBJ_TREEs by removing this assumption, adding an `enum
object_type` parameter to this function's argument list, and renaming it
to `stream_obj_to_pack()` accordingly.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 133e02ce36..f0115efb2e 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -204,10 +204,10 @@ static ssize_t bulk_checkin_source_read(struct bulk_checkin_source *source,
* status before calling us just in case we ask it to call us again
* with a new pack.
*/
-static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
- git_hash_ctx *ctx, off_t *already_hashed_to,
- struct bulk_checkin_source *source,
- unsigned flags)
+static int stream_obj_to_pack(struct bulk_checkin_packfile *state,
+ git_hash_ctx *ctx, off_t *already_hashed_to,
+ struct bulk_checkin_source *source,
+ enum object_type type, unsigned flags)
{
git_zstream s;
unsigned char ibuf[16384];
@@ -220,8 +220,7 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
git_deflate_init(&s, pack_compression_level);
- hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB,
- size);
+ hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);
s.next_out = obuf + hdrlen;
s.avail_out = sizeof(obuf) - hdrlen;
@@ -402,8 +401,8 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
while (1) {
prepare_checkpoint(state, &checkpoint, idx, flags);
- if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
- &source, flags))
+ if (!stream_obj_to_pack(state, &ctx, &already_hashed_to,
+ &source, OBJ_BLOB, flags))
break;
truncate_checkpoint(state, &checkpoint, idx);
if (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)
--
2.42.0.408.g97fac66ae4
^ permalink raw reply related
* [PATCH v3 10/10] builtin/merge-tree.c: implement support for `--write-pack`
From: Taylor Blau @ 2023-10-18 17:08 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697648864.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.408.g97fac66ae4
^ permalink raw reply related
* [PATCH v3 03/10] bulk-checkin: factor out `truncate_checkpoint()`
From: Taylor Blau @ 2023-10-18 17:07 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697648864.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.408.g97fac66ae4
^ permalink raw reply related
* Re: [PATCH v2 5/7] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
From: Taylor Blau @ 2023-10-18 16:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Elijah Newren, Eric W. Biederman, Jeff King,
Patrick Steinhardt
In-Reply-To: <xmqq5y34wu5f.fsf@gitster.g>
On Tue, Oct 17, 2023 at 07:18:04PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > bulk-checkin.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++++
> > bulk-checkin.h | 4 ++
> > 2 files changed, 122 insertions(+)
>
> Unlike the previous four, which were very clear refactoring to
> create reusable helper functions, this step leaves a bad aftertaste
> after reading twice, and I think what is disturbing is that many new
> lines are pretty much literally copied from stream_blob_to_pack().
>
> I wonder if we can introduce an "input" source abstraction, that
> replaces "fd" and "size" (and "path" for error reporting) parameters
> to the stream_blob_to_pack(), so that the bulk of the implementation
> of stream_blob_to_pack() can call its .read() method to read bytes
> up to "size" from such an abstracted interface? That would be a
> good sized first half of this change. Then in the second half, you
> can add another "input" source that works with in-core "buf" and
> "size", whose .read() method will merely be a memcpy().
Thanks, I like this idea. I had initially avoided it in the first couple
of rounds, because the abstraction felt clunky and involved an
unnecessary extra memcpy().
But having applied your suggestion here, I think that the price is well
worth the result, which is that `stream_blob_to_pack()` does not have to
be implemented twice with very subtle differences.
Thanks again for the suggestion, I'm really pleased with how it came
out. Reroll coming shortly...
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 03/11] t: convert tests to use helpers for reference existence
From: Junio C Hamano @ 2023-10-18 16:28 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <ac6a49c7c84ad2b836f099557dd6989703ebda8f.1697607222.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Convert tests that use `test_path_is_file` and `test_path_is_missing` to
> instead use our new helpers `test_ref_exists` and `test_ref_missing`.
> These hook into the reference database directly and thus work indepently
> of the actual reference backend that is being used.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> t/t1430-bad-ref-name.sh | 27 ++++++++++++++++++---------
> t/t3200-branch.sh | 33 ++++++++++++++++++---------------
> t/t5521-pull-options.sh | 4 ++--
> t/t5605-clone-local.sh | 2 +-
> 4 files changed, 39 insertions(+), 27 deletions(-)
I scanned through the changes, and all of them looked sensible. I
also very much liked the added "now we have done something to create
'badname' branch, let's make sure it exists, before trying to see if
the machinery to delete it works correctly" tests.
Looking good.
Thanks.
> diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
> index ff1c967d550..7b7d6953c62 100755
> --- a/t/t1430-bad-ref-name.sh
> +++ b/t/t1430-bad-ref-name.sh
> @@ -205,8 +205,9 @@ test_expect_success 'update-ref --no-deref -d can delete symref to broken name'
> test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...ref" &&
> test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
> test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
> + test_ref_exists refs/heads/badname &&
> git update-ref --no-deref -d refs/heads/badname >output 2>error &&
> - test_path_is_missing .git/refs/heads/badname &&
> + test_ref_missing refs/heads/badname &&
> test_must_be_empty output &&
> test_must_be_empty error
> '
> @@ -216,8 +217,9 @@ test_expect_success 'branch -d can delete symref to broken name' '
> test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...ref" &&
> test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
> test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
> + test_ref_exists refs/heads/badname &&
> git branch -d badname >output 2>error &&
> - test_path_is_missing .git/refs/heads/badname &&
> + test_ref_missing refs/heads/badname &&
> test_i18ngrep "Deleted branch badname (was refs/heads/broken\.\.\.ref)" output &&
> test_must_be_empty error
> '
> @@ -225,8 +227,9 @@ test_expect_success 'branch -d can delete symref to broken name' '
> test_expect_success 'update-ref --no-deref -d can delete dangling symref to broken name' '
> test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
> test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
> + test_ref_exists refs/heads/badname &&
> git update-ref --no-deref -d refs/heads/badname >output 2>error &&
> - test_path_is_missing .git/refs/heads/badname &&
> + test_ref_missing refs/heads/badname &&
> test_must_be_empty output &&
> test_must_be_empty error
> '
> @@ -234,8 +237,9 @@ test_expect_success 'update-ref --no-deref -d can delete dangling symref to brok
> test_expect_success 'branch -d can delete dangling symref to broken name' '
> test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
> test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
> + test_ref_exists refs/heads/badname &&
> git branch -d badname >output 2>error &&
> - test_path_is_missing .git/refs/heads/badname &&
> + test_ref_missing refs/heads/badname &&
> test_i18ngrep "Deleted branch badname (was refs/heads/broken\.\.\.ref)" output &&
> test_must_be_empty error
> '
> @@ -245,8 +249,9 @@ test_expect_success 'update-ref -d can delete broken name through symref' '
> test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...ref" &&
> test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
> test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
> + test_ref_exists refs/heads/broken...ref &&
> git update-ref -d refs/heads/badname >output 2>error &&
> - test_path_is_missing .git/refs/heads/broken...ref &&
> + test_ref_missing refs/heads/broken...ref &&
> test_must_be_empty output &&
> test_must_be_empty error
> '
> @@ -254,8 +259,9 @@ test_expect_success 'update-ref -d can delete broken name through symref' '
> test_expect_success 'update-ref --no-deref -d can delete symref with broken name' '
> printf "ref: refs/heads/main\n" >.git/refs/heads/broken...symref &&
> test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
> + test_ref_exists refs/heads/broken...symref &&
> git update-ref --no-deref -d refs/heads/broken...symref >output 2>error &&
> - test_path_is_missing .git/refs/heads/broken...symref &&
> + test_ref_missing refs/heads/broken...symref &&
> test_must_be_empty output &&
> test_must_be_empty error
> '
> @@ -263,8 +269,9 @@ test_expect_success 'update-ref --no-deref -d can delete symref with broken name
> test_expect_success 'branch -d can delete symref with broken name' '
> printf "ref: refs/heads/main\n" >.git/refs/heads/broken...symref &&
> test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
> + test_ref_exists refs/heads/broken...symref &&
> git branch -d broken...symref >output 2>error &&
> - test_path_is_missing .git/refs/heads/broken...symref &&
> + test_ref_missing refs/heads/broken...symref &&
> test_i18ngrep "Deleted branch broken...symref (was refs/heads/main)" output &&
> test_must_be_empty error
> '
> @@ -272,8 +279,9 @@ test_expect_success 'branch -d can delete symref with broken name' '
> test_expect_success 'update-ref --no-deref -d can delete dangling symref with broken name' '
> printf "ref: refs/heads/idonotexist\n" >.git/refs/heads/broken...symref &&
> test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
> + test_ref_exists refs/heads/broken...symref &&
> git update-ref --no-deref -d refs/heads/broken...symref >output 2>error &&
> - test_path_is_missing .git/refs/heads/broken...symref &&
> + test_ref_missing refs/heads/broken...symref &&
> test_must_be_empty output &&
> test_must_be_empty error
> '
> @@ -281,8 +289,9 @@ test_expect_success 'update-ref --no-deref -d can delete dangling symref with br
> test_expect_success 'branch -d can delete dangling symref with broken name' '
> printf "ref: refs/heads/idonotexist\n" >.git/refs/heads/broken...symref &&
> test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
> + test_ref_exists refs/heads/broken...symref &&
> git branch -d broken...symref >output 2>error &&
> - test_path_is_missing .git/refs/heads/broken...symref &&
> + test_ref_missing refs/heads/broken...symref &&
> test_i18ngrep "Deleted branch broken...symref (was refs/heads/idonotexist)" output &&
> test_must_be_empty error
> '
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 080e4f24a6e..bde4f1485b7 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -25,7 +25,7 @@ test_expect_success 'prepare a trivial repository' '
>
> test_expect_success 'git branch --help should not have created a bogus branch' '
> test_might_fail git branch --man --help </dev/null >/dev/null 2>&1 &&
> - test_path_is_missing .git/refs/heads/--help
> + test_ref_missing refs/heads/--help
> '
>
> test_expect_success 'branch -h in broken repository' '
> @@ -40,7 +40,8 @@ test_expect_success 'branch -h in broken repository' '
> '
>
> test_expect_success 'git branch abc should create a branch' '
> - git branch abc && test_path_is_file .git/refs/heads/abc
> + git branch abc &&
> + test_ref_exists refs/heads/abc
> '
>
> test_expect_success 'git branch abc should fail when abc exists' '
> @@ -61,11 +62,13 @@ test_expect_success 'git branch --force abc should succeed when abc exists' '
> '
>
> test_expect_success 'git branch a/b/c should create a branch' '
> - git branch a/b/c && test_path_is_file .git/refs/heads/a/b/c
> + git branch a/b/c &&
> + test_ref_exists refs/heads/a/b/c
> '
>
> test_expect_success 'git branch mb main... should create a branch' '
> - git branch mb main... && test_path_is_file .git/refs/heads/mb
> + git branch mb main... &&
> + test_ref_exists refs/heads/mb
> '
>
> test_expect_success 'git branch HEAD should fail' '
> @@ -78,14 +81,14 @@ EOF
> test_expect_success 'git branch --create-reflog d/e/f should create a branch and a log' '
> GIT_COMMITTER_DATE="2005-05-26 23:30" \
> git -c core.logallrefupdates=false branch --create-reflog d/e/f &&
> - test_path_is_file .git/refs/heads/d/e/f &&
> + test_ref_exists refs/heads/d/e/f &&
> test_path_is_file .git/logs/refs/heads/d/e/f &&
> test_cmp expect .git/logs/refs/heads/d/e/f
> '
>
> test_expect_success 'git branch -d d/e/f should delete a branch and a log' '
> git branch -d d/e/f &&
> - test_path_is_missing .git/refs/heads/d/e/f &&
> + test_ref_missing refs/heads/d/e/f &&
> test_must_fail git reflog exists refs/heads/d/e/f
> '
>
> @@ -213,7 +216,7 @@ test_expect_success 'git branch -M should leave orphaned HEAD alone' '
> test_commit initial &&
> git checkout --orphan lonely &&
> grep lonely .git/HEAD &&
> - test_path_is_missing .git/refs/head/lonely &&
> + test_ref_missing refs/head/lonely &&
> git branch -M main mistress &&
> grep lonely .git/HEAD
> )
> @@ -799,8 +802,8 @@ test_expect_success 'deleting a symref' '
> git symbolic-ref refs/heads/symref refs/heads/target &&
> echo "Deleted branch symref (was refs/heads/target)." >expect &&
> git branch -d symref >actual &&
> - test_path_is_file .git/refs/heads/target &&
> - test_path_is_missing .git/refs/heads/symref &&
> + test_ref_exists refs/heads/target &&
> + test_ref_missing refs/heads/symref &&
> test_cmp expect actual
> '
>
> @@ -809,16 +812,16 @@ test_expect_success 'deleting a dangling symref' '
> test_path_is_file .git/refs/heads/dangling-symref &&
> echo "Deleted branch dangling-symref (was nowhere)." >expect &&
> git branch -d dangling-symref >actual &&
> - test_path_is_missing .git/refs/heads/dangling-symref &&
> + test_ref_missing refs/heads/dangling-symref &&
> test_cmp expect actual
> '
>
> test_expect_success 'deleting a self-referential symref' '
> git symbolic-ref refs/heads/self-reference refs/heads/self-reference &&
> - test_path_is_file .git/refs/heads/self-reference &&
> + test_ref_exists refs/heads/self-reference &&
> echo "Deleted branch self-reference (was refs/heads/self-reference)." >expect &&
> git branch -d self-reference >actual &&
> - test_path_is_missing .git/refs/heads/self-reference &&
> + test_ref_missing refs/heads/self-reference &&
> test_cmp expect actual
> '
>
> @@ -826,8 +829,8 @@ test_expect_success 'renaming a symref is not allowed' '
> git symbolic-ref refs/heads/topic refs/heads/main &&
> test_must_fail git branch -m topic new-topic &&
> git symbolic-ref refs/heads/topic &&
> - test_path_is_file .git/refs/heads/main &&
> - test_path_is_missing .git/refs/heads/new-topic
> + test_ref_exists refs/heads/main &&
> + test_ref_missing refs/heads/new-topic
> '
>
> test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog for u is a symlink' '
> @@ -1142,7 +1145,7 @@ EOF
> test_expect_success 'git checkout -b g/h/i -l should create a branch and a log' '
> GIT_COMMITTER_DATE="2005-05-26 23:30" \
> git checkout -b g/h/i -l main &&
> - test_path_is_file .git/refs/heads/g/h/i &&
> + test_ref_exists refs/heads/g/h/i &&
> test_path_is_file .git/logs/refs/heads/g/h/i &&
> test_cmp expect .git/logs/refs/heads/g/h/i
> '
> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
> index 079b2f2536e..3681859f983 100755
> --- a/t/t5521-pull-options.sh
> +++ b/t/t5521-pull-options.sh
> @@ -143,7 +143,7 @@ test_expect_success 'git pull --dry-run' '
> cd clonedry &&
> git pull --dry-run ../parent &&
> test_path_is_missing .git/FETCH_HEAD &&
> - test_path_is_missing .git/refs/heads/main &&
> + test_ref_missing refs/heads/main &&
> test_path_is_missing .git/index &&
> test_path_is_missing file
> )
> @@ -157,7 +157,7 @@ test_expect_success 'git pull --all --dry-run' '
> git remote add origin ../parent &&
> git pull --all --dry-run &&
> test_path_is_missing .git/FETCH_HEAD &&
> - test_path_is_missing .git/refs/remotes/origin/main &&
> + test_ref_missing refs/remotes/origin/main &&
> test_path_is_missing .git/index &&
> test_path_is_missing file
> )
> diff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh
> index 1d7b1abda1a..946c5751885 100755
> --- a/t/t5605-clone-local.sh
> +++ b/t/t5605-clone-local.sh
> @@ -69,7 +69,7 @@ test_expect_success 'local clone of repo with nonexistent ref in HEAD' '
> git clone a d &&
> (cd d &&
> git fetch &&
> - test ! -e .git/refs/remotes/origin/HEAD)
> + test_ref_missing refs/remotes/origin/HEAD)
> '
>
> test_expect_success 'bundle clone without .bundle suffix' '
^ permalink raw reply
* Re: [PATCH 02/11] t: allow skipping expected object ID in `ref-store update-ref`
From: Junio C Hamano @ 2023-10-18 16:08 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <1f615d62f99e9ab47d37500f05b29615bafffba2.1697607222.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> We require the caller to pass both the old and new expected object ID to
> our `test-tool ref-store update-ref` helper. When trying to update a
> symbolic reference though it's impossible to specify the expected object
> ID, which means that the test would instead have to force-update the
> reference. This is currently impossible though.
>
> Update the helper to optionally skip verification of the old object ID
> in case the test passes in an empty old object ID as input.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> t/helper/test-ref-store.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
Good.
Even better would be to make the old one optional, though.
> diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
> index 7400f560ab6..7dc83137584 100644
> --- a/t/helper/test-ref-store.c
> +++ b/t/helper/test-ref-store.c
> @@ -322,16 +322,19 @@ static int cmd_update_ref(struct ref_store *refs, const char **argv)
> const char *new_sha1_buf = notnull(*argv++, "new-sha1");
> const char *old_sha1_buf = notnull(*argv++, "old-sha1");
> unsigned int flags = arg_flags(*argv++, "flags", transaction_flags);
> - struct object_id old_oid;
> + struct object_id old_oid, *old_oid_ptr = NULL;
> struct object_id new_oid;
>
> - if (get_oid_hex(old_sha1_buf, &old_oid))
> - die("cannot parse %s as %s", old_sha1_buf, the_hash_algo->name);
> + if (*old_sha1_buf) {
> + if (get_oid_hex(old_sha1_buf, &old_oid))
> + die("cannot parse %s as %s", old_sha1_buf, the_hash_algo->name);
> + old_oid_ptr = &old_oid;
> + }
> if (get_oid_hex(new_sha1_buf, &new_oid))
> die("cannot parse %s as %s", new_sha1_buf, the_hash_algo->name);
>
> return refs_update_ref(refs, msg, refname,
> - &new_oid, &old_oid,
> + &new_oid, old_oid_ptr,
> flags, UPDATE_REFS_DIE_ON_ERR);
> }
^ permalink raw reply
* Re: [PATCH 01/11] t: add helpers to test for reference existence
From: Junio C Hamano @ 2023-10-18 16:06 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <e947feb1c77f7e9f3c7f983bbe47137fbce42367.1697607222.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> There are two major ways to check for the existence of a reference in
> our tests:
>
> - `git rev-parse --verify` can be used to check for existence of a
> reference. This only works in the case where the reference is well
> formed though and resolves to an actual object ID. This does not
> work with malformed reference names or invalid contents.
>
> - `test_path_is_file` can be used to check for existence of a loose
> reference if it is known to not resolve to an actual object ID. It
> by necessity reaches into implementation details of the reference
> backend though.
True. It would be ideal if we can limit the use of latter when we
_care_ how the ref is stored (e.g., "we expect it to be stored as a
loose ref, not packed"). "The ref R at must be pointing at the
commit X" is better asserted by using the former (or "git show-ref")
as we not just only want to see the .git/refs/R file holding the
object name X, but also want to see the production git tools observe
the same---if what rev-parse or show-ref observes is different from
the expected state and they say ref R does not point at commit X, we
should complain (rev-parse or show-ref may be broken in the version
of Git being tested, but we can assume that their breakage will be
caught elsewhere in the test suite as well, so as long as we trust
them, using them as the validator is better than going into the
implementation detail and assuming things like "new refs always
appear as a loose ref" that we might want to change in the future).
> Similarly, there are two equivalent ways to check for the absence of a
> reference:
>
> - `test_must_fail git rev-parse` can be used to check for the
> absence of a reference. It could fail due to a number of reasons
> though, and all of these reasons will be thrown into the same bag
> as an absent reference.
>
> - `test_path_is_missing` can be used to check explicitly for the
> absence of a loose reference, but again reaches into internal
> implementation details of the reference backend.
>
> So both our tooling to check for the presence and for the absence of
> references in tests is lacking as either failure cases are thrown into
> the same bag or we need to reach into internal implementation details of
> the respective reference backend.
> Introduce a new subcommand for our ref-store test helper that explicitly
> checks only for the presence or absence of a reference. This addresses
> these limitations:
>
> - We can check for the presence of references with malformed names.
But for the purpose of tests, we can control the input. When we
perform an operation that we expect a ref R to be created, we would
know R is well formed and we can validate using a tool that we know
would be broken when fed a malformed name. So I do not see this as
a huge "limitation".
> - We can check for the presence of references that don't resolve.
Do you mean a dangling symbolic ref? We are using a wrong tool if
you are using rev-parse for that, aren't we? Isn't symbolic-ref
there for us for this exact use case? That is
> - We can explicitly handle the case where a reference is missing by
> special-casing ENOENT errors.
You probably know the error conditions refs_read_raw_ref() can be
broken better than I do, but this feels a bit too intimate with how
the method for the files backend happens to be implemented, which at
the same time, can risk that [a] other backends can implement their
"ref does not resolve to an object name---is it because it is
missing?" report incorrectly and [b] we would eventually want to
know error conditions other than "the ref requested is missing" and
at that point we would need more "special casing", which does not
smell easy to scale.
> - We don't need to reach into implementation details of the backend,
> which would allow us to use this helper for the future reftable
> backend.
This is exactly what we want to aim for.
> Next to this subcommand we also provide two wrappers `test_ref_exists`
> and `test_ref_missing` that make the helper easier to use.
Hmmmm. This may introduce "who watches the watchers" problem, no?
I briefly wondered if a better approach is to teach the production
code, e.g., rev-parse, to optionally give more detailed diag. It
essentially may be the same (making the code in test-ref-store.c
added by this patch available from rev-parse, we would easily get
there), so I do not think the distinction matters.
> diff --git a/t/README b/t/README
> index 61080859899..779f7e7dd86 100644
> --- a/t/README
> +++ b/t/README
> @@ -928,6 +928,15 @@ see test-lib-functions.sh for the full list and their options.
> committer times to defined state. Subsequent calls will
> advance the times by a fixed amount.
>
> + - test_ref_exists <ref>, test_ref_missing <ref>
> +
> + Check whether a reference exists or is missing. In contrast to
> + git-rev-parse(1), these helpers also work with invalid reference
> + names and references whose contents are unresolvable. The latter
> + function also distinguishes generic errors from the case where a
> + reference explicitly doesn't exist and is thus safer to use than
> + `test_must_fail git rev-parse`.
> +
> - test_commit <message> [<filename> [<contents>]]
>
> Creates a commit with the given message, committing the given
> diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
> index 48552e6a9e0..7400f560ab6 100644
> --- a/t/helper/test-ref-store.c
> +++ b/t/helper/test-ref-store.c
> @@ -1,6 +1,6 @@
> #include "test-tool.h"
> #include "hex.h"
> -#include "refs.h"
> +#include "refs/refs-internal.h"
> #include "setup.h"
> #include "worktree.h"
> #include "object-store-ll.h"
> @@ -221,6 +221,30 @@ static int cmd_verify_ref(struct ref_store *refs, const char **argv)
> return ret;
> }
>
> +static int cmd_ref_exists(struct ref_store *refs, const char **argv)
> +{
> + const char *refname = notnull(*argv++, "refname");
> + struct strbuf unused_referent = STRBUF_INIT;
> + struct object_id unused_oid;
> + unsigned int unused_type;
> + int failure_errno;
> +
> + if (refs_read_raw_ref(refs, refname, &unused_oid, &unused_referent,
> + &unused_type, &failure_errno)) {
> + /*
> + * We handle ENOENT separately here such that it is possible to
> + * distinguish actually-missing references from any kind of
> + * generic error.
> + */
> + if (failure_errno == ENOENT)
> + return 17;
Can we tell between the cases where the ref itself is missing, and
the requested ref is symbolic and points at a missing ref? This
particular case might be OK, but there may other cases where this
"special case" may not be narrow enough.
As long we are going to spend cycles to refine the classification of
error conditions, which is a very good thing to aim for the reason
described in the proposed log message, namely "rev-parse can fail
for reasons other than the ref being absent", I have to wonder again
that the fruit of such an effort should become available in the
production code, instead of being kept only in test-tool.
^ permalink raw reply
* Re: [PATCH 00/11] t: reduce direct disk access to data structures
From: Junio C Hamano @ 2023-10-18 15:32 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <cover.1697607222.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> this patch series refactors a bunch of our tests to perform less direct
> disk access to on-disk data structures. Instead, the tests are converted
> to use Git tools or our test-tool to access data to the best extent
> possible. This serves two benefits:
Laudable goal.
> - We increase test coverage of our own code base.
Meaning the new code added to test-tool for this series will also
get tested and bugs spotted?
> - We become less dependent on the actual on-disk format.
Yes, this is very desirable. Without looking at the implementation,
I see some issues aiming for this goal may involve. [a] using the
production code for validation would mean our expectation to be
compared to the reality to be validated can be affected by the same
bug, making two wrongs to appear right; [b] using a separate
implementation used only for validation would at least mean we will
have to make the same mistake in unique part of both implementations
that is less likely to miss bugs compared to [a], but bugs in shared
part of the production code and validation code will be hidden the
same way as [a].
But you have thought about this series a lot deeper and longer than
I have, so let me read on and find out what your solution is.
> The main motivation for this patch series was the second bullet point as
> it is preparatory work to get the reftable backend upstreamed.
Yay.
> My intent
> is to get rid of many or even most of the current blockers in the Git
> project before trying to send the reftable implementation upstream.
> While this will be a lot of up-front work that is going to span over a
> long time period, I think this approach will make everyones live easier
> by doing comparatively small and incremental improvements to the Git
> project.
Thank you very much for being considerate. I assume this approach
is what those in the Contributors' Summit agreed to be the best way
forward to do anything sizeable.
> Ultimately, the final patch series should in the best case only
> contain the new backend as well as testing infrastructure, but not much
> else.
;-)
^ permalink raw reply
* Re: [PATCH 08/11] t4207: delete replace references via git-update-ref(1)
From: Han-Wen Nienhuys @ 2023-10-18 13:27 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <c4d09e3e5dbd11221cc4d229b815434d441cdb4d.1697607222.git.ps@pks.im>
On Wed, Oct 18, 2023 at 7:35 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> In t4207 we set up a set of replace objects via git-replace(1). Because
> these references should not be impacting subsequent tests we also set up
> some cleanup logic that deletes the replacement references via a call to
> `rm -rf`. This reaches into the internal implementation details of the
> reference backend and will thus break when we grow an alternative refdb
> implementation.
>
> Refactor the tests to delete the replacement refs via Git commands so
> that we become independent of the actual refdb that's in use. As we
> don't have a nice way to delete all replacements or all references in a
> certain namespace, we opt for a combination of git-for-each-ref(1) and
> git-update-ref(1)'s `--stdin` mode.
There is a test helper that can directly access the ref database,
t/helper/test-ref-store.c.
If you use that manipulate refs for testing purposes, you make the
test independent of behavior
git-for-each-ref/git-update-ref, which is what you want for testing
replace-objects?
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Liana Sebastian
^ 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