{"thread":{"id":"37866","subject":"[PATCH v5] lockfile.c: store absolute path","startedAt":"2014-11-02T06:24:37Z","lastAt":"2014-11-05T14:19:28Z","messageCount":3,"participants":["Michael Haggerty","Scott Schmit"],"isPatch":true,"patchVersion":5,"patchTotal":null},"messages":[{"id":"251289","messageId":"1414909477-20030-1-git-send-email-mhagger@alum.mit.edu","threadId":"37866","inReplyTo":null,"subject":"[PATCH v5] lockfile.c: store absolute path","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-02T06:24:37Z","receivedAt":"2014-11-02T06:24:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nLocked paths can be saved in a linked list so that if something wrong\nhappens, *.lock are removed. For relative paths, this works fine if we\nkeep cwd the same, which is true 99% of time except:\n\n- update-index and read-tree hold the lock on $GIT_DIR/index really\n early, then later on may call setup_work_tree() to move cwd.\n\n- Suppose a lock is being held (e.g. by \"git add\") then somewhere\n down the line, somebody calls real_path (e.g. \"link_alt_odb_entry\"),\n which temporarily moves cwd away and back.\n\nDuring that time when cwd is moved (either permanently or temporarily)\nand we decide to die(), attempts to remove relative *.lock will fail,\nand the next operation will complain that some files are still locked.\n\nAvoid this case by turning relative paths to absolute before storing\nthe path in \"filename\" field.\n\nReported-by: Yue Lin Ho \nHelped-by: Ramsay Jones \nHelped-by: Johannes Sixt \nSigned-off-by: Nguyễn Thái Ngọc Duy \nAdapted-by: Michael Haggerty \nSigned-off-by: Michael Haggerty \n---\nThis is a re-roll onto master to resolve conflicts with other changes\nthat have been merged since v4 [1]:\n\n* The length of path is now available, so use strbuf_add() instead of\n strbuf_addstr().\n\n* LOCK_NODEREF has been renamed to LOCK_NO_DEREF.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/255069/focus=256584\n\n lockfile.c | 14 +++++++++++---\n t/t2107-update-index-basic.sh | 15 +++++++++++++++\n 2 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 4f16ee7..9889277 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -128,9 +128,17 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\t path);\n \t}\n \n-\tstrbuf_add(&lk->filename, path, pathlen);\n-\tif (!(flags & LOCK_NO_DEREF))\n-\t\tresolve_symlink(&lk->filename);\n+\tif (flags & LOCK_NO_DEREF) {\n+\t\tstrbuf_add_absolute_path(&lk->filename, path);\n+\t} else {\n+\t\tstruct strbuf resolved_path = STRBUF_INIT;\n+\n+\t\tstrbuf_add(&resolved_path, path, pathlen);\n+\t\tresolve_symlink(&resolved_path);\n+\t\tstrbuf_add_absolute_path(&lk->filename, resolved_path.buf);\n+\t\tstrbuf_release(&resolved_path);\n+\t}\n+\n \tstrbuf_addstr(&lk->filename, LOCK_SUFFIX);\n \tlk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (lk->fd < 0) {\ndiff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh\nindex 1bafb90..dfe02f4 100755\n--- a/t/t2107-update-index-basic.sh\n+++ b/t/t2107-update-index-basic.sh\n@@ -65,4 +65,19 @@ test_expect_success '--cacheinfo mode,sha1,path (new syntax)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '.lock files cleaned up' '\n+\tmkdir cleanup &&\n+\t(\n+\tcd cleanup &&\n+\tmkdir worktree &&\n+\tgit init repo &&\n+\tcd repo &&\n+\tgit config core.worktree ../../worktree &&\n+\t# --refresh triggers late setup_work_tree,\n+\t# active_cache_changed is zero, rollback_lock_file fails\n+\tgit update-index --refresh &&\n+\t! test -f .git/index.lock\n+\t)\n+'\n+\n test_done\n-- \n2.1.1\n"},{"id":"251399","messageId":"20141105022315.GA28292@odin.ulthar.us","threadId":"37866","inReplyTo":"1414909477-20030-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5] lockfile.c: store absolute path","fromName":"Scott Schmit","fromEmail":"i.grok@comcast.net","sentAt":"2014-11-05T02:23:15Z","receivedAt":"2014-11-05T02:23:15Z","isPatch":true,"sender":{"key":"i.grok@comcast.net","avatar":null},"body":"On Sun, Nov 02, 2014 at 07:24:37AM +0100, Michael Haggerty wrote:\n> Locked paths can be saved in a linked list so that if something wrong\n> happens, *.lock are removed. For relative paths, this works fine if we\n> keep cwd the same, which is true 99% of time except:\n> \n> - update-index and read-tree hold the lock on $GIT_DIR/index really\n> early, then later on may call setup_work_tree() to move cwd.\n> \n> - Suppose a lock is being held (e.g. by \"git add\") then somewhere\n> down the line, somebody calls real_path (e.g. \"link_alt_odb_entry\"),\n> which temporarily moves cwd away and back.\n> \n> During that time when cwd is moved (either permanently or temporarily)\n> and we decide to die(), attempts to remove relative *.lock will fail,\n> and the next operation will complain that some files are still locked.\n> \n> Avoid this case by turning relative paths to absolute before storing\n> the path in \"filename\" field.\n\nThis might be a little pathological, but it seems like this scheme would\nrun into trouble if the entire repo is moved while the lock is held.\n"},{"id":"251410","messageId":"545A31F0.5080306@alum.mit.edu","threadId":"37866","inReplyTo":"20141105022315.GA28292@odin.ulthar.us","subject":"Re: [PATCH v5] lockfile.c: store absolute path","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-05T14:19:28Z","receivedAt":"2014-11-05T14:19:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/05/2014 03:23 AM, Scott Schmit wrote:\n> On Sun, Nov 02, 2014 at 07:24:37AM +0100, Michael Haggerty wrote:\n>> Locked paths can be saved in a linked list so that if something wrong\n>> happens, *.lock are removed. For relative paths, this works fine if we\n>> keep cwd the same, which is true 99% of time except:\n>>\n>> - update-index and read-tree hold the lock on $GIT_DIR/index really\n>> early, then later on may call setup_work_tree() to move cwd.\n>>\n>> - Suppose a lock is being held (e.g. by \"git add\") then somewhere\n>> down the line, somebody calls real_path (e.g. \"link_alt_odb_entry\"),\n>> which temporarily moves cwd away and back.\n>>\n>> During that time when cwd is moved (either permanently or temporarily)\n>> and we decide to die(), attempts to remove relative *.lock will fail,\n>> and the next operation will complain that some files are still locked.\n>>\n>> Avoid this case by turning relative paths to absolute before storing\n>> the path in \"filename\" field.\n> \n> This might be a little pathological, but it seems like this scheme would\n> run into trouble if the entire repo is moved while the lock is held.\n\nCorrect. You shouldn't move a repository while Git operations are\nrunning in it. That would break for many reasons, not just because of\nthis change to lockfile handling.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"}]}