Add action for amending a commit #283

kristijanhusak · 2018-09-12T22:53:21Z

Add M shortcut to files panel that allows amending last commit.

Restore old file merging algorithm

golangcibot · 2018-09-12T22:53:33Z

pkg/gui/files_panel.go

 	commitMessageView := gui.getCommitMessageView(g)
+	if commitMessageView.Title == gui.Tr.SLocalize("AmendLastCommit") {
+		commitMessageView.Clear()
+		commitMessageView.SetCursor(0, 0)


Error return value of commitMessageView.SetCursor is not checked

golangcibot · 2018-09-12T22:53:33Z

pkg/gui/files_panel.go

+
+	commitMessageView.Title = gui.Tr.SLocalize("CommitMessage")
+	g.Update(func(g *gocui.Gui) error {
+		g.SetViewOnTop("commitMessage")


Error return value of g.SetViewOnTop is not checked

golangcibot · 2018-09-12T22:53:33Z

pkg/gui/files_panel.go

+	commitMessageView.Title = gui.Tr.SLocalize("CommitMessage")
+	g.Update(func(g *gocui.Gui) error {
+		g.SetViewOnTop("commitMessage")
+		gui.switchFocus(g, filesView, commitMessageView)


Error return value of gui.switchFocus is not checked

golangcibot · 2018-09-12T22:53:33Z

pkg/gui/files_panel.go

 	g.Update(func(g *gocui.Gui) error {
 		g.SetViewOnTop("commitMessage")
 		gui.switchFocus(g, filesView, commitMessageView)
+		lastCommitName := gui.State.Commits[0].Name
+		commitMessageView.Write([]byte(lastCommitName))


Error return value of commitMessageView.Write is not checked

golangcibot · 2018-09-12T22:53:33Z

pkg/gui/files_panel.go

 	g.Update(func(g *gocui.Gui) error {
 		g.SetViewOnTop("commitMessage")
 		gui.switchFocus(g, filesView, commitMessageView)
+		lastCommitName := gui.State.Commits[0].Name
+		commitMessageView.Write([]byte(lastCommitName))
+		commitMessageView.SetCursor(len(lastCommitName), 0)


Error return value of commitMessageView.SetCursor is not checked

jesseduffield · 2018-09-13T06:55:27Z

Thanks for making this :) There is currently a Gui rework PR up #276 so I'm gonna wait until that's merged (hopefully soon) before giving this a review

pkg/i18n/english.go

pkg/gui/files_panel.go

jesseduffield · 2018-09-23T04:16:36Z

@kristijanhusak the PR I mentioned earlier has been put off until its creator has some more time on his hands. Let me know when this PR is ready for review :) (I'm looking forward to using it myself!)

kristijanhusak · 2018-09-23T08:23:36Z

@jesseduffield it should be ready. I didn't fix these bot ci errors because most of that code is copied.

jesseduffield · 2018-09-25T09:12:58Z

I like the fact that we're bringing up the commit message panel so you can change the name of the commit if you wish, but this presents a design inconsistency. Currently we support entering a commit message directly in the commit message panel, or via an editor like vim. the 'c' keybinding does the former, and the 'shift+c' keybinding does the latter. The same is true of renaming a commit i.e. r vs shift+r.

So here, if we want to support renaming the commit as you amend it, we'll need to support renaming it via the editor as well.

For this reason, I think we should just amend the commit without changing the commit message. If the user then decides they also want to change the commit message, they can do so in the commits panel using r/shift+r.

Instead I think we should have a confirmation popup saying 'are you sure you want to amend this commit' and upon hitting enter it runs git commit --amend

What are your thoughts on this approach @kristijanhusak ?

kristijanhusak · 2018-09-25T09:29:27Z

@jesseduffield I think we should allow amending commit also via $EDITOR in that case. That way we could leave the commit message panel as it is now, and just add a keybinding that will allow amending via editor. Only issue is that m keybinding is already taken, so we cannot go key/shift+key route with it. Any suggestions for a different keybinding?

jesseduffield · 2018-09-25T10:14:49Z

I'm not sure. Do you think there is a genuine use case for having two keybindings here though? I imagine that most people using git commit --amend don't need to change the commit message, and if they do, they can currently do that after the fact with r/shift+r in the commit panel

kristijanhusak · 2018-09-25T10:32:52Z

I guess you're right. Most of the time i do not change it, but it looked good to have an option to do that easily. I didn't even figure out by myself that there's an option to rename commit from commits panel, so maybe the amend confirmation popup should contain some short instructions that will let user know that they can rename the commit from the commits panel, something like "Are you sure you want to amend this commit? You can change commit message later from commits panel."

jesseduffield · 2018-09-25T10:48:13Z

That sounds good to me :)

kristijanhusak · 2018-09-25T11:45:48Z

Ok great, i'll update PR in the following days when i catch some time.

…feature/commit-amend

golangcibot · 2018-09-25T20:12:13Z

pkg/gui/files_panel.go

+		lastCommitMsg := gui.State.Commits[0].Name
+		_, err := gui.GitCommand.Commit(lastCommitMsg, true)
+		if err != nil {
+			gui.createErrorPanel(g, err.Error())


Error return value of gui.createErrorPanel is not checked

I know this might be copy+pasted but to appease golangci could we wrap this line an error check:

if err := gui.createErrorPanel(g, err.Error()); err != nil { return err }

kristijanhusak · 2018-09-25T20:15:28Z

@jesseduffield I updated it according to our agreement. If you have better suggestion for the confirmation text, let me know.
Also, for polish and dutch, I used google translate, so i'm not sure if translation is good enough.

Edit: Also, should this introduce a force push option? Amended commits cannot be regularly pushed (using shift + p).

jesseduffield · 2018-09-25T23:21:34Z

Currently if there are commits to pull, and you hit shift+p then it'll ask you whether you want to force push. After we do a git commit --amend it should then give you that prompt when you try to push. If not we might need to refresh something

jesseduffield · 2018-09-25T23:23:34Z

@mjarkk I forget what the procedure is for translations. What should a contributor do if they are adding new strings to the english.go file?

kristijanhusak · 2018-09-26T19:34:53Z

@jesseduffield code that checks if force push is required does not work for me, at least not in all cases.
I have my neovim config repo, and a branch named defx on it. I tried amending and force pushing, and it tried regular push without asking.
After that i tried running git commands that lazygit uses to determine this, and i get error from git:

fatal: no upstream configured for branch 'defx'

for command:

git rev-list @{u}..head --count

I know i could fix this by setting up an upstream for a branch, but even without setting up an upstream, i can properly commit/push/force push to that branch manually.
Is there something wrong with my git, or lazygit needs to do this check in a different way?

Also in my global .gitconfig, i have this added (Not sure if relevant):

[push]
	default = current

jesseduffield · 2018-10-04T07:33:26Z

Sorry for the late reply, I've been swamped by other stuff recently and have had to take a minor hiatus from the repo.

Pushing via lazygit should set the upstream branch to origin/branchname, so that should resolve the issue with git rev-list @{u}..head --count, and then force pushing should work after that. Does that resolve your issue?

kristijanhusak · 2018-10-04T08:57:35Z

@jesseduffield no worries.

I just tried it, and unfortunately it doesn't work.
I commited and pushed through lazygit, and after that tried an amend that same commit, it failed.

jesseduffield · 2018-10-04T22:28:14Z

I'll pull down your branch and see if I get the same issue

jesseduffield · 2018-10-04T22:41:00Z

Ah so the issue is that the status bar isn't being refreshed, meaning we're not getting an update on whether the pull/push count has changed, and that's what the pull method looks at to decide whether it needs to force push. It's not really good to have the app's state and UI coupled in this way but I'm happy to leave improvements on that to a refactor PR.

This patch should fix the issue:

diff --git a/pkg/gui/files_panel.go b/pkg/gui/files_panel.go
index d76ba57..f478e63 100644
--- a/pkg/gui/files_panel.go
+++ b/pkg/gui/files_panel.go
@@ -223,8 +223,7 @@ func (gui *Gui) handleAmendCommitPress(g *gocui.Gui, filesView *gocui.View) erro
                if err != nil {
                        gui.createErrorPanel(g, err.Error())
                }
-
-               return gui.refreshFiles(g)
+               return gui.refreshSidePanels(g)
        }, nil)
 }

kristijanhusak · 2018-10-05T09:00:20Z

Thanks, i'll fix it today or during the weekend.

kristijanhusak · 2018-10-06T05:46:06Z

@jesseduffield i tried it, it still didn't work for me. Then i updated all head parts to HEAD in git commands, and it worked. Not sure how lowercase head worked for anyone. It's working as intended now, no need to refresh all side panels.

jesseduffield · 2018-10-06T07:36:16Z

Ah cool, git is case insensitive when it comes to HEAD on osx so that would be it. I would still refresh the status panel though because after the amend, the pushable/pullable count should be updated

…equires it.

kristijanhusak · 2018-10-06T07:54:56Z

Makes sense. Updated it.

jesseduffield · 2018-10-07T21:25:06Z

pkg/commands/git.go

+func (c *GitCommand) Commit(message string, amend bool) (*exec.Cmd, error) {
+	amendParam := ""
+	if amend {
+		amendParam = "--amend "


nitpick but if you're gonna use fmt.Sprintf we should add that trailing space character to the format string i.e. "git commit %s -m %s" and then say amendParam = "--amend"

@jesseduffield i did this, but i reverted it back with a space before --. Lot's of tests fail because moving space to Sprintf causes two spaces between git commit and -m if there's no amend.

jesseduffield · 2018-10-07T21:29:48Z

pkg/gui/files_panel.go

+	title := strings.Title(gui.Tr.SLocalize("AmendLastCommit"))
+	question := gui.Tr.SLocalize("SureToAmend")
+	return gui.createConfirmationPanel(g, filesView, title, question, func(g *gocui.Gui, v *gocui.View) error {
+		lastCommitMsg := gui.State.Commits[0].Name


it is possible that we have just done a git init so there are no commits yet. I would chuck a check for that here:

if len(gui.State.Commits) == 0 { return errors.New(gui.Tr.SLocalize("NoCommitToAmend")) }

Should i do it before the confirmation panel, or inside of it?

jesseduffield · 2018-10-07T21:34:10Z

pkg/gui/keybindings.go

@@ -125,6 +125,12 @@ func (gui *Gui) GetKeybindings() []*Binding {
 			Modifier:    gocui.ModNone,
 			Handler:     gui.handleCommitPress,
 			Description: gui.Tr.SLocalize("CommitChanges"),
+		}, {
+			ViewName:    "files",
+			Key:         'M',


I see that the 'A' keybinding is used to abort merges, but this seems like an operation that wouldn't be used often. What are your thoughts on using shift+M for abort merge and shift+A for amend commit?

Yeah that makes sense. I'll switch it.

jesseduffield

Awesome work! I left some comments. Also tests are failing on CI

…hortcut.

codecov-io · 2018-10-08T20:23:15Z

Codecov Report

Merging #283 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #283     +/-   ##
=========================================
+ Coverage   93.97%   94.08%   +0.1%     
=========================================
  Files          12       12             
  Lines        1661     1691     +30     
=========================================
+ Hits         1561     1591     +30     
  Misses         98       98             
  Partials        2        2

Impacted Files	Coverage Δ
pkg/i18n/english.go	`100% <100%> (ø)`	⬆️
pkg/commands/git.go	`92.49% <100%> (+0.07%)`	⬆️
pkg/i18n/dutch.go	`100% <100%> (ø)`	⬆️
pkg/i18n/polish.go	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8b59ba...4287f8a. Read the comment docs.

kristijanhusak · 2018-10-08T20:25:11Z

@jesseduffield I fixed most of the things. I left comments where i wasn't able to fix it according to your suggestion.

jesseduffield · 2018-10-10T06:51:16Z

Fantastic work man, I'm gonna be making use of this feature myself! Thanks!

Merge pull request #279 from jesseduffield/hotfix/file-ordering

7fb2caf

Restore old file merging algorithm

golangcibot reviewed Sep 12, 2018

View reviewed changes

dawidd6 reviewed Sep 13, 2018

View reviewed changes

pkg/i18n/english.go Outdated Show resolved Hide resolved

dawidd6 reviewed Sep 13, 2018

View reviewed changes

pkg/gui/files_panel.go Outdated Show resolved Hide resolved

Add ammend commit action.

61f0801

Merge branch 'master' into feature/commit-amend

7164f37

kristijanhusak added 2 commits September 25, 2018 21:17

Merge branch 'master' of git://github.com/jesseduffield/lazygit into …

b6b21bc

…feature/commit-amend

Use confirmation popup for amending last commit.

28fe3d6

golangcibot reviewed Sep 25, 2018

View reviewed changes

Refresh side panels and use uppercase HEAD in all git commands that r…

5f7ac97

…equires it.

Merge branch 'master' into feature/commit-amend

ac65586

jesseduffield reviewed Oct 7, 2018

View reviewed changes

jesseduffield requested changes Oct 7, 2018

View reviewed changes

kristijanhusak added 2 commits October 8, 2018 21:19

Check if there is any commit to amend and use 'A' instead of 'M' as s…

190309e

…hortcut.

Fix tests and add test scenarios for amend.

4287f8a

jesseduffield merged commit d5f6460 into jesseduffield:master Oct 10, 2018

Add action for amending a commit #283

Add action for amending a commit #283

Conversation

kristijanhusak commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesseduffield commented Sep 13, 2018

jesseduffield commented Sep 23, 2018

kristijanhusak commented Sep 23, 2018

jesseduffield commented Sep 25, 2018

kristijanhusak commented Sep 25, 2018

jesseduffield commented Sep 25, 2018

kristijanhusak commented Sep 25, 2018

jesseduffield commented Sep 25, 2018

kristijanhusak commented Sep 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristijanhusak commented Sep 25, 2018 • edited Loading

jesseduffield commented Sep 25, 2018 • edited Loading

jesseduffield commented Sep 25, 2018

kristijanhusak commented Sep 26, 2018

jesseduffield commented Oct 4, 2018

kristijanhusak commented Oct 4, 2018

jesseduffield commented Oct 4, 2018

jesseduffield commented Oct 4, 2018 • edited Loading

kristijanhusak commented Oct 5, 2018

kristijanhusak commented Oct 6, 2018

jesseduffield commented Oct 6, 2018

kristijanhusak commented Oct 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesseduffield left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 8, 2018 • edited Loading

Codecov Report

kristijanhusak commented Oct 8, 2018

jesseduffield commented Oct 10, 2018

kristijanhusak commented Sep 25, 2018 •

edited

Loading

jesseduffield commented Sep 25, 2018 •

edited

Loading

jesseduffield commented Oct 4, 2018 •

edited

Loading

codecov-io commented Oct 8, 2018 •

edited

Loading