From 14bca6c63cec3bd6e9b636fd1ff5b0ea2bd5edce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Tue, 27 Feb 2018 22:30:09 +0100 Subject: [PATCH 1/5] sequencer: make lockfiles non-static MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we can have lockfiles on the stack. One of these functions fails to always roll back the lock. That will be fixed in the next commit. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- sequencer.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/sequencer.c b/sequencer.c index e9baaf59bd..d9f865926d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -339,7 +339,7 @@ static void print_advice(int show_hint, struct replay_opts *opts) static int write_message(const void *buf, size_t len, const char *filename, int append_eol) { - static struct lock_file msg_file; + struct lock_file msg_file = LOCK_INIT; int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0); if (msg_fd < 0) @@ -485,7 +485,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next, struct tree *result, *next_tree, *base_tree, *head_tree; int clean; char **xopt; - static struct lock_file index_lock; + struct lock_file index_lock = LOCK_INIT; if (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR) < 0) return -1; @@ -1705,7 +1705,7 @@ static int prepare_revs(struct replay_opts *opts) static int read_and_refresh_cache(struct replay_opts *opts) { - static struct lock_file index_lock; + struct lock_file index_lock = LOCK_INIT; int index_fd = hold_locked_index(&index_lock, 0); if (read_index_preload(&the_index, NULL) < 0) { rollback_lock_file(&index_lock); @@ -2099,7 +2099,7 @@ static int create_seq_dir(void) static int save_head(const char *head) { - static struct lock_file head_lock; + struct lock_file head_lock = LOCK_INIT; struct strbuf buf = STRBUF_INIT; int fd; ssize_t written; @@ -2224,7 +2224,7 @@ int sequencer_rollback(struct replay_opts *opts) static int save_todo(struct todo_list *todo_list, struct replay_opts *opts) { - static struct lock_file todo_lock; + struct lock_file todo_lock = LOCK_INIT; const char *todo_path = get_todo_path(opts); int next = todo_list->current, offset, fd; From 6481652432eed5b073428411763d5b5d712b6745 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Tue, 27 Feb 2018 22:30:10 +0100 Subject: [PATCH 2/5] sequencer: always roll back lock in `do_recursive_merge()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we return early, we forget to roll back the lockfile. Do so. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- sequencer.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index d9f865926d..bca6922823 100644 --- a/sequencer.c +++ b/sequencer.c @@ -514,8 +514,10 @@ static int do_recursive_merge(struct commit *base, struct commit *next, fputs(o.obuf.buf, stdout); strbuf_release(&o.obuf); diff_warn_rename_limit("merge.renamelimit", o.needed_rename_limit, 0); - if (clean < 0) + if (clean < 0) { + rollback_lock_file(&index_lock); return clean; + } if (active_cache_changed && write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) From 51d3f43d2fd7f93cd19494eef4aef712fad597c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Wed, 28 Feb 2018 20:07:56 +0100 Subject: [PATCH 3/5] merge-recursive: always roll back lock in `merge_recursive_generic()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we return early, or if `active_cache_changed` is false, we forget to roll back the lockfile. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- merge-recursive.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/merge-recursive.c b/merge-recursive.c index 6ff971f9a2..129577987b 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2218,12 +2218,15 @@ int merge_recursive_generic(struct merge_options *o, hold_locked_index(&lock, LOCK_DIE_ON_ERROR); clean = merge_recursive(o, head_commit, next_commit, ca, result); - if (clean < 0) + if (clean < 0) { + rollback_lock_file(&lock); return clean; + } if (active_cache_changed && write_locked_index(&the_index, &lock, COMMIT_LOCK)) return err(o, _("Unable to write index.")); + rollback_lock_file(&lock); return clean ? 0 : 1; } From 5790d25881a60a1c3a780e997a59774e48c2c026 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Wed, 28 Feb 2018 20:07:57 +0100 Subject: [PATCH 4/5] merge: always roll back lock in `checkout_fast_forward()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This function originated in builtin/merge.c. It was moved to merge.c in commit db699a8a1f (Move try_merge_command and checkout_fast_forward to libgit.a, 2012-10-26), but was used from sequencer.c even before that. If a problem occurs, the function returns without rolling back the lockfile. Teach it to do so. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- merge.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/merge.c b/merge.c index 195b578700..f06a4773d4 100644 --- a/merge.c +++ b/merge.c @@ -113,17 +113,23 @@ int checkout_fast_forward(const struct object_id *head, setup_unpack_trees_porcelain(&opts, "merge"); trees[nr_trees] = parse_tree_indirect(head); - if (!trees[nr_trees++]) + if (!trees[nr_trees++]) { + rollback_lock_file(&lock_file); return -1; + } trees[nr_trees] = parse_tree_indirect(remote); - if (!trees[nr_trees++]) + if (!trees[nr_trees++]) { + rollback_lock_file(&lock_file); return -1; + } for (i = 0; i < nr_trees; i++) { parse_tree(trees[i]); init_tree_desc(t+i, trees[i]->buffer, trees[i]->size); } - if (unpack_trees(nr_trees, t, &opts)) + if (unpack_trees(nr_trees, t, &opts)) { + rollback_lock_file(&lock_file); return -1; + } if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) return error(_("unable to write new index file")); return 0; From 350292a1efb38bbcd6255a424df6adbfe78910ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Wed, 28 Feb 2018 20:07:58 +0100 Subject: [PATCH 5/5] sequencer: do not roll back lockfile unnecessarily MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If `commit_lock_file()` or `hold_lock_file_for_update()` fail, there is no need to call `rollback_lock_file()` on the lockfile. It doesn't hurt either, but it does make different callers in this file inconsistent, which might be confusing. While at it, remove a trailing '.' from a recurring error message. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- sequencer.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/sequencer.c b/sequencer.c index bca6922823..548ad997b5 100644 --- a/sequencer.c +++ b/sequencer.c @@ -352,10 +352,8 @@ static int write_message(const void *buf, size_t len, const char *filename, rollback_lock_file(&msg_file); return error_errno(_("could not write eol to '%s'"), filename); } - if (commit_lock_file(&msg_file) < 0) { - rollback_lock_file(&msg_file); - return error(_("failed to finalize '%s'."), filename); - } + if (commit_lock_file(&msg_file) < 0) + return error(_("failed to finalize '%s'"), filename); return 0; } @@ -2107,10 +2105,8 @@ static int save_head(const char *head) ssize_t written; fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0); - if (fd < 0) { - rollback_lock_file(&head_lock); + if (fd < 0) return error_errno(_("could not lock HEAD")); - } strbuf_addf(&buf, "%s\n", head); written = write_in_full(fd, buf.buf, buf.len); strbuf_release(&buf); @@ -2119,10 +2115,8 @@ static int save_head(const char *head) return error_errno(_("could not write to '%s'"), git_path_head_file()); } - if (commit_lock_file(&head_lock) < 0) { - rollback_lock_file(&head_lock); - return error(_("failed to finalize '%s'."), git_path_head_file()); - } + if (commit_lock_file(&head_lock) < 0) + return error(_("failed to finalize '%s'"), git_path_head_file()); return 0; } @@ -2246,7 +2240,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts) todo_list->buf.len - offset) < 0) return error_errno(_("could not write to '%s'"), todo_path); if (commit_lock_file(&todo_lock) < 0) - return error(_("failed to finalize '%s'."), todo_path); + return error(_("failed to finalize '%s'"), todo_path); if (is_rebase_i(opts)) { const char *done_path = rebase_path_done();