{"thread":{"id":"43912","subject":"[PATCH 07/15] sequencer: lib'ify read_populate_opts()","startedAt":"2016-08-23T16:07:47Z","lastAt":"2016-09-11T08:10:27Z","messageCount":91,"participants":["Johannes Schindelin","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":15},"messages":[{"id":"299937","messageId":"85a8ec8273994c599402c380abd383ad2f539777.1471968378.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1471968378.git.johannes.schindelin@gmx.de","subject":"[PATCH 07/15] sequencer: lib'ify read_populate_opts()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-23T16:07:15Z","receivedAt":"2016-08-23T16:07:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"To be truly useful, the sequencer should never die() but always return\nan error.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 5f6b020..808cd53 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -806,12 +806,14 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n \treturn 0;\n }\n \n-static void read_populate_opts(struct replay_opts **opts_ptr)\n+static int read_populate_opts(struct replay_opts **opts)\n {\n \tif (!file_exists(git_path_opts_file()))\n-\t\treturn;\n-\tif (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0)\n-\t\tdie(_(\"Malformed options sheet: %s\"), git_path_opts_file());\n+\t\treturn 0;\n+\tif (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)\n+\t\treturn error(_(\"Malformed options sheet: %s\"),\n+\t\t\tgit_path_opts_file());\n+\treturn 0;\n }\n \n static void walk_revs_populate_todo(struct commit_list **todo_list,\n@@ -1017,8 +1019,8 @@ static int sequencer_continue(struct replay_opts *opts)\n \n \tif (!file_exists(git_path_todo_file()))\n \t\treturn continue_single_pick();\n-\tread_populate_opts(&opts);\n-\tif (read_populate_todo(&todo_list, opts))\n+\tif (read_populate_opts(&opts) ||\n+\t\t\tread_populate_todo(&todo_list, opts))\n \t\treturn -1;\n \n \t/* Verify that the conflict has been resolved */\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"299938","messageId":"6dc5e927cbdd1847db5b6cd75041609dd1e1f76d.1471968378.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1471968378.git.johannes.schindelin@gmx.de","subject":"[PATCH 01/15] sequencer: lib'ify write_message()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-23T16:06:36Z","receivedAt":"2016-08-23T16:07:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"To be truly useful, the sequencer should never die() but always return\nan error.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 2e9c7d0..c75296c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -180,17 +180,20 @@ static void print_advice(int show_hint, struct replay_opts *opts)\n \t}\n }\n \n-static void write_message(struct strbuf *msgbuf, const char *filename)\n+static int write_message(struct strbuf *msgbuf, const char *filename)\n {\n \tstatic struct lock_file msg_file;\n \n-\tint msg_fd = hold_lock_file_for_update(&msg_file, filename,\n-\t\t\t\t\t LOCK_DIE_ON_ERROR);\n+\tint msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);\n+\tif (msg_fd < 0)\n+\t\treturn error_errno(_(\"Could not lock '%s'\"), filename);\n \tif (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)\n-\t\tdie_errno(_(\"Could not write to %s\"), filename);\n+\t\treturn error_errno(_(\"Could not write to %s\"), filename);\n \tstrbuf_release(msgbuf);\n \tif (commit_lock_file(&msg_file) < 0)\n-\t\tdie(_(\"Error wrapping up %s.\"), filename);\n+\t\treturn error(_(\"Error wrapping up %s.\"), filename);\n+\n+\treturn 0;\n }\n \n static struct tree *empty_tree(void)\n@@ -564,16 +567,16 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t\t\t\t head, &msgbuf, opts);\n \t\tif (res < 0)\n \t\t\treturn res;\n-\t\twrite_message(&msgbuf, git_path_merge_msg());\n+\t\tres |= write_message(&msgbuf, git_path_merge_msg());\n \t} else {\n \t\tstruct commit_list *common = NULL;\n \t\tstruct commit_list *remotes = NULL;\n \n-\t\twrite_message(&msgbuf, git_path_merge_msg());\n+\t\tres = write_message(&msgbuf, git_path_merge_msg());\n \n \t\tcommit_list_insert(base, &common);\n \t\tcommit_list_insert(next, &remotes);\n-\t\tres = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,\n+\t\tres |= try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,\n \t\t\t\t\tcommon, sha1_to_hex(head), remotes);\n \t\tfree_commit_list(common);\n \t\tfree_commit_list(remotes);\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"299939","messageId":"0de75bbce8ade0c6e5cf87d3647faa71a89c6275.1471968378.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1471968378.git.johannes.schindelin@gmx.de","subject":"[PATCH 06/15] sequencer: lib'ify read_populate_todo()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-23T16:07:11Z","receivedAt":"2016-08-23T16:07:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"To be truly useful, the sequencer should never die() but always return\nan error.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex a8c3a48..5f6b020 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -746,7 +746,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,\n \treturn 0;\n }\n \n-static void read_populate_todo(struct commit_list **todo_list,\n+static int read_populate_todo(struct commit_list **todo_list,\n \t\t\tstruct replay_opts *opts)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -754,18 +754,21 @@ static void read_populate_todo(struct commit_list **todo_list,\n \n \tfd = open(git_path_todo_file(), O_RDONLY);\n \tif (fd < 0)\n-\t\tdie_errno(_(\"Could not open %s\"), git_path_todo_file());\n+\t\treturn error(_(\"Could not open %s (%s)\"),\n+\t\t\tgit_path_todo_file(), strerror(errno));\n \tif (strbuf_read(&buf, fd, 0) < 0) {\n \t\tclose(fd);\n \t\tstrbuf_release(&buf);\n-\t\tdie(_(\"Could not read %s.\"), git_path_todo_file());\n+\t\treturn error(_(\"Could not read %s.\"), git_path_todo_file());\n \t}\n \tclose(fd);\n \n \tres = parse_insn_buffer(buf.buf, todo_list, opts);\n \tstrbuf_release(&buf);\n \tif (res)\n-\t\tdie(_(\"Unusable instruction sheet: %s\"), git_path_todo_file());\n+\t\treturn error(_(\"Unusable instruction sheet: %s\"),\n+\t\t\tgit_path_todo_file());\n+\treturn 0;\n }\n \n static int populate_opts_cb(const char *key, const char *value, void *data)\n@@ -1015,7 +1018,8 @@ static int sequencer_continue(struct replay_opts *opts)\n \tif (!file_exists(git_path_todo_file()))\n \t\treturn continue_single_pick();\n \tread_populate_opts(&opts);\n-\tread_populate_todo(&todo_list, opts);\n+\tif (read_populate_todo(&todo_list, opts))\n+\t\treturn -1;\n \n \t/* Verify that the conflict has been resolved */\n \tif (file_exists(git_path_cherry_pick_head()) ||\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"299940","messageId":"ce6a7cac9f0bd53e44129c8f72818adc4bb4a934.1471968378.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1471968378.git.johannes.schindelin@gmx.de","subject":"[PATCH 04/15] sequencer: lib'ify prepare_revs()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-23T16:06:50Z","receivedAt":"2016-08-23T16:08:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"To be truly useful, the sequencer should never die() but always return\nan error.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 6ac2187..b90294f 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -621,7 +621,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \treturn res;\n }\n \n-static void prepare_revs(struct replay_opts *opts)\n+static int prepare_revs(struct replay_opts *opts)\n {\n \t/*\n \t * picking (but not reverting) ranges (but not individual revisions)\n@@ -631,10 +631,11 @@ static void prepare_revs(struct replay_opts *opts)\n \t\topts->revs->reverse ^= 1;\n \n \tif (prepare_revision_walk(opts->revs))\n-\t\tdie(_(\"revision walk setup failed\"));\n+\t\treturn error(_(\"revision walk setup failed\"));\n \n \tif (!opts->revs->commits)\n-\t\tdie(_(\"empty commit set passed\"));\n+\t\treturn error(_(\"empty commit set passed\"));\n+\treturn 0;\n }\n \n static void read_and_refresh_cache(struct replay_opts *opts)\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"299941","messageId":"e00df1449af0d8c55000a93c734d8a241b1cb5f0.1471968378.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1471968378.git.johannes.schindelin@gmx.de","subject":"[PATCH 02/15] sequencer: lib'ify do_recursive_merge()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-23T16:06:41Z","receivedAt":"2016-08-23T16:08:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"To be truly useful, the sequencer should never die() but always return\nan error.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex c75296c..0c8c955 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -303,7 +303,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \tif (active_cache_changed &&\n \t write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n \t\t/* TRANSLATORS: %s will be \"revert\" or \"cherry-pick\" */\n-\t\tdie(_(\"%s: Unable to write new index file\"), action_name(opts));\n+\t\treturn error(_(\"%s: Unable to write new index file\"),\n+\t\t\taction_name(opts));\n \trollback_lock_file(&index_lock);\n \n \tif (opts->signoff)\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"299942","messageId":"cover.1471968378.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":null,"subject":"[PATCH 00/15] Lib'ify quite a few functions in sequencer.c","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-23T16:06:32Z","receivedAt":"2016-08-23T16:08:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series is one of the half dozen patch series left to move the\nbulk of rebase -i into a builtin.\n\nThe purpose of this patch series is to switch the functions in\nsequencer.c from die()ing to returning errors instead, as proper library\nfunctions should do, to give callers a chance to clean up after an\nerror.\n\n\nJohannes Schindelin (15):\n sequencer: lib'ify write_message()\n sequencer: lib'ify do_recursive_merge()\n sequencer: lib'ify do_pick_commit()\n sequencer: lib'ify prepare_revs()\n sequencer: lib'ify read_and_refresh_cache()\n sequencer: lib'ify read_populate_todo()\n sequencer: lib'ify read_populate_opts()\n sequencer: lib'ify walk_revs_populate_todo()\n sequencer: lib'ify create_seq_dir()\n sequencer: lib'ify save_head()\n sequencer: lib'ify save_todo()\n sequencer: lib'ify save_opts()\n sequencer: lib'ify sequencer_pick_revisions()\n sequencer: do not die() in do_pick_commit()\n sequencer: do not die() in fast_forward_to()\n\n sequencer.c | 168 ++++++++++++++++++++++++++++++++++++------------------------\n 1 file changed, 101 insertions(+), 67 deletions(-)\n\nPublished-As: https://github.com/dscho/git/releases/tag/libify-sequencer-v1\nFetch-It-Via: git fetch https://github.com/dscho/git libify-sequencer-v1\n\n-- \n2.10.0.rc1.99.gcd66998\n\nbase-commit: 2632c897f74b1cc9b5533f467da459b9ec725538\n"},{"id":"299943","messageId":"92fdb70f3db157f8a3d2442d872e989d81c6756e.1471968378.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1471968378.git.johannes.schindelin@gmx.de","subject":"[PATCH 03/15] sequencer: lib'ify do_pick_commit()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-23T16:06:46Z","receivedAt":"2016-08-23T16:08:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"To be truly useful, the sequencer should never die() but always return\nan error.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 0c8c955..6ac2187 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -464,7 +464,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t * to work on.\n \t\t */\n \t\tif (write_cache_as_tree(head, 0, NULL))\n-\t\t\tdie (_(\"Your index file is unmerged.\"));\n+\t\t\treturn error (_(\"Your index file is unmerged.\"));\n \t} else {\n \t\tunborn = get_sha1(\"HEAD\", head);\n \t\tif (unborn)\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"299944","messageId":"f9a888ca68915f5e407331c3bea94e2b09651412.1471968378.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1471968378.git.johannes.schindelin@gmx.de","subject":"[PATCH 13/15] sequencer: lib'ify sequencer_pick_revisions()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-23T16:07:36Z","receivedAt":"2016-08-23T16:09:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"To be truly useful, the sequencer should never die() but always return\nan error.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex bac32ea..324463f 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1093,10 +1093,11 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t\tif (!get_sha1(name, sha1)) {\n \t\t\tif (!lookup_commit_reference_gently(sha1, 1)) {\n \t\t\t\tenum object_type type = sha1_object_info(sha1, NULL);\n-\t\t\t\tdie(_(\"%s: can't cherry-pick a %s\"), name, typename(type));\n+\t\t\t\treturn error(_(\"%s: can't cherry-pick a %s\"),\n+\t\t\t\t\tname, typename(type));\n \t\t\t}\n \t\t} else\n-\t\t\tdie(_(\"%s: bad revision\"), name);\n+\t\t\treturn error(_(\"%s: bad revision\"), name);\n \t}\n \n \t/*\n@@ -1112,10 +1113,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t !opts->revs->cmdline.rev->flags) {\n \t\tstruct commit *cmit;\n \t\tif (prepare_revision_walk(opts->revs))\n-\t\t\tdie(_(\"revision walk setup failed\"));\n+\t\t\treturn error(_(\"revision walk setup failed\"));\n \t\tcmit = get_revision(opts->revs);\n \t\tif (!cmit || get_revision(opts->revs))\n-\t\t\tdie(\"BUG: expected exactly one commit from walk\");\n+\t\t\treturn error(\"BUG: expected exactly one commit from walk\");\n \t\treturn single_pick(cmit, opts);\n \t}\n \n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"299945","messageId":"ba172fe9ce41b2430b9f123aba8019e0dbb634dd.1471968378.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1471968378.git.johannes.schindelin@gmx.de","subject":"[PATCH 15/15] sequencer: do not die() in fast_forward_to()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-23T16:07:57Z","receivedAt":"2016-08-23T16:09:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"A fast-forward may fail e.g. when it would overwrite an untracked\nfile. We still must not exit() in that case: the sequencer is\nsupposedly providing a library function.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex c29de64..9ddd054 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -226,7 +226,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \n \tread_cache();\n \tif (checkout_fast_forward(from, to, 1))\n-\t\texit(128); /* the callee should have complained already */\n+\t\treturn -1; /* the callee should have complained already */\n \n \tstrbuf_addf(&sb, _(\"%s: fast-forward\"), action_name(opts));\n \n-- \n2.10.0.rc1.99.gcd66998\n"},{"id":"299947","messageId":"fc1bc9ae5aeb315813d35701e19928cfe52557e3.1471968378.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1471968378.git.johannes.schindelin@gmx.de","subject":"[PATCH 08/15] sequencer: lib'ify walk_revs_populate_todo()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-23T16:07:18Z","receivedAt":"2016-08-23T16:14:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"To be truly useful, the sequencer should never die() but always return\nan error.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 808cd53..1bcea84 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -816,17 +816,19 @@ static int read_populate_opts(struct replay_opts **opts)\n \treturn 0;\n }\n \n-static void walk_revs_populate_todo(struct commit_list **todo_list,\n+static int walk_revs_populate_todo(struct commit_list **todo_list,\n \t\t\t\tstruct replay_opts *opts)\n {\n \tstruct commit *commit;\n \tstruct commit_list **next;\n \n-\tprepare_revs(opts);\n+\tif (prepare_revs(opts))\n+\t\treturn -1;\n \n \tnext = todo_list;\n \twhile ((commit = get_revision(opts->revs)))\n \t\tnext = commit_list_append(commit, next);\n+\treturn 0;\n }\n \n static int create_seq_dir(void)\n@@ -1111,8 +1113,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t * progress\n \t */\n \n-\twalk_revs_populate_todo(&todo_list, opts);\n-\tif (create_seq_dir() < 0)\n+\tif (walk_revs_populate_todo(&todo_list, opts) ||\n+\t\t\tcreate_seq_dir() < 0)\n \t\treturn -1;\n \tif (get_sha1(\"HEAD\", sha1) && (opts->action == REPLAY_REVERT))\n \t\treturn error(_(\"Can't revert as initial commit\"));\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"299948","messageId":"131eea01901cc3c366bad2098f6abe8738922d58.1471968378.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1471968378.git.johannes.schindelin@gmx.de","subject":"[PATCH 05/15] sequencer: lib'ify read_and_refresh_cache()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-23T16:07:08Z","receivedAt":"2016-08-23T16:14:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"To be truly useful, the sequencer should never die() but always return\nan error.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex b90294f..a8c3a48 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -638,18 +638,21 @@ static int prepare_revs(struct replay_opts *opts)\n \treturn 0;\n }\n \n-static void read_and_refresh_cache(struct replay_opts *opts)\n+static int read_and_refresh_cache(struct replay_opts *opts)\n {\n \tstatic struct lock_file index_lock;\n \tint index_fd = hold_locked_index(&index_lock, 0);\n \tif (read_index_preload(&the_index, NULL) < 0)\n-\t\tdie(_(\"git %s: failed to read the index\"), action_name(opts));\n+\t\treturn error(_(\"git %s: failed to read the index\"),\n+\t\t\taction_name(opts));\n \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n \tif (the_index.cache_changed && index_fd >= 0) {\n \t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n-\t\t\tdie(_(\"git %s: failed to refresh the index\"), action_name(opts));\n+\t\t\treturn error(_(\"git %s: failed to refresh the index\"),\n+\t\t\t\taction_name(opts));\n \t}\n \trollback_lock_file(&index_lock);\n+\treturn 0;\n }\n \n static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n@@ -977,7 +980,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n \tif (opts->allow_ff)\n \t\tassert(!(opts->signoff || opts->no_commit ||\n \t\t\t\topts->record_origin || opts->edit));\n-\tread_and_refresh_cache(opts);\n+\tif (read_and_refresh_cache(opts))\n+\t\treturn -1;\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n \t\tsave_todo(cur, opts);\n@@ -1041,7 +1045,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \tif (opts->subcommand == REPLAY_NONE)\n \t\tassert(opts->revs);\n \n-\tread_and_refresh_cache(opts);\n+\tif (read_and_refresh_cache(opts))\n+\t\treturn -1;\n \n \t/*\n \t * Decide what to do depending on the arguments; a fresh\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"299949","messageId":"80062be27256704617280fdb479176e0d2afb4d4.1471968378.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1471968378.git.johannes.schindelin@gmx.de","subject":"[PATCH 09/15] sequencer: lib'ify create_seq_dir()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-23T16:07:22Z","receivedAt":"2016-08-23T16:14:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"To be truly useful, the sequencer should never die() but always return\nan error.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 1bcea84..1706ef4 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -839,8 +839,8 @@ static int create_seq_dir(void)\n \t\treturn -1;\n \t}\n \telse if (mkdir(git_path_seq_dir(), 0777) < 0)\n-\t\tdie_errno(_(\"Could not create sequencer directory %s\"),\n-\t\t\t git_path_seq_dir());\n+\t\treturn error(_(\"Could not create sequencer directory %s (%s)\"),\n+\t\t\t git_path_seq_dir(), strerror(errno));\n \treturn 0;\n }\n \n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"299950","messageId":"fff4cdfa36355af7917a06be9f67b50b56bfce99.1471968378.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1471968378.git.johannes.schindelin@gmx.de","subject":"[PATCH 11/15] sequencer: lib'ify save_todo()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-23T16:07:28Z","receivedAt":"2016-08-23T16:14:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"To be truly useful, the sequencer should never die() but always return\nan error.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 18 ++++++++++++------\n 1 file changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3e07aa0..17f2c8b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -929,24 +929,29 @@ static int sequencer_rollback(struct replay_opts *opts)\n \treturn -1;\n }\n \n-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n+static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n {\n \tstatic struct lock_file todo_lock;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint fd;\n \n-\tfd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), LOCK_DIE_ON_ERROR);\n+\tfd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), 0);\n+\tif (fd < 0)\n+\t\treturn error_errno(_(\"Could not lock '%s'\"),\n+\t\t\t\t git_path_todo_file());\n \tif (format_todo(&buf, todo_list, opts) < 0)\n-\t\tdie(_(\"Could not format %s.\"), git_path_todo_file());\n+\t\treturn error(_(\"Could not format %s.\"), git_path_todo_file());\n \tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n \t\tstrbuf_release(&buf);\n-\t\tdie_errno(_(\"Could not write to %s\"), git_path_todo_file());\n+\t\treturn error_errno(_(\"Could not write to %s\"),\n+\t\t\t\t git_path_todo_file());\n \t}\n \tif (commit_lock_file(&todo_lock) < 0) {\n \t\tstrbuf_release(&buf);\n-\t\tdie(_(\"Error wrapping up %s.\"), git_path_todo_file());\n+\t\treturn error(_(\"Error wrapping up %s.\"), git_path_todo_file());\n \t}\n \tstrbuf_release(&buf);\n+\treturn 0;\n }\n \n static void save_opts(struct replay_opts *opts)\n@@ -995,7 +1000,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n \t\treturn -1;\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n-\t\tsave_todo(cur, opts);\n+\t\tif (save_todo(cur, opts))\n+\t\t\treturn -1;\n \t\tres = do_pick_commit(cur->item, opts);\n \t\tif (res)\n \t\t\treturn res;\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"299951","messageId":"416659fa3df92eabf3265ec079a6ea8cbc17366f.1471968378.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1471968378.git.johannes.schindelin@gmx.de","subject":"[PATCH 10/15] sequencer: lib'ify save_head()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-23T16:07:25Z","receivedAt":"2016-08-23T16:14:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"To be truly useful, the sequencer should never die() but always return\nan error.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 1706ef4..3e07aa0 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -844,18 +844,22 @@ static int create_seq_dir(void)\n \treturn 0;\n }\n \n-static void save_head(const char *head)\n+static int save_head(const char *head)\n {\n \tstatic struct lock_file head_lock;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint fd;\n \n-\tfd = hold_lock_file_for_update(&head_lock, git_path_head_file(), LOCK_DIE_ON_ERROR);\n+\tfd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);\n+\tif (fd < 0)\n+\t\treturn error_errno(_(\"Could not lock HEAD\"));\n \tstrbuf_addf(&buf, \"%s\\n\", head);\n \tif (write_in_full(fd, buf.buf, buf.len) < 0)\n-\t\tdie_errno(_(\"Could not write to %s\"), git_path_head_file());\n+\t\treturn error_errno(_(\"Could not write to %s\"),\n+\t\t\t\t git_path_head_file());\n \tif (commit_lock_file(&head_lock) < 0)\n-\t\tdie(_(\"Error wrapping up %s.\"), git_path_head_file());\n+\t\treturn error(_(\"Error wrapping up %s.\"), git_path_head_file());\n+\treturn 0;\n }\n \n static int reset_for_rollback(const unsigned char *sha1)\n@@ -1118,7 +1122,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t\treturn -1;\n \tif (get_sha1(\"HEAD\", sha1) && (opts->action == REPLAY_REVERT))\n \t\treturn error(_(\"Can't revert as initial commit\"));\n-\tsave_head(sha1_to_hex(sha1));\n+\tif (save_head(sha1_to_hex(sha1)))\n+\t\treturn -1;\n \tsave_opts(opts);\n \treturn pick_commits(todo_list, opts);\n }\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"299952","messageId":"7768b55e65c771a5615a6f1209b40395dc705425.1471968378.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1471968378.git.johannes.schindelin@gmx.de","subject":"[PATCH 12/15] sequencer: lib'ify save_opts()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-23T16:07:33Z","receivedAt":"2016-08-23T16:14:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"To be truly useful, the sequencer should never die() but always return\nan error.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 26 ++++++++++++++------------\n 1 file changed, 14 insertions(+), 12 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 17f2c8b..bac32ea 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -954,37 +954,39 @@ static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n \treturn 0;\n }\n \n-static void save_opts(struct replay_opts *opts)\n+static int save_opts(struct replay_opts *opts)\n {\n \tconst char *opts_file = git_path_opts_file();\n+\tint res = 0;\n \n \tif (opts->no_commit)\n-\t\tgit_config_set_in_file(opts_file, \"options.no-commit\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.no-commit\", \"true\");\n \tif (opts->edit)\n-\t\tgit_config_set_in_file(opts_file, \"options.edit\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.edit\", \"true\");\n \tif (opts->signoff)\n-\t\tgit_config_set_in_file(opts_file, \"options.signoff\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.signoff\", \"true\");\n \tif (opts->record_origin)\n-\t\tgit_config_set_in_file(opts_file, \"options.record-origin\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.record-origin\", \"true\");\n \tif (opts->allow_ff)\n-\t\tgit_config_set_in_file(opts_file, \"options.allow-ff\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.allow-ff\", \"true\");\n \tif (opts->mainline) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tstrbuf_addf(&buf, \"%d\", opts->mainline);\n-\t\tgit_config_set_in_file(opts_file, \"options.mainline\", buf.buf);\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.mainline\", buf.buf);\n \t\tstrbuf_release(&buf);\n \t}\n \tif (opts->strategy)\n-\t\tgit_config_set_in_file(opts_file, \"options.strategy\", opts->strategy);\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.strategy\", opts->strategy);\n \tif (opts->gpg_sign)\n-\t\tgit_config_set_in_file(opts_file, \"options.gpg-sign\", opts->gpg_sign);\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.gpg-sign\", opts->gpg_sign);\n \tif (opts->xopts) {\n \t\tint i;\n \t\tfor (i = 0; i < opts->xopts_nr; i++)\n-\t\t\tgit_config_set_multivar_in_file(opts_file,\n+\t\t\tres |= git_config_set_multivar_in_file_gently(opts_file,\n \t\t\t\t\t\t\t\"options.strategy-option\",\n \t\t\t\t\t\t\topts->xopts[i], \"^$\", 0);\n \t}\n+\treturn res;\n }\n \n static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n@@ -1128,9 +1130,9 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t\treturn -1;\n \tif (get_sha1(\"HEAD\", sha1) && (opts->action == REPLAY_REVERT))\n \t\treturn error(_(\"Can't revert as initial commit\"));\n-\tif (save_head(sha1_to_hex(sha1)))\n+\tif (save_head(sha1_to_hex(sha1)) ||\n+\t\t\tsave_opts(opts))\n \t\treturn -1;\n-\tsave_opts(opts);\n \treturn pick_commits(todo_list, opts);\n }\n \n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"299954","messageId":"3b72bde2c086996166b28d9937e0b940da594a9b.1471968378.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1471968378.git.johannes.schindelin@gmx.de","subject":"[PATCH 14/15] sequencer: do not die() in do_pick_commit()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-23T16:07:54Z","receivedAt":"2016-08-23T16:14:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Signed-off-by: Johannes Schindelin \n---\n sequencer.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 324463f..c29de64 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -589,12 +589,14 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t * However, if the merge did not even start, then we don't want to\n \t * write it at all.\n \t */\n-\tif (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))\n-\t\tupdate_ref(NULL, \"CHERRY_PICK_HEAD\", commit->object.oid.hash, NULL,\n-\t\t\t REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);\n-\tif (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))\n-\t\tupdate_ref(NULL, \"REVERT_HEAD\", commit->object.oid.hash, NULL,\n-\t\t\t REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);\n+\tif (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1) &&\n+\t update_ref(NULL, \"CHERRY_PICK_HEAD\", commit->object.oid.hash, NULL,\n+\t\t REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))\n+\t\tres = -1;\n+\tif (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1) &&\n+\t update_ref(NULL, \"REVERT_HEAD\", commit->object.oid.hash, NULL,\n+\t\t REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))\n+\t\tres = -1;\n \n \tif (res) {\n \t\terror(opts->action == REPLAY_REVERT\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"300024","messageId":"CAPig+cSX_F-vQ_zSrTU6BQSfMeU=5V3krE7xgmBL=g=Jr8i2MA@mail.gmail.com","threadId":"43912","inReplyTo":"6dc5e927cbdd1847db5b6cd75041609dd1e1f76d.1471968378.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 01/15] sequencer: lib'ify write_message()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-08-24T07:09:17Z","receivedAt":"2016-08-24T07:09:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 23, 2016 at 12:06 PM, Johannes Schindelin\n wrote:\n> To be truly useful, the sequencer should never die() but always return\n> an error.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> diff --git a/sequencer.c b/sequencer.c\n> @@ -180,17 +180,20 @@ static void print_advice(int show_hint, struct replay_opts *opts)\n> -static void write_message(struct strbuf *msgbuf, const char *filename)\n> +static int write_message(struct strbuf *msgbuf, const char *filename)\n> {\n> static struct lock_file msg_file;\n>\n> - int msg_fd = hold_lock_file_for_update(&msg_file, filename,\n> - LOCK_DIE_ON_ERROR);\n> + int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);\n> + if (msg_fd < 0)\n> + return error_errno(_(\"Could not lock '%s'\"), filename);\n> if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)\n> - die_errno(_(\"Could not write to %s\"), filename);\n> + return error_errno(_(\"Could not write to %s\"), filename);\n\nDoes this need to rollback the lockfile before returning[*]?\n\n[*] I'm not terribly familiar with the lockfile mechanism and I don't\nhave a lot of time to study it presently, so ignore me if this is a\nstupid question.\n\n> strbuf_release(msgbuf);\n> if (commit_lock_file(&msg_file) < 0)\n> - die(_(\"Error wrapping up %s.\"), filename);\n> + return error(_(\"Error wrapping up %s.\"), filename);\n> +\n> + return 0;\n> }\n"},{"id":"300025","messageId":"CAPig+cTCotTDT83x9=q5ORR1ZWR9oewvXiun9sjvEbTV8OuChA@mail.gmail.com","threadId":"43912","inReplyTo":"e00df1449af0d8c55000a93c734d8a241b1cb5f0.1471968378.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 02/15] sequencer: lib'ify do_recursive_merge()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-08-24T07:16:38Z","receivedAt":"2016-08-24T07:16:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 23, 2016 at 12:06 PM, Johannes Schindelin\n wrote:\n> To be truly useful, the sequencer should never die() but always return\n> an error.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> diff --git a/sequencer.c b/sequencer.c\n> @@ -303,7 +303,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n> if (active_cache_changed &&\n> write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n> /* TRANSLATORS: %s will be \"revert\" or \"cherry-pick\" */\n> - die(_(\"%s: Unable to write new index file\"), action_name(opts));\n> + return error(_(\"%s: Unable to write new index file\"),\n> + action_name(opts));\n\nDoes this need to rollback the lockfile before returning?\n\nA cursory scan of read-cache.c:do_write_locked_index() seems to\nindicate that lockfile disposition is not handled automatically in\ncase of error (unless I'm misreading).\n\n> rollback_lock_file(&index_lock);\n>\n> if (opts->signoff)\n"},{"id":"300026","messageId":"CAPig+cTP+GNubCaveO6Sf9t44VQit0EaFjXCFexq2CtCMG2=Fw@mail.gmail.com","threadId":"43912","inReplyTo":"131eea01901cc3c366bad2098f6abe8738922d58.1471968378.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 05/15] sequencer: lib'ify read_and_refresh_cache()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-08-24T07:20:35Z","receivedAt":"2016-08-24T07:20:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin\n wrote:\n> To be truly useful, the sequencer should never die() but always return\n> an error.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> diff --git a/sequencer.c b/sequencer.c\n> @@ -638,18 +638,21 @@ static int prepare_revs(struct replay_opts *opts)\n> -static void read_and_refresh_cache(struct replay_opts *opts)\n> +static int read_and_refresh_cache(struct replay_opts *opts)\n> {\n> static struct lock_file index_lock;\n> int index_fd = hold_locked_index(&index_lock, 0);\n> if (read_index_preload(&the_index, NULL) < 0)\n> - die(_(\"git %s: failed to read the index\"), action_name(opts));\n> + return error(_(\"git %s: failed to read the index\"),\n> + action_name(opts));\n> refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n> if (the_index.cache_changed && index_fd >= 0) {\n> if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n> - die(_(\"git %s: failed to refresh the index\"), action_name(opts));\n> + return error(_(\"git %s: failed to refresh the index\"),\n> + action_name(opts));\n\nDo these two error returns need to rollback the lockfile?\n\n> }\n> rollback_lock_file(&index_lock);\n> + return 0;\n> }\n"},{"id":"300027","messageId":"CAPig+cSR8BHFdMJz5mFkpJ3UU6w0Xmjav+tu6f3DrkG4Gi-v6A@mail.gmail.com","threadId":"43912","inReplyTo":"0de75bbce8ade0c6e5cf87d3647faa71a89c6275.1471968378.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 06/15] sequencer: lib'ify read_populate_todo()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-08-24T07:24:46Z","receivedAt":"2016-08-24T07:25:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin\n wrote:\n> To be truly useful, the sequencer should never die() but always return\n> an error.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> diff --git a/sequencer.c b/sequencer.c\n> @@ -754,18 +754,21 @@ static void read_populate_todo(struct commit_list **todo_list,\n>\n> fd = open(git_path_todo_file(), O_RDONLY);\n> if (fd < 0)\n> - die_errno(_(\"Could not open %s\"), git_path_todo_file());\n> + return error(_(\"Could not open %s (%s)\"),\n> + git_path_todo_file(), strerror(errno));\n\nerror_errno() perhaps?\n\n> if (strbuf_read(&buf, fd, 0) < 0) {\n> close(fd);\n> strbuf_release(&buf);\n> - die(_(\"Could not read %s.\"), git_path_todo_file());\n> + return error(_(\"Could not read %s.\"), git_path_todo_file());\n> }\n> close(fd);\n>\n> res = parse_insn_buffer(buf.buf, todo_list, opts);\n> strbuf_release(&buf);\n> if (res)\n> - die(_(\"Unusable instruction sheet: %s\"), git_path_todo_file());\n> + return error(_(\"Unusable instruction sheet: %s\"),\n> + git_path_todo_file());\n\nNeither 'fd' nor 'buf' are leaked by these two new error returns. Good.\n\n> + return 0;\n> }\n"},{"id":"300028","messageId":"CAPig+cT7fNsyhk1J0Z6evk8xRxa=g8ygUniBB+EHSRMRFcNLcQ@mail.gmail.com","threadId":"43912","inReplyTo":"80062be27256704617280fdb479176e0d2afb4d4.1471968378.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 09/15] sequencer: lib'ify create_seq_dir()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-08-24T07:28:44Z","receivedAt":"2016-08-24T07:29:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin\n wrote:\n> To be truly useful, the sequencer should never die() but always return\n> an error.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> diff --git a/sequencer.c b/sequencer.c\n> @@ -839,8 +839,8 @@ static int create_seq_dir(void)\n> return -1;\n> }\n> else if (mkdir(git_path_seq_dir(), 0777) < 0)\n> - die_errno(_(\"Could not create sequencer directory %s\"),\n> - git_path_seq_dir());\n> + return error(_(\"Could not create sequencer directory %s (%s)\"),\n> + git_path_seq_dir(), strerror(errno));\n\nerror_errno()?\n\n> return 0;\n> }\n"},{"id":"300029","messageId":"CAPig+cSxeOkT7fLuCQrGJNabPfx1855+1-5pqcq0==22mzJk9w@mail.gmail.com","threadId":"43912","inReplyTo":"416659fa3df92eabf3265ec079a6ea8cbc17366f.1471968378.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 10/15] sequencer: lib'ify save_head()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-08-24T07:30:51Z","receivedAt":"2016-08-24T07:32:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin\n wrote:\n> To be truly useful, the sequencer should never die() but always return\n> an error.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> diff --git a/sequencer.c b/sequencer.c\n> @@ -844,18 +844,22 @@ static int create_seq_dir(void)\n> -static void save_head(const char *head)\n> +static int save_head(const char *head)\n> {\n> static struct lock_file head_lock;\n> struct strbuf buf = STRBUF_INIT;\n> int fd;\n>\n> - fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), LOCK_DIE_ON_ERROR);\n> + fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);\n> + if (fd < 0)\n> + return error_errno(_(\"Could not lock HEAD\"));\n> strbuf_addf(&buf, \"%s\\n\", head);\n> if (write_in_full(fd, buf.buf, buf.len) < 0)\n> - die_errno(_(\"Could not write to %s\"), git_path_head_file());\n> + return error_errno(_(\"Could not write to %s\"),\n> + git_path_head_file());\n\nDoes this need to rollback the lockfile before returning?\n\n> if (commit_lock_file(&head_lock) < 0)\n> - die(_(\"Error wrapping up %s.\"), git_path_head_file());\n> + return error(_(\"Error wrapping up %s.\"), git_path_head_file());\n> + return 0;\n> }\n"},{"id":"300030","messageId":"CAPig+cQ2xH1ytsTGNDeDZiHTZpnp28XUJoAY+W-+iw+85gMbtg@mail.gmail.com","threadId":"43912","inReplyTo":"fff4cdfa36355af7917a06be9f67b50b56bfce99.1471968378.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 11/15] sequencer: lib'ify save_todo()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-08-24T07:36:56Z","receivedAt":"2016-08-24T07:38:36Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin\n wrote:\n> To be truly useful, the sequencer should never die() but always return\n> an error.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> diff --git a/sequencer.c b/sequencer.c\n> @@ -929,24 +929,29 @@ static int sequencer_rollback(struct replay_opts *opts)\n> -static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n> +static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n> {\n> static struct lock_file todo_lock;\n> struct strbuf buf = STRBUF_INIT;\n> int fd;\n>\n> - fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), LOCK_DIE_ON_ERROR);\n> + fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), 0);\n> + if (fd < 0)\n> + return error_errno(_(\"Could not lock '%s'\"),\n> + git_path_todo_file());\n> if (format_todo(&buf, todo_list, opts) < 0)\n> - die(_(\"Could not format %s.\"), git_path_todo_file());\n> + return error(_(\"Could not format %s.\"), git_path_todo_file());\n\nformat_todo() doesn't seem to make any promises about the state of the\nstrbuf upon error, so should this be releasing the strbuf before\nreturning?\n\n> if (write_in_full(fd, buf.buf, buf.len) < 0) {\n> strbuf_release(&buf);\n> - die_errno(_(\"Could not write to %s\"), git_path_todo_file());\n> + return error_errno(_(\"Could not write to %s\"),\n> + git_path_todo_file());\n\nDo the above two new error returns need to rollback the lockfile?\n\n> }\n> if (commit_lock_file(&todo_lock) < 0) {\n> strbuf_release(&buf);\n> - die(_(\"Error wrapping up %s.\"), git_path_todo_file());\n> + return error(_(\"Error wrapping up %s.\"), git_path_todo_file());\n> }\n> strbuf_release(&buf);\n> + return 0;\n> }\n"},{"id":"300061","messageId":"alpine.DEB.2.20.1608241749190.4924@virtualbox","threadId":"43912","inReplyTo":"CAPig+cSX_F-vQ_zSrTU6BQSfMeU=5V3krE7xgmBL=g=Jr8i2MA@mail.gmail.com","subject":"Re: [PATCH 01/15] sequencer: lib'ify write_message()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-24T15:52:54Z","receivedAt":"2016-08-24T15:53:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Wed, 24 Aug 2016, Eric Sunshine wrote:\n\n> On Tue, Aug 23, 2016 at 12:06 PM, Johannes Schindelin\n> wrote:\n> > To be truly useful, the sequencer should never die() but always return\n> > an error.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > diff --git a/sequencer.c b/sequencer.c\n> > @@ -180,17 +180,20 @@ static void print_advice(int show_hint, struct replay_opts *opts)\n> > -static void write_message(struct strbuf *msgbuf, const char *filename)\n> > +static int write_message(struct strbuf *msgbuf, const char *filename)\n> > {\n> > static struct lock_file msg_file;\n> >\n> > - int msg_fd = hold_lock_file_for_update(&msg_file, filename,\n> > - LOCK_DIE_ON_ERROR);\n> > + int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);\n> > + if (msg_fd < 0)\n> > + return error_errno(_(\"Could not lock '%s'\"), filename);\n> > if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)\n> > - die_errno(_(\"Could not write to %s\"), filename);\n> > + return error_errno(_(\"Could not write to %s\"), filename);\n> \n> Does this need to rollback the lockfile before returning[*]?\n> \n> [*] I'm not terribly familiar with the lockfile mechanism and I don't\n> have a lot of time to study it presently, so ignore me if this is a\n> stupid question.\n\nNot a stupid question at all.\n\nThe way lockfiles work is that there is an atexit() handler that ensures\nthat uncommitted lockfiles get rolled back automatically. So it happens to\nwork correctly right now, because all (direct and transitive) callers\nsimply stop doing things as quickly as possible. That means that no\nsubsequent attempt is made to write to the same file.\n\nTechnically, I agree with you that it may look a bit cleaner to roll back\nat this point. However, this is outside the purview of this here patch\nseries, as it would actually change the behavior (the previous die() would\nrely on the atexit() handler to roll back the locked file, too). So I\nthink if this is to be changed, it has to be in its own, separate patch\nseries.\n\nCiao,\nDscho\n"},{"id":"300062","messageId":"alpine.DEB.2.20.1608241753150.4924@virtualbox","threadId":"43912","inReplyTo":"CAPig+cTCotTDT83x9=q5ORR1ZWR9oewvXiun9sjvEbTV8OuChA@mail.gmail.com","subject":"Re: [PATCH 02/15] sequencer: lib'ify do_recursive_merge()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-24T15:53:50Z","receivedAt":"2016-08-24T15:54:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Wed, 24 Aug 2016, Eric Sunshine wrote:\n\n> On Tue, Aug 23, 2016 at 12:06 PM, Johannes Schindelin\n> wrote:\n> > To be truly useful, the sequencer should never die() but always return\n> > an error.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > diff --git a/sequencer.c b/sequencer.c\n> > @@ -303,7 +303,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n> > if (active_cache_changed &&\n> > write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n> > /* TRANSLATORS: %s will be \"revert\" or \"cherry-pick\" */\n> > - die(_(\"%s: Unable to write new index file\"), action_name(opts));\n> > + return error(_(\"%s: Unable to write new index file\"),\n> > + action_name(opts));\n> \n> Does this need to rollback the lockfile before returning?\n> \n> A cursory scan of read-cache.c:do_write_locked_index() seems to\n> indicate that lockfile disposition is not handled automatically in\n> case of error (unless I'm misreading).\n\nAs mentioned elsewhere in this thread: an atexit() handler is tasked with\nrolling back uncommitted lockfiles.\n\nCiao,\nDscho\n"},{"id":"300063","messageId":"alpine.DEB.2.20.1608241754590.4924@virtualbox","threadId":"43912","inReplyTo":"CAPig+cSR8BHFdMJz5mFkpJ3UU6w0Xmjav+tu6f3DrkG4Gi-v6A@mail.gmail.com","subject":"Re: [PATCH 06/15] sequencer: lib'ify read_populate_todo()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-24T15:57:17Z","receivedAt":"2016-08-24T15:57:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Wed, 24 Aug 2016, Eric Sunshine wrote:\n\n> On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin\n> wrote:\n> > To be truly useful, the sequencer should never die() but always return\n> > an error.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > diff --git a/sequencer.c b/sequencer.c\n> > @@ -754,18 +754,21 @@ static void read_populate_todo(struct commit_list **todo_list,\n> >\n> > fd = open(git_path_todo_file(), O_RDONLY);\n> > if (fd < 0)\n> > - die_errno(_(\"Could not open %s\"), git_path_todo_file());\n> > + return error(_(\"Could not open %s (%s)\"),\n> > + git_path_todo_file(), strerror(errno));\n> \n> error_errno() perhaps?\n\nAbsolutely!\n\nThanks,\nDscho\n"},{"id":"300064","messageId":"alpine.DEB.2.20.1608241758400.4924@virtualbox","threadId":"43912","inReplyTo":"CAPig+cT7fNsyhk1J0Z6evk8xRxa=g8ygUniBB+EHSRMRFcNLcQ@mail.gmail.com","subject":"Re: [PATCH 09/15] sequencer: lib'ify create_seq_dir()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-24T15:58:51Z","receivedAt":"2016-08-24T15:59:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Wed, 24 Aug 2016, Eric Sunshine wrote:\n\n> On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin\n> wrote:\n> > To be truly useful, the sequencer should never die() but always return\n> > an error.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > diff --git a/sequencer.c b/sequencer.c\n> > @@ -839,8 +839,8 @@ static int create_seq_dir(void)\n> > return -1;\n> > }\n> > else if (mkdir(git_path_seq_dir(), 0777) < 0)\n> > - die_errno(_(\"Could not create sequencer directory %s\"),\n> > - git_path_seq_dir());\n> > + return error(_(\"Could not create sequencer directory %s (%s)\"),\n> > + git_path_seq_dir(), strerror(errno));\n> \n> error_errno()?\n\nYep!\n\nThanks,\nDscho\n"},{"id":"300065","messageId":"alpine.DEB.2.20.1608241759370.4924@virtualbox","threadId":"43912","inReplyTo":"CAPig+cSxeOkT7fLuCQrGJNabPfx1855+1-5pqcq0==22mzJk9w@mail.gmail.com","subject":"Re: [PATCH 10/15] sequencer: lib'ify save_head()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-24T15:59:56Z","receivedAt":"2016-08-24T16:00:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Wed, 24 Aug 2016, Eric Sunshine wrote:\n\n> On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin\n> wrote:\n> > To be truly useful, the sequencer should never die() but always return\n> > an error.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > diff --git a/sequencer.c b/sequencer.c\n> > @@ -844,18 +844,22 @@ static int create_seq_dir(void)\n> > -static void save_head(const char *head)\n> > +static int save_head(const char *head)\n> > {\n> > static struct lock_file head_lock;\n> > struct strbuf buf = STRBUF_INIT;\n> > int fd;\n> >\n> > - fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), LOCK_DIE_ON_ERROR);\n> > + fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);\n> > + if (fd < 0)\n> > + return error_errno(_(\"Could not lock HEAD\"));\n> > strbuf_addf(&buf, \"%s\\n\", head);\n> > if (write_in_full(fd, buf.buf, buf.len) < 0)\n> > - die_errno(_(\"Could not write to %s\"), git_path_head_file());\n> > + return error_errno(_(\"Could not write to %s\"),\n> > + git_path_head_file());\n> \n> Does this need to rollback the lockfile before returning?\n\nAgain, this is handled by the atexit() handler, as before.\n\nThanks!\nDscho\n"},{"id":"300067","messageId":"alpine.DEB.2.20.1608241801030.4924@virtualbox","threadId":"43912","inReplyTo":"CAPig+cQ2xH1ytsTGNDeDZiHTZpnp28XUJoAY+W-+iw+85gMbtg@mail.gmail.com","subject":"Re: [PATCH 11/15] sequencer: lib'ify save_todo()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-24T16:05:08Z","receivedAt":"2016-08-24T16:05:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Wed, 24 Aug 2016, Eric Sunshine wrote:\n\n> On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin\n> wrote:\n> > To be truly useful, the sequencer should never die() but always return\n> > an error.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > diff --git a/sequencer.c b/sequencer.c\n> > @@ -929,24 +929,29 @@ static int sequencer_rollback(struct replay_opts *opts)\n> > -static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n> > +static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n> > {\n> > static struct lock_file todo_lock;\n> > struct strbuf buf = STRBUF_INIT;\n> > int fd;\n> >\n> > - fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), LOCK_DIE_ON_ERROR);\n> > + fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), 0);\n> > + if (fd < 0)\n> > + return error_errno(_(\"Could not lock '%s'\"),\n> > + git_path_todo_file());\n> > if (format_todo(&buf, todo_list, opts) < 0)\n> > - die(_(\"Could not format %s.\"), git_path_todo_file());\n> > + return error(_(\"Could not format %s.\"), git_path_todo_file());\n> \n> format_todo() doesn't seem to make any promises about the state of the\n> strbuf upon error, so should this be releasing the strbuf before\n> returning?\n\nYes, it should. Thank you!\n\n> > if (write_in_full(fd, buf.buf, buf.len) < 0) {\n> > strbuf_release(&buf);\n> > - die_errno(_(\"Could not write to %s\"), git_path_todo_file());\n> > + return error_errno(_(\"Could not write to %s\"),\n> > + git_path_todo_file());\n> \n> Do the above two new error returns need to rollback the lockfile?\n\nAs before, atexit() handler to the rescue ;-)\n\nThanks,\nDscho\n"},{"id":"300070","messageId":"alpine.DEB.2.20.1608241754080.4924@virtualbox","threadId":"43912","inReplyTo":"CAPig+cTP+GNubCaveO6Sf9t44VQit0EaFjXCFexq2CtCMG2=Fw@mail.gmail.com","subject":"Re: [PATCH 05/15] sequencer: lib'ify read_and_refresh_cache()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-24T15:54:35Z","receivedAt":"2016-08-24T16:11:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Wed, 24 Aug 2016, Eric Sunshine wrote:\n\n> On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin\n> wrote:\n> > To be truly useful, the sequencer should never die() but always return\n> > an error.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > diff --git a/sequencer.c b/sequencer.c\n> > @@ -638,18 +638,21 @@ static int prepare_revs(struct replay_opts *opts)\n> > -static void read_and_refresh_cache(struct replay_opts *opts)\n> > +static int read_and_refresh_cache(struct replay_opts *opts)\n> > {\n> > static struct lock_file index_lock;\n> > int index_fd = hold_locked_index(&index_lock, 0);\n> > if (read_index_preload(&the_index, NULL) < 0)\n> > - die(_(\"git %s: failed to read the index\"), action_name(opts));\n> > + return error(_(\"git %s: failed to read the index\"),\n> > + action_name(opts));\n> > refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n> > if (the_index.cache_changed && index_fd >= 0) {\n> > if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n> > - die(_(\"git %s: failed to refresh the index\"), action_name(opts));\n> > + return error(_(\"git %s: failed to refresh the index\"),\n> > + action_name(opts));\n> \n> Do these two error returns need to rollback the lockfile?\n\nHere, too, the atexit() handler does the job, and again, this is not a\nchange in behavior.\n\nThanks for your review!\nDscho\n"},{"id":"300200","messageId":"xmqqh9a8lf9t.fsf@gitster.mtv.corp.google.com","threadId":"43912","inReplyTo":"92fdb70f3db157f8a3d2442d872e989d81c6756e.1471968378.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 03/15] sequencer: lib'ify do_pick_commit()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-25T21:17:34Z","receivedAt":"2016-08-25T21:19:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> To be truly useful, the sequencer should never die() but always return\n> an error.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n\nInstead of dying there, you let the caller high up in the callchain\nto notice the error and handle it (by dying). The eventual caller\nof do_pick_commit() is sequencer_pick_revisions(), which already\nrelays an reported error from its helper functions (including this\none), and both of its two callers know how to react to a negative\nreturn correctly. So this makes do_pick_commit() callable from new\ncallers that want it not to die, without changing the external\nbehaviour of anything existing.\n\nGood.\n\n> sequencer.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 0c8c955..6ac2187 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -464,7 +464,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n> \t\t * to work on.\n> \t\t */\n> \t\tif (write_cache_as_tree(head, 0, NULL))\n> -\t\t\tdie (_(\"Your index file is unmerged.\"));\n> +\t\t\treturn error (_(\"Your index file is unmerged.\"));\n\nWhile you are touching the line, it is a good idea to correct an\nobvious style error like this one. \"Do one thing and one thing well\nin a commit\" is a good discipline, but it is absurd to take it to\nthe extreme.\n\n> \t} else {\n> \t\tunborn = get_sha1(\"HEAD\", head);\n> \t\tif (unborn)\n"},{"id":"300209","messageId":"xmqqmvk0jwgj.fsf@gitster.mtv.corp.google.com","threadId":"43912","inReplyTo":"131eea01901cc3c366bad2098f6abe8738922d58.1471968378.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 05/15] sequencer: lib'ify read_and_refresh_cache()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-25T22:49:16Z","receivedAt":"2016-08-25T22:50:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> To be truly useful, the sequencer should never die() but always return\n> an error.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> sequencer.c | 15 ++++++++++-----\n> 1 file changed, 10 insertions(+), 5 deletions(-)\n\nInstead of dying there, you let the caller high up in the callchain\nto notice the error and handle it (by dying). There are two call\nsites of read_and_refresh_cache(), one of which is pick_commits(),\nwhose callers you already verified that they are prepared to do the\nright thing given an \"error\" return from it when you did 3/15, so\nthe conversion is safe. The other one, sequencer_pick_revisions()\nis also prepared to relay an error return back to its caller and you\nmade sure its callers are all safe when you did 3/15.\n\nGood.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index b90294f..a8c3a48 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -638,18 +638,21 @@ static int prepare_revs(struct replay_opts *opts)\n> \treturn 0;\n> }\n> \n> -static void read_and_refresh_cache(struct replay_opts *opts)\n> +static int read_and_refresh_cache(struct replay_opts *opts)\n> {\n> \tstatic struct lock_file index_lock;\n> \tint index_fd = hold_locked_index(&index_lock, 0);\n> \tif (read_index_preload(&the_index, NULL) < 0)\n> -\t\tdie(_(\"git %s: failed to read the index\"), action_name(opts));\n> +\t\treturn error(_(\"git %s: failed to read the index\"),\n> +\t\t\taction_name(opts));\n> \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n> \tif (the_index.cache_changed && index_fd >= 0) {\n> \t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n> -\t\t\tdie(_(\"git %s: failed to refresh the index\"), action_name(opts));\n> +\t\t\treturn error(_(\"git %s: failed to refresh the index\"),\n> +\t\t\t\taction_name(opts));\n> \t}\n> \trollback_lock_file(&index_lock);\n> +\treturn 0;\n> }\n> \n> static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n> @@ -977,7 +980,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n> \tif (opts->allow_ff)\n> \t\tassert(!(opts->signoff || opts->no_commit ||\n> \t\t\t\topts->record_origin || opts->edit));\n> -\tread_and_refresh_cache(opts);\n> +\tif (read_and_refresh_cache(opts))\n> +\t\treturn -1;\n> \n> \tfor (cur = todo_list; cur; cur = cur->next) {\n> \t\tsave_todo(cur, opts);\n> @@ -1041,7 +1045,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n> \tif (opts->subcommand == REPLAY_NONE)\n> \t\tassert(opts->revs);\n> \n> -\tread_and_refresh_cache(opts);\n> +\tif (read_and_refresh_cache(opts))\n> +\t\treturn -1;\n> \n> \t/*\n> \t * Decide what to do depending on the arguments; a fresh\n"},{"id":"300210","messageId":"xmqqinuojwc4.fsf@gitster.mtv.corp.google.com","threadId":"43912","inReplyTo":"ce6a7cac9f0bd53e44129c8f72818adc4bb4a934.1471968378.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 04/15] sequencer: lib'ify prepare_revs()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-25T22:51:55Z","receivedAt":"2016-08-25T22:53:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> To be truly useful, the sequencer should never die() but always return\n> an error.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n\nI am still looking at sequencer.c in 'master', but I do not think\nthat the sole caller of this function, walk_revs_populate_todo(),\nis prepared to act on an error return from this function and instead\nit expects this to die() when in trouble. And I do not think I saw\nthe function touched in the steps so far.\n\nSo this step smells like a fishy conversion to me.\n\n> sequencer.c | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 6ac2187..b90294f 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -621,7 +621,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n> \treturn res;\n> }\n> \n> -static void prepare_revs(struct replay_opts *opts)\n> +static int prepare_revs(struct replay_opts *opts)\n> {\n> \t/*\n> \t * picking (but not reverting) ranges (but not individual revisions)\n> @@ -631,10 +631,11 @@ static void prepare_revs(struct replay_opts *opts)\n> \t\topts->revs->reverse ^= 1;\n> \n> \tif (prepare_revision_walk(opts->revs))\n> -\t\tdie(_(\"revision walk setup failed\"));\n> +\t\treturn error(_(\"revision walk setup failed\"));\n> \n> \tif (!opts->revs->commits)\n> -\t\tdie(_(\"empty commit set passed\"));\n> +\t\treturn error(_(\"empty commit set passed\"));\n> +\treturn 0;\n> }\n> \n> static void read_and_refresh_cache(struct replay_opts *opts)\n"},{"id":"300211","messageId":"xmqqeg5cjw02.fsf@gitster.mtv.corp.google.com","threadId":"43912","inReplyTo":"0de75bbce8ade0c6e5cf87d3647faa71a89c6275.1471968378.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 06/15] sequencer: lib'ify read_populate_todo()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-25T22:59:09Z","receivedAt":"2016-08-25T23:05:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> To be truly useful, the sequencer should never die() but always return\n> an error.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> sequencer.c | 14 +++++++++-----\n> 1 file changed, 9 insertions(+), 5 deletions(-)\n\nInstead of dying there, you let the caller high up in the callchain\nto notice the error and handle it (by dying).\n\nThe only caller of read_populate_todo(), sequencer_continue() can\nalready return errors, so its caller must be already prepared to\nhandle error returns, and with this step, you make it notice an\nerror return from this function. So this is a safe conversion to\nmake read_populate_todo() callable from new callers that want it not\nto die, without changing the external behaviour of anything\nexisting.\n\nGood.\n\nBy the way, I am writing these as review comments because I do not\nwant to keep repeating this kind of analysis as a reviewer. I am\ndemonstrating what should have been in the commit log message\ninstead, so that the reviewer does not have to spend extra time, if\nthe reviewer trusts the author's diligence well enough, to see if\nthe conversion makes sense.\n\nPlease follow the example when/if you have to reroll. I want the\npatches to show the evidence of careful analysis to reviewers so\nthat they can gauge the trustworthiness of the patches. With this\nround of patches, honestly, I cannot tell if it is a mechanical\nsubstitution alone, or such a substitution followed by a careful\nverification of the callers.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index a8c3a48..5f6b020 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -746,7 +746,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,\n> \treturn 0;\n> }\n> \n> -static void read_populate_todo(struct commit_list **todo_list,\n> +static int read_populate_todo(struct commit_list **todo_list,\n> \t\t\tstruct replay_opts *opts)\n> {\n> \tstruct strbuf buf = STRBUF_INIT;\n> @@ -754,18 +754,21 @@ static void read_populate_todo(struct commit_list **todo_list,\n> \n> \tfd = open(git_path_todo_file(), O_RDONLY);\n> \tif (fd < 0)\n> -\t\tdie_errno(_(\"Could not open %s\"), git_path_todo_file());\n> +\t\treturn error(_(\"Could not open %s (%s)\"),\n> +\t\t\tgit_path_todo_file(), strerror(errno));\n> \tif (strbuf_read(&buf, fd, 0) < 0) {\n> \t\tclose(fd);\n> \t\tstrbuf_release(&buf);\n> -\t\tdie(_(\"Could not read %s.\"), git_path_todo_file());\n> +\t\treturn error(_(\"Could not read %s.\"), git_path_todo_file());\n> \t}\n> \tclose(fd);\n> \n> \tres = parse_insn_buffer(buf.buf, todo_list, opts);\n> \tstrbuf_release(&buf);\n> \tif (res)\n> -\t\tdie(_(\"Unusable instruction sheet: %s\"), git_path_todo_file());\n> +\t\treturn error(_(\"Unusable instruction sheet: %s\"),\n> +\t\t\tgit_path_todo_file());\n> +\treturn 0;\n> }\n> \n> static int populate_opts_cb(const char *key, const char *value, void *data)\n> @@ -1015,7 +1018,8 @@ static int sequencer_continue(struct replay_opts *opts)\n> \tif (!file_exists(git_path_todo_file()))\n> \t\treturn continue_single_pick();\n> \tread_populate_opts(&opts);\n> -\tread_populate_todo(&todo_list, opts);\n> +\tif (read_populate_todo(&todo_list, opts))\n> +\t\treturn -1;\n> \n> \t/* Verify that the conflict has been resolved */\n> \tif (file_exists(git_path_cherry_pick_head()) ||\n"},{"id":"300236","messageId":"alpine.DEB.2.20.1608261355470.4924@virtualbox","threadId":"43912","inReplyTo":"xmqqh9a8lf9t.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 03/15] sequencer: lib'ify do_pick_commit()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-26T11:56:30Z","receivedAt":"2016-08-26T12:03:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 25 Aug 2016, Junio C Hamano wrote:\n\n> > \t\tif (write_cache_as_tree(head, 0, NULL))\n> > -\t\t\tdie (_(\"Your index file is unmerged.\"));\n> > +\t\t\treturn error (_(\"Your index file is unmerged.\"));\n> \n> While you are touching the line, it is a good idea to correct an\n> obvious style error like this one. \"Do one thing and one thing well\n> in a commit\" is a good discipline, but it is absurd to take it to\n> the extreme.\n\nTo be quite honest, I had to look *really* hard to see that space. It took\nme literally 30 seconds to spot the style issue.\n\nFixed in v2,\nDscho\n"},{"id":"300239","messageId":"alpine.DEB.2.20.1608261534110.4924@virtualbox","threadId":"43912","inReplyTo":"xmqqinuojwc4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 04/15] sequencer: lib'ify prepare_revs()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-26T13:40:34Z","receivedAt":"2016-08-26T13:41:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 25 Aug 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > To be truly useful, the sequencer should never die() but always return\n> > an error.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> \n> I am still looking at sequencer.c in 'master', but I do not think\n> that the sole caller of this function, walk_revs_populate_todo(),\n> is prepared to act on an error return from this function and instead\n> it expects this to die() when in trouble. And I do not think I saw\n> the function touched in the steps so far.\n> \n> So this step smells like a fishy conversion to me.\n\nThe fishy part is, of course, that I managed not to realize that the patch\nlibifiying walk_revs_populate_todo() came *after* this patch that libified\none of its callees, prepare_revs().\n\nFWIW the same is true for sequencer_pick_revisions(), which was libified\nway too late in the game.\n\nThis is once again a demonstration that a final patch series does *not*\nreflect the process of developing it: in this case, the order of the\npatches is the *opposite* of their development, as I libified functions\nfrom the lowest, deepest call depth up to the top-level functions.\n\nFixed in v2,\nDscho\n"},{"id":"300240","messageId":"alpine.DEB.2.20.1608261540470.4924@virtualbox","threadId":"43912","inReplyTo":"xmqqeg5cjw02.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 06/15] sequencer: lib'ify read_populate_todo()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-26T13:45:07Z","receivedAt":"2016-08-26T13:47:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 25 Aug 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > To be truly useful, the sequencer should never die() but always return\n> > an error.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > sequencer.c | 14 +++++++++-----\n> > 1 file changed, 9 insertions(+), 5 deletions(-)\n> \n> Instead of dying there, you let the caller high up in the callchain\n> to notice the error and handle it (by dying).\n> \n> The only caller of read_populate_todo(), sequencer_continue() can\n> already return errors, so its caller must be already prepared to\n> handle error returns, and with this step, you make it notice an\n> error return from this function. So this is a safe conversion to\n> make read_populate_todo() callable from new callers that want it not\n> to die, without changing the external behaviour of anything\n> existing.\n> \n> Good.\n> \n> By the way, I am writing these as review comments because I do not\n> want to keep repeating this kind of analysis as a reviewer. I am\n> demonstrating what should have been in the commit log message\n> instead, so that the reviewer does not have to spend extra time, if\n> the reviewer trusts the author's diligence well enough, to see if\n> the conversion makes sense.\n> \n> Please follow the example when/if you have to reroll. I want the\n> patches to show the evidence of careful analysis to reviewers so\n> that they can gauge the trustworthiness of the patches. With this\n> round of patches, honestly, I cannot tell if it is a mechanical\n> substitution alone, or such a substitution followed by a careful\n> verification of the callers.\n\nDuly noted.\n\nI fixed up the patch order as well as the commit messages.\n\nNow on to a request of my own: I am once again reminded that I have *no*\ngood mail client to serve me. (Before you suggest it: no, emacs won't cut\nit for me, nor mutt. Thank you very much for your suggestion.) So it is\nreally annoying for me to scroll through quoted text, page after page,\nonly to find that none of it got a response after all.\n\nIn short: I would really appreciate it if you could cut quoted text after\nyour last response.\n\nThanks,\nDscho\n\nP.S.: It's this mailing list thing again. I would *love* to switch to a\nmail client more to my liking, but the ones I like all won't respect white\nspace in patch files that are pasted verbatim into the mail body.\n"},{"id":"300242","messageId":"39d03662fc6ea81d737c0650bb9b7292e812ae02.1472219214.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1472219214.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 05/14] sequencer: lib'ify do_pick_commit()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-26T13:47:39Z","receivedAt":"2016-08-26T13:48:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain notice\nthe error and handle it (by dying, still).\n\nThe only two callers of do_pick_commit(), pick_commits() and\nsingle_pick() already check the return value and pass it on to their\ncallers, so their callers must be already prepared to handle error\nreturns, and with this step, we make it notice an error return from\nthis function.\n\nSo this is a safe conversion to make do_pick_commit() callable from\nnew callers that want it not to die, without changing the external\nbehaviour of anything existing.\n\nWhile at it, remove the superfluous space.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f6cdf65..7eef512 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -464,7 +464,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t * to work on.\n \t\t */\n \t\tif (write_cache_as_tree(head, 0, NULL))\n-\t\t\tdie (_(\"Your index file is unmerged.\"));\n+\t\t\treturn error(_(\"Your index file is unmerged.\"));\n \t} else {\n \t\tunborn = get_sha1(\"HEAD\", head);\n \t\tif (unborn)\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"300243","messageId":"3a472f0d39080f9a98df0e85b447007a582eb11f.1472219214.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1472219214.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 09/14] sequencer: lib'ify read_populate_todo()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-26T13:47:53Z","receivedAt":"2016-08-26T13:48:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain\nnotice the error and handle it (by dying, still).\n\nThe only caller of read_populate_todo(), sequencer_continue() can\nalready return errors, so its caller must be already prepared to\nhandle error returns, and with this step, we make it notice an\nerror return from this function.\n\nSo this is a safe conversion to make read_populate_todo() callable\nfrom new callers that want it not to die, without changing the\nexternal behaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e30aa82..e11b24f 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -748,7 +748,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,\n \treturn 0;\n }\n \n-static void read_populate_todo(struct commit_list **todo_list,\n+static int read_populate_todo(struct commit_list **todo_list,\n \t\t\tstruct replay_opts *opts)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -756,18 +756,21 @@ static void read_populate_todo(struct commit_list **todo_list,\n \n \tfd = open(git_path_todo_file(), O_RDONLY);\n \tif (fd < 0)\n-\t\tdie_errno(_(\"Could not open %s\"), git_path_todo_file());\n+\t\treturn error_errno(_(\"Could not open %s\"),\n+\t\t\t\t git_path_todo_file());\n \tif (strbuf_read(&buf, fd, 0) < 0) {\n \t\tclose(fd);\n \t\tstrbuf_release(&buf);\n-\t\tdie(_(\"Could not read %s.\"), git_path_todo_file());\n+\t\treturn error(_(\"Could not read %s.\"), git_path_todo_file());\n \t}\n \tclose(fd);\n \n \tres = parse_insn_buffer(buf.buf, todo_list, opts);\n \tstrbuf_release(&buf);\n \tif (res)\n-\t\tdie(_(\"Unusable instruction sheet: %s\"), git_path_todo_file());\n+\t\treturn error(_(\"Unusable instruction sheet: %s\"),\n+\t\t\tgit_path_todo_file());\n+\treturn 0;\n }\n \n static int populate_opts_cb(const char *key, const char *value, void *data)\n@@ -1019,7 +1022,8 @@ static int sequencer_continue(struct replay_opts *opts)\n \tif (!file_exists(git_path_todo_file()))\n \t\treturn continue_single_pick();\n \tread_populate_opts(&opts);\n-\tread_populate_todo(&todo_list, opts);\n+\tif (read_populate_todo(&todo_list, opts))\n+\t\treturn -1;\n \n \t/* Verify that the conflict has been resolved */\n \tif (file_exists(git_path_cherry_pick_head()) ||\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"300244","messageId":"729cf9d7cca4ec3fe9caeb6f21a322f3c68ce135.1472219214.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1472219214.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 12/14] sequencer: lib'ify save_head()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-26T13:48:01Z","receivedAt":"2016-08-26T13:48:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain notice\nthe error and handle it (by dying, still).\n\nThe only caller of save_head(), sequencer_pick_revisions() can already\nreturn errors, so its caller must be already prepared to handle error\nreturns, and with this step, we make it notice an error return from\nthis function.\n\nSo this is a safe conversion to make save_head() callable from new\ncallers that want it not to die, without changing the external\nbehaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 9a1f0af..a819919 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -846,18 +846,22 @@ static int create_seq_dir(void)\n \treturn 0;\n }\n \n-static void save_head(const char *head)\n+static int save_head(const char *head)\n {\n \tstatic struct lock_file head_lock;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint fd;\n \n-\tfd = hold_lock_file_for_update(&head_lock, git_path_head_file(), LOCK_DIE_ON_ERROR);\n+\tfd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);\n+\tif (fd < 0)\n+\t\treturn error_errno(_(\"Could not lock HEAD\"));\n \tstrbuf_addf(&buf, \"%s\\n\", head);\n \tif (write_in_full(fd, buf.buf, buf.len) < 0)\n-\t\tdie_errno(_(\"Could not write to %s\"), git_path_head_file());\n+\t\treturn error_errno(_(\"Could not write to %s\"),\n+\t\t\t\t git_path_head_file());\n \tif (commit_lock_file(&head_lock) < 0)\n-\t\tdie(_(\"Error wrapping up %s.\"), git_path_head_file());\n+\t\treturn error(_(\"Error wrapping up %s.\"), git_path_head_file());\n+\treturn 0;\n }\n \n static int reset_for_rollback(const unsigned char *sha1)\n@@ -1121,7 +1125,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t\treturn -1;\n \tif (get_sha1(\"HEAD\", sha1) && (opts->action == REPLAY_REVERT))\n \t\treturn error(_(\"Can't revert as initial commit\"));\n-\tsave_head(sha1_to_hex(sha1));\n+\tif (save_head(sha1_to_hex(sha1)))\n+\t\treturn -1;\n \tsave_opts(opts);\n \treturn pick_commits(todo_list, opts);\n }\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"300245","messageId":"27e7e00297627039d17f6431c58fb1ce3aff08b5.1472219214.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1472219214.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 11/14] sequencer: lib'ify create_seq_dir()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-26T13:47:58Z","receivedAt":"2016-08-26T13:49:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain notice\nthe error and handle it (by dying, still).\n\nThe only caller of create_seq_dir(), sequencer_pick_revisions() can\nalready return errors, so its caller must be already prepared to\nhandle error returns, and with this step, we make it notice an error\nreturn from this function.\n\nSo this is a safe conversion to make create_seq_dir() callable from\nnew callers that want it not to die, without changing the external\nbehaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex be6020a..9a1f0af 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -841,8 +841,8 @@ static int create_seq_dir(void)\n \t\treturn -1;\n \t}\n \telse if (mkdir(git_path_seq_dir(), 0777) < 0)\n-\t\tdie_errno(_(\"Could not create sequencer directory %s\"),\n-\t\t\t git_path_seq_dir());\n+\t\treturn error_errno(_(\"Could not create sequencer directory %s\"),\n+\t\t\t\t git_path_seq_dir());\n \treturn 0;\n }\n \n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"300246","messageId":"0fde7522953f71bd07e6aa7a15081dbf069aa6b6.1472219214.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1472219214.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 10/14] sequencer: lib'ify read_populate_opts()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-26T13:47:56Z","receivedAt":"2016-08-26T13:49:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain notice\nthe error and handle it (by dying, still).\n\nThe only caller of read_populate_opts(), sequencer_continue() can\nalready return errors, so its caller must be already prepared to\nhandle error returns, and with this step, we make it notice an error\nreturn from this function.\n\nSo this is a safe conversion to make read_populate_opts() callable\nfrom new callers that want it not to die, without changing the\nexternal behaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e11b24f..be6020a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -808,12 +808,14 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n \treturn 0;\n }\n \n-static void read_populate_opts(struct replay_opts **opts_ptr)\n+static int read_populate_opts(struct replay_opts **opts)\n {\n \tif (!file_exists(git_path_opts_file()))\n-\t\treturn;\n-\tif (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0)\n-\t\tdie(_(\"Malformed options sheet: %s\"), git_path_opts_file());\n+\t\treturn 0;\n+\tif (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)\n+\t\treturn error(_(\"Malformed options sheet: %s\"),\n+\t\t\tgit_path_opts_file());\n+\treturn 0;\n }\n \n static int walk_revs_populate_todo(struct commit_list **todo_list,\n@@ -1021,8 +1023,8 @@ static int sequencer_continue(struct replay_opts *opts)\n \n \tif (!file_exists(git_path_todo_file()))\n \t\treturn continue_single_pick();\n-\tread_populate_opts(&opts);\n-\tif (read_populate_todo(&todo_list, opts))\n+\tif (read_populate_opts(&opts) ||\n+\t\t\tread_populate_todo(&todo_list, opts))\n \t\treturn -1;\n \n \t/* Verify that the conflict has been resolved */\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"300247","messageId":"2d6d71e2a2d410b12d783f0a8edd22791f303c12.1472219214.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1472219214.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 14/14] sequencer: lib'ify save_opts()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-26T13:48:06Z","receivedAt":"2016-08-26T13:49:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain notice\nthe error and handle it (by dying, still).\n\nThe only caller of save_opts(), sequencer_pick_revisions() can already\nreturn errors, so its caller must be already prepared to handle error\nreturns, and with this step, we make it notice an error return from\nthis function.\n\nSo this is a safe conversion to make save_opts() callable from new\ncallers that want it not to die, without changing the external\nbehaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 26 ++++++++++++++------------\n 1 file changed, 14 insertions(+), 12 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3e877f1..b6481bb 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -958,37 +958,39 @@ static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n \treturn 0;\n }\n \n-static void save_opts(struct replay_opts *opts)\n+static int save_opts(struct replay_opts *opts)\n {\n \tconst char *opts_file = git_path_opts_file();\n+\tint res = 0;\n \n \tif (opts->no_commit)\n-\t\tgit_config_set_in_file(opts_file, \"options.no-commit\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.no-commit\", \"true\");\n \tif (opts->edit)\n-\t\tgit_config_set_in_file(opts_file, \"options.edit\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.edit\", \"true\");\n \tif (opts->signoff)\n-\t\tgit_config_set_in_file(opts_file, \"options.signoff\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.signoff\", \"true\");\n \tif (opts->record_origin)\n-\t\tgit_config_set_in_file(opts_file, \"options.record-origin\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.record-origin\", \"true\");\n \tif (opts->allow_ff)\n-\t\tgit_config_set_in_file(opts_file, \"options.allow-ff\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.allow-ff\", \"true\");\n \tif (opts->mainline) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tstrbuf_addf(&buf, \"%d\", opts->mainline);\n-\t\tgit_config_set_in_file(opts_file, \"options.mainline\", buf.buf);\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.mainline\", buf.buf);\n \t\tstrbuf_release(&buf);\n \t}\n \tif (opts->strategy)\n-\t\tgit_config_set_in_file(opts_file, \"options.strategy\", opts->strategy);\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.strategy\", opts->strategy);\n \tif (opts->gpg_sign)\n-\t\tgit_config_set_in_file(opts_file, \"options.gpg-sign\", opts->gpg_sign);\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.gpg-sign\", opts->gpg_sign);\n \tif (opts->xopts) {\n \t\tint i;\n \t\tfor (i = 0; i < opts->xopts_nr; i++)\n-\t\t\tgit_config_set_multivar_in_file(opts_file,\n+\t\t\tres |= git_config_set_multivar_in_file_gently(opts_file,\n \t\t\t\t\t\t\t\"options.strategy-option\",\n \t\t\t\t\t\t\topts->xopts[i], \"^$\", 0);\n \t}\n+\treturn res;\n }\n \n static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n@@ -1133,9 +1135,9 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t\treturn -1;\n \tif (get_sha1(\"HEAD\", sha1) && (opts->action == REPLAY_REVERT))\n \t\treturn error(_(\"Can't revert as initial commit\"));\n-\tif (save_head(sha1_to_hex(sha1)))\n+\tif (save_head(sha1_to_hex(sha1)) ||\n+\t\t\tsave_opts(opts))\n \t\treturn -1;\n-\tsave_opts(opts);\n \treturn pick_commits(todo_list, opts);\n }\n \n-- \n2.10.0.rc1.99.gcd66998\n"},{"id":"300248","messageId":"28807445076a3d1d5f0077697ef8517f3c8d8c17.1472219214.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1472219214.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 04/14] sequencer: lib'ify do_recursive_merge()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-26T13:47:36Z","receivedAt":"2016-08-26T13:49:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain\nnotice the error and handle it (by dying, still).\n\nThe only caller of do_recursive_merge(), do_pick_commit() already\nchecks the return value and passes it on to its callers, so its caller\nmust be already prepared to handle error returns, and with this step,\nwe make it notice an error return from this function.\n\nSo this is a safe conversion to make do_recursive_merge() callable\nfrom new callers that want it not to die, without changing the\nexternal behaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 17bee60..f6cdf65 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -303,7 +303,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \tif (active_cache_changed &&\n \t write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n \t\t/* TRANSLATORS: %s will be \"revert\" or \"cherry-pick\" */\n-\t\tdie(_(\"%s: Unable to write new index file\"), action_name(opts));\n+\t\treturn error(_(\"%s: Unable to write new index file\"),\n+\t\t\taction_name(opts));\n \trollback_lock_file(&index_lock);\n \n \tif (opts->signoff)\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"300249","messageId":"6cc2e834ae9e595625fbbc6922613994098fd7dc.1472219214.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1472219214.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 03/14] sequencer: lib'ify write_message()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-26T13:47:17Z","receivedAt":"2016-08-26T13:49:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain\nnotice the error and handle it (by dying, still).\n\nThe only caller of write_message(), do_pick_commit() already checks\nthe return value and passes it on to its callers, so its caller must\nbe already prepared to handle error returns, and with this step, we\nmake it notice an error return from this function.\n\nSo this is a safe conversion to make write_message() callable\nfrom new callers that want it not to die, without changing the\nexternal behaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 21 ++++++++++++---------\n 1 file changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex baf6b40..17bee60 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -180,17 +180,20 @@ static void print_advice(int show_hint, struct replay_opts *opts)\n \t}\n }\n \n-static void write_message(struct strbuf *msgbuf, const char *filename)\n+static int write_message(struct strbuf *msgbuf, const char *filename)\n {\n \tstatic struct lock_file msg_file;\n \n-\tint msg_fd = hold_lock_file_for_update(&msg_file, filename,\n-\t\t\t\t\t LOCK_DIE_ON_ERROR);\n+\tint msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);\n+\tif (msg_fd < 0)\n+\t\treturn error_errno(_(\"Could not lock '%s'\"), filename);\n \tif (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)\n-\t\tdie_errno(_(\"Could not write to %s\"), filename);\n+\t\treturn error_errno(_(\"Could not write to %s\"), filename);\n \tstrbuf_release(msgbuf);\n \tif (commit_lock_file(&msg_file) < 0)\n-\t\tdie(_(\"Error wrapping up %s.\"), filename);\n+\t\treturn error(_(\"Error wrapping up %s.\"), filename);\n+\n+\treturn 0;\n }\n \n static struct tree *empty_tree(void)\n@@ -223,7 +226,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \n \tread_cache();\n \tif (checkout_fast_forward(from, to, 1))\n-\t\texit(128); /* the callee should have complained already */\n+\t\treturn -1; /* the callee should have complained already */\n \n \tstrbuf_addf(&sb, _(\"%s: fast-forward\"), action_name(opts));\n \n@@ -564,16 +567,16 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t\t\t\t head, &msgbuf, opts);\n \t\tif (res < 0)\n \t\t\treturn res;\n-\t\twrite_message(&msgbuf, git_path_merge_msg());\n+\t\tres |= write_message(&msgbuf, git_path_merge_msg());\n \t} else {\n \t\tstruct commit_list *common = NULL;\n \t\tstruct commit_list *remotes = NULL;\n \n-\t\twrite_message(&msgbuf, git_path_merge_msg());\n+\t\tres = write_message(&msgbuf, git_path_merge_msg());\n \n \t\tcommit_list_insert(base, &common);\n \t\tcommit_list_insert(next, &remotes);\n-\t\tres = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,\n+\t\tres |= try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,\n \t\t\t\t\tcommon, sha1_to_hex(head), remotes);\n \t\tfree_commit_list(common);\n \t\tfree_commit_list(remotes);\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"300250","messageId":"1cd7ba2d13dff6d1fed3560e2889c438fa468e87.1472219214.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1472219214.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 13/14] sequencer: lib'ify save_todo()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-26T13:48:04Z","receivedAt":"2016-08-26T13:49:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain notice\nthe error and handle it (by dying, still).\n\nThe only caller of save_todo(), pick_commits() can already return\nerrors, so its caller must be already prepared to handle error\nreturns, and with this step, we make it notice an error return from\nthis function.\n\nSo this is a safe conversion to make save_todo() callable\nfrom new callers that want it not to die, without changing the\nexternal behaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 22 +++++++++++++++-------\n 1 file changed, 15 insertions(+), 7 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex a819919..3e877f1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -931,24 +931,31 @@ static int sequencer_rollback(struct replay_opts *opts)\n \treturn -1;\n }\n \n-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n+static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n {\n \tstatic struct lock_file todo_lock;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint fd;\n \n-\tfd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), LOCK_DIE_ON_ERROR);\n-\tif (format_todo(&buf, todo_list, opts) < 0)\n-\t\tdie(_(\"Could not format %s.\"), git_path_todo_file());\n+\tfd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), 0);\n+\tif (fd < 0)\n+\t\treturn error_errno(_(\"Could not lock '%s'\"),\n+\t\t\t\t git_path_todo_file());\n+\tif (format_todo(&buf, todo_list, opts) < 0) {\n+\t\tstrbuf_release(&buf);\n+\t\treturn error(_(\"Could not format %s.\"), git_path_todo_file());\n+\t}\n \tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n \t\tstrbuf_release(&buf);\n-\t\tdie_errno(_(\"Could not write to %s\"), git_path_todo_file());\n+\t\treturn error_errno(_(\"Could not write to %s\"),\n+\t\t\t\t git_path_todo_file());\n \t}\n \tif (commit_lock_file(&todo_lock) < 0) {\n \t\tstrbuf_release(&buf);\n-\t\tdie(_(\"Error wrapping up %s.\"), git_path_todo_file());\n+\t\treturn error(_(\"Error wrapping up %s.\"), git_path_todo_file());\n \t}\n \tstrbuf_release(&buf);\n+\treturn 0;\n }\n \n static void save_opts(struct replay_opts *opts)\n@@ -997,7 +1004,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n \t\treturn -1;\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n-\t\tsave_todo(cur, opts);\n+\t\tif (save_todo(cur, opts))\n+\t\t\treturn -1;\n \t\tres = do_pick_commit(cur->item, opts);\n \t\tif (res)\n \t\t\treturn res;\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"300251","messageId":"752e8fcd89fc08f198f06df145857873dbcdb000.1472219214.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1472219214.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 06/14] sequencer: lib'ify walk_revs_populate_todo()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-26T13:47:42Z","receivedAt":"2016-08-26T13:49:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain notice\nthe error and handle it (by dying, still).\n\nThe function sequencer_pick_revisions() is the only caller of\nwalk_revs_populate_todo(), and it already returns errors\nappropriately, so its caller must be already prepared to handle error\nreturns, and with this step, we make it notice an error return from\nthis function.\n\nSo this is a safe conversion to make walk_revs_populate_todo()\ncallable from new callers that want it not to die, without changing\nthe external behaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7eef512..ea2681e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -809,17 +809,19 @@ static void read_populate_opts(struct replay_opts **opts_ptr)\n \t\tdie(_(\"Malformed options sheet: %s\"), git_path_opts_file());\n }\n \n-static void walk_revs_populate_todo(struct commit_list **todo_list,\n+static int walk_revs_populate_todo(struct commit_list **todo_list,\n \t\t\t\tstruct replay_opts *opts)\n {\n \tstruct commit *commit;\n \tstruct commit_list **next;\n \n-\tprepare_revs(opts);\n+\tif (prepare_revs(opts))\n+\t\treturn -1;\n \n \tnext = todo_list;\n \twhile ((commit = get_revision(opts->revs)))\n \t\tnext = commit_list_append(commit, next);\n+\treturn 0;\n }\n \n static int create_seq_dir(void)\n@@ -1102,8 +1104,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t * progress\n \t */\n \n-\twalk_revs_populate_todo(&todo_list, opts);\n-\tif (create_seq_dir() < 0)\n+\tif (walk_revs_populate_todo(&todo_list, opts) ||\n+\t\t\tcreate_seq_dir() < 0)\n \t\treturn -1;\n \tif (get_sha1(\"HEAD\", sha1) && (opts->action == REPLAY_REVERT))\n \t\treturn error(_(\"Can't revert as initial commit\"));\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"300252","messageId":"1d53ae5e3f4f3d294baa0f9ae96fce1c3516adf3.1472219214.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1472219214.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 07/14] sequencer: lib'ify prepare_revs()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-26T13:47:46Z","receivedAt":"2016-08-26T13:49:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain notice\nthe error and handle it (by dying, still).\n\nThe only caller of prepare_revs(), walk_revs_populate_todo() was just\ntaught to return errors, after verifying that its callers are prepared\nto handle error returns, and with this step, we make it notice an\nerror return from this function.\n\nSo this is a safe conversion to make prepare_revs() callable from new\ncallers that want it not to die, without changing the external\nbehaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex ea2681e..c006cae 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -623,7 +623,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \treturn res;\n }\n \n-static void prepare_revs(struct replay_opts *opts)\n+static int prepare_revs(struct replay_opts *opts)\n {\n \t/*\n \t * picking (but not reverting) ranges (but not individual revisions)\n@@ -633,10 +633,11 @@ static void prepare_revs(struct replay_opts *opts)\n \t\topts->revs->reverse ^= 1;\n \n \tif (prepare_revision_walk(opts->revs))\n-\t\tdie(_(\"revision walk setup failed\"));\n+\t\treturn error(_(\"revision walk setup failed\"));\n \n \tif (!opts->revs->commits)\n-\t\tdie(_(\"empty commit set passed\"));\n+\t\treturn error(_(\"empty commit set passed\"));\n+\treturn 0;\n }\n \n static void read_and_refresh_cache(struct replay_opts *opts)\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"300253","messageId":"99f3ab3a30a469b271300ff61309ac4c4a8af448.1472219214.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1472219214.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 08/14] sequencer: lib'ify read_and_refresh_cache()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-26T13:47:50Z","receivedAt":"2016-08-26T13:49:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain\nnotice the error and handle it (by dying, still).\n\nThere are two call sites of read_and_refresh_cache(), one of which is\npick_commits(), whose callers were already prepared to do the right\nthing given an \"error\" return from it by an earlier patch, so the\nconversion is safe.\n\nThe other one, sequencer_pick_revisions() was also prepared to relay\nan error return back to its caller in all remaining cases in an\nearlier patch.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex c006cae..e30aa82 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -640,18 +640,21 @@ static int prepare_revs(struct replay_opts *opts)\n \treturn 0;\n }\n \n-static void read_and_refresh_cache(struct replay_opts *opts)\n+static int read_and_refresh_cache(struct replay_opts *opts)\n {\n \tstatic struct lock_file index_lock;\n \tint index_fd = hold_locked_index(&index_lock, 0);\n \tif (read_index_preload(&the_index, NULL) < 0)\n-\t\tdie(_(\"git %s: failed to read the index\"), action_name(opts));\n+\t\treturn error(_(\"git %s: failed to read the index\"),\n+\t\t\taction_name(opts));\n \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n \tif (the_index.cache_changed && index_fd >= 0) {\n \t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n-\t\t\tdie(_(\"git %s: failed to refresh the index\"), action_name(opts));\n+\t\t\treturn error(_(\"git %s: failed to refresh the index\"),\n+\t\t\t\taction_name(opts));\n \t}\n \trollback_lock_file(&index_lock);\n+\treturn 0;\n }\n \n static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n@@ -981,7 +984,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n \tif (opts->allow_ff)\n \t\tassert(!(opts->signoff || opts->no_commit ||\n \t\t\t\topts->record_origin || opts->edit));\n-\tread_and_refresh_cache(opts);\n+\tif (read_and_refresh_cache(opts))\n+\t\treturn -1;\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n \t\tsave_todo(cur, opts);\n@@ -1045,7 +1049,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \tif (opts->subcommand == REPLAY_NONE)\n \t\tassert(opts->revs);\n \n-\tread_and_refresh_cache(opts);\n+\tif (read_and_refresh_cache(opts))\n+\t\treturn -1;\n \n \t/*\n \t * Decide what to do depending on the arguments; a fresh\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"300254","messageId":"cover.1472219214.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1471968378.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 00/14] Lib'ify quite a few functions in sequencer.c","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-26T13:47:03Z","receivedAt":"2016-08-26T14:03:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series is one of the half dozen patch series left to move the\nbulk of rebase -i into a builtin.\n\nThe purpose of this patch series is to switch the functions in\nsequencer.c from die()ing to returning errors instead, as proper library\nfunctions should do, to give callers a chance to clean up after an\nerror.\n\nChanges since v1:\n\n- two \"return error()\"s replacing \"die_errno()\"s were turned into \"return\n error_errno()\"s instead.\n\n- an strbuf is now released when format_todo() failed (and may have left\n the strbuf with allocated memory).\n\n- a superfluous space (which was inherited from the previous code) was\n fixed, while at it.\n\n- fixed commit messages to report that callers of the libified functions\n are already libified.\n\n- reordered patches to ensure that callers of libified functions are\n already libified.\n\n\nJohannes Schindelin (14):\n sequencer: lib'ify sequencer_pick_revisions()\n sequencer: do not die() in do_pick_commit()\n sequencer: lib'ify write_message()\n sequencer: lib'ify do_recursive_merge()\n sequencer: lib'ify do_pick_commit()\n sequencer: lib'ify walk_revs_populate_todo()\n sequencer: lib'ify prepare_revs()\n sequencer: lib'ify read_and_refresh_cache()\n sequencer: lib'ify read_populate_todo()\n sequencer: lib'ify read_populate_opts()\n sequencer: lib'ify create_seq_dir()\n sequencer: lib'ify save_head()\n sequencer: lib'ify save_todo()\n sequencer: lib'ify save_opts()\n\n sequencer.c | 172 ++++++++++++++++++++++++++++++++++++------------------------\n 1 file changed, 104 insertions(+), 68 deletions(-)\n\nPublished-As: https://github.com/dscho/git/releases/tag/libify-sequencer-v2\nFetch-It-Via: git fetch https://github.com/dscho/git libify-sequencer-v2\n\nInterdiff vs v1:\n\n diff --git a/sequencer.c b/sequencer.c\n index caba11d..b6481bb 100644\n --- a/sequencer.c\n +++ b/sequencer.c\n @@ -464,7 +464,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t * to work on.\n \t\t */\n \t\tif (write_cache_as_tree(head, 0, NULL))\n -\t\t\treturn error (_(\"Your index file is unmerged.\"));\n +\t\t\treturn error(_(\"Your index file is unmerged.\"));\n \t} else {\n \t\tunborn = get_sha1(\"HEAD\", head);\n \t\tif (unborn)\n @@ -756,8 +756,8 @@ static int read_populate_todo(struct commit_list **todo_list,\n \n \tfd = open(git_path_todo_file(), O_RDONLY);\n \tif (fd < 0)\n -\t\treturn error(_(\"Could not open %s (%s)\"),\n -\t\t\tgit_path_todo_file(), strerror(errno));\n +\t\treturn error_errno(_(\"Could not open %s\"),\n +\t\t\t\t git_path_todo_file());\n \tif (strbuf_read(&buf, fd, 0) < 0) {\n \t\tclose(fd);\n \t\tstrbuf_release(&buf);\n @@ -841,8 +841,8 @@ static int create_seq_dir(void)\n \t\treturn -1;\n \t}\n \telse if (mkdir(git_path_seq_dir(), 0777) < 0)\n -\t\treturn error(_(\"Could not create sequencer directory %s (%s)\"),\n -\t\t\t git_path_seq_dir(), strerror(errno));\n +\t\treturn error_errno(_(\"Could not create sequencer directory %s\"),\n +\t\t\t\t git_path_seq_dir());\n \treturn 0;\n }\n \n @@ -941,8 +941,10 @@ static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n \tif (fd < 0)\n \t\treturn error_errno(_(\"Could not lock '%s'\"),\n \t\t\t\t git_path_todo_file());\n -\tif (format_todo(&buf, todo_list, opts) < 0)\n +\tif (format_todo(&buf, todo_list, opts) < 0) {\n +\t\tstrbuf_release(&buf);\n \t\treturn error(_(\"Could not format %s.\"), git_path_todo_file());\n +\t}\n \tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n \t\tstrbuf_release(&buf);\n \t\treturn error_errno(_(\"Could not write to %s\"),\n\n-- \n2.10.0.rc1.99.gcd66998\n\nbase-commit: 5cb0d5ad05e027cbddcb0a3c7518ddeea0f7c286\n"},{"id":"300255","messageId":"1c74a1ef148a6aa32dc7089b20d6fa2500a0ce39.1472219214.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1472219214.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 01/14] sequencer: lib'ify sequencer_pick_revisions()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-26T13:47:10Z","receivedAt":"2016-08-26T14:03:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain notice\nthe error and handle it (by dying, still).\n\nThe function sequencer_pick_revisions() has only two callers,\ncmd_revert() and cmd_cherry_pick(), both of which check the return\nvalue and react appropriately upon errors.\n\nSo this is a safe conversion to make sequencer_pick_revisions()\ncallable from new callers that want it not to die, without changing\nthe external behaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3804fa9..76b1c52 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1063,10 +1063,11 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t\tif (!get_sha1(name, sha1)) {\n \t\t\tif (!lookup_commit_reference_gently(sha1, 1)) {\n \t\t\t\tenum object_type type = sha1_object_info(sha1, NULL);\n-\t\t\t\tdie(_(\"%s: can't cherry-pick a %s\"), name, typename(type));\n+\t\t\t\treturn error(_(\"%s: can't cherry-pick a %s\"),\n+\t\t\t\t\tname, typename(type));\n \t\t\t}\n \t\t} else\n-\t\t\tdie(_(\"%s: bad revision\"), name);\n+\t\t\treturn error(_(\"%s: bad revision\"), name);\n \t}\n \n \t/*\n@@ -1082,10 +1083,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t !opts->revs->cmdline.rev->flags) {\n \t\tstruct commit *cmit;\n \t\tif (prepare_revision_walk(opts->revs))\n-\t\t\tdie(_(\"revision walk setup failed\"));\n+\t\t\treturn error(_(\"revision walk setup failed\"));\n \t\tcmit = get_revision(opts->revs);\n \t\tif (!cmit || get_revision(opts->revs))\n-\t\t\tdie(\"BUG: expected exactly one commit from walk\");\n+\t\t\treturn error(\"BUG: expected exactly one commit from walk\");\n \t\treturn single_pick(cmit, opts);\n \t}\n \n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"300256","messageId":"00c1114b4717a020ad107a0fcd864bc8eaccbbf9.1472219214.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1472219214.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 02/14] sequencer: do not die() in do_pick_commit()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-26T13:47:14Z","receivedAt":"2016-08-26T14:03:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain\nnotice the error and handle it (by dying, still).\n\nThe eventual caller of do_pick_commit() is sequencer_pick_revisions(),\nwhich already relays a reported error from its helper functions\n(including this one), and both of its two callers know how to react to\na negative return correctly.\n\nSo this makes do_pick_commit() callable from new callers that want it\nnot to die, without changing the external behaviour of anything\nexisting.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 76b1c52..baf6b40 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -585,12 +585,14 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t * However, if the merge did not even start, then we don't want to\n \t * write it at all.\n \t */\n-\tif (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))\n-\t\tupdate_ref(NULL, \"CHERRY_PICK_HEAD\", commit->object.oid.hash, NULL,\n-\t\t\t REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);\n-\tif (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))\n-\t\tupdate_ref(NULL, \"REVERT_HEAD\", commit->object.oid.hash, NULL,\n-\t\t\t REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);\n+\tif (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1) &&\n+\t update_ref(NULL, \"CHERRY_PICK_HEAD\", commit->object.oid.hash, NULL,\n+\t\t REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))\n+\t\tres = -1;\n+\tif (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1) &&\n+\t update_ref(NULL, \"REVERT_HEAD\", commit->object.oid.hash, NULL,\n+\t\t REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))\n+\t\tres = -1;\n \n \tif (res) {\n \t\terror(opts->action == REPLAY_REVERT\n-- \n2.10.0.rc1.99.gcd66998\n\n\n"},{"id":"300264","messageId":"xmqqmvjzii1d.fsf@gitster.mtv.corp.google.com","threadId":"43912","inReplyTo":"alpine.DEB.2.20.1608261540470.4924@virtualbox","subject":"Re: [PATCH 06/15] sequencer: lib'ify read_populate_todo()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-26T16:58:22Z","receivedAt":"2016-08-26T16:58:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> In short: I would really appreciate it if you could cut quoted text after\n> your last response.\n\nI think you are referring to the patch part in this case.\n\nAs I was not making point-by-point comments on the proposed commit\nlog message, quoting only that part and cutting the patch text would\nhave been pointless. I could have cut the proposed log message and\nleft the patch in, though, because the comments were not about what\nwas in the proposed log message, but about what was not in it [*1*].\n\nI left the patch part for other people's use, to make it easy for\nthem to see what I was saying was correct and appropriate for what\nthe patch does. Removing that would not have made much sense.\n\nBut that is only true in this case. I try to see if I can trim\nquote more aggressively, but I would still err on the side of\nover-quoting than under-quoting [*2*].\n\n\n[Footnote]\n\n*1* As to what was IN the proposed log message, I have one comment.\nI do not think \"To be truly useful\" adds any value, as there is\nnothing \"truly\" about what this series does. The original was\n\"truly\" useful for the purpose of the sequencer machinery and its\nuse in the current callers, just like with this series it becomes\n\"truly\" useful for envisioned new callers that want to handle error\nconditions themselves. The change is making it \"more useful\" for\none different use case. It may not be \"truly\" useful for other use\ncases that sequencer machinery could be used even with the\n\"eradicate die and exit\" change and other people may bring a new use\ncase that wants it to be even \"more useful\". The fact that it may\nnot be directly usable by a new use case without further change does\nnot make the result of applying this proposed series less than\n\"truly useful\". The \"truly\" adjective implies absolute, but there\nis nothing absolute in incremental improvements. It is always\nrelative to the context within which the machinery was designed to\nbe used. \"To make it usable for callers that want to handle errors\nthemselves (instead of just dying and the calling process handle\nit), let's turn die's and exit's to returning negative values.\" is\nprobably closer to what I would have expected.\n\n*2* As I read the quoted part before sending my response out, erring\non the side not to underquote tends to avoid a mistake that invites:\n\"I think you misunderstood what I wrote in the part you snipped from\nyour quote; perhaps you skimmed it without fully reading.\"\n"},{"id":"300275","messageId":"xmqq1t1big38.fsf@gitster.mtv.corp.google.com","threadId":"43912","inReplyTo":"85a8ec8273994c599402c380abd383ad2f539777.1471968378.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 07/15] sequencer: lib'ify read_populate_opts()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-26T17:40:27Z","receivedAt":"2016-08-26T17:42:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> -static void read_populate_opts(struct replay_opts **opts_ptr)\n> +static int read_populate_opts(struct replay_opts **opts)\n> {\n> \tif (!file_exists(git_path_opts_file()))\n> -\t\treturn;\n> -\tif (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0)\n> -\t\tdie(_(\"Malformed options sheet: %s\"), git_path_opts_file());\n> +\t\treturn 0;\n> +\tif (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)\n> +\t\treturn error(_(\"Malformed options sheet: %s\"),\n> +\t\t\tgit_path_opts_file());\n> +\treturn 0;\n\nThis may not be sufficient to avoid die(), unless we know that the\nfile we are reading is syntactically sound. git_config_from_file()\nwill die in config.c::git_parse_source() when the config_source sets\ndie_on_error, and it is set in config.c::do_config_from_file().\n\nThe source we are reading from is created when the sequencer\nmachinery starts and is only written by save_opts() which is\ncalled by the sequencer machinery using git_config_set*() calls,\nso I think it is OK to assume that we won't hit errors that would\ncause git_config_from_file() to die, at least for now.\n\n\n"},{"id":"300276","messageId":"xmqqwpj3h1bg.fsf@gitster.mtv.corp.google.com","threadId":"43912","inReplyTo":"7768b55e65c771a5615a6f1209b40395dc705425.1471968378.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 12/15] sequencer: lib'ify save_opts()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-26T17:44:51Z","receivedAt":"2016-08-26T17:45:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n> @@ -1128,9 +1130,9 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n> \t\treturn -1;\n> \tif (get_sha1(\"HEAD\", sha1) && (opts->action == REPLAY_REVERT))\n> \t\treturn error(_(\"Can't revert as initial commit\"));\n> -\tif (save_head(sha1_to_hex(sha1)))\n> +\tif (save_head(sha1_to_hex(sha1)) ||\n> +\t\t\tsave_opts(opts))\n> \t\treturn -1;\n> -\tsave_opts(opts);\n\nI think it is much easier to read to keep this on a single line. It\nwould be more verbose but an even easier would be to keep these two\noperations two separate steps, i.e.\n\n if (save_head())\n return -1;\n if (save_opts())\n return -1;\n"},{"id":"300456","messageId":"alpine.DEB.2.20.1608291405080.129229@virtualbox","threadId":"43912","inReplyTo":"xmqq1t1big38.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 07/15] sequencer: lib'ify read_populate_opts()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-29T12:06:26Z","receivedAt":"2016-08-29T12:06:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 26 Aug 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > -static void read_populate_opts(struct replay_opts **opts_ptr)\n> > +static int read_populate_opts(struct replay_opts **opts)\n> > {\n> > \tif (!file_exists(git_path_opts_file()))\n> > -\t\treturn;\n> > -\tif (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0)\n> > -\t\tdie(_(\"Malformed options sheet: %s\"), git_path_opts_file());\n> > +\t\treturn 0;\n> > +\tif (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)\n> > +\t\treturn error(_(\"Malformed options sheet: %s\"),\n> > +\t\t\tgit_path_opts_file());\n> > +\treturn 0;\n> \n> This may not be sufficient to avoid die(), unless we know that the\n> file we are reading is syntactically sound. git_config_from_file()\n> will die in config.c::git_parse_source() when the config_source sets\n> die_on_error, and it is set in config.c::do_config_from_file().\n> \n> The source we are reading from is created when the sequencer\n> machinery starts and is only written by save_opts() which is\n> called by the sequencer machinery using git_config_set*() calls,\n> so I think it is OK to assume that we won't hit errors that would\n> cause git_config_from_file() to die, at least for now.\n\nI amended the commit message.\n\nCiao,\nDscho\n"},{"id":"300457","messageId":"alpine.DEB.2.20.1608291409210.129229@virtualbox","threadId":"43912","inReplyTo":"xmqqwpj3h1bg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 12/15] sequencer: lib'ify save_opts()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-29T12:09:33Z","receivedAt":"2016-08-29T12:10:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 26 Aug 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n> > @@ -1128,9 +1130,9 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n> > \t\treturn -1;\n> > \tif (get_sha1(\"HEAD\", sha1) && (opts->action == REPLAY_REVERT))\n> > \t\treturn error(_(\"Can't revert as initial commit\"));\n> > -\tif (save_head(sha1_to_hex(sha1)))\n> > +\tif (save_head(sha1_to_hex(sha1)) ||\n> > +\t\t\tsave_opts(opts))\n> > \t\treturn -1;\n> > -\tsave_opts(opts);\n> \n> I think it is much easier to read to keep this on a single line. It\n> would be more verbose but an even easier would be to keep these two\n> operations two separate steps, i.e.\n> \n> if (save_head())\n> return -1;\n> if (save_opts())\n> return -1;\n\nDone,\nDscho\n"},{"id":"300496","messageId":"xmqqfupn9v76.fsf@gitster.mtv.corp.google.com","threadId":"43912","inReplyTo":"6cc2e834ae9e595625fbbc6922613994098fd7dc.1472219214.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 03/14] sequencer: lib'ify write_message()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-29T20:27:57Z","receivedAt":"2016-08-29T20:28:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Instead of dying there, let the caller high up in the callchain\n> notice the error and handle it (by dying, still).\n>\n> The only caller of write_message(), do_pick_commit() already checks\n> the return value and passes it on to its callers, so its caller must\n> be already prepared to handle error returns, and with this step, we\n> make it notice an error return from this function.\n> ...\n> @@ -223,7 +226,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n> \n> \tread_cache();\n> \tif (checkout_fast_forward(from, to, 1))\n> -\t\texit(128); /* the callee should have complained already */\n> +\t\treturn -1; /* the callee should have complained already */\n\nThis hunk does not seem to have anything to do with write_message()\nconversion, other than that its only caller, do_pick_commit(), also\ncalls write_message(). checkout_fast_forward() itself can die when\nit cannot write the index, though, so this hunk may deserve to be in\nits own patch that teaches checkout_fast_forward() to instead return\nan error if/when its caller desires.\n\nWith the updated message, the series has become far easier to review,\nand the reordering them to sequencer-pick-revisions at the beginning\nmakes quite a lot of sense.\n"},{"id":"300498","messageId":"xmqqbn0b9uyx.fsf@gitster.mtv.corp.google.com","threadId":"43912","inReplyTo":"39d03662fc6ea81d737c0650bb9b7292e812ae02.1472219214.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 05/14] sequencer: lib'ify do_pick_commit()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-29T20:32:54Z","receivedAt":"2016-08-29T20:33:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Instead of dying there, let the caller high up in the callchain notice\n> the error and handle it (by dying, still).\n>\n> The only two callers of do_pick_commit(), pick_commits() and\n> single_pick() already check the return value and pass it on to their\n> callers, so their callers must be already prepared to handle error\n> returns, and with this step, we make it notice an error return from\n> this function.\n>\n> So this is a safe conversion to make do_pick_commit() callable from\n> new callers that want it not to die, without changing the external\n> behaviour of anything existing.\n>\n> While at it, remove the superfluous space.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> sequencer.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index f6cdf65..7eef512 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -464,7 +464,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n> \t\t * to work on.\n> \t\t */\n> \t\tif (write_cache_as_tree(head, 0, NULL))\n> -\t\t\tdie (_(\"Your index file is unmerged.\"));\n> +\t\t\treturn error(_(\"Your index file is unmerged.\"));\n\nMakes sense.\n\ndo_pick_commit() still calls fast_forward_to(), which was silently\nhalf libified in 3/14 but can still die in checkout_fast_forward(),\nthough.\n"},{"id":"300499","messageId":"xmqq7faz9ufi.fsf@gitster.mtv.corp.google.com","threadId":"43912","inReplyTo":"99f3ab3a30a469b271300ff61309ac4c4a8af448.1472219214.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 08/14] sequencer: lib'ify read_and_refresh_cache()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-29T20:44:33Z","receivedAt":"2016-08-29T20:44:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Instead of dying there, let the caller high up in the callchain\n> notice the error and handle it (by dying, still).\n>\n> There are two call sites of read_and_refresh_cache(), one of which is\n> pick_commits(), whose callers were already prepared to do the right\n> thing given an \"error\" return from it by an earlier patch, so the\n> conversion is safe.\n>\n> The other one, sequencer_pick_revisions() was also prepared to relay\n> an error return back to its caller in all remaining cases in an\n> earlier patch.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> sequencer.c | 15 ++++++++++-----\n> 1 file changed, 10 insertions(+), 5 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index c006cae..e30aa82 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -640,18 +640,21 @@ static int prepare_revs(struct replay_opts *opts)\n> \treturn 0;\n> }\n> \n> -static void read_and_refresh_cache(struct replay_opts *opts)\n> +static int read_and_refresh_cache(struct replay_opts *opts)\n> {\n> \tstatic struct lock_file index_lock;\n> \tint index_fd = hold_locked_index(&index_lock, 0);\n> \tif (read_index_preload(&the_index, NULL) < 0)\n> -\t\tdie(_(\"git %s: failed to read the index\"), action_name(opts));\n> +\t\treturn error(_(\"git %s: failed to read the index\"),\n> +\t\t\taction_name(opts));\n> \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n> \tif (the_index.cache_changed && index_fd >= 0) {\n> \t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n> -\t\t\tdie(_(\"git %s: failed to refresh the index\"), action_name(opts));\n> +\t\t\treturn error(_(\"git %s: failed to refresh the index\"),\n> +\t\t\t\taction_name(opts));\n> \t}\n> \trollback_lock_file(&index_lock);\n> +\treturn 0;\n> }\n\nWith the current set of callers, a caller that notices an error from\nthis function will immediately exit without doing any further\ndamage.\n\nSo in that sense, this is a \"safe\" conversion.\n\nBut is it a sensible conversion? When the caller wants to do\nanything else (e.g. clean-up and try something else, perhaps read\nthe index again), the caller can't, as the index is still locked,\nbecause even though the code knows that the lock will not be\nreleased until the process exit, it chose to return error without\nreleasing the lock.\n\nFor a file-scope static helper, that probably is sufficient. But if\nthis can be reached from a public entry point in the API, the caller\nof that entry point will find this not-so-useful, I would think.\n\nI suspect doing the \"right thing\" to future-proof it may not be too\nmuch more work.\n\n\tstatic int read_and_refresh_cache(struct replay_opts *opts)\n {\n+ \tint retval = 0; /* assume success */\n\t\t...\n if (read_idnex_preload(...) < 0) {\n\t\t\tretval = error(...);\n goto finish;\n\t\t}\n refresh_index(...);\n if (...changed...) {\n \tif (write_locked_index(...))\n \tretval = error(...);\n\t\t}\n+\tfinish:\n rollback_lock_file(&index_lock);\n return retval;\n\t}\n\nor something like that on top?\n"},{"id":"300500","messageId":"xmqq37ln9ubo.fsf@gitster.mtv.corp.google.com","threadId":"43912","inReplyTo":"0fde7522953f71bd07e6aa7a15081dbf069aa6b6.1472219214.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 10/14] sequencer: lib'ify read_populate_opts()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-29T20:46:51Z","receivedAt":"2016-08-29T20:46:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Instead of dying there, let the caller high up in the callchain notice\n> the error and handle it (by dying, still).\n>\n> The only caller of read_populate_opts(), sequencer_continue() can\n> already return errors, so its caller must be already prepared to\n> handle error returns, and with this step, we make it notice an error\n> return from this function.\n>\n> So this is a safe conversion to make read_populate_opts() callable\n> from new callers that want it not to die, without changing the\n> external behaviour of anything existing.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> sequencer.c | 14 ++++++++------\n> 1 file changed, 8 insertions(+), 6 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index e11b24f..be6020a 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -808,12 +808,14 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n> \treturn 0;\n> }\n> \n> -static void read_populate_opts(struct replay_opts **opts_ptr)\n> +static int read_populate_opts(struct replay_opts **opts)\n> {\n> \tif (!file_exists(git_path_opts_file()))\n> -\t\treturn;\n> -\tif (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0)\n> -\t\tdie(_(\"Malformed options sheet: %s\"), git_path_opts_file());\n> +\t\treturn 0;\n> +\tif (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)\n> +\t\treturn error(_(\"Malformed options sheet: %s\"),\n> +\t\t\tgit_path_opts_file());\n> +\treturn 0;\n\nAs discussed, perhaps have a comment immediately before calling\nconfig-from-file that says that the call could die when it is fed a\nsyntactically broken file, but we ignore it for now because we will\nbe writing the file we have written, or something?\n\n> }\n> \n> static int walk_revs_populate_todo(struct commit_list **todo_list,\n> @@ -1021,8 +1023,8 @@ static int sequencer_continue(struct replay_opts *opts)\n> \n> \tif (!file_exists(git_path_todo_file()))\n> \t\treturn continue_single_pick();\n> -\tread_populate_opts(&opts);\n> -\tif (read_populate_todo(&todo_list, opts))\n> +\tif (read_populate_opts(&opts) ||\n> +\t\t\tread_populate_todo(&todo_list, opts))\n> \t\treturn -1;\n> \n> \t/* Verify that the conflict has been resolved */\n"},{"id":"300501","messageId":"xmqqtwe38fjj.fsf@gitster.mtv.corp.google.com","threadId":"43912","inReplyTo":"cover.1472219214.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 00/14] Lib'ify quite a few functions in sequencer.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-29T20:51:28Z","receivedAt":"2016-08-29T20:51:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> This patch series is one of the half dozen patch series left to move the\n> bulk of rebase -i into a builtin.\n\nThis was a lot easier to understand compared to the previous round,\nand overall looked alright.\n\nThanks.\n"},{"id":"300502","messageId":"xmqqy43f8fnc.fsf@gitster.mtv.corp.google.com","threadId":"43912","inReplyTo":"729cf9d7cca4ec3fe9caeb6f21a322f3c68ce135.1472219214.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 12/14] sequencer: lib'ify save_head()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-29T20:49:11Z","receivedAt":"2016-08-29T20:55:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> \tstrbuf_addf(&buf, \"%s\\n\", head);\n> \tif (write_in_full(fd, buf.buf, buf.len) < 0)\n> -\t\tdie_errno(_(\"Could not write to %s\"), git_path_head_file());\n> +\t\treturn error_errno(_(\"Could not write to %s\"),\n> +\t\t\t\t git_path_head_file());\n\nSame comment around a left-over lockfile applies to this. An extra\nrollback being minimally intrusive also applies here, I think.\n\n> \tif (commit_lock_file(&head_lock) < 0)\n> -\t\tdie(_(\"Error wrapping up %s.\"), git_path_head_file());\n> +\t\treturn error(_(\"Error wrapping up %s.\"), git_path_head_file());\n> +\treturn 0;\n> }\n"},{"id":"300509","messageId":"xmqqh9a38eh7.fsf@gitster.mtv.corp.google.com","threadId":"43912","inReplyTo":"xmqq37ln9ubo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 10/14] sequencer: lib'ify read_populate_opts()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-29T21:14:28Z","receivedAt":"2016-08-29T21:14:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +\tif (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)\n>> +\t\treturn error(_(\"Malformed options sheet: %s\"),\n>> +\t\t\tgit_path_opts_file());\n>> +\treturn 0;\n>\n> As discussed, perhaps have a comment immediately before calling\n> config-from-file that says that the call could die when it is fed a\n> syntactically broken file, but we ignore it for now because we will\n> be writing the file we have written, or something?\n\nObviously we will be _READING_ here ;-)\n"},{"id":"300542","messageId":"alpine.DEB.2.20.1608300947540.129229@virtualbox","threadId":"43912","inReplyTo":"xmqqfupn9v76.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 03/14] sequencer: lib'ify write_message()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-30T07:49:26Z","receivedAt":"2016-08-30T07:49:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 29 Aug 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Instead of dying there, let the caller high up in the callchain\n> > notice the error and handle it (by dying, still).\n> >\n> > The only caller of write_message(), do_pick_commit() already checks\n> > the return value and passes it on to its callers, so its caller must\n> > be already prepared to handle error returns, and with this step, we\n> > make it notice an error return from this function.\n> > ...\n> > @@ -223,7 +226,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n> > \n> > \tread_cache();\n> > \tif (checkout_fast_forward(from, to, 1))\n> > -\t\texit(128); /* the callee should have complained already */\n> > +\t\treturn -1; /* the callee should have complained already */\n> \n> This hunk does not seem to have anything to do with write_message()\n> conversion, other than that its only caller, do_pick_commit(), also\n> calls write_message().\n\nDarn. Sorry that this slipped through. I split it out into its own commit,\nand fixed checkout_fast_forward_to() in yet another commit.\n\nCiao,\nDscho\n"},{"id":"300547","messageId":"alpine.DEB.2.20.1608301106220.129229@virtualbox","threadId":"43912","inReplyTo":"xmqq7faz9ufi.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 08/14] sequencer: lib'ify read_and_refresh_cache()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-30T09:09:31Z","receivedAt":"2016-08-30T10:21:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 29 Aug 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Instead of dying there, let the caller high up in the callchain\n> > notice the error and handle it (by dying, still).\n> >\n> > There are two call sites of read_and_refresh_cache(), one of which is\n> > pick_commits(), whose callers were already prepared to do the right\n> > thing given an \"error\" return from it by an earlier patch, so the\n> > conversion is safe.\n> >\n> > The other one, sequencer_pick_revisions() was also prepared to relay\n> > an error return back to its caller in all remaining cases in an\n> > earlier patch.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > sequencer.c | 15 ++++++++++-----\n> > 1 file changed, 10 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/sequencer.c b/sequencer.c\n> > index c006cae..e30aa82 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -640,18 +640,21 @@ static int prepare_revs(struct replay_opts *opts)\n> > \treturn 0;\n> > }\n> > \n> > -static void read_and_refresh_cache(struct replay_opts *opts)\n> > +static int read_and_refresh_cache(struct replay_opts *opts)\n> > {\n> > \tstatic struct lock_file index_lock;\n> > \tint index_fd = hold_locked_index(&index_lock, 0);\n> > \tif (read_index_preload(&the_index, NULL) < 0)\n> > -\t\tdie(_(\"git %s: failed to read the index\"), action_name(opts));\n> > +\t\treturn error(_(\"git %s: failed to read the index\"),\n> > +\t\t\taction_name(opts));\n> > \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n> > \tif (the_index.cache_changed && index_fd >= 0) {\n> > \t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n> > -\t\t\tdie(_(\"git %s: failed to refresh the index\"), action_name(opts));\n> > +\t\t\treturn error(_(\"git %s: failed to refresh the index\"),\n> > +\t\t\t\taction_name(opts));\n> > \t}\n> > \trollback_lock_file(&index_lock);\n> > +\treturn 0;\n> > }\n> \n> With the current set of callers, a caller that notices an error from\n> this function will immediately exit without doing any further\n> damage.\n> \n> So in that sense, this is a \"safe\" conversion.\n> \n> But is it a sensible conversion? When the caller wants to do\n> anything else (e.g. clean-up and try something else, perhaps read\n> the index again), the caller can't, as the index is still locked,\n> because even though the code knows that the lock will not be\n> released until the process exit, it chose to return error without\n> releasing the lock.\n\nIt depends what the caller wants to do. The case about which I care most\nis when some helpful advice should be printed (see e.g. 3be18b4 (t5520:\nverify that `pull --rebase` shows the helpful advice when failing,\n2016-07-26)). Those callers do not need to care, as the atexit() handler\nwill clean up the lock file.\n\nHowever, I am sympathetic to your angle, even if I do not expect any such\ncaller to arise anytime soon.\n\n> For a file-scope static helper, that probably is sufficient. But if\n> this can be reached from a public entry point in the API, the caller\n> of that entry point will find this not-so-useful, I would think.\n> \n> I suspect doing the \"right thing\" to future-proof it may not be too\n> much more work.\n> \n> \tstatic int read_and_refresh_cache(struct replay_opts *opts)\n> {\n> + \tint retval = 0; /* assume success */\n> \t\t...\n> if (read_idnex_preload(...) < 0) {\n> \t\t\tretval = error(...);\n> goto finish;\n> \t\t}\n> refresh_index(...);\n> if (...changed...) {\n> \tif (write_locked_index(...))\n> \tretval = error(...);\n> \t\t}\n> +\tfinish:\n> rollback_lock_file(&index_lock);\n> return retval;\n> \t}\n> \n> or something like that on top?\n\nI settled for rolling back in the if() clauses.\n\nCiao,\nDscho\n"},{"id":"300548","messageId":"alpine.DEB.2.20.1608301116450.129229@virtualbox","threadId":"43912","inReplyTo":"xmqq37ln9ubo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 10/14] sequencer: lib'ify read_populate_opts()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-30T09:17:38Z","receivedAt":"2016-08-30T10:29:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 29 Aug 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Instead of dying there, let the caller high up in the callchain notice\n> > the error and handle it (by dying, still).\n> >\n> > The only caller of read_populate_opts(), sequencer_continue() can\n> > already return errors, so its caller must be already prepared to\n> > handle error returns, and with this step, we make it notice an error\n> > return from this function.\n> >\n> > So this is a safe conversion to make read_populate_opts() callable\n> > from new callers that want it not to die, without changing the\n> > external behaviour of anything existing.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > sequencer.c | 14 ++++++++------\n> > 1 file changed, 8 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/sequencer.c b/sequencer.c\n> > index e11b24f..be6020a 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -808,12 +808,14 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n> > \treturn 0;\n> > }\n> > \n> > -static void read_populate_opts(struct replay_opts **opts_ptr)\n> > +static int read_populate_opts(struct replay_opts **opts)\n> > {\n> > \tif (!file_exists(git_path_opts_file()))\n> > -\t\treturn;\n> > -\tif (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0)\n> > -\t\tdie(_(\"Malformed options sheet: %s\"), git_path_opts_file());\n> > +\t\treturn 0;\n> > +\tif (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)\n> > +\t\treturn error(_(\"Malformed options sheet: %s\"),\n> > +\t\t\tgit_path_opts_file());\n> > +\treturn 0;\n> \n> As discussed, perhaps have a comment immediately before calling\n> config-from-file that says that the call could die when it is fed a\n> syntactically broken file, but we ignore it for now because we will\n> be writing the file we have written, or something?\n\nSure. I added a code comment.\n\nI still think that it is a serious mistake for library functions to die().\nBut I have no time to take care of git_parse_source() right now.\n\nCiao,\nDscho\n"},{"id":"300549","messageId":"alpine.DEB.2.20.1608301120510.129229@virtualbox","threadId":"43912","inReplyTo":"xmqqy43f8fnc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 12/14] sequencer: lib'ify save_head()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-30T09:21:09Z","receivedAt":"2016-08-30T10:32:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 29 Aug 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > \tstrbuf_addf(&buf, \"%s\\n\", head);\n> > \tif (write_in_full(fd, buf.buf, buf.len) < 0)\n> > -\t\tdie_errno(_(\"Could not write to %s\"), git_path_head_file());\n> > +\t\treturn error_errno(_(\"Could not write to %s\"),\n> > +\t\t\t\t git_path_head_file());\n> \n> Same comment around a left-over lockfile applies to this. An extra\n> rollback being minimally intrusive also applies here, I think.\n\nSure. I added rollbacks in case of failure.\n\nCiao,\nDscho\n"},{"id":"300575","messageId":"xmqqoa4a5g24.fsf@gitster.mtv.corp.google.com","threadId":"43912","inReplyTo":"alpine.DEB.2.20.1608301106220.129229@virtualbox","subject":"Re: [PATCH v2 08/14] sequencer: lib'ify read_and_refresh_cache()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-30T17:20:51Z","receivedAt":"2016-08-30T17:21:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> With the current set of callers, a caller that notices an error from\n>> this function will immediately exit without doing any further\n>> damage.\n>> \n>> So in that sense, this is a \"safe\" conversion.\n>> \n>> But is it a sensible conversion? When the caller wants to do\n>> anything else (e.g. clean-up and try something else, perhaps read\n>> the index again), the caller can't, as the index is still locked,\n>> because even though the code knows that the lock will not be\n>> released until the process exit, it chose to return error without\n>> releasing the lock.\n>\n> It depends what the caller wants to do. The case about which I care most\n> is when some helpful advice should be printed (see e.g. 3be18b4 (t5520:\n> verify that `pull --rebase` shows the helpful advice when failing,\n> 2016-07-26)). Those callers do not need to care, as the atexit() handler\n> will clean up the lock file.\n>\n> However, I am sympathetic to your angle, even if I do not expect any such\n> caller to arise anytime soon.\n\nWe just fixed a similar \"why are we allowing the 'if the_index\nhasn't been read, read unconditionally from $GIT_INDEX_FILE\" that is\nreached by a codepath that is specifically designed to read from a\ntemporary index file while reviewing a separate topic, and that is\nwhere my reaction \"this is not very helpful for other callers\" comes\nfrom.\n"},{"id":"300576","messageId":"xmqqk2ey5g0m.fsf@gitster.mtv.corp.google.com","threadId":"43912","inReplyTo":"alpine.DEB.2.20.1608301116450.129229@virtualbox","subject":"Re: [PATCH v2 10/14] sequencer: lib'ify read_populate_opts()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-30T17:21:45Z","receivedAt":"2016-08-30T17:21:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I still think that it is a serious mistake for library functions to die().\n> But I have no time to take care of git_parse_source() right now.\n\nI think we already agreed on both points during the previous round\nof review ;-)\n"},{"id":"301495","messageId":"cover.1473431645.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1472219214.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 00/17] Lib'ify quite a few functions in sequencer.c","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T14:35:32Z","receivedAt":"2016-09-09T14:35:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series is one of the half dozen patch series left to move the\nbulk of rebase -i into a builtin.\n\nThe purpose of this patch series is to switch the functions in\nsequencer.c from die()ing to returning errors instead, as proper library\nfunctions should do, to give callers a chance to clean up after an\nerror.\n\nChanges since v2:\n\n- the commit message of the read_populate_opts() patch now clarifies why\n we do not take care of git_config_from_file() possibly die()ing.\n\n- the save_head() and the save_opts() conditionals are now separate, for\n improved readability.\n\n- the fast_forward_to() libification now happens in its own commit.\n\n- checkout_fast_forward_to() is now libified, too.\n\n- added a code comment to clarify why we don't care about\n git_parse_source() being able to die().\n\n- added rollbacks in case of failure in read_and_refresh_cache() and\n save_head().\n\n\nJohannes Schindelin (17):\n sequencer: lib'ify sequencer_pick_revisions()\n sequencer: do not die() in do_pick_commit()\n sequencer: lib'ify write_message()\n sequencer: lib'ify do_recursive_merge()\n sequencer: lib'ify do_pick_commit()\n sequencer: lib'ify walk_revs_populate_todo()\n sequencer: lib'ify prepare_revs()\n sequencer: lib'ify read_and_refresh_cache()\n sequencer: lib'ify read_populate_todo()\n sequencer: lib'ify read_populate_opts()\n sequencer: lib'ify create_seq_dir()\n sequencer: lib'ify save_head()\n sequencer: lib'ify save_todo()\n sequencer: lib'ify save_opts()\n sequencer: lib'ify fast_forward_to()\n lib'ify checkout_fast_forward_to()\n sequencer: ensure to release the lock when we could not read the index\n\n merge.c | 9 ++-\n sequencer.c | 197 ++++++++++++++++++++++++++++++++++++++----------------------\n 2 files changed, 131 insertions(+), 75 deletions(-)\n\nPublished-As: https://github.com/dscho/git/releases/tag/libify-sequencer-v3\nFetch-It-Via: git fetch https://github.com/dscho/git libify-sequencer-v3\n\nInterdiff vs v2:\n\n diff --git a/merge.c b/merge.c\n index 5db7d56..23866c9 100644\n --- a/merge.c\n +++ b/merge.c\n @@ -57,7 +57,8 @@ int checkout_fast_forward(const unsigned char *head,\n \n \trefresh_cache(REFRESH_QUIET);\n \n -\thold_locked_index(lock_file, 1);\n +\tif (hold_locked_index(lock_file, 0) < 0)\n +\t\treturn -1;\n \n \tmemset(&trees, 0, sizeof(trees));\n \tmemset(&opts, 0, sizeof(opts));\n @@ -90,7 +91,9 @@ int checkout_fast_forward(const unsigned char *head,\n \t}\n \tif (unpack_trees(nr_trees, t, &opts))\n \t\treturn -1;\n -\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n -\t\tdie(_(\"unable to write new index file\"));\n +\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) {\n +\t\trollback_lock_file(lock_file);\n +\t\treturn error(_(\"unable to write new index file\"));\n +\t}\n \treturn 0;\n }\n diff --git a/sequencer.c b/sequencer.c\n index b6481bb..eec8a60 100644\n --- a/sequencer.c\n +++ b/sequencer.c\n @@ -644,14 +644,18 @@ static int read_and_refresh_cache(struct replay_opts *opts)\n {\n \tstatic struct lock_file index_lock;\n \tint index_fd = hold_locked_index(&index_lock, 0);\n -\tif (read_index_preload(&the_index, NULL) < 0)\n +\tif (read_index_preload(&the_index, NULL) < 0) {\n +\t\trollback_lock_file(&index_lock);\n \t\treturn error(_(\"git %s: failed to read the index\"),\n \t\t\taction_name(opts));\n +\t}\n \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n \tif (the_index.cache_changed && index_fd >= 0) {\n -\t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n +\t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {\n +\t\t\trollback_lock_file(&index_lock);\n \t\t\treturn error(_(\"git %s: failed to refresh the index\"),\n \t\t\t\taction_name(opts));\n +\t\t}\n \t}\n \trollback_lock_file(&index_lock);\n \treturn 0;\n @@ -812,6 +816,12 @@ static int read_populate_opts(struct replay_opts **opts)\n {\n \tif (!file_exists(git_path_opts_file()))\n \t\treturn 0;\n +\t/*\n +\t * The function git_parse_source(), called from git_config_from_file(),\n +\t * may die() in case of a syntactically incorrect file. We do not care\n +\t * about this case, though, because we wrote that file ourselves, so we\n +\t * are pretty certain that it is syntactically correct.\n +\t */\n \tif (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)\n \t\treturn error(_(\"Malformed options sheet: %s\"),\n \t\t\tgit_path_opts_file());\n @@ -853,14 +863,20 @@ static int save_head(const char *head)\n \tint fd;\n \n \tfd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);\n -\tif (fd < 0)\n +\tif (fd < 0) {\n +\t\trollback_lock_file(&head_lock);\n \t\treturn error_errno(_(\"Could not lock HEAD\"));\n +\t}\n \tstrbuf_addf(&buf, \"%s\\n\", head);\n -\tif (write_in_full(fd, buf.buf, buf.len) < 0)\n +\tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n +\t\trollback_lock_file(&head_lock);\n \t\treturn error_errno(_(\"Could not write to %s\"),\n \t\t\t\t git_path_head_file());\n -\tif (commit_lock_file(&head_lock) < 0)\n +\t}\n +\tif (commit_lock_file(&head_lock) < 0) {\n +\t\trollback_lock_file(&head_lock);\n \t\treturn error(_(\"Error wrapping up %s.\"), git_path_head_file());\n +\t}\n \treturn 0;\n }\n \n @@ -1135,8 +1151,9 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t\treturn -1;\n \tif (get_sha1(\"HEAD\", sha1) && (opts->action == REPLAY_REVERT))\n \t\treturn error(_(\"Can't revert as initial commit\"));\n -\tif (save_head(sha1_to_hex(sha1)) ||\n -\t\t\tsave_opts(opts))\n +\tif (save_head(sha1_to_hex(sha1)))\n +\t\treturn -1;\n +\tif (save_opts(opts))\n \t\treturn -1;\n \treturn pick_commits(todo_list, opts);\n }\n\n-- \n2.10.0.windows.1.10.g803177d\n\nbase-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b\n"},{"id":"301496","messageId":"2646059f7b3e7366744106380466f1b8904b4e85.1473431645.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1473431645.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 01/17] sequencer: lib'ify sequencer_pick_revisions()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T14:35:37Z","receivedAt":"2016-09-09T14:36:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain notice\nthe error and handle it (by dying, still).\n\nThe function sequencer_pick_revisions() has only two callers,\ncmd_revert() and cmd_cherry_pick(), both of which check the return\nvalue and react appropriately upon errors.\n\nSo this is a safe conversion to make sequencer_pick_revisions()\ncallable from new callers that want it not to die, without changing\nthe external behaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3804fa9..76b1c52 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1063,10 +1063,11 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t\tif (!get_sha1(name, sha1)) {\n \t\t\tif (!lookup_commit_reference_gently(sha1, 1)) {\n \t\t\t\tenum object_type type = sha1_object_info(sha1, NULL);\n-\t\t\t\tdie(_(\"%s: can't cherry-pick a %s\"), name, typename(type));\n+\t\t\t\treturn error(_(\"%s: can't cherry-pick a %s\"),\n+\t\t\t\t\tname, typename(type));\n \t\t\t}\n \t\t} else\n-\t\t\tdie(_(\"%s: bad revision\"), name);\n+\t\t\treturn error(_(\"%s: bad revision\"), name);\n \t}\n \n \t/*\n@@ -1082,10 +1083,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t !opts->revs->cmdline.rev->flags) {\n \t\tstruct commit *cmit;\n \t\tif (prepare_revision_walk(opts->revs))\n-\t\t\tdie(_(\"revision walk setup failed\"));\n+\t\t\treturn error(_(\"revision walk setup failed\"));\n \t\tcmit = get_revision(opts->revs);\n \t\tif (!cmit || get_revision(opts->revs))\n-\t\t\tdie(\"BUG: expected exactly one commit from walk\");\n+\t\t\treturn error(\"BUG: expected exactly one commit from walk\");\n \t\treturn single_pick(cmit, opts);\n \t}\n \n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301497","messageId":"5a090b8f817bbe5a9b32b3af556416b2e341d221.1473431645.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1473431645.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 02/17] sequencer: do not die() in do_pick_commit()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T14:37:02Z","receivedAt":"2016-09-09T14:37:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain\nnotice the error and handle it (by dying, still).\n\nThe eventual caller of do_pick_commit() is sequencer_pick_revisions(),\nwhich already relays a reported error from its helper functions\n(including this one), and both of its two callers know how to react to\na negative return correctly.\n\nSo this makes do_pick_commit() callable from new callers that want it\nnot to die, without changing the external behaviour of anything\nexisting.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 76b1c52..baf6b40 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -585,12 +585,14 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t * However, if the merge did not even start, then we don't want to\n \t * write it at all.\n \t */\n-\tif (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))\n-\t\tupdate_ref(NULL, \"CHERRY_PICK_HEAD\", commit->object.oid.hash, NULL,\n-\t\t\t REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);\n-\tif (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))\n-\t\tupdate_ref(NULL, \"REVERT_HEAD\", commit->object.oid.hash, NULL,\n-\t\t\t REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);\n+\tif (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1) &&\n+\t update_ref(NULL, \"CHERRY_PICK_HEAD\", commit->object.oid.hash, NULL,\n+\t\t REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))\n+\t\tres = -1;\n+\tif (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1) &&\n+\t update_ref(NULL, \"REVERT_HEAD\", commit->object.oid.hash, NULL,\n+\t\t REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))\n+\t\tres = -1;\n \n \tif (res) {\n \t\terror(opts->action == REPLAY_REVERT\n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301498","messageId":"6d105cab003f2a28108557e889ddbbea70b1c453.1473431645.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1473431645.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 04/17] sequencer: lib'ify do_recursive_merge()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T14:37:10Z","receivedAt":"2016-09-09T14:37:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain\nnotice the error and handle it (by dying, still).\n\nThe only caller of do_recursive_merge(), do_pick_commit() already\nchecks the return value and passes it on to its callers, so its caller\nmust be already prepared to handle error returns, and with this step,\nwe make it notice an error return from this function.\n\nSo this is a safe conversion to make do_recursive_merge() callable\nfrom new callers that want it not to die, without changing the\nexternal behaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex ec85fe7..eb70091 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -303,7 +303,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \tif (active_cache_changed &&\n \t write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n \t\t/* TRANSLATORS: %s will be \"revert\" or \"cherry-pick\" */\n-\t\tdie(_(\"%s: Unable to write new index file\"), action_name(opts));\n+\t\treturn error(_(\"%s: Unable to write new index file\"),\n+\t\t\taction_name(opts));\n \trollback_lock_file(&index_lock);\n \n \tif (opts->signoff)\n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301499","messageId":"bc896e5c4611422d4d269c960dc3a33282de688e.1473431645.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1473431645.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 05/17] sequencer: lib'ify do_pick_commit()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T14:37:12Z","receivedAt":"2016-09-09T14:37:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain notice\nthe error and handle it (by dying, still).\n\nThe only two callers of do_pick_commit(), pick_commits() and\nsingle_pick() already check the return value and pass it on to their\ncallers, so their callers must be already prepared to handle error\nreturns, and with this step, we make it notice an error return from\nthis function.\n\nSo this is a safe conversion to make do_pick_commit() callable from\nnew callers that want it not to die, without changing the external\nbehaviour of anything existing.\n\nWhile at it, remove the superfluous space.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex eb70091..96b9ae1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -464,7 +464,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t * to work on.\n \t\t */\n \t\tif (write_cache_as_tree(head, 0, NULL))\n-\t\t\tdie (_(\"Your index file is unmerged.\"));\n+\t\t\treturn error(_(\"Your index file is unmerged.\"));\n \t} else {\n \t\tunborn = get_sha1(\"HEAD\", head);\n \t\tif (unborn)\n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301500","messageId":"70b3840e62c8eaf2e2af829b65c746971694bec0.1473431645.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1473431645.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 06/17] sequencer: lib'ify walk_revs_populate_todo()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T14:37:15Z","receivedAt":"2016-09-09T14:37:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain notice\nthe error and handle it (by dying, still).\n\nThe function sequencer_pick_revisions() is the only caller of\nwalk_revs_populate_todo(), and it already returns errors\nappropriately, so its caller must be already prepared to handle error\nreturns, and with this step, we make it notice an error return from\nthis function.\n\nSo this is a safe conversion to make walk_revs_populate_todo()\ncallable from new callers that want it not to die, without changing\nthe external behaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 96b9ae1..ab599e0 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -809,17 +809,19 @@ static void read_populate_opts(struct replay_opts **opts_ptr)\n \t\tdie(_(\"Malformed options sheet: %s\"), git_path_opts_file());\n }\n \n-static void walk_revs_populate_todo(struct commit_list **todo_list,\n+static int walk_revs_populate_todo(struct commit_list **todo_list,\n \t\t\t\tstruct replay_opts *opts)\n {\n \tstruct commit *commit;\n \tstruct commit_list **next;\n \n-\tprepare_revs(opts);\n+\tif (prepare_revs(opts))\n+\t\treturn -1;\n \n \tnext = todo_list;\n \twhile ((commit = get_revision(opts->revs)))\n \t\tnext = commit_list_append(commit, next);\n+\treturn 0;\n }\n \n static int create_seq_dir(void)\n@@ -1102,8 +1104,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t * progress\n \t */\n \n-\twalk_revs_populate_todo(&todo_list, opts);\n-\tif (create_seq_dir() < 0)\n+\tif (walk_revs_populate_todo(&todo_list, opts) ||\n+\t\t\tcreate_seq_dir() < 0)\n \t\treturn -1;\n \tif (get_sha1(\"HEAD\", sha1) && (opts->action == REPLAY_REVERT))\n \t\treturn error(_(\"Can't revert as initial commit\"));\n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301501","messageId":"107e669eb6996b8bcfed4177241550da778c6b73.1473431645.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1473431645.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 09/17] sequencer: lib'ify read_populate_todo()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T14:37:24Z","receivedAt":"2016-09-09T14:37:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain\nnotice the error and handle it (by dying, still).\n\nThe only caller of read_populate_todo(), sequencer_continue() can\nalready return errors, so its caller must be already prepared to\nhandle error returns, and with this step, we make it notice an\nerror return from this function.\n\nSo this is a safe conversion to make read_populate_todo() callable\nfrom new callers that want it not to die, without changing the\nexternal behaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 631b75d..c73cdfd 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -748,7 +748,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,\n \treturn 0;\n }\n \n-static void read_populate_todo(struct commit_list **todo_list,\n+static int read_populate_todo(struct commit_list **todo_list,\n \t\t\tstruct replay_opts *opts)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -756,18 +756,21 @@ static void read_populate_todo(struct commit_list **todo_list,\n \n \tfd = open(git_path_todo_file(), O_RDONLY);\n \tif (fd < 0)\n-\t\tdie_errno(_(\"Could not open %s\"), git_path_todo_file());\n+\t\treturn error_errno(_(\"Could not open %s\"),\n+\t\t\t\t git_path_todo_file());\n \tif (strbuf_read(&buf, fd, 0) < 0) {\n \t\tclose(fd);\n \t\tstrbuf_release(&buf);\n-\t\tdie(_(\"Could not read %s.\"), git_path_todo_file());\n+\t\treturn error(_(\"Could not read %s.\"), git_path_todo_file());\n \t}\n \tclose(fd);\n \n \tres = parse_insn_buffer(buf.buf, todo_list, opts);\n \tstrbuf_release(&buf);\n \tif (res)\n-\t\tdie(_(\"Unusable instruction sheet: %s\"), git_path_todo_file());\n+\t\treturn error(_(\"Unusable instruction sheet: %s\"),\n+\t\t\tgit_path_todo_file());\n+\treturn 0;\n }\n \n static int populate_opts_cb(const char *key, const char *value, void *data)\n@@ -1019,7 +1022,8 @@ static int sequencer_continue(struct replay_opts *opts)\n \tif (!file_exists(git_path_todo_file()))\n \t\treturn continue_single_pick();\n \tread_populate_opts(&opts);\n-\tread_populate_todo(&todo_list, opts);\n+\tif (read_populate_todo(&todo_list, opts))\n+\t\treturn -1;\n \n \t/* Verify that the conflict has been resolved */\n \tif (file_exists(git_path_cherry_pick_head()) ||\n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301502","messageId":"cfe6288ab534c859d385e3abec64ca06bf5e35f1.1473431645.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1473431645.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 08/17] sequencer: lib'ify read_and_refresh_cache()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T14:37:21Z","receivedAt":"2016-09-09T14:37:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain\nnotice the error and handle it (by dying, still).\n\nThere are two call sites of read_and_refresh_cache(), one of which is\npick_commits(), whose callers were already prepared to do the right\nthing given an \"error\" return from it by an earlier patch, so the\nconversion is safe.\n\nThe other one, sequencer_pick_revisions() was also prepared to relay\nan error return back to its caller in all remaining cases in an\nearlier patch.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7fd0f99..631b75d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -640,18 +640,21 @@ static int prepare_revs(struct replay_opts *opts)\n \treturn 0;\n }\n \n-static void read_and_refresh_cache(struct replay_opts *opts)\n+static int read_and_refresh_cache(struct replay_opts *opts)\n {\n \tstatic struct lock_file index_lock;\n \tint index_fd = hold_locked_index(&index_lock, 0);\n \tif (read_index_preload(&the_index, NULL) < 0)\n-\t\tdie(_(\"git %s: failed to read the index\"), action_name(opts));\n+\t\treturn error(_(\"git %s: failed to read the index\"),\n+\t\t\taction_name(opts));\n \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n \tif (the_index.cache_changed && index_fd >= 0) {\n \t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n-\t\t\tdie(_(\"git %s: failed to refresh the index\"), action_name(opts));\n+\t\t\treturn error(_(\"git %s: failed to refresh the index\"),\n+\t\t\t\taction_name(opts));\n \t}\n \trollback_lock_file(&index_lock);\n+\treturn 0;\n }\n \n static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n@@ -981,7 +984,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n \tif (opts->allow_ff)\n \t\tassert(!(opts->signoff || opts->no_commit ||\n \t\t\t\topts->record_origin || opts->edit));\n-\tread_and_refresh_cache(opts);\n+\tif (read_and_refresh_cache(opts))\n+\t\treturn -1;\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n \t\tsave_todo(cur, opts);\n@@ -1045,7 +1049,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \tif (opts->subcommand == REPLAY_NONE)\n \t\tassert(opts->revs);\n \n-\tread_and_refresh_cache(opts);\n+\tif (read_and_refresh_cache(opts))\n+\t\treturn -1;\n \n \t/*\n \t * Decide what to do depending on the arguments; a fresh\n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301503","messageId":"cdd0b6551b11171b4173ce4252078aa806f4a942.1473431645.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1473431645.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 03/17] sequencer: lib'ify write_message()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T14:37:05Z","receivedAt":"2016-09-09T14:38:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain\nnotice the error and handle it (by dying, still).\n\nThe only caller of write_message(), do_pick_commit() already checks\nthe return value and passes it on to its callers, so its caller must\nbe already prepared to handle error returns, and with this step, we\nmake it notice an error return from this function.\n\nSo this is a safe conversion to make write_message() callable\nfrom new callers that want it not to die, without changing the\nexternal behaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex baf6b40..ec85fe7 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -180,17 +180,20 @@ static void print_advice(int show_hint, struct replay_opts *opts)\n \t}\n }\n \n-static void write_message(struct strbuf *msgbuf, const char *filename)\n+static int write_message(struct strbuf *msgbuf, const char *filename)\n {\n \tstatic struct lock_file msg_file;\n \n-\tint msg_fd = hold_lock_file_for_update(&msg_file, filename,\n-\t\t\t\t\t LOCK_DIE_ON_ERROR);\n+\tint msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);\n+\tif (msg_fd < 0)\n+\t\treturn error_errno(_(\"Could not lock '%s'\"), filename);\n \tif (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)\n-\t\tdie_errno(_(\"Could not write to %s\"), filename);\n+\t\treturn error_errno(_(\"Could not write to %s\"), filename);\n \tstrbuf_release(msgbuf);\n \tif (commit_lock_file(&msg_file) < 0)\n-\t\tdie(_(\"Error wrapping up %s.\"), filename);\n+\t\treturn error(_(\"Error wrapping up %s.\"), filename);\n+\n+\treturn 0;\n }\n \n static struct tree *empty_tree(void)\n@@ -564,16 +567,16 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t\t\t\t head, &msgbuf, opts);\n \t\tif (res < 0)\n \t\t\treturn res;\n-\t\twrite_message(&msgbuf, git_path_merge_msg());\n+\t\tres |= write_message(&msgbuf, git_path_merge_msg());\n \t} else {\n \t\tstruct commit_list *common = NULL;\n \t\tstruct commit_list *remotes = NULL;\n \n-\t\twrite_message(&msgbuf, git_path_merge_msg());\n+\t\tres = write_message(&msgbuf, git_path_merge_msg());\n \n \t\tcommit_list_insert(base, &common);\n \t\tcommit_list_insert(next, &remotes);\n-\t\tres = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,\n+\t\tres |= try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,\n \t\t\t\t\tcommon, sha1_to_hex(head), remotes);\n \t\tfree_commit_list(common);\n \t\tfree_commit_list(remotes);\n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301504","messageId":"b06592dcf50d808728ed7f6d1041ff45a5b6d6ec.1473431645.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1473431645.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 12/17] sequencer: lib'ify save_head()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T14:37:47Z","receivedAt":"2016-09-09T14:38:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain notice\nthe error and handle it (by dying, still).\n\nThe only caller of save_head(), sequencer_pick_revisions() can already\nreturn errors, so its caller must be already prepared to handle error\nreturns, and with this step, we make it notice an error return from\nthis function.\n\nSo this is a safe conversion to make save_head() callable from new\ncallers that want it not to die, without changing the external\nbehaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 25 ++++++++++++++++++-------\n 1 file changed, 18 insertions(+), 7 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex eb9c473..7a1561e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -852,18 +852,28 @@ static int create_seq_dir(void)\n \treturn 0;\n }\n \n-static void save_head(const char *head)\n+static int save_head(const char *head)\n {\n \tstatic struct lock_file head_lock;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint fd;\n \n-\tfd = hold_lock_file_for_update(&head_lock, git_path_head_file(), LOCK_DIE_ON_ERROR);\n+\tfd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);\n+\tif (fd < 0) {\n+\t\trollback_lock_file(&head_lock);\n+\t\treturn error_errno(_(\"Could not lock HEAD\"));\n+\t}\n \tstrbuf_addf(&buf, \"%s\\n\", head);\n-\tif (write_in_full(fd, buf.buf, buf.len) < 0)\n-\t\tdie_errno(_(\"Could not write to %s\"), git_path_head_file());\n-\tif (commit_lock_file(&head_lock) < 0)\n-\t\tdie(_(\"Error wrapping up %s.\"), git_path_head_file());\n+\tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n+\t\trollback_lock_file(&head_lock);\n+\t\treturn error_errno(_(\"Could not write to %s\"),\n+\t\t\t\t git_path_head_file());\n+\t}\n+\tif (commit_lock_file(&head_lock) < 0) {\n+\t\trollback_lock_file(&head_lock);\n+\t\treturn error(_(\"Error wrapping up %s.\"), git_path_head_file());\n+\t}\n+\treturn 0;\n }\n \n static int reset_for_rollback(const unsigned char *sha1)\n@@ -1127,7 +1137,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t\treturn -1;\n \tif (get_sha1(\"HEAD\", sha1) && (opts->action == REPLAY_REVERT))\n \t\treturn error(_(\"Can't revert as initial commit\"));\n-\tsave_head(sha1_to_hex(sha1));\n+\tif (save_head(sha1_to_hex(sha1)))\n+\t\treturn -1;\n \tsave_opts(opts);\n \treturn pick_commits(todo_list, opts);\n }\n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301505","messageId":"77e4701a438aa266ea09e10df0820a8a8bb2feea.1473431645.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1473431645.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 10/17] sequencer: lib'ify read_populate_opts()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T14:37:27Z","receivedAt":"2016-09-09T14:38:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain notice\nthe error and handle it (by dying, still).\n\nThe only caller of read_populate_opts(), sequencer_continue() can\nalready return errors, so its caller must be already prepared to\nhandle error returns, and with this step, we make it notice an error\nreturn from this function.\n\nSo this is a safe conversion to make read_populate_opts() callable\nfrom new callers that want it not to die, without changing the\nexternal behaviour of anything existing.\n\nNote that the function git_config_from_file(), called from\nread_populate_opts(), can currently still die() (in git_parse_source(),\nbecause the do_config_from_file() function sets die_on_error = 1). We do\nnot try to fix that here, as it would have larger ramifications on the\nconfig code, and we also assume that we write the opts file\nprogrammatically, hence any parse errors would be bugs.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 20 ++++++++++++++------\n 1 file changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex c73cdfd..1614efb 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -808,12 +808,20 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n \treturn 0;\n }\n \n-static void read_populate_opts(struct replay_opts **opts_ptr)\n+static int read_populate_opts(struct replay_opts **opts)\n {\n \tif (!file_exists(git_path_opts_file()))\n-\t\treturn;\n-\tif (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0)\n-\t\tdie(_(\"Malformed options sheet: %s\"), git_path_opts_file());\n+\t\treturn 0;\n+\t/*\n+\t * The function git_parse_source(), called from git_config_from_file(),\n+\t * may die() in case of a syntactically incorrect file. We do not care\n+\t * about this case, though, because we wrote that file ourselves, so we\n+\t * are pretty certain that it is syntactically correct.\n+\t */\n+\tif (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)\n+\t\treturn error(_(\"Malformed options sheet: %s\"),\n+\t\t\tgit_path_opts_file());\n+\treturn 0;\n }\n \n static int walk_revs_populate_todo(struct commit_list **todo_list,\n@@ -1021,8 +1029,8 @@ static int sequencer_continue(struct replay_opts *opts)\n \n \tif (!file_exists(git_path_todo_file()))\n \t\treturn continue_single_pick();\n-\tread_populate_opts(&opts);\n-\tif (read_populate_todo(&todo_list, opts))\n+\tif (read_populate_opts(&opts) ||\n+\t\t\tread_populate_todo(&todo_list, opts))\n \t\treturn -1;\n \n \t/* Verify that the conflict has been resolved */\n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301506","messageId":"7a6dc6abaf9ef3ea204b63c542b3f5f89d292b91.1473431645.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1473431645.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 11/17] sequencer: lib'ify create_seq_dir()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T14:37:44Z","receivedAt":"2016-09-09T14:38:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain notice\nthe error and handle it (by dying, still).\n\nThe only caller of create_seq_dir(), sequencer_pick_revisions() can\nalready return errors, so its caller must be already prepared to\nhandle error returns, and with this step, we make it notice an error\nreturn from this function.\n\nSo this is a safe conversion to make create_seq_dir() callable from\nnew callers that want it not to die, without changing the external\nbehaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 1614efb..eb9c473 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -847,8 +847,8 @@ static int create_seq_dir(void)\n \t\treturn -1;\n \t}\n \telse if (mkdir(git_path_seq_dir(), 0777) < 0)\n-\t\tdie_errno(_(\"Could not create sequencer directory %s\"),\n-\t\t\t git_path_seq_dir());\n+\t\treturn error_errno(_(\"Could not create sequencer directory %s\"),\n+\t\t\t\t git_path_seq_dir());\n \treturn 0;\n }\n \n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301507","messageId":"15198727511147562a86610ae8894284a13b652c.1473431645.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1473431645.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 14/17] sequencer: lib'ify save_opts()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T14:37:53Z","receivedAt":"2016-09-09T14:38:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain notice\nthe error and handle it (by dying, still).\n\nThe only caller of save_opts(), sequencer_pick_revisions() can already\nreturn errors, so its caller must be already prepared to handle error\nreturns, and with this step, we make it notice an error return from\nthis function.\n\nSo this is a safe conversion to make save_opts() callable from new\ncallers that want it not to die, without changing the external\nbehaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 25 ++++++++++++++-----------\n 1 file changed, 14 insertions(+), 11 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 32c53bb..021ddf3 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -970,37 +970,39 @@ static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n \treturn 0;\n }\n \n-static void save_opts(struct replay_opts *opts)\n+static int save_opts(struct replay_opts *opts)\n {\n \tconst char *opts_file = git_path_opts_file();\n+\tint res = 0;\n \n \tif (opts->no_commit)\n-\t\tgit_config_set_in_file(opts_file, \"options.no-commit\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.no-commit\", \"true\");\n \tif (opts->edit)\n-\t\tgit_config_set_in_file(opts_file, \"options.edit\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.edit\", \"true\");\n \tif (opts->signoff)\n-\t\tgit_config_set_in_file(opts_file, \"options.signoff\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.signoff\", \"true\");\n \tif (opts->record_origin)\n-\t\tgit_config_set_in_file(opts_file, \"options.record-origin\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.record-origin\", \"true\");\n \tif (opts->allow_ff)\n-\t\tgit_config_set_in_file(opts_file, \"options.allow-ff\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.allow-ff\", \"true\");\n \tif (opts->mainline) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tstrbuf_addf(&buf, \"%d\", opts->mainline);\n-\t\tgit_config_set_in_file(opts_file, \"options.mainline\", buf.buf);\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.mainline\", buf.buf);\n \t\tstrbuf_release(&buf);\n \t}\n \tif (opts->strategy)\n-\t\tgit_config_set_in_file(opts_file, \"options.strategy\", opts->strategy);\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.strategy\", opts->strategy);\n \tif (opts->gpg_sign)\n-\t\tgit_config_set_in_file(opts_file, \"options.gpg-sign\", opts->gpg_sign);\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.gpg-sign\", opts->gpg_sign);\n \tif (opts->xopts) {\n \t\tint i;\n \t\tfor (i = 0; i < opts->xopts_nr; i++)\n-\t\t\tgit_config_set_multivar_in_file(opts_file,\n+\t\t\tres |= git_config_set_multivar_in_file_gently(opts_file,\n \t\t\t\t\t\t\t\"options.strategy-option\",\n \t\t\t\t\t\t\topts->xopts[i], \"^$\", 0);\n \t}\n+\treturn res;\n }\n \n static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n@@ -1147,7 +1149,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t\treturn error(_(\"Can't revert as initial commit\"));\n \tif (save_head(sha1_to_hex(sha1)))\n \t\treturn -1;\n-\tsave_opts(opts);\n+\tif (save_opts(opts))\n+\t\treturn -1;\n \treturn pick_commits(todo_list, opts);\n }\n \n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301508","messageId":"f624be21280cb2f7624ed7bb4d890903cccf1c7d.1473431645.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1473431645.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 16/17] lib'ify checkout_fast_forward_to()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T14:38:00Z","receivedAt":"2016-09-09T14:38:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain\nnotice the error and handle it (by dying, still).\n\nThe only callers of checkout_fast_forward_to(), cmd_merge(),\npull_into_void(), cmd_pull() and sequencer's fast_forward_to(),\nalready check the return value and handle it appropriately. With this\nstep, we make it notice an error return from this function.\n\nSo this is a safe conversion to make checkout_fast_forward_to()\ncallable from new callers that want it not to die, without changing\nthe external behaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n merge.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/merge.c b/merge.c\nindex 5db7d56..23866c9 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -57,7 +57,8 @@ int checkout_fast_forward(const unsigned char *head,\n \n \trefresh_cache(REFRESH_QUIET);\n \n-\thold_locked_index(lock_file, 1);\n+\tif (hold_locked_index(lock_file, 0) < 0)\n+\t\treturn -1;\n \n \tmemset(&trees, 0, sizeof(trees));\n \tmemset(&opts, 0, sizeof(opts));\n@@ -90,7 +91,9 @@ int checkout_fast_forward(const unsigned char *head,\n \t}\n \tif (unpack_trees(nr_trees, t, &opts))\n \t\treturn -1;\n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n-\t\tdie(_(\"unable to write new index file\"));\n+\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) {\n+\t\trollback_lock_file(lock_file);\n+\t\treturn error(_(\"unable to write new index file\"));\n+\t}\n \treturn 0;\n }\n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301509","messageId":"4ea0065f8f064f56840b446c30758b9873479ae0.1473431645.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1473431645.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 13/17] sequencer: lib'ify save_todo()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T14:37:50Z","receivedAt":"2016-09-09T14:38:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain notice\nthe error and handle it (by dying, still).\n\nThe only caller of save_todo(), pick_commits() can already return\nerrors, so its caller must be already prepared to handle error\nreturns, and with this step, we make it notice an error return from\nthis function.\n\nSo this is a safe conversion to make save_todo() callable\nfrom new callers that want it not to die, without changing the\nexternal behaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 22 +++++++++++++++-------\n 1 file changed, 15 insertions(+), 7 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7a1561e..32c53bb 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -943,24 +943,31 @@ static int sequencer_rollback(struct replay_opts *opts)\n \treturn -1;\n }\n \n-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n+static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n {\n \tstatic struct lock_file todo_lock;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint fd;\n \n-\tfd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), LOCK_DIE_ON_ERROR);\n-\tif (format_todo(&buf, todo_list, opts) < 0)\n-\t\tdie(_(\"Could not format %s.\"), git_path_todo_file());\n+\tfd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), 0);\n+\tif (fd < 0)\n+\t\treturn error_errno(_(\"Could not lock '%s'\"),\n+\t\t\t\t git_path_todo_file());\n+\tif (format_todo(&buf, todo_list, opts) < 0) {\n+\t\tstrbuf_release(&buf);\n+\t\treturn error(_(\"Could not format %s.\"), git_path_todo_file());\n+\t}\n \tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n \t\tstrbuf_release(&buf);\n-\t\tdie_errno(_(\"Could not write to %s\"), git_path_todo_file());\n+\t\treturn error_errno(_(\"Could not write to %s\"),\n+\t\t\t\t git_path_todo_file());\n \t}\n \tif (commit_lock_file(&todo_lock) < 0) {\n \t\tstrbuf_release(&buf);\n-\t\tdie(_(\"Error wrapping up %s.\"), git_path_todo_file());\n+\t\treturn error(_(\"Error wrapping up %s.\"), git_path_todo_file());\n \t}\n \tstrbuf_release(&buf);\n+\treturn 0;\n }\n \n static void save_opts(struct replay_opts *opts)\n@@ -1009,7 +1016,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n \t\treturn -1;\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n-\t\tsave_todo(cur, opts);\n+\t\tif (save_todo(cur, opts))\n+\t\t\treturn -1;\n \t\tres = do_pick_commit(cur->item, opts);\n \t\tif (res)\n \t\t\treturn res;\n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301510","messageId":"5da26b12bcc1a118c32c52f452aaac879f848543.1473431645.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1473431645.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 15/17] sequencer: lib'ify fast_forward_to()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T14:37:55Z","receivedAt":"2016-09-09T14:38:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain\nnotice the error and handle it (by dying, still).\n\nThe only caller of fast_forward_to(), do_pick_commit() already checks\nthe return value and passes it on to its callers, so its caller must\nbe already prepared to handle error returns, and with this step, we\nmake it notice an error return from this function.\n\nSo this is a safe conversion to make fast_forward_to() callable from\nnew callers that want it not to die, without changing the external\nbehaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 021ddf3..d92a632 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -226,7 +226,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \n \tread_cache();\n \tif (checkout_fast_forward(from, to, 1))\n-\t\texit(128); /* the callee should have complained already */\n+\t\treturn -1; /* the callee should have complained already */\n \n \tstrbuf_addf(&sb, _(\"%s: fast-forward\"), action_name(opts));\n \n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301511","messageId":"c9f22511aabf3df7287b9279869691448e3305ed.1473431645.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1473431645.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 07/17] sequencer: lib'ify prepare_revs()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T14:37:18Z","receivedAt":"2016-09-09T14:38:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of dying there, let the caller high up in the callchain notice\nthe error and handle it (by dying, still).\n\nThe only caller of prepare_revs(), walk_revs_populate_todo() was just\ntaught to return errors, after verifying that its callers are prepared\nto handle error returns, and with this step, we make it notice an\nerror return from this function.\n\nSo this is a safe conversion to make prepare_revs() callable from new\ncallers that want it not to die, without changing the external\nbehaviour of anything existing.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex ab599e0..7fd0f99 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -623,7 +623,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \treturn res;\n }\n \n-static void prepare_revs(struct replay_opts *opts)\n+static int prepare_revs(struct replay_opts *opts)\n {\n \t/*\n \t * picking (but not reverting) ranges (but not individual revisions)\n@@ -633,10 +633,11 @@ static void prepare_revs(struct replay_opts *opts)\n \t\topts->revs->reverse ^= 1;\n \n \tif (prepare_revision_walk(opts->revs))\n-\t\tdie(_(\"revision walk setup failed\"));\n+\t\treturn error(_(\"revision walk setup failed\"));\n \n \tif (!opts->revs->commits)\n-\t\tdie(_(\"empty commit set passed\"));\n+\t\treturn error(_(\"empty commit set passed\"));\n+\treturn 0;\n }\n \n static void read_and_refresh_cache(struct replay_opts *opts)\n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301512","messageId":"4c7876d88eac80d9cf05847477d4d468a7c01acc.1473431645.git.johannes.schindelin@gmx.de","threadId":"43912","inReplyTo":"cover.1473431645.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 17/17] sequencer: ensure to release the lock when we could not read the index","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T14:38:20Z","receivedAt":"2016-09-09T14:38:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"A future caller of read_and_refresh_cache() may want to do more than just\nprint some helpful advice in case of failure.\n\nSuggested by Junio Hamano.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex d92a632..eec8a60 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -644,14 +644,18 @@ static int read_and_refresh_cache(struct replay_opts *opts)\n {\n \tstatic struct lock_file index_lock;\n \tint index_fd = hold_locked_index(&index_lock, 0);\n-\tif (read_index_preload(&the_index, NULL) < 0)\n+\tif (read_index_preload(&the_index, NULL) < 0) {\n+\t\trollback_lock_file(&index_lock);\n \t\treturn error(_(\"git %s: failed to read the index\"),\n \t\t\taction_name(opts));\n+\t}\n \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n \tif (the_index.cache_changed && index_fd >= 0) {\n-\t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n+\t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {\n+\t\t\trollback_lock_file(&index_lock);\n \t\t\treturn error(_(\"git %s: failed to refresh the index\"),\n \t\t\t\taction_name(opts));\n+\t\t}\n \t}\n \trollback_lock_file(&index_lock);\n \treturn 0;\n-- \n2.10.0.windows.1.10.g803177d\n"},{"id":"301537","messageId":"xmqqzinhaq5p.fsf@gitster.mtv.corp.google.com","threadId":"43912","inReplyTo":"f624be21280cb2f7624ed7bb4d890903cccf1c7d.1473431645.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 16/17] lib'ify checkout_fast_forward_to()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-09T18:23:14Z","receivedAt":"2016-09-09T18:23:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Instead of dying there, let the caller high up in the callchain\n> notice the error and handle it (by dying, still).\n>\n> The only callers of checkout_fast_forward_to(), cmd_merge(),\n> pull_into_void(), cmd_pull() and sequencer's fast_forward_to(),\n> already check the return value and handle it appropriately. With this\n> step, we make it notice an error return from this function.\n>\n> So this is a safe conversion to make checkout_fast_forward_to()\n> callable from new callers that want it not to die, without changing\n> the external behaviour of anything existing.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n\nI'll retitle this to\n\n sequencer: lib'ify chckout_fast_forward()\n\nand checkout_fast_forward_to() in the second paragraph to match the\nreality. Other than that, the above analysis matches what I see in\nthe code and the libification done here looks correct.\n\nThanks.\n\n> merge.c | 9 ++++++---\n> 1 file changed, 6 insertions(+), 3 deletions(-)\n>\n> diff --git a/merge.c b/merge.c\n> index 5db7d56..23866c9 100644\n> --- a/merge.c\n> +++ b/merge.c\n> @@ -57,7 +57,8 @@ int checkout_fast_forward(const unsigned char *head,\n> \n> \trefresh_cache(REFRESH_QUIET);\n> \n> -\thold_locked_index(lock_file, 1);\n> +\tif (hold_locked_index(lock_file, 0) < 0)\n> +\t\treturn -1;\n> \n> \tmemset(&trees, 0, sizeof(trees));\n> \tmemset(&opts, 0, sizeof(opts));\n> @@ -90,7 +91,9 @@ int checkout_fast_forward(const unsigned char *head,\n> \t}\n> \tif (unpack_trees(nr_trees, t, &opts))\n> \t\treturn -1;\n> -\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n> -\t\tdie(_(\"unable to write new index file\"));\n> +\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) {\n> +\t\trollback_lock_file(lock_file);\n> +\t\treturn error(_(\"unable to write new index file\"));\n> +\t}\n> \treturn 0;\n> }\n"},{"id":"301538","messageId":"xmqqvay5aq03.fsf@gitster.mtv.corp.google.com","threadId":"43912","inReplyTo":"4c7876d88eac80d9cf05847477d4d468a7c01acc.1473431645.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 17/17] sequencer: ensure to release the lock when we could not read the index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-09T18:26:36Z","receivedAt":"2016-09-09T18:26:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> A future caller of read_and_refresh_cache() may want to do more than just\n> print some helpful advice in case of failure.\n\nI recall commenting on unreleased locks on other parts of the series\nbut didn't see this. Looks good. Thanks for being thorough.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index d92a632..eec8a60 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -644,14 +644,18 @@ static int read_and_refresh_cache(struct replay_opts *opts)\n> {\n> \tstatic struct lock_file index_lock;\n> \tint index_fd = hold_locked_index(&index_lock, 0);\n> -\tif (read_index_preload(&the_index, NULL) < 0)\n> +\tif (read_index_preload(&the_index, NULL) < 0) {\n> +\t\trollback_lock_file(&index_lock);\n> \t\treturn error(_(\"git %s: failed to read the index\"),\n> \t\t\taction_name(opts));\n> +\t}\n> \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n> \tif (the_index.cache_changed && index_fd >= 0) {\n> -\t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n> +\t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {\n> +\t\t\trollback_lock_file(&index_lock);\n> \t\t\treturn error(_(\"git %s: failed to refresh the index\"),\n> \t\t\t\taction_name(opts));\n> +\t\t}\n> \t}\n> \trollback_lock_file(&index_lock);\n> \treturn 0;\n"},{"id":"301622","messageId":"alpine.DEB.2.20.1609111008370.129229@virtualbox","threadId":"43912","inReplyTo":"xmqqzinhaq5p.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 16/17] lib'ify checkout_fast_forward_to()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-11T08:09:42Z","receivedAt":"2016-09-11T08:10:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 9 Sep 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Instead of dying there, let the caller high up in the callchain\n> > notice the error and handle it (by dying, still).\n> >\n> > The only callers of checkout_fast_forward_to(), cmd_merge(),\n> > pull_into_void(), cmd_pull() and sequencer's fast_forward_to(),\n> > already check the return value and handle it appropriately. With this\n> > step, we make it notice an error return from this function.\n> >\n> > So this is a safe conversion to make checkout_fast_forward_to()\n> > callable from new callers that want it not to die, without changing\n> > the external behaviour of anything existing.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> \n> I'll retitle this to\n> \n> sequencer: lib'ify chckout_fast_forward()\n> \n> and checkout_fast_forward_to() in the second paragraph to match the\n> reality. Other than that, the above analysis matches what I see in\n> the code and the libification done here looks correct.\n\nGood catch. Please also fix it in the third paragraph. (I changed it also\nlocally, in case a v4 is required.)\n\nCiao,\nDscho\n"}]}