[PATCH 0/3] rebase --edit-todo --exec

[PATCH 0/3] rebase --edit-todo --exec

[Andrei Rybak]

I've wanted this feature for a long time, and now with rebase working without
forking rebase--interactive (thanks to Phillip Wood for working on that), I
finally got around to implementing it.

It still needs validation for arguments. Right now, I have two ideas:

  1. iterate over original argv and make sure its just '--exec's with its
     arguments.
  2. check all other vars in options[].

Andrei Rybak (3):
  rebase: prepare cmd before choosing action
  rebase: extract add_exec()
  rebase -i: allow --edit-todo with --exec

 Documentation/git-rebase.txt  |  3 +-
 builtin/rebase.c              | 63 ++++++++++++++++++++++-------------
 t/t3404-rebase-interactive.sh | 32 ++++++++++++++++++
 3 files changed, 74 insertions(+), 24 deletions(-)

-- 
2.24.0.windows.2

[PATCH 1/3] rebase: prepare cmd before choosing action

[Andrei Rybak]

When git rebase is started with option --exec, its arguments are parsed
into string_list exec and then converted into options.cmd.

In following commits, action --edit-todo will be taught to use arguments
passed with --exec option.  Prepare options.cmd before switch (action)
to make it available for the ACTION_EDIT_TODO branch of the switch.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 builtin/rebase.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 4a20582e72..9457912f9d 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1595,6 +1595,21 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			trace2_cmd_mode(action_names[action]);
 	}
 
+	for (i = 0; i < exec.nr; i++)
+		if (check_exec_cmd(exec.items[i].string))
+			exit(1);
+
+	if (exec.nr) {
+		int i;
+
+		imply_interactive(&options, "--exec");
+
+		strbuf_reset(&buf);
+		for (i = 0; i < exec.nr; i++)
+			strbuf_addf(&buf, "exec %s\n", exec.items[i].string);
+		options.cmd = xstrdup(buf.buf);
+	}
+
 	switch (action) {
 	case ACTION_CONTINUE: {
 		struct object_id head;
@@ -1731,10 +1746,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	for (i = 0; i < exec.nr; i++)
-		if (check_exec_cmd(exec.items[i].string))
-			exit(1);
-
 	if (!(options.flags & REBASE_NO_QUIET))
 		argv_array_push(&options.git_am_opts, "-q");
 
@@ -1746,17 +1757,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
 	}
 
-	if (exec.nr) {
-		int i;
-
-		imply_interactive(&options, "--exec");
-
-		strbuf_reset(&buf);
-		for (i = 0; i < exec.nr; i++)
-			strbuf_addf(&buf, "exec %s\n", exec.items[i].string);
-		options.cmd = xstrdup(buf.buf);
-	}
-
 	if (rebase_merges) {
 		if (!*rebase_merges)
 			; /* default mode; do nothing */
-- 
2.24.0.windows.2

[PATCH 2/3] rebase: extract add_exec()

[Andrei Rybak]

In following commit addition of commands to the todo file will be
implemented for the --edit-todo action.  So extract a function to avoid
duplicating the code for splitting of opts->cmd into list of commands.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 builtin/rebase.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 9457912f9d..27507d3cf6 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -299,6 +299,17 @@ static void split_exec_commands(const char *cmd, struct string_list *commands)
 	}
 }
 
+static int add_exec(const char *cmd)
+{
+	struct string_list commands = STRING_LIST_INIT_DUP;
+	int ret;
+
+	split_exec_commands(cmd, &commands);
+	ret = add_exec_commands(&commands);
+	string_list_clear(&commands, 0);
+	return ret;
+}
+
 static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 {
 	int ret;
@@ -420,11 +431,7 @@ static int run_rebase_interactive(struct rebase_options *opts,
 		ret = rearrange_squash_in_todo_file();
 		break;
 	case ACTION_ADD_EXEC: {
-		struct string_list commands = STRING_LIST_INIT_DUP;
-
-		split_exec_commands(opts->cmd, &commands);
-		ret = add_exec_commands(&commands);
-		string_list_clear(&commands, 0);
+		ret = add_exec(opts->cmd);
 		break;
 	}
 	default:
-- 
2.24.0.windows.2

[PATCH 3/3] rebase -i: allow --edit-todo with --exec

[Andrei Rybak]

When using rebase, option --exec can be used, for example, to run tests
after every commit created by rebase.  When using interactive rebase, I
don't always know, if I would want to run something against each commit
before I start rebasing.  Sometimes, I realize this only after doing
some editing in the middle of the rebase.  To do that, I have to
manually edit the todo file.  Additing exec command by hand or
semi-automatically is cumbersome and error prone.  Especially if the
file is big or complex, e.g. when option --rebase-merges is used.

Allow using the --edit-todo action of git rebase with option --exec.
New test is based on test 'rebase --edit-todo can be used to modify
todo'.  Contents of todo file are checked using set_cat_todo_editor
similarly to what test 'respects rebase.abbreviateCommands with fixup,
squash and exec' does.

Remove unnecessary braces around call to usage_with_options, while we're
here.

TODO: Still need better validation of options. With current
      implementation, the following is not rejected:

	git rebase --edit-todo -x 'git show HEAD' --autostash

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 Documentation/git-rebase.txt  |  3 ++-
 builtin/rebase.c              | 16 +++++++++++++---
 t/t3404-rebase-interactive.sh | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 639a4179d1..b5db5e80d4 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -12,7 +12,8 @@ SYNOPSIS
 	[--onto <newbase> | --keep-base] [<upstream> [<branch>]]
 'git rebase' [-i | --interactive] [<options>] [--exec <cmd>] [--onto <newbase>]
 	--root [<branch>]
-'git rebase' (--continue | --skip | --abort | --quit | --edit-todo | --show-current-patch)
+'git rebase' (--continue | --skip | --abort | --quit |
+	--edit-todo [--exec<cmd>] | --show-current-patch)
 
 DESCRIPTION
 -----------
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 27507d3cf6..1ee55b48e7 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -33,7 +33,8 @@ static char const * const builtin_rebase_usage[] = {
 		"[--onto <newbase> | --keep-base] [<upstream> [<branch>]]"),
 	N_("git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] "
 		"--root [<branch>]"),
-	N_("git rebase --continue | --abort | --skip | --edit-todo"),
+	N_("git rebase --continue | --abort | --skip | "
+		"--edit-todo [--exec <cmd>]"),
 	NULL
 };
 
@@ -409,6 +410,11 @@ static int run_rebase_interactive(struct rebase_options *opts,
 		break;
 	}
 	case ACTION_EDIT_TODO:
+		if (opts->cmd) {
+			ret = add_exec(opts->cmd);
+			if (ret)
+				break;
+		}
 		ret = edit_todo_file(flags);
 		break;
 	case ACTION_SHOW_CURRENT_PATCH: {
@@ -1565,15 +1571,19 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			     builtin_rebase_options,
 			     builtin_rebase_usage, 0);
 
-	if (action != ACTION_NONE && total_argc != 2) {
+	if (action != ACTION_NONE && action != ACTION_EDIT_TODO &&
+	    total_argc != 2)
 		usage_with_options(builtin_rebase_usage,
 				   builtin_rebase_options);
-	}
 
 	if (argc > 2)
 		usage_with_options(builtin_rebase_usage,
 				   builtin_rebase_options);
 
+	if (action == ACTION_EDIT_TODO && argc > 0)
+		usage_with_options(builtin_rebase_usage,
+				   builtin_rebase_options);
+
 	if (options.type == REBASE_PRESERVE_MERGES)
 		warning(_("git rebase --preserve-merges is deprecated. "
 			  "Use --rebase-merges instead."));
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d2dfbe46b9..5decb8570e 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1082,6 +1082,38 @@ test_expect_success 'rebase --edit-todo can be used to modify todo' '
 	test L = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
+test_expect_success 'rebase --edit-todo can be used with -x' '
+	test_when_finished "reset_rebase" &&
+	git reset --hard &&
+	git checkout no-conflict-branch^0 &&
+	cat >expected <<-EOF &&
+	pick $(git rev-list --abbrev-commit -1 HEAD^) L
+	exec git show HEAD
+	pick $(git rev-list --abbrev-commit -1 HEAD) M
+	exec git show HEAD
+	EOF
+	set_fake_editor &&
+	FAKE_LINES="1 edit 2 3 4" git rebase -i HEAD~4 &&
+	set_cat_todo_editor &&
+	test_must_fail git rebase --edit-todo -x "git show HEAD" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_failure 'rebase --edit-todo -x does not allow other arguments' '
+	test_when_finished "reset_rebase" &&
+	git reset --hard &&
+	git checkout no-conflict-branch^0 &&
+	cat >expected <<-EOF &&
+	pick $(git rev-list --abbrev-commit -1 HEAD^) L
+	exec git show HEAD
+	pick $(git rev-list --abbrev-commit -1 HEAD) M
+	exec git show HEAD
+	EOF
+	set_fake_editor &&
+	FAKE_LINES="1 edit 2 3 4" git rebase -i HEAD~4 &&
+	test_must_fail git rebase --edit-todo -x "git show HEAD" --autostash
+'
+
 test_expect_success 'rebase -i produces readable reflog' '
 	git reset --hard &&
 	git branch -f branch-reflog-test H &&
-- 
2.24.0.windows.2

Re: [PATCH 1/3] rebase: prepare cmd before choosing action

[Junio C Hamano]

Andrei Rybak <rybak.a.v@gmail.com> writes:

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 4a20582e72..9457912f9d 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1595,6 +1595,21 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			trace2_cmd_mode(action_names[action]);
>  	}
>  
> +	for (i = 0; i < exec.nr; i++)
> +		if (check_exec_cmd(exec.items[i].string))
> +			exit(1);
> +
> +	if (exec.nr) {
> +		int i;

This masks the outer "i" (I am assuming such a thing exists, judging
from its use in the above loop you added), but I do not see a need
for it.  With or without this masked local "i", the value of outer "i"
will be equal to exec.nr+1 after the control leaves this if block.

> +		imply_interactive(&options, "--exec");
> +
> +		strbuf_reset(&buf);
> +		for (i = 0; i < exec.nr; i++)
> +			strbuf_addf(&buf, "exec %s\n", exec.items[i].string);
> +		options.cmd = xstrdup(buf.buf);
> +	}
> +

[RFC PATCH v2 0/4] rebase --edit-todo --exec

[Andrei Rybak]

Differences from v1:
  - Added missing RFC marker.
  - Fixed CC list.
  - Addressed Junio's comment about masked variable in a for loop: another
    preparatory patch is added.
  - Cleaned up copy-pasted unused code in the new test.

----

Original cover letter:
I've wanted this feature for a long time, and now with rebase working without
forking rebase--interactive (thanks to Phillip Wood for working on that), I
finally got around to implementing it.

It still needs validation for arguments. Right now, I have two ideas:

  1. iterate over original argv and make sure its just '--exec's with its
     arguments.
  2. check all other vars in options[].

Andrei Rybak (4):
  builtin/rebase.c: reuse loop variable
  rebase: prepare cmd before choosing action
  rebase: extract add_exec()
  rebase -i: allow --edit-todo with --exec

 Documentation/git-rebase.txt  |  3 +-
 builtin/rebase.c              | 63 +++++++++++++++++++++--------------
 t/t3404-rebase-interactive.sh | 32 ++++++++++++++++++
 3 files changed, 72 insertions(+), 26 deletions(-)

-- 
2.24.0.windows.2

[RFC PATCH v2 1/4] builtin/rebase.c: reuse loop variable

[Andrei Rybak]

Variable "int i" is already defined at the top of the function
cmd_rebase, so reuse it instead of declaring other variables, which mask
the outer "i".

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 builtin/rebase.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 4a20582e72..793cac1386 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1747,8 +1747,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 
 	if (exec.nr) {
-		int i;
-
 		imply_interactive(&options, "--exec");
 
 		strbuf_reset(&buf);
@@ -1769,8 +1767,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 
 	if (strategy_options.nr) {
-		int i;
-
 		if (!options.strategy)
 			options.strategy = "recursive";
 
-- 
2.24.0.windows.2

[RFC PATCH v2 2/4] rebase: prepare cmd before choosing action

[Andrei Rybak]

When git rebase is started with option --exec, its arguments are parsed
into string_list exec and then converted into options.cmd.

In following commits, action --edit-todo will be taught to use arguments
passed with --exec option.  Prepare options.cmd before switch (action)
to make it available for the ACTION_EDIT_TODO branch of the switch.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 builtin/rebase.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 793cac1386..fa27f7b439 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1595,6 +1595,19 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			trace2_cmd_mode(action_names[action]);
 	}
 
+	for (i = 0; i < exec.nr; i++)
+		if (check_exec_cmd(exec.items[i].string))
+			exit(1);
+
+	if (exec.nr) {
+		imply_interactive(&options, "--exec");
+
+		strbuf_reset(&buf);
+		for (i = 0; i < exec.nr; i++)
+			strbuf_addf(&buf, "exec %s\n", exec.items[i].string);
+		options.cmd = xstrdup(buf.buf);
+	}
+
 	switch (action) {
 	case ACTION_CONTINUE: {
 		struct object_id head;
@@ -1731,10 +1744,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	for (i = 0; i < exec.nr; i++)
-		if (check_exec_cmd(exec.items[i].string))
-			exit(1);
-
 	if (!(options.flags & REBASE_NO_QUIET))
 		argv_array_push(&options.git_am_opts, "-q");
 
@@ -1746,15 +1755,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
 	}
 
-	if (exec.nr) {
-		imply_interactive(&options, "--exec");
-
-		strbuf_reset(&buf);
-		for (i = 0; i < exec.nr; i++)
-			strbuf_addf(&buf, "exec %s\n", exec.items[i].string);
-		options.cmd = xstrdup(buf.buf);
-	}
-
 	if (rebase_merges) {
 		if (!*rebase_merges)
 			; /* default mode; do nothing */
-- 
2.24.0.windows.2

[RFC PATCH v2 3/4] rebase: extract add_exec()

[Andrei Rybak]

In following commit addition of commands to the todo file will be
implemented for the --edit-todo action.  So extract a function to avoid
duplicating the code for splitting of opts->cmd into list of commands.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 builtin/rebase.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index fa27f7b439..8a6ac7439b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -299,6 +299,17 @@ static void split_exec_commands(const char *cmd, struct string_list *commands)
 	}
 }
 
+static int add_exec(const char *cmd)
+{
+	struct string_list commands = STRING_LIST_INIT_DUP;
+	int ret;
+
+	split_exec_commands(cmd, &commands);
+	ret = add_exec_commands(&commands);
+	string_list_clear(&commands, 0);
+	return ret;
+}
+
 static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 {
 	int ret;
@@ -420,11 +431,7 @@ static int run_rebase_interactive(struct rebase_options *opts,
 		ret = rearrange_squash_in_todo_file();
 		break;
 	case ACTION_ADD_EXEC: {
-		struct string_list commands = STRING_LIST_INIT_DUP;
-
-		split_exec_commands(opts->cmd, &commands);
-		ret = add_exec_commands(&commands);
-		string_list_clear(&commands, 0);
+		ret = add_exec(opts->cmd);
 		break;
 	}
 	default:
-- 
2.24.0.windows.2

[RFC PATCH v2 4/4] rebase -i: allow --edit-todo with --exec

[Andrei Rybak]

When using rebase, option --exec can be used, for example, to run tests
after every commit created by rebase.  When using interactive rebase, I
don't always know, if I would want to run something against each commit
before I start rebasing.  Sometimes, I realize this only after doing
some editing in the middle of the rebase.  To do that, I have to
manually edit the todo file.  Additing exec command by hand or
semi-automatically is cumbersome and error prone.  Especially if the
file is big or complex, e.g. when option --rebase-merges is used.

Allow using the --edit-todo action of git rebase with option --exec.
New test is based on test 'rebase --edit-todo can be used to modify
todo'.  Contents of todo file are checked using set_cat_todo_editor
similarly to what test 'respects rebase.abbreviateCommands with fixup,
squash and exec' does.

Remove unnecessary braces around call to usage_with_options, while we're
here.

TODO: Still need better validation of options. With current
      implementation, the following is not rejected:

	git rebase --edit-todo -x 'git show HEAD' --autostash

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 Documentation/git-rebase.txt  |  3 ++-
 builtin/rebase.c              | 16 +++++++++++++---
 t/t3404-rebase-interactive.sh | 26 ++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 639a4179d1..b5db5e80d4 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -12,7 +12,8 @@ SYNOPSIS
 	[--onto <newbase> | --keep-base] [<upstream> [<branch>]]
 'git rebase' [-i | --interactive] [<options>] [--exec <cmd>] [--onto <newbase>]
 	--root [<branch>]
-'git rebase' (--continue | --skip | --abort | --quit | --edit-todo | --show-current-patch)
+'git rebase' (--continue | --skip | --abort | --quit |
+	--edit-todo [--exec<cmd>] | --show-current-patch)
 
 DESCRIPTION
 -----------
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 8a6ac7439b..4fd55cfbb4 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -33,7 +33,8 @@ static char const * const builtin_rebase_usage[] = {
 		"[--onto <newbase> | --keep-base] [<upstream> [<branch>]]"),
 	N_("git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] "
 		"--root [<branch>]"),
-	N_("git rebase --continue | --abort | --skip | --edit-todo"),
+	N_("git rebase --continue | --abort | --skip | "
+		"--edit-todo [--exec <cmd>]"),
 	NULL
 };
 
@@ -409,6 +410,11 @@ static int run_rebase_interactive(struct rebase_options *opts,
 		break;
 	}
 	case ACTION_EDIT_TODO:
+		if (opts->cmd) {
+			ret = add_exec(opts->cmd);
+			if (ret)
+				break;
+		}
 		ret = edit_todo_file(flags);
 		break;
 	case ACTION_SHOW_CURRENT_PATCH: {
@@ -1565,15 +1571,19 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			     builtin_rebase_options,
 			     builtin_rebase_usage, 0);
 
-	if (action != ACTION_NONE && total_argc != 2) {
+	if (action != ACTION_NONE && action != ACTION_EDIT_TODO &&
+	    total_argc != 2)
 		usage_with_options(builtin_rebase_usage,
 				   builtin_rebase_options);
-	}
 
 	if (argc > 2)
 		usage_with_options(builtin_rebase_usage,
 				   builtin_rebase_options);
 
+	if (action == ACTION_EDIT_TODO && argc > 0)
+		usage_with_options(builtin_rebase_usage,
+				   builtin_rebase_options);
+
 	if (options.type == REBASE_PRESERVE_MERGES)
 		warning(_("git rebase --preserve-merges is deprecated. "
 			  "Use --rebase-merges instead."));
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d2dfbe46b9..c3b640575d 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1082,6 +1082,32 @@ test_expect_success 'rebase --edit-todo can be used to modify todo' '
 	test L = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
+test_expect_success 'rebase --edit-todo can be used with -x' '
+	test_when_finished "reset_rebase" &&
+	git reset --hard &&
+	git checkout no-conflict-branch^0 &&
+	cat >expected <<-EOF &&
+	pick $(git rev-list --abbrev-commit -1 HEAD^) L
+	exec git show HEAD
+	pick $(git rev-list --abbrev-commit -1 HEAD) M
+	exec git show HEAD
+	EOF
+	set_fake_editor &&
+	FAKE_LINES="1 edit 2 3 4" git rebase -i HEAD~4 &&
+	set_cat_todo_editor &&
+	test_must_fail git rebase --edit-todo -x "git show HEAD" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_failure 'rebase --edit-todo -x does not allow other arguments' '
+	test_when_finished "reset_rebase" &&
+	git reset --hard &&
+	git checkout no-conflict-branch^0 &&
+	set_fake_editor &&
+	FAKE_LINES="1 edit 2 3 4" git rebase -i HEAD~4 &&
+	test_must_fail git rebase --edit-todo -x "git show HEAD" --autostash
+'
+
 test_expect_success 'rebase -i produces readable reflog' '
 	git reset --hard &&
 	git branch -f branch-reflog-test H &&
-- 
2.24.0.windows.2

Re: [RFC PATCH v2 1/4] builtin/rebase.c: reuse loop variable

[Junio C Hamano]

Andrei Rybak <rybak.a.v@gmail.com> writes:

> Variable "int i" is already defined at the top of the function
> cmd_rebase, so reuse it instead of declaring other variables, which mask
> the outer "i".

The log message must also mention the other reason why this
simplification is correct, namely, that outer "i" is dead at the
point in the code that is touched by this patch and the value is
never used in the later parts of the code (I just followed the
codepath and made sure that is the case---iow, I think this patch
is correct).

Thanks.

>
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
>  builtin/rebase.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 4a20582e72..793cac1386 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1747,8 +1747,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	if (exec.nr) {
> -		int i;
> -
>  		imply_interactive(&options, "--exec");
>  
>  		strbuf_reset(&buf);
> @@ -1769,8 +1767,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	if (strategy_options.nr) {
> -		int i;
> -
>  		if (!options.strategy)
>  			options.strategy = "recursive";

Re: [RFC PATCH v2 2/4] rebase: prepare cmd before choosing action

[Junio C Hamano]

Andrei Rybak <rybak.a.v@gmail.com> writes:

> When git rebase is started with option --exec, its arguments are parsed
> into string_list exec and then converted into options.cmd.
>
> In following commits, action --edit-todo will be taught to use arguments
> passed with --exec option.  Prepare options.cmd before switch (action)
> to make it available for the ACTION_EDIT_TODO branch of the switch.

Hmph.  With or without this change, when we hit the run_rebase label
in this function and call into run_rebase_interactive(), opts->cmd
does contain what came from the --exec option.  In that function, I
see ACTION_EDIT_TODO calls edit_todo_file() that edits the on-disk
file without paying attention to opts->cmd (the only thing in the
function that pays attention to this field is ACTION_ADD_EXEC).

So I am not sure what makes this step necessary.  I guess it is not
wrong per-se, but if the objetive of this series is to add what
came from the --exec option when the user interacts with the editor
in rebase-interactive.c::edit_todo_list(), wouldn't it be sufficient
to skip this step, pass opts to edit_todo_file() and let the helper
use opts->cmd while preparing the todo_list it passes to underlying
edit_todo_list() function?

I am not claiming that it would be a better way---I wouldn't be
surprised if it is an incorrect approach---but it is unclear why
this step is needed and why the tweak of the todo list must be done
in the "switch (action)" we see in the post context of the first
hunk in this patch.

Thanks for working on this.


> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
>  builtin/rebase.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 793cac1386..fa27f7b439 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1595,6 +1595,19 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			trace2_cmd_mode(action_names[action]);
>  	}
>  
> +	for (i = 0; i < exec.nr; i++)
> +		if (check_exec_cmd(exec.items[i].string))
> +			exit(1);
> +
> +	if (exec.nr) {
> +		imply_interactive(&options, "--exec");
> +
> +		strbuf_reset(&buf);
> +		for (i = 0; i < exec.nr; i++)
> +			strbuf_addf(&buf, "exec %s\n", exec.items[i].string);
> +		options.cmd = xstrdup(buf.buf);
> +	}
> +
>  	switch (action) {
>  	case ACTION_CONTINUE: {
>  		struct object_id head;
> @@ -1731,10 +1744,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		}
>  	}
>  
> -	for (i = 0; i < exec.nr; i++)
> -		if (check_exec_cmd(exec.items[i].string))
> -			exit(1);
> -
>  	if (!(options.flags & REBASE_NO_QUIET))
>  		argv_array_push(&options.git_am_opts, "-q");
>  
> @@ -1746,15 +1755,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
>  	}
>  
> -	if (exec.nr) {
> -		imply_interactive(&options, "--exec");
> -
> -		strbuf_reset(&buf);
> -		for (i = 0; i < exec.nr; i++)
> -			strbuf_addf(&buf, "exec %s\n", exec.items[i].string);
> -		options.cmd = xstrdup(buf.buf);
> -	}
> -
>  	if (rebase_merges) {
>  		if (!*rebase_merges)
>  			; /* default mode; do nothing */

Re: [RFC PATCH v2 2/4] rebase: prepare cmd before choosing action

[Andrei Rybak]

On 2019-11-21 03:00, Junio C Hamano wrote:
> Andrei Rybak <rybak.a.v@gmail.com> writes:
>> When git rebase is started with option --exec, its arguments are parsed
>> into string_list exec and then converted into options.cmd.
>>
>> In following commits, action --edit-todo will be taught to use arguments
>> passed with --exec option.  Prepare options.cmd before switch (action)
>> to make it available for the ACTION_EDIT_TODO branch of the switch.
> Hmph.  With or without this change, when we hit the run_rebase label
> in this function and call into run_rebase_interactive(), opts->cmd
> does contain what came from the --exec option.  In that function, I
> see ACTION_EDIT_TODO calls edit_todo_file() that edits the on-disk
> file without paying attention to opts->cmd (the only thing in the
> function that pays attention to this field is ACTION_ADD_EXEC).
>
> So I am not sure what makes this step necessary.  I guess it is not
> wrong per-se, but if the objetive of this series is to add what
> came from the --exec option when the user interacts with the editor
> in rebase-interactive.c::edit_todo_list(), wouldn't it be sufficient
> to skip this step, pass opts to edit_todo_file() and let the helper
> use opts->cmd while preparing the todo_list it passes to underlying
> edit_todo_list() function?
>
> I am not claiming that it would be a better way---I wouldn't be
> surprised if it is an incorrect approach---but it is unclear why
> this step is needed and why the tweak of the todo list must be done
> in the "switch (action)" we see in the post context of the first
> hunk in this patch.

I would guess that it had something to do with passing this value to the helper
binary rebase--interactive before libification.  I couldn't figure out this
mechanism before commit 460bc3ce73 ("rebase -i: run without forking
rebase--interactive", 2019-04-17), so that's just a guess.

I will look into possible simplification of this to avoid the chain of
conversions string_list -> char * -> string_list.

> Thanks for working on this.

Thank you for review :-)