Re: [PATCH 14/19] [GSOC] cat-file: reuse ref-filter logic

Christian Couder [off-list ref] 于2021年7月12日周一 下午9:17写道：

On Mon, Jul 12, 2021 at 1:47 PM ZheNing Hu via GitGitGadget
[off-list ref] wrote:

quoted

From: ZheNing Hu <redacted>

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

quoted

/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".

Saying that a later patch will add a fast path which will mitigate the
performance regression introduced by this patch might help reassure
reviewers.

OK.

By the way it is not clear if adding the fast path fully mitigates
this performance regression or not. You might want to discuss that in
the cover letter, or maybe in the patch adding the fast path.

I mentioned it: "By using this fast path, we can reduce some of the
extra overhead
when cat-file --batch using ref-filter. The running time of
git cat-file --batch-check will be similar to before, and the
running time of git cat-file --batch will be 9.1% less than before."
which is using the result of t/perf/p1006-cat-file.sh.

I wonder if the above function and some of the tests below could be
introduced in a preparatory patch before this one. It could help check
that reusing ref-filter doesn't change the behavior with some atoms
that were previously supported or rejected. Of course if some atoms
are now failing or are now supported, then it's ok to add new tests
for these atoms in this patch.

Yes, it might be worth splitting into two commits.

quoted

+batch_test_atom refs/heads/main '%(refname)' fail
+batch_test_atom refs/heads/main '%(refname:)' fail

[...]

quoted

+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
+'

It's a bit strange that this test is added in this patch while the
commit message doesn't talk about %(rest). So I wonder if this new
test could move to another previous commit.

It's just used for checking the uncommonly atoms "%(rest)".
But as you said, we can move it to a split commit.

quoted

+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)'
+
+

It looks like there are two empty lines instead of one above.

quoted

+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
+'

I also wonder if the above new test belong to this commit or if it
could be moved to a previous commit.

It's same.

Thanks.
--
ZheNing Hu