From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-4.0 required=3.0 tests=AWL,BAYES_00, FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS, RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD shortcircuit=no autolearn=ham autolearn_force=no version=3.4.0 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id 26D8D1F4F8 for ; Fri, 14 Oct 2016 13:20:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932779AbcJNNUM (ORCPT ); Fri, 14 Oct 2016 09:20:12 -0400 Received: from mout.gmx.net ([212.227.17.21]:54796 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755785AbcJNNSK (ORCPT ); Fri, 14 Oct 2016 09:18:10 -0400 Received: from virtualbox ([37.24.142.40]) by mail.gmx.com (mrgmx101) with ESMTPSA (Nemesis) id 0MQeET-1cOyLC1Kk1-00U25J; Fri, 14 Oct 2016 15:17:51 +0200 Date: Fri, 14 Oct 2016 15:17:50 +0200 (CEST) From: Johannes Schindelin X-X-Sender: virtualbox@virtualbox To: git@vger.kernel.org cc: Junio C Hamano , =?UTF-8?Q?Jakub_Nar=C4=99bski?= , Johannes Sixt , Ramsay Jones Subject: [PATCH v4 08/25] sequencer: completely revamp the "todo" script parsing In-Reply-To: Message-ID: References: User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Provags-ID: V03:K0:KduciZrcqZAbiJdR2x1dirRglN4MmYWrOU11UhbAPT254BXa9jt qWcOUCPq3TNGdoG2zeQ4T4zcaDdVBQAyShysHiT5tOLYn1GgwE6H6IR/S58fw/N8dTM4gQa gQO2NOcd2gav62cWNKUTyUpzqzC+AIFDaT9pl+i9OvQKAz2zyDtmXzUU7obvGkZPem5wBq2 u1BBsrOVE466uqlkpxMsw== X-UI-Out-Filterresults: notjunk:1;V01:K0:nt8Z4PLyeyo=:nnqdx7fgsHQCOCG9AFyVhe As+feLJwcJEX5KKGhNUdxLglczVILviAUoW4E4RJ1z90No/czalH2rZUus6YjOuSsvwg7R6t4 WdM3SaN4CNy7EdbKcMywJseQBBYp9yHaau8JXKWxjqsemmtcIRZv74al5JY5abnquQFb/WQFZ W9FxlXxWxwc9pTRJOCqyF8AVJlVD8ucoUzK6ksF6gjpwAlEf19Tinj84Mfx0ZKdHTepmVfWIa XSln2DP0GJIW/TUMpj8Vb0fSOaLAmRNM1Mp/wkGuktlq8dgu0QaIUnGIQuzupxAbj9bf0hu4P +6IN27bRX8AvU1hJlnNUcDHVYwpBdflkK+VWC+d0RI/qs3uP2dxmIQjkk4lA1RqiuU7U00/8O cbSjP7LYuo84TsXLSrs/ClAVlfNmDoJWw0BnuQMRSpj6pgzCSAk2Nh/VB1bb0XaIZUT4vZYqN +dYlSw419py+y/uQWG0F3KbvKxfDM1edwTNjXpazEK0LF2sky3XwRvOSCFaLdqdVJNCDfZyNb CqtesmHS97QAQUlL9qSzXq1H80UVTnok55o+BODeBxWT1zLTdyDJajn04hgZnCp7ZbxZg8fPC FbDOSSBxTezg704CjY8tw53HhY46RQKFKaayzqjl9cxQfD5gY/89RXw50xlReSkb1+st8+oBH LdP4tpT4zUTrzUlAIcUOM1g5UuAGJZadaX8nzkCZkWVfldGAHC2IJkWP6OjqMDpjKCcb4dfcA kKDE1xd18DBTtnoWDOghw8HMJnukEO2kZ4It/p//h8wJoY6b17ij9OGBwOdTrl+6dQ0cTJOjr 7/KFxuC Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 499f5ee..f797e8a 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 todo_item { + enum todo_command command; + struct commit *commit; + size_t offset_in_buf; +}; + +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) { - 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; + strbuf_release(&todo_list->buf); + free(todo_list->items); + todo_list->items = NULL; + todo_list->nr = todo_list->alloc = 0; +} - 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; +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 struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts) +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; - - 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; + int i, saved, status, padding; + + 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 /* strip 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 @@ static int sequencer_rollback(struct replay_opts *opts) 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) -- 2.10.1.513.g00ef6dd