Re: [PATCH v2 04/16] sequencer: introduce todo_list_write_to_file()
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
Re: [PATCH v2 04/16] sequencer: introduce todo_list_write_to_file()
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
---
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(, "%.*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(, command_to_char(item->command));
+ strbuf_addch(buf, command_to_char(item->command));
else
- strbuf_addstr(, 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(, " -c");
+ strbuf_addstr(buf, " -c");
else
- strbuf_addstr(, " -C");
+ strbuf_addstr(buf, " -C");
}
- strbuf_addf(, " %s", oid);
+ strbuf_addf(buf, " %s", oid);
}
/* add all the rest */
if (!item->arg_len)
- strbuf_addch(, '\n');
+ strbuf_addch(buf, '\n');
else
- strbuf_addf(, " %.*s\n", item->arg_len, item->arg);
+ strbuf_addf(buf, " %.*s\n", item->arg_len, item->arg);
}
+}
- strbuf_reset(_list->buf);
- strbuf_add(_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, ,
[PATCH v2 04/16] sequencer: introduce todo_list_write_to_file()
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
---
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(, "%.*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(, command_to_char(item->command));
+ strbuf_addch(buf, command_to_char(item->command));
else
- strbuf_addstr(, 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(, " -c");
+ strbuf_addstr(buf, " -c");
else
- strbuf_addstr(, " -C");
+ strbuf_addstr(buf, " -C");
}
- strbuf_addf(, " %s", oid);
+ strbuf_addf(buf, " %s", oid);
}
/* add all the rest */
if (!item->arg_len)
- strbuf_addch(, '\n');
+ strbuf_addch(buf, '\n');
else
- strbuf_addf(, " %.*s\n", item->arg_len, item->arg);
+ strbuf_addf(buf, " %.*s\n", item->arg_len, item->arg);
}
+}
- strbuf_reset(_list->buf);
- strbuf_add(_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, , num, flags);
+
+ if (append_help) {
+ if (!edit_todo) {
+ strbuf_addch(, '\n');
+ strbuf_commented_addf(, 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, );
+ }
+
+ res = write_message(buf.buf, buf.len, file, 0);
strbuf_release();
- 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
