rebase --keep-base: imply --reapply-cherry-picks and --no-fork-point #1323
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
phillipwood
force-pushed
the
wip/rebase--keep-base-tweaks
branch
2 times, most recently
from
13921bd
to
68bcd10
Compare
|
/submit |
|
Submitted as pull.1323.git.1660576283.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -79,8 +79,10 @@ test_expect_success 'rebase -i --onto main...topic' ' | |||
| git reset --hard && | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> As $EDITOR is exported setting it in one test affects all subsequent
> tests. Avoid this by always setting it in a subshell and remove a
> couple of unnecessary call to set_fake_editor.
Unnecessary because it reuses the one that was established in the
previous test [1]? Or unnecessary because we know "rebase -i" would
fail even before it gets to the point of asking an editor to tweak
the todo sequence [2]? Or something else?
If [1], it makes us wonder what happens when an earlier test gets
skipped. If [2], it makes us wonder what happens when "rebase -i"
fails to fail as expected (does the test correctly diagnose it as a
new breakage in "rebase -i"?).
> @@ -102,7 +106,6 @@ test_expect_success 'rebase -i --onto main...side' '
> git checkout side &&
> git reset --hard K &&
>
> - set_fake_editor &&
> test_must_fail git rebase -i --onto main...side J
> '
This is one of the "removing" instances.
> @@ -187,8 +194,12 @@ test_expect_success 'rebase -i --keep-base main from side' '
> git checkout side &&
> git reset --hard K &&
>
> - set_fake_editor &&
> test_must_fail git rebase -i --keep-base main
> '
And this is the other one.
Thanks.There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi Junio
On 15/08/2022 17:53, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> As $EDITOR is exported setting it in one test affects all subsequent
>> tests. Avoid this by always setting it in a subshell and remove a
>> couple of unnecessary call to set_fake_editor.
> > Unnecessary because it reuses the one that was established in the
> previous test [1]? Or unnecessary because we know "rebase -i" would
> fail even before it gets to the point of asking an editor to tweak
> the todo sequence [2]? Or something else?
I meant unnecessary as the editor does not change the todo list, but [2] also applies.
> If [1], it makes us wonder what happens when an earlier test gets
> skipped. If [2], it makes us wonder what happens when "rebase -i"
> fails to fail as expected (does the test correctly diagnose it as a
> new breakage in "rebase -i"?).
I think those tests could be tightened up, I'll add a new patch that renames them to describe what they are testing (that we fail if there is more than one merge base) and greps for the expected error message.
Best Wishes
Phillip
>> @@ -102,7 +106,6 @@ test_expect_success 'rebase -i --onto main...side' '
>> git checkout side &&
>> git reset --hard K &&
>> >> - set_fake_editor &&
>> test_must_fail git rebase -i --onto main...side J
>> '
> > This is one of the "removing" instances.
> >> @@ -187,8 +194,12 @@ test_expect_success 'rebase -i --keep-base main from side' '
>> git checkout side &&
>> git reset --hard K &&
>> >> - set_fake_editor &&
>> test_must_fail git rebase -i --keep-base main
>> '
> > And this is the other one.
> > Thanks.There was a problem hiding this comment.
On the Git mailing list, Jonathan Tan wrote (reply to this):
Phillip Wood <phillip.wood123@gmail.com> writes:
> Hi Junio
>
> On 15/08/2022 17:53, Junio C Hamano wrote:
> > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> >> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >>
> >> As $EDITOR is exported setting it in one test affects all subsequent
> >> tests. Avoid this by always setting it in a subshell and remove a
> >> couple of unnecessary call to set_fake_editor.
> >
> > Unnecessary because it reuses the one that was established in the
> > previous test [1]? Or unnecessary because we know "rebase -i" would
> > fail even before it gets to the point of asking an editor to tweak
> > the todo sequence [2]? Or something else?
>
> I meant unnecessary as the editor does not change the todo list, but [2]
> also applies.
Maybe this is moot with the other changes you're planning, but even if
the editor doesn't change the todo list, it's still necessary, right? At
the very least, we need to suppress the default interactive editor and
replace it with one that just reuses the input file without any
modification.There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi Jonathan
On 24/08/2022 23:28, Jonathan Tan wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>> Hi Junio
>>
>> On 15/08/2022 17:53, Junio C Hamano wrote:
>>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>>
>>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>>
>>>> As $EDITOR is exported setting it in one test affects all subsequent
>>>> tests. Avoid this by always setting it in a subshell and remove a
>>>> couple of unnecessary call to set_fake_editor.
>>>
>>> Unnecessary because it reuses the one that was established in the
>>> previous test [1]? Or unnecessary because we know "rebase -i" would
>>> fail even before it gets to the point of asking an editor to tweak
>>> the todo sequence [2]? Or something else?
>>
>> I meant unnecessary as the editor does not change the todo list, but [2]
>> also applies.
> > Maybe this is moot with the other changes you're planning, but even if
> the editor doesn't change the todo list, it's still necessary, right? At
> the very least, we need to suppress the default interactive editor and
> replace it with one that just reuses the input file without any
> modification.
The default GIT_EDITOR when running the test suite is ":" and GIT_SEQUENCE_EDITOR and sequence.editor are unset so we don't need to set an editor in the tests unless we want to change the todo list.
Best Wishes
Phillip
| @@ -68,7 +68,7 @@ struct rebase_options { | |||
| const char *upstream_name; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Using a struct commit rather than a struct oid to hold orig_head means
> that we error out straight away if branch being rebased does not point
> to a commit. It also simplifies the code than handles finding the
> merge base and fork point as it not longer has to convert from an oid
> to a commit.
As lookup_commit_reference_by_name() eventually calls into deref_tag(),
a command like
git rebase -i maint $(git rev-parse v2.37.2)
would presumably still work, which is good.
| @@ -68,7 +68,7 @@ struct rebase_options { | |||
| const char *upstream_name; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Separate out calculating the merge base between onto and head from the
> check for whether we can fast-forward or not. This means we can skip
> the fast-forward checks when the rebase is forced and avoid
> calculating the merge-base twice when --keep-base is given.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> Note the unnecessary braces around "if (keep_base)" are added here
> reduce the churn on the next commit.
> ---
> builtin/rebase.c | 35 +++++++++++++++++++++++------------
> 1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 6cf9c95f4e1..86ea731ca3a 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -871,13 +871,9 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
> struct commit_list *merge_bases = NULL;
> int res = 0;
>
> - merge_bases = get_merge_bases(onto, head);
> - if (!merge_bases || merge_bases->next) {
> - oidcpy(merge_base, null_oid());
> + if (is_null_oid(merge_base))
> goto done;
> - }
>
> - oidcpy(merge_base, &merge_bases->item->object.oid);
> if (!oideq(merge_base, &onto->object.oid))
> goto done;
Looking at the change in "git show -W", it seems that this function
no longer touches merge_bases at all, other than initializing it to
NULL at the beginning and then calling free_commit_list() on it at
the end. Shouldn't it be removed?
> @@ -902,6 +898,20 @@ done:
> return res && is_linear_history(onto, head);
> }
>
> +static void fill_merge_base(struct rebase_options *options,
> + struct object_id *merge_base)
> +{
> + struct commit_list *merge_bases = NULL;
> +
> + merge_bases = get_merge_bases(options->onto, options->orig_head);
> + if (!merge_bases || merge_bases->next)
> + oidcpy(merge_base, null_oid());
> + else
> + oidcpy(merge_base, &merge_bases->item->object.oid);
> +
> + free_commit_list(merge_bases);
> +}
> +
> static int parse_opt_am(const struct option *opt, const char *arg, int unset)
> {
> struct rebase_options *opts = opt->value;
> @@ -1668,7 +1678,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> die(_("Does not point to a valid commit '%s'"),
> options.onto_name);
> }
> -
> + if (keep_base) {
> + oidcpy(&merge_base, &options.onto->object.oid);
> + } else {
> + fill_merge_base(&options, &merge_base);
> + }
No need for braces around single-statement block on either side.
This is not a new issue introduced by this patch per-se, but
"merge_base" is becoming less and less accurate description of what
this variable really is. Perhaps it is a good time to rename it?
It is "the base commit to rebuild the history on top of", aka "onto
commit", isn't it? We often use merge-base between the upstream and
our tip of the history for it, but the variable often does not even
hold the merge-base in it, not because we cannot compute a single
merge-base but because depending on the operation mode we do not
want to use merge-base for further operation using that variable.
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Junio,
On Mon, 15 Aug 2022, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >
> > Separate out calculating the merge base between onto and head from the
> > check for whether we can fast-forward or not. This means we can skip
> > the fast-forward checks when the rebase is forced and avoid
> > calculating the merge-base twice when --keep-base is given.
> >
> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> > ---
> > Note the unnecessary braces around "if (keep_base)" are added here
> > reduce the churn on the next commit.
This note...
> > @@ -1668,7 +1678,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> > die(_("Does not point to a valid commit '%s'"),
> > options.onto_name);
> > }
> > -
> > + if (keep_base) {
> > + oidcpy(&merge_base, &options.onto->object.oid);
> > + } else {
> > + fill_merge_base(&options, &merge_base);
> > + }
>
> No need for braces around single-statement block on either side.
... already addresses this feedback.
Ciao,
DschoThere was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi Junio
On 15/08/2022 18:22, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Separate out calculating the merge base between onto and head from the
>> check for whether we can fast-forward or not. This means we can skip
>> the fast-forward checks when the rebase is forced and avoid
>> calculating the merge-base twice when --keep-base is given.
I should clarify that this is referring to the merge base of onto and upstream.
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>> Note the unnecessary braces around "if (keep_base)" are added here
>> reduce the churn on the next commit.
>> ---
>> builtin/rebase.c | 35 +++++++++++++++++++++++------------
>> 1 file changed, 23 insertions(+), 12 deletions(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 6cf9c95f4e1..86ea731ca3a 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -871,13 +871,9 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
>> struct commit_list *merge_bases = NULL;
>> int res = 0;
>> >> - merge_bases = get_merge_bases(onto, head);
>> - if (!merge_bases || merge_bases->next) {
>> - oidcpy(merge_base, null_oid());
>> + if (is_null_oid(merge_base))
>> goto done;
>> - }
>> >> - oidcpy(merge_base, &merge_bases->item->object.oid);
>> if (!oideq(merge_base, &onto->object.oid))
>> goto done;
> > Looking at the change in "git show -W", it seems that this function
> no longer touches merge_bases at all, other than initializing it to
> NULL at the beginning and then calling free_commit_list() on it at
> the end. Shouldn't it be removed?
There is still the line
merge_bases = get_merge_bases(upstream, head);
lower down. I should remove the call to free_commit_list() just above that line though as it is no longer needed.
>> @@ -902,6 +898,20 @@ done:
>> return res && is_linear_history(onto, head);
>> }
>> >> +static void fill_merge_base(struct rebase_options *options,
>> + struct object_id *merge_base)
>> +{
>> + struct commit_list *merge_bases = NULL;
>> +
>> + merge_bases = get_merge_bases(options->onto, options->orig_head);
>> + if (!merge_bases || merge_bases->next)
>> + oidcpy(merge_base, null_oid());
>> + else
>> + oidcpy(merge_base, &merge_bases->item->object.oid);
>> +
>> + free_commit_list(merge_bases);
>> +}
>> +
>> static int parse_opt_am(const struct option *opt, const char *arg, int unset)
>> {
>> struct rebase_options *opts = opt->value;
>> @@ -1668,7 +1678,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>> die(_("Does not point to a valid commit '%s'"),
>> options.onto_name);
>> }
>> -
>> + if (keep_base) {
>> + oidcpy(&merge_base, &options.onto->object.oid);
This is actually unnecessary as we do
options.onto = lookup_commit_reference_by_name(options.onto_name);
above when calculating onto.
>> + } else {
>> + fill_merge_base(&options, &merge_base);
>> + }
> > No need for braces around single-statement block on either side.
> > This is not a new issue introduced by this patch per-se, but
> "merge_base" is becoming less and less accurate description of what
> this variable really is. Perhaps it is a good time to rename it?
Yes, merge_base is not a very descriptive name as it prompts the question "merge base of which commits". I think base_commit or branch_base would be better.
> It is "the base commit to rebuild the history on top of", aka "onto
> commit", isn't it?
I think it is the base commit of the branch i.e. we rebase all the commits in the range merge_base..branch onto the "onto commit".
> We often use merge-base between the upstream and
> our tip of the history for it,
I'm pretty sure it is always the merge-base of the "onto commit" and our tip of the history. When using --keep-base we calculate the "onto commit" as the merge base of upstream and our tip of the history which makes it look were using that for merge_base but that commit is also the merge-base of the "onto commit" and our tip of history.
Best Wishes
Phillip
> but the variable often does not even
> hold the merge-base in it, not because we cannot compute a single
> merge-base but because depending on the operation mode we do not
> want to use merge-base for further operation using that variable.There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Junio,
>
> On Mon, 15 Aug 2022, Junio C Hamano wrote:
>
>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
>> >
>> > Separate out calculating the merge base between onto and head from the
>> > check for whether we can fast-forward or not. This means we can skip
>> > the fast-forward checks when the rebase is forced and avoid
>> > calculating the merge-base twice when --keep-base is given.
>> >
>> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> > ---
>> > Note the unnecessary braces around "if (keep_base)" are added here
>> > reduce the churn on the next commit.
>
> This note...
>
>> > @@ -1668,7 +1678,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>> > die(_("Does not point to a valid commit '%s'"),
>> > options.onto_name);
>> > }
>> > -
>> > + if (keep_base) {
>> > + oidcpy(&merge_base, &options.onto->object.oid);
>> > + } else {
>> > + fill_merge_base(&options, &merge_base);
>> > + }
>>
>> No need for braces around single-statement block on either side.
>
> ... already addresses this feedback.
Yeah, but the point is instead of wasting readers' bandwidth with an
additional note, the series can add braces in the step where they
become necessary, i.e. the later step where there is a new statement
in the body of the "if-true" side.There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Phillip Wood <phillip.wood123@gmail.com> writes:
>>> - merge_bases = get_merge_bases(onto, head);
>>> - if (!merge_bases || merge_bases->next) {
>>> - oidcpy(merge_base, null_oid());
>>> + if (is_null_oid(merge_base))
>>> goto done;
>>> - }
>>> - oidcpy(merge_base, &merge_bases->item->object.oid);
>>> if (!oideq(merge_base, &onto->object.oid))
>>> goto done;
>> Looking at the change in "git show -W", it seems that this function
>> no longer touches merge_bases at all, other than initializing it to
>> NULL at the beginning and then calling free_commit_list() on it at
>> the end. Shouldn't it be removed?
>
> There is still the line
>
> merge_bases = get_merge_bases(upstream, head);
>
> lower down. I should remove the call to free_commit_list() just above
> that line though as it is no longer needed.
Yup, that is correct.
Thanks.
|
This branch is now known as |
|
This patch series was integrated into seen via git@95fe5ff. |
|
This patch series was integrated into seen via git@e49c21c. |
| @@ -218,7 +218,7 @@ leave out at most one of A and B, in which case it defaults to HEAD. | |||
| merge base of `<upstream>` and `<branch>`. Running | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> As --keep-base does not rebase the branch it is confusing if it
> removes commits that have been cherry-picked to the upstream branch.
> As --reapply-cherry-picks is not supported by the "apply" backend this
> commit ensures that cherry-picks are reapplied by forcing the upstream
> commit to match the onto commit unless --no-reapply-cherry-picks is
> given.
>
> Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> Documentation/git-rebase.txt | 2 +-
> builtin/rebase.c | 15 ++++++++++++++-
> t/t3416-rebase-onto-threedots.sh | 21 +++++++++++++++++++++
> 3 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 080658c8710..dc0c6c54e27 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -218,7 +218,7 @@ leave out at most one of A and B, in which case it defaults to HEAD.
> merge base of `<upstream>` and `<branch>`. Running
> `git rebase --keep-base <upstream> <branch>` is equivalent to
> running
> - `git rebase --onto <upstream>...<branch> <upstream> <branch>`.
> + `git rebase --reapply-cherry-picks --onto <upstream>...<branch> <upstream> <branch>`.
> +
> This option is useful in the case where one is developing a feature on
> top of an upstream branch. While the feature is being worked on, the
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 86ea731ca3a..b6b3e00e3b1 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1181,6 +1181,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> prepare_repo_settings(the_repository);
> the_repository->settings.command_requires_full_index = 0;
>
> + options.reapply_cherry_picks = -1;
> options.allow_empty_message = 1;
> git_config(rebase_config, &options);
> /* options.gpg_sign_opt will be either "-S" or NULL */
> @@ -1240,6 +1241,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> if (options.root)
> die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--root");
> }
> + /*
> + * --keep-base defaults to --reapply-cherry-picks as it is confusing if
> + * commits disappear when using this option.
> + */
> + if (options.reapply_cherry_picks < 0)
> + options.reapply_cherry_picks = keep_base;
It makes me wonder if an explicit "--no-reapply-cherry-picks" makes
sense in combination with "--keep-base". If that happens, we do not
take this "By default, reapply is enabled with keep-base".
> @@ -1416,7 +1423,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> if (options.empty != EMPTY_UNSPECIFIED)
> imply_merge(&options, "--empty");
>
> - if (options.reapply_cherry_picks)
> + /*
> + * --keep-base implements --reapply-cherry-picks by altering upstream so
> + * it works with both backends.
> + */
> + if (options.reapply_cherry_picks && !keep_base)
> imply_merge(&options, "--reapply-cherry-picks");
Interesting. The idea is that we shouldn't care how much progress
(which may include cherry-picks) the upstream side made, and it is
no use to compare the commits between the F (fork point) and O (our
tip) against the commits between updated U (upstream) and F (fork
point) to notice that X' is a cherry-pick from our X.
o---X---o---O (our work)
/
----F----o----o----o----X'----U (upstream)
So almost ignoring U (except for obviously figure out F, possibly,
for the purpose of keep-base) is an effective way to keep X on our
history, and when it happens, we do not have to explicitly pass the
"--reapply" option to underlying rebase machinery. Makes sense.
If an explicit "--no-reapply-cherry-picks" with "--keep-base" is
given, we still skip this and do not call imply_merge() ...
> @@ -1680,6 +1691,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> }
> if (keep_base) {
> oidcpy(&merge_base, &options.onto->object.oid);
> + if (options.reapply_cherry_picks)
> + options.upstream = options.onto;
... but this is also skipped in such a case. I do not offhand know
if the combination makes practical sense, but this should allow the
combination to "work". OK.
Thanks.
|
There was a status update in the "New Topics" section about the branch "git rebase --keep-base" used to discard the commits that are already cherry-picked to the upstream, even when "keep-base" meant that the base, on top of which the history is being rebuilt, does not yet include these cherry-picked commits. The --keep-base option now implies --reapply-cherry-picks and --no-fork-point options. Needs review. source: <pull.1323.git.1660576283.gitgitgadget@gmail.com> |
| @@ -218,7 +218,7 @@ leave out at most one of A and B, in which case it defaults to HEAD. | |||
| merge base of `<upstream>` and `<branch>`. Running | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Given the name of the option it is confusing if --keep-base actually
> changes the base of the branch without --fork-point being explicitly
> given on the command line.
Does it merely "imply"? As keep-base requests exactly the same base
commit reused from the current history, doesn't fork-point a
competing and conflicting request, i.e. "please compute an
appropriate fork-point by looking at merge base with possibly
rewound tips of upstream branch"?
> + /*
> + * --keep-base ignores config.forkPoint as it is confusing if
> + * the branch base changes when using this option.
> + */
The comment singles out config.forkPoint (Isn't that "rebase.forkPoint"???)
as "confusing". Do we ignore rebase.forkPoint when --keep-base is given?
Do we honor --fork-point from the command line when --keep-base is given?
> + if (options.fork_point < 0)
> + options.fork_point = 0;
| @@ -68,7 +68,7 @@ struct rebase_options { | |||
| const char *upstream_name; | |||
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Phillip,
On Mon, 15 Aug 2022, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Using a struct commit rather than a struct oid to hold orig_head means
> that we error out straight away if branch being rebased does not point
> to a commit. It also simplifies the code than handles finding the
> merge base and fork point as it not longer has to convert from an oid
> to a commit.
Very nice!
The diff is necessarily "chatty", therefore it is especially good that you
separated out this concern into its own, easily-reviewable commit.
Thank you,
Dscho
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi Phillip,
On Mon, 15 Aug 2022, Phillip Wood via GitGitGadget wrote:
> A while a go Philippe reported [1] that he was surprised 'git rebase
> --keep-base' removed commits that had been cherry-picked upstream even
> though to branch was not being rebased.
This has bitten me, too (I did just not get around to reply to Philippe's
mail).
> I think it is also surprising if '--keep-base' changes the base of the
> branch without '--fork-point' being explicitly given on the command
> line. This series therefore changes the default behavior of
> '--keep-base' to imply '--reapply-cherry-picks' and '--no-fork-point' so
> that the base of the branch is unchanged and no commits are removed.
In my mind, `--keep-base` should always have adjusted the `<upstream>` to
point to the base commit, which is what patch 4/5 does.
So I am very much in favor of this patch series.
Junio seems to be on top of the code review, so I'll avoid stepping on his
toes by adding mine ;-)
Ciao,
Dscho |
|
User |
|
This patch series was integrated into seen via git@20f8869. |
Add a check for the correct error message to the tests that check we require a single merge base so we can be sure the rebase failed for the correct reason. Also rename the tests to reflect what they are testing. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
|
This patch series was integrated into seen via git@27d51a3. |
|
This patch series was integrated into seen via git@963732b. |
|
There was a status update in the "Cooking" section about the branch "git rebase --keep-base" used to discard the commits that are already cherry-picked to the upstream, even when "keep-base" meant that the base, on top of which the history is being rebuilt, does not yet include these cherry-picked commits. The --keep-base option now implies --reapply-cherry-picks and --no-fork-point options. Needs review. source: <pull.1323.git.1660576283.gitgitgadget@gmail.com> |
| @@ -68,7 +68,7 @@ struct rebase_options { | |||
| const char *upstream_name; | |||
There was a problem hiding this comment.
On the Git mailing list, Elijah Newren wrote (reply to this):
On Mon, Aug 15, 2022 at 8:14 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Using a struct commit rather than a struct oid to hold orig_head means
> that we error out straight away if branch being rebased does not point
> to a commit. It also simplifies the code than handles finding the
s/than/that/ ?
> merge base and fork point as it not longer has to convert from an oid
> to a commit.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> builtin/rebase.c | 62 ++++++++++++++++++++++--------------------------
> 1 file changed, 28 insertions(+), 34 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 56e4214b441..6cf9c95f4e1 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -68,7 +68,7 @@ struct rebase_options {
> const char *upstream_name;
> const char *upstream_arg;
> char *head_name;
> - struct object_id orig_head;
> + struct commit *orig_head;
> struct commit *onto;
> const char *onto_name;
> const char *revisions;
> @@ -261,13 +261,13 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
> struct replay_opts replay = get_replay_opts(opts);
> struct string_list commands = STRING_LIST_INIT_DUP;
>
> - if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,
> + if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head->object.oid,
> &revisions, &shortrevisions))
> return -1;
>
> if (init_basic_state(&replay,
> opts->head_name ? opts->head_name : "detached HEAD",
> - opts->onto, &opts->orig_head)) {
> + opts->onto, &opts->orig_head->object.oid)) {
> free(revisions);
> free(shortrevisions);
>
> @@ -298,9 +298,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
> split_exec_commands(opts->cmd, &commands);
> ret = complete_action(the_repository, &replay, flags,
> shortrevisions, opts->onto_name, opts->onto,
> - &opts->orig_head, &commands, opts->autosquash,
> - opts->update_refs,
> - &todo_list);
> + &opts->orig_head->object.oid, &commands,
> + opts->autosquash, opts->update_refs, &todo_list);
> }
>
> string_list_clear(&commands, 0);
> @@ -448,7 +447,8 @@ static int read_basic_state(struct rebase_options *opts)
> } else if (!read_oneliner(&buf, state_dir_path("head", opts),
> READ_ONELINER_WARN_MISSING))
> return -1;
> - if (get_oid(buf.buf, &opts->orig_head))
> + opts->orig_head = lookup_commit_reference_by_name(buf.buf);
> + if (!opts->orig_head)
> return error(_("invalid orig-head: '%s'"), buf.buf);
>
> if (file_exists(state_dir_path("quiet", opts)))
> @@ -517,7 +517,7 @@ static int rebase_write_basic_state(struct rebase_options *opts)
> write_file(state_dir_path("onto", opts), "%s",
> opts->onto ? oid_to_hex(&opts->onto->object.oid) : "");
> write_file(state_dir_path("orig-head", opts), "%s",
> - oid_to_hex(&opts->orig_head));
> + oid_to_hex(&opts->orig_head->object.oid));
> if (!(opts->flags & REBASE_NO_QUIET))
> write_file(state_dir_path("quiet", opts), "%s", "");
> if (opts->flags & REBASE_VERBOSE)
> @@ -646,7 +646,7 @@ static int run_am(struct rebase_options *opts)
> /* this is now equivalent to !opts->upstream */
> &opts->onto->object.oid :
> &opts->upstream->object.oid),
> - oid_to_hex(&opts->orig_head));
> + oid_to_hex(&opts->orig_head->object.oid));
>
> rebased_patches = xstrdup(git_path("rebased-patches"));
> format_patch.out = open(rebased_patches,
> @@ -680,7 +680,7 @@ static int run_am(struct rebase_options *opts)
> free(rebased_patches);
> strvec_clear(&am.args);
>
> - ropts.oid = &opts->orig_head;
> + ropts.oid = &opts->orig_head->object.oid;
> ropts.branch = opts->head_name;
> ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
> reset_head(the_repository, &ropts);
> @@ -833,7 +833,7 @@ static int checkout_up_to_date(struct rebase_options *options)
> strbuf_addf(&buf, "%s: checkout %s",
> getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
> options->switch_to);
> - ropts.oid = &options->orig_head;
> + ropts.oid = &options->orig_head->object.oid;
> ropts.branch = options->head_name;
> ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
> if (!ropts.branch)
> @@ -866,15 +866,11 @@ static int is_linear_history(struct commit *from, struct commit *to)
>
> static int can_fast_forward(struct commit *onto, struct commit *upstream,
> struct commit *restrict_revision,
> - struct object_id *head_oid, struct object_id *merge_base)
> + struct commit *head, struct object_id *merge_base)
> {
> - struct commit *head = lookup_commit(the_repository, head_oid);
> struct commit_list *merge_bases = NULL;
> int res = 0;
>
> - if (!head)
> - goto done;
> -
> merge_bases = get_merge_bases(onto, head);
> if (!merge_bases || merge_bases->next) {
> oidcpy(merge_base, null_oid());
> @@ -1312,13 +1308,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>
> if (read_basic_state(&options))
> exit(1);
> - ropts.oid = &options.orig_head;
> + ropts.oid = &options.orig_head->object.oid;
> ropts.branch = options.head_name;
> ropts.flags = RESET_HEAD_HARD;
> ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
> if (reset_head(the_repository, &ropts) < 0)
> die(_("could not move back to %s"),
> - oid_to_hex(&options.orig_head));
> + oid_to_hex(&options.orig_head->object.oid));
> remove_branch_state(the_repository, 0);
> ret = finish_rebase(&options);
> goto cleanup;
> @@ -1610,17 +1606,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> /* Is it a local branch? */
> strbuf_reset(&buf);
> strbuf_addf(&buf, "refs/heads/%s", branch_name);
> - if (!read_ref(buf.buf, &options.orig_head)) {
> + options.orig_head = lookup_commit_reference_by_name(buf.buf);
> + if (options.orig_head) {
> die_if_checked_out(buf.buf, 1);
> options.head_name = xstrdup(buf.buf);
> /* If not is it a valid ref (branch or commit)? */
> } else {
> - struct commit *commit =
> + options.orig_head =
> lookup_commit_reference_by_name(branch_name);
> - if (!commit)
> + if (!options.orig_head)
> die(_("no such branch/commit '%s'"),
> branch_name);
> - oidcpy(&options.orig_head, &commit->object.oid);
> options.head_name = NULL;
> }
> } else if (argc == 0) {
> @@ -1639,8 +1635,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> FREE_AND_NULL(options.head_name);
> branch_name = "HEAD";
> }
> - if (get_oid("HEAD", &options.orig_head))
> - die(_("Could not resolve HEAD to a revision"));
> + options.orig_head = lookup_commit_reference_by_name("HEAD");
> + if (!options.orig_head)
> + die(_("Could not resolve HEAD to a commit"));
> } else
> BUG("unexpected number of arguments left to parse");
>
> @@ -1672,13 +1669,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> options.onto_name);
> }
>
> - if (options.fork_point > 0) {
> - struct commit *head =
> - lookup_commit_reference(the_repository,
> - &options.orig_head);
> + if (options.fork_point > 0)
> options.restrict_revision =
> - get_fork_point(options.upstream_name, head);
> - }
> + get_fork_point(options.upstream_name, options.orig_head);
> +
>
> if (repo_read_index(the_repository) < 0)
> die(_("could not read index"));
> @@ -1708,7 +1702,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> * call it before checking allow_preemptive_ff.
> */
> if (can_fast_forward(options.onto, options.upstream, options.restrict_revision,
> - &options.orig_head, &merge_base) &&
> + options.orig_head, &merge_base) &&
> allow_preemptive_ff) {
> int flag;
>
> @@ -1785,7 +1779,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> strbuf_addf(&msg, "%s: checkout %s",
> getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
> ropts.oid = &options.onto->object.oid;
> - ropts.orig_head = &options.orig_head,
> + ropts.orig_head = &options.orig_head->object.oid,
> ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
> RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
> ropts.head_msg = msg.buf;
> @@ -1799,7 +1793,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> * we just fast-forwarded.
> */
> strbuf_reset(&msg);
> - if (oideq(&merge_base, &options.orig_head)) {
> + if (oideq(&merge_base, &options.orig_head->object.oid)) {
> printf(_("Fast-forwarded %s to %s.\n"),
> branch_name, options.onto_name);
> strbuf_addf(&msg, "rebase finished: %s onto %s",
> @@ -1820,7 +1814,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> (options.restrict_revision ?
> oid_to_hex(&options.restrict_revision->object.oid) :
> oid_to_hex(&options.upstream->object.oid)),
> - oid_to_hex(&options.orig_head));
> + oid_to_hex(&options.orig_head->object.oid));
>
> options.revisions = revisions.buf;
>
> --
> gitgitgadget
>
|
User |
| @@ -68,7 +68,7 @@ struct rebase_options { | |||
| const char *upstream_name; | |||
There was a problem hiding this comment.
On the Git mailing list, Elijah Newren wrote (reply to this):
On Mon, Aug 15, 2022 at 8:14 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Separate out calculating the merge base between onto and head from the
> check for whether we can fast-forward or not. This means we can skip
> the fast-forward checks when the rebase is forced and avoid
> calculating the merge-base twice when --keep-base is given.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> Note the unnecessary braces around "if (keep_base)" are added here
> reduce the churn on the next commit.
> ---
> builtin/rebase.c | 35 +++++++++++++++++++++++------------
> 1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 6cf9c95f4e1..86ea731ca3a 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -871,13 +871,9 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
> struct commit_list *merge_bases = NULL;
> int res = 0;
>
> - merge_bases = get_merge_bases(onto, head);
> - if (!merge_bases || merge_bases->next) {
> - oidcpy(merge_base, null_oid());
> + if (is_null_oid(merge_base))
> goto done;
> - }
>
> - oidcpy(merge_base, &merge_bases->item->object.oid);
> if (!oideq(merge_base, &onto->object.oid))
> goto done;
>
> @@ -902,6 +898,20 @@ done:
> return res && is_linear_history(onto, head);
> }
>
> +static void fill_merge_base(struct rebase_options *options,
> + struct object_id *merge_base)
> +{
> + struct commit_list *merge_bases = NULL;
> +
> + merge_bases = get_merge_bases(options->onto, options->orig_head);
> + if (!merge_bases || merge_bases->next)
> + oidcpy(merge_base, null_oid());
> + else
> + oidcpy(merge_base, &merge_bases->item->object.oid);
> +
> + free_commit_list(merge_bases);
> +}
> +
> static int parse_opt_am(const struct option *opt, const char *arg, int unset)
> {
> struct rebase_options *opts = opt->value;
> @@ -1668,7 +1678,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> die(_("Does not point to a valid commit '%s'"),
> options.onto_name);
> }
> -
> + if (keep_base) {
> + oidcpy(&merge_base, &options.onto->object.oid);
> + } else {
> + fill_merge_base(&options, &merge_base);
> + }
> if (options.fork_point > 0)
> options.restrict_revision =
> get_fork_point(options.upstream_name, options.orig_head);
> @@ -1697,13 +1711,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> * Check if we are already based on onto with linear history,
> * in which case we could fast-forward without replacing the commits
> * with new commits recreated by replaying their changes.
> - *
> - * Note that can_fast_forward() initializes merge_base, so we have to
> - * call it before checking allow_preemptive_ff.
> */
> - if (can_fast_forward(options.onto, options.upstream, options.restrict_revision,
> - options.orig_head, &merge_base) &&
> - allow_preemptive_ff) {
> + if (allow_preemptive_ff &&
> + can_fast_forward(options.onto, options.upstream, options.restrict_revision,
> + options.orig_head, &merge_base)) {
I didn't catch anything new in my review of this patch, but I just
really wanted to say how happy this final hunk makes me. I hated that
can_fast_forward() had to be called first; thanks for fixing that.
> int flag;
>
> if (!(options.flags & REBASE_FORCE)) {
> --
> gitgitgadget
phillipwood
force-pushed
the
wip/rebase--keep-base-tweaks
branch
2 times, most recently
from
c36cea3
to
9d5b907
Compare
As $EDITOR is exported, setting it in one test affects all subsequent tests. Avoid this by always setting it in a subshell. Also remove a couple of unnecessary call to set_fake_editor where the editor does not change the todo list. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
phillipwood
force-pushed
the
wip/rebase--keep-base-tweaks
branch
from
9d5b907
to
7dcc624
Compare
|
This patch series was integrated into seen via git@a0fa10a. |
|
Submitted as pull.1323.v4.git.1666012665.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@a524c62. |
|
This patch series was integrated into seen via git@5bf24c2. |
|
There was a status update in the "Cooking" section about the branch "git rebase --keep-base" used to discard the commits that are already cherry-picked to the upstream, even when "keep-base" meant that the base, on top of which the history is being rebuilt, does not yet include these cherry-picked commits. The --keep-base option now implies --reapply-cherry-picks and --no-fork-point options. Will merge to 'next'?? source: <pull.1323.v4.git.1666012665.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@9cd4cae. |
|
This patch series was integrated into seen via git@6be938f. |
|
There was a status update in the "Cooking" section about the branch "git rebase --keep-base" used to discard the commits that are already cherry-picked to the upstream, even when "keep-base" meant that the base, on top of which the history is being rebuilt, does not yet include these cherry-picked commits. The --keep-base option now implies --reapply-cherry-picks and --no-fork-point options. Will merge to 'next'?? source: <pull.1323.v4.git.1666012665.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch "git rebase --keep-base" used to discard the commits that are already cherry-picked to the upstream, even when "keep-base" meant that the base, on top of which the history is being rebuilt, does not yet include these cherry-picked commits. The --keep-base option now implies --reapply-cherry-picks and --no-fork-point options. Will merge to 'next'?? source: <pull.1323.v4.git.1666012665.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@40b9ec5. |
|
This patch series was integrated into seen via git@e88cb92. |
|
This patch series was integrated into seen via git@498c199. |
|
This patch series was integrated into seen via git@be9fbcc. |
|
This patch series was integrated into seen via git@6ed146f. |
|
This patch series was integrated into seen via git@152ad53. |
|
This patch series was integrated into seen via git@de75ec9. |
|
This patch series was integrated into seen via git@33cdf74. |
|
This patch series was integrated into seen via git@9cfe367. |
|
There was a status update in the "Cooking" section about the branch "git rebase --keep-base" used to discard the commits that are already cherry-picked to the upstream, even when "keep-base" meant that the base, on top of which the history is being rebuilt, does not yet include these cherry-picked commits. The --keep-base option now implies --reapply-cherry-picks and --no-fork-point options. Will merge to 'next'. source: <pull.1323.v4.git.1666012665.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@633c797. |
|
This patch series was integrated into next via git@802359a. |
|
This patch series was integrated into seen via git@44f7f4d. |
|
There was a status update in the "Cooking" section about the branch "git rebase --keep-base" used to discard the commits that are already cherry-picked to the upstream, even when "keep-base" meant that the base, on top of which the history is being rebuilt, does not yet include these cherry-picked commits. The --keep-base option now implies --reapply-cherry-picks and --no-fork-point options. Will merge to 'master'. source: <pull.1323.v4.git.1666012665.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@003f815. |
|
This patch series was integrated into master via git@003f815. |
|
This patch series was integrated into next via git@003f815. |
|
Closed via 003f815. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A while a go Philippe reported [1] that he was surprised 'git rebase --keep-base' removed commits that had been cherry-picked upstream even though to branch was not being rebased. I think it is also surprising if '--keep-base' changes the base of the branch without '--fork-point' being explicitly given on the command line. This series therefore changes the default behavior of '--keep-base' to imply '--reapply-cherry-picks' and '--no-fork-point' so that the base of the branch is unchanged and no commits are removed.
Thanks to Junio and Ævar for their comments, the changes since V3 are:
merge_basetobranch_basebut I think this is an improvement overall.Thanks to Junio for his comments, the changes since V2 are:
Thanks to everyone who commented for their reviews, the changes since V1 are:
[1] https://lore.kernel.org/git/0EA8C067-5805-40A7-857A-55C2633B8570@gmail.com/
Cc: Philippe Blain levraiphilippeblain@gmail.com
Cc: Denton Liu liu.denton@gmail.com
Cc: Johannes Schindelin Johannes.Schindelin@gmx.de
cc: Phillip Wood phillip.wood123@gmail.com
cc: Elijah Newren newren@gmail.com
cc: Junio C Hamano gitster@pobox.com
cc: Jonathan Tan jonathantanmy@google.com
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com