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

From: ZheNing Hu <adlternative@gmail.com>
Date: Jul 25, 2021, 12:47 UTC
Message-ID: <CAOLTT8Sm=qs0K84C=yJOL7Ct5GUeHYe=ODXiGpwCSx11vgEnwA@mail.gmail.com>
In-Reply-To: <xmqqy29vk3sp.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> 于2021年7月25日周日 上午1:41写道：

Show 37 quoted lines

>
> ZheNing Hu <adlternative@gmail.com> writes:
>
> >> 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.
> >
> >>
> >> 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.
>
> Sorry, but I think you misread what I wrote.
>
> These three were not offered as "you can do one of these three, pick
> one you like" choices.  I meant to say "I think it makes sense to do
> all these three things, but the last one is optional, doing only the
> first two may be good enough".  As the second one requires that the
> first is done, of course, doing only the second one would not make
> sense.
>

OK. I know it now, I will do all of them.

Show 7 quoted lines

> Also, you seem to have missed the distinction between _INIT and
> _UNSPECIFIED.  You can initialize something to (1) a reasonable
> default value, or (2) unusable value that you can detect at runtime
> and notice that it was not set.  If you called something to
> FOO_INIT, your readers cannot tell which one it is, but if you call
> it FOO_UNSPECIFIED, it is clear it is the latter case.
>

Thanks for clarification, I understand the difference between them now.

Show 5 quoted lines

> In many places, we do use ssize_t for "normally this is size, but we
> can express exception cases (like errors) by storing negative value"
> in our codebase (grep for it), and I think the member in question is
> prime candidate for such use.
>

Yeah, as abspath.c:137, `ssize_t len` used to distinguish situations if an error is returned from strbuf_readlink().

           ssize_t len;
           ...
           len = strbuf_readlink(&symlink, resolved->buf,
                                             st.st_size);
           if (len < 0) {
                               if (flags & REALPATH_DIE_ON_ERROR)
                                       die_errno("Invalid symlink '%s'",
                                                 resolved->buf);
                               else
                                       goto error_out;
                       }
           ...

Or just read() and write(), they return is ssize_t too, which can return -1 and set errno when an exception occurs.

Thanks. -- ZheNing Hu