Re: What's cooking in git.git (Dec 2018, #01; Sun, 9)
Hi Junio, Le 09/12/2018 à 09:42, Junio C Hamano a écrit :> -%<- > * ag/sequencer-reduce-rewriting-todo (2018-11-12) 16 commits > . rebase--interactive: move transform_todo_file() to rebase--interactive.c > . sequencer: fix a call to error() in transform_todo_file() > . sequencer: use edit_todo_list() in complete_action() > . rebase-interactive: rewrite edit_todo_list() to handle the initial edit > . rebase-interactive: append_todo_help() changes > . rebase-interactive: use todo_list_write_to_file() in edit_todo_list() > . sequencer: refactor skip_unnecessary_picks() to work on a todo_list > . sequencer: change complete_action() to use the refactored functions > . sequencer: make sequencer_make_script() write its script to a strbuf > . sequencer: refactor rearrange_squash() to work on a todo_list > . sequencer: refactor sequencer_add_exec_commands() to work on a todo_list > . sequencer: refactor check_todo_list() to work on a todo_list > . sequencer: introduce todo_list_write_to_file() > . sequencer: refactor transform_todos() to work on a todo_list > . sequencer: make the todo_list structure public > . sequencer: changes in parse_insn_buffer() > > The scripted version of "git rebase -i" wrote and rewrote the todo > list many times during a single step of its operation, and the > recent C-rewrite made a faithful conversion of the logic to C. The > implementation has been updated to carry necessary information > around in-core to avoid rewriting the same file over and over > unnecessarily. > > With too many topics in-flight that touch sequencer and rebaser, > this need to wait giving precedence to other topics that fix bugs. > If I’m not mistaken, there is currently 3 others series touching rebase, rebase-interactive and/or the sequencer: en/rebase-merge-on-sequencer, nd/the-index, and the new js/rebase-i-redo-exec. Among these, I believe only nd/the-index actually touches the same places as ag/sequencer-reduce-rewriting-todo, and is not a behaviour change. Is this safe to reroll this in the next few days? -- Alban
ag/sequencer-reduce-rewriting-todo, was Re: What's cooking in git.git (Dec 2018, #02; Fri, 28)
Hi Junio, Le 28/12/2018 à 19:04, Junio C Hamano a écrit : > * ag/sequencer-reduce-rewriting-todo (2018-11-12) 16 commits > . rebase--interactive: move transform_todo_file() to rebase--interactive.c > . sequencer: fix a call to error() in transform_todo_file() > . sequencer: use edit_todo_list() in complete_action() > . rebase-interactive: rewrite edit_todo_list() to handle the initial edit > . rebase-interactive: append_todo_help() changes > . rebase-interactive: use todo_list_write_to_file() in edit_todo_list() > . sequencer: refactor skip_unnecessary_picks() to work on a todo_list > . sequencer: change complete_action() to use the refactored functions > . sequencer: make sequencer_make_script() write its script to a strbuf > . sequencer: refactor rearrange_squash() to work on a todo_list > . sequencer: refactor sequencer_add_exec_commands() to work on a todo_list > . sequencer: refactor check_todo_list() to work on a todo_list > . sequencer: introduce todo_list_write_to_file() > . sequencer: refactor transform_todos() to work on a todo_list > . sequencer: make the todo_list structure public > . sequencer: changes in parse_insn_buffer() > > The scripted version of "git rebase -i" wrote and rewrote the todo > list many times during a single step of its operation, and the > recent C-rewrite made a faithful conversion of the logic to C. The > implementation has been updated to carry necessary information > around in-core to avoid rewriting the same file over and over > unnecessarily. > > With too many topics in-flight that touch sequencer and rebaser, > this need to wait giving precedence to other topics that fix bugs. > > Most of these topics have reached master and have been released in git 2.20. Currently, there is four topics actually touching rebase, interactive rebase and/or the sequencer (js/rebase-i-redo-exec, nd/backup-log, en/rebase-merge-on-sequencer and nd/the-index). Among these, only nd/the-index conflicts with my series. Should I consider this comment as outdated, and reroll my series (rebased on top of nd/the-index) in the next few days? Cheers, Alban
Re: [PATCH v3 06/16] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
Hi Phillip and Johannes, many thanks for your suggestions and feedback, I really appreciate it. Le 10/12/2018 à 15:33, Phillip Wood a écrit : > On 30/11/2018 19:06, Johannes Schindelin wrote: >> Hi, >> >> On Fri, 30 Nov 2018, Phillip Wood wrote: >> diff --git a/sequencer.c b/sequencer.c index 900899ef20..11692d0b98 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4394,24 +4394,29 @@ int sequencer_make_script(FILE *out, int argc, const char **argv, return 0; } -/* - * Add commands after pick and (series of) squash/fixup commands - * in the todo list. - */ -int sequencer_add_exec_commands(const char *commands) +static void todo_list_add_exec_commands(struct todo_list *todo_list, + struct string_list *commands) { - const char *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; - struct strbuf *buf = &todo_list.buf; - size_t offset = 0, commands_len = strlen(commands); - int i, insert; + struct strbuf *buf = &todo_list->buf; + const char *old_buf = buf->buf; + size_t base_length = buf->len; + int i, insert, nr = 0, alloc = 0; + struct todo_item *items = NULL, *base_items = NULL; - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) - return error(_("could not read '%s'."), todo_file); + for (i = 0; i < commands->nr; ++i) { + strbuf_addstr(buf, commands->items[i].string); + strbuf_addch(buf, '\n'); + } >>> >>> This could cause buf->buf to be reallocated in which case all the >>> todo_list_item.arg pointers will be invalidated. >> >> I guess it is a good time for me to admit that the `arg` idea was not my >> best. Maybe it would be good to convert that from a pointer to an offset, >> e.g. `size_t arg_offset_in_buf;`? Or maybe we should just drop the >> `_in_buf` suffix and clarify in a comment next to the declaration of the >> fields that they are offsets in the strbuf? > > I think that sounds sensible (though I haven't looked at how much work > it would be), having a comment and calling it arg_offset would be my > preference. > It’s not a lot of work, actually. Most usages of 'arg' are concentrated in two functions (parse_insn_line() and pick_commits()). Some of the subsequent patches of this series also use 'arg', and adapting them is trivial. In the end, most of the work went into todo_list_add_exec_commands(), and the result is pretty clean. Cheers, Alban
[PATCH v4 01/16] sequencer: changes in parse_insn_buffer()
This clears the number of items of a todo_list before parsing it to allow to parse the same list multiple times without issues. As its items are not dynamically allocated, or don’t need to allocate memory, no additionnal memory management is required here. Furthermore, if a line is invalid, the type of the corresponding command is set to a garbage value, and its argument is defined properly. This will allow to recreate the text of a todo list from its commands, even if one of them is incorrect. Signed-off-by: Alban Gruin --- The full diff changed due to the conflict resolution with nd/the-index, but the "core change" is the same as in the v3. sequencer.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index d726f77e11..a7afaf6882 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2091,6 +2091,8 @@ static int parse_insn_buffer(struct repository *r, char *buf, char *p = buf, *next_p; int i, res = 0, fixup_okay = file_exists(rebase_path_done()); + todo_list->current = todo_list->nr = 0; + for (i = 1; *p; i++, p = next_p) { char *eol = strchrnul(p, '\n'); @@ -2104,7 +2106,10 @@ static int parse_insn_buffer(struct repository *r, char *buf, if (parse_insn_line(r, item, p, eol)) { res = error(_("invalid line %d: %.*s"), i, (int)(eol - p), p); - item->command = TODO_NOOP; + item->command = TODO_COMMENT + 1; + item->arg = p; + item->arg_len = (int)(eol - p); + item->commit = NULL; } if (fixup_okay) -- 2.20.1
[PATCH v4 00/16] sequencer: refactor functions working on a todo_list
At the center of the "interactive" part of the interactive rebase lies the todo list. When the user starts an interactive rebase, a todo list is generated, presented to the user (who then edits it using a text editor), read back, and then is checked and processed before the actual rebase takes place. Some of this processing includes adding execs commands, reordering fixup! and squash! commits, and checking if no commits were accidentally dropped by the user. Before I converted the interactive rebase in C, these functions were called by git-rebase--interactive.sh through git-rebase--helper. Since the only way to pass around a large amount of data between a shell script and a C program is to use a file (or any declination of a file), the functions that checked and processed the todo list were directly working on a file, the same file that the user edited. During the conversion, I did not address this issue, which lead to a complete_action() that reads the todo list file, does some computation based on its content, and writes it back to the disk, several times in the same function. As it is not an efficient way to handle a data structure, this patch series refactor the functions that processes the todo list to work on a todo_list structure instead of reading it from the disk. Some commits consists in modifying edit_todo_list() (initially used by --edit-todo) to handle the initial edition of the todo list, to increase code sharing. This is based on nd/the-index (36e7ed69de, "rebase-interactive.c: remove the_repository references"), as it introduced a lot of conflicts. The result does not conflict with pu (e31bc98f4b, "Merge branch 'md/list-objects-filter-by-depth' into pu"). Changes since v3: - Replacing the 'arg' field from todo_item by 'arg_offset' to avoid dealing with pointers on the todo list buffer in todo_list_add_exec_commands(). This has led to some additionnal changes. - Rewording some commits. - Dropping the commit "sequencer: fix a call to error() in transform_todo_file()". The call to error() after reading the todo file is replaced by error_errno() in "sequencer: refactor transform_todos() to work on a todo_list". The same change has been applied to sequencer_add_exec_commands() in "sequencer: refactor sequencer_add_exec_commands() to work on a todo_list". - transform_todo_file(), sequencer_add_exec_commands() and rearrange_squash_in_todo_file() now print an error if they fail to write to the todo file. - A lot of changes were introduced by the conflict resolution with nd/the-index. Alban Gruin (16): sequencer: changes in parse_insn_buffer() sequencer: make the todo_list structure public sequencer: remove the 'arg' field from todo_item sequencer: refactor transform_todos() to work on a todo_list sequencer: introduce todo_list_write_to_file() sequencer: refactor check_todo_list() to work on a todo_list sequencer: refactor sequencer_add_exec_commands() to work on a todo_list sequencer: refactor rearrange_squash() to work on a todo_list sequencer: make sequencer_make_script() write its script to a strbuf sequencer: change complete_action() to use the refactored functions sequencer: refactor skip_unnecessary_picks() to work on a todo_list rebase-interactive: use todo_list_write_to_file() in edit_todo_list() rebase-interactive: append_todo_help() changes rebase-interactive: rewrite edit_todo_list() to handle the initial edit sequencer: use edit_todo_list() in complete_action() rebase--interactive: move transform_todo_file() to rebase--interactive.c builtin/rebase--interactive.c | 90 +++-- rebase-interactive.c | 143 +-- rebase-interactive.h | 8 +- sequencer.c | 677 +- sequencer.h | 82 +++- 5 files changed, 525 insertions(+), 475 deletions(-) -- 2.20.1
[PATCH v4 06/16] sequencer: refactor check_todo_list() to work on a todo_list
This refactors check_todo_list() to work on a todo_list to avoid redundant reads and writes to the disk. The function is renamed todo_list_check(). The parsing of the two todo lists is left to the caller. As rebase -p still need to check the todo list from the disk, a new function is introduced, check_todo_list_from_file(). It reads the file from the disk, parses it, pass the todo_list to todo_list_check(), and writes it back to the disk. As get_missing_commit_check_level() and the enum missing_commit_check_level are no longer needed inside of sequencer.c, they are moved to rebase-interactive.c, and made static again. Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 2 +- rebase-interactive.c | 91 - rebase-interactive.h | 1 + sequencer.c | 121 +++--- sequencer.h | 9 +-- 5 files changed, 116 insertions(+), 108 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 0898eb4c59..df19ccaeb9 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -256,7 +256,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = transform_todo_file(the_repository, flags); break; case CHECK_TODO_LIST: - ret = check_todo_list(the_repository); + ret = check_todo_list_from_file(the_repository); break; case REARRANGE_SQUASH: ret = rearrange_squash(the_repository); diff --git a/rebase-interactive.c b/rebase-interactive.c index 842fa07e7e..6157247e1f 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -1,8 +1,32 @@ #include "cache.h" #include "commit.h" -#include "rebase-interactive.h" #include "sequencer.h" +#include "rebase-interactive.h" #include "strbuf.h" +#include "commit-slab.h" +#include "config.h" + +enum missing_commit_check_level { + MISSING_COMMIT_CHECK_IGNORE = 0, + MISSING_COMMIT_CHECK_WARN, + MISSING_COMMIT_CHECK_ERROR +}; + +static enum missing_commit_check_level get_missing_commit_check_level(void) +{ + const char *value; + + if (git_config_get_value("rebase.missingcommitscheck", &value) || + !strcasecmp("ignore", value)) + return MISSING_COMMIT_CHECK_IGNORE; + if (!strcasecmp("warn", value)) + return MISSING_COMMIT_CHECK_WARN; + if (!strcasecmp("error", value)) + return MISSING_COMMIT_CHECK_ERROR; + warning(_("unrecognized setting %s for option " + "rebase.missingCommitsCheck. Ignoring."), value); + return MISSING_COMMIT_CHECK_IGNORE; +} void append_todo_help(unsigned edit_todo, unsigned keep_empty, struct strbuf *buf) @@ -89,3 +113,68 @@ int edit_todo_list(struct repository *r, unsigned flags) return 0; } + +define_commit_slab(commit_seen, unsigned char); +/* + * Check if the user dropped some commits by mistake + * Behaviour determined by rebase.missingCommitsCheck. + * Check if there is an unrecognized command or a + * bad SHA-1 in a command. + */ +int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo) +{ + enum missing_commit_check_level check_level = get_missing_commit_check_level(); + struct strbuf missing = STRBUF_INIT; + int res = 0, i; + struct commit_seen commit_seen; + + init_commit_seen(&commit_seen); + + if (check_level == MISSING_COMMIT_CHECK_IGNORE) + goto leave_check; + + /* Mark the commits in git-rebase-todo as seen */ + for (i = 0; i < new_todo->nr; i++) { + struct commit *commit = new_todo->items[i].commit; + if (commit) + *commit_seen_at(&commit_seen, commit) = 1; + } + + /* Find commits in git-rebase-todo.backup yet unseen */ + for (i = old_todo->nr - 1; i >= 0; i--) { + struct todo_item *item = old_todo->items + i; + struct commit *commit = item->commit; + if (commit && !*commit_seen_at(&commit_seen, commit)) { + strbuf_addf(&missing, " - %s %.*s\n", + find_unique_abbrev(&commit->object.oid, DEFAULT_ABBREV), + item->arg_len, + old_todo->buf.buf + item->arg_offset); + *commit_seen_at(&commit_seen, commit) = 1; + } + } + + /* Warn about missing commits */ + if (!missing.len) + goto leave_check; + + if (check_level == MISSING_COMMIT_CHECK_ERROR) + res =
[PATCH v4 02/16] sequencer: make the todo_list structure public
This makes the structures todo_list and todo_item, and the functions todo_list_release() and parse_insn_buffer(), accessible outside of sequencer.c. Signed-off-by: Alban Gruin --- Some changes were introduced because of the conflict resolution with nd/the-index. They mostly consist of adding a parameter ("r") when calling todo_list_parse_insn_buffer. sequencer.c | 69 ++--- sequencer.h | 50 ++ 2 files changed, 62 insertions(+), 57 deletions(-) diff --git a/sequencer.c b/sequencer.c index a7afaf6882..5b84a20532 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1460,32 +1460,6 @@ static int allow_empty(struct repository *r, return 1; } -/* - * Note that ordering matters in this enum. Not only must it match the mapping - * below, it is also divided into several sections that matter. When adding - * new commands, make sure you add it in the right section. - */ -enum todo_command { - /* commands that handle commits */ - TODO_PICK = 0, - TODO_REVERT, - TODO_EDIT, - TODO_REWORD, - TODO_FIXUP, - TODO_SQUASH, - /* commands that do something else than handling a single commit */ - TODO_EXEC, - TODO_BREAK, - TODO_LABEL, - TODO_RESET, - TODO_MERGE, - /* commands that do nothing but are counted for reporting progress */ - TODO_NOOP, - TODO_DROP, - /* comments (not counted for reporting progress) */ - TODO_COMMENT -}; - static struct { char c; const char *str; @@ -1962,26 +1936,7 @@ enum todo_item_flags { TODO_EDIT_MERGE_MSG = 1 }; -struct todo_item { - enum todo_command command; - struct commit *commit; - unsigned int flags; - const char *arg; - int arg_len; - size_t offset_in_buf; -}; - -struct todo_list { - struct strbuf buf; - struct todo_item *items; - int nr, alloc, current; - int done_nr, total_nr; - struct stat_data stat; -}; - -#define TODO_LIST_INIT { STRBUF_INIT } - -static void todo_list_release(struct todo_list *todo_list) +void todo_list_release(struct todo_list *todo_list) { strbuf_release(&todo_list->buf); FREE_AND_NULL(todo_list->items); @@ -2084,8 +2039,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, return !item->commit; } -static int parse_insn_buffer(struct repository *r, char *buf, -struct todo_list *todo_list) +int todo_list_parse_insn_buffer(struct repository *r, char *buf, + struct todo_list *todo_list) { struct todo_item *item; char *p = buf, *next_p; @@ -2184,7 +2139,7 @@ static int read_populate_todo(struct repository *r, return error(_("could not stat '%s'"), todo_file); fill_stat_data(&todo_list->stat, &st); - res = parse_insn_buffer(r, todo_list->buf.buf, todo_list); + res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list); if (res) { if (is_rebase_i(opts)) return error(_("please fix this using " @@ -2215,7 +2170,7 @@ static int read_populate_todo(struct repository *r, FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w"); if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 && - !parse_insn_buffer(r, done.buf.buf, &done)) + !todo_list_parse_insn_buffer(r, done.buf.buf, &done)) todo_list->done_nr = count_commands(&done); else todo_list->done_nr = 0; @@ -4501,7 +4456,7 @@ int sequencer_add_exec_commands(struct repository *r, if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) return error(_("could not read '%s'."), todo_file); - if (parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { + if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { todo_list_release(&todo_list); return error(_("unusable todo list: '%s'"), todo_file); } @@ -4557,7 +4512,7 @@ int transform_todos(struct repository *r, unsigned flags) if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) return error(_("could not read '%s'."), todo_file); - if (parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { + if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { todo_list_release(&todo_list); return error(_("unusable todo list: '%s'"), todo_file); } @@ -4643,7 +4598,7 @@ int check_todo_list(struct repository *r) goto leave_ch
[PATCH v4 07/16] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
This refactors sequencer_add_exec_commands() to work on a todo_list to avoid redundant reads and writes to the disk. Instead of inserting the `exec' commands between the other commands and re-parsing the buffer at the end, they are appended to the buffer once, and a new list of items is created. Items from the old list are copied across and new `exec' items are appended when necessary. This eliminates the need to reparse the buffer, but this also means we have to use todo_list_write_to_disk() to write the file. todo_list_add_exec_commands() and sequencer_add_exec_commands() are modified to take a string list instead of a string -- one item for each command. This makes it easier to insert a new command to the todo list for each command to execute. sequencer_add_exec_commands() still reads the todo list from the disk, as it is needed by rebase -p. complete_action() still uses sequencer_add_exec_commands() for now. This will be changed in a future commit. Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 15 +++-- sequencer.c | 110 +- sequencer.h | 5 +- 3 files changed, 82 insertions(+), 48 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index df19ccaeb9..53056ee713 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -65,7 +65,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, const char *onto, const char *onto_name, const char *squash_onto, const char *head_name, const char *restrict_revision, char *raw_strategies, -const char *cmd, unsigned autosquash) +struct string_list *commands, unsigned autosquash) { int ret; const char *head_hash = NULL; @@ -116,7 +116,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, discard_cache(); ret = complete_action(the_repository, opts, flags, shortrevisions, onto_name, onto, - head_hash, cmd, autosquash); + head_hash, commands, autosquash); } free(revisions); @@ -139,6 +139,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) const char *onto = NULL, *onto_name = NULL, *restrict_revision = NULL, *squash_onto = NULL, *upstream = NULL, *head_name = NULL, *switch_to = NULL, *cmd = NULL; + struct string_list commands = STRING_LIST_INIT_DUP; char *raw_strategies = NULL; enum { NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH, @@ -221,6 +222,12 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) warning(_("--[no-]rebase-cousins has no effect without " "--rebase-merges")); + if (cmd && *cmd) { + string_list_split(&commands, cmd, '\n', -1); + if (strlen(commands.items[commands.nr - 1].string) == 0) + --commands.nr; + } + switch (command) { case NONE: if (!onto && !upstream) @@ -228,7 +235,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = do_interactive_rebase(&opts, flags, switch_to, upstream, onto, onto_name, squash_onto, head_name, restrict_revision, - raw_strategies, cmd, autosquash); + raw_strategies, &commands, autosquash); break; case SKIP: { struct string_list merge_rr = STRING_LIST_INIT_DUP; @@ -262,7 +269,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = rearrange_squash(the_repository); break; case ADD_EXEC: - ret = sequencer_add_exec_commands(the_repository, cmd); + ret = sequencer_add_exec_commands(the_repository, &commands); break; default: BUG("invalid command '%d'", command); diff --git a/sequencer.c b/sequencer.c index 347a1a701f..2df64b3677 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4440,25 +4440,27 @@ int sequencer_make_script(struct repository *r, FILE *out, return 0; } -/* - * Add commands after pick and (series of) squash/fixup commands - * in the todo list. - */ -int sequencer_add_exec_commands(struct repository *r, - const char *commands) +static void todo_list_add_exec_commands(struct todo_list *todo_list, +
[PATCH v4 15/16] sequencer: use edit_todo_list() in complete_action()
This changes complete_action() to use edit_todo_list(), now that it can handle the initial edit of the todo list. Signed-off-by: Alban Gruin --- Changes due to conflicts with nd/the-index. sequencer.c | 21 +++-- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/sequencer.c b/sequencer.c index 0500a32e80..127bb0b68e 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4703,6 +4703,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla struct todo_list new_todo = TODO_LIST_INIT; struct strbuf *buf = &todo_list->buf; struct object_id oid; + int res; get_oid(onto, &oid); shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV); @@ -4727,24 +4728,16 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla return error(_("nothing to do")); } - if (todo_list_write_to_file(r, todo_list, todo_file, - shortrevisions, shortonto, -1, - flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP)) - return error_errno(_("could not write '%s'"), todo_file); - - if (copy_file(rebase_path_todo_backup(), todo_file, 0666)) - return error(_("could not copy '%s' to '%s'."), todo_file, -rebase_path_todo_backup()); - - if (launch_sequence_editor(todo_file, &new_todo.buf, NULL)) { + res = edit_todo_list(r, todo_list, &new_todo, shortrevisions, +shortonto, flags); + if (res == -1) + return -1; + else if (res == -2) { apply_autostash(opts); sequencer_remove_state(opts); return -1; - } - - strbuf_stripspace(&new_todo.buf, 1); - if (new_todo.buf.len == 0) { + } else if (res == -3) { apply_autostash(opts); sequencer_remove_state(opts); todo_list_release(&new_todo); -- 2.20.1
[PATCH v4 03/16] sequencer: remove the 'arg' field from todo_item
The 'arg' field of todo_item used to store the address of the first byte of the parameter of a command in a todo list. It was associated with the length of the parameter (the 'arg_len' field). This replaces the 'arg' field by 'arg_offset'. This new field does not store the address of the parameter, but the position of the first character of the parameter in the buffer. This will prevent todo_list_add_exec_commands() from having to do awful pointer arithmetics when growing the todo list buffer. Signed-off-by: Alban Gruin --- This is a new commit. sequencer.c | 61 - sequencer.h | 4 ++-- 2 files changed, 34 insertions(+), 31 deletions(-) diff --git a/sequencer.c b/sequencer.c index 5b84a20532..61be04ccc5 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1950,7 +1950,7 @@ static struct todo_item *append_new_todo(struct todo_list *todo_list) } static int parse_insn_line(struct repository *r, struct todo_item *item, - const char *bol, char *eol) + const char *buf, const char *bol, char *eol) { struct object_id commit_oid; char *end_of_object_name; @@ -1964,7 +1964,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, if (bol == eol || *bol == '\r' || *bol == comment_line_char) { item->command = TODO_COMMENT; item->commit = NULL; - item->arg = bol; + item->arg_offset = bol - buf; item->arg_len = eol - bol; return 0; } @@ -1991,7 +1991,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, return error(_("%s does not accept arguments: '%s'"), command_to_string(item->command), bol); item->commit = NULL; - item->arg = bol; + item->arg_offset = bol - buf; item->arg_len = eol - bol; return 0; } @@ -2003,7 +2003,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, if (item->command == TODO_EXEC || item->command == TODO_LABEL || item->command == TODO_RESET) { item->commit = NULL; - item->arg = bol; + item->arg_offset = bol - buf; item->arg_len = (int)(eol - bol); return 0; } @@ -2017,7 +2017,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, } else { item->flags |= TODO_EDIT_MERGE_MSG; item->commit = NULL; - item->arg = bol; + item->arg_offset = bol - buf; item->arg_len = (int)(eol - bol); return 0; } @@ -2029,8 +2029,9 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, status = get_oid(bol, &commit_oid); *end_of_object_name = saved; - item->arg = end_of_object_name + strspn(end_of_object_name, " \t"); - item->arg_len = (int)(eol - item->arg); + bol = end_of_object_name + strspn(end_of_object_name, " \t"); + item->arg_offset = bol - buf; + item->arg_len = (int)(eol - bol); if (status < 0) return -1; @@ -2058,11 +2059,11 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf, item = append_new_todo(todo_list); item->offset_in_buf = p - todo_list->buf.buf; - if (parse_insn_line(r, item, p, eol)) { + if (parse_insn_line(r, item, buf, p, eol)) { res = error(_("invalid line %d: %.*s"), i, (int)(eol - p), p); item->command = TODO_COMMENT + 1; - item->arg = p; + item->arg_offset = p - buf; item->arg_len = (int)(eol - p); item->commit = NULL; } @@ -2402,7 +2403,7 @@ static int walk_revs_populate_todo(struct todo_list *todo_list, item->command = command; item->commit = commit; - item->arg = NULL; + item->arg_offset = 0; item->arg_len = 0; item->offset_in_buf = todo_list->buf.len; subject_len = find_commit_subject(commit_buffer, &subject); @@ -3438,6 +3439,8 @@ static int pick_commits(struct repository *r, while (todo_list->current < todo_list->nr) { struct todo_item *item = todo_list->items + todo_list->current; + c
[PATCH v4 04/16] sequencer: refactor transform_todos() to work on a todo_list
This refactors transform_todos() to work on a todo_list. The function is renamed todo_list_transform(). As rebase -p still need to check the todo list from the disk, a new function is introduced, transform_todo_file(). It is still used by complete_action() and edit_todo_list() for now, but they will be replaced in a future commit. todo_list_transform() is not a static function, because it will be used by edit_todo_list() from rebase-interactive.c in a future commit. Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 2 +- rebase-interactive.c | 4 +-- sequencer.c | 54 +++ sequencer.h | 4 ++- 4 files changed, 42 insertions(+), 22 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index dd2a55ab1d..0898eb4c59 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -253,7 +253,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) } case SHORTEN_OIDS: case EXPAND_OIDS: - ret = transform_todos(the_repository, flags); + ret = transform_todo_file(the_repository, flags); break; case CHECK_TODO_LIST: ret = check_todo_list(the_repository); diff --git a/rebase-interactive.c b/rebase-interactive.c index 68aff1dac2..842fa07e7e 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -69,7 +69,7 @@ int edit_todo_list(struct repository *r, unsigned flags) strbuf_release(&buf); - transform_todos(r, flags | TODO_LIST_SHORTEN_IDS); + transform_todo_file(r, flags | TODO_LIST_SHORTEN_IDS); if (strbuf_read_file(&buf, todo_file, 0) < 0) return error_errno(_("could not read '%s'."), todo_file); @@ -85,7 +85,7 @@ int edit_todo_list(struct repository *r, unsigned flags) if (launch_sequence_editor(todo_file, NULL, NULL)) return -1; - transform_todos(r, flags & ~(TODO_LIST_SHORTEN_IDS)); + transform_todo_file(r, flags & ~(TODO_LIST_SHORTEN_IDS)); return 0; } diff --git a/sequencer.c b/sequencer.c index 61be04ccc5..6b4eb7a9ba 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4501,27 +4501,18 @@ int sequencer_add_exec_commands(struct repository *r, return i; } -int transform_todos(struct repository *r, unsigned flags) +void todo_list_transform(struct repository *r, struct todo_list *todo_list, +unsigned flags) { - const char *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; struct strbuf buf = STRBUF_INIT; struct todo_item *item; int i; - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) - return error(_("could not read '%s'."), todo_file); - - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { - todo_list_release(&todo_list); - return error(_("unusable todo list: '%s'"), todo_file); - } - - for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) { + for (item = todo_list->items, i = 0; i < todo_list->nr; i++, item++) { /* if the item is not a command write it and continue */ if (item->command >= TODO_COMMENT) { strbuf_addf(&buf, "%.*s\n", item->arg_len, - todo_list.buf.buf + item->arg_offset); + todo_list->buf.buf + item->arg_offset); continue; } @@ -4552,12 +4543,39 @@ int transform_todos(struct repository *r, unsigned flags) strbuf_addch(&buf, '\n'); else strbuf_addf(&buf, " %.*s\n", item->arg_len, - todo_list.buf.buf + item->arg_offset); + todo_list->buf.buf + item->arg_offset); } - i = write_message(buf.buf, buf.len, todo_file, 0); + strbuf_reset(&todo_list->buf); + strbuf_add(&todo_list->buf, buf.buf, buf.len); + strbuf_release(&buf); + + if (todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list)) + BUG("unusable todo list"); +} + +int transform_todo_file(struct repository *r, unsigned flags) +{ + const char *todo_file = rebase_path_todo(); + struct todo_list todo_list = TODO_LIST_INIT; + int res; + + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) + return error_errno(_("could not read '%s'."), todo_file); + + if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { + tod
[PATCH v4 12/16] rebase-interactive: use todo_list_write_to_file() in edit_todo_list()
Just like complete_action(), edit_todo_list() used a function (transform_todo_file()) that read the todo list from the disk and wrote it back, resulting in useless disk accesses. This changes edit_todo_list() to call directly todo_list_write_to_file() instead. Signed-off-by: Alban Gruin --- Changes due to conflicts with nd/the-index. rebase-interactive.c | 38 -- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/rebase-interactive.c b/rebase-interactive.c index 6157247e1f..994f0f9753 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -79,39 +79,33 @@ void append_todo_help(unsigned edit_todo, unsigned keep_empty, int edit_todo_list(struct repository *r, unsigned flags) { - struct strbuf buf = STRBUF_INIT; const char *todo_file = rebase_path_todo(); + struct todo_list todo_list = TODO_LIST_INIT; + int res = 0; - if (strbuf_read_file(&buf, todo_file, 0) < 0) + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) return error_errno(_("could not read '%s'."), todo_file); - strbuf_stripspace(&buf, 1); - if (write_message(buf.buf, buf.len, todo_file, 0)) { - strbuf_release(&buf); + strbuf_stripspace(&todo_list.buf, 1); + todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list); + if (todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, -1, + flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP)) { + todo_list_release(&todo_list); return -1; } - strbuf_release(&buf); - - transform_todo_file(r, flags | TODO_LIST_SHORTEN_IDS); - - if (strbuf_read_file(&buf, todo_file, 0) < 0) - return error_errno(_("could not read '%s'."), todo_file); - - append_todo_help(1, 0, &buf); - if (write_message(buf.buf, buf.len, todo_file, 0)) { - strbuf_release(&buf); + strbuf_reset(&todo_list.buf); + if (launch_sequence_editor(todo_file, &todo_list.buf, NULL)) { + todo_list_release(&todo_list); return -1; } - strbuf_release(&buf); + if (!todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) + res = todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, -1, + flags & ~(TODO_LIST_SHORTEN_IDS)); - if (launch_sequence_editor(todo_file, NULL, NULL)) - return -1; - - transform_todo_file(r, flags & ~(TODO_LIST_SHORTEN_IDS)); - - return 0; + todo_list_release(&todo_list); + return res; } define_commit_slab(commit_seen, unsigned char); -- 2.20.1
[PATCH v4 05/16] sequencer: introduce todo_list_write_to_file()
This introduces a new function to recreate the text of a todo list from its commands and write it to a file. This will be useful as the next few commits will change the use of the buffer in struct todo_list so it will no longer be a mirror of the file on disk. This functionality already exists in todo_list_transform(), but this function was made to replace the buffer of a todo list, which is not what we want here. Thus, the part of todo_list_transform() that replaces the buffer is dropped, and the function is renamed todo_list_to_strbuf(). It is called by todo_list_write_to_file() to fill the buffer to write to the disk. todo_list_write_to_file() can also take care of appending the help text to the buffer before writing it to the disk, or to write only the first n items of the list. This feature will be used by skip_unnecessary_picks(), which has to write done commands in a file. Signed-off-by: Alban Gruin --- sequencer.c | 61 +++-- sequencer.h | 11 ++ 2 files changed, 48 insertions(+), 24 deletions(-) diff --git a/sequencer.c b/sequencer.c index 6b4eb7a9ba..b932ca34f2 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4501,26 +4501,28 @@ int sequencer_add_exec_commands(struct repository *r, return i; } -void todo_list_transform(struct repository *r, struct todo_list *todo_list, -unsigned flags) +static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_list, + struct strbuf *buf, int num, unsigned flags) { - struct strbuf buf = STRBUF_INIT; struct todo_item *item; - int i; + int i, max = todo_list->nr; - for (item = todo_list->items, i = 0; i < todo_list->nr; i++, item++) { + if (num > 0 && num < max) + max = num; + + for (item = todo_list->items, i = 0; i < max; i++, item++) { /* if the item is not a command write it and continue */ if (item->command >= TODO_COMMENT) { - strbuf_addf(&buf, "%.*s\n", item->arg_len, + strbuf_addf(buf, "%.*s\n", item->arg_len, todo_list->buf.buf + item->arg_offset); continue; } /* add command to the buffer */ if (flags & TODO_LIST_ABBREVIATE_CMDS) - strbuf_addch(&buf, command_to_char(item->command)); + strbuf_addch(buf, command_to_char(item->command)); else - strbuf_addstr(&buf, command_to_string(item->command)); + strbuf_addstr(buf, command_to_string(item->command)); /* add commit id */ if (item->commit) { @@ -4530,28 +4532,48 @@ void todo_list_transform(struct repository *r, struct todo_list *todo_list, if (item->command == TODO_MERGE) { if (item->flags & TODO_EDIT_MERGE_MSG) - strbuf_addstr(&buf, " -c"); + strbuf_addstr(buf, " -c"); else - strbuf_addstr(&buf, " -C"); + strbuf_addstr(buf, " -C"); } - strbuf_addf(&buf, " %s", oid); + strbuf_addf(buf, " %s", oid); } /* add all the rest */ if (!item->arg_len) - strbuf_addch(&buf, '\n'); + strbuf_addch(buf, '\n'); else - strbuf_addf(&buf, " %.*s\n", item->arg_len, + strbuf_addf(buf, " %.*s\n", item->arg_len, todo_list->buf.buf + item->arg_offset); } +} - strbuf_reset(&todo_list->buf); - strbuf_add(&todo_list->buf, buf.buf, buf.len); +int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list, + const char *file, const char *shortrevisions, + const char *shortonto, int num, unsigned flags) +{ + int edit_todo = !(shortrevisions && shortonto), res; + struct strbuf buf = STRBUF_INIT; + + todo_list_to_strbuf(r, todo_list, &buf, num, flags); + + if (flags & TODO_LIST_APPEND_TODO_HELP) { + int command_count = count_commands(todo_list); + if (!edit_todo) { + strbuf_addch(&buf, '\n'); + strbuf_commented_addf(&am
[PATCH v4 10/16] sequencer: change complete_action() to use the refactored functions
complete_action() used functions that read the todo-list file, made some changes to it, and wrote it back to the disk. The previous commits were dedicated to separate the part that deals with the file from the actual logic of these functions. Now that this is done, we can call directly the "logic" functions to avoid useless file access. The parsing of the list has to be done by the caller. If the buffer of the todo list provided by the caller is empty, a `noop' command is directly added to the todo list, without touching the buffer. Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 20 +++-- sequencer.c | 81 +++ sequencer.h | 2 +- 3 files changed, 42 insertions(+), 61 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 92026739c9..2dbf8fc08b 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -71,7 +71,6 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, const char *head_hash = NULL; char *revisions = NULL, *shortrevisions = NULL; struct argv_array make_script_args = ARGV_ARRAY_INIT; - FILE *todo_list_file; struct todo_list todo_list = TODO_LIST_INIT; if (prepare_branch_to_be_rebased(opts, switch_to)) @@ -94,14 +93,6 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, if (!upstream && squash_onto) write_file(path_squash_onto(), "%s\n", squash_onto); - todo_list_file = fopen(rebase_path_todo(), "w"); - if (!todo_list_file) { - free(revisions); - free(shortrevisions); - - return error_errno(_("could not open %s"), rebase_path_todo()); - } - argv_array_pushl(&make_script_args, "", revisions, NULL); if (restrict_revision) argv_array_push(&make_script_args, restrict_revision); @@ -109,16 +100,17 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, ret = sequencer_make_script(the_repository, &todo_list.buf, make_script_args.argc, make_script_args.argv, flags); - fputs(todo_list.buf.buf, todo_list_file); - fclose(todo_list_file); if (ret) error(_("could not generate todo list")); else { discard_cache(); - ret = complete_action(the_repository, opts, flags, - shortrevisions, onto_name, onto, - head_hash, commands, autosquash); + if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf, + &todo_list)) + BUG("unusable todo list"); + + ret = complete_action(the_repository, opts, flags, shortrevisions, onto_name, + onto, head_hash, commands, autosquash, &todo_list); } free(revisions); diff --git a/sequencer.c b/sequencer.c index f6527fb418..ea5cea81a8 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4748,100 +4748,89 @@ static int skip_unnecessary_picks(struct repository *r, struct object_id *output return 0; } +static int todo_list_rearrange_squash(struct todo_list *todo_list); + int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags, const char *shortrevisions, const char *onto_name, const char *onto, const char *orig_head, struct string_list *commands, - unsigned autosquash) + unsigned autosquash, struct todo_list *todo_list) { const char *shortonto, *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; - struct strbuf *buf = &(todo_list.buf); + struct todo_list new_todo = TODO_LIST_INIT; + struct strbuf *buf = &todo_list->buf; struct object_id oid; - struct stat st; get_oid(onto, &oid); shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV); - if (!lstat(todo_file, &st) && st.st_size == 0 && - write_message("noop\n", 5, todo_file, 0)) - return -1; + if (buf->len == 0) { + struct todo_item *item = append_new_todo(todo_list); + item->command = TODO_NOOP; + item->commit = NULL; + item->arg_len = item->arg_offset = item->flags = item->offset_in_buf = 0; + } - if (autosquash && rearrange_squash_in_todo_file(r)) + if (autosquash && todo_list_rearrange_squash(todo_list)) return -1; if (co
[PATCH v4 13/16] rebase-interactive: append_todo_help() changes
This moves the writing of the comment "Rebase $shortrevisions onto $shortonto ($command_count commands)" from todo_list_write_to_file() to append_todo_help(). shortrevisions, shortonto, and command_count are passed as parameters to append_todo_help(). During the initial edit of the todo list, shortrevisions and shortonto are not NULL. Therefore, if shortrevisions or shortonto is NULL, then edit_todo would be true, otherwise it would be false. Thus, edit_todo is removed from the parameters of append_todo_help(). Signed-off-by: Alban Gruin --- Slight rewording of the message and changes due to conflicts with nd/the-index. rebase-interactive.c | 12 +++- rebase-interactive.h | 3 ++- sequencer.c | 17 - 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/rebase-interactive.c b/rebase-interactive.c index 994f0f9753..32f95002df 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -28,7 +28,8 @@ static enum missing_commit_check_level get_missing_commit_check_level(void) return MISSING_COMMIT_CHECK_IGNORE; } -void append_todo_help(unsigned edit_todo, unsigned keep_empty, +void append_todo_help(unsigned keep_empty, int command_count, + const char *shortrevisions, const char *shortonto, struct strbuf *buf) { const char *msg = _("\nCommands:\n" @@ -48,6 +49,15 @@ void append_todo_help(unsigned edit_todo, unsigned keep_empty, ". specified). Use -c to reword the commit message.\n" "\n" "These lines can be re-ordered; they are executed from top to bottom.\n"); + unsigned edit_todo = !(shortrevisions && shortonto); + + if (!edit_todo) { + strbuf_addch(buf, '\n'); + strbuf_commented_addf(buf, Q_("Rebase %s onto %s (%d command)", + "Rebase %s onto %s (%d commands)", + command_count), + shortrevisions, shortonto, command_count); + } strbuf_add_commented_lines(buf, msg, strlen(msg)); diff --git a/rebase-interactive.h b/rebase-interactive.h index dd0d717bc1..2f6675eabd 100644 --- a/rebase-interactive.h +++ b/rebase-interactive.h @@ -4,7 +4,8 @@ struct strbuf; struct repository; -void append_todo_help(unsigned edit_todo, unsigned keep_empty, +void append_todo_help(unsigned keep_empty, int command_count, + const char *shortrevisions, const char *shortonto, struct strbuf *buf); int edit_todo_list(struct repository *r, unsigned flags); int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo); diff --git a/sequencer.c b/sequencer.c index dfdba5cec0..dfc705291d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4574,22 +4574,13 @@ int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list, const char *file, const char *shortrevisions, const char *shortonto, int num, unsigned flags) { - int edit_todo = !(shortrevisions && shortonto), res; + int res; struct strbuf buf = STRBUF_INIT; todo_list_to_strbuf(r, todo_list, &buf, num, flags); - - if (flags & TODO_LIST_APPEND_TODO_HELP) { - int command_count = count_commands(todo_list); - if (!edit_todo) { - strbuf_addch(&buf, '\n'); - strbuf_commented_addf(&buf, Q_("Rebase %s onto %s (%d command)", - "Rebase %s onto %s (%d commands)", - command_count), - shortrevisions, shortonto, command_count); - } - append_todo_help(edit_todo, flags & TODO_LIST_KEEP_EMPTY, &buf); - } + if (flags & TODO_LIST_APPEND_TODO_HELP) + append_todo_help(flags & TODO_LIST_KEEP_EMPTY, count_commands(todo_list), +shortrevisions, shortonto, &buf); res = write_message(buf.buf, buf.len, file, 0); strbuf_release(&buf); -- 2.20.1
[PATCH v4 16/16] rebase--interactive: move transform_todo_file() to rebase--interactive.c
As transform_todo_file() is only needed inside of rebase--interactive.c, it is moved there from sequencer.c. Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 26 +- sequencer.c | 23 --- sequencer.h | 1 - 3 files changed, 25 insertions(+), 25 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 645ac587f7..7f1e88a087 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -35,6 +35,30 @@ static int edit_todo_file(unsigned flags) return 0; } +static int transform_todo_file(unsigned flags) +{ + const char *todo_file = rebase_path_todo(); + struct todo_list todo_list = TODO_LIST_INIT; + int res; + + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) + return error_errno(_("could not read '%s'."), todo_file); + + if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf, + &todo_list)) { + todo_list_release(&todo_list); + return error(_("unusable todo list: '%s'"), todo_file); + } + + res = todo_list_write_to_file(the_repository, &todo_list, todo_file, + NULL, NULL, -1, flags); + todo_list_release(&todo_list); + + if (res) + return error_errno(_("could not write '%s'."), todo_file); + return 0; +} + static int get_revision_ranges(const char *upstream, const char *onto, const char **head_hash, char **revisions, char **shortrevisions) @@ -277,7 +301,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) } case SHORTEN_OIDS: case EXPAND_OIDS: - ret = transform_todo_file(the_repository, flags); + ret = transform_todo_file(flags); break; case CHECK_TODO_LIST: ret = check_todo_list_from_file(the_repository); diff --git a/sequencer.c b/sequencer.c index 127bb0b68e..6566247a7c 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4587,29 +4587,6 @@ int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list, return res; } -int transform_todo_file(struct repository *r, unsigned flags) -{ - const char *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; - int res; - - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) - return error_errno(_("could not read '%s'."), todo_file); - - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { - todo_list_release(&todo_list); - return error(_("unusable todo list: '%s'"), todo_file); - } - - res = todo_list_write_to_file(r, &todo_list, todo_file, - NULL, NULL, -1, flags); - todo_list_release(&todo_list); - - if (res) - return error_errno(_("could not write '%s'."), todo_file); - return 0; -} - static const char edit_todo_list_advice[] = N_("You can fix this with 'git rebase --edit-todo' " "and then run 'git rebase --continue'.\n" diff --git a/sequencer.h b/sequencer.h index 7dd85bb399..bb3ca5783b 100644 --- a/sequencer.h +++ b/sequencer.h @@ -146,7 +146,6 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc, int sequencer_add_exec_commands(struct repository *r, struct string_list *commands); -int transform_todo_file(struct repository *r, unsigned flags); int check_todo_list_from_file(struct repository *r); int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags, const char *shortrevisions, const char *onto_name, -- 2.20.1
[PATCH v4 14/16] rebase-interactive: rewrite edit_todo_list() to handle the initial edit
edit_todo_list() is changed to work on a todo_list, and to handle the initial edition of the todo list (ie. making a backup of the todo list). It does not check for dropped commits yet, as todo_list_check() does not take the commits that have already been processed by the rebase (ie. the todo list is edited in the middle of a rebase session). Signed-off-by: Alban Gruin --- Changes due to conflicts with nd/the-index. builtin/rebase--interactive.c | 24 +- rebase-interactive.c | 48 ++- rebase-interactive.h | 4 ++- sequencer.c | 3 +-- sequencer.h | 1 + 5 files changed, 53 insertions(+), 27 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 2dbf8fc08b..645ac587f7 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -13,6 +13,28 @@ static GIT_PATH_FUNC(path_state_dir, "rebase-merge/") static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto") static GIT_PATH_FUNC(path_interactive, "rebase-merge/interactive") +static int edit_todo_file(unsigned flags) +{ + const char *todo_file = rebase_path_todo(); + struct todo_list todo_list = TODO_LIST_INIT, + new_todo = TODO_LIST_INIT; + + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) + return error_errno(_("could not read '%s'."), todo_file); + + strbuf_stripspace(&todo_list.buf, 1); + if (!edit_todo_list(the_repository, &todo_list, + &new_todo, NULL, NULL, flags) && + todo_list_write_to_file(the_repository, &new_todo, todo_file, NULL, NULL, + -1, flags & ~(TODO_LIST_SHORTEN_IDS)) < 0) + return error_errno(_("could not write '%s'"), todo_file); + + todo_list_release(&todo_list); + todo_list_release(&new_todo); + + return 0; +} + static int get_revision_ranges(const char *upstream, const char *onto, const char **head_hash, char **revisions, char **shortrevisions) @@ -242,7 +264,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) break; } case EDIT_TODO: - ret = edit_todo_list(the_repository, flags); + ret = edit_todo_file(flags); break; case SHOW_CURRENT_PATCH: { struct child_process cmd = CHILD_PROCESS_INIT; diff --git a/rebase-interactive.c b/rebase-interactive.c index 32f95002df..7542e70a55 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -87,35 +87,37 @@ void append_todo_help(unsigned keep_empty, int command_count, } } -int edit_todo_list(struct repository *r, unsigned flags) +int edit_todo_list(struct repository *r, struct todo_list *todo_list, + struct todo_list *new_todo, const char *shortrevisions, + const char *shortonto, unsigned flags) { const char *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; - int res = 0; - - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) - return error_errno(_("could not read '%s'."), todo_file); - - strbuf_stripspace(&todo_list.buf, 1); - todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list); - if (todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, -1, - flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP)) { - todo_list_release(&todo_list); - return -1; + unsigned initial = shortrevisions && shortonto; + + if (initial) { + todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto, + -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP); + + if (copy_file(rebase_path_todo_backup(), todo_file, 0666)) + return error(_("could not copy '%s' to '%s'."), todo_file, +rebase_path_todo_backup()); + } else { + todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list); + todo_list_write_to_file(r, todo_list, todo_file, NULL, NULL, -1, + flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP); } - strbuf_reset(&todo_list.buf); - if (launch_sequence_editor(todo_file, &todo_list.buf, NULL)) { - todo_list_release(&todo_list); - return -1; - } + if (launch_sequence_editor(todo_file, &new_todo->buf, NULL)) + return -2;
[PATCH v4 09/16] sequencer: make sequencer_make_script() write its script to a strbuf
This makes sequencer_make_script() write its script to a strbuf (ie. the buffer of a todo_list) instead of a FILE. This reduce the amount of read/write made by rebase interactive. Signed-off-by: Alban Gruin --- Changes due to conflicts with nd/the-index. builtin/rebase--interactive.c | 13 ++- sequencer.c | 41 +++ sequencer.h | 5 ++--- 3 files changed, 28 insertions(+), 31 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 4f0eae9239..92026739c9 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -71,7 +71,8 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, const char *head_hash = NULL; char *revisions = NULL, *shortrevisions = NULL; struct argv_array make_script_args = ARGV_ARRAY_INIT; - FILE *todo_list; + FILE *todo_list_file; + struct todo_list todo_list = TODO_LIST_INIT; if (prepare_branch_to_be_rebased(opts, switch_to)) return -1; @@ -93,8 +94,8 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, if (!upstream && squash_onto) write_file(path_squash_onto(), "%s\n", squash_onto); - todo_list = fopen(rebase_path_todo(), "w"); - if (!todo_list) { + todo_list_file = fopen(rebase_path_todo(), "w"); + if (!todo_list_file) { free(revisions); free(shortrevisions); @@ -105,10 +106,11 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, if (restrict_revision) argv_array_push(&make_script_args, restrict_revision); - ret = sequencer_make_script(the_repository, todo_list, + ret = sequencer_make_script(the_repository, &todo_list.buf, make_script_args.argc, make_script_args.argv, flags); - fclose(todo_list); + fputs(todo_list.buf.buf, todo_list_file); + fclose(todo_list_file); if (ret) error(_("could not generate todo list")); @@ -121,6 +123,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, free(revisions); free(shortrevisions); + todo_list_release(&todo_list); argv_array_clear(&make_script_args); return ret; diff --git a/sequencer.c b/sequencer.c index 931ad9848e..f6527fb418 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4154,7 +4154,7 @@ static const char *label_oid(struct object_id *oid, const char *label, } static int make_script_with_merges(struct pretty_print_context *pp, - struct rev_info *revs, FILE *out, + struct rev_info *revs, struct strbuf *out, unsigned flags) { int keep_empty = flags & TODO_LIST_KEEP_EMPTY; @@ -4299,7 +4299,7 @@ static int make_script_with_merges(struct pretty_print_context *pp, * gathering commits not yet shown, reversing the list on the fly, * then outputting that list (labeling revisions as needed). */ - fprintf(out, "%s onto\n", cmd_label); + strbuf_addf(out, "%s onto\n", cmd_label); for (iter = tips; iter; iter = iter->next) { struct commit_list *list = NULL, *iter2; @@ -4309,9 +4309,9 @@ static int make_script_with_merges(struct pretty_print_context *pp, entry = oidmap_get(&state.commit2label, &commit->object.oid); if (entry) - fprintf(out, "\n%c Branch %s\n", comment_line_char, entry->string); + strbuf_addf(out, "\n%c Branch %s\n", comment_line_char, entry->string); else - fprintf(out, "\n"); + strbuf_addch(out, '\n'); while (oidset_contains(&interesting, &commit->object.oid) && !oidset_contains(&shown, &commit->object.oid)) { @@ -4324,8 +4324,8 @@ static int make_script_with_merges(struct pretty_print_context *pp, } if (!commit) - fprintf(out, "%s %s\n", cmd_reset, - rebase_cousins ? "onto" : "[new root]"); + strbuf_addf(out, "%s %s\n", cmd_reset, + rebase_cousins ? "onto" : "[new root]"); else { const char *to = NULL; @@ -4338,12 +4338,12 @@ static int make_script_with_merges(struct pretty_print_context *pp, &
[PATCH v4 11/16] sequencer: refactor skip_unnecessary_picks() to work on a todo_list
This refactors skip_unnecessary_picks() to work on a todo_list. As this function is only called by complete_action() (and thus is not used by rebase -p), the file-handling logic is completely dropped here. Instead of truncating the todo list’s buffer, the items are moved to the beginning of the list, eliminating the need to reparse the list. This also means its buffer cannot be directly written to the disk. rewrite_file() is then removed, as it is now unused. Signed-off-by: Alban Gruin --- sequencer.c | 78 - 1 file changed, 17 insertions(+), 61 deletions(-) diff --git a/sequencer.c b/sequencer.c index ea5cea81a8..dfdba5cec0 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4655,52 +4655,22 @@ int check_todo_list_from_file(struct repository *r) return res; } -static int rewrite_file(const char *path, const char *buf, size_t len) -{ - int rc = 0; - int fd = open(path, O_WRONLY | O_TRUNC); - if (fd < 0) - return error_errno(_("could not open '%s' for writing"), path); - if (write_in_full(fd, buf, len) < 0) - rc = error_errno(_("could not write to '%s'"), path); - if (close(fd) && !rc) - rc = error_errno(_("could not close '%s'"), path); - return rc; -} - /* skip picking commits whose parents are unchanged */ -static int skip_unnecessary_picks(struct repository *r, struct object_id *output_oid) +static int skip_unnecessary_picks(struct repository *r, + struct todo_list *todo_list, + struct object_id *output_oid) { - const char *todo_file = rebase_path_todo(); - struct strbuf buf = STRBUF_INIT; - struct todo_list todo_list = TODO_LIST_INIT; struct object_id *parent_oid; - int fd, i; - - if (!read_oneliner(&buf, rebase_path_onto(), 0)) - return error(_("could not read 'onto'")); - if (get_oid(buf.buf, output_oid)) { - strbuf_release(&buf); - return error(_("need a HEAD to fixup")); - } - strbuf_release(&buf); - - if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0) - return -1; - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list) < 0) { - todo_list_release(&todo_list); - return -1; - } + int i; - for (i = 0; i < todo_list.nr; i++) { - struct todo_item *item = todo_list.items + i; + for (i = 0; i < todo_list->nr; i++) { + struct todo_item *item = todo_list->items + i; if (item->command >= TODO_NOOP) continue; if (item->command != TODO_PICK) break; if (parse_commit(item->commit)) { - todo_list_release(&todo_list); return error(_("could not parse commit '%s'"), oid_to_hex(&item->commit->object.oid)); } @@ -4714,37 +4684,21 @@ static int skip_unnecessary_picks(struct repository *r, struct object_id *output oidcpy(output_oid, &item->commit->object.oid); } if (i > 0) { - int offset = get_item_line_offset(&todo_list, i); const char *done_path = rebase_path_done(); - fd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666); - if (fd < 0) { - error_errno(_("could not open '%s' for writing"), - done_path); - todo_list_release(&todo_list); - return -1; - } - if (write_in_full(fd, todo_list.buf.buf, offset) < 0) { + if (todo_list_write_to_file(r, todo_list, done_path, NULL, NULL, i, 0)) { error_errno(_("could not write to '%s'"), done_path); - todo_list_release(&todo_list); - close(fd); return -1; } - close(fd); - if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset, -todo_list.buf.len - offset) < 0) { - todo_list_release(&todo_list); - return -1; - } + MOVE_ARRAY(todo_list->items, todo_list->items + i, todo_list->nr - i); + todo_list->nr -= i; + todo_list->current = 0; - todo_list.current = i; - if (is_fixup(peek_command(&todo_list, 0))) - record_in_rew
[PATCH v4 08/16] sequencer: refactor rearrange_squash() to work on a todo_list
This refactors rearrange_squash() to work on a todo_list to avoid redundant reads and writes. The function is renamed todo_list_rearrange_squash(). The old version created a new buffer, which was directly written to the disk. This new version creates a new item list by just copying items from the old item list, without creating a new buffer. This eliminates the need to reparse the todo list, but this also means its buffer cannot be directly written to the disk. As rebase -p still need to check the todo list from the disk, a new function is introduced, rearrange_squash_in_todo_file(). complete_action() still uses rearrange_squash_in_todo_file() for now. This will be changed in a future commit. Signed-off-by: Alban Gruin --- Slight rewording of the commit message and changes due to conflicts with nd/the-index. builtin/rebase--interactive.c | 2 +- sequencer.c | 92 ++- sequencer.h | 2 +- 3 files changed, 49 insertions(+), 47 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 53056ee713..4f0eae9239 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -266,7 +266,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = check_todo_list_from_file(the_repository); break; case REARRANGE_SQUASH: - ret = rearrange_squash(the_repository); + ret = rearrange_squash_in_todo_file(the_repository); break; case ADD_EXEC: ret = sequencer_add_exec_commands(the_repository, &commands); diff --git a/sequencer.c b/sequencer.c index 2df64b3677..931ad9848e 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4771,7 +4771,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla write_message("noop\n", 5, todo_file, 0)) return -1; - if (autosquash && rearrange_squash(r)) + if (autosquash && rearrange_squash_in_todo_file(r)) return -1; if (commands->nr) @@ -4877,21 +4877,13 @@ define_commit_slab(commit_todo_item, struct todo_item *); * message will have to be retrieved from the commit (as the oneline in the * script cannot be trusted) in order to normalize the autosquash arrangement. */ -int rearrange_squash(struct repository *r) +static int todo_list_rearrange_squash(struct todo_list *todo_list) { - const char *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; struct hashmap subject2item; - int res = 0, rearranged = 0, *next, *tail, i; + int rearranged = 0, *next, *tail, i, nr = 0, alloc = 0; char **subjects; struct commit_todo_item commit_todo; - - if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0) - return -1; - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list) < 0) { - todo_list_release(&todo_list); - return -1; - } + struct todo_item *items = NULL; init_commit_todo_item(&commit_todo); /* @@ -4904,13 +4896,13 @@ int rearrange_squash(struct repository *r) * be moved to appear after the i'th. */ hashmap_init(&subject2item, (hashmap_cmp_fn) subject2item_cmp, -NULL, todo_list.nr); - ALLOC_ARRAY(next, todo_list.nr); - ALLOC_ARRAY(tail, todo_list.nr); - ALLOC_ARRAY(subjects, todo_list.nr); - for (i = 0; i < todo_list.nr; i++) { +NULL, todo_list->nr); + ALLOC_ARRAY(next, todo_list->nr); + ALLOC_ARRAY(tail, todo_list->nr); + ALLOC_ARRAY(subjects, todo_list->nr); + for (i = 0; i < todo_list->nr; i++) { struct strbuf buf = STRBUF_INIT; - struct todo_item *item = todo_list.items + i; + struct todo_item *item = todo_list->items + i; const char *commit_buffer, *subject, *p; size_t subject_len; int i2 = -1; @@ -4923,7 +4915,6 @@ int rearrange_squash(struct repository *r) } if (is_fixup(item->command)) { - todo_list_release(&todo_list); clear_commit_todo_item(&commit_todo); return error(_("the script was already rearranged.")); } @@ -4958,7 +4949,7 @@ int rearrange_squash(struct repository *r) *commit_todo_item_at(&commit_todo, commit2)) /* found by commit name */ i2 = *commit_todo_item_at(&commit_todo, commit2) - - todo_list.items; + - todo_list
ag/sequencer-reduce-rewriting-todo Re: What's cooking in git.git (Jan 2019, #01; Mon, 7)
Hi Junio, Le 08/01/2019 à 00:34, Junio C Hamano a écrit : > * ag/sequencer-reduce-rewriting-todo (2018-11-12) 16 commits > . rebase--interactive: move transform_todo_file() to rebase--interactive.c > . sequencer: fix a call to error() in transform_todo_file() > . sequencer: use edit_todo_list() in complete_action() > . rebase-interactive: rewrite edit_todo_list() to handle the initial edit > . rebase-interactive: append_todo_help() changes > . rebase-interactive: use todo_list_write_to_file() in edit_todo_list() > . sequencer: refactor skip_unnecessary_picks() to work on a todo_list > . sequencer: change complete_action() to use the refactored functions > . sequencer: make sequencer_make_script() write its script to a strbuf > . sequencer: refactor rearrange_squash() to work on a todo_list > . sequencer: refactor sequencer_add_exec_commands() to work on a todo_list > . sequencer: refactor check_todo_list() to work on a todo_list > . sequencer: introduce todo_list_write_to_file() > . sequencer: refactor transform_todos() to work on a todo_list > . sequencer: make the todo_list structure public > . sequencer: changes in parse_insn_buffer() > > The scripted version of "git rebase -i" wrote and rewrote the todo > list many times during a single step of its operation, and the > recent C-rewrite made a faithful conversion of the logic to C. The > implementation has been updated to carry necessary information > around in-core to avoid rewriting the same file over and over > unnecessarily. > > With too many topics in-flight that touch sequencer and rebaser, > this need to wait giving precedence to other topics that fix bugs. > > I submitted a new version of this topic a week ago, you may have missed it: <20181229160413.19333-1-alban.gr...@gmail.com>. Cheers, Alban
Re: [PATCH] userdiff: Add a builtin pattern for dts files
Hi Stephen, thank you for your patch. I left a few comments below. Le 11/01/2019 à 22:51, Stephen Boyd a écrit : > The Linux kernel receives many patches to the devicetree files each > release. The hunk header for those patches typically show nothing, > making it difficult to figure out what node is being modified without > applying the patch or opening the file and seeking to the context. Let's > add a builtin 'dts' pattern to git so that users can get better diff > output on dts files when they use the diff=dts driver. > > The regex has been constructed based on the spec at devicetree.org[1] > > [1] https://github.com/devicetree-org/devicetree-specification/releases/latest > > Cc: Rob Herring > Signed-off-by: Stephen Boyd > --- > Documentation/gitattributes.txt | 2 ++ > t/t4018-diff-funcname.sh| 1 + > t/t4018/dts-labels | 8 +++ > t/t4018/dts-node-unitless | 8 +++ > t/t4018/dts-nodes | 8 +++ > t/t4018/dts-reference | 8 +++ > t/t4034-diff-words.sh | 1 + > t/t4034/dts/expect | 37 + > t/t4034/dts/post| 32 > t/t4034/dts/pre | 32 > userdiff.c | 9 > 11 files changed, 146 insertions(+) > create mode 100644 t/t4018/dts-labels > create mode 100644 t/t4018/dts-node-unitless > create mode 100644 t/t4018/dts-nodes > create mode 100644 t/t4018/dts-reference > create mode 100644 t/t4034/dts/expect > create mode 100644 t/t4034/dts/post > create mode 100644 t/t4034/dts/pre > > -%<- > diff --git a/userdiff.c b/userdiff.c > index 97007abe5b16..2bc964e11089 100644 > --- a/userdiff.c > +++ b/userdiff.c > @@ -23,6 +23,15 @@ IPATTERN("ada", >"[a-zA-Z][a-zA-Z0-9_]*" >"|[-+]?[0-9][0-9#_.aAbBcCdDeEfF]*([eE][+-]?[0-9_]+)?" >"|=>|\\.\\.|\\*\\*|:=|/=|>=|<=|<<|>>|<>"), > +PATTERNS("dts", > + /* Node name (with optional label and unit address) */ > + "^[ \t]*((([a-zA-Z_][a-zA-Z0-9_]*: > )?[a-zA-Z][a-zA-Z0-9,._+-]*(@[a-zA-Z0-9,._+-]+)?" >From the spec, label and node names “shall be [between] 1 to 31 characters in length”. It’s not enforced here, and I guess it’s not really git’s job to check for this kind of rule. Others may disagree with me, though. Should labels end with exactly one space after the colon, or can there be more, or none at all? > + /* Reference */ > + "|&[a-zA-Z_][a-zA-Z0-9_]*[ \t]*)[ \t]*\\{)[ \t]*$", It’s not specified in the spec, but these lines must end with a curly brace? What if there is a comment after the curly brace? This pattern does not match the root node, but I guess it’s not important as most of the interesting stuff in a dts is not directly in it. > + /* -- */ > + /* Property names and math operators */ > + "[a-zA-Z0-9,._+?#-]+" > + "|[-+*/%&^|!~]"), There is a `%' operator here and in your tests, but it’s not mentioned in the spec if I’m not mistaken. Does it actually exists? > IPATTERN("fortran", >"!^([C*]|[ \t]*!)\n" >"!^[ \t]*MODULE[ \t]+PROCEDURE[ \t]\n" > Cheers, Alban
Re: Students projects: looking for small and medium project ideas
Hi Matthieu, Le 14/01/2019 à 18:53, Matthieu Moy a écrit : > Hi, > > I haven't been active for a while on this list, but for those who don't > know me, I'm a CS teacher and I'm regularly offering my students to > contribute to open-source projects as part of their school projects. A > few nice features like "git rebase -i --exec" or many of the hints in > "git status" were implemented as part of these projects. > > I'm starting another instance of such project next week. > > Part of the work of students is to choose which feature they want to > work on, but I try to prepare this for them. I'm keeping a list of ideas > here: > > https://git.wiki.kernel.org/index.php/SmallProjectsIdeas > > (At some point, I should probably migrate this to git.github.io, since > the wiki only seems half-alive these days). > > I'm looking for small to medium size projects (typically, a GSoC project > is far too big in comparison, but we may expect more than just > microprojects). > > You may suggest ideas by editting the wiki page, or just by replying to > this email (I'll point my students to the thread). Don't hesitate to > remove entries (or ask me to do so) on the wiki page if you think they > are not relevant anymore. > > Thanks in advance, > When rebase.missingCommitsCheck is enabled, git will warn the user if a commit was dropped with `git rebase -i'. This check only occurs after the initial editing. But the user can edit the todo list with `--edit-todo'. Here, git won’t warn the user if a commit was dropped. The goal is to have `--edit-todo' to warn the user when rebase.missingCommitsCheck is enabled, too. Cheers, Alban
Re: [PATCH] userdiff: Add a builtin pattern for dts files
Hi Junio, Le 14/01/2019 à 19:34, Junio C Hamano a écrit : > Alban Gruin writes: > >> thank you for your patch. I left a few comments below. >> >> Le 11/01/2019 à 22:51, Stephen Boyd a écrit: >>> The Linux kernel receives many patches to the devicetree files each >>> release. The hunk header for those patches typically show nothing, >>> making it difficult to figure out what node is being modified without >>> applying the patch or opening the file and seeking to the context. Let's >>> add a builtin 'dts' pattern to git so that users can get better diff >>> output on dts files when they use the diff=dts driver. > > A sort of meta-question. > > What is missing in the current git that prevents the folks involved > in device-tree project from achieving what this patch tries to > accomplish without having to wait the Git project to act on it? To > put it another way, is it a symptom of a bad design that from time > to time the Git project has to add built-in patterns? > > Ability to ship arbitrary piece of text that you would normally > place in .git/config is not exactly an answer to the above question, > and will not happen as that has grave security implications. > > But perhaps we can start accepting an in-tree config-like file whose > contents are limited to verified-safe settings > (e.g. "diff.*.xfuncname" and nothing else), so that projects can > ship two files in-tree: > > - ".gitattributes" that says "*.dts diff=dts" > > - ".gitpreferences" that says "[diff "dts"] xfuncname=..." to >define the pattern the patch under review adds. > > without waiting for the next release of Git to add one more built-in > pattern? > > Anything that defines executable (e.g. "diff.*.command") should > never be accepted as part of the in-tree config-like file (for two > reasons: security and portability), but there should be some > "obviously safe" subset of config settings that we can allow project > to impose on its users, I hope. > I really don’t know what to think about this. I like your proposal, but it will take some time to write such a feature, while there is a patch almost ready to support the dts syntax. But I guess that if it is merged, it will be nearly-impossible to remove from the source if a feature like you proposed is eventually implemented.
Re: [PATCH] userdiff: Add a builtin pattern for dts files
Hi, sorry for the late answer. Le 14/01/2019 à 19:34, Stephen Boyd a écrit : > Quoting Alban Gruin (2019-01-13 13:26:21) >> Hi Stephen, >> >> thank you for your patch. I left a few comments below. >> >> Le 11/01/2019 à 22:51, Stephen Boyd a écrit : >>> diff --git a/userdiff.c b/userdiff.c >>> index 97007abe5b16..2bc964e11089 100644 >>> --- a/userdiff.c >>> +++ b/userdiff.c >>> @@ -23,6 +23,15 @@ IPATTERN("ada", >>>"[a-zA-Z][a-zA-Z0-9_]*" >>>"|[-+]?[0-9][0-9#_.aAbBcCdDeEfF]*([eE][+-]?[0-9_]+)?" >>>"|=>|\\.\\.|\\*\\*|:=|/=|>=|<=|<<|>>|<>"), >>> +PATTERNS("dts", >>> + /* Node name (with optional label and unit address) */ >>> + "^[ \t]*((([a-zA-Z_][a-zA-Z0-9_]*: >>> )?[a-zA-Z][a-zA-Z0-9,._+-]*(@[a-zA-Z0-9,._+-]+)?" >> >> From the spec, label and node names “shall be [between] 1 to 31 >> characters in length”. It’s not enforced here, and I guess it’s not >> really git’s job to check for this kind of rule. Others may disagree >> with me, though. >> >> Should labels end with exactly one space after the colon, or can there >> be more, or none at all? > > There can be any number of spaces after the colon. I can fix the regex > here to accept any amount of whitespace after the colon. > >> >>> + /* Reference */ >>> + "|&[a-zA-Z_][a-zA-Z0-9_]*[ \t]*)[ \t]*\\{)[ \t]*$", >> >> It’s not specified in the spec, but these lines must end with a curly >> brace? > > That isn't common but it is supported. I can change the regex to look > for a line that ends in '{' or something that isn't ';' with anything > after the node name? > >> What if there is a comment after the curly brace? > > There can be a comment after the curly brace or before the curly brace. > The spec allows C style /* */ type comments, in addition to C++ style // > comments. I've never really seen that happen in practice though so it's > not very common. Grepping the linux sources shows two hits: > > arch/arm/boot/dts/lpc3250-ea3250.dts:&ohci /* &usbd */ { > arch/arm/boot/dts/lpc3250-phy3250.dts:&ohci /* &usbd */ { > I grepped through Linux and uboot’s sources and it seems that “it is not common” is actually “it does not exists in the wild”. Perhaps it’s not worth to support them. >> >>> + /* -- */ >>> + /* Property names and math operators */ >>> + "[a-zA-Z0-9,._+?#-]+" >>> + "|[-+*/%&^|!~]"), >> >> There is a `%' operator here and in your tests, but it’s not mentioned >> in the spec if I’m not mistaken. Does it actually exists? > > The compiler doesn't seem to complain when it's used. I can send a patch > to update the spec for this rather esoteric feature. I can also include > more tests and support for the boolean relational operators which also > seem to be supported but probably never used. > I’d like you to do this, yes. To be fair, I don’t know what to think about this patch after Junio’s message.
[PATCH v5 04/16] sequencer: refactor transform_todos() to work on a todo_list
This refactors transform_todos() to work on a todo_list. The function is renamed todo_list_transform(). As rebase -p still need to check the todo list from the disk, a new function is introduced, transform_todo_file(). It is still used by complete_action() and edit_todo_list() for now, but they will be replaced in a future commit. todo_list_transform() is not a static function, because it will be used by edit_todo_list() from rebase-interactive.c in a future commit. Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 2 +- rebase-interactive.c | 4 +-- sequencer.c | 54 +++ sequencer.h | 4 ++- 4 files changed, 42 insertions(+), 22 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index dd2a55ab1d..0898eb4c59 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -253,7 +253,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) } case SHORTEN_OIDS: case EXPAND_OIDS: - ret = transform_todos(the_repository, flags); + ret = transform_todo_file(the_repository, flags); break; case CHECK_TODO_LIST: ret = check_todo_list(the_repository); diff --git a/rebase-interactive.c b/rebase-interactive.c index 68aff1dac2..842fa07e7e 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -69,7 +69,7 @@ int edit_todo_list(struct repository *r, unsigned flags) strbuf_release(&buf); - transform_todos(r, flags | TODO_LIST_SHORTEN_IDS); + transform_todo_file(r, flags | TODO_LIST_SHORTEN_IDS); if (strbuf_read_file(&buf, todo_file, 0) < 0) return error_errno(_("could not read '%s'."), todo_file); @@ -85,7 +85,7 @@ int edit_todo_list(struct repository *r, unsigned flags) if (launch_sequence_editor(todo_file, NULL, NULL)) return -1; - transform_todos(r, flags & ~(TODO_LIST_SHORTEN_IDS)); + transform_todo_file(r, flags & ~(TODO_LIST_SHORTEN_IDS)); return 0; } diff --git a/sequencer.c b/sequencer.c index 0e7ab16a05..7a295cbd3f 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4507,27 +4507,18 @@ int sequencer_add_exec_commands(struct repository *r, return i; } -int transform_todos(struct repository *r, unsigned flags) +void todo_list_transform(struct repository *r, struct todo_list *todo_list, +unsigned flags) { - const char *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; struct strbuf buf = STRBUF_INIT; struct todo_item *item; int i; - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) - return error(_("could not read '%s'."), todo_file); - - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { - todo_list_release(&todo_list); - return error(_("unusable todo list: '%s'"), todo_file); - } - - for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) { + for (item = todo_list->items, i = 0; i < todo_list->nr; i++, item++) { /* if the item is not a command write it and continue */ if (item->command >= TODO_COMMENT) { strbuf_addf(&buf, "%.*s\n", item->arg_len, - todo_item_get_arg(&todo_list, item)); + todo_item_get_arg(todo_list, item)); continue; } @@ -4558,12 +4549,39 @@ int transform_todos(struct repository *r, unsigned flags) strbuf_addch(&buf, '\n'); else strbuf_addf(&buf, " %.*s\n", item->arg_len, - todo_item_get_arg(&todo_list, item)); + todo_item_get_arg(todo_list, item)); } - i = write_message(buf.buf, buf.len, todo_file, 0); + strbuf_reset(&todo_list->buf); + strbuf_add(&todo_list->buf, buf.buf, buf.len); + strbuf_release(&buf); + + if (todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list)) + BUG("unusable todo list"); +} + +int transform_todo_file(struct repository *r, unsigned flags) +{ + const char *todo_file = rebase_path_todo(); + struct todo_list todo_list = TODO_LIST_INIT; + int res; + + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) + return error_errno(_("could not read '%s'."), todo_file); + + if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { + todo_list_releas
[PATCH v5 05/16] sequencer: introduce todo_list_write_to_file()
This introduces a new function to recreate the text of a todo list from its commands and write it to a file. This will be useful as the next few commits will change the use of the buffer in struct todo_list so it will no longer be a mirror of the file on disk. This functionality already exists in todo_list_transform(), but this function was made to replace the buffer of a todo list, which is not what we want here. Thus, the part of todo_list_transform() that replaces the buffer is dropped, and the function is renamed todo_list_to_strbuf(). It is called by todo_list_write_to_file() to fill the buffer to write to the disk. todo_list_write_to_file() can also take care of appending the help text to the buffer before writing it to the disk, or to write only the first n items of the list. This feature will be used by skip_unnecessary_picks(), which has to write done commands in a file. Signed-off-by: Alban Gruin --- sequencer.c | 61 +++-- sequencer.h | 11 ++ 2 files changed, 48 insertions(+), 24 deletions(-) diff --git a/sequencer.c b/sequencer.c index 7a295cbd3f..87b43994ff 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4507,26 +4507,28 @@ int sequencer_add_exec_commands(struct repository *r, return i; } -void todo_list_transform(struct repository *r, struct todo_list *todo_list, -unsigned flags) +static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_list, + struct strbuf *buf, int num, unsigned flags) { - struct strbuf buf = STRBUF_INIT; struct todo_item *item; - int i; + int i, max = todo_list->nr; - for (item = todo_list->items, i = 0; i < todo_list->nr; i++, item++) { + if (num > 0 && num < max) + max = num; + + for (item = todo_list->items, i = 0; i < max; i++, item++) { /* if the item is not a command write it and continue */ if (item->command >= TODO_COMMENT) { - strbuf_addf(&buf, "%.*s\n", item->arg_len, + strbuf_addf(buf, "%.*s\n", item->arg_len, todo_item_get_arg(todo_list, item)); continue; } /* add command to the buffer */ if (flags & TODO_LIST_ABBREVIATE_CMDS) - strbuf_addch(&buf, command_to_char(item->command)); + strbuf_addch(buf, command_to_char(item->command)); else - strbuf_addstr(&buf, command_to_string(item->command)); + strbuf_addstr(buf, command_to_string(item->command)); /* add commit id */ if (item->commit) { @@ -4536,28 +4538,48 @@ void todo_list_transform(struct repository *r, struct todo_list *todo_list, if (item->command == TODO_MERGE) { if (item->flags & TODO_EDIT_MERGE_MSG) - strbuf_addstr(&buf, " -c"); + strbuf_addstr(buf, " -c"); else - strbuf_addstr(&buf, " -C"); + strbuf_addstr(buf, " -C"); } - strbuf_addf(&buf, " %s", oid); + strbuf_addf(buf, " %s", oid); } /* add all the rest */ if (!item->arg_len) - strbuf_addch(&buf, '\n'); + strbuf_addch(buf, '\n'); else - strbuf_addf(&buf, " %.*s\n", item->arg_len, + strbuf_addf(buf, " %.*s\n", item->arg_len, todo_item_get_arg(todo_list, item)); } +} - strbuf_reset(&todo_list->buf); - strbuf_add(&todo_list->buf, buf.buf, buf.len); +int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list, + const char *file, const char *shortrevisions, + const char *shortonto, int num, unsigned flags) +{ + int edit_todo = !(shortrevisions && shortonto), res; + struct strbuf buf = STRBUF_INIT; + + todo_list_to_strbuf(r, todo_list, &buf, num, flags); + + if (flags & TODO_LIST_APPEND_TODO_HELP) { + int command_count = count_commands(todo_list); + if (!edit_todo) { + strbuf_addch(&buf, '\n'); + strbuf_commented_addf(&buf, Q_(&q
[PATCH v5 07/16] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
This refactors sequencer_add_exec_commands() to work on a todo_list to avoid redundant reads and writes to the disk. Instead of inserting the `exec' commands between the other commands and re-parsing the buffer at the end, they are appended to the buffer once, and a new list of items is created. Items from the old list are copied across and new `exec' items are appended when necessary. This eliminates the need to reparse the buffer, but this also means we have to use todo_list_write_to_disk() to write the file. todo_list_add_exec_commands() and sequencer_add_exec_commands() are modified to take a string list instead of a string -- one item for each command. This makes it easier to insert a new command to the todo list for each command to execute. sequencer_add_exec_commands() still reads the todo list from the disk, as it is needed by rebase -p. complete_action() still uses sequencer_add_exec_commands() for now. This will be changed in a future commit. Signed-off-by: Alban Gruin --- Unchanged since v4. builtin/rebase--interactive.c | 15 +++-- sequencer.c | 110 +- sequencer.h | 5 +- 3 files changed, 82 insertions(+), 48 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index df19ccaeb9..53056ee713 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -65,7 +65,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, const char *onto, const char *onto_name, const char *squash_onto, const char *head_name, const char *restrict_revision, char *raw_strategies, -const char *cmd, unsigned autosquash) +struct string_list *commands, unsigned autosquash) { int ret; const char *head_hash = NULL; @@ -116,7 +116,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, discard_cache(); ret = complete_action(the_repository, opts, flags, shortrevisions, onto_name, onto, - head_hash, cmd, autosquash); + head_hash, commands, autosquash); } free(revisions); @@ -139,6 +139,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) const char *onto = NULL, *onto_name = NULL, *restrict_revision = NULL, *squash_onto = NULL, *upstream = NULL, *head_name = NULL, *switch_to = NULL, *cmd = NULL; + struct string_list commands = STRING_LIST_INIT_DUP; char *raw_strategies = NULL; enum { NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH, @@ -221,6 +222,12 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) warning(_("--[no-]rebase-cousins has no effect without " "--rebase-merges")); + if (cmd && *cmd) { + string_list_split(&commands, cmd, '\n', -1); + if (strlen(commands.items[commands.nr - 1].string) == 0) + --commands.nr; + } + switch (command) { case NONE: if (!onto && !upstream) @@ -228,7 +235,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = do_interactive_rebase(&opts, flags, switch_to, upstream, onto, onto_name, squash_onto, head_name, restrict_revision, - raw_strategies, cmd, autosquash); + raw_strategies, &commands, autosquash); break; case SKIP: { struct string_list merge_rr = STRING_LIST_INIT_DUP; @@ -262,7 +269,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = rearrange_squash(the_repository); break; case ADD_EXEC: - ret = sequencer_add_exec_commands(the_repository, cmd); + ret = sequencer_add_exec_commands(the_repository, &commands); break; default: BUG("invalid command '%d'", command); diff --git a/sequencer.c b/sequencer.c index 266f80d704..3a90b419d7 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4446,25 +4446,27 @@ int sequencer_make_script(struct repository *r, FILE *out, return 0; } -/* - * Add commands after pick and (series of) squash/fixup commands - * in the todo list. - */ -int sequencer_add_exec_commands(struct repository *r, - const char *commands) +static void todo_list_add_exec_commands(struc
[PATCH v5 12/16] rebase-interactive: use todo_list_write_to_file() in edit_todo_list()
Just like complete_action(), edit_todo_list() used a function (transform_todo_file()) that read the todo list from the disk and wrote it back, resulting in useless disk accesses. This changes edit_todo_list() to call directly todo_list_write_to_file() instead. Signed-off-by: Alban Gruin --- Squashed <5440ddf5-0b80-3d00-7daf-133a8611e...@ramsayjones.plus.com> from Ramsay Jones. rebase-interactive.c | 38 -- sequencer.c | 4 ++-- sequencer.h | 3 --- 3 files changed, 18 insertions(+), 27 deletions(-) diff --git a/rebase-interactive.c b/rebase-interactive.c index dfa6dd530f..d396ecc599 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -79,39 +79,33 @@ void append_todo_help(unsigned edit_todo, unsigned keep_empty, int edit_todo_list(struct repository *r, unsigned flags) { - struct strbuf buf = STRBUF_INIT; const char *todo_file = rebase_path_todo(); + struct todo_list todo_list = TODO_LIST_INIT; + int res = 0; - if (strbuf_read_file(&buf, todo_file, 0) < 0) + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) return error_errno(_("could not read '%s'."), todo_file); - strbuf_stripspace(&buf, 1); - if (write_message(buf.buf, buf.len, todo_file, 0)) { - strbuf_release(&buf); + strbuf_stripspace(&todo_list.buf, 1); + todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list); + if (todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, -1, + flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP)) { + todo_list_release(&todo_list); return -1; } - strbuf_release(&buf); - - transform_todo_file(r, flags | TODO_LIST_SHORTEN_IDS); - - if (strbuf_read_file(&buf, todo_file, 0) < 0) - return error_errno(_("could not read '%s'."), todo_file); - - append_todo_help(1, 0, &buf); - if (write_message(buf.buf, buf.len, todo_file, 0)) { - strbuf_release(&buf); + strbuf_reset(&todo_list.buf); + if (launch_sequence_editor(todo_file, &todo_list.buf, NULL)) { + todo_list_release(&todo_list); return -1; } - strbuf_release(&buf); + if (!todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) + res = todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, -1, + flags & ~(TODO_LIST_SHORTEN_IDS)); - if (launch_sequence_editor(todo_file, NULL, NULL)) - return -1; - - transform_todo_file(r, flags & ~(TODO_LIST_SHORTEN_IDS)); - - return 0; + todo_list_release(&todo_list); + return res; } define_commit_slab(commit_seen, unsigned char); diff --git a/sequencer.c b/sequencer.c index a817afffa9..d8d045067c 100644 --- a/sequencer.c +++ b/sequencer.c @@ -383,8 +383,8 @@ static void print_advice(struct repository *r, int show_hint, } } -int write_message(const void *buf, size_t len, const char *filename, - int append_eol) +static int write_message(const void *buf, size_t len, const char *filename, +int append_eol) { struct lock_file msg_file = LOCK_INIT; diff --git a/sequencer.h b/sequencer.h index 0c30e43f0a..c5bee8124c 100644 --- a/sequencer.h +++ b/sequencer.h @@ -64,9 +64,6 @@ struct replay_opts { }; #define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT } -int write_message(const void *buf, size_t len, const char *filename, - int append_eol); - /* * Note that ordering matters in this enum. Not only must it match the mapping * of todo_command_info (in sequencer.c), it is also divided into several -- 2.20.1
[PATCH v5 13/16] rebase-interactive: append_todo_help() changes
This moves the writing of the comment "Rebase $shortrevisions onto $shortonto ($command_count commands)" from todo_list_write_to_file() to append_todo_help(). shortrevisions, shortonto, and command_count are passed as parameters to append_todo_help(). During the initial edit of the todo list, shortrevisions and shortonto are not NULL. Therefore, if shortrevisions or shortonto is NULL, then edit_todo would be true, otherwise it would be false. Thus, edit_todo is removed from the parameters of append_todo_help(). Signed-off-by: Alban Gruin --- rebase-interactive.c | 12 +++- rebase-interactive.h | 3 ++- sequencer.c | 17 - 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/rebase-interactive.c b/rebase-interactive.c index d396ecc599..807f8370db 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -28,7 +28,8 @@ static enum missing_commit_check_level get_missing_commit_check_level(void) return MISSING_COMMIT_CHECK_IGNORE; } -void append_todo_help(unsigned edit_todo, unsigned keep_empty, +void append_todo_help(unsigned keep_empty, int command_count, + const char *shortrevisions, const char *shortonto, struct strbuf *buf) { const char *msg = _("\nCommands:\n" @@ -48,6 +49,15 @@ void append_todo_help(unsigned edit_todo, unsigned keep_empty, ". specified). Use -c to reword the commit message.\n" "\n" "These lines can be re-ordered; they are executed from top to bottom.\n"); + unsigned edit_todo = !(shortrevisions && shortonto); + + if (!edit_todo) { + strbuf_addch(buf, '\n'); + strbuf_commented_addf(buf, Q_("Rebase %s onto %s (%d command)", + "Rebase %s onto %s (%d commands)", + command_count), + shortrevisions, shortonto, command_count); + } strbuf_add_commented_lines(buf, msg, strlen(msg)); diff --git a/rebase-interactive.h b/rebase-interactive.h index 187b5032d6..0e5925e3aa 100644 --- a/rebase-interactive.h +++ b/rebase-interactive.h @@ -5,7 +5,8 @@ struct strbuf; struct repository; struct todo_list; -void append_todo_help(unsigned edit_todo, unsigned keep_empty, +void append_todo_help(unsigned keep_empty, int command_count, + const char *shortrevisions, const char *shortonto, struct strbuf *buf); int edit_todo_list(struct repository *r, unsigned flags); int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo); diff --git a/sequencer.c b/sequencer.c index d8d045067c..92de982bc4 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4580,22 +4580,13 @@ int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list, const char *file, const char *shortrevisions, const char *shortonto, int num, unsigned flags) { - int edit_todo = !(shortrevisions && shortonto), res; + int res; struct strbuf buf = STRBUF_INIT; todo_list_to_strbuf(r, todo_list, &buf, num, flags); - - if (flags & TODO_LIST_APPEND_TODO_HELP) { - int command_count = count_commands(todo_list); - if (!edit_todo) { - strbuf_addch(&buf, '\n'); - strbuf_commented_addf(&buf, Q_("Rebase %s onto %s (%d command)", - "Rebase %s onto %s (%d commands)", - command_count), - shortrevisions, shortonto, command_count); - } - append_todo_help(edit_todo, flags & TODO_LIST_KEEP_EMPTY, &buf); - } + if (flags & TODO_LIST_APPEND_TODO_HELP) + append_todo_help(flags & TODO_LIST_KEEP_EMPTY, count_commands(todo_list), +shortrevisions, shortonto, &buf); res = write_message(buf.buf, buf.len, file, 0); strbuf_release(&buf); -- 2.20.1
[PATCH v5 01/16] sequencer: changes in parse_insn_buffer()
This clears the number of items of a todo_list before parsing it to allow to parse the same list multiple times without issues. As its items are not dynamically allocated, or don’t need to allocate memory, no additionnal memory management is required here. Furthermore, if a line is invalid, the type of the corresponding command is set to a garbage value, and its argument is defined properly. This will allow to recreate the text of a todo list from its commands, even if one of them is incorrect. Signed-off-by: Alban Gruin --- Unchanged since v4. sequencer.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index d726f77e11..a7afaf6882 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2091,6 +2091,8 @@ static int parse_insn_buffer(struct repository *r, char *buf, char *p = buf, *next_p; int i, res = 0, fixup_okay = file_exists(rebase_path_done()); + todo_list->current = todo_list->nr = 0; + for (i = 1; *p; i++, p = next_p) { char *eol = strchrnul(p, '\n'); @@ -2104,7 +2106,10 @@ static int parse_insn_buffer(struct repository *r, char *buf, if (parse_insn_line(r, item, p, eol)) { res = error(_("invalid line %d: %.*s"), i, (int)(eol - p), p); - item->command = TODO_NOOP; + item->command = TODO_COMMENT + 1; + item->arg = p; + item->arg_len = (int)(eol - p); + item->commit = NULL; } if (fixup_okay) -- 2.20.1
[PATCH v5 08/16] sequencer: refactor rearrange_squash() to work on a todo_list
This refactors rearrange_squash() to work on a todo_list to avoid redundant reads and writes. The function is renamed todo_list_rearrange_squash(). The old version created a new buffer, which was directly written to the disk. This new version creates a new item list by just copying items from the old item list, without creating a new buffer. This eliminates the need to reparse the todo list, but this also means its buffer cannot be directly written to the disk. As rebase -p still need to check the todo list from the disk, a new function is introduced, rearrange_squash_in_todo_file(). complete_action() still uses rearrange_squash_in_todo_file() for now. This will be changed in a future commit. Signed-off-by: Alban Gruin --- Unchanged since v4. builtin/rebase--interactive.c | 2 +- sequencer.c | 92 ++- sequencer.h | 2 +- 3 files changed, 49 insertions(+), 47 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 53056ee713..4f0eae9239 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -266,7 +266,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = check_todo_list_from_file(the_repository); break; case REARRANGE_SQUASH: - ret = rearrange_squash(the_repository); + ret = rearrange_squash_in_todo_file(the_repository); break; case ADD_EXEC: ret = sequencer_add_exec_commands(the_repository, &commands); diff --git a/sequencer.c b/sequencer.c index 3a90b419d7..11456be5cc 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4777,7 +4777,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla write_message("noop\n", 5, todo_file, 0)) return -1; - if (autosquash && rearrange_squash(r)) + if (autosquash && rearrange_squash_in_todo_file(r)) return -1; if (commands->nr) @@ -4883,21 +4883,13 @@ define_commit_slab(commit_todo_item, struct todo_item *); * message will have to be retrieved from the commit (as the oneline in the * script cannot be trusted) in order to normalize the autosquash arrangement. */ -int rearrange_squash(struct repository *r) +static int todo_list_rearrange_squash(struct todo_list *todo_list) { - const char *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; struct hashmap subject2item; - int res = 0, rearranged = 0, *next, *tail, i; + int rearranged = 0, *next, *tail, i, nr = 0, alloc = 0; char **subjects; struct commit_todo_item commit_todo; - - if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0) - return -1; - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list) < 0) { - todo_list_release(&todo_list); - return -1; - } + struct todo_item *items = NULL; init_commit_todo_item(&commit_todo); /* @@ -4910,13 +4902,13 @@ int rearrange_squash(struct repository *r) * be moved to appear after the i'th. */ hashmap_init(&subject2item, (hashmap_cmp_fn) subject2item_cmp, -NULL, todo_list.nr); - ALLOC_ARRAY(next, todo_list.nr); - ALLOC_ARRAY(tail, todo_list.nr); - ALLOC_ARRAY(subjects, todo_list.nr); - for (i = 0; i < todo_list.nr; i++) { +NULL, todo_list->nr); + ALLOC_ARRAY(next, todo_list->nr); + ALLOC_ARRAY(tail, todo_list->nr); + ALLOC_ARRAY(subjects, todo_list->nr); + for (i = 0; i < todo_list->nr; i++) { struct strbuf buf = STRBUF_INIT; - struct todo_item *item = todo_list.items + i; + struct todo_item *item = todo_list->items + i; const char *commit_buffer, *subject, *p; size_t subject_len; int i2 = -1; @@ -4929,7 +4921,6 @@ int rearrange_squash(struct repository *r) } if (is_fixup(item->command)) { - todo_list_release(&todo_list); clear_commit_todo_item(&commit_todo); return error(_("the script was already rearranged.")); } @@ -4964,7 +4955,7 @@ int rearrange_squash(struct repository *r) *commit_todo_item_at(&commit_todo, commit2)) /* found by commit name */ i2 = *commit_todo_item_at(&commit_todo, commit2) - - todo_list.items; + - todo_list->items; else { /* copy can b
[PATCH v5 06/16] sequencer: refactor check_todo_list() to work on a todo_list
This refactors check_todo_list() to work on a todo_list to avoid redundant reads and writes to the disk. The function is renamed todo_list_check(). The parsing of the two todo lists is left to the caller. As rebase -p still need to check the todo list from the disk, a new function is introduced, check_todo_list_from_file(). It reads the file from the disk, parses it, pass the todo_list to todo_list_check(), and writes it back to the disk. As get_missing_commit_check_level() and the enum missing_commit_check_level are no longer needed inside of sequencer.c, they are moved to rebase-interactive.c, and made static again. Signed-off-by: Alban Gruin --- Squashed from Ramsay Jones. builtin/rebase--interactive.c | 2 +- rebase-interactive.c | 91 - rebase-interactive.h | 2 + sequencer.c | 121 +++--- sequencer.h | 9 +-- 5 files changed, 117 insertions(+), 108 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 0898eb4c59..df19ccaeb9 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -256,7 +256,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = transform_todo_file(the_repository, flags); break; case CHECK_TODO_LIST: - ret = check_todo_list(the_repository); + ret = check_todo_list_from_file(the_repository); break; case REARRANGE_SQUASH: ret = rearrange_squash(the_repository); diff --git a/rebase-interactive.c b/rebase-interactive.c index 842fa07e7e..dfa6dd530f 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -1,8 +1,32 @@ #include "cache.h" #include "commit.h" -#include "rebase-interactive.h" #include "sequencer.h" +#include "rebase-interactive.h" #include "strbuf.h" +#include "commit-slab.h" +#include "config.h" + +enum missing_commit_check_level { + MISSING_COMMIT_CHECK_IGNORE = 0, + MISSING_COMMIT_CHECK_WARN, + MISSING_COMMIT_CHECK_ERROR +}; + +static enum missing_commit_check_level get_missing_commit_check_level(void) +{ + const char *value; + + if (git_config_get_value("rebase.missingcommitscheck", &value) || + !strcasecmp("ignore", value)) + return MISSING_COMMIT_CHECK_IGNORE; + if (!strcasecmp("warn", value)) + return MISSING_COMMIT_CHECK_WARN; + if (!strcasecmp("error", value)) + return MISSING_COMMIT_CHECK_ERROR; + warning(_("unrecognized setting %s for option " + "rebase.missingCommitsCheck. Ignoring."), value); + return MISSING_COMMIT_CHECK_IGNORE; +} void append_todo_help(unsigned edit_todo, unsigned keep_empty, struct strbuf *buf) @@ -89,3 +113,68 @@ int edit_todo_list(struct repository *r, unsigned flags) return 0; } + +define_commit_slab(commit_seen, unsigned char); +/* + * Check if the user dropped some commits by mistake + * Behaviour determined by rebase.missingCommitsCheck. + * Check if there is an unrecognized command or a + * bad SHA-1 in a command. + */ +int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo) +{ + enum missing_commit_check_level check_level = get_missing_commit_check_level(); + struct strbuf missing = STRBUF_INIT; + int res = 0, i; + struct commit_seen commit_seen; + + init_commit_seen(&commit_seen); + + if (check_level == MISSING_COMMIT_CHECK_IGNORE) + goto leave_check; + + /* Mark the commits in git-rebase-todo as seen */ + for (i = 0; i < new_todo->nr; i++) { + struct commit *commit = new_todo->items[i].commit; + if (commit) + *commit_seen_at(&commit_seen, commit) = 1; + } + + /* Find commits in git-rebase-todo.backup yet unseen */ + for (i = old_todo->nr - 1; i >= 0; i--) { + struct todo_item *item = old_todo->items + i; + struct commit *commit = item->commit; + if (commit && !*commit_seen_at(&commit_seen, commit)) { + strbuf_addf(&missing, " - %s %.*s\n", + find_unique_abbrev(&commit->object.oid, DEFAULT_ABBREV), + item->arg_len, + todo_item_get_arg(old_todo, item)); + *commit_seen_at(&commit_seen, commit) = 1; + } + } + + /* Warn about missing commits */ + if (!missing.len) + goto leave_check; + + if (check_level == MISSING_COMMIT_CHECK_ERROR) +
[PATCH v5 10/16] sequencer: change complete_action() to use the refactored functions
complete_action() used functions that read the todo-list file, made some changes to it, and wrote it back to the disk. The previous commits were dedicated to separate the part that deals with the file from the actual logic of these functions. Now that this is done, we can call directly the "logic" functions to avoid useless file access. The parsing of the list has to be done by the caller. If the buffer of the todo list provided by the caller is empty, a `noop' command is directly added to the todo list, without touching the buffer. Signed-off-by: Alban Gruin --- Unchanged since v4. builtin/rebase--interactive.c | 20 +++-- sequencer.c | 81 +++ sequencer.h | 2 +- 3 files changed, 42 insertions(+), 61 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 92026739c9..2dbf8fc08b 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -71,7 +71,6 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, const char *head_hash = NULL; char *revisions = NULL, *shortrevisions = NULL; struct argv_array make_script_args = ARGV_ARRAY_INIT; - FILE *todo_list_file; struct todo_list todo_list = TODO_LIST_INIT; if (prepare_branch_to_be_rebased(opts, switch_to)) @@ -94,14 +93,6 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, if (!upstream && squash_onto) write_file(path_squash_onto(), "%s\n", squash_onto); - todo_list_file = fopen(rebase_path_todo(), "w"); - if (!todo_list_file) { - free(revisions); - free(shortrevisions); - - return error_errno(_("could not open %s"), rebase_path_todo()); - } - argv_array_pushl(&make_script_args, "", revisions, NULL); if (restrict_revision) argv_array_push(&make_script_args, restrict_revision); @@ -109,16 +100,17 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, ret = sequencer_make_script(the_repository, &todo_list.buf, make_script_args.argc, make_script_args.argv, flags); - fputs(todo_list.buf.buf, todo_list_file); - fclose(todo_list_file); if (ret) error(_("could not generate todo list")); else { discard_cache(); - ret = complete_action(the_repository, opts, flags, - shortrevisions, onto_name, onto, - head_hash, commands, autosquash); + if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf, + &todo_list)) + BUG("unusable todo list"); + + ret = complete_action(the_repository, opts, flags, shortrevisions, onto_name, + onto, head_hash, commands, autosquash, &todo_list); } free(revisions); diff --git a/sequencer.c b/sequencer.c index f1c62c5960..2a43ca685b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4754,100 +4754,89 @@ static int skip_unnecessary_picks(struct repository *r, struct object_id *output return 0; } +static int todo_list_rearrange_squash(struct todo_list *todo_list); + int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags, const char *shortrevisions, const char *onto_name, const char *onto, const char *orig_head, struct string_list *commands, - unsigned autosquash) + unsigned autosquash, struct todo_list *todo_list) { const char *shortonto, *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; - struct strbuf *buf = &(todo_list.buf); + struct todo_list new_todo = TODO_LIST_INIT; + struct strbuf *buf = &todo_list->buf; struct object_id oid; - struct stat st; get_oid(onto, &oid); shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV); - if (!lstat(todo_file, &st) && st.st_size == 0 && - write_message("noop\n", 5, todo_file, 0)) - return -1; + if (buf->len == 0) { + struct todo_item *item = append_new_todo(todo_list); + item->command = TODO_NOOP; + item->commit = NULL; + item->arg_len = item->arg_offset = item->flags = item->offset_in_buf = 0; + } - if (autosquash && rearrange_squash_in_todo_file(r)) + if (autosquash && todo_list_rearrange_squash(todo_list)) return -1; i
[PATCH v5 15/16] sequencer: use edit_todo_list() in complete_action()
This changes complete_action() to use edit_todo_list(), now that it can handle the initial edit of the todo list. Signed-off-by: Alban Gruin --- Unchanged since v4. sequencer.c | 21 +++-- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/sequencer.c b/sequencer.c index 8f47f0cf39..21b04e0642 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4709,6 +4709,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla struct todo_list new_todo = TODO_LIST_INIT; struct strbuf *buf = &todo_list->buf; struct object_id oid; + int res; get_oid(onto, &oid); shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV); @@ -4733,24 +4734,16 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla return error(_("nothing to do")); } - if (todo_list_write_to_file(r, todo_list, todo_file, - shortrevisions, shortonto, -1, - flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP)) - return error_errno(_("could not write '%s'"), todo_file); - - if (copy_file(rebase_path_todo_backup(), todo_file, 0666)) - return error(_("could not copy '%s' to '%s'."), todo_file, -rebase_path_todo_backup()); - - if (launch_sequence_editor(todo_file, &new_todo.buf, NULL)) { + res = edit_todo_list(r, todo_list, &new_todo, shortrevisions, +shortonto, flags); + if (res == -1) + return -1; + else if (res == -2) { apply_autostash(opts); sequencer_remove_state(opts); return -1; - } - - strbuf_stripspace(&new_todo.buf, 1); - if (new_todo.buf.len == 0) { + } else if (res == -3) { apply_autostash(opts); sequencer_remove_state(opts); todo_list_release(&new_todo); -- 2.20.1
[PATCH v5 09/16] sequencer: make sequencer_make_script() write its script to a strbuf
This makes sequencer_make_script() write its script to a strbuf (ie. the buffer of a todo_list) instead of a FILE. This reduce the amount of read/write made by rebase interactive. Signed-off-by: Alban Gruin --- Unchanged since v4. builtin/rebase--interactive.c | 13 ++- sequencer.c | 41 +++ sequencer.h | 5 ++--- 3 files changed, 28 insertions(+), 31 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 4f0eae9239..92026739c9 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -71,7 +71,8 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, const char *head_hash = NULL; char *revisions = NULL, *shortrevisions = NULL; struct argv_array make_script_args = ARGV_ARRAY_INIT; - FILE *todo_list; + FILE *todo_list_file; + struct todo_list todo_list = TODO_LIST_INIT; if (prepare_branch_to_be_rebased(opts, switch_to)) return -1; @@ -93,8 +94,8 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, if (!upstream && squash_onto) write_file(path_squash_onto(), "%s\n", squash_onto); - todo_list = fopen(rebase_path_todo(), "w"); - if (!todo_list) { + todo_list_file = fopen(rebase_path_todo(), "w"); + if (!todo_list_file) { free(revisions); free(shortrevisions); @@ -105,10 +106,11 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, if (restrict_revision) argv_array_push(&make_script_args, restrict_revision); - ret = sequencer_make_script(the_repository, todo_list, + ret = sequencer_make_script(the_repository, &todo_list.buf, make_script_args.argc, make_script_args.argv, flags); - fclose(todo_list); + fputs(todo_list.buf.buf, todo_list_file); + fclose(todo_list_file); if (ret) error(_("could not generate todo list")); @@ -121,6 +123,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, free(revisions); free(shortrevisions); + todo_list_release(&todo_list); argv_array_clear(&make_script_args); return ret; diff --git a/sequencer.c b/sequencer.c index 11456be5cc..f1c62c5960 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4160,7 +4160,7 @@ static const char *label_oid(struct object_id *oid, const char *label, } static int make_script_with_merges(struct pretty_print_context *pp, - struct rev_info *revs, FILE *out, + struct rev_info *revs, struct strbuf *out, unsigned flags) { int keep_empty = flags & TODO_LIST_KEEP_EMPTY; @@ -4305,7 +4305,7 @@ static int make_script_with_merges(struct pretty_print_context *pp, * gathering commits not yet shown, reversing the list on the fly, * then outputting that list (labeling revisions as needed). */ - fprintf(out, "%s onto\n", cmd_label); + strbuf_addf(out, "%s onto\n", cmd_label); for (iter = tips; iter; iter = iter->next) { struct commit_list *list = NULL, *iter2; @@ -4315,9 +4315,9 @@ static int make_script_with_merges(struct pretty_print_context *pp, entry = oidmap_get(&state.commit2label, &commit->object.oid); if (entry) - fprintf(out, "\n%c Branch %s\n", comment_line_char, entry->string); + strbuf_addf(out, "\n%c Branch %s\n", comment_line_char, entry->string); else - fprintf(out, "\n"); + strbuf_addch(out, '\n'); while (oidset_contains(&interesting, &commit->object.oid) && !oidset_contains(&shown, &commit->object.oid)) { @@ -4330,8 +4330,8 @@ static int make_script_with_merges(struct pretty_print_context *pp, } if (!commit) - fprintf(out, "%s %s\n", cmd_reset, - rebase_cousins ? "onto" : "[new root]"); + strbuf_addf(out, "%s %s\n", cmd_reset, + rebase_cousins ? "onto" : "[new root]"); else { const char *to = NULL; @@ -4344,12 +4344,12 @@ static int make_script_with_merges(struct pretty_print_context *pp, &state); if (!to || !strcmp
[PATCH v5 00/16] sequencer: refactor functions working on a todo_list
At the center of the "interactive" part of the interactive rebase lies the todo list. When the user starts an interactive rebase, a todo list is generated, presented to the user (who then edits it using a text editor), read back, and then is checked and processed before the actual rebase takes place. Some of this processing includes adding execs commands, reordering fixup! and squash! commits, and checking if no commits were accidentally dropped by the user. Before I converted the interactive rebase in C, these functions were called by git-rebase--interactive.sh through git-rebase--helper. Since the only way to pass around a large amount of data between a shell script and a C program is to use a file (or any declination of a file), the functions that checked and processed the todo list were directly working on a file, the same file that the user edited. During the conversion, I did not address this issue, which lead to a complete_action() that reads the todo list file, does some computation based on its content, and writes it back to the disk, several times in the same function. As it is not an efficient way to handle a data structure, this patch series refactor the functions that processes the todo list to work on a todo_list structure instead of reading it from the disk. Some commits consists in modifying edit_todo_list() (initially used by --edit-todo) to handle the initial edition of the todo list, to increase code sharing. This is based on nd/the-index (36e7ed69de, "rebase-interactive.c: remove the_repository references"). Changes since v4: - Adding todo_item_get_arg() to return the address of the parameter of an item to avoid confusion. - Squashed and <5440ddf5-0b80-3d00-7daf-133a8611e...@ramsayjones.plus.com> from Ramsay Jones. Alban Gruin (16): sequencer: changes in parse_insn_buffer() sequencer: make the todo_list structure public sequencer: remove the 'arg' field from todo_item sequencer: refactor transform_todos() to work on a todo_list sequencer: introduce todo_list_write_to_file() sequencer: refactor check_todo_list() to work on a todo_list sequencer: refactor sequencer_add_exec_commands() to work on a todo_list sequencer: refactor rearrange_squash() to work on a todo_list sequencer: make sequencer_make_script() write its script to a strbuf sequencer: change complete_action() to use the refactored functions sequencer: refactor skip_unnecessary_picks() to work on a todo_list rebase-interactive: use todo_list_write_to_file() in edit_todo_list() rebase-interactive: append_todo_help() changes rebase-interactive: rewrite edit_todo_list() to handle the initial edit sequencer: use edit_todo_list() in complete_action() rebase--interactive: move transform_todo_file() to rebase--interactive.c builtin/rebase--interactive.c | 90 +++-- rebase-interactive.c | 143 +-- rebase-interactive.h | 9 +- sequencer.c | 687 ++ sequencer.h | 81 +++- 5 files changed, 533 insertions(+), 477 deletions(-) -- 2.20.1
[PATCH v5 03/16] sequencer: remove the 'arg' field from todo_item
The 'arg' field of todo_item used to store the address of the first byte of the parameter of a command in a todo list. It was associated with the length of the parameter (the 'arg_len' field). This replaces the 'arg' field by 'arg_offset'. This new field does not store the address of the parameter, but the position of the first character of the parameter in the buffer. todo_item_get_arg() is added to return the address of the parameter of an item. This will prevent todo_list_add_exec_commands() from having to do awful pointer arithmetics when growing the todo list buffer. Signed-off-by: Alban Gruin --- Introduction and use of todo_item_get_arg(). sequencer.c | 67 ++--- sequencer.h | 6 +++-- 2 files changed, 42 insertions(+), 31 deletions(-) diff --git a/sequencer.c b/sequencer.c index 5b84a20532..0e7ab16a05 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1949,8 +1949,14 @@ static struct todo_item *append_new_todo(struct todo_list *todo_list) return todo_list->items + todo_list->nr++; } +const char *todo_item_get_arg(struct todo_list *todo_list, + struct todo_item *item) +{ + return todo_list->buf.buf + item->arg_offset; +} + static int parse_insn_line(struct repository *r, struct todo_item *item, - const char *bol, char *eol) + const char *buf, const char *bol, char *eol) { struct object_id commit_oid; char *end_of_object_name; @@ -1964,7 +1970,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, if (bol == eol || *bol == '\r' || *bol == comment_line_char) { item->command = TODO_COMMENT; item->commit = NULL; - item->arg = bol; + item->arg_offset = bol - buf; item->arg_len = eol - bol; return 0; } @@ -1991,7 +1997,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, return error(_("%s does not accept arguments: '%s'"), command_to_string(item->command), bol); item->commit = NULL; - item->arg = bol; + item->arg_offset = bol - buf; item->arg_len = eol - bol; return 0; } @@ -2003,7 +2009,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, if (item->command == TODO_EXEC || item->command == TODO_LABEL || item->command == TODO_RESET) { item->commit = NULL; - item->arg = bol; + item->arg_offset = bol - buf; item->arg_len = (int)(eol - bol); return 0; } @@ -2017,7 +2023,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, } else { item->flags |= TODO_EDIT_MERGE_MSG; item->commit = NULL; - item->arg = bol; + item->arg_offset = bol - buf; item->arg_len = (int)(eol - bol); return 0; } @@ -2029,8 +2035,9 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, status = get_oid(bol, &commit_oid); *end_of_object_name = saved; - item->arg = end_of_object_name + strspn(end_of_object_name, " \t"); - item->arg_len = (int)(eol - item->arg); + bol = end_of_object_name + strspn(end_of_object_name, " \t"); + item->arg_offset = bol - buf; + item->arg_len = (int)(eol - bol); if (status < 0) return -1; @@ -2058,11 +2065,11 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf, item = append_new_todo(todo_list); item->offset_in_buf = p - todo_list->buf.buf; - if (parse_insn_line(r, item, p, eol)) { + if (parse_insn_line(r, item, buf, p, eol)) { res = error(_("invalid line %d: %.*s"), i, (int)(eol - p), p); item->command = TODO_COMMENT + 1; - item->arg = p; + item->arg_offset = p - buf; item->arg_len = (int)(eol - p); item->commit = NULL; } @@ -2402,7 +2409,7 @@ static int walk_revs_populate_todo(struct todo_list *todo_list, item->command = command; item->commit = commit; - item->arg = NULL; + item->arg_offset = 0; item->arg_len = 0; item->offset_in_buf = to
[PATCH v5 02/16] sequencer: make the todo_list structure public
This makes the structures todo_list and todo_item, and the functions todo_list_release() and parse_insn_buffer(), accessible outside of sequencer.c. Signed-off-by: Alban Gruin --- Unchanged since v4. sequencer.c | 69 ++--- sequencer.h | 50 ++ 2 files changed, 62 insertions(+), 57 deletions(-) diff --git a/sequencer.c b/sequencer.c index a7afaf6882..5b84a20532 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1460,32 +1460,6 @@ static int allow_empty(struct repository *r, return 1; } -/* - * Note that ordering matters in this enum. Not only must it match the mapping - * below, it is also divided into several sections that matter. When adding - * new commands, make sure you add it in the right section. - */ -enum todo_command { - /* commands that handle commits */ - TODO_PICK = 0, - TODO_REVERT, - TODO_EDIT, - TODO_REWORD, - TODO_FIXUP, - TODO_SQUASH, - /* commands that do something else than handling a single commit */ - TODO_EXEC, - TODO_BREAK, - TODO_LABEL, - TODO_RESET, - TODO_MERGE, - /* commands that do nothing but are counted for reporting progress */ - TODO_NOOP, - TODO_DROP, - /* comments (not counted for reporting progress) */ - TODO_COMMENT -}; - static struct { char c; const char *str; @@ -1962,26 +1936,7 @@ enum todo_item_flags { TODO_EDIT_MERGE_MSG = 1 }; -struct todo_item { - enum todo_command command; - struct commit *commit; - unsigned int flags; - const char *arg; - int arg_len; - size_t offset_in_buf; -}; - -struct todo_list { - struct strbuf buf; - struct todo_item *items; - int nr, alloc, current; - int done_nr, total_nr; - struct stat_data stat; -}; - -#define TODO_LIST_INIT { STRBUF_INIT } - -static void todo_list_release(struct todo_list *todo_list) +void todo_list_release(struct todo_list *todo_list) { strbuf_release(&todo_list->buf); FREE_AND_NULL(todo_list->items); @@ -2084,8 +2039,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, return !item->commit; } -static int parse_insn_buffer(struct repository *r, char *buf, -struct todo_list *todo_list) +int todo_list_parse_insn_buffer(struct repository *r, char *buf, + struct todo_list *todo_list) { struct todo_item *item; char *p = buf, *next_p; @@ -2184,7 +2139,7 @@ static int read_populate_todo(struct repository *r, return error(_("could not stat '%s'"), todo_file); fill_stat_data(&todo_list->stat, &st); - res = parse_insn_buffer(r, todo_list->buf.buf, todo_list); + res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list); if (res) { if (is_rebase_i(opts)) return error(_("please fix this using " @@ -2215,7 +2170,7 @@ static int read_populate_todo(struct repository *r, FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w"); if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 && - !parse_insn_buffer(r, done.buf.buf, &done)) + !todo_list_parse_insn_buffer(r, done.buf.buf, &done)) todo_list->done_nr = count_commands(&done); else todo_list->done_nr = 0; @@ -4501,7 +4456,7 @@ int sequencer_add_exec_commands(struct repository *r, if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) return error(_("could not read '%s'."), todo_file); - if (parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { + if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { todo_list_release(&todo_list); return error(_("unusable todo list: '%s'"), todo_file); } @@ -4557,7 +4512,7 @@ int transform_todos(struct repository *r, unsigned flags) if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) return error(_("could not read '%s'."), todo_file); - if (parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { + if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { todo_list_release(&todo_list); return error(_("unusable todo list: '%s'"), todo_file); } @@ -4643,7 +4598,7 @@ int check_todo_list(struct repository *r) goto leave_check; } advise_to_edit_todo = res = - parse_insn_buffer(r, todo_list.buf.buf, &todo_list); + todo_list_parse_insn_
[PATCH v5 11/16] sequencer: refactor skip_unnecessary_picks() to work on a todo_list
This refactors skip_unnecessary_picks() to work on a todo_list. As this function is only called by complete_action() (and thus is not used by rebase -p), the file-handling logic is completely dropped here. Instead of truncating the todo list’s buffer, the items are moved to the beginning of the list, eliminating the need to reparse the list. This also means its buffer cannot be directly written to the disk. rewrite_file() is then removed, as it is now unused. Signed-off-by: Alban Gruin --- Unchanged since v4. sequencer.c | 78 - 1 file changed, 17 insertions(+), 61 deletions(-) diff --git a/sequencer.c b/sequencer.c index 2a43ca685b..a817afffa9 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4661,52 +4661,22 @@ int check_todo_list_from_file(struct repository *r) return res; } -static int rewrite_file(const char *path, const char *buf, size_t len) -{ - int rc = 0; - int fd = open(path, O_WRONLY | O_TRUNC); - if (fd < 0) - return error_errno(_("could not open '%s' for writing"), path); - if (write_in_full(fd, buf, len) < 0) - rc = error_errno(_("could not write to '%s'"), path); - if (close(fd) && !rc) - rc = error_errno(_("could not close '%s'"), path); - return rc; -} - /* skip picking commits whose parents are unchanged */ -static int skip_unnecessary_picks(struct repository *r, struct object_id *output_oid) +static int skip_unnecessary_picks(struct repository *r, + struct todo_list *todo_list, + struct object_id *output_oid) { - const char *todo_file = rebase_path_todo(); - struct strbuf buf = STRBUF_INIT; - struct todo_list todo_list = TODO_LIST_INIT; struct object_id *parent_oid; - int fd, i; - - if (!read_oneliner(&buf, rebase_path_onto(), 0)) - return error(_("could not read 'onto'")); - if (get_oid(buf.buf, output_oid)) { - strbuf_release(&buf); - return error(_("need a HEAD to fixup")); - } - strbuf_release(&buf); - - if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0) - return -1; - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list) < 0) { - todo_list_release(&todo_list); - return -1; - } + int i; - for (i = 0; i < todo_list.nr; i++) { - struct todo_item *item = todo_list.items + i; + for (i = 0; i < todo_list->nr; i++) { + struct todo_item *item = todo_list->items + i; if (item->command >= TODO_NOOP) continue; if (item->command != TODO_PICK) break; if (parse_commit(item->commit)) { - todo_list_release(&todo_list); return error(_("could not parse commit '%s'"), oid_to_hex(&item->commit->object.oid)); } @@ -4720,37 +4690,21 @@ static int skip_unnecessary_picks(struct repository *r, struct object_id *output oidcpy(output_oid, &item->commit->object.oid); } if (i > 0) { - int offset = get_item_line_offset(&todo_list, i); const char *done_path = rebase_path_done(); - fd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666); - if (fd < 0) { - error_errno(_("could not open '%s' for writing"), - done_path); - todo_list_release(&todo_list); - return -1; - } - if (write_in_full(fd, todo_list.buf.buf, offset) < 0) { + if (todo_list_write_to_file(r, todo_list, done_path, NULL, NULL, i, 0)) { error_errno(_("could not write to '%s'"), done_path); - todo_list_release(&todo_list); - close(fd); return -1; } - close(fd); - if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset, -todo_list.buf.len - offset) < 0) { - todo_list_release(&todo_list); - return -1; - } + MOVE_ARRAY(todo_list->items, todo_list->items + i, todo_list->nr - i); + todo_list->nr -= i; + todo_list->current = 0; - todo_list.current = i; - if (is_fixup(peek_command(&todo_list, 0))) -
[PATCH v5 14/16] rebase-interactive: rewrite edit_todo_list() to handle the initial edit
edit_todo_list() is changed to work on a todo_list, and to handle the initial edition of the todo list (ie. making a backup of the todo list). It does not check for dropped commits yet, as todo_list_check() does not take the commits that have already been processed by the rebase (ie. the todo list is edited in the middle of a rebase session). Signed-off-by: Alban Gruin --- Unchanged since v4. builtin/rebase--interactive.c | 24 +- rebase-interactive.c | 48 ++- rebase-interactive.h | 4 ++- sequencer.c | 3 +-- sequencer.h | 1 + 5 files changed, 53 insertions(+), 27 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 2dbf8fc08b..645ac587f7 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -13,6 +13,28 @@ static GIT_PATH_FUNC(path_state_dir, "rebase-merge/") static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto") static GIT_PATH_FUNC(path_interactive, "rebase-merge/interactive") +static int edit_todo_file(unsigned flags) +{ + const char *todo_file = rebase_path_todo(); + struct todo_list todo_list = TODO_LIST_INIT, + new_todo = TODO_LIST_INIT; + + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) + return error_errno(_("could not read '%s'."), todo_file); + + strbuf_stripspace(&todo_list.buf, 1); + if (!edit_todo_list(the_repository, &todo_list, + &new_todo, NULL, NULL, flags) && + todo_list_write_to_file(the_repository, &new_todo, todo_file, NULL, NULL, + -1, flags & ~(TODO_LIST_SHORTEN_IDS)) < 0) + return error_errno(_("could not write '%s'"), todo_file); + + todo_list_release(&todo_list); + todo_list_release(&new_todo); + + return 0; +} + static int get_revision_ranges(const char *upstream, const char *onto, const char **head_hash, char **revisions, char **shortrevisions) @@ -242,7 +264,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) break; } case EDIT_TODO: - ret = edit_todo_list(the_repository, flags); + ret = edit_todo_file(flags); break; case SHOW_CURRENT_PATCH: { struct child_process cmd = CHILD_PROCESS_INIT; diff --git a/rebase-interactive.c b/rebase-interactive.c index 807f8370db..3301efbe52 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -87,35 +87,37 @@ void append_todo_help(unsigned keep_empty, int command_count, } } -int edit_todo_list(struct repository *r, unsigned flags) +int edit_todo_list(struct repository *r, struct todo_list *todo_list, + struct todo_list *new_todo, const char *shortrevisions, + const char *shortonto, unsigned flags) { const char *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; - int res = 0; - - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) - return error_errno(_("could not read '%s'."), todo_file); - - strbuf_stripspace(&todo_list.buf, 1); - todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list); - if (todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, -1, - flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP)) { - todo_list_release(&todo_list); - return -1; + unsigned initial = shortrevisions && shortonto; + + if (initial) { + todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto, + -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP); + + if (copy_file(rebase_path_todo_backup(), todo_file, 0666)) + return error(_("could not copy '%s' to '%s'."), todo_file, +rebase_path_todo_backup()); + } else { + todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list); + todo_list_write_to_file(r, todo_list, todo_file, NULL, NULL, -1, + flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP); } - strbuf_reset(&todo_list.buf); - if (launch_sequence_editor(todo_file, &todo_list.buf, NULL)) { - todo_list_release(&todo_list); - return -1; - } + if (launch_sequence_editor(todo_file, &new_todo->buf, NULL)) + return -2; - if (!todo_list_
[PATCH v5 16/16] rebase--interactive: move transform_todo_file() to rebase--interactive.c
As transform_todo_file() is only needed inside of rebase--interactive.c, it is moved there from sequencer.c. Signed-off-by: Alban Gruin --- Unchanged since v4. builtin/rebase--interactive.c | 26 +- sequencer.c | 23 --- sequencer.h | 1 - 3 files changed, 25 insertions(+), 25 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 645ac587f7..7f1e88a087 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -35,6 +35,30 @@ static int edit_todo_file(unsigned flags) return 0; } +static int transform_todo_file(unsigned flags) +{ + const char *todo_file = rebase_path_todo(); + struct todo_list todo_list = TODO_LIST_INIT; + int res; + + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) + return error_errno(_("could not read '%s'."), todo_file); + + if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf, + &todo_list)) { + todo_list_release(&todo_list); + return error(_("unusable todo list: '%s'"), todo_file); + } + + res = todo_list_write_to_file(the_repository, &todo_list, todo_file, + NULL, NULL, -1, flags); + todo_list_release(&todo_list); + + if (res) + return error_errno(_("could not write '%s'."), todo_file); + return 0; +} + static int get_revision_ranges(const char *upstream, const char *onto, const char **head_hash, char **revisions, char **shortrevisions) @@ -277,7 +301,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) } case SHORTEN_OIDS: case EXPAND_OIDS: - ret = transform_todo_file(the_repository, flags); + ret = transform_todo_file(flags); break; case CHECK_TODO_LIST: ret = check_todo_list_from_file(the_repository); diff --git a/sequencer.c b/sequencer.c index 21b04e0642..5239700efc 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4593,29 +4593,6 @@ int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list, return res; } -int transform_todo_file(struct repository *r, unsigned flags) -{ - const char *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; - int res; - - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) - return error_errno(_("could not read '%s'."), todo_file); - - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { - todo_list_release(&todo_list); - return error(_("unusable todo list: '%s'"), todo_file); - } - - res = todo_list_write_to_file(r, &todo_list, todo_file, - NULL, NULL, -1, flags); - todo_list_release(&todo_list); - - if (res) - return error_errno(_("could not write '%s'."), todo_file); - return 0; -} - static const char edit_todo_list_advice[] = N_("You can fix this with 'git rebase --edit-todo' " "and then run 'git rebase --continue'.\n" diff --git a/sequencer.h b/sequencer.h index 68acab980b..11afd47aa9 100644 --- a/sequencer.h +++ b/sequencer.h @@ -145,7 +145,6 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc, int sequencer_add_exec_commands(struct repository *r, struct string_list *commands); -int transform_todo_file(struct repository *r, unsigned flags); int check_todo_list_from_file(struct repository *r); int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags, const char *shortrevisions, const char *onto_name, -- 2.20.1
Re: [PATCH v5 00/16] sequencer: refactor functions working on a todo_list
[I’m resending this as I clicked on the wrong button…] Hi, Le 24/01/2019 à 22:54, Junio C Hamano a écrit : > Alban Gruin writes: > > Before I comment on anything else. > > > This is based on nd/the-index (36e7ed69de, "rebase-interactive.c: remove > > the_repository references"). > > My attempt to apply these in order on top of that commit seems to > stop at step 5/16. Are you sure you based them on it? > > Thanks. It is based on that commit, but I mistakenly added a newline between `--- a/ sequencer.c` and `+++ b/sequencer.c` before sending this series. Sorry about this. I just reapplied all the patches I sent, and there is no other problem. Should I send this back? -- Alban
Re: [PATCH v2] rebase: move state_dir to tmp prior to deletion
Hi Ben, Le 26/01/2019 à 11:16, Ben Woosley a écrit : > From: Ben Woosley > > To avoid partial deletion / zombie rebases. > > Example behavior under partial deletion, after > Ctrl-Cing out of a standard rebase: > > $ git rebase target > First, rewinding head to replay your work on top of it... > Applying: [...] > ^C > $ git status > rebase in progress; onto (null) > You are currently rebasing. > (all conflicts fixed: run "git rebase --continue") > > Changes to be committed: > (use "git reset HEAD ..." to unstage) > [...] > $ git rebase --continue > error: could not read '.git/rebase-apply/head-name': No such file or > directory > $ git rebase --abort > error: could not read '.git/rebase-apply/head-name': No such file or > directory > > Others report this issue here: > https://stackoverflow.com/questions/3685001/git-how-to-fix-corrupted-interactive-rebase > --- > git-legacy-rebase.sh | 17 ++--- > git-rebase--preserve-merges.sh | 2 +- > 2 files changed, 15 insertions(+), 4 deletions(-) > This patch does not cover the new builtin rebase, even though it’s also affected by this bug. I ran in two bugs trying to reproduce this issue with the builtin: 1. Right after invoking `git rebase $target', $target was checked out but the state directory has not been written yet, leaving the user on the target without any possibility of aborting the rebase. I think it’s because the checkout is done by builtin/rebase.c, whereas the state directory is created by git-rebase--am -- and it might not be created at all! 2. If I wait a little bit, I have the same bug as yours. I tried to reproduce the issue on master (16a465bc01, "Third batch after 2.20") and builtin.userebase set to false, without success. It seems this issue has been fixed in the shell version, but not in the builtin version -- and it seems that you are using the latter, so it won’t solve your problem anyway. Try `git -c rebase.usebuiltin=false rebase --abort', and the state directory will be removed without any errors. Things are a bit different with interactive rebase: it checks out the target branch, then create its state directory. Only the first issue can happen, and you have to be very unlucky to run into it. The same goes for rebase -p. > diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh > index b4c7dbfa575d3..832a211c925c3 100755 > --- a/git-legacy-rebase.sh > +++ b/git-legacy-rebase.sh > @@ -128,11 +128,22 @@ read_basic_state () { > } > } > > +remove_rebase_state () { > + removal_dir=$(mktemp -d -t "git-rebase-state-XX") > + if test -d "$removal_dir" > + then > +mv "$state_dir" "$removal_dir" > + else > +removal_dir="$state_dir" > + fi > + rm -rf "$removal_dir" > +} > + > finish_rebase () { > rm -f "$(git rev-parse --git-path REBASE_HEAD)" > apply_autostash && > { git gc --auto || true; } && > - rm -rf "$state_dir" > + remove_rebase_state > } > > run_interactive () { > @@ -194,7 +205,7 @@ run_specific_rebase () { > elif test $ret -eq 2 # special exit status for rebase -p > then > apply_autostash && > - rm -rf "$state_dir" && > + remove_rebase_state && > die "Nothing to do" > fi > exit $ret > @@ -439,7 +450,7 @@ abort) > exit > ;; > quit) > - exec rm -rf "$state_dir" > + remove_rebase_state > ;; > edit-todo) > run_specific_rebase > diff --git a/git-rebase--preserve-merges.sh b/git-rebase--preserve-merges.sh > index afbb65765d461..146b52df14928 100644 > --- a/git-rebase--preserve-merges.sh > +++ b/git-rebase--preserve-merges.sh > @@ -226,7 +226,7 @@ Once you are satisfied with your changes, run > > die_abort () { > apply_autostash > - rm -rf "$state_dir" > + remove_rebase_state > die "$1" > } > > > -- > https://github.com/git/git/pull/569 > It seems that you tried to implement the solution proposed in the stackoverflow thread you linked. Unfortunately, if it fixed something, it has a problem: it won’t bring you back to the commit where you called `git rebase', unlike a `git rebase --abort' on a uncorrupted rebase state. This is what happens when aborting a corrupted rebase with the shell version of rebase. I think it’s because git-rebase--am will only create a state directory if it has a problem (ie. a conflict or the user hits ^C). So, there is definitely a problem with git-rebase--am, but this does not address it. -- Alban
Re: [PATCH v2 4/4] built-in rebase: call `git am` directly
Hi Johannes, Le 18/01/2019 à 16:09, Johannes Schindelin via GitGitGadget a écrit : > -%<- > +static int run_am(struct rebase_options *opts) > +{ > -%<- > + > + if (!status) { > + return move_to_original_branch(opts); > + } > + > + if (is_directory(opts->state_dir)) > + write_basic_state(opts); > + > + return status; > +} > + This means that the state directory will be created only if there is a problem (ie. a conflict), not if the user cancels the rebase by hitting ^C. In this case, the user will be left in a corrupted state where the builtin rebase cannot abort the rebase properly. -- Alban
[PATCH v6 07/16] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
This refactors sequencer_add_exec_commands() to work on a todo_list to avoid redundant reads and writes to the disk. Instead of inserting the `exec' commands between the other commands and re-parsing the buffer at the end, they are appended to the buffer once, and a new list of items is created. Items from the old list are copied across and new `exec' items are appended when necessary. This eliminates the need to reparse the buffer, but this also means we have to use todo_list_write_to_disk() to write the file. todo_list_add_exec_commands() and sequencer_add_exec_commands() are modified to take a string list instead of a string -- one item for each command. This makes it easier to insert a new command to the todo list for each command to execute. sequencer_add_exec_commands() still reads the todo list from the disk, as it is needed by rebase -p. complete_action() still uses sequencer_add_exec_commands() for now. This will be changed in a future commit. Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 15 +++-- sequencer.c | 110 +- sequencer.h | 5 +- 3 files changed, 82 insertions(+), 48 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index df19ccaeb9..53056ee713 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -65,7 +65,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, const char *onto, const char *onto_name, const char *squash_onto, const char *head_name, const char *restrict_revision, char *raw_strategies, -const char *cmd, unsigned autosquash) +struct string_list *commands, unsigned autosquash) { int ret; const char *head_hash = NULL; @@ -116,7 +116,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, discard_cache(); ret = complete_action(the_repository, opts, flags, shortrevisions, onto_name, onto, - head_hash, cmd, autosquash); + head_hash, commands, autosquash); } free(revisions); @@ -139,6 +139,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) const char *onto = NULL, *onto_name = NULL, *restrict_revision = NULL, *squash_onto = NULL, *upstream = NULL, *head_name = NULL, *switch_to = NULL, *cmd = NULL; + struct string_list commands = STRING_LIST_INIT_DUP; char *raw_strategies = NULL; enum { NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH, @@ -221,6 +222,12 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) warning(_("--[no-]rebase-cousins has no effect without " "--rebase-merges")); + if (cmd && *cmd) { + string_list_split(&commands, cmd, '\n', -1); + if (strlen(commands.items[commands.nr - 1].string) == 0) + --commands.nr; + } + switch (command) { case NONE: if (!onto && !upstream) @@ -228,7 +235,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = do_interactive_rebase(&opts, flags, switch_to, upstream, onto, onto_name, squash_onto, head_name, restrict_revision, - raw_strategies, cmd, autosquash); + raw_strategies, &commands, autosquash); break; case SKIP: { struct string_list merge_rr = STRING_LIST_INIT_DUP; @@ -262,7 +269,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = rearrange_squash(the_repository); break; case ADD_EXEC: - ret = sequencer_add_exec_commands(the_repository, cmd); + ret = sequencer_add_exec_commands(the_repository, &commands); break; default: BUG("invalid command '%d'", command); diff --git a/sequencer.c b/sequencer.c index 266f80d704..3a90b419d7 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4446,25 +4446,27 @@ int sequencer_make_script(struct repository *r, FILE *out, return 0; } -/* - * Add commands after pick and (series of) squash/fixup commands - * in the todo list. - */ -int sequencer_add_exec_commands(struct repository *r, - const char *commands) +static void todo_list_add_exec_commands(struct todo_list *todo_list, +
[PATCH v6 11/16] sequencer: refactor skip_unnecessary_picks() to work on a todo_list
This refactors skip_unnecessary_picks() to work on a todo_list. As this function is only called by complete_action() (and thus is not used by rebase -p), the file-handling logic is completely dropped here. Instead of truncating the todo list’s buffer, the items are moved to the beginning of the list, eliminating the need to reparse the list. This also means its buffer cannot be directly written to the disk. rewrite_file() is then removed, as it is now unused. Signed-off-by: Alban Gruin --- sequencer.c | 78 - 1 file changed, 17 insertions(+), 61 deletions(-) diff --git a/sequencer.c b/sequencer.c index 2a43ca685b..a817afffa9 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4661,52 +4661,22 @@ int check_todo_list_from_file(struct repository *r) return res; } -static int rewrite_file(const char *path, const char *buf, size_t len) -{ - int rc = 0; - int fd = open(path, O_WRONLY | O_TRUNC); - if (fd < 0) - return error_errno(_("could not open '%s' for writing"), path); - if (write_in_full(fd, buf, len) < 0) - rc = error_errno(_("could not write to '%s'"), path); - if (close(fd) && !rc) - rc = error_errno(_("could not close '%s'"), path); - return rc; -} - /* skip picking commits whose parents are unchanged */ -static int skip_unnecessary_picks(struct repository *r, struct object_id *output_oid) +static int skip_unnecessary_picks(struct repository *r, + struct todo_list *todo_list, + struct object_id *output_oid) { - const char *todo_file = rebase_path_todo(); - struct strbuf buf = STRBUF_INIT; - struct todo_list todo_list = TODO_LIST_INIT; struct object_id *parent_oid; - int fd, i; - - if (!read_oneliner(&buf, rebase_path_onto(), 0)) - return error(_("could not read 'onto'")); - if (get_oid(buf.buf, output_oid)) { - strbuf_release(&buf); - return error(_("need a HEAD to fixup")); - } - strbuf_release(&buf); - - if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0) - return -1; - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list) < 0) { - todo_list_release(&todo_list); - return -1; - } + int i; - for (i = 0; i < todo_list.nr; i++) { - struct todo_item *item = todo_list.items + i; + for (i = 0; i < todo_list->nr; i++) { + struct todo_item *item = todo_list->items + i; if (item->command >= TODO_NOOP) continue; if (item->command != TODO_PICK) break; if (parse_commit(item->commit)) { - todo_list_release(&todo_list); return error(_("could not parse commit '%s'"), oid_to_hex(&item->commit->object.oid)); } @@ -4720,37 +4690,21 @@ static int skip_unnecessary_picks(struct repository *r, struct object_id *output oidcpy(output_oid, &item->commit->object.oid); } if (i > 0) { - int offset = get_item_line_offset(&todo_list, i); const char *done_path = rebase_path_done(); - fd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666); - if (fd < 0) { - error_errno(_("could not open '%s' for writing"), - done_path); - todo_list_release(&todo_list); - return -1; - } - if (write_in_full(fd, todo_list.buf.buf, offset) < 0) { + if (todo_list_write_to_file(r, todo_list, done_path, NULL, NULL, i, 0)) { error_errno(_("could not write to '%s'"), done_path); - todo_list_release(&todo_list); - close(fd); return -1; } - close(fd); - if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset, -todo_list.buf.len - offset) < 0) { - todo_list_release(&todo_list); - return -1; - } + MOVE_ARRAY(todo_list->items, todo_list->items + i, todo_list->nr - i); + todo_list->nr -= i; + todo_list->current = 0; - todo_list.current = i; - if (is_fixup(peek_command(&todo_list, 0))) - record_in_rew
[PATCH v6 10/16] sequencer: change complete_action() to use the refactored functions
complete_action() used functions that read the todo-list file, made some changes to it, and wrote it back to the disk. The previous commits were dedicated to separate the part that deals with the file from the actual logic of these functions. Now that this is done, we can call directly the "logic" functions to avoid useless file access. The parsing of the list has to be done by the caller. If the buffer of the todo list provided by the caller is empty, a `noop' command is directly added to the todo list, without touching the buffer. Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 20 +++-- sequencer.c | 81 +++ sequencer.h | 2 +- 3 files changed, 42 insertions(+), 61 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 92026739c9..2dbf8fc08b 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -71,7 +71,6 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, const char *head_hash = NULL; char *revisions = NULL, *shortrevisions = NULL; struct argv_array make_script_args = ARGV_ARRAY_INIT; - FILE *todo_list_file; struct todo_list todo_list = TODO_LIST_INIT; if (prepare_branch_to_be_rebased(opts, switch_to)) @@ -94,14 +93,6 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, if (!upstream && squash_onto) write_file(path_squash_onto(), "%s\n", squash_onto); - todo_list_file = fopen(rebase_path_todo(), "w"); - if (!todo_list_file) { - free(revisions); - free(shortrevisions); - - return error_errno(_("could not open %s"), rebase_path_todo()); - } - argv_array_pushl(&make_script_args, "", revisions, NULL); if (restrict_revision) argv_array_push(&make_script_args, restrict_revision); @@ -109,16 +100,17 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, ret = sequencer_make_script(the_repository, &todo_list.buf, make_script_args.argc, make_script_args.argv, flags); - fputs(todo_list.buf.buf, todo_list_file); - fclose(todo_list_file); if (ret) error(_("could not generate todo list")); else { discard_cache(); - ret = complete_action(the_repository, opts, flags, - shortrevisions, onto_name, onto, - head_hash, commands, autosquash); + if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf, + &todo_list)) + BUG("unusable todo list"); + + ret = complete_action(the_repository, opts, flags, shortrevisions, onto_name, + onto, head_hash, commands, autosquash, &todo_list); } free(revisions); diff --git a/sequencer.c b/sequencer.c index f1c62c5960..2a43ca685b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4754,100 +4754,89 @@ static int skip_unnecessary_picks(struct repository *r, struct object_id *output return 0; } +static int todo_list_rearrange_squash(struct todo_list *todo_list); + int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags, const char *shortrevisions, const char *onto_name, const char *onto, const char *orig_head, struct string_list *commands, - unsigned autosquash) + unsigned autosquash, struct todo_list *todo_list) { const char *shortonto, *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; - struct strbuf *buf = &(todo_list.buf); + struct todo_list new_todo = TODO_LIST_INIT; + struct strbuf *buf = &todo_list->buf; struct object_id oid; - struct stat st; get_oid(onto, &oid); shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV); - if (!lstat(todo_file, &st) && st.st_size == 0 && - write_message("noop\n", 5, todo_file, 0)) - return -1; + if (buf->len == 0) { + struct todo_item *item = append_new_todo(todo_list); + item->command = TODO_NOOP; + item->commit = NULL; + item->arg_len = item->arg_offset = item->flags = item->offset_in_buf = 0; + } - if (autosquash && rearrange_squash_in_todo_file(r)) + if (autosquash && todo_list_rearrange_squash(todo_list)) return -1; if (co
[PATCH v6 08/16] sequencer: refactor rearrange_squash() to work on a todo_list
This refactors rearrange_squash() to work on a todo_list to avoid redundant reads and writes. The function is renamed todo_list_rearrange_squash(). The old version created a new buffer, which was directly written to the disk. This new version creates a new item list by just copying items from the old item list, without creating a new buffer. This eliminates the need to reparse the todo list, but this also means its buffer cannot be directly written to the disk. As rebase -p still need to check the todo list from the disk, a new function is introduced, rearrange_squash_in_todo_file(). complete_action() still uses rearrange_squash_in_todo_file() for now. This will be changed in a future commit. Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 2 +- sequencer.c | 92 ++- sequencer.h | 2 +- 3 files changed, 49 insertions(+), 47 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 53056ee713..4f0eae9239 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -266,7 +266,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = check_todo_list_from_file(the_repository); break; case REARRANGE_SQUASH: - ret = rearrange_squash(the_repository); + ret = rearrange_squash_in_todo_file(the_repository); break; case ADD_EXEC: ret = sequencer_add_exec_commands(the_repository, &commands); diff --git a/sequencer.c b/sequencer.c index 3a90b419d7..11456be5cc 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4777,7 +4777,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla write_message("noop\n", 5, todo_file, 0)) return -1; - if (autosquash && rearrange_squash(r)) + if (autosquash && rearrange_squash_in_todo_file(r)) return -1; if (commands->nr) @@ -4883,21 +4883,13 @@ define_commit_slab(commit_todo_item, struct todo_item *); * message will have to be retrieved from the commit (as the oneline in the * script cannot be trusted) in order to normalize the autosquash arrangement. */ -int rearrange_squash(struct repository *r) +static int todo_list_rearrange_squash(struct todo_list *todo_list) { - const char *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; struct hashmap subject2item; - int res = 0, rearranged = 0, *next, *tail, i; + int rearranged = 0, *next, *tail, i, nr = 0, alloc = 0; char **subjects; struct commit_todo_item commit_todo; - - if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0) - return -1; - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list) < 0) { - todo_list_release(&todo_list); - return -1; - } + struct todo_item *items = NULL; init_commit_todo_item(&commit_todo); /* @@ -4910,13 +4902,13 @@ int rearrange_squash(struct repository *r) * be moved to appear after the i'th. */ hashmap_init(&subject2item, (hashmap_cmp_fn) subject2item_cmp, -NULL, todo_list.nr); - ALLOC_ARRAY(next, todo_list.nr); - ALLOC_ARRAY(tail, todo_list.nr); - ALLOC_ARRAY(subjects, todo_list.nr); - for (i = 0; i < todo_list.nr; i++) { +NULL, todo_list->nr); + ALLOC_ARRAY(next, todo_list->nr); + ALLOC_ARRAY(tail, todo_list->nr); + ALLOC_ARRAY(subjects, todo_list->nr); + for (i = 0; i < todo_list->nr; i++) { struct strbuf buf = STRBUF_INIT; - struct todo_item *item = todo_list.items + i; + struct todo_item *item = todo_list->items + i; const char *commit_buffer, *subject, *p; size_t subject_len; int i2 = -1; @@ -4929,7 +4921,6 @@ int rearrange_squash(struct repository *r) } if (is_fixup(item->command)) { - todo_list_release(&todo_list); clear_commit_todo_item(&commit_todo); return error(_("the script was already rearranged.")); } @@ -4964,7 +4955,7 @@ int rearrange_squash(struct repository *r) *commit_todo_item_at(&commit_todo, commit2)) /* found by commit name */ i2 = *commit_todo_item_at(&commit_todo, commit2) - - todo_list.items; + - todo_list->items; else { /* copy can be a prefix of the commit
[PATCH v6 06/16] sequencer: refactor check_todo_list() to work on a todo_list
This refactors check_todo_list() to work on a todo_list to avoid redundant reads and writes to the disk. The function is renamed todo_list_check(). The parsing of the two todo lists is left to the caller. As rebase -p still need to check the todo list from the disk, a new function is introduced, check_todo_list_from_file(). It reads the file from the disk, parses it, pass the todo_list to todo_list_check(), and writes it back to the disk. As get_missing_commit_check_level() and the enum missing_commit_check_level are no longer needed inside of sequencer.c, they are moved to rebase-interactive.c, and made static again. Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 2 +- rebase-interactive.c | 91 - rebase-interactive.h | 2 + sequencer.c | 121 +++--- sequencer.h | 9 +-- 5 files changed, 117 insertions(+), 108 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 0898eb4c59..df19ccaeb9 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -256,7 +256,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = transform_todo_file(the_repository, flags); break; case CHECK_TODO_LIST: - ret = check_todo_list(the_repository); + ret = check_todo_list_from_file(the_repository); break; case REARRANGE_SQUASH: ret = rearrange_squash(the_repository); diff --git a/rebase-interactive.c b/rebase-interactive.c index 842fa07e7e..dfa6dd530f 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -1,8 +1,32 @@ #include "cache.h" #include "commit.h" -#include "rebase-interactive.h" #include "sequencer.h" +#include "rebase-interactive.h" #include "strbuf.h" +#include "commit-slab.h" +#include "config.h" + +enum missing_commit_check_level { + MISSING_COMMIT_CHECK_IGNORE = 0, + MISSING_COMMIT_CHECK_WARN, + MISSING_COMMIT_CHECK_ERROR +}; + +static enum missing_commit_check_level get_missing_commit_check_level(void) +{ + const char *value; + + if (git_config_get_value("rebase.missingcommitscheck", &value) || + !strcasecmp("ignore", value)) + return MISSING_COMMIT_CHECK_IGNORE; + if (!strcasecmp("warn", value)) + return MISSING_COMMIT_CHECK_WARN; + if (!strcasecmp("error", value)) + return MISSING_COMMIT_CHECK_ERROR; + warning(_("unrecognized setting %s for option " + "rebase.missingCommitsCheck. Ignoring."), value); + return MISSING_COMMIT_CHECK_IGNORE; +} void append_todo_help(unsigned edit_todo, unsigned keep_empty, struct strbuf *buf) @@ -89,3 +113,68 @@ int edit_todo_list(struct repository *r, unsigned flags) return 0; } + +define_commit_slab(commit_seen, unsigned char); +/* + * Check if the user dropped some commits by mistake + * Behaviour determined by rebase.missingCommitsCheck. + * Check if there is an unrecognized command or a + * bad SHA-1 in a command. + */ +int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo) +{ + enum missing_commit_check_level check_level = get_missing_commit_check_level(); + struct strbuf missing = STRBUF_INIT; + int res = 0, i; + struct commit_seen commit_seen; + + init_commit_seen(&commit_seen); + + if (check_level == MISSING_COMMIT_CHECK_IGNORE) + goto leave_check; + + /* Mark the commits in git-rebase-todo as seen */ + for (i = 0; i < new_todo->nr; i++) { + struct commit *commit = new_todo->items[i].commit; + if (commit) + *commit_seen_at(&commit_seen, commit) = 1; + } + + /* Find commits in git-rebase-todo.backup yet unseen */ + for (i = old_todo->nr - 1; i >= 0; i--) { + struct todo_item *item = old_todo->items + i; + struct commit *commit = item->commit; + if (commit && !*commit_seen_at(&commit_seen, commit)) { + strbuf_addf(&missing, " - %s %.*s\n", + find_unique_abbrev(&commit->object.oid, DEFAULT_ABBREV), + item->arg_len, + todo_item_get_arg(old_todo, item)); + *commit_seen_at(&commit_seen, commit) = 1; + } + } + + /* Warn about missing commits */ + if (!missing.len) + goto leave_check; + + if (check_level == MISSING_COMMIT_CHECK_ERROR) + res = 1; +
[PATCH v6 00/16] sequencer: refactor functions working on a todo_list
At the center of the "interactive" part of the interactive rebase lies the todo list. When the user starts an interactive rebase, a todo list is generated, presented to the user (who then edits it using a text editor), read back, and then is checked and processed before the actual rebase takes place. Some of this processing includes adding execs commands, reordering fixup! and squash! commits, and checking if no commits were accidentally dropped by the user. Before I converted the interactive rebase in C, these functions were called by git-rebase--interactive.sh through git-rebase--helper. Since the only way to pass around a large amount of data between a shell script and a C program is to use a file (or any declination of a file), the functions that checked and processed the todo list were directly working on a file, the same file that the user edited. During the conversion, I did not address this issue, which lead to a complete_action() that reads the todo list file, does some computation based on its content, and writes it back to the disk, several times in the same function. As it is not an efficient way to handle a data structure, this patch series refactor the functions that processes the todo list to work on a todo_list structure instead of reading it from the disk. Some commits consists in modifying edit_todo_list() (initially used by --edit-todo) to handle the initial edition of the todo list, to increase code sharing. This is based on nd/the-index (36e7ed69de, "rebase-interactive.c: remove the_repository references"). I am rerolling this because I accidentally added a newline to a place I shouldn’t in the patch 05/16 of the v5. There is no change in this version. Alban Gruin (16): sequencer: changes in parse_insn_buffer() sequencer: make the todo_list structure public sequencer: remove the 'arg' field from todo_item sequencer: refactor transform_todos() to work on a todo_list sequencer: introduce todo_list_write_to_file() sequencer: refactor check_todo_list() to work on a todo_list sequencer: refactor sequencer_add_exec_commands() to work on a todo_list sequencer: refactor rearrange_squash() to work on a todo_list sequencer: make sequencer_make_script() write its script to a strbuf sequencer: change complete_action() to use the refactored functions sequencer: refactor skip_unnecessary_picks() to work on a todo_list rebase-interactive: use todo_list_write_to_file() in edit_todo_list() rebase-interactive: append_todo_help() changes rebase-interactive: rewrite edit_todo_list() to handle the initial edit sequencer: use edit_todo_list() in complete_action() rebase--interactive: move transform_todo_file() to rebase--interactive.c builtin/rebase--interactive.c | 90 +++-- rebase-interactive.c | 143 +-- rebase-interactive.h | 9 +- sequencer.c | 687 ++ sequencer.h | 81 +++- 5 files changed, 533 insertions(+), 477 deletions(-) -- 2.20.1
[PATCH v6 13/16] rebase-interactive: append_todo_help() changes
This moves the writing of the comment "Rebase $shortrevisions onto $shortonto ($command_count commands)" from todo_list_write_to_file() to append_todo_help(). shortrevisions, shortonto, and command_count are passed as parameters to append_todo_help(). During the initial edit of the todo list, shortrevisions and shortonto are not NULL. Therefore, if shortrevisions or shortonto is NULL, then edit_todo would be true, otherwise it would be false. Thus, edit_todo is removed from the parameters of append_todo_help(). Signed-off-by: Alban Gruin --- rebase-interactive.c | 12 +++- rebase-interactive.h | 3 ++- sequencer.c | 17 - 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/rebase-interactive.c b/rebase-interactive.c index d396ecc599..807f8370db 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -28,7 +28,8 @@ static enum missing_commit_check_level get_missing_commit_check_level(void) return MISSING_COMMIT_CHECK_IGNORE; } -void append_todo_help(unsigned edit_todo, unsigned keep_empty, +void append_todo_help(unsigned keep_empty, int command_count, + const char *shortrevisions, const char *shortonto, struct strbuf *buf) { const char *msg = _("\nCommands:\n" @@ -48,6 +49,15 @@ void append_todo_help(unsigned edit_todo, unsigned keep_empty, ". specified). Use -c to reword the commit message.\n" "\n" "These lines can be re-ordered; they are executed from top to bottom.\n"); + unsigned edit_todo = !(shortrevisions && shortonto); + + if (!edit_todo) { + strbuf_addch(buf, '\n'); + strbuf_commented_addf(buf, Q_("Rebase %s onto %s (%d command)", + "Rebase %s onto %s (%d commands)", + command_count), + shortrevisions, shortonto, command_count); + } strbuf_add_commented_lines(buf, msg, strlen(msg)); diff --git a/rebase-interactive.h b/rebase-interactive.h index 187b5032d6..0e5925e3aa 100644 --- a/rebase-interactive.h +++ b/rebase-interactive.h @@ -5,7 +5,8 @@ struct strbuf; struct repository; struct todo_list; -void append_todo_help(unsigned edit_todo, unsigned keep_empty, +void append_todo_help(unsigned keep_empty, int command_count, + const char *shortrevisions, const char *shortonto, struct strbuf *buf); int edit_todo_list(struct repository *r, unsigned flags); int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo); diff --git a/sequencer.c b/sequencer.c index d8d045067c..92de982bc4 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4580,22 +4580,13 @@ int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list, const char *file, const char *shortrevisions, const char *shortonto, int num, unsigned flags) { - int edit_todo = !(shortrevisions && shortonto), res; + int res; struct strbuf buf = STRBUF_INIT; todo_list_to_strbuf(r, todo_list, &buf, num, flags); - - if (flags & TODO_LIST_APPEND_TODO_HELP) { - int command_count = count_commands(todo_list); - if (!edit_todo) { - strbuf_addch(&buf, '\n'); - strbuf_commented_addf(&buf, Q_("Rebase %s onto %s (%d command)", - "Rebase %s onto %s (%d commands)", - command_count), - shortrevisions, shortonto, command_count); - } - append_todo_help(edit_todo, flags & TODO_LIST_KEEP_EMPTY, &buf); - } + if (flags & TODO_LIST_APPEND_TODO_HELP) + append_todo_help(flags & TODO_LIST_KEEP_EMPTY, count_commands(todo_list), +shortrevisions, shortonto, &buf); res = write_message(buf.buf, buf.len, file, 0); strbuf_release(&buf); -- 2.20.1
[PATCH v6 02/16] sequencer: make the todo_list structure public
This makes the structures todo_list and todo_item, and the functions todo_list_release() and parse_insn_buffer(), accessible outside of sequencer.c. Signed-off-by: Alban Gruin --- sequencer.c | 69 ++--- sequencer.h | 50 ++ 2 files changed, 62 insertions(+), 57 deletions(-) diff --git a/sequencer.c b/sequencer.c index a7afaf6882..5b84a20532 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1460,32 +1460,6 @@ static int allow_empty(struct repository *r, return 1; } -/* - * Note that ordering matters in this enum. Not only must it match the mapping - * below, it is also divided into several sections that matter. When adding - * new commands, make sure you add it in the right section. - */ -enum todo_command { - /* commands that handle commits */ - TODO_PICK = 0, - TODO_REVERT, - TODO_EDIT, - TODO_REWORD, - TODO_FIXUP, - TODO_SQUASH, - /* commands that do something else than handling a single commit */ - TODO_EXEC, - TODO_BREAK, - TODO_LABEL, - TODO_RESET, - TODO_MERGE, - /* commands that do nothing but are counted for reporting progress */ - TODO_NOOP, - TODO_DROP, - /* comments (not counted for reporting progress) */ - TODO_COMMENT -}; - static struct { char c; const char *str; @@ -1962,26 +1936,7 @@ enum todo_item_flags { TODO_EDIT_MERGE_MSG = 1 }; -struct todo_item { - enum todo_command command; - struct commit *commit; - unsigned int flags; - const char *arg; - int arg_len; - size_t offset_in_buf; -}; - -struct todo_list { - struct strbuf buf; - struct todo_item *items; - int nr, alloc, current; - int done_nr, total_nr; - struct stat_data stat; -}; - -#define TODO_LIST_INIT { STRBUF_INIT } - -static void todo_list_release(struct todo_list *todo_list) +void todo_list_release(struct todo_list *todo_list) { strbuf_release(&todo_list->buf); FREE_AND_NULL(todo_list->items); @@ -2084,8 +2039,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, return !item->commit; } -static int parse_insn_buffer(struct repository *r, char *buf, -struct todo_list *todo_list) +int todo_list_parse_insn_buffer(struct repository *r, char *buf, + struct todo_list *todo_list) { struct todo_item *item; char *p = buf, *next_p; @@ -2184,7 +2139,7 @@ static int read_populate_todo(struct repository *r, return error(_("could not stat '%s'"), todo_file); fill_stat_data(&todo_list->stat, &st); - res = parse_insn_buffer(r, todo_list->buf.buf, todo_list); + res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list); if (res) { if (is_rebase_i(opts)) return error(_("please fix this using " @@ -2215,7 +2170,7 @@ static int read_populate_todo(struct repository *r, FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w"); if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 && - !parse_insn_buffer(r, done.buf.buf, &done)) + !todo_list_parse_insn_buffer(r, done.buf.buf, &done)) todo_list->done_nr = count_commands(&done); else todo_list->done_nr = 0; @@ -4501,7 +4456,7 @@ int sequencer_add_exec_commands(struct repository *r, if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) return error(_("could not read '%s'."), todo_file); - if (parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { + if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { todo_list_release(&todo_list); return error(_("unusable todo list: '%s'"), todo_file); } @@ -4557,7 +4512,7 @@ int transform_todos(struct repository *r, unsigned flags) if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) return error(_("could not read '%s'."), todo_file); - if (parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { + if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { todo_list_release(&todo_list); return error(_("unusable todo list: '%s'"), todo_file); } @@ -4643,7 +4598,7 @@ int check_todo_list(struct repository *r) goto leave_check; } advise_to_edit_todo = res = - parse_insn_buffer(r, todo_list.buf.buf, &todo_list); + todo_list_parse_insn_buffer(r, to
[PATCH v6 04/16] sequencer: refactor transform_todos() to work on a todo_list
This refactors transform_todos() to work on a todo_list. The function is renamed todo_list_transform(). As rebase -p still need to check the todo list from the disk, a new function is introduced, transform_todo_file(). It is still used by complete_action() and edit_todo_list() for now, but they will be replaced in a future commit. todo_list_transform() is not a static function, because it will be used by edit_todo_list() from rebase-interactive.c in a future commit. Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 2 +- rebase-interactive.c | 4 +-- sequencer.c | 54 +++ sequencer.h | 4 ++- 4 files changed, 42 insertions(+), 22 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index dd2a55ab1d..0898eb4c59 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -253,7 +253,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) } case SHORTEN_OIDS: case EXPAND_OIDS: - ret = transform_todos(the_repository, flags); + ret = transform_todo_file(the_repository, flags); break; case CHECK_TODO_LIST: ret = check_todo_list(the_repository); diff --git a/rebase-interactive.c b/rebase-interactive.c index 68aff1dac2..842fa07e7e 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -69,7 +69,7 @@ int edit_todo_list(struct repository *r, unsigned flags) strbuf_release(&buf); - transform_todos(r, flags | TODO_LIST_SHORTEN_IDS); + transform_todo_file(r, flags | TODO_LIST_SHORTEN_IDS); if (strbuf_read_file(&buf, todo_file, 0) < 0) return error_errno(_("could not read '%s'."), todo_file); @@ -85,7 +85,7 @@ int edit_todo_list(struct repository *r, unsigned flags) if (launch_sequence_editor(todo_file, NULL, NULL)) return -1; - transform_todos(r, flags & ~(TODO_LIST_SHORTEN_IDS)); + transform_todo_file(r, flags & ~(TODO_LIST_SHORTEN_IDS)); return 0; } diff --git a/sequencer.c b/sequencer.c index 0e7ab16a05..7a295cbd3f 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4507,27 +4507,18 @@ int sequencer_add_exec_commands(struct repository *r, return i; } -int transform_todos(struct repository *r, unsigned flags) +void todo_list_transform(struct repository *r, struct todo_list *todo_list, +unsigned flags) { - const char *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; struct strbuf buf = STRBUF_INIT; struct todo_item *item; int i; - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) - return error(_("could not read '%s'."), todo_file); - - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { - todo_list_release(&todo_list); - return error(_("unusable todo list: '%s'"), todo_file); - } - - for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) { + for (item = todo_list->items, i = 0; i < todo_list->nr; i++, item++) { /* if the item is not a command write it and continue */ if (item->command >= TODO_COMMENT) { strbuf_addf(&buf, "%.*s\n", item->arg_len, - todo_item_get_arg(&todo_list, item)); + todo_item_get_arg(todo_list, item)); continue; } @@ -4558,12 +4549,39 @@ int transform_todos(struct repository *r, unsigned flags) strbuf_addch(&buf, '\n'); else strbuf_addf(&buf, " %.*s\n", item->arg_len, - todo_item_get_arg(&todo_list, item)); + todo_item_get_arg(todo_list, item)); } - i = write_message(buf.buf, buf.len, todo_file, 0); + strbuf_reset(&todo_list->buf); + strbuf_add(&todo_list->buf, buf.buf, buf.len); + strbuf_release(&buf); + + if (todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list)) + BUG("unusable todo list"); +} + +int transform_todo_file(struct repository *r, unsigned flags) +{ + const char *todo_file = rebase_path_todo(); + struct todo_list todo_list = TODO_LIST_INIT; + int res; + + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) + return error_errno(_("could not read '%s'."), todo_file); + + if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { + todo_list_releas
[PATCH v6 03/16] sequencer: remove the 'arg' field from todo_item
The 'arg' field of todo_item used to store the address of the first byte of the parameter of a command in a todo list. It was associated with the length of the parameter (the 'arg_len' field). This replaces the 'arg' field by 'arg_offset'. This new field does not store the address of the parameter, but the position of the first character of the parameter in the buffer. todo_item_get_arg() is added to return the address of the parameter of an item. This will prevent todo_list_add_exec_commands() from having to do awful pointer arithmetics when growing the todo list buffer. Signed-off-by: Alban Gruin --- sequencer.c | 67 ++--- sequencer.h | 6 +++-- 2 files changed, 42 insertions(+), 31 deletions(-) diff --git a/sequencer.c b/sequencer.c index 5b84a20532..0e7ab16a05 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1949,8 +1949,14 @@ static struct todo_item *append_new_todo(struct todo_list *todo_list) return todo_list->items + todo_list->nr++; } +const char *todo_item_get_arg(struct todo_list *todo_list, + struct todo_item *item) +{ + return todo_list->buf.buf + item->arg_offset; +} + static int parse_insn_line(struct repository *r, struct todo_item *item, - const char *bol, char *eol) + const char *buf, const char *bol, char *eol) { struct object_id commit_oid; char *end_of_object_name; @@ -1964,7 +1970,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, if (bol == eol || *bol == '\r' || *bol == comment_line_char) { item->command = TODO_COMMENT; item->commit = NULL; - item->arg = bol; + item->arg_offset = bol - buf; item->arg_len = eol - bol; return 0; } @@ -1991,7 +1997,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, return error(_("%s does not accept arguments: '%s'"), command_to_string(item->command), bol); item->commit = NULL; - item->arg = bol; + item->arg_offset = bol - buf; item->arg_len = eol - bol; return 0; } @@ -2003,7 +2009,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, if (item->command == TODO_EXEC || item->command == TODO_LABEL || item->command == TODO_RESET) { item->commit = NULL; - item->arg = bol; + item->arg_offset = bol - buf; item->arg_len = (int)(eol - bol); return 0; } @@ -2017,7 +2023,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, } else { item->flags |= TODO_EDIT_MERGE_MSG; item->commit = NULL; - item->arg = bol; + item->arg_offset = bol - buf; item->arg_len = (int)(eol - bol); return 0; } @@ -2029,8 +2035,9 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, status = get_oid(bol, &commit_oid); *end_of_object_name = saved; - item->arg = end_of_object_name + strspn(end_of_object_name, " \t"); - item->arg_len = (int)(eol - item->arg); + bol = end_of_object_name + strspn(end_of_object_name, " \t"); + item->arg_offset = bol - buf; + item->arg_len = (int)(eol - bol); if (status < 0) return -1; @@ -2058,11 +2065,11 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf, item = append_new_todo(todo_list); item->offset_in_buf = p - todo_list->buf.buf; - if (parse_insn_line(r, item, p, eol)) { + if (parse_insn_line(r, item, buf, p, eol)) { res = error(_("invalid line %d: %.*s"), i, (int)(eol - p), p); item->command = TODO_COMMENT + 1; - item->arg = p; + item->arg_offset = p - buf; item->arg_len = (int)(eol - p); item->commit = NULL; } @@ -2402,7 +2409,7 @@ static int walk_revs_populate_todo(struct todo_list *todo_list, item->command = command; item->commit = commit; - item->arg = NULL; + item->arg_offset = 0; item->arg_len = 0; item->offset_in_buf = todo_list->buf.len; subject_le
[PATCH v6 01/16] sequencer: changes in parse_insn_buffer()
This clears the number of items of a todo_list before parsing it to allow to parse the same list multiple times without issues. As its items are not dynamically allocated, or don’t need to allocate memory, no additionnal memory management is required here. Furthermore, if a line is invalid, the type of the corresponding command is set to a garbage value, and its argument is defined properly. This will allow to recreate the text of a todo list from its commands, even if one of them is incorrect. Signed-off-by: Alban Gruin --- sequencer.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index d726f77e11..a7afaf6882 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2091,6 +2091,8 @@ static int parse_insn_buffer(struct repository *r, char *buf, char *p = buf, *next_p; int i, res = 0, fixup_okay = file_exists(rebase_path_done()); + todo_list->current = todo_list->nr = 0; + for (i = 1; *p; i++, p = next_p) { char *eol = strchrnul(p, '\n'); @@ -2104,7 +2106,10 @@ static int parse_insn_buffer(struct repository *r, char *buf, if (parse_insn_line(r, item, p, eol)) { res = error(_("invalid line %d: %.*s"), i, (int)(eol - p), p); - item->command = TODO_NOOP; + item->command = TODO_COMMENT + 1; + item->arg = p; + item->arg_len = (int)(eol - p); + item->commit = NULL; } if (fixup_okay) -- 2.20.1
[PATCH v6 09/16] sequencer: make sequencer_make_script() write its script to a strbuf
This makes sequencer_make_script() write its script to a strbuf (ie. the buffer of a todo_list) instead of a FILE. This reduce the amount of read/write made by rebase interactive. Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 13 ++- sequencer.c | 41 +++ sequencer.h | 5 ++--- 3 files changed, 28 insertions(+), 31 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 4f0eae9239..92026739c9 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -71,7 +71,8 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, const char *head_hash = NULL; char *revisions = NULL, *shortrevisions = NULL; struct argv_array make_script_args = ARGV_ARRAY_INIT; - FILE *todo_list; + FILE *todo_list_file; + struct todo_list todo_list = TODO_LIST_INIT; if (prepare_branch_to_be_rebased(opts, switch_to)) return -1; @@ -93,8 +94,8 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, if (!upstream && squash_onto) write_file(path_squash_onto(), "%s\n", squash_onto); - todo_list = fopen(rebase_path_todo(), "w"); - if (!todo_list) { + todo_list_file = fopen(rebase_path_todo(), "w"); + if (!todo_list_file) { free(revisions); free(shortrevisions); @@ -105,10 +106,11 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, if (restrict_revision) argv_array_push(&make_script_args, restrict_revision); - ret = sequencer_make_script(the_repository, todo_list, + ret = sequencer_make_script(the_repository, &todo_list.buf, make_script_args.argc, make_script_args.argv, flags); - fclose(todo_list); + fputs(todo_list.buf.buf, todo_list_file); + fclose(todo_list_file); if (ret) error(_("could not generate todo list")); @@ -121,6 +123,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, free(revisions); free(shortrevisions); + todo_list_release(&todo_list); argv_array_clear(&make_script_args); return ret; diff --git a/sequencer.c b/sequencer.c index 11456be5cc..f1c62c5960 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4160,7 +4160,7 @@ static const char *label_oid(struct object_id *oid, const char *label, } static int make_script_with_merges(struct pretty_print_context *pp, - struct rev_info *revs, FILE *out, + struct rev_info *revs, struct strbuf *out, unsigned flags) { int keep_empty = flags & TODO_LIST_KEEP_EMPTY; @@ -4305,7 +4305,7 @@ static int make_script_with_merges(struct pretty_print_context *pp, * gathering commits not yet shown, reversing the list on the fly, * then outputting that list (labeling revisions as needed). */ - fprintf(out, "%s onto\n", cmd_label); + strbuf_addf(out, "%s onto\n", cmd_label); for (iter = tips; iter; iter = iter->next) { struct commit_list *list = NULL, *iter2; @@ -4315,9 +4315,9 @@ static int make_script_with_merges(struct pretty_print_context *pp, entry = oidmap_get(&state.commit2label, &commit->object.oid); if (entry) - fprintf(out, "\n%c Branch %s\n", comment_line_char, entry->string); + strbuf_addf(out, "\n%c Branch %s\n", comment_line_char, entry->string); else - fprintf(out, "\n"); + strbuf_addch(out, '\n'); while (oidset_contains(&interesting, &commit->object.oid) && !oidset_contains(&shown, &commit->object.oid)) { @@ -4330,8 +4330,8 @@ static int make_script_with_merges(struct pretty_print_context *pp, } if (!commit) - fprintf(out, "%s %s\n", cmd_reset, - rebase_cousins ? "onto" : "[new root]"); + strbuf_addf(out, "%s %s\n", cmd_reset, + rebase_cousins ? "onto" : "[new root]"); else { const char *to = NULL; @@ -4344,12 +4344,12 @@ static int make_script_with_merges(struct pretty_print_context *pp, &state); if (!to || !strcmp
[PATCH v6 16/16] rebase--interactive: move transform_todo_file() to rebase--interactive.c
As transform_todo_file() is only needed inside of rebase--interactive.c, it is moved there from sequencer.c. Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 26 +- sequencer.c | 23 --- sequencer.h | 1 - 3 files changed, 25 insertions(+), 25 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 645ac587f7..7f1e88a087 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -35,6 +35,30 @@ static int edit_todo_file(unsigned flags) return 0; } +static int transform_todo_file(unsigned flags) +{ + const char *todo_file = rebase_path_todo(); + struct todo_list todo_list = TODO_LIST_INIT; + int res; + + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) + return error_errno(_("could not read '%s'."), todo_file); + + if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf, + &todo_list)) { + todo_list_release(&todo_list); + return error(_("unusable todo list: '%s'"), todo_file); + } + + res = todo_list_write_to_file(the_repository, &todo_list, todo_file, + NULL, NULL, -1, flags); + todo_list_release(&todo_list); + + if (res) + return error_errno(_("could not write '%s'."), todo_file); + return 0; +} + static int get_revision_ranges(const char *upstream, const char *onto, const char **head_hash, char **revisions, char **shortrevisions) @@ -277,7 +301,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) } case SHORTEN_OIDS: case EXPAND_OIDS: - ret = transform_todo_file(the_repository, flags); + ret = transform_todo_file(flags); break; case CHECK_TODO_LIST: ret = check_todo_list_from_file(the_repository); diff --git a/sequencer.c b/sequencer.c index 21b04e0642..5239700efc 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4593,29 +4593,6 @@ int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list, return res; } -int transform_todo_file(struct repository *r, unsigned flags) -{ - const char *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; - int res; - - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) - return error_errno(_("could not read '%s'."), todo_file); - - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { - todo_list_release(&todo_list); - return error(_("unusable todo list: '%s'"), todo_file); - } - - res = todo_list_write_to_file(r, &todo_list, todo_file, - NULL, NULL, -1, flags); - todo_list_release(&todo_list); - - if (res) - return error_errno(_("could not write '%s'."), todo_file); - return 0; -} - static const char edit_todo_list_advice[] = N_("You can fix this with 'git rebase --edit-todo' " "and then run 'git rebase --continue'.\n" diff --git a/sequencer.h b/sequencer.h index 68acab980b..11afd47aa9 100644 --- a/sequencer.h +++ b/sequencer.h @@ -145,7 +145,6 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc, int sequencer_add_exec_commands(struct repository *r, struct string_list *commands); -int transform_todo_file(struct repository *r, unsigned flags); int check_todo_list_from_file(struct repository *r); int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags, const char *shortrevisions, const char *onto_name, -- 2.20.1
[PATCH v6 14/16] rebase-interactive: rewrite edit_todo_list() to handle the initial edit
edit_todo_list() is changed to work on a todo_list, and to handle the initial edition of the todo list (ie. making a backup of the todo list). It does not check for dropped commits yet, as todo_list_check() does not take the commits that have already been processed by the rebase (ie. the todo list is edited in the middle of a rebase session). Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 24 +- rebase-interactive.c | 48 ++- rebase-interactive.h | 4 ++- sequencer.c | 3 +-- sequencer.h | 1 + 5 files changed, 53 insertions(+), 27 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 2dbf8fc08b..645ac587f7 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -13,6 +13,28 @@ static GIT_PATH_FUNC(path_state_dir, "rebase-merge/") static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto") static GIT_PATH_FUNC(path_interactive, "rebase-merge/interactive") +static int edit_todo_file(unsigned flags) +{ + const char *todo_file = rebase_path_todo(); + struct todo_list todo_list = TODO_LIST_INIT, + new_todo = TODO_LIST_INIT; + + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) + return error_errno(_("could not read '%s'."), todo_file); + + strbuf_stripspace(&todo_list.buf, 1); + if (!edit_todo_list(the_repository, &todo_list, + &new_todo, NULL, NULL, flags) && + todo_list_write_to_file(the_repository, &new_todo, todo_file, NULL, NULL, + -1, flags & ~(TODO_LIST_SHORTEN_IDS)) < 0) + return error_errno(_("could not write '%s'"), todo_file); + + todo_list_release(&todo_list); + todo_list_release(&new_todo); + + return 0; +} + static int get_revision_ranges(const char *upstream, const char *onto, const char **head_hash, char **revisions, char **shortrevisions) @@ -242,7 +264,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) break; } case EDIT_TODO: - ret = edit_todo_list(the_repository, flags); + ret = edit_todo_file(flags); break; case SHOW_CURRENT_PATCH: { struct child_process cmd = CHILD_PROCESS_INIT; diff --git a/rebase-interactive.c b/rebase-interactive.c index 807f8370db..3301efbe52 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -87,35 +87,37 @@ void append_todo_help(unsigned keep_empty, int command_count, } } -int edit_todo_list(struct repository *r, unsigned flags) +int edit_todo_list(struct repository *r, struct todo_list *todo_list, + struct todo_list *new_todo, const char *shortrevisions, + const char *shortonto, unsigned flags) { const char *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; - int res = 0; - - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) - return error_errno(_("could not read '%s'."), todo_file); - - strbuf_stripspace(&todo_list.buf, 1); - todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list); - if (todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, -1, - flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP)) { - todo_list_release(&todo_list); - return -1; + unsigned initial = shortrevisions && shortonto; + + if (initial) { + todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto, + -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP); + + if (copy_file(rebase_path_todo_backup(), todo_file, 0666)) + return error(_("could not copy '%s' to '%s'."), todo_file, +rebase_path_todo_backup()); + } else { + todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list); + todo_list_write_to_file(r, todo_list, todo_file, NULL, NULL, -1, + flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP); } - strbuf_reset(&todo_list.buf); - if (launch_sequence_editor(todo_file, &todo_list.buf, NULL)) { - todo_list_release(&todo_list); - return -1; - } + if (launch_sequence_editor(todo_file, &new_todo->buf, NULL)) + return -2; - if (!todo_list_
[PATCH v6 05/16] sequencer: introduce todo_list_write_to_file()
This introduces a new function to recreate the text of a todo list from its commands and write it to a file. This will be useful as the next few commits will change the use of the buffer in struct todo_list so it will no longer be a mirror of the file on disk. This functionality already exists in todo_list_transform(), but this function was made to replace the buffer of a todo list, which is not what we want here. Thus, the part of todo_list_transform() that replaces the buffer is dropped, and the function is renamed todo_list_to_strbuf(). It is called by todo_list_write_to_file() to fill the buffer to write to the disk. todo_list_write_to_file() can also take care of appending the help text to the buffer before writing it to the disk, or to write only the first n items of the list. This feature will be used by skip_unnecessary_picks(), which has to write done commands in a file. Signed-off-by: Alban Gruin --- sequencer.c | 61 +++-- sequencer.h | 11 ++ 2 files changed, 48 insertions(+), 24 deletions(-) diff --git a/sequencer.c b/sequencer.c index 7a295cbd3f..87b43994ff 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4507,26 +4507,28 @@ int sequencer_add_exec_commands(struct repository *r, return i; } -void todo_list_transform(struct repository *r, struct todo_list *todo_list, -unsigned flags) +static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_list, + struct strbuf *buf, int num, unsigned flags) { - struct strbuf buf = STRBUF_INIT; struct todo_item *item; - int i; + int i, max = todo_list->nr; - for (item = todo_list->items, i = 0; i < todo_list->nr; i++, item++) { + if (num > 0 && num < max) + max = num; + + for (item = todo_list->items, i = 0; i < max; i++, item++) { /* if the item is not a command write it and continue */ if (item->command >= TODO_COMMENT) { - strbuf_addf(&buf, "%.*s\n", item->arg_len, + strbuf_addf(buf, "%.*s\n", item->arg_len, todo_item_get_arg(todo_list, item)); continue; } /* add command to the buffer */ if (flags & TODO_LIST_ABBREVIATE_CMDS) - strbuf_addch(&buf, command_to_char(item->command)); + strbuf_addch(buf, command_to_char(item->command)); else - strbuf_addstr(&buf, command_to_string(item->command)); + strbuf_addstr(buf, command_to_string(item->command)); /* add commit id */ if (item->commit) { @@ -4536,28 +4538,48 @@ void todo_list_transform(struct repository *r, struct todo_list *todo_list, if (item->command == TODO_MERGE) { if (item->flags & TODO_EDIT_MERGE_MSG) - strbuf_addstr(&buf, " -c"); + strbuf_addstr(buf, " -c"); else - strbuf_addstr(&buf, " -C"); + strbuf_addstr(buf, " -C"); } - strbuf_addf(&buf, " %s", oid); + strbuf_addf(buf, " %s", oid); } /* add all the rest */ if (!item->arg_len) - strbuf_addch(&buf, '\n'); + strbuf_addch(buf, '\n'); else - strbuf_addf(&buf, " %.*s\n", item->arg_len, + strbuf_addf(buf, " %.*s\n", item->arg_len, todo_item_get_arg(todo_list, item)); } +} - strbuf_reset(&todo_list->buf); - strbuf_add(&todo_list->buf, buf.buf, buf.len); +int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list, + const char *file, const char *shortrevisions, + const char *shortonto, int num, unsigned flags) +{ + int edit_todo = !(shortrevisions && shortonto), res; + struct strbuf buf = STRBUF_INIT; + + todo_list_to_strbuf(r, todo_list, &buf, num, flags); + + if (flags & TODO_LIST_APPEND_TODO_HELP) { + int command_count = count_commands(todo_list); + if (!edit_todo) { + strbuf_addch(&buf, '\n'); + strbuf_commented_addf(&buf, Q_(&q
[PATCH v6 12/16] rebase-interactive: use todo_list_write_to_file() in edit_todo_list()
Just like complete_action(), edit_todo_list() used a function (transform_todo_file()) that read the todo list from the disk and wrote it back, resulting in useless disk accesses. This changes edit_todo_list() to call directly todo_list_write_to_file() instead. Signed-off-by: Alban Gruin --- rebase-interactive.c | 38 -- sequencer.c | 4 ++-- sequencer.h | 3 --- 3 files changed, 18 insertions(+), 27 deletions(-) diff --git a/rebase-interactive.c b/rebase-interactive.c index dfa6dd530f..d396ecc599 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -79,39 +79,33 @@ void append_todo_help(unsigned edit_todo, unsigned keep_empty, int edit_todo_list(struct repository *r, unsigned flags) { - struct strbuf buf = STRBUF_INIT; const char *todo_file = rebase_path_todo(); + struct todo_list todo_list = TODO_LIST_INIT; + int res = 0; - if (strbuf_read_file(&buf, todo_file, 0) < 0) + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) return error_errno(_("could not read '%s'."), todo_file); - strbuf_stripspace(&buf, 1); - if (write_message(buf.buf, buf.len, todo_file, 0)) { - strbuf_release(&buf); + strbuf_stripspace(&todo_list.buf, 1); + todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list); + if (todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, -1, + flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP)) { + todo_list_release(&todo_list); return -1; } - strbuf_release(&buf); - - transform_todo_file(r, flags | TODO_LIST_SHORTEN_IDS); - - if (strbuf_read_file(&buf, todo_file, 0) < 0) - return error_errno(_("could not read '%s'."), todo_file); - - append_todo_help(1, 0, &buf); - if (write_message(buf.buf, buf.len, todo_file, 0)) { - strbuf_release(&buf); + strbuf_reset(&todo_list.buf); + if (launch_sequence_editor(todo_file, &todo_list.buf, NULL)) { + todo_list_release(&todo_list); return -1; } - strbuf_release(&buf); + if (!todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) + res = todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, -1, + flags & ~(TODO_LIST_SHORTEN_IDS)); - if (launch_sequence_editor(todo_file, NULL, NULL)) - return -1; - - transform_todo_file(r, flags & ~(TODO_LIST_SHORTEN_IDS)); - - return 0; + todo_list_release(&todo_list); + return res; } define_commit_slab(commit_seen, unsigned char); diff --git a/sequencer.c b/sequencer.c index a817afffa9..d8d045067c 100644 --- a/sequencer.c +++ b/sequencer.c @@ -383,8 +383,8 @@ static void print_advice(struct repository *r, int show_hint, } } -int write_message(const void *buf, size_t len, const char *filename, - int append_eol) +static int write_message(const void *buf, size_t len, const char *filename, +int append_eol) { struct lock_file msg_file = LOCK_INIT; diff --git a/sequencer.h b/sequencer.h index 0c30e43f0a..c5bee8124c 100644 --- a/sequencer.h +++ b/sequencer.h @@ -64,9 +64,6 @@ struct replay_opts { }; #define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT } -int write_message(const void *buf, size_t len, const char *filename, - int append_eol); - /* * Note that ordering matters in this enum. Not only must it match the mapping * of todo_command_info (in sequencer.c), it is also divided into several -- 2.20.1
[PATCH v6 15/16] sequencer: use edit_todo_list() in complete_action()
This changes complete_action() to use edit_todo_list(), now that it can handle the initial edit of the todo list. Signed-off-by: Alban Gruin --- sequencer.c | 21 +++-- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/sequencer.c b/sequencer.c index 8f47f0cf39..21b04e0642 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4709,6 +4709,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla struct todo_list new_todo = TODO_LIST_INIT; struct strbuf *buf = &todo_list->buf; struct object_id oid; + int res; get_oid(onto, &oid); shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV); @@ -4733,24 +4734,16 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla return error(_("nothing to do")); } - if (todo_list_write_to_file(r, todo_list, todo_file, - shortrevisions, shortonto, -1, - flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP)) - return error_errno(_("could not write '%s'"), todo_file); - - if (copy_file(rebase_path_todo_backup(), todo_file, 0666)) - return error(_("could not copy '%s' to '%s'."), todo_file, -rebase_path_todo_backup()); - - if (launch_sequence_editor(todo_file, &new_todo.buf, NULL)) { + res = edit_todo_list(r, todo_list, &new_todo, shortrevisions, +shortonto, flags); + if (res == -1) + return -1; + else if (res == -2) { apply_autostash(opts); sequencer_remove_state(opts); return -1; - } - - strbuf_stripspace(&new_todo.buf, 1); - if (new_todo.buf.len == 0) { + } else if (res == -3) { apply_autostash(opts); sequencer_remove_state(opts); todo_list_release(&new_todo); -- 2.20.1
Re: [PATCH v6 10/16] sequencer: change complete_action() to use the refactored functions
Hi Junio, Le 29/01/2019 à 21:14, Junio C Hamano a écrit : > Alban Gruin writes: > >> if (opts->allow_ff && skip_unnecessary_picks(r, &oid)) >> return error(_("could not skip unnecessary pick commands")); >> >> if (checkout_onto(opts, onto_name, oid_to_hex(&oid), orig_head)) >> return -1; >> -; >> + >> if (require_clean_work_tree(r, "rebase", "", 1, 1)) >> return -1; > > This hunk was fishy (in my tree, there is no such line with ';' > alone, as I believe that we've already fixed it) and made me spend > some time to figure out why the patch does not apply, but once the > cause was known it was trivial to fix. > Indeed, this semicolon was removed by 29d51e214c ("sequencer.c: remove a stray semicolon", 2018-11-03). But this commit isn’t part of nd/the-index. > I've looked at the difference since the previous version, but in > essence, the only change I saw was that four instances of similar > expressions are replaced with calls to a new todo_item_get_arg() > helper and no other changes. > > Thanks, will replace. > Cheers, Alban
Re: Broken interactive rebase text after some UTF-8 characters
Hi Phillip and Michal, I think I found the bug. If you look at .git/rebase-merge/git-rebase-todo.backup, which is created before the editor is opened, you can see that it does not have this problem. In between, transform_todos() is called and causes the problem reported by Michal. This seems to be caused by a single line, sequencer:4661 (on b5101f9297, "Fourth batch after 2.20", 2019-01-29)[1]. If you add just before a something like this: fwrite(item->arg, item->arg_len, sizeof(char), stdout); You will see that the argument is properly written to stdout. But if you write this: printf("%.*s\n", item->arg_len, item->arg); You will have the same broken output as in the todo file. Are we misusing C formats? [1] https://github.com/git/git/blob/master/sequencer.c#L4661 Cheers, Alban
Re: [PATCH v6 07/16] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
Hi Phillip, Le 31/01/2019 à 15:30, Phillip Wood a écrit : > Hi Alban > > On 29/01/2019 15:01, Alban Gruin wrote: >> This refactors sequencer_add_exec_commands() to work on a todo_list to >> avoid redundant reads and writes to the disk. >> >> Instead of inserting the `exec' commands between the other commands and >> re-parsing the buffer at the end, they are appended to the buffer once, >> and a new list of items is created. Items from the old list are copied >> across and new `exec' items are appended when necessary. This >> eliminates the need to reparse the buffer, but this also means we have >> to use todo_list_write_to_disk() to write the file. >> >> todo_list_add_exec_commands() and sequencer_add_exec_commands() are >> modified to take a string list instead of a string -- one item for each >> command. This makes it easier to insert a new command to the todo list >> for each command to execute. >> >> sequencer_add_exec_commands() still reads the todo list from the disk, >> as it is needed by rebase -p. >> >> complete_action() still uses sequencer_add_exec_commands() for now. >> This will be changed in a future commit. >> >> Signed-off-by: Alban Gruin >> --- >> builtin/rebase--interactive.c | 15 +++-- >> sequencer.c | 110 +- >> sequencer.h | 5 +- >> 3 files changed, 82 insertions(+), 48 deletions(-) >> >> diff --git a/builtin/rebase--interactive.c >> b/builtin/rebase--interactive.c >> index df19ccaeb9..53056ee713 100644 >> --- a/builtin/rebase--interactive.c >> +++ b/builtin/rebase--interactive.c >> @@ -65,7 +65,7 @@ static int do_interactive_rebase(struct replay_opts >> *opts, unsigned flags, >> const char *onto, const char *onto_name, >> const char *squash_onto, const char *head_name, >> const char *restrict_revision, char *raw_strategies, >> - const char *cmd, unsigned autosquash) >> + struct string_list *commands, unsigned autosquash) >> { >> int ret; >> const char *head_hash = NULL; >> @@ -116,7 +116,7 @@ static int do_interactive_rebase(struct >> replay_opts *opts, unsigned flags, >> discard_cache(); >> ret = complete_action(the_repository, opts, flags, >> shortrevisions, onto_name, onto, >> - head_hash, cmd, autosquash); >> + head_hash, commands, autosquash); >> } >> >> free(revisions); >> @@ -139,6 +139,7 @@ int cmd_rebase__interactive(int argc, const char >> **argv, const char *prefix) >> const char *onto = NULL, *onto_name = NULL, *restrict_revision = >> NULL, >> *squash_onto = NULL, *upstream = NULL, *head_name = NULL, >> *switch_to = NULL, *cmd = NULL; >> + struct string_list commands = STRING_LIST_INIT_DUP; >> char *raw_strategies = NULL; >> enum { >> NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH, >> @@ -221,6 +222,12 @@ int cmd_rebase__interactive(int argc, const char >> **argv, const char *prefix) >> warning(_("--[no-]rebase-cousins has no effect without " >> "--rebase-merges")); >> >> + if (cmd && *cmd) { >> + string_list_split(&commands, cmd, '\n', -1); > > This whole splitting and later skipping 'exec ' is a bit of a shame - it > would be much nicer if we could just have one exec command per -x option > but I think that is outside the scope of this series (If I have time I'd > like to look at calling do_interactive_rebase() directly from > builtin/rebase.c without forking rebase--interactive). > Yes, I completely agree with you. I thought to do this in preparation to drop rebase -r. >> + if (strlen(commands.items[commands.nr - 1].string) == 0) > > I'd be tempted just to test the string using !* rather than calling > strlen. > Right. I’m still not used to this pattern. > Also is there ever a case where the last string isn't empty? I don’t think so. When rebase.c prepares the arguments for rebase--interactive, it always add a newline at the end[1]. Do you want me to drop this check? >> + --commands.nr; >> + } >> + >> switch (command) { >> case NONE: >> if (!onto && !upstream) >> @@ -228,7 +235,7 @@ int cmd_rebase__interactive(int argc, const char >> **argv, const char *prefix)
Re: Broken interactive rebase text after some UTF-8 characters
Hi Phillip, Le 31/01/2019 à 21:40, Phillip Wood a écrit : > Hi Alban > > On 31/01/2019 17:43, Alban Gruin wrote: >> Hi Phillip and Michal, >> >> I think I found the bug. > > Good find! Which os are you on? I’m on Linux, but I found this on OpenIndiana running under QEMU. -- Alban
Re: Broken interactive rebase text after some UTF-8 characters
Hi Johannes, Le 01/02/2019 à 08:38, Johannes Schindelin a écrit : > Hi, > > On Thu, 31 Jan 2019, Junio C Hamano wrote: > >> Phillip Wood writes: >> Are we misusing C formats? >>> >>> The C standard and POSIX both say that the * refers to the maximum >>> number of bytes to print but it looks like it is being treated as the >>> maximum number of characters on OpenIndiana. >>> >>> Johannes - Perhaps we should change it to use fwrite() unless printf() >>> gets fixed and we're sure no other operating systems are affected? >> >> Avoid such a rewrite, as "%*.s" that takes (int, char *) are used in >> many other places in our codebase, if you can. > > Yes, this would be painful in particular in cases like > > master:advice.c:101: fprintf(stderr, _("%shint: %.*s%s\n"), > > where we want to write more than just a variable-length buffer. > > I am curious: is libintl (gettext) used on OpenIndiana? I ask because > AFAIR fprintf() is overridden in that case, and the bug might be a lot > easier to fix if it is in libintl rather than in libc. > > Of course, it might *still* be a bug in libc by virtue of handing '%.*s' > through to libc's implementation. > > Alban, can you test this with NO_GETTEXT? Sure. :) The bug no longer happens when git is built with NO_GETTEXT. All is working as expected. > > Thanks, > Johannes > Cheers, Alban
Re: Broken interactive rebase text after some UTF-8 characters
Hi Michal, Le 01/02/2019 à 10:06, Michal Nowak a écrit : > Johannes, > > On Friday, February 1, 2019 at 8:38 AM, Johannes Schindelin > wrote: >> Hi, >> >> On Thu, 31 Jan 2019, Junio C Hamano wrote: >> >>> Phillip Wood writes: >>> > Are we misusing C formats? The C standard and POSIX both say that the * refers to the maximum number of bytes to print but it looks like it is being treated as the maximum number of characters on OpenIndiana. Johannes - Perhaps we should change it to use fwrite() unless >>> printf() gets fixed and we're sure no other operating systems are affected? >>> >>> Avoid such a rewrite, as "%*.s" that takes (int, char *) are used in >>> many other places in our codebase, if you can. >> >> Yes, this would be painful in particular in cases like >> >> master:advice.c:101: fprintf(stderr, _("%shint: %.*s%s\n"), >> >> where we want to write more than just a variable-length buffer. >> >> I am curious: is libintl (gettext) used on OpenIndiana? I ask because >> AFAIR fprintf() is overridden in that case, and the bug might be a lot >> easier to fix if it is in libintl rather than in libc. > > here you can see the full output of the OpenIndiana git build: > https://hipster.openindiana.org/logs/oi-userland/latest/git.publish.log. > > From what I see there, libintl was found. > > If you believe this is illumos libc bug, it would be cool if someone created > an simple testcase, which I can forward to the illumos developers. > I attached a test case to this email. You can build it with `gcc test-case.c', and run it with `./a.out'. Output on my Linux system: Before setting locale: Expected output: áaaa Actual output: áaaa After setting locale: Expected output: áaaa Actual output: áaaa Output on an OpenIndiana system: Before setting locale: Expected output: áaaa Actual output: áaaa After setting locale: Expected output: áaaa Actual output: á > Thanks, > Michal > >> >> Of course, it might *still* be a bug in libc by virtue of handing '%.*s' >> through to libc's implementation. >> >> Alban, can you test this with NO_GETTEXT? >> >> Thanks, >> Johannes Cheers, Alban /* * Test case for OpenIndiana '%.*s' bug * Build with `gcc test-case.c' * Run with `./a.out' */ #include #include #include #include #include static void compare_output(const char *str, int len) { puts("Expected output:"); fwrite(str, len, sizeof(char), stdout); puts("\nActual output:"); printf("%.*s\n", len, str); } int main(int argc, char **argv) { char buf[] = "áaa"; puts("Before setting locale:"); compare_output(buf, 5); setlocale(LC_ALL, ""); puts("\nAfter setting locale:"); compare_output(buf, 5); return EXIT_SUCCESS; }
Re: [PATCH v6 14/16] rebase-interactive: rewrite edit_todo_list() to handle the initial edit
Hi Phillip, Le 01/02/2019 à 12:03, Phillip Wood a écrit : >> } >> - strbuf_reset(&todo_list.buf); >> - if (launch_sequence_editor(todo_file, &todo_list.buf, NULL)) { >> - todo_list_release(&todo_list); >> - return -1; >> - } >> + if (launch_sequence_editor(todo_file, &new_todo->buf, NULL)) >> + return -2; >> - if (!todo_list_parse_insn_buffer(r, todo_list.buf.buf, >> &todo_list)) >> - res = todo_list_write_to_file(r, &todo_list, todo_file, NULL, >> NULL, -1, >> - flags & ~(TODO_LIST_SHORTEN_IDS)); >> + strbuf_stripspace(&new_todo->buf, 1); >> + if (initial && new_todo->buf.len == 0) >> + return -3; >> - todo_list_release(&todo_list); >> - return res; >> + if (!initial) >> + todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo); > > error handling. Also why don't we try parse the file for the initial > edit - is it done somewhere else? > Yes, it’s done in complete_action(). -- Alban > Best Wishes > > Phillip > >> + >> + return 0; >> } >> define_commit_slab(commit_seen, unsigned char); >> diff --git a/rebase-interactive.h b/rebase-interactive.h >> index 0e5925e3aa..44dbb06311 100644 >> --- a/rebase-interactive.h >> +++ b/rebase-interactive.h >> @@ -8,7 +8,9 @@ struct todo_list; >> void append_todo_help(unsigned keep_empty, int command_count, >> const char *shortrevisions, const char *shortonto, >> struct strbuf *buf); >> -int edit_todo_list(struct repository *r, unsigned flags); >> +int edit_todo_list(struct repository *r, struct todo_list *todo_list, >> + struct todo_list *new_todo, const char *shortrevisions, >> + const char *shortonto, unsigned flags); >> int todo_list_check(struct todo_list *old_todo, struct todo_list >> *new_todo); >> #endif >> diff --git a/sequencer.c b/sequencer.c >> index 92de982bc4..8f47f0cf39 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -55,8 +55,7 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge") >> * file and written to the tail of 'done'. >> */ >> GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo") >> -static GIT_PATH_FUNC(rebase_path_todo_backup, >> - "rebase-merge/git-rebase-todo.backup") >> +GIT_PATH_FUNC(rebase_path_todo_backup, >> "rebase-merge/git-rebase-todo.backup") >> /* >> * The rebase command lines that have already been processed. A line >> diff --git a/sequencer.h b/sequencer.h >> index c5bee8124c..68acab980b 100644 >> --- a/sequencer.h >> +++ b/sequencer.h >> @@ -10,6 +10,7 @@ struct repository; >> const char *git_path_commit_editmsg(void); >> const char *git_path_seq_dir(void); >> const char *rebase_path_todo(void); >> +const char *rebase_path_todo_backup(void); >> #define APPEND_SIGNOFF_DEDUP (1u << 0) >> >
Re: [PATCH v6 16/16] rebase--interactive: move transform_todo_file() to rebase--interactive.c
Hi Phillip, Le 01/02/2019 à 12:15, Phillip Wood a écrit : > Hi Alban > > On 29/01/2019 15:01, Alban Gruin wrote: >> As transform_todo_file() is only needed inside of rebase--interactive.c, >> it is moved there from sequencer.c. > > I think I'd prefer to minimize the code under builtin and move this to > rebase-interactive.c when it is modified earlier in the series. (I'd be > quite happy if all the files in builtin just consisted of some option > parsing followed by a call to run_git_foo() which resides in libgit) > I do agree, but transform_todo_file() is only used by rebase -p. Once it is deprecated, it’s much easier to see this function is no longer used if it’s marked as static thanks to -Werror=unused-function. Now that I think about it, check_todo_list_from_file(), rearrange_squash_in_todo_list(), and sequencer_add_exec_commands() are only used by rebase -p, but I left them in sequencer.c. > Also I wonder if we should be moving more functions (e.g. > todo_list_write_file() and possibly add_exec_commands(), > rearrange_squash() and the script generation) from sequencer.c to > rebase-interactive.c when they're rewritten (possibly in a separate > commit for ease of review) but I haven't looked if this is practical or > if there are some dependencies that make that tricky. Unless there are > some simple cases it should probably be a separate series. > It might be doable, but I think it’s a bit more difficult with sequencer_make_script() (and especially sequencer_make_script_with_merges()). I will explore this after this series. > Thanks for working on this series, it's great to see the todo list > handling becoming more efficient. > You’re welcome ;-) -- Alban > Best Wishes > > Phillip >
Re: [PATCH v6 07/16] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
Le 01/02/2019 à 15:51, Phillip Wood a écrit : >>> @@ -4473,38 +4475,62 @@ int sequencer_add_exec_commands(struct >>> repository *r, >>> * those chains if there are any. >>> */ >>> insert = -1; >>> - for (i = 0; i < todo_list.nr; i++) { >>> - enum todo_command command = todo_list.items[i].command; >>> - >>> - if (insert >= 0) { >>> - /* skip fixup/squash chains */ >>> - if (command == TODO_COMMENT) >>> - continue; >>> - else if (is_fixup(command)) { >>> - insert = i + 1; >>> - continue; >>> - } >>> - strbuf_insert(buf, >>> - todo_list.items[insert].offset_in_buf + >>> - offset, commands, commands_len); >> >> In a todo list that looks like >> pick abc message >> #pick cde empty commit >> This inserts the exec command for the first pick above the commented >> out pick. I think your translation puts it below the commented out >> pick as it ignores the value of insert. I think it's probably easiest >> to add an INSERT_ARRAY macro to insert it in the right place. An >> alternative might be to track the last insert position and only copy >> commands across when there is another exec to insert but that might >> get complicated in cases such as >> >> pick abc message >> #squash cde squash! message //empty commit for rewording >> fixup 123 fixup! message >> #pick 456 empty commit > > Thinking about this, I'm not sure it can happen as the empty squash > commit will be commented out before rearrange_squash() is called so I > think it would actually look like > > pick abc message > fixup 123 fixup! message > #pick cde squash! message > #pick 456 empty commit > > So I wonder if we can get away with treating insert as a flag and > inserting exec commands when > insert && command == TODO_COMMENT > > We could probably do with some tests for this to be sure. > I will try this approach. -- Alban > Best Wishes > > Phillip >
Re: [PATCH 2/2] mingw: enable DEP and ASLR
Hi Johannes, Le 01/05/2019 à 00:41, Johannes Schindelin a écrit : > Hi Hannes, > > On Tue, 30 Apr 2019, Johannes Sixt wrote: > >> [had to add Dscho as recipient manually, mind you] > > I usually pick up responses to GitGitGadget patch series even if I am not > on explicit Cc: (but it might take a couple of days when I am too busy > elsewhere to read the Git mailing list). > >> Am 29.04.19 um 23:56 schrieb İsmail Dönmez via GitGitGadget: >>> From: =?UTF-8?q?=C4=B0smail=20D=C3=B6nmez?= >>> >>> Enable DEP (Data Execution Prevention) and ASLR (Address Space Layout >>> Randomization) support. This applies to both 32bit and 64bit builds >>> and makes it substantially harder to exploit security holes in Git by >>> offering a much more unpredictable attack surface. >>> >>> ASLR interferes with GDB's ability to set breakpoints. A similar issue >>> holds true when compiling with -O2 (in which case single-stepping is >>> messed up because GDB cannot map the code back to the original source >>> code properly). Therefore we simply enable ASLR only when an I don’t know if it stands true when combined with something like -ggdb3, but I may be very wrong. Feel free to correct me. >>> optimization flag is present in the CFLAGS, using it as an indicator >>> that the developer does not want to debug in GDB anyway. >>> >>> Signed-off-by: İsmail Dönmez >>> Signed-off-by: Johannes Schindelin >>> --- >>> config.mak.uname | 6 ++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/config.mak.uname b/config.mak.uname >>> index e7c7d14e5f..a9edcc5f0b 100644 >>> --- a/config.mak.uname >>> +++ b/config.mak.uname >>> @@ -570,6 +570,12 @@ else >>> ifeq ($(shell expr "$(uname_R)" : '2\.'),2) >>> # MSys2 >>> prefix = /usr/ >>> + # Enable DEP >>> + BASIC_LDFLAGS += -Wl,--nxcompat >>> + # Enable ASLR (unless debugging) >>> + ifneq (,$(findstring -O,$(CFLAGS))) >>> + BASIC_LDFLAGS += -Wl,--dynamicbase >>> + endif >>> ifeq (MINGW32,$(MSYSTEM)) >>> prefix = /mingw32 >>> HOST_CPU = i686 >>> >> >> I'm a bit concerned that this breaks my debug sessions where I use -O0. >> But I'll test without -O0 before I really complain. > > Weird. Jameson Miller also mentioned this very concern in an internal > review. > > I guess I'll do something like > > ifneq (,$(findstring -O,$(filter-out -O0,$(CFLAGS > -Og also exists to debug[0], even if it’s far less known. Perhaps it’s better to check for -g (and its variants[1]) as the user clearly states their intent to debug the resulting binary, rather than checking for special cases. > Does that work for you? > > Ciao, > Dscho > [0] https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-Og [1] https://gcc.gnu.org/onlinedocs/gcc/Debugging-Options.html Cheers, Alban
Why is there still git-rebase--am.sh?
Hi, I was wandering in rebase’s source code, and found out that even though run_specific_rebase() no longer calls git-rebase--am.sh since 21853626ea ("built-in rebase: call `git am` directly", 2019-01-18), this commit did not remove the code to call it. I guess it was an oversight. Now that git-legacy-rebase.sh is gone (since d03ebd411c, "rebase: remove the rebase.useBuiltin setting", 2019-03-18), I would expect this file to be completely removed, too. May I send a patch to remove git-rebase--am.sh, or is there a reason to keep it? Cheers, Alban
[RFC PATCH 0/9] rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co.
To prevent mistakes when editing a branch, rebase features a knob, rebase.missingCommitsCheck, to warn the user if a commit was dropped. Unfortunately, this check is only effective for the initial edit, which means that if you edit the todo list at a later point of the rebase and dropped a commit, no warnings or errors would be issued. This adds the ability to check if commits were dropped when resuming a rebase (with `rebase --continue'), when editing the todo list (with `rebase --edit-todo'), or when reloading the todo list after an `exec' command. The idea to extend this feature was suggested to me more than a year ago by Phillip Wood, if I'm not mistaken. I postponed this until four month ago, when ag/sequencer-reduce-rewriting-todo finally hit master, but I had to stop because of other obligations. I could go back to work one month ago, when I did the bulk of this series, but I lacked time to polish it, so it waited a bit more. Now, I think it is in a good shape to be sent, although it is still RFC-quality to me. The advertised functionality should work well, but perhaps there is some flaws I missed. The first two patches are new tests, demonstrating that after the initial edit, the check is not done. The next four are what could be qualified as omissions from ag/sequencer-reduce-rewriting-todo, but they are quite important (IMHO) for the rest of the series. The last three actually extend rebase.missingCommitsCheck. This is based on master (9d418600f4, "The fifth batch"). The tip of this series is tagged as "edit-todo-drop-rfc" in https://github.com/agrn/git. Alban Gruin (9): t3404: demonstrate that --edit-todo does not check for dropped commits t3429: demonstrate that rebase exec does not check for dropped commits sequencer: update `total_nr' when adding an item to a todo list sequencer: update `done_nr' when skipping commands in a todo list sequencer: move the code writing total_nr on the disk to a new function sequencer: add a parameter to sequencer_continue() to accept a todo list rebase-interactive: todo_list_check() also uses the done list rebase-interactive: warn if commit is dropped with --edit-todo sequencer: have read_populate_todo() check for dropped commits builtin/rebase.c | 2 +- builtin/revert.c | 2 +- rebase-interactive.c | 67 +++- rebase-interactive.h | 6 ++- sequencer.c | 53 ++ sequencer.h | 3 +- t/t3404-rebase-interactive.sh | 82 +++ t/t3429-rebase-edit-todo.sh | 44 ++- 8 files changed, 224 insertions(+), 35 deletions(-) -- 2.22.0
[RFC PATCH 1/9] t3404: demonstrate that --edit-todo does not check for dropped commits
When set to "warn" or "error", `rebase.missingCommitCheck' would make rebase -i warn if the user removed commits from the todo list to prevent mistakes. Unfortunately, rebase --edit-todo and rebase --continue don't take it into account. This adds three tests to t3404 to demonstrate this. The first one is not broken, as when `rebase.missingCommitsCheck' is not set, nothing in particular must be done towards dropped commits. The two others are broken, demonstrating the problem. Signed-off-by: Alban Gruin --- t/t3404-rebase-interactive.sh | 82 +++ 1 file changed, 82 insertions(+) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 461dd539ff..f5c0a8d2bb 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1345,6 +1345,88 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' ' test B = $(git cat-file commit HEAD^ | sed -ne \$p) ' +test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = ignore' ' + test_config rebase.missingCommitsCheck ignore && + rebase_setup_and_clean missing-commit && + set_fake_editor && + test_must_fail env FAKE_LINES="1 2 bad 3 4" \ + git rebase -i --root >/dev/null 2>stderr && + FAKE_LINES="1 2 4" git rebase --edit-todo && + git rebase --continue 2>actual && + test D = $(git cat-file commit HEAD | sed -ne \$p) && + test_i18ngrep \ + "Successfully rebased and updated refs/heads/missing-commit" \ + actual +' + +cat >expect <expect.2 + +test_expect_failure 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' ' + test_config rebase.missingCommitsCheck warn && + rebase_setup_and_clean missing-commit && + set_fake_editor && + test_must_fail env FAKE_LINES="1 2 3 4 bad 5" \ + git rebase -i --root >/dev/null 2>stderr && + FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual && + test_i18ncmp expect actual && + git rebase --continue 2>actual.2 && + head -n 8 actual && + test_i18ncmp expect.2 actual && + test D = $(git cat-file commit HEAD | sed -ne \$p) && + test_i18ngrep \ + "Successfully rebased and updated refs/heads/missing-commit" \ + actual.2 +' + +cat >expect <expect.2 + +test_expect_failure 'rebase --edit-todo respects rebase.missingCommitsCheck = error' ' + test_config rebase.missingCommitsCheck error && + rebase_setup_and_clean missing-commit && + set_fake_editor && + test_must_fail env FAKE_LINES="1 2 bad 3 4" \ + git rebase -i --root >/dev/null 2>stderr && + test_must_fail env FAKE_LINES="1 2 4" \ + git rebase --edit-todo 2>actual && + test_i18ncmp expect actual && + test_must_fail git rebase --continue 2>actual && + test_i18ncmp expect.2 actual && + cp .git/rebase-merge/git-rebase-todo.backup \ + .git/rebase-merge/git-rebase-todo && + FAKE_LINES="1 2 drop 3 4 drop 5" \ + git rebase --edit-todo && + git rebase --continue 2>actual && + test D = $(git cat-file commit HEAD | sed -ne \$p) && + test_i18ngrep \ + "Successfully rebased and updated refs/heads/missing-commit" \ + actual +' + test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' ' rebase_setup_and_clean abbrevcmd && test_commit "first" file1.txt "first line" first && -- 2.22.0
[RFC PATCH 2/9] t3429: demonstrate that rebase exec does not check for dropped commits
After executing a command, rebase reloads the todo list from the disk, in case the script has modified it, but does not honour rebase.missingCommitsCheck. This adds three tests to t3429 to demonstrate this. The first one is not broken, as when `rebase.missingCommitsCheck' is not set, nothing should be done for dropped commits. The last two are, demonstrating the problem. Signed-off-by: Alban Gruin --- t/t3429-rebase-edit-todo.sh | 44 +++-- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh index 76f6d306ea..2bb9fb65fa 100755 --- a/t/t3429-rebase-edit-todo.sh +++ b/t/t3429-rebase-edit-todo.sh @@ -4,15 +4,16 @@ test_description='rebase should reread the todo file if an exec modifies it' . ./test-lib.sh +todo=.git/rebase-merge/git-rebase-todo + test_expect_success 'rebase exec modifies rebase-todo' ' test_commit initial && - todo=.git/rebase-merge/git-rebase-todo && git rebase HEAD -x "echo exec touch F >>$todo" && test -e F ' test_expect_success SHA1 'loose object cache vs re-reading todo list' ' - GIT_REBASE_TODO=.git/rebase-merge/git-rebase-todo && + GIT_REBASE_TODO=$todo && export GIT_REBASE_TODO && write_script append-todo.sh <<-\EOS && # For values 5 and 6, this yields SHA-1s with the same first two digits @@ -33,4 +34,43 @@ test_expect_success SHA1 'loose object cache vs re-reading todo list' ' git rebase HEAD -x "./append-todo.sh 5 6" ' +test_expect_success 'rebase exec respects rebase.missingCommitsCheck = ignore' ' + test_config rebase.missingCommitsCheck ignore && + git rebase HEAD~2 --keep-empty -x "echo >$todo" && + test 5 = $(git cat-file commit HEAD | sed -ne \$p) +' + +cat >expect <$todo" 2>actual.2 && + head -n8 actual.2 | tail -n7 >actual && + test_i18ncmp expect actual && + test 5 = $(git cat-file commit HEAD | sed -ne \$p) +' + +test_expect_failure 'rebase exec respects rebase.missingCommitsCheck = error' ' + test_config rebase.missingCommitsCheck error && + git reset --hard HEAD@{2} && + test_must_fail git rebase HEAD~2 --keep-empty -x "echo >$todo" 2>actual.2 && + head -n8 actual.2 | tail -n7 >actual && + test_i18ncmp expect actual && + echo drop $(git rev-list --pretty=oneline -1 HEAD@{1}) >$todo && + git rebase --continue 2>actual && + test 5 = $(git cat-file commit HEAD | sed -ne \$p) && + test_i18ngrep \ + "Successfully rebased and updated refs/heads/master" \ + actual +' + test_done -- 2.22.0
[RFC PATCH 3/9] sequencer: update `total_nr' when adding an item to a todo list
`total_nr' is the total amount of items, done and toto, that are in a todo list. But unlike `nr', it was not updated when an item was appended to the list. Signed-off-by: Alban Gruin --- sequencer.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sequencer.c b/sequencer.c index cf262701e8..e61ae75451 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2070,6 +2070,7 @@ void todo_list_release(struct todo_list *todo_list) static struct todo_item *append_new_todo(struct todo_list *todo_list) { ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc); + ++todo_list->total_nr; return todo_list->items + todo_list->nr++; } -- 2.22.0
[RFC PATCH 4/9] sequencer: update `done_nr' when skipping commands in a todo list
In a todo list, `done_nr' is the amount of commands that were executed or skipped, but skip_unnecessary_picks() did not update it. Signed-off-by: Alban Gruin --- sequencer.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sequencer.c b/sequencer.c index e61ae75451..ec9c3d4dc5 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4939,6 +4939,7 @@ static int skip_unnecessary_picks(struct repository *r, MOVE_ARRAY(todo_list->items, todo_list->items + i, todo_list->nr - i); todo_list->nr -= i; todo_list->current = 0; + todo_list->done_nr += i; if (is_fixup(peek_command(todo_list, 0))) record_in_rewritten(base_oid, peek_command(todo_list, 0)); -- 2.22.0
[RFC PATCH 6/9] sequencer: add a parameter to sequencer_continue() to accept a todo list
As it is called by sequencer_continue() or after an exec command, read_populate_todo() is a great place to check for dropped commits in the todo list, but complete_action() (a caller of sequencer_continue()) already does. Double-checking would show the message twice. This adds a parameter to sequencer_continue() to accept a todo list. If a valid list is provided, read_populate_todo() won't be called. complete_action() is modified to pass its todo list to sequencer_continue(). This also avoids reloading the todo list from the disk just after releasing it. Signed-off-by: Alban Gruin --- builtin/rebase.c | 2 +- builtin/revert.c | 2 +- sequencer.c | 26 +++--- sequencer.h | 3 ++- 4 files changed, 19 insertions(+), 14 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 89fc4b8153..205916ca11 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -393,7 +393,7 @@ static int run_rebase_interactive(struct rebase_options *opts, case ACTION_CONTINUE: { struct replay_opts replay_opts = get_replay_opts(opts); - ret = sequencer_continue(the_repository, &replay_opts); + ret = sequencer_continue(the_repository, &replay_opts, NULL); break; } case ACTION_EDIT_TODO: diff --git a/builtin/revert.c b/builtin/revert.c index 4e71b2f2aa..45a5c6217d 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -207,7 +207,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) return ret; } if (cmd == 'c') - return sequencer_continue(the_repository, opts); + return sequencer_continue(the_repository, opts, NULL); if (cmd == 'a') return sequencer_rollback(the_repository, opts); return sequencer_pick_revisions(the_repository, opts); diff --git a/sequencer.c b/sequencer.c index d66b80d49f..3fb15ff8d9 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4147,9 +4147,11 @@ static int commit_staged_changes(struct repository *r, return 0; } -int sequencer_continue(struct repository *r, struct replay_opts *opts) +int sequencer_continue(struct repository *r, struct replay_opts *opts, + struct todo_list *p_todo_list) { - struct todo_list todo_list = TODO_LIST_INIT; + struct todo_list todo_list = TODO_LIST_INIT, + *ptr_todo = (p_todo_list) ? p_todo_list : &todo_list; int res; if (read_and_refresh_cache(r, opts)) @@ -4158,13 +4160,13 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts) if (read_populate_opts(opts)) return -1; if (is_rebase_i(opts)) { - if ((res = read_populate_todo(r, &todo_list, opts))) + if (!p_todo_list && (res = read_populate_todo(r, &todo_list, opts))) goto release_todo_list; - if (commit_staged_changes(r, opts, &todo_list)) + if (commit_staged_changes(r, opts, ptr_todo)) return -1; } else if (!file_exists(get_todo_path(opts))) return continue_single_pick(r); - else if ((res = read_populate_todo(r, &todo_list, opts))) + else if (!p_todo_list && (res = read_populate_todo(r, &todo_list, opts))) goto release_todo_list; if (!is_rebase_i(opts)) { @@ -4179,18 +4181,18 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts) res = error_dirty_index(r, opts); goto release_todo_list; } - todo_list.current++; + ptr_todo->current++; } else if (file_exists(rebase_path_stopped_sha())) { struct strbuf buf = STRBUF_INIT; struct object_id oid; if (read_oneliner(&buf, rebase_path_stopped_sha(), 1) && !get_oid_committish(buf.buf, &oid)) - record_in_rewritten(&oid, peek_command(&todo_list, 0)); + record_in_rewritten(&oid, peek_command(ptr_todo, 0)); strbuf_release(&buf); } - res = pick_commits(r, &todo_list, opts); + res = pick_commits(r, ptr_todo, opts); release_todo_list: todo_list_release(&todo_list); return res; @@ -5025,15 +5027,17 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla return error_errno(_("could not write '%s'"), todo_file); } - todo_list_release(&new_todo); - if (checkout_onto(r, opts, onto_name, &oid, orig_head)) return -1; if (require_clean_work_tree(r, "rebase", "", 1, 1)) return -1; - return
[RFC PATCH 5/9] sequencer: move the code writing total_nr on the disk to a new function
The total amount of commands can be used to show the progression of the rebasing in a shell. This number is written to the disk by read_populate_todo() when the todo list is loaded from sequencer_continue() or pick_commits(), but not by complete_action(), which releases its todo list before calling sequencer_continue(), which reloads it immediatly afterwards. To avoid to reload the todo list from the disk, sequencer_continue() will be modified to accept a todo list, and if it is not null, read_populate_todo() will not be called. This moves the part writing total_nr to a new function so it can be called by complete_action(). Signed-off-by: Alban Gruin --- sequencer.c | 16 +++- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/sequencer.c b/sequencer.c index ec9c3d4dc5..d66b80d49f 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2343,6 +2343,16 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose) sequencer_remove_state(&opts); } +static void todo_list_write_total_nr(struct todo_list *todo_list) +{ + FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w"); + + if (f) { + fprintf(f, "%d\n", todo_list->total_nr); + fclose(f); + } +} + static int read_populate_todo(struct repository *r, struct todo_list *todo_list, struct replay_opts *opts) @@ -2388,7 +2398,6 @@ static int read_populate_todo(struct repository *r, if (is_rebase_i(opts)) { struct todo_list done = TODO_LIST_INIT; - FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w"); if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 && !todo_list_parse_insn_buffer(r, done.buf.buf, &done)) @@ -2400,10 +2409,7 @@ static int read_populate_todo(struct repository *r, + count_commands(todo_list); todo_list_release(&done); - if (f) { - fprintf(f, "%d\n", todo_list->total_nr); - fclose(f); - } + todo_list_write_total_nr(todo_list); } return 0; -- 2.22.0
[RFC PATCH 9/9] sequencer: have read_populate_todo() check for dropped commits
This adds the ability to check if commits were dropped when resuming a rebase (with `--continue') or when reloading the todo list after an `exec' command. Tests added previously should work now. Signed-off-by: Alban Gruin --- sequencer.c | 5 - t/t3404-rebase-interactive.sh | 4 ++-- t/t3429-rebase-edit-todo.sh | 4 ++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/sequencer.c b/sequencer.c index 0638c92f12..d2c4459e7c 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2360,7 +2360,6 @@ static int read_populate_todo(struct repository *r, struct stat st; const char *todo_file = get_todo_path(opts); int res; - strbuf_reset(&todo_list->buf); if (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0) return -1; @@ -2378,6 +2377,10 @@ static int read_populate_todo(struct repository *r, return error(_("unusable instruction sheet: '%s'"), todo_file); } + res = todo_list_check_against_backup(r, todo_list); + if (res) + return -1; + if (!todo_list->nr && (!is_rebase_i(opts) || !file_exists(rebase_path_done( return error(_("no commits parsed.")); diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index f5c0a8d2bb..090a496bcc 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1373,7 +1373,7 @@ EOF tail -n 8 expect.2 -test_expect_failure 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' ' +test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' ' test_config rebase.missingCommitsCheck warn && rebase_setup_and_clean missing-commit && set_fake_editor && @@ -1405,7 +1405,7 @@ EOF tail -n 9 expect.2 -test_expect_failure 'rebase --edit-todo respects rebase.missingCommitsCheck = error' ' +test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = error' ' test_config rebase.missingCommitsCheck error && rebase_setup_and_clean missing-commit && set_fake_editor && diff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh index 2bb9fb65fa..79cd5657b3 100755 --- a/t/t3429-rebase-edit-todo.sh +++ b/t/t3429-rebase-edit-todo.sh @@ -50,7 +50,7 @@ Use 'git config rebase.missingCommitsCheck' to change the level of warnings. The possible behaviours are: ignore, warn, error. EOF -test_expect_failure 'rebase exec respects rebase.missingCommitsCheck = warn' ' +test_expect_success 'rebase exec respects rebase.missingCommitsCheck = warn' ' test_config rebase.missingCommitsCheck warn && git reset --hard HEAD@{2} && git rebase HEAD~2 --keep-empty -x "echo >$todo" 2>actual.2 && @@ -59,7 +59,7 @@ test_expect_failure 'rebase exec respects rebase.missingCommitsCheck = warn' ' test 5 = $(git cat-file commit HEAD | sed -ne \$p) ' -test_expect_failure 'rebase exec respects rebase.missingCommitsCheck = error' ' +test_expect_success 'rebase exec respects rebase.missingCommitsCheck = error' ' test_config rebase.missingCommitsCheck error && git reset --hard HEAD@{2} && test_must_fail git rebase HEAD~2 --keep-empty -x "echo >$todo" 2>actual.2 && -- 2.22.0
[RFC PATCH 7/9] rebase-interactive: todo_list_check() also uses the done list
todo_list_check() works by checking if every commit in old_todo (the backup list) is also present in new_todo (the todo list to check). This works only when no commits have been picked (ie. right after the initial edit). In other cases, the backup list will contain one or several commits that have been applied and are no longer in the todo list. Let's take this todo list, for instance: pick 123abc An interesting change break pick cba321 A boring change After executing the `break' command, the backup list will contain the original todo list, while the todo list will only contain the last `pick' insn. Even if commit 123abc has been picked up, in this case, todo_list_check() will fail because it is not part of the todo list. There is another list containing every command that have been executed: the "done" list. In our example, after the `break' command, the "done" list will contain the first commit (123abc). Comparing the backup list against the todo list and the done list will fix this problem. This changes todo_list_check() to compare old_todo against new_todo _and_ the done list. This should be useful to check the todo list when resuming a rebase or reloading the todo list after an `exec' command, but not for `--edit-todo' (the todo list is copied before edition). But, for the sake of safety, it is still done in the next commit. This also adds a function, todo_list_check_against_backup(), to load the done list and the backup list, and call todo_list_check(). Signed-off-by: Alban Gruin --- rebase-interactive.c | 57 ++-- rebase-interactive.h | 6 - sequencer.c | 4 ++-- 3 files changed, 57 insertions(+), 10 deletions(-) diff --git a/rebase-interactive.c b/rebase-interactive.c index aa18ae82b7..c7dea85553 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -6,6 +6,8 @@ #include "commit-slab.h" #include "config.h" +static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done") + enum missing_commit_check_level { MISSING_COMMIT_CHECK_IGNORE = 0, MISSING_COMMIT_CHECK_WARN, @@ -124,13 +126,27 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list, } define_commit_slab(commit_seen, unsigned char); + +static void mark_commits_as_seen(struct commit_seen *commit_seen, +struct todo_list *list) +{ + int i; + + for (i = 0; i < list->nr; i++) { + struct commit *commit = list->items[i].commit; + if (commit) + *commit_seen_at(commit_seen, commit) = 1; + } +} + /* * Check if the user dropped some commits by mistake * Behaviour determined by rebase.missingCommitsCheck. * Check if there is an unrecognized command or a * bad SHA-1 in a command. */ -int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo) +int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo, + struct todo_list *done) { enum missing_commit_check_level check_level = get_missing_commit_check_level(); struct strbuf missing = STRBUF_INIT; @@ -142,12 +158,11 @@ int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo) if (check_level == MISSING_COMMIT_CHECK_IGNORE) goto leave_check; - /* Mark the commits in git-rebase-todo as seen */ - for (i = 0; i < new_todo->nr; i++) { - struct commit *commit = new_todo->items[i].commit; - if (commit) - *commit_seen_at(&commit_seen, commit) = 1; - } + /* Mark the commits in git-rebase-todo and git-rebase-done as + seen */ + mark_commits_as_seen(&commit_seen, new_todo); + if (done) + mark_commits_as_seen(&commit_seen, done); /* Find commits in git-rebase-todo.backup yet unseen */ for (i = old_todo->nr - 1; i >= 0; i--) { @@ -187,3 +202,31 @@ int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo) clear_commit_seen(&commit_seen); return res; } + +int todo_list_check_against_backup(struct repository *r, + struct todo_list *todo_list) +{ + struct todo_list done = TODO_LIST_INIT, initial = TODO_LIST_INIT; + int res; + + if (strbuf_read_file(&done.buf, rebase_path_done(), 0) < 0 && errno != ENOENT) + return error(_("could not read '%s'"), rebase_path_done()); + + strbuf_read_file(&done.buf, rebase_path_done(), 0); + todo_list_parse_insn_buffer(r, done.buf.buf, &done); + + if (strbuf_read_file(&initial.buf, rebase_path_todo_backup(), 0) < 0 && + errno != ENOENT) { + todo_list_release(&done); +
[RFC PATCH 8/9] rebase-interactive: warn if commit is dropped with --edit-todo
This adds the ability for --edit-todo to check if commits were dropped by the user using todo_list_check_against_backup(). Signed-off-by: Alban Gruin --- rebase-interactive.c | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/rebase-interactive.c b/rebase-interactive.c index c7dea85553..c346d4ced4 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -95,6 +95,7 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list, { const char *todo_file = rebase_path_todo(); unsigned initial = shortrevisions && shortonto; + int res = 0; /* If the user is editing the todo list, we first try to parse * it. If there is an error, we do not return, because the user @@ -119,10 +120,13 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list, /* For the initial edit, the todo list gets parsed in * complete_action(). */ - if (!initial) - return todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo); + if (!initial) { + res = todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo); + if (!res) + res = todo_list_check_against_backup(r, new_todo); + } - return 0; + return res; } define_commit_slab(commit_seen, unsigned char); -- 2.22.0
Re: [RFC PATCH 1/9] t3404: demonstrate that --edit-todo does not check for dropped commits
Hi Junio, Le 18/07/2019 à 20:31, Junio C Hamano a écrit : > Alban Gruin writes: > > When set to "warn" or "error", `rebase.missingCommitCheck' would make > > rebase -i warn if the user removed commits from the todo list to prevent > > mistakes. Unfortunately, rebase --edit-todo and rebase --continue don't > > take it into account. > > > > This adds three tests to t3404 to demonstrate this. The first one is > > not broken, as when `rebase.missingCommitsCheck' is not set, nothing in > > particular must be done towards dropped commits. The two others are > > broken, demonstrating the problem. > > > > Signed-off-by: Alban Gruin > > --- > > > > t/t3404-rebase-interactive.sh | 82 +++ > > 1 file changed, 82 insertions(+) > > > > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > > index 461dd539ff..f5c0a8d2bb 100755 > > --- a/t/t3404-rebase-interactive.sh > > +++ b/t/t3404-rebase-interactive.sh > > @@ -1345,6 +1345,88 @@ test_expect_success 'rebase -i respects > > rebase.missingCommitsCheck = error' '> > > test B = $(git cat-file commit HEAD^ | sed -ne \$p) > > > > ' > > > > +test_expect_success 'rebase --edit-todo respects > > rebase.missingCommitsCheck = ignore' ' +test_config > > rebase.missingCommitsCheck ignore && > > + rebase_setup_and_clean missing-commit && > > + set_fake_editor && > > + test_must_fail env FAKE_LINES="1 2 bad 3 4" \ > > + git rebase -i --root >/dev/null 2>stderr && > > Do you need to capture into stderr? Nobody seems to use it. > No. I’m changing this. > > + FAKE_LINES="1 2 4" git rebase --edit-todo && > > + git rebase --continue 2>actual && > > + test D = $(git cat-file commit HEAD | sed -ne \$p) && > > + test_i18ngrep \ > > + "Successfully rebased and updated refs/heads/missing- commit" \ > > + actual > > +' > > + > > +cat >expect < > +error: invalid line 5: badcmd $(git rev-list --pretty=oneline > > --abbrev-commit -1 master) +Warning: some commits may have been dropped > > accidentally. > > +Dropped commits (newer to older): > > + - $(git rev-list --pretty=oneline --abbrev-commit -1 master) > > +To avoid this message, use "drop" to explicitly remove a commit. > > + > > +Use 'git config rebase.missingCommitsCheck' to change the level of > > warnings. +The possible behaviours are: ignore, warn, error. > > + > > +EOF > > + > > +tail -n 8 expect.2 > > Having this outside the test_expect_success block that uses the file > is bad. You may have mimicked other tests in the same script, but > that is not a good excuse to make things worse. Just make sure > these new stuff follow the best-current-practice pattern without > touching the existing bad examples (and later fix them up after the > dust settles, but don't let it distract you from the theme these > patches are addressing). > Okay. > > + > > +test_expect_failure 'rebase --edit-todo respects > > rebase.missingCommitsCheck = warn' ' + test_config > > rebase.missingCommitsCheck warn && > > + rebase_setup_and_clean missing-commit && > > + set_fake_editor && > > + test_must_fail env FAKE_LINES="1 2 3 4 bad 5" \ > > + git rebase -i --root >/dev/null 2>stderr && > > Ditto. > > > + FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual && > > + test_i18ncmp expect actual && > > So, after "--edit-todo", you are supposed to get an error and a warning, > but ... > > > + git rebase --continue 2>actual.2 && > > + head -n 8 actual && > > + test_i18ncmp expect.2 actual && > > ... after "--continue", you do not get any error, as you removed > 'bad' from the input, but you still get a warning, followed by a > report of the fact that a commit has been dropped. OK. > > > + test D = $(git cat-file commit HEAD | sed -ne \$p) && > > + test_i18ngrep \ > > + "Successfully rebased and updated refs/heads/missing- commit" \ > > + actual.2 > > +' > > + > > +cat >expect < > +error: invalid line 3: badcmd $(git rev-list --pretty=oneline > > --abbrev-commit -1 master~2)
Re: [RFC PATCH 3/9] sequencer: update `total_nr' when adding an item to a todo list
Hi, Le 18/07/2019 à 21:52, Junio C Hamano a écrit : > Alban Gruin writes: > > `total_nr' is the total amount of items, done and toto, that are in a > > "amount" -> "number" perhaps. Also s/toto/todo/ perhaps but I am > not sure what you wanted to say here, so... > `total_nr' is the number of commands, whether they have been executed or not. It’s used to show the progression of the rebase. > > todo list. But unlike `nr', it was not updated when an item was > > appended to the list. > > Good finding. > > > Signed-off-by: Alban Gruin > > --- > > > > sequencer.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/sequencer.c b/sequencer.c > > index cf262701e8..e61ae75451 100644 > > --- a/sequencer.c > > +++ b/sequencer.c > > @@ -2070,6 +2070,7 @@ void todo_list_release(struct todo_list *todo_list) > > > > static struct todo_item *append_new_todo(struct todo_list *todo_list) > > { > > > > ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc); > > > > + ++todo_list->total_nr; > > When we do not use the value in an expression, we prefer post > increment, not pre increment. > > > return todo_list->items + todo_list->nr++; > > > > }
Re: [RFC PATCH 4/9] sequencer: update `done_nr' when skipping commands in a todo list
Hi, Le 18/07/2019 à 21:55, Junio C Hamano a écrit : > Alban Gruin writes: > > In a todo list, `done_nr' is the amount of commands that were executed > > or skipped, but skip_unnecessary_picks() did not update it. > > OK. Together with 3/9 and this one, any increment of total_nr and > done_nr in the existing code is not removed; does it mean that > nobody actually cares what these fields contain? IOW, there is no > code that says "if (list->total_nr <= i) { we are done; }" etc.? > > Or are these fields used later, but somehow the lack of increment in > the places touched by 3/9 and 4/9 is compensated? > `total_nr' is not used for this, because it’s not necessarily the number of items in the todo list. That’s the role of `nr'. So the comparison is more like "if (list->nr <= i) { we are done; }". Same think for `done_nr'. Each time a command is executed, git prints "Rebasing ($done_nr/$total_nr)". These two variables are written to the disk, and might be used by a shell prompt (eg. git-prompt.sh, oh my zsh…) And this is actually how I found this. Originally, I wrote what became 5/9 and 6/9, without touching to `done_nr' and `total_nr'. All rebase tests (t34??*) passed, but t9903.15 ("prompt - rebase merge") failed, because the value was incorrect. The reason is that, before I changed sequencer_continue() in 6/9, it called another function, read_populate_todo(), which would recompute `done_nr' and `total_nr', then write them to the disk. With my changes, these values would not have been updated after adding `exec' commands or skipping picks in complete_action(), so the numbers written to the disk were incorrect. tl;dr: it does not impact how the rebase works, but it might impact the messages printed while rebasing or shell prompts. > > Signed-off-by: Alban Gruin > > --- > > > > sequencer.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/sequencer.c b/sequencer.c > > index e61ae75451..ec9c3d4dc5 100644 > > --- a/sequencer.c > > +++ b/sequencer.c > > @@ -4939,6 +4939,7 @@ static int skip_unnecessary_picks(struct repository > > *r,> > > MOVE_ARRAY(todo_list->items, todo_list->items + i, todo_list->nr - i); > > todo_list->nr -= i; > > todo_list->current = 0; > > > > + todo_list->done_nr += i; > > > > if (is_fixup(peek_command(todo_list, 0))) > > > > record_in_rewritten(base_oid, peek_command(todo_list, 0));
Re: [RFC PATCH 5/9] sequencer: move the code writing total_nr on the disk to a new function
Hi, Le 18/07/2019 à 22:04, Junio C Hamano a écrit : > Alban Gruin writes: > > The total amount of commands can be used to show the progression of the > > rebasing in a shell. This number is written to the disk by > > read_populate_todo() when the todo list is loaded from > > sequencer_continue() or pick_commits(), but not by complete_action(), > > which releases its todo list before calling sequencer_continue(), which > > reloads it immediatly afterwards. > > > > To avoid to reload the todo list from the disk, sequencer_continue() > > will be modified to accept a todo list, and if it is not null, > > read_populate_todo() will not be called. > > That may be good as a plan, but readers who are reading this step > are left puzzled as the changes so far do not seem to have much to > do with that change. Perhaps this paragraph belongs to the step > that actually makes that modification? > > > This moves the part writing total_nr to a new function so it can be > > called by complete_action(). > > So without 3/9 and 4/9, we have been simply writing out wrong > number? Good. > Not here, the numbers computed in read_populate_todo() are correct. But they may be incorrect in complete_action(), before calling sequencer_continue(). This was not a big deal as the todo list was released before and sequencer_continue() re-read it, but it would have became a problem with 6/9. > > Signed-off-by: Alban Gruin > > --- > > > > sequencer.c | 16 +++- > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/sequencer.c b/sequencer.c > > index ec9c3d4dc5..d66b80d49f 100644 > > --- a/sequencer.c > > +++ b/sequencer.c > > @@ -2343,6 +2343,16 @@ void sequencer_post_commit_cleanup(struct > > repository *r, int verbose)> > > sequencer_remove_state(&opts); > > > > } > > > > +static void todo_list_write_total_nr(struct todo_list *todo_list) > > +{ > > + FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w"); > > + > > + if (f) { > > + fprintf(f, "%d\n", todo_list->total_nr); > > + fclose(f); > > + } > > +} > > + > > > > static int read_populate_todo(struct repository *r, > > > > struct todo_list *todo_list, > > struct replay_opts *opts) > > > > @@ -2388,7 +2398,6 @@ static int read_populate_todo(struct repository *r, > > > > if (is_rebase_i(opts)) { > > > > struct todo_list done = TODO_LIST_INIT; > > > > - FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w"); > > > > if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 && > > > > !todo_list_parse_insn_buffer(r, done.buf.buf, &done)) > > > > @@ -2400,10 +2409,7 @@ static int read_populate_todo(struct repository *r, > > > > + count_commands(todo_list); > > > > todo_list_release(&done); > > > > - if (f) { > > - fprintf(f, "%d\n", todo_list->total_nr); > > - fclose(f); > > - } > > + todo_list_write_total_nr(todo_list); > > > > } > > > > return 0;
Re: [RFC PATCH 0/9] rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co.
Hi Phillip, Le 24/07/2019 à 15:29, Phillip Wood a écrit : > Hi Alban > > Thanks for working on this, it's great to see you back on the list and I > think it would be a useful addition to rebase. Unfortunately I'm not > sure about this implementation though (although the early bug fix > patches are useful in their own right) > > On 17/07/2019 15:39, Alban Gruin wrote: >> To prevent mistakes when editing a branch, rebase features a knob, >> rebase.missingCommitsCheck, to warn the user if a commit was dropped. >> Unfortunately, this check is only effective for the initial edit, which >> means that if you edit the todo list at a later point of the rebase and >> dropped a commit, no warnings or errors would be issued. >> >> This adds the ability to check if commits were dropped when resuming a >> rebase (with `rebase --continue'), when editing the todo list (with >> `rebase --edit-todo'), or when reloading the todo list after an `exec' >> command. > > I'm not sure if we really need to check the todo list when continuing or > after an exec command. In which case I don’t really understand why there is an `error' mode if one can completely bypass it with `--continue'. > The official way to edit the todo list is to run > 'git rebase --edit-todo' and I'm not sure if we support scripts writing > to .git/rebase-merge/git-rebase-todo directly. If we only support the > check after --edit-todo then I think the implementation can be > simplified as we can just write a copy of the file before it is edited > and don't need to check .git/rebase-merge/done. Additionally that would > catch commits that are added by the user and then deleted in a later > edit. They wont be in the original list so I don't think this series > will detect their deletion. > True -- but with this solution, if a bad command is introduced, there will be false negatives. Given the pitfall of my solution, this should be an acceptable trade-off. > At the extreme I have a script around rebase that runs 'rebase -i HEAD' > and then fills in the todo list with a fake editor that adds 'reset ...' > as the first line to set the starting point of the rebase. I think > dscho's garden-shears script does something similar. Under the proposed > scheme if I subsequently edit the todo list it will not catch any > deleted commits as the original list is empty. > > Best Wishes > > Phillip > Cheers, Alban
Re: What's cooking in git.git (Oct 2018, #04; Fri, 19)
Le 19/10/2018 à 20:05, Alban Gruin a écrit : > Le 19/10/2018 à 14:46, SZEDER Gábor a écrit : >> On Fri, Oct 19, 2018 at 03:02:22PM +0900, Junio C Hamano wrote: >>> Two large set of topics on "rebase in C" and "rebase -i in C" are >>> now in 'next'. >> >> I see occasional failures in 't5520-pull.sh': >> >> […] >> >> When running t5520 in a loop, it tends to fail between 10-40 >> iterations, even when the machine is not under heavy load. >> >> It appears that these failures started with commit 5541bd5b8f (rebase: >> default to using the builtin rebase, 2018-08-08), i.e. tip of >> 'pk/rebase-in-c-6-final', but it's a "flip the big switch" commit, so >> not very useful... >> The bug can be bisected by prepending the command chain of 5520.25 with `git config --bool rebase.usebuiltin true &&`. Commit 7debdaa4ad (“builtin rebase: support `--autostash` option”) is the culprit, but some commits that cause this test to fail with a different error (ie. “TODO”…) must be marked as good. The error comes from the call to `git stash apply $stash_id' in builtin/rebase.c:261. When $stash_id only contains decimals and no letters, git-stash tries to apply stash@{$stash_id}[0][1]. Thas was not a real problem with the shell script, because it did not abbreviate the object id of the stashed commit, so it was very unlikely that the oid would contain only digits. builtin/rebase.c shortens the oid[2], making this problem more likely to occur. We could add a switch to git-stash to tell it that the parameter is an oid, not anything else. [0] https://github.com/git/git/blob/master/git-stash.sh#L514-L520 [1] https://github.com/git/git/blob/pu/builtin/stash.c#L167-L168 [2] https://github.com/git/git/blob/next/builtin/rebase.c#L1373 Cheers, Alban
Re: [PATCH v2 3/3] rebase (autostash): use an explicit OID to apply the stash
Hi Johannes, this looks good to me, too! Cheers, Alban
[PATCH v2 01/16] sequencer: changes in parse_insn_buffer()
This clears the number of items of a todo_list before parsing it to allow to parse the same list multiple times without issues. As its items are not dynamically allocated, or don’t need to allocate memory, no additionnal memory management is required here. Furthermore, if a line is invalid, the type of the corresponding command is set to a garbage value, and its argument is defined properly. This will allow to recreate the text of a todo list from its commands, even if one of them is incorrect. Signed-off-by: Alban Gruin --- sequencer.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 8dd6db5a01..9c8bd3f632 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2023,6 +2023,8 @@ static int parse_insn_buffer(char *buf, struct todo_list *todo_list) char *p = buf, *next_p; int i, res = 0, fixup_okay = file_exists(rebase_path_done()); + todo_list->current = todo_list->nr = 0; + for (i = 1; *p; i++, p = next_p) { char *eol = strchrnul(p, '\n'); @@ -2036,7 +2038,10 @@ static int parse_insn_buffer(char *buf, struct todo_list *todo_list) if (parse_insn_line(item, p, eol)) { res = error(_("invalid line %d: %.*s"), i, (int)(eol - p), p); - item->command = TODO_NOOP; + item->command = TODO_COMMENT + 1; + item->arg = p; + item->arg_len = (int)(eol - p); + item->commit = NULL; } if (fixup_okay) -- 2.19.1
[PATCH v2 03/16] sequencer: refactor transform_todos() to work on a todo_list
This refactors transform_todos() to work on a todo_list. The function is renamed todo_list_transform(). As rebase -p still need to check the todo list from the disk, a new function is introduced, transform_todo_file(). It is still used by complete_action() and edit_todo_list() for now, but they will be replaced in a future commit. todo_list_transform() is not a static function, because it will be used by edit_todo_list() from rebase-interactive.c in a future commit. Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 2 +- rebase-interactive.c | 4 +-- sequencer.c | 46 +++ sequencer.h | 3 ++- 4 files changed, 35 insertions(+), 20 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index a2ab68ed06..abdf6126df 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -252,7 +252,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) } case SHORTEN_OIDS: case EXPAND_OIDS: - ret = transform_todos(flags); + ret = transform_todo_file(flags); break; case CHECK_TODO_LIST: ret = check_todo_list(); diff --git a/rebase-interactive.c b/rebase-interactive.c index 0f4119cbae..49f2f549e1 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -68,7 +68,7 @@ int edit_todo_list(unsigned flags) strbuf_release(&buf); - transform_todos(flags | TODO_LIST_SHORTEN_IDS); + transform_todo_file(flags | TODO_LIST_SHORTEN_IDS); if (strbuf_read_file(&buf, todo_file, 0) < 0) return error_errno(_("could not read '%s'."), todo_file); @@ -84,7 +84,7 @@ int edit_todo_list(unsigned flags) if (launch_sequence_editor(todo_file, NULL, NULL)) return -1; - transform_todos(flags & ~(TODO_LIST_SHORTEN_IDS)); + transform_todo_file(flags & ~(TODO_LIST_SHORTEN_IDS)); return 0; } diff --git a/sequencer.c b/sequencer.c index f791729271..07296f1f57 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4256,23 +4256,13 @@ int sequencer_add_exec_commands(const char *commands) return i; } -int transform_todos(unsigned flags) +void todo_list_transform(struct todo_list *todo_list, unsigned flags) { - const char *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; struct strbuf buf = STRBUF_INIT; struct todo_item *item; int i; - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) - return error(_("could not read '%s'."), todo_file); - - if (todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list)) { - todo_list_release(&todo_list); - return error(_("unusable todo list: '%s'"), todo_file); - } - - for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) { + for (item = todo_list->items, i = 0; i < todo_list->nr; i++, item++) { /* if the item is not a command write it and continue */ if (item->command >= TODO_COMMENT) { strbuf_addf(&buf, "%.*s\n", item->arg_len, item->arg); @@ -4308,9 +4298,33 @@ int transform_todos(unsigned flags) strbuf_addf(&buf, " %.*s\n", item->arg_len, item->arg); } - i = write_message(buf.buf, buf.len, todo_file, 0); + strbuf_reset(&todo_list->buf); + strbuf_add(&todo_list->buf, buf.buf, buf.len); + strbuf_release(&buf); + + if (todo_list_parse_insn_buffer(todo_list->buf.buf, todo_list)) + BUG("unusable todo list"); +} + +int transform_todo_file(unsigned flags) +{ + const char *todo_file = rebase_path_todo(); + struct todo_list todo_list = TODO_LIST_INIT; + int res; + + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) + return error(_("could not read '%s'."), todo_file); + + if (todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list)) { + todo_list_release(&todo_list); + return error(_("unusable todo list: '%s'"), todo_file); + } + + todo_list_transform(&todo_list, flags); + + res = write_message(todo_list.buf.buf, todo_list.buf.len, todo_file, 0); todo_list_release(&todo_list); - return i; + return res; } enum missing_commit_check_level get_missing_commit_check_level(void) @@ -4571,7 +4585,7 @@ int complete_action(struct replay_opts *opts, unsigned flags, return error(_("could not copy '%s' to '%s'."), todo_file, re
[PATCH v2 00/16] sequencer: refactor functions working on a todo_list
At the center of the "interactive" part of the interactive rebase lies the todo list. When the user starts an interactive rebase, a todo list is generated, presented to the user (who then edits it using a text editor), read back, and then is checked and processed before the actual rebase takes place. Some of this processing includes adding execs commands, reordering fixup! and squash! commits, and checking if no commits were accidentally dropped by the user. Before I converted the interactive rebase in C, these functions were called by git-rebase--interactive.sh through git-rebase--helper. Since the only way to pass around a large amount of data between a shell script and a C program is to use a file (or any declination of a file), the functions that checked and processed the todo list were directly working on a file, the same file that the user edited. During the conversion, I did not address this issue, which lead to a complete_action() that reads the todo list file, does some computation based on its content, and writes it back to the disk, several times in the same function. As it is not an efficient way to handle a data structure, this patch series refactor the functions that processes the todo list to work on a todo_list structure instead of reading it from the disk. Some commits consists in modifying edit_todo_list() (initially used by --edit-todo) to handle the initial edition of the todo list, to increase code sharing. It is based onto ag/rebase-i-in-c (34b4731, "rebase -i: move rebase--helper modes to rebase--interactive"). Changes since v1: - When a line is invalid, parse_insn_buffer() sets the type of the corresponding command to a garbage value instead of `noop', and its argument is defined properly. - todo_list_add_exec_commands(), todo_list_rearrange_squash(), skip_unnecessary_picks() don’t reparse the todo list after processing them. Instead, they recreate a new item list. - Due to the previous change, a todo list buffer can’t directly be written to the disk. A new function, todo_list_write_to_disk(), is introduced to take care of this task. - rewrite_file() has been deleted. - A call to strbuf_addf(&buf, "\n"); has been replaced by strbuf_addch(…). - complete_action() and todo_list_check() expect that their input todo list have already been parsed. - complete_action() no longer writes "noop\n" to the todo list buffer if it is empty. Instead, it appends a `noop' command to the item list. Alban Gruin (16): sequencer: changes in parse_insn_buffer() sequencer: make the todo_list structure public sequencer: refactor transform_todos() to work on a todo_list sequencer: introduce todo_list_write_to_file() sequencer: refactor check_todo_list() to work on a todo_list sequencer: refactor sequencer_add_exec_commands() to work on a todo_list sequencer: refactor rearrange_squash() to work on a todo_list sequencer: make sequencer_make_script() write its script to a strbuf sequencer: change complete_action() to use the refactored functions sequencer: refactor skip_unnecessary_picks() to work on a todo_list rebase-interactive: use todo_list_write_to_file() in edit_todo_list() rebase-interactive: append_todo_help() changes rebase-interactive: rewrite edit_todo_list() to handle the initial edit sequencer: use edit_todo_list() in complete_action() sequencer: fix a call to error() in transform_todo_file() rebase--interactive: move transform_todo_file() to rebase--interactive.c builtin/rebase--interactive.c | 68 +++- rebase-interactive.c | 142 ++-- rebase-interactive.h | 8 +- sequencer.c | 589 +- sequencer.h | 68 +++- 5 files changed, 455 insertions(+), 420 deletions(-) -- 2.19.1
[PATCH v2 02/16] sequencer: make the todo_list structure public
This makes the structures todo_list and todo_item, and the functions todo_list_release() and parse_insn_buffer(), accessible outside of sequencer.c. Signed-off-by: Alban Gruin --- No changes since v1. sequencer.c | 66 + sequencer.h | 48 ++ 2 files changed, 59 insertions(+), 55 deletions(-) diff --git a/sequencer.c b/sequencer.c index 9c8bd3f632..f791729271 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1401,31 +1401,6 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit) return 1; } -/* - * Note that ordering matters in this enum. Not only must it match the mapping - * below, it is also divided into several sections that matter. When adding - * new commands, make sure you add it in the right section. - */ -enum todo_command { - /* commands that handle commits */ - TODO_PICK = 0, - TODO_REVERT, - TODO_EDIT, - TODO_REWORD, - TODO_FIXUP, - TODO_SQUASH, - /* commands that do something else than handling a single commit */ - TODO_EXEC, - TODO_LABEL, - TODO_RESET, - TODO_MERGE, - /* commands that do nothing but are counted for reporting progress */ - TODO_NOOP, - TODO_DROP, - /* comments (not counted for reporting progress) */ - TODO_COMMENT -}; - static struct { char c; const char *str; @@ -1897,26 +1872,7 @@ enum todo_item_flags { TODO_EDIT_MERGE_MSG = 1 }; -struct todo_item { - enum todo_command command; - struct commit *commit; - unsigned int flags; - const char *arg; - int arg_len; - size_t offset_in_buf; -}; - -struct todo_list { - struct strbuf buf; - struct todo_item *items; - int nr, alloc, current; - int done_nr, total_nr; - struct stat_data stat; -}; - -#define TODO_LIST_INIT { STRBUF_INIT } - -static void todo_list_release(struct todo_list *todo_list) +void todo_list_release(struct todo_list *todo_list) { strbuf_release(&todo_list->buf); FREE_AND_NULL(todo_list->items); @@ -2017,7 +1973,7 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol) return !item->commit; } -static int parse_insn_buffer(char *buf, struct todo_list *todo_list) +int todo_list_parse_insn_buffer(char *buf, struct todo_list *todo_list) { struct todo_item *item; char *p = buf, *next_p; @@ -2115,7 +2071,7 @@ static int read_populate_todo(struct todo_list *todo_list, return error(_("could not stat '%s'"), todo_file); fill_stat_data(&todo_list->stat, &st); - res = parse_insn_buffer(todo_list->buf.buf, todo_list); + res = todo_list_parse_insn_buffer(todo_list->buf.buf, todo_list); if (res) { if (is_rebase_i(opts)) return error(_("please fix this using " @@ -2146,7 +2102,7 @@ static int read_populate_todo(struct todo_list *todo_list, FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w"); if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 && - !parse_insn_buffer(done.buf.buf, &done)) + !todo_list_parse_insn_buffer(done.buf.buf, &done)) todo_list->done_nr = count_commands(&done); else todo_list->done_nr = 0; @@ -4276,7 +4232,7 @@ int sequencer_add_exec_commands(const char *commands) if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) return error(_("could not read '%s'."), todo_file); - if (parse_insn_buffer(todo_list.buf.buf, &todo_list)) { + if (todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list)) { todo_list_release(&todo_list); return error(_("unusable todo list: '%s'"), todo_file); } @@ -4311,7 +4267,7 @@ int transform_todos(unsigned flags) if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) return error(_("could not read '%s'."), todo_file); - if (parse_insn_buffer(todo_list.buf.buf, &todo_list)) { + if (todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list)) { todo_list_release(&todo_list); return error(_("unusable todo list: '%s'"), todo_file); } @@ -4397,7 +4353,7 @@ int check_todo_list(void) goto leave_check; } advise_to_edit_todo = res = - parse_insn_buffer(todo_list.buf.buf, &todo_list); + todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list); if (res || check_level == MISSING_COMMIT_CHECK_IGNORE)
[PATCH v2 10/16] sequencer: refactor skip_unnecessary_picks() to work on a todo_list
This refactors skip_unnecessary_picks() to work on a todo_list. The file-handling logic is completely dropped here, as its only usage is made by complete_action(). Instead of truncating the todo list’s buffer, the items are moved to the beginning of the list, eliminating the need to reparse the list. This also means its buffer cannot be directly written to the disk. rewrite_file() is then removed, as it is now unused. Signed-off-by: Alban Gruin --- sequencer.c | 80 - 1 file changed, 18 insertions(+), 62 deletions(-) diff --git a/sequencer.c b/sequencer.c index 1f7579328b..7286498572 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4408,52 +4408,21 @@ int check_todo_list_from_file(void) return res; } -static int rewrite_file(const char *path, const char *buf, size_t len) -{ - int rc = 0; - int fd = open(path, O_WRONLY | O_TRUNC); - if (fd < 0) - return error_errno(_("could not open '%s' for writing"), path); - if (write_in_full(fd, buf, len) < 0) - rc = error_errno(_("could not write to '%s'"), path); - if (close(fd) && !rc) - rc = error_errno(_("could not close '%s'"), path); - return rc; -} - /* skip picking commits whose parents are unchanged */ -static int skip_unnecessary_picks(struct object_id *output_oid) +static int skip_unnecessary_picks(struct todo_list *todo_list, + struct object_id *output_oid) { - const char *todo_file = rebase_path_todo(); - struct strbuf buf = STRBUF_INIT; - struct todo_list todo_list = TODO_LIST_INIT; struct object_id *parent_oid; - int fd, i; - - if (!read_oneliner(&buf, rebase_path_onto(), 0)) - return error(_("could not read 'onto'")); - if (get_oid(buf.buf, output_oid)) { - strbuf_release(&buf); - return error(_("need a HEAD to fixup")); - } - strbuf_release(&buf); - - if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0) - return -1; - if (todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) { - todo_list_release(&todo_list); - return -1; - } + int i; - for (i = 0; i < todo_list.nr; i++) { - struct todo_item *item = todo_list.items + i; + for (i = 0; i < todo_list->nr; i++) { + struct todo_item *item = todo_list->items + i; if (item->command >= TODO_NOOP) continue; if (item->command != TODO_PICK) break; if (parse_commit(item->commit)) { - todo_list_release(&todo_list); return error(_("could not parse commit '%s'"), oid_to_hex(&item->commit->object.oid)); } @@ -4467,37 +4436,22 @@ static int skip_unnecessary_picks(struct object_id *output_oid) oidcpy(output_oid, &item->commit->object.oid); } if (i > 0) { - int offset = get_item_line_offset(&todo_list, i); const char *done_path = rebase_path_done(); - fd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666); - if (fd < 0) { - error_errno(_("could not open '%s' for writing"), - done_path); - todo_list_release(&todo_list); - return -1; - } - if (write_in_full(fd, todo_list.buf.buf, offset) < 0) { + if (todo_list_write_to_file(todo_list, done_path, NULL, NULL, 0, 0, i, 0)) { error_errno(_("could not write to '%s'"), done_path); - todo_list_release(&todo_list); - close(fd); return -1; } - close(fd); - if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset, -todo_list.buf.len - offset) < 0) { - todo_list_release(&todo_list); - return -1; - } + memmove(todo_list->items, todo_list->items + i, + sizeof(struct todo_item) * (todo_list->nr - i)); + todo_list->nr -= i; + todo_list->current = 0; - todo_list.current = i; - if (is_fixup(peek_command(&todo_list, 0))) - record_in_rewritten(output_oid, peek_command(&todo_list, 0)); + if (is_fixup(peek_comm
[PATCH v2 05/16] sequencer: refactor check_todo_list() to work on a todo_list
This refactors check_todo_list() to work on a todo_list to avoid redundant reads and writes to the disk. The function is renamed todo_list_check(). The parsing of the two todo lists is left to the caller. As rebase -p still need to check the todo list from the disk, a new function is introduced, check_todo_list_from_file(). It reads the file from the disk, parses it, pass the todo_list to todo_list_check(), and writes it back to the disk. As get_missing_commit_check_level() and the enum missing_commit_check_level are no longer needed inside of sequencer.c, they are moved to rebase-interactive.c, and made static again. Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 2 +- rebase-interactive.c | 90 - rebase-interactive.h | 1 + sequencer.c | 120 +++--- sequencer.h | 9 +-- 5 files changed, 115 insertions(+), 107 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index abdf6126df..c1d87c0fe6 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -255,7 +255,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = transform_todo_file(flags); break; case CHECK_TODO_LIST: - ret = check_todo_list(); + ret = check_todo_list_from_file(); break; case REARRANGE_SQUASH: ret = rearrange_squash(); diff --git a/rebase-interactive.c b/rebase-interactive.c index 49f2f549e1..9e81ecfe9f 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -1,8 +1,32 @@ #include "cache.h" #include "commit.h" -#include "rebase-interactive.h" #include "sequencer.h" +#include "rebase-interactive.h" #include "strbuf.h" +#include "commit-slab.h" +#include "config.h" + +enum missing_commit_check_level { + MISSING_COMMIT_CHECK_IGNORE = 0, + MISSING_COMMIT_CHECK_WARN, + MISSING_COMMIT_CHECK_ERROR +}; + +static enum missing_commit_check_level get_missing_commit_check_level(void) +{ + const char *value; + + if (git_config_get_value("rebase.missingcommitscheck", &value) || + !strcasecmp("ignore", value)) + return MISSING_COMMIT_CHECK_IGNORE; + if (!strcasecmp("warn", value)) + return MISSING_COMMIT_CHECK_WARN; + if (!strcasecmp("error", value)) + return MISSING_COMMIT_CHECK_ERROR; + warning(_("unrecognized setting %s for option " + "rebase.missingCommitsCheck. Ignoring."), value); + return MISSING_COMMIT_CHECK_IGNORE; +} void append_todo_help(unsigned edit_todo, unsigned keep_empty, struct strbuf *buf) @@ -88,3 +112,67 @@ int edit_todo_list(unsigned flags) return 0; } + +define_commit_slab(commit_seen, unsigned char); +/* + * Check if the user dropped some commits by mistake + * Behaviour determined by rebase.missingCommitsCheck. + * Check if there is an unrecognized command or a + * bad SHA-1 in a command. + */ +int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo) +{ + enum missing_commit_check_level check_level = get_missing_commit_check_level(); + struct strbuf missing = STRBUF_INIT; + int res = 0, i; + struct commit_seen commit_seen; + + init_commit_seen(&commit_seen); + + if (check_level == MISSING_COMMIT_CHECK_IGNORE) + goto leave_check; + + /* Mark the commits in git-rebase-todo as seen */ + for (i = 0; i < new_todo->nr; i++) { + struct commit *commit = new_todo->items[i].commit; + if (commit) + *commit_seen_at(&commit_seen, commit) = 1; + } + + /* Find commits in git-rebase-todo.backup yet unseen */ + for (i = old_todo->nr - 1; i >= 0; i--) { + struct todo_item *item = old_todo->items + i; + struct commit *commit = item->commit; + if (commit && !*commit_seen_at(&commit_seen, commit)) { + strbuf_addf(&missing, " - %s %.*s\n", + find_unique_abbrev(&commit->object.oid, DEFAULT_ABBREV), + item->arg_len, item->arg); + *commit_seen_at(&commit_seen, commit) = 1; + } + } + + /* Warn about missing commits */ + if (!missing.len) + goto leave_check; + + if (check_level == MISSING_COMMIT_CHECK_ERROR) + res = 1; + + fprintf(stderr, + _("Warning: some commits may have been dropped accidentally.\n" + "Dr
[PATCH v2 08/16] sequencer: make sequencer_make_script() write its script to a strbuf
This makes sequencer_make_script() write its script to a strbuf (ie. the buffer of a todo_list) instead of a FILE. This reduce the amount of read/write made by rebase interactive. Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 13 +++- sequencer.c | 38 --- sequencer.h | 2 +- 3 files changed, 26 insertions(+), 27 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index f827e53f05..eef1ff2e83 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -71,7 +71,8 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, const char *head_hash = NULL; char *revisions = NULL, *shortrevisions = NULL; struct argv_array make_script_args = ARGV_ARRAY_INIT; - FILE *todo_list; + FILE *todo_list_file; + struct todo_list todo_list = TODO_LIST_INIT; if (prepare_branch_to_be_rebased(opts, switch_to)) return -1; @@ -93,8 +94,8 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, if (!upstream && squash_onto) write_file(path_squash_onto(), "%s\n", squash_onto); - todo_list = fopen(rebase_path_todo(), "w"); - if (!todo_list) { + todo_list_file = fopen(rebase_path_todo(), "w"); + if (!todo_list_file) { free(revisions); free(shortrevisions); @@ -105,10 +106,11 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, if (restrict_revision) argv_array_push(&make_script_args, restrict_revision); - ret = sequencer_make_script(todo_list, + ret = sequencer_make_script(&todo_list.buf, make_script_args.argc, make_script_args.argv, flags); - fclose(todo_list); + fputs(todo_list.buf.buf, todo_list_file); + fclose(todo_list_file); if (ret) error(_("could not generate todo list")); @@ -120,6 +122,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, free(revisions); free(shortrevisions); + todo_list_release(&todo_list); argv_array_clear(&make_script_args); return ret; diff --git a/sequencer.c b/sequencer.c index 09e32f3e5a..94167588a2 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3927,7 +3927,7 @@ static const char *label_oid(struct object_id *oid, const char *label, } static int make_script_with_merges(struct pretty_print_context *pp, - struct rev_info *revs, FILE *out, + struct rev_info *revs, struct strbuf *out, unsigned flags) { int keep_empty = flags & TODO_LIST_KEEP_EMPTY; @@ -4076,7 +4076,7 @@ static int make_script_with_merges(struct pretty_print_context *pp, * gathering commits not yet shown, reversing the list on the fly, * then outputting that list (labeling revisions as needed). */ - fprintf(out, "%s onto\n", cmd_label); + strbuf_addf(out, "%s onto\n", cmd_label); for (iter = tips; iter; iter = iter->next) { struct commit_list *list = NULL, *iter2; @@ -4086,9 +4086,9 @@ static int make_script_with_merges(struct pretty_print_context *pp, entry = oidmap_get(&state.commit2label, &commit->object.oid); if (entry) - fprintf(out, "\n# Branch %s\n", entry->string); + strbuf_addf(out, "\n%c Branch %s\n", comment_line_char, entry->string); else - fprintf(out, "\n"); + strbuf_addch(out, '\n'); while (oidset_contains(&interesting, &commit->object.oid) && !oidset_contains(&shown, &commit->object.oid)) { @@ -4101,8 +4101,8 @@ static int make_script_with_merges(struct pretty_print_context *pp, } if (!commit) - fprintf(out, "%s %s\n", cmd_reset, - rebase_cousins ? "onto" : "[new root]"); + strbuf_addf(out, "%s %s\n", cmd_reset, + rebase_cousins ? "onto" : "[new root]"); else { const char *to = NULL; @@ -4115,12 +4115,12 @@ static int make_script_with_merges(struct pretty_print_context *pp, &state); if (!to || !strcmp(to, "onto")) - fprintf(out, "
[PATCH v2 12/16] rebase-interactive: append_todo_help() changes
This moves the writing of the comment "Rebase $shortrevisions onto $shortonto ($command_count commands)" from complete_action() to append_todo_help(). shortrevisions, shortonto, and command_count are passed as parameters to append_todo_help(). During the initial edit of the todo list, shortrevisions and shortonto are not NULL. Therefore, if shortrevisions or shortonto is NULL, then edit_todo would be true, otherwise it would be false. Thus, edit_todo is removed from the parameters of append_todo_help(). edit_todo_list() and complete_action() are modified to fit these changes. Signed-off-by: Alban Gruin --- rebase-interactive.c | 12 +++- rebase-interactive.h | 3 ++- sequencer.c | 16 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/rebase-interactive.c b/rebase-interactive.c index b2be99a900..0643d54b1b 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -28,7 +28,8 @@ static enum missing_commit_check_level get_missing_commit_check_level(void) return MISSING_COMMIT_CHECK_IGNORE; } -void append_todo_help(unsigned edit_todo, unsigned keep_empty, +void append_todo_help(unsigned keep_empty, int command_count, + const char *shortrevisions, const char *shortonto, struct strbuf *buf) { const char *msg = _("\nCommands:\n" @@ -47,6 +48,15 @@ void append_todo_help(unsigned edit_todo, unsigned keep_empty, ". specified). Use -c to reword the commit message.\n" "\n" "These lines can be re-ordered; they are executed from top to bottom.\n"); + unsigned edit_todo = !(shortrevisions && shortonto); + + if (!edit_todo) { + strbuf_addch(buf, '\n'); + strbuf_commented_addf(buf, Q_("Rebase %s onto %s (%d command)", + "Rebase %s onto %s (%d commands)", + command_count), + shortrevisions, shortonto, command_count); + } strbuf_add_commented_lines(buf, msg, strlen(msg)); diff --git a/rebase-interactive.h b/rebase-interactive.h index 6bc7bc315d..61858f3a02 100644 --- a/rebase-interactive.h +++ b/rebase-interactive.h @@ -1,7 +1,8 @@ #ifndef REBASE_INTERACTIVE_H #define REBASE_INTERACTIVE_H -void append_todo_help(unsigned edit_todo, unsigned keep_empty, +void append_todo_help(unsigned keep_empty, int command_count, + const char *shortrevisions, const char *shortonto, struct strbuf *buf); int edit_todo_list(unsigned flags); int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo); diff --git a/sequencer.c b/sequencer.c index 7286498572..4c664d6388 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4331,21 +4331,13 @@ int todo_list_write_to_file(struct todo_list *todo_list, const char *file, const char *shortrevisions, const char *shortonto, int command_count, int append_help, int num, unsigned flags) { - int edit_todo = !(shortrevisions && shortonto), res; + int res; struct strbuf buf = STRBUF_INIT; todo_list_to_strbuf(todo_list, &buf, num, flags); - - if (append_help) { - if (!edit_todo) { - strbuf_addch(&buf, '\n'); - strbuf_commented_addf(&buf, Q_("Rebase %s onto %s (%d command)", - "Rebase %s onto %s (%d commands)", - command_count), - shortrevisions, shortonto, command_count); - } - append_todo_help(edit_todo, flags & TODO_LIST_KEEP_EMPTY, &buf); - } + if (append_help) + append_todo_help(flags & TODO_LIST_KEEP_EMPTY, command_count, +shortrevisions, shortonto, &buf); res = write_message(buf.buf, buf.len, file, 0); strbuf_release(&buf); -- 2.19.1
[PATCH v2 11/16] rebase-interactive: use todo_list_write_to_file() in edit_todo_list()
Just like complete_action(), edit_todo_list() used a function (transform_todo_file()) that read the todo list from the disk and wrote it back, resulting in useless disk accesses. This changes edit_todo_list() to call directly todo_list_write_to_file() instead. Signed-off-by: Alban Gruin --- rebase-interactive.c | 38 -- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/rebase-interactive.c b/rebase-interactive.c index 9e81ecfe9f..b2be99a900 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -78,39 +78,33 @@ void append_todo_help(unsigned edit_todo, unsigned keep_empty, int edit_todo_list(unsigned flags) { - struct strbuf buf = STRBUF_INIT; const char *todo_file = rebase_path_todo(); + struct todo_list todo_list = TODO_LIST_INIT; + int res = 0; - if (strbuf_read_file(&buf, todo_file, 0) < 0) + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) return error_errno(_("could not read '%s'."), todo_file); - strbuf_stripspace(&buf, 1); - if (write_message(buf.buf, buf.len, todo_file, 0)) { - strbuf_release(&buf); + strbuf_stripspace(&todo_list.buf, 1); + todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list); + if (todo_list_write_to_file(&todo_list, todo_file, NULL, NULL, 0, 1, -1, + flags | TODO_LIST_SHORTEN_IDS)) { + todo_list_release(&todo_list); return -1; } - strbuf_release(&buf); - - transform_todo_file(flags | TODO_LIST_SHORTEN_IDS); - - if (strbuf_read_file(&buf, todo_file, 0) < 0) - return error_errno(_("could not read '%s'."), todo_file); - - append_todo_help(1, 0, &buf); - if (write_message(buf.buf, buf.len, todo_file, 0)) { - strbuf_release(&buf); + strbuf_reset(&todo_list.buf); + if (launch_sequence_editor(todo_file, &todo_list.buf, NULL)) { + todo_list_release(&todo_list); return -1; } - strbuf_release(&buf); + if (!todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list)) + res = todo_list_write_to_file(&todo_list, todo_file, NULL, NULL, 0, 0, -1, + flags & ~(TODO_LIST_SHORTEN_IDS)); - if (launch_sequence_editor(todo_file, NULL, NULL)) - return -1; - - transform_todo_file(flags & ~(TODO_LIST_SHORTEN_IDS)); - - return 0; + todo_list_release(&todo_list); + return res; } define_commit_slab(commit_seen, unsigned char); -- 2.19.1
[PATCH v2 13/16] rebase-interactive: rewrite edit_todo_list() to handle the initial edit
edit_todo_list() is changed to work on a todo_list, and to handle the initial edition of the todo list (ie. making a backup of the todo list). It does not check for dropped commits yet, as todo_list_check() does not take the commits that have already been processed by the rebase (ie. the todo list is edited in the middle of a rebase session). Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 23 - rebase-interactive.c | 48 ++- rebase-interactive.h | 4 ++- sequencer.c | 3 +-- sequencer.h | 1 + 5 files changed, 52 insertions(+), 27 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index c07330bdff..0f83422aa0 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -13,6 +13,27 @@ static GIT_PATH_FUNC(path_state_dir, "rebase-merge/") static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto") static GIT_PATH_FUNC(path_interactive, "rebase-merge/interactive") +static int edit_todo_file(unsigned flags) +{ + const char *todo_file = rebase_path_todo(); + struct todo_list todo_list = TODO_LIST_INIT, + new_todo = TODO_LIST_INIT; + + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) + return error_errno(_("could not read '%s'."), todo_file); + + strbuf_stripspace(&todo_list.buf, 1); + if (!edit_todo_list(&todo_list, &new_todo, flags, 0, NULL, NULL) && + todo_list_write_to_file(&new_todo, todo_file, NULL, NULL, 0, 0, -1, + flags & ~(TODO_LIST_SHORTEN_IDS)) < 0) + return error_errno(_("could not write '%s'"), todo_file); + + todo_list_release(&todo_list); + todo_list_release(&new_todo); + + return 0; +} + static int get_revision_ranges(const char *upstream, const char *onto, const char **head_hash, char **revisions, char **shortrevisions) @@ -234,7 +255,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) break; } case EDIT_TODO: - ret = edit_todo_list(flags); + ret = edit_todo_file(flags); break; case SHOW_CURRENT_PATCH: { struct child_process cmd = CHILD_PROCESS_INIT; diff --git a/rebase-interactive.c b/rebase-interactive.c index 0643d54b1b..d7a0818ac9 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -86,35 +86,37 @@ void append_todo_help(unsigned keep_empty, int command_count, } } -int edit_todo_list(unsigned flags) +int edit_todo_list(struct todo_list *todo_list, struct todo_list *new_todo, + unsigned flags, int command_count, + const char *shortrevisions, const char *shortonto) { const char *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; - int res = 0; - - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) - return error_errno(_("could not read '%s'."), todo_file); - - strbuf_stripspace(&todo_list.buf, 1); - todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list); - if (todo_list_write_to_file(&todo_list, todo_file, NULL, NULL, 0, 1, -1, - flags | TODO_LIST_SHORTEN_IDS)) { - todo_list_release(&todo_list); - return -1; + unsigned initial = shortrevisions && shortonto; + + if (initial) { + todo_list_write_to_file(todo_list, todo_file, shortrevisions, shortonto, + 0, 1, -1, flags | TODO_LIST_SHORTEN_IDS); + + if (copy_file(rebase_path_todo_backup(), todo_file, 0666)) + return error(_("could not copy '%s' to '%s'."), todo_file, +rebase_path_todo_backup()); + } else { + todo_list_parse_insn_buffer(todo_list->buf.buf, todo_list); + todo_list_write_to_file(todo_list, todo_file, NULL, NULL, 0, 1, -1, + flags | TODO_LIST_SHORTEN_IDS); } - strbuf_reset(&todo_list.buf); - if (launch_sequence_editor(todo_file, &todo_list.buf, NULL)) { - todo_list_release(&todo_list); - return -1; - } + if (launch_sequence_editor(todo_file, &new_todo->buf, NULL)) + return -2; - if (!todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list)) - res = todo_list_write_to_file(&todo_list, todo_file, NULL, NULL, 0, 0, -1, - flags &am