From f132a127eebb8f1b14b87fc317ca68278fceb2e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Thu, 5 Oct 2017 22:32:03 +0200 Subject: [PATCH 01/12] sha1_file: do not leak `lock_file` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is no longer any need to allocate and leak a `struct lock_file`. Initialize it on the stack instead. Before this patch, we set `lock = NULL` to signal that we have already rolled back, and that we should not do any more work. We need to take another approach now that we cannot assign NULL. We could, e.g., use `is_lock_file_locked()`. But we already have another variable that we could use instead, `found`. Its scope is only too small. Bump `found` to the scope of the whole function and rearrange the "roll back or write?"-checks to a straightforward if-else on `found`. This also future-proves the code by making it obvious that we intend to take exactly one of these paths. Improved-by: Jeff King Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- sha1_file.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 5a2014811f..1d1747099a 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -456,19 +456,19 @@ struct alternate_object_database *alloc_alt_odb(const char *dir) void add_to_alternates_file(const char *reference) { - struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); + struct lock_file lock = LOCK_INIT; char *alts = git_pathdup("objects/info/alternates"); FILE *in, *out; + int found = 0; - hold_lock_file_for_update(lock, alts, LOCK_DIE_ON_ERROR); - out = fdopen_lock_file(lock, "w"); + hold_lock_file_for_update(&lock, alts, LOCK_DIE_ON_ERROR); + out = fdopen_lock_file(&lock, "w"); if (!out) die_errno("unable to fdopen alternates lockfile"); in = fopen(alts, "r"); if (in) { struct strbuf line = STRBUF_INIT; - int found = 0; while (strbuf_getline(&line, in) != EOF) { if (!strcmp(reference, line.buf)) { @@ -480,18 +480,15 @@ void add_to_alternates_file(const char *reference) strbuf_release(&line); fclose(in); - - if (found) { - rollback_lock_file(lock); - lock = NULL; - } } else if (errno != ENOENT) die_errno("unable to read alternates file"); - if (lock) { + if (found) { + rollback_lock_file(&lock); + } else { fprintf_or_die(out, "%s\n", reference); - if (commit_lock_file(lock)) + if (commit_lock_file(&lock)) die_errno("unable to move new alternates file into place"); if (alt_odb_tail) link_alt_odb_entries(reference, '\n', NULL, 0); From 837e34eba47f209a38fc9ab458bd103fd7515325 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Thu, 5 Oct 2017 22:32:04 +0200 Subject: [PATCH 02/12] treewide: prefer lockfiles on the stack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is no longer any need to allocate and leak a `struct lock_file`. The previous patch addressed an instance where we needed a minor tweak alongside the trivial changes. Deal with the remaining instances where we allocate and leak a struct within a single function. Change them to have the `struct lock_file` on the stack instead. These instances were identified by running `git grep "^\s*struct lock_file\s*\*"`. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- builtin/am.c | 24 +++++++++++------------- builtin/checkout.c | 14 ++++++-------- builtin/clone.c | 7 +++---- builtin/diff.c | 7 +++---- config.c | 17 ++++++++--------- merge-recursive.c | 6 +++--- merge.c | 8 ++++---- wt-status.c | 8 ++++---- 8 files changed, 42 insertions(+), 49 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index d7513f5375..4e16fd428f 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1134,11 +1134,11 @@ static const char *msgnum(const struct am_state *state) */ static void refresh_and_write_cache(void) { - struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file)); + struct lock_file lock_file = LOCK_INIT; - hold_locked_index(lock_file, LOCK_DIE_ON_ERROR); + hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR); refresh_cache(REFRESH_QUIET); - if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) + if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) die(_("unable to write index file")); } @@ -1946,15 +1946,14 @@ next: */ static int fast_forward_to(struct tree *head, struct tree *remote, int reset) { - struct lock_file *lock_file; + struct lock_file lock_file = LOCK_INIT; struct unpack_trees_options opts; struct tree_desc t[2]; if (parse_tree(head) || parse_tree(remote)) return -1; - lock_file = xcalloc(1, sizeof(struct lock_file)); - hold_locked_index(lock_file, LOCK_DIE_ON_ERROR); + hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR); refresh_cache(REFRESH_QUIET); @@ -1970,11 +1969,11 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset) init_tree_desc(&t[1], remote->buffer, remote->size); if (unpack_trees(2, t, &opts)) { - rollback_lock_file(lock_file); + rollback_lock_file(&lock_file); return -1; } - if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) + if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) die(_("unable to write new index file")); return 0; @@ -1986,15 +1985,14 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset) */ static int merge_tree(struct tree *tree) { - struct lock_file *lock_file; + struct lock_file lock_file = LOCK_INIT; struct unpack_trees_options opts; struct tree_desc t[1]; if (parse_tree(tree)) return -1; - lock_file = xcalloc(1, sizeof(struct lock_file)); - hold_locked_index(lock_file, LOCK_DIE_ON_ERROR); + hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR); memset(&opts, 0, sizeof(opts)); opts.head_idx = 1; @@ -2005,11 +2003,11 @@ static int merge_tree(struct tree *tree) init_tree_desc(&t[0], tree->buffer, tree->size); if (unpack_trees(1, t, &opts)) { - rollback_lock_file(lock_file); + rollback_lock_file(&lock_file); return -1; } - if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) + if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) die(_("unable to write new index file")); return 0; diff --git a/builtin/checkout.c b/builtin/checkout.c index 3345a0d16f..34c3c5b615 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -247,7 +247,7 @@ static int checkout_paths(const struct checkout_opts *opts, struct object_id rev; struct commit *head; int errs = 0; - struct lock_file *lock_file; + struct lock_file lock_file = LOCK_INIT; if (opts->track != BRANCH_TRACK_UNSPECIFIED) die(_("'%s' cannot be used with updating paths"), "--track"); @@ -275,9 +275,7 @@ static int checkout_paths(const struct checkout_opts *opts, return run_add_interactive(revision, "--patch=checkout", &opts->pathspec); - lock_file = xcalloc(1, sizeof(struct lock_file)); - - hold_locked_index(lock_file, LOCK_DIE_ON_ERROR); + hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR); if (read_cache_preload(&opts->pathspec) < 0) return error(_("index file corrupt")); @@ -376,7 +374,7 @@ static int checkout_paths(const struct checkout_opts *opts, } errs |= finish_delayed_checkout(&state); - if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) + if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) die(_("unable to write new index file")); read_ref_full("HEAD", 0, rev.hash, NULL); @@ -472,9 +470,9 @@ static int merge_working_tree(const struct checkout_opts *opts, int *writeout_error) { int ret; - struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file)); + struct lock_file lock_file = LOCK_INIT; - hold_locked_index(lock_file, LOCK_DIE_ON_ERROR); + hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR); if (read_cache_preload(NULL) < 0) return error(_("index file corrupt")); @@ -591,7 +589,7 @@ static int merge_working_tree(const struct checkout_opts *opts, if (!cache_tree_fully_valid(active_cache_tree)) cache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR); - if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) + if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) die(_("unable to write new index file")); if (!opts->force && !opts->quiet) diff --git a/builtin/clone.c b/builtin/clone.c index dbddd98f80..96a3aaaa1f 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -706,7 +706,7 @@ static int checkout(int submodule_progress) { struct object_id oid; char *head; - struct lock_file *lock_file; + struct lock_file lock_file = LOCK_INIT; struct unpack_trees_options opts; struct tree *tree; struct tree_desc t; @@ -733,8 +733,7 @@ static int checkout(int submodule_progress) /* We need to be in the new work tree for the checkout */ setup_work_tree(); - lock_file = xcalloc(1, sizeof(struct lock_file)); - hold_locked_index(lock_file, LOCK_DIE_ON_ERROR); + hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR); memset(&opts, 0, sizeof opts); opts.update = 1; @@ -750,7 +749,7 @@ static int checkout(int submodule_progress) if (unpack_trees(1, &t, &opts) < 0) die(_("unable to checkout working tree")); - if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) + if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) die(_("unable to write new index file")); err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1), diff --git a/builtin/diff.c b/builtin/diff.c index 7e3ebcea38..91995965d7 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -203,17 +203,16 @@ static int builtin_diff_combined(struct rev_info *revs, static void refresh_index_quietly(void) { - struct lock_file *lock_file; + struct lock_file lock_file = LOCK_INIT; int fd; - lock_file = xcalloc(1, sizeof(struct lock_file)); - fd = hold_locked_index(lock_file, 0); + fd = hold_locked_index(&lock_file, 0); if (fd < 0) return; discard_cache(); read_cache(); refresh_cache(REFRESH_QUIET|REFRESH_UNMERGED); - update_index_if_able(&the_index, lock_file); + update_index_if_able(&the_index, &lock_file); } static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv) diff --git a/config.c b/config.c index 345d78c2ba..0e61e863d6 100644 --- a/config.c +++ b/config.c @@ -2748,7 +2748,7 @@ int git_config_rename_section_in_file(const char *config_filename, { int ret = 0, remove = 0; char *filename_buf = NULL; - struct lock_file *lock; + struct lock_file lock = LOCK_INIT; int out_fd; char buf[1024]; FILE *config_file = NULL; @@ -2762,8 +2762,7 @@ int git_config_rename_section_in_file(const char *config_filename, if (!config_filename) config_filename = filename_buf = git_pathdup("config"); - lock = xcalloc(1, sizeof(struct lock_file)); - out_fd = hold_lock_file_for_update(lock, config_filename, 0); + out_fd = hold_lock_file_for_update(&lock, config_filename, 0); if (out_fd < 0) { ret = error("could not lock config file %s", config_filename); goto out; @@ -2782,9 +2781,9 @@ int git_config_rename_section_in_file(const char *config_filename, goto out; } - if (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) { + if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) { ret = error_errno("chmod on %s failed", - get_lock_file_path(lock)); + get_lock_file_path(&lock)); goto out; } @@ -2805,7 +2804,7 @@ int git_config_rename_section_in_file(const char *config_filename, } store.baselen = strlen(new_name); if (write_section(out_fd, new_name) < 0) { - ret = write_error(get_lock_file_path(lock)); + ret = write_error(get_lock_file_path(&lock)); goto out; } /* @@ -2831,20 +2830,20 @@ int git_config_rename_section_in_file(const char *config_filename, continue; length = strlen(output); if (write_in_full(out_fd, output, length) < 0) { - ret = write_error(get_lock_file_path(lock)); + ret = write_error(get_lock_file_path(&lock)); goto out; } } fclose(config_file); config_file = NULL; commit_and_out: - if (commit_lock_file(lock) < 0) + if (commit_lock_file(&lock) < 0) ret = error_errno("could not write config file %s", config_filename); out: if (config_file) fclose(config_file); - rollback_lock_file(lock); + rollback_lock_file(&lock); out_no_rollback: free(filename_buf); return ret; diff --git a/merge-recursive.c b/merge-recursive.c index 1d3f8f0d22..24c5c26a6a 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2162,7 +2162,7 @@ int merge_recursive_generic(struct merge_options *o, struct commit **result) { int clean; - struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); + struct lock_file lock = LOCK_INIT; struct commit *head_commit = get_ref(head, o->branch1); struct commit *next_commit = get_ref(merge, o->branch2); struct commit_list *ca = NULL; @@ -2178,14 +2178,14 @@ int merge_recursive_generic(struct merge_options *o, } } - hold_locked_index(lock, LOCK_DIE_ON_ERROR); + hold_locked_index(&lock, LOCK_DIE_ON_ERROR); clean = merge_recursive(o, head_commit, next_commit, ca, result); if (clean < 0) return clean; if (active_cache_changed && - write_locked_index(&the_index, lock, COMMIT_LOCK)) + write_locked_index(&the_index, &lock, COMMIT_LOCK)) return err(o, _("Unable to write index.")); return clean ? 0 : 1; diff --git a/merge.c b/merge.c index 1d441ad942..a18a452b5c 100644 --- a/merge.c +++ b/merge.c @@ -53,11 +53,11 @@ int checkout_fast_forward(const struct object_id *head, struct tree_desc t[MAX_UNPACK_TREES]; int i, nr_trees = 0; struct dir_struct dir; - struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file)); + struct lock_file lock_file = LOCK_INIT; refresh_cache(REFRESH_QUIET); - if (hold_locked_index(lock_file, LOCK_REPORT_ON_ERROR) < 0) + if (hold_locked_index(&lock_file, LOCK_REPORT_ON_ERROR) < 0) return -1; memset(&trees, 0, sizeof(trees)); @@ -91,8 +91,8 @@ int checkout_fast_forward(const struct object_id *head, } if (unpack_trees(nr_trees, t, &opts)) return -1; - if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) { - rollback_lock_file(lock_file); + if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) { + rollback_lock_file(&lock_file); return error(_("unable to write new index file")); } return 0; diff --git a/wt-status.c b/wt-status.c index 6f730ee8f2..7d88e7ca85 100644 --- a/wt-status.c +++ b/wt-status.c @@ -2299,14 +2299,14 @@ int has_uncommitted_changes(int ignore_submodules) */ int require_clean_work_tree(const char *action, const char *hint, int ignore_submodules, int gently) { - struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file)); + struct lock_file lock_file = LOCK_INIT; int err = 0, fd; - fd = hold_locked_index(lock_file, 0); + fd = hold_locked_index(&lock_file, 0); refresh_cache(REFRESH_QUIET); if (0 <= fd) - update_index_if_able(&the_index, lock_file); - rollback_lock_file(lock_file); + update_index_if_able(&the_index, &lock_file); + rollback_lock_file(&lock_file); if (has_unstaged_changes(ignore_submodules)) { /* TRANSLATORS: the action is e.g. "pull with rebase" */ From d613576dfe7010b13157e63b2401d321074bbab8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Thu, 5 Oct 2017 22:32:05 +0200 Subject: [PATCH 03/12] lockfile: fix documentation on `close_lock_file_gently()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 83a3069a3 (lockfile: do not rollback lock on failed close, 2017-09-05) forgot to update the documentation by the function definition to reflect that the lock is not rolled back in case closing fails. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- lockfile.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lockfile.h b/lockfile.h index 7c1c484d7c..f401c979f0 100644 --- a/lockfile.h +++ b/lockfile.h @@ -240,8 +240,8 @@ extern char *get_locked_file_path(struct lock_file *lk); * If the lockfile is still open, close it (and the file pointer if it * has been opened using `fdopen_lock_file()`) without renaming the * lockfile over the file being locked. Return 0 upon success. On - * failure to `close(2)`, return a negative value and roll back the - * lock file. Usually `commit_lock_file()`, `commit_lock_file_to()`, + * failure to `close(2)`, return a negative value (the lockfile is not + * rolled back). Usually `commit_lock_file()`, `commit_lock_file_to()`, * or `rollback_lock_file()` should eventually be called. */ static inline int close_lock_file_gently(struct lock_file *lk) From 5de134ca8594de22f2a0b738824f9b5176ae6c00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Thu, 5 Oct 2017 22:32:06 +0200 Subject: [PATCH 04/12] tempfile: fix documentation on `delete_tempfile()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The function has always been documented as returning 0 or -1. It is in fact `void`. Correct that. As part of the rearrangements we lose the mention that `delete_tempfile()` might set `errno`. Because there is no return value, the user can't really know whether it did anyway. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- tempfile.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tempfile.h b/tempfile.h index b8f4b5e145..450908b2e0 100644 --- a/tempfile.h +++ b/tempfile.h @@ -68,10 +68,10 @@ * `create_tempfile()` returns an allocated tempfile on success or NULL * on failure. On errors, `errno` describes the reason for failure. * - * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile_gently()` - * return 0 on success. On failure they set `errno` appropriately and return - * -1. `delete` and `rename` (but not `close`) do their best to delete the - * temporary file before returning. + * `rename_tempfile()` and `close_tempfile_gently()` return 0 on success. + * On failure they set `errno` appropriately and return -1. + * `delete_tempfile()` and `rename` (but not `close`) do their best to + * delete the temporary file before returning. */ struct tempfile { From 02ae242fdde2ba8acb6b87e32df2b1e33786e78b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Thu, 5 Oct 2017 22:32:07 +0200 Subject: [PATCH 05/12] checkout-index: simplify locking logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `newfd` starts out negative. If we then take the lock, `newfd` will become non-negative. We later check for exactly that property before calling `write_locked_index()`. That is, we are simply using `newfd` as a boolean to keep track of whether we took the lock or not. (We always use `newfd` and `lock_file` together, so they really are mirroring each other.) Drop `newfd` and check with `is_lock_file_locked()` instead. While at it, move the `static struct lock_file` into `cmd_checkout_index()` and make it non-static. It is only used in this function, and after 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we can have lockfiles on the stack. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- builtin/checkout-index.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c index 39c8be05dc..b0e78b819d 100644 --- a/builtin/checkout-index.c +++ b/builtin/checkout-index.c @@ -129,8 +129,6 @@ static const char * const builtin_checkout_index_usage[] = { NULL }; -static struct lock_file lock_file; - static int option_parse_stage(const struct option *opt, const char *arg, int unset) { @@ -150,7 +148,7 @@ static int option_parse_stage(const struct option *opt, int cmd_checkout_index(int argc, const char **argv, const char *prefix) { int i; - int newfd = -1; + struct lock_file lock_file = LOCK_INIT; int all = 0; int read_from_stdin = 0; int prefix_length; @@ -206,7 +204,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) if (index_opt && !state.base_dir_len && !to_tempfile) { state.refresh_cache = 1; state.istate = &the_index; - newfd = hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR); + hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR); } /* Check out named files first */ @@ -251,7 +249,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) if (all) checkout_all(prefix, prefix_length); - if (0 <= newfd && + if (is_lock_file_locked(&lock_file) && write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) die("Unable to write new index file"); return 0; From 2954e5ec4361c6932593663859aa437583edb500 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Thu, 5 Oct 2017 22:32:08 +0200 Subject: [PATCH 06/12] cache-tree: simplify locking logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After we have taken the lock using `LOCK_DIE_ON_ERROR`, we know that `newfd` is non-negative. So when we check for exactly that property before calling `write_locked_index()`, the outcome is guaranteed. If we write and commit successfully, we set `newfd = -1`, so that we can later avoid calling `rollback_lock_file` on an already-committed lock. But we might just as well unconditionally call `rollback_lock_file()` -- it will be a no-op if we have already committed. All in all, we use `newfd` as a bool and the only benefit we get from it is that we can avoid calling a no-op. Remove `newfd` so that we have one variable less to reason about. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- cache-tree.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/cache-tree.c b/cache-tree.c index 71d092ed51..f646f56731 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -602,11 +602,11 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, const char *index_path, int flags, const char *prefix) { - int entries, was_valid, newfd; + int entries, was_valid; struct lock_file lock_file = LOCK_INIT; int ret = 0; - newfd = hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR); + hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR); entries = read_index_from(index_state, index_path); if (entries < 0) { @@ -625,10 +625,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co ret = WRITE_TREE_UNMERGED_INDEX; goto out; } - if (0 <= newfd) { - if (!write_locked_index(index_state, &lock_file, COMMIT_LOCK)) - newfd = -1; - } + write_locked_index(index_state, &lock_file, COMMIT_LOCK); /* Not being able to write is fine -- we are only interested * in updating the cache-tree part, and if the next caller * ends up using the old index with unupdated cache-tree part @@ -650,8 +647,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co hashcpy(sha1, index_state->cache_tree->oid.hash); out: - if (0 <= newfd) - rollback_lock_file(&lock_file); + rollback_lock_file(&lock_file); return ret; } From 6d058c882643dc45c8d708be98e86c38f25511a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Thu, 5 Oct 2017 22:32:09 +0200 Subject: [PATCH 07/12] apply: move lockfile into `apply_state` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We have two users of `struct apply_state` and the related functionality in apply.c. Each user sets up its `apply_state` by handing over a pointer to its static `lock_file`. (Before 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we could never free lockfiles, so making them static was a reasonable approach.) Other than that, they never directly access their `lock_file`s, which are instead handled by the functionality in apply.c. To make life easier for the caller and to make it less tempting for a future caller to mess with the lock, make apply.c fully responsible for setting up the `lock_file`. As mentioned above, it is now safe to free a `lock_file`, so we can make the `struct apply_state` contain an actual `struct lock_file` instead of a pointer to one. The user in builtin/apply.c is rather simple. For builtin/am.c, we might worry that the lock state is actually meant to be inherited across calls. But the lock is only taken as `apply_all_patches()` executes, and code inspection shows that it will always be released. Alternatively, we can observe that the lock itself is never queried directly. When we decide whether we should lock, we check a related variable `newfd`. That variable is not inherited, so from the point of view of apply.c, the state machine really is reset with each call to `init_apply_state()`. (It would be a bug if `newfd` and the lock status were not in sync. The duplication of information in `newfd` and the lock will be addressed in the next patch.) Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- apply.c | 14 +++++--------- apply.h | 5 ++--- builtin/am.c | 3 +-- builtin/apply.c | 4 +--- 4 files changed, 9 insertions(+), 17 deletions(-) diff --git a/apply.c b/apply.c index c022af53a2..5a6ca10a7a 100644 --- a/apply.c +++ b/apply.c @@ -75,12 +75,10 @@ static int parse_ignorewhitespace_option(struct apply_state *state, } int init_apply_state(struct apply_state *state, - const char *prefix, - struct lock_file *lock_file) + const char *prefix) { memset(state, 0, sizeof(*state)); state->prefix = prefix; - state->lock_file = lock_file; state->newfd = -1; state->apply = 1; state->line_termination = '\n'; @@ -146,8 +144,6 @@ int check_apply_state(struct apply_state *state, int force_apply) } if (state->check_index) state->unsafe_paths = 0; - if (!state->lock_file) - return error("BUG: state->lock_file should not be NULL"); if (state->apply_verbosity <= verbosity_silent) { state->saved_error_routine = get_error_routine(); @@ -4711,11 +4707,11 @@ static int apply_patch(struct apply_state *state, state->update_index = state->check_index && state->apply; if (state->update_index && state->newfd < 0) { if (state->index_file) - state->newfd = hold_lock_file_for_update(state->lock_file, + state->newfd = hold_lock_file_for_update(&state->lock_file, state->index_file, LOCK_DIE_ON_ERROR); else - state->newfd = hold_locked_index(state->lock_file, LOCK_DIE_ON_ERROR); + state->newfd = hold_locked_index(&state->lock_file, LOCK_DIE_ON_ERROR); } if (state->check_index && read_apply_cache(state) < 0) { @@ -4911,7 +4907,7 @@ int apply_all_patches(struct apply_state *state, } if (state->update_index) { - res = write_locked_index(&the_index, state->lock_file, COMMIT_LOCK); + res = write_locked_index(&the_index, &state->lock_file, COMMIT_LOCK); if (res) { error(_("Unable to write new index file")); res = -128; @@ -4924,7 +4920,7 @@ int apply_all_patches(struct apply_state *state, end: if (state->newfd >= 0) { - rollback_lock_file(state->lock_file); + rollback_lock_file(&state->lock_file); state->newfd = -1; } diff --git a/apply.h b/apply.h index d9b3957703..cf00cda171 100644 --- a/apply.h +++ b/apply.h @@ -37,7 +37,7 @@ struct apply_state { const char *prefix; /* These are lock_file related */ - struct lock_file *lock_file; + struct lock_file lock_file; int newfd; /* These control what gets looked at and modified */ @@ -116,8 +116,7 @@ extern int apply_parse_options(int argc, const char **argv, int *force_apply, int *options, const char * const *apply_usage); extern int init_apply_state(struct apply_state *state, - const char *prefix, - struct lock_file *lock_file); + const char *prefix); extern void clear_apply_state(struct apply_state *state); extern int check_apply_state(struct apply_state *state, int force_apply); diff --git a/builtin/am.c b/builtin/am.c index 4e16fd428f..40968428dd 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1488,11 +1488,10 @@ static int run_apply(const struct am_state *state, const char *index_file) struct argv_array apply_opts = ARGV_ARRAY_INIT; struct apply_state apply_state; int res, opts_left; - static struct lock_file lock_file; int force_apply = 0; int options = 0; - if (init_apply_state(&apply_state, NULL, &lock_file)) + if (init_apply_state(&apply_state, NULL)) die("BUG: init_apply_state() failed"); argv_array_push(&apply_opts, "apply"); diff --git a/builtin/apply.c b/builtin/apply.c index 81b9a61c37..48d3989331 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -9,8 +9,6 @@ static const char * const apply_usage[] = { NULL }; -static struct lock_file lock_file; - int cmd_apply(int argc, const char **argv, const char *prefix) { int force_apply = 0; @@ -18,7 +16,7 @@ int cmd_apply(int argc, const char **argv, const char *prefix) int ret; struct apply_state state; - if (init_apply_state(&state, prefix, &lock_file)) + if (init_apply_state(&state, prefix)) exit(128); argc = apply_parse_options(argc, argv, From d13cd4c92760d8fe01fb51fbab556a4d1f3713b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Thu, 5 Oct 2017 22:32:10 +0200 Subject: [PATCH 08/12] apply: remove `newfd` from `struct apply_state` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Similar to a previous patch, we do not need to use `newfd` to signal that we have a lockfile to clean up. We can just unconditionally call `rollback_lock_file`. If we do not hold the lock, it will be a no-op. Where we check `newfd` to decide whether we need to take the lock, we can instead use `is_lock_file_locked()`. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- apply.c | 17 ++++++----------- apply.h | 3 +-- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/apply.c b/apply.c index 5a6ca10a7a..d676debd59 100644 --- a/apply.c +++ b/apply.c @@ -79,7 +79,6 @@ int init_apply_state(struct apply_state *state, { memset(state, 0, sizeof(*state)); state->prefix = prefix; - state->newfd = -1; state->apply = 1; state->line_termination = '\n'; state->p_value = 1; @@ -4705,13 +4704,13 @@ static int apply_patch(struct apply_state *state, state->apply = 0; state->update_index = state->check_index && state->apply; - if (state->update_index && state->newfd < 0) { + if (state->update_index && !is_lock_file_locked(&state->lock_file)) { if (state->index_file) - state->newfd = hold_lock_file_for_update(&state->lock_file, - state->index_file, - LOCK_DIE_ON_ERROR); + hold_lock_file_for_update(&state->lock_file, + state->index_file, + LOCK_DIE_ON_ERROR); else - state->newfd = hold_locked_index(&state->lock_file, LOCK_DIE_ON_ERROR); + hold_locked_index(&state->lock_file, LOCK_DIE_ON_ERROR); } if (state->check_index && read_apply_cache(state) < 0) { @@ -4913,16 +4912,12 @@ int apply_all_patches(struct apply_state *state, res = -128; goto end; } - state->newfd = -1; } res = !!errs; end: - if (state->newfd >= 0) { - rollback_lock_file(&state->lock_file); - state->newfd = -1; - } + rollback_lock_file(&state->lock_file); if (state->apply_verbosity <= verbosity_silent) { set_error_routine(state->saved_error_routine); diff --git a/apply.h b/apply.h index cf00cda171..dc4a019057 100644 --- a/apply.h +++ b/apply.h @@ -36,9 +36,8 @@ enum apply_verbosity { struct apply_state { const char *prefix; - /* These are lock_file related */ + /* Lock file */ struct lock_file lock_file; - int newfd; /* These control what gets looked at and modified */ int apply; /* this is not a dry-run */ From 8dc3834610b676c2eb64cfe2a08b8b3d05be05df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Thu, 5 Oct 2017 22:32:11 +0200 Subject: [PATCH 09/12] cache.h: document `write_locked_index()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The next patches will tweak the behavior of this function. Document it in order to establish a basis for those patches. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- cache.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/cache.h b/cache.h index ea6c236e0f..e9d9556e34 100644 --- a/cache.h +++ b/cache.h @@ -601,9 +601,25 @@ extern int do_read_index(struct index_state *istate, const char *path, extern int read_index_from(struct index_state *, const char *path); extern int is_index_unborn(struct index_state *); extern int read_index_unmerged(struct index_state *); + +/* For use with `write_locked_index()`. */ #define COMMIT_LOCK (1 << 0) #define CLOSE_LOCK (1 << 1) + +/* + * Write the index while holding an already-taken lock. The flags may + * contain at most one of `COMMIT_LOCK` and `CLOSE_LOCK`. + * + * Unless a split index is in use, write the index into the lockfile. + * + * With a split index, write the shared index to a temporary file, + * adjust its permissions and rename it into place, then write the + * split index to the lockfile. If the temporary file for the shared + * index cannot be created, fall back to the behavior described in + * the previous paragraph. + */ extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags); + extern int discard_index(struct index_state *); extern void move_index_extensions(struct index_state *dst, struct index_state *src); extern int unmerged_index(const struct index_state *); From 812d6b00750b56fc4b6a75277a30c628cc7be2ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Fri, 6 Oct 2017 22:12:12 +0200 Subject: [PATCH 10/12] read-cache: drop explicit `CLOSE_LOCK`-flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `write_locked_index()` takes two flags: `COMMIT_LOCK` and `CLOSE_LOCK`. At most one is allowed. But it is also possible to use no flag, i.e., `0`. But when `write_locked_index()` calls `do_write_index()`, the temporary file, a.k.a. the lockfile, will be closed. So passing `0` is effectively the same as `CLOSE_LOCK`, which seems like a bug. We might feel tempted to restructure the code in order to close the file later, or conditionally. It also feels a bit unfortunate that we simply "happen" to close the lock by way of an implementation detail of lockfiles. But note that we need to close the temporary file before `stat`-ing it, at least on Windows. See 9f41c7a6b (read-cache: close index.lock in do_write_index, 2017-04-26). Drop `CLOSE_LOCK` and make it explicit that `write_locked_index()` always closes the lock. Whether it is also committed is governed by the remaining flag, `COMMIT_LOCK`. This means we neither have nor suggest that we have a mode to write the index and leave the file open. Whatever extra contents we might eventually want to write, we should probably write it from within `write_locked_index()` itself anyway. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- builtin/commit.c | 10 +++++----- cache.h | 5 ++--- read-cache.c | 14 ++++++++------ 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 0f8ddb6866..32dc2101f5 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -355,7 +355,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix refresh_cache_or_die(refresh_flags); - if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK)) + if (write_locked_index(&the_index, &index_lock, 0)) die(_("unable to create temporary index")); old_index_env = getenv(INDEX_ENVIRONMENT); @@ -374,7 +374,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) { if (reopen_lock_file(&index_lock) < 0) die(_("unable to write index file")); - if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK)) + if (write_locked_index(&the_index, &index_lock, 0)) die(_("unable to update temporary index")); } else warning(_("Failed to update main cache tree")); @@ -401,7 +401,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix add_files_to_cache(also ? prefix : NULL, &pathspec, 0); refresh_cache_or_die(refresh_flags); update_main_cache_tree(WRITE_TREE_SILENT); - if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK)) + if (write_locked_index(&the_index, &index_lock, 0)) die(_("unable to write new_index file")); commit_style = COMMIT_NORMAL; ret = get_lock_file_path(&index_lock); @@ -474,7 +474,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix add_remove_files(&partial); refresh_cache(REFRESH_QUIET); update_main_cache_tree(WRITE_TREE_SILENT); - if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK)) + if (write_locked_index(&the_index, &index_lock, 0)) die(_("unable to write new_index file")); hold_lock_file_for_update(&false_lock, @@ -486,7 +486,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix add_remove_files(&partial); refresh_cache(REFRESH_QUIET); - if (write_locked_index(&the_index, &false_lock, CLOSE_LOCK)) + if (write_locked_index(&the_index, &false_lock, 0)) die(_("unable to write temporary index file")); discard_cache(); diff --git a/cache.h b/cache.h index e9d9556e34..21a6856c5b 100644 --- a/cache.h +++ b/cache.h @@ -604,11 +604,10 @@ extern int read_index_unmerged(struct index_state *); /* For use with `write_locked_index()`. */ #define COMMIT_LOCK (1 << 0) -#define CLOSE_LOCK (1 << 1) /* - * Write the index while holding an already-taken lock. The flags may - * contain at most one of `COMMIT_LOCK` and `CLOSE_LOCK`. + * Write the index while holding an already-taken lock. Close the lock, + * and if `COMMIT_LOCK` is given, commit it. * * Unless a split index is in use, write the index into the lockfile. * diff --git a/read-cache.c b/read-cache.c index 65f4fe8375..c7aa3632a2 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2187,6 +2187,13 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfile rollback_lock_file(lockfile); } +/* + * On success, `tempfile` is closed. If it is the temporary file + * of a `struct lock_file`, we will therefore effectively perform + * a 'close_lock_file_gently()`. Since that is an implementation + * detail of lockfiles, callers of `do_write_index()` should not + * rely on it. + */ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, int strip_extensions) { @@ -2343,14 +2350,9 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l int ret = do_write_index(istate, lock->tempfile, 0); if (ret) return ret; - assert((flags & (COMMIT_LOCK | CLOSE_LOCK)) != - (COMMIT_LOCK | CLOSE_LOCK)); if (flags & COMMIT_LOCK) return commit_locked_index(lock); - else if (flags & CLOSE_LOCK) - return close_lock_file_gently(lock); - else - return ret; + return close_lock_file_gently(lock); } static int write_split_index(struct index_state *istate, From df60cf5789782191b092169f86255aa44525b7d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Fri, 6 Oct 2017 22:12:13 +0200 Subject: [PATCH 11/12] read-cache: leave lock in right state in `write_locked_index()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the original version of `write_locked_index()` returned with an error, it didn't roll back the lockfile unless the error occured at the very end, during closing/committing. See commit 03b866477 (read-cache: new API write_locked_index instead of write_index/write_cache, 2014-06-13). In commit 9f41c7a6b (read-cache: close index.lock in do_write_index, 2017-04-26), we learned to close the lock slightly earlier in the callstack. That was mostly a side-effect of lockfiles being implemented using temporary files, but didn't cause any real harm. Recently, commit 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05) introduced a subtle bug. If the temporary file is deleted (i.e., the lockfile is rolled back), the tempfile-pointer in the `struct lock_file` will be left dangling. Thus, an attempt to reuse the lockfile, or even just to roll it back, will induce undefined behavior -- most likely a crash. Besides not crashing, we clearly want to make things consistent. The guarantees which the lockfile-machinery itself provides is A) if we ask to commit and it fails, roll back, and B) if we ask to close and it fails, do _not_ roll back. Let's do the same for consistency. Do not delete the temporary file in `do_write_index()`. One of its callers, `write_locked_index()` will thereby avoid rolling back the lock. The other caller, `write_shared_index()`, will delete its temporary file anyway. Both of these callers will avoid undefined behavior (crashing). Teach `write_locked_index(..., COMMIT_LOCK)` to roll back the lock before returning. If we have already succeeded and committed, it will be a noop. Simplify the existing callers where we now have a superfluous call to `rollback_lockfile()`. That should keep future readers from wondering why the callers are inconsistent. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- builtin/difftool.c | 1 - cache.h | 4 ++++ merge.c | 4 +--- read-cache.c | 14 ++++++++------ sequencer.c | 1 - 5 files changed, 13 insertions(+), 11 deletions(-) diff --git a/builtin/difftool.c b/builtin/difftool.c index b2d3ba7539..bcc79d1888 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -616,7 +616,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, if (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 || write_locked_index(&wtindex, &lock, COMMIT_LOCK)) { ret = error("could not write %s", buf.buf); - rollback_lock_file(&lock); goto finish; } changed_files(&wt_modified, buf.buf, workdir); diff --git a/cache.h b/cache.h index 21a6856c5b..0e26224b9e 100644 --- a/cache.h +++ b/cache.h @@ -616,6 +616,10 @@ extern int read_index_unmerged(struct index_state *); * split index to the lockfile. If the temporary file for the shared * index cannot be created, fall back to the behavior described in * the previous paragraph. + * + * With `COMMIT_LOCK`, the lock is always committed or rolled back. + * Without it, the lock is closed, but neither committed nor rolled + * back. */ extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags); diff --git a/merge.c b/merge.c index a18a452b5c..e5d796c9f2 100644 --- a/merge.c +++ b/merge.c @@ -91,9 +91,7 @@ int checkout_fast_forward(const struct object_id *head, } if (unpack_trees(nr_trees, t, &opts)) return -1; - if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) { - rollback_lock_file(&lock_file); + if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) return error(_("unable to write new index file")); - } return 0; } diff --git a/read-cache.c b/read-cache.c index c7aa3632a2..0d8d2dedee 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2182,9 +2182,8 @@ static int has_racy_timestamp(struct index_state *istate) void update_index_if_able(struct index_state *istate, struct lock_file *lockfile) { if ((istate->cache_changed || has_racy_timestamp(istate)) && - verify_index(istate) && - write_locked_index(istate, lockfile, COMMIT_LOCK)) - rollback_lock_file(lockfile); + verify_index(istate)) + write_locked_index(istate, lockfile, COMMIT_LOCK); } /* @@ -2321,7 +2320,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, return -1; if (close_tempfile_gently(tempfile)) { error(_("could not close '%s'"), tempfile->filename.buf); - delete_tempfile(&tempfile); return -1; } if (stat(tempfile->filename.buf, &st)) @@ -2501,7 +2499,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock, (istate->cache_changed & ~EXTMASK)) { if (si) hashclr(si->base_sha1); - return do_write_locked_index(istate, lock, flags); + ret = do_write_locked_index(istate, lock, flags); + goto out; } if (getenv("GIT_TEST_SPLIT_INDEX")) { @@ -2517,7 +2516,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock, if (new_shared_index) { ret = write_shared_index(istate, lock, flags); if (ret) - return ret; + goto out; } ret = write_split_index(istate, lock, flags); @@ -2526,6 +2525,9 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock, if (!ret && !new_shared_index) freshen_shared_index(sha1_to_hex(si->base_sha1), 1); +out: + if (flags & COMMIT_LOCK) + rollback_lock_file(lock); return ret; } diff --git a/sequencer.c b/sequencer.c index 60636ce54b..d56c380819 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1183,7 +1183,6 @@ static int read_and_refresh_cache(struct replay_opts *opts) refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL); if (the_index.cache_changed && index_fd >= 0) { if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) { - rollback_lock_file(&index_lock); return error(_("git %s: failed to refresh the index"), _(action_name(opts))); } From b74c90fb419b002c664b0236f2941c34786b18b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Fri, 6 Oct 2017 22:12:14 +0200 Subject: [PATCH 12/12] read_cache: roll back lock in `update_index_if_able()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `update_index_if_able()` used to always commit the lock or roll it back. Commit 03b866477 (read-cache: new API write_locked_index instead of write_index/write_cache, 2014-06-13) stopped rolling it back in case a write was not even attempted. This change in behavior is not motivated in the commit message and appears to be accidental: the `else`-path was removed, although that changed the behavior in case the `if` shortcuts. Reintroduce the rollback and document this behavior. While at it, move the documentation on this function from the function definition to the function declaration in cache.h. If `write_locked_index(..., COMMIT_LOCK)` fails, it will roll back the lock for us (see the previous commit). Noticed-by: Junio C Hamano Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- cache.h | 4 ++++ read-cache.c | 5 ++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/cache.h b/cache.h index 0e26224b9e..8c24937669 100644 --- a/cache.h +++ b/cache.h @@ -734,6 +734,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st); extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg); extern struct cache_entry *refresh_cache_entry(struct cache_entry *, unsigned int); +/* + * Opportunistically update the index but do not complain if we can't. + * The lockfile is always committed or rolled back. + */ extern void update_index_if_able(struct index_state *, struct lock_file *); extern int hold_locked_index(struct lock_file *, int); diff --git a/read-cache.c b/read-cache.c index 0d8d2dedee..87188d390d 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2176,14 +2176,13 @@ static int has_racy_timestamp(struct index_state *istate) return 0; } -/* - * Opportunistically update the index but do not complain if we can't - */ void update_index_if_able(struct index_state *istate, struct lock_file *lockfile) { if ((istate->cache_changed || has_racy_timestamp(istate)) && verify_index(istate)) write_locked_index(istate, lockfile, COMMIT_LOCK); + else + rollback_lock_file(lockfile); } /*