Re: [PATCH 1/3] improve reliability of fixup_pack_header_footer()

From: Shawn O. Pearce <spearce@spearce.org>
Date: Aug 29, 2008, 14:30 UTC
Message-ID: <20080829143023.GA7403@spearce.org>
In-Reply-To: <alpine.LFD.1.10.0808290844200.1624@xanadu.home>

Nicolas Pitre <nico@cam.org> wrote:

Show 9 quoted lines

> On Thu, 28 Aug 2008, Shawn O. Pearce wrote:
> > 
> > I found your implementation in fixup_pack_header to be very
> > contorted.  Did you read the JGit patch I posted?
> 
> Well, actually, I don't find the JGit implementation easier to follow at 
> all.  Of course I wrote the C version while you wrote the Java version.  
> Maybe without our respective bias then things are just fine in both 
> cases.

I probably should have gone and edited my eariler comments after I
reached the end of the patch and the light dawned about why you are
summing the tail.  Most of what I found to be complex in your code
was just to handle that boundary condition at partial_pack_offset
and restart the checksum for the tail.  But I honestly cannot see
an easier way to write that, so what you have is just fine.

Show 13 quoted lines

> > >  void fixup_pack_header_footer(int pack_fd,
> > ...
> > > +	if (partial_pack_sha1 && !partial_pack_offset) {
> > > +		partial_pack_offset = lseek(pack_fd, 0, SEEK_CUR);
> > > +		if (partial_pack_offset == (off_t)-1)
> > 
> > Eh?
> > 
> > I find this to be nonsense.
> 
> This is not for thin packs but for split packs in repack-objects. And 
> yeah, the caller has the offset information already in that case too, so 
> this could be removed.  It just felt more generic that way originally.

Oh, yea, that makes sense.  It still seems like playing with fire.

I'd rather the caller pass in the proper offset than rely on it being the current position of the fd. Especially if the caller does actually have it available.

If you change anything, I'd like to see this lseek(SEEK_CUR) go away.

> And another thing I had in store (but for which you _again_ beat me to :-) )
> is to realign data reads onto filesystem blocks.

That _really_ made the JGit code ugly.  But I think its worth it.

I also want to try and buffer the whole object appending we do
during fixThinPack(), as right now we write the object header in
one write and then compressed data bursts in the others.  Moving it
to at least write a full 4k at a time should remove about 2 write
calls per object.

Show 7 quoted lines

> > That's freaking brilliant.  And something I missed in JGit.
> > The way its implemented is downright difficult to follow though.
> > I'll admit it took me a good 10 minutes to understand what you were
> > doing here, and why.
> 
> Again maybe that's just because you're just too biased by your own 
> implementation.  I don't consider this code particularly obscur.

My own code in JGit got "obscure" when I added this check too. I take back the remark above.

-- 
Shawn.