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

From: ZheNing Hu <adlternative@gmail.com>
Date: Jul 24, 2021, 07:57 UTC
Message-ID: <CAOLTT8SggCXkajPG3om+6zhM_K8fyAb2qTBDj40JJa1pszshzg@mail.gmail.com>
In-Reply-To: <xmqqh7glouiw.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> 于2021年7月24日周六 上午12:38写道：

Show 39 quoted lines

>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> >  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_INIT { \
> > +     .s_size = -1 \
> > +}
>
> This will get turned into size_t that is unsigned, and comparisons
> like this one and ...
>
> >       case QUOTE_NONE:
> > -             strbuf_addstr(s, str);
> > +             if (len != -1)
> > +                     strbuf_add(s, str, len);
> > +             else
> > +                     strbuf_addstr(s, str);
>
> this one ...
>
> > +             if (v->s_size != -1)
> > +                     strbuf_add(&state->stack->output, v->s, v->s_size);
> > +             else
> > +                     strbuf_addstr(&state->stack->output, v->s);
> >       return 0;
> >  }
>
> may work as expected, but it makes readers wonder if there is
> significance to negative values other than -1 (in other words, what
> does it mean if v->s_size == -2, for example?).
>

Yes, any value less than -1 is meaningless here.

> It may make sense to
>
>  * Turn atom_value.s_size field into ssize_t instead of size_t
>

Will the conversion of size_t and ssize_t break -Wsign-conversion? Although there is a lot of code in Git that has broken it, but I am not sure if it is wise to use ssize_t here.

>  * Rewrite (v->s_size != -1) comparison to (v->s_size < 0)
>

For size_t, this scheme is wrong.

Show 6 quoted lines

>
> Optionally we could introduce #define ATOM_SIZE_UNSPECIFIED (-1) and
> use it to initialize .s_size in ATOM_VALUE_INIT, but if we are just
> going to test "is it negative, then it is not given", then it probably
> is not needed.
>

It seems that this is the only method left. Although I think ATOM_SIZE_UNSPECIFIED is not very useful becasue we already have ATOM_VALUE_INIT.

Show 5 quoted lines

> Other changes in the whole series relative to what has been queued
> looked all sensible to me.
>
> THanks.
>

Thanks. -- ZheNing Hu