git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [GSoC][PATCH v2 0/7] rebase -i: rewrite some parts in C
@ 2018-07-02 10:57 Alban Gruin
  2018-07-02 10:57 ` [GSoC][PATCH v2 1/7] sequencer: make two functions and an enum from sequencer.c public Alban Gruin
                   ` (7 more replies)
  0 siblings, 8 replies; 50+ messages in thread
From: Alban Gruin @ 2018-07-02 10:57 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, Alban Gruin

This patch series rewrites some parts of interactive rebase from shell
to C:

 - append_todo_help(). The C version covers a bit more than the shell
   version.

 - The edit-todo functionnality.

 - The reflog operations.

The v1 of this series is an aggregate made by Junio of my patch series
about append_todo_help() (v3), edit-todo (v2) and reflog (v5), and can
be found in the branch `ag/rebase-i-in-c`.

This branch is based on master (as of 2018-07-02).

Changes since v1:

 - Introducing rebase-interactive.c to contain functions necessary for
   interactive rebase.

 - Show an error message when append_todo_help() fails to edit the todo
   list.

 - Renaming enumeration check_level and its values to avoid namespace
   pollution.

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

Alban Gruin (7):
  sequencer: make two functions and an enum from sequencer.c public
  rebase--interactive: rewrite append_todo_help() in C
  editor: add a function to launch the sequence editor
  rebase-interactive: rewrite the edit-todo functionality in C
  sequencer: add a new function to silence a command, except if it
    fails.
  rebase -i: rewrite setup_reflog_action() in C
  rebase -i: rewrite checkout_onto() in C

 Makefile                   |   1 +
 builtin/rebase--helper.c   |  24 +++++++-
 cache.h                    |   1 +
 editor.c                   |  27 ++++++++-
 git-rebase--interactive.sh | 100 +++----------------------------
 rebase-interactive.c       |  99 ++++++++++++++++++++++++++++++
 rebase-interactive.h       |   7 +++
 sequencer.c                | 120 +++++++++++++++++++++++++------------
 sequencer.h                |  14 +++++
 strbuf.h                   |   2 +
 10 files changed, 260 insertions(+), 135 deletions(-)
 create mode 100644 rebase-interactive.c
 create mode 100644 rebase-interactive.h

-- 
2.18.0


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

* [GSoC][PATCH v2 1/7] sequencer: make two functions and an enum from sequencer.c public
  2018-07-02 10:57 [GSoC][PATCH v2 0/7] rebase -i: rewrite some parts in C Alban Gruin
@ 2018-07-02 10:57 ` Alban Gruin
  2018-07-03 20:20   ` Junio C Hamano
  2018-07-02 10:57 ` [GSoC][PATCH v2 2/7] rebase--interactive: rewrite append_todo_help() in C Alban Gruin
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2018-07-02 10:57 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, Alban Gruin

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

diff --git a/sequencer.c b/sequencer.c
index 5354d4d51..57fd58bc1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -51,7 +51,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
@@ -4221,24 +4221,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);
@@ -4250,7 +4246,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;
@@ -4267,7 +4263,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 */
@@ -4302,7 +4298,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


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

* [GSoC][PATCH v2 2/7] rebase--interactive: rewrite append_todo_help() in C
  2018-07-02 10:57 [GSoC][PATCH v2 0/7] rebase -i: rewrite some parts in C Alban Gruin
  2018-07-02 10:57 ` [GSoC][PATCH v2 1/7] sequencer: make two functions and an enum from sequencer.c public Alban Gruin
@ 2018-07-02 10:57 ` Alban Gruin
  2018-07-03 20:29   ` Junio C Hamano
  2018-07-02 10:57 ` [GSoC][PATCH v2 3/7] editor: add a function to launch the sequence editor Alban Gruin
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2018-07-02 10:57 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, Alban Gruin

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

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


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

* [GSoC][PATCH v2 3/7] editor: add a function to launch the sequence editor
  2018-07-02 10:57 [GSoC][PATCH v2 0/7] rebase -i: rewrite some parts in C Alban Gruin
  2018-07-02 10:57 ` [GSoC][PATCH v2 1/7] sequencer: make two functions and an enum from sequencer.c public Alban Gruin
  2018-07-02 10:57 ` [GSoC][PATCH v2 2/7] rebase--interactive: rewrite append_todo_help() in C Alban Gruin
@ 2018-07-02 10:57 ` Alban Gruin
  2018-07-03 20:32   ` Junio C Hamano
  2018-07-02 10:57 ` [GSoC][PATCH v2 4/7] rebase-interactive: rewrite the edit-todo functionality in C Alban Gruin
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2018-07-02 10:57 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, 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 d49092d94..33fa70f55 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.18.0


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

* [GSoC][PATCH v2 4/7] rebase-interactive: rewrite the edit-todo functionality in C
  2018-07-02 10:57 [GSoC][PATCH v2 0/7] rebase -i: rewrite some parts in C Alban Gruin
                   ` (2 preceding siblings ...)
  2018-07-02 10:57 ` [GSoC][PATCH v2 3/7] editor: add a function to launch the sequence editor Alban Gruin
@ 2018-07-02 10:57 ` Alban Gruin
  2018-07-02 10:57 ` [GSoC][PATCH v2 5/7] sequencer: add a new function to silence a command, except if it fails Alban Gruin
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2018-07-02 10:57 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, 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 015e08cd0..fb7ad401a 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] 50+ messages in thread

* [GSoC][PATCH v2 5/7] sequencer: add a new function to silence a command, except if it fails.
  2018-07-02 10:57 [GSoC][PATCH v2 0/7] rebase -i: rewrite some parts in C Alban Gruin
                   ` (3 preceding siblings ...)
  2018-07-02 10:57 ` [GSoC][PATCH v2 4/7] rebase-interactive: rewrite the edit-todo functionality in C Alban Gruin
@ 2018-07-02 10:57 ` Alban Gruin
  2018-07-03 20:36   ` Junio C Hamano
  2018-07-02 10:57 ` [GSoC][PATCH v2 6/7] rebase -i: rewrite setup_reflog_action() in C Alban Gruin
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2018-07-02 10:57 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, Alban Gruin

This adds a new function, run_command_silent_on_success(), to
redirect the stdout and stderr of a command to a strbuf, and then to run
that command. This strbuf is printed only if the command fails. It is
functionnaly similar to output() from git-rebase.sh.

run_git_commit() is then refactored to use of
run_command_silent_on_success().

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 49 ++++++++++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 57fd58bc1..9e2b34a49 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -768,6 +768,24 @@ N_("you have staged changes in your working tree\n"
 #define VERIFY_MSG  (1<<4)
 #define CREATE_ROOT_COMMIT (1<<5)
 
+static int run_command_silent_on_success(struct child_process *cmd)
+{
+	struct strbuf buf = STRBUF_INIT;
+	int rc;
+
+	cmd->stdout_to_stderr = 1;
+	rc = pipe_command(cmd,
+			  NULL, 0,
+			  /* stdout is already redirected */
+			  NULL, 0,
+			  &buf, 0);
+
+	if (rc)
+		fputs(buf.buf, stderr);
+	strbuf_release(&buf);
+	return rc;
+}
+
 /*
  * If we are cherry-pick, and if the merge did not result in
  * hand-editing, we will hit this commit and inherit the original
@@ -822,18 +840,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 
 	cmd.git_cmd = 1;
 
-	if (is_rebase_i(opts)) {
-		if (!(flags & EDIT_MSG)) {
-			cmd.stdout_to_stderr = 1;
-			cmd.err = -1;
-		}
+	if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {
+		const char *gpg_opt = gpg_sign_opt_quoted(opts);
 
-		if (read_env_script(&cmd.env_array)) {
-			const char *gpg_opt = gpg_sign_opt_quoted(opts);
-
-			return error(_(staged_changes_advice),
-				     gpg_opt, gpg_opt);
-		}
+		return error(_(staged_changes_advice),
+			     gpg_opt, gpg_opt);
 	}
 
 	argv_array_push(&cmd.args, "commit");
@@ -863,20 +874,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	if (opts->allow_empty_message)
 		argv_array_push(&cmd.args, "--allow-empty-message");
 
-	if (cmd.err == -1) {
-		/* hide stderr on success */
-		struct strbuf buf = STRBUF_INIT;
-		int rc = pipe_command(&cmd,
-				      NULL, 0,
-				      /* stdout is already redirected */
-				      NULL, 0,
-				      &buf, 0);
-		if (rc)
-			fputs(buf.buf, stderr);
-		strbuf_release(&buf);
-		return rc;
-	}
-
+	if (is_rebase_i(opts) && !(flags & EDIT_MSG))
+		return run_command_silent_on_success(&cmd);
 	return run_command(&cmd);
 }
 
-- 
2.18.0


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

* [GSoC][PATCH v2 6/7] rebase -i: rewrite setup_reflog_action() in C
  2018-07-02 10:57 [GSoC][PATCH v2 0/7] rebase -i: rewrite some parts in C Alban Gruin
                   ` (4 preceding siblings ...)
  2018-07-02 10:57 ` [GSoC][PATCH v2 5/7] sequencer: add a new function to silence a command, except if it fails Alban Gruin
@ 2018-07-02 10:57 ` Alban Gruin
  2018-07-03 20:37   ` Junio C Hamano
  2018-07-02 10:57 ` [GSoC][PATCH v2 7/7] rebase -i: rewrite checkout_onto() " Alban Gruin
  2018-07-10 12:15 ` [GSoC][PATCH v3 00/13] rebase -i: rewrite some parts " Alban Gruin
  7 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2018-07-02 10:57 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, Alban Gruin

This rewrites (the misnamed) setup_reflog_action() from shell to C. The
new version is called prepare_branch_to_be_rebased().

A new command is added to rebase--helper.c, “checkout-base”, as well as
a new flag, “verbose”, to avoid silencing the output of the checkout
operation called by checkout_base_commit().

The function `run_git_checkout()` will also be used in the next commit,
therefore its code is not part of `checkout_base_commit()`.

The shell version is then stripped in favour of a call to the helper.

As $GIT_REFLOG_ACTION is no longer set at the first call of
checkout_onto(), a call to comment_for_reflog() is added at the
beginning of this function.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase--helper.c   |  9 +++++++--
 git-rebase--interactive.sh | 16 ++--------------
 sequencer.c                | 30 ++++++++++++++++++++++++++++++
 sequencer.h                |  3 +++
 4 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 731a64971..6c789e78b 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;
+	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 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, EDIT_TODO
+		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -28,6 +28,7 @@ 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__VERBOSE(&verbose, N_("be verbose")),
 		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
 				CONTINUE),
 		OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
@@ -51,6 +52,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		OPT_CMDMODE(0, "edit-todo", &command,
 			    N_("edit the todo list during an interactive rebase"),
 			    EDIT_TODO),
+		OPT_CMDMODE(0, "prepare-branch", &command,
+			    N_("prepare the branch to be rebased"), PREPARE_BRANCH),
 		OPT_END()
 	};
 
@@ -94,5 +97,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!append_todo_help(0, keep_empty);
 	if (command == EDIT_TODO && argc == 1)
 		return !!edit_todo_list(flags);
+	if (command == PREPARE_BRANCH && argc == 2)
+		return !!prepare_branch_to_be_rebased(&opts, argv[1], verbose);
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2defe607f..77e972bb6 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -72,6 +72,7 @@ collapse_todo_ids() {
 
 # Switch to the branch in $into and notify it in the reflog
 checkout_onto () {
+	comment_for_reflog start
 	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
 	output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
 	git update-ref ORIG_HEAD $orig_head
@@ -119,19 +120,6 @@ initiate_action () {
 	esac
 }
 
-setup_reflog_action () {
-	comment_for_reflog start
-
-	if test ! -z "$switch_to"
-	then
-		GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
-		output git checkout "$switch_to" -- ||
-			die "$(eval_gettext "Could not checkout \$switch_to")"
-
-		comment_for_reflog start
-	fi
-}
-
 init_basic_state () {
 	orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
 	mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
@@ -211,7 +199,7 @@ git_rebase__interactive () {
 		return 0
 	fi
 
-	setup_reflog_action
+	git rebase--helper --prepare-branch "$switch_to" ${verbose:+--verbose}
 	init_basic_state
 
 	init_revisions_and_shortrevisions
diff --git a/sequencer.c b/sequencer.c
index 9e2b34a49..a61276d2c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3132,6 +3132,36 @@ static const char *reflog_message(struct replay_opts *opts,
 	return buf.buf;
 }
 
+static int run_git_checkout(struct replay_opts *opts, const char *commit,
+				int verbose, const char *action)
+{
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	cmd.git_cmd = 1;
+
+	argv_array_push(&cmd.args, "checkout");
+	argv_array_push(&cmd.args, commit);
+	argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
+
+	if (verbose)
+		return run_command(&cmd);
+	return run_command_silent_on_success(&cmd);
+}
+
+int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit,
+				 int verbose)
+{
+	const char *action;
+
+	if (commit && *commit) {
+		action = reflog_message(opts, "start", "checkout %s", commit);
+		if (run_git_checkout(opts, commit, verbose, action))
+			return error(_("Could not checkout %s"), commit);
+	}
+
+	return 0;
+}
+
 static const char rescheduled_advice[] =
 N_("Could not execute the todo command\n"
 "\n"
diff --git a/sequencer.h b/sequencer.h
index ffab798f1..3821420b0 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -106,6 +106,9 @@ int update_head_with_reflog(const struct commit *old_head,
 void commit_post_rewrite(const struct commit *current_head,
 			 const struct object_id *new_head);
 
+int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit,
+				 int verbose);
+
 #define SUMMARY_INITIAL_COMMIT   (1 << 0)
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
 void print_commit_summary(const char *prefix, const struct object_id *oid,
-- 
2.18.0


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

* [GSoC][PATCH v2 7/7] rebase -i: rewrite checkout_onto() in C
  2018-07-02 10:57 [GSoC][PATCH v2 0/7] rebase -i: rewrite some parts in C Alban Gruin
                   ` (5 preceding siblings ...)
  2018-07-02 10:57 ` [GSoC][PATCH v2 6/7] rebase -i: rewrite setup_reflog_action() in C Alban Gruin
@ 2018-07-02 10:57 ` " Alban Gruin
  2018-07-10 12:15 ` [GSoC][PATCH v3 00/13] rebase -i: rewrite some parts " Alban Gruin
  7 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2018-07-02 10:57 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, Alban Gruin

This rewrites checkout_onto() from shell to C.

A new command (“checkout-onto”) is added to rebase--helper.c. The shell
version is then stripped.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase--helper.c   |  7 ++++++-
 git-rebase--interactive.sh | 25 ++++---------------------
 sequencer.c                | 19 +++++++++++++++++++
 sequencer.h                |  3 +++
 4 files changed, 32 insertions(+), 22 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 6c789e78b..0091e094b 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -18,7 +18,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	enum {
 		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
 		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
-		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH
+		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH,
+		CHECKOUT_ONTO
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -54,6 +55,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 			    EDIT_TODO),
 		OPT_CMDMODE(0, "prepare-branch", &command,
 			    N_("prepare the branch to be rebased"), PREPARE_BRANCH),
+		OPT_CMDMODE(0, "checkout-onto", &command,
+			    N_("checkout a commit"), CHECKOUT_ONTO),
 		OPT_END()
 	};
 
@@ -99,5 +102,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!edit_todo_list(flags);
 	if (command == PREPARE_BRANCH && argc == 2)
 		return !!prepare_branch_to_be_rebased(&opts, argv[1], verbose);
+	if (command == CHECKOUT_ONTO && argc == 4)
+		return !!checkout_onto(&opts, argv[1], argv[2], argv[3], verbose);
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 77e972bb6..b68f108f2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -28,17 +28,6 @@ case "$comment_char" in
 	;;
 esac
 
-orig_reflog_action="$GIT_REFLOG_ACTION"
-
-comment_for_reflog () {
-	case "$orig_reflog_action" in
-	''|rebase*)
-		GIT_REFLOG_ACTION="rebase -i ($1)"
-		export GIT_REFLOG_ACTION
-		;;
-	esac
-}
-
 die_abort () {
 	apply_autostash
 	rm -rf "$state_dir"
@@ -70,14 +59,6 @@ collapse_todo_ids() {
 	git rebase--helper --shorten-ids
 }
 
-# Switch to the branch in $into and notify it in the reflog
-checkout_onto () {
-	comment_for_reflog start
-	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
-	output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
-	git update-ref ORIG_HEAD $orig_head
-}
-
 get_missing_commit_check_level () {
 	check_level=$(git config --get rebase.missingCommitsCheck)
 	check_level=${check_level:-ignore}
@@ -176,7 +157,8 @@ EOF
 
 	git rebase--helper --check-todo-list || {
 		ret=$?
-		checkout_onto
+		git rebase--helper --checkout-onto "$onto_name" "$onto" \
+		    "$orig_head" ${verbose:+--verbose}
 		exit $ret
 	}
 
@@ -186,7 +168,8 @@ EOF
 	onto="$(git rebase--helper --skip-unnecessary-picks)" ||
 	die "Could not skip unnecessary pick commands"
 
-	checkout_onto
+	git rebase--helper --checkout-onto "$onto_name" "$onto" "$orig_head" \
+	    ${verbose:+--verbose}
 	require_clean_work_tree "rebase"
 	exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
 	     --continue
diff --git a/sequencer.c b/sequencer.c
index a61276d2c..bc0836ac9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3162,6 +3162,25 @@ int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit,
 	return 0;
 }
 
+int checkout_onto(struct replay_opts *opts,
+		  const char *onto_name, const char *onto,
+		  const char *orig_head, unsigned verbose)
+{
+	struct object_id oid;
+	const char *action = reflog_message(opts, "start", "checkout %s", onto_name);
+
+	if (get_oid(orig_head, &oid))
+		return error(_("%s: not a valid OID"), orig_head);
+
+	if (run_git_checkout(opts, onto, verbose, action)) {
+		apply_autostash(opts);
+		sequencer_remove_state(opts);
+		return error(_("could not detach HEAD"));
+	}
+
+	return update_ref(NULL, "ORIG_HEAD", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+}
+
 static const char rescheduled_advice[] =
 N_("Could not execute the todo command\n"
 "\n"
diff --git a/sequencer.h b/sequencer.h
index 3821420b0..ba5e59f02 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -108,6 +108,9 @@ void commit_post_rewrite(const struct commit *current_head,
 
 int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit,
 				 int verbose);
+int checkout_onto(struct replay_opts *opts,
+		  const char *onto_name, const char *onto,
+		  const char *orig_head, unsigned verbose);
 
 #define SUMMARY_INITIAL_COMMIT   (1 << 0)
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
-- 
2.18.0


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

* Re: [GSoC][PATCH v2 1/7] sequencer: make two functions and an enum from sequencer.c public
  2018-07-02 10:57 ` [GSoC][PATCH v2 1/7] sequencer: make two functions and an enum from sequencer.c public Alban Gruin
@ 2018-07-03 20:20   ` Junio C Hamano
  2018-07-05 15:35     ` Alban Gruin
  0 siblings, 1 reply; 50+ messages in thread
From: Junio C Hamano @ 2018-07-03 20:20 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

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

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

The new name definitely is better than "check_level" in the global
context, but "missing_commit" is much less important thing to say
than "this symbol is to be used when driving 'rebase' (or even
'rebase-i')", I think.  "enum rebase_i_drop_commit_check" with
"get_rebase_i_drop_commit_check()" perhaps?

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

* Re: [GSoC][PATCH v2 2/7] rebase--interactive: rewrite append_todo_help() in C
  2018-07-02 10:57 ` [GSoC][PATCH v2 2/7] rebase--interactive: rewrite append_todo_help() in C Alban Gruin
@ 2018-07-03 20:29   ` Junio C Hamano
  0 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2018-07-03 20:29 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

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

> +	ret = fputs(buf.buf, todo);
> +	if (ret < 0)
> +		error_errno(_("Could not append help text to '%s'"), rebase_path_todo());

Error checking fputs() return is an improvement from the version
that has been queued on 'pu'.  I think messages given to error() and
friends begin with lowercase uncapitalized by convention, though.

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

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

* Re: [GSoC][PATCH v2 3/7] editor: add a function to launch the sequence editor
  2018-07-02 10:57 ` [GSoC][PATCH v2 3/7] editor: add a function to launch the sequence editor Alban Gruin
@ 2018-07-03 20:32   ` Junio C Hamano
  0 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2018-07-03 20:32 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

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

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

Looks the same from the previous round, and the changes look good.

You can say "Unchanged from what has been queued on 'pu'" or
somesuch to save reviewer's time after the line with three-dashes

Thansk.

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

* Re: [GSoC][PATCH v2 5/7] sequencer: add a new function to silence a command, except if it fails.
  2018-07-02 10:57 ` [GSoC][PATCH v2 5/7] sequencer: add a new function to silence a command, except if it fails Alban Gruin
@ 2018-07-03 20:36   ` Junio C Hamano
  0 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2018-07-03 20:36 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

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

> Subject: Re: [GSoC][PATCH v2 5/7] sequencer: add a new function to silence a command, except if it fails.

Lose the full-stop at the end.

> This adds a new function, run_command_silent_on_success(), to
> redirect the stdout and stderr of a command to a strbuf, and then to run
> that command. This strbuf is printed only if the command fails. It is
> functionnaly similar to output() from git-rebase.sh.
>
> run_git_commit() is then refactored to use of
> run_command_silent_on_success().
>
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  sequencer.c | 49 ++++++++++++++++++++++++-------------------------
>  1 file changed, 24 insertions(+), 25 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 57fd58bc1..9e2b34a49 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -768,6 +768,24 @@ N_("you have staged changes in your working tree\n"
>  #define VERIFY_MSG  (1<<4)
>  #define CREATE_ROOT_COMMIT (1<<5)
>  
> +static int run_command_silent_on_success(struct child_process *cmd)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	int rc;
> +
> +	cmd->stdout_to_stderr = 1;
> +	rc = pipe_command(cmd,
> +			  NULL, 0,
> +			  /* stdout is already redirected */

Are we reviewing the correct version?

I thought we discarded this comment in the previous round.

> +			  NULL, 0,
> +			  &buf, 0);


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

* Re: [GSoC][PATCH v2 6/7] rebase -i: rewrite setup_reflog_action() in C
  2018-07-02 10:57 ` [GSoC][PATCH v2 6/7] rebase -i: rewrite setup_reflog_action() in C Alban Gruin
@ 2018-07-03 20:37   ` Junio C Hamano
  2018-07-06 12:58     ` Johannes Schindelin
  0 siblings, 1 reply; 50+ messages in thread
From: Junio C Hamano @ 2018-07-03 20:37 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

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

> +static int run_git_checkout(struct replay_opts *opts, const char *commit,
> +				int verbose, const char *action)
> +{
> +	struct child_process cmd = CHILD_PROCESS_INIT;
> +
> +	cmd.git_cmd = 1;
> +
> +	argv_array_push(&cmd.args, "checkout");
> +	argv_array_push(&cmd.args, commit);
> +	argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
> +
> +	if (verbose)
> +		return run_command(&cmd);
> +	return run_command_silent_on_success(&cmd);

I thought we made this into

	if verbose
		return run_command
	else
		return run_command_silently

to help readers in the previous round already.

Are we reviewing the correct version?

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

* Re: [GSoC][PATCH v2 1/7] sequencer: make two functions and an enum from sequencer.c public
  2018-07-03 20:20   ` Junio C Hamano
@ 2018-07-05 15:35     ` Alban Gruin
  0 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2018-07-05 15:35 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

Hi Junio,

Le 03/07/2018 à 22:20, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
> 
>> -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)
> 
> The new name definitely is better than "check_level" in the global
> context, but "missing_commit" is much less important thing to say
> than "this symbol is to be used when driving 'rebase' (or even
> 'rebase-i')", I think.  "enum rebase_i_drop_commit_check" with
> "get_rebase_i_drop_commit_check()" perhaps?
> 

I don’t really like those names, but the function and the enum should
eventually move to rebase-interactive.c and become static again, so we
could revert their names in due course.

Cheers,
Alban


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

* Re: [GSoC][PATCH v2 6/7] rebase -i: rewrite setup_reflog_action() in C
  2018-07-03 20:37   ` Junio C Hamano
@ 2018-07-06 12:58     ` Johannes Schindelin
  2018-07-06 15:02       ` Junio C Hamano
  0 siblings, 1 reply; 50+ messages in thread
From: Johannes Schindelin @ 2018-07-06 12:58 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Alban Gruin, git, Stefan Beller, Christian Couder, Pratik Karki,
	phillip.wood

Hi Junio,

On Tue, 3 Jul 2018, Junio C Hamano wrote:

> Alban Gruin <alban.gruin@gmail.com> writes:
> 
> > +static int run_git_checkout(struct replay_opts *opts, const char *commit,
> > +				int verbose, const char *action)
> > +{
> > +	struct child_process cmd = CHILD_PROCESS_INIT;
> > +
> > +	cmd.git_cmd = 1;
> > +
> > +	argv_array_push(&cmd.args, "checkout");
> > +	argv_array_push(&cmd.args, commit);
> > +	argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
> > +
> > +	if (verbose)
> > +		return run_command(&cmd);
> > +	return run_command_silent_on_success(&cmd);
> 
> I thought we made this into
> 
> 	if verbose
> 		return run_command
> 	else
> 		return run_command_silently
> 
> to help readers in the previous round already.

FWIW we had quite a couple of reviews in the recent years which pointed
out "unnecessary else" after returning or die()ing. Maybe we should make
up our minds, and set a consistent rule to follow.

Ciao,
Dscho

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

* Re: [GSoC][PATCH v2 6/7] rebase -i: rewrite setup_reflog_action() in C
  2018-07-06 12:58     ` Johannes Schindelin
@ 2018-07-06 15:02       ` Junio C Hamano
  2018-07-06 18:54         ` Johannes Schindelin
  0 siblings, 1 reply; 50+ messages in thread
From: Junio C Hamano @ 2018-07-06 15:02 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Alban Gruin, git, Stefan Beller, Christian Couder, Pratik Karki,
	phillip.wood

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> I thought we made this into
>> 
>> 	if verbose
>> 		return run_command
>> 	else
>> 		return run_command_silently
>> 
>> to help readers in the previous round already.
>
> FWIW we had quite a couple of reviews in the recent years which pointed
> out "unnecessary else" after returning or die()ing. Maybe we should make
> up our minds, and set a consistent rule to follow.

FWIW the pattern you are referring to is

	do something;
	if (error detected) {
		return error(...);
	} else {
		perform
		remaining
		actions
		the function needs
		to complete
	        its task;
	}

and those reviewers (including me) are absolutely right that such an
"else" is not just unnecessary but actively harms readability of the
flow of logic.

I am also absolutely right when I say what is quoted at the top
makes 100% more sense than

	if (verbose)
        	return run_command();
	return run_command_silently();

as these two are about doing the same thing a bit differently.  

The way to think about the latter is that we won't have this "if
(verbose)" if there were a variant of run_command() that took a set
of option bits among which is a verbose bit, but instead would have
a single call to that function that returns..  So it's not like "in
an exceptional case, return after calling this function; otherwise
keep going, which happens to only return after calling another".  It
is more like "here we need to return after spawning a command, but
depending on this bit, we spawn the command using a different
function".

Good programers recognize the difference between these two patterns
without being told, and mentors should teach and GSoC student should
learn that an overly simplified rule like "when 'if' block ends with
return, do not write 'else'" is harmful.

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

* Re: [GSoC][PATCH v2 6/7] rebase -i: rewrite setup_reflog_action() in C
  2018-07-06 15:02       ` Junio C Hamano
@ 2018-07-06 18:54         ` Johannes Schindelin
  2018-07-06 21:55           ` Junio C Hamano
  0 siblings, 1 reply; 50+ messages in thread
From: Johannes Schindelin @ 2018-07-06 18:54 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Alban Gruin, git, Stefan Beller, Christian Couder, Pratik Karki,
	phillip.wood

Hi Junio,

On Fri, 6 Jul 2018, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> I thought we made this into
> >> 
> >> 	if verbose
> >> 		return run_command
> >> 	else
> >> 		return run_command_silently
> >> 
> >> to help readers in the previous round already.
> >
> > FWIW we had quite a couple of reviews in the recent years which pointed
> > out "unnecessary else" after returning or die()ing. Maybe we should make
> > up our minds, and set a consistent rule to follow.
> 
> FWIW the pattern you are referring to is
> 
> 	do something;
> 	if (error detected) {
> 		return error(...);
> 	} else {
> 		perform
> 		remaining
> 		actions
> 		the function needs
> 		to complete
> 	        its task;
> 	}
> 
> and those reviewers (including me) are absolutely right that such an
> "else" is not just unnecessary but actively harms readability of the
> flow of logic.
> 
> I am also absolutely right when I say what is quoted at the top
> makes 100% more sense than
> 
> 	if (verbose)
>         	return run_command();
> 	return run_command_silently();
> 
> as these two are about doing the same thing a bit differently.  
> 
> The way to think about the latter is that we won't have this "if
> (verbose)" if there were a variant of run_command() that took a set
> of option bits among which is a verbose bit, but instead would have
> a single call to that function that returns..  So it's not like "in
> an exceptional case, return after calling this function; otherwise
> keep going, which happens to only return after calling another".  It
> is more like "here we need to return after spawning a command, but
> depending on this bit, we spawn the command using a different
> function".
> 
> Good programers recognize the difference between these two patterns
> without being told, and mentors should teach and GSoC student should
> learn that an overly simplified rule like "when 'if' block ends with
> return, do not write 'else'" is harmful.

You recently also suggested this if...else... dance to Pratik, where it
was not at all about doing the same thing slightly differently, but rather
two different things: 1) return an error because execvp() returned an
error, 2) indicate a serious bug (and you did not even suggest using BUG()
IIRC which is also wrong).

Maybe I am the only one who finds this inconsistent and confusing. If that
is that case, I'll quiet down.

Ciao,
Johannes

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

* Re: [GSoC][PATCH v2 6/7] rebase -i: rewrite setup_reflog_action() in C
  2018-07-06 18:54         ` Johannes Schindelin
@ 2018-07-06 21:55           ` Junio C Hamano
  2018-07-07 16:40             ` Junio C Hamano
  0 siblings, 1 reply; 50+ messages in thread
From: Junio C Hamano @ 2018-07-06 21:55 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Alban Gruin, git, Stefan Beller, Christian Couder, Pratik Karki,
	phillip.wood

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> You recently also suggested this if...else... dance to Pratik, where it
> was not at all about doing the same thing slightly differently, but rather
> two different things: 1) return an error because execvp() returned an
> error, 2) indicate a serious bug (and you did not even suggest using BUG()
> IIRC which is also wrong).

I was going to avoid wasting time on this topic, but I happened to
have a chance to review the execvp() thing.  

I actually think that is a good example of doing the same thing
slightly differently.  We want to abort the process when execvp()
gives control back to that caller, and we want to do so with
die_errno() when we know we have errno (i.e. sane_execvp() gave us
negative).  When sane_execvp() gave control back to us with
non-negative return, we cannot use die_errno(), but we would want to
abort the process regardless, as it is an indication that the callee
is not sane after all ;-)  It is a bug in underlying execvp() that
we cannot do much about, though.



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

* Re: [GSoC][PATCH v2 6/7] rebase -i: rewrite setup_reflog_action() in C
  2018-07-06 21:55           ` Junio C Hamano
@ 2018-07-07 16:40             ` Junio C Hamano
  0 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2018-07-07 16:40 UTC (permalink / raw)
  To: git
  Cc: Alban Gruin, Stefan Beller, Christian Couder, Pratik Karki,
	phillip.wood, Johannes Schindelin

Junio C Hamano <gitster@pobox.com> writes:

> I actually think that is a good example of doing the same thing
> slightly differently.  ...

[jc: Beating the dead horse, only to avoid misleading those who are
learning from the sidelines...]

The above was a stupid thing to say and end the message with, as it
made it sound as if there are only two cases and we have a simple
(but not that simple as alluded to earlier by Dscho that essentially
says not to write 'else' when 'if' body returns or exits) rule that
you should blindly apply after telling these two cases apart.  

That is a total opposite of the message that should be read from the
whole thread.


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

* [GSoC][PATCH v3 00/13] rebase -i: rewrite some parts in C
  2018-07-02 10:57 [GSoC][PATCH v2 0/7] rebase -i: rewrite some parts in C Alban Gruin
                   ` (6 preceding siblings ...)
  2018-07-02 10:57 ` [GSoC][PATCH v2 7/7] rebase -i: rewrite checkout_onto() " Alban Gruin
@ 2018-07-10 12:15 ` " Alban Gruin
  2018-07-10 12:15   ` [GSoC][PATCH v3 01/13] sequencer: make two functions and an enum from sequencer.c public Alban Gruin
                     ` (12 more replies)
  7 siblings, 13 replies; 50+ messages in thread
From: Alban Gruin @ 2018-07-10 12:15 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, Alban Gruin

This patch series rewrites some parts of interactive rebase from shell
to C:

 - append_todo_help(). The C version covers a bit more than the shell
   version.

 - The edit-todo functionnality.

 - The reflog operations.

 - complete_action().

 - init_revisions_and_shortrevisions().

This patch series is based on master (as of 2018-07-10).

Changes since v2:

 - Lowercasing error messages.

 - Removing a comment from run_command_silent_on_success().

 - Using the `else` keyword to call run_command_silent_on_success() or
   run_command() in run_git_commit() and run_git_checkout().

 - Dropping the `verbose` parameter in run_git_checkout(),
   prepare_branch_to_be_rebased(), and checkout_onto(), as the
   replay_opts structure already has a `verbose` field.

 - Rewriting complete_action() and init_revisions_and_shortrevisions().

Alban Gruin (13):
  sequencer: make two functions and an enum from sequencer.c public
  rebase--interactive: rewrite append_todo_help() in C
  editor: add a function to launch the sequence editor
  rebase-interactive: rewrite the edit-todo functionality in C
  sequencer: add a new function to silence a command, except if it fails
  rebase -i: rewrite setup_reflog_action() in C
  rebase -i: rewrite checkout_onto() in C
  sequencer: refactor append_todo_help() to write its message to a
    buffer
  sequencer: change the way skip_unnecessary_picks() returns its result
  rebase--interactive: rewrite complete_action() in C
  rebase--interactive: remove unused modes and functions
  rebase -i: implement the logic to initialize the variable $revision in
    C
  rebase -i: rewrite the rest of init_revisions_and_shortrevisions in C

 Makefile                   |   1 +
 builtin/rebase--helper.c   | 123 +++++++++++++++++----
 cache.h                    |   1 +
 editor.c                   |  27 ++++-
 git-rebase--interactive.sh | 216 ++-----------------------------------
 git-rebase.sh              |   2 +-
 rebase-interactive.c       |  96 +++++++++++++++++
 rebase-interactive.h       |  11 ++
 sequencer.c                | 215 +++++++++++++++++++++++++++++-------
 sequencer.h                |  15 ++-
 strbuf.h                   |   2 +
 11 files changed, 439 insertions(+), 270 deletions(-)
 create mode 100644 rebase-interactive.c
 create mode 100644 rebase-interactive.h

-- 
2.18.0


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

* [GSoC][PATCH v3 01/13] sequencer: make two functions and an enum from sequencer.c public
  2018-07-10 12:15 ` [GSoC][PATCH v3 00/13] rebase -i: rewrite some parts " Alban Gruin
@ 2018-07-10 12:15   ` Alban Gruin
  2018-07-10 17:53     ` Junio C Hamano
  2018-07-10 12:15   ` [GSoC][PATCH v3 02/13] rebase--interactive: rewrite append_todo_help() in C Alban Gruin
                     ` (11 subsequent siblings)
  12 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2018-07-10 12:15 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, Alban Gruin

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 function and this enum will eventually be moved to
rebase-interactive.c and become static again, so no special attention
was given to the naming.

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>
---
Unchanged from v2.

 sequencer.c | 22 +++++++++-------------
 sequencer.h |  8 ++++++++
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 5354d4d51..57fd58bc1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -51,7 +51,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
@@ -4221,24 +4221,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);
@@ -4250,7 +4246,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;
@@ -4267,7 +4263,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 */
@@ -4302,7 +4298,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


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

* [GSoC][PATCH v3 02/13] rebase--interactive: rewrite append_todo_help() in C
  2018-07-10 12:15 ` [GSoC][PATCH v3 00/13] rebase -i: rewrite some parts " Alban Gruin
  2018-07-10 12:15   ` [GSoC][PATCH v3 01/13] sequencer: make two functions and an enum from sequencer.c public Alban Gruin
@ 2018-07-10 12:15   ` Alban Gruin
  2018-07-10 12:21     ` Alban Gruin
  2018-07-10 17:56     ` Junio C Hamano
  2018-07-10 12:15   ` [GSoC][PATCH v3 03/13] editor: add a function to launch the sequence editor Alban Gruin
                     ` (10 subsequent siblings)
  12 siblings, 2 replies; 50+ messages in thread
From: Alban Gruin @ 2018-07-10 12:15 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, Alban Gruin

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>
---
Unchanged from what have been queued on `pu` (ag/rebase-i-in-c), and
from v2.

 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

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


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

* [GSoC][PATCH v3 03/13] editor: add a function to launch the sequence editor
  2018-07-10 12:15 ` [GSoC][PATCH v3 00/13] rebase -i: rewrite some parts " Alban Gruin
  2018-07-10 12:15   ` [GSoC][PATCH v3 01/13] sequencer: make two functions and an enum from sequencer.c public Alban Gruin
  2018-07-10 12:15   ` [GSoC][PATCH v3 02/13] rebase--interactive: rewrite append_todo_help() in C Alban Gruin
@ 2018-07-10 12:15   ` Alban Gruin
  2018-07-10 12:23     ` Alban Gruin
  2018-07-10 17:58     ` Junio C Hamano
  2018-07-10 12:15   ` [GSoC][PATCH v3 04/13] rebase-interactive: rewrite the edit-todo functionality in C Alban Gruin
                     ` (9 subsequent siblings)
  12 siblings, 2 replies; 50+ messages in thread
From: Alban Gruin @ 2018-07-10 12:15 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, 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 d49092d94..33fa70f55 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.18.0


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

* [GSoC][PATCH v3 04/13] rebase-interactive: rewrite the edit-todo functionality in C
  2018-07-10 12:15 ` [GSoC][PATCH v3 00/13] rebase -i: rewrite some parts " Alban Gruin
                     ` (2 preceding siblings ...)
  2018-07-10 12:15   ` [GSoC][PATCH v3 03/13] editor: add a function to launch the sequence editor Alban Gruin
@ 2018-07-10 12:15   ` Alban Gruin
  2018-07-10 18:00     ` Junio C Hamano
  2018-07-10 12:15   ` [GSoC][PATCH v3 05/13] sequencer: add a new function to silence a command, except if it fails Alban Gruin
                     ` (8 subsequent siblings)
  12 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2018-07-10 12:15 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, 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>
---
Unchanged from v2.

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 d7996bc8d..403ecbf3c 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] 50+ messages in thread

* [GSoC][PATCH v3 05/13] sequencer: add a new function to silence a command, except if it fails
  2018-07-10 12:15 ` [GSoC][PATCH v3 00/13] rebase -i: rewrite some parts " Alban Gruin
                     ` (3 preceding siblings ...)
  2018-07-10 12:15   ` [GSoC][PATCH v3 04/13] rebase-interactive: rewrite the edit-todo functionality in C Alban Gruin
@ 2018-07-10 12:15   ` Alban Gruin
  2018-07-10 18:13     ` Junio C Hamano
  2018-07-10 12:15   ` [GSoC][PATCH v3 06/13] rebase -i: rewrite setup_reflog_action() in C Alban Gruin
                     ` (7 subsequent siblings)
  12 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2018-07-10 12:15 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, Alban Gruin

This adds a new function, run_command_silent_on_success(), to
redirect the stdout and stderr of a command to a strbuf, and then to run
that command. This strbuf is printed only if the command fails. It is
functionnaly similar to output() from git-rebase.sh.

run_git_commit() is then refactored to use of
run_command_silent_on_success().

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 51 +++++++++++++++++++++++++--------------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 57fd58bc1..1b5d50298 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -768,6 +768,23 @@ N_("you have staged changes in your working tree\n"
 #define VERIFY_MSG  (1<<4)
 #define CREATE_ROOT_COMMIT (1<<5)
 
+static int run_command_silent_on_success(struct child_process *cmd)
+{
+	struct strbuf buf = STRBUF_INIT;
+	int rc;
+
+	cmd->stdout_to_stderr = 1;
+	rc = pipe_command(cmd,
+			  NULL, 0,
+			  NULL, 0,
+			  &buf, 0);
+
+	if (rc)
+		fputs(buf.buf, stderr);
+	strbuf_release(&buf);
+	return rc;
+}
+
 /*
  * If we are cherry-pick, and if the merge did not result in
  * hand-editing, we will hit this commit and inherit the original
@@ -822,18 +839,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 
 	cmd.git_cmd = 1;
 
-	if (is_rebase_i(opts)) {
-		if (!(flags & EDIT_MSG)) {
-			cmd.stdout_to_stderr = 1;
-			cmd.err = -1;
-		}
-
-		if (read_env_script(&cmd.env_array)) {
-			const char *gpg_opt = gpg_sign_opt_quoted(opts);
+	if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {
+		const char *gpg_opt = gpg_sign_opt_quoted(opts);
 
-			return error(_(staged_changes_advice),
-				     gpg_opt, gpg_opt);
-		}
+		return error(_(staged_changes_advice),
+			     gpg_opt, gpg_opt);
 	}
 
 	argv_array_push(&cmd.args, "commit");
@@ -863,21 +873,10 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	if (opts->allow_empty_message)
 		argv_array_push(&cmd.args, "--allow-empty-message");
 
-	if (cmd.err == -1) {
-		/* hide stderr on success */
-		struct strbuf buf = STRBUF_INIT;
-		int rc = pipe_command(&cmd,
-				      NULL, 0,
-				      /* stdout is already redirected */
-				      NULL, 0,
-				      &buf, 0);
-		if (rc)
-			fputs(buf.buf, stderr);
-		strbuf_release(&buf);
-		return rc;
-	}
-
-	return run_command(&cmd);
+	if (is_rebase_i(opts) && !(flags & EDIT_MSG))
+		return run_command_silent_on_success(&cmd);
+	else
+		return run_command(&cmd);
 }
 
 static int rest_is_empty(const struct strbuf *sb, int start)
-- 
2.18.0


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

* [GSoC][PATCH v3 06/13] rebase -i: rewrite setup_reflog_action() in C
  2018-07-10 12:15 ` [GSoC][PATCH v3 00/13] rebase -i: rewrite some parts " Alban Gruin
                     ` (4 preceding siblings ...)
  2018-07-10 12:15   ` [GSoC][PATCH v3 05/13] sequencer: add a new function to silence a command, except if it fails Alban Gruin
@ 2018-07-10 12:15   ` Alban Gruin
  2018-07-10 18:20     ` Junio C Hamano
  2018-07-10 12:15   ` [GSoC][PATCH v3 07/13] rebase -i: rewrite checkout_onto() " Alban Gruin
                     ` (6 subsequent siblings)
  12 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2018-07-10 12:15 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, Alban Gruin

This rewrites (the misnamed) setup_reflog_action() from shell to C. The
new version is called prepare_branch_to_be_rebased().

A new command is added to rebase--helper.c, “checkout-base”, as well as
a new flag, “verbose”, to avoid silencing the output of the checkout
operation called by checkout_base_commit().

The function `run_git_checkout()` will also be used in the next commit,
therefore its code is not part of `checkout_base_commit()`.

The shell version is then stripped in favour of a call to the helper.

As $GIT_REFLOG_ACTION is no longer set at the first call of
checkout_onto(), a call to comment_for_reflog() is added at the
beginning of this function.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase--helper.c   | 10 ++++++++--
 git-rebase--interactive.sh | 16 ++--------------
 sequencer.c                | 30 ++++++++++++++++++++++++++++++
 sequencer.h                |  2 ++
 4 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 731a64971..76bdc6fdb 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;
+	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 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, EDIT_TODO
+		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -28,6 +28,7 @@ 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__VERBOSE(&verbose, N_("be verbose")),
 		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
 				CONTINUE),
 		OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
@@ -51,6 +52,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		OPT_CMDMODE(0, "edit-todo", &command,
 			    N_("edit the todo list during an interactive rebase"),
 			    EDIT_TODO),
+		OPT_CMDMODE(0, "prepare-branch", &command,
+			    N_("prepare the branch to be rebased"), PREPARE_BRANCH),
 		OPT_END()
 	};
 
@@ -60,6 +63,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	opts.action = REPLAY_INTERACTIVE_REBASE;
 	opts.allow_ff = 1;
 	opts.allow_empty = 1;
+	opts.verbose = verbose;
 
 	argc = parse_options(argc, argv, NULL, options,
 			builtin_rebase_helper_usage, PARSE_OPT_KEEP_ARGV0);
@@ -94,5 +98,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!append_todo_help(0, keep_empty);
 	if (command == EDIT_TODO && argc == 1)
 		return !!edit_todo_list(flags);
+	if (command == PREPARE_BRANCH && argc == 2)
+		return !!prepare_branch_to_be_rebased(&opts, argv[1]);
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2defe607f..77e972bb6 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -72,6 +72,7 @@ collapse_todo_ids() {
 
 # Switch to the branch in $into and notify it in the reflog
 checkout_onto () {
+	comment_for_reflog start
 	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
 	output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
 	git update-ref ORIG_HEAD $orig_head
@@ -119,19 +120,6 @@ initiate_action () {
 	esac
 }
 
-setup_reflog_action () {
-	comment_for_reflog start
-
-	if test ! -z "$switch_to"
-	then
-		GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
-		output git checkout "$switch_to" -- ||
-			die "$(eval_gettext "Could not checkout \$switch_to")"
-
-		comment_for_reflog start
-	fi
-}
-
 init_basic_state () {
 	orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
 	mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
@@ -211,7 +199,7 @@ git_rebase__interactive () {
 		return 0
 	fi
 
-	setup_reflog_action
+	git rebase--helper --prepare-branch "$switch_to" ${verbose:+--verbose}
 	init_basic_state
 
 	init_revisions_and_shortrevisions
diff --git a/sequencer.c b/sequencer.c
index 1b5d50298..b5ea35f21 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3132,6 +3132,36 @@ static const char *reflog_message(struct replay_opts *opts,
 	return buf.buf;
 }
 
+static int run_git_checkout(struct replay_opts *opts, const char *commit,
+			    const char *action)
+{
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	cmd.git_cmd = 1;
+
+	argv_array_push(&cmd.args, "checkout");
+	argv_array_push(&cmd.args, commit);
+	argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
+
+	if (opts->verbose)
+		return run_command(&cmd);
+	else
+		return run_command_silent_on_success(&cmd);
+}
+
+int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit)
+{
+	const char *action;
+
+	if (commit && *commit) {
+		action = reflog_message(opts, "start", "checkout %s", commit);
+		if (run_git_checkout(opts, commit, action))
+			return error(_("could not checkout %s"), commit);
+	}
+
+	return 0;
+}
+
 static const char rescheduled_advice[] =
 N_("Could not execute the todo command\n"
 "\n"
diff --git a/sequencer.h b/sequencer.h
index ffab798f1..93da713fe 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -106,6 +106,8 @@ int update_head_with_reflog(const struct commit *old_head,
 void commit_post_rewrite(const struct commit *current_head,
 			 const struct object_id *new_head);
 
+int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit);
+
 #define SUMMARY_INITIAL_COMMIT   (1 << 0)
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
 void print_commit_summary(const char *prefix, const struct object_id *oid,
-- 
2.18.0


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

* [GSoC][PATCH v3 07/13] rebase -i: rewrite checkout_onto() in C
  2018-07-10 12:15 ` [GSoC][PATCH v3 00/13] rebase -i: rewrite some parts " Alban Gruin
                     ` (5 preceding siblings ...)
  2018-07-10 12:15   ` [GSoC][PATCH v3 06/13] rebase -i: rewrite setup_reflog_action() in C Alban Gruin
@ 2018-07-10 12:15   ` " Alban Gruin
  2018-07-10 18:22     ` Junio C Hamano
  2018-07-10 12:15   ` [GSoC][PATCH v3 08/13] sequencer: refactor append_todo_help() to write its message to a buffer Alban Gruin
                     ` (5 subsequent siblings)
  12 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2018-07-10 12:15 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, Alban Gruin

This rewrites checkout_onto() from shell to C.

A new command (“checkout-onto”) is added to rebase--helper.c. The shell
version is then stripped.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase--helper.c   |  7 ++++++-
 git-rebase--interactive.sh | 25 ++++---------------------
 sequencer.c                | 19 +++++++++++++++++++
 sequencer.h                |  3 +++
 4 files changed, 32 insertions(+), 22 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 76bdc6fdb..1d9595bdb 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -18,7 +18,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	enum {
 		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
 		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
-		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH
+		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH,
+		CHECKOUT_ONTO
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -54,6 +55,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 			    EDIT_TODO),
 		OPT_CMDMODE(0, "prepare-branch", &command,
 			    N_("prepare the branch to be rebased"), PREPARE_BRANCH),
+		OPT_CMDMODE(0, "checkout-onto", &command,
+			    N_("checkout a commit"), CHECKOUT_ONTO),
 		OPT_END()
 	};
 
@@ -100,5 +103,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!edit_todo_list(flags);
 	if (command == PREPARE_BRANCH && argc == 2)
 		return !!prepare_branch_to_be_rebased(&opts, argv[1]);
+	if (command == CHECKOUT_ONTO && argc == 4)
+		return !!checkout_onto(&opts, argv[1], argv[2], argv[3]);
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 77e972bb6..b68f108f2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -28,17 +28,6 @@ case "$comment_char" in
 	;;
 esac
 
-orig_reflog_action="$GIT_REFLOG_ACTION"
-
-comment_for_reflog () {
-	case "$orig_reflog_action" in
-	''|rebase*)
-		GIT_REFLOG_ACTION="rebase -i ($1)"
-		export GIT_REFLOG_ACTION
-		;;
-	esac
-}
-
 die_abort () {
 	apply_autostash
 	rm -rf "$state_dir"
@@ -70,14 +59,6 @@ collapse_todo_ids() {
 	git rebase--helper --shorten-ids
 }
 
-# Switch to the branch in $into and notify it in the reflog
-checkout_onto () {
-	comment_for_reflog start
-	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
-	output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
-	git update-ref ORIG_HEAD $orig_head
-}
-
 get_missing_commit_check_level () {
 	check_level=$(git config --get rebase.missingCommitsCheck)
 	check_level=${check_level:-ignore}
@@ -176,7 +157,8 @@ EOF
 
 	git rebase--helper --check-todo-list || {
 		ret=$?
-		checkout_onto
+		git rebase--helper --checkout-onto "$onto_name" "$onto" \
+		    "$orig_head" ${verbose:+--verbose}
 		exit $ret
 	}
 
@@ -186,7 +168,8 @@ EOF
 	onto="$(git rebase--helper --skip-unnecessary-picks)" ||
 	die "Could not skip unnecessary pick commands"
 
-	checkout_onto
+	git rebase--helper --checkout-onto "$onto_name" "$onto" "$orig_head" \
+	    ${verbose:+--verbose}
 	require_clean_work_tree "rebase"
 	exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
 	     --continue
diff --git a/sequencer.c b/sequencer.c
index b5ea35f21..2b6ddc6cf 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3162,6 +3162,25 @@ int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit)
 	return 0;
 }
 
+int checkout_onto(struct replay_opts *opts,
+		  const char *onto_name, const char *onto,
+		  const char *orig_head)
+{
+	struct object_id oid;
+	const char *action = reflog_message(opts, "start", "checkout %s", onto_name);
+
+	if (get_oid(orig_head, &oid))
+		return error(_("%s: not a valid OID"), orig_head);
+
+	if (run_git_checkout(opts, onto, action)) {
+		apply_autostash(opts);
+		sequencer_remove_state(opts);
+		return error(_("could not detach HEAD"));
+	}
+
+	return update_ref(NULL, "ORIG_HEAD", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+}
+
 static const char rescheduled_advice[] =
 N_("Could not execute the todo command\n"
 "\n"
diff --git a/sequencer.h b/sequencer.h
index 93da713fe..11a533461 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -107,6 +107,9 @@ void commit_post_rewrite(const struct commit *current_head,
 			 const struct object_id *new_head);
 
 int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit);
+int checkout_onto(struct replay_opts *opts,
+		  const char *onto_name, const char *onto,
+		  const char *orig_head);
 
 #define SUMMARY_INITIAL_COMMIT   (1 << 0)
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
-- 
2.18.0


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

* [GSoC][PATCH v3 08/13] sequencer: refactor append_todo_help() to write its message to a buffer
  2018-07-10 12:15 ` [GSoC][PATCH v3 00/13] rebase -i: rewrite some parts " Alban Gruin
                     ` (6 preceding siblings ...)
  2018-07-10 12:15   ` [GSoC][PATCH v3 07/13] rebase -i: rewrite checkout_onto() " Alban Gruin
@ 2018-07-10 12:15   ` Alban Gruin
  2018-07-10 18:30     ` Junio C Hamano
  2018-07-10 12:15   ` [GSoC][PATCH v3 09/13] sequencer: change the way skip_unnecessary_picks() returns its result Alban Gruin
                     ` (4 subsequent siblings)
  12 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2018-07-10 12:15 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, Alban Gruin

This refactors append_todo_help() to write its message to a buffer
instead of the todo-list.  This is needed for the rewrite of
complete_action(), which will come after the next commit.

As rebase--helper still needs the file manipulation part of
append_todo_help(), it is extracted to a temporary function,
append_todo_help_to_file().  This function will go away after the
rewrite of complete_action().

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase--helper.c |  2 +-
 rebase-interactive.c     | 45 ++++++++++++++++++++++++++++------------
 rebase-interactive.h     |  7 ++++++-
 3 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 1d9595bdb..b9af96af7 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -98,7 +98,7 @@ 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(0, keep_empty);
+		return !!append_todo_help_to_file(0, keep_empty);
 	if (command == EDIT_TODO && argc == 1)
 		return !!edit_todo_list(flags);
 	if (command == PREPARE_BRANCH && argc == 2)
diff --git a/rebase-interactive.c b/rebase-interactive.c
index 403ecbf3c..d8b946559 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -4,11 +4,9 @@
 #include "sequencer.h"
 #include "strbuf.h"
 
-int append_todo_help(unsigned edit_todo, unsigned keep_empty)
+void append_todo_help(unsigned edit_todo, unsigned keep_empty,
+		      struct strbuf *buf)
 {
-	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"
@@ -26,11 +24,7 @@ int append_todo_help(unsigned edit_todo, unsigned keep_empty)
 "\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));
+	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' "
@@ -39,7 +33,7 @@ int append_todo_help(unsigned edit_todo, unsigned keep_empty)
 		msg = _("\nIf you remove a line here "
 			 "THAT COMMIT WILL BE LOST.\n");
 
-	strbuf_add_commented_lines(&buf, msg, strlen(msg));
+	strbuf_add_commented_lines(buf, msg, strlen(msg));
 
 	if (edit_todo)
 		msg = _("\nYou are editing the todo file "
@@ -50,12 +44,25 @@ int append_todo_help(unsigned edit_todo, unsigned keep_empty)
 		msg = _("\nHowever, if you remove everything, "
 			"the rebase will be aborted.\n\n");
 
-	strbuf_add_commented_lines(&buf, msg, strlen(msg));
+	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));
+		strbuf_add_commented_lines(buf, msg, strlen(msg));
 	}
+}
+
+int append_todo_help_to_file(unsigned edit_todo, unsigned keep_empty)
+{
+	struct strbuf buf = STRBUF_INIT;
+	FILE *todo;
+	int ret;
+
+	todo = fopen_or_warn(rebase_path_todo(), "a");
+	if (!todo)
+		return 1;
+
+	append_todo_help(edit_todo, keep_empty, &buf);
 
 	ret = fputs(buf.buf, todo);
 	if (ret < 0)
@@ -88,7 +95,19 @@ int edit_todo_list(unsigned flags)
 	strbuf_release(&buf);
 
 	transform_todos(flags | TODO_LIST_SHORTEN_IDS);
-	append_todo_help(1, 0);
+
+	strbuf_read_file(&buf, todo_file, 0);
+	append_todo_help(1, 0, &buf);
+
+	todo = fopen_or_warn(todo_file, "w");
+	if (!todo) {
+		strbuf_release(&buf);
+		return 1;
+	}
+
+	strbuf_write(&buf, todo);
+	fclose(todo);
+	strbuf_release(&buf);
 
 	if (launch_sequence_editor(todo_file, NULL, NULL))
 		return 1;
diff --git a/rebase-interactive.h b/rebase-interactive.h
index 155219e74..c0ba83be3 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -1,7 +1,12 @@
 #ifndef REBASE_INTERACTIVE_H
 #define REBASE_INTERACTIVE_H
 
-int append_todo_help(unsigned edit_todo, unsigned keep_empty);
+#include <cache.h>
+#include <strbuf.h>
+
+void append_todo_help(unsigned edit_todo, unsigned keep_empty,
+		      struct strbuf *buf);
+int append_todo_help_to_file(unsigned edit_todo, unsigned keep_empty);
 int edit_todo_list(unsigned flags);
 
 #endif
-- 
2.18.0


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

* [GSoC][PATCH v3 09/13] sequencer: change the way skip_unnecessary_picks() returns its result
  2018-07-10 12:15 ` [GSoC][PATCH v3 00/13] rebase -i: rewrite some parts " Alban Gruin
                     ` (7 preceding siblings ...)
  2018-07-10 12:15   ` [GSoC][PATCH v3 08/13] sequencer: refactor append_todo_help() to write its message to a buffer Alban Gruin
@ 2018-07-10 12:15   ` Alban Gruin
  2018-07-10 18:38     ` Junio C Hamano
  2018-07-10 18:52     ` Junio C Hamano
  2018-07-10 12:15   ` [GSoC][PATCH v3 10/13] rebase--interactive: rewrite complete_action() in C Alban Gruin
                     ` (3 subsequent siblings)
  12 siblings, 2 replies; 50+ messages in thread
From: Alban Gruin @ 2018-07-10 12:15 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, Alban Gruin

Instead of skip_unnecessary_picks() printing its result to stdout, it
returns it into a const char *, as the rewrite of complete_action()
(to come in the next commit) will need it.

rebase--helper then is modified to fit this change.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase--helper.c | 11 ++++++++---
 sequencer.c              |  4 ++--
 sequencer.h              |  2 +-
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index b9af96af7..d4cfe43e7 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -14,7 +14,8 @@ 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, verbose = 0;
-	int abbreviate_commands = 0, rebase_cousins = -1;
+	int abbreviate_commands = 0, rebase_cousins = -1, ret;
+	const char *oid = NULL;
 	enum {
 		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
 		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
@@ -91,8 +92,12 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!transform_todos(flags);
 	if (command == CHECK_TODO_LIST && argc == 1)
 		return !!check_todo_list();
-	if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
-		return !!skip_unnecessary_picks();
+	if (command == SKIP_UNNECESSARY_PICKS && argc == 1) {
+		ret = !!skip_unnecessary_picks(&oid);
+		if (!ret && oid)
+			puts(oid);
+		return ret;
+	}
 	if (command == REARRANGE_SQUASH && argc == 1)
 		return !!rearrange_squash();
 	if (command == ADD_EXEC && argc == 2)
diff --git a/sequencer.c b/sequencer.c
index 2b6ddc6cf..676ac1320 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4392,7 +4392,7 @@ static int rewrite_file(const char *path, const char *buf, size_t len)
 }
 
 /* skip picking commits whose parents are unchanged */
-int skip_unnecessary_picks(void)
+int skip_unnecessary_picks(const char **output_oid)
 {
 	const char *todo_file = rebase_path_todo();
 	struct strbuf buf = STRBUF_INIT;
@@ -4467,7 +4467,7 @@ int skip_unnecessary_picks(void)
 	}
 
 	todo_list_release(&todo_list);
-	printf("%s\n", oid_to_hex(oid));
+	*output_oid = oid_to_hex(oid);
 
 	return 0;
 }
diff --git a/sequencer.h b/sequencer.h
index 11a533461..25b50efe2 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -88,7 +88,7 @@ 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 skip_unnecessary_picks(const char **output_oid);
 int rearrange_squash(void);
 
 extern const char sign_off_header[];
-- 
2.18.0


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

* [GSoC][PATCH v3 10/13] rebase--interactive: rewrite complete_action() in C
  2018-07-10 12:15 ` [GSoC][PATCH v3 00/13] rebase -i: rewrite some parts " Alban Gruin
                     ` (8 preceding siblings ...)
  2018-07-10 12:15   ` [GSoC][PATCH v3 09/13] sequencer: change the way skip_unnecessary_picks() returns its result Alban Gruin
@ 2018-07-10 12:15   ` Alban Gruin
  2018-07-10 22:33     ` Junio C Hamano
  2018-07-10 12:15   ` [GSoC][PATCH v3 11/13] rebase--interactive: remove unused modes and functions Alban Gruin
                     ` (2 subsequent siblings)
  12 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2018-07-10 12:15 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, Alban Gruin

This rewrites complete_action() from shell to C.

A new mode is added to rebase--helper (`--complete-action`), as well as
a new flag (`--autosquash`).

Finally, complete_action() is stripped from git-rebase--interactive.sh.

The original complete_action() checked twice that the todo-list had at
least one operations (the first check took place after the help message
was inserted, the second after the user edited the todo-list).  If there
are no actions, complete_action() would return with the code 2.  This is
a special case for rebase -i and -p; git-rebase.sh would then apply the
autostash, delete the state directory, and die with the message "nothing
to do".  This rewrite remove the first check (because a noop is added to
the todo-list if it is empty).  For the second case, the cleanup is
rewritten in C instead of returning 2.

As rebase -i no longer returns 2, the comment describing this behaviour
in git-rebase.sh is updated to reflect this change.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase--helper.c   | 13 +++++-
 git-rebase--interactive.sh | 53 ++---------------------
 git-rebase.sh              |  2 +-
 sequencer.c                | 89 ++++++++++++++++++++++++++++++++++++++
 sequencer.h                |  4 ++
 5 files changed, 109 insertions(+), 52 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index d4cfe43e7..bb3698dba 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,14 +13,15 @@ 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, verbose = 0;
+	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0,
+		autosquash = 0;
 	int abbreviate_commands = 0, rebase_cousins = -1, ret;
 	const char *oid = NULL;
 	enum {
 		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
 		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
 		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH,
-		CHECKOUT_ONTO
+		CHECKOUT_ONTO, COMPLETE_ACTION
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -30,6 +31,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, "autosquash", &autosquash,
+			 N_("move commits thas begin with squash!/fixup!")),
 		OPT__VERBOSE(&verbose, N_("be verbose")),
 		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
 				CONTINUE),
@@ -58,6 +61,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 			    N_("prepare the branch to be rebased"), PREPARE_BRANCH),
 		OPT_CMDMODE(0, "checkout-onto", &command,
 			    N_("checkout a commit"), CHECKOUT_ONTO),
+		OPT_CMDMODE(0, "complete-action", &command,
+			    N_("complete the action"), COMPLETE_ACTION),
 		OPT_END()
 	};
 
@@ -110,5 +115,9 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!prepare_branch_to_be_rebased(&opts, argv[1]);
 	if (command == CHECKOUT_ONTO && argc == 4)
 		return !!checkout_onto(&opts, argv[1], argv[2], argv[3]);
+	if (command == COMPLETE_ACTION && argc == 6)
+		return !!complete_action(&opts, flags, argv[1], argv[2], argv[3],
+					 argv[4], argv[5], autosquash, keep_empty);
+
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index b68f108f2..59dc4888a 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -127,54 +127,6 @@ init_revisions_and_shortrevisions () {
 	fi
 }
 
-complete_action() {
-	test -s "$todo" || echo noop >> "$todo"
-	test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
-	test -n "$cmd" && git rebase--helper --add-exec-commands "$cmd"
-
-	todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
-	todocount=${todocount##* }
-
-cat >>"$todo" <<EOF
-
-$comment_char $(eval_ngettext \
-	"Rebase \$shortrevisions onto \$shortonto (\$todocount command)" \
-	"Rebase \$shortrevisions onto \$shortonto (\$todocount commands)" \
-	"$todocount")
-EOF
-	git rebase--helper --append-todo-help ${keep_empty:+--keep-empty}
-
-	has_action "$todo" ||
-		return 2
-
-	cp "$todo" "$todo".backup
-	collapse_todo_ids
-	git_sequence_editor "$todo" ||
-		die_abort "$(gettext "Could not execute editor")"
-
-	has_action "$todo" ||
-		return 2
-
-	git rebase--helper --check-todo-list || {
-		ret=$?
-		git rebase--helper --checkout-onto "$onto_name" "$onto" \
-		    "$orig_head" ${verbose:+--verbose}
-		exit $ret
-	}
-
-	expand_todo_ids
-
-	test -n "$force_rebase" ||
-	onto="$(git rebase--helper --skip-unnecessary-picks)" ||
-	die "Could not skip unnecessary pick commands"
-
-	git rebase--helper --checkout-onto "$onto_name" "$onto" "$orig_head" \
-	    ${verbose:+--verbose}
-	require_clean_work_tree "rebase"
-	exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
-	     --continue
-}
-
 git_rebase__interactive () {
 	initiate_action "$action"
 	ret=$?
@@ -193,5 +145,8 @@ git_rebase__interactive () {
 		$revisions ${restrict_revision+^$restrict_revision} >"$todo" ||
 	die "$(gettext "Could not generate todo list")"
 
-	complete_action
+	exec git rebase--helper --complete-action "$shortrevisions" "$onto_name" \
+		"$shortonto" "$orig_head" "$cmd" $allow_empty_message \
+		${autosquash:+--autosquash} ${keep_empty:+--keep-empty} \
+		${verbose:+--verbose} ${force_rebase:+--no-ff}
 }
diff --git a/git-rebase.sh b/git-rebase.sh
index 19bdebb48..de2a9f39f 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -219,7 +219,7 @@ run_specific_rebase () {
 	if test $ret -eq 0
 	then
 		finish_rebase
-	elif test $ret -eq 2 # special exit status for rebase -i
+	elif test $ret -eq 2 # special exit status for rebase -p
 	then
 		apply_autostash &&
 		rm -rf "$state_dir" &&
diff --git a/sequencer.c b/sequencer.c
index 676ac1320..da975285d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -29,6 +29,7 @@
 #include "oidset.h"
 #include "commit-slab.h"
 #include "alias.h"
+#include "rebase-interactive.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -52,6 +53,9 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge")
  * file and written to the tail of 'done'.
  */
 GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
+static GIT_PATH_FUNC(rebase_path_todo_backup,
+		     "rebase-merge/git-rebase-todo.backup")
+
 /*
  * The rebase command lines that have already been processed. A line
  * is moved here when it is first handled, before any associated user
@@ -4472,6 +4476,91 @@ int skip_unnecessary_picks(const char **output_oid)
 	return 0;
 }
 
+int complete_action(struct replay_opts *opts, unsigned flags,
+		    const char *shortrevisions, const char *onto_name,
+		    const char *onto, const char *orig_head, const char *cmd,
+		    unsigned autosquash, unsigned keep_empty)
+{
+	const char *shortonto, *todo_file = rebase_path_todo();
+	struct todo_list todo_list = TODO_LIST_INIT;
+	struct strbuf *buf = &(todo_list.buf);
+	struct object_id oid;
+	struct stat st;
+
+	get_oid(onto, &oid);
+	shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);
+
+	if (!lstat(todo_file, &st) && st.st_size == 0 &&
+	    write_message("noop\n", 5, todo_file, 0))
+		return error_errno(_("could not write '%s'"), todo_file);
+
+	if (autosquash && rearrange_squash())
+		return 0;
+
+	if (cmd && *cmd)
+		sequencer_add_exec_commands(cmd);
+
+	if (strbuf_read_file(buf, todo_file, 0) < 0)
+		return error_errno(_("could not read '%s'."), todo_file);
+
+	if (parse_insn_buffer(buf->buf, &todo_list)) {
+		todo_list_release(&todo_list);
+		return error(_("unusable todo list: '%s'"), todo_file);
+	}
+
+	strbuf_addch(buf, '\n');
+	strbuf_commented_addf(buf, Q_("Rebase %s onto %s (%d command)",
+				      "Rebase %s onto %s (%d commands)",
+				      todo_list.nr),
+			      shortrevisions, shortonto, todo_list.nr);
+	append_todo_help(0, keep_empty, buf);
+
+	if (write_message(buf->buf, buf->len, todo_file, 0)) {
+		todo_list_release(&todo_list);
+		return error_errno(_("could not write '%s'"), todo_file);
+	}
+
+	copy_file(rebase_path_todo_backup(), todo_file, 0666);
+	transform_todos(flags | TODO_LIST_SHORTEN_IDS);
+
+	strbuf_reset(buf);
+
+	if (launch_sequence_editor(todo_file, buf, NULL)) {
+		apply_autostash(opts);
+		sequencer_remove_state(opts);
+		todo_list_release(&todo_list);
+
+		return error(_("could not execute editor"));
+	}
+
+	strbuf_stripspace(buf, 1);
+	if (buf->len == 0) {
+		apply_autostash(opts);
+		sequencer_remove_state(opts);
+		todo_list_release(&todo_list);
+
+		return error(_("nothing to do"));
+	}
+
+	todo_list_release(&todo_list);
+
+	if (check_todo_list()) {
+		checkout_onto(opts, onto_name, onto, orig_head);
+		return 1;
+	}
+
+	transform_todos(flags & ~(TODO_LIST_SHORTEN_IDS));
+
+	if (opts->allow_ff && skip_unnecessary_picks(&onto))
+		return error(_("could not skip unnecessary pick commands"));
+
+	checkout_onto(opts, onto_name, onto, orig_head);
+	if (require_clean_work_tree("rebase", "", 1, 1))
+		return 1;
+
+	return sequencer_continue(opts);
+}
+
 struct subject2item_entry {
 	struct hashmap_entry entry;
 	int i;
diff --git a/sequencer.h b/sequencer.h
index 25b50efe2..02772b927 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -89,6 +89,10 @@ 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(const char **output_oid);
+int complete_action(struct replay_opts *opts, unsigned flags,
+		    const char *shortrevisions, const char *onto_name,
+		    const char *onto, const char *orig_head, const char *cmd,
+		    unsigned autosquash, unsigned keep_empty);
 int rearrange_squash(void);
 
 extern const char sign_off_header[];
-- 
2.18.0


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

* [GSoC][PATCH v3 11/13] rebase--interactive: remove unused modes and functions
  2018-07-10 12:15 ` [GSoC][PATCH v3 00/13] rebase -i: rewrite some parts " Alban Gruin
                     ` (9 preceding siblings ...)
  2018-07-10 12:15   ` [GSoC][PATCH v3 10/13] rebase--interactive: rewrite complete_action() in C Alban Gruin
@ 2018-07-10 12:15   ` Alban Gruin
  2018-07-10 22:36     ` Junio C Hamano
  2018-07-10 12:15   ` [GSoC][PATCH v3 12/13] rebase -i: implement the logic to initialize the variable $revision in C Alban Gruin
  2018-07-10 12:15   ` [GSoC][PATCH v3 13/13] rebase -i: rewrite the rest of init_revisions_and_shortrevisions " Alban Gruin
  12 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2018-07-10 12:15 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, Alban Gruin

This removes the modes `--skip-unnecessary-picks`, `--append-todo-help`,
`--checkout-onto`, `--shorten-ids` and `--expand-ids` from
rebase--helper.c, the functions of git-rebase--interactive.sh that were
rendered useless by the rewrite of complete_action(), and
append_todo_help_to_file() from rebase-interactive.c.

skip_unnecessary_picks() and checkout_onto() becomes static, as they are
only used inside of the sequencer.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase--helper.c   | 32 +++---------------------
 git-rebase--interactive.sh | 50 --------------------------------------
 rebase-interactive.c       | 22 -----------------
 rebase-interactive.h       |  1 -
 sequencer.c                |  8 +++---
 sequencer.h                |  4 ---
 6 files changed, 7 insertions(+), 110 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index bb3698dba..8ab808da4 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -15,13 +15,10 @@ 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, verbose = 0,
 		autosquash = 0;
-	int abbreviate_commands = 0, rebase_cousins = -1, ret;
-	const char *oid = NULL;
+	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, EDIT_TODO, PREPARE_BRANCH,
-		CHECKOUT_ONTO, COMPLETE_ACTION
+		CONTINUE = 1, ABORT, MAKE_SCRIPT, CHECK_TODO_LIST, REARRANGE_SQUASH,
+		ADD_EXEC, EDIT_TODO, PREPARE_BRANCH, COMPLETE_ACTION
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -40,27 +37,17 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 				ABORT),
 		OPT_CMDMODE(0, "make-script", &command,
 			N_("make rebase script"), MAKE_SCRIPT),
-		OPT_CMDMODE(0, "shorten-ids", &command,
-			N_("shorten commit ids in the todo list"), SHORTEN_OIDS),
-		OPT_CMDMODE(0, "expand-ids", &command,
-			N_("expand commit ids in the todo list"), EXPAND_OIDS),
 		OPT_CMDMODE(0, "check-todo-list", &command,
 			N_("check the todo list"), CHECK_TODO_LIST),
-		OPT_CMDMODE(0, "skip-unnecessary-picks", &command,
-			N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
 		OPT_CMDMODE(0, "rearrange-squash", &command,
 			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_CMDMODE(0, "edit-todo", &command,
 			    N_("edit the todo list during an interactive rebase"),
 			    EDIT_TODO),
 		OPT_CMDMODE(0, "prepare-branch", &command,
 			    N_("prepare the branch to be rebased"), PREPARE_BRANCH),
-		OPT_CMDMODE(0, "checkout-onto", &command,
-			    N_("checkout a commit"), CHECKOUT_ONTO),
 		OPT_CMDMODE(0, "complete-action", &command,
 			    N_("complete the action"), COMPLETE_ACTION),
 		OPT_END()
@@ -81,7 +68,6 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
 	flags |= rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
 	flags |= rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
-	flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
 
 	if (rebase_cousins >= 0 && !rebase_merges)
 		warning(_("--[no-]rebase-cousins has no effect without "
@@ -93,28 +79,16 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!sequencer_remove_state(&opts);
 	if (command == MAKE_SCRIPT && argc > 1)
 		return !!sequencer_make_script(stdout, argc, argv, flags);
-	if ((command == SHORTEN_OIDS || command == EXPAND_OIDS) && argc == 1)
-		return !!transform_todos(flags);
 	if (command == CHECK_TODO_LIST && argc == 1)
 		return !!check_todo_list();
-	if (command == SKIP_UNNECESSARY_PICKS && argc == 1) {
-		ret = !!skip_unnecessary_picks(&oid);
-		if (!ret && oid)
-			puts(oid);
-		return ret;
-	}
 	if (command == REARRANGE_SQUASH && argc == 1)
 		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_to_file(0, keep_empty);
 	if (command == EDIT_TODO && argc == 1)
 		return !!edit_todo_list(flags);
 	if (command == PREPARE_BRANCH && argc == 2)
 		return !!prepare_branch_to_be_rebased(&opts, argv[1]);
-	if (command == CHECKOUT_ONTO && argc == 4)
-		return !!checkout_onto(&opts, argv[1], argv[2], argv[3]);
 	if (command == COMPLETE_ACTION && argc == 6)
 		return !!complete_action(&opts, flags, argv[1], argv[2], argv[3],
 					 argv[4], argv[5], autosquash, keep_empty);
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 59dc4888a..0d66c0f8b 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -16,56 +16,6 @@ todo="$state_dir"/git-rebase-todo
 GIT_CHERRY_PICK_HELP="$resolvemsg"
 export GIT_CHERRY_PICK_HELP
 
-comment_char=$(git config --get core.commentchar 2>/dev/null)
-case "$comment_char" in
-'' | auto)
-	comment_char="#"
-	;;
-?)
-	;;
-*)
-	comment_char=$(echo "$comment_char" | cut -c1)
-	;;
-esac
-
-die_abort () {
-	apply_autostash
-	rm -rf "$state_dir"
-	die "$1"
-}
-
-has_action () {
-	test -n "$(git stripspace --strip-comments <"$1")"
-}
-
-git_sequence_editor () {
-	if test -z "$GIT_SEQUENCE_EDITOR"
-	then
-		GIT_SEQUENCE_EDITOR="$(git config sequence.editor)"
-		if [ -z "$GIT_SEQUENCE_EDITOR" ]
-		then
-			GIT_SEQUENCE_EDITOR="$(git var GIT_EDITOR)" || return $?
-		fi
-	fi
-
-	eval "$GIT_SEQUENCE_EDITOR" '"$@"'
-}
-
-expand_todo_ids() {
-	git rebase--helper --expand-ids
-}
-
-collapse_todo_ids() {
-	git rebase--helper --shorten-ids
-}
-
-get_missing_commit_check_level () {
-	check_level=$(git config --get rebase.missingCommitsCheck)
-	check_level=${check_level:-ignore}
-	# Don't be case sensitive
-	printf '%s' "$check_level" | tr 'A-Z' 'a-z'
-}
-
 # Initiate an action. If the cannot be any
 # further action it  may exec a command
 # or exit and not return.
diff --git a/rebase-interactive.c b/rebase-interactive.c
index d8b946559..f99e596d2 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -52,28 +52,6 @@ void append_todo_help(unsigned edit_todo, unsigned keep_empty,
 	}
 }
 
-int append_todo_help_to_file(unsigned edit_todo, unsigned keep_empty)
-{
-	struct strbuf buf = STRBUF_INIT;
-	FILE *todo;
-	int ret;
-
-	todo = fopen_or_warn(rebase_path_todo(), "a");
-	if (!todo)
-		return 1;
-
-	append_todo_help(edit_todo, keep_empty, &buf);
-
-	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;
-}
-
 int edit_todo_list(unsigned flags)
 {
 	struct strbuf buf = STRBUF_INIT;
diff --git a/rebase-interactive.h b/rebase-interactive.h
index c0ba83be3..47025c47a 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -6,7 +6,6 @@
 
 void append_todo_help(unsigned edit_todo, unsigned keep_empty,
 		      struct strbuf *buf);
-int append_todo_help_to_file(unsigned edit_todo, unsigned keep_empty);
 int edit_todo_list(unsigned flags);
 
 #endif
diff --git a/sequencer.c b/sequencer.c
index da975285d..6ceada4a9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3166,9 +3166,9 @@ int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit)
 	return 0;
 }
 
-int checkout_onto(struct replay_opts *opts,
-		  const char *onto_name, const char *onto,
-		  const char *orig_head)
+static int checkout_onto(struct replay_opts *opts,
+			 const char *onto_name, const char *onto,
+			 const char *orig_head)
 {
 	struct object_id oid;
 	const char *action = reflog_message(opts, "start", "checkout %s", onto_name);
@@ -4396,7 +4396,7 @@ static int rewrite_file(const char *path, const char *buf, size_t len)
 }
 
 /* skip picking commits whose parents are unchanged */
-int skip_unnecessary_picks(const char **output_oid)
+static int skip_unnecessary_picks(const char **output_oid)
 {
 	const char *todo_file = rebase_path_todo();
 	struct strbuf buf = STRBUF_INIT;
diff --git a/sequencer.h b/sequencer.h
index 02772b927..b9f9e7827 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -88,7 +88,6 @@ 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(const char **output_oid);
 int complete_action(struct replay_opts *opts, unsigned flags,
 		    const char *shortrevisions, const char *onto_name,
 		    const char *onto, const char *orig_head, const char *cmd,
@@ -111,9 +110,6 @@ void commit_post_rewrite(const struct commit *current_head,
 			 const struct object_id *new_head);
 
 int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit);
-int checkout_onto(struct replay_opts *opts,
-		  const char *onto_name, const char *onto,
-		  const char *orig_head);
 
 #define SUMMARY_INITIAL_COMMIT   (1 << 0)
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
-- 
2.18.0


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

* [GSoC][PATCH v3 12/13] rebase -i: implement the logic to initialize the variable $revision in C
  2018-07-10 12:15 ` [GSoC][PATCH v3 00/13] rebase -i: rewrite some parts " Alban Gruin
                     ` (10 preceding siblings ...)
  2018-07-10 12:15   ` [GSoC][PATCH v3 11/13] rebase--interactive: remove unused modes and functions Alban Gruin
@ 2018-07-10 12:15   ` Alban Gruin
  2018-07-12 18:15     ` Junio C Hamano
  2018-07-10 12:15   ` [GSoC][PATCH v3 13/13] rebase -i: rewrite the rest of init_revisions_and_shortrevisions " Alban Gruin
  12 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2018-07-10 12:15 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, Alban Gruin

This rewrites the part of init_revisions_and_shortrevisions() needed by
`--make-script` from shell to C.  The new version is called
get_revision_ranges(), and is a static function inside of
rebase--helper.c.

Unlike init_revisions_and_shortrevisions(), get_revision_ranges()
doesn’t write $squash_onto to the state directory, it’s done by the
handler of `--make-script` instead.

Finally, this drops the $revision argument passed to `--make-script` in
git-rebase--interactive.sh, and rebase--helper is changed accordingly.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase--helper.c   | 56 ++++++++++++++++++++++++++++++++++++--
 git-rebase--interactive.sh |  4 ++-
 2 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 8ab808da4..5845f80de 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -4,6 +4,25 @@
 #include "parse-options.h"
 #include "sequencer.h"
 #include "rebase-interactive.h"
+#include "argv-array.h"
+
+static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto")
+
+static int get_revision_ranges(const char *upstream, const char *onto,
+			       const char **head_hash,
+			       char **revisions)
+{
+	const char *base_rev = upstream ? upstream : onto;
+	struct object_id orig_head;
+
+	if (get_oid("HEAD", &orig_head))
+		return error(_("no HEAD?"));
+
+	*head_hash = find_unique_abbrev(&orig_head, GIT_MAX_HEXSZ);
+	*revisions = xstrfmt("%s...%s", base_rev, *head_hash);
+
+	return 0;
+}
 
 static const char * const builtin_rebase_helper_usage[] = {
 	N_("git rebase--helper [<options>]"),
@@ -15,7 +34,9 @@ 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, verbose = 0,
 		autosquash = 0;
-	int abbreviate_commands = 0, rebase_cousins = -1;
+	int abbreviate_commands = 0, rebase_cousins = -1, ret;
+	const char *head_hash = NULL, *onto = NULL, *restrict_revisions = NULL,
+		*squash_onto = NULL, *upstream = NULL;
 	enum {
 		CONTINUE = 1, ABORT, MAKE_SCRIPT, CHECK_TODO_LIST, REARRANGE_SQUASH,
 		ADD_EXEC, EDIT_TODO, PREPARE_BRANCH, COMPLETE_ACTION
@@ -50,6 +71,13 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 			    N_("prepare the branch to be rebased"), PREPARE_BRANCH),
 		OPT_CMDMODE(0, "complete-action", &command,
 			    N_("complete the action"), COMPLETE_ACTION),
+		OPT_STRING(0, "onto", &onto, N_("onto"), N_("onto")),
+		OPT_STRING(0, "restrict-revision", &restrict_revisions,
+			   N_("restrict-revision"), N_("restrict revision")),
+		OPT_STRING(0, "squash-onto", &squash_onto, N_("squash-onto"),
+			   N_("squash onto")),
+		OPT_STRING(0, "upstream", &upstream, N_("upstream"),
+			   N_("the upstream commit")),
 		OPT_END()
 	};
 
@@ -77,8 +105,30 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!sequencer_continue(&opts);
 	if (command == ABORT && argc == 1)
 		return !!sequencer_remove_state(&opts);
-	if (command == MAKE_SCRIPT && argc > 1)
-		return !!sequencer_make_script(stdout, argc, argv, flags);
+	if (command == MAKE_SCRIPT && (argc == 1 || argc == 2)) {
+		char *revisions = NULL;
+		struct argv_array make_script_args = ARGV_ARRAY_INIT;
+
+		if (!upstream && squash_onto)
+			write_file(path_squash_onto(), "%s\n", squash_onto);
+
+		ret = get_revision_ranges(upstream, onto, &head_hash, &revisions);
+		if (ret)
+			return ret;
+
+		argv_array_pushl(&make_script_args, "", revisions, NULL);
+		if (argc == 2)
+			argv_array_push(&make_script_args, restrict_revisions);
+
+		ret = !!sequencer_make_script(stdout,
+					      make_script_args.argc, make_script_args.argv,
+					      flags);
+
+		free(revisions);
+		argv_array_clear(&make_script_args);
+
+		return ret;
+	}
 	if (command == CHECK_TODO_LIST && argc == 1)
 		return !!check_todo_list();
 	if (command == REARRANGE_SQUASH && argc == 1)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0d66c0f8b..4ca47aed1 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -92,7 +92,9 @@ git_rebase__interactive () {
 	git rebase--helper --make-script ${keep_empty:+--keep-empty} \
 		${rebase_merges:+--rebase-merges} \
 		${rebase_cousins:+--rebase-cousins} \
-		$revisions ${restrict_revision+^$restrict_revision} >"$todo" ||
+		${upstream:+--upstream "$upstream"} ${onto:+--onto "$onto"} \
+		${squash_onto:+--squash-onto "$squash_onto"} \
+		${restrict_revision:+--restrict-revision ^"$restrict_revision"} >"$todo" ||
 	die "$(gettext "Could not generate todo list")"
 
 	exec git rebase--helper --complete-action "$shortrevisions" "$onto_name" \
-- 
2.18.0


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

* [GSoC][PATCH v3 13/13] rebase -i: rewrite the rest of init_revisions_and_shortrevisions in C
  2018-07-10 12:15 ` [GSoC][PATCH v3 00/13] rebase -i: rewrite some parts " Alban Gruin
                     ` (11 preceding siblings ...)
  2018-07-10 12:15   ` [GSoC][PATCH v3 12/13] rebase -i: implement the logic to initialize the variable $revision in C Alban Gruin
@ 2018-07-10 12:15   ` " Alban Gruin
  12 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2018-07-10 12:15 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster, Alban Gruin

This rewrites the part of init_revisions_and_shortrevisions() needed by
`--complete-action` (which initialize $shortrevisions) from shell to C.

When `upstream` is empty, it means that the user launched a `rebase
--root`, and `onto` contains the ID of an empty commit.  As a range
between an empty commit and `head` is not really meaningful, `onto` is
not used to initialize `shortrevisions` in this case.

The corresponding arguments passed to `--complete-action` are then
dropped, and init_revisions_and_shortrevisions is stripped from
git-rebase--interactive.sh

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase--helper.c   | 40 ++++++++++++++++++++++++++++++++------
 git-rebase--interactive.sh | 27 ++++---------------------
 2 files changed, 38 insertions(+), 29 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 5845f80de..59e788f22 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -10,7 +10,7 @@ static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto")
 
 static int get_revision_ranges(const char *upstream, const char *onto,
 			       const char **head_hash,
-			       char **revisions)
+			       char **revisions, char **shortrevisions)
 {
 	const char *base_rev = upstream ? upstream : onto;
 	struct object_id orig_head;
@@ -19,7 +19,25 @@ static int get_revision_ranges(const char *upstream, const char *onto,
 		return error(_("no HEAD?"));
 
 	*head_hash = find_unique_abbrev(&orig_head, GIT_MAX_HEXSZ);
-	*revisions = xstrfmt("%s...%s", base_rev, *head_hash);
+
+	if (revisions)
+		*revisions = xstrfmt("%s...%s", base_rev, *head_hash);
+	if (shortrevisions) {
+		const char *shorthead;
+
+		shorthead = find_unique_abbrev(&orig_head, DEFAULT_ABBREV);
+
+		if (upstream) {
+			const char *shortrev;
+			struct object_id rev_oid;
+
+			get_oid(base_rev, &rev_oid);
+			shortrev = find_unique_abbrev(&rev_oid, DEFAULT_ABBREV);
+
+			*shortrevisions = xstrfmt("%s..%s", shortrev, shorthead);
+		} else
+			*shortrevisions = xstrdup(shorthead);
+	}
 
 	return 0;
 }
@@ -112,7 +130,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		if (!upstream && squash_onto)
 			write_file(path_squash_onto(), "%s\n", squash_onto);
 
-		ret = get_revision_ranges(upstream, onto, &head_hash, &revisions);
+		ret = get_revision_ranges(upstream, onto, &head_hash, &revisions, NULL);
 		if (ret)
 			return ret;
 
@@ -139,9 +157,19 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!edit_todo_list(flags);
 	if (command == PREPARE_BRANCH && argc == 2)
 		return !!prepare_branch_to_be_rebased(&opts, argv[1]);
-	if (command == COMPLETE_ACTION && argc == 6)
-		return !!complete_action(&opts, flags, argv[1], argv[2], argv[3],
-					 argv[4], argv[5], autosquash, keep_empty);
+	if (command == COMPLETE_ACTION && argc == 3) {
+		char *shortrevisions = NULL;
+
+		ret = get_revision_ranges(upstream, onto, &head_hash, NULL, &shortrevisions);
+		if (ret)
+			return ret;
+
+		ret = !!complete_action(&opts, flags, shortrevisions, argv[1], onto,
+					head_hash, argv[2], autosquash, keep_empty);
+
+		free(shortrevisions);
+		return ret;
+	}
 
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 4ca47aed1..08e9a21c2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -60,23 +60,6 @@ init_basic_state () {
 	write_basic_state
 }
 
-init_revisions_and_shortrevisions () {
-	shorthead=$(git rev-parse --short $orig_head)
-	shortonto=$(git rev-parse --short $onto)
-	if test -z "$rebase_root"
-		# this is now equivalent to ! -z "$upstream"
-	then
-		shortupstream=$(git rev-parse --short $upstream)
-		revisions=$upstream...$orig_head
-		shortrevisions=$shortupstream..$shorthead
-	else
-		revisions=$onto...$orig_head
-		shortrevisions=$shorthead
-		test -z "$squash_onto" ||
-		echo "$squash_onto" >"$state_dir"/squash-onto
-	fi
-}
-
 git_rebase__interactive () {
 	initiate_action "$action"
 	ret=$?
@@ -87,8 +70,6 @@ git_rebase__interactive () {
 	git rebase--helper --prepare-branch "$switch_to" ${verbose:+--verbose}
 	init_basic_state
 
-	init_revisions_and_shortrevisions
-
 	git rebase--helper --make-script ${keep_empty:+--keep-empty} \
 		${rebase_merges:+--rebase-merges} \
 		${rebase_cousins:+--rebase-cousins} \
@@ -97,8 +78,8 @@ git_rebase__interactive () {
 		${restrict_revision:+--restrict-revision ^"$restrict_revision"} >"$todo" ||
 	die "$(gettext "Could not generate todo list")"
 
-	exec git rebase--helper --complete-action "$shortrevisions" "$onto_name" \
-		"$shortonto" "$orig_head" "$cmd" $allow_empty_message \
-		${autosquash:+--autosquash} ${keep_empty:+--keep-empty} \
-		${verbose:+--verbose} ${force_rebase:+--no-ff}
+	exec git rebase--helper --complete-action "$onto_name" "$cmd" \
+		$allow_empty_message ${autosquash:+--autosquash} ${verbose:+--verbose} \
+		${keep_empty:+--keep-empty} ${force_rebase:+--no-ff} \
+		${upstream:+--upstream "$upstream"} ${onto:+--onto "$onto"}
 }
-- 
2.18.0


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

* Re: [GSoC][PATCH v3 02/13] rebase--interactive: rewrite append_todo_help() in C
  2018-07-10 12:15   ` [GSoC][PATCH v3 02/13] rebase--interactive: rewrite append_todo_help() in C Alban Gruin
@ 2018-07-10 12:21     ` Alban Gruin
  2018-07-10 17:56     ` Junio C Hamano
  1 sibling, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2018-07-10 12:21 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster

Le 10/07/2018 à 14:15, Alban Gruin a écrit :
> 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>
> ---
> Unchanged from what have been queued on `pu` (ag/rebase-i-in-c), and
> from v2.

Sorry, this has actually changed since v2.


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

* Re: [GSoC][PATCH v3 03/13] editor: add a function to launch the sequence editor
  2018-07-10 12:15   ` [GSoC][PATCH v3 03/13] editor: add a function to launch the sequence editor Alban Gruin
@ 2018-07-10 12:23     ` Alban Gruin
  2018-07-10 17:58     ` Junio C Hamano
  1 sibling, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2018-07-10 12:23 UTC (permalink / raw)
  To: git
  Cc: Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood, gitster

Le 10/07/2018 à 14:15, Alban Gruin a écrit :
> 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>
> ---

And this has not changed from what have been queued in pu
(ag/rebase-i-in-c), and from v2.


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

* Re: [GSoC][PATCH v3 01/13] sequencer: make two functions and an enum from sequencer.c public
  2018-07-10 12:15   ` [GSoC][PATCH v3 01/13] sequencer: make two functions and an enum from sequencer.c public Alban Gruin
@ 2018-07-10 17:53     ` Junio C Hamano
  0 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2018-07-10 17:53 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

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

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

It is not too big a deal, but we prefer to hear our story told in a
consistent voice.  We'd phrase often the above as if giving an order
to the code base to "become like so", i.e.

	Make X, Y, and Z accessible outside sequencer.c; rename A to
	B and prefix its values by doing C to avoid namespace
	pollusion.

> This function and this enum will eventually be moved to
> rebase-interactive.c and become static again, so no special attention
> was given to the naming.
>
> 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>
> ---
> Unchanged from v2.

You added an excuse for being sloppy in the naming to the log
message; the log message is an important and integral part of the
patch, so please do not label an update like this as unchanged.

"eventually" is fine, but that means we cannot split the early parts
of the topic, once they are polished well enough, into a separate
bit and have that graduate earlier than the remainder of the topic
until that "moved and become static again" happens, which may be a
bit unfortunate.  Let's make sure we can get to that "eventually"
step soon.



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

* Re: [GSoC][PATCH v3 02/13] rebase--interactive: rewrite append_todo_help() in C
  2018-07-10 12:15   ` [GSoC][PATCH v3 02/13] rebase--interactive: rewrite append_todo_help() in C Alban Gruin
  2018-07-10 12:21     ` Alban Gruin
@ 2018-07-10 17:56     ` Junio C Hamano
  1 sibling, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2018-07-10 17:56 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

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

> 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>
> ---
> Unchanged from what have been queued on `pu` (ag/rebase-i-in-c), and
> from v2.

With something like '...except downcasing an error message "Could
not append..."', a comment like this is very much appreciated as it
helps readers who have read the previous round and remember what was
in there ;-)

> + ...
> +	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));

Nice use of strbuf_add_comment_lines() function ;-).


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

* Re: [GSoC][PATCH v3 03/13] editor: add a function to launch the sequence editor
  2018-07-10 12:15   ` [GSoC][PATCH v3 03/13] editor: add a function to launch the sequence editor Alban Gruin
  2018-07-10 12:23     ` Alban Gruin
@ 2018-07-10 17:58     ` Junio C Hamano
  1 sibling, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2018-07-10 17:58 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

For those reading from sidelines, this is unchanged from the
previous round, and looking good.

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

> +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");

Nice code reuse.



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

* Re: [GSoC][PATCH v3 04/13] rebase-interactive: rewrite the edit-todo functionality in C
  2018-07-10 12:15   ` [GSoC][PATCH v3 04/13] rebase-interactive: rewrite the edit-todo functionality in C Alban Gruin
@ 2018-07-10 18:00     ` Junio C Hamano
  0 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2018-07-10 18:00 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

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

> 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>
> ---
> Unchanged from v2.

A quite straight-forward rewrite.  Looks sensible.

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

* Re: [GSoC][PATCH v3 05/13] sequencer: add a new function to silence a command, except if it fails
  2018-07-10 12:15   ` [GSoC][PATCH v3 05/13] sequencer: add a new function to silence a command, except if it fails Alban Gruin
@ 2018-07-10 18:13     ` Junio C Hamano
  0 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2018-07-10 18:13 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

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

> This adds a new function, run_command_silent_on_success(), to
> redirect the stdout and stderr of a command to a strbuf, and then to run
> that command. This strbuf is printed only if the command fails. It is
> functionnaly similar to output() from git-rebase.sh.
>
> run_git_commit() is then refactored to use of
> run_command_silent_on_success().
>
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  sequencer.c | 51 +++++++++++++++++++++++++--------------------------
>  1 file changed, 25 insertions(+), 26 deletions(-)

Looks good to me; it seems to cover the glitches pointed out in the
previous round well.

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

* Re: [GSoC][PATCH v3 06/13] rebase -i: rewrite setup_reflog_action() in C
  2018-07-10 12:15   ` [GSoC][PATCH v3 06/13] rebase -i: rewrite setup_reflog_action() in C Alban Gruin
@ 2018-07-10 18:20     ` Junio C Hamano
  0 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2018-07-10 18:20 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

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

> This rewrites (the misnamed) setup_reflog_action() from shell to C. The
> new version is called prepare_branch_to_be_rebased().
>
> A new command is added to rebase--helper.c, “checkout-base”, as well as
> a new flag, “verbose”, to avoid silencing the output of the checkout
> operation called by checkout_base_commit().
>
> The function `run_git_checkout()` will also be used in the next commit,
> therefore its code is not part of `checkout_base_commit()`.
>
> The shell version is then stripped in favour of a call to the helper.
>
> As $GIT_REFLOG_ACTION is no longer set at the first call of
> checkout_onto(), a call to comment_for_reflog() is added at the
> beginning of this function.
>
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  builtin/rebase--helper.c   | 10 ++++++++--
>  git-rebase--interactive.sh | 16 ++--------------
>  sequencer.c                | 30 ++++++++++++++++++++++++++++++
>  sequencer.h                |  2 ++
>  4 files changed, 42 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index 731a64971..76bdc6fdb 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;
> +	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;
...
>  	struct option options[] = {
>  		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
> @@ -28,6 +28,7 @@ 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__VERBOSE(&verbose, N_("be verbose")),
...
> @@ -60,6 +63,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>  	opts.action = REPLAY_INTERACTIVE_REBASE;
>  	opts.allow_ff = 1;
>  	opts.allow_empty = 1;
> +	opts.verbose = verbose;
>  
>  	argc = parse_options(argc, argv, NULL, options,
>  			builtin_rebase_helper_usage, PARSE_OPT_KEEP_ARGV0);
> @@ -94,5 +98,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>  		return !!append_todo_help(0, keep_empty);
>  	if (command == EDIT_TODO && argc == 1)
>  		return !!edit_todo_list(flags);
> +	if (command == PREPARE_BRANCH && argc == 2)
> +		return !!prepare_branch_to_be_rebased(&opts, argv[1]);

Not passing verbose as a separate parameter, and using opts.verbose
that already exists instead, is a sensible improvement from the
previous round.  I wonder if we need a new local variable, though?
Just like &opts.allow_ff is used directly in options[] array, can't
we give &opts.verbose to OPT__VERBOSE(), or would we break something
if we did so?



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

* Re: [GSoC][PATCH v3 07/13] rebase -i: rewrite checkout_onto() in C
  2018-07-10 12:15   ` [GSoC][PATCH v3 07/13] rebase -i: rewrite checkout_onto() " Alban Gruin
@ 2018-07-10 18:22     ` Junio C Hamano
  0 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2018-07-10 18:22 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

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

> This rewrites checkout_onto() from shell to C.
>
> A new command (“checkout-onto”) is added to rebase--helper.c. The shell
> version is then stripped.
>
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---

The only change relative to the previous round seems to be that this
round does not pass verbose flag as a separate parameter, but reuses
the opts.verbose field where "struct replay_opts" is being passed
around.

Looks OK from a quick scan so far.

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

* Re: [GSoC][PATCH v3 08/13] sequencer: refactor append_todo_help() to write its message to a buffer
  2018-07-10 12:15   ` [GSoC][PATCH v3 08/13] sequencer: refactor append_todo_help() to write its message to a buffer Alban Gruin
@ 2018-07-10 18:30     ` Junio C Hamano
  0 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2018-07-10 18:30 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

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

> diff --git a/rebase-interactive.h b/rebase-interactive.h
> index 155219e74..c0ba83be3 100644
> --- a/rebase-interactive.h
> +++ b/rebase-interactive.h
> @@ -1,7 +1,12 @@
>  #ifndef REBASE_INTERACTIVE_H
>  #define REBASE_INTERACTIVE_H
>  
> -int append_todo_help(unsigned edit_todo, unsigned keep_empty);
> +#include <cache.h>
> +#include <strbuf.h>

Even though arguments can be made that using angle-bracket to refer
to our own headers like the above is more kosher, such a clean-up is
not part of the GSoC topic, so let's refrain from doing a little
crusade like this and imitate the way the other existing files
include things.

Actually, unless there is a definition of the structure that you
require in your header file to define your own structure, we are
better off not including another header from your own header.

Whoever includes "rebase-interactive.h", if it needs to know what
strbuf looks like in order to call append_todo_help(), should
include "strbuf.h" itself.  Those who only need to call
append_todo_help_to_file or edit_todo_list hence do not need to know
about strbuf, if such a caller existed, should not be forced to
include "strbuf.h".

> +
> +void append_todo_help(unsigned edit_todo, unsigned keep_empty,
> +		      struct strbuf *buf);
> +int append_todo_help_to_file(unsigned edit_todo, unsigned keep_empty);
>  int edit_todo_list(unsigned flags);
>  
>  #endif

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

* Re: [GSoC][PATCH v3 09/13] sequencer: change the way skip_unnecessary_picks() returns its result
  2018-07-10 12:15   ` [GSoC][PATCH v3 09/13] sequencer: change the way skip_unnecessary_picks() returns its result Alban Gruin
@ 2018-07-10 18:38     ` Junio C Hamano
  2018-07-10 18:52     ` Junio C Hamano
  1 sibling, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2018-07-10 18:38 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

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

> @@ -4467,7 +4467,7 @@ int skip_unnecessary_picks(void)
>  	}
>  
>  	todo_list_release(&todo_list);
> -	printf("%s\n", oid_to_hex(oid));
> +	*output_oid = oid_to_hex(oid);

The return value from oid_to_hex() is volatile and does not survive
across multiple calls to it.  If this interface is meant to be long
lived (as opposed to an intermediate step during the conversion that
will soon disappear), it probably makes more sense to have the
caller supply an output buffer and call oid_to_hex_r() into it, or
something like that.

> diff --git a/sequencer.h b/sequencer.h
> index 11a533461..25b50efe2 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -88,7 +88,7 @@ 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 skip_unnecessary_picks(const char **output_oid);
>  int rearrange_squash(void);
>  
>  extern const char sign_off_header[];

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

* Re: [GSoC][PATCH v3 09/13] sequencer: change the way skip_unnecessary_picks() returns its result
  2018-07-10 12:15   ` [GSoC][PATCH v3 09/13] sequencer: change the way skip_unnecessary_picks() returns its result Alban Gruin
  2018-07-10 18:38     ` Junio C Hamano
@ 2018-07-10 18:52     ` Junio C Hamano
  1 sibling, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2018-07-10 18:52 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

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

> -	if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
> -		return !!skip_unnecessary_picks();
> +	if (command == SKIP_UNNECESSARY_PICKS && argc == 1) {
> +		ret = !!skip_unnecessary_picks(&oid);
> +		if (!ret && oid)
> +			puts(oid);

Think why you wrote !! there; in other words, avoid getting into a
habit of blindly copying and pasting existing code without thinking.

The original is synthesizing an integer value that is to be used as
the final exit status from a process, which has to be a non negative
integer, out of a helper function, whose error return may be
nagative [*1*], and it knows that the caller cares only about one
bit "did we or did we not have an error?", so it turns any non-zero
return value into 1.  That is why it uses !!.

	Side note *1* The normal convention for helper functions is
        to signal an error with negative return value, and success
        with 0.  There may be different negative values used to
        signal different error conditions to the caller, depending
        on how well the API is designed.

But realize that !! is an operation that loses information, so in
general you would want to delay its application til when you need
it.  In this particular case, you do not care what kind of error
you got (or you didn't), so you can just say

	ret = skip_unnecessary_picks(&oid);
	if (!ret && oid)
		puts(oid); /* happy */

without applying !! before receiving the return value from the
helper in "ret".

> +		return ret;

And ensure that you return only 0 or 1 by applying !! here instead.

> diff --git a/sequencer.c b/sequencer.c
> index 2b6ddc6cf..676ac1320 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4392,7 +4392,7 @@ static int rewrite_file(const char *path, const char *buf, size_t len)
>  }
>  
>  /* skip picking commits whose parents are unchanged */
> -int skip_unnecessary_picks(void)
> +int skip_unnecessary_picks(const char **output_oid)
>  {
>  	const char *todo_file = rebase_path_todo();
>  	struct strbuf buf = STRBUF_INIT;
> @@ -4467,7 +4467,7 @@ int skip_unnecessary_picks(void)
>  	}
>  
>  	todo_list_release(&todo_list);
> -	printf("%s\n", oid_to_hex(oid));
> +	*output_oid = oid_to_hex(oid);

The return value of oid_to_hex() is not meant to be long-lived.
Consider writing into caller supplied buffer with oid_to_hex_r()
or something like that.

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

* Re: [GSoC][PATCH v3 10/13] rebase--interactive: rewrite complete_action() in C
  2018-07-10 12:15   ` [GSoC][PATCH v3 10/13] rebase--interactive: rewrite complete_action() in C Alban Gruin
@ 2018-07-10 22:33     ` Junio C Hamano
  2018-07-11 14:25       ` Alban Gruin
  0 siblings, 1 reply; 50+ messages in thread
From: Junio C Hamano @ 2018-07-10 22:33 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

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

> This rewrites complete_action() from shell to C.
>
> A new mode is added to rebase--helper (`--complete-action`), as well as
> a new flag (`--autosquash`).
>
> Finally, complete_action() is stripped from git-rebase--interactive.sh.
>
> The original complete_action() checked twice that the todo-list had at
> least one operations (the first check took place after the help message
> was inserted, the second after the user edited the todo-list).  If there
> are no actions, complete_action() would return with the code 2.  This is
> a special case for rebase -i and -p; git-rebase.sh would then apply the
> autostash, delete the state directory, and die with the message "nothing
> to do".  This rewrite remove the first check (because a noop is added to
> the todo-list if it is empty).  For the second case, the cleanup is
> rewritten in C instead of returning 2.

The description about the first check seems unclear here.  Is it
"there was a first check but because the list is never empty due to
a noop, the check was unneeded"?  or is it "I decided to add noop to
force the list to be never empty, and that made the first check
unneeded"?  I'd imagine that the end-user experience would become
different if it were the latter (we'd see a todo whose sole entry is
no-op in the editor, instead of getting an error before editor
opens), and such a change would require a separate justification.

> -complete_action() {
> -	test -s "$todo" || echo noop >> "$todo"
> -...
> -
> -	has_action "$todo" ||
> -		return 2

It seems that the answer to my question turns out to be #1 after
some digging, i.e. the original, when it starteed adding noop at
ff74126c ("rebase -i: do not fail when there is no commit to
cherry-pick", 2008-10-10), should have noticed that the first
"Nothing to do" check has now become ineffective and removed it, and
you are fixing this age-old bug while rewriting it to C.

But you shouldn't force reviewers to figure that out.

	Side note.  This age-old omission is especially interesting,
	as the primary point of that change, which can be read from
	its title, was to prevent the command from dying that way.
	I wonder why reviewers did not notice it back then.

        ... ah, it seems that it was done during my absense ;-)

> -	complete_action
> +	exec git rebase--helper --complete-action "$shortrevisions" "$onto_name" \
> +		"$shortonto" "$orig_head" "$cmd" $allow_empty_message \
> +		${autosquash:+--autosquash} ${keep_empty:+--keep-empty} \
> +		${verbose:+--verbose} ${force_rebase:+--no-ff}

The $allow_empty_message and later options are all dashed ones.
git-rebase.sh gives us either empty or --allow-empty-message in the
variable for $allow_empty_message, and if you follow suit to prepare
all the other variables that way, the ugly ${frotz+=--frotz} dance
will all become unnecessary here.

> +int complete_action(struct replay_opts *opts, unsigned flags,
> +		    const char *shortrevisions, const char *onto_name,
> +		    const char *onto, const char *orig_head, const char *cmd,
> +		    unsigned autosquash, unsigned keep_empty)
> +{
> +	const char *shortonto, *todo_file = rebase_path_todo();
> +	struct todo_list todo_list = TODO_LIST_INIT;
> +	struct strbuf *buf = &(todo_list.buf);
> +	struct object_id oid;
> +	struct stat st;
> +
> +	get_oid(onto, &oid);
> +	shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);
> +
> +	if (!lstat(todo_file, &st) && st.st_size == 0 &&
> +	    write_message("noop\n", 5, todo_file, 0))
> +		return error_errno(_("could not write '%s'"), todo_file);

Wait a minute... thinking back to the early "age-old ommission"
topic, can the todo file be a non-empty file that does not have any
command (e.g. just a single blank line, or full of comments and
nothing else)?  The original wouldn't have added "noop" and then the
first "do we have anything to do?" check would still have been
necessary, which would mean that ff74126c's not removing the first
check was actually not a bug but was a cautious and sensible thing
to do, and removal of that exact check by this commit becomes a
regression.  So,... can the todo file be a non-empty file that does
not have any command in it at this point?

> +	if (autosquash && rearrange_squash())
> +		return 0;

The original, when rearrange-squash failed, exited with failure,
like so:

-       test -z "$autosquash" || git rebase--helper --rearrange-squash || exit

Now this function returns success when rearrange_squash fails.  
Is that intended?

> +	if (cmd && *cmd)

Shouldn't it be a BUG (programming error) if cmd == NULL?  I thought
the caller of complete_action() in this patch insisted that it got
argc == 6 or something, so *cmd might be NUL but cmd would never be
NULL if I understand your code correctly.  IOW, shouldn't the line
be more like:

	if (!cmd)
		BUG("...");
	if (*cmd)

?

> +		sequencer_add_exec_commands(cmd);

The sequencer_add_exec_commands() function does not take the name of
todo file as a parameter, so even though we have one in todo_file
variable we cannot use it to call the function, which is a bit sad,
but is OK.

> +	if (strbuf_read_file(buf, todo_file, 0) < 0)
> +		return error_errno(_("could not read '%s'."), todo_file);
> +	if (parse_insn_buffer(buf->buf, &todo_list)) {

Nice that we have parse* function.  We do not have to work with
stripspace plus "wc -l" ;-).

> +		todo_list_release(&todo_list);
> +		return error(_("unusable todo list: '%s'"), todo_file);

When the users see this error message, is it easy for them to
diagnose what went wrong (e.g. has parse_insn_buffer() already
issued some message to help pinpoint which insn in the todo list is
misspelt, for example)?

> +	}
> +
> +	strbuf_addch(buf, '\n');
> +	strbuf_commented_addf(buf, Q_("Rebase %s onto %s (%d command)",
> +				      "Rebase %s onto %s (%d commands)",
> +				      todo_list.nr),
> +			      shortrevisions, shortonto, todo_list.nr);
> +	append_todo_help(0, keep_empty, buf);
> +
> +	if (write_message(buf->buf, buf->len, todo_file, 0)) {
> +		todo_list_release(&todo_list);
> +		return error_errno(_("could not write '%s'"), todo_file);
> +	}
> +	copy_file(rebase_path_todo_backup(), todo_file, 0666);
> +	transform_todos(flags | TODO_LIST_SHORTEN_IDS);
> +

It is a bit sad that we are mostly working on the byte array
buf->buf (or external file touched by various helper functions we
call), even though we have a perfectly usable parsed representation
in todo_list structure in all of the above and the rest of this
function.

It might be better to grab todo_list.nr into a local simple integer
variable immediately after parse_insn_buffer() returns and then call
todo_list_release() on todo_list, as the parsed todo-list is only
used for two purposes (i.e. detecting format error by seeing if
parser returns success, and seeing how many insns are on the todo
list by checking todo_list.nr field) and nothing else.  By releasing
the otherwise unused todo_list early, you do not have to sprinkle
various error return codepaths with calls to todo_list_release().

That incidentally would manage too high expectation from readers of
the code as well ;-).

>  struct subject2item_entry {
>  	struct hashmap_entry entry;
>  	int i;
> diff --git a/sequencer.h b/sequencer.h
> index 25b50efe2..02772b927 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -89,6 +89,10 @@ 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(const char **output_oid);
> +int complete_action(struct replay_opts *opts, unsigned flags,
> +		    const char *shortrevisions, const char *onto_name,
> +		    const char *onto, const char *orig_head, const char *cmd,
> +		    unsigned autosquash, unsigned keep_empty);
>  int rearrange_squash(void);
>  
>  extern const char sign_off_header[];

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

* Re: [GSoC][PATCH v3 11/13] rebase--interactive: remove unused modes and functions
  2018-07-10 12:15   ` [GSoC][PATCH v3 11/13] rebase--interactive: remove unused modes and functions Alban Gruin
@ 2018-07-10 22:36     ` Junio C Hamano
  0 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2018-07-10 22:36 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

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

> This removes the modes `--skip-unnecessary-picks`, `--append-todo-help`,
> `--checkout-onto`, `--shorten-ids` and `--expand-ids` from
> rebase--helper.c, the functions of git-rebase--interactive.sh that were
> rendered useless by the rewrite of complete_action(), and
> append_todo_help_to_file() from rebase-interactive.c.

Good.  So far up to this step, I thnk the organization of the series
looks good, even if the execution in each individual step we have
seen previously may still need to be improved.


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

* Re: [GSoC][PATCH v3 10/13] rebase--interactive: rewrite complete_action() in C
  2018-07-10 22:33     ` Junio C Hamano
@ 2018-07-11 14:25       ` Alban Gruin
  0 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2018-07-11 14:25 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

Hi Junio,

Le 11/07/2018 à 00:33, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
>> -	complete_action
>> +	exec git rebase--helper --complete-action "$shortrevisions" "$onto_name" \
>> +		"$shortonto" "$orig_head" "$cmd" $allow_empty_message \
>> +		${autosquash:+--autosquash} ${keep_empty:+--keep-empty} \
>> +		${verbose:+--verbose} ${force_rebase:+--no-ff}
> 
> The $allow_empty_message and later options are all dashed ones.
> git-rebase.sh gives us either empty or --allow-empty-message in the
> variable for $allow_empty_message, and if you follow suit to prepare
> all the other variables that way, the ugly ${frotz+=--frotz} dance
> will all become unnecessary here.
> 

Good idea.

>> +int complete_action(struct replay_opts *opts, unsigned flags,
>> +		    const char *shortrevisions, const char *onto_name,
>> +		    const char *onto, const char *orig_head, const char *cmd,
>> +		    unsigned autosquash, unsigned keep_empty)
>> +{
>> +	const char *shortonto, *todo_file = rebase_path_todo();
>> +	struct todo_list todo_list = TODO_LIST_INIT;
>> +	struct strbuf *buf = &(todo_list.buf);
>> +	struct object_id oid;
>> +	struct stat st;
>> +
>> +	get_oid(onto, &oid);
>> +	shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);
>> +
>> +	if (!lstat(todo_file, &st) && st.st_size == 0 &&
>> +	    write_message("noop\n", 5, todo_file, 0))
>> +		return error_errno(_("could not write '%s'"), todo_file);
> 
> Wait a minute... thinking back to the early "age-old ommission"
> topic, can the todo file be a non-empty file that does not have any
> command (e.g. just a single blank line, or full of comments and
> nothing else)?  The original wouldn't have added "noop" and then the
> first "do we have anything to do?" check would still have been
> necessary, which would mean that ff74126c's not removing the first
> check was actually not a bug but was a cautious and sensible thing
> to do, and removal of that exact check by this commit becomes a
> regression.  So,... can the todo file be a non-empty file that does
> not have any command in it at this point?
> 

Hum, yes, if the commits are empty, and if rebase was invoked without
the `--keep-empty` flag.  In this case, it would die with the message
“Nothing to do”, instead of dropping the commit altogether.

I will add a test case in the next iteration.

>> +	if (autosquash && rearrange_squash())
>> +		return 0;
> 
> The original, when rearrange-squash failed, exited with failure,
> like so:
> 
> -       test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
> 
> Now this function returns success when rearrange_squash fails.  
> Is that intended?
> 

Yes, it is.  I just saw in the man page that `exit` returns the last
exit status.

>> +	if (cmd && *cmd)
> 
> Shouldn't it be a BUG (programming error) if cmd == NULL?  I thought
> the caller of complete_action() in this patch insisted that it got
> argc == 6 or something, so *cmd might be NUL but cmd would never be
> NULL if I understand your code correctly.  IOW, shouldn't the line
> be more like:
> 
> 	if (!cmd)
> 		BUG("...");
> 	if (*cmd)
> 
> ?
> 

I don’t know, it’s not really problematic if cmd is NULL.  And I think
that when interactive rebase will be a builtin, it will be possible for
cmd to be NULL.

> 
>> +	if (strbuf_read_file(buf, todo_file, 0) < 0)
>> +		return error_errno(_("could not read '%s'."), todo_file);
>> +	if (parse_insn_buffer(buf->buf, &todo_list)) {
> 
> Nice that we have parse* function.  We do not have to work with
> stripspace plus "wc -l" ;-).
> 
>> +		todo_list_release(&todo_list);
>> +		return error(_("unusable todo list: '%s'"), todo_file);
> 
> When the users see this error message, is it easy for them to
> diagnose what went wrong (e.g. has parse_insn_buffer() already
> issued some message to help pinpoint which insn in the todo list is
> misspelt, for example)?
> 

Yes, parse_insn_buffer() will say which line caused the error.  But at
this point, the user should not have changed the todo-list, so this
error should never appear.

Perhaps this is a good use case of BUG()?

>> +	}
>> +
>> +	strbuf_addch(buf, '\n');
>> +	strbuf_commented_addf(buf, Q_("Rebase %s onto %s (%d command)",
>> +				      "Rebase %s onto %s (%d commands)",
>> +				      todo_list.nr),
>> +			      shortrevisions, shortonto, todo_list.nr);
>> +	append_todo_help(0, keep_empty, buf);
>> +
>> +	if (write_message(buf->buf, buf->len, todo_file, 0)) {
>> +		todo_list_release(&todo_list);
>> +		return error_errno(_("could not write '%s'"), todo_file);
>> +	}
>> +	copy_file(rebase_path_todo_backup(), todo_file, 0666);
>> +	transform_todos(flags | TODO_LIST_SHORTEN_IDS);
>> +
> 
> It is a bit sad that we are mostly working on the byte array
> buf->buf (or external file touched by various helper functions we
> call), even though we have a perfectly usable parsed representation
> in todo_list structure in all of the above and the rest of this
> function.
> 
> It might be better to grab todo_list.nr into a local simple integer
> variable immediately after parse_insn_buffer() returns and then call
> todo_list_release() on todo_list, as the parsed todo-list is only
> used for two purposes (i.e. detecting format error by seeing if
> parser returns success, and seeing how many insns are on the todo
> list by checking todo_list.nr field) and nothing else.  By releasing
> the otherwise unused todo_list early, you do not have to sprinkle
> various error return codepaths with calls to todo_list_release().
> 
> That incidentally would manage too high expectation from readers of
> the code as well ;-).
> 

Unfortunately, this strbuf is part of the todo_list, and it will be
freed if I call todo_list_release().

:/

Cheers,
Alban


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

* Re: [GSoC][PATCH v3 12/13] rebase -i: implement the logic to initialize the variable $revision in C
  2018-07-10 12:15   ` [GSoC][PATCH v3 12/13] rebase -i: implement the logic to initialize the variable $revision in C Alban Gruin
@ 2018-07-12 18:15     ` Junio C Hamano
  2018-07-12 18:24       ` Junio C Hamano
  0 siblings, 1 reply; 50+ messages in thread
From: Junio C Hamano @ 2018-07-12 18:15 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

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

> @@ -50,6 +71,13 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>  			    N_("prepare the branch to be rebased"), PREPARE_BRANCH),
>  		OPT_CMDMODE(0, "complete-action", &command,
>  			    N_("complete the action"), COMPLETE_ACTION),
> +		OPT_STRING(0, "onto", &onto, N_("onto"), N_("onto")),
> +		OPT_STRING(0, "restrict-revision", &restrict_revisions,
> +			   N_("restrict-revision"), N_("restrict revision")),
> +		OPT_STRING(0, "squash-onto", &squash_onto, N_("squash-onto"),
> +			   N_("squash onto")),
> +		OPT_STRING(0, "upstream", &upstream, N_("upstream"),
> +			   N_("the upstream commit")),
>  		OPT_END()
>  	};
>  
> @@ -77,8 +105,30 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>  		return !!sequencer_continue(&opts);
>  	if (command == ABORT && argc == 1)
>  		return !!sequencer_remove_state(&opts);
> -	if (command == MAKE_SCRIPT && argc > 1)
> -		return !!sequencer_make_script(stdout, argc, argv, flags);
> +	if (command == MAKE_SCRIPT && (argc == 1 || argc == 2)) {

Sorry but I am confused.  The call to rebase--helper --make-script
in git-rebase--interactive.sh we see below has only dashed options
(like --upstream, --onto, --squash-onto, --restrict-revision) and
the option parameters given to these options.

What are the remaining 1 or 2 arguments we are processing here?

Well, actually argc==1 means there is nothing else, so I am asking
about the case where argc==2.

> +		char *revisions = NULL;
> +		struct argv_array make_script_args = ARGV_ARRAY_INIT;
> +
> +		if (!upstream && squash_onto)
> +			write_file(path_squash_onto(), "%s\n", squash_onto);
> +
> +		ret = get_revision_ranges(upstream, onto, &head_hash, &revisions);
> +		if (ret)
> +			return ret;
> +
> +		argv_array_pushl(&make_script_args, "", revisions, NULL);
> +		if (argc == 2)
> +			argv_array_push(&make_script_args, restrict_revisions);
> +
> +		ret = !!sequencer_make_script(stdout,
> +					      make_script_args.argc, make_script_args.argv,
> +					      flags);

Exactly the same comment on !! as an earlier step applies here.

> +		free(revisions);
> +		argv_array_clear(&make_script_args);

If I am reading the body of this if() block correctly, I think it
does everything init_revisions_and_shortrevisions shell function
does, i.e. compute $revisions for both cases with or without
upstream and write squash-onto state if needed, so that we can call
the sequencer_make_script() helper with necessary $revisions arg
without being passed from the command line of --make-script helper.

But the hunk below tells me that you are still calling
init_revisions_and_shortrevisions shell function before we are
called.  Why?  IOW, why isn't this step removing that shell function
*and* the call to it, now its logic is fully implemented in the body
of this if() block?

> +		return ret;
> +	}
>  	if (command == CHECK_TODO_LIST && argc == 1)
>  		return !!check_todo_list();
>  	if (command == REARRANGE_SQUASH && argc == 1)

Thanks.

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 0d66c0f8b..4ca47aed1 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -92,7 +92,9 @@ git_rebase__interactive () {
>  	git rebase--helper --make-script ${keep_empty:+--keep-empty} \
>  		${rebase_merges:+--rebase-merges} \
>  		${rebase_cousins:+--rebase-cousins} \
> -		$revisions ${restrict_revision+^$restrict_revision} >"$todo" ||
> +		${upstream:+--upstream "$upstream"} ${onto:+--onto "$onto"} \
> +		${squash_onto:+--squash-onto "$squash_onto"} \
> +		${restrict_revision:+--restrict-revision ^"$restrict_revision"} >"$todo" ||
>  	die "$(gettext "Could not generate todo list")"
>  
>  	exec git rebase--helper --complete-action "$shortrevisions" "$onto_name" \

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

* Re: [GSoC][PATCH v3 12/13] rebase -i: implement the logic to initialize the variable $revision in C
  2018-07-12 18:15     ` Junio C Hamano
@ 2018-07-12 18:24       ` Junio C Hamano
  0 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2018-07-12 18:24 UTC (permalink / raw)
  To: Alban Gruin
  Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
	Johannes Schindelin, phillip.wood

Junio C Hamano <gitster@pobox.com> writes:

> If I am reading the body of this if() block correctly, I think it
> does everything init_revisions_and_shortrevisions shell function
> does, i.e. compute $revisions for both cases with or without
> upstream and write squash-onto state if needed, so that we can call
> the sequencer_make_script() helper with necessary $revisions arg
> without being passed from the command line of --make-script helper.
>
> But the hunk below tells me that you are still calling
> init_revisions_and_shortrevisions shell function before we are
> called.  Why?  IOW, why isn't this step removing that shell function
> *and* the call to it, now its logic is fully implemented in the body
> of this if() block?

You can ignore this part (but not the rest) of my comments, as 13/13
answers it adequately.  After this step, the shell function still
needs to be called to set $shortrevisions.

Thanks.

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

end of thread, back to index

Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-02 10:57 [GSoC][PATCH v2 0/7] rebase -i: rewrite some parts in C Alban Gruin
2018-07-02 10:57 ` [GSoC][PATCH v2 1/7] sequencer: make two functions and an enum from sequencer.c public Alban Gruin
2018-07-03 20:20   ` Junio C Hamano
2018-07-05 15:35     ` Alban Gruin
2018-07-02 10:57 ` [GSoC][PATCH v2 2/7] rebase--interactive: rewrite append_todo_help() in C Alban Gruin
2018-07-03 20:29   ` Junio C Hamano
2018-07-02 10:57 ` [GSoC][PATCH v2 3/7] editor: add a function to launch the sequence editor Alban Gruin
2018-07-03 20:32   ` Junio C Hamano
2018-07-02 10:57 ` [GSoC][PATCH v2 4/7] rebase-interactive: rewrite the edit-todo functionality in C Alban Gruin
2018-07-02 10:57 ` [GSoC][PATCH v2 5/7] sequencer: add a new function to silence a command, except if it fails Alban Gruin
2018-07-03 20:36   ` Junio C Hamano
2018-07-02 10:57 ` [GSoC][PATCH v2 6/7] rebase -i: rewrite setup_reflog_action() in C Alban Gruin
2018-07-03 20:37   ` Junio C Hamano
2018-07-06 12:58     ` Johannes Schindelin
2018-07-06 15:02       ` Junio C Hamano
2018-07-06 18:54         ` Johannes Schindelin
2018-07-06 21:55           ` Junio C Hamano
2018-07-07 16:40             ` Junio C Hamano
2018-07-02 10:57 ` [GSoC][PATCH v2 7/7] rebase -i: rewrite checkout_onto() " Alban Gruin
2018-07-10 12:15 ` [GSoC][PATCH v3 00/13] rebase -i: rewrite some parts " Alban Gruin
2018-07-10 12:15   ` [GSoC][PATCH v3 01/13] sequencer: make two functions and an enum from sequencer.c public Alban Gruin
2018-07-10 17:53     ` Junio C Hamano
2018-07-10 12:15   ` [GSoC][PATCH v3 02/13] rebase--interactive: rewrite append_todo_help() in C Alban Gruin
2018-07-10 12:21     ` Alban Gruin
2018-07-10 17:56     ` Junio C Hamano
2018-07-10 12:15   ` [GSoC][PATCH v3 03/13] editor: add a function to launch the sequence editor Alban Gruin
2018-07-10 12:23     ` Alban Gruin
2018-07-10 17:58     ` Junio C Hamano
2018-07-10 12:15   ` [GSoC][PATCH v3 04/13] rebase-interactive: rewrite the edit-todo functionality in C Alban Gruin
2018-07-10 18:00     ` Junio C Hamano
2018-07-10 12:15   ` [GSoC][PATCH v3 05/13] sequencer: add a new function to silence a command, except if it fails Alban Gruin
2018-07-10 18:13     ` Junio C Hamano
2018-07-10 12:15   ` [GSoC][PATCH v3 06/13] rebase -i: rewrite setup_reflog_action() in C Alban Gruin
2018-07-10 18:20     ` Junio C Hamano
2018-07-10 12:15   ` [GSoC][PATCH v3 07/13] rebase -i: rewrite checkout_onto() " Alban Gruin
2018-07-10 18:22     ` Junio C Hamano
2018-07-10 12:15   ` [GSoC][PATCH v3 08/13] sequencer: refactor append_todo_help() to write its message to a buffer Alban Gruin
2018-07-10 18:30     ` Junio C Hamano
2018-07-10 12:15   ` [GSoC][PATCH v3 09/13] sequencer: change the way skip_unnecessary_picks() returns its result Alban Gruin
2018-07-10 18:38     ` Junio C Hamano
2018-07-10 18:52     ` Junio C Hamano
2018-07-10 12:15   ` [GSoC][PATCH v3 10/13] rebase--interactive: rewrite complete_action() in C Alban Gruin
2018-07-10 22:33     ` Junio C Hamano
2018-07-11 14:25       ` Alban Gruin
2018-07-10 12:15   ` [GSoC][PATCH v3 11/13] rebase--interactive: remove unused modes and functions Alban Gruin
2018-07-10 22:36     ` Junio C Hamano
2018-07-10 12:15   ` [GSoC][PATCH v3 12/13] rebase -i: implement the logic to initialize the variable $revision in C Alban Gruin
2018-07-12 18:15     ` Junio C Hamano
2018-07-12 18:24       ` Junio C Hamano
2018-07-10 12:15   ` [GSoC][PATCH v3 13/13] rebase -i: rewrite the rest of init_revisions_and_shortrevisions " Alban Gruin

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