Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better support rebase workflows #5953

Closed
billygriffin opened this issue Oct 19, 2018 · 9 comments

Comments

Projects
None yet
5 participants
@billygriffin
Copy link
Contributor

commented Oct 19, 2018

A big theme of the past year of our work on Desktop has been working to make collaboration easier. A big part of that, and something we're still iterating on, is supporting merging work from one branch to another. However, in hearing from our users and talking to folks in the wild, we know that many workflows either require or benefit from rebasing as opposed to merging. Therefore, we'd like to make this possible in Desktop.

There are two primary problem areas we want to focus on.

  1. This is the problem described in #3422: When I pull in Desktop, I don't want it to create a merge commit because it clutters my commit history and makes it harder to quickly discern "real" commits.

  2. I'm required to (or prefer to) rebase when bringing code from one branch to another branch, and Desktop does not allow me to do this. (described in #1652)
    2a. Merging brings together branches in one fell swoop, and I can't tell what code was written when chronologically.

Considerations for problem 1:

  1. Is this an optional selection that exists in preferences? Is there some other way to choose to rebase when pulling as opposed to pulling normally?
  2. What happens if there's a conflict / multiple conflicts during the pull?

Considerations for problem 2:

  1. How do you enter the rebase experience?
  2. How do we show people what commits are going to be rebased so they have comfort around what's going to happen?
  3. How does it work when you run into a conflict / multiple conflicts throughout the rebase?
  4. How do you know when you're done?

Note: We're considering interactive rebasing as out of scope for this initial iteration, and may be something we look at further along.

Next steps:

  • Design prototype
  • Usability testing of prototype
  • Take feedback to produce designs and share
  • Build and release initial iteration

cc: @shiftkey @ampinsk @nerdneha

@billygriffin billygriffin added this to the 1.6.0 milestone Oct 25, 2018

@billygriffin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2018

A couple updates on this based on initial usability testing and internal discussion with @shiftkey, @ampinsk, and me:

  • We discussed the concept of force pushing after a rebase, as we are rewriting history in a rebase operation and therefore the local commits will sometimes differ from the remote commits. There are a couple scenarios where this will likely need to happen, and we want to be sure to make sure people understand this before rebasing:

    • Scenario 1: If you're rebasing branches that are both local and haven't been published, there's no concern about force pushing. Therefore we don't need to warn the user, and it'll be a regular push after rebasing.
    • Scenario 2: If the branch you're on is already published and you rebase in Desktop, you'll need to force push after rebasing. We want to account for this in two ways:
      • Warn the user before rebasing that they're rewriting history and will subsequently need to force push, possibly with a confirm dialogue (with "Don't show this again" option).
      • The "Push" button should indicate that it's a force push and not a regular push. The language and icon could possibly indicate this distinction.
  • We also discussed the concept of "Undo," and decided that our existing behavior is still appropriate after a rebase where we're allowing the last commit to be undone but only if it's a local commit and not yet on the remote.

  • We also discussed the concept of git pull --rebase if there are uncommitted changes. We decided that initially we'll handle this in the same way we handle git pull currently, but that there's potentially room to think about this in more detail in #6107. I'll update that issue with further detail.

@ampinsk

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2018

Designs

Here's where we've landed so far after a quick round of usability testing!

Problem 1: Pulling with rebase

When a user has rebase set in their config, the "Pull" button will update to "Pull with rebase".

No conflicts

With no conflicts, the pull would behave as normally

pull-clean

With conflicts

With conflicts, we'll introduce the conflict resolution modal with updated language for this use case

pull-conflicts

Problem 2: Rebase with another branch

  • Users will enter the experience from the Branch > Rebase Current Branch....
  • We'll show a dialog similar to the merge dialog to let you select a branch. A hint will let users know how many commits are going to be rebased.
  • If you'll have to force push, we'll let you know up front. Users can opt out of seeing this again.
  • A progress bar will take users through the commits they are rebasing.
  • When done, we'll show a success banner.
  • When force pushing, we'll warn you again. Users can opt out of seeing this again.

No conflicts

rebase-clean

With conflicts

  • This is the same flow, but when you encounter conflicts, we'll give you a heads up on the hint
  • As the progress bar takes you through commit by commit, we'll let you resolve conflicts along the way

rebase-conflicts

@sbonami

This comment has been minimized.

Copy link

commented Nov 14, 2018

While not directly related to inter-branch rebasing described above, the fixup/amend (#1644) feature request may be a great addition to the larger rebase workflows being considered.

At the very least, could we consider re-opening that issue as part of the larger re-prioritization of the milestone? (I'd be happy to discuss over there, it wasn't clear if this was the right place or if rebasing workflows were intentionally more succinct; no thread-hijacking intended!)

@billygriffin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

@sbonami Thanks for the courtesy, I didn't at all take it as thread hijacking. We're going to consider that separately, but having the underpinnings of rebasing in place certainly would make that easier to tackle looking forward.

Our primary goal here is to provide support for folks who either cannot (because their org mandates a rebase workflow) or choose not to use the default merge workflow, especially when pulling. I totally agree that it's reasonable to consider providing the ability to amend previous commits outside of the ability to "Undo" local commits that exists today, and we're slowly working our way through the future proposals, so we'll definitely be getting to that one and figuring out where it lies on prioritization. Thanks again for the feedback.

@EwoutH

This comment has been minimized.

Copy link

commented Jan 21, 2019

@ampinsk Looks amazing, when do you think it will be landed?

@billygriffin

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

Hi @EwoutH, thanks for the feedback! We're looking to ship at least the first portion of this in 1.6.3, likely next month. We'll likely ship that (pull --rebase support) on its own to make sure we understand fully the implications, and then ship explicitly rebasing one branch onto another later.

@ampinsk

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

The v1 of this is going to encompass pull with rebase. Here's a prototype for how that would go if you ran into conflicts, then needed to force push:

pull-rebase-force

cc @shiftkey

@shiftkey

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

@billygriffin there's one outstanding task in this issue body:

  • Build and release initial iteration

If you think the scope of 1.7.0 covers what we're planning to ship can we close this out?

@billygriffin

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

If you're ok with it @shiftkey, I'd love to keep this as the tracking issue for the broader set of work that's set to ship in 1.7.0, and it'll be closed then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.