Hi Alban,


On Tue, 19 Jun 2018, Alban Gruin wrote:

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

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

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

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

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

s/as such as/as well as/

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

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

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

s/not longer/no longer/

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

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

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

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

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

The rest looks fine to me, thank you!
Dscho