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

Fix Discard of deleted files #689

Merged
merged 6 commits into from
Jan 22, 2025
Merged

Fix Discard of deleted files #689

merged 6 commits into from
Jan 22, 2025

Conversation

isc-etamarch
Copy link
Collaborator

Fixes #688

@isc-etamarch isc-etamarch linked an issue Jan 16, 2025 that may be closed by this pull request
@isc-etamarch
Copy link
Collaborator Author

isc-etamarch commented Jan 16, 2025

I did not fix stash, since stashing deleted files is frustrating. The best solution would be 'git stash push --staged' which would stash all staged files (just the deleted file), but this seems to break the unstaged files a little, where we now cannot see a diff from the Git UI, and git loses track of the type of operation (i.e. if a file is new, changed, etc.).

Until I find a solution that doesn't break everything, recommendation would be to discard deletions (especially since deleting is an easy operation to redo if needed).

@isc-etamarch isc-etamarch marked this pull request as draft January 16, 2025 20:26
@isc-etamarch isc-etamarch marked this pull request as ready for review January 16, 2025 20:28
@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 35.29412% with 11 lines in your changes missing coverage. Please review.

Project coverage is 40.47%. Comparing base (4f1f989) to head (679ef41).

Files with missing lines Patch % Lines
cls/SourceControl/Git/DiscardState.cls 35.29% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #689      +/-   ##
==========================================
- Coverage   40.60%   40.47%   -0.13%     
==========================================
  Files          23       23              
  Lines        3145     3150       +5     
==========================================
- Hits         1277     1275       -2     
- Misses       1868     1875       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@isc-pbarton
Copy link
Collaborator

@isc-etamarch I think this fixes a somewhat different issue than what we saw today that motivated #688. I'm still able to replicate that one by:

  • Add a production to source control and commit it.
  • Use the "List Productions" page to delete the production
  • In the Git repository, run "git add " to add the delete to the index. (This simulates what might happen if I tried to commit the delete through Web UI, the add succeeds, but the commit fails).
  • In the Git Web UI, select the deleted file and use the "Discard" option. It fails with "fatal: pathspec 'cls/Diagram/SampleProduction.cls' did not match any files" because it is trying to do a "git add" but the file was already removed from the index.

This goes through a different code path that doesn't involve SourceControl.Git.DiscardState

@isc-etamarch
Copy link
Collaborator Author

@isc-pbarton I see. That last commit should fix that as well (The other changes I made are still necessary for it to work)

Copy link
Collaborator

@isc-tleavitt isc-tleavitt left a comment

Choose a reason for hiding this comment

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

One really minor nitpick - feel free to tack on a commit to fix, then merge.

@isc-etamarch isc-etamarch merged commit c1d8963 into main Jan 22, 2025
2 checks passed
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.

Discard and Stash fail on 'delete' operation
4 participants