Re: [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence.

From: Shawn O. Pearce <spearce@spearce.org>
Date: Jan 9, 2007, 23:25 UTC
Message-ID: <20070109232540.GA30023@spearce.org>
In-Reply-To: <45A40C15.1070200@shadowen.org>

Andy Whitcroft <apw@shadowen.org> wrote:

Show 9 quoted lines

> Stefan-W. Hahn wrote:
> > Using cygwin with cygwin.dll before 1.5.22 the system call pread() is buggy.
> > This patch introduces NO_PREAD. If NO_PREAD is set git uses a sequence of
> > lseek()/xread()/lseek() to emulate pread.
> > +
> > +        rc=read_in_full(fd, buf, count);
> 
> Seems to be style inconsistancy between current_offset = and rc= I
> believe the former is preferred.

With the exception of this style difference, the patch looked pretty good. Nice work Stefan. Andy's right, we do tend to prefer "rc = read_in_full" over "rc=read_in_full". Quite a bit actually, though Junio is the final decider on all such matters as he gets to choose to accept or reject the patch. ;-)

Show 9 quoted lines

> > +
> > +        if (current_offset != lseek(fd, current_offset, SEEK_SET))
> > +                return -1;
> 
> How likely are we ever to be in the right place here?  Seems vanishingly
> small putting us firmly in the four syscalls per call space.  I wonder
> if git ever actually cares about the seek location.  ie if we could stop
> reading and resetting it.  Probabally not worth working it out I guess
> as any _sane_ system has one.

Andy's right actually. If we are using pread() we aren't relying on the current file pointer. Which means its unnecessary to get the current pointer before seeking to the requested offset, and its unnecessary to restore it before the git_pread() function returns.

Though its a possibly unnecessary optimization as like Andy points out, most sane systems already have a working pread() implementation. And those that don't, well, probably should be made to be sane. But we don't need to make Git suffer there if we don't have to.

-- 
Shawn.