From b9b946d4a4e9e30bc0dba9ce3ff81ab3d3666632 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 26 Aug 2016 15:47:10 +0200 Subject: sequencer: lib'ify sequencer_pick_revisions() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The function sequencer_pick_revisions() has only two callers, cmd_revert() and cmd_cherry_pick(), both of which check the return value and react appropriately upon errors. So this is a safe conversion to make sequencer_pick_revisions() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/sequencer.c b/sequencer.c index 3804fa931d..76b1c52f75 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1063,10 +1063,11 @@ int sequencer_pick_revisions(struct replay_opts *opts) if (!get_sha1(name, sha1)) { if (!lookup_commit_reference_gently(sha1, 1)) { enum object_type type = sha1_object_info(sha1, NULL); - die(_("%s: can't cherry-pick a %s"), name, typename(type)); + return error(_("%s: can't cherry-pick a %s"), + name, typename(type)); } } else - die(_("%s: bad revision"), name); + return error(_("%s: bad revision"), name); } /* @@ -1082,10 +1083,10 @@ int sequencer_pick_revisions(struct replay_opts *opts) !opts->revs->cmdline.rev->flags) { struct commit *cmit; if (prepare_revision_walk(opts->revs)) - die(_("revision walk setup failed")); + return error(_("revision walk setup failed")); cmit = get_revision(opts->revs); if (!cmit || get_revision(opts->revs)) - die("BUG: expected exactly one commit from walk"); + return error("BUG: expected exactly one commit from walk"); return single_pick(cmit, opts); } -- cgit v1.2.3 From dbfad033d443f27c5579bbb67bd14243b0cbaba6 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 26 Aug 2016 15:47:14 +0200 Subject: sequencer: do not die() in do_pick_commit() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The eventual caller of do_pick_commit() is sequencer_pick_revisions(), which already relays a reported error from its helper functions (including this one), and both of its two callers know how to react to a negative return correctly. So this makes do_pick_commit() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/sequencer.c b/sequencer.c index 76b1c52f75..baf6b40e7a 100644 --- a/sequencer.c +++ b/sequencer.c @@ -585,12 +585,14 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) * However, if the merge did not even start, then we don't want to * write it at all. */ - if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1)) - update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.oid.hash, NULL, - REF_NODEREF, UPDATE_REFS_DIE_ON_ERR); - if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1)) - update_ref(NULL, "REVERT_HEAD", commit->object.oid.hash, NULL, - REF_NODEREF, UPDATE_REFS_DIE_ON_ERR); + if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1) && + update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.oid.hash, NULL, + REF_NODEREF, UPDATE_REFS_MSG_ON_ERR)) + res = -1; + if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1) && + update_ref(NULL, "REVERT_HEAD", commit->object.oid.hash, NULL, + REF_NODEREF, UPDATE_REFS_MSG_ON_ERR)) + res = -1; if (res) { error(opts->action == REPLAY_REVERT -- cgit v1.2.3 From 4ef3d8f0336d4d1d5a57de9ac7c0cfdb09b974b1 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:05 +0200 Subject: sequencer: lib'ify write_message() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of write_message(), do_pick_commit() already checks the return value and passes it on to its callers, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make write_message() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/sequencer.c b/sequencer.c index baf6b40e7a..ec85fe77b4 100644 --- a/sequencer.c +++ b/sequencer.c @@ -180,17 +180,20 @@ static void print_advice(int show_hint, struct replay_opts *opts) } } -static void write_message(struct strbuf *msgbuf, const char *filename) +static int write_message(struct strbuf *msgbuf, const char *filename) { static struct lock_file msg_file; - int msg_fd = hold_lock_file_for_update(&msg_file, filename, - LOCK_DIE_ON_ERROR); + int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0); + if (msg_fd < 0) + return error_errno(_("Could not lock '%s'"), filename); if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0) - die_errno(_("Could not write to %s"), filename); + return error_errno(_("Could not write to %s"), filename); strbuf_release(msgbuf); if (commit_lock_file(&msg_file) < 0) - die(_("Error wrapping up %s."), filename); + return error(_("Error wrapping up %s."), filename); + + return 0; } static struct tree *empty_tree(void) @@ -564,16 +567,16 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) head, &msgbuf, opts); if (res < 0) return res; - write_message(&msgbuf, git_path_merge_msg()); + res |= write_message(&msgbuf, git_path_merge_msg()); } else { struct commit_list *common = NULL; struct commit_list *remotes = NULL; - write_message(&msgbuf, git_path_merge_msg()); + res = write_message(&msgbuf, git_path_merge_msg()); commit_list_insert(base, &common); commit_list_insert(next, &remotes); - res = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts, + res |= try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts, common, sha1_to_hex(head), remotes); free_commit_list(common); free_commit_list(remotes); -- cgit v1.2.3 From c527b55e74181291da9bf9d6ca4cd2f7133f934b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:10 +0200 Subject: sequencer: lib'ify do_recursive_merge() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of do_recursive_merge(), do_pick_commit() already checks the return value and passes it on to its callers, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make do_recursive_merge() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index ec85fe77b4..eb700913a1 100644 --- a/sequencer.c +++ b/sequencer.c @@ -303,7 +303,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next, if (active_cache_changed && write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) /* TRANSLATORS: %s will be "revert" or "cherry-pick" */ - die(_("%s: Unable to write new index file"), action_name(opts)); + return error(_("%s: Unable to write new index file"), + action_name(opts)); rollback_lock_file(&index_lock); if (opts->signoff) -- cgit v1.2.3 From f74087f6ed21e65a926cb4facfa39d142eeffa96 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:12 +0200 Subject: sequencer: lib'ify do_pick_commit() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only two callers of do_pick_commit(), pick_commits() and single_pick() already check the return value and pass it on to their callers, so their callers must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make do_pick_commit() callable from new callers that want it not to die, without changing the external behaviour of anything existing. While at it, remove the superfluous space. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index eb700913a1..96b9ae1978 100644 --- a/sequencer.c +++ b/sequencer.c @@ -464,7 +464,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) * to work on. */ if (write_cache_as_tree(head, 0, NULL)) - die (_("Your index file is unmerged.")); + return error(_("Your index file is unmerged.")); } else { unborn = get_sha1("HEAD", head); if (unborn) -- cgit v1.2.3 From 34b0528b735cd22e5600576c10904dff262b8332 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:15 +0200 Subject: sequencer: lib'ify walk_revs_populate_todo() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The function sequencer_pick_revisions() is the only caller of walk_revs_populate_todo(), and it already returns errors appropriately, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make walk_revs_populate_todo() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/sequencer.c b/sequencer.c index 96b9ae1978..ab599e04df 100644 --- a/sequencer.c +++ b/sequencer.c @@ -809,17 +809,19 @@ static void read_populate_opts(struct replay_opts **opts_ptr) die(_("Malformed options sheet: %s"), git_path_opts_file()); } -static void walk_revs_populate_todo(struct commit_list **todo_list, +static int walk_revs_populate_todo(struct commit_list **todo_list, struct replay_opts *opts) { struct commit *commit; struct commit_list **next; - prepare_revs(opts); + if (prepare_revs(opts)) + return -1; next = todo_list; while ((commit = get_revision(opts->revs))) next = commit_list_append(commit, next); + return 0; } static int create_seq_dir(void) @@ -1102,8 +1104,8 @@ int sequencer_pick_revisions(struct replay_opts *opts) * progress */ - walk_revs_populate_todo(&todo_list, opts); - if (create_seq_dir() < 0) + if (walk_revs_populate_todo(&todo_list, opts) || + create_seq_dir() < 0) return -1; if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT)) return error(_("Can't revert as initial commit")); -- cgit v1.2.3 From c3e8618c1f26f9b6b658d02a09530ae8eb85fc04 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:18 +0200 Subject: sequencer: lib'ify prepare_revs() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of prepare_revs(), walk_revs_populate_todo() was just taught to return errors, after verifying that its callers are prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make prepare_revs() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/sequencer.c b/sequencer.c index ab599e04df..7fd0f99650 100644 --- a/sequencer.c +++ b/sequencer.c @@ -623,7 +623,7 @@ leave: return res; } -static void prepare_revs(struct replay_opts *opts) +static int prepare_revs(struct replay_opts *opts) { /* * picking (but not reverting) ranges (but not individual revisions) @@ -633,10 +633,11 @@ static void prepare_revs(struct replay_opts *opts) opts->revs->reverse ^= 1; if (prepare_revision_walk(opts->revs)) - die(_("revision walk setup failed")); + return error(_("revision walk setup failed")); if (!opts->revs->commits) - die(_("empty commit set passed")); + return error(_("empty commit set passed")); + return 0; } static void read_and_refresh_cache(struct replay_opts *opts) -- cgit v1.2.3 From 0d9c6dc9ec316c5b117ee53b44d346349bd8b818 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:21 +0200 Subject: sequencer: lib'ify read_and_refresh_cache() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). There are two call sites of read_and_refresh_cache(), one of which is pick_commits(), whose callers were already prepared to do the right thing given an "error" return from it by an earlier patch, so the conversion is safe. The other one, sequencer_pick_revisions() was also prepared to relay an error return back to its caller in all remaining cases in an earlier patch. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/sequencer.c b/sequencer.c index 7fd0f99650..631b75dabe 100644 --- a/sequencer.c +++ b/sequencer.c @@ -640,18 +640,21 @@ static int prepare_revs(struct replay_opts *opts) return 0; } -static void read_and_refresh_cache(struct replay_opts *opts) +static int read_and_refresh_cache(struct replay_opts *opts) { static struct lock_file index_lock; int index_fd = hold_locked_index(&index_lock, 0); if (read_index_preload(&the_index, NULL) < 0) - die(_("git %s: failed to read the index"), action_name(opts)); + return error(_("git %s: failed to read the index"), + action_name(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)) - die(_("git %s: failed to refresh the index"), action_name(opts)); + return error(_("git %s: failed to refresh the index"), + action_name(opts)); } rollback_lock_file(&index_lock); + return 0; } static int format_todo(struct strbuf *buf, struct commit_list *todo_list, @@ -981,7 +984,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts) if (opts->allow_ff) assert(!(opts->signoff || opts->no_commit || opts->record_origin || opts->edit)); - read_and_refresh_cache(opts); + if (read_and_refresh_cache(opts)) + return -1; for (cur = todo_list; cur; cur = cur->next) { save_todo(cur, opts); @@ -1045,7 +1049,8 @@ int sequencer_pick_revisions(struct replay_opts *opts) if (opts->subcommand == REPLAY_NONE) assert(opts->revs); - read_and_refresh_cache(opts); + if (read_and_refresh_cache(opts)) + return -1; /* * Decide what to do depending on the arguments; a fresh -- cgit v1.2.3 From 0ae42a038da1f63a5b090b70502087817cc54f5d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:24 +0200 Subject: sequencer: lib'ify read_populate_todo() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of read_populate_todo(), sequencer_continue() can already return errors, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make read_populate_todo() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/sequencer.c b/sequencer.c index 631b75dabe..c73cdfdac1 100644 --- a/sequencer.c +++ b/sequencer.c @@ -748,7 +748,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list, return 0; } -static void read_populate_todo(struct commit_list **todo_list, +static int read_populate_todo(struct commit_list **todo_list, struct replay_opts *opts) { struct strbuf buf = STRBUF_INIT; @@ -756,18 +756,21 @@ static void read_populate_todo(struct commit_list **todo_list, fd = open(git_path_todo_file(), O_RDONLY); if (fd < 0) - die_errno(_("Could not open %s"), git_path_todo_file()); + return error_errno(_("Could not open %s"), + git_path_todo_file()); if (strbuf_read(&buf, fd, 0) < 0) { close(fd); strbuf_release(&buf); - die(_("Could not read %s."), git_path_todo_file()); + return error(_("Could not read %s."), git_path_todo_file()); } close(fd); res = parse_insn_buffer(buf.buf, todo_list, opts); strbuf_release(&buf); if (res) - die(_("Unusable instruction sheet: %s"), git_path_todo_file()); + return error(_("Unusable instruction sheet: %s"), + git_path_todo_file()); + return 0; } static int populate_opts_cb(const char *key, const char *value, void *data) @@ -1019,7 +1022,8 @@ static int sequencer_continue(struct replay_opts *opts) if (!file_exists(git_path_todo_file())) return continue_single_pick(); read_populate_opts(&opts); - read_populate_todo(&todo_list, opts); + if (read_populate_todo(&todo_list, opts)) + return -1; /* Verify that the conflict has been resolved */ if (file_exists(git_path_cherry_pick_head()) || -- cgit v1.2.3 From 0d00da7bb3780c29e3d42ee255e0a09de8c43587 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:27 +0200 Subject: sequencer: lib'ify read_populate_opts() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of read_populate_opts(), sequencer_continue() can already return errors, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make read_populate_opts() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Note that the function git_config_from_file(), called from read_populate_opts(), can currently still die() (in git_parse_source(), because the do_config_from_file() function sets die_on_error = 1). We do not try to fix that here, as it would have larger ramifications on the config code, and we also assume that we write the opts file programmatically, hence any parse errors would be bugs. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/sequencer.c b/sequencer.c index c73cdfdac1..1614efb8db 100644 --- a/sequencer.c +++ b/sequencer.c @@ -808,12 +808,20 @@ static int populate_opts_cb(const char *key, const char *value, void *data) return 0; } -static void read_populate_opts(struct replay_opts **opts_ptr) +static int read_populate_opts(struct replay_opts **opts) { if (!file_exists(git_path_opts_file())) - return; - if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0) - die(_("Malformed options sheet: %s"), git_path_opts_file()); + return 0; + /* + * The function git_parse_source(), called from git_config_from_file(), + * may die() in case of a syntactically incorrect file. We do not care + * about this case, though, because we wrote that file ourselves, so we + * are pretty certain that it is syntactically correct. + */ + if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0) + return error(_("Malformed options sheet: %s"), + git_path_opts_file()); + return 0; } static int walk_revs_populate_todo(struct commit_list **todo_list, @@ -1021,8 +1029,8 @@ static int sequencer_continue(struct replay_opts *opts) if (!file_exists(git_path_todo_file())) return continue_single_pick(); - read_populate_opts(&opts); - if (read_populate_todo(&todo_list, opts)) + if (read_populate_opts(&opts) || + read_populate_todo(&todo_list, opts)) return -1; /* Verify that the conflict has been resolved */ -- cgit v1.2.3 From f6e82b0d91251ac207da94952ee129642bd2d2b9 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:44 +0200 Subject: sequencer: lib'ify create_seq_dir() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of create_seq_dir(), sequencer_pick_revisions() can already return errors, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make create_seq_dir() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sequencer.c b/sequencer.c index 1614efb8db..eb9c473328 100644 --- a/sequencer.c +++ b/sequencer.c @@ -847,8 +847,8 @@ static int create_seq_dir(void) return -1; } else if (mkdir(git_path_seq_dir(), 0777) < 0) - die_errno(_("Could not create sequencer directory %s"), - git_path_seq_dir()); + return error_errno(_("Could not create sequencer directory %s"), + git_path_seq_dir()); return 0; } -- cgit v1.2.3 From 311fd397f0e750333e0aaa8e45b9284a5227e353 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:47 +0200 Subject: sequencer: lib'ify save_head() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of save_head(), sequencer_pick_revisions() can already return errors, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make save_head() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/sequencer.c b/sequencer.c index eb9c473328..7a1561e6a0 100644 --- a/sequencer.c +++ b/sequencer.c @@ -852,18 +852,28 @@ static int create_seq_dir(void) return 0; } -static void save_head(const char *head) +static int save_head(const char *head) { static struct lock_file head_lock; struct strbuf buf = STRBUF_INIT; int fd; - fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), LOCK_DIE_ON_ERROR); + fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0); + if (fd < 0) { + rollback_lock_file(&head_lock); + return error_errno(_("Could not lock HEAD")); + } strbuf_addf(&buf, "%s\n", head); - if (write_in_full(fd, buf.buf, buf.len) < 0) - die_errno(_("Could not write to %s"), git_path_head_file()); - if (commit_lock_file(&head_lock) < 0) - die(_("Error wrapping up %s."), git_path_head_file()); + if (write_in_full(fd, buf.buf, buf.len) < 0) { + rollback_lock_file(&head_lock); + 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(_("Error wrapping up %s."), git_path_head_file()); + } + return 0; } static int reset_for_rollback(const unsigned char *sha1) @@ -1127,7 +1137,8 @@ int sequencer_pick_revisions(struct replay_opts *opts) return -1; if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT)) return error(_("Can't revert as initial commit")); - save_head(sha1_to_hex(sha1)); + if (save_head(sha1_to_hex(sha1))) + return -1; save_opts(opts); return pick_commits(todo_list, opts); } -- cgit v1.2.3 From 221675de8d7ef363944161d8d887fb1b7afdf915 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:50 +0200 Subject: sequencer: lib'ify save_todo() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of save_todo(), pick_commits() can already return errors, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make save_todo() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/sequencer.c b/sequencer.c index 7a1561e6a0..32c53bb2b7 100644 --- a/sequencer.c +++ b/sequencer.c @@ -943,24 +943,31 @@ fail: return -1; } -static void save_todo(struct commit_list *todo_list, struct replay_opts *opts) +static int save_todo(struct commit_list *todo_list, struct replay_opts *opts) { static struct lock_file todo_lock; struct strbuf buf = STRBUF_INIT; int fd; - fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), LOCK_DIE_ON_ERROR); - if (format_todo(&buf, todo_list, opts) < 0) - die(_("Could not format %s."), git_path_todo_file()); + fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), 0); + if (fd < 0) + return error_errno(_("Could not lock '%s'"), + git_path_todo_file()); + if (format_todo(&buf, todo_list, opts) < 0) { + strbuf_release(&buf); + return error(_("Could not format %s."), git_path_todo_file()); + } if (write_in_full(fd, buf.buf, buf.len) < 0) { strbuf_release(&buf); - die_errno(_("Could not write to %s"), git_path_todo_file()); + return error_errno(_("Could not write to %s"), + git_path_todo_file()); } if (commit_lock_file(&todo_lock) < 0) { strbuf_release(&buf); - die(_("Error wrapping up %s."), git_path_todo_file()); + return error(_("Error wrapping up %s."), git_path_todo_file()); } strbuf_release(&buf); + return 0; } static void save_opts(struct replay_opts *opts) @@ -1009,7 +1016,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts) return -1; for (cur = todo_list; cur; cur = cur->next) { - save_todo(cur, opts); + if (save_todo(cur, opts)) + return -1; res = do_pick_commit(cur->item, opts); if (res) return res; -- cgit v1.2.3 From 88d5a271b01cbbdcfe558a6bf98b601911c33c03 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:53 +0200 Subject: sequencer: lib'ify save_opts() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of save_opts(), sequencer_pick_revisions() can already return errors, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make save_opts() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/sequencer.c b/sequencer.c index 32c53bb2b7..021ddf36d0 100644 --- a/sequencer.c +++ b/sequencer.c @@ -970,37 +970,39 @@ static int save_todo(struct commit_list *todo_list, struct replay_opts *opts) return 0; } -static void save_opts(struct replay_opts *opts) +static int save_opts(struct replay_opts *opts) { const char *opts_file = git_path_opts_file(); + int res = 0; if (opts->no_commit) - git_config_set_in_file(opts_file, "options.no-commit", "true"); + res |= git_config_set_in_file_gently(opts_file, "options.no-commit", "true"); if (opts->edit) - git_config_set_in_file(opts_file, "options.edit", "true"); + res |= git_config_set_in_file_gently(opts_file, "options.edit", "true"); if (opts->signoff) - git_config_set_in_file(opts_file, "options.signoff", "true"); + res |= git_config_set_in_file_gently(opts_file, "options.signoff", "true"); if (opts->record_origin) - git_config_set_in_file(opts_file, "options.record-origin", "true"); + res |= git_config_set_in_file_gently(opts_file, "options.record-origin", "true"); if (opts->allow_ff) - git_config_set_in_file(opts_file, "options.allow-ff", "true"); + res |= git_config_set_in_file_gently(opts_file, "options.allow-ff", "true"); if (opts->mainline) { struct strbuf buf = STRBUF_INIT; strbuf_addf(&buf, "%d", opts->mainline); - git_config_set_in_file(opts_file, "options.mainline", buf.buf); + res |= git_config_set_in_file_gently(opts_file, "options.mainline", buf.buf); strbuf_release(&buf); } if (opts->strategy) - git_config_set_in_file(opts_file, "options.strategy", opts->strategy); + res |= git_config_set_in_file_gently(opts_file, "options.strategy", opts->strategy); if (opts->gpg_sign) - git_config_set_in_file(opts_file, "options.gpg-sign", opts->gpg_sign); + res |= git_config_set_in_file_gently(opts_file, "options.gpg-sign", opts->gpg_sign); if (opts->xopts) { int i; for (i = 0; i < opts->xopts_nr; i++) - git_config_set_multivar_in_file(opts_file, + res |= git_config_set_multivar_in_file_gently(opts_file, "options.strategy-option", opts->xopts[i], "^$", 0); } + return res; } static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts) @@ -1147,7 +1149,8 @@ int sequencer_pick_revisions(struct replay_opts *opts) return error(_("Can't revert as initial commit")); if (save_head(sha1_to_hex(sha1))) return -1; - save_opts(opts); + if (save_opts(opts)) + return -1; return pick_commits(todo_list, opts); } -- cgit v1.2.3 From 0e408fc347088aa54f97cada3b880afd2560cc94 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:55 +0200 Subject: sequencer: lib'ify fast_forward_to() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of fast_forward_to(), do_pick_commit() already checks the return value and passes it on to its callers, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make fast_forward_to() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 021ddf36d0..d92a632f29 100644 --- a/sequencer.c +++ b/sequencer.c @@ -226,7 +226,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from, read_cache(); if (checkout_fast_forward(from, to, 1)) - exit(128); /* the callee should have complained already */ + return -1; /* the callee should have complained already */ strbuf_addf(&sb, _("%s: fast-forward"), action_name(opts)); -- cgit v1.2.3 From 55f5704da69d3e6836620f01bee0093ad5e331e8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:38:00 +0200 Subject: sequencer: lib'ify checkout_fast_forward() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only callers of checkout_fast_forward(), cmd_merge(), pull_into_void(), cmd_pull() and sequencer's fast_forward_to(), already check the return value and handle it appropriately. With this step, we make it notice an error return from this function. So this is a safe conversion to make checkout_fast_forward() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- merge.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/merge.c b/merge.c index 5db7d56b90..23866c9165 100644 --- a/merge.c +++ b/merge.c @@ -57,7 +57,8 @@ int checkout_fast_forward(const unsigned char *head, refresh_cache(REFRESH_QUIET); - hold_locked_index(lock_file, 1); + if (hold_locked_index(lock_file, 0) < 0) + return -1; memset(&trees, 0, sizeof(trees)); memset(&opts, 0, sizeof(opts)); @@ -90,7 +91,9 @@ int checkout_fast_forward(const unsigned char *head, } if (unpack_trees(nr_trees, t, &opts)) return -1; - if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) - die(_("unable to write new index 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; } -- cgit v1.2.3 From 49fb937e9a082b3bdecd85f1c539a1e1683e1a3d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:38:20 +0200 Subject: sequencer: ensure to release the lock when we could not read the index A future caller of read_and_refresh_cache() may want to do more than just print some helpful advice in case of failure. Suggested by Junio Hamano. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/sequencer.c b/sequencer.c index d92a632f29..eec8a60d6b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -644,14 +644,18 @@ static int read_and_refresh_cache(struct replay_opts *opts) { static struct lock_file index_lock; int index_fd = hold_locked_index(&index_lock, 0); - if (read_index_preload(&the_index, NULL) < 0) + if (read_index_preload(&the_index, NULL) < 0) { + rollback_lock_file(&index_lock); return error(_("git %s: failed to read the index"), action_name(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)) + 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)); + } } rollback_lock_file(&index_lock); return 0; -- cgit v1.2.3