feat: add stash option to include untracked files #1980

ajhynes7 · 2022-06-03T19:42:32Z

This PR adds an option to stash all changes including untracked files.

Relevant issue: #1974

Unfortunately the integration test is failing for me locally with this error:

Diff:
--- Expected
+++ Actual
@@ -19,3 +19,3 @@
git stash list:
-stash@{0}: WIP on master: 592d439 Initial commit
+stash@{0}: WIP on master: 393c632 Initial commit

But I don't know how to fix that, so I figured I'd commit it anyways and get some feedback from the reviewers.

Screenshot

mark2185 · 2022-06-03T19:45:26Z

The only thing I'd suggest is to move the option below all the other "stash all changes" options, i.e. the third row.

ajhynes7 · 2022-06-10T23:06:34Z

Do you know how to fix the integration test failure I posted above?

mark2185 · 2022-06-13T20:40:07Z

Ah, I hadn't noticed that part. I'll take a look at it first thing in the morning and get back to you.

Unless you've had some luck in the meantime? :)

ajhynes7 · 2022-06-14T00:58:35Z

Sorry haha I haven't looked at this since.

mark2185 · 2022-06-21T09:56:18Z

Said morning had a lot of first things

In any case, I noticed that the given stash message is never actually used, it always ends up as WIP on <branch>.
~~I presume some if goes the wrong way. Look into this if / this function.~~

And let us know if you need any pointers :)

EDIT: looks like fixing that and giving the stash a message actually fixes the integration tests

mark2185 · 2022-06-21T13:39:51Z

pkg/commands/git_commands/stash.go

 }
+
+func (self *StashCommands) StashUntrackedChanges(message string) error {
+	if err := self.cmd.New("git stash -u").Run(); err != nil {


Redundant if, you could just return self.cmd.New().

I'd also suggest using the extended form of the flag, i.e. --include-untracked, just so it's easier to see what's going on.

mark2185 · 2022-06-21T21:27:24Z

Do integration tests work for you now? Can't seem to pass locally.

If it fails for you too, could you try adding a random message for stashing in the test? That seems to fix it

ajhynes7 · 2022-06-21T21:53:41Z

It's passing for me locally now.

❯ go test ./pkg/gui -run /stashIncludeUntrackedChanges
ok  	github.com/jesseduffield/lazygit/pkg/gui	4.589s

jesseduffield

Couple things :)

jesseduffield · 2022-07-01T08:03:12Z

pkg/commands/git_commands/stash.go

 	return nil
 }
+
+func (self *StashCommands) StashUntrackedChanges(message string) error {


StashChangedIncludingUntracked is a little more correct

Ok I've changed it now to StashIncludeUntrackedChanges to be consistent with LcStashIncludeUntrackedChanges.

But I can change those names again if you'd prefer.

jesseduffield · 2022-07-01T08:04:11Z

pkg/gui/controllers/files_controller.go


 func (self *FilesController) handleStashSave(stashFunc func(message string) error, action string, errorMsg string) error {
-	if !self.helpers.WorkingTree.IsWorkingTreeDirty() {
+	if action != self.c.Tr.Actions.StashIncludeUntrackedChanges && !self.helpers.WorkingTree.IsWorkingTreeDirty() {


what's the intention behind this line?

The method IsWorkingTreeDirty returns false if there are only untracked files, so the original expression evaluates to true and an error is raised. But I want to be able to stash these untracked files even if they're the only ones shown by git status.

I see. Rather than have conditionally logic inside this function for the sake of one use case, I say we pull the check out to the places that actually need it. This will add some duplication but I much prefer duplication to a clunky abstraction. So in the end handleStashSave will only call self.c.Prompt and it'll be up to the calling function to handle the validation

jesseduffield

Just one thing

jesseduffield · 2022-07-04T08:42:12Z

pkg/gui/controllers/files_controller.go


 func (self *FilesController) handleStashSave(stashFunc func(message string) error, action string, errorMsg string) error {
-	if !self.helpers.WorkingTree.IsWorkingTreeDirty() {
+	if action != self.c.Tr.Actions.StashIncludeUntrackedChanges && !self.helpers.WorkingTree.IsWorkingTreeDirty() {


I see. Rather than have conditionally logic inside this function for the sake of one use case, I say we pull the check out to the places that actually need it. This will add some duplication but I much prefer duplication to a clunky abstraction. So in the end handleStashSave will only call self.c.Prompt and it'll be up to the calling function to handle the validation

ajhynes7 · 2022-07-25T21:29:02Z

So in the end handleStashSave will only call self.c.Prompt and it'll be up to the calling function to handle the validation

Sorry, I can't figure out how to do this. The stashFunc function needs to check if the working tree is dirty, but I don't see how to access the IsWorkingTreeDirty method in the stash functions, which are located in pkg/commands/git_commands/stash.go. I can't import the FilesController type into this file.

I'm not a Go developer so I might just be missing something here.

jesseduffield · 2022-07-25T22:31:53Z

@ajhynes7 this can all happen within the files controller. We can remove the validation from handleStashSave like so:

func (self *FilesController) handleStashSave(stashFunc func(message string) error, action string) error {
	return self.c.Prompt(types.PromptOpts{
		Title: self.c.Tr.StashChanges,
		HandleConfirm: func(stashComment string) error {
			self.c.LogAction(action)

			if err := stashFunc(stashComment); err != nil {
				return self.c.Error(err)
			}
			return self.c.Refresh(types.RefreshOptions{Scope: []types.RefreshableView{types.STASH, types.FILES}})
		},
	})
}

And then we can pull the validation up to the callsite like so:

func (self *FilesController) createStashMenu() error {
	return self.c.Menu(types.CreateMenuOptions{
		Title: self.c.Tr.LcStashOptions,
		Items: []*types.MenuItem{
			{
				Label: self.c.Tr.LcStashAllChanges,
				OnPress: func() error {
					if !self.helpers.WorkingTree.IsWorkingTreeDirty() {
						return self.c.ErrorMsg(self.c.Tr.NoFilesToStash)
					}

					return self.handleStashSave(self.git.Stash.Save, self.c.Tr.Actions.StashAllChanges)
				},
				Key: 'a',
			},
			...

mark2185 · 2022-09-14T17:10:40Z

Since there's a transition to a new integration testing system, would you kindly create the test in the new framework? Just so we don't have to convert them down the road.

Here are the instructions.

ajhynes7 · 2022-11-01T18:39:20Z

Hi, just checking in about this PR. Is this still under review?

jesseduffield

Couple things :)

jesseduffield · 2022-11-12T04:08:52Z

pkg/integration/tests/stash/stash.go

+		assert.InMenu()
+
+		input.PressKeys("a")
+		input.Type("stash name")


we should assert that when 'a' is pressed, a prompt appears. Here's an example from another file:

input.PressKeys("a") assert.InPrompt() assert.MatchCurrentViewTitle(Equals("Enter a file name")) input.Type("my file")

jesseduffield · 2022-11-12T04:09:02Z

pkg/integration/tests/stash/stash_including_untracked_files.go

+
+		input.PressKeys("U")
+		input.Type("stash name")
+		input.Confirm()


same as above

jesseduffield · 2022-11-12T04:10:06Z

pkg/integration/tests/tests.go

 	"github.com/jesseduffield/lazygit/pkg/integration/tests/custom_commands"
 	"github.com/jesseduffield/lazygit/pkg/integration/tests/interactive_rebase"
 	"github.com/jesseduffield/lazygit/pkg/integration/tests/stash"
+	"github.com/jesseduffield/lazygit/pkg/utils"


we've got some linting issues that need to be fixed here

…security-fixes-lead-to-fatal-transport-file-not-allowed-error-in-ci-systems-cve-2022-39253.html) try this WIP

ajhynes7 · 2022-11-12T21:47:47Z

Had to deal with some merge conflicts. Should be good for another review now.

jesseduffield

one more thing

jesseduffield · 2022-11-13T01:58:17Z

pkg/gui/controllers/files_controller.go

 	return self.c.PostRefreshUpdate(self.context())
 }

 func (self *FilesController) handleStashSave(stashFunc func(message string) error, action string, errorMsg string) error {


we can remove errorMsg because it's no longer used here

jesseduffield · 2022-11-14T07:47:03Z

Great work @ajhynes7 !

ajhynes7 · 2022-11-14T13:06:42Z

Awesome, thanks @jesseduffield and @mark2185!

mark2185 reviewed Jun 21, 2022

View reviewed changes

jesseduffield requested changes Jul 1, 2022

View reviewed changes

ajhynes7 changed the title ~~Add stash option to include untracked files~~ feat: add stash option to include untracked files Jul 3, 2022

jesseduffield requested changes Jul 4, 2022

View reviewed changes

ajhynes7 requested a review from jesseduffield July 28, 2022 13:17

ajhynes7 requested review from jesseduffield and mark2185 and removed request for jesseduffield and mark2185 September 14, 2022 16:20

ajhynes7 and others added 10 commits September 15, 2022 21:48

feat: add stash option to include untracked changes

7ddb80a

fix: add condition to if statement

c7fd218

test: add integration test

088445b

refactor: change command order

50cf7ac

refactor: use extended flag name

4f8816e

refactor: remove redundant if statement

e66b162

fix: use message in git stash command

6feb301

test: add stash message

a0fd473

refactor: rename method to StashIncludeUntrackedChanges

c7733aa

refactor: move checks for clean working tree

e189546

ajhynes7 force-pushed the stash-untracked-changes branch from 9b62a86 to e189546 Compare September 16, 2022 00:19

ajhynes7 requested review from jesseduffield and mark2185 and removed request for mark2185 October 7, 2022 01:34

Merge branch 'master' into stash-untracked-changes

a47e728

jesseduffield requested changes Nov 12, 2022

View reviewed changes

ajhynes7 and others added 14 commits November 12, 2022 18:09

test: add more assertions

66a2539

Update README.md

af57dbd

Updated README.md

3341021

Update README.md

c769e58

Update README.md

ea09686

fix could-not-access error

97ced9e

fix broken CI (see https://vielmetti.typepad.com/logbook/2022/10/git-…

f98b2ed

…security-fixes-lead-to-fatal-transport-file-not-allowed-error-in-ci-systems-cve-2022-39253.html) try this WIP

Updated README.md

fd581ef

use better colour defaults

91f83c6

Update go-git to handle negative refspecs

f03e1e4

refactor: sort list of integration tests

d7fb441

fix: delete duplicate line

7977c2d

fix: delete duplicate import

9e91aa8

Merge branch 'master' into stash-untracked-changes

e56b05c

ajhynes7 requested a review from jesseduffield November 12, 2022 21:47

jesseduffield requested changes Nov 13, 2022

View reviewed changes

lint: delete unused argument from handleStashSave

3c436b3

ajhynes7 requested a review from jesseduffield November 13, 2022 15:45

lint: try deleting blank line

684d1e9

jesseduffield merged commit b33ec5a into jesseduffield:master Nov 14, 2022

ajhynes7 deleted the stash-untracked-changes branch November 14, 2022 13:06

Uh oh!

feat: add stash option to include untracked files #1980

feat: add stash option to include untracked files #1980

Uh oh!

Conversation

ajhynes7 commented Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshot

Uh oh!

mark2185 commented Jun 3, 2022

Uh oh!

ajhynes7 commented Jun 10, 2022

Uh oh!

mark2185 commented Jun 13, 2022

Uh oh!

ajhynes7 commented Jun 14, 2022

Uh oh!

mark2185 commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mark2185 commented Jun 21, 2022

Uh oh!

ajhynes7 commented Jun 21, 2022

Uh oh!

jesseduffield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jesseduffield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajhynes7 commented Jul 25, 2022

Uh oh!

jesseduffield commented Jul 25, 2022

Uh oh!

mark2185 commented Sep 14, 2022

Uh oh!

ajhynes7 commented Nov 1, 2022

Uh oh!

jesseduffield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajhynes7 commented Nov 12, 2022

Uh oh!

jesseduffield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jesseduffield commented Nov 14, 2022

Uh oh!

ajhynes7 commented Nov 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

ajhynes7 commented Jun 3, 2022 •

edited

Loading

mark2185 commented Jun 21, 2022 •

edited

Loading