[v2,00/16] sequencer: refactor functions working on a todo_list
mbox series

Message ID 20181027212930.9303-1-alban.gruin@gmail.com
Headers show
Series
  • sequencer: refactor functions working on a todo_list
Related show

Message

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

Comments

Junio C Hamano Oct. 29, 2018, 3:05 a.m. UTC | #1
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.
Alban Gruin Oct. 29, 2018, 3:34 p.m. UTC | #2
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.