From: Carlos Rica <jasampler@gmail.com>
Date: Wed, 18 Jul 2007 15:40:09 GMT
Subject: Re: [PATCH] Rename read_pipe() with read_fd() and make its buffer nul-terminated.
Message-ID: <1b46aba20707180840l6c12fecfy4c610fe3a1a36c5e@mail.gmail.com>
In-Reply-To: <7vps2q6tjf.fsf@assigned-by-dhcp.cox.net>

2007/7/18, Junio C Hamano <gitster@pobox.com>:
>
> >       unsigned long size = *return_size;
> >       ssize_t iret;
> >       unsigned long off = 0;
> >
> > +     if (!buf || size <= 1) {
> > +             size = alloc_nr(size);
> > +             buf = xrealloc(buf, size);
> > +     }
> > +
>
> Hmph.  The reason this is not "!size" is because you would want
> more than one.  As your plan is to use this mostly for the -F
> option of "tag/commit", I suspect using a bit larger default,
> such as 80 (just a line), or probably 1k (most log messages
> would fit in such a buffer), would be more practical.

The true reason is because I before tried using (size-1) instead of (size)
in xread(..., size - off), and then I forgot to remove that condition. Sorry.

>
> >       do {
> >               iret = xread(fd, buf + off, size - off);
> >               if (iret > 0) {
> >                       off += iret;
> >                       if (off == size) {
> > -                             size *= 2;
> > +                             size = alloc_nr(size);
> >                               buf = xrealloc(buf, size);
> >                       }
> >               }
> >       } while (iret > 0);
> >
> > +     if (off == size)
> > +             buf = xrealloc(buf, size + 1);
> > +     buf[off] = '\0';
> > +
>
> I wonder if doing xread(... (size-1) - off) in the loop would
> ensure (off <= size-1) here.  You also would need to update the
> realloc condition inside loop if you do so.

I have chosen that solution to leave the code easy to read, but the
(size-1) version will avoid that additional (but non-frequent) realloc.

I will resend the fixed patch along with the builtin-tag.c one.