Re: [PATCH 6/7] worktree: check the result of read_in_full()
- From
Jeff King <peff@peff.net>
- Date
- Sep 25, 2017, 23:25 UTC
- Message-ID
- <20170925232519.ljbt37r7qvb4or32@sigill.intra.peff.net>
- In-Reply-To
- <20170925221426.GE27425@aiede.mtv.corp.google.com>
On Mon, Sep 25, 2017 at 03:14:26PM -0700, Jonathan Nieder wrote:
Show 18 quoted lines
> > diff --git a/builtin/worktree.c b/builtin/worktree.c
> > index 2f4a4ef9cd..87b3d70b0b 100644
> > --- a/builtin/worktree.c
> > +++ b/builtin/worktree.c
> > @@ -59,7 +59,11 @@ static int prune_worktree(const char *id, struct strbuf *reason)
> > }
> > len = xsize_t(st.st_size);
> > path = xmallocz(len);
> > - read_in_full(fd, path, len);
> > + if (read_in_full(fd, path, len) != len) {
> > + strbuf_addf(reason, _("Removing worktrees/%s: gitdir read did not match stat (%s)"),
> > + id, strerror(errno));
>
> I'm a little confused. The 'if' condition checks for a read error but
> the message says something about 'stat'.
>
> If we're trying to double-check the 'stat' result, shouldn't we read
> all the way to EOF in case the file got longer?If you wanted to really double-check the stat result, yes you'd want to make sure there aren't extra bytes either. But really we're just making sure we were able to read _enough_ bytes.
To be honest, I'm tempted to rip out the whole thing and replace it with strbuf_read_file(), which seems more straightforward.
The function does seem to rely on the stat() to get the mtime, so we'd have to leave that part in.
-Peff