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

[Jeff King <peff@peff.net>]

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