From e7ef1978d201756e3152ca2f7c9b02529ea24df7 Mon Sep 17 00:00:00 2001 From: Vladimir Sitnikov Date: Mon, 25 May 2026 22:22:16 +0300 Subject: [PATCH 1/3] fix(gettext): coalesce duplicate file changes in incremental msg* tasks When poFiles aggregates overlapping providers (e.g. allSource plus feature variants reusing the main source set), Gradle can report the same physical file as both ADDED and REMOVED in a single incremental invocation. The previous loop processed events in order, so msgattrib/msgmerge/msgfmt would write the output and then immediately delete it. Downstream tasks then saw the empty output and dropped their own per-locale outputs, leaving e.g. messages_ru.java missing until --rerun-tasks was used. Introduce coalesceFileChanges() and route the three incremental tasks through it. Per file.absolutePath: ADDED/MODIFIED wins over REMOVED, and a pure-REMOVED event is suppressed when the file is still present in the resolved poFiles snapshot. Non-FILE entries are dropped centrally. Add unit tests covering the eight relevant cases. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../gradle/gettext/FileChangeCoalescing.kt | 61 +++++++ .../vlsi/gradle/gettext/MsgAttribTask.kt | 7 +- .../github/vlsi/gradle/gettext/MsgFmtTask.kt | 8 +- .../vlsi/gradle/gettext/MsgMergeTask.kt | 7 +- .../gettext/FileChangeCoalescingTest.kt | 155 ++++++++++++++++++ 5 files changed, 222 insertions(+), 16 deletions(-) create mode 100644 plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/FileChangeCoalescing.kt create mode 100644 plugins/gettext-plugin/src/test/kotlin/com/github/vlsi/gradle/gettext/FileChangeCoalescingTest.kt diff --git a/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/FileChangeCoalescing.kt b/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/FileChangeCoalescing.kt new file mode 100644 index 0000000..56870a1 --- /dev/null +++ b/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/FileChangeCoalescing.kt @@ -0,0 +1,61 @@ +/* + * Copyright 2019 Vladimir Sitnikov + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ +package com.github.vlsi.gradle.gettext + +import org.gradle.api.file.FileType +import org.gradle.work.ChangeType +import org.gradle.work.FileChange +import java.io.File + +/** + * Collapses duplicate [FileChange] events for the same physical file into a single event. + * + * Gradle may report the same file as both ADDED and REMOVED in one invocation when the + * incremental input collection contains multiple entries that resolve to the same path + * (e.g. overlapping providers from feature variants reusing the main source set). + * Acting on both events deletes the freshly produced output. See + * https://github.com/vlsi/vlsi-release-plugins for the original report. + * + * Rules applied per `file.absolutePath`: + * - non-FILE entries (directories, missing) are dropped; + * - any non-REMOVED event (ADDED/MODIFIED) wins over a REMOVED event; + * - a pure-REMOVED event is suppressed when the file is still present in + * [currentSourceFiles], i.e. the snapshot still contains it. + * + * Insertion order is preserved so processing matches the original iteration order. + */ +internal fun coalesceFileChanges( + changes: Iterable, + currentSourceFiles: Set, +): List { + val byPath = LinkedHashMap() + for (change in changes) { + if (change.fileType != FileType.FILE) { + continue + } + val key = change.file.absolutePath + val existing = byPath[key] + if (existing == null || + (existing.changeType == ChangeType.REMOVED && change.changeType != ChangeType.REMOVED) + ) { + byPath[key] = change + } + } + return byPath.values.filter { change -> + change.changeType != ChangeType.REMOVED || change.file !in currentSourceFiles + } +} diff --git a/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/MsgAttribTask.kt b/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/MsgAttribTask.kt index 16086dc..e4889a9 100644 --- a/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/MsgAttribTask.kt +++ b/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/MsgAttribTask.kt @@ -17,7 +17,6 @@ package com.github.vlsi.gradle.gettext import org.gradle.api.file.FileSystemOperations -import org.gradle.api.file.FileType import org.gradle.api.model.ObjectFactory import org.gradle.api.tasks.Input import org.gradle.api.tasks.InputFiles @@ -57,10 +56,8 @@ abstract class MsgAttribTask @Inject constructor( val outDir = outputDir.get().asFile val cmd = executable.get() val arg = args.get() - for (po in inputChanges.getFileChanges(poFiles)) { - if (po.fileType != FileType.FILE) { - continue - } + val currentPoFiles = poFiles.files + for (po in coalesceFileChanges(inputChanges.getFileChanges(poFiles), currentPoFiles)) { val outFile = File(outDir, po.file.name) if (po.changeType == ChangeType.REMOVED) { logger.debug("Removing output {}", outFile) diff --git a/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/MsgFmtTask.kt b/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/MsgFmtTask.kt index aed8f2d..a6f48bb 100644 --- a/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/MsgFmtTask.kt +++ b/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/MsgFmtTask.kt @@ -17,7 +17,6 @@ package com.github.vlsi.gradle.gettext import org.gradle.api.GradleException -import org.gradle.api.file.FileType import org.gradle.api.model.ObjectFactory import org.gradle.api.tasks.Input import org.gradle.api.tasks.InputFiles @@ -82,11 +81,8 @@ abstract class MsgFmtTask @Inject constructor( val cmd = executable.get() val arg = args.get() val targetBundle = targetBundle.get() - for (po in inputChanges.getFileChanges(poFiles)) { - if (po.fileType != FileType.FILE) { - continue - } - + val currentPoFiles = poFiles.files + for (po in coalesceFileChanges(inputChanges.getFileChanges(poFiles), currentPoFiles)) { val locale = po.file.nameWithoutExtension.toJavaLocale() val outputName = when (format.get()) { diff --git a/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/MsgMergeTask.kt b/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/MsgMergeTask.kt index 6d30003..2dd1be0 100644 --- a/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/MsgMergeTask.kt +++ b/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/MsgMergeTask.kt @@ -16,7 +16,6 @@ */ package com.github.vlsi.gradle.gettext -import org.gradle.api.file.FileType import org.gradle.api.model.ObjectFactory import org.gradle.api.tasks.Input import org.gradle.api.tasks.InputFile @@ -61,10 +60,8 @@ abstract class MsgMergeTask @Inject constructor( val cmd = executable.get() val arg = args.get() val pot = potFile.get().asFile.absolutePath - for (po in inputChanges.getFileChanges(poFiles)) { - if (po.fileType != FileType.FILE) { - continue - } + val currentPoFiles = poFiles.files + for (po in coalesceFileChanges(inputChanges.getFileChanges(poFiles), currentPoFiles)) { val outFile = File(outDir, po.file.name) if (po.changeType == ChangeType.REMOVED) { logger.debug("Removing output {}", outFile) diff --git a/plugins/gettext-plugin/src/test/kotlin/com/github/vlsi/gradle/gettext/FileChangeCoalescingTest.kt b/plugins/gettext-plugin/src/test/kotlin/com/github/vlsi/gradle/gettext/FileChangeCoalescingTest.kt new file mode 100644 index 0000000..9cd5ff9 --- /dev/null +++ b/plugins/gettext-plugin/src/test/kotlin/com/github/vlsi/gradle/gettext/FileChangeCoalescingTest.kt @@ -0,0 +1,155 @@ +/* + * Copyright 2019 Vladimir Sitnikov + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ +package com.github.vlsi.gradle.gettext + +import org.gradle.api.file.FileType +import org.gradle.work.ChangeType +import org.gradle.work.FileChange +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Test +import java.io.File + +class FileChangeCoalescingTest { + private data class FakeFileChange( + private val file: File, + private val changeType: ChangeType, + private val fileType: FileType = FileType.FILE, + private val normalizedPath: String = file.name, + ) : FileChange { + override fun getFile(): File = file + override fun getChangeType(): ChangeType = changeType + override fun getFileType(): FileType = fileType + override fun getNormalizedPath(): String = normalizedPath + } + + private fun key(change: FileChange) = change.file.absolutePath to change.changeType + + @Test + fun `ADDED followed by REMOVED for same file collapses to ADDED`() { + val po = File("/src/ru.po") + val changes = listOf( + FakeFileChange(po, ChangeType.ADDED), + FakeFileChange(po, ChangeType.REMOVED), + ) + + val result = coalesceFileChanges(changes, setOf(po)) + + assertEquals(listOf(po.absolutePath to ChangeType.ADDED), result.map(::key)) + } + + @Test + fun `REMOVED followed by ADDED for same file collapses to ADDED`() { + val po = File("/src/ru.po") + val changes = listOf( + FakeFileChange(po, ChangeType.REMOVED), + FakeFileChange(po, ChangeType.ADDED), + ) + + val result = coalesceFileChanges(changes, setOf(po)) + + assertEquals(listOf(po.absolutePath to ChangeType.ADDED), result.map(::key)) + } + + @Test + fun `MODIFIED wins over REMOVED for same file`() { + val po = File("/src/ru.po") + val changes = listOf( + FakeFileChange(po, ChangeType.MODIFIED), + FakeFileChange(po, ChangeType.REMOVED), + ) + + val result = coalesceFileChanges(changes, setOf(po)) + + assertEquals(listOf(po.absolutePath to ChangeType.MODIFIED), result.map(::key)) + } + + @Test + fun `pure REMOVED is suppressed when file still present in snapshot`() { + val po = File("/src/ru.po") + val changes = listOf(FakeFileChange(po, ChangeType.REMOVED)) + + val result = coalesceFileChanges(changes, setOf(po)) + + assertEquals(emptyList>(), result.map(::key)) + } + + @Test + fun `pure REMOVED is kept when file absent from snapshot`() { + val po = File("/src/ru.po") + val changes = listOf(FakeFileChange(po, ChangeType.REMOVED)) + + val result = coalesceFileChanges(changes, emptySet()) + + assertEquals(listOf(po.absolutePath to ChangeType.REMOVED), result.map(::key)) + } + + @Test + fun `non-FILE entries are dropped`() { + val dir = File("/src/locales") + val po = File("/src/locales/ru.po") + val changes = listOf( + FakeFileChange(dir, ChangeType.ADDED, fileType = FileType.DIRECTORY), + FakeFileChange(po, ChangeType.ADDED), + ) + + val result = coalesceFileChanges(changes, setOf(po)) + + assertEquals(listOf(po.absolutePath to ChangeType.ADDED), result.map(::key)) + } + + @Test + fun `distinct files are reported independently`() { + val ru = File("/src/ru.po") + val de = File("/src/de.po") + val es = File("/src/es.po") + val changes = listOf( + FakeFileChange(ru, ChangeType.ADDED), + FakeFileChange(de, ChangeType.MODIFIED), + FakeFileChange(es, ChangeType.REMOVED), + ) + + val result = coalesceFileChanges(changes, setOf(ru, de)) + + assertEquals( + listOf( + ru.absolutePath to ChangeType.ADDED, + de.absolutePath to ChangeType.MODIFIED, + es.absolutePath to ChangeType.REMOVED, + ), + result.map(::key), + ) + } + + @Test + fun `insertion order is preserved across distinct files`() { + val a = File("/src/a.po") + val b = File("/src/b.po") + val c = File("/src/c.po") + val changes = listOf( + FakeFileChange(b, ChangeType.ADDED), + FakeFileChange(a, ChangeType.ADDED), + FakeFileChange(c, ChangeType.ADDED), + ) + + val result = coalesceFileChanges(changes, setOf(a, b, c)) + + assertEquals( + listOf(b.absolutePath, a.absolutePath, c.absolutePath), + result.map { it.file.absolutePath }, + ) + } +} From 4f913905713feb192345b2d540bc2391308abbe4 Mon Sep 17 00:00:00 2001 From: Vladimir Sitnikov Date: Mon, 25 May 2026 22:32:33 +0300 Subject: [PATCH 2/3] refactor(gettext): use safe-call in coalesceFileChanges replacement check Per review feedback. Reorder the predicate so `existing?.changeType` carries its own null-safety instead of relying on smart-cast after an `existing == null` short-circuit (which would make `?.` redundant). Semantics unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../com/github/vlsi/gradle/gettext/FileChangeCoalescing.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/FileChangeCoalescing.kt b/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/FileChangeCoalescing.kt index 56870a1..cce8a98 100644 --- a/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/FileChangeCoalescing.kt +++ b/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/FileChangeCoalescing.kt @@ -49,8 +49,8 @@ internal fun coalesceFileChanges( } val key = change.file.absolutePath val existing = byPath[key] - if (existing == null || - (existing.changeType == ChangeType.REMOVED && change.changeType != ChangeType.REMOVED) + if (existing?.changeType == ChangeType.REMOVED && change.changeType != ChangeType.REMOVED || + existing == null ) { byPath[key] = change } From 9bb005c2dcdd5487ba21ffeb0da0f89aba8b8fb6 Mon Sep 17 00:00:00 2001 From: Vladimir Sitnikov Date: Mon, 25 May 2026 22:37:49 +0300 Subject: [PATCH 3/3] refactor(gettext): put existing == null first so smart-cast removes the safe-call Per review feedback. With `existing == null ||` on the left, Kotlin smart-casts `existing` to non-null in the right operand, so the `?.` is no longer needed. Semantics unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../com/github/vlsi/gradle/gettext/FileChangeCoalescing.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/FileChangeCoalescing.kt b/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/FileChangeCoalescing.kt index cce8a98..9fdb22a 100644 --- a/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/FileChangeCoalescing.kt +++ b/plugins/gettext-plugin/src/main/kotlin/com/github/vlsi/gradle/gettext/FileChangeCoalescing.kt @@ -49,8 +49,8 @@ internal fun coalesceFileChanges( } val key = change.file.absolutePath val existing = byPath[key] - if (existing?.changeType == ChangeType.REMOVED && change.changeType != ChangeType.REMOVED || - existing == null + if (existing == null || + existing.changeType == ChangeType.REMOVED && change.changeType != ChangeType.REMOVED ) { byPath[key] = change }