Re: [PATCH v6 16/16] rebase--interactive: move transform_todo_file() to rebase--interactive.c

[Alban Gruin <alban.gruin@gmail.com>]

Hi Phillip,

Le 01/02/2019 à 12:15, Phillip Wood a écrit :
> Hi Alban
> 
> On 29/01/2019 15:01, Alban Gruin wrote:
>> As transform_todo_file() is only needed inside of rebase--interactive.c,
>> it is moved there from sequencer.c.
> 
> I think I'd prefer to minimize the code under builtin and move this to
> rebase-interactive.c when it is modified earlier in the series. (I'd be
> quite happy if all the files in builtin just consisted of some option
> parsing followed by a call to run_git_foo() which resides in libgit)
> 

I do agree, but transform_todo_file() is only used by rebase -p.  Once
it is deprecated, it’s much easier to see this function is no longer
used if it’s marked as static thanks to -Werror=unused-function.

Now that I think about it, check_todo_list_from_file(),
rearrange_squash_in_todo_list(), and sequencer_add_exec_commands() are
only used by rebase -p, but I left them in sequencer.c.

> Also I wonder if we should be moving more functions (e.g.
> todo_list_write_file() and possibly add_exec_commands(),
> rearrange_squash() and the script generation) from sequencer.c to
> rebase-interactive.c when they're rewritten (possibly in a separate
> commit for ease of review) but I haven't looked if this is practical or
> if there are some dependencies that make that tricky. Unless there are
> some simple cases it should probably be a separate series.
> 

It might be doable, but I think it’s a bit more difficult with
sequencer_make_script() (and especially
sequencer_make_script_with_merges()).  I will explore this after this
series.

> Thanks for working on this series, it's great to see the todo list
> handling becoming more efficient.
> 

You’re welcome ;-)

-- Alban

> Best Wishes
> 
> Phillip
>