On Monday 23 May 2011, Junio C Hamano wrote:
> Johan Herland <johan@xxxxxxxxxxx> 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@xxxxxxxxxxx>
www.herland.net
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html