Re: [bug in next ?] git-fetch/git-push issue
- From
Jeff King <peff@peff.net>
- Date
- Nov 5, 2007, 22:55 UTC
- Message-ID
- <20071105225540.GA10988@sigill.intra.peff.net>
- In-Reply-To
- <Pine.LNX.4.64.0711051620230.7357@iabervon.org>
On Mon, Nov 05, 2007 at 04:41:37PM -0500, Daniel Barkalow wrote:
Show 6 quoted lines
> > 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?
I think the current behavior is more odd. We mark some errors, and then if there were any possible pushes, we replace the marked errors with the status of the actual push, forgetting about the previous errors. Thus the behavior where if 'next' needs pushing, then we don't mark any errors at all (even though we spewed an error to stderr), but if it doesn't, then we return an error.
I don't mind being conservative with updating tracking refs; they really are just an optimization to avoid an extra git-fetch. But the most sensible behavior would be to mark errors for _each_ ref individually, try to push or update tracking branches where appropriate, and then return an error status based on all refs (whether they had an error in prep time or at push time).
Which I guess is what you were trying to accomplish by removing the peer_ref, though I think that doesn't distinguish between "didn't match a remote ref" and "had an error." Perhaps we just need an error flag in the ref struct?
> 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
I would also be fine with that: if your intended push has _any_ problems, then abort the push.
> we fetch, we should see exclusively changes that somebody else (including > hooks remotely) did, not anything that we ourselves did.
I don't necessarily agree, just because the notion of "we" and "somebody else" is sort of pointless in a distributed system. I consider local tracking ref updates to be a "best guess" attempt to optimize and avoid a fetch (but one that will always be overwritten by the _actual_ contents when you do fetch).
> 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
In that case, I think the simple fix is:
diff --git a/builtin-send-pack.c b/builtin-send-pack.c index 947c42b..f773dc8 100644 --- a/builtin-send-pack.c +++ b/builtin-send-pack.c @@ -338,7 +338,7 @@ static int do_send_pack(int in, int out, struct remote *remote, int nr_refspec, packet_flush(out); if (new_refs && !args.dry_run) - ret = pack_objects(out, remote_refs); + ret |= pack_objects(out, remote_refs); close(out); if (expect_status_report) { and then we accept that we don't know _which_ refs shouldn't have their tracking branches updated (and we don't update any). But at least we don't forget that the error occured. And the better solution is an error flag (or bitfield) in struct ref. -Peff