Project config: preserve resources and localized files by ayshushus · Pull Request #4571 · mozilla/pontoon

ayshushus · 2026-10-06T04:05:17Z

Current resources whose path is no longer in paths.ref_paths now go through mark_as_obsolete().
Localized files are now kept.
If the project uses a config and a config file is in the diff, the source file is now added to the diff along with the existing localized files.
Add a holistic test.

… localized file)

mathjazz

Thanks for the patch! Left a few comments inline.

mathjazz · 2026-10-07T19:19:24Z

+    removed_paths = {
+        get_db_path(paths, join(checkout.path, co_path)) for co_path in checkout.removed
+    }
+    if isinstance(paths, L10nConfigPaths):


Marking entities obsolete here can't be undone later. When a path comes back, update_resources() un-obsoletes the resource but not its entities, so it creates a second copy of every Entity, and the old translations, comments and reviews stay stuck on the hidden ones.

Is this effectively #4128? Based on your comment there, I'm applying a content (value & properties) match that is LIFO/newest copy. Also adding a "Restored" log.

mathjazz · 2026-10-07T19:24:41Z

+        get_db_path(paths, join(checkout.path, co_path)) for co_path in checkout.removed
+    }
+    if isinstance(paths, L10nConfigPaths):
+        ref_db_paths = {get_db_path(paths, ref_path) for ref_path in paths.ref_paths}


ref_paths is just whatever the glob matched this run, so a typo in reference (or a source folder moved one commit before the config is updated) makes it empty and wipes out the whole project in one sync.

Shall we refuse to obsolete when the list covers all or nearly all resources?

mathjazz · 2026-10-07T19:27:40Z

+    current = set(project.resources.current().values_list("path", flat=True))
+    for ref_path in paths.ref_paths:
+        if (
+            relpath(ref_path, paths.ref_root) in current


This skips references we already track, so if only the l10n target moves (e.g. {locale}/a.ftl → l10n/{locale}/a.ftl), the files at the new location never get added to target.changed and their translations aren't picked up.

eemeli

A partial review, happy to have Matjaž take the lead here.

eemeli · 2026-10-09T11:18:45Z

+        ref_db_paths = {get_db_path(paths, ref_path) for ref_path in paths.ref_paths}
+        current_paths = set(project.resources.current().values_list("path", flat=True))
+        unreferenced_paths = current_paths - ref_db_paths
+        if len(unreferenced_paths) > 0.9 * len(current_paths):


This magic number should be a constant defined near the top of the file.

eemeli · 2026-10-09T11:19:59Z

+    }
+    if isinstance(paths, L10nConfigPaths):
+        ref_db_paths = {get_db_path(paths, ref_path) for ref_path in paths.ref_paths}
+        current_paths = set(project.resources.current().values_list("path", flat=True))


This is the second time you end up doing the same DB query during the same sync; we should avoid that.

eemeli · 2026-10-09T11:25:25Z

+                ent.obsolete = False
+                ent.date_obsoleted = None
+                restored[key] = ent
+        Entity.objects.bulk_update(restored.values(), ["obsolete", "date_obsoleted"])


Entity deobsoletion seems like quite a bit of scope creep for work under "Project config: preserve resources and localized files". I'm pretty sure this ought to get left out at least for now, and addressed separately -- there are almost certainly side effects that need to be accounted for.

@mathjazz, thoughts?

Alright, let's leave that out.

Co-authored-by: Eemeli Aro <eemeli@gmail.com>

mathjazz

This looks good, nice work!

Co-authored-by: Eemeli Aro <eemeli@gmail.com>

eemeli · 2026-10-14T16:45:13Z

            tgt_path = paths.format_target_path(tgt_template, locale)
-            if tracked and (
-                ref_path in renamed_refs
-                or prev_paths is None
-                or prev_paths.target(ref_path, locale=locale)[0] == tgt_path
-            ):
-                continue
+            if has_current_resource:
+                prev_tgt_path, _ = prev_paths.target(ref_path, locale=locale)
+                if prev_tgt_path == tgt_path:
+                    continue


If the paths match for any locale, doesn't that mean that they'll match for every locale? Could you not compare the template strings instead, outside this loop?

Uhhh... yes. I also just realized that the previous approach was a little inefficient...
Changing now.

eemeli · 2026-10-14T16:53:13Z

+    config_current_paths: set[str] = set()
+    if isinstance(paths, L10nConfigPaths):
+        config_current_paths = {
            renamed_db_paths.get(path, path)
            for path in project.resources.current().values_list("path", flat=True)
        }


The term "current" is confusing me here, because it's not clear if it's a point in time before the sync started, some point during the sync, or after the sync has completed.

Let's avoid it, and use something more verbose but explicit like config_db_paths_with_renames.

eemeli · 2026-10-14T17:07:07Z

+    for ref_path in paths.ref_paths:
+        if not isfile(ref_path) or not is_inside(source.path, ref_path):
+            continue
+        has_current_resource = relpath(ref_path, paths.ref_root) in config_current_paths


This is definitely better, but still a bit confusing, again I think driven by the "current" term being unclear.

Would something like this be accurate?

Suggested change

has_current_resource = relpath(ref_path, paths.ref_root) in config_current_paths

is_new = relpath(ref_path, paths.ref_root) not in config_current_paths

Note that the conditional is inverted here.

eemeli · 2026-10-14T17:10:15Z

+        if not has_current_resource and ref_path not in src_changed:
+            source.changed.append(relpath(ref_path, source.path))
+        if has_current_resource and ref_path in renamed_refs:
+            continue


Continuing with the previous suggestion:

Suggested change

if not has_current_resource and ref_path not in src_changed:

source.changed.append(relpath(ref_path, source.path))

if has_current_resource and ref_path in renamed_refs:

continue

if is_new:

if ref_path not in src_changed:

source.changed.append(relpath(ref_path, source.path))

else:

if ref_path in renamed_refs:

continue

ayshushus added 4 commits October 5, 2026 23:34

Define and add newly configured files

5166374

Update obsoletion in remove_resources()

7f5c41e

Update delete_removed_resources for deobsoletion process (to preserve…

99b1bac

… localized file)

Add tests

065b116

ayshushus requested a review from mathjazz October 6, 2026 04:06

mathjazz reviewed Oct 7, 2026

View reviewed changes

ayshushus added 3 commits October 9, 2026 03:49

Merge branch 'mozilla:main' into 2844-config

5932d0e

Add deobsoletion logic and tests

da2921d

Refuse to obsolete when the list covers >90% resources

82f9cd9

eemeli reviewed Oct 9, 2026

View reviewed changes

ayshushus and others added 5 commits October 9, 2026 10:51

Update pontoon/sync/core/paths.py

ee96421

Co-authored-by: Eemeli Aro <eemeli@gmail.com>

Define constant for the magic number and lint

f97d6f1

Fix repeated DB query and rename, add tests for renamed case

385d0d7

Revert "Add deobsoletion logic and tests"

d2d357c

Read localized files at newly configured target paths

e43553e

ayshushus requested a review from mathjazz October 9, 2026 22:23

alvynabranches mentioned this pull request Oct 11, 2026

Fix P2 bug batch (14 issues, one commit each) #4604

Closed

mathjazz approved these changes Oct 12, 2026

View reviewed changes

eemeli requested changes Oct 12, 2026

View reviewed changes

ayshushus and others added 8 commits October 13, 2026 10:18

Update pontoon/sync/core/checkout.py

dcf00b5

Co-authored-by: Eemeli Aro <eemeli@gmail.com>

Rename current_paths to config_current_paths

90d182a

Extract add_unreferenced_paths

a9bcd0d

Drop get_android_locale

e10d2c6

Update pontoon/sync/tests/test_e2e.py

6d0b441

Co-authored-by: Eemeli Aro <eemeli@gmail.com>

Refactor and address clarity comments

8671301

Update tests and remove symlinks

9ba0df3

Update tests (missed dedent)

83858da

ayshushus requested a review from eemeli October 14, 2026 04:42

eemeli requested changes Oct 14, 2026

View reviewed changes

Work on new feedback and optimize

f2076a5

ayshushus requested a review from eemeli October 14, 2026 18:18

	has_current_resource = relpath(ref_path, paths.ref_root) in config_current_paths
	is_new = relpath(ref_path, paths.ref_root) not in config_current_paths

Conversation

ayshushus commented Oct 6, 2026

Uh oh!

mathjazz left a comment

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!

eemeli left a comment

Choose a reason for hiding this comment

Uh oh!

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!

mathjazz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants