| Message ID | e04b970ccb0cad8c0b651ab11f5f52063bd84606.1625155693.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cat-file: reuse ref-filter logic | expand |
On Thu, Jul 01 2021, ZheNing Hu via GitGitGadget wrote: > From: ZheNing Hu <adlternative@gmail.com> > > In order to let cat-file use ref-filter logic, let's do the > following: > > 1. Change the type of member `format` in struct `batch_options` > to `ref_format`, we will pass it to ref-filter later. > 2. Let `batch_objects()` add atoms to format, and use > `verify_ref_format()` to check atoms. > 3. Use `format_ref_array_item()` in `batch_object_write()` to > get the formatted data corresponding to the object. If the > return value of `format_ref_array_item()` is equals to zero, > use `batch_write()` to print object data; else if the return > value is less than zero, use `die()` to print the error message > and exit; else if return value is greater than zero, only print > the error message, but don't exit. > 4. Use free_ref_array_item_value() to free ref_array_item's > value. > > Most of the atoms in `for-each-ref --format` are now supported, > such as `%(tree)`, `%(parent)`, `%(author)`, `%(tagger)`, `%(if)`, > `%(then)`, `%(else)`, `%(end)`. But these atoms will be rejected: > `%(refname)`, `%(symref)`, `%(upstream)`, `%(push)`, `%(worktreepath)`, > `%(flag)`, `%(HEAD)`, because these atoms are unique to those objects > that pointed to by a ref, "for-each-ref"'s family can naturally use > these atoms, but not all objects are pointed to be a ref, so "cat-file" > will not be able to use them. > > The performance for `git cat-file --batch-all-objects > --batch-check` on the Git repository itself with performance > testing tool `hyperfine` changes from 669.4 ms ± 31.1 ms to > 1.134 s ± 0.063 s. > > The performance for `git cat-file --batch-all-objects --batch >>/dev/null` on the Git repository itself with performance testing > tool `time` change from "27.37s user 0.29s system 98% cpu 28.089 > total" to "33.69s user 1.54s system 87% cpu 40.258 total". This new feature is really nice, but that's a really bad performance regression. A lot of software in the wild relies on "cat-file --batch" to be *the* performant interface to git for mass-extrction of object data. That's in increase of ~70% and ~20%, respectively. Have you dug into (e.g. with a profiler) where we're now spending all this time?
On Fri, Jul 2, 2021 at 3:39 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > On Thu, Jul 01 2021, ZheNing Hu via GitGitGadget wrote: > > The performance for `git cat-file --batch-all-objects > > --batch-check` on the Git repository itself with performance > > testing tool `hyperfine` changes from 669.4 ms ± 31.1 ms to > > 1.134 s ± 0.063 s. > > > > The performance for `git cat-file --batch-all-objects --batch > >>/dev/null` on the Git repository itself with performance testing > > tool `time` change from "27.37s user 0.29s system 98% cpu 28.089 > > total" to "33.69s user 1.54s system 87% cpu 40.258 total". > > This new feature is really nice, but that's a really bad performance > regression. A lot of software in the wild relies on "cat-file --batch" > to be *the* performant interface to git for mass-extrction of object > data. > > That's in increase of ~70% and ~20%, respectively. Have you dug into > (e.g. with a profiler) where we're now spending all this time? Yeah, I think it's better to discuss this performance issue as part of the discussion of this series, instead of as part of the discussion of the blog posts.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2021年7月2日周五 下午9:39写道： > > > On Thu, Jul 01 2021, ZheNing Hu via GitGitGadget wrote: > > > From: ZheNing Hu <adlternative@gmail.com> > > > > In order to let cat-file use ref-filter logic, let's do the > > following: > > > > 1. Change the type of member `format` in struct `batch_options` > > to `ref_format`, we will pass it to ref-filter later. > > 2. Let `batch_objects()` add atoms to format, and use > > `verify_ref_format()` to check atoms. > > 3. Use `format_ref_array_item()` in `batch_object_write()` to > > get the formatted data corresponding to the object. If the > > return value of `format_ref_array_item()` is equals to zero, > > use `batch_write()` to print object data; else if the return > > value is less than zero, use `die()` to print the error message > > and exit; else if return value is greater than zero, only print > > the error message, but don't exit. > > 4. Use free_ref_array_item_value() to free ref_array_item's > > value. > > > > Most of the atoms in `for-each-ref --format` are now supported, > > such as `%(tree)`, `%(parent)`, `%(author)`, `%(tagger)`, `%(if)`, > > `%(then)`, `%(else)`, `%(end)`. But these atoms will be rejected: > > `%(refname)`, `%(symref)`, `%(upstream)`, `%(push)`, `%(worktreepath)`, > > `%(flag)`, `%(HEAD)`, because these atoms are unique to those objects > > that pointed to by a ref, "for-each-ref"'s family can naturally use > > these atoms, but not all objects are pointed to be a ref, so "cat-file" > > will not be able to use them. > > > > The performance for `git cat-file --batch-all-objects > > --batch-check` on the Git repository itself with performance > > testing tool `hyperfine` changes from 669.4 ms ± 31.1 ms to > > 1.134 s ± 0.063 s. > > > > The performance for `git cat-file --batch-all-objects --batch > >>/dev/null` on the Git repository itself with performance testing > > tool `time` change from "27.37s user 0.29s system 98% cpu 28.089 > > total" to "33.69s user 1.54s system 87% cpu 40.258 total". > > This new feature is really nice, but that's a really bad performance > regression. A lot of software in the wild relies on "cat-file --batch" > to be *the* performant interface to git for mass-extrction of object > data. > Thanks, this performance is indeed worrying. > That's in increase of ~70% and ~20%, respectively. Have you dug into > (e.g. with a profiler) where we're now spending all this time? See this two attachment about performance flame graph, oid_object_info_extended() in get_object() is the key to performance limitations. -- ZheNing Hu
On Sat, Jul 03 2021, ZheNing Hu wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2021年7月2日周五 下午9:39写道： >> >> >> On Thu, Jul 01 2021, ZheNing Hu via GitGitGadget wrote: >> >> > From: ZheNing Hu <adlternative@gmail.com> >> > >> > In order to let cat-file use ref-filter logic, let's do the >> > following: >> > >> > 1. Change the type of member `format` in struct `batch_options` >> > to `ref_format`, we will pass it to ref-filter later. >> > 2. Let `batch_objects()` add atoms to format, and use >> > `verify_ref_format()` to check atoms. >> > 3. Use `format_ref_array_item()` in `batch_object_write()` to >> > get the formatted data corresponding to the object. If the >> > return value of `format_ref_array_item()` is equals to zero, >> > use `batch_write()` to print object data; else if the return >> > value is less than zero, use `die()` to print the error message >> > and exit; else if return value is greater than zero, only print >> > the error message, but don't exit. >> > 4. Use free_ref_array_item_value() to free ref_array_item's >> > value. >> > >> > Most of the atoms in `for-each-ref --format` are now supported, >> > such as `%(tree)`, `%(parent)`, `%(author)`, `%(tagger)`, `%(if)`, >> > `%(then)`, `%(else)`, `%(end)`. But these atoms will be rejected: >> > `%(refname)`, `%(symref)`, `%(upstream)`, `%(push)`, `%(worktreepath)`, >> > `%(flag)`, `%(HEAD)`, because these atoms are unique to those objects >> > that pointed to by a ref, "for-each-ref"'s family can naturally use >> > these atoms, but not all objects are pointed to be a ref, so "cat-file" >> > will not be able to use them. >> > >> > The performance for `git cat-file --batch-all-objects >> > --batch-check` on the Git repository itself with performance >> > testing tool `hyperfine` changes from 669.4 ms ± 31.1 ms to >> > 1.134 s ± 0.063 s. >> > >> > The performance for `git cat-file --batch-all-objects --batch >> >>/dev/null` on the Git repository itself with performance testing >> > tool `time` change from "27.37s user 0.29s system 98% cpu 28.089 >> > total" to "33.69s user 1.54s system 87% cpu 40.258 total". >> >> This new feature is really nice, but that's a really bad performance >> regression. A lot of software in the wild relies on "cat-file --batch" >> to be *the* performant interface to git for mass-extrction of object >> data. >> > > Thanks, this performance is indeed worrying. > >> That's in increase of ~70% and ~20%, respectively. Have you dug into >> (e.g. with a profiler) where we're now spending all this time? > > See this two attachment about performance flame graph, > oid_object_info_extended() in get_object() is the key to performance > limitations. Most of the problem, although this may not entirely fix the performance regression, is that you're either looking up everything twice now, or taking a much more expensive path. I think using gprof is probably much more handy here. See [1. I did a `git rev-list --all >rla` and ran that piped into 'git cat-file --batch' with/without your pathces. Results: $ gprof ./git-master ./gmon-master.out | head -n 10 Flat profile: Each sample counts as 0.01 seconds. % cumulative self self total time seconds seconds calls ms/call ms/call name 14.29 0.02 0.02 475186 0.00 0.00 nth_packed_object_offset 14.29 0.04 0.02 237835 0.00 0.00 hash_to_hex_algop_r 7.14 0.05 0.01 5220425 0.00 0.00 hashcmp_algop 7.14 0.06 0.01 4757120 0.00 0.00 hex2chr 7.14 0.07 0.01 1732023 0.00 0.00 find_entry_ptr And: $ gprof ./git-new ./gmon-new.out |head -n 10 Flat profile: Each sample counts as 0.01 seconds. % cumulative self self total time seconds seconds calls ms/call ms/call name 7.32 0.06 0.06 764570 0.00 0.00 lookup_object 7.32 0.12 0.06 237835 0.00 0.00 parse_commit_date 4.88 0.16 0.04 712779 0.00 0.00 nth_packed_object_offset 3.66 0.19 0.03 964574 0.00 0.00 bsearch_hash 3.66 0.22 0.03 237835 0.00 0.00 grab_sub_body_contents If you e.g. make lookup_object() simply die when it's called you'll see that before we don't call it at all, after your patch it's our #1 function. Before when we have the simplest case of writing out an object this is our callstack: (gdb) bt #0 batch_write (opt=0x7fffffffde50, data=0x555555ab9470, len=52) at builtin/cat-file.c:298 #1 0x000055555558b160 in batch_object_write (obj_name=0x55555597cef0 "504fe6b39f7747be6427f28d9ca97decf5e6cecf", scratch=0x7fffffffd8c0, opt=0x7fffffffde50, data=0x7fffffffd7f0) at builtin/cat-file.c:375 #2 0x000055555558b36e in batch_one_object (obj_name=0x55555597cef0 "504fe6b39f7747be6427f28d9ca97decf5e6cecf", scratch=0x7fffffffd8c0, opt=0x7fffffffde50, data=0x7fffffffd7f0) at builtin/cat-file.c:431 #3 0x000055555558b8ed in batch_objects (opt=0x7fffffffde50) at builtin/cat-file.c:588 #4 0x000055555558c0d3 in cmd_cat_file (argc=0, argv=0x7fffffffe1e0, prefix=0x0) at builtin/cat-file.c:716 #5 0x0000555555573adb in run_builtin (p=0x555555941870 <commands+240>, argc=2, argv=0x7fffffffe1e0) at git.c:461 #6 0x0000555555573f00 in handle_builtin (argc=2, argv=0x7fffffffe1e0) at git.c:714 #7 0x0000555555574182 in run_argv (argcp=0x7fffffffe08c, argv=0x7fffffffe080) at git.c:781 #8 0x000055555557460f in cmd_main (argc=2, argv=0x7fffffffe1e0) at git.c:912 #9 0x000055555565b508 in main (argc=3, argv=0x7fffffffe1d8) at common-main.c:52 After (well, here we're not even to writing it, just looking it up), the BUG() is my addition: (gdb) bt #0 BUG_fl (file=0x5555558ade71 "object.c", line=91, fmt=0x5555558ade6e "yo") at usage.c:290 #1 0x00005555557441ca in lookup_object (r=0x5555559755c0 <the_repo>, oid=0x555555975160 <oi>) at object.c:91 #2 0x000055555569dfc8 in lookup_commit (r=0x5555559755c0 <the_repo>, oid=0x555555975160 <oi>) at commit.c:62 #3 0x00005555557445f5 in parse_object_buffer (r=0x5555559755c0 <the_repo>, oid=0x555555975160 <oi>, type=OBJ_COMMIT, size=342, buffer=0x555555ab48e0, eaten_p=0x7fffffffd36c) at object.c:215 #4 0x0000555555785094 in get_object (ref=0x7fffffffd6b0, deref=0, obj=0x7fffffffd520, oi=0x555555975160 <oi>, err=0x7fffffffd860) at ref-filter.c:1803 #5 0x0000555555785c99 in populate_value (ref=0x7fffffffd6b0, err=0x7fffffffd860) at ref-filter.c:2030 #6 0x0000555555785d7b in get_ref_atom_value (ref=0x7fffffffd6b0, atom=0, v=0x7fffffffd628, err=0x7fffffffd860) at ref-filter.c:2064 #7 0x000055555578742f in format_ref_array_item (info=0x7fffffffd6b0, format=0x7fffffffde30, final_buf=0x7fffffffd880, error_buf=0x7fffffffd860) at ref-filter.c:2659 #8 0x000055555558ab1c in batch_object_write (obj_name=0x55555597e3f0 "504fe6b39f7747be6427f28d9ca97decf5e6cecf", scratch=0x7fffffffd880, err=0x7fffffffd860, opt=0x7fffffffde10, data=0x7fffffffd800) at builtin/cat-file.c:225 #9 0x000055555558ade5 in batch_one_object (obj_name=0x55555597e3f0 "504fe6b39f7747be6427f28d9ca97decf5e6cecf", scratch=0x7fffffffd880, err=0x7fffffffd860, opt=0x7fffffffde10, data=0x7fffffffd800) at builtin/cat-file.c:298 #10 0x000055555558b394 in batch_objects (batch=0x7fffffffde10, options=0x7fffffffd900) at builtin/cat-file.c:458 #11 0x000055555558bbd5 in cmd_cat_file (argc=0, argv=0x7fffffffe1d0, prefix=0x0) at builtin/cat-file.c:585 #12 0x0000555555573adb in run_builtin (p=0x555555942850 <commands+240>, argc=2, argv=0x7fffffffe1d0) at git.c:461 #13 0x0000555555573f00 in handle_builtin (argc=2, argv=0x7fffffffe1d0) at git.c:714 #14 0x0000555555574182 in run_argv (argcp=0x7fffffffe07c, argv=0x7fffffffe070) at git.c:781 #15 0x000055555557460f in cmd_main (argc=2, argv=0x7fffffffe1d0) at git.c:912 #16 0x000055555565afc1 in main (argc=3, argv=0x7fffffffe1c8) at common-main.c:52 I.e. before in batch_object_write() we could use a cheap path of doing oid_object_info_extended() and directly emitting the content. With your version we're all the way down to parse_object_buffer(). Meaning that we're going to be creating a "struct commit" or whatever if we're looking at a commit, just to print out the raw contents. I think the best next step here is to add a t/perf/t1006-cat-file.sh test to stress these various cases, i.e. a plain --batch without a format, with format, with --batch-all-objects etc. Try to then run that on each of your commits against the preceding one and see if/when you have regressions. Aside from any double-lookups etc, the problem is also that you're trying to handle a really general case (e.g. with textconv) in a codepath that needs to be really fast. If anything we should be inserting some more more optimization shortcuts for common cases into it. E.g. I was able to trivially speed up 'cat-file --batch-check' on "master" by hardcoding a path for our default format (patch at the end of this mail): # passed all 2 test(s) 1..2 Test origin/master HEAD ------------------------------------------------------------------------- 1006.2: cat-file --batch-check 0.60(0.37+0.23) 0.35(0.33+0.02) -41.7% Anything that needs to handle general format patching is going to be slower. I think /some/ performance regression if we're using something that's not just the current light strbuf_expand() probably can't be avoided, but we could/should try to make up the difference at least for the common case of --batch or --batch-check without --textconv and perhaps hardcode (and document that it's faster) a path for the default formats). 1. https://sourceware.org/binutils/docs/gprof/Output.html diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 5ebf13359e8..775b7dd1b01 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -360,6 +360,11 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt, struct expand_data *data) { + int default_format = !strcmp(opt->format, "%(objectname) %(objecttype) %(objectsize)"); + struct strbuf type_name = STRBUF_INIT; + if (default_format) + data->info.type_name = &type_name; + if (!data->skip_object_info && oid_object_info_extended(the_repository, &data->oid, &data->info, OBJECT_INFO_LOOKUP_REPLACE) < 0) { @@ -369,14 +374,20 @@ static void batch_object_write(const char *obj_name, return; } - strbuf_reset(scratch); - strbuf_expand(scratch, opt->format, expand_format, data); - strbuf_addch(scratch, '\n'); - batch_write(opt, scratch->buf, scratch->len); - - if (opt->print_contents) { - print_object_or_die(opt, data); - batch_write(opt, "\n", 1); + if (default_format && !opt->print_contents) { + fprintf(stdout, "%s %s %"PRIuMAX"\n", oid_to_hex(&data->oid), + data->info.type_name->buf, + (uintmax_t)*data->info.sizep); + } else { + strbuf_reset(scratch); + strbuf_expand(scratch, opt->format, expand_format, data); + strbuf_addch(scratch, '\n'); + batch_write(opt, scratch->buf, scratch->len); + + if (opt->print_contents) { + print_object_or_die(opt, data); + batch_write(opt, "\n", 1); + } } } diff --git a/t/perf/p1006-cat-file.sh b/t/perf/p1006-cat-file.sh new file mode 100755 index 00000000000..a295d334715 --- /dev/null +++ b/t/perf/p1006-cat-file.sh @@ -0,0 +1,16 @@ +#!/bin/sh + +test_description='Basic sort performance tests' +. ./perf-lib.sh + +test_perf_default_repo + +test_expect_success 'setup' ' + git rev-list --all >rla +' + +test_perf 'cat-file --batch-check' ' + git cat-file --batch-check <rla +' + +test_done
ZheNing Hu <adlternative@gmail.com> 于2021年7月3日周六 下午7:45写道： > > See this two attachment about performance flame graph, > oid_object_info_extended() in get_object() is the key to performance > limitations. > Well, from the current point of view, The time of oid_object_info_extended() before and after reconstruction is similar. But after refactoring, there are a lot of additional detections in ref-filter. E.g. append_literal() 1.68% append_atom 1.63% ... -- ZheNing Hu
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2021年7月3日周六 下午10:18写道： > Most of the problem, although this may not entirely fix the performance > regression, is that you're either looking up everything twice now, or > taking a much more expensive path. > Yeah, In get_object(), oid_object_info_extended() is called twice when we want print object's contents. The original intention is to reduce the call of oid_object_info_extended() once when using --textconv. Should we make --textconv and --filters have the same logic? In this way, without using --textconv and --filters, we can call oid_object_info_extended() only once. > I think using gprof is probably much more handy here. See [1. I did a > `git rev-list --all >rla` and ran that piped into 'git cat-file --batch' > with/without your pathces. Results: > > $ gprof ./git-master ./gmon-master.out | head -n 10 > Flat profile: > > Each sample counts as 0.01 seconds. > % cumulative self self total > time seconds seconds calls ms/call ms/call name > 14.29 0.02 0.02 475186 0.00 0.00 nth_packed_object_offset > 14.29 0.04 0.02 237835 0.00 0.00 hash_to_hex_algop_r > 7.14 0.05 0.01 5220425 0.00 0.00 hashcmp_algop > 7.14 0.06 0.01 4757120 0.00 0.00 hex2chr > 7.14 0.07 0.01 1732023 0.00 0.00 find_entry_ptr > > And: > > $ gprof ./git-new ./gmon-new.out |head -n 10 > Flat profile: > > Each sample counts as 0.01 seconds. > % cumulative self self total > time seconds seconds calls ms/call ms/call name > 7.32 0.06 0.06 764570 0.00 0.00 lookup_object > 7.32 0.12 0.06 237835 0.00 0.00 parse_commit_date > 4.88 0.16 0.04 712779 0.00 0.00 nth_packed_object_offset > 3.66 0.19 0.03 964574 0.00 0.00 bsearch_hash > 3.66 0.22 0.03 237835 0.00 0.00 grab_sub_body_contents > It seems that lookup_object() took a lot of time with the patch of my version . > If you e.g. make lookup_object() simply die when it's called you'll see > that before we don't call it at all, after your patch it's our #1 > function. > > Before when we have the simplest case of writing out an object this is > our callstack: > > (gdb) bt > #0 batch_write (opt=0x7fffffffde50, data=0x555555ab9470, len=52) at builtin/cat-file.c:298 > #1 0x000055555558b160 in batch_object_write (obj_name=0x55555597cef0 "504fe6b39f7747be6427f28d9ca97decf5e6cecf", scratch=0x7fffffffd8c0, > opt=0x7fffffffde50, data=0x7fffffffd7f0) at builtin/cat-file.c:375 > #2 0x000055555558b36e in batch_one_object (obj_name=0x55555597cef0 "504fe6b39f7747be6427f28d9ca97decf5e6cecf", scratch=0x7fffffffd8c0, opt=0x7fffffffde50, > data=0x7fffffffd7f0) at builtin/cat-file.c:431 > #3 0x000055555558b8ed in batch_objects (opt=0x7fffffffde50) at builtin/cat-file.c:588 > #4 0x000055555558c0d3 in cmd_cat_file (argc=0, argv=0x7fffffffe1e0, prefix=0x0) at builtin/cat-file.c:716 > #5 0x0000555555573adb in run_builtin (p=0x555555941870 <commands+240>, argc=2, argv=0x7fffffffe1e0) at git.c:461 > #6 0x0000555555573f00 in handle_builtin (argc=2, argv=0x7fffffffe1e0) at git.c:714 > #7 0x0000555555574182 in run_argv (argcp=0x7fffffffe08c, argv=0x7fffffffe080) at git.c:781 > #8 0x000055555557460f in cmd_main (argc=2, argv=0x7fffffffe1e0) at git.c:912 > #9 0x000055555565b508 in main (argc=3, argv=0x7fffffffe1d8) at common-main.c:52 > > After (well, here we're not even to writing it, just looking it up), the > BUG() is my addition: > > (gdb) bt > #0 BUG_fl (file=0x5555558ade71 "object.c", line=91, fmt=0x5555558ade6e "yo") at usage.c:290 > #1 0x00005555557441ca in lookup_object (r=0x5555559755c0 <the_repo>, oid=0x555555975160 <oi>) at object.c:91 > #2 0x000055555569dfc8 in lookup_commit (r=0x5555559755c0 <the_repo>, oid=0x555555975160 <oi>) at commit.c:62 > #3 0x00005555557445f5 in parse_object_buffer (r=0x5555559755c0 <the_repo>, oid=0x555555975160 <oi>, type=OBJ_COMMIT, size=342, buffer=0x555555ab48e0, > eaten_p=0x7fffffffd36c) at object.c:215 > #4 0x0000555555785094 in get_object (ref=0x7fffffffd6b0, deref=0, obj=0x7fffffffd520, oi=0x555555975160 <oi>, err=0x7fffffffd860) at ref-filter.c:1803 > #5 0x0000555555785c99 in populate_value (ref=0x7fffffffd6b0, err=0x7fffffffd860) at ref-filter.c:2030 > #6 0x0000555555785d7b in get_ref_atom_value (ref=0x7fffffffd6b0, atom=0, v=0x7fffffffd628, err=0x7fffffffd860) at ref-filter.c:2064 > #7 0x000055555578742f in format_ref_array_item (info=0x7fffffffd6b0, format=0x7fffffffde30, final_buf=0x7fffffffd880, error_buf=0x7fffffffd860) > at ref-filter.c:2659 > #8 0x000055555558ab1c in batch_object_write (obj_name=0x55555597e3f0 "504fe6b39f7747be6427f28d9ca97decf5e6cecf", scratch=0x7fffffffd880, > err=0x7fffffffd860, opt=0x7fffffffde10, data=0x7fffffffd800) at builtin/cat-file.c:225 > #9 0x000055555558ade5 in batch_one_object (obj_name=0x55555597e3f0 "504fe6b39f7747be6427f28d9ca97decf5e6cecf", scratch=0x7fffffffd880, err=0x7fffffffd860, > opt=0x7fffffffde10, data=0x7fffffffd800) at builtin/cat-file.c:298 > #10 0x000055555558b394 in batch_objects (batch=0x7fffffffde10, options=0x7fffffffd900) at builtin/cat-file.c:458 > #11 0x000055555558bbd5 in cmd_cat_file (argc=0, argv=0x7fffffffe1d0, prefix=0x0) at builtin/cat-file.c:585 > #12 0x0000555555573adb in run_builtin (p=0x555555942850 <commands+240>, argc=2, argv=0x7fffffffe1d0) at git.c:461 > #13 0x0000555555573f00 in handle_builtin (argc=2, argv=0x7fffffffe1d0) at git.c:714 > #14 0x0000555555574182 in run_argv (argcp=0x7fffffffe07c, argv=0x7fffffffe070) at git.c:781 > #15 0x000055555557460f in cmd_main (argc=2, argv=0x7fffffffe1d0) at git.c:912 > #16 0x000055555565afc1 in main (argc=3, argv=0x7fffffffe1c8) at common-main.c:52 > It seems that the call stack is very deep after using my version. > I.e. before in batch_object_write() we could use a cheap path of doing > oid_object_info_extended() and directly emitting the content. With your > version we're all the way down to parse_object_buffer(). Meaning that > we're going to be creating a "struct commit" or whatever if we're > looking at a commit, just to print out the raw contents. > I agree that the logic in ref-filter is too complicated in order to be able to print the structured object data. > I think the best next step here is to add a t/perf/t1006-cat-file.sh > test to stress these various cases, i.e. a plain --batch without a > format, with format, with --batch-all-objects etc. Try to then run that > on each of your commits against the preceding one and see if/when you > have regressions. > Make sence. > Aside from any double-lookups etc, the problem is also that you're > trying to handle a really general case (e.g. with textconv) in a Well, "--textconv" is a common situation? Here I may not be sure which scenarios where the upper application calls "git cat-file --batch" are the most common. > codepath that needs to be really fast. If anything we should be > inserting some more more optimization shortcuts for common cases into > it. E.g. I was able to trivially speed up 'cat-file --batch-check' on > "master" by hardcoding a path for our default format (patch at the end > of this mail): > > # passed all 2 test(s) > 1..2 > Test origin/master HEAD > ------------------------------------------------------------------------- > 1006.2: cat-file --batch-check 0.60(0.37+0.23) 0.35(0.33+0.02) -41.7% > > Anything that needs to handle general format patching is going to be > slower. I think /some/ performance regression if we're using something > that's not just the current light strbuf_expand() probably can't be > avoided, but we could/should try to make up the difference at least for > the common case of --batch or --batch-check without --textconv and > perhaps hardcode (and document that it's faster) a path for the default > formats). > Yeah, Like the hardcode in your patch may be a solution to the performance degradation. This will indeed help those upper-level applications that use the common case. > 1. https://sourceware.org/binutils/docs/gprof/Output.html > > diff --git a/builtin/cat-file.c b/builtin/cat-file.c > index 5ebf13359e8..775b7dd1b01 100644 > --- a/builtin/cat-file.c > +++ b/builtin/cat-file.c > @@ -360,6 +360,11 @@ static void batch_object_write(const char *obj_name, > struct batch_options *opt, > struct expand_data *data) > { > + int default_format = !strcmp(opt->format, "%(objectname) %(objecttype) %(objectsize)"); > + struct strbuf type_name = STRBUF_INIT; > + if (default_format) > + data->info.type_name = &type_name; > + > if (!data->skip_object_info && > oid_object_info_extended(the_repository, &data->oid, &data->info, > OBJECT_INFO_LOOKUP_REPLACE) < 0) { > @@ -369,14 +374,20 @@ static void batch_object_write(const char *obj_name, > return; > } > > - strbuf_reset(scratch); > - strbuf_expand(scratch, opt->format, expand_format, data); > - strbuf_addch(scratch, '\n'); > - batch_write(opt, scratch->buf, scratch->len); > - > - if (opt->print_contents) { > - print_object_or_die(opt, data); > - batch_write(opt, "\n", 1); > + if (default_format && !opt->print_contents) { > + fprintf(stdout, "%s %s %"PRIuMAX"\n", oid_to_hex(&data->oid), > + data->info.type_name->buf, > + (uintmax_t)*data->info.sizep); > + } else { > + strbuf_reset(scratch); > + strbuf_expand(scratch, opt->format, expand_format, data); > + strbuf_addch(scratch, '\n'); > + batch_write(opt, scratch->buf, scratch->len); > + > + if (opt->print_contents) { > + print_object_or_die(opt, data); > + batch_write(opt, "\n", 1); > + } > } > } > > diff --git a/t/perf/p1006-cat-file.sh b/t/perf/p1006-cat-file.sh > new file mode 100755 > index 00000000000..a295d334715 > --- /dev/null > +++ b/t/perf/p1006-cat-file.sh > @@ -0,0 +1,16 @@ > +#!/bin/sh > + > +test_description='Basic sort performance tests' > +. ./perf-lib.sh > + > +test_perf_default_repo > + > +test_expect_success 'setup' ' > + git rev-list --all >rla > +' > + > +test_perf 'cat-file --batch-check' ' > + git cat-file --batch-check <rla > +' > + > +test_done Thanks. -- ZheNing Hu
On Sun, Jul 04 2021, ZheNing Hu wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2021年7月3日周六 下午10:18写道： > >> Most of the problem, although this may not entirely fix the performance >> regression, is that you're either looking up everything twice now, or >> taking a much more expensive path. >> > > Yeah, In get_object(), oid_object_info_extended() is called twice when we > want print object's contents. The original intention is to reduce the call of > oid_object_info_extended() once when using --textconv. Should we make > --textconv and --filters have the same logic? In this way, without using > --textconv and --filters, we can call oid_object_info_extended() only once. > >> I think using gprof is probably much more handy here. See [1. I did a >> `git rev-list --all >rla` and ran that piped into 'git cat-file --batch' >> with/without your pathces. Results: >> >> $ gprof ./git-master ./gmon-master.out | head -n 10 >> Flat profile: >> >> Each sample counts as 0.01 seconds. >> % cumulative self self total >> time seconds seconds calls ms/call ms/call name >> 14.29 0.02 0.02 475186 0.00 0.00 nth_packed_object_offset >> 14.29 0.04 0.02 237835 0.00 0.00 hash_to_hex_algop_r >> 7.14 0.05 0.01 5220425 0.00 0.00 hashcmp_algop >> 7.14 0.06 0.01 4757120 0.00 0.00 hex2chr >> 7.14 0.07 0.01 1732023 0.00 0.00 find_entry_ptr >> >> And: >> >> $ gprof ./git-new ./gmon-new.out |head -n 10 >> Flat profile: >> >> Each sample counts as 0.01 seconds. >> % cumulative self self total >> time seconds seconds calls ms/call ms/call name >> 7.32 0.06 0.06 764570 0.00 0.00 lookup_object >> 7.32 0.12 0.06 237835 0.00 0.00 parse_commit_date >> 4.88 0.16 0.04 712779 0.00 0.00 nth_packed_object_offset >> 3.66 0.19 0.03 964574 0.00 0.00 bsearch_hash >> 3.66 0.22 0.03 237835 0.00 0.00 grab_sub_body_contents >> > > It seems that lookup_object() took a lot of time with the patch of my version . > >> If you e.g. make lookup_object() simply die when it's called you'll see >> that before we don't call it at all, after your patch it's our #1 >> function. >> >> Before when we have the simplest case of writing out an object this is >> our callstack: >> >> (gdb) bt >> #0 batch_write (opt=0x7fffffffde50, data=0x555555ab9470, len=52) at builtin/cat-file.c:298 >> #1 0x000055555558b160 in batch_object_write (obj_name=0x55555597cef0 "504fe6b39f7747be6427f28d9ca97decf5e6cecf", scratch=0x7fffffffd8c0, >> opt=0x7fffffffde50, data=0x7fffffffd7f0) at builtin/cat-file.c:375 >> #2 0x000055555558b36e in batch_one_object (obj_name=0x55555597cef0 "504fe6b39f7747be6427f28d9ca97decf5e6cecf", scratch=0x7fffffffd8c0, opt=0x7fffffffde50, >> data=0x7fffffffd7f0) at builtin/cat-file.c:431 >> #3 0x000055555558b8ed in batch_objects (opt=0x7fffffffde50) at builtin/cat-file.c:588 >> #4 0x000055555558c0d3 in cmd_cat_file (argc=0, argv=0x7fffffffe1e0, prefix=0x0) at builtin/cat-file.c:716 >> #5 0x0000555555573adb in run_builtin (p=0x555555941870 <commands+240>, argc=2, argv=0x7fffffffe1e0) at git.c:461 >> #6 0x0000555555573f00 in handle_builtin (argc=2, argv=0x7fffffffe1e0) at git.c:714 >> #7 0x0000555555574182 in run_argv (argcp=0x7fffffffe08c, argv=0x7fffffffe080) at git.c:781 >> #8 0x000055555557460f in cmd_main (argc=2, argv=0x7fffffffe1e0) at git.c:912 >> #9 0x000055555565b508 in main (argc=3, argv=0x7fffffffe1d8) at common-main.c:52 >> >> After (well, here we're not even to writing it, just looking it up), the >> BUG() is my addition: >> >> (gdb) bt >> #0 BUG_fl (file=0x5555558ade71 "object.c", line=91, fmt=0x5555558ade6e "yo") at usage.c:290 >> #1 0x00005555557441ca in lookup_object (r=0x5555559755c0 <the_repo>, oid=0x555555975160 <oi>) at object.c:91 >> #2 0x000055555569dfc8 in lookup_commit (r=0x5555559755c0 <the_repo>, oid=0x555555975160 <oi>) at commit.c:62 >> #3 0x00005555557445f5 in parse_object_buffer (r=0x5555559755c0 <the_repo>, oid=0x555555975160 <oi>, type=OBJ_COMMIT, size=342, buffer=0x555555ab48e0, >> eaten_p=0x7fffffffd36c) at object.c:215 >> #4 0x0000555555785094 in get_object (ref=0x7fffffffd6b0, deref=0, obj=0x7fffffffd520, oi=0x555555975160 <oi>, err=0x7fffffffd860) at ref-filter.c:1803 >> #5 0x0000555555785c99 in populate_value (ref=0x7fffffffd6b0, err=0x7fffffffd860) at ref-filter.c:2030 >> #6 0x0000555555785d7b in get_ref_atom_value (ref=0x7fffffffd6b0, atom=0, v=0x7fffffffd628, err=0x7fffffffd860) at ref-filter.c:2064 >> #7 0x000055555578742f in format_ref_array_item (info=0x7fffffffd6b0, format=0x7fffffffde30, final_buf=0x7fffffffd880, error_buf=0x7fffffffd860) >> at ref-filter.c:2659 >> #8 0x000055555558ab1c in batch_object_write (obj_name=0x55555597e3f0 "504fe6b39f7747be6427f28d9ca97decf5e6cecf", scratch=0x7fffffffd880, >> err=0x7fffffffd860, opt=0x7fffffffde10, data=0x7fffffffd800) at builtin/cat-file.c:225 >> #9 0x000055555558ade5 in batch_one_object (obj_name=0x55555597e3f0 "504fe6b39f7747be6427f28d9ca97decf5e6cecf", scratch=0x7fffffffd880, err=0x7fffffffd860, >> opt=0x7fffffffde10, data=0x7fffffffd800) at builtin/cat-file.c:298 >> #10 0x000055555558b394 in batch_objects (batch=0x7fffffffde10, options=0x7fffffffd900) at builtin/cat-file.c:458 >> #11 0x000055555558bbd5 in cmd_cat_file (argc=0, argv=0x7fffffffe1d0, prefix=0x0) at builtin/cat-file.c:585 >> #12 0x0000555555573adb in run_builtin (p=0x555555942850 <commands+240>, argc=2, argv=0x7fffffffe1d0) at git.c:461 >> #13 0x0000555555573f00 in handle_builtin (argc=2, argv=0x7fffffffe1d0) at git.c:714 >> #14 0x0000555555574182 in run_argv (argcp=0x7fffffffe07c, argv=0x7fffffffe070) at git.c:781 >> #15 0x000055555557460f in cmd_main (argc=2, argv=0x7fffffffe1d0) at git.c:912 >> #16 0x000055555565afc1 in main (argc=3, argv=0x7fffffffe1c8) at common-main.c:52 >> > > It seems that the call stack is very deep after using my version. > >> I.e. before in batch_object_write() we could use a cheap path of doing >> oid_object_info_extended() and directly emitting the content. With your >> version we're all the way down to parse_object_buffer(). Meaning that >> we're going to be creating a "struct commit" or whatever if we're >> looking at a commit, just to print out the raw contents. >> > > I agree that the logic in ref-filter is too complicated in order to be > able to print > the structured object data. Well, maybe it isn't. I think the main problem with your performance is just looking things up again from the object store needlessly, and possibly redundantly copying things around. E.g. if you have a buffer with a commit object and you need to xstrdup() that, and xstrdup() the answer etc, it adds up. Maybe if all that's gone the formatting will still slow things down, but I bet it's going to be to a much smaller extent. >> I think the best next step here is to add a t/perf/t1006-cat-file.sh >> test to stress these various cases, i.e. a plain --batch without a >> format, with format, with --batch-all-objects etc. Try to then run that >> on each of your commits against the preceding one and see if/when you >> have regressions. >> > > Make sence. > >> Aside from any double-lookups etc, the problem is also that you're >> trying to handle a really general case (e.g. with textconv) in a > > Well, "--textconv" is a common situation? Here I may not be sure which > scenarios where the upper application calls "git cat-file --batch" are the > most common. I think --textconv is much rarer. I think it's used for API purposes e.g. for people who are doing \n translation, or maybe for the likes of git-lfs (or was that --filters)? I'm not very familiar with it. For what it's worth 'git cat-file --batch' is very performance sensitive in backend use-cases like e.g. gitlab.com's gitaly (which executes all git commands for you on that site). Anything that looks up .. any object pretty much .. is prining that object line to a persistent "cat-file --batch" process. There was a case where some very small (unrelated to cat-file per-se) slowdown in that codepath ended up slowing down overall API requests for some calls by >100%, because it's executed in a tight loop. E.g. if you're showing N commits per page that might be N cat-file --batch calls.
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt index 4eb0421b3fd..ef8ab952b2f 100644 --- a/Documentation/git-cat-file.txt +++ b/Documentation/git-cat-file.txt @@ -226,6 +226,12 @@ newline. The available atoms are: after that first run of whitespace (i.e., the "rest" of the line) are output in place of the `%(rest)` atom. +Note that most of the atoms in `for-each-ref --format` are now supported, +such as `%(tree)`, `%(parent)`, `%(author)`, `%(tagger)`, `%(if)`, +`%(then)`, `%(else)`, `%(end)`. But these atoms will be rejected: +`%(refname)`, `%(symref)`, `%(upstream)`, `%(push)`, `%(worktreepath)`, +`%(flag)`, `%(HEAD)`. See linkgit:git-for-each-ref[1]. + If no format is specified, the default format is `%(objectname) %(objecttype) %(objectsize)`. diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 41d407638d5..5b163551fc6 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -16,6 +16,7 @@ #include "packfile.h" #include "object-store.h" #include "promisor-remote.h" +#include "ref-filter.h" struct batch_options { int enabled; @@ -25,7 +26,7 @@ struct batch_options { int all_objects; int unordered; int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */ - const char *format; + struct ref_format format; }; static const char *force_path; @@ -195,99 +196,10 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, struct expand_data { struct object_id oid; - enum object_type type; - unsigned long size; - off_t disk_size; const char *rest; - struct object_id delta_base_oid; - - /* - * If mark_query is true, we do not expand anything, but rather - * just mark the object_info with items we wish to query. - */ - int mark_query; - - /* - * Whether to split the input on whitespace before feeding it to - * get_sha1; this is decided during the mark_query phase based on - * whether we have a %(rest) token in our format. - */ int split_on_whitespace; - - /* - * After a mark_query run, this object_info is set up to be - * passed to oid_object_info_extended. It will point to the data - * elements above, so you can retrieve the response from there. - */ - struct object_info info; - - /* - * This flag will be true if the requested batch format and options - * don't require us to call oid_object_info, which can then be - * optimized out. - */ - unsigned skip_object_info : 1; }; -static int is_atom(const char *atom, const char *s, int slen) -{ - int alen = strlen(atom); - return alen == slen && !memcmp(atom, s, alen); -} - -static void expand_atom(struct strbuf *sb, const char *atom, int len, - void *vdata) -{ - struct expand_data *data = vdata; - - if (is_atom("objectname", atom, len)) { - if (!data->mark_query) - strbuf_addstr(sb, oid_to_hex(&data->oid)); - } else if (is_atom("objecttype", atom, len)) { - if (data->mark_query) - data->info.typep = &data->type; - else - strbuf_addstr(sb, type_name(data->type)); - } else if (is_atom("objectsize", atom, len)) { - if (data->mark_query) - data->info.sizep = &data->size; - else - strbuf_addf(sb, "%"PRIuMAX , (uintmax_t)data->size); - } else if (is_atom("objectsize:disk", atom, len)) { - if (data->mark_query) - data->info.disk_sizep = &data->disk_size; - else - strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size); - } else if (is_atom("rest", atom, len)) { - if (data->mark_query) - data->split_on_whitespace = 1; - else if (data->rest) - strbuf_addstr(sb, data->rest); - } else if (is_atom("deltabase", atom, len)) { - if (data->mark_query) - data->info.delta_base_oid = &data->delta_base_oid; - else - strbuf_addstr(sb, - oid_to_hex(&data->delta_base_oid)); - } else - die("unknown format element: %.*s", len, atom); -} - -static size_t expand_format(struct strbuf *sb, const char *start, void *data) -{ - const char *end; - - if (*start != '(') - return 0; - end = strchr(start + 1, ')'); - if (!end) - die("format element '%s' does not end in ')'", start); - - expand_atom(sb, start + 1, end - start - 1, data); - - return end - start + 1; -} - static void batch_write(struct batch_options *opt, const void *data, int len) { if (opt->buffer_output) { @@ -297,87 +209,34 @@ static void batch_write(struct batch_options *opt, const void *data, int len) write_or_die(1, data, len); } -static void print_object_or_die(struct batch_options *opt, struct expand_data *data) -{ - const struct object_id *oid = &data->oid; - - assert(data->info.typep); - - if (data->type == OBJ_BLOB) { - if (opt->buffer_output) - fflush(stdout); - if (opt->cmdmode) { - char *contents; - unsigned long size; - - if (!data->rest) - die("missing path for '%s'", oid_to_hex(oid)); - - if (opt->cmdmode == 'w') { - if (filter_object(data->rest, 0100644, oid, - &contents, &size)) - die("could not convert '%s' %s", - oid_to_hex(oid), data->rest); - } else if (opt->cmdmode == 'c') { - enum object_type type; - if (!textconv_object(the_repository, - data->rest, 0100644, oid, - 1, &contents, &size)) - contents = read_object_file(oid, - &type, - &size); - if (!contents) - die("could not convert '%s' %s", - oid_to_hex(oid), data->rest); - } else - BUG("invalid cmdmode: %c", opt->cmdmode); - batch_write(opt, contents, size); - free(contents); - } else { - stream_blob(oid); - } - } - else { - enum object_type type; - unsigned long size; - void *contents; - - contents = read_object_file(oid, &type, &size); - if (!contents) - die("object %s disappeared", oid_to_hex(oid)); - if (type != data->type) - die("object %s changed type!?", oid_to_hex(oid)); - if (data->info.sizep && size != data->size) - die("object %s changed size!?", oid_to_hex(oid)); - - batch_write(opt, contents, size); - free(contents); - } -} static void batch_object_write(const char *obj_name, struct strbuf *scratch, struct batch_options *opt, struct expand_data *data) { - 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; - } + int ret; + struct strbuf err = STRBUF_INIT; + struct ref_array_item item = { data->oid, data->rest }; strbuf_reset(scratch); - strbuf_expand(scratch, opt->format, expand_format, data); - strbuf_addch(scratch, '\n'); - batch_write(opt, scratch->buf, scratch->len); - if (opt->print_contents) { - print_object_or_die(opt, data); - batch_write(opt, "\n", 1); + ret = format_ref_array_item(&item, &opt->format, scratch, &err); + if (ret < 0) + die("%s\n", err.buf); + if (ret) { + /* ret > 0 means when the object corresponding to oid + * cannot be found in format_ref_array_item(), we only print + * the error message. + */ + printf("%s\n", err.buf); + fflush(stdout); + } else { + strbuf_addch(scratch, '\n'); + batch_write(opt, scratch->buf, scratch->len); } + free_ref_array_item_value(&item); + strbuf_release(&err); } static void batch_one_object(const char *obj_name, @@ -495,43 +354,37 @@ static int batch_unordered_packed(const struct object_id *oid, return batch_unordered_object(oid, data); } -static int batch_objects(struct batch_options *batch) +static const char * const cat_file_usage[] = { + N_("git cat-file (-t [--allow-unknown-type] | -s [--allow-unknown-type] | -e | -p | <type> | --textconv | --filters) [--path=<path>] <object>"), + N_("git cat-file (--batch[=<format>] | --batch-check[=<format>]) [--follow-symlinks] [--textconv | --filters]"), + NULL +}; + +static int batch_objects(struct batch_options *batch, const struct option *options) { struct strbuf input = STRBUF_INIT; struct strbuf output = STRBUF_INIT; + struct strbuf format = STRBUF_INIT; struct expand_data data; int save_warning; int retval = 0; - if (!batch->format) - batch->format = "%(objectname) %(objecttype) %(objectsize)"; - - /* - * Expand once with our special mark_query flag, which will prime the - * object_info to be handed to oid_object_info_extended for each - * object. - */ memset(&data, 0, sizeof(data)); - data.mark_query = 1; - strbuf_expand(&output, batch->format, expand_format, &data); - data.mark_query = 0; - strbuf_release(&output); - if (batch->cmdmode) - data.split_on_whitespace = 1; - - /* - * If we are printing out the object, then always fill in the type, - * since we will want to decide whether or not to stream. - */ + if (batch->format.format) + strbuf_addstr(&format, batch->format.format); + else + strbuf_addstr(&format, "%(objectname) %(objecttype) %(objectsize)"); if (batch->print_contents) - data.info.typep = &data.type; + strbuf_addstr(&format, "\n%(raw)"); + batch->format.format = format.buf; + if (verify_ref_format(&batch->format)) + usage_with_options(cat_file_usage, options); + + if (batch->cmdmode || batch->format.use_rest) + data.split_on_whitespace = 1; if (batch->all_objects) { struct object_cb_data cb; - struct object_info empty = OBJECT_INFO_INIT; - - if (!memcmp(&data.info, &empty, sizeof(empty))) - data.skip_object_info = 1; if (has_promisor_remote()) warning("This repository uses promisor remotes. Some objects may not be loaded."); @@ -561,6 +414,7 @@ static int batch_objects(struct batch_options *batch) oid_array_clear(&sa); } + strbuf_release(&format); strbuf_release(&output); return 0; } @@ -593,18 +447,13 @@ static int batch_objects(struct batch_options *batch) batch_one_object(input.buf, &output, batch, &data); } + strbuf_release(&format); strbuf_release(&input); strbuf_release(&output); warn_on_object_refname_ambiguity = save_warning; return retval; } -static const char * const cat_file_usage[] = { - N_("git cat-file (-t [--allow-unknown-type] | -s [--allow-unknown-type] | -e | -p | <type> | --textconv | --filters) [--path=<path>] <object>"), - N_("git cat-file (--batch[=<format>] | --batch-check[=<format>]) [--follow-symlinks] [--textconv | --filters]"), - NULL -}; - static int git_cat_file_config(const char *var, const char *value, void *cb) { if (userdiff_config(var, value) < 0) @@ -627,7 +476,7 @@ static int batch_option_callback(const struct option *opt, bo->enabled = 1; bo->print_contents = !strcmp(opt->long_name, "batch"); - bo->format = arg; + bo->format.format = arg; return 0; } @@ -636,7 +485,9 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix) { int opt = 0; const char *exp_type = NULL, *obj_name = NULL; - struct batch_options batch = {0}; + struct batch_options batch = { + .format = REF_FORMAT_INIT + }; int unknown_type = 0; const struct option options[] = { @@ -675,6 +526,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix) git_config(git_cat_file_config, NULL); batch.buffer_output = -1; + batch.format.cat_file_mode = 1; argc = parse_options(argc, argv, prefix, options, cat_file_usage, 0); if (opt) { @@ -718,7 +570,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix) batch.buffer_output = batch.all_objects; if (batch.enabled) - return batch_objects(&batch); + return batch_objects(&batch, options); if (unknown_type && opt != 't' && opt != 's') die("git cat-file --allow-unknown-type: use with -s or -t"); diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh index 18b3779ccb6..7452404f24a 100755 --- a/t/t1006-cat-file.sh +++ b/t/t1006-cat-file.sh @@ -607,5 +607,256 @@ test_expect_success 'cat-file --batch="batman" with --batch-all-objects will wor git -C all-two cat-file --batch-all-objects --batch="batman" >actual && cmp expect actual ' +. "$TEST_DIRECTORY"/lib-gpg.sh +. "$TEST_DIRECTORY"/lib-terminal.sh + +test_expect_success 'cat-file --batch|--batch-check setup' ' + echo 1>blob1 && + printf "a\0b\0\c" >blob2 && + git add blob1 blob2 && + git commit -m "Commit Message" && + git branch -M main && + git tag -a -m "v0.0.0" testtag && + git update-ref refs/myblobs/blob1 HEAD:blob1 && + git update-ref refs/myblobs/blob2 HEAD:blob2 && + git update-ref refs/mytrees/tree1 HEAD^{tree} +' + +batch_test_atom() { + if test "$3" = "fail" + then + test_expect_${4:-success} $PREREQ "basic atom: $1 $2 must fail" " + test_must_fail git cat-file --batch-check='$2' >bad <<-EOF + $1 + EOF + " + else + test_expect_${4:-success} $PREREQ "basic atom: $1 $2" " + git for-each-ref --format='$2' $1 >expected && + git cat-file --batch-check='$2' >actual <<-EOF && + $1 + EOF + sanitize_pgp <actual >actual.clean && + cmp expected actual.clean + " + fi +} + +batch_test_atom refs/heads/main '%(refname)' fail +batch_test_atom refs/heads/main '%(refname:)' fail +batch_test_atom refs/heads/main '%(refname:short)' fail +batch_test_atom refs/heads/main '%(refname:lstrip=1)' fail +batch_test_atom refs/heads/main '%(refname:lstrip=2)' fail +batch_test_atom refs/heads/main '%(refname:lstrip=-1)' fail +batch_test_atom refs/heads/main '%(refname:lstrip=-2)' fail +batch_test_atom refs/heads/main '%(refname:rstrip=1)' fail +batch_test_atom refs/heads/main '%(refname:rstrip=2)' fail +batch_test_atom refs/heads/main '%(refname:rstrip=-1)' fail +batch_test_atom refs/heads/main '%(refname:rstrip=-2)' fail +batch_test_atom refs/heads/main '%(refname:strip=1)' fail +batch_test_atom refs/heads/main '%(refname:strip=2)' fail +batch_test_atom refs/heads/main '%(refname:strip=-1)' fail +batch_test_atom refs/heads/main '%(refname:strip=-2)' fail +batch_test_atom refs/heads/main '%(upstream)' fail +batch_test_atom refs/heads/main '%(upstream:short)' fail +batch_test_atom refs/heads/main '%(upstream:lstrip=2)' fail +batch_test_atom refs/heads/main '%(upstream:lstrip=-2)' fail +batch_test_atom refs/heads/main '%(upstream:rstrip=2)' fail +batch_test_atom refs/heads/main '%(upstream:rstrip=-2)' fail +batch_test_atom refs/heads/main '%(upstream:strip=2)' fail +batch_test_atom refs/heads/main '%(upstream:strip=-2)' fail +batch_test_atom refs/heads/main '%(push)' fail +batch_test_atom refs/heads/main '%(push:short)' fail +batch_test_atom refs/heads/main '%(push:lstrip=1)' fail +batch_test_atom refs/heads/main '%(push:lstrip=-1)' fail +batch_test_atom refs/heads/main '%(push:rstrip=1)' fail +batch_test_atom refs/heads/main '%(push:rstrip=-1)' fail +batch_test_atom refs/heads/main '%(push:strip=1)' fail +batch_test_atom refs/heads/main '%(push:strip=-1)' fail +batch_test_atom refs/heads/main '%(objecttype)' +batch_test_atom refs/heads/main '%(objectsize)' +batch_test_atom refs/heads/main '%(objectsize:disk)' +batch_test_atom refs/heads/main '%(deltabase)' +batch_test_atom refs/heads/main '%(objectname)' +batch_test_atom refs/heads/main '%(objectname:short)' +batch_test_atom refs/heads/main '%(objectname:short=1)' +batch_test_atom refs/heads/main '%(objectname:short=10)' +batch_test_atom refs/heads/main '%(tree)' +batch_test_atom refs/heads/main '%(tree:short)' +batch_test_atom refs/heads/main '%(tree:short=1)' +batch_test_atom refs/heads/main '%(tree:short=10)' +batch_test_atom refs/heads/main '%(parent)' +batch_test_atom refs/heads/main '%(parent:short)' +batch_test_atom refs/heads/main '%(parent:short=1)' +batch_test_atom refs/heads/main '%(parent:short=10)' +batch_test_atom refs/heads/main '%(numparent)' +batch_test_atom refs/heads/main '%(object)' +batch_test_atom refs/heads/main '%(type)' +batch_test_atom refs/heads/main '%(raw)' +batch_test_atom refs/heads/main '%(*objectname)' +batch_test_atom refs/heads/main '%(*objecttype)' +batch_test_atom refs/heads/main '%(author)' +batch_test_atom refs/heads/main '%(authorname)' +batch_test_atom refs/heads/main '%(authoremail)' +batch_test_atom refs/heads/main '%(authoremail:trim)' +batch_test_atom refs/heads/main '%(authoremail:localpart)' +batch_test_atom refs/heads/main '%(authordate)' +batch_test_atom refs/heads/main '%(committer)' +batch_test_atom refs/heads/main '%(committername)' +batch_test_atom refs/heads/main '%(committeremail)' +batch_test_atom refs/heads/main '%(committeremail:trim)' +batch_test_atom refs/heads/main '%(committeremail:localpart)' +batch_test_atom refs/heads/main '%(committerdate)' +batch_test_atom refs/heads/main '%(tag)' +batch_test_atom refs/heads/main '%(tagger)' +batch_test_atom refs/heads/main '%(taggername)' +batch_test_atom refs/heads/main '%(taggeremail)' +batch_test_atom refs/heads/main '%(taggeremail:trim)' +batch_test_atom refs/heads/main '%(taggeremail:localpart)' +batch_test_atom refs/heads/main '%(taggerdate)' +batch_test_atom refs/heads/main '%(creator)' +batch_test_atom refs/heads/main '%(creatordate)' +batch_test_atom refs/heads/main '%(subject)' +batch_test_atom refs/heads/main '%(subject:sanitize)' +batch_test_atom refs/heads/main '%(contents:subject)' +batch_test_atom refs/heads/main '%(body)' +batch_test_atom refs/heads/main '%(contents:body)' +batch_test_atom refs/heads/main '%(contents:signature)' +batch_test_atom refs/heads/main '%(contents)' +batch_test_atom refs/heads/main '%(HEAD)' fail +batch_test_atom refs/heads/main '%(upstream:track)' fail +batch_test_atom refs/heads/main '%(upstream:trackshort)' fail +batch_test_atom refs/heads/main '%(upstream:track,nobracket)' fail +batch_test_atom refs/heads/main '%(upstream:nobracket,track)' fail +batch_test_atom refs/heads/main '%(push:track)' fail +batch_test_atom refs/heads/main '%(push:trackshort)' fail +batch_test_atom refs/heads/main '%(worktreepath)' fail +batch_test_atom refs/heads/main '%(symref)' fail +batch_test_atom refs/heads/main '%(flag)' fail + +batch_test_atom refs/tags/testtag '%(refname)' fail +batch_test_atom refs/tags/testtag '%(refname:short)' fail +batch_test_atom refs/tags/testtag '%(upstream)' fail +batch_test_atom refs/tags/testtag '%(push)' fail +batch_test_atom refs/tags/testtag '%(objecttype)' +batch_test_atom refs/tags/testtag '%(objectsize)' +batch_test_atom refs/tags/testtag '%(objectsize:disk)' +batch_test_atom refs/tags/testtag '%(*objectsize:disk)' +batch_test_atom refs/tags/testtag '%(deltabase)' +batch_test_atom refs/tags/testtag '%(*deltabase)' +batch_test_atom refs/tags/testtag '%(objectname)' +batch_test_atom refs/tags/testtag '%(objectname:short)' +batch_test_atom refs/tags/testtag '%(tree)' +batch_test_atom refs/tags/testtag '%(tree:short)' +batch_test_atom refs/tags/testtag '%(tree:short=1)' +batch_test_atom refs/tags/testtag '%(tree:short=10)' +batch_test_atom refs/tags/testtag '%(parent)' +batch_test_atom refs/tags/testtag '%(parent:short)' +batch_test_atom refs/tags/testtag '%(parent:short=1)' +batch_test_atom refs/tags/testtag '%(parent:short=10)' +batch_test_atom refs/tags/testtag '%(numparent)' +batch_test_atom refs/tags/testtag '%(object)' +batch_test_atom refs/tags/testtag '%(type)' +batch_test_atom refs/tags/testtag '%(*objectname)' +batch_test_atom refs/tags/testtag '%(*objecttype)' +batch_test_atom refs/tags/testtag '%(author)' +batch_test_atom refs/tags/testtag '%(authorname)' +batch_test_atom refs/tags/testtag '%(authoremail)' +batch_test_atom refs/tags/testtag '%(authoremail:trim)' +batch_test_atom refs/tags/testtag '%(authoremail:localpart)' +batch_test_atom refs/tags/testtag '%(authordate)' +batch_test_atom refs/tags/testtag '%(committer)' +batch_test_atom refs/tags/testtag '%(committername)' +batch_test_atom refs/tags/testtag '%(committeremail)' +batch_test_atom refs/tags/testtag '%(committeremail:trim)' +batch_test_atom refs/tags/testtag '%(committeremail:localpart)' +batch_test_atom refs/tags/testtag '%(committerdate)' +batch_test_atom refs/tags/testtag '%(tag)' +batch_test_atom refs/tags/testtag '%(tagger)' +batch_test_atom refs/tags/testtag '%(taggername)' +batch_test_atom refs/tags/testtag '%(taggeremail)' +batch_test_atom refs/tags/testtag '%(taggeremail:trim)' +batch_test_atom refs/tags/testtag '%(taggeremail:localpart)' +batch_test_atom refs/tags/testtag '%(taggerdate)' +batch_test_atom refs/tags/testtag '%(creator)' +batch_test_atom refs/tags/testtag '%(creatordate)' +batch_test_atom refs/tags/testtag '%(subject)' +batch_test_atom refs/tags/testtag '%(subject:sanitize)' +batch_test_atom refs/tags/testtag '%(contents:subject)' +batch_test_atom refs/tags/testtag '%(body)' +batch_test_atom refs/tags/testtag '%(contents:body)' +batch_test_atom refs/tags/testtag '%(contents:signature)' +batch_test_atom refs/tags/testtag '%(contents)' +batch_test_atom refs/tags/testtag '%(HEAD)' fail + +batch_test_atom refs/myblobs/blob1 '%(refname)' fail +batch_test_atom refs/myblobs/blob1 '%(upstream)' fail +batch_test_atom refs/myblobs/blob1 '%(push)' fail +batch_test_atom refs/myblobs/blob1 '%(HEAD)' fail + +batch_test_atom refs/myblobs/blob1 '%(objectname)' +batch_test_atom refs/myblobs/blob1 '%(objecttype)' +batch_test_atom refs/myblobs/blob1 '%(objectsize)' +batch_test_atom refs/myblobs/blob1 '%(objectsize:disk)' +batch_test_atom refs/myblobs/blob1 '%(deltabase)' + +batch_test_atom refs/myblobs/blob1 '%(contents)' +batch_test_atom refs/myblobs/blob2 '%(contents)' + +batch_test_atom refs/myblobs/blob1 '%(raw)' +batch_test_atom refs/myblobs/blob2 '%(raw)' +batch_test_atom refs/mytrees/tree1 '%(raw)' + +batch_test_atom refs/myblobs/blob1 '%(raw:size)' +batch_test_atom refs/myblobs/blob2 '%(raw:size)' +batch_test_atom refs/mytrees/tree1 '%(raw:size)' + +batch_test_atom refs/myblobs/blob1 '%(if:equals=blob)%(objecttype)%(then)commit%(else)not commit%(end)' +batch_test_atom refs/myblobs/blob2 '%(if:equals=blob)%(objecttype)%(then)commit%(else)not commit%(end)' +batch_test_atom refs/mytrees/tree1 '%(if:equals=tree)%(objecttype)%(then)tree%(else)not tree%(end)' + +batch_test_atom refs/heads/main '%(align:60) objectname is %(objectname)%(end)|%(objectname)' +batch_test_atom refs/heads/main '%(align:left,60) objectname is %(objectname)%(end)|%(objectname)' +batch_test_atom refs/heads/main '%(align:middle,60) objectname is %(objectname)%(end)|%(objectname)' +batch_test_atom refs/heads/main '%(align:60,right) objectname is %(objectname)%(end)|%(objectname)' + +batch_test_atom refs/heads/main 'VALID' +batch_test_atom refs/heads/main '%(INVALID)' fail +batch_test_atom refs/heads/main '%(authordate:INVALID)' fail + +test_expect_success '%(rest) works with both a branch and a tag' ' + cat >expected <<-EOF && + 123 commit 123 + 456 tag 456 + EOF + git cat-file --batch-check="%(rest) %(objecttype) %(rest)" >actual <<-EOF && + refs/heads/main 123 + refs/tags/testtag 456 + EOF + test_cmp expected actual +' + +batch_test_atom refs/heads/main '%(objectname) %(objecttype) %(objectsize) +%(raw)' +batch_test_atom refs/tags/testtag '%(objectname) %(objecttype) %(objectsize) +%(raw)' +batch_test_atom refs/myblobs/blob1 '%(objectname) %(objecttype) %(objectsize) +%(raw)' +batch_test_atom refs/myblobs/blob2 '%(objectname) %(objecttype) %(objectsize) +%(raw)' + + +test_expect_success 'cat-file --batch equals to --batch-check with atoms' ' + git cat-file --batch-check="%(objectname) %(objecttype) %(objectsize) +%(raw)" >expected <<-EOF && + refs/heads/main + refs/tags/testtag + EOF + git cat-file --batch >actual <<-EOF && + refs/heads/main + refs/tags/testtag + EOF + cmp expected actual +' test_done