Make rebase smarter when handling PRs with merges #183
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
leandro-lucarella-sociomantic
added a commit
to leandro-lucarella-sociomantic/git-hub
that referenced
this pull request
Sep 6, 2016
If the HEAD of the PR is a merge commit (has more than 1 parent), ask the user if they would like to do a `merge --ff-only` instead, as normally rebasing a merge commit results in horrors and nightmares. This feature should be considered experimental, in the future this might be done by default without asking or an option might be added to force this behaviour beforehand to avoid annoying interactive questions. Fixes sociomantic-tsunami#183.
leandro-lucarella-sociomantic
force-pushed
the
fix-183
branch
from
8aa88b8 to
a7346ce
Compare
leandro-lucarella-sociomantic
added a commit
to leandro-lucarella-sociomantic/git-hub
that referenced
this pull request
Sep 12, 2016
If the HEAD of the PR is a merge commit (has more than 1 parent), ask the user if they would like to do a `merge --ff-only` instead, as normally rebasing a merge commit results in horrors and nightmares. This feature should be considered experimental, in the future this might be done by default without asking or an option might be added to force this behaviour beforehand to avoid annoying interactive questions. Fixes sociomantic-tsunami#183.
|
Updated. The implementation was a bit more complicated than expected, but it seems to work with a simple test. More testing is extremely welcome. A lot of messages still say rebasing instead of merging when merging is used (although technically if a merge is detected, then this is a NOP, is just pushing the HEAD of the PR as the new BASE). I'm not sure if it's needed to change the wording, as conceptually we are doing a "rebase" (even when it won't be one). |
leandro-lucarella-sociomantic
force-pushed
the
fix-183
branch
from
a7346ce to
04c834a
Compare
leandro-lucarella-sociomantic
added a commit
to leandro-lucarella-sociomantic/git-hub
that referenced
this pull request
Sep 14, 2016
If the HEAD of the PR is a merge commit (has more than 1 parent), ask the user if they would like to do a `merge --ff-only` instead, as normally rebasing a merge commit results in horrors and nightmares. This feature should be considered experimental, in the future this might be done by default without asking or an option might be added to force this behaviour beforehand to avoid annoying interactive questions. Fixes sociomantic-tsunami#183.
|
I would merge this and make it part of an alpha release so we can get more people to try it out without having to install the script manually from the devel repo. |
leandro-lucarella-sociomantic
assigned nemanja-boric-sociomantic and unassigned leandro-lucarella-sociomantic
leandro-lucarella-sociomantic
force-pushed
the
fix-183
branch
from
04c834a to
ce19661
Compare
leandro-lucarella-sociomantic
added a commit
to leandro-lucarella-sociomantic/git-hub
that referenced
this pull request
Sep 27, 2016
If the HEAD of the PR is a merge commit (has more than 1 parent), ask the user if they would like to do a `merge --ff-only` instead, as normally rebasing a merge commit results in horrors and nightmares. This feature should be considered experimental, in the future this might be done by default without asking or an option might be added to force this behaviour beforehand to avoid annoying interactive questions. Fixes sociomantic-tsunami#183.
leandro-lucarella-sociomantic
added a commit
to leandro-lucarella-sociomantic/git-hub
that referenced
this pull request
Sep 27, 2016
If the HEAD of the PR is a merge commit (has more than 1 parent), ask the user if they would like to do a `merge --ff-only` instead, as normally rebasing a merge commit results in horrors and nightmares. This feature should be considered experimental, in the future this might be done by default without asking or an option might be added to force this behaviour beforehand to avoid annoying interactive questions. Fixes sociomantic-tsunami#183.
| starting = args.action is None | ||
| try: | ||
| if starting: | ||
| # Last commit is a merge commit, so ask the user to merge |
There was a problem hiding this comment.
Does this really ask user to do anything?
There was a problem hiding this comment.
I think I moved the question to an earlier stage.
There was a problem hiding this comment.
yeah it is here, but this comment is outdated then: https://github.com/sociomantic-tsunami/git-hub/pull/183/files#diff-88fb15e32f4101f90004ef18b66b704bR1800
There was a problem hiding this comment.
Updated.
If the HEAD of the PR is a merge commit (has more than 1 parent), ask the user if they would like to do a `merge --ff-only` instead, as normally rebasing a merge commit results in horrors and nightmares. This feature should be considered experimental, in the future this might be done by default without asking or an option might be added to force this behaviour beforehand to avoid annoying interactive questions. Fixes sociomantic-tsunami#183.
leandro-lucarella-sociomantic
force-pushed
the
fix-183
branch
from
ce19661 to
459959e
Compare
nemanja-boric-sociomantic
pushed a commit
that referenced
this pull request
Sep 27, 2016
If the HEAD of the PR is a merge commit (has more than 1 parent), ask the user if they would like to do a `merge --ff-only` instead, as normally rebasing a merge commit results in horrors and nightmares. This feature should be considered experimental, in the future this might be done by default without asking or an option might be added to force this behaviour beforehand to avoid annoying interactive questions. Fixes #183.
nemanja-boric-sociomantic
approved these changes
Sep 27, 2016
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.
When maintaining multiple branches is common to apply bug fixes in old branches and then merge the old branches in newer ones. If a PR is created for those merges,
git hub rebasewill fail miserably at trying to rebase the PR with a merge.For this reason it could be good to detect merges in PRs when doing a
git hub rebaseand offer the user to do agit merge --ff-onlyinstead of a rebase, which is usually what you want to do for that kind of PRs.