[rebase] menu item and dialog to start rebase operation by shiftkey · Pull Request #6961 · desktop/desktop

shiftkey · 2019-02-27T16:41:05Z

Overview

Closes #6551
Closes #7012
Closes #6961

Description

This is the first part of enabling the user to rebase their current branch from within the app in a guided fashion, and I've omitted much of the background computation from #6959 and #6960 so that I can have something baked to help with testing.

The menu item is enabled only for development currently.

Testing:

can start a rebase, and dialog closes because no conflicts with success banner shown
can start a rebase, and dialog switches to show conflicts if encountered
can start a rebase, can address conflicts, and continue rebase until success banner shown
address TODO comments

Release notes

Notes:

shiftkey · 2019-02-28T14:10:00Z

app/src/main-process/menu/build-default-menu.ts

+          ? 'Rebase Current Branch…'
+          : 'R&ebase current branch…',
+        id: 'rebase-branch',
+        accelerator: 'CmdOrCtrl+Shift+E',


I settled on E here because R or B are already assigned to other shortcuts

shiftkey · 2019-02-28T14:10:57Z

app/src/ui/dispatcher/dispatcher.ts

+
+    log.info(`[rebase] starting rebase for ${beforeSha}`)
+
+    // TODO: this can happen very quickly for a trivial rebase or an OS with


Going to keep this around because I have #6962 to tackle 🔜

shiftkey · 2019-03-01T13:36:26Z

app/src/lib/git/rebase.ts

+    return RebaseResult.CompletedWithoutError
  }

-  if (rebaseEncounteredConflictsRe.test(result.stdout)) {


I bumped dugite to the latest version to replace this regex inspection with proper error codes for the known rebase problem scenarios: desktop/dugite#306

tierninho · 2019-03-05T00:28:19Z

Tested the new menu option/shortcut as of 76d48b9 and found no issues.

Curious if it makes sense to allow the user to rebase the same two branches twice in a row?

shiftkey · 2019-03-05T14:01:47Z

Curious if it makes sense to allow the user to rebase the same two branches twice in a row?

I don't think it's worth actively trying to block this case unless there's downsides to this. The second rebase would be a clean rebase (because the user resolved any conflicts in the first rebase).

tierninho · 2019-03-05T21:08:40Z

If a merge conflict is in progress and is currently in manual resolution mode (banner present), then the Rebase current branch menu option should not be enabled as it results in an error message:

install.ts:23 `git rebase sdfsdfsdfsdfsdfsdfsd test-pr-creation` exited with an unexpected code: 1.
.gitignore: needs merge
UnitTestProject3/UnitTestProject3/UnitTest2.cs: needs merge

error: cannot rebase: You have unstaged changes.
error: additionally, your index contains uncommitted changes.
error: Please commit or stash them.

(⚠️ Note this is part of a wider problem that might want to be addressed elsewhere as multiple menu options are selectable in this state, and doing so can leads to other errors).

shiftkey · 2019-03-05T21:43:49Z

(⚠️ Note this is part of a wider problem that might want to be addressed elsewhere as multiple menu options are selectable in this state, and doing so can leads to other errors).

👍 you can currently try merging a branch while also dealing with merge conflicts. I'll spin this out to an issue to track tomorrow.

shiftkey · 2019-03-06T13:01:32Z

I've opened #7017 to audit the menu items and figure out which should be enabled when dealing with conflicts.

shiftkey · 2019-03-06T14:43:18Z

Marking this as time-sensitive because it's key to all rebase functionality in 1.7.0

app/src/lib/git/rebase.ts

app/src/ui/rebase/rebase-branch-dialog.tsx

app/styles/ui/dialogs/_rebase-conflicts.scss

app/src/ui/rebase/rebase-branch-dialog.tsx

app/styles/ui/dialogs/_rebase-conflicts.scss

Co-Authored-By: shiftkey <brendan@github.com>

shiftkey · 2019-03-08T17:08:27Z

FYI: I've extracted the feedback from this PR that can also be applied to the Merge Conflicts dialog into #7043 - that can be picked up once this and #7030 are merged.

niik · 2019-03-11T08:21:54Z

app/src/ui/dialog/dialog.tsx

+      // createUniqueId handles static strings fine, so in the case of receiving
+      // a JSX element for the title we can just pass in a fixed value rather
+      // than trying to generate a string from an arbitrary element
+      const id = typeof this.props.title === 'string' ? this.props.title : '???'


Not gonna hold up this from landing because of this but ??? stands out to me here. I would have preferred if we either left this code as it was, or that we made this constant something like JSX rather than triple question marks. It's entirely stylistic though so I'm gonna go ahead and approve and land this.

niik · 2019-03-11T08:24:02Z

app/src/ui/rebase/rebase-branch-dialog.tsx

+        onSubmit={this.startRebase}
+        loading={loading}
+        disabled={disabled}
+        dismissable={true}


Again, stylistic, but dismissible is the default value so we could just omit this.

shiftkey added this to the 1.7.0 milestone Feb 27, 2019

This was referenced Feb 27, 2019

[rebase] show progress of an in-flight rebase #6962

Closed

"Rebase current branch" dialog and flow #6551

Closed

shiftkey force-pushed the shiftkey/rebase/menu-item-and-dialog branch 2 times, most recently from 585b588 to 3684e0e Compare February 27, 2019 20:15

shiftkey changed the title ~~menu item and dialog to start rebase operation~~ [rebase] menu item and dialog to start rebase operation Feb 28, 2019

shiftkey marked this pull request as ready for review February 28, 2019 14:09

shiftkey added the ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 28, 2019

shiftkey commented Feb 28, 2019

View reviewed changes

shiftkey mentioned this pull request Feb 28, 2019

[rebase] enable --force-with-lease after rebasing #6981

Merged

5 tasks

shiftkey commented Mar 1, 2019

View reviewed changes

shiftkey mentioned this pull request Mar 1, 2019

Provide a better error message and give users the option to retry a failed clone #926

Closed

shiftkey mentioned this pull request Mar 5, 2019

Update Rebasing terms and UI #7012

Closed

shiftkey force-pushed the shiftkey/rebase/menu-item-and-dialog branch from f113389 to 29b5d2e Compare March 5, 2019 21:54

shiftkey mentioned this pull request Mar 6, 2019

audit menu items to determine which should be enabled when dealing with conflicts #7017

Closed

shiftkey added the time-sensitive Pull Requests where reviews need to happen in a timely manner label Mar 6, 2019

shiftkey added 7 commits March 7, 2019 09:36

add feature flag for new rebase dialog

4c4e774

make rebase return more context about the success/failure

39a15cd

rename ContinueRebaseResult to RebaseResult

e92d548

add menu item to show rebase branch dialog

e4c9c10

reorganise styles to better match components

f70474d

add metric to show when user launches rebase branch dialog

87f3293

add popup to represent rebase branch dialog

a991af0

shiftkey added 7 commits March 7, 2019 09:36

first pass at a dialog to select and start the rebase

678cf44

wireup the menu item to show the dialog

59dddae

bump to latest dugite to get new error code

d5e4e0d

switch over to use error codes instead of parsing Git output

6b57d10

use E for shortcut because R is used for rename

fa24d0c

clarified this word

ee15d85

better formatting for this tracing message

09628d1

shiftkey force-pushed the shiftkey/rebase/menu-item-and-dialog branch from 25bdc88 to 09628d1 Compare March 7, 2019 13:36

niik self-assigned this Mar 8, 2019

niik requested changes Mar 8, 2019

View reviewed changes

niik and others added 9 commits March 8, 2019 11:25

Apply suggestions from code review

b513700

Co-Authored-By: shiftkey <brendan@github.com>

📝 rebase result codes

7e6c785

use dialog props instead of custom code

9809329

tidy up current branch name usage

de796e2

remove unused component

abca195

extract function for resolving selected branch

d3f2dbd

remove unused style

ac9ddb4

allow Dialog to accept a JSX element for the title

6cf4a2a

cleanup duplicated dialog header

7f4cedd

This was referenced Mar 8, 2019

backport fixes from rebase conflicts dialog review to merge conflicts #7043

Closed

add support for setting JSX for title on Dialog props #6032

Closed

shiftkey added 2 commits March 8, 2019 13:18

also ensure this is dismissable

f951f62

better context for this error code

1e48eaa

shiftkey requested a review from niik March 8, 2019 17:31

shiftkey removed the time-sensitive Pull Requests where reviews need to happen in a timely manner label Mar 8, 2019

niik approved these changes Mar 11, 2019

View reviewed changes

niik merged commit ad2f25f into development Mar 11, 2019

shiftkey deleted the shiftkey/rebase/menu-item-and-dialog branch March 11, 2019 15:37

shiftkey mentioned this pull request Jun 13, 2019

add rebaseCurrentBranchMenuCount metric to list #7793

Merged


		log.info(`[rebase] starting rebase for ${beforeSha}`)

		// TODO: this can happen very quickly for a trivial rebase or an OS with

Conversation

shiftkey commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Description

Release notes

Uh oh!

shiftkey Feb 28, 2019

Choose a reason for hiding this comment

Uh oh!

shiftkey Feb 28, 2019

Choose a reason for hiding this comment

Uh oh!

shiftkey Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

tierninho commented Mar 5, 2019

Uh oh!

shiftkey commented Mar 5, 2019

Uh oh!

tierninho commented Mar 5, 2019

Uh oh!

shiftkey commented Mar 5, 2019

Uh oh!

shiftkey commented Mar 6, 2019

Uh oh!

shiftkey commented Mar 6, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shiftkey commented Mar 8, 2019

Uh oh!

niik Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

niik Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shiftkey commented Feb 27, 2019 •

edited

Loading