{"thread":{"id":"36333","subject":"[PATCH 01/22] t3204: test deleting references when lock files already exist","startedAt":"2014-04-01T15:58:08Z","lastAt":"2014-04-07T16:37:19Z","messageCount":62,"participants":["Michael Haggerty","Jeff King","Eric Sunshine","Junio C Hamano","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":22},"messages":[{"id":"238223","messageId":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":null,"subject":"[PATCH 00/22] Lockfile refactoring and pre-activation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:08Z","receivedAt":"2014-04-01T15:58:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"I've had this patch series kicking around for a long time, along with\nsome followup patches to fix a race in reference deletion. I haven't\nhad the time to get everything done and tested, but let me at least\npush this first series out there. I especially want to submit it in\ncase we accept a GSoC student for the project \"Refactor tempfile\nhandling\", because (1) I don't want me and the student to be stepping\non each others' toes, and (2) there are some cleanups and\ndocumentation improvements here that will hopefully be useful to the\nstudent.\n\nThe first patch actually demonstrates the race condition that I hope\nto fix someday. The last patch introduces the lockfile feature that I\nthink is needed to fix it: the ability to activate a packed-refs file\nwhile still holding the lock to prevent another process from\noverwriting it before the accompanying loose reference updates are all\nfinished. But the fix itself is not included here, so you might want\nto keep the last patch on hold until there is a concrete user of the\nfeature.\n\nThe rest of the patches hopefully make the lockfile API more\nconsistent and better documented. There were a lot of situations when\na failure in one step of the lock/commit procedure left a lock in an\nill-defined state. That's probably not a problem very often in\npractice, because we tend to die() soon after any locking problems.\nBut it is still helpful for lockfile objects to have a well-defined\nstate diagram (especially if your goal is to add a new feature to\nthem!)\n\nSeveral patches are also devoted to making lock filename handling use\nstrbufs (for the usual reasons), and reducing the need for callers to\nreach into the \"filename\" field and especially for them to use that\nfield as temporary scratch space.\n\nMichael\n\nMichael Haggerty (22):\n t3204: test deleting references when lock files already exist\n try_merge_strategy(): remove redundant lock_file allocation\n rollback_lock_file(): do not clear filename redundantly\n rollback_lock_file(): set fd to -1\n lockfile: unlock file if lockfile permissions cannot be adjusted\n hold_lock_file_for_append(): release lock on errors\n lock_file(): always add lock_file object to lock_file_list\n struct lock_file: replace on_list field with flags field\n api-lockfile: expand the documentation\n lockfile.c: document the various states of lock_file objects\n lockfile: define a constant LOCK_SUFFIX_LEN\n delete_ref_loose(): don't muck around in the lock_file's filename\n config: change write_error() to take a (struct lock_file *) argument\n lockfile: use strbufs when handling (most) paths\n resolve_symlink(): use a strbuf internally\n commit_lock_file(): don't work with a fixed-length buffer\n lock_file(): exit early if lockfile cannot be opened\n lockfile: also keep track of the filename of the file being locked\n struct lock_file: rename lock_filename field to staging_filename\n remove_lock_file(): call rollback_lock_file()\n lockfile: extract a function reset_lock_file()\n lockfile: allow new file contents to be written while retaining lock\n\n Documentation/technical/api-lockfile.txt | 90 +++++--\n builtin/commit.c | 12 +-\n builtin/merge.c | 1 -\n builtin/reflog.c | 2 +-\n cache.h | 9 +-\n config.c | 11 +-\n lockfile.c | 389 +++++++++++++++++++++----------\n refs.c | 8 +-\n shallow.c | 6 +-\n t/t3204-branch-locks.sh | 40 ++++\n 10 files changed, 403 insertions(+), 165 deletions(-)\n create mode 100755 t/t3204-branch-locks.sh\n\n-- \n1.9.0\n"},{"id":"238206","messageId":"1396367910-7299-2-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 01/22] t3204: test deleting references when lock files already exist","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:09Z","receivedAt":"2014-04-01T15:58:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"When deleting a reference, it might be that another process already\nholds the lock on the loose reference file and/or the packed-refs\nfile. In those cases, there is no alternative but for the delete to\nfail. Verify that in such cases the reference values are left\nunchanged.\n\nBut in fact, when the packed-refs file lock cannot be acquired, the\nloose reference file is deleted anyway, potentially leaving the\nreference with a changed value (its packed value, which might even\npoint at an object that has been garbage collected). Thus one of the\nnew tests is marked test_expect_failure.\n\nSigned-off-by: Michael Haggerty \n---\n t/t3204-branch-locks.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 40 insertions(+)\n create mode 100755 t/t3204-branch-locks.sh\n\ndiff --git a/t/t3204-branch-locks.sh b/t/t3204-branch-locks.sh\nnew file mode 100755\nindex 0000000..cc4e2c1\n--- /dev/null\n+++ b/t/t3204-branch-locks.sh\n@@ -0,0 +1,40 @@\n+#!/bin/sh\n+\n+test_description='git branch lock tests'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'prepare a trivial repository' '\n+\tgit commit --allow-empty -m \"Initial\" &&\n+\tgit commit --allow-empty -m \"First\"\n+'\n+\n+test_expect_success 'delete a ref while loose ref file is locked' '\n+\tgit branch b1 master &&\n+\tgit for-each-ref >expected1 &&\n+\t# Simulate another process holding the loose file lock:\n+\t: >.git/refs/heads/b1.lock &&\n+\ttest_must_fail git branch -D b1 &&\n+\trm .git/refs/heads/b1.lock &&\n+\t# Delete failed; reference values should be unchanged:\n+\tgit for-each-ref >actual1 &&\n+\ttest_cmp expected1 actual1\n+'\n+\n+test_expect_failure 'delete a ref while packed-refs file is locked' '\n+\tgit branch b2 master &&\n+\t# Pack current value of b2:\n+\tgit pack-refs --all &&\n+\t# Overwrite packed value with a loose value:\n+\tgit branch -f b2 master^ &&\n+\tgit for-each-ref >expected2 &&\n+\t# Simulate another process holding the packed-refs file lock:\n+\t: >.git/packed-refs.lock &&\n+\ttest_must_fail git branch -D b2 &&\n+\trm .git/packed-refs.lock &&\n+\t# Delete failed; reference values should be unchanged:\n+\tgit for-each-ref >actual2 &&\n+\ttest_cmp expected2 actual2\n+'\n+\n+test_done\n-- \n1.9.0\n"},{"id":"238226","messageId":"1396367910-7299-3-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 02/22] try_merge_strategy(): remove redundant lock_file allocation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:10Z","receivedAt":"2014-04-01T15:58:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"By the time the \"if\" block is entered, the lock_file instance from the\nmain function block is no longer in use, so re-use that one instead of\nallocating a second one.\n\nNote that the \"lock\" variable in the \"if\" block used to shadow the\n\"lock\" variable at function scope, so the only change needed is to\nremove the inner definition.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/merge.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex e15d0e1..f714961 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -671,7 +671,6 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \tif (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\")) {\n \t\tint clean, x;\n \t\tstruct commit *result;\n-\t\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n \t\tint index_fd;\n \t\tstruct commit_list *reversed = NULL;\n \t\tstruct merge_options o;\n-- \n1.9.0\n"},{"id":"238207","messageId":"1396367910-7299-4-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 03/22] rollback_lock_file(): do not clear filename redundantly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:11Z","receivedAt":"2014-04-01T15:58:11Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is only necessary to clear the lock_file's filename field if it was\nnot already clear.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 8fbcb6a..d26711f 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -277,6 +277,6 @@ void rollback_lock_file(struct lock_file *lk)\n \t\tif (lk->fd >= 0)\n \t\t\tclose(lk->fd);\n \t\tunlink_or_warn(lk->filename);\n+\t\tlk->filename[0] = 0;\n \t}\n-\tlk->filename[0] = 0;\n }\n-- \n1.9.0\n"},{"id":"238227","messageId":"1396367910-7299-5-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 04/22] rollback_lock_file(): set fd to -1","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:12Z","receivedAt":"2014-04-01T15:58:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"When rolling back the lockfile, call close_lock_file() so that the\nlock_file's fd field gets set back to -1. This could help prevent\nconfusion in the face of hypothetical future programming errors.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex d26711f..c1af65b 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -275,7 +275,7 @@ void rollback_lock_file(struct lock_file *lk)\n {\n \tif (lk->filename[0]) {\n \t\tif (lk->fd >= 0)\n-\t\t\tclose(lk->fd);\n+\t\t\tclose_lock_file(lk);\n \t\tunlink_or_warn(lk->filename);\n \t\tlk->filename[0] = 0;\n \t}\n-- \n1.9.0\n"},{"id":"238228","messageId":"1396367910-7299-6-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 05/22] lockfile: unlock file if lockfile permissions cannot be adjusted","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:13Z","receivedAt":"2014-04-01T15:58:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If the call to adjust_shared_perm() fails, lock_file returns -1, which\nto the caller looks like any other failure to lock the file. So in\nthis case, roll back the lockfile before returning so that the lock\nfile is deleted immediately and the lockfile object is left in a\npredictable state that (namely, unlocked). Previously, the lockfile\nwas retained until process cleanup in this situation.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex c1af65b..1928e5e 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -148,9 +148,11 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\t\tlock_file_list = lk;\n \t\t\tlk->on_list = 1;\n \t\t}\n-\t\tif (adjust_shared_perm(lk->filename))\n-\t\t\treturn error(\"cannot fix permission bits on %s\",\n-\t\t\t\t lk->filename);\n+\t\tif (adjust_shared_perm(lk->filename)) {\n+\t\t\terror(\"cannot fix permission bits on %s\", lk->filename);\n+\t\t\trollback_lock_file(lk);\n+\t\t\treturn -1;\n+\t\t}\n \t}\n \telse\n \t\tlk->filename[0] = 0;\n-- \n1.9.0\n"},{"id":"238225","messageId":"1396367910-7299-7-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 06/22] hold_lock_file_for_append(): release lock on errors","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:14Z","receivedAt":"2014-04-01T15:58:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If there is an error copying the old contents to the lockfile, roll\nback the lockfile before exiting so that the lockfile is not held\nuntil process cleanup.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 1928e5e..e679e4c 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -212,13 +212,13 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n \t\tif (errno != ENOENT) {\n \t\t\tif (flags & LOCK_DIE_ON_ERROR)\n \t\t\t\tdie(\"cannot open '%s' for copying\", path);\n-\t\t\tclose(fd);\n+\t\t\trollback_lock_file(lk);\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 (flags & LOCK_DIE_ON_ERROR)\n \t\t\texit(128);\n-\t\tclose(fd);\n+\t\trollback_lock_file(lk);\n \t\treturn -1;\n \t}\n \treturn fd;\n-- \n1.9.0\n"},{"id":"238224","messageId":"1396367910-7299-8-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 07/22] lock_file(): always add lock_file object to lock_file_list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:15Z","receivedAt":"2014-04-01T15:58:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It used to be that if locking failed, lock_file() usually did not\nregister the lock_file object in lock_file_list but sometimes it did.\nThis confusion was compounded if lock_file() was called via\nhold_lock_file_for_append(), which has its own failure modes.\n\nThe ambiguity didn't have any ill effects, because lock_file objects\ncannot be removed from the lock_file_list anyway. But it is\nunnecessary to leave this behavior inconsistent.\n\nSo change lock_file() to *always* ensure that the lock_file object is\nregistered in lock_file_list regardless of whether an error occurs.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 26 ++++++++++++++++----------\n 1 file changed, 16 insertions(+), 10 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex e679e4c..c989f6c 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -130,6 +130,22 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t */\n \tstatic const size_t max_path_len = sizeof(lk->filename) - 5;\n \n+\tif (!lock_file_list) {\n+\t\t/* One-time initialization */\n+\t\tsigchain_push_common(remove_lock_file_on_signal);\n+\t\tatexit(remove_lock_file);\n+\t}\n+\n+\tlk->owner = getpid();\n+\tif (!lk->on_list) {\n+\t\t/* Initialize *lk and add it to lock_file_list: */\n+\t\tlk->fd = -1;\n+\t\tlk->on_list = 1;\n+\t\tlk->filename[0] = 0;\n+\t\tlk->next = lock_file_list;\n+\t\tlock_file_list = lk;\n+\t}\n+\n \tif (strlen(path) >= max_path_len)\n \t\treturn -1;\n \tstrcpy(lk->filename, path);\n@@ -138,16 +154,6 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \tstrcat(lk->filename, \".lock\");\n \tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (0 <= lk->fd) {\n-\t\tif (!lock_file_list) {\n-\t\t\tsigchain_push_common(remove_lock_file_on_signal);\n-\t\t\tatexit(remove_lock_file);\n-\t\t}\n-\t\tlk->owner = getpid();\n-\t\tif (!lk->on_list) {\n-\t\t\tlk->next = lock_file_list;\n-\t\t\tlock_file_list = lk;\n-\t\t\tlk->on_list = 1;\n-\t\t}\n \t\tif (adjust_shared_perm(lk->filename)) {\n \t\t\terror(\"cannot fix permission bits on %s\", lk->filename);\n \t\t\trollback_lock_file(lk);\n-- \n1.9.0\n"},{"id":"238208","messageId":"1396367910-7299-9-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 08/22] struct lock_file: replace on_list field with flags field","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:16Z","receivedAt":"2014-04-01T15:58:16Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This makes space for other bits.\n\nSigned-off-by: Michael Haggerty \n---\n cache.h | 2 +-\n lockfile.c | 9 +++++++--\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 107ac61..08d0e48 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -537,7 +537,7 @@ struct lock_file {\n \tstruct lock_file *next;\n \tint fd;\n \tpid_t owner;\n-\tchar on_list;\n+\tunsigned char flags;\n \tchar filename[PATH_MAX];\n };\n #define LOCK_DIE_ON_ERROR 1\ndiff --git a/lockfile.c b/lockfile.c\nindex c989f6c..33325a4 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -4,6 +4,11 @@\n #include \"cache.h\"\n #include \"sigchain.h\"\n \n+/* Values for lock_file::flags: */\n+\n+/* This lock_file instance is in the lock_file_list */\n+#define LOCK_FLAGS_ON_LIST 0x01\n+\n static struct lock_file *lock_file_list;\n static const char *alternate_index_output;\n \n@@ -137,10 +142,10 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t}\n \n \tlk->owner = getpid();\n-\tif (!lk->on_list) {\n+\tif (!(lk->flags & LOCK_FLAGS_ON_LIST)) {\n \t\t/* Initialize *lk and add it to lock_file_list: */\n \t\tlk->fd = -1;\n-\t\tlk->on_list = 1;\n+\t\tlk->flags |= LOCK_FLAGS_ON_LIST;\n \t\tlk->filename[0] = 0;\n \t\tlk->next = lock_file_list;\n \t\tlock_file_list = lk;\n-- \n1.9.0\n"},{"id":"238215","messageId":"1396367910-7299-10-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 09/22] api-lockfile: expand the documentation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:17Z","receivedAt":"2014-04-01T15:58:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Document a couple more functions and the flags argument as used by\nhold_lock_file_for_update() and hold_lock_file_for_append().\n\nSigned-off-by: Michael Haggerty \n---\n Documentation/technical/api-lockfile.txt | 36 +++++++++++++++++++++++++++++---\n 1 file changed, 33 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex dd89404..95ed03b 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -28,9 +28,39 @@ hold_lock_file_for_update::\n \tthe final destination (e.g. `$GIT_DIR/index`) and a flag\n \t`die_on_error`. Attempt to create a lockfile for the\n \tdestination and return the file descriptor for writing\n-\tto the file. If `die_on_error` flag is true, it dies if\n-\ta lock is already taken for the file; otherwise it\n-\treturns a negative integer to the caller on failure.\n+\tto the file. The flags parameter is a combination of\n++\n+--\n+LOCK_NODEREF::\n+\n+\tUsually symbolic links in path are resolved in path and the\n+\tlockfile is created by adding \".lock\" to the resolved path;\n+\thowever, if `LOCK_NODEREF` is set, then the lockfile is\n+\tcreated by adding \".lock\" to the path argument itself.\n+\n+LOCK_DIE_ON_ERROR::\n+\n+\tIf a lock is already taken for the file, `die()` with an error\n+\tmessage. If this option is not specified, return a negative\n+\tinteger to the caller on failure.\n+--\n+\n+hold_lock_file_for_append::\n+\n+\tLike `hold_lock_file_for_update()`, except that additionally\n+\tthe existing contents of the file (if any) are copied to the\n+\tlockfile and its write pointer is positioned at the end of the\n+\tfile before returning.\n+\n+unable_to_lock_error::\n+\n+\tEmit an error describing that there was an error locking the\n+\tspecified path. The err parameter should be the errno of the\n+\tproblem that caused the failure.\n+\n+unable_to_lock_index_die::\n+\n+\tLike `unable_to_lock_error()`, but also `die()`.\n \n commit_lock_file::\n \n-- \n1.9.0\n"},{"id":"238217","messageId":"1396367910-7299-11-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 10/22] lockfile.c: document the various states of lock_file objects","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:18Z","receivedAt":"2014-04-01T15:58:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Document the valid states of lock_file objects, how they get into each\nstate, and how the state is encoded in the object's fields.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 52 insertions(+)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 33325a4..4a9ceda 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -4,6 +4,58 @@\n #include \"cache.h\"\n #include \"sigchain.h\"\n \n+/*\n+ * File write-locks as used by Git.\n+ *\n+ * When a file at $FILENAME needs to be written, it is done as\n+ * follows:\n+ *\n+ * 1. Obtain a lock on the file by creating a lockfile at path\n+ * $FILENAME.lock. The file is opened for read/write using O_CREAT\n+ * and O_EXCL mode to ensure that it doesn't already exist. Such a\n+ * lock file is respected by writers *but not by readers*.\n+ *\n+ * Usually, if $FILENAME is a symlink, then it is resolved, and the\n+ * file ultimately pointed to is the one that is locked and later\n+ * replaced. However, if LOCK_NODEREF is used, then $FILENAME\n+ * itself is locked and later replaced, even if it is a symlink.\n+ *\n+ * 2. Write the new file contents to the lockfile.\n+ *\n+ * 3. Move the lockfile to its final destination using rename(2).\n+ *\n+ * Instead of (3), the change can be rolled back by deleting lockfile.\n+ *\n+ * This module keeps track of all locked files in lock_file_list.\n+ * When the first file is locked, it registers an atexit(3) handler;\n+ * when the program exits, the handler rolls back any files that have\n+ * been locked but were never committed or rolled back.\n+ *\n+ * A lock_file object can be in several states:\n+ *\n+ * - Uninitialized. In this state the object's flags field must be\n+ * zero but the rest of the contents need not be initialized. As\n+ * soon as the object is used in any way, it is irrevocably\n+ * registered in the lock_file_list, and flags & LOCK_FLAGS_ON_LIST\n+ * is set.\n+ *\n+ * - Locked, lockfile open (after hold_lock_file_for_update() or\n+ * hold_lock_file_for_append()). In this state, the lockfile\n+ * exists, filename holds the filename of the lockfile, fd holds a\n+ * file descriptor open for writing to the lockfile, and owner holds\n+ * the PID of the process that locked the file.\n+ *\n+ * - Locked, lockfile closed (after close_lock_file()). Same as the\n+ * previous state, except that the lockfile is closed and fd is -1.\n+ *\n+ * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a\n+ * failed attempt to lock). In this state, filename[0] == '\\0' and\n+ * fd is -1. The object is left registered in the lock_file_list,\n+ * and flags & LOCK_FLAGS_ON_LIST is set.\n+ *\n+ * See Documentation/api-lockfile.txt for more information.\n+ */\n+\n /* Values for lock_file::flags: */\n \n /* This lock_file instance is in the lock_file_list */\n-- \n1.9.0\n"},{"id":"238218","messageId":"1396367910-7299-12-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 11/22] lockfile: define a constant LOCK_SUFFIX_LEN","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:19Z","receivedAt":"2014-04-01T15:58:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty \n---\n lockfile.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 4a9ceda..4e3ada8 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -178,14 +178,17 @@ static char *resolve_symlink(char *p, size_t s)\n \treturn p;\n }\n \n+/* We append \".lock\" to the filename to derive the lockfile name: */\n+#define LOCK_SUFFIX_LEN 5\n \n static int lock_file(struct lock_file *lk, const char *path, int flags)\n {\n \t/*\n-\t * subtract 5 from size to make sure there's room for adding\n-\t * \".lock\" for the lock file name\n+\t * subtract LOCK_SUFFIX_LEN from size to make sure there's\n+\t * room for adding \".lock\" for the lock file name:\n \t */\n-\tstatic const size_t max_path_len = sizeof(lk->filename) - 5;\n+\tstatic const size_t max_path_len = sizeof(lk->filename) -\n+\t\t LOCK_SUFFIX_LEN;\n \n \tif (!lock_file_list) {\n \t\t/* One-time initialization */\n@@ -301,7 +304,7 @@ int commit_lock_file(struct lock_file *lk)\n \tif (lk->fd >= 0 && close_lock_file(lk))\n \t\treturn -1;\n \tstrcpy(result_file, lk->filename);\n-\ti = strlen(result_file) - 5; /* .lock */\n+\ti = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */\n \tresult_file[i] = 0;\n \tif (rename(lk->filename, result_file))\n \t\treturn -1;\n-- \n1.9.0\n"},{"id":"238219","messageId":"1396367910-7299-13-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 12/22] delete_ref_loose(): don't muck around in the lock_file's filename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:20Z","receivedAt":"2014-04-01T15:58:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It's bad manners. Especially since, if unlink_or_warn() failed, the\nmemory wasn't restored to its original contents.\n\nSo make our own copy to work with.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 28d5eca..11ad23e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2485,12 +2485,14 @@ static int repack_without_ref(const char *refname)\n static int delete_ref_loose(struct ref_lock *lock, int flag)\n {\n \tif (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n-\t\t/* loose */\n-\t\tint err, i = strlen(lock->lk->filename) - 5; /* .lock */\n-\n-\t\tlock->lk->filename[i] = 0;\n-\t\terr = unlink_or_warn(lock->lk->filename);\n-\t\tlock->lk->filename[i] = '.';\n+\t\t/*\n+\t\t * loose. The loose file name is the same as the\n+\t\t * lockfile name, minus \".lock\":\n+\t\t */\n+\t\tchar *loose_filename = xmemdupz(lock->lk->filename,\n+\t\t\t\t\t\tstrlen(lock->lk->filename) - 5);\n+\t\tint err = unlink_or_warn(loose_filename);\n+\t\tfree(loose_filename);\n \t\tif (err && errno != ENOENT)\n \t\t\treturn 1;\n \t}\n-- \n1.9.0\n"},{"id":"238220","messageId":"1396367910-7299-14-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 13/22] config: change write_error() to take a (struct lock_file *) argument","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:21Z","receivedAt":"2014-04-01T15:58:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Reduce the amount of code that has to know about the lock_file's\nfilename field.\n\nSigned-off-by: Michael Haggerty \n---\n config.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 6821cef..1ea3f39 100644\n--- a/config.c\n+++ b/config.c\n@@ -1303,9 +1303,9 @@ static int store_aux(const char *key, const char *value, void *cb)\n \treturn 0;\n }\n \n-static int write_error(const char *filename)\n+static int write_error(struct lock_file *lk)\n {\n-\terror(\"failed to write new configuration file %s\", filename);\n+\terror(\"failed to write new configuration file %s\", lk->filename);\n \n \t/* Same error code as \"failed to rename\". */\n \treturn 4;\n@@ -1706,7 +1706,7 @@ out_free:\n \treturn ret;\n \n write_err_out:\n-\tret = write_error(lock->filename);\n+\tret = write_error(lock);\n \tgoto out_free;\n \n }\n@@ -1821,7 +1821,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n \t\t\t\t}\n \t\t\t\tstore.baselen = strlen(new_name);\n \t\t\t\tif (!store_write_section(out_fd, new_name)) {\n-\t\t\t\t\tret = write_error(lock->filename);\n+\t\t\t\t\tret = write_error(lock);\n \t\t\t\t\tgoto out;\n \t\t\t\t}\n \t\t\t\t/*\n@@ -1847,7 +1847,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n \t\t\tcontinue;\n \t\tlength = strlen(output);\n \t\tif (write_in_full(out_fd, output, length) != length) {\n-\t\t\tret = write_error(lock->filename);\n+\t\t\tret = write_error(lock);\n \t\t\tgoto out;\n \t\t}\n \t}\n-- \n1.9.0\n"},{"id":"238221","messageId":"1396367910-7299-15-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 14/22] lockfile: use strbufs when handling (most) paths","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:22Z","receivedAt":"2014-04-01T15:58:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change struct lock_file's filename field from a fixed-length buffer\ninto a strbuf. This:\n\n* Make buffer overflows a bit less likely\n\n* Removes arbitrary limitations on the length of the path in some\n places, though the restrictions still have to be removed from other\n places to make a real difference\n\n* Shortens the code\n\nName the new strbuf field lock_filename to make it clear that it\ncontains the name of the lockfile as opposed to the name of the file\nbeing locked. Initialize the strbuf the first time the lock_file\nobject is used.\n\nChange resolve_symlink() to work on a strbuf.\n\nReplace last_path_elm() with a new function, trim_last_path_elm(),\nwhich operates on a strbuf.\n\nAdjust other places in the code that refer directly to the filename.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/commit.c | 12 ++--\n builtin/reflog.c | 2 +-\n cache.h | 2 +-\n config.c | 3 +-\n lockfile.c | 183 ++++++++++++++++++++++++-------------------------------\n refs.c | 6 +-\n shallow.c | 6 +-\n 7 files changed, 97 insertions(+), 117 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d9550c5..28944de 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -330,7 +330,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n \t\t\tdie(_(\"unable to create temporary index\"));\n \n \t\told_index_env = getenv(INDEX_ENVIRONMENT);\n-\t\tsetenv(INDEX_ENVIRONMENT, index_lock.filename, 1);\n+\t\tsetenv(INDEX_ENVIRONMENT, index_lock.lock_filename.buf, 1);\n \n \t\tif (interactive_add(argc, argv, prefix, patch_interactive) != 0)\n \t\t\tdie(_(\"interactive add failed\"));\n@@ -341,10 +341,10 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n \t\t\tunsetenv(INDEX_ENVIRONMENT);\n \n \t\tdiscard_cache();\n-\t\tread_cache_from(index_lock.filename);\n+\t\tread_cache_from(index_lock.lock_filename.buf);\n \n \t\tcommit_style = COMMIT_NORMAL;\n-\t\treturn index_lock.filename;\n+\t\treturn index_lock.lock_filename.buf;\n \t}\n \n \t/*\n@@ -368,7 +368,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n \t\t close_lock_file(&index_lock))\n \t\t\tdie(_(\"unable to write new_index file\"));\n \t\tcommit_style = COMMIT_NORMAL;\n-\t\treturn index_lock.filename;\n+\t\treturn index_lock.lock_filename.buf;\n \t}\n \n \t/*\n@@ -453,9 +453,9 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n \t\tdie(_(\"unable to write temporary index file\"));\n \n \tdiscard_cache();\n-\tread_cache_from(false_lock.filename);\n+\tread_cache_from(false_lock.lock_filename.buf);\n \n-\treturn false_lock.filename;\n+\treturn false_lock.lock_filename.buf;\n }\n \n static int run_status(FILE *fp, const char *index_file, const char *prefix, int nowarn,\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex c12a9784..29bc07b 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -431,7 +431,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\n \t\t\t write_str_in_full(lock->lock_fd, \"\\n\") != 1 ||\n \t\t\t close_ref(lock) < 0)) {\n \t\t\tstatus |= error(\"Couldn't write %s\",\n-\t\t\t\tlock->lk->filename);\n+\t\t\t\tlock->lk->lock_filename.buf);\n \t\t\tunlink(newlog_path);\n \t\t} else if (rename(newlog_path, log_file)) {\n \t\t\tstatus |= error(\"cannot rename %s to %s\",\ndiff --git a/cache.h b/cache.h\nindex 08d0e48..3d0a835 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -538,7 +538,7 @@ struct lock_file {\n \tint fd;\n \tpid_t owner;\n \tunsigned char flags;\n-\tchar filename[PATH_MAX];\n+\tstruct strbuf lock_filename;\n };\n #define LOCK_DIE_ON_ERROR 1\n #define LOCK_NODEREF 2\ndiff --git a/config.c b/config.c\nindex 1ea3f39..cea0af0 100644\n--- a/config.c\n+++ b/config.c\n@@ -1305,7 +1305,8 @@ static int store_aux(const char *key, const char *value, void *cb)\n \n static int write_error(struct lock_file *lk)\n {\n-\terror(\"failed to write new configuration file %s\", lk->filename);\n+\terror(\"failed to write new configuration file %s\",\n+\t lk->lock_filename.buf);\n \n \t/* Same error code as \"failed to rename\". */\n \treturn 4;\ndiff --git a/lockfile.c b/lockfile.c\nindex 4e3ada8..931ebbd 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -34,24 +34,25 @@\n * A lock_file object can be in several states:\n *\n * - Uninitialized. In this state the object's flags field must be\n- * zero but the rest of the contents need not be initialized. As\n- * soon as the object is used in any way, it is irrevocably\n- * registered in the lock_file_list, and flags & LOCK_FLAGS_ON_LIST\n- * is set.\n+ * zero but the rest of the contents need not be initialized. In\n+ * particular, the lock_filename strbuf should *not* be initialized\n+ * externally. The first time the object is used in any way, it is\n+ * initialized, permanently registered in the lock_file_list, and\n+ * flags & LOCK_FLAGS_ON_LIST is set.\n *\n * - Locked, lockfile open (after hold_lock_file_for_update() or\n * hold_lock_file_for_append()). In this state, the lockfile\n- * exists, filename holds the filename of the lockfile, fd holds a\n- * file descriptor open for writing to the lockfile, and owner holds\n- * the PID of the process that locked the file.\n+ * exists, lock_filename holds the filename of the lockfile, fd\n+ * holds a file descriptor open for writing to the lockfile, and\n+ * owner holds the PID of the process that locked the file.\n *\n * - Locked, lockfile closed (after close_lock_file()). Same as the\n * previous state, except that the lockfile is closed and fd is -1.\n *\n * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a\n- * failed attempt to lock). In this state, filename[0] == '\\0' and\n- * fd is -1. The object is left registered in the lock_file_list,\n- * and flags & LOCK_FLAGS_ON_LIST is set.\n+ * failed attempt to lock). In this state, lock_filename is the\n+ * empty string and fd is -1. The object is left registered in the\n+ * lock_file_list, and flags & LOCK_FLAGS_ON_LIST is set.\n *\n * See Documentation/api-lockfile.txt for more information.\n */\n@@ -70,10 +71,10 @@ static void remove_lock_file(void)\n \n \twhile (lock_file_list) {\n \t\tif (lock_file_list->owner == me &&\n-\t\t lock_file_list->filename[0]) {\n+\t\t lock_file_list->lock_filename.len) {\n \t\t\tif (lock_file_list->fd >= 0)\n \t\t\t\tclose(lock_file_list->fd);\n-\t\t\tunlink_or_warn(lock_file_list->filename);\n+\t\t\tunlink_or_warn(lock_file_list->lock_filename.buf);\n \t\t}\n \t\tlock_file_list = lock_file_list->next;\n \t}\n@@ -87,95 +88,70 @@ static void remove_lock_file_on_signal(int signo)\n }\n \n /*\n- * p = absolute or relative path name\n+ * path = absolute or relative path name\n *\n- * Return a pointer into p showing the beginning of the last path name\n- * element. If p is empty or the root directory (\"/\"), just return p.\n+ * Remove the last path name element from path (leaving the preceding\n+ * \"/\", if any). If path is empty or the root directory (\"/\"), set\n+ * path to the empty string.\n */\n-static char *last_path_elm(char *p)\n+static void trim_last_path_elm(struct strbuf *path)\n {\n-\t/* r starts pointing to null at the end of the string */\n-\tchar *r = strchr(p, '\\0');\n-\n-\tif (r == p)\n-\t\treturn p; /* just return empty string */\n-\n-\tr--; /* back up to last non-null character */\n+\tint i = path->len;\n \n \t/* back up past trailing slashes, if any */\n-\twhile (r > p && *r == '/')\n-\t\tr--;\n-\n+\twhile (i && path->buf[i - 1] == '/')\n+\t\ti--;\n \t/*\n-\t * then go backwards until I hit a slash, or the beginning of\n-\t * the string\n+\t * then go backwards until a slash, or the beginning of the\n+\t * string\n \t */\n-\twhile (r > p && *(r-1) != '/')\n-\t\tr--;\n-\treturn r;\n-}\n+\twhile (i && path->buf[i - 1] != '/')\n+\t\ti--;\n \n+\tstrbuf_setlen(path, i);\n+}\n \n /* We allow \"recursive\" symbolic links. Only within reason, though */\n #define MAXDEPTH 5\n \n /*\n- * p = path that may be a symlink\n- * s = full size of p\n- *\n- * If p is a symlink, attempt to overwrite p with a path to the real\n- * file or directory (which may or may not exist), following a chain of\n- * symlinks if necessary. Otherwise, leave p unmodified.\n+ * path contains a path that may be a symlink\n *\n- * This is a best-effort routine. If an error occurs, p will either be\n- * left unmodified or will name a different symlink in a symlink chain\n- * that started with p's initial contents.\n+ * If path is a symlink, attempt to overwrite it with a path to the\n+ * real file or directory (which may or may not exist), following a\n+ * chain of symlinks if necessary. Otherwise, leave path unmodified.\n *\n- * Always returns p.\n+ * This is a best-effort routine. If an error occurs, path will\n+ * either be left unmodified or will name a different symlink in a\n+ * symlink chain that started with path's initial contents.\n */\n-\n-static char *resolve_symlink(char *p, size_t s)\n+static void resolve_symlink(struct strbuf *path)\n {\n \tint depth = MAXDEPTH;\n \n \twhile (depth--) {\n \t\tchar link[PATH_MAX];\n-\t\tint link_len = readlink(p, link, sizeof(link));\n+\t\tint link_len = readlink(path->buf, link, sizeof(link));\n \t\tif (link_len < 0) {\n \t\t\t/* not a symlink anymore */\n-\t\t\treturn p;\n+\t\t\treturn;\n \t\t}\n-\t\telse if (link_len < sizeof(link))\n-\t\t\t/* readlink() never null-terminates */\n-\t\t\tlink[link_len] = '\\0';\n-\t\telse {\n-\t\t\twarning(\"%s: symlink too long\", p);\n-\t\t\treturn p;\n+\t\tif (link_len >= sizeof(link)) {\n+\t\t\twarning(\"%s: symlink too long\", path->buf);\n+\t\t\treturn;\n \t\t}\n+\t\t/* readlink() never null-terminates */\n+\t\tlink[link_len] = '\\0';\n \n-\t\tif (is_absolute_path(link)) {\n-\t\t\t/* absolute path simply replaces p */\n-\t\t\tif (link_len < s)\n-\t\t\t\tstrcpy(p, link);\n-\t\t\telse {\n-\t\t\t\twarning(\"%s: symlink too long\", p);\n-\t\t\t\treturn p;\n-\t\t\t}\n-\t\t} else {\n-\t\t\t/*\n-\t\t\t * link is a relative path, so I must replace the\n-\t\t\t * last element of p with it.\n-\t\t\t */\n-\t\t\tchar *r = (char *)last_path_elm(p);\n-\t\t\tif (r - p + link_len < s)\n-\t\t\t\tstrcpy(r, link);\n-\t\t\telse {\n-\t\t\t\twarning(\"%s: symlink too long\", p);\n-\t\t\t\treturn p;\n-\t\t\t}\n-\t\t}\n+\t\tif (is_absolute_path(link))\n+\t\t\t/* an absolute path replaces the whole path: */\n+\t\t\tstrbuf_setlen(path, 0);\n+\t\telse\n+\t\t\t/* a relative path replaces the last element of path: */\n+\t\t\ttrim_last_path_elm(path);\n+\n+\t\tstrbuf_add(path, link, link_len);\n \t}\n-\treturn p;\n }\n \n /* We append \".lock\" to the filename to derive the lockfile name: */\n@@ -183,12 +159,7 @@ static char *resolve_symlink(char *p, size_t s)\n \n static int lock_file(struct lock_file *lk, const char *path, int flags)\n {\n-\t/*\n-\t * subtract LOCK_SUFFIX_LEN from size to make sure there's\n-\t * room for adding \".lock\" for the lock file name:\n-\t */\n-\tstatic const size_t max_path_len = sizeof(lk->filename) -\n-\t\t LOCK_SUFFIX_LEN;\n+\tsize_t path_len = strlen(path);\n \n \tif (!lock_file_list) {\n \t\t/* One-time initialization */\n@@ -197,31 +168,39 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t}\n \n \tlk->owner = getpid();\n-\tif (!(lk->flags & LOCK_FLAGS_ON_LIST)) {\n+\tif (lk->flags & LOCK_FLAGS_ON_LIST) {\n+\t\tassert(!lk->lock_filename.len); /* object not already in use */\n+\t\tif (strbuf_avail(&lk->lock_filename) < path_len + LOCK_SUFFIX_LEN)\n+\t\t\tstrbuf_grow(&lk->lock_filename, path_len + LOCK_SUFFIX_LEN);\n+\t} else {\n \t\t/* Initialize *lk and add it to lock_file_list: */\n \t\tlk->fd = -1;\n \t\tlk->flags |= LOCK_FLAGS_ON_LIST;\n-\t\tlk->filename[0] = 0;\n+\t\tstrbuf_init(&lk->lock_filename, path_len + LOCK_SUFFIX_LEN);\n \t\tlk->next = lock_file_list;\n \t\tlock_file_list = lk;\n \t}\n \n-\tif (strlen(path) >= max_path_len)\n-\t\treturn -1;\n-\tstrcpy(lk->filename, path);\n+\tstrbuf_add(&lk->lock_filename, path, path_len);\n \tif (!(flags & LOCK_NODEREF))\n-\t\tresolve_symlink(lk->filename, max_path_len);\n-\tstrcat(lk->filename, \".lock\");\n-\tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n+\t\tresolve_symlink(&lk->lock_filename);\n+\tstrbuf_addstr(&lk->lock_filename, \".lock\");\n+\n+\tif (lk->lock_filename.len >= PATH_MAX) {\n+\t\terror(\"%s: path too long\", lk->lock_filename.buf);\n+\t\tstrbuf_setlen(&lk->lock_filename, 0);\n+\t\treturn -1;\n+\t}\n+\tlk->fd = open(lk->lock_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (0 <= lk->fd) {\n-\t\tif (adjust_shared_perm(lk->filename)) {\n-\t\t\terror(\"cannot fix permission bits on %s\", lk->filename);\n+\t\tif (adjust_shared_perm(lk->lock_filename.buf)) {\n+\t\t\terror(\"cannot fix permission bits on %s\", lk->lock_filename.buf);\n \t\t\trollback_lock_file(lk);\n \t\t\treturn -1;\n \t\t}\n \t}\n \telse\n-\t\tlk->filename[0] = 0;\n+\t\tstrbuf_setlen(&lk->lock_filename, 0);\n \treturn lk->fd;\n }\n \n@@ -300,15 +279,15 @@ int close_lock_file(struct lock_file *lk)\n int commit_lock_file(struct lock_file *lk)\n {\n \tchar result_file[PATH_MAX];\n-\tsize_t i;\n+\tsize_t path_len = lk->lock_filename.len - LOCK_SUFFIX_LEN;\n+\n \tif (lk->fd >= 0 && close_lock_file(lk))\n \t\treturn -1;\n-\tstrcpy(result_file, lk->filename);\n-\ti = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */\n-\tresult_file[i] = 0;\n-\tif (rename(lk->filename, result_file))\n+\tmemcpy(result_file, lk->lock_filename.buf, path_len);\n+\tresult_file[path_len] = '\\0';\n+\tif (rename(lk->lock_filename.buf, result_file))\n \t\treturn -1;\n-\tlk->filename[0] = 0;\n+\tstrbuf_setlen(&lk->lock_filename, 0);\n \treturn 0;\n }\n \n@@ -330,9 +309,9 @@ int commit_locked_index(struct lock_file *lk)\n \tif (alternate_index_output) {\n \t\tif (lk->fd >= 0 && close_lock_file(lk))\n \t\t\treturn -1;\n-\t\tif (rename(lk->filename, alternate_index_output))\n+\t\tif (rename(lk->lock_filename.buf, alternate_index_output))\n \t\t\treturn -1;\n-\t\tlk->filename[0] = 0;\n+\t\tstrbuf_setlen(&lk->lock_filename, 0);\n \t\treturn 0;\n \t}\n \telse\n@@ -341,10 +320,10 @@ int commit_locked_index(struct lock_file *lk)\n \n void rollback_lock_file(struct lock_file *lk)\n {\n-\tif (lk->filename[0]) {\n+\tif (lk->lock_filename.len) {\n \t\tif (lk->fd >= 0)\n \t\t\tclose_lock_file(lk);\n-\t\tunlink_or_warn(lk->filename);\n-\t\tlk->filename[0] = 0;\n+\t\tunlink_or_warn(lk->lock_filename.buf);\n+\t\tstrbuf_setlen(&lk->lock_filename, 0);\n \t}\n }\ndiff --git a/refs.c b/refs.c\nindex 11ad23e..93abc94 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2489,8 +2489,8 @@ static int delete_ref_loose(struct ref_lock *lock, int flag)\n \t\t * loose. The loose file name is the same as the\n \t\t * lockfile name, minus \".lock\":\n \t\t */\n-\t\tchar *loose_filename = xmemdupz(lock->lk->filename,\n-\t\t\t\t\t\tstrlen(lock->lk->filename) - 5);\n+\t\tchar *loose_filename = xmemdupz(lock->lk->lock_filename.buf,\n+\t\t\t\t\t\tlock->lk->lock_filename.len - 5);\n \t\tint err = unlink_or_warn(loose_filename);\n \t\tfree(loose_filename);\n \t\tif (err && errno != ENOENT)\n@@ -2828,7 +2828,7 @@ int write_ref_sha1(struct ref_lock *lock,\n \tif (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||\n \t write_in_full(lock->lock_fd, &term, 1) != 1\n \t\t|| close_ref(lock) < 0) {\n-\t\terror(\"Couldn't write %s\", lock->lk->filename);\n+\t\terror(\"Couldn't write %s\", lock->lk->lock_filename.buf);\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\ndiff --git a/shallow.c b/shallow.c\nindex 0b267b6..a27d587 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -269,8 +269,8 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,\n \tif (write_shallow_commits(&sb, 0, extra)) {\n \t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n \t\t\tdie_errno(\"failed to write to %s\",\n-\t\t\t\t shallow_lock->filename);\n-\t\t*alternate_shallow_file = shallow_lock->filename;\n+\t\t\t\t shallow_lock->lock_filename.buf);\n+\t\t*alternate_shallow_file = shallow_lock->lock_filename.buf;\n \t} else\n \t\t/*\n \t\t * is_repository_shallow() sees empty string as \"no\n@@ -316,7 +316,7 @@ void prune_shallow(int show_only)\n \tif (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {\n \t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n \t\t\tdie_errno(\"failed to write to %s\",\n-\t\t\t\t shallow_lock.filename);\n+\t\t\t\t shallow_lock.lock_filename.buf);\n \t\tcommit_lock_file(&shallow_lock);\n \t} else {\n \t\tunlink(git_path(\"shallow\"));\n-- \n1.9.0\n"},{"id":"238222","messageId":"1396367910-7299-16-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 15/22] resolve_symlink(): use a strbuf internally","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:23Z","receivedAt":"2014-04-01T15:58:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This removes another place where the path name is arbitrarily limited.\n\nThis gets rid of a warning\n\n %s: symlink too long\n\neven though strbuf_readlink() in fact still limits the length of what\nit can handle to something like 2*PATH_MAX. But (1) the limit is now\nmuch longer, and (2) strbuf_readlink() doesn't return enough\ninformation to distinguish this problem from other symlink-reading\nproblems, so just let it go.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 23 +++++++++--------------\n 1 file changed, 9 insertions(+), 14 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 931ebbd..0ade314 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -128,30 +128,25 @@ static void trim_last_path_elm(struct strbuf *path)\n static void resolve_symlink(struct strbuf *path)\n {\n \tint depth = MAXDEPTH;\n+\tstruct strbuf link;\n+\n+\tstrbuf_init(&link, path->len);\n \n \twhile (depth--) {\n-\t\tchar link[PATH_MAX];\n-\t\tint link_len = readlink(path->buf, link, sizeof(link));\n-\t\tif (link_len < 0) {\n-\t\t\t/* not a symlink anymore */\n-\t\t\treturn;\n-\t\t}\n-\t\tif (link_len >= sizeof(link)) {\n-\t\t\twarning(\"%s: symlink too long\", path->buf);\n-\t\t\treturn;\n-\t\t}\n-\t\t/* readlink() never null-terminates */\n-\t\tlink[link_len] = '\\0';\n+\t\tif (strbuf_readlink(&link, path->buf, path->len) < 0)\n+\t\t\tbreak;\n \n-\t\tif (is_absolute_path(link))\n+\t\tif (is_absolute_path(link.buf))\n \t\t\t/* an absolute path replaces the whole path: */\n \t\t\tstrbuf_setlen(path, 0);\n \t\telse\n \t\t\t/* a relative path replaces the last element of path: */\n \t\t\ttrim_last_path_elm(path);\n \n-\t\tstrbuf_add(path, link, link_len);\n+\t\tstrbuf_addbuf(path, &link);\n \t}\n+\n+\tstrbuf_release(&link);\n }\n \n /* We append \".lock\" to the filename to derive the lockfile name: */\n-- \n1.9.0\n"},{"id":"238209","messageId":"1396367910-7299-17-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 16/22] commit_lock_file(): don't work with a fixed-length buffer","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:24Z","receivedAt":"2014-04-01T15:58:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Allocate temporary space to hold result_file instead of storing it in\na fixed-length buffer. This removes the last arbitrary path-length\nlimitation, so remove the path length check in lock_file().\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 19 ++++++++-----------\n 1 file changed, 8 insertions(+), 11 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 0ade314..c1ca5b1 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -181,11 +181,6 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\tresolve_symlink(&lk->lock_filename);\n \tstrbuf_addstr(&lk->lock_filename, \".lock\");\n \n-\tif (lk->lock_filename.len >= PATH_MAX) {\n-\t\terror(\"%s: path too long\", lk->lock_filename.buf);\n-\t\tstrbuf_setlen(&lk->lock_filename, 0);\n-\t\treturn -1;\n-\t}\n \tlk->fd = open(lk->lock_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (0 <= lk->fd) {\n \t\tif (adjust_shared_perm(lk->lock_filename.buf)) {\n@@ -273,17 +268,19 @@ int close_lock_file(struct lock_file *lk)\n \n int commit_lock_file(struct lock_file *lk)\n {\n-\tchar result_file[PATH_MAX];\n+\tchar *result_file;\n \tsize_t path_len = lk->lock_filename.len - LOCK_SUFFIX_LEN;\n+\tint err = 0;\n \n \tif (lk->fd >= 0 && close_lock_file(lk))\n \t\treturn -1;\n-\tmemcpy(result_file, lk->lock_filename.buf, path_len);\n-\tresult_file[path_len] = '\\0';\n+\tresult_file = xmemdupz(lk->lock_filename.buf, path_len);\n \tif (rename(lk->lock_filename.buf, result_file))\n-\t\treturn -1;\n-\tstrbuf_setlen(&lk->lock_filename, 0);\n-\treturn 0;\n+\t\terr = -1;\n+\telse\n+\t\tstrbuf_setlen(&lk->lock_filename, 0);\n+\tfree(result_file);\n+\treturn err;\n }\n \n int hold_locked_index(struct lock_file *lk, int die_on_error)\n-- \n1.9.0\n"},{"id":"238216","messageId":"1396367910-7299-18-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 17/22] lock_file(): exit early if lockfile cannot be opened","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:25Z","receivedAt":"2014-04-01T15:58:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is a bit easier to read than the old version, which nested part\nof the non-error code in an \"if\" block.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 17 +++++++++--------\n 1 file changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex c1ca5b1..87b40c4 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -182,15 +182,16 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \tstrbuf_addstr(&lk->lock_filename, \".lock\");\n \n \tlk->fd = open(lk->lock_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n-\tif (0 <= lk->fd) {\n-\t\tif (adjust_shared_perm(lk->lock_filename.buf)) {\n-\t\t\terror(\"cannot fix permission bits on %s\", lk->lock_filename.buf);\n-\t\t\trollback_lock_file(lk);\n-\t\t\treturn -1;\n-\t\t}\n-\t}\n-\telse\n+\tif (lk->fd < 0) {\n \t\tstrbuf_setlen(&lk->lock_filename, 0);\n+\t\treturn -1;\n+\t}\n+\tif (adjust_shared_perm(lk->lock_filename.buf)) {\n+\t\terror(\"cannot fix permission bits on %s\", lk->lock_filename.buf);\n+\t\trollback_lock_file(lk);\n+\t\treturn -1;\n+\t}\n+\n \treturn lk->fd;\n }\n \n-- \n1.9.0\n"},{"id":"238211","messageId":"1396367910-7299-19-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 18/22] lockfile: also keep track of the filename of the file being locked","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:26Z","receivedAt":"2014-04-01T15:58:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This reduces the amount of string editing gyrations and makes it\nunnecessary for callers to know how to derive the filename from the\nlock_filename.\n\nSigned-off-by: Michael Haggerty \n---\n cache.h | 1 +\n lockfile.c | 57 ++++++++++++++++++++++++++++++++-------------------------\n refs.c | 10 ++--------\n 3 files changed, 35 insertions(+), 33 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 3d0a835..0fefc66 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -538,6 +538,7 @@ struct lock_file {\n \tint fd;\n \tpid_t owner;\n \tunsigned char flags;\n+\tstruct strbuf filename;\n \tstruct strbuf lock_filename;\n };\n #define LOCK_DIE_ON_ERROR 1\ndiff --git a/lockfile.c b/lockfile.c\nindex 87b40c4..07b5c21 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -35,24 +35,26 @@\n *\n * - Uninitialized. In this state the object's flags field must be\n * zero but the rest of the contents need not be initialized. In\n- * particular, the lock_filename strbuf should *not* be initialized\n- * externally. The first time the object is used in any way, it is\n- * initialized, permanently registered in the lock_file_list, and\n- * flags & LOCK_FLAGS_ON_LIST is set.\n+ * particular, the filename and lock_filename strbufs should *not*\n+ * be initialized externally. The first time the object is used in\n+ * any way, it is initialized, permanently registered in the\n+ * lock_file_list, and flags & LOCK_FLAGS_ON_LIST is set.\n *\n * - Locked, lockfile open (after hold_lock_file_for_update() or\n * hold_lock_file_for_append()). In this state, the lockfile\n- * exists, lock_filename holds the filename of the lockfile, fd\n- * holds a file descriptor open for writing to the lockfile, and\n- * owner holds the PID of the process that locked the file.\n+ * exists, filename holds the filename of the locked file,\n+ * lock_filename holds the filename of the lockfile, fd holds a file\n+ * descriptor open for writing to the lockfile, and owner holds the\n+ * PID of the process that locked the file.\n *\n * - Locked, lockfile closed (after close_lock_file()). Same as the\n * previous state, except that the lockfile is closed and fd is -1.\n *\n * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a\n- * failed attempt to lock). In this state, lock_filename is the\n- * empty string and fd is -1. The object is left registered in the\n- * lock_file_list, and flags & LOCK_FLAGS_ON_LIST is set.\n+ * failed attempt to lock). In this state, filename and\n+ * lock_filename are the empty string and fd is -1. The object is\n+ * left registered in the lock_file_list, and flags &\n+ * LOCK_FLAGS_ON_LIST is set.\n *\n * See Documentation/api-lockfile.txt for more information.\n */\n@@ -164,25 +166,30 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \n \tlk->owner = getpid();\n \tif (lk->flags & LOCK_FLAGS_ON_LIST) {\n-\t\tassert(!lk->lock_filename.len); /* object not already in use */\n-\t\tif (strbuf_avail(&lk->lock_filename) < path_len + LOCK_SUFFIX_LEN)\n-\t\t\tstrbuf_grow(&lk->lock_filename, path_len + LOCK_SUFFIX_LEN);\n+\t\t/* Sanity check that object is not already in use: */\n+\t\tassert(!lk->filename.len);\n+\t\tassert(!lk->lock_filename.len);\n \t} else {\n \t\t/* Initialize *lk and add it to lock_file_list: */\n \t\tlk->fd = -1;\n \t\tlk->flags |= LOCK_FLAGS_ON_LIST;\n-\t\tstrbuf_init(&lk->lock_filename, path_len + LOCK_SUFFIX_LEN);\n+\t\tstrbuf_init(&lk->filename, path_len);\n+\t\tstrbuf_init(&lk->lock_filename, 0);\n \t\tlk->next = lock_file_list;\n \t\tlock_file_list = lk;\n \t}\n \n-\tstrbuf_add(&lk->lock_filename, path, path_len);\n+\tstrbuf_add(&lk->filename, path, path_len);\n \tif (!(flags & LOCK_NODEREF))\n-\t\tresolve_symlink(&lk->lock_filename);\n+\t\tresolve_symlink(&lk->filename);\n+\n+\tstrbuf_grow(&lk->lock_filename, lk->filename.len + LOCK_SUFFIX_LEN);\n+\tstrbuf_addbuf(&lk->lock_filename, &lk->filename);\n \tstrbuf_addstr(&lk->lock_filename, \".lock\");\n \n \tlk->fd = open(lk->lock_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (lk->fd < 0) {\n+\t\tstrbuf_setlen(&lk->filename, 0);\n \t\tstrbuf_setlen(&lk->lock_filename, 0);\n \t\treturn -1;\n \t}\n@@ -269,18 +276,16 @@ int close_lock_file(struct lock_file *lk)\n \n int commit_lock_file(struct lock_file *lk)\n {\n-\tchar *result_file;\n-\tsize_t path_len = lk->lock_filename.len - LOCK_SUFFIX_LEN;\n \tint err = 0;\n \n \tif (lk->fd >= 0 && close_lock_file(lk))\n \t\treturn -1;\n-\tresult_file = xmemdupz(lk->lock_filename.buf, path_len);\n-\tif (rename(lk->lock_filename.buf, result_file))\n+\tif (rename(lk->lock_filename.buf, lk->filename.buf)) {\n \t\terr = -1;\n-\telse\n+\t} else {\n+\t\tstrbuf_setlen(&lk->filename, 0);\n \t\tstrbuf_setlen(&lk->lock_filename, 0);\n-\tfree(result_file);\n+\t}\n \treturn err;\n }\n \n@@ -304,19 +309,21 @@ int commit_locked_index(struct lock_file *lk)\n \t\t\treturn -1;\n \t\tif (rename(lk->lock_filename.buf, alternate_index_output))\n \t\t\treturn -1;\n+\t\tstrbuf_setlen(&lk->filename, 0);\n \t\tstrbuf_setlen(&lk->lock_filename, 0);\n \t\treturn 0;\n-\t}\n-\telse\n+\t} else {\n \t\treturn commit_lock_file(lk);\n+\t}\n }\n \n void rollback_lock_file(struct lock_file *lk)\n {\n-\tif (lk->lock_filename.len) {\n+\tif (lk->filename.len) {\n \t\tif (lk->fd >= 0)\n \t\t\tclose_lock_file(lk);\n \t\tunlink_or_warn(lk->lock_filename.buf);\n+\t\tstrbuf_setlen(&lk->filename, 0);\n \t\tstrbuf_setlen(&lk->lock_filename, 0);\n \t}\n }\ndiff --git a/refs.c b/refs.c\nindex 93abc94..85967e7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2485,14 +2485,8 @@ static int repack_without_ref(const char *refname)\n static int delete_ref_loose(struct ref_lock *lock, int flag)\n {\n \tif (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n-\t\t/*\n-\t\t * loose. The loose file name is the same as the\n-\t\t * lockfile name, minus \".lock\":\n-\t\t */\n-\t\tchar *loose_filename = xmemdupz(lock->lk->lock_filename.buf,\n-\t\t\t\t\t\tlock->lk->lock_filename.len - 5);\n-\t\tint err = unlink_or_warn(loose_filename);\n-\t\tfree(loose_filename);\n+\t\t/* loose */\n+\t\tint err = unlink_or_warn(lock->lk->filename.buf);\n \t\tif (err && errno != ENOENT)\n \t\t\treturn 1;\n \t}\n-- \n1.9.0\n"},{"id":"238214","messageId":"1396367910-7299-20-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 19/22] struct lock_file: rename lock_filename field to staging_filename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:27Z","receivedAt":"2014-04-01T15:58:27Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Now that the filename is stored separately, the external users of this\nfield only use it as the filename to which they are trying to write.\nSoon it will not necessarily be the name of the lock file, so rename\nthe field more generically.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/commit.c | 12 ++++++------\n builtin/reflog.c | 2 +-\n cache.h | 2 +-\n config.c | 2 +-\n lockfile.c | 53 +++++++++++++++++++++++++++--------------------------\n refs.c | 2 +-\n shallow.c | 6 +++---\n 7 files changed, 40 insertions(+), 39 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 28944de..6557bde 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -330,7 +330,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n \t\t\tdie(_(\"unable to create temporary index\"));\n \n \t\told_index_env = getenv(INDEX_ENVIRONMENT);\n-\t\tsetenv(INDEX_ENVIRONMENT, index_lock.lock_filename.buf, 1);\n+\t\tsetenv(INDEX_ENVIRONMENT, index_lock.staging_filename.buf, 1);\n \n \t\tif (interactive_add(argc, argv, prefix, patch_interactive) != 0)\n \t\t\tdie(_(\"interactive add failed\"));\n@@ -341,10 +341,10 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n \t\t\tunsetenv(INDEX_ENVIRONMENT);\n \n \t\tdiscard_cache();\n-\t\tread_cache_from(index_lock.lock_filename.buf);\n+\t\tread_cache_from(index_lock.staging_filename.buf);\n \n \t\tcommit_style = COMMIT_NORMAL;\n-\t\treturn index_lock.lock_filename.buf;\n+\t\treturn index_lock.staging_filename.buf;\n \t}\n \n \t/*\n@@ -368,7 +368,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n \t\t close_lock_file(&index_lock))\n \t\t\tdie(_(\"unable to write new_index file\"));\n \t\tcommit_style = COMMIT_NORMAL;\n-\t\treturn index_lock.lock_filename.buf;\n+\t\treturn index_lock.staging_filename.buf;\n \t}\n \n \t/*\n@@ -453,9 +453,9 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n \t\tdie(_(\"unable to write temporary index file\"));\n \n \tdiscard_cache();\n-\tread_cache_from(false_lock.lock_filename.buf);\n+\tread_cache_from(false_lock.staging_filename.buf);\n \n-\treturn false_lock.lock_filename.buf;\n+\treturn false_lock.staging_filename.buf;\n }\n \n static int run_status(FILE *fp, const char *index_file, const char *prefix, int nowarn,\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 29bc07b..08fd6b3 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -431,7 +431,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\n \t\t\t write_str_in_full(lock->lock_fd, \"\\n\") != 1 ||\n \t\t\t close_ref(lock) < 0)) {\n \t\t\tstatus |= error(\"Couldn't write %s\",\n-\t\t\t\tlock->lk->lock_filename.buf);\n+\t\t\t\tlock->lk->staging_filename.buf);\n \t\t\tunlink(newlog_path);\n \t\t} else if (rename(newlog_path, log_file)) {\n \t\t\tstatus |= error(\"cannot rename %s to %s\",\ndiff --git a/cache.h b/cache.h\nindex 0fefc66..3b9dbd4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -539,7 +539,7 @@ struct lock_file {\n \tpid_t owner;\n \tunsigned char flags;\n \tstruct strbuf filename;\n-\tstruct strbuf lock_filename;\n+\tstruct strbuf staging_filename;\n };\n #define LOCK_DIE_ON_ERROR 1\n #define LOCK_NODEREF 2\ndiff --git a/config.c b/config.c\nindex cea0af0..c39bd31 100644\n--- a/config.c\n+++ b/config.c\n@@ -1306,7 +1306,7 @@ static int store_aux(const char *key, const char *value, void *cb)\n static int write_error(struct lock_file *lk)\n {\n \terror(\"failed to write new configuration file %s\",\n-\t lk->lock_filename.buf);\n+\t lk->staging_filename.buf);\n \n \t/* Same error code as \"failed to rename\". */\n \treturn 4;\ndiff --git a/lockfile.c b/lockfile.c\nindex 07b5c21..3974137 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -35,26 +35,26 @@\n *\n * - Uninitialized. In this state the object's flags field must be\n * zero but the rest of the contents need not be initialized. In\n- * particular, the filename and lock_filename strbufs should *not*\n- * be initialized externally. The first time the object is used in\n- * any way, it is initialized, permanently registered in the\n+ * particular, the filename and staging_filename strbufs should\n+ * *not* be initialized externally. The first time the object is\n+ * used in any way, it is initialized, permanently registered in the\n * lock_file_list, and flags & LOCK_FLAGS_ON_LIST is set.\n *\n * - Locked, lockfile open (after hold_lock_file_for_update() or\n * hold_lock_file_for_append()). In this state, the lockfile\n * exists, filename holds the filename of the locked file,\n- * lock_filename holds the filename of the lockfile, fd holds a file\n- * descriptor open for writing to the lockfile, and owner holds the\n- * PID of the process that locked the file.\n+ * staging_filename holds the filename of the lockfile, fd holds a\n+ * file descriptor open for writing to the lockfile, and owner holds\n+ * the PID of the process that locked the file.\n *\n * - Locked, lockfile closed (after close_lock_file()). Same as the\n * previous state, except that the lockfile is closed and fd is -1.\n *\n * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a\n * failed attempt to lock). In this state, filename and\n- * lock_filename are the empty string and fd is -1. The object is\n- * left registered in the lock_file_list, and flags &\n- * LOCK_FLAGS_ON_LIST is set.\n+ * staging_filename are the empty string and fd is -1. The object is left\n+ * registered in the lock_file_list, and flags & LOCK_FLAGS_ON_LIST\n+ * is set.\n *\n * See Documentation/api-lockfile.txt for more information.\n */\n@@ -73,10 +73,10 @@ static void remove_lock_file(void)\n \n \twhile (lock_file_list) {\n \t\tif (lock_file_list->owner == me &&\n-\t\t lock_file_list->lock_filename.len) {\n+\t\t lock_file_list->staging_filename.len) {\n \t\t\tif (lock_file_list->fd >= 0)\n \t\t\t\tclose(lock_file_list->fd);\n-\t\t\tunlink_or_warn(lock_file_list->lock_filename.buf);\n+\t\t\tunlink_or_warn(lock_file_list->staging_filename.buf);\n \t\t}\n \t\tlock_file_list = lock_file_list->next;\n \t}\n@@ -168,13 +168,13 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \tif (lk->flags & LOCK_FLAGS_ON_LIST) {\n \t\t/* Sanity check that object is not already in use: */\n \t\tassert(!lk->filename.len);\n-\t\tassert(!lk->lock_filename.len);\n+\t\tassert(!lk->staging_filename.len);\n \t} else {\n \t\t/* Initialize *lk and add it to lock_file_list: */\n \t\tlk->fd = -1;\n \t\tlk->flags |= LOCK_FLAGS_ON_LIST;\n \t\tstrbuf_init(&lk->filename, path_len);\n-\t\tstrbuf_init(&lk->lock_filename, 0);\n+\t\tstrbuf_init(&lk->staging_filename, 0);\n \t\tlk->next = lock_file_list;\n \t\tlock_file_list = lk;\n \t}\n@@ -183,18 +183,19 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \tif (!(flags & LOCK_NODEREF))\n \t\tresolve_symlink(&lk->filename);\n \n-\tstrbuf_grow(&lk->lock_filename, lk->filename.len + LOCK_SUFFIX_LEN);\n-\tstrbuf_addbuf(&lk->lock_filename, &lk->filename);\n-\tstrbuf_addstr(&lk->lock_filename, \".lock\");\n+\tstrbuf_grow(&lk->staging_filename, lk->filename.len + LOCK_SUFFIX_LEN);\n+\tstrbuf_addbuf(&lk->staging_filename, &lk->filename);\n+\tstrbuf_addstr(&lk->staging_filename, \".lock\");\n \n-\tlk->fd = open(lk->lock_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n+\tlk->fd = open(lk->staging_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (lk->fd < 0) {\n \t\tstrbuf_setlen(&lk->filename, 0);\n-\t\tstrbuf_setlen(&lk->lock_filename, 0);\n+\t\tstrbuf_setlen(&lk->staging_filename, 0);\n \t\treturn -1;\n \t}\n-\tif (adjust_shared_perm(lk->lock_filename.buf)) {\n-\t\terror(\"cannot fix permission bits on %s\", lk->lock_filename.buf);\n+\tif (adjust_shared_perm(lk->staging_filename.buf)) {\n+\t\terror(\"cannot fix permission bits on %s\",\n+\t\t lk->staging_filename.buf);\n \t\trollback_lock_file(lk);\n \t\treturn -1;\n \t}\n@@ -280,11 +281,11 @@ int commit_lock_file(struct lock_file *lk)\n \n \tif (lk->fd >= 0 && close_lock_file(lk))\n \t\treturn -1;\n-\tif (rename(lk->lock_filename.buf, lk->filename.buf)) {\n+\tif (rename(lk->staging_filename.buf, lk->filename.buf)) {\n \t\terr = -1;\n \t} else {\n \t\tstrbuf_setlen(&lk->filename, 0);\n-\t\tstrbuf_setlen(&lk->lock_filename, 0);\n+\t\tstrbuf_setlen(&lk->staging_filename, 0);\n \t}\n \treturn err;\n }\n@@ -307,10 +308,10 @@ int commit_locked_index(struct lock_file *lk)\n \tif (alternate_index_output) {\n \t\tif (lk->fd >= 0 && close_lock_file(lk))\n \t\t\treturn -1;\n-\t\tif (rename(lk->lock_filename.buf, alternate_index_output))\n+\t\tif (rename(lk->staging_filename.buf, alternate_index_output))\n \t\t\treturn -1;\n \t\tstrbuf_setlen(&lk->filename, 0);\n-\t\tstrbuf_setlen(&lk->lock_filename, 0);\n+\t\tstrbuf_setlen(&lk->staging_filename, 0);\n \t\treturn 0;\n \t} else {\n \t\treturn commit_lock_file(lk);\n@@ -322,8 +323,8 @@ void rollback_lock_file(struct lock_file *lk)\n \tif (lk->filename.len) {\n \t\tif (lk->fd >= 0)\n \t\t\tclose_lock_file(lk);\n-\t\tunlink_or_warn(lk->lock_filename.buf);\n+\t\tunlink_or_warn(lk->staging_filename.buf);\n \t\tstrbuf_setlen(&lk->filename, 0);\n-\t\tstrbuf_setlen(&lk->lock_filename, 0);\n+\t\tstrbuf_setlen(&lk->staging_filename, 0);\n \t}\n }\ndiff --git a/refs.c b/refs.c\nindex 85967e7..47a49a0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2822,7 +2822,7 @@ int write_ref_sha1(struct ref_lock *lock,\n \tif (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||\n \t write_in_full(lock->lock_fd, &term, 1) != 1\n \t\t|| close_ref(lock) < 0) {\n-\t\terror(\"Couldn't write %s\", lock->lk->lock_filename.buf);\n+\t\terror(\"Couldn't write %s\", lock->lk->staging_filename.buf);\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\ndiff --git a/shallow.c b/shallow.c\nindex a27d587..76dc6df 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -269,8 +269,8 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,\n \tif (write_shallow_commits(&sb, 0, extra)) {\n \t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n \t\t\tdie_errno(\"failed to write to %s\",\n-\t\t\t\t shallow_lock->lock_filename.buf);\n-\t\t*alternate_shallow_file = shallow_lock->lock_filename.buf;\n+\t\t\t\t shallow_lock->staging_filename.buf);\n+\t\t*alternate_shallow_file = shallow_lock->staging_filename.buf;\n \t} else\n \t\t/*\n \t\t * is_repository_shallow() sees empty string as \"no\n@@ -316,7 +316,7 @@ void prune_shallow(int show_only)\n \tif (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {\n \t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n \t\t\tdie_errno(\"failed to write to %s\",\n-\t\t\t\t shallow_lock.lock_filename.buf);\n+\t\t\t\t shallow_lock.staging_filename.buf);\n \t\tcommit_lock_file(&shallow_lock);\n \t} else {\n \t\tunlink(git_path(\"shallow\"));\n-- \n1.9.0\n"},{"id":"238212","messageId":"1396367910-7299-21-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 20/22] remove_lock_file(): call rollback_lock_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:28Z","receivedAt":"2014-04-01T15:58:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It does exactly what we need.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 3974137..852d717 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -72,12 +72,8 @@ static void remove_lock_file(void)\n \tpid_t me = getpid();\n \n \twhile (lock_file_list) {\n-\t\tif (lock_file_list->owner == me &&\n-\t\t lock_file_list->staging_filename.len) {\n-\t\t\tif (lock_file_list->fd >= 0)\n-\t\t\t\tclose(lock_file_list->fd);\n-\t\t\tunlink_or_warn(lock_file_list->staging_filename.buf);\n-\t\t}\n+\t\tif (lock_file_list->owner == me)\n+\t\t\trollback_lock_file(lock_file_list);\n \t\tlock_file_list = lock_file_list->next;\n \t}\n }\n-- \n1.9.0\n"},{"id":"238213","messageId":"1396367910-7299-22-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 21/22] lockfile: extract a function reset_lock_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:29Z","receivedAt":"2014-04-01T15:58:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty \n---\n lockfile.c | 31 ++++++++++++++++---------------\n 1 file changed, 16 insertions(+), 15 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 852d717..c06e134 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -85,6 +85,14 @@ static void remove_lock_file_on_signal(int signo)\n \traise(signo);\n }\n \n+static void reset_lock_file(struct lock_file *lk)\n+{\n+\tlk->fd = -1;\n+\tstrbuf_setlen(&lk->filename, 0);\n+\tstrbuf_setlen(&lk->staging_filename, 0);\n+\tlk->flags = LOCK_FLAGS_ON_LIST;\n+}\n+\n /*\n * path = absolute or relative path name\n *\n@@ -185,8 +193,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \n \tlk->fd = open(lk->staging_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (lk->fd < 0) {\n-\t\tstrbuf_setlen(&lk->filename, 0);\n-\t\tstrbuf_setlen(&lk->staging_filename, 0);\n+\t\treset_lock_file(lk);\n \t\treturn -1;\n \t}\n \tif (adjust_shared_perm(lk->staging_filename.buf)) {\n@@ -273,17 +280,12 @@ int close_lock_file(struct lock_file *lk)\n \n int commit_lock_file(struct lock_file *lk)\n {\n-\tint err = 0;\n-\n \tif (lk->fd >= 0 && close_lock_file(lk))\n \t\treturn -1;\n-\tif (rename(lk->staging_filename.buf, lk->filename.buf)) {\n-\t\terr = -1;\n-\t} else {\n-\t\tstrbuf_setlen(&lk->filename, 0);\n-\t\tstrbuf_setlen(&lk->staging_filename, 0);\n-\t}\n-\treturn err;\n+\tif (rename(lk->staging_filename.buf, lk->filename.buf))\n+\t\treturn -1;\n+\treset_lock_file(lk);\n+\treturn 0;\n }\n \n int hold_locked_index(struct lock_file *lk, int die_on_error)\n@@ -306,8 +308,8 @@ int commit_locked_index(struct lock_file *lk)\n \t\t\treturn -1;\n \t\tif (rename(lk->staging_filename.buf, alternate_index_output))\n \t\t\treturn -1;\n-\t\tstrbuf_setlen(&lk->filename, 0);\n-\t\tstrbuf_setlen(&lk->staging_filename, 0);\n+\n+\t\treset_lock_file(lk);\n \t\treturn 0;\n \t} else {\n \t\treturn commit_lock_file(lk);\n@@ -320,7 +322,6 @@ void rollback_lock_file(struct lock_file *lk)\n \t\tif (lk->fd >= 0)\n \t\t\tclose_lock_file(lk);\n \t\tunlink_or_warn(lk->staging_filename.buf);\n-\t\tstrbuf_setlen(&lk->filename, 0);\n-\t\tstrbuf_setlen(&lk->staging_filename, 0);\n+\t\treset_lock_file(lk);\n \t}\n }\n-- \n1.9.0\n"},{"id":"238210","messageId":"1396367910-7299-23-git-send-email-mhagger@alum.mit.edu","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 22/22] lockfile: allow new file contents to be written while retaining lock","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T15:58:30Z","receivedAt":"2014-04-01T15:58:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add a new option flag, LOCK_SEPARATE_STAGING_FILE, that can be passed\nto hold_lock_file_for_update() or hold_lock_file_for_append() to use a\nstaging file that is independent of the lock file.\n\nAdd a new function activate_staging_file() that activates the contents\nthat have been written to the staging file without releasing the lock.\n\nThis functionality can be used to ensure that changes to two files are\nseen by other processes in one order even if correctness requires the\nlocks to be released in another order.\n\nSigned-off-by: Michael Haggerty \n---\n Documentation/technical/api-lockfile.txt | 54 ++++++++----\n cache.h | 4 +\n lockfile.c | 140 ++++++++++++++++++++++++++-----\n 3 files changed, 162 insertions(+), 36 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex 95ed03b..06b49f9 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -43,6 +43,13 @@ LOCK_DIE_ON_ERROR::\n \tIf a lock is already taken for the file, `die()` with an error\n \tmessage. If this option is not specified, return a negative\n \tinteger to the caller on failure.\n+\n+LOCK_SEPARATE_STAGING_FILE::\n+\n+\tUse a separate file (not the lockfile) for staging the new\n+\tfile contents. This option makes it possible to call\n+\tactivate_staging_file() to overwrite the old file while\n+\tretaining the file lock.\n --\n \n hold_lock_file_for_append::\n@@ -64,23 +71,39 @@ unable_to_lock_index_die::\n \n commit_lock_file::\n \n-\tTake a pointer to the `struct lock_file` initialized\n-\twith an earlier call to `hold_lock_file_for_update()`,\n-\tclose the file descriptor and rename the lockfile to its\n-\tfinal destination. Returns 0 upon success, a negative\n-\tvalue on failure to close(2) or rename(2).\n+\tTake a pointer to the `struct lock_file` initialized with an\n+\tearlier call to `hold_lock_file_for_update()` or\n+\t`hold_lock_file_for_append()`. Close the file descriptor.\n+\tThen, if the lock was created with the\n+\tLOCK_SEPARATE_STAGING_FILE flag, rename the staging file on\n+\ttop of the locked file (if `activate_staging_file()` hasn't\n+\talready been called) and delete the lockfile; otherwise,\n+\trename the lockfile to its final destination. Returns 0 upon\n+\tsuccess, a negative value on failure to close(2) or rename(2).\n+\n+activate_staging_file::\n+\n+\tWrite the contents of the separate staging file over those of\n+\tthe locked file while retaining the lock. This function may\n+\tonly be called if the lock was created with the\n+\tLOCK_SEPARATE_STAGING_FILE flag.\n \n rollback_lock_file::\n \n-\tTake a pointer to the `struct lock_file` initialized\n-\twith an earlier call to `hold_lock_file_for_update()`,\n-\tclose the file descriptor and remove the lockfile.\n+\tTake a pointer to the `struct lock_file` initialized with an\n+\tearlier call to `hold_lock_file_for_update()`, close the file\n+\tdescriptor, remove the separate staging file (if any), and\n+\tremove the lockfile. Please note that if\n+\t`activate_staging_file()` has already been called, then the\n+\tcontents committed at that time will remain active.\n \n close_lock_file::\n \tTake a pointer to the `struct lock_file` initialized\n \twith an earlier call to `hold_lock_file_for_update()`,\n \tand close the file descriptor. Returns 0 upon success,\n-\ta negative value on failure to close(2).\n+\ta negative value on failure to close(2). This function must\n+\tonly be called if the lock was created without the\n+\tLOCK_SEPARATE_STAGING_FILE flag.\n \n Because the structure is used in an `atexit(3)` handler, its\n storage has to stay throughout the life of the program. It\n@@ -94,11 +117,10 @@ will close and remove the lockfile.\n If you need to close the file descriptor you obtained from\n `hold_lock_file_for_update` function yourself, do so by calling\n `close_lock_file()`. You should never call `close(2)` yourself!\n-Otherwise the `struct\n-lock_file` structure still remembers that the file descriptor\n-needs to be closed, and a later call to `commit_lock_file()` or\n-`rollback_lock_file()` will result in duplicate calls to\n-`close(2)`. Worse yet, if you `close(2)`, open another file\n-descriptor for completely different purpose, and then call\n-`commit_lock_file()` or `rollback_lock_file()`, they may close\n+Otherwise the `struct lock_file` structure still remembers that the\n+file descriptor needs to be closed, and a later call to\n+`commit_lock_file()` or `rollback_lock_file()` will result in\n+duplicate calls to `close(2)`. Worse yet, if you `close(2)`, open\n+another file descriptor for completely different purpose, and then\n+call `commit_lock_file()` or `rollback_lock_file()`, they may close\n that unrelated file descriptor.\ndiff --git a/cache.h b/cache.h\nindex 3b9dbd4..50fdc68 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -541,12 +541,16 @@ struct lock_file {\n \tstruct strbuf filename;\n \tstruct strbuf staging_filename;\n };\n+\n #define LOCK_DIE_ON_ERROR 1\n #define LOCK_NODEREF 2\n+#define LOCK_SEPARATE_STAGING_FILE 4\n+\n extern int unable_to_lock_error(const char *path, int err);\n extern NORETURN void unable_to_lock_index_die(const char *path, int err);\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 activate_staging_file(struct lock_file *);\n extern int commit_lock_file(struct lock_file *);\n extern void update_index_if_able(struct index_state *, struct lock_file *);\n \ndiff --git a/lockfile.c b/lockfile.c\nindex c06e134..336b914 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -40,8 +40,16 @@\n * used in any way, it is initialized, permanently registered in the\n * lock_file_list, and flags & LOCK_FLAGS_ON_LIST is set.\n *\n- * - Locked, lockfile open (after hold_lock_file_for_update() or\n- * hold_lock_file_for_append()). In this state, the lockfile\n+ * - Initialized but unlocked (after commit_lock_file(),\n+ * rollback_lock_file(), or a failed attempt to lock). In this\n+ * state, filename and staging_filename are the empty string. The\n+ * object is left registered in the lock_file_list, and flags ==\n+ * LOCK_FLAGS_ON_LIST.\n+ *\n+ * - Locked, lockfile open for writing (after\n+ * hold_lock_file_for_update() or hold_lock_file_for_append()\n+ * without the LOCK_SEPARATE_STAGING_FILE flag). In this state,\n+ * flags & LOCK_FLAGS_SEPARATE_STAGING_FILE is not set, the lockfile\n * exists, filename holds the filename of the locked file,\n * staging_filename holds the filename of the lockfile, fd holds a\n * file descriptor open for writing to the lockfile, and owner holds\n@@ -50,11 +58,21 @@\n * - Locked, lockfile closed (after close_lock_file()). Same as the\n * previous state, except that the lockfile is closed and fd is -1.\n *\n- * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a\n- * failed attempt to lock). In this state, filename and\n- * staging_filename are the empty string and fd is -1. The object is left\n- * registered in the lock_file_list, and flags & LOCK_FLAGS_ON_LIST\n- * is set.\n+ * - Locked, separate staging file open for writing (after\n+ * hold_lock_file_for_update() or hold_lock_file_for_append() with\n+ * the LOCK_SEPARATE_STAGING_FILE flag). In this state, flags &\n+ * LOCK_FLAGS_SEPARATE_STAGING_FILE is set, the lockfile exists but\n+ * is closed, filename holds the filename of the locked file,\n+ * staging_filename holds the filename of the separate staging file,\n+ * fd holds a file descriptor open for writing to the separate\n+ * staging file, and owner holds the PID of the process that locked\n+ * the file.\n+ *\n+ * - Locked, staging file closed and activated (after\n+ * activate_staging_file()). Same as the previous state, except\n+ * that the separate staging file has already been closed and\n+ * renamed on top of the file, staging_filename is the empty string,\n+ * and fd is -1.\n *\n * See Documentation/api-lockfile.txt for more information.\n */\n@@ -64,6 +82,9 @@\n /* This lock_file instance is in the lock_file_list */\n #define LOCK_FLAGS_ON_LIST 0x01\n \n+/* A separate staging file is being used */\n+#define LOCK_FLAGS_SEPARATE_STAGING_FILE 0x02\n+\n static struct lock_file *lock_file_list;\n static const char *alternate_index_output;\n \n@@ -155,9 +176,31 @@ static void resolve_symlink(struct strbuf *path)\n \tstrbuf_release(&link);\n }\n \n-/* We append \".lock\" to the filename to derive the lockfile name: */\n+/*\n+ * We append \".lock\" to the filename to derive the lockfile name, and\n+ * \".new\" to derive the staging file name. The longer of the two:\n+ */\n #define LOCK_SUFFIX_LEN 5\n \n+static int open_staging_file(struct lock_file *lk)\n+{\n+\tstrbuf_setlen(&lk->staging_filename, lk->filename.len);\n+\tstrbuf_addstr(&lk->staging_filename, \".new\");\n+\tlk->fd = open(lk->staging_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n+\tif (lk->fd < 0) {\n+\t\treturn -1;\n+\t}\n+\tif (adjust_shared_perm(lk->staging_filename.buf)) {\n+\t\terror(\"cannot fix permission bits on %s\", lk->staging_filename.buf);\n+\t\tif (close(lk->fd))\n+\t\t\terror(\"cannot close staging file %s\", lk->staging_filename.buf);\n+\t\tlk->fd = -1;\n+\t\tunlink_or_warn(lk->staging_filename.buf);\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n+}\n+\n static int lock_file(struct lock_file *lk, const char *path, int flags)\n {\n \tsize_t path_len = strlen(path);\n@@ -199,11 +242,22 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \tif (adjust_shared_perm(lk->staging_filename.buf)) {\n \t\terror(\"cannot fix permission bits on %s\",\n \t\t lk->staging_filename.buf);\n-\t\trollback_lock_file(lk);\n-\t\treturn -1;\n+\t\tgoto rollback_and_fail;\n+\t}\n+\tif (flags & LOCK_SEPARATE_STAGING_FILE) {\n+\t\tif (close_lock_file(lk))\n+\t\t\tgoto rollback_and_fail;\n+\n+\t\tlk->flags |= LOCK_FLAGS_SEPARATE_STAGING_FILE;\n+\t\tif (open_staging_file(lk))\n+\t\t\tgoto rollback_and_fail;\n \t}\n \n \treturn lk->fd;\n+\n+rollback_and_fail:\n+\trollback_lock_file(lk);\n+\treturn -1;\n }\n \n static char *unable_to_lock_message(const char *path, int err)\n@@ -271,19 +325,54 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n \treturn fd;\n }\n \n-int close_lock_file(struct lock_file *lk)\n+static int close_staging_file(struct lock_file *lk)\n {\n \tint fd = lk->fd;\n+\n \tlk->fd = -1;\n \treturn close(fd);\n }\n \n-int commit_lock_file(struct lock_file *lk)\n+int close_lock_file(struct lock_file *lk)\n {\n-\tif (lk->fd >= 0 && close_lock_file(lk))\n-\t\treturn -1;\n-\tif (rename(lk->staging_filename.buf, lk->filename.buf))\n+\tassert(!(lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE));\n+\treturn close_staging_file(lk);\n+}\n+\n+int activate_staging_file(struct lock_file *lk)\n+{\n+\tint err;\n+\n+\tassert(lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE);\n+\tassert(lk->fd >= 0);\n+\tassert(lk->staging_filename.len);\n+\n+\tif (close_staging_file(lk))\n \t\treturn -1;\n+\n+\terr = rename(lk->staging_filename.buf, lk->filename.buf);\n+\tstrbuf_setlen(&lk->staging_filename, 0);\n+\n+\treturn err;\n+}\n+\n+int commit_lock_file(struct lock_file *lk)\n+{\n+\tif (lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE) {\n+\t\tif (lk->staging_filename.len) {\n+\t\t\tassert(lk->fd >= 0);\n+\t\t\tif (activate_staging_file(lk))\n+\t\t\t\treturn -1;\n+\t\t}\n+\t\tstrbuf_addbuf(&lk->staging_filename, &lk->filename);\n+\t\tstrbuf_addstr(&lk->staging_filename, \".lock\");\n+\t\tunlink_or_warn(lk->staging_filename.buf);\n+\t} else {\n+\t\tif (lk->fd >= 0 && close_lock_file(lk))\n+\t\t\treturn -1;\n+\t\tif (rename(lk->staging_filename.buf, lk->filename.buf))\n+\t\t\treturn -1;\n+\t}\n \treset_lock_file(lk);\n \treturn 0;\n }\n@@ -318,10 +407,21 @@ int commit_locked_index(struct lock_file *lk)\n \n void rollback_lock_file(struct lock_file *lk)\n {\n-\tif (lk->filename.len) {\n-\t\tif (lk->fd >= 0)\n-\t\t\tclose_lock_file(lk);\n-\t\tunlink_or_warn(lk->staging_filename.buf);\n-\t\treset_lock_file(lk);\n+\tif (!lk->filename.len)\n+\t\treturn;\n+\n+\tif (lk->fd >= 0)\n+\t\tclose_staging_file(lk);\n+\n+\tif (lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE) {\n+\t\tif (lk->staging_filename.len) {\n+\t\t\tunlink_or_warn(lk->staging_filename.buf);\n+\t\t\tstrbuf_setlen(&lk->staging_filename, 0);\n+\t\t}\n+\t\tstrbuf_addbuf(&lk->staging_filename, &lk->filename);\n+\t\tstrbuf_addstr(&lk->staging_filename, \".lock\");\n \t}\n+\n+\tunlink_or_warn(lk->staging_filename.buf);\n+\treset_lock_file(lk);\n }\n-- \n1.9.0\n"},{"id":"238256","messageId":"20140401195338.GA21715@sigill.intra.peff.net","threadId":"36333","inReplyTo":"1396367910-7299-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 01/22] t3204: test deleting references when lock files already exist","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-01T19:53:39Z","receivedAt":"2014-04-01T19:53:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 01, 2014 at 05:58:09PM +0200, Michael Haggerty wrote:\n\n> When deleting a reference, it might be that another process already\n> holds the lock on the loose reference file and/or the packed-refs\n> file. In those cases, there is no alternative but for the delete to\n> fail. Verify that in such cases the reference values are left\n> unchanged.\n> \n> But in fact, when the packed-refs file lock cannot be acquired, the\n> loose reference file is deleted anyway, potentially leaving the\n> reference with a changed value (its packed value, which might even\n> point at an object that has been garbage collected). Thus one of the\n> new tests is marked test_expect_failure.\n\nNice find. If I understand correctly, the problem is at the plumbing\nlevel, and this could also be demonstrated with update-ref?\n\nI don't think it is a big deal, but I was thrown for a minute by the use\nof \"git branch\" (as in, \"is this something special with branches, or is\nthis about all refs?\").\n\n-Peff\n"},{"id":"238260","messageId":"20140401195615.GB21715@sigill.intra.peff.net","threadId":"36333","inReplyTo":"1396367910-7299-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 02/22] try_merge_strategy(): remove redundant lock_file allocation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-01T19:56:15Z","receivedAt":"2014-04-01T19:56:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 01, 2014 at 05:58:10PM +0200, Michael Haggerty wrote:\n\n> By the time the \"if\" block is entered, the lock_file instance from the\n> main function block is no longer in use, so re-use that one instead of\n> allocating a second one.\n> \n> Note that the \"lock\" variable in the \"if\" block used to shadow the\n> \"lock\" variable at function scope, so the only change needed is to\n> remove the inner definition.\n\nI wonder if this would also be simpler if \"lock\" were simply declared as\na static variable, and we drop the allocation entirely. I suppose that\ndoes create more cognitive load, though, in that it is only correct if\nthe function is not recursive. On the other hand, the current code makes\na reader unfamiliar with \"struct lock\" wonder if there is a free(lock)\nmissing.\n\n-Peff\n"},{"id":"238261","messageId":"20140401195930.GC21715@sigill.intra.peff.net","threadId":"36333","inReplyTo":"1396367910-7299-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 04/22] rollback_lock_file(): set fd to -1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-01T19:59:30Z","receivedAt":"2014-04-01T19:59:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 01, 2014 at 05:58:12PM +0200, Michael Haggerty wrote:\n\n> When rolling back the lockfile, call close_lock_file() so that the\n> lock_file's fd field gets set back to -1. This could help prevent\n> confusion in the face of hypothetical future programming errors.\n\nThis also solves a race. We could be in the middle of rollback_lock_file\nwhen we get a signal, and double-close. It's probably not a big deal,\nthough, since nobody could have opened a new descriptor in the interim\nthat got the same number (so the second close will just fail silently).\n\nStill, this seems like a definite improvement.\n\n-Peff\n"},{"id":"238262","messageId":"20140401200242.GD21715@sigill.intra.peff.net","threadId":"36333","inReplyTo":"1396367910-7299-6-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 05/22] lockfile: unlock file if lockfile permissions cannot be adjusted","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-01T20:02:42Z","receivedAt":"2014-04-01T20:02:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 01, 2014 at 05:58:13PM +0200, Michael Haggerty wrote:\n\n> If the call to adjust_shared_perm() fails, lock_file returns -1, which\n> to the caller looks like any other failure to lock the file. So in\n> this case, roll back the lockfile before returning so that the lock\n> file is deleted immediately and the lockfile object is left in a\n> predictable state that (namely, unlocked). Previously, the lockfile\n> was retained until process cleanup in this situation.\n\nAnother nice find. I wondered if we could test this, but I think it\nwould be hard to trigger. The obvious reason for adjust_shared_perm to\nfail is that you do not have permissions on the file, but by definition\nyou just created it. So I doubt this ever comes up in practice short of\nweird races (somebody dropping the \"x\" bit from an intermediate\ndirectory between the open() and chmod() or something).\n\n-Peff\n"},{"id":"238264","messageId":"20140401200559.GA22174@sigill.intra.peff.net","threadId":"36333","inReplyTo":"20140401200242.GD21715@sigill.intra.peff.net","subject":"Re: [PATCH 05/22] lockfile: unlock file if lockfile permissions cannot be adjusted","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-01T20:05:59Z","receivedAt":"2014-04-01T20:05:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 01, 2014 at 04:02:42PM -0400, Jeff King wrote:\n\n> On Tue, Apr 01, 2014 at 05:58:13PM +0200, Michael Haggerty wrote:\n> \n> > If the call to adjust_shared_perm() fails, lock_file returns -1, which\n> > to the caller looks like any other failure to lock the file. So in\n> > this case, roll back the lockfile before returning so that the lock\n> > file is deleted immediately and the lockfile object is left in a\n> > predictable state that (namely, unlocked). Previously, the lockfile\n> > was retained until process cleanup in this situation.\n> \n> Another nice find. I wondered if we could test this, but I think it\n> would be hard to trigger. The obvious reason for adjust_shared_perm to\n> fail is that you do not have permissions on the file, but by definition\n> you just created it. So I doubt this ever comes up in practice short of\n> weird races (somebody dropping the \"x\" bit from an intermediate\n> directory between the open() and chmod() or something).\n\n...and I should have read the final sentence in your message more\ncarefully. Even if we did trigger it, the problem would only last until\nthe program exits anyway.\n\nI agree that this is a nice cleanup, though; a caller that wants to\nretry the lock before exiting would be much less surprised. And the same\nlogic applies to 06/22.\n\n-Peff\n"},{"id":"238266","messageId":"20140401201659.GE21715@sigill.intra.peff.net","threadId":"36333","inReplyTo":"1396367910-7299-8-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 07/22] lock_file(): always add lock_file object to lock_file_list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-01T20:16:59Z","receivedAt":"2014-04-01T20:16:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 01, 2014 at 05:58:15PM +0200, Michael Haggerty wrote:\n\n> diff --git a/lockfile.c b/lockfile.c\n> index e679e4c..c989f6c 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -130,6 +130,22 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n> \t */\n> \tstatic const size_t max_path_len = sizeof(lk->filename) - 5;\n> \n> +\tif (!lock_file_list) {\n> +\t\t/* One-time initialization */\n> +\t\tsigchain_push_common(remove_lock_file_on_signal);\n> +\t\tatexit(remove_lock_file);\n> +\t}\n> +\n> +\tlk->owner = getpid();\n> +\tif (!lk->on_list) {\n> +\t\t/* Initialize *lk and add it to lock_file_list: */\n> +\t\tlk->fd = -1;\n> +\t\tlk->on_list = 1;\n> +\t\tlk->filename[0] = 0;\n> +\t\tlk->next = lock_file_list;\n> +\t\tlock_file_list = lk;\n> +\t}\n\nInitializing here is good, since we might be interrupted by a signal at\nany time. But what about during the locking procedure? We do:\n\n strcpy(lk->filename, path);\n if (!(flags & LOCK_NODEREF))\n resolve_symlink(lk->filename, max_path_len);\n strcat(lk->filename, \".lock\");\n\nSo for a moment, lk->filename contains the name of the valuable file we\nare locking. If we get a signal at that moment, do we accidentally\ndelete it in remove_lock_file?\n\nI think the answer is \"no\", because we check lk->owner before deleting,\nwhich will not match our pid (it should generally be zero due to xcalloc\nor static initialization, though perhaps we should clear it here).\n\nBut that makes me wonder about the case of a reused lock. It will have\nlk->owner set from a previous invocation, and would potentially suffer\nfrom this problem. In other words, I think the change you are\nintroducing does not have the problem, but the existing code does. :-/\n\nI didn't reproduce it experimentally, though. We should be able to just\n\n lk->owner = 0;\n\nbefore the initial strcpy to fix it, I would think.\n\n-Peff\n"},{"id":"238267","messageId":"20140401201906.GF21715@sigill.intra.peff.net","threadId":"36333","inReplyTo":"1396367910-7299-10-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 09/22] api-lockfile: expand the documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-01T20:19:06Z","receivedAt":"2014-04-01T20:19:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 01, 2014 at 05:58:17PM +0200, Michael Haggerty wrote:\n\n> +unable_to_lock_error::\n> +\n> +\tEmit an error describing that there was an error locking the\n> +\tspecified path. The err parameter should be the errno of the\n> +\tproblem that caused the failure.\n> +\n> +unable_to_lock_index_die::\n> +\n> +\tLike `unable_to_lock_error()`, but also `die()`.\n\nShould this last one lost the \"index\" in its name? I think it is\nvestigial at this point.\n\n-Peff\n"},{"id":"238268","messageId":"20140401202118.GG21715@sigill.intra.peff.net","threadId":"36333","inReplyTo":"1396367910-7299-13-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 12/22] delete_ref_loose(): don't muck around in the lock_file's filename","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-01T20:21:18Z","receivedAt":"2014-04-01T20:21:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 01, 2014 at 05:58:20PM +0200, Michael Haggerty wrote:\n\n> It's bad manners. Especially since, if unlink_or_warn() failed, the\n> memory wasn't restored to its original contents.\n> \n> So make our own copy to work with.\n\nSounds good...\n\n> \tif (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n> -\t\t/* loose */\n> -\t\tint err, i = strlen(lock->lk->filename) - 5; /* .lock */\n> -\n> -\t\tlock->lk->filename[i] = 0;\n> -\t\terr = unlink_or_warn(lock->lk->filename);\n> -\t\tlock->lk->filename[i] = '.';\n> +\t\t/*\n> +\t\t * loose. The loose file name is the same as the\n> +\t\t * lockfile name, minus \".lock\":\n> +\t\t */\n> +\t\tchar *loose_filename = xmemdupz(lock->lk->filename,\n> +\t\t\t\t\t\tstrlen(lock->lk->filename) - 5);\n> +\t\tint err = unlink_or_warn(loose_filename);\n> +\t\tfree(loose_filename);\n\nShould we be using LOCK_SUFFIX_LEN from the previous commit here?\n\n-Peff\n"},{"id":"238270","messageId":"20140401202811.GH21715@sigill.intra.peff.net","threadId":"36333","inReplyTo":"1396367910-7299-15-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 14/22] lockfile: use strbufs when handling (most) paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-01T20:28:11Z","receivedAt":"2014-04-01T20:28:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 01, 2014 at 05:58:22PM +0200, Michael Haggerty wrote:\n\n> /*\n> - * p = path that may be a symlink\n> - * s = full size of p\n> - *\n> - * If p is a symlink, attempt to overwrite p with a path to the real\n> - * file or directory (which may or may not exist), following a chain of\n> - * symlinks if necessary. Otherwise, leave p unmodified.\n> + * path contains a path that may be a symlink\n> *\n> - * This is a best-effort routine. If an error occurs, p will either be\n> - * left unmodified or will name a different symlink in a symlink chain\n> - * that started with p's initial contents.\n> + * If path is a symlink, attempt to overwrite it with a path to the\n> + * real file or directory (which may or may not exist), following a\n> + * chain of symlinks if necessary. Otherwise, leave path unmodified.\n> *\n> - * Always returns p.\n> + * This is a best-effort routine. If an error occurs, path will\n> + * either be left unmodified or will name a different symlink in a\n> + * symlink chain that started with path's initial contents.\n> */\n> -\n> -static char *resolve_symlink(char *p, size_t s)\n> +static void resolve_symlink(struct strbuf *path)\n> [...]\n\nThis is not a problem you are introducing, but do we really want\nresolve_symlink to be best-effort here?\n\nWhat happens if \"a\" is a symlink to \"b\", and two processes try to lock\n\"a\" simultaneously? If one succeeds, it will take \"b.lock\". But the\nother will take \"a.lock\", and both will think they have the target\nlocked.\n\nI suspect we need to recognize ENOENT for cases where we are creating\nthe file for the first time, but it seems like we should bail on locking\nfrom any other transient errors.\n\n-Peff\n"},{"id":"238271","messageId":"20140401203900.GI21715@sigill.intra.peff.net","threadId":"36333","inReplyTo":"1396367910-7299-23-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 22/22] lockfile: allow new file contents to be written while retaining lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-01T20:39:00Z","receivedAt":"2014-04-01T20:39:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 01, 2014 at 05:58:30PM +0200, Michael Haggerty wrote:\n\n> Add a new option flag, LOCK_SEPARATE_STAGING_FILE, that can be passed\n> to hold_lock_file_for_update() or hold_lock_file_for_append() to use a\n> staging file that is independent of the lock file.\n> \n> Add a new function activate_staging_file() that activates the contents\n> that have been written to the staging file without releasing the lock.\n> \n> This functionality can be used to ensure that changes to two files are\n> seen by other processes in one order even if correctness requires the\n> locks to be released in another order.\n\nCan you give an example of when this is useful? I'm guessing the\napplication is for writing out packed-refs before pruning loose refs in\ngit-pack-refs?\n\nIt seems like this makes the API much more confusing. If I understand\ncorrectly, this is basically allowing us to take a lock, write to\n_another_ tmpfile that is not the lock, then rename the tmpfile into\nplace without releasing the lock (and then we can drop the lock at our\nconvenience).\n\nI wonder if it would be simpler to build an API for that around the\nlock_file API, rather than as part of it. Or am I misunderstanding\nwhat's going on?\n\n-Peff\n"},{"id":"238272","messageId":"20140401204447.GJ21715@sigill.intra.peff.net","threadId":"36333","inReplyTo":"1396367910-7299-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 00/22] Lockfile refactoring and pre-activation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-01T20:44:47Z","receivedAt":"2014-04-01T20:44:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 01, 2014 at 05:58:08PM +0200, Michael Haggerty wrote:\n\n> I've had this patch series kicking around for a long time, along with\n> some followup patches to fix a race in reference deletion. I haven't\n> had the time to get everything done and tested, but let me at least\n> push this first series out there. I especially want to submit it in\n> case we accept a GSoC student for the project \"Refactor tempfile\n> handling\", because (1) I don't want me and the student to be stepping\n> on each others' toes, and (2) there are some cleanups and\n> documentation improvements here that will hopefully be useful to the\n> student.\n\nThanks, this sort of preparation for GSoC students is very much welcome.\n\n> The first patch actually demonstrates the race condition that I hope\n> to fix someday. The last patch introduces the lockfile feature that I\n> think is needed to fix it: the ability to activate a packed-refs file\n> while still holding the lock to prevent another process from\n> overwriting it before the accompanying loose reference updates are all\n> finished. But the fix itself is not included here, so you might want\n> to keep the last patch on hold until there is a concrete user of the\n> feature.\n\nI should have read this more carefully when I responded to the final\npatch. I surmised your intent based on our previous work on packed-refs,\nbut here you spell out my guesses explicitly. :)\n\nI think all of the patches look good. I'd prefer to hold back the final\none (and probably 19/22, which has no purpose except to prepare for\n22/22) until seeing its application in practice.\n\nThanks for a very pleasant read.\n\n-Peff\n"},{"id":"238323","messageId":"533BB28B.10906@web.de","threadId":"36333","inReplyTo":"1396367910-7299-6-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 05/22] lockfile: unlock file if lockfile permissions cannot be adjusted","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-04-02T06:47:39Z","receivedAt":"2014-04-02T06:47:39Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"[]\n\ndiff --git a/lockfile.c b/lockfile.c\nindex c1af65b..1928e5e 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -148,9 +148,11 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\t\tlock_file_list = lk;\n \t\t\tlk->on_list = 1;\n \t\t}\n-\t\tif (adjust_shared_perm(lk->filename))\n-\t\t\treturn error(\"cannot fix permission bits on %s\",\n-\t\t\t\t lk->filename);\n+\t\tif (adjust_shared_perm(lk->filename)) {\n+\t\t\terror(\"cannot fix permission bits on %s\", lk->filename);\n+\t\t\trollback_lock_file(lk);\n+\t\t\treturn -1;\n\nWould it make sense to change the order of rollback() and error()?\nMake the rollback first (and as early as possible) and whine then?\n"},{"id":"238329","messageId":"533BB3A1.1080203@web.de","threadId":"36333","inReplyTo":"1396367910-7299-13-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 12/22] delete_ref_loose(): don't muck around in the lock_file's filename","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-04-02T06:52:17Z","receivedAt":"2014-04-02T06:52:17Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\n[]\ndiff --git a/refs.c b/refs.c\nindex 28d5eca..11ad23e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2485,12 +2485,14 @@ static int repack_without_ref(const char *refname)\n static int delete_ref_loose(struct ref_lock *lock, int flag)\n {\n \tif (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n-\t\t/* loose */\n-\t\tint err, i = strlen(lock->lk->filename) - 5; /* .lock */\n-\n-\t\tlock->lk->filename[i] = 0;\n-\t\terr = unlink_or_warn(lock->lk->filename);\n-\t\tlock->lk->filename[i] = '.';\n+\t\t/*\n+\t\t * loose. The loose file name is the same as the\n+\t\t * lockfile name, minus \".lock\":\n+\t\t */\n+\t\tchar *loose_filename = xmemdupz(lock->lk->filename,\n+\t\t\t\t\t\tstrlen(lock->lk->filename) - 5);\n\nAt other places (lockfile.c) we use this\n\n+#define LOCK_SUFFIX_LEN 5\n\nI think it makes sense to move this definition to an include file (lockfile.h ??)\nand use it here.\n"},{"id":"238296","messageId":"20140402065558.GA2395@sigill.intra.peff.net","threadId":"36333","inReplyTo":"533BB3A1.1080203@web.de","subject":"Re: [PATCH 12/22] delete_ref_loose(): don't muck around in the lock_file's filename","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-02T06:55:58Z","receivedAt":"2014-04-02T06:55:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 02, 2014 at 08:52:17AM +0200, Torsten Bögershausen wrote:\n\n> +\t\t/*\n> +\t\t * loose. The loose file name is the same as the\n> +\t\t * lockfile name, minus \".lock\":\n> +\t\t */\n> +\t\tchar *loose_filename = xmemdupz(lock->lk->filename,\n> +\t\t\t\t\t\tstrlen(lock->lk->filename) - 5);\n> \n> At other places (lockfile.c) we use this\n> \n> +#define LOCK_SUFFIX_LEN 5\n> \n> I think it makes sense to move this definition to an include file (lockfile.h ??)\n> and use it here.\n\nI had the same comment, but if you read through to patch 18, this manual\nmunging goes away entirely, and the end result is much cleaner.\n\n-Peff\n"},{"id":"238327","messageId":"533BB519.8030307@web.de","threadId":"36333","inReplyTo":"1396367910-7299-14-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 13/22] config: change write_error() to take a (struct lock_file *) argument","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-04-02T06:58:33Z","receivedAt":"2014-04-02T06:58:33Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 04/01/2014 05:58 PM, Michael Haggerty wrote:\n> Reduce the amount of code that has to know about the lock_file's\n> filename field.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> config.c | 10 +++++-----\n> 1 file changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/config.c b/config.c\n> index 6821cef..1ea3f39 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1303,9 +1303,9 @@ static int store_aux(const char *key, const char *value, void *cb)\n> \treturn 0;\n> }\n> \n> -static int write_error(const char *filename)\n> +static int write_error(struct lock_file *lk)\nDoes the write_error() really need to know about struct lock_file ?\n(The name of the function does not indicate that it is doing something \nwith lk)\nAnd if, would it make sense to rename it into \nwrite_error_and_do_something() ?\n> {\n> -\terror(\"failed to write new configuration file %s\", filename);\n> +\terror(\"failed to write new configuration file %s\", lk->filename);\n> \n> \t/* Same error code as \"failed to rename\". */\n> \treturn 4;\n> @@ -1706,7 +1706,7 @@ out_free:\n> \treturn ret;\n> \n> write_err_out:\n> -\tret = write_error(lock->filename);\n> +\tret = write_error(lock);\n> \tgoto out_free;\n> \n> }\n> @@ -1821,7 +1821,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n> \t\t\t\t}\n> \t\t\t\tstore.baselen = strlen(new_name);\n> \t\t\t\tif (!store_write_section(out_fd, new_name)) {\n> -\t\t\t\t\tret = write_error(lock->filename);\n> +\t\t\t\t\tret = write_error(lock);\n> \t\t\t\t\tgoto out;\n> \t\t\t\t}\n> \t\t\t\t/*\n> @@ -1847,7 +1847,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n> \t\t\tcontinue;\n> \t\tlength = strlen(output);\n> \t\tif (write_in_full(out_fd, output, length) != length) {\n> -\t\t\tret = write_error(lock->filename);\n> +\t\t\tret = write_error(lock);\n> \t\t\tgoto out;\n> \t\t}\n> \t}\n"},{"id":"238334","messageId":"CAPig+cTEC6kgmgCcpcjaRNMf3gQVQGzBQx5i5A+EuDppH9VQxg@mail.gmail.com","threadId":"36333","inReplyTo":"1396367910-7299-22-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 21/22] lockfile: extract a function reset_lock_file()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-04-02T07:06:54Z","receivedAt":"2014-04-02T07:06:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 1, 2014 at 11:58 AM, Michael Haggerty wrote:\n> Signed-off-by: Michael Haggerty \n> ---\n> lockfile.c | 31 ++++++++++++++++---------------\n> 1 file changed, 16 insertions(+), 15 deletions(-)\n>\n> diff --git a/lockfile.c b/lockfile.c\n> index 852d717..c06e134 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -85,6 +85,14 @@ static void remove_lock_file_on_signal(int signo)\n> raise(signo);\n> }\n>\n> +static void reset_lock_file(struct lock_file *lk)\n> +{\n> + lk->fd = -1;\n> + strbuf_setlen(&lk->filename, 0);\n> + strbuf_setlen(&lk->staging_filename, 0);\n\nstrbuf_reset() perhaps?\n\n> + lk->flags = LOCK_FLAGS_ON_LIST;\n> +}\n> +\n> /*\n> * path = absolute or relative path name\n> *\n> @@ -185,8 +193,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n>\n> lk->fd = open(lk->staging_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n> if (lk->fd < 0) {\n> - strbuf_setlen(&lk->filename, 0);\n> - strbuf_setlen(&lk->staging_filename, 0);\n> + reset_lock_file(lk);\n> return -1;\n> }\n> if (adjust_shared_perm(lk->staging_filename.buf)) {\n> @@ -273,17 +280,12 @@ int close_lock_file(struct lock_file *lk)\n>\n> int commit_lock_file(struct lock_file *lk)\n> {\n> - int err = 0;\n> -\n> if (lk->fd >= 0 && close_lock_file(lk))\n> return -1;\n> - if (rename(lk->staging_filename.buf, lk->filename.buf)) {\n> - err = -1;\n> - } else {\n> - strbuf_setlen(&lk->filename, 0);\n> - strbuf_setlen(&lk->staging_filename, 0);\n> - }\n> - return err;\n> + if (rename(lk->staging_filename.buf, lk->filename.buf))\n> + return -1;\n> + reset_lock_file(lk);\n> + return 0;\n> }\n>\n> int hold_locked_index(struct lock_file *lk, int die_on_error)\n> @@ -306,8 +308,8 @@ int commit_locked_index(struct lock_file *lk)\n> return -1;\n> if (rename(lk->staging_filename.buf, alternate_index_output))\n> return -1;\n> - strbuf_setlen(&lk->filename, 0);\n> - strbuf_setlen(&lk->staging_filename, 0);\n> +\n> + reset_lock_file(lk);\n> return 0;\n> } else {\n> return commit_lock_file(lk);\n> @@ -320,7 +322,6 @@ void rollback_lock_file(struct lock_file *lk)\n> if (lk->fd >= 0)\n> close_lock_file(lk);\n> unlink_or_warn(lk->staging_filename.buf);\n> - strbuf_setlen(&lk->filename, 0);\n> - strbuf_setlen(&lk->staging_filename, 0);\n> + reset_lock_file(lk);\n> }\n> }\n> --\n> 1.9.0\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"238293","messageId":"CAPig+cQLsK_X_5_LPj8YfBhr4CQJWZCq52A+E5shZoHMvvwyzw@mail.gmail.com","threadId":"36333","inReplyTo":"1396367910-7299-23-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 22/22] lockfile: allow new file contents to be written while retaining lock","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-04-02T07:20:46Z","receivedAt":"2014-04-02T07:20:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 1, 2014 at 11:58 AM, Michael Haggerty wrote:\n> Add a new option flag, LOCK_SEPARATE_STAGING_FILE, that can be passed\n> to hold_lock_file_for_update() or hold_lock_file_for_append() to use a\n> staging file that is independent of the lock file.\n>\n> Add a new function activate_staging_file() that activates the contents\n> that have been written to the staging file without releasing the lock.\n>\n> This functionality can be used to ensure that changes to two files are\n> seen by other processes in one order even if correctness requires the\n> locks to be released in another order.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> diff --git a/lockfile.c b/lockfile.c\n> index c06e134..336b914 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -271,19 +325,54 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n> return fd;\n> }\n>\n> -int close_lock_file(struct lock_file *lk)\n> +static int close_staging_file(struct lock_file *lk)\n> {\n> int fd = lk->fd;\n> +\n> lk->fd = -1;\n> return close(fd);\n> }\n>\n> -int commit_lock_file(struct lock_file *lk)\n> +int close_lock_file(struct lock_file *lk)\n> {\n> - if (lk->fd >= 0 && close_lock_file(lk))\n> - return -1;\n> - if (rename(lk->staging_filename.buf, lk->filename.buf))\n> + assert(!(lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE));\n> + return close_staging_file(lk);\n> +}\n> +\n> +int activate_staging_file(struct lock_file *lk)\n> +{\n> + int err;\n> +\n> + assert(lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE);\n> + assert(lk->fd >= 0);\n> + assert(lk->staging_filename.len);\n> +\n> + if (close_staging_file(lk))\n> return -1;\n> +\n> + err = rename(lk->staging_filename.buf, lk->filename.buf);\n> + strbuf_setlen(&lk->staging_filename, 0);\n\nstrbuf_reset()?\n\n> +\n> + return err;\n> +}\n> +\n> +int commit_lock_file(struct lock_file *lk)\n> +{\n> + if (lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE) {\n> + if (lk->staging_filename.len) {\n> + assert(lk->fd >= 0);\n> + if (activate_staging_file(lk))\n> + return -1;\n> + }\n> + strbuf_addbuf(&lk->staging_filename, &lk->filename);\n> + strbuf_addstr(&lk->staging_filename, \".lock\");\n> + unlink_or_warn(lk->staging_filename.buf);\n> + } else {\n> + if (lk->fd >= 0 && close_lock_file(lk))\n> + return -1;\n> + if (rename(lk->staging_filename.buf, lk->filename.buf))\n> + return -1;\n> + }\n> reset_lock_file(lk);\n> return 0;\n> }\n> @@ -318,10 +407,21 @@ int commit_locked_index(struct lock_file *lk)\n>\n> void rollback_lock_file(struct lock_file *lk)\n> {\n> - if (lk->filename.len) {\n> - if (lk->fd >= 0)\n> - close_lock_file(lk);\n> - unlink_or_warn(lk->staging_filename.buf);\n> - reset_lock_file(lk);\n> + if (!lk->filename.len)\n> + return;\n> +\n> + if (lk->fd >= 0)\n> + close_staging_file(lk);\n> +\n> + if (lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE) {\n> + if (lk->staging_filename.len) {\n> + unlink_or_warn(lk->staging_filename.buf);\n> + strbuf_setlen(&lk->staging_filename, 0);\n\nstrbuf_reset()?\n\n> + }\n> + strbuf_addbuf(&lk->staging_filename, &lk->filename);\n> + strbuf_addstr(&lk->staging_filename, \".lock\");\n> }\n> +\n> + unlink_or_warn(lk->staging_filename.buf);\n> + reset_lock_file(lk);\n> }\n> --\n> 1.9.0\n"},{"id":"238297","messageId":"533BE63E.5070508@alum.mit.edu","threadId":"36333","inReplyTo":"20140401195338.GA21715@sigill.intra.peff.net","subject":"Re: [PATCH 01/22] t3204: test deleting references when lock files already exist","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-02T10:28:14Z","receivedAt":"2014-04-02T10:28:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/01/2014 09:53 PM, Jeff King wrote:\n> On Tue, Apr 01, 2014 at 05:58:09PM +0200, Michael Haggerty wrote:\n> \n>> When deleting a reference, it might be that another process already\n>> holds the lock on the loose reference file and/or the packed-refs\n>> file. In those cases, there is no alternative but for the delete to\n>> fail. Verify that in such cases the reference values are left\n>> unchanged.\n>>\n>> But in fact, when the packed-refs file lock cannot be acquired, the\n>> loose reference file is deleted anyway, potentially leaving the\n>> reference with a changed value (its packed value, which might even\n>> point at an object that has been garbage collected). Thus one of the\n>> new tests is marked test_expect_failure.\n> \n> Nice find. If I understand correctly, the problem is at the plumbing\n> level, and this could also be demonstrated with update-ref?\n> \n> I don't think it is a big deal, but I was thrown for a minute by the use\n> of \"git branch\" (as in, \"is this something special with branches, or is\n> this about all refs?\").\n\nGood point. Will change.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238331","messageId":"533BEC45.4060803@alum.mit.edu","threadId":"36333","inReplyTo":"20140401195615.GB21715@sigill.intra.peff.net","subject":"Re: [PATCH 02/22] try_merge_strategy(): remove redundant lock_file allocation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-02T10:53:57Z","receivedAt":"2014-04-02T10:53:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/01/2014 09:56 PM, Jeff King wrote:\n> On Tue, Apr 01, 2014 at 05:58:10PM +0200, Michael Haggerty wrote:\n> \n>> By the time the \"if\" block is entered, the lock_file instance from the\n>> main function block is no longer in use, so re-use that one instead of\n>> allocating a second one.\n>>\n>> Note that the \"lock\" variable in the \"if\" block used to shadow the\n>> \"lock\" variable at function scope, so the only change needed is to\n>> remove the inner definition.\n> \n> I wonder if this would also be simpler if \"lock\" were simply declared as\n> a static variable, and we drop the allocation entirely. I suppose that\n> does create more cognitive load, though, in that it is only correct if\n> the function is not recursive. On the other hand, the current code makes\n> a reader unfamiliar with \"struct lock\" wonder if there is a free(lock)\n> missing.\n\nYes, a single lock_file object should suffice. I will make this change\nin the next version of the patch series.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238294","messageId":"533BF638.5060506@alum.mit.edu","threadId":"36333","inReplyTo":"20140401201906.GF21715@sigill.intra.peff.net","subject":"Re: [PATCH 09/22] api-lockfile: expand the documentation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-02T11:36:24Z","receivedAt":"2014-04-02T11:36:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/01/2014 10:19 PM, Jeff King wrote:\n> On Tue, Apr 01, 2014 at 05:58:17PM +0200, Michael Haggerty wrote:\n> \n>> +unable_to_lock_error::\n>> +\n>> +\tEmit an error describing that there was an error locking the\n>> +\tspecified path. The err parameter should be the errno of the\n>> +\tproblem that caused the failure.\n>> +\n>> +unable_to_lock_index_die::\n>> +\n>> +\tLike `unable_to_lock_error()`, but also `die()`.\n> \n> Should this last one lost the \"index\" in its name? I think it is\n> vestigial at this point.\n\nYes. Will be done in re-roll.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238295","messageId":"533BF996.7010804@alum.mit.edu","threadId":"36333","inReplyTo":"20140401202118.GG21715@sigill.intra.peff.net","subject":"Re: [PATCH 12/22] delete_ref_loose(): don't muck around in the lock_file's filename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-02T11:50:46Z","receivedAt":"2014-04-02T11:50:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/01/2014 10:21 PM, Jeff King wrote:\n> On Tue, Apr 01, 2014 at 05:58:20PM +0200, Michael Haggerty wrote:\n> \n>> It's bad manners. Especially since, if unlink_or_warn() failed, the\n>> memory wasn't restored to its original contents.\n>>\n>> So make our own copy to work with.\n> \n> Sounds good...\n> \n>> \tif (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n>> -\t\t/* loose */\n>> -\t\tint err, i = strlen(lock->lk->filename) - 5; /* .lock */\n>> -\n>> -\t\tlock->lk->filename[i] = 0;\n>> -\t\terr = unlink_or_warn(lock->lk->filename);\n>> -\t\tlock->lk->filename[i] = '.';\n>> +\t\t/*\n>> +\t\t * loose. The loose file name is the same as the\n>> +\t\t * lockfile name, minus \".lock\":\n>> +\t\t */\n>> +\t\tchar *loose_filename = xmemdupz(lock->lk->filename,\n>> +\t\t\t\t\t\tstrlen(lock->lk->filename) - 5);\n>> +\t\tint err = unlink_or_warn(loose_filename);\n>> +\t\tfree(loose_filename);\n> \n> Should we be using LOCK_SUFFIX_LEN from the previous commit here?\n\nLOCK_SUFFIX_LEN is not in scope to this file, and I think it should stay\nthat way. But never fear; this figuring-out-filename-from-lockfile-name\nnonsense is gone by the end of the patch series.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238300","messageId":"533C128A.9020406@alum.mit.edu","threadId":"36333","inReplyTo":"CAPig+cTEC6kgmgCcpcjaRNMf3gQVQGzBQx5i5A+EuDppH9VQxg@mail.gmail.com","subject":"Re: [PATCH 21/22] lockfile: extract a function reset_lock_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-02T13:37:14Z","receivedAt":"2014-04-02T13:37:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/02/2014 09:06 AM, Eric Sunshine wrote:\n> On Tue, Apr 1, 2014 at 11:58 AM, Michael Haggerty wrote:\n>> Signed-off-by: Michael Haggerty \n>> ---\n>> lockfile.c | 31 ++++++++++++++++---------------\n>> 1 file changed, 16 insertions(+), 15 deletions(-)\n>>\n>> diff --git a/lockfile.c b/lockfile.c\n>> index 852d717..c06e134 100644\n>> --- a/lockfile.c\n>> +++ b/lockfile.c\n>> @@ -85,6 +85,14 @@ static void remove_lock_file_on_signal(int signo)\n>> raise(signo);\n>> }\n>>\n>> +static void reset_lock_file(struct lock_file *lk)\n>> +{\n>> + lk->fd = -1;\n>> + strbuf_setlen(&lk->filename, 0);\n>> + strbuf_setlen(&lk->staging_filename, 0);\n> \n> strbuf_reset() perhaps?\n\nThanks, Eric. For some reason I always have it in the back of my head\nthat strbuf_reset() frees the memory associated with the strbuf, but of\ncourse that is strbuf_release() that I'm thinking of.\n\nI just fixed all strbuf_setlen(..., 0) -> strbuf_reset(...) throughout\nthe patch series. The fix will be in the re-roll.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238303","messageId":"xmqqy4znwu4g.fsf@gitster.dls.corp.google.com","threadId":"36333","inReplyTo":"20140401195615.GB21715@sigill.intra.peff.net","subject":"Re: [PATCH 02/22] try_merge_strategy(): remove redundant lock_file allocation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-02T16:53:19Z","receivedAt":"2014-04-02T16:53:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Apr 01, 2014 at 05:58:10PM +0200, Michael Haggerty wrote:\n>\n>> By the time the \"if\" block is entered, the lock_file instance from the\n>> main function block is no longer in use, so re-use that one instead of\n>> allocating a second one.\n>> \n>> Note that the \"lock\" variable in the \"if\" block used to shadow the\n>> \"lock\" variable at function scope, so the only change needed is to\n>> remove the inner definition.\n>\n> I wonder if this would also be simpler if \"lock\" were simply declared as\n> a static variable, and we drop the allocation entirely. I suppose that\n> does create more cognitive load, though, in that it is only correct if\n> the function is not recursive. On the other hand, the current code makes\n> a reader unfamiliar with \"struct lock\" wonder if there is a free(lock)\n> missing.\n\nAnother thing that makes a reader wonder if this is a valid rewrite\nis if it is safe to reuse a lock_file structure, especially because\nthe original gives a piece of memory _cleared_ with xcalloc(). The\nsecond invocation of hold_locked_index() is now done on a dirty\npiece of memory, and the reader needs to drill down the callchain to\nsee if that is safe (and if not, hold_locked_index() and probably\nthe underlying lock_file() needs to memset() it to NULs).\n"},{"id":"238326","messageId":"xmqqtxabwtw8.fsf@gitster.dls.corp.google.com","threadId":"36333","inReplyTo":"20140401195930.GC21715@sigill.intra.peff.net","subject":"Re: [PATCH 04/22] rollback_lock_file(): set fd to -1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-02T16:58:15Z","receivedAt":"2014-04-02T16:58:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Apr 01, 2014 at 05:58:12PM +0200, Michael Haggerty wrote:\n>\n>> When rolling back the lockfile, call close_lock_file() so that the\n>> lock_file's fd field gets set back to -1. This could help prevent\n>> confusion in the face of hypothetical future programming errors.\n>\n> This also solves a race. We could be in the middle of rollback_lock_file\n> when we get a signal, and double-close. It's probably not a big deal,\n> though, since nobody could have opened a new descriptor in the interim\n> that got the same number (so the second close will just fail silently).\n>\n> Still, this seems like a definite improvement.\n\nThis is probably related to my comments on 2/22, but is \"fd\" the\nonly thing that has a non-zero safe value? Perhaps lock_file_init()\nthat clears the structure fields to 0/NULL and fd to -1, and a\nconvenience function lock_file_alloc() that does xmalloc() and then\ncalls lock_file_init() may help us a bit when the lockfile structure\nis reused?\n"},{"id":"238314","messageId":"xmqqppkzwtq0.fsf@gitster.dls.corp.google.com","threadId":"36333","inReplyTo":"20140401201659.GE21715@sigill.intra.peff.net","subject":"Re: [PATCH 07/22] lock_file(): always add lock_file object to lock_file_list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-02T17:01:59Z","receivedAt":"2014-04-02T17:01:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So for a moment, lk->filename contains the name of the valuable file we\n> are locking. If we get a signal at that moment, do we accidentally\n> delete it in remove_lock_file?\n>\n> I think the answer is \"no\", because we check lk->owner before deleting,\n> which will not match our pid (it should generally be zero due to xcalloc\n> or static initialization, though perhaps we should clear it here).\n\nThat \"generally be zero\" no longer holds since 2/22, no?\n\n> But that makes me wonder about the case of a reused lock. It will have\n> lk->owner set from a previous invocation, and would potentially suffer\n> from this problem. In other words, I think the change you are\n> introducing does not have the problem, but the existing code does. :-/\n\nYes, I tend to agree.\n\n> I didn't reproduce it experimentally, though. We should be able to just\n>\n> lk->owner = 0;\n>\n> before the initial strcpy to fix it, I would think.\n>\n> -Peff\n"},{"id":"238340","messageId":"xmqqbnwjwt2b.fsf@gitster.dls.corp.google.com","threadId":"36333","inReplyTo":"1396367910-7299-15-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 14/22] lockfile: use strbufs when handling (most) paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-02T17:16:12Z","receivedAt":"2014-04-02T17:16:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Change struct lock_file's filename field from a fixed-length buffer\n> into a strbuf.\n\nGood.\n\nAs I allued to in a review on an unrelated patch, I do not think it\nis a good idea to name the lock filename field \"lock_filename\" in a\nstructure that is about a lockfile, though.\n"},{"id":"238306","messageId":"xmqq7g77wsxj.fsf@gitster.dls.corp.google.com","threadId":"36333","inReplyTo":"1396367910-7299-19-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 18/22] lockfile: also keep track of the filename of the file being locked","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-02T17:19:04Z","receivedAt":"2014-04-02T17:19:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> This reduces the amount of string editing gyrations and makes it\n> unnecessary for callers to know how to derive the filename from the\n> lock_filename.\n\nHmph.\n\nIs it worth duplicating the whole thing? If you are planning to\nbreak the invariant lock_filename === filename + LOCK_SUFFIX in\nfuture changes in the series, it is understandable, though.\n"},{"id":"238304","messageId":"xmqq38hvwskl.fsf@gitster.dls.corp.google.com","threadId":"36333","inReplyTo":"1396367910-7299-23-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 22/22] lockfile: allow new file contents to be written while retaining lock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-02T17:26:50Z","receivedAt":"2014-04-02T17:26:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> +static int open_staging_file(struct lock_file *lk)\n> +{\n> +\tstrbuf_setlen(&lk->staging_filename, lk->filename.len);\n> +\tstrbuf_addstr(&lk->staging_filename, \".new\");\n> +\tlk->fd = open(lk->staging_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n> +\tif (lk->fd < 0) {\n> +\t\treturn -1;\n> +\t}\n\nAll the other \"if (lk->fd < 0)\" calls reset_lock_file(lk). Is it an\nintentional omission that this one does not?\n\nIf so, please drop the extraneous {} around the single \"return -1\"\nstatement.\n\nI also share the same puzzlement in Peff's review.\n"},{"id":"238307","messageId":"xmqqvburvdyo.fsf@gitster.dls.corp.google.com","threadId":"36333","inReplyTo":"1396367910-7299-12-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 11/22] lockfile: define a constant LOCK_SUFFIX_LEN","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-02T17:27:43Z","receivedAt":"2014-04-02T17:27:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Signed-off-by: Michael Haggerty \n> ---\n> lockfile.c | 11 +++++++----\n> 1 file changed, 7 insertions(+), 4 deletions(-)\n>\n> diff --git a/lockfile.c b/lockfile.c\n> index 4a9ceda..4e3ada8 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -178,14 +178,17 @@ static char *resolve_symlink(char *p, size_t s)\n> \treturn p;\n> }\n> \n> +/* We append \".lock\" to the filename to derive the lockfile name: */\n> +#define LOCK_SUFFIX_LEN 5\n> \n> static int lock_file(struct lock_file *lk, const char *path, int flags)\n> {\n> \t/*\n> -\t * subtract 5 from size to make sure there's room for adding\n> -\t * \".lock\" for the lock file name\n> +\t * subtract LOCK_SUFFIX_LEN from size to make sure there's\n> +\t * room for adding \".lock\" for the lock file name:\n> \t */\n> -\tstatic const size_t max_path_len = sizeof(lk->filename) - 5;\n> +\tstatic const size_t max_path_len = sizeof(lk->filename) -\n> +\t\t LOCK_SUFFIX_LEN;\n\nSP{8,} in the indent; I've cleaned it up while queueing.\n\nThanks.\n"},{"id":"238320","messageId":"xmqqob0jvdve.fsf@gitster.dls.corp.google.com","threadId":"36333","inReplyTo":"1396367910-7299-14-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 13/22] config: change write_error() to take a (struct lock_file *) argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-02T17:29:41Z","receivedAt":"2014-04-02T17:29:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Reduce the amount of code that has to know about the lock_file's\n> filename field.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> config.c | 10 +++++-----\n> 1 file changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/config.c b/config.c\n> index 6821cef..1ea3f39 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1303,9 +1303,9 @@ static int store_aux(const char *key, const char *value, void *cb)\n> \treturn 0;\n> }\n> \n> -static int write_error(const char *filename)\n> +static int write_error(struct lock_file *lk)\n> {\n\nThe earlier one would have been usable for reporting an error while\nwriting any file, but the caller must hold a lock file on it with a\nnew one. Would this change warrant a renaming of the function, I\nwonder.\n\nIt is a file-scope static, so all callers know about how they are\nsupposed to call it, hence the keeping the original name would be\nOK, I would think.\n\nThis hunk triggered my smello-meter, primarily because \"write-error\"\nwould not be the name I would pick for this function if I were\nwriting everything in this file from scratch (before or after this\nparticular patch).\n\n> -\terror(\"failed to write new configuration file %s\", filename);\n> +\terror(\"failed to write new configuration file %s\", lk->filename);\n> \n> \t/* Same error code as \"failed to rename\". */\n> \treturn 4;\n> @@ -1706,7 +1706,7 @@ out_free:\n> \treturn ret;\n> \n> write_err_out:\n> -\tret = write_error(lock->filename);\n> +\tret = write_error(lock);\n> \tgoto out_free;\n> \n> }\n> @@ -1821,7 +1821,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n> \t\t\t\t}\n> \t\t\t\tstore.baselen = strlen(new_name);\n> \t\t\t\tif (!store_write_section(out_fd, new_name)) {\n> -\t\t\t\t\tret = write_error(lock->filename);\n> +\t\t\t\t\tret = write_error(lock);\n> \t\t\t\t\tgoto out;\n> \t\t\t\t}\n> \t\t\t\t/*\n> @@ -1847,7 +1847,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n> \t\t\tcontinue;\n> \t\tlength = strlen(output);\n> \t\tif (write_in_full(out_fd, output, length) != length) {\n> -\t\t\tret = write_error(lock->filename);\n> +\t\t\tret = write_error(lock);\n> \t\t\tgoto out;\n> \t\t}\n> \t}\n"},{"id":"238344","messageId":"533D492B.9090505@alum.mit.edu","threadId":"36333","inReplyTo":"20140401204447.GJ21715@sigill.intra.peff.net","subject":"Re: [PATCH 00/22] Lockfile refactoring and pre-activation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-03T11:42:35Z","receivedAt":"2014-04-03T11:42:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/01/2014 10:44 PM, Jeff King wrote:\n> [...]\n> I think all of the patches look good. I'd prefer to hold back the final\n> one (and probably 19/22, which has no purpose except to prepare for\n> 22/22) until seeing its application in practice.\n\nOK, I shuffled 19/22 to the end of the patch series, just before 22/22.\n That way both can be taken or left (probably left for now).\n\n> Thanks for a very pleasant read.\n\nThanks for being a very pleasant (and thorough) reader :-)\n\nI'm still working on the backlog of comments and then I'll get back to\nthe list with v2.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238347","messageId":"533D5755.7030609@alum.mit.edu","threadId":"36333","inReplyTo":"xmqqy4znwu4g.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 02/22] try_merge_strategy(): remove redundant lock_file allocation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-03T12:43:01Z","receivedAt":"2014-04-03T12:43:01Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/02/2014 06:53 PM, Junio C Hamano wrote:\n> Jeff King writes:\n> \n>> On Tue, Apr 01, 2014 at 05:58:10PM +0200, Michael Haggerty wrote:\n>>\n>>> By the time the \"if\" block is entered, the lock_file instance from the\n>>> main function block is no longer in use, so re-use that one instead of\n>>> allocating a second one.\n>>>\n>>> Note that the \"lock\" variable in the \"if\" block used to shadow the\n>>> \"lock\" variable at function scope, so the only change needed is to\n>>> remove the inner definition.\n>>\n>> I wonder if this would also be simpler if \"lock\" were simply declared as\n>> a static variable, and we drop the allocation entirely. I suppose that\n>> does create more cognitive load, though, in that it is only correct if\n>> the function is not recursive. On the other hand, the current code makes\n>> a reader unfamiliar with \"struct lock\" wonder if there is a free(lock)\n>> missing.\n> \n> Another thing that makes a reader wonder if this is a valid rewrite\n> is if it is safe to reuse a lock_file structure, especially because\n> the original gives a piece of memory _cleared_ with xcalloc(). The\n> second invocation of hold_locked_index() is now done on a dirty\n> piece of memory, and the reader needs to drill down the callchain to\n> see if that is safe (and if not, hold_locked_index() and probably\n> the underlying lock_file() needs to memset() it to NULs).\n\nIt's good that you and Peff asked questions about this sort of thing.\n\nWe reuse lock_file structures *all over the place*; for example, just\nsearch for \"static struct lock_file\". It has to be safe...\n\n...and yet it isn't. Look in the definition of lock_file() (before my\nchanges):\n\nstatic int lock_file(struct lock_file *lk, const char *path, int flags)\n{\n\t...\n\tstrcpy(lk->filename, path);\n\tif (!(flags & LOCK_NODEREF))\n\t\tresolve_symlink(lk->filename, max_path_len);\n\tstrcat(lk->filename, \".lock\");\n\nRemember that a reused lock_file structure is already in lock_file_list,\nand there is already a signal handler registered that will call\nremove_lock_file(), which looks like:\n\nstatic void remove_lock_file(void)\n{\n\tpid_t me = getpid();\n\n\twhile (lock_file_list) {\n\t\tif (lock_file_list->owner == me &&\n\t\t lock_file_list->filename[0]) {\n\t\t\tif (lock_file_list->fd >= 0)\n\t\t\t\tclose(lock_file_list->fd);\n\t\t\tunlink_or_warn(lock_file_list->filename);\n\t\t}\n\t\tlock_file_list = lock_file_list->next;\n\t}\n}\n\nSo, if the process gets a signal during the call to resolve_symlink(),\nthe atexit() cleanup routine will delete the valuable file (the one\nbeing locked)!\n\nIt definitely looks like this area needs more work.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238401","messageId":"5341CAE2.8040600@alum.mit.edu","threadId":"36333","inReplyTo":"xmqqtxabwtw8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 04/22] rollback_lock_file(): set fd to -1","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-06T21:45:06Z","receivedAt":"2014-04-06T21:45:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/02/2014 06:58 PM, Junio C Hamano wrote:\n> Jeff King writes:\n> \n>> On Tue, Apr 01, 2014 at 05:58:12PM +0200, Michael Haggerty wrote:\n>>\n>>> When rolling back the lockfile, call close_lock_file() so that the\n>>> lock_file's fd field gets set back to -1. This could help prevent\n>>> confusion in the face of hypothetical future programming errors.\n>>\n>> This also solves a race. We could be in the middle of rollback_lock_file\n>> when we get a signal, and double-close. It's probably not a big deal,\n>> though, since nobody could have opened a new descriptor in the interim\n>> that got the same number (so the second close will just fail silently).\n>>\n>> Still, this seems like a definite improvement.\n> \n> This is probably related to my comments on 2/22, but is \"fd\" the\n> only thing that has a non-zero safe value? Perhaps lock_file_init()\n> that clears the structure fields to 0/NULL and fd to -1, and a\n> convenience function lock_file_alloc() that does xmalloc() and then\n> calls lock_file_init() may help us a bit when the lockfile structure\n> is reused?\n\nThe first use of a lock_file object necessarily passes through\nlock_file(). The only precondition for that function is that the\non_list field is zero, which is satisfied by a xcalloc()ed object.\n\nSubsequent uses of a lock_file object must *not* zero the object.\nlock_file objects are added to the lock_file_list and never removed. So\nzeroing a lock_file object would discard the rest of the linked list.\nBut subsequent uses must also pass through lock_file(), which sees that\non_list is set, and assumes that the object is in a self-consistent\nstate as left by commit_lock_file() or rollback_lock_file().\n\nAt least that's how it is supposed to work. But lock_file objects are\nin fact not cleaned up correctly in all circumstances. The next version\nof this patch series will work to fix that.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238402","messageId":"5341CD33.3000205@alum.mit.edu","threadId":"36333","inReplyTo":"20140401201659.GE21715@sigill.intra.peff.net","subject":"Re: [PATCH 07/22] lock_file(): always add lock_file object to lock_file_list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-06T21:54:59Z","receivedAt":"2014-04-06T21:54:59Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/01/2014 10:16 PM, Jeff King wrote:\n> On Tue, Apr 01, 2014 at 05:58:15PM +0200, Michael Haggerty wrote:\n> \n>> diff --git a/lockfile.c b/lockfile.c\n>> index e679e4c..c989f6c 100644\n>> --- a/lockfile.c\n>> +++ b/lockfile.c\n>> @@ -130,6 +130,22 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n>> \t */\n>> \tstatic const size_t max_path_len = sizeof(lk->filename) - 5;\n>> \n>> +\tif (!lock_file_list) {\n>> +\t\t/* One-time initialization */\n>> +\t\tsigchain_push_common(remove_lock_file_on_signal);\n>> +\t\tatexit(remove_lock_file);\n>> +\t}\n>> +\n>> +\tlk->owner = getpid();\n>> +\tif (!lk->on_list) {\n>> +\t\t/* Initialize *lk and add it to lock_file_list: */\n>> +\t\tlk->fd = -1;\n>> +\t\tlk->on_list = 1;\n>> +\t\tlk->filename[0] = 0;\n>> +\t\tlk->next = lock_file_list;\n>> +\t\tlock_file_list = lk;\n>> +\t}\n> \n> Initializing here is good, since we might be interrupted by a signal at\n> any time. But what about during the locking procedure? We do:\n> \n> strcpy(lk->filename, path);\n> if (!(flags & LOCK_NODEREF))\n> resolve_symlink(lk->filename, max_path_len);\n> strcat(lk->filename, \".lock\");\n> \n> So for a moment, lk->filename contains the name of the valuable file we\n> are locking. If we get a signal at that moment, do we accidentally\n> delete it in remove_lock_file?\n> \n> I think the answer is \"no\", because we check lk->owner before deleting,\n> which will not match our pid (it should generally be zero due to xcalloc\n> or static initialization, though perhaps we should clear it here).\n> \n> But that makes me wonder about the case of a reused lock. It will have\n> lk->owner set from a previous invocation, and would potentially suffer\n> from this problem. In other words, I think the change you are\n> introducing does not have the problem, but the existing code does. :-/\n\nGood point. Yes, I agree that this is a problem in the existing code\nand that it wasn't improved by my work.\n\n> I didn't reproduce it experimentally, though. We should be able to just\n> \n> lk->owner = 0;\n> \n> before the initial strcpy to fix it, I would think.\n\nI think that using the owner field to avoid this problem is a bit\nindirect, so I will soon submit a fix that involves adding a flag to\nlock_file objects indicating whether the filename field currently\ncontains the name of a file that needs to be deleted.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238403","messageId":"5341CF06.9020501@alum.mit.edu","threadId":"36333","inReplyTo":"533BB28B.10906@web.de","subject":"Re: [PATCH 05/22] lockfile: unlock file if lockfile permissions cannot be adjusted","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-06T22:02:46Z","receivedAt":"2014-04-06T22:02:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/02/2014 08:47 AM, Torsten Bögershausen wrote:\n> []\n> \n> diff --git a/lockfile.c b/lockfile.c\n> index c1af65b..1928e5e 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -148,9 +148,11 @@ static int lock_file(struct lock_file *lk, const\n> char *path, int flags)\n> lock_file_list = lk;\n> lk->on_list = 1;\n> }\n> - if (adjust_shared_perm(lk->filename))\n> - return error(\"cannot fix permission bits on %s\",\n> - lk->filename);\n> + if (adjust_shared_perm(lk->filename)) {\n> + error(\"cannot fix permission bits on %s\", lk->filename);\n> + rollback_lock_file(lk);\n> + return -1;\n> \n> Would it make sense to change the order of rollback() and error()?\n> Make the rollback first (and as early as possible) and whine then?\n\nThanks for the feedback. It is a nice idea. But given that\nrollback_lock_file() erases the filename field, making the change you\nsuggest would require us to make a copy before calling\nrollback_lock_file(). The only advantage would be that we hold the lock\nfile for a moment less, right? Since this code path is only reached\nwhen the repository has screwy permissions anyway, the next caller will\nprobably fail too. So the extra code complication doesn't seem worth it\nto me.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238404","messageId":"5341CF72.70904@alum.mit.edu","threadId":"36333","inReplyTo":"533BB519.8030307@web.de","subject":"Re: [PATCH 13/22] config: change write_error() to take a (struct lock_file *) argument","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-06T22:04:34Z","receivedAt":"2014-04-06T22:04:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/02/2014 08:58 AM, Torsten Bögershausen wrote:\n> On 04/01/2014 05:58 PM, Michael Haggerty wrote:\n>> Reduce the amount of code that has to know about the lock_file's\n>> filename field.\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n>> config.c | 10 +++++-----\n>> 1 file changed, 5 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/config.c b/config.c\n>> index 6821cef..1ea3f39 100644\n>> --- a/config.c\n>> +++ b/config.c\n>> @@ -1303,9 +1303,9 @@ static int store_aux(const char *key, const char\n>> *value, void *cb)\n>> return 0;\n>> }\n>> -static int write_error(const char *filename)\n>> +static int write_error(struct lock_file *lk)\n> Does the write_error() really need to know about struct lock_file ?\n> (The name of the function does not indicate that it is doing something\n> with lk)\n> And if, would it make sense to rename it into\n> write_error_and_do_something() ?\n\nI'm going to leave this part out of the next re-roll, because you are\nright: this change is mostly a distraction and probably not an improvement.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238405","messageId":"5341CFC5.6050904@alum.mit.edu","threadId":"36333","inReplyTo":"xmqq7g77wsxj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 18/22] lockfile: also keep track of the filename of the file being locked","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-06T22:05:57Z","receivedAt":"2014-04-06T22:05:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/02/2014 07:19 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> This reduces the amount of string editing gyrations and makes it\n>> unnecessary for callers to know how to derive the filename from the\n>> lock_filename.\n> \n> Hmph.\n> \n> Is it worth duplicating the whole thing? If you are planning to\n> break the invariant lock_filename === filename + LOCK_SUFFIX in\n> future changes in the series, it is understandable, though.\n\nThat is indeed the plan, but I'm splitting that change into a future\npatch series because this one is getting big enough already dealing with\ncorrectness issues.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238436","messageId":"20140407093650.GA4252@sigill.intra.peff.net","threadId":"36333","inReplyTo":"5341CD33.3000205@alum.mit.edu","subject":"Re: [PATCH 07/22] lock_file(): always add lock_file object to lock_file_list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-07T09:36:50Z","receivedAt":"2014-04-07T09:36:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 06, 2014 at 11:54:59PM +0200, Michael Haggerty wrote:\n\n> > I didn't reproduce it experimentally, though. We should be able to just\n> > \n> > lk->owner = 0;\n> > \n> > before the initial strcpy to fix it, I would think.\n> \n> I think that using the owner field to avoid this problem is a bit\n> indirect, so I will soon submit a fix that involves adding a flag to\n> lock_file objects indicating whether the filename field currently\n> contains the name of a file that needs to be deleted.\n\nYeah, I agree that the current code is a bit subtle. I was planning to\naddress this during the tempfile cleanup project (either in GSoC, if it\ngets a slot, or just doing it myself). But I don't mind if you want to\ndo something in the interim.\n\n-Peff\n"},{"id":"238471","messageId":"xmqqfvlpnlj4.fsf@gitster.dls.corp.google.com","threadId":"36333","inReplyTo":"5341CAE2.8040600@alum.mit.edu","subject":"Re: [PATCH 04/22] rollback_lock_file(): set fd to -1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-07T16:37:19Z","receivedAt":"2014-04-07T16:37:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> The first use of a lock_file object necessarily passes through\n> lock_file(). The only precondition for that function is that the\n> on_list field is zero, which is satisfied by a xcalloc()ed object.\n>\n> Subsequent uses of a lock_file object must *not* zero the object.\n> lock_file objects are added to the lock_file_list and never removed. So\n> zeroing a lock_file object would discard the rest of the linked list.\n> But subsequent uses must also pass through lock_file(), which sees that\n> on_list is set, and assumes that the object is in a self-consistent\n> state as left by commit_lock_file() or rollback_lock_file().\n>\n> At least that's how it is supposed to work. But lock_file objects are\n> in fact not cleaned up correctly in all circumstances. The next version\n> of this patch series will work to fix that.\n\nThanks; I see the next round already posted to the list.\n"}]}