[v2,04/16] sequencer: introduce todo_list_write_to_file()
diff mbox series

Message ID 20181027212930.9303-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. 27, 2018, 9:29 p.m. UTC
This introduce a new function to recreate the text of a todo list from
its commands, and then to write it to the disk.  This will be useful in
the future, the buffer of a todo list won’t be treated as a strict
mirror of the todo file by some of its functions once they will be
refactored.

This functionnality can already be found in todo_list_transform(), but
it is specifically made to replace the buffer of a todo list, which is
not the desired behaviour.  Thus, the part of todo_list_transform() that
actually creates the buffer is moved to a new function,
todo_list_to_strbuf().  The rest is unused, and so is dropped.

todo_list_write_to_file() can also take care to append the help text to
the buffer before writing it to the disk, or to write only the first n
items of the list.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 59 ++++++++++++++++++++++++++++++++++++-----------------
 sequencer.h |  4 +++-
 2 files changed, 43 insertions(+), 20 deletions(-)

Comments

Phillip Wood Oct. 30, 2018, 4:28 p.m. UTC | #1
Hi Alban

I like the direction this is going, it is an improvement on re-scanning 
the list at the end of each function.

On 27/10/2018 22:29, Alban Gruin wrote:
> This introduce a new function to recreate the text of a todo list from
> its commands, and then to write it to the disk.  This will be useful in
> the future, the buffer of a todo list won’t be treated as a strict
> mirror of the todo file by some of its functions once they will be
> refactored.

I'd suggest rewording this slightly, maybe something like

This introduces a new function to recreate the text of a todo list from
its commands and write it to a file. This will be useful as the next few 
commits will change the use of the buffer in struct todo_list so it will 
no-longer be a mirror of the file on disk.

> This functionnality can already be found in todo_list_transform(), but

s/functionnality/functionality/

> it is specifically made to replace the buffer of a todo list, which is
> not the desired behaviour.  Thus, the part of todo_list_transform() that
> actually creates the buffer is moved to a new function,
> todo_list_to_strbuf().  The rest is unused, and so is dropped.
> 
> todo_list_write_to_file() can also take care to append the help text to

s/care to append/care of appending/

> the buffer before writing it to the disk, or to write only the first n
> items of the list.

Why/when do we only want to write a subset of the items?

> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>   sequencer.c | 59 ++++++++++++++++++++++++++++++++++++-----------------
>   sequencer.h |  4 +++-
>   2 files changed, 43 insertions(+), 20 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 07296f1f57..0dd45677b1 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4256,24 +4256,27 @@ int sequencer_add_exec_commands(const char *commands)
>   	return i;
>   }
>   
> -void todo_list_transform(struct todo_list *todo_list, unsigned flags)
> +static void todo_list_to_strbuf(struct todo_list *todo_list, struct strbuf *buf,
> +				int num, unsigned flags)
>   {
> -	struct strbuf buf = STRBUF_INIT;
>   	struct todo_item *item;
> -	int i;
> +	int i, max = todo_list->nr;
>   
> -	for (item = todo_list->items, i = 0; i < todo_list->nr; i++, item++) {
> +	if (num > 0 && num < max)
> +		max = num;
> +
> +	for (item = todo_list->items, i = 0; i < max; i++, item++) {
>   		/* if the item is not a command write it and continue */
>   		if (item->command >= TODO_COMMENT) {
> -			strbuf_addf(&buf, "%.*s\n", item->arg_len, item->arg);
> +			strbuf_addf(buf, "%.*s\n", item->arg_len, item->arg);
>   			continue;
>   		}
>   
>   		/* add command to the buffer */
>   		if (flags & TODO_LIST_ABBREVIATE_CMDS)
> -			strbuf_addch(&buf, command_to_char(item->command));
> +			strbuf_addch(buf, command_to_char(item->command));
>   		else
> -			strbuf_addstr(&buf, command_to_string(item->command));
> +			strbuf_addstr(buf, command_to_string(item->command));
>   
>   		/* add commit id */
>   		if (item->commit) {
> @@ -4283,27 +4286,46 @@ void todo_list_transform(struct todo_list *todo_list, unsigned flags)
>   
>   			if (item->command == TODO_MERGE) {
>   				if (item->flags & TODO_EDIT_MERGE_MSG)
> -					strbuf_addstr(&buf, " -c");
> +					strbuf_addstr(buf, " -c");
>   				else
> -					strbuf_addstr(&buf, " -C");
> +					strbuf_addstr(buf, " -C");
>   			}
>   
> -			strbuf_addf(&buf, " %s", oid);
> +			strbuf_addf(buf, " %s", oid);
>   		}
>   
>   		/* add all the rest */
>   		if (!item->arg_len)
> -			strbuf_addch(&buf, '\n');
> +			strbuf_addch(buf, '\n');
>   		else
> -			strbuf_addf(&buf, " %.*s\n", item->arg_len, item->arg);
> +			strbuf_addf(buf, " %.*s\n", item->arg_len, item->arg);
>   	}
> +}
>   
> -	strbuf_reset(&todo_list->buf);
> -	strbuf_add(&todo_list->buf, buf.buf, buf.len);
> +int todo_list_write_to_file(struct todo_list *todo_list, const char *file,
> +			    const char *shortrevisions, const char *shortonto,
> +			    int command_count, int append_help, int num, unsigned flags)

This is a really long argument list which makes it easy for callers to 
get the parameters in the wrong order. I think append_help could 
probably be folded into the flags, I'm not sure what the command_count 
is used for but I've only read the first few patches. Maybe it would be 
better to pass a struct so we have named fields.

Best Wishes

Phillip

> +{
> +	int edit_todo = !(shortrevisions && shortonto), res;
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	todo_list_to_strbuf(todo_list, &buf, num, flags);
> +
> +	if (append_help) {
> +		if (!edit_todo) {
> +			strbuf_addch(&buf, '\n');
> +			strbuf_commented_addf(&buf, Q_("Rebase %s onto %s (%d command)",
> +						       "Rebase %s onto %s (%d commands)",
> +						       command_count),
> +					      shortrevisions, shortonto, command_count);
> +		}
> +		append_todo_help(edit_todo, flags & TODO_LIST_KEEP_EMPTY, &buf);
> +	}
> +
> +	res = write_message(buf.buf, buf.len, file, 0);
>   	strbuf_release(&buf);
>   
> -	if (todo_list_parse_insn_buffer(todo_list->buf.buf, todo_list))
> -		BUG("unusable todo list");
> +	return res;
>   }
>   
>   int transform_todo_file(unsigned flags)
> @@ -4320,9 +4342,8 @@ int transform_todo_file(unsigned flags)
>   		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);
> +	res = todo_list_write_to_file(&todo_list, todo_file,
> +				      NULL, NULL, 0, 0, -1, flags);
>   	todo_list_release(&todo_list);
>   	return res;
>   }
> diff --git a/sequencer.h b/sequencer.h
> index 029d842ac5..a299c977fe 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -113,7 +113,9 @@ 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);
> +int todo_list_write_to_file(struct todo_list *todo_list, const char *file,
> +			    const char *shortrevisions, const char *shortonto,
> +			    int command_count, int append_help, int num, unsigned flags);
>   void todo_list_release(struct todo_list *todo_list);
>   
>   /* Call this to setup defaults before parsing command line options */
>
Alban Gruin Nov. 1, 2018, 11:31 p.m. UTC | #2
Hi Phillip,

Le 30/10/2018 à 17:28, Phillip Wood a écrit :
> Hi Alban
> 
> I like the direction this is going, it is an improvement on re-scanning
> the list at the end of each function.
> 
> On 27/10/2018 22:29, Alban Gruin wrote:
>> This introduce a new function to recreate the text of a todo list from
>> its commands, and then to write it to the disk.  This will be useful in
>> the future, the buffer of a todo list won’t be treated as a strict
>> mirror of the todo file by some of its functions once they will be
>> refactored.
> 
> I'd suggest rewording this slightly, maybe something like
> 
> This introduces a new function to recreate the text of a todo list from
> its commands and write it to a file. This will be useful as the next few
> commits will change the use of the buffer in struct todo_list so it will
> no-longer be a mirror of the file on disk.
> 
>> This functionnality can already be found in todo_list_transform(), but
> 
> s/functionnality/functionality/
> 
>> it is specifically made to replace the buffer of a todo list, which is
>> not the desired behaviour.  Thus, the part of todo_list_transform() that
>> actually creates the buffer is moved to a new function,
>> todo_list_to_strbuf().  The rest is unused, and so is dropped.
>>
>> todo_list_write_to_file() can also take care to append the help text to
> 
> s/care to append/care of appending/
> 
>> the buffer before writing it to the disk, or to write only the first n
>> items of the list.
> 
> Why/when do we only want to write a subset of the items?
>

In skip_unnecessary_picks(), in patch [10/16].  It needs to write the
elements of the todo list that were already done in the `done' file.

> […]
>> +int todo_list_write_to_file(struct todo_list *todo_list, const char
>> *file,
>> +                const char *shortrevisions, const char *shortonto,
>> +                int command_count, int append_help, int num, unsigned
>> flags)
> 
> This is a really long argument list which makes it easy for callers to
> get the parameters in the wrong order. I think append_help could
> probably be folded into the flags, I'm not sure what the command_count
> is used for but I've only read the first few patches. Maybe it would be
> better to pass a struct so we have named fields.
> 

You’re right, command_count is not really needed since we pass the
complete todo list.

The only bit that irks me is that, if I stop passing command_count, I
would have to call count_commands() twice in complete_action(): once to
check if there are any commands in the todo list, and again inside of
todo_list_write_to_file() (see [09/16].)

Perhaps I could move this check before calling todo_list_rearrange_squash()?

As a sidenote, this is not why I added command_count to the parameters
of todo_list_write_to_file().  It was a confusion of my part.

> Best Wishes
> 
> Phillip
> 

Cheers,
Alban

Patch
diff mbox series

diff --git a/sequencer.c b/sequencer.c
index 07296f1f57..0dd45677b1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4256,24 +4256,27 @@  int sequencer_add_exec_commands(const char *commands)
 	return i;
 }
 
-void todo_list_transform(struct todo_list *todo_list, unsigned flags)
+static void todo_list_to_strbuf(struct todo_list *todo_list, struct strbuf *buf,
+				int num, unsigned flags)
 {
-	struct strbuf buf = STRBUF_INIT;
 	struct todo_item *item;
-	int i;
+	int i, max = todo_list->nr;
 
-	for (item = todo_list->items, i = 0; i < todo_list->nr; i++, item++) {
+	if (num > 0 && num < max)
+		max = num;
+
+	for (item = todo_list->items, i = 0; i < max; i++, item++) {
 		/* if the item is not a command write it and continue */
 		if (item->command >= TODO_COMMENT) {
-			strbuf_addf(&buf, "%.*s\n", item->arg_len, item->arg);
+			strbuf_addf(buf, "%.*s\n", item->arg_len, item->arg);
 			continue;
 		}
 
 		/* add command to the buffer */
 		if (flags & TODO_LIST_ABBREVIATE_CMDS)
-			strbuf_addch(&buf, command_to_char(item->command));
+			strbuf_addch(buf, command_to_char(item->command));
 		else
-			strbuf_addstr(&buf, command_to_string(item->command));
+			strbuf_addstr(buf, command_to_string(item->command));
 
 		/* add commit id */
 		if (item->commit) {
@@ -4283,27 +4286,46 @@  void todo_list_transform(struct todo_list *todo_list, unsigned flags)
 
 			if (item->command == TODO_MERGE) {
 				if (item->flags & TODO_EDIT_MERGE_MSG)
-					strbuf_addstr(&buf, " -c");
+					strbuf_addstr(buf, " -c");
 				else
-					strbuf_addstr(&buf, " -C");
+					strbuf_addstr(buf, " -C");
 			}
 
-			strbuf_addf(&buf, " %s", oid);
+			strbuf_addf(buf, " %s", oid);
 		}
 
 		/* add all the rest */
 		if (!item->arg_len)
-			strbuf_addch(&buf, '\n');
+			strbuf_addch(buf, '\n');
 		else
-			strbuf_addf(&buf, " %.*s\n", item->arg_len, item->arg);
+			strbuf_addf(buf, " %.*s\n", item->arg_len, item->arg);
 	}
+}
 
-	strbuf_reset(&todo_list->buf);
-	strbuf_add(&todo_list->buf, buf.buf, buf.len);
+int todo_list_write_to_file(struct todo_list *todo_list, const char *file,
+			    const char *shortrevisions, const char *shortonto,
+			    int command_count, int append_help, int num, unsigned flags)
+{
+	int edit_todo = !(shortrevisions && shortonto), res;
+	struct strbuf buf = STRBUF_INIT;
+
+	todo_list_to_strbuf(todo_list, &buf, num, flags);
+
+	if (append_help) {
+		if (!edit_todo) {
+			strbuf_addch(&buf, '\n');
+			strbuf_commented_addf(&buf, Q_("Rebase %s onto %s (%d command)",
+						       "Rebase %s onto %s (%d commands)",
+						       command_count),
+					      shortrevisions, shortonto, command_count);
+		}
+		append_todo_help(edit_todo, flags & TODO_LIST_KEEP_EMPTY, &buf);
+	}
+
+	res = write_message(buf.buf, buf.len, file, 0);
 	strbuf_release(&buf);
 
-	if (todo_list_parse_insn_buffer(todo_list->buf.buf, todo_list))
-		BUG("unusable todo list");
+	return res;
 }
 
 int transform_todo_file(unsigned flags)
@@ -4320,9 +4342,8 @@  int transform_todo_file(unsigned flags)
 		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);
+	res = todo_list_write_to_file(&todo_list, todo_file,
+				      NULL, NULL, 0, 0, -1, flags);
 	todo_list_release(&todo_list);
 	return res;
 }
diff --git a/sequencer.h b/sequencer.h
index 029d842ac5..a299c977fe 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -113,7 +113,9 @@  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);
+int todo_list_write_to_file(struct todo_list *todo_list, const char *file,
+			    const char *shortrevisions, const char *shortonto,
+			    int command_count, int append_help, int num, unsigned flags);
 void todo_list_release(struct todo_list *todo_list);
 
 /* Call this to setup defaults before parsing command line options */