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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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

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

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

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

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

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