From e879295b208caffe03ce08d789bd81626b12c208 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Oct 2021 16:30:36 -0400 Subject: [PATCH 1/5] t1006: clean up broken objects A few of the tests create intentionally broken objects with broken types. Let's clean them up after we're done with them, so that later tests don't get confused (we hadn't noticed because this only affects tests which use --batch-all-objects, but I'm about to add more). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1006-cat-file.sh | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh index 18b3779ccb..c77db35728 100755 --- a/t/t1006-cat-file.sh +++ b/t/t1006-cat-file.sh @@ -331,6 +331,11 @@ test_expect_success "Size of broken object is correct" ' git cat-file -s --allow-unknown-type $bogus_sha1 >actual && test_cmp expect actual ' + +test_expect_success 'clean up broken object' ' + rm .git/objects/$(test_oid_to_path $bogus_sha1) +' + bogus_type="abcdefghijklmnopqrstuvwxyz1234679" bogus_content="bogus" bogus_size=$(strlen "$bogus_content") @@ -348,6 +353,10 @@ test_expect_success "Size of large broken object is correct when type is large" test_cmp expect actual ' +test_expect_success 'clean up broken object' ' + rm .git/objects/$(test_oid_to_path $bogus_sha1) +' + # Tests for git cat-file --follow-symlinks test_expect_success 'prep for symlink tests' ' echo_without_newline "$hello_content" >morx && From c3660cfb031650b0fe384e3e27c06667f7ae5099 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Oct 2021 16:31:08 -0400 Subject: [PATCH 2/5] cat-file: mention --unordered along with --batch-all-objects The note on ordering for --batch-all-objects was written when that was the only possible ordering. These days we have --unordered, too, so let's point to it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/git-cat-file.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt index 4eb0421b3f..6467707c6e 100644 --- a/Documentation/git-cat-file.txt +++ b/Documentation/git-cat-file.txt @@ -94,8 +94,9 @@ OPTIONS Instead of reading a list of objects on stdin, perform the requested batch operation on all objects in the repository and any alternate object stores (not just reachable objects). - Requires `--batch` or `--batch-check` be specified. Note that - the objects are visited in order sorted by their hashes. + Requires `--batch` or `--batch-check` be specified. By default, + the objects are visited in order sorted by their hashes; see + also `--unordered` below. --buffer:: Normally batch output is flushed after each object is output, so From 5c5b29b459dd6e169c9114910adca9203532f7d7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Oct 2021 16:36:07 -0400 Subject: [PATCH 3/5] cat-file: disable refs/replace with --batch-all-objects When we're enumerating all objects in the object database, it doesn't make sense to respect refs/replace. The point of this option is to enumerate all of the objects in the database at a low level. By definition we'd already show the replacement object's contents (under its real oid), and showing those contents under another oid is almost certainly working against what the user is trying to do. Note that you could make the same argument for something like: git show-index Signed-off-by: Junio C Hamano --- Documentation/git-cat-file.txt | 3 +- builtin/cat-file.c | 2 ++ t/t1006-cat-file.sh | 66 ++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 1 deletion(-) diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt index 6467707c6e..27b27e2b30 100644 --- a/Documentation/git-cat-file.txt +++ b/Documentation/git-cat-file.txt @@ -96,7 +96,8 @@ OPTIONS any alternate object stores (not just reachable objects). Requires `--batch` or `--batch-check` be specified. By default, the objects are visited in order sorted by their hashes; see - also `--unordered` below. + also `--unordered` below. Objects are presented as-is, without + respecting the "replace" mechanism of linkgit:git-replace[1]. --buffer:: Normally batch output is flushed after each object is output, so diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 243fe6844b..b713be545e 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -529,6 +529,8 @@ static int batch_objects(struct batch_options *opt) if (has_promisor_remote()) warning("This repository uses promisor remotes. Some objects may not be loaded."); + read_replace_refs = 0; + cb.opt = opt; cb.expand = &data; cb.scratch = &output; diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh index c77db35728..4a753705ec 100755 --- a/t/t1006-cat-file.sh +++ b/t/t1006-cat-file.sh @@ -617,4 +617,70 @@ test_expect_success 'cat-file --batch="batman" with --batch-all-objects will wor cmp expect actual ' +test_expect_success 'set up replacement object' ' + orig=$(git rev-parse HEAD) && + git cat-file commit $orig >orig && + { + cat orig && + echo extra + } >fake && + fake=$(git hash-object -t commit -w fake) && + orig_size=$(git cat-file -s $orig) && + fake_size=$(git cat-file -s $fake) && + git replace $orig $fake +' + +test_expect_success 'cat-file --batch respects replace objects' ' + git cat-file --batch >actual <<-EOF && + $orig + EOF + { + echo "$orig commit $fake_size" && + cat fake && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success 'cat-file --batch-check respects replace objects' ' + git cat-file --batch-check >actual <<-EOF && + $orig + EOF + echo "$orig commit $fake_size" >expect && + test_cmp expect actual +' + +# Pull the entry for object with oid "$1" out of the output of +# "cat-file --batch", including its object content (which requires +# parsing and reading a set amount of bytes, hence perl). +extract_batch_output () { + perl -ne ' + BEGIN { $oid = shift } + if (/^$oid \S+ (\d+)$/) { + print; + read STDIN, my $buf, $1; + print $buf; + print "\n"; + } + ' "$@" +} + +test_expect_success 'cat-file --batch-all-objects --batch ignores replace' ' + git cat-file --batch-all-objects --batch >actual.raw && + extract_batch_output $orig actual && + { + echo "$orig commit $orig_size" && + cat orig && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success 'cat-file --batch-all-objects --batch-check ignores replace' ' + git cat-file --batch-all-objects --batch-check >actual.raw && + grep ^$orig actual.raw >actual && + echo "$orig commit $orig_size" >expect && + test_cmp expect actual +' + test_done From 818e393084351324501b90142f7e8f95997db62b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Oct 2021 16:36:17 -0400 Subject: [PATCH 4/5] cat-file: split ordered/unordered batch-all-objects callbacks When we originally added --batch-all-objects, it stuffed everything into an oid_array(), and then iterated over that array with a callback to write the actual output. When we later added --unordered, that code path writes immediately as we discover each object, but just calls the same batch_object_cb() as our entry point to the writing code. That callback has a narrow interface; it only receives the oid, but we know much more about each object in the unordered write (which we'll make use of in the next patch). So let's just call batch_object_write() directly. The callback wasn't saving us much effort. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/cat-file.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index b713be545e..b533935d5c 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -470,7 +470,9 @@ static int batch_unordered_object(const struct object_id *oid, void *vdata) if (oidset_insert(data->seen, oid)) return 0; - return batch_object_cb(oid, data); + oidcpy(&data->expand->oid, oid); + batch_object_write(NULL, data->scratch, data->opt, data->expand); + return 0; } static int batch_unordered_loose(const struct object_id *oid, From bf972896d7186f0d29f7807b600f6fdb2837de18 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Oct 2021 16:38:04 -0400 Subject: [PATCH 5/5] cat-file: use packed_object_info() for --batch-all-objects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When "cat-file --batch-all-objects" iterates over each object, it knows where to find each one. But when we look up details of the object, we don't use that information at all. This patch teaches it to use the pack/offset pair when we're iterating over objects in a pack. This yields a measurable speed improvement (timings on a fully packed clone of linux.git): Benchmark #1: ./git.old cat-file --batch-all-objects --unordered --batch-check="%(objecttype) %(objectname)" Time (mean ± σ): 8.128 s ± 0.118 s [User: 7.968 s, System: 0.156 s] Range (min … max): 8.007 s … 8.301 s 10 runs Benchmark #2: ./git.new cat-file --batch-all-objects --unordered --batch-check="%(objecttype) %(objectname)" Time (mean ± σ): 4.294 s ± 0.064 s [User: 4.167 s, System: 0.125 s] Range (min … max): 4.227 s … 4.457 s 10 runs Summary './git.new cat-file --batch-all-objects --unordered --batch-check="%(objecttype) %(objectname)"' ran 1.89 ± 0.04 times faster than './git.old cat-file --batch-all-objects --unordered --batch-check="%(objecttype) %(objectname)" The implementation is pretty simple: we just call packed_object_info() instead of oid_object_info_extended() when we can. Most of the changes are just plumbing the pack/offset pair through the callstack. There is one subtlety: replace lookups are not handled by packed_object_info(). But since those are disabled for --batch-all-objects, and since we'll only have pack info when that option is in effect, we don't have to worry about that. There are a few limitations to this optimization which we could address with further work: - I didn't bother recording when we found an object loose. Technically this could save us doing a fruitless lookup in the pack index. But opening and mmap-ing a loose object is so expensive in the first place that this doesn't matter much. And if your repository is large enough to care about per-object performance, most objects are going to be packed anyway. - This works only in --unordered mode. For the sorted mode, we'd have to record the pack/offset pair as part of our oid-collection. That's more code, plus at least 16 extra bytes of heap per object. It would probably still be a net win in runtime, but we'd need to measure. - For --batch, this still helps us with getting the object metadata, but we still do a from-scratch lookup for the object contents. This probably doesn't matter that much, because the lookup cost will be much smaller relative to the cost of actually unpacking and printing the objects. For small objects, we could probably swap out read_object_file() for using packed_object_info() with a "object_info.contentp" to get the contents. But we'd still need to deal with streaming for larger objects. A better path forward here is to teach the initial oid_object_info_extended() / packed_object_info() calls to retrieve the contents of smaller objects while they are already being accessed. That would save the extra lookup entirely. But it's a non-trivial feature to add to the object_info code, so I left it for now. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/cat-file.c | 50 +++++++++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 14 deletions(-) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index b533935d5c..86fc03242b 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -355,18 +355,34 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d } } +/* + * If "pack" is non-NULL, then "offset" is the byte offset within the pack from + * which the object may be accessed (though note that we may also rely on + * data->oid, too). If "pack" is NULL, then offset is ignored. + */ static void batch_object_write(const char *obj_name, struct strbuf *scratch, struct batch_options *opt, - struct expand_data *data) + struct expand_data *data, + struct packed_git *pack, + off_t offset) { - if (!data->skip_object_info && - oid_object_info_extended(the_repository, &data->oid, &data->info, - OBJECT_INFO_LOOKUP_REPLACE) < 0) { - printf("%s missing\n", - obj_name ? obj_name : oid_to_hex(&data->oid)); - fflush(stdout); - return; + if (!data->skip_object_info) { + int ret; + + if (pack) + ret = packed_object_info(the_repository, pack, offset, + &data->info); + else + ret = oid_object_info_extended(the_repository, + &data->oid, &data->info, + OBJECT_INFO_LOOKUP_REPLACE); + if (ret < 0) { + printf("%s missing\n", + obj_name ? obj_name : oid_to_hex(&data->oid)); + fflush(stdout); + return; + } } strbuf_reset(scratch); @@ -428,7 +444,7 @@ static void batch_one_object(const char *obj_name, return; } - batch_object_write(obj_name, scratch, opt, data); + batch_object_write(obj_name, scratch, opt, data, NULL, 0); } struct object_cb_data { @@ -442,7 +458,8 @@ static int batch_object_cb(const struct object_id *oid, void *vdata) { struct object_cb_data *data = vdata; oidcpy(&data->expand->oid, oid); - batch_object_write(NULL, data->scratch, data->opt, data->expand); + batch_object_write(NULL, data->scratch, data->opt, data->expand, + NULL, 0); return 0; } @@ -463,7 +480,9 @@ static int collect_packed_object(const struct object_id *oid, return 0; } -static int batch_unordered_object(const struct object_id *oid, void *vdata) +static int batch_unordered_object(const struct object_id *oid, + struct packed_git *pack, off_t offset, + void *vdata) { struct object_cb_data *data = vdata; @@ -471,7 +490,8 @@ static int batch_unordered_object(const struct object_id *oid, void *vdata) return 0; oidcpy(&data->expand->oid, oid); - batch_object_write(NULL, data->scratch, data->opt, data->expand); + batch_object_write(NULL, data->scratch, data->opt, data->expand, + pack, offset); return 0; } @@ -479,7 +499,7 @@ static int batch_unordered_loose(const struct object_id *oid, const char *path, void *data) { - return batch_unordered_object(oid, data); + return batch_unordered_object(oid, NULL, 0, data); } static int batch_unordered_packed(const struct object_id *oid, @@ -487,7 +507,9 @@ static int batch_unordered_packed(const struct object_id *oid, uint32_t pos, void *data) { - return batch_unordered_object(oid, data); + return batch_unordered_object(oid, pack, + nth_packed_object_offset(pack, pos), + data); } static int batch_objects(struct batch_options *opt)