git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Phillip Wood <phillip.wood@talktalk.net>
To: Alban Gruin <alban.gruin@gmail.com>, git@vger.kernel.org
Cc: Stefan Beller <sbeller@google.com>,
	Christian Couder <christian.couder@gmail.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	phillip.wood@dunelm.org.uk, Elijah Newren <newren@gmail.com>
Subject: Re: [GSoC][PATCH 1/1] rebase--interactive: rewrite the edit-todo functionality in C
Date: Mon, 11 Jun 2018 16:32:50 +0100
Message-ID: <3bfd3470-4482-fe6a-2cd9-08311a0bbaac@talktalk.net> (raw)
In-Reply-To: <20180611135714.29378-2-alban.gruin@gmail.com>

Hi Alban, it's great to see you making progress with this.

I don't want to add to your workload but a couple of things that might 
be nice to have in the future would be

1) avoid rewriting the todo list and running the editor if 
GIT_SEQUENCE_EDITOR is ':', especially when creating the todo list for 
implicit interactive rebases.

2) have --edit-todo warn if the user drops commits, in the same way as 
rebase does for the initial edit of the todo list. This should also make 
it easier to use the same code for the initial edit as well as when the 
user does 'rebase --edit-todo'

I've got a couple of comments on this patch below.

On 11/06/18 14:57, Alban Gruin wrote:
> This rewrites the edit-todo functionality from shell to C.
> 
> To achieve that, a new command mode, `edit-todo`, is added, and the
> `write-edit-todo` flag is removed, as the shell script does not need to
> write the edit todo help message to the todo list anymore.
> 
> The shell version is then stripped in favour of a call to the helper.
> 
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>   builtin/rebase--helper.c   | 13 ++++++++-----
>   git-rebase--interactive.sh | 11 +----------
>   sequencer.c                | 31 +++++++++++++++++++++++++++++++
>   sequencer.h                |  1 +
>   4 files changed, 41 insertions(+), 15 deletions(-)
> 
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index ded5e291d..d2990b210 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {
>   int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>   {
>   	struct replay_opts opts = REPLAY_OPTS_INIT;
> -	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo = 0;
> +	unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
>   	int abbreviate_commands = 0, rebase_cousins = -1;
>   	enum {
>   		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
>   		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
> -		ADD_EXEC, APPEND_TODO_HELP
> +		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
>   	} command = 0;
>   	struct option options[] = {
>   		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
> @@ -27,8 +27,6 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>   		OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
>   		OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
>   			 N_("keep original branch points of cousins")),
> -		OPT_BOOL(0, "write-edit-todo", &write_edit_todo,
> -			 N_("append the edit-todo message to the todo-list")),
>   		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
>   				CONTINUE),
>   		OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
> @@ -49,6 +47,9 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>   			N_("insert exec commands in todo list"), ADD_EXEC),
>   		OPT_CMDMODE(0, "append-todo-help", &command,
>   			    N_("insert the help in the todo list"), APPEND_TODO_HELP),
> +		OPT_CMDMODE(0, "edit-todo", &command,
> +			    N_("edit the todo list during an interactive rebase"),
> +			    EDIT_TODO),
>   		OPT_END()
>   	};
>   
> @@ -89,6 +90,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>   	if (command == ADD_EXEC && argc == 2)
>   		return !!sequencer_add_exec_commands(argv[1]);
>   	if (command == APPEND_TODO_HELP && argc == 1)
> -		return !!append_todo_help(write_edit_todo, keep_empty);
> +		return !!append_todo_help(0, keep_empty);
> +	if (command == EDIT_TODO && argc == 1)
> +		return !!edit_todo_list(flags);
>   	usage_with_options(builtin_rebase_helper_usage, options);
>   }
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 94c23a7af..2defe607f 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -108,16 +108,7 @@ initiate_action () {
>   		     --continue
>   		;;
>   	edit-todo)
> -		git stripspace --strip-comments <"$todo" >"$todo".new
> -		mv -f "$todo".new "$todo"
> -		collapse_todo_ids
> -		git rebase--helper --append-todo-help --write-edit-todo
> -
> -		git_sequence_editor "$todo" ||
> -			die "$(gettext "Could not execute editor")"
> -		expand_todo_ids
> -
> -		exit
> +		exec git rebase--helper --edit-todo
>   		;;
>   	show-current-patch)
>   		exec git show REBASE_HEAD --
> diff --git a/sequencer.c b/sequencer.c
> index 1ffd990f7..1c1799c91 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4386,6 +4386,37 @@ int append_todo_help(unsigned edit_todo, unsigned keep_empty)
>   	return ret;
>   }
>   
> +int edit_todo_list(unsigned flags)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	const char *todo_file = rebase_path_todo();
> +	FILE *todo;
> +
> +	if (strbuf_read_file(&buf, todo_file, 0) < 0)
> +		return error_errno(_("could not read '%s'."), todo_file);
> +
> +	strbuf_stripspace(&buf, 1);
> +	todo = fopen_or_warn(todo_file, "w");
> +	if (!todo) {
> +		strbuf_release(&buf);
> +		return 1;
> +	}
> +
> +	strbuf_write(&buf, todo);
> +	fclose(todo);
> +	strbuf_release(&buf);
> +
> +	transform_todos(flags | TODO_LIST_SHORTEN_IDS);
> +	append_todo_help(1, 0);
> +
> +	if (launch_editor(todo_file, NULL, NULL))

I'm not sure that this will respect GIT_SEQUENCE_EDITOR, it would be 
nice to have a launch_sequence_editor() function that shared as much 
code as possible with launch_editor()

> +		return 1;
> +
> +	transform_todos(flags & ~(TODO_LIST_SHORTEN_IDS));
> +
> +	return 0;
> +}
> +
>   static int rewrite_file(const char *path, const char *buf, size_t len)
>   {
>   	int rc = 0;
> diff --git a/sequencer.h b/sequencer.h
> index e14f6590e..35730b13e 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -81,6 +81,7 @@ int sequencer_add_exec_commands(const char *command);
>   int transform_todos(unsigned flags);
>   int check_todo_list(void);
>   int append_todo_help(unsigned edit_todo, unsigned keep_empty);

Can this declaration be removed now?

> +int edit_todo_list(unsigned flags);
>   int skip_unnecessary_picks(void);
>   int rearrange_squash(void);

Best Wishes

Phillip


  reply index

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-11 13:57 [GSoC][PATCH 0/1] rebase -i: " Alban Gruin
2018-06-11 13:57 ` [GSoC][PATCH 1/1] rebase--interactive: " Alban Gruin
2018-06-11 15:32   ` Phillip Wood [this message]
2018-06-12 12:33     ` Alban Gruin
2018-06-12 16:20       ` Phillip Wood
2018-06-12 15:46   ` Elijah Newren
2018-06-12 16:11     ` Alban Gruin
2018-06-13 15:22 ` [GSoC][PATCH v2 0/2] rebase -i: " Alban Gruin
2018-06-13 15:22   ` [GSoC][PATCH v2 1/2] editor: add a function to launch the sequence editor Alban Gruin
2018-06-13 15:22   ` [GSoC][PATCH v2 2/2] rebase--interactive: rewrite the edit-todo functionality in C Alban Gruin
2018-06-14 20:14     ` Junio C Hamano
2018-06-14 20:24       ` Alban Gruin
2018-06-14 20:42         ` Elijah Newren
2018-06-14  9:53   ` [GSoC][PATCH v2 0/2] rebase -i: " Phillip Wood
2018-06-26 16:21   ` [GSoC][PATCH v3 " Alban Gruin
2018-06-26 16:21     ` [GSoC][PATCH v3 1/2] editor: add a function to launch the sequence editor Alban Gruin
2018-06-26 16:21     ` [GSoC][PATCH v3 2/2] rebase-interactive: rewrite the edit-todo functionality in C Alban Gruin
2018-06-26 21:47       ` Johannes Schindelin
2018-06-26 21:45     ` [GSoC][PATCH v3 0/2] rebase -i: " Johannes Schindelin

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3bfd3470-4482-fe6a-2cd9-08311a0bbaac@talktalk.net \
    --to=phillip.wood@talktalk.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=alban.gruin@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=newren@gmail.com \
    --cc=phillip.wood@dunelm.org.uk \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox