Re: [PATCH 2/5] [GSOC] ref-filter: add %(raw) atom

From: ZheNing Hu <adlternative@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com>,
	Git List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Christian Couder <christian.couder@gmail.com>,
	Hariom Verma <hariom18599@gmail.com>,
	Bagas Sanjaya <bagasdotme@gmail.com>, Jeff King <peff@peff.net>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Philip Oakley <philipoakley@iee.email>
Subject: Re: [PATCH 2/5] [GSOC] ref-filter: add %(raw) atom
Date: Thu, 22 Jul 2021 16:31:18 +0800	[thread overview]
Message-ID: <CAOLTT8TP4z1vR8i9DX09+oBiPVgaF1isTgD2ZQ4nfoBJwTd9_w@mail.gmail.com> (raw)
In-Reply-To: <877dhivkei.fsf@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2021年7月22日周四 下午4:10写道：

>
>
> On Thu, Jul 22 2021, ZheNing Hu via GitGitGadget wrote:
>
> > [..] add a new member in
> > `struct atom_value`: `s_size`, which can record raw object size, it
> > can help us add raw object data to the buffer or compare two buffers
> > which contain raw object data.
>
> I didn't look in detail, just one nit/comment, no need to re-roll for this:
>
>
> >  struct atom_value {
> >       const char *s;
> > +     size_t s_size;
> >       int (*handler)(struct atom_value *atomv, struct ref_formatting_state *state,
> >                      struct strbuf *err);
> >       uintmax_t value; /* used for sorting when not FIELD_STR */
> >       struct used_atom *atom;
> >  };
> >
> > +#define ATOM_VALUE_S_SIZE_INIT (-1)
> > +
>
> This and then later doing (this appears to be the only initialization?):

Yes, "s_size = -1” is the only initialization.

>
> >       if (format->need_color_reset_at_eol) {
> >               struct atom_value resetv;
> > +             resetv.s_size = ATOM_VALUE_S_SIZE_INIT;
> >               resetv.s = GIT_COLOR_RESET;
> >               if (append_atom(&resetv, &state, error_buf)) {
> >                       pop_stack_element(&state.stack);
>
> Is a rather unusual pattern, more typical would be:
>
>         struct atom_value { .... }
>         #define ATOM_VALUE_INIT { \
>                 .s_size = -1, \
>         }
>
> Then:
>
>         struct atom_value resetv = ATOM_VALUE_INIT;
>
> You could keep that "#define ATOM_VALUE_S_SIZE_INIT (-1)" to check the
> init value, personally I think it's just fine to hardcode the -1 literal
> and do "size < 0" checks, which we us as convention in a lot of other
> places for "not initialized".

Maybe you are right, hard-coding "-1" is ok.

Thanks.
--
ZheNing Hu