Christian Couder <christian.couder@xxxxxxxxx> 于2021年6月21日周一 下午1:55写道：
>
> > 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.
Well, there is something wrong with the expression here. What I want
to express is that we can fill its member "format" with the atoms like
"%(objectname) %(refname)", and then pass it to the ref-filter.
> > 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.
>
Yeah, But I don’t know if I should take it apart step by step, If taken apart,
those intermediate commits are likely to fail the test.
> > 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.
>
Makes sense. The performance has indeed deteriorated.
> [...]
>
> > + 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.
Yes, it is better to put error checking 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).
>
Ah, There is indeed only one situation for ret > 0 for the time being:
Show "<oid> missing" without exiting Git.
> > + 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);
> }
>
Yes, this might be better.
> > + 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.
>
OK.
> > + 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.
>
Although I think it doesn’t matter which one to use first,
I will still follow your suggestions.
> > 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);
>
No. here our opt->format.format will not be changed, it would
be better for us to use `strbuf_release(&format)` for resource
recovery. (strbuf_detach() will forced to let us free opt->format.format)
> > + 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.
>
I agree.
> [...]
>
> > +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.
>
OK.
Thanks.
--
ZheNing Hu
