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

Undo last commit and amend context menu option #1364

Merged
merged 53 commits into from Apr 3, 2018

Conversation

Projects
None yet
6 participants
@kuychaco
Member

kuychaco commented Mar 24, 2018

This PR will introduce an "Undo" button that appears only on the most recent commit. Clicking it will do a soft reset to the parent of HEAD, which will restore the staged changes to the state prior to the last commit. The commit message box will be populated with the message of the undone commit.

The amend checkbox will be replaced with a context-menu option that appears only on the most recent commit. Selecting this will run git commit --amend. If the commit message box is empty, the previous commit message will be used, otherwise the new commit will have the message from the box.

Undo

Note that the commit message and co-author fields are repopulated, and changes from the commit appear back on the stage

undo2

Amend

Note that you can amend the last commit with any combination of the following:

  • a new commit message
  • additional staged changes
  • addition or removal of co-author(s)

amend1

TODO:

  • add tests for amend.
    • that staged changes disappear and are incorporated into the most recent commit.
    • if new commit message is provided, use that. otherwise default to message from previous commit.
    • if no commits exist are we handling that edge case? (might be covered by another test)
  • expanded commit box manual testing
  • remove checkbox from ui
  • rip out current amend state logic
  • fix any tests that fail as a result of that.
  • get rid of "regular" commit message verbage
  • fix bug amending with coauthors (see gif)
  • handle case when user simply wants to amend message
  • when commiting with expanded editor and co authors, ensure new lines separate co author trailers.
  • make toggle co author icon clickable when commit message is expanded
  • move tests for addTrailersToCommitMessage to GSOS tests

Write test cases for commitIsEnabled with and without amend flag

  • staged changes only
  • commit message only
  • coauthors only
  • combinations of the above
  • noop if none of the above
@smashwilson

This comment has been minimized.

Member

smashwilson commented Mar 24, 2018

tenor

@BinaryMuse

This comment has been minimized.

Member

BinaryMuse commented Mar 24, 2018

Finally! 😉

@smashwilson

This comment has been minimized.

Member

smashwilson commented Mar 24, 2018

Wait. I thought I fixed this one...

Git commands for CompositeGitStrategy made of [GitShellOutStrategy] ssh authentication fails the command on dialog cancel - ssh authentication

ಠ_ಠ

@smashwilson

This comment has been minimized.

Member

smashwilson commented Mar 24, 2018

... Oh, wait. #1355 hasn't been merged yet! Never mind me.

@kuychaco kuychaco changed the base branch from ku-niik/co-authors to master Mar 27, 2018

kuychaco and others added some commits Mar 22, 2018

Add 'Undo' button on most recent commit and 'amend' context menu item
Co-authored-by: Tilde Ann Thurium <annthurium@github.com>
Add GSOS#reset method which does a soft reset to the parent of head
Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
Implement undo last commit functionality. Restore message in commit box
Co-authored-by: Tilde Ann Thurium <annthurium@github.com>
Extract `extractCoAuthorsAndRawCommitMessage` method & LINE_ENDING_REGEX
Co-authored-by: Tilde Ann Thurium <annthurium@github.com>
Pull up selectedCoAuthors state to GitTabController
Co-authored-by: Tilde Ann Thurium <annthurium@github.com>
Test that co author state populates after undoing last commit
Co-authored-by: Tilde Ann Thurium <annthurium@github.com>
Initialize state of co authors to empty array
Co-authored-by: Tilde Ann Thurium <annthurium@github.com>
Implement `github:amend-last-commit` command
Co-authored-by: Tilde Ann Thurium <annthurium@github.com>
Fix broken CommitView editor focus test
Co-Authored-By: Katrina Uychaco <kuychaco@github.com>
Add tests for amending with and without a new commit message
Co-authored-by: Katrina Uychaco <kuychaco@github.com>
Remove "amend" checkbox.
Co-authored-by: Katrina Uychaco <kuychaco@github.com>
Rip out plumbing for amend checkbox.
Co-authored-by: Katrina Uychaco <kuychaco@github.com>
Oops co-author trailers don't end with "
Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
Extract CO_AUTHOR_REGEX to helper file
Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
Extract author name in `extractCoAuthorsAndRawCommitMessage`
... in addition to email. So that when we undo the last commit we have
all of the co-author information in order to correctly populate the
selected co authors list. Previously we were getting `undefined` listed
as a co-author.

Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
Fix tests
Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
Fix prop type errors
Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
Make tests stop complaining about PropType mismatches and missing req…
…uired props.

Co-authored-by: Katrina Uychaco <kuychaco@github.com>
Extract `isCommitMessageEditorExpanded` method for readability.
Co-authored-by: Katrina Uychaco <kuychaco@github.com>

simurai and others added some commits Mar 30, 2018

refactor git-tab-controller tests to add beforeEach for amend methods.
Co-authored-by: Katrina Uychaco <kuychaco@github.com>
Refactor gsos#getHeadCommit to use gsos#getRecentCommits
This reduces redundancy and prevents us from parsing the co author
trailer twice.  It also provides more information when you're asking for
the last commit (such as co authors.)

Co-authored-by: Katrina Uychaco <kuychaco@github.com>
WIP - tests for git-tab-controller
Co-authored-by: Katrina Uychaco <kuychaco@github.com>
🔥 unnecessary test
Rather than reading the file at ATOM_COMMIT_MSG_EDIT, we pull the text
from the expanded commit message editor
Oops, we need a `this` when we call `isCommitMessageEditorExpanded`
Co-authored-by: Tilde Ann Thurium <annthurium@github.com>
WIP fix tests
Co-authored-by: Tilde Ann Thurium <annthurium@github.com>
Simplify test setup for successfully removing a co-author via amend
Co-authored-by: Katrina Uychaco <kuychaco@github.com>
Add `includeUnborn` option to GSOS#getCommits
This is to fix an issue where we were rending an unborn commit in the
RecentCommitsView

Co-authored-by: Katrina Uychaco <kuychaco@github.com>
Move merge co author trailer tests into git-strategies.
Co-authored-by: Katrina Uychaco <kuychaco@github.com>

@kuychaco kuychaco requested a review from smashwilson Mar 31, 2018

@smashwilson

This PR makes me happy 😁 So many awkward bits streamlined here.

:shipit: :shipit: :shipit:

} else {
this.setRegularCommitMessage(await fs.readFile(this.getCommitMessagePath(), {encoding: 'utf8'}));
}
this.setCommitMessage(await fs.readFile(this.getCommitMessagePath(), {encoding: 'utf8'}));

This comment has been minimized.

@smashwilson

smashwilson Apr 3, 2018

Member

so much nicer.

@autobind
isCommitMessageEditorExpanded() {
return this.getCommitMessageEditors().length > 0;
}

This comment has been minimized.

@smashwilson

smashwilson Apr 3, 2018

Member

I'm not sure if this actually needs @autobind, because it's called to create a prop rather than being passed directly as one. It doesn't hurt anything though 😄

This comment has been minimized.

@kuychaco

kuychaco Apr 3, 2018

Member

yup, good catch 👍

if (typeof rawMessage !== 'string') {
throw new Error(`Invalid message type ${typeof message}. Must be string.`);
}

This comment has been minimized.

@smashwilson

smashwilson Apr 3, 2018

Member

This bit is a little odd to me. I'm guessing this is where you were wanting TypeScript because some caller was passing in an object and breaking stuff? Can we get rid of this now that you've tracked it down?

This comment has been minimized.

@kuychaco

kuychaco Apr 3, 2018

Member

yup, we can drop this. We used to accept an object that had a filePath key that pointed to the ATOM_COMMIT_EDIT_MSG file and we'd pass that as an option to git commit. But now since we're doing some commit message processing to add co-author trailers we're reading the message from the editor prior to committing, rather than the filesystem at commit time. All that to say, this check is less meaningful now that we always expect a string.

fileExists, isFileExecutable, isFileSymlink, isBinary,
normalizeGitHelperPath, toNativePathSep, toGitPathSep,
extractCoAuthorsAndRawCommitMessage, fileExists, isFileExecutable, isFileSymlink, isBinary,
normalizeGitHelperPath, toNativePathSep, toGitPathSep, LINE_ENDING_REGEX, CO_AUTHOR_REGEX,

This comment has been minimized.

@smashwilson

smashwilson Apr 3, 2018

Member

👍 for putting regexes into the regex pile.

async getRecentCommits(options = {}) {
const {max, ref} = {max: 1, ref: 'HEAD', ...options};
async getCommits(options = {}) {
const {max, ref, includeUnborn} = {max: 1, ref: 'HEAD', includeUnborn: false, ...options};

This comment has been minimized.

@smashwilson

smashwilson Apr 3, 2018

Member

Yah, a better name.

* Undo Operations
*/
reset(type, revision = 'HEAD') {
const validTypes = ['soft'];

This comment has been minimized.

@smashwilson

smashwilson Apr 3, 2018

Member

No 'hard' or 'mixed' yet? 😄

This comment has been minimized.

@kuychaco

kuychaco Apr 3, 2018

Member

heh nope, not yet. @annthurium and I (and maybe @simurai?) talked about it a bit and decided to hold off on those until we have more thorough discussion about UX and other implications. For example, if we implement a way to do a hard reset then we probably want to add some safeguards so that users don't accidentally blow away workdir changes in a way that is unrecoverable. And if someone accidentally does a mixed reset after carefully staging changes, then we've just blown away their index state and we'd probably want to offer a way for them to get back to the state they were in before the reset.

All of this to say, I'd like to have more developed thoughts on the recovery story if users accidentally do one of these operations. Or alternatively, decide on a UX that makes it really hard for users to accidentally or incorrectly do one of these operations. What do you think @smashwilson 💭

@kuychaco

This comment has been minimized.

Member

kuychaco commented Apr 3, 2018

@smashwilson thanks for the review! I'll go ahead and merge this as-is and incorporate changes based on your comments in #1374 so that we don't have to wait for CI to turn green

@kuychaco kuychaco merged commit 9377784 into master Apr 3, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@marcus-blaettermann-ventoro

This comment has been minimized.

marcus-blaettermann-ventoro commented Apr 20, 2018

This PR breaks a workflow. Previously it was possible to see the changes in the last commit by clicking the amend checkbox. Also it was possible to see what the new commit would look like before amending.

Now if I want to do that, I have to undo the last commit so it goes to the staging area. But if it turns no amend was necessary the last commit is already destroyed. This especially severe when doing interactive rebase because this could lead to an unnecessary rewrite of the complete history even when not amending anything.

It should at least be possible to undo the undo, so I can get the old commit back.

Also I find it far less intuitive to hide the amend feature in a context menu.

@smashwilson smashwilson referenced this pull request Apr 23, 2018

Merged

Recent commit changes RFC #1318

3 of 6 tasks complete

@smashwilson smashwilson deleted the ku-tt-undo-last-commit branch Sep 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment