Re: [PATCH v2 04/16] sequencer: introduce todo_list_write_to_file()
- From
- Phillip Wood <phillip.wood@talktalk.net>
- Date
- Oct 30, 2018, 16:28 UTC
- Message-ID
- <03475c29-5317-b105-6102-5cae3a5ae926@talktalk.net>
- In-Reply-To
- <20181027212930.9303-5-alban.gruin@gmail.com>
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:
Show 5 quoted lines
> 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/
Show 6 quoted lines
> 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?
Show 75 quoted lines
> 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
Show 54 quoted lines
> +{
> + 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 */
>