Adding missing file closes by Neko-Box-Coder · Pull Request #3807 · micro-editor/micro

Neko-Box-Coder · 2025-07-19T14:03:52Z

Switching between git branches on Windows sometimes fails due to opened files in micro.
Adding missing file closes fixes this.

dmaluka · 2025-07-19T14:26:33Z

internal/buffer/save.go

 	size, err := file.Write(b)
 	if err != nil {
 		err = util.OverwriteError{err, backupName}
+		file.Close()


Good catch. But why just here? What about all other error returns above, where we also leak file?

True, somehow missed it :/

Anyway, I moved the file opening closer to when we need it, instead of the beginning so now we always do a backup first, which I think is good.

I'm guilty for not adding the file.Close() in the the err-paths above.
Having this backup could be unintentionally, because the user might not like having them at all (permbackup set to false). When the open fails right after and the user decides to drop that change anyway, then the user is faced with the error prompt next time opening this file.

Do we really want that?

then the user is faced with the error prompt next time opening this file.

I don't have enough context, what do you mean exactly? Do we check for backup files (and warn the user?) when we open the same file again?

https://github.com/zyedidia/micro/blob/7492195a5ba4e373323d82ffd96681d6b9f4c4cc/internal/buffer/backup.go#L15-L33

I see.

I have moved the backup part out so I don't have to put file close in so many places.
It now does roughly the same as before except it retains the backup if the file close failed.

Do we really want that?

We don't. As you might remember from #3273, the code structure is what it is, among other things, precisely because we wanted to avoid that.

dmaluka · 2025-07-19T18:53:02Z

internal/buffer/save.go

 	return err
 }

+func (b *Buffer) backupFile(path string) (string, error) {


writeBackup()? backupFile() is not the most suitable name here (for example, because if backupFile() backs up a file, one would expect that backupDir() backs up a directory), right?

Well, we are backing up a file, maybe we should rename backupDir() to getBackupDir() (or backupDirPath()) instead?

This is not the only function where we are backing up a file. We are also doing it in Backup() (because we create backups in more cases than when the user saves a file).

So we should have a common function that performs the backup?

No, because they are different kinds of backups, performed in different ways, as you can see from the code.

Hmm... it looks like we have a couple of bugs with saving with sudo:

🤦‍♂️

Looks like we've to store some more stuff for the withSudo-case and start the cmd in func (wf wrappedFile) Write() earliest.
Good hint regarding conv=fsync, this should do the trick.

Should we consider this as part of milestone v2.0.15?
From my perspective: yes

Looks like we've to store some more stuff for the withSudo-case and start the cmd in func (wf wrappedFile) Write() earliest.

Which would mean that if sudo dd ... fails just because the user hasn't typed the correct password (so dd was not actually executed, so the file was not corrupted), we still write the backup, and don't remove it?

Should we consider this as part of milestone v2.0.15?

You mean the sudo issues? Those are not regressions (before #3273, save with sudo was not safe either, since any save was unsafe), so not necessarily?

Although in general, providing safe writes for sudo indeed seems no less important than providing safe writes in normal case (perhaps even more important, given that with sudo we have absolute power, and are able to corrupt any file in the filesystem).

Which would mean that if sudo dd ... fails just because the user hasn't typed the correct password (so dd was not actually executed, so the file was not corrupted), we still write the backup, and don't remove it?

No, it would require more logic, but anyway it is much easier and conv=notrunc,fsync (fsync only just to be sure) is all we need.

Although in general, providing safe writes for sudo indeed seems no less important than providing safe writes in normal case (perhaps even more important, given that with sudo we have absolute power, and are able to corrupt any file in the filesystem).

This is my concern too.
We should fix this, since the fix looks small and easy (-> #3814).

but anyway it is much easier

No it is not (unless you have an idea of something as easy as #3814 but actually working).

dmaluka · 2025-07-19T18:53:37Z

internal/buffer/save.go

-	if err != nil {
-		err = util.OverwriteError{err, backupName}
-		return size, err
+	{


Is this just to avoid having err2? Why avoid that?

err2 replaces err anyway in the original.

Here, we are returning immediately if the write or close failed, for clarity.

I was asking why do you need this extra scope. I assumed you added it in order to be able to shadow the err variable at line 395. If so, why need to shadow it instead of keeping using another variable? If not, why need this extra scope?

The scope is just for clarity together with the comment.
If we failed inside this scope, we keep the backup (if any).

The err at 395 should be normal assignment instead, missed it.

Now I'm not sure this scope actually improves readability. You had to add this size := 0 which serves no purpose but to work around assigning size inside this scope...

It's a minor thing anyway, that extra size := 0 makes no difference.

The block is surrounded by forceKeepBackup and the comment applies pretty nicely to the whole block as well.

Ok, no biggie.

dmaluka · 2025-07-19T18:54:29Z

internal/buffer/save.go

+		if err != nil {
+			file.Close()
+			err = util.OverwriteError{err, backupName}
+			return 0, err


We can just return 0, util.OverwriteError{err, backupName}?

dmaluka · 2025-07-19T19:02:01Z

internal/buffer/save.go

-			return 0, err
-		}
-	}
+	b.forceKeepBackup = true


Why move setting forceKeepBackup here? We force keeping the backup in case we actually have successfully created this backup, right?

In the original, we force backup (or rather retain the backup) if we failed to do backup or failed to write to the file so that we keep trying again later.

We are doing the same here except we also force the backup if we failed to close the file as well.

In the original, we force backup (or rather retain the backup) if we failed to do backup

No we don't. In the original we don't set forceKeepBackup to true until we have successfully written the backup and are ready to start writing the original file.

No we dont.

True, missed that. But if we failed to backup, then we should keep whatever backup was there no?

Why would we? If we failed to backup, the backup file is a garbage, right?

I missed it but we are already removing it inside backupFile(). Should be fine then right?

And then safeWrite() returns with an error and leaves forceKeepBackup set to true, which is not what we want, right?

dmaluka reviewed Jul 19, 2025

View reviewed changes

Neko-Box-Coder force-pushed the FileClosing branch 5 times, most recently from 2672aa8 to c28c67e Compare July 19, 2025 17:44

dmaluka reviewed Jul 19, 2025

View reviewed changes

Neko-Box-Coder force-pushed the FileClosing branch 2 times, most recently from becf2a3 to ec01071 Compare July 19, 2025 19:33

Adding missing file closes, rewriting safeWrite() to be more robust

be814fd

Neko-Box-Coder force-pushed the FileClosing branch from ec01071 to be814fd Compare July 19, 2025 22:32

dmaluka approved these changes Jul 20, 2025

View reviewed changes

JoeKar approved these changes Jul 22, 2025

View reviewed changes

dmaluka merged commit d7e43d4 into micro-editor:master Jul 22, 2025
6 checks passed

dmaluka mentioned this pull request Aug 3, 2025

Fix spurious backups of unmodified files #3822

Merged

Conversation

Neko-Box-Coder commented Jul 19, 2025

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!

Neko-Box-Coder Jul 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmaluka Jul 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

JoeKar Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

dmaluka Jul 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Neko-Box-Coder Jul 19, 2025 •

edited

Loading

dmaluka Jul 19, 2025 •

edited

Loading

JoeKar Jul 22, 2025 •

edited

Loading

dmaluka Jul 19, 2025 •

edited

Loading