{"thread":{"id":"2827","subject":"[PATCH] Make git-send-pack exit with error when some refs couldn't be pushed out","startedAt":"2005-12-14T00:45:40Z","lastAt":"2005-12-14T19:01:46Z","messageCount":5,"participants":["Petr Baudis","Junio C Hamano","Jon Loeliger"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"13597","messageId":"20051214004540.15040.93518.stgit@machine.or.cz","threadId":"2827","inReplyTo":null,"subject":"[PATCH] Make git-send-pack exit with error when some refs couldn't be pushed out","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2005-12-14T00:45:40Z","receivedAt":"2005-12-14T00:45:40Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"In case some refs couldn't be pushed out due to an error (mostly the\nnot-a-proper-subset error), make git-send-pack exit with non-zero status\nafter the push is over (that is, it still tries to push out the rest\nof the refs).\n\nCogito (0.17pre only, 0.16 is unaffected) needs to know that the push\nfailed so that it won't update its remote branch head pointer - otherwise\nit gets out of sync, which can lead even to loss of commits on the local\nside (this happenned to Jonas Fonseca - thanks for the report, BTW).\nI sort of expected that git-send-pack would return an error in case\nof a failure, so just applying this should magically fix Cogito.\n\nI don't insist on this particular solution though - a fetch-pack-alike\napproach where I get the list of changed refs on stdout is fine too\n(currently I get something on stderr, but I'm reluctant to grab stderr\nof GIT commands and try to heuristically mangle it).\n\nAt any rate, please consider this a major problem from Cogito perspective.\n\nSigned-off-by: Petr Baudis \n---\n\n send-pack.c | 6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex f61c15c..6ce0d9f 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -179,6 +179,7 @@ static int send_pack(int in, int out, in\n {\n \tstruct ref *ref;\n \tint new_refs;\n+\tint ret = 0;\n \n \t/* No funny business with the matcher */\n \tremote_tail = get_remote_heads(in, &remote_refs, 0, NULL, 1);\n@@ -232,6 +233,7 @@ static int send_pack(int in, int out, in\n \t\t\t\terror(\"remote '%s' object %s does not \"\n \t\t\t\t \"exist on local\",\n \t\t\t\t ref->name, sha1_to_hex(ref->old_sha1));\n+\t\t\t\tret = -2;\n \t\t\t\tcontinue;\n \t\t\t}\n \n@@ -245,12 +247,14 @@ static int send_pack(int in, int out, in\n \t\t\t\terror(\"remote ref '%s' is not a strict \"\n \t\t\t\t \"subset of local ref '%s'.\", ref->name,\n \t\t\t\t ref->peer_ref->name);\n+\t\t\t\tret = -2;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n \t\tmemcpy(ref->new_sha1, ref->peer_ref->new_sha1, 20);\n \t\tif (is_zero_sha1(ref->new_sha1)) {\n \t\t\terror(\"cannot happen anymore\");\n+\t\t\tret = -3;\n \t\t\tcontinue;\n \t\t}\n \t\tnew_refs++;\n@@ -267,7 +271,7 @@ static int send_pack(int in, int out, in\n \tif (new_refs)\n \t\tpack_objects(out, remote_refs);\n \tclose(out);\n-\treturn 0;\n+\treturn ret;\n }\n \n \n"},{"id":"13602","messageId":"7vbqzk5tx1.fsf@assigned-by-dhcp.cox.net","threadId":"2827","inReplyTo":"20051214004540.15040.93518.stgit@machine.or.cz","subject":"Re: [PATCH] Make git-send-pack exit with error when some refs couldn't be pushed out","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-12-14T01:50:02Z","receivedAt":"2005-12-14T01:50:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Petr Baudis writes:\n\n> In case some refs couldn't be pushed out due to an error (mostly the\n> not-a-proper-subset error), make git-send-pack exit with non-zero status\n> after the push is over (that is, it still tries to push out the rest\n> of the refs).\n\nI agree this change to send-pack is a good fix (I haven't looked\nat the code, but the above description describes a good fix).\n\nHowever, I am not quite sure if the recent change to Cogito to\nupdate the mirror of remote branch is a good one. Since I am\nnot a Cogito user, I did not question it when I saw the change,\nbut I wondered what you are using the cached \"remote ref is\nsupposed to be pointing at this commit\" knowledge for. If it is\nonly \"pretending to have fetched the remote branch immediately\nafter we pushed into it before anybody else touched the same\nremote branch\", then it probably is benign. But this bit\npuzzles and worries me:\n\n> ... otherwise\n> it gets out of sync, which can lead even to loss of commits on the local\n> side (this happenned to Jonas Fonseca - thanks for the report, BTW).\n\nI do not remember seeing that report so I do not know how that\nlossage happens with unreleased Cogito, but I suspect there is\nsomething very fishy going on here. I presume it happens when\nthe next fetch/pull from the remote is run, but if the locally\ncached information can affect the next fetch/pull in such a way,\nwouldn't updates by other people to the shared remote repository\nbranch also cause things to go out-of-sync and cause the same\nbreakage?\n"},{"id":"13606","messageId":"20051214023005.GS10680@pasky.or.cz","threadId":"2827","inReplyTo":"7vbqzk5tx1.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Make git-send-pack exit with error when some refs couldn't be pushed out","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2005-12-14T02:30:05Z","receivedAt":"2005-12-14T02:30:05Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Dear diary, on Wed, Dec 14, 2005 at 02:50:02AM CET, I got a letter\nwhere Junio C Hamano said that...\n> However, I am not quite sure if the recent change to Cogito to\n> update the mirror of remote branch is a good one. Since I am\n> not a Cogito user, I did not question it when I saw the change,\n> but I wondered what you are using the cached \"remote ref is\n> supposed to be pointing at this commit\" knowledge for. If it is\n> only \"pretending to have fetched the remote branch immediately\n> after we pushed into it before anybody else touched the same\n> remote branch\", then it probably is benign.\n\nExactly, we're just pretending that. It's useful since cg-fetch's\ndiff-tree output is less confusing then, cg-log -r and\ncg-diff -r ... gives expected results, etc.\n\n> But this bit puzzles and worries me:\n> \n> > ... otherwise\n> > it gets out of sync, which can lead even to loss of commits on the local\n> > side (this happenned to Jonas Fonseca - thanks for the report, BTW).\n> \n> I do not remember seeing that report so I do not know how that\n> lossage happens with unreleased Cogito, but I suspect there is\n> something very fishy going on here. I presume it happens when\n> the next fetch/pull from the remote is run, but if the locally\n> cached information can affect the next fetch/pull in such a way,\n> wouldn't updates by other people to the shared remote repository\n> branch also cause things to go out-of-sync and cause the same\n> breakage?\n\nJonas only reported it on IRC as weird cg-commit behaviour (which it\nappeared to be, but the cause turned out to be this and the nice\nside-effect that this kind of breakage cannot lose you actual data at\nleast, just metadata).\n\nIf the push really happened, this shouldn't be a problem, since then we\nget into a \"fast-forward relation\" to the remote branch, and commits by\nother people don't affect it either, since they don't break this\nrelation. The \"interesting behaviour\" Cogito implements happens only if\nyour current HEAD == , which can't happen when\nother people commit, only when you push it out to the remote head.\n\nWhat kicks in is the fast-forward behaviour change I proposed quite some\ntime ago on the mailing list and got zero feedback about --- when\nupdating from a remote branch, the Cogito 0.17pre's fast-forward test\nis extended from\n\n\tif (head ancestor_of remote_new)\n\nto\n\n\tif (head ancestor_of remote_new OR head == remote_old)\n\nwhich will make fast-forward able to follow even remote rebases\nand should have no negative side-effects as long as remote_old\nwas correct (which it is except when our bug comes out). So in\nthe reported case, remote_old was set to head, while remote_new\nstayed as before, and Cogito therefore thought that the remote\nbranch just got rebased from head to remote_new, and adjusted\nthe head appropriately.\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\nVI has two modes: the one in which it beeps and the one in which\nit doesn't.\n"},{"id":"13619","messageId":"1134576909.2478.53.camel@cashmere.sps.mot.com","threadId":"2827","inReplyTo":"20051214023005.GS10680@pasky.or.cz","subject":"Re: [PATCH] Make git-send-pack exit with error when some refs cou ldn't be pushed out","fromName":"Jon Loeliger","fromEmail":"jdl@freescale.com","sentAt":"2005-12-14T16:15:09Z","receivedAt":"2005-12-14T16:15:09Z","isPatch":true,"sender":{"key":"jdl@jdl.com","avatar":"https://gravatar.com/avatar/75ce9a10b151acd2c28ec4ab2136dba7b2ff1634530bd04b155981a749d08a64?d=mp&s=160"},"body":"On Tue, 2005-12-13 at 20:30, Petr Baudis wrote:\n\n> \n> Jonas only reported it on IRC as weird cg-commit behaviour (which it\n\nGah. Are we IRCing now too these days? Did I miss a step\nor an announcment? I thought we were all doing a reasonably\ngood job of communicating here on list... And if I do need\nto sit-n-watch some IRC channel somewhere to get a complete\ngit-fix, please let me know what it is! :-)\n\nThanks,\njdl\n"},{"id":"13626","messageId":"20051214190146.GI22159@pasky.or.cz","threadId":"2827","inReplyTo":"1134576909.2478.53.camel@cashmere.sps.mot.com","subject":"Re: [PATCH] Make git-send-pack exit with error when some refs cou ldn't be pushed out","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2005-12-14T19:01:46Z","receivedAt":"2005-12-14T19:01:46Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Dear diary, on Wed, Dec 14, 2005 at 05:15:09PM CET, I got a letter\nwhere Jon Loeliger said that...\n> On Tue, 2005-12-13 at 20:30, Petr Baudis wrote:\n> > Jonas only reported it on IRC as weird cg-commit behaviour (which it\n> \n> Gah. Are we IRCing now too these days? Did I miss a step\n> or an announcment?\n\nI mentioned the #git channel on the FreeNode network in several Cogito\nannouncements, but this problem report was actually in a different\nchannel (#elinks, since it occurred during work on ELinks) and it\nwouldn't make a difference if it was on a query or in a private mail.\n\n> I thought we were all doing a reasonably good job of communicating\n> here on list...\n\nSure, all important things are going on here, but it's often much more\nconvenient to do some troubleshooting and stuff over IRC, and way too\nmany people also mail me privately (sometimes it makes sense, sometimes\nthey would be better off cc'ing the list).\n\n#git serves mainly as quick help wrt. GIT usage, sometimes people report\nproblems there (but for non-Cogito problems, I usually redir them to the\nmailing list), and sometimes (but quite rarely) larger feature-wise\ndiscussion spins out; but if it's about to get anything near real, it\nusually hits the mailing list anyway. Oh, and CIA reports Cogito commits\non the channel.\n\nThe only IRC-only effort ever (so far) was I believe adding the\ncg-switch command.\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\nVI has two modes: the one in which it beeps and the one in which\nit doesn't.\n"}]}