Junio C Hamano <gitster@pobox.com> 于2021年1月26日周二 下午2:17写道：
Show 14 quoted lines
>
> "阿德烈 via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > the usage in strbuf.h tell us"Alloc is somehow a
> > "private" member that should not be messed with.
> > use `strbuf_avail()`instead."
>
> When we use the word "private", it generally means it is private to
> the implementation of the API. IOW, it is usually fine for the
> implementation of the API (i.e. for strbuf API, what you see in
> strbuf.c) to use private members.
>
Well, I just think most other functions in strbuf.c follow the use of `strbuf_avail()` instead of "sb->alloc-sb->len-1", and the "sb->alloc-sb->len-1" that appears in `strbuf_read()` is not so uniform.
Show 15 quoted lines
> In any case, these changes are _not_ optimizations.
>
> Replacing (alloc - len - 1) with strbuf_avail() is at best an
> equivalent rewrite (which is a good thing from readability's point
> of view, but not an optimization). We know sb->alloc during the
> loop is never 0, but the compiler may miss the fact, so the inlined
> implementation of _avail, i.e.
>
> static inline size_t strbuf_avail(const struct strbuf *sb)
> {
> return sb->alloc ? sb->alloc - sb->len - 1 : 0;
> }
>
> may not incur call overhead, but may be pessimizing the executed
> code.I agree,It may be a good practice not to use redundant inline functions, because it will not make the git binary file too bloated.
Show 20 quoted lines
>
> If you compare the code in the loop in the second hunk below with
> what _setlen() does, I think you'll see the overhead of _setlen()
> relative to the original code is even higher, so it may also be
> pessimizing, not optimizing.
>
> So, overall, I am not all that enthused to see this patch.
>
>
> One thing I noticed is that, whether open coded like sb->len += got
> or made into parameter to strbuf_setlen(sb, sb->len + got), we are
> not careful about sb->len growing too large and overflowing with the
> addition. That may potentially be an interesting thing to look
> into, but at the same time, unlike the usual "compute the number of
> bytes we need to allocate and then call xmalloc()" pattern, where we
> try to be careful in the "compute" step by using st_add() macros,
> this code actually keep growing the buffer, so by the time the size_t
> overflows and wraps around, we'd certainly have exhausted the memory
> already, so it won't be an issue.
>
This is true, but is there any good way to avoid this form of overflow?
Show 7 quoted lines
> But we may want to audit existing code that is not careful when
> preparing the second parameter to strbuf_setlen(). We just
> analyzed, if we were to accept this patch, that "sb->len + got" that
> appear as the second parameter to new call of strbuf_setlen() looks
> bad but would not matter in practice, but we may not be so lucky in
> other places.
>
I thought before `strbuf_read_once()`have almost analogous "strbuf_setlen(sb, sb->len + cnt)",so I change it.May be you are right, "sb->len + got"is not safe.
Show 35 quoted lines
> Thanks for a food for thought.
>
> > diff --git a/strbuf.c b/strbuf.c
> > index e3397cc4c72..76f560a28d0 100644
> > --- a/strbuf.c
> > +++ b/strbuf.c
> > @@ -517,7 +517,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
> >
> > strbuf_grow(sb, hint ? hint : 8192);
> > for (;;) {
> > - ssize_t want = sb->alloc - sb->len - 1;
> > + ssize_t want = strbuf_avail(sb);
> > ssize_t got = read_in_full(fd, sb->buf + sb->len, want);
> >
> > if (got < 0) {
> > @@ -527,7 +527,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
> > strbuf_setlen(sb, oldlen);
> > return -1;
> > }
> > - sb->len += got;
> > + strbuf_setlen(sb, sb->len + got);
> > if (got < want)
> > break;
> > strbuf_grow(sb, 8192);
> > @@ -543,7 +543,7 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)
> > ssize_t cnt;
> >
> > strbuf_grow(sb, hint ? hint : 8192);
> > - cnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);
> > + cnt = xread(fd, sb->buf + sb->len, strbuf_avail(sb));
> > if (cnt > 0)
> > strbuf_setlen(sb, sb->len + cnt);
> > else if (oldalloc == 0)
> >
> > base-commit: 6d3ef5b467eccd2769f1aa1c555d317d3c8dc707
