threads / patch / 48613

patch, 2 partsrebase -i: rewrite append_todo_help() in C

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

## tl;dr

27 messages between May 31, 2018 and Jun 28, 2018. Diffs are folded; open one to read it.

replies: 26people: 5as markdown or json

Alban Gruin· May 31, 2018, 11:01 UTC · lore

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.

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.

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(-)
-- 
2.16.4
Alban Gruin· May 31, 2018, 11:01 UTC · re: Alban Gruin · lore

[GSoC][PATCH 1/2] rebase--interactive: rewrite append_todo_help() in C

This rewrites append_todo_help() from shell to C. It also incorporates some parts of initiate_action() and complete_action() that also write help texts to the todo file.

Two flags are added to rebase--helper.c: one to call append_todo_help() (`--append-todo-help`), and another one to tell append_todo_help() to write the help text suited for the edit-todo mode (`--edit-todo`).

Finally, append_todo_help() is removed from git-rebase--interactive.sh to use `rebase--helper --append-todo-help` instead.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase--helper.c   | 10 ++++++--
 git-rebase--interactive.sh | 52 ++--------------------------------------
 sequencer.c                | 60 ++++++++++++++++++++++++++++++++++++++++++++++
 sequencer.h                |  1 +
 4 files changed, 71 insertions(+), 52 deletions(-)
Show changes to 4 files +71 −52

builtin/rebase--helper.c, git-rebase--interactive.sh, sequencer.c, sequencer.h

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f7c2a5fdc..ad3a3a7eb 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;
+	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, edit_todo = 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
+		ADD_EXEC, APPEND_TODO_HELP
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -27,6 +27,8 @@ 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, "edit-todo", &edit_todo,
+			 N_("edit the todo list during an interactive rebase")),
 		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
 				CONTINUE),
 		OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
@@ -45,6 +47,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
 		OPT_CMDMODE(0, "add-exec-commands", &command,
 			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_END()
 	};
 
@@ -84,5 +88,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!rearrange_squash();
 	if (command == ADD_EXEC && argc == 2)
 		return !!sequencer_add_exec_commands(argv[1]);
+	if (command == APPEND_TODO_HELP && argc == 1)
+		return !!append_todo_help(edit_todo, keep_empty);
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 9884ecd71..419c54068 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -39,38 +39,6 @@ comment_for_reflog () {
 	esac
 }
 
-append_todo_help () {
-	gettext "
-Commands:
-p, pick <commit> = use commit
-r, reword <commit> = use commit, but edit the commit message
-e, edit <commit> = use commit, but stop for amending
-s, squash <commit> = use commit, but meld into previous commit
-f, fixup <commit> = like \"squash\", but discard this commit's log message
-x, exec <commit> = run command (the rest of the line) using shell
-d, drop <commit> = remove commit
-l, label <label> = label current HEAD with a name
-t, reset <label> = reset HEAD to a label
-m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]
-.       create a merge commit using the original merge commit's
-.       message (or the oneline, if no original merge commit was
-.       specified). Use -c <commit> to reword the commit message.
-
-These lines can be re-ordered; they are executed from top to bottom.
-" | git stripspace --comment-lines >>"$todo"
-
-	if test $(get_missing_commit_check_level) = error
-	then
-		gettext "
-Do not remove any line. Use 'drop' explicitly to remove a commit.
-" | git stripspace --comment-lines >>"$todo"
-	else
-		gettext "
-If you remove a line here THAT COMMIT WILL BE LOST.
-" | git stripspace --comment-lines >>"$todo"
-	fi
-}
-
 die_abort () {
 	apply_autostash
 	rm -rf "$state_dir"
@@ -143,13 +111,7 @@ initiate_action () {
 		git stripspace --strip-comments <"$todo" >"$todo".new
 		mv -f "$todo".new "$todo"
 		collapse_todo_ids
-		append_todo_help
-		gettext "
-You are editing the todo file of an ongoing interactive rebase.
-To continue rebase after editing, run:
-    git rebase --continue
-
-" | git stripspace --comment-lines >>"$todo"
+		git rebase--helper --append-todo-help --edit-todo
 
 		git_sequence_editor "$todo" ||
 			die "$(gettext "Could not execute editor")"
@@ -220,17 +182,7 @@ $comment_char $(eval_ngettext \
 	"Rebase \$shortrevisions onto \$shortonto (\$todocount commands)" \
 	"$todocount")
 EOF
-	append_todo_help
-	gettext "
-	However, if you remove everything, the rebase will be aborted.
-
-	" | git stripspace --comment-lines >>"$todo"
-
-	if test -z "$keep_empty"
-	then
-		printf '%s\n' "$comment_char $(gettext "Note that empty commits are commented out")" >>"$todo"
-	fi
-
+	git rebase--helper --append-todo-help ${keep_empty:+--keep-empty}
 
 	has_action "$todo" ||
 		return 2
diff --git a/sequencer.c b/sequencer.c
index 01e561bc2..9b291845e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4223,6 +4223,66 @@ int check_todo_list(void)
 	return res;
 }
 
+int append_todo_help(unsigned edit_todo, unsigned keep_empty)
+{
+	struct strbuf buf = STRBUF_INIT;
+	FILE *todo;
+	int ret;
+	const char *msg = _("\nCommands:\n"
+"p, pick <commit> = use commit\n"
+"r, reword <commit> = use commit, but edit the commit message\n"
+"e, edit <commit> = use commit, but stop for amending\n"
+"s, squash <commit> = use commit, but meld into previous commit\n"
+"f, fixup <commit> = like \"squash\", but discard this commit's log message\n"
+"x, exec <command> = run command (the rest of the line) using shell\n"
+"d, drop <commit> = remove commit\n"
+"l, label <label> = label current HEAD with a name\n"
+"t, reset <label> = reset HEAD to a label\n"
+"m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]\n"
+".       create a merge commit using the original merge commit's\n"
+".       message (or the oneline, if no original merge commit was\n"
+".       specified). Use -c <commit> to reword the commit message.\n"
+"\n"
+"These lines can be re-ordered; they are executed from top to bottom.\n");
+
+	todo = fopen_or_warn(rebase_path_todo(), "a");
+	if (!todo)
+		return 1;
+
+	strbuf_add_commented_lines(&buf, msg, strlen(msg));
+
+	if (get_missing_commit_check_level() == CHECK_ERROR)
+		msg = _("\nDo not remove any line. Use 'drop' "
+			 "explicitly to remove a commit.\n");
+	else
+		msg = _("\nIf you remove a line here "
+			 "THAT COMMIT WILL BE LOST.\n");
+
+	strbuf_add_commented_lines(&buf, msg, strlen(msg));
+
+	if (edit_todo)
+		msg = _("\nYou are editing the todo file "
+			"of an ongoing interactive rebase.\n"
+			"To continue rebase after editing, run:\n"
+			"    git rebase --continue\n\n");
+	else
+		msg = _("\nHowever, if you remove everything, "
+			"the rebase will be aborted.\n\n");
+
+	strbuf_add_commented_lines(&buf, msg, strlen(msg));
+
+	if (!keep_empty) {
+		msg = _("Note that empty commits are commented out");
+		strbuf_add_commented_lines(&buf, msg, strlen(msg));
+	}
+
+	ret = fputs(buf.buf, todo);
+	fclose(todo);
+	strbuf_release(&buf);
+
+	return ret;
+}
+
 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 4b2717881..c1cd01c50 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -76,6 +76,7 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 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 skip_unnecessary_picks(void);
 int rearrange_squash(void);
 
-- 
2.16.4
Alban Gruin· May 31, 2018, 11:01 UTC · re: Alban Gruin · lore

[GSoC][PATCH 2/2] sequencer: remove newlines from append_todo_help() messages

This removes newlines before and after the messages in append_todo_help(). This is done to avoid confusions from translators.

These newlines are now inserted with `strbuf_add_commented_lines()`. Messages were ended by two newlines characters, but here we only insert one at a time. This is because `strbuf_add_commented_lines()` automatically inserts a newline at the end of the input, and ignore the last from the input.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)
Show changes to sequencer.c +14 −10
diff --git a/sequencer.c b/sequencer.c
index 9b291845e..9ab6c28d7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4228,7 +4228,7 @@ int append_todo_help(unsigned edit_todo, unsigned keep_empty)
 	struct strbuf buf = STRBUF_INIT;
 	FILE *todo;
 	int ret;
-	const char *msg = _("\nCommands:\n"
+	const char *msg = _("Commands:\n"
 "p, pick <commit> = use commit\n"
 "r, reword <commit> = use commit, but edit the commit message\n"
 "e, edit <commit> = use commit, but stop for amending\n"
@@ -4243,33 +4243,37 @@ int append_todo_help(unsigned edit_todo, unsigned keep_empty)
 ".       message (or the oneline, if no original merge commit was\n"
 ".       specified). Use -c <commit> to reword the commit message.\n"
 "\n"
-"These lines can be re-ordered; they are executed from top to bottom.\n");
+"These lines can be re-ordered; they are executed from top to bottom.");
 
 	todo = fopen_or_warn(rebase_path_todo(), "a");
 	if (!todo)
 		return 1;
 
+	strbuf_add_commented_lines(&buf, "\n", 1);
 	strbuf_add_commented_lines(&buf, msg, strlen(msg));
 
 	if (get_missing_commit_check_level() == CHECK_ERROR)
-		msg = _("\nDo not remove any line. Use 'drop' "
-			 "explicitly to remove a commit.\n");
+		msg = _("Do not remove any line. Use 'drop' "
+			 "explicitly to remove a commit.");
 	else
-		msg = _("\nIf you remove a line here "
-			 "THAT COMMIT WILL BE LOST.\n");
+		msg = _("If you remove a line here "
+			 "THAT COMMIT WILL BE LOST.");
 
+	strbuf_add_commented_lines(&buf, "\n", 1);
 	strbuf_add_commented_lines(&buf, msg, strlen(msg));
 
 	if (edit_todo)
-		msg = _("\nYou are editing the todo file "
+		msg = _("You are editing the todo file "
 			"of an ongoing interactive rebase.\n"
 			"To continue rebase after editing, run:\n"
-			"    git rebase --continue\n\n");
+			"    git rebase --continue");
 	else
-		msg = _("\nHowever, if you remove everything, "
-			"the rebase will be aborted.\n\n");
+		msg = _("However, if you remove everything, "
+			"the rebase will be aborted.");
 
+	strbuf_add_commented_lines(&buf, "\n", 1);
 	strbuf_add_commented_lines(&buf, msg, strlen(msg));
+	strbuf_add_commented_lines(&buf, "\n", 1);
 
 	if (!keep_empty) {
 		msg = _("Note that empty commits are commented out");
-- 
2.16.4
Phillip Wood· May 31, 2018, 17:48 UTC · re: Alban Gruin · lore

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

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(-)
> 
Stefan Beller· May 31, 2018, 18:44 UTC · re: Phillip Wood · lore

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

On Thu, May 31, 2018 at 10:48 AM, Phillip Wood <phillip.wood@talktalk.net> wrote:

Show 16 quoted lines
> Hi Alban, it's great to see you working on this
>
> On 31/05/18 12:01, Alban Gruin wrote:
>> 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).
Would you have better suggestions for the name of the flag?
Of the top of my head:
  --write-edit-todo
  --hint-todo-edit
  --include-todo-edit-hint
not sure I like these names, though they seem to reflect the
nature of that flag a little bit better.
Show 9 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?
>

The new lines are part of the output and are currently in the part to be translated: For example from the German translation file:

#: git-rebase--interactive.sh:171 msgid "" "\n" "Do not remove any line. Use 'drop' explicitly to remove a commit.\n" msgstr "" "\n" "Keine Zeile entfernen. Benutzen Sie 'drop', um explizit einen Commit zu\n" "entfernen.\n"

After patch 2 is applied, the translators only see "Do not remove any line. Use 'drop' explicitly to remove a commit." as a need to translate, and the two additional new lines (one in front and one after the string) are just put in place autormatically.

Usually we do not want to play sentence lego, but this is a whole sentence for translation; it is rather about formatting the output for the terminal, adding new lines to separate some messages.

I thought this patch would just show goodwill towards translators that do not need to replicate the formatting exactly.

If you feel strongly, I'd rather see Alban drop this second patch and move on instead of waiting for our argument to settle. ( I do not feel strongly about it, but put it out as a suggestion as that seemed like it would lead to a better end state for the project).

Thanks, Stefan

Alban Gruin· May 31, 2018, 19:33 UTC · re: Stefan Beller · lore

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

Hi Stefan,
Le 31/05/2018 à 20:44, Stefan Beller a écrit :
Show 27 quoted lines
> On Thu, May 31, 2018 at 10:48 AM, Phillip Wood
> <phillip.wood@talktalk.net> wrote:
>> Hi Alban, it's great to see you working on this
>>
>> On 31/05/18 12:01, Alban Gruin wrote:
>>> 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).
> 
> Would you have better suggestions for the name of the flag?
> Of the top of my head:
>   --write-edit-todo
>   --hint-todo-edit
>   --include-todo-edit-hint
> not sure I like these names, though they seem to reflect the
> nature of that flag a little bit better.
> 

As my next patch series will probably be about rewriting edit-todo in C, do you really think I should rename the flag?

Show 5 quoted lines
> If you feel strongly, I'd rather see Alban drop this second patch and
> move on instead of waiting for our argument to settle. ( I do not feel
> strongly about it, but put it out as a suggestion as that seemed like
> it would lead to a better end state for the project).
> 
Okay, so I drop this patch and reroll the other?

Cheers, Alban

Stefan Beller· May 31, 2018, 19:41 UTC · re: Alban Gruin · lore

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

On Thu, May 31, 2018 at 12:33 PM, Alban Gruin <alban.gruin@gmail.com> wrote:
Show 33 quoted lines
> Hi Stefan,
>
> Le 31/05/2018 à 20:44, Stefan Beller a écrit :
>> On Thu, May 31, 2018 at 10:48 AM, Phillip Wood
>> <phillip.wood@talktalk.net> wrote:
>>> Hi Alban, it's great to see you working on this
>>>
>>> On 31/05/18 12:01, Alban Gruin wrote:
>>>> 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).
>>
>> Would you have better suggestions for the name of the flag?
>> Of the top of my head:
>>   --write-edit-todo
>>   --hint-todo-edit
>>   --include-todo-edit-hint
>> not sure I like these names, though they seem to reflect the
>> nature of that flag a little bit better.
>>
>
> As my next patch series will probably be about rewriting edit-todo in C,
> do you really think I should rename the flag?

If you reroll, you could think of doing that. If you have the next series prepared already that build on top, it may not be worth it.

Show 7 quoted lines
>> If you feel strongly, I'd rather see Alban drop this second patch and
>> move on instead of waiting for our argument to settle. ( I do not feel
>> strongly about it, but put it out as a suggestion as that seemed like
>> it would lead to a better end state for the project).
>>
>
> Okay, so I drop this patch and reroll the other?
Sure, but maybe give Philip some time to react?
Phillip Wood· Jun 1, 2018, 09:44 UTC · re: Stefan Beller · lore

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

On 31/05/18 19:44, Stefan Beller wrote:
Show 66 quoted lines
> On Thu, May 31, 2018 at 10:48 AM, Phillip Wood
> <phillip.wood@talktalk.net> wrote:
>> Hi Alban, it's great to see you working on this
>>
>> On 31/05/18 12:01, Alban Gruin wrote:
>>> 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).
> 
> Would you have better suggestions for the name of the flag?
> Of the top of my head:
>    --write-edit-todo
>    --hint-todo-edit
>    --include-todo-edit-hint
> not sure I like these names, though they seem to reflect the
> nature of that flag a little bit better.
> 
>>> 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?
>>
> 
> The new lines are part of the output and are currently in the part to
> be translated:
> For example from the German translation file:
> 
> #: git-rebase--interactive.sh:171
> msgid ""
> "\n"
> "Do not remove any line. Use 'drop' explicitly to remove a commit.\n"
> msgstr ""
> "\n"
> "Keine Zeile entfernen. Benutzen Sie 'drop', um explizit einen Commit zu\n"
> "entfernen.\n"
> 
> After patch 2 is applied, the translators only see
> "Do not remove any line. Use 'drop' explicitly to remove a commit."
> as a need to translate, and the two additional new lines (one in front
> and one after the string) are just put in place autormatically.
> 
> Usually we do not want to play sentence lego, but this is a whole
> sentence for translation; it is rather about formatting the output for
> the terminal, adding new lines to separate some messages.
> 
> I thought this patch would just show goodwill towards translators
> that do not need to replicate the formatting exactly.
> 
> If you feel strongly, I'd rather see Alban drop this second patch and
> move on instead of waiting for our argument to settle. ( I do not feel
> strongly about it, but put it out as a suggestion as that seemed like
> it would lead to a better end state for the project).

Thanks for the explanation, I see what you're trying to do. I don't have a strong feeling either way, I can see the potential advantage but as it changes strings that are currently translated I'm not sure it is cost free. Do you know how the translators feel about the change as they're the ones it is aimed at?

Best Wishes
Phillip
> 
> Thanks,
> Stefan
> 
Stefan Beller· Jun 1, 2018, 19:46 UTC · re: Phillip Wood · lore

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

Show 5 quoted lines
> Thanks for the explanation, I see what you're trying to do. I don't have a
> strong feeling either way, I can see the potential advantage but as it
> changes strings that are currently translated I'm not sure it is cost free.
> Do you know how the translators feel about the change as they're the ones it
> is aimed at?
No, not at all.
Hence I am being vague on how much we desire it.
Alban Gruin· May 31, 2018, 19:25 UTC · re: Phillip Wood · lore

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

Hi Phillip,
Le 31/05/2018 à 19:48, Phillip Wood a écrit :
Show 15 quoted lines
> Hi Alban, it's great to see you working on this
> 
> On 31/05/18 12:01, Alban Gruin wrote:
>> 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

Right, what do you think about something like “appends the edit-todo message to the todo list”?

> (but hopefully that will follow in the
> future).
> 
This is the next step :)

Cheers, Alban

Phillip Wood· Jun 1, 2018, 09:38 UTC · re: Alban Gruin · lore

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

On 31/05/18 20:25, Alban Gruin wrote:
Show 21 quoted lines
> Hi Phillip,
> 
> Le 31/05/2018 à 19:48, Phillip Wood a écrit :
>> Hi Alban, it's great to see you working on this
>>
>> On 31/05/18 12:01, Alban Gruin wrote:
>>> 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
> 
> Right, what do you think about something like “appends the edit-todo
> message to the todo list”?

Yes that sounds good, though if you're about to implement editing the todo list with --edit-todo I wouldn't worry too much

Best Wishes
Phillip
Show 9 quoted lines
>> (but hopefully that will follow in the
>> future).
>>
> 
> This is the next step :)
> 
> Cheers,
> Alban
> 
Alban Gruin· Jun 5, 2018, 12:53 UTC · re: Alban Gruin · lore

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

This patch rewrites append_todo_help() from shell to C. The C version covers a bit more than the old shell version. To achieve that, some parameters were added to rebase--helper.

This is part of the effort to rewrite interactive rebase in C.
Changes since v1:
 - Renaming the parameter to insert the edit-todo message from
   `edit-todo` to `write-edit-todo`.
 - Clarifying the `write-edit-todo` help message.
 - Dropping the commit that removed newlines in the messages.
Alban Gruin (1):
  rebase--interactive: rewrite append_todo_help() in C
 builtin/rebase--helper.c   | 10 ++++++--
 git-rebase--interactive.sh | 52 ++--------------------------------------
 sequencer.c                | 60 ++++++++++++++++++++++++++++++++++++++++++++++
 sequencer.h                |  1 +
 4 files changed, 71 insertions(+), 52 deletions(-)
-- 
2.16.4
Alban Gruin· Jun 5, 2018, 12:53 UTC · re: Alban Gruin · lore

[GSoC][PATCH v2 1/1] rebase--interactive: rewrite append_todo_help() in C

This rewrites append_todo_help() from shell to C. It also incorporates some parts of initiate_action() and complete_action() that also write help texts to the todo file.

Two flags are added to rebase--helper.c: one to call append_todo_help() (`--append-todo-help`), and another one to tell append_todo_help() to write the help text suited for the edit-todo mode (`--write-edit-todo`).

Finally, append_todo_help() is removed from git-rebase--interactive.sh to use `rebase--helper --append-todo-help` instead.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase--helper.c   | 10 ++++++--
 git-rebase--interactive.sh | 52 ++--------------------------------------
 sequencer.c                | 60 ++++++++++++++++++++++++++++++++++++++++++++++
 sequencer.h                |  1 +
 4 files changed, 71 insertions(+), 52 deletions(-)
Show changes to 4 files +71 −52

builtin/rebase--helper.c, git-rebase--interactive.sh, sequencer.c, sequencer.h

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f7c2a5fdc..7ef92fbb6 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;
+	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, edit_todo = 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
+		ADD_EXEC, APPEND_TODO_HELP
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -27,6 +27,8 @@ 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", &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"),
@@ -45,6 +47,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
 		OPT_CMDMODE(0, "add-exec-commands", &command,
 			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_END()
 	};
 
@@ -84,5 +88,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!rearrange_squash();
 	if (command == ADD_EXEC && argc == 2)
 		return !!sequencer_add_exec_commands(argv[1]);
+	if (command == APPEND_TODO_HELP && argc == 1)
+		return !!append_todo_help(edit_todo, keep_empty);
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 299ded213..94c23a7af 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -39,38 +39,6 @@ comment_for_reflog () {
 	esac
 }
 
-append_todo_help () {
-	gettext "
-Commands:
-p, pick <commit> = use commit
-r, reword <commit> = use commit, but edit the commit message
-e, edit <commit> = use commit, but stop for amending
-s, squash <commit> = use commit, but meld into previous commit
-f, fixup <commit> = like \"squash\", but discard this commit's log message
-x, exec <command> = run command (the rest of the line) using shell
-d, drop <commit> = remove commit
-l, label <label> = label current HEAD with a name
-t, reset <label> = reset HEAD to a label
-m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]
-.       create a merge commit using the original merge commit's
-.       message (or the oneline, if no original merge commit was
-.       specified). Use -c <commit> to reword the commit message.
-
-These lines can be re-ordered; they are executed from top to bottom.
-" | git stripspace --comment-lines >>"$todo"
-
-	if test $(get_missing_commit_check_level) = error
-	then
-		gettext "
-Do not remove any line. Use 'drop' explicitly to remove a commit.
-" | git stripspace --comment-lines >>"$todo"
-	else
-		gettext "
-If you remove a line here THAT COMMIT WILL BE LOST.
-" | git stripspace --comment-lines >>"$todo"
-	fi
-}
-
 die_abort () {
 	apply_autostash
 	rm -rf "$state_dir"
@@ -143,13 +111,7 @@ initiate_action () {
 		git stripspace --strip-comments <"$todo" >"$todo".new
 		mv -f "$todo".new "$todo"
 		collapse_todo_ids
-		append_todo_help
-		gettext "
-You are editing the todo file of an ongoing interactive rebase.
-To continue rebase after editing, run:
-    git rebase --continue
-
-" | git stripspace --comment-lines >>"$todo"
+		git rebase--helper --append-todo-help --write-edit-todo
 
 		git_sequence_editor "$todo" ||
 			die "$(gettext "Could not execute editor")"
@@ -220,17 +182,7 @@ $comment_char $(eval_ngettext \
 	"Rebase \$shortrevisions onto \$shortonto (\$todocount commands)" \
 	"$todocount")
 EOF
-	append_todo_help
-	gettext "
-	However, if you remove everything, the rebase will be aborted.
-
-	" | git stripspace --comment-lines >>"$todo"
-
-	if test -z "$keep_empty"
-	then
-		printf '%s\n' "$comment_char $(gettext "Note that empty commits are commented out")" >>"$todo"
-	fi
-
+	git rebase--helper --append-todo-help ${keep_empty:+--keep-empty}
 
 	has_action "$todo" ||
 		return 2
diff --git a/sequencer.c b/sequencer.c
index b522523d4..c9d488b11 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4336,6 +4336,66 @@ int check_todo_list(void)
 	return res;
 }
 
+int append_todo_help(unsigned edit_todo, unsigned keep_empty)
+{
+	struct strbuf buf = STRBUF_INIT;
+	FILE *todo;
+	int ret;
+	const char *msg = _("\nCommands:\n"
+"p, pick <commit> = use commit\n"
+"r, reword <commit> = use commit, but edit the commit message\n"
+"e, edit <commit> = use commit, but stop for amending\n"
+"s, squash <commit> = use commit, but meld into previous commit\n"
+"f, fixup <commit> = like \"squash\", but discard this commit's log message\n"
+"x, exec <command> = run command (the rest of the line) using shell\n"
+"d, drop <commit> = remove commit\n"
+"l, label <label> = label current HEAD with a name\n"
+"t, reset <label> = reset HEAD to a label\n"
+"m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]\n"
+".       create a merge commit using the original merge commit's\n"
+".       message (or the oneline, if no original merge commit was\n"
+".       specified). Use -c <commit> to reword the commit message.\n"
+"\n"
+"These lines can be re-ordered; they are executed from top to bottom.\n");
+
+	todo = fopen_or_warn(rebase_path_todo(), "a");
+	if (!todo)
+		return 1;
+
+	strbuf_add_commented_lines(&buf, msg, strlen(msg));
+
+	if (get_missing_commit_check_level() == CHECK_ERROR)
+		msg = _("\nDo not remove any line. Use 'drop' "
+			 "explicitly to remove a commit.\n");
+	else
+		msg = _("\nIf you remove a line here "
+			 "THAT COMMIT WILL BE LOST.\n");
+
+	strbuf_add_commented_lines(&buf, msg, strlen(msg));
+
+	if (edit_todo)
+		msg = _("\nYou are editing the todo file "
+			"of an ongoing interactive rebase.\n"
+			"To continue rebase after editing, run:\n"
+			"    git rebase --continue\n\n");
+	else
+		msg = _("\nHowever, if you remove everything, "
+			"the rebase will be aborted.\n\n");
+
+	strbuf_add_commented_lines(&buf, msg, strlen(msg));
+
+	if (!keep_empty) {
+		msg = _("Note that empty commits are commented out");
+		strbuf_add_commented_lines(&buf, msg, strlen(msg));
+	}
+
+	ret = fputs(buf.buf, todo);
+	fclose(todo);
+	strbuf_release(&buf);
+
+	return ret;
+}
+
 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 c5787c6b5..e14f6590e 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -80,6 +80,7 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 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 skip_unnecessary_picks(void);
 int rearrange_squash(void);
 
-- 
2.16.4
Alban Gruin· Jun 7, 2018, 10:30 UTC · re: Alban Gruin · lore

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

This patch rewrites append_todo_help() from shell to C. The C version covers a bit more than the old shell version. To achieve that, some parameters were added to rebase--helper.

This is part of the effort to rewrite interactive rebase in C.
Changes since v2:
 - Renaming the variable `edit_todo` to `write_edit_todo` to avoid
   confusions, after a comment by Christian Couder[1].
[1] https://github.com/git/git/pull/503#discussion_r193392270
Alban Gruin (1):
  rebase--interactive: rewrite append_todo_help() in C
 builtin/rebase--helper.c   | 10 ++++++--
 git-rebase--interactive.sh | 52 ++--------------------------------------
 sequencer.c                | 60 ++++++++++++++++++++++++++++++++++++++++++++++
 sequencer.h                |  1 +
 4 files changed, 71 insertions(+), 52 deletions(-)
-- 
2.16.4
Alban Gruin· Jun 7, 2018, 10:30 UTC · re: Alban Gruin · lore

[GSoC][PATCH v3 1/1] rebase--interactive: rewrite append_todo_help() in C

This rewrites append_todo_help() from shell to C. It also incorporates some parts of initiate_action() and complete_action() that also write help texts to the todo file.

Two flags are added to rebase--helper.c: one to call append_todo_help() (`--append-todo-help`), and another one to tell append_todo_help() to write the help text suited for the edit-todo mode (`--write-edit-todo`).

Finally, append_todo_help() is removed from git-rebase--interactive.sh to use `rebase--helper --append-todo-help` instead.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase--helper.c   | 10 ++++++--
 git-rebase--interactive.sh | 52 ++--------------------------------------
 sequencer.c                | 60 ++++++++++++++++++++++++++++++++++++++++++++++
 sequencer.h                |  1 +
 4 files changed, 71 insertions(+), 52 deletions(-)
Show changes to 4 files +71 −52

builtin/rebase--helper.c, git-rebase--interactive.sh, sequencer.c, sequencer.h

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f7c2a5fdc..ded5e291d 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;
+	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo = 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
+		ADD_EXEC, APPEND_TODO_HELP
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -27,6 +27,8 @@ 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"),
@@ -45,6 +47,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
 		OPT_CMDMODE(0, "add-exec-commands", &command,
 			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_END()
 	};
 
@@ -84,5 +88,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!rearrange_squash();
 	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);
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 299ded213..94c23a7af 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -39,38 +39,6 @@ comment_for_reflog () {
 	esac
 }
 
-append_todo_help () {
-	gettext "
-Commands:
-p, pick <commit> = use commit
-r, reword <commit> = use commit, but edit the commit message
-e, edit <commit> = use commit, but stop for amending
-s, squash <commit> = use commit, but meld into previous commit
-f, fixup <commit> = like \"squash\", but discard this commit's log message
-x, exec <command> = run command (the rest of the line) using shell
-d, drop <commit> = remove commit
-l, label <label> = label current HEAD with a name
-t, reset <label> = reset HEAD to a label
-m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]
-.       create a merge commit using the original merge commit's
-.       message (or the oneline, if no original merge commit was
-.       specified). Use -c <commit> to reword the commit message.
-
-These lines can be re-ordered; they are executed from top to bottom.
-" | git stripspace --comment-lines >>"$todo"
-
-	if test $(get_missing_commit_check_level) = error
-	then
-		gettext "
-Do not remove any line. Use 'drop' explicitly to remove a commit.
-" | git stripspace --comment-lines >>"$todo"
-	else
-		gettext "
-If you remove a line here THAT COMMIT WILL BE LOST.
-" | git stripspace --comment-lines >>"$todo"
-	fi
-}
-
 die_abort () {
 	apply_autostash
 	rm -rf "$state_dir"
@@ -143,13 +111,7 @@ initiate_action () {
 		git stripspace --strip-comments <"$todo" >"$todo".new
 		mv -f "$todo".new "$todo"
 		collapse_todo_ids
-		append_todo_help
-		gettext "
-You are editing the todo file of an ongoing interactive rebase.
-To continue rebase after editing, run:
-    git rebase --continue
-
-" | git stripspace --comment-lines >>"$todo"
+		git rebase--helper --append-todo-help --write-edit-todo
 
 		git_sequence_editor "$todo" ||
 			die "$(gettext "Could not execute editor")"
@@ -220,17 +182,7 @@ $comment_char $(eval_ngettext \
 	"Rebase \$shortrevisions onto \$shortonto (\$todocount commands)" \
 	"$todocount")
 EOF
-	append_todo_help
-	gettext "
-	However, if you remove everything, the rebase will be aborted.
-
-	" | git stripspace --comment-lines >>"$todo"
-
-	if test -z "$keep_empty"
-	then
-		printf '%s\n' "$comment_char $(gettext "Note that empty commits are commented out")" >>"$todo"
-	fi
-
+	git rebase--helper --append-todo-help ${keep_empty:+--keep-empty}
 
 	has_action "$todo" ||
 		return 2
diff --git a/sequencer.c b/sequencer.c
index cca968043..1ffd990f7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4326,6 +4326,66 @@ int check_todo_list(void)
 	return res;
 }
 
+int append_todo_help(unsigned edit_todo, unsigned keep_empty)
+{
+	struct strbuf buf = STRBUF_INIT;
+	FILE *todo;
+	int ret;
+	const char *msg = _("\nCommands:\n"
+"p, pick <commit> = use commit\n"
+"r, reword <commit> = use commit, but edit the commit message\n"
+"e, edit <commit> = use commit, but stop for amending\n"
+"s, squash <commit> = use commit, but meld into previous commit\n"
+"f, fixup <commit> = like \"squash\", but discard this commit's log message\n"
+"x, exec <command> = run command (the rest of the line) using shell\n"
+"d, drop <commit> = remove commit\n"
+"l, label <label> = label current HEAD with a name\n"
+"t, reset <label> = reset HEAD to a label\n"
+"m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]\n"
+".       create a merge commit using the original merge commit's\n"
+".       message (or the oneline, if no original merge commit was\n"
+".       specified). Use -c <commit> to reword the commit message.\n"
+"\n"
+"These lines can be re-ordered; they are executed from top to bottom.\n");
+
+	todo = fopen_or_warn(rebase_path_todo(), "a");
+	if (!todo)
+		return 1;
+
+	strbuf_add_commented_lines(&buf, msg, strlen(msg));
+
+	if (get_missing_commit_check_level() == CHECK_ERROR)
+		msg = _("\nDo not remove any line. Use 'drop' "
+			 "explicitly to remove a commit.\n");
+	else
+		msg = _("\nIf you remove a line here "
+			 "THAT COMMIT WILL BE LOST.\n");
+
+	strbuf_add_commented_lines(&buf, msg, strlen(msg));
+
+	if (edit_todo)
+		msg = _("\nYou are editing the todo file "
+			"of an ongoing interactive rebase.\n"
+			"To continue rebase after editing, run:\n"
+			"    git rebase --continue\n\n");
+	else
+		msg = _("\nHowever, if you remove everything, "
+			"the rebase will be aborted.\n\n");
+
+	strbuf_add_commented_lines(&buf, msg, strlen(msg));
+
+	if (!keep_empty) {
+		msg = _("Note that empty commits are commented out");
+		strbuf_add_commented_lines(&buf, msg, strlen(msg));
+	}
+
+	ret = fputs(buf.buf, todo);
+	fclose(todo);
+	strbuf_release(&buf);
+
+	return ret;
+}
+
 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 c5787c6b5..e14f6590e 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -80,6 +80,7 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 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 skip_unnecessary_picks(void);
 int rearrange_squash(void);
 
-- 
2.16.4
Alban Gruin· Jun 26, 2018, 16:16 UTC · re: Alban Gruin · lore

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

This patch rewrites append_todo_help() from shell to C. The C version covers a bit more than the old shell version. To achieve that, some parameters were added to rebase--helper.

This also introduce a new source file, rebase-interactive.c.
This is part of the effort to rewrite interactive rebase in C.
This is based on next, as of 2018-06-26.
Changes since v3:
 - Show an error message when append_todo_help() fails to edit the todo
   list.
 - Introducing rebase-interactive.c to contain functions necessary for
   interactive rebase.
Alban Gruin (2):
  sequencer: make two functions and an enum from sequencer.c public
  rebase--interactive: rewrite append_todo_help() in C
 Makefile                   |  1 +
 builtin/rebase--helper.c   | 11 ++++--
 git-rebase--interactive.sh | 52 ++---------------------------
 rebase-interactive.c       | 68 ++++++++++++++++++++++++++++++++++++++
 rebase-interactive.h       |  6 ++++
 sequencer.c                |  8 ++---
 sequencer.h                |  6 ++++
 7 files changed, 94 insertions(+), 58 deletions(-)
 create mode 100644 rebase-interactive.c
 create mode 100644 rebase-interactive.h
-- 
2.18.0
Alban Gruin· Jun 26, 2018, 16:16 UTC · re: Alban Gruin · lore

[GSoC][PATCH v4 1/2] sequencer: make two functions and an enum from sequencer.c public

This makes rebase_path_todo(), get_missign_commit_check_level() and the enum check_level accessible outside sequencer.c. This will be needed for the rewrite of append_todo_help() from shell to C, as it will be in a new library source file, rebase-interactive.c.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 8 ++------
 sequencer.h | 6 ++++++
 2 files changed, 8 insertions(+), 6 deletions(-)
Show changes to 2 files +8 −6

sequencer.c, sequencer.h

diff --git a/sequencer.c b/sequencer.c
index 0a291c91f..881a4f7f4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -52,7 +52,7 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge")
  * the lines are processed, they are removed from the front of this
  * file and written to the tail of 'done'.
  */
-static GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
+GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
 /*
  * The rebase command lines that have already been processed. A line
  * is moved here when it is first handled, before any associated user
@@ -4223,11 +4223,7 @@ int transform_todos(unsigned flags)
 	return i;
 }
 
-enum check_level {
-	CHECK_IGNORE = 0, CHECK_WARN, CHECK_ERROR
-};
-
-static enum check_level get_missing_commit_check_level(void)
+enum check_level get_missing_commit_check_level(void)
 {
 	const char *value;
 
diff --git a/sequencer.h b/sequencer.h
index c5787c6b5..08397b0d1 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -3,6 +3,7 @@
 
 const char *git_path_commit_editmsg(void);
 const char *git_path_seq_dir(void);
+const char *rebase_path_todo(void);
 
 #define APPEND_SIGNOFF_DEDUP (1u << 0)
 
@@ -57,6 +58,10 @@ struct replay_opts {
 };
 #define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }
 
+enum check_level {
+	CHECK_IGNORE = 0, CHECK_WARN, CHECK_ERROR
+};
+
 /* Call this to setup defaults before parsing command line options */
 void sequencer_init_config(struct replay_opts *opts);
 int sequencer_pick_revisions(struct replay_opts *opts);
@@ -79,6 +84,7 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 
 int sequencer_add_exec_commands(const char *command);
 int transform_todos(unsigned flags);
+enum check_level get_missing_commit_check_level(void);
 int check_todo_list(void);
 int skip_unnecessary_picks(void);
 int rearrange_squash(void);
-- 
2.18.0
Johannes Schindelin· Jun 26, 2018, 21:41 UTC · re: Alban Gruin · lore

Re: [GSoC][PATCH v4 1/2] sequencer: make two functions and an enum from sequencer.c public

Hi Alban,
On Tue, 26 Jun 2018, Alban Gruin wrote:
Show 20 quoted lines
> diff --git a/sequencer.h b/sequencer.h
> index c5787c6b5..08397b0d1 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -3,6 +3,7 @@
>  
>  const char *git_path_commit_editmsg(void);
>  const char *git_path_seq_dir(void);
> +const char *rebase_path_todo(void);
>  
>  #define APPEND_SIGNOFF_DEDUP (1u << 0)
>  
> @@ -57,6 +58,10 @@ struct replay_opts {
>  };
>  #define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }
>  
> +enum check_level {
> +	CHECK_IGNORE = 0, CHECK_WARN, CHECK_ERROR
> +};
> +

While this is contained within scopes that include `sequencer.h`, it *is* public now, so I am slightly uneasy about keeping this enum so generic. Maybe we want to use

enum missing_commit_check_level {
	MISSING_COMMIT_CHECK_IGNORE = 0,
	MISSING_COMMIT_CHECK_WARN,
	MISSING_COMMIT_CHECK_ERROR
};
instead?

Ciao, Dscho

Alban Gruin· Jun 27, 2018, 14:54 UTC · re: Johannes Schindelin · lore

Re: [GSoC][PATCH v4 1/2] sequencer: make two functions and an enum from sequencer.c public

Hi Johannes,
Le 26/06/2018 à 23:41, Johannes Schindelin a écrit :
Show 37 quoted lines
> Hi Alban,
> 
> On Tue, 26 Jun 2018, Alban Gruin wrote:
> 
>> diff --git a/sequencer.h b/sequencer.h
>> index c5787c6b5..08397b0d1 100644
>> --- a/sequencer.h
>> +++ b/sequencer.h
>> @@ -3,6 +3,7 @@
>>  
>>  const char *git_path_commit_editmsg(void);
>>  const char *git_path_seq_dir(void);
>> +const char *rebase_path_todo(void);
>>  
>>  #define APPEND_SIGNOFF_DEDUP (1u << 0)
>>  
>> @@ -57,6 +58,10 @@ struct replay_opts {
>>  };
>>  #define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }
>>  
>> +enum check_level {
>> +	CHECK_IGNORE = 0, CHECK_WARN, CHECK_ERROR
>> +};
>> +
> 
> While this is contained within scopes that include `sequencer.h`, it *is*
> public now, so I am slightly uneasy about keeping this enum so generic.
> Maybe we want to use
> 
> enum missing_commit_check_level {
> 	MISSING_COMMIT_CHECK_IGNORE = 0,
> 	MISSING_COMMIT_CHECK_WARN,
> 	MISSING_COMMIT_CHECK_ERROR
> };
> 
> instead?
> 
You’re right, this would be better.

Cheers, Alban

Alban Gruin· Jun 26, 2018, 16:16 UTC · re: Alban Gruin · lore

[GSoC][PATCH v4 2/2] rebase--interactive: rewrite append_todo_help() in C

This rewrites append_todo_help() from shell to C. It also incorporates some parts of initiate_action() and complete_action() that also write help texts to the todo file.

This also introduces the source file rebase-interactive.c. This file will contain functions necessary for interactive rebase that are too specific for the sequencer, and is part of libgit.a.

Two flags are added to rebase--helper.c: one to call append_todo_help() (`--append-todo-help`), and another one to tell append_todo_help() to write the help text suited for the edit-todo mode (`--write-edit-todo`).

Finally, append_todo_help() is removed from git-rebase--interactive.sh to use `rebase--helper --append-todo-help` instead.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 Makefile                   |  1 +
 builtin/rebase--helper.c   | 11 ++++--
 git-rebase--interactive.sh | 52 ++---------------------------
 rebase-interactive.c       | 68 ++++++++++++++++++++++++++++++++++++++
 rebase-interactive.h       |  6 ++++
 5 files changed, 86 insertions(+), 52 deletions(-)
 create mode 100644 rebase-interactive.c
 create mode 100644 rebase-interactive.h
Show changes to 5 files +86 −52

Makefile, builtin/rebase--helper.c, git-rebase--interactive.sh, rebase-interactive.c, rebase-interactive.h

diff --git a/Makefile b/Makefile
index 0cb6590f2..a281139ef 100644
--- a/Makefile
+++ b/Makefile
@@ -922,6 +922,7 @@ LIB_OBJS += protocol.o
 LIB_OBJS += quote.o
 LIB_OBJS += reachable.o
 LIB_OBJS += read-cache.o
+LIB_OBJS += rebase-interactive.o
 LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
 LIB_OBJS += refs/files-backend.o
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f7c2a5fdc..05e73e71d 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -3,6 +3,7 @@
 #include "config.h"
 #include "parse-options.h"
 #include "sequencer.h"
+#include "rebase-interactive.h"
 
 static const char * const builtin_rebase_helper_usage[] = {
 	N_("git rebase--helper [<options>]"),
@@ -12,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;
+	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo = 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
+		ADD_EXEC, APPEND_TODO_HELP
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -27,6 +28,8 @@ 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"),
@@ -45,6 +48,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
 		OPT_CMDMODE(0, "add-exec-commands", &command,
 			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_END()
 	};
 
@@ -84,5 +89,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!rearrange_squash();
 	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);
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 299ded213..94c23a7af 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -39,38 +39,6 @@ comment_for_reflog () {
 	esac
 }
 
-append_todo_help () {
-	gettext "
-Commands:
-p, pick <commit> = use commit
-r, reword <commit> = use commit, but edit the commit message
-e, edit <commit> = use commit, but stop for amending
-s, squash <commit> = use commit, but meld into previous commit
-f, fixup <commit> = like \"squash\", but discard this commit's log message
-x, exec <command> = run command (the rest of the line) using shell
-d, drop <commit> = remove commit
-l, label <label> = label current HEAD with a name
-t, reset <label> = reset HEAD to a label
-m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]
-.       create a merge commit using the original merge commit's
-.       message (or the oneline, if no original merge commit was
-.       specified). Use -c <commit> to reword the commit message.
-
-These lines can be re-ordered; they are executed from top to bottom.
-" | git stripspace --comment-lines >>"$todo"
-
-	if test $(get_missing_commit_check_level) = error
-	then
-		gettext "
-Do not remove any line. Use 'drop' explicitly to remove a commit.
-" | git stripspace --comment-lines >>"$todo"
-	else
-		gettext "
-If you remove a line here THAT COMMIT WILL BE LOST.
-" | git stripspace --comment-lines >>"$todo"
-	fi
-}
-
 die_abort () {
 	apply_autostash
 	rm -rf "$state_dir"
@@ -143,13 +111,7 @@ initiate_action () {
 		git stripspace --strip-comments <"$todo" >"$todo".new
 		mv -f "$todo".new "$todo"
 		collapse_todo_ids
-		append_todo_help
-		gettext "
-You are editing the todo file of an ongoing interactive rebase.
-To continue rebase after editing, run:
-    git rebase --continue
-
-" | git stripspace --comment-lines >>"$todo"
+		git rebase--helper --append-todo-help --write-edit-todo
 
 		git_sequence_editor "$todo" ||
 			die "$(gettext "Could not execute editor")"
@@ -220,17 +182,7 @@ $comment_char $(eval_ngettext \
 	"Rebase \$shortrevisions onto \$shortonto (\$todocount commands)" \
 	"$todocount")
 EOF
-	append_todo_help
-	gettext "
-	However, if you remove everything, the rebase will be aborted.
-
-	" | git stripspace --comment-lines >>"$todo"
-
-	if test -z "$keep_empty"
-	then
-		printf '%s\n' "$comment_char $(gettext "Note that empty commits are commented out")" >>"$todo"
-	fi
-
+	git rebase--helper --append-todo-help ${keep_empty:+--keep-empty}
 
 	has_action "$todo" ||
 		return 2
diff --git a/rebase-interactive.c b/rebase-interactive.c
new file mode 100644
index 000000000..c79c819b9
--- /dev/null
+++ b/rebase-interactive.c
@@ -0,0 +1,68 @@
+#include "cache.h"
+#include "commit.h"
+#include "rebase-interactive.h"
+#include "sequencer.h"
+#include "strbuf.h"
+
+int append_todo_help(unsigned edit_todo, unsigned keep_empty)
+{
+	struct strbuf buf = STRBUF_INIT;
+	FILE *todo;
+	int ret;
+	const char *msg = _("\nCommands:\n"
+"p, pick <commit> = use commit\n"
+"r, reword <commit> = use commit, but edit the commit message\n"
+"e, edit <commit> = use commit, but stop for amending\n"
+"s, squash <commit> = use commit, but meld into previous commit\n"
+"f, fixup <commit> = like \"squash\", but discard this commit's log message\n"
+"x, exec <command> = run command (the rest of the line) using shell\n"
+"d, drop <commit> = remove commit\n"
+"l, label <label> = label current HEAD with a name\n"
+"t, reset <label> = reset HEAD to a label\n"
+"m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]\n"
+".       create a merge commit using the original merge commit's\n"
+".       message (or the oneline, if no original merge commit was\n"
+".       specified). Use -c <commit> to reword the commit message.\n"
+"\n"
+"These lines can be re-ordered; they are executed from top to bottom.\n");
+
+	todo = fopen_or_warn(rebase_path_todo(), "a");
+	if (!todo)
+		return 1;
+
+	strbuf_add_commented_lines(&buf, msg, strlen(msg));
+
+	if (get_missing_commit_check_level() == CHECK_ERROR)
+		msg = _("\nDo not remove any line. Use 'drop' "
+			 "explicitly to remove a commit.\n");
+	else
+		msg = _("\nIf you remove a line here "
+			 "THAT COMMIT WILL BE LOST.\n");
+
+	strbuf_add_commented_lines(&buf, msg, strlen(msg));
+
+	if (edit_todo)
+		msg = _("\nYou are editing the todo file "
+			"of an ongoing interactive rebase.\n"
+			"To continue rebase after editing, run:\n"
+			"    git rebase --continue\n\n");
+	else
+		msg = _("\nHowever, if you remove everything, "
+			"the rebase will be aborted.\n\n");
+
+	strbuf_add_commented_lines(&buf, msg, strlen(msg));
+
+	if (!keep_empty) {
+		msg = _("Note that empty commits are commented out");
+		strbuf_add_commented_lines(&buf, msg, strlen(msg));
+	}
+
+	ret = fputs(buf.buf, todo);
+	if (ret < 0)
+		error_errno(_("Could not append help text to '%s'"), rebase_path_todo());
+
+	fclose(todo);
+	strbuf_release(&buf);
+
+	return ret;
+}
diff --git a/rebase-interactive.h b/rebase-interactive.h
new file mode 100644
index 000000000..47372624e
--- /dev/null
+++ b/rebase-interactive.h
@@ -0,0 +1,6 @@
+#ifndef REBASE_INTERACTIVE_H
+#define REBASE_INTERACTIVE_H
+
+int append_todo_help(unsigned edit_todo, unsigned keep_empty);
+
+#endif
-- 
2.18.0
Johannes Schindelin· Jun 26, 2018, 21:43 UTC · re: Alban Gruin · lore

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

Hi Alban,
On Tue, 26 Jun 2018, Alban Gruin wrote:
Show 15 quoted lines
> This rewrites append_todo_help() from shell to C. It also incorporates
> some parts of initiate_action() and complete_action() that also write
> help texts to the todo file.
> 
> This also introduces the source file rebase-interactive.c. This file
> will contain functions necessary for interactive rebase that are too
> specific for the sequencer, and is part of libgit.a.
> 
> Two flags are added to rebase--helper.c: one to call
> append_todo_help() (`--append-todo-help`), and another one to tell
> append_todo_help() to write the help text suited for the edit-todo
> mode (`--write-edit-todo`).
> 
> Finally, append_todo_help() is removed from git-rebase--interactive.sh
> to use `rebase--helper --append-todo-help` instead.
Looks good!

Thanks, Dscho

Johannes Schindelin· Jun 26, 2018, 21:37 UTC · re: Alban Gruin · lore

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

Hi Alban,
On Tue, 26 Jun 2018, Alban Gruin wrote:
Show 9 quoted lines
> This patch rewrites append_todo_help() from shell to C. The C version
> covers a bit more than the old shell version. To achieve that, some
> parameters were added to rebase--helper.
> 
> This also introduce a new source file, rebase-interactive.c.
> 
> This is part of the effort to rewrite interactive rebase in C.
> 
> This is based on next, as of 2018-06-26.
Out of curiosity: which commits that are not yet in `master` are required?
Show 7 quoted lines
> Changes since v3:
> 
>  - Show an error message when append_todo_help() fails to edit the todo
>    list.
> 
>  - Introducing rebase-interactive.c to contain functions necessary for
>    interactive rebase.

Thank you very much! Dscho

Alban Gruin· Jun 27, 2018, 14:54 UTC · re: Johannes Schindelin · lore

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

Hi Johannes,
Le 26/06/2018 à 23:37, Johannes Schindelin a écrit :
Show 16 quoted lines
> Hi Alban,
> 
> On Tue, 26 Jun 2018, Alban Gruin wrote:
> 
>> This patch rewrites append_todo_help() from shell to C. The C version
>> covers a bit more than the old shell version. To achieve that, some
>> parameters were added to rebase--helper.
>>
>> This also introduce a new source file, rebase-interactive.c.
>>
>> This is part of the effort to rewrite interactive rebase in C.
>>
>> This is based on next, as of 2018-06-26.
> 
> Out of curiosity: which commits that are not yet in `master` are required?
> 
None, actually.

Cheers, Alban

Alban Gruin· Jun 28, 2018, 10:04 UTC · re: Alban Gruin · lore

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

This patch rewrites append_todo_help() from shell to C. The C version covers a bit more than the old shell version. To achieve that, some parameters were added to rebase--helper.

This also introduce a new source file, rebase-interactive.c.
This is part of the effort to rewrite interactive rebase in C.
This is based on next, as of 2018-06-28.
Changes since v4:
 - Renaming enumeration check_level and its values to avoid namespace
   pollution.
Alban Gruin (2):
  sequencer: make two functions and an enum from sequencer.c public
  rebase--interactive: rewrite append_todo_help() in C
 Makefile                   |  1 +
 builtin/rebase--helper.c   | 11 ++++--
 git-rebase--interactive.sh | 52 ++---------------------------
 rebase-interactive.c       | 68 ++++++++++++++++++++++++++++++++++++++
 rebase-interactive.h       |  6 ++++
 sequencer.c                | 22 +++++-------
 sequencer.h                |  8 +++++
 7 files changed, 103 insertions(+), 65 deletions(-)
 create mode 100644 rebase-interactive.c
 create mode 100644 rebase-interactive.h
-- 
2.18.0
Alban Gruin· Jun 28, 2018, 10:04 UTC · re: Alban Gruin · lore

[GSoC][PATCH v5 1/2] sequencer: make two functions and an enum from sequencer.c public

This makes rebase_path_todo(), get_missing_commit_check_level() and the enum check_level accessible outside sequencer.c. check_level is renamed missing_commit_check_level, and its value names are prefixed by MISSING_COMMIT_ to avoid namespace pollution.

This will be needed for the rewrite of append_todo_help() from shell to C, as it will be in a new library source file, rebase-interactive.c.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 22 +++++++++-------------
 sequencer.h |  8 ++++++++
 2 files changed, 17 insertions(+), 13 deletions(-)
Show changes to 2 files +17 −13

sequencer.c, sequencer.h

diff --git a/sequencer.c b/sequencer.c
index 0a291c91f..cb7ec9807 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -52,7 +52,7 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge")
  * the lines are processed, they are removed from the front of this
  * file and written to the tail of 'done'.
  */
-static GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
+GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
 /*
  * The rebase command lines that have already been processed. A line
  * is moved here when it is first handled, before any associated user
@@ -4223,24 +4223,20 @@ int transform_todos(unsigned flags)
 	return i;
 }
 
-enum check_level {
-	CHECK_IGNORE = 0, CHECK_WARN, CHECK_ERROR
-};
-
-static enum check_level get_missing_commit_check_level(void)
+enum missing_commit_check_level get_missing_commit_check_level(void)
 {
 	const char *value;
 
 	if (git_config_get_value("rebase.missingcommitscheck", &value) ||
 			!strcasecmp("ignore", value))
-		return CHECK_IGNORE;
+		return MISSING_COMMIT_CHECK_IGNORE;
 	if (!strcasecmp("warn", value))
-		return CHECK_WARN;
+		return MISSING_COMMIT_CHECK_WARN;
 	if (!strcasecmp("error", value))
-		return CHECK_ERROR;
+		return MISSING_COMMIT_CHECK_ERROR;
 	warning(_("unrecognized setting %s for option "
 		  "rebase.missingCommitsCheck. Ignoring."), value);
-	return CHECK_IGNORE;
+	return MISSING_COMMIT_CHECK_IGNORE;
 }
 
 define_commit_slab(commit_seen, unsigned char);
@@ -4252,7 +4248,7 @@ define_commit_slab(commit_seen, unsigned char);
  */
 int check_todo_list(void)
 {
-	enum check_level check_level = get_missing_commit_check_level();
+	enum missing_commit_check_level check_level = get_missing_commit_check_level();
 	struct strbuf todo_file = STRBUF_INIT;
 	struct todo_list todo_list = TODO_LIST_INIT;
 	struct strbuf missing = STRBUF_INIT;
@@ -4269,7 +4265,7 @@ int check_todo_list(void)
 	advise_to_edit_todo = res =
 		parse_insn_buffer(todo_list.buf.buf, &todo_list);
 
-	if (res || check_level == CHECK_IGNORE)
+	if (res || check_level == MISSING_COMMIT_CHECK_IGNORE)
 		goto leave_check;
 
 	/* Mark the commits in git-rebase-todo as seen */
@@ -4304,7 +4300,7 @@ int check_todo_list(void)
 	if (!missing.len)
 		goto leave_check;
 
-	if (check_level == CHECK_ERROR)
+	if (check_level == MISSING_COMMIT_CHECK_ERROR)
 		advise_to_edit_todo = res = 1;
 
 	fprintf(stderr,
diff --git a/sequencer.h b/sequencer.h
index c5787c6b5..ffab798f1 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -3,6 +3,7 @@
 
 const char *git_path_commit_editmsg(void);
 const char *git_path_seq_dir(void);
+const char *rebase_path_todo(void);
 
 #define APPEND_SIGNOFF_DEDUP (1u << 0)
 
@@ -57,6 +58,12 @@ struct replay_opts {
 };
 #define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }
 
+enum missing_commit_check_level {
+	MISSING_COMMIT_CHECK_IGNORE = 0,
+	MISSING_COMMIT_CHECK_WARN,
+	MISSING_COMMIT_CHECK_ERROR
+};
+
 /* Call this to setup defaults before parsing command line options */
 void sequencer_init_config(struct replay_opts *opts);
 int sequencer_pick_revisions(struct replay_opts *opts);
@@ -79,6 +86,7 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 
 int sequencer_add_exec_commands(const char *command);
 int transform_todos(unsigned flags);
+enum missing_commit_check_level get_missing_commit_check_level(void);
 int check_todo_list(void);
 int skip_unnecessary_picks(void);
 int rearrange_squash(void);
-- 
2.18.0
Alban Gruin· Jun 28, 2018, 10:04 UTC · re: Alban Gruin · lore

[GSoC][PATCH v5 2/2] rebase--interactive: rewrite append_todo_help() in C

This rewrites append_todo_help() from shell to C. It also incorporates some parts of initiate_action() and complete_action() that also write help texts to the todo file.

This also introduces the source file rebase-interactive.c. This file will contain functions necessary for interactive rebase that are too specific for the sequencer, and is part of libgit.a.

Two flags are added to rebase--helper.c: one to call append_todo_help() (`--append-todo-help`), and another one to tell append_todo_help() to write the help text suited for the edit-todo mode (`--write-edit-todo`).

Finally, append_todo_help() is removed from git-rebase--interactive.sh to use `rebase--helper --append-todo-help` instead.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 Makefile                   |  1 +
 builtin/rebase--helper.c   | 11 ++++--
 git-rebase--interactive.sh | 52 ++---------------------------
 rebase-interactive.c       | 68 ++++++++++++++++++++++++++++++++++++++
 rebase-interactive.h       |  6 ++++
 5 files changed, 86 insertions(+), 52 deletions(-)
 create mode 100644 rebase-interactive.c
 create mode 100644 rebase-interactive.h
Show changes to 5 files +86 −52

Makefile, builtin/rebase--helper.c, git-rebase--interactive.sh, rebase-interactive.c, rebase-interactive.h

diff --git a/Makefile b/Makefile
index 0cb6590f2..a281139ef 100644
--- a/Makefile
+++ b/Makefile
@@ -922,6 +922,7 @@ LIB_OBJS += protocol.o
 LIB_OBJS += quote.o
 LIB_OBJS += reachable.o
 LIB_OBJS += read-cache.o
+LIB_OBJS += rebase-interactive.o
 LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
 LIB_OBJS += refs/files-backend.o
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f7c2a5fdc..05e73e71d 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -3,6 +3,7 @@
 #include "config.h"
 #include "parse-options.h"
 #include "sequencer.h"
+#include "rebase-interactive.h"
 
 static const char * const builtin_rebase_helper_usage[] = {
 	N_("git rebase--helper [<options>]"),
@@ -12,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;
+	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo = 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
+		ADD_EXEC, APPEND_TODO_HELP
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -27,6 +28,8 @@ 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"),
@@ -45,6 +48,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
 		OPT_CMDMODE(0, "add-exec-commands", &command,
 			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_END()
 	};
 
@@ -84,5 +89,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!rearrange_squash();
 	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);
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 299ded213..94c23a7af 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -39,38 +39,6 @@ comment_for_reflog () {
 	esac
 }
 
-append_todo_help () {
-	gettext "
-Commands:
-p, pick <commit> = use commit
-r, reword <commit> = use commit, but edit the commit message
-e, edit <commit> = use commit, but stop for amending
-s, squash <commit> = use commit, but meld into previous commit
-f, fixup <commit> = like \"squash\", but discard this commit's log message
-x, exec <command> = run command (the rest of the line) using shell
-d, drop <commit> = remove commit
-l, label <label> = label current HEAD with a name
-t, reset <label> = reset HEAD to a label
-m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]
-.       create a merge commit using the original merge commit's
-.       message (or the oneline, if no original merge commit was
-.       specified). Use -c <commit> to reword the commit message.
-
-These lines can be re-ordered; they are executed from top to bottom.
-" | git stripspace --comment-lines >>"$todo"
-
-	if test $(get_missing_commit_check_level) = error
-	then
-		gettext "
-Do not remove any line. Use 'drop' explicitly to remove a commit.
-" | git stripspace --comment-lines >>"$todo"
-	else
-		gettext "
-If you remove a line here THAT COMMIT WILL BE LOST.
-" | git stripspace --comment-lines >>"$todo"
-	fi
-}
-
 die_abort () {
 	apply_autostash
 	rm -rf "$state_dir"
@@ -143,13 +111,7 @@ initiate_action () {
 		git stripspace --strip-comments <"$todo" >"$todo".new
 		mv -f "$todo".new "$todo"
 		collapse_todo_ids
-		append_todo_help
-		gettext "
-You are editing the todo file of an ongoing interactive rebase.
-To continue rebase after editing, run:
-    git rebase --continue
-
-" | git stripspace --comment-lines >>"$todo"
+		git rebase--helper --append-todo-help --write-edit-todo
 
 		git_sequence_editor "$todo" ||
 			die "$(gettext "Could not execute editor")"
@@ -220,17 +182,7 @@ $comment_char $(eval_ngettext \
 	"Rebase \$shortrevisions onto \$shortonto (\$todocount commands)" \
 	"$todocount")
 EOF
-	append_todo_help
-	gettext "
-	However, if you remove everything, the rebase will be aborted.
-
-	" | git stripspace --comment-lines >>"$todo"
-
-	if test -z "$keep_empty"
-	then
-		printf '%s\n' "$comment_char $(gettext "Note that empty commits are commented out")" >>"$todo"
-	fi
-
+	git rebase--helper --append-todo-help ${keep_empty:+--keep-empty}
 
 	has_action "$todo" ||
 		return 2
diff --git a/rebase-interactive.c b/rebase-interactive.c
new file mode 100644
index 000000000..015e08cd0
--- /dev/null
+++ b/rebase-interactive.c
@@ -0,0 +1,68 @@
+#include "cache.h"
+#include "commit.h"
+#include "rebase-interactive.h"
+#include "sequencer.h"
+#include "strbuf.h"
+
+int append_todo_help(unsigned edit_todo, unsigned keep_empty)
+{
+	struct strbuf buf = STRBUF_INIT;
+	FILE *todo;
+	int ret;
+	const char *msg = _("\nCommands:\n"
+"p, pick <commit> = use commit\n"
+"r, reword <commit> = use commit, but edit the commit message\n"
+"e, edit <commit> = use commit, but stop for amending\n"
+"s, squash <commit> = use commit, but meld into previous commit\n"
+"f, fixup <commit> = like \"squash\", but discard this commit's log message\n"
+"x, exec <command> = run command (the rest of the line) using shell\n"
+"d, drop <commit> = remove commit\n"
+"l, label <label> = label current HEAD with a name\n"
+"t, reset <label> = reset HEAD to a label\n"
+"m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]\n"
+".       create a merge commit using the original merge commit's\n"
+".       message (or the oneline, if no original merge commit was\n"
+".       specified). Use -c <commit> to reword the commit message.\n"
+"\n"
+"These lines can be re-ordered; they are executed from top to bottom.\n");
+
+	todo = fopen_or_warn(rebase_path_todo(), "a");
+	if (!todo)
+		return 1;
+
+	strbuf_add_commented_lines(&buf, msg, strlen(msg));
+
+	if (get_missing_commit_check_level() == MISSING_COMMIT_CHECK_ERROR)
+		msg = _("\nDo not remove any line. Use 'drop' "
+			 "explicitly to remove a commit.\n");
+	else
+		msg = _("\nIf you remove a line here "
+			 "THAT COMMIT WILL BE LOST.\n");
+
+	strbuf_add_commented_lines(&buf, msg, strlen(msg));
+
+	if (edit_todo)
+		msg = _("\nYou are editing the todo file "
+			"of an ongoing interactive rebase.\n"
+			"To continue rebase after editing, run:\n"
+			"    git rebase --continue\n\n");
+	else
+		msg = _("\nHowever, if you remove everything, "
+			"the rebase will be aborted.\n\n");
+
+	strbuf_add_commented_lines(&buf, msg, strlen(msg));
+
+	if (!keep_empty) {
+		msg = _("Note that empty commits are commented out");
+		strbuf_add_commented_lines(&buf, msg, strlen(msg));
+	}
+
+	ret = fputs(buf.buf, todo);
+	if (ret < 0)
+		error_errno(_("Could not append help text to '%s'"), rebase_path_todo());
+
+	fclose(todo);
+	strbuf_release(&buf);
+
+	return ret;
+}
diff --git a/rebase-interactive.h b/rebase-interactive.h
new file mode 100644
index 000000000..47372624e
--- /dev/null
+++ b/rebase-interactive.h
@@ -0,0 +1,6 @@
+#ifndef REBASE_INTERACTIVE_H
+#define REBASE_INTERACTIVE_H
+
+int append_todo_help(unsigned edit_todo, unsigned keep_empty);
+
+#endif
-- 
2.18.0
Junio C Hamano· Jun 28, 2018, 18:05 UTC · re: Alban Gruin · lore

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

Alban Gruin <alban.gruin@gmail.com> writes:
Show 9 quoted lines
> This patch rewrites append_todo_help() from shell to C. The C version
> covers a bit more than the old shell version. To achieve that, some
> parameters were added to rebase--helper.
>
> This also introduce a new source file, rebase-interactive.c.
>
> This is part of the effort to rewrite interactive rebase in C.
>
> This is based on next, as of 2018-06-28.

Please do not base new development on 'next'. You typically do not require the whole of 'next'; there is *NO* reason why this topic must wait until say ds/ewah-cleanup topic graduates to 'master', for example.

You may still depend on just a handful of selected topics that you build on that are not yet in 'master'. Identify them and build on top of the merge of them on top of whatever integration branch you are aiming for (typically 'master').

And list these topics explicitly, instead of saying 'based on next'.

← back to recent threads