{"thread":{"id":"46872","subject":"[PATCH 00/11] various lockfile-leaks and -fixes","startedAt":"2017-10-01T14:56:25Z","lastAt":"2017-10-06T20:13:18Z","messageCount":69,"participants":["Martin Ågren","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"329288","messageId":"cover.1506862824.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":null,"subject":"[PATCH 00/11] various lockfile-leaks and -fixes","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-01T14:56:01Z","receivedAt":"2017-10-01T14:56:25Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"A recent series allowed `struct lock_file`s to be freed [1], so I wanted\nto get rid of the \"simple\" leaks of this kind. I found a couple of\nlock-related cleanups along the way and it resulted in this series. It\nfeels a bit scary at eleven patches -- especially as this is about\nlocking -- but I hope I've managed to put this into understandable and\nreviewable patches. Reviews, thoughts, opinions welcome, as always.\n\n1-2\n\nInstead of allocating and leaking `struct lock_file`s, initialize them\non the stack. These instances were identified by simple grepping.\n\n3-4\n\nDocumentation fixes for lockfile and temporary file APIs.\n\n5-7\n\nNo need to represent the same information twice. No need for the caller\nto provide a (static) lock for us now that we can safely manage our own.\n\n8-11\n\nError-handling in read-cache.c was a bit lacking and over-eager at the\nsame time. Sometimes we'd leave the lock open when we should commit it,\nsometimes we'd roll it back when we should only close it. We'd also\ncrash under rare circumstances. Patch 9 addresses a bug in the API which\nmost likely hasn't hit anyone and maybe never would, but if we don't\nhave the functionality, I don't think we should pretend like we do.\n\nMartin\n\n[1] https://public-inbox.org/git/20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net/\n\nMartin Ågren (11):\n sha1_file: do not leak `lock_file`\n treewide: prefer lockfiles on the stack\n lockfile: fix documentation on `close_lock_file_gently()`\n tempfile: fix documentation on `delete_tempfile()`\n cache-tree: simplify locking logic\n apply: move lockfile into `apply_state`\n apply: remove `newfd` from `struct apply_state`\n cache.h: document `write_locked_index()`\n read-cache: require flags for `write_locked_index()`\n read-cache: don't leave dangling pointer in `do_write_index()`\n read-cache: roll back lock on error with `COMMIT_LOCK`\n\n apply.c | 25 ++++++++-----------------\n apply.h | 8 +++-----\n builtin/am.c | 27 ++++++++++++---------------\n builtin/apply.c | 4 +---\n builtin/checkout.c | 14 ++++++--------\n builtin/clone.c | 7 +++----\n builtin/diff.c | 7 +++----\n builtin/difftool.c | 1 -\n cache-tree.c | 12 ++++--------\n cache.h | 19 +++++++++++++++++++\n config.c | 17 ++++++++---------\n git-compat-util.h | 7 ++++++-\n lockfile.h | 4 ++--\n merge-recursive.c | 6 +++---\n merge.c | 8 +++-----\n read-cache.c | 26 ++++++++++++++------------\n sequencer.c | 1 -\n sha1_file.c | 16 +++++++---------\n tempfile.h | 8 ++++----\n wt-status.c | 8 ++++----\n 20 files changed, 110 insertions(+), 115 deletions(-)\n\n-- \n2.14.1.727.g9ddaf86\n\n"},{"id":"329289","messageId":"9725917d9854a54e4daeda70940c3bb0ff5d9201.1506862824.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1506862824.git.martin.agren@gmail.com","subject":"[PATCH 01/11] sha1_file: do not leak `lock_file`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-01T14:56:02Z","receivedAt":"2017-10-01T14:56:36Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"There is no longer any need to allocate and leak a `struct lock_file`.\nInitialize it on the stack instead.\n\nInstead of setting `lock = NULL` to signal that we have already rolled\nback, and that we should not do any more work, check with\n`is_lock_file_locked()`. Since we take the lock with\n`LOCK_DIE_ON_ERROR`, it evaluates to false exactly when we have called\n`rollback_lock_file()`.\n\nSigned-off-by: Martin Ågren \n---\n sha1_file.c | 16 +++++++---------\n 1 file changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5a2014811..c50d6237e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -456,12 +456,12 @@ struct alternate_object_database *alloc_alt_odb(const char *dir)\n \n void add_to_alternates_file(const char *reference)\n {\n-\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n+\tstruct lock_file lock = LOCK_INIT;\n \tchar *alts = git_pathdup(\"objects/info/alternates\");\n \tFILE *in, *out;\n \n-\thold_lock_file_for_update(lock, alts, LOCK_DIE_ON_ERROR);\n-\tout = fdopen_lock_file(lock, \"w\");\n+\thold_lock_file_for_update(&lock, alts, LOCK_DIE_ON_ERROR);\n+\tout = fdopen_lock_file(&lock, \"w\");\n \tif (!out)\n \t\tdie_errno(\"unable to fdopen alternates lockfile\");\n \n@@ -481,17 +481,15 @@ void add_to_alternates_file(const char *reference)\n \t\tstrbuf_release(&line);\n \t\tfclose(in);\n \n-\t\tif (found) {\n-\t\t\trollback_lock_file(lock);\n-\t\t\tlock = NULL;\n-\t\t}\n+\t\tif (found)\n+\t\t\trollback_lock_file(&lock);\n \t}\n \telse if (errno != ENOENT)\n \t\tdie_errno(\"unable to read alternates file\");\n \n-\tif (lock) {\n+\tif (is_lock_file_locked(&lock)) {\n \t\tfprintf_or_die(out, \"%s\\n\", reference);\n-\t\tif (commit_lock_file(lock))\n+\t\tif (commit_lock_file(&lock))\n \t\t\tdie_errno(\"unable to move new alternates file into place\");\n \t\tif (alt_odb_tail)\n \t\t\tlink_alt_odb_entries(reference, '\\n', NULL, 0);\n-- \n2.14.1.727.g9ddaf86\n\n"},{"id":"329290","messageId":"f241038eeed0f5f8bb346b394495356c20884a39.1506862824.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1506862824.git.martin.agren@gmail.com","subject":"[PATCH 02/11] treewide: prefer lockfiles on the stack","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-01T14:56:03Z","receivedAt":"2017-10-01T14:56:43Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"There is no longer any need to allocate and leak a `struct lock_file`.\nThe previous patch addressed an instance where we needed a minor tweak\nalongside the trivial changes.\n\nDeal with the remaining instances where we allocate and leak a struct\nwithin a single function. Change them to have the `struct lock_file` on\nthe stack instead.\n\nThese instances were identified by running `git grep \"^\\s*struct\nlock_file\\s*\\*\"`.\n\nSigned-off-by: Martin Ågren \n---\nThe changes to config.c conflict on pu (52d59cc64 (branch: add a --copy\n(-c) option to go with --move (-m), 2017-06-18)), both textually and\nbecause that commit add some more \"lock\" that need to be \"&lock\".\n\n builtin/am.c | 24 +++++++++++-------------\n builtin/checkout.c | 14 ++++++--------\n builtin/clone.c | 7 +++----\n builtin/diff.c | 7 +++----\n config.c | 17 ++++++++---------\n merge-recursive.c | 6 +++---\n merge.c | 8 ++++----\n wt-status.c | 8 ++++----\n 8 files changed, 42 insertions(+), 49 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex d7513f537..4e16fd428 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1134,11 +1134,11 @@ static const char *msgnum(const struct am_state *state)\n */\n static void refresh_and_write_cache(void)\n {\n-\tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n+\tstruct lock_file lock_file = LOCK_INIT;\n \n-\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \trefresh_cache(REFRESH_QUIET);\n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write index file\"));\n }\n \n@@ -1946,15 +1946,14 @@ static void am_resolve(struct am_state *state)\n */\n static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n {\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc t[2];\n \n \tif (parse_tree(head) || parse_tree(remote))\n \t\treturn -1;\n \n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \trefresh_cache(REFRESH_QUIET);\n \n@@ -1970,11 +1969,11 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n \tinit_tree_desc(&t[1], remote->buffer, remote->size);\n \n \tif (unpack_trees(2, t, &opts)) {\n-\t\trollback_lock_file(lock_file);\n+\t\trollback_lock_file(&lock_file);\n \t\treturn -1;\n \t}\n \n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n \treturn 0;\n@@ -1986,15 +1985,14 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n */\n static int merge_tree(struct tree *tree)\n {\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc t[1];\n \n \tif (parse_tree(tree))\n \t\treturn -1;\n \n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = 1;\n@@ -2005,11 +2003,11 @@ static int merge_tree(struct tree *tree)\n \tinit_tree_desc(&t[0], tree->buffer, tree->size);\n \n \tif (unpack_trees(1, t, &opts)) {\n-\t\trollback_lock_file(lock_file);\n+\t\trollback_lock_file(&lock_file);\n \t\treturn -1;\n \t}\n \n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n \treturn 0;\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3345a0d16..34c3c5b61 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -247,7 +247,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n \tstruct object_id rev;\n \tstruct commit *head;\n \tint errs = 0;\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \n \tif (opts->track != BRANCH_TRACK_UNSPECIFIED)\n \t\tdie(_(\"'%s' cannot be used with updating paths\"), \"--track\");\n@@ -275,9 +275,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\treturn run_add_interactive(revision, \"--patch=checkout\",\n \t\t\t\t\t &opts->pathspec);\n \n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\n-\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \tif (read_cache_preload(&opts->pathspec) < 0)\n \t\treturn error(_(\"index file corrupt\"));\n \n@@ -376,7 +374,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t}\n \terrs |= finish_delayed_checkout(&state);\n \n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n \tread_ref_full(\"HEAD\", 0, rev.hash, NULL);\n@@ -472,9 +470,9 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t int *writeout_error)\n {\n \tint ret;\n-\tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n+\tstruct lock_file lock_file = LOCK_INIT;\n \n-\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \tif (read_cache_preload(NULL) < 0)\n \t\treturn error(_(\"index file corrupt\"));\n \n@@ -591,7 +589,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \tif (!cache_tree_fully_valid(active_cache_tree))\n \t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n \n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n \tif (!opts->force && !opts->quiet)\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex dbddd98f8..96a3aaaa1 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -706,7 +706,7 @@ static int checkout(int submodule_progress)\n {\n \tstruct object_id oid;\n \tchar *head;\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tstruct unpack_trees_options opts;\n \tstruct tree *tree;\n \tstruct tree_desc t;\n@@ -733,8 +733,7 @@ static int checkout(int submodule_progress)\n \t/* We need to be in the new work tree for the checkout */\n \tsetup_work_tree();\n \n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \tmemset(&opts, 0, sizeof opts);\n \topts.update = 1;\n@@ -750,7 +749,7 @@ static int checkout(int submodule_progress)\n \tif (unpack_trees(1, &t, &opts) < 0)\n \t\tdie(_(\"unable to checkout working tree\"));\n \n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n \terr |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 7e3ebcea3..91995965d 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -203,17 +203,16 @@ static int builtin_diff_combined(struct rev_info *revs,\n \n static void refresh_index_quietly(void)\n {\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tint fd;\n \n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\tfd = hold_locked_index(lock_file, 0);\n+\tfd = hold_locked_index(&lock_file, 0);\n \tif (fd < 0)\n \t\treturn;\n \tdiscard_cache();\n \tread_cache();\n \trefresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);\n-\tupdate_index_if_able(&the_index, lock_file);\n+\tupdate_index_if_able(&the_index, &lock_file);\n }\n \n static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv)\ndiff --git a/config.c b/config.c\nindex 345d78c2b..0e61e863d 100644\n--- a/config.c\n+++ b/config.c\n@@ -2748,7 +2748,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n {\n \tint ret = 0, remove = 0;\n \tchar *filename_buf = NULL;\n-\tstruct lock_file *lock;\n+\tstruct lock_file lock = LOCK_INIT;\n \tint out_fd;\n \tchar buf[1024];\n \tFILE *config_file = NULL;\n@@ -2762,8 +2762,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n \tif (!config_filename)\n \t\tconfig_filename = filename_buf = git_pathdup(\"config\");\n \n-\tlock = xcalloc(1, sizeof(struct lock_file));\n-\tout_fd = hold_lock_file_for_update(lock, config_filename, 0);\n+\tout_fd = hold_lock_file_for_update(&lock, config_filename, 0);\n \tif (out_fd < 0) {\n \t\tret = error(\"could not lock config file %s\", config_filename);\n \t\tgoto out;\n@@ -2782,9 +2781,9 @@ int git_config_rename_section_in_file(const char *config_filename,\n \t\tgoto out;\n \t}\n \n-\tif (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {\n+\tif (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {\n \t\tret = error_errno(\"chmod on %s failed\",\n-\t\t\t\t get_lock_file_path(lock));\n+\t\t\t\t get_lock_file_path(&lock));\n \t\tgoto out;\n \t}\n \n@@ -2805,7 +2804,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 (write_section(out_fd, new_name) < 0) {\n-\t\t\t\t\tret = write_error(get_lock_file_path(lock));\n+\t\t\t\t\tret = write_error(get_lock_file_path(&lock));\n \t\t\t\t\tgoto out;\n \t\t\t\t}\n \t\t\t\t/*\n@@ -2831,20 +2830,20 @@ 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) < 0) {\n-\t\t\tret = write_error(get_lock_file_path(lock));\n+\t\t\tret = write_error(get_lock_file_path(&lock));\n \t\t\tgoto out;\n \t\t}\n \t}\n \tfclose(config_file);\n \tconfig_file = NULL;\n commit_and_out:\n-\tif (commit_lock_file(lock) < 0)\n+\tif (commit_lock_file(&lock) < 0)\n \t\tret = error_errno(\"could not write config file %s\",\n \t\t\t\t config_filename);\n out:\n \tif (config_file)\n \t\tfclose(config_file);\n-\trollback_lock_file(lock);\n+\trollback_lock_file(&lock);\n out_no_rollback:\n \tfree(filename_buf);\n \treturn ret;\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 1d3f8f0d2..24c5c26a6 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2162,7 +2162,7 @@ int merge_recursive_generic(struct merge_options *o,\n \t\t\t struct commit **result)\n {\n \tint clean;\n-\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n+\tstruct lock_file lock = LOCK_INIT;\n \tstruct commit *head_commit = get_ref(head, o->branch1);\n \tstruct commit *next_commit = get_ref(merge, o->branch2);\n \tstruct commit_list *ca = NULL;\n@@ -2178,14 +2178,14 @@ int merge_recursive_generic(struct merge_options *o,\n \t\t}\n \t}\n \n-\thold_locked_index(lock, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n \tclean = merge_recursive(o, head_commit, next_commit, ca,\n \t\t\tresult);\n \tif (clean < 0)\n \t\treturn clean;\n \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 err(o, _(\"Unable to write index.\"));\n \n \treturn clean ? 0 : 1;\ndiff --git a/merge.c b/merge.c\nindex 1d441ad94..a18a452b5 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -53,11 +53,11 @@ int checkout_fast_forward(const struct object_id *head,\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tint i, nr_trees = 0;\n \tstruct dir_struct dir;\n-\tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n+\tstruct lock_file lock_file = LOCK_INIT;\n \n \trefresh_cache(REFRESH_QUIET);\n \n-\tif (hold_locked_index(lock_file, LOCK_REPORT_ON_ERROR) < 0)\n+\tif (hold_locked_index(&lock_file, LOCK_REPORT_ON_ERROR) < 0)\n \t\treturn -1;\n \n \tmemset(&trees, 0, sizeof(trees));\n@@ -91,8 +91,8 @@ int checkout_fast_forward(const struct object_id *head,\n \t}\n \tif (unpack_trees(nr_trees, t, &opts))\n \t\treturn -1;\n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) {\n-\t\trollback_lock_file(lock_file);\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) {\n+\t\trollback_lock_file(&lock_file);\n \t\treturn error(_(\"unable to write new index file\"));\n \t}\n \treturn 0;\ndiff --git a/wt-status.c b/wt-status.c\nindex 6f730ee8f..7d88e7ca8 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -2299,14 +2299,14 @@ int has_uncommitted_changes(int ignore_submodules)\n */\n int require_clean_work_tree(const char *action, const char *hint, int ignore_submodules, int gently)\n {\n-\tstruct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tint err = 0, fd;\n \n-\tfd = hold_locked_index(lock_file, 0);\n+\tfd = hold_locked_index(&lock_file, 0);\n \trefresh_cache(REFRESH_QUIET);\n \tif (0 <= fd)\n-\t\tupdate_index_if_able(&the_index, lock_file);\n-\trollback_lock_file(lock_file);\n+\t\tupdate_index_if_able(&the_index, &lock_file);\n+\trollback_lock_file(&lock_file);\n \n \tif (has_unstaged_changes(ignore_submodules)) {\n \t\t/* TRANSLATORS: the action is e.g. \"pull with rebase\" */\n-- \n2.14.1.727.g9ddaf86\n\n"},{"id":"329291","messageId":"619c2cad465cf3994ef84908c97ad4c18ea26e78.1506862824.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1506862824.git.martin.agren@gmail.com","subject":"[PATCH 03/11] lockfile: fix documentation on `close_lock_file_gently()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-01T14:56:04Z","receivedAt":"2017-10-01T14:56:46Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Commit 83a3069a3 (lockfile: do not rollback lock on failed close,\n2017-09-05) forgot to update the documentation by the function definition\nto reflect that the lock is not rolled back in case closing fails.\n\nSigned-off-by: Martin Ågren \n---\n lockfile.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/lockfile.h b/lockfile.h\nindex 7c1c484d7..f401c979f 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -240,8 +240,8 @@ extern char *get_locked_file_path(struct lock_file *lk);\n * If the lockfile is still open, close it (and the file pointer if it\n * has been opened using `fdopen_lock_file()`) without renaming the\n * lockfile over the file being locked. Return 0 upon success. On\n- * failure to `close(2)`, return a negative value and roll back the\n- * lock file. Usually `commit_lock_file()`, `commit_lock_file_to()`,\n+ * failure to `close(2)`, return a negative value (the lockfile is not\n+ * rolled back). Usually `commit_lock_file()`, `commit_lock_file_to()`,\n * or `rollback_lock_file()` should eventually be called.\n */\n static inline int close_lock_file_gently(struct lock_file *lk)\n-- \n2.14.1.727.g9ddaf86\n\n"},{"id":"329292","messageId":"17640b932b49cf86d53a0149300f65f01bff9135.1506862824.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1506862824.git.martin.agren@gmail.com","subject":"[PATCH 04/11] tempfile: fix documentation on `delete_tempfile()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-01T14:56:05Z","receivedAt":"2017-10-01T14:56:50Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The function has always been documented as returning 0 or -1. It is in\nfact `void`. Correct that. As part of the rearrangements we lose the\nmention that `delete_tempfile()` might set `errno`. Because there is\nno return value, the user can't really know whether it did anyway.\n\nSigned-off-by: Martin Ågren \n---\n tempfile.h | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/tempfile.h b/tempfile.h\nindex b8f4b5e14..450908b2e 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -68,10 +68,10 @@\n * `create_tempfile()` returns an allocated tempfile on success or NULL\n * on failure. On errors, `errno` describes the reason for failure.\n *\n- * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile_gently()`\n- * return 0 on success. On failure they set `errno` appropriately and return\n- * -1. `delete` and `rename` (but not `close`) do their best to delete the\n- * temporary file before returning.\n+ * `rename_tempfile()` and `close_tempfile_gently()` return 0 on success.\n+ * On failure they set `errno` appropriately and return -1.\n+ * `delete_tempfile()` and `rename` (but not `close`) do their best to\n+ * delete the temporary file before returning.\n */\n \n struct tempfile {\n-- \n2.14.1.727.g9ddaf86\n\n"},{"id":"329293","messageId":"3475f0eee33ca0c4b2bc928191f0b06576ee29fb.1506862824.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1506862824.git.martin.agren@gmail.com","subject":"[PATCH 05/11] cache-tree: simplify locking logic","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-01T14:56:06Z","receivedAt":"2017-10-01T14:56:54Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"After we have taken the lock using `LOCK_DIE_ON_ERROR`, we know that\n`newfd` is non-negative. So when we check for exactly that property\nbefore calling `write_locked_index()`, the outcome is guaranteed.\n\nIf we write and commit successfully, we set `newfd = -1`, so that we can\nlater avoid calling `rollback_lock_file` on an already-committed lock.\nBut we might just as well unconditionally call `rollback_lock_file()` --\nit will be a no-op if we have already committed.\n\nAll in all, we use `newfd` as a bool and the only benefit we get from it\nis that we can avoid calling a no-op. Remove `newfd` so that we have one\nvariable less to reason about.\n\nSigned-off-by: Martin Ågren \n---\n cache-tree.c | 12 ++++--------\n 1 file changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 71d092ed5..f646f5673 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -602,11 +602,11 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat\n \n int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, const char *index_path, int flags, const char *prefix)\n {\n-\tint entries, was_valid, newfd;\n+\tint entries, was_valid;\n \tstruct lock_file lock_file = LOCK_INIT;\n \tint ret = 0;\n \n-\tnewfd = hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);\n+\thold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);\n \n \tentries = read_index_from(index_state, index_path);\n \tif (entries < 0) {\n@@ -625,10 +625,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co\n \t\t\tret = WRITE_TREE_UNMERGED_INDEX;\n \t\t\tgoto out;\n \t\t}\n-\t\tif (0 <= newfd) {\n-\t\t\tif (!write_locked_index(index_state, &lock_file, COMMIT_LOCK))\n-\t\t\t\tnewfd = -1;\n-\t\t}\n+\t\twrite_locked_index(index_state, &lock_file, COMMIT_LOCK);\n \t\t/* Not being able to write is fine -- we are only interested\n \t\t * in updating the cache-tree part, and if the next caller\n \t\t * ends up using the old index with unupdated cache-tree part\n@@ -650,8 +647,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co\n \t\thashcpy(sha1, index_state->cache_tree->oid.hash);\n \n out:\n-\tif (0 <= newfd)\n-\t\trollback_lock_file(&lock_file);\n+\trollback_lock_file(&lock_file);\n \treturn ret;\n }\n \n-- \n2.14.1.727.g9ddaf86\n\n"},{"id":"329294","messageId":"e8b32d47ccc6c3fe3aadd70fdc280fad13f46fd8.1506862824.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1506862824.git.martin.agren@gmail.com","subject":"[PATCH 06/11] apply: move lockfile into `apply_state`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-01T14:56:07Z","receivedAt":"2017-10-01T14:56:58Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We have two users of `struct apply_state` and the related functionality\nin apply.c. Each user sets up its `apply_state` by handing over a\npointer to its static `lock_file`. (Before 076aa2cbd (tempfile:\nauto-allocate tempfiles on heap, 2017-09-05), we could never free\nlockfiles, so making them static was a reasonable approach.)\n\nOther than that, they never directly access their `lock_file`s, which\nare instead handled by the functionality in apply.c.\n\nTo make life easier for the caller and to make it less tempting for a\nfuture caller to mess with the lock, make apply.c fully responsible for\nsetting up the `lock_file`. As mentioned above, it is now safe to free a\n`lock_file`, so we can make the `struct apply_state` contain an actual\n`struct lock_file` instead of a pointer to one.\n\nThe user in builtin/apply.c is rather simple. For builtin/am.c, we might\nworry that the lock state is actually meant to be inherited across\ncalls. But the lock is only taken as `apply_all_patches()` executes, and\ncode inspection shows that it will always be released.\n\nAlternatively, we can observe that the lock itself is never queried\ndirectly. When we decide whether we should lock, we check a related\nvariable `newfd`. That variable is not inherited, so from the point of\nview of apply.c, the state machine really is reset with each call to\n`init_apply_state()`. (It would be a bug if `newfd` and the lock status\nwere not in sync. The duplication of information in `newfd` and the lock\nwill be addressed in the next patch.)\n\nSigned-off-by: Martin Ågren \n---\n apply.c | 14 +++++---------\n apply.h | 5 ++---\n builtin/am.c | 3 +--\n builtin/apply.c | 4 +---\n 4 files changed, 9 insertions(+), 17 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex c022af53a..5a6ca10a7 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -75,12 +75,10 @@ static int parse_ignorewhitespace_option(struct apply_state *state,\n }\n \n int init_apply_state(struct apply_state *state,\n-\t\t const char *prefix,\n-\t\t struct lock_file *lock_file)\n+\t\t const char *prefix)\n {\n \tmemset(state, 0, sizeof(*state));\n \tstate->prefix = prefix;\n-\tstate->lock_file = lock_file;\n \tstate->newfd = -1;\n \tstate->apply = 1;\n \tstate->line_termination = '\\n';\n@@ -146,8 +144,6 @@ int check_apply_state(struct apply_state *state, int force_apply)\n \t}\n \tif (state->check_index)\n \t\tstate->unsafe_paths = 0;\n-\tif (!state->lock_file)\n-\t\treturn error(\"BUG: state->lock_file should not be NULL\");\n \n \tif (state->apply_verbosity <= verbosity_silent) {\n \t\tstate->saved_error_routine = get_error_routine();\n@@ -4711,11 +4707,11 @@ static int apply_patch(struct apply_state *state,\n \tstate->update_index = state->check_index && state->apply;\n \tif (state->update_index && state->newfd < 0) {\n \t\tif (state->index_file)\n-\t\t\tstate->newfd = hold_lock_file_for_update(state->lock_file,\n+\t\t\tstate->newfd = hold_lock_file_for_update(&state->lock_file,\n \t\t\t\t\t\t\t\t state->index_file,\n \t\t\t\t\t\t\t\t LOCK_DIE_ON_ERROR);\n \t\telse\n-\t\t\tstate->newfd = hold_locked_index(state->lock_file, LOCK_DIE_ON_ERROR);\n+\t\t\tstate->newfd = hold_locked_index(&state->lock_file, LOCK_DIE_ON_ERROR);\n \t}\n \n \tif (state->check_index && read_apply_cache(state) < 0) {\n@@ -4911,7 +4907,7 @@ int apply_all_patches(struct apply_state *state,\n \t}\n \n \tif (state->update_index) {\n-\t\tres = write_locked_index(&the_index, state->lock_file, COMMIT_LOCK);\n+\t\tres = write_locked_index(&the_index, &state->lock_file, COMMIT_LOCK);\n \t\tif (res) {\n \t\t\terror(_(\"Unable to write new index file\"));\n \t\t\tres = -128;\n@@ -4924,7 +4920,7 @@ int apply_all_patches(struct apply_state *state,\n \n end:\n \tif (state->newfd >= 0) {\n-\t\trollback_lock_file(state->lock_file);\n+\t\trollback_lock_file(&state->lock_file);\n \t\tstate->newfd = -1;\n \t}\n \ndiff --git a/apply.h b/apply.h\nindex d9b395770..cf00cda17 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -37,7 +37,7 @@ struct apply_state {\n \tconst char *prefix;\n \n \t/* These are lock_file related */\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file;\n \tint newfd;\n \n \t/* These control what gets looked at and modified */\n@@ -116,8 +116,7 @@ extern int apply_parse_options(int argc, const char **argv,\n \t\t\t int *force_apply, int *options,\n \t\t\t const char * const *apply_usage);\n extern int init_apply_state(struct apply_state *state,\n-\t\t\t const char *prefix,\n-\t\t\t struct lock_file *lock_file);\n+\t\t\t const char *prefix);\n extern void clear_apply_state(struct apply_state *state);\n extern int check_apply_state(struct apply_state *state, int force_apply);\n \ndiff --git a/builtin/am.c b/builtin/am.c\nindex 4e16fd428..40968428d 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1488,11 +1488,10 @@ static int run_apply(const struct am_state *state, const char *index_file)\n \tstruct argv_array apply_opts = ARGV_ARRAY_INIT;\n \tstruct apply_state apply_state;\n \tint res, opts_left;\n-\tstatic struct lock_file lock_file;\n \tint force_apply = 0;\n \tint options = 0;\n \n-\tif (init_apply_state(&apply_state, NULL, &lock_file))\n+\tif (init_apply_state(&apply_state, NULL))\n \t\tdie(\"BUG: init_apply_state() failed\");\n \n \targv_array_push(&apply_opts, \"apply\");\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 81b9a61c3..48d398933 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -9,8 +9,6 @@ static const char * const apply_usage[] = {\n \tNULL\n };\n \n-static struct lock_file lock_file;\n-\n int cmd_apply(int argc, const char **argv, const char *prefix)\n {\n \tint force_apply = 0;\n@@ -18,7 +16,7 @@ int cmd_apply(int argc, const char **argv, const char *prefix)\n \tint ret;\n \tstruct apply_state state;\n \n-\tif (init_apply_state(&state, prefix, &lock_file))\n+\tif (init_apply_state(&state, prefix))\n \t\texit(128);\n \n \targc = apply_parse_options(argc, argv,\n-- \n2.14.1.727.g9ddaf86\n\n"},{"id":"329295","messageId":"cede314e90ece6e4168634f5469f6c6f9883e375.1506862824.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1506862824.git.martin.agren@gmail.com","subject":"[PATCH 07/11] apply: remove `newfd` from `struct apply_state`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-01T14:56:08Z","receivedAt":"2017-10-01T14:57:03Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Similar to a previous patch, we do not need to use `newfd` to signal\nthat we have a lockfile to clean up. We can just unconditionally call\n`rollback_lock_file`. If we do not hold the lock, it will be a no-op.\n\nWhere we check `newfd` to decide whether we need to take the lock, we\ncan instead use `is_lock_file_locked()`.\n\nSigned-off-by: Martin Ågren \n---\n apply.c | 17 ++++++-----------\n apply.h | 3 +--\n 2 files changed, 7 insertions(+), 13 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 5a6ca10a7..d676debd5 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -79,7 +79,6 @@ int init_apply_state(struct apply_state *state,\n {\n \tmemset(state, 0, sizeof(*state));\n \tstate->prefix = prefix;\n-\tstate->newfd = -1;\n \tstate->apply = 1;\n \tstate->line_termination = '\\n';\n \tstate->p_value = 1;\n@@ -4705,13 +4704,13 @@ static int apply_patch(struct apply_state *state,\n \t\tstate->apply = 0;\n \n \tstate->update_index = state->check_index && state->apply;\n-\tif (state->update_index && state->newfd < 0) {\n+\tif (state->update_index && !is_lock_file_locked(&state->lock_file)) {\n \t\tif (state->index_file)\n-\t\t\tstate->newfd = hold_lock_file_for_update(&state->lock_file,\n-\t\t\t\t\t\t\t\t state->index_file,\n-\t\t\t\t\t\t\t\t LOCK_DIE_ON_ERROR);\n+\t\t\thold_lock_file_for_update(&state->lock_file,\n+\t\t\t\t\t\t state->index_file,\n+\t\t\t\t\t\t LOCK_DIE_ON_ERROR);\n \t\telse\n-\t\t\tstate->newfd = hold_locked_index(&state->lock_file, LOCK_DIE_ON_ERROR);\n+\t\t\thold_locked_index(&state->lock_file, LOCK_DIE_ON_ERROR);\n \t}\n \n \tif (state->check_index && read_apply_cache(state) < 0) {\n@@ -4913,16 +4912,12 @@ int apply_all_patches(struct apply_state *state,\n \t\t\tres = -128;\n \t\t\tgoto end;\n \t\t}\n-\t\tstate->newfd = -1;\n \t}\n \n \tres = !!errs;\n \n end:\n-\tif (state->newfd >= 0) {\n-\t\trollback_lock_file(&state->lock_file);\n-\t\tstate->newfd = -1;\n-\t}\n+\trollback_lock_file(&state->lock_file);\n \n \tif (state->apply_verbosity <= verbosity_silent) {\n \t\tset_error_routine(state->saved_error_routine);\ndiff --git a/apply.h b/apply.h\nindex cf00cda17..dc4a01905 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -36,9 +36,8 @@ enum apply_verbosity {\n struct apply_state {\n \tconst char *prefix;\n \n-\t/* These are lock_file related */\n+\t/* Lock file */\n \tstruct lock_file lock_file;\n-\tint newfd;\n \n \t/* These control what gets looked at and modified */\n \tint apply; /* this is not a dry-run */\n-- \n2.14.1.727.g9ddaf86\n\n"},{"id":"329296","messageId":"0d0e97dc97676a23b93db795e00d67544e6c250e.1506862824.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1506862824.git.martin.agren@gmail.com","subject":"[PATCH 08/11] cache.h: document `write_locked_index()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-01T14:56:09Z","receivedAt":"2017-10-01T14:57:06Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The next patches will tweak the behavior of this function. Document it\nin order to establish a basis for those patches.\n\nSigned-off-by: Martin Ågren \n---\n cache.h | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex ea6c236e0..4605e8228 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -601,9 +601,25 @@ extern int do_read_index(struct index_state *istate, const char *path,\n extern int read_index_from(struct index_state *, const char *path);\n extern int is_index_unborn(struct index_state *);\n extern int read_index_unmerged(struct index_state *);\n+\n+/* Flags for `write_locked_index()`. */\n #define COMMIT_LOCK\t\t(1 << 0)\n #define CLOSE_LOCK\t\t(1 << 1)\n+\n+/*\n+ * Write the index while holding an already-taken lock. The flags may\n+ * contain at most one of `COMMIT_LOCK` and `CLOSE_LOCK`.\n+ *\n+ * Unless a split index is in use, write the index into the lock-file,\n+ * then commit or close it, as indicated by `flags`.\n+ *\n+ * With a split index, write the shared index to a temporary file,\n+ * adjust its permissions and rename it into place, then write the\n+ * split index to the lockfile. If the temporary file for the shared\n+ * index cannot be created, fall back to the normal case.\n+ */\n extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);\n+\n extern int discard_index(struct index_state *);\n extern void move_index_extensions(struct index_state *dst, struct index_state *src);\n extern int unmerged_index(const struct index_state *);\n-- \n2.14.1.727.g9ddaf86\n\n"},{"id":"329297","messageId":"d912e33a1395ff25c1496715d0a537858daa885a.1506862824.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1506862824.git.martin.agren@gmail.com","subject":"[PATCH 09/11] read-cache: require flags for `write_locked_index()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-01T14:56:10Z","receivedAt":"2017-10-01T14:57:09Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"`write_locked_index()` takes two flags: `COMMIT_LOCK` and `CLOSE_LOCK`.\nAt most one is allowed. But it is also possible to use no flag, i.e.,\n`0`. But when `write_locked_index()` calls `do_write_index()`, the\ntemporary file, a.k.a. the lockfile, will be closed. So passing `0` is\neffectively the same as `CLOSE_LOCK`, which seems like a bug.\n\nWe might feel tempted to restructure the code in order to close the file\nlater, or conditionally. It also feels a bit unfortunate that we simply\n\"happen\" to close the lock by way of an implementation detail of\nlockfiles. But note that we need to close the temporary file before\n`stat`-ing it, at least on Windows. See 9f41c7a6b (read-cache: close\nindex.lock in do_write_index, 2017-04-26).\n\nSo let's punt on the restructuring. Instead, require that one of the\nflags is set. Adjust documentation and the assert we already have for\nchecking that we don't have too many flags. Add a macro `HAS_SINGLE_BIT`\n(inspired by `HAS_MULTI_BITS`) to simplify this check and similar checks\nin the future.\n\nWhen (if) we need \"write the index and leave the lockfile open\", we can\nrevisit this.\n\nSigned-off-by: Martin Ågren \n---\n cache.h | 4 ++--\n git-compat-util.h | 7 ++++++-\n read-cache.c | 12 ++++++------\n 3 files changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 4605e8228..32aa8afdf 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -607,8 +607,8 @@ extern int read_index_unmerged(struct index_state *);\n #define CLOSE_LOCK\t\t(1 << 1)\n \n /*\n- * Write the index while holding an already-taken lock. The flags may\n- * contain at most one of `COMMIT_LOCK` and `CLOSE_LOCK`.\n+ * Write the index while holding an already-taken lock. The flags must\n+ * contain precisely one of `COMMIT_LOCK` and `CLOSE_LOCK`.\n *\n * Unless a split index is in use, write the index into the lock-file,\n * then commit or close it, as indicated by `flags`.\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex cedad4d58..fa8a5a95c 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -111,7 +111,12 @@\n #endif\n \n #define MSB(x, bits) ((x) & TYPEOF(x)(~0ULL << (bitsizeof(x) - (bits))))\n-#define HAS_MULTI_BITS(i) ((i) & ((i) - 1)) /* checks if an integer has more than 1 bit set */\n+\n+/* Checks if an integer has more than 1 bit set. */\n+#define HAS_MULTI_BITS(i) ((i) & ((i) - 1))\n+\n+/* Checks if an integer has precisely 1 bit set. */\n+#define HAS_SINGLE_BIT(i) ((i) && !HAS_MULTI_BITS(i))\n \n #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))\n \ndiff --git a/read-cache.c b/read-cache.c\nindex 65f4fe837..1ec2e8304 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2343,14 +2343,14 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l\n \tint ret = do_write_index(istate, lock->tempfile, 0);\n \tif (ret)\n \t\treturn ret;\n-\tassert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=\n-\t (COMMIT_LOCK | CLOSE_LOCK));\n+\tassert(HAS_SINGLE_BIT(flags & (COMMIT_LOCK | CLOSE_LOCK)));\n \tif (flags & COMMIT_LOCK)\n \t\treturn commit_locked_index(lock);\n-\telse if (flags & CLOSE_LOCK)\n-\t\treturn close_lock_file_gently(lock);\n-\telse\n-\t\treturn ret;\n+\t/*\n+\t * Otherwise it's CLOSE_LOCK. The lockfile already happens\n+\t * to have been closed, but let's be specific.\n+\t */\n+\treturn close_lock_file_gently(lock);\n }\n \n static int write_split_index(struct index_state *istate,\n-- \n2.14.1.727.g9ddaf86\n\n"},{"id":"329298","messageId":"6d36f0a8e3c7b81e8c815782fb67fc69f86f2837.1506862824.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1506862824.git.martin.agren@gmail.com","subject":"[PATCH 10/11] read-cache: don't leave dangling pointer in `do_write_index()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-01T14:56:11Z","receivedAt":"2017-10-01T14:57:15Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"If `do_write_index(..., struct tempfile *, ...)` fails to close the\ntemporary file, it deletes it. This resets the pointer to NULL, but only\nthe pointer which is local to `do_write_index()`, not the pointer that\nthe caller holds. If the caller ever dereferences its pointer, we have\nundefined behavior (most likely a crash). One of the two callers tries\nto delete the temporary file on error, so it will dereference it.\n\nWe could change the function to take a `struct tempfile **` instead. But\nwe have another problem here. The other caller, `write_locked_index()`,\npasses in `lock->tempfile`. So if we close the temporary file and reset\n`lock->tempfile` to NULL, we have effectively rolled back the lock. That\ncaller is `write_locked_index()` and if it is used with the\n`CLOSE_LOCK`-file, it means the lock is being rolled back against the\nwishes of the caller. (`write_locked_index()` used to call\n`close_lockfile()`, which would have rolled back on error. Commit\n83a3069a3 (lockfile: do not rollback lock on failed close, 2017-09-05)\nchanged to not rolling back.)\n\nNote also that if we would ever have failed to close the lockfile,\neither with `COMMIT_LOCK` or with `CLOSE_LOCK`, that would have happened\nalready in `do_write_index()` and we would have left the lock in such a\nstate that trying to, e.g., roll it back or reopen the file would most\nlikely have crashed.\n\nDo not delete the temporary file in `do_write_index()`. One caller will\navoid rolling back the lock, the other caller will delete its temporary\nfile anyway, and for both callers we will avoid crashes.\n\nExpand the documentation on `write_locked_index()` to note that the lock\nwill never be rolled back when using `CLOSE_LOCK`. We can not yet make a\nsimilar claim about `COMMIT_LOCK`; we'll fix that in the next commit.\n\nIt does feel a bit unfortunate that we simply \"happen\" to close the lock\nby way of an implementation-detail of lockfiles. But note that we need\nto close the temporary file before `stat`-ing it, at least on Windows.\nSee 9f41c7a6b (read-cache: close index.lock in do_write_index,\n2017-04-26).\n\nSigned-off-by: Martin Ågren \n---\n cache.h | 2 ++\n read-cache.c | 1 -\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/cache.h b/cache.h\nindex 32aa8afdf..4d09da792 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -617,6 +617,8 @@ extern int read_index_unmerged(struct index_state *);\n * adjust its permissions and rename it into place, then write the\n * split index to the lockfile. If the temporary file for the shared\n * index cannot be created, fall back to the normal case.\n+ *\n+ * With `CLOSE_LOCK`, the lock will be neither committed nor rolled back.\n */\n extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);\n \ndiff --git a/read-cache.c b/read-cache.c\nindex 1ec2e8304..8e498e879 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2314,7 +2314,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\treturn -1;\n \tif (close_tempfile_gently(tempfile)) {\n \t\terror(_(\"could not close '%s'\"), tempfile->filename.buf);\n-\t\tdelete_tempfile(&tempfile);\n \t\treturn -1;\n \t}\n \tif (stat(tempfile->filename.buf, &st))\n-- \n2.14.1.727.g9ddaf86\n\n"},{"id":"329299","messageId":"26a9c90a6478ec9ddb4ce34923e251ebe9323ab2.1506862824.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1506862824.git.martin.agren@gmail.com","subject":"[PATCH 11/11] read-cache: roll back lock on error with `COMMIT_LOCK`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-01T14:56:12Z","receivedAt":"2017-10-01T14:57:17Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"When `write_locked_index(..., lock, COMMIT_LOCK)` returns, the lockfile\nmight have been neither committed nor rolled back. This happens if the\ncall to `do_write_index()` early in `do_write_locked_index()` fails, or\nif `write_shared_index()` fails.\n\nSome callers obviously do not expect the lock to remain locked. They\nsimply kick an error up the call stack without rolling back the lock.\nThey typically have the lock on the stack, so there is now no way of\nreleasing it. (Some callers actually do roll back the lock, and many\nothers simply do not care because they are about to die anyway.) Maybe\nwe don't try to take the lock again within the same process, so this is\nnot a problem at the time, but if we ever learn to try, we could\ndeadlock.\n\nWe could teach existing callers to roll back where it matters. That\nwould introduce some boiler-plate code, and future users might\nre-introduce the same mistake. After all, it does seem reasonable to\nexpect that `COMMIT_LOCK` does what `commit_lockfile()` does: commits or\nrolls back.\n\nTeach `write_locked_index(..., COMMIT_LOCK)` to roll back the lock\nbefore returning. If we have already succeeded and committed, it will be\na noop. Simplify the existing callers where we now have a superfluous\ncall to `rollback_lockfile()`. This should keep future readers from\nwondering why the callers are inconsistent.\n\nFor the users which `die()` if `write_locked_index()` fails, this change\ndoes not bring any benefit. But it also doesn't hurt -- it simply means\nwe clean up the lockfile in `do_write_locked_index()` instead of in our\ncustom `atexit(3)` handler.\n\nOut-of-tree callers will be affected by this change, but they will be\ngetting the locking-behavior that they almost certainly expect.\n\nFor the other flag, `CLOSE_LOCK`, leaving the lock as-is on error is\nappropriate, since that is how `close_lockfile_gently()` behaves.\n(There are not many in-tree users and they all `die()` on error.)\n\nSigned-off-by: Martin Ågren \n---\n builtin/difftool.c | 1 -\n cache.h | 1 +\n merge.c | 4 +---\n read-cache.c | 13 ++++++++-----\n sequencer.c | 1 -\n 5 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex b2d3ba753..bcc79d188 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -616,7 +616,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\tif (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||\n \t\t\t write_locked_index(&wtindex, &lock, COMMIT_LOCK)) {\n \t\t\t\tret = error(\"could not write %s\", buf.buf);\n-\t\t\t\trollback_lock_file(&lock);\n \t\t\t\tgoto finish;\n \t\t\t}\n \t\t\tchanged_files(&wt_modified, buf.buf, workdir);\ndiff --git a/cache.h b/cache.h\nindex 4d09da792..0da90a545 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -618,6 +618,7 @@ extern int read_index_unmerged(struct index_state *);\n * split index to the lockfile. If the temporary file for the shared\n * index cannot be created, fall back to the normal case.\n *\n+ * With `COMMIT_LOCK`, the lock is always committed or rolled back.\n * With `CLOSE_LOCK`, the lock will be neither committed nor rolled back.\n */\n extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);\ndiff --git a/merge.c b/merge.c\nindex a18a452b5..e5d796c9f 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -91,9 +91,7 @@ int checkout_fast_forward(const struct object_id *head,\n \t}\n \tif (unpack_trees(nr_trees, t, &opts))\n \t\treturn -1;\n-\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) {\n-\t\trollback_lock_file(&lock_file);\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\treturn error(_(\"unable to write new index file\"));\n-\t}\n \treturn 0;\n }\ndiff --git a/read-cache.c b/read-cache.c\nindex 8e498e879..53f1941c9 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2182,9 +2182,8 @@ static int has_racy_timestamp(struct index_state *istate)\n void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)\n {\n \tif ((istate->cache_changed || has_racy_timestamp(istate)) &&\n-\t verify_index(istate) &&\n-\t write_locked_index(istate, lockfile, COMMIT_LOCK))\n-\t\trollback_lock_file(lockfile);\n+\t verify_index(istate))\n+\t\twrite_locked_index(istate, lockfile, COMMIT_LOCK);\n }\n \n static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n@@ -2498,7 +2497,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \t (istate->cache_changed & ~EXTMASK)) {\n \t\tif (si)\n \t\t\thashclr(si->base_sha1);\n-\t\treturn do_write_locked_index(istate, lock, flags);\n+\t\tret = do_write_locked_index(istate, lock, flags);\n+\t\tgoto out;\n \t}\n \n \tif (getenv(\"GIT_TEST_SPLIT_INDEX\")) {\n@@ -2514,7 +2514,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \tif (new_shared_index) {\n \t\tret = write_shared_index(istate, lock, flags);\n \t\tif (ret)\n-\t\t\treturn ret;\n+\t\t\tgoto out;\n \t}\n \n \tret = write_split_index(istate, lock, flags);\n@@ -2523,6 +2523,9 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \tif (!ret && !new_shared_index)\n \t\tfreshen_shared_index(sha1_to_hex(si->base_sha1), 1);\n \n+out:\n+\tif (flags & COMMIT_LOCK)\n+\t\trollback_lock_file(lock);\n \treturn ret;\n }\n \ndiff --git a/sequencer.c b/sequencer.c\nindex 60636ce54..d56c38081 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1183,7 +1183,6 @@ static int read_and_refresh_cache(struct replay_opts *opts)\n \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n \tif (the_index.cache_changed && index_fd >= 0) {\n \t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {\n-\t\t\trollback_lock_file(&index_lock);\n \t\t\treturn error(_(\"git %s: failed to refresh the index\"),\n \t\t\t\t_(action_name(opts)));\n \t\t}\n-- \n2.14.1.727.g9ddaf86\n\n"},{"id":"329369","messageId":"xmqqpoa6qnp5.fsf@gitster.mtv.corp.google.com","threadId":"46872","inReplyTo":"cover.1506862824.git.martin.agren@gmail.com","subject":"Re: [PATCH 00/11] various lockfile-leaks and -fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-02T02:37:58Z","receivedAt":"2017-10-02T02:38:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> Martin Ågren (11):\n> sha1_file: do not leak `lock_file`\n> treewide: prefer lockfiles on the stack\n> lockfile: fix documentation on `close_lock_file_gently()`\n> tempfile: fix documentation on `delete_tempfile()`\n> cache-tree: simplify locking logic\n> apply: move lockfile into `apply_state`\n> apply: remove `newfd` from `struct apply_state`\n> cache.h: document `write_locked_index()`\n> read-cache: require flags for `write_locked_index()`\n> read-cache: don't leave dangling pointer in `do_write_index()`\n> read-cache: roll back lock on error with `COMMIT_LOCK`\n>\n> apply.c | 25 ++++++++-----------------\n> apply.h | 8 +++-----\n> builtin/am.c | 27 ++++++++++++---------------\n> builtin/apply.c | 4 +---\n> builtin/checkout.c | 14 ++++++--------\n> builtin/clone.c | 7 +++----\n> builtin/diff.c | 7 +++----\n> builtin/difftool.c | 1 -\n> cache-tree.c | 12 ++++--------\n> cache.h | 19 +++++++++++++++++++\n> config.c | 17 ++++++++---------\n> git-compat-util.h | 7 ++++++-\n> lockfile.h | 4 ++--\n> merge-recursive.c | 6 +++---\n> merge.c | 8 +++-----\n> read-cache.c | 26 ++++++++++++++------------\n> sequencer.c | 1 -\n> sha1_file.c | 16 +++++++---------\n> tempfile.h | 8 ++++----\n> wt-status.c | 8 ++++----\n> 20 files changed, 110 insertions(+), 115 deletions(-)\n\nThat's quite a lot of changes to the low-level code. I'll need to\nrevisit this topic later once again, but from a cursory read did not\nfind anything glaringly wrong in it. Thanks for working on this.\n"},{"id":"329370","messageId":"xmqqlgkuqkyi.fsf@gitster.mtv.corp.google.com","threadId":"46872","inReplyTo":"f241038eeed0f5f8bb346b394495356c20884a39.1506862824.git.martin.agren@gmail.com","subject":"Re: [PATCH 02/11] treewide: prefer lockfiles on the stack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-02T03:37:09Z","receivedAt":"2017-10-02T03:37:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> diff --git a/builtin/diff.c b/builtin/diff.c\n> index 7e3ebcea3..91995965d 100644\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n> @@ -203,17 +203,16 @@ static int builtin_diff_combined(struct rev_info *revs,\n> \n> static void refresh_index_quietly(void)\n> {\n> -\tstruct lock_file *lock_file;\n> +\tstruct lock_file lock_file = LOCK_INIT;\n> \tint fd;\n> \n> -\tlock_file = xcalloc(1, sizeof(struct lock_file));\n> -\tfd = hold_locked_index(lock_file, 0);\n> +\tfd = hold_locked_index(&lock_file, 0);\n> \tif (fd < 0)\n> \t\treturn;\n> \tdiscard_cache();\n> \tread_cache();\n> \trefresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);\n> -\tupdate_index_if_able(&the_index, lock_file);\n> +\tupdate_index_if_able(&the_index, &lock_file);\n> }\n\nThis is not a show-stopper for this patch series, but just something\nI noticed, something that used to be unavoidable in the old world\norder that requires us to leak lockfiles, but now it becomes more\nobvious.\n\nupdate_index_if_able() calls write_lock_index() with the COMMIT_LOCK\noption, but it does so conditionally. When it does not make the call,\nthe lockfile is left behind to be cleaned up by the atexit() handler,\nbut when it does, it is closed and released.\n\nPerhaps update_index_if_able() needs to be further cleaned up to\nalways release the resource held by the lockfile structure? Its\ncaller cannot differentiate (and more importantly, its caller does\nnot want to care) if the _if_able call actually has updated the\nindex or not and act differently afterwards.\n\n"},{"id":"329371","messageId":"xmqqh8viqksm.fsf@gitster.mtv.corp.google.com","threadId":"46872","inReplyTo":"3475f0eee33ca0c4b2bc928191f0b06576ee29fb.1506862824.git.martin.agren@gmail.com","subject":"Re: [PATCH 05/11] cache-tree: simplify locking logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-02T03:40:41Z","receivedAt":"2017-10-02T03:40:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> After we have taken the lock using `LOCK_DIE_ON_ERROR`, we know that\n> `newfd` is non-negative. So when we check for exactly that property\n> before calling `write_locked_index()`, the outcome is guaranteed.\n>\n> If we write and commit successfully, we set `newfd = -1`, so that we can\n> later avoid calling `rollback_lock_file` on an already-committed lock.\n> But we might just as well unconditionally call `rollback_lock_file()` --\n> it will be a no-op if we have already committed.\n>\n> All in all, we use `newfd` as a bool and the only benefit we get from it\n> is that we can avoid calling a no-op. Remove `newfd` so that we have one\n> variable less to reason about.\n\nNicely explained. Makes sense.\n\nThanks.\n"},{"id":"329372","messageId":"xmqqd166qkeg.fsf@gitster.mtv.corp.google.com","threadId":"46872","inReplyTo":"d912e33a1395ff25c1496715d0a537858daa885a.1506862824.git.martin.agren@gmail.com","subject":"Re: [PATCH 09/11] read-cache: require flags for `write_locked_index()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-02T03:49:11Z","receivedAt":"2017-10-02T03:49:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> ... Instead, require that one of the\n> flags is set. Adjust documentation and the assert we already have for\n> checking that we don't have too many flags. Add a macro `HAS_SINGLE_BIT`\n> (inspired by `HAS_MULTI_BITS`) to simplify this check and similar checks\n> in the future.\n\nI do not have a strong opinion against this approach, but if\nsomething can take only one of two values, wouldn't it make more\nsense to express it as a single boolean, I wonder. Then there is no\nneed to invent a cute HAS_SINGLE_BIT() macro, either.\n\n\"commit and leave it open\" cannot be expressed with such a scheme,\nbut with the HAS_SINGLE_BIT() scheme it can't anyway, so...\n"},{"id":"329373","messageId":"xmqq60byqjtr.fsf@gitster.mtv.corp.google.com","threadId":"46872","inReplyTo":"26a9c90a6478ec9ddb4ce34923e251ebe9323ab2.1506862824.git.martin.agren@gmail.com","subject":"Re: [PATCH 11/11] read-cache: roll back lock on error with `COMMIT_LOCK`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-02T04:01:36Z","receivedAt":"2017-10-02T04:01:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> Teach `write_locked_index(..., COMMIT_LOCK)` to roll back the lock\n> before returning. If we have already succeeded and committed, it will be\n> a noop. Simplify the existing callers where we now have a superfluous\n> call to `rollback_lockfile()`. This should keep future readers from\n> wondering why the callers are inconsistent.\n\nMakes sense. Thanks for cleaning up. The split-index codepath was\nsomething I've never reviewed seriously with fine toothed comb, and\nI feel a lot better now knowing that somebody other than its authors\nand me have read it through ;-)\n"},{"id":"329375","messageId":"CAN0heSq-7CzyRV2KhzvC8CymoyMOWydPDi0kS5cg1BS7=oCELQ@mail.gmail.com","threadId":"46872","inReplyTo":"xmqqlgkuqkyi.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 02/11] treewide: prefer lockfiles on the stack","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-02T04:12:29Z","receivedAt":"2017-10-02T04:12:34Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 2 October 2017 at 05:37, Junio C Hamano wrote:\n> Martin Ågren writes:\n>\n> This is not a show-stopper for this patch series, but just something\n> I noticed, something that used to be unavoidable in the old world\n> order that requires us to leak lockfiles, but now it becomes more\n> obvious.\n>\n> update_index_if_able() calls write_lock_index() with the COMMIT_LOCK\n> option, but it does so conditionally. When it does not make the call,\n> the lockfile is left behind to be cleaned up by the atexit() handler,\n> but when it does, it is closed and released.\n>\n> Perhaps update_index_if_able() needs to be further cleaned up to\n> always release the resource held by the lockfile structure? Its\n> caller cannot differentiate (and more importantly, its caller does\n> not want to care) if the _if_able call actually has updated the\n> index or not and act differently afterwards.\n\nAh, indeed. I should be able to do that rather easily on top. I tend to\nagree that the caller shouldn't have to worry about it since the\nfunction is just a one-off \"if_able\" without any return value. One\ncaller actually does roll back the lock, but the others don't.\n"},{"id":"329377","messageId":"CAN0heSqPVpgYSfUpmiBaCVtZkMiqWC3rgzFcBu2TVr3m7Gq4UA@mail.gmail.com","threadId":"46872","inReplyTo":"xmqqd166qkeg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 09/11] read-cache: require flags for `write_locked_index()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-02T04:14:03Z","receivedAt":"2017-10-02T04:14:08Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 2 October 2017 at 05:49, Junio C Hamano wrote:\n> Martin Ågren writes:\n>\n>> ... Instead, require that one of the\n>> flags is set. Adjust documentation and the assert we already have for\n>> checking that we don't have too many flags. Add a macro `HAS_SINGLE_BIT`\n>> (inspired by `HAS_MULTI_BITS`) to simplify this check and similar checks\n>> in the future.\n>\n> I do not have a strong opinion against this approach, but if\n> something can take only one of two values, wouldn't it make more\n> sense to express it as a single boolean, I wonder. Then there is no\n> need to invent a cute HAS_SINGLE_BIT() macro, either.\n>\n> \"commit and leave it open\" cannot be expressed with such a scheme,\n> but with the HAS_SINGLE_BIT() scheme it can't anyway, so...\n\nI did briefly consider renaming `flags` to `commit` and re-#defining the\ntwo flags to 0 and 1 (or even updating all the callers to use literal\nzeros and ones). It felt a bit awkward to downgrade `flags` to a bool\n-- normally we'd to the reverse change. But maybe I shouldn't have\nrejected that so easily. If we have a feeling we won't need other flags\n(or the \"don't even close the file\") any time soon, maybe it'd be good\nto tighten things up a bit. Thanks for looking at these.\n"},{"id":"329400","messageId":"20171002052659.hgzkhz4sgez3mkgz@sigill.intra.peff.net","threadId":"46872","inReplyTo":"9725917d9854a54e4daeda70940c3bb0ff5d9201.1506862824.git.martin.agren@gmail.com","subject":"Re: [PATCH 01/11] sha1_file: do not leak `lock_file`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T05:26:59Z","receivedAt":"2017-10-02T05:27:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 01, 2017 at 04:56:02PM +0200, Martin Ågren wrote:\n\n> There is no longer any need to allocate and leak a `struct lock_file`.\n> Initialize it on the stack instead.\n> \n> Instead of setting `lock = NULL` to signal that we have already rolled\n> back, and that we should not do any more work, check with\n> `is_lock_file_locked()`. Since we take the lock with\n> `LOCK_DIE_ON_ERROR`, it evaluates to false exactly when we have called\n> `rollback_lock_file()`.\n\nThis looks correct (and is a good direction, as it drops a leak).\n\nThe original code is actually a bit confusing/unsafe, as we set the\n\"found\" flag early and rollback here:\n\n> @@ -481,17 +481,15 @@ void add_to_alternates_file(const char *reference)\n> \t\tstrbuf_release(&line);\n> \t\tfclose(in);\n> \n> -\t\tif (found) {\n> -\t\t\trollback_lock_file(lock);\n> -\t\t\tlock = NULL;\n> -\t\t}\n> +\t\tif (found)\n> +\t\t\trollback_lock_file(&lock);\n\nand that leaves the \"out\" filehandle dangling. It's correct because of\nthe later \"do we still have the lock\" check:\n\n> -\tif (lock) {\n> +\tif (is_lock_file_locked(&lock)) {\n> \t\tfprintf_or_die(out, \"%s\\n\", reference);\n\nbut the two spots must remain in sync. If I were writing it from scratch\nI might have bumped \"found\" to the scope of the whole function, and then\nreplaced this final \"do we still have the lock\" with:\n\n if (found)\n\trollback_lock_file(&lock);\n else {\n\tfprintf_or_die(out, \"%s\\n\", reference);\n\tif (commit_lock_file(&lock))\n\t...etc...\n }\n\nI don't know if it's worth changing now or not.\n\n-Peff\n"},{"id":"329401","messageId":"20171002053438.kpi3bhycqkgkg2o5@sigill.intra.peff.net","threadId":"46872","inReplyTo":"f241038eeed0f5f8bb346b394495356c20884a39.1506862824.git.martin.agren@gmail.com","subject":"Re: [PATCH 02/11] treewide: prefer lockfiles on the stack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T05:34:38Z","receivedAt":"2017-10-02T05:34:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 01, 2017 at 04:56:03PM +0200, Martin Ågren wrote:\n\n> There is no longer any need to allocate and leak a `struct lock_file`.\n> The previous patch addressed an instance where we needed a minor tweak\n> alongside the trivial changes.\n> \n> Deal with the remaining instances where we allocate and leak a struct\n> within a single function. Change them to have the `struct lock_file` on\n> the stack instead.\n> \n> These instances were identified by running `git grep \"^\\s*struct\n> lock_file\\s*\\*\"`.\n\nThanks. These all look pretty straightforward, and as a general rule\nthis _should_ be a safe conversion with respect to dangling references,\nsince:\n\n - the tempfile pointers copied onto the global cleanup list are always\n allocated, so there's no danger of our stack variables going out of\n scope and leaving cruft in the global list.\n\n - when the lock is committed or rolled back, we NULL the pointer after\n freeing the tempfile. Nobody should be looking at the lock after\n that, but if there is such a bug, we should reliably hit an assert\n (if they use the accessors which check for validity) or a segfault\n (if somebody tries to dereference it directly).\n\n So it's possible that this subtly introduces a new segfault or\n assertion failure, but in that case it would be finding an\n _existing_ bug which was previously just reading cruft from the\n inactive tempfile struct.\n\n-Peff\n"},{"id":"329402","messageId":"20171002053531.ohalpjhfrt4b5dld@sigill.intra.peff.net","threadId":"46872","inReplyTo":"619c2cad465cf3994ef84908c97ad4c18ea26e78.1506862824.git.martin.agren@gmail.com","subject":"Re: [PATCH 03/11] lockfile: fix documentation on `close_lock_file_gently()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T05:35:31Z","receivedAt":"2017-10-02T05:35:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 01, 2017 at 04:56:04PM +0200, Martin Ågren wrote:\n\n> Commit 83a3069a3 (lockfile: do not rollback lock on failed close,\n> 2017-09-05) forgot to update the documentation by the function definition\n> to reflect that the lock is not rolled back in case closing fails.\n\nOops, thanks for catching. Patch looks obviously correct.\n\n-Peff\n"},{"id":"329403","messageId":"20171002053821.itkkmi2png3m6gdw@sigill.intra.peff.net","threadId":"46872","inReplyTo":"17640b932b49cf86d53a0149300f65f01bff9135.1506862824.git.martin.agren@gmail.com","subject":"Re: [PATCH 04/11] tempfile: fix documentation on `delete_tempfile()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T05:38:21Z","receivedAt":"2017-10-02T05:38:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 01, 2017 at 04:56:05PM +0200, Martin Ågren wrote:\n\n> The function has always been documented as returning 0 or -1. It is in\n> fact `void`. Correct that. As part of the rearrangements we lose the\n> mention that `delete_tempfile()` might set `errno`. Because there is\n> no return value, the user can't really know whether it did anyway.\n\nRight, your documentation change makes sense. Thanks for catching this.\n\nI was curious if this inconsistency was introduced during review, but\neven the initial version to the list returns void. So I guess it\nhappened sometime during the writing. :)\n\n-Peff\n"},{"id":"329404","messageId":"20171002054126.xkvv2zglefxcvbia@sigill.intra.peff.net","threadId":"46872","inReplyTo":"3475f0eee33ca0c4b2bc928191f0b06576ee29fb.1506862824.git.martin.agren@gmail.com","subject":"Re: [PATCH 05/11] cache-tree: simplify locking logic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T05:41:27Z","receivedAt":"2017-10-02T05:41:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 01, 2017 at 04:56:06PM +0200, Martin Ågren wrote:\n\n> After we have taken the lock using `LOCK_DIE_ON_ERROR`, we know that\n> `newfd` is non-negative. So when we check for exactly that property\n> before calling `write_locked_index()`, the outcome is guaranteed.\n> \n> If we write and commit successfully, we set `newfd = -1`, so that we can\n> later avoid calling `rollback_lock_file` on an already-committed lock.\n> But we might just as well unconditionally call `rollback_lock_file()` --\n> it will be a no-op if we have already committed.\n> \n> All in all, we use `newfd` as a bool and the only benefit we get from it\n> is that we can avoid calling a no-op. Remove `newfd` so that we have one\n> variable less to reason about.\n\nNice, this looks much simpler and the reasoning above is all sound.\n\nI think cmd_checkout_index() has the exact same thing going on.\n\n-Peff\n"},{"id":"329405","messageId":"20171002054801.ak4dxg2d7mrnqo66@sigill.intra.peff.net","threadId":"46872","inReplyTo":"e8b32d47ccc6c3fe3aadd70fdc280fad13f46fd8.1506862824.git.martin.agren@gmail.com","subject":"Re: [PATCH 06/11] apply: move lockfile into `apply_state`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T05:48:01Z","receivedAt":"2017-10-02T05:48:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 01, 2017 at 04:56:07PM +0200, Martin Ågren wrote:\n\n> We have two users of `struct apply_state` and the related functionality\n> in apply.c. Each user sets up its `apply_state` by handing over a\n> pointer to its static `lock_file`. (Before 076aa2cbd (tempfile:\n> auto-allocate tempfiles on heap, 2017-09-05), we could never free\n> lockfiles, so making them static was a reasonable approach.)\n> \n> Other than that, they never directly access their `lock_file`s, which\n> are instead handled by the functionality in apply.c.\n> \n> To make life easier for the caller and to make it less tempting for a\n> future caller to mess with the lock, make apply.c fully responsible for\n> setting up the `lock_file`. As mentioned above, it is now safe to free a\n> `lock_file`, so we can make the `struct apply_state` contain an actual\n> `struct lock_file` instead of a pointer to one.\n> \n> The user in builtin/apply.c is rather simple. For builtin/am.c, we might\n> worry that the lock state is actually meant to be inherited across\n> calls. But the lock is only taken as `apply_all_patches()` executes, and\n> code inspection shows that it will always be released.\n> \n> Alternatively, we can observe that the lock itself is never queried\n> directly. When we decide whether we should lock, we check a related\n> variable `newfd`. That variable is not inherited, so from the point of\n> view of apply.c, the state machine really is reset with each call to\n> `init_apply_state()`. (It would be a bug if `newfd` and the lock status\n> were not in sync. The duplication of information in `newfd` and the lock\n> will be addressed in the next patch.)\n\nNicely explained. I didn't re-verify your inspection of\napply_all_patches(), but I think it would be fine regardless. We don't\ninit_apply_state() again, so both before and after your patch we are\nalways looking at the \"same\" lock struct (and so if somebody forgot to\nunlock, in either case we'd hit the \"this tempfile is already active\"\nassertion).\n\n-Peff\n"},{"id":"329407","messageId":"20171002055058.uue7uajfqtkntxjg@sigill.intra.peff.net","threadId":"46872","inReplyTo":"cede314e90ece6e4168634f5469f6c6f9883e375.1506862824.git.martin.agren@gmail.com","subject":"Re: [PATCH 07/11] apply: remove `newfd` from `struct apply_state`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T05:50:58Z","receivedAt":"2017-10-02T05:51:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 01, 2017 at 04:56:08PM +0200, Martin Ågren wrote:\n\n> Similar to a previous patch, we do not need to use `newfd` to signal\n> that we have a lockfile to clean up. We can just unconditionally call\n> `rollback_lock_file`. If we do not hold the lock, it will be a no-op.\n> \n> Where we check `newfd` to decide whether we need to take the lock, we\n> can instead use `is_lock_file_locked()`.\n\nLooks good. I was surprised you didn't have to replace any writing to\n\"newfd\". But the writing it all happens via write_locked_index(), which\ndirectly accesses the lock's fd member.\n\n-Peff\n"},{"id":"329410","messageId":"20171002060018.m5dvxt2qtgkeixc7@sigill.intra.peff.net","threadId":"46872","inReplyTo":"d912e33a1395ff25c1496715d0a537858daa885a.1506862824.git.martin.agren@gmail.com","subject":"Re: [PATCH 09/11] read-cache: require flags for `write_locked_index()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T06:00:19Z","receivedAt":"2017-10-02T06:00:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 01, 2017 at 04:56:10PM +0200, Martin Ågren wrote:\n\n> `write_locked_index()` takes two flags: `COMMIT_LOCK` and `CLOSE_LOCK`.\n> At most one is allowed. But it is also possible to use no flag, i.e.,\n> `0`. But when `write_locked_index()` calls `do_write_index()`, the\n> temporary file, a.k.a. the lockfile, will be closed. So passing `0` is\n> effectively the same as `CLOSE_LOCK`, which seems like a bug.\n> \n> We might feel tempted to restructure the code in order to close the file\n> later, or conditionally. It also feels a bit unfortunate that we simply\n> \"happen\" to close the lock by way of an implementation detail of\n> lockfiles. But note that we need to close the temporary file before\n> `stat`-ing it, at least on Windows. See 9f41c7a6b (read-cache: close\n> index.lock in do_write_index, 2017-04-26).\n\nInteresting. So it seems like this is potentially a bug introduced by\n9f41c7a6b, as before then passing \"0\" for the flags would neither commit\nnor close the lockfile.\n\nAt the same time, I cannot imagine why one would want that behavior. The\nonly use would be if you planned to append more to the index. Which\nseems kind of odd to do outside of write_locked_index() itself. And\ncertainly there aren't any callers in the existing codebase who pass 0.\nYour assert() should help catch any that are added.\n\n-Peff\n"},{"id":"329411","messageId":"20171002061529.xaiqccwjrthc74gf@sigill.intra.peff.net","threadId":"46872","inReplyTo":"6d36f0a8e3c7b81e8c815782fb67fc69f86f2837.1506862824.git.martin.agren@gmail.com","subject":"Re: [PATCH 10/11] read-cache: don't leave dangling pointer in `do_write_index()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T06:15:30Z","receivedAt":"2017-10-02T06:15:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 01, 2017 at 04:56:11PM +0200, Martin Ågren wrote:\n\n> If `do_write_index(..., struct tempfile *, ...)` fails to close the\n> temporary file, it deletes it. This resets the pointer to NULL, but only\n> the pointer which is local to `do_write_index()`, not the pointer that\n> the caller holds. If the caller ever dereferences its pointer, we have\n> undefined behavior (most likely a crash). One of the two callers tries\n> to delete the temporary file on error, so it will dereference it.\n\nHrm. I think this was introduced by my lockfile series, since before\nthat the memory would have remained valid (but with its \"active\" flag\nunset, which is enough for delete_tempfile() to happily return early).\n\nPart of my reason for switching delete_tempfile() to take a\npointer-to-pointer was so that we had to investigate each such call. But\nobviously I failed to analyze this case correctly. :(\n\n> We could change the function to take a `struct tempfile **` instead. But\n> we have another problem here. The other caller, `write_locked_index()`,\n> passes in `lock->tempfile`. So if we close the temporary file and reset\n> `lock->tempfile` to NULL, we have effectively rolled back the lock. That\n> caller is `write_locked_index()` and if it is used with the\n> `CLOSE_LOCK`-file, it means the lock is being rolled back against the\n> wishes of the caller. (`write_locked_index()` used to call\n> `close_lockfile()`, which would have rolled back on error. Commit\n> 83a3069a3 (lockfile: do not rollback lock on failed close, 2017-09-05)\n> changed to not rolling back.)\n\nI'm not sure I follow here. It seems like write_locked_index() has three\noutcomes:\n\n - if there was an error, the lock is rolled back\n\n - if we were successful and the caller asked for CLOSE_LOCK, we remain\n locked\n\n - if we were successful and the caller asked for COMMIT_LOCK, we\n commit the lock\n\nIt sounds like you are considering the first outcome a bug, but I think\nit is intentional. Without that, every caller of write_locked_index()\nwould need to release the lock themselves. That's especially cumbersome\nif they called with COMMIT_LOCK, as they otherwise can assume that\nwrite_locked_index() has released the lock one way or another.\n\nSo I actually think that just switching to a \"struct tempfile **\" here\nis a reasonable solution (I'd also be fine with doing this and then\nhaving do_write_locked_index() rollback the lock itself on error).\n\nOr am I missing something?\n\n-Peff\n"},{"id":"329412","messageId":"20171002062044.j7xnmmqqiz5tzguu@sigill.intra.peff.net","threadId":"46872","inReplyTo":"20171002061529.xaiqccwjrthc74gf@sigill.intra.peff.net","subject":"Re: [PATCH 10/11] read-cache: don't leave dangling pointer in `do_write_index()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T06:20:45Z","receivedAt":"2017-10-02T06:20:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 02, 2017 at 02:15:30AM -0400, Jeff King wrote:\n\n> I'm not sure I follow here. It seems like write_locked_index() has three\n> outcomes:\n> \n> - if there was an error, the lock is rolled back\n> \n> - if we were successful and the caller asked for CLOSE_LOCK, we remain\n> locked\n> \n> - if we were successful and the caller asked for COMMIT_LOCK, we\n> commit the lock\n> \n> It sounds like you are considering the first outcome a bug, but I think\n> it is intentional. Without that, every caller of write_locked_index()\n> would need to release the lock themselves. That's especially cumbersome\n> if they called with COMMIT_LOCK, as they otherwise can assume that\n> write_locked_index() has released the lock one way or another.\n> \n> So I actually think that just switching to a \"struct tempfile **\" here\n> is a reasonable solution (I'd also be fine with doing this and then\n> having do_write_locked_index() rollback the lock itself on error).\n> \n> Or am I missing something?\n\nWell, one thing I was certainly missing was reading your patch 11. :)\n\nThat fixes the COMMIT_LOCK case. We are still changing the behavior of\nCLOSE_LOCK on error, though. The existing callers all seem to die\nimmediately so it wouldn't matter either way, but there could in theory\nbe new ones in topics-in-flight.\n\nThe other thing I was missing is that we are absolutely inconsistent\nabout this \"close on error\". It only happens for _one_ of the error\nreturns of do_write_index(), and the others would have left the file\nopen and not rolled-back.\n\nSo in retrospect, I think your approach is the right direction.\n\n-Peff\n"},{"id":"329413","messageId":"20171002062252.didw7guppl4nvxry@sigill.intra.peff.net","threadId":"46872","inReplyTo":"cover.1506862824.git.martin.agren@gmail.com","subject":"Re: [PATCH 00/11] various lockfile-leaks and -fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T06:22:52Z","receivedAt":"2017-10-02T06:22:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 01, 2017 at 04:56:01PM +0200, Martin Ågren wrote:\n\n> A recent series allowed `struct lock_file`s to be freed [1], so I wanted\n> to get rid of the \"simple\" leaks of this kind. I found a couple of\n> lock-related cleanups along the way and it resulted in this series. It\n> feels a bit scary at eleven patches -- especially as this is about\n> locking -- but I hope I've managed to put this into understandable and\n> reviewable patches. Reviews, thoughts, opinions welcome, as always.\n\nThanks for working on this (and cleaning up several messes that I left).\n\nI've read through each and with the exception of patches 10 and 11, they\nall look good to me.\n\nFor 10 and 11, I think you've convinced me that the current behavior is\nbuggy. I do wonder if the subtleties might be easier to understand as a\nsingle patch, but I'm OK either way.\n\n-Peff\n"},{"id":"329415","messageId":"xmqqbmlqoydg.fsf@gitster.mtv.corp.google.com","threadId":"46872","inReplyTo":"20171002062252.didw7guppl4nvxry@sigill.intra.peff.net","subject":"Re: [PATCH 00/11] various lockfile-leaks and -fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-02T06:30:19Z","receivedAt":"2017-10-02T06:30:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I've read through each and with the exception of patches 10 and 11, they\n> all look good to me.\n>\n> For 10 and 11, I think you've convinced me that the current behavior is\n> buggy. I do wonder if the subtleties might be easier to understand as a\n> single patch, but I'm OK either way.\n\nI found it was not too hard to understand what is going on with\n10/11 as separate patches, and I suspect that it would be also OK if\nthey were a single patch (but I cannot easily \"unsee\" them, so this\nis merely a suspicion).\n\nThanks, both. Let's merge this to 'next' soonish.\n\n\n"},{"id":"329429","messageId":"CAN0heSotumvyYDa8zsRx9cNd8LF+CgUOGgd+eFPEmwBqJsjfkA@mail.gmail.com","threadId":"46872","inReplyTo":"20171002052659.hgzkhz4sgez3mkgz@sigill.intra.peff.net","subject":"Re: [PATCH 01/11] sha1_file: do not leak `lock_file`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-02T10:15:20Z","receivedAt":"2017-10-02T10:15:25Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 2 October 2017 at 07:26, Jeff King wrote:\n> On Sun, Oct 01, 2017 at 04:56:02PM +0200, Martin Ågren wrote:\n>\n> The original code is actually a bit confusing/unsafe, as we set the\n> \"found\" flag early and rollback here:\n>\n>> @@ -481,17 +481,15 @@ void add_to_alternates_file(const char *reference)\n>> strbuf_release(&line);\n>> fclose(in);\n>>\n>> - if (found) {\n>> - rollback_lock_file(lock);\n>> - lock = NULL;\n>> - }\n>> + if (found)\n>> + rollback_lock_file(&lock);\n>\n> and that leaves the \"out\" filehandle dangling. It's correct because of\n> the later \"do we still have the lock\" check:\n>\n>> - if (lock) {\n>> + if (is_lock_file_locked(&lock)) {\n>> fprintf_or_die(out, \"%s\\n\", reference);\n>\n> but the two spots must remain in sync. If I were writing it from scratch\n> I might have bumped \"found\" to the scope of the whole function, and then\n> replaced this final \"do we still have the lock\" with:\n>\n> if (found)\n> rollback_lock_file(&lock);\n> else {\n> fprintf_or_die(out, \"%s\\n\", reference);\n> if (commit_lock_file(&lock))\n> ...etc...\n> }\n>\n> I don't know if it's worth changing now or not.\n\nI'd like to address the feedback on other commits in a re-roll. I will\nuse that opportunity to try your approach above instead of this patch.\n\nThanks.\n"},{"id":"329430","messageId":"CAN0heSpRqicsghBd5WruG9kyPqQbx8MHOP0afW9HNEpNDyp9eg@mail.gmail.com","threadId":"46872","inReplyTo":"CAN0heSqPVpgYSfUpmiBaCVtZkMiqWC3rgzFcBu2TVr3m7Gq4UA@mail.gmail.com","subject":"Re: [PATCH 09/11] read-cache: require flags for `write_locked_index()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-02T10:16:36Z","receivedAt":"2017-10-02T10:16:42Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 2 October 2017 at 06:14, Martin Ågren wrote:\n> On 2 October 2017 at 05:49, Junio C Hamano wrote:\n>> Martin Ågren writes:\n>>\n>>> ... Instead, require that one of the\n>>> flags is set. Adjust documentation and the assert we already have for\n>>> checking that we don't have too many flags. Add a macro `HAS_SINGLE_BIT`\n>>> (inspired by `HAS_MULTI_BITS`) to simplify this check and similar checks\n>>> in the future.\n>>\n>> I do not have a strong opinion against this approach, but if\n>> something can take only one of two values, wouldn't it make more\n>> sense to express it as a single boolean, I wonder. Then there is no\n>> need to invent a cute HAS_SINGLE_BIT() macro, either.\n>>\n>> \"commit and leave it open\" cannot be expressed with such a scheme,\n>> but with the HAS_SINGLE_BIT() scheme it can't anyway, so...\n>\n> I did briefly consider renaming `flags` to `commit` and re-#defining the\n> two flags to 0 and 1 (or even updating all the callers to use literal\n> zeros and ones). It felt a bit awkward to downgrade `flags` to a bool\n> -- normally we'd to the reverse change. But maybe I shouldn't have\n> rejected that so easily. If we have a feeling we won't need other flags\n> (or the \"don't even close the file\") any time soon, maybe it'd be good\n> to tighten things up a bit. Thanks for looking at these.\n\nOf course it wouldn't have to be as invasive. It could be \"the lock will\nalways be closed and with COMMIT_LOCK, it will also be committed\".\nCLOSE_LOCK would be removed and the few current users of CLOSE_LOCK\nwould be converted to use 0.\n"},{"id":"329431","messageId":"CAN0heSqsZXoiVs9s7sZ-d_X4ghU4ciG_=8ppY+w0XG535tt2EA@mail.gmail.com","threadId":"46872","inReplyTo":"xmqqbmlqoydg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 00/11] various lockfile-leaks and -fixes","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-02T10:19:55Z","receivedAt":"2017-10-02T10:20:01Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 2 October 2017 at 08:30, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> I've read through each and with the exception of patches 10 and 11, they\n>> all look good to me.\n>>\n>> For 10 and 11, I think you've convinced me that the current behavior is\n>> buggy. I do wonder if the subtleties might be easier to understand as a\n>> single patch, but I'm OK either way.\n>\n> I found it was not too hard to understand what is going on with\n> 10/11 as separate patches, and I suspect that it would be also OK if\n> they were a single patch (but I cannot easily \"unsee\" them, so this\n> is merely a suspicion).\n>\n> Thanks, both. Let's merge this to 'next' soonish.\n\nThanks both of you for your comments. Based on them, I have made the\nfollowing notes:\n\nPatch 1: Make the logic a bit more future-proof/safe.\n\nPatch 9: Instead of the two flags and the `HAS_SINGLE_BIT`-\"cuteness\",\njust drop `CLOSE_LOCK` altogether. That should simplify things a bit\nfurther. It might also help me come up with better explanations for\npatches 10-11.\n\nPatches 10-11: I'm sorry that I didn't succeed better in explaining\nthis. They have a couple of different things going on, but they are\nobviously related and even a bit entangled. Let me try and see what I\ncan do, I'll try squashing them also. Maybe with `CLOSE_LOCK` gone, I'll\ndo better.\n\nEspecially 9-11 make me feel like wanting to re-roll this, for future\nreaders if nothing else. I expect to be able to do so in the middle of\nthis week (I don't know how this interferes with Junio's definition of\n\"soonish\").\n\nAs fallout that could be taken care of now or later, I've noted this:\n\nPatch 2: A follow-up could teach `update_index_if_able()` to always\ncommit or roll back.\n\nPatch 5: A follow-up could clean up a similar construct in\n`cmd_checkout_index()`.\n\nThanks again for your input. It's much appreciated.\n\nMartin\n"},{"id":"329530","messageId":"xmqqy3osiwe6.fsf@gitster.mtv.corp.google.com","threadId":"46872","inReplyTo":"CAN0heSqsZXoiVs9s7sZ-d_X4ghU4ciG_=8ppY+w0XG535tt2EA@mail.gmail.com","subject":"Re: [PATCH 00/11] various lockfile-leaks and -fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-03T06:21:53Z","receivedAt":"2017-10-03T06:22:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> On 2 October 2017 at 08:30, Junio C Hamano wrote:\n> ...\n>> Thanks, both. Let's merge this to 'next' soonish.\n>\n> Thanks both of you for your comments. Based on them, I have made the\n> following notes:\n> ...\n> Especially 9-11 make me feel like wanting to re-roll this, for future\n> readers if nothing else. I expect to be able to do so in the middle of\n> this week (I don't know how this interferes with Junio's definition of\n> \"soonish\").\n\nOK, then I'll hold off and expect such a reroll.\n\nThanks.\n"},{"id":"329787","messageId":"cover.1507228170.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"xmqqy3osiwe6.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2 00/12] Re: various lockfile-leaks and -fixes","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-05T20:32:02Z","receivedAt":"2017-10-05T20:32:36Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 3 October 2017 at 08:21, Junio C Hamano wrote:\n> Martin Ågren writes:\n>> On 2 October 2017 at 08:30, Junio C Hamano wrote:\n>>\n>> Thanks both of you for your comments. Based on them, I have made the\n>> following notes:\n>> ...\n>> Especially 9-11 make me feel like wanting to re-roll this, for future\n>> readers if nothing else. I expect to be able to do so in the middle of\n>> this week (I don't know how this interferes with Junio's definition of\n>> \"soonish\").\n>\n> OK, then I'll hold off and expect such a reroll.\n\nHere it is. Several patches are unchanged (fetched from Junio's tree and\nreposted). There are two new patches (5, 12) to address some similar or\nrelated issues noticed by Peff and Junio.\n\nI think this got better by getting rid of `CLOSE_LOCK` entirely instead\nof playing with `HAS_SINGLE_BIT()`. Patch 11 also improved IMHO by\nsquashing patches 10-11 of v1 and presenting the issues a bit\ndifferently in the commit message. Thanks for your feedback, both of\nyou.\n\nMartin\n\nMartin Ågren (12):\n sha1_file: do not leak `lock_file`\n treewide: prefer lockfiles on the stack\n lockfile: fix documentation on `close_lock_file_gently()`\n tempfile: fix documentation on `delete_tempfile()`\n checkout-index: simplify locking logic\n cache-tree: simplify locking logic\n apply: move lockfile into `apply_state`\n apply: remove `newfd` from `struct apply_state`\n cache.h: document `write_locked_index()`\n read-cache: drop explicit `CLOSE_LOCK`-flag\n read-cache: leave lock in right state in `write_locked_index()`\n read_cache: roll back lock in `update_index_if_able()`\n\n apply.c | 25 ++++++++-----------------\n apply.h | 8 +++-----\n builtin/am.c | 27 ++++++++++++---------------\n builtin/apply.c | 4 +---\n builtin/checkout-index.c | 8 +++-----\n builtin/checkout.c | 14 ++++++--------\n builtin/clone.c | 7 +++----\n builtin/commit.c | 10 +++++-----\n builtin/diff.c | 7 +++----\n builtin/difftool.c | 1 -\n cache-tree.c | 12 ++++--------\n cache.h | 25 ++++++++++++++++++++++++-\n config.c | 17 ++++++++---------\n lockfile.h | 4 ++--\n merge-recursive.c | 6 +++---\n merge.c | 8 +++-----\n read-cache.c | 28 ++++++++++++++--------------\n sequencer.c | 1 -\n sha1_file.c | 19 ++++++++-----------\n tempfile.h | 8 ++++----\n wt-status.c | 8 ++++----\n 21 files changed, 118 insertions(+), 129 deletions(-)\n\n-- \n2.14.2.666.gea220ee40\n\n"},{"id":"329788","messageId":"7f9a7a5d57bf76925e14111efdabe77f1c946e8d.1507228170.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507228170.git.martin.agren@gmail.com","subject":"[PATCH v2 01/12] sha1_file: do not leak `lock_file`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-05T20:32:03Z","receivedAt":"2017-10-05T20:32:40Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"There is no longer any need to allocate and leak a `struct lock_file`.\nInitialize it on the stack instead.\n\nBefore this patch, we set `lock = NULL` to signal that we have already\nrolled back, and that we should not do any more work. We need to take\nanother approach now that we cannot assign NULL. We could, e.g., use\n`is_lock_file_locked()`. But we already have another variable that we\ncould use instead, `found`. Its scope is only too small.\n\nBump `found` to the scope of the whole function and rearrange the \"roll\nback or write?\"-checks to a straightforward if-else on `found`. This\nalso future-proves the code by making it obvious that we intend to take\nexactly one of these paths.\n\nImproved-by: Jeff King \nSigned-off-by: Martin Ågren \n---\nv2: Moved the rollback to the end to have an obvious if-else instead of\nretaining the original logic. Thanks Peff.\n\n sha1_file.c | 19 ++++++++-----------\n 1 file changed, 8 insertions(+), 11 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5a2014811..1d1747099 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -456,19 +456,19 @@ struct alternate_object_database *alloc_alt_odb(const char *dir)\n \n void add_to_alternates_file(const char *reference)\n {\n-\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n+\tstruct lock_file lock = LOCK_INIT;\n \tchar *alts = git_pathdup(\"objects/info/alternates\");\n \tFILE *in, *out;\n+\tint found = 0;\n \n-\thold_lock_file_for_update(lock, alts, LOCK_DIE_ON_ERROR);\n-\tout = fdopen_lock_file(lock, \"w\");\n+\thold_lock_file_for_update(&lock, alts, LOCK_DIE_ON_ERROR);\n+\tout = fdopen_lock_file(&lock, \"w\");\n \tif (!out)\n \t\tdie_errno(\"unable to fdopen alternates lockfile\");\n \n \tin = fopen(alts, \"r\");\n \tif (in) {\n \t\tstruct strbuf line = STRBUF_INIT;\n-\t\tint found = 0;\n \n \t\twhile (strbuf_getline(&line, in) != EOF) {\n \t\t\tif (!strcmp(reference, line.buf)) {\n@@ -480,18 +480,15 @@ void add_to_alternates_file(const char *reference)\n \n \t\tstrbuf_release(&line);\n \t\tfclose(in);\n-\n-\t\tif (found) {\n-\t\t\trollback_lock_file(lock);\n-\t\t\tlock = NULL;\n-\t\t}\n \t}\n \telse if (errno != ENOENT)\n \t\tdie_errno(\"unable to read alternates file\");\n \n-\tif (lock) {\n+\tif (found) {\n+\t\trollback_lock_file(&lock);\n+\t} else {\n \t\tfprintf_or_die(out, \"%s\\n\", reference);\n-\t\tif (commit_lock_file(lock))\n+\t\tif (commit_lock_file(&lock))\n \t\t\tdie_errno(\"unable to move new alternates file into place\");\n \t\tif (alt_odb_tail)\n \t\t\tlink_alt_odb_entries(reference, '\\n', NULL, 0);\n-- \n2.14.2.666.gea220ee40\n\n"},{"id":"329789","messageId":"2fdb048f30131b30ea910018719485fc8cff4b11.1507228170.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507228170.git.martin.agren@gmail.com","subject":"[PATCH v2 02/12] treewide: prefer lockfiles on the stack","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-05T20:32:04Z","receivedAt":"2017-10-05T20:32:45Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"There is no longer any need to allocate and leak a `struct lock_file`.\nThe previous patch addressed an instance where we needed a minor tweak\nalongside the trivial changes.\n\nDeal with the remaining instances where we allocate and leak a struct\nwithin a single function. Change them to have the `struct lock_file` on\nthe stack instead.\n\nThese instances were identified by running `git grep \"^\\s*struct\nlock_file\\s*\\*\"`.\n\nSigned-off-by: Martin Ågren \nSigned-off-by: Junio C Hamano \n---\nv2: Identical.\n\n builtin/am.c | 24 +++++++++++-------------\n builtin/checkout.c | 14 ++++++--------\n builtin/clone.c | 7 +++----\n builtin/diff.c | 7 +++----\n config.c | 17 ++++++++---------\n merge-recursive.c | 6 +++---\n merge.c | 8 ++++----\n wt-status.c | 8 ++++----\n 8 files changed, 42 insertions(+), 49 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex d7513f537..4e16fd428 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1134,11 +1134,11 @@ static const char *msgnum(const struct am_state *state)\n */\n static void refresh_and_write_cache(void)\n {\n-\tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n+\tstruct lock_file lock_file = LOCK_INIT;\n \n-\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \trefresh_cache(REFRESH_QUIET);\n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write index file\"));\n }\n \n@@ -1946,15 +1946,14 @@ static void am_resolve(struct am_state *state)\n */\n static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n {\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc t[2];\n \n \tif (parse_tree(head) || parse_tree(remote))\n \t\treturn -1;\n \n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \trefresh_cache(REFRESH_QUIET);\n \n@@ -1970,11 +1969,11 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n \tinit_tree_desc(&t[1], remote->buffer, remote->size);\n \n \tif (unpack_trees(2, t, &opts)) {\n-\t\trollback_lock_file(lock_file);\n+\t\trollback_lock_file(&lock_file);\n \t\treturn -1;\n \t}\n \n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n \treturn 0;\n@@ -1986,15 +1985,14 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n */\n static int merge_tree(struct tree *tree)\n {\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc t[1];\n \n \tif (parse_tree(tree))\n \t\treturn -1;\n \n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = 1;\n@@ -2005,11 +2003,11 @@ static int merge_tree(struct tree *tree)\n \tinit_tree_desc(&t[0], tree->buffer, tree->size);\n \n \tif (unpack_trees(1, t, &opts)) {\n-\t\trollback_lock_file(lock_file);\n+\t\trollback_lock_file(&lock_file);\n \t\treturn -1;\n \t}\n \n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n \treturn 0;\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3345a0d16..34c3c5b61 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -247,7 +247,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n \tstruct object_id rev;\n \tstruct commit *head;\n \tint errs = 0;\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \n \tif (opts->track != BRANCH_TRACK_UNSPECIFIED)\n \t\tdie(_(\"'%s' cannot be used with updating paths\"), \"--track\");\n@@ -275,9 +275,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\treturn run_add_interactive(revision, \"--patch=checkout\",\n \t\t\t\t\t &opts->pathspec);\n \n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\n-\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \tif (read_cache_preload(&opts->pathspec) < 0)\n \t\treturn error(_(\"index file corrupt\"));\n \n@@ -376,7 +374,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t}\n \terrs |= finish_delayed_checkout(&state);\n \n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n \tread_ref_full(\"HEAD\", 0, rev.hash, NULL);\n@@ -472,9 +470,9 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t int *writeout_error)\n {\n \tint ret;\n-\tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n+\tstruct lock_file lock_file = LOCK_INIT;\n \n-\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \tif (read_cache_preload(NULL) < 0)\n \t\treturn error(_(\"index file corrupt\"));\n \n@@ -591,7 +589,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \tif (!cache_tree_fully_valid(active_cache_tree))\n \t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n \n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n \tif (!opts->force && !opts->quiet)\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex dbddd98f8..96a3aaaa1 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -706,7 +706,7 @@ static int checkout(int submodule_progress)\n {\n \tstruct object_id oid;\n \tchar *head;\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tstruct unpack_trees_options opts;\n \tstruct tree *tree;\n \tstruct tree_desc t;\n@@ -733,8 +733,7 @@ static int checkout(int submodule_progress)\n \t/* We need to be in the new work tree for the checkout */\n \tsetup_work_tree();\n \n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \tmemset(&opts, 0, sizeof opts);\n \topts.update = 1;\n@@ -750,7 +749,7 @@ static int checkout(int submodule_progress)\n \tif (unpack_trees(1, &t, &opts) < 0)\n \t\tdie(_(\"unable to checkout working tree\"));\n \n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n \terr |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 7e3ebcea3..91995965d 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -203,17 +203,16 @@ static int builtin_diff_combined(struct rev_info *revs,\n \n static void refresh_index_quietly(void)\n {\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tint fd;\n \n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\tfd = hold_locked_index(lock_file, 0);\n+\tfd = hold_locked_index(&lock_file, 0);\n \tif (fd < 0)\n \t\treturn;\n \tdiscard_cache();\n \tread_cache();\n \trefresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);\n-\tupdate_index_if_able(&the_index, lock_file);\n+\tupdate_index_if_able(&the_index, &lock_file);\n }\n \n static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv)\ndiff --git a/config.c b/config.c\nindex 345d78c2b..0e61e863d 100644\n--- a/config.c\n+++ b/config.c\n@@ -2748,7 +2748,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n {\n \tint ret = 0, remove = 0;\n \tchar *filename_buf = NULL;\n-\tstruct lock_file *lock;\n+\tstruct lock_file lock = LOCK_INIT;\n \tint out_fd;\n \tchar buf[1024];\n \tFILE *config_file = NULL;\n@@ -2762,8 +2762,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n \tif (!config_filename)\n \t\tconfig_filename = filename_buf = git_pathdup(\"config\");\n \n-\tlock = xcalloc(1, sizeof(struct lock_file));\n-\tout_fd = hold_lock_file_for_update(lock, config_filename, 0);\n+\tout_fd = hold_lock_file_for_update(&lock, config_filename, 0);\n \tif (out_fd < 0) {\n \t\tret = error(\"could not lock config file %s\", config_filename);\n \t\tgoto out;\n@@ -2782,9 +2781,9 @@ int git_config_rename_section_in_file(const char *config_filename,\n \t\tgoto out;\n \t}\n \n-\tif (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {\n+\tif (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {\n \t\tret = error_errno(\"chmod on %s failed\",\n-\t\t\t\t get_lock_file_path(lock));\n+\t\t\t\t get_lock_file_path(&lock));\n \t\tgoto out;\n \t}\n \n@@ -2805,7 +2804,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 (write_section(out_fd, new_name) < 0) {\n-\t\t\t\t\tret = write_error(get_lock_file_path(lock));\n+\t\t\t\t\tret = write_error(get_lock_file_path(&lock));\n \t\t\t\t\tgoto out;\n \t\t\t\t}\n \t\t\t\t/*\n@@ -2831,20 +2830,20 @@ 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) < 0) {\n-\t\t\tret = write_error(get_lock_file_path(lock));\n+\t\t\tret = write_error(get_lock_file_path(&lock));\n \t\t\tgoto out;\n \t\t}\n \t}\n \tfclose(config_file);\n \tconfig_file = NULL;\n commit_and_out:\n-\tif (commit_lock_file(lock) < 0)\n+\tif (commit_lock_file(&lock) < 0)\n \t\tret = error_errno(\"could not write config file %s\",\n \t\t\t\t config_filename);\n out:\n \tif (config_file)\n \t\tfclose(config_file);\n-\trollback_lock_file(lock);\n+\trollback_lock_file(&lock);\n out_no_rollback:\n \tfree(filename_buf);\n \treturn ret;\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 1d3f8f0d2..24c5c26a6 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2162,7 +2162,7 @@ int merge_recursive_generic(struct merge_options *o,\n \t\t\t struct commit **result)\n {\n \tint clean;\n-\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n+\tstruct lock_file lock = LOCK_INIT;\n \tstruct commit *head_commit = get_ref(head, o->branch1);\n \tstruct commit *next_commit = get_ref(merge, o->branch2);\n \tstruct commit_list *ca = NULL;\n@@ -2178,14 +2178,14 @@ int merge_recursive_generic(struct merge_options *o,\n \t\t}\n \t}\n \n-\thold_locked_index(lock, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n \tclean = merge_recursive(o, head_commit, next_commit, ca,\n \t\t\tresult);\n \tif (clean < 0)\n \t\treturn clean;\n \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 err(o, _(\"Unable to write index.\"));\n \n \treturn clean ? 0 : 1;\ndiff --git a/merge.c b/merge.c\nindex 1d441ad94..a18a452b5 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -53,11 +53,11 @@ int checkout_fast_forward(const struct object_id *head,\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tint i, nr_trees = 0;\n \tstruct dir_struct dir;\n-\tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n+\tstruct lock_file lock_file = LOCK_INIT;\n \n \trefresh_cache(REFRESH_QUIET);\n \n-\tif (hold_locked_index(lock_file, LOCK_REPORT_ON_ERROR) < 0)\n+\tif (hold_locked_index(&lock_file, LOCK_REPORT_ON_ERROR) < 0)\n \t\treturn -1;\n \n \tmemset(&trees, 0, sizeof(trees));\n@@ -91,8 +91,8 @@ int checkout_fast_forward(const struct object_id *head,\n \t}\n \tif (unpack_trees(nr_trees, t, &opts))\n \t\treturn -1;\n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) {\n-\t\trollback_lock_file(lock_file);\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) {\n+\t\trollback_lock_file(&lock_file);\n \t\treturn error(_(\"unable to write new index file\"));\n \t}\n \treturn 0;\ndiff --git a/wt-status.c b/wt-status.c\nindex 6f730ee8f..7d88e7ca8 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -2299,14 +2299,14 @@ int has_uncommitted_changes(int ignore_submodules)\n */\n int require_clean_work_tree(const char *action, const char *hint, int ignore_submodules, int gently)\n {\n-\tstruct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tint err = 0, fd;\n \n-\tfd = hold_locked_index(lock_file, 0);\n+\tfd = hold_locked_index(&lock_file, 0);\n \trefresh_cache(REFRESH_QUIET);\n \tif (0 <= fd)\n-\t\tupdate_index_if_able(&the_index, lock_file);\n-\trollback_lock_file(lock_file);\n+\t\tupdate_index_if_able(&the_index, &lock_file);\n+\trollback_lock_file(&lock_file);\n \n \tif (has_unstaged_changes(ignore_submodules)) {\n \t\t/* TRANSLATORS: the action is e.g. \"pull with rebase\" */\n-- \n2.14.2.666.gea220ee40\n\n"},{"id":"329790","messageId":"bbb578e7d1367353d464d5196e0bbfe0a27af6ac.1507228170.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507228170.git.martin.agren@gmail.com","subject":"[PATCH v2 03/12] lockfile: fix documentation on `close_lock_file_gently()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-05T20:32:05Z","receivedAt":"2017-10-05T20:32:47Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Commit 83a3069a3 (lockfile: do not rollback lock on failed close,\n2017-09-05) forgot to update the documentation by the function definition\nto reflect that the lock is not rolled back in case closing fails.\n\nSigned-off-by: Martin Ågren \nSigned-off-by: Junio C Hamano \n---\nv2: Identical.\n\n lockfile.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/lockfile.h b/lockfile.h\nindex 7c1c484d7..f401c979f 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -240,8 +240,8 @@ extern char *get_locked_file_path(struct lock_file *lk);\n * If the lockfile is still open, close it (and the file pointer if it\n * has been opened using `fdopen_lock_file()`) without renaming the\n * lockfile over the file being locked. Return 0 upon success. On\n- * failure to `close(2)`, return a negative value and roll back the\n- * lock file. Usually `commit_lock_file()`, `commit_lock_file_to()`,\n+ * failure to `close(2)`, return a negative value (the lockfile is not\n+ * rolled back). Usually `commit_lock_file()`, `commit_lock_file_to()`,\n * or `rollback_lock_file()` should eventually be called.\n */\n static inline int close_lock_file_gently(struct lock_file *lk)\n-- \n2.14.2.666.gea220ee40\n\n"},{"id":"329791","messageId":"fe78be885bb4f5a4e021abdf80097cd5c497746f.1507228170.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507228170.git.martin.agren@gmail.com","subject":"[PATCH v2 05/12] checkout-index: simplify locking logic","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-05T20:32:07Z","receivedAt":"2017-10-05T20:32:51Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"`newfd` starts out negative. If we then take the lock, `newfd` will\nbecome non-negative. We later check for exactly that property before\ncalling `write_locked_index()`. That is, we are simply using `newfd` as\na boolean to keep track of whether we took the lock or not. (We always\nuse `newfd` and `lock_file` together, so they really are mirroring each\nother.)\n\nDrop `newfd` and check with `is_lock_file_locked()` instead. While at\nit, move the `static struct lock_file` into `cmd_checkout_index()` and\nmake it non-static. It is only used in this function, and after\n076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we\ncan have lockfiles on the stack.\n\nSigned-off-by: Martin Ågren \n---\nv2: New patch.\n\n builtin/checkout-index.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex 39c8be05d..b0e78b819 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -129,8 +129,6 @@ static const char * const builtin_checkout_index_usage[] = {\n \tNULL\n };\n \n-static struct lock_file lock_file;\n-\n static int option_parse_stage(const struct option *opt,\n \t\t\t const char *arg, int unset)\n {\n@@ -150,7 +148,7 @@ static int option_parse_stage(const struct option *opt,\n int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n-\tint newfd = -1;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tint all = 0;\n \tint read_from_stdin = 0;\n \tint prefix_length;\n@@ -206,7 +204,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \tif (index_opt && !state.base_dir_len && !to_tempfile) {\n \t\tstate.refresh_cache = 1;\n \t\tstate.istate = &the_index;\n-\t\tnewfd = hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n+\t\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \t}\n \n \t/* Check out named files first */\n@@ -251,7 +249,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \tif (all)\n \t\tcheckout_all(prefix, prefix_length);\n \n-\tif (0 <= newfd &&\n+\tif (is_lock_file_locked(&lock_file) &&\n \t write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(\"Unable to write new index file\");\n \treturn 0;\n-- \n2.14.2.666.gea220ee40\n\n"},{"id":"329792","messageId":"c9c03b3136a2881c393ab6a6712571dee7a498ed.1507228170.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507228170.git.martin.agren@gmail.com","subject":"[PATCH v2 06/12] cache-tree: simplify locking logic","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-05T20:32:08Z","receivedAt":"2017-10-05T20:32:54Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"After we have taken the lock using `LOCK_DIE_ON_ERROR`, we know that\n`newfd` is non-negative. So when we check for exactly that property\nbefore calling `write_locked_index()`, the outcome is guaranteed.\n\nIf we write and commit successfully, we set `newfd = -1`, so that we can\nlater avoid calling `rollback_lock_file` on an already-committed lock.\nBut we might just as well unconditionally call `rollback_lock_file()` --\nit will be a no-op if we have already committed.\n\nAll in all, we use `newfd` as a bool and the only benefit we get from it\nis that we can avoid calling a no-op. Remove `newfd` so that we have one\nvariable less to reason about.\n\nSigned-off-by: Martin Ågren \nSigned-off-by: Junio C Hamano \n---\nv2: Identical.\n\n cache-tree.c | 12 ++++--------\n 1 file changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 71d092ed5..f646f5673 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -602,11 +602,11 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat\n \n int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, const char *index_path, int flags, const char *prefix)\n {\n-\tint entries, was_valid, newfd;\n+\tint entries, was_valid;\n \tstruct lock_file lock_file = LOCK_INIT;\n \tint ret = 0;\n \n-\tnewfd = hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);\n+\thold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);\n \n \tentries = read_index_from(index_state, index_path);\n \tif (entries < 0) {\n@@ -625,10 +625,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co\n \t\t\tret = WRITE_TREE_UNMERGED_INDEX;\n \t\t\tgoto out;\n \t\t}\n-\t\tif (0 <= newfd) {\n-\t\t\tif (!write_locked_index(index_state, &lock_file, COMMIT_LOCK))\n-\t\t\t\tnewfd = -1;\n-\t\t}\n+\t\twrite_locked_index(index_state, &lock_file, COMMIT_LOCK);\n \t\t/* Not being able to write is fine -- we are only interested\n \t\t * in updating the cache-tree part, and if the next caller\n \t\t * ends up using the old index with unupdated cache-tree part\n@@ -650,8 +647,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co\n \t\thashcpy(sha1, index_state->cache_tree->oid.hash);\n \n out:\n-\tif (0 <= newfd)\n-\t\trollback_lock_file(&lock_file);\n+\trollback_lock_file(&lock_file);\n \treturn ret;\n }\n \n-- \n2.14.2.666.gea220ee40\n\n"},{"id":"329793","messageId":"c8426ab17ccdd07d75282b612c19a88d5d292c5f.1507228170.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507228170.git.martin.agren@gmail.com","subject":"[PATCH v2 07/12] apply: move lockfile into `apply_state`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-05T20:32:09Z","receivedAt":"2017-10-05T20:32:59Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We have two users of `struct apply_state` and the related functionality\nin apply.c. Each user sets up its `apply_state` by handing over a\npointer to its static `lock_file`. (Before 076aa2cbd (tempfile:\nauto-allocate tempfiles on heap, 2017-09-05), we could never free\nlockfiles, so making them static was a reasonable approach.)\n\nOther than that, they never directly access their `lock_file`s, which\nare instead handled by the functionality in apply.c.\n\nTo make life easier for the caller and to make it less tempting for a\nfuture caller to mess with the lock, make apply.c fully responsible for\nsetting up the `lock_file`. As mentioned above, it is now safe to free a\n`lock_file`, so we can make the `struct apply_state` contain an actual\n`struct lock_file` instead of a pointer to one.\n\nThe user in builtin/apply.c is rather simple. For builtin/am.c, we might\nworry that the lock state is actually meant to be inherited across\ncalls. But the lock is only taken as `apply_all_patches()` executes, and\ncode inspection shows that it will always be released.\n\nAlternatively, we can observe that the lock itself is never queried\ndirectly. When we decide whether we should lock, we check a related\nvariable `newfd`. That variable is not inherited, so from the point of\nview of apply.c, the state machine really is reset with each call to\n`init_apply_state()`. (It would be a bug if `newfd` and the lock status\nwere not in sync. The duplication of information in `newfd` and the lock\nwill be addressed in the next patch.)\n\nSigned-off-by: Martin Ågren \nSigned-off-by: Junio C Hamano \n---\nv2: Identical.\n\n apply.c | 14 +++++---------\n apply.h | 5 ++---\n builtin/am.c | 3 +--\n builtin/apply.c | 4 +---\n 4 files changed, 9 insertions(+), 17 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex c022af53a..5a6ca10a7 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -75,12 +75,10 @@ static int parse_ignorewhitespace_option(struct apply_state *state,\n }\n \n int init_apply_state(struct apply_state *state,\n-\t\t const char *prefix,\n-\t\t struct lock_file *lock_file)\n+\t\t const char *prefix)\n {\n \tmemset(state, 0, sizeof(*state));\n \tstate->prefix = prefix;\n-\tstate->lock_file = lock_file;\n \tstate->newfd = -1;\n \tstate->apply = 1;\n \tstate->line_termination = '\\n';\n@@ -146,8 +144,6 @@ int check_apply_state(struct apply_state *state, int force_apply)\n \t}\n \tif (state->check_index)\n \t\tstate->unsafe_paths = 0;\n-\tif (!state->lock_file)\n-\t\treturn error(\"BUG: state->lock_file should not be NULL\");\n \n \tif (state->apply_verbosity <= verbosity_silent) {\n \t\tstate->saved_error_routine = get_error_routine();\n@@ -4711,11 +4707,11 @@ static int apply_patch(struct apply_state *state,\n \tstate->update_index = state->check_index && state->apply;\n \tif (state->update_index && state->newfd < 0) {\n \t\tif (state->index_file)\n-\t\t\tstate->newfd = hold_lock_file_for_update(state->lock_file,\n+\t\t\tstate->newfd = hold_lock_file_for_update(&state->lock_file,\n \t\t\t\t\t\t\t\t state->index_file,\n \t\t\t\t\t\t\t\t LOCK_DIE_ON_ERROR);\n \t\telse\n-\t\t\tstate->newfd = hold_locked_index(state->lock_file, LOCK_DIE_ON_ERROR);\n+\t\t\tstate->newfd = hold_locked_index(&state->lock_file, LOCK_DIE_ON_ERROR);\n \t}\n \n \tif (state->check_index && read_apply_cache(state) < 0) {\n@@ -4911,7 +4907,7 @@ int apply_all_patches(struct apply_state *state,\n \t}\n \n \tif (state->update_index) {\n-\t\tres = write_locked_index(&the_index, state->lock_file, COMMIT_LOCK);\n+\t\tres = write_locked_index(&the_index, &state->lock_file, COMMIT_LOCK);\n \t\tif (res) {\n \t\t\terror(_(\"Unable to write new index file\"));\n \t\t\tres = -128;\n@@ -4924,7 +4920,7 @@ int apply_all_patches(struct apply_state *state,\n \n end:\n \tif (state->newfd >= 0) {\n-\t\trollback_lock_file(state->lock_file);\n+\t\trollback_lock_file(&state->lock_file);\n \t\tstate->newfd = -1;\n \t}\n \ndiff --git a/apply.h b/apply.h\nindex d9b395770..cf00cda17 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -37,7 +37,7 @@ struct apply_state {\n \tconst char *prefix;\n \n \t/* These are lock_file related */\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file;\n \tint newfd;\n \n \t/* These control what gets looked at and modified */\n@@ -116,8 +116,7 @@ extern int apply_parse_options(int argc, const char **argv,\n \t\t\t int *force_apply, int *options,\n \t\t\t const char * const *apply_usage);\n extern int init_apply_state(struct apply_state *state,\n-\t\t\t const char *prefix,\n-\t\t\t struct lock_file *lock_file);\n+\t\t\t const char *prefix);\n extern void clear_apply_state(struct apply_state *state);\n extern int check_apply_state(struct apply_state *state, int force_apply);\n \ndiff --git a/builtin/am.c b/builtin/am.c\nindex 4e16fd428..40968428d 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1488,11 +1488,10 @@ static int run_apply(const struct am_state *state, const char *index_file)\n \tstruct argv_array apply_opts = ARGV_ARRAY_INIT;\n \tstruct apply_state apply_state;\n \tint res, opts_left;\n-\tstatic struct lock_file lock_file;\n \tint force_apply = 0;\n \tint options = 0;\n \n-\tif (init_apply_state(&apply_state, NULL, &lock_file))\n+\tif (init_apply_state(&apply_state, NULL))\n \t\tdie(\"BUG: init_apply_state() failed\");\n \n \targv_array_push(&apply_opts, \"apply\");\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 81b9a61c3..48d398933 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -9,8 +9,6 @@ static const char * const apply_usage[] = {\n \tNULL\n };\n \n-static struct lock_file lock_file;\n-\n int cmd_apply(int argc, const char **argv, const char *prefix)\n {\n \tint force_apply = 0;\n@@ -18,7 +16,7 @@ int cmd_apply(int argc, const char **argv, const char *prefix)\n \tint ret;\n \tstruct apply_state state;\n \n-\tif (init_apply_state(&state, prefix, &lock_file))\n+\tif (init_apply_state(&state, prefix))\n \t\texit(128);\n \n \targc = apply_parse_options(argc, argv,\n-- \n2.14.2.666.gea220ee40\n\n"},{"id":"329794","messageId":"a2cedef325ccc0a2a0aaabd2d290457b05dfe8f0.1507228170.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507228170.git.martin.agren@gmail.com","subject":"[PATCH v2 08/12] apply: remove `newfd` from `struct apply_state`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-05T20:32:10Z","receivedAt":"2017-10-05T20:33:03Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Similar to a previous patch, we do not need to use `newfd` to signal\nthat we have a lockfile to clean up. We can just unconditionally call\n`rollback_lock_file`. If we do not hold the lock, it will be a no-op.\n\nWhere we check `newfd` to decide whether we need to take the lock, we\ncan instead use `is_lock_file_locked()`.\n\nSigned-off-by: Martin Ågren \nSigned-off-by: Junio C Hamano \n---\nv2: Identical.\n\n apply.c | 17 ++++++-----------\n apply.h | 3 +--\n 2 files changed, 7 insertions(+), 13 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 5a6ca10a7..d676debd5 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -79,7 +79,6 @@ int init_apply_state(struct apply_state *state,\n {\n \tmemset(state, 0, sizeof(*state));\n \tstate->prefix = prefix;\n-\tstate->newfd = -1;\n \tstate->apply = 1;\n \tstate->line_termination = '\\n';\n \tstate->p_value = 1;\n@@ -4705,13 +4704,13 @@ static int apply_patch(struct apply_state *state,\n \t\tstate->apply = 0;\n \n \tstate->update_index = state->check_index && state->apply;\n-\tif (state->update_index && state->newfd < 0) {\n+\tif (state->update_index && !is_lock_file_locked(&state->lock_file)) {\n \t\tif (state->index_file)\n-\t\t\tstate->newfd = hold_lock_file_for_update(&state->lock_file,\n-\t\t\t\t\t\t\t\t state->index_file,\n-\t\t\t\t\t\t\t\t LOCK_DIE_ON_ERROR);\n+\t\t\thold_lock_file_for_update(&state->lock_file,\n+\t\t\t\t\t\t state->index_file,\n+\t\t\t\t\t\t LOCK_DIE_ON_ERROR);\n \t\telse\n-\t\t\tstate->newfd = hold_locked_index(&state->lock_file, LOCK_DIE_ON_ERROR);\n+\t\t\thold_locked_index(&state->lock_file, LOCK_DIE_ON_ERROR);\n \t}\n \n \tif (state->check_index && read_apply_cache(state) < 0) {\n@@ -4913,16 +4912,12 @@ int apply_all_patches(struct apply_state *state,\n \t\t\tres = -128;\n \t\t\tgoto end;\n \t\t}\n-\t\tstate->newfd = -1;\n \t}\n \n \tres = !!errs;\n \n end:\n-\tif (state->newfd >= 0) {\n-\t\trollback_lock_file(&state->lock_file);\n-\t\tstate->newfd = -1;\n-\t}\n+\trollback_lock_file(&state->lock_file);\n \n \tif (state->apply_verbosity <= verbosity_silent) {\n \t\tset_error_routine(state->saved_error_routine);\ndiff --git a/apply.h b/apply.h\nindex cf00cda17..dc4a01905 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -36,9 +36,8 @@ enum apply_verbosity {\n struct apply_state {\n \tconst char *prefix;\n \n-\t/* These are lock_file related */\n+\t/* Lock file */\n \tstruct lock_file lock_file;\n-\tint newfd;\n \n \t/* These control what gets looked at and modified */\n \tint apply; /* this is not a dry-run */\n-- \n2.14.2.666.gea220ee40\n\n"},{"id":"329795","messageId":"db82bb7dc0828a415e48e654031b3b0d0ed763a5.1507228170.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507228170.git.martin.agren@gmail.com","subject":"[PATCH v2 10/12] read-cache: drop explicit `CLOSE_LOCK`-flag","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-05T20:32:12Z","receivedAt":"2017-10-05T20:33:05Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"`write_locked_index()` takes two flags: `COMMIT_LOCK` and `CLOSE_LOCK`.\nAt most one is allowed. But it is also possible to use no flag, i.e.,\n`0`. But when `write_locked_index()` calls `do_write_index()`, the\ntemporary file, a.k.a. the lockfile, will be closed. So passing `0` is\neffectively the same as `CLOSE_LOCK`, which seems like a bug.\n\nWe might feel tempted to restructure the code in order to close the file\nlater, or conditionally. It also feels a bit unfortunate that we simply\n\"happen\" to close the lock by way of an implementation detail of\nlockfiles. But note that we need to close the temporary file before\n`stat`-ing it, at least on Windows. See 9f41c7a6b (read-cache: close\nindex.lock in do_write_index, 2017-04-26).\n\nDrop `CLOSE_LOCK` and make it explicit that `write_locked_index()`\nalways closes the lock. Whether it is also committed is governed by the\nremaining flag, `COMMIT_LOCK`.\n\nThis means we neither have nor suggest that we have a mode to write the\nindex and leave the file open. Whatever extra contents we might\neventually want to write, we should probably write it from within\n`write_locked_index()` itself anyway.\n\nSigned-off-by: Martin Ågren \n---\nv2: Drop `CLOSE_LOCK` altogether instead of requiring precisely one of\nthe flags to be set.\n\n builtin/commit.c | 10 +++++-----\n cache.h | 5 ++---\n read-cache.c | 11 +++++------\n 3 files changed, 12 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 0f8ddb686..32dc2101f 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -355,7 +355,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \n \t\trefresh_cache_or_die(refresh_flags);\n \n-\t\tif (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\n+\t\tif (write_locked_index(&the_index, &index_lock, 0))\n \t\t\tdie(_(\"unable to create temporary index\"));\n \n \t\told_index_env = getenv(INDEX_ENVIRONMENT);\n@@ -374,7 +374,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\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-\t\t\tif (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\n+\t\t\tif (write_locked_index(&the_index, &index_lock, 0))\n \t\t\t\tdie(_(\"unable to update temporary index\"));\n \t\t} else\n \t\t\twarning(_(\"Failed to update main cache tree\"));\n@@ -401,7 +401,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\tadd_files_to_cache(also ? prefix : NULL, &pathspec, 0);\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tupdate_main_cache_tree(WRITE_TREE_SILENT);\n-\t\tif (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\n+\t\tif (write_locked_index(&the_index, &index_lock, 0))\n \t\t\tdie(_(\"unable to write new_index file\"));\n \t\tcommit_style = COMMIT_NORMAL;\n \t\tret = get_lock_file_path(&index_lock);\n@@ -474,7 +474,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \tadd_remove_files(&partial);\n \trefresh_cache(REFRESH_QUIET);\n \tupdate_main_cache_tree(WRITE_TREE_SILENT);\n-\tif (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\n+\tif (write_locked_index(&the_index, &index_lock, 0))\n \t\tdie(_(\"unable to write new_index file\"));\n \n \thold_lock_file_for_update(&false_lock,\n@@ -486,7 +486,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \tadd_remove_files(&partial);\n \trefresh_cache(REFRESH_QUIET);\n \n-\tif (write_locked_index(&the_index, &false_lock, CLOSE_LOCK))\n+\tif (write_locked_index(&the_index, &false_lock, 0))\n \t\tdie(_(\"unable to write temporary index file\"));\n \n \tdiscard_cache();\ndiff --git a/cache.h b/cache.h\nindex e9d9556e3..21a6856c5 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -604,11 +604,10 @@ extern int read_index_unmerged(struct index_state *);\n \n /* For use with `write_locked_index()`. */\n #define COMMIT_LOCK\t\t(1 << 0)\n-#define CLOSE_LOCK\t\t(1 << 1)\n \n /*\n- * Write the index while holding an already-taken lock. The flags may\n- * contain at most one of `COMMIT_LOCK` and `CLOSE_LOCK`.\n+ * Write the index while holding an already-taken lock. Close the lock,\n+ * and if `COMMIT_LOCK` is given, commit it.\n *\n * Unless a split index is in use, write the index into the lockfile.\n *\ndiff --git a/read-cache.c b/read-cache.c\nindex 65f4fe837..1c917eba9 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2343,14 +2343,13 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l\n \tint ret = do_write_index(istate, lock->tempfile, 0);\n \tif (ret)\n \t\treturn ret;\n-\tassert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=\n-\t (COMMIT_LOCK | CLOSE_LOCK));\n \tif (flags & COMMIT_LOCK)\n \t\treturn commit_locked_index(lock);\n-\telse if (flags & CLOSE_LOCK)\n-\t\treturn close_lock_file_gently(lock);\n-\telse\n-\t\treturn ret;\n+\t/*\n+\t * The lockfile already happens to have\n+\t * been closed, but let's be specific.\n+\t */\n+\treturn close_lock_file_gently(lock);\n }\n \n static int write_split_index(struct index_state *istate,\n-- \n2.14.2.666.gea220ee40\n\n"},{"id":"329796","messageId":"b5fc950e4594fb9dec07ec8872c0df8514d149ff.1507228170.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507228170.git.martin.agren@gmail.com","subject":"[PATCH v2 11/12] read-cache: leave lock in right state in `write_locked_index()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-05T20:32:13Z","receivedAt":"2017-10-05T20:33:08Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"If the original version of `write_locked_index()` returned with an\nerror, it didn't roll back the lockfile unless the error occured at the\nvery end, during closing/committing. See commit 03b866477 (read-cache:\nnew API write_locked_index instead of write_index/write_cache,\n2014-06-13).\n\nIn commit 9f41c7a6b (read-cache: close index.lock in do_write_index,\n2017-04-26), we learned to roll back the lock slightly earlier in the\ncallstack, but that was mostly a side-effect of lockfiles being\nimplemented using temporary files.\n\nAt that point, the behavior was still mostly the same as originally,\nexcept 1) the file was closed (and possibly rolled back) a few\nCPU-instructions earlier, and 2) the file was closed even if the caller\ndidn't ask us to close it. Case 2) is not very interesting since we\nnever had any such caller and the commit before this one removed the\npossibility of asking to leave the lockfile open.\n\nRecently, commit 076aa2cbd (tempfile: auto-allocate tempfiles on heap,\n2017-09-05) introduced a subtle bug. If the lockfile is rolled back\n(i.e., the temporary file is deleted), the tempfile-pointer in the\n`struct lock_file` will be left dangling. Thus, an attempt to reuse the\nlockfile, or even just to roll it back, will induce undefined behavior\n-- most likely a crash.\n\nBesides not crashing, we clearly want to make things consistent. But\nshould we roll back always on error, or never? The semantics which the\nlockfile-machinery itself provides is A) if we ask to commit and it\nfails, roll back, and B) if we ask to close and it fails, do _not_ roll\nback.\n\nWe should note that commit 83a3069a3 (lockfile: do not rollback lock on\nfailed close, 2017-09-05) recently changed the behavior for B -- we used\nto roll back. We might worry that our callers rely on us rolling back in\ncase of B. But we only did so for some errors, we never documented\nanything, and all our in-tree callers (they are not many) `die()` in\ncase of an error. This is our opportunity for establishing a consistent\nand predictable behavior going forward, so let's enforce the same\nsemantics that 83a3069a3 introduced to the lockfile-machinery itself.\n\nSimilarly, let's ensure that when we are asked to commit, that we always\neither commit or roll back. Right now, we have some early return paths\nwhich fail to roll back the lock.\n\nSo: Do not delete the temporary file in `do_write_index()`. One of its\ncallers, `write_locked_index()` will thereby avoid rolling back the\nlock. The other caller, `write_shared_index()`, will delete its\ntemporary file anyway. Both of these callers will avoid undefined\nbehavior (crashing).\n\nTeach `write_locked_index(..., COMMIT_LOCK)` to roll back the lock\nbefore returning. If we have already succeeded and committed, it will be\na noop. Simplify the existing callers where we now have a superfluous\ncall to `rollback_lockfile()`. This should keep future readers from\nwondering why the callers are inconsistent.\n\nWe still close the lock as we close the temporary file. This is what is\nreferred to as \"1)\" above. It does feel a bit unfortunate that we simply\n\"happen\" to close the lock by way of an implementation-detail of\nlockfiles. But note that we need to close the temporary file before\n`stat`-ing it, at least on Windows. See 9f41c7a6b (read-cache: close\nindex.lock in do_write_index, 2017-04-26).\n\nSigned-off-by: Martin Ågren \n---\nv2: Except for the slightly different documentation in cache.h, this is\na squash of the last two patches of v1. I hope the commit message is\nbetter.\n\n builtin/difftool.c | 1 -\n cache.h | 4 ++++\n merge.c | 4 +---\n read-cache.c | 14 ++++++++------\n sequencer.c | 1 -\n 5 files changed, 13 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex b2d3ba753..bcc79d188 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -616,7 +616,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\tif (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||\n \t\t\t write_locked_index(&wtindex, &lock, COMMIT_LOCK)) {\n \t\t\t\tret = error(\"could not write %s\", buf.buf);\n-\t\t\t\trollback_lock_file(&lock);\n \t\t\t\tgoto finish;\n \t\t\t}\n \t\t\tchanged_files(&wt_modified, buf.buf, workdir);\ndiff --git a/cache.h b/cache.h\nindex 21a6856c5..0e26224b9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -616,6 +616,10 @@ extern int read_index_unmerged(struct index_state *);\n * split index to the lockfile. If the temporary file for the shared\n * index cannot be created, fall back to the behavior described in\n * the previous paragraph.\n+ *\n+ * With `COMMIT_LOCK`, the lock is always committed or rolled back.\n+ * Without it, the lock is closed, but neither committed nor rolled\n+ * back.\n */\n extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);\n \ndiff --git a/merge.c b/merge.c\nindex a18a452b5..e5d796c9f 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -91,9 +91,7 @@ int checkout_fast_forward(const struct object_id *head,\n \t}\n \tif (unpack_trees(nr_trees, t, &opts))\n \t\treturn -1;\n-\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) {\n-\t\trollback_lock_file(&lock_file);\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\treturn error(_(\"unable to write new index file\"));\n-\t}\n \treturn 0;\n }\ndiff --git a/read-cache.c b/read-cache.c\nindex 1c917eba9..0090ab886 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2182,9 +2182,8 @@ static int has_racy_timestamp(struct index_state *istate)\n void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)\n {\n \tif ((istate->cache_changed || has_racy_timestamp(istate)) &&\n-\t verify_index(istate) &&\n-\t write_locked_index(istate, lockfile, COMMIT_LOCK))\n-\t\trollback_lock_file(lockfile);\n+\t verify_index(istate))\n+\t\twrite_locked_index(istate, lockfile, COMMIT_LOCK);\n }\n \n static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n@@ -2314,7 +2313,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\treturn -1;\n \tif (close_tempfile_gently(tempfile)) {\n \t\terror(_(\"could not close '%s'\"), tempfile->filename.buf);\n-\t\tdelete_tempfile(&tempfile);\n \t\treturn -1;\n \t}\n \tif (stat(tempfile->filename.buf, &st))\n@@ -2498,7 +2496,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \t (istate->cache_changed & ~EXTMASK)) {\n \t\tif (si)\n \t\t\thashclr(si->base_sha1);\n-\t\treturn do_write_locked_index(istate, lock, flags);\n+\t\tret = do_write_locked_index(istate, lock, flags);\n+\t\tgoto out;\n \t}\n \n \tif (getenv(\"GIT_TEST_SPLIT_INDEX\")) {\n@@ -2514,7 +2513,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \tif (new_shared_index) {\n \t\tret = write_shared_index(istate, lock, flags);\n \t\tif (ret)\n-\t\t\treturn ret;\n+\t\t\tgoto out;\n \t}\n \n \tret = write_split_index(istate, lock, flags);\n@@ -2523,6 +2522,9 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \tif (!ret && !new_shared_index)\n \t\tfreshen_shared_index(sha1_to_hex(si->base_sha1), 1);\n \n+out:\n+\tif (flags & COMMIT_LOCK)\n+\t\trollback_lock_file(lock);\n \treturn ret;\n }\n \ndiff --git a/sequencer.c b/sequencer.c\nindex 60636ce54..d56c38081 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1183,7 +1183,6 @@ static int read_and_refresh_cache(struct replay_opts *opts)\n \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n \tif (the_index.cache_changed && index_fd >= 0) {\n \t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {\n-\t\t\trollback_lock_file(&index_lock);\n \t\t\treturn error(_(\"git %s: failed to refresh the index\"),\n \t\t\t\t_(action_name(opts)));\n \t\t}\n-- \n2.14.2.666.gea220ee40\n\n"},{"id":"329797","messageId":"5ece540d98b6e4b89a3002d8fd27a696b4ebd1ac.1507228170.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507228170.git.martin.agren@gmail.com","subject":"[PATCH v2 09/12] cache.h: document `write_locked_index()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-05T20:32:11Z","receivedAt":"2017-10-05T20:33:09Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The next patches will tweak the behavior of this function. Document it\nin order to establish a basis for those patches.\n\nSigned-off-by: Martin Ågren \n---\nv2: Tweaked to go better with the changed approach in the later patches.\n\n cache.h | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex ea6c236e0..e9d9556e3 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -601,9 +601,25 @@ extern int do_read_index(struct index_state *istate, const char *path,\n extern int read_index_from(struct index_state *, const char *path);\n extern int is_index_unborn(struct index_state *);\n extern int read_index_unmerged(struct index_state *);\n+\n+/* For use with `write_locked_index()`. */\n #define COMMIT_LOCK\t\t(1 << 0)\n #define CLOSE_LOCK\t\t(1 << 1)\n+\n+/*\n+ * Write the index while holding an already-taken lock. The flags may\n+ * contain at most one of `COMMIT_LOCK` and `CLOSE_LOCK`.\n+ *\n+ * Unless a split index is in use, write the index into the lockfile.\n+ *\n+ * With a split index, write the shared index to a temporary file,\n+ * adjust its permissions and rename it into place, then write the\n+ * split index to the lockfile. If the temporary file for the shared\n+ * index cannot be created, fall back to the behavior described in\n+ * the previous paragraph.\n+ */\n extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);\n+\n extern int discard_index(struct index_state *);\n extern void move_index_extensions(struct index_state *dst, struct index_state *src);\n extern int unmerged_index(const struct index_state *);\n-- \n2.14.2.666.gea220ee40\n\n"},{"id":"329798","messageId":"1da9447e4c8ea0c1137c97f6122b48c06a95b1c7.1507228170.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507228170.git.martin.agren@gmail.com","subject":"[PATCH v2 12/12] read_cache: roll back lock in `update_index_if_able()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-05T20:32:14Z","receivedAt":"2017-10-05T20:33:11Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"`update_index_if_able()` used to always commit the lock or roll it back.\nCommit 03b866477 (read-cache: new API write_locked_index instead of\nwrite_index/write_cache, 2014-06-13) stopped rolling it back in case a\nwrite was not even attempted. This change in behavior is not motivated\nin the commit message and appears to be accidental: the `else`-path was\nremoved, although that changed the behavior in case the `if` shortcuts.\n\nReintroduce the rollback and document this behavior. While at it, move\nthe documentation on this function from the function definition to the\nfunction declaration in cache.h.\n\nIf `write_locked_index(..., COMMIT_LOCK)` fails, it will roll back the\nlock for us (see the previous commit).\n\nNoticed-by: Junio C Hamano \nSigned-off-by: Martin Ågren \n---\nv2: New patch.\n\n cache.h | 4 ++++\n read-cache.c | 5 ++---\n 2 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 0e26224b9..8c2493766 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -734,6 +734,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);\n extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);\n extern struct cache_entry *refresh_cache_entry(struct cache_entry *, unsigned int);\n \n+/*\n+ * Opportunistically update the index but do not complain if we can't.\n+ * The lockfile is always committed or rolled back.\n+ */\n extern void update_index_if_able(struct index_state *, struct lock_file *);\n \n extern int hold_locked_index(struct lock_file *, int);\ndiff --git a/read-cache.c b/read-cache.c\nindex 0090ab886..92ea5df96 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2176,14 +2176,13 @@ static int has_racy_timestamp(struct index_state *istate)\n \treturn 0;\n }\n \n-/*\n- * Opportunistically update the index but do not complain if we can't\n- */\n void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)\n {\n \tif ((istate->cache_changed || has_racy_timestamp(istate)) &&\n \t verify_index(istate))\n \t\twrite_locked_index(istate, lockfile, COMMIT_LOCK);\n+\telse\n+\t\trollback_lock_file(lockfile);\n }\n \n static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n-- \n2.14.2.666.gea220ee40\n\n"},{"id":"329799","messageId":"d7837015eb5706a32918b6a87688d9ac895ecb81.1507228170.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507228170.git.martin.agren@gmail.com","subject":"[PATCH v2 04/12] tempfile: fix documentation on `delete_tempfile()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-05T20:32:06Z","receivedAt":"2017-10-05T20:33:14Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The function has always been documented as returning 0 or -1. It is in\nfact `void`. Correct that. As part of the rearrangements we lose the\nmention that `delete_tempfile()` might set `errno`. Because there is\nno return value, the user can't really know whether it did anyway.\n\nSigned-off-by: Martin Ågren \nSigned-off-by: Junio C Hamano \n---\nv2: Identical.\n\n tempfile.h | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/tempfile.h b/tempfile.h\nindex b8f4b5e14..450908b2e 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -68,10 +68,10 @@\n * `create_tempfile()` returns an allocated tempfile on success or NULL\n * on failure. On errors, `errno` describes the reason for failure.\n *\n- * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile_gently()`\n- * return 0 on success. On failure they set `errno` appropriately and return\n- * -1. `delete` and `rename` (but not `close`) do their best to delete the\n- * temporary file before returning.\n+ * `rename_tempfile()` and `close_tempfile_gently()` return 0 on success.\n+ * On failure they set `errno` appropriately and return -1.\n+ * `delete_tempfile()` and `rename` (but not `close`) do their best to\n+ * delete the temporary file before returning.\n */\n \n struct tempfile {\n-- \n2.14.2.666.gea220ee40\n\n"},{"id":"329829","messageId":"xmqqtvzdyszw.fsf@gitster.mtv.corp.google.com","threadId":"46872","inReplyTo":"7f9a7a5d57bf76925e14111efdabe77f1c946e8d.1507228170.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 01/12] sha1_file: do not leak `lock_file`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-06T01:17:39Z","receivedAt":"2017-10-06T01:17:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> Bump `found` to the scope of the whole function and rearrange the \"roll\n> back or write?\"-checks to a straightforward if-else on `found`. This\n> also future-proves the code by making it obvious that we intend to take\n> exactly one of these paths.\n>\n> Improved-by: Jeff King \n> Signed-off-by: Martin Ågren \n> ---\n> v2: Moved the rollback to the end to have an obvious if-else instead of\n> retaining the original logic. Thanks Peff.\n\nYup, both are correct but this version is 10x easier to follow.\nThanks, both.\n\n"},{"id":"329830","messageId":"xmqqpoa1ysu1.fsf@gitster.mtv.corp.google.com","threadId":"46872","inReplyTo":"fe78be885bb4f5a4e021abdf80097cd5c497746f.1507228170.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 05/12] checkout-index: simplify locking logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-06T01:21:10Z","receivedAt":"2017-10-06T01:21:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> `newfd` starts out negative. If we then take the lock, `newfd` will\n> become non-negative. We later check for exactly that property before\n> calling `write_locked_index()`. That is, we are simply using `newfd` as\n> a boolean to keep track of whether we took the lock or not. (We always\n> use `newfd` and `lock_file` together, so they really are mirroring each\n> other.)\n>\n> Drop `newfd` and check with `is_lock_file_locked()` instead. While at\n> it, move the `static struct lock_file` into `cmd_checkout_index()` and\n> make it non-static. It is only used in this function, and after\n> 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we\n> can have lockfiles on the stack.\n>\n> Signed-off-by: Martin Ågren \n> ---\n> v2: New patch.\n\nWell reasoned and explained. Thanks.\n\n>\n> builtin/checkout-index.c | 8 +++-----\n> 1 file changed, 3 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\n> index 39c8be05d..b0e78b819 100644\n> --- a/builtin/checkout-index.c\n> +++ b/builtin/checkout-index.c\n> @@ -129,8 +129,6 @@ static const char * const builtin_checkout_index_usage[] = {\n> \tNULL\n> };\n> \n> -static struct lock_file lock_file;\n> -\n> static int option_parse_stage(const struct option *opt,\n> \t\t\t const char *arg, int unset)\n> {\n> @@ -150,7 +148,7 @@ static int option_parse_stage(const struct option *opt,\n> int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n> {\n> \tint i;\n> -\tint newfd = -1;\n> +\tstruct lock_file lock_file = LOCK_INIT;\n> \tint all = 0;\n> \tint read_from_stdin = 0;\n> \tint prefix_length;\n> @@ -206,7 +204,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n> \tif (index_opt && !state.base_dir_len && !to_tempfile) {\n> \t\tstate.refresh_cache = 1;\n> \t\tstate.istate = &the_index;\n> -\t\tnewfd = hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n> +\t\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n> \t}\n> \n> \t/* Check out named files first */\n> @@ -251,7 +249,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n> \tif (all)\n> \t\tcheckout_all(prefix, prefix_length);\n> \n> -\tif (0 <= newfd &&\n> +\tif (is_lock_file_locked(&lock_file) &&\n> \t write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n> \t\tdie(\"Unable to write new index file\");\n> \treturn 0;\n"},{"id":"329831","messageId":"xmqqinftyryw.fsf@gitster.mtv.corp.google.com","threadId":"46872","inReplyTo":"db82bb7dc0828a415e48e654031b3b0d0ed763a5.1507228170.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 10/12] read-cache: drop explicit `CLOSE_LOCK`-flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-06T01:39:51Z","receivedAt":"2017-10-06T01:39:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> diff --git a/read-cache.c b/read-cache.c\n> index 65f4fe837..1c917eba9 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2343,14 +2343,13 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l\n> \tint ret = do_write_index(istate, lock->tempfile, 0);\n> \tif (ret)\n> \t\treturn ret;\n> -\tassert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=\n> -\t (COMMIT_LOCK | CLOSE_LOCK));\n> \tif (flags & COMMIT_LOCK)\n> \t\treturn commit_locked_index(lock);\n> -\telse if (flags & CLOSE_LOCK)\n> -\t\treturn close_lock_file_gently(lock);\n> -\telse\n> -\t\treturn ret;\n> +\t/*\n> +\t * The lockfile already happens to have\n> +\t * been closed, but let's be specific.\n> +\t */\n> +\treturn close_lock_file_gently(lock);\n\n\"already happens to have been\" is quite a mouthful, and is not quite\ntruthful, as we do not foresee ever wanting to change that (because\nof that stat(2) issue you mentioned). It might be better to declare\nthat do_write_index() closes the lockfile after successfully writing\nthe data out to it. I dunno if that reasoning is strong enough to\nremove this (extra) close, though.\n\nWhen any of the ce_write() calls in do_write_index() fails, the\nfunction returns -1 without hitting the close/stat (obviously).\nSomebody very high in the callchain (e.g. write_locked_index())\nwould clean it up by calling rollback_lock_file() eventually, so\nthat would not be a problem ;-)\n\n\n\n"},{"id":"329832","messageId":"xmqqefqhyqyg.fsf@gitster.mtv.corp.google.com","threadId":"46872","inReplyTo":"b5fc950e4594fb9dec07ec8872c0df8514d149ff.1507228170.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 11/12] read-cache: leave lock in right state in `write_locked_index()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-06T02:01:43Z","receivedAt":"2017-10-06T02:02:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> v2: Except for the slightly different documentation in cache.h, this is\n> a squash of the last two patches of v1. I hope the commit message is\n> better.\n\nYeah, it is long ;-) but readable.\n\nThanks.\n"},{"id":"329870","messageId":"CAN0heSrB4e4tNcuydztXHB9zg=y8poVSPLk_xNZssdGW3zYcxQ@mail.gmail.com","threadId":"46872","inReplyTo":"xmqqinftyryw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 10/12] read-cache: drop explicit `CLOSE_LOCK`-flag","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-06T11:02:28Z","receivedAt":"2017-10-06T11:02:35Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 6 October 2017 at 03:39, Junio C Hamano wrote:\n> Martin Ågren writes:\n>\n>> diff --git a/read-cache.c b/read-cache.c\n>> index 65f4fe837..1c917eba9 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -2343,14 +2343,13 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l\n>> int ret = do_write_index(istate, lock->tempfile, 0);\n>> if (ret)\n>> return ret;\n>> - assert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=\n>> - (COMMIT_LOCK | CLOSE_LOCK));\n>> if (flags & COMMIT_LOCK)\n>> return commit_locked_index(lock);\n>> - else if (flags & CLOSE_LOCK)\n>> - return close_lock_file_gently(lock);\n>> - else\n>> - return ret;\n>> + /*\n>> + * The lockfile already happens to have\n>> + * been closed, but let's be specific.\n>> + */\n>> + return close_lock_file_gently(lock);\n>\n> \"already happens to have been\" is quite a mouthful, and is not quite\n> truthful, as we do not foresee ever wanting to change that (because\n> of that stat(2) issue you mentioned). It might be better to declare\n> that do_write_index() closes the lockfile after successfully writing\n> the data out to it. I dunno if that reasoning is strong enough to\n> remove this (extra) close, though.\n>\n> When any of the ce_write() calls in do_write_index() fails, the\n> function returns -1 without hitting the close/stat (obviously).\n> Somebody very high in the callchain (e.g. write_locked_index())\n> would clean it up by calling rollback_lock_file() eventually, so\n> that would not be a problem ;-)\n\nWhen I wrote this, I was too stuck in the \"it gets closed accidentally\"\nworld view. It would indeed be cleaner to specify that the close happens\nin `do_write_index()`. As you say, because of the stat-ing, we simply\nhave to close.\n\nIt's still an implementation detail that closing the temporary file is\nthe same as closing the lock. We might want to refactor to hand over the\nlock instead of its tempfile. Except the other caller has no suitable\nlock, only a temporary file. I guess that caller could use a lock\ninstead, but it feels like the wrong solution to the wrong problem.\n\nI'm sure that something could be done here to improve the cleanliness.\nFor this series, I think I'll document better that `do_write_index()`\ncloses the temporary file on success, that this might mean that it\nactually closes a *lock*file, but that the latter should not be relied\nupon. I'll get to this later today.\n\nThanks.\n\nMartin\n"},{"id":"329871","messageId":"CAN0heSojqHEQQfwUaGuLH2TjN+aHSFHg4WAvcd5Ksnxd-54YMg@mail.gmail.com","threadId":"46872","inReplyTo":"xmqqefqhyqyg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 11/12] read-cache: leave lock in right state in `write_locked_index()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-06T11:04:53Z","receivedAt":"2017-10-06T11:04:59Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 6 October 2017 at 04:01, Junio C Hamano wrote:\n> Martin Ågren writes:\n>\n>> v2: Except for the slightly different documentation in cache.h, this is\n>> a squash of the last two patches of v1. I hope the commit message is\n>> better.\n>\n> Yeah, it is long ;-) but readable.\n\n\"Long but readable\"... Yeah. When I rework the previous patch (document\nthe closing-behavior of `do_write_index()`) I could address this. I\nthink there are several interesting details here and I'm not sure which\nI'd want to leave out, but yeah, they add up...\n\nMartin\n"},{"id":"329874","messageId":"xmqqvajsv60e.fsf@gitster.mtv.corp.google.com","threadId":"46872","inReplyTo":"CAN0heSojqHEQQfwUaGuLH2TjN+aHSFHg4WAvcd5Ksnxd-54YMg@mail.gmail.com","subject":"Re: [PATCH v2 11/12] read-cache: leave lock in right state in `write_locked_index()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-06T12:02:25Z","receivedAt":"2017-10-06T12:02:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> On 6 October 2017 at 04:01, Junio C Hamano wrote:\n>> Martin Ågren writes:\n>>\n>>> v2: Except for the slightly different documentation in cache.h, this is\n>>> a squash of the last two patches of v1. I hope the commit message is\n>>> better.\n>>\n>> Yeah, it is long ;-) but readable.\n>\n> \"Long but readable\"... Yeah. When I rework the previous patch (document\n> the closing-behavior of `do_write_index()`) I could address this. I\n> think there are several interesting details here and I'm not sure which\n> I'd want to leave out, but yeah, they add up...\n\nI didn't mean \"long is bad\" at all in this case. \n\nCertainly, from time to time we find commits with overlong\nexplanation that only states obvious, and they are \"long and bad\".\nBut I do not think this one falls into the same category as those.\n\n\n"},{"id":"329922","messageId":"CAN0heSqr77dptPG2mixnLdNKKcgD36aPnEFtnTB6Oyip6WfQBA@mail.gmail.com","threadId":"46872","inReplyTo":"xmqqvajsv60e.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 11/12] read-cache: leave lock in right state in `write_locked_index()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-06T19:44:41Z","receivedAt":"2017-10-06T19:44:47Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 6 October 2017 at 14:02, Junio C Hamano wrote:\n> Martin Ågren writes:\n>\n>> On 6 October 2017 at 04:01, Junio C Hamano wrote:\n>>> Martin Ågren writes:\n>>>\n>>>> v2: Except for the slightly different documentation in cache.h, this is\n>>>> a squash of the last two patches of v1. I hope the commit message is\n>>>> better.\n>>>\n>>> Yeah, it is long ;-) but readable.\n>>\n>> \"Long but readable\"... Yeah. When I rework the previous patch (document\n>> the closing-behavior of `do_write_index()`) I could address this. I\n>> think there are several interesting details here and I'm not sure which\n>> I'd want to leave out, but yeah, they add up...\n>\n> I didn't mean \"long is bad\" at all in this case.\n>\n> Certainly, from time to time we find commits with overlong\n> explanation that only states obvious, and they are \"long and bad\".\n> But I do not think this one falls into the same category as those.\n\nOk, thanks. I've got a rerolled series running through the final checks\nright now. I did end up making this log message a bit more succinct.\n"},{"id":"329923","messageId":"cover.1507320376.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"CAN0heSqr77dptPG2mixnLdNKKcgD36aPnEFtnTB6Oyip6WfQBA@mail.gmail.com","subject":"[PATCH v3 00/12] Re: various lockfile-leaks and -fixes","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-06T20:12:02Z","receivedAt":"2017-10-06T20:12:41Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 6 October 2017 at 21:44, Martin Ågren wrote:\n> Ok, thanks. I've got a rerolled series running through the final checks\n> right now. I did end up making this log message a bit more succinct.\n\nThis is v3 of this series. All patches are identical to\nma/lockfile-fixes in Junio's tree, except 10 and 11.\n\nMartin Ågren (12):\n sha1_file: do not leak `lock_file`\n treewide: prefer lockfiles on the stack\n lockfile: fix documentation on `close_lock_file_gently()`\n tempfile: fix documentation on `delete_tempfile()`\n checkout-index: simplify locking logic\n cache-tree: simplify locking logic\n apply: move lockfile into `apply_state`\n apply: remove `newfd` from `struct apply_state`\n cache.h: document `write_locked_index()`\n read-cache: drop explicit `CLOSE_LOCK`-flag\n read-cache: leave lock in right state in `write_locked_index()`\n read_cache: roll back lock in `update_index_if_able()`\n\n apply.c | 25 ++++++++-----------------\n apply.h | 8 +++-----\n builtin/am.c | 27 ++++++++++++---------------\n builtin/apply.c | 4 +---\n builtin/checkout-index.c | 8 +++-----\n builtin/checkout.c | 14 ++++++--------\n builtin/clone.c | 7 +++----\n builtin/commit.c | 10 +++++-----\n builtin/diff.c | 7 +++----\n builtin/difftool.c | 1 -\n cache-tree.c | 12 ++++--------\n cache.h | 25 ++++++++++++++++++++++++-\n config.c | 17 ++++++++---------\n lockfile.h | 4 ++--\n merge-recursive.c | 6 +++---\n merge.c | 8 +++-----\n read-cache.c | 31 +++++++++++++++++--------------\n sequencer.c | 1 -\n sha1_file.c | 19 ++++++++-----------\n tempfile.h | 8 ++++----\n wt-status.c | 8 ++++----\n 21 files changed, 121 insertions(+), 129 deletions(-)\n\n-- \n2.15.0.rc0\n\n"},{"id":"329924","messageId":"f132a127eebb8f1b14b87fc317ca68278fceb2e8.1507320376.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507320376.git.martin.agren@gmail.com","subject":"[PATCH v3 01/12] sha1_file: do not leak `lock_file`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-06T20:12:03Z","receivedAt":"2017-10-06T20:12:45Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"There is no longer any need to allocate and leak a `struct lock_file`.\nInitialize it on the stack instead.\n\nBefore this patch, we set `lock = NULL` to signal that we have already\nrolled back, and that we should not do any more work. We need to take\nanother approach now that we cannot assign NULL. We could, e.g., use\n`is_lock_file_locked()`. But we already have another variable that we\ncould use instead, `found`. Its scope is only too small.\n\nBump `found` to the scope of the whole function and rearrange the \"roll\nback or write?\"-checks to a straightforward if-else on `found`. This\nalso future-proves the code by making it obvious that we intend to take\nexactly one of these paths.\n\nImproved-by: Jeff King \nSigned-off-by: Martin Ågren \nSigned-off-by: Junio C Hamano \n---\n sha1_file.c | 19 ++++++++-----------\n 1 file changed, 8 insertions(+), 11 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5a2014811..1d1747099 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -456,19 +456,19 @@ struct alternate_object_database *alloc_alt_odb(const char *dir)\n \n void add_to_alternates_file(const char *reference)\n {\n-\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n+\tstruct lock_file lock = LOCK_INIT;\n \tchar *alts = git_pathdup(\"objects/info/alternates\");\n \tFILE *in, *out;\n+\tint found = 0;\n \n-\thold_lock_file_for_update(lock, alts, LOCK_DIE_ON_ERROR);\n-\tout = fdopen_lock_file(lock, \"w\");\n+\thold_lock_file_for_update(&lock, alts, LOCK_DIE_ON_ERROR);\n+\tout = fdopen_lock_file(&lock, \"w\");\n \tif (!out)\n \t\tdie_errno(\"unable to fdopen alternates lockfile\");\n \n \tin = fopen(alts, \"r\");\n \tif (in) {\n \t\tstruct strbuf line = STRBUF_INIT;\n-\t\tint found = 0;\n \n \t\twhile (strbuf_getline(&line, in) != EOF) {\n \t\t\tif (!strcmp(reference, line.buf)) {\n@@ -480,18 +480,15 @@ void add_to_alternates_file(const char *reference)\n \n \t\tstrbuf_release(&line);\n \t\tfclose(in);\n-\n-\t\tif (found) {\n-\t\t\trollback_lock_file(lock);\n-\t\t\tlock = NULL;\n-\t\t}\n \t}\n \telse if (errno != ENOENT)\n \t\tdie_errno(\"unable to read alternates file\");\n \n-\tif (lock) {\n+\tif (found) {\n+\t\trollback_lock_file(&lock);\n+\t} else {\n \t\tfprintf_or_die(out, \"%s\\n\", reference);\n-\t\tif (commit_lock_file(lock))\n+\t\tif (commit_lock_file(&lock))\n \t\t\tdie_errno(\"unable to move new alternates file into place\");\n \t\tif (alt_odb_tail)\n \t\t\tlink_alt_odb_entries(reference, '\\n', NULL, 0);\n-- \n2.15.0.rc0\n\n"},{"id":"329925","messageId":"837e34eba47f209a38fc9ab458bd103fd7515325.1507320376.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507320376.git.martin.agren@gmail.com","subject":"[PATCH v3 02/12] treewide: prefer lockfiles on the stack","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-06T20:12:04Z","receivedAt":"2017-10-06T20:12:48Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"There is no longer any need to allocate and leak a `struct lock_file`.\nThe previous patch addressed an instance where we needed a minor tweak\nalongside the trivial changes.\n\nDeal with the remaining instances where we allocate and leak a struct\nwithin a single function. Change them to have the `struct lock_file` on\nthe stack instead.\n\nThese instances were identified by running `git grep \"^\\s*struct\nlock_file\\s*\\*\"`.\n\nSigned-off-by: Martin Ågren \nSigned-off-by: Junio C Hamano \n---\n builtin/am.c | 24 +++++++++++-------------\n builtin/checkout.c | 14 ++++++--------\n builtin/clone.c | 7 +++----\n builtin/diff.c | 7 +++----\n config.c | 17 ++++++++---------\n merge-recursive.c | 6 +++---\n merge.c | 8 ++++----\n wt-status.c | 8 ++++----\n 8 files changed, 42 insertions(+), 49 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex d7513f537..4e16fd428 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1134,11 +1134,11 @@ static const char *msgnum(const struct am_state *state)\n */\n static void refresh_and_write_cache(void)\n {\n-\tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n+\tstruct lock_file lock_file = LOCK_INIT;\n \n-\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \trefresh_cache(REFRESH_QUIET);\n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write index file\"));\n }\n \n@@ -1946,15 +1946,14 @@ static void am_resolve(struct am_state *state)\n */\n static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n {\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc t[2];\n \n \tif (parse_tree(head) || parse_tree(remote))\n \t\treturn -1;\n \n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \trefresh_cache(REFRESH_QUIET);\n \n@@ -1970,11 +1969,11 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n \tinit_tree_desc(&t[1], remote->buffer, remote->size);\n \n \tif (unpack_trees(2, t, &opts)) {\n-\t\trollback_lock_file(lock_file);\n+\t\trollback_lock_file(&lock_file);\n \t\treturn -1;\n \t}\n \n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n \treturn 0;\n@@ -1986,15 +1985,14 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n */\n static int merge_tree(struct tree *tree)\n {\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc t[1];\n \n \tif (parse_tree(tree))\n \t\treturn -1;\n \n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = 1;\n@@ -2005,11 +2003,11 @@ static int merge_tree(struct tree *tree)\n \tinit_tree_desc(&t[0], tree->buffer, tree->size);\n \n \tif (unpack_trees(1, t, &opts)) {\n-\t\trollback_lock_file(lock_file);\n+\t\trollback_lock_file(&lock_file);\n \t\treturn -1;\n \t}\n \n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n \treturn 0;\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3345a0d16..34c3c5b61 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -247,7 +247,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n \tstruct object_id rev;\n \tstruct commit *head;\n \tint errs = 0;\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \n \tif (opts->track != BRANCH_TRACK_UNSPECIFIED)\n \t\tdie(_(\"'%s' cannot be used with updating paths\"), \"--track\");\n@@ -275,9 +275,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\treturn run_add_interactive(revision, \"--patch=checkout\",\n \t\t\t\t\t &opts->pathspec);\n \n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\n-\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \tif (read_cache_preload(&opts->pathspec) < 0)\n \t\treturn error(_(\"index file corrupt\"));\n \n@@ -376,7 +374,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t}\n \terrs |= finish_delayed_checkout(&state);\n \n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n \tread_ref_full(\"HEAD\", 0, rev.hash, NULL);\n@@ -472,9 +470,9 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t int *writeout_error)\n {\n \tint ret;\n-\tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n+\tstruct lock_file lock_file = LOCK_INIT;\n \n-\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \tif (read_cache_preload(NULL) < 0)\n \t\treturn error(_(\"index file corrupt\"));\n \n@@ -591,7 +589,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \tif (!cache_tree_fully_valid(active_cache_tree))\n \t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n \n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n \tif (!opts->force && !opts->quiet)\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex dbddd98f8..96a3aaaa1 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -706,7 +706,7 @@ static int checkout(int submodule_progress)\n {\n \tstruct object_id oid;\n \tchar *head;\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tstruct unpack_trees_options opts;\n \tstruct tree *tree;\n \tstruct tree_desc t;\n@@ -733,8 +733,7 @@ static int checkout(int submodule_progress)\n \t/* We need to be in the new work tree for the checkout */\n \tsetup_work_tree();\n \n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \tmemset(&opts, 0, sizeof opts);\n \topts.update = 1;\n@@ -750,7 +749,7 @@ static int checkout(int submodule_progress)\n \tif (unpack_trees(1, &t, &opts) < 0)\n \t\tdie(_(\"unable to checkout working tree\"));\n \n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n \terr |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 7e3ebcea3..91995965d 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -203,17 +203,16 @@ static int builtin_diff_combined(struct rev_info *revs,\n \n static void refresh_index_quietly(void)\n {\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tint fd;\n \n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\tfd = hold_locked_index(lock_file, 0);\n+\tfd = hold_locked_index(&lock_file, 0);\n \tif (fd < 0)\n \t\treturn;\n \tdiscard_cache();\n \tread_cache();\n \trefresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);\n-\tupdate_index_if_able(&the_index, lock_file);\n+\tupdate_index_if_able(&the_index, &lock_file);\n }\n \n static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv)\ndiff --git a/config.c b/config.c\nindex 345d78c2b..0e61e863d 100644\n--- a/config.c\n+++ b/config.c\n@@ -2748,7 +2748,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n {\n \tint ret = 0, remove = 0;\n \tchar *filename_buf = NULL;\n-\tstruct lock_file *lock;\n+\tstruct lock_file lock = LOCK_INIT;\n \tint out_fd;\n \tchar buf[1024];\n \tFILE *config_file = NULL;\n@@ -2762,8 +2762,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n \tif (!config_filename)\n \t\tconfig_filename = filename_buf = git_pathdup(\"config\");\n \n-\tlock = xcalloc(1, sizeof(struct lock_file));\n-\tout_fd = hold_lock_file_for_update(lock, config_filename, 0);\n+\tout_fd = hold_lock_file_for_update(&lock, config_filename, 0);\n \tif (out_fd < 0) {\n \t\tret = error(\"could not lock config file %s\", config_filename);\n \t\tgoto out;\n@@ -2782,9 +2781,9 @@ int git_config_rename_section_in_file(const char *config_filename,\n \t\tgoto out;\n \t}\n \n-\tif (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {\n+\tif (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {\n \t\tret = error_errno(\"chmod on %s failed\",\n-\t\t\t\t get_lock_file_path(lock));\n+\t\t\t\t get_lock_file_path(&lock));\n \t\tgoto out;\n \t}\n \n@@ -2805,7 +2804,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 (write_section(out_fd, new_name) < 0) {\n-\t\t\t\t\tret = write_error(get_lock_file_path(lock));\n+\t\t\t\t\tret = write_error(get_lock_file_path(&lock));\n \t\t\t\t\tgoto out;\n \t\t\t\t}\n \t\t\t\t/*\n@@ -2831,20 +2830,20 @@ 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) < 0) {\n-\t\t\tret = write_error(get_lock_file_path(lock));\n+\t\t\tret = write_error(get_lock_file_path(&lock));\n \t\t\tgoto out;\n \t\t}\n \t}\n \tfclose(config_file);\n \tconfig_file = NULL;\n commit_and_out:\n-\tif (commit_lock_file(lock) < 0)\n+\tif (commit_lock_file(&lock) < 0)\n \t\tret = error_errno(\"could not write config file %s\",\n \t\t\t\t config_filename);\n out:\n \tif (config_file)\n \t\tfclose(config_file);\n-\trollback_lock_file(lock);\n+\trollback_lock_file(&lock);\n out_no_rollback:\n \tfree(filename_buf);\n \treturn ret;\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 1d3f8f0d2..24c5c26a6 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2162,7 +2162,7 @@ int merge_recursive_generic(struct merge_options *o,\n \t\t\t struct commit **result)\n {\n \tint clean;\n-\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n+\tstruct lock_file lock = LOCK_INIT;\n \tstruct commit *head_commit = get_ref(head, o->branch1);\n \tstruct commit *next_commit = get_ref(merge, o->branch2);\n \tstruct commit_list *ca = NULL;\n@@ -2178,14 +2178,14 @@ int merge_recursive_generic(struct merge_options *o,\n \t\t}\n \t}\n \n-\thold_locked_index(lock, LOCK_DIE_ON_ERROR);\n+\thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n \tclean = merge_recursive(o, head_commit, next_commit, ca,\n \t\t\tresult);\n \tif (clean < 0)\n \t\treturn clean;\n \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 err(o, _(\"Unable to write index.\"));\n \n \treturn clean ? 0 : 1;\ndiff --git a/merge.c b/merge.c\nindex 1d441ad94..a18a452b5 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -53,11 +53,11 @@ int checkout_fast_forward(const struct object_id *head,\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tint i, nr_trees = 0;\n \tstruct dir_struct dir;\n-\tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n+\tstruct lock_file lock_file = LOCK_INIT;\n \n \trefresh_cache(REFRESH_QUIET);\n \n-\tif (hold_locked_index(lock_file, LOCK_REPORT_ON_ERROR) < 0)\n+\tif (hold_locked_index(&lock_file, LOCK_REPORT_ON_ERROR) < 0)\n \t\treturn -1;\n \n \tmemset(&trees, 0, sizeof(trees));\n@@ -91,8 +91,8 @@ int checkout_fast_forward(const struct object_id *head,\n \t}\n \tif (unpack_trees(nr_trees, t, &opts))\n \t\treturn -1;\n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) {\n-\t\trollback_lock_file(lock_file);\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) {\n+\t\trollback_lock_file(&lock_file);\n \t\treturn error(_(\"unable to write new index file\"));\n \t}\n \treturn 0;\ndiff --git a/wt-status.c b/wt-status.c\nindex 6f730ee8f..7d88e7ca8 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -2299,14 +2299,14 @@ int has_uncommitted_changes(int ignore_submodules)\n */\n int require_clean_work_tree(const char *action, const char *hint, int ignore_submodules, int gently)\n {\n-\tstruct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tint err = 0, fd;\n \n-\tfd = hold_locked_index(lock_file, 0);\n+\tfd = hold_locked_index(&lock_file, 0);\n \trefresh_cache(REFRESH_QUIET);\n \tif (0 <= fd)\n-\t\tupdate_index_if_able(&the_index, lock_file);\n-\trollback_lock_file(lock_file);\n+\t\tupdate_index_if_able(&the_index, &lock_file);\n+\trollback_lock_file(&lock_file);\n \n \tif (has_unstaged_changes(ignore_submodules)) {\n \t\t/* TRANSLATORS: the action is e.g. \"pull with rebase\" */\n-- \n2.15.0.rc0\n\n"},{"id":"329926","messageId":"5de134ca8594de22f2a0b738824f9b5176ae6c00.1507320376.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507320376.git.martin.agren@gmail.com","subject":"[PATCH v3 04/12] tempfile: fix documentation on `delete_tempfile()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-06T20:12:06Z","receivedAt":"2017-10-06T20:12:50Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The function has always been documented as returning 0 or -1. It is in\nfact `void`. Correct that. As part of the rearrangements we lose the\nmention that `delete_tempfile()` might set `errno`. Because there is\nno return value, the user can't really know whether it did anyway.\n\nSigned-off-by: Martin Ågren \nSigned-off-by: Junio C Hamano \n---\n tempfile.h | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/tempfile.h b/tempfile.h\nindex b8f4b5e14..450908b2e 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -68,10 +68,10 @@\n * `create_tempfile()` returns an allocated tempfile on success or NULL\n * on failure. On errors, `errno` describes the reason for failure.\n *\n- * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile_gently()`\n- * return 0 on success. On failure they set `errno` appropriately and return\n- * -1. `delete` and `rename` (but not `close`) do their best to delete the\n- * temporary file before returning.\n+ * `rename_tempfile()` and `close_tempfile_gently()` return 0 on success.\n+ * On failure they set `errno` appropriately and return -1.\n+ * `delete_tempfile()` and `rename` (but not `close`) do their best to\n+ * delete the temporary file before returning.\n */\n \n struct tempfile {\n-- \n2.15.0.rc0\n\n"},{"id":"329927","messageId":"2954e5ec4361c6932593663859aa437583edb500.1507320376.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507320376.git.martin.agren@gmail.com","subject":"[PATCH v3 06/12] cache-tree: simplify locking logic","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-06T20:12:08Z","receivedAt":"2017-10-06T20:12:53Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"After we have taken the lock using `LOCK_DIE_ON_ERROR`, we know that\n`newfd` is non-negative. So when we check for exactly that property\nbefore calling `write_locked_index()`, the outcome is guaranteed.\n\nIf we write and commit successfully, we set `newfd = -1`, so that we can\nlater avoid calling `rollback_lock_file` on an already-committed lock.\nBut we might just as well unconditionally call `rollback_lock_file()` --\nit will be a no-op if we have already committed.\n\nAll in all, we use `newfd` as a bool and the only benefit we get from it\nis that we can avoid calling a no-op. Remove `newfd` so that we have one\nvariable less to reason about.\n\nSigned-off-by: Martin Ågren \nSigned-off-by: Junio C Hamano \n---\n cache-tree.c | 12 ++++--------\n 1 file changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 71d092ed5..f646f5673 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -602,11 +602,11 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat\n \n int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, const char *index_path, int flags, const char *prefix)\n {\n-\tint entries, was_valid, newfd;\n+\tint entries, was_valid;\n \tstruct lock_file lock_file = LOCK_INIT;\n \tint ret = 0;\n \n-\tnewfd = hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);\n+\thold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);\n \n \tentries = read_index_from(index_state, index_path);\n \tif (entries < 0) {\n@@ -625,10 +625,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co\n \t\t\tret = WRITE_TREE_UNMERGED_INDEX;\n \t\t\tgoto out;\n \t\t}\n-\t\tif (0 <= newfd) {\n-\t\t\tif (!write_locked_index(index_state, &lock_file, COMMIT_LOCK))\n-\t\t\t\tnewfd = -1;\n-\t\t}\n+\t\twrite_locked_index(index_state, &lock_file, COMMIT_LOCK);\n \t\t/* Not being able to write is fine -- we are only interested\n \t\t * in updating the cache-tree part, and if the next caller\n \t\t * ends up using the old index with unupdated cache-tree part\n@@ -650,8 +647,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co\n \t\thashcpy(sha1, index_state->cache_tree->oid.hash);\n \n out:\n-\tif (0 <= newfd)\n-\t\trollback_lock_file(&lock_file);\n+\trollback_lock_file(&lock_file);\n \treturn ret;\n }\n \n-- \n2.15.0.rc0\n\n"},{"id":"329928","messageId":"02ae242fdde2ba8acb6b87e32df2b1e33786e78b.1507320376.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507320376.git.martin.agren@gmail.com","subject":"[PATCH v3 05/12] checkout-index: simplify locking logic","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-06T20:12:07Z","receivedAt":"2017-10-06T20:12:55Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"`newfd` starts out negative. If we then take the lock, `newfd` will\nbecome non-negative. We later check for exactly that property before\ncalling `write_locked_index()`. That is, we are simply using `newfd` as\na boolean to keep track of whether we took the lock or not. (We always\nuse `newfd` and `lock_file` together, so they really are mirroring each\nother.)\n\nDrop `newfd` and check with `is_lock_file_locked()` instead. While at\nit, move the `static struct lock_file` into `cmd_checkout_index()` and\nmake it non-static. It is only used in this function, and after\n076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we\ncan have lockfiles on the stack.\n\nSigned-off-by: Martin Ågren \nSigned-off-by: Junio C Hamano \n---\n builtin/checkout-index.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex 39c8be05d..b0e78b819 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -129,8 +129,6 @@ static const char * const builtin_checkout_index_usage[] = {\n \tNULL\n };\n \n-static struct lock_file lock_file;\n-\n static int option_parse_stage(const struct option *opt,\n \t\t\t const char *arg, int unset)\n {\n@@ -150,7 +148,7 @@ static int option_parse_stage(const struct option *opt,\n int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n-\tint newfd = -1;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tint all = 0;\n \tint read_from_stdin = 0;\n \tint prefix_length;\n@@ -206,7 +204,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \tif (index_opt && !state.base_dir_len && !to_tempfile) {\n \t\tstate.refresh_cache = 1;\n \t\tstate.istate = &the_index;\n-\t\tnewfd = hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n+\t\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \t}\n \n \t/* Check out named files first */\n@@ -251,7 +249,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \tif (all)\n \t\tcheckout_all(prefix, prefix_length);\n \n-\tif (0 <= newfd &&\n+\tif (is_lock_file_locked(&lock_file) &&\n \t write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(\"Unable to write new index file\");\n \treturn 0;\n-- \n2.15.0.rc0\n\n"},{"id":"329929","messageId":"d13cd4c92760d8fe01fb51fbab556a4d1f3713b4.1507320376.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507320376.git.martin.agren@gmail.com","subject":"[PATCH v3 08/12] apply: remove `newfd` from `struct apply_state`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-06T20:12:10Z","receivedAt":"2017-10-06T20:12:57Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Similar to a previous patch, we do not need to use `newfd` to signal\nthat we have a lockfile to clean up. We can just unconditionally call\n`rollback_lock_file`. If we do not hold the lock, it will be a no-op.\n\nWhere we check `newfd` to decide whether we need to take the lock, we\ncan instead use `is_lock_file_locked()`.\n\nSigned-off-by: Martin Ågren \nSigned-off-by: Junio C Hamano \n---\n apply.c | 17 ++++++-----------\n apply.h | 3 +--\n 2 files changed, 7 insertions(+), 13 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 5a6ca10a7..d676debd5 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -79,7 +79,6 @@ int init_apply_state(struct apply_state *state,\n {\n \tmemset(state, 0, sizeof(*state));\n \tstate->prefix = prefix;\n-\tstate->newfd = -1;\n \tstate->apply = 1;\n \tstate->line_termination = '\\n';\n \tstate->p_value = 1;\n@@ -4705,13 +4704,13 @@ static int apply_patch(struct apply_state *state,\n \t\tstate->apply = 0;\n \n \tstate->update_index = state->check_index && state->apply;\n-\tif (state->update_index && state->newfd < 0) {\n+\tif (state->update_index && !is_lock_file_locked(&state->lock_file)) {\n \t\tif (state->index_file)\n-\t\t\tstate->newfd = hold_lock_file_for_update(&state->lock_file,\n-\t\t\t\t\t\t\t\t state->index_file,\n-\t\t\t\t\t\t\t\t LOCK_DIE_ON_ERROR);\n+\t\t\thold_lock_file_for_update(&state->lock_file,\n+\t\t\t\t\t\t state->index_file,\n+\t\t\t\t\t\t LOCK_DIE_ON_ERROR);\n \t\telse\n-\t\t\tstate->newfd = hold_locked_index(&state->lock_file, LOCK_DIE_ON_ERROR);\n+\t\t\thold_locked_index(&state->lock_file, LOCK_DIE_ON_ERROR);\n \t}\n \n \tif (state->check_index && read_apply_cache(state) < 0) {\n@@ -4913,16 +4912,12 @@ int apply_all_patches(struct apply_state *state,\n \t\t\tres = -128;\n \t\t\tgoto end;\n \t\t}\n-\t\tstate->newfd = -1;\n \t}\n \n \tres = !!errs;\n \n end:\n-\tif (state->newfd >= 0) {\n-\t\trollback_lock_file(&state->lock_file);\n-\t\tstate->newfd = -1;\n-\t}\n+\trollback_lock_file(&state->lock_file);\n \n \tif (state->apply_verbosity <= verbosity_silent) {\n \t\tset_error_routine(state->saved_error_routine);\ndiff --git a/apply.h b/apply.h\nindex cf00cda17..dc4a01905 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -36,9 +36,8 @@ enum apply_verbosity {\n struct apply_state {\n \tconst char *prefix;\n \n-\t/* These are lock_file related */\n+\t/* Lock file */\n \tstruct lock_file lock_file;\n-\tint newfd;\n \n \t/* These control what gets looked at and modified */\n \tint apply; /* this is not a dry-run */\n-- \n2.15.0.rc0\n\n"},{"id":"329930","messageId":"8dc3834610b676c2eb64cfe2a08b8b3d05be05df.1507320376.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507320376.git.martin.agren@gmail.com","subject":"[PATCH v3 09/12] cache.h: document `write_locked_index()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-06T20:12:11Z","receivedAt":"2017-10-06T20:13:00Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The next patches will tweak the behavior of this function. Document it\nin order to establish a basis for those patches.\n\nSigned-off-by: Martin Ågren \nSigned-off-by: Junio C Hamano \n---\n cache.h | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex ea6c236e0..e9d9556e3 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -601,9 +601,25 @@ extern int do_read_index(struct index_state *istate, const char *path,\n extern int read_index_from(struct index_state *, const char *path);\n extern int is_index_unborn(struct index_state *);\n extern int read_index_unmerged(struct index_state *);\n+\n+/* For use with `write_locked_index()`. */\n #define COMMIT_LOCK\t\t(1 << 0)\n #define CLOSE_LOCK\t\t(1 << 1)\n+\n+/*\n+ * Write the index while holding an already-taken lock. The flags may\n+ * contain at most one of `COMMIT_LOCK` and `CLOSE_LOCK`.\n+ *\n+ * Unless a split index is in use, write the index into the lockfile.\n+ *\n+ * With a split index, write the shared index to a temporary file,\n+ * adjust its permissions and rename it into place, then write the\n+ * split index to the lockfile. If the temporary file for the shared\n+ * index cannot be created, fall back to the behavior described in\n+ * the previous paragraph.\n+ */\n extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);\n+\n extern int discard_index(struct index_state *);\n extern void move_index_extensions(struct index_state *dst, struct index_state *src);\n extern int unmerged_index(const struct index_state *);\n-- \n2.15.0.rc0\n\n"},{"id":"329931","messageId":"db718624ece17a7bed4c956d581dab83b579db0b.1507320376.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507320376.git.martin.agren@gmail.com","subject":"[PATCH v3 10/12] read-cache: drop explicit `CLOSE_LOCK`-flag","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-06T20:12:12Z","receivedAt":"2017-10-06T20:13:05Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"`write_locked_index()` takes two flags: `COMMIT_LOCK` and `CLOSE_LOCK`.\nAt most one is allowed. But it is also possible to use no flag, i.e.,\n`0`. But when `write_locked_index()` calls `do_write_index()`, the\ntemporary file, a.k.a. the lockfile, will be closed. So passing `0` is\neffectively the same as `CLOSE_LOCK`, which seems like a bug.\n\nWe might feel tempted to restructure the code in order to close the file\nlater, or conditionally. It also feels a bit unfortunate that we simply\n\"happen\" to close the lock by way of an implementation detail of\nlockfiles. But note that we need to close the temporary file before\n`stat`-ing it, at least on Windows. See 9f41c7a6b (read-cache: close\nindex.lock in do_write_index, 2017-04-26).\n\nDrop `CLOSE_LOCK` and make it explicit that `write_locked_index()`\nalways closes the lock. Whether it is also committed is governed by the\nremaining flag, `COMMIT_LOCK`.\n\nThis means we neither have nor suggest that we have a mode to write the\nindex and leave the file open. Whatever extra contents we might\neventually want to write, we should probably write it from within\n`write_locked_index()` itself anyway.\n\nSigned-off-by: Martin Ågren \n---\nv3: Dropped the comment in `do_write_locked_index()` and documented how\n`do_write_index()` handles the temporary file instead.\n\n builtin/commit.c | 10 +++++-----\n cache.h | 5 ++---\n read-cache.c | 14 ++++++++------\n 3 files changed, 15 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 0f8ddb686..32dc2101f 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -355,7 +355,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \n \t\trefresh_cache_or_die(refresh_flags);\n \n-\t\tif (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\n+\t\tif (write_locked_index(&the_index, &index_lock, 0))\n \t\t\tdie(_(\"unable to create temporary index\"));\n \n \t\told_index_env = getenv(INDEX_ENVIRONMENT);\n@@ -374,7 +374,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\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-\t\t\tif (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\n+\t\t\tif (write_locked_index(&the_index, &index_lock, 0))\n \t\t\t\tdie(_(\"unable to update temporary index\"));\n \t\t} else\n \t\t\twarning(_(\"Failed to update main cache tree\"));\n@@ -401,7 +401,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\tadd_files_to_cache(also ? prefix : NULL, &pathspec, 0);\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tupdate_main_cache_tree(WRITE_TREE_SILENT);\n-\t\tif (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\n+\t\tif (write_locked_index(&the_index, &index_lock, 0))\n \t\t\tdie(_(\"unable to write new_index file\"));\n \t\tcommit_style = COMMIT_NORMAL;\n \t\tret = get_lock_file_path(&index_lock);\n@@ -474,7 +474,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \tadd_remove_files(&partial);\n \trefresh_cache(REFRESH_QUIET);\n \tupdate_main_cache_tree(WRITE_TREE_SILENT);\n-\tif (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\n+\tif (write_locked_index(&the_index, &index_lock, 0))\n \t\tdie(_(\"unable to write new_index file\"));\n \n \thold_lock_file_for_update(&false_lock,\n@@ -486,7 +486,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \tadd_remove_files(&partial);\n \trefresh_cache(REFRESH_QUIET);\n \n-\tif (write_locked_index(&the_index, &false_lock, CLOSE_LOCK))\n+\tif (write_locked_index(&the_index, &false_lock, 0))\n \t\tdie(_(\"unable to write temporary index file\"));\n \n \tdiscard_cache();\ndiff --git a/cache.h b/cache.h\nindex e9d9556e3..21a6856c5 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -604,11 +604,10 @@ extern int read_index_unmerged(struct index_state *);\n \n /* For use with `write_locked_index()`. */\n #define COMMIT_LOCK\t\t(1 << 0)\n-#define CLOSE_LOCK\t\t(1 << 1)\n \n /*\n- * Write the index while holding an already-taken lock. The flags may\n- * contain at most one of `COMMIT_LOCK` and `CLOSE_LOCK`.\n+ * Write the index while holding an already-taken lock. Close the lock,\n+ * and if `COMMIT_LOCK` is given, commit it.\n *\n * Unless a split index is in use, write the index into the lockfile.\n *\ndiff --git a/read-cache.c b/read-cache.c\nindex 65f4fe837..c7aa3632a 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2187,6 +2187,13 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfile\n \t\trollback_lock_file(lockfile);\n }\n \n+/*\n+ * On success, `tempfile` is closed. If it is the temporary file\n+ * of a `struct lock_file`, we will therefore effectively perform\n+ * a 'close_lock_file_gently()`. Since that is an implementation\n+ * detail of lockfiles, callers of `do_write_index()` should not\n+ * rely on it.\n+ */\n static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\t\t int strip_extensions)\n {\n@@ -2343,14 +2350,9 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l\n \tint ret = do_write_index(istate, lock->tempfile, 0);\n \tif (ret)\n \t\treturn ret;\n-\tassert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=\n-\t (COMMIT_LOCK | CLOSE_LOCK));\n \tif (flags & COMMIT_LOCK)\n \t\treturn commit_locked_index(lock);\n-\telse if (flags & CLOSE_LOCK)\n-\t\treturn close_lock_file_gently(lock);\n-\telse\n-\t\treturn ret;\n+\treturn close_lock_file_gently(lock);\n }\n \n static int write_split_index(struct index_state *istate,\n-- \n2.15.0.rc0\n\n"},{"id":"329932","messageId":"d7e1a460350b084943479a562774d570028afa0e.1507320376.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507320376.git.martin.agren@gmail.com","subject":"[PATCH v3 12/12] read_cache: roll back lock in `update_index_if_able()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-06T20:12:14Z","receivedAt":"2017-10-06T20:13:07Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"`update_index_if_able()` used to always commit the lock or roll it back.\nCommit 03b866477 (read-cache: new API write_locked_index instead of\nwrite_index/write_cache, 2014-06-13) stopped rolling it back in case a\nwrite was not even attempted. This change in behavior is not motivated\nin the commit message and appears to be accidental: the `else`-path was\nremoved, although that changed the behavior in case the `if` shortcuts.\n\nReintroduce the rollback and document this behavior. While at it, move\nthe documentation on this function from the function definition to the\nfunction declaration in cache.h.\n\nIf `write_locked_index(..., COMMIT_LOCK)` fails, it will roll back the\nlock for us (see the previous commit).\n\nNoticed-by: Junio C Hamano \nSigned-off-by: Martin Ågren \nSigned-off-by: Junio C Hamano \n---\n cache.h | 4 ++++\n read-cache.c | 5 ++---\n 2 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 0e26224b9..8c2493766 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -734,6 +734,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);\n extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);\n extern struct cache_entry *refresh_cache_entry(struct cache_entry *, unsigned int);\n \n+/*\n+ * Opportunistically update the index but do not complain if we can't.\n+ * The lockfile is always committed or rolled back.\n+ */\n extern void update_index_if_able(struct index_state *, struct lock_file *);\n \n extern int hold_locked_index(struct lock_file *, int);\ndiff --git a/read-cache.c b/read-cache.c\nindex 0d8d2dede..87188d390 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2176,14 +2176,13 @@ static int has_racy_timestamp(struct index_state *istate)\n \treturn 0;\n }\n \n-/*\n- * Opportunistically update the index but do not complain if we can't\n- */\n void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)\n {\n \tif ((istate->cache_changed || has_racy_timestamp(istate)) &&\n \t verify_index(istate))\n \t\twrite_locked_index(istate, lockfile, COMMIT_LOCK);\n+\telse\n+\t\trollback_lock_file(lockfile);\n }\n \n /*\n-- \n2.15.0.rc0\n\n"},{"id":"329933","messageId":"ac3c5dd103de8364b0289a240d38782eafca2739.1507320376.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507320376.git.martin.agren@gmail.com","subject":"[PATCH v3 11/12] read-cache: leave lock in right state in `write_locked_index()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-06T20:12:13Z","receivedAt":"2017-10-06T20:13:09Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"If the original version of `write_locked_index()` returned with an\nerror, it didn't roll back the lockfile unless the error occured at the\nvery end, during closing/committing. See commit 03b866477 (read-cache:\nnew API write_locked_index instead of write_index/write_cache,\n2014-06-13).\n\nIn commit 9f41c7a6b (read-cache: close index.lock in do_write_index,\n2017-04-26), we learned to close the lock slightly earlier in the\ncallstack. That was mostly a side-effect of lockfiles being implemented\nusing temporary files, but didn't cause any real harm.\n\nRecently, commit 076aa2cbd (tempfile: auto-allocate tempfiles on heap,\n2017-09-05) introduced a subtle bug. If the temporary file is deleted\n(i.e., the lockfile is rolled back), the tempfile-pointer in the `struct\nlock_file` will be left dangling. Thus, an attempt to reuse the\nlockfile, or even just to roll it back, will induce undefined behavior\n-- most likely a crash.\n\nBesides not crashing, we clearly want to make things consistent. The\nguarantees which the lockfile-machinery itself provides is A) if we ask\nto commit and it fails, roll back, and B) if we ask to close and it\nfails, do _not_ roll back. Let's do the same for consistency.\n\nDo not delete the temporary file in `do_write_index()`. One of its\ncallers, `write_locked_index()` will thereby avoid rolling back the\nlock. The other caller, `write_shared_index()`, will delete its\ntemporary file anyway. Both of these callers will avoid undefined\nbehavior (crashing).\n\nTeach `write_locked_index(..., COMMIT_LOCK)` to roll back the lock\nbefore returning. If we have already succeeded and committed, it will be\na noop. Simplify the existing callers where we now have a superfluous\ncall to `rollback_lockfile()`. That should keep future readers from\nwondering why the callers are inconsistent.\n\nSigned-off-by: Martin Ågren \n---\nv3: Maybe the commit message wasn't too long, but it was loong. For\nexample, some of it just duplicated stuff from the previous patch.\n\n builtin/difftool.c | 1 -\n cache.h | 4 ++++\n merge.c | 4 +---\n read-cache.c | 14 ++++++++------\n sequencer.c | 1 -\n 5 files changed, 13 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex b2d3ba753..bcc79d188 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -616,7 +616,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\tif (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||\n \t\t\t write_locked_index(&wtindex, &lock, COMMIT_LOCK)) {\n \t\t\t\tret = error(\"could not write %s\", buf.buf);\n-\t\t\t\trollback_lock_file(&lock);\n \t\t\t\tgoto finish;\n \t\t\t}\n \t\t\tchanged_files(&wt_modified, buf.buf, workdir);\ndiff --git a/cache.h b/cache.h\nindex 21a6856c5..0e26224b9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -616,6 +616,10 @@ extern int read_index_unmerged(struct index_state *);\n * split index to the lockfile. If the temporary file for the shared\n * index cannot be created, fall back to the behavior described in\n * the previous paragraph.\n+ *\n+ * With `COMMIT_LOCK`, the lock is always committed or rolled back.\n+ * Without it, the lock is closed, but neither committed nor rolled\n+ * back.\n */\n extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);\n \ndiff --git a/merge.c b/merge.c\nindex a18a452b5..e5d796c9f 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -91,9 +91,7 @@ int checkout_fast_forward(const struct object_id *head,\n \t}\n \tif (unpack_trees(nr_trees, t, &opts))\n \t\treturn -1;\n-\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) {\n-\t\trollback_lock_file(&lock_file);\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\treturn error(_(\"unable to write new index file\"));\n-\t}\n \treturn 0;\n }\ndiff --git a/read-cache.c b/read-cache.c\nindex c7aa3632a..0d8d2dede 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2182,9 +2182,8 @@ static int has_racy_timestamp(struct index_state *istate)\n void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)\n {\n \tif ((istate->cache_changed || has_racy_timestamp(istate)) &&\n-\t verify_index(istate) &&\n-\t write_locked_index(istate, lockfile, COMMIT_LOCK))\n-\t\trollback_lock_file(lockfile);\n+\t verify_index(istate))\n+\t\twrite_locked_index(istate, lockfile, COMMIT_LOCK);\n }\n \n /*\n@@ -2321,7 +2320,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\treturn -1;\n \tif (close_tempfile_gently(tempfile)) {\n \t\terror(_(\"could not close '%s'\"), tempfile->filename.buf);\n-\t\tdelete_tempfile(&tempfile);\n \t\treturn -1;\n \t}\n \tif (stat(tempfile->filename.buf, &st))\n@@ -2501,7 +2499,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \t (istate->cache_changed & ~EXTMASK)) {\n \t\tif (si)\n \t\t\thashclr(si->base_sha1);\n-\t\treturn do_write_locked_index(istate, lock, flags);\n+\t\tret = do_write_locked_index(istate, lock, flags);\n+\t\tgoto out;\n \t}\n \n \tif (getenv(\"GIT_TEST_SPLIT_INDEX\")) {\n@@ -2517,7 +2516,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \tif (new_shared_index) {\n \t\tret = write_shared_index(istate, lock, flags);\n \t\tif (ret)\n-\t\t\treturn ret;\n+\t\t\tgoto out;\n \t}\n \n \tret = write_split_index(istate, lock, flags);\n@@ -2526,6 +2525,9 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \tif (!ret && !new_shared_index)\n \t\tfreshen_shared_index(sha1_to_hex(si->base_sha1), 1);\n \n+out:\n+\tif (flags & COMMIT_LOCK)\n+\t\trollback_lock_file(lock);\n \treturn ret;\n }\n \ndiff --git a/sequencer.c b/sequencer.c\nindex 60636ce54..d56c38081 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1183,7 +1183,6 @@ static int read_and_refresh_cache(struct replay_opts *opts)\n \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n \tif (the_index.cache_changed && index_fd >= 0) {\n \t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {\n-\t\t\trollback_lock_file(&index_lock);\n \t\t\treturn error(_(\"git %s: failed to refresh the index\"),\n \t\t\t\t_(action_name(opts)));\n \t\t}\n-- \n2.15.0.rc0\n\n"},{"id":"329934","messageId":"d613576dfe7010b13157e63b2401d321074bbab8.1507320376.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507320376.git.martin.agren@gmail.com","subject":"[PATCH v3 03/12] lockfile: fix documentation on `close_lock_file_gently()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-06T20:12:05Z","receivedAt":"2017-10-06T20:13:15Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Commit 83a3069a3 (lockfile: do not rollback lock on failed close,\n2017-09-05) forgot to update the documentation by the function definition\nto reflect that the lock is not rolled back in case closing fails.\n\nSigned-off-by: Martin Ågren \nSigned-off-by: Junio C Hamano \n---\n lockfile.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/lockfile.h b/lockfile.h\nindex 7c1c484d7..f401c979f 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -240,8 +240,8 @@ extern char *get_locked_file_path(struct lock_file *lk);\n * If the lockfile is still open, close it (and the file pointer if it\n * has been opened using `fdopen_lock_file()`) without renaming the\n * lockfile over the file being locked. Return 0 upon success. On\n- * failure to `close(2)`, return a negative value and roll back the\n- * lock file. Usually `commit_lock_file()`, `commit_lock_file_to()`,\n+ * failure to `close(2)`, return a negative value (the lockfile is not\n+ * rolled back). Usually `commit_lock_file()`, `commit_lock_file_to()`,\n * or `rollback_lock_file()` should eventually be called.\n */\n static inline int close_lock_file_gently(struct lock_file *lk)\n-- \n2.15.0.rc0\n\n"},{"id":"329935","messageId":"6d058c882643dc45c8d708be98e86c38f25511a9.1507320376.git.martin.agren@gmail.com","threadId":"46872","inReplyTo":"cover.1507320376.git.martin.agren@gmail.com","subject":"[PATCH v3 07/12] apply: move lockfile into `apply_state`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-06T20:12:09Z","receivedAt":"2017-10-06T20:13:18Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We have two users of `struct apply_state` and the related functionality\nin apply.c. Each user sets up its `apply_state` by handing over a\npointer to its static `lock_file`. (Before 076aa2cbd (tempfile:\nauto-allocate tempfiles on heap, 2017-09-05), we could never free\nlockfiles, so making them static was a reasonable approach.)\n\nOther than that, they never directly access their `lock_file`s, which\nare instead handled by the functionality in apply.c.\n\nTo make life easier for the caller and to make it less tempting for a\nfuture caller to mess with the lock, make apply.c fully responsible for\nsetting up the `lock_file`. As mentioned above, it is now safe to free a\n`lock_file`, so we can make the `struct apply_state` contain an actual\n`struct lock_file` instead of a pointer to one.\n\nThe user in builtin/apply.c is rather simple. For builtin/am.c, we might\nworry that the lock state is actually meant to be inherited across\ncalls. But the lock is only taken as `apply_all_patches()` executes, and\ncode inspection shows that it will always be released.\n\nAlternatively, we can observe that the lock itself is never queried\ndirectly. When we decide whether we should lock, we check a related\nvariable `newfd`. That variable is not inherited, so from the point of\nview of apply.c, the state machine really is reset with each call to\n`init_apply_state()`. (It would be a bug if `newfd` and the lock status\nwere not in sync. The duplication of information in `newfd` and the lock\nwill be addressed in the next patch.)\n\nSigned-off-by: Martin Ågren \nSigned-off-by: Junio C Hamano \n---\n apply.c | 14 +++++---------\n apply.h | 5 ++---\n builtin/am.c | 3 +--\n builtin/apply.c | 4 +---\n 4 files changed, 9 insertions(+), 17 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex c022af53a..5a6ca10a7 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -75,12 +75,10 @@ static int parse_ignorewhitespace_option(struct apply_state *state,\n }\n \n int init_apply_state(struct apply_state *state,\n-\t\t const char *prefix,\n-\t\t struct lock_file *lock_file)\n+\t\t const char *prefix)\n {\n \tmemset(state, 0, sizeof(*state));\n \tstate->prefix = prefix;\n-\tstate->lock_file = lock_file;\n \tstate->newfd = -1;\n \tstate->apply = 1;\n \tstate->line_termination = '\\n';\n@@ -146,8 +144,6 @@ int check_apply_state(struct apply_state *state, int force_apply)\n \t}\n \tif (state->check_index)\n \t\tstate->unsafe_paths = 0;\n-\tif (!state->lock_file)\n-\t\treturn error(\"BUG: state->lock_file should not be NULL\");\n \n \tif (state->apply_verbosity <= verbosity_silent) {\n \t\tstate->saved_error_routine = get_error_routine();\n@@ -4711,11 +4707,11 @@ static int apply_patch(struct apply_state *state,\n \tstate->update_index = state->check_index && state->apply;\n \tif (state->update_index && state->newfd < 0) {\n \t\tif (state->index_file)\n-\t\t\tstate->newfd = hold_lock_file_for_update(state->lock_file,\n+\t\t\tstate->newfd = hold_lock_file_for_update(&state->lock_file,\n \t\t\t\t\t\t\t\t state->index_file,\n \t\t\t\t\t\t\t\t LOCK_DIE_ON_ERROR);\n \t\telse\n-\t\t\tstate->newfd = hold_locked_index(state->lock_file, LOCK_DIE_ON_ERROR);\n+\t\t\tstate->newfd = hold_locked_index(&state->lock_file, LOCK_DIE_ON_ERROR);\n \t}\n \n \tif (state->check_index && read_apply_cache(state) < 0) {\n@@ -4911,7 +4907,7 @@ int apply_all_patches(struct apply_state *state,\n \t}\n \n \tif (state->update_index) {\n-\t\tres = write_locked_index(&the_index, state->lock_file, COMMIT_LOCK);\n+\t\tres = write_locked_index(&the_index, &state->lock_file, COMMIT_LOCK);\n \t\tif (res) {\n \t\t\terror(_(\"Unable to write new index file\"));\n \t\t\tres = -128;\n@@ -4924,7 +4920,7 @@ int apply_all_patches(struct apply_state *state,\n \n end:\n \tif (state->newfd >= 0) {\n-\t\trollback_lock_file(state->lock_file);\n+\t\trollback_lock_file(&state->lock_file);\n \t\tstate->newfd = -1;\n \t}\n \ndiff --git a/apply.h b/apply.h\nindex d9b395770..cf00cda17 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -37,7 +37,7 @@ struct apply_state {\n \tconst char *prefix;\n \n \t/* These are lock_file related */\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file;\n \tint newfd;\n \n \t/* These control what gets looked at and modified */\n@@ -116,8 +116,7 @@ extern int apply_parse_options(int argc, const char **argv,\n \t\t\t int *force_apply, int *options,\n \t\t\t const char * const *apply_usage);\n extern int init_apply_state(struct apply_state *state,\n-\t\t\t const char *prefix,\n-\t\t\t struct lock_file *lock_file);\n+\t\t\t const char *prefix);\n extern void clear_apply_state(struct apply_state *state);\n extern int check_apply_state(struct apply_state *state, int force_apply);\n \ndiff --git a/builtin/am.c b/builtin/am.c\nindex 4e16fd428..40968428d 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1488,11 +1488,10 @@ static int run_apply(const struct am_state *state, const char *index_file)\n \tstruct argv_array apply_opts = ARGV_ARRAY_INIT;\n \tstruct apply_state apply_state;\n \tint res, opts_left;\n-\tstatic struct lock_file lock_file;\n \tint force_apply = 0;\n \tint options = 0;\n \n-\tif (init_apply_state(&apply_state, NULL, &lock_file))\n+\tif (init_apply_state(&apply_state, NULL))\n \t\tdie(\"BUG: init_apply_state() failed\");\n \n \targv_array_push(&apply_opts, \"apply\");\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 81b9a61c3..48d398933 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -9,8 +9,6 @@ static const char * const apply_usage[] = {\n \tNULL\n };\n \n-static struct lock_file lock_file;\n-\n int cmd_apply(int argc, const char **argv, const char *prefix)\n {\n \tint force_apply = 0;\n@@ -18,7 +16,7 @@ int cmd_apply(int argc, const char **argv, const char *prefix)\n \tint ret;\n \tstruct apply_state state;\n \n-\tif (init_apply_state(&state, prefix, &lock_file))\n+\tif (init_apply_state(&state, prefix))\n \t\texit(128);\n \n \targc = apply_parse_options(argc, argv,\n-- \n2.15.0.rc0\n\n"}]}