[PATCH 0/2] Let the sequencer handle the grunt work of rebase -i

[PATCH 0/2] Let the sequencer handle the grunt work of rebase -i

[Johannes Schindelin]

After all of these patch series y'all had to review, this is finally the
one that switches things over.

Please note that it does not (yet) handle the `git rebase -i --root`
invocation; I tried to focus on the common case, and I rarely use --root
myself.

Please note also that --preserve-merges is *not* handled.

The way I designed --preserve-merges is totally stupid and idiotic and I
do not want to spend any further time on it. You cannot "pick" merges
and hope to be able to reorder commits, for example.

And please finally note that this pair of patches does not yet yield the
full speed improvement that I promised earlier. After these patches, the
time is dominated by pre- and post-processing the todo script, at least
on Windows, so there is another patch series that ports those bits and
pieces into the rebase--helper, too.


Johannes Schindelin (2):
  Add a builtin helper for interactive rebases
  rebase -i: use the rebase--helper builtin

 .gitignore                 |  1 +
 Makefile                   |  1 +
 builtin.h                  |  1 +
 builtin/rebase--helper.c   | 40 ++++++++++++++++++++++++++++++++++++++++
 git-rebase--interactive.sh | 13 +++++++++++++
 git.c                      |  1 +
 6 files changed, 57 insertions(+)
 create mode 100644 builtin/rebase--helper.c

Based-On: sequencer-i at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git sequencer-i
Published-As: https://github.com/dscho/git/releases/tag/rebase--helper-v1
Fetch-It-Via: git fetch https://github.com/dscho/git rebase--helper-v1

-- 
2.9.3.windows.3

base-commit: bbec81903b5e46c481fdc0cfe6f10166423526f1

[PATCH 1/2] Add a builtin helper for interactive rebases

[Johannes Schindelin]

Git's interactive rebase is still implemented as a shell script, despite
its complexity. This implies that it suffers from the portability point
of view, from lack of expressibility, and of course also from
performance. The latter issue is particularly serious on Windows, where
we pay a hefty price for relying so much on POSIX.

Unfortunately, being such a huge shell script also means that we missed
the train when it would have been relatively easy to port it to C, and
instead piled feature upon feature onto that poor script that originally
never intended to be more than a slightly pimped cherry-pick in a loop.

To open the road toward better performance (in addition to all the other
benefits of C over shell scripts), let's just start *somewhere*.

The approach taken here is to add a builtin helper that at first intends
to take care of the parts of the interactive rebase that are most
affected by the performance penalties mentioned above.

In particular, after we spent all those efforts on preparing the sequencer
to process rebase -i's git-rebase-todo scripts, we implement the `git
rebase -i --continue` functionality as a new builtin, git-rebase--helper.

Once that is in place, we can work gradually on tackling the rest of the
technical debt.

Note that the rebase--helper needs to learn about the transient
--ff/--no-ff options of git-rebase, as the corresponding flag is not
persisted to, and re-read from, the state directory.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 .gitignore               |  1 +
 Makefile                 |  1 +
 builtin.h                |  1 +
 builtin/rebase--helper.c | 40 ++++++++++++++++++++++++++++++++++++++++
 git.c                    |  1 +
 5 files changed, 44 insertions(+)
 create mode 100644 builtin/rebase--helper.c

diff --git a/.gitignore b/.gitignore
index 05cb58a..a9b8c96 100644
--- a/.gitignore
+++ b/.gitignore
@@ -114,6 +114,7 @@
 /git-read-tree
 /git-rebase
 /git-rebase--am
+/git-rebase--helper
 /git-rebase--interactive
 /git-rebase--merge
 /git-receive-pack
diff --git a/Makefile b/Makefile
index d96ecb7..980e1dc 100644
--- a/Makefile
+++ b/Makefile
@@ -919,6 +919,7 @@ BUILTIN_OBJS += builtin/prune.o
 BUILTIN_OBJS += builtin/pull.o
 BUILTIN_OBJS += builtin/push.o
 BUILTIN_OBJS += builtin/read-tree.o
+BUILTIN_OBJS += builtin/rebase--helper.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
 BUILTIN_OBJS += builtin/remote.o
diff --git a/builtin.h b/builtin.h
index 6b95006..2e5de14 100644
--- a/builtin.h
+++ b/builtin.h
@@ -102,6 +102,7 @@ extern int cmd_prune_packed(int argc, const char **argv, const char *prefix);
 extern int cmd_pull(int argc, const char **argv, const char *prefix);
 extern int cmd_push(int argc, const char **argv, const char *prefix);
 extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_rebase__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_reflog(int argc, const char **argv, const char *prefix);
 extern int cmd_remote(int argc, const char **argv, const char *prefix);
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
new file mode 100644
index 0000000..ca1ebb2
--- /dev/null
+++ b/builtin/rebase--helper.c
@@ -0,0 +1,40 @@
+#include "builtin.h"
+#include "cache.h"
+#include "parse-options.h"
+#include "sequencer.h"
+
+static const char * const builtin_rebase_helper_usage[] = {
+	N_("git rebase--helper [<options>]"),
+	NULL
+};
+
+int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
+{
+	struct replay_opts opts = REPLAY_OPTS_INIT;
+	enum {
+		CONTINUE = 1, ABORT
+	} command = 0;
+	struct option options[] = {
+		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
+		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
+				CONTINUE),
+		OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
+				ABORT),
+		OPT_END()
+	};
+
+	git_config(git_default_config, NULL);
+
+	opts.action = REPLAY_INTERACTIVE_REBASE;
+	opts.allow_ff = 1;
+	opts.allow_empty = 1;
+
+	argc = parse_options(argc, argv, NULL, options,
+			builtin_rebase_helper_usage, PARSE_OPT_KEEP_ARGV0);
+
+	if (command == CONTINUE && argc == 1)
+		return !!sequencer_continue(&opts);
+	if (command == ABORT && argc == 1)
+		return !!sequencer_remove_state(&opts);
+	usage_with_options(builtin_rebase_helper_usage, options);
+}
diff --git a/git.c b/git.c
index 0f1937f..26b4ad3 100644
--- a/git.c
+++ b/git.c
@@ -451,6 +451,7 @@ static struct cmd_struct commands[] = {
 	{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
 	{ "push", cmd_push, RUN_SETUP },
 	{ "read-tree", cmd_read_tree, RUN_SETUP },
+	{ "rebase--helper", cmd_rebase__helper, RUN_SETUP | NEED_WORK_TREE },
 	{ "receive-pack", cmd_receive_pack },
 	{ "reflog", cmd_reflog, RUN_SETUP },
 	{ "remote", cmd_remote, RUN_SETUP },
-- 
2.9.3.windows.3

[PATCH 2/2] rebase -i: use the rebase--helper builtin

[Johannes Schindelin]

Now that the sequencer learned to process a "normal" interactive rebase,
we use it. The original shell script is still used for "non-normal"
interactive rebases, i.e. when --root or --preserve-merges was passed.

Please note that the --root option (via the $squash_onto variable) needs
special handling only for the very first command, hence it is still okay
to use the helper upon continue/skip.

Also please note that the --no-ff setting is volatile, i.e. when the
interactive rebase is interrupted at any stage, there is no record of
it. Therefore, we have to pass it from the shell script to the
rebase--helper.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase--interactive.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 7e558b0..022766b 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1059,6 +1059,10 @@ git_rebase__interactive () {
 
 case "$action" in
 continue)
+	if test ! -d "$rewritten"
+	then
+		exec git rebase--helper ${force_rebase:+--no-ff} --continue
+	fi
 	# do we have anything to commit?
 	if git diff-index --cached --quiet HEAD --
 	then
@@ -1118,6 +1122,10 @@ first and then run 'git rebase --continue' again.")"
 skip)
 	git rerere clear
 
+	if test ! -d "$rewritten"
+	then
+		exec git rebase--helper ${force_rebase:+--no-ff} --continue
+	fi
 	do_rest
 	return 0
 	;;
@@ -1307,6 +1315,11 @@ expand_todo_ids
 test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks
 
 checkout_onto
+if test -z "$rebase_root" && test ! -d "$rewritten"
+then
+	require_clean_work_tree "rebase"
+	exec git rebase--helper ${force_rebase:+--no-ff} --continue
+fi
 do_rest
 
 }
-- 
2.9.3.windows.3

[PATCH v2 0/2] Let the sequencer handle the grunt work of rebase -i

[Johannes Schindelin]

After all of these patch series y'all had to review, this is finally the
one that switches things over.

Please note that it does not (yet) handle the `git rebase -i --root`
invocation; I tried to focus on the common case, and I rarely use --root
myself.

Please note also that --preserve-merges is *not* handled.

The way I designed --preserve-merges is totally stupid and idiotic and I
do not want to spend any further time on it. You cannot "pick" merges
and hope to be able to reorder commits, for example. I may work on
porting Git garden shears' way to recreate the branch structure into
rebase -i proper at some stage.

And please finally note that this pair of patches does not yet yield the
full speed improvement that I promised earlier. After these patches, the
time is dominated by pre- and post-processing the todo script, at least
on Windows, so there is another patch series that ports those bits and
pieces into the rebase--helper, too.

Changes since v1:

- rebased to current master

- this required a change in t3404 because I was bullied^Wasked to change
  some messages (which should not have been conflated with the work I
  actually wanted to do, but whatevs)


Johannes Schindelin (2):
  Add a builtin helper for interactive rebases
  rebase -i: use the rebase--helper builtin

 .gitignore                    |  1 +
 Makefile                      |  1 +
 builtin.h                     |  1 +
 builtin/rebase--helper.c      | 40 ++++++++++++++++++++++++++++++++++++++++
 git-rebase--interactive.sh    | 13 +++++++++++++
 git.c                         |  1 +
 t/t3404-rebase-interactive.sh |  2 +-
 7 files changed, 58 insertions(+), 1 deletion(-)
 create mode 100644 builtin/rebase--helper.c


base-commit: 6e3a7b3398559305c7a239a42e447c21a8f39ff8
Based-On: sequencer-i at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git sequencer-i
Published-As: https://github.com/dscho/git/releases/tag/rebase--helper-v2
Fetch-It-Via: git fetch https://github.com/dscho/git rebase--helper-v2

Interdiff vs v1:

 diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
 index e2f18d11f6..33d392ba11 100755
 --- a/t/t3404-rebase-interactive.sh
 +++ b/t/t3404-rebase-interactive.sh
 @@ -556,7 +556,7 @@ test_expect_success 'clean error after failed "exec"' '
  	echo "edited again" > file7 &&
  	git add file7 &&
  	test_must_fail git rebase --continue 2>error &&
 -	test_i18ngrep "You have staged changes in your working tree." error
 +	test_i18ngrep "you have staged changes in your working tree" error
  '
  
  test_expect_success 'rebase a detached HEAD' '

-- 
2.11.1.windows.1

[PATCH v2 1/2] Add a builtin helper for interactive rebases

[Johannes Schindelin]

Git's interactive rebase is still implemented as a shell script, despite
its complexity. This implies that it suffers from the portability point
of view, from lack of expressibility, and of course also from
performance. The latter issue is particularly serious on Windows, where
we pay a hefty price for relying so much on POSIX.

Unfortunately, being such a huge shell script also means that we missed
the train when it would have been relatively easy to port it to C, and
instead piled feature upon feature onto that poor script that originally
never intended to be more than a slightly pimped cherry-pick in a loop.

To open the road toward better performance (in addition to all the other
benefits of C over shell scripts), let's just start *somewhere*.

The approach taken here is to add a builtin helper that at first intends
to take care of the parts of the interactive rebase that are most
affected by the performance penalties mentioned above.

In particular, after we spent all those efforts on preparing the sequencer
to process rebase -i's git-rebase-todo scripts, we implement the `git
rebase -i --continue` functionality as a new builtin, git-rebase--helper.

Once that is in place, we can work gradually on tackling the rest of the
technical debt.

Note that the rebase--helper needs to learn about the transient
--ff/--no-ff options of git-rebase, as the corresponding flag is not
persisted to, and re-read from, the state directory.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 .gitignore               |  1 +
 Makefile                 |  1 +
 builtin.h                |  1 +
 builtin/rebase--helper.c | 40 ++++++++++++++++++++++++++++++++++++++++
 git.c                    |  1 +
 5 files changed, 44 insertions(+)
 create mode 100644 builtin/rebase--helper.c

diff --git a/.gitignore b/.gitignore
index b1020b875f..833ef3b0b7 100644
--- a/.gitignore
+++ b/.gitignore
@@ -114,6 +114,7 @@
 /git-read-tree
 /git-rebase
 /git-rebase--am
+/git-rebase--helper
 /git-rebase--interactive
 /git-rebase--merge
 /git-receive-pack
diff --git a/Makefile b/Makefile
index 8e4081e061..29f8663509 100644
--- a/Makefile
+++ b/Makefile
@@ -932,6 +932,7 @@ BUILTIN_OBJS += builtin/prune.o
 BUILTIN_OBJS += builtin/pull.o
 BUILTIN_OBJS += builtin/push.o
 BUILTIN_OBJS += builtin/read-tree.o
+BUILTIN_OBJS += builtin/rebase--helper.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
 BUILTIN_OBJS += builtin/remote.o
diff --git a/builtin.h b/builtin.h
index 67f80519da..9e4a89816d 100644
--- a/builtin.h
+++ b/builtin.h
@@ -103,6 +103,7 @@ extern int cmd_prune_packed(int argc, const char **argv, const char *prefix);
 extern int cmd_pull(int argc, const char **argv, const char *prefix);
 extern int cmd_push(int argc, const char **argv, const char *prefix);
 extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_rebase__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_reflog(int argc, const char **argv, const char *prefix);
 extern int cmd_remote(int argc, const char **argv, const char *prefix);
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
new file mode 100644
index 0000000000..ca1ebb2fa1
--- /dev/null
+++ b/builtin/rebase--helper.c
@@ -0,0 +1,40 @@
+#include "builtin.h"
+#include "cache.h"
+#include "parse-options.h"
+#include "sequencer.h"
+
+static const char * const builtin_rebase_helper_usage[] = {
+	N_("git rebase--helper [<options>]"),
+	NULL
+};
+
+int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
+{
+	struct replay_opts opts = REPLAY_OPTS_INIT;
+	enum {
+		CONTINUE = 1, ABORT
+	} command = 0;
+	struct option options[] = {
+		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
+		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
+				CONTINUE),
+		OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
+				ABORT),
+		OPT_END()
+	};
+
+	git_config(git_default_config, NULL);
+
+	opts.action = REPLAY_INTERACTIVE_REBASE;
+	opts.allow_ff = 1;
+	opts.allow_empty = 1;
+
+	argc = parse_options(argc, argv, NULL, options,
+			builtin_rebase_helper_usage, PARSE_OPT_KEEP_ARGV0);
+
+	if (command == CONTINUE && argc == 1)
+		return !!sequencer_continue(&opts);
+	if (command == ABORT && argc == 1)
+		return !!sequencer_remove_state(&opts);
+	usage_with_options(builtin_rebase_helper_usage, options);
+}
diff --git a/git.c b/git.c
index c887272b12..33f52acbcc 100644
--- a/git.c
+++ b/git.c
@@ -473,6 +473,7 @@ static struct cmd_struct commands[] = {
 	{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
 	{ "push", cmd_push, RUN_SETUP },
 	{ "read-tree", cmd_read_tree, RUN_SETUP | SUPPORT_SUPER_PREFIX},
+	{ "rebase--helper", cmd_rebase__helper, RUN_SETUP | NEED_WORK_TREE },
 	{ "receive-pack", cmd_receive_pack },
 	{ "reflog", cmd_reflog, RUN_SETUP },
 	{ "remote", cmd_remote, RUN_SETUP },
-- 
2.11.1.windows.1

[PATCH v2 2/2] rebase -i: use the rebase--helper builtin

[Johannes Schindelin]

Now that the sequencer learned to process a "normal" interactive rebase,
we use it. The original shell script is still used for "non-normal"
interactive rebases, i.e. when --root or --preserve-merges was passed.

Please note that the --root option (via the $squash_onto variable) needs
special handling only for the very first command, hence it is still okay
to use the helper upon continue/skip.

Also please note that the --no-ff setting is volatile, i.e. when the
interactive rebase is interrupted at any stage, there is no record of
it. Therefore, we have to pass it from the shell script to the
rebase--helper.

Note: the test t3404 had to be adjusted because the the error messages
produced by the sequencer comply with our current convention to start with
a lower-case letter.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase--interactive.sh    | 13 +++++++++++++
 t/t3404-rebase-interactive.sh |  2 +-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 4734094a3f..2c9c0165b5 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1069,6 +1069,10 @@ git_rebase__interactive () {
 
 case "$action" in
 continue)
+	if test ! -d "$rewritten"
+	then
+		exec git rebase--helper ${force_rebase:+--no-ff} --continue
+	fi
 	# do we have anything to commit?
 	if git diff-index --cached --quiet HEAD --
 	then
@@ -1128,6 +1132,10 @@ first and then run 'git rebase --continue' again.")"
 skip)
 	git rerere clear
 
+	if test ! -d "$rewritten"
+	then
+		exec git rebase--helper ${force_rebase:+--no-ff} --continue
+	fi
 	do_rest
 	return 0
 	;;
@@ -1314,6 +1322,11 @@ expand_todo_ids
 test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks
 
 checkout_onto
+if test -z "$rebase_root" && test ! -d "$rewritten"
+then
+	require_clean_work_tree "rebase"
+	exec git rebase--helper ${force_rebase:+--no-ff} --continue
+fi
 do_rest
 
 }
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e2f18d11f6..33d392ba11 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -556,7 +556,7 @@ test_expect_success 'clean error after failed "exec"' '
 	echo "edited again" > file7 &&
 	git add file7 &&
 	test_must_fail git rebase --continue 2>error &&
-	test_i18ngrep "You have staged changes in your working tree." error
+	test_i18ngrep "you have staged changes in your working tree" error
 '
 
 test_expect_success 'rebase a detached HEAD' '
-- 
2.11.1.windows.1

Re: [PATCH v2 0/2] Let the sequencer handle the grunt work of rebase -i

[Junio C Hamano]

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

> After all of these patch series y'all had to review, this is finally the
> one that switches things over.
>
> Please note that it does not (yet) handle the `git rebase -i --root`
> invocation; I tried to focus on the common case, and I rarely use --root
> myself.

As long as the longer-term goal is clear enough and the short-term
approach does not conflict with the goal, solving the most common
problem that yields the largest payback first is absolutely the
right thing to do, and omitting "--root" and/or "-p" and getting the
main use of "-i" right is a great way to start.

>  .gitignore                    |  1 +
>  Makefile                      |  1 +
>  builtin.h                     |  1 +
>  builtin/rebase--helper.c      | 40 ++++++++++++++++++++++++++++++++++++++++
>  git-rebase--interactive.sh    | 13 +++++++++++++
>  git.c                         |  1 +
>  t/t3404-rebase-interactive.sh |  2 +-
>  7 files changed, 58 insertions(+), 1 deletion(-)
>  create mode 100644 builtin/rebase--helper.c

And it is surprisingly short and sweet ;-)

Will queue as js/rebase-helper topic, forked at 6e3a7b3398 ("Git
2.12-rc0", 2017-02-03).

Thanks.


PS. in case if anybody is wondering after reading [*1*], at this
point, I _have_ read the patches not just the cover letter, looked
at the branch name the original author gave to the topic, chose the
local topic name I use, and chose where to fork the topic from, but
have not applied the patches (so I may later end up saying "the
patch does not apply cleanly", "the compiler complains on this
line", or "the new code is inconsistent with this existing code that
is a bit beyond the context of the patch that I did not notice when
I reviewed the patches alone" in a separate message).  I do not have
a new entry for this topic in the draft of "What's cooking" report
yet, or have not decided if the topic would hit 'jch' or 'pu' yet
either.


[Reference]

*1* http://public-inbox.org/git/xmqq7f4zqiyj.fsf@gitster.mtv.corp.google.com