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

From: ZheNing Hu <adlternative@gmail.com>
To: Christian Couder <christian.couder@gmail.com>
Cc: "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com>,
	git <git@vger.kernel.org>, "Junio C Hamano" <gitster@pobox.com>,
	"Hariom Verma" <hariom18599@gmail.com>,
	"Bagas Sanjaya" <bagasdotme@gmail.com>,
	"Jeff King" <peff@peff.net>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v3 08/10] [GSOC] cat-file: reuse ref-filter logic
Date: Mon, 21 Jun 2021 21:05:32 +0800	[thread overview]
Message-ID: <CAOLTT8TU+E+GaxR+fMvH5Rc-C+BEJDpqjmmrF_BO5kG-i-_gsQ@mail.gmail.com> (raw)
In-Reply-To: <CAP8UFD18iumUEgayGxL612MmbK9_2uDpHz3i7aqJ5zYSh7skqg@mail.gmail.com>

Christian Couder <christian.couder@gmail.com> 于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