git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH 00/15] sequencer: refactor functions working on a todo_list
@ 2018-10-07 19:54 Alban Gruin
  2018-10-07 19:54 ` [PATCH 01/15] sequencer: clear the number of items of a todo_list before parsing Alban Gruin
                   ` (15 more replies)
  0 siblings, 16 replies; 29+ messages in thread
From: Alban Gruin @ 2018-10-07 19:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

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.

Alban Gruin (15):
  sequencer: clear the number of items of a todo_list before parsing
  sequencer: make the todo_list structure public
  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: refactor transform_todos() 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_transform() 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 |  65 +++--
 rebase-interactive.c          | 161 ++++++++++--
 rebase-interactive.h          |   8 +-
 sequencer.c                   | 479 ++++++++++++----------------------
 sequencer.h                   |  66 ++++-
 5 files changed, 406 insertions(+), 373 deletions(-)

-- 
2.19.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 01/15] sequencer: clear the number of items of a todo_list before parsing
  2018-10-07 19:54 [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
@ 2018-10-07 19:54 ` Alban Gruin
  2018-10-07 19:54 ` [PATCH 02/15] sequencer: make the todo_list structure public Alban Gruin
                   ` (14 subsequent siblings)
  15 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2018-10-07 19:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

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.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 0a3292d5c4..ed798b95d1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2063,6 +2063,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');
 
-- 
2.19.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 02/15] sequencer: make the todo_list structure public
  2018-10-07 19:54 [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
  2018-10-07 19:54 ` [PATCH 01/15] sequencer: clear the number of items of a todo_list before parsing Alban Gruin
@ 2018-10-07 19:54 ` Alban Gruin
  2018-10-07 19:54 ` [PATCH 03/15] sequencer: refactor check_todo_list() to work on a todo_list Alban Gruin
                   ` (13 subsequent siblings)
  15 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2018-10-07 19:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

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 <alban.gruin@gmail.com>
---
 sequencer.c | 66 +++++++++--------------------------------------------
 sequencer.h | 48 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 55 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ed798b95d1..bb8ca2477f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1443,31 +1443,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;
@@ -1937,26 +1912,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);
@@ -2057,7 +2013,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;
@@ -2152,7 +2108,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 "
@@ -2183,7 +2139,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;
@@ -4429,7 +4385,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);
 	}
@@ -4485,7 +4441,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);
 	}
@@ -4571,7 +4527,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)
 		goto leave_check;
@@ -4590,7 +4546,7 @@ int check_todo_list(void)
 		goto leave_check;
 	}
 	strbuf_release(&todo_file);
-	res = !!parse_insn_buffer(todo_list.buf.buf, &todo_list);
+	res = !!todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list);
 
 	/* Find commits in git-rebase-todo.backup yet unseen */
 	for (i = todo_list.nr - 1; i >= 0; i--) {
@@ -4672,7 +4628,7 @@ static int skip_unnecessary_picks(struct object_id *output_oid)
 
 	if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0)
 		return -1;
-	if (parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {
+	if (todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {
 		todo_list_release(&todo_list);
 		return -1;
 	}
@@ -4760,7 +4716,7 @@ int complete_action(struct replay_opts *opts, unsigned flags,
 	if (strbuf_read_file(buf, todo_file, 0) < 0)
 		return error_errno(_("could not read '%s'."), todo_file);
 
-	if (parse_insn_buffer(buf->buf, &todo_list)) {
+	if (todo_list_parse_insn_buffer(buf->buf, &todo_list)) {
 		todo_list_release(&todo_list);
 		return error(_("unusable todo list: '%s'"), todo_file);
 	}
@@ -4868,7 +4824,7 @@ int rearrange_squash(void)
 
 	if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0)
 		return -1;
-	if (parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {
+	if (todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {
 		todo_list_release(&todo_list);
 		return -1;
 	}
diff --git a/sequencer.h b/sequencer.h
index 660cff5050..c786dee543 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -72,6 +72,54 @@ enum missing_commit_check_level {
 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
+ * 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
+};
+
+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 }
+
+int todo_list_parse_insn_buffer(char *buf, struct todo_list *todo_list);
+void todo_list_release(struct todo_list *todo_list);
+
 /* Call this to setup defaults before parsing command line options */
 void sequencer_init_config(struct replay_opts *opts);
 int sequencer_pick_revisions(struct replay_opts *opts);
-- 
2.19.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 03/15] sequencer: refactor check_todo_list() to work on a todo_list
  2018-10-07 19:54 [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
  2018-10-07 19:54 ` [PATCH 01/15] sequencer: clear the number of items of a todo_list before parsing Alban Gruin
  2018-10-07 19:54 ` [PATCH 02/15] sequencer: make the todo_list structure public Alban Gruin
@ 2018-10-07 19:54 ` Alban Gruin
  2018-10-11 11:23   ` Phillip Wood
  2018-10-07 19:54 ` [PATCH 04/15] sequencer: refactor sequencer_add_exec_commands() " Alban Gruin
                   ` (12 subsequent siblings)
  15 siblings, 1 reply; 29+ messages in thread
From: Alban Gruin @ 2018-10-07 19:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

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().

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 <alban.gruin@gmail.com>
---
 builtin/rebase--interactive.c |   2 +-
 rebase-interactive.c          | 106 ++++++++++++++++++++++++++++++++-
 rebase-interactive.h          |   1 +
 sequencer.c                   | 109 ++++------------------------------
 sequencer.h                   |   9 +--
 5 files changed, 120 insertions(+), 107 deletions(-)

diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
index a2ab68ed06..ea1f93ccb6 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_todos(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 0f4119cbae..ef8540245d 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,83 @@ 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 advise_to_edit_todo = 0, res = 0, i;
+	struct commit_seen commit_seen;
+
+	init_commit_seen(&commit_seen);
+
+	res = todo_list_parse_insn_buffer(old_todo->buf.buf, old_todo);
+	if (!res)
+		res = todo_list_parse_insn_buffer(new_todo->buf.buf, new_todo);
+	if (res) {
+		advise_to_edit_todo = res;
+		goto leave_check;
+	}
+
+	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)
+		advise_to_edit_todo = res = 1;
+
+	fprintf(stderr,
+		_("Warning: some commits may have been dropped accidentally.\n"
+		"Dropped commits (newer to older):\n"));
+
+	/* Make the list user-friendly and display */
+	fputs(missing.buf, stderr);
+	strbuf_release(&missing);
+
+	fprintf(stderr, _("To avoid this message, use \"drop\" to "
+		"explicitly remove a commit.\n\n"
+		"Use 'git config rebase.missingCommitsCheck' to change "
+		"the level of warnings.\n"
+		"The possible behaviours are: ignore, warn, error.\n\n"));
+
+leave_check:
+	clear_commit_seen(&commit_seen);
+
+	if (advise_to_edit_todo)
+		fprintf(stderr,
+			_("You can fix this with 'git rebase --edit-todo' "
+			  "and then run 'git rebase --continue'.\n"
+			  "Or you can abort the rebase with 'git rebase"
+			  " --abort'.\n"));
+
+	return res;
+}
diff --git a/rebase-interactive.h b/rebase-interactive.h
index 971da03776..6bc7bc315d 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -4,5 +4,6 @@
 void append_todo_help(unsigned edit_todo, unsigned keep_empty,
 		      struct strbuf *buf);
 int edit_todo_list(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 bb8ca2477f..8dda61927c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4487,111 +4487,26 @@ int transform_todos(unsigned flags)
 	return i;
 }
 
-enum missing_commit_check_level get_missing_commit_check_level(void)
+int check_todo_list_from_file(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;
-}
-
-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 check_todo_list(void)
-{
-	enum missing_commit_check_level check_level = get_missing_commit_check_level();
-	struct strbuf todo_file = STRBUF_INIT;
-	struct todo_list todo_list = TODO_LIST_INIT;
-	struct strbuf missing = STRBUF_INIT;
-	int advise_to_edit_todo = 0, res = 0, i;
-	struct commit_seen commit_seen;
-
-	init_commit_seen(&commit_seen);
+	struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
+	int res = 0;
 
-	strbuf_addstr(&todo_file, rebase_path_todo());
-	if (strbuf_read_file_or_whine(&todo_list.buf, todo_file.buf) < 0) {
+	if (strbuf_read_file_or_whine(&new_todo.buf, rebase_path_todo()) < 0) {
 		res = -1;
-		goto leave_check;
-	}
-	advise_to_edit_todo = res =
-		todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list);
-
-	if (res || check_level == MISSING_COMMIT_CHECK_IGNORE)
-		goto leave_check;
-
-	/* Mark the commits in git-rebase-todo as seen */
-	for (i = 0; i < todo_list.nr; i++) {
-		struct commit *commit = todo_list.items[i].commit;
-		if (commit)
-			*commit_seen_at(&commit_seen, commit) = 1;
+		goto out;
 	}
 
-	todo_list_release(&todo_list);
-	strbuf_addstr(&todo_file, ".backup");
-	if (strbuf_read_file_or_whine(&todo_list.buf, todo_file.buf) < 0) {
+	if (strbuf_read_file_or_whine(&old_todo.buf, rebase_path_todo_backup()) < 0) {
 		res = -1;
-		goto leave_check;
-	}
-	strbuf_release(&todo_file);
-	res = !!todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list);
-
-	/* Find commits in git-rebase-todo.backup yet unseen */
-	for (i = todo_list.nr - 1; i >= 0; i--) {
-		struct todo_item *item = todo_list.items + i;
-		struct commit *commit = item->commit;
-		if (commit && !*commit_seen_at(&commit_seen, commit)) {
-			strbuf_addf(&missing, " - %s %.*s\n",
-				    short_commit_name(commit),
-				    item->arg_len, item->arg);
-			*commit_seen_at(&commit_seen, commit) = 1;
-		}
+		goto out;
 	}
 
-	/* Warn about missing commits */
-	if (!missing.len)
-		goto leave_check;
-
-	if (check_level == MISSING_COMMIT_CHECK_ERROR)
-		advise_to_edit_todo = res = 1;
+	res = todo_list_check(&old_todo, &new_todo);
 
-	fprintf(stderr,
-		_("Warning: some commits may have been dropped accidentally.\n"
-		"Dropped commits (newer to older):\n"));
-
-	/* Make the list user-friendly and display */
-	fputs(missing.buf, stderr);
-	strbuf_release(&missing);
-
-	fprintf(stderr, _("To avoid this message, use \"drop\" to "
-		"explicitly remove a commit.\n\n"
-		"Use 'git config rebase.missingCommitsCheck' to change "
-		"the level of warnings.\n"
-		"The possible behaviours are: ignore, warn, error.\n\n"));
-
-leave_check:
-	clear_commit_seen(&commit_seen);
-	strbuf_release(&todo_file);
-	todo_list_release(&todo_list);
-
-	if (advise_to_edit_todo)
-		fprintf(stderr,
-			_("You can fix this with 'git rebase --edit-todo' "
-			  "and then run 'git rebase --continue'.\n"
-			  "Or you can abort the rebase with 'git rebase"
-			  " --abort'.\n"));
+out:
+	todo_list_release(&old_todo);
+	todo_list_release(&new_todo);
 
 	return res;
 }
@@ -4769,7 +4684,7 @@ int complete_action(struct replay_opts *opts, unsigned flags,
 
 	todo_list_release(&todo_list);
 
-	if (check_todo_list()) {
+	if (check_todo_list_from_file()) {
 		checkout_onto(opts, onto_name, onto, orig_head);
 		return -1;
 	}
diff --git a/sequencer.h b/sequencer.h
index c786dee543..fb8b85bf9e 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -63,12 +63,6 @@ struct replay_opts {
 };
 #define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }
 
-enum missing_commit_check_level {
-	MISSING_COMMIT_CHECK_IGNORE = 0,
-	MISSING_COMMIT_CHECK_WARN,
-	MISSING_COMMIT_CHECK_ERROR
-};
-
 int write_message(const void *buf, size_t len, const char *filename,
 		  int append_eol);
 
@@ -142,8 +136,7 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 
 int sequencer_add_exec_commands(const char *command);
 int transform_todos(unsigned flags);
-enum missing_commit_check_level get_missing_commit_check_level(void);
-int check_todo_list(void);
+int check_todo_list_from_file(void);
 int complete_action(struct replay_opts *opts, unsigned flags,
 		    const char *shortrevisions, const char *onto_name,
 		    const char *onto, const char *orig_head, const char *cmd,
-- 
2.19.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 04/15] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
  2018-10-07 19:54 [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
                   ` (2 preceding siblings ...)
  2018-10-07 19:54 ` [PATCH 03/15] sequencer: refactor check_todo_list() to work on a todo_list Alban Gruin
@ 2018-10-07 19:54 ` " Alban Gruin
  2018-10-11 11:25   ` Phillip Wood
  2018-10-07 19:54 ` [PATCH 05/15] sequencer: refactor rearrange_squash() " Alban Gruin
                   ` (11 subsequent siblings)
  15 siblings, 1 reply; 29+ messages in thread
From: Alban Gruin @ 2018-10-07 19:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

This refactors sequencer_add_exec_commands() to work on a todo_list to
avoid redundant reads and writes to the disk.

sequencer_add_exec_commands() still reads the todo list from the disk,
as it is needed by rebase -p.  todo_list_add_exec_commands() works on a
todo_list structure, and reparses it at the end.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 56 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 8dda61927c..6d998f21a4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4370,34 +4370,21 @@ 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,
+					const char *commands)
 {
-	const char *todo_file = rebase_path_todo();
-	struct todo_list todo_list = TODO_LIST_INIT;
-	struct strbuf *buf = &todo_list.buf;
+	struct strbuf *buf = &todo_list->buf;
 	size_t offset = 0, commands_len = strlen(commands);
 	int i, insert;
 
-	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);
-	}
-
 	/*
 	 * Insert <commands> after every pick. Here, fixup/squash chains
 	 * are considered part of the pick, so we insert the commands *after*
 	 * 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;
+	for (i = 0; i < todo_list->nr; i++) {
+		enum todo_command command = todo_list->items[i].command;
 
 		if (insert >= 0) {
 			/* skip fixup/squash chains */
@@ -4408,7 +4395,7 @@ int sequencer_add_exec_commands(const char *commands)
 				continue;
 			}
 			strbuf_insert(buf,
-				      todo_list.items[insert].offset_in_buf +
+				      todo_list->items[insert].offset_in_buf +
 				      offset, commands, commands_len);
 			offset += commands_len;
 			insert = -1;
@@ -4419,15 +4406,38 @@ int sequencer_add_exec_commands(const char *commands)
 	}
 
 	/* insert or append final <commands> */
-	if (insert >= 0 && insert < todo_list.nr)
-		strbuf_insert(buf, todo_list.items[insert].offset_in_buf +
+	if (insert >= 0 && insert < todo_list->nr)
+		strbuf_insert(buf, todo_list->items[insert].offset_in_buf +
 			      offset, commands, commands_len);
 	else if (insert >= 0 || !offset)
 		strbuf_add(buf, commands, commands_len);
 
-	i = write_message(buf->buf, buf->len, todo_file, 0);
+	if (todo_list_parse_insn_buffer(buf->buf, todo_list))
+		BUG("unusable todo list");}
+
+/*
+ * Add commands after pick and (series of) squash/fixup commands
+ * in the todo list.
+ */
+int sequencer_add_exec_commands(const char *commands)
+{
+	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_add_exec_commands(&todo_list, commands);
+	res = write_message(todo_list.buf.buf, todo_list.buf.len, todo_file, 0);
 	todo_list_release(&todo_list);
-	return i;
+
+	return res;
 }
 
 int transform_todos(unsigned flags)
-- 
2.19.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 05/15] sequencer: refactor rearrange_squash() to work on a todo_list
  2018-10-07 19:54 [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
                   ` (3 preceding siblings ...)
  2018-10-07 19:54 ` [PATCH 04/15] sequencer: refactor sequencer_add_exec_commands() " Alban Gruin
@ 2018-10-07 19:54 ` " Alban Gruin
  2018-10-07 19:54 ` [PATCH 06/15] sequencer: refactor transform_todos() " Alban Gruin
                   ` (10 subsequent siblings)
  15 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2018-10-07 19:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

This refactors rearrange_squash() to work on a todo_list to avoid
redundant reads and writes.  The function is renamed
todo_list_rearrange_squash().

As rebase -p still need to check the todo list from the disk, a new
function is introduced, rearrange_squash_in_todo_file().

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase--interactive.c |  2 +-
 sequencer.c                   | 73 +++++++++++++++++++++--------------
 sequencer.h                   |  2 +-
 3 files changed, 46 insertions(+), 31 deletions(-)

diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
index ea1f93ccb6..8deef126d1 100644
--- a/builtin/rebase--interactive.c
+++ b/builtin/rebase--interactive.c
@@ -258,7 +258,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 		ret = check_todo_list_from_file();
 		break;
 	case REARRANGE_SQUASH:
-		ret = rearrange_squash();
+		ret = rearrange_squash_in_todo_file();
 		break;
 	case ADD_EXEC:
 		ret = sequencer_add_exec_commands(cmd);
diff --git a/sequencer.c b/sequencer.c
index 6d998f21a4..8a6176b3d0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4632,7 +4632,7 @@ int complete_action(struct replay_opts *opts, unsigned flags,
 	    write_message("noop\n", 5, todo_file, 0))
 		return -1;
 
-	if (autosquash && rearrange_squash())
+	if (autosquash && rearrange_squash_in_todo_file())
 		return -1;
 
 	if (cmd && *cmd)
@@ -4738,22 +4738,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(void)
+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;
 	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(todo_list.buf.buf, &todo_list) < 0) {
-		todo_list_release(&todo_list);
-		return -1;
-	}
-
 	init_commit_todo_item(&commit_todo);
 	/*
 	 * The hashmap maps onelines to the respective todo list index.
@@ -4765,13 +4756,13 @@ int rearrange_squash(void)
 	 * 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;
@@ -4784,7 +4775,6 @@ int rearrange_squash(void)
 		}
 
 		if (is_fixup(item->command)) {
-			todo_list_release(&todo_list);
 			clear_commit_todo_item(&commit_todo);
 			return error(_("the script was already rearranged."));
 		}
@@ -4819,7 +4809,7 @@ int rearrange_squash(void)
 				 *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 subject */
 				for (i2 = 0; i2 < i; i2++)
@@ -4832,7 +4822,7 @@ int rearrange_squash(void)
 		}
 		if (i2 >= 0) {
 			rearranged = 1;
-			todo_list.items[i].command =
+			todo_list->items[i].command =
 				starts_with(subject, "fixup!") ?
 				TODO_FIXUP : TODO_SQUASH;
 			if (next[i2] < 0)
@@ -4852,8 +4842,8 @@ int rearrange_squash(void)
 	if (rearranged) {
 		struct strbuf buf = STRBUF_INIT;
 
-		for (i = 0; i < todo_list.nr; i++) {
-			enum todo_command command = todo_list.items[i].command;
+		for (i = 0; i < todo_list->nr; i++) {
+			enum todo_command command = todo_list->items[i].command;
 			int cur = i;
 
 			/*
@@ -4865,12 +4855,12 @@ int rearrange_squash(void)
 
 			while (cur >= 0) {
 				const char *bol =
-					get_item_line(&todo_list, cur);
+					get_item_line(todo_list, cur);
 				const char *eol =
-					get_item_line(&todo_list, cur + 1);
+					get_item_line(todo_list, cur + 1);
 
 				/* replace 'pick', by 'fixup' or 'squash' */
-				command = todo_list.items[cur].command;
+				command = todo_list->items[cur].command;
 				if (is_fixup(command)) {
 					strbuf_addstr(&buf,
 						todo_command_info[command].str);
@@ -4883,18 +4873,43 @@ int rearrange_squash(void)
 			}
 		}
 
-		res = rewrite_file(todo_file, buf.buf, buf.len);
+		strbuf_reset(&todo_list->buf);
+		strbuf_add(&todo_list->buf, buf.buf, buf.len);
 		strbuf_release(&buf);
 	}
 
 	free(next);
 	free(tail);
-	for (i = 0; i < todo_list.nr; i++)
+	for (i = 0; i < todo_list->nr; i++)
 		free(subjects[i]);
 	free(subjects);
 	hashmap_free(&subject2item, 1);
-	todo_list_release(&todo_list);
 
 	clear_commit_todo_item(&commit_todo);
+
+	if (todo_list_parse_insn_buffer(todo_list->buf.buf, todo_list))
+		BUG("unusable todo list");
+
+	return 0;
+}
+
+int rearrange_squash_in_todo_file(void)
+{
+	const char *todo_file = rebase_path_todo();
+	struct todo_list todo_list = TODO_LIST_INIT;
+	int res = 0;
+
+	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;
+	}
+
+	res = todo_list_rearrange_squash(&todo_list);
+	if (!res)
+		res = rewrite_file(todo_file, todo_list.buf.buf, todo_list.buf.len);
+
+	todo_list_release(&todo_list);
 	return res;
 }
diff --git a/sequencer.h b/sequencer.h
index fb8b85bf9e..7f5668500f 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -141,7 +141,7 @@ int complete_action(struct replay_opts *opts, unsigned flags,
 		    const char *shortrevisions, const char *onto_name,
 		    const char *onto, const char *orig_head, const char *cmd,
 		    unsigned autosquash);
-int rearrange_squash(void);
+int rearrange_squash_in_todo_file(void);
 
 extern const char sign_off_header[];
 
-- 
2.19.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 06/15] sequencer: refactor transform_todos() to work on a todo_list
  2018-10-07 19:54 [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
                   ` (4 preceding siblings ...)
  2018-10-07 19:54 ` [PATCH 05/15] sequencer: refactor rearrange_squash() " Alban Gruin
@ 2018-10-07 19:54 ` " Alban Gruin
  2018-10-07 19:54 ` [PATCH 07/15] sequencer: make sequencer_make_script() write its script to a strbuf Alban Gruin
                   ` (9 subsequent siblings)
  15 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2018-10-07 19:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

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().

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 <alban.gruin@gmail.com>
---
 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 8deef126d1..f827e53f05 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_from_file();
diff --git a/rebase-interactive.c b/rebase-interactive.c
index ef8540245d..7c7f720a3d 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -92,7 +92,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);
@@ -108,7 +108,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 8a6176b3d0..30a7fe3958 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4440,23 +4440,13 @@ int sequencer_add_exec_commands(const char *commands)
 	return res;
 }
 
-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);
@@ -4492,9 +4482,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;
 }
 
 int check_todo_list_from_file(void)
@@ -4670,7 +4684,7 @@ int complete_action(struct replay_opts *opts, unsigned flags,
 		return error(_("could not copy '%s' to '%s'."), todo_file,
 			     rebase_path_todo_backup());
 
-	if (transform_todos(flags | TODO_LIST_SHORTEN_IDS))
+	if (transform_todo_file(flags | TODO_LIST_SHORTEN_IDS))
 		return error(_("could not transform the todo list"));
 
 	strbuf_reset(buf);
@@ -4699,7 +4713,7 @@ int complete_action(struct replay_opts *opts, unsigned flags,
 		return -1;
 	}
 
-	if (transform_todos(flags & ~(TODO_LIST_SHORTEN_IDS)))
+	if (transform_todo_file(flags & ~(TODO_LIST_SHORTEN_IDS)))
 		return error(_("could not transform the todo list"));
 
 	if (opts->allow_ff && skip_unnecessary_picks(&oid))
diff --git a/sequencer.h b/sequencer.h
index 7f5668500f..e1faca7884 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -112,6 +112,7 @@ struct todo_list {
 #define TODO_LIST_INIT { STRBUF_INIT }
 
 int todo_list_parse_insn_buffer(char *buf, struct todo_list *todo_list);
+void todo_list_transform(struct todo_list *todo_list, unsigned flags);
 void todo_list_release(struct todo_list *todo_list);
 
 /* Call this to setup defaults before parsing command line options */
@@ -135,7 +136,7 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 			  unsigned flags);
 
 int sequencer_add_exec_commands(const char *command);
-int transform_todos(unsigned flags);
+int transform_todo_file(unsigned flags);
 int check_todo_list_from_file(void);
 int complete_action(struct replay_opts *opts, unsigned flags,
 		    const char *shortrevisions, const char *onto_name,
-- 
2.19.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 07/15] sequencer: make sequencer_make_script() write its script to a strbuf
  2018-10-07 19:54 [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
                   ` (5 preceding siblings ...)
  2018-10-07 19:54 ` [PATCH 06/15] sequencer: refactor transform_todos() " Alban Gruin
@ 2018-10-07 19:54 ` Alban Gruin
  2018-10-12 10:01   ` SZEDER Gábor
  2018-10-07 19:54 ` [PATCH 08/15] sequencer: change complete_action() to use the refactored functions Alban Gruin
                   ` (8 subsequent siblings)
  15 siblings, 1 reply; 29+ messages in thread
From: Alban Gruin @ 2018-10-07 19:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

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 <alban.gruin@gmail.com>
---
 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 30a7fe3958..dfb8d1c974 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4083,7 +4083,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;
@@ -4230,7 +4230,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;
 
@@ -4240,9 +4240,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_addf(out, "\n");
 
 		while (oidset_contains(&interesting, &commit->object.oid) &&
 		       !oidset_contains(&shown, &commit->object.oid)) {
@@ -4255,8 +4255,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;
 
@@ -4269,12 +4269,12 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 					       &state);
 
 			if (!to || !strcmp(to, "onto"))
-				fprintf(out, "%s onto\n", cmd_reset);
+				strbuf_addf(out, "%s onto\n", cmd_reset);
 			else {
 				strbuf_reset(&oneline);
 				pretty_print_commit(pp, commit, &oneline);
-				fprintf(out, "%s %s # %s\n",
-					cmd_reset, to, oneline.buf);
+				strbuf_addf(out, "%s %s # %s\n",
+					    cmd_reset, to, oneline.buf);
 			}
 		}
 
@@ -4283,11 +4283,11 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 			entry = oidmap_get(&commit2todo, oid);
 			/* only show if not already upstream */
 			if (entry)
-				fprintf(out, "%s\n", entry->string);
+				strbuf_addf(out, "%s\n", entry->string);
 			entry = oidmap_get(&state.commit2label, oid);
 			if (entry)
-				fprintf(out, "%s %s\n",
-					cmd_label, entry->string);
+				strbuf_addf(out, "%s %s\n",
+					    cmd_label, entry->string);
 			oidset_insert(&shown, oid);
 		}
 
@@ -4309,12 +4309,11 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 	return 0;
 }
 
-int sequencer_make_script(FILE *out, int argc, const char **argv,
+int sequencer_make_script(struct strbuf *out, int argc, const char **argv,
 			  unsigned flags)
 {
 	char *format = NULL;
 	struct pretty_print_context pp = {0};
-	struct strbuf buf = STRBUF_INIT;
 	struct rev_info revs;
 	struct commit *commit;
 	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
@@ -4357,16 +4356,13 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 
 		if (!is_empty && (commit->object.flags & PATCHSAME))
 			continue;
-		strbuf_reset(&buf);
 		if (!keep_empty && is_empty)
-			strbuf_addf(&buf, "%c ", comment_line_char);
-		strbuf_addf(&buf, "%s %s ", insn,
+			strbuf_addf(out, "%c ", comment_line_char);
+		strbuf_addf(out, "%s %s ", insn,
 			    oid_to_hex(&commit->object.oid));
-		pretty_print_commit(&pp, commit, &buf);
-		strbuf_addch(&buf, '\n');
-		fputs(buf.buf, out);
+		pretty_print_commit(&pp, commit, out);
+		strbuf_addch(out, '\n');
 	}
-	strbuf_release(&buf);
 	return 0;
 }
 
diff --git a/sequencer.h b/sequencer.h
index e1faca7884..21d9ba09ab 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -132,7 +132,7 @@ int sequencer_remove_state(struct replay_opts *opts);
  * commits should be rebased onto the new base, this flag needs to be passed.
  */
 #define TODO_LIST_REBASE_COUSINS (1U << 4)
-int sequencer_make_script(FILE *out, int argc, const char **argv,
+int sequencer_make_script(struct strbuf *out, int argc, const char **argv,
 			  unsigned flags);
 
 int sequencer_add_exec_commands(const char *command);
-- 
2.19.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 08/15] sequencer: change complete_action() to use the refactored functions
  2018-10-07 19:54 [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
                   ` (6 preceding siblings ...)
  2018-10-07 19:54 ` [PATCH 07/15] sequencer: make sequencer_make_script() write its script to a strbuf Alban Gruin
@ 2018-10-07 19:54 ` Alban Gruin
  2018-10-11 13:51   ` Phillip Wood
  2018-10-07 19:54 ` [PATCH 09/15] sequencer: refactor skip_unnecessary_picks() to work on a todo_list Alban Gruin
                   ` (7 subsequent siblings)
  15 siblings, 1 reply; 29+ messages in thread
From: Alban Gruin @ 2018-10-07 19:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

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.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase--interactive.c | 13 +-----
 sequencer.c                   | 76 +++++++++++++++++------------------
 sequencer.h                   |  2 +-
 3 files changed, 38 insertions(+), 53 deletions(-)

diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
index eef1ff2e83..0700339f90 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,15 +100,13 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
 	ret = sequencer_make_script(&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(opts, flags, shortrevisions, onto_name, onto,
-				      head_hash, cmd, autosquash);
+				      head_hash, cmd, autosquash, &todo_list);
 	}
 
 	free(revisions);
diff --git a/sequencer.c b/sequencer.c
index dfb8d1c974..b37935e5ab 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4624,93 +4624,89 @@ static int skip_unnecessary_picks(struct object_id *output_oid)
 	return 0;
 }
 
+static int todo_list_rearrange_squash(struct todo_list *todo_list);
+
 int complete_action(struct replay_opts *opts, unsigned flags,
 		    const char *shortrevisions, const char *onto_name,
 		    const char *onto, const char *orig_head, const char *cmd,
-		    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;
+	int command_count;
 
 	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)
+		strbuf_add(buf, "noop\n", 5);
+
+	if (todo_list_parse_insn_buffer(buf->buf, todo_list))
+		BUG("unusable todo list");
 
-	if (autosquash && rearrange_squash_in_todo_file())
+	if (autosquash && todo_list_rearrange_squash(todo_list))
 		return -1;
 
 	if (cmd && *cmd)
-		sequencer_add_exec_commands(cmd);
+		todo_list_add_exec_commands(todo_list, cmd);
 
-	if (strbuf_read_file(buf, todo_file, 0) < 0)
-		return error_errno(_("could not read '%s'."), todo_file);
-
-	if (todo_list_parse_insn_buffer(buf->buf, &todo_list)) {
-		todo_list_release(&todo_list);
-		return error(_("unusable todo list: '%s'"), todo_file);
-	}
-
-	if (count_commands(&todo_list) == 0) {
+	command_count = count_commands(todo_list);
+	if (command_count == 0) {
 		apply_autostash(opts);
 		sequencer_remove_state(opts);
-		todo_list_release(&todo_list);
 
 		return error(_("nothing to do"));
 	}
 
+	todo_list_transform(todo_list, flags | TODO_LIST_SHORTEN_IDS);
+
 	strbuf_addch(buf, '\n');
 	strbuf_commented_addf(buf, Q_("Rebase %s onto %s (%d command)",
 				      "Rebase %s onto %s (%d commands)",
-				      count_commands(&todo_list)),
-			      shortrevisions, shortonto, count_commands(&todo_list));
+				      command_count),
+			      shortrevisions, shortonto, command_count);
 	append_todo_help(0, flags & TODO_LIST_KEEP_EMPTY, buf);
 
-	if (write_message(buf->buf, buf->len, todo_file, 0)) {
-		todo_list_release(&todo_list);
-		return -1;
-	}
+	if (write_message(buf->buf, buf->len, todo_file, 0))
+		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 (transform_todo_file(flags | TODO_LIST_SHORTEN_IDS))
-		return error(_("could not transform the todo list"));
-
-	strbuf_reset(buf);
-
-	if (launch_sequence_editor(todo_file, buf, NULL)) {
+	if (launch_sequence_editor(todo_file, &new_todo.buf, NULL)) {
 		apply_autostash(opts);
 		sequencer_remove_state(opts);
-		todo_list_release(&todo_list);
 
 		return -1;
 	}
 
-	strbuf_stripspace(buf, 1);
-	if (buf->len == 0) {
+	strbuf_stripspace(&new_todo.buf, 1);
+	if (new_todo.buf.len == 0) {
 		apply_autostash(opts);
 		sequencer_remove_state(opts);
-		todo_list_release(&todo_list);
+		todo_list_release(&new_todo);
 
 		return error(_("nothing to do"));
 	}
 
-	todo_list_release(&todo_list);
-
-	if (check_todo_list_from_file()) {
+	if (todo_list_check(todo_list, &new_todo)) {
 		checkout_onto(opts, onto_name, onto, orig_head);
+		todo_list_release(&new_todo);
+
 		return -1;
 	}
 
-	if (transform_todo_file(flags & ~(TODO_LIST_SHORTEN_IDS)))
-		return error(_("could not transform the todo list"));
+	todo_list_transform(&new_todo, flags & ~(TODO_LIST_SHORTEN_IDS));
+
+	if (rewrite_file(todo_file, new_todo.buf.buf, new_todo.buf.len) < 0) {
+		todo_list_release(&new_todo);
+		return error_errno(_("could not write '%s'"), todo_file);
+	}
+
+	todo_list_release(&new_todo);
 
 	if (opts->allow_ff && skip_unnecessary_picks(&oid))
 		return error(_("could not skip unnecessary pick commands"));
diff --git a/sequencer.h b/sequencer.h
index 21d9ba09ab..5bd3b79282 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -141,7 +141,7 @@ int check_todo_list_from_file(void);
 int complete_action(struct replay_opts *opts, unsigned flags,
 		    const char *shortrevisions, const char *onto_name,
 		    const char *onto, const char *orig_head, const char *cmd,
-		    unsigned autosquash);
+		    unsigned autosquash, struct todo_list *todo_list);
 int rearrange_squash_in_todo_file(void);
 
 extern const char sign_off_header[];
-- 
2.19.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 09/15] sequencer: refactor skip_unnecessary_picks() to work on a todo_list
  2018-10-07 19:54 [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
                   ` (7 preceding siblings ...)
  2018-10-07 19:54 ` [PATCH 08/15] sequencer: change complete_action() to use the refactored functions Alban Gruin
@ 2018-10-07 19:54 ` Alban Gruin
  2018-10-07 19:54 ` [PATCH 10/15] rebase-interactive: use todo_list_transform() in edit_todo_list() Alban Gruin
                   ` (6 subsequent siblings)
  15 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2018-10-07 19:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

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().

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 56 +++++++++++++++--------------------------------------
 1 file changed, 16 insertions(+), 40 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index b37935e5ab..a432b64048 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4545,38 +4545,20 @@ static int rewrite_file(const char *path, const char *buf, size_t len)
 }
 
 /* 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;
-	}
-
-	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));
 		}
@@ -4590,37 +4572,29 @@ 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);
+		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 (write_in_full(fd, todo_list->buf.buf, offset) < 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;
-		}
+		strbuf_splice(&todo_list->buf, 0, offset, NULL, 0);
 
-		todo_list.current = i;
-		if (is_fixup(peek_command(&todo_list, 0)))
-			record_in_rewritten(output_oid, peek_command(&todo_list, 0));
+		todo_list->current = i;
+		if (is_fixup(peek_command(todo_list, 0)))
+			record_in_rewritten(output_oid, peek_command(todo_list, 0));
 	}
 
-	todo_list_release(&todo_list);
-
 	return 0;
 }
 
@@ -4701,6 +4675,11 @@ int complete_action(struct replay_opts *opts, unsigned flags,
 
 	todo_list_transform(&new_todo, flags & ~(TODO_LIST_SHORTEN_IDS));
 
+	if (opts->allow_ff && skip_unnecessary_picks(&new_todo, &oid)) {
+		todo_list_release(&new_todo);
+		return error(_("could not skip unnecessary pick commands"));
+	}
+
 	if (rewrite_file(todo_file, new_todo.buf.buf, new_todo.buf.len) < 0) {
 		todo_list_release(&new_todo);
 		return error_errno(_("could not write '%s'"), todo_file);
@@ -4708,12 +4687,9 @@ int complete_action(struct replay_opts *opts, unsigned flags,
 
 	todo_list_release(&new_todo);
 
-	if (opts->allow_ff && skip_unnecessary_picks(&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("rebase", "", 1, 1))
 		return -1;
 
-- 
2.19.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 10/15] rebase-interactive: use todo_list_transform() in edit_todo_list()
  2018-10-07 19:54 [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
                   ` (8 preceding siblings ...)
  2018-10-07 19:54 ` [PATCH 09/15] sequencer: refactor skip_unnecessary_picks() to work on a todo_list Alban Gruin
@ 2018-10-07 19:54 ` Alban Gruin
  2018-10-11 15:16   ` Phillip Wood
  2018-10-07 19:54 ` [PATCH 11/15] rebase-interactive: append_todo_help() changes Alban Gruin
                   ` (5 subsequent siblings)
  15 siblings, 1 reply; 29+ messages in thread
From: Alban Gruin @ 2018-10-07 19:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

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_transform()
instead.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 rebase-interactive.c | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/rebase-interactive.c b/rebase-interactive.c
index 7c7f720a3d..f42d48e192 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -78,39 +78,37 @@ 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);
-		return -1;
-	}
-
-	strbuf_release(&buf);
+	strbuf_stripspace(&todo_list.buf, 1);
+	if (!todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list))
+		todo_list_transform(&todo_list, 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);
+	append_todo_help(1, 0, &todo_list.buf);
 
-	append_todo_help(1, 0, &buf);
-	if (write_message(buf.buf, buf.len, todo_file, 0)) {
-		strbuf_release(&buf);
+	if (write_message(todo_list.buf.buf, todo_list.buf.len, todo_file, 0)) {
+		todo_list_release(&todo_list);
 		return -1;
 	}
 
-	strbuf_release(&buf);
-
-	if (launch_sequence_editor(todo_file, NULL, NULL))
+	strbuf_reset(&todo_list.buf);
+	if (launch_sequence_editor(todo_file, &todo_list.buf, NULL)) {
+		todo_list_release(&todo_list);
 		return -1;
+	}
 
-	transform_todo_file(flags & ~(TODO_LIST_SHORTEN_IDS));
+	if (!todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list)) {
+		todo_list_transform(&todo_list, flags & ~(TODO_LIST_SHORTEN_IDS));
+		res = write_message(todo_list.buf.buf, todo_list.buf.len, todo_file, 0);
+	}
 
-	return 0;
+	todo_list_release(&todo_list);
+	return res;
 }
 
 define_commit_slab(commit_seen, unsigned char);
-- 
2.19.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 11/15] rebase-interactive: append_todo_help() changes
  2018-10-07 19:54 [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
                   ` (9 preceding siblings ...)
  2018-10-07 19:54 ` [PATCH 10/15] rebase-interactive: use todo_list_transform() in edit_todo_list() Alban Gruin
@ 2018-10-07 19:54 ` Alban Gruin
  2018-10-07 19:54 ` [PATCH 12/15] rebase-interactive: rewrite edit_todo_list() to handle the initial edit Alban Gruin
                   ` (4 subsequent siblings)
  15 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2018-10-07 19:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

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 <alban.gruin@gmail.com>
---
 rebase-interactive.c | 14 ++++++++++++--
 rebase-interactive.h |  3 ++-
 sequencer.c          |  8 ++------
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/rebase-interactive.c b/rebase-interactive.c
index f42d48e192..7168d56d17 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 <commit> 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));
 
@@ -89,7 +99,7 @@ int edit_todo_list(unsigned flags)
 	if (!todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list))
 		todo_list_transform(&todo_list, flags | TODO_LIST_SHORTEN_IDS);
 
-	append_todo_help(1, 0, &todo_list.buf);
+	append_todo_help(flags, 0, NULL, NULL, &todo_list.buf);
 
 	if (write_message(todo_list.buf.buf, todo_list.buf.len, todo_file, 0)) {
 		todo_list_release(&todo_list);
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 a432b64048..94d3058359 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4636,12 +4636,8 @@ int complete_action(struct replay_opts *opts, unsigned flags,
 
 	todo_list_transform(todo_list, flags | TODO_LIST_SHORTEN_IDS);
 
-	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(0, flags & TODO_LIST_KEEP_EMPTY, buf);
+	append_todo_help(flags & TODO_LIST_KEEP_EMPTY, command_count,
+			 shortrevisions, shortonto, buf);
 
 	if (write_message(buf->buf, buf->len, todo_file, 0))
 		return error_errno(_("could not write '%s'"), todo_file);
-- 
2.19.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 12/15] rebase-interactive: rewrite edit_todo_list() to handle the initial edit
  2018-10-07 19:54 [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
                   ` (10 preceding siblings ...)
  2018-10-07 19:54 ` [PATCH 11/15] rebase-interactive: append_todo_help() changes Alban Gruin
@ 2018-10-07 19:54 ` Alban Gruin
  2018-10-07 19:54 ` [PATCH 13/15] sequencer: use edit_todo_list() in complete_action() Alban Gruin
                   ` (3 subsequent siblings)
  15 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2018-10-07 19:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

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
work if the old todo list has invalid commands.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase--interactive.c | 22 +++++++++++++++-
 rebase-interactive.c          | 49 ++++++++++++++++++-----------------
 rebase-interactive.h          |  4 ++-
 sequencer.c                   |  3 +--
 sequencer.h                   |  1 +
 5 files changed, 51 insertions(+), 28 deletions(-)

diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
index 0700339f90..264e940b47 100644
--- a/builtin/rebase--interactive.c
+++ b/builtin/rebase--interactive.c
@@ -13,6 +13,26 @@ 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) &&
+	    write_message(new_todo.buf.buf, new_todo.buf.len, todo_file, 0) < 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)
@@ -231,7 +251,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 7168d56d17..6ee60ac03f 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -86,39 +86,40 @@ 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;
+	unsigned initial = shortrevisions && shortonto;
 
-	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
-		return error_errno(_("could not read '%s'."), todo_file);
+	if (initial || !todo_list_parse_insn_buffer(todo_list->buf.buf, todo_list))
+		todo_list_transform(todo_list, flags | TODO_LIST_SHORTEN_IDS);
 
-	strbuf_stripspace(&todo_list.buf, 1);
-	if (!todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list))
-		todo_list_transform(&todo_list, flags | TODO_LIST_SHORTEN_IDS);
+	if (initial)
+		append_todo_help(flags & TODO_LIST_KEEP_EMPTY, command_count,
+				 shortrevisions, shortonto, &todo_list->buf);
+	else
+		append_todo_help(flags, 0, NULL, NULL, &todo_list->buf);
 
-	append_todo_help(flags, 0, NULL, NULL, &todo_list.buf);
+	if (write_message(todo_list->buf.buf, todo_list->buf.len, todo_file, 0))
+		return error_errno(_("could not write '%s''"), todo_file);
 
-	if (write_message(todo_list.buf.buf, todo_list.buf.len, todo_file, 0)) {
-		todo_list_release(&todo_list);
-		return -1;
-	}
+	if (initial && copy_file(rebase_path_todo_backup(), todo_file, 0666))
+		return error(_("could not copy '%s' to '%s'."), todo_file,
+			     rebase_path_todo_backup());
 
-	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)) {
-		todo_list_transform(&todo_list, flags & ~(TODO_LIST_SHORTEN_IDS));
-		res = write_message(todo_list.buf.buf, todo_list.buf.len, todo_file, 0);
-	}
+	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(new_todo->buf.buf, new_todo))
+		todo_list_transform(new_todo, flags & ~(TODO_LIST_SHORTEN_IDS));
+
+	return 0;
 }
 
 define_commit_slab(commit_seen, unsigned char);
diff --git a/rebase-interactive.h b/rebase-interactive.h
index 61858f3a02..83cde455e6 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -4,7 +4,9 @@
 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 edit_todo_list(struct todo_list *todo_list, struct todo_list *new_todo,
+		   unsigned flags, int command_count,
+		   const char *shortrevisions, const char *shortonto);
 int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
 
 #endif
diff --git a/sequencer.c b/sequencer.c
index 94d3058359..bfcbe8239b 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 5bd3b79282..fa84918c55 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -9,6 +9,7 @@ struct commit;
 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)
 
-- 
2.19.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 13/15] sequencer: use edit_todo_list() in complete_action()
  2018-10-07 19:54 [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
                   ` (11 preceding siblings ...)
  2018-10-07 19:54 ` [PATCH 12/15] rebase-interactive: rewrite edit_todo_list() to handle the initial edit Alban Gruin
@ 2018-10-07 19:54 ` Alban Gruin
  2018-10-07 19:54 ` [PATCH 14/15] sequencer: fix a call to error() in transform_todo_file() Alban Gruin
                   ` (2 subsequent siblings)
  15 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2018-10-07 19:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

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 <alban.gruin@gmail.com>
---
 sequencer.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index bfcbe8239b..93b9b40f66 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4608,7 +4608,7 @@ int complete_action(struct replay_opts *opts, unsigned flags,
 	struct todo_list new_todo = TODO_LIST_INIT;
 	struct strbuf *buf = &todo_list->buf;
 	struct object_id oid;
-	int command_count;
+	int command_count, res;
 
 	get_oid(onto, &oid);
 	shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);
@@ -4633,27 +4633,16 @@ int complete_action(struct replay_opts *opts, unsigned flags,
 		return error(_("nothing to do"));
 	}
 
-	todo_list_transform(todo_list, flags | TODO_LIST_SHORTEN_IDS);
-
-	append_todo_help(flags & TODO_LIST_KEEP_EMPTY, command_count,
-			 shortrevisions, shortonto, buf);
-
-	if (write_message(buf->buf, buf->len, todo_file, 0))
-		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(todo_list, &new_todo, flags,
+			     command_count, shortrevisions, shortonto);
+	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);
@@ -4668,8 +4657,6 @@ int complete_action(struct replay_opts *opts, unsigned flags,
 		return -1;
 	}
 
-	todo_list_transform(&new_todo, flags & ~(TODO_LIST_SHORTEN_IDS));
-
 	if (opts->allow_ff && skip_unnecessary_picks(&new_todo, &oid)) {
 		todo_list_release(&new_todo);
 		return error(_("could not skip unnecessary pick commands"));
-- 
2.19.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 14/15] sequencer: fix a call to error() in transform_todo_file()
  2018-10-07 19:54 [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
                   ` (12 preceding siblings ...)
  2018-10-07 19:54 ` [PATCH 13/15] sequencer: use edit_todo_list() in complete_action() Alban Gruin
@ 2018-10-07 19:54 ` Alban Gruin
  2018-10-07 19:54 ` [PATCH 15/15] rebase--interactive: move transform_todo_file() to rebase--interactive.c Alban Gruin
  2018-10-07 20:51 ` [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
  15 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2018-10-07 19:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

This replaces a call to error() by a call to error_errno() after writing
the content of the todo list to the disk in transform_todo_file().

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 93b9b40f66..65bf251ba5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4421,7 +4421,7 @@ int sequencer_add_exec_commands(const char *commands)
 	int res;
 
 	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
-		return error(_("could not read '%s'."), todo_file);
+		return error_errno(_("could not read '%s'."), todo_file);
 
 	if (todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list)) {
 		todo_list_release(&todo_list);
-- 
2.19.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 15/15] rebase--interactive: move transform_todo_file() to rebase--interactive.c
  2018-10-07 19:54 [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
                   ` (13 preceding siblings ...)
  2018-10-07 19:54 ` [PATCH 14/15] sequencer: fix a call to error() in transform_todo_file() Alban Gruin
@ 2018-10-07 19:54 ` Alban Gruin
  2018-10-07 20:51 ` [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
  15 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2018-10-07 19:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

As transform_todo_file() is only needed inside of rebase--interactive.c,
it is moved there from sequencer.c.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase--interactive.c | 21 +++++++++++++++++++++
 sequencer.c                   | 21 ---------------------
 sequencer.h                   |  1 -
 3 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
index 264e940b47..50b5c25402 100644
--- a/builtin/rebase--interactive.c
+++ b/builtin/rebase--interactive.c
@@ -33,6 +33,27 @@ 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(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 res;
+}
+
 static int get_revision_ranges(const char *upstream, const char *onto,
 			       const char **head_hash,
 			       char **revisions, char **shortrevisions)
diff --git a/sequencer.c b/sequencer.c
index 65bf251ba5..e837e52b64 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4485,27 +4485,6 @@ void todo_list_transform(struct todo_list *todo_list, unsigned flags)
 		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 res;
-}
-
 int check_todo_list_from_file(void)
 {
 	struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
diff --git a/sequencer.h b/sequencer.h
index fa84918c55..a4b0113206 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -137,7 +137,6 @@ int sequencer_make_script(struct strbuf *out, int argc, const char **argv,
 			  unsigned flags);
 
 int sequencer_add_exec_commands(const char *command);
-int transform_todo_file(unsigned flags);
 int check_todo_list_from_file(void);
 int complete_action(struct replay_opts *opts, unsigned flags,
 		    const char *shortrevisions, const char *onto_name,
-- 
2.19.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 00/15] sequencer: refactor functions working on a todo_list
  2018-10-07 19:54 [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
                   ` (14 preceding siblings ...)
  2018-10-07 19:54 ` [PATCH 15/15] rebase--interactive: move transform_todo_file() to rebase--interactive.c Alban Gruin
@ 2018-10-07 20:51 ` Alban Gruin
  15 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2018-10-07 20:51 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano

Le 07/10/2018 à 21:54, Alban Gruin a écrit :
> 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.
> 

And it’s based on the 8th version of my patch series “rebase -i: rewrite
in C”.


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 03/15] sequencer: refactor check_todo_list() to work on a todo_list
  2018-10-07 19:54 ` [PATCH 03/15] sequencer: refactor check_todo_list() to work on a todo_list Alban Gruin
@ 2018-10-11 11:23   ` Phillip Wood
  0 siblings, 0 replies; 29+ messages in thread
From: Phillip Wood @ 2018-10-11 11:23 UTC (permalink / raw)
  To: Alban Gruin, git; +Cc: Johannes Schindelin, Junio C Hamano

Hi Alban

I like the direction that this series is going

On 07/10/2018 20:54, Alban Gruin wrote:
> 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().
> 
> 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.

After this commit we still use check_todo_list_from_file() even without 
'-p', it would be clearer if this (and the following commits) mentioned 
that the new function will be wired up later.

> 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 <alban.gruin@gmail.com>
> ---
>   builtin/rebase--interactive.c |   2 +-
>   rebase-interactive.c          | 106 ++++++++++++++++++++++++++++++++-
>   rebase-interactive.h          |   1 +
>   sequencer.c                   | 109 ++++------------------------------
>   sequencer.h                   |   9 +--
>   5 files changed, 120 insertions(+), 107 deletions(-)
> 
> diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
> index a2ab68ed06..ea1f93ccb6 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_todos(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 0f4119cbae..ef8540245d 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,83 @@ 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 advise_to_edit_todo = 0, res = 0, i;
> +	struct commit_seen commit_seen;
> +
> +	init_commit_seen(&commit_seen);
> +
> +	res = todo_list_parse_insn_buffer(old_todo->buf.buf, old_todo);

While it made sense to parse the old and new todo lists here when they 
were being loaded from a file, I think it is confusing to keep that 
behavior now it is being passed struct todo_lists. When we get to patch 
8 the newly read commit list is passed to this function without being 
parsed which confused me until I came back and check what was going on here.

Best Wishes

Phillip

> +	if (!res)
> +		res = todo_list_parse_insn_buffer(new_todo->buf.buf, new_todo);
> +	if (res) {
> +		advise_to_edit_todo = res;
> +		goto leave_check;
> +	}
> +
> +	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)
> +		advise_to_edit_todo = res = 1;
> +
> +	fprintf(stderr,
> +		_("Warning: some commits may have been dropped accidentally.\n"
> +		"Dropped commits (newer to older):\n"));
> +
> +	/* Make the list user-friendly and display */
> +	fputs(missing.buf, stderr);
> +	strbuf_release(&missing);
> +
> +	fprintf(stderr, _("To avoid this message, use \"drop\" to "
> +		"explicitly remove a commit.\n\n"
> +		"Use 'git config rebase.missingCommitsCheck' to change "
> +		"the level of warnings.\n"
> +		"The possible behaviours are: ignore, warn, error.\n\n"));
> +
> +leave_check:
> +	clear_commit_seen(&commit_seen);
> +
> +	if (advise_to_edit_todo)
> +		fprintf(stderr,
> +			_("You can fix this with 'git rebase --edit-todo' "
> +			  "and then run 'git rebase --continue'.\n"
> +			  "Or you can abort the rebase with 'git rebase"
> +			  " --abort'.\n"));
> +
> +	return res;
> +}
> diff --git a/rebase-interactive.h b/rebase-interactive.h
> index 971da03776..6bc7bc315d 100644
> --- a/rebase-interactive.h
> +++ b/rebase-interactive.h
> @@ -4,5 +4,6 @@
>   void append_todo_help(unsigned edit_todo, unsigned keep_empty,
>   		      struct strbuf *buf);
>   int edit_todo_list(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 bb8ca2477f..8dda61927c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4487,111 +4487,26 @@ int transform_todos(unsigned flags)
>   	return i;
>   }
>   
> -enum missing_commit_check_level get_missing_commit_check_level(void)
> +int check_todo_list_from_file(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;
> -}
> -
> -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 check_todo_list(void)
> -{
> -	enum missing_commit_check_level check_level = get_missing_commit_check_level();
> -	struct strbuf todo_file = STRBUF_INIT;
> -	struct todo_list todo_list = TODO_LIST_INIT;
> -	struct strbuf missing = STRBUF_INIT;
> -	int advise_to_edit_todo = 0, res = 0, i;
> -	struct commit_seen commit_seen;
> -
> -	init_commit_seen(&commit_seen);
> +	struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
> +	int res = 0;
>   
> -	strbuf_addstr(&todo_file, rebase_path_todo());
> -	if (strbuf_read_file_or_whine(&todo_list.buf, todo_file.buf) < 0) {
> +	if (strbuf_read_file_or_whine(&new_todo.buf, rebase_path_todo()) < 0) {
>   		res = -1;
> -		goto leave_check;
> -	}
> -	advise_to_edit_todo = res =
> -		todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list);
> -
> -	if (res || check_level == MISSING_COMMIT_CHECK_IGNORE)
> -		goto leave_check;
> -
> -	/* Mark the commits in git-rebase-todo as seen */
> -	for (i = 0; i < todo_list.nr; i++) {
> -		struct commit *commit = todo_list.items[i].commit;
> -		if (commit)
> -			*commit_seen_at(&commit_seen, commit) = 1;
> +		goto out;
>   	}
>   
> -	todo_list_release(&todo_list);
> -	strbuf_addstr(&todo_file, ".backup");
> -	if (strbuf_read_file_or_whine(&todo_list.buf, todo_file.buf) < 0) {
> +	if (strbuf_read_file_or_whine(&old_todo.buf, rebase_path_todo_backup()) < 0) {
>   		res = -1;
> -		goto leave_check;
> -	}
> -	strbuf_release(&todo_file);
> -	res = !!todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list);
> -
> -	/* Find commits in git-rebase-todo.backup yet unseen */
> -	for (i = todo_list.nr - 1; i >= 0; i--) {
> -		struct todo_item *item = todo_list.items + i;
> -		struct commit *commit = item->commit;
> -		if (commit && !*commit_seen_at(&commit_seen, commit)) {
> -			strbuf_addf(&missing, " - %s %.*s\n",
> -				    short_commit_name(commit),
> -				    item->arg_len, item->arg);
> -			*commit_seen_at(&commit_seen, commit) = 1;
> -		}
> +		goto out;
>   	}
>   
> -	/* Warn about missing commits */
> -	if (!missing.len)
> -		goto leave_check;
> -
> -	if (check_level == MISSING_COMMIT_CHECK_ERROR)
> -		advise_to_edit_todo = res = 1;
> +	res = todo_list_check(&old_todo, &new_todo);
>   
> -	fprintf(stderr,
> -		_("Warning: some commits may have been dropped accidentally.\n"
> -		"Dropped commits (newer to older):\n"));
> -
> -	/* Make the list user-friendly and display */
> -	fputs(missing.buf, stderr);
> -	strbuf_release(&missing);
> -
> -	fprintf(stderr, _("To avoid this message, use \"drop\" to "
> -		"explicitly remove a commit.\n\n"
> -		"Use 'git config rebase.missingCommitsCheck' to change "
> -		"the level of warnings.\n"
> -		"The possible behaviours are: ignore, warn, error.\n\n"));
> -
> -leave_check:
> -	clear_commit_seen(&commit_seen);
> -	strbuf_release(&todo_file);
> -	todo_list_release(&todo_list);
> -
> -	if (advise_to_edit_todo)
> -		fprintf(stderr,
> -			_("You can fix this with 'git rebase --edit-todo' "
> -			  "and then run 'git rebase --continue'.\n"
> -			  "Or you can abort the rebase with 'git rebase"
> -			  " --abort'.\n"));
> +out:
> +	todo_list_release(&old_todo);
> +	todo_list_release(&new_todo);
>   
>   	return res;
>   }
> @@ -4769,7 +4684,7 @@ int complete_action(struct replay_opts *opts, unsigned flags,
>   
>   	todo_list_release(&todo_list);
>   
> -	if (check_todo_list()) {
> +	if (check_todo_list_from_file()) {
>   		checkout_onto(opts, onto_name, onto, orig_head);
>   		return -1;
>   	}
> diff --git a/sequencer.h b/sequencer.h
> index c786dee543..fb8b85bf9e 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -63,12 +63,6 @@ struct replay_opts {
>   };
>   #define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }
>   
> -enum missing_commit_check_level {
> -	MISSING_COMMIT_CHECK_IGNORE = 0,
> -	MISSING_COMMIT_CHECK_WARN,
> -	MISSING_COMMIT_CHECK_ERROR
> -};
> -
>   int write_message(const void *buf, size_t len, const char *filename,
>   		  int append_eol);
>   
> @@ -142,8 +136,7 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
>   
>   int sequencer_add_exec_commands(const char *command);
>   int transform_todos(unsigned flags);
> -enum missing_commit_check_level get_missing_commit_check_level(void);
> -int check_todo_list(void);
> +int check_todo_list_from_file(void);
>   int complete_action(struct replay_opts *opts, unsigned flags,
>   		    const char *shortrevisions, const char *onto_name,
>   		    const char *onto, const char *orig_head, const char *cmd,
> 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 04/15] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
  2018-10-07 19:54 ` [PATCH 04/15] sequencer: refactor sequencer_add_exec_commands() " Alban Gruin
@ 2018-10-11 11:25   ` Phillip Wood
  2018-10-11 16:57     ` Alban Gruin
  0 siblings, 1 reply; 29+ messages in thread
From: Phillip Wood @ 2018-10-11 11:25 UTC (permalink / raw)
  To: Alban Gruin, git; +Cc: Johannes Schindelin, Junio C Hamano

On 07/10/2018 20:54, Alban Gruin wrote:
> This refactors sequencer_add_exec_commands() to work on a todo_list to
> avoid redundant reads and writes to the disk.
> 
> sequencer_add_exec_commands() still reads the todo list from the disk,
> as it is needed by rebase -p.  todo_list_add_exec_commands() works on a
> todo_list structure, and reparses it at the end.
> 
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>   sequencer.c | 56 +++++++++++++++++++++++++++++++----------------------
>   1 file changed, 33 insertions(+), 23 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 8dda61927c..6d998f21a4 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4370,34 +4370,21 @@ 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,
> +					const char *commands)
>   {
> -	const char *todo_file = rebase_path_todo();
> -	struct todo_list todo_list = TODO_LIST_INIT;
> -	struct strbuf *buf = &todo_list.buf;
> +	struct strbuf *buf = &todo_list->buf;
>   	size_t offset = 0, commands_len = strlen(commands);
>   	int i, insert;
>   
> -	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);
> -	}
> -
>   	/*
>   	 * Insert <commands> after every pick. Here, fixup/squash chains
>   	 * are considered part of the pick, so we insert the commands *after*
>   	 * 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;
> +	for (i = 0; i < todo_list->nr; i++) {
> +		enum todo_command command = todo_list->items[i].command;
>   
>   		if (insert >= 0) {
>   			/* skip fixup/squash chains */
> @@ -4408,7 +4395,7 @@ int sequencer_add_exec_commands(const char *commands)
>   				continue;
>   			}
>   			strbuf_insert(buf,
> -				      todo_list.items[insert].offset_in_buf +
> +				      todo_list->items[insert].offset_in_buf +
>   				      offset, commands, commands_len);
>   			offset += commands_len;
>   			insert = -1;
> @@ -4419,15 +4406,38 @@ int sequencer_add_exec_commands(const char *commands)
>   	}
>   
>   	/* insert or append final <commands> */
> -	if (insert >= 0 && insert < todo_list.nr)
> -		strbuf_insert(buf, todo_list.items[insert].offset_in_buf +
> +	if (insert >= 0 && insert < todo_list->nr)
> +		strbuf_insert(buf, todo_list->items[insert].offset_in_buf +
>   			      offset, commands, commands_len);
>   	else if (insert >= 0 || !offset)
>   		strbuf_add(buf, commands, commands_len);
>   
> -	i = write_message(buf->buf, buf->len, todo_file, 0);
> +	if (todo_list_parse_insn_buffer(buf->buf, todo_list))
> +		BUG("unusable todo list");}

It is a shame to have to re-parse the todo list, I wonder how difficult 
it would be to adjust the todo_list item array as the exec commands are 
inserted. The same applies to the next couple of patches

Best Wishes

Phillip

> +
> +/*
> + * Add commands after pick and (series of) squash/fixup commands
> + * in the todo list.
> + */
> +int sequencer_add_exec_commands(const char *commands)
> +{
> +	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_add_exec_commands(&todo_list, commands);
> +	res = write_message(todo_list.buf.buf, todo_list.buf.len, todo_file, 0);
>   	todo_list_release(&todo_list);
> -	return i;
> +
> +	return res;
>   }
>   
>   int transform_todos(unsigned flags)
> 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 08/15] sequencer: change complete_action() to use the refactored functions
  2018-10-07 19:54 ` [PATCH 08/15] sequencer: change complete_action() to use the refactored functions Alban Gruin
@ 2018-10-11 13:51   ` Phillip Wood
  2018-10-11 17:06     ` Alban Gruin
  0 siblings, 1 reply; 29+ messages in thread
From: Phillip Wood @ 2018-10-11 13:51 UTC (permalink / raw)
  To: Alban Gruin, git; +Cc: Johannes Schindelin, Junio C Hamano

On 07/10/2018 20:54, Alban Gruin wrote:
> 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.
> 
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>   builtin/rebase--interactive.c | 13 +-----
>   sequencer.c                   | 76 +++++++++++++++++------------------
>   sequencer.h                   |  2 +-
>   3 files changed, 38 insertions(+), 53 deletions(-)
> 
> diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
> index eef1ff2e83..0700339f90 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,15 +100,13 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
>   	ret = sequencer_make_script(&todo_list.buf,
>   				    make_script_args.argc, make_script_args.argv,
>   				    flags);

I think it would be clearer to parse the todo list here explicitly 
rather than doing it implicitly in complete_action()

> -	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(opts, flags, shortrevisions, onto_name, onto,
> -				      head_hash, cmd, autosquash);
> +				      head_hash, cmd, autosquash, &todo_list);
>   	}
>   
>   	free(revisions);
> diff --git a/sequencer.c b/sequencer.c
> index dfb8d1c974..b37935e5ab 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4624,93 +4624,89 @@ static int skip_unnecessary_picks(struct object_id *output_oid)
>   	return 0;
>   }
>   
> +static int todo_list_rearrange_squash(struct todo_list *todo_list);
> +
>   int complete_action(struct replay_opts *opts, unsigned flags,
>   		    const char *shortrevisions, const char *onto_name,
>   		    const char *onto, const char *orig_head, const char *cmd,
> -		    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;
> +	int command_count;
>   
>   	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)
> +		strbuf_add(buf, "noop\n", 5);
> +
> +	if (todo_list_parse_insn_buffer(buf->buf, todo_list))
> +		BUG("unusable todo list");
>   
> -	if (autosquash && rearrange_squash_in_todo_file())
> +	if (autosquash && todo_list_rearrange_squash(todo_list))
>   		return -1;
>   
>   	if (cmd && *cmd)
> -		sequencer_add_exec_commands(cmd);
> +		todo_list_add_exec_commands(todo_list, cmd);
>   
> -	if (strbuf_read_file(buf, todo_file, 0) < 0)
> -		return error_errno(_("could not read '%s'."), todo_file);
> -
> -	if (todo_list_parse_insn_buffer(buf->buf, &todo_list)) {
> -		todo_list_release(&todo_list);
> -		return error(_("unusable todo list: '%s'"), todo_file);
> -	}
> -
> -	if (count_commands(&todo_list) == 0) {
> +	command_count = count_commands(todo_list);
> +	if (command_count == 0) {
>   		apply_autostash(opts);
>   		sequencer_remove_state(opts);
> -		todo_list_release(&todo_list);
>   
>   		return error(_("nothing to do"));
>   	}
>   
> +	todo_list_transform(todo_list, flags | TODO_LIST_SHORTEN_IDS);
> +
>   	strbuf_addch(buf, '\n');
>   	strbuf_commented_addf(buf, Q_("Rebase %s onto %s (%d command)",
>   				      "Rebase %s onto %s (%d commands)",
> -				      count_commands(&todo_list)),
> -			      shortrevisions, shortonto, count_commands(&todo_list));
> +				      command_count),
> +			      shortrevisions, shortonto, command_count);
>   	append_todo_help(0, flags & TODO_LIST_KEEP_EMPTY, buf);
>   
> -	if (write_message(buf->buf, buf->len, todo_file, 0)) {
> -		todo_list_release(&todo_list);
> -		return -1;
> -	}
> +	if (write_message(buf->buf, buf->len, todo_file, 0))
> +		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 (transform_todo_file(flags | TODO_LIST_SHORTEN_IDS))
> -		return error(_("could not transform the todo list"));
> -
> -	strbuf_reset(buf);
> -
> -	if (launch_sequence_editor(todo_file, buf, NULL)) {
> +	if (launch_sequence_editor(todo_file, &new_todo.buf, NULL)) {
>   		apply_autostash(opts);
>   		sequencer_remove_state(opts);
> -		todo_list_release(&todo_list);
>   
>   		return -1;
>   	}
>   
> -	strbuf_stripspace(buf, 1);
> -	if (buf->len == 0) {
> +	strbuf_stripspace(&new_todo.buf, 1);
> +	if (new_todo.buf.len == 0) {
>   		apply_autostash(opts);
>   		sequencer_remove_state(opts);
> -		todo_list_release(&todo_list);
> +		todo_list_release(&new_todo);
>   
>   		return error(_("nothing to do"));
>   	}
>   
> -	todo_list_release(&todo_list);
> -
> -	if (check_todo_list_from_file()) {
> +	if (todo_list_check(todo_list, &new_todo)) {
>   		checkout_onto(opts, onto_name, onto, orig_head);
> +		todo_list_release(&new_todo);
> +
>   		return -1;
>   	}
>   
> -	if (transform_todo_file(flags & ~(TODO_LIST_SHORTEN_IDS)))
> -		return error(_("could not transform the todo list"));
> +	todo_list_transform(&new_todo, flags & ~(TODO_LIST_SHORTEN_IDS));
> +
> +	if (rewrite_file(todo_file, new_todo.buf.buf, new_todo.buf.len) < 0) {
> +		todo_list_release(&new_todo);
> +		return error_errno(_("could not write '%s'"), todo_file);
> +	}

rewrite_file() can truncate the old version of the file if there is an 
error when writing the new version, I think it would be better to use 
write_message() instead as that atomically updates the file. The same 
applies to patch 5 (refactor rearrange_squash()) after which I think 
there will be no callers to rewrite_file() so it can be deleted.

Best Wishes

Phillip

> +
> +	todo_list_release(&new_todo);
>   
>   	if (opts->allow_ff && skip_unnecessary_picks(&oid))
>   		return error(_("could not skip unnecessary pick commands"));
> diff --git a/sequencer.h b/sequencer.h
> index 21d9ba09ab..5bd3b79282 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -141,7 +141,7 @@ int check_todo_list_from_file(void);
>   int complete_action(struct replay_opts *opts, unsigned flags,
>   		    const char *shortrevisions, const char *onto_name,
>   		    const char *onto, const char *orig_head, const char *cmd,
> -		    unsigned autosquash);
> +		    unsigned autosquash, struct todo_list *todo_list);
>   int rearrange_squash_in_todo_file(void);
>   
>   extern const char sign_off_header[];
> 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 10/15] rebase-interactive: use todo_list_transform() in edit_todo_list()
  2018-10-07 19:54 ` [PATCH 10/15] rebase-interactive: use todo_list_transform() in edit_todo_list() Alban Gruin
@ 2018-10-11 15:16   ` Phillip Wood
  2018-10-11 19:58     ` Alban Gruin
  0 siblings, 1 reply; 29+ messages in thread
From: Phillip Wood @ 2018-10-11 15:16 UTC (permalink / raw)
  To: Alban Gruin, git; +Cc: Johannes Schindelin, Junio C Hamano

On 07/10/2018 20:54, Alban Gruin wrote:
> 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_transform()
> instead.
> 
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>   rebase-interactive.c | 40 +++++++++++++++++++---------------------
>   1 file changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index 7c7f720a3d..f42d48e192 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -78,39 +78,37 @@ 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);
> -		return -1;
> -	}
> -
> -	strbuf_release(&buf);
> +	strbuf_stripspace(&todo_list.buf, 1);
> +	if (!todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list))
> +		todo_list_transform(&todo_list, 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);
> +	append_todo_help(1, 0, &todo_list.buf);
>   
> -	append_todo_help(1, 0, &buf);

I think this patch is fine, I was just wondering if you meant to move 
the call to append_todo_help() above the blank line?

Best Wishes

Phillip

> -	if (write_message(buf.buf, buf.len, todo_file, 0)) {
> -		strbuf_release(&buf);
> +	if (write_message(todo_list.buf.buf, todo_list.buf.len, todo_file, 0)) {
> +		todo_list_release(&todo_list);
>   		return -1;
>   	}
>   
> -	strbuf_release(&buf);
> -
> -	if (launch_sequence_editor(todo_file, NULL, NULL))
> +	strbuf_reset(&todo_list.buf);
> +	if (launch_sequence_editor(todo_file, &todo_list.buf, NULL)) {
> +		todo_list_release(&todo_list);
>   		return -1;
> +	}
>   
> -	transform_todo_file(flags & ~(TODO_LIST_SHORTEN_IDS));
> +	if (!todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list)) {
> +		todo_list_transform(&todo_list, flags & ~(TODO_LIST_SHORTEN_IDS));
> +		res = write_message(todo_list.buf.buf, todo_list.buf.len, todo_file, 0);
> +	}
>   
> -	return 0;
> +	todo_list_release(&todo_list);
> +	return res;
>   }
>   
>   define_commit_slab(commit_seen, unsigned char);
> 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 04/15] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
  2018-10-11 11:25   ` Phillip Wood
@ 2018-10-11 16:57     ` Alban Gruin
  2018-10-12  9:54       ` Phillip Wood
  0 siblings, 1 reply; 29+ messages in thread
From: Alban Gruin @ 2018-10-11 16:57 UTC (permalink / raw)
  To: phillip.wood, git; +Cc: Johannes Schindelin, Junio C Hamano

Hi Phillip,

thanks for taking the time to review my patches.

Le 11/10/2018 à 13:25, Phillip Wood a écrit :
> On 07/10/2018 20:54, Alban Gruin wrote:
>> @@ -4419,15 +4406,38 @@ int sequencer_add_exec_commands(const char
>> *commands)
>>       }
>>         /* insert or append final <commands> */
>> -    if (insert >= 0 && insert < todo_list.nr)
>> -        strbuf_insert(buf, todo_list.items[insert].offset_in_buf +
>> +    if (insert >= 0 && insert < todo_list->nr)
>> +        strbuf_insert(buf, todo_list->items[insert].offset_in_buf +
>>                     offset, commands, commands_len);
>>       else if (insert >= 0 || !offset)
>>           strbuf_add(buf, commands, commands_len);
>>   -    i = write_message(buf->buf, buf->len, todo_file, 0);
>> +    if (todo_list_parse_insn_buffer(buf->buf, todo_list))
>> +        BUG("unusable todo list");}
> 
> It is a shame to have to re-parse the todo list, I wonder how difficult
> it would be to adjust the todo_list item array as the exec commands are
> inserted. The same applies to the next couple of patches
> 

Good question.

This function inserts an `exec' command after every `pick' command.
These commands are stored in a dynamically allocated list, grew with
ALLOW_GROW().

If we want to keep the current structure, we would have to grow the size
of the list by 1 and move several element to the end every time we want
to add an `exec' command.  It would not be very effective.  Perhaps I
should use a linked list here, instead.  It may also work well with
rearrange_squash() and skip_unnecessary_picks().

Maybe we could even get rid of the strbuf at some point.

> Best Wishes
> 
> Phillip
> 

Cheers,
Alban


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 08/15] sequencer: change complete_action() to use the refactored functions
  2018-10-11 13:51   ` Phillip Wood
@ 2018-10-11 17:06     ` Alban Gruin
  0 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2018-10-11 17:06 UTC (permalink / raw)
  To: phillip.wood, git; +Cc: Johannes Schindelin, Junio C Hamano

Le 11/10/2018 à 15:51, Phillip Wood a écrit :
> On 07/10/2018 20:54, Alban Gruin wrote:
>> +    if (rewrite_file(todo_file, new_todo.buf.buf, new_todo.buf.len) <
>> 0) {
>> +        todo_list_release(&new_todo);
>> +        return error_errno(_("could not write '%s'"), todo_file);
>> +    }
> 
> rewrite_file() can truncate the old version of the file if there is an
> error when writing the new version, I think it would be better to use
> write_message() instead as that atomically updates the file. The same
> applies to patch 5 (refactor rearrange_squash()) after which I think
> there will be no callers to rewrite_file() so it can be deleted.

You’re right, I didn’t notice that.

> 
> Best Wishes
> 
> Phillip
> 

Cheers,
Alban


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 10/15] rebase-interactive: use todo_list_transform() in edit_todo_list()
  2018-10-11 15:16   ` Phillip Wood
@ 2018-10-11 19:58     ` Alban Gruin
  0 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2018-10-11 19:58 UTC (permalink / raw)
  To: phillip.wood, git; +Cc: Johannes Schindelin, Junio C Hamano

Le 11/10/2018 à 17:16, Phillip Wood a écrit :
> On 07/10/2018 20:54, Alban Gruin wrote:
>> 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_transform()
>> instead.
>>
>> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
>> ---
>>   rebase-interactive.c | 40 +++++++++++++++++++---------------------
>>   1 file changed, 19 insertions(+), 21 deletions(-)
>>
>> diff --git a/rebase-interactive.c b/rebase-interactive.c
>> index 7c7f720a3d..f42d48e192 100644
>> --- a/rebase-interactive.c
>> +++ b/rebase-interactive.c
>> @@ -78,39 +78,37 @@ 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);
>> -        return -1;
>> -    }
>> -
>> -    strbuf_release(&buf);
>> +    strbuf_stripspace(&todo_list.buf, 1);
>> +    if (!todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list))
>> +        todo_list_transform(&todo_list, 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);
>> +    append_todo_help(1, 0, &todo_list.buf);
>>   -    append_todo_help(1, 0, &buf);
> 
> I think this patch is fine, I was just wondering if you meant to move
> the call to append_todo_help() above the blank line?
> 

No

> Best Wishes
> 
> Phillip
> 

Cheers,
Alban


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 04/15] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
  2018-10-11 16:57     ` Alban Gruin
@ 2018-10-12  9:54       ` Phillip Wood
  2018-10-12 12:23         ` Alban Gruin
  0 siblings, 1 reply; 29+ messages in thread
From: Phillip Wood @ 2018-10-12  9:54 UTC (permalink / raw)
  To: Alban Gruin, phillip.wood, git; +Cc: Johannes Schindelin, Junio C Hamano

On 11/10/2018 17:57, Alban Gruin wrote:
> Hi Phillip,
> 
> thanks for taking the time to review my patches.
> 
> Le 11/10/2018 à 13:25, Phillip Wood a écrit :
>> On 07/10/2018 20:54, Alban Gruin wrote:
>>> @@ -4419,15 +4406,38 @@ int sequencer_add_exec_commands(const char
>>> *commands)
>>>       }
>>>         /* insert or append final <commands> */
>>> -    if (insert >= 0 && insert < todo_list.nr)
>>> -        strbuf_insert(buf, todo_list.items[insert].offset_in_buf +
>>> +    if (insert >= 0 && insert < todo_list->nr)
>>> +        strbuf_insert(buf, todo_list->items[insert].offset_in_buf +
>>>                     offset, commands, commands_len);
>>>       else if (insert >= 0 || !offset)
>>>           strbuf_add(buf, commands, commands_len);
>>>   -    i = write_message(buf->buf, buf->len, todo_file, 0);
>>> +    if (todo_list_parse_insn_buffer(buf->buf, todo_list))
>>> +        BUG("unusable todo list");}
>>
>> It is a shame to have to re-parse the todo list, I wonder how difficult
>> it would be to adjust the todo_list item array as the exec commands are
>> inserted. The same applies to the next couple of patches
>>
> 
> Good question.
> 
> This function inserts an `exec' command after every `pick' command.
> These commands are stored in a dynamically allocated list, grew with
> ALLOW_GROW().
> 
> If we want to keep the current structure, we would have to grow the size
> of the list by 1 and move several element to the end every time we want
> to add an `exec' command.  It would not be very effective.  Perhaps I
> should use a linked list here, instead.  It may also work well with
> rearrange_squash() and skip_unnecessary_picks().
> 
> Maybe we could even get rid of the strbuf at some point.

Another way would be to use the strbuf as a string pool rather than a
copy of the text of the file. There could be a write_todo_list()
function that takes a todo list and some flags, iterates over the items
in the todo list and writes the file. The flags would specify whether to
append the todo help and whether to abbreviate the object ids (so there
is no need for a separate call to transform_todos()). Then
add_exec_commands() could allocate a new array of todo items which it
builds up as it works through the original list and replaces the
original list with the new one at the end. The text of the exec items
can be added to the end of the strbuf (we only need one copy of the exec
text with this scheme). rearrange_squash() can use a temporary array to
build a new list as well or just memmove() things but that might be
slower if there is a lot of rearrangement to do. skip_unecessary_picks()
could just set the current item to the one we want to start with.

Best Wishes

Phillip

> 
>> Best Wishes
>>
>> Phillip
>>
> 
> Cheers,
> Alban
> 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 07/15] sequencer: make sequencer_make_script() write its script to a strbuf
  2018-10-07 19:54 ` [PATCH 07/15] sequencer: make sequencer_make_script() write its script to a strbuf Alban Gruin
@ 2018-10-12 10:01   ` SZEDER Gábor
  2018-10-19  8:16     ` Junio C Hamano
  0 siblings, 1 reply; 29+ messages in thread
From: SZEDER Gábor @ 2018-10-12 10:01 UTC (permalink / raw)
  To: Alban Gruin; +Cc: git, Johannes Schindelin, Phillip Wood, Junio C Hamano

On Sun, Oct 07, 2018 at 09:54:10PM +0200, Alban Gruin wrote:

> diff --git a/sequencer.c b/sequencer.c
> index 30a7fe3958..dfb8d1c974 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4083,7 +4083,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;
> @@ -4230,7 +4230,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;
>  
> @@ -4240,9 +4240,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_addf(out, "\n");

Please use plain strbuf_add() here.

Or strbuf_complete_line()?  Dunno, as seen in the previous hunk, 'out'
won't be empty at this point.


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 04/15] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
  2018-10-12  9:54       ` Phillip Wood
@ 2018-10-12 12:23         ` Alban Gruin
  0 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2018-10-12 12:23 UTC (permalink / raw)
  To: phillip.wood; +Cc: git, Johannes Schindelin, Junio C Hamano

Le 12/10/2018 à 11:54, Phillip Wood a écrit :
> On 11/10/2018 17:57, Alban Gruin wrote:
> > Hi Phillip,
> > 
> > thanks for taking the time to review my patches.
> > 
> > Le 11/10/2018 à 13:25, Phillip Wood a écrit :
> >> On 07/10/2018 20:54, Alban Gruin wrote:
> >>> @@ -4419,15 +4406,38 @@ int sequencer_add_exec_commands(const char
> >>> *commands)
> >>>       }
> >>>         /* insert or append final <commands> */
> >>> -    if (insert >= 0 && insert < todo_list.nr)
> >>> -        strbuf_insert(buf, todo_list.items[insert].offset_in_buf +
> >>> +    if (insert >= 0 && insert < todo_list->nr)
> >>> +        strbuf_insert(buf, todo_list->items[insert].offset_in_buf +
> >>>                     offset, commands, commands_len);
> >>>       else if (insert >= 0 || !offset)
> >>>           strbuf_add(buf, commands, commands_len);
> >>>   -    i = write_message(buf->buf, buf->len, todo_file, 0);
> >>> +    if (todo_list_parse_insn_buffer(buf->buf, todo_list))
> >>> +        BUG("unusable todo list");}
> >> 
> >> It is a shame to have to re-parse the todo list, I wonder how difficult
> >> it would be to adjust the todo_list item array as the exec commands are
> >> inserted. The same applies to the next couple of patches
> > 
> > Good question.
> > 
> > This function inserts an `exec' command after every `pick' command.
> > These commands are stored in a dynamically allocated list, grew with
> > ALLOW_GROW().
> > 
> > If we want to keep the current structure, we would have to grow the size
> > of the list by 1 and move several element to the end every time we want
> > to add an `exec' command.  It would not be very effective.  Perhaps I
> > should use a linked list here, instead.  It may also work well with
> > rearrange_squash() and skip_unnecessary_picks().
> > 
> > Maybe we could even get rid of the strbuf at some point.
> 
> Another way would be to use the strbuf as a string pool rather than a
> copy of the text of the file. There could be a write_todo_list()
> function that takes a todo list and some flags, iterates over the items
> in the todo list and writes the file. The flags would specify whether to
> append the todo help and whether to abbreviate the object ids (so there
> is no need for a separate call to transform_todos()). Then
> add_exec_commands() could allocate a new array of todo items which it
> builds up as it works through the original list and replaces the
> original list with the new one at the end. The text of the exec items
> can be added to the end of the strbuf (we only need one copy of the exec
> text with this scheme). rearrange_squash() can use a temporary array to
> build a new list as well or just memmove() things but that might be
> slower if there is a lot of rearrangement to do. skip_unecessary_picks()
> could just set the current item to the one we want to start with.
> 

This sounds good, and it looks like the solution dscho proposed on IRC a few 
hours ago[0].  I will try to do this.

[0] http://colabti.org/irclogger/irclogger_log/git-devel?date=2018-10-12#l46

Cheers,
Alban





^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 07/15] sequencer: make sequencer_make_script() write its script to a strbuf
  2018-10-12 10:01   ` SZEDER Gábor
@ 2018-10-19  8:16     ` Junio C Hamano
  2018-10-19  9:27       ` SZEDER Gábor
  0 siblings, 1 reply; 29+ messages in thread
From: Junio C Hamano @ 2018-10-19  8:16 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Alban Gruin, git, Johannes Schindelin, Phillip Wood

SZEDER Gábor <szeder.dev@gmail.com> writes:

>>  		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_addf(out, "\n");
>
> Please use plain strbuf_add() here.

FWIW, contrib/coccinelle/strbuf.cocci.patch gave us this:

diff -u -p a/sequencer.c b/sequencer.c
--- a/sequencer.c
+++ b/sequencer.c
@@ -4311,7 +4311,7 @@ static int make_script_with_merges(struc
 		if (entry)
 			strbuf_addf(out, "\n%c Branch %s\n", comment_line_char, entry->string);
 		else
-			strbuf_addf(out, "\n");
+			strbuf_addstr(out, "\n");
 
 		while (oidset_contains(&interesting, &commit->object.oid) &&
 		       !oidset_contains(&shown, &commit->object.oid)) {

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 07/15] sequencer: make sequencer_make_script() write its script to a strbuf
  2018-10-19  8:16     ` Junio C Hamano
@ 2018-10-19  9:27       ` SZEDER Gábor
  0 siblings, 0 replies; 29+ messages in thread
From: SZEDER Gábor @ 2018-10-19  9:27 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Alban Gruin, git, Johannes Schindelin, Phillip Wood

On Fri, Oct 19, 2018 at 05:16:46PM +0900, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> >>  		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_addf(out, "\n");
> >
> > Please use plain strbuf_add() here.
> 
> FWIW, contrib/coccinelle/strbuf.cocci.patch gave us this:
> 
> diff -u -p a/sequencer.c b/sequencer.c
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4311,7 +4311,7 @@ static int make_script_with_merges(struc
>  		if (entry)
>  			strbuf_addf(out, "\n%c Branch %s\n", comment_line_char, entry->string);
>  		else
> -			strbuf_addf(out, "\n");
> +			strbuf_addstr(out, "\n");
>  
>  		while (oidset_contains(&interesting, &commit->object.oid) &&
>  		       !oidset_contains(&shown, &commit->object.oid)) {

Uh, right.  I didn't want to copy-paste a patch with too long lines
into my mailer, as it usually doesn't end too good, so I just typed
the function name.  Evidently I couldn't quite manage.


^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, back to index

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-07 19:54 [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
2018-10-07 19:54 ` [PATCH 01/15] sequencer: clear the number of items of a todo_list before parsing Alban Gruin
2018-10-07 19:54 ` [PATCH 02/15] sequencer: make the todo_list structure public Alban Gruin
2018-10-07 19:54 ` [PATCH 03/15] sequencer: refactor check_todo_list() to work on a todo_list Alban Gruin
2018-10-11 11:23   ` Phillip Wood
2018-10-07 19:54 ` [PATCH 04/15] sequencer: refactor sequencer_add_exec_commands() " Alban Gruin
2018-10-11 11:25   ` Phillip Wood
2018-10-11 16:57     ` Alban Gruin
2018-10-12  9:54       ` Phillip Wood
2018-10-12 12:23         ` Alban Gruin
2018-10-07 19:54 ` [PATCH 05/15] sequencer: refactor rearrange_squash() " Alban Gruin
2018-10-07 19:54 ` [PATCH 06/15] sequencer: refactor transform_todos() " Alban Gruin
2018-10-07 19:54 ` [PATCH 07/15] sequencer: make sequencer_make_script() write its script to a strbuf Alban Gruin
2018-10-12 10:01   ` SZEDER Gábor
2018-10-19  8:16     ` Junio C Hamano
2018-10-19  9:27       ` SZEDER Gábor
2018-10-07 19:54 ` [PATCH 08/15] sequencer: change complete_action() to use the refactored functions Alban Gruin
2018-10-11 13:51   ` Phillip Wood
2018-10-11 17:06     ` Alban Gruin
2018-10-07 19:54 ` [PATCH 09/15] sequencer: refactor skip_unnecessary_picks() to work on a todo_list Alban Gruin
2018-10-07 19:54 ` [PATCH 10/15] rebase-interactive: use todo_list_transform() in edit_todo_list() Alban Gruin
2018-10-11 15:16   ` Phillip Wood
2018-10-11 19:58     ` Alban Gruin
2018-10-07 19:54 ` [PATCH 11/15] rebase-interactive: append_todo_help() changes Alban Gruin
2018-10-07 19:54 ` [PATCH 12/15] rebase-interactive: rewrite edit_todo_list() to handle the initial edit Alban Gruin
2018-10-07 19:54 ` [PATCH 13/15] sequencer: use edit_todo_list() in complete_action() Alban Gruin
2018-10-07 19:54 ` [PATCH 14/15] sequencer: fix a call to error() in transform_todo_file() Alban Gruin
2018-10-07 19:54 ` [PATCH 15/15] rebase--interactive: move transform_todo_file() to rebase--interactive.c Alban Gruin
2018-10-07 20:51 ` [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox