{"thread":{"id":"13960","subject":"[Junio C Hamano] Re: Consolidate SHA1 object file close","startedAt":"2008-06-14T22:42:33Z","lastAt":"2008-07-04T17:44:57Z","messageCount":6,"participants":["Junio C Hamano","Linus Torvalds","Nicolas Pitre","Pierre Habouzit"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"79888","messageId":"7viqwbfxk6.fsf@gitster.siamese.dyndns.org","threadId":"13960","inReplyTo":null,"subject":"[Junio C Hamano] Re: Consolidate SHA1 object file close","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-14T22:42:33Z","receivedAt":"2008-06-14T22:42:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Somehow the thread went off-list, so I'm diverting it back to the list...\n\n\n\nLinus Torvalds writes:\n\n> On Sat, 14 Jun 2008, Junio C Hamano wrote:\n>> \n>> I've queued this...\n>> \n>> commit 6483925999cde47e5108414ac3f57760394ee2d2\n>> Author: Junio C Hamano \n>> Date: Fri Jun 13 23:00:51 2008 -0700\n>> \n>> sha1_file.c: dead code removal\n>\n> Ok, the following three emails will contain three patches on top of that \n> commit that I think are ready for inclusion. They haven't seen a ton of \n> testing, and they are obviously to very core functionality, so in that \n> sense they are a big \"scary\".\n\nOther than that I had to stop for a few seconds to think at the magic \"20\nbytes longer\", they look scary-but-correct ;-).\n\nI've tagged 1.5.6-rc3 with this (and other fixups), but it appears the\nnetwork between me and k.org is crappy today so I'll stop hacking for the\nrest of the afternoon and spend time on something else. Will push the\nresults out sometime tonight.\n\nThanks.\n\n"},{"id":"79946","messageId":"alpine.LFD.1.10.0806151057100.2949@woody.linux-foundation.org","threadId":"13960","inReplyTo":"7viqwbfxk6.fsf@gitster.siamese.dyndns.org","subject":"Re: [Junio C Hamano] Re: Consolidate SHA1 object file close","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-15T18:08:10Z","receivedAt":"2008-06-15T18:08:10Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 14 Jun 2008, Junio C Hamano wrote:\n>\n> Other than that I had to stop for a few seconds to think at the magic \n> \"20 bytes longer\", they look scary-but-correct ;-).\n\nActually, thinking about it, the +20 was not just fairly arbitrary and \nwould have been better off with a comment, it was also not a very good \nnumber to begin with - it just was the old tempfile pattern rounded up to \nthe next \"even\" number.\n\nThe _correct_ number to use is +39, with a better tempfile pattern.\n\nThat needs a few comments, though. So I added those too.\n\nThis patch is totally unimportant, but it does mean that a really \ntraditional filesystem can now do the final rename in-place, because the \ntemporary file is not just in the same directory as the final one, but ti \nalso has the same length, so old-style filesystems can literally just edit \nthe name in place and mark the buffer dirty.\n\nAnd more modern filesystems will generally end up doing more complex \nthings regardless of what we do, but at least this won't _hurt_ either.\n\nThere is one more funny (or sad) detail to this: the old temporary \nfilename was an interesting size: 14 characters. That happens to be the \ntraditional path component limit on really old UNIX filesystems, so if you \nwere to try to use such a filesystem, you'd always be able to create the \ntemporary file, but then you'd never actually be able to do the final link \nor rename due to the final name being too long.\n\nSo take this or leave it as you want - it's really not very important, I \njust wrote this because I was thinking about what really goes on at a very \nlow level when we do that final atomic rename to create the actual \ndirectory entry. This makes it theoretically just a tiny bit more atomic \non old-fashioned filesystems.\n\n\t\tLinus\n\n---\n sha1_file.c | 21 ++++++++++++++++++---\n 1 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 37bcc54..f9ec069 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2110,17 +2110,31 @@ static inline int directory_size(const char *filename)\n *\n * We want to avoid cross-directory filename renames, because those\n * can have problems on various filesystems (FAT, NFS, Coda).\n+ *\n+ * In order to make the final rename as simple as possible to do\n+ * in-place for traditional filesystems, we make the temporary\n+ * filename have the same size as the final one - 38 characters\n+ * (19 bytes of SHA1 info - the first byte is used for the sub-\n+ * directory fan-out).\n+ *\n+ * The temporary filename pattern is\n+ * - 8 characters of \"tmp_obj_\"\n+ * - 23 characters of the object filename\n+ * - 7 characters of \"_XXXXXX\" for mkstemp().\n */\n static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)\n {\n \tint fd, dirlen = directory_size(filename);\n \n-\tif (dirlen + 20 > bufsiz) {\n+\tif (dirlen + 39 > bufsiz) {\n \t\terrno = ENAMETOOLONG;\n \t\treturn -1;\n \t}\n \tmemcpy(buffer, filename, dirlen);\n-\tstrcpy(buffer + dirlen, \"tmp_obj_XXXXXX\");\n+\tmemcpy(buffer + dirlen, \"tmp_obj_\", 8);\n+\tmemcpy(buffer + dirlen + 8, filename + dirlen, 23);\n+\tmemcpy(buffer + dirlen + 31, \"_XXXXXX\", 8);\n+\t\n \tfd = mkstemp(buffer);\n \tif (fd < 0 && dirlen) {\n \t\t/* Make sure the directory exists */\n@@ -2129,7 +2143,8 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)\n \t\t\treturn -1;\n \n \t\t/* Try again */\n-\t\tstrcpy(buffer + dirlen - 1, \"/tmp_obj_XXXXXX\");\n+\t\tbuffer[dirlen-1] = '/';\n+\t\tmemcpy(buffer + dirlen + 32, \"XXXXXX\", 6);\n \t\tfd = mkstemp(buffer);\n \t}\n \treturn fd;\n"},{"id":"79996","messageId":"alpine.LFD.1.10.0806152108410.23110@xanadu.home","threadId":"13960","inReplyTo":"alpine.LFD.1.10.0806151057100.2949@woody.linux-foundation.org","subject":"Re: [Junio C Hamano] Re: Consolidate SHA1 object file close","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-06-16T01:13:41Z","receivedAt":"2008-06-16T01:13:41Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 15 Jun 2008, Linus Torvalds wrote:\n\n> \n> \n> On Sat, 14 Jun 2008, Junio C Hamano wrote:\n> >\n> > Other than that I had to stop for a few seconds to think at the magic \n> > \"20 bytes longer\", they look scary-but-correct ;-).\n\nBTW, isn't the same concerns valid for temporary pack file creation too?\nOr maybe we just don't care in the context of packs?\n\nAlso... this change is lacking its counterpart in \nbuiltin-prune.c:remove_temporary_files().\n\n\nNicolas\n"},{"id":"80099","messageId":"7v4p7s8xjc.fsf@gitster.siamese.dyndns.org","threadId":"13960","inReplyTo":"alpine.LFD.1.10.0806151057100.2949@woody.linux-foundation.org","subject":"Re: [Junio C Hamano] Re: Consolidate SHA1 object file close","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-17T05:01:43Z","receivedAt":"2008-06-17T05:01:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> Actually, thinking about it, the +20 was not just fairly arbitrary and \n> would have been better off with a comment, it was also not a very good \n> number to begin with - it just was the old tempfile pattern rounded up to \n> the next \"even\" number.\n>\n> The _correct_ number to use is +39, with a better tempfile pattern.\n>\n> That needs a few comments, though. So I added those too.\n>\n> This patch is totally unimportant, but it does mean that a really \n> traditional filesystem can now do the final rename in-place, because the \n> temporary file is not just in the same directory as the final one, but ti \n> also has the same length, so old-style filesystems can literally just edit \n> the name in place and mark the buffer dirty.\n\nThat's interesting and somewhat amusing ;-)\n\n> So take this or leave it as you want - it's really not very important, I \n> just wrote this because I was thinking about what really goes on at a very \n> low level when we do that final atomic rename to create the actual \n> directory entry. This makes it theoretically just a tiny bit more atomic \n> on old-fashioned filesystems.\n\nHowever, this may be more important fix. We want to make sure that\nadjust_shared_perm() is called on the success codepath, especially not\nwhen mkdir() does _not_ fail.\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 500584b..e300562 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2125,7 +2125,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)\n \tif (fd < 0 && dirlen) {\n \t\t/* Make sure the directory exists */\n \t\tbuffer[dirlen-1] = 0;\n-\t\tif (mkdir(buffer, 0777) && adjust_shared_perm(buffer))\n+\t\tif (mkdir(buffer, 0777) || adjust_shared_perm(buffer))\n \t\t\treturn -1;\n \n \t\t/* Try again */\n"},{"id":"80150","messageId":"alpine.LFD.1.10.0806170919370.25099@woody.linux-foundation.org","threadId":"13960","inReplyTo":"7v4p7s8xjc.fsf@gitster.siamese.dyndns.org","subject":"Re: [Junio C Hamano] Re: Consolidate SHA1 object file close","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-17T16:30:54Z","receivedAt":"2008-06-17T16:30:54Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 16 Jun 2008, Junio C Hamano wrote:\n> \n> However, this may be more important fix. We want to make sure that\n> adjust_shared_perm() is called on the success codepath, especially not\n> when mkdir() does _not_ fail.\n\nOops. Yes.\n\nThe old code this came from actually did it differently: it failed if \nmkdir _succeeded_ but then adjust_shared_perm() failed. If the mkdir \nfailed, it would just try again. That was what confused me - it was a \ncopy-and-paste thing, but with me misreading the source of it and being \nconfused.\n\nIOW, the old code looked like\n\n\tif (!mkdir(filename, 0777) && adjust_shared_perm(filename)) {\n\t\t.. failure path ..\n\nwhich is just really odd. A failing mkdir would be a \"success\" from this \nstandpoint, leading to another \"link()\" being attempted. I think it may \nhave wanted to return the errno from the link() or something.\n\nYour fix is obviously superior. And equally obviously we don't seem to \nhave any tests for this case. Maybe because I have a umask of 0002, so the \nmkdir() would already make it group-writable?\n\n\t\tLinus\n"},{"id":"82242","messageId":"20080704174457.GA7955@artemis.madism.org","threadId":"13960","inReplyTo":"7viqwbfxk6.fsf@gitster.siamese.dyndns.org","subject":"Re: [Junio C Hamano] Re: Consolidate SHA1 object file close","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-07-04T17:44:57Z","receivedAt":"2008-07-04T17:44:57Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Sat, Jun 14, 2008 at 10:42:33PM +0000, Junio C Hamano wrote:\n> Somehow the thread went off-list, so I'm diverting it back to the list...\n> \n\n FWIW, the person with that patch here do not have the issue with NFS\nat all, whereas people with older git versions have had the issue. IOW,\nI consider my issue with NFS fixed by this patch.\n\n Thanks a lot guys \\o/\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"}]}