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

["Shawn O. Pearce" <spearce@spearce.org>]

Andy Whitcroft <apw@shadowen.org> wrote:
> 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.  ;-)

> > +
> > +        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.