git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH] rebase -i: introduce the 'test' command
@ 2018-11-28 13:28 Paul Morelle
  2018-11-28 15:19 ` Johannes Schindelin
  0 siblings, 1 reply; 19+ messages in thread
From: Paul Morelle @ 2018-11-28 13:28 UTC (permalink / raw)
  To: Git Users

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


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

* Re: [PATCH] rebase -i: introduce the 'test' command
  2018-11-28 13:28 [PATCH] rebase -i: introduce the 'test' command Paul Morelle
@ 2018-11-28 15:19 ` Johannes Schindelin
  2018-11-28 16:56   ` Paul Morelle
  0 siblings, 1 reply; 19+ messages in thread
From: Johannes Schindelin @ 2018-11-28 15:19 UTC (permalink / raw)
  To: Paul Morelle; +Cc: Git Users

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

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

* Re: [PATCH] rebase -i: introduce the 'test' command
  2018-11-28 15:19 ` Johannes Schindelin
@ 2018-11-28 16:56   ` Paul Morelle
  2018-11-29  8:32     ` Johannes Schindelin
  0 siblings, 1 reply; 19+ messages in thread
From: Paul Morelle @ 2018-11-28 16:56 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Git Users

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

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

* Re: [PATCH] rebase -i: introduce the 'test' command
  2018-11-28 16:56   ` Paul Morelle
@ 2018-11-29  8:32     ` Johannes Schindelin
  2018-11-29 10:55       ` Johannes Schindelin
  2018-12-01 20:02       ` Jeff King
  0 siblings, 2 replies; 19+ messages in thread
From: Johannes Schindelin @ 2018-11-29  8:32 UTC (permalink / raw)
  To: Paul Morelle; +Cc: Git Users

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

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

* Re: [PATCH] rebase -i: introduce the 'test' command
  2018-11-29  8:32     ` Johannes Schindelin
@ 2018-11-29 10:55       ` Johannes Schindelin
  2018-12-01 20:02       ` Jeff King
  1 sibling, 0 replies; 19+ messages in thread
From: Johannes Schindelin @ 2018-11-29 10:55 UTC (permalink / raw)
  To: Paul Morelle; +Cc: Git Users

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

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

* Re: [PATCH] rebase -i: introduce the 'test' command
  2018-11-29  8:32     ` Johannes Schindelin
  2018-11-29 10:55       ` Johannes Schindelin
@ 2018-12-01 20:02       ` Jeff King
  2018-12-02  2:28         ` Eric Sunshine
                           ` (4 more replies)
  1 sibling, 5 replies; 19+ messages in thread
From: Jeff King @ 2018-12-01 20:02 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Paul Morelle, Git Users

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

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

* Re: [PATCH] rebase -i: introduce the 'test' command
  2018-12-01 20:02       ` Jeff King
@ 2018-12-02  2:28         ` Eric Sunshine
  2018-12-02  3:31           ` Jeff King
  2018-12-02 19:48         ` Johannes Schindelin
                           ` (3 subsequent siblings)
  4 siblings, 1 reply; 19+ messages in thread
From: Eric Sunshine @ 2018-12-02  2:28 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Schindelin, paul.morelle, Git List

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)

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

* Re: [PATCH] rebase -i: introduce the 'test' command
  2018-12-02  2:28         ` Eric Sunshine
@ 2018-12-02  3:31           ` Jeff King
  0 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2018-12-02  3:31 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Johannes Schindelin, paul.morelle, Git List

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

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

* Re: [PATCH] rebase -i: introduce the 'test' command
  2018-12-01 20:02       ` Jeff King
  2018-12-02  2:28         ` Eric Sunshine
@ 2018-12-02 19:48         ` Johannes Schindelin
  2018-12-03 14:31         ` Phillip Wood
                           ` (2 subsequent siblings)
  4 siblings, 0 replies; 19+ messages in thread
From: Johannes Schindelin @ 2018-12-02 19:48 UTC (permalink / raw)
  To: Jeff King; +Cc: Paul Morelle, Git Users

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

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

* Re: [PATCH] rebase -i: introduce the 'test' command
  2018-12-01 20:02       ` Jeff King
  2018-12-02  2:28         ` Eric Sunshine
  2018-12-02 19:48         ` Johannes Schindelin
@ 2018-12-03 14:31         ` Phillip Wood
  2018-12-03 21:27           ` Jeff King
  2018-12-03 17:27         ` Luc Van Oostenryck
  2018-12-03 17:53         ` Duy Nguyen
  4 siblings, 1 reply; 19+ messages in thread
From: Phillip Wood @ 2018-12-03 14:31 UTC (permalink / raw)
  To: Jeff King, Johannes Schindelin; +Cc: Paul Morelle, Git Users

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
> 


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

* Re: [PATCH] rebase -i: introduce the 'test' command
  2018-12-01 20:02       ` Jeff King
                           ` (2 preceding siblings ...)
  2018-12-03 14:31         ` Phillip Wood
@ 2018-12-03 17:27         ` Luc Van Oostenryck
  2018-12-03 19:01           ` Johannes Schindelin
  2018-12-03 17:53         ` Duy Nguyen
  4 siblings, 1 reply; 19+ messages in thread
From: Luc Van Oostenryck @ 2018-12-03 17:27 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Schindelin, Paul Morelle, Git Users

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

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

* Re: [PATCH] rebase -i: introduce the 'test' command
  2018-12-01 20:02       ` Jeff King
                           ` (3 preceding siblings ...)
  2018-12-03 17:27         ` Luc Van Oostenryck
@ 2018-12-03 17:53         ` Duy Nguyen
  2018-12-03 19:03           ` Johannes Schindelin
  2018-12-03 21:34           ` Jeff King
  4 siblings, 2 replies; 19+ messages in thread
From: Duy Nguyen @ 2018-12-03 17:53 UTC (permalink / raw)
  To: Jeff King, Johannes Schindelin; +Cc: Paul Morelle, Git Users

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

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

* Re: [PATCH] rebase -i: introduce the 'test' command
  2018-12-03 17:27         ` Luc Van Oostenryck
@ 2018-12-03 19:01           ` Johannes Schindelin
  2018-12-03 19:34             ` Luc Van Oostenryck
  2018-12-03 21:31             ` Jeff King
  0 siblings, 2 replies; 19+ messages in thread
From: Johannes Schindelin @ 2018-12-03 19:01 UTC (permalink / raw)
  To: Luc Van Oostenryck; +Cc: Jeff King, Paul Morelle, Git Users

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

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

* Re: [PATCH] rebase -i: introduce the 'test' command
  2018-12-03 17:53         ` Duy Nguyen
@ 2018-12-03 19:03           ` Johannes Schindelin
  2018-12-03 21:34           ` Jeff King
  1 sibling, 0 replies; 19+ messages in thread
From: Johannes Schindelin @ 2018-12-03 19:03 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Jeff King, Paul Morelle, Git Users

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

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

* Re: [PATCH] rebase -i: introduce the 'test' command
  2018-12-03 19:01           ` Johannes Schindelin
@ 2018-12-03 19:34             ` Luc Van Oostenryck
  2018-12-03 21:31             ` Jeff King
  1 sibling, 0 replies; 19+ messages in thread
From: Luc Van Oostenryck @ 2018-12-03 19:34 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Jeff King, Paul Morelle, Git Users

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

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

* Re: [PATCH] rebase -i: introduce the 'test' command
  2018-12-03 14:31         ` Phillip Wood
@ 2018-12-03 21:27           ` Jeff King
  0 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2018-12-03 21:27 UTC (permalink / raw)
  To: phillip.wood; +Cc: Johannes Schindelin, Paul Morelle, Git Users

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

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

* Re: [PATCH] rebase -i: introduce the 'test' command
  2018-12-03 19:01           ` Johannes Schindelin
  2018-12-03 19:34             ` Luc Van Oostenryck
@ 2018-12-03 21:31             ` Jeff King
  2018-12-04  9:13               ` Johannes Schindelin
  1 sibling, 1 reply; 19+ messages in thread
From: Jeff King @ 2018-12-03 21:31 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Luc Van Oostenryck, Paul Morelle, Git Users

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

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

* Re: [PATCH] rebase -i: introduce the 'test' command
  2018-12-03 17:53         ` Duy Nguyen
  2018-12-03 19:03           ` Johannes Schindelin
@ 2018-12-03 21:34           ` Jeff King
  1 sibling, 0 replies; 19+ messages in thread
From: Jeff King @ 2018-12-03 21:34 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Johannes Schindelin, Paul Morelle, Git Users

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

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

* Re: [PATCH] rebase -i: introduce the 'test' command
  2018-12-03 21:31             ` Jeff King
@ 2018-12-04  9:13               ` Johannes Schindelin
  0 siblings, 0 replies; 19+ messages in thread
From: Johannes Schindelin @ 2018-12-04  9:13 UTC (permalink / raw)
  To: Jeff King; +Cc: Luc Van Oostenryck, Paul Morelle, Git Users

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

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

end of thread, back to index

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-28 13:28 [PATCH] rebase -i: introduce the 'test' command Paul Morelle
2018-11-28 15:19 ` Johannes Schindelin
2018-11-28 16:56   ` Paul Morelle
2018-11-29  8:32     ` Johannes Schindelin
2018-11-29 10:55       ` Johannes Schindelin
2018-12-01 20:02       ` Jeff King
2018-12-02  2:28         ` Eric Sunshine
2018-12-02  3:31           ` Jeff King
2018-12-02 19:48         ` Johannes Schindelin
2018-12-03 14:31         ` Phillip Wood
2018-12-03 21:27           ` Jeff King
2018-12-03 17:27         ` Luc Van Oostenryck
2018-12-03 19:01           ` Johannes Schindelin
2018-12-03 19:34             ` Luc Van Oostenryck
2018-12-03 21:31             ` Jeff King
2018-12-04  9:13               ` Johannes Schindelin
2018-12-03 17:53         ` Duy Nguyen
2018-12-03 19:03           ` Johannes Schindelin
2018-12-03 21:34           ` Jeff King

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox