*[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
` (16 more replies)0 siblings, 17 replies; 190+ 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
^permalinkrawreply [flat|nested] 190+ 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
2018-10-27 21:29 ` [PATCH v2 00/16] " Alban Gruin
16 siblings, 0 replies; 190+ 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”.
^permalinkrawreply [flat|nested] 190+ 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 Gruin0 siblings, 1 reply; 190+ 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
>
^permalinkrawreply [flat|nested] 190+ 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 Gruin0 siblings, 0 replies; 190+ 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
^permalinkrawreply [flat|nested] 190+ messages in thread

*[PATCH v2 00/16] 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
` (15 preceding siblings ...)
2018-10-07 20:51 ` [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
@ 2018-10-27 21:29 ` " Alban Gruin
2018-10-27 21:29 ` [PATCH v2 01/16] sequencer: changes in parse_insn_buffer() Alban Gruin
` (17 more replies)16 siblings, 18 replies; 190+ messages in thread
From: Alban Gruin @ 2018-10-27 21:29 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.
It is based onto ag/rebase-i-in-c (34b4731, "rebase -i: move
rebase--helper modes to rebase--interactive").
Changes since v1:
- When a line is invalid, parse_insn_buffer() sets the type of the
corresponding command to a garbage value instead of `noop', and its
argument is defined properly.
- todo_list_add_exec_commands(), todo_list_rearrange_squash(),
skip_unnecessary_picks() don’t reparse the todo list after processing
them. Instead, they recreate a new item list.
- Due to the previous change, a todo list buffer can’t directly be
written to the disk. A new function, todo_list_write_to_disk(), is
introduced to take care of this task.
- rewrite_file() has been deleted.
- A call to strbuf_addf(&buf, "\n"); has been replaced by strbuf_addch(…).
- complete_action() and todo_list_check() expect that their input todo list
have already been parsed.
- complete_action() no longer writes "noop\n" to the todo list buffer
if it is empty. Instead, it appends a `noop' command to the item
list.
Alban Gruin (16):
sequencer: changes in parse_insn_buffer()
sequencer: make the todo_list structure public
sequencer: refactor transform_todos() to work on a todo_list
sequencer: introduce todo_list_write_to_file()
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: 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_write_to_file() 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 | 68 +++-
rebase-interactive.c | 142 ++++++--
rebase-interactive.h | 8 +-
sequencer.c | 589 +++++++++++++---------------------
sequencer.h | 68 +++-
5 files changed, 455 insertions(+), 420 deletions(-)
--
2.19.1
^permalinkrawreply [flat|nested] 190+ messages in thread

*Re: [PATCH v2 00/16] sequencer: refactor functions working on a todo_list
2018-10-27 21:29 ` [PATCH v2 00/16] " Alban Gruin
` (15 preceding siblings ...)
2018-10-27 21:29 ` [PATCH v2 16/16] rebase--interactive: move transform_todo_file() to rebase--interactive.c Alban Gruin
@ 2018-10-29 3:05 ` Junio C Hamano
2018-10-29 15:34 ` Alban Gruin
2018-11-09 8:07 ` [PATCH v3 " Alban Gruin
17 siblings, 1 reply; 190+ messages in thread
From: Junio C Hamano @ 2018-10-29 3:05 UTC (permalink / raw)
To: Alban Gruin; +Cc: git, Johannes Schindelin, Phillip Wood
Alban Gruin <alban.gruin@gmail.com> writes:
> 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.
>
> It is based onto ag/rebase-i-in-c (34b4731, "rebase -i: move
> rebase--helper modes to rebase--interactive").
As there are quite a lot of fixes to the sequencer machinery since
that topic forked from the mainline. For example, [06/16] has
unpleasant merge conflicts with 1ace63bc ("rebase --exec: make it
work with --rebase-merges", 2018-08-09) that has been in master for
the past couple of months. IOW, the tip of ag/rebase-i-in-c is a
bit too old a base to work on by now.
I think I queued the previous round on the result of merging
ag/rebase-i-in-c into master, i.e. 61dc7b24 ("Merge branch
'ag/rebase-i-in-c' into ag/sequencer-reduce-rewriting-todo",
2018-10-09). That may be a more reasonable place to start this
update on.
^permalinkrawreply [flat|nested] 190+ messages in thread

*Re: [PATCH v2 00/16] sequencer: refactor functions working on a todo_list
2018-10-29 3:05 ` [PATCH v2 00/16] sequencer: refactor functions working on a todo_list Junio C Hamano
@ 2018-10-29 15:34 ` Alban Gruin0 siblings, 0 replies; 190+ messages in thread
From: Alban Gruin @ 2018-10-29 15:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin, Phillip Wood
Hi Junio,
Le 29/10/2018 à 04:05, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
[…]
>> It is based onto ag/rebase-i-in-c (34b4731, "rebase -i: move
>> rebase--helper modes to rebase--interactive").
>
> As there are quite a lot of fixes to the sequencer machinery since
> that topic forked from the mainline. For example, [06/16] has
> unpleasant merge conflicts with 1ace63bc ("rebase --exec: make it
> work with --rebase-merges", 2018-08-09) that has been in master for
> the past couple of months. IOW, the tip of ag/rebase-i-in-c is a
> bit too old a base to work on by now.
>
> I think I queued the previous round on the result of merging
> ag/rebase-i-in-c into master, i.e. 61dc7b24 ("Merge branch
> 'ag/rebase-i-in-c' into ag/sequencer-reduce-rewriting-todo",
> 2018-10-09). That may be a more reasonable place to start this
> update on.
>
Right.
My next iteration will be based on 61dc7b24, I just rebased my branch
onto it.
^permalinkrawreply [flat|nested] 190+ messages in thread

*Re: [PATCH v2 04/16] sequencer: introduce todo_list_write_to_file()
2018-10-30 16:28 ` Phillip Wood@ 2018-11-01 23:31 ` Alban Gruin0 siblings, 0 replies; 190+ messages in thread
From: Alban Gruin @ 2018-11-01 23:31 UTC (permalink / raw)
To: phillip.wood, git; +Cc: Johannes Schindelin, Junio C Hamano
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
^permalinkrawreply [flat|nested] 190+ messages in thread

*Re: [PATCH v2 06/16] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
2018-10-30 16:47 ` Phillip Wood@ 2018-11-01 23:31 ` Alban Gruin
2018-11-02 10:09 ` Phillip Wood0 siblings, 1 reply; 190+ messages in thread
From: Alban Gruin @ 2018-11-01 23:31 UTC (permalink / raw)
To: phillip.wood, git; +Cc: Johannes Schindelin, Junio C Hamano
Le 30/10/2018 à 17:47, Phillip Wood a écrit :
> On 27/10/2018 22:29, Alban Gruin wrote:
>> This refactors sequencer_add_exec_commands() to work on a todo_list to
>> avoid redundant reads and writes to the disk.
>>
>> An obvious way to do this would be to insert the `exec' command between
>> the other commands, and reparse it once this is done. This is not what
>> is done here. Instead, the command is appended to the buffer once, and
>> a new list of items is created. Items from the old list are copied to
>> it, and new `exec' items are appended when necessary. This eliminates
>> the need to reparse the todo list, but this also means its buffer cannot
>> be directly written to the disk, hence todo_list_write_to_disk().
>
> I'd reword this slightly, maybe
>
> Instead of just inserting the `exec' command between the other commands,
> and re-parsing the buffer at the end the exec command is appended to the
> buffer once, and a new list of items is created. Items from the old
> list are copied across and new `exec' items are appended when necessary.
> This eliminates the need to reparse the buffer, but this also means we
> have to use todo_list_write_to_disk() to write the file.
>
>> 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.
>
> I think the saying 'reparses' is confusing as that is what we're trying
> to avoid.
>
>> complete_action() still uses sequencer_add_exec_commands() for now.
>> This will be changed in a future commit.
>>
>> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
>> ---
>> sequencer.c | 69 +++++++++++++++++++++++++++++++++++++----------------
>> 1 file changed, 49 insertions(+), 20 deletions(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index e12860c047..12a3efeca8 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -4216,6 +4216,50 @@ int sequencer_make_script(FILE *out, int argc,
>> const char **argv,
>> return 0;
>> }
>> +static void todo_list_add_exec_commands(struct todo_list *todo_list,
>> + const char *commands)
>> +{
>> + struct strbuf *buf = &todo_list->buf;
>> + const char *old_buf = buf->buf;
>> + size_t commands_len = strlen(commands + strlen("exec ")) - 1;
>> + int i, first = 1, nr = 0, alloc = 0;
>
> Minor nit pick, I think it is clearer if first is initialized just
> before the loop as it is in the deleted code below.
>
>> + struct todo_item *items = NULL,
>> + base_item = {TODO_EXEC, NULL, 0, 0, commands_len, 0};
>> +
>> + strbuf_addstr(buf, commands);
>> + base_item.offset_in_buf = buf->len - commands_len - 1;
>> + base_item.arg = buf->buf + base_item.offset_in_buf;
>
> I think if the user gives --exec more than once on the command line then
> commands will contain more than one exec command so this needs to parse
> commands and create one todo_item for each command.
>
Ouch, you’re right. Thanks for the heads up.
>> +
>> + /*
>> + * 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.
>> + */
>> + for (i = 0; i < todo_list->nr; i++) {
>> + enum todo_command command = todo_list->items[i].command;
>> + if (todo_list->items[i].arg)
>> + todo_list->items[i].arg = todo_list->items[i].arg -
>> old_buf + buf->buf;
>> +
>> + if (command == TODO_PICK && !first) {
>> + ALLOC_GROW(items, nr + 1, alloc);
>> + memcpy(items + nr++, &base_item, sizeof(struct todo_item));
>
> I think it would be clearer to say
> items[nr++] = base_item;
> rather than using memcpy. This applies below and to some of the other
> patches as well. Also this needs to loop over all the base_items if the
> user gave --exec more than once on the command line.
>
I agree with you, it’s way more readable, IMO. But for some reason, I
thought it was not possible to assign a struct to another in C.
> Best Wishes
>
> Phillip
>
Cheers,
Alban
^permalinkrawreply [flat|nested] 190+ messages in thread

*Re: [PATCH v2 06/16] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
2018-11-01 23:31 ` Alban Gruin@ 2018-11-02 10:09 ` Phillip Wood
2018-11-02 16:26 ` Alban Gruin0 siblings, 1 reply; 190+ messages in thread
From: Phillip Wood @ 2018-11-02 10:09 UTC (permalink / raw)
To: Alban Gruin, phillip.wood, git; +Cc: Johannes Schindelin, Junio C Hamano
Hi Alban
On 01/11/2018 23:31, Alban Gruin wrote:
> Le 30/10/2018 à 17:47, Phillip Wood a écrit :
>> On 27/10/2018 22:29, Alban Gruin wrote:
>>> This refactors sequencer_add_exec_commands() to work on a todo_list to
>>> avoid redundant reads and writes to the disk.
>>>
>>> An obvious way to do this would be to insert the `exec' command between
>>> the other commands, and reparse it once this is done. This is not what
>>> is done here. Instead, the command is appended to the buffer once, and
>>> a new list of items is created. Items from the old list are copied to
>>> it, and new `exec' items are appended when necessary. This eliminates
>>> the need to reparse the todo list, but this also means its buffer cannot
>>> be directly written to the disk, hence todo_list_write_to_disk().
>>
>> I'd reword this slightly, maybe
>>
>> Instead of just inserting the `exec' command between the other commands,
>> and re-parsing the buffer at the end the exec command is appended to the
>> buffer once, and a new list of items is created. Items from the old
>> list are copied across and new `exec' items are appended when necessary.
>> This eliminates the need to reparse the buffer, but this also means we
>> have to use todo_list_write_to_disk() to write the file.
>>
>>> 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.
>>
>> I think the saying 'reparses' is confusing as that is what we're trying
>> to avoid.
>>
>>> complete_action() still uses sequencer_add_exec_commands() for now.
>>> This will be changed in a future commit.
>>>
>>> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
>>> ---
>>> sequencer.c | 69 +++++++++++++++++++++++++++++++++++++----------------
>>> 1 file changed, 49 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/sequencer.c b/sequencer.c
>>> index e12860c047..12a3efeca8 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -4216,6 +4216,50 @@ int sequencer_make_script(FILE *out, int argc,
>>> const char **argv,
>>> return 0;
>>> }
>>> +static void todo_list_add_exec_commands(struct todo_list *todo_list,
>>> + const char *commands)
>>> +{
>>> + struct strbuf *buf = &todo_list->buf;
>>> + const char *old_buf = buf->buf;
>>> + size_t commands_len = strlen(commands + strlen("exec ")) - 1;
>>> + int i, first = 1, nr = 0, alloc = 0;
>>
>> Minor nit pick, I think it is clearer if first is initialized just
>> before the loop as it is in the deleted code below.
>>
>>> + struct todo_item *items = NULL,
>>> + base_item = {TODO_EXEC, NULL, 0, 0, commands_len, 0};
>>> +
>>> + strbuf_addstr(buf, commands);
>>> + base_item.offset_in_buf = buf->len - commands_len - 1;
>>> + base_item.arg = buf->buf + base_item.offset_in_buf;
>>
>> I think if the user gives --exec more than once on the command line then
>> commands will contain more than one exec command so this needs to parse
>> commands and create one todo_item for each command.
>>
>
> Ouch, you’re right. Thanks for the heads up.
I haven't looked how difficult it would be but it might be best to
change the option parsing to pass an array of strings containing the
exec commands rather than one long string so we can just loop over the
array here.
>
>>> +
>>> + /*
>>> + * 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.
>>> + */
>>> + for (i = 0; i < todo_list->nr; i++) {
>>> + enum todo_command command = todo_list->items[i].command;
>>> + if (todo_list->items[i].arg)
>>> + todo_list->items[i].arg = todo_list->items[i].arg -
>>> old_buf + buf->buf;
>>> +
>>> + if (command == TODO_PICK && !first) {
>>> + ALLOC_GROW(items, nr + 1, alloc);
>>> + memcpy(items + nr++, &base_item, sizeof(struct todo_item));
>>
>> I think it would be clearer to say
>> items[nr++] = base_item;
>> rather than using memcpy. This applies below and to some of the other
>> patches as well. Also this needs to loop over all the base_items if the
>> user gave --exec more than once on the command line.
>>
>
> I agree with you, it’s way more readable, IMO. But for some reason, I
> thought it was not possible to assign a struct to another in C.
In general one needs to be careful as it is only does a shallow copy but
that is exactly what we want here. Having said that if we have an array
of exec commands to insert then it might be worth sticking with memcpy()
here so the whole array can be copied in one go.
Best Wishes
Phillip
>> Best Wishes
>>
>> Phillip
>>
>
> Cheers,
> Alban
>
^permalinkrawreply [flat|nested] 190+ messages in thread

*Re: [PATCH v2 06/16] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
2018-11-02 10:09 ` Phillip Wood@ 2018-11-02 16:26 ` Alban Gruin
2018-11-02 17:09 ` Phillip Wood0 siblings, 1 reply; 190+ messages in thread
From: Alban Gruin @ 2018-11-02 16:26 UTC (permalink / raw)
To: phillip.wood, git; +Cc: Johannes Schindelin, Junio C Hamano
Hi Phillip,
Le 02/11/2018 à 11:09, Phillip Wood a écrit :
>>>> + struct todo_item *items = NULL,
>>>> + base_item = {TODO_EXEC, NULL, 0, 0, commands_len, 0};
>>>> +
>>>> + strbuf_addstr(buf, commands);
>>>> + base_item.offset_in_buf = buf->len - commands_len - 1;
>>>> + base_item.arg = buf->buf + base_item.offset_in_buf;
>>>
>>> I think if the user gives --exec more than once on the command line then
>>> commands will contain more than one exec command so this needs to parse
>>> commands and create one todo_item for each command.
>>>
>>
>> Ouch, you’re right. Thanks for the heads up.
>
> I haven't looked how difficult it would be but it might be best to
> change the option parsing to pass an array of strings containing the
> exec commands rather than one long string so we can just loop over the
> array here.
>
It would be the best way to do so. This string comes from git-rebase.sh
(or builtin/rebase.c) -- they format it this way before invoking
git-rebase--interactive. So either I modify both of them (for this, I
would need to rebase my branch on master), or I can split this string in
builtin/rebase--interactive.c. I prefer the first option, but maybe
changing the base of this series will not please Junio.
Cheers,
Alban
^permalinkrawreply [flat|nested] 190+ messages in thread

*Re: [PATCH v2 06/16] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
2018-11-02 16:26 ` Alban Gruin@ 2018-11-02 17:09 ` Phillip Wood0 siblings, 0 replies; 190+ messages in thread
From: Phillip Wood @ 2018-11-02 17:09 UTC (permalink / raw)
To: Alban Gruin, phillip.wood, git; +Cc: Johannes Schindelin, Junio C Hamano
Hi Alban
On 02/11/2018 16:26, Alban Gruin wrote:
> Hi Phillip,
>
> Le 02/11/2018 à 11:09, Phillip Wood a écrit :
>>>>> + struct todo_item *items = NULL,
>>>>> + base_item = {TODO_EXEC, NULL, 0, 0, commands_len, 0};
>>>>> +
>>>>> + strbuf_addstr(buf, commands);
>>>>> + base_item.offset_in_buf = buf->len - commands_len - 1;
>>>>> + base_item.arg = buf->buf + base_item.offset_in_buf;
>>>>
>>>> I think if the user gives --exec more than once on the command line then
>>>> commands will contain more than one exec command so this needs to parse
>>>> commands and create one todo_item for each command.
>>>>
>>>
>>> Ouch, you’re right. Thanks for the heads up.
>>
>> I haven't looked how difficult it would be but it might be best to
>> change the option parsing to pass an array of strings containing the
>> exec commands rather than one long string so we can just loop over the
>> array here.
>>
>
> It would be the best way to do so. This string comes from git-rebase.sh
> (or builtin/rebase.c) -- they format it this way before invoking
> git-rebase--interactive. So either I modify both of them (for this, I
> would need to rebase my branch on master), or I can split this string in
> builtin/rebase--interactive.c. I prefer the first option, but maybe
> changing the base of this series will not please Junio.
I think in the last 'what's cooking' email Junio said he was going to
merge all the builtin/rebase.c stuff to master so there may not be a
problem if you wait a couple of days.
Best Wishes
Phillip
> Cheers,
> Alban
>
^permalinkrawreply [flat|nested] 190+ messages in thread

*[PATCH v3 00/16] sequencer: refactor functions working on a todo_list
2018-10-27 21:29 ` [PATCH v2 00/16] " Alban Gruin
` (16 preceding siblings ...)
2018-10-29 3:05 ` [PATCH v2 00/16] sequencer: refactor functions working on a todo_list Junio C Hamano
@ 2018-11-09 8:07 ` " Alban Gruin
2018-11-09 8:07 ` [PATCH v3 01/16] sequencer: changes in parse_insn_buffer() Alban Gruin
` (16 more replies)17 siblings, 17 replies; 190+ messages in thread
From: Alban Gruin @ 2018-11-09 8:07 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.
This is based on 8858448bb4 ("Ninth batch for 2.20", 2018-11-06) to
avoid conflicts with 'js/rebase-i-break'.
Changes since v2:
- Clarifying some commit messages
- Reducing the number of parameters in todo_list_write_to_file()
- sequencer_add_exec_commands() now requests a string list instead of a
string.
- Replacing calls to memcpy() by shallow copies
- Applying array.cocci.patch
Alban Gruin (16):
sequencer: changes in parse_insn_buffer()
sequencer: make the todo_list structure public
sequencer: refactor transform_todos() to work on a todo_list
sequencer: introduce todo_list_write_to_file()
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: 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_write_to_file() 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 | 81 +++--
rebase-interactive.c | 142 ++++++--
rebase-interactive.h | 8 +-
sequencer.c | 615 +++++++++++++---------------------
sequencer.h | 75 ++++-
5 files changed, 481 insertions(+), 440 deletions(-)
Range-diff against v2:
1: 55fa1fff03 = 1: 9ae965b73e sequencer: changes in parse_insn_buffer()
2: 192fb771ed < -: ---------- sequencer: make the todo_list structure public
-: ---------- > 2: 9c15cdede4 sequencer: make the todo_list structure public
3: e2f821ffbe = 3: a5c01d5a95 sequencer: refactor transform_todos() to work on a todo_list
4: 0001e8dbde < -: ---------- sequencer: introduce todo_list_write_to_file()
-: ---------- > 4: f2781fe4c3 sequencer: introduce todo_list_write_to_file()
5: aa9554d81d = 5: 337e0dce57 sequencer: refactor check_todo_list() to work on a todo_list
6: 8bd793caf5 < -: ---------- sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
7: e984946cef < -: ---------- sequencer: refactor rearrange_squash() to work on a todo_list
8: cf05254acf < -: ---------- sequencer: make sequencer_make_script() write its script to a strbuf
9: ac79151792 < -: ---------- sequencer: change complete_action() to use the refactored functions
10: e227e38b24 < -: ---------- sequencer: refactor skip_unnecessary_picks() to work on a todo_list
11: 5bfd9d3460 < -: ---------- rebase-interactive: use todo_list_write_to_file() in edit_todo_list()
12: f4d7578a77 < -: ---------- rebase-interactive: append_todo_help() changes
13: 11c43f1dfe < -: ---------- rebase-interactive: rewrite edit_todo_list() to handle the initial edit
14: e9a6396c26 < -: ---------- sequencer: use edit_todo_list() in complete_action()
-: ---------- > 6: 2c75eed153 sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
-: ---------- > 7: 90e01bc713 sequencer: refactor rearrange_squash() to work on a todo_list
-: ---------- > 8: 850261fbf7 sequencer: make sequencer_make_script() write its script to a strbuf
-: ---------- > 9: 7bcfe98241 sequencer: change complete_action() to use the refactored functions
-: ---------- > 10: 50cdea7ef7 sequencer: refactor skip_unnecessary_picks() to work on a todo_list
-: ---------- > 11: 6d4fcf96e0 rebase-interactive: use todo_list_write_to_file() in edit_todo_list()
-: ---------- > 12: 3bea4eb9ba rebase-interactive: append_todo_help() changes
-: ---------- > 13: bfe8f568ee rebase-interactive: rewrite edit_todo_list() to handle the initial edit
-: ---------- > 14: 0d6499b7c8 sequencer: use edit_todo_list() in complete_action()
15: a36d094610 = 15: 21d3d1abd7 sequencer: fix a call to error() in transform_todo_file()
16: 344e8ceeed < -: ---------- rebase--interactive: move transform_todo_file() to rebase--interactive.c
-: ---------- > 16: 22fd27fa2f rebase--interactive: move transform_todo_file() to rebase--interactive.c
--
2.19.1.872.ga867da739e
^permalinkrawreply [flat|nested] 190+ messages in thread

*[PATCH v4 00/16] sequencer: refactor functions working on a todo_list
2018-11-09 8:07 ` [PATCH v3 " Alban Gruin
` (15 preceding siblings ...)
2018-11-09 8:08 ` [PATCH v3 16/16] rebase--interactive: move transform_todo_file() to rebase--interactive.c Alban Gruin
@ 2018-12-29 16:03 ` Alban Gruin
2018-12-29 16:03 ` [PATCH v4 01/16] sequencer: changes in parse_insn_buffer() Alban Gruin
` (16 more replies)16 siblings, 17 replies; 190+ messages in thread
From: Alban Gruin @ 2018-12-29 16:03 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.
This is based on nd/the-index (36e7ed69de, "rebase-interactive.c: remove
the_repository references"), as it introduced a lot of conflicts. The
result does not conflict with pu (e31bc98f4b, "Merge branch
'md/list-objects-filter-by-depth' into pu").
Changes since v3:
- Replacing the 'arg' field from todo_item by 'arg_offset' to avoid
dealing with pointers on the todo list buffer in
todo_list_add_exec_commands(). This has led to some additionnal
changes.
- Rewording some commits.
- Dropping the commit "sequencer: fix a call to error() in
transform_todo_file()". The call to error() after reading the todo
file is replaced by error_errno() in "sequencer: refactor
transform_todos() to work on a todo_list". The same change has been
applied to sequencer_add_exec_commands() in "sequencer: refactor
sequencer_add_exec_commands() to work on a todo_list".
- transform_todo_file(), sequencer_add_exec_commands() and
rearrange_squash_in_todo_file() now print an error if they fail to
write to the todo file.
- A lot of changes were introduced by the conflict resolution with
nd/the-index.
Alban Gruin (16):
sequencer: changes in parse_insn_buffer()
sequencer: make the todo_list structure public
sequencer: remove the 'arg' field from todo_item
sequencer: refactor transform_todos() to work on a todo_list
sequencer: introduce todo_list_write_to_file()
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: 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_write_to_file() 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()
rebase--interactive: move transform_todo_file() to
rebase--interactive.c
builtin/rebase--interactive.c | 90 +++--
rebase-interactive.c | 143 +++++--
rebase-interactive.h | 8 +-
sequencer.c | 677 +++++++++++++---------------------
sequencer.h | 82 +++-
5 files changed, 525 insertions(+), 475 deletions(-)
--
2.20.1
^permalinkrawreply [flat|nested] 190+ messages in thread

*[PATCH v5 00/16] sequencer: refactor functions working on a todo_list
2018-12-29 16:03 ` [PATCH v4 00/16] sequencer: refactor functions working on a todo_list Alban Gruin
` (15 preceding siblings ...)
2018-12-29 16:04 ` [PATCH v4 16/16] rebase--interactive: move transform_todo_file() to rebase--interactive.c Alban Gruin
@ 2019-01-23 20:58 ` Alban Gruin
2019-01-23 20:58 ` [PATCH v5 01/16] sequencer: changes in parse_insn_buffer() Alban Gruin
` (17 more replies)16 siblings, 18 replies; 190+ messages in thread
From: Alban Gruin @ 2019-01-23 20:58 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.
This is based on nd/the-index (36e7ed69de, "rebase-interactive.c: remove
the_repository references").
Changes since v4:
- Adding todo_item_get_arg() to return the address of the parameter of
an item to avoid confusion.
- Squashed <d414e1a2-e5a1-ce15-96b5-cf294c7f3c92@ramsayjones.plus.com>
and <5440ddf5-0b80-3d00-7daf-133a8611efa8@ramsayjones.plus.com> from
Ramsay Jones.
Alban Gruin (16):
sequencer: changes in parse_insn_buffer()
sequencer: make the todo_list structure public
sequencer: remove the 'arg' field from todo_item
sequencer: refactor transform_todos() to work on a todo_list
sequencer: introduce todo_list_write_to_file()
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: 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_write_to_file() 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()
rebase--interactive: move transform_todo_file() to
rebase--interactive.c
builtin/rebase--interactive.c | 90 +++--
rebase-interactive.c | 143 +++++--
rebase-interactive.h | 9 +-
sequencer.c | 687 ++++++++++++++--------------------
sequencer.h | 81 +++-
5 files changed, 533 insertions(+), 477 deletions(-)
--
2.20.1
^permalinkrawreply [flat|nested] 190+ messages in thread

*Re: [PATCH v5 00/16] sequencer: refactor functions working on a todo_list
2019-01-24 21:54 ` [PATCH v5 00/16] sequencer: refactor functions working on a todo_list Junio C Hamano
@ 2019-01-24 22:43 ` Alban Gruin0 siblings, 0 replies; 190+ messages in thread
From: Alban Gruin @ 2019-01-24 22:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin, Phillip Wood
[I’m resending this as I clicked on the wrong button…]
Hi,
Le 24/01/2019 à 22:54, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
>
> Before I comment on anything else.
>
> > This is based on nd/the-index (36e7ed69de, "rebase-interactive.c: remove
> > the_repository references").
>
> My attempt to apply these in order on top of that commit seems to
> stop at step 5/16. Are you sure you based them on it?
>
> Thanks.
It is based on that commit, but I mistakenly added a newline between `--- a/
sequencer.c` and `+++ b/sequencer.c` before sending this series. Sorry about
this.
I just reapplied all the patches I sent, and there is no other problem.
Should I send this back?
-- Alban
^permalinkrawreply [flat|nested] 190+ messages in thread

*[PATCH v6 00/16] sequencer: refactor functions working on a todo_list
2019-01-23 20:58 ` [PATCH v5 00/16] sequencer: refactor functions working on a todo_list Alban Gruin
` (16 preceding siblings ...)
2019-01-24 21:54 ` [PATCH v5 00/16] sequencer: refactor functions working on a todo_list Junio C Hamano
@ 2019-01-29 15:01 ` " Alban Gruin
2019-01-29 15:01 ` [PATCH v6 01/16] sequencer: changes in parse_insn_buffer() Alban Gruin
` (16 more replies)17 siblings, 17 replies; 190+ messages in thread
From: Alban Gruin @ 2019-01-29 15:01 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.
This is based on nd/the-index (36e7ed69de, "rebase-interactive.c: remove
the_repository references").
I am rerolling this because I accidentally added a newline to a place I
shouldn’t in the patch 05/16 of the v5. There is no change in this
version.
Alban Gruin (16):
sequencer: changes in parse_insn_buffer()
sequencer: make the todo_list structure public
sequencer: remove the 'arg' field from todo_item
sequencer: refactor transform_todos() to work on a todo_list
sequencer: introduce todo_list_write_to_file()
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: 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_write_to_file() 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()
rebase--interactive: move transform_todo_file() to
rebase--interactive.c
builtin/rebase--interactive.c | 90 +++--
rebase-interactive.c | 143 +++++--
rebase-interactive.h | 9 +-
sequencer.c | 687 ++++++++++++++--------------------
sequencer.h | 81 +++-
5 files changed, 533 insertions(+), 477 deletions(-)
--
2.20.1
^permalinkrawreply [flat|nested] 190+ messages in thread