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

[Christian Couder <christian.couder@gmail.com>]

On Mon, Jul 12, 2021 at 1:47 PM ZheNing Hu via GitGitGadget
<gitgitgadget@gmail.com> 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".

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

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.

> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Hariom Verma <hariom18599@gmail.com>
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
>  Documentation/git-cat-file.txt |   6 +
>  builtin/cat-file.c             | 242 ++++++-------------------------
>  t/t1006-cat-file.sh            | 251 +++++++++++++++++++++++++++++++++
>  3 files changed, 304 insertions(+), 195 deletions(-)
>
> 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
> +}

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.

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

[...]

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

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

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