rename stash #2220
rename stash #2220
Conversation
Ryooooooga force-pushed the rename-stash branch 2 times, most recently from 1dda24b to bcf8e9f Compare Ryooooooga force-pushed the rename-stash branch from bcf8e9f to 451115f Compare 
jesseduffield left a comment
jesseduffield left a comment There was a problem hiding this comment.
Looking good :) Can we add an integration test as well?
| InitialContent: stashEntry.Name, | ||
| HandleConfirm: func(response string) error { | ||
| self.c.LogAction(self.c.Tr.Actions.RenameStash) | ||
| output, err := self.git.Stash.Drop(stashEntry.Index) |
There was a problem hiding this comment.
I would move this logic of this HandleConfirm callback into pkg/commands/git_commands/stash.go because it's quite specific to git. The LogAction and Refresh calls can stay here
There was a problem hiding this comment.
Moved to stash.go.
| stashShaPattern := regexp.MustCompile(`\(([0-9a-f]+)\)`) | ||
| matches := stashShaPattern.FindStringSubmatch(output) | ||
| stashSha := "" | ||
| if len(matches) > 1 { |
There was a problem hiding this comment.
in what situation would we not find a match? Worth leaving a comment explaining that
There was a problem hiding this comment.
Normally, this regex is always matched, but added error handling
Ryooooooga force-pushed the rename-stash branch 2 times, most recently from ff3b460 to f2c11c5 Compare 
pkg/commands/git_commands/stash.go Outdated
| stashShaPattern := regexp.MustCompile(`\(([0-9a-f]+)\)`) | ||
| matches := stashShaPattern.FindStringSubmatch(output) | ||
| if len(matches) <= 1 { | ||
| return errors.New("Output of `git stash drop` is invalid") // Usually this error does not occur |
There was a problem hiding this comment.
not internationalized because this error never occurs
Ryooooooga force-pushed the rename-stash branch from d465de5 to a6fe332 Compare 
| added an integration test |
pkg/commands/git_commands/stash.go Outdated
| | ||
| // `output` is in the following format: | ||
| // Dropped refs/stash@{0} (f0d0f20f2f61ffd6d6bfe0752deffa38845a3edd) | ||
| stashShaPattern := regexp.MustCompile(`\(([0-9a-f]+)\)`) |
There was a problem hiding this comment.
Just tested this out locally, and it works, but I think we should change the approach a bit. You can use git rev-parse stash@{<index>} to obtain the hash, and I think we ought to do that for a couple of reasons:
git rev-parse's output is never going to change, but I can imagine the output ofgit stash dropchanging. Ifgit stash dropdoes change its output, we'll have just lost the user's stash which is going to make them very mad.- it spares us the complexity of regexes
So I propose we run three commands:
git rev-parse stash@{<index>}to get the hashgit stash drop stash@{<index>}to drop itgit stash store <hash>to store the renamed stash entry
There was a problem hiding this comment.
changed that way:)
| . "github.com/jesseduffield/lazygit/pkg/integration/components" | ||
| ) | ||
| | ||
| var Rename = NewIntegrationTest(NewIntegrationTestArgs{ |
Co-authored-by: Jesse Duffield <jessedduffield@gmail.com>
Ryooooooga force-pushed the rename-stash branch from a6fe332 to 3af2686 Compare Ryooooooga force-pushed the rename-stash branch from 3af2686 to 3bf2e8f Compare Ryooooooga force-pushed the rename-stash branch from 3bf2e8f to 3103398 Compare | shell. | ||
| EmptyCommit("blah"). | ||
| CreateFileAndAdd("foo", "change to stash"). | ||
| StashWithMessage("bar") |
There was a problem hiding this comment.
sorry for missing this the first time but could we add another stash entry beforehand so that when we assert that the text has been updated after renaming the stash, we're also implicitly verifying that the cursor has remained on the stash entry?
jesseduffield left a comment
jesseduffield left a comment There was a problem hiding this comment.
only one more thing I promise ;)
| updated |
| Awesome work @Ryooooooga ! |
2 participants
#2189
same as:
go run scripts/cheatsheet/main.go generate)docs/Config.md) have been updated if necessary