git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [GSoC][PATCH 0/1] rebase -i: rewrite the edit-todo functionality in C
@ 2018-06-11 13:57 Alban Gruin
  2018-06-11 13:57 ` [GSoC][PATCH 1/1] rebase--interactive: " Alban Gruin
  2018-06-13 15:22 ` [GSoC][PATCH v2 0/2] rebase -i: " Alban Gruin
  0 siblings, 2 replies; 19+ messages in thread
From: Alban Gruin @ 2018-06-11 13:57 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Johannes Schindelin,
	phillip.wood, Elijah Newren, Alban Gruin

This patch rewrites the edit-todo functionality from shell to C. This is
part of the effort to rewrite interactive rebase in C.

Alban Gruin (1):
  rebase--interactive: rewrite the edit-todo functionality in C

 builtin/rebase--helper.c   | 13 ++++++++-----
 git-rebase--interactive.sh | 11 +----------
 sequencer.c                | 31 +++++++++++++++++++++++++++++++
 sequencer.h                |  1 +
 4 files changed, 41 insertions(+), 15 deletions(-)

-- 
2.16.4


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [GSoC][PATCH 1/1] rebase--interactive: rewrite the edit-todo functionality in C
  2018-06-11 13:57 [GSoC][PATCH 0/1] rebase -i: rewrite the edit-todo functionality in C Alban Gruin
@ 2018-06-11 13:57 ` " Alban Gruin
  2018-06-11 15:32   ` Phillip Wood
  2018-06-12 15:46   ` Elijah Newren
  2018-06-13 15:22 ` [GSoC][PATCH v2 0/2] rebase -i: " Alban Gruin
  1 sibling, 2 replies; 19+ messages in thread
From: Alban Gruin @ 2018-06-11 13:57 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Johannes Schindelin,
	phillip.wood, Elijah Newren, Alban Gruin

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))
+		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);
+int edit_todo_list(unsigned flags);
 int skip_unnecessary_picks(void);
 int rearrange_squash(void);
 
-- 
2.16.4


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [GSoC][PATCH 1/1] rebase--interactive: rewrite the edit-todo functionality in C
  2018-06-11 13:57 ` [GSoC][PATCH 1/1] rebase--interactive: " Alban Gruin
@ 2018-06-11 15:32   ` Phillip Wood
  2018-06-12 12:33     ` Alban Gruin
  2018-06-12 15:46   ` Elijah Newren
  1 sibling, 1 reply; 19+ messages in thread
From: Phillip Wood @ 2018-06-11 15:32 UTC (permalink / raw)
  To: Alban Gruin, git
  Cc: Stefan Beller, Christian Couder, Johannes Schindelin,
	phillip.wood, Elijah Newren

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [GSoC][PATCH 1/1] rebase--interactive: rewrite the edit-todo functionality in C
  2018-06-11 15:32   ` Phillip Wood
@ 2018-06-12 12:33     ` Alban Gruin
  2018-06-12 16:20       ` Phillip Wood
  0 siblings, 1 reply; 19+ messages in thread
From: Alban Gruin @ 2018-06-12 12:33 UTC (permalink / raw)
  To: phillip.wood, git
  Cc: Stefan Beller, Christian Couder, Johannes Schindelin, Elijah Newren

Hi Phillip,

Le 11/06/2018 à 17:32, Phillip Wood a écrit :
>> +    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()
> 

It could be done by making launch_editor() and launch_sequence_editor()
some kind of wrapper around a function like launch_specified_editor()
(or something like that), that would take the editor as a parameter, in
addition to the path, the buffer and environment variables.  It would be
also very trivial to implement your first point above on top of that.

>>   int append_todo_help(unsigned edit_todo, unsigned keep_empty);
> 
> Can this declaration be removed now?

No, it’s still used in rebase--helper.c for now.

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

Cheers,
Alban


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [GSoC][PATCH 1/1] rebase--interactive: rewrite the edit-todo functionality in C
  2018-06-11 13:57 ` [GSoC][PATCH 1/1] rebase--interactive: " Alban Gruin
  2018-06-11 15:32   ` Phillip Wood
@ 2018-06-12 15:46   ` Elijah Newren
  2018-06-12 16:11     ` Alban Gruin
  1 sibling, 1 reply; 19+ messages in thread
From: Elijah Newren @ 2018-06-12 15:46 UTC (permalink / raw)
  To: Alban Gruin
  Cc: Git Mailing List, Stefan Beller, Christian Couder,
	Johannes Schindelin, Phillip Wood

Hi Alban,

On Mon, Jun 11, 2018 at 6:57 AM, Alban Gruin <alban.gruin@gmail.com> 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.

I looked over the patch and didn't see any problems (though I haven't
worked with rebase--helper before, or the code for todo list editing),
but when I went to apply the patch it failed telling me:

Applying: rebase--interactive: rewrite the edit-todo functionality in C
error: sha1 information is lacking or useless (builtin/rebase--helper.c).
error: could not build fake ancestor
Patch failed at 0001 rebase--interactive: rewrite the edit-todo
functionality in C
Use 'git am --show-current-patch' to see the failed patch

I tried each of master, next, and pu (as of today) to see if it might
apply there.  On which commit is this patch based?  (Do you have other
local commits that this was based on top of?)


Elijah

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [GSoC][PATCH 1/1] rebase--interactive: rewrite the edit-todo functionality in C
  2018-06-12 15:46   ` Elijah Newren
@ 2018-06-12 16:11     ` Alban Gruin
  0 siblings, 0 replies; 19+ messages in thread
From: Alban Gruin @ 2018-06-12 16:11 UTC (permalink / raw)
  To: Elijah Newren
  Cc: Git Mailing List, Stefan Beller, Christian Couder,
	Johannes Schindelin, Phillip Wood

Hi Elijah,

Le 12/06/2018 à 17:46, Elijah Newren a écrit :
> Hi Alban,
> 
> On Mon, Jun 11, 2018 at 6:57 AM, Alban Gruin <alban.gruin@gmail.com> 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.
> 
> I looked over the patch and didn't see any problems (though I haven't
> worked with rebase--helper before, or the code for todo list editing),
> but when I went to apply the patch it failed telling me:
> 
> Applying: rebase--interactive: rewrite the edit-todo functionality in C
> error: sha1 information is lacking or useless (builtin/rebase--helper.c).
> error: could not build fake ancestor
> Patch failed at 0001 rebase--interactive: rewrite the edit-todo
> functionality in C
> Use 'git am --show-current-patch' to see the failed patch
> 
> I tried each of master, next, and pu (as of today) to see if it might
> apply there.  On which commit is this patch based?  (Do you have other
> local commits that this was based on top of?)
> 
> 
> Elijah
> 

It can be applied on top of pu with my patch that rewrites
append_todo_help() in C
(https://public-inbox.org/git/20180607103012.22981-1-alban.gruin@gmail.com/)

Cheers,
Alban


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [GSoC][PATCH 1/1] rebase--interactive: rewrite the edit-todo functionality in C
  2018-06-12 12:33     ` Alban Gruin
@ 2018-06-12 16:20       ` Phillip Wood
  0 siblings, 0 replies; 19+ messages in thread
From: Phillip Wood @ 2018-06-12 16:20 UTC (permalink / raw)
  To: Alban Gruin, phillip.wood, git
  Cc: Stefan Beller, Christian Couder, Johannes Schindelin, Elijah Newren

Hi Alban

On 12/06/18 13:33, Alban Gruin wrote:
> Hi Phillip,
> 
> Le 11/06/2018 à 17:32, Phillip Wood a écrit :
>>> +    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()
>>
> 
> It could be done by making launch_editor() and launch_sequence_editor()
> some kind of wrapper around a function like launch_specified_editor()
> (or something like that), that would take the editor as a parameter, in
> addition to the path, the buffer and environment variables.  It would be
> also very trivial to implement your first point above on top of that.

That sounds like a good way forward, launch_sequence_editor() could just 
call launch_editor() if GIT_SEQUENCE_EDITOR and sequence.editor are not set.

>>>    int append_todo_help(unsigned edit_todo, unsigned keep_empty);
>>
>> Can this declaration be removed now?
> 
> No, it’s still used in rebase--helper.c for now.

Ah, sorry I missed that

Best Wishes

Phillip

> 
>>
>>> +int edit_todo_list(unsigned flags);
>>>    int skip_unnecessary_picks(void);
>>>    int rearrange_squash(void);
>>
>> Best Wishes
>>
>> Phillip
>>
> 
> Cheers,
> Alban
> 


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [GSoC][PATCH v2 0/2] rebase -i: rewrite the edit-todo functionality in C
  2018-06-11 13:57 [GSoC][PATCH 0/1] rebase -i: rewrite the edit-todo functionality in C Alban Gruin
  2018-06-11 13:57 ` [GSoC][PATCH 1/1] rebase--interactive: " Alban Gruin
@ 2018-06-13 15:22 ` " Alban Gruin
  2018-06-13 15:22   ` [GSoC][PATCH v2 1/2] editor: add a function to launch the sequence editor Alban Gruin
                     ` (3 more replies)
  1 sibling, 4 replies; 19+ messages in thread
From: Alban Gruin @ 2018-06-13 15:22 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Johannes Schindelin,
	phillip.wood, Elijah Newren, Alban Gruin

This patch rewrites the edit-todo functionality from shell to C. This is
part of the effort to rewrite interactive rebase in C.

Changes since v1:

 - Add a new function to launch the sequence editor, as advised by
   Phillip Wood[0]

[0] https://public-inbox.org/git/3bfd3470-4482-fe6a-2cd9-08311a0bbaac@talktalk.net/

Alban Gruin (2):
  editor: add a function to launch the sequence editor
  rebase--interactive: rewrite the edit-todo functionality in C

 builtin/rebase--helper.c   | 13 ++++++++-----
 cache.h                    |  1 +
 editor.c                   | 27 +++++++++++++++++++++++++--
 git-rebase--interactive.sh | 11 +----------
 sequencer.c                | 31 +++++++++++++++++++++++++++++++
 sequencer.h                |  1 +
 strbuf.h                   |  2 ++
 7 files changed, 69 insertions(+), 17 deletions(-)

-- 
2.16.4


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [GSoC][PATCH v2 1/2] editor: add a function to launch the sequence editor
  2018-06-13 15:22 ` [GSoC][PATCH v2 0/2] rebase -i: " Alban Gruin
@ 2018-06-13 15:22   ` Alban Gruin
  2018-06-13 15:22   ` [GSoC][PATCH v2 2/2] rebase--interactive: rewrite the edit-todo functionality in C Alban Gruin
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 19+ messages in thread
From: Alban Gruin @ 2018-06-13 15:22 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Johannes Schindelin,
	phillip.wood, Elijah Newren, Alban Gruin

As part of the rewrite of interactive rebase, the sequencer will need to
open the sequence editor to allow the user to edit the todo list.
Instead of duplicating the existing launch_editor() function, this
refactors it to a new function, launch_specified_editor(), which takes
the editor as a parameter, in addition to the path, the buffer and the
environment variables.  launch_sequence_editor() is then added to launch
the sequence editor.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 cache.h  |  1 +
 editor.c | 27 +++++++++++++++++++++++++--
 strbuf.h |  2 ++
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index 89a107a7f..c124849a1 100644
--- a/cache.h
+++ b/cache.h
@@ -1472,6 +1472,7 @@ extern const char *fmt_name(const char *name, const char *email);
 extern const char *ident_default_name(void);
 extern const char *ident_default_email(void);
 extern const char *git_editor(void);
+extern const char *git_sequence_editor(void);
 extern const char *git_pager(int stdout_is_tty);
 extern int is_terminal_dumb(void);
 extern int git_ident_config(const char *, const char *, void *);
diff --git a/editor.c b/editor.c
index 9a9b4e12d..c985eee1f 100644
--- a/editor.c
+++ b/editor.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "config.h"
 #include "strbuf.h"
 #include "run-command.h"
 #include "sigchain.h"
@@ -34,10 +35,21 @@ const char *git_editor(void)
 	return editor;
 }
 
-int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+const char *git_sequence_editor(void)
 {
-	const char *editor = git_editor();
+	const char *editor = getenv("GIT_SEQUENCE_EDITOR");
+
+	if (!editor)
+		git_config_get_string_const("sequence.editor", &editor);
+	if (!editor)
+		editor = git_editor();
 
+	return editor;
+}
+
+static int launch_specified_editor(const char *editor, const char *path,
+				   struct strbuf *buffer, const char *const *env)
+{
 	if (!editor)
 		return error("Terminal is dumb, but EDITOR unset");
 
@@ -95,3 +107,14 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		return error_errno("could not read file '%s'", path);
 	return 0;
 }
+
+int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+{
+	return launch_specified_editor(git_editor(), path, buffer, env);
+}
+
+int launch_sequence_editor(const char *path, struct strbuf *buffer,
+			   const char *const *env)
+{
+	return launch_specified_editor(git_sequence_editor(), path, buffer, env);
+}
diff --git a/strbuf.h b/strbuf.h
index 60a35aef1..66da9822f 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -575,6 +575,8 @@ extern void strbuf_add_unique_abbrev(struct strbuf *sb,
  * file's contents are not read into the buffer upon completion.
  */
 extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);
+extern int launch_sequence_editor(const char *path, struct strbuf *buffer,
+				  const char *const *env);
 
 extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size);
 
-- 
2.16.4


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [GSoC][PATCH v2 2/2] rebase--interactive: rewrite the edit-todo functionality in C
  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   ` Alban Gruin
  2018-06-14 20:14     ` Junio C Hamano
  2018-06-14  9:53   ` [GSoC][PATCH v2 0/2] rebase -i: " Phillip Wood
  2018-06-26 16:21   ` [GSoC][PATCH v3 " Alban Gruin
  3 siblings, 1 reply; 19+ messages in thread
From: Alban Gruin @ 2018-06-13 15:22 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Johannes Schindelin,
	phillip.wood, Elijah Newren, Alban Gruin

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..7cc76332e 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_sequence_editor(todo_file, NULL, NULL))
+		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);
+int edit_todo_list(unsigned flags);
 int skip_unnecessary_picks(void);
 int rearrange_squash(void);
 
-- 
2.16.4


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [GSoC][PATCH v2 0/2] rebase -i: rewrite the edit-todo functionality in C
  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  9:53   ` " Phillip Wood
  2018-06-26 16:21   ` [GSoC][PATCH v3 " Alban Gruin
  3 siblings, 0 replies; 19+ messages in thread
From: Phillip Wood @ 2018-06-14  9:53 UTC (permalink / raw)
  To: Alban Gruin, git
  Cc: Stefan Beller, Christian Couder, Johannes Schindelin,
	phillip.wood, Elijah Newren

Hi Alban

On 13/06/18 16:22, Alban Gruin wrote:
> This patch rewrites the edit-todo functionality from shell to C. This is
> part of the effort to rewrite interactive rebase in C.
> 
> Changes since v1:
> 
>  - Add a new function to launch the sequence editor, as advised by
>    Phillip Wood[0]

That's great, I think these look fine now

Best Wishes

Phillip

> [0] https://public-inbox.org/git/3bfd3470-4482-fe6a-2cd9-08311a0bbaac@talktalk.net/
> 
> Alban Gruin (2):
>   editor: add a function to launch the sequence editor
>   rebase--interactive: rewrite the edit-todo functionality in C
> 
>  builtin/rebase--helper.c   | 13 ++++++++-----
>  cache.h                    |  1 +
>  editor.c                   | 27 +++++++++++++++++++++++++--
>  git-rebase--interactive.sh | 11 +----------
>  sequencer.c                | 31 +++++++++++++++++++++++++++++++
>  sequencer.h                |  1 +
>  strbuf.h                   |  2 ++
>  7 files changed, 69 insertions(+), 17 deletions(-)
> 


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [GSoC][PATCH v2 2/2] rebase--interactive: rewrite the edit-todo functionality in C
  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
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2018-06-14 20:14 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Johannes Schindelin,
	phillip.wood, Elijah Newren

Alban Gruin <alban.gruin@gmail.com> writes:

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

Sorry, but where does "write_edit_todo = 0" in the preimage come from?

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [GSoC][PATCH v2 2/2] rebase--interactive: rewrite the edit-todo functionality in C
  2018-06-14 20:14     ` Junio C Hamano
@ 2018-06-14 20:24       ` Alban Gruin
  2018-06-14 20:42         ` Elijah Newren
  0 siblings, 1 reply; 19+ messages in thread
From: Alban Gruin @ 2018-06-14 20:24 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Stefan Beller, Christian Couder, Johannes Schindelin,
	phillip.wood, Elijah Newren

Hi Junio,

Le 14/06/2018 à 22:14, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
> 
>> 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;
> 
> Sorry, but where does "write_edit_todo = 0" in the preimage come from?
> 

It comes from my conversion of append_todo_help()[0], on which this
series is based.

[0]
https://public-inbox.org/git/20180607103012.22981-1-alban.gruin@gmail.com/

Cheers,
Alban


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [GSoC][PATCH v2 2/2] rebase--interactive: rewrite the edit-todo functionality in C
  2018-06-14 20:24       ` Alban Gruin
@ 2018-06-14 20:42         ` Elijah Newren
  0 siblings, 0 replies; 19+ messages in thread
From: Elijah Newren @ 2018-06-14 20:42 UTC (permalink / raw)
  To: Alban Gruin
  Cc: Junio C Hamano, Git Mailing List, Stefan Beller,
	Christian Couder, Johannes Schindelin, Phillip Wood

Hi Alban,

On Thu, Jun 14, 2018 at 1:24 PM, Alban Gruin <alban.gruin@gmail.com> wrote:
> Le 14/06/2018 à 22:14, Junio C Hamano a écrit :
>> Alban Gruin <alban.gruin@gmail.com> writes:
>>
>>> 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;
>>
>> Sorry, but where does "write_edit_todo = 0" in the preimage come from?
>>
>
> It comes from my conversion of append_todo_help()[0], on which this
> series is based.
>
> [0]
> https://public-inbox.org/git/20180607103012.22981-1-alban.gruin@gmail.com/

Given that both Junio and I had the same basic question, you'll
probably want to put dependency information into your cover letters.
If a series applies on master, then this isn't needed.  But if it
requires other changes that are in a topic in next or pu, you can name
the topic that it depends upon (e.g. ag/rebase-p, as from the "What's
cooking" emails or you can grab them as branchnames from
git://github.com/gitster/git).  If the dependency is something that
Junio hasn't picked up yet, provide a link to the other submission.

Others may have additional suggestions here...

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [GSoC][PATCH v3 0/2] rebase -i: rewrite the edit-todo functionality in C
  2018-06-13 15:22 ` [GSoC][PATCH v2 0/2] rebase -i: " Alban Gruin
                     ` (2 preceding siblings ...)
  2018-06-14  9:53   ` [GSoC][PATCH v2 0/2] rebase -i: " Phillip Wood
@ 2018-06-26 16:21   ` " Alban Gruin
  2018-06-26 16:21     ` [GSoC][PATCH v3 1/2] editor: add a function to launch the sequence editor Alban Gruin
                       ` (2 more replies)
  3 siblings, 3 replies; 19+ messages in thread
From: Alban Gruin @ 2018-06-26 16:21 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, Alban Gruin

This patch rewrites the edit-todo functionality from shell to C. This is
part of the effort to rewrite interactive rebase in C.

This patch is based on the fourth iteration of my series rewriting
append_todo_help() in C.

Changes since v2:

 - Moving edit_todo() from sequencer.c to interactive-rebase.c.

Alban Gruin (2):
  editor: add a function to launch the sequence editor
  rebase-interactive: rewrite the edit-todo functionality in C

 builtin/rebase--helper.c   | 13 ++++++++-----
 cache.h                    |  1 +
 editor.c                   | 27 +++++++++++++++++++++++++--
 git-rebase--interactive.sh | 11 +----------
 rebase-interactive.c       | 31 +++++++++++++++++++++++++++++++
 rebase-interactive.h       |  1 +
 strbuf.h                   |  2 ++
 7 files changed, 69 insertions(+), 17 deletions(-)

-- 
2.18.0


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [GSoC][PATCH v3 1/2] editor: add a function to launch the sequence editor
  2018-06-26 16:21   ` [GSoC][PATCH v3 " Alban Gruin
@ 2018-06-26 16:21     ` 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:45     ` [GSoC][PATCH v3 0/2] rebase -i: " Johannes Schindelin
  2 siblings, 0 replies; 19+ messages in thread
From: Alban Gruin @ 2018-06-26 16:21 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, Alban Gruin

As part of the rewrite of interactive rebase, the sequencer will need to
open the sequence editor to allow the user to edit the todo list.
Instead of duplicating the existing launch_editor() function, this
refactors it to a new function, launch_specified_editor(), which takes
the editor as a parameter, in addition to the path, the buffer and the
environment variables.  launch_sequence_editor() is then added to launch
the sequence editor.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 cache.h  |  1 +
 editor.c | 27 +++++++++++++++++++++++++--
 strbuf.h |  2 ++
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index 8b447652a..d70ae49ca 100644
--- a/cache.h
+++ b/cache.h
@@ -1409,6 +1409,7 @@ extern const char *fmt_name(const char *name, const char *email);
 extern const char *ident_default_name(void);
 extern const char *ident_default_email(void);
 extern const char *git_editor(void);
+extern const char *git_sequence_editor(void);
 extern const char *git_pager(int stdout_is_tty);
 extern int is_terminal_dumb(void);
 extern int git_ident_config(const char *, const char *, void *);
diff --git a/editor.c b/editor.c
index 9a9b4e12d..c985eee1f 100644
--- a/editor.c
+++ b/editor.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "config.h"
 #include "strbuf.h"
 #include "run-command.h"
 #include "sigchain.h"
@@ -34,10 +35,21 @@ const char *git_editor(void)
 	return editor;
 }
 
-int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+const char *git_sequence_editor(void)
 {
-	const char *editor = git_editor();
+	const char *editor = getenv("GIT_SEQUENCE_EDITOR");
+
+	if (!editor)
+		git_config_get_string_const("sequence.editor", &editor);
+	if (!editor)
+		editor = git_editor();
 
+	return editor;
+}
+
+static int launch_specified_editor(const char *editor, const char *path,
+				   struct strbuf *buffer, const char *const *env)
+{
 	if (!editor)
 		return error("Terminal is dumb, but EDITOR unset");
 
@@ -95,3 +107,14 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		return error_errno("could not read file '%s'", path);
 	return 0;
 }
+
+int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+{
+	return launch_specified_editor(git_editor(), path, buffer, env);
+}
+
+int launch_sequence_editor(const char *path, struct strbuf *buffer,
+			   const char *const *env)
+{
+	return launch_specified_editor(git_sequence_editor(), path, buffer, env);
+}
diff --git a/strbuf.h b/strbuf.h
index 60a35aef1..66da9822f 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -575,6 +575,8 @@ extern void strbuf_add_unique_abbrev(struct strbuf *sb,
  * file's contents are not read into the buffer upon completion.
  */
 extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);
+extern int launch_sequence_editor(const char *path, struct strbuf *buffer,
+				  const char *const *env);
 
 extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size);
 
-- 
2.18.0


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [GSoC][PATCH v3 2/2] rebase-interactive: rewrite the edit-todo functionality in C
  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     ` Alban Gruin
  2018-06-26 21:47       ` Johannes Schindelin
  2018-06-26 21:45     ` [GSoC][PATCH v3 0/2] rebase -i: " Johannes Schindelin
  2 siblings, 1 reply; 19+ messages in thread
From: Alban Gruin @ 2018-06-26 16:21 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, Alban Gruin

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 +----------
 rebase-interactive.c       | 31 +++++++++++++++++++++++++++++++
 rebase-interactive.h       |  1 +
 4 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 05e73e71d..731a64971 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,12 +13,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")),
@@ -28,8 +28,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"),
@@ -50,6 +48,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()
 	};
 
@@ -90,6 +91,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/rebase-interactive.c b/rebase-interactive.c
index c79c819b9..ace8e095b 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -66,3 +66,34 @@ 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_sequence_editor(todo_file, NULL, NULL))
+		return 1;
+
+	transform_todos(flags & ~(TODO_LIST_SHORTEN_IDS));
+
+	return 0;
+}
diff --git a/rebase-interactive.h b/rebase-interactive.h
index 47372624e..155219e74 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -2,5 +2,6 @@
 #define REBASE_INTERACTIVE_H
 
 int append_todo_help(unsigned edit_todo, unsigned keep_empty);
+int edit_todo_list(unsigned flags);
 
 #endif
-- 
2.18.0


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [GSoC][PATCH v3 0/2] rebase -i: rewrite the edit-todo functionality in C
  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:45     ` " Johannes Schindelin
  2 siblings, 0 replies; 19+ messages in thread
From: Johannes Schindelin @ 2018-06-26 21:45 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki, phillip.wood

Hi Alban,

On Tue, 26 Jun 2018, Alban Gruin wrote:

> This patch rewrites the edit-todo functionality from shell to C. This is
> part of the effort to rewrite interactive rebase in C.
> 
> This patch is based on the fourth iteration of my series rewriting
> append_todo_help() in C.
> 
> Changes since v2:
> 
>  - Moving edit_todo() from sequencer.c to interactive-rebase.c.

Excellent.

Thank you,
Dscho

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [GSoC][PATCH v3 2/2] rebase-interactive: rewrite the edit-todo functionality in C
  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
  0 siblings, 0 replies; 19+ messages in thread
From: Johannes Schindelin @ 2018-06-26 21:47 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki, phillip.wood

Hi Alban,

On Tue, 26 Jun 2018, 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>

Both patches are ACKed by me,
Dscho

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, back to index

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-11 13:57 [GSoC][PATCH 0/1] rebase -i: rewrite the edit-todo functionality in C Alban Gruin
2018-06-11 13:57 ` [GSoC][PATCH 1/1] rebase--interactive: " Alban Gruin
2018-06-11 15:32   ` Phillip Wood
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

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