threads / patch / 48730

patch, 3 partsrebase -i: rewrite reflog operations in C

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

## tl;dr

51 messages between Jun 18, 2018 and Jul 3, 2018. Diffs are folded; open one to read it.

replies: 50people: 7as markdown or json

Alban Gruin· Jun 18, 2018, 13:18 UTC · lore

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
Alban Gruin· Jun 18, 2018, 13:18 UTC · re: Alban Gruin · lore

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

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(-)
Show changes to sequencer.c +27 −26
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
Phillip Wood· Jun 18, 2018, 15:26 UTC · re: Alban Gruin · lore

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

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
Show 88 quoted lines
> 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)
> 
Alban Gruin· Jun 18, 2018, 16:46 UTC · re: Phillip Wood · lore

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

Hi Phillip,
Le 18/06/2018 à 17:26, Phillip Wood a écrit :
Show 15 quoted lines
> 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

Christian Couder· Jun 18, 2018, 16:26 UTC · re: Alban Gruin · lore

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

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()...
Show 7 quoted lines
> 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.

Alban Gruin· Jun 18, 2018, 17:05 UTC · re: Christian Couder · lore

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

Hi Christian,
Le 18/06/2018 à 18:26, Christian Couder a écrit :
Show 26 quoted lines
> 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

Alban Gruin· Jun 18, 2018, 13:18 UTC · re: Alban Gruin · lore

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

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(-)
Show changes to 4 files +43 −16

builtin/rebase--helper.c, git-rebase--interactive.sh, sequencer.c, sequencer.h

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
Phillip Wood· Jun 18, 2018, 15:34 UTC · re: Alban Gruin · lore

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

On 18/06/18 14:18, Alban Gruin wrote:
Show 5 quoted lines
> 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
Show 155 quoted lines
> 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,
> 
Alban Gruin· Jun 18, 2018, 17:04 UTC · re: Phillip Wood · lore

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

Hi Phillip,
Le 18/06/2018 à 17:34, Phillip Wood a écrit :
Show 10 quoted lines
> 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.
Show 11 quoted lines
> 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

Stefan Beller· Jun 18, 2018, 22:01 UTC · re: Alban Gruin · lore

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

On Mon, Jun 18, 2018 at 6:19 AM Alban Gruin <alban.gruin@gmail.com> wrote:
Show 44 quoted lines
>
> 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.

Show 6 quoted lines
> +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.
Johannes Schindelin· Jun 19, 2018, 06:51 UTC · re: Stefan Beller · lore

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

Hi Stefan,
On Mon, 18 Jun 2018, Stefan Beller wrote:
Show 13 quoted lines
> 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

Alban Gruin· Jun 18, 2018, 13:18 UTC · re: Alban Gruin · lore

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

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(-)
Show changes to 4 files +32 −22

builtin/rebase--helper.c, git-rebase--interactive.sh, sequencer.c, sequencer.h

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
Phillip Wood· Jun 18, 2018, 16:09 UTC · re: Alban Gruin · lore

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

Hi Alban
On 18/06/18 14:18, Alban Gruin wrote:
Show 118 quoted lines
> 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
Show 28 quoted lines
> +
> +	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)
> 
Alban Gruin· Jun 18, 2018, 17:04 UTC · re: Phillip Wood · lore

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

Hi Phillip,
Le 18/06/2018 à 18:09, Phillip Wood a écrit :
Show 6 quoted lines
>> +    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

Alban Gruin· Jun 19, 2018, 15:44 UTC · re: Alban Gruin · lore

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

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
Alban Gruin· Jun 19, 2018, 15:44 UTC · re: Alban Gruin · lore

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

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(-)
Show changes to sequencer.c +29 −26
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
Johannes Schindelin· Jun 21, 2018, 09:37 UTC · re: Alban Gruin · lore

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

Hi Alban,
On Tue, 19 Jun 2018, Alban Gruin wrote:
Show 19 quoted lines
> 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.

Show 24 quoted lines
> +	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.

Show 35 quoted lines
> +	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

Alban Gruin· Jun 21, 2018, 11:53 UTC · re: Johannes Schindelin · lore

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

Hi Johannes,
Le 21/06/2018 à 11:37, Johannes Schindelin a écrit :
Show 114 quoted lines
> 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

Alban Gruin· Jun 19, 2018, 15:44 UTC · re: Alban Gruin · lore

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

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(-)
Show changes to 4 files +40 −16

builtin/rebase--helper.c, git-rebase--interactive.sh, sequencer.c, sequencer.h

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
Johannes Schindelin· Jun 21, 2018, 10:34 UTC · re: Alban Gruin · lore

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

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/
Show 35 quoted lines
> 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?

Show 21 quoted lines
> 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.

Show 18 quoted lines
> +
> +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

Alban Gruin· Jun 19, 2018, 15:44 UTC · re: Alban Gruin · lore

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

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(-)
Show changes to 4 files +32 −22

builtin/rebase--helper.c, git-rebase--interactive.sh, sequencer.c, sequencer.h

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
Johannes Schindelin· Jun 21, 2018, 10:38 UTC · re: Alban Gruin · lore

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

Hi Alban,
On Tue, 19 Jun 2018, Alban Gruin wrote:
Show 19 quoted lines
> 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

Stefan Beller· Jun 19, 2018, 18:35 UTC · re: Alban Gruin · lore

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

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

Johannes Schindelin· Jun 21, 2018, 08:39 UTC · re: Alban Gruin · lore

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

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

Alban Gruin· Jun 21, 2018, 14:17 UTC · re: Alban Gruin · lore

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

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
Alban Gruin· Jun 21, 2018, 14:17 UTC · re: Alban Gruin · lore

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

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(-)
Show changes to sequencer.c +24 −25
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
Junio C Hamano· Jun 21, 2018, 22:03 UTC · re: Alban Gruin · lore

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

Alban Gruin <alban.gruin@gmail.com> writes:
Show 7 quoted lines
> 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."

Show 9 quoted lines
> +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.

Show 8 quoted lines
> +			  NULL, 0,
> +			  &buf, 0);
> +
> +	if (rc)
> +		fputs(buf.buf, stderr);
> +	strbuf_release(&buf);
> +	return rc;
> +}
Show 21 quoted lines
> @@ -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).

Show 22 quoted lines
> @@ -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);
Alban Gruin· Jun 22, 2018, 20:47 UTC · re: Junio C Hamano · lore

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

Hi Junio,
Le 22/06/2018 à 00:03, Junio C Hamano a écrit :
Show 16 quoted lines
> 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 ;)

Show 25 quoted lines
> > +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.
Show 35 quoted lines
> > @@ -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

Alban Gruin· Jun 21, 2018, 14:17 UTC · re: Alban Gruin · lore

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

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(-)
Show changes to 4 files +42 −16

builtin/rebase--helper.c, git-rebase--interactive.sh, sequencer.c, sequencer.h

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
Junio C Hamano· Jun 22, 2018, 16:27 UTC · re: Alban Gruin · lore

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

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.

Show 21 quoted lines
> 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.

Show 6 quoted lines
> +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.

Show 25 quoted lines
> +		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,
Alban Gruin· Jun 22, 2018, 20:48 UTC · re: Junio C Hamano · lore

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

Hi Junio,
Le 22/06/2018 à 18:27, Junio C Hamano a écrit :
Show 16 quoted lines
> 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.

Show 27 quoted lines
> > 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.
Show 11 quoted lines
> > +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.  
Show 31 quoted lines
> > +		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

Junio C Hamano· Jun 25, 2018, 15:34 UTC · re: Alban Gruin · lore

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

Alban Gruin <alban.gruin@gmail.com> writes:
Show 23 quoted lines
> 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?

Alban Gruin· Jun 25, 2018, 18:21 UTC · re: Junio C Hamano · lore

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

Hi Junio,
Le 25/06/2018 à 17:34, Junio C Hamano a écrit :
Show 34 quoted lines
> 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

Johannes Schindelin· Jun 25, 2018, 21:14 UTC · re: Alban Gruin · lore

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

Hi,
On Mon, 25 Jun 2018, Alban Gruin wrote:
Show 38 quoted lines
> 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

Pratik Karki· Jun 26, 2018, 09:13 UTC · re: Johannes Schindelin · lore

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

Hi,

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

Show 5 quoted lines
> 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

Junio C Hamano· Jun 26, 2018, 17:44 UTC · re: Alban Gruin · lore

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

Alban Gruin <alban.gruin@gmail.com> writes:
Show 10 quoted lines
>>>> 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.

Alban Gruin· Jun 21, 2018, 14:17 UTC · re: Alban Gruin · lore

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

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(-)
Show changes to 4 files +32 −22

builtin/rebase--helper.c, git-rebase--interactive.sh, sequencer.c, sequencer.h

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
Alban Gruin· Jun 25, 2018, 13:44 UTC · re: Alban Gruin · lore

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

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
Alban Gruin· Jun 25, 2018, 13:44 UTC · re: Alban Gruin · lore

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

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(-)
Show changes to sequencer.c +25 −26
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
Alban Gruin· Jun 25, 2018, 13:44 UTC · re: Alban Gruin · lore

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

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(-)
Show changes to 4 files +51 −11

builtin/rebase--helper.c, git-rebase--interactive.sh, sequencer.c, sequencer.h

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
Junio C Hamano· Jun 26, 2018, 17:35 UTC · re: Alban Gruin · lore

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

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?

Show 5 quoted lines
>  	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:
Show 9 quoted lines
> +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 ...

Show 8 quoted lines
> +		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 ...
Show 16 quoted lines
> +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.

Show 18 quoted lines
>  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,
Alban Gruin· Jun 25, 2018, 13:44 UTC · re: Alban Gruin · lore

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

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(-)
Show changes to 4 files +22 −15

builtin/rebase--helper.c, git-rebase--interactive.sh, sequencer.c, sequencer.h

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
Alban Gruin· Jun 29, 2018, 15:14 UTC · re: Alban Gruin · lore

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

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
Alban Gruin· Jun 29, 2018, 15:14 UTC · re: Alban Gruin · lore

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

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(-)
Show changes to sequencer.c +25 −26
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
Alban Gruin· Jun 29, 2018, 15:14 UTC · re: Alban Gruin · lore

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

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(-)
Show changes to 4 files +43 −16

builtin/rebase--helper.c, git-rebase--interactive.sh, sequencer.c, sequencer.h

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
Junio C Hamano· Jun 29, 2018, 16:50 UTC · re: Alban Gruin · lore

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

Alban Gruin <alban.gruin@gmail.com> writes:
Show 18 quoted lines
> +	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).

Alban Gruin· Jun 29, 2018, 15:14 UTC · re: Alban Gruin · lore

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

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(-)
Show changes to 4 files +32 −22

builtin/rebase--helper.c, git-rebase--interactive.sh, sequencer.c, sequencer.h

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
Junio C Hamano· Jun 29, 2018, 16:55 UTC · re: Alban Gruin · lore

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

Alban Gruin <alban.gruin@gmail.com> writes:
Show 8 quoted lines
> 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.
Junio C Hamano· Jun 29, 2018, 18:23 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
> 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.
Alban Gruin· Jul 2, 2018, 10:36 UTC · re: Junio C Hamano · lore

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

Hi Junio,
Le 29/06/2018 à 20:23, Junio C Hamano a écrit :
Show 26 quoted lines
> 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

Junio C Hamano· Jul 3, 2018, 18:15 UTC · re: Alban Gruin · lore

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

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.

← back to recent threads