Re: [PATCH v3 08/10] [GSOC] cat-file: reuse ref-filter logic

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

On Sat, Jun 19, 2021 at 9:03 AM ZheNing Hu via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: ZheNing Hu <adlternative@gmail.com>
>
> In order to let cat-file use ref-filter logic, the following
> methods are used:

Maybe: s/the following methods are used/let's do the following/

> 1. Add `cat_file_mode` member in struct `ref_format`, this can
> help us reject atoms in verify_ref_format() which cat-file
> cannot use, e.g. `%(refname)`, `%(push)`, `%(upstream)`...
> 2. Change the type of member `format` in struct `batch_options`
> to `ref_format`, We can add format data in it.

Not sure what "We can add format data in it." means.

> 3. Let `batch_objects()` add atoms to format, and use
> `verify_ref_format()` to check atoms.
> 4. Use `has_object_file()` in `batch_one_object()` to check
> whether the input object exists.
> 5. Let get_object() return 1 and print "<oid> missing" instead
> of returning -1 and printing "missing object <oid> for <refname>",
> this can help `format_ref_array_item()` just report that the
> object is missing without letting Git exit.
> 6. 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 less than zero, use `die()` to print the error message
> and exit; else return value greater than zero, only print the

s/else return value greater/else if the return value is greater/

> error message, but not exit.

s/not exit/don't exit/

> 7. Use free_ref_array_item_value() to free ref_array_item's
> value.

That looks like a lot of changes in a single commit. I wonder if this
commit could be split.

> 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 our objects don't have refname.

s/refname/a refname/

It might be worth talking a bit about possible performance changes.

[...]

> +       ret = format_ref_array_item(&item, &opt->format, scratch, &err);
> +       if (!ret) {
> +               strbuf_addch(scratch, '\n');
> +               batch_write(opt, scratch->buf, scratch->len);
> +       } else if (ret < 0) {
> +               die("%s\n", err.buf);

This if (ret < 0) could be checked first.

> +       } else {
> +               /* when ret > 0 , don't call die and print the err to stdout*/

I think it would be more helpful to tell what ret > 0 means, rather
than what we do below (which can easily be seen).

> +               printf("%s\n", err.buf);
> +               fflush(stdout);
>         }

For example:

       if (ret < 0) {
               die("%s\n", err.buf);
       if (ret) {
               /* ret > 0 means ... */
               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 int batch_objects(struct batch_options *opt, const struct option *options)

It's unfortunate that one argument is called "opt" and the other one
"options". I wonder if the first one could be called "batch" as it
seems to be called this way somewhere else.

> +       if (!opt->format.format)
> +               strbuf_addstr(&format, "%(objectname) %(objecttype) %(objectsize)");
> +       else
> +               strbuf_addstr(&format, opt->format.format);

If there is no reason for the condition to be (!X) over just (X), I
think the latter is a bit better.

>         if (opt->print_contents)
> -               data.info.typep = &data.type;
> +               strbuf_addstr(&format, "\n%(raw)");
> +       opt->format.format = format.buf;

I wonder if this should be:

      opt->format.format = strbuf_detach(&format, NULL);

> +       if (verify_ref_format(&opt->format))
> +               usage_with_options(cat_file_usage, options);

[...]

> @@ -86,7 +87,7 @@ struct ref_format {
>         int need_color_reset_at_eol;
>  };
>
> -#define REF_FORMAT_INIT { NULL, NULL, 0, 0, -1 }
> +#define REF_FORMAT_INIT { NULL, NULL, 0, 0, 0, -1 }

Maybe this can already be changed to a designated initializer, like
Ævar suggested recently.

[...]

> +test_expect_success 'cat-file refs/heads/main refs/tags/testtag %(rest)' '

If this test is about checking that %(rest) works with both a branch
and a tag, it might be better to say it more explicitly.

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