{"thread":{"id":"43282","subject":"Re: Problem with git-push","startedAt":"2006-10-31T14:48:18Z","lastAt":"2006-11-03T02:40:35Z","messageCount":12,"participants":["Junio C Hamano","Johannes Schindelin","Florent Thoumie"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"294539","messageId":"1162306098.41547.4.camel@mayday.esat.net","threadId":"43282","inReplyTo":null,"subject":"Problem with git-push","fromName":"Florent Thoumie","fromEmail":"flz@xbsd.org","sentAt":"2006-10-31T14:48:18Z","receivedAt":"2006-10-31T14:48:18Z","isPatch":false,"sender":{"key":"flz@xbsd.org","avatar":"https://gravatar.com/avatar/1ffcfcd468e2c17dce55b01ec37db4483e2ff79a0916256233a213a2f81e2332?d=mp&s=160"},"body":"Hi list,\n\nI'm having some weird issues with git-push lately (1.4.1 on both ends):\n\n: flz@cream:/tinderbox/portstrees/xorg-modular/ports; git push\nupdating 'refs/heads/xorg'\n from 6d1e4fce0a22da799175ec8e168dc0767f1131ef\n to 496cbd4c75220c204aac4fd1ef9912e40e0314b2\nGenerating pack...\nDone counting 16 objects.\nResult has 11 objects.\nDeltifying 11 objects.\n 100% (11/11) done\nTotal 11, written 11 (delta 4), reused 0 (delta 0)\nUnpacking 11 objects\nunpack unpacker exited with error code\nng refs/heads/xorg n/a (unpacker error)\nunable to write sha1 filename ./objects/2d: Is a directory\nfatal: failed to write object\n: flz@cream:/tinderbox/portstrees/xorg-modular/ports; git push\nupdating 'refs/heads/xorg'\n from 6d1e4fce0a22da799175ec8e168dc0767f1131ef\n to 496cbd4c75220c204aac4fd1ef9912e40e0314b2\nGenerating pack...\nDone counting 16 objects.\nResult has 11 objects.\nDeltifying 11 objects.\n 100% (11/11) done\nTotal 11, written 11 (delta 4), reused 0 (delta 0)\nUnpacking 11 objects\nunpack unpacker exited with error code\nng refs/heads/xorg n/a (unpacker error)\nunable to write sha1 filename ./objects/d7: Is a directory\nfatal: failed to write object\n: flz@cream:/tinderbox/portstrees/xorg-modular/ports; git push\nupdating 'refs/heads/xorg'\n from 6d1e4fce0a22da799175ec8e168dc0767f1131ef\n to 496cbd4c75220c204aac4fd1ef9912e40e0314b2\nGenerating pack...\nDone counting 16 objects.\nResult has 11 objects.\nDeltifying 11 objects.\n 100% (11/11) done\nTotal 11, written 11 (delta 4), reused 0 (delta 0)\nUnpacking 11 objects\nrefs/heads/xorg: 6d1e4fce0a22da799175ec8e168dc0767f1131ef ->\n496cbd4c75220c204aac4fd1ef9912e40e0314b2\n\nAny idea what that could be?\n\nCheers.\n\nFlorent\n"},{"id":"294216","messageId":"7v64e0qclo.fsf@assigned-by-dhcp.cox.net","threadId":"43282","inReplyTo":"1162306098.41547.4.camel@mayday.esat.net","subject":"Re: Problem with git-push","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-31T23:39:31Z","receivedAt":"2006-10-31T23:39:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Florent Thoumie writes:\n\n> I'm having some weird issues with git-push lately (1.4.1 on both ends):\n\nBear with some basic questions.\n\n - \"lately\" means this used to work with earlier git on the same\n machine with same configurations?\n\n - the three runs seem to be pushing the same ref to the same\n repository. Did you want to illustrate that it fails upon\n writing out different objects and eventually succeeds?\n\n - what operating system and what filesystem?\n\n - are you working with a shared repository?\n\n> : flz@cream:/tinderbox/portstrees/xorg-modular/ports; git push\n> updating 'refs/heads/xorg'\n> from 6d1e4fce0a22da799175ec8e168dc0767f1131ef\n> to 496cbd4c75220c204aac4fd1ef9912e40e0314b2\n> Generating pack...\n> Done counting 16 objects.\n> Result has 11 objects.\n> Deltifying 11 objects.\n> 100% (11/11) done\n> Total 11, written 11 (delta 4), reused 0 (delta 0)\n> Unpacking 11 objects\n> unpack unpacker exited with error code\n> ng refs/heads/xorg n/a (unpacker error)\n> unable to write sha1 filename ./objects/2d: Is a directory\n> fatal: failed to write object\n\nThis message comes from sha1_file.c::move_temp_to_file(). The\nunpacker wrote the object whose name begins with 2d in a\ntemporary file .git/obj_XXXXXX by calling write_sha1_file(),\nwhich in turn calls move_temp_to_file() to move it to its final\ndestination \".git/objects/2d/XXXXXX\".\n\nmove_temp_to_file() first tries to make a hardlink in\n.git/objects/2d/ directory, assuming that most of the time you\nalready have it. If it succeeds, then it runs unlink() on the\ntemporary file \".git/obj_XXXXXX\". But taht is not what is\nhappening.\n\nlink_temp_to_file() function first tries to make a hardlink with\nlink(\".git/obj_XXXXXX\", \".git/objects/2d/XXXXXX\"). This call\nmust be failing for you. Then it tries to create the leading\ndirectory \".git/objects/2d\", and does adjust_shared_perm and\nwhen it fails it returns (but forgets to restore the slash it\nremoved earlier -- a patch to fix this is attached at the end).\n\nThis explains why the final filename is truncated at the last\nslash, hence ./objects/2d in the error message. Also it\nexplains why repeated tries would succeed; the first run creates\n.git/objects/2d/ directory so the second run will try hardlink\nand will succeed.\n\nSo the next thing to see is why adjust_shared_perm() is failing\nfor you.\n\nOops.\n\nI think this function is broken at the concept level. This is\nto work around people's broken umask (when you are working with\nother people, your umask should not be stronger than 007) and\ntries to flip group writability bit on.\n\nFirst of all, if the necessary bits are already on, it is not\nnecessary to run chmod(), but more importantly, if the directory\nin question was created by somebody else, I do not think this\nchmod() would succeed even if you are in the same group as the\nowner (i.e. previous creator) of the directory.\n\n[jc: I am CC'ing Johannes to blame him on commit 457f06d6;\ngit-pickaxe is turning out to be quite handy ;-)\n\n\tgit pickaxe -C -f L246,277 v1.4.1 -- path.c\n\n]\n\nA quick workaround until we sort this out would be:\n\n - make sure all your developers have umask suitable for group\n work (i.e. second octal-digit from the right should be zero\n to give group members the same rights as you do).\n\n - run \"chmod g+w .git/objects/??\" in the shared repository.\n\n-- >8 --\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex e89d24c..5707069 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1400,8 +1400,10 @@ static int link_temp_to_file(const char\n \tif (dir) {\n \t\t*dir = 0;\n \t\tmkdir(filename, 0777);\n-\t\tif (adjust_shared_perm(filename))\n+\t\tif (adjust_shared_perm(filename)) {\n+\t\t\t*dir = '/';\n \t\t\treturn -2;\n+\t\t}\n \t\t*dir = '/';\n \t\tif (!link(tmpfile, filename))\n \t\t\treturn 0;\n"},{"id":"297184","messageId":"Pine.LNX.4.63.0611011205340.1670@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"43282","inReplyTo":"7v64e0qclo.fsf@assigned-by-dhcp.cox.net","subject":"Re: Problem with git-push","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-11-01T11:12:43Z","receivedAt":"2006-11-01T11:12:43Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 31 Oct 2006, Junio C Hamano wrote:\n\n> So the next thing to see is why adjust_shared_perm() is failing\n> for you.\n> \n> Oops.\n> \n> I think this function is broken at the concept level. This is\n> to work around people's broken umask (when you are working with\n> other people, your umask should not be stronger than 007) and\n> tries to flip group writability bit on.\n> \n> First of all, if the necessary bits are already on, it is not\n> necessary to run chmod(),\n\nright\n\n> but more importantly, if the directory in question was created by \n> somebody else, I do not think this chmod() would succeed even if you are \n> in the same group as the owner (i.e. previous creator) of the directory.\n\nBut if somebody else created it, it should already have the correct \npermissions in the first place (unless the user played around with them, \nwhich is not a pilot error, but a willfull pointing of the barrel in the \ngeneral direction of your knee).\n\n> [jc: I am CC'ing Johannes to blame him on commit 457f06d6;\n> git-pickaxe is turning out to be quite handy ;-)\n\nI am hating the tool already.\n\n> \tif (dir) {\n> \t\t*dir = 0;\n> \t\tmkdir(filename, 0777);\n> -\t\tif (adjust_shared_perm(filename))\n> +\t\tif (adjust_shared_perm(filename)) {\n> +\t\t\t*dir = '/';\n> \t\t\treturn -2;\n> +\t\t}\n\nHow about this instead:\n\n-\t\tmkdir(filename, 0777);\n-\t\tif (adjust_shared_perm(filename))\n+\t\tif (!mkdir(filename, 0777) && adjust_shared_perm(filename)) {\n+\t\t\t*dir = '/';\n \t\t\treturn -2;\n+\t\t}\n\nCiao,\nDscho\n"},{"id":"295866","messageId":"7v4ptj5ghj.fsf@assigned-by-dhcp.cox.net","threadId":"43282","inReplyTo":"Pine.LNX.4.63.0611011205340.1670@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: Problem with git-push","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-01T15:33:28Z","receivedAt":"2006-11-01T15:33:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> but more importantly, if the directory in question was created by \n>> somebody else, I do not think this chmod() would succeed even if you are \n>> in the same group as the owner (i.e. previous creator) of the directory.\n>\n> But if somebody else created it, it should already have the correct \n> permissions in the first place (unless the user played around with them, \n> which is not a pilot error, but a willfull pointing of the barrel in the \n> general direction of your knee).\n\nTrue; I think the yesterday's analysis is still incomplete. I\nhaven't reached the point where I can explain \"is a directory\".\nIf the directory was there and mkdir() failed (but we do not\ncheck its return value), it would have set errno to EEXIST not\nto EISDIR. There is something else going on.\n\n>> [jc: I am CC'ing Johannes to blame him on commit 457f06d6;\n>> git-pickaxe is turning out to be quite handy ;-)\n>\n> I am hating the tool already.\n\nWell, we could rename it to \"git credit\" ;-).\n"},{"id":"295329","messageId":"7vk62f2l46.fsf@assigned-by-dhcp.cox.net","threadId":"43282","inReplyTo":"Pine.LNX.4.63.0611011205340.1670@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: Problem with git-push","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-01T16:21:45Z","receivedAt":"2006-11-01T16:21:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> How about this instead:\n>\n> -\t\tmkdir(filename, 0777);\n> -\t\tif (adjust_shared_perm(filename))\n> +\t\tif (!mkdir(filename, 0777) && adjust_shared_perm(filename)) {\n> +\t\t\t*dir = '/';\n> \t\t\treturn -2;\n> +\t\t}\n\nNot really. See the comment above the code you just touched.\n"},{"id":"294502","messageId":"1162411035.93716.14.camel@localhost","threadId":"43282","inReplyTo":"7v64e0qclo.fsf@assigned-by-dhcp.cox.net","subject":"Re: Problem with git-push","fromName":"Florent Thoumie","fromEmail":"flz@xbsd.org","sentAt":"2006-11-01T19:57:15Z","receivedAt":"2006-11-01T19:57:15Z","isPatch":false,"sender":{"key":"flz@xbsd.org","avatar":"https://gravatar.com/avatar/1ffcfcd468e2c17dce55b01ec37db4483e2ff79a0916256233a213a2f81e2332?d=mp&s=160"},"body":"On Tue, 2006-10-31 at 15:39 -0800, Junio C Hamano wrote:\n\n> A quick workaround until we sort this out would be:\n> \n> - make sure all your developers have umask suitable for group\n> work (i.e. second octal-digit from the right should be zero\n> to give group members the same rights as you do).\n> \n> - run \"chmod g+w .git/objects/??\" in the shared repository.\n\nI thought I checked on the remote (guess I only looked at my local\nrepo), but that was it, wrong perms.\n\nSorry if this is a dumb question but assuming i did 3 successive\ngit-push's, why is the object name changing from a try to another?\n\nThanks for the answer.\n\nFlorent\n\n"},{"id":"296929","messageId":"7v8xiu3ksl.fsf@assigned-by-dhcp.cox.net","threadId":"43282","inReplyTo":"7v4ptj5ghj.fsf@assigned-by-dhcp.cox.net","subject":"Re: Problem with git-push","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-01T21:43:22Z","receivedAt":"2006-11-01T21:43:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>>> but more importantly, if the directory in question was created by \n>>> somebody else, I do not think this chmod() would succeed even if you are \n>>> in the same group as the owner (i.e. previous creator) of the directory.\n>>\n>> But if somebody else created it, it should already have the correct \n>> permissions in the first place (unless the user played around with them, \n>> which is not a pilot error, but a willfull pointing of the barrel in the \n>> general direction of your knee).\n>\n> True; I think the yesterday's analysis is still incomplete. I\n> haven't reached the point where I can explain \"is a directory\".\n> If the directory was there and mkdir() failed (but we do not\n> check its return value), it would have set errno to EEXIST not\n> to EISDIR. There is something else going on.\n\nActually, Florent's said the directory permission was screwed up\nto begin with, so after following the code a bit more I can see\nwhy it said \"is a directory\".\n\nmkdir() may have failed because somebody else had a directory\nthere, perhaps with wrong permission; link_temp_to_file()\ncorrectly fails from adjust_shared_perm() (because the directory\nhad incorrect permission that it cannot fix), returns -2 with\ntruncated filename \"object/2d\", back to move_temp_to_file(), and\n-2 is not EEXIST so it tries to rename the temporary file to\nthat directory, which would also fail and sets ret to errno\n(which now is EISDIR). After unlinking the temporary file, we\nnotice that ret is EISDIR and reports the failure.\n\nSo there is not much more to explain, other than why mkdir()\nfailed in the first place. Previous driver error?\n\nI think idempotent chmod() on somebody else's file or directory\nwould fail, so this may be a safer addition to the codepath in\nquestion.\n\ndiff --git a/path.c b/path.c\nindex bb89fb0..b82f194 100644\n--- a/path.c\n+++ b/path.c\n@@ -279,7 +279,7 @@ int adjust_shared_perm(const char *path)\n \t\t\t : 0));\n \tif (S_ISDIR(mode))\n \t\tmode |= S_ISGID;\n-\tif (chmod(path, mode) < 0)\n+\tif (mode != st.st_mode && chmod(path, mode) < 0)\n \t\treturn -2;\n \treturn 0;\n }\n\n"},{"id":"294827","messageId":"7virhy24xc.fsf@assigned-by-dhcp.cox.net","threadId":"43282","inReplyTo":"1162421693.93716.22.camel@localhost","subject":"Re: Problem with git-push","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-01T22:11:27Z","receivedAt":"2006-11-01T22:11:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Florent Thoumie writes:\n\n> On Wed, 2006-11-01 at 13:43 -0800, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>> \n>> Actually, Florent's said the directory permission was screwed up\n>> to begin with, so after following the code a bit more I can see\n>> why it said \"is a directory\".\n>\n> I screwed the perms. I have a cron job that does automatic imports and\n> it's running as root. I've been lured by the fact that it uses my name\n> and email address to do those imports, so I thought it was running under\n> my unprivileged account.\n\nThanks for a honest clarification. I was starting to worry if\nthis is a BSD vs Linux filesystem gotcha that is harder to debug\nwithout having an actual box.\n\n\n"},{"id":"296163","messageId":"1162421693.93716.22.camel@localhost","threadId":"43282","inReplyTo":"7v8xiu3ksl.fsf@assigned-by-dhcp.cox.net","subject":"Re: Problem with git-push","fromName":"Florent Thoumie","fromEmail":"flz@xbsd.org","sentAt":"2006-11-01T22:54:53Z","receivedAt":"2006-11-01T22:54:53Z","isPatch":false,"sender":{"key":"flz@xbsd.org","avatar":"https://gravatar.com/avatar/1ffcfcd468e2c17dce55b01ec37db4483e2ff79a0916256233a213a2f81e2332?d=mp&s=160"},"body":"On Wed, 2006-11-01 at 13:43 -0800, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Johannes Schindelin writes:\n> >\n> >>> but more importantly, if the directory in question was created by \n> >>> somebody else, I do not think this chmod() would succeed even if you are \n> >>> in the same group as the owner (i.e. previous creator) of the directory.\n> >>\n> >> But if somebody else created it, it should already have the correct \n> >> permissions in the first place (unless the user played around with them, \n> >> which is not a pilot error, but a willfull pointing of the barrel in the \n> >> general direction of your knee).\n> >\n> > True; I think the yesterday's analysis is still incomplete. I\n> > haven't reached the point where I can explain \"is a directory\".\n> > If the directory was there and mkdir() failed (but we do not\n> > check its return value), it would have set errno to EEXIST not\n> > to EISDIR. There is something else going on.\n> \n> Actually, Florent's said the directory permission was screwed up\n> to begin with, so after following the code a bit more I can see\n> why it said \"is a directory\".\n\nI screwed the perms. I have a cron job that does automatic imports and\nit's running as root. I've been lured by the fact that it uses my name\nand email address to do those imports, so I thought it was running under\nmy unprivileged account.\n\nFlorent\n"},{"id":"294346","messageId":"Pine.LNX.4.63.0611021018040.1670@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"43282","inReplyTo":"7vk62f2l46.fsf@assigned-by-dhcp.cox.net","subject":"Re: Problem with git-push","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-11-02T09:29:12Z","receivedAt":"2006-11-02T09:29:12Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 1 Nov 2006, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > How about this instead:\n> >\n> > -\t\tmkdir(filename, 0777);\n> > -\t\tif (adjust_shared_perm(filename))\n> > +\t\tif (!mkdir(filename, 0777) && adjust_shared_perm(filename)) {\n> > +\t\t\t*dir = '/';\n> > \t\t\treturn -2;\n> > +\t\t}\n> \n> Not really. See the comment above the code you just touched.\n\n /*\n * Try to mkdir the last path component if that failed.\n *\n * Re-try the \"link()\" regardless of whether the mkdir\n * succeeds, since a race might mean that somebody\n * else succeeded.\n */\n\nWhat I proposed does not break that. It only means that you only try to \nadjust the permissions if the mkdir succeeded. If somebody else created \nthe directory in the mean time, it is not our job to adjust the \npermissions. Worse, if the other mkdir was run by a different user, the \npermission adjusting _cannot_ succeed, like you pointed out.\n\nSo I think that my patch is correct, but does not matter very much.\n\nThe other thing you asked for: adjust_shared_permissions should only try \nto adjust the permissions only if they are not already correct. Here is my \nattempt to implement that:\n\nTotally untested.\n\nCiao,\nDscho\n\n-- snip --\nadjust_shared_perm(): Avoid unnecessary chmod()s\n\nSighed-off-by: Johannes E. Schindelin \n---\ndiff --git a/path.c b/path.c\nindex bb89fb0..0a2bc51 100644\n--- a/path.c\n+++ b/path.c\n@@ -279,7 +279,7 @@ int adjust_shared_perm(const char *path)\n \t\t\t : 0));\n \tif (S_ISDIR(mode))\n \t\tmode |= S_ISGID;\n-\tif (chmod(path, mode) < 0)\n+\tif ((mode & st.st_mode != mode) && chmod(path, mode) < 0)\n \t\treturn -2;\n \treturn 0;\n }\n"},{"id":"298262","messageId":"Pine.LNX.4.63.0611021041140.1670@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"43282","inReplyTo":"7v8xiu3ksl.fsf@assigned-by-dhcp.cox.net","subject":"Re: Problem with git-push","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-11-02T09:42:46Z","receivedAt":"2006-11-02T09:42:46Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 1 Nov 2006, Junio C Hamano wrote:\n\n> -\tif (chmod(path, mode) < 0)\n> +\tif (mode != st.st_mode && chmod(path, mode) < 0)\n\nOops. Somehow I missed this mail before replying. I'd like to point out \nthat it's safer to check for (mode & st.st_mode != mode), since we should \nleave broader permissions as are.\n\nCiao,\nDscho\n"},{"id":"294724","messageId":"7vbqnpl0bg.fsf@assigned-by-dhcp.cox.net","threadId":"43282","inReplyTo":"Pine.LNX.4.63.0611021018040.1670@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: Problem with git-push","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-03T02:40:35Z","receivedAt":"2006-11-03T02:40:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> What I proposed does not break that. It only means that you only try to \n> adjust the permissions if the mkdir succeeded. If somebody else created \n> the directory in the mean time, it is not our job to adjust the \n> permissions.\n> So I think that my patch is correct, but does not matter very much.\n\nOk. I misread the patch. Will apply with a one-liner log.\n\nThanks.\n\n"}]}