From 45d6b95807b3642d9e1565d2cb5ba1b4fa241a46 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 26 Aug 2016 15:47:10 +0200 Subject: [PATCH 01/44] sequencer: lib'ify sequencer_pick_revisions() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The function sequencer_pick_revisions() has only two callers, cmd_revert() and cmd_cherry_pick(), both of which check the return value and react appropriately upon errors. So this is a safe conversion to make sequencer_pick_revisions() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/sequencer.c b/sequencer.c index 3804fa931d..76b1c52f75 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1063,10 +1063,11 @@ int sequencer_pick_revisions(struct replay_opts *opts) if (!get_sha1(name, sha1)) { if (!lookup_commit_reference_gently(sha1, 1)) { enum object_type type = sha1_object_info(sha1, NULL); - die(_("%s: can't cherry-pick a %s"), name, typename(type)); + return error(_("%s: can't cherry-pick a %s"), + name, typename(type)); } } else - die(_("%s: bad revision"), name); + return error(_("%s: bad revision"), name); } /* @@ -1082,10 +1083,10 @@ int sequencer_pick_revisions(struct replay_opts *opts) !opts->revs->cmdline.rev->flags) { struct commit *cmit; if (prepare_revision_walk(opts->revs)) - die(_("revision walk setup failed")); + return error(_("revision walk setup failed")); cmit = get_revision(opts->revs); if (!cmit || get_revision(opts->revs)) - die("BUG: expected exactly one commit from walk"); + return error("BUG: expected exactly one commit from walk"); return single_pick(cmit, opts); } From 810a8b1306f87e5e717e57ab72a3a31997201806 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 26 Aug 2016 15:47:14 +0200 Subject: [PATCH 02/44] sequencer: do not die() in do_pick_commit() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The eventual caller of do_pick_commit() is sequencer_pick_revisions(), which already relays a reported error from its helper functions (including this one), and both of its two callers know how to react to a negative return correctly. So this makes do_pick_commit() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/sequencer.c b/sequencer.c index 76b1c52f75..baf6b40e7a 100644 --- a/sequencer.c +++ b/sequencer.c @@ -585,12 +585,14 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) * However, if the merge did not even start, then we don't want to * write it at all. */ - if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1)) - update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.oid.hash, NULL, - REF_NODEREF, UPDATE_REFS_DIE_ON_ERR); - if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1)) - update_ref(NULL, "REVERT_HEAD", commit->object.oid.hash, NULL, - REF_NODEREF, UPDATE_REFS_DIE_ON_ERR); + if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1) && + update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.oid.hash, NULL, + REF_NODEREF, UPDATE_REFS_MSG_ON_ERR)) + res = -1; + if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1) && + update_ref(NULL, "REVERT_HEAD", commit->object.oid.hash, NULL, + REF_NODEREF, UPDATE_REFS_MSG_ON_ERR)) + res = -1; if (res) { error(opts->action == REPLAY_REVERT From ba71c7e3c1a9727f448584e98a69ea351dd2b670 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:05 +0200 Subject: [PATCH 03/44] sequencer: lib'ify write_message() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of write_message(), do_pick_commit() already checks the return value and passes it on to its callers, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make write_message() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/sequencer.c b/sequencer.c index baf6b40e7a..ec85fe77b4 100644 --- a/sequencer.c +++ b/sequencer.c @@ -180,17 +180,20 @@ static void print_advice(int show_hint, struct replay_opts *opts) } } -static void write_message(struct strbuf *msgbuf, const char *filename) +static int write_message(struct strbuf *msgbuf, const char *filename) { static struct lock_file msg_file; - int msg_fd = hold_lock_file_for_update(&msg_file, filename, - LOCK_DIE_ON_ERROR); + int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0); + if (msg_fd < 0) + return error_errno(_("Could not lock '%s'"), filename); if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0) - die_errno(_("Could not write to %s"), filename); + return error_errno(_("Could not write to %s"), filename); strbuf_release(msgbuf); if (commit_lock_file(&msg_file) < 0) - die(_("Error wrapping up %s."), filename); + return error(_("Error wrapping up %s."), filename); + + return 0; } static struct tree *empty_tree(void) @@ -564,16 +567,16 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) head, &msgbuf, opts); if (res < 0) return res; - write_message(&msgbuf, git_path_merge_msg()); + res |= write_message(&msgbuf, git_path_merge_msg()); } else { struct commit_list *common = NULL; struct commit_list *remotes = NULL; - write_message(&msgbuf, git_path_merge_msg()); + res = write_message(&msgbuf, git_path_merge_msg()); commit_list_insert(base, &common); commit_list_insert(next, &remotes); - res = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts, + res |= try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts, common, sha1_to_hex(head), remotes); free_commit_list(common); free_commit_list(remotes); From 7813d1f37a3656e3461f4c759037a890c1c3ae02 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:10 +0200 Subject: [PATCH 04/44] sequencer: lib'ify do_recursive_merge() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of do_recursive_merge(), do_pick_commit() already checks the return value and passes it on to its callers, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make do_recursive_merge() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index ec85fe77b4..eb700913a1 100644 --- a/sequencer.c +++ b/sequencer.c @@ -303,7 +303,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next, if (active_cache_changed && write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) /* TRANSLATORS: %s will be "revert" or "cherry-pick" */ - die(_("%s: Unable to write new index file"), action_name(opts)); + return error(_("%s: Unable to write new index file"), + action_name(opts)); rollback_lock_file(&index_lock); if (opts->signoff) From dcd7d451d4e5d7577c18a5df876d81754e67a96b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:12 +0200 Subject: [PATCH 05/44] sequencer: lib'ify do_pick_commit() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only two callers of do_pick_commit(), pick_commits() and single_pick() already check the return value and pass it on to their callers, so their callers must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make do_pick_commit() callable from new callers that want it not to die, without changing the external behaviour of anything existing. While at it, remove the superfluous space. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index eb700913a1..96b9ae1978 100644 --- a/sequencer.c +++ b/sequencer.c @@ -464,7 +464,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) * to work on. */ if (write_cache_as_tree(head, 0, NULL)) - die (_("Your index file is unmerged.")); + return error(_("Your index file is unmerged.")); } else { unborn = get_sha1("HEAD", head); if (unborn) From 93f2bc9dbbee4d780744503ea9c3f92c32790e22 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:15 +0200 Subject: [PATCH 06/44] sequencer: lib'ify walk_revs_populate_todo() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The function sequencer_pick_revisions() is the only caller of walk_revs_populate_todo(), and it already returns errors appropriately, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make walk_revs_populate_todo() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/sequencer.c b/sequencer.c index 96b9ae1978..ab599e04df 100644 --- a/sequencer.c +++ b/sequencer.c @@ -809,17 +809,19 @@ static void read_populate_opts(struct replay_opts **opts_ptr) die(_("Malformed options sheet: %s"), git_path_opts_file()); } -static void walk_revs_populate_todo(struct commit_list **todo_list, +static int walk_revs_populate_todo(struct commit_list **todo_list, struct replay_opts *opts) { struct commit *commit; struct commit_list **next; - prepare_revs(opts); + if (prepare_revs(opts)) + return -1; next = todo_list; while ((commit = get_revision(opts->revs))) next = commit_list_append(commit, next); + return 0; } static int create_seq_dir(void) @@ -1102,8 +1104,8 @@ int sequencer_pick_revisions(struct replay_opts *opts) * progress */ - walk_revs_populate_todo(&todo_list, opts); - if (create_seq_dir() < 0) + if (walk_revs_populate_todo(&todo_list, opts) || + create_seq_dir() < 0) return -1; if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT)) return error(_("Can't revert as initial commit")); From 383cf8eb41ae27f4baccb5594afc9f79c31735f1 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:18 +0200 Subject: [PATCH 07/44] sequencer: lib'ify prepare_revs() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of prepare_revs(), walk_revs_populate_todo() was just taught to return errors, after verifying that its callers are prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make prepare_revs() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/sequencer.c b/sequencer.c index ab599e04df..7fd0f99650 100644 --- a/sequencer.c +++ b/sequencer.c @@ -623,7 +623,7 @@ leave: return res; } -static void prepare_revs(struct replay_opts *opts) +static int prepare_revs(struct replay_opts *opts) { /* * picking (but not reverting) ranges (but not individual revisions) @@ -633,10 +633,11 @@ static void prepare_revs(struct replay_opts *opts) opts->revs->reverse ^= 1; if (prepare_revision_walk(opts->revs)) - die(_("revision walk setup failed")); + return error(_("revision walk setup failed")); if (!opts->revs->commits) - die(_("empty commit set passed")); + return error(_("empty commit set passed")); + return 0; } static void read_and_refresh_cache(struct replay_opts *opts) From 494269270ca4b1fa91004d218b2974d74814d900 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:21 +0200 Subject: [PATCH 08/44] sequencer: lib'ify read_and_refresh_cache() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). There are two call sites of read_and_refresh_cache(), one of which is pick_commits(), whose callers were already prepared to do the right thing given an "error" return from it by an earlier patch, so the conversion is safe. The other one, sequencer_pick_revisions() was also prepared to relay an error return back to its caller in all remaining cases in an earlier patch. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/sequencer.c b/sequencer.c index 7fd0f99650..631b75dabe 100644 --- a/sequencer.c +++ b/sequencer.c @@ -640,18 +640,21 @@ static int prepare_revs(struct replay_opts *opts) return 0; } -static void read_and_refresh_cache(struct replay_opts *opts) +static int read_and_refresh_cache(struct replay_opts *opts) { static struct lock_file index_lock; int index_fd = hold_locked_index(&index_lock, 0); if (read_index_preload(&the_index, NULL) < 0) - die(_("git %s: failed to read the index"), action_name(opts)); + return error(_("git %s: failed to read the index"), + action_name(opts)); refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL); if (the_index.cache_changed && index_fd >= 0) { if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) - die(_("git %s: failed to refresh the index"), action_name(opts)); + return error(_("git %s: failed to refresh the index"), + action_name(opts)); } rollback_lock_file(&index_lock); + return 0; } static int format_todo(struct strbuf *buf, struct commit_list *todo_list, @@ -981,7 +984,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts) if (opts->allow_ff) assert(!(opts->signoff || opts->no_commit || opts->record_origin || opts->edit)); - read_and_refresh_cache(opts); + if (read_and_refresh_cache(opts)) + return -1; for (cur = todo_list; cur; cur = cur->next) { save_todo(cur, opts); @@ -1045,7 +1049,8 @@ int sequencer_pick_revisions(struct replay_opts *opts) if (opts->subcommand == REPLAY_NONE) assert(opts->revs); - read_and_refresh_cache(opts); + if (read_and_refresh_cache(opts)) + return -1; /* * Decide what to do depending on the arguments; a fresh From 08095465c5922ca0abff8b21f7a39f12a8ded8a6 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:24 +0200 Subject: [PATCH 09/44] sequencer: lib'ify read_populate_todo() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of read_populate_todo(), sequencer_continue() can already return errors, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make read_populate_todo() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/sequencer.c b/sequencer.c index 631b75dabe..c73cdfdac1 100644 --- a/sequencer.c +++ b/sequencer.c @@ -748,7 +748,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list, return 0; } -static void read_populate_todo(struct commit_list **todo_list, +static int read_populate_todo(struct commit_list **todo_list, struct replay_opts *opts) { struct strbuf buf = STRBUF_INIT; @@ -756,18 +756,21 @@ static void read_populate_todo(struct commit_list **todo_list, fd = open(git_path_todo_file(), O_RDONLY); if (fd < 0) - die_errno(_("Could not open %s"), git_path_todo_file()); + return error_errno(_("Could not open %s"), + git_path_todo_file()); if (strbuf_read(&buf, fd, 0) < 0) { close(fd); strbuf_release(&buf); - die(_("Could not read %s."), git_path_todo_file()); + return error(_("Could not read %s."), git_path_todo_file()); } close(fd); res = parse_insn_buffer(buf.buf, todo_list, opts); strbuf_release(&buf); if (res) - die(_("Unusable instruction sheet: %s"), git_path_todo_file()); + return error(_("Unusable instruction sheet: %s"), + git_path_todo_file()); + return 0; } static int populate_opts_cb(const char *key, const char *value, void *data) @@ -1019,7 +1022,8 @@ static int sequencer_continue(struct replay_opts *opts) if (!file_exists(git_path_todo_file())) return continue_single_pick(); read_populate_opts(&opts); - read_populate_todo(&todo_list, opts); + if (read_populate_todo(&todo_list, opts)) + return -1; /* Verify that the conflict has been resolved */ if (file_exists(git_path_cherry_pick_head()) || From 7cea732308ea83a47a70e1bc155bcc66a139b902 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:27 +0200 Subject: [PATCH 10/44] sequencer: lib'ify read_populate_opts() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of read_populate_opts(), sequencer_continue() can already return errors, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make read_populate_opts() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Note that the function git_config_from_file(), called from read_populate_opts(), can currently still die() (in git_parse_source(), because the do_config_from_file() function sets die_on_error = 1). We do not try to fix that here, as it would have larger ramifications on the config code, and we also assume that we write the opts file programmatically, hence any parse errors would be bugs. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/sequencer.c b/sequencer.c index c73cdfdac1..1614efb8db 100644 --- a/sequencer.c +++ b/sequencer.c @@ -808,12 +808,20 @@ static int populate_opts_cb(const char *key, const char *value, void *data) return 0; } -static void read_populate_opts(struct replay_opts **opts_ptr) +static int read_populate_opts(struct replay_opts **opts) { if (!file_exists(git_path_opts_file())) - return; - if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0) - die(_("Malformed options sheet: %s"), git_path_opts_file()); + return 0; + /* + * The function git_parse_source(), called from git_config_from_file(), + * may die() in case of a syntactically incorrect file. We do not care + * about this case, though, because we wrote that file ourselves, so we + * are pretty certain that it is syntactically correct. + */ + if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0) + return error(_("Malformed options sheet: %s"), + git_path_opts_file()); + return 0; } static int walk_revs_populate_todo(struct commit_list **todo_list, @@ -1021,8 +1029,8 @@ static int sequencer_continue(struct replay_opts *opts) if (!file_exists(git_path_todo_file())) return continue_single_pick(); - read_populate_opts(&opts); - if (read_populate_todo(&todo_list, opts)) + if (read_populate_opts(&opts) || + read_populate_todo(&todo_list, opts)) return -1; /* Verify that the conflict has been resolved */ From 3ba6e43477bd6a5d2a52cdc97c20278d71e296ad Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:44 +0200 Subject: [PATCH 11/44] sequencer: lib'ify create_seq_dir() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of create_seq_dir(), sequencer_pick_revisions() can already return errors, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make create_seq_dir() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sequencer.c b/sequencer.c index 1614efb8db..eb9c473328 100644 --- a/sequencer.c +++ b/sequencer.c @@ -847,8 +847,8 @@ static int create_seq_dir(void) return -1; } else if (mkdir(git_path_seq_dir(), 0777) < 0) - die_errno(_("Could not create sequencer directory %s"), - git_path_seq_dir()); + return error_errno(_("Could not create sequencer directory %s"), + git_path_seq_dir()); return 0; } From 4aa1e1ad3e81a926bcb4f0a40d82f83ecd75b2ef Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:47 +0200 Subject: [PATCH 12/44] sequencer: lib'ify save_head() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of save_head(), sequencer_pick_revisions() can already return errors, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make save_head() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/sequencer.c b/sequencer.c index eb9c473328..7a1561e6a0 100644 --- a/sequencer.c +++ b/sequencer.c @@ -852,18 +852,28 @@ static int create_seq_dir(void) return 0; } -static void save_head(const char *head) +static int save_head(const char *head) { static struct lock_file head_lock; struct strbuf buf = STRBUF_INIT; int fd; - fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), LOCK_DIE_ON_ERROR); + fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0); + if (fd < 0) { + rollback_lock_file(&head_lock); + return error_errno(_("Could not lock HEAD")); + } strbuf_addf(&buf, "%s\n", head); - if (write_in_full(fd, buf.buf, buf.len) < 0) - die_errno(_("Could not write to %s"), git_path_head_file()); - if (commit_lock_file(&head_lock) < 0) - die(_("Error wrapping up %s."), git_path_head_file()); + if (write_in_full(fd, buf.buf, buf.len) < 0) { + rollback_lock_file(&head_lock); + return error_errno(_("Could not write to %s"), + git_path_head_file()); + } + if (commit_lock_file(&head_lock) < 0) { + rollback_lock_file(&head_lock); + return error(_("Error wrapping up %s."), git_path_head_file()); + } + return 0; } static int reset_for_rollback(const unsigned char *sha1) @@ -1127,7 +1137,8 @@ int sequencer_pick_revisions(struct replay_opts *opts) return -1; if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT)) return error(_("Can't revert as initial commit")); - save_head(sha1_to_hex(sha1)); + if (save_head(sha1_to_hex(sha1))) + return -1; save_opts(opts); return pick_commits(todo_list, opts); } From 64b6a5366f67bedb2d3eece4a7a0898e4b13a30e Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:50 +0200 Subject: [PATCH 13/44] sequencer: lib'ify save_todo() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of save_todo(), pick_commits() can already return errors, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make save_todo() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/sequencer.c b/sequencer.c index 7a1561e6a0..32c53bb2b7 100644 --- a/sequencer.c +++ b/sequencer.c @@ -943,24 +943,31 @@ fail: return -1; } -static void save_todo(struct commit_list *todo_list, struct replay_opts *opts) +static int save_todo(struct commit_list *todo_list, struct replay_opts *opts) { static struct lock_file todo_lock; struct strbuf buf = STRBUF_INIT; int fd; - fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), LOCK_DIE_ON_ERROR); - if (format_todo(&buf, todo_list, opts) < 0) - die(_("Could not format %s."), git_path_todo_file()); + fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), 0); + if (fd < 0) + return error_errno(_("Could not lock '%s'"), + git_path_todo_file()); + if (format_todo(&buf, todo_list, opts) < 0) { + strbuf_release(&buf); + return error(_("Could not format %s."), git_path_todo_file()); + } if (write_in_full(fd, buf.buf, buf.len) < 0) { strbuf_release(&buf); - die_errno(_("Could not write to %s"), git_path_todo_file()); + return error_errno(_("Could not write to %s"), + git_path_todo_file()); } if (commit_lock_file(&todo_lock) < 0) { strbuf_release(&buf); - die(_("Error wrapping up %s."), git_path_todo_file()); + return error(_("Error wrapping up %s."), git_path_todo_file()); } strbuf_release(&buf); + return 0; } static void save_opts(struct replay_opts *opts) @@ -1009,7 +1016,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts) return -1; for (cur = todo_list; cur; cur = cur->next) { - save_todo(cur, opts); + if (save_todo(cur, opts)) + return -1; res = do_pick_commit(cur->item, opts); if (res) return res; From 380fccc9bc7432bdc6482b9c4204c077d7706327 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:53 +0200 Subject: [PATCH 14/44] sequencer: lib'ify save_opts() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of save_opts(), sequencer_pick_revisions() can already return errors, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make save_opts() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/sequencer.c b/sequencer.c index 32c53bb2b7..021ddf36d0 100644 --- a/sequencer.c +++ b/sequencer.c @@ -970,37 +970,39 @@ static int save_todo(struct commit_list *todo_list, struct replay_opts *opts) return 0; } -static void save_opts(struct replay_opts *opts) +static int save_opts(struct replay_opts *opts) { const char *opts_file = git_path_opts_file(); + int res = 0; if (opts->no_commit) - git_config_set_in_file(opts_file, "options.no-commit", "true"); + res |= git_config_set_in_file_gently(opts_file, "options.no-commit", "true"); if (opts->edit) - git_config_set_in_file(opts_file, "options.edit", "true"); + res |= git_config_set_in_file_gently(opts_file, "options.edit", "true"); if (opts->signoff) - git_config_set_in_file(opts_file, "options.signoff", "true"); + res |= git_config_set_in_file_gently(opts_file, "options.signoff", "true"); if (opts->record_origin) - git_config_set_in_file(opts_file, "options.record-origin", "true"); + res |= git_config_set_in_file_gently(opts_file, "options.record-origin", "true"); if (opts->allow_ff) - git_config_set_in_file(opts_file, "options.allow-ff", "true"); + res |= git_config_set_in_file_gently(opts_file, "options.allow-ff", "true"); if (opts->mainline) { struct strbuf buf = STRBUF_INIT; strbuf_addf(&buf, "%d", opts->mainline); - git_config_set_in_file(opts_file, "options.mainline", buf.buf); + res |= git_config_set_in_file_gently(opts_file, "options.mainline", buf.buf); strbuf_release(&buf); } if (opts->strategy) - git_config_set_in_file(opts_file, "options.strategy", opts->strategy); + res |= git_config_set_in_file_gently(opts_file, "options.strategy", opts->strategy); if (opts->gpg_sign) - git_config_set_in_file(opts_file, "options.gpg-sign", opts->gpg_sign); + res |= git_config_set_in_file_gently(opts_file, "options.gpg-sign", opts->gpg_sign); if (opts->xopts) { int i; for (i = 0; i < opts->xopts_nr; i++) - git_config_set_multivar_in_file(opts_file, + res |= git_config_set_multivar_in_file_gently(opts_file, "options.strategy-option", opts->xopts[i], "^$", 0); } + return res; } static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts) @@ -1147,7 +1149,8 @@ int sequencer_pick_revisions(struct replay_opts *opts) return error(_("Can't revert as initial commit")); if (save_head(sha1_to_hex(sha1))) return -1; - save_opts(opts); + if (save_opts(opts)) + return -1; return pick_commits(todo_list, opts); } From 5c820b533055ac54645b20cb764c98e59d4358d7 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:37:55 +0200 Subject: [PATCH 15/44] sequencer: lib'ify fast_forward_to() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only caller of fast_forward_to(), do_pick_commit() already checks the return value and passes it on to its callers, so its caller must be already prepared to handle error returns, and with this step, we make it notice an error return from this function. So this is a safe conversion to make fast_forward_to() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 021ddf36d0..d92a632f29 100644 --- a/sequencer.c +++ b/sequencer.c @@ -226,7 +226,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from, read_cache(); if (checkout_fast_forward(from, to, 1)) - exit(128); /* the callee should have complained already */ + return -1; /* the callee should have complained already */ strbuf_addf(&sb, _("%s: fast-forward"), action_name(opts)); From cb99c9a4195c22e68116266dc16058a765e2d98f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:38:00 +0200 Subject: [PATCH 16/44] sequencer: lib'ify checkout_fast_forward() Instead of dying there, let the caller high up in the callchain notice the error and handle it (by dying, still). The only callers of checkout_fast_forward(), cmd_merge(), pull_into_void(), cmd_pull() and sequencer's fast_forward_to(), already check the return value and handle it appropriately. With this step, we make it notice an error return from this function. So this is a safe conversion to make checkout_fast_forward() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- merge.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/merge.c b/merge.c index 5db7d56b90..23866c9165 100644 --- a/merge.c +++ b/merge.c @@ -57,7 +57,8 @@ int checkout_fast_forward(const unsigned char *head, refresh_cache(REFRESH_QUIET); - hold_locked_index(lock_file, 1); + if (hold_locked_index(lock_file, 0) < 0) + return -1; memset(&trees, 0, sizeof(trees)); memset(&opts, 0, sizeof(opts)); @@ -90,7 +91,9 @@ int checkout_fast_forward(const unsigned char *head, } if (unpack_trees(nr_trees, t, &opts)) return -1; - if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) - die(_("unable to write new index file")); + if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) { + rollback_lock_file(lock_file); + return error(_("unable to write new index file")); + } return 0; } From c24d7c6ae119c5470c7865fe196dbd3650b255eb Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:38:20 +0200 Subject: [PATCH 17/44] sequencer: ensure to release the lock when we could not read the index A future caller of read_and_refresh_cache() may want to do more than just print some helpful advice in case of failure. Suggested by Junio Hamano. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/sequencer.c b/sequencer.c index d92a632f29..eec8a60d6b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -644,14 +644,18 @@ static int read_and_refresh_cache(struct replay_opts *opts) { static struct lock_file index_lock; int index_fd = hold_locked_index(&index_lock, 0); - if (read_index_preload(&the_index, NULL) < 0) + if (read_index_preload(&the_index, NULL) < 0) { + rollback_lock_file(&index_lock); return error(_("git %s: failed to read the index"), action_name(opts)); + } refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL); if (the_index.cache_changed && index_fd >= 0) { - if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) + if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) { + rollback_lock_file(&index_lock); return error(_("git %s: failed to refresh the index"), action_name(opts)); + } } rollback_lock_file(&index_lock); return 0; From d024f256dd4988a1e91ee1f1ce12d8c0d203c9b3 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 8 Feb 2016 17:16:19 +0100 Subject: [PATCH 18/44] sequencer: use static initializers for replay_opts This change is not completely faithful: instead of initializing all fields to 0, we choose to initialize command and subcommand to -1 (instead of defaulting to REPLAY_REVERT and REPLAY_NONE, respectively). Practically, it makes no difference at all, but future-proofs the code to require explicit assignments for both fields. Signed-off-by: Johannes Schindelin --- builtin/revert.c | 6 ++---- sequencer.h | 1 + 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 4e693808b1..7365559c97 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -178,10 +178,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) int cmd_revert(int argc, const char **argv, const char *prefix) { - struct replay_opts opts; + struct replay_opts opts = REPLAY_OPTS_INIT; int res; - memset(&opts, 0, sizeof(opts)); if (isatty(0)) opts.edit = 1; opts.action = REPLAY_REVERT; @@ -195,10 +194,9 @@ int cmd_revert(int argc, const char **argv, const char *prefix) int cmd_cherry_pick(int argc, const char **argv, const char *prefix) { - struct replay_opts opts; + struct replay_opts opts = REPLAY_OPTS_INIT; int res; - memset(&opts, 0, sizeof(opts)); opts.action = REPLAY_PICK; git_config(git_default_config, NULL); parse_args(argc, argv, &opts); diff --git a/sequencer.h b/sequencer.h index 5ed5cb1d97..db425ad1a6 100644 --- a/sequencer.h +++ b/sequencer.h @@ -47,6 +47,7 @@ struct replay_opts { /* Only used by REPLAY_NONE */ struct rev_info *revs; }; +#define REPLAY_OPTS_INIT { -1, -1 } int sequencer_pick_revisions(struct replay_opts *opts); From afb6c870dde7591cb075fdb27893a758270ad56a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 9 Feb 2016 14:09:05 +0100 Subject: [PATCH 19/44] sequencer: use memoized sequencer directory path Signed-off-by: Johannes Schindelin --- builtin/commit.c | 2 +- sequencer.c | 11 ++++++----- sequencer.h | 5 +---- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 7a1ade0d27..0fdf520903 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -173,7 +173,7 @@ static void determine_whence(struct wt_status *s) whence = FROM_MERGE; else if (file_exists(git_path_cherry_pick_head())) { whence = FROM_CHERRY_PICK; - if (file_exists(git_path(SEQ_DIR))) + if (file_exists(git_path_seq_dir())) sequencer_in_use = 1; } else diff --git a/sequencer.c b/sequencer.c index eec8a60d6b..cb16cbda35 100644 --- a/sequencer.c +++ b/sequencer.c @@ -21,10 +21,11 @@ const char sign_off_header[] = "Signed-off-by: "; static const char cherry_picked_prefix[] = "(cherry picked from commit "; -static GIT_PATH_FUNC(git_path_todo_file, SEQ_TODO_FILE) -static GIT_PATH_FUNC(git_path_opts_file, SEQ_OPTS_FILE) -static GIT_PATH_FUNC(git_path_seq_dir, SEQ_DIR) -static GIT_PATH_FUNC(git_path_head_file, SEQ_HEAD_FILE) +GIT_PATH_FUNC(git_path_seq_dir, "sequencer") + +static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo") +static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts") +static GIT_PATH_FUNC(git_path_head_file, "sequencer/head") static int is_rfc2822_line(const char *buf, int len) { @@ -112,7 +113,7 @@ static void remove_sequencer_state(void) { struct strbuf seq_dir = STRBUF_INIT; - strbuf_addstr(&seq_dir, git_path(SEQ_DIR)); + strbuf_addstr(&seq_dir, git_path_seq_dir()); remove_dir_recursively(&seq_dir, 0); strbuf_release(&seq_dir); } diff --git a/sequencer.h b/sequencer.h index db425ad1a6..dd4d33a572 100644 --- a/sequencer.h +++ b/sequencer.h @@ -1,10 +1,7 @@ #ifndef SEQUENCER_H #define SEQUENCER_H -#define SEQ_DIR "sequencer" -#define SEQ_HEAD_FILE "sequencer/head" -#define SEQ_TODO_FILE "sequencer/todo" -#define SEQ_OPTS_FILE "sequencer/opts" +const char *git_path_seq_dir(void); #define APPEND_SIGNOFF_DEDUP (1u << 0) From 23514d83eec2137fc74e63998abf98765909d40b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 9 Feb 2016 14:23:55 +0100 Subject: [PATCH 20/44] sequencer: avoid unnecessary indirection We really do not need the *pointer to a* pointer to the options in the read_populate_opts() function. Signed-off-by: Johannes Schindelin --- sequencer.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sequencer.c b/sequencer.c index cb16cbda35..c2fbf6f89c 100644 --- a/sequencer.c +++ b/sequencer.c @@ -813,7 +813,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data) return 0; } -static int read_populate_opts(struct replay_opts **opts) +static int read_populate_opts(struct replay_opts *opts) { if (!file_exists(git_path_opts_file())) return 0; @@ -823,7 +823,7 @@ static int read_populate_opts(struct replay_opts **opts) * about this case, though, because we wrote that file ourselves, so we * are pretty certain that it is syntactically correct. */ - if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0) + if (git_config_from_file(populate_opts_cb, git_path_opts_file(), opts) < 0) return error(_("Malformed options sheet: %s"), git_path_opts_file()); return 0; @@ -1054,7 +1054,7 @@ static int sequencer_continue(struct replay_opts *opts) if (!file_exists(git_path_todo_file())) return continue_single_pick(); - if (read_populate_opts(&opts) || + if (read_populate_opts(opts) || read_populate_todo(&todo_list, opts)) return -1; From b841430e33384a7afbecee1a5584b2c0358f6e84 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 9 Feb 2016 16:06:35 +0100 Subject: [PATCH 21/44] sequencer: future-proof remove_sequencer_state() In a couple of commits, we will teach the sequencer to handle the nitty gritty of the interactive rebase, which keeps its state in a different directory. Signed-off-by: Johannes Schindelin --- sequencer.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/sequencer.c b/sequencer.c index c2fbf6f89c..8d272fbdca 100644 --- a/sequencer.c +++ b/sequencer.c @@ -27,6 +27,11 @@ static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo") static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts") static GIT_PATH_FUNC(git_path_head_file, "sequencer/head") +static const char *get_dir(const struct replay_opts *opts) +{ + return git_path_seq_dir(); +} + static int is_rfc2822_line(const char *buf, int len) { int i; @@ -109,13 +114,13 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob, return 1; } -static void remove_sequencer_state(void) +static void remove_sequencer_state(const struct replay_opts *opts) { - struct strbuf seq_dir = STRBUF_INIT; + struct strbuf dir = STRBUF_INIT; - strbuf_addstr(&seq_dir, git_path_seq_dir()); - remove_dir_recursively(&seq_dir, 0); - strbuf_release(&seq_dir); + strbuf_addf(&dir, "%s", get_dir(opts)); + remove_dir_recursively(&dir, 0); + strbuf_release(&dir); } static const char *action_name(const struct replay_opts *opts) @@ -940,7 +945,7 @@ static int sequencer_rollback(struct replay_opts *opts) } if (reset_for_rollback(sha1)) goto fail; - remove_sequencer_state(); + remove_sequencer_state(opts); strbuf_release(&buf); return 0; fail: @@ -1034,7 +1039,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts) * Sequence of picks finished successfully; cleanup by * removing the .git/sequencer directory */ - remove_sequencer_state(); + remove_sequencer_state(opts); return 0; } @@ -1095,7 +1100,7 @@ int sequencer_pick_revisions(struct replay_opts *opts) * one that is being continued */ if (opts->subcommand == REPLAY_REMOVE_STATE) { - remove_sequencer_state(); + remove_sequencer_state(opts); return 0; } if (opts->subcommand == REPLAY_ROLLBACK) From af5d46631e9019e2a1fa458f2284ec68687bfd54 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 9 Jun 2016 09:43:42 +0200 Subject: [PATCH 22/44] sequencer: plug memory leaks for the option values The sequencer is our attempt to lib-ify cherry-pick. Yet it behaves like a one-shot command when it reads its configuration: memory is allocated and released only when the command exits. This is kind of okay for git-cherry-pick, which *is* a one-shot command. All the work to make the sequencer its work horse was done to allow using the functionality as a library function, though, including proper clean-up after use. To remedy that, we now take custody of the option values in question, requiring those values to be malloc()ed or strdup()ed (an alternative approach, to add a list of pointers to malloc()ed data and to ask the sequencer to release all of them in the end, was proposed earlier but rejected during review). Note: this means that we now have to take care to strdup() the values passed via the command-line. Signed-off-by: Johannes Schindelin --- builtin/revert.c | 4 ++++ sequencer.c | 26 ++++++++++++++++++++++---- sequencer.h | 6 +++--- 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 7365559c97..ba5a88cbfd 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -174,6 +174,10 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) if (argc > 1) usage_with_options(usage_str, options); + + /* These option values will be free()d */ + opts->gpg_sign = xstrdup_or_null(opts->gpg_sign); + opts->strategy = xstrdup_or_null(opts->strategy); } int cmd_revert(int argc, const char **argv, const char *prefix) diff --git a/sequencer.c b/sequencer.c index 8d272fbdca..04c55f2087 100644 --- a/sequencer.c +++ b/sequencer.c @@ -117,6 +117,13 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob, static void remove_sequencer_state(const struct replay_opts *opts) { struct strbuf dir = STRBUF_INIT; + int i; + + free(opts->gpg_sign); + free(opts->strategy); + for (i = 0; i < opts->xopts_nr; i++) + free(opts->xopts[i]); + free(opts->xopts); strbuf_addf(&dir, "%s", get_dir(opts)); remove_dir_recursively(&dir, 0); @@ -280,7 +287,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next, struct merge_options o; struct tree *result, *next_tree, *base_tree, *head_tree; int clean; - const char **xopt; + char **xopt; static struct lock_file index_lock; hold_locked_index(&index_lock, 1); @@ -583,7 +590,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) commit_list_insert(base, &common); commit_list_insert(next, &remotes); - res |= try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts, + res |= try_merge_command(opts->strategy, + opts->xopts_nr, (const char **)opts->xopts, common, sha1_to_hex(head), remotes); free_commit_list(common); free_commit_list(remotes); @@ -783,6 +791,16 @@ static int read_populate_todo(struct commit_list **todo_list, return 0; } +static int git_config_string_dup(char **dest, + const char *var, const char *value) +{ + if (!value) + return config_error_nonbool(var); + free(*dest); + *dest = xstrdup(value); + return 0; +} + static int populate_opts_cb(const char *key, const char *value, void *data) { struct replay_opts *opts = data; @@ -803,9 +821,9 @@ static int populate_opts_cb(const char *key, const char *value, void *data) else if (!strcmp(key, "options.mainline")) opts->mainline = git_config_int(key, value); else if (!strcmp(key, "options.strategy")) - git_config_string(&opts->strategy, key, value); + git_config_string_dup(&opts->strategy, key, value); else if (!strcmp(key, "options.gpg-sign")) - git_config_string(&opts->gpg_sign, key, value); + git_config_string_dup(&opts->gpg_sign, key, value); else if (!strcmp(key, "options.strategy-option")) { ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc); opts->xopts[opts->xopts_nr++] = xstrdup(value); diff --git a/sequencer.h b/sequencer.h index dd4d33a572..8453669765 100644 --- a/sequencer.h +++ b/sequencer.h @@ -34,11 +34,11 @@ struct replay_opts { int mainline; - const char *gpg_sign; + char *gpg_sign; /* Merge strategy */ - const char *strategy; - const char **xopts; + char *strategy; + char **xopts; size_t xopts_nr, xopts_alloc; /* Only used by REPLAY_NONE */ From 804426a0f2e7af6cd678b7d27e6da63c8913a683 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 13 Feb 2016 14:31:00 +0100 Subject: [PATCH 23/44] sequencer: future-proof read_populate_todo() Over the next commits, we will work on improving the sequencer to the point where it can process the todo script of an interactive rebase. To that end, we will need to teach the sequencer to read interactive rebase's todo file. In preparation, we consolidate all places where that todo file is needed to call a function that we will later extend. Signed-off-by: Johannes Schindelin --- sequencer.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/sequencer.c b/sequencer.c index 04c55f2087..fb0b94bcf3 100644 --- a/sequencer.c +++ b/sequencer.c @@ -32,6 +32,11 @@ static const char *get_dir(const struct replay_opts *opts) return git_path_seq_dir(); } +static const char *get_todo_path(const struct replay_opts *opts) +{ + return git_path_todo_file(); +} + static int is_rfc2822_line(const char *buf, int len) { int i; @@ -769,25 +774,24 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list, static int read_populate_todo(struct commit_list **todo_list, struct replay_opts *opts) { + const char *todo_file = get_todo_path(opts); struct strbuf buf = STRBUF_INIT; int fd, res; - fd = open(git_path_todo_file(), O_RDONLY); + fd = open(todo_file, O_RDONLY); if (fd < 0) - return error_errno(_("Could not open %s"), - git_path_todo_file()); + return error_errno(_("Could not open %s"), todo_file); if (strbuf_read(&buf, fd, 0) < 0) { close(fd); strbuf_release(&buf); - return error(_("Could not read %s."), git_path_todo_file()); + return error(_("Could not read %s."), todo_file); } close(fd); res = parse_insn_buffer(buf.buf, todo_list, opts); strbuf_release(&buf); if (res) - return error(_("Unusable instruction sheet: %s"), - git_path_todo_file()); + return error(_("Unusable instruction sheet: %s"), todo_file); return 0; } @@ -1075,7 +1079,7 @@ static int sequencer_continue(struct replay_opts *opts) { struct commit_list *todo_list = NULL; - if (!file_exists(git_path_todo_file())) + if (!file_exists(get_todo_path(opts))) return continue_single_pick(); if (read_populate_opts(opts) || read_populate_todo(&todo_list, opts)) From ecd387a8ad0515d17fa669429c9d647c033909ea Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 4 Mar 2016 16:21:16 +0100 Subject: [PATCH 24/44] sequencer: refactor the code to obtain a short commit name Not only does this DRY up the code (providing a better documentation what the code is about, as well as allowing to change the behavior in a single place), it also makes it substantially shorter to use the same functionality in functions to be introduced when we teach the sequencer to process interactive-rebase's git-rebase-todo file. Signed-off-by: Johannes Schindelin --- sequencer.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/sequencer.c b/sequencer.c index fb0b94bcf3..499f5ee89d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -147,13 +147,18 @@ struct commit_message { const char *message; }; +static const char *short_commit_name(struct commit *commit) +{ + return find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV); +} + static int get_message(struct commit *commit, struct commit_message *out) { const char *abbrev, *subject; int subject_len; out->message = logmsg_reencode(commit, NULL, get_commit_output_encoding()); - abbrev = find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV); + abbrev = short_commit_name(commit); subject_len = find_commit_subject(out->message, &subject); @@ -621,8 +626,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) error(opts->action == REPLAY_REVERT ? _("could not revert %s... %s") : _("could not apply %s... %s"), - find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV), - msg.subject); + short_commit_name(commit), msg.subject); print_advice(res == 1, opts); rerere(opts->allow_rerere_auto); goto leave; From 2f3bf7e49636ff3a8c756e3f0bcc9e17b5ba026b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 23 Feb 2016 19:21:31 +0100 Subject: [PATCH 25/44] sequencer: completely revamp the "todo" script parsing When we came up with the "sequencer" idea, we really wanted to have kind of a plumbing equivalent of the interactive rebase. Hence the choice of words: the "todo" script, a "pick", etc. However, when it came time to implement the entire shebang, somehow this idea got lost and the sequencer was used as working horse for cherry-pick and revert instead. So as not to interfere with the interactive rebase, it even uses a separate directory to store its state. Furthermore, it also is stupidly strict about the "todo" script it accepts: while it parses commands in a way that was *designed* to be similar to the interactive rebase, it then goes on to *error out* if the commands disagree with the overall action (cherry-pick or revert). Finally, the sequencer code chose to deviate from the interactive rebase code insofar that when it comes to writing the file with the remaining commands, it *reformats* the "todo" script instead of just writing the part of the parsed script that were not yet processed. This is not only unnecessary churn, but might well lose information that is valuable to the user (i.e. comments after the commands). Let's just bite the bullet and rewrite the entire parser; the code now becomes not only more elegant: it allows us to go on and teach the sequencer how to parse *true* "todo" scripts as used by the interactive rebase itself. In a way, the sequencer is about to grow up to do its older brother's job. Better. In particular, we choose to maintain the list of commands in an array instead of a linked list: this is flexible enough to allow us later on to even implement rebase -i's reordering of fixup!/squash! commits very easily (and with a very nice speed bonus, at least on Windows). While at it, do not stop at the first problem, but list *all* of the problems. This will help the user when the sequencer will do `rebase -i`'s work by allowing to address all issues in one go rather than going back and forth until the todo list is valid. Signed-off-by: Johannes Schindelin --- sequencer.c | 284 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 163 insertions(+), 121 deletions(-) diff --git a/sequencer.c b/sequencer.c index 499f5ee89d..145de786e1 100644 --- a/sequencer.c +++ b/sequencer.c @@ -470,7 +470,26 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit) return 1; } -static int do_pick_commit(struct commit *commit, struct replay_opts *opts) +enum todo_command { + TODO_PICK = 0, + TODO_REVERT +}; + +static const char *todo_command_strings[] = { + "pick", + "revert" +}; + +static const char *command_to_string(const enum todo_command command) +{ + if (command < ARRAY_SIZE(todo_command_strings)) + return todo_command_strings[command]; + die("Unknown command: %d", command); +} + + +static int do_pick_commit(enum todo_command command, struct commit *commit, + struct replay_opts *opts) { unsigned char head[20]; struct commit *base, *next, *parent; @@ -529,10 +548,11 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) return fast_forward_to(commit->object.oid.hash, head, unborn, opts); if (parent && parse_commit(parent) < 0) - /* TRANSLATORS: The first %s will be "revert" or - "cherry-pick", the second %s a SHA1 */ + /* TRANSLATORS: The first %s will be a "todo" command like + "revert" or "pick", the second %s a SHA1. */ return error(_("%s: cannot parse parent commit %s"), - action_name(opts), oid_to_hex(&parent->object.oid)); + command_to_string(command), + oid_to_hex(&parent->object.oid)); if (get_message(commit, &msg) != 0) return error(_("Cannot get commit message for %s"), @@ -545,7 +565,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) * reverse of it if we are revert. */ - if (opts->action == REPLAY_REVERT) { + if (command == TODO_REVERT) { base = commit; base_label = msg.label; next = parent; @@ -586,7 +606,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) } } - if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) { + if (!opts->strategy || !strcmp(opts->strategy, "recursive") || command == TODO_REVERT) { res = do_recursive_merge(base, next, base_label, next_label, head, &msgbuf, opts); if (res < 0) @@ -613,17 +633,17 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) * However, if the merge did not even start, then we don't want to * write it at all. */ - if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1) && + if (command == TODO_PICK && !opts->no_commit && (res == 0 || res == 1) && update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.oid.hash, NULL, REF_NODEREF, UPDATE_REFS_MSG_ON_ERR)) res = -1; - if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1) && + if (command == TODO_REVERT && ((opts->no_commit && res == 0) || res == 1) && update_ref(NULL, "REVERT_HEAD", commit->object.oid.hash, NULL, REF_NODEREF, UPDATE_REFS_MSG_ON_ERR)) res = -1; if (res) { - error(opts->action == REPLAY_REVERT + error(command == TODO_REVERT ? _("could not revert %s... %s") : _("could not apply %s... %s"), short_commit_name(commit), msg.subject); @@ -684,116 +704,122 @@ static int read_and_refresh_cache(struct replay_opts *opts) return 0; } -static int format_todo(struct strbuf *buf, struct commit_list *todo_list, - struct replay_opts *opts) -{ - struct commit_list *cur = NULL; - const char *sha1_abbrev = NULL; - const char *action_str = opts->action == REPLAY_REVERT ? "revert" : "pick"; - const char *subject; - int subject_len; +struct todo_item { + enum todo_command command; + struct commit *commit; + size_t offset_in_buf; +}; - for (cur = todo_list; cur; cur = cur->next) { - const char *commit_buffer = get_commit_buffer(cur->item, NULL); - sha1_abbrev = find_unique_abbrev(cur->item->object.oid.hash, DEFAULT_ABBREV); - subject_len = find_commit_subject(commit_buffer, &subject); - strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev, - subject_len, subject); - unuse_commit_buffer(cur->item, commit_buffer); - } - return 0; +struct todo_list { + struct strbuf buf; + struct todo_item *items; + int nr, alloc, current; +}; + +#define TODO_LIST_INIT { STRBUF_INIT } + +static void todo_list_release(struct todo_list *todo_list) +{ + strbuf_release(&todo_list->buf); + free(todo_list->items); + todo_list->items = NULL; + todo_list->nr = todo_list->alloc = 0; } -static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts) +static struct todo_item *append_new_todo(struct todo_list *todo_list) +{ + ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc); + return todo_list->items + todo_list->nr++; +} + +static int parse_insn_line(struct todo_item *item, const char *bol, char *eol) { unsigned char commit_sha1[20]; - enum replay_action action; char *end_of_object_name; - int saved, status, padding; + int i, saved, status, padding; - if (starts_with(bol, "pick")) { - action = REPLAY_PICK; - bol += strlen("pick"); - } else if (starts_with(bol, "revert")) { - action = REPLAY_REVERT; - bol += strlen("revert"); - } else - return NULL; + for (i = 0; i < ARRAY_SIZE(todo_command_strings); i++) + if (skip_prefix(bol, todo_command_strings[i], &bol)) { + item->command = i; + break; + } + if (i >= ARRAY_SIZE(todo_command_strings)) + return -1; /* Eat up extra spaces/ tabs before object name */ padding = strspn(bol, " \t"); if (!padding) - return NULL; + return -1; bol += padding; - end_of_object_name = bol + strcspn(bol, " \t\n"); + end_of_object_name = (char *) bol + strcspn(bol, " \t\n"); saved = *end_of_object_name; *end_of_object_name = '\0'; status = get_sha1(bol, commit_sha1); *end_of_object_name = saved; - /* - * Verify that the action matches up with the one in - * opts; we don't support arbitrary instructions - */ - if (action != opts->action) { - if (action == REPLAY_REVERT) - error((opts->action == REPLAY_REVERT) - ? _("Cannot revert during another revert.") - : _("Cannot revert during a cherry-pick.")); - else - error((opts->action == REPLAY_REVERT) - ? _("Cannot cherry-pick during a revert.") - : _("Cannot cherry-pick during another cherry-pick.")); - return NULL; - } - if (status < 0) - return NULL; + return -1; - return lookup_commit_reference(commit_sha1); + item->commit = lookup_commit_reference(commit_sha1); + return !item->commit; } -static int parse_insn_buffer(char *buf, struct commit_list **todo_list, - struct replay_opts *opts) +static int parse_insn_buffer(char *buf, struct todo_list *todo_list) { - struct commit_list **next = todo_list; - struct commit *commit; - char *p = buf; - int i; + struct todo_item *item; + char *p = buf, *next_p; + int i, res = 0; - for (i = 1; *p; i++) { + for (i = 1; *p; i++, p = next_p) { char *eol = strchrnul(p, '\n'); - commit = parse_insn_line(p, eol, opts); - if (!commit) - return error(_("Could not parse line %d."), i); - next = commit_list_append(commit, next); - p = *eol ? eol + 1 : eol; + + next_p = *eol ? eol + 1 /* skip LF */ : eol; + + item = append_new_todo(todo_list); + item->offset_in_buf = p - todo_list->buf.buf; + if (parse_insn_line(item, p, eol)) { + res = error(_("Invalid line %d: %.*s"), + i, (int)(eol - p), p); + item->command = -1; + } } - if (!*todo_list) + if (!todo_list->nr) return error(_("No commits parsed.")); - return 0; + return res; } -static int read_populate_todo(struct commit_list **todo_list, +static int read_populate_todo(struct todo_list *todo_list, struct replay_opts *opts) { const char *todo_file = get_todo_path(opts); - struct strbuf buf = STRBUF_INIT; int fd, res; + strbuf_reset(&todo_list->buf); fd = open(todo_file, O_RDONLY); if (fd < 0) return error_errno(_("Could not open %s"), todo_file); - if (strbuf_read(&buf, fd, 0) < 0) { + if (strbuf_read(&todo_list->buf, fd, 0) < 0) { close(fd); - strbuf_release(&buf); return error(_("Could not read %s."), todo_file); } close(fd); - res = parse_insn_buffer(buf.buf, todo_list, opts); - strbuf_release(&buf); + res = parse_insn_buffer(todo_list->buf.buf, todo_list); + if (!res) { + enum todo_command valid = + opts->action == REPLAY_PICK ? TODO_PICK : TODO_REVERT; + int i; + + for (i = 0; i < todo_list->nr; i++) + if (valid == todo_list->items[i].command) + continue; + else if (valid == TODO_PICK) + return error(_("Cannot cherry-pick during a revert.")); + else + return error(_("Cannot revert during a cherry-pick.")); + } + if (res) return error(_("Unusable instruction sheet: %s"), todo_file); return 0; @@ -860,18 +886,31 @@ static int read_populate_opts(struct replay_opts *opts) return 0; } -static int walk_revs_populate_todo(struct commit_list **todo_list, +static int walk_revs_populate_todo(struct todo_list *todo_list, struct replay_opts *opts) { + enum todo_command command = opts->action == REPLAY_PICK ? + TODO_PICK : TODO_REVERT; + const char *command_string = todo_command_strings[command]; struct commit *commit; - struct commit_list **next; if (prepare_revs(opts)) return -1; - next = todo_list; - while ((commit = get_revision(opts->revs))) - next = commit_list_append(commit, next); + while ((commit = get_revision(opts->revs))) { + struct todo_item *item = append_new_todo(todo_list); + const char *commit_buffer = get_commit_buffer(commit, NULL); + const char *subject; + int subject_len; + + item->command = command; + item->commit = commit; + item->offset_in_buf = todo_list->buf.len; + subject_len = find_commit_subject(commit_buffer, &subject); + strbuf_addf(&todo_list->buf, "%s %s %.*s\n", command_string, + short_commit_name(commit), subject_len, subject); + unuse_commit_buffer(commit, commit_buffer); + } return 0; } @@ -979,30 +1018,22 @@ fail: return -1; } -static int save_todo(struct commit_list *todo_list, struct replay_opts *opts) +static int save_todo(struct todo_list *todo_list, struct replay_opts *opts) { static struct lock_file todo_lock; - struct strbuf buf = STRBUF_INIT; - int fd; + const char *todo_path = get_todo_path(opts); + int next = todo_list->current, offset, fd; - fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), 0); + fd = hold_lock_file_for_update(&todo_lock, todo_path, 0); if (fd < 0) - return error_errno(_("Could not lock '%s'"), - git_path_todo_file()); - if (format_todo(&buf, todo_list, opts) < 0) { - strbuf_release(&buf); - return error(_("Could not format %s."), git_path_todo_file()); - } - if (write_in_full(fd, buf.buf, buf.len) < 0) { - strbuf_release(&buf); - return error_errno(_("Could not write to %s"), - git_path_todo_file()); - } - if (commit_lock_file(&todo_lock) < 0) { - strbuf_release(&buf); - return error(_("Error wrapping up %s."), git_path_todo_file()); - } - strbuf_release(&buf); + return error_errno(_("Could not lock '%s'"), todo_path); + offset = next < todo_list->nr ? + todo_list->items[next].offset_in_buf : todo_list->buf.len; + if (write_in_full(fd, todo_list->buf.buf + offset, + todo_list->buf.len - offset) < 0) + return error_errno(_("Could not write to '%s'"), todo_path); + if (commit_lock_file(&todo_lock) < 0) + return error(_("Error wrapping up %s."), todo_path); return 0; } @@ -1041,9 +1072,8 @@ static int save_opts(struct replay_opts *opts) return res; } -static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts) +static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts) { - struct commit_list *cur; int res; setenv(GIT_REFLOG_ACTION, action_name(opts), 0); @@ -1053,10 +1083,12 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts) if (read_and_refresh_cache(opts)) return -1; - for (cur = todo_list; cur; cur = cur->next) { - if (save_todo(cur, opts)) + while (todo_list->current < todo_list->nr) { + struct todo_item *item = todo_list->items + todo_list->current; + if (save_todo(todo_list, opts)) return -1; - res = do_pick_commit(cur->item, opts); + res = do_pick_commit(item->command, item->commit, opts); + todo_list->current++; if (res) return res; } @@ -1081,38 +1113,46 @@ static int continue_single_pick(void) static int sequencer_continue(struct replay_opts *opts) { - struct commit_list *todo_list = NULL; + struct todo_list todo_list = TODO_LIST_INIT; + int res; if (!file_exists(get_todo_path(opts))) return continue_single_pick(); - if (read_populate_opts(opts) || - read_populate_todo(&todo_list, opts)) + if (read_populate_opts(opts)) return -1; + if ((res = read_populate_todo(&todo_list, opts))) + goto release_todo_list; /* Verify that the conflict has been resolved */ if (file_exists(git_path_cherry_pick_head()) || file_exists(git_path_revert_head())) { - int ret = continue_single_pick(); - if (ret) - return ret; + res = continue_single_pick(); + if (res) + goto release_todo_list; } - if (index_differs_from("HEAD", 0)) - return error_dirty_index(opts); - todo_list = todo_list->next; - return pick_commits(todo_list, opts); + if (index_differs_from("HEAD", 0)) { + res = error_dirty_index(opts); + goto release_todo_list; + } + todo_list.current++; + res = pick_commits(&todo_list, opts); +release_todo_list: + todo_list_release(&todo_list); + return res; } static int single_pick(struct commit *cmit, struct replay_opts *opts) { setenv(GIT_REFLOG_ACTION, action_name(opts), 0); - return do_pick_commit(cmit, opts); + return do_pick_commit(opts->action == REPLAY_PICK ? + TODO_PICK : TODO_REVERT, cmit, opts); } int sequencer_pick_revisions(struct replay_opts *opts) { - struct commit_list *todo_list = NULL; + struct todo_list todo_list = TODO_LIST_INIT; unsigned char sha1[20]; - int i; + int i, res; if (opts->subcommand == REPLAY_NONE) assert(opts->revs); @@ -1187,7 +1227,9 @@ int sequencer_pick_revisions(struct replay_opts *opts) return -1; if (save_opts(opts)) return -1; - return pick_commits(todo_list, opts); + res = pick_commits(&todo_list, opts); + todo_list_release(&todo_list); + return res; } void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag) From cb2070f6efd1728504f7f3b811acdc1c3a3a7456 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 9 Oct 2016 10:45:07 +0200 Subject: [PATCH 26/44] sequencer: strip CR from the todo script It is not unheard of that editors on Windows write CR/LF even if the file originally had only LF. This is particularly awkward for exec lines of a rebase -i todo sheet. Take for example the insn "exec echo": The shell script parser splits at the LF and leaves the CR attached to "echo", which leads to the unknown command "echo\r". Work around that by stripping CR when reading the todo commands, as we already do for LF. This happens to fix t9903.14 and .15 in MSYS1 environments (with the rebase--helper patches based on this patch series): the todo script constructed in such a setup contains CR/LF thanks to MSYS1 runtime's cleverness. Based on a report and a patch by Johannes Sixt. Signed-off-by: Johannes Schindelin --- sequencer.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sequencer.c b/sequencer.c index 145de786e1..04fcfd893f 100644 --- a/sequencer.c +++ b/sequencer.c @@ -776,6 +776,9 @@ static int parse_insn_buffer(char *buf, struct todo_list *todo_list) next_p = *eol ? eol + 1 /* skip LF */ : eol; + if (p != eol && eol[-1] == '\r') + eol--; /* strip Carriage Return */ + item = append_new_todo(todo_list); item->offset_in_buf = p - todo_list->buf.buf; if (parse_insn_line(item, p, eol)) { From 78ac58c0fc00744f098111dbd5d7cbbd135a91d8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 24 Feb 2016 14:57:22 +0100 Subject: [PATCH 27/44] sequencer: avoid completely different messages for different actions Signed-off-by: Johannes Schindelin --- sequencer.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/sequencer.c b/sequencer.c index 04fcfd893f..120a8ee351 100644 --- a/sequencer.c +++ b/sequencer.c @@ -229,11 +229,8 @@ static int error_dirty_index(struct replay_opts *opts) if (read_cache_unmerged()) return error_resolve_conflict(action_name(opts)); - /* Different translation strings for cherry-pick and revert */ - if (opts->action == REPLAY_PICK) - error(_("Your local changes would be overwritten by cherry-pick.")); - else - error(_("Your local changes would be overwritten by revert.")); + error(_("Your local changes would be overwritten by %s."), + action_name(opts)); if (advice_commit_before_merge) advise(_("Commit your changes or stash them to proceed.")); From 8e695414620beec11356a05b644b96bc3244be0b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 13 Apr 2016 17:39:02 +0200 Subject: [PATCH 28/44] sequencer: get rid of the subcommand field The subcommands are used exactly once, at the very beginning of sequencer_pick_revisions(), to determine what to do. This is an unnecessary level of indirection: we can simply call the correct function to begin with. So let's do that. While at it, ensure that the subcommands return an error code so that they do not have to die() all over the place (bad practice for library functions...). Signed-off-by: Johannes Schindelin --- builtin/revert.c | 36 ++++++++++++++++-------------------- sequencer.c | 35 +++++++++++------------------------ sequencer.h | 13 ++++--------- 3 files changed, 31 insertions(+), 53 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index ba5a88cbfd..4ca5b51544 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -71,7 +71,7 @@ static void verify_opt_compatible(const char *me, const char *base_opt, ...) die(_("%s: %s cannot be used with %s"), me, this_opt, base_opt); } -static void parse_args(int argc, const char **argv, struct replay_opts *opts) +static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) { const char * const * usage_str = revert_or_cherry_pick_usage(opts); const char *me = action_name(opts); @@ -115,25 +115,15 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) if (opts->keep_redundant_commits) opts->allow_empty = 1; - /* Set the subcommand */ - if (cmd == 'q') - opts->subcommand = REPLAY_REMOVE_STATE; - else if (cmd == 'c') - opts->subcommand = REPLAY_CONTINUE; - else if (cmd == 'a') - opts->subcommand = REPLAY_ROLLBACK; - else - opts->subcommand = REPLAY_NONE; - /* Check for incompatible command line arguments */ - if (opts->subcommand != REPLAY_NONE) { + if (cmd) { char *this_operation; - if (opts->subcommand == REPLAY_REMOVE_STATE) + if (cmd == 'q') this_operation = "--quit"; - else if (opts->subcommand == REPLAY_CONTINUE) + else if (cmd == 'c') this_operation = "--continue"; else { - assert(opts->subcommand == REPLAY_ROLLBACK); + assert(cmd == 'a'); this_operation = "--abort"; } @@ -156,7 +146,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) "--edit", opts->edit, NULL); - if (opts->subcommand != REPLAY_NONE) { + if (cmd) { opts->revs = NULL; } else { struct setup_revision_opt s_r_opt; @@ -178,6 +168,14 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) /* These option values will be free()d */ opts->gpg_sign = xstrdup_or_null(opts->gpg_sign); opts->strategy = xstrdup_or_null(opts->strategy); + + if (cmd == 'q') + return sequencer_remove_state(opts); + if (cmd == 'c') + return sequencer_continue(opts); + if (cmd == 'a') + return sequencer_rollback(opts); + return sequencer_pick_revisions(opts); } int cmd_revert(int argc, const char **argv, const char *prefix) @@ -189,8 +187,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix) opts.edit = 1; opts.action = REPLAY_REVERT; git_config(git_default_config, NULL); - parse_args(argc, argv, &opts); - res = sequencer_pick_revisions(&opts); + res = run_sequencer(argc, argv, &opts); if (res < 0) die(_("revert failed")); return res; @@ -203,8 +200,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix) opts.action = REPLAY_PICK; git_config(git_default_config, NULL); - parse_args(argc, argv, &opts); - res = sequencer_pick_revisions(&opts); + res = run_sequencer(argc, argv, &opts); if (res < 0) die(_("cherry-pick failed")); return res; diff --git a/sequencer.c b/sequencer.c index 120a8ee351..9f22c5ec41 100644 --- a/sequencer.c +++ b/sequencer.c @@ -119,7 +119,7 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob, return 1; } -static void remove_sequencer_state(const struct replay_opts *opts) +int sequencer_remove_state(struct replay_opts *opts) { struct strbuf dir = STRBUF_INIT; int i; @@ -133,6 +133,8 @@ static void remove_sequencer_state(const struct replay_opts *opts) strbuf_addf(&dir, "%s", get_dir(opts)); remove_dir_recursively(&dir, 0); strbuf_release(&dir); + + return 0; } static const char *action_name(const struct replay_opts *opts) @@ -975,7 +977,7 @@ static int rollback_single_pick(void) return reset_for_rollback(head_sha1); } -static int sequencer_rollback(struct replay_opts *opts) +int sequencer_rollback(struct replay_opts *opts) { FILE *f; unsigned char sha1[20]; @@ -1010,9 +1012,8 @@ static int sequencer_rollback(struct replay_opts *opts) } if (reset_for_rollback(sha1)) goto fail; - remove_sequencer_state(opts); strbuf_release(&buf); - return 0; + return sequencer_remove_state(opts); fail: strbuf_release(&buf); return -1; @@ -1097,8 +1098,7 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts) * Sequence of picks finished successfully; cleanup by * removing the .git/sequencer directory */ - remove_sequencer_state(opts); - return 0; + return sequencer_remove_state(opts); } static int continue_single_pick(void) @@ -1111,11 +1111,14 @@ static int continue_single_pick(void) return run_command_v_opt(argv, RUN_GIT_CMD); } -static int sequencer_continue(struct replay_opts *opts) +int sequencer_continue(struct replay_opts *opts) { struct todo_list todo_list = TODO_LIST_INIT; int res; + if (read_and_refresh_cache(opts)) + return -1; + if (!file_exists(get_todo_path(opts))) return continue_single_pick(); if (read_populate_opts(opts)) @@ -1154,26 +1157,10 @@ int sequencer_pick_revisions(struct replay_opts *opts) unsigned char sha1[20]; int i, res; - if (opts->subcommand == REPLAY_NONE) - assert(opts->revs); - + assert(opts->revs); if (read_and_refresh_cache(opts)) return -1; - /* - * Decide what to do depending on the arguments; a fresh - * cherry-pick should be handled differently from an existing - * one that is being continued - */ - if (opts->subcommand == REPLAY_REMOVE_STATE) { - remove_sequencer_state(opts); - return 0; - } - if (opts->subcommand == REPLAY_ROLLBACK) - return sequencer_rollback(opts); - if (opts->subcommand == REPLAY_CONTINUE) - return sequencer_continue(opts); - for (i = 0; i < opts->revs->pending.nr; i++) { unsigned char sha1[20]; const char *name = opts->revs->pending.objects[i].name; diff --git a/sequencer.h b/sequencer.h index 8453669765..7a513c576b 100644 --- a/sequencer.h +++ b/sequencer.h @@ -10,16 +10,8 @@ enum replay_action { REPLAY_PICK }; -enum replay_subcommand { - REPLAY_NONE, - REPLAY_REMOVE_STATE, - REPLAY_CONTINUE, - REPLAY_ROLLBACK -}; - struct replay_opts { enum replay_action action; - enum replay_subcommand subcommand; /* Boolean options */ int edit; @@ -44,9 +36,12 @@ struct replay_opts { /* Only used by REPLAY_NONE */ struct rev_info *revs; }; -#define REPLAY_OPTS_INIT { -1, -1 } +#define REPLAY_OPTS_INIT { -1 } int sequencer_pick_revisions(struct replay_opts *opts); +int sequencer_continue(struct replay_opts *opts); +int sequencer_rollback(struct replay_opts *opts); +int sequencer_remove_state(struct replay_opts *opts); extern const char sign_off_header[]; From 5b94fe2b0e8a536e5aacb4597ed9b4c1d672944c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 9 Mar 2016 17:31:24 +0100 Subject: [PATCH 29/44] sequencer: remember the onelines when parsing the todo file The `git-rebase-todo` file contains a list of commands. Most of those commands have the form The is displayed primarily for the user's convenience, as rebase -i really interprets only the part. However, there are *some* places in interactive rebase where the is used to display messages, e.g. for reporting at which commit we stopped. So let's just remember it when parsing the todo file; we keep a copy of the entire todo file anyway (to write out the new `done` and `git-rebase-todo` file just before processing each command), so all we need to do is remember the begin offsets and lengths. As we will have to parse and remember the command-line for `exec` commands later, we do not call the field "oneline" but rather "arg" (and will reuse that for exec's command-line). Signed-off-by: Johannes Schindelin --- sequencer.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/sequencer.c b/sequencer.c index 9f22c5ec41..3d1fdacc54 100644 --- a/sequencer.c +++ b/sequencer.c @@ -706,6 +706,8 @@ static int read_and_refresh_cache(struct replay_opts *opts) struct todo_item { enum todo_command command; struct commit *commit; + const char *arg; + int arg_len; size_t offset_in_buf; }; @@ -757,6 +759,9 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol) status = get_sha1(bol, commit_sha1); *end_of_object_name = saved; + item->arg = end_of_object_name + strspn(end_of_object_name, " \t"); + item->arg_len = (int)(eol - item->arg); + if (status < 0) return -1; @@ -907,6 +912,8 @@ static int walk_revs_populate_todo(struct todo_list *todo_list, item->command = command; item->commit = commit; + item->arg = NULL; + item->arg_len = 0; item->offset_in_buf = todo_list->buf.len; subject_len = find_commit_subject(commit_buffer, &subject); strbuf_addf(&todo_list->buf, "%s %s %.*s\n", command_string, From 14319804afb0d974c46ddc8f268e78a1deaaf0b4 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 29 Feb 2016 17:17:47 +0100 Subject: [PATCH 30/44] sequencer: prepare for rebase -i's commit functionality In interactive rebases, we commit a little bit differently than the sequencer did so far: we heed the "author-script", the "message" and the "amend" files in the .git/rebase-merge/ subdirectory. Likewise, we may want to edit the commit message *even* when providing a file containing the suggested commit message. Therefore we change the code to not even provide a default message when we do not want any, and to call the editor explicitly. Also, in "interactive rebase" mode we want to skip reading the options in the state directory of the cherry-pick/revert commands. Finally, as interactive rebase's GPG settings are configured differently from how cherry-pick (and therefore sequencer) handles them, we will leave support for that to the next commit. Signed-off-by: Johannes Schindelin --- sequencer.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 89 insertions(+), 10 deletions(-) diff --git a/sequencer.c b/sequencer.c index 3d1fdacc54..6d5fe9485a 100644 --- a/sequencer.c +++ b/sequencer.c @@ -27,6 +27,19 @@ static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo") static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts") static GIT_PATH_FUNC(git_path_head_file, "sequencer/head") +/* + * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and + * GIT_AUTHOR_DATE that will be used for the commit that is currently + * being rebased. + */ +static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script") + +/* We will introduce the 'interactive rebase' mode later */ +static inline int is_rebase_i(const struct replay_opts *opts) +{ + return 0; +} + static const char *get_dir(const struct replay_opts *opts) { return git_path_seq_dir(); @@ -369,20 +382,80 @@ static int is_index_unchanged(void) return !hashcmp(active_cache_tree->sha1, head_commit->tree->object.oid.hash); } +/* + * Read the author-script file into an environment block, ready for use in + * run_command(), that can be free()d afterwards. + */ +static char **read_author_script(void) +{ + struct strbuf script = STRBUF_INIT; + int i, count = 0; + char *p, *p2, **env; + size_t env_size; + + if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0) + return NULL; + + for (p = script.buf; *p; p++) + if (skip_prefix(p, "'\\\\''", (const char **)&p2)) + strbuf_splice(&script, p - script.buf, p2 - p, "'", 1); + else if (*p == '\'') + strbuf_splice(&script, p-- - script.buf, 1, "", 0); + else if (*p == '\n') { + *p = '\0'; + count++; + } + + env_size = (count + 1) * sizeof(*env); + strbuf_grow(&script, env_size); + memmove(script.buf + env_size, script.buf, script.len); + p = script.buf + env_size; + env = (char **)strbuf_detach(&script, NULL); + + for (i = 0; i < count; i++) { + env[i] = p; + p += strlen(p) + 1; + } + env[count] = NULL; + + return env; +} + /* * If we are cherry-pick, and if the merge did not result in * hand-editing, we will hit this commit and inherit the original * author date and name. + * * If we are revert, or if our cherry-pick results in a hand merge, * we had better say that the current user is responsible for that. + * + * An exception is when run_git_commit() is called during an + * interactive rebase: in that case, we will want to retain the + * author metadata. */ static int run_git_commit(const char *defmsg, struct replay_opts *opts, int allow_empty) { + char **env = NULL; struct argv_array array; int rc; const char *value; + if (is_rebase_i(opts)) { + env = read_author_script(); + if (!env) + return error("You have staged changes in your working " + "tree. If these changes are meant to be\n" + "squashed into the previous commit, run:\n\n" + " git commit --amend $gpg_sign_opt_quoted\n\n" + "If they are meant to go into a new commit, " + "run:\n\n" + " git commit $gpg_sign_opt_quoted\n\n" + "In both cases, once you're done, continue " + "with:\n\n" + " git rebase --continue\n"); + } + argv_array_init(&array); argv_array_push(&array, "commit"); argv_array_push(&array, "-n"); @@ -391,14 +464,13 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts, argv_array_pushf(&array, "-S%s", opts->gpg_sign); if (opts->signoff) argv_array_push(&array, "-s"); - if (!opts->edit) { - argv_array_push(&array, "-F"); - argv_array_push(&array, defmsg); - if (!opts->signoff && - !opts->record_origin && - git_config_get_value("commit.cleanup", &value)) - argv_array_push(&array, "--cleanup=verbatim"); - } + if (defmsg) + argv_array_pushl(&array, "-F", defmsg, NULL); + if (opts->edit) + argv_array_push(&array, "-e"); + else if (!opts->signoff && !opts->record_origin && + git_config_get_value("commit.cleanup", &value)) + argv_array_push(&array, "--cleanup=verbatim"); if (allow_empty) argv_array_push(&array, "--allow-empty"); @@ -406,8 +478,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts, if (opts->allow_empty_message) argv_array_push(&array, "--allow-empty-message"); - rc = run_command_v_opt(array.argv, RUN_GIT_CMD); + rc = run_command_v_opt_cd_env(array.argv, RUN_GIT_CMD, NULL, + (const char *const *)env); argv_array_clear(&array); + free(env); + return rc; } @@ -657,7 +732,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, goto leave; } if (!opts->no_commit) - res = run_git_commit(git_path_merge_msg(), opts, allow); + res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(), + opts, allow); leave: free_message(commit, &msg); @@ -879,6 +955,9 @@ static int populate_opts_cb(const char *key, const char *value, void *data) static int read_populate_opts(struct replay_opts *opts) { + if (is_rebase_i(opts)) + return 0; + if (!file_exists(git_path_opts_file())) return 0; /* From a45592a188841ff424780229f0f6c47e2c352665 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 19 May 2016 09:00:44 +0200 Subject: [PATCH 31/44] sequencer: introduce a helper to read files written by scripts As we are slowly teaching the sequencer to perform the hard work for the interactive rebase, we need to read files that were written by shell scripts. These files typically contain a single line and are invariably ended by a line feed (and possibly a carriage return before that). Let's use a helper to read such files and to remove the line ending. Signed-off-by: Johannes Schindelin --- sequencer.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/sequencer.c b/sequencer.c index 6d5fe9485a..282c4d19e8 100644 --- a/sequencer.c +++ b/sequencer.c @@ -234,6 +234,40 @@ static int write_message(struct strbuf *msgbuf, const char *filename) return 0; } +/* + * Reads a file that was presumably written by a shell script, i.e. with an + * end-of-line marker that needs to be stripped. + * + * Note that only the last end-of-line marker is stripped, consistent with the + * behavior of "$(cat path)" in a shell script. + * + * Returns 1 if the file was read, 0 if it could not be read or does not exist. + */ +static int read_oneliner(struct strbuf *buf, + const char *path, int skip_if_empty) +{ + int orig_len = buf->len; + + if (!file_exists(path)) + return 0; + + if (strbuf_read_file(buf, path, 0) < 0) { + warning_errno(_("could not read '%s'"), path); + return 0; + } + + if (buf->len > orig_len && buf->buf[buf->len - 1] == '\n') { + if (--buf->len > orig_len && buf->buf[buf->len - 1] == '\r') + --buf->len; + buf->buf[buf->len] = '\0'; + } + + if (skip_if_empty && buf->len == orig_len) + return 0; + + return 1; +} + static struct tree *empty_tree(void) { return lookup_tree(EMPTY_TREE_SHA1_BIN); From 57672eb1712e80d7a7f29fecf30972cf67a3cca7 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 18 May 2016 14:49:39 +0200 Subject: [PATCH 32/44] sequencer: allow editing the commit message on a case-by-case basis In the upcoming commits, we will implement more and more of rebase -i's functionality inside the sequencer. One particular feature of the commands to come is that some of them allow editing the commit message while others don't, i.e. we cannot define in the replay_opts whether the commit message should be edited or not. Let's add a new parameter to the run_git_commit() function. Previously, it was the duty of the caller to ensure that the opts->edit setting indicates whether to let the user edit the commit message or not, indicating that it is an "all or nothing" setting, i.e. that the sequencer wants to let the user edit *all* commit message, or none at all. In the upcoming rebase -i mode, it will depend on the particular command that is currently executed, though. Signed-off-by: Johannes Schindelin --- sequencer.c | 48 ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/sequencer.c b/sequencer.c index 282c4d19e8..c0a0aa0ed6 100644 --- a/sequencer.c +++ b/sequencer.c @@ -15,6 +15,7 @@ #include "merge-recursive.h" #include "refs.h" #include "argv-array.h" +#include "quote.h" #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" @@ -33,6 +34,11 @@ static GIT_PATH_FUNC(git_path_head_file, "sequencer/head") * being rebased. */ static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script") +/* + * The following files are written by git-rebase just after parsing the + * command-line (and are only consumed, not modified, by the sequencer). + */ +static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt") /* We will introduce the 'interactive rebase' mode later */ static inline int is_rebase_i(const struct replay_opts *opts) @@ -132,6 +138,16 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob, return 1; } +static const char *gpg_sign_opt_quoted(struct replay_opts *opts) +{ + static struct strbuf buf = STRBUF_INIT; + + strbuf_reset(&buf); + if (opts->gpg_sign) + sq_quotef(&buf, "-S%s", opts->gpg_sign); + return buf.buf; +} + int sequencer_remove_state(struct replay_opts *opts) { struct strbuf dir = STRBUF_INIT; @@ -468,7 +484,7 @@ static char **read_author_script(void) * author metadata. */ static int run_git_commit(const char *defmsg, struct replay_opts *opts, - int allow_empty) + int allow_empty, int edit) { char **env = NULL; struct argv_array array; @@ -477,17 +493,20 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts, if (is_rebase_i(opts)) { env = read_author_script(); - if (!env) + if (!env) { + const char *gpg_opt = gpg_sign_opt_quoted(opts); + return error("You have staged changes in your working " "tree. If these changes are meant to be\n" "squashed into the previous commit, run:\n\n" - " git commit --amend $gpg_sign_opt_quoted\n\n" + " git commit --amend %s\n\n" "If they are meant to go into a new commit, " "run:\n\n" - " git commit $gpg_sign_opt_quoted\n\n" + " git commit %s\n\n" "In both cases, once you're done, continue " "with:\n\n" - " git rebase --continue\n"); + " git rebase --continue\n", gpg_opt, gpg_opt); + } } argv_array_init(&array); @@ -500,7 +519,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts, argv_array_push(&array, "-s"); if (defmsg) argv_array_pushl(&array, "-F", defmsg, NULL); - if (opts->edit) + if (edit) argv_array_push(&array, "-e"); else if (!opts->signoff && !opts->record_origin && git_config_get_value("commit.cleanup", &value)) @@ -767,7 +786,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, } if (!opts->no_commit) res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(), - opts, allow); + opts, allow, opts->edit); leave: free_message(commit, &msg); @@ -989,8 +1008,21 @@ static int populate_opts_cb(const char *key, const char *value, void *data) static int read_populate_opts(struct replay_opts *opts) { - if (is_rebase_i(opts)) + if (is_rebase_i(opts)) { + struct strbuf buf = STRBUF_INIT; + + if (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1)) { + if (!starts_with(buf.buf, "-S")) + strbuf_reset(&buf); + else { + free(opts->gpg_sign); + opts->gpg_sign = xstrdup(buf.buf + 2); + } + } + strbuf_release(&buf); + return 0; + } if (!file_exists(git_path_opts_file())) return 0; From 7e77c6a7a755fd73b489a6c9ac895316c6af59f3 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 4 Mar 2016 15:01:48 +0100 Subject: [PATCH 33/44] sequencer: support amending commits This teaches the run_git_commit() function to take an argument that will allow us to implement "todo" commands that need to amend the commit messages ("fixup", "squash" and "reword"). Signed-off-by: Johannes Schindelin --- sequencer.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/sequencer.c b/sequencer.c index c0a0aa0ed6..1ef50a0999 100644 --- a/sequencer.c +++ b/sequencer.c @@ -484,7 +484,7 @@ static char **read_author_script(void) * author metadata. */ static int run_git_commit(const char *defmsg, struct replay_opts *opts, - int allow_empty, int edit) + int allow_empty, int edit, int amend) { char **env = NULL; struct argv_array array; @@ -513,6 +513,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts, argv_array_push(&array, "commit"); argv_array_push(&array, "-n"); + if (amend) + argv_array_push(&array, "--amend"); if (opts->gpg_sign) argv_array_pushf(&array, "-S%s", opts->gpg_sign); if (opts->signoff) @@ -786,7 +788,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, } if (!opts->no_commit) res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(), - opts, allow, opts->edit); + opts, allow, opts->edit, 0); leave: free_message(commit, &msg); From 7167c0a60835c744d291cfaffcf2ded3a8f1228c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 4 Mar 2016 15:01:48 +0100 Subject: [PATCH 34/44] sequencer: support cleaning up commit messages The run_git_commit() function already knows how to amend commits, and with this new option, it can also clean up commit messages (i.e. strip out commented lines). This is needed to implement rebase -i's 'fixup' and 'squash' commands as sequencer commands. Signed-off-by: Johannes Schindelin --- sequencer.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/sequencer.c b/sequencer.c index 1ef50a0999..8646ca56ff 100644 --- a/sequencer.c +++ b/sequencer.c @@ -484,7 +484,8 @@ static char **read_author_script(void) * author metadata. */ static int run_git_commit(const char *defmsg, struct replay_opts *opts, - int allow_empty, int edit, int amend) + int allow_empty, int edit, int amend, + int cleanup_commit_message) { char **env = NULL; struct argv_array array; @@ -521,9 +522,12 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts, argv_array_push(&array, "-s"); if (defmsg) argv_array_pushl(&array, "-F", defmsg, NULL); + if (cleanup_commit_message) + argv_array_push(&array, "--cleanup=strip"); if (edit) argv_array_push(&array, "-e"); - else if (!opts->signoff && !opts->record_origin && + else if (!cleanup_commit_message && + !opts->signoff && !opts->record_origin && git_config_get_value("commit.cleanup", &value)) argv_array_push(&array, "--cleanup=verbatim"); @@ -788,7 +792,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, } if (!opts->no_commit) res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(), - opts, allow, opts->edit, 0); + opts, allow, opts->edit, 0, 0); leave: free_message(commit, &msg); From f345f5a326d7400f7cd623fb2bfc24d407303733 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 15 Apr 2016 17:41:32 +0200 Subject: [PATCH 35/44] sequencer: left-trim lines read from the script Interactive rebase's scripts may be indented; we need to handle this case, too, now that we prepare the sequencer to process interactive rebases. Signed-off-by: Johannes Schindelin --- sequencer.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sequencer.c b/sequencer.c index 8646ca56ff..d74fdce99b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -874,6 +874,9 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol) char *end_of_object_name; int i, saved, status, padding; + /* left-trim */ + bol += strspn(bol, " \t"); + for (i = 0; i < ARRAY_SIZE(todo_command_strings); i++) if (skip_prefix(bol, todo_command_strings[i], &bol)) { item->command = i; From c3d24e13d39108f9e91e10260939758192efa31a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 21 Oct 2016 13:14:01 +0200 Subject: [PATCH 36/44] sequencer: stop releasing the strbuf in write_message() Nothing in the name "write_message()" suggests that the function releases the strbuf passed to it. So let's release the strbuf in the caller instead. Signed-off-by: Johannes Schindelin --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index d74fdce99b..745c86f654 100644 --- a/sequencer.c +++ b/sequencer.c @@ -243,7 +243,6 @@ static int write_message(struct strbuf *msgbuf, const char *filename) return error_errno(_("Could not lock '%s'"), filename); if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0) return error_errno(_("Could not write to %s"), filename); - strbuf_release(msgbuf); if (commit_lock_file(&msg_file) < 0) return error(_("Error wrapping up %s."), filename); @@ -759,6 +758,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, free_commit_list(common); free_commit_list(remotes); } + strbuf_release(&msgbuf); /* * If the merge was clean or if it failed due to conflict, we write From fad8da54b88c6f5ea15a9dcd4cf80c966ae6fb7f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 21 Oct 2016 13:19:25 +0200 Subject: [PATCH 37/44] sequencer: roll back lock file if write_message() failed There is no need to wait until the atexit() handler kicks in at the end. Signed-off-by: Johannes Schindelin --- sequencer.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/sequencer.c b/sequencer.c index 745c86f654..9fced42dff 100644 --- a/sequencer.c +++ b/sequencer.c @@ -241,10 +241,14 @@ static int write_message(struct strbuf *msgbuf, const char *filename) int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0); if (msg_fd < 0) return error_errno(_("Could not lock '%s'"), filename); - if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0) - return error_errno(_("Could not write to %s"), filename); - if (commit_lock_file(&msg_file) < 0) + if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0) { + rollback_lock_file(&msg_file); + return error_errno(_("Could not write to '%s'"), filename); + } + if (commit_lock_file(&msg_file) < 0) { + rollback_lock_file(&msg_file); return error(_("Error wrapping up %s."), filename); + } return 0; } From 4562e3aeea7aedfc500c0e80981dfaea79818968 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 21 Oct 2016 13:23:00 +0200 Subject: [PATCH 38/44] sequencer: refactor write_message() to take a pointer/length Previously, we required an strbuf. But that limits the use case too much. In the upcoming patch series (for which the current patch series prepares the sequencer), we will want to write content to a file for which we have a pointer and a length, not an strbuf. Signed-off-by: Johannes Schindelin --- sequencer.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/sequencer.c b/sequencer.c index 9fced42dff..300952fc16 100644 --- a/sequencer.c +++ b/sequencer.c @@ -234,14 +234,14 @@ static void print_advice(int show_hint, struct replay_opts *opts) } } -static int write_message(struct strbuf *msgbuf, const char *filename) +static int write_message(const void *buf, size_t len, const char *filename) { static struct lock_file msg_file; int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0); if (msg_fd < 0) return error_errno(_("Could not lock '%s'"), filename); - if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0) { + if (write_in_full(msg_fd, buf, len) < 0) { rollback_lock_file(&msg_file); return error_errno(_("Could not write to '%s'"), filename); } @@ -747,12 +747,14 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, head, &msgbuf, opts); if (res < 0) return res; - res |= write_message(&msgbuf, git_path_merge_msg()); + res |= write_message(msgbuf.buf, msgbuf.len, + git_path_merge_msg()); } else { struct commit_list *common = NULL; struct commit_list *remotes = NULL; - res = write_message(&msgbuf, git_path_merge_msg()); + res = write_message(msgbuf.buf, msgbuf.len, + git_path_merge_msg()); commit_list_insert(base, &common); commit_list_insert(next, &remotes); From 8dd3db0d38c56e28a5062cbb95d00677584eb286 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 21 Oct 2016 13:26:36 +0200 Subject: [PATCH 39/44] sequencer: teach write_message() to append an optional LF This commit prepares for future callers that will have a pointer/length to some text to be written that lacks an LF, yet an LF is desired. Instead of requiring the caller to append an LF to the buffer (and potentially allocate memory to do so), the write_message() function learns to append an LF at the end of the file. Signed-off-by: Johannes Schindelin --- sequencer.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/sequencer.c b/sequencer.c index 300952fc16..000ce3e541 100644 --- a/sequencer.c +++ b/sequencer.c @@ -234,7 +234,8 @@ static void print_advice(int show_hint, struct replay_opts *opts) } } -static int write_message(const void *buf, size_t len, const char *filename) +static int write_message(const void *buf, size_t len, const char *filename, + int append_eol) { static struct lock_file msg_file; @@ -245,6 +246,10 @@ static int write_message(const void *buf, size_t len, const char *filename) rollback_lock_file(&msg_file); return error_errno(_("Could not write to '%s'"), filename); } + if (append_eol && write(msg_fd, "\n", 1) < 0) { + rollback_lock_file(&msg_file); + return error_errno(_("Could not write eol to '%s"), filename); + } if (commit_lock_file(&msg_file) < 0) { rollback_lock_file(&msg_file); return error(_("Error wrapping up %s."), filename); @@ -748,13 +753,13 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, if (res < 0) return res; res |= write_message(msgbuf.buf, msgbuf.len, - git_path_merge_msg()); + git_path_merge_msg(), 0); } else { struct commit_list *common = NULL; struct commit_list *remotes = NULL; res = write_message(msgbuf.buf, msgbuf.len, - git_path_merge_msg()); + git_path_merge_msg(), 0); commit_list_insert(base, &common); commit_list_insert(next, &remotes); From 4a6513a255d632c3acddb4d11fa9e2d763dea607 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 23 Feb 2016 19:21:31 +0100 Subject: [PATCH 40/44] sequencer: remove overzealous assumption in rebase -i mode The sequencer was introduced to make the cherry-pick and revert functionality available as library function, with the original idea being to extend the sequencer to also implement the rebase -i functionality. The test to ensure that all of the commands in the script are identical to the overall operation does not mesh well with that. Therefore let's disable the test in rebase -i mode. While at it, error out early if the "instruction sheet" (i.e. the todo script) could not be parsed. Signed-off-by: Johannes Schindelin --- sequencer.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/sequencer.c b/sequencer.c index 000ce3e541..bd11db4273 100644 --- a/sequencer.c +++ b/sequencer.c @@ -962,7 +962,10 @@ static int read_populate_todo(struct todo_list *todo_list, close(fd); res = parse_insn_buffer(todo_list->buf.buf, todo_list); - if (!res) { + if (res) + return error(_("Unusable instruction sheet: %s"), todo_file); + + if (!is_rebase_i(opts)) { enum todo_command valid = opts->action == REPLAY_PICK ? TODO_PICK : TODO_REVERT; int i; @@ -976,8 +979,6 @@ static int read_populate_todo(struct todo_list *todo_list, return error(_("Cannot revert during a cherry-pick.")); } - if (res) - return error(_("Unusable instruction sheet: %s"), todo_file); return 0; } From 524d1738f17b55faefc4882c1c3504331851d403 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:20:26 +0200 Subject: [PATCH 41/44] sequencer: mark action_name() for translation The definition of this function goes back all the way to 043a449 (sequencer: factor code out of revert builtin, 2012-01-11), long before a serious effort was made to translate all the error messages. It is slightly out of the context of the current patch series (whose purpose it is to re-implement the performance critical parts of the interactive rebase in C) to make the error messages in the sequencer translatable, but what the heck. We'll just do it while we're looking at this part of the code. Signed-off-by: Johannes Schindelin --- sequencer.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/sequencer.c b/sequencer.c index bd11db4273..ff76b6f61d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -168,7 +168,7 @@ int sequencer_remove_state(struct replay_opts *opts) static const char *action_name(const struct replay_opts *opts) { - return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick"; + return opts->action == REPLAY_REVERT ? N_("revert") : N_("cherry-pick"); } struct commit_message { @@ -300,10 +300,10 @@ static struct tree *empty_tree(void) static int error_dirty_index(struct replay_opts *opts) { if (read_cache_unmerged()) - return error_resolve_conflict(action_name(opts)); + return error_resolve_conflict(_(action_name(opts))); error(_("Your local changes would be overwritten by %s."), - action_name(opts)); + _(action_name(opts))); if (advice_commit_before_merge) advise(_("Commit your changes or stash them to proceed.")); @@ -321,7 +321,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from, if (checkout_fast_forward(from, to, 1)) return -1; /* the callee should have complained already */ - strbuf_addf(&sb, _("%s: fast-forward"), action_name(opts)); + strbuf_addf(&sb, _("%s: fast-forward"), _(action_name(opts))); transaction = ref_transaction_begin(&err); if (!transaction || @@ -397,7 +397,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next, write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) /* TRANSLATORS: %s will be "revert" or "cherry-pick" */ return error(_("%s: Unable to write new index file"), - action_name(opts)); + _(action_name(opts))); rollback_lock_file(&index_lock); if (opts->signoff) @@ -835,14 +835,14 @@ static int read_and_refresh_cache(struct replay_opts *opts) if (read_index_preload(&the_index, NULL) < 0) { rollback_lock_file(&index_lock); return error(_("git %s: failed to read the index"), - action_name(opts)); + _(action_name(opts))); } refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL); if (the_index.cache_changed && index_fd >= 0) { if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) { rollback_lock_file(&index_lock); return error(_("git %s: failed to refresh the index"), - action_name(opts)); + _(action_name(opts))); } } rollback_lock_file(&index_lock); From fea530966a46b7394a6c583a14b78942f56ea9c5 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 9 Sep 2016 16:46:21 +0200 Subject: [PATCH 42/44] sequencer: quote filenames in error messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes the code consistent by fixing quite a couple of error messages. Suggested by Jakub Narębski. Signed-off-by: Johannes Schindelin --- sequencer.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/sequencer.c b/sequencer.c index ff76b6f61d..340d0edf42 100644 --- a/sequencer.c +++ b/sequencer.c @@ -252,7 +252,7 @@ static int write_message(const void *buf, size_t len, const char *filename, } if (commit_lock_file(&msg_file) < 0) { rollback_lock_file(&msg_file); - return error(_("Error wrapping up %s."), filename); + return error(_("Error wrapping up '%s'."), filename); } return 0; @@ -954,16 +954,16 @@ static int read_populate_todo(struct todo_list *todo_list, strbuf_reset(&todo_list->buf); fd = open(todo_file, O_RDONLY); if (fd < 0) - return error_errno(_("Could not open %s"), todo_file); + return error_errno(_("Could not open '%s'"), todo_file); if (strbuf_read(&todo_list->buf, fd, 0) < 0) { close(fd); - return error(_("Could not read %s."), todo_file); + return error(_("Could not read '%s'."), todo_file); } close(fd); res = parse_insn_buffer(todo_list->buf.buf, todo_list); if (res) - return error(_("Unusable instruction sheet: %s"), todo_file); + return error(_("Unusable instruction sheet: '%s'"), todo_file); if (!is_rebase_i(opts)) { enum todo_command valid = @@ -1054,7 +1054,7 @@ static int read_populate_opts(struct replay_opts *opts) * are pretty certain that it is syntactically correct. */ if (git_config_from_file(populate_opts_cb, git_path_opts_file(), opts) < 0) - return error(_("Malformed options sheet: %s"), + return error(_("Malformed options sheet: '%s'"), git_path_opts_file()); return 0; } @@ -1097,7 +1097,7 @@ static int create_seq_dir(void) return -1; } else if (mkdir(git_path_seq_dir(), 0777) < 0) - return error_errno(_("Could not create sequencer directory %s"), + return error_errno(_("Could not create sequencer directory '%s'"), git_path_seq_dir()); return 0; } @@ -1116,12 +1116,12 @@ static int save_head(const char *head) strbuf_addf(&buf, "%s\n", head); if (write_in_full(fd, buf.buf, buf.len) < 0) { rollback_lock_file(&head_lock); - return error_errno(_("Could not write to %s"), + return error_errno(_("Could not write to '%s'"), git_path_head_file()); } if (commit_lock_file(&head_lock) < 0) { rollback_lock_file(&head_lock); - return error(_("Error wrapping up %s."), git_path_head_file()); + return error(_("Error wrapping up '%s'."), git_path_head_file()); } return 0; } @@ -1166,9 +1166,9 @@ int sequencer_rollback(struct replay_opts *opts) return rollback_single_pick(); } if (!f) - return error_errno(_("cannot open %s"), git_path_head_file()); + return error_errno(_("cannot open '%s'"), git_path_head_file()); if (strbuf_getline_lf(&buf, f)) { - error(_("cannot read %s: %s"), git_path_head_file(), + error(_("cannot read '%s': %s"), git_path_head_file(), ferror(f) ? strerror(errno) : _("unexpected end of file")); fclose(f); goto fail; @@ -1207,7 +1207,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts) todo_list->buf.len - offset) < 0) return error_errno(_("Could not write to '%s'"), todo_path); if (commit_lock_file(&todo_lock) < 0) - return error(_("Error wrapping up %s."), todo_path); + return error(_("Error wrapping up '%s'."), todo_path); return 0; } From 93e7bcf4895d33b74146288589041f21a260c704 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 6 Oct 2016 15:38:09 +0200 Subject: [PATCH 43/44] sequencer: start error messages consistently with lower case Quite a few error messages touched by this developer during the work to speed up rebase -i started with an upper case letter, violating our current conventions. Instead of sneaking in this fix (and forgetting quite a few error messages), let's just have one wholesale patch fixing all of the error messages in the sequencer. While at it, the funny "error: Error wrapping up..." was changed to a less funny, but more helpful, "error: failed to finalize...". Pointed out by Junio Hamano. Signed-off-by: Johannes Schindelin --- sequencer.c | 68 +++++++++++++++++------------------ t/t3501-revert-cherry-pick.sh | 2 +- 2 files changed, 35 insertions(+), 35 deletions(-) diff --git a/sequencer.c b/sequencer.c index 340d0edf42..4c10c9355b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -241,18 +241,18 @@ static int write_message(const void *buf, size_t len, const char *filename, int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0); if (msg_fd < 0) - return error_errno(_("Could not lock '%s'"), filename); + return error_errno(_("could not lock '%s'"), filename); if (write_in_full(msg_fd, buf, len) < 0) { rollback_lock_file(&msg_file); - return error_errno(_("Could not write to '%s'"), filename); + return error_errno(_("could not write to '%s'"), filename); } if (append_eol && write(msg_fd, "\n", 1) < 0) { rollback_lock_file(&msg_file); - return error_errno(_("Could not write eol to '%s"), filename); + return error_errno(_("could not write eol to '%s"), filename); } if (commit_lock_file(&msg_file) < 0) { rollback_lock_file(&msg_file); - return error(_("Error wrapping up '%s'."), filename); + return error(_("failed to finalize '%s'."), filename); } return 0; @@ -302,11 +302,11 @@ static int error_dirty_index(struct replay_opts *opts) if (read_cache_unmerged()) return error_resolve_conflict(_(action_name(opts))); - error(_("Your local changes would be overwritten by %s."), + error(_("your local changes would be overwritten by %s."), _(action_name(opts))); if (advice_commit_before_merge) - advise(_("Commit your changes or stash them to proceed.")); + advise(_("commit your changes or stash them to proceed.")); return -1; } @@ -415,7 +415,7 @@ static int is_index_unchanged(void) struct commit *head_commit; if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, head_sha1, NULL)) - return error(_("Could not resolve HEAD commit\n")); + return error(_("could not resolve HEAD commit\n")); head_commit = lookup_commit(head_sha1); @@ -435,7 +435,7 @@ static int is_index_unchanged(void) if (!cache_tree_fully_valid(active_cache_tree)) if (cache_tree_update(&the_index, 0)) - return error(_("Unable to update cache tree\n")); + return error(_("unable to update cache tree\n")); return !hashcmp(active_cache_tree->sha1, head_commit->tree->object.oid.hash); } @@ -505,7 +505,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts, if (!env) { const char *gpg_opt = gpg_sign_opt_quoted(opts); - return error("You have staged changes in your working " + return error("you have staged changes in your working " "tree. If these changes are meant to be\n" "squashed into the previous commit, run:\n\n" " git commit --amend %s\n\n" @@ -558,12 +558,12 @@ static int is_original_commit_empty(struct commit *commit) const unsigned char *ptree_sha1; if (parse_commit(commit)) - return error(_("Could not parse commit %s\n"), + return error(_("could not parse commit %s\n"), oid_to_hex(&commit->object.oid)); if (commit->parents) { struct commit *parent = commit->parents->item; if (parse_commit(parent)) - return error(_("Could not parse parent commit %s\n"), + return error(_("could not parse parent commit %s\n"), oid_to_hex(&parent->object.oid)); ptree_sha1 = parent->tree->object.oid.hash; } else { @@ -647,7 +647,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, * to work on. */ if (write_cache_as_tree(head, 0, NULL)) - return error(_("Your index file is unmerged.")); + return error(_("your index file is unmerged.")); } else { unborn = get_sha1("HEAD", head); if (unborn) @@ -666,7 +666,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, struct commit_list *p; if (!opts->mainline) - return error(_("Commit %s is a merge but no -m option was given."), + return error(_("commit %s is a merge but no -m option was given."), oid_to_hex(&commit->object.oid)); for (cnt = 1, p = commit->parents; @@ -674,11 +674,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, cnt++) p = p->next; if (cnt != opts->mainline || !p) - return error(_("Commit %s does not have parent %d"), + return error(_("commit %s does not have parent %d"), oid_to_hex(&commit->object.oid), opts->mainline); parent = p->item; } else if (0 < opts->mainline) - return error(_("Mainline was specified but commit %s is not a merge."), + return error(_("mainline was specified but commit %s is not a merge."), oid_to_hex(&commit->object.oid)); else parent = commit->parents->item; @@ -696,7 +696,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, oid_to_hex(&parent->object.oid)); if (get_message(commit, &msg) != 0) - return error(_("Cannot get commit message for %s"), + return error(_("cannot get commit message for %s"), oid_to_hex(&commit->object.oid)); /* @@ -935,13 +935,13 @@ static int parse_insn_buffer(char *buf, struct todo_list *todo_list) item = append_new_todo(todo_list); item->offset_in_buf = p - todo_list->buf.buf; if (parse_insn_line(item, p, eol)) { - res = error(_("Invalid line %d: %.*s"), + res = error(_("invalid line %d: %.*s"), i, (int)(eol - p), p); item->command = -1; } } if (!todo_list->nr) - return error(_("No commits parsed.")); + return error(_("no commits parsed.")); return res; } @@ -954,16 +954,16 @@ static int read_populate_todo(struct todo_list *todo_list, strbuf_reset(&todo_list->buf); fd = open(todo_file, O_RDONLY); if (fd < 0) - return error_errno(_("Could not open '%s'"), todo_file); + return error_errno(_("could not open '%s'"), todo_file); if (strbuf_read(&todo_list->buf, fd, 0) < 0) { close(fd); - return error(_("Could not read '%s'."), todo_file); + return error(_("could not read '%s'."), todo_file); } close(fd); res = parse_insn_buffer(todo_list->buf.buf, todo_list); if (res) - return error(_("Unusable instruction sheet: '%s'"), todo_file); + return error(_("unusable instruction sheet: '%s'"), todo_file); if (!is_rebase_i(opts)) { enum todo_command valid = @@ -974,9 +974,9 @@ static int read_populate_todo(struct todo_list *todo_list, if (valid == todo_list->items[i].command) continue; else if (valid == TODO_PICK) - return error(_("Cannot cherry-pick during a revert.")); + return error(_("cannot cherry-pick during a revert.")); else - return error(_("Cannot revert during a cherry-pick.")); + return error(_("cannot revert during a cherry-pick.")); } return 0; @@ -1019,10 +1019,10 @@ static int populate_opts_cb(const char *key, const char *value, void *data) ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc); opts->xopts[opts->xopts_nr++] = xstrdup(value); } else - return error(_("Invalid key: %s"), key); + return error(_("invalid key: %s"), key); if (!error_flag) - return error(_("Invalid value for %s: %s"), key, value); + return error(_("invalid value for %s: %s"), key, value); return 0; } @@ -1054,7 +1054,7 @@ static int read_populate_opts(struct replay_opts *opts) * are pretty certain that it is syntactically correct. */ if (git_config_from_file(populate_opts_cb, git_path_opts_file(), opts) < 0) - return error(_("Malformed options sheet: '%s'"), + return error(_("malformed options sheet: '%s'"), git_path_opts_file()); return 0; } @@ -1097,7 +1097,7 @@ static int create_seq_dir(void) return -1; } else if (mkdir(git_path_seq_dir(), 0777) < 0) - return error_errno(_("Could not create sequencer directory '%s'"), + return error_errno(_("could not create sequencer directory '%s'"), git_path_seq_dir()); return 0; } @@ -1111,17 +1111,17 @@ static int save_head(const char *head) fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0); if (fd < 0) { rollback_lock_file(&head_lock); - return error_errno(_("Could not lock HEAD")); + return error_errno(_("could not lock HEAD")); } strbuf_addf(&buf, "%s\n", head); if (write_in_full(fd, buf.buf, buf.len) < 0) { rollback_lock_file(&head_lock); - return error_errno(_("Could not write to '%s'"), + return error_errno(_("could not write to '%s'"), git_path_head_file()); } if (commit_lock_file(&head_lock) < 0) { rollback_lock_file(&head_lock); - return error(_("Error wrapping up '%s'."), git_path_head_file()); + return error(_("failed to finalize '%s'."), git_path_head_file()); } return 0; } @@ -1200,14 +1200,14 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts) fd = hold_lock_file_for_update(&todo_lock, todo_path, 0); if (fd < 0) - return error_errno(_("Could not lock '%s'"), todo_path); + return error_errno(_("could not lock '%s'"), todo_path); offset = next < todo_list->nr ? todo_list->items[next].offset_in_buf : todo_list->buf.len; if (write_in_full(fd, todo_list->buf.buf + offset, todo_list->buf.len - offset) < 0) - return error_errno(_("Could not write to '%s'"), todo_path); + return error_errno(_("could not write to '%s'"), todo_path); if (commit_lock_file(&todo_lock) < 0) - return error(_("Error wrapping up '%s'."), todo_path); + return error(_("failed to finalize '%s'."), todo_path); return 0; } @@ -1382,7 +1382,7 @@ int sequencer_pick_revisions(struct replay_opts *opts) create_seq_dir() < 0) return -1; if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT)) - return error(_("Can't revert as initial commit")); + return error(_("can't revert as initial commit")); if (save_head(sha1_to_hex(sha1))) return -1; if (save_opts(opts)) diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh index 51f3bbb8af..394f0005a1 100755 --- a/t/t3501-revert-cherry-pick.sh +++ b/t/t3501-revert-cherry-pick.sh @@ -96,7 +96,7 @@ test_expect_success 'revert forbidden on dirty working tree' ' echo content >extra_file && git add extra_file && test_must_fail git revert HEAD 2>errors && - test_i18ngrep "Your local changes would be overwritten by " errors + test_i18ngrep "your local changes would be overwritten by " errors ' From 869d20dfbd319cb1d3e6531e89166fca43673ea9 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 10 Oct 2016 17:17:14 +0200 Subject: [PATCH 44/44] sequencer: mark all error messages for translation There was actually only one error message that was not yet marked for translation. Signed-off-by: Johannes Schindelin --- sequencer.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/sequencer.c b/sequencer.c index 4c10c9355b..a61fe76f98 100644 --- a/sequencer.c +++ b/sequencer.c @@ -479,6 +479,20 @@ static char **read_author_script(void) return env; } +static const char staged_changes_advice[] = +N_("you have staged changes in your working tree\n" +"If these changes are meant to be squashed into the previous commit, run:\n" +"\n" +" git commit --amend %s\n" +"\n" +"If they are meant to go into a new commit, run:\n" +"\n" +" git commit %s\n" +"\n" +"In both cases, once you're done, continue with:\n" +"\n" +" git rebase --continue\n"); + /* * If we are cherry-pick, and if the merge did not result in * hand-editing, we will hit this commit and inherit the original @@ -505,16 +519,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts, if (!env) { const char *gpg_opt = gpg_sign_opt_quoted(opts); - return error("you have staged changes in your working " - "tree. If these changes are meant to be\n" - "squashed into the previous commit, run:\n\n" - " git commit --amend %s\n\n" - "If they are meant to go into a new commit, " - "run:\n\n" - " git commit %s\n\n" - "In both cases, once you're done, continue " - "with:\n\n" - " git rebase --continue\n", gpg_opt, gpg_opt); + return error(_(staged_changes_advice), + gpg_opt, gpg_opt); } }