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

From: Daniel Barkalow <barkalow@iabervon.org>
Date: Nov 5, 2007, 21:41 UTC
Message-ID: <Pine.LNX.4.64.0711051620230.7357@iabervon.org>
In-Reply-To: <20071105210711.GA9176@sigill.intra.peff.net>

On Mon, 5 Nov 2007, Jeff King wrote:

Show 11 quoted lines

> 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.

Show 7 quoted lines

> 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*