Re: [PATCH/RFH] send-pack: fix pipeline.

From: Andy Whitcroft <apw@shadowen.org>
Date: Jan 2, 2007, 14:06 UTC
Message-ID: <459A66D2.3000804@shadowen.org>
In-Reply-To: <7vlkkql0na.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:

Show 36 quoted lines

> Linus Torvalds <torvalds@osdl.org> writes:
> 
>> On Fri, 29 Dec 2006, Junio C Hamano wrote:
>>> I really need a sanity checking on this one.  I think I got the
>>> botched pipeline fixed with the patch I am replying to, but I do
>>> not understand the waitpid() business.  Care to enlighten me?
>> I think it was a beginning of a half-hearted attempt to check the exit 
>> status of the rev-list in case something went wrong.
>>
>> Which we simply don't do, so if git-rev-list ends up with some problem 
>> (due to a corrupt git repo or something), it will just send a partial 
>> pack.
>>
>> For some reason I thought we had fixed that by just generating the object 
>> list internally, but I guess we don't do that. That's just stupid. We 
>> should make "send-pack.c" use
>>
>> 	list-heads | git pack-objects --revs
>>
>> 	list-heads | git-rev-list --stdin | git-pack-objects
>>
>> because as it is now, I think send-pack is more fragile than it needs to 
>> be.
>>
>> Or maybe I'm just confused.
> 
> Dont' worry, you are no more confused than I am ;-).
> 
> "I thought we've done the 'pack-objects --revs' for the
> upload-pack side but haven't done so on the send-pack side." was
> what I initially wrote, but apparently we haven't.  On the other
> hand, I think upload-pack gets error termination from rev-list
> right.
> 
> It seems that repack is the only thing that uses the internal
> rev-list.

>From what I can see in next/pu (by the time I stopped stuffing food and

booze into myself and remembered how to turn on the computer) you have ripped all this code out and started using the builtin rev-list functions. So what I can see in there now looks sane, and seems to work in some limited testing here.

Reading the code does highlight a weakness in the face of incomplete writes in the ref list send, which has always been in there. Now we may never see these on Linux, but as we do not know what OS is under us and the relevant standards say they can occur we should cope me thinks.

I have just been testing a patch for that which I will post in follow up to this post.

-apw