Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C
Alban Gruin writes: 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.
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 wrote: > 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
Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C
Hi, On Mon, 25 Jun 2018, Alban Gruin wrote: > Le 25/06/2018 à 17:34, Junio C Hamano a écrit : > > Alban Gruin writes: > > > >> Le 22/06/2018 à 18:27, Junio C Hamano a écrit : > >>> Alban Gruin 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 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
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 : > Alban Gruin writes: > >> Hi Junio, >> >> Le 22/06/2018 à 18:27, Junio C Hamano a écrit : >>> Alban Gruin 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 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
Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C
Alban Gruin writes: > Hi Junio, > > Le 22/06/2018 à 18:27, Junio C Hamano a écrit : >> Alban Gruin 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 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?
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 :
> Alban Gruin 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(, "checkout");
> > + argv_array_push(, commit);
> > + argv_array_pushf(_array, GIT_REFLOG_ACTION "=%s", action);
> > +
> > + if (verbose)
> > + return run_command();
> > + return run_command_silent_on_success();
>
> 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
Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C
Alban Gruin 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(, "checkout");
> + argv_array_push(, commit);
> + argv_array_pushf(_array, GIT_REFLOG_ACTION "=%s", action);
> +
> + if (verbose)
> + return run_command();
> + return run_command_silent_on_success();
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,
