{"thread":{"id":"13168","subject":"[PATCH 2/8] Add a lockfile function to append to a file","startedAt":"2008-04-18T00:02:31Z","lastAt":"2008-04-20T19:43:23Z","messageCount":4,"participants":["Daniel Barkalow","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"74702","messageId":"alpine.LNX.1.00.0804171950140.19665@iabervon.org","threadId":"13168","inReplyTo":null,"subject":"[PATCH 2/8] Add a lockfile function to append to a file","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-04-18T00:02:31Z","receivedAt":"2008-04-18T00:02:31Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"This takes care of copying the original contents into the replacement\nfile after the lock is held, so that concurrent additions can't miss\neach other's changes.\n\nSigned-off-by: Daniel Barkalow \n---\nHow about this? Also doesn't leak a fd and catches trying to append to a \nfile you can't read. Should I worry about mmap failing after the open?\n\n cache.h | 1 +\n lockfile.c | 23 +++++++++++++++++++++++\n 2 files changed, 24 insertions(+), 0 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 50b28fa..8d066bf 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -391,6 +391,7 @@ struct lock_file {\n \tchar filename[PATH_MAX];\n };\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n+extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n extern int commit_lock_file(struct lock_file *);\n \n extern int hold_locked_index(struct lock_file *, int);\ndiff --git a/lockfile.c b/lockfile.c\nindex 663f18f..f1b5416 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -160,6 +160,29 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int die_on\n \treturn fd;\n }\n \n+int hold_lock_file_for_append(struct lock_file *lk, const char *path, int die_on_error)\n+{\n+\tint fd = lock_file(lk, path);\n+\tstruct stat st;\n+\tif (fd < 0) {\n+\t\tif (die_on_error)\n+\t\t\tdie(\"unable to create '%s.lock': %s\",\n+\t\t\t path, strerror(errno));\n+\t} else if (!stat(path, &st)) {\n+\t\tint orig_fd = open(path, O_RDONLY);\n+\t\tsize_t mmap_size = xsize_t(st.st_size);\n+\t\tvoid *mmap;\n+\t\tif (orig_fd < 0)\n+\t\t\tdie(\"unable to read %s to append to it\", path);\n+\t\tmmap = xmmap(NULL, mmap_size, PROT_READ, MAP_PRIVATE,\n+\t\t\t\t orig_fd, 0);\n+\t\twrite_or_die(fd, mmap, mmap_size);\n+\t\tmunmap(mmap, mmap_size);\n+\t\tclose(orig_fd);\n+\t}\n+\treturn fd;\n+}\n+\n int close_lock_file(struct lock_file *lk)\n {\n \tint fd = lk->fd;\n-- \n1.5.4.3.610.gea6cd\n"},{"id":"74797","messageId":"7vtzhxce9g.fsf@gitster.siamese.dyndns.org","threadId":"13168","inReplyTo":"alpine.LNX.1.00.0804171950140.19665@iabervon.org","subject":"Re: [PATCH 2/8] Add a lockfile function to append to a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-04-20T04:59:55Z","receivedAt":"2008-04-20T04:59:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow writes:\n\n> This takes care of copying the original contents into the replacement\n> file after the lock is held, so that concurrent additions can't miss\n> each other's changes.\n>\n> Signed-off-by: Daniel Barkalow \n> ---\n> How about this? Also doesn't leak a fd and catches trying to append to a \n> file you can't read. Should I worry about mmap failing after the open?\n\nWe have copy.c to copy small existing files, while detecting failure to\ncopy properly. How about doing something like this instead?\n\n cache.h | 1 +\n lockfile.c | 28 ++++++++++++++++++++++++++++\n 2 files changed, 29 insertions(+), 0 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 50b28fa..8d066bf 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -391,6 +391,7 @@ struct lock_file {\n \tchar filename[PATH_MAX];\n };\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n+extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n extern int commit_lock_file(struct lock_file *);\n \n extern int hold_locked_index(struct lock_file *, int);\ndiff --git a/lockfile.c b/lockfile.c\nindex 663f18f..e9e0095 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -160,6 +160,34 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int die_on\n \treturn fd;\n }\n \n+int hold_lock_file_for_append(struct lock_file *lk, const char *path, int die_on_error)\n+{\n+\tint fd, orig_fd;\n+\n+\tfd = lock_file(lk, path);\n+\tif (fd < 0) {\n+\t\tif (die_on_error)\n+\t\t\tdie(\"unable to create '%s.lock': %s\", path, strerror(errno));\n+\t\treturn fd;\n+\t}\n+\n+\torig_fd = open(path, O_RDONLY);\n+\tif (orig_fd < 0) {\n+\t\tif (errno != ENOENT) {\n+\t\t\tif (die_on_error)\n+\t\t\t\tdie(\"cannot open '%s' for copying\", path);\n+\t\t\tclose(fd);\n+\t\t\treturn error(\"cannot open '%s' for copying\", path);\n+\t\t}\n+\t} else if (copy_fd(orig_fd, fd)) {\n+\t\tif (die_on_error)\n+\t\t\texit(128);\n+\t\tclose(fd);\n+\t\treturn -1;\n+\t}\n+\treturn fd;\n+}\n+\n int close_lock_file(struct lock_file *lk)\n {\n \tint fd = lk->fd;\n"},{"id":"74809","messageId":"alpine.DEB.1.00.0804201714230.5084@eeepc-johanness","threadId":"13168","inReplyTo":"7vtzhxce9g.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/8] Add a lockfile function to append to a file","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-04-20T16:14:36Z","receivedAt":"2008-04-20T16:14:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 19 Apr 2008, Junio C Hamano wrote:\n\n> Daniel Barkalow writes:\n> \n> > This takes care of copying the original contents into the replacement\n> > file after the lock is held, so that concurrent additions can't miss\n> > each other's changes.\n> >\n> > Signed-off-by: Daniel Barkalow \n> > ---\n> > How about this? Also doesn't leak a fd and catches trying to append to a \n> > file you can't read. Should I worry about mmap failing after the open?\n> \n> We have copy.c to copy small existing files, while detecting failure to\n> copy properly. How about doing something like this instead?\n\nI like it.\n\nCiao,\nDscho\n"},{"id":"74828","messageId":"alpine.LNX.1.00.0804201528350.19665@iabervon.org","threadId":"13168","inReplyTo":"7vtzhxce9g.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/8] Add a lockfile function to append to a file","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-04-20T19:43:23Z","receivedAt":"2008-04-20T19:43:23Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 19 Apr 2008, Junio C Hamano wrote:\n\n> Daniel Barkalow writes:\n> \n> > This takes care of copying the original contents into the replacement\n> > file after the lock is held, so that concurrent additions can't miss\n> > each other's changes.\n> >\n> > Signed-off-by: Daniel Barkalow \n> > ---\n> > How about this? Also doesn't leak a fd and catches trying to append to a \n> > file you can't read. Should I worry about mmap failing after the open?\n> \n> We have copy.c to copy small existing files, while detecting failure to\n> copy properly. How about doing something like this instead?\n\nYeah, that's obviously the right thing to use.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}