Drop support for git rebase --preserve-merges
#195
Closed
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
dscho
added
the
needs-work
dscho
force-pushed
the
drop-rebase-p
branch
2 times, most recently
from
9d3ba72
to
ea6237a
Compare
dscho
changed the base branch from
js/rebase-cleanup
to
js/git-svn-use-rebase-merges
dscho
added
ready to submit
|
/submit |
|
Submitted as pull.195.git.1574542242.gitgitgadget@gmail.com |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -678,7 +678,6 @@ config key: svn.authorsProg | |||
| --strategy=<strategy>:: | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Wong wrote (reply to this):
Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:
> We already passed the `--rebase-merges` option to `git rebase` instead,
> now we make this move permanent.
> diff --git a/git-svn.perl b/git-svn.perl
> index 4aa208ff5f..f1fa1bc7f7 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -271,7 +271,6 @@ sub _req_svn {
> 'fetch-all|all' => \$_fetch_all,
> 'dry-run|n' => \$_dry_run,
> 'rebase-merges|p' => \$_rebase_merges,
> - 'preserve-merges|p' => \$_rebase_merges,
> %fc_opts } ],
> 'commit-diff' => [ \&cmd_commit_diff,
> 'Commit a diff between two trees',
Nack, it breaks existing usages. Why the urgency with removal?
I don't know a whole lot about this rebase feature in
particular, but deprecation periods should be measured in years
or even decades because of LTS distros. Not months, especially
for things which have been around for a long while.
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Eric,
On Sat, 23 Nov 2019, Eric Wong wrote:
> Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:
> > We already passed the `--rebase-merges` option to `git rebase` instead=
,
> > now we make this move permanent.
>
> > diff --git a/git-svn.perl b/git-svn.perl
> > index 4aa208ff5f..f1fa1bc7f7 100755
> > --- a/git-svn.perl
> > +++ b/git-svn.perl
> > @@ -271,7 +271,6 @@ sub _req_svn {
> > 'fetch-all|all' =3D> \$_fetch_all,
> > 'dry-run|n' =3D> \$_dry_run,
> > 'rebase-merges|p' =3D> \$_rebase_merges,
> > - 'preserve-merges|p' =3D> \$_rebase_merges,
> > %fc_opts } ],
> > 'commit-diff' =3D> [ \&cmd_commit_diff,
> > 'Commit a diff between two trees',
>
> Nack, it breaks existing usages. Why the urgency with removal?
Which urgency? The cover letter spells it out quite clearly that this is
not even intended for v2.25.0, which is still over 2 months out.
The reason I submitted this patch series now is so that we can avoid
inadvertent new users of the `--preserve-merges` backend.
> I don't know a whole lot about this rebase feature in
> particular, but deprecation periods should be measured in years
> or even decades because of LTS distros. Not months, especially
> for things which have been around for a long while.
The LTS distros will not even pick up this patch. So that's a red herring.
But yes, you're right, v2.25.0 will probably be the first version to even
have the `--rebase-merges` option in `git svn`, and therefore v2.26.0
would be awfully early a time to drop `--preserve-merges` in `git svn`.
Question is whether we want to split this patch series, or just rather
wait with merging it to `master` until a year from now, or something like
that?
Ciao,
Dscho
There was a problem hiding this comment.
On the Git mailing list, Eric Wong wrote (reply to this):
Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:
> > > We already passed the `--rebase-merges` option to `git rebase` instead,
> > > now we make this move permanent.
> >
> > > diff --git a/git-svn.perl b/git-svn.perl
> > > index 4aa208ff5f..f1fa1bc7f7 100755
> > > --- a/git-svn.perl
> > > +++ b/git-svn.perl
> > > @@ -271,7 +271,6 @@ sub _req_svn {
> > > 'fetch-all|all' => \$_fetch_all,
> > > 'dry-run|n' => \$_dry_run,
> > > 'rebase-merges|p' => \$_rebase_merges,
> > > - 'preserve-merges|p' => \$_rebase_merges,
> > > %fc_opts } ],
> > > 'commit-diff' => [ \&cmd_commit_diff,
> > > 'Commit a diff between two trees',
> >
> > Nack, it breaks existing usages. Why the urgency with removal?
>
> Which urgency? The cover letter spells it out quite clearly that this is
> not even intended for v2.25.0, which is still over 2 months out.
"Months" a blink of an eye when it comes to deprecations and removals.
> The reason I submitted this patch series now is so that we can avoid
> inadvertent new users of the `--preserve-merges` backend.
Then documenting it as deprecated and warning is all that's
needed.
> > I don't know a whole lot about this rebase feature in
> > particular, but deprecation periods should be measured in years
> > or even decades because of LTS distros. Not months, especially
> > for things which have been around for a long while.
>
> The LTS distros will not even pick up this patch. So that's a red herring.
>
> But yes, you're right, v2.25.0 will probably be the first version to even
> have the `--rebase-merges` option in `git svn`, and therefore v2.26.0
> would be awfully early a time to drop `--preserve-merges` in `git svn`.
> Question is whether we want to split this patch series, or just rather
> wait with merging it to `master` until a year from now, or something like
> that?
Fwiw, I object to the regressions to all the other commands
(rebase/pull/remote) in this series, too, but I mainly do Perl.
--preserve-merges was only deprecated in v2.22.0 (2019-06-07).
LTS distro users are very likely on pre-v2.22.0, more likely
v2.1x.0 and maybe even v2.x.0.
Their next LTS release could be several years from now. We
could be on git 2.[345]x.0 by then and that's when the LTS
packagers could package the next version. LTS users are likely
to never see the entire period from v2.22.0..v2.25.0 and thus
never see a deprecation warning.
Even Debian stable (not exactly LTS, but still on the slower
side) went from v2.11.0 in Debian 9 all the way to v2.20.1
in Debian 10. Actual LTS users will see bigger jumps.
Even if those tests try to override that setting, let's not use it because it is deprecated: let's use `merges` instead. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
The pull request has 6443 commits. The max allowed is 30. Please split the patch series into multiple pull requests. Also consider squashing related commits. |
|
There are merge commits in this Pull Request: Please rebase the branch and force-push. |
We ignore them silently, but it actually makes sense to warn the users that their config setting has no effect. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
/submit |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
In preparation for `git-rebase--preserve-merges.sh` entering its after life, we remove this (deprecated) option that would still rely on it. To help users transition who still did not receive the memo about the deprecation, we offer a helpful error message instead of throwing our hands in the air and saying that we don't know that option, never heard of it. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This option was deprecated in favor of `--rebase-merges` some time ago, and now we retire it. To assist users to transition away, we do not _actually_ remove the option, but now we no longer implement the functionality. Instead, we offer a helpful error message suggesting which option to use. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
We already passed the `--rebase-merges` option to `git rebase` instead, now we make this move permanent. As pointed out by Ævar Arnfjörð Bjarmason, in contrast to the deprecation of `git rebase`'s `--preserve-merges` backend, `git svn` only deprecated this option in v2.25.0 (because this developer missed `git svn`'s use of that backend when deprecating the rebase backend running up to Git v2.22). Still, v2.25.0 has been released on January 13th, 2020. In other words, `git svn` deprecated this option over one and a half years ago, _and_ has been redirecting to the `--rebase-merges` option during all that time (read: `git svn rebase --preserve-merges` didn't do _precisely_ what the user asked, since v2.25.0, anyway, it fell back to pretending that the user asked for `git svn rebase --rebase-merges` instead). It is time to act on that deprecation and remove that option after all. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It was only used by the `--preserve-merges` backend, which we just removed. Helped-by: Alban Gruin <alban.gruin@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Now that we no longer have a `--preserve-merges` backend, this comment needs to be adjusted. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
We no longer support `--preserve-merges`, therefore it does not make sense to keep mentioning that option, even in code comments. Helped-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
With the `--preserve-merges` option going away, we no longer need this function. Helped-by: Alban Gruin <alban.gruin@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The function to add the `exec` commands to the todo list only needed to be public API because it was not only used internally by the sequencer, but also by `git rebase --preserve-merges`. Now that that mode has been removed, we no longer need that function to be scoped publicly. Helped-by: Alban Gruin <alban.gruin@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
/submit |
|
Submitted as pull.195.v3.git.1631048712.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
|
User |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
| @@ -125,7 +125,6 @@ | |||
| /git-range-diff | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Tue, Sep 07 2021, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 33e09619005..5f8d9f89ba4 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -48,8 +48,7 @@ static GIT_PATH_FUNC(merge_dir, "rebase-merge")
> enum rebase_type {
> REBASE_UNSPECIFIED = -1,
> REBASE_APPLY,
> - REBASE_MERGE,
> - REBASE_PRESERVE_MERGES
> + REBASE_MERGE
> };
This definitely doesn't require a re-roll, but just in case you didn't
know, from CodingGuidelines:
. since early 2012 with e1327023ea, we have been using an enum
definition whose last element is followed by a comma. This, like
an array initializer that ends with a trailing comma, can be used
to reduce the patch noise when adding a new identifier at the end.
(That was added in cc0c42975a2 (CodingGuidelines: spell out post-C89
rules, 2019-07-16))
I.e. in case you're slavisly following this particular bit of C syntax
for C89 compatibility it's not needed anymore, which helps to make diffs
smaller, and writing code generation loops less annoying.
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
|
Merged via 223a1bf |
|
/test 1 (apologies) |
|
Error: User softworkz is not permitted to use GitGitGadget |
|
/test 2 (apologies) |
|
Error: User softworkz is not permitted to use GitGitGadget |
|
/test 3 (apologies) |
|
Error: User softworkz is not permitted to use GitGitGadget |
| @@ -125,7 +125,6 @@ | |||
| /git-range-diff | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Tue, Sep 07 2021, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> [...]
> --p::
> ---preserve-merges::
> - [DEPRECATED: use `--rebase-merges` instead] Recreate merge commits
> - instead of flattening the history by replaying commits a merge commit
> - introduces. Merge conflict resolutions or manual amendments to merge
> - commits are not preserved.
[In reply to an old commit]
I opened "man git-rebase" today due to an on-list discussion and went
through pretty much:
1. /preserve-merges # fails
2. skimming the SYNOPSIS, forgetting what the new thing is called
3. Paging down, eventually findinging & remembering the new thing is
"--rebase-merges".
I wonder if there's objections to reinstating this in the docs
somewhere, just as something like:
--preserve-merges:
An old "rebase" backend which is no longer supported,
and which was removed from git in version v2.35.0.
We don't do that with all flags that we've dropped, but perhaps this one
was well known enough to not leave readers hanging...
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> On Tue, Sep 07 2021, Johannes Schindelin via GitGitGadget wrote:
>
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>> [...]
>> --p::
>> ---preserve-merges::
>> - [DEPRECATED: use `--rebase-merges` instead] Recreate merge commits
>> - instead of flattening the history by replaying commits a merge commit
>> - introduces. Merge conflict resolutions or manual amendments to merge
>> - commits are not preserved.
>
> [In reply to an old commit]
>
> I opened "man git-rebase" today due to an on-list discussion and went
> through pretty much:
>
> 1. /preserve-merges # fails
> 2. skimming the SYNOPSIS, forgetting what the new thing is called
> 3. Paging down, eventually findinging & remembering the new thing is
> "--rebase-merges".
>
> I wonder if there's objections to reinstating this in the docs
> somewhere, just as something like:
>
> --preserve-merges:
> An old "rebase" backend which is no longer supported,
> and which was removed from git in version v2.35.0.
>
> We don't do that with all flags that we've dropped, but perhaps this one
> was well known enough to not leave readers hanging...
My impression is that we consider that we have done so already for a
few releases by keeping "DEPRECATED: use rebase-merges", exactly
because "this one was well known enough", and now it is time to go
one step further, i.e. drop it from the document like the quoted
patch does, while recognising an attempt to use the option and
giving a custom message than the bog-standard "unknown option".
$ git rebase --preserve-merges
fatal: --preserve-merges was replaced by --rebase-merges
Note: Your `pull.rebase` configuration may also be set to 'preserve',
which is no longer supported; use 'merges' instead
The next step will be to drop that custom error support, I think.There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Junio,
On Thu, 21 Jul 2022, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
> > On Tue, Sep 07 2021, Johannes Schindelin via GitGitGadget wrote:
> >
> >> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >> [...]
> >> --p::
> >> ---preserve-merges::
> >> - [DEPRECATED: use `--rebase-merges` instead] Recreate merge commits
> >> - instead of flattening the history by replaying commits a merge commit
> >> - introduces. Merge conflict resolutions or manual amendments to merge
> >> - commits are not preserved.
> >
> > [In reply to an old commit]
> >
> > I opened "man git-rebase" today due to an on-list discussion and went
> > through pretty much:
> >
> > 1. /preserve-merges # fails
> > 2. skimming the SYNOPSIS, forgetting what the new thing is called
> > 3. Paging down, eventually findinging & remembering the new thing is
> > "--rebase-merges".
> >
> > I wonder if there's objections to reinstating this in the docs
> > somewhere, just as something like:
> >
> > --preserve-merges:
> > An old "rebase" backend which is no longer supported,
> > and which was removed from git in version v2.35.0.
> >
> > We don't do that with all flags that we've dropped, but perhaps this one
> > was well known enough to not leave readers hanging...
>
> My impression is that we consider that we have done so already for a
> few releases by keeping "DEPRECATED: use rebase-merges", exactly
> because "this one was well known enough", and now it is time to go
> one step further, i.e. drop it from the document like the quoted
> patch does, while recognising an attempt to use the option and
> giving a custom message than the bog-standard "unknown option".
>
> $ git rebase --preserve-merges
> fatal: --preserve-merges was replaced by --rebase-merges
> Note: Your `pull.rebase` configuration may also be set to 'preserve',
> which is no longer supported; use 'merges' instead
>
> The next step will be to drop that custom error support, I think.
Fully agree.
I _could_ see us introducing a sentence in the explanation of
`--rebase-merges` that leaves a historical note about superseding the
now-removed `--preserve-merges` option. But such historical notes tend to
go pretty stale pretty quickly, and eventually cause more confusion than
clarification.
So just like you said, I'd rather not re-introduce any text mentioning
`--preserve-merges` into the manual page.
Ciao,
Dscho
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.
In 427c3bd (rebase: deprecate --preserve-merges, 2019-03-11) (which was included in v2.22.0), we officially deprecated the
--preserve-mergesbackend. Over two years later, it is time to drop that backend, and here is a patch series that does just that.Changes since v2:
script_snippetvariablegit svnpatch to clarify that the deprecation happened only in v2.25 thereACTION_*enum valuescheck_todo_list_from_file()because it is no longer neededtodo_list_add_exec_commands()function is now scoped to the file (because there are no longer any outside users)--rebase-mergesinsteadChanges since v1:
Cc: Eric Wong e@80x24.org
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com
cc: Johannes Sixt j6t@kdbg.org
cc: Alban Gruin alban.gruin@gmail.com
cc: Phillip Wood phillip.wood123@gmail.com
cc: Elijah Newren newren@gmail.com