# Careful object writing.. 19 messages from 2005-05-03 to 2005-05-04. Participants: Linus Torvalds, Chris Wedgwood, Jan Harkes, Daniel Barkalow, H. Peter Anvin, Alex Riesen, Junio C Hamano, Morten Welinder. Thread: https://gitlist.dev/t/461 ## Linus Torvalds, 2005-05-03 19:15 Subject: Careful object writing.. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.58.0505031204030.26698%40ppc970.osdl.org ``` I just pushed out the commit that tries to finally actually write the sha1 objects the right way in a shared object directory environment. I used to be lazy, and just do "O_CREAT | O_EXCL" on the final name, but that obviously is not very nice when it can result in other people seeing objects that haven't been fully finalized yet. So now I do it "right", and create a temporary file in the "top" object directory, and then when it's all done, I do a "link()" to the final place and unlink the original. I also change the permission to 0444 before it gets its final name. Two notes: - because the objects all get created initially in .git/objects rather than in the subdirectory they get moved to, you can't use symlinks to other filesystems for the 256 object subdirectories. The object directory has to be one filesystem (but it doesn't have to be the same one as you actually keep your working ddirectories on, of course) - The upside of this is that filesystem block allocators should do the right thing. Instead of spreading the objects out (because they are in different directories), they should be created together. Anyway, somebody should double-check the thing. It _should_ now work correctly over NFS etc too, and everything should be nice and atomic (and with any half-way decent filesystem, it also means that even if you have a system crash in the middle, you'll never see half-created objects). NOTE NOTE NOTE! I have _not_ updated all the helper stuff that also write objects. So things like "git-http-pull" etc will still write objects directly into the object directory, and that can cause problems with shared usage. Same goes for "write_sha1_from_fd()" that rpull.c uses. I hope somebody will take a look at those issues.. Anyway, at least the really core operations should now really be "thread-safe" in a shared object directory environment. Linus ``` ## Chris Wedgwood, 2005-05-03 19:27 Subject: Re: Careful object writing.. Message-ID: <20050503192753.GA6435@taniwha.stupidest.org> URL: https://gitlist.dev/e/20050503192753.GA6435%40taniwha.stupidest.org In-Reply-To: ``` On Tue, May 03, 2005 at 12:15:08PM -0700, Linus Torvalds wrote: > So now I do it "right", and create a temporary file in the "top" > object directory, and then when it's all done, I do a "link()" to > the final place and unlink the original. how is this better than a single rename? i take it there is something fundamental from clue.101 i slept though here? also, if you are *really* paranoid you want to fsync *before* you do the link/unklink or rename --- which is what MTAs do[1] however, that said it *kills* performance and if it's not critical it's really a terrible idea also, shouldn't HEAD (and similar)[2] be updated with a temporary and a rename too? > I also change the permission to 0444 before it gets its final name. cool > NOTE NOTE NOTE! I have _not_ updated all the helper stuff that also > write objects. i thought this was all common code? if it's not maybe now is the time to change that? [1] yes, i know this depends on the fs used and various things and ext3 should be fine, blah blah blah, but not everyone uses ext3 and quite probably not everyone will use git under Linux [2] i didn't check the code as i'm still using BK in places ``` ## Linus Torvalds, 2005-05-03 19:47 Subject: Re: Careful object writing.. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.58.0505031242330.26698%40ppc970.osdl.org In-Reply-To: <20050503192753.GA6435@taniwha.stupidest.org> ``` On Tue, 3 May 2005, Chris Wedgwood wrote: > > how is this better than a single rename? i take it there is something > fundamental from clue.101 i slept though here? A rename will overwrite any old object, which means that you cannot do any collision checks. In contrast, a "link()" will return EEXIST if somebody else raced with you and created a new object, and you can do collision checks instead of overwriting another persons object. > also, if you are *really* paranoid you want to fsync *before* you do > the link/unklink or rename --- which is what MTAs do[1] Me, I refuse to slow down my habits for old filesystems. You can either fsck, or use a logging filesystem. I don't see anybody not using logging filesystems these days, so.. > also, shouldn't HEAD (and similar)[2] be updated with a temporary and > a rename too? Maybe. Much less important, though. > > NOTE NOTE NOTE! I have _not_ updated all the helper stuff that also > > write objects. > > i thought this was all common code? if it's not maybe now is the time > to change that? It is all common code, except: - things like "fetch from another host" will use rsync/wget/xxx to actually get the files. To those programs, we're not talking about git objects, we're just talking "regular files" - rpull.c has a special different routine to write its objects. I don't use it, so.. Anyway, it should be reasonably easily fixable. Linus ``` ## Chris Wedgwood, 2005-05-03 19:47 Subject: Re: Careful object writing.. Message-ID: <20050503194739.GA7082@taniwha.stupidest.org> URL: https://gitlist.dev/e/20050503194739.GA7082%40taniwha.stupidest.org In-Reply-To: ``` On Tue, May 03, 2005 at 12:47:36PM -0700, Linus Torvalds wrote: > Me, I refuse to slow down my habits for old filesystems. You can > either fsck, or use a logging filesystem. ok, so you're saying everyone use linux ext3 or similar more or less... how about we drop all the objects in one directory then? ``` ## Linus Torvalds, 2005-05-03 19:56 Subject: Re: Careful object writing.. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.58.0505031254550.26698%40ppc970.osdl.org In-Reply-To: <20050503194739.GA7082@taniwha.stupidest.org> ``` On Tue, 3 May 2005, Chris Wedgwood wrote: > > On Tue, May 03, 2005 at 12:47:36PM -0700, Linus Torvalds wrote: > > > Me, I refuse to slow down my habits for old filesystems. You can > > either fsck, or use a logging filesystem. > > ok, so you're saying everyone use linux ext3 or similar more or > less... No. I'm saying - you can use git-fsck-cache - or you can use a logging filesystem. I happen to use both. > how about we drop all the objects in one directory then? I don't even have directory hashing on, and as mentioned, the logging filesystem is _not_ a requirement. It's just a reality for most of us. Linus ``` ## Jan Harkes, 2005-05-03 20:00 Subject: Re: Careful object writing.. Message-ID: <20050503200034.GA16104@delft.aura.cs.cmu.edu> URL: https://gitlist.dev/e/20050503200034.GA16104%40delft.aura.cs.cmu.edu In-Reply-To: ``` On Tue, May 03, 2005 at 12:15:08PM -0700, Linus Torvalds wrote: > I just pushed out the commit that tries to finally actually write the sha1 > objects the right way in a shared object directory environment. > > I used to be lazy, and just do "O_CREAT | O_EXCL" on the final name, but > that obviously is not very nice when it can result in other people seeing > objects that haven't been fully finalized yet. > > So now I do it "right", and create a temporary file in the "top" object > directory, and then when it's all done, I do a "link()" to the final place > and unlink the original. Annoyingly until this commit, git has been just about the perfect SCM system to run on top of Coda. Almost every other SCM can and will get conflicts on it's repository files, which are pretty much impossible to resolve (just try merging two diverging copies of an RCS archive..) But the only conflicts we ever see with git are when two people create the same SHA1 object. And if the contents are in fact identical this conflict will be trivially resolved. I tried to pull in the latest version of your tree, but it doesn't look like this commit has propagated to rsync.kernel.org yet. Hopefully you will accept a small patch (should be < 5 lines) that makes git work nicely when Coda complains about the cross-directory hardlink without affecting the reliability of using link/unlink on normal filesystems. Jan > > I also change the permission to 0444 before it gets its final name. > > Two notes: > > - because the objects all get created initially in .git/objects rather > than in the subdirectory they get moved to, you can't use symlinks > to other filesystems for the 256 object subdirectories. The object > directory has to be one filesystem (but it doesn't have to be the same > one as you actually keep your working ddirectories on, of course) > > - The upside of this is that filesystem block allocators should do the > right thing. Instead of spreading the objects out (because they are in > different directories), they should be created together. > > Anyway, somebody should double-check the thing. It _should_ now work > correctly over NFS etc too, and everything should be nice and atomic (and > with any half-way decent filesystem, it also means that even if you have a > system crash in the middle, you'll never see half-created objects). > > NOTE NOTE NOTE! I have _not_ updated all the helper stuff that also write > objects. So things like "git-http-pull" etc will still write objects > directly into the object directory, and that can cause problems with > shared usage. Same goes for "write_sha1_from_fd()" that rpull.c uses. I > hope somebody will take a look at those issues.. > > Anyway, at least the really core operations should now really be > "thread-safe" in a shared object directory environment. > > Linus > - > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ``` ## Daniel Barkalow, 2005-05-03 20:02 Subject: Re: Careful object writing.. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.21.0505031539480.30848-100000%40iabervon.org In-Reply-To: <20050503192753.GA6435@taniwha.stupidest.org> ``` On Tue, 3 May 2005, Chris Wedgwood wrote: > i thought this was all common code? if it's not maybe now is the time > to change that? The other versions are getting the tagged compressed object along with the intended hash from an external source. They both verify the result as they go (as opposed to the normal case which takes the uncompressed data and finds the hash, and therefore has to be right). The common part is really "open the file" and "close (and place) the file", which weren't previously sufficiently complex to justify sharing the code. -Daniel *This .sig left intentionally blank* ``` ## Linus Torvalds, 2005-05-03 20:11 Subject: Re: Careful object writing.. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.58.0505031306310.26698%40ppc970.osdl.org In-Reply-To: <20050503200034.GA16104@delft.aura.cs.cmu.edu> ``` On Tue, 3 May 2005, Jan Harkes wrote: > > I tried to pull in the latest version of your tree, but it doesn't look > like this commit has propagated to rsync.kernel.org yet. Hopefully you > will accept a small patch (should be < 5 lines) that makes git work > nicely when Coda complains about the cross-directory hardlink without > affecting the reliability of using link/unlink on normal filesystems. What is it that coda wants to do, and is there some portable way to get there? Is it just that you want to stay within the directory? Or is it any link action that is nasty? What makes resolving renames hard when the file contents are the same? Maybe Coda could just do a trivial resolve of that "conflict" too? Linus ``` ## Jan Harkes, 2005-05-03 20:59 Subject: Re: Careful object writing.. Message-ID: <20050503205957.GA25253@delft.aura.cs.cmu.edu> URL: https://gitlist.dev/e/20050503205957.GA25253%40delft.aura.cs.cmu.edu In-Reply-To: ``` On Tue, May 03, 2005 at 01:11:47PM -0700, Linus Torvalds wrote: > On Tue, 3 May 2005, Jan Harkes wrote: > > I tried to pull in the latest version of your tree, but it doesn't look > > like this commit has propagated to rsync.kernel.org yet. Hopefully you > > will accept a small patch (should be < 5 lines) that makes git work > > nicely when Coda complains about the cross-directory hardlink without > > affecting the reliability of using link/unlink on normal filesystems. > > What is it that coda wants to do, and is there some portable way to get > there? Short summary: rc = link(old, new); if (rc == -1 && errno == EXDEV) rc = rename(old, new); On Coda, the cross-directory link fails, the following cross-directory rename will work fine. On a normal filesystem, if the link fails with EXDEV, the rename will fail with the same. Because our cache consistency model is fairly optimistic, we already have to deal with potential problems with a rename removing an unwanted target. So if we are logging write operations, and the link operation did not return EEXISTS, then the rename will be marked as not having removed any target file. If the target did happen to exist on the server by the time we reintegrate the operations we end up with a reintegration conflict. Longer version: When a server performs conflict resolution it happens on a per-directory basis. So any cross-directory operation already a special case. We cannot guarantee which directory will be resolved first, so if it is the destination of a link or rename the object itself might not exist yet. The advantage of a rename operation is that it contains a reference to both the source and the destination directories. If we don't yet know the renamed object we resolve the source first. That creates the object and allows us to complete the rename operation. However with a link we only have a reference to an object and the directory where the link should be added. But again, the object might not yet exist on all servers. At this point things get a bit more complicated because we don't enforce access based on per object UNIX mode bits, but rely on directory ACLs. So we can't just add a reference to an unknown object in the destination directory because until we know where else this object is located, we can't tell if the user actually is allowed to access the object. > Is it just that you want to stay within the directory? Or is it any link > action that is nasty? We do allow links within the same directory, mostly because that often happens in places like /usr/bin and we know that whenever we encounter the link operation in the resolution log, that the object creation has already been processed. We also know that the new link can't give a user any rights he didn't already have. > What makes resolving renames hard when the file contents are the same? Renames mostly work, there are only a few corner cases left. One is where something is moved up in the directory tree and the source directory is then removed. We end up screwing ourselves because we append the childs logs on removal and the create operation ends up behind the rename operation. But that is a dumb implementation problem. Another issue is of course when someone (validly) hardlinks a file in the same directory and then moves one of the links to another directory. We definitely are not a typical filesystem with UNIX semantics, which is why it is unusual to find an application that seems so well suited for disconnected and weakly connected operation. Jan ``` ## Linus Torvalds, 2005-05-03 22:13 Subject: Re: Careful object writing.. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.58.0505031507460.26698%40ppc970.osdl.org In-Reply-To: <20050503205957.GA25253@delft.aura.cs.cmu.edu> ``` On Tue, 3 May 2005, Jan Harkes wrote: > > Short summary: > > rc = link(old, new); > if (rc == -1 && errno == EXDEV) > rc = rename(old, new); Ok, that is safe enough. Will do. > On Coda, the cross-directory link fails, the following cross-directory > rename will work fine. On a normal filesystem, if the link fails with > EXDEV, the rename will fail with the same. Yup. I do suspect that since you handle the rename anyway, you probably could handle the git link/unlink patterns too, but it's easy enough to just do the rename fallback in git itself. The only reason not to use rename in the first place is literally just to be able to check for collisions. Which we don't actually _do_ right now, but I like to be able to do so in theory. Linus ``` ## Linus Torvalds, 2005-05-03 22:37 Subject: Re: Careful object writing.. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.58.0505031531270.26698%40ppc970.osdl.org In-Reply-To: <20050503200034.GA16104@delft.aura.cs.cmu.edu> ``` On Tue, 3 May 2005, Jan Harkes wrote: > > I tried to pull in the latest version of your tree, but it doesn't look > like this commit has propagated to rsync.kernel.org yet. Hmm.. It's still not there a few hours later. I wonder what the mirroring rules are. Or maybe mirroring is just broken right now. Peter? One change introduced by me is that the new objects changed from 0664 (-rw-rw-r--) to (0444) -r--r--r-- due to the object writing rules. Maybe the mirroring decides that such objects shouldn't be mirrored, since they are "private"? Or maybe it's just that Peter has shut down mirroring in preparation for the imminent memory upgrade on master.kernel.org. Linus ``` ## H. Peter Anvin, 2005-05-03 22:40 Subject: Re: Careful object writing.. Message-ID: <4277FDD9.6030502@zytor.com> URL: https://gitlist.dev/e/4277FDD9.6030502%40zytor.com In-Reply-To: ``` Linus Torvalds wrote: > > On Tue, 3 May 2005, Jan Harkes wrote: > >>I tried to pull in the latest version of your tree, but it doesn't look >>like this commit has propagated to rsync.kernel.org yet. > > > Hmm.. It's still not there a few hours later. I wonder what the mirroring > rules are. Or maybe mirroring is just broken right now. Peter? > > One change introduced by me is that the new objects changed from 0664 > (-rw-rw-r--) to (0444) -r--r--r-- due to the object writing rules. Maybe > the mirroring decides that such objects shouldn't be mirrored, since they > are "private"? > > Or maybe it's just that Peter has shut down mirroring in preparation for > the imminent memory upgrade on master.kernel.org. > No, I had stopped the cron job to fix a script bug and forgot to turn it back on. It's pushing now. -hpa ``` ## Alex Riesen, 2005-05-03 23:04 Subject: Re: Careful object writing.. Message-ID: <81b0412b05050316045fa31c2a@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b05050316045fa31c2a%40mail.gmail.com In-Reply-To: ``` On 5/3/05, Linus Torvalds wrote: > I also change the permission to 0444 before it gets its final name. Maybe umask it first? Just in case. ``` ## Linus Torvalds, 2005-05-03 23:22 Subject: Re: Careful object writing.. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.58.0505031618360.26698%40ppc970.osdl.org In-Reply-To: <81b0412b05050316045fa31c2a@mail.gmail.com> ``` On Wed, 4 May 2005, Alex Riesen wrote: > > On 5/3/05, Linus Torvalds wrote: > > I also change the permission to 0444 before it gets its final name. > > Maybe umask it first? Just in case. I considered it, but it's not worth it. If you don't want somebody else to see your objects, you should just disable execute permission on your .git directory. "umask" is actually a fairly nasty interface, since it takes effect on create(), and we do want to fchmod _after_ the create (some filesystems don't like it when you create a non-writable object and then write to it, but more importantly, since we use "mkstemp()" for the temp-file handling, we don't even have control of the initial umask. So to make it (0444 & umask) git would actually have to jump through silly hoops. Without actually buying you anything new, since the access permissions for git objects really are about the _directory_ anyway. Linus ``` ## Junio C Hamano, 2005-05-03 23:22 Subject: Re: Careful object writing.. Message-ID: <7vr7gnsxma.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vr7gnsxma.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <81b0412b05050316045fa31c2a@mail.gmail.com> ``` >>>>> "AR" == Alex Riesen writes: AR> On 5/3/05, Linus Torvalds wrote: >> I also change the permission to 0444 before it gets its final name. AR> Maybe umask it first? Just in case. In general worrying about umask when you see chmod is a good practice, but it probably is not applicable to this particular case. A person with umask 077 should still get 0444 if the SHA1_FILE_DIRECTORY is shared with other people, and if it is not shared, his initial git-init-db would have made it 0700, so it does not matter it the files files underneath it have 0444. ``` ## Alex Riesen, 2005-05-03 23:25 Subject: Re: Careful object writing.. Message-ID: <81b0412b0505031625478de1a5@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b0505031625478de1a5%40mail.gmail.com In-Reply-To: ``` On 5/4/05, Linus Torvalds wrote: > > > I also change the permission to 0444 before it gets its final name. > > Maybe umask it first? Just in case. > > I considered it, but it's not worth it. > > If you don't want somebody else to see your objects, you should just > disable execute permission on your .git directory. ... Of course. It's leaves even more control to the user. Forget I asked :) ``` ## Daniel Barkalow, 2005-05-04 04:07 Subject: [PATCH] Careful object pulling Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.21.0505040004290.30848-100000%40iabervon.org In-Reply-To: ``` This splits out the careful methods for writing a file and placing it in the correct location from the function to write a buffer to the file and makes the various functions used by git-*-pull programs use those functions to write their files. Signed-off-by: Daniel Barkalow Index: cache.h =================================================================== --- 51a882a2dc62e0d3cdc79e0badc61559fb723481/cache.h (mode:100644 sha1:8dd812827604d510038f5d93e3718c43f9d12c30) +++ 4e31436bacfff09ce673665a1061b41e37ffd661/cache.h (mode:100644 sha1:0d7411c3b86a899cee45627997f4bb7ba0df2ea7) @@ -134,6 +134,12 @@ extern void * read_sha1_file(const unsigned char *sha1, char *type, unsigned long *size); extern int write_sha1_file(char *buf, unsigned long len, const char *type, unsigned char *return_sha1); +/* Open a file for writing a sha1 file. */ +extern int open_sha1_tmpfile(char tmpfile[PATH_MAX]); + +/* Put a sha1 file in the correct place. */ +extern int place_sha1_file(char tmpfile[PATH_MAX], const unsigned char *sha1); + extern int check_sha1_signature(unsigned char *sha1, void *buf, unsigned long size, const char *type); /* Read a tree into the cache */ Index: http-pull.c =================================================================== --- 51a882a2dc62e0d3cdc79e0badc61559fb723481/http-pull.c (mode:100644 sha1:f693aba61b4dcb4b738faf1335ec74aa1545e45d) +++ 4e31436bacfff09ce673665a1061b41e37ffd661/http-pull.c (mode:100644 sha1:793d8b9e125c1f164f774e2888d26beaa75e1df0) @@ -52,8 +52,9 @@ char real_sha1[20]; char *url; char *posn; + char tmpname[PATH_MAX]; - local = open(filename, O_WRONLY | O_CREAT | O_EXCL, 0666); + local = open_sha1_tmpfile(tmpname); if (local < 0) return error("Couldn't open %s\n", filename); @@ -88,15 +89,14 @@ inflateEnd(&stream); SHA1_Final(real_sha1, &c); if (zret != Z_STREAM_END) { - unlink(filename); + unlink(tmpname); return error("File %s (%s) corrupt\n", hex, url); } if (memcmp(sha1, real_sha1, 20)) { - unlink(filename); + unlink(tmpname); return error("File %s has bad hash\n", hex); } - - return 0; + return place_sha1_file(tmpname, sha1); } int main(int argc, char **argv) Index: sha1_file.c =================================================================== --- 51a882a2dc62e0d3cdc79e0badc61559fb723481/sha1_file.c (mode:100644 sha1:e6ce455ae90bd430f2128f454bdb6e0575412486) +++ 4e31436bacfff09ce673665a1061b41e37ffd661/sha1_file.c (mode:100644 sha1:85daa0b0045c3f19e697d1a7aa8ab15ff54eab99) @@ -276,6 +276,55 @@ } } +int open_sha1_tmpfile(char tmpfile[PATH_MAX]) +{ + int fd; + + snprintf(tmpfile, sizeof(tmpfile), "%s/obj_XXXXXX", get_object_directory()); + fd = mkstemp(tmpfile); + if (fd < 0) { + fprintf(stderr, + "unable to create temporary sha1 filename %s: %s", + tmpfile, strerror(errno)); + return -1; + } + return fd; +} + +int place_sha1_file(char tmpfile[PATH_MAX], const unsigned char *sha1) +{ + char *filename = sha1_file_name(sha1); + int ret; + + ret = link(tmpfile, filename); + if (ret < 0) { + ret = errno; + + /* + * Coda hack - coda doesn't like cross-directory links, + * so we fall back to a rename, which will mean that it + * won't be able to check collisions, but that's not a + * big deal. + * + * When this succeeds, we just return 0. We have nothing + * left to unlink. + */ + if (ret == EXDEV && !rename(tmpfile, filename)) + return 0; + } + unlink(tmpfile); + if (ret) { + if (ret != EEXIST) { + fprintf(stderr, + "unable to write sha1 filename %s: %s", + filename, strerror(ret)); + return -1; + } + /* FIXME!!! Collision check here ? */ + } + return 0; +} + int write_sha1_file(char *buf, unsigned long len, const char *type, unsigned char *returnsha1) { int size; @@ -286,7 +335,7 @@ char *filename; static char tmpfile[PATH_MAX]; char hdr[50]; - int fd, hdrlen, ret; + int fd, hdrlen; /* Generate the header */ hdrlen = sprintf(hdr, "%s %lu", type, len)+1; @@ -316,12 +365,9 @@ return -1; } - snprintf(tmpfile, sizeof(tmpfile), "%s/obj_XXXXXX", get_object_directory()); - fd = mkstemp(tmpfile); - if (fd < 0) { - fprintf(stderr, "unable to create temporary sha1 filename %s: %s", tmpfile, strerror(errno)); - return -1; - } + fd = open_sha1_tmpfile(tmpfile); + if (fd < 0) + return fd; /* Set it up */ memset(&stream, 0, sizeof(stream)); @@ -349,53 +395,28 @@ if (write(fd, compressed, size) != size) die("unable to write file"); + fchmod(fd, 0444); close(fd); - ret = link(tmpfile, filename); - if (ret < 0) { - ret = errno; - - /* - * Coda hack - coda doesn't like cross-directory links, - * so we fall back to a rename, which will mean that it - * won't be able to check collisions, but that's not a - * big deal. - * - * When this succeeds, we just return 0. We have nothing - * left to unlink. - */ - if (ret == EXDEV && !rename(tmpfile, filename)) - return 0; - } - unlink(tmpfile); - if (ret) { - if (ret != EEXIST) { - fprintf(stderr, "unable to write sha1 filename %s: %s", filename, strerror(ret)); - return -1; - } - /* FIXME!!! Collision check here ? */ - } - - return 0; + return place_sha1_file(tmpfile, sha1); } int write_sha1_from_fd(const unsigned char *sha1, int fd) { - char *filename = sha1_file_name(sha1); - int local; z_stream stream; unsigned char real_sha1[20]; char buf[4096]; char discard[4096]; + char tmpname[PATH_MAX]; int ret; SHA_CTX c; - local = open(filename, O_WRONLY | O_CREAT | O_EXCL, 0666); + local = open_sha1_tmpfile(tmpname); if (local < 0) - return error("Couldn't open %s\n", filename); + return -1; memset(&stream, 0, sizeof(stream)); @@ -408,7 +429,7 @@ size = read(fd, buf, 4096); if (size <= 0) { close(local); - unlink(filename); + unlink(tmpname); if (!size) return error("Connection closed?"); perror("Reading from connection"); @@ -431,15 +452,15 @@ close(local); SHA1_Final(real_sha1, &c); if (ret != Z_STREAM_END) { - unlink(filename); + unlink(tmpname); return error("File %s corrupted", sha1_to_hex(sha1)); } if (memcmp(sha1, real_sha1, 20)) { - unlink(filename); + unlink(tmpname); return error("File %s has bad hash\n", sha1_to_hex(sha1)); } - - return 0; + + return place_sha1_file(tmpname, sha1); } int has_sha1_file(const unsigned char *sha1) ``` ## Morten Welinder, 2005-05-04 09:35 Subject: Re: [PATCH] Careful object pulling Message-ID: <118833cc050504023569e00d38@mail.gmail.com> URL: https://gitlist.dev/e/118833cc050504023569e00d38%40mail.gmail.com In-Reply-To: ``` Something's fishy there. You are comparing the result from link with EEXIST. Morten ``` ## Daniel Barkalow, 2005-05-04 16:16 Subject: Re: [PATCH] Careful object pulling Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.21.0505041207480.30848-100000%40iabervon.org In-Reply-To: <118833cc050504023569e00d38@mail.gmail.com> ``` On Wed, 4 May 2005, Morten Welinder wrote: > Something's fishy there. You are comparing the result from link with EEXIST. No, it just looks that way. If ret is negative, errno gets written to it. If it's zero, we don't do anything with it. It can't be positive. So, at the point where we test it, it must be the errno from link, which would be EEXIST if the case we're worried about. -Daniel *This .sig left intentionally blank* ```