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

[Johan Herland <johan@herland.net>]

On Monday 23 May 2011, Junio C Hamano wrote:
> 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