From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff King Subject: Re: [bug in next ?] git-fetch/git-push issue Date: Mon, 5 Nov 2007 17:55:41 -0500 Message-ID: <20071105225540.GA10988@sigill.intra.peff.net> References: <20071105175654.GD6205@artemis.corp> <20071105210711.GA9176@sigill.intra.peff.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Pierre Habouzit , Nicolas Pitre , Git ML To: Daniel Barkalow X-From: git-owner@vger.kernel.org Mon Nov 05 23:56:00 2007 Return-path: Envelope-to: gcvg-git-2@gmane.org Received: from vger.kernel.org ([209.132.176.167]) by lo.gmane.org with esmtp (Exim 4.50) id 1IpArP-00062W-RC for gcvg-git-2@gmane.org; Mon, 05 Nov 2007 23:56:00 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754521AbXKEWzp (ORCPT ); Mon, 5 Nov 2007 17:55:45 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1754509AbXKEWzp (ORCPT ); Mon, 5 Nov 2007 17:55:45 -0500 Received: from 66-23-211-5.clients.speedfactory.net ([66.23.211.5]:2976 "EHLO peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754483AbXKEWzo (ORCPT ); Mon, 5 Nov 2007 17:55:44 -0500 Received: (qmail 25869 invoked by uid 111); 5 Nov 2007 22:55:42 -0000 Received: from c-24-125-35-113.hsd1.va.comcast.net (HELO sigill.intra.peff.net) (24.125.35.113) (smtp-auth username relayok, mechanism cram-md5) by peff.net (qpsmtpd/0.32) with ESMTP; Mon, 05 Nov 2007 17:55:42 -0500 Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Mon, 05 Nov 2007 17:55:41 -0500 Content-Disposition: inline In-Reply-To: Sender: git-owner@vger.kernel.org Precedence: bulk X-Mailing-List: git@vger.kernel.org Archived-At: On Mon, Nov 05, 2007 at 04:41:37PM -0500, Daniel Barkalow wrote: > > 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