Re: [PATCHv4 02/10] send-pack: Attempt to retrieve remote status even if pack-objects fails

From: Johan Herland <johan@herland.net>
Date: May 23, 2011, 22:58 UTC
Message-ID: <201105240058.10974.johan@herland.net>
In-Reply-To: <7vei3puqp2.fsf@alter.siamese.dyndns.org>

On Monday 23 May 2011, Junio C Hamano wrote:

Show 30 quoted lines

> Johan Herland <johan@herland.net> writes:
> > @@ -339,25 +339,18 @@ int send_pack(struct send_pack_args *args,
> > 
> >  	}
> >  	
> >  	if (new_refs && cmds_sent) {
> > 
> > -		if (pack_objects(out, remote_refs, extra_have, args) < 0) {
> > -			for (ref = remote_refs; ref; ref = ref->next)
> > -				ref->status = REF_STATUS_NONE;
> > +		if ((ret = pack_objects(out, remote_refs, extra_have, args))) {
> 
> I am not very familiar with this codepath, but you no longer set
> ref->status to REF_STATUS_NONE ...
> 
> > ...
> > 
> >  	if (status_report && cmds_sent)
> > 
> > -		ret = receive_status(in, remote_refs);
> > -	else
> > -		ret = 0;
> > +		ret |= receive_status(in, remote_refs);
> 
> ... before calling receive_status() here, and that function can return
> early without setting anything.
> 
> Would that have any negative effect on the code that comes after this
> part in the codepath?  or if receive_status() returns a failure, we do
> not even look at ref->status?

Hmm... I believe I proved the correctness of this to myself when I first wrote the patch, but looking at it a second time, I see that I only did so for send_pack() itself. The remote_refs (that no longer has each ref->status set to REF_STATUS_NONE on pack_objects() failure) are given as an argument to send_pack(), and are still used by the caller after send_pack() has returned (even when it returns with errors).

Therefore, I was wrong to remove this "ref->status = REF_STATUS_NONE" loop. Will be fixed in the next iteration.

Thanks for noticing,

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net