Re: [bug in next ?] git-fetch/git-push issue

[Daniel Barkalow <barkalow@iabervon.org>]

On Mon, 5 Nov 2007, Jeff King wrote:

> On Mon, Nov 05, 2007 at 01:17:14PM -0500, Daniel Barkalow wrote:
> 
> > I think this is the bit that's wrong. I blame Jeff, in 334f4831. :)
> > 
> > The issue is that, in the previous version, we'd hit a continue on the 
> > not-an-ancestor message and not reach the update_tracking_ref() section 
> > for that ref. In 334f4831, all of the updating is after the loop, and it 
> > doesn't filter out the refs that didn't actually get pushed.
> 
> Nope, that's not the problem. We _only_ update any tracking refs at all
> if ret == 0, and if we fail to push, then we are setting ret to -2.

That's an odd combination of behavior: we update some of the refs, leave 
the ones that didn't work alone, report success on the ones that worked, 
but then we forget that some things worked?

If we're going to refuse to update local tracking refs, whose state 
doesn't matter much, we should certainly refuse to update the remote refs, 
which are probably public and extremely important. If we just pushed and 
we fetch, we should see exclusively changes that somebody else (including 
hooks remotely) did, not anything that we ourselves did.

> Hrm. Oh wait, it looks like we then totally write over the current value
> of 'ret' when we do pack_objects. Oops.
> 
> I'm unclear how to fix this, as I'm not really sure what ret is
> _supposed_ to be communicating. What does the '-2' mean, as compared to
> a '-4'? Should we be doing a 'ret += pack_objects(out, remote_refs)' or
> some other bit-masking magic?

I'd guess -2 is supposed to indicate that there were some errors but some 
things may have worked. If pack_objects() or receive_status() fails, we 
shouldn't update anything locally (because it won't have been accepted 
remotely); otherwise, we should make local updates of every remote efect 
we had, even if we end up returning non-zero to indicate that we didn't do 
everything asked.

	-Daniel
*This .sig left intentionally blank*