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.
> 

> > 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,