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

From: Alban Gruin <alban.gruin@gmail.com>
Date: Feb 2, 2019, 15:05 UTC
Message-ID: <fd7ab34c-3a3f-57e3-0d90-0e97fbbffd3b@gmail.com>
In-Reply-To: <d00c11bc-013e-5839-9766-6999e99edf34@talktalk.net>

Hi Phillip,

Le 01/02/2019 à 12:15, Phillip Wood a écrit :

Show 11 quoted lines

> 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.

Show 8 quoted lines

> 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
>