On Sat, Jun 19, 2021 at 9:03 AM ZheNing Hu via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
>
> From: ZheNing Hu <adlternative@xxxxxxxxx>
>
> 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
> +'
