Repository: jesseduffield/lazygit (language: go)

Issue:
Improvements to Stash Staged changes.
**Is your feature request related to a problem? Please describe.**
Yes. Currently when using Stash Staged changed I noticed some things that can be improved.

1. Two stashes are created unnecessary.
2. Sometimes it errors telling that the selection can't be stashed.
3. When it succeed, the stashed files are keep in the index (I would expect to be removed).

**Describe the solution you'd like**
I think the steps to stash staged files can be simpler. I currently use SmartGit which has an option `Stash Selection`
When looking into the output, the git command ran is:

`git stash push --include-untracked -- FILE_1 FILE_2`

With this info the steps to reproduce this command could be.

1. Get list of staged files
2. Unstage all changes.
3. Run the command like SmartGit.

**Screnshots**
Error in LazyGit
![image](https://github.com/jesseduffield/lazygit/assets/999204/eb2899ce-538f-4829-9f7d-04c94cafda9f)

Two stashed were created.
![image](https://github.com/jesseduffield/lazygit/assets/999204/2c5e4c8b-b698-4f33-aa3a-9a0439977676)


**Version info:**
LazyGit Version: `commit=5e388e21c8ca6aa883dbcbe45c47f6fdd5116815, build date=2023-08-07T14:05:48Z, build source=binaryRelease, version=0.40.2, os=darwin, arch=arm64, git version=2.43.2`
OS: `MacOS Sonoma 14.1`

**Describe alternatives you've considered**
N/A

**Additional context**
I'm happy to work on this if the improvement makes sense.




Discussion / hints:
Could you please fill out the rest of the template, e.g. which version and/or OS you're using?
Updated, I used the Feature Request template, it doesn't have version info.
This sounds more like a bug than an enhancement request.

However, I just did a quick test and the command worked correctly for me: it created one stash, didn't pop up any error, and removed the stashed changes from the working tree (leaving the unstaged changes there).

It would help if you could come up with a more concrete recipe for how to reproduce these failures.

> `git stash push --include-untracked -- FILE_1 FILE_2`

This stashes both staged and unstaged changes, so we can't use it here. (Note that lazygit supports the scenario where you have both staged and unstaged changes within a single file.)
I noticed that that it happens with specific files. In this case with very big files.
I'll try to create a file that I can share to replicate this.

> Note that lazygit supports the scenario where you have both staged and unstaged changes within a single file.

👍  Thanks, I missed this.

I found a way to reproduce it; it seems to happen whenever staged and unstaged changes in the same file are too close. (Not sure that's the only way to trigger it.)

To reproduce, make changes to two lines that have one or two unchanged lines between them; stage only one of the changes. This will trigger the behavior described above. If there are three or more unchanged lines between the changed ones, it works fine.

I haven't yet looked into why it fails, or what we can do about it.
I have looked into this issue and found this in the git docs:
```
-S
--staged
This option is only valid for push and save commands.

Stash only the changes that are currently staged. This is similar to basic git commit except the state is committed to the stash instead of current branch.
```
and I tried this doing the following:
1- Create file1.txt and commit with content:
2- Create file2.tx with any content:
3- Modify file1.txt with the following:

<img width="994" alt="image" src="https://github.com/jesseduffield/lazygit/assets/55502191/f9961a5e-abc7-4eec-a9e1-c5628afdfd6a">

4- Select in lazygit to stage file2.tx (all content)
5- Select in lazygit to stage from file1.tx:

<img width="998" alt="image" src="https://github.com/jesseduffield/lazygit/assets/55502191/934b5591-8c5e-42cc-9fa8-446d621f7561">

6- Open a terminal and paste: `git stash push --staged`
7- The output in the stash is:

<img width="996" alt="image" src="https://github.com/jesseduffield/lazygit/assets/55502191/80814f25-537d-4059-a3a7-3830b00fdb91">

And file1.txt now has:

<img width="994" alt="image" src="https://github.com/jesseduffield/lazygit/assets/55502191/aeb093ce-9dca-4de9-8f8c-66cb5104eb06">

The issue that I get is that I cannot apply/pop the stash after that, since I get this error:

<img width="995" alt="image" src="https://github.com/jesseduffield/lazygit/assets/55502191/13abb0e6-0d7a-469f-a185-fa0637f0e671">

I tested the behaviour of the `git stash push --staged` in "normal" circumstances and it's working as intended, so unless there's a problem with this command that I don't know of I think that is a cleaner approach to "Stashing all staged files"
Ah nice, I wasn't aware of the `--staged` option. Yes, that's obviously better and also solves the problem of #3563, so it seems that we don't need #3568 then.

The problem is that the option was only added in git 2.35.0, but we need to support 2.20.0 and later. We have two options:
- fall back to our current implementation for versions older than 2.35.0, (and then live with the two bugs that it has)
- disable the command for versions older than 2.35.0

I could go with either option, but I have a slight preference for the second because it's easier for us. I'll let @jesseduffield decide what to do.

As for the error that you got when popping the stash: that's normal git behavior. You get the same error, for example, when trying to apply a custom patch for a file that has unstaged changes. The workaround is to stage your unstaged file first, then it will succeed in both cases. I admit that this is very unintuitive, and also the error message is quite confusing, but that's git's problem, not ours.
I also wasn't aware of that option. I stash staged changes all the time so that's going to be useful.

2.35.0 was in 2022 and I don't want to force users to update their git unless it's quite old (i.e. older than 2022) so I think we should use the fallback approach.
> I don't want to force users to update their git unless it's quite old (i.e. older than 2022)

This sounds like you're contradicting yourself. Isn't this saying that you find it ok if users have to update their git if it's older than 2.35.0? I'm confused. (Also, we are not "forcing" users to update their git; only if they want to use the stash staged changes command.)
@stefanhaller I meant to say much older than 2022. Some users seem to very slowly update their git versions (e.g. because they're on OS's with old git versions and because of some company policy they can't update).

As for forcing users to update: it's all on a spectrum. The more useful the feature is, the more that it should be considered 'forcing'. I consider the stash staged feature very useful, and I assume many other users do too.

On net I think it's going to be better for users to support pre 2.35.0 git for stashing staged changes.
So, In summary, for users with git version older than 2.35.9 the flow remains the same, and for users of this version an higher the `git stash push --staged` is used, is that correct?
I can add the needed changes to the current PR https://github.com/jesseduffield/lazygit/pull/3568 or create a new one for this but since the changes are related there could be conflicts if they are separate.
> So, In summary, for users with git version older than 2.35.9 the flow remains the same, and for users of this version an higher the git stash push --staged is used, is that correct?

That's right

> I can add the needed changes to the current PR https://github.com/jesseduffield/lazygit/pull/3568 or create a new one for this but since the changes are related there could be conflicts if they are separate.

Feel free to add the changes to the current PR
No, I would close #3568 and create a new one instead. We don't need the changes in #3568 any more. It would only unnecessarily complicate the code with too little benefit.
To be clear: that PR only helps users on the old git version? If that's the case, I'm okay with keeping the code simple and requiring users on the old git version to upgrade if they want the better behaviour.
Yes, the new version doesn't need the fix.
It's also an uncommon situation for people to have _only_ staged changes, and then to invoke the "Stash staged changes" command. Most people will probably invoke the default stash command in that case.
Good point
Makes sense to me

Fix patch:
diff --git a/pkg/commands/git_commands/stash.go b/pkg/commands/git_commands/stash.go
index 906535fe568..5eeaa6a68c9 100644
--- a/pkg/commands/git_commands/stash.go
+++ b/pkg/commands/git_commands/stash.go
@@ -121,9 +121,22 @@ func (self *StashCommands) StashUnstagedChanges(message string) error {
 	return nil
 }
 
-// SaveStagedChanges stashes only the currently staged changes. This takes a few steps
-// shoutouts to Joe on https://stackoverflow.com/questions/14759748/stashing-only-staged-changes-in-git-is-it-possible
+// SaveStagedChanges stashes only the currently staged changes.
 func (self *StashCommands) SaveStagedChanges(message string) error {
+	if self.version.IsAtLeast(2, 35, 0) {
+		return self.cmd.New(NewGitCmd("stash").Arg("push").Arg("--staged").Arg("-m", message).ToArgv()).Run()
+	}
+
+	// Git versions older than 2.35.0 don't support the --staged flag, so we
+	// need to fall back to a more complex solution.
+	// Shoutouts to Joe on https://stackoverflow.com/questions/14759748/stashing-only-staged-changes-in-git-is-it-possible
+	//
+	// Note that this method has a few bugs:
+	// - it fails when there are *only* staged changes
+	// - it fails when staged and unstaged changes within a single file are too close together
+	// We don't bother fixing these, because users can simply update git when
+	// they are affected by these issues.
+
 	// wrap in 'writing', which uses a mutex
 	if err := self.cmd.New(
 		NewGitCmd("stash").Arg("--keep-index").ToArgv(),


Test patch:
diff --git a/pkg/integration/tests/stash/stash_staged_partial_file.go b/pkg/integration/tests/stash/stash_staged_partial_file.go
new file mode 100644
index 00000000000..d9ac30e1bcb
--- /dev/null
+++ b/pkg/integration/tests/stash/stash_staged_partial_file.go
@@ -0,0 +1,70 @@
+package stash
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var StashStagedPartialFile = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "Stash staged changes when a file is partially staged",
+	ExtraCmdArgs: []string{},
+	GitVersion:   AtLeast("git version 2.35.0"),
+	Skip:         false,
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shell.CreateFileAndAdd("file-staged", "line1\nline2\nline3\nline4\n")
+		shell.Commit("initial commit")
+		shell.UpdateFile("file-staged", "line1\nline2 mod\nline3\nline4 mod\n")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Files().
+			IsFocused().
+			PressEnter()
+
+		t.Views().Staging().
+			Content(
+				Contains(" line1\n-line2\n+line2 mod\n line3\n-line4\n+line4 mod\n"),
+			).
+			PressPrimaryAction().
+			PressPrimaryAction().
+			Content(
+				Contains(" line1\n line2 mod\n line3\n-line4\n+line4 mod\n"),
+			).
+			PressEscape()
+
+		t.Views().Files().
+			IsFocused().
+			Press(keys.Files.ViewStashOptions)
+
+		t.ExpectPopup().Menu().Title(Equals("Stash options")).Select(MatchesRegexp("Stash staged changes$")).Confirm()
+
+		t.ExpectPopup().Prompt().Title(Equals("Stash changes")).Type("my stashed file").Confirm()
+
+		t.Views().Stash().
+			Focus().
+			Lines(
+				Contains("my stashed file"),
+			).
+			PressEnter()
+
+		t.Views().CommitFiles().
+			IsFocused().
+			Lines(
+				Contains("file-staged").IsSelected(),
+			)
+		t.Views().Main().
+			Content(
+				Contains(" line1\n-line2\n+line2 mod\n line3\n line4\n"),
+			)
+
+		t.Views().Files().
+			Lines(
+				Contains("file-staged"),
+			)
+
+		t.Views().Staging().
+			Content(
+				Contains(" line1\n line2\n line3\n-line4\n+line4 mod\n"),
+			)
+	},
+})
diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go
index bf6264cde3d..2548702e9a0 100644
--- a/pkg/integration/tests/test_list.go
+++ b/pkg/integration/tests/test_list.go
@@ -257,6 +257,7 @@ var tests = []*components.IntegrationTest{
 	stash.StashAndKeepIndex,
 	stash.StashIncludingUntrackedFiles,
 	stash.StashStaged,
+	stash.StashStagedPartialFile,
 	stash.StashUnstaged,
 	status.ClickRepoNameToOpenReposMenu,
 	status.ClickToFocus,