{"thread":{"id":"1504","subject":"[RFC][PATCH] Rewriting revs in place in push target repository","startedAt":"2005-08-13T21:47:25Z","lastAt":"2005-09-15T17:16:41Z","messageCount":7,"participants":["Petr Baudis","Linus Torvalds","Junio C Hamano","Chris Wedgwood","Matthias Urlichs"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"7220","messageId":"20050813214725.GM5608@pasky.ji.cz","threadId":"1504","inReplyTo":null,"subject":"[RFC][PATCH] Rewriting revs in place in push target repository","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2005-08-13T21:47:25Z","receivedAt":"2005-08-13T21:47:25Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Rewrite refs in place in receive-pack & friends\n\nWhen updating a ref, it would write a new file with the new ref and\nthen rename it, overwriting the original file. The problem is that\nthis destroys permissions and ownership of the original file, which is\ntroublesome especially in multiuser environment, like the one I live in.\n\nThis might be controversial, but it's a showbreaker for me wrt. pushing\nnow. Some alternative solution barely solving my particular situation\nmight be surely worked out, but this is more general. The question is:\n\n* Does this break atomicity?\n\n\tI think it does not in real setups, since thanks to O_RDWR the\n\tfile should be overwritten only when the write() happens.\n\tCan a 41-byte write() be non-atomic in any real conditions?\n\n* Does this break with full disk/quota?\n\n\tI'm not sure - we are substituting 41 bytes by another 41\n\tbytes; will the system ever be evil enough to truncate the\n\tfile, then decide the user is over his quota and not write\n\tthe new contents?\n\nSigned-off-by: Petr Baudis \n\ndiff --git a/receive-pack.c b/receive-pack.c\n--- a/receive-pack.c\n+++ b/receive-pack.c\n@@ -92,13 +92,7 @@ static int run_update_hook(const char *r\n static int update(const char *name,\n \t\t unsigned char *old_sha1, unsigned char *new_sha1)\n {\n-\tchar new_hex[60], *old_hex, *lock_name;\n-\tint newfd, namelen, written;\n-\n-\tnamelen = strlen(name);\n-\tlock_name = xmalloc(namelen + 10);\n-\tmemcpy(lock_name, name, namelen);\n-\tmemcpy(lock_name + namelen, \".lock\", 6);\n+\tchar new_hex[60], *old_hex;\n \n \tstrcpy(new_hex, sha1_to_hex(new_sha1));\n \told_hex = sha1_to_hex(old_sha1);\n@@ -106,38 +100,38 @@ static int update(const char *name,\n \t\treturn error(\"unpack should have generated %s, \"\n \t\t\t \"but I can't find it!\", new_hex);\n \n-\tsafe_create_leading_directories(lock_name);\n-\n-\tnewfd = open(lock_name, O_CREAT | O_EXCL | O_WRONLY, 0666);\n-\tif (newfd < 0)\n-\t\treturn error(\"unable to create %s (%s)\",\n-\t\t\t lock_name, strerror(errno));\n-\n-\t/* Write the ref with an ending '\\n' */\n-\tnew_hex[40] = '\\n';\n-\tnew_hex[41] = 0;\n-\twritten = write(newfd, new_hex, 41);\n-\t/* Remove the '\\n' again */\n-\tnew_hex[40] = 0;\n-\n-\tclose(newfd);\n-\tif (written != 41) {\n-\t\tunlink(lock_name);\n-\t\treturn error(\"unable to write %s\", lock_name);\n-\t}\n \tif (verify_old_ref(name, old_hex) < 0) {\n-\t\tunlink(lock_name);\n \t\treturn error(\"%s changed during push\", name);\n \t}\n \tif (run_update_hook(name, old_hex, new_hex)) {\n-\t\tunlink(lock_name);\n \t\treturn error(\"hook declined to update %s\\n\", name);\n \t}\n-\telse if (rename(lock_name, name) < 0) {\n-\t\tunlink(lock_name);\n-\t\treturn error(\"unable to replace %s\", name);\n-\t}\n \telse {\n+\t\tchar *name2;\n+\t\tint newfd, written;\n+\n+\t\tname2 = strdup(name);\n+\t\tsafe_create_leading_directories(name2);\n+\t\tfree(name2);\n+\n+\t\tnewfd = open(name, O_CREAT | O_RDWR, 0666);\n+\t\tif (newfd < 0)\n+\t\t\treturn error(\"unable to create %s (%s)\",\n+\t\t\t\t name, strerror(errno));\n+\n+\t\t/* Write the ref with an ending '\\n' */\n+\t\tnew_hex[40] = '\\n';\n+\t\tnew_hex[41] = 0;\n+\t\twritten = write(newfd, new_hex, 41);\n+\t\t/* Remove the '\\n' again */\n+\t\tnew_hex[40] = 0;\n+\n+\t\tclose(newfd);\n+\t\tif (written != 41) {\n+\t\t\tunlink(name);\n+\t\t\treturn error(\"unable to write %s\", name);\n+\t\t}\n+\n \t\tfprintf(stderr, \"%s: %s -> %s\\n\", name, old_hex, new_hex);\n \t\treturn 0;\n \t}\n"},{"id":"7221","messageId":"Pine.LNX.4.58.0508131507260.3553@g5.osdl.org","threadId":"1504","inReplyTo":"20050813214725.GM5608@pasky.ji.cz","subject":"Re: [RFC][PATCH] Rewriting revs in place in push target repository","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-08-13T22:20:36Z","receivedAt":"2005-08-13T22:20:36Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 13 Aug 2005, Petr Baudis wrote:\n> \n> * Does this break atomicity?\n> \n> \tI think it does not in real setups, since thanks to O_RDWR the\n> \tfile should be overwritten only when the write() happens.\n> \tCan a 41-byte write() be non-atomic in any real conditions?\n\nThat's not the problem.\n\nThe problem is that your change means that there is no locking, and you \nnow can have two writers that both update the same file, and they _both_ \nthink that they succeed. They'll both read the old contents, decide that \nit still is the one from before the push, and then they'll both do the \nwrite.\n\nAnd yes, in most (all?) sane filesystems, the end result is that one of \nthem \"wins\", and the end result is a nice 41-byte file. But the problem is \nthat the other write just totally got lost, and the person doing the push \n_thought_ he had updated the thing, but never did.\n\nTo make things worse, with NFS and client-side caching, different clients \nthat look at the tree at around that time can literally see _different_ \nheads winning the race. One of the writers wrote \"first\", and that client \n(and other NFS clients doing a read at that time) will see it succeed. But \nthen the other pusher writes, and now people will see _that_ one succeed.\n\nConfusion reigns.\n\nIn contrast, with the \"create lock-file and rename\" thing, if there is a\nrace, somebody will win, and the loser will hopefully know they lost.\n\n> * Does this break with full disk/quota?\n> \n> \tI'm not sure - we are substituting 41 bytes by another 41\n> \tbytes; will the system ever be evil enough to truncate the\n> \tfile, then decide the user is over his quota and not write\n> \tthe new contents?\n\nProbably not.\n\nBut how about you just try to copy the permission/group of the original\nfile before you do the rename? I assume that if you're depending on \npermissions, it's either a shared group or by having the thing writable by \nothers, so doing a \n\n\tif (!fstat(oldfd, &st)) {\n\t\tfchown(fd, (uid_t) -1, st.st_gid);\n\t\tfchmod(fd, st.st_mode & ALLPERMS);\n\t}\n\t.. do rename here ..\n\nwhich should get you where you want, no?\n\n\t\tLinus\n"},{"id":"7231","messageId":"7vwtmpjq17.fsf@assigned-by-dhcp.cox.net","threadId":"1504","inReplyTo":"20050813214725.GM5608@pasky.ji.cz","subject":"Re: [RFC][PATCH] Rewriting revs in place in push target repository","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-08-14T00:55:16Z","receivedAt":"2005-08-14T00:55:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Petr Baudis writes:\n\n> Rewrite refs in place in receive-pack & friends\n>\n> When updating a ref, it would write a new file with the new ref and\n> then rename it, overwriting the original file. The problem is that\n> this destroys permissions and ownership of the original file, which is\n> troublesome especially in multiuser environment, like the one I live in.\n\nHmph. If a repo is _really_ used multiuser then you should not\nhave to care about ownership. If you can write into a\nrepository for a project (implying that you are a member of that\nproject group), and if your umask is set up correctly (meaning\nit is 002 or looser), and with g+s bit on the directory at the\nrepository root level when it was created, shouldn't your newly\ncreated ref file be also writable by others in that project?\n"},{"id":"7232","messageId":"Pine.LNX.4.58.0508131859190.3553@g5.osdl.org","threadId":"1504","inReplyTo":"7vwtmpjq17.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC][PATCH] Rewriting revs in place in push target repository","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-08-14T02:10:00Z","receivedAt":"2005-08-14T02:10:00Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 13 Aug 2005, Junio C Hamano wrote:\n>\n> Petr Baudis writes: \n> > Rewrite refs in place in receive-pack & friends\n> >\n> > When updating a ref, it would write a new file with the new ref and\n> > then rename it, overwriting the original file. The problem is that\n> > this destroys permissions and ownership of the original file, which is\n> > troublesome especially in multiuser environment, like the one I live in.\n> \n> Hmph. If a repo is _really_ used multiuser then you should not\n> have to care about ownership.\n\nI think Pasky's usage is that different heads are owned by different\ngroups and/or users, and he wants to use the filesystem permissions to\ndetermine who gets to update which branch. Which is reasonable in a way.\n\nOn the other hand, I don't think filesystem permissions are really very \nuseful. I think it's more appropriate to use triggers to say something \nlike \"only allow people in the 'xyz' group to write to this head\".\n\nObviously, triggers aren't about _security_ - somebody who has write \npermissions to the tree can always screw up others. But triggers are fine \nfor things like branch ownership, where you trust your users, but you just \nwant to avoid mistakes.\n\nSo a trigger might be something like\n\n\t#!/bin/sh\n\t. git-sh-setup-script\n\tbranch=\"$1\"\n\told=\"$2\"\n\tnew=\"$3\"\n\tif [ -e $GIT_DIR/permissions/$branch ]; then\n\t\tid=$(id -un)\n\t\tgrep -q \"^$id$\" $GIT_DIR/permissions/$branch ||\n\t\t\tdie \"You're not allowed to write to $branch\"\n\tfi\n\ttrue\n\nand that would allow you to list all users that are allowed to write to \nthe branch in $GIT_DIR/permissions/.\n\nTotally untested, of course. But the concept should work.\n\n\t\tLinus\n"},{"id":"7233","messageId":"20050814022011.GA19897@taniwha.stupidest.org","threadId":"1504","inReplyTo":"20050813214725.GM5608@pasky.ji.cz","subject":"Re: [RFC][PATCH] Rewriting revs in place in push target repository","fromName":"Chris Wedgwood","fromEmail":"cw@f00f.org","sentAt":"2005-08-14T02:20:11Z","receivedAt":"2005-08-14T02:20:11Z","isPatch":true,"sender":{"key":"cw@f00f.org","avatar":null},"body":"On Sat, Aug 13, 2005 at 11:47:25PM +0200, Petr Baudis wrote:\n\n> \tI think it does not in real setups, since thanks to O_RDWR the\n> \tfile should be overwritten only when the write() happens.\n> \tCan a 41-byte write() be non-atomic in any real conditions?\n\nyes\n\nif you journal metadata only you can see a file extended w/o having\nthe block flushed\n"},{"id":"7244","messageId":"pan.2005.08.14.10.02.41.120567@smurf.noris.de","threadId":"1504","inReplyTo":"20050814022011.GA19897@taniwha.stupidest.org","subject":"Re: [RFC][PATCH] Rewriting revs in place in push target repository","fromName":"Matthias Urlichs","fromEmail":"smurf@smurf.noris.de","sentAt":"2005-08-14T10:02:42Z","receivedAt":"2005-08-14T10:02:42Z","isPatch":true,"sender":{"key":"matthias@urlichs.de","avatar":"https://gravatar.com/avatar/2708905af227313eba6f2b2ae0f7d0259b5ac5d71baef58fe5a13c699ce0bbf0?d=mp&s=160"},"body":"Hi, Chris Wedgwood wrote:\n\n> On Sat, Aug 13, 2005 at 11:47:25PM +0200, Petr Baudis wrote:\n> \n>> \tI think it does not in real setups, since thanks to O_RDWR the\n>> \tfile should be overwritten only when the write() happens.\n>> \tCan a 41-byte write() be non-atomic in any real conditions?\n> \n> if you journal metadata only you can see a file extended w/o having\n> the block flushed\n\n??? but the file is *not* extended. Also, whether or not a block is\nflushed should only matter if the machine crashes ..?\n\n-- \nMatthias Urlichs | {M:U} IT Design @ m-u-it.de | smurf@smurf.noris.de\nDisclaimer: The quote was selected randomly. Really. | http://smurf.noris.de\n - -\nCONS [from LISP] 1. v. To add a new element to a list.\t2. CONS UP:\n v. To synthesize from smaller pieces: \"to cons up an example\".\n\t\t\t\t-- From the AI Hackers' Dictionary\n"},{"id":"8612","messageId":"20050915171641.GA23259@pasky.or.cz","threadId":"1504","inReplyTo":"7vwtmpjq17.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC][PATCH] Rewriting revs in place in push target repository","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2005-09-15T17:16:41Z","receivedAt":"2005-09-15T17:16:41Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Dear diary, on Sun, Aug 14, 2005 at 02:55:16AM CEST, I got a letter\nwhere Junio C Hamano told me that...\n> Petr Baudis writes:\n> \n> > Rewrite refs in place in receive-pack & friends\n> >\n> > When updating a ref, it would write a new file with the new ref and\n> > then rename it, overwriting the original file. The problem is that\n> > this destroys permissions and ownership of the original file, which is\n> > troublesome especially in multiuser environment, like the one I live in.\n> \n> Hmph. If a repo is _really_ used multiuser then you should not\n> have to care about ownership. If you can write into a\n> repository for a project (implying that you are a member of that\n> project group), and if your umask is set up correctly (meaning\n> it is 002 or looser), and with g+s bit on the directory at the\n> repository root level when it was created, shouldn't your newly\n> created ref file be also writable by others in that project?\n\nHmm, but how do you actually set the umask correctly just for git\npushing? I'm sorry but it doesn't occur to me.\n\nI like Linus' solution, but have no time to do the patch now, so I did\njust a quick dirty fix and added a chmod to hooks/update. ;-)\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\nIf you want the holes in your knowledge showing up try teaching\nsomeone. -- Alan Cox\n"}]}