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

Nicolas Pitre [off-list ref] wrote:

On Thu, 28 Aug 2008, Shawn O. Pearce wrote:

quoted

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.
 

quoted

quoted

 void fixup_pack_header_footer(int pack_fd,

...

quoted

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

quoted

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.