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

Remove temp files /tmp/fineDiff* for kill-emacs without quit ediff #16765

Merged
merged 1 commit into from
Jan 12, 2025

Conversation

sunlin7
Copy link
Contributor

@sunlin7 sunlin7 commented Dec 28, 2024

Fix a bug that the temp files /tmp/fineDiff* were left after Spacemacs ediff templates.

Reproducing steps: Pressing SPC f e D and press n to locate first diff line, then press SPC f q to quick Spacemacs directly.

Then the /tmp/fineDiff* files left.

@bcc32
Copy link
Collaborator

bcc32 commented Dec 29, 2024

For context, here is the upstream bug which adds the ediff function mentioned here.

@bcc32
Copy link
Collaborator

bcc32 commented Dec 29, 2024

IMO that's a pretty trivial bug, and I don't think "upstream Emacs will make this change eventually" is a generally a good reason on its own to backport a similar patch to Spacemacs---it's not even related to Spacemacs functionality, only ediff itself. I would vote to just close this, but will wait for others to chime in.

@sunlin7
Copy link
Contributor Author

sunlin7 commented Dec 29, 2024

I found the /tmp/fineDiff* files after Spacemacs SPC f e D function, and got the root cause, pushed the patch to emacs upstream.

So I hope Spacemacs user won't be bothered by using the SPC f e D function, that's why I pushed the solution here.

@bcc32
Copy link
Collaborator

bcc32 commented Dec 29, 2024

That doesn't really address my root concern: I don't think backporting random things to Spacemacs is worth the maintenance burden, especially when the impact is so low. (I can't imagine the existence of some files in /tmp is really bothering anybody, especially since they get cleaned up if you properly exit the ediff session).

Can explain why these files not getting cleaned up is actually causing you problems?

@sunlin7
Copy link
Contributor Author

sunlin7 commented Dec 29, 2024

When using Spacemacs on a never-rebooted linux server with shared /tmp/ folder, other users on the server can see the fineDiff* files were created again and again, they will find the creator and kindly reminder me with mail/chat that maybe some leak happened.

@bcc32
Copy link
Collaborator

bcc32 commented Dec 29, 2024

Okay so that's pretty specific to your site and not exactly catastrophic. Why not just fix it in your config, or use site-start?

@sunlin7
Copy link
Contributor Author

sunlin7 commented Dec 29, 2024

This issue happened on using the Spacemacs SPC f e D function, I do fixed it on my local about a month ago. And hope the solution can help the Spacemacs users who still with emacs-28/29/30.

@sunlin7 sunlin7 force-pushed the ediff-remove-temp-files branch from 3ac7821 to c712025 Compare December 30, 2024 02:30
Copy link
Collaborator

@smile13241324 smile13241324 left a comment

Choose a reason for hiding this comment

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

As much as I dislike fixing emacs bugs in Spacemacs we have done this for a lot of broken packages in the past. Given how long it will take until a new emacs version is broadly accepted I think it makes sense to shield our users from those issues.

We can do cleanup once we increase the min emacs Version again.

@smile13241324
Copy link
Collaborator

@sunlin7 can you check the conflicts on your branch? Once its ready please ping me and I will check and merge it.

* core/core-versions.el: New macro spacemacs/block-before-emacs-min-version
* layers/+spacemacs/spacemacs-defaults/funcs.el: New function
    spacemacs//ediff-delete-temp-files.
* layers/+spacemacs/spacemacs-defaults/packages.el: Use
    spacemacs//ediff-delete-temp-files on the kill-emacs-hook
@sunlin7 sunlin7 force-pushed the ediff-remove-temp-files branch from c712025 to b8f5c00 Compare January 12, 2025 06:05
@sunlin7
Copy link
Contributor Author

sunlin7 commented Jan 12, 2025

@smile13241324 Sure, had resolved the conflicts. Please help review again. Thank you !

@smile13241324 smile13241324 merged commit f75db2e into syl20bnr:develop Jan 12, 2025
8 of 9 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.

3 participants