Hi Junio,
Le 22/06/2018 à 18:27, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@xxxxxxxxx> writes:
> > This rewrites (the misnamed) setup_reflog_action() from shell to C. The
> > new version is called checkout_base_commit().
>
> ;-) on the "misnamed" part. Indeed, setting up the comment for the
> reflog entry is secondary to what this function wants to do, which
> is to check out the branch to be rebased.
>
> I do not think "base_commit" is a good name, either, though. When I
> hear 'base' in the context of 'rebase', I would imagine that the
> speaker is talking about the bottom of the range of the commits to
> be rebased (i.e. "rebase --onto ONTO BASE BRANCH", which replays
> commits BASE..BRANCH on top of ONTO and then points BRANCH at the
> result), not the top of the range or the branch these commits are
> taken from.
>
Perhaps should I name this function checkout_onto(), and rename
checkout_onto() to "detach_onto()"? And I would reorder those two commits in
the series, as this would be very confusing.
> > index 51c8ab7ac..27f8453fe 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -3129,6 +3129,36 @@ static const char *reflog_message(struct
> > replay_opts *opts,>
> > return buf.buf;
> >
> > }
> >
> > +static int run_git_checkout(struct replay_opts *opts, const char *commit,
> > + int verbose, const char *action)
> > +{
> > + struct child_process cmd = CHILD_PROCESS_INIT;
> > +
> > + cmd.git_cmd = 1;
> > +
> > + argv_array_push(&cmd.args, "checkout");
> > + argv_array_push(&cmd.args, commit);
> > + argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
> > +
> > + if (verbose)
> > + return run_command(&cmd);
> > + return run_command_silent_on_success(&cmd);
>
> For the same reason as 1/3, I think it makes more sense to have
> "else" here.
>
Right.
> > +int checkout_base_commit(struct replay_opts *opts, const char *commit,
> > + int verbose)
> > +{
> > + const char *action;
> > +
> > + if (commit && *commit) {
>
> Hmm, isn't it a programming error to feed !commit or !*commit here?
> I offhand do not think of a reason why making such an input a silent
> no-op success, instead of making it error out, would be a good idea.
>
I think it’s correct.
> > + action = reflog_message(opts, "start", "checkout %s", commit);
> > + if (run_git_checkout(opts, commit, verbose, action))
> > + return error(_("Could not checkout %s"), commit);
> > + }
> > +
> > + return 0;
> > +}
> > +
> >
> > static const char rescheduled_advice[] =
> > N_("Could not execute the todo command\n"
> > "\n"
> >
> > diff --git a/sequencer.h b/sequencer.h
> > index 35730b13e..42c3dda81 100644
> > --- a/sequencer.h
> > +++ b/sequencer.h
> > @@ -100,6 +100,9 @@ int update_head_with_reflog(const struct commit
> > *old_head,>
> > void commit_post_rewrite(const struct commit *current_head,
> >
> > const struct object_id *new_head);
> >
> > +int checkout_base_commit(struct replay_opts *opts, const char *commit,
> > + int verbose);
> > +
> >
> > #define SUMMARY_INITIAL_COMMIT (1 << 0)
> > #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
> > void print_commit_summary(const char *prefix, const struct object_id
> > *oid,
Cheers,
Alban
