{"thread":{"id":"37633","subject":"[PATCH v6 00/39] Lockfile correctness and refactoring","startedAt":"2014-09-26T10:08:00Z","lastAt":"2014-10-01T08:11:36Z","messageCount":55,"participants":["Michael Haggerty","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":6,"patchTotal":39},"messages":[{"id":"249873","messageId":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":null,"subject":"[PATCH v6 00/39] Lockfile correctness and refactoring","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:00Z","receivedAt":"2014-09-26T10:08:00Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Next iteration of my lockfile fixes and refactoring. Thanks to\nJonathan Nieder and Torsten Bögershausen for their comments about v5.\n\nI believe that this series addresses all of the comments from v1 [1],\nv2 [2], v3 [3], v4 [4], and v5 [5].\n\nChanges since v4:\n\n* Revise API documentation.\n\n* Split out a separate header file for the lockfile API: lockfile.h.\n\n* Change close_lock_file() to rollback on errors and make it into a\n NOOP if the file is already closed.\n\n* Don't set lk->on_list until the lock_file object is completely on\n the lock_file_list.\n\n* Delete some information from the docstring in lockfile.c (now\n lockfile.h) that is redundant with the API docs in api-lockfile.txt.\n\n* Remove the accidental extra call of git_config_clear() in\n git_config_set_multivar_in_file() when commit_lock_file() fails.\n\n* Adjust some comments, error messages, and commit messages.\n\n* Rebase to current master.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/245609\n[2] http://thread.gmane.org/gmane.comp.version-control.git/245801\n[3] http://thread.gmane.org/gmane.comp.version-control.git/246222\n[4] http://thread.gmane.org/gmane.comp.version-control.git/256564\n[5] http://thread.gmane.org/gmane.comp.version-control.git/257159\n\nMichael Haggerty (39):\n unable_to_lock_die(): rename function from unable_to_lock_index_die()\n api-lockfile: revise and expand the documentation\n close_lock_file(): exit (successfully) if file is already closed\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 initialize and register lock_file object\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 close_lock_file(): if close fails, roll back\n commit_lock_file(): rollback lock file on failure to rename\n api-lockfile: document edge cases\n dump_marks(): remove a redundant call to rollback_lock_file()\n git_config_set_multivar_in_file(): avoid call to rollback_lock_file()\n lockfile: avoid transitory invalid states\n struct lock_file: declare some fields volatile\n try_merge_strategy(): remove redundant lock_file allocation\n try_merge_strategy(): use a statically-allocated lock_file object\n commit_lock_file(): use a strbuf to manage temporary space\n Change lock_file::filename into a strbuf\n resolve_symlink(): use a strbuf for internal scratch space\n resolve_symlink(): take a strbuf parameter\n trim_last_path_component(): replace last_path_elm()\n Extract a function commit_lock_file_to()\n Rename LOCK_NODEREF to LOCK_NO_DEREF\n lockfile.c: rename static functions\n get_locked_file_path(): new function\n hold_lock_file_for_append(): restore errno before returning\n Move read_index() definition to read-cache.c\n lockfile.h: extract new header file for the functions in lockfile.c\n\n Documentation/technical/api-lockfile.txt | 220 ++++++++++++++++++------\n builtin/add.c | 1 +\n builtin/apply.c | 1 +\n builtin/checkout-index.c | 2 +-\n builtin/checkout.c | 2 +-\n builtin/clone.c | 1 +\n builtin/commit.c | 17 +-\n builtin/describe.c | 1 +\n builtin/diff.c | 1 +\n builtin/gc.c | 2 +-\n builtin/merge.c | 16 +-\n builtin/mv.c | 2 +-\n builtin/read-tree.c | 1 +\n builtin/receive-pack.c | 1 +\n builtin/reflog.c | 4 +-\n builtin/reset.c | 1 +\n builtin/rm.c | 2 +-\n builtin/update-index.c | 3 +-\n bundle.c | 1 +\n cache-tree.c | 1 +\n cache.h | 20 +--\n config.c | 16 +-\n credential-store.c | 1 +\n fast-import.c | 5 +-\n fetch-pack.c | 1 +\n lockfile.c | 284 +++++++++++++++++--------------\n lockfile.h | 84 +++++++++\n merge-recursive.c | 1 +\n merge.c | 1 +\n read-cache.c | 21 ++-\n refs.c | 30 ++--\n rerere.c | 1 +\n sequencer.c | 1 +\n sha1_file.c | 1 +\n shallow.c | 7 +-\n test-scrap-cache-tree.c | 1 +\n 36 files changed, 492 insertions(+), 263 deletions(-)\n create mode 100644 lockfile.h\n\n-- \n2.1.0\n"},{"id":"249877","messageId":"1411726119-31598-2-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 01/39] unable_to_lock_die(): rename function from unable_to_lock_index_die()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:01Z","receivedAt":"2014-09-26T10:08:01Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This function is used for other things besides the index, so rename it\naccordingly.\n\nSuggested-by: Jeff King \nSigned-off-by: Michael Haggerty \nReviewed-by: Ronnie Sahlberg \nReviewed-by: Jonathan Nieder \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 af590d5..07682cb 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -582,7 +582,7 @@ struct lock_file {\n extern int unable_to_lock_error(const char *path, int err);\n extern void unable_to_lock_message(const char *path, int err,\n \t\t\t\t struct strbuf *buf);\n-extern NORETURN void unable_to_lock_index_die(const char *path, int err);\n+extern NORETURN void unable_to_lock_die(const char *path, int err);\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n extern int commit_lock_file(struct lock_file *);\ndiff --git a/lockfile.c b/lockfile.c\nindex 2a800ce..f1ce154 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -185,7 +185,7 @@ int unable_to_lock_error(const char *path, int err)\n \treturn -1;\n }\n \n-NORETURN void unable_to_lock_index_die(const char *path, int err)\n+NORETURN void unable_to_lock_die(const char *path, int err)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \n@@ -198,7 +198,7 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)\n {\n \tint fd = lock_file(lk, path, flags);\n \tif (fd < 0 && (flags & LOCK_DIE_ON_ERROR))\n-\t\tunable_to_lock_index_die(path, errno);\n+\t\tunable_to_lock_die(path, errno);\n \treturn fd;\n }\n \n@@ -209,7 +209,7 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n \tfd = lock_file(lk, path, flags);\n \tif (fd < 0) {\n \t\tif (flags & LOCK_DIE_ON_ERROR)\n-\t\t\tunable_to_lock_index_die(path, errno);\n+\t\t\tunable_to_lock_die(path, errno);\n \t\treturn fd;\n \t}\n \ndiff --git a/refs.c b/refs.c\nindex 2ce5d69..8cb3539 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2167,7 +2167,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t */\n \t\t\tgoto retry;\n \t\telse\n-\t\t\tunable_to_lock_index_die(ref_file, errno);\n+\t\t\tunable_to_lock_die(ref_file, errno);\n \t}\n \treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n \n-- \n2.1.0\n"},{"id":"249875","messageId":"1411726119-31598-3-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 02/39] api-lockfile: revise and expand the documentation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:02Z","receivedAt":"2014-09-26T10:08:02Z","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().\nReorganize the document to make it more accessible.\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Michael Haggerty \n---\n Documentation/technical/api-lockfile.txt | 199 ++++++++++++++++++++++---------\n 1 file changed, 145 insertions(+), 54 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex dd89404..c14fca5 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -3,20 +3,124 @@ lockfile API\n \n The lockfile API serves two purposes:\n \n-* Mutual exclusion. When we write out a new index file, first\n- we create a new file `$GIT_DIR/index.lock`, write the new\n- contents into it, and rename it to the final destination\n- `$GIT_DIR/index`. We try to create the `$GIT_DIR/index.lock`\n- file with O_EXCL so that we can notice and fail when somebody\n- else is already trying to update the index file.\n-\n-* Automatic cruft removal. After we create the \"lock\" file, we\n- may decide to `die()`, and we would want to make sure that we\n- remove the file that has not been committed to its final\n- destination. This is done by remembering the lockfiles we\n- created in a linked list and cleaning them up from an\n- `atexit(3)` handler. Outstanding lockfiles are also removed\n- when the program dies on a signal.\n+* Mutual exclusion and atomic file updates. When we want to change a\n+ file, we create a lockfile `.lock`, write the new file\n+ contents into it, and then rename the lockfile to its final\n+ destination ``. We create the `.lock` file with\n+ `O_CREAT|O_EXCL` so that we can notice and fail if somebody else has\n+ already locked the file.\n+\n+* Automatic cruft removal. If the program exits after we lock a file\n+ but before the changes have been committed, we want to make sure\n+ that we remove the lockfile. This is done by remembering the\n+ lockfiles we have created in a linked list and setting up an\n+ `atexit(3)` handler and a signal handler that clean up the\n+ lockfiles. This mechanism ensures that outstanding lockfiles are\n+ cleaned up if the program exits (including when `die()` is called)\n+ or if the program dies on a signal.\n+\n+Please note that lockfiles only block other writers. Readers do not\n+block, but they are guaranteed to see either the old contents of the\n+file or the new contents of the file (assuming that the filesystem\n+implements `rename(2)` atomically).\n+\n+\n+Calling sequence\n+----------------\n+\n+The caller:\n+\n+* Allocates a variable `struct lock_file lock` in the bss section or\n+ heap, initialized to zeros. It cannot be an auto variable allocated\n+ on the stack. Because the `lock_file` structure is used in an\n+ `atexit(3)` handler, its storage has to stay throughout the life of\n+ the program, even after the file changes have been committed or\n+ rolled back.\n+\n+* Attempts to create a lockfile by passing that variable and the path\n+ of the final destination (e.g. `$GIT_DIR/index`) to\n+ `hold_lock_file_for_update` or `hold_lock_file_for_append`.\n+\n+* Writes new content for the destination file by writing to the file\n+ descriptor returned by those functions (also available via\n+ `lock->fd`).\n+\n+When finished writing, the caller can:\n+\n+* Close the file descriptor and rename the lockfile to its final\n+ destination by calling `commit_lock_file`.\n+\n+* Close the file descriptor and remove the lockfile by calling\n+ `rollback_lock_file`.\n+\n+* Close the file descriptor without removing or renaming the lockfile\n+ by calling `close_lock_file`, and later call `commit_lock_file` or\n+ `rollback_lock_file`.\n+\n+At this point, the `lock_file` object must not be freed or altered,\n+because it is still registered in the `lock_file_list`. However, it\n+may be reused; just pass it to another call of\n+`hold_lock_file_for_update` or `hold_lock_file_for_append`.\n+\n+If the program exits before you have called one of `commit_lock_file`,\n+`rollback_lock_file`, or `close_lock_file`, an `atexit(3)` handler\n+will close and remove the lockfile, essentially rolling back any\n+uncommitted changes.\n+\n+If you need to close the file descriptor you obtained from a\n+`hold_lock_file_*` function yourself, do so by calling\n+`close_lock_file`. You should never call `close(2)` yourself!\n+Otherwise the `struct lock_file` structure would still think that the\n+file descriptor needs to be closed, and a later call to\n+`commit_lock_file` or `rollback_lock_file` or program exit would\n+result in duplicate calls to `close(2)`. Worse yet, if you `close(2)`\n+and then later open another file descriptor for a completely different\n+purpose, then a call to `commit_lock_file` or `rollback_lock_file`\n+might close that unrelated file descriptor.\n+\n+\n+Error handling\n+--------------\n+\n+The `hold_lock_file_*` functions return a file descriptor on success\n+or -1 on failure (unless `LOCK_DIE_ON_ERROR` is used; see below). On\n+errors, `errno` describes the reason for failure. Errors can be\n+handled by passing `errno` to one of the following helper functions:\n+\n+unable_to_lock_message::\n+\n+\tAppend an appropriate error message to a `strbuf`.\n+\n+unable_to_lock_error::\n+\n+\tEmit an appropriate error message using `error()`.\n+\n+unable_to_lock_die::\n+\n+\tEmit an appropriate error message and `die()`.\n+\n+\n+Flags\n+-----\n+\n+The following flags can be passed to `hold_lock_file_for_update` or\n+`hold_lock_file_for_append`:\n+\n+LOCK_NODEREF::\n+\n+\tUsually symbolic links in the destination path are resolved\n+\tand the lockfile is created by adding \".lock\" to the resolved\n+\tpath. If `LOCK_NODEREF` is set, then the lockfile is created\n+\tby adding \".lock\" to the path argument itself. This option is\n+\tused, for example, when locking a symbolic reference, which\n+\tfor backwards-compatibility reasons can be a symbolic link\n+\tcontaining the name of the referred-to-reference.\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, trying to lock a\n+\tfile that is already locked returns -1 to the caller.\n \n \n The functions\n@@ -24,51 +128,38 @@ The functions\n \n hold_lock_file_for_update::\n \n-\tTake a pointer to `struct lock_file`, the filename of\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+\tTake a pointer to `struct lock_file`, the path of the file to\n+\tbe locked (e.g. `$GIT_DIR/index`) and a flags argument (see\n+\tabove). Attempt to create a lockfile for the destination and\n+\treturn the file descriptor for writing to the file.\n+\n+hold_lock_file_for_append::\n+\n+\tLike `hold_lock_file_for_update`, but before returning copy\n+\tthe existing contents of the file (if any) to the lockfile and\n+\tposition its write pointer at the end of the file.\n \n commit_lock_file::\n \n-\tTake a pointer to the `struct lock_file` initialized\n-\twith an earlier call to `hold_lock_file_for_update()`,\n-\tclose the file descriptor and rename the lockfile to its\n-\tfinal destination. Returns 0 upon success, a negative\n-\tvalue on failure to close(2) or rename(2).\n+\tTake a pointer to the `struct lock_file` initialized with an\n+\tearlier call to `hold_lock_file_for_update` or\n+\t`hold_lock_file_for_append`, close the file descriptor and\n+\trename the lockfile to its final destination. Return 0 upon\n+\tsuccess or a negative value on failure to `close(2)` or\n+\t`rename(2)`.\n \n rollback_lock_file::\n \n-\tTake a pointer to the `struct lock_file` initialized\n-\twith an earlier call to `hold_lock_file_for_update()`,\n-\tclose the file descriptor and remove the lockfile.\n+\tTake a pointer to the `struct lock_file` initialized with an\n+\tearlier call to `hold_lock_file_for_update` or\n+\t`hold_lock_file_for_append`, close the file descriptor and\n+\tremove the lockfile.\n \n close_lock_file::\n-\tTake a pointer to the `struct lock_file` initialized\n-\twith an earlier call to `hold_lock_file_for_update()`,\n-\tand close the file descriptor. Returns 0 upon success,\n-\ta negative value on failure to close(2).\n-\n-Because the structure is used in an `atexit(3)` handler, its\n-storage has to stay throughout the life of the program. It\n-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-\n-If you need to close the file descriptor you obtained from\n-`hold_lock_file_for_update` function yourself, do so by calling\n-`close_lock_file()`. You should never call `close(2)` yourself!\n-Otherwise the `struct\n-lock_file` structure still remembers that the file descriptor\n-needs to be closed, and a later call to `commit_lock_file()` or\n-`rollback_lock_file()` will result in duplicate calls to\n-`close(2)`. Worse yet, if you `close(2)`, open another file\n-descriptor for completely different purpose, and then call\n-`commit_lock_file()` or `rollback_lock_file()`, they may close\n-that unrelated file descriptor.\n+\n+\tTake a pointer to the `struct lock_file` initialized with an\n+\tearlier call to `hold_lock_file_for_update` or\n+\t`hold_lock_file_for_append`, and close the file descriptor.\n+\tReturn 0 upon success or a negative value on failure to\n+\tclose(2). Usually `commit_lock_file` or `rollback_lock_file`\n+\tshould be called after `close_lock_file`.\n-- \n2.1.0\n"},{"id":"249874","messageId":"1411726119-31598-4-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 03/39] close_lock_file(): exit (successfully) if file is already closed","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:03Z","receivedAt":"2014-09-26T10:08:03Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Suggested-by: Jonathan Nieder \nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 6 +++++-\n read-cache.c | 2 +-\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex f1ce154..d02c3bf 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -233,6 +233,10 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n int close_lock_file(struct lock_file *lk)\n {\n \tint fd = lk->fd;\n+\n+\tif (fd < 0)\n+\t\treturn 0;\n+\n \tlk->fd = -1;\n \treturn close(fd);\n }\n@@ -251,7 +255,7 @@ int commit_lock_file(struct lock_file *lk)\n {\n \tchar result_file[PATH_MAX];\n \tsize_t i;\n-\tif (lk->fd >= 0 && close_lock_file(lk))\n+\tif (close_lock_file(lk))\n \t\treturn -1;\n \tstrcpy(result_file, lk->filename);\n \ti = strlen(result_file) - 5; /* .lock */\ndiff --git a/read-cache.c b/read-cache.c\nindex 2fc1182..5ffb1d7 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2042,7 +2042,7 @@ void set_alternate_index_output(const char *name)\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\tif (close_lock_file(lk))\n \t\t\treturn -1;\n \t\tif (rename(lk->filename, alternate_index_output))\n \t\t\treturn -1;\n-- \n2.1.0\n"},{"id":"249894","messageId":"1411726119-31598-5-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 04/39] rollback_lock_file(): do not clear filename redundantly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:04Z","receivedAt":"2014-09-26T10:08:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is only necessary to clear the lock_file's filename field if it was\nnot already clear.\n\nSigned-off-by: Michael Haggerty \nReviewed-by: Ronnie Sahlberg \nReviewed-by: Jonathan Nieder \n---\n lockfile.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex d02c3bf..5330d6a 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -280,6 +280,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":"249897","messageId":"1411726119-31598-6-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 05/39] rollback_lock_file(): exit early if lock is not active","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:05Z","receivedAt":"2014-09-26T10:08:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Eliminate a layer of nesting.\n\nSigned-off-by: Michael Haggerty \nReviewed-by: Ronnie Sahlberg \nReviewed-by: Jonathan Nieder \n---\n lockfile.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 5330d6a..e55149a 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -276,10 +276,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":"249896","messageId":"1411726119-31598-7-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 06/39] rollback_lock_file(): set fd to -1","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:06Z","receivedAt":"2014-09-26T10:08:06Z","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. It also makes it unnecessary to check whether\nthe file has already been closed, because close_lock_file() takes care\nof that.\n\nSigned-off-by: Michael Haggerty \nReviewed-by: Jonathan Nieder \n---\n lockfile.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex e55149a..3df1e83 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -279,8 +279,7 @@ void rollback_lock_file(struct lock_file *lk)\n \tif (!lk->filename[0])\n \t\treturn;\n \n-\tif (lk->fd >= 0)\n-\t\tclose(lk->fd);\n+\tclose_lock_file(lk);\n \tunlink_or_warn(lk->filename);\n \tlk->filename[0] = 0;\n }\n-- \n2.1.0\n"},{"id":"249895","messageId":"1411726119-31598-8-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 07/39] lockfile: unlock file if lockfile permissions cannot be adjusted","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:07Z","receivedAt":"2014-09-26T10:08:07Z","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 \nReviewed-by: Jonathan Nieder \n---\n lockfile.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 3df1e83..d74de8d 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":"249898","messageId":"1411726119-31598-9-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 08/39] hold_lock_file_for_append(): release lock on errors","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:08Z","receivedAt":"2014-09-26T10:08:08Z","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 \nReviewed-by: Jonathan Nieder \n---\n lockfile.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex d74de8d..f4ce79b 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":"249902","messageId":"1411726119-31598-10-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 09/39] lock_file(): always initialize and register lock_file object","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:09Z","receivedAt":"2014-09-26T10:08:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The purpose of this patch is to make the state diagram for lock_file\nobjects simpler and deterministic.\n\nIf locking fails, lock_file() sometimes leaves the lock_file object\npartly initialized, but sometimes not. It sometimes registers the\nobject in lock_file_list, but sometimes not. This makes the state\ndiagram for lock_file objects effectively indeterministic and hard to\nreason about. A future patch will also change the filename field into\na strbuf, which needs more involved initialization, so it will become\neven more important that the state of a lock_file object is\nwell-defined after a failed attempt to lock.\n\nThe ambiguity doesn't currently have any ill effects, because\nlock_file objects cannot be removed from the lock_file_list anyway.\nBut to make it easier to document and reason about the code, make this\nbehavior inconsistent: *always* initialize the lock_file object and\n*always* register it in lock_file_list the first time it is used,\nregardless of whether an error occurs.\n\nWhile we're at it, make sure that all of the lock_file fields are\ninitialized to values appropriate for an unlocked object; the caller\nis only responsible for making sure that on_list is set to zero before\nthe first time it is used.\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 f4ce79b..81143e5 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->filename[0] = 0;\n+\t\tlk->next = lock_file_list;\n+\t\tlock_file_list = lk;\n+\t\tlk->on_list = 1;\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":"249876","messageId":"1411726119-31598-11-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 10/39] lockfile.c: document the various states of lock_file objects","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:10Z","receivedAt":"2014-09-26T10:08:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Document the valid states of lock_file objects, how they get into each\nstate, and how the state is encoded in the object's fields.\n\nSigned-off-by: Michael Haggerty \nReviewed-by: Ronnie Sahlberg \n---\n lockfile.c | 42 ++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 42 insertions(+)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 81143e5..2680dc9 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -4,6 +4,48 @@\n #include \"cache.h\"\n #include \"sigchain.h\"\n \n+/*\n+ * File write-locks as used by Git.\n+ *\n+ * For an overview of how to use the lockfile API, please see\n+ *\n+ * Documentation/technical/api-lockfile.txt\n+ *\n+ * This module keeps track of all locked files in lock_file_list for\n+ * use at cleanup. This list and the lock_file objects that comprise\n+ * it must be kept in self-consistent states at all time, because the\n+ * program can be interrupted any time by a signal, in which case the\n+ * signal handler will walk through the list attempting to clean up\n+ * any open lock files.\n+ *\n+ * A lockfile is owned by the process that created it. The lock_file\n+ * object has an \"owner\" field that records its owner. This field is\n+ * used to prevent a forked process from closing a lockfile created by\n+ * its parent.\n+ *\n+ * A lock_file object can be in several states:\n+ *\n+ * - Uninitialized. In this state the object's on_list field must be\n+ * zero but the rest of its contents need not be initialized. As\n+ * soon as the object is used in any way, it is irrevocably\n+ * registered in the lock_file_list, and on_list is set.\n+ *\n+ * - Locked, lockfile open (after hold_lock_file_for_update(),\n+ * hold_lock_file_for_append(), or reopen_lock_file()). In this\n+ * state, the lockfile exists, filename holds the filename of the\n+ * lockfile, fd holds a file descriptor open for writing to the\n+ * lockfile, and owner holds the PID of the process that locked the\n+ * file.\n+ *\n+ * - Locked, lockfile closed (after close_lock_file()). Same as the\n+ * previous state, except that the lockfile is closed and fd is -1.\n+ *\n+ * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a\n+ * failed attempt to lock). In this state, filename[0] == '\\0' and\n+ * fd is -1. The object is left registered in the lock_file_list,\n+ * and on_list is set.\n+ */\n+\n static struct lock_file *lock_file_list;\n \n static void remove_lock_file(void)\n-- \n2.1.0\n"},{"id":"249899","messageId":"1411726119-31598-12-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 11/39] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:11Z","receivedAt":"2014-09-26T10:08:11Z","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 07682cb..9f09a89 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -570,6 +570,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);\n #define REFRESH_IN_PORCELAIN\t0x0020\t/* user friendly output, not \"needs update\" */\n extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);\n \n+/* String appended to a filename to derive the lockfile name: */\n+#define LOCK_SUFFIX \".lock\"\n+#define LOCK_SUFFIX_LEN 5\n+\n struct lock_file {\n \tstruct lock_file *next;\n \tint fd;\ndiff --git a/lockfile.c b/lockfile.c\nindex 2680dc9..23847fc 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -166,10 +166,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@@ -194,7 +195,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@@ -308,7 +309,7 @@ int commit_lock_file(struct lock_file *lk)\n \tif (close_lock_file(lk))\n \t\treturn -1;\n \tstrcpy(result_file, lk->filename);\n-\ti = strlen(result_file) - 5; /* .lock */\n+\ti = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */\n \tresult_file[i] = 0;\n \tif (rename(lk->filename, result_file))\n \t\treturn -1;\ndiff --git a/refs.c b/refs.c\nindex 8cb3539..53cd4fb 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -79,7 +79,8 @@ out:\n \t\tif (refname[1] == '\\0')\n \t\t\treturn -1; /* Component equals \".\". */\n \t}\n-\tif (cp - refname >= 5 && !memcmp(cp - 5, \".lock\", 5))\n+\tif (cp - refname >= LOCK_SUFFIX_LEN &&\n+\t !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))\n \t\treturn -1; /* Refname ends with \".lock\". */\n \treturn cp - refname;\n }\n@@ -2551,11 +2552,11 @@ static int delete_ref_loose(struct ref_lock *lock, int flag)\n {\n \tif (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n \t\t/* loose */\n-\t\tint err, i = strlen(lock->lk->filename) - 5; /* .lock */\n+\t\tint err, i = strlen(lock->lk->filename) - LOCK_SUFFIX_LEN;\n \n \t\tlock->lk->filename[i] = 0;\n \t\terr = unlink_or_warn(lock->lk->filename);\n-\t\tlock->lk->filename[i] = '.';\n+\t\tlock->lk->filename[i] = LOCK_SUFFIX[0];\n \t\tif (err && errno != ENOENT)\n \t\t\treturn 1;\n \t}\n-- \n2.1.0\n"},{"id":"249878","messageId":"1411726119-31598-13-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 12/39] delete_ref_loose(): don't muck around in the lock_file's filename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:12Z","receivedAt":"2014-09-26T10:08:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It's bad manners. Especially since there could be a signal during the\ncall to unlink_or_warn(), in which case the signal handler will see\nthe wrong filename and delete the reference file, leaving the lockfile\nbehind.\n\nSo make our own copy to work with.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 53cd4fb..11c76ec 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2551,12 +2551,15 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n static int delete_ref_loose(struct ref_lock *lock, int flag)\n {\n \tif (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n-\t\t/* loose */\n-\t\tint err, i = strlen(lock->lk->filename) - LOCK_SUFFIX_LEN;\n-\n-\t\tlock->lk->filename[i] = 0;\n-\t\terr = unlink_or_warn(lock->lk->filename);\n-\t\tlock->lk->filename[i] = LOCK_SUFFIX[0];\n+\t\t/*\n+\t\t * loose. The loose file name is the same as the\n+\t\t * lockfile name, minus \".lock\":\n+\t\t */\n+\t\tchar *loose_filename = xmemdupz(\n+\t\t\t\tlock->lk->filename,\n+\t\t\t\tstrlen(lock->lk->filename) - LOCK_SUFFIX_LEN);\n+\t\tint err = unlink_or_warn(loose_filename);\n+\t\tfree(loose_filename);\n \t\tif (err && errno != ENOENT)\n \t\t\treturn 1;\n \t}\n-- \n2.1.0\n"},{"id":"249879","messageId":"1411726119-31598-14-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 13/39] prepare_index(): declare return value to be (const char *)","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:13Z","receivedAt":"2014-09-26T10:08:13Z","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 b0fe784..70f5935 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":"249893","messageId":"1411726119-31598-15-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 14/39] write_packed_entry_fn(): convert cb_data into a (const int *)","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:14Z","receivedAt":"2014-09-26T10:08:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This makes it obvious that we have no plans to change the integer\npointed to, which is actually the fd field from a struct lock_file.\n\nSigned-off-by: Michael Haggerty \nReviewed-by: Ronnie Sahlberg \n---\n refs.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 11c76ec..4f313bc 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2217,7 +2217,7 @@ static void write_packed_entry(int fd, char *refname, unsigned char *sha1,\n */\n static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)\n {\n-\tint *fd = cb_data;\n+\tconst int *fd = cb_data;\n \tenum peel_status peel_status = peel_entry(entry, 0);\n \n \tif (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)\n-- \n2.1.0\n"},{"id":"249892","messageId":"1411726119-31598-16-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 15/39] lock_file(): exit early if lockfile cannot be opened","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:15Z","receivedAt":"2014-09-26T10:08:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is a bit easier to read than the old version, which nested part\nof the non-error code in an \"if\" block.\n\nSigned-off-by: Michael Haggerty \nReviewed-by: Ronnie Sahlberg \nReviewed-by: Jonathan Nieder \n---\n lockfile.c | 23 +++++++++++------------\n 1 file changed, 11 insertions(+), 12 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 23847fc..a8f32e5 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -197,19 +197,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":"249900","messageId":"1411726119-31598-17-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 16/39] remove_lock_file(): call rollback_lock_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:16Z","receivedAt":"2014-09-26T10:08:16Z","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 \nReviewed-by: Jonathan Nieder \n---\n lockfile.c | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex a8f32e5..f8205f6 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -53,12 +53,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":"249880","messageId":"1411726119-31598-18-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 17/39] commit_lock_file(): inline temporary variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:17Z","receivedAt":"2014-09-26T10:08:17Z","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 f8205f6..e148227 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -300,12 +300,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 (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":"249904","messageId":"1411726119-31598-19-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 18/39] commit_lock_file(): die() if called for unlocked lockfile object","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:18Z","receivedAt":"2014-09-26T10:08:18Z","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 | 3 ++-\n lockfile.c | 3 +++\n 2 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex c14fca5..0f81e53 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -146,7 +146,8 @@ commit_lock_file::\n \t`hold_lock_file_for_append`, close the file descriptor and\n \trename the lockfile to its final destination. Return 0 upon\n \tsuccess or a negative value on failure to `close(2)` or\n-\t`rename(2)`.\n+\t`rename(2)`. It is a bug to call `commit_lock_file()` for a\n+\t`lock_file` object that is not currently locked.\n \n rollback_lock_file::\n \ndiff --git a/lockfile.c b/lockfile.c\nindex e148227..c897dd8 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -301,6 +301,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 (close_lock_file(lk))\n \t\treturn -1;\n \n-- \n2.1.0\n"},{"id":"249883","messageId":"1411726119-31598-20-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 19/39] close_lock_file(): if close fails, roll back","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:19Z","receivedAt":"2014-09-26T10:08:19Z","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 insures that the lock_file object is left\nin a defined state in this error path (namely, unlocked).\n\nThe only caller that is ultimately affected by this change is\ntry_merge_strategy() -> write_locked_index(), which can call\nclose_lock_file() via various execution paths. This caller uses a\nstatic lock_file object which previously could have been reused after\na failed close_lock_file() even though it was still in locked state.\nThis change causes the lock_file object to be unlocked on failure,\nthus fixing this error-handling path.\n\nSigned-off-by: Michael Haggerty \n---\n Documentation/technical/api-lockfile.txt | 7 ++++---\n lockfile.c | 28 ++++++++++++++++++----------\n 2 files changed, 22 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex 0f81e53..7f04db2 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -161,6 +161,7 @@ close_lock_file::\n \tTake a pointer to the `struct lock_file` initialized with an\n \tearlier call to `hold_lock_file_for_update` or\n \t`hold_lock_file_for_append`, and close the file descriptor.\n-\tReturn 0 upon success or a negative value on failure to\n-\tclose(2). Usually `commit_lock_file` or `rollback_lock_file`\n-\tshould be called after `close_lock_file`.\n+\tReturn 0 upon success. On failure to `close(2)`, return a\n+\tnegative value and rollback the lock file. Usually\n+\t`commit_lock_file` or `rollback_lock_file` should eventually\n+\tbe called if `close_lock_file` succeeds.\ndiff --git a/lockfile.c b/lockfile.c\nindex c897dd8..1d18c67 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -37,13 +37,14 @@\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+ * - Locked, lockfile closed (after successful close_lock_file()).\n+ * Same as the previous state, except that the lockfile is closed\n+ * 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+ * - Unlocked (after commit_lock_file(), rollback_lock_file(), a\n+ * failed attempt to lock, or a failed close_lock_file()). In this\n+ * state, filename[0] == '\\0' and fd is -1. The object is left\n+ * registered in the lock_file_list, and on_list is set.\n */\n \n static struct lock_file *lock_file_list;\n@@ -284,7 +285,13 @@ int close_lock_file(struct lock_file *lk)\n \t\treturn 0;\n \n \tlk->fd = -1;\n-\treturn close(fd);\n+\tif (close(fd)) {\n+\t\tint save_errno = errno;\n+\t\trollback_lock_file(lk);\n+\t\terrno = save_errno;\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n }\n \n int reopen_lock_file(struct lock_file *lk)\n@@ -330,7 +337,8 @@ void rollback_lock_file(struct lock_file *lk)\n \tif (!lk->filename[0])\n \t\treturn;\n \n-\tclose_lock_file(lk);\n-\tunlink_or_warn(lk->filename);\n-\tlk->filename[0] = 0;\n+\tif (!close_lock_file(lk)) {\n+\t\tunlink_or_warn(lk->filename);\n+\t\tlk->filename[0] = 0;\n+\t}\n }\n-- \n2.1.0\n"},{"id":"249901","messageId":"1411726119-31598-21-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 20/39] commit_lock_file(): rollback lock file on failure to rename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:20Z","receivedAt":"2014-09-26T10:08:20Z","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 | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 1d18c67..728ce49 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -318,8 +318,13 @@ int commit_lock_file(struct lock_file *lk)\n \t/* remove \".lock\": */\n \tresult_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;\n \n-\tif (rename(lk->filename, result_file))\n+\tif (rename(lk->filename, result_file)) {\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 \tlk->filename[0] = 0;\n \treturn 0;\n }\n-- \n2.1.0\n"},{"id":"249890","messageId":"1411726119-31598-22-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 21/39] api-lockfile: document edge cases","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:21Z","receivedAt":"2014-09-26T10:08:21Z","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 | 20 ++++++++++++++------\n 1 file changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex 7f04db2..d7882df 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -99,6 +99,10 @@ unable_to_lock_die::\n \n \tEmit an appropriate error message and `die()`.\n \n+Similarly, `commit_lock_file` and `close_lock_file` return 0 on\n+success. On failure they set `errno` appropriately, do their best to\n+roll back the lockfile, and return -1.\n+\n \n Flags\n -----\n@@ -143,18 +147,22 @@ commit_lock_file::\n \n \tTake a pointer to the `struct lock_file` initialized with an\n \tearlier call to `hold_lock_file_for_update` or\n-\t`hold_lock_file_for_append`, close the file descriptor and\n+\t`hold_lock_file_for_append`, close the file descriptor, and\n \trename the lockfile to its final destination. Return 0 upon\n-\tsuccess or a negative value on failure to `close(2)` or\n-\t`rename(2)`. It is a bug to call `commit_lock_file()` for a\n-\t`lock_file` object that is not currently locked.\n+\tsuccess. On failure, roll back the lock file and return -1,\n+\twith `errno` set to the value from the failing call to\n+\t`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 with an\n \tearlier call to `hold_lock_file_for_update` or\n \t`hold_lock_file_for_append`, close the file descriptor and\n-\tremove the lockfile.\n+\tremove the lockfile. It is a NOOP to call\n+\t`rollback_lock_file()` for a `lock_file` object that has\n+\talready been committed or rolled back.\n \n close_lock_file::\n \n@@ -162,6 +170,6 @@ close_lock_file::\n \tearlier call to `hold_lock_file_for_update` or\n \t`hold_lock_file_for_append`, and close the file descriptor.\n \tReturn 0 upon success. On failure to `close(2)`, return a\n-\tnegative value and rollback the lock file. Usually\n+\tnegative value and roll back the lock file. Usually\n \t`commit_lock_file` or `rollback_lock_file` should eventually\n \tbe called if `close_lock_file` succeeds.\n-- \n2.1.0\n"},{"id":"249881","messageId":"1411726119-31598-23-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 22/39] dump_marks(): remove a redundant call to rollback_lock_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:22Z","receivedAt":"2014-09-26T10:08:22Z","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 \nReviewed-by: Jonathan Nieder \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 487f1f8..61ac7e0 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1832,10 +1832,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":"249903","messageId":"1411726119-31598-24-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 23/39] git_config_set_multivar_in_file(): avoid call to rollback_lock_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:23Z","receivedAt":"2014-09-26T10:08:23Z","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 | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/config.c b/config.c\nindex a677eb6..123ed29 100644\n--- a/config.c\n+++ b/config.c\n@@ -2083,6 +2083,7 @@ 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\tlock = NULL;\n \t\tgoto out_free;\n \t}\n \n-- \n2.1.0\n"},{"id":"249884","messageId":"1411726119-31598-25-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 24/39] lockfile: avoid transitory invalid states","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:24Z","receivedAt":"2014-09-26T10:08:24Z","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 | 37 ++++++++++++++++++++++++++-----------\n read-cache.c | 1 +\n 3 files changed, 28 insertions(+), 11 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 9f09a89..81c70c7 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -576,6 +576,7 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct\n \n struct lock_file {\n \tstruct lock_file *next;\n+\tvolatile sig_atomic_t active;\n \tint fd;\n \tpid_t owner;\n \tchar on_list;\ndiff --git a/lockfile.c b/lockfile.c\nindex 728ce49..d35ac44 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -23,7 +23,7 @@\n * used to prevent a forked process from closing a lockfile created by\n * its parent.\n *\n- * A lock_file object can be in several states:\n+ * The possible states of a lock_file object 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@@ -32,19 +32,27 @@\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+ * 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 successful close_lock_file()).\n * Same as the previous state, except that the lockfile is closed\n * and fd is -1.\n *\n * - Unlocked (after commit_lock_file(), rollback_lock_file(), a\n- * failed attempt to lock, or a failed close_lock_file()). In this\n- * state, filename[0] == '\\0' and fd is -1. The object is left\n- * registered in the lock_file_list, and on_list is set.\n+ * failed attempt to lock, or a failed close_lock_file()). In this\n+ * state:\n+ * - active is unset\n+ * - filename[0] == '\\0' (usually, though there are transitory states\n+ * in which this condition doesn't hold). Client code should *not*\n+ * rely on this fact!\n+ * - fd is -1\n+ * - the object is left registered in the lock_file_list, and\n+ * on_list is set.\n */\n \n static struct lock_file *lock_file_list;\n@@ -175,9 +183,13 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\tatexit(remove_lock_file);\n \t}\n \n+\tif (lk->active)\n+\t\tdie(\"BUG: cannot lock_file(\\\"%s\\\") using active struct lock_file\",\n+\t\t path);\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->filename[0] = 0;\n \t\tlk->next = lock_file_list;\n@@ -199,6 +211,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@@ -298,7 +311,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 {\n \tchar result_file[PATH_MAX];\n \n-\tif (!lk->filename[0])\n+\tif (!lk->active)\n \t\tdie(\"BUG: attempt to commit unlocked object\");\n \n \tif (close_lock_file(lk))\n@@ -325,6 +338,7 @@ int commit_lock_file(struct lock_file *lk)\n \t\treturn -1;\n \t}\n \n+\tlk->active = 0;\n \tlk->filename[0] = 0;\n \treturn 0;\n }\n@@ -339,11 +353,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 (!close_lock_file(lk)) {\n \t\tunlink_or_warn(lk->filename);\n+\t\tlk->active = 0;\n \t\tlk->filename[0] = 0;\n \t}\n }\ndiff --git a/read-cache.c b/read-cache.c\nindex 5ffb1d7..af69f34 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2046,6 +2046,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":"249908","messageId":"1411726119-31598-26-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 25/39] struct lock_file: declare some fields volatile","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:25Z","receivedAt":"2014-09-26T10:08:25Z","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 81c70c7..0e55bbe 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -575,10 +575,10 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct\n #define LOCK_SUFFIX_LEN 5\n \n struct lock_file {\n-\tstruct lock_file *next;\n+\tstruct lock_file *volatile next;\n \tvolatile sig_atomic_t active;\n-\tint fd;\n-\tpid_t owner;\n+\tvolatile int fd;\n+\tvolatile pid_t owner;\n \tchar on_list;\n \tchar filename[PATH_MAX];\n };\ndiff --git a/lockfile.c b/lockfile.c\nindex d35ac44..89043f5 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -55,7 +55,7 @@\n * on_list is set.\n */\n \n-static struct lock_file *lock_file_list;\n+static struct lock_file *volatile lock_file_list;\n \n static void remove_lock_file(void)\n {\ndiff --git a/refs.c b/refs.c\nindex 4f313bc..9971ac5 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2259,15 +2259,16 @@ int commit_packed_refs(void)\n \t\tget_packed_ref_cache(&ref_cache);\n \tint error = 0;\n \tint save_errno = 0;\n+\tint fd;\n \n \tif (!packed_ref_cache->lock)\n \t\tdie(\"internal error: packed-refs not locked\");\n \twrite_or_die(packed_ref_cache->lock->fd,\n \t\t PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));\n \n+\tfd = packed_ref_cache->lock->fd;\n \tdo_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),\n-\t\t\t\t 0, write_packed_entry_fn,\n-\t\t\t\t &packed_ref_cache->lock->fd);\n+\t\t\t\t 0, write_packed_entry_fn, &fd);\n \tif (commit_lock_file(packed_ref_cache->lock)) {\n \t\tsave_errno = errno;\n \t\terror = -1;\n-- \n2.1.0\n"},{"id":"249907","messageId":"1411726119-31598-27-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 26/39] try_merge_strategy(): remove redundant lock_file allocation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:26Z","receivedAt":"2014-09-26T10:08:26Z","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 ec6fa93..cb8f709 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -668,7 +668,6 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \tif (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\")) {\n \t\tint clean, x;\n \t\tstruct commit *result;\n-\t\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n \t\tstruct commit_list *reversed = NULL;\n \t\tstruct merge_options o;\n \t\tstruct commit_list *j;\n-- \n2.1.0\n"},{"id":"249882","messageId":"1411726119-31598-28-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 27/39] try_merge_strategy(): use a statically-allocated lock_file object","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:27Z","receivedAt":"2014-09-26T10:08:27Z","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 cb8f709..c7fa3bc 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -656,14 +656,14 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\t\t struct commit_list *remoteheads,\n \t\t\t struct commit *head, const char *head_arg)\n {\n-\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n+\tstatic struct lock_file lock;\n \n-\thold_locked_index(lock, 1);\n+\thold_locked_index(&lock, 1);\n \trefresh_cache(REFRESH_QUIET);\n \tif (active_cache_changed &&\n-\t write_locked_index(&the_index, lock, COMMIT_LOCK))\n+\t write_locked_index(&the_index, &lock, COMMIT_LOCK))\n \t\treturn error(_(\"Unable to write index.\"));\n-\trollback_lock_file(lock);\n+\trollback_lock_file(&lock);\n \n \tif (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\")) {\n \t\tint clean, x;\n@@ -695,13 +695,13 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\tfor (j = common; j; j = j->next)\n \t\t\tcommit_list_insert(j->item, &reversed);\n \n-\t\thold_locked_index(lock, 1);\n+\t\thold_locked_index(&lock, 1);\n \t\tclean = merge_recursive(&o, head,\n \t\t\t\tremoteheads->item, reversed, &result);\n \t\tif (active_cache_changed &&\n-\t\t write_locked_index(&the_index, lock, COMMIT_LOCK))\n+\t\t write_locked_index(&the_index, &lock, COMMIT_LOCK))\n \t\t\tdie (_(\"unable to write %s\"), get_index_file());\n-\t\trollback_lock_file(lock);\n+\t\trollback_lock_file(&lock);\n \t\treturn clean ? 0 : 1;\n \t} else {\n \t\treturn try_merge_command(strategy, xopts_nr, xopts,\n-- \n2.1.0\n"},{"id":"249885","messageId":"1411726119-31598-29-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 28/39] commit_lock_file(): use a strbuf to manage temporary space","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:28Z","receivedAt":"2014-09-26T10:08:28Z","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 | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 89043f5..1dd118f 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -319,7 +319,8 @@ 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 err;\n \n \tif (!lk->active)\n \t\tdie(\"BUG: attempt to commit unlocked object\");\n@@ -327,11 +328,12 @@ int commit_lock_file(struct lock_file *lk)\n \tif (close_lock_file(lk))\n \t\treturn -1;\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\tint save_errno = errno;\n \t\trollback_lock_file(lk);\n \t\terrno = save_errno;\n-- \n2.1.0\n"},{"id":"249905","messageId":"1411726119-31598-30-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 29/39] Change lock_file::filename into a strbuf","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:29Z","receivedAt":"2014-09-26T10:08:29Z","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 | 53 ++++++++++++++++++++++++-----------------------------\n read-cache.c | 4 ++--\n refs.c | 6 +++---\n shallow.c | 6 +++---\n 8 files changed, 47 insertions(+), 52 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 70f5935..f55e809 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -341,7 +341,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\t\tdie(_(\"unable to create temporary index\"));\n \n \t\told_index_env = getenv(INDEX_ENVIRONMENT);\n-\t\tsetenv(INDEX_ENVIRONMENT, index_lock.filename, 1);\n+\t\tsetenv(INDEX_ENVIRONMENT, index_lock.filename.buf, 1);\n \n \t\tif (interactive_add(argc, argv, prefix, patch_interactive) != 0)\n \t\t\tdie(_(\"interactive add failed\"));\n@@ -352,7 +352,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\t\tunsetenv(INDEX_ENVIRONMENT);\n \n \t\tdiscard_cache();\n-\t\tread_cache_from(index_lock.filename);\n+\t\tread_cache_from(index_lock.filename.buf);\n \t\tif (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {\n \t\t\tif (reopen_lock_file(&index_lock) < 0)\n \t\t\t\tdie(_(\"unable to write index file\"));\n@@ -362,7 +362,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\t\twarning(_(\"Failed to update main cache tree\"));\n \n \t\tcommit_style = COMMIT_NORMAL;\n-\t\treturn index_lock.filename;\n+\t\treturn index_lock.filename.buf;\n \t}\n \n \t/*\n@@ -385,7 +385,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\tif (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\n \t\t\tdie(_(\"unable to write new_index file\"));\n \t\tcommit_style = COMMIT_NORMAL;\n-\t\treturn index_lock.filename;\n+\t\treturn index_lock.filename.buf;\n \t}\n \n \t/*\n@@ -472,9 +472,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\tdie(_(\"unable to write temporary index file\"));\n \n \tdiscard_cache();\n-\tread_cache_from(false_lock.filename);\n+\tread_cache_from(false_lock.filename.buf);\n \n-\treturn false_lock.filename;\n+\treturn false_lock.filename.buf;\n }\n \n static int run_status(FILE *fp, const char *index_file, const char *prefix, int nowarn,\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex e8a8fb1..7c78b15 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -431,7 +431,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\n \t\t\t write_str_in_full(lock->lock_fd, \"\\n\") != 1 ||\n \t\t\t close_ref(lock) < 0)) {\n \t\t\tstatus |= error(\"Couldn't write %s\",\n-\t\t\t\tlock->lk->filename);\n+\t\t\t\t\tlock->lk->filename.buf);\n \t\t\tunlink(newlog_path);\n \t\t} else if (rename(newlog_path, log_file)) {\n \t\t\tstatus |= error(\"cannot rename %s to %s\",\ndiff --git a/cache.h b/cache.h\nindex 0e55bbe..433fae5 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -580,7 +580,7 @@ struct lock_file {\n \tvolatile int fd;\n \tvolatile pid_t owner;\n \tchar on_list;\n-\tchar filename[PATH_MAX];\n+\tstruct strbuf filename;\n };\n #define LOCK_DIE_ON_ERROR 1\n #define LOCK_NODEREF 2\ndiff --git a/config.c b/config.c\nindex 123ed29..2110779 100644\n--- a/config.c\n+++ b/config.c\n@@ -2024,9 +2024,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@@ -2106,7 +2106,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@@ -2207,9 +2207,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@@ -2230,7 +2230,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@@ -2256,7 +2256,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 1dd118f..85c8648 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -47,9 +47,9 @@\n * failed attempt to lock, or a failed close_lock_file()). In this\n * state:\n * - active is unset\n- * - filename[0] == '\\0' (usually, though there are transitory states\n- * in which this condition doesn't hold). Client code should *not*\n- * rely on this fact!\n+ * - filename is empty (usually, though there are transitory\n+ * states in which this condition doesn't hold). Client code should\n+ * *not* rely on the filename being empty in this state.\n * - fd is -1\n * - the object is left registered in the lock_file_list, and\n * on_list is set.\n@@ -170,13 +170,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@@ -191,30 +184,32 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\tlk->fd = -1;\n \t\tlk->active = 0;\n \t\tlk->owner = 0;\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\tlk->on_list = 1;\n+\t} else if (lk->filename.len) {\n+\t\t/* This shouldn't happen, but better safe than sorry. */\n+\t\tdie(\"BUG: lock_file(\\\"%s\\\") called with improperly-reset lock_file object\",\n+\t\t path);\n \t}\n \n-\tif (strlen(path) >= max_path_len) {\n-\t\terrno = ENAMETOOLONG;\n-\t\treturn -1;\n+\tstrbuf_addstr(&lk->filename, path);\n+\tif (!(flags & LOCK_NODEREF)) {\n+\t\tresolve_symlink(lk->filename.buf, lk->filename.alloc);\n+\t\tstrbuf_setlen(&lk->filename, strlen(lk->filename.buf));\n \t}\n-\tstrcpy(lk->filename, path);\n-\tif (!(flags & LOCK_NODEREF))\n-\t\tresolve_symlink(lk->filename, max_path_len);\n-\tstrcat(lk->filename, LOCK_SUFFIX);\n-\tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n+\tstrbuf_addstr(&lk->filename, LOCK_SUFFIX);\n+\tlk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (lk->fd < 0) {\n-\t\tlk->filename[0] = 0;\n+\t\tstrbuf_reset(&lk->filename);\n \t\treturn -1;\n \t}\n \tlk->owner = getpid();\n \tlk->active = 1;\n-\tif (adjust_shared_perm(lk->filename)) {\n+\tif (adjust_shared_perm(lk->filename.buf)) {\n \t\tint save_errno = errno;\n-\t\terror(\"cannot fix permission bits on %s\", lk->filename);\n+\t\terror(\"cannot fix permission bits on %s\", lk->filename.buf);\n \t\trollback_lock_file(lk);\n \t\terrno = save_errno;\n \t\treturn -1;\n@@ -313,7 +308,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,9 +324,9 @@ int commit_lock_file(struct lock_file *lk)\n \t\treturn -1;\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\tint save_errno = errno;\n@@ -341,7 +336,7 @@ int commit_lock_file(struct lock_file *lk)\n \t}\n \n \tlk->active = 0;\n-\tlk->filename[0] = 0;\n+\tstrbuf_reset(&lk->filename);\n \treturn 0;\n }\n \n@@ -359,8 +354,8 @@ void rollback_lock_file(struct lock_file *lk)\n \t\treturn;\n \n \tif (!close_lock_file(lk)) {\n-\t\tunlink_or_warn(lk->filename);\n+\t\tunlink_or_warn(lk->filename.buf);\n \t\tlk->active = 0;\n-\t\tlk->filename[0] = 0;\n+\t\tstrbuf_reset(&lk->filename);\n \t}\n }\ndiff --git a/read-cache.c b/read-cache.c\nindex af69f34..91bf876 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2044,10 +2044,10 @@ static int commit_locked_index(struct lock_file *lk)\n \tif (alternate_index_output) {\n \t\tif (close_lock_file(lk))\n \t\t\treturn -1;\n-\t\tif (rename(lk->filename, alternate_index_output))\n+\t\tif (rename(lk->filename.buf, alternate_index_output))\n \t\t\treturn -1;\n \t\tlk->active = 0;\n-\t\tlk->filename[0] = 0;\n+\t\tstrbuf_reset(&lk->filename);\n \t\treturn 0;\n \t} else {\n \t\treturn commit_lock_file(lk);\ndiff --git a/refs.c b/refs.c\nindex 9971ac5..c6e15f9a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2557,8 +2557,8 @@ static int delete_ref_loose(struct ref_lock *lock, int flag)\n \t\t * lockfile name, minus \".lock\":\n \t\t */\n \t\tchar *loose_filename = xmemdupz(\n-\t\t\t\tlock->lk->filename,\n-\t\t\t\tstrlen(lock->lk->filename) - LOCK_SUFFIX_LEN);\n+\t\t\t\tlock->lk->filename.buf,\n+\t\t\t\tlock->lk->filename.len - LOCK_SUFFIX_LEN);\n \t\tint err = unlink_or_warn(loose_filename);\n \t\tfree(loose_filename);\n \t\tif (err && errno != ENOENT)\n@@ -2922,7 +2922,7 @@ int write_ref_sha1(struct ref_lock *lock,\n \t write_in_full(lock->lock_fd, &term, 1) != 1 ||\n \t close_ref(lock) < 0) {\n \t\tint save_errno = errno;\n-\t\terror(\"Couldn't write %s\", lock->lk->filename);\n+\t\terror(\"Couldn't write %s\", lock->lk->filename.buf);\n \t\tunlock_ref(lock);\n \t\terrno = save_errno;\n \t\treturn -1;\ndiff --git a/shallow.c b/shallow.c\nindex de07709..33f3c7f 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -269,8 +269,8 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,\n \tif (write_shallow_commits(&sb, 0, extra)) {\n \t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n \t\t\tdie_errno(\"failed to write to %s\",\n-\t\t\t\t shallow_lock->filename);\n-\t\t*alternate_shallow_file = shallow_lock->filename;\n+\t\t\t\t shallow_lock->filename.buf);\n+\t\t*alternate_shallow_file = shallow_lock->filename.buf;\n \t} else\n \t\t/*\n \t\t * is_repository_shallow() sees empty string as \"no\n@@ -316,7 +316,7 @@ void prune_shallow(int show_only)\n \tif (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {\n \t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n \t\t\tdie_errno(\"failed to write to %s\",\n-\t\t\t\t shallow_lock.filename);\n+\t\t\t\t shallow_lock.filename.buf);\n \t\tcommit_lock_file(&shallow_lock);\n \t} else {\n \t\tunlink(git_path(\"shallow\"));\n-- \n2.1.0\n"},{"id":"249910","messageId":"1411726119-31598-31-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 30/39] resolve_symlink(): use a strbuf for internal scratch space","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:30Z","receivedAt":"2014-09-26T10:08:30Z","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 85c8648..cc9b9cb 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -126,44 +126,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":"249906","messageId":"1411726119-31598-32-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 31/39] resolve_symlink(): take a strbuf parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:31Z","receivedAt":"2014-09-26T10:08:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change resolve_symlink() to take a strbuf rather than a string as\nparameter. This simplifies the code and removes an arbitrary pathname\nlength restriction. It also means that lock_file's filename field no\nlonger needs to be initialized to a large size.\n\nHelped-by: Torsten Bögershausen \nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 57 ++++++++++++++++++++++-----------------------------------\n 1 file changed, 22 insertions(+), 35 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex cc9b9cb..5f5bcff 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -109,58 +109,47 @@ static char *last_path_elm(char *p)\n #define MAXDEPTH 5\n \n /*\n- * p = path that may be a symlink\n- * s = full size of p\n+ * path contains a path that might be a symlink.\n *\n- * If p is a symlink, attempt to overwrite p with a path to the real\n- * file or directory (which may or may not exist), following a chain of\n- * symlinks if necessary. Otherwise, leave p unmodified.\n+ * If path is a symlink, attempt to overwrite it with a path to the\n+ * real file or directory (which may or may not exist), following a\n+ * chain of symlinks if necessary. Otherwise, leave path unmodified.\n *\n- * This is a best-effort routine. If an error occurs, p will either be\n- * left unmodified or will name a different symlink in a symlink chain\n- * that started with p's initial contents.\n- *\n- * Always returns p.\n+ * This is a best-effort routine. If an error occurs, path will\n+ * either be left unmodified or will name a different symlink in a\n+ * symlink chain that started with the original path.\n */\n-\n-static char *resolve_symlink(char *p, size_t s)\n+static void resolve_symlink(struct strbuf *path)\n {\n \tint depth = MAXDEPTH;\n \tstatic struct strbuf link = STRBUF_INIT;\n \n \twhile (depth--) {\n-\t\tif (strbuf_readlink(&link, p, strlen(p)) < 0)\n+\t\tif (strbuf_readlink(&link, path->buf, path->len) < 0)\n \t\t\tbreak;\n \n-\t\tif (is_absolute_path(link.buf)) {\n+\t\tif (is_absolute_path(link.buf))\n \t\t\t/* absolute path simply replaces p */\n-\t\t\tif (link.len < s)\n-\t\t\t\tstrcpy(p, link.buf);\n-\t\t\telse {\n-\t\t\t\twarning(\"%s: symlink too long\", p);\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t} else {\n+\t\t\tstrbuf_reset(path);\n+\t\telse {\n \t\t\t/*\n \t\t\t * link is a relative path, so replace the\n \t\t\t * last element of p with it.\n \t\t\t */\n-\t\t\tchar *r = (char *)last_path_elm(p);\n-\t\t\tif (r - p + link.len < s)\n-\t\t\t\tstrcpy(r, link.buf);\n-\t\t\telse {\n-\t\t\t\twarning(\"%s: symlink too long\", p);\n-\t\t\t\tbreak;\n-\t\t\t}\n+\t\t\tchar *r = last_path_elm(path->buf);\n+\t\t\tstrbuf_setlen(path, r - path->buf);\n \t\t}\n+\n+\t\tstrbuf_addbuf(path, &link);\n \t}\n \tstrbuf_reset(&link);\n-\treturn p;\n }\n \n /* Make sure errno contains a meaningful value on error */\n static int lock_file(struct lock_file *lk, const char *path, int flags)\n {\n+\tsize_t pathlen = strlen(path);\n+\n \tif (!lock_file_list) {\n \t\t/* One-time initialization */\n \t\tsigchain_push_common(remove_lock_file_on_signal);\n@@ -175,7 +164,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\tlk->fd = -1;\n \t\tlk->active = 0;\n \t\tlk->owner = 0;\n-\t\tstrbuf_init(&lk->filename, PATH_MAX);\n+\t\tstrbuf_init(&lk->filename, pathlen + LOCK_SUFFIX_LEN);\n \t\tlk->next = lock_file_list;\n \t\tlock_file_list = lk;\n \t\tlk->on_list = 1;\n@@ -185,11 +174,9 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\t path);\n \t}\n \n-\tstrbuf_addstr(&lk->filename, path);\n-\tif (!(flags & LOCK_NODEREF)) {\n-\t\tresolve_symlink(lk->filename.buf, lk->filename.alloc);\n-\t\tstrbuf_setlen(&lk->filename, strlen(lk->filename.buf));\n-\t}\n+\tstrbuf_add(&lk->filename, path, pathlen);\n+\tif (!(flags & LOCK_NODEREF))\n+\t\tresolve_symlink(&lk->filename);\n \tstrbuf_addstr(&lk->filename, LOCK_SUFFIX);\n \tlk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (lk->fd < 0) {\n-- \n2.1.0\n"},{"id":"249888","messageId":"1411726119-31598-33-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 32/39] trim_last_path_component(): replace last_path_elm()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:32Z","receivedAt":"2014-09-26T10:08:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Rewrite last_path_elm() to take a strbuf parameter and to trim off the\nlast path name element in place rather than returning a pointer to the\nbeginning of the last path name element. This simplifies the function\na bit and makes it integrate better with its caller, which is now also\nstrbuf-based. Rename the function accordingly and a bit less tersely.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 38 ++++++++++++++++----------------------\n 1 file changed, 16 insertions(+), 22 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 5f5bcff..56ad7e8 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -76,32 +76,28 @@ static void remove_lock_file_on_signal(int signo)\n }\n \n /*\n- * p = absolute or relative path name\n+ * path = absolute or relative path name\n *\n- * Return a pointer into p showing the beginning of the last path name\n- * element. If p is empty or the root directory (\"/\"), just return p.\n+ * Remove the last path name element from path (leaving the preceding\n+ * \"/\", if any). If path is empty or the root directory (\"/\"), set\n+ * path to the empty string.\n */\n-static char *last_path_elm(char *p)\n+static void trim_last_path_component(struct strbuf *path)\n {\n-\t/* r starts pointing to null at the end of the string */\n-\tchar *r = strchr(p, '\\0');\n-\n-\tif (r == p)\n-\t\treturn p; /* just return empty string */\n-\n-\tr--; /* back up to last non-null character */\n+\tint i = path->len;\n \n \t/* back up past trailing slashes, if any */\n-\twhile (r > p && *r == '/')\n-\t\tr--;\n+\twhile (i && path->buf[i - 1] == '/')\n+\t\ti--;\n \n \t/*\n-\t * then go backwards until I hit a slash, or the beginning of\n-\t * the string\n+\t * then go backwards until a slash, or the beginning of the\n+\t * string\n \t */\n-\twhile (r > p && *(r-1) != '/')\n-\t\tr--;\n-\treturn r;\n+\twhile (i && path->buf[i - 1] != '/')\n+\t\ti--;\n+\n+\tstrbuf_setlen(path, i);\n }\n \n \n@@ -131,14 +127,12 @@ static void resolve_symlink(struct strbuf *path)\n \t\tif (is_absolute_path(link.buf))\n \t\t\t/* absolute path simply replaces p */\n \t\t\tstrbuf_reset(path);\n-\t\telse {\n+\t\telse\n \t\t\t/*\n \t\t\t * link is a relative path, so replace the\n \t\t\t * last element of p with it.\n \t\t\t */\n-\t\t\tchar *r = last_path_elm(path->buf);\n-\t\t\tstrbuf_setlen(path, r - path->buf);\n-\t\t}\n+\t\t\ttrim_last_path_component(path);\n \n \t\tstrbuf_addbuf(path, &link);\n \t}\n-- \n2.1.0\n"},{"id":"249909","messageId":"1411726119-31598-34-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 33/39] Extract a function commit_lock_file_to()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:33Z","receivedAt":"2014-09-26T10:08:33Z","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 | 36 ++++++++++++++++------------\n cache.h | 1 +\n lockfile.c | 40 +++++++++++++++++++++-----------\n read-cache.c | 13 +++--------\n 4 files changed, 51 insertions(+), 39 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex d7882df..0b8db64 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -48,14 +48,14 @@ The caller:\n When finished writing, the caller can:\n \n * Close the file descriptor and rename the lockfile to its final\n- destination by calling `commit_lock_file`.\n+ destination by calling `commit_lock_file` or `commit_lock_file_to`.\n \n * Close the file descriptor and remove the lockfile by calling\n `rollback_lock_file`.\n \n * Close the file descriptor without removing or renaming the lockfile\n- by calling `close_lock_file`, and later call `commit_lock_file` or\n- `rollback_lock_file`.\n+ by calling `close_lock_file`, and later call `commit_lock_file`,\n+ `commit_lock_file_to`, or `rollback_lock_file`.\n \n At this point, the `lock_file` object must not be freed or altered,\n because it is still registered in the `lock_file_list`. However, it\n@@ -63,20 +63,19 @@ may be reused; just pass it to another call of\n `hold_lock_file_for_update` or `hold_lock_file_for_append`.\n \n If the program exits before you have called one of `commit_lock_file`,\n-`rollback_lock_file`, or `close_lock_file`, an `atexit(3)` handler\n-will close and remove the lockfile, essentially rolling back any\n-uncommitted changes.\n+`commit_lock_file_to`, `rollback_lock_file`, or `close_lock_file`, an\n+`atexit(3)` handler will close and remove the lockfile, essentially\n+rolling back any uncommitted changes.\n \n If you need to close the file descriptor you obtained from a\n `hold_lock_file_*` function yourself, do so by calling\n `close_lock_file`. You should never call `close(2)` yourself!\n Otherwise the `struct lock_file` structure would still think that the\n-file descriptor needs to be closed, and a later call to\n-`commit_lock_file` or `rollback_lock_file` or program exit would\n+file descriptor needs to be closed, and a commit or rollback would\n result in duplicate calls to `close(2)`. Worse yet, if you `close(2)`\n and then later open another file descriptor for a completely different\n-purpose, then a call to `commit_lock_file` or `rollback_lock_file`\n-might close that unrelated file descriptor.\n+purpose, then a commit or rollback might close that unrelated file\n+descriptor.\n \n \n Error handling\n@@ -99,9 +98,9 @@ unable_to_lock_die::\n \n \tEmit an appropriate error message and `die()`.\n \n-Similarly, `commit_lock_file` and `close_lock_file` return 0 on\n-success. On failure they set `errno` appropriately, do their best to\n-roll back the lockfile, and return -1.\n+Similarly, `commit_lock_file`, `commit_lock_file_to`, and\n+`close_lock_file` return 0 on success. On failure they set `errno`\n+appropriately, do their best to roll back the lockfile, and return -1.\n \n \n Flags\n@@ -155,6 +154,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 with an\n@@ -171,5 +176,6 @@ close_lock_file::\n \t`hold_lock_file_for_append`, and close the file descriptor.\n \tReturn 0 upon success. On failure to `close(2)`, return a\n \tnegative value and roll back the lock file. Usually\n-\t`commit_lock_file` or `rollback_lock_file` should eventually\n-\tbe called if `close_lock_file` succeeds.\n+\t`commit_lock_file`, `commit_lock_file_to`, or\n+\t`rollback_lock_file` should eventually be called if\n+\t`close_lock_file` succeeds.\ndiff --git a/cache.h b/cache.h\nindex 433fae5..30883b3 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -590,6 +590,7 @@ extern void unable_to_lock_message(const char *path, int err,\n extern NORETURN void unable_to_lock_die(const char *path, int err);\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n+extern int commit_lock_file_to(struct lock_file *, const char *path);\n extern int commit_lock_file(struct lock_file *);\n extern int reopen_lock_file(struct lock_file *);\n extern void update_index_if_able(struct index_state *, struct lock_file *);\ndiff --git a/lockfile.c b/lockfile.c\nindex 56ad7e8..cf7f4d0 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -43,9 +43,9 @@\n * Same as the previous state, except that the lockfile is closed\n * and fd is -1.\n *\n- * - Unlocked (after commit_lock_file(), rollback_lock_file(), a\n- * failed attempt to lock, or a failed close_lock_file()). In this\n- * state:\n+ * - Unlocked (after commit_lock_file(), commit_lock_file_to(),\n+ * rollback_lock_file(), a failed attempt to lock, or a failed\n+ * close_lock_file()). In this state:\n * - active is unset\n * - filename is empty (usually, though there are transitory\n * states in which this condition doesn't hold). Client code should\n@@ -284,23 +284,15 @@ 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 err;\n-\n \tif (!lk->active)\n-\t\tdie(\"BUG: attempt to commit unlocked object\");\n+\t\tdie(\"BUG: attempt to commit unlocked object to \\\"%s\\\"\", path);\n \n \tif (close_lock_file(lk))\n \t\treturn -1;\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\tint save_errno = errno;\n \t\trollback_lock_file(lk);\n \t\terrno = save_errno;\n@@ -312,6 +304,26 @@ int commit_lock_file(struct lock_file *lk)\n \treturn 0;\n }\n \n+int commit_lock_file(struct lock_file *lk)\n+{\n+\tstatic struct strbuf result_file = STRBUF_INIT;\n+\tint err;\n+\n+\tif (!lk->active)\n+\t\tdie(\"BUG: attempt to commit unlocked object\");\n+\n+\tif (lk->filename.len <= LOCK_SUFFIX_LEN ||\n+\t strcmp(lk->filename.buf + lk->filename.len - LOCK_SUFFIX_LEN, LOCK_SUFFIX))\n+\t\tdie(\"BUG: lockfile filename corrupt\");\n+\n+\t/* remove \".lock\": */\n+\tstrbuf_add(&result_file, lk->filename.buf,\n+\t\t lk->filename.len - LOCK_SUFFIX_LEN);\n+\terr = commit_lock_file_to(lk, result_file.buf);\n+\tstrbuf_reset(&result_file);\n+\treturn err;\n+}\n+\n int hold_locked_index(struct lock_file *lk, int die_on_error)\n {\n \treturn hold_lock_file_for_update(lk, get_index_file(),\ndiff --git a/read-cache.c b/read-cache.c\nindex 91bf876..e887e23 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2041,17 +2041,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 (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":"249912","messageId":"1411726119-31598-35-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 34/39] Rename LOCK_NODEREF to LOCK_NO_DEREF","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:34Z","receivedAt":"2014-09-26T10:08:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This makes it harder to misread the name as LOCK_NODE_REF.\n\nSuggested-by: Torsten Bögershausen \nSigned-off-by: Michael Haggerty \n---\n Documentation/technical/api-lockfile.txt | 4 ++--\n cache.h | 2 +-\n lockfile.c | 2 +-\n refs.c | 2 +-\n 4 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex 0b8db64..64b250f 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -109,11 +109,11 @@ Flags\n The following flags can be passed to `hold_lock_file_for_update` or\n `hold_lock_file_for_append`:\n \n-LOCK_NODEREF::\n+LOCK_NO_DEREF::\n \n \tUsually symbolic links in the destination path are resolved\n \tand the lockfile is created by adding \".lock\" to the resolved\n-\tpath. If `LOCK_NODEREF` is set, then the lockfile is created\n+\tpath. If `LOCK_NO_DEREF` is set, then the lockfile is created\n \tby adding \".lock\" to the path argument itself. This option is\n \tused, for example, when locking a symbolic reference, which\n \tfor backwards-compatibility reasons can be a symbolic link\ndiff --git a/cache.h b/cache.h\nindex 30883b3..ca64a42 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -583,7 +583,7 @@ struct lock_file {\n \tstruct strbuf filename;\n };\n #define LOCK_DIE_ON_ERROR 1\n-#define LOCK_NODEREF 2\n+#define LOCK_NO_DEREF 2\n extern int unable_to_lock_error(const char *path, int err);\n extern void unable_to_lock_message(const char *path, int err,\n \t\t\t\t struct strbuf *buf);\ndiff --git a/lockfile.c b/lockfile.c\nindex cf7f4d0..a1cc08a 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -169,7 +169,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t}\n \n \tstrbuf_add(&lk->filename, path, pathlen);\n-\tif (!(flags & LOCK_NODEREF))\n+\tif (!(flags & LOCK_NO_DEREF))\n \t\tresolve_symlink(&lk->filename);\n \tstrbuf_addstr(&lk->filename, LOCK_SUFFIX);\n \tlk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\ndiff --git a/refs.c b/refs.c\nindex c6e15f9a..525ce4b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2134,7 +2134,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tlflags = 0;\n \tif (flags & REF_NODEREF) {\n \t\trefname = orig_refname;\n-\t\tlflags |= LOCK_NODEREF;\n+\t\tlflags |= LOCK_NO_DEREF;\n \t}\n \tlock->ref_name = xstrdup(refname);\n \tlock->orig_ref_name = xstrdup(orig_refname);\n-- \n2.1.0\n"},{"id":"249887","messageId":"1411726119-31598-36-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 35/39] lockfile.c: rename static functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:35Z","receivedAt":"2014-09-26T10:08:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"* remove_lock_file() -> remove_lock_files()\n* remove_lock_file_on_signal() -> remove_lock_files_on_signal()\n\nSuggested-by: Torsten Bögershausen \nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex a1cc08a..0a8c3c8 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -57,7 +57,7 @@\n \n static struct lock_file *volatile lock_file_list;\n \n-static void remove_lock_file(void)\n+static void remove_lock_files(void)\n {\n \tpid_t me = getpid();\n \n@@ -68,9 +68,9 @@ static void remove_lock_file(void)\n \t}\n }\n \n-static void remove_lock_file_on_signal(int signo)\n+static void remove_lock_files_on_signal(int signo)\n {\n-\tremove_lock_file();\n+\tremove_lock_files();\n \tsigchain_pop(signo);\n \traise(signo);\n }\n@@ -146,8 +146,8 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \n \tif (!lock_file_list) {\n \t\t/* One-time initialization */\n-\t\tsigchain_push_common(remove_lock_file_on_signal);\n-\t\tatexit(remove_lock_file);\n+\t\tsigchain_push_common(remove_lock_files_on_signal);\n+\t\tatexit(remove_lock_files);\n \t}\n \n \tif (lk->active)\n-- \n2.1.0\n"},{"id":"249911","messageId":"1411726119-31598-37-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 36/39] get_locked_file_path(): new function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:36Z","receivedAt":"2014-09-26T10:08:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add a function to return the path of the file that is locked by a\nlock_file object. This reduces the knowledge that callers have to have\nabout the lock_file layout.\n\nSuggested-by: Ronnie Sahlberg \nSigned-off-by: Michael Haggerty \n---\n Documentation/technical/api-lockfile.txt | 5 +++++\n cache.h | 1 +\n lockfile.c | 9 +++++++++\n refs.c | 4 +---\n 4 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex 64b250f..74b67c2 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -142,6 +142,11 @@ hold_lock_file_for_append::\n \tthe existing contents of the file (if any) to the lockfile and\n \tposition its write pointer at the end of the file.\n \n+get_locked_file_path::\n+\n+\tReturn the path of the file that is locked by the specified\n+\tlock_file object. The caller must free the memory.\n+\n commit_lock_file::\n \n \tTake a pointer to the `struct lock_file` initialized with an\ndiff --git a/cache.h b/cache.h\nindex ca64a42..0f1201e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -590,6 +590,7 @@ extern void unable_to_lock_message(const char *path, int err,\n extern NORETURN void unable_to_lock_die(const char *path, int err);\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n+extern char *get_locked_file_path(struct lock_file *);\n extern int commit_lock_file_to(struct lock_file *, const char *path);\n extern int commit_lock_file(struct lock_file *);\n extern int reopen_lock_file(struct lock_file *);\ndiff --git a/lockfile.c b/lockfile.c\nindex 0a8c3c8..c51c6ec 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -257,6 +257,15 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n \treturn fd;\n }\n \n+char *get_locked_file_path(struct lock_file *lk)\n+{\n+\tif (!lk->active)\n+\t\tdie(\"BUG: get_locked_file_path() called for unlocked object\");\n+\tif (lk->filename.len <= LOCK_SUFFIX_LEN)\n+\t\tdie(\"BUG: get_locked_file_path() called for malformed lock object\");\n+\treturn xmemdupz(lk->filename.buf, lk->filename.len - LOCK_SUFFIX_LEN);\n+}\n+\n int close_lock_file(struct lock_file *lk)\n {\n \tint fd = lk->fd;\ndiff --git a/refs.c b/refs.c\nindex 525ce4b..5842dd0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2556,9 +2556,7 @@ static int delete_ref_loose(struct ref_lock *lock, int flag)\n \t\t * loose. The loose file name is the same as the\n \t\t * lockfile name, minus \".lock\":\n \t\t */\n-\t\tchar *loose_filename = xmemdupz(\n-\t\t\t\tlock->lk->filename.buf,\n-\t\t\t\tlock->lk->filename.len - LOCK_SUFFIX_LEN);\n+\t\tchar *loose_filename = get_locked_file_path(lock->lk);\n \t\tint err = unlink_or_warn(loose_filename);\n \t\tfree(loose_filename);\n \t\tif (err && errno != ENOENT)\n-- \n2.1.0\n"},{"id":"249886","messageId":"1411726119-31598-38-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 37/39] hold_lock_file_for_append(): restore errno before returning","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:37Z","receivedAt":"2014-09-26T10:08:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Callers who don't pass LOCK_DIE_ON_ERROR might want to examine errno\nto see what went wrong, so restore errno before returning.\n\nIn fact this function only has one caller, add_to_alternates_file(),\nand it *does* use LOCK_DIE_ON_ERROR, but, you know, think of future\ngenerations.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex c51c6ec..b2f5d36 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -243,15 +243,22 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n \torig_fd = open(path, O_RDONLY);\n \tif (orig_fd < 0) {\n \t\tif (errno != ENOENT) {\n+\t\t\tint save_errno = errno;\n+\n \t\t\tif (flags & LOCK_DIE_ON_ERROR)\n \t\t\t\tdie(\"cannot open '%s' for copying\", path);\n \t\t\trollback_lock_file(lk);\n-\t\t\treturn error(\"cannot open '%s' for copying\", path);\n+\t\t\terror(\"cannot open '%s' for copying\", path);\n+\t\t\terrno = save_errno;\n+\t\t\treturn -1;\n \t\t}\n \t} else if (copy_fd(orig_fd, fd)) {\n+\t\tint save_errno = errno;\n+\n \t\tif (flags & LOCK_DIE_ON_ERROR)\n \t\t\texit(128);\n \t\trollback_lock_file(lk);\n+\t\terrno = save_errno;\n \t\treturn -1;\n \t}\n \treturn fd;\n-- \n2.1.0\n"},{"id":"249891","messageId":"1411726119-31598-39-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 38/39] Move read_index() definition to read-cache.c","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:38Z","receivedAt":"2014-09-26T10:08:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"lockfile.c contains the general API for locking any file. Code\nspecifically about the index file doesn't belong here.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 8 --------\n read-cache.c | 8 ++++++++\n 2 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex b2f5d36..63f4e94 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -340,14 +340,6 @@ int commit_lock_file(struct lock_file *lk)\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(),\n-\t\t\t\t\t die_on_error\n-\t\t\t\t\t ? LOCK_DIE_ON_ERROR\n-\t\t\t\t\t : 0);\n-}\n-\n void rollback_lock_file(struct lock_file *lk)\n {\n \tif (!lk->active)\ndiff --git a/read-cache.c b/read-cache.c\nindex e887e23..9f137e7 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1367,6 +1367,14 @@ static int read_index_extension(struct index_state *istate,\n \treturn 0;\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(),\n+\t\t\t\t\t die_on_error\n+\t\t\t\t\t ? LOCK_DIE_ON_ERROR\n+\t\t\t\t\t : 0);\n+}\n+\n int read_index(struct index_state *istate)\n {\n \treturn read_index_from(istate, get_index_file());\n-- \n2.1.0\n"},{"id":"249889","messageId":"1411726119-31598-40-git-send-email-mhagger@alum.mit.edu","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v6 39/39] lockfile.h: extract new header file for the functions in lockfile.c","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:08:39Z","receivedAt":"2014-09-26T10:08:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Move the interface declaration for the functions in lockfile.c from\ncache.h to a new file, lockfile.h. Add #includes where necessary (and\nremove some redundant includes of cache.h by files that already\ninclude builtin.h).\n\nMove the documentation of the lock_file state diagram from lockfile.c\nto the new header file.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/add.c | 1 +\n builtin/apply.c | 1 +\n builtin/checkout-index.c | 2 +-\n builtin/checkout.c | 2 +-\n builtin/clone.c | 1 +\n builtin/commit.c | 1 +\n builtin/describe.c | 1 +\n builtin/diff.c | 1 +\n builtin/gc.c | 2 +-\n builtin/merge.c | 1 +\n builtin/mv.c | 2 +-\n builtin/read-tree.c | 1 +\n builtin/receive-pack.c | 1 +\n builtin/reflog.c | 2 +-\n builtin/reset.c | 1 +\n builtin/rm.c | 2 +-\n builtin/update-index.c | 1 +\n bundle.c | 1 +\n cache-tree.c | 1 +\n cache.h | 27 +---------------\n config.c | 1 +\n credential-store.c | 1 +\n fast-import.c | 1 +\n fetch-pack.c | 1 +\n lockfile.c | 52 +-----------------------------\n lockfile.h | 84 ++++++++++++++++++++++++++++++++++++++++++++++++\n merge-recursive.c | 1 +\n merge.c | 1 +\n read-cache.c | 1 +\n refs.c | 1 +\n rerere.c | 1 +\n sequencer.c | 1 +\n sha1_file.c | 1 +\n shallow.c | 1 +\n test-scrap-cache-tree.c | 1 +\n 35 files changed, 118 insertions(+), 83 deletions(-)\n create mode 100644 lockfile.h\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 352b85e..ae6d3e2 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -5,6 +5,7 @@\n */\n #include \"cache.h\"\n #include \"builtin.h\"\n+#include \"lockfile.h\"\n #include \"dir.h\"\n #include \"pathspec.h\"\n #include \"exec_cmd.h\"\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex f204cca..21c8df2 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -7,6 +7,7 @@\n *\n */\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"cache-tree.h\"\n #include \"quote.h\"\n #include \"blob.h\"\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex 05edd9e..383dccf 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -5,7 +5,7 @@\n *\n */\n #include \"builtin.h\"\n-#include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"quote.h\"\n #include \"cache-tree.h\"\n #include \"parse-options.h\"\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 8afdf2b..570bb09 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1,5 +1,5 @@\n-#include \"cache.h\"\n #include \"builtin.h\"\n+#include \"lockfile.h\"\n #include \"parse-options.h\"\n #include \"refs.h\"\n #include \"commit.h\"\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 3927edf..d3bf953 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -9,6 +9,7 @@\n */\n \n #include \"builtin.h\"\n+#include \"lockfile.h\"\n #include \"parse-options.h\"\n #include \"fetch-pack.h\"\n #include \"refs.h\"\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex f55e809..c230018 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -6,6 +6,7 @@\n */\n \n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"cache-tree.h\"\n #include \"color.h\"\n #include \"dir.h\"\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex ee6a3b9..9103193 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"commit.h\"\n #include \"tag.h\"\n #include \"refs.h\"\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 0f247d2..4326fa5 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -4,6 +4,7 @@\n * Copyright (c) 2006 Junio C Hamano\n */\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"color.h\"\n #include \"commit.h\"\n #include \"blob.h\"\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex ced1456..005adbe 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -11,7 +11,7 @@\n */\n \n #include \"builtin.h\"\n-#include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"parse-options.h\"\n #include \"run-command.h\"\n #include \"sigchain.h\"\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex c7fa3bc..58497e0 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -9,6 +9,7 @@\n #include \"cache.h\"\n #include \"parse-options.h\"\n #include \"builtin.h\"\n+#include \"lockfile.h\"\n #include \"run-command.h\"\n #include \"diff.h\"\n #include \"refs.h\"\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex bf784cb..ea0b58a 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -3,8 +3,8 @@\n *\n * Copyright (C) 2006 Johannes Schindelin\n */\n-#include \"cache.h\"\n #include \"builtin.h\"\n+#include \"lockfile.h\"\n #include \"dir.h\"\n #include \"cache-tree.h\"\n #include \"string-list.h\"\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex e7e1c33..43b47f7 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -5,6 +5,7 @@\n */\n \n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"object.h\"\n #include \"tree.h\"\n #include \"tree-walk.h\"\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex afb8d99..47d4593 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1,4 +1,5 @@\n #include \"builtin.h\"\n+#include \"lockfile.h\"\n #include \"pack.h\"\n #include \"refs.h\"\n #include \"pkt-line.h\"\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 7c78b15..b6388f7 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -1,5 +1,5 @@\n-#include \"cache.h\"\n #include \"builtin.h\"\n+#include \"lockfile.h\"\n #include \"commit.h\"\n #include \"refs.h\"\n #include \"dir.h\"\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 855d478..4c08ddc 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -8,6 +8,7 @@\n * Copyright (c) 2005, 2006 Linus Torvalds and Junio C Hamano\n */\n #include \"builtin.h\"\n+#include \"lockfile.h\"\n #include \"tag.h\"\n #include \"object.h\"\n #include \"commit.h\"\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 2b61d3b..d8a9c86 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -3,8 +3,8 @@\n *\n * Copyright (C) Linus Torvalds 2006\n */\n-#include \"cache.h\"\n #include \"builtin.h\"\n+#include \"lockfile.h\"\n #include \"dir.h\"\n #include \"cache-tree.h\"\n #include \"tree-walk.h\"\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 6c95988..b0e3dc9 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -4,6 +4,7 @@\n * Copyright (C) Linus Torvalds, 2005\n */\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"quote.h\"\n #include \"cache-tree.h\"\n #include \"tree-walk.h\"\ndiff --git a/bundle.c b/bundle.c\nindex b2b89fe..891a3ca 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"bundle.h\"\n #include \"object.h\"\n #include \"commit.h\"\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 75a54fd..215202c 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"tree.h\"\n #include \"tree-walk.h\"\n #include \"cache-tree.h\"\ndiff --git a/cache.h b/cache.h\nindex 0f1201e..7857b80 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -570,36 +570,11 @@ 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 *volatile next;\n-\tvolatile sig_atomic_t active;\n-\tvolatile int fd;\n-\tvolatile pid_t owner;\n-\tchar on_list;\n-\tstruct strbuf filename;\n-};\n-#define LOCK_DIE_ON_ERROR 1\n-#define LOCK_NO_DEREF 2\n-extern int unable_to_lock_error(const char *path, int err);\n-extern void unable_to_lock_message(const char *path, int err,\n-\t\t\t\t struct strbuf *buf);\n-extern NORETURN void unable_to_lock_die(const char *path, int err);\n-extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n-extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n-extern char *get_locked_file_path(struct lock_file *);\n-extern int commit_lock_file_to(struct lock_file *, const char *path);\n-extern int commit_lock_file(struct lock_file *);\n-extern int reopen_lock_file(struct lock_file *);\n extern void update_index_if_able(struct index_state *, struct lock_file *);\n \n extern int hold_locked_index(struct lock_file *, int);\n extern void set_alternate_index_output(const char *);\n-extern int close_lock_file(struct lock_file *);\n-extern void rollback_lock_file(struct lock_file *);\n+\n extern int delete_ref(const char *, const unsigned char *sha1, int delopt);\n \n /* Environment bits from configuration mechanism */\ndiff --git a/config.c b/config.c\nindex 2110779..c31d4d2 100644\n--- a/config.c\n+++ b/config.c\n@@ -6,6 +6,7 @@\n *\n */\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"exec_cmd.h\"\n #include \"strbuf.h\"\n #include \"quote.h\"\ndiff --git a/credential-store.c b/credential-store.c\nindex f9146e5..d435514 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"credential.h\"\n #include \"string-list.h\"\n #include \"parse-options.h\"\ndiff --git a/fast-import.c b/fast-import.c\nindex 61ac7e0..44b6686 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -153,6 +153,7 @@ Format of STDIN stream:\n \n #include \"builtin.h\"\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"object.h\"\n #include \"blob.h\"\n #include \"tree.h\"\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 7487aa7..655ee64 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"refs.h\"\n #include \"pkt-line.h\"\n #include \"commit.h\"\ndiff --git a/lockfile.c b/lockfile.c\nindex 63f4e94..d27e61c 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -2,59 +2,9 @@\n * Copyright (c) 2005, Junio C Hamano\n */\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"sigchain.h\"\n \n-/*\n- * File write-locks as used by Git.\n- *\n- * For an overview of how to use the lockfile API, please see\n- *\n- * Documentation/technical/api-lockfile.txt\n- *\n- * This module keeps track of all locked files in lock_file_list for\n- * use at cleanup. This list and the lock_file objects that comprise\n- * it must be kept in self-consistent states at all time, because the\n- * program can be interrupted any time by a signal, in which case the\n- * signal handler will walk through the list attempting to clean up\n- * any open lock files.\n- *\n- * A lockfile is owned by the process that created it. The lock_file\n- * object has an \"owner\" field that records its owner. This field is\n- * used to prevent a forked process from closing a lockfile created by\n- * its parent.\n- *\n- * The possible states of a lock_file object 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- * 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:\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 successful close_lock_file()).\n- * Same as the previous state, except that the lockfile is closed\n- * and fd is -1.\n- *\n- * - Unlocked (after commit_lock_file(), commit_lock_file_to(),\n- * rollback_lock_file(), a failed attempt to lock, or a failed\n- * close_lock_file()). In this state:\n- * - active is unset\n- * - filename is empty (usually, though there are transitory\n- * states in which this condition doesn't hold). Client code should\n- * *not* rely on the filename being empty in this state.\n- * - fd is -1\n- * - the object is left registered in the lock_file_list, and\n- * on_list is set.\n- */\n-\n static struct lock_file *volatile lock_file_list;\n \n static void remove_lock_files(void)\ndiff --git a/lockfile.h b/lockfile.h\nnew file mode 100644\nindex 0000000..9059e89\n--- /dev/null\n+++ b/lockfile.h\n@@ -0,0 +1,84 @@\n+#ifndef LOCKFILE_H\n+#define LOCKFILE_H\n+\n+/*\n+ * File write-locks as used by Git.\n+ *\n+ * For an overview of how to use the lockfile API, please see\n+ *\n+ * Documentation/technical/api-lockfile.txt\n+ *\n+ * This module keeps track of all locked files in lock_file_list for\n+ * use at cleanup. This list and the lock_file objects that comprise\n+ * it must be kept in self-consistent states at all time, because the\n+ * program can be interrupted any time by a signal, in which case the\n+ * signal handler will walk through the list attempting to clean up\n+ * any open lock files.\n+ *\n+ * A lockfile is owned by the process that created it. The lock_file\n+ * object has an \"owner\" field that records its owner. This field is\n+ * used to prevent a forked process from closing a lockfile created by\n+ * its parent.\n+ *\n+ * The possible states of a lock_file object 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+ * 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:\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 successful close_lock_file()).\n+ * Same as the previous state, except that the lockfile is closed\n+ * and fd is -1.\n+ *\n+ * - Unlocked (after commit_lock_file(), commit_lock_file_to(),\n+ * rollback_lock_file(), a failed attempt to lock, or a failed\n+ * close_lock_file()). In this state:\n+ * - active is unset\n+ * - filename is empty (usually, though there are transitory\n+ * states in which this condition doesn't hold). Client code should\n+ * *not* rely on the filename being empty in this state.\n+ * - fd is -1\n+ * - the object is left registered in the lock_file_list, and\n+ * on_list is set.\n+ */\n+\n+struct lock_file {\n+\tstruct lock_file *volatile next;\n+\tvolatile sig_atomic_t active;\n+\tvolatile int fd;\n+\tvolatile pid_t owner;\n+\tchar on_list;\n+\tstruct strbuf filename;\n+};\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+#define LOCK_DIE_ON_ERROR 1\n+#define LOCK_NO_DEREF 2\n+\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_die(const char *path, int err);\n+extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n+extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n+extern char *get_locked_file_path(struct lock_file *);\n+extern int commit_lock_file_to(struct lock_file *, const char *path);\n+extern int commit_lock_file(struct lock_file *);\n+extern int reopen_lock_file(struct lock_file *);\n+extern int close_lock_file(struct lock_file *);\n+extern void rollback_lock_file(struct lock_file *);\n+\n+#endif /* LOCKFILE_H */\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 22315c3..5a0d58c 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -5,6 +5,7 @@\n */\n #include \"advice.h\"\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"cache-tree.h\"\n #include \"commit.h\"\n #include \"blob.h\"\ndiff --git a/merge.c b/merge.c\nindex 74ced7f..fcff632 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"commit.h\"\n #include \"run-command.h\"\n #include \"resolve-undo.h\"\ndiff --git a/read-cache.c b/read-cache.c\nindex 9f137e7..8f3e9eb 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -5,6 +5,7 @@\n */\n #define NO_THE_INDEX_COMPATIBILITY_MACROS\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"cache-tree.h\"\n #include \"refs.h\"\n #include \"dir.h\"\ndiff --git a/refs.c b/refs.c\nindex 5842dd0..18ddb71 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"refs.h\"\n #include \"object.h\"\n #include \"tag.h\"\ndiff --git a/rerere.c b/rerere.c\nindex 20b18ad..1b0555f 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"string-list.h\"\n #include \"rerere.h\"\n #include \"xdiff-interface.h\"\ndiff --git a/sequencer.c b/sequencer.c\nindex 5e8a207..1b9a35e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"sequencer.h\"\n #include \"dir.h\"\n #include \"object.h\"\ndiff --git a/sha1_file.c b/sha1_file.c\nindex c08c0cb..46ff273 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -8,6 +8,7 @@\n */\n #include \"cache.h\"\n #include \"string-list.h\"\n+#include \"lockfile.h\"\n #include \"delta.h\"\n #include \"pack.h\"\n #include \"blob.h\"\ndiff --git a/shallow.c b/shallow.c\nindex 33f3c7f..08e25a4 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"commit.h\"\n #include \"tag.h\"\n #include \"pkt-line.h\"\ndiff --git a/test-scrap-cache-tree.c b/test-scrap-cache-tree.c\nindex 9ebcbca..6efee31 100644\n--- a/test-scrap-cache-tree.c\n+++ b/test-scrap-cache-tree.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"lockfile.h\"\n #include \"tree.h\"\n #include \"cache-tree.h\"\n \n-- \n2.1.0\n"},{"id":"249915","messageId":"xmqqwq8q8hsa.fsf@gitster.dls.corp.google.com","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v6 00/39] Lockfile correctness and refactoring","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-26T16:31:49Z","receivedAt":"2014-09-26T16:31:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> * Rebase to current master.\n\nWhen you say this, could you be a bit more descriptive? Has the\nseries updated to use something new that appeared on 'master' since\nthe last series was posted and needed to be rebased? Or did you\njust made sure that the series applied on top of the current master\nstill works, even though you have been running \"rebase -i\" on the\nsame fork point since the previous round?\n\nIf the former, naming what it now uses i.e. \"rebased to current\nmaster, to redo PATCH x,y,z using the new X recently graduated\"\nwould be helpful.\n\nIf the latter, well, not rebasing is preferrable if the changes are\nstill viable on the previous fork point, to be absolutely honest.\n\nThanks.\n"},{"id":"249916","messageId":"xmqqlhp68c69.fsf@gitster.dls.corp.google.com","threadId":"37633","inReplyTo":"1411726119-31598-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v6 02/39] api-lockfile: revise and expand the documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-26T18:33:02Z","receivedAt":"2014-09-26T18:33:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Document a couple more functions and the flags argument as used by\n> hold_lock_file_for_update() and hold_lock_file_for_append().\n> Reorganize the document to make it more accessible.\n>\n> Helped-by: Jonathan Nieder \n> Signed-off-by: Michael Haggerty \n> ---\n> Documentation/technical/api-lockfile.txt | 199 ++++++++++++++++++++++---------\n> 1 file changed, 145 insertions(+), 54 deletions(-)\n\nNicely done.\n\n> +* Mutual exclusion and atomic file updates. When we want to change a\n> + file, we create a lockfile `.lock`, write the new file\n> + contents into it, and then rename the lockfile to its final\n> + destination ``. We create the `.lock` file with\n> + `O_CREAT|O_EXCL` so that we can notice and fail if somebody else has\n> + already locked the file.\n\n\nYou may want to say\n\n\tthen atomically rename the lockfile to its final destination\n\tto commit the changes and unlock the file.\n\nhere; that way, your mention of \"commit\" in the next paragraph would\nbecome easier to understand.\n\n> +* Automatic cruft removal. If the program exits after we lock a file\n> + but before the changes have been committed, we want to make sure\n> + that we remove the lockfile.\n\n\n\n\n> +Calling sequence\n> +----------------\n> +\n> +The caller:\n> +\n> +* Allocates a variable `struct lock_file lock` in the bss section or\n> + heap, initialized to zeros. It cannot be an auto variable allocated\n> + on the stack. Because the `lock_file` structure is used in an\n> + `atexit(3)` handler, its storage has to stay throughout the life of\n> + the program, even after the file changes have been committed or\n> + rolled back.\n\nIt is easier to read if you pushed the second sentence (which is a\nrephrase of the first one) and third sentence (which explains why\nthe second sentence is true) out of line as a side-note, I think, or\nprobably remove them from here (see below).\n\n> +* Attempts to create a lockfile by passing that variable and the path\n> + of the final destination (e.g. `$GIT_DIR/index`) to\n> + `hold_lock_file_for_update` or `hold_lock_file_for_append`.\n> +\n> +* Writes new content for the destination file by writing to the file\n> + descriptor returned by those functions (also available via\n> + `lock->fd`).\n> +\n> +When finished writing, the caller can:\n> +\n> +* Close the file descriptor and rename the lockfile to its final\n> + destination by calling `commit_lock_file`.\n> +\n> +* Close the file descriptor and remove the lockfile by calling\n> + `rollback_lock_file`.\n> +\n> +* Close the file descriptor without removing or renaming the lockfile\n> + by calling `close_lock_file`, and later call `commit_lock_file` or\n> + `rollback_lock_file`.\n> +\n> +At this point, the `lock_file` object must not be freed or altered,\n> +because it is still registered in the `lock_file_list`. However, it\n> +may be reused; just pass it to another call of\n> +`hold_lock_file_for_update` or `hold_lock_file_for_append`.\n\nIt feels a bit conflicting between \"must not be freed or ALTERED\"\nand \"it may be REUSED\". Drop \"or altered\" to reduce confusion? And\nthis repeats the third sentence I suggested to remove from the first\nparagraph (above 'see below' refers here).\n\nIs it allowed to tell the name of this lock_file to other people and\nlet them read from it? The answer is yes but it is not obvious from\nthis description.\n\nAlso mention how the above interact with reopen_lock_file() here?\n\n> +If the program exits before you have called one of `commit_lock_file`,\n> +`rollback_lock_file`, or `close_lock_file`, an `atexit(3)` handler\n> +will close and remove the lockfile, essentially rolling back any\n> +uncommitted changes.\n\ns/essentially //;\n\n> +Error handling\n> +--------------\n> +\n> +The `hold_lock_file_*` functions return a file descriptor on success\n> +or -1 on failure (unless `LOCK_DIE_ON_ERROR` is used; see below). On\n> +errors, `errno` describes the reason for failure. Errors can be\n> +handled by passing `errno` to one of the following helper functions:\n\ns/handled by/reported by/; probably. None of these will help you\n\"handle\" errors in the sense to (attempt to) recover from it.\n\n> +unable_to_lock_message::\n> +\n> +\tAppend an appropriate error message to a `strbuf`.\n> +\n> +unable_to_lock_error::\n> +\n> +\tEmit an appropriate error message using `error()`.\n> +\n> +unable_to_lock_die::\n> +\n> +\tEmit an appropriate error message and `die()`.\n"},{"id":"249917","messageId":"xmqqh9zu8ax5.fsf@gitster.dls.corp.google.com","threadId":"37633","inReplyTo":"1411726119-31598-28-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v6 27/39] try_merge_strategy(): use a statically-allocated lock_file object","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-26T19:00:06Z","receivedAt":"2014-09-26T19:00:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Even the one lockfile object needn't be allocated each time the\n> function is called. Instead, define one statically-allocated\n> lock_file object and reuse it for every call.\n>\n> Suggested-by: Jeff King \n> Signed-off-by: Michael Haggerty \n> ---\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\nI wondered if the next step would be to lose the \"lock\" parameter\nfrom {hold,write}_locked_index() and have them work on a\nprocess-global lock, but that would not work well.\n\nThe reason why this patch works is because we are only working with\na single destination (i.e. $GIT_INDEX_FILE typically .git/index),\nright?\n\nInteresting.\n"},{"id":"249918","messageId":"xmqqd2ai8agi.fsf@gitster.dls.corp.google.com","threadId":"37633","inReplyTo":"1411726119-31598-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v6 00/39] Lockfile correctness and refactoring","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-26T19:10:05Z","receivedAt":"2014-09-26T19:10:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Next iteration of my lockfile fixes and refactoring. Thanks to\n> Jonathan Nieder and Torsten Bögershausen for their comments about v5.\n>\n> I believe that this series addresses all of the comments from v1 [1],\n> v2 [2], v3 [3], v4 [4], and v5 [5].\n\nLooked quite good. Will replace---let's aim to merge this topic to\n'next' soonish.\n\nThanks.\n\n>\n> Changes since v4:\n>\n> * Revise API documentation.\n>\n> * Split out a separate header file for the lockfile API: lockfile.h.\n>\n> * Change close_lock_file() to rollback on errors and make it into a\n> NOOP if the file is already closed.\n>\n> * Don't set lk->on_list until the lock_file object is completely on\n> the lock_file_list.\n>\n> * Delete some information from the docstring in lockfile.c (now\n> lockfile.h) that is redundant with the API docs in api-lockfile.txt.\n>\n> * Remove the accidental extra call of git_config_clear() in\n> git_config_set_multivar_in_file() when commit_lock_file() fails.\n>\n> * Adjust some comments, error messages, and commit messages.\n>\n> * Rebase to current master.\n>\n> [1] http://thread.gmane.org/gmane.comp.version-control.git/245609\n> [2] http://thread.gmane.org/gmane.comp.version-control.git/245801\n> [3] http://thread.gmane.org/gmane.comp.version-control.git/246222\n> [4] http://thread.gmane.org/gmane.comp.version-control.git/256564\n> [5] http://thread.gmane.org/gmane.comp.version-control.git/257159\n>\n> Michael Haggerty (39):\n> unable_to_lock_die(): rename function from unable_to_lock_index_die()\n> api-lockfile: revise and expand the documentation\n> close_lock_file(): exit (successfully) if file is already closed\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 initialize and register lock_file object\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> close_lock_file(): if close fails, roll back\n> commit_lock_file(): rollback lock file on failure to rename\n> api-lockfile: document edge cases\n> dump_marks(): remove a redundant call to rollback_lock_file()\n> git_config_set_multivar_in_file(): avoid call to rollback_lock_file()\n> lockfile: avoid transitory invalid states\n> struct lock_file: declare some fields volatile\n> try_merge_strategy(): remove redundant lock_file allocation\n> try_merge_strategy(): use a statically-allocated lock_file object\n> commit_lock_file(): use a strbuf to manage temporary space\n> Change lock_file::filename into a strbuf\n> resolve_symlink(): use a strbuf for internal scratch space\n> resolve_symlink(): take a strbuf parameter\n> trim_last_path_component(): replace last_path_elm()\n> Extract a function commit_lock_file_to()\n> Rename LOCK_NODEREF to LOCK_NO_DEREF\n> lockfile.c: rename static functions\n> get_locked_file_path(): new function\n> hold_lock_file_for_append(): restore errno before returning\n> Move read_index() definition to read-cache.c\n> lockfile.h: extract new header file for the functions in lockfile.c\n>\n> Documentation/technical/api-lockfile.txt | 220 ++++++++++++++++++------\n> builtin/add.c | 1 +\n> builtin/apply.c | 1 +\n> builtin/checkout-index.c | 2 +-\n> builtin/checkout.c | 2 +-\n> builtin/clone.c | 1 +\n> builtin/commit.c | 17 +-\n> builtin/describe.c | 1 +\n> builtin/diff.c | 1 +\n> builtin/gc.c | 2 +-\n> builtin/merge.c | 16 +-\n> builtin/mv.c | 2 +-\n> builtin/read-tree.c | 1 +\n> builtin/receive-pack.c | 1 +\n> builtin/reflog.c | 4 +-\n> builtin/reset.c | 1 +\n> builtin/rm.c | 2 +-\n> builtin/update-index.c | 3 +-\n> bundle.c | 1 +\n> cache-tree.c | 1 +\n> cache.h | 20 +--\n> config.c | 16 +-\n> credential-store.c | 1 +\n> fast-import.c | 5 +-\n> fetch-pack.c | 1 +\n> lockfile.c | 284 +++++++++++++++++--------------\n> lockfile.h | 84 +++++++++\n> merge-recursive.c | 1 +\n> merge.c | 1 +\n> read-cache.c | 21 ++-\n> refs.c | 30 ++--\n> rerere.c | 1 +\n> sequencer.c | 1 +\n> sha1_file.c | 1 +\n> shallow.c | 7 +-\n> test-scrap-cache-tree.c | 1 +\n> 36 files changed, 492 insertions(+), 263 deletions(-)\n> create mode 100644 lockfile.h\n"},{"id":"249921","messageId":"xmqq8ul6869o.fsf@gitster.dls.corp.google.com","threadId":"37633","inReplyTo":"1411726119-31598-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v6 02/39] api-lockfile: revise and expand the documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-26T20:40:35Z","receivedAt":"2014-09-26T20:40:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> +If you need to close the file descriptor you obtained from a\n> +`hold_lock_file_*` function yourself, do so by calling\n> +`close_lock_file`. You should never call `close(2)` yourself!\n\nThis is sometimes untenable, isn't it? A caller may want to\nfreopen(3) a stream on the fd to write into it and then fclose(3);\nwhich would not know to call close_lock_file() but calls close(2).\n"},{"id":"250030","messageId":"542A7E2D.6030305@alum.mit.edu","threadId":"37633","inReplyTo":"xmqqwq8q8hsa.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 00/39] Lockfile correctness and refactoring","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-30T09:55:57Z","receivedAt":"2014-09-30T09:55:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/26/2014 06:31 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> * Rebase to current master.\n> \n> When you say this, could you be a bit more descriptive? Has the\n> series updated to use something new that appeared on 'master' since\n> the last series was posted and needed to be rebased? Or did you\n> just made sure that the series applied on top of the current master\n> still works, even though you have been running \"rebase -i\" on the\n> same fork point since the previous round?\n> \n> If the former, naming what it now uses i.e. \"rebased to current\n> master, to redo PATCH x,y,z using the new X recently graduated\"\n> would be helpful.\n> \n> If the latter, well, not rebasing is preferrable if the changes are\n> still viable on the previous fork point, to be absolutely honest.\n\nI have always routinely rebased my series to the current master each\ntime I reroll them. I thought that was helpful. Thanks for the info that\nyou prefer that I not do it. In the future I will only rebase to master\nif I see that there would be nontrivial conflicts or if I would like to\ntake advantage of something that has graduated.\n\nI've always tried to mention when the rebases were nontrivial. In this\ncase it was unproblematic.\n\n\nAs an aside, I've always found it wishy-washy to talk about \"current\nmaster\" and \"fork points\" and stuff when (AFAIK) format-patch-generated\nemails don't mention the fork point. So unless my cover letter specifies\nthe fork point as an unambiguous commit name, there is no basis *anyway*\nto expect that you will apply the patch series in the same context that\nI developed it.\n\nAnother victim of the format-patch information shredder :-(\n\nCheers,\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"250032","messageId":"542A8B90.50507@alum.mit.edu","threadId":"37633","inReplyTo":"xmqqlhp68c69.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 02/39] api-lockfile: revise and expand the documentation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-30T10:53:04Z","receivedAt":"2014-09-30T10:53:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/26/2014 08:33 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> Document a couple more functions and the flags argument as used by\n>> hold_lock_file_for_update() and hold_lock_file_for_append().\n>> Reorganize the document to make it more accessible.\n>>\n>> Helped-by: Jonathan Nieder \n>> Signed-off-by: Michael Haggerty \n>> ---\n>> Documentation/technical/api-lockfile.txt | 199 ++++++++++++++++++++++---------\n>> 1 file changed, 145 insertions(+), 54 deletions(-)\n> \n> Nicely done.\n> \n>> +* Mutual exclusion and atomic file updates. When we want to change a\n>> + file, we create a lockfile `.lock`, write the new file\n>> + contents into it, and then rename the lockfile to its final\n>> + destination ``. We create the `.lock` file with\n>> + `O_CREAT|O_EXCL` so that we can notice and fail if somebody else has\n>> + already locked the file.\n> \n> \n> You may want to say\n> \n> \tthen atomically rename the lockfile to its final destination\n> \tto commit the changes and unlock the file.\n> \n> here; that way, your mention of \"commit\" in the next paragraph would\n> become easier to understand.\n\nGood; will change.\n\n>> [...]\n>> +Calling sequence\n>> +----------------\n>> +\n>> +The caller:\n>> +\n>> +* Allocates a variable `struct lock_file lock` in the bss section or\n>> + heap, initialized to zeros. It cannot be an auto variable allocated\n>> + on the stack. Because the `lock_file` structure is used in an\n>> + `atexit(3)` handler, its storage has to stay throughout the life of\n>> + the program, even after the file changes have been committed or\n>> + rolled back.\n> \n> It is easier to read if you pushed the second sentence (which is a\n> rephrase of the first one) and third sentence (which explains why\n> the second sentence is true) out of line as a side-note, I think, or\n> probably remove them from here (see below).\n\nI was being repetitive because I didn't want the docs to depend on the\nuser remembering what the \"bss\" section is (which, technically, is also\nnot part of the C standard). I think a better way would be to just not\nmention \"bss section\" at all and reword the rest. Maybe something like\n\n The caller:\n\n * Allocates a variable `struct lock_file lock`, initialized to zeros.\n Because the `lock_file` structure is used in an `atexit(3)` handler,\n its storage has to remain valid throughout the life of the program;\n e.g., it should be a static variable or space allocated on the heap.\n\nBetter?\n\n>> +* Attempts to create a lockfile by passing that variable and the path\n>> + of the final destination (e.g. `$GIT_DIR/index`) to\n>> + `hold_lock_file_for_update` or `hold_lock_file_for_append`.\n>> +\n>> +* Writes new content for the destination file by writing to the file\n>> + descriptor returned by those functions (also available via\n>> + `lock->fd`).\n>> +\n>> +When finished writing, the caller can:\n>> +\n>> +* Close the file descriptor and rename the lockfile to its final\n>> + destination by calling `commit_lock_file`.\n>> +\n>> +* Close the file descriptor and remove the lockfile by calling\n>> + `rollback_lock_file`.\n>> +\n>> +* Close the file descriptor without removing or renaming the lockfile\n>> + by calling `close_lock_file`, and later call `commit_lock_file` or\n>> + `rollback_lock_file`.\n>> +\n>> +At this point, the `lock_file` object must not be freed or altered,\n>> +because it is still registered in the `lock_file_list`. However, it\n>> +may be reused; just pass it to another call of\n>> +`hold_lock_file_for_update` or `hold_lock_file_for_append`.\n> \n> It feels a bit conflicting between \"must not be freed or ALTERED\"\n> and \"it may be REUSED\". Drop \"or altered\" to reduce confusion? And\n> this repeats the third sentence I suggested to remove from the first\n> paragraph (above 'see below' refers here).\n\nThe purpose of \"or altered\" is to make sure that users don't imagine\nthat they should memset() the structure to zeros or something before\nreusing it (especially since the \"caller\" instructions earlier say that\nthe object should be \"initialized to zeros\").\n\nWould it help if I change\n\n s/altered/altered by the caller/\n\n?\n\nI will also drop \"because it is still registered in the `lock_file_list`\".\n\n> Is it allowed to tell the name of this lock_file to other people and\n> let them read from it? The answer is yes but it is not obvious from\n> this description.\n> \n> Also mention how the above interact with reopen_lock_file() here?\n\nI'll take a stab at it, though TBH I haven't really studied the use case\nfor reopen_lock_file(). You might be better able to provide insight into\nthis topic.\n\n>> +If the program exits before you have called one of `commit_lock_file`,\n>> +`rollback_lock_file`, or `close_lock_file`, an `atexit(3)` handler\n>> +will close and remove the lockfile, essentially rolling back any\n>> +uncommitted changes.\n> \n> s/essentially //;\n\nDone.\n\n>> +Error handling\n>> +--------------\n>> +\n>> +The `hold_lock_file_*` functions return a file descriptor on success\n>> +or -1 on failure (unless `LOCK_DIE_ON_ERROR` is used; see below). On\n>> +errors, `errno` describes the reason for failure. Errors can be\n>> +handled by passing `errno` to one of the following helper functions:\n> \n> s/handled by/reported by/; probably. None of these will help you\n> \"handle\" errors in the sense to (attempt to) recover from it.\n\nDone.\n\nThanks for your suggestions!\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"250034","messageId":"542AB323.4080402@alum.mit.edu","threadId":"37633","inReplyTo":"xmqq8ul6869o.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 02/39] api-lockfile: revise and expand the documentation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-30T13:41:55Z","receivedAt":"2014-09-30T13:41:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/26/2014 10:40 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> +If you need to close the file descriptor you obtained from a\n>> +`hold_lock_file_*` function yourself, do so by calling\n>> +`close_lock_file`. You should never call `close(2)` yourself!\n> \n> This is sometimes untenable, isn't it? A caller may want to\n> freopen(3) a stream on the fd to write into it and then fclose(3);\n> which would not know to call close_lock_file() but calls close(2).\n\nYou are absolutely correct. I carried this text over from the old\nversion, where it was just as inaccurate.\n\nI didn't fix it because IMO the correct fix is to add a stdio-oriented\nentry point to the lockfile API, and teach the lockfile code to handle\nclosing the FILE correctly when necessary.\n\nBut I didn't want to add even more changes to this patch series, so I am\nworking on this as a separate patch series. I hope to submit it soon.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"250035","messageId":"542AB853.5000303@alum.mit.edu","threadId":"37633","inReplyTo":"xmqqh9zu8ax5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 27/39] try_merge_strategy(): use a statically-allocated lock_file object","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-30T14:04:03Z","receivedAt":"2014-09-30T14:04:03Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/26/2014 09:00 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> Even the one lockfile object needn't be allocated each time the\n>> function is called. Instead, define one statically-allocated\n>> lock_file object and reuse it for every call.\n>>\n>> Suggested-by: Jeff King \n>> Signed-off-by: Michael Haggerty \n>> ---\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> \n> I wondered if the next step would be to lose the \"lock\" parameter\n> from {hold,write}_locked_index() and have them work on a\n> process-global lock, but that would not work well.\n> \n> The reason why this patch works is because we are only working with\n> a single destination (i.e. $GIT_INDEX_FILE typically .git/index),\n> right?\n> \n> Interesting.\n\nUmmm, this patch wasn't supposed to be interesting. If it is then maybe\nI made a mistake...\n\nMy reasoning was that after the lock is acquired, it is released\nunconditionally before the function exits. Therefore, it should be no\nproblem to reuse it each time the function is called.\n\nOf course, one gap in this argument is the possibility that this\nfunction calls itself recursively. The fact that it calls\nmerge_recursive() should have alerted me to this possibility. So let me\ncheck...\n\n* try_merge_strategy() is only called by cmd_merge()\n* cmd_merge() is only called by the command dispatcher\n\nSo I don't see a way for this function to call itself recursively within\na single process.\n\nA second possible gap in this argument would be if this function can be\ncalled from multiple threads. But hardly any of our code is thread-safe,\nso I hardly think that is likely.\n\nWhat am I missing?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"250038","messageId":"20140930161543.GA10581@peff.net","threadId":"37633","inReplyTo":"542AB323.4080402@alum.mit.edu","subject":"Re: [PATCH v6 02/39] api-lockfile: revise and expand the documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-30T16:15:43Z","receivedAt":"2014-09-30T16:15:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 30, 2014 at 03:41:55PM +0200, Michael Haggerty wrote:\n\n> I didn't fix it because IMO the correct fix is to add a stdio-oriented\n> entry point to the lockfile API, and teach the lockfile code to handle\n> closing the FILE correctly when necessary.\n\nI think so, too, after our discussion[1] surrounding 9540ce5 (refs: write\npacked_refs file using stdio, 2014-09-10).\n\n> But I didn't want to add even more changes to this patch series, so I am\n> working on this as a separate patch series. I hope to submit it soon.\n\nYay.\n\n-Peff\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/256729/focus=256734\n"},{"id":"250039","messageId":"xmqqsij9rose.fsf@gitster.dls.corp.google.com","threadId":"37633","inReplyTo":"542A8B90.50507@alum.mit.edu","subject":"Re: [PATCH v6 02/39] api-lockfile: revise and expand the documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-30T17:39:13Z","receivedAt":"2014-09-30T17:39:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> I was being repetitive because I didn't want the docs to depend on the\n> user remembering what the \"bss\" section is (which, technically, is also\n> not part of the C standard). I think a better way would be to just not\n> mention \"bss section\" at all and reword the rest. Maybe something like\n>\n> The caller:\n>\n> * Allocates a variable `struct lock_file lock`, initialized to zeros.\n> Because the `lock_file` structure is used in an `atexit(3)` handler,\n> its storage has to remain valid throughout the life of the program;\n> e.g., it should be a static variable or space allocated on the heap.\n>\n> Better?\n\nSomewhat. I like that you droped \"BSS\", though.\n\n\tAllocates a 'struct lock_file' either as a static variable\n\tor on the heap, initialized to zeros. Once you use the\n\tstructure to call hold_lock_file_* family of functions, it\n\tbelongs to the lockfile subsystem and its storage must\n\tremain valid throughout the life of the program (i.e. you\n\tcannot use an on-stack variable to hold this structure).\n\n>> It feels a bit conflicting between \"must not be freed or ALTERED\"\n>> and \"it may be REUSED\". Drop \"or altered\" to reduce confusion? And\n>> this repeats the third sentence I suggested to remove from the first\n>> paragraph (above 'see below' refers here).\n>\n> The purpose of \"or altered\" is to make sure that users don't imagine\n> that they should memset() the structure to zeros or something before\n> reusing it (especially since the \"caller\" instructions earlier say that\n> the object should be \"initialized to zeros\").\n>\n> Would it help if I change\n>\n> s/altered/altered by the caller/\n>\n> ?\n\nThe fundamental rule is that callers outside the lockfile system must\nnot touch individual members of \"struct lock_file\" that is \"live\".\nThey must not free, they must not alter, they must not do anything\nother than calling the lockfile API functions.\n\nWhile it is natural that the readers would understand such a rule\nmust be followed for a lockfile that is in either the initialized,\nlocked, closed-but-not-committed state, I agree that it is not just\npossible but likely that people misunderstand and think that once a\nlockfile is committed or rolled back it no longer has to follow that\nrule. We would want to make sure readers do not fall into such a\nmisunderstanding.\n\nI dunno. Your \"if you memset it to NULs, you will break the linked\nlist of the lock and the whole lockfile system and the element\ncannot even be reused.\" may be the most important thing you may have\nwanted to say, but it is not conveyed by that change at all, at\nleast to me.\n\nA small voice in the back of my skull keeps telling me that a rule\nthat is hard to document and explain is a rule that does not make\nsense. Is it possible to allow commit and rollback to safely remove\nthe structure from the atexit(3) list (aka \"no longer owned by the\nlockfile subsystem\")?\n\n>> Is it allowed to tell the name of this lock_file to other people and\n>> let them read from it? The answer is yes but it is not obvious from\n>> this description.\n>> \n>> Also mention how the above interact with reopen_lock_file() here?\n>\n> I'll take a stab at it, though TBH I haven't really studied the use case\n> for reopen_lock_file(). You might be better able to provide insight into\n> this topic.\n\nYou would want to be able to do this:\n\n - hold a lock on a file (say, the index);\n\n - update it in preparation to commit it;\n\n - write it out and make sure the contents is on the disk platter,\n in preparation to call another program and allow it (and nobody\n else) to inspect the contents you wrote, while still holding the\n lock yourself. In our set of API functions, close_lock_file is\n what lets us do this.\n\n - further update it, write it out and commit. We need to read it\n first, open(2) it to write, write(2), and commit_lock_file().\n\nThe set of API functions you described in the document, there is no\nway to say \"I already have a lock on that file, just let me open(2)\nfor writing because I have further updates\" and that is where reopen\ncomes in.\n"},{"id":"250041","messageId":"xmqqoatxrodx.fsf@gitster.dls.corp.google.com","threadId":"37633","inReplyTo":"20140930161543.GA10581@peff.net","subject":"Re: [PATCH v6 02/39] api-lockfile: revise and expand the documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-30T17:47:54Z","receivedAt":"2014-09-30T17:47:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Sep 30, 2014 at 03:41:55PM +0200, Michael Haggerty wrote:\n>\n>> I didn't fix it because IMO the correct fix is to add a stdio-oriented\n>> entry point to the lockfile API, and teach the lockfile code to handle\n>> closing the FILE correctly when necessary.\n>\n> I think so, too, after our discussion[1] surrounding 9540ce5 (refs: write\n> packed_refs file using stdio, 2014-09-10).\n\nYeah, but we already write packed-refs via stdio, so the stdio\noriented lockfile API entry points can no longer be just on the\nmythical todo list but needs to become reality before we can merge\nthis topic sanely.\n\n>> But I didn't want to add even more changes to this patch series, so I am\n>> working on this as a separate patch series. I hope to submit it soon.\n>\n> Yay.\n\nYay.\n\nThanks.\n"},{"id":"250042","messageId":"xmqqiok5rnf4.fsf@gitster.dls.corp.google.com","threadId":"37633","inReplyTo":"542AB853.5000303@alum.mit.edu","subject":"Re: [PATCH v6 27/39] try_merge_strategy(): use a statically-allocated lock_file object","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-30T18:08:47Z","receivedAt":"2014-09-30T18:08:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 09/26/2014 09:00 PM, Junio C Hamano wrote:\n>> Michael Haggerty writes:\n>> \n>>> Even the one lockfile object needn't be allocated each time the\n>>> function is called. Instead, define one statically-allocated\n>>> lock_file object and reuse it for every call.\n>>>\n>>> Suggested-by: Jeff King \n>>> Signed-off-by: Michael Haggerty \n>>> ---\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>> \n>> I wondered if the next step would be to lose the \"lock\" parameter\n>> from {hold,write}_locked_index() and have them work on a\n>> process-global lock, but that would not work well.\n>> \n>> The reason why this patch works is because we are only working with\n>> a single destination (i.e. $GIT_INDEX_FILE typically .git/index),\n>> right?\n>> \n>> Interesting.\n>\n> Ummm, this patch wasn't supposed to be interesting. If it is then maybe\n> I made a mistake...\n\nWell, Interesting is very different from Questionable.\n\nThis lock can never have multiple active instances, as you\nmentioned.\n\nWe didn't have to change this for so long since the function was\nwritten; that probably is because this sequence:\n\n\tlock = hold_lock();\n use(lock);\n commit(lock); /* or rollback(lock) */\n\nis so obviously natural to callers of the API, and commit/rollback\nlooks very much like a declaration that we are done with the object\nand its resource can be freed.\n\nThis change makes sense because the API does not actually allow us\nto free the resource held to use this lock, so reducing the number\nof \"struct lock_file\" ever used during the life of the program makes\ndifference, especially when you use many, even when not many of them\nyou need to hold at the same time.\n\nWhich was counter-intuitive to me, and the realization did not hit\nme until I thought about it, which made it an \"Interesting\" change.\n\nIt may at the same be suggesting that \"once in the lockfile subsystem,\nthe resource becomes reclaimable\" may be something we would want to\nfix, though.\n"},{"id":"250057","messageId":"542BAF33.9090406@alum.mit.edu","threadId":"37633","inReplyTo":"xmqqsij9rose.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 02/39] api-lockfile: revise and expand the documentation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-01T07:37:23Z","receivedAt":"2014-10-01T07:37:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/30/2014 07:39 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> I was being repetitive because I didn't want the docs to depend on the\n>> user remembering what the \"bss\" section is (which, technically, is also\n>> not part of the C standard). I think a better way would be to just not\n>> mention \"bss section\" at all and reword the rest. Maybe something like\n>>\n>> The caller:\n>>\n>> * Allocates a variable `struct lock_file lock`, initialized to zeros.\n>> Because the `lock_file` structure is used in an `atexit(3)` handler,\n>> its storage has to remain valid throughout the life of the program;\n>> e.g., it should be a static variable or space allocated on the heap.\n>>\n>> Better?\n> \n> Somewhat. I like that you droped \"BSS\", though.\n> \n> \tAllocates a 'struct lock_file' either as a static variable\n> \tor on the heap, initialized to zeros. Once you use the\n> \tstructure to call hold_lock_file_* family of functions, it\n> \tbelongs to the lockfile subsystem and its storage must\n> \tremain valid throughout the life of the program (i.e. you\n> \tcannot use an on-stack variable to hold this structure).\n\nOK, I'll use that.\n\n>>> It feels a bit conflicting between \"must not be freed or ALTERED\"\n>>> and \"it may be REUSED\". Drop \"or altered\" to reduce confusion? And\n>>> this repeats the third sentence I suggested to remove from the first\n>>> paragraph (above 'see below' refers here).\n>>\n>> The purpose of \"or altered\" is to make sure that users don't imagine\n>> that they should memset() the structure to zeros or something before\n>> reusing it (especially since the \"caller\" instructions earlier say that\n>> the object should be \"initialized to zeros\").\n>>\n>> Would it help if I change\n>>\n>> s/altered/altered by the caller/\n>>\n>> ?\n> \n> The fundamental rule is that callers outside the lockfile system must\n> not touch individual members of \"struct lock_file\" that is \"live\".\n> They must not free, they must not alter, they must not do anything\n> other than calling the lockfile API functions.\n> \n> While it is natural that the readers would understand such a rule\n> must be followed for a lockfile that is in either the initialized,\n> locked, closed-but-not-committed state, I agree that it is not just\n> possible but likely that people misunderstand and think that once a\n> lockfile is committed or rolled back it no longer has to follow that\n> rule. We would want to make sure readers do not fall into such a\n> misunderstanding.\n> \n> I dunno. Your \"if you memset it to NULs, you will break the linked\n> list of the lock and the whole lockfile system and the element\n> cannot even be reused.\" may be the most important thing you may have\n> wanted to say, but it is not conveyed by that change at all, at\n> least to me.\n> \n> A small voice in the back of my skull keeps telling me that a rule\n> that is hard to document and explain is a rule that does not make\n> sense. Is it possible to allow commit and rollback to safely remove\n> the structure from the atexit(3) list (aka \"no longer owned by the\n> lockfile subsystem\")?\n\nCertainly it is possible. One would probably make lock_file an opaque\ndata structure, always allocated on the heap within the lockfile\nsubsystem, and maybe with a free list to reduce memory allocations.\n\nBut it would be a lot of work and I don't see a commensurate payoff.\nThere are not *that* many callers of the lockfile API.\n\n>>> Is it allowed to tell the name of this lock_file to other people and\n>>> let them read from it? The answer is yes but it is not obvious from\n>>> this description.\n>>>\n>>> Also mention how the above interact with reopen_lock_file() here?\n>>\n>> I'll take a stab at it, though TBH I haven't really studied the use case\n>> for reopen_lock_file(). You might be better able to provide insight into\n>> this topic.\n> \n> You would want to be able to do this:\n> \n> - hold a lock on a file (say, the index);\n> \n> - update it in preparation to commit it;\n> \n> - write it out and make sure the contents is on the disk platter,\n> in preparation to call another program and allow it (and nobody\n> else) to inspect the contents you wrote, while still holding the\n> lock yourself. In our set of API functions, close_lock_file is\n> what lets us do this.\n> \n> - further update it, write it out and commit. We need to read it\n> first, open(2) it to write, write(2), and commit_lock_file().\n> \n> The set of API functions you described in the document, there is no\n> way to say \"I already have a lock on that file, just let me open(2)\n> for writing because I have further updates\" and that is where reopen\n> comes in.\n\nThanks for the clarification. My upcoming reroll will document\nreopen_lock_file().\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"250058","messageId":"542BB738.9080100@alum.mit.edu","threadId":"37633","inReplyTo":"xmqqoatxrodx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 02/39] api-lockfile: revise and expand the documentation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-01T08:11:36Z","receivedAt":"2014-10-01T08:11:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/30/2014 07:47 PM, Junio C Hamano wrote:\n> Jeff King writes:\n> \n>> On Tue, Sep 30, 2014 at 03:41:55PM +0200, Michael Haggerty wrote:\n>>\n>>> I didn't fix it because IMO the correct fix is to add a stdio-oriented\n>>> entry point to the lockfile API, and teach the lockfile code to handle\n>>> closing the FILE correctly when necessary.\n>>\n>> I think so, too, after our discussion[1] surrounding 9540ce5 (refs: write\n>> packed_refs file using stdio, 2014-09-10).\n> \n> Yeah, but we already write packed-refs via stdio, so the stdio\n> oriented lockfile API entry points can no longer be just on the\n> mythical todo list but needs to become reality before we can merge\n> this topic sanely.\n\nThat's not the fault of this topic, which just moves the text of the\n\"rule\" to a different place in the file. And neither is it the fault of\nPeff's change to write packed-refs via stdio. It is the fault of\n\n 60b9004 Use atomic updates to the fast-import mark file (2007-03-08)\n\nwhich also fdopen()ed then fclose()d a lock_file::fd, and of\n\n 0c0478c Document lockfile API (2008-01-16)\n\nwhich documented the \"rule\" that had already been broken.\n\n>>> But I didn't want to add even more changes to this patch series, so I am\n>>> working on this as a separate patch series. I hope to submit it soon.\n>>\n>> Yay.\n> \n> Yay.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"}]}