[04/15] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
diff mbox series

Message ID 20181007195418.25752-5-alban.gruin@gmail.com
State New
Headers show
Series
  • sequencer: refactor functions working on a todo_list
Related show

Commit Message

Alban Gruin Oct. 7, 2018, 7:54 p.m. UTC
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(-)

Comments

Phillip Wood Oct. 11, 2018, 11:25 a.m. UTC | #1
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)
>
Alban Gruin Oct. 11, 2018, 4:57 p.m. UTC | #2
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
Phillip Wood Oct. 12, 2018, 9:54 a.m. UTC | #3
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
>
Alban Gruin Oct. 12, 2018, 12:23 p.m. UTC | #4
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

Patch
diff mbox series

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)