Merge branch 'ma/lockfile-fixes'

An earlier update made it possible to use an on-stack in-core
lockfile structure (as opposed to having to deliberately leak an
on-heap one).  Many codepaths have been updated to take advantage
of this new facility.

* ma/lockfile-fixes:
read_cache: roll back lock in `update_index_if_able()`
read-cache: leave lock in right state in `write_locked_index()`
read-cache: drop explicit `CLOSE_LOCK`-flag
cache.h: document `write_locked_index()`
apply: remove `newfd` from `struct apply_state`
apply: move lockfile into `apply_state`
cache-tree: simplify locking logic
checkout-index: simplify locking logic
tempfile: fix documentation on `delete_tempfile()`
lockfile: fix documentation on `close_lock_file_gently()`
treewide: prefer lockfiles on the stack
sha1_file: do not leak `lock_file`

diff --git a/apply.c b/apply.c
index c022af53a24bf56556d136a3456862cf6a2ca7cc..d676debd5964b90b2d0ca56bc81e58d8cf6f5b6a 100644 (file)
--- a/apply.c
+++ b/apply.c
@@ -75,13 +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';
        state->p_value = 1;
@@ -146,8 +143,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();
@@ -4709,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) {
@@ -4911,22 +4906,18 @@ 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;
                        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 d9b395770364b5495fc1136248efd64bd3a5e66d..dc4a0190570d060d23e8464f465232eddcf76deb 100644 (file)
--- a/apply.h
+++ b/apply.h
@@ -36,9 +36,8 @@ enum apply_verbosity {
 struct apply_state {
        const char *prefix;
 
-       /* These are lock_file related */
-       struct lock_file *lock_file;
-       int newfd;
+       /* Lock file */
+       struct lock_file lock_file;
 
        /* These control what gets looked at and modified */
        int apply; /* this is not a dry-run */
@@ -116,8 +115,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 d7513f53759bc5834aa02bd904432f64e036ccb6..40968428dd5f4f966c07c2bed374015e0b399550 100644 (file)
--- 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"));
 }
 
@@ -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");
@@ -1946,15 +1945,14 @@ static void am_resolve(struct am_state *state)
  */
 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 +1968,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 +1984,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 +2002,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/apply.c b/builtin/apply.c
index 81b9a61c3713a718a912d49916cf2632478cb47c..48d3989331d9b7deeb034642a027729bd1267e38 100644 (file)
--- 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,

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 39c8be05dc4beda2f0cd179e5c5f495ce079f8d9..b0e78b819db3138f1c5ac4fe4b5d850ab306b470 100644 (file)
--- 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;

diff --git a/builtin/checkout.c b/builtin/checkout.c
index fc4f8fd2ea29c7c23d3b1c7ca0ba78824c255a91..fcff0de198edb05ca58ab538e12aa77371425e17 100644 (file)
--- 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 dbddd98f80d6660f2cde190a0e121022f079071d..96a3aaaa1f37fbd61d1a7d20f387053cc7118eeb 100644 (file)
--- 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/commit.c b/builtin/commit.c
index d75b3805ea7fe3475564f337bf660d8155909445..af034553fcf22c463d2be04f4aafff7c9d6176dc 100644 (file)
--- 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/builtin/diff.c b/builtin/diff.c
index f5bbd4d757e12e6e34a86fac006298acb771909f..aa6f746795fb82feb4de1ff28f8f0350ee18092c 100644 (file)
--- 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/builtin/difftool.c b/builtin/difftool.c
index b2d3ba7539d5b9cdc7ec6f910ee24510e709f76c..bcc79d1888f2217bcb380ffb1e7178c100a41e8e 100644 (file)
--- 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-tree.c b/cache-tree.c
index d3f7401278bd5130558dde18da1235c188bb7303..e03e72c34a5c1fc618994ee63f38875d28d91886 100644 (file)
--- 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;
 }
 

diff --git a/cache.h b/cache.h
index 6440e2bf21f5800db98f82fcedff46478188674e..d74f00d8db2035c5109e2bf00a1ea25c14c7f24c 100644 (file)
--- a/cache.h
+++ b/cache.h
@@ -602,9 +602,28 @@ 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. Close the lock,
+ * and if `COMMIT_LOCK` is given, commit it.
+ *
+ * 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.
+ *
+ * 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);
+
 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 *);
@@ -716,6 +735,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/config.c b/config.c
index adb7d7a3e5ee164d24f1d2420677a8ad5b61e49f..903abf9533b188fd472c213c29a9f968eb90eb8b 100644 (file)
--- a/config.c
+++ b/config.c
@@ -2751,7 +2751,7 @@ static int git_config_copy_or_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;
@@ -2766,8 +2766,7 @@ static int git_config_copy_or_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;
@@ -2786,9 +2785,9 @@ static int git_config_copy_or_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;
        }
 
@@ -2812,7 +2811,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
                         */
                        if (copystr.len > 0) {
                                if (write_in_full(out_fd, copystr.buf, copystr.len) != copystr.len) {
-                                       ret = write_error(get_lock_file_path(lock));
+                                       ret = write_error(get_lock_file_path(&lock));
                                        goto out;
                                }
                                strbuf_reset(&copystr);
@@ -2828,7 +2827,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
                                store.baselen = strlen(new_name);
                                if (!copy) {
                                        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;
                                        }
                                        /*
@@ -2862,7 +2861,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
                }
 
                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;
                }
        }
@@ -2874,7 +2873,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
         */
        if (copystr.len > 0) {
                if (write_in_full(out_fd, copystr.buf, copystr.len) != copystr.len) {
-                       ret = write_error(get_lock_file_path(lock));
+                       ret = write_error(get_lock_file_path(&lock));
                        goto out;
                }
                strbuf_reset(&copystr);
@@ -2883,13 +2882,13 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
        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/lockfile.h b/lockfile.h
index 7c1c484d7cfdfab126fd0ce947ac40f650e5d107..f401c979f0ed907b40bfb509ab8f06ad2632c19e 100644 (file)
--- 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)

diff --git a/merge-recursive.c b/merge-recursive.c
index 1d3f8f0d22157e11c1e0db6f230e39240d2f3e63..24c5c26a6abdcc6aaeb1defa194d25b50e7ca6e5 100644 (file)
--- 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 1d441ad94254257ad2ac28b8a17dc0b8e5ce14f9..e5d796c9f2d110bdcd378b361fe2052ac1dcc266 100644 (file)
--- 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,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 65f4fe8375d59234d5e58f87a8593fc50e6336a2..87188d390d8eafcb8deb9cd68a27eab340520e0d 100644 (file)
--- a/read-cache.c
+++ b/read-cache.c
@@ -2176,17 +2176,22 @@ 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))
+           verify_index(istate))
+               write_locked_index(istate, lockfile, COMMIT_LOCK);
+       else
                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)
 {
@@ -2314,7 +2319,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))
@@ -2343,14 +2347,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,
@@ -2499,7 +2498,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")) {
@@ -2515,7 +2515,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);
@@ -2524,6 +2524,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 f2a10cc4f24c10adf072eb26ba427f00037b3c79..46c997e2df50f81662df877384fbfc5287fa6d10 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -1184,7 +1184,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)));
                }

diff --git a/sha1_file.c b/sha1_file.c
index 10c3a0083dedee98dc3d48267ad349589adef3f0..00f5b9e0b2ddd93f1e91e2e74c984c7a76f9c11c 100644 (file)
--- 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);

diff --git a/tempfile.h b/tempfile.h
index b8f4b5e145b8b6b1d80cac299dbaedc8253e861b..450908b2e0bc4a94bc23a862abf04a56366aedd7 100644 (file)
--- 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 {

diff --git a/wt-status.c b/wt-status.c
index 29bc64cc0280675065142bac257930904216d415..93ac645225e6d6ce3000ff866015806ce9630383 100644 (file)
--- a/wt-status.c
+++ b/wt-status.c
@@ -2297,14 +2297,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" */