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

FormCommit: Add soft reset for amending #10699

Merged
merged 3 commits into from Mar 6, 2023

Conversation

mstv
Copy link
Member

@mstv mstv commented Feb 5, 2023

Fixes #2260

Proposed changes

  • Add Reset soft button for amending a commit
  • Disable the Amend commit checkbox only after Soft Reset, not in dependency of staging
  • ReferenceRepository: Write git index (LibGitSharp does not do this automatically)
  • BugFix: Update button states after commit (Reset.Enabled and CommitAndPush.Text)

Screenshots

Before

image

After

image

ResetSoft_twice

Test methodology

  • add testcase for workflow
  • adapt other testcases to ReferenceRepository

Test environment(s)

  • Git Extensions 33.33.33
  • Build a5b0156
  • Git 2.39.2.windows.1
  • Microsoft Windows NT 10.0.19045.0
  • .NET 6.0.14
  • DPI 96dpi (no scaling)
  • Microsoft.WindowsDesktop.App Versions
    Microsoft.WindowsDesktop.App 6.0.14 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
    Microsoft.WindowsDesktop.App 7.0.3 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).


✒️ I contribute this code under The Developer Certificate of Origin.

@mstv mstv self-assigned this Feb 5, 2023
@mstv mstv added this to the vNext milestone Feb 5, 2023
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

I'm not fond of adding more controls to this panel - it is already too "bisy". Other than that, it looks ok.

@mstv
Copy link
Member Author

mstv commented Feb 25, 2023

I'm not fond of adding more controls to this panel - it is already too "busy". Other than that, it looks ok.

This (checkbox and) button are invisible until Amend commit is ticked.
I'd like to merge it tomorrow.

Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

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

+1
Agree to this behavior, something "virtual" showing the complete change without an actual reset is too complicated.
Very brief review though. The enabling of Amend seem to be the change to focus on...

@mstv
Copy link
Member Author

mstv commented Feb 26, 2023

When amending by means of Soft Reset, the changes need to be committed again before another amend makes sense.

ResetSoft_twice

@RussKie
Copy link
Member

RussKie commented Feb 28, 2023

Let's take it! I will annoy you with a UI layout change later 😆

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

Approving but it'd be great to add some tests.

@mstv
Copy link
Member Author

mstv commented Feb 28, 2023

I will annoy you with a UI layout change later 😆

Why do you expect this? Are you going to break the workflow Alt+A, Alt+F, Ctrl+Enter / Alt+P?

but it'd be great to add some tests

I came to the same conclusion when I was testing it manually.

The ReferenceRepository is driving me nuts though. It seems that repository.Index.Add(_fileName); does not really stage the file as expected - but just internally.
Then Commit(repository, commitMessage); creates a commit with the file. But the staged state of the file becomes "deleted".
This confuses me and my test because the repo is not clean. It is not easily possible to bring the repo into the clean state using the ReferenceRepository. Any clues?

image

@RussKie
Copy link
Member

RussKie commented Mar 1, 2023 via email

@mstv
Copy link
Member Author

mstv commented Mar 1, 2023

One needs to call repository.Index.Write(); in order to let LibGitSharp update the .git folder.
Unfortunately, this breaks other tests, which rely on the weird not updated git index.

@mstv mstv marked this pull request as draft March 1, 2023 22:30
@mstv mstv force-pushed the feature/amend_reset_soft branch from 53fd3f6 to 16249e8 Compare March 2, 2023 23:08
@mstv mstv marked this pull request as ready for review March 3, 2023 00:33
@mstv mstv force-pushed the feature/amend_reset_soft branch from 13f1902 to a5b0156 Compare March 3, 2023 00:51
@RussKie
Copy link
Member

RussKie commented Mar 4, 2023

I'm kind of wondering - do we need the "soft reset" button at all? Shouldn't we just execute it when "amend" is ticked? That is, if a user chose to amend, it's logical to assume he/she'd want to see the amended files? Or am I overthinking it?

The other question I have (I apologise, I haven't had a chance to try this branch) is about the experience when the user closes the dialog after soft resetting and not committing the changes, or simply changes the mind about amending. What are we doing with the soft-reset files?

@mstv
Copy link
Member Author

mstv commented Mar 4, 2023

Shouldn't we just execute it when "amend" is ticked?

You have already answered it yourself: "... changes the mind about amending. What are we doing with the soft-reset files?"
And after soft-reset, one does not see any longer what is being changed, i.e. the diff to the commit being amended.
That's why the removal of the previous commit should be a conscious decision by the user, who must take the responsibility for the resulting files, too.

@gerhardol
Copy link
Member

I often toggle amend box to get the commit message to edit it for a squash (or amend! now when I know about it) (instead of using ctrl-x and changing the message)

@RussKie
Copy link
Member

RussKie commented Mar 5, 2023

I do check the box a lot, similar to @gerhardol.

I wonder if this functionality can be approached a little differently. That is, when the box is checked we automatically read HEAD~1 content and display it in the "staged" area. This would allow to review the content. These files can be marked differently - e.g., they can't be immediately unstaged or reset.
An attempt to unstage or reset would show a confirmation dialog - "Do you want to perform soft reset?", and if yes, we perform a soft reset and allow interact with all files.
This is not an attempt to derail or block this implementation, and more an exploration of whether we can provide a more seamless experience.

@gerhardol
Copy link
Member

Showing head~1 in staged would be confusing. A third read only file list could be shown but that will still not show all changes to a file in this list. I often use browse for such manipulations when i do not want to amend just getting an overview for a fixup.

Soft reset is good enough for now.

@vbjay
Copy link
Contributor

vbjay commented Mar 6, 2023

What if they reset soft and then change their minds? Uncheck ammend after soft reset?

@mstv
Copy link
Member Author

mstv commented Mar 6, 2023

What if they reset soft and then change their minds? Uncheck amend after soft reset?

They need to commit again (or use the reflog).
The Amend checkbox is unticked and disabled on soft reset.

@mstv mstv force-pushed the feature/amend_reset_soft branch from a5b0156 to fd0619b Compare March 6, 2023 19:30
@vbjay
Copy link
Contributor

vbjay commented Mar 6, 2023

The Amend checkbox is unticked and disabled on soft reset.

Till they open commit window again? They could merge commits by:

  1. Amend
  2. Soft reset
  3. Close commit window
  4. Open commit window
  5. Amend
  6. Soft reset
  7. Wash..rinse repeat.

@mstv
Copy link
Member Author

mstv commented Mar 6, 2023

This soft reset needs three clicks by default, Jay. We cannot take responsibility of all user clicks. The reflog is still there.

One can squash commits using this with reopening FormCommit. :)

@vbjay
Copy link
Contributor

vbjay commented Mar 6, 2023 via email

@vbjay
Copy link
Contributor

vbjay commented Mar 6, 2023

One more. Have you tried with a skip work tree file that is in commit?

@mstv
Copy link
Member Author

mstv commented Mar 6, 2023

One more. Have you tried with a skip work tree file that is in commit?

I have not. But very likely, the file will not be committed as git was told so.
Forgetting about skip-worktree files is a trap.
Though in the case of amending, it should not be long ago.

@mstv mstv merged commit f5a55ca into gitextensions:master Mar 6, 2023
mstv added a commit that referenced this pull request Mar 6, 2023
mstv added a commit that referenced this pull request Mar 6, 2023
@mstv mstv deleted the feature/amend_reset_soft branch March 6, 2023 21:45
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.

amend should show files of amended commit
4 participants