[GSoC][PATCH 0/3] rebase -i: rewrite reflog operations in C

[GSoC][PATCH 0/3] rebase -i: rewrite reflog operations in C

[Alban Gruin]

This patch series rewrites the reflog operations from shell to C.  This
is part of the effort to rewrite interactive rebase in C.

The first commit is dedicated to creating a function to silence a
command, as the sequencer will do in several places with these patches.

This branch is based on ag/rebase-i-rewrite-todo, and does not conflict
with pu (as of 2018-06-18).

Alban Gruin (3):
  sequencer: add a new function to silence a command, except if it
    fails.
  rebase -i: rewrite setup_reflog_action() in C
  rebase -i: rewrite checkout_onto() in C

 builtin/rebase--helper.c   |  14 +++++-
 git-rebase--interactive.sh |  39 +++--------------
 sequencer.c                | 103 +++++++++++++++++++++++++++++++++------------
 sequencer.h                |   6 +++
 4 files changed, 100 insertions(+), 62 deletions(-)

-- 
2.16.4

[GSoC][PATCH 1/3] sequencer: add a new function to silence a command, except if it fails.

[Alban Gruin]

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

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

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

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

[GSoC][PATCH 2/3] rebase -i: rewrite setup_reflog_action() in C

[Alban Gruin]

This rewrites setup_reflog_action() from shell to C.

A new command is added to rebase--helper.c, “setup-reflog”, as such as a
new flag, “verbose”, to silence the output of the checkout operation
called by setup_reflog_action().

The shell version is then stripped in favour of a call to the helper. As
$GIT_REFLOG_ACTION is not longer set at the first call of
checkout_onto(), a call to comment_for_reflog() is added at the
beginning of this function.

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

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index d2990b210..d677fb663 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts = REPLAY_OPTS_INIT;
-	unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;
 	int abbreviate_commands = 0, rebase_cousins = -1;
 	enum {
 		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
 		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
-		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
+		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
 		OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
 			 N_("keep original branch points of cousins")),
+		OPT_BOOL(0, "verbose", &verbose, N_("verbose")),
 		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
 				CONTINUE),
 		OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
@@ -50,6 +51,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		OPT_CMDMODE(0, "edit-todo", &command,
 			    N_("edit the todo list during an interactive rebase"),
 			    EDIT_TODO),
+		OPT_CMDMODE(0, "setup-reflog", &command,
+			    N_("setup the reflog action"), SETUP_REFLOG),
 		OPT_END()
 	};
 
@@ -93,5 +96,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!append_todo_help(0, keep_empty);
 	if (command == EDIT_TODO && argc == 1)
 		return !!edit_todo_list(flags);
+	if (command == SETUP_REFLOG && argc == 2)
+		return !!setup_reflog_action(&opts, argv[1], verbose);
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2defe607f..048bbf041 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -72,6 +72,7 @@ collapse_todo_ids() {
 
 # Switch to the branch in $into and notify it in the reflog
 checkout_onto () {
+	comment_for_reflog start
 	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
 	output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
 	git update-ref ORIG_HEAD $orig_head
@@ -119,19 +120,6 @@ initiate_action () {
 	esac
 }
 
-setup_reflog_action () {
-	comment_for_reflog start
-
-	if test ! -z "$switch_to"
-	then
-		GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
-		output git checkout "$switch_to" -- ||
-			die "$(eval_gettext "Could not checkout \$switch_to")"
-
-		comment_for_reflog start
-	fi
-}
-
 init_basic_state () {
 	orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
 	mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
@@ -211,7 +199,7 @@ git_rebase__interactive () {
 		return 0
 	fi
 
-	setup_reflog_action
+	git rebase--helper --setup-reflog "$switch_to" ${verbose:+--verbose}
 	init_basic_state
 
 	init_revisions_and_shortrevisions
diff --git a/sequencer.c b/sequencer.c
index 3437673d2..4bfe29c7b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3131,6 +3131,37 @@ static const char *reflog_message(struct replay_opts *opts,
 	return buf.buf;
 }
 
+static int checkout_base_commit(struct replay_opts *opts, const char *commit,
+				int verbose, const char *action)
+{
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	cmd.git_cmd = 1;
+
+	argv_array_push(&cmd.args, "checkout");
+	argv_array_push(&cmd.args, commit);
+	argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
+
+	if (verbose)
+		return run_command(&cmd);
+	else
+		return run_command_silent_on_success(&cmd);
+}
+
+int setup_reflog_action(struct replay_opts *opts, const char *commit,
+			int verbose)
+{
+	const char *action;
+
+	if (commit && *commit) {
+		action = reflog_message(opts, "start", "checkout %s", commit);
+		if (checkout_base_commit(opts, commit, verbose, action))
+			die(_("Could not checkout %s"), commit);
+	}
+
+	return 0;
+}
+
 static const char rescheduled_advice[] =
 N_("Could not execute the todo command\n"
 "\n"
diff --git a/sequencer.h b/sequencer.h
index 35730b13e..55e4057d8 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -100,6 +100,9 @@ int update_head_with_reflog(const struct commit *old_head,
 void commit_post_rewrite(const struct commit *current_head,
 			 const struct object_id *new_head);
 
+int setup_reflog_action(struct replay_opts *opts, const char *commit,
+			int verbose);
+
 #define SUMMARY_INITIAL_COMMIT   (1 << 0)
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
 void print_commit_summary(const char *prefix, const struct object_id *oid,
-- 
2.16.4

[GSoC][PATCH 3/3] rebase -i: rewrite checkout_onto() in C

[Alban Gruin]

This rewrites checkout_onto() from shell to C.

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

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

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

Re: [GSoC][PATCH 1/3] sequencer: add a new function to silence a command, except if it fails.

[Phillip Wood]

Hi Alban

On 18/06/18 14:18, Alban Gruin wrote:
> This adds a new function, run_command_silent_if_successful(), to
> redirect the stdout and stderr of a command to a strbuf, and then to run
> that command. This strbuf is printed only if the command fails. It is
> functionnaly similar to output() from git-rebase.sh.

s/functionnaly/functionally/

It's not quite the same though because the shell versions handles 
--verbose where as here the caller has to put that check in every call 
site. I wonder if it would simplify the callers if the C version did the 
--verbose handling it's self.


Best Wishes

Phillip
> run_git_commit() is then refactored to use of
> run_command_silent_if_successful().
> 
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>   sequencer.c | 53 +++++++++++++++++++++++++++--------------------------
>   1 file changed, 27 insertions(+), 26 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 7cc76332e..3437673d2 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -766,6 +766,25 @@ N_("you have staged changes in your working tree\n"
>   #define VERIFY_MSG  (1<<4)
>   #define CREATE_ROOT_COMMIT (1<<5)
>   
> +static int run_command_silent_on_success(struct child_process *cmd)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	int rc;
> +
> +	/* hide stderr on success */
> +	cmd->stdout_to_stderr = 1;
> +	rc = pipe_command(cmd,
> +			  NULL, 0,
> +			  /* stdout is already redirected */
> +			  NULL, 0,
> +			  &buf, 0);
> +
> +	if (rc)
> +		fputs(buf.buf, stderr);
> +	strbuf_release(&buf);
> +	return rc;
> +}
> +
>   /*
>    * If we are cherry-pick, and if the merge did not result in
>    * hand-editing, we will hit this commit and inherit the original
> @@ -820,18 +839,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>   
>   	cmd.git_cmd = 1;
>   
> -	if (is_rebase_i(opts)) {
> -		if (!(flags & EDIT_MSG)) {
> -			cmd.stdout_to_stderr = 1;
> -			cmd.err = -1;
> -		}
> +	if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {
> +		const char *gpg_opt = gpg_sign_opt_quoted(opts);
>   
> -		if (read_env_script(&cmd.env_array)) {
> -			const char *gpg_opt = gpg_sign_opt_quoted(opts);
> -
> -			return error(_(staged_changes_advice),
> -				     gpg_opt, gpg_opt);
> -		}
> +		return error(_(staged_changes_advice),
> +			     gpg_opt, gpg_opt);
>   	}
>   
>   	argv_array_push(&cmd.args, "commit");
> @@ -861,21 +873,10 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>   	if (opts->allow_empty_message)
>   		argv_array_push(&cmd.args, "--allow-empty-message");
>   
> -	if (cmd.err == -1) {
> -		/* hide stderr on success */
> -		struct strbuf buf = STRBUF_INIT;
> -		int rc = pipe_command(&cmd,
> -				      NULL, 0,
> -				      /* stdout is already redirected */
> -				      NULL, 0,
> -				      &buf, 0);
> -		if (rc)
> -			fputs(buf.buf, stderr);
> -		strbuf_release(&buf);
> -		return rc;
> -	}
> -
> -	return run_command(&cmd);
> +	if (is_rebase_i(opts) && !(flags & EDIT_MSG))
> +		return run_command_silent_on_success(&cmd);
> +	else
> +		return run_command(&cmd);
>   }
>   
>   static int rest_is_empty(const struct strbuf *sb, int start)
> 

Re: [GSoC][PATCH 2/3] rebase -i: rewrite setup_reflog_action() in C

[Phillip Wood]

On 18/06/18 14:18, Alban Gruin wrote:
> This rewrites setup_reflog_action() from shell to C.
> 
> A new command is added to rebase--helper.c, “setup-reflog”, as such as a
> new flag, “verbose”, to silence the output of the checkout operation
> called by setup_reflog_action().

I'm having difficulty understanding what that means, surely the verbose 
flag is to stop the output from being silenced.

I'm not that keen on the naming in this patch, if it's only a staging 
post and will be removed it probably doesn't matter too much. I can see 
why you've based the function and flag names on the shell version, but 
the C version is not setting up the reflog it is checking out the branch 
we're rebasing. --checkout-base or something like that would be clearer.

Also the name of the function that does the checkout is tied to checking 
out the base revision, but then reused in the next patch to checkout the 
'onto' commit. As such I think it would be clearer if it was called 
run_git_checkout() or something like that.

One further thought - how easy would it be to refactor the code in 
do_reset() to handle the checkouts in this patch series, rather than 
having to fork 'git checkout'

Best wishes

Phillip

> The shell version is then stripped in favour of a call to the helper. As
> $GIT_REFLOG_ACTION is not longer set at the first call of
> checkout_onto(), a call to comment_for_reflog() is added at the
> beginning of this function.
> 
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>   builtin/rebase--helper.c   |  9 +++++++--
>   git-rebase--interactive.sh | 16 ++--------------
>   sequencer.c                | 31 +++++++++++++++++++++++++++++++
>   sequencer.h                |  3 +++
>   4 files changed, 43 insertions(+), 16 deletions(-)
> 
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index d2990b210..d677fb663 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {
>   int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>   {
>   	struct replay_opts opts = REPLAY_OPTS_INIT;
> -	unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
> +	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;
>   	int abbreviate_commands = 0, rebase_cousins = -1;
>   	enum {
>   		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
>   		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
> -		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
> +		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG
>   	} command = 0;
>   	struct option options[] = {
>   		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
> @@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>   		OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
>   		OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
>   			 N_("keep original branch points of cousins")),
> +		OPT_BOOL(0, "verbose", &verbose, N_("verbose")),
>   		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
>   				CONTINUE),
>   		OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
> @@ -50,6 +51,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>   		OPT_CMDMODE(0, "edit-todo", &command,
>   			    N_("edit the todo list during an interactive rebase"),
>   			    EDIT_TODO),
> +		OPT_CMDMODE(0, "setup-reflog", &command,
> +			    N_("setup the reflog action"), SETUP_REFLOG),
>   		OPT_END()
>   	};
>   
> @@ -93,5 +96,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>   		return !!append_todo_help(0, keep_empty);
>   	if (command == EDIT_TODO && argc == 1)
>   		return !!edit_todo_list(flags);
> +	if (command == SETUP_REFLOG && argc == 2)
> +		return !!setup_reflog_action(&opts, argv[1], verbose);
>   	usage_with_options(builtin_rebase_helper_usage, options);
>   }
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 2defe607f..048bbf041 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -72,6 +72,7 @@ collapse_todo_ids() {
>   
>   # Switch to the branch in $into and notify it in the reflog
>   checkout_onto () {
> +	comment_for_reflog start
>   	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
>   	output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
>   	git update-ref ORIG_HEAD $orig_head
> @@ -119,19 +120,6 @@ initiate_action () {
>   	esac
>   }
>   
> -setup_reflog_action () {
> -	comment_for_reflog start
> -
> -	if test ! -z "$switch_to"
> -	then
> -		GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
> -		output git checkout "$switch_to" -- ||
> -			die "$(eval_gettext "Could not checkout \$switch_to")"
> -
> -		comment_for_reflog start
> -	fi
> -}
> -
>   init_basic_state () {
>   	orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
>   	mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
> @@ -211,7 +199,7 @@ git_rebase__interactive () {
>   		return 0
>   	fi
>   
> -	setup_reflog_action
> +	git rebase--helper --setup-reflog "$switch_to" ${verbose:+--verbose}
>   	init_basic_state
>   
>   	init_revisions_and_shortrevisions
> diff --git a/sequencer.c b/sequencer.c
> index 3437673d2..4bfe29c7b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3131,6 +3131,37 @@ static const char *reflog_message(struct replay_opts *opts,
>   	return buf.buf;
>   }
>   
> +static int checkout_base_commit(struct replay_opts *opts, const char *commit,
> +				int verbose, const char *action)
> +{
> +	struct child_process cmd = CHILD_PROCESS_INIT;
> +
> +	cmd.git_cmd = 1;
> +
> +	argv_array_push(&cmd.args, "checkout");
> +	argv_array_push(&cmd.args, commit);
> +	argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
> +
> +	if (verbose)
> +		return run_command(&cmd);
> +	else
> +		return run_command_silent_on_success(&cmd);
> +}
> +
> +int setup_reflog_action(struct replay_opts *opts, const char *commit,
> +			int verbose)
> +{
> +	const char *action;
> +
> +	if (commit && *commit) {
> +		action = reflog_message(opts, "start", "checkout %s", commit);
> +		if (checkout_base_commit(opts, commit, verbose, action))
> +			die(_("Could not checkout %s"), commit);
> +	}
> +
> +	return 0;
> +}
> +
>   static const char rescheduled_advice[] =
>   N_("Could not execute the todo command\n"
>   "\n"
> diff --git a/sequencer.h b/sequencer.h
> index 35730b13e..55e4057d8 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -100,6 +100,9 @@ int update_head_with_reflog(const struct commit *old_head,
>   void commit_post_rewrite(const struct commit *current_head,
>   			 const struct object_id *new_head);
>   
> +int setup_reflog_action(struct replay_opts *opts, const char *commit,
> +			int verbose);
> +
>   #define SUMMARY_INITIAL_COMMIT   (1 << 0)
>   #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
>   void print_commit_summary(const char *prefix, const struct object_id *oid,
> 

Re: [GSoC][PATCH 3/3] rebase -i: rewrite checkout_onto() in C

[Phillip Wood]

Hi Alban

On 18/06/18 14:18, Alban Gruin wrote:
> This rewrites checkout_onto() from shell to C.
> 
> A new command (“checkout-onto”) is added to rebase--helper.c. The shell
> version is then stripped.
> 
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>   builtin/rebase--helper.c   |  7 ++++++-
>   git-rebase--interactive.sh | 25 ++++---------------------
>   sequencer.c                | 19 +++++++++++++++++++
>   sequencer.h                |  3 +++
>   4 files changed, 32 insertions(+), 22 deletions(-)
> 
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index d677fb663..f9fffba96 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -17,7 +17,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>   	enum {
>   		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
>   		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
> -		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG
> +		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG,
> +		CHECKOUT_ONTO
>   	} command = 0;
>   	struct option options[] = {
>   		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
> @@ -53,6 +54,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>   			    EDIT_TODO),
>   		OPT_CMDMODE(0, "setup-reflog", &command,
>   			    N_("setup the reflog action"), SETUP_REFLOG),
> +		OPT_CMDMODE(0, "checkout-onto", &command,
> +			    N_("checkout a commit"), CHECKOUT_ONTO),
>   		OPT_END()
>   	};
>   
> @@ -98,5 +101,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>   		return !!edit_todo_list(flags);
>   	if (command == SETUP_REFLOG && argc == 2)
>   		return !!setup_reflog_action(&opts, argv[1], verbose);
> +	if (command == CHECKOUT_ONTO && argc == 4)
> +		return !!checkout_onto(&opts, argv[1], argv[2], argv[3], verbose);
>   	usage_with_options(builtin_rebase_helper_usage, options);
>   }
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 048bbf041..0ae053291 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -28,17 +28,6 @@ case "$comment_char" in
>   	;;
>   esac
>   
> -orig_reflog_action="$GIT_REFLOG_ACTION"
> -
> -comment_for_reflog () {
> -	case "$orig_reflog_action" in
> -	''|rebase*)
> -		GIT_REFLOG_ACTION="rebase -i ($1)"
> -		export GIT_REFLOG_ACTION
> -		;;
> -	esac
> -}
> -
>   die_abort () {
>   	apply_autostash
>   	rm -rf "$state_dir"
> @@ -70,14 +59,6 @@ collapse_todo_ids() {
>   	git rebase--helper --shorten-ids
>   }
>   
> -# Switch to the branch in $into and notify it in the reflog
> -checkout_onto () {
> -	comment_for_reflog start
> -	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
> -	output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
> -	git update-ref ORIG_HEAD $orig_head
> -}
> -
>   get_missing_commit_check_level () {
>   	check_level=$(git config --get rebase.missingCommitsCheck)
>   	check_level=${check_level:-ignore}
> @@ -176,7 +157,8 @@ EOF
>   
>   	git rebase--helper --check-todo-list || {
>   		ret=$?
> -		checkout_onto
> +		git rebase--helper --checkout-onto "$onto_name" "$onto" \
> +		    "$orig_head" ${verbose:+--verbose}
>   		exit $ret
>   	}
>   
> @@ -186,7 +168,8 @@ EOF
>   	onto="$(git rebase--helper --skip-unnecessary-picks)" ||
>   	die "Could not skip unnecessary pick commands"
>   
> -	checkout_onto
> +	git rebase--helper --checkout-onto "$onto_name" "$onto" "$orig_head" \
> +	    ${verbose:+--verbose}
>   	require_clean_work_tree "rebase"
>   	exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
>   	     --continue
> diff --git a/sequencer.c b/sequencer.c
> index 4bfe29c7b..d149cbf92 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3162,6 +3162,25 @@ int setup_reflog_action(struct replay_opts *opts, const char *commit,
>   	return 0;
>   }
>   
> +int checkout_onto(struct replay_opts *opts,
> +		  const char *onto_name, const char *onto,
> +		  const char *orig_head, unsigned verbose)
> +{
> +	struct object_id oid;
> +	const char *action = reflog_message(opts, "start", "checkout %s", onto_name);
> +
> +	if (get_oid(orig_head, &oid))
> +		die(_("%s: not a valid OID"), orig_head);

If this code is going to live long-term in sequencer.c then it would be 
better not to die, but return an error instead as it's part of libgit.

Best Wishes

Phillip

> +
> +	if (checkout_base_commit(opts, onto, verbose, action)) {
> +		apply_autostash(opts);
> +		sequencer_remove_state(opts);
> +		die(_("could not detach HEAD"));
> +	}
> +
> +	return update_ref(NULL, "ORIG_HEAD", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);
> +}
> +
>   static const char rescheduled_advice[] =
>   N_("Could not execute the todo command\n"
>   "\n"
> diff --git a/sequencer.h b/sequencer.h
> index 55e4057d8..9899d90fc 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -102,6 +102,9 @@ void commit_post_rewrite(const struct commit *current_head,
>   
>   int setup_reflog_action(struct replay_opts *opts, const char *commit,
>   			int verbose);
> +int checkout_onto(struct replay_opts *opts,
> +		  const char *onto_name, const char *onto,
> +		  const char *orig_head, unsigned verbose);
>   
>   #define SUMMARY_INITIAL_COMMIT   (1 << 0)
>   #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
> 

Re: [GSoC][PATCH 1/3] sequencer: add a new function to silence a command, except if it fails.

[Christian Couder]

Hi Alban,

On Mon, Jun 18, 2018 at 3:18 PM, Alban Gruin <alban.gruin@gmail.com> wrote:
> This adds a new function, run_command_silent_if_successful(),

He re the function is called run_command_silent_if_successful()...

> to
> redirect the stdout and stderr of a command to a strbuf, and then to run
> that command. This strbuf is printed only if the command fails. It is
> functionnaly similar to output() from git-rebase.sh.
>
> run_git_commit() is then refactored to use of
> run_command_silent_if_successful().

...here also...

[...]

> +static int run_command_silent_on_success(struct child_process *cmd)

...but here it is called run_command_silent_on_success().

Thanks,
Christian.

Re: [GSoC][PATCH 1/3] sequencer: add a new function to silence a command, except if it fails.

[Alban Gruin]

Hi Phillip,

Le 18/06/2018 à 17:26, Phillip Wood a écrit :
> Hi Alban
> 
> On 18/06/18 14:18, Alban Gruin wrote:
>> This adds a new function, run_command_silent_if_successful(), to
>> redirect the stdout and stderr of a command to a strbuf, and then to run
>> that command. This strbuf is printed only if the command fails. It is
>> functionnaly similar to output() from git-rebase.sh.
> 
> s/functionnaly/functionally/
> 
> It's not quite the same though because the shell versions handles
> --verbose where as here the caller has to put that check in every call
> site. I wonder if it would simplify the callers if the C version did the
> --verbose handling it's self.
> 

That’s what I did first, but removed it because I thought it would be
less confusing.  I’m fine with this solution, though.  Do you want me to
do this instead?

Cheers,
Alban

Re: [GSoC][PATCH 2/3] rebase -i: rewrite setup_reflog_action() in C

[Alban Gruin]

Hi Phillip,

Le 18/06/2018 à 17:34, Phillip Wood a écrit :
> On 18/06/18 14:18, Alban Gruin wrote:
>> This rewrites setup_reflog_action() from shell to C.
>>
>> A new command is added to rebase--helper.c, “setup-reflog”, as such as a
>> new flag, “verbose”, to silence the output of the checkout operation
>> called by setup_reflog_action().
> 
> I'm having difficulty understanding what that means, surely the verbose
> flag is to stop the output from being silenced.
> 

I reversed the meaning of the flag, I will correct it in a reroll.

> I'm not that keen on the naming in this patch, if it's only a staging
> post and will be removed it probably doesn't matter too much. I can see
> why you've based the function and flag names on the shell version, but
> the C version is not setting up the reflog it is checking out the branch
> we're rebasing. --checkout-base or something like that would be clearer.
> 
> Also the name of the function that does the checkout is tied to checking
> out the base revision, but then reused in the next patch to checkout the
> 'onto' commit. As such I think it would be clearer if it was called
> run_git_checkout() or something like that.
> 

Right.

> One further thought - how easy would it be to refactor the code in
> do_reset() to handle the checkouts in this patch series, rather than
> having to fork 'git checkout'
> 

Good remark, I did not notice do_reset().  I intend to refactor and
optimize after I have done (most of) the conversion, so this goes into
my todo-list, but it does not really seem difficult at first sight.

Cheers,
Alban

Re: [GSoC][PATCH 3/3] rebase -i: rewrite checkout_onto() in C

[Alban Gruin]

Hi Phillip,

Le 18/06/2018 à 18:09, Phillip Wood a écrit :
>> +    if (get_oid(orig_head, &oid))
>> +        die(_("%s: not a valid OID"), orig_head);
> 
> If this code is going to live long-term in sequencer.c then it would be
> better not to die, but return an error instead as it's part of libgit.
> 

Right, I will fix this.

Cheers,
Alban

Re: [GSoC][PATCH 1/3] sequencer: add a new function to silence a command, except if it fails.

[Alban Gruin]

Hi Christian,

Le 18/06/2018 à 18:26, Christian Couder a écrit :
> Hi Alban,
> 
> On Mon, Jun 18, 2018 at 3:18 PM, Alban Gruin <alban.gruin@gmail.com> wrote:
>> This adds a new function, run_command_silent_if_successful(),
> 
> He re the function is called run_command_silent_if_successful()...
> 
>> to
>> redirect the stdout and stderr of a command to a strbuf, and then to run
>> that command. This strbuf is printed only if the command fails. It is
>> functionnaly similar to output() from git-rebase.sh.
>>
>> run_git_commit() is then refactored to use of
>> run_command_silent_if_successful().
> 
> ...here also...
> 
> [...]
> 
>> +static int run_command_silent_on_success(struct child_process *cmd)
> 
> ...but here it is called run_command_silent_on_success().
> 
> Thanks,
> Christian.
> 

Oops, my bad.  I will fix this in a reroll.

Cheers,
Alban

Re: [GSoC][PATCH 2/3] rebase -i: rewrite setup_reflog_action() in C

[Stefan Beller]

On Mon, Jun 18, 2018 at 6:19 AM Alban Gruin <alban.gruin@gmail.com> wrote:
>
> This rewrites setup_reflog_action() from shell to C.
>
> A new command is added to rebase--helper.c, “setup-reflog”, as such as a
> new flag, “verbose”, to silence the output of the checkout operation
> called by setup_reflog_action().
>
> The shell version is then stripped in favour of a call to the helper. As
> $GIT_REFLOG_ACTION is not longer set at the first call of
> checkout_onto(), a call to comment_for_reflog() is added at the
> beginning of this function.
>
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  builtin/rebase--helper.c   |  9 +++++++--
>  git-rebase--interactive.sh | 16 ++--------------
>  sequencer.c                | 31 +++++++++++++++++++++++++++++++
>  sequencer.h                |  3 +++
>  4 files changed, 43 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index d2990b210..d677fb663 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {
>  int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>  {
>         struct replay_opts opts = REPLAY_OPTS_INIT;
> -       unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
> +       unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;
>         int abbreviate_commands = 0, rebase_cousins = -1;
>         enum {
>                 CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
>                 CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
> -               ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
> +               ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG
>         } command = 0;
>         struct option options[] = {
>                 OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
> @@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>                 OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
>                 OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
>                          N_("keep original branch points of cousins")),
> +               OPT_BOOL(0, "verbose", &verbose, N_("verbose")),

verbose is quite a popular flag name, such that the option parsing
dedicated it its own macro OPT__VERBOSE.


> +int setup_reflog_action(struct replay_opts *opts, const char *commit,
> +                       int verbose)
> +{
> +       const char *action;
> +
> +       if (commit && *commit) {

While this is defensive programming (checking the pointer before dereferencing
it, the first condition (commit == NULL) should never be false here,
as the caller
checks for argc == 2 ? Maybe we could move the logic of the whole
condition there

       if (command == SETUP_REFLOG && argc == 2 && *argv[1])
               return !!setup_reflog_action(&opts, argv[1], verbose);

as then we could loose the outer conditional here.

Re: [GSoC][PATCH 2/3] rebase -i: rewrite setup_reflog_action() in C

[Johannes Schindelin]

Hi Stefan,

On Mon, 18 Jun 2018, Stefan Beller wrote:

> On Mon, Jun 18, 2018 at 6:19 AM Alban Gruin <alban.gruin@gmail.com> wrote:
> >
> > +int setup_reflog_action(struct replay_opts *opts, const char *commit,
> > +                       int verbose)
> > +{
> > +       const char *action;
> > +
> > +       if (commit && *commit) {
> 
> While this is defensive programming (checking the pointer before dereferencing
> it, the first condition (commit == NULL) should never be false here,
> as the caller
> checks for argc == 2 ?

But it is not marked as `static` (or is it?). So we should not rely on the
caller to Do The Right Thing.

Ciao,
Dscho

[GSoC][PATCH v2 0/3] rebase -i: rewrite reflog operations in C

[Alban Gruin]

This patch series rewrites the reflog operations from shell to C.  This
is part of the effort to rewrite interactive rebase in C.

The first commit is dedicated to creating a function to silence a
command, as the sequencer will do in several places with these patches.

This branch is based on ag/rebase-i-rewrite-todo, and does not conflict
with pu (as of 2018-06-19).

Changes since v1:

 - Replacing run_command_silent_if_successful() by
   run_command_silent_on_success() in the first commit’s message (thanks
   Christian!)

 - Adding a “verbose” parameter to run_command_silent_on_success()
   (thanks Phillip!)

 - Using OPT__VERBOSE to parse the “--verbose” flag (thanks Stefan!)

 - Fixing some typos and errors in the commit messages

 - Renaming “setup-reflog” to “checkout-base”

 - Renaming checkout_base_commit() to run_git_checkout()

 - Replacing calls to die() by error()

Alban Gruin (3):
  sequencer: add a new function to silence a command, except if it
    fails.
  rebase -i: rewrite setup_reflog_action() in C
  rebase -i: rewrite checkout_onto() in C

 builtin/rebase--helper.c   |  14 ++++++-
 git-rebase--interactive.sh |  39 +++--------------
 sequencer.c                | 102 +++++++++++++++++++++++++++++++++------------
 sequencer.h                |   6 +++
 4 files changed, 99 insertions(+), 62 deletions(-)

-- 
2.16.4

[GSoC][PATCH v2 1/3] sequencer: add a new function to silence a command, except if it fails.

[Alban Gruin]

This adds a new function, run_command_silent_on_success(), to redirect
the stdout and stderr of a command to a strbuf, and then to run that
command. This strbuf is printed only if the command fails. It also takes
a parameter, “verbose”. When true, the command is executed without
redirecting its output. It is functionnally similar to output() from
git-rebase.sh.

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

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

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

[GSoC][PATCH v2 2/3] rebase -i: rewrite setup_reflog_action() in C

[Alban Gruin]

This rewrites setup_reflog_action() from shell to C. The new version is
called checkout_base_commit().

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

The shell version is then stripped in favour of a call to the helper. As
$GIT_REFLOG_ACTION is not longer set at the first call of
checkout_onto(), a call to comment_for_reflog() is added at the
beginning of this function.

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

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index d2990b210..7cd74da2e 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts = REPLAY_OPTS_INIT;
-	unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;
 	int abbreviate_commands = 0, rebase_cousins = -1;
 	enum {
 		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
 		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
-		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
+		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
 		OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
 			 N_("keep original branch points of cousins")),
+		OPT__VERBOSE(&verbose, N_("print subcommands output even if they succeed")),
 		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
 				CONTINUE),
 		OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
@@ -50,6 +51,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		OPT_CMDMODE(0, "edit-todo", &command,
 			    N_("edit the todo list during an interactive rebase"),
 			    EDIT_TODO),
+		OPT_CMDMODE(0, "checkout-base", &command,
+			    N_("checkout the base commit"), CHECKOUT_BASE),
 		OPT_END()
 	};
 
@@ -93,5 +96,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!append_todo_help(0, keep_empty);
 	if (command == EDIT_TODO && argc == 1)
 		return !!edit_todo_list(flags);
+	if (command == CHECKOUT_BASE && argc == 2)
+		return !!checkout_base_commit(&opts, argv[1], verbose);
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2defe607f..af46cf9c2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -72,6 +72,7 @@ collapse_todo_ids() {
 
 # Switch to the branch in $into and notify it in the reflog
 checkout_onto () {
+	comment_for_reflog start
 	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
 	output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
 	git update-ref ORIG_HEAD $orig_head
@@ -119,19 +120,6 @@ initiate_action () {
 	esac
 }
 
-setup_reflog_action () {
-	comment_for_reflog start
-
-	if test ! -z "$switch_to"
-	then
-		GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
-		output git checkout "$switch_to" -- ||
-			die "$(eval_gettext "Could not checkout \$switch_to")"
-
-		comment_for_reflog start
-	fi
-}
-
 init_basic_state () {
 	orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
 	mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
@@ -211,7 +199,7 @@ git_rebase__interactive () {
 		return 0
 	fi
 
-	setup_reflog_action
+	git rebase--helper --checkout-base "$switch_to" ${verbose:+--verbose}
 	init_basic_state
 
 	init_revisions_and_shortrevisions
diff --git a/sequencer.c b/sequencer.c
index 9aa7ddb33..a7a73e3ef 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3133,6 +3133,34 @@ static const char *reflog_message(struct replay_opts *opts,
 	return buf.buf;
 }
 
+static int run_git_checkout(struct replay_opts *opts, const char *commit,
+				int verbose, const char *action)
+{
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	cmd.git_cmd = 1;
+
+	argv_array_push(&cmd.args, "checkout");
+	argv_array_push(&cmd.args, commit);
+	argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
+
+	return run_command_silent_on_success(&cmd, verbose);
+}
+
+int checkout_base_commit(struct replay_opts *opts, const char *commit,
+			 int verbose)
+{
+	const char *action;
+
+	if (commit && *commit) {
+		action = reflog_message(opts, "start", "checkout %s", commit);
+		if (run_git_checkout(opts, commit, verbose, action))
+			return error(_("Could not checkout %s"), commit);
+	}
+
+	return 0;
+}
+
 static const char rescheduled_advice[] =
 N_("Could not execute the todo command\n"
 "\n"
diff --git a/sequencer.h b/sequencer.h
index 35730b13e..42c3dda81 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -100,6 +100,9 @@ int update_head_with_reflog(const struct commit *old_head,
 void commit_post_rewrite(const struct commit *current_head,
 			 const struct object_id *new_head);
 
+int checkout_base_commit(struct replay_opts *opts, const char *commit,
+			 int verbose);
+
 #define SUMMARY_INITIAL_COMMIT   (1 << 0)
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
 void print_commit_summary(const char *prefix, const struct object_id *oid,
-- 
2.16.4

[GSoC][PATCH v2 3/3] rebase -i: rewrite checkout_onto() in C

[Alban Gruin]

This rewrites checkout_onto() from shell to C.

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

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

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

Re: [GSoC][PATCH v2 0/3] rebase -i: rewrite reflog operations in C

[Stefan Beller]

On Tue, Jun 19, 2018 at 8:45 AM Alban Gruin <alban.gruin@gmail.com> wrote:
>
> This patch series rewrites the reflog operations from shell to C.  This
> is part of the effort to rewrite interactive rebase in C.

This series looks good to me.

Thanks,
Stefan

Re: [GSoC][PATCH v2 0/3] rebase -i: rewrite reflog operations in C

[Johannes Schindelin]

Hi Alban,

On Tue, 19 Jun 2018, Alban Gruin wrote:

> -- 
> 2.16.4

You might want to update ;-)

https://github.com/git-for-windows/git/wiki/FAQ#should-i-upgrade-to-a-newer-git-for-windows-version

Ciao,
Johannes

Re: [GSoC][PATCH v2 1/3] sequencer: add a new function to silence a command, except if it fails.

[Johannes Schindelin]

Hi Alban,

On Tue, 19 Jun 2018, Alban Gruin wrote:

> diff --git a/sequencer.c b/sequencer.c
> index 7cc76332e..9aa7ddb33 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -766,6 +766,29 @@ N_("you have staged changes in your working tree\n"
>  #define VERIFY_MSG  (1<<4)
>  #define CREATE_ROOT_COMMIT (1<<5)
>  
> +static int run_command_silent_on_success(struct child_process *cmd,
> +					 unsigned verbose)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	int rc;
> +
> +	if (verbose)
> +		return run_command(cmd);
> +
> +	/* hide stderr on success */
> +	cmd->stdout_to_stderr = 1;

This comment is a bit misleading: that line does not hide stderr on
success, it redirects stdout to stderr instead.

> +	rc = pipe_command(cmd,
> +			  NULL, 0,
> +			  /* stdout is already redirected */
> +			  NULL, 0,
> +			  &buf, 0);
> +
> +	if (rc)
> +		fputs(buf.buf, stderr);
> +	strbuf_release(&buf);
> +	return rc;
> +}
> +
>  /*
>   * If we are cherry-pick, and if the merge did not result in
>   * hand-editing, we will hit this commit and inherit the original
> @@ -820,18 +843,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>  
>  	cmd.git_cmd = 1;
>  
> -	if (is_rebase_i(opts)) {
> -		if (!(flags & EDIT_MSG)) {
> -			cmd.stdout_to_stderr = 1;
> -			cmd.err = -1;
> -		}

This code made sure that we *only* do this redirection, and stderr
buffering, if `git commit` is called non-interactively. When it is called
interactively, redirecting stdout and stderr is absolutely not what we
want.

> +	if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {
> +		const char *gpg_opt = gpg_sign_opt_quoted(opts);
>  
> -		if (read_env_script(&cmd.env_array)) {
> -			const char *gpg_opt = gpg_sign_opt_quoted(opts);
> -
> -			return error(_(staged_changes_advice),
> -				     gpg_opt, gpg_opt);
> -		}
> +		return error(_(staged_changes_advice),
> +			     gpg_opt, gpg_opt);
>  	}
>  
>  	argv_array_push(&cmd.args, "commit");
> @@ -861,21 +877,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>  	if (opts->allow_empty_message)
>  		argv_array_push(&cmd.args, "--allow-empty-message");
>  
> -	if (cmd.err == -1) {
> -		/* hide stderr on success */
> -		struct strbuf buf = STRBUF_INIT;
> -		int rc = pipe_command(&cmd,
> -				      NULL, 0,
> -				      /* stdout is already redirected */
> -				      NULL, 0,
> -				      &buf, 0);
> -		if (rc)
> -			fputs(buf.buf, stderr);
> -		strbuf_release(&buf);
> -		return rc;
> -	}
> -
> -	return run_command(&cmd);
> +	return run_command_silent_on_success(&cmd,
> +					     !(is_rebase_i(opts) && !(flags & EDIT_MSG)));

It would probably make more sense to change the signature of
`run_command_silent_on_success()` to not even take the `int verbose`
parameter: why call it "silent on success" when we can ask it *not* to be
silent on success?

And then you can avoid this overly-long line (as well as the quite
convoluted Boolean logic that took me a couple of seconds to verify) very
elegantly by this code:

	if (is_rebase_i(opts) && !(flags & EDIT_MSG))
		return run_command_silent_on_success(&cmd);
	return run_command(&cmd);

I vaguely recall that I wanted to make this an option in the `struct
child_process` when I originally introduced this code, but I think it was
Peff who suggested that doing it "by hand" was the more appropriate way
here because I use it only once.

My recollection might fail me, but if it is correct, maybe that would be a
good way forward, to make this an `int silent_on_success:1;` field?

Ciao,
Dscho

Re: [GSoC][PATCH v2 2/3] rebase -i: rewrite setup_reflog_action() in C

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 4392 bytes --]

Hi Alban,


On Tue, 19 Jun 2018, Alban Gruin wrote:

> This rewrites setup_reflog_action() from shell to C. The new version is
> called checkout_base_commit().

This sounds like a serious mistake. How is it that a function that sets up
a reflog action all of a sudden checks out a base commit?

But you are correct, of course, that is exactly what the shell script
function does!

So maybe insert "the (misnamed)" after "This rewrites"? That would help me
in the future when I stumble across this commit again.

> A new command is added to rebase--helper.c, “checkout-base”, as such as

s/as such as/as well as/

> a new flag, “verbose”, to avoid silencing the output of the checkout
> operation called by checkout_base_commit().
> 
> The shell version is then stripped in favour of a call to the helper. As

Please start a new paragraph after this sentence, before the "As ...",
because those two sentences represent different thoughts.

> $GIT_REFLOG_ACTION is not longer set at the first call of

s/not longer/no longer/

> checkout_onto(), a call to comment_for_reflog() is added at the
> beginning of this function.
> 
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  builtin/rebase--helper.c   |  9 +++++++--
>  git-rebase--interactive.sh | 16 ++--------------
>  sequencer.c                | 28 ++++++++++++++++++++++++++++
>  sequencer.h                |  3 +++
>  4 files changed, 40 insertions(+), 16 deletions(-)
> 
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index d2990b210..7cd74da2e 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {
>  int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>  {
>  	struct replay_opts opts = REPLAY_OPTS_INIT;
> -	unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
> +	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;
>  	int abbreviate_commands = 0, rebase_cousins = -1;
>  	enum {
>  		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
>  		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
> -		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
> +		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE
>  	} command = 0;
>  	struct option options[] = {
>  		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
> @@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>  		OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
>  		OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
>  			 N_("keep original branch points of cousins")),
> +		OPT__VERBOSE(&verbose, N_("print subcommands output even if they succeed")),

That's very specific a description that applies only to this command
mode... Maybe just say `"be verbose"`, and as a bonus avoid a too-long
line?

> diff --git a/sequencer.c b/sequencer.c
> index 9aa7ddb33..a7a73e3ef 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3133,6 +3133,34 @@ static const char *reflog_message(struct replay_opts *opts,
>  	return buf.buf;
>  }
>  
> +static int run_git_checkout(struct replay_opts *opts, const char *commit,
> +				int verbose, const char *action)
> +{
> +	struct child_process cmd = CHILD_PROCESS_INIT;
> +
> +	cmd.git_cmd = 1;
> +
> +	argv_array_push(&cmd.args, "checkout");
> +	argv_array_push(&cmd.args, commit);
> +	argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
> +
> +	return run_command_silent_on_success(&cmd, verbose);
> +}

Are you planning on reusing that code anywhere else than in
`checkout_base_commit()`? If yes, sure, it makes sense to keep as separate
function. Otherwise I would fold it into that latter function, for reading
simplicity.

> +
> +int checkout_base_commit(struct replay_opts *opts, const char *commit,
> +			 int verbose)
> +{
> +	const char *action;
> +
> +	if (commit && *commit) {
> +		action = reflog_message(opts, "start", "checkout %s", commit);
> +		if (run_git_checkout(opts, commit, verbose, action))
> +			return error(_("Could not checkout %s"), commit);
> +	}
> +
> +	return 0;
> +}
> +
>  static const char rescheduled_advice[] =
>  N_("Could not execute the todo command\n"
>  "\n"

The rest looks fine to me, thank you!
Dscho

Re: [GSoC][PATCH v2 3/3] rebase -i: rewrite checkout_onto() in C

[Johannes Schindelin]

Hi Alban,

On Tue, 19 Jun 2018, Alban Gruin wrote:

> diff --git a/sequencer.c b/sequencer.c
> index a7a73e3ef..9165bf96c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3161,6 +3161,25 @@ int checkout_base_commit(struct replay_opts *opts, const char *commit,
>  	return 0;
>  }
>  
> +int checkout_onto(struct replay_opts *opts,
> +		  const char *onto_name, const char *onto,
> +		  const char *orig_head, unsigned verbose)
> +{
> +	struct object_id oid;
> +	const char *action = reflog_message(opts, "start", "checkout %s", onto_name);
> +
> +	if (get_oid(orig_head, &oid))
> +		return error(_("%s: not a valid OID"), orig_head);
> +
> +	if (run_git_checkout(opts, onto, verbose, action)) {

Ah, so this is the reason for the split.

If you send a new iteration of this patch series, could you do me a favor
and add a paragraph to the commit message of 2/3, saying something like
this: "The function `run_git_checkout()` will be used on its own in the
next commit, therefore the code is not folded into
`checkout_base_commit()`? That way, I do not have to burden my working
memory with this bit of information ;-)

Thanks,
Dscho

Re: [GSoC][PATCH v2 1/3] sequencer: add a new function to silence a command, except if it fails.

[Alban Gruin]

Hi Johannes,

Le 21/06/2018 à 11:37, Johannes Schindelin a écrit :
> Hi Alban,
> 
> On Tue, 19 Jun 2018, Alban Gruin wrote:
> 
>> diff --git a/sequencer.c b/sequencer.c
>> index 7cc76332e..9aa7ddb33 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -766,6 +766,29 @@ N_("you have staged changes in your working tree\n"
>>  #define VERIFY_MSG  (1<<4)
>>  #define CREATE_ROOT_COMMIT (1<<5)
>>  
>> +static int run_command_silent_on_success(struct child_process *cmd,
>> +					 unsigned verbose)
>> +{
>> +	struct strbuf buf = STRBUF_INIT;
>> +	int rc;
>> +
>> +	if (verbose)
>> +		return run_command(cmd);
>> +
>> +	/* hide stderr on success */
>> +	cmd->stdout_to_stderr = 1;
> 
> This comment is a bit misleading: that line does not hide stderr on
> success, it redirects stdout to stderr instead.
> 
>> +	rc = pipe_command(cmd,
>> +			  NULL, 0,
>> +			  /* stdout is already redirected */
>> +			  NULL, 0,
>> +			  &buf, 0);
>> +
>> +	if (rc)
>> +		fputs(buf.buf, stderr);
>> +	strbuf_release(&buf);
>> +	return rc;
>> +}
>> +
>>  /*
>>   * If we are cherry-pick, and if the merge did not result in
>>   * hand-editing, we will hit this commit and inherit the original
>> @@ -820,18 +843,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>>  
>>  	cmd.git_cmd = 1;
>>  
>> -	if (is_rebase_i(opts)) {
>> -		if (!(flags & EDIT_MSG)) {
>> -			cmd.stdout_to_stderr = 1;
>> -			cmd.err = -1;
>> -		}
> 
> This code made sure that we *only* do this redirection, and stderr
> buffering, if `git commit` is called non-interactively. When it is called
> interactively, redirecting stdout and stderr is absolutely not what we
> want.
> 
>> +	if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {
>> +		const char *gpg_opt = gpg_sign_opt_quoted(opts);
>>  
>> -		if (read_env_script(&cmd.env_array)) {
>> -			const char *gpg_opt = gpg_sign_opt_quoted(opts);
>> -
>> -			return error(_(staged_changes_advice),
>> -				     gpg_opt, gpg_opt);
>> -		}
>> +		return error(_(staged_changes_advice),
>> +			     gpg_opt, gpg_opt);
>>  	}
>>  
>>  	argv_array_push(&cmd.args, "commit");
>> @@ -861,21 +877,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>>  	if (opts->allow_empty_message)
>>  		argv_array_push(&cmd.args, "--allow-empty-message");
>>  
>> -	if (cmd.err == -1) {
>> -		/* hide stderr on success */
>> -		struct strbuf buf = STRBUF_INIT;
>> -		int rc = pipe_command(&cmd,
>> -				      NULL, 0,
>> -				      /* stdout is already redirected */
>> -				      NULL, 0,
>> -				      &buf, 0);
>> -		if (rc)
>> -			fputs(buf.buf, stderr);
>> -		strbuf_release(&buf);
>> -		return rc;
>> -	}
>> -
>> -	return run_command(&cmd);
>> +	return run_command_silent_on_success(&cmd,
>> +					     !(is_rebase_i(opts) && !(flags & EDIT_MSG)));
> 
> It would probably make more sense to change the signature of
> `run_command_silent_on_success()` to not even take the `int verbose`
> parameter: why call it "silent on success" when we can ask it *not* to be
> silent on success?
> 
> And then you can avoid this overly-long line (as well as the quite
> convoluted Boolean logic that took me a couple of seconds to verify) very
> elegantly by this code:
> 
> 	if (is_rebase_i(opts) && !(flags & EDIT_MSG))
> 		return run_command_silent_on_success(&cmd);
> 	return run_command(&cmd);
> 
> I vaguely recall that I wanted to make this an option in the `struct
> child_process` when I originally introduced this code, but I think it was
> Peff who suggested that doing it "by hand" was the more appropriate way
> here because I use it only once.
> 
> My recollection might fail me, but if it is correct, maybe that would be a
> good way forward, to make this an `int silent_on_success:1;` field?
> 

I think I found it:

https://public-inbox.org/git/1e82aeabb906a35175362418b2b4957fae50c3b0.1481642927.git.johannes.schindelin@gmx.de/

Apparently, you wanted to introduce a new RUN_ flag for
run_command_v_opt_cd_env(), but the change was qualified as a “bolted-on
feature” by Johannes Sixt.

So, I will remove the “verbose” argument in the reroll.

Cheers,
Alban

[GSoC][PATCH v3 0/3] rebase -i: rewrite reflog operations in C

[Alban Gruin]

Thes patch series rewrites the reflog operations from shell to C.  This
is part of the effort to rewrite interactive rebase in C.

The first commit is dedicated to creating a function to silence a
command, as the sequencer will do in several places with these patches.

This branch is based on ag/rebase-i-rewrite-todo, and does not conflict
with pu (as of 2018-06-21).

Changes since v2:

 - Removing the “verbose” parameter to run_command_silent_on_success()

 - Rewording some parts of the second commit

 - Changing the help for the “--verbose” flag

Alban Gruin (3):
  sequencer: add a new function to silence a command, except if it
    fails.
  rebase -i: rewrite setup_reflog_action() in C
  rebase -i: rewrite checkout_onto() in C

 builtin/rebase--helper.c   | 14 +++++-
 git-rebase--interactive.sh | 39 ++-------------
 sequencer.c                | 98 ++++++++++++++++++++++++++++----------
 sequencer.h                |  6 +++
 4 files changed, 96 insertions(+), 61 deletions(-)

-- 
2.17.1

[GSoC][PATCH v3 1/3] sequencer: add a new function to silence a command, except if it fails.

[Alban Gruin]

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

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

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

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

[GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C

[Alban Gruin]

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

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

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

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

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

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

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index d2990b210..fb5996a2c 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts = REPLAY_OPTS_INIT;
-	unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;
 	int abbreviate_commands = 0, rebase_cousins = -1;
 	enum {
 		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
 		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
-		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
+		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
 		OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
 			 N_("keep original branch points of cousins")),
+		OPT__VERBOSE(&verbose, N_("be verbose")),
 		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
 				CONTINUE),
 		OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
@@ -50,6 +51,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		OPT_CMDMODE(0, "edit-todo", &command,
 			    N_("edit the todo list during an interactive rebase"),
 			    EDIT_TODO),
+		OPT_CMDMODE(0, "checkout-base", &command,
+			    N_("checkout the base commit"), CHECKOUT_BASE),
 		OPT_END()
 	};
 
@@ -93,5 +96,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!append_todo_help(0, keep_empty);
 	if (command == EDIT_TODO && argc == 1)
 		return !!edit_todo_list(flags);
+	if (command == CHECKOUT_BASE && argc == 2)
+		return !!checkout_base_commit(&opts, argv[1], verbose);
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2defe607f..af46cf9c2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -72,6 +72,7 @@ collapse_todo_ids() {
 
 # Switch to the branch in $into and notify it in the reflog
 checkout_onto () {
+	comment_for_reflog start
 	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
 	output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
 	git update-ref ORIG_HEAD $orig_head
@@ -119,19 +120,6 @@ initiate_action () {
 	esac
 }
 
-setup_reflog_action () {
-	comment_for_reflog start
-
-	if test ! -z "$switch_to"
-	then
-		GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
-		output git checkout "$switch_to" -- ||
-			die "$(eval_gettext "Could not checkout \$switch_to")"
-
-		comment_for_reflog start
-	fi
-}
-
 init_basic_state () {
 	orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
 	mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
@@ -211,7 +199,7 @@ git_rebase__interactive () {
 		return 0
 	fi
 
-	setup_reflog_action
+	git rebase--helper --checkout-base "$switch_to" ${verbose:+--verbose}
 	init_basic_state
 
 	init_revisions_and_shortrevisions
diff --git a/sequencer.c b/sequencer.c
index 51c8ab7ac..27f8453fe 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3129,6 +3129,36 @@ static const char *reflog_message(struct replay_opts *opts,
 	return buf.buf;
 }
 
+static int run_git_checkout(struct replay_opts *opts, const char *commit,
+				int verbose, const char *action)
+{
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	cmd.git_cmd = 1;
+
+	argv_array_push(&cmd.args, "checkout");
+	argv_array_push(&cmd.args, commit);
+	argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
+
+	if (verbose)
+		return run_command(&cmd);
+	return run_command_silent_on_success(&cmd);
+}
+
+int checkout_base_commit(struct replay_opts *opts, const char *commit,
+			 int verbose)
+{
+	const char *action;
+
+	if (commit && *commit) {
+		action = reflog_message(opts, "start", "checkout %s", commit);
+		if (run_git_checkout(opts, commit, verbose, action))
+			return error(_("Could not checkout %s"), commit);
+	}
+
+	return 0;
+}
+
 static const char rescheduled_advice[] =
 N_("Could not execute the todo command\n"
 "\n"
diff --git a/sequencer.h b/sequencer.h
index 35730b13e..42c3dda81 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -100,6 +100,9 @@ int update_head_with_reflog(const struct commit *old_head,
 void commit_post_rewrite(const struct commit *current_head,
 			 const struct object_id *new_head);
 
+int checkout_base_commit(struct replay_opts *opts, const char *commit,
+			 int verbose);
+
 #define SUMMARY_INITIAL_COMMIT   (1 << 0)
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
 void print_commit_summary(const char *prefix, const struct object_id *oid,
-- 
2.17.1

[GSoC][PATCH v3 3/3] rebase -i: rewrite checkout_onto() in C

[Alban Gruin]

This rewrites checkout_onto() from shell to C.

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

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

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

Re: [GSoC][PATCH v3 1/3] sequencer: add a new function to silence a command, except if it fails.

[Junio C Hamano]

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

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

It might be just a difference in viewpoint, but I think it is more
customary in this project (hence it will be easier to understand and
accept by readers of the patch) if a change like this is explained
NOT as "introducing a new function and then rewrite an existing code
to use it", and instead as "extract a helper function from an
existing code so that future callers can reuse it."

> +static int run_command_silent_on_success(struct child_process *cmd)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	int rc;
> +
> +	cmd->stdout_to_stderr = 1;
> +	rc = pipe_command(cmd,
> +			  NULL, 0,
> +			  /* stdout is already redirected */

I can see that this comment was inherited from the original place
this code was lifted from (and that is why I say this is not "adding
a new helper and rewriting an existing piece of code to use it" but
is "extracting this function out of an existing code for future
reuse"), but does it still make sense in the new context to keep the
same comment?

The original in run_git_commit() made the .stdout_to_stderr=1
assignment many lines before it called pipe_command(), and it made
sense to remind readers why we are passing NULL to the out buffer
and only passing the err buffer here.  But in the context of this
new helper function, the redirection that may make such a reminder
necessary sits immediately before the pipe_command() call for
everybody to see.

> +			  NULL, 0,
> +			  &buf, 0);
> +
> +	if (rc)
> +		fputs(buf.buf, stderr);
> +	strbuf_release(&buf);
> +	return rc;
> +}

> @@ -820,18 +838,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>  
>  	cmd.git_cmd = 1;
>  
> -	if (is_rebase_i(opts)) {
> -		if (!(flags & EDIT_MSG)) {
> -			cmd.stdout_to_stderr = 1;
> -			cmd.err = -1;
> -		}
> +	if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {
> +		const char *gpg_opt = gpg_sign_opt_quoted(opts);
>  
> -		if (read_env_script(&cmd.env_array)) {
> -			const char *gpg_opt = gpg_sign_opt_quoted(opts);
> -
> -			return error(_(staged_changes_advice),
> -				     gpg_opt, gpg_opt);
> -		}
> +		return error(_(staged_changes_advice),
> +			     gpg_opt, gpg_opt);
>  	}

The original in this hunk is criminally incomprehensible ;-)  The
assignment of .stdout_to_stderr=1 does not have to happen this early
in the code, and also using .err being -1 to communicate that the
"capture and show error only upon error" magic is going to be used
to the later code is unnecessarily opaque.  The new code gets rid
of that ugliness from this earlier hunk and instead makes this part
only about reading the various state files (and reporting failure to
do so).

> @@ -861,20 +872,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>  	if (opts->allow_empty_message)
>  		argv_array_push(&cmd.args, "--allow-empty-message");
>  
> -	if (cmd.err == -1) {
> -		/* hide stderr on success */
> -		struct strbuf buf = STRBUF_INIT;
> -		int rc = pipe_command(&cmd,
> -				      NULL, 0,
> -				      /* stdout is already redirected */
> -				      NULL, 0,
> -				      &buf, 0);
> -		if (rc)
> -			fputs(buf.buf, stderr);
> -		strbuf_release(&buf);
> -		return rc;
> -	}
> -
> +	if (is_rebase_i(opts) && !(flags & EDIT_MSG))
> +		return run_command_silent_on_success(&cmd);
>  	return run_command(&cmd);
>  }

And then, the updated code in this hunk checks the EDIT_MSG bit
instead.  It probably is easier to understand the code if you added
an "else" after, to highlight the fact that this is choosing one out
of two possible ways to run "cmd", i.e.

	if (is_rebase_i(opts) && !(flags & EDIT_MSG))
		return run_command_silent_on_success(&cmd);
	else
		return run_command(&cmd);

Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C

[Junio C Hamano]

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

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

;-) on the "misnamed" part.  Indeed, setting up the comment for the
reflog entry is secondary to what this function wants to do, which
is to check out the branch to be rebased.

I do not think "base_commit" is a good name, either, though.  When I
hear 'base' in the context of 'rebase', I would imagine that the
speaker is talking about the bottom of the range of the commits to
be rebased (i.e. "rebase --onto ONTO BASE BRANCH", which replays
commits BASE..BRANCH on top of ONTO and then points BRANCH at the
result), not the top of the range or the branch these commits are
taken from.

> index 51c8ab7ac..27f8453fe 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3129,6 +3129,36 @@ static const char *reflog_message(struct replay_opts *opts,
>  	return buf.buf;
>  }
>  
> +static int run_git_checkout(struct replay_opts *opts, const char *commit,
> +				int verbose, const char *action)
> +{
> +	struct child_process cmd = CHILD_PROCESS_INIT;
> +
> +	cmd.git_cmd = 1;
> +
> +	argv_array_push(&cmd.args, "checkout");
> +	argv_array_push(&cmd.args, commit);
> +	argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
> +
> +	if (verbose)
> +		return run_command(&cmd);
> +	return run_command_silent_on_success(&cmd);

For the same reason as 1/3, I think it makes more sense to have
"else" here.

> +int checkout_base_commit(struct replay_opts *opts, const char *commit,
> +			 int verbose)
> +{
> +	const char *action;
> +
> +	if (commit && *commit) {

Hmm, isn't it a programming error to feed !commit or !*commit here?
I offhand do not think of a reason why making such an input a silent
no-op success, instead of making it error out, would be a good idea.

> +		action = reflog_message(opts, "start", "checkout %s", commit);
> +		if (run_git_checkout(opts, commit, verbose, action))
> +			return error(_("Could not checkout %s"), commit);
> +	}
> +
> +	return 0;
> +}
> +
>  static const char rescheduled_advice[] =
>  N_("Could not execute the todo command\n"
>  "\n"
> diff --git a/sequencer.h b/sequencer.h
> index 35730b13e..42c3dda81 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -100,6 +100,9 @@ int update_head_with_reflog(const struct commit *old_head,
>  void commit_post_rewrite(const struct commit *current_head,
>  			 const struct object_id *new_head);
>  
> +int checkout_base_commit(struct replay_opts *opts, const char *commit,
> +			 int verbose);
> +
>  #define SUMMARY_INITIAL_COMMIT   (1 << 0)
>  #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
>  void print_commit_summary(const char *prefix, const struct object_id *oid,

Re: [GSoC][PATCH v3 1/3] sequencer: add a new function to silence a command, except if it fails.

[Alban Gruin]

Hi Junio,

Le 22/06/2018 à 00:03, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
> > This adds a new function, run_command_silent_on_success(), to
> > redirect the stdout and stderr of a command to a strbuf, and then to run
> > that command. This strbuf is printed only if the command fails. It is
> > functionnaly similar to output() from git-rebase.sh.
> > 
> > run_git_commit() is then refactored to use of
> > run_command_silent_on_success().
> 
> It might be just a difference in viewpoint, but I think it is more
> customary in this project (hence it will be easier to understand and
> accept by readers of the patch) if a change like this is explained
> NOT as "introducing a new function and then rewrite an existing code
> to use it", and instead as "extract a helper function from an
> existing code so that future callers can reuse it."
> 

I like your wording.  It’s not a difference of point of view, I’m just bad at 
writing commit messages ;)

> > +static int run_command_silent_on_success(struct child_process *cmd)
> > +{
> > +	struct strbuf buf = STRBUF_INIT;
> > +	int rc;
> > +
> > +	cmd->stdout_to_stderr = 1;
> > +	rc = pipe_command(cmd,
> > +			  NULL, 0,
> > +			  /* stdout is already redirected */
> 
> I can see that this comment was inherited from the original place
> this code was lifted from (and that is why I say this is not "adding
> a new helper and rewriting an existing piece of code to use it" but
> is "extracting this function out of an existing code for future
> reuse"), but does it still make sense in the new context to keep the
> same comment?
> 
> The original in run_git_commit() made the .stdout_to_stderr=1
> assignment many lines before it called pipe_command(), and it made
> sense to remind readers why we are passing NULL to the out buffer
> and only passing the err buffer here.  But in the context of this
> new helper function, the redirection that may make such a reminder
> necessary sits immediately before the pipe_command() call for
> everybody to see.
> 

Yeah, you’re right.

> > @@ -861,20 +872,8 @@ static int run_git_commit(const char *defmsg, struct
> > replay_opts *opts,> 
> >  	if (opts->allow_empty_message)
> >  	
> >  		argv_array_push(&cmd.args, "--allow-empty-message");
> > 
> > -	if (cmd.err == -1) {
> > -		/* hide stderr on success */
> > -		struct strbuf buf = STRBUF_INIT;
> > -		int rc = pipe_command(&cmd,
> > -				      NULL, 0,
> > -				      /* stdout is already redirected */
> > -				      NULL, 0,
> > -				      &buf, 0);
> > -		if (rc)
> > -			fputs(buf.buf, stderr);
> > -		strbuf_release(&buf);
> > -		return rc;
> > -	}
> > -
> > +	if (is_rebase_i(opts) && !(flags & EDIT_MSG))
> > +		return run_command_silent_on_success(&cmd);
> > 
> >  	return run_command(&cmd);
> >  
> >  }
> 
> It probably is easier to understand the code if you added
> an "else" after, to highlight the fact that this is choosing one out
> of two possible ways to run "cmd", i.e.
> 
> 	if (is_rebase_i(opts) && !(flags & EDIT_MSG))
> 		return run_command_silent_on_success(&cmd);
> 	else
> 		return run_command(&cmd);

Okay.

Cheers,
Alban

Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C

[Alban Gruin]

Hi Junio,

Le 22/06/2018 à 18:27, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
> > This rewrites (the misnamed) setup_reflog_action() from shell to C. The
> > new version is called checkout_base_commit().
> 
> ;-) on the "misnamed" part.  Indeed, setting up the comment for the
> reflog entry is secondary to what this function wants to do, which
> is to check out the branch to be rebased.
> 
> I do not think "base_commit" is a good name, either, though.  When I
> hear 'base' in the context of 'rebase', I would imagine that the
> speaker is talking about the bottom of the range of the commits to
> be rebased (i.e. "rebase --onto ONTO BASE BRANCH", which replays
> commits BASE..BRANCH on top of ONTO and then points BRANCH at the
> result), not the top of the range or the branch these commits are
> taken from.
> 

Perhaps should I name this function checkout_onto(), and rename 
checkout_onto() to "detach_onto()"?  And I would reorder those two commits in 
the series, as this would be very confusing.

> > index 51c8ab7ac..27f8453fe 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -3129,6 +3129,36 @@ static const char *reflog_message(struct
> > replay_opts *opts,> 
> >  	return buf.buf;
> >  
> >  }
> > 
> > +static int run_git_checkout(struct replay_opts *opts, const char *commit,
> > +				int verbose, const char *action)
> > +{
> > +	struct child_process cmd = CHILD_PROCESS_INIT;
> > +
> > +	cmd.git_cmd = 1;
> > +
> > +	argv_array_push(&cmd.args, "checkout");
> > +	argv_array_push(&cmd.args, commit);
> > +	argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
> > +
> > +	if (verbose)
> > +		return run_command(&cmd);
> > +	return run_command_silent_on_success(&cmd);
> 
> For the same reason as 1/3, I think it makes more sense to have
> "else" here.
> 

Right.

> > +int checkout_base_commit(struct replay_opts *opts, const char *commit,
> > +			 int verbose)
> > +{
> > +	const char *action;
> > +
> > +	if (commit && *commit) {
> 
> Hmm, isn't it a programming error to feed !commit or !*commit here?
> I offhand do not think of a reason why making such an input a silent
> no-op success, instead of making it error out, would be a good idea.
> 

I think it’s correct.  

> > +		action = reflog_message(opts, "start", "checkout %s", commit);
> > +		if (run_git_checkout(opts, commit, verbose, action))
> > +			return error(_("Could not checkout %s"), commit);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > 
> >  static const char rescheduled_advice[] =
> >  N_("Could not execute the todo command\n"
> >  "\n"
> > 
> > diff --git a/sequencer.h b/sequencer.h
> > index 35730b13e..42c3dda81 100644
> > --- a/sequencer.h
> > +++ b/sequencer.h
> > @@ -100,6 +100,9 @@ int update_head_with_reflog(const struct commit
> > *old_head,> 
> >  void commit_post_rewrite(const struct commit *current_head,
> >  
> >  			 const struct object_id *new_head);
> > 
> > +int checkout_base_commit(struct replay_opts *opts, const char *commit,
> > +			 int verbose);
> > +
> > 
> >  #define SUMMARY_INITIAL_COMMIT   (1 << 0)
> >  #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
> >  void print_commit_summary(const char *prefix, const struct object_id
> >  *oid,

Cheers,
Alban

[GSoC][PATCH v4 0/3] rebase -i: rewrite reflog operations in C

[Alban Gruin]

This patch series rewrites the reflog operations from shell to C.  This
is part of the effort to rewrite interactive rebase in C.

The first commit is dedicated to creating a function to silence a
command, as the sequencer will do in several places with these patches.

This branch is based on ag/rebase-i-rewrite-todo, and does not conflict
with pu (as of 2018-06-25).

Changes since v3:

 - Removing a comment from run_command_silent_on_success()

 - Changing the order of setup_reflog_action() and checkout_onto()
   rewrites in the series

 - Renaming checkout_onto() to detach_onto()

 - Renaming checkout_base_commit() (rewrite of setup_reflog_action()) to
   checkout_onto()

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

Alban Gruin (3):
  sequencer: extract a function to silence a command, except if it fails
  rebase -i: rewrite checkout_onto() in C
  rebase -i: rewrite setup_reflog_action() in C

 builtin/rebase--helper.c   |  13 ++++-
 git-rebase--interactive.sh |  28 ++--------
 sequencer.c                | 101 +++++++++++++++++++++++++++----------
 sequencer.h                |   6 +++
 4 files changed, 97 insertions(+), 51 deletions(-)

-- 
2.18.0

[GSoC][PATCH v4 1/3] sequencer: extract a function to silence a command, except if it fails

[Alban Gruin]

This extracts the part of run_git_commit() that redirects the stdout and
stderr of a command to a strbuf, and prints it if the command fails, to
a helper function: run_command_silent_on_success(). It is intended to
replace output() from git-rebase.sh in the rewrite of
setup_reflog_action() and checkout_onto().

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

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

[GSoC][PATCH v4 2/3] rebase -i: rewrite checkout_onto() in C

[Alban Gruin]

This rewrites checkout_onto() from shell to C. The new version is called
detach_onto(), given its role.

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

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

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

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

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index d2990b210..2dfd55c76 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts = REPLAY_OPTS_INIT;
-	unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;
 	int abbreviate_commands = 0, rebase_cousins = -1;
 	enum {
 		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
 		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
-		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
+		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, DETACH_ONTO
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
 		OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
 			 N_("keep original branch points of cousins")),
+		OPT__VERBOSE(&verbose, N_("be verbose")),
 		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
 				CONTINUE),
 		OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
@@ -50,6 +51,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		OPT_CMDMODE(0, "edit-todo", &command,
 			    N_("edit the todo list during an interactive rebase"),
 			    EDIT_TODO),
+		OPT_CMDMODE(0, "detach-onto", &command,
+			    N_("checkout a commit"), DETACH_ONTO),
 		OPT_END()
 	};
 
@@ -93,5 +96,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!append_todo_help(0, keep_empty);
 	if (command == EDIT_TODO && argc == 1)
 		return !!edit_todo_list(flags);
+	if (command == DETACH_ONTO && argc == 4)
+		return !!detach_onto(&opts, argv[1], argv[2], argv[3], verbose);
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2defe607f..2f5761d49 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -70,13 +70,6 @@ collapse_todo_ids() {
 	git rebase--helper --shorten-ids
 }
 
-# Switch to the branch in $into and notify it in the reflog
-checkout_onto () {
-	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
-	output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
-	git update-ref ORIG_HEAD $orig_head
-}
-
 get_missing_commit_check_level () {
 	check_level=$(git config --get rebase.missingCommitsCheck)
 	check_level=${check_level:-ignore}
@@ -188,7 +181,8 @@ EOF
 
 	git rebase--helper --check-todo-list || {
 		ret=$?
-		checkout_onto
+		git rebase--helper --detach-onto "$onto_name" "$onto" \
+		    "$orig_head" ${verbose:+--verbose}
 		exit $ret
 	}
 
@@ -198,7 +192,8 @@ EOF
 	onto="$(git rebase--helper --skip-unnecessary-picks)" ||
 	die "Could not skip unnecessary pick commands"
 
-	checkout_onto
+	git rebase--helper --detach-onto "$onto_name" "$onto" "$orig_head" \
+	    ${verbose:+--verbose}
 	require_clean_work_tree "rebase"
 	exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
 	     --continue
diff --git a/sequencer.c b/sequencer.c
index 9a9725e23..ee6374921 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3129,6 +3129,42 @@ static const char *reflog_message(struct replay_opts *opts,
 	return buf.buf;
 }
 
+static int run_git_checkout(struct replay_opts *opts, const char *commit,
+				int verbose, const char *action)
+{
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	cmd.git_cmd = 1;
+
+	argv_array_push(&cmd.args, "checkout");
+	argv_array_push(&cmd.args, commit);
+	argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
+
+	if (verbose)
+		return run_command(&cmd);
+	else
+		return run_command_silent_on_success(&cmd);
+}
+
+int detach_onto(struct replay_opts *opts,
+		const char *onto_name, const char *onto,
+		const char *orig_head, unsigned verbose)
+{
+	struct object_id oid;
+	const char *action = reflog_message(opts, "start", "checkout %s", onto_name);
+
+	if (get_oid(orig_head, &oid))
+		return error(_("%s: not a valid OID"), orig_head);
+
+	if (run_git_checkout(opts, onto, verbose, action)) {
+		apply_autostash(opts);
+		sequencer_remove_state(opts);
+		return error(_("could not detach HEAD"));
+	}
+
+	return update_ref(NULL, "ORIG_HEAD", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+}
+
 static const char rescheduled_advice[] =
 N_("Could not execute the todo command\n"
 "\n"
diff --git a/sequencer.h b/sequencer.h
index 35730b13e..9f0ac5e75 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -100,6 +100,10 @@ int update_head_with_reflog(const struct commit *old_head,
 void commit_post_rewrite(const struct commit *current_head,
 			 const struct object_id *new_head);
 
+int detach_onto(struct replay_opts *opts,
+		const char *onto_name, const char *onto,
+		const char *orig_head, unsigned verbose);
+
 #define SUMMARY_INITIAL_COMMIT   (1 << 0)
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
 void print_commit_summary(const char *prefix, const struct object_id *oid,
-- 
2.18.0

[GSoC][PATCH v4 3/3] rebase -i: rewrite setup_reflog_action() in C

[Alban Gruin]

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

A new command is added to rebase--helper.c, “checkout-base”. The shell
version is then stripped.

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

If the commit OID provided to checkout_onto() is empty, nothing happens
and no errors are returned, otherwise it would break some
tests (t3404.92 and t3404.93).

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

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 2dfd55c76..a00b1091d 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -17,7 +17,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	enum {
 		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
 		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
-		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, DETACH_ONTO
+		ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, DETACH_ONTO, CHECKOUT_ONTO
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -53,6 +53,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 			    EDIT_TODO),
 		OPT_CMDMODE(0, "detach-onto", &command,
 			    N_("checkout a commit"), DETACH_ONTO),
+		OPT_CMDMODE(0, "checkout-onto", &command,
+			    N_("checkout the base commit"), CHECKOUT_ONTO),
 		OPT_END()
 	};
 
@@ -98,5 +100,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!edit_todo_list(flags);
 	if (command == DETACH_ONTO && argc == 4)
 		return !!detach_onto(&opts, argv[1], argv[2], argv[3], verbose);
+	if (command == CHECKOUT_ONTO && argc == 2)
+		return !!checkout_onto(&opts, argv[1], verbose);
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2f5761d49..375c36fb5 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -112,19 +112,6 @@ initiate_action () {
 	esac
 }
 
-setup_reflog_action () {
-	comment_for_reflog start
-
-	if test ! -z "$switch_to"
-	then
-		GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
-		output git checkout "$switch_to" -- ||
-			die "$(eval_gettext "Could not checkout \$switch_to")"
-
-		comment_for_reflog start
-	fi
-}
-
 init_basic_state () {
 	orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
 	mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
@@ -206,7 +193,7 @@ git_rebase__interactive () {
 		return 0
 	fi
 
-	setup_reflog_action
+	git rebase--helper --checkout-onto "$switch_to" ${verbose:+--verbose}
 	init_basic_state
 
 	init_revisions_and_shortrevisions
diff --git a/sequencer.c b/sequencer.c
index ee6374921..620b41e96 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3165,6 +3165,20 @@ int detach_onto(struct replay_opts *opts,
 	return update_ref(NULL, "ORIG_HEAD", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);
 }
 
+int checkout_onto(struct replay_opts *opts, const char *commit,
+		  int verbose)
+{
+	const char *action;
+
+	if (commit && *commit) {
+		action = reflog_message(opts, "start", "checkout %s", commit);
+		if (run_git_checkout(opts, commit, verbose, action))
+			return error(_("Could not checkout %s"), commit);
+	}
+
+	return 0;
+}
+
 static const char rescheduled_advice[] =
 N_("Could not execute the todo command\n"
 "\n"
diff --git a/sequencer.h b/sequencer.h
index 9f0ac5e75..afbb2edf1 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -103,6 +103,8 @@ void commit_post_rewrite(const struct commit *current_head,
 int detach_onto(struct replay_opts *opts,
 		const char *onto_name, const char *onto,
 		const char *orig_head, unsigned verbose);
+int checkout_onto(struct replay_opts *opts, const char *commit,
+		  int verbose);
 
 #define SUMMARY_INITIAL_COMMIT   (1 << 0)
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
-- 
2.18.0

Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C

[Junio C Hamano]

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

> Hi Junio,
>
> Le 22/06/2018 à 18:27, Junio C Hamano a écrit :
>> Alban Gruin <alban.gruin@gmail.com> writes:
>> > This rewrites (the misnamed) setup_reflog_action() from shell to C. The
>> > new version is called checkout_base_commit().
>> 
>> ;-) on the "misnamed" part.  Indeed, setting up the comment for the
>> reflog entry is secondary to what this function wants to do, which
>> is to check out the branch to be rebased.
>> 
>> I do not think "base_commit" is a good name, either, though.  When I
>> hear 'base' in the context of 'rebase', I would imagine that the
>> speaker is talking about the bottom of the range of the commits to
>> be rebased (i.e. "rebase --onto ONTO BASE BRANCH", which replays
>> commits BASE..BRANCH on top of ONTO and then points BRANCH at the
>> result), not the top of the range or the branch these commits are
>> taken from.
>> 
>
> Perhaps should I name this function checkout_onto(), and rename 
> checkout_onto() to "detach_onto()"?  And I would reorder those two commits in 
> the series, as this would be very confusing.

I may be misunderstanding what is happening in the function, but I
think it is checking out neither the onto or the base commit.  The
function instead is about checking out the branch to be rebased
before anything else happens when the optional <branch> argument is
given (and when the optional argument is not given, then we rebase
the current branch so there is no need to check it out upfront), no?

Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C

[Alban Gruin]

Hi Junio,

Le 25/06/2018 à 17:34, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
> 
>> Hi Junio,
>>
>> Le 22/06/2018 à 18:27, Junio C Hamano a écrit :
>>> Alban Gruin <alban.gruin@gmail.com> writes:
>>>> This rewrites (the misnamed) setup_reflog_action() from shell to C. The
>>>> new version is called checkout_base_commit().
>>>
>>> ;-) on the "misnamed" part.  Indeed, setting up the comment for the
>>> reflog entry is secondary to what this function wants to do, which
>>> is to check out the branch to be rebased.
>>>
>>> I do not think "base_commit" is a good name, either, though.  When I
>>> hear 'base' in the context of 'rebase', I would imagine that the
>>> speaker is talking about the bottom of the range of the commits to
>>> be rebased (i.e. "rebase --onto ONTO BASE BRANCH", which replays
>>> commits BASE..BRANCH on top of ONTO and then points BRANCH at the
>>> result), not the top of the range or the branch these commits are
>>> taken from.
>>>
>>
>> Perhaps should I name this function checkout_onto(), and rename 
>> checkout_onto() to "detach_onto()"?  And I would reorder those two commits in 
>> the series, as this would be very confusing.
> 
> I may be misunderstanding what is happening in the function, but I
> think it is checking out neither the onto or the base commit.  The
> function instead is about checking out the branch to be rebased
> before anything else happens when the optional <branch> argument is
> given (and when the optional argument is not given, then we rebase
> the current branch so there is no need to check it out upfront), no?
> 
> 

Yes, you’re right.

Now I really don’t know how to call this function.
checkout_top_of_range(), perhaps?

Cheers,
Alban

Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2212 bytes --]

Hi,

On Mon, 25 Jun 2018, Alban Gruin wrote:

> Le 25/06/2018 à 17:34, Junio C Hamano a écrit :
> > Alban Gruin <alban.gruin@gmail.com> writes:
> > 
> >> Le 22/06/2018 à 18:27, Junio C Hamano a écrit :
> >>> Alban Gruin <alban.gruin@gmail.com> writes:
> >>>> This rewrites (the misnamed) setup_reflog_action() from shell to C. The
> >>>> new version is called checkout_base_commit().
> >>>
> >>> ;-) on the "misnamed" part.  Indeed, setting up the comment for the
> >>> reflog entry is secondary to what this function wants to do, which
> >>> is to check out the branch to be rebased.
> >>>
> >>> I do not think "base_commit" is a good name, either, though.  When I
> >>> hear 'base' in the context of 'rebase', I would imagine that the
> >>> speaker is talking about the bottom of the range of the commits to
> >>> be rebased (i.e. "rebase --onto ONTO BASE BRANCH", which replays
> >>> commits BASE..BRANCH on top of ONTO and then points BRANCH at the
> >>> result), not the top of the range or the branch these commits are
> >>> taken from.
> >>>
> >>
> >> Perhaps should I name this function checkout_onto(), and rename 
> >> checkout_onto() to "detach_onto()"?  And I would reorder those two commits in 
> >> the series, as this would be very confusing.
> > 
> > I may be misunderstanding what is happening in the function, but I
> > think it is checking out neither the onto or the base commit.  The
> > function instead is about checking out the branch to be rebased
> > before anything else happens when the optional <branch> argument is
> > given (and when the optional argument is not given, then we rebase
> > the current branch so there is no need to check it out upfront), no?
> > 
> > 
> 
> Yes, you’re right.
> 
> Now I really don’t know how to call this function.
> checkout_top_of_range(), perhaps?

Pratik refactored some code from sequencer.c into checkout.c/checkout.h
today to do exactly that. It is not polished yet, but probably will be
tomorrow. It provides a function `int detach_head_to(struct object_oid
*oid, const char *action, const char *reflog_message)`. Maybe use that
directly, once the commit is available?

Ciao,
Dscho

Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C

[Pratik Karki]

Hi,

On Tue, Jun 26, 2018 at 3:00 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:

> Pratik refactored some code from sequencer.c into checkout.c/checkout.h
> today to do exactly that. It is not polished yet, but probably will be
> tomorrow. It provides a function `int detach_head_to(struct object_oid
> *oid, const char *action, const char *reflog_message)`. Maybe use that
> directly, once the commit is available?

The commit is here[1].

[1]: https://github.com/git/git/pull/505/commits/47031d4706bd1eccd2816ecdaaf6e9cd700909aa

Cheers,
Pratik

Re: [GSoC][PATCH v4 2/3] rebase -i: rewrite checkout_onto() in C

[Junio C Hamano]

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

> This rewrites checkout_onto() from shell to C. The new version is called
> detach_onto(), given its role.

The name, given its role, may be good, but is the implementtaion
robust enough to fulfill the promise its name gives?

>  	git rebase--helper --check-todo-list || {
>  		ret=$?
> -		checkout_onto
> +		git rebase--helper --detach-onto "$onto_name" "$onto" \
> +		    "$orig_head" ${verbose:+--verbose}

Here, $onto_name is what the end-user gave us (e.g. it is
"master..." in "git rebase --onto=master... base"), while $onto is a
40-hex object name of the commit.  $orig_head is also a 40-hex
object name.

And this call shows how the above shell scriptlet calls into the
detach_onto() thing ...

> +	if (command == DETACH_ONTO && argc == 4)
> +		return !!detach_onto(&opts, argv[1], argv[2], argv[3], verbose);

... which is defined like so:

> +int detach_onto(struct replay_opts *opts,
> +		const char *onto_name, const char *onto,
> +		const char *orig_head, unsigned verbose)
> +{
> +	struct object_id oid;
> +	const char *action = reflog_message(opts, "start", "checkout %s", onto_name);
> +
> +	if (get_oid(orig_head, &oid))
> +		return error(_("%s: not a valid OID"), orig_head);

Which means that this can be more strict to use get_oid_hex() to
catch possible mistakes in the caller.

> +	if (run_git_checkout(opts, onto, verbose, action)) {

And this could be a bit problematic, as we can see below how the
"checkout" thing does not guarantee "detaching" at all ...

> +		apply_autostash(opts);
> +		sequencer_remove_state(opts);
> +		return error(_("could not detach HEAD"));
> +	}
> +
> +	return update_ref(NULL, "ORIG_HEAD", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);
> +}
> +

... which can be seen here ...

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

This drives the external command "git checkout" with _any_ string
the caller passes in "commit".  If the variable happens to have
'master', for example, it would be "git checkout master" and if you
have a branch with that name, it will not detach but check out the
branch to build on it.  It is a caller's responsibility to give a
suitable "commit" if it wants to use this helper to detach.

So perhaps the caller of this function in detach_onto() should pass
"%s^0" or even do something like

	struct object_id onto_oid;
	char onto_hex[GIT_MAX_HEXSZ + 1];

	if (get_oid(onto, &onto_oid) || oid_to_hex_r(onto_hex, &onto_oid))
		return error(...);
	if (run_git_checkout(opts, onto_hex, verbose, action)) {
		...

to ensure that it keeps the promise its name gives.

I can hear "Oh, but it is a bug in the caller to give anything that
won't result in detaching in 'onto'" but that is not a valid excuse,
given that this _public_ function is called "detach_onto".  Making
sure detachment happens is its responsibility, not its callers'.

Or we could do a cop-out alternative of commenting the function in *.h
file to say "onto must be given as 40-hex", with a code to make sure
the caller really gave us a 40-hex and not a branch name.  That is a
less ideal but probably acceptable alternative.

>  static const char rescheduled_advice[] =
>  N_("Could not execute the todo command\n"
>  "\n"
> diff --git a/sequencer.h b/sequencer.h
> index 35730b13e..9f0ac5e75 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -100,6 +100,10 @@ int update_head_with_reflog(const struct commit *old_head,
>  void commit_post_rewrite(const struct commit *current_head,
>  			 const struct object_id *new_head);
>  
> +int detach_onto(struct replay_opts *opts,
> +		const char *onto_name, const char *onto,
> +		const char *orig_head, unsigned verbose);
> +
>  #define SUMMARY_INITIAL_COMMIT   (1 << 0)
>  #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
>  void print_commit_summary(const char *prefix, const struct object_id *oid,

Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C

[Junio C Hamano]

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

>>>> I do not think "base_commit" is a good name, either, though.  When I
>>>> hear 'base' in the context of 'rebase', I would imagine that the
>>>> speaker is talking about the bottom of the range of the commits to
>>>> be rebased (i.e. "rebase --onto ONTO BASE BRANCH", which replays
>>>> commits BASE..BRANCH on top of ONTO and then points BRANCH at the
>>>> result), not the top of the range or the branch these commits are
>>>> taken from.
>>> ...
> Now I really don’t know how to call this function.
> checkout_top_of_range(), perhaps?

If this is a straight rewrite of setup_reflog_action, i.e. the
division of labor between its caller and this function is such that
the caller blindly calls it without even checking if it got the
optional "branch to be rebased" argument and this function is
responsible to decide if the preparatory checkout of the named
branch is necessary, then any name that does not even hint that
checkout is done conditionally would not work well.

How about callilng it "prepare_branch_to_be_rebased()"?  This
function, at least the original setup_reflog_action, responds to

	git rebase [--onto ONTO] UPSTREAM

by doing nothing (i.e. the result of preparation is to do nothing
because we are rebasing the commits between UPSTREAM and currently
checked out state on top of ONTO, or UPSTREAM if ONTO is not
given) and it responds to

	git rebase [--onto ONTO] UPSTREAM BRANCH

by checking out BRANCH (most importantly, when given a concrete
branch name, it checks the branch out, and does not detach at the
commit at the tip of the branch), because that is the first
preparatory step to rebase the BRANCH.

[GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations in C

[Alban Gruin]

This patch series rewrites the reflog operations from shell to C.  This
is part of the effort to rewrite interactive rebase in C.

The first commit is dedicated to creating a function to silence a
command, as the sequencer will do in several places with these patches.

This branch is based on ag/rebase-i-rewrite-todo, and does not conflict
with pu (as of 2018-06-29).

Changes since v4:

 - Changing the order of setup_reflog_action() and checkout_onto()
   rewrites in the series

 - checkout_onto() is no longer renamed in C

 - setup_reflog_action() is renamed to prepare_branch_to_be_rebased(),
   and not to checkout_onto().

Alban Gruin (3):
  sequencer: add a new function to silence a command, except if it
    fails.
  rebase -i: rewrite setup_reflog_action() in C
  rebase -i: rewrite checkout_onto() in C

 builtin/rebase--helper.c   |  14 ++++-
 git-rebase--interactive.sh |  39 ++------------
 sequencer.c                | 101 +++++++++++++++++++++++++++----------
 sequencer.h                |   6 +++
 4 files changed, 98 insertions(+), 62 deletions(-)

-- 
2.18.0

[GSoC][PATCH v5 1/3] sequencer: add a new function to silence a command, except if it fails.

[Alban Gruin]

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

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

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

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

[GSoC][PATCH v5 2/3] rebase -i: rewrite setup_reflog_action() in C

[Alban Gruin]

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

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

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

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

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

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

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

[GSoC][PATCH v5 3/3] rebase -i: rewrite checkout_onto() in C

[Alban Gruin]

This rewrites checkout_onto() from shell to C.

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

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

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

Re: [GSoC][PATCH v5 2/3] rebase -i: rewrite setup_reflog_action() in C

[Junio C Hamano]

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

> +	git rebase--helper --prepare-branch "$switch_to" ${verbose:+--verbose}
>  	init_basic_state
>  
>  	init_revisions_and_shortrevisions
> diff --git a/sequencer.c b/sequencer.c
> index d9545b366..dd0cf3cb5 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3134,6 +3134,37 @@ static const char *reflog_message(struct replay_opts *opts,
>  	return buf.buf;
>  }
> ...
> +int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit,
> +				 int verbose)
> +{
> +	const char *action;
> +
> +	if (commit && *commit) {

The sloppyness of this callee is forced by the sloppy caller, which
does not check if "$switch_to" has any value before calling "prepare
branch to be rebased" function.  A less sloppy caller would check to
see if we have the optional "before doing anything, check out this
branch, as it is the branch we are trying to rebase" argument, and
refrain from calling this function, so there is no need for this "if
commit is given and is not an empty string" check.

And hopefully, when GSoC is over, the caller, that is still in shell
at this step, would also be rewritten in C and by that time the
caller will become less sloppy, removing the need for this check.

Hence, I think the reason why the check is still here, and our
desire to eventually remove the check, should be documented with an
in-code comment around here (the usual "NEEDSWORK:" comment is
fine).

Re: [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations in C

[Junio C Hamano]

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

> This patch series rewrites the reflog operations from shell to C.  This
> is part of the effort to rewrite interactive rebase in C.
>
> The first commit is dedicated to creating a function to silence a
> command, as the sequencer will do in several places with these patches.
>
> This branch is based on ag/rebase-i-rewrite-todo, and does not conflict
> with pu (as of 2018-06-29).

Let's aggregate these topics into a single topic, and perhaps call
it ag/rebase-i-in-c or something like that.  Pretending as if they
are separately replaceable does not make much sense, as you are not
rerolling the earlier one and keep going forward with producing more
parts that depends on the parts that have been submitted earlier.

These three patches looked more-or-less good; I may have spotted a
few nits and suggested improvements, though.

Thanks.

Re: [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations in C

[Junio C Hamano]

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

> Let's aggregate these topics into a single topic, and perhaps call
> it ag/rebase-i-in-c or something like that.  Pretending as if they
> are separately replaceable does not make much sense, as you are not
> rerolling the earlier one and keep going forward with producing more
> parts that depends on the parts that have been submitted earlier.

So here is what I tentatively did.

    $ git log --oneline --reverse master..ag/rebase-i-in-c
    4d303fb608 rebase--interactive: rewrite append_todo_help() in C
    b4ffe143a9 editor: add a function to launch the sequence editor
    4ebe39cef9 rebase--interactive: rewrite the edit-todo functionality in C
    0ff6bf7646 sequencer: add a new function to silence a command, except if it fails.
    36784b351f rebase -i: rewrite setup_reflog_action() in C
    415cac57ee rebase -i: rewrite checkout_onto() in C

In several hours please fetch from me and look for "Merge branch
'ag/rebase-i-in-c' to pu" to see how they exactly look like; some of
the patches might not be the latest ones, in which case you may need
to prod me to get them replaced (resending them as a whole with
incremented v$n header is probably the easiest if we need to do so).

Thanks.

Re: [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations in C

[Alban Gruin]

Hi Junio,

Le 29/06/2018 à 20:23, Junio C Hamano a écrit :
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Let's aggregate these topics into a single topic, and perhaps call
>> it ag/rebase-i-in-c or something like that.  Pretending as if they
>> are separately replaceable does not make much sense, as you are not
>> rerolling the earlier one and keep going forward with producing more
>> parts that depends on the parts that have been submitted earlier.
> 
> So here is what I tentatively did.
> 
>     $ git log --oneline --reverse master..ag/rebase-i-in-c
>     4d303fb608 rebase--interactive: rewrite append_todo_help() in C
>     b4ffe143a9 editor: add a function to launch the sequence editor
>     4ebe39cef9 rebase--interactive: rewrite the edit-todo functionality in C
>     0ff6bf7646 sequencer: add a new function to silence a command, except if it fails.
>     36784b351f rebase -i: rewrite setup_reflog_action() in C
>     415cac57ee rebase -i: rewrite checkout_onto() in C
> 
> In several hours please fetch from me and look for "Merge branch
> 'ag/rebase-i-in-c' to pu" to see how they exactly look like; some of
> the patches might not be the latest ones, in which case you may need
> to prod me to get them replaced (resending them as a whole with
> incremented v$n header is probably the easiest if we need to do so).
> 
> Thanks.
> 

The patches about append_todo_help() and edit-todo are not up to date,
so I’ll resend them in a few minutes.  Otherwise, this looks good to me.

Cheers,
Alban

Re: [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations in C

[Junio C Hamano]

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

> The patches about append_todo_help() and edit-todo are not up to date,
> so I’ll resend them in a few minutes

Good.