Re: [GSoC][PATCH 0/2] rebase -i: rewrite append_todo_help() in C

From: Phillip Wood <phillip.wood@talktalk.net>
Date: May 31, 2018, 17:48 UTC
Message-ID: <a6d6e5c1-e7ea-1131-c798-9d5e44603d87@talktalk.net>
In-Reply-To: <20180531110130.18839-1-alban.gruin@gmail.com>

Hi Alban, it's great to see you working on this

On 31/05/18 12:01, Alban Gruin wrote:

Show 6 quoted lines

> This series rewrites append_todo_help() from shell to C. This is part
> of the effort to rewrite interactive rebase in C.
> 
> The first commit rewrites append_todo_help() in C (the C version
> covers a bit more than the old shell version), adds some parameters to
> rebase--helper, etc.

I've had a read of the first patch and I think it looks fine, my only comment would be that the help for '--edit-todo' is a bit misleading at the moment as currently it's just a flag to tell rebase-helper that the todo list is being edited rather than actually implementing the functionality to edit the list (but hopefully that will follow in the future).

Show 5 quoted lines

> 
> The second one strips newlines from append_todo_help() messages, which
> require to update the translations. This change was advised to me by
> Stefan Beller, but Johannes Schindelin voiced concerns. I don’t really
> have a strong opinion about it, so feel free to give yours.

I'm not sure I understand what the point of this patch is, if the newlines are unnecessary then I'd just omit them from the first patch - am I missing something?

Best Wishes

Phillip

Show 10 quoted lines

> Alban Gruin (2):
>   rebase--interactive: rewrite append_todo_help() in C
>   sequencer: remove newlines from append_todo_help() messages
> 
>  builtin/rebase--helper.c   | 10 ++++++--
>  git-rebase--interactive.sh | 52 ++-----------------------------------
>  sequencer.c                | 64 ++++++++++++++++++++++++++++++++++++++++++++++
>  sequencer.h                |  1 +
>  4 files changed, 75 insertions(+), 52 deletions(-)
>