{"thread":{"id":"37501","subject":"[PATCH v4 01/32] unable_to_lock_die(): rename function from unable_to_lock_index_die()","startedAt":"2014-09-06T07:50:14Z","lastAt":"2014-09-14T14:49:11Z","messageCount":66,"participants":["Michael Haggerty","Torsten Bögershausen","Junio C Hamano","Jeff King","Duy Nguyen","Ronnie Sahlberg","Johannes Sixt"],"isPatch":true,"patchVersion":4,"patchTotal":32},"messages":[{"id":"248955","messageId":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":null,"subject":"[PATCH v4 00/32] Lockfile correctness and refactoring","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:14Z","receivedAt":"2014-09-06T07:50:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Sorry for the long delay since v3. This version mostly cleans up a\ncouple more places where the lockfile object was left in an\nill-defined state. Thanks to Johannes Sixt and Torsten Bögershausen\nfor their review of v3.\n\nI believe that this series addresses all of the comments from v1 [1],\nv2 [2], and v3 [3].\n\nThis series applies to the current \"master\". There is a trivial\nconflict between these changes and \"next\", and a few not-too-serious\nconflicts between these changes and Ronnie's reference-related series\nin \"pu\". I've figured out how to resolve the conflicts locally. Is\nthere some form in which I can put the conflict resolution that would\nhelp you?\n\nChanges since v3:\n\n* Rebase to the current master, including adjusting the patch series\n for 93dcaea2 (addition of reopen_lock_file()).\n\n* Perform the internal consistency check right away in\n commit_lock_file(), rather than after possibly having closed the\n file.\n\n* Improve the explanation of the rationale for marking lock_file\n fields volatile.\n\n* Fix comments that still referred to lock_file::filename[0] even\n though it is now a strbuf.\n\n* Change rollback_lock_file() to exit early if the lock is not active\n (rather than nesting the rest of the function in an \"if\" statement).\n\n* Fix Johannes's email address in the trailers.\n\n* Extract a function commit_lock_file_to(lk, filename) and delegate to\n it from commit_lock_file() and commit_locked_index() so that the\n latter gets the benefit of the improvements in this patch series.\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\nMichael Haggerty (32):\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_elm(): replace last_path_elm()\n Extract a function commit_lock_file_to()\n\n Documentation/technical/api-lockfile.txt | 67 +++++--\n builtin/commit.c | 16 +-\n builtin/merge.c | 15 +-\n builtin/reflog.c | 2 +-\n builtin/update-index.c | 2 +-\n cache.h | 16 +-\n config.c | 28 +--\n fast-import.c | 4 +-\n lockfile.c | 299 ++++++++++++++++++-------------\n read-cache.c | 12 +-\n refs.c | 29 +--\n shallow.c | 6 +-\n 12 files changed, 296 insertions(+), 200 deletions(-)\n\n-- \n2.1.0\n"},{"id":"248942","messageId":"1409989846-22401-2-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 01/32] unable_to_lock_die(): rename function from unable_to_lock_index_die()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:15Z","receivedAt":"2014-09-06T07:50:15Z","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 \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 4d5b76c..da77094 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -581,7 +581,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 27927f2..5ae8e69 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2159,7 +2159,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":"248956","messageId":"1409989846-22401-3-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 02/32] api-lockfile: expand the documentation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:16Z","receivedAt":"2014-09-06T07:50:16Z","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":"248957","messageId":"1409989846-22401-4-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 03/32] rollback_lock_file(): do not clear filename redundantly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:17Z","receivedAt":"2014-09-06T07:50:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is only necessary to clear the lock_file's filename field if it was\nnot already clear.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 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":"248961","messageId":"1409989846-22401-5-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 04/32] rollback_lock_file(): exit early if lock is not active","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:18Z","receivedAt":"2014-09-06T07:50:18Z","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 \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":"248943","messageId":"1409989846-22401-6-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 05/32] rollback_lock_file(): set fd to -1","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:19Z","receivedAt":"2014-09-06T07:50:19Z","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 \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":"248958","messageId":"1409989846-22401-7-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 06/32] lockfile: unlock file if lockfile permissions cannot be adjusted","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:20Z","receivedAt":"2014-09-06T07:50:20Z","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":"248945","messageId":"1409989846-22401-8-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 07/32] hold_lock_file_for_append(): release lock on errors","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:21Z","receivedAt":"2014-09-06T07:50:21Z","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":"248944","messageId":"1409989846-22401-9-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 08/32] lock_file(): always add lock_file object to lock_file_list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:22Z","receivedAt":"2014-09-06T07:50:22Z","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":"248964","messageId":"1409989846-22401-10-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 09/32] lockfile.c: document the various states of lock_file objects","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:23Z","receivedAt":"2014-09-06T07:50:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Document the valid states of lock_file objects, how they get into each\nstate, and how the state is encoded in the object's fields.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 52 insertions(+)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 00c972c..964b3fc 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -4,6 +4,58 @@\n #include \"cache.h\"\n #include \"sigchain.h\"\n \n+/*\n+ * File write-locks as used by Git.\n+ *\n+ * When a file at $FILENAME needs to be written, it is done as\n+ * follows:\n+ *\n+ * 1. Obtain a lock on the file by creating a lockfile at path\n+ * $FILENAME.lock. The file is opened for read/write using O_CREAT\n+ * and O_EXCL mode to ensure that it doesn't already exist. Such a\n+ * lock file is respected by writers *but not by readers*.\n+ *\n+ * Usually, if $FILENAME is a symlink, then it is resolved, and the\n+ * file ultimately pointed to is the one that is locked and later\n+ * replaced. However, if LOCK_NODEREF is used, then $FILENAME\n+ * itself is locked and later replaced, even if it is a symlink.\n+ *\n+ * 2. Write the new file contents to the lockfile.\n+ *\n+ * 3. Move the lockfile to its final destination using rename(2).\n+ *\n+ * Instead of (3), the change can be rolled back by deleting lockfile.\n+ *\n+ * This module keeps track of all locked files in lock_file_list.\n+ * When the first file is locked, it registers an atexit(3) handler;\n+ * when the program exits, the handler rolls back any files that have\n+ * been locked but were never committed or rolled back.\n+ *\n+ * A lock_file object can be in several states:\n+ *\n+ * - Uninitialized. In this state the object's 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":"248960","messageId":"1409989846-22401-11-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:24Z","receivedAt":"2014-09-06T07:50:24Z","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 da77094..41d829b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -569,6 +569,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 964b3fc..bfea333 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -176,10 +176,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@@ -204,7 +205,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@@ -314,7 +315,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 5ae8e69..828522d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -74,7 +74,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@@ -2545,11 +2546,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":"248959","messageId":"1409989846-22401-12-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 11/32] delete_ref_loose(): don't muck around in the lock_file's filename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:25Z","receivedAt":"2014-09-06T07:50:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It's bad manners. Especially since, if unlink_or_warn() failed, the\nmemory wasn't restored to its original contents.\n\nSo make our own copy to work with.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 828522d..8a63073 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2545,12 +2545,15 @@ static int repack_without_ref(const char *refname)\n static int delete_ref_loose(struct ref_lock *lock, int flag)\n {\n \tif (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n-\t\t/* loose */\n-\t\tint err, i = strlen(lock->lk->filename) - 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":"248950","messageId":"1409989846-22401-13-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 12/32] prepare_index(): declare return value to be (const char *)","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:26Z","receivedAt":"2014-09-06T07:50:26Z","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 a3eaf4b..046be7e 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":"248946","messageId":"1409989846-22401-14-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 13/32] write_packed_entry_fn(): convert cb_data into a (const int *)","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:27Z","receivedAt":"2014-09-06T07:50:27Z","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 \n---\n refs.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8a63073..21b0da3 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2218,7 +2218,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":"248966","messageId":"1409989846-22401-15-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 14/32] lock_file(): exit early if lockfile cannot be opened","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:28Z","receivedAt":"2014-09-06T07:50:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is a bit easier to read than the old version, which nested part\nof the non-error code in an \"if\" block.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 23 +++++++++++------------\n 1 file changed, 11 insertions(+), 12 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex bfea333..71786c9 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -207,19 +207,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":"248962","messageId":"1409989846-22401-16-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 15/32] remove_lock_file(): call rollback_lock_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:29Z","receivedAt":"2014-09-06T07:50:29Z","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 71786c9..dacfc28 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -63,12 +63,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":"248970","messageId":"1409989846-22401-17-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 16/32] commit_lock_file(): inline temporary variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:30Z","receivedAt":"2014-09-06T07:50:30Z","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 dacfc28..d64cf6b 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -306,12 +306,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":"248948","messageId":"1409989846-22401-18-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 17/32] commit_lock_file(): die() if called for unlocked lockfile object","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:31Z","receivedAt":"2014-09-06T07:50:31Z","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 d64cf6b..716b674 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -307,6 +307,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":"248963","messageId":"1409989846-22401-19-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 18/32] commit_lock_file(): if close fails, roll back","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:32Z","receivedAt":"2014-09-06T07:50:32Z","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 716b674..213783a 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -310,8 +310,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":"248965","messageId":"1409989846-22401-20-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 19/32] commit_lock_file(): rollback lock file on failure to rename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:33Z","receivedAt":"2014-09-06T07:50:33Z","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 213783a..a7cdb64 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -306,25 +306,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":"248971","messageId":"1409989846-22401-21-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 20/32] api-lockfile: document edge cases","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:34Z","receivedAt":"2014-09-06T07:50:34Z","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":"248968","messageId":"1409989846-22401-22-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 21/32] dump_marks(): remove a redundant call to rollback_lock_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:35Z","receivedAt":"2014-09-06T07:50:35Z","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 d73f58c..55cb47a 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1803,10 +1803,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":"248969","messageId":"1409989846-22401-23-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 22/32] git_config_set_multivar_in_file(): avoid call to rollback_lock_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:36Z","receivedAt":"2014-09-06T07:50:36Z","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 a191328..24f0abb 100644\n--- a/config.c\n+++ b/config.c\n@@ -1964,17 +1964,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":"248973","messageId":"1409989846-22401-24-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 23/32] lockfile: avoid transitory invalid states","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:37Z","receivedAt":"2014-09-06T07:50:37Z","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 | 45 ++++++++++++++++++++++++++++++---------------\n read-cache.c | 1 +\n 3 files changed, 32 insertions(+), 15 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 41d829b..e739482 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -575,6 +575,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 a7cdb64..920585d 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -27,11 +27,14 @@\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 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@@ -39,19 +42,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@@ -184,9 +193,12 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\tatexit(remove_lock_file);\n \t}\n \n+\tassert(!lk->active);\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@@ -208,6 +220,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@@ -297,7 +310,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@@ -308,7 +321,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@@ -321,6 +334,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@@ -341,11 +355,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 5d3c8bd..3d43dda 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":"248949","messageId":"1409989846-22401-25-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 24/32] struct lock_file: declare some fields volatile","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:38Z","receivedAt":"2014-09-06T07:50:38Z","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 e739482..f7ad1b5 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -574,10 +574,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 920585d..f51f73f 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -65,7 +65,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 21b0da3..f076f2d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2260,15 +2260,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":"248974","messageId":"1409989846-22401-26-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 25/32] try_merge_strategy(): remove redundant lock_file allocation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:39Z","receivedAt":"2014-09-06T07:50:39Z","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 ce82eb2..0f5c185 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -669,7 +669,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":"248947","messageId":"1409989846-22401-27-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 26/32] try_merge_strategy(): use a statically-allocated lock_file object","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:40Z","receivedAt":"2014-09-06T07:50:40Z","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 0f5c185..d2415fe 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -657,14 +657,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@@ -696,13 +696,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":"248967","messageId":"1409989846-22401-28-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 27/32] commit_lock_file(): use a strbuf to manage temporary space","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:41Z","receivedAt":"2014-09-06T07:50:41Z","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 f51f73f..f6b3866 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -318,8 +318,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@@ -327,13 +328,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":"248951","messageId":"1409989846-22401-29-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 28/32] Change lock_file::filename into a strbuf","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:42Z","receivedAt":"2014-09-06T07:50:42Z","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 | 47 +++++++++++++++++++----------------------------\n read-cache.c | 4 ++--\n refs.c | 6 +++---\n shallow.c | 6 +++---\n 8 files changed, 42 insertions(+), 51 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 046be7e..b2fc9e8 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,10 +352,10 @@ 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 \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@@ -378,7 +378,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@@ -460,9 +460,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 f7ad1b5..bea6296 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -579,7 +579,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 24f0abb..d9247c6 100644\n--- a/config.c\n+++ b/config.c\n@@ -1905,9 +1905,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@@ -1986,7 +1986,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@@ -2087,9 +2087,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@@ -2110,7 +2110,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@@ -2136,7 +2136,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 f6b3866..c34bf6c 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -56,8 +56,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 the empty string (usually, though there are\n+ * transitory 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@@ -180,13 +180,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@@ -201,29 +194,27 @@ 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}\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@@ -312,7 +303,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@@ -329,14 +320,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@@ -361,7 +352,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 3d43dda..158241d 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 f076f2d..717fc34 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2551,8 +2551,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@@ -2918,7 +2918,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":"248972","messageId":"1409989846-22401-30-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 29/32] resolve_symlink(): use a strbuf for internal scratch space","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:43Z","receivedAt":"2014-09-06T07:50:43Z","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 c34bf6c..9413f7a 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -136,44 +136,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":"248952","messageId":"1409989846-22401-31-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 30/32] resolve_symlink(): take a strbuf parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:44Z","receivedAt":"2014-09-06T07:50:44Z","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\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 53 +++++++++++++++++++----------------------------------\n 1 file changed, 19 insertions(+), 34 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 9413f7a..d78b6bf 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -119,53 +119,40 @@ 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@@ -185,16 +172,14 @@ 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, 0);\n \t\tlk->next = lock_file_list;\n \t\tlock_file_list = lk;\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+\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":"248953","messageId":"1409989846-22401-32-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 31/32] trim_last_path_elm(): replace last_path_elm()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:45Z","receivedAt":"2014-09-06T07:50:45Z","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.\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 d78b6bf..4c6e720 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -86,32 +86,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_elm(struct strbuf *path)\n {\n-\t/* r starts pointing to null at the end of the string */\n-\tchar *r = strchr(p, '\\0');\n-\n-\tif (r == p)\n-\t\treturn p; /* just return empty string */\n-\n-\tr--; /* back up to last non-null character */\n+\tint i = path->len;\n \n \t/* back up past trailing slashes, if any */\n-\twhile (r > p && *r == '/')\n-\t\tr--;\n+\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@@ -141,14 +137,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_elm(path);\n \n \t\tstrbuf_addbuf(path, &link);\n \t}\n-- \n2.1.0\n"},{"id":"248954","messageId":"1409989846-22401-33-git-send-email-mhagger@alum.mit.edu","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v4 32/32] Extract a function commit_lock_file_to()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-06T07:50:46Z","receivedAt":"2014-09-06T07:50:46Z","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 | 30 ++++++++++++++++++------------\n read-cache.c | 13 +++----------\n 4 files changed, 32 insertions(+), 26 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 bea6296..cda91fc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -589,6 +589,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 4c6e720..e54d260 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -53,8 +53,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 the empty string (usually, though there are\n * transitory states in which this condition doesn't hold)\n@@ -277,24 +278,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 \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@@ -307,6 +300,19 @@ 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+\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 158241d..11b80a0 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":"249011","messageId":"540C6A02.9070403@web.de","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v4 00/32] Lockfile correctness and refactoring","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-09-07T14:21:54Z","receivedAt":"2014-09-07T14:21:54Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\nOn 2014-09-06 09.50, Michael Haggerty wrote:\n> Sorry for the long delay since v3. This version mostly cleans up a\n> couple more places where the lockfile object was left in an\n> ill-defined state. \nNo problem with the delay.\nThe most important question is if we do the lk->active handling right.\nSet it to false as seen as possible, and to true as late as possible,\nthen die() cleanly.\nSo the ->acive handling looks (more or less, please see below) and\ndeserves another critical review, may be.\n\nInstead of commenting each patch, I collected a mixture of small questions\nand possible suggestions into a diff file.\n\ndiff --git a/lockfile.c b/lockfile.c\nindex e54d260..7f27ea2 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -18,6 +18,10 @@\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+LOCK_NODEREF can be read either as\n+LOCK_NODE_REF or LOCK_NO_DEREF\n+should we change it ?\n+\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@@ -46,9 +50,18 @@\n * state:\n * - the lockfile exists\n * - active is set\n- * - filename holds the filename of the lockfile\n+ * - filename holds the filename of the lockfile in a strbuf\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+question: Why do we need the PID here ?\n+Do we open a lock file and do a fork() ?\n+And if yes, the child gets a new PID, what happens when the\n+child gets a signal ?\n+Who \"owns\" the lockfile, the parent, the child, both ?\n+The child has access to all data, the fd is open and can be used,\n+why do we not allow a rollback, when the child dies ?\n+\n+\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@@ -57,7 +70,7 @@\n * rollback_lock_file(), or a failed attempt to lock). In this\n * state:\n * - active is unset\n- * - filename is the empty string (usually, though there are\n+ * - filename is an empty string buffer (usually, though there are\n * transitory 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@@ -68,7 +81,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 pid_t me = getpid();\n \n@@ -79,9 +92,9 @@ static void remove_lock_file(void)\n }\n }\n \n-static void remove_lock_file_on_signal(int signo)\n+static void remove_lock_files_on_signal(int signo)\n {\n- remove_lock_file();\n+ remove_lock_files();\n sigchain_pop(signo);\n raise(signo);\n }\n@@ -93,7 +106,7 @@ static void remove_lock_file_on_signal(int signo)\n * \"/\", if any). If path is empty or the root directory (\"/\"), set\n * path to the empty string.\n */\n-static void trim_last_path_elm(struct strbuf *path)\n+static void trim_last_path_elem(struct strbuf *path)\n {\n int i = path->len;\n \n@@ -143,7 +156,7 @@ static void resolve_symlink(struct strbuf *path)\n * link is a relative path, so replace the\n * last element of p with it.\n */\n- trim_last_path_elm(path);\n+ trim_last_path_elem(path);\n \n strbuf_addbuf(path, &link);\n }\n@@ -153,13 +166,16 @@ static void resolve_symlink(struct strbuf *path)\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+ struct stat st;\n+ int mode = 0666;\n if (!lock_file_list) {\n /* One-time initialization */\n- sigchain_push_common(remove_lock_file_on_signal);\n- atexit(remove_lock_file);\n+ sigchain_push_common(remove_lock_files_on_signal);\n+ atexit(remove_lock_files);\n }\n \n- assert(!lk->active);\n+ if (lk->active)\n+ die(\"lk->active %s\", path);\n \n if (!lk->on_list) {\n /* Initialize *lk and add it to lock_file_list: */\n@@ -167,16 +183,25 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n lk->active = 0;\n lk->owner = 0;\n lk->on_list = 1;\n- strbuf_init(&lk->filename, 0);\n+ strbuf_init(&lk->filename, strlen(path) + LOCK_SUFFIX_LEN);\n lk->next = lock_file_list;\n lock_file_list = lk;\n }\n \n+ strbuf_reset(&lk->filename); /* Better to be save */\n strbuf_addstr(&lk->filename, path);\n if (!(flags & LOCK_NODEREF))\n resolve_symlink(&lk->filename);\n strbuf_addstr(&lk->filename, LOCK_SUFFIX);\n- lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n+ /*\n+ * adjust_shared_perm() will widen permissions if needed,\n+ * otherwise keep permissions restrictive\n+ *\n+ */\n+ if (!stat(path, &st))\n+ mode = st.st_mode & 07777;\n+\n+ lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, mode);\n if (lk->fd < 0) {\n strbuf_reset(&lk->filename);\n return -1;\n@@ -268,7 +293,7 @@ int close_lock_file(struct lock_file *lk)\n return close(fd);\n }\n \n-int reopen_lock_file(struct lock_file *lk)\n+int reopen_lock_file_UNUSED_CAN_IT_BE_REMOVED(struct lock_file *lk)\n {\n if (0 <= lk->fd)\n die(_(\"BUG: reopen a lockfile that is still open\"));\n@@ -283,7 +308,7 @@ int commit_lock_file_to(struct lock_file *lk, const char *path)\n int save_errno;\n \n if (!lk->active)\n- die(\"BUG: attempt to commit unlocked object\");\n+ die(\"BUG: attempt to commit unlocked object %s\", path);\n \n if (lk->fd >= 0 && close_lock_file(lk))\n goto rollback;\n@@ -325,10 +350,12 @@ void rollback_lock_file(struct lock_file *lk)\n {\n if (!lk->active)\n return;\n+ lk->active = 0; /* We are going to de-activate,\n+ so active is no longer valid already here ? */\n \n if (lk->fd >= 0)\n close_lock_file(lk);\n unlink_or_warn(lk->filename.buf);\n- lk->active = 0;\n+ //lk->active = 0;\n strbuf_reset(&lk->filename);\n }\n"},{"id":"249067","messageId":"xmqq38c1n3ki.fsf@gitster.dls.corp.google.com","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v4 00/32] Lockfile correctness and refactoring","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-08T22:35:25Z","receivedAt":"2014-09-08T22:35:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> This series applies to the current \"master\". There is a trivial\n> conflict between these changes and \"next\", and a few not-too-serious\n> conflicts between these changes and Ronnie's reference-related series\n> in \"pu\".\n\nThe conflicts weren't very pretty as rs/transaction* series\nprogressed, but I think I got something readable queued at the tip\nof 'jch' (which is what I usually run myself and is at somewhere\nbetween 'pu^{/match next}' and 'pu').\n\nI'd appreciate if both of you can double check the result.\n\n> I've figured out how to resolve the conflicts locally. Is\n> there some form in which I can put the conflict resolution that would\n> help you?\n\nA public tree that shows a suggested conflict resolution, so that I\ncan try it myself without looking at it first and then compare my\nresult with yours, would probably be the easiest for both of us. In\nthe worst case, I could fetch from such a tree, use rerere-train in\ncontrib/ and figure out any necessary evil-merges that are not\ncovered by rerere.\n\nThanks.\n"},{"id":"249104","messageId":"xmqqr3zkifl5.fsf@gitster.dls.corp.google.com","threadId":"37501","inReplyTo":"1409989846-22401-7-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v4 06/32] lockfile: unlock file if lockfile permissions cannot be adjusted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-09T22:39:18Z","receivedAt":"2014-09-09T22:39:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> If the call to adjust_shared_perm() fails, lock_file returns -1, which\n> to the caller looks like any other failure to lock the file. So in\n> this case, roll back the lockfile before returning so that the lock\n> file is deleted immediately and the lockfile object is left in a\n> predictable state (namely, unlocked). Previously, the lockfile was\n> retained until process cleanup in this situation.\n\n... which would mean that other processes can grab a lock on the\nsame file a bit earlier. Is there any negative implication caused by\nthat difference? I do not think of any but I could be missing\nsomething.\n\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> lockfile.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/lockfile.c b/lockfile.c\n> index 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"},{"id":"249105","messageId":"xmqqmwa8ifja.fsf@gitster.dls.corp.google.com","threadId":"37501","inReplyTo":"1409989846-22401-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v4 02/32] api-lockfile: expand the documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-09T22:40:25Z","receivedAt":"2014-09-09T22:40:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> +LOCK_NODEREF::\n\nI think I've seen this mentioned already but LOCK_NO_DEREF to avoid\n\"lock the node ref?\" misreading?\n"},{"id":"249106","messageId":"xmqqiokwifi9.fsf@gitster.dls.corp.google.com","threadId":"37501","inReplyTo":"1409989846-22401-8-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v4 07/32] hold_lock_file_for_append(): release lock on errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-09T22:41:02Z","receivedAt":"2014-09-09T22:41:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> If there is an error copying the old contents to the lockfile, roll\n> back the lockfile before exiting so that the lockfile is not held\n> until process cleanup.\n\nSame comment as 06/32 applies here.\n"},{"id":"249111","messageId":"20140910075519.GA16413@peff.net","threadId":"37501","inReplyTo":"1409989846-22401-20-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v4 19/32] commit_lock_file(): rollback lock file on failure to rename","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-10T07:55:19Z","receivedAt":"2014-09-10T07:55:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 06, 2014 at 09:50:33AM +0200, 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\nUnlike the previous patch, in this case the contents of the lockfile\n_are_ defined. So in theory a caller could somehow retry.\n\nI don't see any callers that want to do that, though (and besides, they\nwould not know if the error came from the close or the rename), so I\nthink we can consider that an uninteresting case until somebody\ncreates such a caller (at which point they can take responsibility for\nextending the API).\n\nBTW, while grepping for commit_lock_file calls, I notice we often commit\nthe shallow file without checking the return code. I'm not sure what we\nshould do in each case, but I imagine that calling die() is probably\nbetter than continuing as if it succeeded. +cc Duy\n\n-Peff\n"},{"id":"249112","messageId":"20140910081358.GB16413@peff.net","threadId":"37501","inReplyTo":"1409989846-22401-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v4 00/32] Lockfile correctness and refactoring","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-10T08:13:59Z","receivedAt":"2014-09-10T08:13:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 06, 2014 at 09:50:14AM +0200, Michael Haggerty wrote:\n\n> Sorry for the long delay since v3. This version mostly cleans up a\n> couple more places where the lockfile object was left in an\n> ill-defined state. Thanks to Johannes Sixt and Torsten Bögershausen\n> for their review of v3.\n> \n> I believe that this series addresses all of the comments from v1 [1],\n> v2 [2], and v3 [3].\n\nThis looks pretty good to me overall.\n\nI did coincidentally have an interesting experience with our lockfile\ncode earlier today, which I'd like to relate.\n\nI was running pack-refs on a repository with a very large number of\nloose refs (about 1.8 million). Needless to say, this ran very slowly\nand thrashed the disk, as that's almost 7G using 4K inodes. But it did\neventually generate a packed-refs file, at which point it tried to prune\nthe loose refs.\n\nTo do so, we have to lock each ref before removing it (to protect\nagainst a simultaneous update). Each call to lock_ref_sha1_basic\nallocates a \"struct lock_file\", which then gets added to the global\nlock_file list. Each one contains a fixed PATH_MAX buffer (4K on this\nmachine). After we're done updating the ref, we leak the lock_file\nstruct, since there's no way to remove it from the list.\n\nAs a result, git tried to allocate 7G of RAM and got OOM-killed (the\nmachine had only 8G). In addition to thrashing the disk even harder,\nsince there was no room left for disk cache while we touched millions of\nloose refs. :)\n\nYour change in this series to use a strbuf would make this a lot better.\nBut I still wonder how hard it would be to just remove lock_file structs\nfrom the global list when they are committed or rolled back. That would\npresumably also make the \"avoid transitory valid states\" patch from your\nseries a bit easier, too (you could prepare the lockfile in peace, and\nthen link it in fully formed, and do the opposite when removing it).\n\nI think with your strbuf patch, this leak at least becomes reasonable.\nSo maybe it's not worth going further. But I'd be interested to hear\nyour thoughts since you've been touching the area recently.\n\n-Peff\n"},{"id":"249115","messageId":"CACsJy8AO5Ni1BNJGa1Wy5ZdX=HNE1UuVM-gnwtHBw01sfcZNrg@mail.gmail.com","threadId":"37501","inReplyTo":"20140910081358.GB16413@peff.net","subject":"Re: [PATCH v4 00/32] Lockfile correctness and refactoring","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-09-10T10:25:36Z","receivedAt":"2014-09-10T10:25:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Sep 10, 2014 at 3:13 PM, Jeff King wrote:\n> I was running pack-refs on a repository with a very large number of\n> loose refs (about 1.8 million). Needless to say, this ran very slowly\n> and thrashed the disk, as that's almost 7G using 4K inodes. But it did\n> eventually generate a packed-refs file, at which point it tried to prune\n> the loose refs.\n\nUrghh.. ref advertisment for fetch/push would be unbelievably large.\nGotta look at the \"git protocol v2\" again soon..\n-- \nDuy\n"},{"id":"249116","messageId":"20140910103014.GA15461@peff.net","threadId":"37501","inReplyTo":"CACsJy8AO5Ni1BNJGa1Wy5ZdX=HNE1UuVM-gnwtHBw01sfcZNrg@mail.gmail.com","subject":"Re: [PATCH v4 00/32] Lockfile correctness and refactoring","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-10T10:30:14Z","receivedAt":"2014-09-10T10:30:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 10, 2014 at 05:25:36PM +0700, Duy Nguyen wrote:\n\n> On Wed, Sep 10, 2014 at 3:13 PM, Jeff King wrote:\n> > I was running pack-refs on a repository with a very large number of\n> > loose refs (about 1.8 million). Needless to say, this ran very slowly\n> > and thrashed the disk, as that's almost 7G using 4K inodes. But it did\n> > eventually generate a packed-refs file, at which point it tried to prune\n> > the loose refs.\n> \n> Urghh.. ref advertisment for fetch/push would be unbelievably large.\n> Gotta look at the \"git protocol v2\" again soon..\n\nYes, we don't let normal fetchers see these repos. They're only for\nholding shared objects and the ref tips to keep them reachable. So we\nnever fetch from them, even locally. We only fetch to them from normal\nrepos (and never push to or from them at all).\n\nIt's still rather painful just to do normal things, though. Every git\noperation loads the whole packed-refs file into memory. I'm biding my\ntime on the ref-backend patches that are being worked on. :)\n\n-Peff\n"},{"id":"249120","messageId":"CACsJy8AOvyt3FxcnZKa+x77WJ90Zn7DhiBUpbQ8DhU-D3SLfyA@mail.gmail.com","threadId":"37501","inReplyTo":"20140910075519.GA16413@peff.net","subject":"Re: [PATCH v4 19/32] commit_lock_file(): rollback lock file on failure to rename","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-09-10T12:55:07Z","receivedAt":"2014-09-10T12:55:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Sep 10, 2014 at 2:55 PM, Jeff King wrote:\n> BTW, while grepping for commit_lock_file calls, I notice we often commit\n> the shallow file without checking the return code. I'm not sure what we\n> should do in each case, but I imagine that calling die() is probably\n> better than continuing as if it succeeded. +cc Duy\n\nNoted. To be fixed soon. It looks like we could make gcc warn about\nignoring return code like this with\n__attribute__((warn_unused_result)) (but I haven't tested). If it does\nhelp catch mishandling in future commit_lock_file calls and does not\nupset other compilers, we may want to add it to commit_lock_file.\n-- \nDuy\n"},{"id":"249136","messageId":"xmqq7g1bifm0.fsf@gitster.dls.corp.google.com","threadId":"37501","inReplyTo":"20140910103014.GA15461@peff.net","subject":"Re: [PATCH v4 00/32] Lockfile correctness and refactoring","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-10T16:51:03Z","receivedAt":"2014-09-10T16:51:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yes, we don't let normal fetchers see these repos. They're only for\n> holding shared objects and the ref tips to keep them reachable.\n\nAre these individual refs have relations to the real world after\nthey are created? To ask it another way, let's say that a branch in\na repository, which is using this as a shared object store, caused\none of these refs to be created; now the origin repository rewinds\nor deletes that branch---do you do anything to the ref in the shared\nobject store at that point?\n\nI am wondering if it makes sense to maintain a single ref that\nreaches all the commits in this shared object store repository,\ninstead of keeping these millions of refs. When you need to make\nmore objects kept and reachable, create an octopus with the current\ntip and tips of all these refs that causes you to wish making these\n\"more objects kept and reachable\". Obviously that won't work well\nif the reason why your current scheme uses refs is because you\nadjust individual refs to prune some objects---hence the first\nquestion in this message.\n"},{"id":"249144","messageId":"20140910191159.GA23655@peff.net","threadId":"37501","inReplyTo":"xmqq7g1bifm0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 00/32] Lockfile correctness and refactoring","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-10T19:11:59Z","receivedAt":"2014-09-10T19:11:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 10, 2014 at 09:51:03AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Yes, we don't let normal fetchers see these repos. They're only for\n> > holding shared objects and the ref tips to keep them reachable.\n> \n> Are these individual refs have relations to the real world after\n> they are created? To ask it another way, let's say that a branch in\n> a repository, which is using this as a shared object store, caused\n> one of these refs to be created; now the origin repository rewinds\n> or deletes that branch---do you do anything to the ref in the shared\n> object store at that point?\n\nYes, we fetch from them before doing any maintenance in the shared\nrepository (like running repack). That's how objects migrate into the\nshared repository, as well.\n\n> I am wondering if it makes sense to maintain a single ref that\n> reaches all the commits in this shared object store repository,\n> instead of keeping these millions of refs. When you need to make\n> more objects kept and reachable, create an octopus with the current\n> tip and tips of all these refs that causes you to wish making these\n> \"more objects kept and reachable\". Obviously that won't work well\n> if the reason why your current scheme uses refs is because you\n> adjust individual refs to prune some objects---hence the first\n> question in this message.\n\nExactly. You could do this if you threw away and re-made the octopus\nafter each fetch (and then threw away the individual branches that went\ninto it). For that matter, if all you really want are the tips for\nreachability, you can basically run \"for-each-ref | sort -u\"; most of\nthese refs are tags that are duplicated between each fork.\n\nHowever, having the individual tips does make some things easier. If I\nonly keep unique tips and I drop a tip from fork A, I would then need to\ncheck every other fork to see if any other fork has the same tip. OTOH,\nthat means visiting N packed-refs files, each with (let's say) 3000\nrefs. As opposed to dealing with a packed-refs file with N*3000 refs. So\nit's really not that different.\n\nWe also use the individual ref tips for packing. They factor into the\nbitmap selection, and we have some patches (which I've been meaning to\nupstream for a while now) to make delta selections in the shared-object\nrepository that will have a high chance of reuse in clones of individual\nforks. And it's useful to query them for various reasons (e.g., \"who is\nreferencing this object?\").\n\nThere a lot of different ways to do it, and the giant refs file is a\npain (not to mention writing objects to disk in the forks, and then\nmigrating them separately to the shared storage). But doing it this way\nmeans that the forks and the shared-object repository are all real\nfirst-class git repositories. We follow the usual object reachability\nguarantees, and it's safe to run any stock git command on them.\n\nI am leaning towards a system where the shared-object repository is a\npseudo-repository, forks actually write directly into the shared object\nstore (probably with a symlinked objects/ directory), and implementing a\nref backend that generates a virtual mapping on the fly (e.g., all refs in\n\"../foo.git\" appear as \"refs/remotes/foo/*\" in the pseudo-repository.\nI'm watching the refs-backend work anxiously. :)\n\n-Peff\n"},{"id":"249246","messageId":"CAL=YDWn_PAbO5T+WFPTcFSVCvNp5EUd6egv76RQmheFYiMpPPg@mail.gmail.com","threadId":"37501","inReplyTo":"1409989846-22401-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v4 04/32] rollback_lock_file(): exit early if lock is not active","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-09-11T19:13:16Z","receivedAt":"2014-09-11T19:13:16Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Reviewed-by: Ronnie Sahlberg \n\nOn Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty wrote:\n> Eliminate a layer of nesting.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> lockfile.c | 13 +++++++------\n> 1 file changed, 7 insertions(+), 6 deletions(-)\n>\n> diff --git a/lockfile.c b/lockfile.c\n> index 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> - if (lk->filename[0]) {\n> - if (lk->fd >= 0)\n> - close(lk->fd);\n> - unlink_or_warn(lk->filename);\n> - lk->filename[0] = 0;\n> - }\n> + if (!lk->filename[0])\n> + return;\n> +\n> + if (lk->fd >= 0)\n> + close(lk->fd);\n> + unlink_or_warn(lk->filename);\n> + lk->filename[0] = 0;\n> }\n> --\n> 2.1.0\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"249247","messageId":"CAL=YDW=pNyzdNCcRV89qU7vBy94jVymGmgAUufTXWiMjdnt2hw@mail.gmail.com","threadId":"37501","inReplyTo":"1409989846-22401-4-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v4 03/32] rollback_lock_file(): do not clear filename redundantly","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-09-11T19:13:58Z","receivedAt":"2014-09-11T19:13:58Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Reviewed-by: Ronnie Sahlberg \n\nOn Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty wrote:\n> It is only necessary to clear the lock_file's filename field if it was\n> not already clear.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> lockfile.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/lockfile.c b/lockfile.c\n> index 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> if (lk->fd >= 0)\n> close(lk->fd);\n> unlink_or_warn(lk->filename);\n> + lk->filename[0] = 0;\n> }\n> - lk->filename[0] = 0;\n> }\n> --\n> 2.1.0\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"249253","messageId":"CAL=YDWnHKUTiKnW5tVfdppw0g6HpXHPbL-b_EViRZHheQO3Uqg@mail.gmail.com","threadId":"37501","inReplyTo":"1409989846-22401-14-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v4 13/32] write_packed_entry_fn(): convert cb_data into a (const int *)","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-09-11T19:55:15Z","receivedAt":"2014-09-11T19:55:15Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Reviewed-by: Ronnie Sahlberg \n\nOn Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty wrote:\n> This makes it obvious that we have no plans to change the integer\n> pointed to, which is actually the fd field from a struct lock_file.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> refs.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 8a63073..21b0da3 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2218,7 +2218,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> - int *fd = cb_data;\n> + const int *fd = cb_data;\n> enum peel_status peel_status = peel_entry(entry, 0);\n>\n> if (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)\n> --\n> 2.1.0\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"249254","messageId":"CAL=YDWmD71jJuZELDE-fqZ8x=p3Usp3_MU2uzyJ59HBvXtbg0g@mail.gmail.com","threadId":"37501","inReplyTo":"1409989846-22401-10-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v4 09/32] lockfile.c: document the various states of lock_file objects","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-09-11T19:57:51Z","receivedAt":"2014-09-11T19:57:51Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Reviewed-by: Ronnie Sahlberg \n\nOn Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty wrote:\n> Document the valid states of lock_file objects, how they get into each\n> state, and how the state is encoded in the object's fields.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> lockfile.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 52 insertions(+)\n>\n> diff --git a/lockfile.c b/lockfile.c\n> index 00c972c..964b3fc 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -4,6 +4,58 @@\n> #include \"cache.h\"\n> #include \"sigchain.h\"\n>\n> +/*\n> + * File write-locks as used by Git.\n> + *\n> + * When a file at $FILENAME needs to be written, it is done as\n> + * follows:\n> + *\n> + * 1. Obtain a lock on the file by creating a lockfile at path\n> + * $FILENAME.lock. The file is opened for read/write using O_CREAT\n> + * and O_EXCL mode to ensure that it doesn't already exist. Such a\n> + * lock file is respected by writers *but not by readers*.\n> + *\n> + * Usually, if $FILENAME is a symlink, then it is resolved, and the\n> + * file ultimately pointed to is the one that is locked and later\n> + * replaced. However, if LOCK_NODEREF is used, then $FILENAME\n> + * itself is locked and later replaced, even if it is a symlink.\n> + *\n> + * 2. Write the new file contents to the lockfile.\n> + *\n> + * 3. Move the lockfile to its final destination using rename(2).\n> + *\n> + * Instead of (3), the change can be rolled back by deleting lockfile.\n> + *\n> + * This module keeps track of all locked files in lock_file_list.\n> + * When the first file is locked, it registers an atexit(3) handler;\n> + * when the program exits, the handler rolls back any files that have\n> + * been locked but were never committed or rolled back.\n> + *\n> + * A lock_file object can be in several states:\n> + *\n> + * - Uninitialized. In this state the object's 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> --\n> 2.1.0\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"249265","messageId":"CAL=YDWmhSuk9rqBKRbJLfNSn8QTtebbBMwSJqqAYOKS=ZZ1=qg@mail.gmail.com","threadId":"37501","inReplyTo":"1409989846-22401-11-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-09-11T22:15:40Z","receivedAt":"2014-09-11T22:15:40Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty wrote:\n> There are a few places that use these values, so define constants for\n> them.\n>\n> Signed-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>\n> diff --git a/cache.h b/cache.h\n> index da77094..41d829b 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -569,6 +569,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);\n> #define REFRESH_IN_PORCELAIN 0x0020 /* 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> struct lock_file *next;\n> int fd;\n> diff --git a/lockfile.c b/lockfile.c\n> index 964b3fc..bfea333 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -176,10 +176,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> /*\n> - * subtract 5 from size to make sure there's room for adding\n> - * \".lock\" for the lock file name\n> + * subtract LOCK_SUFFIX_LEN from size to make sure there's\n> + * room for adding \".lock\" for the lock file name:\n> */\n> - static const size_t max_path_len = sizeof(lk->filename) - 5;\n> + static const size_t max_path_len = sizeof(lk->filename) -\n> + LOCK_SUFFIX_LEN;\n>\n> if (!lock_file_list) {\n> /* One-time initialization */\n> @@ -204,7 +205,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n> strcpy(lk->filename, path);\n> if (!(flags & LOCK_NODEREF))\n> resolve_symlink(lk->filename, max_path_len);\n> - strcat(lk->filename, \".lock\");\n> + strcat(lk->filename, LOCK_SUFFIX);\n> lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n> if (0 <= lk->fd) {\n> lk->owner = getpid();\n> @@ -314,7 +315,7 @@ int commit_lock_file(struct lock_file *lk)\n> if (lk->fd >= 0 && close_lock_file(lk))\n> return -1;\n> strcpy(result_file, lk->filename);\n> - i = strlen(result_file) - 5; /* .lock */\n> + i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */\n\nNot a new bug since the previous code is broken too.\nShould probably checkstrlen(result_file) >= 5 here before subtracting 5.\n\nOtherwise, a caller that calls commit_lock_file() with an already\ncommitted/closed lock_file can cause writing outside the bounds of\nthe array on the line below.\n\n\n> result_file[i] = 0;\n> if (rename(lk->filename, result_file))\n> return -1;\n> diff --git a/refs.c b/refs.c\n> index 5ae8e69..828522d 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -74,7 +74,8 @@ out:\n> if (refname[1] == '\\0')\n> return -1; /* Component equals \".\". */\n> }\n> - if (cp - refname >= 5 && !memcmp(cp - 5, \".lock\", 5))\n> + if (cp - refname >= LOCK_SUFFIX_LEN &&\n> + !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))\n> return -1; /* Refname ends with \".lock\". */\n> return cp - refname;\n> }\n> @@ -2545,11 +2546,11 @@ static int delete_ref_loose(struct ref_lock *lock, int flag)\n> {\n> if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n> /* loose */\n> - int err, i = strlen(lock->lk->filename) - 5; /* .lock */\n> + int err, i = strlen(lock->lk->filename) - LOCK_SUFFIX_LEN;\n>\n> lock->lk->filename[i] = 0;\n> err = unlink_or_warn(lock->lk->filename);\n> - lock->lk->filename[i] = '.';\n> + lock->lk->filename[i] = LOCK_SUFFIX[0];\n> if (err && errno != ENOENT)\n> return 1;\n> }\n> --\n> 2.1.0\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"249268","messageId":"CAL=YDWmgwwQsCSpVsQ5U+c=4RcLdzo-j_hmxDk-AJWdRUk_SsQ@mail.gmail.com","threadId":"37501","inReplyTo":"1409989846-22401-11-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-09-11T22:42:59Z","receivedAt":"2014-09-11T22:42:59Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Maybe we should not have a public constant defined for the length :\n+#define LOCK_SUFFIX_LEN 5\n\nsince it encourages unsafe code like : (this was unsafe long before\nyour patch so not a regression)\n+ i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */\n result_file[i] = 0;\n\n\n\nWhat about removing LOCK_SUFFIX_LEN from the public API and introduce\na helper function something like :\n\n\n/* pointer to the character where the lock suffix starts */\nchar *lock_suffix_ptr_safe(const char *filename)\n{\n size_t len = strlen(filename);\n if (len < 5)\n die(\"BUG:...\n if (strcmp(filename + len - 5, LOCK_SUFFIX)\n die(\"BUG:...\n return filename + len - 5;\n}\n\nand use it instead?\n\n\nOn Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty wrote:\n> There are a few places that use these values, so define constants for\n> them.\n>\n> Signed-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>\n> diff --git a/cache.h b/cache.h\n> index da77094..41d829b 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -569,6 +569,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);\n> #define REFRESH_IN_PORCELAIN 0x0020 /* 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> struct lock_file *next;\n> int fd;\n> diff --git a/lockfile.c b/lockfile.c\n> index 964b3fc..bfea333 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -176,10 +176,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> /*\n> - * subtract 5 from size to make sure there's room for adding\n> - * \".lock\" for the lock file name\n> + * subtract LOCK_SUFFIX_LEN from size to make sure there's\n> + * room for adding \".lock\" for the lock file name:\n> */\n> - static const size_t max_path_len = sizeof(lk->filename) - 5;\n> + static const size_t max_path_len = sizeof(lk->filename) -\n> + LOCK_SUFFIX_LEN;\n>\n> if (!lock_file_list) {\n> /* One-time initialization */\n> @@ -204,7 +205,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n> strcpy(lk->filename, path);\n> if (!(flags & LOCK_NODEREF))\n> resolve_symlink(lk->filename, max_path_len);\n> - strcat(lk->filename, \".lock\");\n> + strcat(lk->filename, LOCK_SUFFIX);\n> lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n> if (0 <= lk->fd) {\n> lk->owner = getpid();\n> @@ -314,7 +315,7 @@ int commit_lock_file(struct lock_file *lk)\n> if (lk->fd >= 0 && close_lock_file(lk))\n> return -1;\n> strcpy(result_file, lk->filename);\n> - i = strlen(result_file) - 5; /* .lock */\n> + i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */\n> result_file[i] = 0;\n> if (rename(lk->filename, result_file))\n> return -1;\n> diff --git a/refs.c b/refs.c\n> index 5ae8e69..828522d 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -74,7 +74,8 @@ out:\n> if (refname[1] == '\\0')\n> return -1; /* Component equals \".\". */\n> }\n> - if (cp - refname >= 5 && !memcmp(cp - 5, \".lock\", 5))\n> + if (cp - refname >= LOCK_SUFFIX_LEN &&\n> + !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))\n> return -1; /* Refname ends with \".lock\". */\n> return cp - refname;\n> }\n> @@ -2545,11 +2546,11 @@ static int delete_ref_loose(struct ref_lock *lock, int flag)\n> {\n> if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n> /* loose */\n> - int err, i = strlen(lock->lk->filename) - 5; /* .lock */\n> + int err, i = strlen(lock->lk->filename) - LOCK_SUFFIX_LEN;\n>\n> lock->lk->filename[i] = 0;\n> err = unlink_or_warn(lock->lk->filename);\n> - lock->lk->filename[i] = '.';\n> + lock->lk->filename[i] = LOCK_SUFFIX[0];\n> if (err && errno != ENOENT)\n> return 1;\n> }\n> --\n> 2.1.0\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"249269","messageId":"CAL=YDWnoN_kJPQHDwTY_95bSU4gyMOX9Jf2qio8bSfT38d673g@mail.gmail.com","threadId":"37501","inReplyTo":"1409989846-22401-15-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v4 14/32] lock_file(): exit early if lockfile cannot be opened","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-09-11T22:49:16Z","receivedAt":"2014-09-11T22:49:16Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Reviewed-by: Ronnie Sahlberg \n\n\nOn Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty wrote:\n> This is a bit easier to read than the old version, which nested part\n> of the non-error code in an \"if\" block.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> lockfile.c | 23 +++++++++++------------\n> 1 file changed, 11 insertions(+), 12 deletions(-)\n>\n> diff --git a/lockfile.c b/lockfile.c\n> index bfea333..71786c9 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -207,19 +207,18 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n> resolve_symlink(lk->filename, max_path_len);\n> strcat(lk->filename, LOCK_SUFFIX);\n> lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n> - if (0 <= lk->fd) {\n> - lk->owner = getpid();\n> - if (adjust_shared_perm(lk->filename)) {\n> - int save_errno = errno;\n> - error(\"cannot fix permission bits on %s\",\n> - lk->filename);\n> - rollback_lock_file(lk);\n> - errno = save_errno;\n> - return -1;\n> - }\n> - }\n> - else\n> + if (lk->fd < 0) {\n> lk->filename[0] = 0;\n> + return -1;\n> + }\n> + lk->owner = getpid();\n> + if (adjust_shared_perm(lk->filename)) {\n> + int save_errno = errno;\n> + error(\"cannot fix permission bits on %s\", lk->filename);\n> + rollback_lock_file(lk);\n> + errno = save_errno;\n> + return -1;\n> + }\n> return lk->fd;\n> }\n>\n> --\n> 2.1.0\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"249296","messageId":"5412D2F3.50404@alum.mit.edu","threadId":"37501","inReplyTo":"xmqqr3zkifl5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 06/32] lockfile: unlock file if lockfile permissions cannot be adjusted","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-12T11:03:15Z","receivedAt":"2014-09-12T11:03:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/10/2014 12:39 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> If the call to adjust_shared_perm() fails, lock_file returns -1, which\n>> to the caller looks like any other failure to lock the file. So in\n>> this case, roll back the lockfile before returning so that the lock\n>> file is deleted immediately and the lockfile object is left in a\n>> predictable state (namely, unlocked). Previously, the lockfile was\n>> retained until process cleanup in this situation.\n> \n> ... which would mean that other processes can grab a lock on the\n> same file a bit earlier. Is there any negative implication caused by\n> that difference? I do not think of any but I could be missing\n> something.\n\nI think the end effect would be the same as if another process had\ngrabbed the lock a nanosecond before this process tried to do so. So\nassuming that callers handle that situation correctly, I don't think\nthis change can cause any problems.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249297","messageId":"5412D32C.2060003@alum.mit.edu","threadId":"37501","inReplyTo":"xmqqiokwifi9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 07/32] hold_lock_file_for_append(): release lock on errors","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-12T11:04:12Z","receivedAt":"2014-09-12T11:04:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/10/2014 12:41 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> If there is an error copying the old contents to the lockfile, roll\n>> back the lockfile before exiting so that the lockfile is not held\n>> until process cleanup.\n> \n> Same comment as 06/32 applies here.\n\nSame reply applies here, too.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249298","messageId":"5412D54C.5030602@alum.mit.edu","threadId":"37501","inReplyTo":"xmqq7g1bifm0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 00/32] Lockfile correctness and refactoring","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-12T11:13:16Z","receivedAt":"2014-09-12T11:13:16Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/10/2014 06:51 PM, Junio C Hamano wrote:\n> [...]\n> I am wondering if it makes sense to maintain a single ref that\n> reaches all the commits in this shared object store repository,\n> instead of keeping these millions of refs. When you need to make\n> more objects kept and reachable, create an octopus with the current\n> tip and tips of all these refs that causes you to wish making these\n> \"more objects kept and reachable\". Obviously that won't work well\n> if the reason why your current scheme uses refs is because you\n> adjust individual refs to prune some objects---hence the first\n> question in this message.\n\nThis is an interesting idea.\n\nI wouldn't trust git to handle efficiently commits with thousands of\nparents, but it would be easy to knit a huge number of tips together\nusing multiple layers of, say, 16-parent octopus merges.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249299","messageId":"5412D8CB.4000809@alum.mit.edu","threadId":"37501","inReplyTo":"20140910191159.GA23655@peff.net","subject":"Re: [PATCH v4 00/32] Lockfile correctness and refactoring","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-12T11:28:11Z","receivedAt":"2014-09-12T11:28:11Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/10/2014 09:11 PM, Jeff King wrote:\n> On Wed, Sep 10, 2014 at 09:51:03AM -0700, Junio C Hamano wrote:\n> \n>> Jeff King writes:\n>>\n>>> Yes, we don't let normal fetchers see these repos. They're only for\n>>> holding shared objects and the ref tips to keep them reachable.\n>>\n>> Are these individual refs have relations to the real world after\n>> they are created? To ask it another way, let's say that a branch in\n>> a repository, which is using this as a shared object store, caused\n>> one of these refs to be created; now the origin repository rewinds\n>> or deletes that branch---do you do anything to the ref in the shared\n>> object store at that point?\n> \n> Yes, we fetch from them before doing any maintenance in the shared\n> repository (like running repack). That's how objects migrate into the\n> shared repository, as well.\n> \n>> I am wondering if it makes sense to maintain a single ref that\n>> reaches all the commits in this shared object store repository,\n>> instead of keeping these millions of refs. When you need to make\n>> more objects kept and reachable, create an octopus with the current\n>> tip and tips of all these refs that causes you to wish making these\n>> \"more objects kept and reachable\". Obviously that won't work well\n>> if the reason why your current scheme uses refs is because you\n>> adjust individual refs to prune some objects---hence the first\n>> question in this message.\n> \n> Exactly. You could do this if you threw away and re-made the octopus\n> after each fetch (and then threw away the individual branches that went\n> into it). For that matter, if all you really want are the tips for\n> reachability, you can basically run \"for-each-ref | sort -u\"; most of\n> these refs are tags that are duplicated between each fork.\n> \n> However, having the individual tips does make some things easier. If I\n> only keep unique tips and I drop a tip from fork A, I would then need to\n> check every other fork to see if any other fork has the same tip. OTOH,\n> that means visiting N packed-refs files, each with (let's say) 3000\n> refs. As opposed to dealing with a packed-refs file with N*3000 refs. So\n> it's really not that different.\n\nI think it would make more sense to have one octopus per fork, rather\nthan one octopus for all of the forks. The octopus for a fork could be\ndiscarded and re-created every time that fork changed, without having to\nworry about which of the old tips is still reachable from another fork.\nIn fact, if you happen to know that a particular update to a fork is\npure fast-forward, you could update the fork octopus by merging the\nchanged tips into the old fork octopus without having to re-knit all of\nthe unchanged tips together again.\n\nThe shared repository would need one reference per fork--still much\nbetter than a reference for every reference in every fork. If even that\nis too many references, you could knit the fork octopuses into an\noverall octopus for all forks. But then updating that octopus for a\nchange to a fork would become more difficult, because you would have to\nread the octopuses for the N-1 unchanged forks from the old octopus and\nknit those together with the new octopus for the modified fork.\n\nBut all this imagineering doesn't mitigate the other reasons you list\nbelow for not wanting to guarantee reachability using this trick.\n\n> We also use the individual ref tips for packing. They factor into the\n> bitmap selection, and we have some patches (which I've been meaning to\n> upstream for a while now) to make delta selections in the shared-object\n> repository that will have a high chance of reuse in clones of individual\n> forks. And it's useful to query them for various reasons (e.g., \"who is\n> referencing this object?\").\n> [...]\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249300","messageId":"5412EC0F.5090601@alum.mit.edu","threadId":"37501","inReplyTo":"540C6A02.9070403@web.de","subject":"Re: [PATCH v4 00/32] Lockfile correctness and refactoring","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-12T12:50:23Z","receivedAt":"2014-09-12T12:50:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/07/2014 04:21 PM, Torsten Bögershausen wrote:\n> \n> On 2014-09-06 09.50, Michael Haggerty wrote:\n>> Sorry for the long delay since v3. This version mostly cleans up a\n>> couple more places where the lockfile object was left in an\n>> ill-defined state. \n> No problem with the delay.\n> The most important question is if we do the lk->active handling right.\n> Set it to false as seen as possible, and to true as late as possible,\n> then die() cleanly.\n> So the ->acive handling looks (more or less, please see below) and\n> deserves another critical review, may be.\n> \n> Instead of commenting each patch, I collected a mixture of small questions\n> and possible suggestions into a diff file.\n> \n> diff --git a/lockfile.c b/lockfile.c\n> index e54d260..7f27ea2 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -18,6 +18,10 @@\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> +LOCK_NODEREF can be read either as\n> +LOCK_NODE_REF or LOCK_NO_DEREF\n> +should we change it ?\n> +\n\nGood idea.\n\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> @@ -46,9 +50,18 @@\n> * state:\n> * - the lockfile exists\n> * - active is set\n> - * - filename holds the filename of the lockfile\n> + * - filename holds the filename of the lockfile in a strbuf\n\nI don't think this is necessary. The point of this list is to describe\nthe state machine, not the contents of the lock_file structure, so this\ndetail is only a distraction. And even if a reader is confused, the\ncompiler will warn if he tries to use the strbuf as if it were a string.\n\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> +question: Why do we need the PID here ?\n> +Do we open a lock file and do a fork() ?\n> +And if yes, the child gets a new PID, what happens when the\n> +child gets a signal ?\n> +Who \"owns\" the lockfile, the parent, the child, both ?\n> +The child has access to all data, the fd is open and can be used,\n> +why do we not allow a rollback, when the child dies ?\n\nGood questions. I will add an explanation of the purpose of the pid in\nthis docstring.\n\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> @@ -57,7 +70,7 @@\n> * rollback_lock_file(), or a failed attempt to lock). In this\n> * state:\n> * - active is unset\n> - * - filename is the empty string (usually, though there are\n> + * - filename is an empty string buffer (usually, though there are\n> * transitory 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> @@ -68,7 +81,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\nGood idea. I will rename these two functions.\n\n> {\n> pid_t me = getpid();\n> \n> @@ -79,9 +92,9 @@ static void remove_lock_file(void)\n> }\n> }\n> \n> -static void remove_lock_file_on_signal(int signo)\n> +static void remove_lock_files_on_signal(int signo)\n> {\n> - remove_lock_file();\n> + remove_lock_files();\n> sigchain_pop(signo);\n> raise(signo);\n> }\n> @@ -93,7 +106,7 @@ static void remove_lock_file_on_signal(int signo)\n> * \"/\", if any). If path is empty or the root directory (\"/\"), set\n> * path to the empty string.\n> */\n> -static void trim_last_path_elm(struct strbuf *path)\n> +static void trim_last_path_elem(struct strbuf *path)\n\nI agree that the old name was bad. I will make it even more explicit:\ntrim_last_path_component().\n\n> {\n> int i = path->len;\n> \n> @@ -143,7 +156,7 @@ static void resolve_symlink(struct strbuf *path)\n> * link is a relative path, so replace the\n> * last element of p with it.\n> */\n> - trim_last_path_elm(path);\n> + trim_last_path_elem(path);\n> \n> strbuf_addbuf(path, &link);\n> }\n> @@ -153,13 +166,16 @@ static void resolve_symlink(struct strbuf *path)\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> + struct stat st;\n> + int mode = 0666;\n> if (!lock_file_list) {\n> /* One-time initialization */\n> - sigchain_push_common(remove_lock_file_on_signal);\n> - atexit(remove_lock_file);\n> + sigchain_push_common(remove_lock_files_on_signal);\n> + atexit(remove_lock_files);\n> }\n> \n> - assert(!lk->active);\n> + if (lk->active)\n> + die(\"lk->active %s\", path);\n\nOK, but I will use die(\"BUG:...\") since this would be an indication of a\nbug in git.\n\n> if (!lk->on_list) {\n> /* Initialize *lk and add it to lock_file_list: */\n> @@ -167,16 +183,25 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n> lk->active = 0;\n> lk->owner = 0;\n> lk->on_list = 1;\n> - strbuf_init(&lk->filename, 0);\n> + strbuf_init(&lk->filename, strlen(path) + LOCK_SUFFIX_LEN);\n\nOK.\n\n> lk->next = lock_file_list;\n> lock_file_list = lk;\n> }\n> \n> + strbuf_reset(&lk->filename); /* Better to be save */\n\nI agree, but this would be a bug so I'd rather die(\"BUG:\") in this case.\n\n> strbuf_addstr(&lk->filename, path);\n> if (!(flags & LOCK_NODEREF))\n> resolve_symlink(&lk->filename);\n> strbuf_addstr(&lk->filename, LOCK_SUFFIX);\n> - lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n> + /*\n> + * adjust_shared_perm() will widen permissions if needed,\n> + * otherwise keep permissions restrictive\n> + *\n> + */\n> + if (!stat(path, &st))\n> + mode = st.st_mode & 07777;\n> +\n> + lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, mode);\n\nI think this change is separate from the changes made in this series,\nand needs its own justification (i.e., it's not immediately obvious to\nme whether the change is an improvement). Would you mind submitting it\nas a separate patch?\n\n> if (lk->fd < 0) {\n> strbuf_reset(&lk->filename);\n> return -1;\n> @@ -268,7 +293,7 @@ int close_lock_file(struct lock_file *lk)\n> return close(fd);\n> }\n> \n> -int reopen_lock_file(struct lock_file *lk)\n> +int reopen_lock_file_UNUSED_CAN_IT_BE_REMOVED(struct lock_file *lk)\n\nJunio added this function recently, without any callers, in 93dcaea2. I\nassume he has some diabolical plan for it. Junio?\n\n> {\n> if (0 <= lk->fd)\n> die(_(\"BUG: reopen a lockfile that is still open\"));\n> @@ -283,7 +308,7 @@ int commit_lock_file_to(struct lock_file *lk, const char *path)\n> int save_errno;\n> \n> if (!lk->active)\n> - die(\"BUG: attempt to commit unlocked object\");\n> + die(\"BUG: attempt to commit unlocked object %s\", path);\n\nOK.\n\n> \n> if (lk->fd >= 0 && close_lock_file(lk))\n> goto rollback;\n> @@ -325,10 +350,12 @@ void rollback_lock_file(struct lock_file *lk)\n> {\n> if (!lk->active)\n> return;\n> + lk->active = 0; /* We are going to de-activate,\n> + so active is no longer valid already here ? */\n\nI think the question is: what should happen if a signal arrives at this\nmoment? If we set active=0 here, then the signal handler wouldn't clean\nup this lockfile at all. But we haven't removed it yet, either. So the\nlockfile would get left behind.\n\nSo how much longer can we leave active set here?\n\n> \n> if (lk->fd >= 0)\n> close_lock_file(lk);\n\nclose_lock_file() is careful to clear lk->fd *before* closing it, so it\nshouldn't be possible for the file to get closed twice [1]. Therefore I\nthink it is OK to leave lk->active set during the call to close_lock_file().\n\n> unlink_or_warn(lk->filename.buf);\n\nThis line is a bit dangerous. If we call unlink_or_warn() with\nlk->active set, then there is a chance that unlink_and_warn() will be\ncalled a second time by a signal handler that runs after this line but\nbefore lk->active is cleared. The first call would delete the lockfile\nthat we created, but the second call (which might be delayed for a bit\nwhile our signal handler works its way through the lockfile linked-list)\nmight end up deleting a lockfile that another process created between\nour two calls. That would be a bad outcome, but it requires a double\ncoincidence: our process has to receive a signal at this pessimal moment\n*and* another process has to create a lockfile after that event but\nbefore our signal handler has run. And for *real* damage to occur, a\n*third* process has to incorrectly acquire the *same* lockfile because\nof its incorrect deletion by our process, and it has to make a change\nthat conflicts with the change that the second process is trying to make.\n\nOn the other hand, if we clear lk->active *before* calling\nunlink_or_warn(), then the danger is that we receive a signal and never\nclean up the lockfile. The result is perhaps not as bad as deleting\nanother process's lockfile, but it seems to me that it is far more\nlikely: it can happen regardless of whether another process is racing\nwith us, let alone a third process is trying to acquire the same lock.\n\nCan anybody think of a reasonable way to make this 100% safe?\n\nIf not, I think the code as written is safer than your proposed change.\n\n> - lk->active = 0;\n> + //lk->active = 0;\n> strbuf_reset(&lk->filename);\n> }\n> \n> \n\nThanks very much for your feedback!\n\nMichael\n\n[1] Another question (and I think somebody else brought it up) is\nwhether close_lock_file() should actually close the file *before*\nclearing lk->fd. I will address that question elsewhere.\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"249304","messageId":"54130155.90707@alum.mit.edu","threadId":"37501","inReplyTo":"20140910081358.GB16413@peff.net","subject":"Re: [PATCH v4 00/32] Lockfile correctness and refactoring","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-12T14:21:09Z","receivedAt":"2014-09-12T14:21:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/10/2014 10:13 AM, Jeff King wrote:\n> [...]\n> I did coincidentally have an interesting experience with our lockfile\n> code earlier today, which I'd like to relate.\n> \n> I was running pack-refs on a repository with a very large number of\n> loose refs (about 1.8 million). [...] Each call to lock_ref_sha1_basic\n> allocates a \"struct lock_file\", which then gets added to the global\n> lock_file list. Each one contains a fixed PATH_MAX buffer (4K on this\n> machine). After we're done updating the ref, we leak the lock_file\n> struct, since there's no way to remove it from the list.\n> \n> As a result, git tried to allocate 7G of RAM and got OOM-killed (the\n> machine had only 8G). In addition to thrashing the disk even harder,\n> since there was no room left for disk cache while we touched millions of\n> loose refs. :)\n> \n> Your change in this series to use a strbuf would make this a lot better.\n> But I still wonder how hard it would be to just remove lock_file structs\n> from the global list when they are committed or rolled back. That would\n> presumably also make the \"avoid transitory valid states\" patch from your\n> series a bit easier, too (you could prepare the lockfile in peace, and\n> then link it in fully formed, and do the opposite when removing it).\n> \n> I think with your strbuf patch, this leak at least becomes reasonable.\n> So maybe it's not worth going further. But I'd be interested to hear\n> your thoughts since you've been touching the area recently.\n\nI've thought about this too, but it didn't seem to be worth the effort.\n(Though your use case certainly adds a bit of justification.)\n\nTo make that change, we would have to remove entries from the list of\nlock_file objects in a way that the code can be interrupted at any time\nby a signal while leaving it in a state that is traversable by the\nsignal handler.\n\nI think that can be done pretty easily with a singly-linked list. But\nwith a singly-linked list, we would have to iterate through the list to\nfind the node that needs to be removed. This could get expensive if\nthere are a lot of nodes in the list (see below).\n\nSo we would probably want to switch to using a doubly-linked list. I\nthink this would also be fairly simple, given that the signal handler\nonly needs to iterate through the list in a single direction. You'd just\nhave to be careful about adjusting the pointers in the right order to\nlet (say) a forwards traversal always work.\n\nThen the callers who use heap-allocated lock_file objects would have to\nbe changed to free them when they're done with them, probably using a\nspecial function that releases the strbufs, too. Callers using\nstatically-allocated lock_file objects would probably not have to be\nchanged.\n\nBut...\n\nThe ref-transaction code is, I think, moving in the direction of\nupdating all references in a single transaction. This means that we\nwould need to hold locks for all of the references at once anyway. So it\nmight be all for naught.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249309","messageId":"541322DC.4050009@alum.mit.edu","threadId":"37501","inReplyTo":"CAL=YDWmhSuk9rqBKRbJLfNSn8QTtebbBMwSJqqAYOKS=ZZ1=qg@mail.gmail.com","subject":"Re: [PATCH v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-12T16:44:12Z","receivedAt":"2014-09-12T16:44:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/12/2014 12:15 AM, Ronnie Sahlberg wrote:\n> On Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty wrote:\n>> There are a few places that use these values, so define constants for\n>> them.\n>>\n>> Signed-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>>\n>> diff --git a/cache.h b/cache.h\n>> index da77094..41d829b 100644\n>> --- a/cache.h\n>> +++ b/cache.h\n>> @@ -569,6 +569,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);\n>> #define REFRESH_IN_PORCELAIN 0x0020 /* 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>> struct lock_file *next;\n>> int fd;\n>> diff --git a/lockfile.c b/lockfile.c\n>> index 964b3fc..bfea333 100644\n>> --- a/lockfile.c\n>> +++ b/lockfile.c\n>> @@ -176,10 +176,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>> /*\n>> - * subtract 5 from size to make sure there's room for adding\n>> - * \".lock\" for the lock file name\n>> + * subtract LOCK_SUFFIX_LEN from size to make sure there's\n>> + * room for adding \".lock\" for the lock file name:\n>> */\n>> - static const size_t max_path_len = sizeof(lk->filename) - 5;\n>> + static const size_t max_path_len = sizeof(lk->filename) -\n>> + LOCK_SUFFIX_LEN;\n>>\n>> if (!lock_file_list) {\n>> /* One-time initialization */\n>> @@ -204,7 +205,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n>> strcpy(lk->filename, path);\n>> if (!(flags & LOCK_NODEREF))\n>> resolve_symlink(lk->filename, max_path_len);\n>> - strcat(lk->filename, \".lock\");\n>> + strcat(lk->filename, LOCK_SUFFIX);\n>> lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n>> if (0 <= lk->fd) {\n>> lk->owner = getpid();\n>> @@ -314,7 +315,7 @@ int commit_lock_file(struct lock_file *lk)\n>> if (lk->fd >= 0 && close_lock_file(lk))\n>> return -1;\n>> strcpy(result_file, lk->filename);\n>> - i = strlen(result_file) - 5; /* .lock */\n>> + i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */\n> \n> Not a new bug since the previous code is broken too.\n> Should probably checkstrlen(result_file) >= 5 here before subtracting 5.\n> \n> Otherwise, a caller that calls commit_lock_file() with an already\n> committed/closed lock_file can cause writing outside the bounds of\n> the array on the line below.\n\nGood catch; thanks. I will fix this in the reroll (though probably in a\nlater patch).\n\n>> [...]\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249310","messageId":"541329B8.1050200@alum.mit.edu","threadId":"37501","inReplyTo":"CAL=YDWmgwwQsCSpVsQ5U+c=4RcLdzo-j_hmxDk-AJWdRUk_SsQ@mail.gmail.com","subject":"Re: [PATCH v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-12T17:13:28Z","receivedAt":"2014-09-12T17:13:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/12/2014 12:42 AM, Ronnie Sahlberg wrote:\n> Maybe we should not have a public constant defined for the length :\n> +#define LOCK_SUFFIX_LEN 5\n> \n> since it encourages unsafe code like : (this was unsafe long before\n> your patch so not a regression)\n> + i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */\n> result_file[i] = 0;\n> \n> \n> \n> What about removing LOCK_SUFFIX_LEN from the public API and introduce\n> a helper function something like :\n> \n> \n> /* pointer to the character where the lock suffix starts */\n> char *lock_suffix_ptr_safe(const char *filename)\n> {\n> size_t len = strlen(filename);\n> if (len < 5)\n> die(\"BUG:...\n> if (strcmp(filename + len - 5, LOCK_SUFFIX)\n> die(\"BUG:...\n> return filename + len - 5;\n> }\n> \n> and use it instead?\n\nAt the end of this patch series, LOCK_SUFFIX_LEN is only used in two\nplaces outside of lockfile.c:\n\n* In check_refname_component(), to ensure that no component of a\nreference name ends with \".lock\". This only indirectly has anything to\ndo with lockfiles.\n\n* In delete_ref_loose(), to derive the name of the loose reference file\nfrom the name of the lockfile. It immediately xmemdupz()s the part of\nthe filename that it needs, so it is kosher.\n\nI will add a function get_locked_file_path() for the use of the second\ncaller.\n\nI like being able to use the symbolic constant at the first caller, and\nit is not dangerous. I don't think it is so important to make the\nconstant private, because I think somebody programming sloppily wouldn't\nbe deterred for long by not seeing a symbolic constant for the suffix\nlength. So if it's OK with you I'll leave the constant.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249313","messageId":"CAL=YDWmkphVddWQF028_AmVMzcPQ8nw9sCnpzPSijbN7o5xjdQ@mail.gmail.com","threadId":"37501","inReplyTo":"541329B8.1050200@alum.mit.edu","subject":"Re: [PATCH v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-09-12T17:32:42Z","receivedAt":"2014-09-12T17:32:42Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Fri, Sep 12, 2014 at 10:13 AM, Michael Haggerty wrote:\n> On 09/12/2014 12:42 AM, Ronnie Sahlberg wrote:\n>> Maybe we should not have a public constant defined for the length :\n>> +#define LOCK_SUFFIX_LEN 5\n>>\n>> since it encourages unsafe code like : (this was unsafe long before\n>> your patch so not a regression)\n>> + i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */\n>> result_file[i] = 0;\n>>\n>>\n>>\n>> What about removing LOCK_SUFFIX_LEN from the public API and introduce\n>> a helper function something like :\n>>\n>>\n>> /* pointer to the character where the lock suffix starts */\n>> char *lock_suffix_ptr_safe(const char *filename)\n>> {\n>> size_t len = strlen(filename);\n>> if (len < 5)\n>> die(\"BUG:...\n>> if (strcmp(filename + len - 5, LOCK_SUFFIX)\n>> die(\"BUG:...\n>> return filename + len - 5;\n>> }\n>>\n>> and use it instead?\n>\n> At the end of this patch series, LOCK_SUFFIX_LEN is only used in two\n> places outside of lockfile.c:\n>\n> * In check_refname_component(), to ensure that no component of a\n> reference name ends with \".lock\". This only indirectly has anything to\n> do with lockfiles.\n>\n> * In delete_ref_loose(), to derive the name of the loose reference file\n> from the name of the lockfile. It immediately xmemdupz()s the part of\n> the filename that it needs, so it is kosher.\n>\n> I will add a function get_locked_file_path() for the use of the second\n> caller.\n>\n> I like being able to use the symbolic constant at the first caller, and\n> it is not dangerous. I don't think it is so important to make the\n> constant private, because I think somebody programming sloppily wouldn't\n> be deterred for long by not seeing a symbolic constant for the suffix\n> length. So if it's OK with you I'll leave the constant.\n\nIt is OK with me.\n"},{"id":"249336","messageId":"5413F51E.3060600@kdbg.org","threadId":"37501","inReplyTo":"1409989846-22401-12-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v4 11/32] delete_ref_loose(): don't muck around in the lock_file's filename","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-09-13T07:41:18Z","receivedAt":"2014-09-13T07:41:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 06.09.2014 um 09:50 schrieb Michael Haggerty:\n> It's bad manners. Especially since, if unlink_or_warn() failed, the\n> memory wasn't restored to its original contents.\n\nI do not see how the old code did not restore the file name. Except for\nthis nit, the patch looks good.\n\n> \n> So make our own copy to work with.\n> \n> Signed-off-by: Michael Haggerty \n> ---\n> refs.c | 15 +++++++++------\n> 1 file changed, 9 insertions(+), 6 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index 828522d..8a63073 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2545,12 +2545,15 @@ static int repack_without_ref(const char *refname)\n> static int delete_ref_loose(struct ref_lock *lock, int flag)\n> {\n> \tif (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n> -\t\t/* loose */\n> -\t\tint err, i = strlen(lock->lk->filename) - 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> \n"},{"id":"249352","messageId":"20140913185101.GA24773@peff.net","threadId":"37501","inReplyTo":"54130155.90707@alum.mit.edu","subject":"Re: [PATCH v4 00/32] Lockfile correctness and refactoring","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-13T18:51:01Z","receivedAt":"2014-09-13T18:51:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 12, 2014 at 04:21:09PM +0200, Michael Haggerty wrote:\n\n> > But I still wonder how hard it would be to just remove lock_file structs\n> > from the global list when they are committed or rolled back.\n> [...]\n>\n> To make that change, we would have to remove entries from the list of\n> lock_file objects in a way that the code can be interrupted at any time\n> by a signal while leaving it in a state that is traversable by the\n> signal handler.\n> \n> I think that can be done pretty easily with a singly-linked list. But\n> with a singly-linked list, we would have to iterate through the list to\n> find the node that needs to be removed. This could get expensive if\n> there are a lot of nodes in the list (see below).\n\nYes, I considered that, but noticed that if we actually cleaned up\nclosed files, the list would not grow to more than a handful of entries.\nBut...\n\n> The ref-transaction code is, I think, moving in the direction of\n> updating all references in a single transaction. This means that we\n> would need to hold locks for all of the references at once anyway. So it\n> might be all for naught.\n\nThat nullifies the whole discussion. Besides the list-traversal thing\nabove, it would mean that we literally _do_ have all of the lockfiles\nopen at once. So cleaning up after ourselves would be nice, but it would\nnot impact the peak memory usage, which would necessarily have one\nallocated struct per ref.\n\nThe use of a strbuf is probably a big enough change to save us there.\nThis case was pathological for a few reasons:\n\n 1. A ridiculous number of refs in the repository.\n\n 2. Touching a large number of them in sequence (via pack-refs).\n\n 3. Allocating a 4K buffer per object.\n\nFor (3), if the average allocation is dropped even to 400 bytes (which\nwould accommodate quite a long pathname), that would reduce the memory\nusage to ~700MB. Not amazing, but enough not to tip over most modern\nmachines.\n\n-Peff\n"},{"id":"249377","messageId":"5415353E.8060907@alum.mit.edu","threadId":"37501","inReplyTo":"5413F51E.3060600@kdbg.org","subject":"Re: [PATCH v4 11/32] delete_ref_loose(): don't muck around in the lock_file's filename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-14T06:27:10Z","receivedAt":"2014-09-14T06:27:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/13/2014 09:41 AM, Johannes Sixt wrote:\n> Am 06.09.2014 um 09:50 schrieb Michael Haggerty:\n>> It's bad manners. Especially since, if unlink_or_warn() failed, the\n>> memory wasn't restored to its original contents.\n> \n> I do not see how the old code did not restore the file name. Except for\n> this nit, the patch looks good.\n\nHmmmm, you're quite right. I thought I had found some circumstance in\nwhich unlink_or_warn() could fail to allocate memory and die() or\nsomething. But I can't find anything like that now.\n\nI will remove that sentence from the commit message.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249378","messageId":"541537C9.1080304@alum.mit.edu","threadId":"37501","inReplyTo":"5415353E.8060907@alum.mit.edu","subject":"Re: [PATCH v4 11/32] delete_ref_loose(): don't muck around in the lock_file's filename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-14T06:38:01Z","receivedAt":"2014-09-14T06:38:01Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/14/2014 08:27 AM, Michael Haggerty wrote:\n> On 09/13/2014 09:41 AM, Johannes Sixt wrote:\n>> Am 06.09.2014 um 09:50 schrieb Michael Haggerty:\n>>> It's bad manners. Especially since, if unlink_or_warn() failed, the\n>>> memory wasn't restored to its original contents.\n>>\n>> I do not see how the old code did not restore the file name. Except for\n>> this nit, the patch looks good.\n> \n> Hmmmm, you're quite right. I thought I had found some circumstance in\n> which unlink_or_warn() could fail to allocate memory and die() or\n> something. But I can't find anything like that now.\n> \n> I will remove that sentence from the commit message.\n\nI half withdraw my withdrawal. It's true that the failure of\nunlink_or_warn() wouldn't cause a problem. But a signal could arrive\nwhile unlink_or_warn() is executing, in which case the signal handler\nwould see the wrong filename and try to delete the loose reference file,\nleaving the lockfile behind.\n\nI will clarify the log message.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249398","messageId":"5415AAE7.3060806@kdbg.org","threadId":"37501","inReplyTo":"541537C9.1080304@alum.mit.edu","subject":"Re: [PATCH v4 11/32] delete_ref_loose(): don't muck around in the lock_file's filename","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-09-14T14:49:11Z","receivedAt":"2014-09-14T14:49:11Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 14.09.2014 um 08:38 schrieb Michael Haggerty:\n> On 09/14/2014 08:27 AM, Michael Haggerty wrote:\n>> On 09/13/2014 09:41 AM, Johannes Sixt wrote:\n>>> Am 06.09.2014 um 09:50 schrieb Michael Haggerty:\n>>>> It's bad manners. Especially since, if unlink_or_warn() failed, the\n>>>> memory wasn't restored to its original contents.\n>>>\n>>> I do not see how the old code did not restore the file name. Except for\n>>> this nit, the patch looks good.\n>>\n>> Hmmmm, you're quite right. I thought I had found some circumstance in\n>> which unlink_or_warn() could fail to allocate memory and die() or\n>> something. But I can't find anything like that now.\n>>\n>> I will remove that sentence from the commit message.\n> \n> I half withdraw my withdrawal. It's true that the failure of\n> unlink_or_warn() wouldn't cause a problem. But a signal could arrive\n> while unlink_or_warn() is executing, in which case the signal handler\n> would see the wrong filename and try to delete the loose reference file,\n> leaving the lockfile behind.\n\nGood catch! This makes the patch much more important than just to\nestablish good manners.\n\n-- Hannes\n"}]}