[PATCH] rebase -i: introduce the 'test' command

[PATCH] rebase -i: introduce the 'test' command

[Paul Morelle]

The 'exec' command can be used to run tests on a set of commits,
interrupting on failing commits to let the user fix the tests.

However, the 'exec' line has been consumed, so it won't be ran again by
'git rebase --continue' is ran, even if the tests weren't fixed.

This commit introduces a new command 'test' equivalent to 'exec', except
that it is automatically rescheduled in the todo list if it fails.

Signed-off-by: Paul Morelle <paul.morelle@gmail.com>
---
 Documentation/git-rebase.txt  |  9 ++++++---
 rebase-interactive.c          |  1 +
 sequencer.c                   | 23 +++++++++++++++--------
 t/lib-rebase.sh               |  4 +++-
 t/t3404-rebase-interactive.sh | 16 ++++++++++++++++
 5 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 80793bad8..c8f565637 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -693,8 +693,8 @@ $ git rebase -i -p --onto Q O
 Reordering and editing commits usually creates untested intermediate
 steps.  You may want to check that your history editing did not break
 anything by running a test, or at least recompiling at intermediate
-points in history by using the "exec" command (shortcut "x").  You may
-do so by creating a todo list like this one:
+points in history by using the "exec" command (shortcut "x") or the
+"test" command.  You may do so by creating a todo list like this one:
  -------------------------------------------
 pick deadbee Implement feature XXX
@@ -702,7 +702,7 @@ fixup f1a5c00 Fix to feature XXX
 exec make
 pick c0ffeee The oneline of the next commit
 edit deadbab The oneline of the commit after
-exec cd subdir; make test
+test cd subdir; make test
 ...
 -------------------------------------------
 @@ -715,6 +715,9 @@ in `$SHELL`, or the default shell if `$SHELL` is
not set), so you can
 use shell features (like "cd", ">", ";" ...). The command is run from
 the root of the working tree.
 +The "test" command does the same, but will remain in the todo list as
+the next command, until it succeeds.
+
 ----------------------------------
 $ git rebase -i --exec "make test"
 ----------------------------------
diff --git a/rebase-interactive.c b/rebase-interactive.c
index 78f3263fc..4a408661d 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -14,6 +14,7 @@ void append_todo_help(unsigned edit_todo, unsigned
keep_empty,
 "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"
+"   test <command> = same as exec command, but keep it in TODO if it
fails\n"
 "b, break = stop here (continue rebase later with 'git rebase
--continue')\n"
 "d, drop <commit> = remove commit\n"
 "l, label <label> = label current HEAD with a name\n"
diff --git a/sequencer.c b/sequencer.c
index e1a4dd15f..c3dde6910 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1508,6 +1508,7 @@ enum todo_command {
 	TODO_SQUASH,
 	/* commands that do something else than handling a single commit */
 	TODO_EXEC,
+	TODO_TEST,
 	TODO_BREAK,
 	TODO_LABEL,
 	TODO_RESET,
@@ -1530,6 +1531,7 @@ static struct {
 	{ 'f', "fixup" },
 	{ 's', "squash" },
 	{ 'x', "exec" },
+	{ 0,   "test" },
 	{ 'b', "break" },
 	{ 'l', "label" },
 	{ 't', "reset" },
@@ -2072,7 +2074,7 @@ static int parse_insn_line(struct todo_item *item,
const char *bol, char *eol)
 			     command_to_string(item->command));
  	if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
-	    item->command == TODO_RESET) {
+	    item->command == TODO_RESET || item->command == TODO_TEST) {
 		item->commit = NULL;
 		item->arg = bol;
 		item->arg_len = (int)(eol - bol);
@@ -3576,7 +3578,7 @@ static int pick_commits(struct todo_list
*todo_list, struct replay_opts *opts)
 						item->arg, item->arg_len, opts,
 						res, to_amend);
 			}
-		} else if (item->command == TODO_EXEC) {
+		} else if (item->command == TODO_EXEC || item->command == TODO_TEST) {
 			char *end_of_arg = (char *)(item->arg + item->arg_len);
 			int saved = *end_of_arg;
 			struct stat st;
@@ -3586,9 +3588,12 @@ static int pick_commits(struct todo_list
*todo_list, struct replay_opts *opts)
 			*end_of_arg = saved;
  			/* Reread the todo file if it has changed. */
-			if (res)
+			if (res) {
 				; /* fall through */
-			else if (stat(get_todo_path(opts), &st))
+				if (item->command == TODO_TEST) {
+					reschedule = 1;
+				}
+			} else if (stat(get_todo_path(opts), &st))
 				res = error_errno(_("could not stat '%s'"),
 						  get_todo_path(opts));
 			else if (match_stat_data(&todo_list->stat, &st)) {
@@ -3622,10 +3627,12 @@ static int pick_commits(struct todo_list
*todo_list, struct replay_opts *opts)
 			return error(_("unknown command %d"), item->command);
  		if (reschedule) {
-			advise(_(rescheduled_advice),
-			       get_item_line_length(todo_list,
-						    todo_list->current),
-			       get_item_line(todo_list, todo_list->current));
+			if (item->command != TODO_TEST) {
+				advise(_(rescheduled_advice),
+				       get_item_line_length(todo_list,
+							    todo_list->current),
+				       get_item_line(todo_list, todo_list->current));
+			}
 			todo_list->current--;
 			if (save_todo(todo_list, opts))
 				return -1;
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 7ea30e500..7d36f00bd 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -19,6 +19,8 @@
 #
 #   "exec_cmd_with_args" -- add an "exec cmd with args" line.
 #
+#   "test_cmd_with_args" -- add a "test cmd with args" line.
+#
 #   "#" -- Add a comment line.
 #
 #   ">" -- Add a blank line.
@@ -49,7 +51,7 @@ set_fake_editor () {
 		case $line in
 		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
 			action="$line";;
-		exec_*|x_*|break|b)
+		exec_*|x_*|break|b|test_*)
 			echo "$line" | sed 's/_/ /g' >> "$1";;
 		"#")
 			echo '# comment' >> "$1";;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 7a440e08d..14c60b14d 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1453,4 +1453,20 @@ test_expect_success 'valid author header when
author contains single quote' '
 	test_cmp expected actual
 '
 +test_expect_success 'rebase -i keeps test until it passes' '
+	git checkout master &&
+	(
+	set_fake_editor &&
+	FAKE_LINES="1 test_false 2 3 4 5" &&
+	export FAKE_LINES &&
+	test_must_fail git rebase -i A &&
+	test_cmp_rev B HEAD &&
+	test_must_fail git rebase --continue &&
+	test_cmp_rev B HEAD &&
+	FAKE_LINES="test_true 2 3 4" git rebase --edit-todo &&
+	git rebase --continue
+	) &&
+	test_cmp_rev master HEAD
+'
+
 test_done
-- 
2.19.1

Re: [PATCH] rebase -i: introduce the 'test' command

[Johannes Schindelin]

Hi Paul,

On Wed, 28 Nov 2018, Paul Morelle wrote:

> The 'exec' command can be used to run tests on a set of commits,
> interrupting on failing commits to let the user fix the tests.
> 
> However, the 'exec' line has been consumed, so it won't be ran again by
> 'git rebase --continue' is ran, even if the tests weren't fixed.
> 
> This commit introduces a new command 'test' equivalent to 'exec', except
> that it is automatically rescheduled in the todo list if it fails.
> 
> Signed-off-by: Paul Morelle <paul.morelle@gmail.com>

Would it not make more sense to add a command-line option (and a config
setting) to re-schedule failed `exec` commands? Like so:

-- snip --
diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
index a2ab68ed0632..a9ab009fdbca 100644
--- a/builtin/rebase--interactive.c
+++ b/builtin/rebase--interactive.c
@@ -192,6 +192,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "onto-name", &onto_name, N_("onto-name"), N_("onto name")),
 		OPT_STRING(0, "cmd", &cmd, N_("cmd"), N_("the command to run")),
 		OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto),
+		OPT_BOOL(0, "reschedule-failed-exec", &opts.reschedule_failed_exec,
+			 N_("automatically re-schedule any `exec` that fails")),
 		OPT_END()
 	};
 
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b3e5baec8a0..700cbc4e150e 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -104,6 +104,7 @@ struct rebase_options {
 	int rebase_merges, rebase_cousins;
 	char *strategy, *strategy_opts;
 	struct strbuf git_format_patch_opt;
+	int reschedule_failed_exec;
 };
 
 static int is_interactive(struct rebase_options *opts)
@@ -415,6 +416,8 @@ static int run_specific_rebase(struct rebase_options *opts)
 			argv_array_push(&child.args, opts->gpg_sign_opt);
 		if (opts->signoff)
 			argv_array_push(&child.args, "--signoff");
+		if (opts->reschedule_failed_exec)
+			argv_array_push(&child.args, "--reschedule-failed-exec");
 
 		status = run_command(&child);
 		goto finished_rebase;
@@ -903,6 +906,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 				   "strategy")),
 		OPT_BOOL(0, "root", &options.root,
 			 N_("rebase all reachable commits up to the root(s)")),
+		OPT_BOOL(0, "reschedule-failed-exec",
+			 &options.reschedule_failed_exec,
+			 N_("automatically re-schedule any `exec` that fails")),
 		OPT_END(),
 	};
 	int i;
@@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		break;
 	}
 
+	if (options.reschedule_failed_exec && !is_interactive(&options))
+		die(_("--reschedule-failed-exec requires an interactive rebase"));
+
 	if (options.git_am_opts.argc) {
 		/* all am options except -q are compatible only with --am */
 		for (i = options.git_am_opts.argc - 1; i >= 0; i--)
diff --git a/sequencer.c b/sequencer.c
index e1a4dd15f1a8..69bee63e116f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -158,6 +158,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
 static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
 static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
 static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
+static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
 
 static int git_sequencer_config(const char *k, const char *v, void *cb)
 {
@@ -2362,6 +2363,9 @@ static int read_populate_opts(struct replay_opts *opts)
 			opts->signoff = 1;
 		}
 
+		if (file_exists(rebase_path_reschedule_failed_exec()))
+			opts->reschedule_failed_exec = 1;
+
 		read_strategy_opts(opts, &buf);
 		strbuf_release(&buf);
 
@@ -2443,6 +2447,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
 		write_file(rebase_path_gpg_sign_opt(), "-S%s\n", opts->gpg_sign);
 	if (opts->signoff)
 		write_file(rebase_path_signoff(), "--signoff\n");
+	if (opts->reschedule_failed_exec)
+		write_file(rebase_path_reschedule_failed_exec(), "%s", "");
 
 	return 0;
 }
@@ -3586,9 +3592,10 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
 			*end_of_arg = saved;
 
 			/* Reread the todo file if it has changed. */
-			if (res)
-				; /* fall through */
-			else if (stat(get_todo_path(opts), &st))
+			if (res) {
+				if (opts->reschedule_failed_exec)
+					reschedule = 1;
+			} else if (stat(get_todo_path(opts), &st))
 				res = error_errno(_("could not stat '%s'"),
 						  get_todo_path(opts));
 			else if (match_stat_data(&todo_list->stat, &st)) {
diff --git a/sequencer.h b/sequencer.h
index 5071a73563f1..1f865dae2618 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -39,6 +39,7 @@ struct replay_opts {
 	int allow_empty_message;
 	int keep_redundant_commits;
 	int verbose;
+	int reschedule_failed_exec;
 
 	int mainline;
 
-- snap --

That would avoid your having to add a `--test` option accompanying the `--exec`
option (which is missing from your patch).

Ciao,
Johannes

> ---
>  Documentation/git-rebase.txt  |  9 ++++++---
>  rebase-interactive.c          |  1 +
>  sequencer.c                   | 23 +++++++++++++++--------
>  t/lib-rebase.sh               |  4 +++-
>  t/t3404-rebase-interactive.sh | 16 ++++++++++++++++
>  5 files changed, 41 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 80793bad8..c8f565637 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -693,8 +693,8 @@ $ git rebase -i -p --onto Q O
>  Reordering and editing commits usually creates untested intermediate
>  steps.  You may want to check that your history editing did not break
>  anything by running a test, or at least recompiling at intermediate
> -points in history by using the "exec" command (shortcut "x").  You may
> -do so by creating a todo list like this one:
> +points in history by using the "exec" command (shortcut "x") or the
> +"test" command.  You may do so by creating a todo list like this one:
>   -------------------------------------------
>  pick deadbee Implement feature XXX
> @@ -702,7 +702,7 @@ fixup f1a5c00 Fix to feature XXX
>  exec make
>  pick c0ffeee The oneline of the next commit
>  edit deadbab The oneline of the commit after
> -exec cd subdir; make test
> +test cd subdir; make test
>  ...
>  -------------------------------------------
>  @@ -715,6 +715,9 @@ in `$SHELL`, or the default shell if `$SHELL` is
> not set), so you can
>  use shell features (like "cd", ">", ";" ...). The command is run from
>  the root of the working tree.
>  +The "test" command does the same, but will remain in the todo list as
> +the next command, until it succeeds.
> +
>  ----------------------------------
>  $ git rebase -i --exec "make test"
>  ----------------------------------
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index 78f3263fc..4a408661d 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -14,6 +14,7 @@ void append_todo_help(unsigned edit_todo, unsigned
> keep_empty,
>  "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"
> +"   test <command> = same as exec command, but keep it in TODO if it
> fails\n"
>  "b, break = stop here (continue rebase later with 'git rebase
> --continue')\n"
>  "d, drop <commit> = remove commit\n"
>  "l, label <label> = label current HEAD with a name\n"
> diff --git a/sequencer.c b/sequencer.c
> index e1a4dd15f..c3dde6910 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1508,6 +1508,7 @@ enum todo_command {
>  	TODO_SQUASH,
>  	/* commands that do something else than handling a single commit */
>  	TODO_EXEC,
> +	TODO_TEST,
>  	TODO_BREAK,
>  	TODO_LABEL,
>  	TODO_RESET,
> @@ -1530,6 +1531,7 @@ static struct {
>  	{ 'f', "fixup" },
>  	{ 's', "squash" },
>  	{ 'x', "exec" },
> +	{ 0,   "test" },
>  	{ 'b', "break" },
>  	{ 'l', "label" },
>  	{ 't', "reset" },
> @@ -2072,7 +2074,7 @@ static int parse_insn_line(struct todo_item *item,
> const char *bol, char *eol)
>  			     command_to_string(item->command));
>   	if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
> -	    item->command == TODO_RESET) {
> +	    item->command == TODO_RESET || item->command == TODO_TEST) {
>  		item->commit = NULL;
>  		item->arg = bol;
>  		item->arg_len = (int)(eol - bol);
> @@ -3576,7 +3578,7 @@ static int pick_commits(struct todo_list
> *todo_list, struct replay_opts *opts)
>  						item->arg, item->arg_len, opts,
>  						res, to_amend);
>  			}
> -		} else if (item->command == TODO_EXEC) {
> +		} else if (item->command == TODO_EXEC || item->command == TODO_TEST) {
>  			char *end_of_arg = (char *)(item->arg + item->arg_len);
>  			int saved = *end_of_arg;
>  			struct stat st;
> @@ -3586,9 +3588,12 @@ static int pick_commits(struct todo_list
> *todo_list, struct replay_opts *opts)
>  			*end_of_arg = saved;
>   			/* Reread the todo file if it has changed. */
> -			if (res)
> +			if (res) {
>  				; /* fall through */
> -			else if (stat(get_todo_path(opts), &st))
> +				if (item->command == TODO_TEST) {
> +					reschedule = 1;
> +				}
> +			} else if (stat(get_todo_path(opts), &st))
>  				res = error_errno(_("could not stat '%s'"),
>  						  get_todo_path(opts));
>  			else if (match_stat_data(&todo_list->stat, &st)) {
> @@ -3622,10 +3627,12 @@ static int pick_commits(struct todo_list
> *todo_list, struct replay_opts *opts)
>  			return error(_("unknown command %d"), item->command);
>   		if (reschedule) {
> -			advise(_(rescheduled_advice),
> -			       get_item_line_length(todo_list,
> -						    todo_list->current),
> -			       get_item_line(todo_list, todo_list->current));
> +			if (item->command != TODO_TEST) {
> +				advise(_(rescheduled_advice),
> +				       get_item_line_length(todo_list,
> +							    todo_list->current),
> +				       get_item_line(todo_list, todo_list->current));
> +			}
>  			todo_list->current--;
>  			if (save_todo(todo_list, opts))
>  				return -1;
> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
> index 7ea30e500..7d36f00bd 100644
> --- a/t/lib-rebase.sh
> +++ b/t/lib-rebase.sh
> @@ -19,6 +19,8 @@
>  #
>  #   "exec_cmd_with_args" -- add an "exec cmd with args" line.
>  #
> +#   "test_cmd_with_args" -- add a "test cmd with args" line.
> +#
>  #   "#" -- Add a comment line.
>  #
>  #   ">" -- Add a blank line.
> @@ -49,7 +51,7 @@ set_fake_editor () {
>  		case $line in
>  		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
>  			action="$line";;
> -		exec_*|x_*|break|b)
> +		exec_*|x_*|break|b|test_*)
>  			echo "$line" | sed 's/_/ /g' >> "$1";;
>  		"#")
>  			echo '# comment' >> "$1";;
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 7a440e08d..14c60b14d 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1453,4 +1453,20 @@ test_expect_success 'valid author header when
> author contains single quote' '
>  	test_cmp expected actual
>  '
>  +test_expect_success 'rebase -i keeps test until it passes' '
> +	git checkout master &&
> +	(
> +	set_fake_editor &&
> +	FAKE_LINES="1 test_false 2 3 4 5" &&
> +	export FAKE_LINES &&
> +	test_must_fail git rebase -i A &&
> +	test_cmp_rev B HEAD &&
> +	test_must_fail git rebase --continue &&
> +	test_cmp_rev B HEAD &&
> +	FAKE_LINES="test_true 2 3 4" git rebase --edit-todo &&
> +	git rebase --continue
> +	) &&
> +	test_cmp_rev master HEAD
> +'
> +
>  test_done
> -- 
> 2.19.1
> 
> 

Re: [PATCH] rebase -i: introduce the 'test' command

[Paul Morelle]

Hi Johannes,

On 28/11/18 16:19, Johannes Schindelin wrote:
> Hi Paul,
>
> On Wed, 28 Nov 2018, Paul Morelle wrote:
>
>> The 'exec' command can be used to run tests on a set of commits,
>> interrupting on failing commits to let the user fix the tests.
>>
>> However, the 'exec' line has been consumed, so it won't be ran again by
>> 'git rebase --continue' is ran, even if the tests weren't fixed.
>>
>> This commit introduces a new command 'test' equivalent to 'exec', except
>> that it is automatically rescheduled in the todo list if it fails.
>>
>> Signed-off-by: Paul Morelle <paul.morelle@gmail.com>
> Would it not make more sense to add a command-line option (and a config
> setting) to re-schedule failed `exec` commands? Like so:

Your proposition would do in most cases, however it is not possible to
make a distinction between reschedulable and non-reschedulable commands.

Do you think that we can ignore the distinction between reschedulable
and non-reschedulable commands in the same script?
In this case, I would add some tests to your patch below, and propose
the result on this mailing list.

> -- snip --
> diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
> index a2ab68ed0632..a9ab009fdbca 100644
> --- a/builtin/rebase--interactive.c
> +++ b/builtin/rebase--interactive.c
> @@ -192,6 +192,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
>  		OPT_STRING(0, "onto-name", &onto_name, N_("onto-name"), N_("onto name")),
>  		OPT_STRING(0, "cmd", &cmd, N_("cmd"), N_("the command to run")),
>  		OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto),
> +		OPT_BOOL(0, "reschedule-failed-exec", &opts.reschedule_failed_exec,
> +			 N_("automatically re-schedule any `exec` that fails")),
>  		OPT_END()
>  	};
>  
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 5b3e5baec8a0..700cbc4e150e 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -104,6 +104,7 @@ struct rebase_options {
>  	int rebase_merges, rebase_cousins;
>  	char *strategy, *strategy_opts;
>  	struct strbuf git_format_patch_opt;
> +	int reschedule_failed_exec;
>  };
>  
>  static int is_interactive(struct rebase_options *opts)
> @@ -415,6 +416,8 @@ static int run_specific_rebase(struct rebase_options *opts)
>  			argv_array_push(&child.args, opts->gpg_sign_opt);
>  		if (opts->signoff)
>  			argv_array_push(&child.args, "--signoff");
> +		if (opts->reschedule_failed_exec)
> +			argv_array_push(&child.args, "--reschedule-failed-exec");
>  
>  		status = run_command(&child);
>  		goto finished_rebase;
> @@ -903,6 +906,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  				   "strategy")),
>  		OPT_BOOL(0, "root", &options.root,
>  			 N_("rebase all reachable commits up to the root(s)")),
> +		OPT_BOOL(0, "reschedule-failed-exec",
> +			 &options.reschedule_failed_exec,
> +			 N_("automatically re-schedule any `exec` that fails")),
>  		OPT_END(),
>  	};
>  	int i;
> @@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		break;
>  	}
>  
> +	if (options.reschedule_failed_exec && !is_interactive(&options))
> +		die(_("--reschedule-failed-exec requires an interactive rebase"));
> +
>  	if (options.git_am_opts.argc) {
>  		/* all am options except -q are compatible only with --am */
>  		for (i = options.git_am_opts.argc - 1; i >= 0; i--)
> diff --git a/sequencer.c b/sequencer.c
> index e1a4dd15f1a8..69bee63e116f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -158,6 +158,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
>  static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
>  static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
>  static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
> +static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
>  
>  static int git_sequencer_config(const char *k, const char *v, void *cb)
>  {
> @@ -2362,6 +2363,9 @@ static int read_populate_opts(struct replay_opts *opts)
>  			opts->signoff = 1;
>  		}
>  
> +		if (file_exists(rebase_path_reschedule_failed_exec()))
> +			opts->reschedule_failed_exec = 1;
> +
>  		read_strategy_opts(opts, &buf);
>  		strbuf_release(&buf);
>  
> @@ -2443,6 +2447,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
>  		write_file(rebase_path_gpg_sign_opt(), "-S%s\n", opts->gpg_sign);
>  	if (opts->signoff)
>  		write_file(rebase_path_signoff(), "--signoff\n");
> +	if (opts->reschedule_failed_exec)
> +		write_file(rebase_path_reschedule_failed_exec(), "%s", "");
>  
>  	return 0;
>  }
> @@ -3586,9 +3592,10 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
>  			*end_of_arg = saved;
>  
>  			/* Reread the todo file if it has changed. */
> -			if (res)
> -				; /* fall through */
> -			else if (stat(get_todo_path(opts), &st))
> +			if (res) {
> +				if (opts->reschedule_failed_exec)
> +					reschedule = 1;
> +			} else if (stat(get_todo_path(opts), &st))
>  				res = error_errno(_("could not stat '%s'"),
>  						  get_todo_path(opts));
>  			else if (match_stat_data(&todo_list->stat, &st)) {
> diff --git a/sequencer.h b/sequencer.h
> index 5071a73563f1..1f865dae2618 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -39,6 +39,7 @@ struct replay_opts {
>  	int allow_empty_message;
>  	int keep_redundant_commits;
>  	int verbose;
> +	int reschedule_failed_exec;
>  
>  	int mainline;
>  
> -- snap --
>
> That would avoid your having to add a `--test` option accompanying the `--exec`
> option (which is missing from your patch).

You are right about the missing `--test` option, which I could also add
to a new version of this patch.

Cheers,
Paul

> Ciao,
> Johannes
>
>> ---
>>  Documentation/git-rebase.txt  |  9 ++++++---
>>  rebase-interactive.c          |  1 +
>>  sequencer.c                   | 23 +++++++++++++++--------
>>  t/lib-rebase.sh               |  4 +++-
>>  t/t3404-rebase-interactive.sh | 16 ++++++++++++++++
>>  5 files changed, 41 insertions(+), 12 deletions(-)
>>
>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>> index 80793bad8..c8f565637 100644
>> --- a/Documentation/git-rebase.txt
>> +++ b/Documentation/git-rebase.txt
>> @@ -693,8 +693,8 @@ $ git rebase -i -p --onto Q O
>>  Reordering and editing commits usually creates untested intermediate
>>  steps.  You may want to check that your history editing did not break
>>  anything by running a test, or at least recompiling at intermediate
>> -points in history by using the "exec" command (shortcut "x").  You may
>> -do so by creating a todo list like this one:
>> +points in history by using the "exec" command (shortcut "x") or the
>> +"test" command.  You may do so by creating a todo list like this one:
>>   -------------------------------------------
>>  pick deadbee Implement feature XXX
>> @@ -702,7 +702,7 @@ fixup f1a5c00 Fix to feature XXX
>>  exec make
>>  pick c0ffeee The oneline of the next commit
>>  edit deadbab The oneline of the commit after
>> -exec cd subdir; make test
>> +test cd subdir; make test
>>  ...
>>  -------------------------------------------
>>  @@ -715,6 +715,9 @@ in `$SHELL`, or the default shell if `$SHELL` is
>> not set), so you can
>>  use shell features (like "cd", ">", ";" ...). The command is run from
>>  the root of the working tree.
>>  +The "test" command does the same, but will remain in the todo list as
>> +the next command, until it succeeds.
>> +
>>  ----------------------------------
>>  $ git rebase -i --exec "make test"
>>  ----------------------------------
>> diff --git a/rebase-interactive.c b/rebase-interactive.c
>> index 78f3263fc..4a408661d 100644
>> --- a/rebase-interactive.c
>> +++ b/rebase-interactive.c
>> @@ -14,6 +14,7 @@ void append_todo_help(unsigned edit_todo, unsigned
>> keep_empty,
>>  "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"
>> +"   test <command> = same as exec command, but keep it in TODO if it
>> fails\n"
>>  "b, break = stop here (continue rebase later with 'git rebase
>> --continue')\n"
>>  "d, drop <commit> = remove commit\n"
>>  "l, label <label> = label current HEAD with a name\n"
>> diff --git a/sequencer.c b/sequencer.c
>> index e1a4dd15f..c3dde6910 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -1508,6 +1508,7 @@ enum todo_command {
>>  	TODO_SQUASH,
>>  	/* commands that do something else than handling a single commit */
>>  	TODO_EXEC,
>> +	TODO_TEST,
>>  	TODO_BREAK,
>>  	TODO_LABEL,
>>  	TODO_RESET,
>> @@ -1530,6 +1531,7 @@ static struct {
>>  	{ 'f', "fixup" },
>>  	{ 's', "squash" },
>>  	{ 'x', "exec" },
>> +	{ 0,   "test" },
>>  	{ 'b', "break" },
>>  	{ 'l', "label" },
>>  	{ 't', "reset" },
>> @@ -2072,7 +2074,7 @@ static int parse_insn_line(struct todo_item *item,
>> const char *bol, char *eol)
>>  			     command_to_string(item->command));
>>   	if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
>> -	    item->command == TODO_RESET) {
>> +	    item->command == TODO_RESET || item->command == TODO_TEST) {
>>  		item->commit = NULL;
>>  		item->arg = bol;
>>  		item->arg_len = (int)(eol - bol);
>> @@ -3576,7 +3578,7 @@ static int pick_commits(struct todo_list
>> *todo_list, struct replay_opts *opts)
>>  						item->arg, item->arg_len, opts,
>>  						res, to_amend);
>>  			}
>> -		} else if (item->command == TODO_EXEC) {
>> +		} else if (item->command == TODO_EXEC || item->command == TODO_TEST) {
>>  			char *end_of_arg = (char *)(item->arg + item->arg_len);
>>  			int saved = *end_of_arg;
>>  			struct stat st;
>> @@ -3586,9 +3588,12 @@ static int pick_commits(struct todo_list
>> *todo_list, struct replay_opts *opts)
>>  			*end_of_arg = saved;
>>   			/* Reread the todo file if it has changed. */
>> -			if (res)
>> +			if (res) {
>>  				; /* fall through */
>> -			else if (stat(get_todo_path(opts), &st))
>> +				if (item->command == TODO_TEST) {
>> +					reschedule = 1;
>> +				}
>> +			} else if (stat(get_todo_path(opts), &st))
>>  				res = error_errno(_("could not stat '%s'"),
>>  						  get_todo_path(opts));
>>  			else if (match_stat_data(&todo_list->stat, &st)) {
>> @@ -3622,10 +3627,12 @@ static int pick_commits(struct todo_list
>> *todo_list, struct replay_opts *opts)
>>  			return error(_("unknown command %d"), item->command);
>>   		if (reschedule) {
>> -			advise(_(rescheduled_advice),
>> -			       get_item_line_length(todo_list,
>> -						    todo_list->current),
>> -			       get_item_line(todo_list, todo_list->current));
>> +			if (item->command != TODO_TEST) {
>> +				advise(_(rescheduled_advice),
>> +				       get_item_line_length(todo_list,
>> +							    todo_list->current),
>> +				       get_item_line(todo_list, todo_list->current));
>> +			}
>>  			todo_list->current--;
>>  			if (save_todo(todo_list, opts))
>>  				return -1;
>> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
>> index 7ea30e500..7d36f00bd 100644
>> --- a/t/lib-rebase.sh
>> +++ b/t/lib-rebase.sh
>> @@ -19,6 +19,8 @@
>>  #
>>  #   "exec_cmd_with_args" -- add an "exec cmd with args" line.
>>  #
>> +#   "test_cmd_with_args" -- add a "test cmd with args" line.
>> +#
>>  #   "#" -- Add a comment line.
>>  #
>>  #   ">" -- Add a blank line.
>> @@ -49,7 +51,7 @@ set_fake_editor () {
>>  		case $line in
>>  		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
>>  			action="$line";;
>> -		exec_*|x_*|break|b)
>> +		exec_*|x_*|break|b|test_*)
>>  			echo "$line" | sed 's/_/ /g' >> "$1";;
>>  		"#")
>>  			echo '# comment' >> "$1";;
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index 7a440e08d..14c60b14d 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -1453,4 +1453,20 @@ test_expect_success 'valid author header when
>> author contains single quote' '
>>  	test_cmp expected actual
>>  '
>>  +test_expect_success 'rebase -i keeps test until it passes' '
>> +	git checkout master &&
>> +	(
>> +	set_fake_editor &&
>> +	FAKE_LINES="1 test_false 2 3 4 5" &&
>> +	export FAKE_LINES &&
>> +	test_must_fail git rebase -i A &&
>> +	test_cmp_rev B HEAD &&
>> +	test_must_fail git rebase --continue &&
>> +	test_cmp_rev B HEAD &&
>> +	FAKE_LINES="test_true 2 3 4" git rebase --edit-todo &&
>> +	git rebase --continue
>> +	) &&
>> +	test_cmp_rev master HEAD
>> +'
>> +
>>  test_done
>> -- 
>> 2.19.1
>>
>>

Re: [PATCH] rebase -i: introduce the 'test' command

[Johannes Schindelin]

Hi Paul,

On Wed, 28 Nov 2018, Paul Morelle wrote:

> On 28/11/18 16:19, Johannes Schindelin wrote:
> >
> > On Wed, 28 Nov 2018, Paul Morelle wrote:
> >
> >> The 'exec' command can be used to run tests on a set of commits,
> >> interrupting on failing commits to let the user fix the tests.
> >>
> >> However, the 'exec' line has been consumed, so it won't be ran again by
> >> 'git rebase --continue' is ran, even if the tests weren't fixed.
> >>
> >> This commit introduces a new command 'test' equivalent to 'exec', except
> >> that it is automatically rescheduled in the todo list if it fails.
> >>
> >> Signed-off-by: Paul Morelle <paul.morelle@gmail.com>
> > Would it not make more sense to add a command-line option (and a config
> > setting) to re-schedule failed `exec` commands? Like so:
> 
> Your proposition would do in most cases, however it is not possible to
> make a distinction between reschedulable and non-reschedulable commands.

True. But I don't think that's so terrible.

What I think is something to avoid is two commands that do something very,
very similar, but with two very, very different names.

In reality, I think that it would even make sense to change the default to
reschedule failed `exec` commands. Which is why I suggested to also add a
config option.

> Do you think that we can ignore the distinction between reschedulable
> and non-reschedulable commands in the same script?

Yes, I think that there *should not* be any distinction. It would just
make it harder to use the feature (users would have to keep in mind that
one version reschedules, one version does not).

> In this case, I would add some tests to your patch below, and propose
> the result on this mailing list.

I already added a test... See the reschedule-failed-exec branch on
https://github.com/dscho/git.

And I had another idea. To make this feature more useful for *myself*, I
would like to introduce the `-X` option as a shortcut for
`--reschedule-failed-exec -x`, e.g.

	git rebase -X "make -j15 test" <base>

What do you think?
Johannes

> 
> > -- snip --
> > diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
> > index a2ab68ed0632..a9ab009fdbca 100644
> > --- a/builtin/rebase--interactive.c
> > +++ b/builtin/rebase--interactive.c
> > @@ -192,6 +192,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
> >  		OPT_STRING(0, "onto-name", &onto_name, N_("onto-name"), N_("onto name")),
> >  		OPT_STRING(0, "cmd", &cmd, N_("cmd"), N_("the command to run")),
> >  		OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto),
> > +		OPT_BOOL(0, "reschedule-failed-exec", &opts.reschedule_failed_exec,
> > +			 N_("automatically re-schedule any `exec` that fails")),
> >  		OPT_END()
> >  	};
> >  
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 5b3e5baec8a0..700cbc4e150e 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -104,6 +104,7 @@ struct rebase_options {
> >  	int rebase_merges, rebase_cousins;
> >  	char *strategy, *strategy_opts;
> >  	struct strbuf git_format_patch_opt;
> > +	int reschedule_failed_exec;
> >  };
> >  
> >  static int is_interactive(struct rebase_options *opts)
> > @@ -415,6 +416,8 @@ static int run_specific_rebase(struct rebase_options *opts)
> >  			argv_array_push(&child.args, opts->gpg_sign_opt);
> >  		if (opts->signoff)
> >  			argv_array_push(&child.args, "--signoff");
> > +		if (opts->reschedule_failed_exec)
> > +			argv_array_push(&child.args, "--reschedule-failed-exec");
> >  
> >  		status = run_command(&child);
> >  		goto finished_rebase;
> > @@ -903,6 +906,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >  				   "strategy")),
> >  		OPT_BOOL(0, "root", &options.root,
> >  			 N_("rebase all reachable commits up to the root(s)")),
> > +		OPT_BOOL(0, "reschedule-failed-exec",
> > +			 &options.reschedule_failed_exec,
> > +			 N_("automatically re-schedule any `exec` that fails")),
> >  		OPT_END(),
> >  	};
> >  	int i;
> > @@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >  		break;
> >  	}
> >  
> > +	if (options.reschedule_failed_exec && !is_interactive(&options))
> > +		die(_("--reschedule-failed-exec requires an interactive rebase"));
> > +
> >  	if (options.git_am_opts.argc) {
> >  		/* all am options except -q are compatible only with --am */
> >  		for (i = options.git_am_opts.argc - 1; i >= 0; i--)
> > diff --git a/sequencer.c b/sequencer.c
> > index e1a4dd15f1a8..69bee63e116f 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -158,6 +158,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
> >  static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
> >  static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
> >  static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
> > +static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
> >  
> >  static int git_sequencer_config(const char *k, const char *v, void *cb)
> >  {
> > @@ -2362,6 +2363,9 @@ static int read_populate_opts(struct replay_opts *opts)
> >  			opts->signoff = 1;
> >  		}
> >  
> > +		if (file_exists(rebase_path_reschedule_failed_exec()))
> > +			opts->reschedule_failed_exec = 1;
> > +
> >  		read_strategy_opts(opts, &buf);
> >  		strbuf_release(&buf);
> >  
> > @@ -2443,6 +2447,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
> >  		write_file(rebase_path_gpg_sign_opt(), "-S%s\n", opts->gpg_sign);
> >  	if (opts->signoff)
> >  		write_file(rebase_path_signoff(), "--signoff\n");
> > +	if (opts->reschedule_failed_exec)
> > +		write_file(rebase_path_reschedule_failed_exec(), "%s", "");
> >  
> >  	return 0;
> >  }
> > @@ -3586,9 +3592,10 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
> >  			*end_of_arg = saved;
> >  
> >  			/* Reread the todo file if it has changed. */
> > -			if (res)
> > -				; /* fall through */
> > -			else if (stat(get_todo_path(opts), &st))
> > +			if (res) {
> > +				if (opts->reschedule_failed_exec)
> > +					reschedule = 1;
> > +			} else if (stat(get_todo_path(opts), &st))
> >  				res = error_errno(_("could not stat '%s'"),
> >  						  get_todo_path(opts));
> >  			else if (match_stat_data(&todo_list->stat, &st)) {
> > diff --git a/sequencer.h b/sequencer.h
> > index 5071a73563f1..1f865dae2618 100644
> > --- a/sequencer.h
> > +++ b/sequencer.h
> > @@ -39,6 +39,7 @@ struct replay_opts {
> >  	int allow_empty_message;
> >  	int keep_redundant_commits;
> >  	int verbose;
> > +	int reschedule_failed_exec;
> >  
> >  	int mainline;
> >  
> > -- snap --
> >
> > That would avoid your having to add a `--test` option accompanying the `--exec`
> > option (which is missing from your patch).
> 
> You are right about the missing `--test` option, which I could also add
> to a new version of this patch.
> 
> Cheers,
> Paul
> 
> > Ciao,
> > Johannes
> >
> >> ---
> >>  Documentation/git-rebase.txt  |  9 ++++++---
> >>  rebase-interactive.c          |  1 +
> >>  sequencer.c                   | 23 +++++++++++++++--------
> >>  t/lib-rebase.sh               |  4 +++-
> >>  t/t3404-rebase-interactive.sh | 16 ++++++++++++++++
> >>  5 files changed, 41 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> >> index 80793bad8..c8f565637 100644
> >> --- a/Documentation/git-rebase.txt
> >> +++ b/Documentation/git-rebase.txt
> >> @@ -693,8 +693,8 @@ $ git rebase -i -p --onto Q O
> >>  Reordering and editing commits usually creates untested intermediate
> >>  steps.  You may want to check that your history editing did not break
> >>  anything by running a test, or at least recompiling at intermediate
> >> -points in history by using the "exec" command (shortcut "x").  You may
> >> -do so by creating a todo list like this one:
> >> +points in history by using the "exec" command (shortcut "x") or the
> >> +"test" command.  You may do so by creating a todo list like this one:
> >>   -------------------------------------------
> >>  pick deadbee Implement feature XXX
> >> @@ -702,7 +702,7 @@ fixup f1a5c00 Fix to feature XXX
> >>  exec make
> >>  pick c0ffeee The oneline of the next commit
> >>  edit deadbab The oneline of the commit after
> >> -exec cd subdir; make test
> >> +test cd subdir; make test
> >>  ...
> >>  -------------------------------------------
> >>  @@ -715,6 +715,9 @@ in `$SHELL`, or the default shell if `$SHELL` is
> >> not set), so you can
> >>  use shell features (like "cd", ">", ";" ...). The command is run from
> >>  the root of the working tree.
> >>  +The "test" command does the same, but will remain in the todo list as
> >> +the next command, until it succeeds.
> >> +
> >>  ----------------------------------
> >>  $ git rebase -i --exec "make test"
> >>  ----------------------------------
> >> diff --git a/rebase-interactive.c b/rebase-interactive.c
> >> index 78f3263fc..4a408661d 100644
> >> --- a/rebase-interactive.c
> >> +++ b/rebase-interactive.c
> >> @@ -14,6 +14,7 @@ void append_todo_help(unsigned edit_todo, unsigned
> >> keep_empty,
> >>  "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"
> >> +"   test <command> = same as exec command, but keep it in TODO if it
> >> fails\n"
> >>  "b, break = stop here (continue rebase later with 'git rebase
> >> --continue')\n"
> >>  "d, drop <commit> = remove commit\n"
> >>  "l, label <label> = label current HEAD with a name\n"
> >> diff --git a/sequencer.c b/sequencer.c
> >> index e1a4dd15f..c3dde6910 100644
> >> --- a/sequencer.c
> >> +++ b/sequencer.c
> >> @@ -1508,6 +1508,7 @@ enum todo_command {
> >>  	TODO_SQUASH,
> >>  	/* commands that do something else than handling a single commit */
> >>  	TODO_EXEC,
> >> +	TODO_TEST,
> >>  	TODO_BREAK,
> >>  	TODO_LABEL,
> >>  	TODO_RESET,
> >> @@ -1530,6 +1531,7 @@ static struct {
> >>  	{ 'f', "fixup" },
> >>  	{ 's', "squash" },
> >>  	{ 'x', "exec" },
> >> +	{ 0,   "test" },
> >>  	{ 'b', "break" },
> >>  	{ 'l', "label" },
> >>  	{ 't', "reset" },
> >> @@ -2072,7 +2074,7 @@ static int parse_insn_line(struct todo_item *item,
> >> const char *bol, char *eol)
> >>  			     command_to_string(item->command));
> >>   	if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
> >> -	    item->command == TODO_RESET) {
> >> +	    item->command == TODO_RESET || item->command == TODO_TEST) {
> >>  		item->commit = NULL;
> >>  		item->arg = bol;
> >>  		item->arg_len = (int)(eol - bol);
> >> @@ -3576,7 +3578,7 @@ static int pick_commits(struct todo_list
> >> *todo_list, struct replay_opts *opts)
> >>  						item->arg, item->arg_len, opts,
> >>  						res, to_amend);
> >>  			}
> >> -		} else if (item->command == TODO_EXEC) {
> >> +		} else if (item->command == TODO_EXEC || item->command == TODO_TEST) {
> >>  			char *end_of_arg = (char *)(item->arg + item->arg_len);
> >>  			int saved = *end_of_arg;
> >>  			struct stat st;
> >> @@ -3586,9 +3588,12 @@ static int pick_commits(struct todo_list
> >> *todo_list, struct replay_opts *opts)
> >>  			*end_of_arg = saved;
> >>   			/* Reread the todo file if it has changed. */
> >> -			if (res)
> >> +			if (res) {
> >>  				; /* fall through */
> >> -			else if (stat(get_todo_path(opts), &st))
> >> +				if (item->command == TODO_TEST) {
> >> +					reschedule = 1;
> >> +				}
> >> +			} else if (stat(get_todo_path(opts), &st))
> >>  				res = error_errno(_("could not stat '%s'"),
> >>  						  get_todo_path(opts));
> >>  			else if (match_stat_data(&todo_list->stat, &st)) {
> >> @@ -3622,10 +3627,12 @@ static int pick_commits(struct todo_list
> >> *todo_list, struct replay_opts *opts)
> >>  			return error(_("unknown command %d"), item->command);
> >>   		if (reschedule) {
> >> -			advise(_(rescheduled_advice),
> >> -			       get_item_line_length(todo_list,
> >> -						    todo_list->current),
> >> -			       get_item_line(todo_list, todo_list->current));
> >> +			if (item->command != TODO_TEST) {
> >> +				advise(_(rescheduled_advice),
> >> +				       get_item_line_length(todo_list,
> >> +							    todo_list->current),
> >> +				       get_item_line(todo_list, todo_list->current));
> >> +			}
> >>  			todo_list->current--;
> >>  			if (save_todo(todo_list, opts))
> >>  				return -1;
> >> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
> >> index 7ea30e500..7d36f00bd 100644
> >> --- a/t/lib-rebase.sh
> >> +++ b/t/lib-rebase.sh
> >> @@ -19,6 +19,8 @@
> >>  #
> >>  #   "exec_cmd_with_args" -- add an "exec cmd with args" line.
> >>  #
> >> +#   "test_cmd_with_args" -- add a "test cmd with args" line.
> >> +#
> >>  #   "#" -- Add a comment line.
> >>  #
> >>  #   ">" -- Add a blank line.
> >> @@ -49,7 +51,7 @@ set_fake_editor () {
> >>  		case $line in
> >>  		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
> >>  			action="$line";;
> >> -		exec_*|x_*|break|b)
> >> +		exec_*|x_*|break|b|test_*)
> >>  			echo "$line" | sed 's/_/ /g' >> "$1";;
> >>  		"#")
> >>  			echo '# comment' >> "$1";;
> >> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> >> index 7a440e08d..14c60b14d 100755
> >> --- a/t/t3404-rebase-interactive.sh
> >> +++ b/t/t3404-rebase-interactive.sh
> >> @@ -1453,4 +1453,20 @@ test_expect_success 'valid author header when
> >> author contains single quote' '
> >>  	test_cmp expected actual
> >>  '
> >>  +test_expect_success 'rebase -i keeps test until it passes' '
> >> +	git checkout master &&
> >> +	(
> >> +	set_fake_editor &&
> >> +	FAKE_LINES="1 test_false 2 3 4 5" &&
> >> +	export FAKE_LINES &&
> >> +	test_must_fail git rebase -i A &&
> >> +	test_cmp_rev B HEAD &&
> >> +	test_must_fail git rebase --continue &&
> >> +	test_cmp_rev B HEAD &&
> >> +	FAKE_LINES="test_true 2 3 4" git rebase --edit-todo &&
> >> +	git rebase --continue
> >> +	) &&
> >> +	test_cmp_rev master HEAD
> >> +'
> >> +
> >>  test_done
> >> -- 
> >> 2.19.1
> >>
> >>
> 

Re: [PATCH] rebase -i: introduce the 'test' command

[Johannes Schindelin]

Hi Paul,

On Thu, 29 Nov 2018, Johannes Schindelin wrote:

> I already added a test... See the reschedule-failed-exec branch on
> https://github.com/dscho/git.

And now I put up a proper Pull Request, to be submitted via GitGitGadget
right after Git v2.20.0 will be released (technically, we are in a
feature freeze period right now, I just could no resist, as I found your
reasoning for rescheduling failed `exec` commands compelling, and there
were no `rebase` bugs for me to fix).

https://github.com/gitgitgadget/git/pull/90

Ciao,
Johannes

Re: [PATCH] rebase -i: introduce the 'test' command

[Jeff King]

On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote:

> > > Would it not make more sense to add a command-line option (and a config
> > > setting) to re-schedule failed `exec` commands? Like so:
> > 
> > Your proposition would do in most cases, however it is not possible to
> > make a distinction between reschedulable and non-reschedulable commands.
> 
> True. But I don't think that's so terrible.
> 
> What I think is something to avoid is two commands that do something very,
> very similar, but with two very, very different names.
> 
> In reality, I think that it would even make sense to change the default to
> reschedule failed `exec` commands. Which is why I suggested to also add a
> config option.

I sometimes add "x false" to the top of the todo list to stop and create
new commits before the first one. That would be awkward if I could never
get past that line. However, I think elsewhere a "pause" line has been
discussed, which would serve the same purpose.

I wonder how often this kind of "yes, I know it fails, but keep going
anyway" situation would come up. And what the interface is like for
getting past it. E.g., what if you fixed a bunch of stuff but your tests
still fail? You may not want to abandon the changes you've made, but you
need to "rebase --continue" to move forward. I encounter this often when
the correct fix is actually in an earlier commit than the one that
yields the test failure. You can't rewind an interactive rebase, so I
complete and restart it, adding an "e"dit at the earlier commit.

How would I move past the test that fails to continue? I guess "git
rebase --edit-todo" and then manually remove it (and any other remaining
test lines)?

That's not too bad, but I wonder if people would find it more awkward
than the current way (which is to just "rebase --continue" until you get
to the end).

I dunno. I am not sure if I am for or against changing the default, so
these are just some musings. :)

-Peff

Re: [PATCH] rebase -i: introduce the 'test' command

[Eric Sunshine]

On Sat, Dec 1, 2018 at 3:02 PM Jeff King <peff@peff.net> wrote:
> On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote:
> > In reality, I think that it would even make sense to change the default to
> > reschedule failed `exec` commands. Which is why I suggested to also add a
> > config option.
>
> I sometimes add "x false" to the top of the todo list to stop and create
> new commits before the first one. That would be awkward if I could never
> get past that line. However, I think elsewhere a "pause" line has been
> discussed, which would serve the same purpose.

Are you thinking of the "break" command (not "pause") which Dscho
already added[1]?

[1]: 71f82465b1 (rebase -i: introduce the 'break' command, 2018-10-12)

Re: [PATCH] rebase -i: introduce the 'test' command

[Jeff King]

On Sat, Dec 01, 2018 at 09:28:47PM -0500, Eric Sunshine wrote:

> On Sat, Dec 1, 2018 at 3:02 PM Jeff King <peff@peff.net> wrote:
> > On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote:
> > > In reality, I think that it would even make sense to change the default to
> > > reschedule failed `exec` commands. Which is why I suggested to also add a
> > > config option.
> >
> > I sometimes add "x false" to the top of the todo list to stop and create
> > new commits before the first one. That would be awkward if I could never
> > get past that line. However, I think elsewhere a "pause" line has been
> > discussed, which would serve the same purpose.
> 
> Are you thinking of the "break" command (not "pause") which Dscho
> already added[1]?
> 
> [1]: 71f82465b1 (rebase -i: introduce the 'break' command, 2018-10-12)

Yes, thanks (as you can see, I haven't actually used it yet ;) ).

-Peff

Re: [PATCH] rebase -i: introduce the 'test' command

[Johannes Schindelin]

Hi Peff,

On Sat, 1 Dec 2018, Jeff King wrote:

> On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote:
> 
> > > > Would it not make more sense to add a command-line option (and a config
> > > > setting) to re-schedule failed `exec` commands? Like so:
> > > 
> > > Your proposition would do in most cases, however it is not possible to
> > > make a distinction between reschedulable and non-reschedulable commands.
> > 
> > True. But I don't think that's so terrible.
> > 
> > What I think is something to avoid is two commands that do something very,
> > very similar, but with two very, very different names.
> > 
> > In reality, I think that it would even make sense to change the default to
> > reschedule failed `exec` commands. Which is why I suggested to also add a
> > config option.
> 
> I sometimes add "x false" to the top of the todo list to stop and create
> new commits before the first one. That would be awkward if I could never
> get past that line. However, I think elsewhere a "pause" line has been
> discussed, which would serve the same purpose.

Yep, `break`, as Eric pointed out.

After all, you did not really want a command to fail, you just wanted the
interactive rebase to give you a break.

> I wonder how often this kind of "yes, I know it fails, but keep going
> anyway" situation would come up. And what the interface is like for
> getting past it. E.g., what if you fixed a bunch of stuff but your tests
> still fail? You may not want to abandon the changes you've made, but you
> need to "rebase --continue" to move forward. I encounter this often when
> the correct fix is actually in an earlier commit than the one that
> yields the test failure. You can't rewind an interactive rebase, so I
> complete and restart it, adding an "e"dit at the earlier commit.
> 
> How would I move past the test that fails to continue? I guess "git
> rebase --edit-todo" and then manually remove it (and any other remaining
> test lines)?

Yes, the current way would be to use `git rebase --edit-todo`.

> That's not too bad, but I wonder if people would find it more awkward
> than the current way (which is to just "rebase --continue" until you get
> to the end).
> 
> I dunno. I am not sure if I am for or against changing the default, so
> these are just some musings. :)

It's good that you chimed in with your side of things. If you missed the
`break` command, so will many others have missed it. And continue to miss
it.

Besides, Junio mentioned elsewhere that he is in the camp of people who
wait for enough users to complain why some config option isn't the default
to actually change the default.

So... I guess we'll leave the default where it is for now.

Ciao,
Dscho

Re: [PATCH] rebase -i: introduce the 'test' command

[Phillip Wood]

On 01/12/2018 20:02, Jeff King wrote:
> On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote:
> 
>>>> Would it not make more sense to add a command-line option (and a config
>>>> setting) to re-schedule failed `exec` commands? Like so:
>>>
>>> Your proposition would do in most cases, however it is not possible to
>>> make a distinction between reschedulable and non-reschedulable commands.
>>
>> True. But I don't think that's so terrible.
>>
>> What I think is something to avoid is two commands that do something very,
>> very similar, but with two very, very different names.
>>
>> In reality, I think that it would even make sense to change the default to
>> reschedule failed `exec` commands. Which is why I suggested to also add a
>> config option.
> 
> I sometimes add "x false" to the top of the todo list to stop and create
> new commits before the first one. That would be awkward if I could never
> get past that line. However, I think elsewhere a "pause" line has been
> discussed, which would serve the same purpose.
> 
> I wonder how often this kind of "yes, I know it fails, but keep going
> anyway" situation would come up. And what the interface is like for
> getting past it. E.g., what if you fixed a bunch of stuff but your tests
> still fail? You may not want to abandon the changes you've made, but you
> need to "rebase --continue" to move forward. I encounter this often when
> the correct fix is actually in an earlier commit than the one that
> yields the test failure. You can't rewind an interactive rebase, so I
> complete and restart it, adding an "e"dit at the earlier commit.
> 
> How would I move past the test that fails to continue? I guess "git
> rebase --edit-todo" and then manually remove it (and any other remaining
> test lines)?

Perhaps we could teach git rebase --skip to skip a rescheduled command, 
it could be useful if people want to skip rescheduled picks as well 
(though I don't think I've ever had that happen in the wild). I can see 
myself turning on the rescheduling config setting but occasionally 
wanting to be able to skip over the rescheduled exec command.


Best Wishes

Phillip

> That's not too bad, but I wonder if people would find it more awkward
> than the current way (which is to just "rebase --continue" until you get
> to the end).
> 
> I dunno. I am not sure if I am for or against changing the default, so
> these are just some musings. :)
> 
> -Peff
> 

Re: [PATCH] rebase -i: introduce the 'test' command

[Luc Van Oostenryck]

On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote:
> On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote:
> 
> > > > Would it not make more sense to add a command-line option (and a config
> > > > setting) to re-schedule failed `exec` commands? Like so:
> > > 
> > > Your proposition would do in most cases, however it is not possible to
> > > make a distinction between reschedulable and non-reschedulable commands.
> > 
> > True. But I don't think that's so terrible.
> > 
> > What I think is something to avoid is two commands that do something very,
> > very similar, but with two very, very different names.
> > 
> > In reality, I think that it would even make sense to change the default to
> > reschedule failed `exec` commands. Which is why I suggested to also add a
> > config option.
> 
> I sometimes add "x false" to the top of the todo list to stop and create
> new commits before the first one. That would be awkward if I could never
> get past that line. However, I think elsewhere a "pause" line has been
> discussed, which would serve the same purpose.
> 
> I wonder how often this kind of "yes, I know it fails, but keep going
> anyway" situation would come up. And what the interface is like for
> getting past it. E.g., what if you fixed a bunch of stuff but your tests
> still fail? You may not want to abandon the changes you've made, but you
> need to "rebase --continue" to move forward. I encounter this often when
> the correct fix is actually in an earlier commit than the one that
> yields the test failure. You can't rewind an interactive rebase, so I
> complete and restart it, adding an "e"dit at the earlier commit.

In this sort of situation, I often whish to be able to do nested rebases.
Even more because it happen relatively often that I forget that I'm
working in a rebase and not on the head, and then it's quite natural
to me to type things like 'git rebase -i @^^^' while already rebasing.
But I suppose this has already been discussed.

-- Luc

Re: [PATCH] rebase -i: introduce the 'test' command

[Duy Nguyen]

On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote:
> I sometimes add "x false" to the top of the todo list to stop and create
> new commits before the first one.

And here I've been doing the same by "edit" the first commit, add a
new commit then reorder them in the second interactive rebase :P

This made me look at git-rebase.txt to really learn about interactive
rebase. I think the interactive rebase section could use some
improvements. Its style looks.. umm.. more story telling than a
reference. Perhaps something like this to at least highlight the
commands.

-- 8< --
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 80793bad8d..c569b3370b 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -637,22 +637,22 @@ The oneline descriptions are purely for your pleasure; 'git rebase' will
 not look at them but at the commit names ("deadbee" and "fa1afe1" in this
 example), so do not delete or edit the names.
 
-By replacing the command "pick" with the command "edit", you can tell
+By replacing the command `pick` with the command `edit`, you can tell
 'git rebase' to stop after applying that commit, so that you can edit
 the files and/or the commit message, amend the commit, and continue
 rebasing.
 
 To interrupt the rebase (just like an "edit" command would do, but without
-cherry-picking any commit first), use the "break" command.
+cherry-picking any commit first), use the `break` command.
 
 If you just want to edit the commit message for a commit, replace the
-command "pick" with the command "reword".
+command "pick" with the command `reword`.
 
-To drop a commit, replace the command "pick" with "drop", or just
+To drop a commit, replace the command "pick" with `drop`, or just
 delete the matching line.
 
 If you want to fold two or more commits into one, replace the command
-"pick" for the second and subsequent commits with "squash" or "fixup".
+"pick" for the second and subsequent commits with `squash` or `fixup`.
 If the commits had different authors, the folded commit will be
 attributed to the author of the first commit.  The suggested commit
 message for the folded commit is the concatenation of the commit
@@ -693,7 +693,7 @@ $ git rebase -i -p --onto Q O
 Reordering and editing commits usually creates untested intermediate
 steps.  You may want to check that your history editing did not break
 anything by running a test, or at least recompiling at intermediate
-points in history by using the "exec" command (shortcut "x").  You may
+points in history by using the `exec` command (shortcut `x`).  You may
 do so by creating a todo list like this one:
 
 -------------------------------------------
-- 8< --
--
Duy

Re: [PATCH] rebase -i: introduce the 'test' command

[Johannes Schindelin]

Hi Luc,

On Mon, 3 Dec 2018, Luc Van Oostenryck wrote:

> On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote:
> > On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote:
> > 
> > > > > Would it not make more sense to add a command-line option (and a config
> > > > > setting) to re-schedule failed `exec` commands? Like so:
> > > > 
> > > > Your proposition would do in most cases, however it is not possible to
> > > > make a distinction between reschedulable and non-reschedulable commands.
> > > 
> > > True. But I don't think that's so terrible.
> > > 
> > > What I think is something to avoid is two commands that do something very,
> > > very similar, but with two very, very different names.
> > > 
> > > In reality, I think that it would even make sense to change the default to
> > > reschedule failed `exec` commands. Which is why I suggested to also add a
> > > config option.
> > 
> > I sometimes add "x false" to the top of the todo list to stop and create
> > new commits before the first one. That would be awkward if I could never
> > get past that line. However, I think elsewhere a "pause" line has been
> > discussed, which would serve the same purpose.
> > 
> > I wonder how often this kind of "yes, I know it fails, but keep going
> > anyway" situation would come up. And what the interface is like for
> > getting past it. E.g., what if you fixed a bunch of stuff but your tests
> > still fail? You may not want to abandon the changes you've made, but you
> > need to "rebase --continue" to move forward. I encounter this often when
> > the correct fix is actually in an earlier commit than the one that
> > yields the test failure. You can't rewind an interactive rebase, so I
> > complete and restart it, adding an "e"dit at the earlier commit.
> 
> In this sort of situation, I often whish to be able to do nested rebases.
> Even more because it happen relatively often that I forget that I'm
> working in a rebase and not on the head, and then it's quite natural
> to me to type things like 'git rebase -i @^^^' while already rebasing.
> But I suppose this has already been discussed.

Varieties of this have been discussed, but no, not nested rebases.

The closest we thought about was re-scheduling the latest <n> commits,
which is now harder because of the `--rebase-merges` mode.

But I think it would be doable. Your idea of a "nested" rebase actually
opens that door quite nicely. It would not *really* be a nested rebase,
and it would still only be possible in interactive mode, but I could
totally see

	git rebase --nested -i HEAD~3

to generate and prepend the following lines to the `git-rebase-todo` file:

	reset abcdef01 # This is HEAD~3
	pick abcdef02 # This is HEAD~2
	pick abcdef03 # This is HEAD~
	pick abcdef04 # This is HEAD

(assuming that the latest 3 commits were non-merge commits; It would look
quite a bit more complicated in other situations.)

Ciao,
Dscho

Re: [PATCH] rebase -i: introduce the 'test' command

[Johannes Schindelin]

Hi Duy,

On Mon, 3 Dec 2018, Duy Nguyen wrote:

> On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote:
> > I sometimes add "x false" to the top of the todo list to stop and create
> > new commits before the first one.
> 
> And here I've been doing the same by "edit" the first commit, add a
> new commit then reorder them in the second interactive rebase :P
> 
> This made me look at git-rebase.txt to really learn about interactive
> rebase. I think the interactive rebase section could use some
> improvements. Its style looks.. umm.. more story telling than a
> reference. Perhaps something like this to at least highlight the
> commands.

And maybe, just maybe, that "story telling" is more useful for users who
want to learn about the interactive rebase, just like yourself, when
compared to a mere "reference".

Ciao,
Johannes

Re: [PATCH] rebase -i: introduce the 'test' command

[Luc Van Oostenryck]

On Mon, Dec 03, 2018 at 08:01:44PM +0100, Johannes Schindelin wrote:
> Hi Luc,
> 
> On Mon, 3 Dec 2018, Luc Van Oostenryck wrote:
> 
> > On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote:
> > > I sometimes add "x false" to the top of the todo list to stop and create
> > > new commits before the first one. That would be awkward if I could never
> > > get past that line. However, I think elsewhere a "pause" line has been
> > > discussed, which would serve the same purpose.
> > > 
> > > I wonder how often this kind of "yes, I know it fails, but keep going
> > > anyway" situation would come up. And what the interface is like for
> > > getting past it. E.g., what if you fixed a bunch of stuff but your tests
> > > still fail? You may not want to abandon the changes you've made, but you
> > > need to "rebase --continue" to move forward. I encounter this often when
> > > the correct fix is actually in an earlier commit than the one that
> > > yields the test failure. You can't rewind an interactive rebase, so I
> > > complete and restart it, adding an "e"dit at the earlier commit.
> > 
> > In this sort of situation, I often whish to be able to do nested rebases.
> > Even more because it happen relatively often that I forget that I'm
> > working in a rebase and not on the head, and then it's quite natural
> > to me to type things like 'git rebase -i @^^^' while already rebasing.
> > But I suppose this has already been discussed.
> 
> Varieties of this have been discussed, but no, not nested rebases.

Interesting :)

> The closest we thought about was re-scheduling the latest <n> commits,
> which is now harder because of the `--rebase-merges` mode.
> 
> But I think it would be doable. Your idea of a "nested" rebase actually
> opens that door quite nicely. It would not *really* be a nested rebase,
> and it would still only be possible in interactive mode, but I could
> totally see
> 
> 	git rebase --nested -i HEAD~3

I don't mind much if it would be "really nested" or "as-if nested" but
with this flag --nested I wonder what would happen if I would use it
in a 'top-level' rebase (or, said in another way, would I be able
to alias 'rebase' to 'rebase --nested')?

> to generate and prepend the following lines to the `git-rebase-todo` file:
> 
> 	reset abcdef01 # This is HEAD~3
> 	pick abcdef02 # This is HEAD~2
> 	pick abcdef03 # This is HEAD~
> 	pick abcdef04 # This is HEAD
> 
 
OK, I see.
This would not be nestable/stackable but would solve the problem nicely.

Best regards,
-- Luc

Re: [PATCH] rebase -i: introduce the 'test' command

[Jeff King]

On Mon, Dec 03, 2018 at 02:31:37PM +0000, Phillip Wood wrote:

> > How would I move past the test that fails to continue? I guess "git
> > rebase --edit-todo" and then manually remove it (and any other remaining
> > test lines)?
> 
> Perhaps we could teach git rebase --skip to skip a rescheduled command, it
> could be useful if people want to skip rescheduled picks as well (though I
> don't think I've ever had that happen in the wild). I can see myself turning
> on the rescheduling config setting but occasionally wanting to be able to
> skip over the rescheduled exec command.

Yeah, I agree that would give a nice user experience.

-Peff

Re: [PATCH] rebase -i: introduce the 'test' command

[Jeff King]

On Mon, Dec 03, 2018 at 08:01:44PM +0100, Johannes Schindelin wrote:

> > In this sort of situation, I often whish to be able to do nested rebases.
> > Even more because it happen relatively often that I forget that I'm
> > working in a rebase and not on the head, and then it's quite natural
> > to me to type things like 'git rebase -i @^^^' while already rebasing.
> > But I suppose this has already been discussed.
> 
> Varieties of this have been discussed, but no, not nested rebases.
> 
> The closest we thought about was re-scheduling the latest <n> commits,
> which is now harder because of the `--rebase-merges` mode.
> 
> But I think it would be doable. Your idea of a "nested" rebase actually
> opens that door quite nicely. It would not *really* be a nested rebase,
> and it would still only be possible in interactive mode, but I could
> totally see
> 
> 	git rebase --nested -i HEAD~3
> 
> to generate and prepend the following lines to the `git-rebase-todo` file:
> 
> 	reset abcdef01 # This is HEAD~3
> 	pick abcdef02 # This is HEAD~2
> 	pick abcdef03 # This is HEAD~
> 	pick abcdef04 # This is HEAD
> 
> (assuming that the latest 3 commits were non-merge commits; It would look
> quite a bit more complicated in other situations.)

Yeah, I would probably use that if it existed.

It would be nicer to have real nested sequencer operations, I think, for
other situations. E.g., cherry-picking a sequence of commits while
you're in the middle of a rebase.

But I suspect getting that right would be _loads_ more work, and
probably would involve some funky UI corner cases to handle the stack of
operations (so truly aborting a rebase may mean an arbitrary number of
"rebase --abort" calls to pop the stack). Your suggestion is probably a
reasonable trick in the meantime.

-Peff

Re: [PATCH] rebase -i: introduce the 'test' command

[Jeff King]

On Mon, Dec 03, 2018 at 06:53:22PM +0100, Duy Nguyen wrote:

> On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote:
> > I sometimes add "x false" to the top of the todo list to stop and create
> > new commits before the first one.
> 
> And here I've been doing the same by "edit" the first commit, add a
> new commit then reorder them in the second interactive rebase :P
> 
> This made me look at git-rebase.txt to really learn about interactive
> rebase. I think the interactive rebase section could use some
> improvements. Its style looks.. umm.. more story telling than a
> reference. Perhaps something like this to at least highlight the
> commands.

Yeah, I think the typographic change is an improvement that doesn't
really have a downside.

As Dscho mentioned, sometimes the story style is what you want, so I
don't think we'd want to simply rearrange it. It may be useful to
present the material twice, though: once as a table/list for reference,
and then once as a story working through an example.

-Peff

Re: [PATCH] rebase -i: introduce the 'test' command

[Johannes Schindelin]

Hi Peff,

On Mon, 3 Dec 2018, Jeff King wrote:

> On Mon, Dec 03, 2018 at 08:01:44PM +0100, Johannes Schindelin wrote:
> 
> > > In this sort of situation, I often whish to be able to do nested rebases.
> > > Even more because it happen relatively often that I forget that I'm
> > > working in a rebase and not on the head, and then it's quite natural
> > > to me to type things like 'git rebase -i @^^^' while already rebasing.
> > > But I suppose this has already been discussed.
> > 
> > Varieties of this have been discussed, but no, not nested rebases.
> > 
> > The closest we thought about was re-scheduling the latest <n> commits,
> > which is now harder because of the `--rebase-merges` mode.
> > 
> > But I think it would be doable. Your idea of a "nested" rebase actually
> > opens that door quite nicely. It would not *really* be a nested rebase,
> > and it would still only be possible in interactive mode, but I could
> > totally see
> > 
> > 	git rebase --nested -i HEAD~3
> > 
> > to generate and prepend the following lines to the `git-rebase-todo` file:
> > 
> > 	reset abcdef01 # This is HEAD~3
> > 	pick abcdef02 # This is HEAD~2
> > 	pick abcdef03 # This is HEAD~
> > 	pick abcdef04 # This is HEAD
> > 
> > (assuming that the latest 3 commits were non-merge commits; It would look
> > quite a bit more complicated in other situations.)
> 
> Yeah, I would probably use that if it existed.

I kind of use it, even if it does not exist ;-)

> It would be nicer to have real nested sequencer operations, I think, for
> other situations.

I agree. But for the moment, our data format is too married to the exact
layout of .git/, thanks to `git rebase`'s evolution from a Unix shell
script.

Alban has this really great patch series to work on the todo list
in-memory, and that paves the way to decouple the entire sequencer thing
from the file system.

The most notably thing that still would need to be encapsulated would be
the options: currently, there is a plethora of inconsistent options files
being saved into the state directory (for some, the mere presence
indicates `true`, some contain `true` or `false`, others contain text,
etc).

> E.g., cherry-picking a sequence of commits while you're in the middle of
> a rebase.

You will be delighted to learn that you can cherry-pick a sequence of
commits in the middle of a rebase already. I do `exec git cherry-pick
<range>` *all* the time.

> But I suspect getting that right would be _loads_ more work, and
> probably would involve some funky UI corner cases to handle the stack of
> operations (so truly aborting a rebase may mean an arbitrary number of
> "rebase --abort" calls to pop the stack). Your suggestion is probably a
> reasonable trick in the meantime.

You know what is an even more reasonable trick? Worktrees.

I only thought about that this morning, but I should have mentioned it
right away, as I use it quite frequently.

When I have tricky nested rebases to perform, I do use throw-away
worktrees where I check out unnamed branches, work on those, and then
integrate them back into the "outer rebase" via the `reset` command in the
todo list.

Ciao,
Dscho

Re: [PATCH] rebase -i: introduce the 'test' command

[Ævar Arnfjörð Bjarmason]

On Sun, Dec 02 2018, Jeff King wrote:

> On Sat, Dec 01, 2018 at 09:28:47PM -0500, Eric Sunshine wrote:
>
>> On Sat, Dec 1, 2018 at 3:02 PM Jeff King <peff@peff.net> wrote:
>> > On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote:
>> > > In reality, I think that it would even make sense to change the default to
>> > > reschedule failed `exec` commands. Which is why I suggested to also add a
>> > > config option.
>> >
>> > I sometimes add "x false" to the top of the todo list to stop and create
>> > new commits before the first one. That would be awkward if I could never
>> > get past that line. However, I think elsewhere a "pause" line has been
>> > discussed, which would serve the same purpose.
>>
>> Are you thinking of the "break" command (not "pause") which Dscho
>> already added[1]?
>>
>> [1]: 71f82465b1 (rebase -i: introduce the 'break' command, 2018-10-12)
>
> Yes, thanks (as you can see, I haven't actually used it yet ;) ).

Related: I got poked about a bug where someone was doing "exec bash" to
do the same, and would then CD to other repos, and git operations would
still be executed on the original repo because we set GIT_DIR=* in the
envioronment (but not for "break").

Not a big deal, but I wondered if that was a bug in itself, i.e. if we
need to set GIT_DIR et al in the environment for "exec". Isn't having
the current directory set to the checkout sufficient?

Re: [PATCH] rebase -i: introduce the 'test' command

[Jeff King]

On Tue, Dec 11, 2018 at 01:40:25PM +0100, Ævar Arnfjörð Bjarmason wrote:

> >> Are you thinking of the "break" command (not "pause") which Dscho
> >> already added[1]?
> >>
> >> [1]: 71f82465b1 (rebase -i: introduce the 'break' command, 2018-10-12)
> >
> > Yes, thanks (as you can see, I haven't actually used it yet ;) ).
> 
> Related: I got poked about a bug where someone was doing "exec bash" to
> do the same, and would then CD to other repos, and git operations would
> still be executed on the original repo because we set GIT_DIR=* in the
> envioronment (but not for "break").
> 
> Not a big deal, but I wondered if that was a bug in itself, i.e. if we
> need to set GIT_DIR et al in the environment for "exec". Isn't having
> the current directory set to the checkout sufficient?

This behavior is discussed in this sub-thread:

  https://public-inbox.org/git/xmqqsh4jt5d0.fsf@gitster-ct.c.googlers.com/

-Peff