Make external file change detection explicit in save pipeline by iron-prog · Pull Request #3026 · metabrainz/picard

iron-prog · 2026-02-03T18:16:30Z

Summary

This is a…
- Bug fix
- Feature addition
- Refactoring
- Minor / simple change (like a typo)
- Other
Describe this change in 1-2 sentences:
Make external file change detection explicit in the save pipeline and prevent silent overwrites by aborting the save and marking the file as error when the on-disk file has changed after loading.

Problem

PR #2963 introduced FileIdentity to detect external file changes.
However, when a file was modified or replaced on disk after being loaded, Picard would only log a warning and continue, potentially overwriting external changes silently.
This could lead to unintended data loss without user awareness.

JIRA ticket (optional): PICARD-XXX

Solution

This change:
• Reuses FileIdentity to detect external modifications before saving.
• Raises ExternalFileModifiedError if the file was modified or deleted.
• Aborts the save operation.
• Marks the file as ERROR state with a clear message:
“Cannot save file: it was modified externally after being loaded.”
• Ensures no silent overwrite occurs.
• Keeps behavior batch-safe (no blocking dialogs per file).

This keeps the logic deterministic and avoids UI complexity for batch operations.

Limitation

•	No conflict resolution UI is implemented in this PR.
•	When multiple files are modified externally, each file is marked as error individually.
•	No batch policy (“overwrite all”, “reload all”) is included yet.
•	No diff view of tag differences.

These can be explored in a future PR if desired.

Action

Additional actions required:

Update Picard documentation (please include a reference to this PR)
Other (please specify below)

iron-prog · 2026-02-03T18:16:51Z

This change is limited to detection/logging only.
If this direction looks reasonable, I can follow up with user-visible handling in a separate PR; otherwise I’m happy to adjust or drop this.

zas

The main issue is that the PR adds return statements that abort saves when external changes are detected, whereas the original code only logged warnings and continued. This needs to be clearly documented as it's a significant behavioral change. Users will see no UI feedback when saves are silently aborted.

So I think, to be accepted, the user interface should be updated too, and user should be able to choose what he wants when a change was detected.

It seems that's what you suggest in your comment, and I think that's the way to go before we think about merging this.

picard/file.py

zas · 2026-03-03T10:45:57Z

@iron-prog ensure all tests pass (ruff)

…rites

iron-prog · 2026-03-04T11:49:03Z

@zas Update on implementation:
• Added retry support after external modification
• Added batch summary logging for skipped files
• Simplified error message

zas · 2026-03-04T11:57:48Z

picard/file.py

+            # Report summary of skipped files (once batch finishes)
+            if self.tagger._external_change_count > 0 and not self.is_pending():
+                log.warning(
+                    _("%d files were skipped because they were modified externally.")


Minor remark, but we don't usually translate log output (log should stay in English to ease parsing and ease reading when shared.

I have fixed it ,any other improvement or suggestion

zas · 2026-03-04T15:48:50Z

picard/file.py

+                self.state = File.State.ERROR
+                self.error_append(_("Cannot save file: it was modified externally after loading."))
+                # Track batch external changes
+                self.tagger._external_change_count += 1


Where is _external_change_count initialized?

sorry it's my mistake I have created this in tagger.py in init_tagger_entities which I does not uploaded

zas · 2026-03-04T15:52:00Z

picard/file.py

+            change = self._external_file_change(old_filename)
+            if change == ExternalChange.MISSING:
+                raise FileNotFoundError(f"{old_filename} no longer exists")
+            elif change:


Suggested change

elif change:

elif change == ExternalChange.MODIFIED:

Right?

Thanks for this

zas · 2026-03-04T15:53:38Z

picard/file.py

+        if self.state == File.State.ERROR:
+            change = self._external_file_change(self.filename)
+            if change == ExternalChange.MODIFIED:
+                self._loaded_identity = FileIdentity(self.filename)


Between checking for ExternalChange.MODIFIED and creating the new FileIdentity, the file could be modified again. Not sure if that's a real problem though.

there is indeed a small race window there. Since the retry simply refreshes the file identity before the next save attempt, I think the behavior remains consistent with the current optimistic checks.

zas · 2026-03-04T15:54:56Z

picard/file.py

+                log.warning(
+                    "%d files were skipped because they were modified externally.", self.tagger._external_change_count
+                )
+                self.tagger._external_change_count = 0


I think there's a logic flaw here:

This assumes the last file to finish saving will reset the counter, but with a single-threaded save pool, this might work accidentally

If multiple files are saved and some fail with external modifications, the counter might be reset prematurely by a successful save

The logic ties the counter to individual file state rather than batch completion

You’re right — my initial approach incorrectly tied the counter reset to individual file completion instead of batch completion. I see now that this makes the logic fragile even with a single-threaded save pool, because a successful save could reset the counter before all failures are accounted for.

@zas if any other changes requred I'm happy to make and if it's good then should I work on a prototype of giving option to user for batch changes (overwrite , skip, reload)?

iron-prog changed the title ~~File external change detection~~ Make external file change detection explicit in save pipeline Feb 3, 2026

zas requested changes Mar 1, 2026

View reviewed changes

picard/file.py Show resolved Hide resolved

Make external file change detection explicit in save pipeline

5356846

iron-prog force-pushed the file-external-change-detection branch 2 times, most recently from 81b245e to 5356846 Compare March 2, 2026 02:08

Make external file change detection user-aware and add conflict handling

8135219

iron-prog requested a review from zas March 2, 2026 04:03

zas reviewed Mar 2, 2026

View reviewed changes

picard/file.py Outdated Show resolved Hide resolved

fixing ruff fails

9a41236

iron-prog requested a review from zas March 2, 2026 16:17

iron-prog force-pushed the file-external-change-detection branch from 3edacc9 to 9a41236 Compare March 3, 2026 11:13

Handle externally modified files during save and prevent silent overw…

ed6760d

…rites

zas reviewed Mar 4, 2026

View reviewed changes

iron-prog added 2 commits March 4, 2026 17:34

English as per project conventions

04af7c4

ruff fail fix

944af8f

zas requested changes Mar 4, 2026

View reviewed changes

changes as requested

a5b07dc

iron-prog requested a review from zas March 5, 2026 13:08

Uh oh!

Conversation

iron-prog commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Solution

Limitation

Action

Uh oh!

iron-prog commented Feb 3, 2026

Uh oh!

zas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zas commented Mar 3, 2026

Uh oh!

iron-prog commented Mar 4, 2026

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

iron-prog commented Feb 3, 2026 •

edited

Loading