{"thread":{"id":"37580","subject":"[PATCH v5 05/35] rollback_lock_file(): set fd to -1","startedAt":"2014-09-16T19:33:21Z","lastAt":"2014-09-23T13:40:33Z","messageCount":73,"participants":["Michael Haggerty","Jonathan Nieder","Torsten Bögershausen"],"isPatch":true,"patchVersion":5,"patchTotal":35},"messages":[{"id":"249534","messageId":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":null,"subject":"[PATCH v5 00/35] Lockfile correctness and refactoring","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:21Z","receivedAt":"2014-09-16T19:33:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Next iteration of my lockfile fixes and refactoring. Thanks to\nTorsten Bögershausen, Junio, Peff, Ronnie Sahlberg, and Johannes Sixt\nfor their comments about v4.\n\nI believe that this series addresses all of the comments from v1 [1],\nv2 [2], v3 [3], and v4 [4].\n\nChanges since v4:\n\n* Rebase to current master.\n\n* Explain lock_file ownership and the point of its pid field.\n\n* Correct the log message for\n\n \"delete_ref_loose(): don't muck around in the lock_file's filename\"\n\n* Replace an assert() with a die(\"BUG:\").\n\n* Add a sanity check that lk->filename is empty before reusing a\n lock_file object.\n\n* Initialize the length of lk->filename more intelligently.\n\n* Rename trim_last_path_elm() to trim_last_path_component().\n\n* Make some die() messages more informative.\n\n* Add some sanity checks to commit_lock_file().\n\n* Rename REF_NODEREF to REF_NO_DEREF.\n\n* Rename some static functions:\n * remove_lock_file() -> remove_lock_files()\n * remove_lock_file_on_signal() -> remove_lock_files_on_signal()\n\n* Add a function get_locked_file_path(), to isolate callers a bit more\n from the innards of lock_file.\n\nThere are some conflicts with branch rs/ref-transaction; I pushed my\nproposed merge of these two branches to\n\n https://github.com/mhagger/git.git lock-correctness-v5-rs-ref-transaction\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/245609\n[2] http://thread.gmane.org/gmane.comp.version-control.git/245801\n[3] http://thread.gmane.org/gmane.comp.version-control.git/246222\n[4] http://thread.gmane.org/gmane.comp.version-control.git/256564\n\nMichael Haggerty (35):\n unable_to_lock_die(): rename function from unable_to_lock_index_die()\n api-lockfile: expand the documentation\n rollback_lock_file(): do not clear filename redundantly\n rollback_lock_file(): exit early if lock is not active\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 lockfile.c: document the various states of lock_file objects\n cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN\n delete_ref_loose(): don't muck around in the lock_file's filename\n prepare_index(): declare return value to be (const char *)\n write_packed_entry_fn(): convert cb_data into a (const int *)\n lock_file(): exit early if lockfile cannot be opened\n remove_lock_file(): call rollback_lock_file()\n commit_lock_file(): inline temporary variable\n commit_lock_file(): die() if called for unlocked lockfile object\n commit_lock_file(): if close fails, roll back\n commit_lock_file(): rollback lock file on failure to rename\n api-lockfile: document edge cases\n dump_marks(): remove a redundant call to rollback_lock_file()\n git_config_set_multivar_in_file(): avoid call to rollback_lock_file()\n lockfile: avoid transitory invalid states\n struct lock_file: declare some fields volatile\n try_merge_strategy(): remove redundant lock_file allocation\n try_merge_strategy(): use a statically-allocated lock_file object\n commit_lock_file(): use a strbuf to manage temporary space\n Change lock_file::filename into a strbuf\n resolve_symlink(): use a strbuf for internal scratch space\n resolve_symlink(): take a strbuf parameter\n trim_last_path_component(): replace last_path_elm()\n Extract a function commit_lock_file_to()\n Rename LOCK_NODEREF to LOCK_NO_DEREF\n lockfile.c: rename static functions\n get_locked_file_path(): new function\n\n Documentation/technical/api-lockfile.txt | 72 +++++--\n builtin/commit.c | 16 +-\n builtin/merge.c | 15 +-\n builtin/reflog.c | 2 +-\n builtin/update-index.c | 2 +-\n cache.h | 19 +-\n config.c | 28 +--\n fast-import.c | 4 +-\n lockfile.c | 334 +++++++++++++++++++------------\n read-cache.c | 12 +-\n refs.c | 29 +--\n shallow.c | 6 +-\n 12 files changed, 334 insertions(+), 205 deletions(-)\n\n-- \n2.1.0\n"},{"id":"249535","messageId":"1410896036-12750-2-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 01/35] unable_to_lock_die(): rename function from unable_to_lock_index_die()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:22Z","receivedAt":"2014-09-16T19:33:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This function is used for other things besides the index, so rename it\naccordingly.\n\nSuggested-by: Jeff King \nSigned-off-by: Michael Haggerty \nReviewed-by: Ronnie Sahlberg \n---\n builtin/update-index.c | 2 +-\n cache.h | 2 +-\n lockfile.c | 6 +++---\n refs.c | 2 +-\n 4 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex e8c7fd4..6c95988 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -942,7 +942,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\tif (newfd < 0) {\n \t\t\tif (refresh_args.flags & REFRESH_QUIET)\n \t\t\t\texit(128);\n-\t\t\tunable_to_lock_index_die(get_index_file(), lock_error);\n+\t\t\tunable_to_lock_die(get_index_file(), lock_error);\n \t\t}\n \t\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n \t\t\tdie(\"Unable to write new index file\");\ndiff --git a/cache.h b/cache.h\nindex dfa1a56..73ba5d0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -582,7 +582,7 @@ struct lock_file {\n extern int unable_to_lock_error(const char *path, int err);\n extern void unable_to_lock_message(const char *path, int err,\n \t\t\t\t struct strbuf *buf);\n-extern NORETURN void unable_to_lock_index_die(const char *path, int err);\n+extern NORETURN void unable_to_lock_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 commit_lock_file(struct lock_file *);\ndiff --git a/lockfile.c b/lockfile.c\nindex 2a800ce..f1ce154 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -185,7 +185,7 @@ int unable_to_lock_error(const char *path, int err)\n \treturn -1;\n }\n \n-NORETURN void unable_to_lock_index_die(const char *path, int err)\n+NORETURN void unable_to_lock_die(const char *path, int err)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \n@@ -198,7 +198,7 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)\n {\n \tint fd = lock_file(lk, path, flags);\n \tif (fd < 0 && (flags & LOCK_DIE_ON_ERROR))\n-\t\tunable_to_lock_index_die(path, errno);\n+\t\tunable_to_lock_die(path, errno);\n \treturn fd;\n }\n \n@@ -209,7 +209,7 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n \tfd = lock_file(lk, path, flags);\n \tif (fd < 0) {\n \t\tif (flags & LOCK_DIE_ON_ERROR)\n-\t\t\tunable_to_lock_index_die(path, errno);\n+\t\t\tunable_to_lock_die(path, errno);\n \t\treturn fd;\n \t}\n \ndiff --git a/refs.c b/refs.c\nindex 2ce5d69..8cb3539 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2167,7 +2167,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t */\n \t\t\tgoto retry;\n \t\telse\n-\t\t\tunable_to_lock_index_die(ref_file, errno);\n+\t\t\tunable_to_lock_die(ref_file, errno);\n \t}\n \treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n \n-- \n2.1.0\n"},{"id":"249537","messageId":"1410896036-12750-3-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 02/35] api-lockfile: expand the documentation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:23Z","receivedAt":"2014-09-16T19:33:23Z","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..b53e300 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_die::\n+\n+\tLike `unable_to_lock_error()`, but also `die()`.\n \n commit_lock_file::\n \n-- \n2.1.0\n"},{"id":"249542","messageId":"1410896036-12750-4-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 03/35] rollback_lock_file(): do not clear filename redundantly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:24Z","receivedAt":"2014-09-16T19:33:24Z","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 \nReviewed-by: Ronnie Sahlberg \n---\n lockfile.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex f1ce154..a548e08 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -276,6 +276,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-- \n2.1.0\n"},{"id":"249536","messageId":"1410896036-12750-5-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 04/35] rollback_lock_file(): exit early if lock is not active","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:25Z","receivedAt":"2014-09-16T19:33:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Eliminate a layer of nesting.\n\nSigned-off-by: Michael Haggerty \nReviewed-by: Ronnie Sahlberg \n---\n lockfile.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex a548e08..49179d8 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -272,10 +272,11 @@ int hold_locked_index(struct lock_file *lk, int die_on_error)\n \n 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\tunlink_or_warn(lk->filename);\n-\t\tlk->filename[0] = 0;\n-\t}\n+\tif (!lk->filename[0])\n+\t\treturn;\n+\n+\tif (lk->fd >= 0)\n+\t\tclose(lk->fd);\n+\tunlink_or_warn(lk->filename);\n+\tlk->filename[0] = 0;\n }\n-- \n2.1.0\n"},{"id":"249521","messageId":"1410896036-12750-6-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 05/35] rollback_lock_file(): set fd to -1","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:26Z","receivedAt":"2014-09-16T19:33:26Z","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 keeps the lock_file\nobject in a valid state, which is important because these objects are\nallowed to be reused.\n\nSigned-off-by: Michael Haggerty \nReviewed-by: Ronnie Sahlberg \n---\n lockfile.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 49179d8..b1c4ba0 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -276,7 +276,7 @@ void rollback_lock_file(struct lock_file *lk)\n \t\treturn;\n \n \tif (lk->fd >= 0)\n-\t\tclose(lk->fd);\n+\t\tclose_lock_file(lk);\n \tunlink_or_warn(lk->filename);\n \tlk->filename[0] = 0;\n }\n-- \n2.1.0\n"},{"id":"249540","messageId":"1410896036-12750-7-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 06/35] lockfile: unlock file if lockfile permissions cannot be adjusted","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:27Z","receivedAt":"2014-09-16T19:33:27Z","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 (namely, unlocked). Previously, the lockfile was\nretained until process cleanup in this situation.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex b1c4ba0..d4f165d 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -153,6 +153,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\t\tint save_errno = errno;\n \t\t\terror(\"cannot fix permission bits on %s\",\n \t\t\t lk->filename);\n+\t\t\trollback_lock_file(lk);\n \t\t\terrno = save_errno;\n \t\t\treturn -1;\n \t\t}\n-- \n2.1.0\n"},{"id":"249522","messageId":"1410896036-12750-8-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 07/35] hold_lock_file_for_append(): release lock on errors","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:28Z","receivedAt":"2014-09-16T19:33:28Z","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 d4f165d..983c3ec 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -219,13 +219,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-- \n2.1.0\n"},{"id":"249548","messageId":"1410896036-12750-9-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 08/35] lock_file(): always add lock_file object to lock_file_list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:29Z","receivedAt":"2014-09-16T19:33:29Z","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 | 25 ++++++++++++++++---------\n 1 file changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 983c3ec..00c972c 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -129,6 +129,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+\tif (!lk->on_list) {\n+\t\t/* Initialize *lk and add it to lock_file_list: */\n+\t\tlk->fd = -1;\n+\t\tlk->owner = 0;\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\terrno = ENAMETOOLONG;\n \t\treturn -1;\n@@ -139,16 +155,7 @@ 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\tint save_errno = errno;\n \t\t\terror(\"cannot fix permission bits on %s\",\n-- \n2.1.0\n"},{"id":"249547","messageId":"1410896036-12750-10-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 09/35] lockfile.c: document the various states of lock_file objects","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:30Z","receivedAt":"2014-09-16T19:33:30Z","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 \nReviewed-by: Ronnie Sahlberg \n---\n lockfile.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 57 insertions(+)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 00c972c..b6fe848 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -4,6 +4,63 @@\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 is owned by the process that created it. The lock_file\n+ * object has an \"owner\" field that records its owner. This field is\n+ * used to prevent a forked process from closing a lock_file of its\n+ * parent.\n+ *\n+ * A lock_file object can be in several states:\n+ *\n+ * - Uninitialized. In this state the object's on_list field must be\n+ * zero but the rest of its 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 on_list is set.\n+ *\n+ * - Locked, lockfile open (after hold_lock_file_for_update(),\n+ * hold_lock_file_for_append(), or reopen_lock_file()). In this\n+ * state, the lockfile exists, filename holds the filename of the\n+ * lockfile, fd holds a file descriptor open for writing to the\n+ * lockfile, and owner holds the PID of the process that locked the\n+ * 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 on_list is set.\n+ *\n+ * See Documentation/api-lockfile.txt for more information.\n+ */\n+\n static struct lock_file *lock_file_list;\n \n static void remove_lock_file(void)\n-- \n2.1.0\n"},{"id":"249523","messageId":"1410896036-12750-11-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 10/35] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:31Z","receivedAt":"2014-09-16T19:33:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"There are a few places that use these values, so define constants for\nthem.\n\nSigned-off-by: Michael Haggerty \n---\n cache.h | 4 ++++\n lockfile.c | 11 ++++++-----\n refs.c | 7 ++++---\n 3 files changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 73ba5d0..e4e7c56 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -570,6 +570,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);\n #define REFRESH_IN_PORCELAIN\t0x0020\t/* user friendly output, not \"needs update\" */\n extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);\n \n+/* String appended to a filename to derive the lockfile name: */\n+#define LOCK_SUFFIX \".lock\"\n+#define LOCK_SUFFIX_LEN 5\n+\n struct lock_file {\n \tstruct lock_file *next;\n \tint fd;\ndiff --git a/lockfile.c b/lockfile.c\nindex b6fe848..99a774b 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -181,10 +181,11 @@ static char *resolve_symlink(char *p, size_t s)\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\t\t\t LOCK_SUFFIX_LEN;\n \n \tif (!lock_file_list) {\n \t\t/* One-time initialization */\n@@ -209,7 +210,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\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+\tstrcat(lk->filename, LOCK_SUFFIX);\n \tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (0 <= lk->fd) {\n \t\tlk->owner = getpid();\n@@ -319,7 +320,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;\ndiff --git a/refs.c b/refs.c\nindex 8cb3539..53cd4fb 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -79,7 +79,8 @@ out:\n \t\tif (refname[1] == '\\0')\n \t\t\treturn -1; /* Component equals \".\". */\n \t}\n-\tif (cp - refname >= 5 && !memcmp(cp - 5, \".lock\", 5))\n+\tif (cp - refname >= LOCK_SUFFIX_LEN &&\n+\t !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))\n \t\treturn -1; /* Refname ends with \".lock\". */\n \treturn cp - refname;\n }\n@@ -2551,11 +2552,11 @@ 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+\t\tint err, i = strlen(lock->lk->filename) - LOCK_SUFFIX_LEN;\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\tlock->lk->filename[i] = LOCK_SUFFIX[0];\n \t\tif (err && errno != ENOENT)\n \t\t\treturn 1;\n \t}\n-- \n2.1.0\n"},{"id":"249545","messageId":"1410896036-12750-12-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 11/35] delete_ref_loose(): don't muck around in the lock_file's filename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:32Z","receivedAt":"2014-09-16T19:33:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It's bad manners. Especially since there could be a signal during the\ncall to unlink_or_warn(), in which case the signal handler will see\nthe wrong filename and delete the reference file, leaving the lockfile\nbehind.\n\nSo make our own copy to work with.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 53cd4fb..11c76ec 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2551,12 +2551,15 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)\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) - LOCK_SUFFIX_LEN;\n-\n-\t\tlock->lk->filename[i] = 0;\n-\t\terr = unlink_or_warn(lock->lk->filename);\n-\t\tlock->lk->filename[i] = LOCK_SUFFIX[0];\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(\n+\t\t\t\tlock->lk->filename,\n+\t\t\t\tstrlen(lock->lk->filename) - LOCK_SUFFIX_LEN);\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-- \n2.1.0\n"},{"id":"249524","messageId":"1410896036-12750-13-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 12/35] prepare_index(): declare return value to be (const char *)","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:33Z","receivedAt":"2014-09-16T19:33:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Declare the return value to be const to make it clear that we aren't\ngiving callers permission to write over the string that it points at.\n(The return value is the filename field of a struct lock_file, which\ncan be used by a signal handler at any time and therefore shouldn't be\ntampered with.)\n\nSigned-off-by: Michael Haggerty \n---\n builtin/commit.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 41f481b..d4228c9 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -315,8 +315,8 @@ static void refresh_cache_or_die(int refresh_flags)\n \t\tdie_resolve_conflict(\"commit\");\n }\n \n-static char *prepare_index(int argc, const char **argv, const char *prefix,\n-\t\t\t const struct commit *current_head, int is_status)\n+static const char *prepare_index(int argc, const char **argv, const char *prefix,\n+\t\t\t\t const struct commit *current_head, int is_status)\n {\n \tstruct string_list partial;\n \tstruct pathspec pathspec;\n-- \n2.1.0\n"},{"id":"249539","messageId":"1410896036-12750-14-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 13/35] write_packed_entry_fn(): convert cb_data into a (const int *)","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:34Z","receivedAt":"2014-09-16T19:33:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This makes it obvious that we have no plans to change the integer\npointed to, which is actually the fd field from a struct lock_file.\n\nSigned-off-by: Michael Haggerty \nReviewed-by: Ronnie Sahlberg \n---\n refs.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 11c76ec..4f313bc 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2217,7 +2217,7 @@ static void write_packed_entry(int fd, char *refname, unsigned char *sha1,\n */\n static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)\n {\n-\tint *fd = cb_data;\n+\tconst int *fd = cb_data;\n \tenum peel_status peel_status = peel_entry(entry, 0);\n \n \tif (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)\n-- \n2.1.0\n"},{"id":"249538","messageId":"1410896036-12750-15-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 14/35] lock_file(): exit early if lockfile cannot be opened","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:35Z","receivedAt":"2014-09-16T19:33:35Z","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 \nReviewed-by: Ronnie Sahlberg \n---\n lockfile.c | 23 +++++++++++------------\n 1 file changed, 11 insertions(+), 12 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 99a774b..911f123 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -212,19 +212,18 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\tresolve_symlink(lk->filename, max_path_len);\n \tstrcat(lk->filename, LOCK_SUFFIX);\n \tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n-\tif (0 <= lk->fd) {\n-\t\tlk->owner = getpid();\n-\t\tif (adjust_shared_perm(lk->filename)) {\n-\t\t\tint save_errno = errno;\n-\t\t\terror(\"cannot fix permission bits on %s\",\n-\t\t\t lk->filename);\n-\t\t\trollback_lock_file(lk);\n-\t\t\terrno = save_errno;\n-\t\t\treturn -1;\n-\t\t}\n-\t}\n-\telse\n+\tif (lk->fd < 0) {\n \t\tlk->filename[0] = 0;\n+\t\treturn -1;\n+\t}\n+\tlk->owner = getpid();\n+\tif (adjust_shared_perm(lk->filename)) {\n+\t\tint save_errno = errno;\n+\t\terror(\"cannot fix permission bits on %s\", lk->filename);\n+\t\trollback_lock_file(lk);\n+\t\terrno = save_errno;\n+\t\treturn -1;\n+\t}\n \treturn lk->fd;\n }\n \n-- \n2.1.0\n"},{"id":"249543","messageId":"1410896036-12750-16-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 15/35] remove_lock_file(): call rollback_lock_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:36Z","receivedAt":"2014-09-16T19:33:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It does just 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 911f123..1c85b18 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -68,12 +68,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->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\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-- \n2.1.0\n"},{"id":"249541","messageId":"1410896036-12750-17-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 16/35] commit_lock_file(): inline temporary variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:37Z","receivedAt":"2014-09-16T19:33:37Z","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 | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 1c85b18..9e1cdd2 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -311,12 +311,14 @@ int reopen_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+\n \tif (lk->fd >= 0 && close_lock_file(lk))\n \t\treturn -1;\n+\n \tstrcpy(result_file, lk->filename);\n-\ti = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */\n-\tresult_file[i] = 0;\n+\t/* remove \".lock\": */\n+\tresult_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;\n+\n \tif (rename(lk->filename, result_file))\n \t\treturn -1;\n \tlk->filename[0] = 0;\n-- \n2.1.0\n"},{"id":"249525","messageId":"1410896036-12750-18-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 17/35] commit_lock_file(): die() if called for unlocked lockfile object","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:38Z","receivedAt":"2014-09-16T19:33:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It was previously a bug to call commit_lock_file() with a lock_file\nobject that was not active (an illegal access would happen within the\nfunction). It was presumably never done, but this would be an easy\nprogramming error to overlook. So before continuing, do a consistency\ncheck that the lock_file object really is locked.\n\nHelped-by: Johannes Sixt \nSigned-off-by: Michael Haggerty \n---\n Documentation/technical/api-lockfile.txt | 4 +++-\n lockfile.c | 3 +++\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex b53e300..9a94ead 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -68,7 +68,9 @@ commit_lock_file::\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+\tvalue on failure to close(2) or rename(2). It is a bug to\n+\tcall `commit_lock_file()` for a `lock_file` object that is not\n+\tcurrently locked.\n \n rollback_lock_file::\n \ndiff --git a/lockfile.c b/lockfile.c\nindex 9e1cdd2..6436111 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -312,6 +312,9 @@ int commit_lock_file(struct lock_file *lk)\n {\n \tchar result_file[PATH_MAX];\n \n+\tif (!lk->filename[0])\n+\t\tdie(\"BUG: attempt to commit unlocked object\");\n+\n \tif (lk->fd >= 0 && close_lock_file(lk))\n \t\treturn -1;\n \n-- \n2.1.0\n"},{"id":"249526","messageId":"1410896036-12750-19-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 18/35] commit_lock_file(): if close fails, roll back","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:39Z","receivedAt":"2014-09-16T19:33:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If closing an open lockfile fails, then we cannot be sure of the\ncontents of the lockfile, so there is nothing sensible to do but\ndelete it. This change also leaves the lock_file object in a defined\nstate in this error path (namely, unlocked).\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 6436111..becb3da 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -315,8 +315,12 @@ int commit_lock_file(struct lock_file *lk)\n \tif (!lk->filename[0])\n \t\tdie(\"BUG: attempt to commit unlocked object\");\n \n-\tif (lk->fd >= 0 && close_lock_file(lk))\n+\tif (lk->fd >= 0 && close_lock_file(lk)) {\n+\t\tint save_errno = errno;\n+\t\trollback_lock_file(lk);\n+\t\terrno = save_errno;\n \t\treturn -1;\n+\t}\n \n \tstrcpy(result_file, lk->filename);\n \t/* remove \".lock\": */\n-- \n2.1.0\n"},{"id":"249546","messageId":"1410896036-12750-20-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 19/35] commit_lock_file(): rollback lock file on failure to rename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:40Z","receivedAt":"2014-09-16T19:33:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If rename() fails, call rollback_lock_file() to delete the lock file\n(in case it is still present) and reset the filename field to the\nempty string so that the lockfile object is left in a valid state.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 18 +++++++++++-------\n 1 file changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex becb3da..6ae5c84 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -311,25 +311,29 @@ int reopen_lock_file(struct lock_file *lk)\n int commit_lock_file(struct lock_file *lk)\n {\n \tchar result_file[PATH_MAX];\n+\tint save_errno;\n \n \tif (!lk->filename[0])\n \t\tdie(\"BUG: attempt to commit unlocked object\");\n \n-\tif (lk->fd >= 0 && close_lock_file(lk)) {\n-\t\tint save_errno = errno;\n-\t\trollback_lock_file(lk);\n-\t\terrno = save_errno;\n-\t\treturn -1;\n-\t}\n+\tif (lk->fd >= 0 && close_lock_file(lk))\n+\t\tgoto rollback;\n \n \tstrcpy(result_file, lk->filename);\n \t/* remove \".lock\": */\n \tresult_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;\n \n \tif (rename(lk->filename, result_file))\n-\t\treturn -1;\n+\t\tgoto rollback;\n+\n \tlk->filename[0] = 0;\n \treturn 0;\n+\n+rollback:\n+\tsave_errno = errno;\n+\trollback_lock_file(lk);\n+\terrno = save_errno;\n+\treturn -1;\n }\n \n int hold_locked_index(struct lock_file *lk, int die_on_error)\n-- \n2.1.0\n"},{"id":"249549","messageId":"1410896036-12750-21-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 20/35] api-lockfile: document edge cases","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:41Z","receivedAt":"2014-09-16T19:33:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"* Document the behavior of commit_lock_file() when it fails, namely\n that it rolls back the lock_file object and sets errno\n appropriately.\n\n* Document the behavior of rollback_lock_file() when called for a\n lock_file object that has already been committed or rolled back,\n namely that it is a NOOP.\n\nSigned-off-by: Michael Haggerty \n---\n Documentation/technical/api-lockfile.txt | 17 ++++++++++-------\n 1 file changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex 9a94ead..2514559 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -64,19 +64,22 @@ unable_to_lock_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). It is a bug to\n-\tcall `commit_lock_file()` for a `lock_file` object that is not\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 and rename the lockfile to its final destination.\n+\tReturn 0 upon success. On failure, rollback the lock file and\n+\treturn -1, with `errno` set to the value from the failing call\n+\tto `close(2)` or `rename(2)`. It is a bug to call\n+\t`commit_lock_file()` for a `lock_file` object that is not\n \tcurrently locked.\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+\tclose the file descriptor and remove the lockfile. It is a\n+\tNOOP to call `rollback_lock_file()` for a `lock_file` object\n+\tthat has already been committed or rolled back.\n \n close_lock_file::\n \tTake a pointer to the `struct lock_file` initialized\n-- \n2.1.0\n"},{"id":"249528","messageId":"1410896036-12750-22-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 21/35] dump_marks(): remove a redundant call to rollback_lock_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:42Z","receivedAt":"2014-09-16T19:33:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"When commit_lock_file() fails, it now always calls\nrollback_lock_file() internally, so there is no need to call that\nfunction here.\n\nSigned-off-by: Michael Haggerty \n---\n fast-import.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex c071253..742d4d7 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1828,10 +1828,8 @@ static void dump_marks(void)\n \t}\n \n \tif (commit_lock_file(&mark_lock)) {\n-\t\tint saved_errno = errno;\n-\t\trollback_lock_file(&mark_lock);\n \t\tfailure |= error(\"Unable to commit marks file %s: %s\",\n-\t\t\texport_marks_file, strerror(saved_errno));\n+\t\t\texport_marks_file, strerror(errno));\n \t\treturn;\n \t}\n }\n-- \n2.1.0\n"},{"id":"249527","messageId":"1410896036-12750-23-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 22/35] git_config_set_multivar_in_file(): avoid call to rollback_lock_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:43Z","receivedAt":"2014-09-16T19:33:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"After commit_lock_file() is called, then the lock_file object is\nnecessarily either committed or rolled back. So there is no need to\ncall rollback_lock_file() again in either of these cases.\n\nSigned-off-by: Michael Haggerty \n---\n config.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 83c913a..a8ab809 100644\n--- a/config.c\n+++ b/config.c\n@@ -2076,17 +2076,17 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \tif (commit_lock_file(lock) < 0) {\n \t\terror(\"could not commit config file %s\", config_filename);\n \t\tret = CONFIG_NO_WRITE;\n-\t\tgoto out_free;\n-\t}\n+\t} else\n+\t\tret = 0;\n \n \t/*\n-\t * lock is committed, so don't try to roll it back below.\n-\t * NOTE: Since lockfile.c keeps a linked list of all created\n-\t * lock_file structures, it isn't safe to free(lock). It's\n-\t * better to just leave it hanging around.\n+\t * lock is committed or rolled back now, so there is no need\n+\t * to roll it back below. NOTE: Since lockfile.c keeps a\n+\t * linked list of all created lock_file structures, it isn't\n+\t * safe to free(lock). We have to just leave it hanging\n+\t * around.\n \t */\n \tlock = NULL;\n-\tret = 0;\n \n \t/* Invalidate the config cache */\n \tgit_config_clear();\n-- \n2.1.0\n"},{"id":"249530","messageId":"1410896036-12750-24-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 23/35] lockfile: avoid transitory invalid states","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:44Z","receivedAt":"2014-09-16T19:33:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Because remove_lock_file() can be called any time by the signal\nhandler, it is important that any lock_file objects that are in the\nlock_file_list are always in a valid state. And since lock_file\nobjects are often reused (but are never removed from lock_file_list),\nthat means we have to be careful whenever mutating a lock_file object\nto always keep it in a well-defined state.\n\nThis was formerly not the case, because part of the state was encoded\nby setting lk->filename to the empty string vs. a valid filename. It\nis wrong to assume that this string can be updated atomically; for\nexample, even\n\n strcpy(lk->filename, value)\n\nis unsafe. But the old code was even more reckless; for example,\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\nDuring the call to resolve_symlink(), lk->filename contained the name\nof the file that was being locked, not the name of the lockfile. If a\nsignal were raised during that interval, then the signal handler would\nhave deleted the valuable file!\n\nWe could probably continue to use the filename field to encode the\nstate by being careful to write characters 1..N-1 of the filename\nfirst, and then overwrite the NUL at filename[0] with the first\ncharacter of the filename, but that would be awkward and error-prone.\n\nSo, instead of using the filename field to determine whether the\nlock_file object is active, add a new field \"lock_file::active\" for\nthis purpose. Be careful to set this field only when filename really\ncontains the name of a file that should be deleted on cleanup.\n\nHelped-by: Johannes Sixt \nSigned-off-by: Michael Haggerty \n---\n cache.h | 1 +\n lockfile.c | 47 ++++++++++++++++++++++++++++++++---------------\n read-cache.c | 1 +\n 3 files changed, 34 insertions(+), 15 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex e4e7c56..a367220 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -576,6 +576,7 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct\n \n struct lock_file {\n \tstruct lock_file *next;\n+\tvolatile sig_atomic_t active;\n \tint fd;\n \tpid_t owner;\n \tchar on_list;\ndiff --git a/lockfile.c b/lockfile.c\nindex 6ae5c84..55fbb41 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -27,16 +27,19 @@\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+ * When the first file is locked, it registers an atexit(3) handler\n+ * and a signal handler; when the program exits, the handler rolls\n+ * back any files that have been locked but were never committed or\n+ * rolled back.\n *\n * A lock_file is owned by the process that created it. The lock_file\n * object has an \"owner\" field that records its owner. This field is\n * used to prevent a forked process from closing a lock_file of its\n * parent.\n *\n- * A lock_file object can be in several states:\n+ * Because the signal handler can be called at any time, a lock_file\n+ * object must always be in a well-defined state. The possible states\n+ * are as follows:\n *\n * - Uninitialized. In this state the object's on_list field must be\n * zero but the rest of its contents need not be initialized. As\n@@ -44,19 +47,25 @@\n * registered in the lock_file_list, and on_list is set.\n *\n * - Locked, lockfile open (after hold_lock_file_for_update(),\n- * hold_lock_file_for_append(), or reopen_lock_file()). In this\n- * state, the lockfile exists, filename holds the filename of the\n- * lockfile, fd holds a file descriptor open for writing to the\n- * lockfile, and owner holds the PID of the process that locked the\n- * file.\n+ * hold_lock_file_for_append(), or reopen_lock_file()). In this\n+ * state:\n+ * - the lockfile exists\n+ * - active is set\n+ * - filename holds the filename of the lockfile\n+ * - fd holds a file descriptor open for writing to the lockfile\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 on_list is set.\n+ * failed attempt to lock). In this state:\n+ * - active is unset\n+ * - filename[0] == '\\0' (usually, though there are transitory states\n+ * in which this condition doesn't hold)\n+ * - fd is -1\n+ * - the object is left registered in the lock_file_list, and\n+ * on_list is set.\n *\n * See Documentation/api-lockfile.txt for more information.\n */\n@@ -189,9 +198,14 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\tatexit(remove_lock_file);\n \t}\n \n+\tif (lk->active)\n+\t\tdie(\"BUG: lock_file(\\\"%s\\\") called with an active lock_file object\",\n+\t\t path);\n+\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->active = 0;\n \t\tlk->owner = 0;\n \t\tlk->on_list = 1;\n \t\tlk->filename[0] = 0;\n@@ -213,6 +227,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\treturn -1;\n \t}\n \tlk->owner = getpid();\n+\tlk->active = 1;\n \tif (adjust_shared_perm(lk->filename)) {\n \t\tint save_errno = errno;\n \t\terror(\"cannot fix permission bits on %s\", lk->filename);\n@@ -302,7 +317,7 @@ int reopen_lock_file(struct lock_file *lk)\n {\n \tif (0 <= lk->fd)\n \t\tdie(_(\"BUG: reopen a lockfile that is still open\"));\n-\tif (!lk->filename[0])\n+\tif (!lk->active)\n \t\tdie(_(\"BUG: reopen a lockfile that has been committed\"));\n \tlk->fd = open(lk->filename, O_WRONLY);\n \treturn lk->fd;\n@@ -313,7 +328,7 @@ int commit_lock_file(struct lock_file *lk)\n \tchar result_file[PATH_MAX];\n \tint save_errno;\n \n-\tif (!lk->filename[0])\n+\tif (!lk->active)\n \t\tdie(\"BUG: attempt to commit unlocked object\");\n \n \tif (lk->fd >= 0 && close_lock_file(lk))\n@@ -326,6 +341,7 @@ int commit_lock_file(struct lock_file *lk)\n \tif (rename(lk->filename, result_file))\n \t\tgoto rollback;\n \n+\tlk->active = 0;\n \tlk->filename[0] = 0;\n \treturn 0;\n \n@@ -346,11 +362,12 @@ int hold_locked_index(struct lock_file *lk, int die_on_error)\n \n void rollback_lock_file(struct lock_file *lk)\n {\n-\tif (!lk->filename[0])\n+\tif (!lk->active)\n \t\treturn;\n \n \tif (lk->fd >= 0)\n \t\tclose_lock_file(lk);\n \tunlink_or_warn(lk->filename);\n+\tlk->active = 0;\n \tlk->filename[0] = 0;\n }\ndiff --git a/read-cache.c b/read-cache.c\nindex b5917e0..e64bc87 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2028,6 +2028,7 @@ static int commit_locked_index(struct lock_file *lk)\n \t\t\treturn -1;\n \t\tif (rename(lk->filename, alternate_index_output))\n \t\t\treturn -1;\n+\t\tlk->active = 0;\n \t\tlk->filename[0] = 0;\n \t\treturn 0;\n \t} else {\n-- \n2.1.0\n"},{"id":"249556","messageId":"1410896036-12750-25-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 24/35] struct lock_file: declare some fields volatile","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:45Z","receivedAt":"2014-09-16T19:33:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The function remove_lock_file_on_signal() is used as a signal handler.\nIt is not realistic to make the signal handler conform strictly to the\nC standard, which is very restrictive about what a signal handler is\nallowed to do. But let's increase the likelihood that it will work:\n\nThe lock_file_list global variable and several fields from struct\nlock_file are used by the signal handler. Declare those values\n\"volatile\" to (1) force the main process to write the values to RAM\npromptly, and (2) prevent updates to these fields from being reordered\nin a way that leaves an opportunity for a jump to the signal handler\nwhile the object is in an inconsistent state.\n\nWe don't mark the filename field volatile because that would prevent\nthe use of strcpy(), and it is anyway unlikely that a compiler\nre-orders a strcpy() call across other expressions. So in practice it\nshould be possible to get away without \"volatile\" in the \"filename\"\ncase.\n\nSuggested-by: Johannes Sixt \nSigned-off-by: Michael Haggerty \n---\n cache.h | 6 +++---\n lockfile.c | 2 +-\n refs.c | 5 +++--\n 3 files changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex a367220..f251acd 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -575,10 +575,10 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct\n #define LOCK_SUFFIX_LEN 5\n \n struct lock_file {\n-\tstruct lock_file *next;\n+\tstruct lock_file *volatile next;\n \tvolatile sig_atomic_t active;\n-\tint fd;\n-\tpid_t owner;\n+\tvolatile int fd;\n+\tvolatile pid_t owner;\n \tchar on_list;\n \tchar filename[PATH_MAX];\n };\ndiff --git a/lockfile.c b/lockfile.c\nindex 55fbb41..a09b4a3 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -70,7 +70,7 @@\n * See Documentation/api-lockfile.txt for more information.\n */\n \n-static struct lock_file *lock_file_list;\n+static struct lock_file *volatile lock_file_list;\n \n static void remove_lock_file(void)\n {\ndiff --git a/refs.c b/refs.c\nindex 4f313bc..9971ac5 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2259,15 +2259,16 @@ int commit_packed_refs(void)\n \t\tget_packed_ref_cache(&ref_cache);\n \tint error = 0;\n \tint save_errno = 0;\n+\tint fd;\n \n \tif (!packed_ref_cache->lock)\n \t\tdie(\"internal error: packed-refs not locked\");\n \twrite_or_die(packed_ref_cache->lock->fd,\n \t\t PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));\n \n+\tfd = packed_ref_cache->lock->fd;\n \tdo_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),\n-\t\t\t\t 0, write_packed_entry_fn,\n-\t\t\t\t &packed_ref_cache->lock->fd);\n+\t\t\t\t 0, write_packed_entry_fn, &fd);\n \tif (commit_lock_file(packed_ref_cache->lock)) {\n \t\tsave_errno = errno;\n \t\terror = -1;\n-- \n2.1.0\n"},{"id":"249529","messageId":"1410896036-12750-26-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 25/35] try_merge_strategy(): remove redundant lock_file allocation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:46Z","receivedAt":"2014-09-16T19:33:46Z","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 shadowed the \"lock\"\nvariable at function scope, so the only change needed is to remove the\ninner 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 9da9e30..6b430f0 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -668,7 +668,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\tstruct commit_list *reversed = NULL;\n \t\tstruct merge_options o;\n \t\tstruct commit_list *j;\n-- \n2.1.0\n"},{"id":"249551","messageId":"1410896036-12750-27-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 26/35] try_merge_strategy(): use a statically-allocated lock_file object","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:47Z","receivedAt":"2014-09-16T19:33:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Even the one lockfile object needn't be allocated each time the\nfunction is called. Instead, define one statically-allocated\nlock_file object and reuse it for every call.\n\nSuggested-by: Jeff King \nSigned-off-by: Michael Haggerty \n---\n builtin/merge.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 6b430f0..1a5a5f2 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -656,14 +656,14 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\t\t struct commit_list *remoteheads,\n \t\t\t struct commit *head, const char *head_arg)\n {\n-\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n+\tstatic struct lock_file lock;\n \n-\thold_locked_index(lock, 1);\n+\thold_locked_index(&lock, 1);\n \trefresh_cache(REFRESH_QUIET);\n \tif (active_cache_changed &&\n-\t write_locked_index(&the_index, lock, COMMIT_LOCK))\n+\t write_locked_index(&the_index, &lock, COMMIT_LOCK))\n \t\treturn error(_(\"Unable to write index.\"));\n-\trollback_lock_file(lock);\n+\trollback_lock_file(&lock);\n \n \tif (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\")) {\n \t\tint clean, x;\n@@ -695,13 +695,13 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\tfor (j = common; j; j = j->next)\n \t\t\tcommit_list_insert(j->item, &reversed);\n \n-\t\thold_locked_index(lock, 1);\n+\t\thold_locked_index(&lock, 1);\n \t\tclean = merge_recursive(&o, head,\n \t\t\t\tremoteheads->item, reversed, &result);\n \t\tif (active_cache_changed &&\n-\t\t write_locked_index(&the_index, lock, COMMIT_LOCK))\n+\t\t write_locked_index(&the_index, &lock, COMMIT_LOCK))\n \t\t\tdie (_(\"unable to write %s\"), get_index_file());\n-\t\trollback_lock_file(lock);\n+\t\trollback_lock_file(&lock);\n \t\treturn clean ? 0 : 1;\n \t} else {\n \t\treturn try_merge_command(strategy, xopts_nr, xopts,\n-- \n2.1.0\n"},{"id":"249552","messageId":"1410896036-12750-28-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 27/35] commit_lock_file(): use a strbuf to manage temporary space","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:48Z","receivedAt":"2014-09-16T19:33:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Avoid relying on the filename length restrictions that are currently\nchecked by lock_file().\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex a09b4a3..59c822d 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -325,8 +325,9 @@ int reopen_lock_file(struct lock_file *lk)\n \n int commit_lock_file(struct lock_file *lk)\n {\n-\tchar result_file[PATH_MAX];\n+\tstatic struct strbuf result_file = STRBUF_INIT;\n \tint save_errno;\n+\tint err;\n \n \tif (!lk->active)\n \t\tdie(\"BUG: attempt to commit unlocked object\");\n@@ -334,13 +335,13 @@ int commit_lock_file(struct lock_file *lk)\n \tif (lk->fd >= 0 && close_lock_file(lk))\n \t\tgoto rollback;\n \n-\tstrcpy(result_file, lk->filename);\n \t/* remove \".lock\": */\n-\tresult_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;\n-\n-\tif (rename(lk->filename, result_file))\n+\tstrbuf_add(&result_file, lk->filename,\n+\t\t strlen(lk->filename) - LOCK_SUFFIX_LEN);\n+\terr = rename(lk->filename, result_file.buf);\n+\tstrbuf_reset(&result_file);\n+\tif (err)\n \t\tgoto rollback;\n-\n \tlk->active = 0;\n \tlk->filename[0] = 0;\n \treturn 0;\n-- \n2.1.0\n"},{"id":"249544","messageId":"1410896036-12750-29-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 28/35] Change lock_file::filename into a strbuf","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:49Z","receivedAt":"2014-09-16T19:33:49Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"For now, we still make sure to allocate at least PATH_MAX characters\nfor the strbuf because resolve_symlink() doesn't know how to expand\nthe space for its return value. (That will be fixed in a moment.)\n\nAnother alternative would be to just use a strbuf as scratch space in\nlock_file() but then store a pointer to the naked string in struct\nlock_file. But lock_file objects are often reused. By reusing the\nsame strbuf, we can avoid having to reallocate the string most times\nwhen a lock_file object is reused.\n\nHelped-by: Torsten Bögershausen \nSigned-off-by: Michael Haggerty \n---\n builtin/commit.c | 12 ++++++------\n builtin/reflog.c | 2 +-\n cache.h | 2 +-\n config.c | 14 +++++++-------\n lockfile.c | 51 +++++++++++++++++++++++----------------------------\n read-cache.c | 4 ++--\n refs.c | 6 +++---\n shallow.c | 6 +++---\n 8 files changed, 46 insertions(+), 51 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d4228c9..4049d06 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -341,7 +341,7 @@ static const 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.filename.buf, 1);\n \n \t\tif (interactive_add(argc, argv, prefix, patch_interactive) != 0)\n \t\t\tdie(_(\"interactive add failed\"));\n@@ -352,7 +352,7 @@ static const 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.filename.buf);\n \t\tif (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {\n \t\t\tif (reopen_lock_file(&index_lock) < 0)\n \t\t\t\tdie(_(\"unable to write index file\"));\n@@ -362,7 +362,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\t\twarning(_(\"Failed to update main cache tree\"));\n \n \t\tcommit_style = COMMIT_NORMAL;\n-\t\treturn index_lock.filename;\n+\t\treturn index_lock.filename.buf;\n \t}\n \n \t/*\n@@ -385,7 +385,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\tif (write_locked_index(&the_index, &index_lock, CLOSE_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.filename.buf;\n \t}\n \n \t/*\n@@ -472,9 +472,9 @@ static const 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.filename.buf);\n \n-\treturn false_lock.filename;\n+\treturn false_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 e8a8fb1..7c78b15 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\t\tlock->lk->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 f251acd..307cc8e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -580,7 +580,7 @@ struct lock_file {\n \tvolatile int fd;\n \tvolatile pid_t owner;\n \tchar on_list;\n-\tchar filename[PATH_MAX];\n+\tstruct strbuf filename;\n };\n #define LOCK_DIE_ON_ERROR 1\n #define LOCK_NODEREF 2\ndiff --git a/config.c b/config.c\nindex a8ab809..34c93c7 100644\n--- a/config.c\n+++ b/config.c\n@@ -2017,9 +2017,9 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t\t\tMAP_PRIVATE, in_fd, 0);\n \t\tclose(in_fd);\n \n-\t\tif (chmod(lock->filename, st.st_mode & 07777) < 0) {\n+\t\tif (chmod(lock->filename.buf, st.st_mode & 07777) < 0) {\n \t\t\terror(\"chmod on %s failed: %s\",\n-\t\t\t\tlock->filename, strerror(errno));\n+\t\t\t\tlock->filename.buf, strerror(errno));\n \t\t\tret = CONFIG_NO_WRITE;\n \t\t\tgoto out_free;\n \t\t}\n@@ -2098,7 +2098,7 @@ out_free:\n \treturn ret;\n \n write_err_out:\n-\tret = write_error(lock->filename);\n+\tret = write_error(lock->filename.buf);\n \tgoto out_free;\n \n }\n@@ -2199,9 +2199,9 @@ int git_config_rename_section_in_file(const char *config_filename,\n \n \tfstat(fileno(config_file), &st);\n \n-\tif (chmod(lock->filename, st.st_mode & 07777) < 0) {\n+\tif (chmod(lock->filename.buf, st.st_mode & 07777) < 0) {\n \t\tret = error(\"chmod on %s failed: %s\",\n-\t\t\t\tlock->filename, strerror(errno));\n+\t\t\t\tlock->filename.buf, strerror(errno));\n \t\tgoto out;\n \t}\n \n@@ -2222,7 +2222,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->filename.buf);\n \t\t\t\t\tgoto out;\n \t\t\t\t}\n \t\t\t\t/*\n@@ -2248,7 +2248,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->filename.buf);\n \t\t\tgoto out;\n \t\t}\n \t}\ndiff --git a/lockfile.c b/lockfile.c\nindex 59c822d..eecb526 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -61,8 +61,8 @@\n * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a\n * failed attempt to lock). In this state:\n * - active is unset\n- * - filename[0] == '\\0' (usually, though there are transitory states\n- * in which this condition doesn't hold)\n+ * - filename is empty (usually, though there are transitory\n+ * states in which this condition doesn't hold)\n * - fd is -1\n * - the object is left registered in the lock_file_list, and\n * on_list is set.\n@@ -185,13 +185,6 @@ static char *resolve_symlink(char *p, size_t s)\n /* Make sure errno contains a meaningful value on error */\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\t\t\t LOCK_SUFFIX_LEN;\n-\n \tif (!lock_file_list) {\n \t\t/* One-time initialization */\n \t\tsigchain_push_common(remove_lock_file_on_signal);\n@@ -208,29 +201,31 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\tlk->active = 0;\n \t\tlk->owner = 0;\n \t\tlk->on_list = 1;\n-\t\tlk->filename[0] = 0;\n+\t\tstrbuf_init(&lk->filename, PATH_MAX);\n \t\tlk->next = lock_file_list;\n \t\tlock_file_list = lk;\n+\t} else if (lk->filename.len) {\n+\t\t/* This shouldn't happen, but better safe than sorry. */\n+\t\tdie(\"BUG: lock_file(\\\"%s\\\") called with improperly-reset lock_file object\",\n+\t\t path);\n \t}\n \n-\tif (strlen(path) >= max_path_len) {\n-\t\terrno = ENAMETOOLONG;\n-\t\treturn -1;\n+\tstrbuf_addstr(&lk->filename, path);\n+\tif (!(flags & LOCK_NODEREF)) {\n+\t\tresolve_symlink(lk->filename.buf, lk->filename.alloc);\n+\t\tstrbuf_setlen(&lk->filename, strlen(lk->filename.buf));\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_SUFFIX);\n-\tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\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) {\n-\t\tlk->filename[0] = 0;\n+\t\tstrbuf_reset(&lk->filename);\n \t\treturn -1;\n \t}\n \tlk->owner = getpid();\n \tlk->active = 1;\n-\tif (adjust_shared_perm(lk->filename)) {\n+\tif (adjust_shared_perm(lk->filename.buf)) {\n \t\tint save_errno = errno;\n-\t\terror(\"cannot fix permission bits on %s\", lk->filename);\n+\t\terror(\"cannot fix permission bits on %s\", lk->filename.buf);\n \t\trollback_lock_file(lk);\n \t\terrno = save_errno;\n \t\treturn -1;\n@@ -319,7 +314,7 @@ int reopen_lock_file(struct lock_file *lk)\n \t\tdie(_(\"BUG: reopen a lockfile that is still open\"));\n \tif (!lk->active)\n \t\tdie(_(\"BUG: reopen a lockfile that has been committed\"));\n-\tlk->fd = open(lk->filename, O_WRONLY);\n+\tlk->fd = open(lk->filename.buf, O_WRONLY);\n \treturn lk->fd;\n }\n \n@@ -336,14 +331,14 @@ int commit_lock_file(struct lock_file *lk)\n \t\tgoto rollback;\n \n \t/* remove \".lock\": */\n-\tstrbuf_add(&result_file, lk->filename,\n-\t\t strlen(lk->filename) - LOCK_SUFFIX_LEN);\n-\terr = rename(lk->filename, result_file.buf);\n+\tstrbuf_add(&result_file, lk->filename.buf,\n+\t\t lk->filename.len - LOCK_SUFFIX_LEN);\n+\terr = rename(lk->filename.buf, result_file.buf);\n \tstrbuf_reset(&result_file);\n \tif (err)\n \t\tgoto rollback;\n \tlk->active = 0;\n-\tlk->filename[0] = 0;\n+\tstrbuf_reset(&lk->filename);\n \treturn 0;\n \n rollback:\n@@ -368,7 +363,7 @@ void rollback_lock_file(struct lock_file *lk)\n \n \tif (lk->fd >= 0)\n \t\tclose_lock_file(lk);\n-\tunlink_or_warn(lk->filename);\n+\tunlink_or_warn(lk->filename.buf);\n \tlk->active = 0;\n-\tlk->filename[0] = 0;\n+\tstrbuf_reset(&lk->filename);\n }\ndiff --git a/read-cache.c b/read-cache.c\nindex e64bc87..c126916 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2026,10 +2026,10 @@ static 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->filename.buf, alternate_index_output))\n \t\t\treturn -1;\n \t\tlk->active = 0;\n-\t\tlk->filename[0] = 0;\n+\t\tstrbuf_reset(&lk->filename);\n \t\treturn 0;\n \t} else {\n \t\treturn commit_lock_file(lk);\ndiff --git a/refs.c b/refs.c\nindex 9971ac5..c6e15f9a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2557,8 +2557,8 @@ static int delete_ref_loose(struct ref_lock *lock, int flag)\n \t\t * lockfile name, minus \".lock\":\n \t\t */\n \t\tchar *loose_filename = xmemdupz(\n-\t\t\t\tlock->lk->filename,\n-\t\t\t\tstrlen(lock->lk->filename) - LOCK_SUFFIX_LEN);\n+\t\t\t\tlock->lk->filename.buf,\n+\t\t\t\tlock->lk->filename.len - LOCK_SUFFIX_LEN);\n \t\tint err = unlink_or_warn(loose_filename);\n \t\tfree(loose_filename);\n \t\tif (err && errno != ENOENT)\n@@ -2922,7 +2922,7 @@ int write_ref_sha1(struct ref_lock *lock,\n \t write_in_full(lock->lock_fd, &term, 1) != 1 ||\n \t close_ref(lock) < 0) {\n \t\tint save_errno = errno;\n-\t\terror(\"Couldn't write %s\", lock->lk->filename);\n+\t\terror(\"Couldn't write %s\", lock->lk->filename.buf);\n \t\tunlock_ref(lock);\n \t\terrno = save_errno;\n \t\treturn -1;\ndiff --git a/shallow.c b/shallow.c\nindex de07709..33f3c7f 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->filename.buf);\n+\t\t*alternate_shallow_file = shallow_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.filename.buf);\n \t\tcommit_lock_file(&shallow_lock);\n \t} else {\n \t\tunlink(git_path(\"shallow\"));\n-- \n2.1.0\n"},{"id":"249550","messageId":"1410896036-12750-30-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 29/35] resolve_symlink(): use a strbuf for internal scratch space","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:50Z","receivedAt":"2014-09-16T19:33:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Aside from shortening and simplifying the code, this removes another\nplace where the path name length is arbitrarily limited.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 33 ++++++++++++---------------------\n 1 file changed, 12 insertions(+), 21 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex eecb526..ae55f33 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -141,44 +141,35 @@ static char *last_path_elm(char *p)\n static char *resolve_symlink(char *p, size_t s)\n {\n \tint depth = MAXDEPTH;\n+\tstatic struct strbuf link = STRBUF_INIT;\n \n \twhile (depth--) {\n-\t\tchar link[PATH_MAX];\n-\t\tint link_len = readlink(p, 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}\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\t}\n+\t\tif (strbuf_readlink(&link, p, strlen(p)) < 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/* absolute path simply replaces p */\n-\t\t\tif (link_len < s)\n-\t\t\t\tstrcpy(p, link);\n+\t\t\tif (link.len < s)\n+\t\t\t\tstrcpy(p, link.buf);\n \t\t\telse {\n \t\t\t\twarning(\"%s: symlink too long\", p);\n-\t\t\t\treturn p;\n+\t\t\t\tbreak;\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 * link is a relative path, so 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\tif (r - p + link.len < s)\n+\t\t\t\tstrcpy(r, link.buf);\n \t\t\telse {\n \t\t\t\twarning(\"%s: symlink too long\", p);\n-\t\t\t\treturn p;\n+\t\t\t\tbreak;\n \t\t\t}\n \t\t}\n \t}\n+\tstrbuf_reset(&link);\n \treturn p;\n }\n \n-- \n2.1.0\n"},{"id":"249553","messageId":"1410896036-12750-31-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 30/35] resolve_symlink(): take a strbuf parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:51Z","receivedAt":"2014-09-16T19:33:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change resolve_symlink() to take a strbuf rather than a string as\nparameter. This simplifies the code and removes an arbitrary pathname\nlength restriction. It also means that lock_file's filename field no\nlonger needs to be initialized to a large size.\n\nHelped-by: Torsten Bögershausen \nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 57 ++++++++++++++++++++++-----------------------------------\n 1 file changed, 22 insertions(+), 35 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex ae55f33..e689a84 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -124,58 +124,47 @@ static char *last_path_elm(char *p)\n #define MAXDEPTH 5\n \n /*\n- * p = path that may be a symlink\n- * s = full size of p\n+ * path contains a path that might be a symlink.\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+ * 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- * 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- *\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 the original path.\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 \tstatic struct strbuf link = STRBUF_INIT;\n \n \twhile (depth--) {\n-\t\tif (strbuf_readlink(&link, p, strlen(p)) < 0)\n+\t\tif (strbuf_readlink(&link, path->buf, path->len) < 0)\n \t\t\tbreak;\n \n-\t\tif (is_absolute_path(link.buf)) {\n+\t\tif (is_absolute_path(link.buf))\n \t\t\t/* absolute path simply replaces p */\n-\t\t\tif (link.len < s)\n-\t\t\t\tstrcpy(p, link.buf);\n-\t\t\telse {\n-\t\t\t\twarning(\"%s: symlink too long\", p);\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t} else {\n+\t\t\tstrbuf_reset(path);\n+\t\telse {\n \t\t\t/*\n \t\t\t * link is a relative path, so 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.buf);\n-\t\t\telse {\n-\t\t\t\twarning(\"%s: symlink too long\", p);\n-\t\t\t\tbreak;\n-\t\t\t}\n+\t\t\tchar *r = last_path_elm(path->buf);\n+\t\t\tstrbuf_setlen(path, r - path->buf);\n \t\t}\n+\n+\t\tstrbuf_addbuf(path, &link);\n \t}\n \tstrbuf_reset(&link);\n-\treturn p;\n }\n \n /* Make sure errno contains a meaningful value on error */\n static int lock_file(struct lock_file *lk, const char *path, int flags)\n {\n+\tsize_t pathlen = strlen(path);\n+\n \tif (!lock_file_list) {\n \t\t/* One-time initialization */\n \t\tsigchain_push_common(remove_lock_file_on_signal);\n@@ -192,7 +181,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\tlk->active = 0;\n \t\tlk->owner = 0;\n \t\tlk->on_list = 1;\n-\t\tstrbuf_init(&lk->filename, PATH_MAX);\n+\t\tstrbuf_init(&lk->filename, pathlen + LOCK_SUFFIX_LEN);\n \t\tlk->next = lock_file_list;\n \t\tlock_file_list = lk;\n \t} else if (lk->filename.len) {\n@@ -201,11 +190,9 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\t path);\n \t}\n \n-\tstrbuf_addstr(&lk->filename, path);\n-\tif (!(flags & LOCK_NODEREF)) {\n-\t\tresolve_symlink(lk->filename.buf, lk->filename.alloc);\n-\t\tstrbuf_setlen(&lk->filename, strlen(lk->filename.buf));\n-\t}\n+\tstrbuf_add(&lk->filename, path, pathlen);\n+\tif (!(flags & LOCK_NODEREF))\n+\t\tresolve_symlink(&lk->filename);\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) {\n-- \n2.1.0\n"},{"id":"249531","messageId":"1410896036-12750-32-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 31/35] trim_last_path_component(): replace last_path_elm()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:52Z","receivedAt":"2014-09-16T19:33:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Rewrite last_path_elm() to take a strbuf parameter and to trim off the\nlast path name element in place rather than returning a pointer to the\nbeginning of the last path name element. This simplifies the function\na bit and makes it integrate better with its caller, which is now also\nstrbuf-based. Rename the function accordingly and a bit less tersely.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 38 ++++++++++++++++----------------------\n 1 file changed, 16 insertions(+), 22 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex e689a84..3d5ab4f 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -91,32 +91,28 @@ 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_component(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+\twhile (i && path->buf[i - 1] == '/')\n+\t\ti--;\n \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+\twhile (i && path->buf[i - 1] != '/')\n+\t\ti--;\n+\n+\tstrbuf_setlen(path, i);\n }\n \n \n@@ -146,14 +142,12 @@ static void resolve_symlink(struct strbuf *path)\n \t\tif (is_absolute_path(link.buf))\n \t\t\t/* absolute path simply replaces p */\n \t\t\tstrbuf_reset(path);\n-\t\telse {\n+\t\telse\n \t\t\t/*\n \t\t\t * link is a relative path, so replace the\n \t\t\t * last element of p with it.\n \t\t\t */\n-\t\t\tchar *r = last_path_elm(path->buf);\n-\t\t\tstrbuf_setlen(path, r - path->buf);\n-\t\t}\n+\t\t\ttrim_last_path_component(path);\n \n \t\tstrbuf_addbuf(path, &link);\n \t}\n-- \n2.1.0\n"},{"id":"249555","messageId":"1410896036-12750-33-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 32/35] Extract a function commit_lock_file_to()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:53Z","receivedAt":"2014-09-16T19:33:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"commit_locked_index(), when writing to an alternate index file,\nduplicates (poorly) the code in commit_lock_file(). And anyway, it\nshouldn't have to know so much about the internal workings of lockfile\nobjects. So extract a new function commit_lock_file_to() that does the\nwork common to the two functions, and call it from both\ncommit_lock_file() and commit_locked_index().\n\nSigned-off-by: Michael Haggerty \n---\n Documentation/technical/api-lockfile.txt | 14 ++++++++----\n cache.h | 1 +\n lockfile.c | 39 +++++++++++++++++++++-----------\n read-cache.c | 13 +++--------\n 4 files changed, 40 insertions(+), 27 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex 2514559..3ee4299 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -73,6 +73,12 @@ commit_lock_file::\n \t`commit_lock_file()` for a `lock_file` object that is not\n \tcurrently locked.\n \n+commit_lock_file_to::\n+\n+\tLike `commit_lock_file()`, except that it takes an explicit\n+\t`path` argument to which the lockfile should be renamed. The\n+\t`path` must be on the same filesystem as the lock file.\n+\n rollback_lock_file::\n \n \tTake a pointer to the `struct lock_file` initialized\n@@ -91,10 +97,10 @@ Because the structure is used in an `atexit(3)` handler, its\n storage has to stay throughout the life of the program. It\n cannot be an auto variable allocated on the stack.\n \n-Call `commit_lock_file()` or `rollback_lock_file()` when you are\n-done writing to the file descriptor. If you do not call either\n-and simply `exit(3)` from the program, an `atexit(3)` handler\n-will close and remove the lockfile.\n+Call `commit_lock_file()`, `commit_lock_file_to()`, or\n+`rollback_lock_file()` when you are done writing to the file\n+descriptor. If you do not call either and simply `exit(3)` from the\n+program, an `atexit(3)` handler will close and remove the lockfile.\n \n If you need to close the file descriptor you obtained from\n `hold_lock_file_for_update` function yourself, do so by calling\ndiff --git a/cache.h b/cache.h\nindex 307cc8e..45688d5 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -590,6 +590,7 @@ extern void unable_to_lock_message(const char *path, int err,\n extern NORETURN void unable_to_lock_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 commit_lock_file_to(struct lock_file *, const char *path);\n extern int commit_lock_file(struct lock_file *);\n extern int reopen_lock_file(struct lock_file *);\n extern void update_index_if_able(struct index_state *, struct lock_file *);\ndiff --git a/lockfile.c b/lockfile.c\nindex 3d5ab4f..480c2ba 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -58,8 +58,9 @@\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:\n+ * - Unlocked (after commit_lock_file(), commit_lock_file_to(),\n+ * rollback_lock_file(), or a failed attempt to lock). In this\n+ * state:\n * - active is unset\n * - filename is empty (usually, though there are transitory\n * states in which this condition doesn't hold)\n@@ -290,24 +291,16 @@ int reopen_lock_file(struct lock_file *lk)\n \treturn lk->fd;\n }\n \n-int commit_lock_file(struct lock_file *lk)\n+int commit_lock_file_to(struct lock_file *lk, const char *path)\n {\n-\tstatic struct strbuf result_file = STRBUF_INIT;\n \tint save_errno;\n-\tint err;\n \n \tif (!lk->active)\n-\t\tdie(\"BUG: attempt to commit unlocked object\");\n+\t\tdie(\"BUG: attempt to commit unlocked object to \\\"%s\\\"\", path);\n \n \tif (lk->fd >= 0 && close_lock_file(lk))\n \t\tgoto rollback;\n-\n-\t/* remove \".lock\": */\n-\tstrbuf_add(&result_file, lk->filename.buf,\n-\t\t lk->filename.len - LOCK_SUFFIX_LEN);\n-\terr = rename(lk->filename.buf, result_file.buf);\n-\tstrbuf_reset(&result_file);\n-\tif (err)\n+\tif (rename(lk->filename.buf, path))\n \t\tgoto rollback;\n \tlk->active = 0;\n \tstrbuf_reset(&lk->filename);\n@@ -320,6 +313,26 @@ rollback:\n \treturn -1;\n }\n \n+int commit_lock_file(struct lock_file *lk)\n+{\n+\tstatic struct strbuf result_file = STRBUF_INIT;\n+\tint err;\n+\n+\tif (!lk->active)\n+\t\tdie(\"BUG: attempt to commit unlocked object\");\n+\n+\tif (lk->filename.len <= LOCK_SUFFIX_LEN ||\n+\t strcmp(lk->filename.buf + lk->filename.len - LOCK_SUFFIX_LEN, LOCK_SUFFIX))\n+\t\tdie(\"BUG: lockfile filename corrupt\");\n+\n+\t/* remove \".lock\": */\n+\tstrbuf_add(&result_file, lk->filename.buf,\n+\t\t lk->filename.len - LOCK_SUFFIX_LEN);\n+\terr = commit_lock_file_to(lk, result_file.buf);\n+\tstrbuf_reset(&result_file);\n+\treturn err;\n+}\n+\n int hold_locked_index(struct lock_file *lk, int die_on_error)\n {\n \treturn hold_lock_file_for_update(lk, get_index_file(),\ndiff --git a/read-cache.c b/read-cache.c\nindex c126916..f3eb5e9 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2023,17 +2023,10 @@ void set_alternate_index_output(const char *name)\n \n static int commit_locked_index(struct lock_file *lk)\n {\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.buf, alternate_index_output))\n-\t\t\treturn -1;\n-\t\tlk->active = 0;\n-\t\tstrbuf_reset(&lk->filename);\n-\t\treturn 0;\n-\t} else {\n+\tif (alternate_index_output)\n+\t\treturn commit_lock_file_to(lk, alternate_index_output);\n+\telse\n \t\treturn commit_lock_file(lk);\n-\t}\n }\n \n static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,\n-- \n2.1.0\n"},{"id":"249532","messageId":"1410896036-12750-34-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 33/35] Rename LOCK_NODEREF to LOCK_NO_DEREF","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:54Z","receivedAt":"2014-09-16T19:33:54Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This makes it harder to misread the name as LOCK_NODE_REF.\n\nSuggested-by: Torsten Bögershausen \nSigned-off-by: Michael Haggerty \n---\n Documentation/technical/api-lockfile.txt | 4 ++--\n cache.h | 2 +-\n lockfile.c | 4 ++--\n refs.c | 2 +-\n 4 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex 3ee4299..40cd524 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -31,11 +31,11 @@ hold_lock_file_for_update::\n \tto the file. The flags parameter is a combination of\n +\n --\n-LOCK_NODEREF::\n+LOCK_NO_DEREF::\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+\thowever, if `LOCK_NO_DEREF` is set, then the lockfile is\n \tcreated by adding \".lock\" to the path argument itself.\n \n LOCK_DIE_ON_ERROR::\ndiff --git a/cache.h b/cache.h\nindex 45688d5..d610fab 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -583,7 +583,7 @@ struct lock_file {\n \tstruct strbuf filename;\n };\n #define LOCK_DIE_ON_ERROR 1\n-#define LOCK_NODEREF 2\n+#define LOCK_NO_DEREF 2\n extern int unable_to_lock_error(const char *path, int err);\n extern void unable_to_lock_message(const char *path, int err,\n \t\t\t\t struct strbuf *buf);\ndiff --git a/lockfile.c b/lockfile.c\nindex 480c2ba..432d624 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -17,7 +17,7 @@\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+ * replaced. However, if LOCK_NO_DEREF 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@@ -186,7 +186,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t}\n \n \tstrbuf_add(&lk->filename, path, pathlen);\n-\tif (!(flags & LOCK_NODEREF))\n+\tif (!(flags & LOCK_NO_DEREF))\n \t\tresolve_symlink(&lk->filename);\n \tstrbuf_addstr(&lk->filename, LOCK_SUFFIX);\n \tlk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\ndiff --git a/refs.c b/refs.c\nindex c6e15f9a..525ce4b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2134,7 +2134,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tlflags = 0;\n \tif (flags & REF_NODEREF) {\n \t\trefname = orig_refname;\n-\t\tlflags |= LOCK_NODEREF;\n+\t\tlflags |= LOCK_NO_DEREF;\n \t}\n \tlock->ref_name = xstrdup(refname);\n \tlock->orig_ref_name = xstrdup(orig_refname);\n-- \n2.1.0\n"},{"id":"249554","messageId":"1410896036-12750-35-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 34/35] lockfile.c: rename static functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:55Z","receivedAt":"2014-09-16T19:33:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"* remove_lock_file() -> remove_lock_files()\n* remove_lock_file_on_signal() -> remove_lock_files_on_signal()\n\nSuggested-by: Torsten Bögershausen \nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 432d624..0b63554 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -73,7 +73,7 @@\n \n static struct lock_file *volatile lock_file_list;\n \n-static void remove_lock_file(void)\n+static void remove_lock_files(void)\n {\n \tpid_t me = getpid();\n \n@@ -84,9 +84,9 @@ static void remove_lock_file(void)\n \t}\n }\n \n-static void remove_lock_file_on_signal(int signo)\n+static void remove_lock_files_on_signal(int signo)\n {\n-\tremove_lock_file();\n+\tremove_lock_files();\n \tsigchain_pop(signo);\n \traise(signo);\n }\n@@ -162,8 +162,8 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\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\tsigchain_push_common(remove_lock_files_on_signal);\n+\t\tatexit(remove_lock_files);\n \t}\n \n \tif (lk->active)\n-- \n2.1.0\n"},{"id":"249533","messageId":"1410896036-12750-36-git-send-email-mhagger@alum.mit.edu","threadId":"37580","inReplyTo":"1410896036-12750-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v5 35/35] get_locked_file_path(): new function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-16T19:33:56Z","receivedAt":"2014-09-16T19:33:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add a function to return the path of the file that is locked by a\nlock_file object. This reduces the knowledge that callers have to have\nabout the lock_file layout.\n\nSuggested-by: Ronnie Sahlberg \nSigned-off-by: Michael Haggerty \n---\n Documentation/technical/api-lockfile.txt | 5 +++++\n cache.h | 1 +\n lockfile.c | 9 +++++++++\n refs.c | 4 +---\n 4 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex 40cd524..22f1b5c 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -52,6 +52,11 @@ hold_lock_file_for_append::\n \tlockfile and its write pointer is positioned at the end of the\n \tfile before returning.\n \n+get_locked_file_path::\n+\n+\tReturn the path of the file that is locked by the specified\n+\tlock_file object. The caller must free the memory.\n+\n unable_to_lock_error::\n \n \tEmit an error describing that there was an error locking the\ndiff --git a/cache.h b/cache.h\nindex d610fab..4bbaffa 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -590,6 +590,7 @@ extern void unable_to_lock_message(const char *path, int err,\n extern NORETURN void unable_to_lock_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 char *get_locked_file_path(struct lock_file *);\n extern int commit_lock_file_to(struct lock_file *, const char *path);\n extern int commit_lock_file(struct lock_file *);\n extern int reopen_lock_file(struct lock_file *);\ndiff --git a/lockfile.c b/lockfile.c\nindex 0b63554..42cea0a 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -274,6 +274,15 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n \treturn fd;\n }\n \n+char *get_locked_file_path(struct lock_file *lk)\n+{\n+\tif (!lk->active)\n+\t\tdie(\"BUG: get_locked_file_path() called for unlocked object\");\n+\tif (lk->filename.len <= LOCK_SUFFIX_LEN)\n+\t\tdie(\"BUG: get_locked_file_path() called for malformed lock object\");\n+\treturn xmemdupz(lk->filename.buf, lk->filename.len - LOCK_SUFFIX_LEN);\n+}\n+\n int close_lock_file(struct lock_file *lk)\n {\n \tint fd = lk->fd;\ndiff --git a/refs.c b/refs.c\nindex 525ce4b..5842dd0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2556,9 +2556,7 @@ 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(\n-\t\t\t\tlock->lk->filename.buf,\n-\t\t\t\tlock->lk->filename.len - LOCK_SUFFIX_LEN);\n+\t\tchar *loose_filename = get_locked_file_path(lock->lk);\n \t\tint err = unlink_or_warn(loose_filename);\n \t\tfree(loose_filename);\n \t\tif (err && errno != ENOENT)\n-- \n2.1.0\n"},{"id":"249557","messageId":"20140916195241.GA29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 01/35] unable_to_lock_die(): rename function from unable_to_lock_index_die()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T19:52:41Z","receivedAt":"2014-09-16T19:52:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> This function is used for other things besides the index, so rename it\n> accordingly.\n\nMakes sense.\n\n[...]\n> builtin/update-index.c | 2 +-\n> cache.h | 2 +-\n> lockfile.c | 6 +++---\n> refs.c | 2 +-\n> 4 files changed, 6 insertions(+), 6 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"249558","messageId":"20140916202525.GB29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 02/35] api-lockfile: expand the documentation","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T20:25:25Z","receivedAt":"2014-09-16T20:25:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> Document a couple more functions and the flags argument as used by\n> hold_lock_file_for_update() and hold_lock_file_for_append().\n\nThanks.\n\n[...]\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\nContext: this document has structure\n\n\tlockfile API\n\t============\n\n\tExplanation of purpose (nice!).\n\n\tThe functions\n\t-------------\n\n\tQuick descriptions of each of the four functions\n\t`hold_lock_file_for_update`, `commit_lock_file`,\n\t`rollback_lock_file`, `close_lock_file`.\n\n\tReminder about lifetime of the lock_file structure.\n\n\tDescription of cleanup convention (thou shalt either\n\tcommit or roll back; if you forget to, the atexit\n\thandler will roll back for you).\n\n\tLong warning about the harder use cases. The above\n\t\"thou shalt\" was a lie --- you can also\n\tclose_lock_file if you know what you're doing\n\t[jn: why is that function part of the public API?].\n\nWhat's nice about the existing structure is that you can get\na sense of how to use the API at a glance. Would there be a\nway to add this extra information while preserving that property?\n\nE.g.:\n\n\tlockfile API\n\t============\n\n\tNice brief explanation of purpose (\"is this the API\n\tI want to use?\"), as before.\n\n\tCalling sequence\n\t----------------\n\n\tThe caller:\n\n\t* Allocates a variable `struct lock_file lock` in the bss\n\tsection or heap. Because the `lock_file` structure is used\n\tin an `atexit(3)` handler, its storage has to stay\n\tthroughout the life of the program. It cannot be an auto\n\tvariable allocated on the stack.\n\n\t* Attempts to create a lockfile by passing that variable and\n\tthe filename of the final destination (e.g. `$GIT_DIR/index`)\n\tto `hold_lock_file_for_update` or `hold_lock_file_for_append`.\n\t+\n\tIf the `die_on_error` flag is true, git dies if a lock is\n\talready taken for the file.\n\n\t* Writes new content for the destination file by writing to\n\t`lock->fd`.\n\n\tWhen finished writing, the caller can:\n\n\t* Close the file descriptor and rename the lockfile to\n\tits final destination by calling `commit_lock_file`.\n\n\t* Close the file descriptor and remove the lockfile by\n\tcalling `rollback_lock_file`.\n\n\t* Close the file descriptor without removing or renaming\n\tthe lockfile by calling `close_lock_file`.\n\n\tIf you do not call one of `commit_lock_file`,\n\t`rollback_lock_file`, and `close_lock_file` and instead\n\tsimply `exit(3)` from the program, an `atexit(3)` handler will\n\tclose and remove the lockfile.\n\n\tYou should never call `close(2)` on `lock->fd` yourself~\n\tOtherwise the ...\n\n\tError handling\n\t--------------\n\n\tFunctions return 0 on success, -1 on failure. errno is?\n\tisn't? meaningful on error.\n\n\t... description of unable_to_lock_error and unable_to_lock_die\n\there ...\n\n\tFlags\n\t-----\n\n\tLOCK_NODEREF::\n\n\t\tUsually symbolic links in the destination path are\n\t\tresolved and the lockfile is created by adding \".lock\"\n\t\tto the resolved path. If `LOCK_NODEREF` is set, then\n\t\tthe lockfile is created by adding \".lock\" to the path\n\t\targument itself.\n\nWhat is the user-visible effect of that flag? When would I want\nto pass that flag, and when wouldn't I?\n\n\tLOCK_DIE_ON_ERROR::\n\n\t\tIf a lock is already taken for the file, `die()` with\n\t\tan error message. If this option is not specified,\n\t\ttrying to hold a lock file that is already taken will\n\t\treturn -1 to the caller.\n\nSensible?\nJonathan\n"},{"id":"249561","messageId":"20140916203741.GC29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-4-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 03/35] rollback_lock_file(): do not clear filename redundantly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T20:37:41Z","receivedAt":"2014-09-16T20:37:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> It is only necessary to clear the lock_file's filename field if it was\n> not already clear.\n[...]\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -276,6 +276,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\nNow that it does nothing when lk->filename[0] == '\\0', this could be\nde-indented a little:\n\n\tif (!lk->filename[0])\n\t\treturn;\n\tif (lk->fd >= 0)\n\t\tclose(lk->fd);\n\tunlink_or_warn(lk->filename);\n\tlk->filename[0] = 0;\n\nThis could technically double-close if interrupted by a signal that\ntries to remove the file again, right? Should this use\nclose_lock_file which sets fd to -1 before closing?\n\n\tif (!lk->filename[0])\n\t\treturn;\n\tclose_lock_file(lk);\n\tunlink_or_warn(lk->filename);\n\tlk->filename[0] = 0;\n\nWith or without such changes,\nReviewed-by: Jonathan Nieder \n"},{"id":"249562","messageId":"20140916203826.GD29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 04/35] rollback_lock_file(): exit early if lock is not active","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T20:38:26Z","receivedAt":"2014-09-16T20:38:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> Eliminate a layer of nesting.\n\nOh --- guess I should have been more patient. :)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"249563","messageId":"20140916203852.GE29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-6-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 05/35] rollback_lock_file(): set fd to -1","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T20:38:52Z","receivedAt":"2014-09-16T20:38:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -276,7 +276,7 @@ void rollback_lock_file(struct lock_file *lk)\n> \t\treturn;\n> \n> \tif (lk->fd >= 0)\n> -\t\tclose(lk->fd);\n> +\t\tclose_lock_file(lk);\n\nDoesn't need to be guarded by the 'if'.\n"},{"id":"249564","messageId":"20140916203957.GF29050@google.com","threadId":"37580","inReplyTo":"20140916203852.GE29050@google.com","subject":"Re: [PATCH v5 05/35] rollback_lock_file(): set fd to -1","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T20:39:57Z","receivedAt":"2014-09-16T20:39:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n\n>> --- a/lockfile.c\n>> +++ b/lockfile.c\n>> @@ -276,7 +276,7 @@ void rollback_lock_file(struct lock_file *lk)\n>> \t\treturn;\n>> \n>> \tif (lk->fd >= 0)\n>> -\t\tclose(lk->fd);\n>> +\t\tclose_lock_file(lk);\n>\n> Doesn't need to be guarded by the 'if'.\n\nErr, yes it does.\n\nWhy doesn't close_lock_file bail out early when fd < 0?\n\nIn any case,\nReviewed-by: Jonathan Nieder \n"},{"id":"249565","messageId":"20140916204210.GG29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-7-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 06/35] lockfile: unlock file if lockfile permissions cannot be adjusted","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T20:42:10Z","receivedAt":"2014-09-16T20:42:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> Signed-off-by: Michael Haggerty \n> ---\n> lockfile.c | 1 +\n> 1 file changed, 1 insertion(+)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"249566","messageId":"20140916204853.GH29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-8-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 07/35] hold_lock_file_for_append(): release lock on errors","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T20:48:53Z","receivedAt":"2014-09-16T20:48:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -219,13 +219,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\nMakes sense.\n\nNow that I'm here, I wonder a little at the error convention. If the\ncaller doesn't pass LOCK_DIE_ON_ERROR, are they supposed to be able to\nuse unable_to_lock_message? What errno would they pass in the err\nparameter? Would callers want handle failure to acquire a lock\ndifferently from other errors (e.g., by sleeping and trying again),\nand if not, what is the optionally-die behavior in hold_lock_file\nabout?\n\nIn any case,\nReviewed-by: Jonathan Nieder \n"},{"id":"249567","messageId":"20140916205736.GI29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-9-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 08/35] lock_file(): always add lock_file object to lock_file_list","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T20:57:36Z","receivedAt":"2014-09-16T20:57:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> The ambiguity didn't have any ill effects, because lock_file objects\n> cannot be removed from the lock_file_list anyway. But it is\n> unnecessary to leave this behavior inconsistent.\n\nNit: commit messages usually use present tense for current behavior\n(and imperative for the behavior after the patch).\n\nMore importantly, the above + the diffstat don't leave me very happy\nabout the change.\n\nCan you spell out more about the intended effect? E.g., is this about\nmaking sure other code is appropriately exercised to tolerate the\nsignal handler even when there are a lot of errors (which should make\ndebugging easier) or something?\n"},{"id":"249568","messageId":"20140916210317.GJ29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-10-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 09/35] lockfile.c: document the various states of lock_file objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T21:03:17Z","receivedAt":"2014-09-16T21:03:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -4,6 +4,63 @@\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\nThis overlaps a lot with the API doc, which makes me worry a little\nabout it going out of date. Would improving the API doc help, or if\naspects are especially relevant here, is there a way to summarize them\nmore quickly to avoid some of the redundancy?\n\n[...]\n> + * A lock_file object can be in several states:\n\nWould it make sense for this information to go near the definition of\n'struct lock_file'? That's where I'd start if I were looking for\ninformation on what states a lock_file can be in.\n\nMy two cents,\nJonathan\n"},{"id":"249569","messageId":"20140916210538.GK29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-11-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 10/35] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T21:05:38Z","receivedAt":"2014-09-16T21:05:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> There are a few places that use these values, so define constants for\n> them.\n\nSeems like a symptom of the API leaving out a useful helper (e.g.,\nsomething that strips off the lock suffix and returns a memdupz'd\nfilename).\n\n[...]\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -570,6 +570,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);\n> #define REFRESH_IN_PORCELAIN\t0x0020\t/* user friendly output, not \"needs update\" */\n> extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);\n> \n> +/* String appended to a filename to derive the lockfile name: */\n> +#define LOCK_SUFFIX \".lock\"\n> +#define LOCK_SUFFIX_LEN 5\n\nMy suspicion is that error handling would be better if fewer callers\noutside of lockfile.c did the '- LOCK_SUFFIX_LEN' dance, so this seems\nlike a step in the wrong direction.\n\nAdding constants in lockfile.c would make sense, though.\n\nThanks,\nJonathan\n"},{"id":"249570","messageId":"20140916211140.GL29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-12-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 11/35] delete_ref_loose(): don't muck around in the lock_file's filename","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T21:11:40Z","receivedAt":"2014-09-16T21:11:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> It's bad manners. Especially since there could be a signal during the\n> call to unlink_or_warn(), in which case the signal handler will see\n> the wrong filename and delete the reference file, leaving the lockfile\n> behind.\n>\n> So make our own copy to work with.\n\nNice. Could this be a helper, to help others avoid a PATH_MAX\nsized buffer too?\n\n[...]\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2551,12 +2551,15 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n> static int delete_ref_loose(struct ref_lock *lock, int flag)\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(\n> +\t\t\t\tlock->lk->filename,\n> +\t\t\t\tstrlen(lock->lk->filename) - LOCK_SUFFIX_LEN);\n\nRonnie mentioned that this would misbehave in the\n!lock->lk->filename[0] case, which shouldn't come up because\nlock_ref_sha1_basic dies on error but seems worth futureproofing\nagainst. Maybe something like\n\n\tassert(lock->lk->filename[0]);\n\nwould be enough to make the assumption obvious for people reading.\n\nOtherwise looks good.\n\nThanks,\nJonathan\n"},{"id":"249571","messageId":"20140916211709.GM29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-13-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 12/35] prepare_index(): declare return value to be (const char *)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T21:17:09Z","receivedAt":"2014-09-16T21:17:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> Declare the return value to be const to make it clear that we aren't\n> giving callers permission to write over the string that it points at.\n> (The return value is the filename field of a struct lock_file, which\n> can be used by a signal handler at any time and therefore shouldn't be\n> tampered with.)\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> builtin/commit.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n\nReviewed-by: Jonathan Nieder \n\nI wonder if we should just bite the bullet and make lock_file an\nopaque struct with accessors for int fd and const char *filename.\n"},{"id":"249573","messageId":"20140916221226.GN29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-15-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 14/35] lock_file(): exit early if lockfile cannot be opened","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T22:12:27Z","receivedAt":"2014-09-16T22:12:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> lockfile.c | 23 +++++++++++------------\n> 1 file changed, 11 insertions(+), 12 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"249574","messageId":"20140916221312.GO29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-16-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 15/35] remove_lock_file(): call rollback_lock_file()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T22:13:12Z","receivedAt":"2014-09-16T22:13:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> lockfile.c | 8 ++------\n> 1 file changed, 2 insertions(+), 6 deletions(-)\n\nNice.\n\nReviewed-by: Jonathan Nieder \n"},{"id":"249575","messageId":"20140916221647.GP29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-17-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 16/35] commit_lock_file(): inline temporary variable","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T22:16:47Z","receivedAt":"2014-09-16T22:16:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -311,12 +311,14 @@ int reopen_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> +\n> \tif (lk->fd >= 0 && close_lock_file(lk))\n> \t\treturn -1;\n> +\n> \tstrcpy(result_file, lk->filename);\n> -\ti = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */\n> -\tresult_file[i] = 0;\n> +\t/* remove \".lock\": */\n> +\tresult_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;\n> +\n\nSame comment as the delete_ref_loose patch: the reader can gain some\npiece of mind from an assertion at the top that makes sure filename is\nvalid (which it always should be, according to the API):\n\n\tassert(lk->filename[0]);\n\nIt would also be nice to use the same xmemdupz-based code as in the\ndelete_ref_loose case (ideally via a helper), to avoid having to work\nwith a PATH_MAX-sized buffer.\n\nOtherwise looks good.\n\nThanks,\nJonathan\n"},{"id":"249576","messageId":"20140916221745.GQ29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-18-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 17/35] commit_lock_file(): die() if called for unlocked lockfile object","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T22:17:45Z","receivedAt":"2014-09-16T22:17:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -312,6 +312,9 @@ int commit_lock_file(struct lock_file *lk)\n> {\n> \tchar result_file[PATH_MAX];\n> \n> +\tif (!lk->filename[0])\n> +\t\tdie(\"BUG: attempt to commit unlocked object\");\n\nSure, this is fine instead of an assert() too. :)\n\nThanks,\nJonathan\n"},{"id":"249577","messageId":"20140916221956.GR29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-19-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 18/35] commit_lock_file(): if close fails, roll back","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T22:19:56Z","receivedAt":"2014-09-16T22:19:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> If closing an open lockfile fails, then we cannot be sure of the\n> contents of the lockfile\n\nIs that true? It seems more like a bug in close_lock_file: if it\nfails, perhaps it should either set lk->fd back to fd or unlink the\nlockfile itself.\n\nWhat do other callers do on close_lock_file failure?\n\nJonathan\n"},{"id":"249578","messageId":"20140916222207.GS29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-20-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 19/35] commit_lock_file(): rollback lock file on failure to rename","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T22:22:08Z","receivedAt":"2014-09-16T22:22:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> If rename() fails, call rollback_lock_file() to delete the lock file\n> (in case it is still present) and reset the filename field to the\n> empty string so that the lockfile object is left in a valid state.\n\nCan you spell out more what the goal is? Is the idea to keep the\n.lock file for a shorter period of time, so other processes can lock\nthat file before the current process exits?\n"},{"id":"249579","messageId":"20140916222344.GT29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-21-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 20/35] api-lockfile: document edge cases","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T22:23:44Z","receivedAt":"2014-09-16T22:23:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> * Document the behavior of commit_lock_file() when it fails, namely\n> that it rolls back the lock_file object and sets errno\n> appropriately.\n>\n> * Document the behavior of rollback_lock_file() when called for a\n> lock_file object that has already been committed or rolled back,\n> namely that it is a NOOP.\n\nI think this would be easier to read in a separate error handling\nsection. That way, when writing new code using the lockfile API,\nI can quickly find what functions to use and quickly find out what\nthe error handling conventions are --- each use of the documentation\nwouldn't interfere with the other.\n\nJonathan\n"},{"id":"249580","messageId":"20140916222436.GU29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-22-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 21/35] dump_marks(): remove a redundant call to rollback_lock_file()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T22:24:36Z","receivedAt":"2014-09-16T22:24:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> Signed-off-by: Michael Haggerty \n> ---\n> fast-import.c | 4 +---\n> 1 file changed, 1 insertion(+), 3 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"249582","messageId":"20140916222814.GV29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-23-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 22/35] git_config_set_multivar_in_file(): avoid call to rollback_lock_file()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T22:28:14Z","receivedAt":"2014-09-16T22:28:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> After commit_lock_file() is called, then the lock_file object is\n> necessarily either committed or rolled back. So there is no need to\n> call rollback_lock_file() again in either of these cases.\n>\n> Signed-off-by: Michael Haggerty \n\nThis seems to involve an unadvertised behavior change: before\nit wouldn't call git_config_clear() on failure, and after the\npatch it would. Intended?\n\nI think it would be clearer with the goto for exception handling\nmaintained (and just a second 'lock = NULL' assignment).\n"},{"id":"249583","messageId":"20140916224523.GW29050@google.com","threadId":"37580","inReplyTo":"1410896036-12750-24-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 23/35] lockfile: avoid transitory invalid states","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-16T22:45:23Z","receivedAt":"2014-09-16T22:45:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> We could probably continue to use the filename field to encode the\n> state by being careful to write characters 1..N-1 of the filename\n> first, and then overwrite the NUL at filename[0] with the first\n> character of the filename, but that would be awkward and error-prone.\n> \n> So, instead of using the filename field to determine whether the\n> lock_file object is active, add a new field \"lock_file::active\" for\n> this purpose.\n\nNice.\n\n[...]\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -576,6 +576,7 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct\n> \n> struct lock_file {\n> \tstruct lock_file *next;\n> +\tvolatile sig_atomic_t active;\n> \tint fd;\n> \tpid_t owner;\n> \tchar on_list;\n[...]\n> +++ b/lockfile.c\n> @@ -27,16 +27,19 @@\n[...]\n> @@ -189,9 +198,14 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n> \t\tatexit(remove_lock_file);\n> \t}\n> \n> +\tif (lk->active)\n> +\t\tdie(\"BUG: lock_file(\\\"%s\\\") called with an active lock_file object\",\n> +\t\t path);\n\nThe error message doesn't make it entirely obvious to me what went\nwrong.\n\nMaybe something like\n\n\t\tdie(\"BUG: cannot lock_file(\\\"%s\\\") on active struct lock_file\",\n\t\t path);\n\nlock_file already assumed on_list was initialized to zero but it\nwasn't particularly obvious since everything else is blindly\nscribbled over. Probably worth mentioning in the API docs that\nthe lock_file should be zeroed before calling hold_lock_file_...\n\n[...]\n> @@ -326,6 +341,7 @@ int commit_lock_file(struct lock_file *lk)\n> \tif (rename(lk->filename, result_file))\n> \t\tgoto rollback;\n> \n> +\tlk->active = 0;\n> \tlk->filename[0] = 0;\n\nIs it useful to set filename[0] any more?\n\nIt seems potentially fragile to set both, since new code can appear\nthat only sets or checks one or the other. Would it make sense to\nrename the filename field to make sure no new callers relying on the\nfilename[0] convention sneak in (with the new convention being that\nthe filename doesn't get cleared, to avoid problems)?\n\nJonathan\n"},{"id":"249598","messageId":"5419A299.2010700@alum.mit.edu","threadId":"37580","inReplyTo":"20140916203957.GF29050@google.com","subject":"Re: [PATCH v5 05/35] rollback_lock_file(): set fd to -1","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-17T15:02:49Z","receivedAt":"2014-09-17T15:02:49Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/16/2014 10:39 PM, Jonathan Nieder wrote:\n> Jonathan Nieder wrote:\n>> Michael Haggerty wrote:\n> \n>>> --- a/lockfile.c\n>>> +++ b/lockfile.c\n>>> @@ -276,7 +276,7 @@ void rollback_lock_file(struct lock_file *lk)\n>>> \t\treturn;\n>>> \n>>> \tif (lk->fd >= 0)\n>>> -\t\tclose(lk->fd);\n>>> +\t\tclose_lock_file(lk);\n>>\n>> Doesn't need to be guarded by the 'if'.\n> \n> Err, yes it does.\n> \n> Why doesn't close_lock_file bail out early when fd < 0?\n\nOK, I will change close_lock_file() to exit (with no error) when fd < 0.\nThen this \"if\" can also go away.\n\nThanks,\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249599","messageId":"5419AB44.1070109@alum.mit.edu","threadId":"37580","inReplyTo":"20140916204853.GH29050@google.com","subject":"Re: [PATCH v5 07/35] hold_lock_file_for_append(): release lock on errors","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-17T15:39:48Z","receivedAt":"2014-09-17T15:39:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/16/2014 10:48 PM, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n>> --- a/lockfile.c\n>> +++ b/lockfile.c\n>> @@ -219,13 +219,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> \n> Makes sense.\n> \n> Now that I'm here, I wonder a little at the error convention. If the\n> caller doesn't pass LOCK_DIE_ON_ERROR, are they supposed to be able to\n> use unable_to_lock_message? What errno would they pass in the err\n> parameter? Would callers want handle failure to acquire a lock\n> differently from other errors (e.g., by sleeping and trying again),\n> and if not, what is the optionally-die behavior in hold_lock_file\n> about?\n\nThe same applies to hold_lock_file_for_update(), so I'll discuss both at\nonce:\n\nMost callers do pass LOCK_DIE_ON_ERROR. Of the ones that don't, a couple\nappear to want to emit more meaningful error messages. A couple don't\ndie at all but return an error code to their caller. At least one\n(add_to_alternates_file()) calls die_errno().\n\nAs it happens, hold_lock_file_for_append() sometimes overwrites errno\nbefore it returns. I will add a patch on top of this series that fixes that.\n\nI don't see any callers that retry, though I've thought about\nimplementing that in some places. But it's outside of the scope of this\npatch series.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249600","messageId":"5419B276.9000808@alum.mit.edu","threadId":"37580","inReplyTo":"20140916205736.GI29050@google.com","subject":"Re: [PATCH v5 08/35] lock_file(): always add lock_file object to lock_file_list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-17T16:10:30Z","receivedAt":"2014-09-17T16:10:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/16/2014 10:57 PM, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n>> The ambiguity didn't have any ill effects, because lock_file objects\n>> cannot be removed from the lock_file_list anyway. But it is\n>> unnecessary to leave this behavior inconsistent.\n> \n> Nit: commit messages usually use present tense for current behavior\n> (and imperative for the behavior after the patch).\n\nOK, will change.\n\n> More importantly, the above + the diffstat don't leave me very happy\n> about the change.\n> \n> Can you spell out more about the intended effect? E.g., is this about\n> making sure other code is appropriately exercised to tolerate the\n> signal handler even when there are a lot of errors (which should make\n> debugging easier) or something?\n\nThis patch is mostly about two things:\n\n* Making the state diagram for lock_file objects easier to document and\nreason about.\n\n* Defining a clear moment when the lock_file object is initialized,\nearly in the function. This will be useful later when its filename field\nis turned into a strbuf and requires nontrivial initialization before it\ncan even be used to construct the filename of the lockfile.\n\nBy \"diffstat\" I assume you mean that some lines were added to the\ninitialization. This is also to make the state diagram of lock_objects\nvery clear.\n\nI will rewrite the commit message to try to explain the commit's purpose\nbetter. Let me know if you still think that the commit is unjustified.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249627","messageId":"541A6047.6000707@web.de","threadId":"37580","inReplyTo":"1410896036-12750-9-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v5 08/35] lock_file(): always add lock_file object to lock_file_list","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-09-18T04:32:07Z","receivedAt":"2014-09-18T04:32:07Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 09/16/2014 09:33 PM, Michael Haggerty wrote:\n[]\n>\n> diff --git a/lockfile.c b/lockfile.c\n> index 983c3ec..00c972c 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -129,6 +129,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> +\tif (!lk->on_list) {\n> +\t\t/* Initialize *lk and add it to lock_file_list: */\n> +\t\tlk->fd = -1;\n> +\t\tlk->owner = 0;\n> +\t\tlk->on_list = 1;\n> +\t\tlk->filename[0] = 0;\nDoes it makes sense to change the order here:\n\nDo the full initialization, and once that is completed, set on_list = 1\n+\t\tlk->filename[0] = 0;\n+\t\tlk->on_list = 1;\n"},{"id":"249629","messageId":"541A8E06.8080609@alum.mit.edu","threadId":"37580","inReplyTo":"541A6047.6000707@web.de","subject":"Re: [PATCH v5 08/35] lock_file(): always add lock_file object to lock_file_list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-18T07:47:18Z","receivedAt":"2014-09-18T07:47:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/18/2014 06:32 AM, Torsten Bögershausen wrote:\n> On 09/16/2014 09:33 PM, Michael Haggerty wrote:\n> []\n>>\n>> diff --git a/lockfile.c b/lockfile.c\n>> index 983c3ec..00c972c 100644\n>> --- a/lockfile.c\n>> +++ b/lockfile.c\n>> @@ -129,6 +129,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>> +\tif (!lk->on_list) {\n>> +\t\t/* Initialize *lk and add it to lock_file_list: */\n>> +\t\tlk->fd = -1;\n>> +\t\tlk->owner = 0;\n>> +\t\tlk->on_list = 1;\n>> +\t\tlk->filename[0] = 0;\n> Does it makes sense to change the order here:\n> \n> Do the full initialization, and once that is completed, set on_list = 1\n> +\t\tlk->filename[0] = 0;\n> +\t\tlk->on_list = 1;\n\nFrom a functional standpoint, it doesn't matter. This function is the\nonly place that uses on_list, and it is basically only used to make sure\nthat each lock_file structure is initialized and registered in\nlock_file_list exactly once. In particular, the signal handling code\ndoesn't care about the on_list field.\n\nSo the only important timing requirement WRT on_list is that it be set\nbefore this function is called again with the same lock_file object. But\nany code that would call this function twice, simultaneously, with the\nsame lock_file argument would be broken far more seriously than could be\nfixed by changing the order that the fields are initialized.\n\nBut I guess you are right that it looks more natural to set this field\nonly after all of the initialization is done. I will make the change.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249735","messageId":"54202E93.4050707@alum.mit.edu","threadId":"37580","inReplyTo":"20140916202525.GB29050@google.com","subject":"Re: [PATCH v5 02/35] api-lockfile: expand the documentation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-22T14:13:39Z","receivedAt":"2014-09-22T14:13:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/16/2014 10:25 PM, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n>> Document a couple more functions and the flags argument as used by\n>> hold_lock_file_for_update() and hold_lock_file_for_append().\n> \n> Thanks.\n> \n> [...]\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> \n> Context: this document has structure\n> \n> \tlockfile API\n> \t============\n> \n> \tExplanation of purpose (nice!).\n> \n> \tThe functions\n> \t-------------\n> \n> \tQuick descriptions of each of the four functions\n> \t`hold_lock_file_for_update`, `commit_lock_file`,\n> \t`rollback_lock_file`, `close_lock_file`.\n> \n> \tReminder about lifetime of the lock_file structure.\n> \n> \tDescription of cleanup convention (thou shalt either\n> \tcommit or roll back; if you forget to, the atexit\n> \thandler will roll back for you).\n> \n> \tLong warning about the harder use cases. The above\n> \t\"thou shalt\" was a lie --- you can also\n> \tclose_lock_file if you know what you're doing\n> \t[jn: why is that function part of the public API?].\n> \n> What's nice about the existing structure is that you can get\n> a sense of how to use the API at a glance. Would there be a\n> way to add this extra information while preserving that property?\n> \n> E.g.:\n> \n> \tlockfile API\n> \t============\n> \n> \tNice brief explanation of purpose (\"is this the API\n> \tI want to use?\"), as before.\n> \n> \tCalling sequence\n> \t----------------\n> \n> \tThe caller:\n> \n> \t* Allocates a variable `struct lock_file lock` in the bss\n> \tsection or heap. Because the `lock_file` structure is used\n> \tin an `atexit(3)` handler, its storage has to stay\n> \tthroughout the life of the program. It cannot be an auto\n> \tvariable allocated on the stack.\n> \n> \t* Attempts to create a lockfile by passing that variable and\n> \tthe filename of the final destination (e.g. `$GIT_DIR/index`)\n> \tto `hold_lock_file_for_update` or `hold_lock_file_for_append`.\n> \t+\n> \tIf the `die_on_error` flag is true, git dies if a lock is\n> \talready taken for the file.\n> \n> \t* Writes new content for the destination file by writing to\n> \t`lock->fd`.\n> \n> \tWhen finished writing, the caller can:\n> \n> \t* Close the file descriptor and rename the lockfile to\n> \tits final destination by calling `commit_lock_file`.\n> \n> \t* Close the file descriptor and remove the lockfile by\n> \tcalling `rollback_lock_file`.\n> \n> \t* Close the file descriptor without removing or renaming\n> \tthe lockfile by calling `close_lock_file`.\n> \n> \tIf you do not call one of `commit_lock_file`,\n> \t`rollback_lock_file`, and `close_lock_file` and instead\n> \tsimply `exit(3)` from the program, an `atexit(3)` handler will\n> \tclose and remove the lockfile.\n> \n> \tYou should never call `close(2)` on `lock->fd` yourself~\n> \tOtherwise the ...\n> \n> \tError handling\n> \t--------------\n> \n> \tFunctions return 0 on success, -1 on failure. errno is?\n> \tisn't? meaningful on error.\n> \n> \t... description of unable_to_lock_error and unable_to_lock_die\n> \there ...\n> \n> \tFlags\n> \t-----\n> \n> \tLOCK_NODEREF::\n> \n> \t\tUsually symbolic links in the destination path are\n> \t\tresolved and the lockfile is created by adding \".lock\"\n> \t\tto the resolved path. If `LOCK_NODEREF` is set, then\n> \t\tthe lockfile is created by adding \".lock\" to the path\n> \t\targument itself.\n> \n> What is the user-visible effect of that flag? When would I want\n> to pass that flag, and when wouldn't I?\n> \n> \tLOCK_DIE_ON_ERROR::\n> \n> \t\tIf a lock is already taken for the file, `die()` with\n> \t\tan error message. If this option is not specified,\n> \t\ttrying to hold a lock file that is already taken will\n> \t\treturn -1 to the caller.\n> \n> Sensible?\n> Jonathan\n\nOK, in the next reroll I will revise the documentation pretty\nthoroughly. Please let me know what you think.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249737","messageId":"54203E4D.7030800@alum.mit.edu","threadId":"37580","inReplyTo":"20140916210317.GJ29050@google.com","subject":"Re: [PATCH v5 09/35] lockfile.c: document the various states of lock_file objects","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-22T15:20:45Z","receivedAt":"2014-09-22T15:20:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/16/2014 11:03 PM, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n>> --- a/lockfile.c\n>> +++ b/lockfile.c\n>> @@ -4,6 +4,63 @@\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> This overlaps a lot with the API doc, which makes me worry a little\n> about it going out of date. Would improving the API doc help, or if\n> aspects are especially relevant here, is there a way to summarize them\n> more quickly to avoid some of the redundancy?\n> \n> [...]\n>> + * A lock_file object can be in several states:\n> \n> Would it make sense for this information to go near the definition of\n> 'struct lock_file'? That's where I'd start if I were looking for\n> information on what states a lock_file can be in.\n\nI agree with your point about overlap. I will split the documentation\ninto two parts with less redundancy:\n\n* Documentation/technical/api-lockfile.txt: How to use the API.\n\n* lockfile.{c,h}: Internal implementation details.\n\nI think the implementation details would get lost among the thousand\nthings in cache.h. So instead, I will add a commit on top of the patch\nseries that splits out a lockfile.h header file (which I think is a good\nidea anyway), and moves the internal documentation there. Sound good?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249736","messageId":"54203F5E.5090805@alum.mit.edu","threadId":"37580","inReplyTo":"20140916210538.GK29050@google.com","subject":"Re: [PATCH v5 10/35] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-22T15:25:18Z","receivedAt":"2014-09-22T15:25:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/16/2014 11:05 PM, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n>> There are a few places that use these values, so define constants for\n>> them.\n> \n> Seems like a symptom of the API leaving out a useful helper (e.g.,\n> something that strips off the lock suffix and returns a memdupz'd\n> filename).\n\nI will add a function get_locked_file_path().\n\n> [...]\n>> --- a/cache.h\n>> +++ b/cache.h\n>> @@ -570,6 +570,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);\n>> #define REFRESH_IN_PORCELAIN\t0x0020\t/* user friendly output, not \"needs update\" */\n>> extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);\n>> \n>> +/* String appended to a filename to derive the lockfile name: */\n>> +#define LOCK_SUFFIX \".lock\"\n>> +#define LOCK_SUFFIX_LEN 5\n> \n> My suspicion is that error handling would be better if fewer callers\n> outside of lockfile.c did the '- LOCK_SUFFIX_LEN' dance, so this seems\n> like a step in the wrong direction.\n> \n> Adding constants in lockfile.c would make sense, though.\n\nI agree that other call sites shouldn't be grubbing around in the\nlock_file data structure. But with the addition of\nget_locked_file_path(), the only remaining user of these constants is in\nrefs.c, in check_refname_component():\n\n\tif (cp - refname >= LOCK_SUFFIX_LEN &&\n\t !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))\n\t\treturn -1; /* Refname ends with \".lock\". */\n\nI think it is forgivable there.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249760","messageId":"20140922223449.GH1175@google.com","threadId":"37580","inReplyTo":"54203E4D.7030800@alum.mit.edu","subject":"Re: [PATCH v5 09/35] lockfile.c: document the various states of lock_file objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-22T22:34:49Z","receivedAt":"2014-09-22T22:34:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> I agree with your point about overlap. I will split the documentation\n> into two parts with less redundancy:\n>\n> * Documentation/technical/api-lockfile.txt: How to use the API.\n>\n> * lockfile.{c,h}: Internal implementation details.\n>\n> I think the implementation details would get lost among the thousand\n> things in cache.h. So instead, I will add a commit on top of the patch\n> series that splits out a lockfile.h header file (which I think is a good\n> idea anyway), and moves the internal documentation there. Sound good?\n\nYep, a separate lockfile.h header file sounds sensible to me.\n\nThanks,\nJonathan\n"},{"id":"249782","messageId":"542159CF.7020105@alum.mit.edu","threadId":"37580","inReplyTo":"20140916221956.GR29050@google.com","subject":"Re: [PATCH v5 18/35] commit_lock_file(): if close fails, roll back","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-23T11:30:23Z","receivedAt":"2014-09-23T11:30:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/17/2014 12:19 AM, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n>> If closing an open lockfile fails, then we cannot be sure of the\n>> contents of the lockfile\n> \n> Is that true? It seems more like a bug in close_lock_file: if it\n> fails, perhaps it should either set lk->fd back to fd or unlink the\n> lockfile itself.\n\n>From close(2) (note especially the last sentence):\n\n> Not checking the return value of close() is a common but nevertheless serious programming\n> error. It is quite possible that errors on a previous write(2) operation are first\n> reported at the final close(). Not checking the return value when closing the file may\n> lead to silent loss of data. This can especially be observed with NFS and with disk\n> quota. Note that the return value should only be used for diagnostics. In particular\n> close() should not be retried after an EINTR since this may cause a reused descriptor from\n> another thread to be closed.\n\n>From that I conclude that if close() fails, pretty much all bets are off\nabout the contents of the lockfile. So we wouldn't want to set lk->fd\nback to fd.\n\nBut finishing the rollback itself might be an alternative...\n\n> What do other callers do on close_lock_file failure?\n\n>From what I can see, the only callers that don't die() immediately are\nthe following (which call close_lock_file() directly or indirectly via\nwrite_locked_index()):\n\ntry_merge_strategy(): returns an error. It looks like this could end up\nreusing the same lock_file object before it had been rolled back ->\nwould be improved if close_lock_file() would rollback on failure.\n\nwrite_cache_as_tree(): does a rollback -> wouldn't mind an automatic\nrollback.\n\nmerge_recursive_generic(): returns an error, and caller exits\nimmediately -> wouldn't mind an automatic rollback.\n\nSo, I will change close_lock_file() to roll back on errors.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249783","messageId":"54216027.90406@alum.mit.edu","threadId":"37580","inReplyTo":"20140916222207.GS29050@google.com","subject":"Re: [PATCH v5 19/35] commit_lock_file(): rollback lock file on failure to rename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-23T11:57:27Z","receivedAt":"2014-09-23T11:57:27Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/17/2014 12:22 AM, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n>> If rename() fails, call rollback_lock_file() to delete the lock file\n>> (in case it is still present) and reset the filename field to the\n>> empty string so that the lockfile object is left in a valid state.\n> \n> Can you spell out more what the goal is? Is the idea to keep the\n> .lock file for a shorter period of time, so other processes can lock\n> that file before the current process exits?\n\nI doubt that there are situations where releasing the .lock file\nimmediately vs. holding it until the end of the process makes any\npractical difference. Most callers die() right away if the commit fails.\n\nThis change is more about cleaning up the API by making the state\ndiagram for lock_file objects well-defined. commit_lock_file() can fail\nin multiple ways that callers cannot easily distinguish. So all of the\nfailure paths have to leave the lock_file object in the same final\nstate, otherwise the callers cannot know what state it is in. Since\nlock_file objects are reusable, this is an API design bug.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249788","messageId":"54216DF9.3020307@alum.mit.edu","threadId":"37580","inReplyTo":"20140916222344.GT29050@google.com","subject":"Re: [PATCH v5 20/35] api-lockfile: document edge cases","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-23T12:56:25Z","receivedAt":"2014-09-23T12:56:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/17/2014 12:23 AM, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n>> * Document the behavior of commit_lock_file() when it fails, namely\n>> that it rolls back the lock_file object and sets errno\n>> appropriately.\n>>\n>> * Document the behavior of rollback_lock_file() when called for a\n>> lock_file object that has already been committed or rolled back,\n>> namely that it is a NOOP.\n> \n> I think this would be easier to read in a separate error handling\n> section. That way, when writing new code using the lockfile API,\n> I can quickly find what functions to use and quickly find out what\n> the error handling conventions are --- each use of the documentation\n> wouldn't interfere with the other.\n\nI added a little blurb in the error handling section explaining how\ncommit_lock_file() and close_lock_file() behave on failure, but I left\nthe detailed information in the sections on those functions because I\ncouldn't find a graceful way to put all of the information in the error\nhandling sections.\n\nIf you have a concrete suggestion, as they say, \"patches are welcome\" :-)\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249789","messageId":"542170DF.8050205@alum.mit.edu","threadId":"37580","inReplyTo":"20140916222814.GV29050@google.com","subject":"Re: [PATCH v5 22/35] git_config_set_multivar_in_file(): avoid call to rollback_lock_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-23T13:08:47Z","receivedAt":"2014-09-23T13:08:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/17/2014 12:28 AM, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n>> After commit_lock_file() is called, then the lock_file object is\n>> necessarily either committed or rolled back. So there is no need to\n>> call rollback_lock_file() again in either of these cases.\n>>\n>> Signed-off-by: Michael Haggerty \n> \n> This seems to involve an unadvertised behavior change: before\n> it wouldn't call git_config_clear() on failure, and after the\n> patch it would. Intended?\n> \n> I think it would be clearer with the goto for exception handling\n> maintained (and just a second 'lock = NULL' assignment).\n\nGood catch; will fix.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249790","messageId":"54217851.6020209@alum.mit.edu","threadId":"37580","inReplyTo":"20140916224523.GW29050@google.com","subject":"Re: [PATCH v5 23/35] lockfile: avoid transitory invalid states","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-23T13:40:33Z","receivedAt":"2014-09-23T13:40:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/17/2014 12:45 AM, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n>> We could probably continue to use the filename field to encode the\n>> state by being careful to write characters 1..N-1 of the filename\n>> first, and then overwrite the NUL at filename[0] with the first\n>> character of the filename, but that would be awkward and error-prone.\n>>\n>> So, instead of using the filename field to determine whether the\n>> lock_file object is active, add a new field \"lock_file::active\" for\n>> this purpose.\n> \n> Nice.\n> \n> [...]\n>> --- a/cache.h\n>> +++ b/cache.h\n>> @@ -576,6 +576,7 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct\n>> \n>> struct lock_file {\n>> \tstruct lock_file *next;\n>> +\tvolatile sig_atomic_t active;\n>> \tint fd;\n>> \tpid_t owner;\n>> \tchar on_list;\n> [...]\n>> +++ b/lockfile.c\n>> @@ -27,16 +27,19 @@\n> [...]\n>> @@ -189,9 +198,14 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n>> \t\tatexit(remove_lock_file);\n>> \t}\n>> \n>> +\tif (lk->active)\n>> +\t\tdie(\"BUG: lock_file(\\\"%s\\\") called with an active lock_file object\",\n>> +\t\t path);\n> \n> The error message doesn't make it entirely obvious to me what went\n> wrong.\n> \n> Maybe something like\n> \n> \t\tdie(\"BUG: cannot lock_file(\\\"%s\\\") on active struct lock_file\",\n> \t\t path);\n\nThis is an internal sanity check that users should never see, and if\nthey do the first thing a developer will do is grep the source code for\nthe error message in the bug report and then he/she will see exactly\nwhat went wrong. So I don't think it is very important that this error\nmessage be super self-explanatory.\n\nBut it doesn't hurt, so I'll make the change you suggest.\n\n> lock_file already assumed on_list was initialized to zero but it\n> wasn't particularly obvious since everything else is blindly\n> scribbled over. Probably worth mentioning in the API docs that\n> the lock_file should be zeroed before calling hold_lock_file_...\n\nGood point. I will document this better.\n\n> [...]\n>> @@ -326,6 +341,7 @@ int commit_lock_file(struct lock_file *lk)\n>> \tif (rename(lk->filename, result_file))\n>> \t\tgoto rollback;\n>> \n>> +\tlk->active = 0;\n>> \tlk->filename[0] = 0;\n> \n> Is it useful to set filename[0] any more?\n> \n> It seems potentially fragile to set both, since new code can appear\n> that only sets or checks one or the other. Would it make sense to\n> rename the filename field to make sure no new callers relying on the\n> filename[0] convention sneak in (with the new convention being that\n> the filename doesn't get cleared, to avoid problems)?\n\nI admit that nobody should be relying on filename being cleared anymore.\nAnd I can see your point that somebody might come to rely on this\nimplementation detail. But I don't like leaving valid filenames around\nwhere a bug might cause them to be accessed accidentally. I would rather\nset the filename to the empty string so that any attempt to use it\ncauses an immediate error message from the OS rather than accessing the\nwrong file.\n\nI will note in the lock_file docstring that client code should not rely\non the filename being empty when in the 'unlocked' state.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"}]}