diff --git a/platform/vcs-impl/shared/src/com/intellij/platform/vcs/impl/shared/rpc/ChangeListDto.kt b/platform/vcs-impl/shared/src/com/intellij/platform/vcs/impl/shared/rpc/ChangeListDto.kt index b2e58fe635b8..bf3c1404c1f7 100644 --- a/platform/vcs-impl/shared/src/com/intellij/platform/vcs/impl/shared/rpc/ChangeListDto.kt +++ b/platform/vcs-impl/shared/src/com/intellij/platform/vcs/impl/shared/rpc/ChangeListDto.kt @@ -39,42 +39,80 @@ data class ChangeListDto( } setDefault(isDefault) setId(id) - }.setChanges(changes.map { ChangeListChange(it.change, name, id) }).build() + }.setChanges(changes.map { it.toChange(name, id) }).build() } } +/** + * A serialized [Change] belonging to a [ChangeListDto]. + * + * The concrete subtype encodes how the backend represents the change, which is what [toChange] reconstructs. + * This drives [ChangeId.getId] to produce the same value on both frontend and backend so that commit inclusion + * round-trips correctly (see IJPL-246371): + * - [ChangelistChangeDto] for VCSes supporting partial changelists (e.g. Git), wrapped into a [ChangeListChange]; + * - [PlainChangeDto] for the others (e.g. Mercurial, SVN), kept as a plain [Change]. + */ @Serializable @ApiStatus.Internal -data class ChangeDto( - private val beforeRevision: ContentRevisionDto?, - private val afterRevision: ContentRevisionDto?, - private val fileStatusId: String, - @Transient private val localValue: Change? = null, -) { - val change: Change by lazy { - if (localValue != null) return@lazy localValue +sealed class ChangeDto { + protected abstract val beforeRevision: ContentRevisionDto? + protected abstract val afterRevision: ContentRevisionDto? + protected abstract val fileStatusId: String + protected abstract val localValue: Change? - val fileStatus = PLATFORM_FILE_STATUSES[fileStatusId] - Change(beforeRevision?.contentRevision, afterRevision?.contentRevision, fileStatus) + /** The plain [Change] carried by this DTO, without any change list wrapping. */ + val change: Change by lazy { + localValue ?: Change(beforeRevision?.contentRevision, afterRevision?.contentRevision, PLATFORM_FILE_STATUSES[fileStatusId]) } + /** Reconstructs the change the way the backend represents it inside the change list [listName]/[listId]. */ + abstract fun toChange(listName: @Nls String, listId: @NonNls String): Change + companion object { - fun toDto(change: Change): ChangeDto = ChangeDto( - beforeRevision = change.beforeRevision?.toDto(), - afterRevision = change.afterRevision?.toDto(), - fileStatusId = change.fileStatus.id, - localValue = change, - ) + fun toDto(change: Change): ChangeDto { + val beforeRevision = change.beforeRevision?.toDto() + val afterRevision = change.afterRevision?.toDto() + val fileStatusId = change.fileStatus.id + return if (change is ChangeListChange) { + ChangelistChangeDto(beforeRevision, afterRevision, fileStatusId, localValue = change) + } + else { + PlainChangeDto(beforeRevision, afterRevision, fileStatusId, localValue = change) + } + } private fun ContentRevision.toDto() = ContentRevisionDto( revisionString = revisionNumber.asString(), filePath = FilePathDto.toDto(file), localValue = this, ) - } } +/** A change from a VCS without partial changelist support; reconstructed as a plain [Change]. */ +@Serializable +@ApiStatus.Internal +data class PlainChangeDto( + override val beforeRevision: ContentRevisionDto?, + override val afterRevision: ContentRevisionDto?, + override val fileStatusId: String, + @Transient override val localValue: Change? = null, +) : ChangeDto() { + override fun toChange(listName: String, listId: String): Change = change +} + +/** A change from a VCS with partial changelist support; reconstructed as a [ChangeListChange]. */ +@Serializable +@ApiStatus.Internal +data class ChangelistChangeDto( + override val beforeRevision: ContentRevisionDto?, + override val afterRevision: ContentRevisionDto?, + override val fileStatusId: String, + @Transient override val localValue: ChangeListChange? = null, +) : ChangeDto() { + override fun toChange(listName: String, listId: String): ChangeListChange = ChangeListChange(change, listName, listId) +} + private val PLATFORM_FILE_STATUSES: Map by lazy { FileStatusFactory.getInstance().globalFileStatuses.associateBy { it.id } } \ No newline at end of file diff --git a/platform/vcs-tests/testSrc/com/intellij/openapi/vcs/changes/ChangeListDtoChangeIdRoundTripTest.kt b/platform/vcs-tests/testSrc/com/intellij/openapi/vcs/changes/ChangeListDtoChangeIdRoundTripTest.kt new file mode 100644 index 000000000000..9e41ff97cc00 --- /dev/null +++ b/platform/vcs-tests/testSrc/com/intellij/openapi/vcs/changes/ChangeListDtoChangeIdRoundTripTest.kt @@ -0,0 +1,64 @@ +// Copyright 2000-2026 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. +package com.intellij.openapi.vcs.changes + +import com.intellij.openapi.vcs.LocalFilePath +import com.intellij.platform.vcs.impl.shared.rpc.ChangeDto +import com.intellij.platform.vcs.impl.shared.rpc.ChangeId +import com.intellij.platform.vcs.impl.shared.rpc.ChangeListDto +import com.intellij.testFramework.fixtures.BasePlatformTestCase + +/** + * Verifies that the [ChangeId] a change gets on the frontend (after being reconstructed from [ChangeListDto]) matches + * the [ChangeId] the backend keys its cache with, for VCSes both with and without partial changelist support. + * + * Regression test: previously [ChangeListDto] unconditionally wrapped every change into [ChangeListChange], producing + * a `ChangeListChangeId`. For a VCS without partial changelist support (e.g. Mercurial, SVN) the backend keeps the + * plain [Change] and thus keys the cache by a `NonChangeListChangeId`, so the ids did not match, the backend inclusion + * stayed empty, and the Commit button reported "Select files to commit". + * + * @see ChangeListDto.getChangeList (frontend reconstruction / wrapping decision) + * @see com.intellij.vcs.changes.ChangesViewChangeIdProvider (backend cache, keyed by [ChangeId.getId]) + */ +internal class ChangeListDtoChangeIdRoundTripTest : BasePlatformTestCase() { + fun `test ChangeId matches for VCS without partial changelist support`() { + assertFrontendChangeIdMatchesBackend(partialChangelistsSupported = false) + } + + fun `test ChangeId matches for VCS with partial changelist support`() { + assertFrontendChangeIdMatchesBackend(partialChangelistsSupported = true) + } + + private fun assertFrontendChangeIdMatchesBackend(partialChangelistsSupported: Boolean) { + val listName = "Test List" + val listId = "test-list-id" + val filePath = LocalFilePath("/root/file.txt", false) + val change = Change( + SimpleContentRevision("base", filePath, "1"), + SimpleContentRevision("content", filePath, "2"), + ) + + // How the backend represents the change and keys the ChangeId cache, see ChangeListWorker.getChangesMapping: + // it wraps into a ChangeListChange only for VCSes that support partial changelists. + val backendChange: Change = + if (partialChangelistsSupported) ChangeListChange(change, listName, listId) else change + val backendId = ChangeId.getId(backendChange) + + // How the frontend reconstructs the change from the serialized DTO, see ChangeListDto.getChangeList. + // The backend serializes its own representation of the change (backendChange), so ChangeDto.toDto picks the + // matching DTO subtype based on whether the change is wrapped into a ChangeListChange. + val dto = ChangeListDto( + name = listName, + comment = null, + id = listId, + isDefault = false, + changes = listOf(ChangeDto.toDto(backendChange)), + ) + val frontendChange = dto.getChangeList(project).changes.single() + val frontendId = ChangeId.getId(frontendChange) + + assertEquals( + "Frontend and backend must compute the same ChangeId (partialChangelistsSupported=$partialChangelistsSupported)", + backendId, frontendId, + ) + } +}