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

Allow amending the last commit #12353

Merged
merged 12 commits into from
Jun 4, 2021
Merged

Allow amending the last commit #12353

merged 12 commits into from
Jun 4, 2021

Conversation

sergiou87
Copy link
Member

Closes #1644

Description

This PR introduces the ability to amend the last commit in your branch (if it's not pushed) by right-clicking it in the History tab and selecting the new Amend Commit… action.

This will switch Desktop to a "amending" state, where all selected changes and the new summary and description will be applied to the last commit.

Screenshots

amend-last-commit.mov

Release notes

Notes: [New] Amend the last local commit in your branch

Comment on lines +49 to +66
const newState = merge(currentState, newValues)

const isSameLastLocalCommit =
currentState.localCommitSHAs.length > 0 &&
newState.localCommitSHAs.length > 0 &&
currentState.localCommitSHAs[0] === newState.localCommitSHAs[0]

// Only keep the "is amending" state if the last local commit hasn't changed
// and there is no "fixing conflicts" state.
const newIsAmending =
newState.isAmending &&
isSameLastLocalCommit &&
newState.changesState.conflictState === null

this.repositoryState.set(repository.hash, {
...newState,
isAmending: newIsAmending,
})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you check my previous approach, I intended to stop amending manually whenever the user started any operation that could lead to a state where the user would amend the wrong commit (for example, you switch to a completely different branch… the amend state should be cleared… and same for pulling, pushing, merging, cherry-picking, squashing…).

After thinking a bit about it, I decided to go with this other centralized and (more) future-proof approach, where the amending state will be cleared whenever:

  1. The last local commit sha changes
  2. The app is in a "fix conflicts" state

This should cover all our present needs, at least!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

</>
) : (
commitVerb
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion. I found this hard to read.. maybe worth separating into two variables. commitDefaultContents and amendDefaultContents and then defaultContents = isAmending ? amendDefaultContents : commitDefaultContents

const commitVerb = loading ? 'Committing' : 'Commit'

const isAmending = this.props.commitToAmend !== null
const commitVerb = isAmending
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one isn't as hard to follow but.. maybe still clearer if broken into a variable for each type like below.

Copy link
Contributor

@tidy-dev tidy-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great and I pulled it down and tested it out. It is awesome!

I just a small suggestion on the double tertiary's. The first one not so much, but the second one feels really hard to process when reading it.

@sergiou87 sergiou87 requested a review from tidy-dev June 3, 2021 14:25
@sergiou87
Copy link
Member Author

Addressed both issues, thank you! ❤️

tidy-dev
tidy-dev previously approved these changes Jun 3, 2021
Copy link
Contributor

@tidy-dev tidy-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨ Thank you for those changes!

Copy link
Contributor

@ampinsk ampinsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks great! We chatted in Slack about updating the text above the commit button to:

Your changes will modify your most recent commit. Stop amending to make these changes as a new commit.

sergiou87 and others added 2 commits June 4, 2021 09:48
Co-authored-by: Billy Griffin <5091167+billygriffin@users.noreply.github.com>
Base automatically changed from undo-context-menu to development June 4, 2021 14:33
Copy link
Contributor

@tidy-dev tidy-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨ This is so nice! Looking forward to using this feature. I reviewed recent commits and pulled down and retested. LGTM!

@sergiou87 sergiou87 merged commit 1728411 into development Jun 4, 2021
@sergiou87 sergiou87 deleted the amend-commits branch June 4, 2021 14:57
@MichaIng
Copy link

the ability to amend the last commit in your branch (if it's not pushed)

What is the reason for not allowing to amend to pushed commits? I regularly run into issues with GitHub desktop where it doesn't allow things for none obvious reasons which work fine with git. Sometimes we just want it to do what we want, and as long as it does not allow steps that can be done fine with git, it's very difficult for users to switch, running into a bunch of unexpected difficulties 🤔.

@sergiou87
Copy link
Member Author

We didn't want to create this kind of "force-push" situations very often, but we've talked about it and we'll allow to amend pushed commits 😄

Thank you for your feedback @MichaIng !!

@MichaIng
Copy link

Many thanks 👍!

At least I use this quite often to squash or repair commits in a PR without bloating the commit history. Currently I am forced to use git CLI for that, and sadly on an additional Linux system since git for Windows has a bunch of issues related to Windows 10 security/sandboxing features. Much easier to do everything from the same desktop system where I run notepad++ and browse GitHub 🙂.

@sergiou87
Copy link
Member Author

@MichaIng hopefully will get merged soon: #13384 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: Amend last commit
5 participants